Attention is currently required from: osmith.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-gprs/+/33811 )
Change subject: sm: Handle GMMSM-MODIFY.ind primitive
......................................................................
Patch Set 6:
(2 comments)
File include/osmocom/gprs/sm/sm_private.h:
https://gerrit.osmocom.org/c/libosmo-gprs/+/33811/comment/5f3b91e3_a2d2bab8
PS6, Line 159: struct osmo_gprs_sm_prim *gprs_sm_prim_alloc_smreg_pdp_deact_ind(void);
> should everything about deact_ind go into a separate patch? or how does it relate to GMMSM-MODIFY. […]
It's just part of the handling of GMMSM-MODIFY.ind, see the rest of the patch.
In summary, GMMSM-MODIFY.ind is trigged by rx GMM RAU Accept, which contains a PDP Context Status IE, which may trigger in the SM layer the deactivation of PDP context, which are then to be announced to the upper layers (through the SMREG interface).
File src/sm/sm_prim.c:
https://gerrit.osmocom.org/c/libosmo-gprs/+/33811/comment/c79ed4a4_bd54e9dd
PS6, Line 603: */
> did you leave this and the TODO below intentionally?
Yes, this is not really needed for now since it's for acknowledged mode, which afaik we are not even implementing on the network side.
That's really another feature which may be implemented in the future. I'm just leaving the TODO there since I was looking at related specs.
--
To view, visit https://gerrit.osmocom.org/c/libosmo-gprs/+/33811
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-gprs
Gerrit-Branch: master
Gerrit-Change-Id: Ic765b7a565cac4abcf34d8c6868e103971d17822
Gerrit-Change-Number: 33811
Gerrit-PatchSet: 6
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 20 Jul 2023 11:10:47 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: osmith <osmith(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: pespin.
osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmo-gprs/+/33811 )
Change subject: sm: Handle GMMSM-MODIFY.ind primitive
......................................................................
Patch Set 6:
(2 comments)
File include/osmocom/gprs/sm/sm_private.h:
https://gerrit.osmocom.org/c/libosmo-gprs/+/33811/comment/e6045034_aa105b0b
PS6, Line 159: struct osmo_gprs_sm_prim *gprs_sm_prim_alloc_smreg_pdp_deact_ind(void);
should everything about deact_ind go into a separate patch? or how does it relate to GMMSM-MODIFY.ind?
File src/sm/sm_prim.c:
https://gerrit.osmocom.org/c/libosmo-gprs/+/33811/comment/d68da27d_3ea25019
PS6, Line 603: */
did you leave this and the TODO below intentionally?
--
To view, visit https://gerrit.osmocom.org/c/libosmo-gprs/+/33811
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-gprs
Gerrit-Branch: master
Gerrit-Change-Id: Ic765b7a565cac4abcf34d8c6868e103971d17822
Gerrit-Change-Number: 33811
Gerrit-PatchSet: 6
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 20 Jul 2023 11:05:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: dexter.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/33844 )
Change subject: nacc_fsm: fix uninitialized neigh_key variable
......................................................................
Patch Set 2:
(1 comment)
File src/nacc_fsm.c:
https://gerrit.osmocom.org/c/osmo-pcu/+/33844/comment/82799e82_515a4976
PS2, Line 279: struct nacc_fsm_ctx *ctx)
this should be const, it's only used for logging.
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33844
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I7e74beda03829d909b6542659316241c275a36b3
Gerrit-Change-Number: 33844
Gerrit-PatchSet: 2
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: Jenkins Builder
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 20 Jul 2023 10:44:23 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: pespin.
Hello pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmo-pcu/+/33844
to look at the new patch set (#2).
Change subject: nacc_fsm: fix uninitialized neigh_key variable
......................................................................
nacc_fsm: fix uninitialized neigh_key variable
in handle_retrans_pkt_cell_chg_notif, the variable neigh_key is used
uninitialized at end of the function. This has been introduced with
Change I96280f0ec5955ed3cb17641bf4118496c929bdac, where we modified
fill_neigh_key_from_bts_pkt_cell_chg()_not so that it write directly
at the ctx variable. This works in st_initial but not in
handle_retrans_pkt_cell_chg_notif(), so let's have neigh_key and
neigh_key_present as a parameter so that the caller can decide where
the result is stored.
Fixes: CID#322150
Related: OS#6100
Change-Id: I7e74beda03829d909b6542659316241c275a36b3
---
M src/nacc_fsm.c
1 file changed, 42 insertions(+), 16 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/44/33844/2
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33844
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I7e74beda03829d909b6542659316241c275a36b3
Gerrit-Change-Number: 33844
Gerrit-PatchSet: 2
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: Jenkins Builder
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: newpatchset
dexter has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-pcu/+/33844 )
Change subject: nacc_fsm: fix uninitialized neigh_key variable
......................................................................
nacc_fsm: fix uninitialized neigh_key variable
in handle_retrans_pkt_cell_chg_notif, the variable neigh_key is used
uninitialized at end of the function. This has been introduced with
Change I96280f0ec5955ed3cb17641bf4118496c929bdac, where we modified
fill_neigh_key_from_bts_pkt_cell_chg()_not so that it write directly
at the ctx variable. This works in st_initial but not in
handle_retrans_pkt_cell_chg_notif(), so let's have neigh_key and
neigh_key_present as a parameter so that the caller can decide where
the result is stored.
Fixes: CID#322150
Related: OS#6100
Change-Id: I7e74beda03829d909b6542659316241c275a36b3
---
M src/nacc_fsm.c
1 file changed, 42 insertions(+), 16 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/44/33844/1
diff --git a/src/nacc_fsm.c b/src/nacc_fsm.c
index 6ccfee9..35aa16d 100644
--- a/src/nacc_fsm.c
+++ b/src/nacc_fsm.c
@@ -270,16 +270,20 @@
return 0;
}
-static int fill_neigh_key_from_bts_pkt_cell_chg_not(struct nacc_fsm_ctx *ctx,
+/* Generate neighbour key information from a given PacketCellCHangeNotification and a BTS object (lac and cid). The
+ * ctx variable is used for logging only. */
+static int fill_neigh_key_from_bts_pkt_cell_chg_not(struct neigh_cache_entry_key *neigh_key,
+ bool *neigh_key_present,
const struct gprs_rlcmac_bts *bts,
- const Packet_Cell_Change_Notification_t *notif)
+ const Packet_Cell_Change_Notification_t *notif,
+ struct nacc_fsm_ctx *ctx)
{
const Target_Cell_GSM_Notif_t *notif_gsm;
const Target_Cell_3G_Notif_t *notif_3g;
const Target_Cell_4G_Notif_t *notif_4g;
memset(&ctx->neigh_key, 0, sizeof(ctx->neigh_key));
- ctx->neigh_key_present = false;
+ *neigh_key_present = false;
switch (notif->Target_Cell.UnionType) {
case 0: /* GSM */
@@ -287,11 +291,11 @@
LOGPFSML(ctx->fi, LOGL_NOTICE, "TargetCell: RAT=GSM, ARFCN=%u, BSIC=%u\n",
notif_gsm->ARFCN, notif_gsm->BSIC);
- ctx->neigh_key.local_lac = bts->cgi_ps.rai.lac.lac;
- ctx->neigh_key.local_ci = bts->cgi_ps.cell_identity;
- ctx->neigh_key.tgt_arfcn = notif_gsm->ARFCN;
- ctx->neigh_key.tgt_bsic = notif_gsm->BSIC;
- ctx->neigh_key_present = true;
+ neigh_key->local_lac = bts->cgi_ps.rai.lac.lac;
+ neigh_key->local_ci = bts->cgi_ps.cell_identity;
+ neigh_key->tgt_arfcn = notif_gsm->ARFCN;
+ neigh_key->tgt_bsic = notif_gsm->BSIC;
+ *neigh_key_present = true;
return 0;
default:
switch (notif->Target_Cell.u.Target_Other_RAT_Notif.UnionType) {
@@ -316,11 +320,11 @@
if (notif_4g->Exist_Arfcn) {
LOGPFSML(ctx->fi, LOGL_NOTICE, "TargetCell: RAT=GSM, ARFCN=%u, BSIC=%u\n",
notif_4g->Arfcn, notif_4g->bsic);
- ctx->neigh_key.local_lac = bts->cgi_ps.rai.lac.lac;
- ctx->neigh_key.local_ci = bts->cgi_ps.cell_identity;
- ctx->neigh_key.tgt_arfcn = notif_4g->Arfcn;
- ctx->neigh_key.tgt_bsic = notif_4g->bsic;
- ctx->neigh_key_present = true;
+ neigh_key->local_lac = bts->cgi_ps.rai.lac.lac;
+ neigh_key->local_ci = bts->cgi_ps.cell_identity;
+ neigh_key->tgt_arfcn = notif_4g->Arfcn;
+ neigh_key->tgt_bsic = notif_4g->bsic;
+ *neigh_key_present = true;
return 0;
}
if (notif_4g->Exist_3G_Target_Cell) {
@@ -387,15 +391,16 @@
{
struct gprs_rlcmac_bts *bts = ctx->ms->bts;
struct neigh_cache_entry_key neigh_key;
+ bool neigh_key_present;
int rc;
- rc = fill_neigh_key_from_bts_pkt_cell_chg_not(ctx, bts, notif);
+ rc = fill_neigh_key_from_bts_pkt_cell_chg_not(&neigh_key, &neigh_key_present, bts, notif, ctx);
if (rc < 0) {
/* (see comment below) */
if (ctx->fi->state != NACC_ST_TX_CELL_CHG_CONTINUE)
nacc_fsm_state_chg(ctx->fi, NACC_ST_TX_CELL_CHG_CONTINUE);
return;
- } else if (!ctx->neigh_key_present) {
+ } else if (!neigh_key_present) {
/* In case no neighbour key information is present, (This would be the case for UTRAN or EUTRAN cells)
* then we will not provide any system information. Instead we will send the PacketCellChangeContinue
* message immediately. This also applies in the case of re-transmissions. See also: 3GPP TS 48.018,
@@ -407,6 +412,7 @@
/* If tgt cell changed, restart resolving it */
if (!neigh_cache_entry_key_eq(&ctx->neigh_key, &neigh_key)) {
ctx->neigh_key = neigh_key;
+ ctx->neigh_key_present = neigh_key_present;
nacc_fsm_state_chg(ctx->fi, NACC_ST_WAIT_RESOLVE_RAC_CI);
}
/* else: ignore it, it's a dup, carry on what we were doing */
@@ -426,7 +432,7 @@
switch (event) {
case NACC_EV_RX_CELL_CHG_NOTIFICATION:
notif = (Packet_Cell_Change_Notification_t *)data;
- rc = fill_neigh_key_from_bts_pkt_cell_chg_not(ctx, bts, notif);
+ rc = fill_neigh_key_from_bts_pkt_cell_chg_not(&ctx->neigh_key, &ctx->neigh_key_present,bts, notif, ctx);
if (rc < 0)
osmo_fsm_inst_term(fi, OSMO_FSM_TERM_ERROR, NULL);
else if (!ctx->neigh_key_present) {
--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/33844
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: I7e74beda03829d909b6542659316241c275a36b3
Gerrit-Change-Number: 33844
Gerrit-PatchSet: 1
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: newchange