dexter has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31618 )
Change subject: pcu_sock: handle multiple BTSs with one BSC co-located PCU (in theory)
......................................................................
pcu_sock: handle multiple BTSs with one BSC co-located PCU (in theory)
The current PCU implementation has never been tested with multiple BTS
attached to it. This is due to the fact that it has been used
exclusively in an BTS co-located setup where naturally only one BTS is
present. The PCU sock protocol supports multiple BTSs in theory and we
should handle this correctly. We also should add a check and print an
error message in case two or more BTSs with BSC co-located PCU (Ericsson
RBS) are configured.
Related: OS#5198
Change-Id: I0b42c2c130106f6ffca2dd08d079e1a7bda41f0b
---
M src/osmo-bsc/pcu_sock.c
1 file changed, 114 insertions(+), 58 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/18/31618/1
diff --git a/src/osmo-bsc/pcu_sock.c b/src/osmo-bsc/pcu_sock.c
index f136e21..933ff5f 100644
--- a/src/osmo-bsc/pcu_sock.c
+++ b/src/osmo-bsc/pcu_sock.c
@@ -361,11 +361,11 @@
if (pcu_connected(bts)) {
/* In cases where the CCU is connected via an E1 line, we transmit the connection parameters for the
* PDCH before we announce the other BTS related parameters. At the moment Ericsson RBS is the only
- * E1 BTS we support. */
- if (is_ericsson_bts(bts))
+ * E1 BTS we support and also the only BTS we support with a BSC co-located-pcu */
+ if (is_ericsson_bts(bts)) {
pcu_tx_e1_ccu_ind(bts);
-
- pcu_tx_info_ind(bts);
+ pcu_tx_info_ind(bts);
+ }
}
}
@@ -713,8 +713,9 @@
int rc = 0;
struct gsm_bts *bts;
- /* FIXME: allow multiple BTS */
- bts = llist_entry(net->bts_list.next, struct gsm_bts, list);
+ bts = gsm_bts_num(net, pcu_prim->bts_nr);
+ if (!bts)
+ return -EINVAL;
switch (msg_type) {
case PCU_IF_MSG_DATA_REQ:
@@ -766,25 +767,11 @@
return 0;
}
-static void pcu_sock_close(struct pcu_sock_state *state)
+static void pdch_deact_bts(struct gsm_bts *bts)
{
- struct osmo_fd *bfd = &state->conn_bfd;
- struct gsm_bts *bts;
struct gsm_bts_trx *trx;
int j;
- /* FIXME: allow multiple BTS */
- bts = llist_entry(state->net->bts_list.next, struct gsm_bts, list);
-
- LOGP(DPCU, LOGL_NOTICE, "PCU socket has LOST connection\n");
-
- close(bfd->fd);
- bfd->fd = -1;
- osmo_fd_unregister(bfd);
-
- /* re-enable the generation of ACCEPT for new connections */
- osmo_fd_read_enable(&state->listen_bfd);
-
#if 0
/* remove si13, ... */
bts->si_valid &= ~(1 << SYSINFO_TYPE_13);
@@ -806,6 +793,27 @@
}
}
}
+}
+
+static void pcu_sock_close(struct pcu_sock_state *state)
+{
+ struct osmo_fd *bfd = &state->conn_bfd;
+ struct gsm_bts *bts;
+
+ LOGP(DPCU, LOGL_NOTICE, "PCU socket has LOST connection\n");
+
+ close(bfd->fd);
+ bfd->fd = -1;
+ osmo_fd_unregister(bfd);
+
+ /* re-enable the generation of ACCEPT for new connections */
+ osmo_fd_read_enable(&state->listen_bfd);
+
+ /* Disable all PDCHs on all BTSs that are served by the PCU */
+ llist_for_each_entry(bts, &state->net->bts_list, list) {
+ if (is_ericsson_bts(bts))
+ pdch_deact_bts(bts);
+ }
/* flush the queue */
while (!llist_empty(&state->upqueue)) {
@@ -923,46 +931,10 @@
return rc;
}
-/* accept connection coming from PCU */
-static int pcu_sock_accept(struct osmo_fd *bfd, unsigned int flags)
+static void pdch_act_bts(struct gsm_bts *bts)
{
- struct pcu_sock_state *state = (struct pcu_sock_state *)bfd->data;
- struct osmo_fd *conn_bfd = &state->conn_bfd;
- struct sockaddr_un un_addr;
- struct gsm_bts *bts;
struct gsm_bts_trx *trx;
int j;
- socklen_t len;
- int rc;
-
- /* FIXME: allow multiple BTS */
- bts = llist_entry(state->net->bts_list.next, struct gsm_bts, list);
-
- len = sizeof(un_addr);
- rc = accept(bfd->fd, (struct sockaddr *) &un_addr, &len);
- if (rc < 0) {
- LOG_BTS(bts, DPCU, LOGL_ERROR, "Failed to accept a new connection\n");
- return -1;
- }
-
- if (conn_bfd->fd >= 0) {
- LOG_BTS(bts, DPCU, LOGL_NOTICE, "PCU connects but we already have another active connection ?!?\n");
- /* We already have one PCU connected, this is all we support */
- osmo_fd_read_disable(&state->listen_bfd);
- close(rc);
- return 0;
- }
-
- osmo_fd_setup(conn_bfd, rc, OSMO_FD_READ, pcu_sock_cb, state, 0);
-
- if (osmo_fd_register(conn_bfd) != 0) {
- LOG_BTS(bts, DPCU, LOGL_ERROR, "Failed to register new connection fd\n");
- close(conn_bfd->fd);
- conn_bfd->fd = -1;
- return -1;
- }
-
- LOG_BTS(bts, DPCU, LOGL_NOTICE, "PCU socket connected to external PCU\n");
/* activate PDCH */
llist_for_each_entry(trx, &bts->trx_list, list) {
@@ -975,6 +947,72 @@
}
}
}
+}
+
+/* Check if there are multiple BTSs configured that would require a BSC co-located PCU */
+static bool check_for_multiple_bts(struct llist_head *bts_list)
+{
+ unsigned int bts_count = 0;
+ struct gsm_bts *bts;
+
+ llist_for_each_entry(bts, bts_list, list) {
+ if (is_ericsson_bts(bts))
+ bts_count++;
+ }
+
+ if (bts_count > 1)
+ return true;
+ return false;
+}
+
+
+/* accept connection coming from PCU */
+static int pcu_sock_accept(struct osmo_fd *bfd, unsigned int flags)
+{
+ struct pcu_sock_state *state = (struct pcu_sock_state *)bfd->data;
+ struct osmo_fd *conn_bfd = &state->conn_bfd;
+ struct sockaddr_un un_addr;
+ struct gsm_bts *bts;
+ socklen_t len;
+ int rc;
+
+ /* FIXME: allow multiple BTS in OsmoPCU */
+ if (check_for_multiple_bts(&state->net->bts_list)) {
+ LOGP(DPCU, LOGL_ERROR, "This version of OsmoBSC can only handle one (Ericsson RBS) BTS with BSC co-located PCU\n");
+ LOGP(DPCU, LOGL_ERROR, "Failed to accept a new connection\n");
+ }
+
+ len = sizeof(un_addr);
+ rc = accept(bfd->fd, (struct sockaddr *) &un_addr, &len);
+ if (rc < 0) {
+ LOGP(DPCU, LOGL_ERROR, "Failed to accept a new connection\n");
+ return -1;
+ }
+
+ if (conn_bfd->fd >= 0) {
+ LOGP(DPCU, LOGL_NOTICE, "PCU connects but we already have another active connection ?!?\n");
+ /* We already have one PCU connected, this is all we support */
+ osmo_fd_read_disable(&state->listen_bfd);
+ close(rc);
+ return 0;
+ }
+
+ osmo_fd_setup(conn_bfd, rc, OSMO_FD_READ, pcu_sock_cb, state, 0);
+
+ if (osmo_fd_register(conn_bfd) != 0) {
+ LOGP(DPCU, LOGL_ERROR, "Failed to register new connection fd\n");
+ close(conn_bfd->fd);
+ conn_bfd->fd = -1;
+ return -1;
+ }
+
+ LOGP(DPCU, LOGL_NOTICE, "PCU socket connected to external PCU\n");
+
+ /* Activate all PDCHs on all BTSs that are served by the PCU */
+ llist_for_each_entry(bts, &state->net->bts_list, list) {
+ if (is_ericsson_bts(bts))
+ pdch_act_bts(bts);
+ }
return 0;
}
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31618
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I0b42c2c130106f6ffca2dd08d079e1a7bda41f0b
Gerrit-Change-Number: 31618
Gerrit-PatchSet: 1
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: newchange
Attention is currently required from: neels, pespin, dexter.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/29475 )
Change subject: fix gsm0808_sc_cfg <-> gsm48_mr_cfg conversion
......................................................................
Patch Set 4:
(1 comment)
File include/osmocom/gsm/protocol/gsm_04_08.h:
https://gerrit.osmocom.org/c/libosmocore/+/29475/comment/65ec494f_715300db
PS2, Line 627: return ((uint8_t *)cfg)[1];
> gsm48_multi_rate_conf has two uint8_t members, AFAICT adding a union would require adding another ex […]
You don't really need to name the union itself, it can be anonymous (just like structures). The problem is that bit-fields are treated as individual fields in a union, so doing what Pau suggested:
```
/* I removed the LE variant for the sake of readability
* see https://people.osmocom.org/fixeria/union_bfield_test.c */
struct gsm48_multi_rate_conf {
uint8_t ver:3, nscb:1, icmi:1, spare:1, smod:2;
union {
uint8_t m12_2:1, m10_2:1, m7_95:1, m7_40:1,
m6_70:1, m5_90:1, m5_15:1, m4_75:1;
uint8_t mode;
};
} __attribute__((packed));
```
will result in the following behavior:
```
struct gsm48_multi_rate_conf mrc = { 0 };
// mrc.mode = 0x00
// m4_75=0, m5_15=0, m5_90=0, m6_70=0, m7_40=0, m7_95=0, m10_2=0, m12_2=0
mrc.m4_75 = 1;
// mrc.mode = 0x01
// m4_75=1, m5_15=1, m5_90=1, m6_70=1, m7_40=1, m7_95=1, m10_2=1, m12_2=1
mrc.m10_2 = 0;
// mrc.mode = 0x00
// m4_75=0, m5_15=0, m5_90=0, m6_70=0, m7_40=0, m7_95=0, m10_2=0, m12_2=0
mrc.m12_2 = 1;
// mrc.mode = 0x01
// m4_75=1, m5_15=1, m5_90=1, m6_70=1, m7_40=1, m7_95=1, m10_2=1, m12_2=1
```
Definitely not what we want. Here is an alternative approach:
```
union gsm48_multi_rate_conf_u {
struct gsm48_multi_rate_conf mrc;
struct {
uint8_t hdr;
uint8_t mode;
};
};
```
This should work as expected, but I am fine with the current approach. Keep it as it is.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/29475
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I900fda192742fa8f6dd54e9131ef1704b14cc41a
Gerrit-Change-Number: 29475
Gerrit-PatchSet: 4
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 01 Mar 2023 11:29:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
dexter has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31499 )
(
5 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: pcu_sock: print OML alerts from PCU
......................................................................
pcu_sock: print OML alerts from PCU
The PCU is able to send OML alerts via the BTS to the BSC. When the PCU
operates in co-location to the BSC we just print the alerts in the log
directly
Change-Id: Id32553556356c2affe32e47ae1c3ae6a514efdce
Related: OS#5198
---
M src/osmo-bsc/pcu_sock.c
1 file changed, 17 insertions(+), 0 deletions(-)
Approvals:
pespin: Looks good to me, but someone else must approve
laforge: Looks good to me, approved
Jenkins Builder: Verified
diff --git a/src/osmo-bsc/pcu_sock.c b/src/osmo-bsc/pcu_sock.c
index add968a..a0c9827 100644
--- a/src/osmo-bsc/pcu_sock.c
+++ b/src/osmo-bsc/pcu_sock.c
@@ -686,6 +686,9 @@
if (rc < 0)
return -EINVAL;
break;
+ case PCU_OML_ALERT:
+ LOG_BTS(bts, DPCU, LOGL_ERROR, "PCU external alarm: %s\n", txt->text);
+ break;
default:
LOGP(DPCU, LOGL_ERROR, "Unknown TXT_IND type %u received\n",
txt->type);
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31499
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Id32553556356c2affe32e47ae1c3ae6a514efdce
Gerrit-Change-Number: 31499
Gerrit-PatchSet: 6
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
Attention is currently required from: iedemam.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31617 )
Change subject: utils: fix incorrect string checks in meas_db_insert()
......................................................................
Patch Set 1:
(2 comments)
Patchset:
PS1:
> Good catch, apologies for the miss. […]
No worries, it's absolutely normal and we all make mistakes.
File src/utils/meas_db.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31617/comment/8b5c6a0b_ab216ab6
PS1, Line 97: if (mfm->imsi[0] != '\0')
> The original checks before my refactor were using strlen(). […]
Let's see what the others think. If people find `strlen()` more readable, I can amend the patch. The compiler would optimize `strlen()` out anyway.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31617
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I8a41078070119bc22d594c0dfff5d98b5d16f970
Gerrit-Change-Number: 31617
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: iedemam <michael(a)kapsulate.com>
Gerrit-Attention: iedemam <michael(a)kapsulate.com>
Gerrit-Comment-Date: Wed, 01 Mar 2023 09:57:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: iedemam <michael(a)kapsulate.com>
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
iedemam has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31617 )
Change subject: utils: fix incorrect string checks in meas_db_insert()
......................................................................
Patch Set 1: Code-Review+1
(2 comments)
Patchset:
PS1:
Good catch, apologies for the miss.
This looks ok to me, only a note about how this check was performed before.
File src/utils/meas_db.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31617/comment/50de4a0d_fcfccde0
PS1, Line 97: if (mfm->imsi[0] != '\0')
The original checks before my refactor were using strlen(). Maybe that's cleaner?
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31617
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I8a41078070119bc22d594c0dfff5d98b5d16f970
Gerrit-Change-Number: 31617
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: iedemam <michael(a)kapsulate.com>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 01 Mar 2023 09:28:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment