| Message ID | 3307221.dy6xvn45xe@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Sat, 15 Dec 2012, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Currently, the PM core disables runtime PM for all devices right > after executing subsystem/driver .suspend() callbacks for them > and re-enables it right before executing subsystem/driver .resume() > callbacks for them. This may lead to problems when there are > two devices such that the .suspend() callback executed for one of > them depends on runtime PM working for the other. In that case, > if runtime PM has already been disabled for the second device, > the first one's .suspend() won't work correctly (and analogously > for resume). > > To make those issues go away, make the PM core disable runtime PM > for devices right before executing subsystem/driver .suspend_late() > callbacks for them and enable runtime PM for them right after > executing subsystem/driver .resume_early() callbacks for them. This > way the potential conflitcs between .suspend_late()/.resume_early() > and their runtime PM counterparts are still prevented from happening, > but the subtle ordering issues related to disabling/enabling runtime > PM for devices during system suspend/resume are much easier to avoid. > > Reported-and-tested-by: Jan-Matthias Braun <jan_braun@gmx.net> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Hi Rafael, just curious what is the reason for resend? Do you want to gather more Acks before pushing this upstream? Thanks.
On Saturday, December 15, 2012 10:16:29 PM Jiri Kosina wrote: > On Sat, 15 Dec 2012, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Currently, the PM core disables runtime PM for all devices right > > after executing subsystem/driver .suspend() callbacks for them > > and re-enables it right before executing subsystem/driver .resume() > > callbacks for them. This may lead to problems when there are > > two devices such that the .suspend() callback executed for one of > > them depends on runtime PM working for the other. In that case, > > if runtime PM has already been disabled for the second device, > > the first one's .suspend() won't work correctly (and analogously > > for resume). > > > > To make those issues go away, make the PM core disable runtime PM > > for devices right before executing subsystem/driver .suspend_late() > > callbacks for them and enable runtime PM for them right after > > executing subsystem/driver .resume_early() callbacks for them. This > > way the potential conflitcs between .suspend_late()/.resume_early() > > and their runtime PM counterparts are still prevented from happening, > > but the subtle ordering issues related to disabling/enabling runtime > > PM for devices during system suspend/resume are much easier to avoid. > > > > Reported-and-tested-by: Jan-Matthias Braun <jan_braun@gmx.net> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Hi Rafael, > > just curious what is the reason for resend? Do you want to gather more > Acks before pushing this upstream? Well, I thought that some people might actually look at it when they found it again in their mailboxes. :-) Thanks, Rafael
On Sun, 16 Dec 2012, Rafael J. Wysocki wrote: > On Saturday, December 15, 2012 10:16:29 PM Jiri Kosina wrote: > > On Sat, 15 Dec 2012, Rafael J. Wysocki wrote: > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Currently, the PM core disables runtime PM for all devices right > > > after executing subsystem/driver .suspend() callbacks for them > > > and re-enables it right before executing subsystem/driver .resume() > > > callbacks for them. This may lead to problems when there are > > > two devices such that the .suspend() callback executed for one of > > > them depends on runtime PM working for the other. In that case, > > > if runtime PM has already been disabled for the second device, > > > the first one's .suspend() won't work correctly (and analogously > > > for resume). > > > > > > To make those issues go away, make the PM core disable runtime PM > > > for devices right before executing subsystem/driver .suspend_late() > > > callbacks for them and enable runtime PM for them right after > > > executing subsystem/driver .resume_early() callbacks for them. This > > > way the potential conflitcs between .suspend_late()/.resume_early() > > > and their runtime PM counterparts are still prevented from happening, > > > but the subtle ordering issues related to disabling/enabling runtime > > > PM for devices during system suspend/resume are much easier to avoid. > > > > > > Reported-and-tested-by: Jan-Matthias Braun <jan_braun@gmx.net> > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Hi Rafael, > > > > just curious what is the reason for resend? Do you want to gather more > > Acks before pushing this upstream? > > Well, I thought that some people might actually look at it when they found it > again in their mailboxes. :-) I did look at it the first time it appeared. It seemed to be okay, but I haven't tried any testing. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16 December 2012 16:29, Alan Stern <stern@rowland.harvard.edu> wrote: > On Sun, 16 Dec 2012, Rafael J. Wysocki wrote: > >> On Saturday, December 15, 2012 10:16:29 PM Jiri Kosina wrote: >> > On Sat, 15 Dec 2012, Rafael J. Wysocki wrote: >> > >> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> > > >> > > Currently, the PM core disables runtime PM for all devices right >> > > after executing subsystem/driver .suspend() callbacks for them >> > > and re-enables it right before executing subsystem/driver .resume() >> > > callbacks for them. This may lead to problems when there are >> > > two devices such that the .suspend() callback executed for one of >> > > them depends on runtime PM working for the other. In that case, >> > > if runtime PM has already been disabled for the second device, >> > > the first one's .suspend() won't work correctly (and analogously >> > > for resume). >> > > >> > > To make those issues go away, make the PM core disable runtime PM >> > > for devices right before executing subsystem/driver .suspend_late() >> > > callbacks for them and enable runtime PM for them right after >> > > executing subsystem/driver .resume_early() callbacks for them. This >> > > way the potential conflitcs between .suspend_late()/.resume_early() >> > > and their runtime PM counterparts are still prevented from happening, >> > > but the subtle ordering issues related to disabling/enabling runtime >> > > PM for devices during system suspend/resume are much easier to avoid. >> > > >> > > Reported-and-tested-by: Jan-Matthias Braun <jan_braun@gmx.net> >> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> > >> > Hi Rafael, >> > >> > just curious what is the reason for resend? Do you want to gather more >> > Acks before pushing this upstream? >> >> Well, I thought that some people might actually look at it when they found it >> again in their mailboxes. :-) > > I did look at it the first time it appeared. It seemed to be okay, but > I haven't tried any testing. > > Alan Stern > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html I believe this should work fine for ux500 platforms, so: Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Ulf Hansson -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, December 17, 2012 10:18:14 PM Ulf Hansson wrote: > On 16 December 2012 16:29, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Sun, 16 Dec 2012, Rafael J. Wysocki wrote: > > > >> On Saturday, December 15, 2012 10:16:29 PM Jiri Kosina wrote: > >> > On Sat, 15 Dec 2012, Rafael J. Wysocki wrote: > >> > > >> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> > > > >> > > Currently, the PM core disables runtime PM for all devices right > >> > > after executing subsystem/driver .suspend() callbacks for them > >> > > and re-enables it right before executing subsystem/driver .resume() > >> > > callbacks for them. This may lead to problems when there are > >> > > two devices such that the .suspend() callback executed for one of > >> > > them depends on runtime PM working for the other. In that case, > >> > > if runtime PM has already been disabled for the second device, > >> > > the first one's .suspend() won't work correctly (and analogously > >> > > for resume). > >> > > > >> > > To make those issues go away, make the PM core disable runtime PM > >> > > for devices right before executing subsystem/driver .suspend_late() > >> > > callbacks for them and enable runtime PM for them right after > >> > > executing subsystem/driver .resume_early() callbacks for them. This > >> > > way the potential conflitcs between .suspend_late()/.resume_early() > >> > > and their runtime PM counterparts are still prevented from happening, > >> > > but the subtle ordering issues related to disabling/enabling runtime > >> > > PM for devices during system suspend/resume are much easier to avoid. > >> > > > >> > > Reported-and-tested-by: Jan-Matthias Braun <jan_braun@gmx.net> > >> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> > > >> > Hi Rafael, > >> > > >> > just curious what is the reason for resend? Do you want to gather more > >> > Acks before pushing this upstream? > >> > >> Well, I thought that some people might actually look at it when they found it > >> again in their mailboxes. :-) > > > > I did look at it the first time it appeared. It seemed to be okay, but > > I haven't tried any testing. > > > > Alan Stern > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > I believe this should work fine for ux500 platforms, so: > > Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org> Thanks!
"Rafael J. Wysocki" <rjw@sisk.pl> writes: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Currently, the PM core disables runtime PM for all devices right > after executing subsystem/driver .suspend() callbacks for them > and re-enables it right before executing subsystem/driver .resume() > callbacks for them. This may lead to problems when there are > two devices such that the .suspend() callback executed for one of > them depends on runtime PM working for the other. In that case, > if runtime PM has already been disabled for the second device, > the first one's .suspend() won't work correctly (and analogously > for resume). > > To make those issues go away, make the PM core disable runtime PM > for devices right before executing subsystem/driver .suspend_late() > callbacks for them and enable runtime PM for them right after > executing subsystem/driver .resume_early() callbacks for them. This > way the potential conflitcs between .suspend_late()/.resume_early() > and their runtime PM counterparts are still prevented from happening, > but the subtle ordering issues related to disabling/enabling runtime > PM for devices during system suspend/resume are much easier to avoid. > > Reported-and-tested-by: Jan-Matthias Braun <jan_braun@gmx.net> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Yes! Of course if there are dependencies between drivers late/early callbacks, we'll still have the same problems, but those should be *very* rare compared to the suspend/resume dependencies. I haven't been able to do any testing with this yet (I'm away from my hardware for a bit), but I totally support this change. Reviewed-by: Kevin Hilman <khilman@deeprootsystems.com> Thanks! Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, December 21, 2012 11:52:56 AM Kevin Hilman wrote: > "Rafael J. Wysocki" <rjw@sisk.pl> writes: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Currently, the PM core disables runtime PM for all devices right > > after executing subsystem/driver .suspend() callbacks for them > > and re-enables it right before executing subsystem/driver .resume() > > callbacks for them. This may lead to problems when there are > > two devices such that the .suspend() callback executed for one of > > them depends on runtime PM working for the other. In that case, > > if runtime PM has already been disabled for the second device, > > the first one's .suspend() won't work correctly (and analogously > > for resume). > > > > To make those issues go away, make the PM core disable runtime PM > > for devices right before executing subsystem/driver .suspend_late() > > callbacks for them and enable runtime PM for them right after > > executing subsystem/driver .resume_early() callbacks for them. This > > way the potential conflitcs between .suspend_late()/.resume_early() > > and their runtime PM counterparts are still prevented from happening, > > but the subtle ordering issues related to disabling/enabling runtime > > PM for devices during system suspend/resume are much easier to avoid. > > > > Reported-and-tested-by: Jan-Matthias Braun <jan_braun@gmx.net> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Yes! > > Of course if there are dependencies between drivers late/early > callbacks, we'll still have the same problems, but those should be > *very* rare compared to the suspend/resume dependencies. Well, let's put it this way: Any dependencies between drivers' late/early callbacks that make things break because runtime PM is disabled at that time are bugs that need to be fixed. Why? Because runtime PM has always been disabled during late/early callbacks since they were introduced and if somebody conveniently ignored that, then this is this person's problem entirely. > I haven't been able to do any testing with this yet (I'm away from my > hardware for a bit), but I totally support this change. > > Reviewed-by: Kevin Hilman <khilman@deeprootsystems.com> Thanks! Rafael
Index: linux/drivers/base/power/main.c =================================================================== --- linux.orig/drivers/base/power/main.c +++ linux/drivers/base/power/main.c @@ -513,6 +513,8 @@ static int device_resume_early(struct de Out: TRACE_RESUME(error); + + pm_runtime_enable(dev); return error; } @@ -589,8 +591,6 @@ static int device_resume(struct device * if (!dev->power.is_suspended) goto Unlock; - pm_runtime_enable(dev); - if (dev->pm_domain) { info = "power domain "; callback = pm_op(&dev->pm_domain->ops, state); @@ -930,6 +930,8 @@ static int device_suspend_late(struct de pm_callback_t callback = NULL; char *info = NULL; + __pm_runtime_disable(dev, false); + if (dev->power.syscore) return 0; @@ -1133,11 +1135,8 @@ static int __device_suspend(struct devic Complete: complete_all(&dev->power.completion); - if (error) async_error = error; - else if (dev->power.is_suspended) - __pm_runtime_disable(dev, false); return error; } Index: linux/Documentation/power/runtime_pm.txt =================================================================== --- linux.orig/Documentation/power/runtime_pm.txt +++ linux/Documentation/power/runtime_pm.txt @@ -642,12 +642,13 @@ out the following operations: * During system suspend it calls pm_runtime_get_noresume() and pm_runtime_barrier() for every device right before executing the subsystem-level .suspend() callback for it. In addition to that it calls - pm_runtime_disable() for every device right after executing the - subsystem-level .suspend() callback for it. + __pm_runtime_disable() with 'false' as the second argument for every device + right before executing the subsystem-level .suspend_late() callback for it. * During system resume it calls pm_runtime_enable() and pm_runtime_put_sync() - for every device right before and right after executing the subsystem-level - .resume() callback for it, respectively. + for every device right after executing the subsystem-level .resume_early() + callback and right after executing the subsystem-level .resume() callback + for it, respectively. 7. Generic subsystem callbacks