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

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

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