Bluetooth: Controller: Fix HCI command buffer allocation failure

Fix HCI command buffer allocation failure, that can cause loss of Host Number of Completed Packets command. Fail by rejecting the HCI Host Buffer Size command if the required number of HCI command buffers are not allocated in the Controller implementation. When Controller to Host data flow control is supported in the Controller only build, ensure that BT_BUF_CMD_TX_COUNT is greater than or equal to (BT_BUF_ACL_RX_COUNT + Ncmd), where Ncmd is supported maximum Num_HCI_Command_Packets in the Controller implementation. Relates to commit 8161430 ("Bluetooth: Add workaround for no command buffer available")'. Relates to commit 297f4f4 ("Bluetooth: Split HCI command & event buffers to two pools"). Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
zephyrproject-rtos · Jan 10, 2025 · 8369ca2 · 8369ca2
1 parent bc6eded
commit 8369ca2
Show file tree

Hide file tree

Showing 28 changed files with 125 additions and 94 deletions.
diff --git a/samples/bluetooth/hci_ipc/nrf5340_cpunet_bis-bt_ll_sw_split.conf b/samples/bluetooth/hci_ipc/nrf5340_cpunet_bis-bt_ll_sw_split.conf
@@ -10,12 +10,7 @@ CONFIG_HEAP_MEM_POOL_SIZE=4096
 CONFIG_BT=y
 CONFIG_BT_HCI_RAW=y
 
-# Workaround: Unable to allocate command buffer when using K_NO_WAIT since
-# Host number of completed commands does not follow normal flow control.
-CONFIG_BT_BUF_CMD_TX_COUNT=10
-
 CONFIG_BT_BUF_EVT_RX_COUNT=16
-
 CONFIG_BT_BUF_EVT_RX_SIZE=255
 CONFIG_BT_BUF_ACL_RX_SIZE=255
 CONFIG_BT_BUF_ACL_TX_SIZE=251

diff --git a/samples/bluetooth/hci_ipc/nrf5340_cpunet_bt_mesh-bt_ll_sw_split.conf b/samples/bluetooth/hci_ipc/nrf5340_cpunet_bt_mesh-bt_ll_sw_split.conf
@@ -10,11 +10,6 @@ CONFIG_BT=y
 CONFIG_BT_HCI_RAW=y
 CONFIG_BT_MAX_CONN=16
 
-
-# Workaround: Unable to allocate command buffer when using K_NO_WAIT since
-# Host number of completed commands does not follow normal flow control.
-CONFIG_BT_BUF_CMD_TX_COUNT=10
-
 # Controller
 CONFIG_BT_LL_SW_SPLIT=y
 

diff --git a/samples/bluetooth/hci_ipc/nrf5340_cpunet_cis-bt_ll_sw_split.conf b/samples/bluetooth/hci_ipc/nrf5340_cpunet_cis-bt_ll_sw_split.conf
@@ -10,12 +10,7 @@ CONFIG_HEAP_MEM_POOL_SIZE=4096
 CONFIG_BT=y
 CONFIG_BT_HCI_RAW=y
 
-# Workaround: Unable to allocate command buffer when using K_NO_WAIT since
-# Host number of completed commands does not follow normal flow control.
-CONFIG_BT_BUF_CMD_TX_COUNT=10
-
 CONFIG_BT_BUF_EVT_RX_COUNT=16
-
 CONFIG_BT_BUF_EVT_RX_SIZE=255
 CONFIG_BT_BUF_ACL_RX_SIZE=255
 CONFIG_BT_BUF_ACL_TX_SIZE=251

diff --git a/samples/bluetooth/hci_ipc/nrf5340_cpunet_df-bt_ll_sw_split.conf b/samples/bluetooth/hci_ipc/nrf5340_cpunet_df-bt_ll_sw_split.conf
@@ -10,12 +10,7 @@ CONFIG_HEAP_MEM_POOL_SIZE=4096
 CONFIG_BT=y
 CONFIG_BT_HCI_RAW=y
 
-# Workaround: Unable to allocate command buffer when using K_NO_WAIT since
-# Host number of completed commands does not follow normal flow control.
-CONFIG_BT_BUF_CMD_TX_COUNT=10
-
 CONFIG_BT_BUF_EVT_RX_COUNT=16
-
 CONFIG_BT_BUF_EVT_RX_SIZE=255
 CONFIG_BT_BUF_ACL_RX_SIZE=255
 CONFIG_BT_BUF_ACL_TX_SIZE=251

diff --git a/samples/bluetooth/hci_ipc/nrf5340_cpunet_iso-bt_ll_sw_split.conf b/samples/bluetooth/hci_ipc/nrf5340_cpunet_iso-bt_ll_sw_split.conf
@@ -14,12 +14,7 @@ CONFIG_LTO=y
 CONFIG_BT=y
 CONFIG_BT_HCI_RAW=y
 
-# Workaround: Unable to allocate command buffer when using K_NO_WAIT since
-# Host number of completed commands does not follow normal flow control.
-CONFIG_BT_BUF_CMD_TX_COUNT=10
-
 CONFIG_BT_BUF_EVT_RX_COUNT=16
-
 CONFIG_BT_BUF_EVT_RX_SIZE=255
 CONFIG_BT_BUF_ACL_RX_SIZE=255
 CONFIG_BT_BUF_ACL_TX_SIZE=251

diff --git a/samples/bluetooth/hci_ipc/nrf5340_cpunet_iso_broadcast-bt_ll_sw_split.conf b/samples/bluetooth/hci_ipc/nrf5340_cpunet_iso_broadcast-bt_ll_sw_split.conf
@@ -10,10 +10,6 @@ CONFIG_HEAP_MEM_POOL_SIZE=4096
 CONFIG_BT=y
 CONFIG_BT_HCI_RAW=y
 
-# Workaround: Unable to allocate command buffer when using K_NO_WAIT since
-# Host number of completed commands does not follow normal flow control.
-CONFIG_BT_BUF_CMD_TX_COUNT=10
-
 # Host and Controller common dependencies
 CONFIG_BT_BROADCASTER=y
 CONFIG_BT_OBSERVER=n

diff --git a/samples/bluetooth/hci_ipc/nrf5340_cpunet_iso_central-bt_ll_sw_split.conf b/samples/bluetooth/hci_ipc/nrf5340_cpunet_iso_central-bt_ll_sw_split.conf
@@ -10,10 +10,6 @@ CONFIG_HEAP_MEM_POOL_SIZE=4096
 CONFIG_BT=y
 CONFIG_BT_HCI_RAW=y
 
-# Workaround: Unable to allocate command buffer when using K_NO_WAIT since
-# Host number of completed commands does not follow normal flow control.
-CONFIG_BT_BUF_CMD_TX_COUNT=10
-
 CONFIG_BT_BUF_EVT_RX_SIZE=255
 CONFIG_BT_BUF_ACL_RX_SIZE=255
 CONFIG_BT_BUF_ACL_TX_SIZE=251

diff --git a/samples/bluetooth/hci_ipc/nrf5340_cpunet_iso_peripheral-bt_ll_sw_split.conf b/samples/bluetooth/hci_ipc/nrf5340_cpunet_iso_peripheral-bt_ll_sw_split.conf
@@ -10,10 +10,6 @@ CONFIG_HEAP_MEM_POOL_SIZE=4096
 CONFIG_BT=y
 CONFIG_BT_HCI_RAW=y
 
-# Workaround: Unable to allocate command buffer when using K_NO_WAIT since
-# Host number of completed commands does not follow normal flow control.
-CONFIG_BT_BUF_CMD_TX_COUNT=10
-
 CONFIG_BT_BUF_EVT_RX_SIZE=255
 CONFIG_BT_BUF_ACL_RX_SIZE=255
 CONFIG_BT_BUF_ACL_TX_SIZE=251

diff --git a/samples/bluetooth/hci_ipc/nrf5340_cpunet_iso_receive-bt_ll_sw_split.conf b/samples/bluetooth/hci_ipc/nrf5340_cpunet_iso_receive-bt_ll_sw_split.conf
@@ -10,10 +10,6 @@ CONFIG_HEAP_MEM_POOL_SIZE=4096
 CONFIG_BT=y
 CONFIG_BT_HCI_RAW=y
 
-# Workaround: Unable to allocate command buffer when using K_NO_WAIT since
-# Host number of completed commands does not follow normal flow control.
-CONFIG_BT_BUF_CMD_TX_COUNT=10
-
 # Host and Controller common dependencies
 CONFIG_BT_BROADCASTER=n
 CONFIG_BT_OBSERVER=y

diff --git a/samples/bluetooth/hci_ipc/prj.conf b/samples/bluetooth/hci_ipc/prj.conf
@@ -10,11 +10,6 @@ CONFIG_BT=y
 CONFIG_BT_HCI_RAW=y
 CONFIG_BT_MAX_CONN=16
 
-
-# Workaround: Unable to allocate command buffer when using K_NO_WAIT since
-# Host number of completed commands does not follow normal flow control.
-CONFIG_BT_BUF_CMD_TX_COUNT=10
-
 # Enable and adjust the below value as necessary
 # CONFIG_BT_BUF_EVT_RX_COUNT=16
 # CONFIG_BT_BUF_EVT_RX_SIZE=255

diff --git a/samples/bluetooth/hci_spi/prj.conf b/samples/bluetooth/hci_spi/prj.conf
@@ -6,7 +6,3 @@ CONFIG_BT=y
 CONFIG_BT_HCI_RAW=y
 CONFIG_BT_MAX_CONN=16
 CONFIG_BT_SEND_ECC_EMULATION=n
-
-# Workaround: Unable to allocate command buffer when using K_NO_WAIT since
-# Host number of completed commands does not follow normal flow control.
-CONFIG_BT_BUF_CMD_TX_COUNT=10
diff --git a/samples/bluetooth/hci_uart/prj.conf b/samples/bluetooth/hci_uart/prj.conf
@@ -17,7 +17,3 @@ CONFIG_BT_SEND_ECC_EMULATION=n
 CONFIG_BT_CTLR_DTM_HCI=y
 
 CONFIG_SYSTEM_WORKQUEUE_STACK_SIZE=512
-
-# Workaround: Unable to allocate command buffer when using K_NO_WAIT since
-# Host number of completed commands does not follow normal flow control.
-CONFIG_BT_BUF_CMD_TX_COUNT=10
diff --git a/samples/bluetooth/hci_uart_3wire/prj.conf b/samples/bluetooth/hci_uart_3wire/prj.conf
@@ -16,7 +16,3 @@ CONFIG_BT_SEND_ECC_EMULATION=n
 CONFIG_BT_CTLR_DTM_HCI=y
 
 CONFIG_SYSTEM_WORKQUEUE_STACK_SIZE=512
-
-# Workaround: Unable to allocate command buffer when using K_NO_WAIT since
-# Host number of completed commands does not follow normal flow control.
-CONFIG_BT_BUF_CMD_TX_COUNT=10
diff --git a/samples/bluetooth/hci_usb/prj.conf b/samples/bluetooth/hci_usb/prj.conf
@@ -11,7 +11,3 @@ CONFIG_USB_DEVICE_INITIALIZE_AT_BOOT=n
 CONFIG_SERIAL=n
 CONFIG_CONSOLE=n
 CONFIG_UART_CONSOLE=n
-
-# Workaround: Unable to allocate command buffer when using K_NO_WAIT since
-# Host number of completed commands does not follow normal flow control.
-CONFIG_BT_BUF_CMD_TX_COUNT=10
diff --git a/samples/boards/nordic/mesh/onoff-app/prj.conf b/samples/boards/nordic/mesh/onoff-app/prj.conf
@@ -90,7 +90,6 @@ CONFIG_BT_RX_STACK_SIZE=4096
 CONFIG_BT_MAX_CONN=1
 CONFIG_BT_CTLR_RX_BUFFERS=6
 CONFIG_BT_BUF_ACL_TX_COUNT=4
-CONFIG_BT_BUF_CMD_TX_COUNT=4
 
 CONFIG_BT_ATT_PREPARE_COUNT=2
 

diff --git a/subsys/bluetooth/common/Kconfig b/subsys/bluetooth/common/Kconfig
@@ -183,12 +183,39 @@ config BT_BUF_CMD_TX_SIZE
 	  to the maximum of 255.
 
 config BT_BUF_CMD_TX_COUNT
-	int "Number of HCI command buffers"
-	default 2
-	range 2 64
+	int "Number of HCI command buffers" if !HAS_BT_CTLR
+	# !BT_HCI_ACL_FLOW_CONTROL ==> Permit configurable Num_HCI_Command_Packets for Host only
+	# build but hidden when HAS_BT_CTLR (default 1 for Controller only build).
+	# !(BT_HCI_RAW && HAS_BT_CTLR) ==> Host only build
+	depends on !BT_HCI_RAW || !HAS_BT_CTLR || !BT_HCI_ACL_FLOW_CONTROL
+	default 2 if BT_HCI_ACL_FLOW_CONTROL
+	default 1
+	range 1 64
 	help
 	  Number of buffers available for outgoing HCI commands from the Host.
 
+	  HCI Controllers may not support Num_HCI_Command_Packets > 1, hence
+	  default to 1 when not enabling Controller to Host data flow control,
+	  BT_HCI_ACL_FLOW_CONTROL.
+
+	  Normal HCI commands follow the HCI command flow control using
+	  Num_HCI_Command_Packets return in HCI command complete and status.
+
+	  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.
+
+	  When Controller to Host data flow control is enabled in the Host only
+	  build, ensure that BT_BUF_CMD_TX_COUNT is at least 2, to be able to
+	  send one normal HCI command and one Host Number of Completed Packets
+	  command.
+
+	  When Controller to Host data flow control is supported in the
+	  Controller only build, ensure that BT_BUF_CMD_TX_COUNT is greater than
+	  or equal to (BT_BUF_ACL_RX_COUNT + Ncmd), where Ncmd is supported
+	  maximum Num_HCI_Command_Packets in the Controller implementation.
+
 endmenu
 
 # Workaround to have commas on function arguments

diff --git a/subsys/bluetooth/common/hci_common_internal.h b/subsys/bluetooth/common/hci_common_internal.h
@@ -0,0 +1,29 @@
+/*
+ * Copyright (c) 2025 Nordic Semiconductor ASA
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/* Following build configurations use configurable CONFIG_BT_BUF_CMD_TX_COUNT:
+ * 1. Host + Controller build with and without Controller to Host data flow control, or
+ * 2. Host only with and without Controller to Host data flow control, or
+ * 3. Controller only without Controller to Host data flow control support
+ */
+#if !defined(CONFIG_HAS_BT_CTLR) || !defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL)
+#if defined(CONFIG_HAS_BT_CTLR)
+BUILD_ASSERT((CONFIG_BT_BUF_CMD_TX_COUNT == CONFIG_BT_CTLR_HCI_CMD_TX_COUNT),
+	     "Mismatch in allocated HCI command buffers compared to Controller supported value.");
+#endif /* CONFIG_HAS_BT_CTLR */
+#define BT_BUF_CMD_TX_COUNT CONFIG_BT_BUF_CMD_TX_COUNT
+
+/* When Controller to Host data flow control is supported in the Controller only build, ensure
+ * that BT_BUF_CMD_TX_COUNT is greater than or equal to (BT_BUF_ACL_RX_COUNT + Ncmd), where Ncmd
+ * is supported maximum Num_HCI_Command_Packets in the Controller implementation.
+ */
+#elif defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL)
+BUILD_ASSERT(!IS_ENABLED(CONFIG_BT_BUF_CMD_TX_COUNT),
+	     "Configurable HCI command buffer count disallowed.");
+BUILD_ASSERT(IS_ENABLED(CONFIG_BT_CTLR_HCI_CMD_TX_COUNT),
+	     "Undefined Controller implementation supported Num_HCI_Command_Packets value.");
+#define BT_BUF_CMD_TX_COUNT (BT_BUF_ACL_RX_COUNT + CONFIG_BT_CTLR_HCI_CMD_TX_COUNT)
+#endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */
diff --git a/subsys/bluetooth/controller/Kconfig.ll_sw_split b/subsys/bluetooth/controller/Kconfig.ll_sw_split
@@ -126,6 +126,12 @@ config BT_CTLR_RX_STACK_SIZE
 	int
 	default 896
 
+config BT_CTLR_HCI_CMD_TX_COUNT
+	# Hidden Controller implementation supported Num_HCI_Command_Packets value */
+	int
+	depends on BT_CTLR_HCI
+	default 1
+
 config BT_CTLR_SETTINGS
 	bool "Settings System"
 	depends on SETTINGS

diff --git a/subsys/bluetooth/controller/hci/hci.c b/subsys/bluetooth/controller/hci/hci.c
@@ -21,6 +21,8 @@
 #include <zephyr/bluetooth/hci_vs.h>
 #include <zephyr/bluetooth/buf.h>
 
+#include "../common/hci_common_internal.h"
+
 #include "../host/hci_ecc.h"
 
 #include "util/util.h"
@@ -184,13 +186,13 @@ static uint8_t sf_curr;
 #endif
 
 #if defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL)
-int32_t    hci_hbuf_total;
-uint32_t    hci_hbuf_sent;
-uint32_t    hci_hbuf_acked;
-uint16_t    hci_hbuf_pend[CONFIG_BT_MAX_CONN];
+int32_t  hci_hbuf_total;
+uint32_t hci_hbuf_sent;
+uint32_t hci_hbuf_acked;
+uint16_t hci_hbuf_pend[CONFIG_BT_MAX_CONN];
 atomic_t hci_state_mask;
 static struct k_poll_signal *hbuf_signal;
-#endif
+#endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */
 
 #if defined(CONFIG_BT_CONN)
 static uint32_t conn_count;
@@ -475,7 +477,7 @@ static void reset(struct net_buf *buf, struct net_buf **evt)
 		atomic_set_bit(&hci_state_mask, HCI_STATE_BIT_RESET);
 		k_poll_signal_raise(hbuf_signal, 0x0);
 	}
-#endif
+#endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */
 
 	hci_recv_fifo_reset();
 }
@@ -536,10 +538,33 @@ static void host_buffer_size(struct net_buf *buf, struct net_buf **evt)
 		ccst->status = BT_HCI_ERR_CMD_DISALLOWED;
 		return;
 	}
-	/* fragmentation from controller to host not supported, require
+
+	/* Fragmentation from Controller to Host not supported, require
 	 * ACL MTU to be at least the LL MTU
 	 */
 	if (acl_mtu < LL_LENGTH_OCTETS_RX_MAX) {
+		LOG_ERR("FC: Require Host ACL MTU (%u) >= LL Max Data Length (%u)", acl_mtu,
+			LL_LENGTH_OCTETS_RX_MAX);
+		ccst->status = BT_HCI_ERR_INVALID_PARAM;
+		return;
+	}
+
+	/* 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.
+	 *
+	 * Note: Zephyr Controller does not support Num_HCI_Command_Packets > 1.
+	 */
+	if (acl_pkts >= BT_BUF_CMD_TX_COUNT) {
+		LOG_ERR("FC: Require Host ACL packets (%u) < BT_BUF_CMD_TX_COUNT (%u)",
+			acl_pkts, BT_BUF_CMD_TX_COUNT);
 		ccst->status = BT_HCI_ERR_INVALID_PARAM;
 		return;
 	}
@@ -586,7 +611,7 @@ static void host_num_completed_packets(struct net_buf *buf,
 	hci_hbuf_acked += count;
 	k_poll_signal_raise(hbuf_signal, 0x0);
 }
-#endif
+#endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */
 
 #if defined(CONFIG_BT_CTLR_LE_PING)
 static void read_auth_payload_timeout(struct net_buf *buf, struct net_buf **evt)
@@ -747,7 +772,7 @@ static int ctrl_bb_cmd_handle(uint16_t  ocf, struct net_buf *cmd,
 	case BT_OCF(BT_HCI_OP_HOST_NUM_COMPLETED_PACKETS):
 		host_num_completed_packets(cmd, evt);
 		break;
-#endif
+#endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */
 
 #if defined(CONFIG_BT_CTLR_LE_PING)
 	case BT_OCF(BT_HCI_OP_READ_AUTH_PAYLOAD_TIMEOUT):
@@ -9094,7 +9119,7 @@ void hci_acl_encode(struct node_rx_pdu *node_rx, struct net_buf *buf)
 			LL_ASSERT(handle < ARRAY_SIZE(hci_hbuf_pend));
 			hci_hbuf_pend[handle]++;
 		}
-#endif
+#endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */
 		break;
 
 	default:
@@ -9300,6 +9325,7 @@ void hci_init(struct k_poll_signal *signal_host_buf)
 {
 #if defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL)
 	hbuf_signal = signal_host_buf;
-#endif
+#endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */
+
 	reset(NULL, NULL);
 }
diff --git a/subsys/bluetooth/host/hci_common.c b/subsys/bluetooth/host/hci_common.c
@@ -33,7 +33,17 @@ struct net_buf *bt_hci_cmd_complete_create(uint16_t op, uint8_t plen)
 	buf = bt_hci_evt_create(BT_HCI_EVT_CMD_COMPLETE, sizeof(*cc) + plen);
 
 	cc = net_buf_add(buf, sizeof(*cc));
+
+	/* The Num_HCI_Command_Packets parameter allows the Controller to
+	 * indicate the number of HCI command packets the Host can send to the
+	 * Controller. If the Controller requires the Host to stop sending
+	 * commands, Num_HCI_Command_Packets will be set to zero.
+	 *
+	 * NOTE: Zephyr Controller (and may be other Controllers) do not support
+	 *       higher Number of HCI Command packets than 1.
+	 */
 	cc->ncmd = 1U;
+
 	cc->opcode = sys_cpu_to_le16(op);
 
 	return buf;
@@ -48,7 +58,17 @@ struct net_buf *bt_hci_cmd_status_create(uint16_t op, uint8_t status)
 
 	cs = net_buf_add(buf, sizeof(*cs));
 	cs->status = status;
+
+	/* The Num_HCI_Command_Packets parameter allows the Controller to
+	 * indicate the number of HCI command packets the Host can send to the
+	 * Controller. If the Controller requires the Host to stop sending
+	 * commands, Num_HCI_Command_Packets will be set to zero.
+	 *
+	 * NOTE: Zephyr Controller (and may be other Controllers) do not support
+	 *       higher Number of HCI Command packets than 1.
+	 */
 	cs->ncmd = 1U;
+
 	cs->opcode = sys_cpu_to_le16(op);
 
 	return buf;