netdev - Re: [PATCH v6 5/5] Bluetooth: let HCI_QUALITY_REPORT persist over adapter power cycle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <0DB8D064-222A-4420-8F9F-2BEEA63DF76A@holtmann.org>
Date:   Thu, 2 Jun 2022 17:59:23 +0200
From:   Marcel Holtmann <marcel@...tmann.org>
To:     Joseph Hwang <josephsih@...omium.org>
Cc:     BlueZ <linux-bluetooth@...r.kernel.org>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        Pali Rohár <pali@...nel.org>,
        CrosBT Upstreaming <chromeos-bluetooth-upstreaming@...omium.org>,
        Joseph Hwang <josephsih@...gle.com>,
        Archie Pusaka <apusaka@...omium.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Paolo Abeni <pabeni@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH v6 5/5] Bluetooth: let HCI_QUALITY_REPORT persist over
 adapter power cycle

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@...omium.org>
> Reviewed-by: Archie Pusaka <apusaka@...omium.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