[PATCH v2 0/2] btusb: Introduce the use of vendor extension(s)

[PATCH v2 0/2] btusb: Introduce the use of vendor extension(s)

[Miao-chen Chou]

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 v2:
- Define struct msft_vnd_ext and add a field of this type to struct
hci_dev to facilitate the support of Microsoft vendor extension.
- Issue a HCI_VS_MSFT_Read_Supported_Features command with
__hci_cmd_sync() instead of constructing a request.

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          | 17 ++++++-
 include/net/bluetooth/hci_core.h   | 10 ++++
 include/net/bluetooth/vendor_hci.h | 51 +++++++++++++++++++
 net/bluetooth/hci_core.c           | 78 ++++++++++++++++++++++++++++++
 4 files changed, 154 insertions(+), 2 deletions(-)
 create mode 100644 include/net/bluetooth/vendor_hci.h

-- 
2.24.1

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

[Miao-chen Chou]

This adds a bit mask of driver_info for Microsoft vendor extension and
indicates the support for Intel 9460/9560 and 9160/9260. See
https://docs.microsoft.com/en-us/windows-hardware/drivers/bluetooth/
microsoft-defined-bluetooth-hci-commands-and-events for more information
about the extension. This was verified with Intel ThunderPeak BT controller
where msft_vnd_ext_opcode is 0xFC1E.

Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
---

Changes in v2:
- Define struct msft_vnd_ext and add a field of this type to struct
hci_dev to facilitate the support of Microsoft vendor extension.

 drivers/bluetooth/btusb.c        | 14 ++++++++++++--
 include/net/bluetooth/hci_core.h |  6 ++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 3bdec42c9612..4c49f394f174 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -58,6 +58,7 @@ static struct usb_driver btusb_driver;
 #define BTUSB_CW6622		0x100000
 #define BTUSB_MEDIATEK		0x200000
 #define BTUSB_WIDEBAND_SPEECH	0x400000
+#define BTUSB_MSFT_VND_EXT	0x800000
 
 static const struct usb_device_id btusb_table[] = {
 	/* Generic Bluetooth USB device */
@@ -335,7 +336,8 @@ static const struct usb_device_id blacklist_table[] = {
 
 	/* Intel Bluetooth devices */
 	{ USB_DEVICE(0x8087, 0x0025), .driver_info = BTUSB_INTEL_NEW |
-						     BTUSB_WIDEBAND_SPEECH },
+						     BTUSB_WIDEBAND_SPEECH |
+						     BTUSB_MSFT_VND_EXT },
 	{ USB_DEVICE(0x8087, 0x0026), .driver_info = BTUSB_INTEL_NEW |
 						     BTUSB_WIDEBAND_SPEECH },
 	{ USB_DEVICE(0x8087, 0x0029), .driver_info = BTUSB_INTEL_NEW |
@@ -348,7 +350,8 @@ static const struct usb_device_id blacklist_table[] = {
 	{ USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL |
 						     BTUSB_WIDEBAND_SPEECH },
 	{ USB_DEVICE(0x8087, 0x0aaa), .driver_info = BTUSB_INTEL_NEW |
-						     BTUSB_WIDEBAND_SPEECH },
+						     BTUSB_WIDEBAND_SPEECH |
+						     BTUSB_MSFT_VND_EXT },
 
 	/* Other Intel Bluetooth devices */
 	{ USB_VENDOR_AND_INTERFACE_INFO(0x8087, 0xe0, 0x01, 0x01),
@@ -3734,6 +3737,8 @@ static int btusb_probe(struct usb_interface *intf,
 	hdev->send   = btusb_send_frame;
 	hdev->notify = btusb_notify;
 
+	hdev->msft_ext.opcode = HCI_OP_NOP;
+
 #ifdef CONFIG_PM
 	err = btusb_config_oob_wake(hdev);
 	if (err)
@@ -3800,6 +3805,11 @@ static int btusb_probe(struct usb_interface *intf,
 		set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
 		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
 		set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
+
+		if (id->driver_info & BTUSB_MSFT_VND_EXT &&
+			(id->idProduct == 0x0025 || id->idProduct == 0x0aaa)) {
+			hdev->msft_ext.opcode = 0xFC1E;
+		}
 	}
 
 	if (id->driver_info & BTUSB_MARVELL)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index d4e28773d378..0ec3d9b41d81 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -244,6 +244,10 @@ struct amp_assoc {
 
 #define HCI_MAX_PAGES	3
 
+struct msft_vnd_ext {
+	__u16	opcode;
+};
+
 struct hci_dev {
 	struct list_head list;
 	struct mutex	lock;
@@ -343,6 +347,8 @@ struct hci_dev {
 
 	struct amp_assoc	loc_assoc;
 
+	struct msft_vnd_ext	msft_ext;
+
 	__u8		flow_ctl_mode;
 
 	unsigned int	auto_accept_delay;
-- 
2.24.1

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

[Marcel Holtmann]

Hi Miao-chen,

> This adds a bit mask of driver_info for Microsoft vendor extension and
> indicates the support for Intel 9460/9560 and 9160/9260. See
> https://docs.microsoft.com/en-us/windows-hardware/drivers/bluetooth/
> microsoft-defined-bluetooth-hci-commands-and-events for more information
> about the extension. This was verified with Intel ThunderPeak BT controller
> where msft_vnd_ext_opcode is 0xFC1E.
> 
> Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
> ---
> 
> Changes in v2:
> - Define struct msft_vnd_ext and add a field of this type to struct
> hci_dev to facilitate the support of Microsoft vendor extension.
> 
> drivers/bluetooth/btusb.c        | 14 ++++++++++++--
> include/net/bluetooth/hci_core.h |  6 ++++++
> 2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 3bdec42c9612..4c49f394f174 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -58,6 +58,7 @@ static struct usb_driver btusb_driver;
> #define BTUSB_CW6622		0x100000
> #define BTUSB_MEDIATEK		0x200000
> #define BTUSB_WIDEBAND_SPEECH	0x400000
> +#define BTUSB_MSFT_VND_EXT	0x800000
> 
> static const struct usb_device_id btusb_table[] = {
> 	/* Generic Bluetooth USB device */
> @@ -335,7 +336,8 @@ static const struct usb_device_id blacklist_table[] = {
> 
> 	/* Intel Bluetooth devices */
> 	{ USB_DEVICE(0x8087, 0x0025), .driver_info = BTUSB_INTEL_NEW |
> -						     BTUSB_WIDEBAND_SPEECH },
> +						     BTUSB_WIDEBAND_SPEECH |
> +						     BTUSB_MSFT_VND_EXT },
> 	{ USB_DEVICE(0x8087, 0x0026), .driver_info = BTUSB_INTEL_NEW |
> 						     BTUSB_WIDEBAND_SPEECH },
> 	{ USB_DEVICE(0x8087, 0x0029), .driver_info = BTUSB_INTEL_NEW |
> @@ -348,7 +350,8 @@ static const struct usb_device_id blacklist_table[] = {
> 	{ USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL |
> 						     BTUSB_WIDEBAND_SPEECH },
> 	{ USB_DEVICE(0x8087, 0x0aaa), .driver_info = BTUSB_INTEL_NEW |
> -						     BTUSB_WIDEBAND_SPEECH },
> +						     BTUSB_WIDEBAND_SPEECH |
> +						     BTUSB_MSFT_VND_EXT },
> 
> 	/* Other Intel Bluetooth devices */
> 	{ USB_VENDOR_AND_INTERFACE_INFO(0x8087, 0xe0, 0x01, 0x01),
> @@ -3734,6 +3737,8 @@ static int btusb_probe(struct usb_interface *intf,
> 	hdev->send   = btusb_send_frame;
> 	hdev->notify = btusb_notify;
> 
> +	hdev->msft_ext.opcode = HCI_OP_NOP;
> +

do this in the hci_alloc_dev procedure for every driver. This doesn’t belong in the driver.

> #ifdef CONFIG_PM
> 	err = btusb_config_oob_wake(hdev);
> 	if (err)
> @@ -3800,6 +3805,11 @@ static int btusb_probe(struct usb_interface *intf,
> 		set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
> 		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
> 		set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
> +
> +		if (id->driver_info & BTUSB_MSFT_VND_EXT &&
> +			(id->idProduct == 0x0025 || id->idProduct == 0x0aaa)) {

Please scrap this extra check. You already selected out the PID with the blacklist_table. In addition, I do not want to add a PID in two places in the driver.

An alternative is to not use BTUSB_MSFT_VND_EXT and let the Intel code set it based on the hardware / firmware revision it finds. We might need to discuss which is the better approach for the Intel hardware since not all PIDs are unique.

> +			hdev->msft_ext.opcode = 0xFC1E;
> +		}
> 	}
> 
> 	if (id->driver_info & BTUSB_MARVELL)
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index d4e28773d378..0ec3d9b41d81 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -244,6 +244,10 @@ struct amp_assoc {
> 
> #define HCI_MAX_PAGES	3
> 
> +struct msft_vnd_ext {
> +	__u16	opcode;
> +};
> +
> struct hci_dev {
> 	struct list_head list;
> 	struct mutex	lock;
> @@ -343,6 +347,8 @@ struct hci_dev {
> 
> 	struct amp_assoc	loc_assoc;
> 
> +	struct msft_vnd_ext	msft_ext;
> +
> 	__u8		flow_ctl_mode;
> 
> 	unsigned int	auto_accept_delay;

Regards

Marcel

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

[Miao-chen Chou]

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

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

[Marcel Holtmann]

Hi Miao-chen,

>>> This adds a bit mask of driver_info for Microsoft vendor extension and
>>> indicates the support for Intel 9460/9560 and 9160/9260. See
>>> https://docs.microsoft.com/en-us/windows-hardware/drivers/bluetooth/
>>> microsoft-defined-bluetooth-hci-commands-and-events for more information
>>> about the extension. This was verified with Intel ThunderPeak BT controller
>>> where msft_vnd_ext_opcode is 0xFC1E.
>>> 
>>> Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
>>> ---
>>> 
>>> Changes in v2:
>>> - Define struct msft_vnd_ext and add a field of this type to struct
>>> hci_dev to facilitate the support of Microsoft vendor extension.
>>> 
>>> drivers/bluetooth/btusb.c        | 14 ++++++++++++--
>>> include/net/bluetooth/hci_core.h |  6 ++++++
>>> 2 files changed, 18 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 3bdec42c9612..4c49f394f174 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -58,6 +58,7 @@ static struct usb_driver btusb_driver;
>>> #define BTUSB_CW6622          0x100000
>>> #define BTUSB_MEDIATEK                0x200000
>>> #define BTUSB_WIDEBAND_SPEECH 0x400000
>>> +#define BTUSB_MSFT_VND_EXT   0x800000
>>> 
>>> static const struct usb_device_id btusb_table[] = {
>>>      /* Generic Bluetooth USB device */
>>> @@ -335,7 +336,8 @@ static const struct usb_device_id blacklist_table[] = {
>>> 
>>>      /* Intel Bluetooth devices */
>>>      { USB_DEVICE(0x8087, 0x0025), .driver_info = BTUSB_INTEL_NEW |
>>> -                                                  BTUSB_WIDEBAND_SPEECH },
>>> +                                                  BTUSB_WIDEBAND_SPEECH |
>>> +                                                  BTUSB_MSFT_VND_EXT },
>>>      { USB_DEVICE(0x8087, 0x0026), .driver_info = BTUSB_INTEL_NEW |
>>>                                                   BTUSB_WIDEBAND_SPEECH },
>>>      { USB_DEVICE(0x8087, 0x0029), .driver_info = BTUSB_INTEL_NEW |
>>> @@ -348,7 +350,8 @@ static const struct usb_device_id blacklist_table[] = {
>>>      { USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL |
>>>                                                   BTUSB_WIDEBAND_SPEECH },
>>>      { USB_DEVICE(0x8087, 0x0aaa), .driver_info = BTUSB_INTEL_NEW |
>>> -                                                  BTUSB_WIDEBAND_SPEECH },
>>> +                                                  BTUSB_WIDEBAND_SPEECH |
>>> +                                                  BTUSB_MSFT_VND_EXT },
>>> 
>>>      /* Other Intel Bluetooth devices */
>>>      { USB_VENDOR_AND_INTERFACE_INFO(0x8087, 0xe0, 0x01, 0x01),
>>> @@ -3734,6 +3737,8 @@ static int btusb_probe(struct usb_interface *intf,
>>>      hdev->send   = btusb_send_frame;
>>>      hdev->notify = btusb_notify;
>>> 
>>> +     hdev->msft_ext.opcode = HCI_OP_NOP;
>>> +
>> 
>> do this in the hci_alloc_dev procedure for every driver. This doesn’t belong in the driver.
> Thanks for the note, I will address this.
>> 
>>> #ifdef CONFIG_PM
>>>      err = btusb_config_oob_wake(hdev);
>>>      if (err)
>>> @@ -3800,6 +3805,11 @@ static int btusb_probe(struct usb_interface *intf,
>>>              set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
>>>              set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>>>              set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
>>> +
>>> +             if (id->driver_info & BTUSB_MSFT_VND_EXT &&
>>> +                     (id->idProduct == 0x0025 || id->idProduct == 0x0aaa)) {
>> 
>> Please scrap this extra check. You already selected out the PID with the blacklist_table. In addition, I do not want to add a PID in two places in the driver.
> If we scrap the check around idProduct, how do we tell two controllers
> apart if they use different opcode for Microsoft vendor extension?

for Intel controllers this is highly unlikely. If we really decide to change the opcode in newer firmware versions, we then deal with it at that point.

However for Intel controllers I have the feeling that we better do it after the Read the Intel version information and then do it based on hardware revision and firmware version.

>> An alternative is to not use BTUSB_MSFT_VND_EXT and let the Intel code set it based on the hardware / firmware revision it finds. We might need to discuss which is the better approach for the Intel hardware since not all PIDs are unique.
> We are expecting to indicate the vendor extension for non-Intel
> controllers as well, and having BTUSB_MSFT_VND_EXT seems to be more
> generic. What do you think?

We don’t have to have one specific way of doing it. As I said, if we ever have Zephyr based controller with MSFT extension, we have a vendor command to determine the support and the opcode. So that will not require any extra quirks or alike.

Anyhow, maybe we introduce BTUSB_MSFT_VND_EXT_FC1E that just says set the opcode to FC1E. For all other opcodes we will introduce similar constants. At most I assume we end up with 5-6 constants.

>> 
>>> +                     hdev->msft_ext.opcode = 0xFC1E;
>>> +             }
>>>      }

Regards

Marcel

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

[Miao-chen Chou]

On Wed, Mar 25, 2020 at 2:37 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Miao-chen,
>
> >>> This adds a bit mask of driver_info for Microsoft vendor extension and
> >>> indicates the support for Intel 9460/9560 and 9160/9260. See
> >>> https://docs.microsoft.com/en-us/windows-hardware/drivers/bluetooth/
> >>> microsoft-defined-bluetooth-hci-commands-and-events for more information
> >>> about the extension. This was verified with Intel ThunderPeak BT controller
> >>> where msft_vnd_ext_opcode is 0xFC1E.
> >>>
> >>> Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
> >>> ---
> >>>
> >>> Changes in v2:
> >>> - Define struct msft_vnd_ext and add a field of this type to struct
> >>> hci_dev to facilitate the support of Microsoft vendor extension.
> >>>
> >>> drivers/bluetooth/btusb.c        | 14 ++++++++++++--
> >>> include/net/bluetooth/hci_core.h |  6 ++++++
> >>> 2 files changed, 18 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> >>> index 3bdec42c9612..4c49f394f174 100644
> >>> --- a/drivers/bluetooth/btusb.c
> >>> +++ b/drivers/bluetooth/btusb.c
> >>> @@ -58,6 +58,7 @@ static struct usb_driver btusb_driver;
> >>> #define BTUSB_CW6622          0x100000
> >>> #define BTUSB_MEDIATEK                0x200000
> >>> #define BTUSB_WIDEBAND_SPEECH 0x400000
> >>> +#define BTUSB_MSFT_VND_EXT   0x800000
> >>>
> >>> static const struct usb_device_id btusb_table[] = {
> >>>      /* Generic Bluetooth USB device */
> >>> @@ -335,7 +336,8 @@ static const struct usb_device_id blacklist_table[] = {
> >>>
> >>>      /* Intel Bluetooth devices */
> >>>      { USB_DEVICE(0x8087, 0x0025), .driver_info = BTUSB_INTEL_NEW |
> >>> -                                                  BTUSB_WIDEBAND_SPEECH },
> >>> +                                                  BTUSB_WIDEBAND_SPEECH |
> >>> +                                                  BTUSB_MSFT_VND_EXT },
> >>>      { USB_DEVICE(0x8087, 0x0026), .driver_info = BTUSB_INTEL_NEW |
> >>>                                                   BTUSB_WIDEBAND_SPEECH },
> >>>      { USB_DEVICE(0x8087, 0x0029), .driver_info = BTUSB_INTEL_NEW |
> >>> @@ -348,7 +350,8 @@ static const struct usb_device_id blacklist_table[] = {
> >>>      { USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL |
> >>>                                                   BTUSB_WIDEBAND_SPEECH },
> >>>      { USB_DEVICE(0x8087, 0x0aaa), .driver_info = BTUSB_INTEL_NEW |
> >>> -                                                  BTUSB_WIDEBAND_SPEECH },
> >>> +                                                  BTUSB_WIDEBAND_SPEECH |
> >>> +                                                  BTUSB_MSFT_VND_EXT },
> >>>
> >>>      /* Other Intel Bluetooth devices */
> >>>      { USB_VENDOR_AND_INTERFACE_INFO(0x8087, 0xe0, 0x01, 0x01),
> >>> @@ -3734,6 +3737,8 @@ static int btusb_probe(struct usb_interface *intf,
> >>>      hdev->send   = btusb_send_frame;
> >>>      hdev->notify = btusb_notify;
> >>>
> >>> +     hdev->msft_ext.opcode = HCI_OP_NOP;
> >>> +
> >>
> >> do this in the hci_alloc_dev procedure for every driver. This doesn’t belong in the driver.
> > Thanks for the note, I will address this.
> >>
> >>> #ifdef CONFIG_PM
> >>>      err = btusb_config_oob_wake(hdev);
> >>>      if (err)
> >>> @@ -3800,6 +3805,11 @@ static int btusb_probe(struct usb_interface *intf,
> >>>              set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
> >>>              set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
> >>>              set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
> >>> +
> >>> +             if (id->driver_info & BTUSB_MSFT_VND_EXT &&
> >>> +                     (id->idProduct == 0x0025 || id->idProduct == 0x0aaa)) {
> >>
> >> Please scrap this extra check. You already selected out the PID with the blacklist_table. In addition, I do not want to add a PID in two places in the driver.
> > If we scrap the check around idProduct, how do we tell two controllers
> > apart if they use different opcode for Microsoft vendor extension?
>
> for Intel controllers this is highly unlikely. If we really decide to change the opcode in newer firmware versions, we then deal with it at that point.
>
> However for Intel controllers I have the feeling that we better do it after the Read the Intel version information and then do it based on hardware revision and firmware version.
I would agree with you given that the FW loaded for the same HW can
differ, and different FW version may have different configuration in
terms of the use of extensions. But it's not clear to me how we can
tell whether an extension is supported based on a version number. Is
there any implication on the support of an extension given a FW
version (e.g. any FW version greater than 10 would support MSFT
extension)?
>
> >> An alternative is to not use BTUSB_MSFT_VND_EXT and let the Intel code set it based on the hardware / firmware revision it finds. We might need to discuss which is the better approach for the Intel hardware since not all PIDs are unique.
> > We are expecting to indicate the vendor extension for non-Intel
> > controllers as well, and having BTUSB_MSFT_VND_EXT seems to be more
> > generic. What do you think?
>
> We don’t have to have one specific way of doing it. As I said, if we ever have Zephyr based controller with MSFT extension, we have a vendor command to determine the support and the opcode. So that will not require any extra quirks or alike.
>
> Anyhow, maybe we introduce BTUSB_MSFT_VND_EXT_FC1E that just says set the opcode to FC1E. For all other opcodes we will introduce similar constants. At most I assume we end up with 5-6 constants.
>
> >>
> >>> +                     hdev->msft_ext.opcode = 0xFC1E;
> >>> +             }
> >>>      }
Regards,
Miao

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

[Marcel Holtmann]

Hi Miao-chen,

>>>>> This adds a bit mask of driver_info for Microsoft vendor extension and
>>>>> indicates the support for Intel 9460/9560 and 9160/9260. See
>>>>> https://docs.microsoft.com/en-us/windows-hardware/drivers/bluetooth/
>>>>> microsoft-defined-bluetooth-hci-commands-and-events for more information
>>>>> about the extension. This was verified with Intel ThunderPeak BT controller
>>>>> where msft_vnd_ext_opcode is 0xFC1E.
>>>>> 
>>>>> Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
>>>>> ---
>>>>> 
>>>>> Changes in v2:
>>>>> - Define struct msft_vnd_ext and add a field of this type to struct
>>>>> hci_dev to facilitate the support of Microsoft vendor extension.
>>>>> 
>>>>> drivers/bluetooth/btusb.c        | 14 ++++++++++++--
>>>>> include/net/bluetooth/hci_core.h |  6 ++++++
>>>>> 2 files changed, 18 insertions(+), 2 deletions(-)
>>>>> 
>>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>>> index 3bdec42c9612..4c49f394f174 100644
>>>>> --- a/drivers/bluetooth/btusb.c
>>>>> +++ b/drivers/bluetooth/btusb.c
>>>>> @@ -58,6 +58,7 @@ static struct usb_driver btusb_driver;
>>>>> #define BTUSB_CW6622          0x100000
>>>>> #define BTUSB_MEDIATEK                0x200000
>>>>> #define BTUSB_WIDEBAND_SPEECH 0x400000
>>>>> +#define BTUSB_MSFT_VND_EXT   0x800000
>>>>> 
>>>>> static const struct usb_device_id btusb_table[] = {
>>>>>     /* Generic Bluetooth USB device */
>>>>> @@ -335,7 +336,8 @@ static const struct usb_device_id blacklist_table[] = {
>>>>> 
>>>>>     /* Intel Bluetooth devices */
>>>>>     { USB_DEVICE(0x8087, 0x0025), .driver_info = BTUSB_INTEL_NEW |
>>>>> -                                                  BTUSB_WIDEBAND_SPEECH },
>>>>> +                                                  BTUSB_WIDEBAND_SPEECH |
>>>>> +                                                  BTUSB_MSFT_VND_EXT },
>>>>>     { USB_DEVICE(0x8087, 0x0026), .driver_info = BTUSB_INTEL_NEW |
>>>>>                                                  BTUSB_WIDEBAND_SPEECH },
>>>>>     { USB_DEVICE(0x8087, 0x0029), .driver_info = BTUSB_INTEL_NEW |
>>>>> @@ -348,7 +350,8 @@ static const struct usb_device_id blacklist_table[] = {
>>>>>     { USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL |
>>>>>                                                  BTUSB_WIDEBAND_SPEECH },
>>>>>     { USB_DEVICE(0x8087, 0x0aaa), .driver_info = BTUSB_INTEL_NEW |
>>>>> -                                                  BTUSB_WIDEBAND_SPEECH },
>>>>> +                                                  BTUSB_WIDEBAND_SPEECH |
>>>>> +                                                  BTUSB_MSFT_VND_EXT },
>>>>> 
>>>>>     /* Other Intel Bluetooth devices */
>>>>>     { USB_VENDOR_AND_INTERFACE_INFO(0x8087, 0xe0, 0x01, 0x01),
>>>>> @@ -3734,6 +3737,8 @@ static int btusb_probe(struct usb_interface *intf,
>>>>>     hdev->send   = btusb_send_frame;
>>>>>     hdev->notify = btusb_notify;
>>>>> 
>>>>> +     hdev->msft_ext.opcode = HCI_OP_NOP;
>>>>> +
>>>> 
>>>> do this in the hci_alloc_dev procedure for every driver. This doesn’t belong in the driver.
>>> Thanks for the note, I will address this.
>>>> 
>>>>> #ifdef CONFIG_PM
>>>>>     err = btusb_config_oob_wake(hdev);
>>>>>     if (err)
>>>>> @@ -3800,6 +3805,11 @@ static int btusb_probe(struct usb_interface *intf,
>>>>>             set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
>>>>>             set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
>>>>>             set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
>>>>> +
>>>>> +             if (id->driver_info & BTUSB_MSFT_VND_EXT &&
>>>>> +                     (id->idProduct == 0x0025 || id->idProduct == 0x0aaa)) {
>>>> 
>>>> Please scrap this extra check. You already selected out the PID with the blacklist_table. In addition, I do not want to add a PID in two places in the driver.
>>> If we scrap the check around idProduct, how do we tell two controllers
>>> apart if they use different opcode for Microsoft vendor extension?
>> 
>> for Intel controllers this is highly unlikely. If we really decide to change the opcode in newer firmware versions, we then deal with it at that point.
>> 
>> However for Intel controllers I have the feeling that we better do it after the Read the Intel version information and then do it based on hardware revision and firmware version.
> I would agree with you given that the FW loaded for the same HW can
> differ, and different FW version may have different configuration in
> terms of the use of extensions. But it's not clear to me how we can
> tell whether an extension is supported based on a version number. Is
> there any implication on the support of an extension given a FW
> version (e.g. any FW version greater than 10 would support MSFT
> extension)?

that is for us to figure out. I will get back to you on that.

Regards

Marcel

[PATCH v2 2/2] Bluetooth: btusb: Read the supported features of Microsoft vendor extension

[Miao-chen Chou]

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 v2:
- Issue a HCI_VS_MSFT_Read_Supported_Features command with
__hci_cmd_sync() instead of constructing a request.

 drivers/bluetooth/btusb.c          |  3 ++
 include/net/bluetooth/hci_core.h   |  4 ++
 include/net/bluetooth/vendor_hci.h | 51 +++++++++++++++++++
 net/bluetooth/hci_core.c           | 78 ++++++++++++++++++++++++++++++
 4 files changed, 136 insertions(+)
 create mode 100644 include/net/bluetooth/vendor_hci.h

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 4c49f394f174..410d50dbd4e2 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3738,6 +3738,9 @@ static int btusb_probe(struct usb_interface *intf,
 	hdev->notify = btusb_notify;
 
 	hdev->msft_ext.opcode = HCI_OP_NOP;
+	hdev->msft_ext.features = 0;
+	hdev->msft_ext.evt_prefix_len = 0;
+	hdev->msft_ext.evt_prefix = NULL;
 
 #ifdef CONFIG_PM
 	err = btusb_config_oob_wake(hdev);
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 0ec3d9b41d81..f2876c5067a4 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
@@ -246,6 +247,9 @@ struct amp_assoc {
 
 struct msft_vnd_ext {
 	__u16	opcode;
+	__u64	features;
+	__u8	evt_prefix_len;
+	void	*evt_prefix;
 };
 
 struct hci_dev {
diff --git a/include/net/bluetooth/vendor_hci.h b/include/net/bluetooth/vendor_hci.h
new file mode 100644
index 000000000000..89a6795e672c
--- /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  evt_prefix_len;
+	__u8  evt_prefix[0];
+} __packed;
+
+#endif /* __VENDOR_HCI_H */
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index dbd2ad3a26ed..1ea32d10ed08 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1407,6 +1407,76 @@ static void hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
 	bacpy(&hdev->public_addr, &ba);
 }
 
+static void process_msft_vnd_ext_cmd_complete(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 (IS_ERR(skb)) {
+		BT_WARN("%s: Microsoft extension response packet invalid",
+			hdev->name);
+		return;
+	}
+
+	if (status) {
+		BT_WARN("%s: Microsoft extension sub command 0x%2.2x failed",
+			hdev->name, 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;
+		u8 prefix_len = rp->evt_prefix_len;
+
+		hdev->msft_ext.features = __le64_to_cpu(rp->features);
+		hdev->msft_ext.evt_prefix_len = prefix_len;
+		hdev->msft_ext.evt_prefix = kmalloc(prefix_len, GFP_ATOMIC);
+		if (!hdev->msft_ext.evt_prefix) {
+			BT_WARN("%s: Microsoft extension invalid event prefix",
+				hdev->name);
+			return;
+		}
+
+		memcpy(hdev->msft_ext.evt_prefix, rp->evt_prefix, prefix_len);
+		BT_INFO("%s: Microsoft extension features 0x%016llx",
+			hdev->name, hdev->msft_ext.features);
+		break;
+	}
+	default:
+		BT_WARN("%s: Microsoft extension unknown sub opcode 0x%2.2x",
+			hdev->name, sub_opcode);
+		break;
+	}
+}
+
+static void read_vendor_extension_features(struct hci_dev *hdev)
+{
+	struct sk_buff *skb;
+	const u16 msft_opcode = hdev->msft_ext.opcode;
+
+	if (msft_opcode !=  HCI_OP_NOP) {
+		struct msft_cp_read_supported_features cp;
+
+		cp.sub_opcode = MSFT_OP_READ_SUPPORTED_FEATURES;
+		skb = __hci_cmd_sync(hdev, msft_opcode, sizeof(cp), &cp,
+				     HCI_CMD_TIMEOUT);
+
+		process_msft_vnd_ext_cmd_complete(hdev, skb);
+		if (skb) {
+			kfree_skb(skb);
+			skb = NULL;
+		}
+	}
+}
+
 static int hci_dev_do_open(struct hci_dev *hdev)
 {
 	int ret = 0;
@@ -1554,6 +1624,11 @@ static int hci_dev_do_open(struct hci_dev *hdev)
 		}
 	}
 
+	/* Check features supported by HCI extensions after the init procedure
+	 * completed.
+	 */
+	read_vendor_extension_features(hdev);
+
 	/* If the HCI Reset command is clearing all diagnostic settings,
 	 * then they need to be reprogrammed after the init procedure
 	 * completed.
@@ -1733,6 +1808,9 @@ int hci_dev_do_close(struct hci_dev *hdev)
 			cancel_delayed_work_sync(&adv_instance->rpa_expired_cb);
 	}
 
+	kfree(hdev->msft_ext.evt_prefix);
+	hdev->msft_ext.evt_prefix = NULL;
+
 	/* Avoid potential lockdep warnings from the *_flush() calls by
 	 * ensuring the workqueue is empty up front.
 	 */
-- 
2.24.1

Re: [PATCH v2 2/2] Bluetooth: btusb: Read the supported features of Microsoft vendor extension

[Marcel Holtmann]

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 v2:
> - Issue a HCI_VS_MSFT_Read_Supported_Features command with
> __hci_cmd_sync() instead of constructing a request.
> 
> drivers/bluetooth/btusb.c          |  3 ++
> include/net/bluetooth/hci_core.h   |  4 ++
> include/net/bluetooth/vendor_hci.h | 51 +++++++++++++++++++
> net/bluetooth/hci_core.c           | 78 ++++++++++++++++++++++++++++++
> 4 files changed, 136 insertions(+)
> create mode 100644 include/net/bluetooth/vendor_hci.h
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 4c49f394f174..410d50dbd4e2 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -3738,6 +3738,9 @@ static int btusb_probe(struct usb_interface *intf,
> 	hdev->notify = btusb_notify;
> 
> 	hdev->msft_ext.opcode = HCI_OP_NOP;
> +	hdev->msft_ext.features = 0;
> +	hdev->msft_ext.evt_prefix_len = 0;
> +	hdev->msft_ext.evt_prefix = NULL;

as noted in the other review, let hci_alloc_dev and hci_free_dev deal with this.

> 
> #ifdef CONFIG_PM
> 	err = btusb_config_oob_wake(hdev);
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 0ec3d9b41d81..f2876c5067a4 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
> @@ -246,6 +247,9 @@ struct amp_assoc {
> 
> struct msft_vnd_ext {
> 	__u16	opcode;
> +	__u64	features;
> +	__u8	evt_prefix_len;
> +	void	*evt_prefix;
> };
> 
> struct hci_dev {
> diff --git a/include/net/bluetooth/vendor_hci.h b/include/net/bluetooth/vendor_hci.h
> new file mode 100644
> index 000000000000..89a6795e672c
> --- /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  evt_prefix_len;
> +	__u8  evt_prefix[0];
> +} __packed;
> +
> +#endif /* __VENDOR_HCI_H */

Lets put this all in net/bluetooth/msft.c for now.

> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index dbd2ad3a26ed..1ea32d10ed08 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1407,6 +1407,76 @@ static void hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
> 	bacpy(&hdev->public_addr, &ba);
> }
> 
> +static void process_msft_vnd_ext_cmd_complete(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 (IS_ERR(skb)) {
> +		BT_WARN("%s: Microsoft extension response packet invalid",
> +			hdev->name);
> +		return;
> +	}
> +
> +	if (status) {
> +		BT_WARN("%s: Microsoft extension sub command 0x%2.2x failed",
> +			hdev->name, 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;
> +		u8 prefix_len = rp->evt_prefix_len;
> +
> +		hdev->msft_ext.features = __le64_to_cpu(rp->features);
> +		hdev->msft_ext.evt_prefix_len = prefix_len;
> +		hdev->msft_ext.evt_prefix = kmalloc(prefix_len, GFP_ATOMIC);

Are we really in interrupt context here? I don’t think there is a need for GFP_ATOMIC.

> +		if (!hdev->msft_ext.evt_prefix) {
> +			BT_WARN("%s: Microsoft extension invalid event prefix",
> +				hdev->name);

Please start using bt_dev_warn etc.

> +			return;
> +		}
> +
> +		memcpy(hdev->msft_ext.evt_prefix, rp->evt_prefix, prefix_len);
> +		BT_INFO("%s: Microsoft extension features 0x%016llx",
> +			hdev->name, hdev->msft_ext.features);
> +		break;
> +	}
> +	default:
> +		BT_WARN("%s: Microsoft extension unknown sub opcode 0x%2.2x",
> +			hdev->name, sub_opcode);
> +		break;
> +	}
> +}
> +
> +static void read_vendor_extension_features(struct hci_dev *hdev)
> +{
> +	struct sk_buff *skb;
> +	const u16 msft_opcode = hdev->msft_ext.opcode;
> +
> +	if (msft_opcode !=  HCI_OP_NOP) {

I really prefer it this way

	if (!something_supported)
		return;

> +		struct msft_cp_read_supported_features cp;
> +
> +		cp.sub_opcode = MSFT_OP_READ_SUPPORTED_FEATURES;
> +		skb = __hci_cmd_sync(hdev, msft_opcode, sizeof(cp), &cp,
> +				     HCI_CMD_TIMEOUT);
> +
> +		process_msft_vnd_ext_cmd_complete(hdev, skb);
> +		if (skb) {
> +			kfree_skb(skb);
> +			skb = NULL;
> +		}
> +	}
> +}
> +
> static int hci_dev_do_open(struct hci_dev *hdev)
> {
> 	int ret = 0;
> @@ -1554,6 +1624,11 @@ static int hci_dev_do_open(struct hci_dev *hdev)
> 		}
> 	}
> 
> +	/* Check features supported by HCI extensions after the init procedure
> +	 * completed.
> +	 */
> +	read_vendor_extension_features(hdev);
> +

	msft_do_open(hdev);


> 	/* If the HCI Reset command is clearing all diagnostic settings,
> 	 * then they need to be reprogrammed after the init procedure
> 	 * completed.
> @@ -1733,6 +1808,9 @@ int hci_dev_do_close(struct hci_dev *hdev)
> 			cancel_delayed_work_sync(&adv_instance->rpa_expired_cb);
> 	}
> 
> +	kfree(hdev->msft_ext.evt_prefix);
> +	hdev->msft_ext.evt_prefix = NULL;
> +

	msft_do_close(hdev);


And let these two function clear, init, free etc. everything except hdev->msft_ext.opcode

That said, I would actually also introduce a wrapper msft_set_opcode(hdev, opcode); so that the driver doesn’t have to know the internal on how that opcode is stored.

We can also keep the struct msft_ext internal to msft.c and don’t have to expose the internal details. So all stay confined in net/bluetooth/msft.c.

> 	/* Avoid potential lockdep warnings from the *_flush() calls by
> 	 * ensuring the workqueue is empty up front.
> 	 */

Regards

Marcel

Re: [PATCH v2 2/2] Bluetooth: btusb: Read the supported features of Microsoft vendor extension

[Miao-chen Chou]

On Wed, Mar 25, 2020 at 1:25 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> 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 v2:
> > - Issue a HCI_VS_MSFT_Read_Supported_Features command with
> > __hci_cmd_sync() instead of constructing a request.
> >
> > drivers/bluetooth/btusb.c          |  3 ++
> > include/net/bluetooth/hci_core.h   |  4 ++
> > include/net/bluetooth/vendor_hci.h | 51 +++++++++++++++++++
> > net/bluetooth/hci_core.c           | 78 ++++++++++++++++++++++++++++++
> > 4 files changed, 136 insertions(+)
> > create mode 100644 include/net/bluetooth/vendor_hci.h
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 4c49f394f174..410d50dbd4e2 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -3738,6 +3738,9 @@ static int btusb_probe(struct usb_interface *intf,
> >       hdev->notify = btusb_notify;
> >
> >       hdev->msft_ext.opcode = HCI_OP_NOP;
> > +     hdev->msft_ext.features = 0;
> > +     hdev->msft_ext.evt_prefix_len = 0;
> > +     hdev->msft_ext.evt_prefix = NULL;
>
> as noted in the other review, let hci_alloc_dev and hci_free_dev deal with this.
Will address this in v3.
>
> >
> > #ifdef CONFIG_PM
> >       err = btusb_config_oob_wake(hdev);
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 0ec3d9b41d81..f2876c5067a4 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
> > @@ -246,6 +247,9 @@ struct amp_assoc {
> >
> > struct msft_vnd_ext {
> >       __u16   opcode;
> > +     __u64   features;
> > +     __u8    evt_prefix_len;
> > +     void    *evt_prefix;
> > };
> >
> > struct hci_dev {
> > diff --git a/include/net/bluetooth/vendor_hci.h b/include/net/bluetooth/vendor_hci.h
> > new file mode 100644
> > index 000000000000..89a6795e672c
> > --- /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  evt_prefix_len;
> > +     __u8  evt_prefix[0];
> > +} __packed;
> > +
> > +#endif /* __VENDOR_HCI_H */
>
> Lets put this all in net/bluetooth/msft.c for now.
Will address in v3.
>
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index dbd2ad3a26ed..1ea32d10ed08 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -1407,6 +1407,76 @@ static void hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
> >       bacpy(&hdev->public_addr, &ba);
> > }
> >
> > +static void process_msft_vnd_ext_cmd_complete(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 (IS_ERR(skb)) {
> > +             BT_WARN("%s: Microsoft extension response packet invalid",
> > +                     hdev->name);
> > +             return;
> > +     }
> > +
> > +     if (status) {
> > +             BT_WARN("%s: Microsoft extension sub command 0x%2.2x failed",
> > +                     hdev->name, 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;
> > +             u8 prefix_len = rp->evt_prefix_len;
> > +
> > +             hdev->msft_ext.features = __le64_to_cpu(rp->features);
> > +             hdev->msft_ext.evt_prefix_len = prefix_len;
> > +             hdev->msft_ext.evt_prefix = kmalloc(prefix_len, GFP_ATOMIC);
>
> Are we really in interrupt context here? I don’t think there is a need for GFP_ATOMIC.
Not really, will change this to GFP_KERNEL.
>
> > +             if (!hdev->msft_ext.evt_prefix) {
> > +                     BT_WARN("%s: Microsoft extension invalid event prefix",
> > +                             hdev->name);
>
> Please start using bt_dev_warn etc.
Will address in v3.
>
> > +                     return;
> > +             }
> > +
> > +             memcpy(hdev->msft_ext.evt_prefix, rp->evt_prefix, prefix_len);
> > +             BT_INFO("%s: Microsoft extension features 0x%016llx",
> > +                     hdev->name, hdev->msft_ext.features);
> > +             break;
> > +     }
> > +     default:
> > +             BT_WARN("%s: Microsoft extension unknown sub opcode 0x%2.2x",
> > +                     hdev->name, sub_opcode);
> > +             break;
> > +     }
> > +}
> > +
> > +static void read_vendor_extension_features(struct hci_dev *hdev)
> > +{
> > +     struct sk_buff *skb;
> > +     const u16 msft_opcode = hdev->msft_ext.opcode;
> > +
> > +     if (msft_opcode !=  HCI_OP_NOP) {
>
> I really prefer it this way
>
>         if (!something_supported)
>                 return;
Will address in v3.
>
> > +             struct msft_cp_read_supported_features cp;
> > +
> > +             cp.sub_opcode = MSFT_OP_READ_SUPPORTED_FEATURES;
> > +             skb = __hci_cmd_sync(hdev, msft_opcode, sizeof(cp), &cp,
> > +                                  HCI_CMD_TIMEOUT);
> > +
> > +             process_msft_vnd_ext_cmd_complete(hdev, skb);
> > +             if (skb) {
> > +                     kfree_skb(skb);
> > +                     skb = NULL;
> > +             }
> > +     }
> > +}
> > +
> > static int hci_dev_do_open(struct hci_dev *hdev)
> > {
> >       int ret = 0;
> > @@ -1554,6 +1624,11 @@ static int hci_dev_do_open(struct hci_dev *hdev)
> >               }
> >       }
> >
> > +     /* Check features supported by HCI extensions after the init procedure
> > +      * completed.
> > +      */
> > +     read_vendor_extension_features(hdev);
> > +
>
>         msft_do_open(hdev);
>
>
> >       /* If the HCI Reset command is clearing all diagnostic settings,
> >        * then they need to be reprogrammed after the init procedure
> >        * completed.
> > @@ -1733,6 +1808,9 @@ int hci_dev_do_close(struct hci_dev *hdev)
> >                       cancel_delayed_work_sync(&adv_instance->rpa_expired_cb);
> >       }
> >
> > +     kfree(hdev->msft_ext.evt_prefix);
> > +     hdev->msft_ext.evt_prefix = NULL;
> > +
>
>         msft_do_close(hdev);
>
>
> And let these two function clear, init, free etc. everything except hdev->msft_ext.opcode
>
> That said, I would actually also introduce a wrapper msft_set_opcode(hdev, opcode); so that the driver doesn’t have to know the internal on how that opcode is stored.
>
> We can also keep the struct msft_ext internal to msft.c and don’t have to expose the internal details. So all stay confined in net/bluetooth/msft.c.
Good point. Will add net/bluetooth/msft.c in v3.
>
> >       /* Avoid potential lockdep warnings from the *_flush() calls by
> >        * ensuring the workqueue is empty up front.
> >        */
Regards,
Miao