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

[Harald Welte]

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 
 #include 
 #include 
+#include 
 
 /*
  * 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(_ramp->step_timer))
-   acc_ramp_trigger(acc_ramp);
-   break;
-   case NM_STATE_LOCKED:
-   case NM_STATE_SHUTDOWN:
-   acc_ramp_abort(acc_ramp);
-   break;
- 

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

[Harald Welte]

Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3291439655598fb5ddc891a3e4cc35b0bad250f
Gerrit-PatchSet: 4
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Owner: Stefan Sperling 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-Reviewer: Stefan Sperling 
Gerrit-HasComments: No

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

[Pau Espin Pedrol]

Patch Set 4:

Remember to backport all related patches to openbsc!

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3291439655598fb5ddc891a3e4cc35b0bad250f
Gerrit-PatchSet: 4
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Owner: Stefan Sperling 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-Reviewer: Stefan Sperling 
Gerrit-HasComments: No

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

[Pau Espin Pedrol]

Patch Set 4: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3291439655598fb5ddc891a3e4cc35b0bad250f
Gerrit-PatchSet: 4
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Owner: Stefan Sperling 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-Reviewer: Stefan Sperling 
Gerrit-HasComments: No

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

[Stefan Sperling]

Hello Pau Espin Pedrol, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/7784

to look at the new patch set (#4).

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(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/84/7784/4

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 
 #include 
 #include 
+#include 
 
 /*
  * 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(_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, 

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

[Stefan Sperling]

Patch Set 3:

(3 comments)

https://gerrit.osmocom.org/#/c/7784/3/src/libbsc/acc_ramp.c
File src/libbsc/acc_ramp.c:

Line 182:   if (trx_is_usable(trx)) /* cross-check with 
operational state */
> As discussed a few mins ago, when coming from S_NM_STATECHG_OPER signal in 
Indeed. Fixed in next patch set.


Line 190:   break;
> we can probably removed this break to print an error too, unless NULL is re
Sure.


Line 222:   if (trigger_ramping && 
!osmo_timer_pending(_ramp->step_timer))
> do we really need this osmo_timer_pending check here? This should never hap
I'm afraid of putting an assert in there.

It should not be needed in the next patch set so I've just removed the check 
entirely.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3291439655598fb5ddc891a3e4cc35b0bad250f
Gerrit-PatchSet: 3
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Owner: Stefan Sperling 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-Reviewer: Stefan Sperling 
Gerrit-HasComments: Yes

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

[Pau Espin Pedrol]

Patch Set 3: Code-Review-1

(5 comments)

https://gerrit.osmocom.org/#/c/7784/3/src/libbsc/acc_ramp.c
File src/libbsc/acc_ramp.c:

Line 182:   if (trx_is_usable(trx)) /* cross-check with 
operational state */
As discussed a few mins ago, when coming from S_NM_STATECHG_OPER signal in 
abis_nm_rx_statechg_rep, it can be that both operational and administrative 
status change at the same time (same call to this function).

Case: admin LOCKED->UNLOCKED + oper DISABLED->ENABLED, we are not enabling 
ramping here because trx_is_usable() probably checks the current/old status 
which is still DISABLED.


Line 190:   break;
we can probably removed this break to print an error too, unless NULL is really 
a valid state from the spec point of view (one to which we can change to).


Line 201:   if (trx->mo.nm_state.administrative == 
NM_STATE_UNLOCKED)
Again, we need to check after the new one as it could have be different than 
the old/current one.


Line 208:   break;
same, remove break?


Line 222:   if (trigger_ramping && 
!osmo_timer_pending(_ramp->step_timer))
do we really need this osmo_timer_pending check here? This should never happen 
right? better move it inside the if() and add an OSMO_ASSERT() with it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3291439655598fb5ddc891a3e4cc35b0bad250f
Gerrit-PatchSet: 3
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Owner: Stefan Sperling 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pau Espin Pedrol 
Gerrit-HasComments: Yes

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

[Stefan Sperling]

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.
Still needs to be tested with 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, 72 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/84/7784/3

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..da61094 100644
--- a/src/libbsc/acc_ramp.c
+++ b/src/libbsc/acc_ramp.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /*
  * 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,6 +156,17 @@
 
trx = nsd->obj;
 
+   if (signal == S_NM_STATECHG_ADM)
+   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));
+   else if (signal == S_NM_STATECHG_OPER)
+   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;
@@ -162,29 +175,54 @@
if (trx->rsl_link == NULL)
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(_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 

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

[Stefan Sperling]

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.
Still needs to be tested with 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, 72 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/84/7784/2

diff --git a/src/libbsc/abis_nm.c b/src/libbsc/abis_nm.c
index 2ee2e24..66eba12 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..da61094 100644
--- a/src/libbsc/acc_ramp.c
+++ b/src/libbsc/acc_ramp.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /*
  * 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,6 +156,17 @@
 
trx = nsd->obj;
 
+   if (signal == S_NM_STATECHG_ADM)
+   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));
+   else if (signal == S_NM_STATECHG_OPER)
+   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;
@@ -162,29 +175,54 @@
if (trx->rsl_link == NULL)
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(_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 

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

[Stefan Sperling]

Review at  https://gerrit.osmocom.org/7784

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.

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, 72 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/84/7784/1

diff --git a/src/libbsc/abis_nm.c b/src/libbsc/abis_nm.c
index 2ee2e24..66eba12 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..da61094 100644
--- a/src/libbsc/acc_ramp.c
+++ b/src/libbsc/acc_ramp.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /*
  * 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,6 +156,17 @@
 
trx = nsd->obj;
 
+   if (signal == S_NM_STATECHG_ADM)
+   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));
+   else if (signal == S_NM_STATECHG_OPER)
+   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;
@@ -162,29 +175,54 @@
if (trx->rsl_link == NULL)
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(_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 !=