From 83fcbfd0beabb9b424b1d39ef798bbce71ef0f2d Mon Sep 17 00:00:00 2001 From: Morten Priess Date: Wed, 2 Aug 2023 12:22:55 +0200 Subject: [PATCH] Bluetooth: controller: Fixes for CIS Central error handling - Fix masking for no coded phy - Allow ISO_Interval < SDU_Interval for framed mode - Change BT_HCI_ERR_INSUFFICIENT_RESOURCES to BT_HCI_ERR_CONN_LIMIT_EXCEEDED. - Prevent starting same CIS twice - Cancel an initiated CIS creation procedure if terminated before sending CIS_IND. - Implement canceling of local CC procedure. Respond to CIS_RSP with REJECT, if canceled after CIS_REQ was sent. - Introduce state CIG_STATE_INITIATING for central, to keep track of initiating CIS connection, in transition between CONFIGURABLE and ACTIVE. Fixes EBQ test /HCI/CIS/BC-03-C. Signed-off-by: Morten Priess --- .../controller/ll_sw/ull_central_iso.c | 40 ++++++--- subsys/bluetooth/controller/ll_sw/ull_conn.c | 46 ++++++++++- .../controller/ll_sw/ull_conn_iso_types.h | 10 ++- subsys/bluetooth/controller/ll_sw/ull_llcp.c | 14 ++++ subsys/bluetooth/controller/ll_sw/ull_llcp.h | 5 ++ .../bluetooth/controller/ll_sw/ull_llcp_cc.c | 82 +++++++++++++++++++ .../controller/ll_sw/ull_llcp_internal.h | 1 + .../bluetooth/controller/mock_ctrl/src/ull.c | 5 ++ 8 files changed, 188 insertions(+), 15 deletions(-) diff --git a/subsys/bluetooth/controller/ll_sw/ull_central_iso.c b/subsys/bluetooth/controller/ll_sw/ull_central_iso.c index e37ec01a4b7..0c7a8e5ef35 100644 --- a/subsys/bluetooth/controller/ll_sw/ull_central_iso.c +++ b/subsys/bluetooth/controller/ll_sw/ull_central_iso.c @@ -59,6 +59,12 @@ #define STREAMS_PER_GROUP CONFIG_BT_CTLR_CONN_ISO_STREAMS_PER_GROUP +#if defined(CONFIG_BT_CTLR_PHY_CODED) +#define PHY_VALID_MASK (BT_HCI_ISO_PHY_VALID_MASK) +#else +#define PHY_VALID_MASK (BT_HCI_ISO_PHY_VALID_MASK & ~BIT(2)) +#endif + #if (CONFIG_BT_CTLR_CENTRAL_SPACING == 0) static void cig_offset_get(struct ll_conn_iso_stream *cis); static void mfy_cig_offset_get(void *param); @@ -267,7 +273,8 @@ uint8_t ll_cig_parameters_commit(uint8_t cig_id, uint16_t *handles) if (!cis) { /* No space for new CIS */ ll_iso_setup.cis_idx = 0U; - err = BT_HCI_ERR_INSUFFICIENT_RESOURCES; + + err = BT_HCI_ERR_CONN_LIMIT_EXCEEDED; goto ll_cig_parameters_commit_cleanup; } @@ -323,7 +330,7 @@ uint8_t ll_cig_parameters_commit(uint8_t cig_id, uint16_t *handles) tx = cis->lll.tx.bn && cis->lll.tx.max_pdu; rx = cis->lll.rx.bn && cis->lll.rx.max_pdu; } else { - LL_ASSERT(iso_interval_us >= cig->c_sdu_interval); + LL_ASSERT(cis->framed || iso_interval_us >= cig->c_sdu_interval); tx = cig->c_sdu_interval && cis->c_max_sdu; rx = cig->p_sdu_interval && cis->p_max_sdu; @@ -642,7 +649,13 @@ uint8_t ll_cis_create_check(uint16_t cis_handle, uint16_t acl_handle) /* Verify handle validity and association */ cis = ll_conn_iso_stream_get(cis_handle); - if (cis->lll.handle == cis_handle) { + + if (cis->group && (cis->lll.handle == cis_handle)) { + if (cis->established) { + /* CIS is already created */ + return BT_HCI_ERR_CONN_ALREADY_EXISTS; + } + return BT_HCI_ERR_SUCCESS; } } @@ -689,7 +702,14 @@ void ll_cis_create(uint16_t cis_handle, uint16_t acl_handle) cis->lll.link_tx_free = NULL; /* Initiate CIS Request Control Procedure */ - (void)ull_cp_cis_create(conn, cis); + if (ull_cp_cis_create(conn, cis) == BT_HCI_ERR_SUCCESS) { + LL_ASSERT(cis->group); + + if (cis->group->state == CIG_STATE_CONFIGURABLE) { + /* This CIG is now initiating an ISO connection */ + cis->group->state = CIG_STATE_INITIATING; + } + } } /* Core 5.3 Vol 6, Part B section 7.8.100: @@ -712,8 +732,8 @@ uint8_t ll_cig_remove(uint8_t cig_id) return BT_HCI_ERR_UNKNOWN_CONN_ID; } - if (cig->state == CIG_STATE_ACTIVE) { - /* CIG is in active state */ + if ((cig->state == CIG_STATE_INITIATING) || (cig->state == CIG_STATE_ACTIVE)) { + /* CIG is in initiating- or active state */ return BT_HCI_ERR_CMD_DISALLOWED; } @@ -1194,7 +1214,7 @@ static uint8_t ll_cig_parameters_validate(void) /* Requested number of CISes not available by configuration. Check as last * to avoid interfering with qualification parameter checks. */ - return BT_HCI_ERR_INSUFFICIENT_RESOURCES; + return BT_HCI_ERR_CONN_LIMIT_EXCEEDED; } return BT_HCI_ERR_SUCCESS; @@ -1210,13 +1230,13 @@ static uint8_t ll_cis_parameters_validate(uint8_t cis_idx, uint8_t cis_id, return BT_HCI_ERR_INVALID_PARAM; } - if (!c_phy || ((c_phy & ~BT_HCI_ISO_PHY_VALID_MASK) != 0U) || - !p_phy || ((p_phy & ~BT_HCI_ISO_PHY_VALID_MASK) != 0U)) { + if (!c_phy || ((c_phy & ~PHY_VALID_MASK) != 0U) || + !p_phy || ((p_phy & ~PHY_VALID_MASK) != 0U)) { return BT_HCI_ERR_UNSUPP_FEATURE_PARAM_VAL; } if (cis_idx >= STREAMS_PER_GROUP) { - return BT_HCI_ERR_INSUFFICIENT_RESOURCES; + return BT_HCI_ERR_CONN_LIMIT_EXCEEDED; } return BT_HCI_ERR_SUCCESS; diff --git a/subsys/bluetooth/controller/ll_sw/ull_conn.c b/subsys/bluetooth/controller/ll_sw/ull_conn.c index ebaf5b4fe87..0b54a5dee79 100644 --- a/subsys/bluetooth/controller/ll_sw/ull_conn.c +++ b/subsys/bluetooth/controller/ll_sw/ull_conn.c @@ -416,8 +416,52 @@ uint8_t ll_terminate_ind_send(uint16_t handle, uint8_t reason) #if defined(CONFIG_BT_CTLR_PERIPHERAL_ISO) || defined(CONFIG_BT_CTLR_CENTRAL_ISO) if (IS_CIS_HANDLE(handle)) { cis = ll_iso_stream_connected_get(handle); - /* Disallow if CIS is not connected */ if (!cis) { +#if defined(CONFIG_BT_CTLR_CENTRAL_ISO) + /* CIS is not connected - get the unconnected instance */ + cis = ll_conn_iso_stream_get(handle); + + /* Sanity-check instance to make sure it's created but not connected */ + if (cis->group && cis->lll.handle == handle && !cis->established) { + if (cis->group->state == CIG_STATE_CONFIGURABLE) { + /* Disallow if CIG is still in configurable state */ + return BT_HCI_ERR_CMD_DISALLOWED; + + } else if (cis->group->state == CIG_STATE_INITIATING) { + conn = ll_connected_get(cis->lll.acl_handle); + + /* CIS is not yet established - try to cancel procedure */ + if (ull_cp_cc_cancel(conn)) { + /* Successfully canceled - complete disconnect */ + struct node_rx_pdu *node_terminate; + + node_terminate = ull_pdu_rx_alloc(); + LL_ASSERT(node_terminate); + + node_terminate->hdr.handle = handle; + node_terminate->hdr.type = NODE_RX_TYPE_TERMINATE; + *((uint8_t *)node_terminate->pdu) = + BT_HCI_ERR_LOCALHOST_TERM_CONN; + + ll_rx_put_sched(node_terminate->hdr.link, + node_terminate); + + /* We're no longer initiating a connection */ + cis->group->state = CIG_STATE_CONFIGURABLE; + + /* This is now a successful disconnection */ + return BT_HCI_ERR_SUCCESS; + } + + /* Procedure could not be canceled in the current + * state - let it run its course and enqueue a + * terminate procedure. + */ + return ull_cp_cis_terminate(conn, cis, reason); + } + } +#endif /* CONFIG_BT_CTLR_CENTRAL_ISO */ + /* Disallow if CIS is not connected */ return BT_HCI_ERR_CMD_DISALLOWED; } diff --git a/subsys/bluetooth/controller/ll_sw/ull_conn_iso_types.h b/subsys/bluetooth/controller/ll_sw/ull_conn_iso_types.h index 7bef110400c..e8ca29da7af 100644 --- a/subsys/bluetooth/controller/ll_sw/ull_conn_iso_types.h +++ b/subsys/bluetooth/controller/ll_sw/ull_conn_iso_types.h @@ -10,8 +10,9 @@ typedef void (*ll_iso_stream_released_cb_t)(struct ll_conn *conn); #define CIG_STATE_NO_CIG 0 #define CIG_STATE_CONFIGURABLE 1 /* Central only */ -#define CIG_STATE_ACTIVE 2 -#define CIG_STATE_INACTIVE 3 +#define CIG_STATE_INITIATING 2 /* Central only */ +#define CIG_STATE_ACTIVE 3 +#define CIG_STATE_INACTIVE 4 struct ll_conn_iso_stream { struct ll_iso_stream_hdr hdr; @@ -71,8 +72,9 @@ struct ll_conn_iso_group { uint16_t iso_interval; uint8_t cig_id; - uint8_t state:2; /* CIG_STATE_NO_CIG, CIG_STATE_CONFIGURABLE (central only), - * CIG_STATE_ACTIVE or CIG_STATE_INACTIVE. + uint8_t state:3; /* CIG_STATE_NO_CIG, CIG_STATE_CONFIGURABLE (central only), + * CIG_STATE_INITIATING (central only), CIG_STATE_ACTIVE or + * CIG_STATE_INACTIVE. */ uint8_t sca_update:4; /* (new SCA)+1 to trigger restart of ticker */ diff --git a/subsys/bluetooth/controller/ll_sw/ull_llcp.c b/subsys/bluetooth/controller/ll_sw/ull_llcp.c index 7ab387f624b..2b93c17a201 100644 --- a/subsys/bluetooth/controller/ll_sw/ull_llcp.c +++ b/subsys/bluetooth/controller/ll_sw/ull_llcp.c @@ -1398,6 +1398,20 @@ bool ull_cp_cc_awaiting_established(struct ll_conn *conn) return false; } +#if defined(CONFIG_BT_CTLR_CENTRAL_ISO) +bool ull_cp_cc_cancel(struct ll_conn *conn) +{ + struct proc_ctx *ctx; + + ctx = llcp_lr_peek(conn); + if (ctx && ctx->proc == PROC_CIS_CREATE) { + return llcp_lp_cc_cancel(conn, ctx); + } + + return false; +} +#endif /* CONFIG_BT_CTLR_CENTRAL_ISO */ + void ull_cp_cc_established(struct ll_conn *conn, uint8_t error_code) { struct proc_ctx *ctx; diff --git a/subsys/bluetooth/controller/ll_sw/ull_llcp.h b/subsys/bluetooth/controller/ll_sw/ull_llcp.h index 2d406182ef6..ca0ea2c8ed0 100644 --- a/subsys/bluetooth/controller/ll_sw/ull_llcp.h +++ b/subsys/bluetooth/controller/ll_sw/ull_llcp.h @@ -199,6 +199,11 @@ bool ull_cp_cc_awaiting_reply(struct ll_conn *conn); */ bool ull_cp_cc_awaiting_established(struct ll_conn *conn); +/** + * @brief Cancel ongoing create cis procedure + */ +bool ull_cp_cc_cancel(struct ll_conn *conn); + /** * @brief Get handle of ongoing create cis procedure. * @return 0xFFFF if none diff --git a/subsys/bluetooth/controller/ll_sw/ull_llcp_cc.c b/subsys/bluetooth/controller/ll_sw/ull_llcp_cc.c index afb75637bf4..387cc884dea 100644 --- a/subsys/bluetooth/controller/ll_sw/ull_llcp_cc.c +++ b/subsys/bluetooth/controller/ll_sw/ull_llcp_cc.c @@ -647,6 +647,8 @@ enum { LP_CC_STATE_WAIT_OFFSET_CALC_TX_REQ, LP_CC_STATE_WAIT_TX_CIS_REQ, LP_CC_STATE_WAIT_RX_CIS_RSP, + LP_CC_STATE_WAIT_NOTIFY_CANCEL, + LP_CC_STATE_WAIT_RX_CIS_RSP_CANCEL, LP_CC_STATE_WAIT_TX_CIS_IND, LP_CC_STATE_WAIT_INSTANT, LP_CC_STATE_WAIT_ESTABLISHED, @@ -916,6 +918,59 @@ static void lp_cc_st_wait_rx_cis_rsp(struct ll_conn *conn, struct proc_ctx *ctx, } } +static void lp_cc_st_wait_notify_cancel(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t evt, + void *param) +{ + switch (evt) { + case LP_CC_EVT_RUN: + if (llcp_ntf_alloc_is_available()) { + ctx->node_ref.rx = llcp_ntf_alloc(); + + /* Mark node as RETAIN to trigger put/sched */ + ctx->node_ref.rx->hdr.type = NODE_RX_TYPE_RETAIN; + ctx->state = LP_CC_STATE_WAIT_ESTABLISHED; + + llcp_lp_cc_established(conn, ctx); + } + break; + default: + /* Ignore other evts */ + break; + } +} + +static void lp_cc_st_wait_rx_cis_rsp_cancel(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t evt, + void *param) +{ + struct pdu_data *pdu; + struct node_tx *tx; + + switch (evt) { + case LP_CC_EVT_CIS_RSP: + /* Allocate tx node */ + tx = llcp_tx_alloc(conn, ctx); + LL_ASSERT(tx); + + pdu = (struct pdu_data *)tx->pdu; + + /* Encode LL Control PDU */ + llcp_pdu_encode_reject_ext_ind(pdu, PDU_DATA_LLCTRL_TYPE_CIS_RSP, + ctx->data.cis_create.error); + + /* Enqueue LL Control PDU towards LLL */ + llcp_tx_enqueue(conn, tx); + lp_cc_complete(conn, ctx, evt, param); + break; + case LP_CC_EVT_UNKNOWN: + case LP_CC_EVT_REJECT: + lp_cc_complete(conn, ctx, evt, param); + break; + default: + /* Ignore other evts */ + break; + } +} + static void lp_cc_st_wait_tx_cis_ind(struct ll_conn *conn, struct proc_ctx *ctx, uint8_t evt, void *param) { @@ -997,6 +1052,12 @@ static void lp_cc_execute_fsm(struct ll_conn *conn, struct proc_ctx *ctx, uint8_ case LP_CC_STATE_WAIT_RX_CIS_RSP: lp_cc_st_wait_rx_cis_rsp(conn, ctx, evt, param); break; + case LP_CC_STATE_WAIT_NOTIFY_CANCEL: + lp_cc_st_wait_notify_cancel(conn, ctx, evt, param); + break; + case LP_CC_STATE_WAIT_RX_CIS_RSP_CANCEL: + lp_cc_st_wait_rx_cis_rsp_cancel(conn, ctx, evt, param); + break; case LP_CC_STATE_WAIT_TX_CIS_IND: lp_cc_st_wait_tx_cis_ind(conn, ctx, evt, param); break; @@ -1032,4 +1093,25 @@ void llcp_lp_cc_established(struct ll_conn *conn, struct proc_ctx *ctx) { lp_cc_execute_fsm(conn, ctx, LP_CC_EVT_ESTABLISHED, NULL); } + +bool llcp_lp_cc_cancel(struct ll_conn *conn, struct proc_ctx *ctx) +{ + ctx->data.cis_create.error = BT_HCI_ERR_OP_CANCELLED_BY_HOST; + + switch (ctx->state) { + case LP_CC_STATE_IDLE: + case LP_CC_STATE_WAIT_OFFSET_CALC: + case LP_CC_STATE_WAIT_OFFSET_CALC_TX_REQ: + case LP_CC_STATE_WAIT_TX_CIS_REQ: + ctx->state = LP_CC_STATE_WAIT_NOTIFY_CANCEL; + return true; + case LP_CC_STATE_WAIT_RX_CIS_RSP: + ctx->state = LP_CC_STATE_WAIT_RX_CIS_RSP_CANCEL; + return true; + default: + break; + } + + return false; +} #endif /* CONFIG_BT_CTLR_CENTRAL_ISO */ diff --git a/subsys/bluetooth/controller/ll_sw/ull_llcp_internal.h b/subsys/bluetooth/controller/ll_sw/ull_llcp_internal.h index f2fc0e287d9..0e1afb9ff1d 100644 --- a/subsys/bluetooth/controller/ll_sw/ull_llcp_internal.h +++ b/subsys/bluetooth/controller/ll_sw/ull_llcp_internal.h @@ -738,6 +738,7 @@ void llcp_lp_cc_run(struct ll_conn *conn, struct proc_ctx *ctx, void *param); bool llcp_lp_cc_is_active(struct proc_ctx *ctx); bool llcp_lp_cc_awaiting_established(struct proc_ctx *ctx); void llcp_lp_cc_established(struct ll_conn *conn, struct proc_ctx *ctx); +bool llcp_lp_cc_cancel(struct ll_conn *conn, struct proc_ctx *ctx); void llcp_rp_cc_init_proc(struct proc_ctx *ctx); void llcp_rp_cc_rx(struct ll_conn *conn, struct proc_ctx *ctx, struct node_rx_pdu *rx); diff --git a/tests/bluetooth/controller/mock_ctrl/src/ull.c b/tests/bluetooth/controller/mock_ctrl/src/ull.c index a7e9785c9c6..2b9c6711813 100644 --- a/tests/bluetooth/controller/mock_ctrl/src/ull.c +++ b/tests/bluetooth/controller/mock_ctrl/src/ull.c @@ -263,6 +263,11 @@ int ull_disable(void *lll) return 0; } +void *ull_pdu_rx_alloc(void) +{ + return NULL; +} + void ull_rx_put(memq_link_t *link, void *rx) { }