Attention is currently required from: laforge, fixeria.
10 comments:
Patchset:
Most messages are improvements which can be done at a later stuff. Some others really need looking at.
File include/l1ctl_proto.h:
Patch Set #10, 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:
Patch Set #10, 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:
Patch Set #10, 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.
Patch Set #10, 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).
Patch Set #10, Line 73: /* FIXME: return; */
yes, you must return here and not schedule it.
File src/shared/l1gprs.c:
Patch Set #10, 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.
Patch Set #10, 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.
Patch Set #10, 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.
Patch Set #10, Line 330: l1gprs_register_tbf(gprs, tbf);
Same as per what I said on ul_tbf above.
To view, visit change 30743. To unsubscribe, or for help writing mail filters, visit settings.