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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels <[email protected]> Gerrit-CC: fixeria <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-Attention: neels <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: fixeria <[email protected]> Gerrit-Comment-Date: Wed, 01 Mar 2023 12:50:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <[email protected]> Comment-In-Reply-To: fixeria <[email protected]> Gerrit-MessageType: comment
