[MERGED] osmo-bsc[master]: fix handling of state changes in acc ramping

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

Harald Welte gerrit-no-reply at lists.osmocom.org
Sat Apr 14 11:39:08 UTC 2018


Harald Welte has submitted this change and it was merged.

Change subject: fix handling of state changes in acc ramping
......................................................................


fix handling of state changes in acc ramping

Take both the operative and administrative states into account
when deciding whether to start ACC ramping, and examine old/new
state values to avoid triggering ramping for a no-op state change.

This requires a fix to gsm_trx_lock_rf(): This function overwrote
the old administrative state of a trx before enqueuing a state
change request towards the BTS.
The BTS will confirm this request with an ACK, at which time a
signal is generated which the ACC ramp code listens to. We must
not overwrite the old state value until the signal has been handled,
otherwise the signal handler cannot tell what the old state was.

Tested with a virtphy setup, nanobts, and osmo-bts.

Change-Id: Ib3291439655598fb5ddc891a3e4cc35b0bad250f
Related: OS#2591
---
M src/libbsc/abis_nm.c
M src/libbsc/acc_ramp.c
M src/libbsc/bsc_init.c
3 files changed, 113 insertions(+), 37 deletions(-)

Approvals:
  Pau Espin Pedrol: Looks good to me, but someone else must approve
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/libbsc/abis_nm.c b/src/libbsc/abis_nm.c
index 2ee2e24..e3c4408 100644
--- a/src/libbsc/abis_nm.c
+++ b/src/libbsc/abis_nm.c
@@ -2844,13 +2844,17 @@
 {
 	uint8_t new_state = locked ? NM_STATE_LOCKED : NM_STATE_UNLOCKED;
 
-	LOGP(DNM, LOGL_NOTICE, "(bts=%d,trx=%d) Changing adm. state %s -> %s [%s]\n", trx->bts->nr, trx->nr,
+
+	if (!trx->bts || !trx->bts->oml_link) {
+		/* Set initial state which will be sent when BTS connects. */
+		trx->mo.nm_state.administrative = new_state;
+		return;
+	}
+
+	LOGP(DNM, LOGL_NOTICE, "(bts=%d,trx=%d) Requesting administrative state change %s -> %s [%s]\n",
+	     trx->bts->nr, trx->nr,
 	     get_value_string(abis_nm_adm_state_names, trx->mo.nm_state.administrative),
 	     get_value_string(abis_nm_adm_state_names, new_state), reason);
-
-	trx->mo.nm_state.administrative = new_state;
-	if (!trx->bts || !trx->bts->oml_link)
-		return;
 
 	abis_nm_chg_adm_state(trx->bts, NM_OC_RADIO_CARRIER,
 			      trx->bts->bts_nr, trx->nr, 0xff,
diff --git a/src/libbsc/acc_ramp.c b/src/libbsc/acc_ramp.c
index 7116107..ff8ff0e 100644
--- a/src/libbsc/acc_ramp.c
+++ b/src/libbsc/acc_ramp.c
@@ -28,6 +28,7 @@
 #include <osmocom/bsc/gsm_data.h>
 #include <osmocom/bsc/chan_alloc.h>
 #include <osmocom/bsc/signal.h>
+#include <osmocom/bsc/abis_nm.h>
 
 /*
  * Check if an ACC has been permanently barred for a BTS,
@@ -144,6 +145,7 @@
 	struct nm_statechg_signal_data *nsd = signal_data;
 	struct acc_ramp *acc_ramp = handler_data;
 	struct gsm_bts_trx *trx = NULL;
+	bool trigger_ramping = false, abort_ramping = false;
 
 	/* Handled signals map to an Administrative State Change ACK, or a State Changed Event Report. */
 	if (signal != S_NM_STATECHG_ADM && signal != S_NM_STATECHG_OPER)
@@ -154,37 +156,109 @@
 
 	trx = nsd->obj;
 
+	LOGP(DRSL, LOGL_DEBUG, "(bts=%d,trx=%d) ACC RAMP: administrative state %s -> %s\n",
+	    acc_ramp->bts->nr, trx->nr,
+	    get_value_string(abis_nm_adm_state_names, nsd->old_state->administrative),
+	    get_value_string(abis_nm_adm_state_names, nsd->new_state->administrative));
+	LOGP(DRSL, LOGL_DEBUG, "(bts=%d,trx=%d) ACC RAMP: operational state %s -> %s\n",
+	    acc_ramp->bts->nr, trx->nr,
+	    abis_nm_opstate_name(nsd->old_state->operational),
+	    abis_nm_opstate_name(nsd->new_state->operational));
+
 	/* We only care about state changes of the first TRX. */
 	if (trx->nr != 0)
 		return 0;
 
 	/* RSL must already be up. We cannot send RACH system information to the BTS otherwise. */
-	if (trx->rsl_link == NULL)
+	if (trx->rsl_link == NULL) {
+		LOGP(DRSL, LOGL_DEBUG, "(bts=%d,trx=%d) ACC RAMP: ignoring state change because RSL link is down\n",
+		     acc_ramp->bts->nr, trx->nr);
 		return 0;
-
-	/* Trigger or abort ACC ramping based on the new 'RF lock' state of this TRX. */
-	switch (nsd->new_state->administrative) {
-	case NM_STATE_UNLOCKED:
-		/*
-		 * Do not re-trigger ACC ramping if ramping is already in progress.
-		 * A BTS might send several "unlock" change events: One in the Administrative
-		 * State Change ACK, and/or another in a State Changed Event Report.
-		 * For instance, the nanobts is known to send both.
-		 */
-		if (!osmo_timer_pending(&acc_ramp->step_timer))
-			acc_ramp_trigger(acc_ramp);
-		break;
-	case NM_STATE_LOCKED:
-	case NM_STATE_SHUTDOWN:
-		acc_ramp_abort(acc_ramp);
-		break;
-	case NM_STATE_NULL:
-		break;
-	default:
-		LOGP(DRSL, LOGL_NOTICE, "(bts=%d) ACC RAMP: unrecognized administrative state '0x%x' reported for TRX 0\n",
-		    acc_ramp->bts->nr, nsd->new_state->administrative);
-		break;
 	}
+
+	/* Trigger or abort ACC ramping based on the new state of this TRX. */
+	if (nsd->old_state->administrative != nsd->new_state->administrative) {
+		switch (nsd->new_state->administrative) {
+		case NM_STATE_UNLOCKED:
+			if (nsd->old_state->operational != nsd->new_state->operational) {
+				/*
+				 * Administrative and operational state have both changed.
+				 * Trigger ramping only if TRX 0 will be both enabled and unlocked.
+				 */
+				if (nsd->new_state->operational == NM_OPSTATE_ENABLED)
+					trigger_ramping = true;
+				else
+					LOGP(DRSL, LOGL_DEBUG, "(bts=%d,trx=%d) ACC RAMP: ignoring state change "
+					     "because TRX is transitioning into operational state '%s'\n",
+					     acc_ramp->bts->nr, trx->nr,
+					     abis_nm_opstate_name(nsd->new_state->operational));
+			} else {
+				/*
+				 * Operational state has not changed.
+				 * Trigger ramping only if TRX 0 is already usable.
+				 */
+				if (trx_is_usable(trx))
+					trigger_ramping = true;
+				else
+					LOGP(DRSL, LOGL_DEBUG, "(bts=%d,trx=%d) ACC RAMP: ignoring state change "
+					     "because TRX is not usable\n", acc_ramp->bts->nr, trx->nr);
+			}
+			break;
+		case NM_STATE_LOCKED:
+		case NM_STATE_SHUTDOWN:
+			abort_ramping = true;
+			break;
+		case NM_STATE_NULL:
+		default:
+			LOGP(DRSL, LOGL_NOTICE, "(bts=%d) ACC RAMP: unrecognized administrative state '0x%x' "
+			    "reported for TRX 0\n", acc_ramp->bts->nr, nsd->new_state->administrative);
+			break;
+		}
+	}
+	if (nsd->old_state->operational != nsd->new_state->operational) {
+		switch (nsd->new_state->operational) {
+		case NM_OPSTATE_ENABLED:
+			if (nsd->old_state->administrative != nsd->new_state->administrative) {
+				/*
+				 * Administrative and operational state have both changed.
+				 * Trigger ramping only if TRX 0 will be both enabled and unlocked.
+				 */
+				if (nsd->new_state->administrative == NM_STATE_UNLOCKED)
+					trigger_ramping = true;
+				else
+					LOGP(DRSL, LOGL_DEBUG, "(bts=%d,trx=%d) ACC RAMP: ignoring state change "
+					     "because TRX is transitioning into administrative state '%s'\n",
+					     acc_ramp->bts->nr, trx->nr,
+					     get_value_string(abis_nm_adm_state_names, nsd->new_state->administrative));
+			} else {
+				/*
+				 * Administrative state has not changed.
+				 * Trigger ramping only if TRX 0 is already unlocked.
+				 */
+				if (trx->mo.nm_state.administrative == NM_STATE_UNLOCKED)
+					trigger_ramping = true;
+				else
+					LOGP(DRSL, LOGL_DEBUG, "(bts=%d,trx=%d) ACC RAMP: ignoring state change "
+					     "because TRX is in administrative state '%s'\n",
+					     acc_ramp->bts->nr, trx->nr,
+					     get_value_string(abis_nm_adm_state_names, trx->mo.nm_state.administrative));
+			}
+			break;
+		case NM_OPSTATE_DISABLED:
+			abort_ramping = true;
+			break;
+		case NM_OPSTATE_NULL:
+		default:
+			LOGP(DRSL, LOGL_NOTICE, "(bts=%d) ACC RAMP: unrecognized operational state '0x%x' "
+			     "reported for TRX 0\n", acc_ramp->bts->nr, nsd->new_state->administrative);
+			break;
+		}
+	}
+
+	if (trigger_ramping)
+		acc_ramp_trigger(acc_ramp);
+	else if (abort_ramping)
+		acc_ramp_abort(acc_ramp);
 
 	return 0;
 }
@@ -270,13 +344,9 @@
 	acc_ramp_abort(acc_ramp);
 
 	if (acc_ramp_is_enabled(acc_ramp)) {
-		struct gsm_bts_trx *trx0 = gsm_bts_trx_by_nr(acc_ramp->bts, 0);
-		/* TRX 0 should be usable and unlocked, otherwise starting ACC ramping is pointless. */
-		if (trx0 && trx_is_usable(trx0) && trx0->mo.nm_state.administrative == NM_STATE_UNLOCKED) {
-			/* Set all available ACCs to barred and start ramping up. */
-			barr_all_accs(acc_ramp);
-			do_acc_ramping_step(acc_ramp);
-		}
+		/* Set all available ACCs to barred and start ramping up. */
+		barr_all_accs(acc_ramp);
+		do_acc_ramping_step(acc_ramp);
 	}
 }
 
diff --git a/src/libbsc/bsc_init.c b/src/libbsc/bsc_init.c
index c357105..429d3c7 100644
--- a/src/libbsc/bsc_init.c
+++ b/src/libbsc/bsc_init.c
@@ -339,8 +339,10 @@
 	/*
 	 * Trigger ACC ramping before sending system information to BTS.
 	 * This ensures that RACH control in system information is configured correctly.
+	 * TRX 0 should be usable and unlocked, otherwise starting ACC ramping is pointless.
 	 */
-	acc_ramp_trigger(&trx->bts->acc_ramp);
+	if (trx_is_usable(trx) && trx->mo.nm_state.administrative == NM_STATE_UNLOCKED)
+		acc_ramp_trigger(&trx->bts->acc_ramp);
 
 	gsm_bts_trx_set_system_infos(trx);
 

-- 
To view, visit https://gerrit.osmocom.org/7784
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib3291439655598fb5ddc891a3e4cc35b0bad250f
Gerrit-PatchSet: 4
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Owner: Stefan Sperling <ssperling at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Stefan Sperling <ssperling at sysmocom.de>



More information about the gerrit-log mailing list