* [PATCH] Bluetooth: Allow suspend even when preparation has failed
@ 2020-06-03 20:21 Abhishek Pandit-Subedi
2020-06-04 10:46 ` Rafael J. Wysocki
0 siblings, 1 reply; 3+ messages in thread
From: Abhishek Pandit-Subedi @ 2020-06-03 20:21 UTC (permalink / raw)
To: marcel, linux-bluetooth
Cc: len.brown, chromeos-bluetooth-upstreaming, linux-pm, rafael,
todd.e.brandt, rui.zhang, Abhishek Pandit-Subedi,
David S. Miller, Johan Hedberg, netdev, linux-kernel,
Jakub Kicinski
It is preferable to allow suspend even when Bluetooth has problems
preparing for sleep. When Bluetooth fails to finish preparing for
suspend, log the error and allow the suspend notifier to continue
instead.
To also make it clearer why suspend failed, change bt_dev_dbg to
bt_dev_err when handling the suspend timeout.
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
To verify this is properly working, I added an additional change to
hci_suspend_wait_event to always return -16. This validates that suspend
continues even when an error has occurred during the suspend
preparation.
Example on Chromebook:
[ 55.834524] PM: Syncing filesystems ... done.
[ 55.841930] PM: Preparing system for sleep (s2idle)
[ 55.940492] Bluetooth: hci_core.c:hci_suspend_notifier() hci0: Suspend notifier action (3) failed: -16
[ 55.940497] Freezing user space processes ... (elapsed 0.001 seconds) done.
[ 55.941692] OOM killer disabled.
[ 55.941693] Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
[ 55.942632] PM: Suspending system (s2idle)
I ran this through a suspend_stress_test in the following scenarios:
* Peer classic device connected: 50+ suspends
* No devices connected: 100 suspends
* With the above test case returning -EBUSY: 50 suspends
I also ran this through our automated testing for suspend and wake on
BT from suspend continues to work.
net/bluetooth/hci_core.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index dbe2d79f233fba..54da48441423e0 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3289,10 +3289,10 @@ static int hci_suspend_wait_event(struct hci_dev *hdev)
WAKE_COND, SUSPEND_NOTIFIER_TIMEOUT);
if (ret == 0) {
- bt_dev_dbg(hdev, "Timed out waiting for suspend");
+ bt_dev_err(hdev, "Timed out waiting for suspend events");
for (i = 0; i < __SUSPEND_NUM_TASKS; ++i) {
if (test_bit(i, hdev->suspend_tasks))
- bt_dev_dbg(hdev, "Bit %d is set", i);
+ bt_dev_err(hdev, "Suspend timeout bit: %d", i);
clear_bit(i, hdev->suspend_tasks);
}
@@ -3360,12 +3360,15 @@ static int hci_suspend_notifier(struct notifier_block *nb, unsigned long action,
ret = hci_change_suspend_state(hdev, BT_RUNNING);
}
- /* If suspend failed, restore it to running */
- if (ret && action == PM_SUSPEND_PREPARE)
- hci_change_suspend_state(hdev, BT_RUNNING);
-
done:
- return ret ? notifier_from_errno(-EBUSY) : NOTIFY_STOP;
+ /* We always allow suspend even if suspend preparation failed and
+ * attempt to recover in resume.
+ */
+ if (ret)
+ bt_dev_err(hdev, "Suspend notifier action (%x) failed: %d",
+ action, ret);
+
+ return NOTIFY_STOP;
}
/* Alloc HCI device */
--
2.27.0.rc2.251.g90737beb825-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] Bluetooth: Allow suspend even when preparation has failed
2020-06-03 20:21 [PATCH] Bluetooth: Allow suspend even when preparation has failed Abhishek Pandit-Subedi
@ 2020-06-04 10:46 ` Rafael J. Wysocki
2020-06-05 4:29 ` Abhishek Pandit-Subedi
0 siblings, 1 reply; 3+ messages in thread
From: Rafael J. Wysocki @ 2020-06-04 10:46 UTC (permalink / raw)
To: Abhishek Pandit-Subedi, Len Brown, Todd Brandt
Cc: Marcel Holtmann, open list:BLUETOOTH DRIVERS,
chromeos-bluetooth-upstreaming, Linux PM, Rafael J. Wysocki,
Zhang, Rui, David S. Miller, Johan Hedberg, netdev,
Linux Kernel Mailing List, Jakub Kicinski
On Wed, Jun 3, 2020 at 10:22 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> It is preferable to allow suspend even when Bluetooth has problems
> preparing for sleep. When Bluetooth fails to finish preparing for
> suspend, log the error and allow the suspend notifier to continue
> instead.
>
> To also make it clearer why suspend failed, change bt_dev_dbg to
> bt_dev_err when handling the suspend timeout.
>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Thanks for the patch, it looks reasonable to me.
It would be good to add a Fixes tag to it to indicate that it works
around an issue introduced by an earlier commit.
Len, Todd, would it be possible to test this one on the affected machines?
> ---
> To verify this is properly working, I added an additional change to
> hci_suspend_wait_event to always return -16. This validates that suspend
> continues even when an error has occurred during the suspend
> preparation.
>
> Example on Chromebook:
> [ 55.834524] PM: Syncing filesystems ... done.
> [ 55.841930] PM: Preparing system for sleep (s2idle)
> [ 55.940492] Bluetooth: hci_core.c:hci_suspend_notifier() hci0: Suspend notifier action (3) failed: -16
> [ 55.940497] Freezing user space processes ... (elapsed 0.001 seconds) done.
> [ 55.941692] OOM killer disabled.
> [ 55.941693] Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
> [ 55.942632] PM: Suspending system (s2idle)
>
> I ran this through a suspend_stress_test in the following scenarios:
> * Peer classic device connected: 50+ suspends
> * No devices connected: 100 suspends
> * With the above test case returning -EBUSY: 50 suspends
>
> I also ran this through our automated testing for suspend and wake on
> BT from suspend continues to work.
>
>
> net/bluetooth/hci_core.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index dbe2d79f233fba..54da48441423e0 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3289,10 +3289,10 @@ static int hci_suspend_wait_event(struct hci_dev *hdev)
> WAKE_COND, SUSPEND_NOTIFIER_TIMEOUT);
>
> if (ret == 0) {
> - bt_dev_dbg(hdev, "Timed out waiting for suspend");
> + bt_dev_err(hdev, "Timed out waiting for suspend events");
> for (i = 0; i < __SUSPEND_NUM_TASKS; ++i) {
> if (test_bit(i, hdev->suspend_tasks))
> - bt_dev_dbg(hdev, "Bit %d is set", i);
> + bt_dev_err(hdev, "Suspend timeout bit: %d", i);
> clear_bit(i, hdev->suspend_tasks);
> }
>
> @@ -3360,12 +3360,15 @@ static int hci_suspend_notifier(struct notifier_block *nb, unsigned long action,
> ret = hci_change_suspend_state(hdev, BT_RUNNING);
> }
>
> - /* If suspend failed, restore it to running */
> - if (ret && action == PM_SUSPEND_PREPARE)
> - hci_change_suspend_state(hdev, BT_RUNNING);
> -
> done:
> - return ret ? notifier_from_errno(-EBUSY) : NOTIFY_STOP;
> + /* We always allow suspend even if suspend preparation failed and
> + * attempt to recover in resume.
> + */
> + if (ret)
> + bt_dev_err(hdev, "Suspend notifier action (%x) failed: %d",
> + action, ret);
> +
> + return NOTIFY_STOP;
> }
>
> /* Alloc HCI device */
> --
> 2.27.0.rc2.251.g90737beb825-goog
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Bluetooth: Allow suspend even when preparation has failed
2020-06-04 10:46 ` Rafael J. Wysocki
@ 2020-06-05 4:29 ` Abhishek Pandit-Subedi
0 siblings, 0 replies; 3+ messages in thread
From: Abhishek Pandit-Subedi @ 2020-06-05 4:29 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Len Brown, Todd Brandt, Marcel Holtmann,
open list:BLUETOOTH DRIVERS, ChromeOS Bluetooth Upstreaming,
Linux PM, Zhang, Rui, David S. Miller, Johan Hedberg, netdev,
Linux Kernel Mailing List, Jakub Kicinski
Sent a v2 with proper fixes and reported-by tags.
Thanks
Abhishek
On Thu, Jun 4, 2020 at 3:46 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Wed, Jun 3, 2020 at 10:22 PM Abhishek Pandit-Subedi
> <abhishekpandit@chromium.org> wrote:
> >
> > It is preferable to allow suspend even when Bluetooth has problems
> > preparing for sleep. When Bluetooth fails to finish preparing for
> > suspend, log the error and allow the suspend notifier to continue
> > instead.
> >
> > To also make it clearer why suspend failed, change bt_dev_dbg to
> > bt_dev_err when handling the suspend timeout.
> >
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>
> Thanks for the patch, it looks reasonable to me.
>
> It would be good to add a Fixes tag to it to indicate that it works
> around an issue introduced by an earlier commit.
>
> Len, Todd, would it be possible to test this one on the affected machines?
>
> > ---
> > To verify this is properly working, I added an additional change to
> > hci_suspend_wait_event to always return -16. This validates that suspend
> > continues even when an error has occurred during the suspend
> > preparation.
> >
> > Example on Chromebook:
> > [ 55.834524] PM: Syncing filesystems ... done.
> > [ 55.841930] PM: Preparing system for sleep (s2idle)
> > [ 55.940492] Bluetooth: hci_core.c:hci_suspend_notifier() hci0: Suspend notifier action (3) failed: -16
> > [ 55.940497] Freezing user space processes ... (elapsed 0.001 seconds) done.
> > [ 55.941692] OOM killer disabled.
> > [ 55.941693] Freezing remaining freezable tasks ... (elapsed 0.000 seconds) done.
> > [ 55.942632] PM: Suspending system (s2idle)
> >
> > I ran this through a suspend_stress_test in the following scenarios:
> > * Peer classic device connected: 50+ suspends
> > * No devices connected: 100 suspends
> > * With the above test case returning -EBUSY: 50 suspends
> >
> > I also ran this through our automated testing for suspend and wake on
> > BT from suspend continues to work.
> >
> >
> > net/bluetooth/hci_core.c | 17 ++++++++++-------
> > 1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index dbe2d79f233fba..54da48441423e0 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -3289,10 +3289,10 @@ static int hci_suspend_wait_event(struct hci_dev *hdev)
> > WAKE_COND, SUSPEND_NOTIFIER_TIMEOUT);
> >
> > if (ret == 0) {
> > - bt_dev_dbg(hdev, "Timed out waiting for suspend");
> > + bt_dev_err(hdev, "Timed out waiting for suspend events");
> > for (i = 0; i < __SUSPEND_NUM_TASKS; ++i) {
> > if (test_bit(i, hdev->suspend_tasks))
> > - bt_dev_dbg(hdev, "Bit %d is set", i);
> > + bt_dev_err(hdev, "Suspend timeout bit: %d", i);
> > clear_bit(i, hdev->suspend_tasks);
> > }
> >
> > @@ -3360,12 +3360,15 @@ static int hci_suspend_notifier(struct notifier_block *nb, unsigned long action,
> > ret = hci_change_suspend_state(hdev, BT_RUNNING);
> > }
> >
> > - /* If suspend failed, restore it to running */
> > - if (ret && action == PM_SUSPEND_PREPARE)
> > - hci_change_suspend_state(hdev, BT_RUNNING);
> > -
> > done:
> > - return ret ? notifier_from_errno(-EBUSY) : NOTIFY_STOP;
> > + /* We always allow suspend even if suspend preparation failed and
> > + * attempt to recover in resume.
> > + */
> > + if (ret)
> > + bt_dev_err(hdev, "Suspend notifier action (%x) failed: %d",
> > + action, ret);
> > +
> > + return NOTIFY_STOP;
> > }
> >
> > /* Alloc HCI device */
> > --
> > 2.27.0.rc2.251.g90737beb825-goog
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-06-05 4:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-03 20:21 [PATCH] Bluetooth: Allow suspend even when preparation has failed Abhishek Pandit-Subedi
2020-06-04 10:46 ` Rafael J. Wysocki
2020-06-05 4:29 ` Abhishek Pandit-Subedi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).