From fc14570585fad16d2dc9396c4e3c1fdf11d8dfd1 Mon Sep 17 00:00:00 2001 From: Vinayak Kariappa Chettimada Date: Sun, 30 Mar 2025 06:50:53 +0200 Subject: [PATCH] Bluetooth: Controller: Fix Central CIS offset for dissimilar intervals Fix Central CIS offset calculation for dissimilar ACL and ISO intervals in use. Mayfly execution of `mfy_cig_offset_get()` could be after "LLL Prepare" or before depending on whether a previous radio event is being preempted or not, respectively; the `conn->lll.event_counter` may not be pre-incremented. This race condition is fixed by the fact that we use a constant instant delta value now. Dissimilar ACL and ISO intervals may lead to ACL overlapping or be too close to ISO event, causing preemption; under this case ACLs "LLL Prepare" would run after `mfy_cig_offset_get` causing incorrect calculation of CIS offset without the fix. Remove redundant `instant` member in `ll_conn_iso_stream` structure as a constant CIS Create instant delta is now used. Signed-off-by: Vinayak Kariappa Chettimada --- .../controller/ll_sw/ull_central_iso.c | 38 ++++++++----------- .../controller/ll_sw/ull_conn_iso_types.h | 1 - 2 files changed, 16 insertions(+), 23 deletions(-) diff --git a/subsys/bluetooth/controller/ll_sw/ull_central_iso.c b/subsys/bluetooth/controller/ll_sw/ull_central_iso.c index 0737a3e4890..5717c9adfd1 100644 --- a/subsys/bluetooth/controller/ll_sw/ull_central_iso.c +++ b/subsys/bluetooth/controller/ll_sw/ull_central_iso.c @@ -65,6 +65,14 @@ #define PHY_VALID_MASK (BT_HCI_ISO_PHY_VALID_MASK & ~BIT(2)) #endif +/* CIS Create Procedure uses 3 PDU transmissions, and one connection interval to process the LLCP + * requested, hence minimum relative instant not be less than 4. I.e. the CIS_REQ PDU will be + * transmitted in the next ACL interval. + * The +1 also helps with the fact that currently we do not have Central implementation to handle + * event latencies at the instant. Refer to `ull_conn_iso_start()` implementation. + */ +#define CIS_CREATE_INSTANT_DELTA_MIN 4U + #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); @@ -925,10 +933,10 @@ uint8_t ull_central_iso_setup(uint16_t cis_handle, cis->lll.prepared = 0U; #endif /* !CONFIG_BT_CTLR_JIT_SCHEDULING */ - cis->central.instant = instant; #if defined(CONFIG_BT_CTLR_ISOAL_PSN_IGNORE) cis->pkt_seq_num = 0U; #endif /* CONFIG_BT_CTLR_ISOAL_PSN_IGNORE */ + /* It is intentional to initialize to the 39 bit maximum value and rollover to 0 in the * prepare function, the event counter is pre-incremented in prepare function for the * current ISO event. @@ -975,19 +983,11 @@ int ull_central_iso_cis_offset_get(uint16_t cis_handle, conn = ll_conn_get(cis->lll.acl_handle); - /* NOTE: CIS Create Procedure uses 3 PDU transmissions, hence minimum relative instant not - * be less than 3. As the CIS_REQ PDU will be transmitted in the next ACL interval, - * add +1 to the instant. The +1 also helps with the fact that currently we do not - * have Central implementation to handle event latencies at the instant. Refer to - * `ull_conn_iso_start()` implementation. - * - * `ull_conn_llcp()` is called before `ull_ref_inc()` hence we do not need to use - * `ull_conn_event_counter()`. + /* `ull_conn_llcp()` (caller of this function) is called before `ull_ref_inc()` hence we do + * not need to use `ull_conn_event_counter()`. */ - cis->central.instant = conn->lll.event_counter + conn->lll.latency_prepare + - conn->llcp.prep.lazy + 4U; - - *conn_event_count = cis->central.instant; + *conn_event_count = conn->lll.event_counter + conn->lll.latency_prepare + + conn->llcp.prep.lazy + CIS_CREATE_INSTANT_DELTA_MIN; /* Provide CIS offset range * CIS_Offset_Max < (connInterval - (CIG_Sync_Delay + T_MSS)) @@ -1090,7 +1090,6 @@ static void cis_offset_get(struct ll_conn_iso_stream *cis) static void mfy_cis_offset_get(void *param) { uint32_t elapsed_acl_us, elapsed_cig_us; - uint16_t latency_acl, latency_cig; struct ll_conn_iso_stream *cis; struct ll_conn_iso_group *cig; uint32_t cig_remainder_us; @@ -1098,10 +1097,11 @@ static void mfy_cis_offset_get(void *param) uint32_t cig_interval_us; uint32_t offset_limit_us; uint32_t ticks_to_expire; + uint32_t remainder = 0U; uint32_t ticks_current; uint32_t offset_min_us; struct ll_conn *conn; - uint32_t remainder = 0U; + uint16_t latency_cig; uint8_t ticker_id; uint16_t lazy; uint8_t retry; @@ -1188,13 +1188,7 @@ static void mfy_cis_offset_get(void *param) * and latency counts (typically 3) is low enough to avoid 32-bit * overflow. Refer to ull_central_iso_cis_offset_get(). */ - /* FIXME: Mayfly execution of `mfy_cig_offset_get()` could be before "LLL Prepare" or after - * conn->lll.event_counter could have been pre-incremented. - * This race condition needs a fix. - */ - latency_acl = cis->central.instant - conn->lll.event_counter - conn->lll.latency_prepare - - conn->llcp.prep.lazy; - elapsed_acl_us = latency_acl * conn->lll.interval * CONN_INT_UNIT_US; + elapsed_acl_us = CIS_CREATE_INSTANT_DELTA_MIN * conn->lll.interval * CONN_INT_UNIT_US; /* Calculate elapsed CIG intervals until the instant */ cig_interval_us = cig->iso_interval * ISO_INT_UNIT_US; 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 39cee21d0b1..be9e8e56e5e 100644 --- a/subsys/bluetooth/controller/ll_sw/ull_conn_iso_types.h +++ b/subsys/bluetooth/controller/ll_sw/ull_conn_iso_types.h @@ -34,7 +34,6 @@ struct ll_conn_iso_stream { struct { uint8_t c_rtn; uint8_t p_rtn; - uint16_t instant; } central; };