Fix DL_TBF PACCH ass done on UL_TBF already scheduled to tx last PKT CTRL ACK

Dispatching TBF_EV_CONTENTION_RESOLUTION_MS_SUCCESS means the UL TBF is
able to be used to assign DL TBFs over PACCH.
However, there's an extra implicit restriction: if the PCU already sent
the final UL ACK/NACK to that UL TBF, then whatever message sent
after it cannot be reliably answered, since the MS will go back to idle
state after issues the PKT CTRL ACK for that final UL ACK/NACK.
This condition is already being checked in
contention_resolution_success():

"""
	if (ms_need_dl_tbf(ms()) && !tbf_ul_ack_waiting_cnf_final_ack(this))
		ms_new_dl_tbf_assigned_on_pacch(ms(), this);
"""

Since we are considering the UL TBF to have done contention resolution
when we transmit the *first* UL ACK/NACK, it mans we are doing both things
on the same code path iif the *first* UL ACK/NACK is at the same time
the *final* UL ACK for that UL TBF. This can happen if the UL TBF is
only sending 1 block, which will have CV=0. This can usually happen
during GMM Attach Request.
In this scenario, with current code goes into a situation where first
the TBF_EV_CONTENTION_RESOLUTION_MS_SUCCESS is triggered and *afterwards*
the UL_ACK/NACK state is updated. As a result, the code snippet check
above (!tbf_ul_ack_waiting_cnf_final_ack()) will be true and the DL TBF
be assigned, but afterwards in the same code path it figures out the
final ack happens.

In order to fix this, first update the ACK/NACK state and only
afterwards trigger the Contention Resolution Success event.

Change-Id: I62ae91b494e4fd0ade3f4a3ba3817bcaedbdebf5

2 files changed, 25 insertions, 18 deletions

diff --git a/src/tbf_ul_ack_fsm.c b/src/tbf_ul_ack_fsm.c
index ad6ad02f..2fa20d70 100644
--- a/src/tbf_ul_ack_fsm.c
+++ b/src/tbf_ul_ack_fsm.c
@@ -65,7 +65,6 @@ static struct msgb *create_ul_ack_nack(const struct tbf_ul_ack_fsm_ctx *ctx,
 	unsigned int rrbp = 0;
 	uint32_t new_poll_fn = 0;
 	struct gprs_rlcmac_tbf *tbf = ul_tbf_as_tbf(ctx->tbf);
-	struct GprsMs *ms = tbf_ms(tbf);
 
 	if (final) {
 		rc = tbf_check_polling(tbf, d->pdch, d->fn, &new_poll_fn, &rrbp);
@@ -86,21 +85,6 @@ static struct msgb *create_ul_ack_nack(const struct tbf_ul_ack_fsm_ctx *ctx,
 	bitvec_pack(ack_vec, msgb_put(msg, 23));
 	bitvec_free(ack_vec);
 
-	/* TS 44.060 7a.2.1.1: "The contention resolution is completed on
-	 * the network side when the network receives an RLC data block that
-	 * comprises the TLLI value that identifies the mobile station and the
-	 * TFI value associated with the TBF." (see TBF_EV_FIRST_UL_DATA_RECVD).
-	 *
-	 * However, it's handier for us to mark contention resolution success here
-	 * since upon rx UL ACK is the time at which MS realizes contention resolution
-	 * succeeds:
-	 * TS 44.060 7.1.2.3: "The contention resolution is successfully completed
-	 * on the mobile station side when the mobile station receives a
-	 * PACKET UPLINK ACK/NACK"
-	 */
-	if (ms_tlli(ms) != GSM_RESERVED_TMSI && !ul_tbf_contention_resolution_done(ctx->tbf))
-		osmo_fsm_inst_dispatch(tbf_state_fi(tbf), TBF_EV_CONTENTION_RESOLUTION_MS_SUCCESS, NULL);
-
 	if (final) {
 		tbf_set_polling(tbf, d->pdch, new_poll_fn, PDCH_ULC_POLL_UL_ACK);
 		LOGPTBFUL(ctx->tbf, LOGL_DEBUG,
@@ -126,6 +110,7 @@ static void st_sched_ul_ack(struct osmo_fsm_inst *fi, uint32_t event, void *data
 {
 	struct tbf_ul_ack_fsm_ctx *ctx = (struct tbf_ul_ack_fsm_ctx *)fi->priv;
 	struct gprs_rlcmac_ul_tbf *tbf = ctx->tbf;
+	struct GprsMs *ms = tbf_ms(ul_tbf_as_tbf(tbf));
 	struct tbf_ul_ack_ev_create_rlcmac_msg_ctx *data_ctx;
 	bool final;
 
@@ -144,6 +129,28 @@ static void st_sched_ul_ack(struct osmo_fsm_inst *fi, uint32_t event, void *data
 			tbf_ul_ack_fsm_state_chg(fi, TBF_UL_ACK_ST_WAIT_ACK);
 		else
 			tbf_ul_ack_fsm_state_chg(fi, TBF_UL_ACK_ST_NONE);
+
+		/* TS 44.060 7a.2.1.1: "The contention resolution is completed on
+		* the network side when the network receives an RLC data block that
+		* comprises the TLLI value that identifies the mobile station and the
+		* TFI value associated with the TBF." (see TBF_EV_FIRST_UL_DATA_RECVD).
+		*
+		* However, it's handier for us to mark contention resolution success here
+		* since upon rx UL ACK is the time at which MS realizes contention resolution
+		* succeeds:
+		* TS 44.060 7.1.2.3: "The contention resolution is successfully completed
+		* on the mobile station side when the mobile station receives a
+		* PACKET UPLINK ACK/NACK"
+		*
+		* This event must be triggered here *after* potentially transitioning
+		* to ST_WAIT_ACK above, so that gprs_ms knows whether it can create a
+		* DL TBF on PACCH of the UL_TBF or not (not possible if we are in
+		* ST_WAIT_ACK, since UL TBF is terminating sending the final PKT CTRL
+		* ACK).
+		*/
+		if (ms_tlli(ms) != GSM_RESERVED_TMSI && !ul_tbf_contention_resolution_done(ctx->tbf))
+			osmo_fsm_inst_dispatch(tbf_state_fi(ul_tbf_as_tbf(ctx->tbf)), TBF_EV_CONTENTION_RESOLUTION_MS_SUCCESS, NULL);
+
 		break;
 	default:
 		OSMO_ASSERT(0);
diff --git a/tests/tbf/TbfTest.err b/tests/tbf/TbfTest.err
index 95411fd8..867ee9ec 100644
--- a/tests/tbf/TbfTest.err
+++ b/tests/tbf/TbfTest.err
@@ -2072,11 +2072,11 @@ Got 'TBF(UL:TFI-0-0-0:G:TLLI-0xf1223344){FINISHED}', TA=7
 Got MS: TLLI = 0xf1223344, TA = 7
 UL_ACK_TBF(UL:TFI-0-0-0:G){SCHED_UL_ACK}: Received Event CREATE_RLCMAC_MSG
 PDCH(bts=0,trx=0,ts=7) POLL scheduled at FN 2654167 + 17 = 2654184
-UL_TBF(UL:TFI-0-0-0:G){FINISHED}: Received Event CONTENTION_RESOLUTION_MS_SUCCESS
-TBF(UL:TFI-0-0-0:G:TLLI-0xf1223344){FINISHED} stopping timer T3141 [Contention resolution success (UL-TBF, CCCH)]
 PDCH(bts=0,trx=0,ts=7) Reserving FN 2654184 for type POLL
 TBF(UL:TFI-0-0-0:G:TLLI-0xf1223344){FINISHED} Scheduled UL Acknowledgement polling on PACCH (FN=2654184, TS=7)
 UL_ACK_TBF(UL:TFI-0-0-0:G){SCHED_UL_ACK}: state_chg to WAIT_ACK
+UL_TBF(UL:TFI-0-0-0:G){FINISHED}: Received Event CONTENTION_RESOLUTION_MS_SUCCESS
+TBF(UL:TFI-0-0-0:G:TLLI-0xf1223344){FINISHED} stopping timer T3141 [Contention resolution success (UL-TBF, CCCH)]
 PDCH(bts=0,trx=0,ts=7) FN=2654167 Scheduling control message at RTS for TBF(UL:TFI-0-0-0:G:TLLI-0xf1223344){FINISHED}
 Modifying MS object, TLLI = 0xf1223344, IMSI '' -> '0011223344'
 Modifying MS object, TLLI: 0xf1223344 confirmed