Bluetooth: Controller: Advertising can only be started 2^16 times

[JordanYates]

Describe the bug
On Nordic devices, Bluetooth advertising can only be started ~2^16 times before all calls to bt_le_adv_start return -EIO.

rc = bt_le_adv_start(adv_param, adv_data, adv_data_len, scan_rsp_data, scan_rsp_len);
if (rc < 0) {
	LOG_ERR("Advertising failed to start: %d", rc);
	return;
}
k_sleep(K_MSEC(50));
rc = bt_le_adv_stop();
if (rc < 0) {
	LOG_ERR("Advertising failed to stop: %d", rc);
	return;
}

Will eventually result in :

[00:00:08.656,616] <wrn> bt_hci_core: opcode 0x200a status 0x03
[00:00:08.656,646] <err> bt_hci_core: Failed to start advertiser
[00:00:08.656,646] <err> app: Advertising failed to start: -5

The root cause of this issue is the failure to release onoff_manager dependencies in lll_clock.c
Each time advertising is started, we wait until the LF clock is ready:
ll_adv_enable -> lll_clock_wait -> blocking_on -> onoff_request

Each call to blocking_on adds an additional dependency via onoff_request that is never released with a call to onoff_release.
Eventually the refs counter will reach UINT16_MAX, which will cause onoff_request to always return -EAGAIN.

To Reproduce
Add the following to clock_ready in lll_clock.c to reduce reproduction time:
If the refs were being properly released, there is still a headroom of ~156 to allow for normal operation.

static bool first = true;
if (first) {
	printk("Overriding refs\n");
	mgr->refs = 65400;
	first = false;
}

Repeatedly call bt_le_adv_start and bt_le_adv_stop until -EIO is always observed.

Expected behavior
Advertising sets should be able to be started and stopped any number of times without errors.

Impact
This bug can only be recovered from by rebooting the device, as nothing will naturally reduce the reference count.
If a reboot is not triggered, the device can no longer be interacted with via Bluetooth.
For devices without physical access to the PCB, this bug bricks the device.

Logs and console output
Standard error messages are included above.

Environment (please complete the following information):

• Zephyr 2.4

[JordanYates]

As lll_clock provides no function which calls onoff_release, the same issue presumably affects connection creation as well, which calls lll_clock_wait in ll_create_connection.

[cvinayak]

The regression was introduced here: 2b47630#diff-4b2727dd1031ae4d5de482dc43a4c5d2295a89f718b04e5480f6c8f8082846b4

The following is missing from lll_clock_wait

	static bool done;

	if (done) {
		return 0;
	}
	done = true;

I will send a PR soon. Thanks for the issue report and detailed analysis.

[JordanYates]

static bool done;

if (done) {
return 0;
}
done = true;

While I imagine this would work in most real-world cases, it's not strictly correct.

1. A second thread calling lll_clock_wait would return immediately without the clock being ready.
2. If the first clock setup fails, future calls don't recognize that the clock is not running and return 0 immediately.

[cvinayak]

@JordanYates This is the LF clock which is also the core system clock, it is only started once on power up. The lll_clock_wait only needs to ensure the clock has settled, i.e. truely switched to crystal/RC source and stable. This clock is not to be stopped after power up.

A second thread calling lll_clock_wait would return immediately without the clock being ready.

By design, only one thread shall dispatch HCI cmd and data to the controller, and is a co-operative thread.

If the first clock setup fails, future calls don't recognize that the clock is not running and return 0 immediately.

Clock setup/settle failure is consider hardware fatal error, there is no expectation to succeed future calls, example, missing/damaged 32 KHz crystal on the board.