Review at  https://gerrit.osmocom.org/6808

trxcon/scheduler: move prim management outside lchan handlers

Previously, each lchan handler used to obtain and delete primitives
from a timeslot's tranmit queue itself. This approach entails many
potential problems and bugs:

  - The lchan handlers shall not do that by definition, they
    should encode and decode frames according to GSM 05.03.

  - In some cases (e.g. TCH), a single transmit queue may contain
    primitives of different types (e.g. TCH, FACCH and SACCH). At
    the same time, the lchan handlers don't care and don't even
    know about each other. So, this could cause an unexpected
    behaviour in some cases.

This change separates all primitive management routines,
providing a new API for obtaining and dropping them.

"Write programs that do one thing and do it well."

Change-Id: I29503ece51903784bc53541015285234471c8d15
---
M src/host/trxcon/sched_lchan_rach.c
M src/host/trxcon/sched_lchan_sch.c
M src/host/trxcon/sched_lchan_tchf.c
M src/host/trxcon/sched_lchan_xcch.c
M src/host/trxcon/sched_prim.c
M src/host/trxcon/sched_trx.c
M src/host/trxcon/sched_trx.h
7 files changed, 118 insertions(+), 79 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmocom-bb refs/changes/08/6808/1

diff --git a/src/host/trxcon/sched_lchan_rach.c 
b/src/host/trxcon/sched_lchan_rach.c
index bf81fd2..b26e279 100644
--- a/src/host/trxcon/sched_lchan_rach.c
+++ b/src/host/trxcon/sched_lchan_rach.c
@@ -24,11 +24,9 @@
 
 #include <errno.h>
 #include <string.h>
-#include <talloc.h>
 #include <stdint.h>
 #include <stdbool.h>
 
-#include <osmocom/core/linuxlist.h>
 #include <osmocom/core/logging.h>
 #include <osmocom/core/bits.h>
 
@@ -57,15 +55,13 @@
 int tx_rach_fn(struct trx_instance *trx, struct trx_ts *ts,
        struct trx_lchan_state *lchan, uint32_t fn, uint8_t bid)
 {
-       struct trx_ts_prim *prim;
        struct l1ctl_rach_req *req;
        uint8_t burst[GSM_BURST_LEN];
        uint8_t payload[36];
        int rc;
 
-       /* Get a message from TX queue */
-       prim = llist_entry(ts->tx_prims.next, struct trx_ts_prim, list);
-       req = (struct l1ctl_rach_req *) prim->payload;
+       /* Get the payload from a current primitive */
+       req = (struct l1ctl_rach_req *) lchan->prim->payload;
 
        /* Delay RACH sending according to offset value */
        if (req->offset-- > 0)
@@ -75,6 +71,10 @@
        rc = gsm0503_rach_ext_encode(payload, req->ra, trx->bsic, false);
        if (rc) {
                LOGP(DSCHD, LOGL_ERROR, "Could not encode RACH burst\n");
+
+               /* Forget this primitive */
+               sched_prim_drop(lchan);
+
                return rc;
        }
 
@@ -90,15 +90,18 @@
        rc = trx_if_tx_burst(trx, ts->index, fn, trx->tx_power, burst);
        if (rc) {
                LOGP(DSCHD, LOGL_ERROR, "Could not send burst to 
transceiver\n");
+
+               /* Forget this primitive */
+               sched_prim_drop(lchan);
+
                return rc;
        }
 
        /* Confirm RACH request */
        l1ctl_tx_rach_conf(trx->l1l, fn);
 
-       /* Remove primitive from queue and free memory */
-       llist_del(&prim->list);
-       talloc_free(prim);
+       /* Forget processed primitive */
+       sched_prim_drop(lchan);
 
        return 0;
 }
diff --git a/src/host/trxcon/sched_lchan_sch.c 
b/src/host/trxcon/sched_lchan_sch.c
index e66cad4..b2377fa 100644
--- a/src/host/trxcon/sched_lchan_sch.c
+++ b/src/host/trxcon/sched_lchan_sch.c
@@ -29,7 +29,6 @@
 
 #include <arpa/inet.h>
 
-#include <osmocom/core/linuxlist.h>
 #include <osmocom/core/logging.h>
 #include <osmocom/core/bits.h>
 
diff --git a/src/host/trxcon/sched_lchan_tchf.c 
b/src/host/trxcon/sched_lchan_tchf.c
index da24b23..e562a49 100644
--- a/src/host/trxcon/sched_lchan_tchf.c
+++ b/src/host/trxcon/sched_lchan_tchf.c
@@ -24,10 +24,8 @@
 
 #include <errno.h>
 #include <string.h>
-#include <talloc.h>
 #include <stdint.h>
 
-#include <osmocom/core/linuxlist.h>
 #include <osmocom/core/logging.h>
 #include <osmocom/core/bits.h>
 
@@ -171,11 +169,10 @@
        struct trx_lchan_state *lchan, uint32_t fn, uint8_t bid)
 {
        const struct trx_lchan_desc *lchan_desc;
-       struct trx_ts_prim *prim;
        ubit_t burst[GSM_BURST_LEN];
        ubit_t *buffer, *offset;
-       uint8_t *mask, *l2;
        const uint8_t *tsc;
+       uint8_t *mask;
        size_t l2_len;
        int rc;
 
@@ -206,35 +203,37 @@
                 * TODO: AMR requires a dedicated loop,
                 * which will be implemented later...
                 */
-               /* TODO: drop prim here */
-               LOGP(DSCHD, LOGL_ERROR, "AMR isn't supported yet\n");
+               LOGP(DSCHD, LOGL_ERROR, "AMR isn't supported yet, "
+                       "dropping frame...\n");
+
+               /* Forget this primitive */
+               sched_prim_drop(lchan);
+
                return -ENOTSUP;
        default:
-               /* TODO: drop prim here */
-               LOGP(DSCHD, LOGL_ERROR, "Invalid TCH mode: %u\n",
-                       lchan->tch_mode);
+               LOGP(DSCHD, LOGL_ERROR, "Invalid TCH mode: %u, "
+                       "dropping frame...\n", lchan->tch_mode);
+
+               /* Forget this primitive */
+               sched_prim_drop(lchan);
+
                return -EINVAL;
        }
 
-       /* Get a message from TX queue */
-       prim = sched_prim_dequeue_tch(&ts->tx_prims);
-       l2 = (uint8_t *) prim->payload;
-
        /* Determine payload length */
-       if (prim->payload_len == GSM_MACBLOCK_LEN)
+       if (lchan->prim->payload_len == GSM_MACBLOCK_LEN)
                l2_len = GSM_MACBLOCK_LEN;
 
        /* Shift buffer by 4 bursts back for interleaving */
        memcpy(buffer, buffer + 464, 464);
 
        /* Encode payload */
-       rc = gsm0503_tch_fr_encode(buffer, l2, l2_len, 1);
+       rc = gsm0503_tch_fr_encode(buffer, lchan->prim->payload, l2_len, 1);
        if (rc) {
                LOGP(DSCHD, LOGL_ERROR, "Failed to encode L2 payload\n");
 
-               /* Remove primitive from queue and free memory */
-               llist_del(&prim->list);
-               talloc_free(prim);
+               /* Forget this primitive */
+               sched_prim_drop(lchan);
 
                return -EINVAL;
        }
@@ -264,10 +263,8 @@
        if (rc) {
                LOGP(DSCHD, LOGL_ERROR, "Could not send burst to 
transceiver\n");
 
-               /* Remove primitive from queue and free memory */
-               prim = llist_entry(ts->tx_prims.next, struct trx_ts_prim, list);
-               llist_del(&prim->list);
-               talloc_free(prim);
+               /* Forget this primitive */
+               sched_prim_drop(lchan);
 
                /* Reset mask */
                *mask = 0x00;
@@ -277,15 +274,12 @@
 
        /* If we have sent the last (4/4) burst */
        if (*mask == 0x0f) {
-               /* Get pointer to a prim which was sent */
-               prim = llist_entry(ts->tx_prims.next, struct trx_ts_prim, list);
-
                /* Confirm data / traffic sending */
-               sched_send_data_conf(trx, ts, lchan, fn, prim->payload_len);
+               sched_send_data_conf(trx, ts, lchan, fn,
+                       lchan->prim->payload_len);
 
-               /* Remove primitive from queue and free memory */
-               llist_del(&prim->list);
-               talloc_free(prim);
+               /* Forget processed primitive */
+               sched_prim_drop(lchan);
 
                /* Reset mask */
                *mask = 0x00;
diff --git a/src/host/trxcon/sched_lchan_xcch.c 
b/src/host/trxcon/sched_lchan_xcch.c
index 3129097..d395b1d 100644
--- a/src/host/trxcon/sched_lchan_xcch.c
+++ b/src/host/trxcon/sched_lchan_xcch.c
@@ -24,10 +24,8 @@
 
 #include <errno.h>
 #include <string.h>
-#include <talloc.h>
 #include <stdint.h>
 
-#include <osmocom/core/linuxlist.h>
 #include <osmocom/core/logging.h>
 #include <osmocom/core/bits.h>
 
@@ -122,11 +120,10 @@
        struct trx_lchan_state *lchan, uint32_t fn, uint8_t bid)
 {
        const struct trx_lchan_desc *lchan_desc;
-       struct trx_ts_prim *prim;
        ubit_t burst[GSM_BURST_LEN];
        ubit_t *buffer, *offset;
-       uint8_t *mask, *l2;
        const uint8_t *tsc;
+       uint8_t *mask;
        int rc;
 
        /* Set up pointers */
@@ -142,20 +139,13 @@
                        return 0;
        }
 
-       /* Encode payload if not yet */
-
-       /* Get a message from TX queue */
-       prim = llist_entry(ts->tx_prims.next, struct trx_ts_prim, list);
-       l2 = (uint8_t *) prim->payload;
-
-       /* Encode bursts */
-       rc = gsm0503_xcch_encode(buffer, l2);
+       /* Encode payload */
+       rc = gsm0503_xcch_encode(buffer, lchan->prim->payload);
        if (rc) {
                LOGP(DSCHD, LOGL_ERROR, "Failed to encode L2 payload\n");
 
-               /* Remove primitive from queue and free memory */
-               llist_del(&prim->list);
-               talloc_free(prim);
+               /* Forget this primitive */
+               sched_prim_drop(lchan);
 
                return -EINVAL;
        }
@@ -185,10 +175,8 @@
        if (rc) {
                LOGP(DSCHD, LOGL_ERROR, "Could not send burst to 
transceiver\n");
 
-               /* Remove primitive from queue and free memory */
-               prim = llist_entry(ts->tx_prims.next, struct trx_ts_prim, list);
-               llist_del(&prim->list);
-               talloc_free(prim);
+               /* Forget this primitive */
+               sched_prim_drop(lchan);
 
                /* Reset mask */
                *mask = 0x00;
@@ -198,10 +186,8 @@
 
        /* If we have sent the last (4/4) burst */
        if ((*mask & 0x0f) == 0x0f) {
-               /* Remove primitive from queue and free memory */
-               prim = llist_entry(ts->tx_prims.next, struct trx_ts_prim, list);
-               llist_del(&prim->list);
-               talloc_free(prim);
+               /* Forget processed primitive */
+               sched_prim_drop(lchan);
 
                /* Reset mask */
                *mask = 0x00;
diff --git a/src/host/trxcon/sched_prim.c b/src/host/trxcon/sched_prim.c
index 1d1a940..3a46ab6 100644
--- a/src/host/trxcon/sched_prim.c
+++ b/src/host/trxcon/sched_prim.c
@@ -143,19 +143,20 @@
  * @param  queue a transmit queue to take a prim from
  * @return       a FACCH or TCH primitive
  */
-struct trx_ts_prim *sched_prim_dequeue_tch(struct llist_head *queue)
+static struct trx_ts_prim *sched_prim_dequeue_tch(struct llist_head *queue)
 {
        struct trx_ts_prim *a, *b;
 
-       /* Obtain the first prim from TX queue */
+       /* Dequeue the first prim */
        a = llist_entry(queue->next, struct trx_ts_prim, list);
+       llist_del(&a->list);
 
-       /* If this is the only one => do nothing... */
-       if (queue->next->next == queue)
+       /* If this was the only one => do nothing... */
+       if (llist_empty(queue))
                return a;
 
-       /* Obtain the second prim from TX queue */
-       b = llist_entry(queue->next->next, struct trx_ts_prim, list);
+       /* Obtain the next prim */
+       b = llist_entry(queue->next, struct trx_ts_prim, list);
 
        /* Find and prioritize FACCH  */
        if (PRIM_IS_FACCH(a) && PRIM_IS_TCH(b)) {
@@ -171,7 +172,7 @@
                 * Case 2: first is TCH, second is FACCH:
                 * Prioritize FACCH, dropping TCH
                 */
-               llist_del(&a->list);
+               llist_del(&b->list);
                talloc_free(a);
                return b;
        } else {
@@ -184,6 +185,49 @@
 }
 
 /**
+ * Dequeues a single primitive of required type
+ * from a specified transmit queue.
+ *
+ * @param  queue      a transmit queue to take a prim from
+ * @param  lchan_type required primitive type
+ * @return            a primitive or NULL if not found
+ */
+struct trx_ts_prim *sched_prim_dequeue(struct llist_head *queue,
+       enum trx_lchan_type lchan_type)
+{
+       struct trx_ts_prim *prim;
+
+       /* There is nothing to dequeue */
+       if (llist_empty(queue))
+               return NULL;
+
+       /* TCH requires FACCH prioritization, so handle it separately */
+       if (CHAN_IS_TCH(lchan_type))
+               return sched_prim_dequeue_tch(queue);
+
+       llist_for_each_entry(prim, queue, list) {
+               if (prim->chan == lchan_type) {
+                       llist_del(&prim->list);
+                       return prim;
+               }
+       }
+
+       return NULL;
+}
+
+/**
+ * Drops the current primitive of specified logical channel
+ *
+ * @param lchan a logical channel to drop prim from
+ */
+void sched_prim_drop(struct trx_lchan_state *lchan)
+{
+       /* Forget this primitive */
+       talloc_free(lchan->prim);
+       lchan->prim = NULL;
+}
+
+/**
  * Flushes a queue of primitives
  *
  * @param list list of prims going to be flushed
diff --git a/src/host/trxcon/sched_trx.c b/src/host/trxcon/sched_trx.c
index ec2f448..ee62e70 100644
--- a/src/host/trxcon/sched_trx.c
+++ b/src/host/trxcon/sched_trx.c
@@ -43,7 +43,6 @@
        const struct trx_frame *frame;
        struct trx_lchan_state *lchan;
        trx_lchan_tx_func *handler;
-       struct trx_ts_prim *prim;
        enum trx_lchan_type chan;
        uint8_t offset, bid;
        struct trx_ts *ts;
@@ -59,10 +58,6 @@
 
                /* Timeslot is not configured */
                if (ts->mf_layout == NULL)
-                       continue;
-
-               /* There is nothing to send */
-               if (llist_empty(&ts->tx_prims))
                        continue;
 
                /**
@@ -90,12 +85,19 @@
                if (lchan == NULL)
                        continue;
 
-               /* Get a message from TX queue */
-               prim = llist_entry(ts->tx_prims.next, struct trx_ts_prim, list);
+               /**
+                * If we aren't processing any primitive yet,
+                * attempt to obtain a new one from queue
+                */
+               if (lchan->prim == NULL)
+                       lchan->prim = sched_prim_dequeue(&ts->tx_prims, chan);
+
+               /* If there is no primitive, do nothing */
+               if (lchan->prim == NULL)
+                       continue;
 
                /* Poke lchan handler */
-               if (prim->chan == chan)
-                       handler(trx, ts, lchan, fn, bid);
+               handler(trx, ts, lchan, fn, bid);
        }
 }
 
@@ -394,6 +396,9 @@
        talloc_free(lchan->rx_bursts);
        talloc_free(lchan->tx_bursts);
 
+       /* Forget the current prim */
+       sched_prim_drop(lchan);
+
        lchan->active = 0;
 
        return 0;
@@ -414,6 +419,9 @@
                talloc_free(lchan->rx_bursts);
                talloc_free(lchan->tx_bursts);
 
+               /* Forget the current prim */
+               sched_prim_drop(lchan);
+
                lchan->active = 0;
        }
 }
diff --git a/src/host/trxcon/sched_trx.h b/src/host/trxcon/sched_trx.h
index f1ad7d3..c857601 100644
--- a/src/host/trxcon/sched_trx.h
+++ b/src/host/trxcon/sched_trx.h
@@ -160,6 +160,9 @@
        /*! \brief Burst buffer for TX */
        ubit_t *tx_bursts;
 
+       /*! \brief A primitive being sent */
+       struct trx_ts_prim *prim;
+
        /*! \brief Mode for TCH channels */
        uint8_t rsl_cmode, tch_mode;
 
@@ -269,7 +272,9 @@
 int sched_prim_push(struct trx_instance *trx,
        struct trx_ts_prim *prim, uint8_t chan_nr);
 
-struct trx_ts_prim *sched_prim_dequeue_tch(struct llist_head *queue);
+struct trx_ts_prim *sched_prim_dequeue(struct llist_head *queue,
+       enum trx_lchan_type lchan_type);
+void sched_prim_drop(struct trx_lchan_state *lchan);
 void sched_prim_flush_queue(struct llist_head *list);
 
 int sched_trx_handle_rx_burst(struct trx_instance *trx, uint8_t tn,

-- 
To view, visit https://gerrit.osmocom.org/6808
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I29503ece51903784bc53541015285234471c8d15
Gerrit-PatchSet: 1
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Owner: Harald Welte <lafo...@gnumonks.org>

Reply via email to