[RESEND 1/2] Bluetooth: btusb: Add support for queuing during polling interval

[RESEND 1/2] Bluetooth: btusb: Add support for queuing during polling interval

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This makes btusb to queue ACL and events during a polling interval
by using of a delayed work, with the interval working as a time window
where frames received from different endpoints are considered to be
arrived at same time and then attempt to resolve potential conflics by
processing the events ahead of ACL packets.

It worth noting though that priorizing events over ACL data may result
in inverting the order compared to how they appeared over the air, for
instance there may be packets received before a disconnect event that
will be discarded and unencrypted packets received before encryption
change which would considered encrypted, because of these potential
changes on the order the support for queuing during the polling
interval is not enabled by default so platforms have the following
means to enable it:

At build-time:

    CONFIG_BT_HCIBTUSB_INTERVAL=y

At runtime with use of module option:

    enable_interval

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 drivers/bluetooth/Kconfig |  7 ++++
 drivers/bluetooth/btusb.c | 88 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 84 insertions(+), 11 deletions(-)

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 4e73a531b377..2f20a853d946 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -41,6 +41,13 @@ config BT_HCIBTUSB_AUTOSUSPEND
 	  This can be overridden by passing btusb.enable_autosuspend=[y|n]
 	  on the kernel commandline.
 
+config BT_HCIBTUSB_INTERVAL
+	bool "Enable notification of USB polling interval"
+	depends on BT_HCIBTUSB
+	help
+	  Say Y here to enable notification of USB polling interval for
+	  Bluetooth USB devices by default.
+
 config BT_HCIBTUSB_BCM
 	bool "Broadcom protocol support"
 	depends on BT_HCIBTUSB
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index b14102fba601..38cb5448fc69 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -30,7 +30,7 @@
 static bool disable_scofix;
 static bool force_scofix;
 static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
-
+static bool enable_interval = IS_ENABLED(CONFIG_BT_HCIBTUSB_INTERVAL);
 static bool reset = true;
 
 static struct usb_driver btusb_driver;
@@ -519,8 +519,12 @@ struct btusb_data {
 
 	unsigned long flags;
 
-	struct work_struct work;
-	struct work_struct waker;
+	struct work_struct  work;
+	struct work_struct  waker;
+	struct delayed_work rx_work;
+
+	struct sk_buff_head acl_q;
+	struct sk_buff_head evt_q;
 
 	struct usb_anchor deferred;
 	struct usb_anchor tx_anchor;
@@ -557,7 +561,7 @@ struct btusb_data {
 	int isoc_altsetting;
 	int suspend_count;
 
-	int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
+	int (*recv_event)(struct btusb_data *data, struct sk_buff *skb);
 	int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
 
 	int (*setup_on_usb)(struct hci_dev *hdev);
@@ -707,7 +711,7 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
 
 		if (!hci_skb_expect(skb)) {
 			/* Complete frame */
-			data->recv_event(data->hdev, skb);
+			data->recv_event(data, skb);
 			skb = NULL;
 		}
 	}
@@ -718,6 +722,25 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
 	return err;
 }
 
+static int btusb_rx_queue(struct btusb_data *data, struct sk_buff *skb,
+			  struct sk_buff_head *queue, unsigned int interval)
+{
+	skb_queue_tail(queue, skb);
+
+	schedule_delayed_work(&data->rx_work, interval);
+
+	return 0;
+}
+
+static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
+{
+	if (!enable_interval)
+		return hci_recv_frame(data->hdev, skb);
+
+	/* TODO: Calculate polling interval based on endpoint bInterval? */
+	return btusb_rx_queue(data, skb, &data->acl_q, msecs_to_jiffies(1));
+}
+
 static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
 {
 	struct sk_buff *skb;
@@ -765,7 +788,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
 
 		if (!hci_skb_expect(skb)) {
 			/* Complete frame */
-			hci_recv_frame(data->hdev, skb);
+			btusb_recv_acl(data, skb);
 			skb = NULL;
 		}
 	}
@@ -1383,9 +1406,13 @@ static int btusb_close(struct hci_dev *hdev)
 
 	BT_DBG("%s", hdev->name);
 
+	cancel_delayed_work(&data->rx_work);
 	cancel_work_sync(&data->work);
 	cancel_work_sync(&data->waker);
 
+	skb_queue_purge(&data->acl_q);
+	skb_queue_purge(&data->evt_q);
+
 	clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
 	clear_bit(BTUSB_BULK_RUNNING, &data->flags);
 	clear_bit(BTUSB_INTR_RUNNING, &data->flags);
@@ -1417,6 +1444,11 @@ static int btusb_flush(struct hci_dev *hdev)
 
 	BT_DBG("%s", hdev->name);
 
+	cancel_delayed_work(&data->rx_work);
+
+	skb_queue_purge(&data->acl_q);
+	skb_queue_purge(&data->evt_q);
+
 	usb_kill_anchored_urbs(&data->tx_anchor);
 	btusb_free_frags(data);
 
@@ -1769,6 +1801,25 @@ static void btusb_waker(struct work_struct *work)
 	usb_autopm_put_interface(data->intf);
 }
 
+static void btusb_rx_dequeue(struct btusb_data *data,
+			     struct sk_buff_head *queue)
+{
+	struct sk_buff *skb;
+
+	while ((skb = skb_dequeue(queue)))
+		hci_recv_frame(data->hdev, skb);
+}
+
+static void btusb_rx_work(struct work_struct *work)
+{
+	struct btusb_data *data = container_of(work, struct btusb_data,
+					       rx_work.work);
+
+	/* Process HCI event packets so states changes are synchronized first */
+	btusb_rx_dequeue(data, &data->evt_q);
+	btusb_rx_dequeue(data, &data->acl_q);
+}
+
 static int btusb_setup_bcm92035(struct hci_dev *hdev)
 {
 	struct sk_buff *skb;
@@ -2304,10 +2355,8 @@ static void btusb_intel_secure_send_result(struct btusb_data *data,
 		wake_up_bit(&data->flags, BTUSB_DOWNLOADING);
 }
 
-static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
+static int btusb_recv_event_intel(struct btusb_data *data, struct sk_buff *skb)
 {
-	struct btusb_data *data = hci_get_drvdata(hdev);
-
 	if (test_bit(BTUSB_BOOTLOADER, &data->flags)) {
 		struct hci_event_hdr *hdr = (void *)skb->data;
 
@@ -2336,7 +2385,7 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
 		}
 	}
 
-	return hci_recv_frame(hdev, skb);
+	return hci_recv_frame(data->hdev, skb);
 }
 
 static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
@@ -4279,6 +4328,15 @@ static int btusb_shutdown_qca(struct hci_dev *hdev)
 	return 0;
 }
 
+static int btusb_recv_evt(struct btusb_data *data, struct sk_buff *skb)
+{
+	if (!enable_interval)
+		return hci_recv_frame(data->hdev, skb);
+
+	/* Don't delay event processing */
+	return btusb_rx_queue(data, skb, &data->evt_q, 0);
+}
+
 static int btusb_probe(struct usb_interface *intf,
 		       const struct usb_device_id *id)
 {
@@ -4362,6 +4420,11 @@ static int btusb_probe(struct usb_interface *intf,
 
 	INIT_WORK(&data->work, btusb_work);
 	INIT_WORK(&data->waker, btusb_waker);
+	INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work);
+
+	skb_queue_head_init(&data->acl_q);
+	skb_queue_head_init(&data->evt_q);
+
 	init_usb_anchor(&data->deferred);
 	init_usb_anchor(&data->tx_anchor);
 	spin_lock_init(&data->txlock);
@@ -4378,7 +4441,7 @@ static int btusb_probe(struct usb_interface *intf,
 		data->recv_bulk = btusb_recv_bulk_intel;
 		set_bit(BTUSB_BOOTLOADER, &data->flags);
 	} else {
-		data->recv_event = hci_recv_frame;
+		data->recv_event = btusb_recv_evt;
 		data->recv_bulk = btusb_recv_bulk;
 	}
 
@@ -4867,6 +4930,9 @@ MODULE_PARM_DESC(force_scofix, "Force fixup of wrong SCO buffers size");
 module_param(enable_autosuspend, bool, 0644);
 MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
 
+module_param(enable_interval, bool, 0644);
+MODULE_PARM_DESC(enable_interval, "Enable USB polling interval by default");
+
 module_param(reset, bool, 0644);
 MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
 
-- 
2.26.2

[RESEND 2/2] Bluetooth: L2CAP: Fix handling fragmented length

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Bluetooth Core Specification v5.2, Vol. 3, Part A, section 1.4, table
1.1:

 'Start Fragments always either begin with the first octet of the Basic
  L2CAP header of a PDU or they have a length of zero (see [Vol 2] Part
  B, Section 6.6.2).'

Apparently this was changed by the following errata:

https://www.bluetooth.org/tse/errata_view.cfm?errata_id=10216

Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
 include/net/bluetooth/l2cap.h |   1 +
 net/bluetooth/l2cap_core.c    | 118 +++++++++++++++++++++++++++-------
 2 files changed, 94 insertions(+), 25 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 1d1232917de7..61800a7b6192 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -207,6 +207,7 @@ struct l2cap_hdr {
 	__le16     len;
 	__le16     cid;
 } __packed;
+#define L2CAP_LEN_SIZE		2
 #define L2CAP_HDR_SIZE		4
 #define L2CAP_ENH_HDR_SIZE	6
 #define L2CAP_EXT_HDR_SIZE	8
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 17b87b57a175..a24183734bd9 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -8276,10 +8276,73 @@ static void l2cap_security_cfm(struct hci_conn *hcon, u8 status, u8 encrypt)
 	mutex_unlock(&conn->chan_lock);
 }
 
+/* Append fragment into frame respecting the maximum len of rx_skb */
+static int l2cap_recv_frag(struct l2cap_conn *conn, struct sk_buff *skb,
+			   u16 len)
+{
+	if (!conn->rx_skb) {
+		/* Allocate skb for the complete frame (with header) */
+		conn->rx_skb = bt_skb_alloc(len, GFP_KERNEL);
+		if (!conn->rx_skb)
+			return -ENOMEM;
+		/* Init rx_len */
+		conn->rx_len = len;
+	}
+
+	/* Copy as much as the rx_skb can hold */
+	len = min_t(u16, len, skb->len);
+	skb_copy_from_linear_data(skb, skb_put(conn->rx_skb, len), len);
+	skb_pull(skb, len);
+	conn->rx_len -= len;
+
+	return len;
+}
+
+static int l2cap_recv_len(struct l2cap_conn *conn, struct sk_buff *skb)
+{
+	struct sk_buff *rx_skb;
+	int len;
+
+	/* Append just enough to complete the header */
+	len = l2cap_recv_frag(conn, skb, L2CAP_LEN_SIZE - conn->rx_skb->len);
+
+	/* If header could not be read just continue */
+	if (len < 0 || conn->rx_skb->len < L2CAP_LEN_SIZE)
+		return len;
+
+	rx_skb = conn->rx_skb;
+	len = get_unaligned_le16(rx_skb->data);
+
+	/* Check if rx_skb has enough space to received all fragments */
+	if (len + (L2CAP_HDR_SIZE - L2CAP_LEN_SIZE) <= skb_tailroom(rx_skb)) {
+		/* Update expected len */
+		conn->rx_len = len + (L2CAP_HDR_SIZE - L2CAP_LEN_SIZE);
+		return L2CAP_LEN_SIZE;
+	}
+
+	/* Reset conn->rx_skb since it will need to be reallocated in order to
+	 * fit all fragments.
+	 */
+	conn->rx_skb = NULL;
+
+	/* Reallocates rx_skb using the exact expected length */
+	len = l2cap_recv_frag(conn, rx_skb,
+			      len + (L2CAP_HDR_SIZE - L2CAP_LEN_SIZE));
+	kfree_skb(rx_skb);
+
+	return len;
+}
+
+static void l2cap_recv_reset(struct l2cap_conn *conn)
+{
+	kfree_skb(conn->rx_skb);
+	conn->rx_skb = NULL;
+	conn->rx_len = 0;
+}
+
 void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
 {
 	struct l2cap_conn *conn = hcon->l2cap_data;
-	struct l2cap_hdr *hdr;
 	int len;
 
 	/* For AMP controller do not create l2cap conn */
@@ -8298,23 +8361,23 @@ void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
 	case ACL_START:
 	case ACL_START_NO_FLUSH:
 	case ACL_COMPLETE:
-		if (conn->rx_len) {
+		if (conn->rx_skb) {
 			BT_ERR("Unexpected start frame (len %d)", skb->len);
-			kfree_skb(conn->rx_skb);
-			conn->rx_skb = NULL;
-			conn->rx_len = 0;
+			l2cap_recv_reset(conn);
 			l2cap_conn_unreliable(conn, ECOMM);
 		}
 
-		/* Start fragment always begin with Basic L2CAP header */
-		if (skb->len < L2CAP_HDR_SIZE) {
-			BT_ERR("Frame is too short (len %d)", skb->len);
-			l2cap_conn_unreliable(conn, ECOMM);
-			goto drop;
+		/* Start fragment may not contain the L2CAP length so just
+		 * copy the initial byte when that happens and use conn->mtu as
+		 * expected length.
+		 */
+		if (skb->len < L2CAP_LEN_SIZE) {
+			if (l2cap_recv_frag(conn, skb, conn->mtu) < 0)
+				goto drop;
+			return;
 		}
 
-		hdr = (struct l2cap_hdr *) skb->data;
-		len = __le16_to_cpu(hdr->len) + L2CAP_HDR_SIZE;
+		len = get_unaligned_le16(skb->data) + L2CAP_HDR_SIZE;
 
 		if (len == skb->len) {
 			/* Complete frame received */
@@ -8331,38 +8394,43 @@ void l2cap_recv_acldata(struct hci_conn *hcon, struct sk_buff *skb, u16 flags)
 			goto drop;
 		}
 
-		/* Allocate skb for the complete frame (with header) */
-		conn->rx_skb = bt_skb_alloc(len, GFP_KERNEL);
-		if (!conn->rx_skb)
+		/* Append fragment into frame (with header) */
+		if (l2cap_recv_frag(conn, skb, len) < 0)
 			goto drop;
 
-		skb_copy_from_linear_data(skb, skb_put(conn->rx_skb, skb->len),
-					  skb->len);
-		conn->rx_len = len - skb->len;
 		break;
 
 	case ACL_CONT:
 		BT_DBG("Cont: frag len %d (expecting %d)", skb->len, conn->rx_len);
 
-		if (!conn->rx_len) {
+		if (!conn->rx_skb) {
 			BT_ERR("Unexpected continuation frame (len %d)", skb->len);
 			l2cap_conn_unreliable(conn, ECOMM);
 			goto drop;
 		}
 
+		/* Complete the L2CAP length if it has not been read */
+		if (conn->rx_skb->len < L2CAP_LEN_SIZE) {
+			if (l2cap_recv_len(conn, skb) < 0) {
+				l2cap_conn_unreliable(conn, ECOMM);
+				goto drop;
+			}
+
+			/* Header still could not be read just continue */
+			if (conn->rx_skb->len < L2CAP_LEN_SIZE)
+				return;
+		}
+
 		if (skb->len > conn->rx_len) {
 			BT_ERR("Fragment is too long (len %d, expected %d)",
 			       skb->len, conn->rx_len);
-			kfree_skb(conn->rx_skb);
-			conn->rx_skb = NULL;
-			conn->rx_len = 0;
+			l2cap_recv_reset(conn);
 			l2cap_conn_unreliable(conn, ECOMM);
 			goto drop;
 		}
 
-		skb_copy_from_linear_data(skb, skb_put(conn->rx_skb, skb->len),
-					  skb->len);
-		conn->rx_len -= skb->len;
+		/* Append fragment into frame (with header) */
+		l2cap_recv_frag(conn, skb, skb->len);
 
 		if (!conn->rx_len) {
 			/* Complete frame received. l2cap_recv_frame
-- 
2.26.2

Re: [RESEND 1/2] Bluetooth: btusb: Add support for queuing during polling interval

[Luiz Augusto von Dentz]

Hi Abhishek,

On Wed, Jan 13, 2021 at 3:28 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This makes btusb to queue ACL and events during a polling interval
> by using of a delayed work, with the interval working as a time window
> where frames received from different endpoints are considered to be
> arrived at same time and then attempt to resolve potential conflics by
> processing the events ahead of ACL packets.
>
> It worth noting though that priorizing events over ACL data may result
> in inverting the order compared to how they appeared over the air, for
> instance there may be packets received before a disconnect event that
> will be discarded and unencrypted packets received before encryption
> change which would considered encrypted, because of these potential
> changes on the order the support for queuing during the polling
> interval is not enabled by default so platforms have the following
> means to enable it:
>
> At build-time:
>
>     CONFIG_BT_HCIBTUSB_INTERVAL=y
>
> At runtime with use of module option:
>
>     enable_interval
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
>  drivers/bluetooth/Kconfig |  7 ++++
>  drivers/bluetooth/btusb.c | 88 ++++++++++++++++++++++++++++++++++-----
>  2 files changed, 84 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 4e73a531b377..2f20a853d946 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -41,6 +41,13 @@ config BT_HCIBTUSB_AUTOSUSPEND
>           This can be overridden by passing btusb.enable_autosuspend=[y|n]
>           on the kernel commandline.
>
> +config BT_HCIBTUSB_INTERVAL
> +       bool "Enable notification of USB polling interval"
> +       depends on BT_HCIBTUSB
> +       help
> +         Say Y here to enable notification of USB polling interval for
> +         Bluetooth USB devices by default.
> +
>  config BT_HCIBTUSB_BCM
>         bool "Broadcom protocol support"
>         depends on BT_HCIBTUSB
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index b14102fba601..38cb5448fc69 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -30,7 +30,7 @@
>  static bool disable_scofix;
>  static bool force_scofix;
>  static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> -
> +static bool enable_interval = IS_ENABLED(CONFIG_BT_HCIBTUSB_INTERVAL);
>  static bool reset = true;
>
>  static struct usb_driver btusb_driver;
> @@ -519,8 +519,12 @@ struct btusb_data {
>
>         unsigned long flags;
>
> -       struct work_struct work;
> -       struct work_struct waker;
> +       struct work_struct  work;
> +       struct work_struct  waker;
> +       struct delayed_work rx_work;
> +
> +       struct sk_buff_head acl_q;
> +       struct sk_buff_head evt_q;
>
>         struct usb_anchor deferred;
>         struct usb_anchor tx_anchor;
> @@ -557,7 +561,7 @@ struct btusb_data {
>         int isoc_altsetting;
>         int suspend_count;
>
> -       int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> +       int (*recv_event)(struct btusb_data *data, struct sk_buff *skb);
>         int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
>
>         int (*setup_on_usb)(struct hci_dev *hdev);
> @@ -707,7 +711,7 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
>
>                 if (!hci_skb_expect(skb)) {
>                         /* Complete frame */
> -                       data->recv_event(data->hdev, skb);
> +                       data->recv_event(data, skb);
>                         skb = NULL;
>                 }
>         }
> @@ -718,6 +722,25 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
>         return err;
>  }
>
> +static int btusb_rx_queue(struct btusb_data *data, struct sk_buff *skb,
> +                         struct sk_buff_head *queue, unsigned int interval)
> +{
> +       skb_queue_tail(queue, skb);
> +
> +       schedule_delayed_work(&data->rx_work, interval);
> +
> +       return 0;
> +}
> +
> +static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
> +{
> +       if (!enable_interval)
> +               return hci_recv_frame(data->hdev, skb);
> +
> +       /* TODO: Calculate polling interval based on endpoint bInterval? */
> +       return btusb_rx_queue(data, skb, &data->acl_q, msecs_to_jiffies(1));
> +}
> +
>  static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
>  {
>         struct sk_buff *skb;
> @@ -765,7 +788,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
>
>                 if (!hci_skb_expect(skb)) {
>                         /* Complete frame */
> -                       hci_recv_frame(data->hdev, skb);
> +                       btusb_recv_acl(data, skb);
>                         skb = NULL;
>                 }
>         }
> @@ -1383,9 +1406,13 @@ static int btusb_close(struct hci_dev *hdev)
>
>         BT_DBG("%s", hdev->name);
>
> +       cancel_delayed_work(&data->rx_work);
>         cancel_work_sync(&data->work);
>         cancel_work_sync(&data->waker);
>
> +       skb_queue_purge(&data->acl_q);
> +       skb_queue_purge(&data->evt_q);
> +
>         clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
>         clear_bit(BTUSB_BULK_RUNNING, &data->flags);
>         clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> @@ -1417,6 +1444,11 @@ static int btusb_flush(struct hci_dev *hdev)
>
>         BT_DBG("%s", hdev->name);
>
> +       cancel_delayed_work(&data->rx_work);
> +
> +       skb_queue_purge(&data->acl_q);
> +       skb_queue_purge(&data->evt_q);
> +
>         usb_kill_anchored_urbs(&data->tx_anchor);
>         btusb_free_frags(data);
>
> @@ -1769,6 +1801,25 @@ static void btusb_waker(struct work_struct *work)
>         usb_autopm_put_interface(data->intf);
>  }
>
> +static void btusb_rx_dequeue(struct btusb_data *data,
> +                            struct sk_buff_head *queue)
> +{
> +       struct sk_buff *skb;
> +
> +       while ((skb = skb_dequeue(queue)))
> +               hci_recv_frame(data->hdev, skb);
> +}
> +
> +static void btusb_rx_work(struct work_struct *work)
> +{
> +       struct btusb_data *data = container_of(work, struct btusb_data,
> +                                              rx_work.work);
> +
> +       /* Process HCI event packets so states changes are synchronized first */
> +       btusb_rx_dequeue(data, &data->evt_q);
> +       btusb_rx_dequeue(data, &data->acl_q);
> +}
> +
>  static int btusb_setup_bcm92035(struct hci_dev *hdev)
>  {
>         struct sk_buff *skb;
> @@ -2304,10 +2355,8 @@ static void btusb_intel_secure_send_result(struct btusb_data *data,
>                 wake_up_bit(&data->flags, BTUSB_DOWNLOADING);
>  }
>
> -static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> +static int btusb_recv_event_intel(struct btusb_data *data, struct sk_buff *skb)
>  {
> -       struct btusb_data *data = hci_get_drvdata(hdev);
> -
>         if (test_bit(BTUSB_BOOTLOADER, &data->flags)) {
>                 struct hci_event_hdr *hdr = (void *)skb->data;
>
> @@ -2336,7 +2385,7 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
>                 }
>         }
>
> -       return hci_recv_frame(hdev, skb);
> +       return hci_recv_frame(data->hdev, skb);
>  }
>
>  static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
> @@ -4279,6 +4328,15 @@ static int btusb_shutdown_qca(struct hci_dev *hdev)
>         return 0;
>  }
>
> +static int btusb_recv_evt(struct btusb_data *data, struct sk_buff *skb)
> +{
> +       if (!enable_interval)
> +               return hci_recv_frame(data->hdev, skb);
> +
> +       /* Don't delay event processing */
> +       return btusb_rx_queue(data, skb, &data->evt_q, 0);
> +}
> +
>  static int btusb_probe(struct usb_interface *intf,
>                        const struct usb_device_id *id)
>  {
> @@ -4362,6 +4420,11 @@ static int btusb_probe(struct usb_interface *intf,
>
>         INIT_WORK(&data->work, btusb_work);
>         INIT_WORK(&data->waker, btusb_waker);
> +       INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work);
> +
> +       skb_queue_head_init(&data->acl_q);
> +       skb_queue_head_init(&data->evt_q);
> +
>         init_usb_anchor(&data->deferred);
>         init_usb_anchor(&data->tx_anchor);
>         spin_lock_init(&data->txlock);
> @@ -4378,7 +4441,7 @@ static int btusb_probe(struct usb_interface *intf,
>                 data->recv_bulk = btusb_recv_bulk_intel;
>                 set_bit(BTUSB_BOOTLOADER, &data->flags);
>         } else {
> -               data->recv_event = hci_recv_frame;
> +               data->recv_event = btusb_recv_evt;
>                 data->recv_bulk = btusb_recv_bulk;
>         }
>
> @@ -4867,6 +4930,9 @@ MODULE_PARM_DESC(force_scofix, "Force fixup of wrong SCO buffers size");
>  module_param(enable_autosuspend, bool, 0644);
>  MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
>
> +module_param(enable_interval, bool, 0644);
> +MODULE_PARM_DESC(enable_interval, "Enable USB polling interval by default");
> +
>  module_param(reset, bool, 0644);
>  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
>
> --
> 2.26.2

Can you test these with the set of tests you guys have for Chrome OS?

-- 
Luiz Augusto von Dentz

Re: [RESEND 1/2] Bluetooth: btusb: Add support for queuing during polling interval

[Abhishek Pandit-Subedi]

+ Archie, Alain

It looks like we already landed this change into Chrome a while ago
and we haven't had any issues.
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2312116

Archie, can you confirm that this is the case?

Thanks
Abhishek

On Wed, Jan 13, 2021 at 3:33 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Abhishek,
>
> On Wed, Jan 13, 2021 at 3:28 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
> > This makes btusb to queue ACL and events during a polling interval
> > by using of a delayed work, with the interval working as a time window
> > where frames received from different endpoints are considered to be
> > arrived at same time and then attempt to resolve potential conflics by
> > processing the events ahead of ACL packets.
> >
> > It worth noting though that priorizing events over ACL data may result
> > in inverting the order compared to how they appeared over the air, for
> > instance there may be packets received before a disconnect event that
> > will be discarded and unencrypted packets received before encryption
> > change which would considered encrypted, because of these potential
> > changes on the order the support for queuing during the polling
> > interval is not enabled by default so platforms have the following
> > means to enable it:
> >
> > At build-time:
> >
> >     CONFIG_BT_HCIBTUSB_INTERVAL=y
> >
> > At runtime with use of module option:
> >
> >     enable_interval
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> >  drivers/bluetooth/Kconfig |  7 ++++
> >  drivers/bluetooth/btusb.c | 88 ++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 84 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > index 4e73a531b377..2f20a853d946 100644
> > --- a/drivers/bluetooth/Kconfig
> > +++ b/drivers/bluetooth/Kconfig
> > @@ -41,6 +41,13 @@ config BT_HCIBTUSB_AUTOSUSPEND
> >           This can be overridden by passing btusb.enable_autosuspend=[y|n]
> >           on the kernel commandline.
> >
> > +config BT_HCIBTUSB_INTERVAL
> > +       bool "Enable notification of USB polling interval"
> > +       depends on BT_HCIBTUSB
> > +       help
> > +         Say Y here to enable notification of USB polling interval for
> > +         Bluetooth USB devices by default.
> > +
> >  config BT_HCIBTUSB_BCM
> >         bool "Broadcom protocol support"
> >         depends on BT_HCIBTUSB
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index b14102fba601..38cb5448fc69 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -30,7 +30,7 @@
> >  static bool disable_scofix;
> >  static bool force_scofix;
> >  static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> > -
> > +static bool enable_interval = IS_ENABLED(CONFIG_BT_HCIBTUSB_INTERVAL);
> >  static bool reset = true;
> >
> >  static struct usb_driver btusb_driver;
> > @@ -519,8 +519,12 @@ struct btusb_data {
> >
> >         unsigned long flags;
> >
> > -       struct work_struct work;
> > -       struct work_struct waker;
> > +       struct work_struct  work;
> > +       struct work_struct  waker;
> > +       struct delayed_work rx_work;
> > +
> > +       struct sk_buff_head acl_q;
> > +       struct sk_buff_head evt_q;
> >
> >         struct usb_anchor deferred;
> >         struct usb_anchor tx_anchor;
> > @@ -557,7 +561,7 @@ struct btusb_data {
> >         int isoc_altsetting;
> >         int suspend_count;
> >
> > -       int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> > +       int (*recv_event)(struct btusb_data *data, struct sk_buff *skb);
> >         int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
> >
> >         int (*setup_on_usb)(struct hci_dev *hdev);
> > @@ -707,7 +711,7 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> >
> >                 if (!hci_skb_expect(skb)) {
> >                         /* Complete frame */
> > -                       data->recv_event(data->hdev, skb);
> > +                       data->recv_event(data, skb);
> >                         skb = NULL;
> >                 }
> >         }
> > @@ -718,6 +722,25 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> >         return err;
> >  }
> >
> > +static int btusb_rx_queue(struct btusb_data *data, struct sk_buff *skb,
> > +                         struct sk_buff_head *queue, unsigned int interval)
> > +{
> > +       skb_queue_tail(queue, skb);
> > +
> > +       schedule_delayed_work(&data->rx_work, interval);
> > +
> > +       return 0;
> > +}
> > +
> > +static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
> > +{
> > +       if (!enable_interval)
> > +               return hci_recv_frame(data->hdev, skb);
> > +
> > +       /* TODO: Calculate polling interval based on endpoint bInterval? */
> > +       return btusb_rx_queue(data, skb, &data->acl_q, msecs_to_jiffies(1));
> > +}
> > +
> >  static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> >  {
> >         struct sk_buff *skb;
> > @@ -765,7 +788,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> >
> >                 if (!hci_skb_expect(skb)) {
> >                         /* Complete frame */
> > -                       hci_recv_frame(data->hdev, skb);
> > +                       btusb_recv_acl(data, skb);
> >                         skb = NULL;
> >                 }
> >         }
> > @@ -1383,9 +1406,13 @@ static int btusb_close(struct hci_dev *hdev)
> >
> >         BT_DBG("%s", hdev->name);
> >
> > +       cancel_delayed_work(&data->rx_work);
> >         cancel_work_sync(&data->work);
> >         cancel_work_sync(&data->waker);
> >
> > +       skb_queue_purge(&data->acl_q);
> > +       skb_queue_purge(&data->evt_q);
> > +
> >         clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> >         clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> >         clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> > @@ -1417,6 +1444,11 @@ static int btusb_flush(struct hci_dev *hdev)
> >
> >         BT_DBG("%s", hdev->name);
> >
> > +       cancel_delayed_work(&data->rx_work);
> > +
> > +       skb_queue_purge(&data->acl_q);
> > +       skb_queue_purge(&data->evt_q);
> > +
> >         usb_kill_anchored_urbs(&data->tx_anchor);
> >         btusb_free_frags(data);
> >
> > @@ -1769,6 +1801,25 @@ static void btusb_waker(struct work_struct *work)
> >         usb_autopm_put_interface(data->intf);
> >  }
> >
> > +static void btusb_rx_dequeue(struct btusb_data *data,
> > +                            struct sk_buff_head *queue)
> > +{
> > +       struct sk_buff *skb;
> > +
> > +       while ((skb = skb_dequeue(queue)))
> > +               hci_recv_frame(data->hdev, skb);
> > +}
> > +
> > +static void btusb_rx_work(struct work_struct *work)
> > +{
> > +       struct btusb_data *data = container_of(work, struct btusb_data,
> > +                                              rx_work.work);
> > +
> > +       /* Process HCI event packets so states changes are synchronized first */
> > +       btusb_rx_dequeue(data, &data->evt_q);
> > +       btusb_rx_dequeue(data, &data->acl_q);
> > +}
> > +
> >  static int btusb_setup_bcm92035(struct hci_dev *hdev)
> >  {
> >         struct sk_buff *skb;
> > @@ -2304,10 +2355,8 @@ static void btusb_intel_secure_send_result(struct btusb_data *data,
> >                 wake_up_bit(&data->flags, BTUSB_DOWNLOADING);
> >  }
> >
> > -static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > +static int btusb_recv_event_intel(struct btusb_data *data, struct sk_buff *skb)
> >  {
> > -       struct btusb_data *data = hci_get_drvdata(hdev);
> > -
> >         if (test_bit(BTUSB_BOOTLOADER, &data->flags)) {
> >                 struct hci_event_hdr *hdr = (void *)skb->data;
> >
> > @@ -2336,7 +2385,7 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> >                 }
> >         }
> >
> > -       return hci_recv_frame(hdev, skb);
> > +       return hci_recv_frame(data->hdev, skb);
> >  }
> >
> >  static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > @@ -4279,6 +4328,15 @@ static int btusb_shutdown_qca(struct hci_dev *hdev)
> >         return 0;
> >  }
> >
> > +static int btusb_recv_evt(struct btusb_data *data, struct sk_buff *skb)
> > +{
> > +       if (!enable_interval)
> > +               return hci_recv_frame(data->hdev, skb);
> > +
> > +       /* Don't delay event processing */
> > +       return btusb_rx_queue(data, skb, &data->evt_q, 0);
> > +}
> > +
> >  static int btusb_probe(struct usb_interface *intf,
> >                        const struct usb_device_id *id)
> >  {
> > @@ -4362,6 +4420,11 @@ static int btusb_probe(struct usb_interface *intf,
> >
> >         INIT_WORK(&data->work, btusb_work);
> >         INIT_WORK(&data->waker, btusb_waker);
> > +       INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work);
> > +
> > +       skb_queue_head_init(&data->acl_q);
> > +       skb_queue_head_init(&data->evt_q);
> > +
> >         init_usb_anchor(&data->deferred);
> >         init_usb_anchor(&data->tx_anchor);
> >         spin_lock_init(&data->txlock);
> > @@ -4378,7 +4441,7 @@ static int btusb_probe(struct usb_interface *intf,
> >                 data->recv_bulk = btusb_recv_bulk_intel;
> >                 set_bit(BTUSB_BOOTLOADER, &data->flags);
> >         } else {
> > -               data->recv_event = hci_recv_frame;
> > +               data->recv_event = btusb_recv_evt;
> >                 data->recv_bulk = btusb_recv_bulk;
> >         }
> >
> > @@ -4867,6 +4930,9 @@ MODULE_PARM_DESC(force_scofix, "Force fixup of wrong SCO buffers size");
> >  module_param(enable_autosuspend, bool, 0644);
> >  MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> >
> > +module_param(enable_interval, bool, 0644);
> > +MODULE_PARM_DESC(enable_interval, "Enable USB polling interval by default");
> > +
> >  module_param(reset, bool, 0644);
> >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> >
> > --
> > 2.26.2
>
> Can you test these with the set of tests you guys have for Chrome OS?
>
> --
> Luiz Augusto von Dentz

Re: [RESEND 1/2] Bluetooth: btusb: Add support for queuing during polling interval

[Luiz Augusto von Dentz]

Hi Abhishek,

On Wed, Jan 13, 2021 at 3:48 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> + Archie, Alain
>
> It looks like we already landed this change into Chrome a while ago
> and we haven't had any issues.
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2312116

Good to know, but can you also confirm it is running with
CONFIG_BT_HCIBTUSB_INTERVAL=y?

> Archie, can you confirm that this is the case?
>
> Thanks
> Abhishek
>
> On Wed, Jan 13, 2021 at 3:33 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Abhishek,
> >
> > On Wed, Jan 13, 2021 at 3:28 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > >
> > > This makes btusb to queue ACL and events during a polling interval
> > > by using of a delayed work, with the interval working as a time window
> > > where frames received from different endpoints are considered to be
> > > arrived at same time and then attempt to resolve potential conflics by
> > > processing the events ahead of ACL packets.
> > >
> > > It worth noting though that priorizing events over ACL data may result
> > > in inverting the order compared to how they appeared over the air, for
> > > instance there may be packets received before a disconnect event that
> > > will be discarded and unencrypted packets received before encryption
> > > change which would considered encrypted, because of these potential
> > > changes on the order the support for queuing during the polling
> > > interval is not enabled by default so platforms have the following
> > > means to enable it:
> > >
> > > At build-time:
> > >
> > >     CONFIG_BT_HCIBTUSB_INTERVAL=y
> > >
> > > At runtime with use of module option:
> > >
> > >     enable_interval
> > >
> > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > ---
> > >  drivers/bluetooth/Kconfig |  7 ++++
> > >  drivers/bluetooth/btusb.c | 88 ++++++++++++++++++++++++++++++++++-----
> > >  2 files changed, 84 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > > index 4e73a531b377..2f20a853d946 100644
> > > --- a/drivers/bluetooth/Kconfig
> > > +++ b/drivers/bluetooth/Kconfig
> > > @@ -41,6 +41,13 @@ config BT_HCIBTUSB_AUTOSUSPEND
> > >           This can be overridden by passing btusb.enable_autosuspend=[y|n]
> > >           on the kernel commandline.
> > >
> > > +config BT_HCIBTUSB_INTERVAL
> > > +       bool "Enable notification of USB polling interval"
> > > +       depends on BT_HCIBTUSB
> > > +       help
> > > +         Say Y here to enable notification of USB polling interval for
> > > +         Bluetooth USB devices by default.
> > > +
> > >  config BT_HCIBTUSB_BCM
> > >         bool "Broadcom protocol support"
> > >         depends on BT_HCIBTUSB
> > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > index b14102fba601..38cb5448fc69 100644
> > > --- a/drivers/bluetooth/btusb.c
> > > +++ b/drivers/bluetooth/btusb.c
> > > @@ -30,7 +30,7 @@
> > >  static bool disable_scofix;
> > >  static bool force_scofix;
> > >  static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> > > -
> > > +static bool enable_interval = IS_ENABLED(CONFIG_BT_HCIBTUSB_INTERVAL);
> > >  static bool reset = true;
> > >
> > >  static struct usb_driver btusb_driver;
> > > @@ -519,8 +519,12 @@ struct btusb_data {
> > >
> > >         unsigned long flags;
> > >
> > > -       struct work_struct work;
> > > -       struct work_struct waker;
> > > +       struct work_struct  work;
> > > +       struct work_struct  waker;
> > > +       struct delayed_work rx_work;
> > > +
> > > +       struct sk_buff_head acl_q;
> > > +       struct sk_buff_head evt_q;
> > >
> > >         struct usb_anchor deferred;
> > >         struct usb_anchor tx_anchor;
> > > @@ -557,7 +561,7 @@ struct btusb_data {
> > >         int isoc_altsetting;
> > >         int suspend_count;
> > >
> > > -       int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> > > +       int (*recv_event)(struct btusb_data *data, struct sk_buff *skb);
> > >         int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
> > >
> > >         int (*setup_on_usb)(struct hci_dev *hdev);
> > > @@ -707,7 +711,7 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> > >
> > >                 if (!hci_skb_expect(skb)) {
> > >                         /* Complete frame */
> > > -                       data->recv_event(data->hdev, skb);
> > > +                       data->recv_event(data, skb);
> > >                         skb = NULL;
> > >                 }
> > >         }
> > > @@ -718,6 +722,25 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> > >         return err;
> > >  }
> > >
> > > +static int btusb_rx_queue(struct btusb_data *data, struct sk_buff *skb,
> > > +                         struct sk_buff_head *queue, unsigned int interval)
> > > +{
> > > +       skb_queue_tail(queue, skb);
> > > +
> > > +       schedule_delayed_work(&data->rx_work, interval);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
> > > +{
> > > +       if (!enable_interval)
> > > +               return hci_recv_frame(data->hdev, skb);
> > > +
> > > +       /* TODO: Calculate polling interval based on endpoint bInterval? */
> > > +       return btusb_rx_queue(data, skb, &data->acl_q, msecs_to_jiffies(1));
> > > +}
> > > +
> > >  static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> > >  {
> > >         struct sk_buff *skb;
> > > @@ -765,7 +788,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> > >
> > >                 if (!hci_skb_expect(skb)) {
> > >                         /* Complete frame */
> > > -                       hci_recv_frame(data->hdev, skb);
> > > +                       btusb_recv_acl(data, skb);
> > >                         skb = NULL;
> > >                 }
> > >         }
> > > @@ -1383,9 +1406,13 @@ static int btusb_close(struct hci_dev *hdev)
> > >
> > >         BT_DBG("%s", hdev->name);
> > >
> > > +       cancel_delayed_work(&data->rx_work);
> > >         cancel_work_sync(&data->work);
> > >         cancel_work_sync(&data->waker);
> > >
> > > +       skb_queue_purge(&data->acl_q);
> > > +       skb_queue_purge(&data->evt_q);
> > > +
> > >         clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > >         clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> > >         clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> > > @@ -1417,6 +1444,11 @@ static int btusb_flush(struct hci_dev *hdev)
> > >
> > >         BT_DBG("%s", hdev->name);
> > >
> > > +       cancel_delayed_work(&data->rx_work);
> > > +
> > > +       skb_queue_purge(&data->acl_q);
> > > +       skb_queue_purge(&data->evt_q);
> > > +
> > >         usb_kill_anchored_urbs(&data->tx_anchor);
> > >         btusb_free_frags(data);
> > >
> > > @@ -1769,6 +1801,25 @@ static void btusb_waker(struct work_struct *work)
> > >         usb_autopm_put_interface(data->intf);
> > >  }
> > >
> > > +static void btusb_rx_dequeue(struct btusb_data *data,
> > > +                            struct sk_buff_head *queue)
> > > +{
> > > +       struct sk_buff *skb;
> > > +
> > > +       while ((skb = skb_dequeue(queue)))
> > > +               hci_recv_frame(data->hdev, skb);
> > > +}
> > > +
> > > +static void btusb_rx_work(struct work_struct *work)
> > > +{
> > > +       struct btusb_data *data = container_of(work, struct btusb_data,
> > > +                                              rx_work.work);
> > > +
> > > +       /* Process HCI event packets so states changes are synchronized first */
> > > +       btusb_rx_dequeue(data, &data->evt_q);
> > > +       btusb_rx_dequeue(data, &data->acl_q);
> > > +}
> > > +
> > >  static int btusb_setup_bcm92035(struct hci_dev *hdev)
> > >  {
> > >         struct sk_buff *skb;
> > > @@ -2304,10 +2355,8 @@ static void btusb_intel_secure_send_result(struct btusb_data *data,
> > >                 wake_up_bit(&data->flags, BTUSB_DOWNLOADING);
> > >  }
> > >
> > > -static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > +static int btusb_recv_event_intel(struct btusb_data *data, struct sk_buff *skb)
> > >  {
> > > -       struct btusb_data *data = hci_get_drvdata(hdev);
> > > -
> > >         if (test_bit(BTUSB_BOOTLOADER, &data->flags)) {
> > >                 struct hci_event_hdr *hdr = (void *)skb->data;
> > >
> > > @@ -2336,7 +2385,7 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > >                 }
> > >         }
> > >
> > > -       return hci_recv_frame(hdev, skb);
> > > +       return hci_recv_frame(data->hdev, skb);
> > >  }
> > >
> > >  static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > @@ -4279,6 +4328,15 @@ static int btusb_shutdown_qca(struct hci_dev *hdev)
> > >         return 0;
> > >  }
> > >
> > > +static int btusb_recv_evt(struct btusb_data *data, struct sk_buff *skb)
> > > +{
> > > +       if (!enable_interval)
> > > +               return hci_recv_frame(data->hdev, skb);
> > > +
> > > +       /* Don't delay event processing */
> > > +       return btusb_rx_queue(data, skb, &data->evt_q, 0);
> > > +}
> > > +
> > >  static int btusb_probe(struct usb_interface *intf,
> > >                        const struct usb_device_id *id)
> > >  {
> > > @@ -4362,6 +4420,11 @@ static int btusb_probe(struct usb_interface *intf,
> > >
> > >         INIT_WORK(&data->work, btusb_work);
> > >         INIT_WORK(&data->waker, btusb_waker);
> > > +       INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work);
> > > +
> > > +       skb_queue_head_init(&data->acl_q);
> > > +       skb_queue_head_init(&data->evt_q);
> > > +
> > >         init_usb_anchor(&data->deferred);
> > >         init_usb_anchor(&data->tx_anchor);
> > >         spin_lock_init(&data->txlock);
> > > @@ -4378,7 +4441,7 @@ static int btusb_probe(struct usb_interface *intf,
> > >                 data->recv_bulk = btusb_recv_bulk_intel;
> > >                 set_bit(BTUSB_BOOTLOADER, &data->flags);
> > >         } else {
> > > -               data->recv_event = hci_recv_frame;
> > > +               data->recv_event = btusb_recv_evt;
> > >                 data->recv_bulk = btusb_recv_bulk;
> > >         }
> > >
> > > @@ -4867,6 +4930,9 @@ MODULE_PARM_DESC(force_scofix, "Force fixup of wrong SCO buffers size");
> > >  module_param(enable_autosuspend, bool, 0644);
> > >  MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> > >
> > > +module_param(enable_interval, bool, 0644);
> > > +MODULE_PARM_DESC(enable_interval, "Enable USB polling interval by default");
> > > +
> > >  module_param(reset, bool, 0644);
> > >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> > >
> > > --
> > > 2.26.2
> >
> > Can you test these with the set of tests you guys have for Chrome OS?
> >
> > --
> > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [RESEND 1/2] Bluetooth: btusb: Add support for queuing during polling interval

[Luiz Augusto von Dentz]

Hi,

On Wed, Jan 13, 2021 at 3:51 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Abhishek,
>
> On Wed, Jan 13, 2021 at 3:48 PM Abhishek Pandit-Subedi
> <abhishekpandit@chromium.org> wrote:
> >
> > + Archie, Alain
> >
> > It looks like we already landed this change into Chrome a while ago
> > and we haven't had any issues.
> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2312116
>
> Good to know, but can you also confirm it is running with
> CONFIG_BT_HCIBTUSB_INTERVAL=y?

Correction, CONFIG_BT_HCIBTUSB_INTERVAL=y or passing enable_interval to btusb.

> > Archie, can you confirm that this is the case?
> >
> > Thanks
> > Abhishek
> >
> > On Wed, Jan 13, 2021 at 3:33 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Abhishek,
> > >
> > > On Wed, Jan 13, 2021 at 3:28 PM Luiz Augusto von Dentz
> > > <luiz.dentz@gmail.com> wrote:
> > > >
> > > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > >
> > > > This makes btusb to queue ACL and events during a polling interval
> > > > by using of a delayed work, with the interval working as a time window
> > > > where frames received from different endpoints are considered to be
> > > > arrived at same time and then attempt to resolve potential conflics by
> > > > processing the events ahead of ACL packets.
> > > >
> > > > It worth noting though that priorizing events over ACL data may result
> > > > in inverting the order compared to how they appeared over the air, for
> > > > instance there may be packets received before a disconnect event that
> > > > will be discarded and unencrypted packets received before encryption
> > > > change which would considered encrypted, because of these potential
> > > > changes on the order the support for queuing during the polling
> > > > interval is not enabled by default so platforms have the following
> > > > means to enable it:
> > > >
> > > > At build-time:
> > > >
> > > >     CONFIG_BT_HCIBTUSB_INTERVAL=y
> > > >
> > > > At runtime with use of module option:
> > > >
> > > >     enable_interval
> > > >
> > > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > > ---
> > > >  drivers/bluetooth/Kconfig |  7 ++++
> > > >  drivers/bluetooth/btusb.c | 88 ++++++++++++++++++++++++++++++++++-----
> > > >  2 files changed, 84 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > > > index 4e73a531b377..2f20a853d946 100644
> > > > --- a/drivers/bluetooth/Kconfig
> > > > +++ b/drivers/bluetooth/Kconfig
> > > > @@ -41,6 +41,13 @@ config BT_HCIBTUSB_AUTOSUSPEND
> > > >           This can be overridden by passing btusb.enable_autosuspend=[y|n]
> > > >           on the kernel commandline.
> > > >
> > > > +config BT_HCIBTUSB_INTERVAL
> > > > +       bool "Enable notification of USB polling interval"
> > > > +       depends on BT_HCIBTUSB
> > > > +       help
> > > > +         Say Y here to enable notification of USB polling interval for
> > > > +         Bluetooth USB devices by default.
> > > > +
> > > >  config BT_HCIBTUSB_BCM
> > > >         bool "Broadcom protocol support"
> > > >         depends on BT_HCIBTUSB
> > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > > index b14102fba601..38cb5448fc69 100644
> > > > --- a/drivers/bluetooth/btusb.c
> > > > +++ b/drivers/bluetooth/btusb.c
> > > > @@ -30,7 +30,7 @@
> > > >  static bool disable_scofix;
> > > >  static bool force_scofix;
> > > >  static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> > > > -
> > > > +static bool enable_interval = IS_ENABLED(CONFIG_BT_HCIBTUSB_INTERVAL);
> > > >  static bool reset = true;
> > > >
> > > >  static struct usb_driver btusb_driver;
> > > > @@ -519,8 +519,12 @@ struct btusb_data {
> > > >
> > > >         unsigned long flags;
> > > >
> > > > -       struct work_struct work;
> > > > -       struct work_struct waker;
> > > > +       struct work_struct  work;
> > > > +       struct work_struct  waker;
> > > > +       struct delayed_work rx_work;
> > > > +
> > > > +       struct sk_buff_head acl_q;
> > > > +       struct sk_buff_head evt_q;
> > > >
> > > >         struct usb_anchor deferred;
> > > >         struct usb_anchor tx_anchor;
> > > > @@ -557,7 +561,7 @@ struct btusb_data {
> > > >         int isoc_altsetting;
> > > >         int suspend_count;
> > > >
> > > > -       int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> > > > +       int (*recv_event)(struct btusb_data *data, struct sk_buff *skb);
> > > >         int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
> > > >
> > > >         int (*setup_on_usb)(struct hci_dev *hdev);
> > > > @@ -707,7 +711,7 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> > > >
> > > >                 if (!hci_skb_expect(skb)) {
> > > >                         /* Complete frame */
> > > > -                       data->recv_event(data->hdev, skb);
> > > > +                       data->recv_event(data, skb);
> > > >                         skb = NULL;
> > > >                 }
> > > >         }
> > > > @@ -718,6 +722,25 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> > > >         return err;
> > > >  }
> > > >
> > > > +static int btusb_rx_queue(struct btusb_data *data, struct sk_buff *skb,
> > > > +                         struct sk_buff_head *queue, unsigned int interval)
> > > > +{
> > > > +       skb_queue_tail(queue, skb);
> > > > +
> > > > +       schedule_delayed_work(&data->rx_work, interval);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
> > > > +{
> > > > +       if (!enable_interval)
> > > > +               return hci_recv_frame(data->hdev, skb);
> > > > +
> > > > +       /* TODO: Calculate polling interval based on endpoint bInterval? */
> > > > +       return btusb_rx_queue(data, skb, &data->acl_q, msecs_to_jiffies(1));
> > > > +}
> > > > +
> > > >  static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> > > >  {
> > > >         struct sk_buff *skb;
> > > > @@ -765,7 +788,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> > > >
> > > >                 if (!hci_skb_expect(skb)) {
> > > >                         /* Complete frame */
> > > > -                       hci_recv_frame(data->hdev, skb);
> > > > +                       btusb_recv_acl(data, skb);
> > > >                         skb = NULL;
> > > >                 }
> > > >         }
> > > > @@ -1383,9 +1406,13 @@ static int btusb_close(struct hci_dev *hdev)
> > > >
> > > >         BT_DBG("%s", hdev->name);
> > > >
> > > > +       cancel_delayed_work(&data->rx_work);
> > > >         cancel_work_sync(&data->work);
> > > >         cancel_work_sync(&data->waker);
> > > >
> > > > +       skb_queue_purge(&data->acl_q);
> > > > +       skb_queue_purge(&data->evt_q);
> > > > +
> > > >         clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > > >         clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> > > >         clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> > > > @@ -1417,6 +1444,11 @@ static int btusb_flush(struct hci_dev *hdev)
> > > >
> > > >         BT_DBG("%s", hdev->name);
> > > >
> > > > +       cancel_delayed_work(&data->rx_work);
> > > > +
> > > > +       skb_queue_purge(&data->acl_q);
> > > > +       skb_queue_purge(&data->evt_q);
> > > > +
> > > >         usb_kill_anchored_urbs(&data->tx_anchor);
> > > >         btusb_free_frags(data);
> > > >
> > > > @@ -1769,6 +1801,25 @@ static void btusb_waker(struct work_struct *work)
> > > >         usb_autopm_put_interface(data->intf);
> > > >  }
> > > >
> > > > +static void btusb_rx_dequeue(struct btusb_data *data,
> > > > +                            struct sk_buff_head *queue)
> > > > +{
> > > > +       struct sk_buff *skb;
> > > > +
> > > > +       while ((skb = skb_dequeue(queue)))
> > > > +               hci_recv_frame(data->hdev, skb);
> > > > +}
> > > > +
> > > > +static void btusb_rx_work(struct work_struct *work)
> > > > +{
> > > > +       struct btusb_data *data = container_of(work, struct btusb_data,
> > > > +                                              rx_work.work);
> > > > +
> > > > +       /* Process HCI event packets so states changes are synchronized first */
> > > > +       btusb_rx_dequeue(data, &data->evt_q);
> > > > +       btusb_rx_dequeue(data, &data->acl_q);
> > > > +}
> > > > +
> > > >  static int btusb_setup_bcm92035(struct hci_dev *hdev)
> > > >  {
> > > >         struct sk_buff *skb;
> > > > @@ -2304,10 +2355,8 @@ static void btusb_intel_secure_send_result(struct btusb_data *data,
> > > >                 wake_up_bit(&data->flags, BTUSB_DOWNLOADING);
> > > >  }
> > > >
> > > > -static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > > +static int btusb_recv_event_intel(struct btusb_data *data, struct sk_buff *skb)
> > > >  {
> > > > -       struct btusb_data *data = hci_get_drvdata(hdev);
> > > > -
> > > >         if (test_bit(BTUSB_BOOTLOADER, &data->flags)) {
> > > >                 struct hci_event_hdr *hdr = (void *)skb->data;
> > > >
> > > > @@ -2336,7 +2385,7 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > >                 }
> > > >         }
> > > >
> > > > -       return hci_recv_frame(hdev, skb);
> > > > +       return hci_recv_frame(data->hdev, skb);
> > > >  }
> > > >
> > > >  static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > > @@ -4279,6 +4328,15 @@ static int btusb_shutdown_qca(struct hci_dev *hdev)
> > > >         return 0;
> > > >  }
> > > >
> > > > +static int btusb_recv_evt(struct btusb_data *data, struct sk_buff *skb)
> > > > +{
> > > > +       if (!enable_interval)
> > > > +               return hci_recv_frame(data->hdev, skb);
> > > > +
> > > > +       /* Don't delay event processing */
> > > > +       return btusb_rx_queue(data, skb, &data->evt_q, 0);
> > > > +}
> > > > +
> > > >  static int btusb_probe(struct usb_interface *intf,
> > > >                        const struct usb_device_id *id)
> > > >  {
> > > > @@ -4362,6 +4420,11 @@ static int btusb_probe(struct usb_interface *intf,
> > > >
> > > >         INIT_WORK(&data->work, btusb_work);
> > > >         INIT_WORK(&data->waker, btusb_waker);
> > > > +       INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work);
> > > > +
> > > > +       skb_queue_head_init(&data->acl_q);
> > > > +       skb_queue_head_init(&data->evt_q);
> > > > +
> > > >         init_usb_anchor(&data->deferred);
> > > >         init_usb_anchor(&data->tx_anchor);
> > > >         spin_lock_init(&data->txlock);
> > > > @@ -4378,7 +4441,7 @@ static int btusb_probe(struct usb_interface *intf,
> > > >                 data->recv_bulk = btusb_recv_bulk_intel;
> > > >                 set_bit(BTUSB_BOOTLOADER, &data->flags);
> > > >         } else {
> > > > -               data->recv_event = hci_recv_frame;
> > > > +               data->recv_event = btusb_recv_evt;
> > > >                 data->recv_bulk = btusb_recv_bulk;
> > > >         }
> > > >
> > > > @@ -4867,6 +4930,9 @@ MODULE_PARM_DESC(force_scofix, "Force fixup of wrong SCO buffers size");
> > > >  module_param(enable_autosuspend, bool, 0644);
> > > >  MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> > > >
> > > > +module_param(enable_interval, bool, 0644);
> > > > +MODULE_PARM_DESC(enable_interval, "Enable USB polling interval by default");
> > > > +
> > > >  module_param(reset, bool, 0644);
> > > >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> > > >
> > > > --
> > > > 2.26.2
> > >
> > > Can you test these with the set of tests you guys have for Chrome OS?
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz



--
Luiz Augusto von Dentz

Re: [RESEND 1/2] Bluetooth: btusb: Add support for queuing during polling interval

[Abhishek Pandit-Subedi]

Hi Luiz,

Yes, we have that enabled:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2312117/3

Abhishek

On Wed, Jan 13, 2021 at 3:53 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi,
>
> On Wed, Jan 13, 2021 at 3:51 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Abhishek,
> >
> > On Wed, Jan 13, 2021 at 3:48 PM Abhishek Pandit-Subedi
> > <abhishekpandit@chromium.org> wrote:
> > >
> > > + Archie, Alain
> > >
> > > It looks like we already landed this change into Chrome a while ago
> > > and we haven't had any issues.
> > > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2312116
> >
> > Good to know, but can you also confirm it is running with
> > CONFIG_BT_HCIBTUSB_INTERVAL=y?
>
> Correction, CONFIG_BT_HCIBTUSB_INTERVAL=y or passing enable_interval to btusb.
>
> > > Archie, can you confirm that this is the case?
> > >
> > > Thanks
> > > Abhishek
> > >
> > > On Wed, Jan 13, 2021 at 3:33 PM Luiz Augusto von Dentz
> > > <luiz.dentz@gmail.com> wrote:
> > > >
> > > > Hi Abhishek,
> > > >
> > > > On Wed, Jan 13, 2021 at 3:28 PM Luiz Augusto von Dentz
> > > > <luiz.dentz@gmail.com> wrote:
> > > > >
> > > > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > > >
> > > > > This makes btusb to queue ACL and events during a polling interval
> > > > > by using of a delayed work, with the interval working as a time window
> > > > > where frames received from different endpoints are considered to be
> > > > > arrived at same time and then attempt to resolve potential conflics by
> > > > > processing the events ahead of ACL packets.
> > > > >
> > > > > It worth noting though that priorizing events over ACL data may result
> > > > > in inverting the order compared to how they appeared over the air, for
> > > > > instance there may be packets received before a disconnect event that
> > > > > will be discarded and unencrypted packets received before encryption
> > > > > change which would considered encrypted, because of these potential
> > > > > changes on the order the support for queuing during the polling
> > > > > interval is not enabled by default so platforms have the following
> > > > > means to enable it:
> > > > >
> > > > > At build-time:
> > > > >
> > > > >     CONFIG_BT_HCIBTUSB_INTERVAL=y
> > > > >
> > > > > At runtime with use of module option:
> > > > >
> > > > >     enable_interval
> > > > >
> > > > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > > > ---
> > > > >  drivers/bluetooth/Kconfig |  7 ++++
> > > > >  drivers/bluetooth/btusb.c | 88 ++++++++++++++++++++++++++++++++++-----
> > > > >  2 files changed, 84 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > > > > index 4e73a531b377..2f20a853d946 100644
> > > > > --- a/drivers/bluetooth/Kconfig
> > > > > +++ b/drivers/bluetooth/Kconfig
> > > > > @@ -41,6 +41,13 @@ config BT_HCIBTUSB_AUTOSUSPEND
> > > > >           This can be overridden by passing btusb.enable_autosuspend=[y|n]
> > > > >           on the kernel commandline.
> > > > >
> > > > > +config BT_HCIBTUSB_INTERVAL
> > > > > +       bool "Enable notification of USB polling interval"
> > > > > +       depends on BT_HCIBTUSB
> > > > > +       help
> > > > > +         Say Y here to enable notification of USB polling interval for
> > > > > +         Bluetooth USB devices by default.
> > > > > +
> > > > >  config BT_HCIBTUSB_BCM
> > > > >         bool "Broadcom protocol support"
> > > > >         depends on BT_HCIBTUSB
> > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > > > index b14102fba601..38cb5448fc69 100644
> > > > > --- a/drivers/bluetooth/btusb.c
> > > > > +++ b/drivers/bluetooth/btusb.c
> > > > > @@ -30,7 +30,7 @@
> > > > >  static bool disable_scofix;
> > > > >  static bool force_scofix;
> > > > >  static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> > > > > -
> > > > > +static bool enable_interval = IS_ENABLED(CONFIG_BT_HCIBTUSB_INTERVAL);
> > > > >  static bool reset = true;
> > > > >
> > > > >  static struct usb_driver btusb_driver;
> > > > > @@ -519,8 +519,12 @@ struct btusb_data {
> > > > >
> > > > >         unsigned long flags;
> > > > >
> > > > > -       struct work_struct work;
> > > > > -       struct work_struct waker;
> > > > > +       struct work_struct  work;
> > > > > +       struct work_struct  waker;
> > > > > +       struct delayed_work rx_work;
> > > > > +
> > > > > +       struct sk_buff_head acl_q;
> > > > > +       struct sk_buff_head evt_q;
> > > > >
> > > > >         struct usb_anchor deferred;
> > > > >         struct usb_anchor tx_anchor;
> > > > > @@ -557,7 +561,7 @@ struct btusb_data {
> > > > >         int isoc_altsetting;
> > > > >         int suspend_count;
> > > > >
> > > > > -       int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> > > > > +       int (*recv_event)(struct btusb_data *data, struct sk_buff *skb);
> > > > >         int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
> > > > >
> > > > >         int (*setup_on_usb)(struct hci_dev *hdev);
> > > > > @@ -707,7 +711,7 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> > > > >
> > > > >                 if (!hci_skb_expect(skb)) {
> > > > >                         /* Complete frame */
> > > > > -                       data->recv_event(data->hdev, skb);
> > > > > +                       data->recv_event(data, skb);
> > > > >                         skb = NULL;
> > > > >                 }
> > > > >         }
> > > > > @@ -718,6 +722,25 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> > > > >         return err;
> > > > >  }
> > > > >
> > > > > +static int btusb_rx_queue(struct btusb_data *data, struct sk_buff *skb,
> > > > > +                         struct sk_buff_head *queue, unsigned int interval)
> > > > > +{
> > > > > +       skb_queue_tail(queue, skb);
> > > > > +
> > > > > +       schedule_delayed_work(&data->rx_work, interval);
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
> > > > > +{
> > > > > +       if (!enable_interval)
> > > > > +               return hci_recv_frame(data->hdev, skb);
> > > > > +
> > > > > +       /* TODO: Calculate polling interval based on endpoint bInterval? */
> > > > > +       return btusb_rx_queue(data, skb, &data->acl_q, msecs_to_jiffies(1));
> > > > > +}
> > > > > +
> > > > >  static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> > > > >  {
> > > > >         struct sk_buff *skb;
> > > > > @@ -765,7 +788,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> > > > >
> > > > >                 if (!hci_skb_expect(skb)) {
> > > > >                         /* Complete frame */
> > > > > -                       hci_recv_frame(data->hdev, skb);
> > > > > +                       btusb_recv_acl(data, skb);
> > > > >                         skb = NULL;
> > > > >                 }
> > > > >         }
> > > > > @@ -1383,9 +1406,13 @@ static int btusb_close(struct hci_dev *hdev)
> > > > >
> > > > >         BT_DBG("%s", hdev->name);
> > > > >
> > > > > +       cancel_delayed_work(&data->rx_work);
> > > > >         cancel_work_sync(&data->work);
> > > > >         cancel_work_sync(&data->waker);
> > > > >
> > > > > +       skb_queue_purge(&data->acl_q);
> > > > > +       skb_queue_purge(&data->evt_q);
> > > > > +
> > > > >         clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > > > >         clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> > > > >         clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> > > > > @@ -1417,6 +1444,11 @@ static int btusb_flush(struct hci_dev *hdev)
> > > > >
> > > > >         BT_DBG("%s", hdev->name);
> > > > >
> > > > > +       cancel_delayed_work(&data->rx_work);
> > > > > +
> > > > > +       skb_queue_purge(&data->acl_q);
> > > > > +       skb_queue_purge(&data->evt_q);
> > > > > +
> > > > >         usb_kill_anchored_urbs(&data->tx_anchor);
> > > > >         btusb_free_frags(data);
> > > > >
> > > > > @@ -1769,6 +1801,25 @@ static void btusb_waker(struct work_struct *work)
> > > > >         usb_autopm_put_interface(data->intf);
> > > > >  }
> > > > >
> > > > > +static void btusb_rx_dequeue(struct btusb_data *data,
> > > > > +                            struct sk_buff_head *queue)
> > > > > +{
> > > > > +       struct sk_buff *skb;
> > > > > +
> > > > > +       while ((skb = skb_dequeue(queue)))
> > > > > +               hci_recv_frame(data->hdev, skb);
> > > > > +}
> > > > > +
> > > > > +static void btusb_rx_work(struct work_struct *work)
> > > > > +{
> > > > > +       struct btusb_data *data = container_of(work, struct btusb_data,
> > > > > +                                              rx_work.work);
> > > > > +
> > > > > +       /* Process HCI event packets so states changes are synchronized first */
> > > > > +       btusb_rx_dequeue(data, &data->evt_q);
> > > > > +       btusb_rx_dequeue(data, &data->acl_q);
> > > > > +}
> > > > > +
> > > > >  static int btusb_setup_bcm92035(struct hci_dev *hdev)
> > > > >  {
> > > > >         struct sk_buff *skb;
> > > > > @@ -2304,10 +2355,8 @@ static void btusb_intel_secure_send_result(struct btusb_data *data,
> > > > >                 wake_up_bit(&data->flags, BTUSB_DOWNLOADING);
> > > > >  }
> > > > >
> > > > > -static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > > > +static int btusb_recv_event_intel(struct btusb_data *data, struct sk_buff *skb)
> > > > >  {
> > > > > -       struct btusb_data *data = hci_get_drvdata(hdev);
> > > > > -
> > > > >         if (test_bit(BTUSB_BOOTLOADER, &data->flags)) {
> > > > >                 struct hci_event_hdr *hdr = (void *)skb->data;
> > > > >
> > > > > @@ -2336,7 +2385,7 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > > >                 }
> > > > >         }
> > > > >
> > > > > -       return hci_recv_frame(hdev, skb);
> > > > > +       return hci_recv_frame(data->hdev, skb);
> > > > >  }
> > > > >
> > > > >  static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > > > @@ -4279,6 +4328,15 @@ static int btusb_shutdown_qca(struct hci_dev *hdev)
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +static int btusb_recv_evt(struct btusb_data *data, struct sk_buff *skb)
> > > > > +{
> > > > > +       if (!enable_interval)
> > > > > +               return hci_recv_frame(data->hdev, skb);
> > > > > +
> > > > > +       /* Don't delay event processing */
> > > > > +       return btusb_rx_queue(data, skb, &data->evt_q, 0);
> > > > > +}
> > > > > +
> > > > >  static int btusb_probe(struct usb_interface *intf,
> > > > >                        const struct usb_device_id *id)
> > > > >  {
> > > > > @@ -4362,6 +4420,11 @@ static int btusb_probe(struct usb_interface *intf,
> > > > >
> > > > >         INIT_WORK(&data->work, btusb_work);
> > > > >         INIT_WORK(&data->waker, btusb_waker);
> > > > > +       INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work);
> > > > > +
> > > > > +       skb_queue_head_init(&data->acl_q);
> > > > > +       skb_queue_head_init(&data->evt_q);
> > > > > +
> > > > >         init_usb_anchor(&data->deferred);
> > > > >         init_usb_anchor(&data->tx_anchor);
> > > > >         spin_lock_init(&data->txlock);
> > > > > @@ -4378,7 +4441,7 @@ static int btusb_probe(struct usb_interface *intf,
> > > > >                 data->recv_bulk = btusb_recv_bulk_intel;
> > > > >                 set_bit(BTUSB_BOOTLOADER, &data->flags);
> > > > >         } else {
> > > > > -               data->recv_event = hci_recv_frame;
> > > > > +               data->recv_event = btusb_recv_evt;
> > > > >                 data->recv_bulk = btusb_recv_bulk;
> > > > >         }
> > > > >
> > > > > @@ -4867,6 +4930,9 @@ MODULE_PARM_DESC(force_scofix, "Force fixup of wrong SCO buffers size");
> > > > >  module_param(enable_autosuspend, bool, 0644);
> > > > >  MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> > > > >
> > > > > +module_param(enable_interval, bool, 0644);
> > > > > +MODULE_PARM_DESC(enable_interval, "Enable USB polling interval by default");
> > > > > +
> > > > >  module_param(reset, bool, 0644);
> > > > >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> > > > >
> > > > > --
> > > > > 2.26.2
> > > >
> > > > Can you test these with the set of tests you guys have for Chrome OS?
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

RE: [RESEND,1/2] Bluetooth: btusb: Add support for queuing during polling interval

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2247 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=414239

---Test result---

##############################
    Test: CheckPatch - PASS
    

    ##############################
    Test: CheckGitLint - FAIL
    workflow: Add workflow files for ci
1: T1 Title exceeds max length (92>72): "Merge 87921db36c96bab632616d50037372f429e1f0c5 into 7bd37bbd62975c101d62dfe872ff507dfc34f269"
3: B6 Body message is missing

Bluetooth: btusb: Add support for queuing during polling interval
1: T1 Title exceeds max length (92>72): "Merge 87921db36c96bab632616d50037372f429e1f0c5 into 7bd37bbd62975c101d62dfe872ff507dfc34f269"
3: B6 Body message is missing

Bluetooth: L2CAP: Fix handling fragmented length
1: T1 Title exceeds max length (92>72): "Merge 87921db36c96bab632616d50037372f429e1f0c5 into 7bd37bbd62975c101d62dfe872ff507dfc34f269"
3: B6 Body message is missing


    ##############################
    Test: CheckBuildK - PASS
    

    ##############################
    Test: CheckTestRunner: Setup - PASS
    

    ##############################
    Test: CheckTestRunner: l2cap-tester - PASS
    Total: 40, Passed: 34 (85.0%), Failed: 0, Not Run: 6

    ##############################
    Test: CheckTestRunner: bnep-tester - PASS
    Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: mgmt-tester - PASS
    Total: 416, Passed: 402 (96.6%), Failed: 0, Not Run: 14

    ##############################
    Test: CheckTestRunner: rfcomm-tester - PASS
    Total: 9, Passed: 9 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: sco-tester - PASS
    Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: smp-tester - PASS
    Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0

    ##############################
    Test: CheckTestRunner: userchan-tester - PASS
    Total: 3, Passed: 3 (100.0%), Failed: 0, Not Run: 0

    

---
Regards,
Linux Bluetooth


[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 43340 bytes --]

[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3531 bytes --]

[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 546678 bytes --]

[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 11651 bytes --]

[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 9886 bytes --]

[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11797 bytes --]

[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 5428 bytes --]

Re: [RESEND 1/2] Bluetooth: btusb: Add support for queuing during polling interval

[Archie Pusaka]

Hi Abhishek and Luiz,

After the patch is applied, the number of endpoint races we have is
dramatically reduced from tens in a single day to just one or two in
the past six months. I think that counts as virtually no issues and I
strongly support this patch to be merged.

Thanks,
Archie

On Thu, 14 Jan 2021 at 08:27, Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> Hi Luiz,
>
> Yes, we have that enabled:
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2312117/3
>
> Abhishek
>
> On Wed, Jan 13, 2021 at 3:53 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi,
> >
> > On Wed, Jan 13, 2021 at 3:51 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi Abhishek,
> > >
> > > On Wed, Jan 13, 2021 at 3:48 PM Abhishek Pandit-Subedi
> > > <abhishekpandit@chromium.org> wrote:
> > > >
> > > > + Archie, Alain
> > > >
> > > > It looks like we already landed this change into Chrome a while ago
> > > > and we haven't had any issues.
> > > > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2312116
> > >
> > > Good to know, but can you also confirm it is running with
> > > CONFIG_BT_HCIBTUSB_INTERVAL=y?
> >
> > Correction, CONFIG_BT_HCIBTUSB_INTERVAL=y or passing enable_interval to btusb.
> >
> > > > Archie, can you confirm that this is the case?
> > > >
> > > > Thanks
> > > > Abhishek
> > > >
> > > > On Wed, Jan 13, 2021 at 3:33 PM Luiz Augusto von Dentz
> > > > <luiz.dentz@gmail.com> wrote:
> > > > >
> > > > > Hi Abhishek,
> > > > >
> > > > > On Wed, Jan 13, 2021 at 3:28 PM Luiz Augusto von Dentz
> > > > > <luiz.dentz@gmail.com> wrote:
> > > > > >
> > > > > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > > > >
> > > > > > This makes btusb to queue ACL and events during a polling interval
> > > > > > by using of a delayed work, with the interval working as a time window
> > > > > > where frames received from different endpoints are considered to be
> > > > > > arrived at same time and then attempt to resolve potential conflics by
> > > > > > processing the events ahead of ACL packets.
> > > > > >
> > > > > > It worth noting though that priorizing events over ACL data may result
> > > > > > in inverting the order compared to how they appeared over the air, for
> > > > > > instance there may be packets received before a disconnect event that
> > > > > > will be discarded and unencrypted packets received before encryption
> > > > > > change which would considered encrypted, because of these potential
> > > > > > changes on the order the support for queuing during the polling
> > > > > > interval is not enabled by default so platforms have the following
> > > > > > means to enable it:
> > > > > >
> > > > > > At build-time:
> > > > > >
> > > > > >     CONFIG_BT_HCIBTUSB_INTERVAL=y
> > > > > >
> > > > > > At runtime with use of module option:
> > > > > >
> > > > > >     enable_interval
> > > > > >
> > > > > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > > > > ---
> > > > > >  drivers/bluetooth/Kconfig |  7 ++++
> > > > > >  drivers/bluetooth/btusb.c | 88 ++++++++++++++++++++++++++++++++++-----
> > > > > >  2 files changed, 84 insertions(+), 11 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > > > > > index 4e73a531b377..2f20a853d946 100644
> > > > > > --- a/drivers/bluetooth/Kconfig
> > > > > > +++ b/drivers/bluetooth/Kconfig
> > > > > > @@ -41,6 +41,13 @@ config BT_HCIBTUSB_AUTOSUSPEND
> > > > > >           This can be overridden by passing btusb.enable_autosuspend=[y|n]
> > > > > >           on the kernel commandline.
> > > > > >
> > > > > > +config BT_HCIBTUSB_INTERVAL
> > > > > > +       bool "Enable notification of USB polling interval"
> > > > > > +       depends on BT_HCIBTUSB
> > > > > > +       help
> > > > > > +         Say Y here to enable notification of USB polling interval for
> > > > > > +         Bluetooth USB devices by default.
> > > > > > +
> > > > > >  config BT_HCIBTUSB_BCM
> > > > > >         bool "Broadcom protocol support"
> > > > > >         depends on BT_HCIBTUSB
> > > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > > > > index b14102fba601..38cb5448fc69 100644
> > > > > > --- a/drivers/bluetooth/btusb.c
> > > > > > +++ b/drivers/bluetooth/btusb.c
> > > > > > @@ -30,7 +30,7 @@
> > > > > >  static bool disable_scofix;
> > > > > >  static bool force_scofix;
> > > > > >  static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> > > > > > -
> > > > > > +static bool enable_interval = IS_ENABLED(CONFIG_BT_HCIBTUSB_INTERVAL);
> > > > > >  static bool reset = true;
> > > > > >
> > > > > >  static struct usb_driver btusb_driver;
> > > > > > @@ -519,8 +519,12 @@ struct btusb_data {
> > > > > >
> > > > > >         unsigned long flags;
> > > > > >
> > > > > > -       struct work_struct work;
> > > > > > -       struct work_struct waker;
> > > > > > +       struct work_struct  work;
> > > > > > +       struct work_struct  waker;
> > > > > > +       struct delayed_work rx_work;
> > > > > > +
> > > > > > +       struct sk_buff_head acl_q;
> > > > > > +       struct sk_buff_head evt_q;
> > > > > >
> > > > > >         struct usb_anchor deferred;
> > > > > >         struct usb_anchor tx_anchor;
> > > > > > @@ -557,7 +561,7 @@ struct btusb_data {
> > > > > >         int isoc_altsetting;
> > > > > >         int suspend_count;
> > > > > >
> > > > > > -       int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> > > > > > +       int (*recv_event)(struct btusb_data *data, struct sk_buff *skb);
> > > > > >         int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
> > > > > >
> > > > > >         int (*setup_on_usb)(struct hci_dev *hdev);
> > > > > > @@ -707,7 +711,7 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> > > > > >
> > > > > >                 if (!hci_skb_expect(skb)) {
> > > > > >                         /* Complete frame */
> > > > > > -                       data->recv_event(data->hdev, skb);
> > > > > > +                       data->recv_event(data, skb);
> > > > > >                         skb = NULL;
> > > > > >                 }
> > > > > >         }
> > > > > > @@ -718,6 +722,25 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> > > > > >         return err;
> > > > > >  }
> > > > > >
> > > > > > +static int btusb_rx_queue(struct btusb_data *data, struct sk_buff *skb,
> > > > > > +                         struct sk_buff_head *queue, unsigned int interval)
> > > > > > +{
> > > > > > +       skb_queue_tail(queue, skb);
> > > > > > +
> > > > > > +       schedule_delayed_work(&data->rx_work, interval);
> > > > > > +
> > > > > > +       return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
> > > > > > +{
> > > > > > +       if (!enable_interval)
> > > > > > +               return hci_recv_frame(data->hdev, skb);
> > > > > > +
> > > > > > +       /* TODO: Calculate polling interval based on endpoint bInterval? */
> > > > > > +       return btusb_rx_queue(data, skb, &data->acl_q, msecs_to_jiffies(1));
> > > > > > +}
> > > > > > +
> > > > > >  static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> > > > > >  {
> > > > > >         struct sk_buff *skb;
> > > > > > @@ -765,7 +788,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> > > > > >
> > > > > >                 if (!hci_skb_expect(skb)) {
> > > > > >                         /* Complete frame */
> > > > > > -                       hci_recv_frame(data->hdev, skb);
> > > > > > +                       btusb_recv_acl(data, skb);
> > > > > >                         skb = NULL;
> > > > > >                 }
> > > > > >         }
> > > > > > @@ -1383,9 +1406,13 @@ static int btusb_close(struct hci_dev *hdev)
> > > > > >
> > > > > >         BT_DBG("%s", hdev->name);
> > > > > >
> > > > > > +       cancel_delayed_work(&data->rx_work);
> > > > > >         cancel_work_sync(&data->work);
> > > > > >         cancel_work_sync(&data->waker);
> > > > > >
> > > > > > +       skb_queue_purge(&data->acl_q);
> > > > > > +       skb_queue_purge(&data->evt_q);
> > > > > > +
> > > > > >         clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > > > > >         clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> > > > > >         clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> > > > > > @@ -1417,6 +1444,11 @@ static int btusb_flush(struct hci_dev *hdev)
> > > > > >
> > > > > >         BT_DBG("%s", hdev->name);
> > > > > >
> > > > > > +       cancel_delayed_work(&data->rx_work);
> > > > > > +
> > > > > > +       skb_queue_purge(&data->acl_q);
> > > > > > +       skb_queue_purge(&data->evt_q);
> > > > > > +
> > > > > >         usb_kill_anchored_urbs(&data->tx_anchor);
> > > > > >         btusb_free_frags(data);
> > > > > >
> > > > > > @@ -1769,6 +1801,25 @@ static void btusb_waker(struct work_struct *work)
> > > > > >         usb_autopm_put_interface(data->intf);
> > > > > >  }
> > > > > >
> > > > > > +static void btusb_rx_dequeue(struct btusb_data *data,
> > > > > > +                            struct sk_buff_head *queue)
> > > > > > +{
> > > > > > +       struct sk_buff *skb;
> > > > > > +
> > > > > > +       while ((skb = skb_dequeue(queue)))
> > > > > > +               hci_recv_frame(data->hdev, skb);
> > > > > > +}
> > > > > > +
> > > > > > +static void btusb_rx_work(struct work_struct *work)
> > > > > > +{
> > > > > > +       struct btusb_data *data = container_of(work, struct btusb_data,
> > > > > > +                                              rx_work.work);
> > > > > > +
> > > > > > +       /* Process HCI event packets so states changes are synchronized first */
> > > > > > +       btusb_rx_dequeue(data, &data->evt_q);
> > > > > > +       btusb_rx_dequeue(data, &data->acl_q);
> > > > > > +}
> > > > > > +
> > > > > >  static int btusb_setup_bcm92035(struct hci_dev *hdev)
> > > > > >  {
> > > > > >         struct sk_buff *skb;
> > > > > > @@ -2304,10 +2355,8 @@ static void btusb_intel_secure_send_result(struct btusb_data *data,
> > > > > >                 wake_up_bit(&data->flags, BTUSB_DOWNLOADING);
> > > > > >  }
> > > > > >
> > > > > > -static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > > > > +static int btusb_recv_event_intel(struct btusb_data *data, struct sk_buff *skb)
> > > > > >  {
> > > > > > -       struct btusb_data *data = hci_get_drvdata(hdev);
> > > > > > -
> > > > > >         if (test_bit(BTUSB_BOOTLOADER, &data->flags)) {
> > > > > >                 struct hci_event_hdr *hdr = (void *)skb->data;
> > > > > >
> > > > > > @@ -2336,7 +2385,7 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > > > >                 }
> > > > > >         }
> > > > > >
> > > > > > -       return hci_recv_frame(hdev, skb);
> > > > > > +       return hci_recv_frame(data->hdev, skb);
> > > > > >  }
> > > > > >
> > > > > >  static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > > > > @@ -4279,6 +4328,15 @@ static int btusb_shutdown_qca(struct hci_dev *hdev)
> > > > > >         return 0;
> > > > > >  }
> > > > > >
> > > > > > +static int btusb_recv_evt(struct btusb_data *data, struct sk_buff *skb)
> > > > > > +{
> > > > > > +       if (!enable_interval)
> > > > > > +               return hci_recv_frame(data->hdev, skb);
> > > > > > +
> > > > > > +       /* Don't delay event processing */
> > > > > > +       return btusb_rx_queue(data, skb, &data->evt_q, 0);
> > > > > > +}
> > > > > > +
> > > > > >  static int btusb_probe(struct usb_interface *intf,
> > > > > >                        const struct usb_device_id *id)
> > > > > >  {
> > > > > > @@ -4362,6 +4420,11 @@ static int btusb_probe(struct usb_interface *intf,
> > > > > >
> > > > > >         INIT_WORK(&data->work, btusb_work);
> > > > > >         INIT_WORK(&data->waker, btusb_waker);
> > > > > > +       INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work);
> > > > > > +
> > > > > > +       skb_queue_head_init(&data->acl_q);
> > > > > > +       skb_queue_head_init(&data->evt_q);
> > > > > > +
> > > > > >         init_usb_anchor(&data->deferred);
> > > > > >         init_usb_anchor(&data->tx_anchor);
> > > > > >         spin_lock_init(&data->txlock);
> > > > > > @@ -4378,7 +4441,7 @@ static int btusb_probe(struct usb_interface *intf,
> > > > > >                 data->recv_bulk = btusb_recv_bulk_intel;
> > > > > >                 set_bit(BTUSB_BOOTLOADER, &data->flags);
> > > > > >         } else {
> > > > > > -               data->recv_event = hci_recv_frame;
> > > > > > +               data->recv_event = btusb_recv_evt;
> > > > > >                 data->recv_bulk = btusb_recv_bulk;
> > > > > >         }
> > > > > >
> > > > > > @@ -4867,6 +4930,9 @@ MODULE_PARM_DESC(force_scofix, "Force fixup of wrong SCO buffers size");
> > > > > >  module_param(enable_autosuspend, bool, 0644);
> > > > > >  MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> > > > > >
> > > > > > +module_param(enable_interval, bool, 0644);
> > > > > > +MODULE_PARM_DESC(enable_interval, "Enable USB polling interval by default");
> > > > > > +
> > > > > >  module_param(reset, bool, 0644);
> > > > > >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> > > > > >
> > > > > > --
> > > > > > 2.26.2
> > > > >
> > > > > Can you test these with the set of tests you guys have for Chrome OS?
> > > > >
> > > > > --
> > > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz

Re: [RESEND 1/2] Bluetooth: btusb: Add support for queuing during polling interval

[Luiz Augusto von Dentz]

Hi Archie,

On Wed, Jan 13, 2021 at 6:50 PM Archie Pusaka <apusaka@google.com> wrote:
>
> Hi Abhishek and Luiz,
>
> After the patch is applied, the number of endpoint races we have is
> dramatically reduced from tens in a single day to just one or two in
> the past six months. I think that counts as virtually no issues and I
> strongly support this patch to be merged.

Great, so you probably want to have Tested-by added, how about the
second patch in this set, have that been tested by you guys as well?

> Thanks,
> Archie
>
> On Thu, 14 Jan 2021 at 08:27, Abhishek Pandit-Subedi
> <abhishekpandit@chromium.org> wrote:
> >
> > Hi Luiz,
> >
> > Yes, we have that enabled:
> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2312117/3
> >
> > Abhishek
> >
> > On Wed, Jan 13, 2021 at 3:53 PM Luiz Augusto von Dentz
> > <luiz.dentz@gmail.com> wrote:
> > >
> > > Hi,
> > >
> > > On Wed, Jan 13, 2021 at 3:51 PM Luiz Augusto von Dentz
> > > <luiz.dentz@gmail.com> wrote:
> > > >
> > > > Hi Abhishek,
> > > >
> > > > On Wed, Jan 13, 2021 at 3:48 PM Abhishek Pandit-Subedi
> > > > <abhishekpandit@chromium.org> wrote:
> > > > >
> > > > > + Archie, Alain
> > > > >
> > > > > It looks like we already landed this change into Chrome a while ago
> > > > > and we haven't had any issues.
> > > > > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2312116
> > > >
> > > > Good to know, but can you also confirm it is running with
> > > > CONFIG_BT_HCIBTUSB_INTERVAL=y?
> > >
> > > Correction, CONFIG_BT_HCIBTUSB_INTERVAL=y or passing enable_interval to btusb.
> > >
> > > > > Archie, can you confirm that this is the case?
> > > > >
> > > > > Thanks
> > > > > Abhishek
> > > > >
> > > > > On Wed, Jan 13, 2021 at 3:33 PM Luiz Augusto von Dentz
> > > > > <luiz.dentz@gmail.com> wrote:
> > > > > >
> > > > > > Hi Abhishek,
> > > > > >
> > > > > > On Wed, Jan 13, 2021 at 3:28 PM Luiz Augusto von Dentz
> > > > > > <luiz.dentz@gmail.com> wrote:
> > > > > > >
> > > > > > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > > > > >
> > > > > > > This makes btusb to queue ACL and events during a polling interval
> > > > > > > by using of a delayed work, with the interval working as a time window
> > > > > > > where frames received from different endpoints are considered to be
> > > > > > > arrived at same time and then attempt to resolve potential conflics by
> > > > > > > processing the events ahead of ACL packets.
> > > > > > >
> > > > > > > It worth noting though that priorizing events over ACL data may result
> > > > > > > in inverting the order compared to how they appeared over the air, for
> > > > > > > instance there may be packets received before a disconnect event that
> > > > > > > will be discarded and unencrypted packets received before encryption
> > > > > > > change which would considered encrypted, because of these potential
> > > > > > > changes on the order the support for queuing during the polling
> > > > > > > interval is not enabled by default so platforms have the following
> > > > > > > means to enable it:
> > > > > > >
> > > > > > > At build-time:
> > > > > > >
> > > > > > >     CONFIG_BT_HCIBTUSB_INTERVAL=y
> > > > > > >
> > > > > > > At runtime with use of module option:
> > > > > > >
> > > > > > >     enable_interval
> > > > > > >
> > > > > > > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > > > > > ---
> > > > > > >  drivers/bluetooth/Kconfig |  7 ++++
> > > > > > >  drivers/bluetooth/btusb.c | 88 ++++++++++++++++++++++++++++++++++-----
> > > > > > >  2 files changed, 84 insertions(+), 11 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > > > > > > index 4e73a531b377..2f20a853d946 100644
> > > > > > > --- a/drivers/bluetooth/Kconfig
> > > > > > > +++ b/drivers/bluetooth/Kconfig
> > > > > > > @@ -41,6 +41,13 @@ config BT_HCIBTUSB_AUTOSUSPEND
> > > > > > >           This can be overridden by passing btusb.enable_autosuspend=[y|n]
> > > > > > >           on the kernel commandline.
> > > > > > >
> > > > > > > +config BT_HCIBTUSB_INTERVAL
> > > > > > > +       bool "Enable notification of USB polling interval"
> > > > > > > +       depends on BT_HCIBTUSB
> > > > > > > +       help
> > > > > > > +         Say Y here to enable notification of USB polling interval for
> > > > > > > +         Bluetooth USB devices by default.
> > > > > > > +
> > > > > > >  config BT_HCIBTUSB_BCM
> > > > > > >         bool "Broadcom protocol support"
> > > > > > >         depends on BT_HCIBTUSB
> > > > > > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > > > > > > index b14102fba601..38cb5448fc69 100644
> > > > > > > --- a/drivers/bluetooth/btusb.c
> > > > > > > +++ b/drivers/bluetooth/btusb.c
> > > > > > > @@ -30,7 +30,7 @@
> > > > > > >  static bool disable_scofix;
> > > > > > >  static bool force_scofix;
> > > > > > >  static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> > > > > > > -
> > > > > > > +static bool enable_interval = IS_ENABLED(CONFIG_BT_HCIBTUSB_INTERVAL);
> > > > > > >  static bool reset = true;
> > > > > > >
> > > > > > >  static struct usb_driver btusb_driver;
> > > > > > > @@ -519,8 +519,12 @@ struct btusb_data {
> > > > > > >
> > > > > > >         unsigned long flags;
> > > > > > >
> > > > > > > -       struct work_struct work;
> > > > > > > -       struct work_struct waker;
> > > > > > > +       struct work_struct  work;
> > > > > > > +       struct work_struct  waker;
> > > > > > > +       struct delayed_work rx_work;
> > > > > > > +
> > > > > > > +       struct sk_buff_head acl_q;
> > > > > > > +       struct sk_buff_head evt_q;
> > > > > > >
> > > > > > >         struct usb_anchor deferred;
> > > > > > >         struct usb_anchor tx_anchor;
> > > > > > > @@ -557,7 +561,7 @@ struct btusb_data {
> > > > > > >         int isoc_altsetting;
> > > > > > >         int suspend_count;
> > > > > > >
> > > > > > > -       int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> > > > > > > +       int (*recv_event)(struct btusb_data *data, struct sk_buff *skb);
> > > > > > >         int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
> > > > > > >
> > > > > > >         int (*setup_on_usb)(struct hci_dev *hdev);
> > > > > > > @@ -707,7 +711,7 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> > > > > > >
> > > > > > >                 if (!hci_skb_expect(skb)) {
> > > > > > >                         /* Complete frame */
> > > > > > > -                       data->recv_event(data->hdev, skb);
> > > > > > > +                       data->recv_event(data, skb);
> > > > > > >                         skb = NULL;
> > > > > > >                 }
> > > > > > >         }
> > > > > > > @@ -718,6 +722,25 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> > > > > > >         return err;
> > > > > > >  }
> > > > > > >
> > > > > > > +static int btusb_rx_queue(struct btusb_data *data, struct sk_buff *skb,
> > > > > > > +                         struct sk_buff_head *queue, unsigned int interval)
> > > > > > > +{
> > > > > > > +       skb_queue_tail(queue, skb);
> > > > > > > +
> > > > > > > +       schedule_delayed_work(&data->rx_work, interval);
> > > > > > > +
> > > > > > > +       return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
> > > > > > > +{
> > > > > > > +       if (!enable_interval)
> > > > > > > +               return hci_recv_frame(data->hdev, skb);
> > > > > > > +
> > > > > > > +       /* TODO: Calculate polling interval based on endpoint bInterval? */
> > > > > > > +       return btusb_rx_queue(data, skb, &data->acl_q, msecs_to_jiffies(1));
> > > > > > > +}
> > > > > > > +
> > > > > > >  static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> > > > > > >  {
> > > > > > >         struct sk_buff *skb;
> > > > > > > @@ -765,7 +788,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> > > > > > >
> > > > > > >                 if (!hci_skb_expect(skb)) {
> > > > > > >                         /* Complete frame */
> > > > > > > -                       hci_recv_frame(data->hdev, skb);
> > > > > > > +                       btusb_recv_acl(data, skb);
> > > > > > >                         skb = NULL;
> > > > > > >                 }
> > > > > > >         }
> > > > > > > @@ -1383,9 +1406,13 @@ static int btusb_close(struct hci_dev *hdev)
> > > > > > >
> > > > > > >         BT_DBG("%s", hdev->name);
> > > > > > >
> > > > > > > +       cancel_delayed_work(&data->rx_work);
> > > > > > >         cancel_work_sync(&data->work);
> > > > > > >         cancel_work_sync(&data->waker);
> > > > > > >
> > > > > > > +       skb_queue_purge(&data->acl_q);
> > > > > > > +       skb_queue_purge(&data->evt_q);
> > > > > > > +
> > > > > > >         clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > > > > > >         clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> > > > > > >         clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> > > > > > > @@ -1417,6 +1444,11 @@ static int btusb_flush(struct hci_dev *hdev)
> > > > > > >
> > > > > > >         BT_DBG("%s", hdev->name);
> > > > > > >
> > > > > > > +       cancel_delayed_work(&data->rx_work);
> > > > > > > +
> > > > > > > +       skb_queue_purge(&data->acl_q);
> > > > > > > +       skb_queue_purge(&data->evt_q);
> > > > > > > +
> > > > > > >         usb_kill_anchored_urbs(&data->tx_anchor);
> > > > > > >         btusb_free_frags(data);
> > > > > > >
> > > > > > > @@ -1769,6 +1801,25 @@ static void btusb_waker(struct work_struct *work)
> > > > > > >         usb_autopm_put_interface(data->intf);
> > > > > > >  }
> > > > > > >
> > > > > > > +static void btusb_rx_dequeue(struct btusb_data *data,
> > > > > > > +                            struct sk_buff_head *queue)
> > > > > > > +{
> > > > > > > +       struct sk_buff *skb;
> > > > > > > +
> > > > > > > +       while ((skb = skb_dequeue(queue)))
> > > > > > > +               hci_recv_frame(data->hdev, skb);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void btusb_rx_work(struct work_struct *work)
> > > > > > > +{
> > > > > > > +       struct btusb_data *data = container_of(work, struct btusb_data,
> > > > > > > +                                              rx_work.work);
> > > > > > > +
> > > > > > > +       /* Process HCI event packets so states changes are synchronized first */
> > > > > > > +       btusb_rx_dequeue(data, &data->evt_q);
> > > > > > > +       btusb_rx_dequeue(data, &data->acl_q);
> > > > > > > +}
> > > > > > > +
> > > > > > >  static int btusb_setup_bcm92035(struct hci_dev *hdev)
> > > > > > >  {
> > > > > > >         struct sk_buff *skb;
> > > > > > > @@ -2304,10 +2355,8 @@ static void btusb_intel_secure_send_result(struct btusb_data *data,
> > > > > > >                 wake_up_bit(&data->flags, BTUSB_DOWNLOADING);
> > > > > > >  }
> > > > > > >
> > > > > > > -static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > > > > > +static int btusb_recv_event_intel(struct btusb_data *data, struct sk_buff *skb)
> > > > > > >  {
> > > > > > > -       struct btusb_data *data = hci_get_drvdata(hdev);
> > > > > > > -
> > > > > > >         if (test_bit(BTUSB_BOOTLOADER, &data->flags)) {
> > > > > > >                 struct hci_event_hdr *hdr = (void *)skb->data;
> > > > > > >
> > > > > > > @@ -2336,7 +2385,7 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > > > > >                 }
> > > > > > >         }
> > > > > > >
> > > > > > > -       return hci_recv_frame(hdev, skb);
> > > > > > > +       return hci_recv_frame(data->hdev, skb);
> > > > > > >  }
> > > > > > >
> > > > > > >  static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > > > > > > @@ -4279,6 +4328,15 @@ static int btusb_shutdown_qca(struct hci_dev *hdev)
> > > > > > >         return 0;
> > > > > > >  }
> > > > > > >
> > > > > > > +static int btusb_recv_evt(struct btusb_data *data, struct sk_buff *skb)
> > > > > > > +{
> > > > > > > +       if (!enable_interval)
> > > > > > > +               return hci_recv_frame(data->hdev, skb);
> > > > > > > +
> > > > > > > +       /* Don't delay event processing */
> > > > > > > +       return btusb_rx_queue(data, skb, &data->evt_q, 0);
> > > > > > > +}
> > > > > > > +
> > > > > > >  static int btusb_probe(struct usb_interface *intf,
> > > > > > >                        const struct usb_device_id *id)
> > > > > > >  {
> > > > > > > @@ -4362,6 +4420,11 @@ static int btusb_probe(struct usb_interface *intf,
> > > > > > >
> > > > > > >         INIT_WORK(&data->work, btusb_work);
> > > > > > >         INIT_WORK(&data->waker, btusb_waker);
> > > > > > > +       INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work);
> > > > > > > +
> > > > > > > +       skb_queue_head_init(&data->acl_q);
> > > > > > > +       skb_queue_head_init(&data->evt_q);
> > > > > > > +
> > > > > > >         init_usb_anchor(&data->deferred);
> > > > > > >         init_usb_anchor(&data->tx_anchor);
> > > > > > >         spin_lock_init(&data->txlock);
> > > > > > > @@ -4378,7 +4441,7 @@ static int btusb_probe(struct usb_interface *intf,
> > > > > > >                 data->recv_bulk = btusb_recv_bulk_intel;
> > > > > > >                 set_bit(BTUSB_BOOTLOADER, &data->flags);
> > > > > > >         } else {
> > > > > > > -               data->recv_event = hci_recv_frame;
> > > > > > > +               data->recv_event = btusb_recv_evt;
> > > > > > >                 data->recv_bulk = btusb_recv_bulk;
> > > > > > >         }
> > > > > > >
> > > > > > > @@ -4867,6 +4930,9 @@ MODULE_PARM_DESC(force_scofix, "Force fixup of wrong SCO buffers size");
> > > > > > >  module_param(enable_autosuspend, bool, 0644);
> > > > > > >  MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> > > > > > >
> > > > > > > +module_param(enable_interval, bool, 0644);
> > > > > > > +MODULE_PARM_DESC(enable_interval, "Enable USB polling interval by default");
> > > > > > > +
> > > > > > >  module_param(reset, bool, 0644);
> > > > > > >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> > > > > > >
> > > > > > > --
> > > > > > > 2.26.2
> > > > > >
> > > > > > Can you test these with the set of tests you guys have for Chrome OS?
> > > > > >
> > > > > > --
> > > > > > Luiz Augusto von Dentz
> > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz



-- 
Luiz Augusto von Dentz

Re: [RESEND 1/2] Bluetooth: btusb: Add support for queuing during polling interval

[Archie Pusaka]

Hi Luiz,
I don't think we have tested the second patch yet.

Hi Abhishek,
Do you have context on this?

On Thu, 14 Jan 2021 at 11:57, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> This makes btusb to queue ACL and events during a polling interval
> by using of a delayed work, with the interval working as a time window
> where frames received from different endpoints are considered to be
> arrived at same time and then attempt to resolve potential conflics by
> processing the events ahead of ACL packets.
>
> It worth noting though that priorizing events over ACL data may result
> in inverting the order compared to how they appeared over the air, for
> instance there may be packets received before a disconnect event that
> will be discarded and unencrypted packets received before encryption
> change which would considered encrypted, because of these potential
> changes on the order the support for queuing during the polling
> interval is not enabled by default so platforms have the following
> means to enable it:
>
> At build-time:
>
>     CONFIG_BT_HCIBTUSB_INTERVAL=y
>
> At runtime with use of module option:
>
>     enable_interval
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

Tested-by: Archie Pusaka <apusaka@chromium.org>

> ---
>  drivers/bluetooth/Kconfig |  7 ++++
>  drivers/bluetooth/btusb.c | 88 ++++++++++++++++++++++++++++++++++-----
>  2 files changed, 84 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 4e73a531b377..2f20a853d946 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -41,6 +41,13 @@ config BT_HCIBTUSB_AUTOSUSPEND
>           This can be overridden by passing btusb.enable_autosuspend=[y|n]
>           on the kernel commandline.
>
> +config BT_HCIBTUSB_INTERVAL
> +       bool "Enable notification of USB polling interval"
> +       depends on BT_HCIBTUSB
> +       help
> +         Say Y here to enable notification of USB polling interval for
> +         Bluetooth USB devices by default.
> +
>  config BT_HCIBTUSB_BCM
>         bool "Broadcom protocol support"
>         depends on BT_HCIBTUSB
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index b14102fba601..38cb5448fc69 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -30,7 +30,7 @@
>  static bool disable_scofix;
>  static bool force_scofix;
>  static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> -
> +static bool enable_interval = IS_ENABLED(CONFIG_BT_HCIBTUSB_INTERVAL);
>  static bool reset = true;
>
>  static struct usb_driver btusb_driver;
> @@ -519,8 +519,12 @@ struct btusb_data {
>
>         unsigned long flags;
>
> -       struct work_struct work;
> -       struct work_struct waker;
> +       struct work_struct  work;
> +       struct work_struct  waker;
> +       struct delayed_work rx_work;
> +
> +       struct sk_buff_head acl_q;
> +       struct sk_buff_head evt_q;
>
>         struct usb_anchor deferred;
>         struct usb_anchor tx_anchor;
> @@ -557,7 +561,7 @@ struct btusb_data {
>         int isoc_altsetting;
>         int suspend_count;
>
> -       int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> +       int (*recv_event)(struct btusb_data *data, struct sk_buff *skb);
>         int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
>
>         int (*setup_on_usb)(struct hci_dev *hdev);
> @@ -707,7 +711,7 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
>
>                 if (!hci_skb_expect(skb)) {
>                         /* Complete frame */
> -                       data->recv_event(data->hdev, skb);
> +                       data->recv_event(data, skb);
>                         skb = NULL;
>                 }
>         }
> @@ -718,6 +722,25 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
>         return err;
>  }
>
> +static int btusb_rx_queue(struct btusb_data *data, struct sk_buff *skb,
> +                         struct sk_buff_head *queue, unsigned int interval)
> +{
> +       skb_queue_tail(queue, skb);
> +
> +       schedule_delayed_work(&data->rx_work, interval);
> +
> +       return 0;
> +}
> +
> +static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
> +{
> +       if (!enable_interval)
> +               return hci_recv_frame(data->hdev, skb);
> +
> +       /* TODO: Calculate polling interval based on endpoint bInterval? */
> +       return btusb_rx_queue(data, skb, &data->acl_q, msecs_to_jiffies(1));
> +}
> +
>  static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
>  {
>         struct sk_buff *skb;
> @@ -765,7 +788,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
>
>                 if (!hci_skb_expect(skb)) {
>                         /* Complete frame */
> -                       hci_recv_frame(data->hdev, skb);
> +                       btusb_recv_acl(data, skb);
>                         skb = NULL;
>                 }
>         }
> @@ -1383,9 +1406,13 @@ static int btusb_close(struct hci_dev *hdev)
>
>         BT_DBG("%s", hdev->name);
>
> +       cancel_delayed_work(&data->rx_work);
>         cancel_work_sync(&data->work);
>         cancel_work_sync(&data->waker);
>
> +       skb_queue_purge(&data->acl_q);
> +       skb_queue_purge(&data->evt_q);
> +
>         clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
>         clear_bit(BTUSB_BULK_RUNNING, &data->flags);
>         clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> @@ -1417,6 +1444,11 @@ static int btusb_flush(struct hci_dev *hdev)
>
>         BT_DBG("%s", hdev->name);
>
> +       cancel_delayed_work(&data->rx_work);
> +
> +       skb_queue_purge(&data->acl_q);
> +       skb_queue_purge(&data->evt_q);
> +
>         usb_kill_anchored_urbs(&data->tx_anchor);
>         btusb_free_frags(data);
>
> @@ -1769,6 +1801,25 @@ static void btusb_waker(struct work_struct *work)
>         usb_autopm_put_interface(data->intf);
>  }
>
> +static void btusb_rx_dequeue(struct btusb_data *data,
> +                            struct sk_buff_head *queue)
> +{
> +       struct sk_buff *skb;
> +
> +       while ((skb = skb_dequeue(queue)))
> +               hci_recv_frame(data->hdev, skb);
> +}
> +
> +static void btusb_rx_work(struct work_struct *work)
> +{
> +       struct btusb_data *data = container_of(work, struct btusb_data,
> +                                              rx_work.work);
> +
> +       /* Process HCI event packets so states changes are synchronized first */
> +       btusb_rx_dequeue(data, &data->evt_q);
> +       btusb_rx_dequeue(data, &data->acl_q);
> +}
> +
>  static int btusb_setup_bcm92035(struct hci_dev *hdev)
>  {
>         struct sk_buff *skb;
> @@ -2304,10 +2355,8 @@ static void btusb_intel_secure_send_result(struct btusb_data *data,
>                 wake_up_bit(&data->flags, BTUSB_DOWNLOADING);
>  }
>
> -static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> +static int btusb_recv_event_intel(struct btusb_data *data, struct sk_buff *skb)
>  {
> -       struct btusb_data *data = hci_get_drvdata(hdev);
> -
>         if (test_bit(BTUSB_BOOTLOADER, &data->flags)) {
>                 struct hci_event_hdr *hdr = (void *)skb->data;
>
> @@ -2336,7 +2385,7 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
>                 }
>         }
>
> -       return hci_recv_frame(hdev, skb);
> +       return hci_recv_frame(data->hdev, skb);
>  }
>
>  static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
> @@ -4279,6 +4328,15 @@ static int btusb_shutdown_qca(struct hci_dev *hdev)
>         return 0;
>  }
>
> +static int btusb_recv_evt(struct btusb_data *data, struct sk_buff *skb)
> +{
> +       if (!enable_interval)
> +               return hci_recv_frame(data->hdev, skb);
> +
> +       /* Don't delay event processing */
> +       return btusb_rx_queue(data, skb, &data->evt_q, 0);
> +}
> +
>  static int btusb_probe(struct usb_interface *intf,
>                        const struct usb_device_id *id)
>  {
> @@ -4362,6 +4420,11 @@ static int btusb_probe(struct usb_interface *intf,
>
>         INIT_WORK(&data->work, btusb_work);
>         INIT_WORK(&data->waker, btusb_waker);
> +       INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work);
> +
> +       skb_queue_head_init(&data->acl_q);
> +       skb_queue_head_init(&data->evt_q);
> +
>         init_usb_anchor(&data->deferred);
>         init_usb_anchor(&data->tx_anchor);
>         spin_lock_init(&data->txlock);
> @@ -4378,7 +4441,7 @@ static int btusb_probe(struct usb_interface *intf,
>                 data->recv_bulk = btusb_recv_bulk_intel;
>                 set_bit(BTUSB_BOOTLOADER, &data->flags);
>         } else {
> -               data->recv_event = hci_recv_frame;
> +               data->recv_event = btusb_recv_evt;
>                 data->recv_bulk = btusb_recv_bulk;
>         }
>
> @@ -4867,6 +4930,9 @@ MODULE_PARM_DESC(force_scofix, "Force fixup of wrong SCO buffers size");
>  module_param(enable_autosuspend, bool, 0644);
>  MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
>
> +module_param(enable_interval, bool, 0644);
> +MODULE_PARM_DESC(enable_interval, "Enable USB polling interval by default");
> +
>  module_param(reset, bool, 0644);
>  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
>
> --
> 2.26.2
>

Thanks,
Archie

Re: [RESEND 1/2] Bluetooth: btusb: Add support for queuing during polling interval

[Abhishek Pandit-Subedi]

Hi Luiz,

I think only one patch was sent out last time. Looking back through
patchworks, I only see 1 email in that series:
https://patchwork.kernel.org/project/bluetooth/list/?series=&submitter=&state=&q=Bluetooth%3A+btusb%3A+Add+support+for+queuing+during+polling+interval&archive=both&delegate=

Abhishek

On Thu, Jan 14, 2021 at 11:05 PM Archie Pusaka <apusaka@google.com> wrote:
>
> Hi Luiz,
> I don't think we have tested the second patch yet.
>
> Hi Abhishek,
> Do you have context on this?
>
> On Thu, 14 Jan 2021 at 11:57, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
> > This makes btusb to queue ACL and events during a polling interval
> > by using of a delayed work, with the interval working as a time window
> > where frames received from different endpoints are considered to be
> > arrived at same time and then attempt to resolve potential conflics by
> > processing the events ahead of ACL packets.
> >
> > It worth noting though that priorizing events over ACL data may result
> > in inverting the order compared to how they appeared over the air, for
> > instance there may be packets received before a disconnect event that
> > will be discarded and unencrypted packets received before encryption
> > change which would considered encrypted, because of these potential
> > changes on the order the support for queuing during the polling
> > interval is not enabled by default so platforms have the following
> > means to enable it:
> >
> > At build-time:
> >
> >     CONFIG_BT_HCIBTUSB_INTERVAL=y
> >
> > At runtime with use of module option:
> >
> >     enable_interval
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>
> Tested-by: Archie Pusaka <apusaka@chromium.org>
>
> > ---
> >  drivers/bluetooth/Kconfig |  7 ++++
> >  drivers/bluetooth/btusb.c | 88 ++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 84 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > index 4e73a531b377..2f20a853d946 100644
> > --- a/drivers/bluetooth/Kconfig
> > +++ b/drivers/bluetooth/Kconfig
> > @@ -41,6 +41,13 @@ config BT_HCIBTUSB_AUTOSUSPEND
> >           This can be overridden by passing btusb.enable_autosuspend=[y|n]
> >           on the kernel commandline.
> >
> > +config BT_HCIBTUSB_INTERVAL
> > +       bool "Enable notification of USB polling interval"
> > +       depends on BT_HCIBTUSB
> > +       help
> > +         Say Y here to enable notification of USB polling interval for
> > +         Bluetooth USB devices by default.
> > +
> >  config BT_HCIBTUSB_BCM
> >         bool "Broadcom protocol support"
> >         depends on BT_HCIBTUSB
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index b14102fba601..38cb5448fc69 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -30,7 +30,7 @@
> >  static bool disable_scofix;
> >  static bool force_scofix;
> >  static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> > -
> > +static bool enable_interval = IS_ENABLED(CONFIG_BT_HCIBTUSB_INTERVAL);
> >  static bool reset = true;
> >
> >  static struct usb_driver btusb_driver;
> > @@ -519,8 +519,12 @@ struct btusb_data {
> >
> >         unsigned long flags;
> >
> > -       struct work_struct work;
> > -       struct work_struct waker;
> > +       struct work_struct  work;
> > +       struct work_struct  waker;
> > +       struct delayed_work rx_work;
> > +
> > +       struct sk_buff_head acl_q;
> > +       struct sk_buff_head evt_q;
> >
> >         struct usb_anchor deferred;
> >         struct usb_anchor tx_anchor;
> > @@ -557,7 +561,7 @@ struct btusb_data {
> >         int isoc_altsetting;
> >         int suspend_count;
> >
> > -       int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> > +       int (*recv_event)(struct btusb_data *data, struct sk_buff *skb);
> >         int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
> >
> >         int (*setup_on_usb)(struct hci_dev *hdev);
> > @@ -707,7 +711,7 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> >
> >                 if (!hci_skb_expect(skb)) {
> >                         /* Complete frame */
> > -                       data->recv_event(data->hdev, skb);
> > +                       data->recv_event(data, skb);
> >                         skb = NULL;
> >                 }
> >         }
> > @@ -718,6 +722,25 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> >         return err;
> >  }
> >
> > +static int btusb_rx_queue(struct btusb_data *data, struct sk_buff *skb,
> > +                         struct sk_buff_head *queue, unsigned int interval)
> > +{
> > +       skb_queue_tail(queue, skb);
> > +
> > +       schedule_delayed_work(&data->rx_work, interval);
> > +
> > +       return 0;
> > +}
> > +
> > +static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
> > +{
> > +       if (!enable_interval)
> > +               return hci_recv_frame(data->hdev, skb);
> > +
> > +       /* TODO: Calculate polling interval based on endpoint bInterval? */
> > +       return btusb_rx_queue(data, skb, &data->acl_q, msecs_to_jiffies(1));
> > +}
> > +
> >  static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> >  {
> >         struct sk_buff *skb;
> > @@ -765,7 +788,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> >
> >                 if (!hci_skb_expect(skb)) {
> >                         /* Complete frame */
> > -                       hci_recv_frame(data->hdev, skb);
> > +                       btusb_recv_acl(data, skb);
> >                         skb = NULL;
> >                 }
> >         }
> > @@ -1383,9 +1406,13 @@ static int btusb_close(struct hci_dev *hdev)
> >
> >         BT_DBG("%s", hdev->name);
> >
> > +       cancel_delayed_work(&data->rx_work);
> >         cancel_work_sync(&data->work);
> >         cancel_work_sync(&data->waker);
> >
> > +       skb_queue_purge(&data->acl_q);
> > +       skb_queue_purge(&data->evt_q);
> > +
> >         clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> >         clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> >         clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> > @@ -1417,6 +1444,11 @@ static int btusb_flush(struct hci_dev *hdev)
> >
> >         BT_DBG("%s", hdev->name);
> >
> > +       cancel_delayed_work(&data->rx_work);
> > +
> > +       skb_queue_purge(&data->acl_q);
> > +       skb_queue_purge(&data->evt_q);
> > +
> >         usb_kill_anchored_urbs(&data->tx_anchor);
> >         btusb_free_frags(data);
> >
> > @@ -1769,6 +1801,25 @@ static void btusb_waker(struct work_struct *work)
> >         usb_autopm_put_interface(data->intf);
> >  }
> >
> > +static void btusb_rx_dequeue(struct btusb_data *data,
> > +                            struct sk_buff_head *queue)
> > +{
> > +       struct sk_buff *skb;
> > +
> > +       while ((skb = skb_dequeue(queue)))
> > +               hci_recv_frame(data->hdev, skb);
> > +}
> > +
> > +static void btusb_rx_work(struct work_struct *work)
> > +{
> > +       struct btusb_data *data = container_of(work, struct btusb_data,
> > +                                              rx_work.work);
> > +
> > +       /* Process HCI event packets so states changes are synchronized first */
> > +       btusb_rx_dequeue(data, &data->evt_q);
> > +       btusb_rx_dequeue(data, &data->acl_q);
> > +}
> > +
> >  static int btusb_setup_bcm92035(struct hci_dev *hdev)
> >  {
> >         struct sk_buff *skb;
> > @@ -2304,10 +2355,8 @@ static void btusb_intel_secure_send_result(struct btusb_data *data,
> >                 wake_up_bit(&data->flags, BTUSB_DOWNLOADING);
> >  }
> >
> > -static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > +static int btusb_recv_event_intel(struct btusb_data *data, struct sk_buff *skb)
> >  {
> > -       struct btusb_data *data = hci_get_drvdata(hdev);
> > -
> >         if (test_bit(BTUSB_BOOTLOADER, &data->flags)) {
> >                 struct hci_event_hdr *hdr = (void *)skb->data;
> >
> > @@ -2336,7 +2385,7 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> >                 }
> >         }
> >
> > -       return hci_recv_frame(hdev, skb);
> > +       return hci_recv_frame(data->hdev, skb);
> >  }
> >
> >  static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > @@ -4279,6 +4328,15 @@ static int btusb_shutdown_qca(struct hci_dev *hdev)
> >         return 0;
> >  }
> >
> > +static int btusb_recv_evt(struct btusb_data *data, struct sk_buff *skb)
> > +{
> > +       if (!enable_interval)
> > +               return hci_recv_frame(data->hdev, skb);
> > +
> > +       /* Don't delay event processing */
> > +       return btusb_rx_queue(data, skb, &data->evt_q, 0);
> > +}
> > +
> >  static int btusb_probe(struct usb_interface *intf,
> >                        const struct usb_device_id *id)
> >  {
> > @@ -4362,6 +4420,11 @@ static int btusb_probe(struct usb_interface *intf,
> >
> >         INIT_WORK(&data->work, btusb_work);
> >         INIT_WORK(&data->waker, btusb_waker);
> > +       INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work);
> > +
> > +       skb_queue_head_init(&data->acl_q);
> > +       skb_queue_head_init(&data->evt_q);
> > +
> >         init_usb_anchor(&data->deferred);
> >         init_usb_anchor(&data->tx_anchor);
> >         spin_lock_init(&data->txlock);
> > @@ -4378,7 +4441,7 @@ static int btusb_probe(struct usb_interface *intf,
> >                 data->recv_bulk = btusb_recv_bulk_intel;
> >                 set_bit(BTUSB_BOOTLOADER, &data->flags);
> >         } else {
> > -               data->recv_event = hci_recv_frame;
> > +               data->recv_event = btusb_recv_evt;
> >                 data->recv_bulk = btusb_recv_bulk;
> >         }
> >
> > @@ -4867,6 +4930,9 @@ MODULE_PARM_DESC(force_scofix, "Force fixup of wrong SCO buffers size");
> >  module_param(enable_autosuspend, bool, 0644);
> >  MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> >
> > +module_param(enable_interval, bool, 0644);
> > +MODULE_PARM_DESC(enable_interval, "Enable USB polling interval by default");
> > +
> >  module_param(reset, bool, 0644);
> >  MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
> >
> > --
> > 2.26.2
> >
>
> Thanks,
> Archie

Re: [RESEND 1/2] Bluetooth: btusb: Add support for queuing during polling interval

[Marcel Holtmann]

Hi Luiz,

> This makes btusb to queue ACL and events during a polling interval
> by using of a delayed work, with the interval working as a time window
> where frames received from different endpoints are considered to be
> arrived at same time and then attempt to resolve potential conflics by
> processing the events ahead of ACL packets.
> 
> It worth noting though that priorizing events over ACL data may result
> in inverting the order compared to how they appeared over the air, for
> instance there may be packets received before a disconnect event that
> will be discarded and unencrypted packets received before encryption
> change which would considered encrypted, because of these potential
> changes on the order the support for queuing during the polling
> interval is not enabled by default so platforms have the following
> means to enable it:
> 
> At build-time:
> 
>    CONFIG_BT_HCIBTUSB_INTERVAL=y
> 
> At runtime with use of module option:
> 
>    enable_interval
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> drivers/bluetooth/Kconfig |  7 ++++
> drivers/bluetooth/btusb.c | 88 ++++++++++++++++++++++++++++++++++-----
> 2 files changed, 84 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 4e73a531b377..2f20a853d946 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -41,6 +41,13 @@ config BT_HCIBTUSB_AUTOSUSPEND
> 	  This can be overridden by passing btusb.enable_autosuspend=[y|n]
> 	  on the kernel commandline.
> 
> +config BT_HCIBTUSB_INTERVAL
> +	bool "Enable notification of USB polling interval"
> +	depends on BT_HCIBTUSB
> +	help
> +	  Say Y here to enable notification of USB polling interval for
> +	  Bluetooth USB devices by default.
> +
> config BT_HCIBTUSB_BCM
> 	bool "Broadcom protocol support"
> 	depends on BT_HCIBTUSB
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index b14102fba601..38cb5448fc69 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -30,7 +30,7 @@
> static bool disable_scofix;
> static bool force_scofix;
> static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> -
> +static bool enable_interval = IS_ENABLED(CONFIG_BT_HCIBTUSB_INTERVAL);
> static bool reset = true;
> 
> static struct usb_driver btusb_driver;
> @@ -519,8 +519,12 @@ struct btusb_data {
> 
> 	unsigned long flags;
> 
> -	struct work_struct work;
> -	struct work_struct waker;
> +	struct work_struct  work;
> +	struct work_struct  waker;
> +	struct delayed_work rx_work;
> +
> +	struct sk_buff_head acl_q;
> +	struct sk_buff_head evt_q;

so does it make sense to keep them separate if we delay processing anyway.

> 
> 	struct usb_anchor deferred;
> 	struct usb_anchor tx_anchor;
> @@ -557,7 +561,7 @@ struct btusb_data {
> 	int isoc_altsetting;
> 	int suspend_count;
> 
> -	int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> +	int (*recv_event)(struct btusb_data *data, struct sk_buff *skb);
> 	int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
> 
> 	int (*setup_on_usb)(struct hci_dev *hdev);
> @@ -707,7 +711,7 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> 
> 		if (!hci_skb_expect(skb)) {
> 			/* Complete frame */
> -			data->recv_event(data->hdev, skb);
> +			data->recv_event(data, skb);
> 			skb = NULL;
> 		}
> 	}
> @@ -718,6 +722,25 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> 	return err;
> }
> 
> +static int btusb_rx_queue(struct btusb_data *data, struct sk_buff *skb,
> +			  struct sk_buff_head *queue, unsigned int interval)
> +{
> +	skb_queue_tail(queue, skb);
> +
> +	schedule_delayed_work(&data->rx_work, interval);
> +
> +	return 0;
> +}
> +
> +static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
> +{
> +	if (!enable_interval)
> +		return hci_recv_frame(data->hdev, skb);
> +
> +	/* TODO: Calculate polling interval based on endpoint bInterval? */
> +	return btusb_rx_queue(data, skb, &data->acl_q, msecs_to_jiffies(1));
> +}

Lets get this also fixed before we consider applying the patch.

> +
> static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> {
> 	struct sk_buff *skb;
> @@ -765,7 +788,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> 
> 		if (!hci_skb_expect(skb)) {
> 			/* Complete frame */
> -			hci_recv_frame(data->hdev, skb);
> +			btusb_recv_acl(data, skb);
> 			skb = NULL;
> 		}
> 	}
> @@ -1383,9 +1406,13 @@ static int btusb_close(struct hci_dev *hdev)
> 
> 	BT_DBG("%s", hdev->name);
> 
> +	cancel_delayed_work(&data->rx_work);
> 	cancel_work_sync(&data->work);
> 	cancel_work_sync(&data->waker);
> 
> +	skb_queue_purge(&data->acl_q);
> +	skb_queue_purge(&data->evt_q);
> +
> 	clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> 	clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> 	clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> @@ -1417,6 +1444,11 @@ static int btusb_flush(struct hci_dev *hdev)
> 
> 	BT_DBG("%s", hdev->name);
> 
> +	cancel_delayed_work(&data->rx_work);
> +
> +	skb_queue_purge(&data->acl_q);
> +	skb_queue_purge(&data->evt_q);
> +
> 	usb_kill_anchored_urbs(&data->tx_anchor);
> 	btusb_free_frags(data);
> 
> @@ -1769,6 +1801,25 @@ static void btusb_waker(struct work_struct *work)
> 	usb_autopm_put_interface(data->intf);
> }
> 
> +static void btusb_rx_dequeue(struct btusb_data *data,
> +			     struct sk_buff_head *queue)
> +{
> +	struct sk_buff *skb;
> +
> +	while ((skb = skb_dequeue(queue)))
> +		hci_recv_frame(data->hdev, skb);
> +}
> +
> +static void btusb_rx_work(struct work_struct *work)
> +{
> +	struct btusb_data *data = container_of(work, struct btusb_data,
> +					       rx_work.work);
> +
> +	/* Process HCI event packets so states changes are synchronized first */
> +	btusb_rx_dequeue(data, &data->evt_q);
> +	btusb_rx_dequeue(data, &data->acl_q);
> +}
> +
> static int btusb_setup_bcm92035(struct hci_dev *hdev)
> {
> 	struct sk_buff *skb;
> @@ -2304,10 +2355,8 @@ static void btusb_intel_secure_send_result(struct btusb_data *data,
> 		wake_up_bit(&data->flags, BTUSB_DOWNLOADING);
> }
> 
> -static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> +static int btusb_recv_event_intel(struct btusb_data *data, struct sk_buff *skb)
> {
> -	struct btusb_data *data = hci_get_drvdata(hdev);
> -
> 	if (test_bit(BTUSB_BOOTLOADER, &data->flags)) {
> 		struct hci_event_hdr *hdr = (void *)skb->data;
> 
> @@ -2336,7 +2385,7 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> 		}
> 	}
> 
> -	return hci_recv_frame(hdev, skb);
> +	return hci_recv_frame(data->hdev, skb);
> }
> 
> static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
> @@ -4279,6 +4328,15 @@ static int btusb_shutdown_qca(struct hci_dev *hdev)
> 	return 0;
> }
> 
> +static int btusb_recv_evt(struct btusb_data *data, struct sk_buff *skb)
> +{
> +	if (!enable_interval)
> +		return hci_recv_frame(data->hdev, skb);
> +
> +	/* Don't delay event processing */
> +	return btusb_rx_queue(data, skb, &data->evt_q, 0);
> +}
> +
> static int btusb_probe(struct usb_interface *intf,
> 		       const struct usb_device_id *id)
> {
> @@ -4362,6 +4420,11 @@ static int btusb_probe(struct usb_interface *intf,
> 
> 	INIT_WORK(&data->work, btusb_work);
> 	INIT_WORK(&data->waker, btusb_waker);
> +	INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work);
> +
> +	skb_queue_head_init(&data->acl_q);
> +	skb_queue_head_init(&data->evt_q);
> +
> 	init_usb_anchor(&data->deferred);
> 	init_usb_anchor(&data->tx_anchor);
> 	spin_lock_init(&data->txlock);
> @@ -4378,7 +4441,7 @@ static int btusb_probe(struct usb_interface *intf,
> 		data->recv_bulk = btusb_recv_bulk_intel;
> 		set_bit(BTUSB_BOOTLOADER, &data->flags);
> 	} else {
> -		data->recv_event = hci_recv_frame;
> +		data->recv_event = btusb_recv_evt;
> 		data->recv_bulk = btusb_recv_bulk;
> 	}
> 
> @@ -4867,6 +4930,9 @@ MODULE_PARM_DESC(force_scofix, "Force fixup of wrong SCO buffers size");
> module_param(enable_autosuspend, bool, 0644);
> MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> 
> +module_param(enable_interval, bool, 0644);
> +MODULE_PARM_DESC(enable_interval, "Enable USB polling interval by default");
> +
> module_param(reset, bool, 0644);
> MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");

Regards

Marcel

Re: [RESEND 2/2] Bluetooth: L2CAP: Fix handling fragmented length

[Marcel Holtmann]

Hi Luiz,

> Bluetooth Core Specification v5.2, Vol. 3, Part A, section 1.4, table
> 1.1:
> 
> 'Start Fragments always either begin with the first octet of the Basic
>  L2CAP header of a PDU or they have a length of zero (see [Vol 2] Part
>  B, Section 6.6.2).'
> 
> Apparently this was changed by the following errata:
> 
> https://www.bluetooth.org/tse/errata_view.cfm?errata_id=10216
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> include/net/bluetooth/l2cap.h |   1 +
> net/bluetooth/l2cap_core.c    | 118 +++++++++++++++++++++++++++-------
> 2 files changed, 94 insertions(+), 25 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

Re: [RESEND 1/2] Bluetooth: btusb: Add support for queuing during polling interval

[Luiz Augusto von Dentz]

Hi Marcel,

On Mon, Jan 25, 2021 at 10:20 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > This makes btusb to queue ACL and events during a polling interval
> > by using of a delayed work, with the interval working as a time window
> > where frames received from different endpoints are considered to be
> > arrived at same time and then attempt to resolve potential conflics by
> > processing the events ahead of ACL packets.
> >
> > It worth noting though that priorizing events over ACL data may result
> > in inverting the order compared to how they appeared over the air, for
> > instance there may be packets received before a disconnect event that
> > will be discarded and unencrypted packets received before encryption
> > change which would considered encrypted, because of these potential
> > changes on the order the support for queuing during the polling
> > interval is not enabled by default so platforms have the following
> > means to enable it:
> >
> > At build-time:
> >
> >    CONFIG_BT_HCIBTUSB_INTERVAL=y
> >
> > At runtime with use of module option:
> >
> >    enable_interval
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > drivers/bluetooth/Kconfig |  7 ++++
> > drivers/bluetooth/btusb.c | 88 ++++++++++++++++++++++++++++++++++-----
> > 2 files changed, 84 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > index 4e73a531b377..2f20a853d946 100644
> > --- a/drivers/bluetooth/Kconfig
> > +++ b/drivers/bluetooth/Kconfig
> > @@ -41,6 +41,13 @@ config BT_HCIBTUSB_AUTOSUSPEND
> >         This can be overridden by passing btusb.enable_autosuspend=[y|n]
> >         on the kernel commandline.
> >
> > +config BT_HCIBTUSB_INTERVAL
> > +     bool "Enable notification of USB polling interval"
> > +     depends on BT_HCIBTUSB
> > +     help
> > +       Say Y here to enable notification of USB polling interval for
> > +       Bluetooth USB devices by default.
> > +
> > config BT_HCIBTUSB_BCM
> >       bool "Broadcom protocol support"
> >       depends on BT_HCIBTUSB
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index b14102fba601..38cb5448fc69 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -30,7 +30,7 @@
> > static bool disable_scofix;
> > static bool force_scofix;
> > static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> > -
> > +static bool enable_interval = IS_ENABLED(CONFIG_BT_HCIBTUSB_INTERVAL);
> > static bool reset = true;
> >
> > static struct usb_driver btusb_driver;
> > @@ -519,8 +519,12 @@ struct btusb_data {
> >
> >       unsigned long flags;
> >
> > -     struct work_struct work;
> > -     struct work_struct waker;
> > +     struct work_struct  work;
> > +     struct work_struct  waker;
> > +     struct delayed_work rx_work;
> > +
> > +     struct sk_buff_head acl_q;
> > +     struct sk_buff_head evt_q;
>
> so does it make sense to keep them separate if we delay processing anyway.


We need them to reorder in case they are received at the same time
(within the same polling interval), we could reorder in place but then
will need to iterate into the queue to find where to insert events,
using 2 queues is a lot simpler to understand and probably perform
better.

>
> >
> >       struct usb_anchor deferred;
> >       struct usb_anchor tx_anchor;
> > @@ -557,7 +561,7 @@ struct btusb_data {
> >       int isoc_altsetting;
> >       int suspend_count;
> >
> > -     int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> > +     int (*recv_event)(struct btusb_data *data, struct sk_buff *skb);
> >       int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
> >
> >       int (*setup_on_usb)(struct hci_dev *hdev);
> > @@ -707,7 +711,7 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> >
> >               if (!hci_skb_expect(skb)) {
> >                       /* Complete frame */
> > -                     data->recv_event(data->hdev, skb);
> > +                     data->recv_event(data, skb);
> >                       skb = NULL;
> >               }
> >       }
> > @@ -718,6 +722,25 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> >       return err;
> > }
> >
> > +static int btusb_rx_queue(struct btusb_data *data, struct sk_buff *skb,
> > +                       struct sk_buff_head *queue, unsigned int interval)
> > +{
> > +     skb_queue_tail(queue, skb);
> > +
> > +     schedule_delayed_work(&data->rx_work, interval);
> > +
> > +     return 0;
> > +}
> > +
> > +static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
> > +{
> > +     if (!enable_interval)
> > +             return hci_recv_frame(data->hdev, skb);
> > +
> > +     /* TODO: Calculate polling interval based on endpoint bInterval? */
> > +     return btusb_rx_queue(data, skb, &data->acl_q, msecs_to_jiffies(1));
> > +}
>
> Lets get this also fixed before we consider applying the patch.

Will update the use of the bInterval then, it just needs to be
converted to msec though.

> > +
> > static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> > {
> >       struct sk_buff *skb;
> > @@ -765,7 +788,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> >
> >               if (!hci_skb_expect(skb)) {
> >                       /* Complete frame */
> > -                     hci_recv_frame(data->hdev, skb);
> > +                     btusb_recv_acl(data, skb);
> >                       skb = NULL;
> >               }
> >       }
> > @@ -1383,9 +1406,13 @@ static int btusb_close(struct hci_dev *hdev)
> >
> >       BT_DBG("%s", hdev->name);
> >
> > +     cancel_delayed_work(&data->rx_work);
> >       cancel_work_sync(&data->work);
> >       cancel_work_sync(&data->waker);
> >
> > +     skb_queue_purge(&data->acl_q);
> > +     skb_queue_purge(&data->evt_q);
> > +
> >       clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> >       clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> >       clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> > @@ -1417,6 +1444,11 @@ static int btusb_flush(struct hci_dev *hdev)
> >
> >       BT_DBG("%s", hdev->name);
> >
> > +     cancel_delayed_work(&data->rx_work);
> > +
> > +     skb_queue_purge(&data->acl_q);
> > +     skb_queue_purge(&data->evt_q);
> > +
> >       usb_kill_anchored_urbs(&data->tx_anchor);
> >       btusb_free_frags(data);
> >
> > @@ -1769,6 +1801,25 @@ static void btusb_waker(struct work_struct *work)
> >       usb_autopm_put_interface(data->intf);
> > }
> >
> > +static void btusb_rx_dequeue(struct btusb_data *data,
> > +                          struct sk_buff_head *queue)
> > +{
> > +     struct sk_buff *skb;
> > +
> > +     while ((skb = skb_dequeue(queue)))
> > +             hci_recv_frame(data->hdev, skb);
> > +}
> > +
> > +static void btusb_rx_work(struct work_struct *work)
> > +{
> > +     struct btusb_data *data = container_of(work, struct btusb_data,
> > +                                            rx_work.work);
> > +
> > +     /* Process HCI event packets so states changes are synchronized first */
> > +     btusb_rx_dequeue(data, &data->evt_q);
> > +     btusb_rx_dequeue(data, &data->acl_q);
> > +}
> > +
> > static int btusb_setup_bcm92035(struct hci_dev *hdev)
> > {
> >       struct sk_buff *skb;
> > @@ -2304,10 +2355,8 @@ static void btusb_intel_secure_send_result(struct btusb_data *data,
> >               wake_up_bit(&data->flags, BTUSB_DOWNLOADING);
> > }
> >
> > -static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > +static int btusb_recv_event_intel(struct btusb_data *data, struct sk_buff *skb)
> > {
> > -     struct btusb_data *data = hci_get_drvdata(hdev);
> > -
> >       if (test_bit(BTUSB_BOOTLOADER, &data->flags)) {
> >               struct hci_event_hdr *hdr = (void *)skb->data;
> >
> > @@ -2336,7 +2385,7 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> >               }
> >       }
> >
> > -     return hci_recv_frame(hdev, skb);
> > +     return hci_recv_frame(data->hdev, skb);
> > }
> >
> > static int btusb_send_frame_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > @@ -4279,6 +4328,15 @@ static int btusb_shutdown_qca(struct hci_dev *hdev)
> >       return 0;
> > }
> >
> > +static int btusb_recv_evt(struct btusb_data *data, struct sk_buff *skb)
> > +{
> > +     if (!enable_interval)
> > +             return hci_recv_frame(data->hdev, skb);
> > +
> > +     /* Don't delay event processing */
> > +     return btusb_rx_queue(data, skb, &data->evt_q, 0);
> > +}
> > +
> > static int btusb_probe(struct usb_interface *intf,
> >                      const struct usb_device_id *id)
> > {
> > @@ -4362,6 +4420,11 @@ static int btusb_probe(struct usb_interface *intf,
> >
> >       INIT_WORK(&data->work, btusb_work);
> >       INIT_WORK(&data->waker, btusb_waker);
> > +     INIT_DELAYED_WORK(&data->rx_work, btusb_rx_work);
> > +
> > +     skb_queue_head_init(&data->acl_q);
> > +     skb_queue_head_init(&data->evt_q);
> > +
> >       init_usb_anchor(&data->deferred);
> >       init_usb_anchor(&data->tx_anchor);
> >       spin_lock_init(&data->txlock);
> > @@ -4378,7 +4441,7 @@ static int btusb_probe(struct usb_interface *intf,
> >               data->recv_bulk = btusb_recv_bulk_intel;
> >               set_bit(BTUSB_BOOTLOADER, &data->flags);
> >       } else {
> > -             data->recv_event = hci_recv_frame;
> > +             data->recv_event = btusb_recv_evt;
> >               data->recv_bulk = btusb_recv_bulk;
> >       }
> >
> > @@ -4867,6 +4930,9 @@ MODULE_PARM_DESC(force_scofix, "Force fixup of wrong SCO buffers size");
> > module_param(enable_autosuspend, bool, 0644);
> > MODULE_PARM_DESC(enable_autosuspend, "Enable USB autosuspend by default");
> >
> > +module_param(enable_interval, bool, 0644);
> > +MODULE_PARM_DESC(enable_interval, "Enable USB polling interval by default");
> > +
> > module_param(reset, bool, 0644);
> > MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
>
> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz

Re: [RESEND 1/2] Bluetooth: btusb: Add support for queuing during polling interval

[Marcel Holtmann]

Hi Luiz,

>>> This makes btusb to queue ACL and events during a polling interval
>>> by using of a delayed work, with the interval working as a time window
>>> where frames received from different endpoints are considered to be
>>> arrived at same time and then attempt to resolve potential conflics by
>>> processing the events ahead of ACL packets.
>>> 
>>> It worth noting though that priorizing events over ACL data may result
>>> in inverting the order compared to how they appeared over the air, for
>>> instance there may be packets received before a disconnect event that
>>> will be discarded and unencrypted packets received before encryption
>>> change which would considered encrypted, because of these potential
>>> changes on the order the support for queuing during the polling
>>> interval is not enabled by default so platforms have the following
>>> means to enable it:
>>> 
>>> At build-time:
>>> 
>>>   CONFIG_BT_HCIBTUSB_INTERVAL=y
>>> 
>>> At runtime with use of module option:
>>> 
>>>   enable_interval
>>> 
>>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>> ---
>>> drivers/bluetooth/Kconfig |  7 ++++
>>> drivers/bluetooth/btusb.c | 88 ++++++++++++++++++++++++++++++++++-----
>>> 2 files changed, 84 insertions(+), 11 deletions(-)
>>> 
>>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
>>> index 4e73a531b377..2f20a853d946 100644
>>> --- a/drivers/bluetooth/Kconfig
>>> +++ b/drivers/bluetooth/Kconfig
>>> @@ -41,6 +41,13 @@ config BT_HCIBTUSB_AUTOSUSPEND
>>>        This can be overridden by passing btusb.enable_autosuspend=[y|n]
>>>        on the kernel commandline.
>>> 
>>> +config BT_HCIBTUSB_INTERVAL
>>> +     bool "Enable notification of USB polling interval"
>>> +     depends on BT_HCIBTUSB
>>> +     help
>>> +       Say Y here to enable notification of USB polling interval for
>>> +       Bluetooth USB devices by default.
>>> +
>>> config BT_HCIBTUSB_BCM
>>>      bool "Broadcom protocol support"
>>>      depends on BT_HCIBTUSB
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index b14102fba601..38cb5448fc69 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -30,7 +30,7 @@
>>> static bool disable_scofix;
>>> static bool force_scofix;
>>> static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
>>> -
>>> +static bool enable_interval = IS_ENABLED(CONFIG_BT_HCIBTUSB_INTERVAL);
>>> static bool reset = true;
>>> 
>>> static struct usb_driver btusb_driver;
>>> @@ -519,8 +519,12 @@ struct btusb_data {
>>> 
>>>      unsigned long flags;
>>> 
>>> -     struct work_struct work;
>>> -     struct work_struct waker;
>>> +     struct work_struct  work;
>>> +     struct work_struct  waker;
>>> +     struct delayed_work rx_work;
>>> +
>>> +     struct sk_buff_head acl_q;
>>> +     struct sk_buff_head evt_q;
>> 
>> so does it make sense to keep them separate if we delay processing anyway.
> 
> 
> We need them to reorder in case they are received at the same time
> (within the same polling interval), we could reorder in place but then
> will need to iterate into the queue to find where to insert events,
> using 2 queues is a lot simpler to understand and probably perform
> better.

do we actually have to queue the HCI event as well. Or just make sure that the bulk endpoints are processed at the same interval as the interrupt endpoint?

Regards

Marcel

Re: [RESEND 1/2] Bluetooth: btusb: Add support for queuing during polling interval

[Luiz Augusto von Dentz]

Hi Marcel,

On Tue, Jan 26, 2021 at 1:16 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> >>> This makes btusb to queue ACL and events during a polling interval
> >>> by using of a delayed work, with the interval working as a time window
> >>> where frames received from different endpoints are considered to be
> >>> arrived at same time and then attempt to resolve potential conflics by
> >>> processing the events ahead of ACL packets.
> >>>
> >>> It worth noting though that priorizing events over ACL data may result
> >>> in inverting the order compared to how they appeared over the air, for
> >>> instance there may be packets received before a disconnect event that
> >>> will be discarded and unencrypted packets received before encryption
> >>> change which would considered encrypted, because of these potential
> >>> changes on the order the support for queuing during the polling
> >>> interval is not enabled by default so platforms have the following
> >>> means to enable it:
> >>>
> >>> At build-time:
> >>>
> >>>   CONFIG_BT_HCIBTUSB_INTERVAL=y
> >>>
> >>> At runtime with use of module option:
> >>>
> >>>   enable_interval
> >>>
> >>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >>> ---
> >>> drivers/bluetooth/Kconfig |  7 ++++
> >>> drivers/bluetooth/btusb.c | 88 ++++++++++++++++++++++++++++++++++-----
> >>> 2 files changed, 84 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> >>> index 4e73a531b377..2f20a853d946 100644
> >>> --- a/drivers/bluetooth/Kconfig
> >>> +++ b/drivers/bluetooth/Kconfig
> >>> @@ -41,6 +41,13 @@ config BT_HCIBTUSB_AUTOSUSPEND
> >>>        This can be overridden by passing btusb.enable_autosuspend=[y|n]
> >>>        on the kernel commandline.
> >>>
> >>> +config BT_HCIBTUSB_INTERVAL
> >>> +     bool "Enable notification of USB polling interval"
> >>> +     depends on BT_HCIBTUSB
> >>> +     help
> >>> +       Say Y here to enable notification of USB polling interval for
> >>> +       Bluetooth USB devices by default.
> >>> +
> >>> config BT_HCIBTUSB_BCM
> >>>      bool "Broadcom protocol support"
> >>>      depends on BT_HCIBTUSB
> >>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> >>> index b14102fba601..38cb5448fc69 100644
> >>> --- a/drivers/bluetooth/btusb.c
> >>> +++ b/drivers/bluetooth/btusb.c
> >>> @@ -30,7 +30,7 @@
> >>> static bool disable_scofix;
> >>> static bool force_scofix;
> >>> static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> >>> -
> >>> +static bool enable_interval = IS_ENABLED(CONFIG_BT_HCIBTUSB_INTERVAL);
> >>> static bool reset = true;
> >>>
> >>> static struct usb_driver btusb_driver;
> >>> @@ -519,8 +519,12 @@ struct btusb_data {
> >>>
> >>>      unsigned long flags;
> >>>
> >>> -     struct work_struct work;
> >>> -     struct work_struct waker;
> >>> +     struct work_struct  work;
> >>> +     struct work_struct  waker;
> >>> +     struct delayed_work rx_work;
> >>> +
> >>> +     struct sk_buff_head acl_q;
> >>> +     struct sk_buff_head evt_q;
> >>
> >> so does it make sense to keep them separate if we delay processing anyway.
> >
> >
> > We need them to reorder in case they are received at the same time
> > (within the same polling interval), we could reorder in place but then
> > will need to iterate into the queue to find where to insert events,
> > using 2 queues is a lot simpler to understand and probably perform
> > better.
>
> do we actually have to queue the HCI event as well. Or just make sure that the bulk endpoints are processed at the same interval as the interrupt endpoint?

Perhaps you are suggesting to just maintain one queue for ACL data and
then dequeue it if an event is received?

> Regards
>
> Marcel
>


-- 
Luiz Augusto von Dentz