mbox series

[0/2] Bluetooth: Suspend improvements

Message ID 20210301200605.106607-1-abhishekpandit@chromium.org
Headers show
Series Bluetooth: Suspend improvements | expand

Message

Abhishek Pandit-Subedi March 1, 2021, 8:06 p.m. UTC 
Hi Marcel (and linux bluetooth),

Here are a few suspend improvements based on user reports we saw on
ChromeOS and feedback from Hans de Goede on the mailing list.

I have tested this using our ChromeOS suspend/resume automated tests
(full SRHealth test coverage and some suspend resume stress tests).

Thanks
Abhishek



Abhishek Pandit-Subedi (2):
  Bluetooth: Notify suspend on le conn failed
  Bluetooth: Remove unneeded commands for suspend

 include/net/bluetooth/hci.h |  1 +
 net/bluetooth/hci_conn.c    | 10 +++++++++
 net/bluetooth/hci_event.c   | 31 ++++++++++++++++++++++++++
 net/bluetooth/hci_request.c | 44 +++++++++++++++++++++++--------------
 4 files changed, 69 insertions(+), 17 deletions(-)

Comments

Marcel Holtmann March 2, 2021, 1:56 p.m. UTC | #1 
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@chromium.org>
> Reviewed-by: Archie Pusaka <apusaka@chromium.org>
> Reviewed-by: Alain Michaud <alainm@chromium.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