Attention is currently required from: laforge, pespin, fixeria, dexter.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/31341 )
Change subject: pcu_l1_if_phy: flexible phy access ......................................................................
Patch Set 11:
(11 comments)
Patchset:
PS11: how does it work that only one phy ops entry is added? ...by only linking one of the phy implementation .c files, right?
File src/osmobts_sock.c:
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/b57d8d16_a63bc618 PS11, Line 114: the_pcu->phy_ops->l1if_close_pdch(bts->trx[trx].fl1h); if (the_pcu->phy_ops->l1if_close_pdch) the_pcu->phy_ops->l1if_close_pdch(bts->trx[trx].fl1h);
in general, cb functions may be left NULL if nothing should happen
File src/pcu_l1_if.cpp:
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/45fc38d9_d5047329 PS11, Line 218: the_pcu->phy_ops->l1if_pdch_req(bts->trx[trx].fl1h, ts, 0, fn, arfcn, block_nr, if (...
check against NULL cb
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/a55bd44c_0063a6a3 PS11, Line 247: the_pcu->phy_ops->l1if_pdch_req(bts->trx[trx].fl1h, ts, 1, fn, arfcn, block_nr, data, data_len); check NULL cb
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/b6991b0a_1108bab9 PS11, Line 869: if (!bts->trx[trx_nr].fl1h) && cb != NULL
File src/pcu_l1_if_phy.c:
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/8a6cd8ba_2d317868 PS11, Line 2: * (C) 2023 by sysmocom s.f.m.c. GmbH info@sysmocom.de back in the days Holger asked me to add a dash
sysmocom - s.f.m.c. GmbH
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/878685d1_413a8772 PS11, Line 51: l1if_phy_entry = talloc_zero(NULL, struct l1if_phy_entry); (i wonder if -fsanitize will report this as leaked memory whenever osmo-pcu exits .. i guess it would be better to either add a ctx arg to talloc the entry from, or to add a pcu_l1if_cleanup() that talloc_free()s all list entries that the main() calls before exiting)
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/450a8b1e_823b4167 PS11, Line 71: LOGPC(DLGLOBAL, LOGL_NOTICE, "\n"); please let's never ever use LOGPC() anymore! It was a messed up concept from the start, and in particular is very awkward in gsmtap_log.
either a static buffer with OSMO_STRBUF_PRINTF() or
char *msg = NULL; llist_for... { osmo_talloc_asprintf(OTC_SELECT, msg, "foo"); } LOGP(msg)
and maybe add, at the start,
if (!log_check_level(DLGLOBAL, LOGL_NOTICE)) return;
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/b9a9e931_0aeefcdd PS11, Line 75: ops suggest name
singleton_ops
...but seems to me the task this return arg solves could be achieved more elegantly? I'd like to see some code that acts on non-singleton phy lists, i guess will come up in another patch.
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/a3620cd4_a4ecaa24 PS11, Line 81: *ops = NULL; let's add
/* make sure a singleton is not mixed with other PHY */ llist_for_each_entry(l1if_phy_entry, &phy_list, list) if (l1if_phy_entry->ops->singleton) OSMO_ASSERT(llist_count(&phy_list) == 1); without this check, below loop would initialize other non-singleton phy before exiting the list on the first singleton phy, and would skip any other entries ... i.e. act weird without complaining.
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/a8dfec3e_eb6928f0 PS11, Line 100: if (*ops) { so the indication whether a phy is in use depends on the argument that the caller passes in to be zero initialized? seems awkward to me / gives weird errors when forgetting to zero initialize. When i see non-singleton code i might have a suggestion for a more elegant solution... for now i can only complain =)