Bluetooth: L2CAP: Process fixed channels in the RX thread

Now that the TX path and TX context (bt_conn_tx) has been redesigned
to free the contexts always in the system workqueue, it means the
system workqueue is the only context where their allocation may also
fail. This is particularly problematic with us having all L2CAP
channels (fixed & CoC alike) deferred to the system workqueue. It is
especially bad for fixed channels where being able to send responses
for SM, L2CAP signaling and ATT is critical to avoid timeouts for the
connection.

This patch moves the processing of all fixed L2CAP channels back to
the RX thread, thereby making it possible (and safe) to block while
waiting for a TX context to become available.

Signed-off-by: Johan Hedberg <[email protected]>

Author: Johan Hedberg

include/bluetooth/l2cap.h

@@ -90,9 +90,6 @@ struct bt_l2cap_chan {
 	struct k_delayed_work		rtx_work;
 	ATOMIC_DEFINE(status, BT_L2CAP_NUM_STATUS);
 
-	struct k_work			rx_work;
-	struct k_fifo			rx_queue;
-
 #if defined(CONFIG_BT_L2CAP_DYNAMIC_CHANNEL)
 	bt_l2cap_chan_state_t		state;
 	/** Remote PSM to be connected */
@@ -132,6 +129,9 @@ struct bt_l2cap_le_chan {
 	/** Segment SDU packet from upper layer */
 	struct net_buf			*_sdu;
 	u16_t				_sdu_len;
+
+	struct k_work			rx_work;
+	struct k_fifo			rx_queue;
 };
 
 /** @def BT_L2CAP_LE_CHAN(_ch)

subsys/bluetooth/host/l2cap.c

@@ -27,7 +27,7 @@
 #include "l2cap_internal.h"
 
 #define LE_CHAN_RTX(_w) CONTAINER_OF(_w, struct bt_l2cap_le_chan, chan.rtx_work)
-#define CHAN_RX(_w) CONTAINER_OF(_w, struct bt_l2cap_chan, rx_work)
+#define CHAN_RX(_w) CONTAINER_OF(_w, struct bt_l2cap_le_chan, rx_work)
 
 #define L2CAP_LE_MIN_MTU		23
 
@@ -221,8 +221,6 @@ void bt_l2cap_chan_set_state(struct bt_l2cap_chan *chan,
 
 void bt_l2cap_chan_del(struct bt_l2cap_chan *chan)
 {
-	struct net_buf *buf;
-
 	BT_DBG("conn %p chan %p", chan->conn, chan);
 
 	if (!chan->conn) {
@@ -242,11 +240,6 @@ void bt_l2cap_chan_del(struct bt_l2cap_chan *chan)
 	chan->psm = 0U;
 #endif
 
-	/* Remove buffers on the RX queue */
-	while ((buf = net_buf_get(&chan->rx_queue, K_NO_WAIT))) {
-		net_buf_unref(buf);
-	}
-
 	if (chan->destroy) {
 		chan->destroy(chan);
 	}
@@ -262,19 +255,22 @@ static void l2cap_rtx_timeout(struct k_work *work)
 	bt_l2cap_chan_del(&chan->chan);
 }
 
-static void l2cap_chan_recv(struct bt_l2cap_chan *chan, struct net_buf *buf);
+#if defined(CONFIG_BT_L2CAP_DYNAMIC_CHANNEL)
+static void l2cap_chan_le_recv(struct bt_l2cap_le_chan *chan,
+			       struct net_buf *buf);
 
 static void l2cap_rx_process(struct k_work *work)
 {
-	struct bt_l2cap_chan *chan = CHAN_RX(work);
+	struct bt_l2cap_le_chan *ch = CHAN_RX(work);
 	struct net_buf *buf;
 
-	while ((buf = net_buf_get(&chan->rx_queue, K_NO_WAIT))) {
-		BT_DBG("chan %p buf %p", chan, buf);
-		l2cap_chan_recv(chan, buf);
+	while ((buf = net_buf_get(&ch->rx_queue, K_NO_WAIT))) {
+		BT_DBG("ch %p buf %p", ch, buf);
+		l2cap_chan_le_recv(ch, buf);
 		net_buf_unref(buf);
 	}
 }
+#endif /* CONFIG_BT_L2CAP_DYNAMIC_CHANNEL */
 
 void bt_l2cap_chan_add(struct bt_conn *conn, struct bt_l2cap_chan *chan,
 		       bt_l2cap_chan_destroy_t destroy)
@@ -304,15 +300,16 @@ static bool l2cap_chan_add(struct bt_conn *conn, struct bt_l2cap_chan *chan,
 	}
 
 	k_delayed_work_init(&chan->rtx_work, l2cap_rtx_timeout);
-	k_work_init(&chan->rx_work, l2cap_rx_process);
-	k_fifo_init(&chan->rx_queue);
 
 	bt_l2cap_chan_add(conn, chan, destroy);
 
-	if (IS_ENABLED(CONFIG_BT_L2CAP_DYNAMIC_CHANNEL) &&
-	    L2CAP_LE_CID_IS_DYN(ch->rx.cid)) {
+#if defined(CONFIG_BT_L2CAP_DYNAMIC_CHANNEL)
+	if (L2CAP_LE_CID_IS_DYN(ch->rx.cid)) {
+		k_work_init(&ch->rx_work, l2cap_rx_process);
+		k_fifo_init(&ch->rx_queue);
 		bt_l2cap_chan_set_state(chan, BT_L2CAP_CONNECT);
 	}
+#endif /* CONFIG_BT_L2CAP_DYNAMIC_CHANNEL */
 
 	return true;
 }
@@ -759,6 +756,11 @@ static void l2cap_chan_destroy(struct bt_l2cap_chan *chan)
 		net_buf_unref(buf);
 	}
 
+	/* Remove buffers on the RX queue */
+	while ((buf = net_buf_get(&ch->rx_queue, K_NO_WAIT))) {
+		net_buf_unref(buf);
+	}
+
 	/* Destroy segmented SDU if it exists */
 	if (ch->_sdu) {
 		net_buf_unref(ch->_sdu);
@@ -1660,7 +1662,8 @@ static void l2cap_chan_recv(struct bt_l2cap_chan *chan, struct net_buf *buf)
 	struct bt_l2cap_le_chan *ch = BT_L2CAP_LE_CHAN(chan);
 
 	if (L2CAP_LE_CID_IS_DYN(ch->rx.cid)) {
-		l2cap_chan_le_recv(ch, buf);
+		net_buf_put(&ch->rx_queue, net_buf_ref(buf));
+		k_work_submit(&ch->rx_work);
 		return;
 	}
 #endif /* CONFIG_BT_L2CAP_DYNAMIC_CHANNEL */
@@ -1670,13 +1673,6 @@ static void l2cap_chan_recv(struct bt_l2cap_chan *chan, struct net_buf *buf)
 	chan->ops->recv(chan, buf);
 }
 
-static void l2cap_chan_recv_queue(struct bt_l2cap_chan *chan,
-				  struct net_buf *buf)
-{
-	net_buf_put(&chan->rx_queue, buf);
-	k_work_submit(&chan->rx_work);
-}
-
 void bt_l2cap_recv(struct bt_conn *conn, struct net_buf *buf)
 {
 	struct bt_l2cap_hdr *hdr;
@@ -1707,7 +1703,8 @@ void bt_l2cap_recv(struct bt_conn *conn, struct net_buf *buf)
 		return;
 	}
 
-	l2cap_chan_recv_queue(chan, buf);
+	l2cap_chan_recv(chan, buf);
+	net_buf_unref(buf);
 }
 
 int bt_l2cap_update_conn_param(struct bt_conn *conn,