* [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 related [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
* [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 related [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
* [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 related [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: [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 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