From b74a2c8e290e58a371c07b9d8a82872e54de6a2f Mon Sep 17 00:00:00 2001 From: Neels Hofmeyr Date: Wed, 24 Aug 2016 16:57:31 +0200 Subject: dyn TS: clearly use lchan[0], fixing minor confusion The dyn_ts_switchover_*() functions made the impression that they act on a specific lchan of a timeslot. The assumption that we would remember to use e.g. lchan[1] across a PDCH deactivation is brain damaged to begin with; and factually we always use lchan[0] anyway (the only case for using lchan[1] would be when switching to TCH/H, but the channel allocator will always return lchan[0] for that). Instead of the brain damaged lchan args, use a ts arg across all dyn_ts_switchover_*() functions, with one exception: The dyn_ts_switchover_complete() actually receives an RSL activation ack message on a specific lchan and needs to evaluate its lchan type. This will always be lchan[0] as it is now, but we should stick with the lchan the message was sent for. For PDCH, a check to use lchan[0] already existed, when composing the ACT message in rsl_chan_activate_lchan_as_pdch(). Replace with an assertion. Adjust all callers to pass ts instead of lchan. In dyn_ts_switchover_start(), there was a dead code check that jumps to switchover_complete() in case the pchan already matches. This never hits, because we only call dyn_ts_switchover_start() when pchans mismatch. So avoid guessing at passing lchan[0] to dyn_ts_switchover_complete() by not calling it at all but logging an error instead. In rsl_chan_activate_lchan(), we remember some values before going into switchover from PDCH. Explicitly store them in lchan[0], because after a PDCH release we have always and will activate no other than lchan[0]. In dyn_ts_switchover_continue(), move the check for any existing lchan->rqd_ref further above, and more correctly check all lchans that were so far valid on the TS, instead of just one. This partly prepares for a subsequent commit to fix the act_timer use for dyn TS: with the old lchan arg, we might schedule an activation timer on lchan[1] but receive an ack on lchan[0] (for PDCH), leading to an act_timer expiry. Change-Id: I3f5d48a9bdaa49a42a1908d4a03744638c59796a --- openbsc/include/openbsc/abis_rsl.h | 2 +- openbsc/src/libbsc/abis_rsl.c | 94 ++++++++++++++++++++++---------------- openbsc/src/libbsc/bsc_dyn_ts.c | 2 +- 3 files changed, 56 insertions(+), 42 deletions(-) diff --git a/openbsc/include/openbsc/abis_rsl.h b/openbsc/include/openbsc/abis_rsl.h index 8313f489e..758c5557a 100644 --- a/openbsc/include/openbsc/abis_rsl.h +++ b/openbsc/include/openbsc/abis_rsl.h @@ -108,7 +108,7 @@ int rsl_start_t3109(struct gsm_lchan *lchan); int rsl_direct_rf_release(struct gsm_lchan *lchan); void dyn_ts_init(struct gsm_bts_trx_ts *ts); -int dyn_ts_switchover_start(struct gsm_lchan *lchan, +int dyn_ts_switchover_start(struct gsm_bts_trx_ts *ts, enum gsm_phys_chan_config to_pchan); #endif /* RSL_MT_H */ diff --git a/openbsc/src/libbsc/abis_rsl.c b/openbsc/src/libbsc/abis_rsl.c index d04b70739..42281ba8f 100644 --- a/openbsc/src/libbsc/abis_rsl.c +++ b/openbsc/src/libbsc/abis_rsl.c @@ -55,8 +55,8 @@ enum sacch_deact { static int rsl_send_imm_assignment(struct gsm_lchan *lchan); static void error_timeout_cb(void *data); -static int dyn_ts_switchover_continue(struct gsm_lchan *lchan); -static int dyn_ts_switchover_failed(struct gsm_lchan *lchan, int rc); +static int dyn_ts_switchover_continue(struct gsm_bts_trx_ts *ts); +static int dyn_ts_switchover_failed(struct gsm_bts_trx_ts *ts, int rc); static void dyn_ts_switchover_complete(struct gsm_lchan *lchan); static void send_lchan_signal(int sig_no, struct gsm_lchan *lchan, @@ -460,9 +460,9 @@ static int rsl_chan_activate_lchan_as_pdch(struct gsm_lchan *lchan) struct abis_rsl_dchan_hdr *dh; /* This might be called after release of the second lchan of a TCH/H, - * but PDCH activation should always happen on the first lchan. So - * switch to lchan->nr == 0. */ - lchan = lchan->ts->lchan; + * but PDCH activation must always happen on the first lchan. Make sure + * the calling code passes the correct lchan. */ + OSMO_ASSERT(lchan == lchan->ts->lchan); rsl_lchan_set_state(lchan, LCHAN_S_ACT_REQ); @@ -521,16 +521,21 @@ int rsl_chan_activate_lchan(struct gsm_lchan *lchan, uint8_t act_type, enum gsm_phys_chan_config pchan_want; pchan_want = pchan_for_lchant(lchan->type); if (lchan->ts->dyn.pchan_is != pchan_want) { - lchan->dyn.act_type = act_type, - lchan->dyn.ho_ref = ho_ref; - lchan->dyn.rqd_ref = lchan->rqd_ref; - lchan->dyn.rqd_ta = lchan->rqd_ta; + /* + * Make sure to record on lchan[0] so that we'll find + * it after the PDCH release. + */ + struct gsm_lchan *lchan0 = lchan->ts->lchan; + lchan0->dyn.act_type = act_type, + lchan0->dyn.ho_ref = ho_ref; + lchan0->dyn.rqd_ref = lchan->rqd_ref; + lchan0->dyn.rqd_ta = lchan->rqd_ta; lchan->rqd_ref = NULL; lchan->rqd_ta = 0; DEBUGP(DRSL, "%s saved rqd_ref=%p ta=%u\n", - gsm_lchan_name(lchan), lchan->rqd_ref, - lchan->rqd_ta); - return dyn_ts_switchover_start(lchan, pchan_want); + gsm_lchan_name(lchan0), lchan0->rqd_ref, + lchan0->rqd_ta); + return dyn_ts_switchover_start(lchan->ts, pchan_want); } } @@ -774,7 +779,7 @@ static void error_timeout_cb(void *data) rsl_ipacc_pdch_activate(lchan->ts, 1); if (dyn_ts_should_switch_to_pdch(lchan->ts)) - dyn_ts_switchover_start(lchan, GSM_PCHAN_PDCH); + dyn_ts_switchover_start(lchan->ts, GSM_PCHAN_PDCH); } static int rsl_rx_rf_chan_rel_ack(struct gsm_lchan *lchan); @@ -910,11 +915,11 @@ static int rsl_rx_rf_chan_rel_ack(struct gsm_lchan *lchan) /* (a) */ if (ts->dyn.pchan_is != ts->dyn.pchan_want) - return dyn_ts_switchover_continue(lchan); + return dyn_ts_switchover_continue(ts); /* (b) */ if (dyn_ts_should_switch_to_pdch(ts)) - return dyn_ts_switchover_start(lchan, GSM_PCHAN_PDCH); + return dyn_ts_switchover_start(ts, GSM_PCHAN_PDCH); } /* @@ -2338,11 +2343,10 @@ static int abis_rsl_rx_ipacc(struct msgb *msg) return rc; } -int dyn_ts_switchover_start(struct gsm_lchan *lchan, +int dyn_ts_switchover_start(struct gsm_bts_trx_ts *ts, enum gsm_phys_chan_config to_pchan) { int ss; - struct gsm_bts_trx_ts *ts = lchan->ts; int rc = -EIO; OSMO_ASSERT(ts->pchan == GSM_PCHAN_TCH_F_TCH_H_PDCH); @@ -2360,15 +2364,14 @@ int dyn_ts_switchover_start(struct gsm_lchan *lchan, if (ts->dyn.pchan_is == to_pchan) { LOGP(DRSL, LOGL_INFO, - "%s %s Already is in %s mode, skipping switchover.\n", + "%s %s Already is in %s mode, cannot switchover.\n", gsm_ts_name(ts), gsm_pchan_name(ts->pchan), gsm_pchan_name(to_pchan)); - dyn_ts_switchover_complete(lchan); - return 0; + return -EINVAL; } /* Paranoia: let's make sure all is indeed released. */ - for (ss = 0; ss < ts_subslots(lchan->ts); ss++) { + for (ss = 0; ss < ts_subslots(ts); ss++) { struct gsm_lchan *lc = &ts->lchan[ss]; if (lc->state != LCHAN_S_NONE) { LOGP(DRSL, LOGL_ERROR, @@ -2386,16 +2389,16 @@ int dyn_ts_switchover_start(struct gsm_lchan *lchan, /* * To switch from PDCH, we need to initiate the release from the BSC * side. dyn_ts_switchover_continue() will be called from - * rsl_rx_rf_chan_rel_ack(). + * rsl_rx_rf_chan_rel_ack(). PDCH is always on lchan[0]. */ if (ts->dyn.pchan_is == GSM_PCHAN_PDCH) { - rsl_lchan_set_state(lchan, LCHAN_S_REL_REQ); - rc = rsl_rf_chan_release(lchan, 0, SACCH_NONE); + rsl_lchan_set_state(ts->lchan, LCHAN_S_REL_REQ); + rc = rsl_rf_chan_release(ts->lchan, 0, SACCH_NONE); if (rc) { LOGP(DRSL, LOGL_ERROR, "%s RSL RF Chan Release failed\n", gsm_ts_and_pchan_name(ts)); - return dyn_ts_switchover_failed(lchan, rc); + return dyn_ts_switchover_failed(ts, rc); } return 0; } @@ -2405,15 +2408,16 @@ int dyn_ts_switchover_start(struct gsm_lchan *lchan, * rsl_rx_rf_chan_rel_ack(), i.e. release is complete. Go ahead and * activate as new type. This will always be PDCH. */ - return dyn_ts_switchover_continue(lchan); + return dyn_ts_switchover_continue(ts); } -static int dyn_ts_switchover_continue(struct gsm_lchan *lchan) +static int dyn_ts_switchover_continue(struct gsm_bts_trx_ts *ts) { int rc; uint8_t act_type; uint8_t ho_ref; - struct gsm_bts_trx_ts *ts = lchan->ts; + int ss; + struct gsm_lchan *lchan; OSMO_ASSERT(ts->pchan == GSM_PCHAN_TCH_F_TCH_H_PDCH); DEBUGP(DRSL, "%s switchover: release complete," @@ -2427,6 +2431,25 @@ static int dyn_ts_switchover_continue(struct gsm_lchan *lchan) gsm_ts_and_pchan_name(ts)); return 0; } + + for (ss = 0; ss < ts_subslots(ts); ss++) { + lchan = &ts->lchan[ss]; + if (lchan->rqd_ref) { + LOGP(DRSL, LOGL_ERROR, + "%s During dyn TS switchover, expecting no" + " Request Reference to be pending. Discarding!\n", + gsm_lchan_name(lchan)); + talloc_free(lchan->rqd_ref); + lchan->rqd_ref = NULL; + } + } + + /* + * When switching pchan modes, all lchans are unused. So always + * activate whatever wants to be activated on the first lchan. (We + * wouldn't remember to use lchan[1] across e.g. a PDCH deact anyway) + */ + lchan = ts->lchan; /* * For TCH/x, the lchan->type has been set in lchan_alloc(), but it may @@ -2459,14 +2482,6 @@ static int dyn_ts_switchover_continue(struct gsm_lchan *lchan) : lchan->dyn.ho_ref; /* Fetch the rqd_ref back from before switchover started. */ - if (lchan->rqd_ref) { - LOGP(DRSL, LOGL_ERROR, - "%s During dyn TS switchover, expecting no" - " Request Reference to be pending. Discarding!\n", - gsm_lchan_name(lchan)); - talloc_free(lchan->rqd_ref); - lchan->rqd_ref = NULL; - } lchan->rqd_ref = lchan->dyn.rqd_ref; lchan->rqd_ta = lchan->dyn.rqd_ta; lchan->dyn.rqd_ref = NULL; @@ -2477,14 +2492,13 @@ static int dyn_ts_switchover_continue(struct gsm_lchan *lchan) LOGP(DRSL, LOGL_ERROR, "%s RSL Chan Activate failed\n", gsm_ts_and_pchan_name(ts)); - return dyn_ts_switchover_failed(lchan, rc); + return dyn_ts_switchover_failed(ts, rc); } return 0; } -static int dyn_ts_switchover_failed(struct gsm_lchan *lchan, int rc) +static int dyn_ts_switchover_failed(struct gsm_bts_trx_ts *ts, int rc) { - struct gsm_bts_trx_ts *ts = lchan->ts; ts->dyn.pchan_want = ts->dyn.pchan_is; LOGP(DRSL, LOGL_ERROR, "%s Error %d during dynamic channel switchover." " Going back to previous pchan.\n", gsm_ts_and_pchan_name(ts), @@ -2510,7 +2524,7 @@ static void dyn_ts_switchover_complete(struct gsm_lchan *lchan) LOGP(DRSL, LOGL_ERROR, "%s Requested transition does not match lchan type %s\n", gsm_ts_and_pchan_name(ts), - gsm_lchant_name(ts->lchan[0].type)); + gsm_lchant_name(lchan->type)); pchan_was = ts->dyn.pchan_is; ts->dyn.pchan_is = ts->dyn.pchan_want = pchan_act; diff --git a/openbsc/src/libbsc/bsc_dyn_ts.c b/openbsc/src/libbsc/bsc_dyn_ts.c index 456500a89..e5422fc5c 100644 --- a/openbsc/src/libbsc/bsc_dyn_ts.c +++ b/openbsc/src/libbsc/bsc_dyn_ts.c @@ -52,7 +52,7 @@ void tchf_tchh_pdch_ts_init(struct gsm_bts_trx_ts *ts) return; } - dyn_ts_switchover_start(ts->lchan, GSM_PCHAN_PDCH); + dyn_ts_switchover_start(ts, GSM_PCHAN_PDCH); } void dyn_ts_init(struct gsm_bts_trx_ts *ts) -- cgit v1.2.3