Bluetooth: controller: fixing issue re. assert on overlapping conn upd proc

If a connection update (from central) overlaps a peripheral initiated
conneection param request, then central rejects peripheral request
and continues central procedure. This lead to an assert in peripheral
On instant there would not be an rx_node for notification, as this
would have been discarded by the reception of the REJECT.

This fix ensures that once an rx_node has been 'accepted' for retention
it will not be discarded/overwritten by future rx_nodes

The test will trigger the assert without the fix in.

Signed-off-by: Erik Brockhoff <[email protected]>

Author: erbr-ot

subsys/bluetooth/controller/ll_sw/ull_llcp_conn_upd.c

@@ -676,6 +676,42 @@ static void lp_cu_st_wait_instant(struct ll_conn *conn, struct proc_ctx *ctx, ui
 	case LP_CU_EVT_RUN:
 		lp_cu_check_instant(conn, ctx, evt, param);
 		break;
+	case LP_CU_EVT_REJECT:
+		/* In case of a central overtaking a peripheral initiated connection
+		 * param request the following will occur:
+		 * Since CONNECTION_UPDATE_IND PDU is used both as response to the peripheral
+		 * connection param request AND as initiation of a remote connection update
+		 * the peripheral cannot tell the difference in the case when there is a
+		 * collision. Ie when the central and peripheral 'exchange' CONNECTION_PARAM_REQ
+		 * and CONNECTION_UPDATE_IND in the same connection event. In this case
+		 * the central should follow the CONNECTION_UPDATE_IND with a REJECT_IND to
+		 * signal procedure collision and then complete procedure as initiated.
+		 * The peer on the other hand, should abandon the local initiated procedure
+		 * and instead run the remote connection update to completion. What happens
+		 * instead is that the peripheral reaches WAIT_FOR_INSTANT state as it
+		 * assumes the UPDATE_IND was a response to the local procedure. As a result
+		 * the REJECT_IND will be passed into the local procedure RX path and end up
+		 * 'here'. See test case: test_conn_update_periph_loc_reject_central_overlap
+		 * in unit test: tests/bluetooth/controller/ctrl_conn_update/src/main.c
+		 *
+		 * In the current implementation of LLCP there is no way of handling
+		 * this transition from local initiated to remote initiated procedure and thus
+		 * the only way to handle this 'corner' case is to allow the local procedure to
+		 * complete and accept impact of not having it complete as a remote procedure.
+		 *
+		 * The impact being:
+		 *  -> since it runs as a local procedure it will block other local procedures
+		 *     from being initiated while non-complete. Since non-instant based procedures
+		 *     are allowed this is a limitation
+		 *  -> since procedure continues as local initiated the procedure timeout will
+		 *     be off (too short) by as much as the time between CONNECTION_PARAM_REQ
+		 *     and CONNECTION_UPDATE_IND pdus
+		 *
+		 * The work around for this is to ignore the REJECT_IND at this stage and
+		 * ensure proper function of RX node retention mechanism.
+		 * (see comment in ull_llcp_local.c::llcp_lr_rx() for details on this)
+		 */
+		/* Fall through to ignore */
 	default:
 		/* Ignore other evts */
 		break;

subsys/bluetooth/controller/ll_sw/ull_llcp_local.c

@@ -243,9 +243,21 @@ void llcp_lr_flush_procedures(struct ll_conn *conn)
 void llcp_lr_rx(struct ll_conn *conn, struct proc_ctx *ctx, memq_link_t *link,
 		struct node_rx_pdu *rx)
 {
-	/* Store RX node and link */
-	ctx->node_ref.rx = rx;
-	ctx->node_ref.link = link;
+	/* In the case of a specific connection update procedure collision it can occur that
+	 * an 'unexpected' REJECT_IND_PDU is received and passed as RX'ed and will then result in
+	 * discarding of the retention of the previously received CONNECTION_UPDATE_IND
+	 * and following this, an assert will be hit when attempting to use this retained
+	 * RX node for creating the notification on completion of connection param request.
+	 * (see comment in ull_llcp_conn_upd.c::lp_cu_st_wait_instant() for more details)
+	 *
+	 * The workaround/fix for this is to only store an RX node for retention if
+	 * 'we havent already' got one
+	 */
+	if (!ctx->node_ref.rx) {
+		/* Store RX node and link */
+		ctx->node_ref.rx = rx;
+		ctx->node_ref.link = link;
+	}
 
 	switch (ctx->proc) {
 #if defined(CONFIG_BT_CTLR_LE_PING)

subsys/bluetooth/controller/ll_sw/ull_llcp_remote.c

@@ -236,9 +236,12 @@ void llcp_rr_flush_procedures(struct ll_conn *conn)
 void llcp_rr_rx(struct ll_conn *conn, struct proc_ctx *ctx, memq_link_t *link,
 		struct node_rx_pdu *rx)
 {
-	/* Store RX node and link */
-	ctx->node_ref.rx = rx;
-	ctx->node_ref.link = link;
+	/* See comment in ull_llcp_local.c::llcp_lr_rx() */
+	if (!ctx->node_ref.rx) {
+		/* Store RX node and link */
+		ctx->node_ref.rx = rx;
+		ctx->node_ref.link = link;
+	}
 
 	switch (ctx->proc) {
 	case PROC_UNKNOWN:

tests/bluetooth/controller/ctrl_conn_update/src/main.c

@@ -1991,6 +1991,130 @@ ZTEST(periph_loc, test_conn_update_periph_loc_reject)
 		      "Free CTX buffers %d", llcp_ctx_buffers_free());
 }
 
+/*
+ * Peripheral-initiated Connection Parameters Request procedure. (A)
+ * Peripheral requests change in LE connection parameters, central rejects due to
+ * Central-initiated Connection Update procedure (B) overlapping.
+ * Central rejects peripheral init and assumes 'own' connection update to complete
+ *
+ * +-----+                    +-------+                          +-----+
+ * | UT  |                    | LL_P  |                          | LT  |
+ * +-----+                    +-------+                          +-----+
+ *    |                           |                                 |
+ *    | LE Connection Update (A)  |                                 |
+ *    |-------------------------->|                                 |
+ *    |                           | LL_CONNECTION_PARAM_REQ         | (A)
+ *    |                           |-------------------------------->|
+ *    |                           |                                 |
+ *    |                           |<--------------------------------|
+ *    |                           |        LL_CONNECTION_UPDATE_IND | (B)
+ *    |                           |                                 |
+ *    |                           |              LL_REJECT_EXT_IND  | (A)
+ *    |                           |<--------------------------------|
+ *    |                           |                                 |
+ *    |                           |                                 |
+ *    |      LE Connection Update |                                 |
+ *    |                  Complete |                                 | (A/B)
+ *    |<--------------------------|                                 |
+ *    |                           |                                 |
+ */
+ZTEST(periph_loc, test_conn_update_periph_loc_reject_central_overlap)
+{
+	uint8_t err;
+	uint16_t instant;
+	struct node_tx *tx;
+	struct node_rx_pdu *ntf;
+	struct node_rx_pu cu2 = { .status = BT_HCI_ERR_SUCCESS };
+	struct pdu_data_llctrl_reject_ext_ind reject_ext_ind = {
+		.reject_opcode = PDU_DATA_LLCTRL_TYPE_CONN_PARAM_REQ,
+		.error_code = BT_HCI_ERR_LL_PROC_COLLISION
+	};
+
+	/* Role */
+	test_set_role(&conn, BT_HCI_ROLE_PERIPHERAL);
+
+	/* Connect */
+	ull_cp_state_set(&conn, ULL_CP_CONNECTED);
+
+	/* Initiate a Connection Parameter Request Procedure */
+	err = ull_cp_conn_update(&conn, INTVL_MIN, INTVL_MAX, LATENCY, TIMEOUT, NULL);
+	zassert_equal(err, BT_HCI_ERR_SUCCESS);
+
+	/* Prepare */
+	event_prepare(&conn);
+	conn_param_req.reference_conn_event_count = event_counter(&conn);
+
+	/* Tx Queue should have one LL Control PDU */
+	lt_rx(LL_CONNECTION_PARAM_REQ, &conn, &tx, &conn_param_req);
+	lt_rx_q_is_empty(&conn);
+
+	/* Done */
+	event_done(&conn);
+
+	/* Release Tx */
+	ull_cp_release_tx(&conn, tx);
+
+	/* Prepare */
+	event_prepare(&conn);
+
+	cu_ind_B->instant = instant = event_counter(&conn) + 6;
+	lt_tx(LL_CONNECTION_UPDATE_IND, &conn, cu_ind_B);
+
+	/* Done */
+	event_done(&conn);
+
+	/* Release Tx */
+	ull_cp_release_tx(&conn, tx);
+
+	/* Tx Queue should NOT have a LL Control PDU */
+	lt_rx_q_is_empty(&conn);
+
+	/* Prepare */
+	event_prepare(&conn);
+
+	/* Rx */
+	lt_tx(LL_REJECT_EXT_IND, &conn, &reject_ext_ind);
+
+	/* Done */
+	event_done(&conn);
+
+	/* There should be no host notification */
+	ut_rx_q_is_empty();
+
+	/* */
+	while (!is_instant_reached(&conn, instant)) {
+		/* Prepare */
+		event_prepare(&conn);
+
+		/* (B) Tx Queue should NOT have a LL Control PDU */
+		lt_rx_q_is_empty(&conn);
+
+		/* Done */
+		event_done(&conn);
+
+		/* (B) There should NOT be a host notification */
+		ut_rx_q_is_empty();
+	}
+
+	/* Prepare */
+	event_prepare(&conn);
+
+	/* (B) Tx Queue should NOT have a LL Control PDU */
+	lt_rx_q_is_empty(&conn);
+
+	/* Done */
+	event_done(&conn);
+
+	/* (B) There should be one host notification */
+	ut_rx_node(NODE_CONN_UPDATE, &ntf, &cu2);
+	ut_rx_q_is_empty();
+
+	/* Release Ntf */
+	release_ntf(ntf);
+	zassert_equal(llcp_ctx_buffers_free(), test_ctx_buffers_cnt(),
+		      "Free CTX buffers %d", llcp_ctx_buffers_free());
+}
+
 /*
  * Peripheral-initiated Connection Parameters Request procedure.
  * Peripheral requests change in LE connection parameters, central’s Controller do not