| 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
| ||
|
Date: Tue, 2 Mar 2021 14:56:56 +0100
From: Marcel Holtmann <marcel@...tmann.org>
To: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
Cc: CrosBT Upstreaming <chromeos-bluetooth-upstreaming@...omium.org>,
Hans de Goede <hdegoede@...hat.com>,
Bluetooth Kernel Mailing List
<linux-bluetooth@...r.kernel.org>,
Archie Pusaka <apusaka@...omium.org>,
Alain Michaud <alainm@...omium.org>,
"David S. Miller" <davem@...emloft.net>,
Johan Hedberg <johan.hedberg@...il.com>,
netdev <netdev@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Jakub Kicinski <kuba@...nel.org>,
Luiz Augusto von Dentz <luiz.dentz@...il.com>
Subject: Re: [PATCH 2/2] Bluetooth: Remove unneeded commands for suspend
Hi Abhishek,
> During suspend, there are a few scan enable and set event filter
> commands that don't need to be sent unless there are actual BR/EDR
> devices capable of waking the system. Check the HCI_PSCAN bit before
> writing scan enable and use a new dev flag, HCI_EVENT_FILTER_CONFIGURED
> to control whether to clear the event filter.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@...omium.org>
> Reviewed-by: Archie Pusaka <apusaka@...omium.org>
> Reviewed-by: Alain Michaud <alainm@...omium.org>
> ---
>
> include/net/bluetooth/hci.h | 1 +
> net/bluetooth/hci_event.c | 31 ++++++++++++++++++++++++++
> net/bluetooth/hci_request.c | 44 +++++++++++++++++++++++--------------
> 3 files changed, 59 insertions(+), 17 deletions(-)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index ba2f439bc04d34..ea4ae551c42687 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -320,6 +320,7 @@ enum {
> HCI_BREDR_ENABLED,
> HCI_LE_SCAN_INTERRUPTED,
> HCI_WIDEBAND_SPEECH_ENABLED,
> + HCI_EVENT_FILTER_CONFIGURED,
>
> HCI_DUT_MODE,
> HCI_VENDOR_DIAG,
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 67668be3461e93..17847e672b98cf 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -395,6 +395,33 @@ static void hci_cc_write_scan_enable(struct hci_dev *hdev, struct sk_buff *skb)
> hci_dev_unlock(hdev);
> }
>
> +static void hci_cc_set_event_filter(struct hci_dev *hdev, struct sk_buff *skb)
> +{
> + __u8 status = *((__u8 *)skb->data);
> + struct hci_cp_set_event_filter *cp;
> + void *sent;
> +
> + BT_DBG("%s status 0x%2.2x", hdev->name, status);
> +
> + sent = hci_sent_cmd_data(hdev, HCI_OP_SET_EVENT_FLT);
> + if (!sent)
> + return;
> +
> + cp = (struct hci_cp_set_event_filter *)sent;
> +
> + hci_dev_lock(hdev);
> +
> + if (status)
> + goto done;
So I have no idea why this is inside the hdev_lock scope. Just leave the function right before sent assignment if you don’t care about handling the failure.
> +
> + if (cp->flt_type == HCI_FLT_CLEAR_ALL)
> + hci_dev_clear_flag(hdev, HCI_EVENT_FILTER_CONFIGURED);
> + else
> + hci_dev_set_flag(hdev, HCI_EVENT_FILTER_CONFIGURED);
> +done:
> + hci_dev_unlock(hdev);
> +}
> +
> static void hci_cc_read_class_of_dev(struct hci_dev *hdev, struct sk_buff *skb)
> {
> struct hci_rp_read_class_of_dev *rp = (void *) skb->data;
> @@ -3328,6 +3355,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *skb,
> hci_cc_write_scan_enable(hdev, skb);
> break;
>
> + case HCI_OP_SET_EVENT_FLT:
> + hci_cc_set_event_filter(hdev, skb);
> + break;
> +
> case HCI_OP_READ_CLASS_OF_DEV:
> hci_cc_read_class_of_dev(hdev, skb);
> break;
> diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
> index e55976db4403e7..75a42178c82d9b 100644
> --- a/net/bluetooth/hci_request.c
> +++ b/net/bluetooth/hci_request.c
> @@ -1131,14 +1131,14 @@ static void hci_req_clear_event_filter(struct hci_request *req)
> {
> struct hci_cp_set_event_filter f;
>
> - memset(&f, 0, sizeof(f));
> - f.flt_type = HCI_FLT_CLEAR_ALL;
> - hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, &f);
> + if (!hci_dev_test_flag(req->hdev, HCI_BREDR_ENABLED))
> + return;
>
> - /* Update page scan state (since we may have modified it when setting
> - * the event filter).
> - */
> - __hci_req_update_scan(req);
> + if (hci_dev_test_flag(req->hdev, HCI_EVENT_FILTER_CONFIGURED)) {
> + memset(&f, 0, sizeof(f));
> + f.flt_type = HCI_FLT_CLEAR_ALL;
> + hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, &f);
> + }
> }
>
> static void hci_req_set_event_filter(struct hci_request *req)
> @@ -1147,6 +1147,10 @@ static void hci_req_set_event_filter(struct hci_request *req)
> struct hci_cp_set_event_filter f;
> struct hci_dev *hdev = req->hdev;
> u8 scan = SCAN_DISABLED;
> + bool scanning = test_bit(HCI_PSCAN, &hdev->flags);
> +
> + if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED))
> + return;
>
> /* Always clear event filter when starting */
> hci_req_clear_event_filter(req);
> @@ -1167,12 +1171,13 @@ static void hci_req_set_event_filter(struct hci_request *req)
> scan = SCAN_PAGE;
> }
>
> - if (scan)
> + if (scan && !scanning) {
> set_bit(SUSPEND_SCAN_ENABLE, hdev->suspend_tasks);
> - else
> + hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
> + } else if (!scan && scanning) {
> set_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
> -
> - hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
> + hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, &scan);
> + }
> }
>
> static void cancel_adv_timeout(struct hci_dev *hdev)
> @@ -1315,9 +1320,14 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
>
> hdev->advertising_paused = true;
> hdev->advertising_old_state = old_state;
> - /* Disable page scan */
> - page_scan = SCAN_DISABLED;
> - hci_req_add(&req, HCI_OP_WRITE_SCAN_ENABLE, 1, &page_scan);
> +
> + /* Disable page scan if enabled */
> + if (test_bit(HCI_PSCAN, &hdev->flags)) {
> + page_scan = SCAN_DISABLED;
> + hci_req_add(&req, HCI_OP_WRITE_SCAN_ENABLE, 1,
> + &page_scan);
> + set_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
> + }
>
> /* Disable LE passive scan if enabled */
> if (hci_dev_test_flag(hdev, HCI_LE_SCAN)) {
> @@ -1328,9 +1338,6 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
> /* Disable advertisement filters */
> hci_req_add_set_adv_filter_enable(&req, false);
>
> - /* Mark task needing completion */
> - set_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
> -
> /* Prevent disconnects from causing scanning to be re-enabled */
> hdev->scanning_paused = true;
>
> @@ -1364,7 +1371,10 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum suspended_state next)
> hdev->suspended = false;
> hdev->scanning_paused = false;
>
> + /* Clear any event filters and restore scan state */
> hci_req_clear_event_filter(&req);
> + __hci_req_update_scan(&req);
> +
> /* Reset passive/background scanning to normal */
> __hci_update_background_scan(&req);
> /* Enable all of the advertisement filters */
Rest looks good.
Regards
Marcel
Powered by blists - more mailing lists