Bluetooth: Controller: Ensure controller can handle flow control buffers

The host will generate up to acl_pkts number of Host Number of Completed
Packets plus a number of normal HCI commands, which use
Num_HCI_Command_Packets.
As such, if we do not provide enough (host) TX CMD buffers for the
possible number of acl_pkts plus Num_HCI_Command_Packets, we may run
into a deadlock.

Currently the SDC controller delays sending disconnect events until it
received all the ACKs for the ACL data packets sent to the host to avoid
race conditions.
Which, in case there aren't enough (host) TX CMD buffers, can result in
a deadlock visible by a missing disconnect event.

When Controller to Host data flow control is supported, this commit
ensures that the `CONFIG_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. Ncmd is always 1.

Furthermore this commit restricts the `host_total_num_acl_data_packets`
communicated to the controller to how many acknowledgements + Ncmd the
controller is able to receive from the host. This is especially valuable
in a controller-only build where it cannot be foreseen what settings are
used for the host.

Signed-off-by: Kyra Lengfeld <[email protected]>
(cherry picked from commit 7766183)
Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Author: KyraLengfeld

CODEOWNERS

@@ -311,6 +311,7 @@ Kconfig*                                  @tejlmand
 /tests/modules/mcuboot/external_flash/    @hakonfam @sigvartmh
 /tests/nrf5340_audio/                     @koffes @alexsven @erikrobstad @rick1082 @nordic-auko
 /tests/subsys/audio_module/               @koffes @alexsven @erikrobstad @rick1082 @gWacey
+/tests/subsys/bluetooth/controller/        @nrfconnect/ncs-dragoon
 /tests/subsys/bluetooth/gatt_dm/          @doki-nordic
 /tests/subsys/bluetooth/mesh/             @ludvigsj
 /tests/subsys/bluetooth/fast_pair/        @alstrzebonski @MarekPieta @kapi-no

boards/arm/thingy91_nrf52840/thingy91_nrf52840_defconfig

@@ -15,3 +15,6 @@ CONFIG_SERIAL=y
 CONFIG_CONSOLE=y
 
 CONFIG_BOOTLOADER_MCUBOOT=y
+
+# BT
+CONFIG_BT_BUF_CMD_TX_COUNT=3

subsys/bluetooth/controller/CMakeLists.txt

@@ -9,6 +9,7 @@ zephyr_library()
 zephyr_library_sources(
   hci_driver.c
   hci_internal.c
+  hci_internal_wrappers.c
 )
 
 zephyr_library_sources_ifdef(

subsys/bluetooth/controller/hci_internal.c

@@ -16,6 +16,7 @@
 #include <sdc_hci_vs.h>
 
 #include "hci_internal.h"
+#include "hci_internal_wrappers.h"
 #include "ecdh.h"
 
 #define CMD_COMPLETE_MIN_SIZE (BT_HCI_EVT_HDR_SIZE \
@@ -892,7 +893,7 @@ static uint8_t controller_and_baseband_cmd_put(uint8_t const * const cmd,
 	case SDC_HCI_OPCODE_CMD_CB_SET_CONTROLLER_TO_HOST_FLOW_CONTROL:
 		return sdc_hci_cmd_cb_set_controller_to_host_flow_control((void *)cmd_params);
 	case SDC_HCI_OPCODE_CMD_CB_HOST_BUFFER_SIZE:
-		return sdc_hci_cmd_cb_host_buffer_size((void *)cmd_params);
+		return sdc_hci_cmd_cb_host_buffer_size_wrapper((void *)cmd_params);
 	case SDC_HCI_OPCODE_CMD_CB_HOST_NUMBER_OF_COMPLETED_PACKETS:
 		return sdc_hci_cmd_cb_host_number_of_completed_packets((void *)cmd_params);
 #endif

subsys/bluetooth/controller/hci_internal_wrappers.c

@@ -0,0 +1,78 @@
+/*
+ * Copyright (c) 2025 Nordic Semiconductor ASA
+ *
+ * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
+ */
+
+#include "hci_internal_wrappers.h"
+#include <zephyr/sys/util.h>
+#include <zephyr/bluetooth/buf.h>
+
+#if defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL)
+/*
+ * The Host will generate up to acl_pkts number of Host Number of Completed Packets command plus a
+ * number of normal HCI commands, as such we need to ensure the tx command buffer count is big
+ * enough to not block incoming ACKs from the host.
+ *
+ * When Controller to Host data flow control is supported, ensure that `CONFIG_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.
+ *
+ * The SDC controller (currently) does not support Num_HCI_Command_Packets > 1, which means Ncmd
+ * is always 1.
+ *
+ * `CONFIG_BT_BUF_CMD_TX_COUNT` is used differently depending on whether `CONFIG_BT_HCI_HOST` or
+ * `BT_HCI_RAW` is defined. And as ACL packet pools are only used in connections,
+ * we need to limit the build assert as such. See the comments above the asserts for more
+ * information on the specific scenario.
+ */
+#if defined(CONFIG_BT_HCI_HOST) && (BT_BUF_ACL_RX_COUNT > 0)
+/*
+ * When `CONFIG_BT_HCI_HOST`, `CONFIG_BT_BUF_CMD_TX_COUNT` controls the capacity of
+ * `bt_hci_cmd_create`. Which is used for all BT HCI Commands, and can be called concurrently from
+ * many different application contexts, initiating various processes.
+ *
+ * Currently `bt_hci_cmd_create` is generally `K_FOREVER`, as such this build assert only guarantees
+ * to avoid deadlocks due to missing CMD buffers, if the host is only allocating the next command
+ * once the previous is completed.
+ */
+BUILD_ASSERT(BT_BUF_ACL_RX_COUNT < CONFIG_BT_BUF_CMD_TX_COUNT,
+	     "Too low HCI command buffers compared to ACL Rx buffers.");
+#else  /* controller-only build */
+/*
+ * On a controller-only build (`BT_HCI_RAW`) `CONFIG_BT_BUF_CMD_TX_COUNT` controls the capacity of
+ * `bt_buf_get_tx(BT_BUF_CMD)`. Which is only used to receive commands from the Host at the rate the
+ * command flow control dictates. Considering one buffer is needed to the Num_HCI_Command_Packet, to
+ * do flow control, at least one more buffer is needed.
+ *
+ */
+BUILD_ASSERT((CONFIG_BT_BUF_CMD_TX_COUNT - 1) > 0,
+	     "We need at least two HCI command buffers to avoid deadlocks.");
+#endif /* CONFIG_BT_CONN && CONFIG_BT_HCI_HOST */
+
+/*
+ * This wrapper addresses a limitation in some Zephyr HCI samples such as Zephyr hci_ipc, namely the
+ * dropping of Host Number of Completed Packets (HNCP) messages when buffers are full. Dropping
+ * these messages causes a resource leak.
+ *
+ * The buffers are full when CONFIG_BT_BUF_CMD_TX_COUNT is exhausted. This wrapper implements a
+ * workaround that ensures the buffers are never exhausted, by limiting the
+ * Host_Total_Num_ACL_Data_Packets value. Limiting this value naturally limits the number of HNCP to
+ * one the sample buffers can handle.
+ *
+ * Considering the sample uses the same buffer pool for normal commands, which take up one buffer at
+ * most (look up `HCI_Command_Complete` for more details), this wrapper artifically limits the
+ * Host_Total_Num_ACL_Data_Packets to at most one less than the CONFIG_BT_BUF_CMD_TX_COUNT pool
+ * capacity.
+ * .
+ */
+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 = *cmd_params;
+
+	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(&ctrl_cmd_params);
+}
+#endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */

subsys/bluetooth/controller/hci_internal_wrappers.h

@@ -0,0 +1,22 @@
+/*
+ * Copyright (c) 2025 Nordic Semiconductor ASA
+ *
+ * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
+ */
+
+/** @file
+ *  @brief Internal HCI interface Wrappers
+ */
+
+#include <stdint.h>
+#include <stdbool.h>
+#include <sdc_hci_cmd_controller_baseband.h>
+
+#ifndef HCI_INTERNAL_WRAPPERS_H__
+#define HCI_INTERNAL_WRAPPERS_H__
+
+#if defined(__DOXYGEN__) || defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL)
+int sdc_hci_cmd_cb_host_buffer_size_wrapper(const sdc_hci_cmd_cb_host_buffer_size_t *cmd_params);
+#endif
+
+#endif

tests/subsys/bluetooth/controller/CMakeLists.txt

@@ -0,0 +1,26 @@
+#
+# Copyright (c) 2025 Nordic Semiconductor ASA
+#
+# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
+#
+
+cmake_minimum_required(VERSION 3.20.0)
+
+find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
+project(hci_cmd_cb_host_buffer_size_test)
+
+# Generate runner for the test
+test_runner_generate(src/hci_cmd_cb_host_buffer_size_test.c)
+
+# Add Unit Under Test source files
+target_sources(app PRIVATE ${ZEPHYR_NRF_MODULE_DIR}/subsys/bluetooth/controller/hci_internal_wrappers.c)
+
+# Add test source file
+target_sources(app PRIVATE src/hci_cmd_cb_host_buffer_size_test.c)
+
+# Create mocks
+cmock_handle(${ZEPHYR_NRFXLIB_MODULE_DIR}/softdevice_controller/include/sdc_hci_cmd_controller_baseband.h)
+
+# Include paths
+include_directories(${ZEPHYR_HAL_NORDIC_MODULE_DIR}/nrfx)
+include_directories(${ZEPHYR_NRF_MODULE_DIR}/subsys/bluetooth/controller)

tests/subsys/bluetooth/controller/prj.conf

@@ -0,0 +1,13 @@
+#
+# Copyright (c) 2025 Nordic Semiconductor ASA
+#
+# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
+#
+CONFIG_BT_CENTRAL=y
+CONFIG_BT_HCI=y
+CONFIG_BT=y
+
+CONFIG_BT_HCI_ACL_FLOW_CONTROL=y
+CONFIG_BT_BUF_CMD_TX_COUNT=10
+
+CONFIG_UNITY=y

tests/subsys/bluetooth/controller/src/hci_cmd_cb_host_buffer_size_test.c

@@ -0,0 +1,62 @@
+/*
+ * Copyright (c) 2025 Nordic Semiconductor ASA
+ *
+ * SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
+ */
+#include <unity.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <errno.h>
+#include <zephyr/sys/util.h>
+
+#include "hci_internal_wrappers.h"
+#include "cmock_sdc_hci_cmd_controller_baseband.h"
+
+#define TEST_HOST_ACL_DATA_PCKT_LNGTH  55
+#define TEST_HOST_SYNC_DATA_PCKT_LNGTH 55
+#define TEST_HOST_NUM_SYNC_DATA_PCKTS  5555
+
+/* The unity_main is not declared in any header file. It is only defined in the generated test
+ * runner because of ncs' unity configuration. It is therefore declared here to avoid a compiler
+ * warning.
+ */
+extern int unity_main(void);
+
+typedef struct {
+	uint16_t acl_input_pckts;
+	uint16_t exp_acl_pckts;
+} test_vector_t;
+
+static const test_vector_t test_vectors[] = {
+	{5, 5},					  /* Input within range, no adjustment needed */
+	{15, (CONFIG_BT_BUF_CMD_TX_COUNT - 1)},	  /* Input exceeds TX buffer count - 1 limit */
+	{0xFF, (CONFIG_BT_BUF_CMD_TX_COUNT - 1)}, /* Maximum input value, corner case */
+};
+
+void test_sdc_hci_cmd_cb_host_buffer_size_wrapper(void)
+{
+	sdc_hci_cmd_cb_host_buffer_size_t cmd_params;
+
+	cmd_params.host_acl_data_packet_length = TEST_HOST_ACL_DATA_PCKT_LNGTH;
+	cmd_params.host_sync_data_packet_length = TEST_HOST_SYNC_DATA_PCKT_LNGTH;
+	cmd_params.host_total_num_sync_data_packets = TEST_HOST_NUM_SYNC_DATA_PCKTS;
+
+	for (size_t i = 0; i < ARRAY_SIZE(test_vectors); i++) {
+		const test_vector_t *v = &test_vectors[i];
+
+		sdc_hci_cmd_cb_host_buffer_size_t exp_cmd_params = cmd_params;
+
+		exp_cmd_params.host_total_num_acl_data_packets = v->exp_acl_pckts;
+		__cmock_sdc_hci_cmd_cb_host_buffer_size_ExpectAndReturn(&exp_cmd_params, 0);
+
+		cmd_params.host_total_num_acl_data_packets = v->acl_input_pckts;
+		sdc_hci_cmd_cb_host_buffer_size_wrapper(&cmd_params);
+	}
+}
+
+int main(void)
+{
+	(void)unity_main();
+
+	return 0;
+}

tests/subsys/bluetooth/controller/testcase.yaml

@@ -0,0 +1,9 @@
+tests:
+  bluetooth.controller:
+    platform_allow:
+      - native_sim
+    integration_platforms:
+      - native_sim
+    tags:
+      - unittest
+      - ci_tests_subsys_bluetooth_controller