Attention is currently required from: neels. pespin 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: (20 comments) File include/osmocom/hnbgw/context_map.h: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/78c4e6a7_c816e36b PS1, Line 56: * and each Response gets one ps_rab_ass FSM. Each such message can contain any number and any combination of you mean each req+response pair has a ps_rab_ass? or one for each? File include/osmocom/hnbgw/hnbgw.h: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/3164de7d_a9d87d48 PS1, Line 193: static inline bool hnb_gw_is_gtp_mapping_enabled(const struct hnb_gw *gw) can you document this function? what does gtp_mapping mean exactly? File include/osmocom/hnbgw/ps_rab_fsm.h: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/46e3ee4d_7f5355d8 PS1, Line 8: struct addr_teid { Maybe add a one line documenttion. Is this for GTP? gtp_addr_teid? https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/072a90fe_91559445 PS1, Line 14: /* Two of this make up a GTP mapping, one for the Access (HNB) side, one for the Core (CN) side. what does "this" mean here? https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/5545552b_6c00f4cb PS1, Line 24: struct half_gtp_map { gtp_map_leg? to use some term more "usual". https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/6918eb71_7d6d4276 PS1, Line 36: /* Whether the RANAP message this RAB's remote address was obtained from encoded the address in x213_nsap */ "from encoded the address" looks wrongly written? https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/599123b2_f33523c9 PS1, Line 41: struct osmo_fsm_inst *fi; document the type of fsm? because I see 2 more "fi" below. https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/62222c6a_ed79e330 PS1, Line 54: /* Backpointer to the ps_rab_ass_fsm for the RAB Assignment Request from Core that created this RAB. */ (I wonder why do we need 2 different FSMs here, looks like the same followup procedure to me, req + resp). https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/44ff19a5_495ee9d2 PS1, Line 79: bool ps_rab_is_established(struct ps_rab *rab); constify param File src/osmo-hnbgw/hnbgw.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/3f1ebd77_b41cb27a PS1, Line 230: LOGP(DHNBAP, LOGL_INFO, "deallocating UE context: id 0x%x, imsi %s, tmsi 0x%x\n", this can probably go on a different commit. https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/f793fa67_08fc6623 PS1, Line 354: LOGP(DMAIN, LOGL_INFO, "Deallocating hnb_context\n"); this can probably go on a different commit. File src/osmo-hnbgw/hnbgw_cn.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/21909278_7fd593a4 PS1, Line 373: if (!map->is_ps) { not super important, but probably makes it easier to read if you do "if (map->is_ps)" now, and move the existing code to the "else" condition. https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/97e22bf5_e60ace6f PS1, Line 396: rc = ranap_ran_rx_co_decode(map, message, msgb_l2(oph->msg), msgb_l2len(oph->msg)); You are actually duplicating these 2 lines now. They can be moved outside the if/else condition. The "if (rc == 0)" and ranap_ran_rx_co_free(message); can also be in the single path. File src/osmo-hnbgw/hnbgw_pfcp.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/997a3c13_30542dc8 PS1, Line 77: if (!hnb_gw_is_gtp_mapping_enabled(hnb_gw)) { I'd move this to the main function, to avoid calling hnbgw_pfcp_init(). I think for readers will be easier to understand the whole thing is disabled by this when looking at the main file. File src/osmo-hnbgw/hnbgw_rua.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/53c609a4_c6c59aec PS1, Line 284: if (!map->is_ps) { same, probably easier to read with "if (map->is_ps)" File src/osmo-hnbgw/hnbgw_vty.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/644d88f9_101a9297 PS1, Line 474: if (g_hnb_gw->config.pfcp.local_addr) I think we usually print first local addresses, then remote addresses. File src/osmo-hnbgw/ps_rab_ass_fsm.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/02a7a95c_0e96ce3e PS1, Line 490: /* See whether all RABs are done establishing, and replace RTP info in the RAB Assignment Response message, so that we s/RTP/GTP/ https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/5e0bc258_c7d0befb PS1, Line 625: void hnbgw_gtpmap_release(struct hnbgw_context_map *map) I'm reading "gsmtap" all the time ;) File src/osmo-hnbgw/ps_rab_fsm.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/82146a8a_df878e97 PS1, Line 786: .name = "PS_RAB_ST_WAIT_PFCP_DEL_RESP", WAIT_PFCP_DEL_RESP https://gerrit.osmocom.org/c/osmo-hnbgw/+/28816/comment/f152c4db_1bf98735 PS1, Line 797: .name = "PS_RAB_ST_WAIT_USE_COUNT", WAIT_USE_COUNT -- 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: neels <nhofmeyr(a)sysmocom.de> Gerrit-Comment-Date: Wed, 27 Jul 2022 14:42:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment