linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 1/4] Bluetooth: aosp: Support AOSP Bluetooth Quality Report
@ 2021-09-26  7:07 Joseph Hwang
  2021-09-26  7:07 ` [PATCH v4 2/4] Bluetooth: hci_qca: enable Qualcomm WCN399x for AOSP extension Joseph Hwang
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Joseph Hwang @ 2021-09-26  7:07 UTC (permalink / raw)
  To: linux-bluetooth, marcel, luiz.dentz, pali
  Cc: josephsih, chromeos-bluetooth-upstreaming, Joseph Hwang,
	Miao-chen Chou, David S. Miller, Jakub Kicinski, Johan Hedberg,
	linux-kernel, netdev

This patch adds the support of the AOSP Bluetooth Quality Report
(BQR) events.

Multiple vendors have supported the AOSP Bluetooth Quality Report.
When a Bluetooth controller supports the capability, it can enable
the capability through hci_set_aosp_capable. Then hci_core will
set up the hdev->set_quality_report callback accordingly.

Note that Intel also supports a distinct telemetry quality report
specification. Intel sets up the hdev->set_quality_report callback
in the btusb driver module.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
Signed-off-by: Joseph Hwang <josephsih@chromium.org>

---

Changes in v4:
- Move the AOSP BQR support from the driver level to net/bluetooth/aosp.
- Fix the drivers to use hci_set_aosp_capable to enable aosp.
- Add Mediatek to support the capability too.

Changes in v3:
- Fix the auto build test ERROR
  "undefined symbol: btandroid_set_quality_report" that occurred
  with some kernel configs.
- Note that the mgmt-tester "Read Exp Feature - Success" failed.
  But on my test device, the same test passed. Please kindly let me
  know what may be going wrong. These patches do not actually
  modify read/set experimental features.
- As to CheckPatch failed. No need to modify the MAINTAINERS file.
  Thanks.

Changes in v2:
- Fix the titles of patches 2/3 and 3/3 and reduce their lengths.

 net/bluetooth/aosp.c     | 79 ++++++++++++++++++++++++++++++++++++++++
 net/bluetooth/aosp.h     |  7 ++++
 net/bluetooth/hci_core.c | 17 +++++++++
 3 files changed, 103 insertions(+)

diff --git a/net/bluetooth/aosp.c b/net/bluetooth/aosp.c
index a1b7762335a5..c2b22bc83fb2 100644
--- a/net/bluetooth/aosp.c
+++ b/net/bluetooth/aosp.c
@@ -33,3 +33,82 @@ void aosp_do_close(struct hci_dev *hdev)
 
 	bt_dev_dbg(hdev, "Cleanup of AOSP extension");
 }
+
+/* BQR command */
+#define BQR_OPCODE			hci_opcode_pack(0x3f, 0x015e)
+
+/* BQR report action */
+#define REPORT_ACTION_ADD		0x00
+#define REPORT_ACTION_DELETE		0x01
+#define REPORT_ACTION_CLEAR		0x02
+
+/* BQR event masks */
+#define QUALITY_MONITORING		BIT(0)
+#define APPRAOCHING_LSTO		BIT(1)
+#define A2DP_AUDIO_CHOPPY		BIT(2)
+#define SCO_VOICE_CHOPPY		BIT(3)
+
+#define DEFAULT_BQR_EVENT_MASK	(QUALITY_MONITORING | APPRAOCHING_LSTO | \
+				 A2DP_AUDIO_CHOPPY | SCO_VOICE_CHOPPY)
+
+/* Reporting at milliseconds so as not to stress the controller too much.
+ * Range: 0 ~ 65535 ms
+ */
+#define DEFALUT_REPORT_INTERVAL_MS	5000
+
+struct aosp_bqr_cp {
+	__u8	report_action;
+	__u32	event_mask;
+	__u16	min_report_interval;
+} __packed;
+
+static int enable_quality_report(struct hci_dev *hdev)
+{
+	struct sk_buff *skb;
+	struct aosp_bqr_cp cp;
+
+	cp.report_action = REPORT_ACTION_ADD;
+	cp.event_mask = DEFAULT_BQR_EVENT_MASK;
+	cp.min_report_interval = DEFALUT_REPORT_INTERVAL_MS;
+
+	skb = __hci_cmd_sync(hdev, BQR_OPCODE, sizeof(cp), &cp,
+			     HCI_CMD_TIMEOUT);
+	if (IS_ERR(skb)) {
+		bt_dev_err(hdev, "Enabling Android BQR failed (%ld)",
+			   PTR_ERR(skb));
+		return PTR_ERR(skb);
+	}
+
+	kfree_skb(skb);
+	return 0;
+}
+
+static int disable_quality_report(struct hci_dev *hdev)
+{
+	struct sk_buff *skb;
+	struct aosp_bqr_cp cp = { 0 };
+
+	cp.report_action = REPORT_ACTION_CLEAR;
+
+	skb = __hci_cmd_sync(hdev, BQR_OPCODE, sizeof(cp), &cp,
+			     HCI_CMD_TIMEOUT);
+	if (IS_ERR(skb)) {
+		bt_dev_err(hdev, "Disabling Android BQR failed (%ld)",
+			   PTR_ERR(skb));
+		return PTR_ERR(skb);
+	}
+
+	kfree_skb(skb);
+	return 0;
+}
+
+int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
+{
+	bt_dev_info(hdev, "quality report enable %d", enable);
+
+	/* Enable or disable the quality report feature. */
+	if (enable)
+		return enable_quality_report(hdev);
+	else
+		return disable_quality_report(hdev);
+}
diff --git a/net/bluetooth/aosp.h b/net/bluetooth/aosp.h
index 328fc6d39f70..384e111c1260 100644
--- a/net/bluetooth/aosp.h
+++ b/net/bluetooth/aosp.h
@@ -8,9 +8,16 @@
 void aosp_do_open(struct hci_dev *hdev);
 void aosp_do_close(struct hci_dev *hdev);
 
+int aosp_set_quality_report(struct hci_dev *hdev, bool enable);
+
 #else
 
 static inline void aosp_do_open(struct hci_dev *hdev) {}
 static inline void aosp_do_close(struct hci_dev *hdev) {}
 
+static inline int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
+{
+	return false;
+}
+
 #endif
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index aeec5a3031a6..a2c22a4921d4 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1315,6 +1315,21 @@ static void hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
 	bacpy(&hdev->public_addr, &ba);
 }
 
+static void hci_set_quality_report(struct hci_dev *hdev)
+{
+#ifdef CONFIG_BT_AOSPEXT
+	if (hdev->aosp_capable) {
+		/* The hdev->set_quality_report callback is setup here for
+		 * the vendors that support AOSP quality report specification.
+		 * Note that Intel, while supporting a distinct telemetry
+		 * quality report specification, sets up the
+		 * hdev->set_quality_report callback in the btusb module.
+		 */
+		hdev->set_quality_report = aosp_set_quality_report;
+	}
+#endif
+}
+
 static int hci_dev_do_open(struct hci_dev *hdev)
 {
 	int ret = 0;
@@ -1394,6 +1409,8 @@ static int hci_dev_do_open(struct hci_dev *hdev)
 		if (ret)
 			goto setup_failed;
 
+		hci_set_quality_report(hdev);
+
 		if (test_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks)) {
 			if (!bacmp(&hdev->public_addr, BDADDR_ANY))
 				hci_dev_get_bd_addr_from_property(hdev);
-- 
2.33.0.685.g46640cef36-goog


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v4 2/4] Bluetooth: hci_qca: enable Qualcomm WCN399x for AOSP extension
  2021-09-26  7:07 [PATCH v4 1/4] Bluetooth: aosp: Support AOSP Bluetooth Quality Report Joseph Hwang
@ 2021-09-26  7:07 ` Joseph Hwang
  2021-09-28 11:09   ` Marcel Holtmann
  2021-09-26  7:07 ` [PATCH v4 3/4] Bluetooth: btrtl: enable Realtek 8822C/8852A to support " Joseph Hwang
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Joseph Hwang @ 2021-09-26  7:07 UTC (permalink / raw)
  To: linux-bluetooth, marcel, luiz.dentz, pali
  Cc: josephsih, chromeos-bluetooth-upstreaming, Joseph Hwang,
	kernel test robot, Miao-chen Chou, Johan Hedberg, linux-kernel

This patch enables Qualcomm WCN399x to support the AOSP extension.

Reported-by: kernel test robot <lkp@intel.com>

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
Signed-off-by: Joseph Hwang <josephsih@chromium.org>

---

Changes in v4:
- Call hci_set_aosp_capable in the driver.

Changes in v3:
- Fix the auto build test ERROR
  "undefined symbol: btandroid_set_quality_report" that occurred
  with some kernel configs.

Changes in v2:
- Fix the title

 drivers/bluetooth/hci_qca.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 53deea2eb7b4..e2566d606c93 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -1730,6 +1730,7 @@ static int qca_setup(struct hci_uart *hu)
 	if (qca_is_wcn399x(soc_type) ||
 	    qca_is_wcn6750(soc_type)) {
 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
+		hci_set_aosp_capable(hdev);
 
 		ret = qca_read_soc_version(hdev, &ver, soc_type);
 		if (ret)
-- 
2.33.0.685.g46640cef36-goog


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v4 3/4] Bluetooth: btrtl: enable Realtek 8822C/8852A to support AOSP extension
  2021-09-26  7:07 [PATCH v4 1/4] Bluetooth: aosp: Support AOSP Bluetooth Quality Report Joseph Hwang
  2021-09-26  7:07 ` [PATCH v4 2/4] Bluetooth: hci_qca: enable Qualcomm WCN399x for AOSP extension Joseph Hwang
@ 2021-09-26  7:07 ` Joseph Hwang
  2021-09-28 11:09   ` Marcel Holtmann
  2021-09-26  7:07 ` [PATCH v4 4/4] Bluetooth: btusb: enable Mediatek MT7921 " Joseph Hwang
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Joseph Hwang @ 2021-09-26  7:07 UTC (permalink / raw)
  To: linux-bluetooth, marcel, luiz.dentz, pali
  Cc: josephsih, chromeos-bluetooth-upstreaming, Joseph Hwang,
	Miao-chen Chou, Johan Hedberg, linux-kernel

This patch enables Realtek 8822C and 8852A to support the AOSP
extension.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
Signed-off-by: Joseph Hwang <josephsih@chromium.org>

---

Changes in v4:
- Call hci_set_aosp_capable in the driver.

Changes in v3:
- Fix the auto build test ERROR
  "undefined symbol: btandroid_set_quality_report" that occurred
  with some kernel configs.

Changes in v2:
- Fix the title

 drivers/bluetooth/btrtl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 1f8afa0244d8..5a1090b7c69a 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -746,6 +746,7 @@ void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev)
 	case CHIP_ID_8852A:
 		set_bit(HCI_QUIRK_VALID_LE_STATES, &hdev->quirks);
 		set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, &hdev->quirks);
+		hci_set_aosp_capable(hdev);
 		break;
 	default:
 		rtl_dev_dbg(hdev, "Central-peripheral role not enabled.");
-- 
2.33.0.685.g46640cef36-goog


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v4 4/4] Bluetooth: btusb: enable Mediatek MT7921 to support AOSP extension
  2021-09-26  7:07 [PATCH v4 1/4] Bluetooth: aosp: Support AOSP Bluetooth Quality Report Joseph Hwang
  2021-09-26  7:07 ` [PATCH v4 2/4] Bluetooth: hci_qca: enable Qualcomm WCN399x for AOSP extension Joseph Hwang
  2021-09-26  7:07 ` [PATCH v4 3/4] Bluetooth: btrtl: enable Realtek 8822C/8852A to support " Joseph Hwang
@ 2021-09-26  7:07 ` Joseph Hwang
  2021-09-28 11:12   ` Marcel Holtmann
  2021-09-27 23:08 ` [v4,1/4] Bluetooth: aosp: Support AOSP Bluetooth Quality Report bluez.test.bot
  2021-09-28 10:44 ` [PATCH v4 1/4] " Marcel Holtmann
  4 siblings, 1 reply; 10+ messages in thread
From: Joseph Hwang @ 2021-09-26  7:07 UTC (permalink / raw)
  To: linux-bluetooth, marcel, luiz.dentz, pali
  Cc: josephsih, chromeos-bluetooth-upstreaming, Joseph Hwang,
	Miao-chen Chou, Johan Hedberg, linux-kernel

This patch enables Mediatek MT7921 to support the AOSP extension.

Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
Signed-off-by: Joseph Hwang <josephsih@chromium.org>

---

Changes in v4:
- Call hci_set_aosp_capable in the driver.
- This patch is added in this Series-changes 4.

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

diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index da85cc14f931..de0228e2245b 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -60,6 +60,7 @@ static struct usb_driver btusb_driver;
 #define BTUSB_VALID_LE_STATES   0x800000
 #define BTUSB_QCA_WCN6855	0x1000000
 #define BTUSB_INTEL_BROKEN_INITIAL_NCMD 0x4000000
+#define BTUSB_AOSP		0x8000000
 
 static const struct usb_device_id btusb_table[] = {
 	/* Generic Bluetooth USB device */
@@ -394,6 +395,7 @@ static const struct usb_device_id blacklist_table[] = {
 	/* MediaTek Bluetooth devices */
 	{ USB_VENDOR_AND_INTERFACE_INFO(0x0e8d, 0xe0, 0x01, 0x01),
 	  .driver_info = BTUSB_MEDIATEK |
+			 BTUSB_AOSP |
 			 BTUSB_WIDEBAND_SPEECH |
 			 BTUSB_VALID_LE_STATES },
 
@@ -407,6 +409,7 @@ static const struct usb_device_id blacklist_table[] = {
 
 	/* Additional MediaTek MT7921 Bluetooth devices */
 	{ USB_DEVICE(0x04ca, 0x3802), .driver_info = BTUSB_MEDIATEK |
+						     BTUSB_AOSP |
 						     BTUSB_WIDEBAND_SPEECH |
 						     BTUSB_VALID_LE_STATES },
 	{ USB_DEVICE(0x13d3, 0x3563), .driver_info = BTUSB_MEDIATEK |
@@ -3867,6 +3870,9 @@ static int btusb_probe(struct usb_interface *intf,
 		hdev->set_bdaddr = btusb_set_bdaddr_mtk;
 		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
 		data->recv_acl = btusb_recv_acl_mtk;
+
+		if (id->driver_info & BTUSB_AOSP)
+			hci_set_aosp_capable(hdev);
 	}
 
 	if (id->driver_info & BTUSB_SWAVE) {
-- 
2.33.0.685.g46640cef36-goog


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [v4,1/4] Bluetooth: aosp: Support AOSP Bluetooth Quality Report
  2021-09-26  7:07 [PATCH v4 1/4] Bluetooth: aosp: Support AOSP Bluetooth Quality Report Joseph Hwang
                   ` (2 preceding siblings ...)
  2021-09-26  7:07 ` [PATCH v4 4/4] Bluetooth: btusb: enable Mediatek MT7921 " Joseph Hwang
@ 2021-09-27 23:08 ` bluez.test.bot
  2021-09-28 10:44 ` [PATCH v4 1/4] " Marcel Holtmann
  4 siblings, 0 replies; 10+ messages in thread
From: bluez.test.bot @ 2021-09-27 23:08 UTC (permalink / raw)
  To: linux-bluetooth, josephsih

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=552941

---Test result---

Test Summary:
CheckPatch                    PASS      6.65 seconds
GitLint                       PASS      3.74 seconds
BuildKernel                   PASS      657.27 seconds
TestRunner: Setup             PASS      467.76 seconds
TestRunner: l2cap-tester      PASS      10.96 seconds
TestRunner: bnep-tester       PASS      5.44 seconds
TestRunner: mgmt-tester       PASS      86.03 seconds
TestRunner: rfcomm-tester     PASS      6.68 seconds
TestRunner: sco-tester        PASS      7.05 seconds
TestRunner: smp-tester        PASS      6.70 seconds
TestRunner: userchan-tester   PASS      5.67 seconds



---
Regards,
Linux Bluetooth


[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 51539 bytes --]

[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3906 bytes --]

[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 626719 bytes --]

[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 14762 bytes --]

[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 13924 bytes --]

[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11830 bytes --]

[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 7740 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 1/4] Bluetooth: aosp: Support AOSP Bluetooth Quality Report
  2021-09-26  7:07 [PATCH v4 1/4] Bluetooth: aosp: Support AOSP Bluetooth Quality Report Joseph Hwang
                   ` (3 preceding siblings ...)
  2021-09-27 23:08 ` [v4,1/4] Bluetooth: aosp: Support AOSP Bluetooth Quality Report bluez.test.bot
@ 2021-09-28 10:44 ` Marcel Holtmann
  2021-10-19 12:11   ` Joseph Hwang
  4 siblings, 1 reply; 10+ messages in thread
From: Marcel Holtmann @ 2021-09-28 10:44 UTC (permalink / raw)
  To: Joseph Hwang
  Cc: linux-bluetooth, Luiz Augusto von Dentz, Pali Rohár,
	Joseph Hwang, CrosBT Upstreaming, Miao-chen Chou,
	David S. Miller, Jakub Kicinski, Johan Hedberg, open list,
	netdev

Hi Joseph,

> This patch adds the support of the AOSP Bluetooth Quality Report
> (BQR) events.
> 
> Multiple vendors have supported the AOSP Bluetooth Quality Report.
> When a Bluetooth controller supports the capability, it can enable
> the capability through hci_set_aosp_capable. Then hci_core will
> set up the hdev->set_quality_report callback accordingly.
> 
> Note that Intel also supports a distinct telemetry quality report
> specification. Intel sets up the hdev->set_quality_report callback
> in the btusb driver module.
> 
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> Signed-off-by: Joseph Hwang <josephsih@chromium.org>
> 
> ---
> 
> Changes in v4:
> - Move the AOSP BQR support from the driver level to net/bluetooth/aosp.
> - Fix the drivers to use hci_set_aosp_capable to enable aosp.
> - Add Mediatek to support the capability too.
> 
> Changes in v3:
> - Fix the auto build test ERROR
>  "undefined symbol: btandroid_set_quality_report" that occurred
>  with some kernel configs.
> - Note that the mgmt-tester "Read Exp Feature - Success" failed.
>  But on my test device, the same test passed. Please kindly let me
>  know what may be going wrong. These patches do not actually
>  modify read/set experimental features.
> - As to CheckPatch failed. No need to modify the MAINTAINERS file.
>  Thanks.
> 
> Changes in v2:
> - Fix the titles of patches 2/3 and 3/3 and reduce their lengths.
> 
> net/bluetooth/aosp.c     | 79 ++++++++++++++++++++++++++++++++++++++++
> net/bluetooth/aosp.h     |  7 ++++
> net/bluetooth/hci_core.c | 17 +++++++++
> 3 files changed, 103 insertions(+)
> 
> diff --git a/net/bluetooth/aosp.c b/net/bluetooth/aosp.c
> index a1b7762335a5..c2b22bc83fb2 100644
> --- a/net/bluetooth/aosp.c
> +++ b/net/bluetooth/aosp.c
> @@ -33,3 +33,82 @@ void aosp_do_close(struct hci_dev *hdev)
> 
> 	bt_dev_dbg(hdev, "Cleanup of AOSP extension");
> }
> +
> +/* BQR command */
> +#define BQR_OPCODE			hci_opcode_pack(0x3f, 0x015e)
> +
> +/* BQR report action */
> +#define REPORT_ACTION_ADD		0x00
> +#define REPORT_ACTION_DELETE		0x01
> +#define REPORT_ACTION_CLEAR		0x02
> +
> +/* BQR event masks */
> +#define QUALITY_MONITORING		BIT(0)
> +#define APPRAOCHING_LSTO		BIT(1)
> +#define A2DP_AUDIO_CHOPPY		BIT(2)
> +#define SCO_VOICE_CHOPPY		BIT(3)
> +
> +#define DEFAULT_BQR_EVENT_MASK	(QUALITY_MONITORING | APPRAOCHING_LSTO | \
> +				 A2DP_AUDIO_CHOPPY | SCO_VOICE_CHOPPY)
> +
> +/* Reporting at milliseconds so as not to stress the controller too much.
> + * Range: 0 ~ 65535 ms
> + */
> +#define DEFALUT_REPORT_INTERVAL_MS	5000
> +
> +struct aosp_bqr_cp {
> +	__u8	report_action;
> +	__u32	event_mask;
> +	__u16	min_report_interval;
> +} __packed;
> +
> +static int enable_quality_report(struct hci_dev *hdev)
> +{
> +	struct sk_buff *skb;
> +	struct aosp_bqr_cp cp;
> +
> +	cp.report_action = REPORT_ACTION_ADD;
> +	cp.event_mask = DEFAULT_BQR_EVENT_MASK;
> +	cp.min_report_interval = DEFALUT_REPORT_INTERVAL_MS;
> +
> +	skb = __hci_cmd_sync(hdev, BQR_OPCODE, sizeof(cp), &cp,
> +			     HCI_CMD_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		bt_dev_err(hdev, "Enabling Android BQR failed (%ld)",
> +			   PTR_ERR(skb));
> +		return PTR_ERR(skb);
> +	}
> +
> +	kfree_skb(skb);
> +	return 0;
> +}
> +
> +static int disable_quality_report(struct hci_dev *hdev)
> +{
> +	struct sk_buff *skb;
> +	struct aosp_bqr_cp cp = { 0 };
> +
> +	cp.report_action = REPORT_ACTION_CLEAR;
> +
> +	skb = __hci_cmd_sync(hdev, BQR_OPCODE, sizeof(cp), &cp,
> +			     HCI_CMD_TIMEOUT);
> +	if (IS_ERR(skb)) {
> +		bt_dev_err(hdev, "Disabling Android BQR failed (%ld)",
> +			   PTR_ERR(skb));
> +		return PTR_ERR(skb);
> +	}
> +
> +	kfree_skb(skb);
> +	return 0;
> +}
> +
> +int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> +{
> +	bt_dev_info(hdev, "quality report enable %d", enable);
> +
> +	/* Enable or disable the quality report feature. */
> +	if (enable)
> +		return enable_quality_report(hdev);
> +	else
> +		return disable_quality_report(hdev);
> +}
> diff --git a/net/bluetooth/aosp.h b/net/bluetooth/aosp.h
> index 328fc6d39f70..384e111c1260 100644
> --- a/net/bluetooth/aosp.h
> +++ b/net/bluetooth/aosp.h
> @@ -8,9 +8,16 @@
> void aosp_do_open(struct hci_dev *hdev);
> void aosp_do_close(struct hci_dev *hdev);
> 
> +int aosp_set_quality_report(struct hci_dev *hdev, bool enable);
> +
> #else
> 
> static inline void aosp_do_open(struct hci_dev *hdev) {}
> static inline void aosp_do_close(struct hci_dev *hdev) {}
> 
> +static inline int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> +{
> +	return false;
> +}
> +
> #endif
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index aeec5a3031a6..a2c22a4921d4 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1315,6 +1315,21 @@ static void hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
> 	bacpy(&hdev->public_addr, &ba);
> }
> 
> +static void hci_set_quality_report(struct hci_dev *hdev)
> +{
> +#ifdef CONFIG_BT_AOSPEXT
> +	if (hdev->aosp_capable) {
> +		/* The hdev->set_quality_report callback is setup here for
> +		 * the vendors that support AOSP quality report specification.
> +		 * Note that Intel, while supporting a distinct telemetry
> +		 * quality report specification, sets up the
> +		 * hdev->set_quality_report callback in the btusb module.
> +		 */
> +		hdev->set_quality_report = aosp_set_quality_report;
> +	}
> +#endif
> +}
> +

I think that I wasn’t super clear in my review on how I wanted this feature. So hdev->set_quality_report should really only ever set by a transport driver. The core stack should never touch it.

So I wanted something like this:

	if (hdev->set_quality_report)
		err = hdev->set_quality_report(hdev, val);
	else
		err = aosp_set_quality_report(hdev, val);

I send a RFC showing you how I think this should be done.

An extra important step of course is to check if the Android extension actually supports the quality report feature in the first place.

And while writing that patch, I realized that your initial support has a mistake. I send a patch for fixing it. The mgmt document is pretty clear on how experimental flags are defined.

Read Experimental Features Information Command
==============================================

	Command Code:		0x0049
	Controller Index:	<controller id> or <non-controller>
	Command Parameters:
	Return Parameters:	Feature_Count (2 Octets)
				Feature1 {
					UUID (16 Octets)
					Flags (4 Octets)
				}
				Feature2 {  }
				...

	This command is used to retrieve the supported experimental features
	by the host stack.

	The UUID values are not defined here. They can change over time and
	are on purpose not stable. Features that mature will be removed at
	some point. The mapping of feature UUID to the actual functionality
	of a given feature is out of scope here.

	The following bits are defined for the Flags parameter:

		0	Feature active
		1	Causes change in supported settings

So please don’t just make up things and exp UUID should only be present if they are supported. If they are not supported because the hardware is lacking support, they should not be reported.

Regards

Marcel


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 2/4] Bluetooth: hci_qca: enable Qualcomm WCN399x for AOSP extension
  2021-09-26  7:07 ` [PATCH v4 2/4] Bluetooth: hci_qca: enable Qualcomm WCN399x for AOSP extension Joseph Hwang
@ 2021-09-28 11:09   ` Marcel Holtmann
  0 siblings, 0 replies; 10+ messages in thread
From: Marcel Holtmann @ 2021-09-28 11:09 UTC (permalink / raw)
  To: Joseph Hwang
  Cc: linux-bluetooth, Luiz Augusto von Dentz, Pali Rohár,
	josephsih, chromeos-bluetooth-upstreaming, kernel test robot,
	Miao-chen Chou, Johan Hedberg, linux-kernel

Hi Joseph,

> This patch enables Qualcomm WCN399x to support the AOSP extension.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> 
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> Signed-off-by: Joseph Hwang <josephsih@chromium.org>
> 
> ---
> 
> Changes in v4:
> - Call hci_set_aosp_capable in the driver.
> 
> Changes in v3:
> - Fix the auto build test ERROR
>  "undefined symbol: btandroid_set_quality_report" that occurred
>  with some kernel configs.
> 
> Changes in v2:
> - Fix the title
> 
> drivers/bluetooth/hci_qca.c | 1 +
> 1 file changed, 1 insertion(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 3/4] Bluetooth: btrtl: enable Realtek 8822C/8852A to support AOSP extension
  2021-09-26  7:07 ` [PATCH v4 3/4] Bluetooth: btrtl: enable Realtek 8822C/8852A to support " Joseph Hwang
@ 2021-09-28 11:09   ` Marcel Holtmann
  0 siblings, 0 replies; 10+ messages in thread
From: Marcel Holtmann @ 2021-09-28 11:09 UTC (permalink / raw)
  To: Joseph Hwang
  Cc: linux-bluetooth, Luiz Augusto von Dentz, Pali Rohár,
	Joseph Hwang, CrosBT Upstreaming, Miao-chen Chou, Johan Hedberg,
	linux-kernel

Hi Joseph,

> This patch enables Realtek 8822C and 8852A to support the AOSP
> extension.
> 
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> Signed-off-by: Joseph Hwang <josephsih@chromium.org>
> 
> ---
> 
> Changes in v4:
> - Call hci_set_aosp_capable in the driver.
> 
> Changes in v3:
> - Fix the auto build test ERROR
>  "undefined symbol: btandroid_set_quality_report" that occurred
>  with some kernel configs.
> 
> Changes in v2:
> - Fix the title
> 
> drivers/bluetooth/btrtl.c | 1 +
> 1 file changed, 1 insertion(+)

patch has been applied to bluetooth-next tree.

Regards

Marcel


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 4/4] Bluetooth: btusb: enable Mediatek MT7921 to support AOSP extension
  2021-09-26  7:07 ` [PATCH v4 4/4] Bluetooth: btusb: enable Mediatek MT7921 " Joseph Hwang
@ 2021-09-28 11:12   ` Marcel Holtmann
  0 siblings, 0 replies; 10+ messages in thread
From: Marcel Holtmann @ 2021-09-28 11:12 UTC (permalink / raw)
  To: Joseph Hwang
  Cc: linux-bluetooth, Luiz Augusto von Dentz, pali, josephsih,
	chromeos-bluetooth-upstreaming, Miao-chen Chou, Johan Hedberg,
	linux-kernel

Hi Joseph,

> This patch enables Mediatek MT7921 to support the AOSP extension.
> 
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> Signed-off-by: Joseph Hwang <josephsih@chromium.org>
> 
> ---
> 
> Changes in v4:
> - Call hci_set_aosp_capable in the driver.
> - This patch is added in this Series-changes 4.
> 
> drivers/bluetooth/btusb.c | 6 ++++++
> 1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index da85cc14f931..de0228e2245b 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -60,6 +60,7 @@ static struct usb_driver btusb_driver;
> #define BTUSB_VALID_LE_STATES   0x800000
> #define BTUSB_QCA_WCN6855	0x1000000
> #define BTUSB_INTEL_BROKEN_INITIAL_NCMD 0x4000000
> +#define BTUSB_AOSP		0x8000000
> 
> static const struct usb_device_id btusb_table[] = {
> 	/* Generic Bluetooth USB device */
> @@ -394,6 +395,7 @@ static const struct usb_device_id blacklist_table[] = {
> 	/* MediaTek Bluetooth devices */
> 	{ USB_VENDOR_AND_INTERFACE_INFO(0x0e8d, 0xe0, 0x01, 0x01),
> 	  .driver_info = BTUSB_MEDIATEK |
> +			 BTUSB_AOSP |
> 			 BTUSB_WIDEBAND_SPEECH |
> 			 BTUSB_VALID_LE_STATES },
> 
> @@ -407,6 +409,7 @@ static const struct usb_device_id blacklist_table[] = {
> 
> 	/* Additional MediaTek MT7921 Bluetooth devices */
> 	{ USB_DEVICE(0x04ca, 0x3802), .driver_info = BTUSB_MEDIATEK |
> +						     BTUSB_AOSP |
> 						     BTUSB_WIDEBAND_SPEECH |
> 						     BTUSB_VALID_LE_STATES },
> 	{ USB_DEVICE(0x13d3, 0x3563), .driver_info = BTUSB_MEDIATEK |
> @@ -3867,6 +3870,9 @@ static int btusb_probe(struct usb_interface *intf,
> 		hdev->set_bdaddr = btusb_set_bdaddr_mtk;
> 		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
> 		data->recv_acl = btusb_recv_acl_mtk;
> +
> +		if (id->driver_info & BTUSB_AOSP)
> +			hci_set_aosp_capable(hdev);
> 	}

so I don’t like this. Do we have Mediatek devices that don’t support the AOSP extensions and can’t we determine via some vendor command what features are supported? I do not want to clutter btusb.c any further. The vendor specific setup should be able to figure out what the hardware supports and what it doesn’t.

Regards

Marcel


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v4 1/4] Bluetooth: aosp: Support AOSP Bluetooth Quality Report
  2021-09-28 10:44 ` [PATCH v4 1/4] " Marcel Holtmann
@ 2021-10-19 12:11   ` Joseph Hwang
  0 siblings, 0 replies; 10+ messages in thread
From: Joseph Hwang @ 2021-10-19 12:11 UTC (permalink / raw)
  To: Marcel Holtmann
  Cc: linux-bluetooth, Luiz Augusto von Dentz, Pali Rohár,
	CrosBT Upstreaming, Miao-chen Chou, David S. Miller,
	Jakub Kicinski, Johan Hedberg, open list, netdev

Hi Marcel:

  Thanks for the RFC. I have fixed the patches per your request and
submitted them for re-review.

Thanks and regards!


On Tue, Sep 28, 2021 at 6:45 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Joseph,
>
> > This patch adds the support of the AOSP Bluetooth Quality Report
> > (BQR) events.
> >
> > Multiple vendors have supported the AOSP Bluetooth Quality Report.
> > When a Bluetooth controller supports the capability, it can enable
> > the capability through hci_set_aosp_capable. Then hci_core will
> > set up the hdev->set_quality_report callback accordingly.
> >
> > Note that Intel also supports a distinct telemetry quality report
> > specification. Intel sets up the hdev->set_quality_report callback
> > in the btusb driver module.
> >
> > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > Signed-off-by: Joseph Hwang <josephsih@chromium.org>
> >
> > ---
> >
> > Changes in v4:
> > - Move the AOSP BQR support from the driver level to net/bluetooth/aosp.
> > - Fix the drivers to use hci_set_aosp_capable to enable aosp.
> > - Add Mediatek to support the capability too.
> >
> > Changes in v3:
> > - Fix the auto build test ERROR
> >  "undefined symbol: btandroid_set_quality_report" that occurred
> >  with some kernel configs.
> > - Note that the mgmt-tester "Read Exp Feature - Success" failed.
> >  But on my test device, the same test passed. Please kindly let me
> >  know what may be going wrong. These patches do not actually
> >  modify read/set experimental features.
> > - As to CheckPatch failed. No need to modify the MAINTAINERS file.
> >  Thanks.
> >
> > Changes in v2:
> > - Fix the titles of patches 2/3 and 3/3 and reduce their lengths.
> >
> > net/bluetooth/aosp.c     | 79 ++++++++++++++++++++++++++++++++++++++++
> > net/bluetooth/aosp.h     |  7 ++++
> > net/bluetooth/hci_core.c | 17 +++++++++
> > 3 files changed, 103 insertions(+)
> >
> > diff --git a/net/bluetooth/aosp.c b/net/bluetooth/aosp.c
> > index a1b7762335a5..c2b22bc83fb2 100644
> > --- a/net/bluetooth/aosp.c
> > +++ b/net/bluetooth/aosp.c
> > @@ -33,3 +33,82 @@ void aosp_do_close(struct hci_dev *hdev)
> >
> >       bt_dev_dbg(hdev, "Cleanup of AOSP extension");
> > }
> > +
> > +/* BQR command */
> > +#define BQR_OPCODE                   hci_opcode_pack(0x3f, 0x015e)
> > +
> > +/* BQR report action */
> > +#define REPORT_ACTION_ADD            0x00
> > +#define REPORT_ACTION_DELETE         0x01
> > +#define REPORT_ACTION_CLEAR          0x02
> > +
> > +/* BQR event masks */
> > +#define QUALITY_MONITORING           BIT(0)
> > +#define APPRAOCHING_LSTO             BIT(1)
> > +#define A2DP_AUDIO_CHOPPY            BIT(2)
> > +#define SCO_VOICE_CHOPPY             BIT(3)
> > +
> > +#define DEFAULT_BQR_EVENT_MASK       (QUALITY_MONITORING | APPRAOCHING_LSTO | \
> > +                              A2DP_AUDIO_CHOPPY | SCO_VOICE_CHOPPY)
> > +
> > +/* Reporting at milliseconds so as not to stress the controller too much.
> > + * Range: 0 ~ 65535 ms
> > + */
> > +#define DEFALUT_REPORT_INTERVAL_MS   5000
> > +
> > +struct aosp_bqr_cp {
> > +     __u8    report_action;
> > +     __u32   event_mask;
> > +     __u16   min_report_interval;
> > +} __packed;
> > +
> > +static int enable_quality_report(struct hci_dev *hdev)
> > +{
> > +     struct sk_buff *skb;
> > +     struct aosp_bqr_cp cp;
> > +
> > +     cp.report_action = REPORT_ACTION_ADD;
> > +     cp.event_mask = DEFAULT_BQR_EVENT_MASK;
> > +     cp.min_report_interval = DEFALUT_REPORT_INTERVAL_MS;
> > +
> > +     skb = __hci_cmd_sync(hdev, BQR_OPCODE, sizeof(cp), &cp,
> > +                          HCI_CMD_TIMEOUT);
> > +     if (IS_ERR(skb)) {
> > +             bt_dev_err(hdev, "Enabling Android BQR failed (%ld)",
> > +                        PTR_ERR(skb));
> > +             return PTR_ERR(skb);
> > +     }
> > +
> > +     kfree_skb(skb);
> > +     return 0;
> > +}
> > +
> > +static int disable_quality_report(struct hci_dev *hdev)
> > +{
> > +     struct sk_buff *skb;
> > +     struct aosp_bqr_cp cp = { 0 };
> > +
> > +     cp.report_action = REPORT_ACTION_CLEAR;
> > +
> > +     skb = __hci_cmd_sync(hdev, BQR_OPCODE, sizeof(cp), &cp,
> > +                          HCI_CMD_TIMEOUT);
> > +     if (IS_ERR(skb)) {
> > +             bt_dev_err(hdev, "Disabling Android BQR failed (%ld)",
> > +                        PTR_ERR(skb));
> > +             return PTR_ERR(skb);
> > +     }
> > +
> > +     kfree_skb(skb);
> > +     return 0;
> > +}
> > +
> > +int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> > +{
> > +     bt_dev_info(hdev, "quality report enable %d", enable);
> > +
> > +     /* Enable or disable the quality report feature. */
> > +     if (enable)
> > +             return enable_quality_report(hdev);
> > +     else
> > +             return disable_quality_report(hdev);
> > +}
> > diff --git a/net/bluetooth/aosp.h b/net/bluetooth/aosp.h
> > index 328fc6d39f70..384e111c1260 100644
> > --- a/net/bluetooth/aosp.h
> > +++ b/net/bluetooth/aosp.h
> > @@ -8,9 +8,16 @@
> > void aosp_do_open(struct hci_dev *hdev);
> > void aosp_do_close(struct hci_dev *hdev);
> >
> > +int aosp_set_quality_report(struct hci_dev *hdev, bool enable);
> > +
> > #else
> >
> > static inline void aosp_do_open(struct hci_dev *hdev) {}
> > static inline void aosp_do_close(struct hci_dev *hdev) {}
> >
> > +static inline int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> > +{
> > +     return false;
> > +}
> > +
> > #endif
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index aeec5a3031a6..a2c22a4921d4 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -1315,6 +1315,21 @@ static void hci_dev_get_bd_addr_from_property(struct hci_dev *hdev)
> >       bacpy(&hdev->public_addr, &ba);
> > }
> >
> > +static void hci_set_quality_report(struct hci_dev *hdev)
> > +{
> > +#ifdef CONFIG_BT_AOSPEXT
> > +     if (hdev->aosp_capable) {
> > +             /* The hdev->set_quality_report callback is setup here for
> > +              * the vendors that support AOSP quality report specification.
> > +              * Note that Intel, while supporting a distinct telemetry
> > +              * quality report specification, sets up the
> > +              * hdev->set_quality_report callback in the btusb module.
> > +              */
> > +             hdev->set_quality_report = aosp_set_quality_report;
> > +     }
> > +#endif
> > +}
> > +
>
> I think that I wasn’t super clear in my review on how I wanted this feature. So hdev->set_quality_report should really only ever set by a transport driver. The core stack should never touch it.
>
> So I wanted something like this:
>
>         if (hdev->set_quality_report)
>                 err = hdev->set_quality_report(hdev, val);
>         else
>                 err = aosp_set_quality_report(hdev, val);
>
> I send a RFC showing you how I think this should be done.
>
> An extra important step of course is to check if the Android extension actually supports the quality report feature in the first place.
>
> And while writing that patch, I realized that your initial support has a mistake. I send a patch for fixing it. The mgmt document is pretty clear on how experimental flags are defined.
>
> Read Experimental Features Information Command
> ==============================================
>
>         Command Code:           0x0049
>         Controller Index:       <controller id> or <non-controller>
>         Command Parameters:
>         Return Parameters:      Feature_Count (2 Octets)
>                                 Feature1 {
>                                         UUID (16 Octets)
>                                         Flags (4 Octets)
>                                 }
>                                 Feature2 {  }
>                                 ...
>
>         This command is used to retrieve the supported experimental features
>         by the host stack.
>
>         The UUID values are not defined here. They can change over time and
>         are on purpose not stable. Features that mature will be removed at
>         some point. The mapping of feature UUID to the actual functionality
>         of a given feature is out of scope here.
>
>         The following bits are defined for the Flags parameter:
>
>                 0       Feature active
>                 1       Causes change in supported settings
>
> So please don’t just make up things and exp UUID should only be present if they are supported. If they are not supported because the hardware is lacking support, they should not be reported.
>
> Regards
>
> Marcel
>


-- 

Joseph Shyh-In Hwang
Email: josephsih@google.com

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-10-19 12:11 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-26  7:07 [PATCH v4 1/4] Bluetooth: aosp: Support AOSP Bluetooth Quality Report Joseph Hwang
2021-09-26  7:07 ` [PATCH v4 2/4] Bluetooth: hci_qca: enable Qualcomm WCN399x for AOSP extension Joseph Hwang
2021-09-28 11:09   ` Marcel Holtmann
2021-09-26  7:07 ` [PATCH v4 3/4] Bluetooth: btrtl: enable Realtek 8822C/8852A to support " Joseph Hwang
2021-09-28 11:09   ` Marcel Holtmann
2021-09-26  7:07 ` [PATCH v4 4/4] Bluetooth: btusb: enable Mediatek MT7921 " Joseph Hwang
2021-09-28 11:12   ` Marcel Holtmann
2021-09-27 23:08 ` [v4,1/4] Bluetooth: aosp: Support AOSP Bluetooth Quality Report bluez.test.bot
2021-09-28 10:44 ` [PATCH v4 1/4] " Marcel Holtmann
2021-10-19 12:11   ` Joseph Hwang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).