Sent a v2 with proper fixes and reported-by tags.
Thanks
Abhishek
On Thu, Jun 4, 2020 at 3:46 AM Rafael J. Wysocki <raf...@kernel.org> wrote:
>
> On Wed, Jun 3, 2020 at 10:22 PM Abhishek Pandit-Subedi
> <abhishekpan...@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 <abhishekpan...@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
> >
