From e65be5841f5e6933bdf3f67aa7a27547748e0c6a Mon Sep 17 00:00:00 2001 From: Piotr Pryga Date: Mon, 16 May 2022 07:22:53 +0200 Subject: [PATCH] Bluetooth: Controller: Fix per sync cancel and sync establ synchronize Current implementation of ll_sync_create_cancel does not allow to stop synchronization after ull_sync_setup is called. When that is done, sync->timeout_reload is not zero and the ll_sync_create_cancel will return BT_HCI_ERR_CMD_DISALLOWED. That means the Controller is able to cancel periodic advertising synchronization only in period between call to ll_sync_create and reception of AUX_ADV_IND that has SyncInfo field. The Controller should be able to cancell synchronization until first AUX_SYNC_IND PDU is received and host notified about synchronization established. Complete information about synchronization status is provdied by two ll_sync_set members: node_rx_sync_established and timeout_reload. These two members of the structure were used in ll_sync_create_cancel function to do a proper cancel and cleanup. The node_rx_sync_established member was not cleared when sync was established or expired. That was required to get a proper information about synchronization state. Besides that, to avoid race condition between ll_sync_create_cancel and ull_sync_established_report, the latter function was extended to check if cancel operation or sync lost has happened. Signed-off-by: Piotr Pryga --- subsys/bluetooth/controller/ll_sw/ull_sync.c | 66 +++++++++++++++++-- .../controller/ll_sw/ull_sync_types.h | 12 +++- 2 files changed, 69 insertions(+), 9 deletions(-) diff --git a/subsys/bluetooth/controller/ll_sw/ull_sync.c b/subsys/bluetooth/controller/ll_sw/ull_sync.c index 057c61b824623..0fade581936bf 100644 --- a/subsys/bluetooth/controller/ll_sw/ull_sync.c +++ b/subsys/bluetooth/controller/ll_sw/ull_sync.c @@ -182,9 +182,14 @@ uint8_t ll_sync_create(uint8_t options, uint8_t sid, uint8_t adv_addr_type, /* Initialize sync context */ node_rx->link = link_sync_estab; - sync->node_rx_sync_estab = node_rx; sync->node_rx_lost.hdr.link = link_sync_lost; + /* Make sure that the node_rx_sync_establ hasn't got anything assigned. It is used to + * mark when sync establishment is in progress. + */ + LL_ASSERT(!sync->node_rx_sync_estab); + sync->node_rx_sync_estab = node_rx; + /* Reporting initially enabled/disabled */ sync->rx_enable = !(options & BT_HCI_LE_PER_ADV_CREATE_SYNC_FP_REPORTS_DISABLED); @@ -301,16 +306,43 @@ uint8_t ll_sync_create_cancel(void **rx) } cpu_dmb(); sync = scan->periodic.sync; - if (!sync || sync->timeout_reload) { - /* FIXME: sync establishment in progress looking for first - * AUX_SYNC_IND. Cleanup by stopping ticker and disabling - * LLL events. - */ + if (!sync) { return BT_HCI_ERR_CMD_DISALLOWED; } + /* node_rx_sync_estab is assigned when Host calls create sync and cleared when sync is + * established. timeout_reload is set when sync is found and setup. It is non-zero until + * sync is terminated. Together they give information about current sync state: + * - node_rx_sync_estab == NULL && timeout_reload != 0 => sync is established + * - node_rx_sync_estab == NULL && timeout_reload == 0 => sync is terminated + * - node_rx_sync_estab != NULL && timeout_reload == 0 => sync is created + * - node_rx_sync_estab != NULL && timeout_reload != 0 => sync is waiting to be established + */ + if (!sync->node_rx_sync_estab) { + /* There is no sync to be cancelled */ + return BT_HCI_ERR_CMD_DISALLOWED; + } + + sync->is_stop = 1U; + cpu_dmb(); + + if (sync->timeout_reload != 0) { + uint16_t sync_handle = ull_sync_handle_get(sync); + + LL_ASSERT(sync_handle <= UINT8_MAX); + + /* Sync is not established yet, so stop sync ticker */ + int err = ull_ticker_stop_with_mark( + TICKER_ID_SCAN_SYNC_BASE + (uint8_t)sync_handle, sync, &sync->lll); + + LL_ASSERT(err == 0 || err == -EALREADY); + if (err != 0 && err != -EALREADY) { + return BT_HCI_ERR_CMD_DISALLOWED; + } + } /* else: sync was created but not yet setup, there is no sync ticker yet. */ + /* It is safe to remove association with scanner as cancelled flag is - * set and sync has not been established. + * set, sync is_stop flag was set and sync has not been established. */ ull_sync_setup_reset(scan); @@ -327,6 +359,11 @@ uint8_t ll_sync_create_cancel(void **rx) ll_rx_link_release(link_sync_estab); ll_rx_release(node_rx); + /* Clear the node after release to mark the sync establish as being completed. + * In this case the completion reason is sync cancelled by Host. + */ + sync->node_rx_sync_estab = NULL; + node_rx = (void *)&sync->node_rx_lost; node_rx->hdr.type = NODE_RX_TYPE_SYNC; node_rx->hdr.handle = LLL_HANDLE_INVALID; @@ -792,6 +829,11 @@ void ull_sync_established_report(memq_link_t *link, struct node_rx_hdr *rx) lll = ftr->param; sync = HDR_LLL2ULL(lll); + /* Do nothing if sync is cancelled or lost. */ + if (unlikely(sync->is_stop || !sync->timeout_reload)) { + return; + } + #if defined(CONFIG_BT_CTLR_SYNC_PERIODIC_CTE_TYPE_FILTERING) enum sync_status sync_status; @@ -832,6 +874,11 @@ void ull_sync_established_report(memq_link_t *link, struct node_rx_hdr *rx) rx_establ->hdr.type = NODE_RX_TYPE_SYNC; rx_establ->hdr.handle = ull_sync_handle_get(sync); se = (void *)rx_establ->pdu; + /* Clear the node to mark the sync establish as being completed. + * In this case the completion reason is sync being established. + */ + sync->node_rx_sync_estab = NULL; + #if defined(CONFIG_BT_CTLR_SYNC_PERIODIC_CTE_TYPE_FILTERING) se->status = (ftr->sync_status == SYNC_STAT_TERM) ? BT_HCI_ERR_UNSUPP_REMOTE_FEATURE : @@ -1242,6 +1289,11 @@ static void sync_expire(void *param) rx->hdr.type = NODE_RX_TYPE_SYNC; rx->hdr.handle = LLL_HANDLE_INVALID; + /* Clear the node to mark the sync establish as being completed. + * In this case the completion reason is sync expire. + */ + sync->node_rx_sync_estab = NULL; + /* NOTE: struct node_rx_sync_estab has uint8_t member following the * struct node_rx_hdr to store the reason. */ diff --git a/subsys/bluetooth/controller/ll_sw/ull_sync_types.h b/subsys/bluetooth/controller/ll_sw/ull_sync_types.h index 8b2288c45b896..0c1b71aacaa6c 100644 --- a/subsys/bluetooth/controller/ll_sw/ull_sync_types.h +++ b/subsys/bluetooth/controller/ll_sw/ull_sync_types.h @@ -18,7 +18,12 @@ struct ll_sync_set { uint16_t skip; uint16_t timeout; - uint16_t volatile timeout_reload; /* Non-zero when sync established */ + /* Non-zero when sync is setup. It can be in two sub-stated: + * - Waiting for first AUX_SYNC_IND, before sync established was notified to Host. + * If sync establishment is in progress node_rx_sync_estab is not NULL. + * - sync is already established, node_rx_sync_estab is NULL. + */ + uint16_t volatile timeout_reload; uint16_t timeout_expire; /* Member to store periodic advertising sync prepare. @@ -49,7 +54,7 @@ struct ll_sync_set { uint8_t is_term:1; #endif /* CONFIG_BT_CTLR_SYNC_PERIODIC_CTE_TYPE_FILTERING && !CONFIG_BT_CTLR_CTEINLINE_SUPPORT */ - uint8_t is_stop:1; /* sync terminate requested */ + uint8_t is_stop:1; /* sync terminate or cancel requested */ uint8_t sync_expire:3; /* countdown of 6 before fail to establish */ #if defined(CONFIG_BT_CTLR_CHECK_SAME_PEER_SYNC) @@ -68,6 +73,9 @@ struct ll_sync_set { }; } node_rx_lost; + /* Not-Null when sync was setup and Controller is waiting for first AUX_SYNC_IND PDU. + * It means the sync was not estalished yet. + */ struct node_rx_hdr *node_rx_sync_estab; #if defined(CONFIG_BT_CTLR_SYNC_ISO)