diff mbox series

[v6,5/5] Bluetooth: let HCI_QUALITY_REPORT persist over adapter power cycle

Message ID 20220526112135.2486883-3-josephsih@chromium.org (mailing list archive)
State New, archived
Headers show
Series [v6,1/5] Bluetooth: mgmt: add MGMT_OP_SET_QUALITY_REPORT for quality report | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/checkpatch success Checkpatch PASS
tedd_an/gitlint success Gitlint PASS
tedd_an/subjectprefix success PASS

Commit Message

Joseph Hwang May 26, 2022, 11:21 a.m. UTC 
The quality report specifications, including AOSP Bluetooth Quality
Report and Intel Telemetry Event, do not define what happen when
the adapter is turned off and then on. To be consistent among
different specifications and vendors, the quality report feature is
turned off when the adapter is powered off and is turned on when
the adapter is powered on if the feature has been on before power
cycle.

Signed-off-by: Joseph Hwang <josephsih@chromium.org>
Reviewed-by: Archie Pusaka <apusaka@chromium.org>
---

(no changes since v5)

Changes in v5:
- This is a new patch in this series changes version.

 include/net/bluetooth/hci_core.h |  1 -
 net/bluetooth/hci_sync.c         | 35 +++++++++++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 2 deletions(-)

Comments

Marcel Holtmann June 2, 2022, 3:59 p.m. UTC | #1 
Hi Joseph,

> The quality report specifications, including AOSP Bluetooth Quality
> Report and Intel Telemetry Event, do not define what happen when
> the adapter is turned off and then on. To be consistent among
> different specifications and vendors, the quality report feature is
> turned off when the adapter is powered off and is turned on when
> the adapter is powered on if the feature has been on before power
> cycle.
> 
> Signed-off-by: Joseph Hwang <josephsih@chromium.org>
> Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> ---
> 
> (no changes since v5)
> 
> Changes in v5:
> - This is a new patch in this series changes version.
> 
> include/net/bluetooth/hci_core.h |  1 -
> net/bluetooth/hci_sync.c         | 35 +++++++++++++++++++++++++++++++-
> 2 files changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 9e48d606591e..5788350efa68 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -807,7 +807,6 @@ extern struct mutex hci_cb_list_lock;
> 		hci_dev_clear_flag(hdev, HCI_LE_ADV);		\
> 		hci_dev_clear_flag(hdev, HCI_LL_RPA_RESOLUTION);\
> 		hci_dev_clear_flag(hdev, HCI_PERIODIC_INQ);	\
> -		hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT);	\
> 	} while (0)

this really need to go into your 1/5 patch.

> 
> #define hci_dev_le_state_simultaneous(hdev) \
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index a6ada9dcede5..12a18d046bb6 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -3849,6 +3849,31 @@ static const struct {
> 			 "advertised, but not supported.")
> };
> 
> +static void suspend_resume_quality_report(struct hci_dev *hdev, bool enable)
> +{
> +	int err;
> +
> +	/* Suspend and resume quality report only when the feature has
> +	 * already been enabled. The HCI_QUALITY_REPORT flag, as an indicator
> +	 * whether to re-enable the feature after resume, is not changed by
> +	 * suspend/resume.
> +	 */
> +	if (!hci_dev_test_flag(hdev, HCI_QUALITY_REPORT))
> +		return;
> +
> +	if (hdev->set_quality_report)
> +		err = hdev->set_quality_report(hdev, enable);
> +	else
> +		err = aosp_set_quality_report(hdev, enable);
> +
> +	if (err)
> +		bt_dev_err(hdev, "%s quality report error %d",
> +			   enable ? "resume" : "suspend", err);
> +	else
> +		bt_dev_info(hdev, "%s quality report",
> +			    enable ? "resume" : "suspend");

Do you really need this “debug” output?

> +}
> +
> int hci_dev_open_sync(struct hci_dev *hdev)
> {
> 	int ret = 0;
> @@ -4013,6 +4038,7 @@ int hci_dev_open_sync(struct hci_dev *hdev)
> 	if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
> 		msft_do_open(hdev);
> 		aosp_do_open(hdev);
> +		suspend_resume_quality_report(hdev, true);
> 	}
> 
> 	clear_bit(HCI_INIT, &hdev->flags);
> @@ -4095,6 +4121,14 @@ int hci_dev_close_sync(struct hci_dev *hdev)
> 
> 	hci_request_cancel_all(hdev);
> 
> +	/* Disable quality report and close aosp before shutdown()
> +	 * is called. Otherwise, some chips may panic.
> +	 */
> +	if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
> +		suspend_resume_quality_report(hdev, false);
> +		aosp_do_close(hdev);
> +	}
> +

Why move aosp_do_close here. I prefer to keep it where it was.

> 	if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) &&
> 	    !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
> 	    test_bit(HCI_UP, &hdev->flags)) {
> @@ -4158,7 +4192,6 @@ int hci_dev_close_sync(struct hci_dev *hdev)
> 	hci_sock_dev_event(hdev, HCI_DEV_DOWN);
> 
> 	if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
> -		aosp_do_close(hdev);
> 		msft_do_close(hdev);
> 	}

Regards

Marcel
diff mbox series

Patch

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 9e48d606591e..5788350efa68 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -807,7 +807,6 @@  extern struct mutex hci_cb_list_lock;
 		hci_dev_clear_flag(hdev, HCI_LE_ADV);		\
 		hci_dev_clear_flag(hdev, HCI_LL_RPA_RESOLUTION);\
 		hci_dev_clear_flag(hdev, HCI_PERIODIC_INQ);	\
-		hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT);	\
 	} while (0)
 
 #define hci_dev_le_state_simultaneous(hdev) \
diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index a6ada9dcede5..12a18d046bb6 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -3849,6 +3849,31 @@  static const struct {
 			 "advertised, but not supported.")
 };
 
+static void suspend_resume_quality_report(struct hci_dev *hdev, bool enable)
+{
+	int err;
+
+	/* Suspend and resume quality report only when the feature has
+	 * already been enabled. The HCI_QUALITY_REPORT flag, as an indicator
+	 * whether to re-enable the feature after resume, is not changed by
+	 * suspend/resume.
+	 */
+	if (!hci_dev_test_flag(hdev, HCI_QUALITY_REPORT))
+		return;
+
+	if (hdev->set_quality_report)
+		err = hdev->set_quality_report(hdev, enable);
+	else
+		err = aosp_set_quality_report(hdev, enable);
+
+	if (err)
+		bt_dev_err(hdev, "%s quality report error %d",
+			   enable ? "resume" : "suspend", err);
+	else
+		bt_dev_info(hdev, "%s quality report",
+			    enable ? "resume" : "suspend");
+}
+
 int hci_dev_open_sync(struct hci_dev *hdev)
 {
 	int ret = 0;
@@ -4013,6 +4038,7 @@  int hci_dev_open_sync(struct hci_dev *hdev)
 	if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
 		msft_do_open(hdev);
 		aosp_do_open(hdev);
+		suspend_resume_quality_report(hdev, true);
 	}
 
 	clear_bit(HCI_INIT, &hdev->flags);
@@ -4095,6 +4121,14 @@  int hci_dev_close_sync(struct hci_dev *hdev)
 
 	hci_request_cancel_all(hdev);
 
+	/* Disable quality report and close aosp before shutdown()
+	 * is called. Otherwise, some chips may panic.
+	 */
+	if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
+		suspend_resume_quality_report(hdev, false);
+		aosp_do_close(hdev);
+	}
+
 	if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) &&
 	    !hci_dev_test_flag(hdev, HCI_USER_CHANNEL) &&
 	    test_bit(HCI_UP, &hdev->flags)) {
@@ -4158,7 +4192,6 @@  int hci_dev_close_sync(struct hci_dev *hdev)
 	hci_sock_dev_event(hdev, HCI_DEV_DOWN);
 
 	if (!hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
-		aosp_do_close(hdev);
 		msft_do_close(hdev);
 	}