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.