bluetooth: controller assertion when scanning with multiple active connections

[LeBlue]

Describe the bug

With the hci_usb sample, if more than one peripheral is connected, enableing scanning hits always the following assert:

ASSERTION FAIL [(ret == 0) || (ret == 2)] @ WEST_TOPDIR/zephyr/subsys/bluetooth/controller/ll_sw/nordic/lll/lll.c:714

The value is ret == 1 == TICKER_STATUS_FAILURE

zephyr/subsys/bluetooth/controller/ll_sw/nordic/lll/lll.c

Lines 713 to 715 in c71e2dc

	ret = preempt_ticker_start(p, ticker_start_next_op_cb);
	LL_ASSERT((ret == TICKER_STATUS_SUCCESS) ||
	(ret == TICKER_STATUS_BUSY));

To Reproduce
Steps to reproduce the behavior:

1. build the hci_usb sample with increased allowed connections CONFIG_BT_MAX_CONN=5
2. connect two peripherals
3. enable scanning
4. See assertion

What have you tried to diagnose or workaround this issue?

@cvinayak Looking through the history, the issue seems related to commit 8edc998

Impact
showstopper

Environment (please complete the following information):

• Commit SHA or Version used: v2.6.0-rc1 and main@c71e2dc0077

Additional context
Reverting 8edc998 with the following (plain revert had conflicts), seems to work. But this just ignores the TICKER_STATUS_FAILURE return value. Additionally, my conflict resolution may not be correct.

diff --git a/subsys/bluetooth/controller/ll_sw/nordic/lll/lll.c b/subsys/bluetooth/controller/ll_sw/nordic/lll/lll.c
index 84dd4d7ae0..e671710800 100644
--- a/subsys/bluetooth/controller/ll_sw/nordic/lll/lll.c
+++ b/subsys/bluetooth/controller/ll_sw/nordic/lll/lll.c
@@ -700,19 +700,6 @@ int lll_prepare_resolve(lll_is_abort_cb_t is_abort_cb, lll_abort_cb_t abort_cb,
 	LL_ASSERT((ret == TICKER_STATUS_SUCCESS) ||
 		  (ret == TICKER_STATUS_FAILURE) ||
 		  (ret == TICKER_STATUS_BUSY));
-
-	/* Find next prepare needing preempt timeout to be setup */
-	do {
-		p = ull_prepare_dequeue_iter(&idx);
-		if (!p) {
-			return err;
-		}
-	} while (p->is_aborted || p->is_resume);
-
-	/* Start the preempt timeout */
-	ret = preempt_ticker_start(p, ticker_start_next_op_cb);
-	LL_ASSERT((ret == TICKER_STATUS_SUCCESS) ||
-		  (ret == TICKER_STATUS_BUSY));
 #endif /* !CONFIG_BT_CTLR_LOW_LAT */

 	return err;
@@ -861,7 +848,7 @@ static void preempt(void *param)
 		next->is_aborted = 1;
 		next->abort_cb(&next->prepare_param, next->prepare_param.param);

-		return;
+		goto preempt_next;
 	}

 	/* Abort the current event */
@@ -900,6 +887,16 @@ static void preempt(void *param)
 	} else {
 		LL_ASSERT(err == -ECANCELED);
 	}
+
+preempt_next:
+	do {
+		next = ull_prepare_dequeue_iter(&idx);
+		if (!next) {
+			return;
+		}
+	} while (next->is_aborted || next->is_resume);
+
+	preempt_ticker_start(next, ticker_start_op_cb); // using ticker_start_next_op_cb also asserts
 }
 #else /* CONFIG_BT_CTLR_LOW_LAT */

[cvinayak]

Thank you for reporting the bug.

But this just ignores the TICKER_STATUS_FAILURE return value.

This is how it was, before the offending commit. To avoid increased ticker operation structure memory allocations that is required (upto the amount of elements that could get enqueued in the prepare pipeline queue) to successfully enqueue ticker_start and then check for failure to start in callback is unnecessary hence for now ignoring the return value is good.

In the future, I will try adding better check in a form of binary semaphore implementation that optimize out ticker_start from being called if it is already started.

For now, please review the fix PR #35478 and provide your approval.