lkml.org 
[lkml]   [2008]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [linux-pm] Fundamental flaw in system suspend, exposed by freezer removal
    Date
    On Wednesday, 27 of February 2008, Alan Stern wrote:
    > On Wed, 27 Feb 2008, Rafael J. Wysocki wrote:
    >
    > > > I've got some ideas on how to implement this.
    > > >
    > > > We can add a new field "suspend_called" to dev->power.
    > >
    > > I'd call it "sleeping" or something like this, for it will also be used by
    > > hibernation callbacks.
    >
    > The name refers to the "suspend" method, not the type of sleep being
    > carried out. We use the same method for both suspend and hibernation.

    We won't in the future.

    > But maybe "sleeping" would be better.
    >
    > > > It would be owned by the PM core (protect by dpm_list_mtx) and read-only to
    > > > drivers. Normally it will contain 0, but when the suspend method is
    > > > running we set it to SUSPEND_RUNNING and when the method returns
    > > > successfully we set it to SUSPEND_DONE. Before calling the resume
    > > > method we set it back to 0.
    > >
    > > Why before? I'd think that any non-suspended children should not be visible
    > > by the partent's ->resume().
    >
    > All right, we can set it to RESUME_RUNNING before calling the resume
    > method and then set it to 0 afterwards. The point is that the value
    > shouldn't remain SUSPEND_DONE while resume runs, because it should be
    > legal for resume to register new children.

    I'm not sure. The core moves the device to dpm_active only after ->resume()
    has run. Thus, if ->resume() registers new children, the ordering of
    dpm_active will be wrong.

    > > > When a new device is registered we check its parent's suspend_called
    > > > value. If it is SUSPEND_DONE then the caller has a bug and we have to
    > > > fail the registration. If it is SUSPEND_RUNNING then the registration
    > > > is legal, but we remember what happened.
    > >
    > > This seems to require some trickery. Namely, device_add() will notice that
    > > the registration is done concurrently with the running ->suspend() of the
    > > parent and will have to communicate that to dpm_suspend() which is supposed
    > > to resume the master in the next step.
    >
    > It will get noticed in device_pm_add() while holding dpm_list_mtx.
    > The information can be stored in a static private flag
    > "child_added_while_parent_suspends" (or maybe something more terse!).

    Hmm, yes, we can do it this way.

    > > > Then when the currently-running suspend method returns and we reacquire the
    > > > dpm_list_mtx, we will realize that a race was lost.
    > >
    > > How exactly do you want to check that?
    >
    > Check whether child_added_while_parent_suspends is nonzero.
    >
    > > > If the method completed successfully (which it shouldn't) we can resume that
    > > > device immediately without ever taking it off the dpm_active list; but either
    > > > way we should continue the suspend loop. Now the new child will be at
    > > > the end of the dpm_active_list, so it will be suspended before the
    > > > parent is reached again.
    > > >
    > > > This way we can recover from drivers that are willing to suspend their
    > > > device even though there are unsuspended children. The only drawback
    > > > will be that for a short time the child will be active while its parent
    > > > is suspended.
    > >
    > > Well, if the parent is a bus, that will be a problem.
    >
    > Sure. But it won't be the PM core's problem; it will be a bug in the
    > bus's driver. We will print a warning in the log so the bug can be
    > tracked down.
    >
    > > > We should not abort the entire sleep transition simply because we lost
    > > > a race.
    > >
    > > I don't agree here. If we require drivers to prevent such races from happening
    > > and they don't comply, we can give up instead of trying to work around the
    > > non-compilance.
    >
    > You misunderstand.

    Well, I misunderstood indeed.

    > We can't require drivers to prevent these races entirely. As an example, a
    > properly-written, compliant driver might work like this:
    >
    > Task 0 Task 1
    > ------ ------
    > dev->power.sleeping =
    > SUSPEND_RUNNING;
    > Call (drv->suspend)(dev)
    > Register a child below dev
    > suspend method prevents new
    > child registrations
    > suspend method waits for
    > existing registration to
    > finish
    > Check dev->power.sleeping and set
    > child_added_while_parent_suspends
    > Registration completes successfully
    > suspend method sees there is
    > an unsuspended child and
    > returns -EBUSY
    >
    > Check child_added_while_parent_suspends
    > and realize that we lost the race
    >
    > There's nothing illegal about this; it's just an accident of timing.
    > Nothing has gone wrong and we shouldn't abort the sleep. We should
    > continue where we left off, by suspending the new child and then trying
    > to suspend the parent again.
    >
    > > > With this scheme we won't even need the pm_sleep_rwsem; the
    > > > dpm_list_mtx will provide all the necessary protection.
    > > >
    > > > This is more intricate than it should be. It would have been better to
    > > > have had "disable_new_children" and "enable_new_children" methods from
    > > > the beginning; then there wouldn't be any races at all. That's life...
    > > >
    > > > The one tricky thing to watch out for is when a suspend or resume
    > > > method wants to unregister the device being suspended or resumed.
    > >
    > > That can't happen, because dev->sem is taken by suspend_device() and
    > > device_del() would lock up attempting to acquire it once again.
    >
    > We'll have to fix device_del() to prevent that from happening. Your
    > in_sleep_context() approach should work.

    I'm not sure if we need to do it. It's always been like this, so the current
    drivers' ->suspend() and ->resume() don't unregister the device they're called
    for. I don't see any advantage from doing that for future drivers.

    > > > Unregistration should always be allowed, and registration should be
    > > > allowed whenever the parent isn't suspended.
    > >
    > > I'm still thinking that registering while the parent is suspending should not
    > > be allowed.
    >
    > Unfortunately the lack of "prevent_new_children" and
    > "allow_new_children" methods gives us no choice. The example above
    > shows why.

    Yes, it does.

    Thanks,
    Rafael


    \
     
     \ /
      Last update: 2008-02-27 20:55    [from the cache]
    ©2003-2014 Jasper Spaans. Advertise on this site