| Message ID | 2381177.mrXhZ4Wotn@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Fri, 25 Apr 2014, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to > resume all runtime-suspended devices during system suspend, mostly > because those devices may need to be reprogrammed due to different > wakeup settings for system sleep and for runtime PM. However, at > least in some cases, that isn't really necessary, because the wakeup > settings may not be really different. > > The idea here is that subsystems should know whether or not it is > necessary to reprogram a given device during system suspend and they > should be able to tell the PM core about that. For that reason, add > two new device PM flags, power.resume_not_needed and > power.use_runtime_resume, such that: > > (1) If power.resume_not_needed is set for the given device and for > all of its children and the device is runtime-suspended during > device_suspend(), the remaining device's system suspend/resume > callbacks need not be executed. The patch doesn't do that last part. That is, it still invokes the callbacks even when resume_not_needed is set. I'm not sure that you should skip the resume callbacks. We expect devices to be resumed during system resume even if they were in runtime suspend beforehand. Yes, this means calling resume_noirq without calling suspend_noirq (ditto for the other callbacks). Think of it as a form of optimization -- we could have called suspend_noirq, but we know that the subsystem would have seen that the device was already in runtime suspend and then returned immediately. By not calling suspend_noirq, we spare the subsystem from testing the runtime status. (And I also think "resume_not_needed" is too general. It should be something more like "runtime_resume_not_needed_during_system_suspend", only not quite so long.) > (2) If power.use_runtime_resume is set for the given device and the > device is runtime-suspended in device_suspend_late(), its late/early > and noirq system suspend/resume callbacks should be skipped and > it should be resumed through pm_runtime_resume() in device_resume(). IMO this should be a separate patch. It has no direct connection with the main goal of providing subsystems with a mechanism to avoid waking up devices for reprogramming during system suspend. The main goal of this other patch will be to allow devices which were in runtime suspend throughout the system suspend phases to remain in runtime suspend throughout the system resume. When they do finally get resumed, it will be by a ->runtime_resume() callback, not ->resume(). This will require coordination with the child devices. If the child expects the parent always to be resumed during the resume_early phase, we mustn't skip the resume_early callback. I'm not sure if the coordination provided by the resume_not_needed flag is sufficient. ("use_runtime_resume" is also too general. "remain_in_runtime_suspend_during_system_resume"?) 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 Thursday, May 01, 2014 05:39:31 PM Alan Stern wrote: > On Fri, 25 Apr 2014, Rafael J. Wysocki wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to > > resume all runtime-suspended devices during system suspend, mostly > > because those devices may need to be reprogrammed due to different > > wakeup settings for system sleep and for runtime PM. However, at > > least in some cases, that isn't really necessary, because the wakeup > > settings may not be really different. > > > > The idea here is that subsystems should know whether or not it is > > necessary to reprogram a given device during system suspend and they > > should be able to tell the PM core about that. For that reason, add > > two new device PM flags, power.resume_not_needed and > > power.use_runtime_resume, such that: > > > > (1) If power.resume_not_needed is set for the given device and for > > all of its children and the device is runtime-suspended during > > device_suspend(), the remaining device's system suspend/resume > > callbacks need not be executed. > > The patch doesn't do that last part. That is, it still invokes the > callbacks even when resume_not_needed is set. Yes, it does and the above paragraph doesn't say that it won't. It only says that the flag indicates no need to do that. > I'm not sure that you should skip the resume callbacks. The reason why is that if the device were suspended using ->runtime_suspend(), its driver (or bus type) may be confused if ->runtime_resume() is not used to resume it. > We expect devices to be resumed during system resume even if they were in > runtime suspend beforehand. Yes, this means calling resume_noirq without > calling suspend_noirq (ditto for the other callbacks). Think of it as > a form of optimization -- we could have called suspend_noirq, but we > know that the subsystem would have seen that the device was already in > runtime suspend and then returned immediately. By not calling > suspend_noirq, we spare the subsystem from testing the runtime status. The driver/subsystem has the right to expect that ->resume_noirq() will always be called after ->suspend_noirq() (if both are implemented). Thus calling the former without the latter may be a bug (for the particular driver). Since we've always had symmetry there, the risk of breaking stuff if we change that is relatively high. On the other hand, ->runtime_resume() should not make any assumptions about the hardware state when it is called anyway, so it should handle the device during system resume properly, given that the last PM callback executed for it was ->runtime_suspend(), regardless of what the firmware did to it in the meantime. > (And I also think "resume_not_needed" is too general. It should be > something more like "runtime_resume_not_needed_during_system_suspend", > only not quite so long.) Well, I can agree with that, but I couldn't invent a better name. In particular, one that wouldn't be too long. :-) > > (2) If power.use_runtime_resume is set for the given device and the > > device is runtime-suspended in device_suspend_late(), its late/early > > and noirq system suspend/resume callbacks should be skipped and > > it should be resumed through pm_runtime_resume() in device_resume(). > > IMO this should be a separate patch. It has no direct connection with > the main goal of providing subsystems with a mechanism to avoid waking > up devices for reprogramming during system suspend. I'm not sure what you mean, honestly. The main goal here is to allow devices that were runtime suspended to stay that way throughout system suspend and resume up until device_resume() is called for them. > The main goal of this other patch will be to allow devices which were > in runtime suspend throughout the system suspend phases to remain in > runtime suspend throughout the system resume. When they do finally get > resumed, it will be by a ->runtime_resume() callback, not ->resume(). Yes, and that's what *this* patch does, isn't it? I have no idea how to split it so that both parts still make sense. > This will require coordination with the child devices. If the > child expects the parent always to be resumed during the resume_early > phase, we mustn't skip the resume_early callback. I'm not sure if the > coordination provided by the resume_not_needed flag is sufficient. Setting resume_not_needed means "if I'm runtime-suspended, I don't mind if you leave me like that". If the child is resumed during system suspend, the parent will have to be resumed either and they will both go through system suspend-resume callbacks. Otherwise, the child will be resumed by its ->runtime_resume() in device_resume() - after the parent. I don't think that any more coordination is needed. > ("use_runtime_resume" is also too general. > "remain_in_runtime_suspend_during_system_resume"?) Well, again, I have problems with inventing names for those things. Rafael -- 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, May 02, 2014 01:15:14 AM Rafael J. Wysocki wrote: > On Thursday, May 01, 2014 05:39:31 PM Alan Stern wrote: > > On Fri, 25 Apr 2014, Rafael J. Wysocki wrote: > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to > > > resume all runtime-suspended devices during system suspend, mostly > > > because those devices may need to be reprogrammed due to different > > > wakeup settings for system sleep and for runtime PM. However, at > > > least in some cases, that isn't really necessary, because the wakeup > > > settings may not be really different. > > > > > > The idea here is that subsystems should know whether or not it is > > > necessary to reprogram a given device during system suspend and they > > > should be able to tell the PM core about that. For that reason, add > > > two new device PM flags, power.resume_not_needed and > > > power.use_runtime_resume, such that: > > > > > > (1) If power.resume_not_needed is set for the given device and for > > > all of its children and the device is runtime-suspended during > > > device_suspend(), the remaining device's system suspend/resume > > > callbacks need not be executed. > > > > The patch doesn't do that last part. That is, it still invokes the > > callbacks even when resume_not_needed is set. > > Yes, it does and the above paragraph doesn't say that it won't. It only > says that the flag indicates no need to do that. > > > I'm not sure that you should skip the resume callbacks. > > The reason why is that if the device were suspended using ->runtime_suspend(), > its driver (or bus type) may be confused if ->runtime_resume() is not used > to resume it. > > > We expect devices to be resumed during system resume even if they were in > > runtime suspend beforehand. Yes, this means calling resume_noirq without > > calling suspend_noirq (ditto for the other callbacks). Think of it as > > a form of optimization -- we could have called suspend_noirq, but we > > know that the subsystem would have seen that the device was already in > > runtime suspend and then returned immediately. By not calling > > suspend_noirq, we spare the subsystem from testing the runtime status. > > The driver/subsystem has the right to expect that ->resume_noirq() will > always be called after ->suspend_noirq() (if both are implemented). Thus > calling the former without the latter may be a bug (for the particular > driver). Since we've always had symmetry there, the risk of breaking stuff > if we change that is relatively high. > > On the other hand, ->runtime_resume() should not make any assumptions > about the hardware state when it is called anyway, so it should handle > the device during system resume properly, given that the last PM callback > executed for it was ->runtime_suspend(), regardless of what the firmware > did to it in the meantime. > > > (And I also think "resume_not_needed" is too general. It should be > > something more like "runtime_resume_not_needed_during_system_suspend", > > only not quite so long.) > > Well, I can agree with that, but I couldn't invent a better name. In particular, > one that wouldn't be too long. :-) > > > > (2) If power.use_runtime_resume is set for the given device and the > > > device is runtime-suspended in device_suspend_late(), its late/early > > > and noirq system suspend/resume callbacks should be skipped and > > > it should be resumed through pm_runtime_resume() in device_resume(). > > > > IMO this should be a separate patch. It has no direct connection with > > the main goal of providing subsystems with a mechanism to avoid waking > > up devices for reprogramming during system suspend. > > I'm not sure what you mean, honestly. The main goal here is to allow > devices that were runtime suspended to stay that way throughout system > suspend and resume up until device_resume() is called for them. > > > The main goal of this other patch will be to allow devices which were > > in runtime suspend throughout the system suspend phases to remain in > > runtime suspend throughout the system resume. When they do finally get > > resumed, it will be by a ->runtime_resume() callback, not ->resume(). > > Yes, and that's what *this* patch does, isn't it? > > I have no idea how to split it so that both parts still make sense. OK, I guess you mean that the first flag (resume_not_needed) should be introduced by one patch and the second one by another, but I'm not sure if that would really make any difference. Both of them are needed anyway and resume_not_needed without the other flag wouldn't have any effect. Well, perhaps I should make __device_suspend() check that use_runtime_resume is only set when resume_not_needed is set too and return an error if that's not the case. Rafael -- 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 Fri, 2 May 2014, Rafael J. Wysocki wrote: > On Friday, May 02, 2014 01:15:14 AM Rafael J. Wysocki wrote: > > On Thursday, May 01, 2014 05:39:31 PM Alan Stern wrote: > > > On Fri, 25 Apr 2014, Rafael J. Wysocki wrote: > > > > > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > > > > > Currently, some subsystems (e.g. PCI and the ACPI PM domain) have to > > > > resume all runtime-suspended devices during system suspend, mostly > > > > because those devices may need to be reprogrammed due to different > > > > wakeup settings for system sleep and for runtime PM. However, at > > > > least in some cases, that isn't really necessary, because the wakeup > > > > settings may not be really different. > > > > > > > > The idea here is that subsystems should know whether or not it is > > > > necessary to reprogram a given device during system suspend and they > > > > should be able to tell the PM core about that. For that reason, add > > > > two new device PM flags, power.resume_not_needed and > > > > power.use_runtime_resume, such that: > > > > > > > > (1) If power.resume_not_needed is set for the given device and for > > > > all of its children and the device is runtime-suspended during > > > > device_suspend(), the remaining device's system suspend/resume > > > > callbacks need not be executed. > > > > > > The patch doesn't do that last part. That is, it still invokes the > > > callbacks even when resume_not_needed is set. > > > > Yes, it does and the above paragraph doesn't say that it won't. It only > > says that the flag indicates no need to do that. But then the reader is left wondering: What is the reason for having the flag, if it doesn't cause the PM core to do anything different? This is partly a weakness of the patch description. Here's a suggestion for an improved description: -------------------------------------------------------------------- Many devices have different wakeup settings for runtime suspend and system suspend. If such a device is already runtime-suspended when a system suspend starts, it will be necessary to perform a runtime resume in order to reprogram the device's wakeup settings. Thus, the device will always enter system suspend in a runtime-active state. Other devices may need to be runtime-active when a system suspend starts for other reasons. Still, there are quite a few devices which have no such requirements. If they are already in runtime suspend, there's no reason why they shouldn't remain that way during the system suspend. And avoiding an extra runtime-resume/device-suspend pair could help speed up the procedure. Some subsystems, such as PCI and the ACPI PM domain, automatically resume all runtime-suspended devices when a system suspend starts, on the theory that the device may require reprogramming. As described above, we would like to avoid doing this in cases where it isn't needed. This patch introduces a standardized way for drivers to tell their subsystems that no reprogramming is needed and a device may remain in runtime suspend during a system suspend. A new flag, dev->power.resume_not_needed, is added. Drivers set this flag during their ->prepare() callbacks to tell the subsystems that no runtime resume is necessary. If the flag is set and the device is runtime-suspended, the subsystem can return immediately from its ->suspend(), ->suspend_late(), and ->suspend_noirq() callbacks. (However, the various resume callbacks should continue to function as before, because we want the device to end up at full power when the system resume is over.) Note: If dev->power.ignore_children is set then dev->power.resume_not_needed probably should not be set. This is because the driver for the child device may expect dev always to be at full power when the child's suspend routines run. -------------------------------------------------------------------- This description doesn't say anything about the PM core skipping the various suspend callbacks. That's because the patch doesn't actually skip them. It also doesn't say anything about requiring resume_not_needed to be set in all the descendant devices. That's because this isn't necessary, if all you want to accomplish is to avoid the unnecessary runtime resumes. > > > > (2) If power.use_runtime_resume is set for the given device and the > > > > device is runtime-suspended in device_suspend_late(), its late/early > > > > and noirq system suspend/resume callbacks should be skipped and > > > > it should be resumed through pm_runtime_resume() in device_resume(). > > > > > > IMO this should be a separate patch. It has no direct connection with > > > the main goal of providing subsystems with a mechanism to avoid waking > > > up devices for reprogramming during system suspend. > > > > I'm not sure what you mean, honestly. The main goal here is to allow > > devices that were runtime suspended to stay that way throughout system > > suspend and resume up until device_resume() is called for them. No, not exactly. The main goal is to allow devices that were runtime suspended to stay that way throughout system suspend _only_. We expect system resume to function as it has in the past. Changing the expectations for system resume should be the subject of a second patch. > > I have no idea how to split it so that both parts still make sense. > > OK, I guess you mean that the first flag (resume_not_needed) should be introduced > by one patch and the second one by another, but I'm not sure if that would really > make any difference. Both of them are needed anyway and resume_not_needed without > the other flag wouldn't have any effect. > > Well, perhaps I should make __device_suspend() check that use_runtime_resume is > only set when resume_not_needed is set too and return an error if that's not > the case. Here's my suggestion for the second patch's description. It describes what I have in mind: -------------------------------------------------------------------- Now that we have a mechanism for allowing devices to remain in a runtime-suspended state during system suspend, it makes sense to add a mechanism allowing them to remain in runtime suspend during system resume as well -- in other words, throughout an entire system suspend cycle. In theory, all the PM core has to do is avoid invoking the suspend and resume callbacks for devices that are runtime suspended. However, some drivers may not be prepared to handle this new behavior. They may assume that their various resume callbacks always get called, regardless of the device's runtime status, because that's how it works now. Or they may assume that the device's parent is always at full power when the device's resume callbacks run, which wouldn't have to hold if the parent's power.ignore_children flag was set. Therefore this patch adds a new flag: dev->power.use_runtime_resume. Drivers can set this flag in their ->prepare() routines. When __device_suspend() runs, if the device and all its descendants are runtime suspended, and if power.use_runtime_resume is set in the device and all its descendants, then the PM core will skip the ->suspend(), ->suspend_late(), ->suspend_noirq(), ->resume_noirq(), ->resume_early(), and ->resume() callbacks. The device will not return to full power until a runtime resume occurs. -------------------------------------------------------------------- This isn't exactly the same as what you implemented, but I think the description explains the situation well enough that the reasons for the differences are clear. 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
Index: linux-pm/include/linux/pm.h =================================================================== --- linux-pm.orig/include/linux/pm.h +++ linux-pm/include/linux/pm.h @@ -546,6 +546,8 @@ struct dev_pm_info { bool is_late_suspended:1; bool ignore_children:1; bool early_init:1; /* Owned by the PM core */ + bool resume_not_needed:1; + bool use_runtime_resume:1; spinlock_t lock; #ifdef CONFIG_PM_SLEEP struct list_head entry; Index: linux-pm/drivers/base/power/main.c =================================================================== --- linux-pm.orig/drivers/base/power/main.c +++ linux-pm/drivers/base/power/main.c @@ -479,7 +479,7 @@ static int device_resume_noirq(struct de TRACE_DEVICE(dev); TRACE_RESUME(0); - if (dev->power.syscore) + if (dev->power.syscore || dev->power.use_runtime_resume) goto Out; if (!dev->power.is_noirq_suspended) @@ -605,7 +605,7 @@ static int device_resume_early(struct de TRACE_DEVICE(dev); TRACE_RESUME(0); - if (dev->power.syscore) + if (dev->power.syscore || dev->power.use_runtime_resume) goto Out; if (!dev->power.is_late_suspended) @@ -735,6 +735,11 @@ static int device_resume(struct device * if (dev->power.syscore) goto Complete; + if (dev->power.use_runtime_resume) { + pm_runtime_resume(dev); + goto Complete; + } + dpm_wait(dev->parent, async); dpm_watchdog_set(&wd, dev); device_lock(dev); @@ -1007,7 +1012,7 @@ static int __device_suspend_noirq(struct goto Complete; } - if (dev->power.syscore) + if (dev->power.syscore || dev->power.use_runtime_resume) goto Complete; dpm_wait_for_children(dev, async); @@ -1146,7 +1151,7 @@ static int __device_suspend_late(struct goto Complete; } - if (dev->power.syscore) + if (dev->power.syscore || dev->power.use_runtime_resume) goto Complete; dpm_wait_for_children(dev, async); @@ -1383,9 +1388,29 @@ static int __device_suspend(struct devic End: if (!error) { dev->power.is_suspended = true; - if (dev->power.wakeup_path - && dev->parent && !dev->parent->power.ignore_children) - dev->parent->power.wakeup_path = true; + if (dev->parent) { + spin_lock_irq(&dev->parent->power.lock); + + if (dev->power.wakeup_path + && !dev->parent->power.ignore_children) + dev->parent->power.wakeup_path = true; + + /* + * Subsystems are supposed to set resume_not_needed in + * their ->prepare() callbacks for devices whose + * remaining system suspend and resume callbacks are + * generally safe to be skipped if the devices are + * runtime-suspended. + * + * If the device's resume_not_needed is not set at this + * point, it has to be cleared for its parent too (even + * if the subsystem has set that flag for it). + */ + if (!dev->power.resume_not_needed) + dev->parent->power.resume_not_needed = false; + + spin_unlock_irq(&dev->parent->power.lock); + } } device_unlock(dev); @@ -1553,6 +1578,8 @@ int dpm_prepare(pm_message_t state) struct device *dev = to_device(dpm_list.next); get_device(dev); + dev->power.use_runtime_resume = false; + dev->power.resume_not_needed = false; mutex_unlock(&dpm_list_mtx); error = device_prepare(dev, state);