June 2023 - gerrit-log - lists.osmocom.org

[S] Change in osmo-hnbgw[master]: coverity: fix type of local var
by neels 05 Jun '23

05 Jun '23

neels has submitted this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/33158 ) Change subject: coverity: fix type of local var ...................................................................... coverity: fix type of local var osmo_sccp_instance_next_conn_id() may return negative on error, so use int instead of uint. Fixup for recent 548d3d2f6639b728710b9a1905087a189f47de78 'use new osmo_sccp_instance_next_conn_id()' Related: CID#316694 Change-Id: I2623c06c23691acb75f6ee6ff3a42ac7d10a4b1f --- M src/osmo-hnbgw/context_map.c 1 file changed, 17 insertions(+), 1 deletion(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/src/osmo-hnbgw/context_map.c b/src/osmo-hnbgw/context_map.c index cdc526b..2af685e 100644 --- a/src/osmo-hnbgw/context_map.c +++ b/src/osmo-hnbgw/context_map.c @@ -57,7 +57,7 @@ bool is_ps) { struct hnbgw_context_map *map; - uint32_t new_scu_conn_id; + int new_scu_conn_id; struct hnbgw_cnlink *cnlink; struct hnbgw_sccp_user *hsu; -- To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33158 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hnbgw Gerrit-Branch: master Gerrit-Change-Id: I2623c06c23691acb75f6ee6ff3a42ac7d10a4b1f Gerrit-Change-Number: 33158 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[M] Change in osmo-hnbgw[master]: cnpool: split up context_map_find_or_create_by_rua_ctx_id()
by neels 05 Jun '23

05 Jun '23

neels has submitted this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/33127 ) Change subject: cnpool: split up context_map_find_or_create_by_rua_ctx_id() ...................................................................... cnpool: split up context_map_find_or_create_by_rua_ctx_id() Have separate find, alloc, set_cnlink functions instead of all in one. For CN pooling, I need these operations to be separate: - we need to decode the RANAP and L3 PDU only if no existing context_map was found (see I373d665c9684b607207f68094188eab63209db51 ). - while decoding RANAP and L3 PDU, and while deciding which cnlink to use, I already want to use the logging context of LOG_MAP(). So I want to allocate a map first, and connect it to the selected cnlink later. Related: SYS#6412 Change-Id: Ifc5242547260154eb5aecd3a9d9c2aac8419e8db --- M include/osmocom/hnbgw/context_map.h M src/osmo-hnbgw/context_map.c M src/osmo-hnbgw/hnbgw_rua.c 3 files changed, 100 insertions(+), 39 deletions(-) Approvals: pespin: Looks good to me, but someone else must approve Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/include/osmocom/hnbgw/context_map.h b/include/osmocom/hnbgw/context_map.h index fe9d3e0..55d96ea 100644 --- a/include/osmocom/hnbgw/context_map.h +++ b/include/osmocom/hnbgw/context_map.h @@ -136,8 +136,9 @@ enum hnbgw_context_map_state map_rua_get_state(struct hnbgw_context_map *map); enum hnbgw_context_map_state map_sccp_get_state(struct hnbgw_context_map *map); -struct hnbgw_context_map *context_map_find_or_create_by_rua_ctx_id(struct hnb_context *hnb, uint32_t rua_ctx_id, - bool is_ps); +struct hnbgw_context_map *context_map_find_by_rua_ctx_id(struct hnb_context *hnb, uint32_t rua_ctx_id, bool is_ps); +struct hnbgw_context_map *context_map_alloc(struct hnb_context *hnb, uint32_t rua_ctx_id, bool is_ps); +int context_map_set_cnlink(struct hnbgw_context_map *map, struct hnbgw_cnlink *cnlink_selected); void map_rua_fsm_alloc(struct hnbgw_context_map *map); void map_sccp_fsm_alloc(struct hnbgw_context_map *map); diff --git a/src/osmo-hnbgw/context_map.c b/src/osmo-hnbgw/context_map.c index 2af685e..13aae0e 100644 --- a/src/osmo-hnbgw/context_map.c +++ b/src/osmo-hnbgw/context_map.c @@ -53,13 +53,9 @@ } /* Map from a HNB + ContextID to the SCCP-side Connection ID */ -struct hnbgw_context_map *context_map_find_or_create_by_rua_ctx_id(struct hnb_context *hnb, uint32_t rua_ctx_id, - bool is_ps) +struct hnbgw_context_map *context_map_find_by_rua_ctx_id(struct hnb_context *hnb, uint32_t rua_ctx_id, bool is_ps) { struct hnbgw_context_map *map; - int new_scu_conn_id; - struct hnbgw_cnlink *cnlink; - struct hnbgw_sccp_user *hsu; llist_for_each_entry(map, &hnb->map_list, hnb_list) { if (map->is_ps != is_ps) @@ -77,51 +73,58 @@ /* Already exists */ return map; } + return NULL; +} - /* Does not exist yet, create a new hnbgw_context_map. */ +struct hnbgw_context_map *context_map_alloc(struct hnb_context *hnb, uint32_t rua_ctx_id, bool is_ps) +{ + struct hnbgw_context_map *map; - /* From the RANAP/RUA input, determine which cnlink to use */ - cnlink = hnbgw_cnlink_select(is_ps); - if (!cnlink) { - LOGHNB(hnb, DMAIN, LOGL_ERROR, "Failed to select CN link\n"); - return NULL; - } - - /* Allocate new SCCP conn id on the SCCP instance the cnlink is on. */ - hsu = cnlink->hnbgw_sccp_user; - if (!hsu) { - LOGHNB(hnb, DMAIN, LOGL_ERROR, "Cannot allocate context map: No SCCP instance for CN link %s\n", - cnlink->name); - return NULL; - } - - new_scu_conn_id = osmo_sccp_instance_next_conn_id(hsu->ss7->sccp); - if (new_scu_conn_id < 0) { - LOGHNB(hnb, DMAIN, LOGL_ERROR, "Unable to allocate SCCP conn ID on %s\n", hsu->name); - return NULL; - } - - /* allocate a new map entry. */ + /* allocate a new map entry now, so we have logging context */ map = talloc_zero(hnb, struct hnbgw_context_map); - map->cnlink = cnlink; map->hnb_ctx = hnb; map->rua_ctx_id = rua_ctx_id; map->is_ps = is_ps; - map->scu_conn_id = new_scu_conn_id; INIT_LLIST_HEAD(&map->ps_rab_ass); INIT_LLIST_HEAD(&map->ps_rabs); map_rua_fsm_alloc(map); - map_sccp_fsm_alloc(map); llist_add_tail(&map->hnb_list, &hnb->map_list); - llist_add_tail(&map->hnbgw_cnlink_entry, &cnlink->map_list); + + LOG_MAP(map, DRUA, LOGL_INFO, "New RUA CTX\n"); + return map; +} + +int context_map_set_cnlink(struct hnbgw_context_map *map, struct hnbgw_cnlink *cnlink_selected) +{ + int new_scu_conn_id; + struct hnbgw_sccp_user *hsu; + + /* Allocate new SCCP conn id on the SCCP instance the cnlink is on. */ + hsu = cnlink_selected->hnbgw_sccp_user; + if (!hsu) { + LOG_MAP(map, DCN, LOGL_ERROR, "Cannot map RUA context to SCCP: No SCCP instance for CN link %s\n", + cnlink_selected->name); + return -EIO; + } + + new_scu_conn_id = osmo_sccp_instance_next_conn_id(hsu->ss7->sccp); + if (new_scu_conn_id < 0) { + LOG_MAP(map, DCN, LOGL_ERROR, "Unable to allocate SCCP conn ID on %s\n", hsu->name); + return new_scu_conn_id; + } + + map->cnlink = cnlink_selected; + map->scu_conn_id = new_scu_conn_id; + llist_add_tail(&map->hnbgw_cnlink_entry, &cnlink_selected->map_list); + hash_add(hsu->hnbgw_context_map_by_conn_id, &map->hnbgw_sccp_user_entry, new_scu_conn_id); - LOG_MAP(map, DMAIN, LOGL_INFO, "Creating new Mapping RUA CTX %u <-> SCU Conn ID %s/%u\n", - rua_ctx_id, hsu->name, new_scu_conn_id); + LOG_MAP(map, DMAIN, LOGL_INFO, "Creating new Mapping RUA CTX %u <-> SCU Conn ID %u to %s on %s\n", + map->rua_ctx_id, new_scu_conn_id, cnlink_selected->name, hsu->name); - return map; + return 0; } int _map_rua_dispatch(struct hnbgw_context_map *map, uint32_t event, struct msgb *ranap_msg, diff --git a/src/osmo-hnbgw/hnbgw_rua.c b/src/osmo-hnbgw/hnbgw_rua.c index 8157cfc..abda5ea 100644 --- a/src/osmo-hnbgw/hnbgw_rua.c +++ b/src/osmo-hnbgw/hnbgw_rua.c @@ -33,7 +33,7 @@ #include "asn1helpers.h" -#include <osmocom/hnbgw/hnbgw.h> +#include <osmocom/hnbgw/hnbgw_cn.h> #include <osmocom/hnbgw/hnbgw_ranap.h> #include <osmocom/rua/rua_common.h> #include <osmocom/rua/rua_ies_defs.h> @@ -189,6 +189,44 @@ return get_value_string(rua_procedure_code_names, val); } +static struct hnbgw_context_map *find_or_create_context_map(struct hnb_context *hnb, uint32_t rua_ctx_id, bool is_ps, + struct msgb *ranap_msg) +{ + struct hnbgw_context_map *map; + struct hnbgw_cnlink *cnlink; + + map = context_map_find_by_rua_ctx_id(hnb, rua_ctx_id, is_ps); + if (map) { + /* Already established SCCP link. Continue to use that. */ + return map; + } + + /* Establish a new context map. From the RUA Connect, extract a mobile identity, if any, and select a CN link + * based on an NRI found in the mobile identity, if any. */ + + /* Allocate a map for logging context */ + map = context_map_alloc(hnb, rua_ctx_id, is_ps); + OSMO_ASSERT(map); + + /* FUTURE: extract mobile identity and store in map-> */ + + cnlink = hnbgw_cnlink_select(map); + if (!cnlink) { + LOG_MAP(map, DCN, LOGL_ERROR, "Failed to select %s link\n", is_ps ? "IuPS" : "IuCS"); + context_map_free(map); + return NULL; + } + + if (context_map_set_cnlink(map, cnlink)) { + LOG_MAP(map, DCN, LOGL_ERROR, "Failed to establish link to %s\n", cnlink->name); + context_map_free(map); + return NULL; + } + + LOG_MAP(map, DCN, LOGL_INFO, "establishing SCCP link: selected %s\n", cnlink->name); + return map; +} + /* dispatch a RUA connection-oriented message received from a HNB to a context mapping's RUA FSM, so that it is * forwarded to the CN via SCCP connection-oriented messages. * Connectionless messages are handled in hnbgw_ranap_rx() instead, not here. */ @@ -224,7 +262,7 @@ memcpy(ranap_msg->l2h, data, len); } - map = context_map_find_or_create_by_rua_ctx_id(hnb, context_id, is_ps); + map = find_or_create_context_map(hnb, context_id, is_ps, ranap_msg); if (!map) { LOGHNB(hnb, DRUA, LOGL_ERROR, "Failed to create context map for %s: rx RUA %s with %u bytes RANAP data\n", -- To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33127 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hnbgw Gerrit-Branch: master Gerrit-Change-Id: Ifc5242547260154eb5aecd3a9d9c2aac8419e8db Gerrit-Change-Number: 33127 Gerrit-PatchSet: 2 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[M] Change in osmo-bts[master]: paging: parse PCUIF data indication outside of paging.c
by fixeria 05 Jun '23

05 Jun '23

Attention is currently required from: pespin, dexter. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/32733 ) Change subject: paging: parse PCUIF data indication outside of paging.c ...................................................................... Patch Set 5: (2 comments) File src/common/paging.c: https://gerrit.osmocom.org/c/osmo-bts/+/32733/comment/13119fcf_1b03b1df PS5, Line 273: uint8_t macblock_len In GSM all MAC blocks must have `length == GSM_MACBLOCK_LEN`. IMO, it should be responsibility of the calling function to ensure the correct length. I see no need to pass and enforce the length here in this function. Not critical though. File src/common/pcu_sock.c: https://gerrit.osmocom.org/c/osmo-bts/+/32733/comment/e73b003b_8a8d5a4a PS5, Line 679: memcpy(imsi, data_req->data, 3); `osmo_strlcpy(&imsi[0], data_req->data, sizeof(imsi))` - this looks more readable and requires no prior initialization of `imsi[]`. -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32733 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I9f3799916e8102bf1ce0f21891f2d24f43091f01 Gerrit-Change-Number: 32733 Gerrit-PatchSet: 5 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Mon, 05 Jun 2023 12:39:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in osmocom-bb[master]: fake_trx.py: remove SETSLOT based burst filtering
by fixeria 05 Jun '23

05 Jun '23

Attention is currently required from: laforge, pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/33165 ) Change subject: fake_trx.py: remove SETSLOT based burst filtering ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > I'm not sure I am following your explanation 100% but it sounds really like this kind of change. This sounds like a kludge on top of a kludge on top of a kludge. Well, the main problem here is indeed inability of fake_trx.py to generate NOPE.ind. I agree that it's more a kludge rather than a proper solution to make trxcon generate NOPE.req and convert them to NOPE.ind. But it was the quickest and the easiest solution [0] I came up with back in 2021, when adding TTCN-3 testcases for features like the interference reporting, which do rely on NOPE indications. [0] https://gerrit.osmocom.org/q/I1c7f1315b8ef44f651efd6a22fb5b854f65c0946 > What we shold be doing is make sure that the fake_trx setup looks more like a real setup - both towards trxcon as well as towards osmo-bts-trx. Ideally, yes. But I am afraid we have reached the limit of what we can do with this "quick testing hack" implementation. Back in 2020 I tried to implement [1] proper burst queueing a few years ago, but we ended up with serious performance regressions when running ttcn3-bts-test and had to revert [2] these patches. [1] https://gerrit.osmocom.org/q/Ie66ef9667dc8d156ad578ce324941a816c07c105 [2] https://gerrit.osmocom.org/q/If29b4f6718cbc8af18fe18a5e3eca3912e8af01e > Maybe the real solutoin is to replace fake_trx.py with an implementation that doesn't have scalability/performance issues, if needed in C? It's indeed an option. I am all for it, but this does not sound like something I could quickly hack on during one working day. This would definitely require several days of work. If you think it's worth spending time on this right now, I would be happy to start rewriting from Python to C. Not sure though given the current amount of pending work. While writing the above part I realized that making fake_trx behave like a real transceiver with its own clock and queues, we would *have* to use fn-advance in trxcon. This will make testing the MS side GPRS implementation impossible, because proper Uplink timings are crucial for proper RLC/MAC operation. IMO, for now we can live with "a kludge on top of a kludge on top of a kludge". All I am trying to achieve with this patchset is simplifying life for those (mostly Pau and me) who need to test patches quickly and conveniently, without having to connect to the physical MS SDR setup in Sysmocom's office. -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/33165 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: Ia42550d5c2d8b49efbdf8ef0ce46b26afd1c464e Gerrit-Change-Number: 33165 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Mon, 05 Jun 2023 12:20:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Gerrit-MessageType: comment

1 0

[S] Change in libosmocore[master]: gsm48_rest_octets: fix wrong value
by laforge 05 Jun '23

05 Jun '23

laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/33160 ) Change subject: gsm48_rest_octets: fix wrong value ...................................................................... gsm48_rest_octets: fix wrong value The compiler complains: gsm48_rest_octets.c:79:11: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides] 79 | [0x08] = {2, 2}, | ^~~~~~~ gsm48_rest_octets.c:78:11: note: previous initialization is here 78 | [0x08] = {2, 1}, | ^~~~~~~ And a quick look at 04.18 confirms: 0 1 0 0 0 2 1 0 1 0 0 1 2 2 Change-Id: I672e2ce53fd07157d15fec350cfbe32a19a52941 --- M src/gsm/gsm48_rest_octets.c 1 file changed, 25 insertions(+), 1 deletion(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, but someone else must approve fixeria: Looks good to me, approved diff --git a/src/gsm/gsm48_rest_octets.c b/src/gsm/gsm48_rest_octets.c index b63c63f..57c2c99 100644 --- a/src/gsm/gsm48_rest_octets.c +++ b/src/gsm/gsm48_rest_octets.c @@ -76,7 +76,7 @@ [0x07] = {2, 0}, [0x08] = {2, 1}, - [0x08] = {2, 2}, + [0x09] = {2, 2}, [0x0a] = {2, 3}, [0x0b] = {2, 4}, [0x0c] = {2, 5}, -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/33160 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I672e2ce53fd07157d15fec350cfbe32a19a52941 Gerrit-Change-Number: 33160 Gerrit-PatchSet: 1 Gerrit-Owner: Hoernchen <ewild(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-MessageType: merged

1 0

[S] Change in osmocom-bb[master]: trxcon: do not advance Uplink TDMA Fn by default
by laforge 05 Jun '23

05 Jun '23

Attention is currently required from: fixeria. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/33166 ) Change subject: trxcon: do not advance Uplink TDMA Fn by default ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/33166 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: I838b1ebc54e4c5d116f8af2155d97215a6133ba4 Gerrit-Change-Number: 33166 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Mon, 05 Jun 2023 11:31:37 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in osmocom-bb[master]: trxcon: get rid of the timer driven clock module
by laforge 05 Jun '23

05 Jun '23

Attention is currently required from: fixeria, pespin. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/32575 ) Change subject: trxcon: get rid of the timer driven clock module ...................................................................... Patch Set 3: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/32575 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: Ic8a5b6277c6b16392026e0557376257d71c9d230 Gerrit-Change-Number: 32575 Gerrit-PatchSet: 3 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: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Mon, 05 Jun 2023 11:30:51 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in osmocom-bb[master]: fake_trx.py: remove SETSLOT based burst filtering
by laforge 05 Jun '23

05 Jun '23

Attention is currently required from: fixeria. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/33165 ) Change subject: fake_trx.py: remove SETSLOT based burst filtering ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: I'm not sure I am following your explanation 100% but it sounds really like this kind of change. This sounds like a kludge on top of a kludge on top of a kludge. What we shold be doing is make sure that the fake_trx setup looks more like a real setup - both towards trxcon as well as towards osmo-bts-trx. Maybe the real solutoin is to replace fake_trx.py with an implementation that doesn't have scalability/performance issues, if needed in C? -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/33165 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: Ia42550d5c2d8b49efbdf8ef0ce46b26afd1c464e Gerrit-Change-Number: 33165 Gerrit-PatchSet: 1 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <laforge(a)osmocom.org> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Comment-Date: Mon, 05 Jun 2023 11:29:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[L] Change in osmocom-bb[master]: trxcon: get rid of the timer driven clock module
by fixeria 04 Jun '23

04 Jun '23

Attention is currently required from: laforge, pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/32575 ) Change subject: trxcon: get rid of the timer driven clock module ...................................................................... Patch Set 3: (3 comments) File src/host/trxcon/src/trx_if.c: https://gerrit.osmocom.org/c/osmocom-bb/+/32575/comment/4c47b592_63016590 PS2, Line 707: .fn = bi.fn, > the trx_fn_advance may still make sense to be possible to apply it here? I like the idea, thanks! Implemented in the new patchset. https://gerrit.osmocom.org/c/osmocom-bb/+/32575/comment/27b22f13_1b5ac0ef PS2, Line 711: trxcon_phyif_handle_rts_ind(trx->priv, &rts); > does that mean that now if we don't receive DL data, we'll never to UL RTS.ind? Correct. > Is that supposed to work because the BTS is expected to always send data? Ack. File src/host/trxcon/src/trxcon_main.c: https://gerrit.osmocom.org/c/osmocom-bb/+/32575/comment/be6edbc7_acd9a0a6 PS2, Line 244: XXX: no-op > Ack Not needed, this param will remain operational. -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/32575 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: Ic8a5b6277c6b16392026e0557376257d71c9d230 Gerrit-Change-Number: 32575 Gerrit-PatchSet: 3 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: 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: Sun, 04 Jun 2023 21:31:11 +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

1 0

[L] Change in osmocom-bb[master]: trxcon: get rid of the timer driven clock module
by fixeria 04 Jun '23

04 Jun '23

Attention is currently required from: laforge, fixeria. Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmocom-bb/+/32575 to look at the new patch set (#3). Change subject: trxcon: get rid of the timer driven clock module ...................................................................... trxcon: get rid of the timer driven clock module trxcon was heavily inspired by osmo-bts-trx, and among with many other scheduling related parts also inherited the timer driven clock module. This clock module is driving the Uplink burst scheduling, just like it does drive the Downlink burst scheduling in osmo-bts-trx. Just like in osmo-bts-trx, the clock module relies on periodic CLCK indications from the PHY, which are needed to compensate for the clock drifting. The key difference is that trxcon is using Downlink bursts as the CLCK indications, see 'bi.fn % 51' in trx_data_rx_cb(). This is possible because the MS is a clock slave of the BTS: the MS PHY needs to sync its freq. and clock first, and only after that it can Rx and Tx. So far we've had no problems with the clock module in trxcon until we started adding GPRS support and integrated the l1gprs. While the CS domain is quite flexible in terms of timings and delays, the PS domain is a lot more sensetive to the timing issues. Sometimes it happens that the trxcon's clock module is ticking quicker than it should, resulting in Uplink PDCH blocks being scheduled earlier than the respective Downlink PDCH blocks are received: 20230502021957724 l1sched_pull_burst(): PDTCH/U Tx time (fn=56103) 20230502021957744 (PDCH-7) Rx DL BLOCK.ind (fn=56103, len=23): ... 20230502021957747 l1sched_pull_burst(): PDTCH/U Tx time (fn=56108) 20230502021957765 l1sched_pull_burst(): PDTCH/U Tx time (fn=56112) 20230502021957767 (PDCH-7) Rx DL BLOCK.ind (fn=56108, len=23): ... 20230502021957768 (PDCH-7) Rx UL BLOCK.req (fn=56112, len=54): ... 20230502021957784 l1sched_pull_burst(): PDTCH/U Tx time (fn=56116) 20230502021957784 TS7-PDTCH dropping Tx primitive (current Fn=56116, prim Fn=56112) This is impossible in reality, because Uplink is intentionally lagging behind Downlink by 3 TDMA timeslot periods. In a virtual setup this causes sporadic dropping of Uplink PDCH blocks, as can be seen from the logging snippet above, and significantly degrades the RLC/MAC performance for GPRS. Let's remove the internal clock module and trigger the Uplink burst transmission each time we receive a Downlink burst. This helps to overcome the GPRS scheduling issues and replicates the approach of osmo-trx-ms more closely. Change-Id: Ic8a5b6277c6b16392026e0557376257d71c9d230 Related: OS#5500 --- M src/host/trxcon/include/osmocom/bb/l1sched/l1sched.h M src/host/trxcon/include/osmocom/bb/trxcon/phyif.h M src/host/trxcon/include/osmocom/bb/trxcon/trx_if.h M src/host/trxcon/include/osmocom/bb/trxcon/trxcon.h M src/host/trxcon/src/Makefile.am D src/host/trxcon/src/sched_clck.c M src/host/trxcon/src/sched_trx.c M src/host/trxcon/src/trx_if.c M src/host/trxcon/src/trxcon_inst.c M src/host/trxcon/src/trxcon_main.c M src/host/trxcon/src/trxcon_shim.c 11 files changed, 68 insertions(+), 263 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmocom-bb refs/changes/75/32575/3 -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/32575 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: Ic8a5b6277c6b16392026e0557376257d71c9d230 Gerrit-Change-Number: 32575 Gerrit-PatchSet: 3 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: 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-MessageType: newpatchset

1 0