Attention is currently required from: laforge, pespin.
fixeria 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 11:
(5 comments)
File include/l1ctl_proto.h:
https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/4395d2b1_3a6490a1
PS10, Line 367: uint8_t padding[3];
> I still don't see the point in adding these padding bytes here. […]
I already tried to explain and additionally brought this topic up in the IRC. Let's please avoid spending even more time discussing this. Padding is already present in existing L1CTL messages, so it will be in the new ones too.
File src/host/virt_phy/src/virt_prim_pdch.c:
https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/942ed7ba_34ff2bc0
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 b […]
I think it's fine to have a single function for handling both L1CTL messages, because a) we want to check `ms->gprs` against NULL, and b) we need to `pull()` the `l1ctl_hdr`. This is common for both messages and having two separate functions would be code duplication. The two different functions below are "external" ones from l1gprs. So I disagree that it's "totally unnecesary" and prefer to do it this way. It's not like I am mixing up all GPRS related L1CTL messages here, just the two related ones.
https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/7f2ed3a0_7fad25de
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).
No. The frame number must match, otherwise we're sending stuff earlier or later then requested, what is unacceptable in GPRS. We're violating this and not returning early below, but this is rather a temporary hack.
https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/67a04d89_0da714d1
PS10, Line 73: /* FIXME: return; */
> yes, you must return here and not schedule it.
Well, at the moment we always hit this block. So if we return, all UL blocks will be discarded. The root problem needs to be fixed first. Either virtphy is scheduling stuff in advance, or the upper layers are sending block too late. For now it's a FIXME.
File src/shared/l1gprs.c:
https://gerrit.osmocom.org/c/osmocom-bb/+/30743/comment/ac1f7c60_60c68060
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.
What do we win by having two different lists?
--
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: 11
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 16 Mar 2023 14:28:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/31572 )
Change subject: modem: route L1CTL prims to/from libosmo-gprs-rlcmac
......................................................................
Patch Set 7:
(1 comment)
File src/host/layer23/src/modem/rlcmac.c:
https://gerrit.osmocom.org/c/osmocom-bb/+/31572/comment/c55887cb_943c202e
PS3, Line 131: lp->pdch_data_req.ts_nr,
> return l1ctl_tx_gprs_ul_block_req(ms, […]
Not yet fixed. I'm not against aligning parameters, what I'm saying is that you should then align all of them to the same indentation, not doing so for 1..N params and keeping the first one at a different indentation.
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/31572
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: I38e9a686f8edc3fe55f961d75e68602c33bbbaaf
Gerrit-Change-Number: 31572
Gerrit-PatchSet: 7
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 16 Mar 2023 14:11:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/31945 )
Change subject: layer23/{mobile,modem}: fix segfault on VTY connection
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Patchset:
PS1:
so that means the struct is not copied by the function but keeps a reference to it, did I understand correctly?
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/31945
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: I75843a964254243c70bedcf8ff97d854107ee21a
Gerrit-Change-Number: 31945
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 16 Mar 2023 14:04:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/31571 )
Change subject: layer23: implement Rx/Tx API for GPRS related messages
......................................................................
Patch Set 7: Code-Review+1
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/31571
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: I87950e893ef96ff8328f43f1548111aa9f66439b
Gerrit-Change-Number: 31571
Gerrit-PatchSet: 7
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 16 Mar 2023 13:54:57 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
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(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 16 Mar 2023 13:51:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
Hello Jenkins Builder, laforge, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmocom-bb/+/31571
to look at the new patch set (#7).
Change subject: layer23: implement Rx/Tx API for GPRS related messages
......................................................................
layer23: implement Rx/Tx API for GPRS related messages
Change-Id: I87950e893ef96ff8328f43f1548111aa9f66439b
Related: OS#5500
---
M src/host/layer23/include/osmocom/bb/common/l1ctl.h
M src/host/layer23/include/osmocom/bb/common/ms.h
M src/host/layer23/src/common/l1ctl.c
3 files changed, 127 insertions(+), 0 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmocom-bb refs/changes/71/31571/7
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/31571
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: I87950e893ef96ff8328f43f1548111aa9f66439b
Gerrit-Change-Number: 31571
Gerrit-PatchSet: 7
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newpatchset