Attention is currently required from: neels, pespin, fixeria.
Hello Jenkins Builder, laforge, pespin, fixeria,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-bsc/+/31578
to look at the new patch set (#10).
Change subject: pcu_sock: use struct to transfer IMMEDIATE ASSIGNMENT for PCH
......................................................................
pcu_sock: use struct to transfer IMMEDIATE ASSIGNMENT for PCH
When the IMMEDIATE ASSIGNMENT is sent from the PCU to the BSC using the
"direct TLLI" method, the TLLI (and the last three digits of the IMSI)
is prepended to the MAC block. Currently we are taking the fields apart
manually using offsets. The code for this is difficult to read and the
method is error prone. Lets define a struct that we can just overlay
to access the fields directly. Lets also transfer the full IMSI.
Change-Id: Id6acbd243adf26169e5e8319dd66bb68dd6a3c22
Related: OS#5198
---
M include/osmocom/bsc/pcuif_proto.h
M src/osmo-bsc/pcu_sock.c
2 files changed, 38 insertions(+), 10 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/78/31578/10
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31578
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Id6acbd243adf26169e5e8319dd66bb68dd6a3c22
Gerrit-Change-Number: 31578
Gerrit-PatchSet: 10
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newpatchset
dexter has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31673 )
Change subject: pcu_sock: get rid of leaking message buffer
......................................................................
pcu_sock: get rid of leaking message buffer
When a data request is received from the PCU, some of the switch cases
allocate a message buffer but the message buffer is only used to pass
its data and length to other functions. The message buffer itself is not
passed anywhere and it is also not freed. Lets get rid of the message
buffer and avoid unnecessary memcopy calls.
Related: OS#5198
Change-Id: Ibfaae177585a4d42d797b6bbd90e402641620140
---
M src/osmo-bsc/pcu_sock.c
1 file changed, 19 insertions(+), 23 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/73/31673/1
diff --git a/src/osmo-bsc/pcu_sock.c b/src/osmo-bsc/pcu_sock.c
index 3d8c254..fa4193f 100644
--- a/src/osmo-bsc/pcu_sock.c
+++ b/src/osmo-bsc/pcu_sock.c
@@ -532,7 +532,6 @@
static int pcu_rx_data_req(struct gsm_bts *bts, uint8_t msg_type,
struct gsm_pcu_if_data *data_req)
{
- struct msgb *msg;
uint32_t tlli = -1;
uint8_t pag_grp;
int rc = 0;
@@ -549,18 +548,8 @@
pcu_rx_rr_paging(bts, pag_grp, data_req->data+3);
break;
case PCU_IF_SAPI_AGCH:
- msg = msgb_alloc(data_req->len, "pcu_agch");
- if (!msg) {
- rc = -ENOMEM;
- break;
- }
- msg->l3h = msgb_put(msg, data_req->len);
- memcpy(msg->l3h, data_req->data, data_req->len);
-
- if (rsl_imm_assign_cmd(bts, msg->len, msg->data)) {
- msgb_free(msg);
+ if (rsl_imm_assign_cmd(bts, data_req->len, data_req->data))
rc = -EIO;
- }
break;
case PCU_IF_SAPI_PCH_DT:
/* DT = direct TLLI. A tlli is prefixed so that the BSC/BTS can confirm the sending of the downlink
@@ -580,29 +569,20 @@
LOGP(DPCU, LOGL_DEBUG, "PCU Sends immediate assignment via PCH (tlli=0x%08x, pag_grp=0x%02x)\n",
tlli, pag_grp);
- msg = msgb_alloc(data_req->len - 7, "pcu_pch");
- if (!msg) {
- rc = -ENOMEM;
- break;
- }
- msg->l3h = msgb_put(msg, data_req->len - 7);
- memcpy(msg->l3h, data_req->data + 7, data_req->len - 7);
/* NOTE: Sending an IMMEDIATE ASSIGNMENT via PCH became necessary with GPRS in order to be able to
* assign downlink TBFs directly through the paging channel. However, this method never became part
* of the RSL specs. This means that each BTS vendor has to come up with a proprietary method. At
* the moment we only support Ericsson RBS here. */
if (bts->type == GSM_BTS_TYPE_RBS2000) {
- rc = rsl_ericsson_imm_assign_cmd(bts, tlli, msg->len, msg->data, pag_grp);
+ rc = rsl_ericsson_imm_assign_cmd(bts, tlli, data_req->len - 7, data_req->data + 7, pag_grp);
} else {
LOGP(DPCU, LOGL_ERROR, "BTS model does not support sending immediate assignment via PCH!\n");
rc = -ENOTSUP;
}
- if (rc) {
- msgb_free(msg);
+ if (rc)
rc = -EIO;
- }
break;
default:
LOGP(DPCU, LOGL_ERROR, "Received PCU data request with "
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31673
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Ibfaae177585a4d42d797b6bbd90e402641620140
Gerrit-Change-Number: 31673
Gerrit-PatchSet: 1
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: newchange
Attention is currently required from: neels, pespin, fixeria.
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31578 )
Change subject: pcu_sock: use struct to transfer IMMEDIATE ASSIGNMENT for PCH
......................................................................
Patch Set 10:
(3 comments)
Commit Message:
https://gerrit.osmocom.org/c/osmo-bsc/+/31578/comment/20c11cd3_bb8716ad
PS8, Line 10: "direct TLLI" method, the TLLI (and the paging group) is premended to
> probably "prepended"
Done
File src/osmo-bsc/pcu_sock.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31578/comment/c3c0c5a4_605ce3e7
PS8, Line 581: msg = msgb_alloc(sizeof(pch_dt->data), "pcu_pch");
> this is just moving code from a previous patch, but in fact this msgb is leaked! It should have been […]
Thanks. I didn't see this while editing the code. Thats a really nasty bug.
https://gerrit.osmocom.org/c/osmo-bsc/+/31578/comment/eb263bad_699a1b8d
PS8, Line 595: msg->len, msg->data
> ... […]
Done
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31578
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Id6acbd243adf26169e5e8319dd66bb68dd6a3c22
Gerrit-Change-Number: 31578
Gerrit-PatchSet: 10
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 06 Mar 2023 10:44:45 +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
Attention is currently required from: neels, fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31669 )
Change subject: gsm0502: add burst length definitions from chapter 5.2
......................................................................
Patch Set 1:
(1 comment)
File include/osmocom/gsm/gsm0502.h:
https://gerrit.osmocom.org/c/libosmocore/+/31669/comment/2fe531d7_4b5f9115
PS1, Line 36: #define GSM_NBITS_NB_GMSK_PAYLOAD 2 * 58
> > I think shitfting operations take precedence over multiplication? […]
ok, still if you use this after a division you'll have problems:
45 / GSM_NBITS_NB_GMSK_PAYLOAD = 45 / 2 * 58 = 22 * 58
but in this case you'd actually want 45 / (2 * 58) = 45 / 116
But tbh, simply don't care about details and put parenthesis as a rule of thumb.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31669
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I1c38ccc2b64ba9046bb3b1221d99cc55ec493802
Gerrit-Change-Number: 31669
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 06 Mar 2023 10:14:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
osmith has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31551 )
Change subject: assignment_fsm: chan mode check: support CSD
......................................................................
assignment_fsm: chan mode check: support CSD
Replace check_requires_voice_stream, which used to iterate over
ch_mode_rate_list and verify that all entries are either for speech or
signalling. Instead verify in check_chan_mode_rate_against_ch_indctr,
that all entries of ch_mode_rate_list have a chan_mode that matches the
ch_indctr (data, speech, signalling; called "speech / data indicator" in
3GPP TS 48.008 § 3.2.2.11).
This ensures that all of them are either data, speech or signalling and
not mixed.
Related: OS#4393
Change-Id: Iee5cbfee84d7f2ad59ee2d5a19891a2b59bbafff
---
M include/osmocom/bsc/gsm_data.h
M src/osmo-bsc/assignment_fsm.c
M src/osmo-bsc/osmo_bsc_bssap.c
3 files changed, 49 insertions(+), 32 deletions(-)
Approvals:
pespin: Looks good to me, but someone else must approve
laforge: Looks good to me, but someone else must approve
Jenkins Builder: Verified
osmith: Looks good to me, approved
diff --git a/include/osmocom/bsc/gsm_data.h b/include/osmocom/bsc/gsm_data.h
index cf66a04..d4615b2 100644
--- a/include/osmocom/bsc/gsm_data.h
+++ b/include/osmocom/bsc/gsm_data.h
@@ -163,6 +163,9 @@
/* The TSC to use if 'use' is true, otherwise automatically determine the TSC value to use. Valid range is 0 to
* 7, as described in 3GPP TS 45.002. */
struct optional_val tsc;
+
+ /* The "speech / data indicator" from 3GPP TS 48.008 § 3.2.2.11 Channel Type (speech/data/signalling). */
+ enum gsm0808_chan_indicator ch_indctr;
};
/* State of an ongoing Assignment, while the assignment_fsm is still busy. This serves as state separation to keep the
diff --git a/src/osmo-bsc/assignment_fsm.c b/src/osmo-bsc/assignment_fsm.c
index 7f6a7ea..6dc09c6 100644
--- a/src/osmo-bsc/assignment_fsm.c
+++ b/src/osmo-bsc/assignment_fsm.c
@@ -370,62 +370,53 @@
OSMO_ASSERT(osmo_fsm_register(&assignment_fsm) == 0);
}
-static int check_requires_voice(bool *requires_voice, enum gsm48_chan_mode chan_mode)
+static int chan_mode_to_ch_indctr(enum gsm48_chan_mode chan_mode)
{
- *requires_voice = false;
-
switch (gsm48_chan_mode_to_non_vamos(chan_mode)) {
+ case GSM48_CMODE_DATA_14k5:
+ case GSM48_CMODE_DATA_12k0:
+ case GSM48_CMODE_DATA_6k0:
+ case GSM48_CMODE_DATA_3k6:
+ return GSM0808_CHAN_DATA;
case GSM48_CMODE_SPEECH_V1:
case GSM48_CMODE_SPEECH_EFR:
case GSM48_CMODE_SPEECH_AMR:
- *requires_voice = true;
- break;
+ return GSM0808_CHAN_SPEECH;
case GSM48_CMODE_SIGN:
- *requires_voice = false;
- break;
+ return GSM0808_CHAN_SIGN;
default:
return -EINVAL;
}
-
- return 0;
}
-/* Check if the incoming assignment request requires a voice stream or not,
- * we will look at the preferred and the alternate channel mode and also make
- * sure that both are consistent. */
-static int check_requires_voice_stream(struct gsm_subscriber_connection *conn)
+/* Check if the incoming assignment request has a channel mode that is
+ * inconsistent with ch_indctr, e.g. GSM48_CMODE_DATA_14k5 and
+ * GSM0808_CHAN_SPEECH */
+static int check_chan_mode_rate_against_ch_indctr(struct gsm_subscriber_connection *conn)
{
- bool requires_voice_pref = false, requires_voice_alt;
struct assignment_request *req = &conn->assignment.req;
struct osmo_fsm_inst *fi = conn->fi;
- int i, rc;
-
- /* When the assignment request indicates that there is an alternate
- * rate available (e.g. "Full or Half rate channel, Half rate
- * preferred..."), then both must be either voice or either signalling,
- * a mismatch is not permitted */
+ int i;
+ uint8_t ch_indctr;
for (i = 0; i < req->n_ch_mode_rate; i++) {
- rc = check_requires_voice(&requires_voice_alt, req->ch_mode_rate_list[i].chan_mode);
- if (rc < 0) {
+ ch_indctr = chan_mode_to_ch_indctr(req->ch_mode_rate_list[i].chan_mode);
+ if (ch_indctr < 0) {
assignment_fail(GSM0808_CAUSE_REQ_CODEC_TYPE_OR_CONFIG_NOT_SUPP,
"Channel mode not supported (prev level %d): %s", i,
gsm48_chan_mode_name(req->ch_mode_rate_list[i].chan_mode));
return -EINVAL;
}
- if (i==0)
- requires_voice_pref = requires_voice_alt;
- else if (requires_voice_alt != requires_voice_pref) {
+ if (ch_indctr != req->ch_indctr) {
assignment_fail(GSM0808_CAUSE_REQ_CODEC_TYPE_OR_CONFIG_NOT_SUPP,
- "Requested a mix of Signalling and non-Signalling channel modes: %s != %s",
- gsm48_chan_mode_name(req->ch_mode_rate_list[0].chan_mode),
- gsm48_chan_mode_name(req->ch_mode_rate_list[i].chan_mode));
+ "Channel mode %s has ch_indctr %d, channel type has ch_indctr %d",
+ gsm48_chan_mode_name(req->ch_mode_rate_list[i].chan_mode),
+ ch_indctr, req->ch_indctr);
return -EINVAL;
}
}
- conn->assignment.requires_voice_stream = requires_voice_pref;
return 0;
}
@@ -471,6 +462,8 @@
.present = (tsc >= 0),
.val = tsc,
},
+
+ .ch_indctr = chan_mode_to_ch_indctr(lchan->current_ch_mode_rate.chan_mode),
};
if (to_lchan)
@@ -529,10 +522,9 @@
assignment_count(CTR_ASSIGNMENT_ATTEMPTED);
- /* Check if we need a voice stream. If yes, set the appropriate struct
- * members in conn */
- if (check_requires_voice_stream(conn) < 0)
+ if (check_chan_mode_rate_against_ch_indctr(conn) < 0)
return;
+ conn->assignment.requires_voice_stream = (req->ch_indctr != GSM0808_CHAN_SIGN);
if (!req->target_lchan && reuse_existing_lchan(conn)) {
/* The already existing lchan is suitable for this mode */
diff --git a/src/osmo-bsc/osmo_bsc_bssap.c b/src/osmo-bsc/osmo_bsc_bssap.c
index e38a975..916e3b8 100644
--- a/src/osmo-bsc/osmo_bsc_bssap.c
+++ b/src/osmo-bsc/osmo_bsc_bssap.c
@@ -1136,6 +1136,8 @@
goto reject;
}
+ req.ch_indctr = ct.ch_indctr;
+
return osmo_fsm_inst_dispatch(conn->fi, GSCON_EV_ASSIGNMENT_START, &req);
reject:
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31551
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Iee5cbfee84d7f2ad59ee2d5a19891a2b59bbafff
Gerrit-Change-Number: 31551
Gerrit-PatchSet: 5
Gerrit-Owner: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
osmith has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31550 )
(
2 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: bssmap_handle_ass_req_tp_codec_list: tweak log msg
......................................................................
bssmap_handle_ass_req_tp_codec_list: tweak log msg
Remove "speech mode" from the log message, as the log message is
relevant for CSD too. According to 3GPP TS 48.008 § 3.2.1.1 note 13 the
IE shall be included for AoIP unless channel type is signalling.
Related: OS#4393
Change-Id: Idfab0b7f6e97a6b67d140f967ddfe9b29818586e
---
M src/osmo-bsc/osmo_bsc_bssap.c
1 file changed, 15 insertions(+), 1 deletion(-)
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/osmo_bsc_bssap.c b/src/osmo-bsc/osmo_bsc_bssap.c
index 59d5aa5..e38a975 100644
--- a/src/osmo-bsc/osmo_bsc_bssap.c
+++ b/src/osmo-bsc/osmo_bsc_bssap.c
@@ -977,7 +977,7 @@
}
if (aoip && !conn->codec_list.len) {
- LOGP(DMSC, LOGL_ERROR, "%s: AoIP speech mode Assignment Request:"
+ LOGP(DMSC, LOGL_ERROR, "%s: AoIP Assignment Request:"
" Missing or empty Speech Codec List IE\n", bsc_subscr_name(conn->bsub));
*cause = GSM0808_CAUSE_INFORMATION_ELEMENT_OR_FIELD_MISSING;
return -1;
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31550
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Idfab0b7f6e97a6b67d140f967ddfe9b29818586e
Gerrit-Change-Number: 31550
Gerrit-PatchSet: 5
Gerrit-Owner: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged