[PATCH v1 0/3] This series allows a higher level HFP implementation to enable wbs.

[PATCH v1 0/3] This series allows a higher level HFP implementation to enable wbs.

[Alain Michaud]

In particular this series :

1. It allows the driver to indicate support for wide band speech.
2. Use a quirk tied to a MGMT local controller settings allowing a
higherlevel HFP implementation to check if the local controller supports
wide band speech.
3. When the controller reports support for wide band speech, enable
erroneous data reporting to allow a higher level PLC to deal with
erroneous or missing packets.


Alain Michaud (3):
  bluetooth: Add flag to define wideband speech capability
  bluetooth: Support querying for WBS support through MGMT
  bluetooth: Enable erroneous data reporting if wbs is supported

 drivers/bluetooth/btusb.c    | 22 ++++++++++++++++------
 include/net/bluetooth/hci.h  | 16 ++++++++++++++++
 include/net/bluetooth/mgmt.h |  1 +
 net/bluetooth/hci_core.c     | 33 +++++++++++++++++++++++++++++++++
 net/bluetooth/mgmt.c         |  4 ++++
 5 files changed, 70 insertions(+), 6 deletions(-)

-- 
2.25.1.481.gfbce0eb801-goog

[PATCH v1 1/3] bluetooth: Add flag to define wideband speech capability

[Alain Michaud]

This change adds a new flag to define a controller's wideband speech
capability.  This is required since no reliable over HCI mechanism
exists to query the controller and driver's compatibility with
wideband speech.

Signed-off-by: Alain Michaud <alainm@chromium.org>
---

 drivers/bluetooth/btusb.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index f5924f3e8b8d..1b175ba3085c 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -57,6 +57,7 @@ static struct usb_driver btusb_driver;
 #define BTUSB_IFNUM_2		0x80000
 #define BTUSB_CW6622		0x100000
 #define BTUSB_MEDIATEK		0x200000
+#define BTUSB_WIDEBAND_SPEECH	0x400000
 
 static const struct usb_device_id btusb_table[] = {
 	/* Generic Bluetooth USB device */
@@ -333,15 +334,21 @@ static const struct usb_device_id blacklist_table[] = {
 	{ USB_DEVICE(0x1286, 0x204e), .driver_info = BTUSB_MARVELL },
 
 	/* Intel Bluetooth devices */
-	{ USB_DEVICE(0x8087, 0x0025), .driver_info = BTUSB_INTEL_NEW },
-	{ USB_DEVICE(0x8087, 0x0026), .driver_info = BTUSB_INTEL_NEW },
-	{ USB_DEVICE(0x8087, 0x0029), .driver_info = BTUSB_INTEL_NEW },
+	{ USB_DEVICE(0x8087, 0x0025), .driver_info = BTUSB_INTEL_NEW |
+						     BTUSB_WIDEBAND_SPEECH },
+	{ USB_DEVICE(0x8087, 0x0026), .driver_info = BTUSB_INTEL_NEW |
+						     BTUSB_WIDEBAND_SPEECH },
+	{ USB_DEVICE(0x8087, 0x0029), .driver_info = BTUSB_INTEL_NEW |
+						     BTUSB_WIDEBAND_SPEECH },
 	{ USB_DEVICE(0x8087, 0x07da), .driver_info = BTUSB_CSR },
 	{ USB_DEVICE(0x8087, 0x07dc), .driver_info = BTUSB_INTEL },
 	{ USB_DEVICE(0x8087, 0x0a2a), .driver_info = BTUSB_INTEL },
-	{ USB_DEVICE(0x8087, 0x0a2b), .driver_info = BTUSB_INTEL_NEW },
-	{ USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL },
-	{ USB_DEVICE(0x8087, 0x0aaa), .driver_info = BTUSB_INTEL_NEW },
+	{ USB_DEVICE(0x8087, 0x0a2b), .driver_info = BTUSB_INTEL_NEW |
+						     BTUSB_WIDEBAND_SPEECH },
+	{ USB_DEVICE(0x8087, 0x0aa7), .driver_info = BTUSB_INTEL |
+						     BTUSB_WIDEBAND_SPEECH },
+	{ USB_DEVICE(0x8087, 0x0aaa), .driver_info = BTUSB_INTEL_NEW |
+						     BTUSB_WIDEBAND_SPEECH },
 
 	/* Other Intel Bluetooth devices */
 	{ USB_VENDOR_AND_INTERFACE_INFO(0x8087, 0xe0, 0x01, 0x01),
-- 
2.25.1.481.gfbce0eb801-goog

Re: [PATCH v1 1/3] bluetooth: Add flag to define wideband speech capability

[Marcel Holtmann]

Hi Alain,

> This change adds a new flag to define a controller's wideband speech
> capability.  This is required since no reliable over HCI mechanism
> exists to query the controller and driver's compatibility with
> wideband speech.
> 
> Signed-off-by: Alain Michaud <alainm@chromium.org>
> ---
> 
> drivers/bluetooth/btusb.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)

patch has been applied to bluetooth-next tree.

Regards

Marcel

[PATCH v1 2/3] bluetooth: Support querying for WBS support through MGMT

[Alain Michaud]

This patch provides a mechanism for MGMT interface client to query the
capability of the controller to support WBS.

Signed-off-by: Alain Michaud <alainm@chromium.org>
---

 drivers/bluetooth/btusb.c    | 3 +++
 include/net/bluetooth/hci.h  | 9 +++++++++
 include/net/bluetooth/mgmt.h | 1 +
 net/bluetooth/mgmt.c         | 4 ++++
 4 files changed, 17 insertions(+)

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 1b175ba3085c..95613f103d17 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3866,6 +3866,9 @@ static int btusb_probe(struct usb_interface *intf,
 	if (id->driver_info & BTUSB_BROKEN_ISOC)
 		data->isoc = NULL;
 
+	if (id->driver_info & BTUSB_WIDEBAND_SPEECH)
+		set_bit(HCI_QUIRK_WIDE_BAND_SPEECH_SUPPORTED, &hdev->quirks);
+
 	if (id->driver_info & BTUSB_DIGIANSWER) {
 		data->cmdreq_type = USB_TYPE_VENDOR;
 		set_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks);
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 6293bdd7d862..b214c4358d1c 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -205,6 +205,15 @@ enum {
 	 *
 	 */
 	HCI_QUIRK_NON_PERSISTENT_SETUP,
+
+	/* When this quirt is set, wide band speech is supported by
+	 * the driver since no reliable mechanism exist to report
+	 * this from the hardware, a driver flag is use to convey
+	 * this support
+	 *
+	 * This quirk must be set before hci_register_dev is called.
+	 */
+	HCI_QUIRK_WIDE_BAND_SPEECH_SUPPORTED,
 };
 
 /* HCI device flags */
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index a90666af05bd..51339584c3a9 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -102,6 +102,7 @@ struct mgmt_rp_read_index_list {
 #define MGMT_SETTING_CONFIGURATION	0x00004000
 #define MGMT_SETTING_STATIC_ADDRESS	0x00008000
 #define MGMT_SETTING_PHY_CONFIGURATION  0x00010000
+#define MGMT_SETTING_WIDE_BAND_SPEECH	0x00020000
 
 #define MGMT_OP_READ_INFO		0x0004
 #define MGMT_READ_INFO_SIZE		0
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 0dc610faab70..a08691d679a8 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -762,6 +762,10 @@ static u32 get_supported_settings(struct hci_dev *hdev)
 
 		if (lmp_sc_capable(hdev))
 			settings |= MGMT_SETTING_SECURE_CONN;
+
+		if (test_bit(HCI_QUIRK_WIDE_BAND_SPEECH_SUPPORTED,
+			     &hdev->quirks))
+			settings |= MGMT_SETTING_WIDE_BAND_SPEECH;
 	}
 
 	if (lmp_le_capable(hdev)) {
-- 
2.25.1.481.gfbce0eb801-goog

Re: [PATCH v1 2/3] bluetooth: Support querying for WBS support through MGMT

[Marcel Holtmann]

Hi Alain,

> This patch provides a mechanism for MGMT interface client to query the
> capability of the controller to support WBS.
> 
> Signed-off-by: Alain Michaud <alainm@chromium.org>
> ---
> 
> drivers/bluetooth/btusb.c    | 3 +++
> include/net/bluetooth/hci.h  | 9 +++++++++
> include/net/bluetooth/mgmt.h | 1 +
> net/bluetooth/mgmt.c         | 4 ++++
> 4 files changed, 17 insertions(+)

patch has been applied to bluetooth-next tree.

However ..

> +
> +	/* When this quirt is set, wide band speech is supported by
> +	 * the driver since no reliable mechanism exist to report
> +	 * this from the hardware, a driver flag is use to convey
> +	 * this support
> +	 *
> +	 * This quirk must be set before hci_register_dev is called.
> +	 */
> +	HCI_QUIRK_WIDE_BAND_SPEECH_SUPPORTED,
> };

quirt - a short-handled riding whip with a braided leather lash.

.. this totally made my day and I was tempted to leave this typo in the code, but I fixed it up in the end ;)

Regards

Marcel

[PATCH v1 3/3] bluetooth: Enable erroneous data reporting if wbs is supported

[Alain Michaud]

This change will enable erroneous data reporting if wide band speech is
supported by the controller as indicated by the
HCI_QUIRK_WIDE_BAND_SPEECH_SUPPORTED quirk.

Signed-off-by: Alain Michaud <alainm@chromium.org>
---

 include/net/bluetooth/hci.h |  7 +++++++
 net/bluetooth/hci_core.c    | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index b214c4358d1c..807fd40bb358 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -1095,6 +1095,13 @@ struct hci_rp_read_inq_rsp_tx_power {
 	__s8     tx_power;
 } __packed;
 
+#define HCI_OP_WRITE_DEFAULT_ERRONEOUS_DATA_REPORTING	0x0c5b
+	#define ERRONEOUS_DATA_REPORTING_DISABLED	0x00
+	#define ERRONEOUS_DATA_REPORTING_ENABLED	0x01
+struct hci_cp_write_default_erroneous_data_reporting {
+	__u8     erroneous_data_reporting;
+} __packed;
+
 #define HCI_OP_SET_EVENT_MASK_PAGE_2	0x0c63
 
 #define HCI_OP_READ_LOCATION_DATA	0x0c64
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 1ca7508b6ca7..19afb85ad399 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1382,6 +1382,29 @@ static void hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
 	bacpy(&hdev->public_addr, &ba);
 }
 
+static int hci_write_default_erroneous_data_req(struct hci_request *req,
+						unsigned long opt)
+{
+	struct hci_cp_write_default_erroneous_data_reporting cp = {};
+
+	cp.erroneous_data_reporting = (__u8)opt;
+	hci_req_add(req, HCI_OP_WRITE_DEFAULT_ERRONEOUS_DATA_REPORTING,
+		    sizeof(cp), &cp);
+	return 0;
+}
+
+static void hci_write_default_erroneous_data(struct hci_dev *hdev,
+					     __u8 erroneous_data_reporting)
+{
+	int err = __hci_req_sync(hdev,
+				 hci_write_default_erroneous_data_req,
+				 erroneous_data_reporting, HCI_CMD_TIMEOUT,
+				 NULL);
+	if (err) {
+		BT_ERR("HCI_OP_WRITE_DEFAULT_ERRONEOUS_DATA_REPORTING failed");
+	}
+}
+
 static int hci_dev_do_open(struct hci_dev *hdev)
 {
 	int ret = 0;
@@ -1547,6 +1570,16 @@ static int hci_dev_do_open(struct hci_dev *hdev)
 		set_bit(HCI_UP, &hdev->flags);
 		hci_sock_dev_event(hdev, HCI_DEV_UP);
 		hci_leds_update_powered(hdev, true);
+
+		/* If the controller supports wide_band_speech, enable erroneous
+		 * data reporting.
+		 */
+		if (test_bit(HCI_QUIRK_WIDE_BAND_SPEECH_SUPPORTED,
+			     &hdev->quirks)) {
+			hci_write_default_erroneous_data(hdev,
+					      ERRONEOUS_DATA_REPORTING_ENABLED);
+		}
+
 		if (!hci_dev_test_flag(hdev, HCI_SETUP) &&
 		    !hci_dev_test_flag(hdev, HCI_CONFIG) &&
 		    !hci_dev_test_flag(hdev, HCI_UNCONFIGURED) &&
-- 
2.25.1.481.gfbce0eb801-goog

Re: [PATCH v1 3/3] bluetooth: Enable erroneous data reporting if wbs is supported

[Marcel Holtmann]

Hi Alain,

> This change will enable erroneous data reporting if wide band speech is
> supported by the controller as indicated by the
> HCI_QUIRK_WIDE_BAND_SPEECH_SUPPORTED quirk.
> 
> Signed-off-by: Alain Michaud <alainm@chromium.org>
> ---
> 
> include/net/bluetooth/hci.h |  7 +++++++
> net/bluetooth/hci_core.c    | 33 +++++++++++++++++++++++++++++++++
> 2 files changed, 40 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index b214c4358d1c..807fd40bb358 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -1095,6 +1095,13 @@ struct hci_rp_read_inq_rsp_tx_power {
> 	__s8     tx_power;
> } __packed;
> 
> +#define HCI_OP_WRITE_DEFAULT_ERRONEOUS_DATA_REPORTING	0x0c5b
> +	#define ERRONEOUS_DATA_REPORTING_DISABLED	0x00
> +	#define ERRONEOUS_DATA_REPORTING_ENABLED	0x01
> +struct hci_cp_write_default_erroneous_data_reporting {
> +	__u8     erroneous_data_reporting;
> +} __packed;
> +

So I realized that in monitor/bt.h I also used the full name, but inside the kernel code it feels way too long and creates harder to read code.

My proposal is HCI_OP_READ_DEF_ERR_DATA_REPORTING and HCI_OP_WRITE_DEF_ERR_DATA_REPORTING.

You might realize that I included the read command here as well. So first, we need to read the current value of the controller if supported and then also start tracking changes to it. You will find examples in hci_event.c and how that is done. Store the current value in hci_dev.

I also realized that we should only advertise Wideband speech support if this command is actually supported. So that you might need to fix as well.

> #define HCI_OP_SET_EVENT_MASK_PAGE_2	0x0c63
> 
> #define HCI_OP_READ_LOCATION_DATA	0x0c64
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 1ca7508b6ca7..19afb85ad399 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1382,6 +1382,29 @@ static void hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
> 	bacpy(&hdev->public_addr, &ba);
> }
> 
> +static int hci_write_default_erroneous_data_req(struct hci_request *req,
> +						unsigned long opt)
> +{
> +	struct hci_cp_write_default_erroneous_data_reporting cp = {};
> +
> +	cp.erroneous_data_reporting = (__u8)opt;
> +	hci_req_add(req, HCI_OP_WRITE_DEFAULT_ERRONEOUS_DATA_REPORTING,
> +		    sizeof(cp), &cp);
> +	return 0;
> +}
> +
> +static void hci_write_default_erroneous_data(struct hci_dev *hdev,
> +					     __u8 erroneous_data_reporting)
> +{
> +	int err = __hci_req_sync(hdev,
> +				 hci_write_default_erroneous_data_req,
> +				 erroneous_data_reporting, HCI_CMD_TIMEOUT,
> +				 NULL);
> +	if (err) {
> +		BT_ERR("HCI_OP_WRITE_DEFAULT_ERRONEOUS_DATA_REPORTING failed");
> +	}
> +}
> +
> static int hci_dev_do_open(struct hci_dev *hdev)
> {
> 	int ret = 0;
> @@ -1547,6 +1570,16 @@ static int hci_dev_do_open(struct hci_dev *hdev)
> 		set_bit(HCI_UP, &hdev->flags);
> 		hci_sock_dev_event(hdev, HCI_DEV_UP);
> 		hci_leds_update_powered(hdev, true);
> +
> +		/* If the controller supports wide_band_speech, enable erroneous
> +		 * data reporting.
> +		 */
> +		if (test_bit(HCI_QUIRK_WIDE_BAND_SPEECH_SUPPORTED,
> +			     &hdev->quirks)) {
> +			hci_write_default_erroneous_data(hdev,
> +					      ERRONEOUS_DATA_REPORTING_ENABLED);
> +		}
> +
> 		if (!hci_dev_test_flag(hdev, HCI_SETUP) &&
> 		    !hci_dev_test_flag(hdev, HCI_CONFIG) &&
> 		    !hci_dev_test_flag(hdev, HCI_UNCONFIGURED) &&

Don’t do this in hci_dev_do_open. You want to read the current value in the init procedure and change it if needed. This is similar to Secure Connections and other values that we only allow to change while the controller is down.

Regards

Marcel