| Message ID | 20200325000332.v2.1.I0e975833a6789e8acc74be7756cd54afde6ba98c@changeid (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Marcel Holtmann |
| Headers | show |
| Series | btusb: Introduce the use of vendor extension(s) | expand |
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@chromium.org> > --- > > 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. > #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. 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. > + 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/bluetooth/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 Marcel
On Wed, Mar 25, 2020 at 1:10 AM Marcel Holtmann <marcel@holtmann.org> 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@chromium.org> > > --- > > > > 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
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@chromium.org> >>> --- >>> >>> 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? for Intel controllers this is highly unlikely. If we really decide to change the opcode in newer firmware versions, we then deal with it at that point. However for Intel controllers I have the feeling that we better do it after the Read the Intel version information and then do it based on hardware revision and firmware version. >> 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? We don’t have to have one specific way of doing it. As I said, if we ever have Zephyr based controller with MSFT extension, we have a vendor command to determine the support and the opcode. So that will not require any extra quirks or alike. Anyhow, maybe we introduce BTUSB_MSFT_VND_EXT_FC1E that just says set the opcode to FC1E. For all other opcodes we will introduce similar constants. At most I assume we end up with 5-6 constants. >> >>> + hdev->msft_ext.opcode = 0xFC1E; >>> + } >>> } Regards Marcel
On Wed, Mar 25, 2020 at 2:37 PM Marcel Holtmann <marcel@holtmann.org> 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@chromium.org> > >>> --- > >>> > >>> 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? > > for Intel controllers this is highly unlikely. If we really decide to change the opcode in newer firmware versions, we then deal with it at that point. > > However for Intel controllers I have the feeling that we better do it after the Read the Intel version information and then do it based on hardware revision and firmware version. I would agree with you given that the FW loaded for the same HW can differ, and different FW version may have different configuration in terms of the use of extensions. But it's not clear to me how we can tell whether an extension is supported based on a version number. Is there any implication on the support of an extension given a FW version (e.g. any FW version greater than 10 would support MSFT 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? > > We don’t have to have one specific way of doing it. As I said, if we ever have Zephyr based controller with MSFT extension, we have a vendor command to determine the support and the opcode. So that will not require any extra quirks or alike. > > Anyhow, maybe we introduce BTUSB_MSFT_VND_EXT_FC1E that just says set the opcode to FC1E. For all other opcodes we will introduce similar constants. At most I assume we end up with 5-6 constants. > > >> > >>> + hdev->msft_ext.opcode = 0xFC1E; > >>> + } > >>> } Regards, Miao
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@chromium.org> >>>>> --- >>>>> >>>>> 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? >> >> for Intel controllers this is highly unlikely. If we really decide to change the opcode in newer firmware versions, we then deal with it at that point. >> >> However for Intel controllers I have the feeling that we better do it after the Read the Intel version information and then do it based on hardware revision and firmware version. > I would agree with you given that the FW loaded for the same HW can > differ, and different FW version may have different configuration in > terms of the use of extensions. But it's not clear to me how we can > tell whether an extension is supported based on a version number. Is > there any implication on the support of an extension given a FW > version (e.g. any FW version greater than 10 would support MSFT > extension)? that is for us to figure out. I will get back to you on that. Regards Marcel
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; + #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)) { + 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/bluetooth/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;
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@chromium.org> --- 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(-)