bartoszek/zephyr
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Browse Source 
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 <erbr@oticon.com>
This commit is contained in:
Erik Brockhoff 2024-08-21 13:25:35 +02:00 committed by Fabio Baltieri
parent 3786b617bd
commit 670bd3bed2
4 changed files with 181 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
subsys/bluetooth/controller/ll_sw/ull_llcp_conn_upd.c
View File

@ -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;

18
subsys/bluetooth/controller/ll_sw/ull_llcp_local.c
View File

@ -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)

9
subsys/bluetooth/controller/ll_sw/ull_llcp_remote.c
View File

@ -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:

124
tests/bluetooth/controller/ctrl_conn_update/src/main.c
View File

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