June 2023 - gerrit-log - lists.osmocom.org

[M] Change in osmo-pcu[master]: Use always RFN when handling RACH indications
by pespin 02 Jun '23

02 Jun '23

pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/33119 ) Change subject: Use always RFN when handling RACH indications ...................................................................... Use always RFN when handling RACH indications The previous code was really confusing, passing full FNs as RFNs under certain external conditions, and then assuming the RFN input of rfn_to_fn() function could actually be a FN. As a result, we had a lot of code behaving slightly different depending on whether the incomding FN from pcuif was filled in by a BSC or a BTS. Avoid this b ehavior differentiation and always assume the most restricted one, aka RFN. Change-Id: Ib3b5702168195b595711cd0ff32c211b9aba429d --- M src/bts.cpp M src/bts.h M src/pcu_l1_if.cpp M src/pcu_utils.h M tests/tbf/TbfTest.cpp 5 files changed, 45 insertions(+), 23 deletions(-) Approvals: Jenkins Builder: Verified fixeria: Looks good to me, but someone else must approve laforge: Looks good to me, approved diff --git a/src/bts.cpp b/src/bts.cpp index d4bb35f..8d6f156 100644 --- a/src/bts.cpp +++ b/src/bts.cpp @@ -52,7 +52,6 @@ #include <errno.h> #include <string.h> -#define RFN_MODULUS 42432 #define RFN_THRESHOLD RFN_MODULUS / 2 extern void *tall_pcu_ctx; @@ -744,25 +743,13 @@ uint32_t m_cur_rfn; uint32_t fn_rounded; - /* make sure RFN does not exceed the maximum possible value of a valid - * GSM frame number. */ - OSMO_ASSERT(rfn < GSM_MAX_FN); - - /* Note: If a BTS is sending in a rach request it will be fully aware - * of the frame number. If the PCU is used in a BSC-co-located setup. - * The BSC will forward the incoming RACH request. The RACH request - * only contains the relative frame number (Fn % 42432) in its request - * reference. This PCU implementation has to fit both scenarios, so - * we need to assume that Fn is a relative frame number. */ - /* Ensure that all following calculations are performed with the * relative frame number */ - if (rfn >= RFN_MODULUS) - return rfn; + OSMO_ASSERT(rfn < RFN_MODULUS); /* Compute an internal relative frame number from the full internal frame number */ - m_cur_rfn = bts->cur_fn % RFN_MODULUS; + m_cur_rfn = fn2rfn(bts->cur_fn); /* Compute a "rounded" version of the internal frame number, which * exactly fits in the RFN_MODULUS raster */ diff --git a/src/bts.h b/src/bts.h index 7dd8547..7a67f76 100644 --- a/src/bts.h +++ b/src/bts.h @@ -196,7 +196,7 @@ uint16_t ra; uint8_t trx_nr; uint8_t ts_nr; - uint32_t rfn; + uint16_t rfn; int16_t qta; }; diff --git a/src/pcu_l1_if.cpp b/src/pcu_l1_if.cpp index aadc640..cda7af7 100644 --- a/src/pcu_l1_if.cpp +++ b/src/pcu_l1_if.cpp @@ -630,7 +630,7 @@ .ra = 0x00, .trx_nr = trx_nr, .ts_nr = ts_nr, - .rfn = fn, + .rfn = fn2rfn(fn), .qta = qta, }; @@ -640,11 +640,22 @@ static int pcu_rx_rach_ind(struct gprs_rlcmac_bts *bts, const struct gsm_pcu_if_rach_ind *rach_ind) { int rc = 0; - int current_fn = bts_current_frame_number(bts); + uint32_t current_fn = bts_current_frame_number(bts); + uint16_t rfn; - LOGP(DL1IF, LOGL_INFO, "RACH request received: sapi=%d " - "qta=%d, ra=0x%02x, fn=%u, cur_fn=%d, is_11bit=%d\n", rach_ind->sapi, rach_ind->qta, - rach_ind->ra, rach_ind->fn, current_fn, rach_ind->is_11bit); + /* Note: If a BSC is sending a RACH req to us, it is actually forwarding it to + * us from BTS as a result of receiving an RFN (Fn % 42432) over RSL + * (see 3GPP TS 48.058, section 9.3.19). + * If a BTS is sending a RACH req to us, it may contain a full FN + * (current osmo-bts does that) instead of an RFN. + * For consistency, and taking into account the BSC case limitations, + * work always with RFNs here: + */ + rfn = fn2rfn(rach_ind->fn); + + LOGP(DL1IF, LOGL_INFO, + "RACH request received: sapi=%d qta=%d, ra=0x%02x, fn=%u (rfn=%u), cur_fn=%d, is_11bit=%d\n", + rach_ind->sapi, rach_ind->qta, rach_ind->ra, rach_ind->fn, rfn, current_fn, rach_ind->is_11bit); if (OSMO_UNLIKELY(rach_ind->fn > GSM_TDMA_HYPERFRAME - 1)) { LOGP(DL1IF, LOGL_ERROR, "RACH request contains fn=%u that exceeds valid limits (0-%u) -- ignored!\n", @@ -658,7 +669,7 @@ .ra = rach_ind->ra, .trx_nr = rach_ind->trx_nr, .ts_nr = rach_ind->ts_nr, - .rfn = rach_ind->fn, + .rfn = rfn, .qta = rach_ind->qta, }; diff --git a/src/pcu_utils.h b/src/pcu_utils.h index 1c698ca..a6beab0 100644 --- a/src/pcu_utils.h +++ b/src/pcu_utils.h @@ -84,6 +84,12 @@ return x & -x; } +#define RFN_MODULUS 42432 +static inline uint16_t fn2rfn(uint32_t fn) +{ + return fn % RFN_MODULUS; +} + /* Used to store a C++ class in a llist used by C code */ struct llist_item { struct llist_head list; /* item used by llist */ diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp index bbd6821..ae8b56f 100644 --- a/tests/tbf/TbfTest.cpp +++ b/tests/tbf/TbfTest.cpp @@ -80,7 +80,7 @@ .ra = ra, .trx_nr = 0, .ts_nr = 0, - .rfn = Fn, + .rfn = fn2rfn(Fn), .qta = qta, }; -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33119 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Ib3b5702168195b595711cd0ff32c211b9aba429d Gerrit-Change-Number: 33119 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(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

1 0

[M] Change in osmo-pcu[master]: encoding: pass RFN to write_immediate_assignment(_reject)()
by fixeria 02 Jun '23

02 Jun '23

Attention is currently required from: pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/33123 ) Change subject: encoding: pass RFN to write_immediate_assignment(_reject)() ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33123 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: If9b758434c00f2a3868534d5be84946809c989a9 Gerrit-Change-Number: 33123 Gerrit-PatchSet: 3 Gerrit-Owner: pespin <pespin(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-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Fri, 02 Jun 2023 12:37:30 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in osmo-pcu[master]: Use fn_valid() helper in pcu_rx_time_ind()
by pespin 02 Jun '23

02 Jun '23

Attention is currently required from: pespin. Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-pcu/+/33138 to look at the new patch set (#2). Change subject: Use fn_valid() helper in pcu_rx_time_ind() ...................................................................... Use fn_valid() helper in pcu_rx_time_ind() Change-Id: I5b1f1d4cd621d81fb99b87761a878af242227a10 --- M src/pcu_l1_if.cpp 1 file changed, 10 insertions(+), 3 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/38/33138/2 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33138 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I5b1f1d4cd621d81fb99b87761a878af242227a10 Gerrit-Change-Number: 33138 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[M] Change in osmo-pcu[master]: encoding: pass RFN to write_immediate_assignment(_reject)()
by pespin 02 Jun '23

02 Jun '23

Attention is currently required from: pespin. Hello Jenkins Builder, laforge, fixeria, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-pcu/+/33123 to look at the new patch set (#3). Change subject: encoding: pass RFN to write_immediate_assignment(_reject)() ...................................................................... encoding: pass RFN to write_immediate_assignment(_reject)() Those function don't really require the full FN, hence let's pass only the required information. This makes the implementation here less dependent on how/if we are able to calculate full FNs based on RFN: We get an RFN, and we have to encode so that the RFN can be derived again, so feels less cumbersome having to go through RFN->FN->RFN which may only cause possible issues if there's some FN timing bug. 3GPP TS 44.018 10.5.2.30 Request Reference: "The purpose of the Request Reference information element is to provide the random access information used in the channel request and the frame number, FN modulo 42432" 3GPP TS 44.018 10.5.2.38 Starting Time: "The purpose of the Starting Time information element is to provide the start TDMA frame number, FN modulo 42432." Change-Id: If9b758434c00f2a3868534d5be84946809c989a9 --- M src/bts.cpp M src/encoding.cpp M src/encoding.h M tests/types/TypesTest.cpp 4 files changed, 55 insertions(+), 31 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/23/33123/3 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33123 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: If9b758434c00f2a3868534d5be84946809c989a9 Gerrit-Change-Number: 33123 Gerrit-PatchSet: 3 Gerrit-Owner: pespin <pespin(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-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[M] Change in osmo-pcu[master]: Move call to bts_set_current_frame_number() earlier in the code path
by pespin 02 Jun '23

02 Jun '23

Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-pcu/+/33154 to look at the new patch set (#2). Change subject: Move call to bts_set_current_frame_number() earlier in the code path ...................................................................... Move call to bts_set_current_frame_number() earlier in the code path The FN time counter is not really PDCH specific, but to the whole BTS, and all other calls t the bts_set_current_frame_number() are already laced in pcu_l1_if.cpp; move it there. Change-Id: If36f22a1067c904fa7fda87bed5062b6738f0dd1 --- M src/pcu_l1_if.cpp M src/pdch.cpp M tests/tbf/TbfTest.err 3 files changed, 34 insertions(+), 51 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/54/33154/2 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33154 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: If36f22a1067c904fa7fda87bed5062b6738f0dd1 Gerrit-Change-Number: 33154 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-MessageType: newpatchset

1 0

[S] Change in osmo-pcu[master]: Derive FN from RFN once and cache it in struct rach_ind_params
by pespin 02 Jun '23

02 Jun '23

Attention is currently required from: pespin. Hello Jenkins Builder, laforge, fixeria, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-pcu/+/33121 to look at the new patch set (#4). Change subject: Derive FN from RFN once and cache it in struct rach_ind_params ...................................................................... Derive FN from RFN once and cache it in struct rach_ind_params Change-Id: Iaefb9650dfc5083360a4a24b9c17fdbf3115e51f --- M src/bts.cpp M src/bts.h M src/pcu_l1_if.cpp M tests/tbf/TbfTest.cpp 4 files changed, 15 insertions(+), 4 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/21/33121/4 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33121 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Iaefb9650dfc5083360a4a24b9c17fdbf3115e51f Gerrit-Change-Number: 33121 Gerrit-PatchSet: 4 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[S] Change in osmo-pcu[master]: Use OSMO_UNLIKELY() in bts_rfn_to_fn()
by pespin 02 Jun '23

02 Jun '23

pespin has uploaded a new patch set (#2). ( https://gerrit.osmocom.org/c/osmo-pcu/+/33157 ) Change subject: Use OSMO_UNLIKELY() in bts_rfn_to_fn() ...................................................................... Use OSMO_UNLIKELY() in bts_rfn_to_fn() That case should only happen under really rare conditions, like receiving a RACH.ind before having received any DATA.ind. Change-Id: I4c71f3481764b501a4441bc735a87725884a3e75 --- M src/bts.cpp 1 file changed, 13 insertions(+), 1 deletion(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/57/33157/2 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33157 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I4c71f3481764b501a4441bc735a87725884a3e75 Gerrit-Change-Number: 33157 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-CC: Jenkins Builder Gerrit-MessageType: newpatchset

1 0

[S] Change in osmo-pcu[master]: Use OSMO_UNLIKELY() in bts_rfn_to_fn()
by pespin 02 Jun '23

02 Jun '23

pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-pcu/+/33157 ) Change subject: Use OSMO_UNLIKELY() in bts_rfn_to_fn() ...................................................................... Use OSMO_UNLIKELY() in bts_rfn_to_fn() That case should only happen under really rare conditions, like receiving a RACH.ind before having received any DATA.ind. Change-Id: I4c71f3481764b501a4441bc735a87725884a3e75 --- M src/bts.cpp 1 file changed, 13 insertions(+), 1 deletion(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/57/33157/1 diff --git a/src/bts.cpp b/src/bts.cpp index 5160fd5..44acbed 100644 --- a/src/bts.cpp +++ b/src/bts.cpp @@ -748,7 +748,7 @@ OSMO_ASSERT(rfn < RFN_MODULUS); m_cur_fn = bts_current_frame_number(bts); - if (m_cur_fn == FN_UNSET) { + if (OSMO_UNLIKELY(m_cur_fn == FN_UNSET)) { LOGP(DRLCMAC, LOGL_ERROR, "Unable to calculate full FN from RFN %u: Current FN not known!\n", rfn); return rfn; -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33157 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I4c71f3481764b501a4441bc735a87725884a3e75 Gerrit-Change-Number: 33157 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newchange

1 0

[M] Change in osmo-pcu[master]: encoding: only RFN needed in write_immediate_assignment(_reject)()
by fixeria 02 Jun '23

02 Jun '23

Attention is currently required from: pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/33123 ) Change subject: encoding: only RFN needed in write_immediate_assignment(_reject)() ...................................................................... Patch Set 2: Code-Review+1 (1 comment) Patchset: PS2: > Re-adding +1 and assuming Vadim is fine after I updated the commit message with what I wrote in the […] Yeah, I am generally fine. But I would change the commit name to: `encoding: pass RFN to write_immediate_assignment(_reject)()`. Not going to block though. -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33123 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: If9b758434c00f2a3868534d5be84946809c989a9 Gerrit-Change-Number: 33123 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(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-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Fri, 02 Jun 2023 12:29:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[M] Change in osmo-pcu[master]: encoding: only RFN needed in write_immediate_assignment(_reject)()
by pespin 02 Jun '23

02 Jun '23

pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/33123 ) Change subject: encoding: only RFN needed in write_immediate_assignment(_reject)() ...................................................................... Patch Set 2: Code-Review+2 (1 comment) Patchset: PS2: Re-adding +1 and assuming Vadim is fine after I updated the commit message with what I wrote in the comment. -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33123 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: If9b758434c00f2a3868534d5be84946809c989a9 Gerrit-Change-Number: 33123 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Fri, 02 Jun 2023 12:26:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0