Attention is currently required from: fixeria, dexter. neels 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 2: (19 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/ed4cd07a_aeea2adf PS2, Line 10: po typo Patchset: PS2: looks good in general! let's just polish some details... File doc/timeslot.msc: https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/9c435994_9ac70549 PS2, Line 95: bsc_ts abox bsc_ts [label="UNUSED"]; a timeslot FSM cannot transition from ST_PDCH directly to ST_UNUSED. Please insert here the actual transition as i see in the implementation: bsc_ts abox bsc_ts [label="WAIT_PDCH_DEACT (4s, T23001)"]; bts <= bsc_ts [label="RSL RF Chan Release of PDCH",ID="Osmocom style"]; ...; bts => bsc_ts [label="RSL RF Chan Release ACK",ID="Osmocom style"]; bsc_ts abox bsc_ts [label="UNUSED"]; Same below for PDCH activation... https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/8c881ef0_8db1a260 PS2, Line 98: " \nfor all timeslots in state UNUSED:" https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/f329b054_31e0657c PS2, Line 99: PDCH UNUSED https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/63d8ad9a_d24aca23 PS2, Line 100: bsc_ts <- pcu_sock [label="TS_EV_PDCH_ACT"]; bsc_ts abox bsc_ts [label="WAIT_PDCH_ACT (4s, T23001)"]; bts <= bsc_ts [label="RSL Chan Activ of PDCH",ID="Osmocom style"]; ...; bts => bsc_ts [label="RSL RF Chan Activ ACK",ID="Osmocom style"]; bsc_ts abox bsc_ts [label="PDCH"]; File include/osmocom/bsc/timeslot_fsm.h: https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/9ecfa52e_5e39de9d PS2, Line 38: TS_EV_LCHAN_UNUSED, Would be nice to add comments to clarify how the "TS_EV_PDCH_*" are fundamentally different: /* RSL responses received from the BTS: */ https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/43ad28b0_6c1a3956 PS2, Line 42: TS_EV_PDCH_DEACT_NACK, /* An Ericsson PCU has disconnected from OsmoBSC, deactivate PDCH: */ https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/895197c7_ad141738 PS2, Line 43: TS_EV_PDCH_DEACT, /* An Ericsson PCU has reconnected to OsmoBSC, re-activate PDCH: */ File src/osmo-bsc/pcu_sock.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/20e5fa50_64da4632 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? https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/18d0ddda_bea86340 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 https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/a5e5a216_bf4db00c 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 comment saying so explicitly. How about GSM_PCHAN_TCH_F_PDCH dynamic timeslots? I guess they don't apply to Ericsson RBS -- also good to clarify that in a comment. https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/013eb5ea_4a1592c5 PS2, Line 802: ts->fi->state == TS_ST_PDCH generally true, but here i agree with checking the state explicitly: Receiving EV_PDCH_DEACT only makes sense when in ST_PDCH. We could add an allstate-event handling in timeslot_fsm.c that does this same check, but emitting EV_PDCH_DEACT logging for every single timeslot, in ST_PDCH or not, would be spammy. I think my favorite would be exactly this code, but in a function implemented in timeslot_fsm.c and called from here: void ts_pdch_deact(...) { if (ts->fi->state != TS_ST_PDCH) return; osmo_fsm_inst_dispatch(ts->fi, TS_EV_PDCH_DEACT); } so exactly this code, but kept closer to the ts fsm implementation instead of spreading ts fsm behavior details into pcu_sock.c. https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/39790d92_377bf241 PS2, Line 940: /* FIXME: allow multiple BTS */ what makes it hard to implement this now instead of adding a FIXME comment? https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/55eb9537_4ea93938 PS2, Line 941: bts = llist_entry(state->net->bts_list.next, struct gsm_bts, list); rather use llist_first_entry_or_null() but ... instead of always acting on the first BTS, shouldn't this find the exact BTS that the specific PCU is responsible for? .. I think I understand that this is a special case for RBS where the PCU is running next to the BSC instead of the usual next-to-BTS. Right? Add a code comment to clarify? https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/f16b4e18_e7f5e3c3 PS2, Line 973: gsm_bts_trx_num agree https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/b2c9d9ed_dc64dc7a 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 days osmo code uses ARRAY_SIZE https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/cc3a9e76_beaeb054 PS2, Line 979: && ts->pchan_on_init == GSM_PCHAN_OSMO_DYN && ts->fi->state == TS_ST_UNUSED) { Also for GSM_PCHAN_TCH_F_PDCH ? ...complementary to above, here i would love to see a function ts_pdch_act() called, function implementation in timeslot_fsm.c, so that we don't add ts_fsm state logic outside of timeslot_fsm.c File src/osmo-bsc/timeslot_fsm.c: https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/d0917b6a_27105e03 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 the reader can easily see that the "if is_ericsson" part is being added and the rest remains unchanged (also if someone from the future reviews the git log) -- 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: 2 Gerrit-Owner: dexter &lt;pmaier(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-CC: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Attention: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Attention: dexter &lt;pmaier(a)sysmocom.de&gt; Gerrit-Comment-Date: Sun, 26 Feb 2023 23:45:16 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-MessageType: comment