[PATCH 0/2] Bluetooth: virtio: improve RX handling

[PATCH 0/2] Bluetooth: virtio: improve RX handling

[Igor Skalkin]

This series fixes virtio Bluetooth RX buffer sizing, then reworks RX
processing to drain completed buffers directly from the virtqueue
callback and keep the RX ring populated.

Both patches were tested together against real Bluetooth hardware: a
host MediaTek USB controller exposed to a Linux guest through QEMU's
virtio-bt-pci device via HCI_CHANNEL_USER, not a stub or loopback
transport. The guest paired and bonded with a phone, ran SDP over
L2CAP to resolve its services, and exchanged the resulting HCI/ACL
traffic through the resized and reworked RX path with no drops,
crashes, or warnings observed.

The driver object builds with this series, and each patch passes
scripts/checkpatch.pl --strict independently.

Igor Skalkin (2):
  Bluetooth: virtio: fix RX buffer size
  Bluetooth: virtio: process RX buffers in virtqueue callback

 drivers/bluetooth/virtio_bt.c | 139 ++++++++++++++++++++++++++--------
 1 file changed, 108 insertions(+), 31 deletions(-)

-- 
2.53.0

[PATCH 1/2] Bluetooth: virtio: fix RX buffer size

[Igor Skalkin]

The fixed 1000-byte RX buffer is too small for an HCI frame at the
advertised ACL MTU. Size the buffer for HCI_MAX_FRAME_SIZE and one HCI
packet-type byte instead.

Signed-off-by: Igor Skalkin <igor.skalkin@oss.qualcomm.com>
---
 drivers/bluetooth/virtio_bt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c
index 140ab55c9..96f21501e 100644
--- a/drivers/bluetooth/virtio_bt.c
+++ b/drivers/bluetooth/virtio_bt.c
@@ -12,7 +12,7 @@
 #include <net/bluetooth/hci_core.h>
 
 #define VERSION "0.1"
-#define VIRTBT_RX_BUF_SIZE 1000
+#define VIRTBT_RX_BUF_SIZE	(HCI_MAX_FRAME_SIZE + 1)
 
 enum {
 	VIRTBT_VQ_TX,
-- 
2.53.0

[PATCH 2/2] Bluetooth: virtio: process RX buffers in virtqueue callback

[Igor Skalkin]

The RX virtqueue is sized for many in-flight descriptors (64 on
virtio-bt-pci), but the driver only ever posts a single buffer:
virtbt_open_vdev() posts one buffer at open, and the RX callback
replenishes exactly the one buffer it just consumed before kicking
again. The device can never have more than one RX descriptor
available, so every received frame serializes behind a full
kick/interrupt/workqueue round trip before the next one can even be
queued.

Drain all completed RX buffers from the virtqueue callback and pass
valid frames directly to hci_recv_frame(). Populate the RX ring until
it is full and replenish each consumed buffer in callback context with
GFP_ATOMIC. Keep a worker solely to retry refills with GFP_KERNEL
after an atomic allocation failure.

Serialize callback, refill, and teardown access to the RX virtqueue so
a refill cannot race with device removal.

Signed-off-by: Igor Skalkin <igor.skalkin@oss.qualcomm.com>
---
 drivers/bluetooth/virtio_bt.c | 137 ++++++++++++++++++++++++++--------
 1 file changed, 107 insertions(+), 30 deletions(-)

diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c
index 96f21501e..cb42b322b 100644
--- a/drivers/bluetooth/virtio_bt.c
+++ b/drivers/bluetooth/virtio_bt.c
@@ -23,24 +23,39 @@ enum {
 struct virtio_bluetooth {
 	struct virtio_device *vdev;
 	struct virtqueue *vqs[VIRTBT_NUM_VQS];
-	struct work_struct rx;
+	struct work_struct rx_refill;
+	/* Serializes RX virtqueue operations and teardown. */
+	spinlock_t rx_lock;
 	struct hci_dev *hdev;
+	unsigned int rx_buf_count;
+	bool stopped;
 };
 
-static int virtbt_add_inbuf(struct virtio_bluetooth *vbt)
+static int virtbt_add_inbuf(struct virtio_bluetooth *vbt, gfp_t gfp)
 {
 	struct virtqueue *vq = vbt->vqs[VIRTBT_VQ_RX];
 	struct scatterlist sg[1];
 	struct sk_buff *skb;
+	unsigned long flags;
 	int err;
 
-	skb = alloc_skb(VIRTBT_RX_BUF_SIZE, GFP_KERNEL);
+	skb = alloc_skb(VIRTBT_RX_BUF_SIZE, gfp);
 	if (!skb)
 		return -ENOMEM;
 
 	sg_init_one(sg, skb->data, VIRTBT_RX_BUF_SIZE);
 
-	err = virtqueue_add_inbuf(vq, sg, 1, skb, GFP_KERNEL);
+	spin_lock_irqsave(&vbt->rx_lock, flags);
+	if (vbt->stopped) {
+		err = -ESHUTDOWN;
+	} else {
+		/* The lock serializes RX callback and refill worker producers. */
+		err = virtqueue_add_inbuf(vq, sg, 1, skb, GFP_ATOMIC);
+		if (!err)
+			vbt->rx_buf_count++;
+	}
+	spin_unlock_irqrestore(&vbt->rx_lock, flags);
+
 	if (err < 0) {
 		kfree_skb(skb);
 		return err;
@@ -56,10 +71,20 @@ static int virtbt_open(struct hci_dev *hdev)
 
 static int virtbt_open_vdev(struct virtio_bluetooth *vbt)
 {
-	if (virtbt_add_inbuf(vbt) < 0)
+	struct virtqueue *vq = vbt->vqs[VIRTBT_VQ_RX];
+	int err;
+
+	if (!virtqueue_get_vring_size(vq))
 		return -EIO;
 
-	virtqueue_kick(vbt->vqs[VIRTBT_VQ_RX]);
+	do {
+		err = virtbt_add_inbuf(vbt, GFP_KERNEL);
+	} while (!err);
+
+	if (!vbt->rx_buf_count)
+		return err;
+
+	virtqueue_kick(vq);
 	return 0;
 }
 
@@ -70,9 +95,15 @@ static int virtbt_close(struct hci_dev *hdev)
 
 static int virtbt_close_vdev(struct virtio_bluetooth *vbt)
 {
+	unsigned long flags;
 	int i;
 
-	cancel_work_sync(&vbt->rx);
+	spin_lock_irqsave(&vbt->rx_lock, flags);
+	vbt->stopped = true;
+	virtqueue_disable_cb(vbt->vqs[VIRTBT_VQ_RX]);
+	spin_unlock_irqrestore(&vbt->rx_lock, flags);
+
+	cancel_work_sync(&vbt->rx_refill);
 
 	for (i = 0; i < ARRAY_SIZE(vbt->vqs); i++) {
 		struct virtqueue *vq = vbt->vqs[i];
@@ -83,6 +114,10 @@ static int virtbt_close_vdev(struct virtio_bluetooth *vbt)
 		cond_resched();
 	}
 
+	spin_lock_irqsave(&vbt->rx_lock, flags);
+	vbt->rx_buf_count = 0;
+	spin_unlock_irqrestore(&vbt->rx_lock, flags);
+
 	return 0;
 }
 
@@ -234,31 +269,17 @@ static void virtbt_rx_handle(struct virtio_bluetooth *vbt, struct sk_buff *skb)
 	hci_recv_frame(vbt->hdev, skb);
 }
 
-static void virtbt_rx_work(struct work_struct *work)
+static void virtbt_rx_refill_work(struct work_struct *work)
 {
 	struct virtio_bluetooth *vbt = container_of(work,
-						    struct virtio_bluetooth, rx);
-	struct sk_buff *skb;
-	unsigned int len;
-
-	skb = virtqueue_get_buf(vbt->vqs[VIRTBT_VQ_RX], &len);
-	if (!skb)
-		return;
-
-	if (!len || len > VIRTBT_RX_BUF_SIZE) {
-		bt_dev_err_ratelimited(vbt->hdev,
-				       "rx reply len %u outside [1, %u]\n",
-				       len, VIRTBT_RX_BUF_SIZE);
-		kfree_skb(skb);
-	} else {
-		skb_put(skb, len);
-		virtbt_rx_handle(vbt, skb);
-	}
+						    struct virtio_bluetooth, rx_refill);
+	bool kick = false;
 
-	if (virtbt_add_inbuf(vbt) < 0)
-		return;
+	while (!virtbt_add_inbuf(vbt, GFP_KERNEL))
+		kick = true;
 
-	virtqueue_kick(vbt->vqs[VIRTBT_VQ_RX]);
+	if (kick)
+		virtqueue_kick(vbt->vqs[VIRTBT_VQ_RX]);
 }
 
 static void virtbt_tx_done(struct virtqueue *vq)
@@ -273,8 +294,63 @@ static void virtbt_tx_done(struct virtqueue *vq)
 static void virtbt_rx_done(struct virtqueue *vq)
 {
 	struct virtio_bluetooth *vbt = vq->vdev->priv;
+	bool cb_enabled = true;
+	bool kick = false;
+
+	for (;;) {
+		struct sk_buff *skb;
+		unsigned int len;
+		unsigned long flags;
+
+		spin_lock_irqsave(&vbt->rx_lock, flags);
+		if (vbt->stopped) {
+			spin_unlock_irqrestore(&vbt->rx_lock, flags);
+			return;
+		}
+
+		if (cb_enabled) {
+			virtqueue_disable_cb(vq);
+			cb_enabled = false;
+		}
+
+		skb = virtqueue_get_buf(vq, &len);
+		if (skb)
+			vbt->rx_buf_count--;
+
+		if (!skb) {
+			if (kick) {
+				spin_unlock_irqrestore(&vbt->rx_lock, flags);
+				kick = false;
+				virtqueue_kick(vq);
+				continue;
+			}
+
+			if (virtqueue_enable_cb(vq)) {
+				spin_unlock_irqrestore(&vbt->rx_lock, flags);
+				return;
+			}
+			cb_enabled = true;
+		}
+		spin_unlock_irqrestore(&vbt->rx_lock, flags);
 
-	schedule_work(&vbt->rx);
+		if (!skb)
+			continue;
+
+		if (!len || len > VIRTBT_RX_BUF_SIZE) {
+			bt_dev_err_ratelimited(vbt->hdev,
+					       "rx reply len %u outside [1, %u]\n",
+					       len, VIRTBT_RX_BUF_SIZE);
+			kfree_skb(skb);
+		} else {
+			skb_put(skb, len);
+			virtbt_rx_handle(vbt, skb);
+		}
+
+		if (virtbt_add_inbuf(vbt, GFP_ATOMIC) < 0)
+			schedule_work(&vbt->rx_refill);
+		else
+			kick = true;
+	}
 }
 
 static int virtbt_probe(struct virtio_device *vdev)
@@ -307,7 +383,8 @@ static int virtbt_probe(struct virtio_device *vdev)
 	vdev->priv = vbt;
 	vbt->vdev = vdev;
 
-	INIT_WORK(&vbt->rx, virtbt_rx_work);
+	INIT_WORK(&vbt->rx_refill, virtbt_rx_refill_work);
+	spin_lock_init(&vbt->rx_lock);
 
 	err = virtio_find_vqs(vdev, VIRTBT_NUM_VQS, vbt->vqs, vqs_info, NULL);
 	if (err)
-- 
2.53.0

RE: Bluetooth: virtio: improve RX handling

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1142326

---Test result---

Test Summary:
CheckPatch                    PASS      1.46 seconds
VerifyFixes                   PASS      0.13 seconds
VerifySignedoff               PASS      0.13 seconds
GitLint                       PASS      0.66 seconds
SubjectPrefix                 PASS      0.26 seconds
BuildKernel                   PASS      25.38 seconds
CheckAllWarning               PASS      28.10 seconds
CheckSparse                   PASS      26.78 seconds
BuildKernel32                 PASS      25.05 seconds
CheckKernelLLVM               SKIP      0.00 seconds
TestRunnerSetup               PASS      461.99 seconds
IncrementalBuild              PASS      27.38 seconds

Details
##############################
Test: CheckKernelLLVM - SKIP
Desc: Build kernel with LLVM + context analysis
Output:
Clang not found


https://github.com/bluez/bluetooth-next/pull/550

---
Regards,
Linux Bluetooth