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@xxxxxxxxx>
>>> ---
>>> 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
