michaelh/zephyr
1
0
Fork 0
Code Releases Activity

Bluetooth: Controller: Fix CTE_REQ disable lock if there is no CTE_RSP

Browse source 
The periodic CTE_REQ disable command, requested by Host, may be locked
until connection is dropped due to missing CTE_RSP from peer device.
That is caused by implementation of CTE_REQ disable and CTE_REQ
control procedure handling.

The procedure is marked as active when CTE request was send to peer
device. It is marked as inactive after completion of the procedure.
That caused locking of CTE disable on a semaphore.

The BT 5.3 Core Spec, Vol 4, Part E, section 7.8.85 says the HCI_LE_-
Connection_CTE_Request_Enable should be considered active on a conne-
ction from when Host successfully issues the command with Enable=0x1
until a command is issued with Enable=0x0 or single LLCP CTE request
has finished (CTE_Request_Interval=0x0). Also there is a clarification
from BT SIG that the command with Enable=0x0 does not affect any
initiated LLCP CTE request. That means Controller is allowed to finish
already started procedure and it is not allowed to start new LLCP CTE
request procedure after completion of the command with Enable=0x0.

Taking that into account, there is no need to synchronize ULL and LLL
in regard of disable the LLCP CTE request while the procedure is
pending. Controller is free to complete the procedure or terminate it.

The change removes all code related with cte_req.is_active, disable
callback and waiting of ULL for LLL to finish the LLCP CTE request.

The ULL will complete the HCI_LE_Connection_Request_Enable with
Enable=0x0 immediately. In case the procedure is disabled in before
the response arrives, then further processing of the response is
dropped and the procedure context released.

The context is not released by the code responsible for disable
handling, to have single place where it is done.

Signed-off-by: Piotr Pryga <piotr.pryga@nordicsemi.no>
This commit is contained in:
Piotr Pryga 2022-04-01 14:52:33 +02:00 • committed by Maureen Helm
commit e81414d28f
6 changed files with 80 additions and 87 deletions
Download patch file Download diff file Expand all files Collapse all files

7
subsys/bluetooth/controller/ll_sw/ull_conn.c
View file

@ -1672,13 +1672,8 @@ void ull_conn_done(struct node_rx_event_done *done)
if (err == BT_HCI_ERR_CMD_DISALLOWED) {
/* Conditions has changed e.g. PHY was changed to CODED.
* New CTE REQ is not possible. Disable the periodic requests.
*
* If the CTE REQ is in active state, let it complete and disable
* in regular control procedure way.
*/
if (!conn->llcp.cte_req.is_active) {
ull_cp_cte_req_set_disable(conn);
}
ull_cp_cte_req_set_disable(conn);
}
}
}

5
subsys/bluetooth/controller/ll_sw/ull_conn_types.h
View file

@ -428,15 +428,12 @@ struct llcp_struct {
/* Procedure may be active periodically, active state must be stored.
* If procedure is active, request parameters update may not be issued.
*/
uint8_t is_enabled:1;
uint8_t is_active:1;
volatile uint8_t is_enabled;
uint8_t cte_type;
/* Minimum requested CTE length in 8us units */
uint8_t min_cte_len;
uint16_t req_interval;
uint16_t req_expire;
void *disable_param;
void (*disable_cb)(void *param);
} cte_req;
#endif /* CONFIG_BT_CTLR_DF_CONN_CTE_REQ */

16
subsys/bluetooth/controller/ll_sw/ull_df.c
View file

@ -1123,12 +1123,12 @@ uint8_t ll_df_set_conn_cte_rx_params(uint16_t handle, uint8_t sampling_enable,
}
#endif /* CONFIG_BT_CTLR_DF_CONN_CTE_RX */
#if defined(CONFIG_BT_CTLR_DF_CONN_CTE_REQ) || defined(CONFIG_BT_CTLR_DF_CONN_CTE_RSP)
#if defined(CONFIG_BT_CTLR_DF_CONN_CTE_RSP)
static void df_conn_cte_req_disable(void *param)
{
k_sem_give(param);
}
#endif /* CONFIG_BT_CTLR_DF_CONN_CTE_REQ || CONFIG_BT_CTLR_DF_CONN_CTE_RSP */
#endif /* CONFIG_BT_CTLR_DF_CONN_CTE_RSP */
#if defined(CONFIG_BT_CTLR_DF_CONN_CTE_REQ)
/* @brief Function enables or disables CTE request control procedure for a connection.
@ -1165,18 +1165,6 @@ uint8_t ll_df_set_conn_cte_req_enable(uint16_t handle, uint8_t enable,
if (!enable) {
ull_cp_cte_req_set_disable(conn);
if (conn->llcp.cte_req.is_active) {
struct k_sem sem;
k_sem_init(&sem, 0U, 1U);
conn->llcp.cte_req.disable_param = &sem;
conn->llcp.cte_req.disable_cb = df_conn_cte_req_disable;
if (!conn->llcp.cte_req.is_active) {
k_sem_take(&sem, K_FOREVER);
}
}
return BT_HCI_ERR_SUCCESS;
} else {
if (!conn->lll.df_rx_cfg.is_initialized) {

5
subsys/bluetooth/controller/ll_sw/ull_llcp.c
View file

@ -485,9 +485,6 @@ void ull_llcp_init(struct ll_conn *conn)
#if defined(CONFIG_BT_CTLR_DF_CONN_CTE_REQ)
conn->llcp.cte_req.is_enabled = 0U;
conn->llcp.cte_req.req_expire = 0U;
conn->llcp.cte_req.is_active = 0U;
conn->llcp.cte_req.disable_param = NULL;
conn->llcp.cte_req.disable_cb = NULL;
#endif /* CONFIG_BT_CTLR_DF_CONN_CTE_REQ */
#if defined(CONFIG_BT_CTLR_DF_CONN_CTE_RSP)
conn->llcp.cte_rsp.is_enabled = 0U;
@ -981,8 +978,6 @@ uint8_t ull_cp_cte_req(struct ll_conn *conn, uint8_t min_cte_len, uint8_t cte_ty
ctx->data.cte_req.min_len = min_cte_len;
ctx->data.cte_req.type = cte_type;
conn->llcp.cte_req.is_active = 1U;
llcp_lr_enqueue(conn, ctx);
return BT_HCI_ERR_SUCCESS;

115
subsys/bluetooth/controller/ll_sw/ull_llcp_common.c
View file

@ -93,6 +93,9 @@ 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);
/*
* LLCP Local Procedure Common FSM
*/
@ -205,23 +208,6 @@ static void lp_comm_complete_cte_req_finalize(struct ll_conn *conn)
{
llcp_rr_set_paused_cmd(conn, PROC_NONE);
llcp_lr_complete(conn);
conn->llcp.cte_req.is_active = 0U;
/* Disable the CTE request procedure when it is completed in case it was executed as
* non-periodic.
*/
if (conn->llcp.cte_req.req_interval == 0U) {
conn->llcp.cte_req.is_enabled = 0U;
}
/* If disable_cb is not NULL then there is waiting CTE REQ disable request
* from host. Execute the callback to notify waiting thread that the
* procedure is inactive.
*/
if (conn->llcp.cte_req.disable_cb) {
conn->llcp.cte_req.disable_cb(conn->llcp.cte_req.disable_param);
}
}
static void lp_comm_ntf_cte_req(struct ll_conn *conn, struct proc_ctx *ctx, struct pdu_data *pdu)
@ -241,6 +227,56 @@ static void lp_comm_ntf_cte_req(struct ll_conn *conn, struct proc_ctx *ctx, stru
LL_ASSERT(0);
}
}
static void lp_comm_complete_cte_req(struct ll_conn *conn, struct proc_ctx *ctx)
{
if (conn->llcp.cte_req.is_enabled) {
if (ctx->response_opcode == PDU_DATA_LLCTRL_TYPE_CTE_RSP) {
if (ctx->data.cte_remote_rsp.has_cte) {
if (conn->llcp.cte_req.req_interval != 0U) {
conn->llcp.cte_req.req_expire =
conn->llcp.cte_req.req_interval;
}
ctx->state = LP_COMMON_STATE_IDLE;
} else if (llcp_ntf_alloc_is_available()) {
lp_comm_ntf(conn, ctx);
ull_cp_cte_req_set_disable(conn);
ctx->state = LP_COMMON_STATE_IDLE;
} else {
ctx->state = LP_COMMON_STATE_WAIT_NTF;
}
} else if (ctx->response_opcode == PDU_DATA_LLCTRL_TYPE_REJECT_EXT_IND &&
ctx->reject_ext_ind.reject_opcode == PDU_DATA_LLCTRL_TYPE_CTE_REQ) {
if (llcp_ntf_alloc_is_available()) {
lp_comm_ntf(conn, ctx);
ull_cp_cte_req_set_disable(conn);
ctx->state = LP_COMMON_STATE_IDLE;
} else {
ctx->state = LP_COMMON_STATE_WAIT_NTF;
}
} else if (ctx->response_opcode == PDU_DATA_LLCTRL_TYPE_UNUSED) {
/* This path is related with handling disable the CTE REQ when PHY
* has been changed to CODED PHY. BT 5.3 Core Vol 4 Part E 7.8.85
* says CTE REQ has to be automatically disabled as if it had been requested
* by Host. There is no notification send to Host.
*/
ull_cp_cte_req_set_disable(conn);
ctx->state = LP_COMMON_STATE_IDLE;
} else {
/* Illegal response opcode */
lp_comm_terminate_invalid_pdu(conn, ctx);
}
} else {
/* The CTE_REQ was disabled by Host after the request was send.
* It does not matter if response has arrived, it should not be handled.
*/
ctx->state = LP_COMMON_STATE_IDLE;
}
if (ctx->state == LP_COMMON_STATE_IDLE) {
lp_comm_complete_cte_req_finalize(conn);
}
}
#endif /* CONFIG_BT_CTLR_DF_CONN_CTE_REQ */
static void lp_comm_ntf(struct ll_conn *conn, struct proc_ctx *ctx)
@ -388,45 +424,7 @@ static void lp_comm_complete(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t
#endif /* CONFIG_BT_CTLR_DATA_LENGTH */
#if defined(CONFIG_BT_CTLR_DF_CONN_CTE_REQ)
case PROC_CTE_REQ:
if (ctx->response_opcode == PDU_DATA_LLCTRL_TYPE_CTE_RSP) {
if (ctx->data.cte_remote_rsp.has_cte) {
if (conn->llcp.cte_req.req_interval != 0U) {
conn->llcp.cte_req.req_expire =
conn->llcp.cte_req.req_interval;
}
ctx->state = LP_COMMON_STATE_IDLE;
} else if (llcp_ntf_alloc_is_available()) {
lp_comm_ntf(conn, ctx);
ull_cp_cte_req_set_disable(conn);
ctx->state = LP_COMMON_STATE_IDLE;
} else {
ctx->state = LP_COMMON_STATE_WAIT_NTF;
}
} else if (ctx->response_opcode == PDU_DATA_LLCTRL_TYPE_REJECT_EXT_IND &&
ctx->reject_ext_ind.reject_opcode == PDU_DATA_LLCTRL_TYPE_CTE_REQ) {
if (llcp_ntf_alloc_is_available()) {
lp_comm_ntf(conn, ctx);
ull_cp_cte_req_set_disable(conn);
ctx->state = LP_COMMON_STATE_IDLE;
} else {
ctx->state = LP_COMMON_STATE_WAIT_NTF;
}
} else if (ctx->response_opcode == PDU_DATA_LLCTRL_TYPE_UNUSED) {
/* This path is related with handling disable the CTE REQ when PHY
* has been changed to CODED PHY. BT 5.3 Core Vol 4 Part E 7.8.85
* says CTE REQ has to be automatically disabled as if it had been requested
* by Host. There is no notification send to Host.
*/
ull_cp_cte_req_set_disable(conn);
ctx->state = LP_COMMON_STATE_IDLE;
} else {
/* Illegal response opcode */
lp_comm_terminate_invalid_pdu(conn, ctx);
}
if (ctx->state == LP_COMMON_STATE_IDLE) {
lp_comm_complete_cte_req_finalize(conn);
}
lp_comm_complete_cte_req(conn, ctx);
break;
#endif /* CONFIG_BT_CTLR_DF_CONN_CTE_REQ */
default:
@ -518,10 +516,11 @@ static void lp_comm_send_req(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t
#endif /* CONFIG_BT_CTLR_DATA_LENGTH */
#if defined(CONFIG_BT_CTLR_DF_CONN_CTE_REQ)
case PROC_CTE_REQ:
if (conn->llcp.cte_req.is_enabled &&
#if defined(CONFIG_BT_CTLR_PHY)
if (conn->lll.phy_rx != PHY_CODED) {
conn->lll.phy_rx != PHY_CODED) {
#else
if (1) {
1) {
#endif /* CONFIG_BT_CTLR_PHY */
if (llcp_lr_ispaused(conn) || !llcp_tx_alloc_peek(conn, ctx) ||
(llcp_rr_get_paused_cmd(conn) == PROC_CTE_REQ)) {

19
tests/bluetooth/controller/ctrl_cte_req/src/main.c
View file

@ -84,7 +84,10 @@ void test_cte_req_central_local(void)
/* Connect */
ull_cp_state_set(&conn, ULL_CP_CONNECTED);
/* Initiate an CTE Request Procedure */
conn.llcp.cte_req.is_enabled = 1U;
err = ull_cp_cte_req(&conn, local_cte_req.min_cte_len_req, local_cte_req.cte_type_req);
zassert_equal(err, BT_HCI_ERR_SUCCESS, NULL);
@ -261,6 +264,8 @@ void test_cte_req_peripheral_local(void)
ull_cp_state_set(&conn, ULL_CP_CONNECTED);
/* Initiate an CTE Request Procedure */
conn.llcp.cte_req.is_enabled = 1U;
err = ull_cp_cte_req(&conn, local_cte_req.min_cte_len_req, local_cte_req.cte_type_req);
zassert_equal(err, BT_HCI_ERR_SUCCESS, NULL);
@ -475,6 +480,8 @@ void test_cte_req_rejected_inv_ll_param_central_local(void)
ull_cp_state_set(&conn, ULL_CP_CONNECTED);
/* Initiate an CTE Request Procedure */
conn.llcp.cte_req.is_enabled = 1U;
err = ull_cp_cte_req(&conn, local_cte_req.min_cte_len_req, local_cte_req.cte_type_req);
zassert_equal(err, BT_HCI_ERR_SUCCESS, NULL);
@ -549,6 +556,8 @@ void test_cte_req_rejected_inv_ll_param_peripheral_local(void)
ull_cp_state_set(&conn, ULL_CP_CONNECTED);
/* Initiate an CTE Request Procedure */
conn.llcp.cte_req.is_enabled = 1U;
err = ull_cp_cte_req(&conn, local_cte_req.min_cte_len_req, local_cte_req.cte_type_req);
zassert_equal(err, BT_HCI_ERR_SUCCESS, NULL);
@ -1124,6 +1133,8 @@ static void test_local_cte_req_wait_for_phy_update_complete_and_disable(uint8_t
zassert_equal(err, BT_HCI_ERR_SUCCESS, NULL);
/* Initiate an CTE Request Procedure */
conn.llcp.cte_req.is_enabled = 1U;
err = ull_cp_cte_req(&conn, local_cte_req.min_cte_len_req, local_cte_req.cte_type_req);
zassert_equal(err, BT_HCI_ERR_SUCCESS, NULL);
@ -1189,6 +1200,8 @@ static void test_local_cte_req_wait_for_phy_update_complete(uint8_t role)
zassert_equal(err, BT_HCI_ERR_SUCCESS, NULL);
/* Initiate an CTE Request Procedure */
conn.llcp.cte_req.is_enabled = 1U;
err = ull_cp_cte_req(&conn, local_cte_req.min_cte_len_req, local_cte_req.cte_type_req);
zassert_equal(err, BT_HCI_ERR_SUCCESS, NULL);
@ -1235,6 +1248,8 @@ static void test_local_phy_update_wait_for_cte_req_complete(uint8_t role)
ull_cp_state_set(&conn, ULL_CP_CONNECTED);
/* Initiate an CTE Request Procedure */
conn.llcp.cte_req.is_enabled = 1U;
err = ull_cp_cte_req(&conn, local_cte_req.min_cte_len_req, local_cte_req.cte_type_req);
zassert_equal(err, BT_HCI_ERR_SUCCESS, NULL);
@ -1385,6 +1400,8 @@ static void test_cte_req_wait_for_remote_phy_update_complete_and_disable(uint8_t
event_done(&conn);
/* Initiate an CTE Request Procedure */
conn.llcp.cte_req.is_enabled = 1U;
err = ull_cp_cte_req(&conn, local_cte_req.min_cte_len_req, local_cte_req.cte_type_req);
zassert_equal(err, BT_HCI_ERR_SUCCESS, NULL);
@ -1439,6 +1456,8 @@ static void test_cte_req_wait_for_remote_phy_update_complete(uint8_t role)
event_done(&conn);
/* Initiate an CTE Request Procedure */
conn.llcp.cte_req.is_enabled = 1U;
err = ull_cp_cte_req(&conn, local_cte_req.min_cte_len_req, local_cte_req.cte_type_req);
zassert_equal(err, BT_HCI_ERR_SUCCESS, NULL);