Neels Hofmeyr has submitted this change and it was merged. ( https://gerrit.osmocom.org/11667 )
Change subject: lchan release: always Deact SACCH ...................................................................... lchan release: always Deact SACCH If an lchan is being released and had a SACCH active, there is no reason to omit the Deact SACCH message ever. All of the callers that passed do_deact_sacch = false did so for no good reason. Drop the do_deact_sacch flag everywhere and, when the lchan type matches and SAPI[0] is still active, simply always send a Deact SACCH message. The do_deact_sacch flag was carried over from legacy code, by me, mainly because I never really understood why it was there. I do hope I'm correct now, asserting that having this flag makes no sense. Change-Id: Id3301df059582da2377ef82feae554e94fa42035 --- 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 M tests/handover/handover_test.c 13 files changed, 32 insertions(+), 32 deletions(-) Approvals: Neels Hofmeyr: Looks good to me, approved Jenkins Builder: Verified diff --git a/include/osmocom/bsc/bsc_subscr_conn_fsm.h b/include/osmocom/bsc/bsc_subscr_conn_fsm.h index fcdba50..f5ed7bd 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, bool do_rr_release); +void gscon_release_lchans(struct gsm_subscriber_connection *conn, 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 5805a5f..3712b97 100644 --- a/include/osmocom/bsc/gsm_data.h +++ b/include/osmocom/bsc/gsm_data.h @@ -513,7 +513,6 @@ /* If an event to release the lchan comes in while still waiting for responses, just mark this * 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 d3315a6..48cd383 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 do_deact_sacch, bool do_rr_release, +void lchan_release(struct gsm_lchan *lchan, 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 0ec8fbc..c16f044 100644 --- a/src/osmo-bsc/abis_rsl.c +++ b/src/osmo-bsc/abis_rsl.c @@ -896,7 +896,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, false, true, *cause_p); + lchan_release(lchan, 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 653681e..93362f8 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, false, true, RSL_ERR_EQUIPMENT_FAIL); + lchan_release(lchan, 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, true, RSL_ERR_EQUIPMENT_FAIL); + lchan_release(conn->lchan, 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 5856d7a..ec06e9b 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 do_rr_release, bool err, enum gsm48_rr_cause cause_rr) + 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, do_rr_release, err, cause_rr); + lchan_release(lchan, do_rr_release, err, cause_rr); } -void gscon_release_lchans(struct gsm_subscriber_connection *conn, bool do_sacch_deact, bool do_rr_release) +void gscon_release_lchans(struct gsm_subscriber_connection *conn, 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, do_rr_release, false, 0); + gscon_release_lchan(conn, conn->lchan, 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, false, 0); + gscon_release_lchan(conn, old_lchan, 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, true); + gscon_release_lchans(conn, 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, true); + gscon_release_lchans(conn, 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, true, RSL_ERR_INTERWORKING); + gscon_release_lchan(conn, conn->lchan, 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 9f42d8a..8eb0692 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, !!(lchan->conn), false, 0); + lchan_release(lchan, !!(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 c3dd307..4659c1a 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, false, true, RSL_ERR_EQUIPMENT_FAIL); + lchan_release(lchan, 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 48e87fc..062c878 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, false); + gscon_release_lchans(_conn, false); return 1; } diff --git a/src/osmo-bsc/handover_fsm.c b/src/osmo-bsc/handover_fsm.c index 8076eeb..f2836cf 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, false, true, RSL_ERR_EQUIPMENT_FAIL); + lchan_release(conn->ho.new_lchan, 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, true, GSM48_RR_CAUSE_ABNORMAL_TIMER); + lchan_release(conn->lchan, 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, false, true, RSL_ERR_EQUIPMENT_FAIL); + lchan_release(ho->new_lchan, 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 f432644..5428d97 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, false, true, RSL_ERR_EQUIPMENT_FAIL); + lchan_release(lchan, 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, false, true, RSL_ERR_EQUIPMENT_FAIL); + lchan_release(lchan, 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, false, true, RSL_ERR_EQUIPMENT_FAIL); + lchan_release(lchan, 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, false, true, RSL_ERR_EQUIPMENT_FAIL); + lchan_release(lchan, 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, false, true, RSL_ERR_EQUIPMENT_FAIL); + lchan_release(lchan, 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, false, true, RSL_ERR_EQUIPMENT_FAIL); + lchan_release(lchan, 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, false, true, RSL_ERR_EQUIPMENT_FAIL); + lchan_release(lchan, 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, false, true, RSL_ERR_EQUIPMENT_FAIL); + lchan_release(lchan, 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 @@ -935,7 +935,7 @@ if (lchan->fi_rtp) osmo_fsm_inst_dispatch(lchan->fi_rtp, LCHAN_RTP_EV_RELEASE, 0); - if (lchan->deact_sacch && should_sacch_deact(lchan)) + if (should_sacch_deact(lchan)) rsl_deact_sacch(lchan); } @@ -1296,7 +1296,7 @@ } } -void lchan_release(struct gsm_lchan *lchan, bool sacch_deact, bool do_rr_release, +void lchan_release(struct gsm_lchan *lchan, bool do_rr_release, bool err, enum gsm48_rr_cause cause_rr) { if (!lchan || !lchan->fi) @@ -1309,7 +1309,6 @@ struct osmo_fsm_inst *fi = lchan->fi; 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 diff --git a/tests/gsm0408/gsm0408_test.c b/tests/gsm0408/gsm0408_test.c index 1e6a097..d15e149 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 do_deact_sacch, bool do_rr_release, +void lchan_release(struct gsm_lchan *lchan, 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; } diff --git a/tests/handover/handover_test.c b/tests/handover/handover_test.c index 7cb4086..f728c5b 100644 --- a/tests/handover/handover_test.c +++ b/tests/handover/handover_test.c @@ -462,6 +462,8 @@ break; case RSL_MT_IPAC_CRCX: break; + case RSL_MT_DEACTIVATE_SACCH: + break; default: printf("unknown rsl message=0x%x\n", dh->c.msg_type); } -- To view, visit https://gerrit.osmocom.org/11667 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: Id3301df059582da2377ef82feae554e94fa42035 Gerrit-Change-Number: 11667 Gerrit-PatchSet: 3 Gerrit-Owner: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Pau Espin Pedrol <pes...@sysmocom.de>