Attention is currently required from: pespin. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816 ) Change subject: add ps_rab_ass FSM to map GTP via UPF ...................................................................... Patch Set 1: (11 comments) File include/osmocom/hnbgw/context_map.h: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/5f3a5fe8_9670b6ad PS1, Line 56: * and each Response gets one ps_rab_ass FSM. Each such message can contain any number and any combination of one each. tried to think of an ascii art way to draw the relations because it is strugglesome to explain in words ... first i tried to have one FSM for req + resp + PFCP for all RABs, then gradually noticed that each RAB needs a separate PFCP FSM, and then that i also need to have separate FSMs for RANAP req and resp. File include/osmocom/hnbgw/ps_rab_fsm.h: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/49c62d11_8063c541 PS1, Line 24: struct half_gtp_map { gtp_tunnel would be accurate, except that the FAR (Forwarding Action Rule) is closely related to the *other* GTP tunnel. I'll expand the comment. https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/8ec7ec53_37a06b83 PS1, Line 36: /* Whether the RANAP message this RAB's remote address was obtained from encoded the address in x213_nsap */ it is accurate but i'll rephrase to less ambiguous grammar https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/80c397c7_60c89080 PS1, Line 54: /* Backpointer to the ps_rab_ass_fsm for the RAB Assignment Request from Core that created this RAB. */ commenting in code... File src/osmo-hnbgw/hnbgw.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/2bc0f340_b5d5eb4d PS1, Line 230: LOGP(DHNBAP, LOGL_INFO, "deallocating UE context: id 0x%x, imsi %s, tmsi 0x%x\n", missed this one, i think i needed it for debugging, could just drop it File src/osmo-hnbgw/hnbgw_cn.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/0b5c7053_b1792c48 PS1, Line 373: if (!map->is_ps) { we (i?) usually list CS above PS ... so the is_ps maybe should have been named is_cs instead; i agree that it is slightly annoying, but i've made peace with that over the years (this line is unchanged from before this patch despite gerrit rendering on green background, I'm only adding an 'else') https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/6c9af076_7cd2ecf3 PS1, Line 396: rc = ranap_ran_rx_co_decode(map, message, msgb_l2(oph->msg), msgb_l2len(oph->msg)); i think i even considered that when writing the patch, but left it like this to show that the CS code is completely unchanged... but true. i'll remove the code dup in a separate patch File src/osmo-hnbgw/hnbgw_pfcp.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/6254cfaf_7516e07d PS1, Line 77: if (!hnb_gw_is_gtp_mapping_enabled(hnb_gw)) { that's a matter of taste ... personally i like it very much when things are completely contained in a single file, with the parts appearing in main() as minimal as possible. but i see the LOGP above seems at the wrong place File src/osmo-hnbgw/hnbgw_rua.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/3b1f3a66_383eaeb2 PS1, Line 284: if (!map->is_ps) { idk i like CS above PS better, also before this patch there was "!is_ps" above File src/osmo-hnbgw/hnbgw_vty.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/975c18f5_a6158113 PS1, Line 474: if (g_hnb_gw->config.pfcp.local_addr) same order here as where the cmds are added to vty, but ok whatever File src/osmo-hnbgw/ps_rab_ass_fsm.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/4eec37a5_8a12da06 PS1, Line 490: /* See whether all RABs are done establishing, and replace RTP info in the RAB Assignment Response message, so that we lol thx -- To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hnbgw Gerrit-Branch: master Gerrit-Change-Id: Ic9bc30f322c4c6c6e82462d1da50cb15b336c63a Gerrit-Change-Number: 28816 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Thu, 28 Jul 2022 10:03:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment