From 6c02a2b307297638bb92c18ccea514ea4326e71b Mon Sep 17 00:00:00 2001 From: Kyra Lengfeld Date: Mon, 13 Jan 2025 12:20:23 +0100 Subject: [PATCH] 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 --- CODEOWNERS | 1 + .../thingy91/thingy91_nrf52840_defconfig | 3 + .../releases_and_maturity/known_issues.rst | 10 +++ scripts/ci/tags.yaml | 5 ++ subsys/bluetooth/controller/CMakeLists.txt | 1 + subsys/bluetooth/controller/hci_internal.c | 3 +- .../controller/hci_internal_wrappers.c | 78 +++++++++++++++++++ .../controller/hci_internal_wrappers.h | 22 ++++++ .../bluetooth/controller/CMakeLists.txt | 26 +++++++ tests/subsys/bluetooth/controller/prj.conf | 13 ++++ .../src/hci_cmd_cb_host_buffer_size_test.c | 62 +++++++++++++++ .../subsys/bluetooth/controller/testcase.yaml | 9 +++ 12 files changed, 232 insertions(+), 1 deletion(-) create mode 100644 subsys/bluetooth/controller/hci_internal_wrappers.c create mode 100644 subsys/bluetooth/controller/hci_internal_wrappers.h create mode 100644 tests/subsys/bluetooth/controller/CMakeLists.txt create mode 100644 tests/subsys/bluetooth/controller/prj.conf create mode 100644 tests/subsys/bluetooth/controller/src/hci_cmd_cb_host_buffer_size_test.c create mode 100644 tests/subsys/bluetooth/controller/testcase.yaml diff --git a/CODEOWNERS b/CODEOWNERS index 2adbab8697a9..f2a168ab8b29 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -865,6 +865,7 @@ /tests/subsys/app_event_manager/ @nrfconnect/ncs-si-muffin @nrfconnect/ncs-si-bluebagel /tests/subsys/audio/audio_module_template/ @nrfconnect/ncs-audio /tests/subsys/audio_module/ @nrfconnect/ncs-audio +/tests/subsys/bluetooth/controller/ @nrfconnect/ncs-dragoon /tests/subsys/bluetooth/gatt_dm/ @nrfconnect/ncs-si-muffin /tests/subsys/bluetooth/enocean/ @nrfconnect/ncs-paladin /tests/subsys/bluetooth/fast_pair/ @nrfconnect/ncs-si-bluebagel diff --git a/boards/nordic/thingy91/thingy91_nrf52840_defconfig b/boards/nordic/thingy91/thingy91_nrf52840_defconfig index a21adb47660d..e123dfe24767 100644 --- a/boards/nordic/thingy91/thingy91_nrf52840_defconfig +++ b/boards/nordic/thingy91/thingy91_nrf52840_defconfig @@ -14,3 +14,6 @@ CONFIG_CONSOLE=y CONFIG_USB_DEVICE_STACK=y CONFIG_BOOTLOADER_MCUBOOT=y + +# BT +CONFIG_BT_BUF_CMD_TX_COUNT=3 diff --git a/doc/nrf/releases_and_maturity/known_issues.rst b/doc/nrf/releases_and_maturity/known_issues.rst index 55bac3351c53..dc8566deb700 100644 --- a/doc/nrf/releases_and_maturity/known_issues.rst +++ b/doc/nrf/releases_and_maturity/known_issues.rst @@ -212,6 +212,16 @@ NCSDK-31528: Deadlock on system workqueue with ``tx_notify`` in host .. rst-class:: v2-9-0-nRF54H20-rc1 v2-9-0 v2-8-0 v2-7-0 v2-6-3 v2-6-2 v2-6-1 v2-6-0 +DRGN-24352: Missing disconnection events when HCI Controller to host flow control is enabled + The :zephyr:code-sample:`bluetooth_hci_ipc` sample and :ref:`ipc_radio` application do not support stream flow control and drop bytes. + With the deferred disconnection complete generation added in the |NCS| v2.6.0, there are cases where the :kconfig:option:`CONFIG_BT_HCI_ACL_FLOW_CONTROL` Kconfig option has value ``y``, and the value of :kconfig:option:`CONFIG_BT_BUF_CMD_TX_COUNT` is smaller than :kconfig:option:`CONFIG_BT_BUF_ACL_RX_COUNT` + 1. + This might result in dropped HCI command packets. + The visible symptom is a missing disconnect event and subsequent failure in connection, if a peripheral device falls out of range or is powered off. + + **Workaround:** Make sure that the value of the :kconfig:option:`CONFIG_BT_BUF_ACL_RX_COUNT` Kconfig option is smaller than the one for :kconfig:option:`CONFIG_BT_BUF_CMD_TX_COUNT` in your project. + +.. rst-class:: v2-9-0-nRF54H20-rc1 v2-9-0 v2-8-0 v2-7-0 v2-6-3 v2-6-2 v2-6-1 v2-6-0 + NCSDK-31095: Issues with the :kconfig:option:`CONFIG_SEGGER_SYSVIEW` Kconfig option Using this Kconfig option causes the data parameter in the macros :c:macro:`k_fifo_put`, :c:macro:`k_fifo_alloc_put`, :c:macro:`k_lifo_put`, and :c:macro:`k_lifo_alloc_put` to be evaluated multiple times. This can cause problems if the data parameter is a function call incrementing a reference counter. diff --git a/scripts/ci/tags.yaml b/scripts/ci/tags.yaml index 429ad2f26dd1..be6beeada084 100644 --- a/scripts/ci/tags.yaml +++ b/scripts/ci/tags.yaml @@ -940,6 +940,11 @@ ci_tests_subsys_audio_module: - nrf/subsys/audio/ - nrf/tests/subsys/audio/ +ci_tests_subsys_bluetooth_controller: + files: + - nrf/subsys/bluetooth/controller + - nrf/tests/subsys/bluetooth/controller + ci_tests_subsys_mpsl: files: - nrf/subsys/mpsl/ diff --git a/subsys/bluetooth/controller/CMakeLists.txt b/subsys/bluetooth/controller/CMakeLists.txt index 77936e9bedc7..c8d8bb19dddb 100644 --- a/subsys/bluetooth/controller/CMakeLists.txt +++ b/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( diff --git a/subsys/bluetooth/controller/hci_internal.c b/subsys/bluetooth/controller/hci_internal.c index 8dbdcb26f92f..c78253a0c78b 100644 --- a/subsys/bluetooth/controller/hci_internal.c +++ b/subsys/bluetooth/controller/hci_internal.c @@ -16,6 +16,7 @@ #include #include "hci_internal.h" +#include "hci_internal_wrappers.h" #include "ecdh.h" #define CMD_COMPLETE_MIN_SIZE (BT_HCI_EVT_HDR_SIZE \ @@ -903,7 +904,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 diff --git a/subsys/bluetooth/controller/hci_internal_wrappers.c b/subsys/bluetooth/controller/hci_internal_wrappers.c new file mode 100644 index 000000000000..74847e921c9d --- /dev/null +++ b/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 +#include + +#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 */ diff --git a/subsys/bluetooth/controller/hci_internal_wrappers.h b/subsys/bluetooth/controller/hci_internal_wrappers.h new file mode 100644 index 000000000000..1bfa772a9943 --- /dev/null +++ b/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 +#include +#include + +#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 diff --git a/tests/subsys/bluetooth/controller/CMakeLists.txt b/tests/subsys/bluetooth/controller/CMakeLists.txt new file mode 100644 index 000000000000..68f2a9255e72 --- /dev/null +++ b/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) diff --git a/tests/subsys/bluetooth/controller/prj.conf b/tests/subsys/bluetooth/controller/prj.conf new file mode 100644 index 000000000000..8c317c5c4c56 --- /dev/null +++ b/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 diff --git a/tests/subsys/bluetooth/controller/src/hci_cmd_cb_host_buffer_size_test.c b/tests/subsys/bluetooth/controller/src/hci_cmd_cb_host_buffer_size_test.c new file mode 100644 index 000000000000..4f3353989546 --- /dev/null +++ b/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 +#include +#include +#include +#include + +#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; +} diff --git a/tests/subsys/bluetooth/controller/testcase.yaml b/tests/subsys/bluetooth/controller/testcase.yaml new file mode 100644 index 000000000000..72ad74b6f9f1 --- /dev/null +++ b/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