Change in ...osmo-bsc[master]: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel c...
laforge has submitted this change and it was merged. ( https://gerrit.osmocom.org/c/osmo-bsc/+/15408 ) Change subject: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel closed during WAIT_CC .. bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel closed during WAIT_CC TTCN3 BSC_Tests.TC_ms_rel_ind_does_not_cause_bssmap_reset seems to sometimes run into a race condition on the order of messages received by osmo-bsc comming from MSC and BTS. Usual (expected) scenario): BTS->BSC EST IND BSC->MSC CL3 Info BSC<-MSC CC BTS->BSC REL IND BTS<-BSC DEACT SACCH BSC->MSC ClearRequest BSC<-MSC ClearCommand BSC->MSC ClearComplete BTS<-BSC RF Chan Release BTS->BSC RF Chan Release ACK Sometimes CC message and REL IND message are received swapped (because they are sent by different components asynchronously in TTCN3). As a result, osmo-bsc was failing to go into CLEARING state and was unable to send the ClearRequest because CC was still not received. So the idea is to stay in WAIT_CC until CC is received, then check if the lchan was dropped and in that case go into clearing state. Change-Id: Id1abf5ee44c60925b478123409f26bd29006202b --- M src/osmo-bsc/bsc_subscr_conn_fsm.c 1 file changed, 28 insertions(+), 5 deletions(-) Approvals: Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/src/osmo-bsc/bsc_subscr_conn_fsm.c b/src/osmo-bsc/bsc_subscr_conn_fsm.c index f8784f9..60f035d 100644 --- a/src/osmo-bsc/bsc_subscr_conn_fsm.c +++ b/src/osmo-bsc/bsc_subscr_conn_fsm.c @@ -332,8 +332,19 @@ struct gsm_subscriber_connection *conn = fi->priv; switch (event) { case GSCON_EV_A_CONN_CFM: - /* MSC has confirmed the connection, we now change into the -* active state and wait there for further operations */ + /* MSC has confirmed the connection */ + + if (!conn->lchan) { + /* If associated lchan was released while we were waiting for the + confirmed connection, then instead simply drop the connection */ + LOGPFSML(fi, LOGL_INFO, +"Connection confirmed but lchan was dropped previously, clearing conn\n"); + osmo_fsm_inst_state_chg(conn->fi, ST_CLEARING, 60, 999); + gscon_bssmap_clear(conn, GSM0808_CAUSE_EQUIPMENT_FAILURE); + break; + } + + /* We now change into the active state and wait there for further operations. */ conn_fsm_state_chg(ST_ACTIVE); /* if there's user payload, forward it just like EV_MT_DTAP */ /* FIXME: Question: if there's user payload attached to the CC, forward it like EV_MT_DTAP? */ @@ -589,7 +600,7 @@ [ST_WAIT_CC] = { .name = "WAIT_CC", .in_event_mask = S(GSCON_EV_A_CONN_CFM), - .out_state_mask = S(ST_ACTIVE), + .out_state_mask = S(ST_ACTIVE) | S(ST_CLEARING), .action = gscon_fsm_wait_cc, }, [ST_ACTIVE] = { @@ -651,10 +662,22 @@ lchan_forget_conn(conn->lchan); conn->lchan = NULL; } + /* If the conn has no lchan anymore, it was released by the BTS and needs to Clear towards MSC. */ if (!conn->lchan) { - if (conn->fi->state != ST_CLEARING) + switch (conn->fi->state) { + case ST_WAIT_CC: + /* The SCCP connection was not yet confirmed by a CC, the BSSAP is not fully established + yet so we cannot release it. First wait for the CC, and release in gscon_fsm_wait_cc(). */ + break; + default: + /* Ensure that the FSM is in ST_CLEARING. */ osmo_fsm_inst_state_chg(conn->fi, ST_CLEARING, 60, 999); - gscon_bssmap_clear(conn, GSM0808_CAUSE_EQUIPMENT_FAILURE); + /* fall thru, omit an error log if already in ST_CLEARING */ + case ST_CLEARING: + /* Request a Clear Command from the MSC. */ + gscon_bssmap_clear(conn, GSM0808_CAUSE_EQUIPMENT_FAILURE); + break; + } } } -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/15408 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Id1abf5ee44c60925b478123409f26bd29006202b Gerrit-Change-Number: 15408 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-MessageType: merged
Change in ...osmo-bsc[master]: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel c...
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/15408 ) Change subject: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel closed during WAIT_CC .. Patch Set 4: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/15408 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Id1abf5ee44c60925b478123409f26bd29006202b Gerrit-Change-Number: 15408 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Comment-Date: Thu, 26 Sep 2019 19:32:03 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-bsc[master]: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel c...
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/15408 ) Change subject: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel closed during WAIT_CC .. Patch Set 4: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/15408 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Id1abf5ee44c60925b478123409f26bd29006202b Gerrit-Change-Number: 15408 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Comment-Date: Fri, 20 Sep 2019 15:23:18 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-bsc[master]: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel c...
Hello fixeria, neels, laforge, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-bsc/+/15408 to look at the new patch set (#4). Change subject: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel closed during WAIT_CC .. bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel closed during WAIT_CC TTCN3 BSC_Tests.TC_ms_rel_ind_does_not_cause_bssmap_reset seems to sometimes run into a race condition on the order of messages received by osmo-bsc comming from MSC and BTS. Usual (expected) scenario): BTS->BSC EST IND BSC->MSC CL3 Info BSC<-MSC CC BTS->BSC REL IND BTS<-BSC DEACT SACCH BSC->MSC ClearRequest BSC<-MSC ClearCommand BSC->MSC ClearComplete BTS<-BSC RF Chan Release BTS->BSC RF Chan Release ACK Sometimes CC message and REL IND message are received swapped (because they are sent by different components asynchronously in TTCN3). As a result, osmo-bsc was failing to go into CLEARING state and was unable to send the ClearRequest because CC was still not received. So the idea is to stay in WAIT_CC until CC is received, then check if the lchan was dropped and in that case go into clearing state. Change-Id: Id1abf5ee44c60925b478123409f26bd29006202b --- M src/osmo-bsc/bsc_subscr_conn_fsm.c 1 file changed, 28 insertions(+), 5 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/08/15408/4 -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/15408 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Id1abf5ee44c60925b478123409f26bd29006202b Gerrit-Change-Number: 15408 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-MessageType: newpatchset
Change in ...osmo-bsc[master]: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel c...
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/15408 ) Change subject: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel closed during WAIT_CC .. Patch Set 3: Code-Review+1 (2 comments) https://gerrit.osmocom.org/#/c/15408/3/src/osmo-bsc/bsc_subscr_conn_fsm.c File src/osmo-bsc/bsc_subscr_conn_fsm.c: https://gerrit.osmocom.org/#/c/15408/3/src/osmo-bsc/bsc_subscr_conn_fsm.c@340 PS3, Line 340: * simply drop the connection */ (use full line width) https://gerrit.osmocom.org/#/c/15408/3/src/osmo-bsc/bsc_subscr_conn_fsm.c@670 PS3, Line 670: yet so we cannot release it. First wait for the CC, and release in gscon_fsm_wait_cc(). */ (a '*' missing) -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/15408 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Id1abf5ee44c60925b478123409f26bd29006202b Gerrit-Change-Number: 15408 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 18 Sep 2019 00:44:08 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-bsc[master]: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel c...
Hello fixeria, neels, laforge, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-bsc/+/15408 to look at the new patch set (#3). Change subject: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel closed during WAIT_CC .. bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel closed during WAIT_CC TTCN3 BSC_Tests.TC_ms_rel_ind_does_not_cause_bssmap_reset seems to sometimes run into a race condition on the order of messages received by osmo-bsc comming from MSC and BTS. Usual (expected) scenario): BTS->BSC EST IND BSC->MSC CL3 Info BSC<-MSC CC BTS->BSC REL IND BTS<-BSC DEACT SACCH BSC->MSC ClearRequest BSC<-MSC ClearCommand BSC->MSC ClearComplete BTS<-BSC RF Chan Release BTS->BSC RF Chan Release ACK Sometimes CC message and REL IND message are received swapped (because they are sent by different components asynchronously in TTCN3). As a result, osmo-bsc was failing to go into CLEARING state and was unable to send the ClearRequest because CC was still not received. So the idea is to stay in WAIT_CC until CC is received, then check if the lchan was dropped and in that case go into clearing state. Change-Id: Id1abf5ee44c60925b478123409f26bd29006202b --- M src/osmo-bsc/bsc_subscr_conn_fsm.c 1 file changed, 28 insertions(+), 5 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/08/15408/3 -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/15408 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Id1abf5ee44c60925b478123409f26bd29006202b Gerrit-Change-Number: 15408 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-MessageType: newpatchset
Change in ...osmo-bsc[master]: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel c...
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/15408 ) Change subject: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel closed during WAIT_CC .. Patch Set 1: Code-Review-1 (2 comments) Found one side effect functional change, and got some style nitpicks https://gerrit.osmocom.org/#/c/15408/1/src/osmo-bsc/bsc_subscr_conn_fsm.c File src/osmo-bsc/bsc_subscr_conn_fsm.c: https://gerrit.osmocom.org/#/c/15408/1/src/osmo-bsc/bsc_subscr_conn_fsm.c@343 PS1, Line 343: if (conn->lchan) { (would prefer the early-exit style; main flow remains undiffed) if (!conn->lchan) { log... state_chg... clear... break; } conn_fsm_state_chg(ST_ACTIVE); https://gerrit.osmocom.org/#/c/15408/1/src/osmo-bsc/bsc_subscr_conn_fsm.c@671 PS1, Line 671: if (!conn->lchan && gscon_is_active(conn)) { Before, when we already were in the ST_CLEARING, we would still re-send the BSSMAP Clear Request again. I am not sure if that is desired, but if that needs to change, that should be a separate patch. It is fine for the BSC to send repeated Clear Requests, they are only polite hints for the MSC. Code design wise, it's not such a good idea to have gscon_is_active() listing all *other* states. If a new state gets added in the future, the author is almost guaranteed to forget to also change that function. Instead I would prefer to more explicitly except only this situation, e.g.: /* If the conn has no lchan anymore, it was released by the BTS and needs to Clear towards MSC. */ if (!conn->lchan) { switch (conn->fi_state) { case ST_INIT: case ST_WAIT_CC: /* The SCCP connection was not yet confirmed by a CC, the BSSAP is not fully established * yet. First wait for the CC, and release in gscon_fsm_wait_cc(). */ break; default: /* Ensure that the FSM is in ST_CLEARING. */ osmo_fsm_inst_state_chg(...ST_CLEARING...) /* fall thru, omit an error log if already in ST_CLEARING */ case ST_CLEARING: /* Request a Clear Command from the MSC. gscon_bssmap_clear([no diff] break; } } Are you sure that ST_INIT should also be in that condition? -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/15408 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Id1abf5ee44c60925b478123409f26bd29006202b Gerrit-Change-Number: 15408 Gerrit-PatchSet: 1 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Comment-Date: Mon, 16 Sep 2019 15:51:35 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-bsc[master]: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel c...
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/15408 ) Change subject: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel closed during WAIT_CC .. Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/15408 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Id1abf5ee44c60925b478123409f26bd29006202b Gerrit-Change-Number: 15408 Gerrit-PatchSet: 1 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Comment-Date: Fri, 06 Sep 2019 19:06:05 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...osmo-bsc[master]: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel c...
pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-bsc/+/15408 Change subject: bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel closed during WAIT_CC .. bsc_subscr_conn_fsm: Cleanly clear BSSAP conn if associated channel closed during WAIT_CC TTCN3 BSC_Tests.TC_ms_rel_ind_does_not_cause_bssmap_reset seems to sometimes run into a race condition on the order of messages received by osmo-bsc comming from MSC and BTS. Usual (expected) scenario): BTS->BSC EST IND BSC->MSC CL3 Info BSC<-MSC CC BTS->BSC REL IND BTS<-BSC DEACT SACCH BSC->MSC ClearRequest BSC<-MSC ClearCommand BSC->MSC ClearComplete BTS<-BSC RF Chan Release BTS->BSC RF Chan Release ACK Sometimes CC message and REL IND message are received swapped (because they are sent by different components asynchronously in TTCN3). As a result, osmo-bsc was failing to go into CLEARING state and was unable to send the ClearRequest because CC was still not received. So the idea is to stay in WAIT_CC until CC is received, then check if the lchan was dropped and in that case go into clearing state. Change-Id: Id1abf5ee44c60925b478123409f26bd29006202b --- M src/osmo-bsc/bsc_subscr_conn_fsm.c 1 file changed, 24 insertions(+), 8 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/08/15408/1 diff --git a/src/osmo-bsc/bsc_subscr_conn_fsm.c b/src/osmo-bsc/bsc_subscr_conn_fsm.c index f8784f9..b5edeb8 100644 --- a/src/osmo-bsc/bsc_subscr_conn_fsm.c +++ b/src/osmo-bsc/bsc_subscr_conn_fsm.c @@ -99,6 +99,12 @@ conn->network->T_defs, \ -1) +static bool gscon_is_active(struct gsm_subscriber_connection *conn) +{ + enum gscon_fsm_states st = conn->fi->state; + return st == ST_ACTIVE || st == ST_ASSIGNMENT || st == ST_HANDOVER; +} + /* forward MT DTAP from BSSAP side to RSL side */ static inline void submit_dtap(struct gsm_subscriber_connection *conn, struct msgb *msg) { @@ -333,10 +339,19 @@ switch (event) { case GSCON_EV_A_CONN_CFM: /* MSC has confirmed the connection, we now change into the -* active state and wait there for further operations */ - conn_fsm_state_chg(ST_ACTIVE); - /* if there's user payload, forward it just like EV_MT_DTAP */ - /* FIXME: Question: if there's user payload attached to the CC, forward it like EV_MT_DTAP? */ +* active state and wait there for further operations. */ + if (conn->lchan) { + conn_fsm_state_chg(ST_ACTIVE); + /* if there's user payload, forward it just like EV_MT_DTAP */ + /* FIXME: Question: if there's user payload attached to the CC, forward it like EV_MT_DTAP? */ + } else { + /* If associated lchan was released while we were +* waiting for the confirmed connection, then instead +* simply drop the connection */ + LOGPFSML(fi, LOGL_INFO, "Connection confirmed but lchan was dropped before, clearing conn\n"); + osmo_fsm_inst_state_chg(conn->fi, ST_CLEARING, 60, 999); + gscon_bssmap_clear(conn, GSM0808_CAUSE_EQUIPMENT_FAILURE); + } break; default: OSMO_ASSERT(false); @@ -589,7 +604,7 @@ [ST_WAIT_CC] = { .name = "WAIT_CC", .in_event_mask = S(GSCON_EV_A_CONN_CFM), - .out_state_mask = S(ST_ACTIVE), + .out_state_mask = S(ST_ACTIVE) | S(ST_CLEARING), .action = gscon_fsm_wait_cc, }, [ST_ACTIVE] = { @@ -651,9 +666,10 @@ lchan_forget_conn(conn->lchan); conn->lchan = NULL; } - if (!conn->lchan) { - if (conn->fi->state != ST_CLEARING) - osmo_fsm_inst_state_chg(conn->fi, ST_CLEARING, 60, 999); + /* if conn is not active we cannot send bssap. If on WAIT_CC, then let's +* wait until CC is received to tear down */ + if (!conn->lchan && gscon_is_active(conn)) { + osmo_fsm_inst_state_chg(conn->fi, ST_CLEARING, 60, 999); gscon_bssmap_clear(conn, GSM0808_CAUSE_EQUIPMENT_FAILURE); } } -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/15408 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Id1abf5ee44c60925b478123409f26bd29006202b Gerrit-Change-Number: 15408 Gerrit-PatchSet: 1 Gerrit-Owner: pespin Gerrit-MessageType: newchange