pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-gprs/+/34313?usp=email ) Change subject: tbf_ul_ass_fsm: Listen only on 1 TS for PKT UL ASS when assignment done from DL TBF ...................................................................... tbf_ul_ass_fsm: Listen only on 1 TS for PKT UL ASS when assignment done from DL TBF This also fixes a memcpy writing out of bounds reported by Coverity CID#323120 in gprs_rlcmac_tbf_ul_ass_start_from_dl_tbf_ack_nack(), due to the difference of size between struct gprs_rlcmac_ul_tbf_allocation and struct gprs_rlcmac_dl_tbf_allocation. While fixing it, actually properly implement passing of the 1 only interesting TS to the tbf_ul_ass_fsm at that point in time. Change-Id: I89b15982b73f00599183981142495d7b9befbb78 --- M include/osmocom/gprs/rlcmac/rlcmac_enc.h M include/osmocom/gprs/rlcmac/tbf_dl.h M include/osmocom/gprs/rlcmac/tbf_ul_ass_fsm.h M src/rlcmac/rlcmac_enc.c M src/rlcmac/sched.c M src/rlcmac/tbf_dl.c M src/rlcmac/tbf_ul_ass_fsm.c M tests/rlcmac/rlcmac_prim_test.err 8 files changed, 47 insertions(+), 24 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/rlcmac_enc.h b/include/osmocom/gprs/rlcmac/rlcmac_enc.h index 298d364..1ed02be 100644 --- a/include/osmocom/gprs/rlcmac/rlcmac_enc.h +++ b/include/osmocom/gprs/rlcmac/rlcmac_enc.h @@ -47,6 +47,6 @@ void gprs_rlcmac_enc_prepare_pkt_resource_req(RlcMacUplink_t *block, struct gprs_rlcmac_ul_tbf *ul_tbf, enum gprs_rlcmac_access_type acc_type); -void gprs_rlcmac_enc_prepare_pkt_downlink_ack_nack(RlcMacUplink_t *block, const struct gprs_rlcmac_dl_tbf *dl_tbf); +void gprs_rlcmac_enc_prepare_pkt_downlink_ack_nack(RlcMacUplink_t *block, const struct gprs_rlcmac_dl_tbf *dl_tbf, bool chan_req); void gprs_rlcmac_enc_prepare_pkt_ctrl_ack(RlcMacUplink_t *block, uint32_t tlli); diff --git a/include/osmocom/gprs/rlcmac/tbf_dl.h b/include/osmocom/gprs/rlcmac/tbf_dl.h index fe0b2ef..fc75972 100644 --- a/include/osmocom/gprs/rlcmac/tbf_dl.h +++ b/include/osmocom/gprs/rlcmac/tbf_dl.h @@ -45,7 +45,7 @@ int gprs_rlcmac_dl_tbf_configure_l1ctl(struct gprs_rlcmac_dl_tbf *dl_tbf); -struct msgb *gprs_rlcmac_dl_tbf_create_pkt_dl_ack_nack(struct gprs_rlcmac_dl_tbf *dl_tbf); +struct msgb *gprs_rlcmac_dl_tbf_create_pkt_dl_ack_nack(struct gprs_rlcmac_dl_tbf *dl_tbf, uint8_t tn); int gprs_rlcmac_dl_tbf_rcv_data_block(struct gprs_rlcmac_dl_tbf *dl_tbf, const struct gprs_rlcmac_rlc_data_info *rlc, diff --git a/include/osmocom/gprs/rlcmac/tbf_ul_ass_fsm.h b/include/osmocom/gprs/rlcmac/tbf_ul_ass_fsm.h index ea9e521..ad32471 100644 --- a/include/osmocom/gprs/rlcmac/tbf_ul_ass_fsm.h +++ b/include/osmocom/gprs/rlcmac/tbf_ul_ass_fsm.h @@ -82,7 +82,7 @@ int gprs_rlcmac_tbf_ul_ass_start(struct gprs_rlcmac_ul_tbf *ul_tbf, enum gprs_rlcmac_tbf_ul_ass_type type); int gprs_rlcmac_tbf_ul_ass_start_from_releasing_ul_tbf(struct gprs_rlcmac_ul_tbf *ul_tbf, struct gprs_rlcmac_ul_tbf *old_ul_tbf); -int gprs_rlcmac_tbf_ul_ass_start_from_dl_tbf_ack_nack(struct gprs_rlcmac_ul_tbf *ul_tbf, const struct gprs_rlcmac_dl_tbf *dl_tbf); +int gprs_rlcmac_tbf_ul_ass_start_from_dl_tbf_ack_nack(struct gprs_rlcmac_ul_tbf *ul_tbf, const struct gprs_rlcmac_dl_tbf *dl_tbf, uint8_t tn); bool gprs_rlcmac_tbf_ul_ass_pending(struct gprs_rlcmac_ul_tbf *ul_tbf); bool gprs_rlcmac_tbf_ul_ass_match_rach_req(struct gprs_rlcmac_ul_tbf *ul_tbf, uint8_t ra); diff --git a/src/rlcmac/rlcmac_enc.c b/src/rlcmac/rlcmac_enc.c index d8cd61e..dec2fa6 100644 --- a/src/rlcmac/rlcmac_enc.c +++ b/src/rlcmac/rlcmac_enc.c @@ -384,11 +384,9 @@ /* TODO: fill cqr from info stored probably in the gre object. */ } -void gprs_rlcmac_enc_prepare_pkt_downlink_ack_nack(RlcMacUplink_t *block, const struct gprs_rlcmac_dl_tbf *dl_tbf) +void gprs_rlcmac_enc_prepare_pkt_downlink_ack_nack(RlcMacUplink_t *block, const struct gprs_rlcmac_dl_tbf *dl_tbf, bool chan_req) { Packet_Downlink_Ack_Nack_t *ack = &block->u.Packet_Downlink_Ack_Nack; - struct gprs_rlcmac_entity *gre = dl_tbf->tbf.gre; - int rc; memset(block, 0, sizeof(*block)); ack->MESSAGE_TYPE = OSMO_GPRS_RLCMAC_UL_MSGT_PACKET_DOWNLINK_ACK_NACK; @@ -398,9 +396,7 @@ ack->DOWNLINK_TFI = dl_tbf->cur_alloc.dl_tfi; gprs_rlcmac_enc_prepare_pkt_ack_nack_desc_gprs(&ack->Ack_Nack_Description, dl_tbf); - /* Channel Request Description. Request a UL-TBF if we have UL data - * queued to send and no active UL BF (TS 44.060 8.1.2.5) */ - if (!gre->ul_tbf && gprs_rlcmac_entity_have_tx_data_queued(gre)) { + if (chan_req) { Channel_Request_Description_t *chan_req = &ack->Channel_Request_Description; ack->Exist_Channel_Request_Description = 1; chan_req->PEAK_THROUGHPUT_CLASS = 0; /* TODO */ @@ -408,11 +404,6 @@ chan_req->RLC_MODE = GPRS_RLCMAC_RLC_MODE_ACKNOWLEDGED; chan_req->LLC_PDU_TYPE = GPRS_RLCMAC_LLC_PDU_TYPE_ACKNOWLEDGED; chan_req->RLC_OCTET_COUNT = 0; /* TODO */ - - gre->ul_tbf = gprs_rlcmac_ul_tbf_alloc(gre); - rc = gprs_rlcmac_tbf_ul_ass_start_from_dl_tbf_ack_nack(gre->ul_tbf, dl_tbf); - if (rc < 0) - LOGPTBFDL(dl_tbf, LOGL_ERROR, "Failed starting assignment of requested UL TBF (%d)\n", rc); } else { ack->Exist_Channel_Request_Description = 0; } diff --git a/src/rlcmac/sched.c b/src/rlcmac/sched.c index 6af2b6a..faa9ff1 100644 --- a/src/rlcmac/sched.c +++ b/src/rlcmac/sched.c @@ -175,7 +175,7 @@ if (tbfs->poll_dl_ack_final_ack) { LOGRLCMAC(LOGL_DEBUG, "(ts=%u,fn=%u,usf=%u) Tx DL ACK/NACK FinalAck=1\n", bi->ts, bi->fn, bi->usf); - msg = gprs_rlcmac_dl_tbf_create_pkt_dl_ack_nack(tbfs->poll_dl_ack_final_ack); + msg = gprs_rlcmac_dl_tbf_create_pkt_dl_ack_nack(tbfs->poll_dl_ack_final_ack, bi->ts); if (msg) return msg; } @@ -237,7 +237,7 @@ if (tbfs->poll_dl_ack) { LOGRLCMAC(LOGL_DEBUG, "(ts=%u,fn=%u,usf=%u) Tx DL ACK/NACK\n", bi->ts, bi->fn, bi->usf); - msg = gprs_rlcmac_dl_tbf_create_pkt_dl_ack_nack(tbfs->poll_dl_ack); + msg = gprs_rlcmac_dl_tbf_create_pkt_dl_ack_nack(tbfs->poll_dl_ack, bi->ts); if (msg) return msg; } diff --git a/src/rlcmac/tbf_dl.c b/src/rlcmac/tbf_dl.c index a7f0ea9..b8d37a8 100644 --- a/src/rlcmac/tbf_dl.c +++ b/src/rlcmac/tbf_dl.c @@ -160,11 +160,13 @@ return gprs_rlcmac_prim_call_down_cb(rlcmac_prim); } -struct msgb *gprs_rlcmac_dl_tbf_create_pkt_dl_ack_nack(struct gprs_rlcmac_dl_tbf *dl_tbf) +struct msgb *gprs_rlcmac_dl_tbf_create_pkt_dl_ack_nack(struct gprs_rlcmac_dl_tbf *dl_tbf, uint8_t tn) { struct msgb *msg; struct bitvec bv; RlcMacUplink_t ul_block; + bool chan_req = false; + struct gprs_rlcmac_entity *gre = dl_tbf->tbf.gre; int rc; OSMO_ASSERT(dl_tbf); @@ -173,6 +175,17 @@ if (!msg) return NULL; + /* Channel Request Description. Request a UL-TBF if we have UL data + * queued to send and no active UL TBF (TS 44.060 8.1.2.5) */ + if (!gre->ul_tbf && gprs_rlcmac_entity_have_tx_data_queued(gre)) { + gre->ul_tbf = gprs_rlcmac_ul_tbf_alloc(gre); + rc = gprs_rlcmac_tbf_ul_ass_start_from_dl_tbf_ack_nack(gre->ul_tbf, dl_tbf, tn); + if (rc < 0) + LOGPTBFDL(dl_tbf, LOGL_ERROR, "Failed starting assignment of requested UL TBF (%d)\n", rc); + else + chan_req = true; + } + /* Initialize a bit vector that uses allocated msgb as the data buffer. */ bv = (struct bitvec){ .data = msgb_put(msg, GSM_MACBLOCK_LEN), @@ -180,7 +193,7 @@ }; bitvec_unhex(&bv, GPRS_RLCMAC_DUMMY_VEC); - gprs_rlcmac_enc_prepare_pkt_downlink_ack_nack(&ul_block, dl_tbf); + gprs_rlcmac_enc_prepare_pkt_downlink_ack_nack(&ul_block, dl_tbf, chan_req); rc = osmo_gprs_rlcmac_encode_uplink(&bv, &ul_block); if (rc < 0) { LOGPTBFDL(dl_tbf, LOGL_ERROR, "Encoding of Packet Downlink ACK/NACK failed (%d)\n", rc); diff --git a/src/rlcmac/tbf_ul_ass_fsm.c b/src/rlcmac/tbf_ul_ass_fsm.c index 0f0f9ee..5587ddb 100644 --- a/src/rlcmac/tbf_ul_ass_fsm.c +++ b/src/rlcmac/tbf_ul_ass_fsm.c @@ -680,14 +680,17 @@ /* A DL-TBF requested a UL TBF over DL ACK/NACK, wait to receive Pkt Ul Ass for * it, aka switch the FSM to trigger the 2hpase directly (tx Pkt Res Req) */ -int gprs_rlcmac_tbf_ul_ass_start_from_dl_tbf_ack_nack(struct gprs_rlcmac_ul_tbf *ul_tbf, const struct gprs_rlcmac_dl_tbf *dl_tbf) +int gprs_rlcmac_tbf_ul_ass_start_from_dl_tbf_ack_nack(struct gprs_rlcmac_ul_tbf *ul_tbf, const struct gprs_rlcmac_dl_tbf *dl_tbf, uint8_t tn) { int rc; ul_tbf->ul_ass_fsm.dl_tbf = dl_tbf; - /* FIXME: Ideally this should only be the TS where the PKT UL ASS was received... */ - ul_tbf->ul_ass_fsm.phase1_alloc.num_ts = dl_tbf->cur_alloc.num_ts; - memcpy(&ul_tbf->ul_ass_fsm.phase1_alloc.ts[0], &dl_tbf->cur_alloc.ts[0], - sizeof(ul_tbf->ul_ass_fsm.phase1_alloc.ts)); + + /* The TS where the PKT UL ASS is to be received is the one where the DL + * ACK/NACK was sent in the DL TBF (control TS): */ + ul_tbf->ul_ass_fsm.phase1_alloc.num_ts = 1; + ul_tbf->ul_ass_fsm.phase1_alloc.ts[tn].allocated = true; + ul_tbf->ul_ass_fsm.phase1_alloc.ts[tn].usf = 0xff; + rc = osmo_fsm_inst_dispatch(ul_tbf->ul_ass_fsm.fi, GPRS_RLCMAC_TBF_UL_ASS_EV_START_FROM_DL_TBF, NULL); diff --git a/tests/rlcmac/rlcmac_prim_test.err b/tests/rlcmac/rlcmac_prim_test.err index 95dc475..41773b2 100644 --- a/tests/rlcmac/rlcmac_prim_test.err +++ b/tests/rlcmac/rlcmac_prim_test.err @@ -950,13 +950,13 @@ DLGLOBAL DEBUG GRE(00000001) Enqueueing LLC-PDU len=14 SAPI=GMM radio_prio=1 DLGLOBAL DEBUG Rx from lower layers: L1CTL-PDCH_RTS.indication DLGLOBAL DEBUG (ts=7,fn=21,usf=0) Tx DL ACK/NACK FinalAck=1 -DLGLOBAL DEBUG TBF(DL:NR-0:TLLI-00000001) - SSN 1, V(N): "IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIR" R=Received I=Invalid, FINAL_ACK=1 DLGLOBAL INFO UL_TBF{NEW}: Allocated DLGLOBAL INFO UL_TBF_ASS{IDLE}: Allocated DLGLOBAL INFO UL_TBF_ASS{IDLE}: Received Event START_FROM_DL_TBF DLGLOBAL INFO UL_TBF{NEW}: Received Event UL_ASS_START DLGLOBAL INFO UL_TBF{NEW}: state_chg to ASSIGN DLGLOBAL INFO UL_TBF_ASS{IDLE}: state_chg to WAIT_PKT_UL_ASS +DLGLOBAL DEBUG TBF(DL:NR-0:TLLI-00000001) - SSN 1, V(N): "IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIR" R=Received I=Invalid, FINAL_ACK=1 DLGLOBAL INFO TBF(DL:NR-0:TLLI-00000001) Starting T3192 (0 ms) DLGLOBAL DEBUG Tx to lower layers: L1CTL-PDCH_DATA.request DLGLOBAL DEBUG Rx from lower layers: L1CTL-PDCH_DATA.indication -- To view, visit https://gerrit.osmocom.org/c/libosmo-gprs/+/34313?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: I89b15982b73f00599183981142495d7b9befbb78 Gerrit-Change-Number: 34313 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> Gerrit-Reviewer: pespin <pespin@sysmocom.de> Gerrit-MessageType: merged