From 670bd3bed213be82ca7320eb38c865c4e5de2ce1 Mon Sep 17 00:00:00 2001 From: Erik Brockhoff Date: Wed, 21 Aug 2024 13:25:35 +0200 Subject: [PATCH] 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 --- .../controller/ll_sw/ull_llcp_conn_upd.c | 36 +++++ .../controller/ll_sw/ull_llcp_local.c | 18 ++- .../controller/ll_sw/ull_llcp_remote.c | 9 +- .../controller/ctrl_conn_update/src/main.c | 124 ++++++++++++++++++ 4 files changed, 181 insertions(+), 6 deletions(-) diff --git a/subsys/bluetooth/controller/ll_sw/ull_llcp_conn_upd.c b/subsys/bluetooth/controller/ll_sw/ull_llcp_conn_upd.c index db0d2711748..b107371dc77 100644 --- a/subsys/bluetooth/controller/ll_sw/ull_llcp_conn_upd.c +++ b/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; diff --git a/subsys/bluetooth/controller/ll_sw/ull_llcp_local.c b/subsys/bluetooth/controller/ll_sw/ull_llcp_local.c index 249943b10b2..6307b50e332 100644 --- a/subsys/bluetooth/controller/ll_sw/ull_llcp_local.c +++ b/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) diff --git a/subsys/bluetooth/controller/ll_sw/ull_llcp_remote.c b/subsys/bluetooth/controller/ll_sw/ull_llcp_remote.c index 66476aae6dd..94351132ddc 100644 --- a/subsys/bluetooth/controller/ll_sw/ull_llcp_remote.c +++ b/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: diff --git a/tests/bluetooth/controller/ctrl_conn_update/src/main.c b/tests/bluetooth/controller/ctrl_conn_update/src/main.c index 635e36c4ce1..ed2b694a159 100644 --- a/tests/bluetooth/controller/ctrl_conn_update/src/main.c +++ b/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