lkml.org 
[lkml]   [2008]   [Feb]   [26]   [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 Tuesday, 26 of February 2008, Alan Stern wrote:
> On Tue, 26 Feb 2008, Rafael J. Wysocki wrote:
>
> > > > IMO the device driver should assure that no new children will be registered
> > > > concurrently with the ->suspend() method (IOW, ->suspend() should wait for
> > > > all such registrations to complete and should prevent any new ones from
> > > > being started) and it should make it impossible to register any new children
> > > > after ->suspend() has run. It's the driver's problem how to achieve that.
> > >
> > > Exactly; this has to be added to the PM documentation.
> >
> > Into Documentation/power/devices.txt, I gather?
>
> Yes.
>
> > > > > The PM core could help detect errors here. If it tries to suspend a
> > > > > device and sees that the device's parent is already suspended, then the
> > > > > parent's driver has a bug.
> > > >
> > > > Yes, I think we ought to fail the suspend in such cases. Still, that's not
> > > > sufficient to prevent a child from being registered after we've run
> > > > dpm_suspend(). For this reason, we could also leave dpm_suspend() with
> > > > dpm_list_mtx held and not release it until the next dpm_resume() is run.
> > >
> > > The pm_sleep_rwsem will do a better job of catching such errors.
> >
> > But we should not leave a window between releasing dpm_list_mtx and taking
> > pm_sleep_rwsem. Either that, or we should make sure that dpm_active is
> > empty after acquiring pm_sleep_rwsem.
>
> 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.

> 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().

> Drivers can use this field as an easy way of checking that all the child
> devices have been suspended.
>
> 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.

> 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?

> 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.

> 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.

> 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.

> Even that should be doable (set suspend_called to UNREGISTERED or something
> like that).
>
> > > > That will potentially cause some trouble to CPU hotplug cotifiers, but we can
> > > > handle that, for example, by using the in_suspend_context() test.
> > >
> > > Do they need to register new CPUs at some point? There ought to be a
> > > way to handle that.
> >
> > No, they don't, but there are some CPU-related device objects that get
> > uregistered/registered. Still, all of this work is really redundant if the CPU
> > in question comes back up during the resume, so it should be avoided in
> > general. The CPU hotplug notifiers should only unregister those objects if
> > the CPU hasn't gone on line during the resume and they have all information
> > necessary for discovering that.
>
> 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.

> For devices with no parent, we can imagine there is a fictitious parent at
> the root of the device tree. Conceptually it gets suspended after every real
> device and resumed before. Maybe even before dpm_power_up(), meaning that
> devices with no parent could be registered by a resume_early method.
>
> When your lock-removal stuff gets into Greg's tree, I'll write all
> this. Sound good?

The direction seems to be fine, but the details need a bit more clarification,
as far as I'm concerned. Having a patch to discuss will certainly help a lot,
though. ;-)

Thanks,
Rafael


\
 
 \ /
  Last update: 2008-02-27 00:21    [W:0.400 / U:0.408 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site