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 <[email protected]>

Author: ppryga-nordic

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.
 	 */

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)