Bluetooth: controller: llcp: fix issue re. missing ack of terminate ind

On remote terminate on central the conn clean-up would happen before ack of terminate ind was sent to peer. Now clean-up is 'postponed' until subsequent event. Also now data tx is paused on rx of terminate ind to ensure no data is tx'ed after rx of terminate ind Signed-off-by: Erik Brockhoff <erbr@oticon.com>
2022-06-21 13:26:49 +02:00 · 2022-06-21 13:26:49 +02:00 · 8b1d50b981
parent 8b912f1488
commit 8b1d50b981
4 changed files with 76 additions and 24 deletions
--- a/subsys/bluetooth/controller/ll_sw/ull_conn.c
+++ b/subsys/bluetooth/controller/ll_sw/ull_conn.c
@ -1479,32 +1479,33 @@ void ull_conn_done(struct node_rx_event_done *done)
 	}
 #endif /* CONFIG_BT_CTLR_LE_ENC */

-	/* Peripheral received terminate ind or
+	/* Legacy LLCP:
+	 * Peripheral received terminate ind or
 	 * Central received ack for the transmitted terminate ind or
 	 * Central transmitted ack for the received terminate ind or
 	 * there has been MIC failure
+	 * Refactored LLCP:
+	 * reason_final is set exactly under the above conditions
 	 */
 	reason_final = conn->llcp_terminate.reason_final;
 	if (reason_final && (
-#if defined(CONFIG_BT_PERIPHERAL)
-			    lll->role ||
-#else /* CONFIG_BT_PERIPHERAL */
-			    0 ||
-#endif /* CONFIG_BT_PERIPHERAL */
-#if defined(CONFIG_BT_CENTRAL)
 #if defined(CONFIG_BT_LL_SW_LLCP_LEGACY)
-			    (((conn->llcp_terminate.req -
-			       conn->llcp_terminate.ack) & 0xFF) ==
-			     TERM_ACKED) ||
-			    conn->central.terminate_ack ||
-			    (reason_final == BT_HCI_ERR_TERM_DUE_TO_MIC_FAIL)
-#else /* CONFIG_BT_LL_SW_LLCP_LEGACY */
-			    1
-#endif /* CONFIG_BT_LL_SW_LLCP_LEGACY */
-#else /* CONFIG_BT_CENTRAL */
-			    1
+#if defined(CONFIG_BT_CENTRAL)
+		(((conn->llcp_terminate.req -
+		       conn->llcp_terminate.ack) & 0xFF) ==
+		     TERM_ACKED) ||
+		    conn->central.terminate_ack ||
+		    (reason_final == BT_HCI_ERR_TERM_DUE_TO_MIC_FAIL) ||
 #endif /* CONFIG_BT_CENTRAL */
-			    )) {
+#if defined(CONFIG_BT_PERIPHERAL)
+		lll->role
+#else /* CONFIG_BT_PERIPHERAL */
+		false
+#endif /* CONFIG_BT_PERIPHERAL */
+#else /* defined(CONFIG_BT_LL_SW_LLCP_LEGACY) */
+	    true
+#endif /* !defined(CONFIG_BT_LL_SW_LLCP_LEGACY) */
+		)) {
 		conn_cleanup(conn, reason_final);

 		return;
--- a/subsys/bluetooth/controller/ll_sw/ull_llcp_common.c
+++ b/subsys/bluetooth/controller/ll_sw/ull_llcp_common.c
@ -77,6 +77,7 @@ enum {
 enum {
 	RP_COMMON_STATE_IDLE,
 	RP_COMMON_STATE_WAIT_RX,
+	RP_COMMON_STATE_POSTPONE_TERMINATE,
 	RP_COMMON_STATE_WAIT_TX,
 	RP_COMMON_STATE_WAIT_TX_ACK,
 	RP_COMMON_STATE_WAIT_NTF,
@ -93,6 +94,7 @@ enum {
 	RP_COMMON_EVT_REQUEST,
 };

+
 static void lp_comm_ntf(struct ll_conn *conn, struct proc_ctx *ctx);
 static void lp_comm_terminate_invalid_pdu(struct ll_conn *conn, struct proc_ctx *ctx);

@ -781,8 +783,17 @@ void llcp_lp_comm_init_proc(struct proc_ctx *ctx)
 void llcp_lp_comm_run(struct ll_conn *conn, struct proc_ctx *ctx, void *param)
 {
 	lp_comm_execute_fsm(conn, ctx, LP_COMMON_EVT_RUN, param);
+
 }

+static void rp_comm_terminate(struct ll_conn *conn, struct proc_ctx *ctx)
+{
+	llcp_rr_complete(conn);
+	ctx->state = RP_COMMON_STATE_IDLE;
+
+	/* Mark the connection for termination */
+	conn->llcp_terminate.reason_final = ctx->data.term.error_code;
+}
 /*
 * LLCP Remote Procedure Common FSM
 */
@ -821,6 +832,8 @@ static void rp_comm_rx_decode(struct ll_conn *conn, struct proc_ctx *ctx, struct
 		break;
 	case PDU_DATA_LLCTRL_TYPE_TERMINATE_IND:
 		llcp_pdu_decode_terminate_ind(ctx, pdu);
+		/* Make sure no data is tx'ed after RX of terminate ind */
+		llcp_tx_pause_data(conn, LLCP_TX_QUEUE_PAUSE_DATA_TERMINATE);
 		break;
 #if defined(CONFIG_BT_CTLR_DATA_LENGTH)
 	case PDU_DATA_LLCTRL_TYPE_LENGTH_REQ:
@ -1051,12 +1064,19 @@ static void rp_comm_send_rsp(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t
 		break;
 #endif /* CONFIG_BT_CTLR_MIN_USED_CHAN && CONFIG_BT_CENTRAL */
 	case PROC_TERMINATE:
-		/* No response */
-		llcp_rr_complete(conn);
-		ctx->state = RP_COMMON_STATE_IDLE;
-
-		/* Mark the connection for termination */
-		conn->llcp_terminate.reason_final = ctx->data.term.error_code;
+#if defined(CONFIG_BT_CENTRAL)
+		if (conn->lll.role == BT_HCI_ROLE_CENTRAL) {
+			/* No response, but postpone terminate until next event
+			 * to ensure acking the reception of TERMINATE_IND
+			 */
+			ctx->state = RP_COMMON_STATE_POSTPONE_TERMINATE;
+			break;
+		}
+#endif
+#if defined(CONFIG_BT_PERIPHERAL)
+		/* Terminate right away */
+		rp_comm_terminate(conn, ctx);
+#endif
 		break;
 #if defined(CONFIG_BT_CTLR_DATA_LENGTH)
 	case PROC_DATA_LENGTH_UPDATE:
@ -1102,6 +1122,26 @@ static void rp_comm_st_wait_rx(struct ll_conn *conn, struct proc_ctx *ctx, uint8
 	}
 }

+static void rp_comm_st_postpone_terminate(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t evt,
+				   void *param)
+{
+	switch (evt) {
+	case RP_COMMON_EVT_RUN:
+		LL_ASSERT(ctx->proc == PROC_TERMINATE);
+
+		/* Note: now we terminate, mimicking legacy LLCP behaviour
+		 * A check should be added to ensure that the ack of the terminate_ind was
+		 * indeed tx'ed and not scheduled out/postponed by LLL
+		 */
+		rp_comm_terminate(conn, ctx);
+
+		break;
+	default:
+		/* Ignore other evts */
+		break;
+	}
+}
+
 static void rp_comm_st_wait_tx(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t evt, void *param)
 {
 	switch (evt) {
@ -1181,6 +1221,9 @@ static void rp_comm_execute_fsm(struct ll_conn *conn, struct proc_ctx *ctx, uint
 	case RP_COMMON_STATE_WAIT_RX:
 		rp_comm_st_wait_rx(conn, ctx, evt, param);
 		break;
+	case RP_COMMON_STATE_POSTPONE_TERMINATE:
+		rp_comm_st_postpone_terminate(conn, ctx, evt, param);
+		break;
 	case RP_COMMON_STATE_WAIT_TX:
 		rp_comm_st_wait_tx(conn, ctx, evt, param);
 		break;
--- a/subsys/bluetooth/controller/ll_sw/ull_llcp_internal.h
+++ b/subsys/bluetooth/controller/ll_sw/ull_llcp_internal.h
@ -33,6 +33,7 @@ enum llcp_tx_q_pause_data_mask {
 	LLCP_TX_QUEUE_PAUSE_DATA_ENCRYPTION = 0x01,
 	LLCP_TX_QUEUE_PAUSE_DATA_PHY_UPDATE = 0x02,
 	LLCP_TX_QUEUE_PAUSE_DATA_DATA_LENGTH = 0x04,
+	LLCP_TX_QUEUE_PAUSE_DATA_TERMINATE = 0x08,
 };

 #if ((CONFIG_BT_CTLR_LLCP_COMMON_TX_CTRL_BUF_NUM <\
--- a/tests/bluetooth/controller/ctrl_terminate/src/main.c
+++ b/tests/bluetooth/controller/ctrl_terminate/src/main.c
@ -66,9 +66,16 @@ static void test_terminate_rem(uint8_t role)
 	/* Done */
 	event_done(&conn);

+	/* Prepare */
+	event_prepare(&conn);
+
+	/* Done */
+	event_done(&conn);
+
 	/* There should be no host notification */
 	ut_rx_q_is_empty();

+
 	zassert_equal(ctx_buffers_free(), test_ctx_buffers_cnt(),
 		      "Free CTX buffers %d", ctx_buffers_free());
 }