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 <[email protected]>

Author: erbr-ot

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_LL_SW_LLCP_LEGACY)
+#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 ||
+		lll->role
 #else /* CONFIG_BT_PERIPHERAL */
-			    0 ||
+		false
 #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
-#endif /* CONFIG_BT_CENTRAL */
-			    )) {
+#else /* defined(CONFIG_BT_LL_SW_LLCP_LEGACY) */
+	    true
+#endif /* !defined(CONFIG_BT_LL_SW_LLCP_LEGACY) */
+		)) {
 		conn_cleanup(conn, reason_final);
 
 		return;

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;

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 <\

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());
 }