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

[PATCH v3 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 v3:
- Create net/bluetooth/msft.c with struct msft_vnd_ext defined internally
and change the hdev->msft_ext field to void*.
- Define and expose msft_vnd_ext_set_opcode() for btusb use.
- Init hdev->msft_ext in hci_alloc_dev() and deinit it in hci_free_dev().
- Introduce msft_vnd_ext_do_open() and msft_vnd_ext_do_close().

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        |  10 ++-
 include/net/bluetooth/hci_core.h |   4 ++
 net/bluetooth/hci_core.c         | 120 +++++++++++++++++++++++++++++++
 net/bluetooth/msft.c             |  52 ++++++++++++++
 4 files changed, 184 insertions(+), 2 deletions(-)
 create mode 100644 net/bluetooth/msft.c

-- 
2.24.1

[PATCH v3 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 v3:
- Create net/bluetooth/msft.c with struct msft_vnd_ext defined internally
and change the hdev->msft_ext field to void*.
- Define and expose msft_vnd_ext_set_opcode() for btusb use.
- Init hdev->msft_ext in hci_alloc_dev() and deinit it in hci_free_dev().

 drivers/bluetooth/btusb.c        | 10 ++++++++--
 include/net/bluetooth/hci_core.h |  4 ++++
 net/bluetooth/hci_core.c         | 25 +++++++++++++++++++++++++
 net/bluetooth/msft.c             | 25 +++++++++++++++++++++++++
 4 files changed, 62 insertions(+), 2 deletions(-)
 create mode 100644 net/bluetooth/msft.c

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 3bdec42c9612..f9ce35f1be58 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),
@@ -3800,6 +3803,9 @@ 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)
+			msft_vnd_ext_set_opcode(hdev, 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..7dc5a1d00a87 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -414,6 +414,8 @@ struct hci_dev {
 	void			*smp_data;
 	void			*smp_bredr_data;
 
+	void			*msft_ext;
+
 	struct discovery_state	discovery;
 
 	int			discovery_old_state;
@@ -1658,6 +1660,8 @@ void hci_le_start_enc(struct hci_conn *conn, __le16 ediv, __le64 rand,
 void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
 			       u8 *bdaddr_type);
 
+void msft_vnd_ext_set_opcode(struct hci_dev *hdev, u16 opcode);
+
 #define SCO_AIRMODE_MASK       0x0003
 #define SCO_AIRMODE_CVSD       0x0000
 #define SCO_AIRMODE_TRANSP     0x0003
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index dbd2ad3a26ed..286294540bed 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -44,6 +44,7 @@
 #include "hci_debugfs.h"
 #include "smp.h"
 #include "leds.h"
+#include "msft.c"
 
 static void hci_rx_work(struct work_struct *work);
 static void hci_cmd_work(struct work_struct *work);
@@ -3269,6 +3270,19 @@ void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
 	}
 }
 
+/* This function sets the opcode of Microsoft vendor extension */
+void msft_vnd_ext_set_opcode(struct hci_dev *hdev, u16 opcode)
+{
+	struct msft_vnd_ext *msft_ext;
+
+	if (!hdev || !hdev->msft_ext)
+		return;
+
+	msft_ext = (struct msft_vnd_ext *)hdev->msft_ext;
+	msft_ext->opcode = opcode;
+}
+EXPORT_SYMBOL(msft_vnd_ext_set_opcode);
+
 static int hci_suspend_wait_event(struct hci_dev *hdev)
 {
 #define WAKE_COND                                                              \
@@ -3360,6 +3374,7 @@ static int hci_suspend_notifier(struct notifier_block *nb, unsigned long action,
 struct hci_dev *hci_alloc_dev(void)
 {
 	struct hci_dev *hdev;
+	struct msft_vnd_ext *msft_ext;
 
 	hdev = kzalloc(sizeof(*hdev), GFP_KERNEL);
 	if (!hdev)
@@ -3408,6 +3423,14 @@ struct hci_dev *hci_alloc_dev(void)
 	hdev->auth_payload_timeout = DEFAULT_AUTH_PAYLOAD_TIMEOUT;
 	hdev->min_enc_key_size = HCI_MIN_ENC_KEY_SIZE;
 
+	msft_ext = kzalloc(sizeof(*msft_ext), GFP_KERNEL);
+	if (!msft_ext) {
+		kfree(hdev);
+		return NULL;
+	}
+	msft_ext->opcode = HCI_OP_NOP;
+	hdev->msft_ext = (void*)msft_ext;
+
 	mutex_init(&hdev->lock);
 	mutex_init(&hdev->req_lock);
 
@@ -3459,6 +3482,8 @@ EXPORT_SYMBOL(hci_alloc_dev);
 /* Free HCI device */
 void hci_free_dev(struct hci_dev *hdev)
 {
+	kfree(hdev->msft_ext);
+
 	/* will free via device release */
 	put_device(&hdev->dev);
 }
diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
new file mode 100644
index 000000000000..c7ede27095be
--- /dev/null
+++ b/net/bluetooth/msft.c
@@ -0,0 +1,25 @@
+// 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.
+
+struct msft_vnd_ext {
+	__u16	opcode;
+};
-- 
2.24.1

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

[Joe Perches]

On Thu, 2020-03-26 at 00:59 -0700, Miao-chen Chou wrote:
> 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
[]
> @@ -414,6 +414,8 @@ struct hci_dev {
>  	void			*smp_data;
>  	void			*smp_bredr_data;
>  
> +	void			*msft_ext;

Why should this be a void * and not a msft_vnd_ext * ?

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

[Miao-chen Chou]

On Thu, Mar 26, 2020 at 1:16 AM Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2020-03-26 at 00:59 -0700, Miao-chen Chou wrote:
> > 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
> []
> > @@ -414,6 +414,8 @@ struct hci_dev {
> >       void                    *smp_data;
> >       void                    *smp_bredr_data;
> >
> > +     void                    *msft_ext;
>
> Why should this be a void * and not a msft_vnd_ext * ?
The intention is to hide msft_vnd_ext from the driver.

Re: [PATCH v3 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 v3:
> - Create net/bluetooth/msft.c with struct msft_vnd_ext defined internally
> and change the hdev->msft_ext field to void*.
> - Define and expose msft_vnd_ext_set_opcode() for btusb use.
> - Init hdev->msft_ext in hci_alloc_dev() and deinit it in hci_free_dev().

so I spent some cycles on thinking about on how we can have this nice and cleanly without putting too much into the core stack or hci_dev. I took your patches and converted them a little bit into how I would do it. Please have a look.

Regards

Marcel

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

[Miao-chen Chou]

On Thu, Mar 26, 2020 at 2:01 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 v3:
> > - Create net/bluetooth/msft.c with struct msft_vnd_ext defined internally
> > and change the hdev->msft_ext field to void*.
> > - Define and expose msft_vnd_ext_set_opcode() for btusb use.
> > - Init hdev->msft_ext in hci_alloc_dev() and deinit it in hci_free_dev().
>
> so I spent some cycles on thinking about on how we can have this nice and cleanly without putting too much into the core stack or hci_dev. I took your patches and converted them a little bit into how I would do it. Please have a look.
Thanks for brainstorming the framework. I will address your suggestion
in v4 shortly.

Regards,
Miao

[PATCH v3 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 ThunderPeak BT controller where
the Microsoft vendor extension features are 0x000000000000003f.

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

Changes in v3:
- Introduce msft_vnd_ext_do_open() and msft_vnd_ext_do_close().

 net/bluetooth/hci_core.c | 95 ++++++++++++++++++++++++++++++++++++++++
 net/bluetooth/msft.c     | 27 ++++++++++++
 2 files changed, 122 insertions(+)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 286294540bed..1062692e8612 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1408,6 +1408,87 @@ 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;
+	struct msft_vnd_ext *msft_ext = (struct msft_vnd_ext *)hdev->msft_ext;
+	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_dev_warn(hdev, "Microsoft extension response packet invalid");
+		return;
+	}
+
+	if (status) {
+		bt_dev_warn(hdev, "Microsoft extension sub command 0x%2.2x failed",
+			    sub_opcode);
+		return;
+	}
+
+	bt_dev_dbg(hdev, "status 0x%2.2x sub opcode 0x%2.2x", 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;
+
+		msft_ext->features = __le64_to_cpu(rp->features);
+		msft_ext->evt_prefix_len = prefix_len;
+		msft_ext->evt_prefix = kmalloc(prefix_len, GFP_ATOMIC);
+		if (!msft_ext->evt_prefix) {
+			bt_dev_warn(hdev, "Microsoft extension invalid event prefix");
+			return;
+		}
+
+		memcpy(msft_ext->evt_prefix, rp->evt_prefix, prefix_len);
+		bt_dev_info(hdev, "Microsoft extension features 0x%016llx",
+			    msft_ext->features);
+		break;
+	}
+	default:
+		bt_dev_warn(hdev, "Microsoft extension unknown sub opcode 0x%2.2x",
+			    sub_opcode);
+		break;
+	}
+}
+
+static bool msft_vnd_ext_supported(struct hci_dev *hdev)
+{
+	struct msft_vnd_ext *msft_ext = (struct msft_vnd_ext *)hdev->msft_ext;
+
+	return msft_ext->opcode != HCI_OP_NOP;
+}
+
+static void msft_vnd_ext_do_open(struct hci_dev *hdev)
+{
+	struct sk_buff *skb;
+	struct msft_vnd_ext *msft_ext = (struct msft_vnd_ext *)hdev->msft_ext;
+	struct msft_cp_read_supported_features cp;
+
+	msft_ext->features = 0;
+	msft_ext->evt_prefix_len = 0;
+	msft_ext->evt_prefix = NULL;
+
+	if (!msft_vnd_ext_supported(hdev))
+		return;
+
+	cp.sub_opcode = MSFT_OP_READ_SUPPORTED_FEATURES;
+	skb = __hci_cmd_sync(hdev, msft_ext->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;
@@ -1555,6 +1636,8 @@ static int hci_dev_do_open(struct hci_dev *hdev)
 		}
 	}
 
+	msft_vnd_ext_do_open(hdev);
+
 	/* If the HCI Reset command is clearing all diagnostic settings,
 	 * then they need to be reprogrammed after the init procedure
 	 * completed.
@@ -1685,6 +1768,16 @@ static void hci_pend_le_actions_clear(struct hci_dev *hdev)
 	BT_DBG("All LE pending actions cleared");
 }
 
+static void msft_vnd_ext_do_close(struct hci_dev *hdev)
+{
+	struct msft_vnd_ext *msft_ext = (struct msft_vnd_ext *)hdev->msft_ext;
+
+	msft_ext->features = 0;
+	msft_ext->evt_prefix_len = 0;
+	kfree(msft_ext->evt_prefix);
+	msft_ext->evt_prefix = NULL;
+}
+
 int hci_dev_do_close(struct hci_dev *hdev)
 {
 	bool auto_off;
@@ -1734,6 +1827,8 @@ int hci_dev_do_close(struct hci_dev *hdev)
 			cancel_delayed_work_sync(&adv_instance->rpa_expired_cb);
 	}
 
+	msft_vnd_ext_do_close(hdev);
+
 	/* Avoid potential lockdep warnings from the *_flush() calls by
 	 * ensuring the workqueue is empty up front.
 	 */
diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
index c7ede27095be..21cb32105206 100644
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -20,6 +20,33 @@
 // COPYRIGHTS, TRADEMARKS OR OTHER RIGHTS, RELATING TO USE OF THIS
 // SOFTWARE IS DISCLAIMED.
 
+#define MSFT_EVT_PREFIX_MAX_LEN			255
+
 struct msft_vnd_ext {
 	__u16	opcode;
+	__u64	features;
+	__u8	evt_prefix_len;
+	__u8	*evt_prefix;
 };
+
+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;
-- 
2.24.1