From c4a17d35ecea72c0b50bc1ac030a6693f8be8262 Mon Sep 17 00:00:00 2001 From: Pau Espin Pedrol Date: Mon, 3 Apr 2023 18:38:55 +0200 Subject: [PATCH] nm: Apply BTS/TRX/TS OML Attributes through NM FSMs This way we have further control on how to handle the SetAttr meessages received. For instance, NACK them if the NM object FSMs are not at the expected correct state. The originating msgs are now kept owned and freed by the OML layer (oml.c), and the NM FSMs only uses them and create new OML msgb when answering with ACK/NACK. Related: OS#5992 Change-Id: Id68868e25bbf96227ab6459fcd3c9181852ed28e --- include/osmo-bts/nm_common_fsm.h | 6 ++-- include/osmo-bts/oml.h | 1 + src/common/nm_bb_transc_fsm.c | 2 +- src/common/nm_bts_fsm.c | 10 ++++++- src/common/nm_bts_sm_fsm.c | 2 +- src/common/nm_channel_fsm.c | 20 +++++++++++-- src/common/nm_common_fsm.c | 1 + src/common/nm_radio_carrier_fsm.c | 10 ++++++- src/common/oml.c | 48 +++++++++++++++++++++++++++---- src/osmo-bts-lc15/oml.c | 3 +- src/osmo-bts-oc2g/oml.c | 3 +- src/osmo-bts-octphy/l1_oml.c | 3 +- src/osmo-bts-omldummy/bts_model.c | 3 +- src/osmo-bts-sysmo/oml.c | 3 +- src/osmo-bts-trx/l1_if.c | 3 +- src/osmo-bts-virtual/bts_model.c | 3 +- 16 files changed, 93 insertions(+), 28 deletions(-) diff --git a/include/osmo-bts/nm_common_fsm.h b/include/osmo-bts/nm_common_fsm.h index 1f0accc5a..2363779f1 100644 --- a/include/osmo-bts/nm_common_fsm.h +++ b/include/osmo-bts/nm_common_fsm.h @@ -30,6 +30,7 @@ /* Common */ enum nm_fsm_events { NM_EV_SW_ACT, + NM_EV_RX_SETATTR, /* data: struct nm_fsm_ev_setattr_data */ NM_EV_SETATTR_ACK, /* data: struct nm_fsm_ev_setattr_data */ NM_EV_SETATTR_NACK, /* data: struct nm_fsm_ev_setattr_data */ NM_EV_OPSTART_ACK, @@ -50,8 +51,9 @@ enum nm_fsm_events { extern const struct value_string nm_fsm_event_names[]; struct nm_fsm_ev_setattr_data { - struct msgb *msg; /* msgb ownership is transferred to FSM */ - int cause; + struct msgb *msg; + struct tlv_parsed *tp; + int cause; /* set in NM_EV_SETATTR_(N)ACK */ }; diff --git a/include/osmo-bts/oml.h b/include/osmo-bts/oml.h index 90c907700..c59f6ba29 100644 --- a/include/osmo-bts/oml.h +++ b/include/osmo-bts/oml.h @@ -63,6 +63,7 @@ int oml_tx_state_changed(const struct gsm_abis_mo *mo); int oml_mo_tx_sw_act_rep(const struct gsm_abis_mo *mo); int oml_fom_ack_nack(struct msgb *old_msg, uint8_t cause); +int oml_fom_ack_nack_copy_msg(const struct msgb *old_msg, uint8_t cause); int oml_mo_fom_ack_nack(const struct gsm_abis_mo *mo, uint8_t orig_msg_type, uint8_t cause); diff --git a/src/common/nm_bb_transc_fsm.c b/src/common/nm_bb_transc_fsm.c index ca78256ce..82ef89dcd 100644 --- a/src/common/nm_bb_transc_fsm.c +++ b/src/common/nm_bb_transc_fsm.c @@ -128,7 +128,7 @@ static void st_op_disabled_offline(struct osmo_fsm_inst *fi, uint32_t event, voi case NM_EV_SETATTR_NACK: setattr_data = (struct nm_fsm_ev_setattr_data *)data; bb_transc->mo.setattr_success = setattr_data->cause == 0; - oml_fom_ack_nack(setattr_data->msg, setattr_data->cause); + oml_fom_ack_nack_copy_msg(setattr_data->msg, setattr_data->cause); break; case NM_EV_OPSTART_ACK: bb_transc->mo.opstart_success = true; diff --git a/src/common/nm_bts_fsm.c b/src/common/nm_bts_fsm.c index ea3a6c3a1..8ff57feb7 100644 --- a/src/common/nm_bts_fsm.c +++ b/src/common/nm_bts_fsm.c @@ -111,13 +111,20 @@ static void st_op_disabled_offline(struct osmo_fsm_inst *fi, uint32_t event, voi { struct gsm_bts *bts = (struct gsm_bts *)fi->priv; struct nm_fsm_ev_setattr_data *setattr_data; + int rc; switch (event) { + case NM_EV_RX_SETATTR: + setattr_data = (struct nm_fsm_ev_setattr_data *)data; + rc = bts_model_apply_oml(bts, setattr_data->msg, setattr_data->tp, + NM_OC_BTS, bts); + (void)rc; + break; case NM_EV_SETATTR_ACK: case NM_EV_SETATTR_NACK: setattr_data = (struct nm_fsm_ev_setattr_data *)data; bts->mo.setattr_success = setattr_data->cause == 0; - oml_fom_ack_nack(setattr_data->msg, setattr_data->cause); + oml_fom_ack_nack_copy_msg(setattr_data->msg, setattr_data->cause); break; case NM_EV_OPSTART_ACK: bts->mo.opstart_success = true; @@ -178,6 +185,7 @@ static struct osmo_fsm_state nm_bts_fsm_states[] = { }, [NM_BTS_ST_OP_DISABLED_OFFLINE] = { .in_event_mask = + X(NM_EV_RX_SETATTR) | X(NM_EV_SETATTR_ACK) | X(NM_EV_SETATTR_NACK) | X(NM_EV_OPSTART_ACK) | diff --git a/src/common/nm_bts_sm_fsm.c b/src/common/nm_bts_sm_fsm.c index 2d433153f..66c40770c 100644 --- a/src/common/nm_bts_sm_fsm.c +++ b/src/common/nm_bts_sm_fsm.c @@ -92,7 +92,7 @@ static void st_op_disabled_offline(struct osmo_fsm_inst *fi, uint32_t event, voi case NM_EV_SETATTR_NACK: setattr_data = (struct nm_fsm_ev_setattr_data *)data; site_mgr->mo.setattr_success = setattr_data->cause == 0; - oml_fom_ack_nack(setattr_data->msg, setattr_data->cause); + oml_fom_ack_nack_copy_msg(setattr_data->msg, setattr_data->cause); break; case NM_EV_OPSTART_ACK: site_mgr->mo.opstart_success = true; diff --git a/src/common/nm_channel_fsm.c b/src/common/nm_channel_fsm.c index 503ddfb9b..557d5b985 100644 --- a/src/common/nm_channel_fsm.c +++ b/src/common/nm_channel_fsm.c @@ -94,13 +94,20 @@ static void st_op_disabled_dependency(struct osmo_fsm_inst *fi, uint32_t event, { struct gsm_bts_trx_ts *ts = (struct gsm_bts_trx_ts *)fi->priv; struct nm_fsm_ev_setattr_data *setattr_data; + int rc; switch (event) { + case NM_EV_RX_SETATTR: + setattr_data = (struct nm_fsm_ev_setattr_data *)data; + rc = bts_model_apply_oml(ts->trx->bts, setattr_data->msg, setattr_data->tp, + NM_OC_CHANNEL, ts); + (void)rc; + break; case NM_EV_SETATTR_ACK: case NM_EV_SETATTR_NACK: setattr_data = (struct nm_fsm_ev_setattr_data *)data; ts->mo.setattr_success = setattr_data->cause == 0; - oml_fom_ack_nack(setattr_data->msg, setattr_data->cause); + oml_fom_ack_nack_copy_msg(setattr_data->msg, setattr_data->cause); break; case NM_EV_OPSTART_ACK: LOGPFSML(fi, LOGL_NOTICE, "BSC trying to activate TS while still in avail=dependency. " @@ -138,13 +145,20 @@ static void st_op_disabled_offline(struct osmo_fsm_inst *fi, uint32_t event, voi { struct gsm_bts_trx_ts *ts = (struct gsm_bts_trx_ts *)fi->priv; struct nm_fsm_ev_setattr_data *setattr_data; + int rc; switch (event) { + case NM_EV_RX_SETATTR: + setattr_data = (struct nm_fsm_ev_setattr_data *)data; + rc = bts_model_apply_oml(ts->trx->bts, setattr_data->msg, setattr_data->tp, + NM_OC_CHANNEL, ts); + (void)rc; + break; case NM_EV_SETATTR_ACK: case NM_EV_SETATTR_NACK: setattr_data = (struct nm_fsm_ev_setattr_data *)data; ts->mo.setattr_success = setattr_data->cause == 0; - oml_fom_ack_nack(setattr_data->msg, setattr_data->cause); + oml_fom_ack_nack_copy_msg(setattr_data->msg, setattr_data->cause); break; case NM_EV_OPSTART_ACK: ts->mo.opstart_success = true; @@ -220,6 +234,7 @@ static struct osmo_fsm_state nm_chan_fsm_states[] = { }, [NM_CHAN_ST_OP_DISABLED_DEPENDENCY] = { .in_event_mask = + X(NM_EV_RX_SETATTR) | X(NM_EV_SETATTR_ACK) | X(NM_EV_SETATTR_NACK) | X(NM_EV_OPSTART_ACK) | /* backward compatibility, buggy BSC */ @@ -238,6 +253,7 @@ static struct osmo_fsm_state nm_chan_fsm_states[] = { }, [NM_CHAN_ST_OP_DISABLED_OFFLINE] = { .in_event_mask = + X(NM_EV_RX_SETATTR) | X(NM_EV_SETATTR_ACK) | X(NM_EV_SETATTR_NACK) | X(NM_EV_OPSTART_ACK) | diff --git a/src/common/nm_common_fsm.c b/src/common/nm_common_fsm.c index be11bef35..6a8d3d15c 100644 --- a/src/common/nm_common_fsm.c +++ b/src/common/nm_common_fsm.c @@ -25,6 +25,7 @@ const struct value_string nm_fsm_event_names[] = { { NM_EV_SW_ACT, "SW_ACT" }, + { NM_EV_SETATTR_ACK, "RX_SETATTR" }, { NM_EV_SETATTR_ACK, "SETATTR_ACK" }, { NM_EV_SETATTR_NACK, "SETATTR_NACK" }, { NM_EV_OPSTART_ACK, "OPSTART_ACK" }, diff --git a/src/common/nm_radio_carrier_fsm.c b/src/common/nm_radio_carrier_fsm.c index 88930dd7d..587847fec 100644 --- a/src/common/nm_radio_carrier_fsm.c +++ b/src/common/nm_radio_carrier_fsm.c @@ -103,13 +103,20 @@ static void st_op_disabled_offline(struct osmo_fsm_inst *fi, uint32_t event, voi struct nm_fsm_ev_setattr_data *setattr_data; bool phy_state_connected; bool rsl_link_connected; + int rc; switch (event) { + case NM_EV_RX_SETATTR: + setattr_data = (struct nm_fsm_ev_setattr_data *)data; + rc = bts_model_apply_oml(trx->bts, setattr_data->msg, setattr_data->tp, + NM_OC_RADIO_CARRIER, trx); + (void)rc; + break; case NM_EV_SETATTR_ACK: case NM_EV_SETATTR_NACK: setattr_data = (struct nm_fsm_ev_setattr_data *)data; trx->mo.setattr_success = setattr_data->cause == 0; - oml_fom_ack_nack(setattr_data->msg, setattr_data->cause); + oml_fom_ack_nack_copy_msg(setattr_data->msg, setattr_data->cause); break; case NM_EV_OPSTART_ACK: trx->mo.opstart_success = true; @@ -219,6 +226,7 @@ static struct osmo_fsm_state nm_rcarrier_fsm_states[] = { }, [NM_RCARRIER_ST_OP_DISABLED_OFFLINE] = { .in_event_mask = + X(NM_EV_RX_SETATTR) | X(NM_EV_SETATTR_ACK) | X(NM_EV_SETATTR_NACK) | X(NM_EV_OPSTART_ACK) | diff --git a/src/common/oml.c b/src/common/oml.c index 5474fb1d1..2d81054d7 100644 --- a/src/common/oml.c +++ b/src/common/oml.c @@ -48,6 +48,7 @@ #include #include #include +#include #define LOGPFOH(ss, lvl, foh, fmt, args ...) LOGP(ss, lvl, "%s: " fmt, abis_nm_dump_foh(foh), ## args) #define DEBUGPFOH(ss, foh, fmt, args ...) LOGPFOH(ss, LOGL_DEBUG, foh, fmt, ## args) @@ -472,6 +473,15 @@ int oml_fom_ack_nack(struct msgb *msg, uint8_t cause) return 1; } +/* Copy msg before calling oml_fom_ack_nack(), which takes its ownership */ +int oml_fom_ack_nack_copy_msg(const struct msgb *old_msg, uint8_t cause) +{ + struct msgb *msg = msgb_copy(old_msg, "OML-ack_nack"); + msg->trx = old_msg->trx; + oml_fom_ack_nack(msg, cause); + return 0; +} + /* * Formatted O&M messages */ @@ -549,6 +559,7 @@ static int oml_rx_set_bts_attr(struct gsm_bts *bts, struct msgb *msg) struct tlv_parsed tp, *tp_merged; int rc, i; const uint8_t *payload; + struct nm_fsm_ev_setattr_data ev_data; DEBUGPFOH(DOML, foh, "Rx SET BTS ATTR\n"); @@ -742,8 +753,15 @@ static int oml_rx_set_bts_attr(struct gsm_bts *bts, struct msgb *msg) } } - /* call into BTS driver to apply new attributes to hardware */ - return bts_model_apply_oml(bts, msg, tp_merged, NM_OC_BTS, bts); + ev_data = (struct nm_fsm_ev_setattr_data){ + .msg = msg, + .tp = tp_merged, + }; + + rc = osmo_fsm_inst_dispatch(bts->mo.fi, NM_EV_RX_SETATTR, &ev_data); + if (rc < 0) + return oml_fom_ack_nack(msg, NM_NACK_CANT_PERFORM); + return rc; } /* 8.6.2 Set Radio Attributes has been received */ @@ -752,6 +770,7 @@ static int oml_rx_set_radio_attr(struct gsm_bts_trx *trx, struct msgb *msg) struct abis_om_fom_hdr *foh = msgb_l3(msg); struct tlv_parsed tp, *tp_merged; int rc; + struct nm_fsm_ev_setattr_data ev_data; DEBUGPFOH(DOML, foh, "Rx SET RADIO CARRIER ATTR\n"); @@ -830,8 +849,17 @@ static int oml_rx_set_radio_attr(struct gsm_bts_trx *trx, struct msgb *msg) trx->arfcn = arfcn; } #endif - /* call into BTS driver to apply new attributes to hardware */ - return bts_model_apply_oml(trx->bts, msg, tp_merged, NM_OC_RADIO_CARRIER, trx); + + ev_data = (struct nm_fsm_ev_setattr_data){ + .msg = msg, + .tp = tp_merged, + }; + + rc = osmo_fsm_inst_dispatch(trx->mo.fi, NM_EV_RX_SETATTR, &ev_data); + if (rc < 0) + return oml_fom_ack_nack(msg, NM_NACK_CANT_PERFORM); + return rc; + } static int handle_chan_comb(struct gsm_bts_trx_ts *ts, const uint8_t comb) @@ -927,6 +955,7 @@ static int oml_rx_set_chan_attr(struct gsm_bts_trx_ts *ts, struct msgb *msg) struct gsm_bts *bts = ts->trx->bts; struct tlv_parsed tp, *tp_merged; int rc, i; + struct nm_fsm_ev_setattr_data ev_data; DEBUGPFOH(DOML, foh, "Rx SET CHAN ATTR\n"); @@ -1035,8 +1064,15 @@ static int oml_rx_set_chan_attr(struct gsm_bts_trx_ts *ts, struct msgb *msg) ts->hopping.hsn, ts->hopping.maio, ts->hopping.arfcn_num); LOGPC(DOML, LOGL_INFO, ")\n"); - /* call into BTS driver to apply new attributes to hardware */ - return bts_model_apply_oml(bts, msg, tp_merged, NM_OC_CHANNEL, ts); + ev_data = (struct nm_fsm_ev_setattr_data){ + .msg = msg, + .tp = tp_merged, + }; + + rc = osmo_fsm_inst_dispatch(ts->mo.fi, NM_EV_RX_SETATTR, &ev_data); + if (rc < 0) + return oml_fom_ack_nack(msg, NM_NACK_CANT_PERFORM); + return rc; } /* 8.9.2 Opstart has been received */ diff --git a/src/osmo-bts-lc15/oml.c b/src/osmo-bts-lc15/oml.c index 1842008a4..9502765cc 100644 --- a/src/osmo-bts-lc15/oml.c +++ b/src/osmo-bts-lc15/oml.c @@ -1859,8 +1859,7 @@ int bts_model_apply_oml(struct gsm_bts *bts, struct msgb *msg, rc = osmo_fsm_inst_dispatch(mo->fi, ev_data.cause == 0 ? NM_EV_SETATTR_ACK : NM_EV_SETATTR_NACK, &ev_data); - /* msgb ownership is transferred to FSM if it received ev: */ - return rc == 0 ? 1 : 0; + return rc; } /* callback from OML */ diff --git a/src/osmo-bts-oc2g/oml.c b/src/osmo-bts-oc2g/oml.c index 98c2fbc2f..40c1c85d6 100644 --- a/src/osmo-bts-oc2g/oml.c +++ b/src/osmo-bts-oc2g/oml.c @@ -1864,8 +1864,7 @@ int bts_model_apply_oml(struct gsm_bts *bts, struct msgb *msg, rc = osmo_fsm_inst_dispatch(mo->fi, ev_data.cause == 0 ? NM_EV_SETATTR_ACK : NM_EV_SETATTR_NACK, &ev_data); - /* msgb ownsership is transferred to FSM if it received ev: */ - return rc == 0 ? 1 : 0; + return rc; } /* callback from OML */ diff --git a/src/osmo-bts-octphy/l1_oml.c b/src/osmo-bts-octphy/l1_oml.c index 01e3d56af..8ffd1ac2e 100644 --- a/src/osmo-bts-octphy/l1_oml.c +++ b/src/osmo-bts-octphy/l1_oml.c @@ -1765,8 +1765,7 @@ int bts_model_apply_oml(struct gsm_bts *bts, struct msgb *msg, rc = osmo_fsm_inst_dispatch(mo->fi, ev_data.cause == 0 ? NM_EV_SETATTR_ACK : NM_EV_SETATTR_NACK, &ev_data); - /* msgb ownsership is transferred to FSM if it received ev: */ - return rc == 0 ? 1 : 0; + return rc; } diff --git a/src/osmo-bts-omldummy/bts_model.c b/src/osmo-bts-omldummy/bts_model.c index 7fb58f767..af358b6ae 100644 --- a/src/osmo-bts-omldummy/bts_model.c +++ b/src/osmo-bts-omldummy/bts_model.c @@ -119,8 +119,7 @@ int bts_model_apply_oml(struct gsm_bts *bts, struct msgb *msg, rc = osmo_fsm_inst_dispatch(mo->fi, ev_data.cause == 0 ? NM_EV_SETATTR_ACK : NM_EV_SETATTR_NACK, &ev_data); - /* msgb ownsership is transferred to FSM if it received ev: */ - return rc == 0 ? 1 : 0; + return rc; } /* MO: TS 12.21 Managed Object */ diff --git a/src/osmo-bts-sysmo/oml.c b/src/osmo-bts-sysmo/oml.c index 67e127507..12b50bfb1 100644 --- a/src/osmo-bts-sysmo/oml.c +++ b/src/osmo-bts-sysmo/oml.c @@ -1741,8 +1741,7 @@ int bts_model_apply_oml(struct gsm_bts *bts, struct msgb *msg, rc = osmo_fsm_inst_dispatch(mo->fi, ev_data.cause == 0 ? NM_EV_SETATTR_ACK : NM_EV_SETATTR_NACK, &ev_data); - /* msgb ownsership is transferred to FSM if it received ev: */ - return rc == 0 ? 1 : 0; + return rc; } /* callback from OML */ diff --git a/src/osmo-bts-trx/l1_if.c b/src/osmo-bts-trx/l1_if.c index 4989bcc70..1834cf5f4 100644 --- a/src/osmo-bts-trx/l1_if.c +++ b/src/osmo-bts-trx/l1_if.c @@ -574,8 +574,7 @@ int bts_model_apply_oml(struct gsm_bts *bts, struct msgb *msg, rc = osmo_fsm_inst_dispatch(mo->fi, ev_data.cause == 0 ? NM_EV_SETATTR_ACK : NM_EV_SETATTR_NACK, &ev_data); - /* msgb ownsership is transferred to FSM if it received ev: */ - return rc == 0 ? 1 : 0; + return rc; } /* callback from OML */ diff --git a/src/osmo-bts-virtual/bts_model.c b/src/osmo-bts-virtual/bts_model.c index 57e5304ba..477ffb3f3 100644 --- a/src/osmo-bts-virtual/bts_model.c +++ b/src/osmo-bts-virtual/bts_model.c @@ -154,8 +154,7 @@ int bts_model_apply_oml(struct gsm_bts *bts, struct msgb *msg, rc = osmo_fsm_inst_dispatch(mo->fi, ev_data.cause == 0 ? NM_EV_SETATTR_ACK : NM_EV_SETATTR_NACK, &ev_data); - /* msgb ownsership is transferred to FSM if it received ev: */ - return rc == 0 ? 1 : 0; + return rc; } /* MO: TS 12.21 Managed Object */