diff mbox

[6/6] gprs-ns: Fix reset state handling

Message ID 1412762718-31961-6-git-send-email-jerlbeck@sysmocom.de
State Accepted
Headers show

Commit Message

Jacob Erlbeck Oct. 8, 2014, 10:05 a.m. UTC 
Currently the NS-VC's state is updated from within gprs_ns_tx_reset,
which can lead to an inconsistent state when the RESET_ACK is lost.
In this state, the NSE_S_RESET bit is set but the Tns-reset timer is
not started.

This patch moves the state update into gprs_nsvc_reset. This way, the
state flags are consistent with the timer.

Addresses:
  SGSN -> BSS       NS_ALIVE
  BSS -> SGSN       NS_ALIVE_ACK
  BSS -> SGSN       BVC_RESET
  SGSN -> BSS       NS_STATUS, Cause: NS-VC blocked, NS VCI: 0x65
  and there is no BSS->SGSN NS_ALIVE

Ticket: OW#1213
Sponsored-by: On-Waves ehf
---
 src/gb/gprs_ns.c         |  9 ++++-----
 tests/gb/gprs_ns_test.c  |  2 --
 tests/gb/gprs_ns_test.ok | 27 +++++++++++++++++++++++----
 3 files changed, 27 insertions(+), 11 deletions(-)

Comments

Holger Freyther Oct. 9, 2014, 5:52 a.m. UTC | #1 
On Wed, Oct 08, 2014 at 12:05:18PM +0200, Jacob Erlbeck wrote:
> Currently the NS-VC's state is updated from within gprs_ns_tx_reset,
> which can lead to an inconsistent state when the RESET_ACK is lost.
> In this state, the NSE_S_RESET bit is set but the Tns-reset timer is
> not started.
>  	sent_pdu_type = -1;
>  	send_ns_alive(nsi, &sgsn_peer);
> -	/* Disabled, since it is not yet fixed
>  	OSMO_ASSERT(sent_pdu_type == -1);
>  	send_ns_reset_ack(nsi, &sgsn_peer, SGSN_NSEI+1, SGSN_NSEI);
>  	OSMO_ASSERT(sent_pdu_type == NS_PDUT_ALIVE);
>  	send_ns_alive_ack(nsi, &sgsn_peer);
>  	OSMO_ASSERT(sent_pdu_type == NS_PDUT_UNBLOCK);
>  	send_ns_unblock_ack(nsi, &sgsn_peer);
> -	*/
>  
>  	send_ns_unitdata(nsi, &sgsn_peer, 0x1234, dummy_sdu, sizeof(dummy_sdu));

I think it would be nice if you could directly ASSERT the unblocked
alive state of the NSI? It should be fairly to do?

All patches look great and it is nice incremental work. I am going
to merge them now. Please provide a follow-up patch that addresses
the points you think are valid.

holger
diff mbox

Patch

diff --git a/src/gb/gprs_ns.c b/src/gb/gprs_ns.c
index cf7adaf..65d0494 100644
--- a/src/gb/gprs_ns.c
+++ b/src/gb/gprs_ns.c
@@ -348,8 +348,6 @@  int gprs_ns_tx_reset(struct gprs_nsvc *nsvc, uint8_t cause)
 	LOGP(DNS, LOGL_INFO, "NSEI=%u Tx NS RESET (NSVCI=%u, cause=%s)\n",
 		nsvc->nsei, nsvc->nsvci, gprs_ns_cause_str(cause));
 
-	nsvc->state |= NSE_S_RESET;
-
 	msg->l2h = msgb_put(msg, sizeof(*nsh));
 	nsh = (struct gprs_ns_hdr *) msg->l2h;
 	nsh->pdu_type = NS_PDUT_RESET;
@@ -1250,8 +1248,8 @@  int gprs_ns_process_msg(struct gprs_ns_inst *nsi, struct msgb *msg,
 		 * and should send a NS-RESET to make sure everything recovers
 		 * fine. */
 		if ((*nsvc)->state == NSE_S_BLOCKED)
-			rc = gprs_ns_tx_reset((*nsvc), NS_CAUSE_PDU_INCOMP_PSTATE);
-		else
+			rc = gprs_nsvc_reset((*nsvc), NS_CAUSE_PDU_INCOMP_PSTATE);
+		else if (!((*nsvc)->state & NSE_S_RESET))
 			rc = gprs_ns_tx_alive_ack(*nsvc);
 		break;
 	case NS_PDUT_ALIVE_ACK:
@@ -1503,7 +1501,8 @@  int gprs_nsvc_reset(struct gprs_nsvc *nsvc, uint8_t cause)
 		nsvc->nsei);
 
 	/* Mark NS-VC locally as blocked and dead */
-	nsvc->state = NSE_S_BLOCKED;
+	nsvc->state = NSE_S_BLOCKED | NSE_S_RESET;
+
 	/* Send NS-RESET PDU */
 	rc = gprs_ns_tx_reset(nsvc, cause);
 	if (rc < 0) {
diff --git a/tests/gb/gprs_ns_test.c b/tests/gb/gprs_ns_test.c
index ad8e6d5..2185ff8 100644
--- a/tests/gb/gprs_ns_test.c
+++ b/tests/gb/gprs_ns_test.c
@@ -839,14 +839,12 @@  static void test_sgsn_reset_invalid_state()
 
 	sent_pdu_type = -1;
 	send_ns_alive(nsi, &sgsn_peer);
-	/* Disabled, since it is not yet fixed
 	OSMO_ASSERT(sent_pdu_type == -1);
 	send_ns_reset_ack(nsi, &sgsn_peer, SGSN_NSEI+1, SGSN_NSEI);
 	OSMO_ASSERT(sent_pdu_type == NS_PDUT_ALIVE);
 	send_ns_alive_ack(nsi, &sgsn_peer);
 	OSMO_ASSERT(sent_pdu_type == NS_PDUT_UNBLOCK);
 	send_ns_unblock_ack(nsi, &sgsn_peer);
-	*/
 
 	send_ns_unitdata(nsi, &sgsn_peer, 0x1234, dummy_sdu, sizeof(dummy_sdu));
 
diff --git a/tests/gb/gprs_ns_test.ok b/tests/gb/gprs_ns_test.ok
index 66b1dbb..1e0bf7f 100644
--- a/tests/gb/gprs_ns_test.ok
+++ b/tests/gb/gprs_ns_test.ok
@@ -806,18 +806,37 @@  result (ALIVE) = 12
 PROCESSING ALIVE from 0x05060708:32000
 0a 
 
+result (ALIVE) = 0
+
+PROCESSING RESET_ACK from 0x05060708:32000
+03 01 82 01 01 04 82 01 00 
+
 MESSAGE to SGSN, msg length 1
+0a 
+
+result (RESET_ACK) = 1
+
+PROCESSING ALIVE_ACK from 0x05060708:32000
 0b 
 
-result (ALIVE) = 1
+MESSAGE to SGSN, msg length 1
+06 
+
+result (ALIVE_ACK) = 1
+
+PROCESSING UNBLOCK_ACK from 0x05060708:32000
+07 
+
+==> got signal NS_UNBLOCK, NS-VC 0x0101/5.6.7.8:32000
+result (UNBLOCK_ACK) = 0
 
 PROCESSING UNITDATA from 0x05060708:32000
 00 00 12 34 01 02 03 04 
 
-MESSAGE to SGSN, msg length 8
-08 00 81 03 01 82 01 01 
+CALLBACK, event 0, msg length 4, bvci 0x1234
+01 02 03 04 
 
-result (UNITDATA) = 8
+result (UNITDATA) = 0
 
 Current NS-VCIs: