Bluetooth: Controller: build assert if comand buffer is too small #19869

KyraLengfeld · 2025-01-13T11:26:46Z

Host Number of Completed Packets command does not follow normal flow control of HCI commands and the Controller side HCI drivers that allocates HCI command buffers with K_NO_WAIT can end up running out of command buffers.

Host will generate up to acl_pkts number of Host Number of Completed Packets command plus a number of normal HCI commands.

Normal HCI commands follow the HCI command flow control using Num_HCI_Command_Packets return in HCI command complete and status.

When Controller to Host data flow control is supported, this commit ensures that BT_BUF_CMD_TX_COUNT is greater than or equal to (BT_BUF_ACL_RX_COUNT + Ncmd), where Ncmd is the supported maximum Num_HCI_Command_Packets.

Note: The SDC controller (currently) does not support Num_HCI_Command_Packets > 1, which means Ncmd is always 1.

NordicBuilder · 2025-01-13T11:30:19Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 9

Inputs:

Sources:

sdk-nrf: PR head: eb14af132e9e5cf396988e27634cfd4c7e9700f7

more details

sdk-nrf:

PR head: eb14af132e9e5cf396988e27634cfd4c7e9700f7
merge base: 7f1e6c6122bf650942f8c2201185c583c77a2ce9
target head (main): 233a9e7006af441e368a7ea947f33e5ddeb898e5
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (12)

doc
│  ├── nrf
│  │  ├── releases_and_maturity
│  │  │  │ known_issues.rst
scripts
│  ├── ci
│  │  │ tags.yaml
subsys
│  ├── bluetooth
│  │  ├── controller
│  │  │  ├── CMakeLists.txt
│  │  │  ├── hci_internal.c
│  │  │  ├── hci_internal_wrappers.c
│  │  │  │ hci_internal_wrappers.h
tests
│  ├── subsys
│  │  ├── bluetooth
│  │  │  ├── controller
│  │  │  │  ├── CMakeLists.txt
│  │  │  │  ├── prj.conf
│  │  │  │  ├── src
│  │  │  │  │  │ hci_cmd_cb_host_buffer_size_test.c
│  │  │  │  │ testcase.yaml
│  │  │  ├── enocean
│  │  │  │  │ testcase.yaml
│  │  ├── mpsl
│  │  │  ├── pm
│  │  │  │  │ testcase.yaml

Outputs:

Toolchain

Version: 342151af73
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:342151af73_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
❌ Build twister
❌ Integration tests
- ❌ desktop52_verification
- ❌ test_ble_nrf_config
- ❌ test-fw-nrfconnect-ble_samples
- ❌ test-fw-nrfconnect-chip
- ❌ test-fw-nrfconnect-zigbee
- ❌ test-sdk-find-my
- ❌ test-sdk-sidewalk

Disabled integration tests

- doc-internal
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-boot
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_libmodem-nrf
- test-fw-nrfconnect-nrf-iot_lwm2m
- test-fw-nrfconnect-nrf-iot_mosh
- test-fw-nrfconnect-nrf-iot_positioning
- test-fw-nrfconnect-nrf-iot_samples
- test-fw-nrfconnect-nrf-iot_serial_lte_modem
- test-fw-nrfconnect-nrf-iot_thingy91
- test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- test-fw-nrfconnect-nrf_crypto
- test-fw-nrfconnect-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-thread
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-mcuboot
- test-sdk-pmic-samples
- test-sdk-wifi
- test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

NordicBuilder · 2025-01-13T11:45:33Z

You can find the documentation preview for this PR at this link.

Note: This comment is automatically posted by the Documentation Publish GitHub Action.

doc/nrf/releases_and_maturity/known_issues.rst

cvinayak · 2025-01-14T12:26:33Z

subsys/bluetooth/controller/hci_driver.c

+   * The SDC controller (currently) does not support Num_HCI_Command_Packets > 1, which means Ncmd
+   * is always 1.
+   */
+BUILD_ASSERT(CONFIG_BT_BUF_ACL_RX_COUNT < CONFIG_BT_BUF_CMD_TX_COUNT,


Is CONFIG_BT_BUF_ACL_RX_COUNT the maximum number of HCI ACL packets that can be generated? is this the value used to configure the SoftDevice Controller?

git grep BT_BUF_ACL_RX_COUNT gives me no reference that convince me that it is used... hmmm?

It does not give me a reference neither when grepping for it, but that is the critical config (or rather the command buffer size aligned with this), when customers are making samples. Checked that the customer sample we were looking at runs into the assert.
I couldn't really find which other configs to base such build assert on otherwise. Do you have some other check in mind? (Can also discuss offline, if you want. :) )

@cvinayak I've added the runtime modification as we discussed personally. Could you take a look whether the functions name and variable names are good (I tend to not be good at choosing them)? And if the wrapper function should be placed as I did? Also not sure if I can improve the comment above it.

seko-nordic · 2025-01-17T15:39:47Z

subsys/bluetooth/controller/hci_internal.c

+ * approach is to align the communicated host RX Buffer Count to it here.
+ */
+#if defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL)
+static int sdc_hci_cmd_cb_host_buffer_size_wrapper(void *cmd_params)


Suggested change

static int sdc_hci_cmd_cb_host_buffer_size_wrapper(void *cmd_params)

static int sdc_hci_cmd_cb_host_buffer_size_wrapper(const sdc_hci_cmd_cb_host_buffer_size_t *cmd_params)

seko-nordic · 2025-01-17T15:40:34Z

subsys/bluetooth/controller/hci_internal.c

+	sdc_hci_cmd_cb_host_buffer_size_t ctrl_cmd_params =
+		*(const sdc_hci_cmd_cb_host_buffer_size_t *)cmd_params;


Suggested change

sdc_hci_cmd_cb_host_buffer_size_t ctrl_cmd_params =

*(const sdc_hci_cmd_cb_host_buffer_size_t *)cmd_params;

sdc_hci_cmd_cb_host_buffer_size_t ctrl_cmd_params = *cmd_params;

seko-nordic · 2025-01-17T15:41:04Z

subsys/bluetooth/controller/hci_internal.c

+	ctrl_cmd_params.host_total_num_acl_data_packets = MIN(
+		ctrl_cmd_params.host_total_num_acl_data_packets, (CONFIG_BT_BUF_CMD_TX_COUNT - 1));
+
+	return sdc_hci_cmd_cb_host_buffer_size((const void *)&ctrl_cmd_params);


Suggested change

return sdc_hci_cmd_cb_host_buffer_size((const void *)&ctrl_cmd_params);

return sdc_hci_cmd_cb_host_buffer_size(&ctrl_cmd_params);

seko-nordic · 2025-01-17T15:41:48Z

subsys/bluetooth/controller/hci_driver.c

+ * The SDC controller (currently) does not support Num_HCI_Command_Packets > 1, which means Ncmd
+ * is always 1.
+ */
+BUILD_ASSERT(BT_BUF_ACL_RX_COUNT < CONFIG_BT_BUF_CMD_TX_COUNT,


let's move it inside sdc_hci_cmd_cb_host_buffer_size_wrapper, so it all in one place

I realized that, when building Controller-only builds BT_BUF_ACL_RX_COUNT is always 0! Hence, need to use a count that is allocated in the Controller?

https://github.com/nrfconnect/sdk-zephyr/blob/9471a136b58bfa6853b4854308ca4429ba670934/include/zephyr/bluetooth/buf.h#L91-L107

It's not important how many buffers are allocated inside controller, it still can send out more RX packets than that, if host delays HCI_Host_Number_Of_Completed_Packets. What is important is how many the transport can accept from the controller? Or if transport does not buffer, but just pass-through then checking against 0 here should be fine since it always pass?

cvinayak · 2025-01-20T07:06:43Z

subsys/bluetooth/controller/hci_driver.c

+ * The SDC controller (currently) does not support Num_HCI_Command_Packets > 1, which means Ncmd
+ * is always 1.
+ */
+BUILD_ASSERT(BT_BUF_ACL_RX_COUNT < CONFIG_BT_BUF_CMD_TX_COUNT,


I realized that, when building Controller-only builds BT_BUF_ACL_RX_COUNT is always 0! Hence, need to use a count that is allocated in the Controller?

https://github.com/nrfconnect/sdk-zephyr/blob/9471a136b58bfa6853b4854308ca4429ba670934/include/zephyr/bluetooth/buf.h#L91-L107

seko-nordic · 2025-01-21T16:40:42Z

subsys/bluetooth/controller/hci_driver.c

+// not BT_BUF_ACL_RX_COUNT, but what is set in controller.
+BUILD_ASSERT(CONFIG_BT_BUF_CMD_TX_COUNT > 2,
+	     "We need at least two HCI command buffers to avoid deadlocks.");


Suggested change

// not BT_BUF_ACL_RX_COUNT, but what is set in controller.

BUILD_ASSERT(CONFIG_BT_BUF_CMD_TX_COUNT > 2,

"We need at least two HCI command buffers to avoid deadlocks.");

BUILD_ASSERT(CONFIG_BT_BUF_CMD_TX_COUNT - 1 > 0,

"We need at least two HCI command buffers to avoid deadlocks.");

it's not about controller, right? It's that adjustment that should not become 0 https://github.com/nrfconnect/sdk-nrf/pull/19869/files#diff-813ccd03aac379c4e1f534c16d65b666f48162012fd09e35a1f1ddcf69d88511R22

Also I would still like to place both static assert and dynamic adjustment at the same place in the code. It's harder to find pieces and understated them when they scattered.

Host Number of Completed Packets command does not follow normal flow control of HCI commands and the Controller side HCI drivers that allocates HCI command buffers with K_NO_WAIT can end up running out of command buffers. Host will generate up to acl_pkts number of Host Number of Completed Packets command plus a number of normal HCI commands. Normal HCI commands follow the HCI command flow control using Num_HCI_Command_Packets return in HCI command complete and status. When Controller to Host data flow control is supported, this commit ensures that BT_BUF_CMD_TX_COUNT is greater than or equal to (BT_BUF_ACL_RX_COUNT + Ncmd), where Ncmd is the supported maximum Num_HCI_Command_Packets, for a controller + host, or host-only build. Furthermore this commit restricts the `host_total_num_acl_data_packets` communicated to the controller to how many acknoledgements the controller is able to receive from the host, in a controller-only build. Note: The SDC controller (currently) does not support Num_HCI_Command_Packets > 1, which means Ncmd is always 1. Signed-off-by: Kyra Lengfeld <[email protected]>

This commit extracts the wrapper function in its own file, to avoid bloating the hci_internal file, as well as tests its funcionality in a unit test. Signed-off-by: Kyra Lengfeld <[email protected]>

nordic-piks

Looking at test tags and scripts/ci/tags.yaml, it look good.

github-actions bot added doc-required PR must not be merged without tech writer approval. changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. labels Jan 13, 2025

KyraLengfeld force-pushed the cmd_buffer_workaround branch from 7e4cf76 to 7dd9904 Compare January 13, 2025 12:37

KyraLengfeld marked this pull request as ready for review January 13, 2025 12:37

KyraLengfeld requested review from a team as code owners January 13, 2025 12:37

KyraLengfeld requested a review from cvinayak January 13, 2025 12:37

peknis requested changes Jan 13, 2025

View reviewed changes

rugeGerritsen reviewed Jan 13, 2025

View reviewed changes

doc/nrf/releases_and_maturity/known_issues.rst Outdated Show resolved Hide resolved

KyraLengfeld force-pushed the cmd_buffer_workaround branch 2 times, most recently from e9664c2 to 5fabd4a Compare January 13, 2025 16:25

peknis requested changes Jan 14, 2025

View reviewed changes

doc/nrf/releases_and_maturity/known_issues.rst Outdated Show resolved Hide resolved

doc/nrf/releases_and_maturity/known_issues.rst Outdated Show resolved Hide resolved

doc/nrf/releases_and_maturity/known_issues.rst Outdated Show resolved Hide resolved

KyraLengfeld force-pushed the cmd_buffer_workaround branch from 5fabd4a to b1c56cf Compare January 14, 2025 08:53

peknis approved these changes Jan 14, 2025

View reviewed changes

cvinayak reviewed Jan 14, 2025

View reviewed changes

KyraLengfeld force-pushed the cmd_buffer_workaround branch 2 times, most recently from 0459e3f to c1ceef7 Compare January 17, 2025 15:09

seko-nordic reviewed Jan 17, 2025

View reviewed changes

cvinayak requested changes Jan 20, 2025

View reviewed changes

KyraLengfeld force-pushed the cmd_buffer_workaround branch from c1ceef7 to eba935e Compare January 21, 2025 16:28

KyraLengfeld requested review from a team, nordic-piks, PerMac and katgiadla as code owners January 21, 2025 16:28

KyraLengfeld requested a review from a team as a code owner January 21, 2025 16:28

seko-nordic requested changes Jan 21, 2025

View reviewed changes

KyraLengfeld added 2 commits January 21, 2025 18:32

Bluetooth: Controller: add Unit tests for hci_internal wrapper

eb14af1

This commit extracts the wrapper function in its own file, to avoid bloating the hci_internal file, as well as tests its funcionality in a unit test. Signed-off-by: Kyra Lengfeld <[email protected]>

KyraLengfeld force-pushed the cmd_buffer_workaround branch from eba935e to eb14af1 Compare January 21, 2025 17:50

nordic-piks approved these changes Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: Controller: build assert if comand buffer is too small #19869

Bluetooth: Controller: build assert if comand buffer is too small #19869

KyraLengfeld commented Jan 13, 2025

NordicBuilder commented Jan 13, 2025 •

edited

Loading

NordicBuilder commented Jan 13, 2025

cvinayak Jan 14, 2025

KyraLengfeld Jan 14, 2025

KyraLengfeld Jan 17, 2025 •

edited

Loading

seko-nordic Jan 17, 2025

seko-nordic Jan 17, 2025

seko-nordic Jan 17, 2025

seko-nordic Jan 17, 2025

cvinayak Jan 20, 2025

seko-nordic Jan 20, 2025

cvinayak Jan 20, 2025

seko-nordic Jan 21, 2025

nordic-piks left a comment

	static int sdc_hci_cmd_cb_host_buffer_size_wrapper(void *cmd_params)
	static int sdc_hci_cmd_cb_host_buffer_size_wrapper(const sdc_hci_cmd_cb_host_buffer_size_t *cmd_params)

		sdc_hci_cmd_cb_host_buffer_size_t ctrl_cmd_params =
		(const sdc_hci_cmd_cb_host_buffer_size_t )cmd_params;

	return sdc_hci_cmd_cb_host_buffer_size((const void *)&ctrl_cmd_params);
	return sdc_hci_cmd_cb_host_buffer_size(&ctrl_cmd_params);

Bluetooth: Controller: build assert if comand buffer is too small #19869

Are you sure you want to change the base?

Bluetooth: Controller: build assert if comand buffer is too small #19869

Conversation

KyraLengfeld commented Jan 13, 2025

NordicBuilder commented Jan 13, 2025 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

NordicBuilder commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KyraLengfeld Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordic-piks left a comment

Choose a reason for hiding this comment

NordicBuilder commented Jan 13, 2025 •

edited

Loading

KyraLengfeld Jan 17, 2025 •

edited

Loading