Re: [PATCH v2 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260

[Miao-chen Chou]

On Wed, Mar 25, 2020 at 1:10 AM Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
>
> Hi Miao-chen,
>
> > This adds a bit mask of driver_info for Microsoft vendor extension and
> > indicates the support for Intel 9460/9560 and 9160/9260. See
> > https://docs.microsoft.com/en-us/windows-hardware/drivers/bluetooth/
> > microsoft-defined-bluetooth-hci-commands-and-events for more information
> > about the extension. This was verified with Intel ThunderPeak BT controller
> > where msft_vnd_ext_opcode is 0xFC1E.
> >
> > Signed-off-by: Miao-chen Chou <mcchou@xxxxxxxxxxxx>
> > ---
> >
> > Changes in v2:
> > - Define struct msft_vnd_ext and add a field of this type to struct
> > hci_dev to facilitate the support of Microsoft vendor extension.
> >
> > drivers/bluetooth/btusb.c        | 14 ++++++++++++--
> > include/net/bluetooth/hci_core.h |  6 ++++++
> > 2 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 3bdec42c9612..4c49f394f174 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -58,6 +58,7 @@ static struct usb_driver btusb_driver;
> > #define BTUSB_CW6622          0x100000
> > #define BTUSB_MEDIATEK                0x200000
> > #define BTUSB_WIDEBAND_SPEECH 0x400000
> > +#define BTUSB_MSFT_VND_EXT   0x800000
> >
> > static const struct usb_device_id btusb_table[] = {
> >       /* Generic Bluetooth USB device */
> > @@ -335,7 +336,8 @@ static const struct usb_device_id blacklist_table[] = {
> >
> >       /* Intel Bluetooth devices */
> >       { USB_DEVICE(0x8087, 0x0025), .driver_info = BTUSB_INTEL_NEW |
> > -                                                  BTUSB_WIDEBAND_SPEECH },
> > +                                                  BTUSB_WIDEBAND_SPEECH |
> > +                                                  BTUSB_MSFT_VND_EXT },
> >       { USB_DEVICE(0x8087, 0x0026), .driver_info = BTUSB_INTEL_NEW |
> >                                                    BTUSB_WIDEBAND_SPEECH },
> >       { USB_DEVICE(0x8087, 0x0029), .driver_info = BTUSB_INTEL_NEW |
> > @@ -348,7 +350,8 @@ static const struct usb_device_id blacklist_table[] = {
> >       { USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL |
> >                                                    BTUSB_WIDEBAND_SPEECH },
> >       { USB_DEVICE(0x8087, 0x0aaa), .driver_info = BTUSB_INTEL_NEW |
> > -                                                  BTUSB_WIDEBAND_SPEECH },
> > +                                                  BTUSB_WIDEBAND_SPEECH |
> > +                                                  BTUSB_MSFT_VND_EXT },
> >
> >       /* Other Intel Bluetooth devices */
> >       { USB_VENDOR_AND_INTERFACE_INFO(0x8087, 0xe0, 0x01, 0x01),
> > @@ -3734,6 +3737,8 @@ static int btusb_probe(struct usb_interface *intf,
> >       hdev->send   = btusb_send_frame;
> >       hdev->notify = btusb_notify;
> >
> > +     hdev->msft_ext.opcode = HCI_OP_NOP;
> > +
>
> do this in the hci_alloc_dev procedure for every driver. This doesnât belong in the driver.
Thanks for the note, I will address this.
>
> > #ifdef CONFIG_PM
> >       err = btusb_config_oob_wake(hdev);
> >       if (err)
> > @@ -3800,6 +3805,11 @@ static int btusb_probe(struct usb_interface *intf,
> >               set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
> >               set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
> >               set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
> > +
> > +             if (id->driver_info & BTUSB_MSFT_VND_EXT &&
> > +                     (id->idProduct == 0x0025 || id->idProduct == 0x0aaa)) {
>
> Please scrap this extra check. You already selected out the PID with the blacklist_table. In addition, I do not want to add a PID in two places in the driver.
If we scrap the check around idProduct, how do we tell two controllers
apart if they use different opcode for Microsoft vendor extension?
>
> An alternative is to not use BTUSB_MSFT_VND_EXT and let the Intel code set it based on the hardware / firmware revision it finds. We might need to discuss which is the better approach for the Intel hardware since not all PIDs are unique.
We are expecting to indicate the vendor extension for non-Intel
controllers as well, and having BTUSB_MSFT_VND_EXT seems to be more
generic. What do you think?
>
> > +                     hdev->msft_ext.opcode = 0xFC1E;
> > +             }
> >       }
> >
> >       if (id->driver_info & BTUSB_MARVELL)
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index d4e28773d378..0ec3d9b41d81 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/BTUSB_MSFT_VND_EXTBTUSB_MSFT_VND_EXTBTUSB_MSFT_VND_EXTbluetooth/hci_core.h
> > @@ -244,6 +244,10 @@ struct amp_assoc {
> >
> > #define HCI_MAX_PAGES 3
> >
> > +struct msft_vnd_ext {
> > +     __u16   opcode;
> > +};
> > +
> > struct hci_dev {
> >       struct list_head list;
> >       struct mutex    lock;
> > @@ -343,6 +347,8 @@ struct hci_dev {
> >
> >       struct amp_assoc        loc_assoc;
> >
> > +     struct msft_vnd_ext     msft_ext;
> > +
> >       __u8            flow_ctl_mode;
> >
> >       unsigned int    auto_accept_delay;
>
Regards,
Miao