lkml.org 
[lkml]   [2018]   [Oct]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RFC PATCH] PM / core: skip suspend next time if resume returns an error
    Hi,

    On Tue, Oct 2, 2018 at 1:29 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
    >
    > On Tue, Oct 2, 2018 at 10:05 AM Pavel Machek <pavel@ucw.cz> wrote:
    > >
    > > Hi!
    > >
    > > > In general Linux doesn't behave super great if you get an error while
    > > > executing a device's resume handler. Nothing will come along later
    > > > and and try again to resume the device (and all devices that depend on
    > > > it), so pretty much you're left with a non-functioning device and
    > > > that's not good.
    > > >
    > > > However, even though you'll end up with a non-functioning device we
    > > > still don't consider resume failures to be fatal to the system. We'll
    > > > keep chugging along and just hope that the device that failed to
    > > > resume wasn't too critical. This establishes the precedent that we
    > > > should at least try our best not to fully bork the system after a
    > > > resume failure.
    > > >
    > > > I will argue that the best way to keep the system in the best shape is
    > > > to assume that if a resume callback failed that it did as close to
    > > > no-op as possible. Because of this we should consider the device
    > > > still suspended and shouldn't try to suspend the device again next
    > > > time around. Today that's not what happens. AKA if you have a
    > > > device
    > >
    > > I don't think there are many guarantees when device resume fail. It
    > > may have done nothing, and it may have resumed the device almost
    > > fully.

    With today's drivers you're probably right. I don't think too many
    drivers properly undo everything when they fail in the resume path.

    ...but it makes sense to have some sort of standard for how drivers
    ought to behave. IMO the most sane thing to do (and what we do in
    probe--which is why EPROBE_DEFER mostly works) is that if a function
    returns an error then it should try its best to undo everything it
    did. Then you can always try calling it again later.


    > > I guess the best option would be to refuse system suspend after some
    > > device failed like that.

    Yeah, I thought about that. I think we sometimes end up in that
    situation today (without my patch) because the suspend callback
    doesn't always work so great if the previous resume failed, so it's
    entirely likely it'll return an error and block suspend...

    ...another thing I thought about was possibly trying to forcibly
    unbind the device. AKA: it's misbehaving so kick it out of the
    system... Of course if "resume" failed perhaps "remove" won't be so
    happy...


    > > That leaves user possibility to debug it...

    You mean they wouldn't notice that some peripheral failed to resume
    but they'd notice that their computer doesn't suspend any more? Maybe
    they'd notice, but it'd probably because they pulled their laptop out
    of their bag and it was hot and had a dead batter.


    > I guess so.
    >
    > Doing that in all cases is kind of risky IMO, because we haven't taken
    > the return values of the ->resume* callbacks into account so far
    > (except for printing the information that is), so there may be
    > non-lethal cases when that happens and the $subject patch would make
    > them not work any more.

    I think you're arguing that the best option is to leave the code / API
    exactly as-is because someone could be relying on the existing
    behavior? That is certainly the least likely to introduce any new
    bugs. ;-P

    ...would you accept a patch adding a comment codifying the existing
    behavior (AKA suspend will be called again even if resume failed) as
    the officially documented behavior? Then we can start making new
    drivers behave correctly at least. If nothing else I can add a
    boolean inside my driver data that says "resume failed, ignore the
    next suspend".

    ...or if the official word is that if your resume fails you're totally
    unrecoverable then I can start simplifying the error handling in
    resume. AKA instead of:

    hypothetical_resume(...) {
    ret = clk_enable(...);
    if (ret)
    return ret;
    ret = regulator_enable(...);
    if (ret)
    clk_disable(...);
    return ret;

    ...I can just change it to:

    hypothetical_resume(...) {
    ret = clk_enable(...);
    if (ret)
    return ret;
    return regulator_enable(...);

    ...the above would leave no way to recover the system because if
    hypothetical_resume() returned an error we'd have no idea if the clock
    was left enabled or not. ...but if we're unrecoverable anyway why not
    save the code?


    -Doug

    \
     
     \ /
      Last update: 2018-10-02 23:03    [W:6.501 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site