Change in osmo-bsc[master]: fix: send RR Release (e.g. after BSSMAP Clear Cmd)

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Wed Nov 14 16:16:30 UTC 2018


Neels Hofmeyr has submitted this change and it was merged. ( https://gerrit.osmocom.org/11664 )

Change subject: fix: send RR Release (e.g. after BSSMAP Clear Cmd)
......................................................................

fix: send RR Release (e.g. after BSSMAP Clear Cmd)

After commit [1], the code makes sure to disassociate lchan and conn before
invoking the lchan release. However, we only send RR Release if a conn is
present, which clearly is nonsense after [1].

[1] commit 8b818a01b00ea3daad4ad58c162ac52b4f08a5cb
    "subscr conn: properly forget lchan before release"
    Change-Id: I4fd582b41ba4599af704d670af83651d2450b1db

Manage sending of RR Release via a flag, set during invoking lchan release.

Add do_rr_release arg to lchan_release(), gscon_release_lchans(). In
lchan_fsm.c, send RR Release only if do_rr_release was passed true; do not care
whether a conn is still associated (because it won't ever be since [1]).

That way we can intelligently decide what release process makes sense (whether
the lchan terminates the subscriber connection or whether the connection goes
on at another lchan), and still disassociate lchan and conn early.

BTW, this problem wasn't caught by the stock OsmoBSC TTCN3 tests, because the
f_expect_chan_rel() don't care whether an RR Release happens or not. This is
being fixed by Ibc64058f1e214bea585f4e8dcb66f3df8ead3845.

So far this patch should fix BSC_Tests_LCLS.TC_lcls_connect_clear.

Related: OS#3413
Change-Id: I666b3b4f45706d898d664d380bd0fd2b018be358
---
M include/osmocom/bsc/bsc_subscr_conn_fsm.h
M include/osmocom/bsc/gsm_data.h
M include/osmocom/bsc/lchan_fsm.h
M src/osmo-bsc/abis_rsl.c
M src/osmo-bsc/assignment_fsm.c
M src/osmo-bsc/bsc_subscr_conn_fsm.c
M src/osmo-bsc/bsc_vty.c
M src/osmo-bsc/gsm_04_08_rr.c
M src/osmo-bsc/gsm_08_08.c
M src/osmo-bsc/handover_fsm.c
M src/osmo-bsc/lchan_fsm.c
M tests/gsm0408/gsm0408_test.c
12 files changed, 32 insertions(+), 30 deletions(-)

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



diff --git a/include/osmocom/bsc/bsc_subscr_conn_fsm.h b/include/osmocom/bsc/bsc_subscr_conn_fsm.h
index d4de1c9..fcdba50 100644
--- a/include/osmocom/bsc/bsc_subscr_conn_fsm.h
+++ b/include/osmocom/bsc/bsc_subscr_conn_fsm.h
@@ -71,7 +71,7 @@
 			    struct assignment_request *req);
 
 void gscon_change_primary_lchan(struct gsm_subscriber_connection *conn, struct gsm_lchan *new_lchan);
-void gscon_release_lchans(struct gsm_subscriber_connection *conn, bool do_sacch_deact);
+void gscon_release_lchans(struct gsm_subscriber_connection *conn, bool do_sacch_deact, bool do_rr_release);
 
 void gscon_lchan_releasing(struct gsm_subscriber_connection *conn, struct gsm_lchan *lchan);
 void gscon_forget_lchan(struct gsm_subscriber_connection *conn, struct gsm_lchan *lchan);
diff --git a/include/osmocom/bsc/gsm_data.h b/include/osmocom/bsc/gsm_data.h
index 1be9ae8..5805a5f 100644
--- a/include/osmocom/bsc/gsm_data.h
+++ b/include/osmocom/bsc/gsm_data.h
@@ -514,6 +514,7 @@
 	 * flag, so that the lchan will gracefully release at the next sensible junction. */
 	bool release_requested;
 	bool deact_sacch;
+	bool do_rr_release;
 
 	char *last_error;
 
diff --git a/include/osmocom/bsc/lchan_fsm.h b/include/osmocom/bsc/lchan_fsm.h
index d2e8724..d3315a6 100644
--- a/include/osmocom/bsc/lchan_fsm.h
+++ b/include/osmocom/bsc/lchan_fsm.h
@@ -49,7 +49,7 @@
 void lchan_fsm_init();
 
 void lchan_fsm_alloc(struct gsm_lchan *lchan);
-void lchan_release(struct gsm_lchan *lchan, bool sacch_deact,
+void lchan_release(struct gsm_lchan *lchan, bool do_deact_sacch, bool do_rr_release,
 		   bool err, enum gsm48_rr_cause cause_rr);
 
 struct lchan_activate_info {
diff --git a/src/osmo-bsc/abis_rsl.c b/src/osmo-bsc/abis_rsl.c
index bd104ed..ea537fe 100644
--- a/src/osmo-bsc/abis_rsl.c
+++ b/src/osmo-bsc/abis_rsl.c
@@ -901,7 +901,7 @@
 	 * the connection will presumably be torn down and lead to an lchan release. During initial
 	 * Channel Request from the MS, an lchan has no conn yet, so in that case release now. */
 	if (!lchan->conn) {
-		lchan_release(lchan, false, true, *cause_p);
+		lchan_release(lchan, false, false, true, *cause_p);
 	} else
 		osmo_fsm_inst_dispatch(lchan->conn->fi, GSCON_EV_RSL_CONN_FAIL, (void*)cause_p);
 
diff --git a/src/osmo-bsc/assignment_fsm.c b/src/osmo-bsc/assignment_fsm.c
index 3f553ff..653681e 100644
--- a/src/osmo-bsc/assignment_fsm.c
+++ b/src/osmo-bsc/assignment_fsm.c
@@ -98,7 +98,7 @@
 	if (conn->assignment.new_lchan) {
 		struct gsm_lchan *lchan = conn->assignment.new_lchan;
 		conn->assignment.new_lchan = NULL;
-		lchan_release(lchan, false, true, RSL_ERR_EQUIPMENT_FAIL);
+		lchan_release(lchan, false, false, true, RSL_ERR_EQUIPMENT_FAIL);
 	}
 
 	if (conn->assignment.created_ci_for_msc) {
@@ -213,7 +213,7 @@
 	if (!conn->assignment.fi) {
 		/* The lchan was ready, and we failed to tell the MSC about it. By releasing this lchan,
 		 * the conn will notice that its primary lchan is gone and should clean itself up. */
-		lchan_release(conn->lchan, false, true, RSL_ERR_EQUIPMENT_FAIL);
+		lchan_release(conn->lchan, false, true, true, RSL_ERR_EQUIPMENT_FAIL);
 		return;
 	}
 
diff --git a/src/osmo-bsc/bsc_subscr_conn_fsm.c b/src/osmo-bsc/bsc_subscr_conn_fsm.c
index 4d6521f..5856d7a 100644
--- a/src/osmo-bsc/bsc_subscr_conn_fsm.c
+++ b/src/osmo-bsc/bsc_subscr_conn_fsm.c
@@ -152,7 +152,7 @@
 
 /* Release an lchan in such a way that it doesn't fire events back to the conn. */
 static void gscon_release_lchan(struct gsm_subscriber_connection *conn, struct gsm_lchan *lchan,
-				bool do_sacch_deact, bool err, enum gsm48_rr_cause cause_rr)
+				bool do_sacch_deact, bool do_rr_release, bool err, enum gsm48_rr_cause cause_rr)
 {
 	if (!lchan || !conn)
 		return;
@@ -164,17 +164,17 @@
 		conn->ho.new_lchan = NULL;
 	if (conn->assignment.new_lchan == lchan)
 		conn->assignment.new_lchan = NULL;
-	lchan_release(lchan, do_sacch_deact, err, cause_rr);
+	lchan_release(lchan, do_sacch_deact, do_rr_release, err, cause_rr);
 }
 
-void gscon_release_lchans(struct gsm_subscriber_connection *conn, bool do_sacch_deact)
+void gscon_release_lchans(struct gsm_subscriber_connection *conn, bool do_sacch_deact, bool do_rr_release)
 {
 	if (conn->ho.fi)
 		handover_end(conn, HO_RESULT_CONN_RELEASE);
 
 	assignment_reset(conn);
 
-	gscon_release_lchan(conn, conn->lchan, do_sacch_deact, false, 0);
+	gscon_release_lchan(conn, conn->lchan, do_sacch_deact, do_rr_release, false, 0);
 }
 
 static void handle_bssap_n_connect(struct osmo_fsm_inst *fi, struct osmo_scu_prim *scu_prim)
@@ -620,7 +620,7 @@
 		osmo_fsm_inst_dispatch(conn->lchan->fi_rtp, LCHAN_RTP_EV_ESTABLISHED, 0);
 
 	if (old_lchan && (old_lchan != new_lchan))
-		gscon_release_lchan(conn, old_lchan, false, false, 0);
+		gscon_release_lchan(conn, old_lchan, false, false, false, 0);
 }
 
 void gscon_lchan_releasing(struct gsm_subscriber_connection *conn, struct gsm_lchan *lchan)
@@ -716,7 +716,7 @@
 		if (conn->fi->state != ST_CLEARING)
 			osmo_fsm_inst_state_chg(fi, ST_CLEARING, 60, 999);
 		LOGPFSML(fi, LOGL_DEBUG, "Releasing all lchans (if any) after BSSMAP Clear Command\n");
-		gscon_release_lchans(conn, true);
+		gscon_release_lchans(conn, true, true);
 		/* FIXME: Release all terestrial resources in ST_CLEARING */
 		/* According to 3GPP 48.008 3.1.9.1. "The BSS need not wait for the radio channel
 		 * release to be completed or for the guard timer to expire before returning the
@@ -792,7 +792,7 @@
 	}
 
 	LOGPFSML(fi, LOGL_DEBUG, "Releasing all lchans (if any) because this conn is terminating\n");
-	gscon_release_lchans(conn, false);
+	gscon_release_lchans(conn, false, true);
 
 	/* drop pending messages */
 	gscon_dtap_queue_flush(conn, 0);
@@ -806,7 +806,7 @@
 
 	switch (fi->T) {
 	case 993210:
-		gscon_release_lchan(conn, conn->lchan, false, true, RSL_ERR_INTERWORKING);
+		gscon_release_lchan(conn, conn->lchan, false, true, true, RSL_ERR_INTERWORKING);
 
 		/* MSC has not responded/confirmed connection with CC, this
 		 * could indicate a bad SCCP connection. We now inform the the
diff --git a/src/osmo-bsc/bsc_vty.c b/src/osmo-bsc/bsc_vty.c
index 84752dd..9f42d8a 100644
--- a/src/osmo-bsc/bsc_vty.c
+++ b/src/osmo-bsc/bsc_vty.c
@@ -4588,7 +4588,7 @@
 		}
 		vty_out(vty, "%% Asking for release of %s in state %s%s", gsm_lchan_name(lchan),
 			osmo_fsm_inst_state_name(lchan->fi), VTY_NEWLINE);
-		lchan_release(lchan, false, false, 0);
+		lchan_release(lchan, false, !!(lchan->conn), false, 0);
 	}
 
 	return CMD_SUCCESS;
diff --git a/src/osmo-bsc/gsm_04_08_rr.c b/src/osmo-bsc/gsm_04_08_rr.c
index 4659c1a..c3dd307 100644
--- a/src/osmo-bsc/gsm_04_08_rr.c
+++ b/src/osmo-bsc/gsm_04_08_rr.c
@@ -942,7 +942,7 @@
 		/* allocate a new connection */
 		lchan->conn = bsc_subscr_con_allocate(msg->lchan->ts->trx->bts->network);
 		if (!lchan->conn) {
-			lchan_release(lchan, false, true, RSL_ERR_EQUIPMENT_FAIL);
+			lchan_release(lchan, false, false, true, RSL_ERR_EQUIPMENT_FAIL);
 			return -1;
 		}
 		lchan->conn->lchan = lchan;
diff --git a/src/osmo-bsc/gsm_08_08.c b/src/osmo-bsc/gsm_08_08.c
index 062c878..48e87fc 100644
--- a/src/osmo-bsc/gsm_08_08.c
+++ b/src/osmo-bsc/gsm_08_08.c
@@ -525,7 +525,7 @@
 		/* FIXME: I have not the slightest idea what move_to_msc() intends to do; during lchan
 		 * FSM introduction, I changed this and hope it is the appropriate action. I actually
 		 * assume this is unused legacy code for osmo-bsc_nat?? */
-		gscon_release_lchans(_conn, false);
+		gscon_release_lchans(_conn, false, false);
 		return 1;
 	}
 
diff --git a/src/osmo-bsc/handover_fsm.c b/src/osmo-bsc/handover_fsm.c
index 0107ef0..8076eeb 100644
--- a/src/osmo-bsc/handover_fsm.c
+++ b/src/osmo-bsc/handover_fsm.c
@@ -243,7 +243,7 @@
 	struct mgwep_ci *ci;
 	if (conn->ho.new_lchan)
 		/* New lchan was activated but never passed to a conn */
-		lchan_release(conn->ho.new_lchan, true, true, RSL_ERR_EQUIPMENT_FAIL);
+		lchan_release(conn->ho.new_lchan, true, false, true, RSL_ERR_EQUIPMENT_FAIL);
 
 	ci = conn->ho.created_ci_for_msc;
 	if (ci) {
@@ -725,7 +725,7 @@
 				/* 3GPP TS 48.008 3.1.5.3.3 "Abnormal Conditions": if neither MS reports
 				 * HO Failure nor the MSC sends a Clear Command, we should release the
 				 * dedicated radio resources and send a Clear Request to the MSC. */
-				lchan_release(conn->lchan, false, true, GSM48_RR_CAUSE_ABNORMAL_TIMER);
+				lchan_release(conn->lchan, false, true, true, GSM48_RR_CAUSE_ABNORMAL_TIMER);
 				/* Once the channel release is through, the BSSMAP Clear will follow. */
 				break;
 			}
@@ -756,7 +756,7 @@
 	/* Detach the new_lchan last, so we can still see it in above logging */
 	if (ho->new_lchan) {
 		/* Release new lchan, it didn't work out */
-		lchan_release(ho->new_lchan, false, true, RSL_ERR_EQUIPMENT_FAIL);
+		lchan_release(ho->new_lchan, false, false, true, RSL_ERR_EQUIPMENT_FAIL);
 		ho->new_lchan = NULL;
 	}
 
diff --git a/src/osmo-bsc/lchan_fsm.c b/src/osmo-bsc/lchan_fsm.c
index 726a5bb..39f29f2 100644
--- a/src/osmo-bsc/lchan_fsm.c
+++ b/src/osmo-bsc/lchan_fsm.c
@@ -156,14 +156,14 @@
 			LOG_LCHAN(lchan, LOGL_ERROR,
 				  "lchan activation for assignment succeeded, but lchan has no conn:"
 				  " cannot trigger appropriate actions. Release.\n");
-			lchan_release(lchan, false, true, RSL_ERR_EQUIPMENT_FAIL);
+			lchan_release(lchan, false, false, true, RSL_ERR_EQUIPMENT_FAIL);
 			break;
 		}
 		if (!lchan->conn->assignment.fi) {
 			LOG_LCHAN(lchan, LOGL_ERROR,
 				  "lchan activation for assignment succeeded, but lchan has no"
 				  " assignment ongoing: cannot trigger appropriate actions. Release.\n");
-			lchan_release(lchan, false, true, RSL_ERR_EQUIPMENT_FAIL);
+			lchan_release(lchan, false, false, true, RSL_ERR_EQUIPMENT_FAIL);
 			break;
 		}
 		osmo_fsm_inst_dispatch(lchan->conn->assignment.fi, ASSIGNMENT_EV_LCHAN_ESTABLISHED,
@@ -177,14 +177,14 @@
 		if (!lchan->conn) {
 			LOG_LCHAN(lchan, LOGL_ERROR,
 				  "lchan activation for handover succeeded, but lchan has no conn\n");
-			lchan_release(lchan, false, true, RSL_ERR_EQUIPMENT_FAIL);
+			lchan_release(lchan, false, false, true, RSL_ERR_EQUIPMENT_FAIL);
 			break;
 		}
 		if (!lchan->conn->ho.fi) {
 			LOG_LCHAN(lchan, LOGL_ERROR,
 				  "lchan activation for handover succeeded, but lchan has no"
 				  " handover ongoing\n");
-			lchan_release(lchan, false, true, RSL_ERR_EQUIPMENT_FAIL);
+			lchan_release(lchan, false, false, true, RSL_ERR_EQUIPMENT_FAIL);
 			break;
 		}
 		osmo_fsm_inst_dispatch(lchan->conn->ho.fi, HO_EV_LCHAN_ESTABLISHED, lchan);
@@ -720,14 +720,14 @@
 			LOG_LCHAN(lchan, LOGL_ERROR,
 				  "lchan activation for assignment succeeded, but lchan has no conn:"
 				  " cannot trigger appropriate actions. Release.\n");
-			lchan_release(lchan, false, true, RSL_ERR_EQUIPMENT_FAIL);
+			lchan_release(lchan, false, false, true, RSL_ERR_EQUIPMENT_FAIL);
 			break;
 		}
 		if (!lchan->conn->assignment.fi) {
 			LOG_LCHAN(lchan, LOGL_ERROR,
 				  "lchan activation for assignment succeeded, but lchan has no"
 				  " assignment ongoing: cannot trigger appropriate actions. Release.\n");
-			lchan_release(lchan, false, true, RSL_ERR_EQUIPMENT_FAIL);
+			lchan_release(lchan, false, false, true, RSL_ERR_EQUIPMENT_FAIL);
 			break;
 		}
 		/* After the Chan Activ Ack, the MS expects to receive an RR Assignment Command.
@@ -740,14 +740,14 @@
 			LOG_LCHAN(lchan, LOGL_ERROR,
 				  "lchan activation for handover succeeded, but lchan has no conn:"
 				  " cannot trigger appropriate actions. Release.\n");
-			lchan_release(lchan, false, true, RSL_ERR_EQUIPMENT_FAIL);
+			lchan_release(lchan, false, false, true, RSL_ERR_EQUIPMENT_FAIL);
 			break;
 		}
 		if (!lchan->conn->ho.fi) {
 			LOG_LCHAN(lchan, LOGL_ERROR,
 				  "lchan activation for handover succeeded, but lchan has no"
 				  " handover ongoing: cannot trigger appropriate actions. Release.\n");
-			lchan_release(lchan, false, true, RSL_ERR_EQUIPMENT_FAIL);
+			lchan_release(lchan, false, false, true, RSL_ERR_EQUIPMENT_FAIL);
 			break;
 		}
 		/* After the Chan Activ Ack of the new lchan, send the MS an RR Handover Command on the
@@ -937,7 +937,7 @@
 		if (lchan->sapis[sapi])
 			LOG_LCHAN(lchan, LOGL_DEBUG, "SAPI[%d] = %d\n", sapi, lchan->sapis[sapi]);
 
-	if (lchan->conn && lchan->sapis[0] != LCHAN_SAPI_UNUSED)
+	if (lchan->do_rr_release && lchan->sapis[0] != LCHAN_SAPI_UNUSED)
 		gsm48_send_rr_release(lchan);
 
 	if (lchan->fi_rtp)
@@ -1291,7 +1291,7 @@
 	}
 }
 
-void lchan_release(struct gsm_lchan *lchan, bool sacch_deact,
+void lchan_release(struct gsm_lchan *lchan, bool sacch_deact, bool do_rr_release,
 		   bool err, enum gsm48_rr_cause cause_rr)
 {
 	if (!lchan || !lchan->fi)
@@ -1305,6 +1305,7 @@
 	lchan->release_in_error = err;
 	lchan->rsl_error_cause = cause_rr;
 	lchan->deact_sacch = sacch_deact;
+	lchan->do_rr_release = do_rr_release;
 
 	/* States waiting for events will notice the desire to release when done waiting, so it is enough
 	 * to mark for release. */
diff --git a/tests/gsm0408/gsm0408_test.c b/tests/gsm0408/gsm0408_test.c
index f2b85e4..1e6a097 100644
--- a/tests/gsm0408/gsm0408_test.c
+++ b/tests/gsm0408/gsm0408_test.c
@@ -986,7 +986,7 @@
 
 const char *bsc_subscr_name(struct bsc_subscr *bsub) { return NULL; }
 
-void lchan_release(struct gsm_lchan *lchan, bool sacch_deact,
+void lchan_release(struct gsm_lchan *lchan, bool do_deact_sacch, bool do_rr_release,
 		   bool err, enum gsm48_rr_cause cause_rr) {}
 
 int rsl_data_request(struct msgb *msg, uint8_t link_id) { return 0; }

-- 
To view, visit https://gerrit.osmocom.org/11664
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I666b3b4f45706d898d664d380bd0fd2b018be358
Gerrit-Change-Number: 11664
Gerrit-PatchSet: 2
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181114/015275dd/attachment.html>


More information about the gerrit-log mailing list