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 _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
