[Backport v2.7-branch] Bluetooth: fix HCI command / data deadlock

include/bluetooth/l2cap.h

@@ -266,6 +266,19 @@ struct bt_l2cap_chan_ops {
  */
  void (*encrypt_change)(struct bt_l2cap_chan *chan, uint8_t hci_status);
 
+ /** @brief Channel alloc_seg callback
+ *
+ * If this callback is provided the channel will use it to allocate
+ * buffers to store segments. This avoids wasting big SDU buffers with
+ * potentially much smaller PDUs. If this callback is supplied, it must
+ * return a valid buffer.
+ *
+ * @param chan The channel requesting a buffer.
+ *
+ * @return Allocated buffer.
+ */
+ struct net_buf *(*alloc_seg)(struct bt_l2cap_chan *chan);
+
  /** @brief Channel alloc_buf callback
  *
  * If this callback is provided the channel will use it to allocate

include/net/buf.h

@@ -889,15 +889,6 @@ static inline void net_buf_simple_restore(struct net_buf_simple *buf,
  buf->len = state->len;
 }
 
-/**
- * Flag indicating that the buffer has associated fragments. Only used
- * internally by the buffer handling code while the buffer is inside a
- * FIFO, meaning this never needs to be explicitly set or unset by the
- * net_buf API user. As long as the buffer is outside of a FIFO, i.e.
- * in practice always for the user for this API, the buf->frags pointer
- * should be used instead.
- */
-#define NET_BUF_FRAGS BIT(0)
 /**
  * Flag indicating that the buffer's associated data pointer, points to
  * externally allocated memory. Therefore once ref goes down to zero, the
@@ -907,7 +898,7 @@ static inline void net_buf_simple_restore(struct net_buf_simple *buf,
  * Reference count mechanism however will behave the same way, and ref
  * count going to 0 will free the net_buf but no the data pointer in it.
  */
-#define NET_BUF_EXTERNAL_DATA BIT(1)
+#define NET_BUF_EXTERNAL_DATA BIT(0)
 
 /**
  * @brief Network buffer representation.
@@ -917,13 +908,11 @@ static inline void net_buf_simple_restore(struct net_buf_simple *buf,
  * using the net_buf_alloc() API.
  */
 struct net_buf {
- union {
- /** Allow placing the buffer into sys_slist_t */
- sys_snode_t node;
+ /** Allow placing the buffer into sys_slist_t */
+ sys_snode_t node;
 
- /** Fragments associated with this buffer. */
- struct net_buf *frags;
- };
+ /** Fragments associated with this buffer. */
+ struct net_buf *frags;
 
  /** Reference count. */
  uint8_t ref;

subsys/bluetooth/host/conn.c

@@ -41,6 +41,11 @@
 
 struct tx_meta {
  struct bt_conn_tx *tx;
+ /* This flag indicates if the current buffer has already been partially
+ * sent to the controller (ie, the next fragments should be sent as
+ * continuations).
+ */
+ bool is_cont;
 };
 
 #define tx_data(buf) ((struct tx_meta *)net_buf_user_data(buf))
@@ -396,6 +401,8 @@ int bt_conn_send_cb(struct bt_conn *conn, struct net_buf *buf,
  tx_data(buf)->tx = NULL;
  }
 
+ tx_data(buf)->is_cont = false;
+
  net_buf_put(&conn->tx_queue, buf);
  return 0;
 }
@@ -464,25 +471,41 @@ static int send_iso(struct bt_conn *conn, struct net_buf *buf, uint8_t flags)
  return bt_send(buf);
 }
 
-static bool send_frag(struct bt_conn *conn, struct net_buf *buf, uint8_t flags,
- bool always_consume)
+static inline uint16_t conn_mtu(struct bt_conn *conn)
+{
+#if defined(CONFIG_BT_BREDR)
+ if (conn->type == BT_CONN_TYPE_BR || !bt_dev.le.acl_mtu) {
+ return bt_dev.br.mtu;
+ }
+#endif /* CONFIG_BT_BREDR */
+#if defined(CONFIG_BT_ISO)
+ if (conn->type == BT_CONN_TYPE_ISO && bt_dev.le.iso_mtu) {
+ return bt_dev.le.iso_mtu;
+ }
+#endif /* CONFIG_BT_ISO */
+#if defined(CONFIG_BT_CONN)
+ return bt_dev.le.acl_mtu;
+#else
+ return 0;
+#endif /* CONFIG_BT_CONN */
+}
+
+static int do_send_frag(struct bt_conn *conn, struct net_buf *buf, uint8_t flags)
 {
  struct bt_conn_tx *tx = tx_data(buf)->tx;
- uint32_t *pending_no_cb;
+ uint32_t *pending_no_cb = NULL;
  unsigned int key;
  int err = 0;
 
- BT_DBG("conn %p buf %p len %u flags 0x%02x", conn, buf, buf->len,
- flags);
-
- /* Wait until the controller can accept ACL packets */
- k_sem_take(bt_conn_get_pkts(conn), K_FOREVER);
-
  /* Check for disconnection while waiting for pkts_sem */
  if (conn->state != BT_CONN_CONNECTED) {
+ err = -ENOTCONN;
  goto fail;
  }
 
+ BT_DBG("conn %p buf %p len %u flags 0x%02x", conn, buf, buf->len,
+ flags);
+
  /* Add to pending, it must be done before bt_buf_set_type */
  key = irq_lock();
  if (tx) {
@@ -520,46 +543,61 @@ static bool send_frag(struct bt_conn *conn, struct net_buf *buf, uint8_t flags,
  (*pending_no_cb)--;
  }
  irq_unlock(key);
+
+ /* We don't want to end up in a situation where send_acl/iso
+ * returns the same error code as when we don't get a buffer in
+ * time.
+ */
+ err = -EIO;
  goto fail;
  }
 
- return true;
+ return 0;
 
 fail:
+ /* If we get here, something has seriously gone wrong:
+ * We also need to destroy the `parent` buf.
+ */
  k_sem_give(bt_conn_get_pkts(conn));
  if (tx) {
  tx_free(tx);
  }
 
- if (always_consume) {
- net_buf_unref(buf);
- }
- return false;
+ return err;
 }
 
-static inline uint16_t conn_mtu(struct bt_conn *conn)
+static int send_frag(struct bt_conn *conn,
+ struct net_buf *buf, struct net_buf *frag,
+ uint8_t flags)
 {
-#if defined(CONFIG_BT_BREDR)
- if (conn->type == BT_CONN_TYPE_BR || !bt_dev.le.acl_mtu) {
- return bt_dev.br.mtu;
+ /* Check if the controller can accept ACL packets */
+ if (k_sem_take(bt_conn_get_pkts(conn), K_NO_WAIT)) {
+ BT_DBG("no controller bufs");
+ return -ENOBUFS;
  }
-#endif /* CONFIG_BT_BREDR */
-#if defined(CONFIG_BT_ISO)
- if (conn->type == BT_CONN_TYPE_ISO && bt_dev.le.iso_mtu) {
- return bt_dev.le.iso_mtu;
+
+ /* Add the data to the buffer */
+ if (frag) {
+ uint16_t frag_len = MIN(conn_mtu(conn), net_buf_tailroom(frag));
+
+ net_buf_add_mem(frag, buf->data, frag_len);
+ net_buf_pull(buf, frag_len);
+ } else {
+ /* De-queue the buffer now that we know we can send it.
+ * Only applies if the buffer to be sent is the original buffer,
+ * and not one of its fragments.
+ * This buffer was fetched from the FIFO using a peek operation.
+ */
+ buf = net_buf_get(&conn->tx_queue, K_NO_WAIT);
+ frag = buf;
  }
-#endif /* CONFIG_BT_ISO */
-#if defined(CONFIG_BT_CONN)
- return bt_dev.le.acl_mtu;
-#else
- return 0;
-#endif /* CONFIG_BT_CONN */
+
+ return do_send_frag(conn, frag, flags);
 }
 
 static struct net_buf *create_frag(struct bt_conn *conn, struct net_buf *buf)
 {
  struct net_buf *frag;
- uint16_t frag_len;
 
  switch (conn->type) {
 #if defined(CONFIG_BT_ISO)
@@ -583,52 +621,55 @@ static struct net_buf *create_frag(struct bt_conn *conn, struct net_buf *buf)
 
  /* Fragments never have a TX completion callback */
  tx_data(frag)->tx = NULL;
-
- frag_len = MIN(conn_mtu(conn), net_buf_tailroom(frag));
-
- net_buf_add_mem(frag, buf->data, frag_len);
- net_buf_pull(buf, frag_len);
+ tx_data(frag)->is_cont = false;
 
  return frag;
 }
 
-static bool send_buf(struct bt_conn *conn, struct net_buf *buf)
+static int send_buf(struct bt_conn *conn, struct net_buf *buf)
 {
  struct net_buf *frag;
+ uint8_t flags;
+ int err;
 
  BT_DBG("conn %p buf %p len %u", conn, buf, buf->len);
 
  /* Send directly if the packet fits the ACL MTU */
- if (buf->len <= conn_mtu(conn)) {
- return send_frag(conn, buf, FRAG_SINGLE, false);
- }
-
- /* Create & enqueue first fragment */
- frag = create_frag(conn, buf);
- if (!frag) {
- return false;
- }
-
- if (!send_frag(conn, frag, FRAG_START, true)) {
- return false;
+ if (buf->len <= conn_mtu(conn) && !tx_data(buf)->is_cont) {
+ BT_DBG("send single");
+ return send_frag(conn, buf, NULL, FRAG_SINGLE);
  }
 
+ BT_DBG("start fragmenting");
  /*
  * Send the fragments. For the last one simply use the original
- * buffer (which works since we've used net_buf_pull on it.
+ * buffer (which works since we've used net_buf_pull on it).
  */
+ flags = FRAG_START;
+ if (tx_data(buf)->is_cont) {
+ flags = FRAG_CONT;
+ }
+
  while (buf->len > conn_mtu(conn)) {
  frag = create_frag(conn, buf);
  if (!frag) {
- return false;
+ return -ENOMEM;
  }
 
- if (!send_frag(conn, frag, FRAG_CONT, true)) {
- return false;
+ err = send_frag(conn, buf, frag, flags);
+ if (err) {
+ BT_DBG("%p failed, mark as existing frag", buf);
+ tx_data(buf)->is_cont = flags != FRAG_START;
+ net_buf_unref(frag);
+ return err;
  }
+
+ flags = FRAG_CONT;
  }
 
- return send_frag(conn, buf, FRAG_END, false);
+ BT_DBG("last frag");
+ tx_data(buf)->is_cont = true;
+ return send_frag(conn, buf, NULL, FRAG_END);
 }
 
 static struct k_poll_signal conn_change =
@@ -674,10 +715,26 @@ static int conn_prepare_events(struct bt_conn *conn,
 
  BT_DBG("Adding conn %p to poll list", conn);
 
- k_poll_event_init(&events[0],
- K_POLL_TYPE_FIFO_DATA_AVAILABLE,
- K_POLL_MODE_NOTIFY_ONLY,
- &conn->tx_queue);
+ bool buffers_available = k_sem_count_get(bt_conn_get_pkts(conn)) > 0;
+ bool packets_waiting = !k_fifo_is_empty(&conn->tx_queue);
+
+ if (packets_waiting && !buffers_available) {
+ /* Only resume sending when the controller has buffer space
+ * available for this connection.
+ */
+ BT_DBG("wait on ctlr buffers");
+ k_poll_event_init(&events[0],
+ K_POLL_TYPE_SEM_AVAILABLE,
+ K_POLL_MODE_NOTIFY_ONLY,
+ bt_conn_get_pkts(conn));
+ } else {
+ /* Wait until there is more data to send. */
+ BT_DBG("wait on host fifo");
+ k_poll_event_init(&events[0],
+ K_POLL_TYPE_FIFO_DATA_AVAILABLE,
+ K_POLL_MODE_NOTIFY_ONLY,
+ &conn->tx_queue);
+ }
  events[0].tag = BT_EVENT_CONN_TX_QUEUE;
 
  return 0;
@@ -720,6 +777,7 @@ int bt_conn_prepare_events(struct k_poll_event events[])
 void bt_conn_process_tx(struct bt_conn *conn)
 {
  struct net_buf *buf;
+ int err;
 
  BT_DBG("conn %p", conn);
 
@@ -730,10 +788,26 @@ void bt_conn_process_tx(struct bt_conn *conn)
  return;
  }
 
- /* Get next ACL packet for connection */
- buf = net_buf_get(&conn->tx_queue, K_NO_WAIT);
+ /* Get next ACL packet for connection. The buffer will only get dequeued
+ * if there is a free controller buffer to put it in.
+ *
+ * Important: no operations should be done on `buf` until it is properly
+ * dequeued from the FIFO, using the `net_buf_get()` API.
+ */
+ buf = k_fifo_peek_head(&conn->tx_queue);
  BT_ASSERT(buf);
- if (!send_buf(conn, buf)) {
+
+ /* Since we used `peek`, the queue still owns the reference to the
+ * buffer, so we need to take an explicit additional reference here.
+ */
+ buf = net_buf_ref(buf);
+ err = send_buf(conn, buf);
+ net_buf_unref(buf);
+
+ if (err == -EIO) {
+ tx_data(buf)->tx = NULL;
+
+ /* destroy the buffer */
  net_buf_unref(buf);
  }
 }

subsys/bluetooth/host/hci_core.c

@@ -2372,6 +2372,13 @@ static void process_events(struct k_poll_event *ev, int count)
  switch (ev->state) {
  case K_POLL_STATE_SIGNALED:
  break;
+ case K_POLL_STATE_SEM_AVAILABLE:
+ /* After this fn is exec'd, `bt_conn_prepare_events()`
+ * will be called once again, and this time buffers will
+ * be available, so the FIFO will be added to the poll
+ * list instead of the ctlr buffers semaphore.
+ */
+ break;
  case K_POLL_STATE_FIFO_DATA_AVAILABLE:
  if (ev->tag == BT_EVENT_CMD_TX) {
  send_cmd();
@@ -2431,6 +2438,7 @@ static void hci_tx_thread(void *p1, void *p2, void *p3)
  events[0].state = K_POLL_STATE_NOT_READY;
  ev_count = 1;
 
+ /* This adds the FIFO per-connection */
  if (IS_ENABLED(CONFIG_BT_CONN) || IS_ENABLED(CONFIG_BT_ISO)) {
  ev_count += bt_conn_prepare_events(&events[1]);
  }