[osmo-pcu 3/5] bts: Return the special type for {ul, dl}_tbf_by_* functions

[Daniel Willmann]

Start returning the special type instead of the base gprs_rlcmac_tbf
when retrieving a TBF.

Ticket: SYS#389
Sponsored-by: On-Waves ehf
---
src/bts.cpp | 36 ++++++++++++++++++------------------
src/bts.h | 16 ++++++++--------
2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/src/bts.cpp b/src/bts.cpp
index f614dab..68d72f2 100644
--- a/src/bts.cpp
+++ b/src/bts.cpp
@@ -217,15 +217,15 @@ int BTS::add_paging(uint8_t chan_needed, uint8_t *identity_lv)
}

/* search for active downlink tbf */
-gprs_rlcmac_tbf *BTS::dl_tbf_by_tlli(uint32_t tlli)
+gprs_rlcmac_dl_tbf *BTS::dl_tbf_by_tlli(uint32_t tlli)
{
- return tbf_by_tlli(tlli, GPRS_RLCMAC_DL_TBF);
+ return (gprs_rlcmac_dl_tbf *)tbf_by_tlli(tlli, GPRS_RLCMAC_DL_TBF);
}

/* search for active uplink tbf */
-gprs_rlcmac_tbf *BTS::ul_tbf_by_tlli(uint32_t tlli)
+gprs_rlcmac_ul_tbf *BTS::ul_tbf_by_tlli(uint32_t tlli)
{
- return tbf_by_tlli(tlli, GPRS_RLCMAC_UL_TBF);
+ return (gprs_rlcmac_ul_tbf *)tbf_by_tlli(tlli, GPRS_RLCMAC_UL_TBF);
}

/* search for active downlink or uplink tbf */
@@ -250,9 +250,9 @@ gprs_rlcmac_tbf *BTS::tbf_by_tlli(uint32_t tlli, enum gprs_rlcmac_tbf_direction
return NULL;
}

-gprs_rlcmac_tbf *BTS::dl_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts)
+gprs_rlcmac_dl_tbf *BTS::dl_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts)
{
- struct gprs_rlcmac_tbf *tbf;
+ struct gprs_rlcmac_dl_tbf *tbf;

/* only one TBF can poll on specific TS/FN, because scheduler can only
* schedule one downlink control block (with polling) at a FN per TS */
@@ -262,14 +262,14 @@ gprs_rlcmac_tbf *BTS::dl_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts)
&& tbf->poll_fn == fn && tbf->trx->trx_no == trx
&& tbf->control_ts == ts) {
OSMO_ASSERT(tbf->direction == GPRS_RLCMAC_DL_TBF);
- return tbf;
+ return (gprs_rlcmac_dl_tbf *)tbf;
}
}
return NULL;
}
-gprs_rlcmac_tbf *BTS::ul_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts)
+gprs_rlcmac_ul_tbf *BTS::ul_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts)
{
- struct gprs_rlcmac_tbf *tbf;
+ struct gprs_rlcmac_ul_tbf *tbf;

/* only one TBF can poll on specific TS/FN, because scheduler can only
* schedule one downlink control block (with polling) at a FN per TS */
@@ -279,22 +279,22 @@ gprs_rlcmac_tbf *BTS::ul_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts)
&& tbf->poll_fn == fn && tbf->trx->trx_no == trx
&& tbf->control_ts == ts) {
OSMO_ASSERT(tbf->direction == GPRS_RLCMAC_UL_TBF);
- return tbf;
+ return (gprs_rlcmac_ul_tbf *)tbf;
}
}
return NULL;
}

/* lookup downlink TBF Entity (by TFI) */
-gprs_rlcmac_tbf *BTS::dl_tbf_by_tfi(uint8_t tfi, uint8_t trx)
+gprs_rlcmac_dl_tbf *BTS::dl_tbf_by_tfi(uint8_t tfi, uint8_t trx)
{
- return tbf_by_tfi(tfi, trx, GPRS_RLCMAC_DL_TBF);
+ return (gprs_rlcmac_dl_tbf *)tbf_by_tfi(tfi, trx, GPRS_RLCMAC_DL_TBF);
}

/* lookup uplink TBF Entity (by TFI) */
-gprs_rlcmac_tbf *BTS::ul_tbf_by_tfi(uint8_t tfi, uint8_t trx)
+gprs_rlcmac_ul_tbf *BTS::ul_tbf_by_tfi(uint8_t tfi, uint8_t trx)
{
- return tbf_by_tfi(tfi, trx, GPRS_RLCMAC_UL_TBF);
+ return (gprs_rlcmac_ul_tbf *)tbf_by_tfi(tfi, trx, GPRS_RLCMAC_UL_TBF);
}

/* lookup TBF Entity (by TFI) */
@@ -1047,12 +1047,12 @@ gprs_rlcmac_tbf *gprs_rlcmac_pdch::tbf_from_list_by_tfi(struct llist_head *tbf_l
return NULL;
}

-gprs_rlcmac_tbf *gprs_rlcmac_pdch::ul_tbf_by_tfi(uint8_t tfi)
+gprs_rlcmac_ul_tbf *gprs_rlcmac_pdch::ul_tbf_by_tfi(uint8_t tfi)
{
- return tbf_from_list_by_tfi(&bts_data()->ul_tbfs, tfi, GPRS_RLCMAC_UL_TBF);
+ return (gprs_rlcmac_ul_tbf *)tbf_from_list_by_tfi(&bts_data()->ul_tbfs, tfi, GPRS_RLCMAC_UL_TBF);
}

-gprs_rlcmac_tbf *gprs_rlcmac_pdch::dl_tbf_by_tfi(uint8_t tfi)
+gprs_rlcmac_dl_tbf *gprs_rlcmac_pdch::dl_tbf_by_tfi(uint8_t tfi)
{
- return tbf_from_list_by_tfi(&bts_data()->dl_tbfs, tfi, GPRS_RLCMAC_DL_TBF);
+ return (gprs_rlcmac_dl_tbf *)tbf_from_list_by_tfi(&bts_data()->dl_tbfs, tfi, GPRS_RLCMAC_DL_TBF);
}
diff --git a/src/bts.h b/src/bts.h
index a0b808e..8f99942 100644
--- a/src/bts.h
+++ b/src/bts.h
@@ -62,8 +62,8 @@ struct gprs_rlcmac_pdch {
BTS *bts() const;
uint8_t trx_no() const;

- struct gprs_rlcmac_tbf *ul_tbf_by_tfi(uint8_t tfi);
- struct gprs_rlcmac_tbf *dl_tbf_by_tfi(uint8_t tfi);
+ struct gprs_rlcmac_ul_tbf *ul_tbf_by_tfi(uint8_t tfi);
+ struct gprs_rlcmac_dl_tbf *dl_tbf_by_tfi(uint8_t tfi);
#endif

uint8_t m_is_enabled; /* TS is enabled */
@@ -193,12 +193,12 @@ public:
/** add paging to paging queue(s) */
int add_paging(uint8_t chan_needed, uint8_t *identity_lv);

- gprs_rlcmac_tbf *dl_tbf_by_tlli(uint32_t tlli);
- gprs_rlcmac_tbf *ul_tbf_by_tlli(uint32_t tlli);
- gprs_rlcmac_tbf *dl_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts);
- gprs_rlcmac_tbf *ul_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts);
- gprs_rlcmac_tbf *dl_tbf_by_tfi(uint8_t tfi, uint8_t trx);
- gprs_rlcmac_tbf *ul_tbf_by_tfi(uint8_t tfi, uint8_t trx);
+ gprs_rlcmac_dl_tbf *dl_tbf_by_tlli(uint32_t tlli);
+ gprs_rlcmac_ul_tbf *ul_tbf_by_tlli(uint32_t tlli);
+ gprs_rlcmac_dl_tbf *dl_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts);
+ gprs_rlcmac_ul_tbf *ul_tbf_by_poll_fn(uint32_t fn, uint8_t trx, uint8_t ts);
+ gprs_rlcmac_dl_tbf *dl_tbf_by_tfi(uint8_t tfi, uint8_t trx);
+ gprs_rlcmac_ul_tbf *ul_tbf_by_tfi(uint8_t tfi, uint8_t trx);

int tfi_find_free(enum gprs_rlcmac_tbf_direction dir, uint8_t *_trx, int8_t use_trx);

[Daniel Willmann]

UL and DL tbfs are used in very separate parts and are not the same
thing so split the alloc function and use the UL/DL version throughout
the code.

Ticket: SYS#389
Sponsored-by: On-Waves ehf
---
src/bts.cpp | 2 +-
src/tbf.cpp | 102 +++++++++++++++++++++++++++++++++-------------
src/tbf.h | 9 +++-
tests/alloc/AllocTest.cpp | 31 +++++++++-----
tests/tbf/TbfTest.cpp | 8 ++--
5 files changed, 106 insertions(+), 46 deletions(-)

diff --git a/src/bts.cpp b/src/bts.cpp
index 8489431..5bf139d 100644
--- a/src/bts.cpp
+++ b/src/bts.cpp
@@ -461,7 +461,7 @@ int BTS::rcv_rach(uint8_t ra, uint32_t Fn, int16_t qta)
return -EBUSY;
}
/* set class to 0, since we don't know the multislot class yet */
- tbf = (gprs_rlcmac_ul_tbf *)tbf_alloc(&m_bts, NULL, GPRS_RLCMAC_UL_TBF, tfi, trx_no, 0, 1);
+ tbf = tbf_alloc_ul_tbf(&m_bts, NULL, tfi, trx_no, 0, 1);
if (!tbf) {
LOGP(DRLCMAC, LOGL_NOTICE, "No PDCH resource\n");
/* FIXME: send reject */
diff --git a/src/tbf.cpp b/src/tbf.cpp
index 15301a1..1201642 100644
--- a/src/tbf.cpp
+++ b/src/tbf.cpp
@@ -165,7 +165,7 @@ static int tbf_new_dl_assignment(struct gprs_rlcmac_bts *bts,
return -EBUSY;
}
/* set number of downlink slots according to multislot class */
- tbf = tbf_alloc(bts, tbf, GPRS_RLCMAC_DL_TBF, tfi, trx, ms_class, ss);
+ tbf = tbf_alloc_dl_tbf(bts, tbf, tfi, trx, ms_class, ss);
if (!tbf) {
LOGP(DRLCMAC, LOGL_NOTICE, "No PDCH resource\n");
/* FIXME: send reject */
@@ -231,7 +231,7 @@ gprs_rlcmac_ul_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts,
return NULL;
}
/* use multislot class of downlink TBF */
- tbf = (gprs_rlcmac_ul_tbf *)tbf_alloc(bts, dl_tbf, GPRS_RLCMAC_UL_TBF, tfi, trx, ms_class, 0);
+ tbf = tbf_alloc_ul_tbf(bts, dl_tbf, tfi, trx, ms_class, 0);
if (!tbf) {
LOGP(DRLCMAC, LOGL_NOTICE, "No PDCH resource\n");
/* FIXME: send reject */
@@ -479,33 +479,20 @@ void gprs_rlcmac_tbf::poll_timeout()
LOGP(DRLCMAC, LOGL_ERROR, "- Poll Timeout, but no event!\n");
}

-struct gprs_rlcmac_tbf *tbf_alloc(struct gprs_rlcmac_bts *bts,
- struct gprs_rlcmac_tbf *old_tbf, enum gprs_rlcmac_tbf_direction dir,
- uint8_t tfi, uint8_t trx,
+static int setup_tbf(struct gprs_rlcmac_tbf *tbf, struct gprs_rlcmac_bts *bts,
+ struct gprs_rlcmac_tbf *old_tbf, uint8_t tfi, uint8_t trx,
uint8_t ms_class, uint8_t single_slot)
{
- struct gprs_rlcmac_tbf *tbf;
int rc;

- LOGP(DRLCMAC, LOGL_DEBUG, "********** TBF starts here **********\n");
- LOGP(DRLCMAC, LOGL_INFO, "Allocating %s TBF: TFI=%d TRX=%d "
- "MS_CLASS=%d\n", (dir == GPRS_RLCMAC_UL_TBF) ? "UL" : "DL",
- tfi, trx, ms_class);
-
if (trx >= 8 || tfi >= 32)
- return NULL;
-
- if (dir == GPRS_RLCMAC_UL_TBF)
- tbf = talloc_zero(tall_pcu_ctx, struct gprs_rlcmac_ul_tbf);
- else
- tbf = talloc_zero(tall_pcu_ctx, struct gprs_rlcmac_dl_tbf);
+ return -1;

if (!tbf)
- return NULL;
+ return -1;

tbf->m_created_ts = time(NULL);
tbf->bts = bts->bts;
- tbf->direction = dir;
tbf->m_tfi = tfi;
tbf->trx = &bts->trx[trx];
tbf->ms_class = ms_class;
@@ -514,16 +501,14 @@ struct gprs_rlcmac_tbf *tbf_alloc(struct gprs_rlcmac_bts *bts,
single_slot);
/* if no resource */
if (rc < 0) {
- talloc_free(tbf);
- return NULL;
+ return -1;
}
/* assign control ts */
tbf->control_ts = 0xff;
rc = tbf_assign_control_ts(tbf);
/* if no resource */
if (rc < 0) {
- talloc_free(tbf);
- return NULL;
+ return -1;
}

/* set timestamp */
@@ -532,14 +517,73 @@ struct gprs_rlcmac_tbf *tbf_alloc(struct gprs_rlcmac_bts *bts,
gettimeofday(&tbf->meas.dl_loss_tv, NULL);

tbf->m_llc.init();
- if (dir == GPRS_RLCMAC_UL_TBF) {
- llist_add(&tbf->list, &bts->ul_tbfs);
- tbf->bts->tbf_ul_created();
- } else {
- llist_add(&tbf->list, &bts->dl_tbfs);
- tbf->bts->tbf_dl_created();
+ return 0;
+}
+
+
+struct gprs_rlcmac_ul_tbf *tbf_alloc_ul_tbf(struct gprs_rlcmac_bts *bts,
+ struct gprs_rlcmac_tbf *old_tbf, uint8_t tfi, uint8_t trx,
+ uint8_t ms_class, uint8_t single_slot)
+{
+ struct gprs_rlcmac_ul_tbf *tbf;
+ int rc;
+
+ LOGP(DRLCMAC, LOGL_DEBUG, "********** TBF starts here **********\n");
+ LOGP(DRLCMAC, LOGL_INFO, "Allocating %s TBF: TFI=%d TRX=%d "
+ "MS_CLASS=%d\n", "UL", tfi, trx, ms_class);
+
+ if (trx >= 8 || tfi >= 32)
+ return NULL;
+
+ tbf = talloc_zero(tall_pcu_ctx, struct gprs_rlcmac_ul_tbf);
+
+ if (!tbf)
+ return NULL;
+
+ tbf->direction = GPRS_RLCMAC_UL_TBF;
+ rc = setup_tbf(tbf, bts, old_tbf, tfi, trx, ms_class, single_slot);
+ /* if no resource */
+ if (rc < 0) {
+ talloc_free(tbf);
+ return NULL;
+ }
+
+ llist_add(&tbf->list, &bts->ul_tbfs);
+ tbf->bts->tbf_ul_created();
+
+ return tbf;
+}
+
+struct gprs_rlcmac_dl_tbf *tbf_alloc_dl_tbf(struct gprs_rlcmac_bts *bts,
+ struct gprs_rlcmac_tbf *old_tbf, uint8_t tfi, uint8_t trx,
+ uint8_t ms_class, uint8_t single_slot)
+{
+ struct gprs_rlcmac_dl_tbf *tbf;
+ int rc;
+
+ LOGP(DRLCMAC, LOGL_DEBUG, "********** TBF starts here **********\n");
+ LOGP(DRLCMAC, LOGL_INFO, "Allocating %s TBF: TFI=%d TRX=%d "
+ "MS_CLASS=%d\n", "DL", tfi, trx, ms_class);
+
+ if (trx >= 8 || tfi >= 32)
+ return NULL;
+
+ tbf = talloc_zero(tall_pcu_ctx, struct gprs_rlcmac_dl_tbf);
+
+ if (!tbf)
+ return NULL;
+
+ tbf->direction = GPRS_RLCMAC_DL_TBF;
+ rc = setup_tbf(tbf, bts, old_tbf, tfi, trx, ms_class, single_slot);
+ /* if no resource */
+ if (rc < 0) {
+ talloc_free(tbf);
+ return NULL;
}

+ llist_add(&tbf->list, &bts->dl_tbfs);
+ tbf->bts->tbf_dl_created();
+
return tbf;
}

diff --git a/src/tbf.h b/src/tbf.h
index d650021..4d20987 100644
--- a/src/tbf.h
+++ b/src/tbf.h
@@ -263,9 +263,14 @@ struct gprs_rlcmac_ul_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts,
int8_t use_trx, uint8_t ms_class,
uint32_t tlli, uint8_t ta, struct gprs_rlcmac_tbf *dl_tbf);

-struct gprs_rlcmac_tbf *tbf_alloc(struct gprs_rlcmac_bts *bts,
+struct gprs_rlcmac_ul_tbf *tbf_alloc_ul_tbf(struct gprs_rlcmac_bts *bts,
struct gprs_rlcmac_tbf *old_tbf,
- enum gprs_rlcmac_tbf_direction dir, uint8_t tfi, uint8_t trx,
+ uint8_t tfi, uint8_t trx,
+ uint8_t ms_class, uint8_t single_slot);
+
+struct gprs_rlcmac_dl_tbf *tbf_alloc_dl_tbf(struct gprs_rlcmac_bts *bts,
+ struct gprs_rlcmac_tbf *old_tbf,
+ uint8_t tfi, uint8_t trx,
uint8_t ms_class, uint8_t single_slot);

void tbf_free(struct gprs_rlcmac_tbf *tbf);
diff --git a/tests/alloc/AllocTest.cpp b/tests/alloc/AllocTest.cpp
index 80e8c34..830ca90 100644
--- a/tests/alloc/AllocTest.cpp
+++ b/tests/alloc/AllocTest.cpp
@@ -36,6 +36,17 @@ extern "C" {
void *tall_pcu_ctx;
int16_t spoof_mnc = 0, spoof_mcc = 0;

+static gprs_rlcmac_tbf *tbf_alloc(struct gprs_rlcmac_bts *bts,
+ struct gprs_rlcmac_tbf *old_tbf, gprs_rlcmac_tbf_direction dir,
+ uint8_t tfi, uint8_t trx,
+ uint8_t ms_class, uint8_t single_slot)
+{
+ if (dir == GPRS_RLCMAC_UL_TBF)
+ return tbf_alloc_ul_tbf(bts, old_tbf, tfi, trx, ms_class, single_slot);
+ else
+ return tbf_alloc_dl_tbf(bts, old_tbf, tfi, trx, ms_class, single_slot);
+}
+
static void test_alloc_a(gprs_rlcmac_tbf_direction dir, const int count)
{
int tfi;
@@ -136,14 +147,14 @@ static void test_alloc_b(int ms_class)

tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1);
OSMO_ASSERT(tfi >= 0);
- ul_tbf = tbf_alloc(bts, NULL, GPRS_RLCMAC_UL_TBF, tfi, trx_no, ms_class, 1);
+ ul_tbf = tbf_alloc_ul_tbf(bts, NULL, tfi, trx_no, ms_class, 1);
OSMO_ASSERT(ul_tbf);
dump_assignment(ul_tbf, "UL");

/* assume final ack has not been sent */
tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1);
OSMO_ASSERT(tfi >= 0);
- dl_tbf = tbf_alloc(bts, ul_tbf, GPRS_RLCMAC_DL_TBF, tfi, trx_no, ms_class, 0);
+ dl_tbf = tbf_alloc_dl_tbf(bts, ul_tbf, tfi, trx_no, ms_class, 0);
OSMO_ASSERT(dl_tbf);
dump_assignment(dl_tbf, "DL");

@@ -177,7 +188,7 @@ static void test_alloc_b(int ms_class)

tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1);
OSMO_ASSERT(tfi >= 0);
- dl_tbf = tbf_alloc(bts, NULL, GPRS_RLCMAC_DL_TBF, tfi, trx_no, ms_class, 1);
+ dl_tbf = tbf_alloc_dl_tbf(bts, NULL, tfi, trx_no, ms_class, 1);
dl_tbf->m_tlli = 0x23;
dl_tbf->m_tlli_valid = true;
OSMO_ASSERT(dl_tbf);
@@ -185,7 +196,7 @@ static void test_alloc_b(int ms_class)

tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1);
OSMO_ASSERT(tfi >= 0);
- ul_tbf = tbf_alloc(bts, dl_tbf, GPRS_RLCMAC_UL_TBF, tfi, trx_no, ms_class, 0);
+ ul_tbf = tbf_alloc_ul_tbf(bts, dl_tbf, tfi, trx_no, ms_class, 0);
ul_tbf->m_tlli = 0x23;
ul_tbf->m_tlli_valid = true;
ul_tbf->dir.ul.contention_resolution_done = 1;
@@ -226,14 +237,14 @@ static void test_alloc_b(int ms_class)

tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1);
OSMO_ASSERT(tfi >= 0);
- ul_tbf = tbf_alloc(bts, NULL, GPRS_RLCMAC_UL_TBF, tfi, trx_no, ms_class, 0);
+ ul_tbf = tbf_alloc_ul_tbf(bts, NULL, tfi, trx_no, ms_class, 0);
OSMO_ASSERT(ul_tbf);
dump_assignment(ul_tbf, "UL");

/* assume final ack has not been sent */
tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1);
OSMO_ASSERT(tfi >= 0);
- dl_tbf = tbf_alloc(bts, ul_tbf, GPRS_RLCMAC_DL_TBF, tfi, trx_no, ms_class, 0);
+ dl_tbf = tbf_alloc_dl_tbf(bts, ul_tbf, tfi, trx_no, ms_class, 0);
OSMO_ASSERT(dl_tbf);
dump_assignment(dl_tbf, "DL");

@@ -290,13 +301,13 @@ static void test_alloc_b(bool ts0, bool ts1, bool ts2, bool ts3, bool ts4, bool
tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1);

OSMO_ASSERT(tfi >= 0);
- ul_tbf = tbf_alloc(bts, NULL, GPRS_RLCMAC_UL_TBF, tfi, trx_no, ms_class, 1);
+ ul_tbf = tbf_alloc_ul_tbf(bts, NULL, tfi, trx_no, ms_class, 1);
OSMO_ASSERT(ul_tbf);

/* assume final ack has not been sent */
tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1);
OSMO_ASSERT(tfi >= 0);
- dl_tbf = tbf_alloc(bts, ul_tbf, GPRS_RLCMAC_DL_TBF, tfi, trx_no, ms_class, 0);
+ dl_tbf = tbf_alloc_dl_tbf(bts, ul_tbf, tfi, trx_no, ms_class, 0);
OSMO_ASSERT(dl_tbf);

/* verify that both are on the same ts */
@@ -333,14 +344,14 @@ static void test_alloc_b(bool ts0, bool ts1, bool ts2, bool ts3, bool ts4, bool

tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1);
OSMO_ASSERT(tfi >= 0);
- dl_tbf = tbf_alloc(bts, NULL, GPRS_RLCMAC_DL_TBF, tfi, trx_no, ms_class, 1);
+ dl_tbf = tbf_alloc_dl_tbf(bts, NULL, tfi, trx_no, ms_class, 1);
OSMO_ASSERT(dl_tbf);
dl_tbf->m_tlli = 0x23;
dl_tbf->m_tlli_valid = true;

tfi = the_bts.tfi_find_free(GPRS_RLCMAC_UL_TBF, &trx_no, -1);
OSMO_ASSERT(tfi >= 0);
- ul_tbf = tbf_alloc(bts, dl_tbf, GPRS_RLCMAC_UL_TBF, tfi, trx_no, ms_class, 0);
+ ul_tbf = tbf_alloc_ul_tbf(bts, dl_tbf, tfi, trx_no, ms_class, 0);
OSMO_ASSERT(ul_tbf);
ul_tbf->m_tlli = 0x23;
ul_tbf->m_tlli_valid = true;
diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp
index 38975f9..decf4d9 100644
--- a/tests/tbf/TbfTest.cpp
+++ b/tests/tbf/TbfTest.cpp
@@ -44,16 +44,16 @@ static void test_tbf_tlli_update()
/*
* Make a uplink and downlink allocation
*/
- gprs_rlcmac_tbf *dl_tbf = tbf_alloc(the_bts.bts_data(),
- NULL, GPRS_RLCMAC_DL_TBF, 0,
+ gprs_rlcmac_tbf *dl_tbf = tbf_alloc_dl_tbf(the_bts.bts_data(),
+ NULL, 0,
0, 0, 0);
dl_tbf->update_tlli(0x2342);
dl_tbf->tlli_mark_valid();
dl_tbf->ta = 4;
the_bts.timing_advance()->remember(0x2342, dl_tbf->ta);

- gprs_rlcmac_tbf *ul_tbf = tbf_alloc(the_bts.bts_data(),
- ul_tbf, GPRS_RLCMAC_UL_TBF, 0,
+ gprs_rlcmac_tbf *ul_tbf = tbf_alloc_ul_tbf(the_bts.bts_data(),
+ ul_tbf, 0,
0, 0, 0);
ul_tbf->update_tlli(0x2342);
ul_tbf->tlli_mark_valid();

[Daniel Willmann]

Many functions only ever deal with or return a UL or a DL TBF.
Explicitly change the type to reflect which TBF is used where.

Ticket: SYS#389
Sponsored-by: On-Waves ehf
---
src/bts.cpp | 38 +++++++++++++++++++-------------------
src/tbf.cpp | 6 +++---
src/tbf.h | 2 +-
3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/src/bts.cpp b/src/bts.cpp
index 68d72f2..8489431 100644
--- a/src/bts.cpp
+++ b/src/bts.cpp
@@ -417,7 +417,7 @@ int BTS::rcv_imm_ass_cnf(const uint8_t *data, uint32_t fn)

int BTS::rcv_rach(uint8_t ra, uint32_t Fn, int16_t qta)
{
- struct gprs_rlcmac_tbf *tbf;
+ struct gprs_rlcmac_ul_tbf *tbf;
uint8_t trx_no, ts_no = 0;
int8_t tfi; /* must be signed */
uint8_t sb = 0;
@@ -461,7 +461,7 @@ int BTS::rcv_rach(uint8_t ra, uint32_t Fn, int16_t qta)
return -EBUSY;
}
/* set class to 0, since we don't know the multislot class yet */
- tbf = tbf_alloc(&m_bts, NULL, GPRS_RLCMAC_UL_TBF, tfi, trx_no, 0, 1);
+ tbf = (gprs_rlcmac_ul_tbf *)tbf_alloc(&m_bts, NULL, GPRS_RLCMAC_UL_TBF, tfi, trx_no, 0, 1);
if (!tbf) {
LOGP(DRLCMAC, LOGL_NOTICE, "No PDCH resource\n");
/* FIXME: send reject */
@@ -692,7 +692,7 @@ void gprs_rlcmac_pdch::add_paging(struct gprs_rlcmac_paging *pag)
*/
int gprs_rlcmac_pdch::rcv_data_block_acknowledged(uint8_t *data, uint8_t len, int8_t rssi)
{
- struct gprs_rlcmac_tbf *tbf;
+ struct gprs_rlcmac_ul_tbf *tbf;
struct rlc_ul_header *rh = (struct rlc_ul_header *)data;

switch (len) {
@@ -867,24 +867,24 @@ void gprs_rlcmac_pdch::rcv_control_dl_ack_nack(Packet_Downlink_Ack_Nack_t *ack_n

void gprs_rlcmac_pdch::rcv_resource_request(Packet_Resource_Request_t *request, uint32_t fn)
{
- struct gprs_rlcmac_tbf *tbf;
+ struct gprs_rlcmac_ul_tbf *ul_tbf;
+ struct gprs_rlcmac_dl_tbf *dl_tbf;
struct gprs_rlcmac_sba *sba;
int rc;

if (request->ID.UnionType) {
uint32_t tlli = request->ID.u.TLLI;
uint8_t ms_class = 0;
- struct gprs_rlcmac_tbf *dl_tbf;
uint8_t ta;

- tbf = bts()->ul_tbf_by_tlli(tlli);
- if (tbf) {
+ ul_tbf = bts()->ul_tbf_by_tlli(tlli);
+ if (ul_tbf) {
LOGP(DRLCMACUL, LOGL_NOTICE, "Got RACH from "
"TLLI=0x%08x while %s still "
"exists. Killing pending DL TBF\n",
- tlli, tbf_name(tbf));
- tbf_free(tbf);
- tbf = NULL;
+ tlli, tbf_name(ul_tbf));
+ tbf_free(ul_tbf);
+ ul_tbf = NULL;
}

if ((dl_tbf = bts()->dl_tbf_by_tlli(tlli))) {
@@ -918,34 +918,34 @@ void gprs_rlcmac_pdch::rcv_resource_request(Packet_Resource_Request_t *request,
ms_class = Decoding::get_ms_class_by_capability(&request->MS_Radio_Access_capability);
if (!ms_class)
LOGP(DRLCMAC, LOGL_NOTICE, "MS does not give us a class.\n");
- tbf = tbf_alloc_ul(bts_data(), trx_no(), ms_class, tlli, ta, NULL);
- if (!tbf)
+ ul_tbf = tbf_alloc_ul(bts_data(), trx_no(), ms_class, tlli, ta, NULL);
+ if (!ul_tbf)
return;
/* set control ts to current MS's TS, until assignment complete */
LOGP(DRLCMAC, LOGL_DEBUG, "Change control TS to %d until assinment is complete.\n", ts_no);
- tbf->control_ts = ts_no;
+ ul_tbf->control_ts = ts_no;
/* schedule uplink assignment */
- tbf->ul_ass_state = GPRS_RLCMAC_UL_ASS_SEND_ASS;
+ ul_tbf->ul_ass_state = GPRS_RLCMAC_UL_ASS_SEND_ASS;
return;
}

if (request->ID.u.Global_TFI.UnionType) {
int8_t tfi = request->ID.u.Global_TFI.u.DOWNLINK_TFI;
- tbf = bts()->dl_tbf_by_tfi(tfi, trx_no());
- if (!tbf) {
+ dl_tbf = bts()->dl_tbf_by_tfi(tfi, trx_no());
+ if (!dl_tbf) {
LOGP(DRLCMAC, LOGL_NOTICE, "PACKET RESSOURCE REQ unknown downlink TFI=%d\n", tfi);
return;
}
} else {
int8_t tfi = request->ID.u.Global_TFI.u.UPLINK_TFI;
- tbf = bts()->ul_tbf_by_tfi(tfi, trx_no());
- if (!tbf) {
+ ul_tbf = bts()->ul_tbf_by_tfi(tfi, trx_no());
+ if (!ul_tbf) {
LOGP(DRLCMAC, LOGL_NOTICE, "PACKET RESSOURCE REQ unknown uplink TFI=%d\n", tfi);
return;
}
}

- LOGP(DRLCMAC, LOGL_ERROR, "RX: [PCU <- BTS] %s FIXME: Packet resource request\n", tbf_name(tbf));
+ LOGP(DRLCMAC, LOGL_ERROR, "RX: [PCU <- BTS] %s FIXME: Packet resource request\n", tbf_name(ul_tbf));
}

void gprs_rlcmac_pdch::rcv_measurement_report(Packet_Measurement_Report_t *report, uint32_t fn)
diff --git a/src/tbf.cpp b/src/tbf.cpp
index 39549f1..15301a1 100644
--- a/src/tbf.cpp
+++ b/src/tbf.cpp
@@ -214,12 +214,12 @@ int gprs_rlcmac_tbf::handle(struct gprs_rlcmac_bts *bts,
return tbf_new_dl_assignment(bts, imsi, tlli, ms_class, data, len);
}

-struct gprs_rlcmac_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts,
+gprs_rlcmac_ul_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts,
int8_t use_trx, uint8_t ms_class,
uint32_t tlli, uint8_t ta, struct gprs_rlcmac_tbf *dl_tbf)
{
uint8_t trx;
- struct gprs_rlcmac_tbf *tbf;
+ struct gprs_rlcmac_ul_tbf *tbf;
int8_t tfi; /* must be signed */

#warning "Copy and paste with tbf_new_dl_assignment"
@@ -231,7 +231,7 @@ struct gprs_rlcmac_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts,
return NULL;
}
/* use multislot class of downlink TBF */
- tbf = tbf_alloc(bts, dl_tbf, GPRS_RLCMAC_UL_TBF, tfi, trx, ms_class, 0);
+ tbf = (gprs_rlcmac_ul_tbf *)tbf_alloc(bts, dl_tbf, GPRS_RLCMAC_UL_TBF, tfi, trx, ms_class, 0);
if (!tbf) {
LOGP(DRLCMAC, LOGL_NOTICE, "No PDCH resource\n");
/* FIXME: send reject */
diff --git a/src/tbf.h b/src/tbf.h
index 8464e19..d650021 100644
--- a/src/tbf.h
+++ b/src/tbf.h
@@ -259,7 +259,7 @@ protected:
};


-struct gprs_rlcmac_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts,
+struct gprs_rlcmac_ul_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts,
int8_t use_trx, uint8_t ms_class,
uint32_t tlli, uint8_t ta, struct gprs_rlcmac_tbf *dl_tbf);

[Holger Hans Peter Freyther]

Post by Daniel Willmann
void gprs_rlcmac_pdch::rcv_resource_request(Packet_Resource_Request_t *request, uint32_t fn)
{
- struct gprs_rlcmac_tbf *tbf;
+ struct gprs_rlcmac_ul_tbf *ul_tbf;
+ struct gprs_rlcmac_dl_tbf *dl_tbf;

Limit the dl_tbf (and ul_tbf) in scope. The dl_tbf one part of the code
is dealing with is not the one the other is using. We should not accidently
leak a tbf into another scope just because this method is huge.

In general cppcheck's style warnings complain about this kind of thing

[Holger Hans Peter Freyther]

Post by Daniel Willmann
- LOGP(DRLCMAC, LOGL_ERROR, "RX: [PCU <- BTS] %s FIXME: Packet resource request\n", tbf_name(tbf));
+ LOGP(DRLCMAC, LOGL_ERROR, "RX: [PCU <- BTS] %s FIXME: Packet resource request\n", tbf_name(ul_tbf));

And the compiler just re-inforced me with a warning:

home/ich/install/openbsc/include/osmocom/core/logging.h: In member function 'void gprs_rlcmac_pdch::rcv_resource_request(Packet_Resource_Request_t*, uint32_t)':
/home/ich/install/openbsc/include/osmocom/core/logging.h:42:53: warning: 'ul_tbf' may be used uninitialized in this function [-Wmaybe-uninitialized]
logp2(ss, level, __FILE__, __LINE__, 0, fmt, ##args)


After the last if/else either dl_tbf or ul_tbf will be assigned. Maybe just
move the warning into the if/else and remove the declaration of the dl_tbf
ul_tbf.

[Daniel Willmann]

Ticket: SYS#389
Sponsored-by: On-Waves ehf
---
src/tbf.cpp | 6 +++++-
src/tbf.h | 6 ++++++
2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/tbf.cpp b/src/tbf.cpp
index f913f97..39549f1 100644
--- a/src/tbf.cpp
+++ b/src/tbf.cpp
@@ -495,7 +495,11 @@ struct gprs_rlcmac_tbf *tbf_alloc(struct gprs_rlcmac_bts *bts,
if (trx >= 8 || tfi >= 32)
return NULL;

- tbf = talloc_zero(tall_pcu_ctx, struct gprs_rlcmac_tbf);
+ if (dir == GPRS_RLCMAC_UL_TBF)
+ tbf = talloc_zero(tall_pcu_ctx, struct gprs_rlcmac_ul_tbf);
+ else
+ tbf = talloc_zero(tall_pcu_ctx, struct gprs_rlcmac_dl_tbf);
+
if (!tbf)
return NULL;

diff --git a/src/tbf.h b/src/tbf.h
index 80e2068..8464e19 100644
--- a/src/tbf.h
+++ b/src/tbf.h
@@ -326,6 +326,12 @@ inline time_t gprs_rlcmac_tbf::created_ts() const
return m_created_ts;
}

+struct gprs_rlcmac_dl_tbf : public gprs_rlcmac_tbf {
+};
+
+struct gprs_rlcmac_ul_tbf : public gprs_rlcmac_tbf {
+};
+
#endif

#ifdef __cplusplus

[Holger Hans Peter Freyther]

On Wed, Jul 16, 2014 at 07:04:28PM +0200, Daniel Willmann wrote:

Hi!

as we have to move forward I will merge it. One comment in regard to
the approach taken here.

+gprs_rlcmac_tbf *gprs_rlcmac_pdch::tbf_from_list_by_tfi(struct llist_head *tbf_list, uint8_t tfi,
+ enum gprs_rlcmac_tbf_direction dir)
{
gprs_rlcmac_tbf *tbf;
llist_for_each_entry(tbf, tbf_list, list) {
+ OSMO_ASSERT(tbf->direction == dir);
+ return tbf_from_list_by_tfi(&bts_data()->ul_tbfs, tfi, GPRS_RLCMAC_UL_TBF);
+ return tbf_from_list_by_tfi(&bts_data()->dl_tbfs, tfi, GPRS_RLCMAC_DL_TBF);

assert it on _entry_ into the ul_tbfs and dl_tbfs. If we only have one
place where the list is manipulated we can easily pay the price on entry.

[Daniel Willmann]

Hi,

Post by Holger Hans Peter Freyther
as we have to move forward I will merge it. One comment in regard to
the approach taken here.

+gprs_rlcmac_tbf *gprs_rlcmac_pdch::tbf_from_list_by_tfi(struct llist_head *tbf_list, uint8_t tfi,
+ enum gprs_rlcmac_tbf_direction dir)
{
gprs_rlcmac_tbf *tbf;
llist_for_each_entry(tbf, tbf_list, list) {
+ OSMO_ASSERT(tbf->direction == dir);
+ return tbf_from_list_by_tfi(&bts_data()->ul_tbfs, tfi, GPRS_RLCMAC_UL_TBF);
+ return tbf_from_list_by_tfi(&bts_data()->dl_tbfs, tfi, GPRS_RLCMAC_DL_TBF);

assert it on _entry_ into the ul_tbfs and dl_tbfs. If we only have one
place where the list is manipulated we can easily pay the price on entry.

This is obsolete now anyway as the functions that do llist_add
explicitly allocate a DL/UL TBF.

I have a patch that will remove the ASSERTs completely

Regards,
Daniel Willmann