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@xxxxxxxxxxxx>
> Reviewed-by: Archie Pusaka <apusaka@xxxxxxxxxxxx>
> ---
>
> (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
