Attention is currently required from: lynxis lazus. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/29788 )

Change subject: Add BTS ramping to prevent mass configuration of BTS at the same time ......................................................................

Patch Set 1: Code-Review-1

(16 comments)

File include/osmocom/bsc/nm_bts_ramp.h:

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/83cf1130_b414ce9b PS1, Line 36: bool bts_ramp_active(struct gsm_network *net); bts_ramp ("BTS ramp" concept I already saw written in the commit decription) is too generic. What about bts_bringup_ramp or bts_setup_ramp?

I'd also drop the "nm" part in the file name, and use same bts_bringup_ramp or similar as in the APIs.

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/1add034f_d401edfb PS1, Line 40: /*! All this documentation we always put it in the .c file.

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/13b189a2_b159ce0d PS1, Line 43: * \param max_bts allow how many bts to provision how many concurrent right? so max_concurrent_bts.

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/fc7b6b68_876c67d6 PS1, Line 65: * \brief bts_ramp_unblock_bts IMprove documentation on what unblock means here.

File src/osmo-bsc/bsc_vty.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/56175227_38cc6403 PS1, Line 1284: "allow-bts-configuration <0-65535>", bts-unblock-bringup-ramping? The "allow-bts-configuration" thing is not really descriptivie, it's too generic.

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/8bef54db_7d66d863 PS1, Line 2659: vty_out(vty, " bts ramp limit %d within %d seconds%s", description all way to generic. One really needs to look at the code to understand what the limit means (units).

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/ec8d391f_3eab7192 PS1, Line 3087: "bts ramp limit <0-999> within <0-999> seconds", We already have "access-control-class-ramping" set of commands in osmo-bsc and "power-ramp" set of commands in osmo-bts. Both use same terminology and format, so let's better use the same here.

That'd probably be: no bts-bringup-ramping bts-bringup-ramping bts-bringup-ramping-step-interval <0-999> bts-bringup-ramping-step-size <0-999>

BTW I see no reason to limit the time interval to 0-999.

File src/osmo-bsc/nm_bb_transc_fsm.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/3cf29b4d_bb22ee50 PS1, Line 345: case NM_EV_RAMP_GO: Since this only makes sense when in DISABLED_DEPENDENCY or DISABLED_OFFLINE, better move it (duplicated with different boolean passed) to each of those 2 states. This makes all the FSMs far easier to follow.

Do it for all the FSM files you modify.

File src/osmo-bsc/nm_bts_ramp.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/1992775a_d7a98f97 PS1, Line 42: /* inform all MOs */ It may make sense to have this moved into a function in nm_common_fsm.c (with generic event and data pointers).

nm_fsm_dispatch_all(int action, void *data);

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/fa2de165_7c1c2f12 PS1, Line 63: if (llist_entry_empty(&bts->bts_ramp.list)) did you think about initing the list during constructor time? then you wouldn't need this kind of stuff afaiu.

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/d6e9134e_e67bc558 PS1, Line 86: net->bts_ramp.count >= net->bts_ramp.max_bts) wrong indentation

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/6545f276_ce0244f3 PS1, Line 113: void bts_ramp_deactivate(struct gsm_network *net) bts_ramp_disable.

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/d33aad6c_e04504ce PS1, Line 121: bool bts_ramp_active(struct gsm_network *net) "active" would be count < max_bts. What you mean here is probably "bts_ramp_enabled". Active sounds more like there's BTS in the list still waiting.

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/384a5d40_1f95815a PS1, Line 143: OSMO_ASSERT(llist_entry_empty(&bts->bts_ramp.list)); if you remove the BTS previously using bts_ramp_remove() then this will most probably assert.

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/62cfd077_87e2e672 PS1, Line 172: if (!llist_entry_empty(&bts->bts_ramp.list)) does this API check if the list is all zeroed?

File src/osmo-bsc/nm_bts_sm_fsm.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/29788/comment/846ba00b_ff6eb759 PS1, Line 162: case NM_EV_RAMP_GO: You should call configure_loop() here instead of copying the code again.