From 3b9dcb3fc1abfa3780b4da2a9a05bd79f65a5c44 Mon Sep 17 00:00:00 2001 From: Philipp Maier Date: Mon, 22 Oct 2018 14:21:57 +0200 Subject: gsm_04_08: improve gsm48_multirate_config() The function gsm48_multirate_config() generates the multirate configuration IE, that is sent via RSL to configure the active set of AMR codecs inside the BTS. The function already works, but it does not check the input data for consistancy. Lets add some consistancy check to make sure that inconsistant parameters are rejected. Also allow the output pointer to be NULL, so that the function can be used to perform a dry run to be able to verify parameters. - Check for invalid / inconsistant configuration parameters - Perform a dry-run when lv pointer is set to NULL Change-Id: I06beb7dd7236c81c3a91af4d09c31891f4b910a4 Related: OS#3529 --- include/osmocom/bsc/gsm_04_08_rr.h | 3 +- src/osmo-bsc/bsc_vty.c | 5 +- src/osmo-bsc/gsm_04_08_rr.c | 112 +++++++++++++++++++++++------- tests/gsm0408/Makefile.am | 1 + tests/gsm0408/gsm0408_test.c | 138 +++++++++++++++++++++++++++++++++++++ tests/gsm0408/gsm0408_test.ok | 4 ++ 6 files changed, 237 insertions(+), 26 deletions(-) diff --git a/include/osmocom/bsc/gsm_04_08_rr.h b/include/osmocom/bsc/gsm_04_08_rr.h index e2e861db4..8e4f78785 100644 --- a/include/osmocom/bsc/gsm_04_08_rr.h +++ b/include/osmocom/bsc/gsm_04_08_rr.h @@ -23,7 +23,8 @@ int gsm48_handle_paging_resp(struct gsm_subscriber_connection *conn, struct msgb *msg, struct bsc_subscr *bsub); int gsm48_send_rr_classmark_enquiry(struct gsm_lchan *lchan); int gsm48_send_rr_ciph_mode(struct gsm_lchan *lchan, int want_imeisv); -int gsm48_multirate_config(uint8_t *lv, const struct amr_multirate_conf *mr, const struct amr_mode *modes); +int gsm48_multirate_config(uint8_t *lv, const struct gsm48_multi_rate_conf *mr_conf, + const struct amr_mode *modes, unsigned int num_modes); struct msgb *gsm48_make_ho_cmd(struct gsm_lchan *new_lchan, uint8_t power_command, uint8_t ho_ref); int gsm48_send_ho_cmd(struct gsm_lchan *old_lchan, struct gsm_lchan *new_lchan, uint8_t power_command, uint8_t ho_ref); diff --git a/src/osmo-bsc/bsc_vty.c b/src/osmo-bsc/bsc_vty.c index f156cc874..08b5dad77 100644 --- a/src/osmo-bsc/bsc_vty.c +++ b/src/osmo-bsc/bsc_vty.c @@ -4391,11 +4391,12 @@ static int lchan_set_single_amr_mode(struct gsm_lchan *lchan, uint8_t amr_mode) mr.ms_mode[0].mode = amr_mode; mr.bts_mode[0].mode = amr_mode; + mr.num_modes = 1; /* encode this configuration into the lchan for both uplink and * downlink direction */ - gsm48_multirate_config(lchan->mr_ms_lv, &mr, mr.ms_mode); - gsm48_multirate_config(lchan->mr_bts_lv, &mr, mr.bts_mode); + gsm48_multirate_config(lchan->mr_ms_lv, mr_conf, mr.ms_mode, mr.num_modes); + gsm48_multirate_config(lchan->mr_bts_lv, mr_conf, mr.bts_mode, mr.num_modes); return 0; } diff --git a/src/osmo-bsc/gsm_04_08_rr.c b/src/osmo-bsc/gsm_04_08_rr.c index 35044a320..4659c1ac9 100644 --- a/src/osmo-bsc/gsm_04_08_rr.c +++ b/src/osmo-bsc/gsm_04_08_rr.c @@ -321,47 +321,113 @@ static void gsm48_cell_desc(struct gsm48_cell_desc *cd, cd->arfcn_lo = bts->c0->arfcn & 0xff; } -/*! \brief Encode a TS 04.08 multirate config LV according to 10.5.2.21aa - * \param[out] lv caller-allocated buffer of 7 bytes. First octet is IS length - * \param[in] mr multi-rate configuration to encode - * \param[in] modes array describing the AMR modes - * \returns 0 on success */ -int gsm48_multirate_config(uint8_t *lv, const struct amr_multirate_conf *mr, const struct amr_mode *modes) +/*! \brief Encode a TS 04.08 multirate config LV according to 10.5.2.21aa. + * \param[out] lv caller-allocated buffer of 7 bytes. First octet is is length. + * \param[in] mr_conf multi-rate configuration to encode (selected modes). + * \param[in] modes array describing the AMR modes. + * \param[in] num_modes length of the modes array. + * \returns 0 on success, -EINVAL on failure. */ +int gsm48_multirate_config(uint8_t *lv, + const struct gsm48_multi_rate_conf *mr_conf, + const struct amr_mode *modes, unsigned int num_modes) { - int num = 0, i; + int num = 0; + unsigned int i; + unsigned int k; + unsigned int m = 0; + bool mode_valid; + uint8_t *gsm48_ie = (uint8_t *) mr_conf; + const struct amr_mode *modes_selected[4]; + + /* Check if modes for consistency (order and duplicates) */ + for (i = 0; i < num_modes; i++) { + if (i > 0 && modes[i - 1].mode > modes[i].mode) { + LOGP(DRR, LOGL_ERROR, + "BUG: Multirate codec with inconsistant config (mode order).\n"); + return -EINVAL; + } + if (i > 0 && modes[i - 1].mode == modes[i].mode) { + LOGP(DRR, LOGL_ERROR, + "BUG: Multirate codec with inconsistant config (duplicate modes).\n"); + return -EINVAL; + } + } + /* Check if the active set that is defined in mr_conf has at least one + * mode but not more than 4 modes set */ for (i = 0; i < 8; i++) { - if (((mr->gsm48_ie[1] >> i) & 1)) + if (((gsm48_ie[1] >> i) & 1)) num++; } if (num > 4) { - LOGP(DRR, LOGL_ERROR, "BUG: Using multirate codec with too " - "many modes in config.\n"); - num = 4; + LOGP(DRR, LOGL_ERROR, + "BUG: Multirate codec with too many modes in config.\n"); + return -EINVAL; } if (num < 1) { - LOGP(DRR, LOGL_ERROR, "BUG: Using multirate codec with no " - "mode in config.\n"); - num = 1; + LOGP(DRR, LOGL_ERROR, + "BUG: Multirate codec with no mode in config.\n"); + return -EINVAL; } + /* Do not accept excess hysteresis or threshold values */ + for (i = 0; i < num_modes; i++) { + if (modes[i].threshold >= 64) { + LOGP(DRR, LOGL_ERROR, + "BUG: Multirate codec with excessive threshold values.\n"); + return -EINVAL; + } + if (modes[i].hysteresis >= 16) { + LOGP(DRR, LOGL_ERROR, + "BUG: Multirate codec with excessive hysteresis values.\n"); + return -EINVAL; + } + } + + /* Scan through the selected modes and find a matching threshold/ + * hysteresis value for that mode. */ + for (i = 0; i < 8; i++) { + if (((gsm48_ie[1] >> i) & 1)) { + mode_valid = false; + for (k = 0; k < num_modes; k++) { + if (modes[k].mode == i) { + mode_valid = true; + modes_selected[m] = &modes[k]; + m++; + } + } + if (!mode_valid) { + LOGP(DRR, LOGL_ERROR, + "BUG: Multirate codec with inconsistant config (no mode defined).\n"); + return -EINVAL; + } + } + } + OSMO_ASSERT(m <= 4); + + /* When the caller is not interested in any result, skip the actual + * composition of the IE (dry run) */ + if (!lv) + return 0; + + /* Compose output buffer */ lv[0] = (num == 1) ? 2 : (num + 2); - memcpy(lv + 1, mr->gsm48_ie, 2); + memcpy(lv + 1, gsm48_ie, 2); if (num == 1) return 0; - lv[3] = modes[0].threshold & 0x3f; - lv[4] = modes[0].hysteresis << 4; + lv[3] = modes_selected[0]->threshold & 0x3f; + lv[4] = modes_selected[0]->hysteresis << 4; if (num == 2) return 0; - lv[4] |= (modes[1].threshold & 0x3f) >> 2; - lv[5] = modes[1].threshold << 6; - lv[5] |= (modes[1].hysteresis & 0x0f) << 2; + lv[4] |= (modes_selected[1]->threshold & 0x3f) >> 2; + lv[5] = modes_selected[1]->threshold << 6; + lv[5] |= (modes_selected[1]->hysteresis & 0x0f) << 2; if (num == 3) return 0; - lv[5] |= (modes[2].threshold & 0x3f) >> 4; - lv[6] = modes[2].threshold << 4; - lv[6] |= modes[2].hysteresis & 0x0f; + lv[5] |= (modes_selected[2]->threshold & 0x3f) >> 4; + lv[6] = modes_selected[2]->threshold << 4; + lv[6] |= modes_selected[2]->hysteresis & 0x0f; return 0; } diff --git a/tests/gsm0408/Makefile.am b/tests/gsm0408/Makefile.am index d790fc870..b207f8b09 100644 --- a/tests/gsm0408/Makefile.am +++ b/tests/gsm0408/Makefile.am @@ -23,6 +23,7 @@ gsm0408_test_SOURCES = \ $(NULL) gsm0408_test_LDADD = \ + $(top_builddir)/src/osmo-bsc/gsm_04_08_rr.o \ $(top_builddir)/src/osmo-bsc/arfcn_range_encode.o \ $(top_builddir)/src/osmo-bsc/gsm_data.o \ $(top_builddir)/src/osmo-bsc/gsm_timers.o \ diff --git a/tests/gsm0408/gsm0408_test.c b/tests/gsm0408/gsm0408_test.c index bc2777d3c..faeca39fa 100644 --- a/tests/gsm0408/gsm0408_test.c +++ b/tests/gsm0408/gsm0408_test.c @@ -35,6 +35,8 @@ #include #include +#include + #define COMPARE(result, op, value) \ if (!((result) op (value))) {\ fprintf(stderr, "Compare failed. Was %x should be %x in %s:%d\n",result, value, __FILE__, __LINE__); \ @@ -799,6 +801,106 @@ static void test_gsm48_ra_id_by_bts() OSMO_ASSERT(pass); } +static void test_gsm48_multirate_config() +{ + uint8_t lv[7]; + struct gsm48_multi_rate_conf *gsm48_ie; + struct amr_multirate_conf mr; + int rc; + + memset(&mr, 0, sizeof(mr)); + + /* Use some made up threshold and hysteresis values */ + mr.ms_mode[0].threshold = 11; + mr.ms_mode[1].threshold = 12; + mr.ms_mode[2].threshold = 13; + mr.ms_mode[0].hysteresis = 15; + mr.ms_mode[1].hysteresis = 12; + mr.ms_mode[2].hysteresis = 8; + + gsm48_ie = (struct gsm48_multi_rate_conf *)&mr.gsm48_ie; + gsm48_ie->ver = 1; + gsm48_ie->m5_90 = 1; + gsm48_ie->m7_40 = 1; + gsm48_ie->m7_95 = 1; + gsm48_ie->m12_2 = 1; + + /* Test #1: Normal configuration with 4 active set members */ + mr.ms_mode[0].mode = 2; + mr.ms_mode[1].mode = 4; + mr.ms_mode[2].mode = 5; + mr.ms_mode[3].mode = 7; + rc = gsm48_multirate_config(lv, gsm48_ie, mr.ms_mode, 4); + OSMO_ASSERT(rc == 0); + printf("gsm48_multirate_config(): rc=%i, lv=%s\n", rc, + osmo_hexdump_nospc(lv, lv[0])); + + /* Test #2: 4 active set members, but wrong mode order: */ + mr.ms_mode[3].mode = 2; + mr.ms_mode[2].mode = 4; + mr.ms_mode[1].mode = 5; + mr.ms_mode[0].mode = 7; + rc = gsm48_multirate_config(lv, gsm48_ie, mr.ms_mode, 4); + OSMO_ASSERT(rc == -EINVAL); + + /* Test #3: Normal configuration with 3 active set members */ + mr.ms_mode[0].mode = 2; + mr.ms_mode[1].mode = 4; + mr.ms_mode[2].mode = 5; + mr.ms_mode[3].mode = 0; + gsm48_ie->m12_2 = 0; + mr.ms_mode[2].threshold = 0; + mr.ms_mode[2].hysteresis = 0; + + rc = gsm48_multirate_config(lv, gsm48_ie, mr.ms_mode, 3); + OSMO_ASSERT(rc == 0); + printf("gsm48_multirate_config(): rc=%i, lv=%s\n", rc, + osmo_hexdump_nospc(lv, lv[0])); + + /* Test #4: 3 active set members, but wrong mode order: */ + mr.ms_mode[0].mode = 2; + mr.ms_mode[2].mode = 4; + mr.ms_mode[1].mode = 5; + rc = gsm48_multirate_config(lv, gsm48_ie, mr.ms_mode, 3); + OSMO_ASSERT(rc == -EINVAL); + + /* Test #5: Normal configuration with 2 active set members */ + mr.ms_mode[0].mode = 2; + mr.ms_mode[1].mode = 4; + mr.ms_mode[2].mode = 0; + gsm48_ie->m7_95 = 0; + mr.ms_mode[1].threshold = 0; + mr.ms_mode[1].hysteresis = 0; + + rc = gsm48_multirate_config(lv, gsm48_ie, mr.ms_mode, 2); + OSMO_ASSERT(rc == 0); + printf("gsm48_multirate_config(): rc=%i, lv=%s\n", rc, + osmo_hexdump_nospc(lv, lv[0])); + + /* Test #6: 2 active set members, but wrong mode order: */ + mr.ms_mode[1].mode = 2; + mr.ms_mode[0].mode = 4; + rc = gsm48_multirate_config(lv, gsm48_ie, mr.ms_mode, 2); + OSMO_ASSERT(rc == -EINVAL); + + /* Test #7: Normal configuration with 1 active set member */ + mr.ms_mode[0].mode = 2; + mr.ms_mode[1].mode = 0; + gsm48_ie->m7_40 = 0; + mr.ms_mode[0].threshold = 0; + mr.ms_mode[0].hysteresis = 0; + + rc = gsm48_multirate_config(lv, gsm48_ie, mr.ms_mode, 1); + OSMO_ASSERT(rc == 0); + printf("gsm48_multirate_config(): rc=%i, lv=%s\n", rc, + osmo_hexdump_nospc(lv, lv[0])); + + /* Test #8: 0 active set members: */ + mr.ms_mode[0].mode = 0; + rc = gsm48_multirate_config(lv, gsm48_ie, mr.ms_mode, 1); + OSMO_ASSERT(rc == -EINVAL); +} + static const struct log_info_cat log_categories[] = { }; @@ -839,6 +941,8 @@ int main(int argc, char **argv) test_gsm48_ra_id_by_bts(); + test_gsm48_multirate_config(); + printf("Done.\n"); return EXIT_SUCCESS; @@ -854,3 +958,37 @@ bool on_gsm_ts_init(struct gsm_bts_trx_ts *ts) } void ts_fsm_alloc(struct gsm_bts_trx_ts *ts) {} + +void bsc_cm_update(struct gsm_subscriber_connection *conn, + const uint8_t *cm2, uint8_t cm2_len, + const uint8_t *cm3, uint8_t cm3_len) {} + +int rsl_siemens_mrpci(struct gsm_lchan *lchan, struct rsl_mrpci *mrpci) +{ return 0; } + +int rsl_chan_mode_modify_req(struct gsm_lchan *ts) { return 0; } + +int rsl_tx_ipacc_crcx(struct gsm_lchan *lchan) { return 0; } + +void gscon_submit_rsl_dtap(struct gsm_subscriber_connection *conn, + struct msgb *msg, int link_id, int allow_sacch) {} + +bool lchan_may_receive_data(struct gsm_lchan *lchan) { return true; } + +int bsc_compl_l3(struct gsm_subscriber_connection *conn, struct msgb *msg, + uint16_t chosen_channel) { return 0; } + +void bsc_dtap(struct gsm_subscriber_connection *conn, uint8_t link_id, + struct msgb *msg) {} + +void bsc_cipher_mode_compl(struct gsm_subscriber_connection *conn, + struct msgb *msg, uint8_t chosen_encr) {} + +const char *bsc_subscr_name(struct bsc_subscr *bsub) { return NULL; } + +void lchan_release(struct gsm_lchan *lchan, bool sacch_deact, + bool err, enum gsm48_rr_cause cause_rr) {} + +int rsl_data_request(struct msgb *msg, uint8_t link_id) { return 0; } + +int rsl_encryption_cmd(struct msgb *msg) { return 0; } diff --git a/tests/gsm0408/gsm0408_test.ok b/tests/gsm0408/gsm0408_test.ok index 727072137..9424167f3 100644 --- a/tests/gsm0408/gsm0408_test.ok +++ b/tests/gsm0408/gsm0408_test.ok @@ -226,4 +226,8 @@ test_gsm48_ra_id_by_bts[3]: digits='000000' lac=0x0000=htons(0) rac=0x00=0 pass test_gsm48_ra_id_by_bts[4]: digits='999999' lac=0xffff=htons(65535) rac=0xff=255 pass test_gsm48_ra_id_by_bts[5]: digits='09f909' lac=0xcdab=htons(43981) rac=0xab=171 pass test_gsm48_ra_id_by_bts[6]: digits='090990' lac=0xcdab=htons(43981) rac=0xab=171 pass +gsm48_multirate_config(): rc=0, lv=0620b40bf330 +gsm48_multirate_config(): rc=0, lv=0520340bf3 +gsm48_multirate_config(): rc=0, lv=0420140b +gsm48_multirate_config(): rc=0, lv=0220 Done. -- cgit v1.2.3