* [PATCH] Bluetooth: btusb: Add support for queuing during polling interval
@ 2021-11-08 23:02 Luiz Augusto von Dentz
2021-11-18 4:39 ` Marcel Holtmann
0 siblings, 1 reply; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2021-11-08 23:02 UTC (permalink / raw)
To: linux-bluetooth
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 runtime with use of module option:
enable_poll_sync
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
drivers/bluetooth/btusb.c | 89 +++++++++++++++++++++++++++++++++------
1 file changed, 77 insertions(+), 12 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 46d892bbde62..29aa0f346ace 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -31,7 +31,7 @@
static bool disable_scofix;
static bool force_scofix;
static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
-
+static bool enable_poll_sync;
static bool reset = true;
static struct usb_driver btusb_driver;
@@ -550,8 +550,12 @@ struct btusb_data {
unsigned long flags;
- struct work_struct work;
- struct work_struct waker;
+ int intr_interval;
+ struct work_struct work;
+ struct work_struct waker;
+ struct delayed_work rx_work;
+
+ struct sk_buff_head acl_q;
struct usb_anchor deferred;
struct usb_anchor tx_anchor;
@@ -588,8 +592,8 @@ struct btusb_data {
int isoc_altsetting;
int suspend_count;
- int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
int (*recv_acl)(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);
@@ -761,7 +765,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;
}
}
@@ -772,6 +776,18 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
return err;
}
+static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
+{
+ if (!enable_poll_sync)
+ return data->recv_acl(data->hdev, skb);
+
+ skb_queue_tail(&data->acl_q, skb);
+
+ schedule_delayed_work(&data->rx_work, data->intr_interval);
+
+ return 0;
+}
+
static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
{
struct sk_buff *skb;
@@ -819,7 +835,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
if (!hci_skb_expect(skb)) {
/* Complete frame */
- data->recv_acl(data->hdev, skb);
+ btusb_recv_acl(data, skb);
skb = NULL;
}
}
@@ -971,6 +987,19 @@ static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t mem_flags)
usb_unanchor_urb(urb);
}
+ /* The units are frames (milliseconds) for full and low speed devices,
+ * and microframes (1/8 millisecond) for highspeed and SuperSpeed
+ * devices.
+ */
+ switch (urb->dev->speed) {
+ case USB_SPEED_SUPER_PLUS:
+ case USB_SPEED_SUPER: /* units are 125us */
+ data->intr_interval = usecs_to_jiffies(urb->interval * 125);
+ break;
+ default:
+ data->intr_interval = msecs_to_jiffies(urb->interval);
+ }
+
usb_free_urb(urb);
return err;
@@ -1430,9 +1459,12 @@ 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);
+
clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
clear_bit(BTUSB_BULK_RUNNING, &data->flags);
clear_bit(BTUSB_INTR_RUNNING, &data->flags);
@@ -1464,6 +1496,10 @@ 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);
+
usb_kill_anchored_urbs(&data->tx_anchor);
btusb_free_frags(data);
@@ -1827,6 +1863,17 @@ static void btusb_waker(struct work_struct *work)
usb_autopm_put_interface(data->intf);
}
+static void btusb_rx_work(struct work_struct *work)
+{
+ struct btusb_data *data = container_of(work, struct btusb_data,
+ rx_work.work);
+ struct sk_buff *skb;
+
+ /* Dequeue ACL data received during the interval */
+ while ((skb = skb_dequeue(&data->acl_q)))
+ data->recv_acl(data->hdev, skb);
+}
+
static int btusb_setup_bcm92035(struct hci_dev *hdev)
{
struct sk_buff *skb;
@@ -2028,9 +2075,9 @@ static int btusb_recv_bulk_intel(struct btusb_data *data, void *buffer,
return btusb_recv_bulk(data, buffer, count);
}
-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)
{
- if (btintel_test_flag(hdev, INTEL_BOOTLOADER)) {
+ if (btintel_test_flag(data->hdev, INTEL_BOOTLOADER)) {
struct hci_event_hdr *hdr = (void *)skb->data;
if (skb->len > HCI_EVENT_HDR_SIZE && hdr->evt == 0xff &&
@@ -2044,7 +2091,7 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
* the device sends a vendor specific event
* indicating that the bootup completed.
*/
- btintel_bootup(hdev, ptr, len);
+ btintel_bootup(data->hdev, ptr, len);
break;
case 0x06:
/* When the firmware loading completes the
@@ -2052,13 +2099,14 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
* indicating the result of the firmware
* loading.
*/
- btintel_secure_send_result(hdev, ptr, len);
+ btintel_secure_send_result(data->hdev, ptr,
+ len);
break;
}
}
}
- 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)
@@ -3372,6 +3420,16 @@ 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_poll_sync) {
+ /* Trigger dequeue immediatelly if an event is received */
+ schedule_delayed_work(&data->rx_work, 0);
+ }
+
+ return hci_recv_frame(data->hdev, skb);
+}
+
static int btusb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
@@ -3455,6 +3513,10 @@ 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);
+
init_usb_anchor(&data->deferred);
init_usb_anchor(&data->tx_anchor);
spin_lock_init(&data->txlock);
@@ -3468,7 +3530,7 @@ static int btusb_probe(struct usb_interface *intf,
priv_size = 0;
- data->recv_event = hci_recv_frame;
+ data->recv_event = btusb_recv_evt;
data->recv_bulk = btusb_recv_bulk;
if (id->driver_info & BTUSB_INTEL_COMBINED) {
@@ -3942,6 +4004,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_poll_sync, bool, 0644);
+MODULE_PARM_DESC(enable_poll_sync, "Enable URB polling interval synchronization");
+
module_param(reset, bool, 0644);
MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
--
2.31.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] Bluetooth: btusb: Add support for queuing during polling interval
2021-11-08 23:02 [PATCH] Bluetooth: btusb: Add support for queuing during polling interval Luiz Augusto von Dentz
@ 2021-11-18 4:39 ` Marcel Holtmann
2021-11-18 16:00 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 6+ messages in thread
From: Marcel Holtmann @ 2021-11-18 4:39 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
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 runtime with use of module option:
>
> enable_poll_sync
is this still needed?
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> drivers/bluetooth/btusb.c | 89 +++++++++++++++++++++++++++++++++------
> 1 file changed, 77 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 46d892bbde62..29aa0f346ace 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -31,7 +31,7 @@
> static bool disable_scofix;
> static bool force_scofix;
> static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> -
> +static bool enable_poll_sync;
> static bool reset = true;
>
> static struct usb_driver btusb_driver;
> @@ -550,8 +550,12 @@ struct btusb_data {
>
> unsigned long flags;
>
> - struct work_struct work;
> - struct work_struct waker;
> + int intr_interval;
> + struct work_struct work;
> + struct work_struct waker;
> + struct delayed_work rx_work;
> +
> + struct sk_buff_head acl_q;
>
> struct usb_anchor deferred;
> struct usb_anchor tx_anchor;
> @@ -588,8 +592,8 @@ struct btusb_data {
> int isoc_altsetting;
> int suspend_count;
>
> - int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> int (*recv_acl)(struct hci_dev *hdev, struct sk_buff *skb);
> + int (*recv_event)(struct btusb_data *data, struct sk_buff *skb);
This change is a bit unfortunate since I really wanted to allow recv_event = hci_recv_frame assignment.
> int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
>
> int (*setup_on_usb)(struct hci_dev *hdev);
> @@ -761,7 +765,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;
> }
> }
> @@ -772,6 +776,18 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> return err;
> }
>
> +static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
> +{
> + if (!enable_poll_sync)
> + return data->recv_acl(data->hdev, skb);
> +
> + skb_queue_tail(&data->acl_q, skb);
> +
> + schedule_delayed_work(&data->rx_work, data->intr_interval);
> +
> + return 0;
> +}
> +
Starting to think about this, I really have a problem with these massive if-one-or-another checks in the main receive path. The idea was really that only hardware that needs special handling assigns different callbacks.
This means if we really want to support this, then we need to have independent recv_acl and recv_event callbacks depending on sync_poll behavior is enabled or not.
We also need to bind switching the behavior to controllers that are powered down. Otherwise this really becomes a mess. Checking a module parameter variable on every receiving packet is not a good idea.
> static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> {
> struct sk_buff *skb;
> @@ -819,7 +835,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
>
> if (!hci_skb_expect(skb)) {
> /* Complete frame */
> - data->recv_acl(data->hdev, skb);
> + btusb_recv_acl(data, skb);
> skb = NULL;
> }
> }
> @@ -971,6 +987,19 @@ static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t mem_flags)
> usb_unanchor_urb(urb);
> }
>
> + /* The units are frames (milliseconds) for full and low speed devices,
> + * and microframes (1/8 millisecond) for highspeed and SuperSpeed
> + * devices.
> + */
> + switch (urb->dev->speed) {
> + case USB_SPEED_SUPER_PLUS:
> + case USB_SPEED_SUPER: /* units are 125us */
> + data->intr_interval = usecs_to_jiffies(urb->interval * 125);
> + break;
> + default:
> + data->intr_interval = msecs_to_jiffies(urb->interval);
> + }
> +
> usb_free_urb(urb);
>
> return err;
> @@ -1430,9 +1459,12 @@ 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);
> +
> clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> @@ -1464,6 +1496,10 @@ 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);
> +
> usb_kill_anchored_urbs(&data->tx_anchor);
> btusb_free_frags(data);
>
> @@ -1827,6 +1863,17 @@ static void btusb_waker(struct work_struct *work)
> usb_autopm_put_interface(data->intf);
> }
>
> +static void btusb_rx_work(struct work_struct *work)
> +{
> + struct btusb_data *data = container_of(work, struct btusb_data,
> + rx_work.work);
> + struct sk_buff *skb;
> +
> + /* Dequeue ACL data received during the interval */
> + while ((skb = skb_dequeue(&data->acl_q)))
> + data->recv_acl(data->hdev, skb);
> +}
> +
> static int btusb_setup_bcm92035(struct hci_dev *hdev)
> {
> struct sk_buff *skb;
> @@ -2028,9 +2075,9 @@ static int btusb_recv_bulk_intel(struct btusb_data *data, void *buffer,
> return btusb_recv_bulk(data, buffer, count);
> }
>
> -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)
> {
> - if (btintel_test_flag(hdev, INTEL_BOOTLOADER)) {
> + if (btintel_test_flag(data->hdev, INTEL_BOOTLOADER)) {
> struct hci_event_hdr *hdr = (void *)skb->data;
>
> if (skb->len > HCI_EVENT_HDR_SIZE && hdr->evt == 0xff &&
> @@ -2044,7 +2091,7 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> * the device sends a vendor specific event
> * indicating that the bootup completed.
> */
> - btintel_bootup(hdev, ptr, len);
> + btintel_bootup(data->hdev, ptr, len);
> break;
> case 0x06:
> /* When the firmware loading completes the
> @@ -2052,13 +2099,14 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> * indicating the result of the firmware
> * loading.
> */
> - btintel_secure_send_result(hdev, ptr, len);
> + btintel_secure_send_result(data->hdev, ptr,
> + len);
> break;
> }
> }
> }
>
> - 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)
> @@ -3372,6 +3420,16 @@ 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_poll_sync) {
> + /* Trigger dequeue immediatelly if an event is received */
> + schedule_delayed_work(&data->rx_work, 0);
> + }
> +
> + return hci_recv_frame(data->hdev, skb);
> +}
> +
> static int btusb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> @@ -3455,6 +3513,10 @@ 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);
> +
> init_usb_anchor(&data->deferred);
> init_usb_anchor(&data->tx_anchor);
> spin_lock_init(&data->txlock);
> @@ -3468,7 +3530,7 @@ static int btusb_probe(struct usb_interface *intf,
>
> priv_size = 0;
>
> - data->recv_event = hci_recv_frame;
> + data->recv_event = btusb_recv_evt;
> data->recv_bulk = btusb_recv_bulk;
>
> if (id->driver_info & BTUSB_INTEL_COMBINED) {
> @@ -3942,6 +4004,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_poll_sync, bool, 0644);
> +MODULE_PARM_DESC(enable_poll_sync, "Enable URB polling interval synchronization");
> +
> module_param(reset, bool, 0644);
> MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
Regards
Marcel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Bluetooth: btusb: Add support for queuing during polling interval
2021-11-18 4:39 ` Marcel Holtmann
@ 2021-11-18 16:00 ` Luiz Augusto von Dentz
2021-11-18 17:17 ` Marcel Holtmann
0 siblings, 1 reply; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2021-11-18 16:00 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Wed, Nov 17, 2021 at 8:39 PM 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 runtime with use of module option:
> >
> > enable_poll_sync
>
> is this still needed?
Yes, it is not enabled by default but I can change that or perhaps we
shall use a quirk instead so models that don't have a workaround in
the firmware shall mark that quirk?
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > drivers/bluetooth/btusb.c | 89 +++++++++++++++++++++++++++++++++------
> > 1 file changed, 77 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 46d892bbde62..29aa0f346ace 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -31,7 +31,7 @@
> > static bool disable_scofix;
> > static bool force_scofix;
> > static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
> > -
> > +static bool enable_poll_sync;
> > static bool reset = true;
> >
> > static struct usb_driver btusb_driver;
> > @@ -550,8 +550,12 @@ struct btusb_data {
> >
> > unsigned long flags;
> >
> > - struct work_struct work;
> > - struct work_struct waker;
> > + int intr_interval;
> > + struct work_struct work;
> > + struct work_struct waker;
> > + struct delayed_work rx_work;
> > +
> > + struct sk_buff_head acl_q;
> >
> > struct usb_anchor deferred;
> > struct usb_anchor tx_anchor;
> > @@ -588,8 +592,8 @@ struct btusb_data {
> > int isoc_altsetting;
> > int suspend_count;
> >
> > - int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
> > int (*recv_acl)(struct hci_dev *hdev, struct sk_buff *skb);
> > + int (*recv_event)(struct btusb_data *data, struct sk_buff *skb);
>
> This change is a bit unfortunate since I really wanted to allow recv_event = hci_recv_frame assignment.
I can probably change it back and then use hci_get_drvdata if it needs queuing.
> > int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
> >
> > int (*setup_on_usb)(struct hci_dev *hdev);
> > @@ -761,7 +765,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;
> > }
> > }
> > @@ -772,6 +776,18 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> > return err;
> > }
> >
> > +static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
> > +{
> > + if (!enable_poll_sync)
> > + return data->recv_acl(data->hdev, skb);
> > +
> > + skb_queue_tail(&data->acl_q, skb);
> > +
> > + schedule_delayed_work(&data->rx_work, data->intr_interval);
> > +
> > + return 0;
> > +}
> > +
>
> Starting to think about this, I really have a problem with these massive if-one-or-another checks in the main receive path. The idea was really that only hardware that needs special handling assigns different callbacks.
>
> This means if we really want to support this, then we need to have independent recv_acl and recv_event callbacks depending on sync_poll behavior is enabled or not.
Yep, I can change that to use dedicated sync_poll callbacks variants.
> We also need to bind switching the behavior to controllers that are powered down. Otherwise this really becomes a mess. Checking a module parameter variable on every receiving packet is not a good idea.
Not sure what do you mean by bind switching, the module's parameters
cannot be changed while the module is loaded, right? So it shouldn't
really change during the lifetime of an hci_dev if that is your
concern, but yes I got the idea that we shouldn't need to check it on
every packet so using dedicated callbacks makes sense.
> > static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> > {
> > struct sk_buff *skb;
> > @@ -819,7 +835,7 @@ static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> >
> > if (!hci_skb_expect(skb)) {
> > /* Complete frame */
> > - data->recv_acl(data->hdev, skb);
> > + btusb_recv_acl(data, skb);
> > skb = NULL;
> > }
> > }
> > @@ -971,6 +987,19 @@ static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t mem_flags)
> > usb_unanchor_urb(urb);
> > }
> >
> > + /* The units are frames (milliseconds) for full and low speed devices,
> > + * and microframes (1/8 millisecond) for highspeed and SuperSpeed
> > + * devices.
> > + */
> > + switch (urb->dev->speed) {
> > + case USB_SPEED_SUPER_PLUS:
> > + case USB_SPEED_SUPER: /* units are 125us */
> > + data->intr_interval = usecs_to_jiffies(urb->interval * 125);
> > + break;
> > + default:
> > + data->intr_interval = msecs_to_jiffies(urb->interval);
> > + }
> > +
> > usb_free_urb(urb);
> >
> > return err;
> > @@ -1430,9 +1459,12 @@ 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);
> > +
> > clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> > clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> > clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> > @@ -1464,6 +1496,10 @@ 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);
> > +
> > usb_kill_anchored_urbs(&data->tx_anchor);
> > btusb_free_frags(data);
> >
> > @@ -1827,6 +1863,17 @@ static void btusb_waker(struct work_struct *work)
> > usb_autopm_put_interface(data->intf);
> > }
> >
> > +static void btusb_rx_work(struct work_struct *work)
> > +{
> > + struct btusb_data *data = container_of(work, struct btusb_data,
> > + rx_work.work);
> > + struct sk_buff *skb;
> > +
> > + /* Dequeue ACL data received during the interval */
> > + while ((skb = skb_dequeue(&data->acl_q)))
> > + data->recv_acl(data->hdev, skb);
> > +}
> > +
> > static int btusb_setup_bcm92035(struct hci_dev *hdev)
> > {
> > struct sk_buff *skb;
> > @@ -2028,9 +2075,9 @@ static int btusb_recv_bulk_intel(struct btusb_data *data, void *buffer,
> > return btusb_recv_bulk(data, buffer, count);
> > }
> >
> > -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)
> > {
> > - if (btintel_test_flag(hdev, INTEL_BOOTLOADER)) {
> > + if (btintel_test_flag(data->hdev, INTEL_BOOTLOADER)) {
> > struct hci_event_hdr *hdr = (void *)skb->data;
> >
> > if (skb->len > HCI_EVENT_HDR_SIZE && hdr->evt == 0xff &&
> > @@ -2044,7 +2091,7 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > * the device sends a vendor specific event
> > * indicating that the bootup completed.
> > */
> > - btintel_bootup(hdev, ptr, len);
> > + btintel_bootup(data->hdev, ptr, len);
> > break;
> > case 0x06:
> > /* When the firmware loading completes the
> > @@ -2052,13 +2099,14 @@ static int btusb_recv_event_intel(struct hci_dev *hdev, struct sk_buff *skb)
> > * indicating the result of the firmware
> > * loading.
> > */
> > - btintel_secure_send_result(hdev, ptr, len);
> > + btintel_secure_send_result(data->hdev, ptr,
> > + len);
> > break;
> > }
> > }
> > }
> >
> > - 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)
> > @@ -3372,6 +3420,16 @@ 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_poll_sync) {
> > + /* Trigger dequeue immediatelly if an event is received */
> > + schedule_delayed_work(&data->rx_work, 0);
> > + }
> > +
> > + return hci_recv_frame(data->hdev, skb);
> > +}
> > +
> > static int btusb_probe(struct usb_interface *intf,
> > const struct usb_device_id *id)
> > {
> > @@ -3455,6 +3513,10 @@ 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);
> > +
> > init_usb_anchor(&data->deferred);
> > init_usb_anchor(&data->tx_anchor);
> > spin_lock_init(&data->txlock);
> > @@ -3468,7 +3530,7 @@ static int btusb_probe(struct usb_interface *intf,
> >
> > priv_size = 0;
> >
> > - data->recv_event = hci_recv_frame;
> > + data->recv_event = btusb_recv_evt;
> > data->recv_bulk = btusb_recv_bulk;
> >
> > if (id->driver_info & BTUSB_INTEL_COMBINED) {
> > @@ -3942,6 +4004,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_poll_sync, bool, 0644);
> > +MODULE_PARM_DESC(enable_poll_sync, "Enable URB polling interval synchronization");
> > +
> > module_param(reset, bool, 0644);
> > MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
>
> Regards
>
> Marcel
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Bluetooth: btusb: Add support for queuing during polling interval
2021-11-18 16:00 ` Luiz Augusto von Dentz
@ 2021-11-18 17:17 ` Marcel Holtmann
0 siblings, 0 replies; 6+ messages in thread
From: Marcel Holtmann @ 2021-11-18 17:17 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
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 runtime with use of module option:
>>>
>>> enable_poll_sync
>>
>> is this still needed?
>
> Yes, it is not enabled by default but I can change that or perhaps we
> shall use a quirk instead so models that don't have a workaround in
> the firmware shall mark that quirk?
>
>>>
>>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>> ---
>>> drivers/bluetooth/btusb.c | 89 +++++++++++++++++++++++++++++++++------
>>> 1 file changed, 77 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 46d892bbde62..29aa0f346ace 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -31,7 +31,7 @@
>>> static bool disable_scofix;
>>> static bool force_scofix;
>>> static bool enable_autosuspend = IS_ENABLED(CONFIG_BT_HCIBTUSB_AUTOSUSPEND);
>>> -
>>> +static bool enable_poll_sync;
>>> static bool reset = true;
>>>
>>> static struct usb_driver btusb_driver;
>>> @@ -550,8 +550,12 @@ struct btusb_data {
>>>
>>> unsigned long flags;
>>>
>>> - struct work_struct work;
>>> - struct work_struct waker;
>>> + int intr_interval;
>>> + struct work_struct work;
>>> + struct work_struct waker;
>>> + struct delayed_work rx_work;
>>> +
>>> + struct sk_buff_head acl_q;
>>>
>>> struct usb_anchor deferred;
>>> struct usb_anchor tx_anchor;
>>> @@ -588,8 +592,8 @@ struct btusb_data {
>>> int isoc_altsetting;
>>> int suspend_count;
>>>
>>> - int (*recv_event)(struct hci_dev *hdev, struct sk_buff *skb);
>>> int (*recv_acl)(struct hci_dev *hdev, struct sk_buff *skb);
>>> + int (*recv_event)(struct btusb_data *data, struct sk_buff *skb);
>>
>> This change is a bit unfortunate since I really wanted to allow recv_event = hci_recv_frame assignment.
>
> I can probably change it back and then use hci_get_drvdata if it needs queuing.
>
>>> int (*recv_bulk)(struct btusb_data *data, void *buffer, int count);
>>>
>>> int (*setup_on_usb)(struct hci_dev *hdev);
>>> @@ -761,7 +765,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;
>>> }
>>> }
>>> @@ -772,6 +776,18 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
>>> return err;
>>> }
>>>
>>> +static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
>>> +{
>>> + if (!enable_poll_sync)
>>> + return data->recv_acl(data->hdev, skb);
>>> +
>>> + skb_queue_tail(&data->acl_q, skb);
>>> +
>>> + schedule_delayed_work(&data->rx_work, data->intr_interval);
>>> +
>>> + return 0;
>>> +}
>>> +
>>
>> Starting to think about this, I really have a problem with these massive if-one-or-another checks in the main receive path. The idea was really that only hardware that needs special handling assigns different callbacks.
>>
>> This means if we really want to support this, then we need to have independent recv_acl and recv_event callbacks depending on sync_poll behavior is enabled or not.
>
> Yep, I can change that to use dedicated sync_poll callbacks variants.
>
>> We also need to bind switching the behavior to controllers that are powered down. Otherwise this really becomes a mess. Checking a module parameter variable on every receiving packet is not a good idea.
>
> Not sure what do you mean by bind switching, the module's parameters
> cannot be changed while the module is loaded, right? So it shouldn't
> really change during the lifetime of an hci_dev if that is your
> concern, but yes I got the idea that we shouldn't need to check it on
> every packet so using dedicated callbacks makes sense.
you can change the module parameter at runtime. See the mode in module_param(enable_poll_sync, bool, 0644);.
Regards
Marcel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Bluetooth: btusb: Add support for queuing during polling interval
2021-02-08 21:32 Luiz Augusto von Dentz
@ 2021-02-09 3:04 ` Marcel Holtmann
0 siblings, 0 replies; 6+ messages in thread
From: Marcel Holtmann @ 2021-02-09 3:04 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
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 runtime with use of module option:
>
> enable_poll_sync
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> v2: Calculate the delayed_work interval based on the intr urb->interval
> which is derived from endpoint bInterval.
> v3: Drop use of a queue for events.
> v4: Rename enable_interval to enable_poll_sync and drop use of Kconfig option.
>
> drivers/bluetooth/btusb.c | 84 ++++++++++++++++++++++++++++++++++-----
> 1 file changed, 73 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index c9374ca46d87..66ada8217797 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_poll_sync;
> 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;
> + int intr_interval;
> + struct work_struct work;
> + struct work_struct waker;
> + struct delayed_work rx_work;
> +
> + struct sk_buff_head acl_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,18 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
> return err;
> }
>
> +static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
> +{
> + if (!enable_poll_sync)
> + return hci_recv_frame(data->hdev, skb);
> +
> + skb_queue_tail(&data->acl_q, skb);
> +
> + schedule_delayed_work(&data->rx_work, data->intr_interval);
> +
> + return 0;
this is purely cosmetic, but I think we should highlight the non-standard path and not the other way around.
if (enable_poll_sync) {
skb_queue_tail(&data->acl_q, skb);
schedule_delayed_work(&data->rx_work, data->intr_interval);
return 0;
}
return hci_recv_frame(data->hdev, skb);
> +}
> +
> static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
> {
> struct sk_buff *skb;
> @@ -775,7 +791,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;
> }
> }
> @@ -927,6 +943,19 @@ static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t mem_flags)
> usb_unanchor_urb(urb);
> }
>
> + /* The units are frames (milliseconds) for full and low speed devices,
> + * and microframes (1/8 millisecond) for highspeed and SuperSpeed
> + * devices.
> + */
> + switch (urb->dev->speed) {
> + case USB_SPEED_SUPER_PLUS:
> + case USB_SPEED_SUPER: /* units are 125us */
> + data->intr_interval = usecs_to_jiffies(urb->interval * 125);
> + break;
> + default:
> + data->intr_interval = msecs_to_jiffies(urb->interval);
> + }
> +
> usb_free_urb(urb);
>
> return err;
> @@ -1393,9 +1422,12 @@ 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);
> +
> clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
> clear_bit(BTUSB_BULK_RUNNING, &data->flags);
> clear_bit(BTUSB_INTR_RUNNING, &data->flags);
> @@ -1427,6 +1459,10 @@ 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);
> +
> usb_kill_anchored_urbs(&data->tx_anchor);
> btusb_free_frags(data);
>
> @@ -1786,6 +1822,17 @@ static void btusb_waker(struct work_struct *work)
> usb_autopm_put_interface(data->intf);
> }
>
> +static void btusb_rx_work(struct work_struct *work)
> +{
> + struct btusb_data *data = container_of(work, struct btusb_data,
> + rx_work.work);
> + struct sk_buff *skb;
> +
> + /* Dequeue ACL data received during the interval */
> + while ((skb = skb_dequeue(&data->acl_q)))
> + hci_recv_frame(data->hdev, skb);
> +}
> +
> static int btusb_setup_bcm92035(struct hci_dev *hdev)
> {
> struct sk_buff *skb;
> @@ -2321,10 +2368,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;
>
> @@ -2353,7 +2398,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)
> @@ -4303,6 +4348,16 @@ 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_poll_sync) {
> + /* Trigger dequeue immediatelly if an event is received */
Fix the spelling mistake.
> + schedule_delayed_work(&data->rx_work, 0);
> + }
> +
> + return hci_recv_frame(data->hdev, skb);
> +}
> +
> static int btusb_probe(struct usb_interface *intf,
> const struct usb_device_id *id)
> {
> @@ -4386,6 +4441,10 @@ 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);
> +
> init_usb_anchor(&data->deferred);
> init_usb_anchor(&data->tx_anchor);
> spin_lock_init(&data->txlock);
> @@ -4402,7 +4461,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;
> }
>
> @@ -4891,6 +4950,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_poll_sync, bool, 0644);
> +MODULE_PARM_DESC(enable_poll_sync, "Enable URB polling interval synchronization");
> +
> module_param(reset, bool, 0644);
> MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
Otherwise this looks good to me. Now we just need to get this tested if this help minimize the problem.
Regards
Marcel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] Bluetooth: btusb: Add support for queuing during polling interval
@ 2021-02-08 21:32 Luiz Augusto von Dentz
2021-02-09 3:04 ` Marcel Holtmann
0 siblings, 1 reply; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2021-02-08 21:32 UTC (permalink / raw)
To: linux-bluetooth
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 runtime with use of module option:
enable_poll_sync
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
v2: Calculate the delayed_work interval based on the intr urb->interval
which is derived from endpoint bInterval.
v3: Drop use of a queue for events.
v4: Rename enable_interval to enable_poll_sync and drop use of Kconfig option.
drivers/bluetooth/btusb.c | 84 ++++++++++++++++++++++++++++++++++-----
1 file changed, 73 insertions(+), 11 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index c9374ca46d87..66ada8217797 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_poll_sync;
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;
+ int intr_interval;
+ struct work_struct work;
+ struct work_struct waker;
+ struct delayed_work rx_work;
+
+ struct sk_buff_head acl_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,18 @@ static int btusb_recv_intr(struct btusb_data *data, void *buffer, int count)
return err;
}
+static int btusb_recv_acl(struct btusb_data *data, struct sk_buff *skb)
+{
+ if (!enable_poll_sync)
+ return hci_recv_frame(data->hdev, skb);
+
+ skb_queue_tail(&data->acl_q, skb);
+
+ schedule_delayed_work(&data->rx_work, data->intr_interval);
+
+ return 0;
+}
+
static int btusb_recv_bulk(struct btusb_data *data, void *buffer, int count)
{
struct sk_buff *skb;
@@ -775,7 +791,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;
}
}
@@ -927,6 +943,19 @@ static int btusb_submit_intr_urb(struct hci_dev *hdev, gfp_t mem_flags)
usb_unanchor_urb(urb);
}
+ /* The units are frames (milliseconds) for full and low speed devices,
+ * and microframes (1/8 millisecond) for highspeed and SuperSpeed
+ * devices.
+ */
+ switch (urb->dev->speed) {
+ case USB_SPEED_SUPER_PLUS:
+ case USB_SPEED_SUPER: /* units are 125us */
+ data->intr_interval = usecs_to_jiffies(urb->interval * 125);
+ break;
+ default:
+ data->intr_interval = msecs_to_jiffies(urb->interval);
+ }
+
usb_free_urb(urb);
return err;
@@ -1393,9 +1422,12 @@ 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);
+
clear_bit(BTUSB_ISOC_RUNNING, &data->flags);
clear_bit(BTUSB_BULK_RUNNING, &data->flags);
clear_bit(BTUSB_INTR_RUNNING, &data->flags);
@@ -1427,6 +1459,10 @@ 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);
+
usb_kill_anchored_urbs(&data->tx_anchor);
btusb_free_frags(data);
@@ -1786,6 +1822,17 @@ static void btusb_waker(struct work_struct *work)
usb_autopm_put_interface(data->intf);
}
+static void btusb_rx_work(struct work_struct *work)
+{
+ struct btusb_data *data = container_of(work, struct btusb_data,
+ rx_work.work);
+ struct sk_buff *skb;
+
+ /* Dequeue ACL data received during the interval */
+ while ((skb = skb_dequeue(&data->acl_q)))
+ hci_recv_frame(data->hdev, skb);
+}
+
static int btusb_setup_bcm92035(struct hci_dev *hdev)
{
struct sk_buff *skb;
@@ -2321,10 +2368,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;
@@ -2353,7 +2398,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)
@@ -4303,6 +4348,16 @@ 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_poll_sync) {
+ /* Trigger dequeue immediatelly if an event is received */
+ schedule_delayed_work(&data->rx_work, 0);
+ }
+
+ return hci_recv_frame(data->hdev, skb);
+}
+
static int btusb_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
@@ -4386,6 +4441,10 @@ 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);
+
init_usb_anchor(&data->deferred);
init_usb_anchor(&data->tx_anchor);
spin_lock_init(&data->txlock);
@@ -4402,7 +4461,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;
}
@@ -4891,6 +4950,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_poll_sync, bool, 0644);
+MODULE_PARM_DESC(enable_poll_sync, "Enable URB polling interval synchronization");
+
module_param(reset, bool, 0644);
MODULE_PARM_DESC(reset, "Send HCI reset command on initialization");
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-18 17:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-08 23:02 [PATCH] Bluetooth: btusb: Add support for queuing during polling interval Luiz Augusto von Dentz
2021-11-18 4:39 ` Marcel Holtmann
2021-11-18 16:00 ` Luiz Augusto von Dentz
2021-11-18 17:17 ` Marcel Holtmann
-- strict thread matches above, loose matches on Subject: below --
2021-02-08 21:32 Luiz Augusto von Dentz
2021-02-09 3:04 ` Marcel Holtmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.