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

[Luiz Augusto von Dentz <luiz.dentz@gmail.com>]

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