| Message ID | 20200323002820.v1.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 v1: > - Add a bit mask of driver_info for Microsoft vendor extension. > - Indicates the support of Microsoft vendor extension for Intel > 9460/9560 and 9160/9260. > - Add fields to struct hci_dev to facilitate the support of Microsoft > vendor extension. > > drivers/bluetooth/btusb.c | 18 ++++++++++++++++-- > include/net/bluetooth/hci.h | 2 ++ > include/net/bluetooth/hci_core.h | 4 ++++ > 3 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index 3bdec42c9612..5eb27d1c4ac7 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,11 @@ static int btusb_probe(struct usb_interface *intf, > hdev->send = btusb_send_frame; > hdev->notify = btusb_notify; > > + hdev->msft_vnd_ext_opcode = HCI_OP_NOP; > + hdev->msft_vnd_ext_features = 0; > + hdev->msft_vnd_ext_evt_prefix_len = 0; > + hdev->msft_vnd_ext_evt_prefix = NULL; > + Add the extra parameters when you start using them. Keep this patch for just the opcode. > #ifdef CONFIG_PM > err = btusb_config_oob_wake(hdev); > if (err) > @@ -3800,6 +3808,12 @@ 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)) { I don’t get the extra check here. It is not needed. Just rely on id->driver_info. > + hdev->msft_vnd_ext_opcode = > + hci_opcode_pack(HCI_VND_DEBUG_CMD_OGF, 0x001E); Don’t bother with opcode_pack here. Just assign it 0xFC1E. > + } > } > > if (id->driver_info & BTUSB_MARVELL) > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > index 5f60e135aeb6..b85e95454367 100644 > --- a/include/net/bluetooth/hci.h > +++ b/include/net/bluetooth/hci.h > @@ -38,6 +38,8 @@ > > #define HCI_MAX_CSB_DATA_SIZE 252 > > +#define HCI_VND_DEBUG_CMD_OGF 0x3f > + > /* HCI dev events */ > #define HCI_DEV_REG 1 > #define HCI_DEV_UNREG 2 > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index d4e28773d378..15daf3b2d4f0 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -315,6 +315,10 @@ struct hci_dev { > __u8 ssp_debug_mode; > __u8 hw_error_code; > __u32 clock; > + __u16 msft_vnd_ext_opcode; > + __u64 msft_vnd_ext_features; > + __u8 msft_vnd_ext_evt_prefix_len; > + void *msft_vnd_ext_evt_prefix; > > __u16 devid_source; > __u16 devid_vendor; Regards Marcel
On Mon, 2020-03-23 at 18:56 +0100, Marcel Holtmann 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. [] > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h [] > > @@ -315,6 +315,10 @@ struct hci_dev { > > __u8 ssp_debug_mode; > > __u8 hw_error_code; > > __u32 clock; > > + __u16 msft_vnd_ext_opcode; > > + __u64 msft_vnd_ext_features; > > + __u8 msft_vnd_ext_evt_prefix_len; > > + void *msft_vnd_ext_evt_prefix; msft is just another vendor. If there are to be vendor extensions, this should likely use a blank line above and below and not be prefixed with msft_
Hi Joe, >>> 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. > [] >>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > [] >>> @@ -315,6 +315,10 @@ struct hci_dev { >>> __u8 ssp_debug_mode; >>> __u8 hw_error_code; >>> __u32 clock; >>> + __u16 msft_vnd_ext_opcode; >>> + __u64 msft_vnd_ext_features; >>> + __u8 msft_vnd_ext_evt_prefix_len; >>> + void *msft_vnd_ext_evt_prefix; > > msft is just another vendor. > > If there are to be vendor extensions, this should > likely use a blank line above and below and not > be prefixed with msft_ there are other vendors, but all of them are different. So this needs to be prefixed with msft_ actually. But I agree that having empty lines above and below makes it more readable. Regards Marcel
On Mon, 2020-03-23 at 19:48 +0100, Marcel Holtmann wrote: > Hi Joe, Hello Marcel. > > > > 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. > > [] > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > [] > > > > @@ -315,6 +315,10 @@ struct hci_dev { > > > > __u8 ssp_debug_mode; > > > > __u8 hw_error_code; > > > > __u32 clock; > > > > + __u16 msft_vnd_ext_opcode; > > > > + __u64 msft_vnd_ext_features; > > > > + __u8 msft_vnd_ext_evt_prefix_len; > > > > + void *msft_vnd_ext_evt_prefix; > > > > msft is just another vendor. > > > > If there are to be vendor extensions, this should > > likely use a blank line above and below and not > > be prefixed with msft_ > > there are other vendors, but all of them are different. So this needs to be prefixed with msft_ actually. But I agree that having empty lines above and below makes it more readable. So struct hci_dev should become a clutter of random vendor extensions? Perhaps there should instead be something like an array of char at the end of the struct and various vendor specific extensions could be overlaid on that array or just add a void * to whatever info that vendors require.
On Mon, Mar 23, 2020 at 4:11 PM Joe Perches <joe@perches.com> wrote: > > On Mon, 2020-03-23 at 19:48 +0100, Marcel Holtmann wrote: > > Hi Joe, > > Hello Marcel. > > > > > > 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. > > > [] > > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > > [] > > > > > @@ -315,6 +315,10 @@ struct hci_dev { > > > > > __u8 ssp_debug_mode; > > > > > __u8 hw_error_code; > > > > > __u32 clock; > > > > > + __u16 msft_vnd_ext_opcode; > > > > > + __u64 msft_vnd_ext_features; > > > > > + __u8 msft_vnd_ext_evt_prefix_len; > > > > > + void *msft_vnd_ext_evt_prefix; > > > > > > msft is just another vendor. > > > > > > If there are to be vendor extensions, this should > > > likely use a blank line above and below and not > > > be prefixed with msft_ > > > > there are other vendors, but all of them are different. So this needs to be prefixed with msft_ actually. But I agree that having empty lines above and below makes it more readable. > > So struct hci_dev should become a clutter > of random vendor extensions? > > Perhaps there should instead be something like > an array of char at the end of the struct and > various vendor specific extensions could be > overlaid on that array or just add a void * > to whatever info that vendors require. I don't particularly like trailing buffers, but I agree we could possibly organize this a little better by with a struct. something like: struct msft_vnd_ext { bool supported; // <-- Clearly calls out if the extension is supported. __u16 msft_vnd_ext_opcode; // <-- Note that this also needs to be provided by the driver. I don't recommend we have this read from the hardware since we just cause an extra redirection that isn't necessary. Ideally, this should come from the usb_table const. __u64 msft_vnd_ext_features; __u8 msft_vnd_ext_evt_prefix_len; void *msft_vnd_ext_evt_prefix; }; And then simply add the struct msft_vnd_ext (and any others) to hci_dev. > > >
On Tue, 2020-03-24 at 11:10 -0400, Alain Michaud wrote: > On Mon, Mar 23, 2020 at 4:11 PM Joe Perches <joe@perches.com> wrote: > > On Mon, 2020-03-23 at 19:48 +0100, Marcel Holtmann wrote: > > > Hi Joe, > > > > Hello Marcel. > > > > > > > > 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. > > > > [] > > > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > > > [] > > > > > > @@ -315,6 +315,10 @@ struct hci_dev { > > > > > > __u8 ssp_debug_mode; > > > > > > __u8 hw_error_code; > > > > > > __u32 clock; > > > > > > + __u16 msft_vnd_ext_opcode; > > > > > > + __u64 msft_vnd_ext_features; > > > > > > + __u8 msft_vnd_ext_evt_prefix_len; > > > > > > + void *msft_vnd_ext_evt_prefix; > > > > > > > > msft is just another vendor. > > > > > > > > If there are to be vendor extensions, this should > > > > likely use a blank line above and below and not > > > > be prefixed with msft_ > > > > > > there are other vendors, but all of them are different. So this needs to be prefixed with msft_ actually. But I agree that having empty lines above and below makes it more readable. > > > > So struct hci_dev should become a clutter > > of random vendor extensions? > > > > Perhaps there should instead be something like > > an array of char at the end of the struct and > > various vendor specific extensions could be > > overlaid on that array or just add a void * > > to whatever info that vendors require. > I don't particularly like trailing buffers, but I agree we could > possibly organize this a little better by with a struct. something > like: > > struct msft_vnd_ext { > bool supported; // <-- Clearly calls out if the > extension is supported. > __u16 msft_vnd_ext_opcode; // <-- Note that this also > needs to be provided by the driver. I don't recommend we have this > read from the hardware since we just cause an extra redirection that > isn't necessary. Ideally, this should come from the usb_table const. > __u64 msft_vnd_ext_features; > __u8 msft_vnd_ext_evt_prefix_len; > void *msft_vnd_ext_evt_prefix; > }; > > And then simply add the struct msft_vnd_ext (and any others) to hci_dev. Or use an anonymous union
On Tue, Mar 24, 2020 at 11:19 AM Joe Perches <joe@perches.com> wrote: > > On Tue, 2020-03-24 at 11:10 -0400, Alain Michaud wrote: > > On Mon, Mar 23, 2020 at 4:11 PM Joe Perches <joe@perches.com> wrote: > > > On Mon, 2020-03-23 at 19:48 +0100, Marcel Holtmann wrote: > > > > Hi Joe, > > > > > > Hello Marcel. > > > > > > > > > > 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. > > > > > [] > > > > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > > > > [] > > > > > > > @@ -315,6 +315,10 @@ struct hci_dev { > > > > > > > __u8 ssp_debug_mode; > > > > > > > __u8 hw_error_code; > > > > > > > __u32 clock; > > > > > > > + __u16 msft_vnd_ext_opcode; > > > > > > > + __u64 msft_vnd_ext_features; > > > > > > > + __u8 msft_vnd_ext_evt_prefix_len; > > > > > > > + void *msft_vnd_ext_evt_prefix; > > > > > > > > > > msft is just another vendor. > > > > > > > > > > If there are to be vendor extensions, this should > > > > > likely use a blank line above and below and not > > > > > be prefixed with msft_ > > > > > > > > there are other vendors, but all of them are different. So this needs to be prefixed with msft_ actually. But I agree that having empty lines above and below makes it more readable. > > > > > > So struct hci_dev should become a clutter > > > of random vendor extensions? > > > > > > Perhaps there should instead be something like > > > an array of char at the end of the struct and > > > various vendor specific extensions could be > > > overlaid on that array or just add a void * > > > to whatever info that vendors require. > > I don't particularly like trailing buffers, but I agree we could > > possibly organize this a little better by with a struct. something > > like: > > > > struct msft_vnd_ext { > > bool supported; // <-- Clearly calls out if the > > extension is supported. > > __u16 msft_vnd_ext_opcode; // <-- Note that this also > > needs to be provided by the driver. I don't recommend we have this > > read from the hardware since we just cause an extra redirection that > > isn't necessary. Ideally, this should come from the usb_table const. > > __u64 msft_vnd_ext_features; > > __u8 msft_vnd_ext_evt_prefix_len; > > void *msft_vnd_ext_evt_prefix; > > }; > > > > And then simply add the struct msft_vnd_ext (and any others) to hci_dev. > > Or use an anonymous union That would also work, but would need to be an array of unions, perhaps following your original idea to have them be in a trailing array of unions since a controller may support more than one extension. This might be going overboard :) > > >
On Tue, 2020-03-24 at 11:24 -0400, Alain Michaud wrote: > On Tue, Mar 24, 2020 at 11:19 AM Joe Perches <joe@perches.com> wrote: > > On Tue, 2020-03-24 at 11:10 -0400, Alain Michaud wrote: > > > On Mon, Mar 23, 2020 at 4:11 PM Joe Perches <joe@perches.com> wrote: > > > > On Mon, 2020-03-23 at 19:48 +0100, Marcel Holtmann wrote: > > > > > Hi Joe, > > > > > > > > Hello Marcel. > > > > > > > > > > > > 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. > > > > > > [] > > > > > > > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > > > > > [] > > > > > > > > @@ -315,6 +315,10 @@ struct hci_dev { > > > > > > > > __u8 ssp_debug_mode; > > > > > > > > __u8 hw_error_code; > > > > > > > > __u32 clock; > > > > > > > > + __u16 msft_vnd_ext_opcode; > > > > > > > > + __u64 msft_vnd_ext_features; > > > > > > > > + __u8 msft_vnd_ext_evt_prefix_len; > > > > > > > > + void *msft_vnd_ext_evt_prefix; > > > > > > > > > > > > msft is just another vendor. > > > > > > > > > > > > If there are to be vendor extensions, this should > > > > > > likely use a blank line above and below and not > > > > > > be prefixed with msft_ > > > > > > > > > > there are other vendors, but all of them are different. So this needs to be prefixed with msft_ actually. But I agree that having empty lines above and below makes it more readable. > > > > > > > > So struct hci_dev should become a clutter > > > > of random vendor extensions? > > > > > > > > Perhaps there should instead be something like > > > > an array of char at the end of the struct and > > > > various vendor specific extensions could be > > > > overlaid on that array or just add a void * > > > > to whatever info that vendors require. > > > I don't particularly like trailing buffers, but I agree we could > > > possibly organize this a little better by with a struct. something > > > like: > > > > > > struct msft_vnd_ext { > > > bool f supported; // <-- Clearly calls out if the > > > extension is supported. > > > __u16 msft_vnd_ext_opcode; // <-- Note that this also > > > needs to be provided by the driver. I don't recommend we have this > > > read from the hardware since we just cause an extra redirection that > > > isn't necessary. Ideally, this should come from the usb_table const. > > > __u64 msft_vnd_ext_features; > > > __u8 msft_vnd_ext_evt_prefix_len; > > > void *msft_vnd_ext_evt_prefix; > > > }; > > > > > > And then simply add the struct msft_vnd_ext (and any others) to hci_dev. > > > > Or use an anonymous union > That would also work, but would need to be an array of unions, perhaps > following your original idea to have them be in a trailing array of > unions since a controller may support more than one extension. This > might be going overboard :) True. Especially true if the controller supports multiple concurrent extensions.
Hi Alain, >>>>>> 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. >>>> [] >>>>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h >>>> [] >>>>>> @@ -315,6 +315,10 @@ struct hci_dev { >>>>>> __u8 ssp_debug_mode; >>>>>> __u8 hw_error_code; >>>>>> __u32 clock; >>>>>> + __u16 msft_vnd_ext_opcode; >>>>>> + __u64 msft_vnd_ext_features; >>>>>> + __u8 msft_vnd_ext_evt_prefix_len; >>>>>> + void *msft_vnd_ext_evt_prefix; >>>> >>>> msft is just another vendor. >>>> >>>> If there are to be vendor extensions, this should >>>> likely use a blank line above and below and not >>>> be prefixed with msft_ >>> >>> there are other vendors, but all of them are different. So this needs to be prefixed with msft_ actually. But I agree that having empty lines above and below makes it more readable. >> >> So struct hci_dev should become a clutter >> of random vendor extensions? >> >> Perhaps there should instead be something like >> an array of char at the end of the struct and >> various vendor specific extensions could be >> overlaid on that array or just add a void * >> to whatever info that vendors require. > I don't particularly like trailing buffers, but I agree we could > possibly organize this a little better by with a struct. something > like: > > struct msft_vnd_ext { > bool supported; // <-- Clearly calls out if the > extension is supported. > __u16 msft_vnd_ext_opcode; // <-- Note that this also > needs to be provided by the driver. I don't recommend we have this > read from the hardware since we just cause an extra redirection that > isn't necessary. Ideally, this should come from the usb_table const. Actually supported == false is the same as opcode == 0x0000. And supported == true is opcode != 0x0000. > __u64 msft_vnd_ext_features; > __u8 msft_vnd_ext_evt_prefix_len; > void *msft_vnd_ext_evt_prefix; > }; > > And then simply add the struct msft_vnd_ext (and any others) to hci_dev. Anyway, Lets keep these for now as hci_dev->msft_vnd_ext_*. We can fix this up later without any impact. Regards Marcel
On Tue, Mar 24, 2020 at 2:35 PM Marcel Holtmann <marcel@holtmann.org> wrote: > > Hi Alain, > > >>>>>> 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. > >>>> [] > >>>>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > >>>> [] > >>>>>> @@ -315,6 +315,10 @@ struct hci_dev { > >>>>>> __u8 ssp_debug_mode; > >>>>>> __u8 hw_error_code; > >>>>>> __u32 clock; > >>>>>> + __u16 msft_vnd_ext_opcode; > >>>>>> + __u64 msft_vnd_ext_features; > >>>>>> + __u8 msft_vnd_ext_evt_prefix_len; > >>>>>> + void *msft_vnd_ext_evt_prefix; > >>>> > >>>> msft is just another vendor. > >>>> > >>>> If there are to be vendor extensions, this should > >>>> likely use a blank line above and below and not > >>>> be prefixed with msft_ > >>> > >>> there are other vendors, but all of them are different. So this needs to be prefixed with msft_ actually. But I agree that having empty lines above and below makes it more readable. > >> > >> So struct hci_dev should become a clutter > >> of random vendor extensions? > >> > >> Perhaps there should instead be something like > >> an array of char at the end of the struct and > >> various vendor specific extensions could be > >> overlaid on that array or just add a void * > >> to whatever info that vendors require. > > I don't particularly like trailing buffers, but I agree we could > > possibly organize this a little better by with a struct. something > > like: > > > > struct msft_vnd_ext { > > bool supported; // <-- Clearly calls out if the > > extension is supported. > > __u16 msft_vnd_ext_opcode; // <-- Note that this also > > needs to be provided by the driver. I don't recommend we have this > > read from the hardware since we just cause an extra redirection that > > isn't necessary. Ideally, this should come from the usb_table const. > > Actually supported == false is the same as opcode == 0x0000. And supported == true is opcode != 0x0000. I was thinking of a more generic way to check if the extension is supported so the higher level doesn't need to understand that opcode==0 means it's not supported. For the android extension for example, this would be a simple boolean (there isn't any opcodes). > > > __u64 msft_vnd_ext_features; > > __u8 msft_vnd_ext_evt_prefix_len; > > void *msft_vnd_ext_evt_prefix; > > }; > > > > And then simply add the struct msft_vnd_ext (and any others) to hci_dev. > > Anyway, Lets keep these for now as hci_dev->msft_vnd_ext_*. We can fix this up later without any impact. I agree, this doesn't have a whole lot of long term consequences, although some will want to cherry-pick this to older kernels so if there is something we can do now, it will reduce burden on some products. > > Regards > > Marcel >
On Tue, Mar 24, 2020 at 12:32 PM Alain Michaud <alainmichaud@google.com> wrote: > > On Tue, Mar 24, 2020 at 2:35 PM Marcel Holtmann <marcel@holtmann.org> wrote: > > > > Hi Alain, > > > > >>>>>> 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. > > >>>> [] > > >>>>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > > >>>> [] > > >>>>>> @@ -315,6 +315,10 @@ struct hci_dev { > > >>>>>> __u8 ssp_debug_mode; > > >>>>>> __u8 hw_error_code; > > >>>>>> __u32 clock; > > >>>>>> + __u16 msft_vnd_ext_opcode; > > >>>>>> + __u64 msft_vnd_ext_features; > > >>>>>> + __u8 msft_vnd_ext_evt_prefix_len; > > >>>>>> + void *msft_vnd_ext_evt_prefix; > > >>>> > > >>>> msft is just another vendor. > > >>>> > > >>>> If there are to be vendor extensions, this should > > >>>> likely use a blank line above and below and not > > >>>> be prefixed with msft_ > > >>> > > >>> there are other vendors, but all of them are different. So this needs to be prefixed with msft_ actually. But I agree that having empty lines above and below makes it more readable. > > >> > > >> So struct hci_dev should become a clutter > > >> of random vendor extensions? > > >> > > >> Perhaps there should instead be something like > > >> an array of char at the end of the struct and > > >> various vendor specific extensions could be > > >> overlaid on that array or just add a void * > > >> to whatever info that vendors require. > > > I don't particularly like trailing buffers, but I agree we could > > > possibly organize this a little better by with a struct. something > > > like: > > > > > > struct msft_vnd_ext { > > > bool supported; // <-- Clearly calls out if the > > > extension is supported. > > > __u16 msft_vnd_ext_opcode; // <-- Note that this also > > > needs to be provided by the driver. I don't recommend we have this > > > read from the hardware since we just cause an extra redirection that > > > isn't necessary. Ideally, this should come from the usb_table const. > > > > Actually supported == false is the same as opcode == 0x0000. And supported == true is opcode != 0x0000. > I was thinking of a more generic way to check if the extension is > supported so the higher level doesn't need to understand that > opcode==0 means it's not supported. For the android extension for > example, this would be a simple boolean (there isn't any opcodes). > > > > > __u64 msft_vnd_ext_features; > > > __u8 msft_vnd_ext_evt_prefix_len; > > > void *msft_vnd_ext_evt_prefix; > > > }; > > > > > > And then simply add the struct msft_vnd_ext (and any others) to hci_dev. > > > > Anyway, Lets keep these for now as hci_dev->msft_vnd_ext_*. We can fix this up later without any impact. > I agree, this doesn't have a whole lot of long term consequences, > although some will want to cherry-pick this to older kernels so if > there is something we can do now, it will reduce burden on some > products. Thanks for all your inputs. I will group these msft_vnd_ext_* into a struct msft_vnd_ext with future refactoring in mind if new extensions are introduced.
Hi Alain, >>>>>>>> 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. >>>>>> [] >>>>>>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h >>>>>> [] >>>>>>>> @@ -315,6 +315,10 @@ struct hci_dev { >>>>>>>> __u8 ssp_debug_mode; >>>>>>>> __u8 hw_error_code; >>>>>>>> __u32 clock; >>>>>>>> + __u16 msft_vnd_ext_opcode; >>>>>>>> + __u64 msft_vnd_ext_features; >>>>>>>> + __u8 msft_vnd_ext_evt_prefix_len; >>>>>>>> + void *msft_vnd_ext_evt_prefix; >>>>>> >>>>>> msft is just another vendor. >>>>>> >>>>>> If there are to be vendor extensions, this should >>>>>> likely use a blank line above and below and not >>>>>> be prefixed with msft_ >>>>> >>>>> there are other vendors, but all of them are different. So this needs to be prefixed with msft_ actually. But I agree that having empty lines above and below makes it more readable. >>>> >>>> So struct hci_dev should become a clutter >>>> of random vendor extensions? >>>> >>>> Perhaps there should instead be something like >>>> an array of char at the end of the struct and >>>> various vendor specific extensions could be >>>> overlaid on that array or just add a void * >>>> to whatever info that vendors require. >>> I don't particularly like trailing buffers, but I agree we could >>> possibly organize this a little better by with a struct. something >>> like: >>> >>> struct msft_vnd_ext { >>> bool supported; // <-- Clearly calls out if the >>> extension is supported. >>> __u16 msft_vnd_ext_opcode; // <-- Note that this also >>> needs to be provided by the driver. I don't recommend we have this >>> read from the hardware since we just cause an extra redirection that >>> isn't necessary. Ideally, this should come from the usb_table const. >> >> Actually supported == false is the same as opcode == 0x0000. And supported == true is opcode != 0x0000. > I was thinking of a more generic way to check if the extension is > supported so the higher level doesn't need to understand that > opcode==0 means it's not supported. For the android extension for > example, this would be a simple boolean (there isn't any opcodes). since the extensions are not equal, I think there is no point in trying to generalize it in hci_dev. Here we have to do the heavy lifting anyway to make this fly. Then again, lets focus on the msft ones first. Keep it simple. And then we look at how we extend this to other extensions. >> >>> __u64 msft_vnd_ext_features; >>> __u8 msft_vnd_ext_evt_prefix_len; >>> void *msft_vnd_ext_evt_prefix; >>> }; >>> >>> And then simply add the struct msft_vnd_ext (and any others) to hci_dev. >> >> Anyway, Lets keep these for now as hci_dev->msft_vnd_ext_*. We can fix this up later without any impact. > I agree, this doesn't have a whole lot of long term consequences, > although some will want to cherry-pick this to older kernels so if > there is something we can do now, it will reduce burden on some > products. You end up having to pick up everything anyway. So I doubt it will make a huge difference. We can always evolve the patches before applying parts of it. Personally I like to get things that look sane and clean applied to we widen the audience of testers. Regards Marcel
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index 3bdec42c9612..5eb27d1c4ac7 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,11 @@ static int btusb_probe(struct usb_interface *intf, hdev->send = btusb_send_frame; hdev->notify = btusb_notify; + hdev->msft_vnd_ext_opcode = HCI_OP_NOP; + hdev->msft_vnd_ext_features = 0; + hdev->msft_vnd_ext_evt_prefix_len = 0; + hdev->msft_vnd_ext_evt_prefix = NULL; + #ifdef CONFIG_PM err = btusb_config_oob_wake(hdev); if (err) @@ -3800,6 +3808,12 @@ 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_vnd_ext_opcode = + hci_opcode_pack(HCI_VND_DEBUG_CMD_OGF, 0x001E); + } } if (id->driver_info & BTUSB_MARVELL) diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h index 5f60e135aeb6..b85e95454367 100644 --- a/include/net/bluetooth/hci.h +++ b/include/net/bluetooth/hci.h @@ -38,6 +38,8 @@ #define HCI_MAX_CSB_DATA_SIZE 252 +#define HCI_VND_DEBUG_CMD_OGF 0x3f + /* HCI dev events */ #define HCI_DEV_REG 1 #define HCI_DEV_UNREG 2 diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index d4e28773d378..15daf3b2d4f0 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -315,6 +315,10 @@ struct hci_dev { __u8 ssp_debug_mode; __u8 hw_error_code; __u32 clock; + __u16 msft_vnd_ext_opcode; + __u64 msft_vnd_ext_features; + __u8 msft_vnd_ext_evt_prefix_len; + void *msft_vnd_ext_evt_prefix; __u16 devid_source; __u16 devid_vendor;
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 v1: - Add a bit mask of driver_info for Microsoft vendor extension. - Indicates the support of Microsoft vendor extension for Intel 9460/9560 and 9160/9260. - Add fields to struct hci_dev to facilitate the support of Microsoft vendor extension. drivers/bluetooth/btusb.c | 18 ++++++++++++++++-- include/net/bluetooth/hci.h | 2 ++ include/net/bluetooth/hci_core.h | 4 ++++ 3 files changed, 22 insertions(+), 2 deletions(-)