* [PATCH v1 0/2] btusb: Introduce the use of vendor extension(s) @ 2020-03-23 7:28 Miao-chen Chou 2020-03-23 7:28 ` [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 Miao-chen Chou 2020-03-23 7:28 ` [PATCH v1 2/2] Bluetooth: btusb: Read the supported features of Microsoft vendor extension Miao-chen Chou 0 siblings, 2 replies; 17+ messages in thread From: Miao-chen Chou @ 2020-03-23 7:28 UTC (permalink / raw) To: Bluetooth Kernel Mailing List Cc: Luiz Augusto von Dentz, Marcel Holtmann, Alain Michaud, Miao-chen Chou, David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel, netdev Hi Marcel and Luiz, The standard HCI does not provide commands/events regarding to advertisement monitoring with content filter while there are few vendors providing this feature. Chrome OS BT would like to introduce the use of vendor specific features where Microsoft vendor extension is targeted at this moment. Chrome OS BT would like to utilize Microsoft vendor extension's advertisement monitoring feature which is not yet a part of standard Bluetooth specification. This series introduces the driver information for Microsoft vendor extension, and this was verified with kernel 4.4 on Atlas Chromebook. Thanks Miao 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. - Add vendor_hci.h to facilitate opcodes and packet structures of vendor extension(s). - Add opcode for the HCI_VS_MSFT_Read_Supported_Features command from Microsoft vendor extension. - Issue a HCI_VS_MSFT_Read_Supported_Features command upon hci_dev_do_open and save the return values. Miao-chen Chou (2): Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 Bluetooth: btusb: Read the supported features of Microsoft vendor extension drivers/bluetooth/btusb.c | 18 +++++++++-- include/net/bluetooth/hci.h | 2 ++ include/net/bluetooth/hci_core.h | 5 +++ include/net/bluetooth/vendor_hci.h | 51 ++++++++++++++++++++++++++++++ net/bluetooth/hci_core.c | 30 ++++++++++++++++++ net/bluetooth/hci_event.c | 49 +++++++++++++++++++++++++++- 6 files changed, 152 insertions(+), 3 deletions(-) create mode 100644 include/net/bluetooth/vendor_hci.h -- 2.24.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 2020-03-23 7:28 [PATCH v1 0/2] btusb: Introduce the use of vendor extension(s) Miao-chen Chou @ 2020-03-23 7:28 ` Miao-chen Chou 2020-03-23 17:56 ` Marcel Holtmann 2020-03-23 7:28 ` [PATCH v1 2/2] Bluetooth: btusb: Read the supported features of Microsoft vendor extension Miao-chen Chou 1 sibling, 1 reply; 17+ messages in thread From: Miao-chen Chou @ 2020-03-23 7:28 UTC (permalink / raw) To: Bluetooth Kernel Mailing List Cc: Luiz Augusto von Dentz, Marcel Holtmann, Alain Michaud, Miao-chen Chou, David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel, netdev 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; + #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; -- 2.24.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 2020-03-23 7:28 ` [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 Miao-chen Chou @ 2020-03-23 17:56 ` Marcel Holtmann 2020-03-23 18:45 ` Joe Perches 0 siblings, 1 reply; 17+ messages in thread From: Marcel Holtmann @ 2020-03-23 17:56 UTC (permalink / raw) To: Miao-chen Chou Cc: Bluetooth Kernel Mailing List, Luiz Augusto von Dentz, Alain Michaud, David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel, netdev 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 2020-03-23 17:56 ` Marcel Holtmann @ 2020-03-23 18:45 ` Joe Perches 2020-03-23 18:48 ` Marcel Holtmann 0 siblings, 1 reply; 17+ messages in thread From: Joe Perches @ 2020-03-23 18:45 UTC (permalink / raw) To: Marcel Holtmann, Miao-chen Chou Cc: Bluetooth Kernel Mailing List, Luiz Augusto von Dentz, Alain Michaud, David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel, netdev 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_ ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 2020-03-23 18:45 ` Joe Perches @ 2020-03-23 18:48 ` Marcel Holtmann 2020-03-23 20:09 ` Joe Perches 0 siblings, 1 reply; 17+ messages in thread From: Marcel Holtmann @ 2020-03-23 18:48 UTC (permalink / raw) To: Joe Perches Cc: Miao-chen Chou, Bluetooth Kernel Mailing List, Luiz Augusto von Dentz, Alain Michaud, David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel, netdev 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 2020-03-23 18:48 ` Marcel Holtmann @ 2020-03-23 20:09 ` Joe Perches 2020-03-24 15:10 ` Alain Michaud 0 siblings, 1 reply; 17+ messages in thread From: Joe Perches @ 2020-03-23 20:09 UTC (permalink / raw) To: Marcel Holtmann Cc: Miao-chen Chou, Bluetooth Kernel Mailing List, Luiz Augusto von Dentz, Alain Michaud, David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel, netdev 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 2020-03-23 20:09 ` Joe Perches @ 2020-03-24 15:10 ` Alain Michaud 2020-03-24 15:17 ` Joe Perches 2020-03-24 18:35 ` Marcel Holtmann 0 siblings, 2 replies; 17+ messages in thread From: Alain Michaud @ 2020-03-24 15:10 UTC (permalink / raw) To: Joe Perches Cc: Marcel Holtmann, Miao-chen Chou, Bluetooth Kernel Mailing List, Luiz Augusto von Dentz, Alain Michaud, David S. Miller, Jakub Kicinski, Johan Hedberg, LKML, netdev 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. > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 2020-03-24 15:10 ` Alain Michaud @ 2020-03-24 15:17 ` Joe Perches 2020-03-24 15:24 ` Alain Michaud 2020-03-24 18:35 ` Marcel Holtmann 1 sibling, 1 reply; 17+ messages in thread From: Joe Perches @ 2020-03-24 15:17 UTC (permalink / raw) To: Alain Michaud Cc: Marcel Holtmann, Miao-chen Chou, Bluetooth Kernel Mailing List, Luiz Augusto von Dentz, Alain Michaud, David S. Miller, Jakub Kicinski, Johan Hedberg, LKML, netdev 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 2020-03-24 15:17 ` Joe Perches @ 2020-03-24 15:24 ` Alain Michaud 2020-03-24 16:45 ` Joe Perches 0 siblings, 1 reply; 17+ messages in thread From: Alain Michaud @ 2020-03-24 15:24 UTC (permalink / raw) To: Joe Perches Cc: Marcel Holtmann, Miao-chen Chou, Bluetooth Kernel Mailing List, Luiz Augusto von Dentz, Alain Michaud, David S. Miller, Jakub Kicinski, Johan Hedberg, LKML, netdev 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 :) > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 2020-03-24 15:24 ` Alain Michaud @ 2020-03-24 16:45 ` Joe Perches 0 siblings, 0 replies; 17+ messages in thread From: Joe Perches @ 2020-03-24 16:45 UTC (permalink / raw) To: Alain Michaud Cc: Marcel Holtmann, Miao-chen Chou, Bluetooth Kernel Mailing List, Luiz Augusto von Dentz, Alain Michaud, David S. Miller, Jakub Kicinski, Johan Hedberg, LKML, netdev 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 2020-03-24 15:10 ` Alain Michaud 2020-03-24 15:17 ` Joe Perches @ 2020-03-24 18:35 ` Marcel Holtmann 2020-03-24 19:32 ` Alain Michaud 1 sibling, 1 reply; 17+ messages in thread From: Marcel Holtmann @ 2020-03-24 18:35 UTC (permalink / raw) To: Alain Michaud Cc: Joe Perches, Miao-chen Chou, Bluetooth Kernel Mailing List, Luiz Augusto von Dentz, Alain Michaud, David S. Miller, Jakub Kicinski, Johan Hedberg, LKML, netdev 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 2020-03-24 18:35 ` Marcel Holtmann @ 2020-03-24 19:32 ` Alain Michaud 2020-03-25 5:18 ` Miao-chen Chou 2020-03-25 8:02 ` Marcel Holtmann 0 siblings, 2 replies; 17+ messages in thread From: Alain Michaud @ 2020-03-24 19:32 UTC (permalink / raw) To: Marcel Holtmann Cc: Joe Perches, Miao-chen Chou, Bluetooth Kernel Mailing List, Luiz Augusto von Dentz, Alain Michaud, David S. Miller, Jakub Kicinski, Johan Hedberg, LKML, netdev 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 2020-03-24 19:32 ` Alain Michaud @ 2020-03-25 5:18 ` Miao-chen Chou 2020-03-25 8:02 ` Marcel Holtmann 1 sibling, 0 replies; 17+ messages in thread From: Miao-chen Chou @ 2020-03-25 5:18 UTC (permalink / raw) To: Alain Michaud Cc: Marcel Holtmann, Joe Perches, Bluetooth Kernel Mailing List, Luiz Augusto von Dentz, Alain Michaud, David S. Miller, Jakub Kicinski, Johan Hedberg, LKML, netdev 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 2020-03-24 19:32 ` Alain Michaud 2020-03-25 5:18 ` Miao-chen Chou @ 2020-03-25 8:02 ` Marcel Holtmann 1 sibling, 0 replies; 17+ messages in thread From: Marcel Holtmann @ 2020-03-25 8:02 UTC (permalink / raw) To: Alain Michaud Cc: Joe Perches, Miao-chen Chou, Bluetooth Kernel Mailing List, Luiz Augusto von Dentz, Alain Michaud, David S. Miller, Jakub Kicinski, Johan Hedberg, LKML, netdev 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v1 2/2] Bluetooth: btusb: Read the supported features of Microsoft vendor extension 2020-03-23 7:28 [PATCH v1 0/2] btusb: Introduce the use of vendor extension(s) Miao-chen Chou 2020-03-23 7:28 ` [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 Miao-chen Chou @ 2020-03-23 7:28 ` Miao-chen Chou 2020-03-23 17:30 ` kbuild test robot 2020-03-23 18:05 ` Marcel Holtmann 1 sibling, 2 replies; 17+ messages in thread From: Miao-chen Chou @ 2020-03-23 7:28 UTC (permalink / raw) To: Bluetooth Kernel Mailing List Cc: Luiz Augusto von Dentz, Marcel Holtmann, Alain Michaud, Miao-chen Chou, David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel, netdev This adds a new header to facilitate the opcode and packet structures of vendor extension(s). For now, we add only the HCI_VS_MSFT_Read_Supported_Features command from Microsoft vendor extension. See https://docs.microsoft.com/en-us/windows-hardware/drivers/ bluetooth/microsoft-defined-bluetooth-hci-commands-and-events# microsoft-defined-bluetooth-hci-events for more details. Upon initialization of a hci_dev, we issue a HCI_VS_MSFT_Read_Supported_Features command to read the supported features of Microsoft vendor extension if the opcode of Microsoft vendor extension is valid. See https://docs.microsoft.com/en-us/windows-hardware/drivers/ bluetooth/microsoft-defined-bluetooth-hci-commands-and-events# hci_vs_msft_read_supported_features for more details. This was verified on a device with Intel ThhunderPeak BT controller where the Microsoft vendor extension features are 0x000000000000003f. Signed-off-by: Miao-chen Chou <mcchou@chromium.org> --- Changes in v1: - Add vendor_hci.h to facilitate opcodes and packet structures of vendor extension(s). - Add opcode for the HCI_VS_MSFT_Read_Supported_Features command from Microsoft vendor extension. - Issue a HCI_VS_MSFT_Read_Supported_Features command upon hci_dev_do_open and save the return values. include/net/bluetooth/hci_core.h | 1 + include/net/bluetooth/vendor_hci.h | 51 ++++++++++++++++++++++++++++++ net/bluetooth/hci_core.c | 30 ++++++++++++++++++ net/bluetooth/hci_event.c | 49 +++++++++++++++++++++++++++- 4 files changed, 130 insertions(+), 1 deletion(-) create mode 100644 include/net/bluetooth/vendor_hci.h diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index 15daf3b2d4f0..941e71bdad42 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -30,6 +30,7 @@ #include <net/bluetooth/hci.h> #include <net/bluetooth/hci_sock.h> +#include <net/bluetooth/vendor_hci.h> /* HCI priority */ #define HCI_PRIO_MAX 7 diff --git a/include/net/bluetooth/vendor_hci.h b/include/net/bluetooth/vendor_hci.h new file mode 100644 index 000000000000..bd374a7bf976 --- /dev/null +++ b/include/net/bluetooth/vendor_hci.h @@ -0,0 +1,51 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * BlueZ - Bluetooth protocol stack for Linux + * Copyright (C) 2020 Google Corporation + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation; + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT OF THIRD PARTY RIGHTS. + * IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) AND AUTHOR(S) BE LIABLE FOR ANY + * CLAIM, OR ANY SPECIAL INDIRECT OR CONSEQUENTIAL DAMAGES, OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + * + * ALL LIABILITY, INCLUDING LIABILITY FOR INFRINGEMENT OF ANY PATENTS, + * COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS + * SOFTWARE IS DISCLAIMED. + */ + +#ifndef __VENDOR_HCI_H +#define __VENDOR_HCI_H + +#define MSFT_EVT_PREFIX_MAX_LEN 255 + +struct msft_cmd_cmp_info { + __u8 status; + __u8 sub_opcode; +} __packed; + +/* Microsoft Vendor HCI subcommands */ +#define MSFT_OP_READ_SUPPORTED_FEATURES 0x00 +#define MSFT_FEATURE_MASK_RSSI_MONITOR_BREDR_CONN 0x0000000000000001 +#define MSFT_FEATURE_MASK_RSSI_MONITOR_LE_CONN 0x0000000000000002 +#define MSFT_FEATURE_MASK_RSSI_MONITOR_LE_ADV 0x0000000000000004 +#define MSFT_FEATURE_MASK_ADV_MONITOR_LE_ADV 0x0000000000000008 +#define MSFT_FEATURE_MASK_VERIFY_CURVE_ 0x0000000000000010 +#define MSFT_FEATURE_MASK_CONCURRENT_ADV_MONITOR 0x0000000000000020 +struct msft_cp_read_supported_features { + __u8 sub_opcode; +} __packed; +struct msft_rp_read_supported_features { + __u64 features; + __u8 msft_evt_prefix_len; + __u8 msft_evt_prefix[0]; +} __packed; + +#endif /* __VENDOR_HCI_H */ diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index dbd2ad3a26ed..3cec58ca0047 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -1407,6 +1407,32 @@ static void hci_dev_get_bd_addr_from_property(struct hci_dev *hdev) bacpy(&hdev->public_addr, &ba); } +static int hci_msft_read_supported_features_req(struct hci_request *req, + unsigned long opt) +{ + struct hci_dev *hdev = req->hdev; + struct msft_cp_read_supported_features cp; + + if (hdev->msft_vnd_ext_opcode == HCI_OP_NOP) + return -EOPNOTSUPP; + + cp.sub_opcode = MSFT_OP_READ_SUPPORTED_FEATURES; + hci_req_add(req, hdev->msft_vnd_ext_opcode, sizeof(cp), &cp); + return 0; +} + +static void read_vendor_extension_features(struct hci_dev *hdev) +{ + int err; + + if (hdev->msft_vnd_ext_opcode != HCI_OP_NOP) { + err = __hci_req_sync(hdev, hci_msft_read_supported_features_req, + 0, HCI_CMD_TIMEOUT, NULL); + if (err) + BT_ERR("HCI_VS_MSFT_Read_Supported_Feature failed"); + } +} + static int hci_dev_do_open(struct hci_dev *hdev) { int ret = 0; @@ -1572,6 +1598,10 @@ static int hci_dev_do_open(struct hci_dev *hdev) set_bit(HCI_UP, &hdev->flags); hci_sock_dev_event(hdev, HCI_DEV_UP); hci_leds_update_powered(hdev, true); + + /* Check features supported by HCI extensions. */ + read_vendor_extension_features(hdev); + if (!hci_dev_test_flag(hdev, HCI_SETUP) && !hci_dev_test_flag(hdev, HCI_CONFIG) && !hci_dev_test_flag(hdev, HCI_UNCONFIGURED) && diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 20408d386268..6a156167dfc9 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -23,7 +23,6 @@ */ /* Bluetooth HCI event handling. */ - #include <asm/unaligned.h> #include <net/bluetooth/bluetooth.h> @@ -1787,6 +1786,47 @@ static void hci_cc_write_ssp_debug_mode(struct hci_dev *hdev, struct sk_buff *sk hdev->ssp_debug_mode = *mode; } +static void hci_cc_msft_vnd_ext(struct hci_dev *hdev, struct sk_buff *skb) +{ + struct msft_cmd_cmp_info *info = (void *)skb->data; + const u8 status = info->status; + const u16 sub_opcode = __le16_to_cpu(info->sub_opcode); + + skb_pull(skb, sizeof(*info)); + + if (status) { + BT_ERR("Microsoft vendor extension sub command 0x%2.2x failed", + sub_opcode); + return; + } + + BT_DBG("%s status 0x%2.2x sub opcode 0x%2.2x", hdev->name, status, + sub_opcode); + + switch (sub_opcode) { + case MSFT_OP_READ_SUPPORTED_FEATURES: { + struct msft_rp_read_supported_features *rp = (void *)skb->data; + + hdev->msft_vnd_ext_features = __le64_to_cpu(rp->features); + hdev->msft_vnd_ext_evt_prefix_len = rp->msft_evt_prefix_len; + hdev->msft_vnd_ext_evt_prefix = + kmalloc(hdev->msft_vnd_ext_evt_prefix_len, GFP_ATOMIC); + if (!hdev->msft_vnd_ext_evt_prefix) + return; + + memcpy(hdev->msft_vnd_ext_evt_prefix, rp->msft_evt_prefix, + hdev->msft_vnd_ext_evt_prefix_len); + BT_DBG("%s Microsoft vendor extension features 0x%016llx", + hdev->name, hdev->msft_vnd_ext_features); + break; + } + default: + BT_ERR("%s unknown sub opcode 0x%2.2x", hdev->name, + sub_opcode); + break; + } +} + static void hci_cs_inquiry(struct hci_dev *hdev, __u8 status) { BT_DBG("%s status 0x%2.2x", hdev->name, status); @@ -3198,6 +3238,7 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb, hci_req_complete_skb_t *req_complete_skb) { struct hci_ev_cmd_complete *ev = (void *) skb->data; + const u16 msft_vnd_ext_opcode = hdev->msft_vnd_ext_opcode; *opcode = __le16_to_cpu(ev->opcode); *status = skb->data[sizeof(*ev)]; @@ -3538,6 +3579,12 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb, break; default: + /* Check if it's a Microsoft vendor extension opcode. */ + if (msft_vnd_ext_opcode != HCI_OP_NOP && + *opcode == msft_vnd_ext_opcode) { + hci_cc_msft_vnd_ext(hdev, skb); + break; + } BT_DBG("%s opcode 0x%4.4x", hdev->name, *opcode); break; } -- 2.24.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v1 2/2] Bluetooth: btusb: Read the supported features of Microsoft vendor extension 2020-03-23 7:28 ` [PATCH v1 2/2] Bluetooth: btusb: Read the supported features of Microsoft vendor extension Miao-chen Chou @ 2020-03-23 17:30 ` kbuild test robot 2020-03-23 18:05 ` Marcel Holtmann 1 sibling, 0 replies; 17+ messages in thread From: kbuild test robot @ 2020-03-23 17:30 UTC (permalink / raw) To: Miao-chen Chou; +Cc: kbuild-all, Bluetooth Kernel Mailing List Hi Miao-chen, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on bluetooth-next/master] [also build test WARNING on next-20200323] [cannot apply to v5.6-rc7] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Miao-chen-Chou/btusb-Introduce-the-use-of-vendor-extension-s/20200323-165723 base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master reproduce: # apt-get install sparse # sparse version: v0.6.1-187-gbff9b106-dirty make ARCH=x86_64 allmodconfig make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> sparse warnings: (new ones prefixed by >>) >> net/bluetooth/hci_event.c:1793:32: sparse: sparse: cast to restricted __le16 >> net/bluetooth/hci_event.c:1810:47: sparse: sparse: cast to restricted __le64 arch/x86/include/asm/bitops.h:77:37: sparse: sparse: cast truncates bits from constant value (ffffff7f becomes 7f) arch/x86/include/asm/bitops.h:77:37: sparse: sparse: cast truncates bits from constant value (ffffff7f becomes 7f) arch/x86/include/asm/bitops.h:77:37: sparse: sparse: cast truncates bits from constant value (ffffff7f becomes 7f) arch/x86/include/asm/bitops.h:77:37: sparse: sparse: cast truncates bits from constant value (ffffff7f becomes 7f) arch/x86/include/asm/bitops.h:77:37: sparse: sparse: cast truncates bits from constant value (ffffff7f becomes 7f) vim +1793 net/bluetooth/hci_event.c 1788 1789 static void hci_cc_msft_vnd_ext(struct hci_dev *hdev, struct sk_buff *skb) 1790 { 1791 struct msft_cmd_cmp_info *info = (void *)skb->data; 1792 const u8 status = info->status; > 1793 const u16 sub_opcode = __le16_to_cpu(info->sub_opcode); 1794 1795 skb_pull(skb, sizeof(*info)); 1796 1797 if (status) { 1798 BT_ERR("Microsoft vendor extension sub command 0x%2.2x failed", 1799 sub_opcode); 1800 return; 1801 } 1802 1803 BT_DBG("%s status 0x%2.2x sub opcode 0x%2.2x", hdev->name, status, 1804 sub_opcode); 1805 1806 switch (sub_opcode) { 1807 case MSFT_OP_READ_SUPPORTED_FEATURES: { 1808 struct msft_rp_read_supported_features *rp = (void *)skb->data; 1809 > 1810 hdev->msft_vnd_ext_features = __le64_to_cpu(rp->features); 1811 hdev->msft_vnd_ext_evt_prefix_len = rp->msft_evt_prefix_len; 1812 hdev->msft_vnd_ext_evt_prefix = 1813 kmalloc(hdev->msft_vnd_ext_evt_prefix_len, GFP_ATOMIC); 1814 if (!hdev->msft_vnd_ext_evt_prefix) 1815 return; 1816 1817 memcpy(hdev->msft_vnd_ext_evt_prefix, rp->msft_evt_prefix, 1818 hdev->msft_vnd_ext_evt_prefix_len); 1819 BT_DBG("%s Microsoft vendor extension features 0x%016llx", 1820 hdev->name, hdev->msft_vnd_ext_features); 1821 break; 1822 } 1823 default: 1824 BT_ERR("%s unknown sub opcode 0x%2.2x", hdev->name, 1825 sub_opcode); 1826 break; 1827 } 1828 } 1829 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v1 2/2] Bluetooth: btusb: Read the supported features of Microsoft vendor extension 2020-03-23 7:28 ` [PATCH v1 2/2] Bluetooth: btusb: Read the supported features of Microsoft vendor extension Miao-chen Chou 2020-03-23 17:30 ` kbuild test robot @ 2020-03-23 18:05 ` Marcel Holtmann 1 sibling, 0 replies; 17+ messages in thread From: Marcel Holtmann @ 2020-03-23 18:05 UTC (permalink / raw) To: Miao-chen Chou Cc: Bluetooth Kernel Mailing List, Luiz Augusto von Dentz, Alain Michaud, David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel, netdev Hi Miao-chen, > This adds a new header to facilitate the opcode and packet structures of > vendor extension(s). For now, we add only the > HCI_VS_MSFT_Read_Supported_Features command from Microsoft vendor > extension. See https://docs.microsoft.com/en-us/windows-hardware/drivers/ > bluetooth/microsoft-defined-bluetooth-hci-commands-and-events# > microsoft-defined-bluetooth-hci-events for more details. > Upon initialization of a hci_dev, we issue a > HCI_VS_MSFT_Read_Supported_Features command to read the supported features > of Microsoft vendor extension if the opcode of Microsoft vendor extension > is valid. See https://docs.microsoft.com/en-us/windows-hardware/drivers/ > bluetooth/microsoft-defined-bluetooth-hci-commands-and-events# > hci_vs_msft_read_supported_features for more details. > This was verified on a device with Intel ThhunderPeak BT controller where > the Microsoft vendor extension features are 0x000000000000003f. > > Signed-off-by: Miao-chen Chou <mcchou@chromium.org> > --- > > Changes in v1: > - Add vendor_hci.h to facilitate opcodes and packet structures of vendor > extension(s). Put the structs directly into hci_msft.c and try to isolate all commands into that file. And prefix everything with msft_. > - Add opcode for the HCI_VS_MSFT_Read_Supported_Features command from > Microsoft vendor extension. > - Issue a HCI_VS_MSFT_Read_Supported_Features command upon > hci_dev_do_open and save the return values. > > include/net/bluetooth/hci_core.h | 1 + > include/net/bluetooth/vendor_hci.h | 51 ++++++++++++++++++++++++++++++ > net/bluetooth/hci_core.c | 30 ++++++++++++++++++ > net/bluetooth/hci_event.c | 49 +++++++++++++++++++++++++++- > 4 files changed, 130 insertions(+), 1 deletion(-) > create mode 100644 include/net/bluetooth/vendor_hci.h > > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h > index 15daf3b2d4f0..941e71bdad42 100644 > --- a/include/net/bluetooth/hci_core.h > +++ b/include/net/bluetooth/hci_core.h > @@ -30,6 +30,7 @@ > > #include <net/bluetooth/hci.h> > #include <net/bluetooth/hci_sock.h> > +#include <net/bluetooth/vendor_hci.h> > > /* HCI priority */ > #define HCI_PRIO_MAX 7 > diff --git a/include/net/bluetooth/vendor_hci.h b/include/net/bluetooth/vendor_hci.h > new file mode 100644 > index 000000000000..bd374a7bf976 > --- /dev/null > +++ b/include/net/bluetooth/vendor_hci.h > @@ -0,0 +1,51 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * BlueZ - Bluetooth protocol stack for Linux > + * Copyright (C) 2020 Google Corporation > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation; > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS > + * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT OF THIRD PARTY RIGHTS. > + * IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) AND AUTHOR(S) BE LIABLE FOR ANY > + * CLAIM, OR ANY SPECIAL INDIRECT OR CONSEQUENTIAL DAMAGES, OR ANY DAMAGES > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > + * > + * ALL LIABILITY, INCLUDING LIABILITY FOR INFRINGEMENT OF ANY PATENTS, > + * COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS > + * SOFTWARE IS DISCLAIMED. > + */ > + > +#ifndef __VENDOR_HCI_H > +#define __VENDOR_HCI_H > + > +#define MSFT_EVT_PREFIX_MAX_LEN 255 > + > +struct msft_cmd_cmp_info { > + __u8 status; > + __u8 sub_opcode; > +} __packed; > + > +/* Microsoft Vendor HCI subcommands */ > +#define MSFT_OP_READ_SUPPORTED_FEATURES 0x00 > +#define MSFT_FEATURE_MASK_RSSI_MONITOR_BREDR_CONN 0x0000000000000001 > +#define MSFT_FEATURE_MASK_RSSI_MONITOR_LE_CONN 0x0000000000000002 > +#define MSFT_FEATURE_MASK_RSSI_MONITOR_LE_ADV 0x0000000000000004 > +#define MSFT_FEATURE_MASK_ADV_MONITOR_LE_ADV 0x0000000000000008 > +#define MSFT_FEATURE_MASK_VERIFY_CURVE_ 0x0000000000000010 > +#define MSFT_FEATURE_MASK_CONCURRENT_ADV_MONITOR 0x0000000000000020 > +struct msft_cp_read_supported_features { > + __u8 sub_opcode; > +} __packed; > +struct msft_rp_read_supported_features { > + __u64 features; > + __u8 msft_evt_prefix_len; > + __u8 msft_evt_prefix[0]; > +} __packed; > + > +#endif /* __VENDOR_HCI_H */ > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index dbd2ad3a26ed..3cec58ca0047 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -1407,6 +1407,32 @@ static void hci_dev_get_bd_addr_from_property(struct hci_dev *hdev) > bacpy(&hdev->public_addr, &ba); > } > > +static int hci_msft_read_supported_features_req(struct hci_request *req, > + unsigned long opt) > +{ > + struct hci_dev *hdev = req->hdev; > + struct msft_cp_read_supported_features cp; > + > + if (hdev->msft_vnd_ext_opcode == HCI_OP_NOP) > + return -EOPNOTSUPP; > + > + cp.sub_opcode = MSFT_OP_READ_SUPPORTED_FEATURES; > + hci_req_add(req, hdev->msft_vnd_ext_opcode, sizeof(cp), &cp); > + return 0; > +} > + > +static void read_vendor_extension_features(struct hci_dev *hdev) > +{ > + int err; > + > + if (hdev->msft_vnd_ext_opcode != HCI_OP_NOP) { > + err = __hci_req_sync(hdev, hci_msft_read_supported_features_req, > + 0, HCI_CMD_TIMEOUT, NULL); > + if (err) > + BT_ERR("HCI_VS_MSFT_Read_Supported_Feature failed"); > + } > +} > + > static int hci_dev_do_open(struct hci_dev *hdev) > { > int ret = 0; > @@ -1572,6 +1598,10 @@ static int hci_dev_do_open(struct hci_dev *hdev) > set_bit(HCI_UP, &hdev->flags); > hci_sock_dev_event(hdev, HCI_DEV_UP); > hci_leds_update_powered(hdev, true); > + > + /* Check features supported by HCI extensions. */ > + read_vendor_extension_features(hdev); > + Lets not do this after the HCI_DEV_UP event. You need to do this after the init sequence has executed. > if (!hci_dev_test_flag(hdev, HCI_SETUP) && > !hci_dev_test_flag(hdev, HCI_CONFIG) && > !hci_dev_test_flag(hdev, HCI_UNCONFIGURED) && > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index 20408d386268..6a156167dfc9 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -23,7 +23,6 @@ > */ > > /* Bluetooth HCI event handling. */ > - This change doesn’t belong here. > #include <asm/unaligned.h> > > #include <net/bluetooth/bluetooth.h> > @@ -1787,6 +1786,47 @@ static void hci_cc_write_ssp_debug_mode(struct hci_dev *hdev, struct sk_buff *sk > hdev->ssp_debug_mode = *mode; > } > > +static void hci_cc_msft_vnd_ext(struct hci_dev *hdev, struct sk_buff *skb) > +{ > + struct msft_cmd_cmp_info *info = (void *)skb->data; > + const u8 status = info->status; > + const u16 sub_opcode = __le16_to_cpu(info->sub_opcode); > + > + skb_pull(skb, sizeof(*info)); > + > + if (status) { > + BT_ERR("Microsoft vendor extension sub command 0x%2.2x failed", > + sub_opcode); > + return; > + } > + > + BT_DBG("%s status 0x%2.2x sub opcode 0x%2.2x", hdev->name, status, > + sub_opcode); > + > + switch (sub_opcode) { > + case MSFT_OP_READ_SUPPORTED_FEATURES: { > + struct msft_rp_read_supported_features *rp = (void *)skb->data; > + > + hdev->msft_vnd_ext_features = __le64_to_cpu(rp->features); > + hdev->msft_vnd_ext_evt_prefix_len = rp->msft_evt_prefix_len; > + hdev->msft_vnd_ext_evt_prefix = > + kmalloc(hdev->msft_vnd_ext_evt_prefix_len, GFP_ATOMIC); > + if (!hdev->msft_vnd_ext_evt_prefix) > + return; > + > + memcpy(hdev->msft_vnd_ext_evt_prefix, rp->msft_evt_prefix, > + hdev->msft_vnd_ext_evt_prefix_len); > + BT_DBG("%s Microsoft vendor extension features 0x%016llx", > + hdev->name, hdev->msft_vnd_ext_features); > + break; > + } > + default: > + BT_ERR("%s unknown sub opcode 0x%2.2x", hdev->name, > + sub_opcode); > + break; > + } > +} > + > static void hci_cs_inquiry(struct hci_dev *hdev, __u8 status) > { > BT_DBG("%s status 0x%2.2x", hdev->name, status); > @@ -3198,6 +3238,7 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb, > hci_req_complete_skb_t *req_complete_skb) > { > struct hci_ev_cmd_complete *ev = (void *) skb->data; > + const u16 msft_vnd_ext_opcode = hdev->msft_vnd_ext_opcode; > > *opcode = __le16_to_cpu(ev->opcode); > *status = skb->data[sizeof(*ev)]; > @@ -3538,6 +3579,12 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb, > break; > > default: > + /* Check if it's a Microsoft vendor extension opcode. */ > + if (msft_vnd_ext_opcode != HCI_OP_NOP && > + *opcode == msft_vnd_ext_opcode) { > + hci_cc_msft_vnd_ext(hdev, skb); > + break; > + } > BT_DBG("%s opcode 0x%4.4x", hdev->name, *opcode); > break; > } For now we only have synchronous commands. The features reading can be done with sync call. Regards Marcel ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-03-25 8:02 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-23 7:28 [PATCH v1 0/2] btusb: Introduce the use of vendor extension(s) Miao-chen Chou 2020-03-23 7:28 ` [PATCH v1 1/2] Bluetooth: btusb: Indicate Microsoft vendor extension for Intel 9460/9560 and 9160/9260 Miao-chen Chou 2020-03-23 17:56 ` Marcel Holtmann 2020-03-23 18:45 ` Joe Perches 2020-03-23 18:48 ` Marcel Holtmann 2020-03-23 20:09 ` Joe Perches 2020-03-24 15:10 ` Alain Michaud 2020-03-24 15:17 ` Joe Perches 2020-03-24 15:24 ` Alain Michaud 2020-03-24 16:45 ` Joe Perches 2020-03-24 18:35 ` Marcel Holtmann 2020-03-24 19:32 ` Alain Michaud 2020-03-25 5:18 ` Miao-chen Chou 2020-03-25 8:02 ` Marcel Holtmann 2020-03-23 7:28 ` [PATCH v1 2/2] Bluetooth: btusb: Read the supported features of Microsoft vendor extension Miao-chen Chou 2020-03-23 17:30 ` kbuild test robot 2020-03-23 18:05 ` Marcel Holtmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).