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

[M] Change in osmo-bsc[master]: pcu_sock: improve logging
by fixeria 01 Mar '23

01 Mar '23

Attention is currently required from: dexter. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31579 ) Change subject: pcu_sock: improve logging ...................................................................... Patch Set 4: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31579 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I4a3ff23ffccf803b1a97e3f2604d3b422c6d6afd Gerrit-Change-Number: 31579 Gerrit-PatchSet: 4 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-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Wed, 01 Mar 2023 13:20:26 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in osmo-bsc[master]: pcu_sock: use struct to transfer IMMEDIATE ASSIGNMENT for PCH
by fixeria 01 Mar '23

01 Mar '23

Attention is currently required from: dexter. fixeria 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 4: Code-Review-1 (1 comment) File include/osmocom/bsc/pcuif_proto.h: https://gerrit.osmocom.org/c/osmo-bsc/+/31578/comment/79f24e71_951b9e76 PS4, Line 280: pgroup See my comment https://gerrit.osmocom.org/c/osmo-pcu/+/31145/comments/669099fc_aa3e5889. It's not a paging group but just the last three digits of IMSI, so it should be named 'imsi', and let's use `uint16_t` as Pau suggested. -- 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: 4 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-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Wed, 01 Mar 2023 13:19:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in osmo-bsc[master]: abis_rsl: guard against over long IMMEDIATE ASSIGNMENT Messages
by fixeria 01 Mar '23

01 Mar '23

Attention is currently required from: laforge, dexter. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31577 ) Change subject: abis_rsl: guard against over long IMMEDIATE ASSIGNMENT Messages ...................................................................... Patch Set 4: (1 comment) File src/osmo-bsc/abis_rsl.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31577/comment/be7a0cd2_6e1b5b5f PS4, Line 947: if (len > sizeof(buf)) { I think you can even move this at the beginning of the function, because generally an Immediate Assignment PDU cannot exceed the limit of GSM_MACBLOCK_LEN bytes. -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31577 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I9417b35fb8c0517f2555e17059bf8ac60fa59791 Gerrit-Change-Number: 31577 Gerrit-PatchSet: 4 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-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Wed, 01 Mar 2023 13:14:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in osmo-bsc[master]: abis_rsl: guard against over long IMMEDIATE ASSIGNMENT Messages
by fixeria 01 Mar '23

01 Mar '23

Attention is currently required from: laforge, dexter. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31577 ) Change subject: abis_rsl: guard against over long IMMEDIATE ASSIGNMENT Messages ...................................................................... Patch Set 4: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31577 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I9417b35fb8c0517f2555e17059bf8ac60fa59791 Gerrit-Change-Number: 31577 Gerrit-PatchSet: 4 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-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Wed, 01 Mar 2023 13:10:33 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in osmo-bsc[master]: pcu_sock: drop usage of PCUIF flag PCU_IF_FLAG_DT
by dexter 01 Mar '23

01 Mar '23

dexter has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31619 ) Change subject: pcu_sock: drop usage of PCUIF flag PCU_IF_FLAG_DT ...................................................................... pcu_sock: drop usage of PCUIF flag PCU_IF_FLAG_DT The flag PCU_IF_FLAG_DT is no longer needed. The PCU implementation will distinguish by the version number instead. Change-Id: I0dc20e351deb14906b2edffc39499bad9659cc35 Related: OS#5198 --- M include/osmocom/bsc/pcuif_proto.h M src/osmo-bsc/pcu_sock.c 2 files changed, 13 insertions(+), 2 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/19/31619/1 diff --git a/include/osmocom/bsc/pcuif_proto.h b/include/osmocom/bsc/pcuif_proto.h index 1d12f50..d05a4aa 100644 --- a/include/osmocom/bsc/pcuif_proto.h +++ b/include/osmocom/bsc/pcuif_proto.h @@ -45,7 +45,6 @@ /* flags */ #define PCU_IF_FLAG_ACTIVE (1 << 0)/* BTS is active */ #define PCU_IF_FLAG_SYSMO (1 << 1)/* access PDCH of sysmoBTS directly */ -#define PCU_IF_FLAG_DT (1 << 2)/* use TLLI for confirmation directly */ #define PCU_IF_FLAG_CS1 (1 << 16) #define PCU_IF_FLAG_CS2 (1 << 17) #define PCU_IF_FLAG_CS3 (1 << 18) diff --git a/src/osmo-bsc/pcu_sock.c b/src/osmo-bsc/pcu_sock.c index 933ff5f..c5b1a8d 100644 --- a/src/osmo-bsc/pcu_sock.c +++ b/src/osmo-bsc/pcu_sock.c @@ -201,7 +201,6 @@ info_ind->version = PCU_IF_VERSION; info_ind->flags |= PCU_IF_FLAG_ACTIVE; info_ind->flags |= PCU_IF_FLAG_SYSMO; - info_ind->flags |= PCU_IF_FLAG_DT; /* RAI */ info_ind->mcc = bts->network->plmn.mcc; -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31619 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I0dc20e351deb14906b2edffc39499bad9659cc35 Gerrit-Change-Number: 31619 Gerrit-PatchSet: 1 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-MessageType: newchange

1 0

[S] Change in osmo-bsc[master]: abis_rsl: guard against over long IMMEDIATE ASSIGNMENT Messages
by dexter 01 Mar '23

01 Mar '23

Attention is currently required from: laforge. Hello Jenkins Builder, laforge, pespin, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-bsc/+/31577 to look at the new patch set (#4). Change subject: abis_rsl: guard against over long IMMEDIATE ASSIGNMENT Messages ...................................................................... abis_rsl: guard against over long IMMEDIATE ASSIGNMENT Messages The length parameter in rsl_imm_assign_cmd_common() may cause a buffer overflow when it is chosen larger than GSM_MACBLOCK_LEN. Lets make sure this cannot happen. Change-Id: I9417b35fb8c0517f2555e17059bf8ac60fa59791 --- M src/osmo-bsc/abis_rsl.c 1 file changed, 19 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/77/31577/4 -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31577 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I9417b35fb8c0517f2555e17059bf8ac60fa59791 Gerrit-Change-Number: 31577 Gerrit-PatchSet: 4 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in osmo-bsc[master]: pcu_sock: activate/deactivate PDCH on pcu reconnect
by dexter 01 Mar '23

01 Mar '23

Attention is currently required from: neels, laforge, fixeria. dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31497 ) Change subject: pcu_sock: activate/deactivate PDCH on pcu reconnect ...................................................................... Patch Set 6: (10 comments) File src/osmo-bsc/pcu_sock.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/67063c24_41dc2174 PS2, Line 801: printf("l1sap_chan_rel(trx,gsm_lchan2chan_nr(ts->lchan));\n"); > either keep this printf, or if it should be dropped, drop it in a separate patch? I think its correct to drop it here. The code was ported from osmo-bts some years ago and I think this printf was left there as a placeholder for the code we now add with this patch. https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/3e2cf751_41a20ed3 PS2, Line 794: for (i = 0; i < PCU_IF_NUM_TRX; i++) { > this is a fix of a magic number, very good but rather in a separate patch Done https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/ae63d0a7_d4cd98bf PS2, Line 801: && ts->pchan_on_init == GSM_PCHAN_OSMO_DYN > Why does this apply only to dynamic timeslots? I think I understand why, but I'd like to see a code […] I have added a comment. I hope it makes it clearer now. As far as I know GSM_PCHAN_TCH_F_PDCH is the ip.access style of dynamic timeslots. This is not applicable here. https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/3f0db475_16bf1200 PS2, Line 802: ts->fi->state == TS_ST_PDCH > generally true, but here i agree with checking the state explicitly: […] Done https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/6c3d8917_cf7e940c PS2, Line 935: struct gsm_bts_trx_ts *ts; > This variable should be moved to the inner for-loop. Done https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/e75c97c8_d56c212d PS2, Line 941: bts = llist_entry(state->net->bts_list.next, struct gsm_bts, list); > rather use llist_first_entry_or_null() […] The PCU still has a limitation that it can only handle one BTS at a time. Thats at least what it is tested for. It indeed has a list with BTSs, but it never ran with multiple BTSs. Also the PHY implementations pick only the first BTS in that list. As far as the BSC is concerned we can fix this. For the sock_accept and sock_close function we have to run over all BTSs in the network. When we get the primitive from the PCU it has the BTS number in it. I will fix this in a different patch since it is a bit out of scope here. I will also keep the llist_entry() to keep consistency with the existing code. https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/62e097d6_ba76a0fe PS2, Line 973: gsm_bts_trx_num > agree Done https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/8bad96dd_676f090a PS2, Line 976: 8 > could also use TRX_NR_TS, but ARRAY_SIZE is also my favorite; TRX_NR_TS is our legacy way, these day […] Done https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/8001ce33_69ada64d PS2, Line 979: && ts->pchan_on_init == GSM_PCHAN_OSMO_DYN && ts->fi->state == TS_ST_UNUSED) { > Also for GSM_PCHAN_TCH_F_PDCH ? […] Done File src/osmo-bsc/timeslot_fsm.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/7786740f_cf433924 PS2, Line 326: static void ts_fsm_unused_pdch_act(struct osmo_fsm_inst *fi) > it would be cool to add this function in a separate patch before this patch, so that in this patch t […] lets skip this part, we are a bit under pressure to get this patch set merged. -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31497 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I9ea0c53a5e68a51c781ef43bae71f947cdb95678 Gerrit-Change-Number: 31497 Gerrit-PatchSet: 6 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Wed, 01 Mar 2023 12:50:29 +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> Gerrit-MessageType: comment

1 0

[M] Change in osmo-bsc[master]: pcu_sock: activate/deactivate PDCH on pcu reconnect
by dexter 01 Mar '23

01 Mar '23

Attention is currently required from: neels, laforge, fixeria. Hello Jenkins Builder, neels, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-bsc/+/31497 to look at the new patch set (#6). Change subject: pcu_sock: activate/deactivate PDCH on pcu reconnect ...................................................................... pcu_sock: activate/deactivate PDCH on pcu reconnect When the PCU is disconnected while the BSC keeps running the PDCH should be closed. Also the PDCH should be reopened when the PCU is reconnected. Change-Id: I9ea0c53a5e68a51c781ef43bae71f947cdb95678 Related: OS#5198 --- M doc/timeslot.msc M include/osmocom/bsc/timeslot_fsm.h M src/osmo-bsc/pcu_sock.c M src/osmo-bsc/timeslot_fsm.c 4 files changed, 134 insertions(+), 25 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/97/31497/6 -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31497 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I9ea0c53a5e68a51c781ef43bae71f947cdb95678 Gerrit-Change-Number: 31497 Gerrit-PatchSet: 6 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[S] Change in osmo-bsc[master]: abis_rsl: guard against over long IMMEDIATE ASSIGNMENT Messages
by dexter 01 Mar '23

01 Mar '23

Attention is currently required from: laforge. dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31577 ) Change subject: abis_rsl: guard against over long IMMEDIATE ASSIGNMENT Messages ...................................................................... Patch Set 4: (1 comment) File src/osmo-bsc/abis_rsl.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31577/comment/91c70e06_e0877102 PS3, Line 948: val > we have to be a bit carefult with assert's to avoid ending up in the sukchan-style of software devel […] I ran into this due to a programming mistake, but yes, its probably wiser to return NULL. -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31577 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I9417b35fb8c0517f2555e17059bf8ac60fa59791 Gerrit-Change-Number: 31577 Gerrit-PatchSet: 4 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Comment-Date: Wed, 01 Mar 2023 12:50:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Gerrit-MessageType: comment

1 0

[M] Change in osmo-bsc[master]: pcu_sock: use struct to transfer IMMEDIATE ASSIGNMENT for PCH
by dexter 01 Mar '23

01 Mar '23

Attention is currently required from: dexter. Hello Jenkins Builder, laforge, 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 (#4). 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 paging group) is premended 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. Change-Id: Id6acbd243adf26169e5e8319dd66bb68dd6a3c22 Related: OS#5198 --- M include/osmocom/bsc/pcuif_proto.h M src/osmo-bsc/pcu_sock.c 2 files changed, 39 insertions(+), 11 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/78/31578/4 -- 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: 4 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0