Bluetooth: Host: Fix redundant ACL Rx buffer allocation

HCI ACL Rx buffer count is not required in addition, the
calculated BT_BUF_ACL_RX_COUNT is sufficient for L2CAP
recombination.

But it is required to have additional Rx buffers for the
HCI ACL Rx that can happen due to the generated HCI Host
Number of Completed packets events.

Fixes commit d382fca ("Bluetooth: Controller: Fix HCI
command buffer allocation failure").

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>

Author: cvinayak

include/zephyr/bluetooth/buf.h

@@ -151,8 +151,20 @@ static inline enum bt_buf_type bt_buf_type_from_h4(uint8_t h4_type, enum bt_buf_
  * re-assembly into, and if all links are re-assembling, there will be no buffer
  * available for the HCI driver to allocate from.
  */
-#define BT_BUF_ACL_RX_COUNT_EXTRA CONFIG_BT_BUF_ACL_RX_COUNT_EXTRA
-#define BT_BUF_ACL_RX_COUNT       (1 + BT_BUF_ACL_RX_COUNT_EXTRA)
+#define BT_BUF_ACL_RX_COUNT_RESERVED     1
+
+/* Host will block unreferencing one Rx buffer per active ACL connection and generate HCI Host
+ * Number of Completed packets for the rest. This means new HCI data packets will be generated by
+ * the Controller minus the minimum number of Rx buffer count blocked unreferencing in the Host.
+ */
+#define BT_BUF_ACL_RX_COUNT_BLOCKED_MIN  1
+#define BT_BUF_ACL_RX_COUNT_BLOCKED_MAX  CONFIG_BT_MAX_CONN
+
+/* To avoid deadlock, account for maximum Rx buffers blocked in the Host */
+#define BT_BUF_ACL_RX_COUNT_EXTRA        MAX(BT_BUF_ACL_RX_COUNT_BLOCKED_MAX, \
+					     CONFIG_BT_BUF_ACL_RX_COUNT_EXTRA)
+/* ACL Rx Buffers being processed by the Host */
+#define BT_BUF_ACL_RX_COUNT              (BT_BUF_ACL_RX_COUNT_RESERVED + BT_BUF_ACL_RX_COUNT_EXTRA)
 #else
 #define BT_BUF_ACL_RX_COUNT_EXTRA 0
 #define BT_BUF_ACL_RX_COUNT       0
@@ -171,9 +183,18 @@ BUILD_ASSERT(BT_BUF_ACL_RX_COUNT <= BT_BUF_ACL_RX_COUNT_MAX,
 BUILD_ASSERT(CONFIG_BT_BUF_EVT_RX_COUNT > CONFIG_BT_BUF_ACL_TX_COUNT,
 	     "Increase Event RX buffer count to be greater than ACL TX buffer count");
 
+#if defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL)
+/* TODO: Introduce CONFIG_BT_BUF_HCI_ACL_RX_COUNT replace BT_BUF_HCI_ACL_RX_COUNT. For now,
+ * hard-coded 2. We do not want the use of CONFIG_BT_BUF_EVT_RX_COUNT in the derivation of
+ * BT_BUF_HCI_ACL_RX_COUNT.
+ */
+#define BT_BUF_RX_COUNT (2U + BT_BUF_ACL_RX_COUNT)
+
+#else /* !CONFIG_BT_HCI_ACL_FLOW_CONTROL */
 /** Buffer count needed for HCI ACL or HCI ISO plus Event RX buffers */
 #define BT_BUF_RX_COUNT (CONFIG_BT_BUF_EVT_RX_COUNT + \
 			 MAX(BT_BUF_ACL_RX_COUNT, BT_BUF_ISO_RX_COUNT))
+#endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */
 
 /** Data size needed for HCI Command buffers. */
 #define BT_BUF_CMD_TX_SIZE BT_BUF_CMD_SIZE(CONFIG_BT_BUF_CMD_TX_SIZE)

subsys/bluetooth/common/hci_common_internal.h

@@ -41,15 +41,13 @@ BUILD_ASSERT((CONFIG_BT_BUF_CMD_TX_COUNT == CONFIG_BT_CTLR_HCI_NUM_CMD_PKT_MAX),
  *
  * Host keeps the first, and subsequent, Rx buffers (that comes from the driver) for each connection
  * to do re-assembly into, up to the L2CAP SDU length required number of Rx buffers.
- * BT_BUF_ACL_RX_COUNT_EXTRA holds the application configured number of buffers across active
- * connections for recombination of HCI data packets to L2CAP SDUs.
- *
- * BT_BUF_HCI_EVT_RX_COUNT defines the number of available buffers reserved for "synchronous"
- * processing of HCI events like Number of Completed Packets, disconnection complete etc.
+ * BT_BUF_ACL_RX_COUNT_BLOCKED_MAX represents the maximum number of buffers that can be blocked
+ * across active connections for recombination of HCI data packets to L2CAP SDUs.
  *
  * BT_BUF_HCI_ACL_RX_COUNT defines the number of available buffers for Controller to Host data
  * flow control; keeping the application configured BT_BUF_ACL_RX_COUNT_EXTRA number of buffers
- * available for L2CAP recombination, and a reserved number of buffers for processing HCI events.
+ * available for L2CAP recombination, and BT_BUF_ACL_RX_COUNT_RESERVED reserved number of buffers
+ * for processing incoming HCI data packets.
  */
 
 /* FIXME: Calculate the maximum number of HCI events of different types that a connection can
@@ -67,12 +65,23 @@ BUILD_ASSERT((CONFIG_BT_BUF_CMD_TX_COUNT == CONFIG_BT_CTLR_HCI_NUM_CMD_PKT_MAX),
  * control to restrict buffers required on resource constraint devices, i.e. if these events are not
  * processed "synchronous".
  */
-#define BT_BUF_HCI_EVT_RX_COUNT          1
-#define BT_BUF_HCI_ACL_RX_COUNT          (BT_BUF_RX_COUNT - BT_BUF_HCI_EVT_RX_COUNT - \
-					  BT_BUF_ACL_RX_COUNT_EXTRA)
+#define BT_BUF_HCI_ACL_RX_COUNT          (BT_BUF_RX_COUNT - BT_BUF_ACL_RX_COUNT_EXTRA)
+
+/* During L2CAP recombination, maximum one Rx buffer per each active ACL connection can be held or
+ * blocked unreferencing in the Host, i.e. BT_BUF_ACL_RX_COUNT_BLOCKED_MAX.
+ *
+ * But at the instant of generating Host Number of Completed packets events a minimum of one Rx
+ * buffer is still blocked unreferencing, i.e. BT_BUF_ACL_RX_COUNT_BLOCKED_MIN.
+ *
+ * BT_BUF_HCI_ACL_RX_COUNT_EXTRA defines the outstanding number of HCI ACL data packets that the
+ * Controller can generated.
+ */
+#define BT_BUF_HCI_ACL_RX_COUNT_EXTRA    (BT_BUF_HCI_ACL_RX_COUNT - BT_BUF_ACL_RX_COUNT_RESERVED - \
+					  BT_BUF_ACL_RX_COUNT_BLOCKED_MIN)
 #define BT_BUF_CMD_TX_HOST_NUM_CMPLT_PKT (BT_BUF_HCI_ACL_RX_COUNT)
 
 #else /* !CONFIG_BT_HCI_ACL_FLOW_CONTROL */
+#define BT_BUF_HCI_ACL_RX_COUNT_EXTRA    0
 #define BT_BUF_CMD_TX_HOST_NUM_CMPLT_PKT 0
 #endif /* !CONFIG_BT_HCI_ACL_FLOW_CONTROL */
 

subsys/bluetooth/host/buf.c

@@ -83,9 +83,13 @@ static void evt_pool_destroy(struct net_buf *buf)
 	buf_rx_freed_notify(BT_BUF_EVT);
 }
 
-NET_BUF_POOL_DEFINE(acl_in_pool, (BT_BUF_ACL_RX_COUNT_EXTRA + BT_BUF_HCI_ACL_RX_COUNT),
-		    BT_BUF_ACL_SIZE(CONFIG_BT_BUF_ACL_RX_SIZE), sizeof(struct bt_conn_rx),
-		    acl_in_pool_destroy);
+/* Allocate Rx buffer count sufficient to block unreferencing one Rx buffer per active ACL
+ * connection, and additional HCI ACL data packets that the Controller can generate for the
+ * Host Number of Completed data packets count that the Host has given back to the Controller.
+ */
+NET_BUF_POOL_DEFINE(acl_in_pool, (BT_BUF_ACL_RX_COUNT_EXTRA + BT_BUF_HCI_ACL_RX_COUNT_EXTRA),
+		    BT_BUF_ACL_SIZE(CONFIG_BT_BUF_ACL_RX_SIZE),
+		    sizeof(struct bt_conn_rx), acl_in_pool_destroy);
 
 NET_BUF_POOL_FIXED_DEFINE(evt_pool, CONFIG_BT_BUF_EVT_RX_COUNT, BT_BUF_EVT_RX_SIZE, 0,
 			  evt_pool_destroy);