From: "Rafael J. Wysocki" <rjw@rjwysocki.net>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Brian Norris <briannorris@chromium.org>,
Pavel Machek <pavel@ucw.cz>, Len Brown <len.brown@intel.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
lkml <linux-kernel@vger.kernel.org>,
Doug Anderson <dianders@chromium.org>,
Brian Norris <computersforpeace@gmail.com>,
Jeffy Chen <jeffy.chen@rock-chips.com>,
"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
Chuansheng Liu <chuansheng.liu@intel.com>
Subject: Re: [PATCH v2 2/2] PM / sleep: don't suspend parent when async child suspend_{noirq,late} fails
Date: Wed, 02 Nov 2016 04:51:08 +0100 [thread overview]
Message-ID: <50971906.K6xak2t6Z6@vostro.rjw.lan> (raw)
In-Reply-To: <CAKdAkRS+uUrx72DLKuyDiUMkKzxMTRXx5Pv1JBky6T8huv4Fhg@mail.gmail.com>
On Tuesday, November 01, 2016 12:04:28 AM Dmitry Torokhov wrote:
> On Mon, Oct 31, 2016 at 10:25 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Thursday, October 27, 2016 09:05:34 AM Brian Norris wrote:
> >> Consider two devices, A and B, where B is a child of A, and B utilizes
> >> asynchronous suspend (it does not matter whether A is sync or async). If
> >> B fails to suspend_noirq() or suspend_late(), or is interrupted by a
> >> wakeup (pm_wakeup_pending()), then it aborts and sets the async_error
> >> variable. However, device A does not (immediately) check the async_error
> >> variable; it may continue to run its own suspend_noirq()/suspend_late()
> >> callback. This is bad.
> >>
> >> We can resolve this problem by checking the async_error flag after
> >> waiting for children to suspend, using the same logic for the noirq and
> >> late suspend cases as we already do for __device_suspend().
> >>
> >> It's easy to observe this erroneous behavior by, for example, forcing a
> >> device to sleep a bit in its suspend_noirq() (to ensure the parent is
> >> waiting for the child to complete), then return an error, and watch the
> >> parent suspend_noirq() still get called. (Or similarly, fake a wakeup
> >> event at the right (or is it wrong?) time.)
> >>
> >> Fixes: de377b397272 ("PM / sleep: Asynchronous threads for suspend_late")
> >> Fixes: 28b6fd6e3779 ("PM / sleep: Asynchronous threads for suspend_noirq")
> >> Reported-by: Jeffy Chen <jeffy.chen@rock-chips.com>
> >> Signed-off-by: Brian Norris <briannorris@chromium.org>
> >> Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >> ---
> >> v2: s/early/late/ in commit message
> >>
> >> drivers/base/power/main.c | 6 ++++++
> >> 1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> >> index c58563581345..eaf6b53463a5 100644
> >> --- a/drivers/base/power/main.c
> >> +++ b/drivers/base/power/main.c
> >> @@ -1040,6 +1040,9 @@ static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool a
> >>
> >> dpm_wait_for_children(dev, async);
> >>
> >> + if (async_error)
> >> + goto Complete;
> >> +
> >
> > This is a second chech for async_error in this routine and is the first one
> > really needed after adding this?
>
> There is really no point in waiting for children to be suspended if
> error has already been signalled; that's what first check achieves.
> The 2nd check ensures that we abort suspend if any of the children
> failed to suspend.
>
> I'd say both checks are needed (well, 1st is helpful, 2nd is essential).
OK, fair enough.
Thanks,
Rafael
next prev parent reply other threads:[~2016-11-02 3:43 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-10-20 0:26 [RESEND PATCH 1/2] PM / sleep: print function name of callbacks Brian Norris
2016-10-20 0:26 ` [PATCH 2/2] PM / sleep: don't suspend parent when async child suspend_{noirq,early} fails Brian Norris
2016-10-20 0:46 ` Brian Norris
2016-10-27 15:34 ` Greg Kroah-Hartman
2016-10-27 16:03 ` Brian Norris
2016-10-20 0:56 ` Dmitry Torokhov
2016-10-27 16:05 ` [PATCH v2 2/2] PM / sleep: don't suspend parent when async child suspend_{noirq,late} fails Brian Norris
2016-11-01 4:25 ` Rafael J. Wysocki
2016-11-01 5:22 ` Brian Norris
2016-11-01 6:04 ` Dmitry Torokhov
2016-11-02 3:51 ` Rafael J. Wysocki [this message]
2016-11-02 5:07 ` Brian Norris
2016-11-10 0:08 ` Rafael J. Wysocki
2016-11-10 0:18 ` Brian Norris
2016-11-10 1:21 ` [PATCH v3] " Brian Norris
2016-11-10 1:53 ` Rafael J. Wysocki
2016-11-10 2:00 ` Brian Norris
2016-11-11 1:42 ` Rafael J. Wysocki
2016-10-20 0:52 ` [RESEND PATCH 1/2] PM / sleep: print function name of callbacks Dmitry Torokhov
2016-11-01 4:27 ` Rafael J. Wysocki
2016-11-02 21:02 ` Brian Norris
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50971906.K6xak2t6Z6@vostro.rjw.lan \
--to=rjw@rjwysocki.net \
--cc=briannorris@chromium.org \
--cc=chuansheng.liu@intel.com \
--cc=computersforpeace@gmail.com \
--cc=dianders@chromium.org \
--cc=dmitry.torokhov@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=jeffy.chen@rock-chips.com \
--cc=len.brown@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=pavel@ucw.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).