[v2] Bluetooth: hci_sync: Fix suspending with wrong filter policy

Commit Message

Luiz Augusto von Dentz July 16, 2024, 3:01 p.m. UTC

From: Luiz Augusto von Dentz <[e-mail]>

When suspending the scan filter policy cannot be 0x00 (no acceptlist)
since that means the host has to process every advertisement report
waking up the system, so this attempts to check if hdev is marked as
suspended and if the resulting filter policy would be 0x00 (no
acceptlist) then skip passive scanning if thre no devices in the
acceptlist otherwise reset the filter policy to 0x01 so the acceptlist
is used since the devices programmed there can still wakeup be system.

Fixes: 182ee45da083 ("Bluetooth: hci_sync: Rework hci_suspend_notifier")
Signed-off-by: Luiz Augusto von Dentz <[e-mail]>
---
 net/bluetooth/hci_sync.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

[e-mail] July 16, 2024, 3:33 p.m. UTC | #1

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=871711

---Test result---

Test Summary:
CheckPatch                    PASS      0.51 seconds
GitLint                       PASS      0.21 seconds
SubjectPrefix                 PASS      0.07 seconds
BuildKernel                   PASS      29.74 seconds
CheckAllWarning               PASS      32.75 seconds
CheckSparse                   PASS      38.10 seconds
CheckSmatch                   PASS      102.56 seconds
BuildKernel32                 PASS      28.75 seconds
TestRunnerSetup               PASS      528.01 seconds
TestRunner_l2cap-tester       PASS      22.05 seconds
TestRunner_iso-tester         PASS      35.14 seconds
TestRunner_bnep-tester        PASS      4.83 seconds
TestRunner_mgmt-tester        FAIL      112.32 seconds
TestRunner_rfcomm-tester      PASS      7.47 seconds
TestRunner_sco-tester         PASS      15.07 seconds
TestRunner_ioctl-tester       PASS      7.93 seconds
TestRunner_mesh-tester        PASS      6.00 seconds
TestRunner_smp-tester         PASS      11.27 seconds
TestRunner_userchan-tester    PASS      5.00 seconds
IncrementalBuild              PASS      28.15 seconds

Details
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 492, Passed: 489 (99.4%), Failed: 1, Not Run: 2

Failed Test Cases
LL Privacy - Add Device 7 (AL is full)               Failed       0.192 seconds


---
Regards,
Linux Bluetooth

Paul Menzel July 16, 2024, 3:57 p.m. UTC | #2

Dear Luiz,


Thank you for the patch. Four minor comments.

Am 16.07.24 um 17:01 schrieb Luiz Augusto von Dentz:
> From: Luiz Augusto von Dentz <[e-mail]>
> 
> When suspending the scan filter policy cannot be 0x00 (no acceptlist)
> since that means the host has to process every advertisement report
> waking up the system, so this attempts to check if hdev is marked as
> suspended and if the resulting filter policy would be 0x00 (no
> acceptlist) then skip passive scanning if thre no devices in the

there are

> acceptlist otherwise reset the filter policy to 0x01 so the acceptlist
> is used since the devices programmed there can still wakeup be system.

wake the system up?

> Fixes: 182ee45da083 ("Bluetooth: hci_sync: Rework hci_suspend_notifier")
> Signed-off-by: Luiz Augusto von Dentz <[e-mail]>
> ---
>   net/bluetooth/hci_sync.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index cd2ed16da8a4..a31d39a821f4 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -2976,6 +2976,27 @@ static int hci_passive_scan_sync(struct hci_dev *hdev)
>   	 */
>   	filter_policy = hci_update_accept_list_sync(hdev);
>   
> +	/* If suspended and filter_policy set to 0x00 (no acceptlist) then
> +	 * passive scanning cannot be started since that would require the host
> +	 * to be woken up to process the reports.
> +	 */
> +	if (hdev->suspended && !filter_policy) {
> +		/* Check if accept list is empty then there is no need to scan
> +		 * while suspended.
> +		 */
> +		if (list_empty(&hdev->le_accept_list))
> +			return 0;
> +
> +		/* If there are devices is the accept_list that means some

i*n* the

> +		 * devices could not be programmed which in non-suspended case
> +		 * means filter_policy needs to be set to 0x00 so the host needs
> +		 * to filter, but since this is treating suspended case we
> +		 * can ignore device needing host to filter to allow devices in
> +		 * the acceptlist to be able to wakeup the system.

to wake up

> +		 */
> +		filter_policy = 0x01;
> +	}
> +
>   	/* When the controller is using random resolvable addresses and
>   	 * with that having LE privacy enabled, then controllers with
>   	 * Extended Scanner Filter Policies support can now enable support


Kind regards,

Paul

[e-mail] July 17, 2024, 8 p.m. UTC | #3

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <[e-mail]>:

On Tue, 16 Jul 2024 11:01:26 -0400 you wrote:
> From: Luiz Augusto von Dentz <[e-mail]>
> 
> When suspending the scan filter policy cannot be 0x00 (no acceptlist)
> since that means the host has to process every advertisement report
> waking up the system, so this attempts to check if hdev is marked as
> suspended and if the resulting filter policy would be 0x00 (no
> acceptlist) then skip passive scanning if thre no devices in the
> acceptlist otherwise reset the filter policy to 0x01 so the acceptlist
> is used since the devices programmed there can still wakeup be system.
> 
> [...]

Here is the summary with links:
  - [v2] Bluetooth: hci_sync: Fix suspending with wrong filter policy
    https://git.kernel.org/bluetooth/bluetooth-next/c/54dd4796336d

You are awesome, thank you!

Patch

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index cd2ed16da8a4..a31d39a821f4 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -2976,6 +2976,27 @@  static int hci_passive_scan_sync(struct hci_dev *hdev)
 	 */
 	filter_policy = hci_update_accept_list_sync(hdev);
 
+	/* If suspended and filter_policy set to 0x00 (no acceptlist) then
+	 * passive scanning cannot be started since that would require the host
+	 * to be woken up to process the reports.
+	 */
+	if (hdev->suspended && !filter_policy) {
+		/* Check if accept list is empty then there is no need to scan
+		 * while suspended.
+		 */
+		if (list_empty(&hdev->le_accept_list))
+			return 0;
+
+		/* If there are devices is the accept_list that means some
+		 * devices could not be programmed which in non-suspended case
+		 * means filter_policy needs to be set to 0x00 so the host needs
+		 * to filter, but since this is treating suspended case we
+		 * can ignore device needing host to filter to allow devices in
+		 * the acceptlist to be able to wakeup the system.
+		 */
+		filter_policy = 0x01;
+	}
+
 	/* When the controller is using random resolvable addresses and
 	 * with that having LE privacy enabled, then controllers with
 	 * Extended Scanner Filter Policies support can now enable support