Bluetooth: Controller: Fix missing connection handle invalidate

[cvinayak]

Fix missing connection handle invalidate on Controller power up.

The connection context are zero-initialized on startup and calls to ll_connected_get() would incorrectly return a valid connection context pointer for connection handle 0.

[Copilot]

Pull Request Overview

This PR fixes an issue where connection handles were not properly invalidated on Controller power up, preventing an erroneous valid connection context for handle 0. Key changes include:

• Stopping active tickers related to connection roles before reset.
• Shifting connection handle invalidation to init_reset().
• Removing redundant invalidation from the disable() function.

[Copilot]

Pull Request Overview

This PR addresses an issue where connection handle 0 was not properly invalidated on power up, which caused ll_connected_get() to return an incorrect valid pointer. Key changes include adding a ticker stop routine, explicitly invalidating connection handles in init_reset, and removing duplicate invalidation logic from disable.

Comments suppressed due to low confidence (2)

subsys/bluetooth/controller/ll_sw/ull_conn.c:1672

• Ensure that ll_conn_get(handle) always returns a valid pointer or add an assertion to prevent potential null dereference if future changes alter connection pool initialization.

for (uint16_t handle = 0U; handle < CONFIG_BT_MAX_CONN; handle++) {

subsys/bluetooth/controller/ll_sw/ull_conn.c:1676

• [nitpick] With the invalidation now performed in init_reset, please confirm that removing it from disable does not result in inconsistent states when disable is invoked later in the connection lifecycle.

conn->lll.handle = LLL_HANDLE_INVALID;

[cvinayak]

Updated commit message to refer to cvinayak@fa02dc4

[Copilot]

Pull Request Overview

This PR ensures connection contexts are properly invalidated on power-up to prevent stale handles (especially handle 0) from appearing valid.

• Add range checks in ll_conn_get and ll_connected_get to return NULL for out-of-range or uninitialized handles
• Initialize all connection handles to LLL_HANDLE_INVALID in init_reset
• Sprinkle LL_ASSERT(conn) after handle lookups to catch invalid contexts early

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
subsys/bluetooth/controller/ll_sw/ull_peripheral_iso.c	Added LL_ASSERT(conn) to guard against NULL
subsys/bluetooth/controller/ll_sw/ull_iso.c	Added LL_ASSERT(conn) in resume ticker start
subsys/bluetooth/controller/ll_sw/ull_conn_iso.c	Added LL_ASSERT(conn) in multiple callbacks
subsys/bluetooth/controller/ll_sw/ull_conn.c	Added range check in ll_conn_get/ll_connected_get, invalidation loop in init_reset
subsys/bluetooth/controller/ll_sw/ull_central_iso.c	Added LL_ASSERT(conn) after ll_conn_get calls
subsys/bluetooth/controller/ll_sw/ull.c	Added LL_ASSERT(conn) in RX release and demux
subsys/bluetooth/controller/ll_sw/nordic/lll/lll_peripheral_iso.c	Added LL_ASSERT(conn_lll) guards in LLL prepare and ISR
subsys/bluetooth/controller/ll_sw/nordic/lll/lll_central_iso.c	Added LL_ASSERT(conn_lll) guards in LLL callbacks and ISRs

Comments suppressed due to low confidence (2)

subsys/bluetooth/controller/ll_sw/ull_conn.c:1571

• The assertion LL_ASSERT(link->next) is redundant here since this branch is only entered when link->next == (void *)tx, which cannot be NULL. Consider removing the assert or adjusting it to capture a more meaningful invariant.

LL_ASSERT(link->next);

subsys/bluetooth/controller/ll_sw/ull_conn.c:1864

• The code removed resetting conn->lll.handle in disable(), so after disabling a connection the handle field remains valid and ll_connected_get() may return a stale context. Reintroduce invalidation of conn->lll.handle here if disabling should fully tear down the connection state.

conn->lll.link_tx_free = NULL;

[Thalley]

Suggested change

	if (!conn || conn->lll.handle != handle) {
	if (conn == NULL || conn->lll.handle != handle) {

[Thalley]

Suggested change

	if (!conn) {
	if (conn == NULL) {

[Thalley]

Suggested change

	LL_ASSERT(conn_lll);
	LL_ASSERT(conn_lll != NULL);

Applies many places

[cvinayak]

is this new requirement? Most Bluetooth subsystem code uses !buf style. Shouldn't existing code be updated to be consistent?

[Thalley]

It's been in our coding guidelines for quite a while actually: Rule 85 of https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html: https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_14_04.c

[cvinayak]

I added a commit and a revert commit as there is code size increase in use of explicit comparison.

The assertion check is correct to have, but the explicit checks seems an overhead due to coding style (both Bluetooth subsystem and few kernel code break this style anyway).

I have come to realization that these invalid checks like LL_ASSERT; and explicit comparisons total ~15 KB of code size on Cortex-M CPUs in the Controller hci_uart builds!

Reference exercise: #90599

[Thalley]

That's surprising :o Do you know why changing from e.g. conn_lll) to conn_lll != NULL increase the code size?

[cvinayak]

Apparently, on investigation, the assert string includes the conditional as string! ... i will put back the MISRA coding style.

[Thalley]

Will approve for now to get this fix in. The code size and coding guidelines issues can be further investigated outside this PR

[cvinayak]

Will approve for now to get this fix in. The code size and coding guidelines issues can be further investigated outside this PR

On some more investigation, the code size increase is due to the assert string including the conditional (not due to explicit comparison!) . I will go back to using MISRA coding guidelines in this PR. The code size increase is acceptable.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
15.2% Duplication on New Code

See analysis details on SonarQube Cloud

[Copilot]

Pull Request Overview

This PR addresses an issue where a connection context for handle 0 was incorrectly considered valid by ensuring proper invalidation of connection handles and adding runtime assertions. Key changes include:

• Adding LL_ASSERT checks after retrieving connection contexts.
• Modifying ll_conn_get() and ll_connected_get() to better validate connection handles.
• Introducing connection handle invalidation during initialization.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
subsys/bluetooth/controller/ll_sw/ull_peripheral_iso.c	Added assertion to confirm non-NULL connection after retrieval from ll_conn_get().
subsys/bluetooth/controller/ll_sw/ull_iso.c	Added similar LL_ASSERT checks for connection context retrieval.
subsys/bluetooth/controller/ll_sw/ull_conn_iso.c	Introduced LL_ASSERT checks to ensure valid connection context before operating on it.
subsys/bluetooth/controller/ll_sw/ull_conn.c	Updated ll_conn_get() and ll_connected_get(); moved connection handle invalidation to init_reset.
subsys/bluetooth/controller/ll_sw/ull_central_iso.c	Added assertions confirming non-NULL connection contexts in various CIS-related functions.
subsys/bluetooth/controller/ll_sw/ull.c	Added LL_ASSERT checks for connection usage in RX memory release and demux functions.
subsys/bluetooth/controller/ll_sw/nordic/lll_peripheral_iso.c	Added assertions ensuring valid ACL context retrieval.
subsys/bluetooth/controller/ll_sw/nordic/lll_central_iso.c	Added additional LL_ASSERT checks to validate ACL context retrieval.

Comments suppressed due to low confidence (1)

subsys/bluetooth/controller/ll_sw/ull_conn.c:1692

• The removal of direct connection handle invalidation in the disable() function may lead to cases where a stale connection handle is still referenced; please confirm that moving invalidation to init_reset() covers all necessary scenarios.

conn->lll.handle = LLL_HANDLE_INVALID;