Re: [PATCH] Bluetooth: btusb: Add support for queuing during polling interval

Hi Luiz,

quoted

quoted

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?

quoted

quoted

Signed-off-by: Luiz Augusto von Dentz <redacted>
---
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.

quoted

quoted

     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.

quoted

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