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

[PATCH v4 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 v4:
- Introduce CONFIG_BT_MSFTEXT as a starting point of providing a
framework to use Microsoft extension
- Create include/net/bluetooth/msft.h and net/bluetooth/msft.c to
facilitate functions of Microsoft extension.
- Move MSFT's do_open() and do_close() from net/bluetooth/hci_core.c to
net/bluetooth/msft.c.
- Other than msft opcode, define struct msft_data to host the rest of
information of Microsoft extension and leave a void* pointing to a
msft_data in struct hci_dev.

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().

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        |  11 ++-
 include/net/bluetooth/hci_core.h |   5 ++
 net/bluetooth/Kconfig            |   9 +-
 net/bluetooth/Makefile           |   1 +
 net/bluetooth/hci_core.c         |   5 ++
 net/bluetooth/hci_event.c        |   5 ++
 net/bluetooth/msft.c             | 142 +++++++++++++++++++++++++++++++
 net/bluetooth/msft.h             |  29 +++++++
 8 files changed, 204 insertions(+), 3 deletions(-)
 create mode 100644 net/bluetooth/msft.c
 create mode 100644 net/bluetooth/msft.h

-- 
2.24.1

[PATCH v4 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 also add a kernel config, BT_MSFTEXT, and a
source file to facilitate Microsoft vendor extension functions.
This was verified with Intel ThunderPeak BT controller
where msft_vnd_ext_opcode is 0xFC1E.

Signed-off-by: Marcel Holtmann <marcel@holtmann.org>

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

Changes in v4:
- Introduce CONFIG_BT_MSFTEXT as a starting point of providing a
framework to use Microsoft extension
- Create include/net/bluetooth/msft.h and net/bluetooth/msft.c to
facilitate functions of Microsoft extension.

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().

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        | 11 +++++++++--
 include/net/bluetooth/hci_core.h |  4 ++++
 net/bluetooth/Kconfig            |  9 ++++++++-
 net/bluetooth/Makefile           |  1 +
 net/bluetooth/msft.c             | 16 ++++++++++++++++
 net/bluetooth/msft.h             | 19 +++++++++++++++++++
 6 files changed, 57 insertions(+), 3 deletions(-)
 create mode 100644 net/bluetooth/msft.c
 create mode 100644 net/bluetooth/msft.h

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 3bdec42c9612..0fe47708d3c8 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -21,6 +21,7 @@
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 
+#include "../../net/bluetooth/msft.h"
 #include "btintel.h"
 #include "btbcm.h"
 #include "btrtl.h"
@@ -58,6 +59,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 +337,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 +351,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 +3804,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_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..239cae2d9998 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -484,6 +484,10 @@ struct hci_dev {
 	struct led_trigger	*power_led;
 #endif
 
+#if IS_ENABLED(CONFIG_BT_MSFTEXT)
+	__u16			msft_opcode;
+#endif
+
 	int (*open)(struct hci_dev *hdev);
 	int (*close)(struct hci_dev *hdev);
 	int (*flush)(struct hci_dev *hdev);
diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
index 165148c7c4ce..5929ccb02b39 100644
--- a/net/bluetooth/Kconfig
+++ b/net/bluetooth/Kconfig
@@ -30,7 +30,7 @@ menuconfig BT
 		L2CAP (Logical Link Control and Adaptation Protocol)
 		SMP (Security Manager Protocol) on LE (Low Energy) links
 	     HCI Device drivers (Interface to the hardware)
-	     RFCOMM Module (RFCOMM Protocol)  
+	     RFCOMM Module (RFCOMM Protocol)
 	     BNEP Module (Bluetooth Network Encapsulation Protocol)
 	     CMTP Module (CAPI Message Transport Protocol)
 	     HIDP Module (Human Interface Device Protocol)
@@ -93,6 +93,13 @@ config BT_LEDS
 	  This option selects a few LED triggers for different
 	  Bluetooth events.
 
+config BT_MSFTEXT
+	bool "Enable Microsoft extensions"
+	depends on BT
+	help
+	  This options enables support for the Microsoft defined HCI
+	  vendor extensions.
+
 config BT_SELFTEST
 	bool "Bluetooth self testing support"
 	depends on BT && DEBUG_KERNEL
diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile
index fda41c0b4781..41dd541a44a5 100644
--- a/net/bluetooth/Makefile
+++ b/net/bluetooth/Makefile
@@ -19,5 +19,6 @@ bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \
 bluetooth-$(CONFIG_BT_BREDR) += sco.o
 bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o
 bluetooth-$(CONFIG_BT_LEDS) += leds.o
+bluetooth-$(CONFIG_BT_MSFTEXT) += msft.o
 bluetooth-$(CONFIG_BT_DEBUGFS) += hci_debugfs.o
 bluetooth-$(CONFIG_BT_SELFTEST) += selftest.o
diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
new file mode 100644
index 000000000000..7609932c48ca
--- /dev/null
+++ b/net/bluetooth/msft.c
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* Copyright (C) 2020 Google Corporation */
+
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci_core.h>
+
+#include "msft.h"
+
+void msft_set_opcode(struct hci_dev *hdev, __u16 opcode)
+{
+	hdev->msft_opcode = opcode;
+
+	bt_dev_info(hdev, "Enabling MSFT extensions with opcode 0x%2.2x",
+		    hdev->msft_opcode);
+}
+EXPORT_SYMBOL(msft_set_opcode);
diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
new file mode 100644
index 000000000000..7218ea759dde
--- /dev/null
+++ b/net/bluetooth/msft.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Copyright (C) 2020 Google Corporation */
+
+#ifndef __MSFT_H
+#define __MSFT_H
+
+#include <net/bluetooth/hci_core.h>
+
+#if IS_ENABLED(CONFIG_BT_MSFTEXT)
+
+void msft_set_opcode(struct hci_dev *hdev, __u16 opcode);
+
+#else
+
+static inline void msft_set_opcode(struct hci_dev *hdev, __u16 opcode) {}
+
+#endif
+
+#endif /* __MSFT_H*/
-- 
2.24.1

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

[Miao-chen Chou]

This defines opcode and packet structures of Microsoft vendor extension.
For now, we add only the HCI_VS_MSFT_Read_Supported_Features command. 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: Marcel Holtmann <marcel@holtmann.org>

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

Changes in v4:
- Move MSFT's do_open() and do_close() from net/bluetooth/hci_core.c to
net/bluetooth/msft.c.
- Other than msft opcode, define struct msft_data to host the rest of
information of Microsoft extension and leave a void* pointing to a
msft_data in struct hci_dev.

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

Changes in v2:
- Issue a HCI_VS_MSFT_Read_Supported_Features command with
__hci_cmd_sync() instead of constructing a request.

 include/net/bluetooth/hci_core.h |   1 +
 net/bluetooth/hci_core.c         |   5 ++
 net/bluetooth/hci_event.c        |   5 ++
 net/bluetooth/msft.c             | 126 +++++++++++++++++++++++++++++++
 net/bluetooth/msft.h             |  10 +++
 5 files changed, 147 insertions(+)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 239cae2d9998..59ddcd3a52cc 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -486,6 +486,7 @@ struct hci_dev {
 
 #if IS_ENABLED(CONFIG_BT_MSFTEXT)
 	__u16			msft_opcode;
+	void			*msft_data;
 #endif
 
 	int (*open)(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index dbd2ad3a26ed..c38707de767a 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.h"
 
 static void hci_rx_work(struct work_struct *work);
 static void hci_cmd_work(struct work_struct *work);
@@ -1563,6 +1564,8 @@ static int hci_dev_do_open(struct hci_dev *hdev)
 	    hci_dev_test_flag(hdev, HCI_VENDOR_DIAG) && hdev->set_diag)
 		ret = hdev->set_diag(hdev, true);
 
+	msft_do_open(hdev);
+
 	clear_bit(HCI_INIT, &hdev->flags);
 
 	if (!ret) {
@@ -1758,6 +1761,8 @@ int hci_dev_do_close(struct hci_dev *hdev)
 
 	hci_sock_dev_event(hdev, HCI_DEV_DOWN);
 
+	msft_do_close(hdev);
+
 	if (hdev->flush)
 		hdev->flush(hdev);
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 20408d386268..42b5871151a6 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -35,6 +35,7 @@
 #include "a2mp.h"
 #include "amp.h"
 #include "smp.h"
+#include "msft.h"
 
 #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
 		 "\x00\x00\x00\x00\x00\x00\x00\x00"
@@ -6144,6 +6145,10 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
 		hci_num_comp_blocks_evt(hdev, skb);
 		break;
 
+	case HCI_EV_VENDOR:
+		msft_vendor_evt(hdev, skb);
+		break;
+
 	default:
 		BT_DBG("%s event 0x%2.2x", hdev->name, event);
 		break;
diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
index 7609932c48ca..f76e4c79556e 100644
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -6,6 +6,24 @@
 
 #include "msft.h"
 
+#define MSFT_OP_READ_SUPPORTED_FEATURES		0x00
+struct msft_cp_read_supported_features {
+	__u8   sub_opcode;
+} __packed;
+struct msft_rp_read_supported_features {
+	__u8   status;
+	__u8   sub_opcode;
+	__le64 features;
+	__u8   evt_prefix_len;
+	__u8   evt_prefix[0];
+} __packed;
+
+struct msft_data {
+	__u64 features;
+	__u8  evt_prefix_len;
+	__u8  *evt_prefix;
+};
+
 void msft_set_opcode(struct hci_dev *hdev, __u16 opcode)
 {
 	hdev->msft_opcode = opcode;
@@ -14,3 +32,111 @@ void msft_set_opcode(struct hci_dev *hdev, __u16 opcode)
 		    hdev->msft_opcode);
 }
 EXPORT_SYMBOL(msft_set_opcode);
+
+static struct msft_data *read_supported_features(struct hci_dev *hdev)
+{
+	struct msft_data *msft;
+	struct msft_cp_read_supported_features cp;
+	struct msft_rp_read_supported_features *rp;
+	struct sk_buff *skb;
+
+	cp.sub_opcode = MSFT_OP_READ_SUPPORTED_FEATURES;
+
+	skb = __hci_cmd_sync(hdev, hdev->msft_opcode, sizeof(cp), &cp,
+			     HCI_CMD_TIMEOUT);
+	if (IS_ERR(skb)) {
+		bt_dev_err(hdev, "Failed to read MSFT supported features (%ld)",
+			   PTR_ERR(skb));
+		return NULL;
+	}
+
+	if (skb->len < sizeof(*rp)) {
+		bt_dev_err(hdev, "MSFT supported features length mismatch");
+		goto failed;
+	}
+
+	rp = (struct msft_rp_read_supported_features *)skb->data;
+
+	if (rp->sub_opcode != MSFT_OP_READ_SUPPORTED_FEATURES)
+		goto failed;
+
+	msft = kzalloc(sizeof(*msft), GFP_KERNEL);
+	if (!msft)
+		goto failed;
+
+	if (rp->evt_prefix_len > 0) {
+		msft->evt_prefix = kmemdup(rp->evt_prefix, rp->evt_prefix_len,
+					   GFP_KERNEL);
+		if (!msft->evt_prefix)
+			goto failed;
+	}
+
+	msft->evt_prefix_len = rp->evt_prefix_len;
+	msft->features = __le64_to_cpu(rp->features);
+	kfree_skb(skb);
+
+	bt_dev_info(hdev, "MSFT supported features %llx", msft->features);
+	return msft;
+
+failed:
+	kfree_skb(skb);
+	return NULL;
+}
+
+void msft_do_open(struct hci_dev *hdev)
+{
+	if (hdev->msft_opcode == HCI_OP_NOP)
+		return;
+
+	bt_dev_dbg(hdev, "Initialize MSFT extension");
+	hdev->msft_data = read_supported_features(hdev);
+}
+
+void msft_do_close(struct hci_dev *hdev)
+{
+	struct msft_data *msft = hdev->msft_data;
+
+	if (!msft)
+		return;
+
+	bt_dev_dbg(hdev, "Cleanup of MSFT extension");
+
+	hdev->msft_data = NULL;
+
+	kfree(msft->evt_prefix);
+	kfree(msft);
+}
+
+int msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	struct msft_data *msft = hdev->msft_data;
+	u8 event;
+
+	if (!msft)
+		return -ENOSYS;
+
+	/* When the extension has defined an event prefix, check that it
+	 * matches, and otherwise just return.
+	 */
+	if (msft->evt_prefix_len > 0) {
+		if (skb->len < msft->evt_prefix_len)
+			return -ENOSYS;
+
+		if (memcmp(skb->data, msft->evt_prefix, msft->evt_prefix_len))
+			return -ENOSYS;
+
+		skb_pull(skb, msft->evt_prefix_len);
+	}
+
+	/* Every event starts at least with an event code and the rest of
+	 * the data is variable and depends on the event code. Returns true
+	 */
+	if (skb->len < 1)
+		return -EBADMSG;
+
+	event = *skb->data;
+	skb_pull(skb, 1);
+
+	bt_dev_dbg(hdev, "MSFT vendor event %u", event);
+	return 0;
+}
diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
index 7218ea759dde..6a7d0ac6c66c 100644
--- a/net/bluetooth/msft.h
+++ b/net/bluetooth/msft.h
@@ -4,15 +4,25 @@
 #ifndef __MSFT_H
 #define __MSFT_H
 
+#include <linux/errno.h>
 #include <net/bluetooth/hci_core.h>
 
 #if IS_ENABLED(CONFIG_BT_MSFTEXT)
 
 void msft_set_opcode(struct hci_dev *hdev, __u16 opcode);
+void msft_do_open(struct hci_dev *hdev);
+void msft_do_close(struct hci_dev *hdev);
+int msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
 
 #else
 
 static inline void msft_set_opcode(struct hci_dev *hdev, __u16 opcode) {}
+static inline void msft_do_open(struct hci_dev *hdev) {}
+static inline void msft_do_close(struct hci_dev *hdev) {}
+static inline int msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
+{
+	return -ENOSYS;
+}
 
 #endif
 
-- 
2.24.1

Re: [PATCH v4 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 also add a kernel config, BT_MSFTEXT, and a
> source file to facilitate Microsoft vendor extension functions.
> This was verified with Intel ThunderPeak BT controller
> where msft_vnd_ext_opcode is 0xFC1E.
> 
> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> 
> Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
> ---
> 
> Changes in v4:
> - Introduce CONFIG_BT_MSFTEXT as a starting point of providing a
> framework to use Microsoft extension
> - Create include/net/bluetooth/msft.h and net/bluetooth/msft.c to
> facilitate functions of Microsoft extension.
> 
> 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().
> 
> 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        | 11 +++++++++--
> include/net/bluetooth/hci_core.h |  4 ++++

so I don’t like the intermixing of core features and drivers unless it is needed. In this case it is not needed since we can first introduce the core support and then enable the driver to use it.

> net/bluetooth/Kconfig            |  9 ++++++++-
> net/bluetooth/Makefile           |  1 +
> net/bluetooth/msft.c             | 16 ++++++++++++++++
> net/bluetooth/msft.h             | 19 +++++++++++++++++++
> 6 files changed, 57 insertions(+), 3 deletions(-)
> create mode 100644 net/bluetooth/msft.c
> create mode 100644 net/bluetooth/msft.h
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 3bdec42c9612..0fe47708d3c8 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -21,6 +21,7 @@
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> 
> +#include "../../net/bluetooth/msft.h"

This was my bad. I didn’t realized that drivers need to the set the opcode and not the core. I updated the patches to fix this.

> #include "btintel.h"
> #include "btbcm.h"
> #include "btrtl.h"
> @@ -58,6 +59,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 +337,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 +351,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 },

Lets start with ThunderPeak 0x0025 for now. We are looking into enabling this in a more generic fashion, but for now lets just enable one card.

> 
> 	/* Other Intel Bluetooth devices */
> 	{ USB_VENDOR_AND_INTERFACE_INFO(0x8087, 0xe0, 0x01, 0x01),
> @@ -3800,6 +3804,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_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..239cae2d9998 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -484,6 +484,10 @@ struct hci_dev {
> 	struct led_trigger	*power_led;
> #endif
> 
> +#if IS_ENABLED(CONFIG_BT_MSFTEXT)
> +	__u16			msft_opcode;
> +#endif
> +
> 	int (*open)(struct hci_dev *hdev);
> 	int (*close)(struct hci_dev *hdev);
> 	int (*flush)(struct hci_dev *hdev);
> diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
> index 165148c7c4ce..5929ccb02b39 100644
> --- a/net/bluetooth/Kconfig
> +++ b/net/bluetooth/Kconfig
> @@ -30,7 +30,7 @@ menuconfig BT
> 		L2CAP (Logical Link Control and Adaptation Protocol)
> 		SMP (Security Manager Protocol) on LE (Low Energy) links
> 	     HCI Device drivers (Interface to the hardware)
> -	     RFCOMM Module (RFCOMM Protocol)  
> +	     RFCOMM Module (RFCOMM Protocol)

Unrelated changes don’t belong here.

> 	     BNEP Module (Bluetooth Network Encapsulation Protocol)
> 	     CMTP Module (CAPI Message Transport Protocol)
> 	     HIDP Module (Human Interface Device Protocol)
> @@ -93,6 +93,13 @@ config BT_LEDS
> 	  This option selects a few LED triggers for different
> 	  Bluetooth events.
> 
> +config BT_MSFTEXT
> +	bool "Enable Microsoft extensions"
> +	depends on BT
> +	help
> +	  This options enables support for the Microsoft defined HCI
> +	  vendor extensions.
> +
> config BT_SELFTEST
> 	bool "Bluetooth self testing support"
> 	depends on BT && DEBUG_KERNEL
> diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile
> index fda41c0b4781..41dd541a44a5 100644
> --- a/net/bluetooth/Makefile
> +++ b/net/bluetooth/Makefile
> @@ -19,5 +19,6 @@ bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \
> bluetooth-$(CONFIG_BT_BREDR) += sco.o
> bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o
> bluetooth-$(CONFIG_BT_LEDS) += leds.o
> +bluetooth-$(CONFIG_BT_MSFTEXT) += msft.o
> bluetooth-$(CONFIG_BT_DEBUGFS) += hci_debugfs.o
> bluetooth-$(CONFIG_BT_SELFTEST) += selftest.o
> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> new file mode 100644
> index 000000000000..7609932c48ca
> --- /dev/null
> +++ b/net/bluetooth/msft.c
> @@ -0,0 +1,16 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* Copyright (C) 2020 Google Corporation */
> +
> +#include <net/bluetooth/bluetooth.h>
> +#include <net/bluetooth/hci_core.h>
> +
> +#include "msft.h"
> +
> +void msft_set_opcode(struct hci_dev *hdev, __u16 opcode)
> +{
> +	hdev->msft_opcode = opcode;
> +
> +	bt_dev_info(hdev, "Enabling MSFT extensions with opcode 0x%2.2x",
> +		    hdev->msft_opcode);
> +}
> +EXPORT_SYMBOL(msft_set_opcode);
> diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
> new file mode 100644
> index 000000000000..7218ea759dde
> --- /dev/null
> +++ b/net/bluetooth/msft.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* Copyright (C) 2020 Google Corporation */
> +
> +#ifndef __MSFT_H
> +#define __MSFT_H
> +
> +#include <net/bluetooth/hci_core.h>
> +
> +#if IS_ENABLED(CONFIG_BT_MSFTEXT)
> +
> +void msft_set_opcode(struct hci_dev *hdev, __u16 opcode);
> +
> +#else
> +
> +static inline void msft_set_opcode(struct hci_dev *hdev, __u16 opcode) {}
> +
> +#endif
> +
> +#endif /* __MSFT_H*/

Regards

Marcel

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

[Marcel Holtmann]

Hi Miao-chen,

> This defines opcode and packet structures of Microsoft vendor extension.
> For now, we add only the HCI_VS_MSFT_Read_Supported_Features command. 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: Marcel Holtmann <marcel@holtmann.org>
> 
> Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
> ---
> 
> Changes in v4:
> - Move MSFT's do_open() and do_close() from net/bluetooth/hci_core.c to
> net/bluetooth/msft.c.
> - Other than msft opcode, define struct msft_data to host the rest of
> information of Microsoft extension and leave a void* pointing to a
> msft_data in struct hci_dev.
> 
> Changes in v3:
> - Introduce msft_vnd_ext_do_open() and msft_vnd_ext_do_close().
> 
> Changes in v2:
> - Issue a HCI_VS_MSFT_Read_Supported_Features command with
> __hci_cmd_sync() instead of constructing a request.
> 
> include/net/bluetooth/hci_core.h |   1 +
> net/bluetooth/hci_core.c         |   5 ++
> net/bluetooth/hci_event.c        |   5 ++
> net/bluetooth/msft.c             | 126 +++++++++++++++++++++++++++++++
> net/bluetooth/msft.h             |  10 +++
> 5 files changed, 147 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 239cae2d9998..59ddcd3a52cc 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -486,6 +486,7 @@ struct hci_dev {
> 
> #if IS_ENABLED(CONFIG_BT_MSFTEXT)
> 	__u16			msft_opcode;
> +	void			*msft_data;
> #endif
> 
> 	int (*open)(struct hci_dev *hdev);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index dbd2ad3a26ed..c38707de767a 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.h"
> 
> static void hci_rx_work(struct work_struct *work);
> static void hci_cmd_work(struct work_struct *work);
> @@ -1563,6 +1564,8 @@ static int hci_dev_do_open(struct hci_dev *hdev)
> 	    hci_dev_test_flag(hdev, HCI_VENDOR_DIAG) && hdev->set_diag)
> 		ret = hdev->set_diag(hdev, true);
> 
> +	msft_do_open(hdev);
> +
> 	clear_bit(HCI_INIT, &hdev->flags);
> 
> 	if (!ret) {
> @@ -1758,6 +1761,8 @@ int hci_dev_do_close(struct hci_dev *hdev)
> 
> 	hci_sock_dev_event(hdev, HCI_DEV_DOWN);
> 
> +	msft_do_close(hdev);
> +
> 	if (hdev->flush)
> 		hdev->flush(hdev);
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 20408d386268..42b5871151a6 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -35,6 +35,7 @@
> #include "a2mp.h"
> #include "amp.h"
> #include "smp.h"
> +#include "msft.h"
> 
> #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
> 		 "\x00\x00\x00\x00\x00\x00\x00\x00"
> @@ -6144,6 +6145,10 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
> 		hci_num_comp_blocks_evt(hdev, skb);
> 		break;
> 
> +	case HCI_EV_VENDOR:
> +		msft_vendor_evt(hdev, skb);
> +		break;
> +
> 	default:
> 		BT_DBG("%s event 0x%2.2x", hdev->name, event);
> 		break;
> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> index 7609932c48ca..f76e4c79556e 100644
> --- a/net/bluetooth/msft.c
> +++ b/net/bluetooth/msft.c
> @@ -6,6 +6,24 @@
> 
> #include "msft.h"
> 
> +#define MSFT_OP_READ_SUPPORTED_FEATURES		0x00
> +struct msft_cp_read_supported_features {
> +	__u8   sub_opcode;
> +} __packed;
> +struct msft_rp_read_supported_features {
> +	__u8   status;
> +	__u8   sub_opcode;
> +	__le64 features;
> +	__u8   evt_prefix_len;
> +	__u8   evt_prefix[0];
> +} __packed;
> +
> +struct msft_data {
> +	__u64 features;
> +	__u8  evt_prefix_len;
> +	__u8  *evt_prefix;
> +};
> +
> void msft_set_opcode(struct hci_dev *hdev, __u16 opcode)
> {
> 	hdev->msft_opcode = opcode;
> @@ -14,3 +32,111 @@ void msft_set_opcode(struct hci_dev *hdev, __u16 opcode)
> 		    hdev->msft_opcode);
> }
> EXPORT_SYMBOL(msft_set_opcode);
> +
> +static struct msft_data *read_supported_features(struct hci_dev *hdev)
> +{
> +	struct msft_data *msft;

I used a second parameter, but yes, my initial code was totally flawed with the msft_data access.

> +	struct msft_cp_read_supported_features cp;
> +	struct msft_rp_read_supported_features *rp;
> +	struct sk_buff *skb;
> +
> +	cp.sub_opcode = MSFT_OP_READ_SUPPORTED_FEATURES;
> +
> +	skb = __hci_cmd_sync(hdev, hdev->msft_opcode, sizeof(cp), &cp,
> +			     HCI_CMD_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		bt_dev_err(hdev, "Failed to read MSFT supported features (%ld)",
> +			   PTR_ERR(skb));
> +		return NULL;
> +	}
> +
> +	if (skb->len < sizeof(*rp)) {
> +		bt_dev_err(hdev, "MSFT supported features length mismatch");
> +		goto failed;
> +	}
> +
> +	rp = (struct msft_rp_read_supported_features *)skb->data;
> +
> +	if (rp->sub_opcode != MSFT_OP_READ_SUPPORTED_FEATURES)
> +		goto failed;
> +
> +	msft = kzalloc(sizeof(*msft), GFP_KERNEL);
> +	if (!msft)
> +		goto failed;
> +
> +	if (rp->evt_prefix_len > 0) {
> +		msft->evt_prefix = kmemdup(rp->evt_prefix, rp->evt_prefix_len,
> +					   GFP_KERNEL);
> +		if (!msft->evt_prefix)
> +			goto failed;
> +	}
> +
> +	msft->evt_prefix_len = rp->evt_prefix_len;
> +	msft->features = __le64_to_cpu(rp->features);
> +	kfree_skb(skb);
> +
> +	bt_dev_info(hdev, "MSFT supported features %llx", msft->features);
> +	return msft;
> +
> +failed:
> +	kfree_skb(skb);
> +	return NULL;
> +}
> +
> +void msft_do_open(struct hci_dev *hdev)
> +{
> +	if (hdev->msft_opcode == HCI_OP_NOP)
> +		return;
> +
> +	bt_dev_dbg(hdev, "Initialize MSFT extension");
> +	hdev->msft_data = read_supported_features(hdev);
> +}
> +
> +void msft_do_close(struct hci_dev *hdev)
> +{
> +	struct msft_data *msft = hdev->msft_data;
> +
> +	if (!msft)
> +		return;
> +
> +	bt_dev_dbg(hdev, "Cleanup of MSFT extension");
> +
> +	hdev->msft_data = NULL;
> +
> +	kfree(msft->evt_prefix);
> +	kfree(msft);
> +}
> +
> +int msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> +{

So this was on purpose void. There is no point in returning any feedback from this function. It either handles the event or it doesn’t. The caller function doesn’t care.

> +	struct msft_data *msft = hdev->msft_data;
> +	u8 event;
> +
> +	if (!msft)
> +		return -ENOSYS;
> +
> +	/* When the extension has defined an event prefix, check that it
> +	 * matches, and otherwise just return.
> +	 */
> +	if (msft->evt_prefix_len > 0) {
> +		if (skb->len < msft->evt_prefix_len)
> +			return -ENOSYS;
> +
> +		if (memcmp(skb->data, msft->evt_prefix, msft->evt_prefix_len))
> +			return -ENOSYS;
> +
> +		skb_pull(skb, msft->evt_prefix_len);
> +	}
> +
> +	/* Every event starts at least with an event code and the rest of
> +	 * the data is variable and depends on the event code. Returns true
> +	 */
> +	if (skb->len < 1)
> +		return -EBADMSG;
> +
> +	event = *skb->data;
> +	skb_pull(skb, 1);
> +
> +	bt_dev_dbg(hdev, "MSFT vendor event %u", event);
> +	return 0;
> +}
> diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
> index 7218ea759dde..6a7d0ac6c66c 100644
> --- a/net/bluetooth/msft.h
> +++ b/net/bluetooth/msft.h
> @@ -4,15 +4,25 @@
> #ifndef __MSFT_H
> #define __MSFT_H
> 
> +#include <linux/errno.h>
> #include <net/bluetooth/hci_core.h>
> 
> #if IS_ENABLED(CONFIG_BT_MSFTEXT)
> 
> void msft_set_opcode(struct hci_dev *hdev, __u16 opcode);
> +void msft_do_open(struct hci_dev *hdev);
> +void msft_do_close(struct hci_dev *hdev);
> +int msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
> 
> #else
> 
> static inline void msft_set_opcode(struct hci_dev *hdev, __u16 opcode) {}
> +static inline void msft_do_open(struct hci_dev *hdev) {}
> +static inline void msft_do_close(struct hci_dev *hdev) {}
> +static inline int msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> +	return -ENOSYS;
> +}
> 
> #endif

Regards

Marcel

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

[Miao-chen Chou]

Hi Marcel,

On Mon, Mar 30, 2020 at 3:06 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 also add a kernel config, BT_MSFTEXT, and a
> > source file to facilitate Microsoft vendor extension functions.
> > This was verified with Intel ThunderPeak BT controller
> > where msft_vnd_ext_opcode is 0xFC1E.
> >
> > Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> >
> > Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
> > ---
> >
> > Changes in v4:
> > - Introduce CONFIG_BT_MSFTEXT as a starting point of providing a
> > framework to use Microsoft extension
> > - Create include/net/bluetooth/msft.h and net/bluetooth/msft.c to
> > facilitate functions of Microsoft extension.
> >
> > 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().
> >
> > 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        | 11 +++++++++--
> > include/net/bluetooth/hci_core.h |  4 ++++
>
> so I don’t like the intermixing of core features and drivers unless it is needed. In this case it is not needed since we can first introduce the core support and then enable the driver to use it.
I will make btusb changes as a different commit in v5.
>
> > net/bluetooth/Kconfig            |  9 ++++++++-
> > net/bluetooth/Makefile           |  1 +
> > net/bluetooth/msft.c             | 16 ++++++++++++++++
> > net/bluetooth/msft.h             | 19 +++++++++++++++++++
> > 6 files changed, 57 insertions(+), 3 deletions(-)
> > create mode 100644 net/bluetooth/msft.c
> > create mode 100644 net/bluetooth/msft.h
> >
> > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> > index 3bdec42c9612..0fe47708d3c8 100644
> > --- a/drivers/bluetooth/btusb.c
> > +++ b/drivers/bluetooth/btusb.c
> > @@ -21,6 +21,7 @@
> > #include <net/bluetooth/bluetooth.h>
> > #include <net/bluetooth/hci_core.h>
> >
> > +#include "../../net/bluetooth/msft.h"
>
> This was my bad. I didn’t realized that drivers need to the set the opcode and not the core. I updated the patches to fix this.
I will move it to include/net/bluetooth/.
>
> > #include "btintel.h"
> > #include "btbcm.h"
> > #include "btrtl.h"
> > @@ -58,6 +59,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 +337,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 +351,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 },
>
> Lets start with ThunderPeak 0x0025 for now. We are looking into enabling this in a more generic fashion, but for now lets just enable one card.
Ack.
>
> >
> >       /* Other Intel Bluetooth devices */
> >       { USB_VENDOR_AND_INTERFACE_INFO(0x8087, 0xe0, 0x01, 0x01),
> > @@ -3800,6 +3804,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_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..239cae2d9998 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -484,6 +484,10 @@ struct hci_dev {
> >       struct led_trigger      *power_led;
> > #endif
> >
> > +#if IS_ENABLED(CONFIG_BT_MSFTEXT)
> > +     __u16                   msft_opcode;
> > +#endif
> > +
> >       int (*open)(struct hci_dev *hdev);
> >       int (*close)(struct hci_dev *hdev);
> >       int (*flush)(struct hci_dev *hdev);
> > diff --git a/net/bluetooth/Kconfig b/net/bluetooth/Kconfig
> > index 165148c7c4ce..5929ccb02b39 100644
> > --- a/net/bluetooth/Kconfig
> > +++ b/net/bluetooth/Kconfig
> > @@ -30,7 +30,7 @@ menuconfig BT
> >               L2CAP (Logical Link Control and Adaptation Protocol)
> >               SMP (Security Manager Protocol) on LE (Low Energy) links
> >            HCI Device drivers (Interface to the hardware)
> > -          RFCOMM Module (RFCOMM Protocol)
> > +          RFCOMM Module (RFCOMM Protocol)
>
> Unrelated changes don’t belong here.
This was probably done by my editor, I will recover it.
>
> >            BNEP Module (Bluetooth Network Encapsulation Protocol)
> >            CMTP Module (CAPI Message Transport Protocol)
> >            HIDP Module (Human Interface Device Protocol)
> > @@ -93,6 +93,13 @@ config BT_LEDS
> >         This option selects a few LED triggers for different
> >         Bluetooth events.
> >
> > +config BT_MSFTEXT
> > +     bool "Enable Microsoft extensions"
> > +     depends on BT
> > +     help
> > +       This options enables support for the Microsoft defined HCI
> > +       vendor extensions.
> > +
> > config BT_SELFTEST
> >       bool "Bluetooth self testing support"
> >       depends on BT && DEBUG_KERNEL
> > diff --git a/net/bluetooth/Makefile b/net/bluetooth/Makefile
> > index fda41c0b4781..41dd541a44a5 100644
> > --- a/net/bluetooth/Makefile
> > +++ b/net/bluetooth/Makefile
> > @@ -19,5 +19,6 @@ bluetooth-y := af_bluetooth.o hci_core.o hci_conn.o hci_event.o mgmt.o \
> > bluetooth-$(CONFIG_BT_BREDR) += sco.o
> > bluetooth-$(CONFIG_BT_HS) += a2mp.o amp.o
> > bluetooth-$(CONFIG_BT_LEDS) += leds.o
> > +bluetooth-$(CONFIG_BT_MSFTEXT) += msft.o
> > bluetooth-$(CONFIG_BT_DEBUGFS) += hci_debugfs.o
> > bluetooth-$(CONFIG_BT_SELFTEST) += selftest.o
> > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> > new file mode 100644
> > index 000000000000..7609932c48ca
> > --- /dev/null
> > +++ b/net/bluetooth/msft.c
> > @@ -0,0 +1,16 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/* Copyright (C) 2020 Google Corporation */
> > +
> > +#include <net/bluetooth/bluetooth.h>
> > +#include <net/bluetooth/hci_core.h>
> > +
> > +#include "msft.h"
> > +
> > +void msft_set_opcode(struct hci_dev *hdev, __u16 opcode)
> > +{
> > +     hdev->msft_opcode = opcode;
> > +
> > +     bt_dev_info(hdev, "Enabling MSFT extensions with opcode 0x%2.2x",
> > +                 hdev->msft_opcode);
> > +}
> > +EXPORT_SYMBOL(msft_set_opcode);
> > diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
> > new file mode 100644
> > index 000000000000..7218ea759dde
> > --- /dev/null
> > +++ b/net/bluetooth/msft.h
> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/* Copyright (C) 2020 Google Corporation */
> > +
> > +#ifndef __MSFT_H
> > +#define __MSFT_H
> > +
> > +#include <net/bluetooth/hci_core.h>
> > +
> > +#if IS_ENABLED(CONFIG_BT_MSFTEXT)
> > +
> > +void msft_set_opcode(struct hci_dev *hdev, __u16 opcode);
> > +
> > +#else
> > +
> > +static inline void msft_set_opcode(struct hci_dev *hdev, __u16 opcode) {}
> > +
> > +#endif
> > +
> > +#endif /* __MSFT_H*/
Thanks,
Miao

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

[Miao-chen Chou]

Hi Marcel,

On Mon, Mar 30, 2020 at 3:08 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Miao-chen,
>
> > This defines opcode and packet structures of Microsoft vendor extension.
> > For now, we add only the HCI_VS_MSFT_Read_Supported_Features command. 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: Marcel Holtmann <marcel@holtmann.org>
> >
> > Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
> > ---
> >
> > Changes in v4:
> > - Move MSFT's do_open() and do_close() from net/bluetooth/hci_core.c to
> > net/bluetooth/msft.c.
> > - Other than msft opcode, define struct msft_data to host the rest of
> > information of Microsoft extension and leave a void* pointing to a
> > msft_data in struct hci_dev.
> >
> > Changes in v3:
> > - Introduce msft_vnd_ext_do_open() and msft_vnd_ext_do_close().
> >
> > Changes in v2:
> > - Issue a HCI_VS_MSFT_Read_Supported_Features command with
> > __hci_cmd_sync() instead of constructing a request.
> >
> > include/net/bluetooth/hci_core.h |   1 +
> > net/bluetooth/hci_core.c         |   5 ++
> > net/bluetooth/hci_event.c        |   5 ++
> > net/bluetooth/msft.c             | 126 +++++++++++++++++++++++++++++++
> > net/bluetooth/msft.h             |  10 +++
> > 5 files changed, 147 insertions(+)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 239cae2d9998..59ddcd3a52cc 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -486,6 +486,7 @@ struct hci_dev {
> >
> > #if IS_ENABLED(CONFIG_BT_MSFTEXT)
> >       __u16                   msft_opcode;
> > +     void                    *msft_data;
> > #endif
> >
> >       int (*open)(struct hci_dev *hdev);
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index dbd2ad3a26ed..c38707de767a 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.h"
> >
> > static void hci_rx_work(struct work_struct *work);
> > static void hci_cmd_work(struct work_struct *work);
> > @@ -1563,6 +1564,8 @@ static int hci_dev_do_open(struct hci_dev *hdev)
> >           hci_dev_test_flag(hdev, HCI_VENDOR_DIAG) && hdev->set_diag)
> >               ret = hdev->set_diag(hdev, true);
> >
> > +     msft_do_open(hdev);
> > +
> >       clear_bit(HCI_INIT, &hdev->flags);
> >
> >       if (!ret) {
> > @@ -1758,6 +1761,8 @@ int hci_dev_do_close(struct hci_dev *hdev)
> >
> >       hci_sock_dev_event(hdev, HCI_DEV_DOWN);
> >
> > +     msft_do_close(hdev);
> > +
> >       if (hdev->flush)
> >               hdev->flush(hdev);
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 20408d386268..42b5871151a6 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -35,6 +35,7 @@
> > #include "a2mp.h"
> > #include "amp.h"
> > #include "smp.h"
> > +#include "msft.h"
> >
> > #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
> >                "\x00\x00\x00\x00\x00\x00\x00\x00"
> > @@ -6144,6 +6145,10 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
> >               hci_num_comp_blocks_evt(hdev, skb);
> >               break;
> >
> > +     case HCI_EV_VENDOR:
> > +             msft_vendor_evt(hdev, skb);
> > +             break;
> > +
> >       default:
> >               BT_DBG("%s event 0x%2.2x", hdev->name, event);
> >               break;
> > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> > index 7609932c48ca..f76e4c79556e 100644
> > --- a/net/bluetooth/msft.c
> > +++ b/net/bluetooth/msft.c
> > @@ -6,6 +6,24 @@
> >
> > #include "msft.h"
> >
> > +#define MSFT_OP_READ_SUPPORTED_FEATURES              0x00
> > +struct msft_cp_read_supported_features {
> > +     __u8   sub_opcode;
> > +} __packed;
> > +struct msft_rp_read_supported_features {
> > +     __u8   status;
> > +     __u8   sub_opcode;
> > +     __le64 features;
> > +     __u8   evt_prefix_len;
> > +     __u8   evt_prefix[0];
> > +} __packed;
> > +
> > +struct msft_data {
> > +     __u64 features;
> > +     __u8  evt_prefix_len;
> > +     __u8  *evt_prefix;
> > +};
> > +
> > void msft_set_opcode(struct hci_dev *hdev, __u16 opcode)
> > {
> >       hdev->msft_opcode = opcode;
> > @@ -14,3 +32,111 @@ void msft_set_opcode(struct hci_dev *hdev, __u16 opcode)
> >                   hdev->msft_opcode);
> > }
> > EXPORT_SYMBOL(msft_set_opcode);
> > +
> > +static struct msft_data *read_supported_features(struct hci_dev *hdev)
> > +{
> > +     struct msft_data *msft;
>
> I used a second parameter, but yes, my initial code was totally flawed with the msft_data access.
Ack.
>
> > +     struct msft_cp_read_supported_features cp;
> > +     struct msft_rp_read_supported_features *rp;
> > +     struct sk_buff *skb;
> > +
> > +     cp.sub_opcode = MSFT_OP_READ_SUPPORTED_FEATURES;
> > +
> > +     skb = __hci_cmd_sync(hdev, hdev->msft_opcode, sizeof(cp), &cp,
> > +                          HCI_CMD_TIMEOUT);
> > +     if (IS_ERR(skb)) {
> > +             bt_dev_err(hdev, "Failed to read MSFT supported features (%ld)",
> > +                        PTR_ERR(skb));
> > +             return NULL;
> > +     }
> > +
> > +     if (skb->len < sizeof(*rp)) {
> > +             bt_dev_err(hdev, "MSFT supported features length mismatch");
> > +             goto failed;
> > +     }
> > +
> > +     rp = (struct msft_rp_read_supported_features *)skb->data;
> > +
> > +     if (rp->sub_opcode != MSFT_OP_READ_SUPPORTED_FEATURES)
> > +             goto failed;
> > +
> > +     msft = kzalloc(sizeof(*msft), GFP_KERNEL);
> > +     if (!msft)
> > +             goto failed;
> > +
> > +     if (rp->evt_prefix_len > 0) {
> > +             msft->evt_prefix = kmemdup(rp->evt_prefix, rp->evt_prefix_len,
> > +                                        GFP_KERNEL);
> > +             if (!msft->evt_prefix)
> > +                     goto failed;
> > +     }
> > +
> > +     msft->evt_prefix_len = rp->evt_prefix_len;
> > +     msft->features = __le64_to_cpu(rp->features);
> > +     kfree_skb(skb);
> > +
> > +     bt_dev_info(hdev, "MSFT supported features %llx", msft->features);
> > +     return msft;
> > +
> > +failed:
> > +     kfree_skb(skb);
> > +     return NULL;
> > +}
> > +
> > +void msft_do_open(struct hci_dev *hdev)
> > +{
> > +     if (hdev->msft_opcode == HCI_OP_NOP)
> > +             return;
> > +
> > +     bt_dev_dbg(hdev, "Initialize MSFT extension");
> > +     hdev->msft_data = read_supported_features(hdev);
> > +}
> > +
> > +void msft_do_close(struct hci_dev *hdev)
> > +{
> > +     struct msft_data *msft = hdev->msft_data;
> > +
> > +     if (!msft)
> > +             return;
> > +
> > +     bt_dev_dbg(hdev, "Cleanup of MSFT extension");
> > +
> > +     hdev->msft_data = NULL;
> > +
> > +     kfree(msft->evt_prefix);
> > +     kfree(msft);
> > +}
> > +
> > +int msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
>
> So this was on purpose void. There is no point in returning any feedback from this function. It either handles the event or it doesn’t. The caller function doesn’t care.
I was thinking that if there are two extensions, the vendor events
should be processed either msft or the other function. Therefore,
should we use the return value to determine whether to hand skb to the
other function?
>
> > +     struct msft_data *msft = hdev->msft_data;
> > +     u8 event;
> > +
> > +     if (!msft)
> > +             return -ENOSYS;
> > +
> > +     /* When the extension has defined an event prefix, check that it
> > +      * matches, and otherwise just return.
> > +      */
> > +     if (msft->evt_prefix_len > 0) {
> > +             if (skb->len < msft->evt_prefix_len)
> > +                     return -ENOSYS;
> > +
> > +             if (memcmp(skb->data, msft->evt_prefix, msft->evt_prefix_len))
> > +                     return -ENOSYS;
> > +
> > +             skb_pull(skb, msft->evt_prefix_len);
> > +     }
> > +
> > +     /* Every event starts at least with an event code and the rest of
> > +      * the data is variable and depends on the event code. Returns true
> > +      */
> > +     if (skb->len < 1)
> > +             return -EBADMSG;
> > +
> > +     event = *skb->data;
> > +     skb_pull(skb, 1);
> > +
> > +     bt_dev_dbg(hdev, "MSFT vendor event %u", event);
> > +     return 0;
> > +}
> > diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
> > index 7218ea759dde..6a7d0ac6c66c 100644
> > --- a/net/bluetooth/msft.h
> > +++ b/net/bluetooth/msft.h
> > @@ -4,15 +4,25 @@
> > #ifndef __MSFT_H
> > #define __MSFT_H
> >
> > +#include <linux/errno.h>
> > #include <net/bluetooth/hci_core.h>
> >
> > #if IS_ENABLED(CONFIG_BT_MSFTEXT)
> >
> > void msft_set_opcode(struct hci_dev *hdev, __u16 opcode);
> > +void msft_do_open(struct hci_dev *hdev);
> > +void msft_do_close(struct hci_dev *hdev);
> > +int msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb);
> >
> > #else
> >
> > static inline void msft_set_opcode(struct hci_dev *hdev, __u16 opcode) {}
> > +static inline void msft_do_open(struct hci_dev *hdev) {}
> > +static inline void msft_do_close(struct hci_dev *hdev) {}
> > +static inline int msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > +     return -ENOSYS;
> > +}
> >
> > #endif
Thanks,
Miao

Re: [PATCH v4 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 also add a kernel config, BT_MSFTEXT, and a
>>> source file to facilitate Microsoft vendor extension functions.
>>> This was verified with Intel ThunderPeak BT controller
>>> where msft_vnd_ext_opcode is 0xFC1E.
>>> 
>>> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
>>> 
>>> Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
>>> ---
>>> 
>>> Changes in v4:
>>> - Introduce CONFIG_BT_MSFTEXT as a starting point of providing a
>>> framework to use Microsoft extension
>>> - Create include/net/bluetooth/msft.h and net/bluetooth/msft.c to
>>> facilitate functions of Microsoft extension.
>>> 
>>> 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().
>>> 
>>> 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        | 11 +++++++++--
>>> include/net/bluetooth/hci_core.h |  4 ++++
>> 
>> so I don’t like the intermixing of core features and drivers unless it is needed. In this case it is not needed since we can first introduce the core support and then enable the driver to use it.
> I will make btusb changes as a different commit in v5.

check the series that I posted. I tested them on ThunderPeak and if it also works, we use that as a base and then go from there.

>> 
>>> net/bluetooth/Kconfig            |  9 ++++++++-
>>> net/bluetooth/Makefile           |  1 +
>>> net/bluetooth/msft.c             | 16 ++++++++++++++++
>>> net/bluetooth/msft.h             | 19 +++++++++++++++++++
>>> 6 files changed, 57 insertions(+), 3 deletions(-)
>>> create mode 100644 net/bluetooth/msft.c
>>> create mode 100644 net/bluetooth/msft.h
>>> 
>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>> index 3bdec42c9612..0fe47708d3c8 100644
>>> --- a/drivers/bluetooth/btusb.c
>>> +++ b/drivers/bluetooth/btusb.c
>>> @@ -21,6 +21,7 @@
>>> #include <net/bluetooth/bluetooth.h>
>>> #include <net/bluetooth/hci_core.h>
>>> 
>>> +#include "../../net/bluetooth/msft.h"
>> 
>> This was my bad. I didn’t realized that drivers need to the set the opcode and not the core. I updated the patches to fix this.
> I will move it to include/net/bluetooth/.

I put it in hci_core.h since don’t want to add any extra needed include for driver. They are big enough already and adding more files doesn’t really help.

Regards

Marcel

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

[Marcel Holtmann]

Hi Miao-chen,

>>> This defines opcode and packet structures of Microsoft vendor extension.
>>> For now, we add only the HCI_VS_MSFT_Read_Supported_Features command. 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: Marcel Holtmann <marcel@holtmann.org>
>>> 
>>> Signed-off-by: Miao-chen Chou <mcchou@chromium.org>
>>> ---
>>> 
>>> Changes in v4:
>>> - Move MSFT's do_open() and do_close() from net/bluetooth/hci_core.c to
>>> net/bluetooth/msft.c.
>>> - Other than msft opcode, define struct msft_data to host the rest of
>>> information of Microsoft extension and leave a void* pointing to a
>>> msft_data in struct hci_dev.
>>> 
>>> Changes in v3:
>>> - Introduce msft_vnd_ext_do_open() and msft_vnd_ext_do_close().
>>> 
>>> Changes in v2:
>>> - Issue a HCI_VS_MSFT_Read_Supported_Features command with
>>> __hci_cmd_sync() instead of constructing a request.
>>> 
>>> include/net/bluetooth/hci_core.h |   1 +
>>> net/bluetooth/hci_core.c         |   5 ++
>>> net/bluetooth/hci_event.c        |   5 ++
>>> net/bluetooth/msft.c             | 126 +++++++++++++++++++++++++++++++
>>> net/bluetooth/msft.h             |  10 +++
>>> 5 files changed, 147 insertions(+)
>>> 
>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>> index 239cae2d9998..59ddcd3a52cc 100644
>>> --- a/include/net/bluetooth/hci_core.h
>>> +++ b/include/net/bluetooth/hci_core.h
>>> @@ -486,6 +486,7 @@ struct hci_dev {
>>> 
>>> #if IS_ENABLED(CONFIG_BT_MSFTEXT)
>>>      __u16                   msft_opcode;
>>> +     void                    *msft_data;
>>> #endif
>>> 
>>>      int (*open)(struct hci_dev *hdev);
>>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>>> index dbd2ad3a26ed..c38707de767a 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.h"
>>> 
>>> static void hci_rx_work(struct work_struct *work);
>>> static void hci_cmd_work(struct work_struct *work);
>>> @@ -1563,6 +1564,8 @@ static int hci_dev_do_open(struct hci_dev *hdev)
>>>          hci_dev_test_flag(hdev, HCI_VENDOR_DIAG) && hdev->set_diag)
>>>              ret = hdev->set_diag(hdev, true);
>>> 
>>> +     msft_do_open(hdev);
>>> +
>>>      clear_bit(HCI_INIT, &hdev->flags);
>>> 
>>>      if (!ret) {
>>> @@ -1758,6 +1761,8 @@ int hci_dev_do_close(struct hci_dev *hdev)
>>> 
>>>      hci_sock_dev_event(hdev, HCI_DEV_DOWN);
>>> 
>>> +     msft_do_close(hdev);
>>> +
>>>      if (hdev->flush)
>>>              hdev->flush(hdev);
>>> 
>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>> index 20408d386268..42b5871151a6 100644
>>> --- a/net/bluetooth/hci_event.c
>>> +++ b/net/bluetooth/hci_event.c
>>> @@ -35,6 +35,7 @@
>>> #include "a2mp.h"
>>> #include "amp.h"
>>> #include "smp.h"
>>> +#include "msft.h"
>>> 
>>> #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
>>>               "\x00\x00\x00\x00\x00\x00\x00\x00"
>>> @@ -6144,6 +6145,10 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
>>>              hci_num_comp_blocks_evt(hdev, skb);
>>>              break;
>>> 
>>> +     case HCI_EV_VENDOR:
>>> +             msft_vendor_evt(hdev, skb);
>>> +             break;
>>> +
>>>      default:
>>>              BT_DBG("%s event 0x%2.2x", hdev->name, event);
>>>              break;
>>> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
>>> index 7609932c48ca..f76e4c79556e 100644
>>> --- a/net/bluetooth/msft.c
>>> +++ b/net/bluetooth/msft.c
>>> @@ -6,6 +6,24 @@
>>> 
>>> #include "msft.h"
>>> 
>>> +#define MSFT_OP_READ_SUPPORTED_FEATURES              0x00
>>> +struct msft_cp_read_supported_features {
>>> +     __u8   sub_opcode;
>>> +} __packed;
>>> +struct msft_rp_read_supported_features {
>>> +     __u8   status;
>>> +     __u8   sub_opcode;
>>> +     __le64 features;
>>> +     __u8   evt_prefix_len;
>>> +     __u8   evt_prefix[0];
>>> +} __packed;
>>> +
>>> +struct msft_data {
>>> +     __u64 features;
>>> +     __u8  evt_prefix_len;
>>> +     __u8  *evt_prefix;
>>> +};
>>> +
>>> void msft_set_opcode(struct hci_dev *hdev, __u16 opcode)
>>> {
>>>      hdev->msft_opcode = opcode;
>>> @@ -14,3 +32,111 @@ void msft_set_opcode(struct hci_dev *hdev, __u16 opcode)
>>>                  hdev->msft_opcode);
>>> }
>>> EXPORT_SYMBOL(msft_set_opcode);
>>> +
>>> +static struct msft_data *read_supported_features(struct hci_dev *hdev)
>>> +{
>>> +     struct msft_data *msft;
>> 
>> I used a second parameter, but yes, my initial code was totally flawed with the msft_data access.
> Ack.
>> 
>>> +     struct msft_cp_read_supported_features cp;
>>> +     struct msft_rp_read_supported_features *rp;
>>> +     struct sk_buff *skb;
>>> +
>>> +     cp.sub_opcode = MSFT_OP_READ_SUPPORTED_FEATURES;
>>> +
>>> +     skb = __hci_cmd_sync(hdev, hdev->msft_opcode, sizeof(cp), &cp,
>>> +                          HCI_CMD_TIMEOUT);
>>> +     if (IS_ERR(skb)) {
>>> +             bt_dev_err(hdev, "Failed to read MSFT supported features (%ld)",
>>> +                        PTR_ERR(skb));
>>> +             return NULL;
>>> +     }
>>> +
>>> +     if (skb->len < sizeof(*rp)) {
>>> +             bt_dev_err(hdev, "MSFT supported features length mismatch");
>>> +             goto failed;
>>> +     }
>>> +
>>> +     rp = (struct msft_rp_read_supported_features *)skb->data;
>>> +
>>> +     if (rp->sub_opcode != MSFT_OP_READ_SUPPORTED_FEATURES)
>>> +             goto failed;
>>> +
>>> +     msft = kzalloc(sizeof(*msft), GFP_KERNEL);
>>> +     if (!msft)
>>> +             goto failed;
>>> +
>>> +     if (rp->evt_prefix_len > 0) {
>>> +             msft->evt_prefix = kmemdup(rp->evt_prefix, rp->evt_prefix_len,
>>> +                                        GFP_KERNEL);
>>> +             if (!msft->evt_prefix)
>>> +                     goto failed;
>>> +     }
>>> +
>>> +     msft->evt_prefix_len = rp->evt_prefix_len;
>>> +     msft->features = __le64_to_cpu(rp->features);
>>> +     kfree_skb(skb);
>>> +
>>> +     bt_dev_info(hdev, "MSFT supported features %llx", msft->features);
>>> +     return msft;
>>> +
>>> +failed:
>>> +     kfree_skb(skb);
>>> +     return NULL;
>>> +}
>>> +
>>> +void msft_do_open(struct hci_dev *hdev)
>>> +{
>>> +     if (hdev->msft_opcode == HCI_OP_NOP)
>>> +             return;
>>> +
>>> +     bt_dev_dbg(hdev, "Initialize MSFT extension");
>>> +     hdev->msft_data = read_supported_features(hdev);
>>> +}
>>> +
>>> +void msft_do_close(struct hci_dev *hdev)
>>> +{
>>> +     struct msft_data *msft = hdev->msft_data;
>>> +
>>> +     if (!msft)
>>> +             return;
>>> +
>>> +     bt_dev_dbg(hdev, "Cleanup of MSFT extension");
>>> +
>>> +     hdev->msft_data = NULL;
>>> +
>>> +     kfree(msft->evt_prefix);
>>> +     kfree(msft);
>>> +}
>>> +
>>> +int msft_vendor_evt(struct hci_dev *hdev, struct sk_buff *skb)
>>> +{
>> 
>> So this was on purpose void. There is no point in returning any feedback from this function. It either handles the event or it doesn’t. The caller function doesn’t care.
> I was thinking that if there are two extensions, the vendor events
> should be processed either msft or the other function. Therefore,
> should we use the return value to determine whether to hand skb to the
> other function?

my thinking was that we just hand the vendor events to all functions. Let them deal with the details. I would not over-design this right now. Keep it simple. As long as it is not userspace facing API, we can easily change that when we need it.

Regards

Marcel