pespin has submitted this change. ( 
https://gerrit.osmocom.org/c/libosmo-gprs/+/35775?usp=email )

Change subject: Delay deleting UL TBF until last Pkt Ctrl Ack is fully 
transmitted
......................................................................

Delay deleting UL TBF until last Pkt Ctrl Ack is fully transmitted

This in turn delays reconfiguring the lower layers (L1CTL-CFG_UL_TBF.req
mask=0x0) until the last block has been transmited.

Change-Id: Ic38b4207623ccbda3b77d4b0a47431c25de95034
---
M include/osmocom/gprs/rlcmac/tbf_ul_fsm.h
M src/rlcmac/rlcmac_prim.c
M src/rlcmac/sched.c
M src/rlcmac/tbf_ul.c
M src/rlcmac/tbf_ul_fsm.c
M tests/rlcmac/rlcmac_prim_test.c
M tests/rlcmac/rlcmac_prim_test.err
7 files changed, 100 insertions(+), 18 deletions(-)

Approvals:
  fixeria: Looks good to me, but someone else must approve
  pespin: Looks good to me, approved
  Jenkins Builder: Verified




diff --git a/include/osmocom/gprs/rlcmac/tbf_ul_fsm.h 
b/include/osmocom/gprs/rlcmac/tbf_ul_fsm.h
index e3aeb3f..6809216 100644
--- a/include/osmocom/gprs/rlcmac/tbf_ul_fsm.h
+++ b/include/osmocom/gprs/rlcmac/tbf_ul_fsm.h
@@ -1,6 +1,7 @@
 /* Uplink TBF, 3GPP TS 44.060 */
 #pragma once

+#include <stdint.h>
 #include <osmocom/core/fsm.h>

 #include <osmocom/gprs/rlcmac/rlcmac_private.h>
@@ -25,10 +26,16 @@
        unsigned int pkt_acc_proc_attempts;
        /* 9.3.3.3.2: The block with CV=0 shall not be retransmitted more than 
four times. */
        unsigned int last_data_block_retrans_attempts;
-       /* Whether the Received Packet UL ACK/NACK w/ FinalAck=1 had 'TBF Est' 
field to '1'.
-        * Used during ST_RELEASING to find out if a new UL TBF can be recreated
-        * when ansering the final UL ACK. */
-       bool rx_final_pkt_ul_ack_nack_has_tbf_est;
+       struct {
+               /* Whether the Received Packet UL ACK/NACK w/ FinalAck=1 had 
'TBF Est' field to '1'.
+               * Used during ST_RELEASING to find out if a new UL TBF can be 
recreated
+               * when ansering the final UL ACK. */
+               bool has_tbf_est;
+               /* FN and TN of the registered poll to Tx last PKT CTRL ACK
+                * answering the final UL ACK. */
+               uint32_t pkt_ctrl_ack_fn;
+               uint8_t pkt_ctrl_ack_tn;
+       } rx_final_pkt_ul_ack_nack;
 };

 enum tbf_ul_fsm_event {
@@ -39,6 +46,7 @@
        GPRS_RLCMAC_TBF_UL_EV_N3104_MAX,
        GPRS_RLCMAC_TBF_UL_EV_RX_UL_ACK_NACK, /* data: struct 
tbf_ul_ass_ev_rx_ul_ack_nack* */
        GPRS_RLCMAC_TBF_UL_EV_LAST_UL_DATA_SENT,
+       GPRS_RLCMAC_TBF_UL_EV_TX_COMPL_PKT_CTRL_ACK,
 };

 struct tbf_ul_ass_ev_rx_ul_ack_nack {
@@ -52,3 +60,5 @@
 void gprs_rlcmac_tbf_ul_fsm_destructor(struct gprs_rlcmac_ul_tbf *ul_tbf);

 enum gprs_rlcmac_tbf_ul_fsm_states gprs_rlcmac_tbf_ul_state(const struct 
gprs_rlcmac_ul_tbf *ul_tbf);
+
+bool gprs_rlcmac_ul_tbf_waiting_pkt_ctrl_ack_tx_confirmation(const struct 
gprs_rlcmac_ul_tbf *ul_tbf, uint32_t fn, uint8_t ts_nr);
diff --git a/src/rlcmac/rlcmac_prim.c b/src/rlcmac/rlcmac_prim.c
index 97e3cc9..496554e 100644
--- a/src/rlcmac/rlcmac_prim.c
+++ b/src/rlcmac/rlcmac_prim.c
@@ -646,6 +646,36 @@
        return rc;
 }

+static int rlcmac_prim_handle_l1ctl_pdch_data_cnf(struct osmo_gprs_rlcmac_prim 
*rlcmac_prim)
+{
+       struct gprs_rlcmac_entity *gre;
+
+#if 0
+       /* TODO: enable once we have originating req data in primitive coming 
from lower layers. */
+       /* 3GPP TS 44.060 10.3.2 Uplink RLC/MAC control block: */
+       enum osmo_gprs_rlcmac_ul_msg_type msg_type;
+       if (rlcmac_prim->l1ctl.pdch_data_cnf.data_len < 1)
+               return -EINVAL;
+       msg_type = (rlcmac_prim->l1ctl.pdch_data_cnf.data & 0xC0) >> 2;
+       if (msg_type != OSMO_GPRS_RLCMAC_UL_MSGT_PACKET_CONTROL_ACK)
+               return -EINVAL;
+#endif
+
+       llist_for_each_entry(gre, &g_rlcmac_ctx->gre_list, entry) {
+               if (!gre->ul_tbf)
+                       continue;
+               if 
(!gprs_rlcmac_ul_tbf_waiting_pkt_ctrl_ack_tx_confirmation(gre->ul_tbf,
+                                                                            
rlcmac_prim->l1ctl.pdch_data_cnf.fn,
+                                                                            
rlcmac_prim->l1ctl.pdch_data_cnf.ts_nr))
+                       continue;
+
+               osmo_fsm_inst_dispatch(gre->ul_tbf->state_fsm.fi, 
GPRS_RLCMAC_TBF_UL_EV_TX_COMPL_PKT_CTRL_ACK, NULL);
+               /* gre->ul_tbf is NULL here. */
+               break;
+       }
+       return 0;
+}
+
 static int rlcmac_prim_handle_l1ctl_ccch_ready_ind(struct 
osmo_gprs_rlcmac_prim *rlcmac_prim)
 {
        struct gprs_rlcmac_entity *gre;
@@ -670,6 +700,9 @@
        case OSMO_PRIM(OSMO_GPRS_RLCMAC_L1CTL_PDCH_DATA, PRIM_OP_INDICATION):
                rc = rlcmac_prim_handle_l1ctl_pdch_data_ind(rlcmac_prim);
                break;
+       case OSMO_PRIM(OSMO_GPRS_RLCMAC_L1CTL_PDCH_DATA, PRIM_OP_CONFIRM):
+               rc = rlcmac_prim_handle_l1ctl_pdch_data_cnf(rlcmac_prim);
+               break;
        case OSMO_PRIM(OSMO_GPRS_RLCMAC_L1CTL_CCCH_DATA, PRIM_OP_INDICATION):
                rc = rlcmac_prim_handle_l1ctl_ccch_data_ind(rlcmac_prim);
                break;
diff --git a/src/rlcmac/sched.c b/src/rlcmac/sched.c
index 1620934..5f12d5a 100644
--- a/src/rlcmac/sched.c
+++ b/src/rlcmac/sched.c
@@ -158,18 +158,18 @@
 /*! Select a CTRL message to transmit, based on different messages priority.
  * \param[in] bi  RTS block indication information.
  * \param[in] tbfs  TBF candidates having CTRL messages to send, filled in by 
get_ctrl_msg_tbf_candidates()
- * \param[out] tbf_to_free  TBF to free after sending the generated message
+ * \param[out] need_block_conf  Whether we require Tx confirmation of this 
block from lower layers.
  */
 static struct msgb *sched_select_ctrl_msg(const struct 
gprs_rlcmac_rts_block_ind *bi,
                                          const struct 
tbf_sched_ctrl_candidates *tbfs,
-                                         struct gprs_rlcmac_tbf **tbf_to_free)
+                                         bool *need_block_conf)
 {
        struct msgb *msg = NULL;
        struct gprs_rlcmac_entity *gre;
        int rc;

-       /* No TBF to be freed by default: */
-       *tbf_to_free = NULL;
+       /* No TBF needs block conf by default: */
+       *need_block_conf = false;

        /* 8.1.2.2 1) (EGPRS) PACKET DOWNLINK ACK/NACK w/ FinalAckInd=1 */
        if (tbfs->poll_dl_ack_final_ack) {
@@ -209,7 +209,6 @@
                          bi->ts, bi->fn, bi->usf);
                msg = 
gprs_rlcmac_gre_create_pkt_ctrl_ack(ul_tbf_as_tbf(tbfs->poll_ul_ack)->gre);
                /* Last UL message, freeing (after passing msg to lower layers) 
*/
-               *tbf_to_free = ul_tbf_as_tbf(tbfs->poll_ul_ack);
                return msg;
        }
        if (tbfs->poll_dl_ass) {
@@ -288,7 +287,7 @@
 {
        struct msgb *msg = NULL;
        struct tbf_sched_ctrl_candidates tbf_cand = {0};
-       struct gprs_rlcmac_tbf *tbf_to_free;
+       bool need_block_conf;
        struct osmo_gprs_rlcmac_prim *rlcmac_prim_tx;
        int rc = 0;

@@ -296,7 +295,7 @@

        get_ctrl_msg_tbf_candidates(bi, &tbf_cand);

-       if ((msg = sched_select_ctrl_msg(bi, &tbf_cand, &tbf_to_free)))
+       if ((msg = sched_select_ctrl_msg(bi, &tbf_cand, &need_block_conf)))
                goto tx_msg;

        if ((msg = sched_select_ul_data_msg(bi)))
@@ -312,10 +311,11 @@
 tx_msg:
        rlcmac_prim_tx = gprs_rlcmac_prim_alloc_l1ctl_pdch_data_req(bi->ts, 
bi->fn, msgb_data(msg), 0);
        rlcmac_prim_tx->l1ctl.pdch_data_req.data_len = msgb_length(msg);
+       /* TODO: enable once we support conditional confirmation:
+        * rlcmac_prim_tx->l1ctl.pdch_data_req.needs_conf = need_block_conf;
+        */
        rc = gprs_rlcmac_prim_call_down_cb(rlcmac_prim_tx);
        msgb_free(msg);
-       if (tbf_to_free)
-               gprs_rlcmac_tbf_free(tbf_to_free);

 ret_rc:
        gprs_rlcmac_pdch_ulc_expire_fn(g_rlcmac_ctx->sched.ulc[bi->ts], bi->fn);
diff --git a/src/rlcmac/tbf_ul.c b/src/rlcmac/tbf_ul.c
index 3d4ba2b..26d710e 100644
--- a/src/rlcmac/tbf_ul.c
+++ b/src/rlcmac/tbf_ul.c
@@ -212,7 +212,7 @@
                return false;

        /* "TBF Est field is set to '1'"" */
-       if (!ul_tbf->state_fsm.rx_final_pkt_ul_ack_nack_has_tbf_est)
+       if (!ul_tbf->state_fsm.rx_final_pkt_ul_ack_nack.has_tbf_est)
                return false;

        /* the mobile station has new data to transmit */
diff --git a/src/rlcmac/tbf_ul_fsm.c b/src/rlcmac/tbf_ul_fsm.c
index 92d1d64..d5aa9c3 100644
--- a/src/rlcmac/tbf_ul_fsm.c
+++ b/src/rlcmac/tbf_ul_fsm.c
@@ -41,6 +41,7 @@
        { GPRS_RLCMAC_TBF_UL_EV_N3104_MAX,                      "N3104_MAX" },
        { GPRS_RLCMAC_TBF_UL_EV_RX_UL_ACK_NACK,                 
"RX_UL_ACK_NACK" },
        { GPRS_RLCMAC_TBF_UL_EV_LAST_UL_DATA_SENT,              
"LAST_UL_DATA_SENT" },
+       { GPRS_RLCMAC_TBF_UL_EV_TX_COMPL_PKT_CTRL_ACK,          
"TX_COMPL_PKT_CTRL_ACK" },
        { 0, NULL }
 };

@@ -248,6 +249,7 @@
                const Packet_Uplink_Ack_Nack_t *ack = 
&dlbi->dl_block.u.Packet_Uplink_Ack_Nack;
                const PU_AckNack_GPRS_t *gprs = &ack->u.PU_AckNack_GPRS_Struct;
                const Ack_Nack_Description_t *ack_desc = 
&gprs->Ack_Nack_Description;
+               uint32_t poll_fn = 0xffffffff; /* Invalid FN */

                if (gprs_rlcmac_ul_tbf_in_contention_resolution(ctx->ul_tbf)) {
                        _contention_resolution_succeeded(ctx);
@@ -262,7 +264,7 @@
                }
                /* If RRBP contains valid data, schedule a response (PKT 
CONTROL ACK or PKT RESOURCE REQ). */
                if (dlbi->dl_block.SP) {
-                       uint32_t poll_fn = rrbp2fn(dlbi->fn, 
dlbi->dl_block.RRBP);
+                       poll_fn = rrbp2fn(dlbi->fn, dlbi->dl_block.RRBP);
                        
gprs_rlcmac_pdch_ulc_reserve(g_rlcmac_ctx->sched.ulc[dlbi->ts_nr], poll_fn,
                                                     
GPRS_RLCMAC_PDCH_ULC_POLL_UL_ACK,
                                                     
ul_tbf_as_tbf(ctx->ul_tbf));
@@ -270,7 +272,9 @@
                if (ack_desc->FINAL_ACK_INDICATION) {
                        bool tbf_est = (gprs->Exist_AdditionsR99 && 
gprs->AdditionsR99.TBF_EST);
                        LOGPFSMLDLBI(ctx->fi, dlbi, LOGL_DEBUG, "Final ACK 
received\n");
-                       ctx->rx_final_pkt_ul_ack_nack_has_tbf_est = tbf_est;
+                       ctx->rx_final_pkt_ul_ack_nack.has_tbf_est = tbf_est;
+                       ctx->rx_final_pkt_ul_ack_nack.pkt_ctrl_ack_fn = poll_fn;
+                       ctx->rx_final_pkt_ul_ack_nack.pkt_ctrl_ack_tn = 
dlbi->ts_nr;
                        tbf_ul_fsm_state_chg(fi, 
GPRS_RLCMAC_TBF_UL_ST_RELEASING);
                }
                break;
@@ -301,8 +305,11 @@
 /* Waiting for scheduler to transmit PKT CTRL ACK for the already received UL 
ACK/NACK FinalAck=1 */
 static void st_releasing(struct osmo_fsm_inst *fi, uint32_t event, void *data)
 {
-       //struct gprs_rlcmac_tbf_ul_fsm_ctx *ctx = (struct 
gprs_rlcmac_tbf_ul_fsm_ctx *)fi->priv;
+       struct gprs_rlcmac_tbf_ul_fsm_ctx *ctx = (struct 
gprs_rlcmac_tbf_ul_fsm_ctx *)fi->priv;
        switch (event) {
+       case GPRS_RLCMAC_TBF_UL_EV_TX_COMPL_PKT_CTRL_ACK:
+               gprs_rlcmac_ul_tbf_free(ctx->ul_tbf);
+               break;
        default:
                OSMO_ASSERT(0);
        }
@@ -352,7 +359,7 @@
                .action = st_finished,
        },
        [GPRS_RLCMAC_TBF_UL_ST_RELEASING] = {
-               .in_event_mask = 0,
+               .in_event_mask = X(GPRS_RLCMAC_TBF_UL_EV_TX_COMPL_PKT_CTRL_ACK),
                .out_state_mask = 0,
                .name = "RELEASING",
                .action = st_releasing,
@@ -449,3 +456,15 @@
        const struct gprs_rlcmac_tbf_ul_fsm_ctx *ctx = &ul_tbf->state_fsm;
        return ctx->fi->state;
 }
+
+/* Whether the UL TBF requested a Tx confirmation for a Pkt Ctrl Ack it 
transmitted at {FN,TN}. */
+bool gprs_rlcmac_ul_tbf_waiting_pkt_ctrl_ack_tx_confirmation(const struct 
gprs_rlcmac_ul_tbf *ul_tbf, uint32_t fn, uint8_t ts_nr)
+{
+       const struct gprs_rlcmac_tbf_ul_fsm_ctx *ctx = &ul_tbf->state_fsm;
+       /* PKT CTRL ACK Tx confirmation is only needed for final ack, to avoid
+        * freeing (unregistering with lower layers) the TBF too early. */
+       if (ctx->fi->state != GPRS_RLCMAC_TBF_UL_ST_RELEASING)
+               return false;
+       return ctx->rx_final_pkt_ul_ack_nack.pkt_ctrl_ack_fn == fn &&
+               ctx->rx_final_pkt_ul_ack_nack.pkt_ctrl_ack_tn == ts_nr;
+}
diff --git a/tests/rlcmac/rlcmac_prim_test.c b/tests/rlcmac/rlcmac_prim_test.c
index 179888a..bfc491f 100644
--- a/tests/rlcmac/rlcmac_prim_test.c
+++ b/tests/rlcmac/rlcmac_prim_test.c
@@ -623,6 +623,12 @@
        rc = osmo_gprs_rlcmac_prim_lower_up(rlcmac_prim);
        OSMO_ASSERT(rc == 0);

+       /* Trigger transmission confirmation of PKT CTRL ACK */
+       rlcmac_prim = osmo_gprs_rlcmac_prim_alloc_l1ctl_pdch_data_cnf(ts_nr, 
rts_fn, NULL, 0);
+       rc = osmo_gprs_rlcmac_prim_lower_up(rlcmac_prim);
+       OSMO_ASSERT(rc == 0);
+
+
        printf("=== %s end ===\n", __func__);
        cleanup_test();
 }
diff --git a/tests/rlcmac/rlcmac_prim_test.err 
b/tests/rlcmac/rlcmac_prim_test.err
index 366e321..554b6d0 100644
--- a/tests/rlcmac/rlcmac_prim_test.err
+++ b/tests/rlcmac/rlcmac_prim_test.err
@@ -69,6 +69,8 @@
 DLGLOBAL DEBUG (ts=7,fn=21,usf=0) Tx Pkt Control Ack (UL ACK/NACK poll)
 DLGLOBAL DEBUG GRE(00002342) Tx Packet Control Ack
 DLGLOBAL DEBUG Tx to lower layers: L1CTL-PDCH_DATA.request
+DLGLOBAL DEBUG Rx from lower layers: L1CTL-PDCH_DATA.confirm
+DLGLOBAL INFO UL_TBF{RELEASING}: Received Event TX_COMPL_PKT_CTRL_ACK
 DLGLOBAL INFO UL_TBF_ASS{IDLE}: Deallocated
 DLGLOBAL INFO UL_TBF{RELEASING}: Send L1CTL-CFG_UL_TBF.req ul_tbf_nr=0 
(release)
 DLGLOBAL DEBUG Tx to lower layers: L1CTL-CFG_UL_TBF.request

--
To view, visit https://gerrit.osmocom.org/c/libosmo-gprs/+/35775?usp=email
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: libosmo-gprs
Gerrit-Branch: master
Gerrit-Change-Id: Ic38b4207623ccbda3b77d4b0a47431c25de95034
Gerrit-Change-Number: 35775
Gerrit-PatchSet: 5
Gerrit-Owner: fixeria <[email protected]>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <[email protected]>
Gerrit-Reviewer: pespin <[email protected]>
Gerrit-MessageType: merged

Reply via email to