Attention is currently required from: laforge, fixeria. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/30743 ) Change subject: {trxcon,virt_phy}: shared GPRS L1 (MAC) implementation ...................................................................... Patch Set 10: (10 comments) Patchset: PS10: Most messages are improvements which can be done at a later stuff. Some others really need looking at. File include/l1ctl_proto.h: https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/b567ecd3_2af0709c PS10, Line 367: uint8_t padding[3]; I still don't see the point in adding these padding bytes here. Can you explain? What's the rationale behind aligning to 4 bytes? File include/l1gprs.h: https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/aa8c6737_9baa6b40 PS10, Line 39: /*! Uplink and Downlink TBFs */ It probably makes sense to have 1 list for DL_TBF and another one for UL_TBF. After all, you get a CFG UL TBF or CFG DL TBF, so you know which of the 2 lists to lookup. File src/host/virt_phy/src/virt_prim_pdch.c: https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/970fc438_950cac98 PS10, Line 34: void l1ctl_rx_gprs_uldl_tbf_cfg_req(struct l1_model_ms *ms, struct msgb *msg) So you send both L1CTL messages to this function, to then split it again to 2 differents functions below. This function sounds totally unnecesary and only makes things more complex/confusing. https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/dcd07d9f_1a1bbc5b PS10, Line 69: if (OSMO_UNLIKELY(fn_sched != req.hdr.fn)) { AFAIU this should be OK if req.hdr.fn < fn_sched (taking wraparound into account). https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/3fc69557_4b7292e3 PS10, Line 73: /* FIXME: return; */ yes, you must return here and not schedule it. File src/shared/l1gprs.c: https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/6380e460_9a75a685 PS10, Line 83: static struct l1gprs_tbf *l1gprs_find_tbf(struct l1gprs_state *gprs, In the end you find based on uplink and downlink, so yeah having 2 llists makes sense imho. https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/c93808bb_096403d7 PS10, Line 111: if (tbf->uplink) { Again sounds like you could perfectly have 2 functions. You could also have 2 tbf structs (UL and DL) and spare the dl_tfi_mask in ul_tbf ones. https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/ce85645c_b0a3408f PS10, Line 294: l1gprs_register_tbf(gprs, tbf); I think it may be good to always first unregister, and if != 0x00, also register. This way a TBF can updated its TS allocation in 1 go. https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/974fee06_900233fb PS10, Line 330: l1gprs_register_tbf(gprs, tbf); Same as per what I said on ul_tbf above. -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/30743 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: I9567d64f9d00262e36147e8d7e541e5e246bda5f Gerrit-Change-Number: 30743 Gerrit-PatchSet: 10 Gerrit-Owner: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge@osmocom.org> Gerrit-CC: pespin <pespin@sysmocom.de> Gerrit-Attention: laforge <laforge@osmocom.org> Gerrit-Attention: fixeria <vyanitskiy@sysmocom.de> Gerrit-Comment-Date: Thu, 16 Mar 2023 13:51:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment