| Message ID | 7htxblx2eo.fsf@paris.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26 February 2014 17:30, Kevin Hilman <khilman@linaro.org> wrote: > Ulf Hansson <ulf.hansson@linaro.org> writes: > >> Patch 1 -> 2: >> These patches provides two new runtime PM helper functions which intend to be >> used from system suspend/resume callbacks, to make sure devices are put into low >> power state during system suspend and brought back to full power at system >> resume. >> >> The prerequisite is to have all levels of a device's runtime PM callbacks to be >> defined through the SET_PM_RUNTIME_PM_OPS macro, which means these are available >> for CONFIG_PM. >> >> By using the new runtime PM helper functions especially the two scenarios below >> will be addressed. >> >> 1) The PM core prevents .runtime_suspend callbacks from being invoked during >> system suspend. That means even for a runtime PM centric subsystem and driver, >> the device needs to be put into low power state from a system suspend callback. >> Otherwise it may very well be left in full power state (runtime resumed) while >> the system is suspended. By using the new helper functions, we make sure to walk >> the hierarchy of a device's power domain, subsystem and driver. > > I thought it was the case that runtime PM was only disabled during the > 'late' phase now. Isn't that enough to allow runtime callbacks in the > normal suspend/resume hooks now? /me looks. I am not sure, I get your point here. The PM core disables runtime PM at suspend_late. That is somewhat not related to this patch, since the helper functions are supposed to work standalone. Subsystem/drivers will in some cases need to invoke the helper functions in an earlier phase than suspend_late, thus those can not rely on runtime PM to be disabled, but need to handle that themselves. > > oh, wait. Ee still have the _get_noresume() in device_prepare(). hmm > > Either way, I'm not not a big fan of new functions. Personally, I think > subsystems/busses/pm_domains should be able to opt out of the PM core > behavior that blocks runtime PM transitions during system suspend. > Something like the (untested) hack below. That way, we could avoid the > need for new helper functions. > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > index 1b41fca3d65a..e0770009ba8e 100644 > --- a/drivers/base/power/main.c > +++ b/drivers/base/power/main.c > @@ -832,7 +832,8 @@ static void device_complete(struct device *dev, pm_message_t state) > > device_unlock(dev); > > - pm_runtime_put(dev); > + if (dev->power.block_rpm_during_suspend) > + pm_runtime_put(dev); > } > > /** > @@ -1318,7 +1319,8 @@ static int device_prepare(struct device *dev, pm_message_t state) > * block runtime suspend here, during the prepare phase, and allow > * it again during the complete phase. > */ > - pm_runtime_get_noresume(dev); > + if (dev->power.block_rpm_during_suspend) > + pm_runtime_get_noresume(dev); > > device_lock(dev); > > @@ -1350,7 +1352,7 @@ static int device_prepare(struct device *dev, pm_message_t state) > > device_unlock(dev); > > - if (error) > + if (error && dev->power.block_rpm_during_suspend) > pm_runtime_put(dev); > > return error; > diff --git a/include/linux/pm.h b/include/linux/pm.h > index 8c6583a53a06..692cd543b71d 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -551,6 +551,7 @@ struct dev_pm_info { > struct wakeup_source *wakeup; > bool wakeup_path:1; > bool syscore:1; > + unsigned int block_rpm_during_suspend:1; > #else > unsigned int should_wakeup:1; > #endif This approach is what I initially started with earlier this autumn - primarily to kick off the discussion. :-) This is not the way to go, there are several reasons. Alan and Rafael, should be given cred for being so patient with me while the pointed out the reasons. Please have look at the discussion below. http://marc.info/?t=138436024400005&r=1&w=2 Kind regards Uffe > > Kevin
On Wednesday, February 26, 2014 08:30:07 AM Kevin Hilman wrote: > Ulf Hansson <ulf.hansson@linaro.org> writes: > > > Patch 1 -> 2: > > These patches provides two new runtime PM helper functions which intend to be > > used from system suspend/resume callbacks, to make sure devices are put into low > > power state during system suspend and brought back to full power at system > > resume. > > > > The prerequisite is to have all levels of a device's runtime PM callbacks to be > > defined through the SET_PM_RUNTIME_PM_OPS macro, which means these are available > > for CONFIG_PM. > > > > By using the new runtime PM helper functions especially the two scenarios below > > will be addressed. > > > > 1) The PM core prevents .runtime_suspend callbacks from being invoked during > > system suspend. That means even for a runtime PM centric subsystem and driver, > > the device needs to be put into low power state from a system suspend callback. > > Otherwise it may very well be left in full power state (runtime resumed) while > > the system is suspended. By using the new helper functions, we make sure to walk > > the hierarchy of a device's power domain, subsystem and driver. > > I thought it was the case that runtime PM was only disabled during the > 'late' phase now. Isn't that enough to allow runtime callbacks in the > normal suspend/resume hooks now? /me looks. > > oh, wait. Ee still have the _get_noresume() in device_prepare(). hmm > > Either way, I'm not not a big fan of new functions. Personally, I think > subsystems/busses/pm_domains should be able to opt out of the PM core > behavior that blocks runtime PM transitions during system suspend. > Something like the (untested) hack below. That way, we could avoid the > need for new helper functions. And if one of the subsystems in question is the platform bus type, then adding the flag doesn't really make sense, because that means that on many system it will be set for the majority of devices. :-) That said I'm tired of this stuff already. If you really really want to remove the bumping up and dropping of the usage counter during system suspend/resume by the core, please feel free to submit a patch for that and I'll apply it. However, if it causes any regressions to happen anywhere, it will be dropped and we'll never talk about this again. Deal?
On 27 February 2014 02:22, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Wednesday, February 26, 2014 08:30:07 AM Kevin Hilman wrote: >> Ulf Hansson <ulf.hansson@linaro.org> writes: >> >> > Patch 1 -> 2: >> > These patches provides two new runtime PM helper functions which intend to be >> > used from system suspend/resume callbacks, to make sure devices are put into low >> > power state during system suspend and brought back to full power at system >> > resume. >> > >> > The prerequisite is to have all levels of a device's runtime PM callbacks to be >> > defined through the SET_PM_RUNTIME_PM_OPS macro, which means these are available >> > for CONFIG_PM. >> > >> > By using the new runtime PM helper functions especially the two scenarios below >> > will be addressed. >> > >> > 1) The PM core prevents .runtime_suspend callbacks from being invoked during >> > system suspend. That means even for a runtime PM centric subsystem and driver, >> > the device needs to be put into low power state from a system suspend callback. >> > Otherwise it may very well be left in full power state (runtime resumed) while >> > the system is suspended. By using the new helper functions, we make sure to walk >> > the hierarchy of a device's power domain, subsystem and driver. >> >> I thought it was the case that runtime PM was only disabled during the >> 'late' phase now. Isn't that enough to allow runtime callbacks in the >> normal suspend/resume hooks now? /me looks. >> >> oh, wait. Ee still have the _get_noresume() in device_prepare(). hmm >> >> Either way, I'm not not a big fan of new functions. Personally, I think >> subsystems/busses/pm_domains should be able to opt out of the PM core >> behavior that blocks runtime PM transitions during system suspend. >> Something like the (untested) hack below. That way, we could avoid the >> need for new helper functions. > > And if one of the subsystems in question is the platform bus type, then > adding the flag doesn't really make sense, because that means that on > many system it will be set for the majority of devices. :-) > > That said I'm tired of this stuff already. If you really really want to > remove the bumping up and dropping of the usage counter during system > suspend/resume by the core, please feel free to submit a patch for that > and I'll apply it. However, if it causes any regressions to happen > anywhere, it will be dropped and we'll never talk about this again. > > Deal? No deal. I prefer the runtime PM helpers, I think that is the right approach. I also believe this is what Alan Stern also would prefer, right!? So let's convince Kevin instead. :-) Let's just be clear of why I don't think Kevin suggested solution is the way to go: 1. The runtime PM sysfs interface. Userspace may prevent ->runtime_suspend callbacks from being invoked anyway. So, it doesn't matter if PM core does it too. 2. Currently we have CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP. Subsystem and driver, must cope with both. I know there were a discussion about combining them as one define, but we rejected that - at least for now. Anyway, my point is, subsystem/driver must not rely on PM runtime core (like pm_runtime_put_sync) from the system suspend callback to put their devices into low power state. Kind regards Uffe > > -- > I speak only for myself. > Rafael J. Wysocki, Intel Open Source Technology Center.
Ulf Hansson <ulf.hansson@linaro.org> writes: > On 27 February 2014 02:22, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> On Wednesday, February 26, 2014 08:30:07 AM Kevin Hilman wrote: >>> Ulf Hansson <ulf.hansson@linaro.org> writes: >>> >>> > Patch 1 -> 2: >>> > These patches provides two new runtime PM helper functions which intend to be >>> > used from system suspend/resume callbacks, to make sure devices are put into low >>> > power state during system suspend and brought back to full power at system >>> > resume. >>> > >>> > The prerequisite is to have all levels of a device's runtime PM callbacks to be >>> > defined through the SET_PM_RUNTIME_PM_OPS macro, which means these are available >>> > for CONFIG_PM. >>> > >>> > By using the new runtime PM helper functions especially the two scenarios below >>> > will be addressed. >>> > >>> > 1) The PM core prevents .runtime_suspend callbacks from being invoked during >>> > system suspend. That means even for a runtime PM centric subsystem and driver, >>> > the device needs to be put into low power state from a system suspend callback. >>> > Otherwise it may very well be left in full power state (runtime resumed) while >>> > the system is suspended. By using the new helper functions, we make sure to walk >>> > the hierarchy of a device's power domain, subsystem and driver. >>> >>> I thought it was the case that runtime PM was only disabled during the >>> 'late' phase now. Isn't that enough to allow runtime callbacks in the >>> normal suspend/resume hooks now? /me looks. >>> >>> oh, wait. Ee still have the _get_noresume() in device_prepare(). hmm >>> >>> Either way, I'm not not a big fan of new functions. Personally, I think >>> subsystems/busses/pm_domains should be able to opt out of the PM core >>> behavior that blocks runtime PM transitions during system suspend. >>> Something like the (untested) hack below. That way, we could avoid the >>> need for new helper functions. >> >> And if one of the subsystems in question is the platform bus type, then >> adding the flag doesn't really make sense, because that means that on >> many system it will be set for the majority of devices. :-) >> >> That said I'm tired of this stuff already. If you really really want to >> remove the bumping up and dropping of the usage counter during system >> suspend/resume by the core, please feel free to submit a patch for that >> and I'll apply it. However, if it causes any regressions to happen >> anywhere, it will be dropped and we'll never talk about this again. >> >> Deal? > > No deal. I prefer the runtime PM helpers, I think that is the right > approach. I also believe this is what Alan Stern also would prefer, > right!? So let's convince Kevin instead. :-) I don't need much convincing. My preference is for fewer functions/helpers because runtime PM is already complicated enough for drivers. However, I'm not going to object to the helpers because they will allow us to simplify many drivers/subsystems that are trying to handle the various combinations of suspend/resume and runtime PM, so it's a step in the right direction. Kevin
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c index 1b41fca3d65a..e0770009ba8e 100644 --- a/drivers/base/power/main.c +++ b/drivers/base/power/main.c @@ -832,7 +832,8 @@ static void device_complete(struct device *dev, pm_message_t state) device_unlock(dev); - pm_runtime_put(dev); + if (dev->power.block_rpm_during_suspend) + pm_runtime_put(dev); } /** @@ -1318,7 +1319,8 @@ static int device_prepare(struct device *dev, pm_message_t state) * block runtime suspend here, during the prepare phase, and allow * it again during the complete phase. */ - pm_runtime_get_noresume(dev); + if (dev->power.block_rpm_during_suspend) + pm_runtime_get_noresume(dev); device_lock(dev); @@ -1350,7 +1352,7 @@ static int device_prepare(struct device *dev, pm_message_t state) device_unlock(dev); - if (error) + if (error && dev->power.block_rpm_during_suspend) pm_runtime_put(dev); return error; diff --git a/include/linux/pm.h b/include/linux/pm.h index 8c6583a53a06..692cd543b71d 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -551,6 +551,7 @@ struct dev_pm_info { struct wakeup_source *wakeup; bool wakeup_path:1; bool syscore:1; + unsigned int block_rpm_during_suspend:1; #else unsigned int should_wakeup:1; #endif