* [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. @ 2014-06-14 10:03 ` Allen Yu 0 siblings, 0 replies; 65+ messages in thread From: Allen Yu @ 2014-06-14 10:03 UTC (permalink / raw) To: Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm Cc: linux-kernel, Allen Yu dev->power.is_suspended is set after core suspends device during system suspend. This flag mostly means device is not operational (all I/O been quiesced, no more data read or write acceptible, etc.), hence it's dangerous to access hardware if device is suspended even though runtime PM status is RPM_ACTIVE. In turn, we should allow device to be accessed in case device is *not* suspended and runtime PM status is RPM_ACTIVE, even if runtime PM disabled. This corner case can happen to a device in a generic PM domain if the domain is not powered off while preparing for a system-wide power transition. In this case, runtime PM status will be set to RPM_ACTIVE and then runtime PM is disabled. After that, device driver may call pm_runtime_get_sync() and rpm_resume() should return 1, because the device is still active as long as not been suspended. Signed-off-by: Allen Yu <alleny@nvidia.com> --- drivers/base/power/runtime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 67c7938..39885f1 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -608,7 +608,7 @@ static int rpm_resume(struct device *dev, int rpmflags) repeat: if (dev->power.runtime_error) retval = -EINVAL; - else if (dev->power.disable_depth == 1 && dev->power.is_suspended + else if (dev->power.disable_depth == 1 && !dev->power.is_suspended && dev->power.runtime_status == RPM_ACTIVE) retval = 1; else if (dev->power.disable_depth > 0) -- 1.8.1.5 ^ permalink raw reply related [flat|nested] 65+ messages in thread
* [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. @ 2014-06-14 10:03 ` Allen Yu 0 siblings, 0 replies; 65+ messages in thread From: Allen Yu @ 2014-06-14 10:03 UTC (permalink / raw) To: Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm Cc: linux-kernel, Allen Yu dev->power.is_suspended is set after core suspends device during system suspend. This flag mostly means device is not operational (all I/O been quiesced, no more data read or write acceptible, etc.), hence it's dangerous to access hardware if device is suspended even though runtime PM status is RPM_ACTIVE. In turn, we should allow device to be accessed in case device is *not* suspended and runtime PM status is RPM_ACTIVE, even if runtime PM disabled. This corner case can happen to a device in a generic PM domain if the domain is not powered off while preparing for a system-wide power transition. In this case, runtime PM status will be set to RPM_ACTIVE and then runtime PM is disabled. After that, device driver may call pm_runtime_get_sync() and rpm_resume() should return 1, because the device is still active as long as not been suspended. Signed-off-by: Allen Yu <alleny@nvidia.com> --- drivers/base/power/runtime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c index 67c7938..39885f1 100644 --- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -608,7 +608,7 @@ static int rpm_resume(struct device *dev, int rpmflags) repeat: if (dev->power.runtime_error) retval = -EINVAL; - else if (dev->power.disable_depth == 1 && dev->power.is_suspended + else if (dev->power.disable_depth == 1 && !dev->power.is_suspended && dev->power.runtime_status == RPM_ACTIVE) retval = 1; else if (dev->power.disable_depth > 0) -- 1.8.1.5 ^ permalink raw reply related [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-14 10:03 ` Allen Yu @ 2014-06-14 14:32 ` Alan Stern -1 siblings, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-06-14 14:32 UTC (permalink / raw) To: Allen Yu Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Sat, 14 Jun 2014, Allen Yu wrote: > dev->power.is_suspended is set after core suspends device during system suspend. > This flag mostly means device is not operational (all I/O been quiesced, no more > data read or write acceptible, etc.), hence it's dangerous to access hardware if > device is suspended even though runtime PM status is RPM_ACTIVE. > > In turn, we should allow device to be accessed in case device is *not* suspended > and runtime PM status is RPM_ACTIVE, even if runtime PM disabled. This corner case > can happen to a device in a generic PM domain if the domain is not powered off while > preparing for a system-wide power transition. I don't understand. Even if the PM domain isn't powered off, the device's is_suspended flag will still be set by __device_suspend(). > In this case, runtime PM status will > be set to RPM_ACTIVE and then runtime PM is disabled. After that, device driver may > call pm_runtime_get_sync() and rpm_resume() should return 1, because the device is > still active as long as not been suspended. Isn't that what the existing code does already? Your patch seems to change it so that it _doesn't_ behave the way you want. > Signed-off-by: Allen Yu <alleny@nvidia.com> > --- > drivers/base/power/runtime.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index 67c7938..39885f1 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -608,7 +608,7 @@ static int rpm_resume(struct device *dev, int rpmflags) > repeat: > if (dev->power.runtime_error) > retval = -EINVAL; > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > + else if (dev->power.disable_depth == 1 && !dev->power.is_suspended > && dev->power.runtime_status == RPM_ACTIVE) > retval = 1; Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. @ 2014-06-14 14:32 ` Alan Stern 0 siblings, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-06-14 14:32 UTC (permalink / raw) To: Allen Yu Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Sat, 14 Jun 2014, Allen Yu wrote: > dev->power.is_suspended is set after core suspends device during system suspend. > This flag mostly means device is not operational (all I/O been quiesced, no more > data read or write acceptible, etc.), hence it's dangerous to access hardware if > device is suspended even though runtime PM status is RPM_ACTIVE. > > In turn, we should allow device to be accessed in case device is *not* suspended > and runtime PM status is RPM_ACTIVE, even if runtime PM disabled. This corner case > can happen to a device in a generic PM domain if the domain is not powered off while > preparing for a system-wide power transition. I don't understand. Even if the PM domain isn't powered off, the device's is_suspended flag will still be set by __device_suspend(). > In this case, runtime PM status will > be set to RPM_ACTIVE and then runtime PM is disabled. After that, device driver may > call pm_runtime_get_sync() and rpm_resume() should return 1, because the device is > still active as long as not been suspended. Isn't that what the existing code does already? Your patch seems to change it so that it _doesn't_ behave the way you want. > Signed-off-by: Allen Yu <alleny@nvidia.com> > --- > drivers/base/power/runtime.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > index 67c7938..39885f1 100644 > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -608,7 +608,7 @@ static int rpm_resume(struct device *dev, int rpmflags) > repeat: > if (dev->power.runtime_error) > retval = -EINVAL; > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > + else if (dev->power.disable_depth == 1 && !dev->power.is_suspended > && dev->power.runtime_status == RPM_ACTIVE) > retval = 1; Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* RE: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-14 14:32 ` Alan Stern (?) @ 2014-06-16 3:03 ` Allen Yu 2014-06-16 14:43 ` Alan Stern -1 siblings, 1 reply; 65+ messages in thread From: Allen Yu @ 2014-06-16 3:03 UTC (permalink / raw) To: Alan Stern Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Sat, 14 Jun 2014, Alan Stern wrote: > > dev->power.is_suspended is set after core suspends device during system > suspend. > > This flag mostly means device is not operational (all I/O been > > quiesced, no more data read or write acceptible, etc.), hence it's > > dangerous to access hardware if device is suspended even though runtime > PM status is RPM_ACTIVE. > > > > In turn, we should allow device to be accessed in case device is *not* > > suspended and runtime PM status is RPM_ACTIVE, even if runtime PM > > disabled. This corner case can happen to a device in a generic PM > > domain if the domain is not powered off while preparing for a system-wide > power transition. > > I don't understand. Even if the PM domain isn't powered off, the device's > is_suspended flag will still be set by __device_suspend(). Yes, is_suspended flag will be set by __device_suspend(). But runtime PM can be disabled in pm_genpd_prepare(): 914 static int pm_genpd_prepare(struct device *dev){ ... 956 /* 957 * The PM domain must be in the GPD_STATE_ACTIVE state at this point, 958 * so pm_genpd_poweron() will return immediately, but if the device 959 * is suspended (e.g. it's been stopped by genpd_stop_dev()), we need 960 * to make it operational. 961 */ 962 pm_runtime_resume(dev); 963 __pm_runtime_disable(dev, false); ... 978 } And there is a gap between pm_genpd_prepare() and __device_suspend(), during which is_suspended flag is *not* yet set but runtime PM is disabled and device status is RPM_ACTIVE. > > > In this case, runtime PM status will > > be set to RPM_ACTIVE and then runtime PM is disabled. After that, > > device driver may call pm_runtime_get_sync() and rpm_resume() should > > return 1, because the device is still active as long as not been suspended. > > Isn't that what the existing code does already? Your patch seems to change > it so that it _doesn't_ behave the way you want. > The existing code return 1 for case is_suspended flag is set. That means __device_suspend() has been called and device is not in operational state. Whereas the case I mentioned above is before device is suspended. It's dangerous to access device if it's in suspended state, so I propose only allowing access to a device if it's not suspended (i.e. value of "is_suspneded" flag is false). > > Signed-off-by: Allen Yu <alleny@nvidia.com> > > --- > > drivers/base/power/runtime.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/base/power/runtime.c > > b/drivers/base/power/runtime.c index 67c7938..39885f1 100644 > > --- a/drivers/base/power/runtime.c > > +++ b/drivers/base/power/runtime.c > > @@ -608,7 +608,7 @@ static int rpm_resume(struct device *dev, int > rpmflags) > > repeat: > > if (dev->power.runtime_error) > > retval = -EINVAL; > > - else if (dev->power.disable_depth == 1 && dev- > >power.is_suspended > > + else if (dev->power.disable_depth == 1 && !dev- > >power.is_suspended > > && dev->power.runtime_status == RPM_ACTIVE) > > retval = 1; > > Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* RE: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-16 3:03 ` Allen Yu @ 2014-06-16 14:43 ` Alan Stern 0 siblings, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-06-16 14:43 UTC (permalink / raw) To: Allen Yu Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Mon, 16 Jun 2014, Allen Yu wrote: > On Sat, 14 Jun 2014, Alan Stern wrote: > > > > dev->power.is_suspended is set after core suspends device during system > > suspend. > > > This flag mostly means device is not operational (all I/O been > > > quiesced, no more data read or write acceptible, etc.), hence it's > > > dangerous to access hardware if device is suspended even though runtime > > PM status is RPM_ACTIVE. > > > > > > In turn, we should allow device to be accessed in case device is *not* > > > suspended and runtime PM status is RPM_ACTIVE, even if runtime PM > > > disabled. This corner case can happen to a device in a generic PM > > > domain if the domain is not powered off while preparing for a system-wide > > power transition. > > > > I don't understand. Even if the PM domain isn't powered off, the device's > > is_suspended flag will still be set by __device_suspend(). > > Yes, is_suspended flag will be set by __device_suspend(). But runtime PM can be disabled in pm_genpd_prepare(): > > 914 static int pm_genpd_prepare(struct device *dev){ > ... > 956 /* > 957 * The PM domain must be in the GPD_STATE_ACTIVE state at this point, > 958 * so pm_genpd_poweron() will return immediately, but if the device > 959 * is suspended (e.g. it's been stopped by genpd_stop_dev()), we need > 960 * to make it operational. > 961 */ > 962 pm_runtime_resume(dev); > 963 __pm_runtime_disable(dev, false); > ... > 978 } > > And there is a gap between pm_genpd_prepare() and __device_suspend(), during which is_suspended flag is *not* yet set but runtime PM is disabled and device status is RPM_ACTIVE. It sounds like the real problem lies in pm_genpd_prepare(). It disables the device too early. > > > In this case, runtime PM status will > > > be set to RPM_ACTIVE and then runtime PM is disabled. After that, > > > device driver may call pm_runtime_get_sync() and rpm_resume() should > > > return 1, because the device is still active as long as not been suspended. > > > > Isn't that what the existing code does already? Your patch seems to change > > it so that it _doesn't_ behave the way you want. > > > > The existing code return 1 for case is_suspended flag is set. That means __device_suspend() has been called and device is not in operational state. Whereas the case I mentioned above is before device is suspended. > It's dangerous to access device if it's in suspended state, so I propose only allowing access to a device if it's not suspended (i.e. value of "is_suspneded" flag is false). Okay, now I understand what you want to do. I couldn't figure it out just from the patch description. A better solution might be to remove the __pm_runtime_disable() call from pm_genpd_prepare(). There's no obvious reason for it to be there, especially since the PM core does a disable in __device_suspend_late(). Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-14 10:03 ` Allen Yu @ 2014-06-16 17:40 ` Alan Stern -1 siblings, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-06-16 17:40 UTC (permalink / raw) To: Allen Yu Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Sat, 14 Jun 2014, Allen Yu wrote: > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -608,7 +608,7 @@ static int rpm_resume(struct device *dev, int rpmflags) > repeat: > if (dev->power.runtime_error) > retval = -EINVAL; > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > + else if (dev->power.disable_depth == 1 && !dev->power.is_suspended > && dev->power.runtime_status == RPM_ACTIVE) > retval = 1; For reasons having nothing to do with Allen's suggested change, I wonder if we shouldn't replace this line with something like: - else if (dev->power.disable_depth == 1 && dev->power.is_suspended + else if (dev->power.disable > 0 && !dev->power.is_suspended && dev->power.runtime_status == RPM_ACTIVE) retval = 1; It seems that I've been bitten by this several times in the past. When a device is disabled for runtime PM, and more or less permanently stuck in the RPM_ACTIVE state, calls to pm_runtime_resume() or pm_runtime_get_sync() shouldn't fail. For example, suppose some devices of a certain type support runtime power management but others don't. We naturally want to call pm_runtime_disable() for the ones that don't. But we also want the same driver to work for all the devices, which means that pm_runtime_get_sync() should return success -- otherwise the driver will think that something has gone wrong. Rafael, what do you think? Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. @ 2014-06-16 17:40 ` Alan Stern 0 siblings, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-06-16 17:40 UTC (permalink / raw) To: Allen Yu Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Sat, 14 Jun 2014, Allen Yu wrote: > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -608,7 +608,7 @@ static int rpm_resume(struct device *dev, int rpmflags) > repeat: > if (dev->power.runtime_error) > retval = -EINVAL; > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > + else if (dev->power.disable_depth == 1 && !dev->power.is_suspended > && dev->power.runtime_status == RPM_ACTIVE) > retval = 1; For reasons having nothing to do with Allen's suggested change, I wonder if we shouldn't replace this line with something like: - else if (dev->power.disable_depth == 1 && dev->power.is_suspended + else if (dev->power.disable > 0 && !dev->power.is_suspended && dev->power.runtime_status == RPM_ACTIVE) retval = 1; It seems that I've been bitten by this several times in the past. When a device is disabled for runtime PM, and more or less permanently stuck in the RPM_ACTIVE state, calls to pm_runtime_resume() or pm_runtime_get_sync() shouldn't fail. For example, suppose some devices of a certain type support runtime power management but others don't. We naturally want to call pm_runtime_disable() for the ones that don't. But we also want the same driver to work for all the devices, which means that pm_runtime_get_sync() should return success -- otherwise the driver will think that something has gone wrong. Rafael, what do you think? Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-16 17:40 ` Alan Stern (?) @ 2014-06-16 21:29 ` Rafael J. Wysocki 2014-06-17 14:11 ` Alan Stern -1 siblings, 1 reply; 65+ messages in thread From: Rafael J. Wysocki @ 2014-06-16 21:29 UTC (permalink / raw) To: Alan Stern Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Monday, June 16, 2014 01:40:05 PM Alan Stern wrote: > On Sat, 14 Jun 2014, Allen Yu wrote: > > > --- a/drivers/base/power/runtime.c > > +++ b/drivers/base/power/runtime.c > > @@ -608,7 +608,7 @@ static int rpm_resume(struct device *dev, int rpmflags) > > repeat: > > if (dev->power.runtime_error) > > retval = -EINVAL; > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > + else if (dev->power.disable_depth == 1 && !dev->power.is_suspended > > && dev->power.runtime_status == RPM_ACTIVE) > > retval = 1; > > For reasons having nothing to do with Allen's suggested change, I > wonder if we shouldn't replace this line with something like: > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > + else if (dev->power.disable > 0 && !dev->power.is_suspended > && dev->power.runtime_status == RPM_ACTIVE) > retval = 1; > > It seems that I've been bitten by this several times in the past. > When a device is disabled for runtime PM, and more or less permanently > stuck in the RPM_ACTIVE state, calls to pm_runtime_resume() or > pm_runtime_get_sync() shouldn't fail. > > For example, suppose some devices of a certain type support runtime > power management but others don't. We naturally want to call > pm_runtime_disable() for the ones that don't. But we also want the > same driver to work for all the devices, which means that > pm_runtime_get_sync() should return success -- otherwise the driver > will think that something has gone wrong. > > Rafael, what do you think? That condition is there specifically to take care of the system suspend code path. It means that if runtime PM is disabled, but it only has been disabled by the system suspend code path, we should treat the device as "active" (ie. return 1). That won't work after the proposed change. I guess drivers that want to work with devices where runtime PM may be disabled can just check the return value of rpm_resume() for -EACCES? Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-16 21:29 ` Rafael J. Wysocki @ 2014-06-17 14:11 ` Alan Stern 0 siblings, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-06-17 14:11 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Mon, 16 Jun 2014, Rafael J. Wysocki wrote: > > For reasons having nothing to do with Allen's suggested change, I > > wonder if we shouldn't replace this line with something like: > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > + else if (dev->power.disable > 0 && !dev->power.is_suspended > > && dev->power.runtime_status == RPM_ACTIVE) > > retval = 1; > > > > It seems that I've been bitten by this several times in the past. > > When a device is disabled for runtime PM, and more or less permanently > > stuck in the RPM_ACTIVE state, calls to pm_runtime_resume() or > > pm_runtime_get_sync() shouldn't fail. > > > > For example, suppose some devices of a certain type support runtime > > power management but others don't. We naturally want to call > > pm_runtime_disable() for the ones that don't. But we also want the > > same driver to work for all the devices, which means that > > pm_runtime_get_sync() should return success -- otherwise the driver > > will think that something has gone wrong. > > > > Rafael, what do you think? > > That condition is there specifically to take care of the system suspend > code path. It means that if runtime PM is disabled, but it only has been > disabled by the system suspend code path, we should treat the device as > "active" (ie. return 1). That won't work after the proposed change. Ah, yes, quite true. Okay, suppose we replace that line with just: + else if (dev->power.disable > 0 > I guess drivers that want to work with devices where runtime PM may be > disabled can just check the return value of rpm_resume() for -EACCES? They could, but it's extra work and it's extremely easy to forget about. I'd prefer not to do things that way. Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. @ 2014-06-17 14:11 ` Alan Stern 0 siblings, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-06-17 14:11 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Mon, 16 Jun 2014, Rafael J. Wysocki wrote: > > For reasons having nothing to do with Allen's suggested change, I > > wonder if we shouldn't replace this line with something like: > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > + else if (dev->power.disable > 0 && !dev->power.is_suspended > > && dev->power.runtime_status == RPM_ACTIVE) > > retval = 1; > > > > It seems that I've been bitten by this several times in the past. > > When a device is disabled for runtime PM, and more or less permanently > > stuck in the RPM_ACTIVE state, calls to pm_runtime_resume() or > > pm_runtime_get_sync() shouldn't fail. > > > > For example, suppose some devices of a certain type support runtime > > power management but others don't. We naturally want to call > > pm_runtime_disable() for the ones that don't. But we also want the > > same driver to work for all the devices, which means that > > pm_runtime_get_sync() should return success -- otherwise the driver > > will think that something has gone wrong. > > > > Rafael, what do you think? > > That condition is there specifically to take care of the system suspend > code path. It means that if runtime PM is disabled, but it only has been > disabled by the system suspend code path, we should treat the device as > "active" (ie. return 1). That won't work after the proposed change. Ah, yes, quite true. Okay, suppose we replace that line with just: + else if (dev->power.disable > 0 > I guess drivers that want to work with devices where runtime PM may be > disabled can just check the return value of rpm_resume() for -EACCES? They could, but it's extra work and it's extremely easy to forget about. I'd prefer not to do things that way. Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-17 14:11 ` Alan Stern (?) @ 2014-06-17 20:26 ` Rafael J. Wysocki 2014-06-17 20:37 ` Rafael J. Wysocki -1 siblings, 1 reply; 65+ messages in thread From: Rafael J. Wysocki @ 2014-06-17 20:26 UTC (permalink / raw) To: Alan Stern Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Tuesday, June 17, 2014 10:11:32 AM Alan Stern wrote: > On Mon, 16 Jun 2014, Rafael J. Wysocki wrote: > > > > For reasons having nothing to do with Allen's suggested change, I > > > wonder if we shouldn't replace this line with something like: > > > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > > + else if (dev->power.disable > 0 && !dev->power.is_suspended > > > && dev->power.runtime_status == RPM_ACTIVE) > > > retval = 1; > > > > > > It seems that I've been bitten by this several times in the past. > > > When a device is disabled for runtime PM, and more or less permanently > > > stuck in the RPM_ACTIVE state, calls to pm_runtime_resume() or > > > pm_runtime_get_sync() shouldn't fail. > > > > > > For example, suppose some devices of a certain type support runtime > > > power management but others don't. We naturally want to call > > > pm_runtime_disable() for the ones that don't. But we also want the > > > same driver to work for all the devices, which means that > > > pm_runtime_get_sync() should return success -- otherwise the driver > > > will think that something has gone wrong. > > > > > > Rafael, what do you think? > > > > That condition is there specifically to take care of the system suspend > > code path. It means that if runtime PM is disabled, but it only has been > > disabled by the system suspend code path, we should treat the device as > > "active" (ie. return 1). That won't work after the proposed change. > > Ah, yes, quite true. Okay, suppose we replace that line with just: > > + else if (dev->power.disable > 0 > > > I guess drivers that want to work with devices where runtime PM may be > > disabled can just check the return value of rpm_resume() for -EACCES? > > They could, but it's extra work and it's extremely easy to forget > about. I'd prefer not to do things that way. In that case we need to audit all code that checks the return value of __pm_runtime_resume() to verify that it doesn't depend on the current behavior in any way. It shouldn't, but still. Also we probably should drop the -EACCES return value from rpm_resume() in the same patch, because it specifically only covers the dev->power.disable > 0 case (which BTW is consistent with the suspend side of things, so I'm totally unsure about that being the right thing to do to be honest). Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-17 20:26 ` Rafael J. Wysocki @ 2014-06-17 20:37 ` Rafael J. Wysocki 2014-06-17 20:46 ` Rafael J. Wysocki 0 siblings, 1 reply; 65+ messages in thread From: Rafael J. Wysocki @ 2014-06-17 20:37 UTC (permalink / raw) To: Alan Stern Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Tuesday, June 17, 2014 10:26:14 PM Rafael J. Wysocki wrote: > On Tuesday, June 17, 2014 10:11:32 AM Alan Stern wrote: > > On Mon, 16 Jun 2014, Rafael J. Wysocki wrote: > > > > > > For reasons having nothing to do with Allen's suggested change, I > > > > wonder if we shouldn't replace this line with something like: > > > > > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > > > + else if (dev->power.disable > 0 && !dev->power.is_suspended > > > > && dev->power.runtime_status == RPM_ACTIVE) > > > > retval = 1; > > > > > > > > It seems that I've been bitten by this several times in the past. > > > > When a device is disabled for runtime PM, and more or less permanently > > > > stuck in the RPM_ACTIVE state, calls to pm_runtime_resume() or > > > > pm_runtime_get_sync() shouldn't fail. > > > > > > > > For example, suppose some devices of a certain type support runtime > > > > power management but others don't. We naturally want to call > > > > pm_runtime_disable() for the ones that don't. But we also want the > > > > same driver to work for all the devices, which means that > > > > pm_runtime_get_sync() should return success -- otherwise the driver > > > > will think that something has gone wrong. > > > > > > > > Rafael, what do you think? > > > > > > That condition is there specifically to take care of the system suspend > > > code path. It means that if runtime PM is disabled, but it only has been > > > disabled by the system suspend code path, we should treat the device as > > > "active" (ie. return 1). That won't work after the proposed change. > > > > Ah, yes, quite true. Okay, suppose we replace that line with just: > > > > + else if (dev->power.disable > 0 > > > > > I guess drivers that want to work with devices where runtime PM may be > > > disabled can just check the return value of rpm_resume() for -EACCES? > > > > They could, but it's extra work and it's extremely easy to forget > > about. I'd prefer not to do things that way. > > In that case we need to audit all code that checks the return value of > __pm_runtime_resume() to verify that it doesn't depend on the current > behavior in any way. It shouldn't, but still. > > Also we probably should drop the -EACCES return value from rpm_resume() in the > same patch, because it specifically only covers the dev->power.disable > 0 case > (which BTW is consistent with the suspend side of things, so I'm totally unsure > about that being the right thing to do to be honest). Perhaps it'd be better to rework __pm_runtime_resume() to convert the -EACCES return value from rpm_resume() into 0 if RPM_GET_PUT is set? Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-17 20:37 ` Rafael J. Wysocki @ 2014-06-17 20:46 ` Rafael J. Wysocki 2014-06-18 15:30 ` Alan Stern 0 siblings, 1 reply; 65+ messages in thread From: Rafael J. Wysocki @ 2014-06-17 20:46 UTC (permalink / raw) To: Alan Stern Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Tuesday, June 17, 2014 10:37:03 PM Rafael J. Wysocki wrote: > On Tuesday, June 17, 2014 10:26:14 PM Rafael J. Wysocki wrote: > > On Tuesday, June 17, 2014 10:11:32 AM Alan Stern wrote: > > > On Mon, 16 Jun 2014, Rafael J. Wysocki wrote: > > > > > > > > For reasons having nothing to do with Allen's suggested change, I > > > > > wonder if we shouldn't replace this line with something like: > > > > > > > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > > > > + else if (dev->power.disable > 0 && !dev->power.is_suspended > > > > > && dev->power.runtime_status == RPM_ACTIVE) > > > > > retval = 1; > > > > > > > > > > It seems that I've been bitten by this several times in the past. > > > > > When a device is disabled for runtime PM, and more or less permanently > > > > > stuck in the RPM_ACTIVE state, calls to pm_runtime_resume() or > > > > > pm_runtime_get_sync() shouldn't fail. > > > > > > > > > > For example, suppose some devices of a certain type support runtime > > > > > power management but others don't. We naturally want to call > > > > > pm_runtime_disable() for the ones that don't. But we also want the > > > > > same driver to work for all the devices, which means that > > > > > pm_runtime_get_sync() should return success -- otherwise the driver > > > > > will think that something has gone wrong. > > > > > > > > > > Rafael, what do you think? > > > > > > > > That condition is there specifically to take care of the system suspend > > > > code path. It means that if runtime PM is disabled, but it only has been > > > > disabled by the system suspend code path, we should treat the device as > > > > "active" (ie. return 1). That won't work after the proposed change. > > > > > > Ah, yes, quite true. Okay, suppose we replace that line with just: > > > > > > + else if (dev->power.disable > 0 > > > > > > > I guess drivers that want to work with devices where runtime PM may be > > > > disabled can just check the return value of rpm_resume() for -EACCES? > > > > > > They could, but it's extra work and it's extremely easy to forget > > > about. I'd prefer not to do things that way. > > > > In that case we need to audit all code that checks the return value of > > __pm_runtime_resume() to verify that it doesn't depend on the current > > behavior in any way. It shouldn't, but still. > > > > Also we probably should drop the -EACCES return value from rpm_resume() in the > > same patch, because it specifically only covers the dev->power.disable > 0 case > > (which BTW is consistent with the suspend side of things, so I'm totally unsure > > about that being the right thing to do to be honest). > > Perhaps it'd be better to rework __pm_runtime_resume() to convert the -EACCES > return value from rpm_resume() into 0 if RPM_GET_PUT is set? Or do something like this? --- drivers/base/power/runtime.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) Index: linux-pm/drivers/base/power/runtime.c =================================================================== --- linux-pm.orig/drivers/base/power/runtime.c +++ linux-pm/drivers/base/power/runtime.c @@ -608,7 +608,8 @@ static int rpm_resume(struct device *dev repeat: if (dev->power.runtime_error) retval = -EINVAL; - else if (dev->power.disable_depth == 1 && dev->power.is_suspended + else if (((dev->power.disable_depth > 0 && (rpmflags & RPM_GET_PUT)) + || (dev->power.disable_depth == 1 && dev->power.is_suspended)) && dev->power.runtime_status == RPM_ACTIVE) retval = 1; else if (dev->power.disable_depth > 0) ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-17 20:46 ` Rafael J. Wysocki @ 2014-06-18 15:30 ` Alan Stern 0 siblings, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-06-18 15:30 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Tue, 17 Jun 2014, Rafael J. Wysocki wrote: > On Tuesday, June 17, 2014 10:37:03 PM Rafael J. Wysocki wrote: > > On Tuesday, June 17, 2014 10:26:14 PM Rafael J. Wysocki wrote: > > > On Tuesday, June 17, 2014 10:11:32 AM Alan Stern wrote: > > > > On Mon, 16 Jun 2014, Rafael J. Wysocki wrote: > > > > > > > > > > For reasons having nothing to do with Allen's suggested change, I > > > > > > wonder if we shouldn't replace this line with something like: > > > > > > > > > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > > > > > + else if (dev->power.disable > 0 && !dev->power.is_suspended > > > > > > && dev->power.runtime_status == RPM_ACTIVE) > > > > > > retval = 1; > > > > > > > > > > > > It seems that I've been bitten by this several times in the past. > > > > > > When a device is disabled for runtime PM, and more or less permanently > > > > > > stuck in the RPM_ACTIVE state, calls to pm_runtime_resume() or > > > > > > pm_runtime_get_sync() shouldn't fail. > > > > > > > > > > > > For example, suppose some devices of a certain type support runtime > > > > > > power management but others don't. We naturally want to call > > > > > > pm_runtime_disable() for the ones that don't. But we also want the > > > > > > same driver to work for all the devices, which means that > > > > > > pm_runtime_get_sync() should return success -- otherwise the driver > > > > > > will think that something has gone wrong. > > > > > > > > > > > > Rafael, what do you think? > > > > > > > > > > That condition is there specifically to take care of the system suspend > > > > > code path. It means that if runtime PM is disabled, but it only has been > > > > > disabled by the system suspend code path, we should treat the device as > > > > > "active" (ie. return 1). That won't work after the proposed change. > > > > > > > > Ah, yes, quite true. Okay, suppose we replace that line with just: > > > > > > > > + else if (dev->power.disable > 0 > > > > > > > > > I guess drivers that want to work with devices where runtime PM may be > > > > > disabled can just check the return value of rpm_resume() for -EACCES? > > > > > > > > They could, but it's extra work and it's extremely easy to forget > > > > about. I'd prefer not to do things that way. > > > > > > In that case we need to audit all code that checks the return value of > > > __pm_runtime_resume() to verify that it doesn't depend on the current > > > behavior in any way. It shouldn't, but still. > > > > > > Also we probably should drop the -EACCES return value from rpm_resume() in the > > > same patch, because it specifically only covers the dev->power.disable > 0 case > > > (which BTW is consistent with the suspend side of things, so I'm totally unsure > > > about that being the right thing to do to be honest). It's still the correct action with runtime PM is disabled and the device's runtime_status isn't RPM_ACTIVE. > > Perhaps it'd be better to rework __pm_runtime_resume() to convert the -EACCES > > return value from rpm_resume() into 0 if RPM_GET_PUT is set? > > Or do something like this? > > --- > drivers/base/power/runtime.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > Index: linux-pm/drivers/base/power/runtime.c > =================================================================== > --- linux-pm.orig/drivers/base/power/runtime.c > +++ linux-pm/drivers/base/power/runtime.c > @@ -608,7 +608,8 @@ static int rpm_resume(struct device *dev > repeat: > if (dev->power.runtime_error) > retval = -EINVAL; > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > + else if (((dev->power.disable_depth > 0 && (rpmflags & RPM_GET_PUT)) > + || (dev->power.disable_depth == 1 && dev->power.is_suspended)) > && dev->power.runtime_status == RPM_ACTIVE) > retval = 1; > else if (dev->power.disable_depth > 0) So pm_runtime_resume() and pm_request_resume() would still fail, but pm_runtime_get() and pm_runtime_get_sync() would work? I'm not sure about the reason for this distinction. Allen, what do you think? Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. @ 2014-06-18 15:30 ` Alan Stern 0 siblings, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-06-18 15:30 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Tue, 17 Jun 2014, Rafael J. Wysocki wrote: > On Tuesday, June 17, 2014 10:37:03 PM Rafael J. Wysocki wrote: > > On Tuesday, June 17, 2014 10:26:14 PM Rafael J. Wysocki wrote: > > > On Tuesday, June 17, 2014 10:11:32 AM Alan Stern wrote: > > > > On Mon, 16 Jun 2014, Rafael J. Wysocki wrote: > > > > > > > > > > For reasons having nothing to do with Allen's suggested change, I > > > > > > wonder if we shouldn't replace this line with something like: > > > > > > > > > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > > > > > + else if (dev->power.disable > 0 && !dev->power.is_suspended > > > > > > && dev->power.runtime_status == RPM_ACTIVE) > > > > > > retval = 1; > > > > > > > > > > > > It seems that I've been bitten by this several times in the past. > > > > > > When a device is disabled for runtime PM, and more or less permanently > > > > > > stuck in the RPM_ACTIVE state, calls to pm_runtime_resume() or > > > > > > pm_runtime_get_sync() shouldn't fail. > > > > > > > > > > > > For example, suppose some devices of a certain type support runtime > > > > > > power management but others don't. We naturally want to call > > > > > > pm_runtime_disable() for the ones that don't. But we also want the > > > > > > same driver to work for all the devices, which means that > > > > > > pm_runtime_get_sync() should return success -- otherwise the driver > > > > > > will think that something has gone wrong. > > > > > > > > > > > > Rafael, what do you think? > > > > > > > > > > That condition is there specifically to take care of the system suspend > > > > > code path. It means that if runtime PM is disabled, but it only has been > > > > > disabled by the system suspend code path, we should treat the device as > > > > > "active" (ie. return 1). That won't work after the proposed change. > > > > > > > > Ah, yes, quite true. Okay, suppose we replace that line with just: > > > > > > > > + else if (dev->power.disable > 0 > > > > > > > > > I guess drivers that want to work with devices where runtime PM may be > > > > > disabled can just check the return value of rpm_resume() for -EACCES? > > > > > > > > They could, but it's extra work and it's extremely easy to forget > > > > about. I'd prefer not to do things that way. > > > > > > In that case we need to audit all code that checks the return value of > > > __pm_runtime_resume() to verify that it doesn't depend on the current > > > behavior in any way. It shouldn't, but still. > > > > > > Also we probably should drop the -EACCES return value from rpm_resume() in the > > > same patch, because it specifically only covers the dev->power.disable > 0 case > > > (which BTW is consistent with the suspend side of things, so I'm totally unsure > > > about that being the right thing to do to be honest). It's still the correct action with runtime PM is disabled and the device's runtime_status isn't RPM_ACTIVE. > > Perhaps it'd be better to rework __pm_runtime_resume() to convert the -EACCES > > return value from rpm_resume() into 0 if RPM_GET_PUT is set? > > Or do something like this? > > --- > drivers/base/power/runtime.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > Index: linux-pm/drivers/base/power/runtime.c > =================================================================== > --- linux-pm.orig/drivers/base/power/runtime.c > +++ linux-pm/drivers/base/power/runtime.c > @@ -608,7 +608,8 @@ static int rpm_resume(struct device *dev > repeat: > if (dev->power.runtime_error) > retval = -EINVAL; > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > + else if (((dev->power.disable_depth > 0 && (rpmflags & RPM_GET_PUT)) > + || (dev->power.disable_depth == 1 && dev->power.is_suspended)) > && dev->power.runtime_status == RPM_ACTIVE) > retval = 1; > else if (dev->power.disable_depth > 0) So pm_runtime_resume() and pm_request_resume() would still fail, but pm_runtime_get() and pm_runtime_get_sync() would work? I'm not sure about the reason for this distinction. Allen, what do you think? Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-18 15:30 ` Alan Stern (?) @ 2014-06-18 23:57 ` Rafael J. Wysocki 2014-06-19 8:23 ` Allen Yu 2014-06-19 14:34 ` Alan Stern -1 siblings, 2 replies; 65+ messages in thread From: Rafael J. Wysocki @ 2014-06-18 23:57 UTC (permalink / raw) To: Alan Stern Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Wednesday, June 18, 2014 11:30:51 AM Alan Stern wrote: > On Tue, 17 Jun 2014, Rafael J. Wysocki wrote: > > > On Tuesday, June 17, 2014 10:37:03 PM Rafael J. Wysocki wrote: > > > On Tuesday, June 17, 2014 10:26:14 PM Rafael J. Wysocki wrote: > > > > On Tuesday, June 17, 2014 10:11:32 AM Alan Stern wrote: > > > > > On Mon, 16 Jun 2014, Rafael J. Wysocki wrote: > > > > > > > > > > > > For reasons having nothing to do with Allen's suggested change, I > > > > > > > wonder if we shouldn't replace this line with something like: > > > > > > > > > > > > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > > > > > > + else if (dev->power.disable > 0 && !dev->power.is_suspended > > > > > > > && dev->power.runtime_status == RPM_ACTIVE) > > > > > > > retval = 1; > > > > > > > > > > > > > > It seems that I've been bitten by this several times in the past. > > > > > > > When a device is disabled for runtime PM, and more or less permanently > > > > > > > stuck in the RPM_ACTIVE state, calls to pm_runtime_resume() or > > > > > > > pm_runtime_get_sync() shouldn't fail. > > > > > > > > > > > > > > For example, suppose some devices of a certain type support runtime > > > > > > > power management but others don't. We naturally want to call > > > > > > > pm_runtime_disable() for the ones that don't. But we also want the > > > > > > > same driver to work for all the devices, which means that > > > > > > > pm_runtime_get_sync() should return success -- otherwise the driver > > > > > > > will think that something has gone wrong. > > > > > > > > > > > > > > Rafael, what do you think? > > > > > > > > > > > > That condition is there specifically to take care of the system suspend > > > > > > code path. It means that if runtime PM is disabled, but it only has been > > > > > > disabled by the system suspend code path, we should treat the device as > > > > > > "active" (ie. return 1). That won't work after the proposed change. > > > > > > > > > > Ah, yes, quite true. Okay, suppose we replace that line with just: > > > > > > > > > > + else if (dev->power.disable > 0 > > > > > > > > > > > I guess drivers that want to work with devices where runtime PM may be > > > > > > disabled can just check the return value of rpm_resume() for -EACCES? > > > > > > > > > > They could, but it's extra work and it's extremely easy to forget > > > > > about. I'd prefer not to do things that way. > > > > > > > > In that case we need to audit all code that checks the return value of > > > > __pm_runtime_resume() to verify that it doesn't depend on the current > > > > behavior in any way. It shouldn't, but still. > > > > > > > > Also we probably should drop the -EACCES return value from rpm_resume() in the > > > > same patch, because it specifically only covers the dev->power.disable > 0 case > > > > (which BTW is consistent with the suspend side of things, so I'm totally unsure > > > > about that being the right thing to do to be honest). > > It's still the correct action with runtime PM is disabled and the > device's runtime_status isn't RPM_ACTIVE. Well, we used to have the notion that runtime_status is not meaningful for devices with dev->power.disable_depth greater than 0 (except for the special case in the suspend code path where we know why it is greater than 0). I think it was useful. :-) > > > Perhaps it'd be better to rework __pm_runtime_resume() to convert the -EACCES > > > return value from rpm_resume() into 0 if RPM_GET_PUT is set? > > > > Or do something like this? > > > > --- > > drivers/base/power/runtime.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > Index: linux-pm/drivers/base/power/runtime.c > > =================================================================== > > --- linux-pm.orig/drivers/base/power/runtime.c > > +++ linux-pm/drivers/base/power/runtime.c > > @@ -608,7 +608,8 @@ static int rpm_resume(struct device *dev > > repeat: > > if (dev->power.runtime_error) > > retval = -EINVAL; > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > + else if (((dev->power.disable_depth > 0 && (rpmflags & RPM_GET_PUT)) > > + || (dev->power.disable_depth == 1 && dev->power.is_suspended)) > > && dev->power.runtime_status == RPM_ACTIVE) > > retval = 1; > > else if (dev->power.disable_depth > 0) > > So pm_runtime_resume() and pm_request_resume() would still fail, but > pm_runtime_get() and pm_runtime_get_sync() would work? I'm not sure > about the reason for this distinction. The meaning of pm_runtime_get()/pm_runtime_get_sync() is "prevent the device from being suspended from now on and resume it if necessary" while "runtime PM disabled and runtime_status == RPM_ACTIVE" may be interpreted as "not necessary to resume", so it is reasonable to special case this particular situation for these particular routines IMHO. Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* RE: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-18 23:57 ` Rafael J. Wysocki @ 2014-06-19 8:23 ` Allen Yu 2014-06-19 14:34 ` Alan Stern 1 sibling, 0 replies; 65+ messages in thread From: Allen Yu @ 2014-06-19 8:23 UTC (permalink / raw) To: Rafael J. Wysocki, Alan Stern Cc: Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 7049 bytes --] On Thursday, June 19, 2014, Rafael J. Wysocki wrote: > On Wednesday, June 18, 2014 11:30:51 AM Alan Stern wrote: > > On Tue, 17 Jun 2014, Rafael J. Wysocki wrote: > > > > > On Tuesday, June 17, 2014 10:37:03 PM Rafael J. Wysocki wrote: > > > > On Tuesday, June 17, 2014 10:26:14 PM Rafael J. Wysocki wrote: > > > > > On Tuesday, June 17, 2014 10:11:32 AM Alan Stern wrote: > > > > > > On Mon, 16 Jun 2014, Rafael J. Wysocki wrote: > > > > > > > > > > > > > > For reasons having nothing to do with Allen's suggested > > > > > > > > change, I wonder if we shouldn't replace this line with > something like: > > > > > > > > > > > > > > > > - else if (dev->power.disable_depth == 1 && dev- > >power.is_suspended > > > > > > > > + else if (dev->power.disable > 0 && > > > > > > > > +!dev->power.is_suspended > > > > > > > > && dev->power.runtime_status == RPM_ACTIVE) > > > > > > > > retval = 1; > > > > > > > > > > > > > > > > It seems that I've been bitten by this several times in the past. > > > > > > > > When a device is disabled for runtime PM, and more or less > > > > > > > > permanently stuck in the RPM_ACTIVE state, calls to > > > > > > > > pm_runtime_resume() or > > > > > > > > pm_runtime_get_sync() shouldn't fail. > > > > > > > > > > > > > > > > For example, suppose some devices of a certain type > > > > > > > > support runtime power management but others don't. We > > > > > > > > naturally want to call > > > > > > > > pm_runtime_disable() for the ones that don't. But we also > > > > > > > > want the same driver to work for all the devices, which > > > > > > > > means that > > > > > > > > pm_runtime_get_sync() should return success -- otherwise > > > > > > > > the driver will think that something has gone wrong. > > > > > > > > > > > > > > > > Rafael, what do you think? > > > > > > > > > > > > > > That condition is there specifically to take care of the > > > > > > > system suspend code path. It means that if runtime PM is > > > > > > > disabled, but it only has been disabled by the system > > > > > > > suspend code path, we should treat the device as "active" (ie. > return 1). That won't work after the proposed change. > > > > > > > > > > > > Ah, yes, quite true. Okay, suppose we replace that line with just: > > > > > > > > > > > > + else if (dev->power.disable > 0 > > > > > > > > > > > > > I guess drivers that want to work with devices where runtime > > > > > > > PM may be disabled can just check the return value of > rpm_resume() for -EACCES? > > > > > > > > > > > > They could, but it's extra work and it's extremely easy to > > > > > > forget about. I'd prefer not to do things that way. > > > > > > > > > > In that case we need to audit all code that checks the return > > > > > value of > > > > > __pm_runtime_resume() to verify that it doesn't depend on the > > > > > current behavior in any way. It shouldn't, but still. > > > > > > > > > > Also we probably should drop the -EACCES return value from > > > > > rpm_resume() in the same patch, because it specifically only > > > > > covers the dev->power.disable > 0 case (which BTW is consistent > > > > > with the suspend side of things, so I'm totally unsure about that being > the right thing to do to be honest). > > > > It's still the correct action with runtime PM is disabled and the > > device's runtime_status isn't RPM_ACTIVE. > > Well, we used to have the notion that runtime_status is not meaningful for > devices with dev->power.disable_depth greater than 0 (except for the > special case in the suspend code path where we know why it is greater than > 0). I think it was useful. :-) So what's the exact state of device if dev->power.is_suspended flag is set and runtime_status is RPM_ACTIVE? Is it a state like "suspended but still can be accessed"? I'm just afraid the existing code would cause a device hang if we allow it to be accessed even though it's suspended (at this point RPM_ACTIVE could be meaningless). I don't understand the original motivation of these code. If it's a valid case, most likely it should be handled in the specific device driver instead of the PM core. > > > > > Perhaps it'd be better to rework __pm_runtime_resume() to convert > > > > the -EACCES return value from rpm_resume() into 0 if RPM_GET_PUT is > set? > > > > > > Or do something like this? > > > > > > --- > > > drivers/base/power/runtime.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > Index: linux-pm/drivers/base/power/runtime.c > > > > ========================================================== > ========= > > > --- linux-pm.orig/drivers/base/power/runtime.c > > > +++ linux-pm/drivers/base/power/runtime.c > > > @@ -608,7 +608,8 @@ static int rpm_resume(struct device *dev > > > repeat: > > > if (dev->power.runtime_error) > > > retval = -EINVAL; > > > - else if (dev->power.disable_depth == 1 && dev- > >power.is_suspended > > > + else if (((dev->power.disable_depth > 0 && (rpmflags & > RPM_GET_PUT)) > > > + || (dev->power.disable_depth == 1 && dev- > >power.is_suspended)) > > > && dev->power.runtime_status == RPM_ACTIVE) > > > retval = 1; > > > else if (dev->power.disable_depth > 0) > > > > So pm_runtime_resume() and pm_request_resume() would still fail, but > > pm_runtime_get() and pm_runtime_get_sync() would work? I'm not sure > > about the reason for this distinction. > > The meaning of pm_runtime_get()/pm_runtime_get_sync() is "prevent the > device from being suspended from now on and resume it if necessary" while > "runtime PM disabled and runtime_status == RPM_ACTIVE" may be > interpreted as "not necessary to resume", so it is reasonable to special case > this particular situation for these particular routines IMHO. As Rafael mentioned above that runtime_sataus is not meaningful if runtime PM is disabled, so shouldn't we avoid using the runtime_staus here and instead use dev->power.is_suspended only to decide the return value? @@ -608,11 +608,13 @@ static int rpm_resume(struct device *dev, int rpmflags) repeat: if (dev->power.runtime_error) retval = -EINVAL; - else if (dev->power.disable_depth == 1 && dev->power.is_suspended - && dev->power.runtime_status == RPM_ACTIVE) - retval = 1; - else if (dev->power.disable_depth > 0) - retval = -EACCES; + else if (dev->power.disable_depth > 0) { + if (!dev->power.is_suspended) + retval = 1; + else + retval = -EACCES; + } + if (retval) goto out; However, this requires us to make sure device is in full functional state if it's not suspended before disabling runtime PM, just like the case runtime PM is not configured at all. And also requires device suspend routine to check dev->power.usage_count before suspending device. Thanks, Allen Yu ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 65+ messages in thread
* RE: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. @ 2014-06-19 8:23 ` Allen Yu 0 siblings, 0 replies; 65+ messages in thread From: Allen Yu @ 2014-06-19 8:23 UTC (permalink / raw) To: Rafael J. Wysocki, Alan Stern Cc: Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Thursday, June 19, 2014, Rafael J. Wysocki wrote: > On Wednesday, June 18, 2014 11:30:51 AM Alan Stern wrote: > > On Tue, 17 Jun 2014, Rafael J. Wysocki wrote: > > > > > On Tuesday, June 17, 2014 10:37:03 PM Rafael J. Wysocki wrote: > > > > On Tuesday, June 17, 2014 10:26:14 PM Rafael J. Wysocki wrote: > > > > > On Tuesday, June 17, 2014 10:11:32 AM Alan Stern wrote: > > > > > > On Mon, 16 Jun 2014, Rafael J. Wysocki wrote: > > > > > > > > > > > > > > For reasons having nothing to do with Allen's suggested > > > > > > > > change, I wonder if we shouldn't replace this line with > something like: > > > > > > > > > > > > > > > > - else if (dev->power.disable_depth == 1 && dev- > >power.is_suspended > > > > > > > > + else if (dev->power.disable > 0 && > > > > > > > > +!dev->power.is_suspended > > > > > > > > && dev->power.runtime_status == RPM_ACTIVE) > > > > > > > > retval = 1; > > > > > > > > > > > > > > > > It seems that I've been bitten by this several times in the past. > > > > > > > > When a device is disabled for runtime PM, and more or less > > > > > > > > permanently stuck in the RPM_ACTIVE state, calls to > > > > > > > > pm_runtime_resume() or > > > > > > > > pm_runtime_get_sync() shouldn't fail. > > > > > > > > > > > > > > > > For example, suppose some devices of a certain type > > > > > > > > support runtime power management but others don't. We > > > > > > > > naturally want to call > > > > > > > > pm_runtime_disable() for the ones that don't. But we also > > > > > > > > want the same driver to work for all the devices, which > > > > > > > > means that > > > > > > > > pm_runtime_get_sync() should return success -- otherwise > > > > > > > > the driver will think that something has gone wrong. > > > > > > > > > > > > > > > > Rafael, what do you think? > > > > > > > > > > > > > > That condition is there specifically to take care of the > > > > > > > system suspend code path. It means that if runtime PM is > > > > > > > disabled, but it only has been disabled by the system > > > > > > > suspend code path, we should treat the device as "active" (ie. > return 1). That won't work after the proposed change. > > > > > > > > > > > > Ah, yes, quite true. Okay, suppose we replace that line with just: > > > > > > > > > > > > + else if (dev->power.disable > 0 > > > > > > > > > > > > > I guess drivers that want to work with devices where runtime > > > > > > > PM may be disabled can just check the return value of > rpm_resume() for -EACCES? > > > > > > > > > > > > They could, but it's extra work and it's extremely easy to > > > > > > forget about. I'd prefer not to do things that way. > > > > > > > > > > In that case we need to audit all code that checks the return > > > > > value of > > > > > __pm_runtime_resume() to verify that it doesn't depend on the > > > > > current behavior in any way. It shouldn't, but still. > > > > > > > > > > Also we probably should drop the -EACCES return value from > > > > > rpm_resume() in the same patch, because it specifically only > > > > > covers the dev->power.disable > 0 case (which BTW is consistent > > > > > with the suspend side of things, so I'm totally unsure about that being > the right thing to do to be honest). > > > > It's still the correct action with runtime PM is disabled and the > > device's runtime_status isn't RPM_ACTIVE. > > Well, we used to have the notion that runtime_status is not meaningful for > devices with dev->power.disable_depth greater than 0 (except for the > special case in the suspend code path where we know why it is greater than > 0). I think it was useful. :-) So what's the exact state of device if dev->power.is_suspended flag is set and runtime_status is RPM_ACTIVE? Is it a state like "suspended but still can be accessed"? I'm just afraid the existing code would cause a device hang if we allow it to be accessed even though it's suspended (at this point RPM_ACTIVE could be meaningless). I don't understand the original motivation of these code. If it's a valid case, most likely it should be handled in the specific device driver instead of the PM core. > > > > > Perhaps it'd be better to rework __pm_runtime_resume() to convert > > > > the -EACCES return value from rpm_resume() into 0 if RPM_GET_PUT is > set? > > > > > > Or do something like this? > > > > > > --- > > > drivers/base/power/runtime.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > Index: linux-pm/drivers/base/power/runtime.c > > > > ========================================================== > ========= > > > --- linux-pm.orig/drivers/base/power/runtime.c > > > +++ linux-pm/drivers/base/power/runtime.c > > > @@ -608,7 +608,8 @@ static int rpm_resume(struct device *dev > > > repeat: > > > if (dev->power.runtime_error) > > > retval = -EINVAL; > > > - else if (dev->power.disable_depth == 1 && dev- > >power.is_suspended > > > + else if (((dev->power.disable_depth > 0 && (rpmflags & > RPM_GET_PUT)) > > > + || (dev->power.disable_depth == 1 && dev- > >power.is_suspended)) > > > && dev->power.runtime_status == RPM_ACTIVE) > > > retval = 1; > > > else if (dev->power.disable_depth > 0) > > > > So pm_runtime_resume() and pm_request_resume() would still fail, but > > pm_runtime_get() and pm_runtime_get_sync() would work? I'm not sure > > about the reason for this distinction. > > The meaning of pm_runtime_get()/pm_runtime_get_sync() is "prevent the > device from being suspended from now on and resume it if necessary" while > "runtime PM disabled and runtime_status == RPM_ACTIVE" may be > interpreted as "not necessary to resume", so it is reasonable to special case > this particular situation for these particular routines IMHO. As Rafael mentioned above that runtime_sataus is not meaningful if runtime PM is disabled, so shouldn't we avoid using the runtime_staus here and instead use dev->power.is_suspended only to decide the return value? @@ -608,11 +608,13 @@ static int rpm_resume(struct device *dev, int rpmflags) repeat: if (dev->power.runtime_error) retval = -EINVAL; - else if (dev->power.disable_depth == 1 && dev->power.is_suspended - && dev->power.runtime_status == RPM_ACTIVE) - retval = 1; - else if (dev->power.disable_depth > 0) - retval = -EACCES; + else if (dev->power.disable_depth > 0) { + if (!dev->power.is_suspended) + retval = 1; + else + retval = -EACCES; + } + if (retval) goto out; However, this requires us to make sure device is in full functional state if it's not suspended before disabling runtime PM, just like the case runtime PM is not configured at all. And also requires device suspend routine to check dev->power.usage_count before suspending device. Thanks, Allen Yu ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-19 8:23 ` Allen Yu (?) @ 2014-06-19 13:55 ` Rafael J. Wysocki 2014-06-19 14:34 ` Allen Yu -1 siblings, 1 reply; 65+ messages in thread From: Rafael J. Wysocki @ 2014-06-19 13:55 UTC (permalink / raw) To: Allen Yu Cc: Alan Stern, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Thursday, June 19, 2014 04:23:29 PM Allen Yu wrote: > On Thursday, June 19, 2014, Rafael J. Wysocki wrote: > > On Wednesday, June 18, 2014 11:30:51 AM Alan Stern wrote: > > > On Tue, 17 Jun 2014, Rafael J. Wysocki wrote: > > > > > > > On Tuesday, June 17, 2014 10:37:03 PM Rafael J. Wysocki wrote: > > > > > On Tuesday, June 17, 2014 10:26:14 PM Rafael J. Wysocki wrote: > > > > > > On Tuesday, June 17, 2014 10:11:32 AM Alan Stern wrote: > > > > > > > On Mon, 16 Jun 2014, Rafael J. Wysocki wrote: > > > > > > > > > > > > > > > > For reasons having nothing to do with Allen's suggested > > > > > > > > > change, I wonder if we shouldn't replace this line with > > something like: > > > > > > > > > > > > > > > > > > - else if (dev->power.disable_depth == 1 && dev- > > >power.is_suspended > > > > > > > > > + else if (dev->power.disable > 0 && > > > > > > > > > +!dev->power.is_suspended > > > > > > > > > && dev->power.runtime_status == RPM_ACTIVE) > > > > > > > > > retval = 1; > > > > > > > > > > > > > > > > > > It seems that I've been bitten by this several times in the past. > > > > > > > > > When a device is disabled for runtime PM, and more or less > > > > > > > > > permanently stuck in the RPM_ACTIVE state, calls to > > > > > > > > > pm_runtime_resume() or > > > > > > > > > pm_runtime_get_sync() shouldn't fail. > > > > > > > > > > > > > > > > > > For example, suppose some devices of a certain type > > > > > > > > > support runtime power management but others don't. We > > > > > > > > > naturally want to call > > > > > > > > > pm_runtime_disable() for the ones that don't. But we also > > > > > > > > > want the same driver to work for all the devices, which > > > > > > > > > means that > > > > > > > > > pm_runtime_get_sync() should return success -- otherwise > > > > > > > > > the driver will think that something has gone wrong. > > > > > > > > > > > > > > > > > > Rafael, what do you think? > > > > > > > > > > > > > > > > That condition is there specifically to take care of the > > > > > > > > system suspend code path. It means that if runtime PM is > > > > > > > > disabled, but it only has been disabled by the system > > > > > > > > suspend code path, we should treat the device as "active" (ie. > > return 1). That won't work after the proposed change. > > > > > > > > > > > > > > Ah, yes, quite true. Okay, suppose we replace that line with just: > > > > > > > > > > > > > > + else if (dev->power.disable > 0 > > > > > > > > > > > > > > > I guess drivers that want to work with devices where runtime > > > > > > > > PM may be disabled can just check the return value of > > rpm_resume() for -EACCES? > > > > > > > > > > > > > > They could, but it's extra work and it's extremely easy to > > > > > > > forget about. I'd prefer not to do things that way. > > > > > > > > > > > > In that case we need to audit all code that checks the return > > > > > > value of > > > > > > __pm_runtime_resume() to verify that it doesn't depend on the > > > > > > current behavior in any way. It shouldn't, but still. > > > > > > > > > > > > Also we probably should drop the -EACCES return value from > > > > > > rpm_resume() in the same patch, because it specifically only > > > > > > covers the dev->power.disable > 0 case (which BTW is consistent > > > > > > with the suspend side of things, so I'm totally unsure about that being > > the right thing to do to be honest). > > > > > > It's still the correct action with runtime PM is disabled and the > > > device's runtime_status isn't RPM_ACTIVE. > > > > Well, we used to have the notion that runtime_status is not meaningful for > > devices with dev->power.disable_depth greater than 0 (except for the > > special case in the suspend code path where we know why it is greater than > > 0). I think it was useful. :-) > > So what's the exact state of device if dev->power.is_suspended flag is set and runtime_status is RPM_ACTIVE? Is it a state like "suspended but still can be accessed"? > > I'm just afraid the existing code would cause a device hang if we allow it to be accessed even though it's suspended (at this point RPM_ACTIVE could be meaningless). I don't understand the original motivation of these code. If it's a valid case, most likely it should be handled in the specific device driver instead of the PM core. > > > > > > > > Perhaps it'd be better to rework __pm_runtime_resume() to convert > > > > > the -EACCES return value from rpm_resume() into 0 if RPM_GET_PUT is > > set? > > > > > > > > Or do something like this? > > > > > > > > --- > > > > drivers/base/power/runtime.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > Index: linux-pm/drivers/base/power/runtime.c > > > > > > ========================================================== > > ========= > > > > --- linux-pm.orig/drivers/base/power/runtime.c > > > > +++ linux-pm/drivers/base/power/runtime.c > > > > @@ -608,7 +608,8 @@ static int rpm_resume(struct device *dev > > > > repeat: > > > > if (dev->power.runtime_error) > > > > retval = -EINVAL; > > > > - else if (dev->power.disable_depth == 1 && dev- > > >power.is_suspended > > > > + else if (((dev->power.disable_depth > 0 && (rpmflags & > > RPM_GET_PUT)) > > > > + || (dev->power.disable_depth == 1 && dev- > > >power.is_suspended)) > > > > && dev->power.runtime_status == RPM_ACTIVE) > > > > retval = 1; > > > > else if (dev->power.disable_depth > 0) > > > > > > So pm_runtime_resume() and pm_request_resume() would still fail, but > > > pm_runtime_get() and pm_runtime_get_sync() would work? I'm not sure > > > about the reason for this distinction. > > > > The meaning of pm_runtime_get()/pm_runtime_get_sync() is "prevent the > > device from being suspended from now on and resume it if necessary" while > > "runtime PM disabled and runtime_status == RPM_ACTIVE" may be > > interpreted as "not necessary to resume", so it is reasonable to special case > > this particular situation for these particular routines IMHO. > > As Rafael mentioned above that runtime_sataus is not meaningful if runtime PM > is disabled, so shouldn't we avoid using the runtime_staus here and instead use > dev->power.is_suspended only to decide the return value? No, we shouldn't. This is a special case. If dev->power.disable_depth == 1 and dev->power.is_suspended is set at the same time, we know for a fact that runtime PM was only disabled by the system suspend code path and it was enabled otherwise, so dev->power.runtime_status equal to RPM_ACTIVE is actually meaningful in that particular case. > @@ -608,11 +608,13 @@ static int rpm_resume(struct device *dev, int rpmflags) > repeat: > if (dev->power.runtime_error) > retval = -EINVAL; > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > - && dev->power.runtime_status == RPM_ACTIVE) > - retval = 1; > - else if (dev->power.disable_depth > 0) > - retval = -EACCES; > + else if (dev->power.disable_depth > 0) { > + if (!dev->power.is_suspended) > + retval = 1; > + else > + retval = -EACCES; > + } > + > if (retval) > goto out; > > However, this requires us to make sure device is in full functional state if > it's not suspended before disabling runtime PM, just like the case runtime PM > is not configured at all. If runtime PM is not configured at all, the device has to be in full functional state (from the PM perspective) outside of the system suspend-resume sequence. The only problematic case I can see is when runtime PM is disabled, runtime_status is RPM_ACTIVE, but the device is actually suspended for some reason. I'd say that avoiding it is the caller's problem. > And also requires device suspend routine to check dev->power.usage_count before > suspending device. Why? And which routine exactly are you talking about? Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* RE: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-19 13:55 ` Rafael J. Wysocki @ 2014-06-19 14:34 ` Allen Yu 0 siblings, 0 replies; 65+ messages in thread From: Allen Yu @ 2014-06-19 14:34 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Alan Stern, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 9664 bytes --] On Thursday, June 19, 2014, Rafael J. Wysocki wrote: > On Thursday, June 19, 2014 04:23:29 PM Allen Yu wrote: > > On Thursday, June 19, 2014, Rafael J. Wysocki wrote: > > > On Wednesday, June 18, 2014 11:30:51 AM Alan Stern wrote: > > > > On Tue, 17 Jun 2014, Rafael J. Wysocki wrote: > > > > > > > > > On Tuesday, June 17, 2014 10:37:03 PM Rafael J. Wysocki wrote: > > > > > > On Tuesday, June 17, 2014 10:26:14 PM Rafael J. Wysocki wrote: > > > > > > > On Tuesday, June 17, 2014 10:11:32 AM Alan Stern wrote: > > > > > > > > On Mon, 16 Jun 2014, Rafael J. Wysocki wrote: > > > > > > > > > > > > > > > > > > For reasons having nothing to do with Allen's > > > > > > > > > > suggested change, I wonder if we shouldn't replace > > > > > > > > > > this line with > > > something like: > > > > > > > > > > > > > > > > > > > > - else if (dev->power.disable_depth == 1 && dev- > > > >power.is_suspended > > > > > > > > > > + else if (dev->power.disable > 0 && > > > > > > > > > > +!dev->power.is_suspended > > > > > > > > > > && dev->power.runtime_status == RPM_ACTIVE) > > > > > > > > > > retval = 1; > > > > > > > > > > > > > > > > > > > > It seems that I've been bitten by this several times in the > past. > > > > > > > > > > When a device is disabled for runtime PM, and more or > > > > > > > > > > less permanently stuck in the RPM_ACTIVE state, calls > > > > > > > > > > to > > > > > > > > > > pm_runtime_resume() or > > > > > > > > > > pm_runtime_get_sync() shouldn't fail. > > > > > > > > > > > > > > > > > > > > For example, suppose some devices of a certain type > > > > > > > > > > support runtime power management but others don't. We > > > > > > > > > > naturally want to call > > > > > > > > > > pm_runtime_disable() for the ones that don't. But we > > > > > > > > > > also want the same driver to work for all the devices, > > > > > > > > > > which means that > > > > > > > > > > pm_runtime_get_sync() should return success -- > > > > > > > > > > otherwise the driver will think that something has gone > wrong. > > > > > > > > > > > > > > > > > > > > Rafael, what do you think? > > > > > > > > > > > > > > > > > > That condition is there specifically to take care of the > > > > > > > > > system suspend code path. It means that if runtime PM > > > > > > > > > is disabled, but it only has been disabled by the system > > > > > > > > > suspend code path, we should treat the device as "active" (ie. > > > return 1). That won't work after the proposed change. > > > > > > > > > > > > > > > > Ah, yes, quite true. Okay, suppose we replace that line with just: > > > > > > > > > > > > > > > > + else if (dev->power.disable > 0 > > > > > > > > > > > > > > > > > I guess drivers that want to work with devices where > > > > > > > > > runtime PM may be disabled can just check the return > > > > > > > > > value of > > > rpm_resume() for -EACCES? > > > > > > > > > > > > > > > > They could, but it's extra work and it's extremely easy to > > > > > > > > forget about. I'd prefer not to do things that way. > > > > > > > > > > > > > > In that case we need to audit all code that checks the > > > > > > > return value of > > > > > > > __pm_runtime_resume() to verify that it doesn't depend on > > > > > > > the current behavior in any way. It shouldn't, but still. > > > > > > > > > > > > > > Also we probably should drop the -EACCES return value from > > > > > > > rpm_resume() in the same patch, because it specifically only > > > > > > > covers the dev->power.disable > 0 case (which BTW is > > > > > > > consistent with the suspend side of things, so I'm totally > > > > > > > unsure about that being > > > the right thing to do to be honest). > > > > > > > > It's still the correct action with runtime PM is disabled and the > > > > device's runtime_status isn't RPM_ACTIVE. > > > > > > Well, we used to have the notion that runtime_status is not > > > meaningful for devices with dev->power.disable_depth greater than 0 > > > (except for the special case in the suspend code path where we know > > > why it is greater than 0). I think it was useful. :-) > > > > So what's the exact state of device if dev->power.is_suspended flag is set > and runtime_status is RPM_ACTIVE? Is it a state like "suspended but still can > be accessed"? > > > > I'm just afraid the existing code would cause a device hang if we allow it to > be accessed even though it's suspended (at this point RPM_ACTIVE could be > meaningless). I don't understand the original motivation of these code. If it's > a valid case, most likely it should be handled in the specific device driver > instead of the PM core. > > > > > > > > > > > Perhaps it'd be better to rework __pm_runtime_resume() to > > > > > > convert the -EACCES return value from rpm_resume() into 0 if > > > > > > RPM_GET_PUT is > > > set? > > > > > > > > > > Or do something like this? > > > > > > > > > > --- > > > > > drivers/base/power/runtime.c | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > Index: linux-pm/drivers/base/power/runtime.c > > > > > > > > > ========================================================== > > > ========= > > > > > --- linux-pm.orig/drivers/base/power/runtime.c > > > > > +++ linux-pm/drivers/base/power/runtime.c > > > > > @@ -608,7 +608,8 @@ static int rpm_resume(struct device *dev > > > > > repeat: > > > > > if (dev->power.runtime_error) > > > > > retval = -EINVAL; > > > > > - else if (dev->power.disable_depth == 1 && dev- > > > >power.is_suspended > > > > > + else if (((dev->power.disable_depth > 0 && (rpmflags & > > > RPM_GET_PUT)) > > > > > + || (dev->power.disable_depth == 1 && dev- > > > >power.is_suspended)) > > > > > && dev->power.runtime_status == RPM_ACTIVE) > > > > > retval = 1; > > > > > else if (dev->power.disable_depth > 0) > > > > > > > > So pm_runtime_resume() and pm_request_resume() would still fail, > > > > but > > > > pm_runtime_get() and pm_runtime_get_sync() would work? I'm not > > > > sure about the reason for this distinction. > > > > > > The meaning of pm_runtime_get()/pm_runtime_get_sync() is "prevent > > > the device from being suspended from now on and resume it if > > > necessary" while "runtime PM disabled and runtime_status == > > > RPM_ACTIVE" may be interpreted as "not necessary to resume", so it > > > is reasonable to special case this particular situation for these particular > routines IMHO. > > > > As Rafael mentioned above that runtime_sataus is not meaningful if > > runtime PM is disabled, so shouldn't we avoid using the runtime_staus > > here and instead use > > dev->power.is_suspended only to decide the return value? > > No, we shouldn't. > > This is a special case. If dev->power.disable_depth == 1 and dev- > >power.is_suspended is set at the same time, we know for a fact that > runtime PM was only disabled by the system suspend code path and it was > enabled otherwise, so dev->power.runtime_status equal to RPM_ACTIVE is > actually meaningful in that particular case. I'm still confused about the state of device if dev->power.is_suspended flag is set and runtime_status is RPM_ACTIVE. Is it a state like "suspended but still can be accessed"? > > > @@ -608,11 +608,13 @@ static int rpm_resume(struct device *dev, int > rpmflags) > > repeat: > > if (dev->power.runtime_error) > > retval = -EINVAL; > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > - && dev->power.runtime_status == RPM_ACTIVE) > > - retval = 1; > > - else if (dev->power.disable_depth > 0) > > - retval = -EACCES; > > + else if (dev->power.disable_depth > 0) { > > + if (!dev->power.is_suspended) > > + retval = 1; > > + else > > + retval = -EACCES; > > + } > > + > > if (retval) > > goto out; > > > > However, this requires us to make sure device is in full functional > > state if it's not suspended before disabling runtime PM, just like the > > case runtime PM is not configured at all. > > If runtime PM is not configured at all, the device has to be in full functional > state (from the PM perspective) outside of the system suspend-resume > sequence. So before disabling runtime PM of a device, caller need to make it full functional as long as it's outside of system suspend-resume sequence (or to be more specific, is_suspended flag is not set)? - This is in fact what my comment above meant. If this is true, can't we just return 1 in case dev->power.disable_depth > 0 && dev->power.is_suspended == false? - This is in fact what the code above meant to do. This should work for both pm_runtime_resume() and pm_runtime_get() then. > > The only problematic case I can see is when runtime PM is disabled, > runtime_status is RPM_ACTIVE, but the device is actually suspended for > some reason. I'd say that avoiding it is the caller's problem. > > > And also requires device suspend routine to check > > dev->power.usage_count before suspending device. > > Why? And which routine exactly are you talking about? I meant the device suspend callback like dev->bus->pm->suspend. Suspend callback need to check dev->power.usage_count because if it's greater than 1 (device_prepare already called pm_runtime_get_noresume() on device so the usage_count is at least 1), it means somewhere else has called pm_runtime_get() on the device thus it should not be suspended. > > Rafael ÿôèº{.nÇ+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥ ^ permalink raw reply [flat|nested] 65+ messages in thread
* RE: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. @ 2014-06-19 14:34 ` Allen Yu 0 siblings, 0 replies; 65+ messages in thread From: Allen Yu @ 2014-06-19 14:34 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Alan Stern, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Thursday, June 19, 2014, Rafael J. Wysocki wrote: > On Thursday, June 19, 2014 04:23:29 PM Allen Yu wrote: > > On Thursday, June 19, 2014, Rafael J. Wysocki wrote: > > > On Wednesday, June 18, 2014 11:30:51 AM Alan Stern wrote: > > > > On Tue, 17 Jun 2014, Rafael J. Wysocki wrote: > > > > > > > > > On Tuesday, June 17, 2014 10:37:03 PM Rafael J. Wysocki wrote: > > > > > > On Tuesday, June 17, 2014 10:26:14 PM Rafael J. Wysocki wrote: > > > > > > > On Tuesday, June 17, 2014 10:11:32 AM Alan Stern wrote: > > > > > > > > On Mon, 16 Jun 2014, Rafael J. Wysocki wrote: > > > > > > > > > > > > > > > > > > For reasons having nothing to do with Allen's > > > > > > > > > > suggested change, I wonder if we shouldn't replace > > > > > > > > > > this line with > > > something like: > > > > > > > > > > > > > > > > > > > > - else if (dev->power.disable_depth == 1 && dev- > > > >power.is_suspended > > > > > > > > > > + else if (dev->power.disable > 0 && > > > > > > > > > > +!dev->power.is_suspended > > > > > > > > > > && dev->power.runtime_status == RPM_ACTIVE) > > > > > > > > > > retval = 1; > > > > > > > > > > > > > > > > > > > > It seems that I've been bitten by this several times in the > past. > > > > > > > > > > When a device is disabled for runtime PM, and more or > > > > > > > > > > less permanently stuck in the RPM_ACTIVE state, calls > > > > > > > > > > to > > > > > > > > > > pm_runtime_resume() or > > > > > > > > > > pm_runtime_get_sync() shouldn't fail. > > > > > > > > > > > > > > > > > > > > For example, suppose some devices of a certain type > > > > > > > > > > support runtime power management but others don't. We > > > > > > > > > > naturally want to call > > > > > > > > > > pm_runtime_disable() for the ones that don't. But we > > > > > > > > > > also want the same driver to work for all the devices, > > > > > > > > > > which means that > > > > > > > > > > pm_runtime_get_sync() should return success -- > > > > > > > > > > otherwise the driver will think that something has gone > wrong. > > > > > > > > > > > > > > > > > > > > Rafael, what do you think? > > > > > > > > > > > > > > > > > > That condition is there specifically to take care of the > > > > > > > > > system suspend code path. It means that if runtime PM > > > > > > > > > is disabled, but it only has been disabled by the system > > > > > > > > > suspend code path, we should treat the device as "active" (ie. > > > return 1). That won't work after the proposed change. > > > > > > > > > > > > > > > > Ah, yes, quite true. Okay, suppose we replace that line with just: > > > > > > > > > > > > > > > > + else if (dev->power.disable > 0 > > > > > > > > > > > > > > > > > I guess drivers that want to work with devices where > > > > > > > > > runtime PM may be disabled can just check the return > > > > > > > > > value of > > > rpm_resume() for -EACCES? > > > > > > > > > > > > > > > > They could, but it's extra work and it's extremely easy to > > > > > > > > forget about. I'd prefer not to do things that way. > > > > > > > > > > > > > > In that case we need to audit all code that checks the > > > > > > > return value of > > > > > > > __pm_runtime_resume() to verify that it doesn't depend on > > > > > > > the current behavior in any way. It shouldn't, but still. > > > > > > > > > > > > > > Also we probably should drop the -EACCES return value from > > > > > > > rpm_resume() in the same patch, because it specifically only > > > > > > > covers the dev->power.disable > 0 case (which BTW is > > > > > > > consistent with the suspend side of things, so I'm totally > > > > > > > unsure about that being > > > the right thing to do to be honest). > > > > > > > > It's still the correct action with runtime PM is disabled and the > > > > device's runtime_status isn't RPM_ACTIVE. > > > > > > Well, we used to have the notion that runtime_status is not > > > meaningful for devices with dev->power.disable_depth greater than 0 > > > (except for the special case in the suspend code path where we know > > > why it is greater than 0). I think it was useful. :-) > > > > So what's the exact state of device if dev->power.is_suspended flag is set > and runtime_status is RPM_ACTIVE? Is it a state like "suspended but still can > be accessed"? > > > > I'm just afraid the existing code would cause a device hang if we allow it to > be accessed even though it's suspended (at this point RPM_ACTIVE could be > meaningless). I don't understand the original motivation of these code. If it's > a valid case, most likely it should be handled in the specific device driver > instead of the PM core. > > > > > > > > > > > Perhaps it'd be better to rework __pm_runtime_resume() to > > > > > > convert the -EACCES return value from rpm_resume() into 0 if > > > > > > RPM_GET_PUT is > > > set? > > > > > > > > > > Or do something like this? > > > > > > > > > > --- > > > > > drivers/base/power/runtime.c | 3 ++- > > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > > > Index: linux-pm/drivers/base/power/runtime.c > > > > > > > > > ========================================================== > > > ========= > > > > > --- linux-pm.orig/drivers/base/power/runtime.c > > > > > +++ linux-pm/drivers/base/power/runtime.c > > > > > @@ -608,7 +608,8 @@ static int rpm_resume(struct device *dev > > > > > repeat: > > > > > if (dev->power.runtime_error) > > > > > retval = -EINVAL; > > > > > - else if (dev->power.disable_depth == 1 && dev- > > > >power.is_suspended > > > > > + else if (((dev->power.disable_depth > 0 && (rpmflags & > > > RPM_GET_PUT)) > > > > > + || (dev->power.disable_depth == 1 && dev- > > > >power.is_suspended)) > > > > > && dev->power.runtime_status == RPM_ACTIVE) > > > > > retval = 1; > > > > > else if (dev->power.disable_depth > 0) > > > > > > > > So pm_runtime_resume() and pm_request_resume() would still fail, > > > > but > > > > pm_runtime_get() and pm_runtime_get_sync() would work? I'm not > > > > sure about the reason for this distinction. > > > > > > The meaning of pm_runtime_get()/pm_runtime_get_sync() is "prevent > > > the device from being suspended from now on and resume it if > > > necessary" while "runtime PM disabled and runtime_status == > > > RPM_ACTIVE" may be interpreted as "not necessary to resume", so it > > > is reasonable to special case this particular situation for these particular > routines IMHO. > > > > As Rafael mentioned above that runtime_sataus is not meaningful if > > runtime PM is disabled, so shouldn't we avoid using the runtime_staus > > here and instead use > > dev->power.is_suspended only to decide the return value? > > No, we shouldn't. > > This is a special case. If dev->power.disable_depth == 1 and dev- > >power.is_suspended is set at the same time, we know for a fact that > runtime PM was only disabled by the system suspend code path and it was > enabled otherwise, so dev->power.runtime_status equal to RPM_ACTIVE is > actually meaningful in that particular case. I'm still confused about the state of device if dev->power.is_suspended flag is set and runtime_status is RPM_ACTIVE. Is it a state like "suspended but still can be accessed"? > > > @@ -608,11 +608,13 @@ static int rpm_resume(struct device *dev, int > rpmflags) > > repeat: > > if (dev->power.runtime_error) > > retval = -EINVAL; > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > - && dev->power.runtime_status == RPM_ACTIVE) > > - retval = 1; > > - else if (dev->power.disable_depth > 0) > > - retval = -EACCES; > > + else if (dev->power.disable_depth > 0) { > > + if (!dev->power.is_suspended) > > + retval = 1; > > + else > > + retval = -EACCES; > > + } > > + > > if (retval) > > goto out; > > > > However, this requires us to make sure device is in full functional > > state if it's not suspended before disabling runtime PM, just like the > > case runtime PM is not configured at all. > > If runtime PM is not configured at all, the device has to be in full functional > state (from the PM perspective) outside of the system suspend-resume > sequence. So before disabling runtime PM of a device, caller need to make it full functional as long as it's outside of system suspend-resume sequence (or to be more specific, is_suspended flag is not set)? - This is in fact what my comment above meant. If this is true, can't we just return 1 in case dev->power.disable_depth > 0 && dev->power.is_suspended == false? - This is in fact what the code above meant to do. This should work for both pm_runtime_resume() and pm_runtime_get() then. > > The only problematic case I can see is when runtime PM is disabled, > runtime_status is RPM_ACTIVE, but the device is actually suspended for > some reason. I'd say that avoiding it is the caller's problem. > > > And also requires device suspend routine to check > > dev->power.usage_count before suspending device. > > Why? And which routine exactly are you talking about? I meant the device suspend callback like dev->bus->pm->suspend. Suspend callback need to check dev->power.usage_count because if it's greater than 1 (device_prepare already called pm_runtime_get_noresume() on device so the usage_count is at least 1), it means somewhere else has called pm_runtime_get() on the device thus it should not be suspended. > > Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-19 14:34 ` Allen Yu (?) @ 2014-06-20 14:04 ` Rafael J. Wysocki -1 siblings, 0 replies; 65+ messages in thread From: Rafael J. Wysocki @ 2014-06-20 14:04 UTC (permalink / raw) To: Allen Yu Cc: Alan Stern, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Thursday, June 19, 2014 10:34:51 PM Allen Yu wrote: > On Thursday, June 19, 2014, Rafael J. Wysocki wrote: > > On Thursday, June 19, 2014 04:23:29 PM Allen Yu wrote: > > > On Thursday, June 19, 2014, Rafael J. Wysocki wrote: [cut] > > > > Well, we used to have the notion that runtime_status is not > > > > meaningful for devices with dev->power.disable_depth greater than 0 > > > > (except for the special case in the suspend code path where we know > > > > why it is greater than 0). I think it was useful. :-) > > > > > So what's the exact state of device if dev->power.is_suspended flag is set > > and runtime_status is RPM_ACTIVE? Is it a state like "suspended but still can > > be accessed"? The real state of the device should be known to its driver at this point anyway. From the runtime PM perspectiver it is "active", because runtime PM doesn't track system suspend operations. From the system suspend sequence standpoint it is "suspended", but that only means that its system resume callbacks should be run for it before completing the sequence. The PM core doesn't have a common device status notion valid for both runtime PM and the system suspend/resume frameworks at the same time. Perhaps it would be useful to add something like that to the PM core, but it hasn't been clearly demonstrated so far. > > > > > I'm just afraid the existing code would cause a device hang if we allow it to > > be accessed even though it's suspended (at this point RPM_ACTIVE could be That depends on what "we" means here. The driver/bus type are responsible for maintaining the correct state of the device. > > meaningless). I don't understand the original motivation of these code. If it's > > a valid case, most likely it should be handled in the specific device driver > > instead of the PM core. You may be right here (please follow the Alan's discussion with Kevin). [cut] > > > > > > As Rafael mentioned above that runtime_sataus is not meaningful if > > > runtime PM is disabled, so shouldn't we avoid using the runtime_staus > > > here and instead use > > > dev->power.is_suspended only to decide the return value? > > > > No, we shouldn't. > > > > This is a special case. If dev->power.disable_depth == 1 and dev- > > >power.is_suspended is set at the same time, we know for a fact that > > runtime PM was only disabled by the system suspend code path and it was > > enabled otherwise, so dev->power.runtime_status equal to RPM_ACTIVE is > > actually meaningful in that particular case. > > I'm still confused about the state of device if dev->power.is_suspended flag is set > and runtime_status is RPM_ACTIVE. Is it a state like "suspended but still can > be accessed"? Please see above. That basically is for drivers that want to run pm_runtime_get_sync() from their late suspend or early resume system suspend callbacks and don't want to worry about the return value of that. It may be argued that this is a hack, but then it is a somewhat useful one. :-) > > > > > @@ -608,11 +608,13 @@ static int rpm_resume(struct device *dev, int > > rpmflags) > > > repeat: > > > if (dev->power.runtime_error) > > > retval = -EINVAL; > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > > - && dev->power.runtime_status == RPM_ACTIVE) > > > - retval = 1; > > > - else if (dev->power.disable_depth > 0) > > > - retval = -EACCES; > > > + else if (dev->power.disable_depth > 0) { > > > + if (!dev->power.is_suspended) > > > + retval = 1; > > > + else > > > + retval = -EACCES; > > > + } > > > + > > > if (retval) > > > goto out; > > > > > > However, this requires us to make sure device is in full functional > > > state if it's not suspended before disabling runtime PM, just like the > > > case runtime PM is not configured at all. > > > > If runtime PM is not configured at all, the device has to be in full functional > > state (from the PM perspective) outside of the system suspend-resume > > sequence. > > So before disabling runtime PM of a device, caller need to make it full > functional as long as it's outside of system suspend-resume sequence (or > to be more specific, is_suspended flag is not set)? No, it doesn't need to do that. Runtime PM disabled means "don't execute runtime PM callbacks for this device". It has nothing to do with the device's state. However, *before* enabling runtime PM for the device again the caller must ensure that runtime_status is appropriate. And the runtime PM status only means which runtime PM callback has been executed for the device most recently, which information is only useful to the PM core if it is *allowed* to execute runtime PM callbacks for that device. > - This is in fact what my comment above meant. > If this is true, It isn't. > can't we just return 1 in > case dev->power.disable_depth > 0 && dev->power.is_suspended == false? Why do we need to check dev->power.is_suspended here at all, then? > - This is in fact what the code above meant to do. > This should work for both pm_runtime_resume() and pm_runtime_get() then. What's the use case? I don't see it, honestly. That seems to be another instance of "I don't need to track the state of the device in the driver, because the PM core does that for me" thinking, which isn't correct, because the PM core doesn't do that. Can you please accept the fact that runtime PM and system suspend/resume are different frameworks and drivers need to do some work to coordinate between them? And they should not use the PM core's internal data structures for that, because those data structures are not suitable for this purpose in many cases? Yes, we need to integrate runtime PM with system suspend/resume more tightly, but not by making ad hoc changes to the PM core. Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* RE: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-19 8:23 ` Allen Yu (?) (?) @ 2014-06-19 14:56 ` Alan Stern 2014-06-19 19:25 ` Kevin Hilman -1 siblings, 1 reply; 65+ messages in thread From: Alan Stern @ 2014-06-19 14:56 UTC (permalink / raw) To: Allen Yu, Kevin Hilman Cc: Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Thu, 19 Jun 2014, Allen Yu wrote: > So what's the exact state of device if dev->power.is_suspended flag > is set and runtime_status is RPM_ACTIVE? Is it a state like > "suspended but still can be accessed"? > > I'm just afraid the existing code would cause a device hang if we > allow it to be accessed even though it's suspended (at this point > RPM_ACTIVE could be meaningless). I don't understand the original > motivation of these code. If it's a valid case, most likely it should > be handled in the specific device driver instead of the PM core. You should read the Changelog for commit 6f3c77b040f (PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2). It explains why the code looks the way it does. However, I'm starting to think the reasoning in that commit may not be valid. While perhaps it is okay for some I2C devices (mentioned in the commit log), it probably isn't okay in general. Kevin, do have any comments on this matter? What do you think about making the following change to rpm_resume(): repeat: if (dev->power.runtime_error) retval = -EINVAL; - else if (dev->power.disable_depth == 1 && dev->power.is_suspended + else if (dev->power.disable_depth > 0 && dev->power.runtime_status == RPM_ACTIVE) retval = 1; else if (dev->power.disable_depth > 0) Or even: + else if (dev->power.disable_depth > 0 && !dev->power.is_suspended although this would require the I2C driver you mentioned in your commit to change. Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-19 14:56 ` Alan Stern @ 2014-06-19 19:25 ` Kevin Hilman 0 siblings, 0 replies; 65+ messages in thread From: Kevin Hilman @ 2014-06-19 19:25 UTC (permalink / raw) To: Alan Stern Cc: Allen Yu, Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel Alan Stern <stern@rowland.harvard.edu> writes: > On Thu, 19 Jun 2014, Allen Yu wrote: > >> So what's the exact state of device if dev->power.is_suspended flag >> is set and runtime_status is RPM_ACTIVE? Is it a state like >> "suspended but still can be accessed"? >> >> I'm just afraid the existing code would cause a device hang if we >> allow it to be accessed even though it's suspended (at this point >> RPM_ACTIVE could be meaningless). I don't understand the original >> motivation of these code. If it's a valid case, most likely it should >> be handled in the specific device driver instead of the PM core. > > You should read the Changelog for commit 6f3c77b040f (PM / Runtime: > let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2). It > explains why the code looks the way it does. > > However, I'm starting to think the reasoning in that commit may not be > valid. While perhaps it is okay for some I2C devices (mentioned in the > commit log), it probably isn't okay in general. Why not? > Kevin, do have any comments on this matter? What do you think about > making the following change to rpm_resume(): > > repeat: > if (dev->power.runtime_error) > retval = -EINVAL; > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > + else if (dev->power.disable_depth > 0 > && dev->power.runtime_status == RPM_ACTIVE) > retval = 1; > else if (dev->power.disable_depth > 0) > > Or even: > > + else if (dev->power.disable_depth > 0 && !dev->power.is_suspended > > although this would require the I2C driver you mentioned in your commit > to change. My change was introduced to catch a very specific case. Namely, when we know that the core has successfully asked the device to do a system suspend (dev->power.is_suspended == true) *and* we know that runtime PM was disabled *only* by the PM core (disable_depth == 0) while the device was still active (runtime_status == RPM_ACTIVE.) In your first idea above, it would allow a _get() to succeed even if someone other than the core (including the driver itself) had called pm_runtime_disable(). I don't think we want that. In the second idea above, we'd completely miss the case where runtime PM has been disabled by the core (because the core would have set dev->power.is_suspended) In both cases, we're no longer just checking for that specific condition I was after, so I'd have to spend more time thinking about any other possible consequences as well. I think part of the confusion here is coming from what dev->power.is_suspended means. From the core's perspective, it just means that the core has called the ->suspend callback, and didn't detect any errors. Depending on the driver though, it doesn't have to mean that the device is actually fully suspended. For example, there are several cases of "runtime PM centric" drivers are may be needed by other devices during the system suspend/resume process and so are runtime PM resumed during system suspend. Kevin ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. @ 2014-06-19 19:25 ` Kevin Hilman 0 siblings, 0 replies; 65+ messages in thread From: Kevin Hilman @ 2014-06-19 19:25 UTC (permalink / raw) To: Alan Stern Cc: Allen Yu, Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel Alan Stern <stern@rowland.harvard.edu> writes: > On Thu, 19 Jun 2014, Allen Yu wrote: > >> So what's the exact state of device if dev->power.is_suspended flag >> is set and runtime_status is RPM_ACTIVE? Is it a state like >> "suspended but still can be accessed"? >> >> I'm just afraid the existing code would cause a device hang if we >> allow it to be accessed even though it's suspended (at this point >> RPM_ACTIVE could be meaningless). I don't understand the original >> motivation of these code. If it's a valid case, most likely it should >> be handled in the specific device driver instead of the PM core. > > You should read the Changelog for commit 6f3c77b040f (PM / Runtime: > let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2). It > explains why the code looks the way it does. > > However, I'm starting to think the reasoning in that commit may not be > valid. While perhaps it is okay for some I2C devices (mentioned in the > commit log), it probably isn't okay in general. Why not? > Kevin, do have any comments on this matter? What do you think about > making the following change to rpm_resume(): > > repeat: > if (dev->power.runtime_error) > retval = -EINVAL; > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > + else if (dev->power.disable_depth > 0 > && dev->power.runtime_status == RPM_ACTIVE) > retval = 1; > else if (dev->power.disable_depth > 0) > > Or even: > > + else if (dev->power.disable_depth > 0 && !dev->power.is_suspended > > although this would require the I2C driver you mentioned in your commit > to change. My change was introduced to catch a very specific case. Namely, when we know that the core has successfully asked the device to do a system suspend (dev->power.is_suspended == true) *and* we know that runtime PM was disabled *only* by the PM core (disable_depth == 0) while the device was still active (runtime_status == RPM_ACTIVE.) In your first idea above, it would allow a _get() to succeed even if someone other than the core (including the driver itself) had called pm_runtime_disable(). I don't think we want that. In the second idea above, we'd completely miss the case where runtime PM has been disabled by the core (because the core would have set dev->power.is_suspended) In both cases, we're no longer just checking for that specific condition I was after, so I'd have to spend more time thinking about any other possible consequences as well. I think part of the confusion here is coming from what dev->power.is_suspended means. From the core's perspective, it just means that the core has called the ->suspend callback, and didn't detect any errors. Depending on the driver though, it doesn't have to mean that the device is actually fully suspended. For example, there are several cases of "runtime PM centric" drivers are may be needed by other devices during the system suspend/resume process and so are runtime PM resumed during system suspend. Kevin ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-19 19:25 ` Kevin Hilman (?) @ 2014-06-19 20:13 ` Alan Stern 2014-06-20 13:20 ` Rafael J. Wysocki 2014-06-20 21:31 ` Kevin Hilman -1 siblings, 2 replies; 65+ messages in thread From: Alan Stern @ 2014-06-19 20:13 UTC (permalink / raw) To: Kevin Hilman Cc: Allen Yu, Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Thu, 19 Jun 2014, Kevin Hilman wrote: > Alan Stern <stern@rowland.harvard.edu> writes: > > > On Thu, 19 Jun 2014, Allen Yu wrote: > > > >> So what's the exact state of device if dev->power.is_suspended flag > >> is set and runtime_status is RPM_ACTIVE? Is it a state like > >> "suspended but still can be accessed"? > >> > >> I'm just afraid the existing code would cause a device hang if we > >> allow it to be accessed even though it's suspended (at this point > >> RPM_ACTIVE could be meaningless). I don't understand the original > >> motivation of these code. If it's a valid case, most likely it should > >> be handled in the specific device driver instead of the PM core. > > > > You should read the Changelog for commit 6f3c77b040f (PM / Runtime: > > let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2). It > > explains why the code looks the way it does. > > > > However, I'm starting to think the reasoning in that commit may not be > > valid. While perhaps it is okay for some I2C devices (mentioned in the > > commit log), it probably isn't okay in general. > > Why not? See below. > > Kevin, do have any comments on this matter? What do you think about > > making the following change to rpm_resume(): > > > > repeat: > > if (dev->power.runtime_error) > > retval = -EINVAL; > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > + else if (dev->power.disable_depth > 0 > > && dev->power.runtime_status == RPM_ACTIVE) > > retval = 1; > > else if (dev->power.disable_depth > 0) > > > > Or even: > > > > + else if (dev->power.disable_depth > 0 && !dev->power.is_suspended > > > > although this would require the I2C driver you mentioned in your commit > > to change. > > My change was introduced to catch a very specific case. Namely, when we > know that the core has successfully asked the device to do a system suspend > (dev->power.is_suspended == true) *and* we know that runtime PM was > disabled *only* by the PM core (disable_depth == 0) while the device was > still active (runtime_status == RPM_ACTIVE.) For a general device, the fact that dev->power.is_suspended is set means the device _has_ been powered down. Even though the runtime_status may not have changed, the PM core has to assume the device is not available for use. While your I2C devices may be useable even after the ->suspend callback returns, for most devices this isn't true. So we shouldn't allow rpm_resume() to return imediately when is_suspended is set. > In your first idea above, it would allow a _get() to succeed even if > someone other than the core (including the driver itself) had called > pm_runtime_disable(). I don't think we want that. Why not? The fact that the device is disabled for runtime PM means that the PM core mustn't try to change its power state. But if the runtime status is RPM_ACTIVE then the device should already be powered up, so there's no harm in letting runtime_resume() succeed. To put it another way, disabled_depth > 0 means that the PM core isn't allowed to invoke any of the runtime PM callbacks. But when runtime_status == RPM_ACTIVE, runtime_resume() can run successfully without invoking any callbacks. > In the second idea above, we'd completely miss the case where runtime PM > has been disabled by the core (because the core would have set > dev->power.is_suspended) That's the intention. When is_suspended is set, the PM core assumes that the device has been powered down in preparation for system suspend. We don't want to mess that up by performing a runtime resume. > In both cases, we're no longer just checking for that specific condition > I was after, so I'd have to spend more time thinking about any other > possible consequences as well. Indeed. Hopefully the fallout won't affect more than a few drivers. > I think part of the confusion here is coming from what > dev->power.is_suspended means. From the core's perspective, it just > means that the core has called the ->suspend callback, and didn't detect > any errors. Yes. But the core also has to assume that the ->runtime_resume callback will undo the effect of ->suspend. Therefore the core should not call ->runtime_resume if is_suspended is set. > Depending on the driver though, it doesn't have to mean that the device > is actually fully suspended. For example, there are several cases of > "runtime PM centric" drivers are may be needed by other devices during > the system suspend/resume process and so are runtime PM resumed during > system suspend. At what stage do these devices get powered down? During suspend_late or suspend_irq? At such times the PM core won't invoke the runtime PM callbacks anyway. It sounds like what you really want for these devices is to have dev->power.is_suspended get set at the start of __device_suspend_late() rather than at the end of __device_suspend(). Or maybe even not to get set at all. Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-19 20:13 ` Alan Stern @ 2014-06-20 13:20 ` Rafael J. Wysocki 2014-06-20 14:48 ` Alan Stern 2014-06-20 21:31 ` Kevin Hilman 1 sibling, 1 reply; 65+ messages in thread From: Rafael J. Wysocki @ 2014-06-20 13:20 UTC (permalink / raw) To: Alan Stern Cc: Kevin Hilman, Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Thursday, June 19, 2014 04:13:07 PM Alan Stern wrote: > On Thu, 19 Jun 2014, Kevin Hilman wrote: > > > Alan Stern <stern@rowland.harvard.edu> writes: > > > > > On Thu, 19 Jun 2014, Allen Yu wrote: > > > > > >> So what's the exact state of device if dev->power.is_suspended flag > > >> is set and runtime_status is RPM_ACTIVE? Is it a state like > > >> "suspended but still can be accessed"? > > >> > > >> I'm just afraid the existing code would cause a device hang if we > > >> allow it to be accessed even though it's suspended (at this point > > >> RPM_ACTIVE could be meaningless). I don't understand the original > > >> motivation of these code. If it's a valid case, most likely it should > > >> be handled in the specific device driver instead of the PM core. > > > > > > You should read the Changelog for commit 6f3c77b040f (PM / Runtime: > > > let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2). It > > > explains why the code looks the way it does. > > > > > > However, I'm starting to think the reasoning in that commit may not be > > > valid. While perhaps it is okay for some I2C devices (mentioned in the > > > commit log), it probably isn't okay in general. > > > > Why not? > > See below. > > > > Kevin, do have any comments on this matter? What do you think about > > > making the following change to rpm_resume(): > > > > > > repeat: > > > if (dev->power.runtime_error) > > > retval = -EINVAL; > > > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended > > > + else if (dev->power.disable_depth > 0 > > > && dev->power.runtime_status == RPM_ACTIVE) > > > retval = 1; > > > else if (dev->power.disable_depth > 0) > > > > > > Or even: > > > > > > + else if (dev->power.disable_depth > 0 && !dev->power.is_suspended > > > > > > although this would require the I2C driver you mentioned in your commit > > > to change. > > > > My change was introduced to catch a very specific case. Namely, when we > > know that the core has successfully asked the device to do a system suspend > > (dev->power.is_suspended == true) *and* we know that runtime PM was > > disabled *only* by the PM core (disable_depth == 0) while the device was > > still active (runtime_status == RPM_ACTIVE.) > > For a general device, the fact that dev->power.is_suspended is set > means the device _has_ been powered down. Even though the > runtime_status may not have changed, the PM core has to assume the > device is not available for use. This seems to go a bit too far. What power.is_suspended actually means is that __device_suspend() has run for the device successfully. What the implications of that are depends on the bus type (or subsystem in general) and device driver. > While your I2C devices may be useable even after the ->suspend callback > returns, for most devices this isn't true. So we shouldn't allow > rpm_resume() to return imediately when is_suspended is set. I can agree with that. > > In your first idea above, it would allow a _get() to succeed even if > > someone other than the core (including the driver itself) had called > > pm_runtime_disable(). I don't think we want that. > > Why not? The fact that the device is disabled for runtime PM means > that the PM core mustn't try to change its power state. But if the > runtime status is RPM_ACTIVE then the device should already be powered > up, so there's no harm in letting runtime_resume() succeed. > > To put it another way, disabled_depth > 0 means that the PM core isn't > allowed to invoke any of the runtime PM callbacks. But when > runtime_status == RPM_ACTIVE, runtime_resume() can run successfully > without invoking any callbacks. Theoretically. That is, unless someone changes the status from RPM_SUSPENDED to RPM_ACTIVE while runtime PM is disabled for the device, which is documented as a *valid* thing to do. So really, perhaps we should go back to thinking that runtime_status is meaningless while runtime PM is disabled, which really is the case? Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-20 13:20 ` Rafael J. Wysocki @ 2014-06-20 14:48 ` Alan Stern 2014-06-20 21:34 ` Kevin Hilman 2014-06-22 13:24 ` Rafael J. Wysocki 0 siblings, 2 replies; 65+ messages in thread From: Alan Stern @ 2014-06-20 14:48 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Kevin Hilman, Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Fri, 20 Jun 2014, Rafael J. Wysocki wrote: > > For a general device, the fact that dev->power.is_suspended is set > > means the device _has_ been powered down. Even though the > > runtime_status may not have changed, the PM core has to assume the > > device is not available for use. > > This seems to go a bit too far. What power.is_suspended actually means is > that __device_suspend() has run for the device successfully. What the > implications of that are depends on the bus type (or subsystem in general) > and device driver. > > > While your I2C devices may be useable even after the ->suspend callback > > returns, for most devices this isn't true. So we shouldn't allow > > rpm_resume() to return imediately when is_suspended is set. > > I can agree with that. We really do need to decide more precisely how runtime PM and system PM will interact. Should ->runtime_resume callbacks be allowed after ->suspend has returned? Kevin has stated that some devices do need this ability. But most don't. The PM core needs to handle these conflicting requirements somehow. Note: this is a separate issue from the meaning of disabled_depth > 0. Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-20 14:48 ` Alan Stern @ 2014-06-20 21:34 ` Kevin Hilman 2014-06-22 13:24 ` Rafael J. Wysocki 1 sibling, 0 replies; 65+ messages in thread From: Kevin Hilman @ 2014-06-20 21:34 UTC (permalink / raw) To: Alan Stern Cc: Rafael J. Wysocki, Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel Alan Stern <stern@rowland.harvard.edu> writes: > On Fri, 20 Jun 2014, Rafael J. Wysocki wrote: > >> > For a general device, the fact that dev->power.is_suspended is set >> > means the device _has_ been powered down. Even though the >> > runtime_status may not have changed, the PM core has to assume the >> > device is not available for use. >> >> This seems to go a bit too far. What power.is_suspended actually means is >> that __device_suspend() has run for the device successfully. What the >> implications of that are depends on the bus type (or subsystem in general) >> and device driver. >> >> > While your I2C devices may be useable even after the ->suspend callback >> > returns, for most devices this isn't true. So we shouldn't allow >> > rpm_resume() to return imediately when is_suspended is set. >> >> I can agree with that. > > We really do need to decide more precisely how runtime PM and system PM > will interact. Yes! > Should ->runtime_resume callbacks be allowed after ->suspend has > returned? Abolutely. > Kevin has stated that some devices do need this ability. But most > don't. Does it matter if most don't? As long a some do, we need to support this. It may not be "most" devices, but on the (mostly embedded) SoCs I work on, the devices that do need this tend to be rather crucial core devices that are used during the PM of other devices (e.g. I2C, SPI, GPIOs, etc. etc.) > The PM core needs to handle these conflicting requirements > somehow. I agree. We've gone back and forth a few times on the various interactions between system PM and runtime PM over the years but it seems there are still things to clarify. Kevin ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. @ 2014-06-20 21:34 ` Kevin Hilman 0 siblings, 0 replies; 65+ messages in thread From: Kevin Hilman @ 2014-06-20 21:34 UTC (permalink / raw) To: Alan Stern Cc: Rafael J. Wysocki, Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel Alan Stern <stern@rowland.harvard.edu> writes: > On Fri, 20 Jun 2014, Rafael J. Wysocki wrote: > >> > For a general device, the fact that dev->power.is_suspended is set >> > means the device _has_ been powered down. Even though the >> > runtime_status may not have changed, the PM core has to assume the >> > device is not available for use. >> >> This seems to go a bit too far. What power.is_suspended actually means is >> that __device_suspend() has run for the device successfully. What the >> implications of that are depends on the bus type (or subsystem in general) >> and device driver. >> >> > While your I2C devices may be useable even after the ->suspend callback >> > returns, for most devices this isn't true. So we shouldn't allow >> > rpm_resume() to return imediately when is_suspended is set. >> >> I can agree with that. > > We really do need to decide more precisely how runtime PM and system PM > will interact. Yes! > Should ->runtime_resume callbacks be allowed after ->suspend has > returned? Abolutely. > Kevin has stated that some devices do need this ability. But most > don't. Does it matter if most don't? As long a some do, we need to support this. It may not be "most" devices, but on the (mostly embedded) SoCs I work on, the devices that do need this tend to be rather crucial core devices that are used during the PM of other devices (e.g. I2C, SPI, GPIOs, etc. etc.) > The PM core needs to handle these conflicting requirements > somehow. I agree. We've gone back and forth a few times on the various interactions between system PM and runtime PM over the years but it seems there are still things to clarify. Kevin ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-20 21:34 ` Kevin Hilman (?) @ 2014-06-22 13:40 ` Rafael J. Wysocki -1 siblings, 0 replies; 65+ messages in thread From: Rafael J. Wysocki @ 2014-06-22 13:40 UTC (permalink / raw) To: Kevin Hilman Cc: Alan Stern, Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Friday, June 20, 2014 02:34:14 PM Kevin Hilman wrote: > Alan Stern <stern@rowland.harvard.edu> writes: > > > On Fri, 20 Jun 2014, Rafael J. Wysocki wrote: > > > >> > For a general device, the fact that dev->power.is_suspended is set > >> > means the device _has_ been powered down. Even though the > >> > runtime_status may not have changed, the PM core has to assume the > >> > device is not available for use. > >> > >> This seems to go a bit too far. What power.is_suspended actually means is > >> that __device_suspend() has run for the device successfully. What the > >> implications of that are depends on the bus type (or subsystem in general) > >> and device driver. > >> > >> > While your I2C devices may be useable even after the ->suspend callback > >> > returns, for most devices this isn't true. So we shouldn't allow > >> > rpm_resume() to return imediately when is_suspended is set. > >> > >> I can agree with that. > > > > We really do need to decide more precisely how runtime PM and system PM > > will interact. > > Yes! > > > Should ->runtime_resume callbacks be allowed after ->suspend has > > returned? > > Abolutely. > > > Kevin has stated that some devices do need this ability. But most > > don't. > > Does it matter if most don't? As long a some do, we need to support > this. It may not be "most" devices, but on the (mostly embedded) SoCs I > work on, the devices that do need this tend to be rather crucial core > devices that are used during the PM of other devices (e.g. I2C, SPI, > GPIOs, etc. etc.) > > > The PM core needs to handle these conflicting requirements > > somehow. > > I agree. We've gone back and forth a few times on the various > interactions between system PM and runtime PM over the years but it > seems there are still things to clarify. Well, we only considered a specific use case every time without looking at the big picture, mostly because we didn't really know what the big picture was. I guess today we have enough experience to try to address all of these problems together. I guess we need to start with making a list of different types of bus type/driver behavior existing today and how the core is supposed to interact with them. Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-20 14:48 ` Alan Stern 2014-06-20 21:34 ` Kevin Hilman @ 2014-06-22 13:24 ` Rafael J. Wysocki 1 sibling, 0 replies; 65+ messages in thread From: Rafael J. Wysocki @ 2014-06-22 13:24 UTC (permalink / raw) To: Alan Stern Cc: Kevin Hilman, Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Friday, June 20, 2014 10:48:09 AM Alan Stern wrote: > On Fri, 20 Jun 2014, Rafael J. Wysocki wrote: > > > > For a general device, the fact that dev->power.is_suspended is set > > > means the device _has_ been powered down. Even though the > > > runtime_status may not have changed, the PM core has to assume the > > > device is not available for use. > > > > This seems to go a bit too far. What power.is_suspended actually means is > > that __device_suspend() has run for the device successfully. What the > > implications of that are depends on the bus type (or subsystem in general) > > and device driver. > > > > > While your I2C devices may be useable even after the ->suspend callback > > > returns, for most devices this isn't true. So we shouldn't allow > > > rpm_resume() to return imediately when is_suspended is set. > > > > I can agree with that. > > We really do need to decide more precisely how runtime PM and system PM > will interact. Should ->runtime_resume callbacks be allowed after > ->suspend has returned? > > Kevin has stated that some devices do need this ability. But most > don't. The PM core needs to handle these conflicting requirements > somehow. I agree. I guess we'll have to introduce a separate opt-in flag for drivers with this specific need. At least I don't see any other way to take that into account. > Note: this is a separate issue from the meaning of disabled_depth > 0. Yes, it is. Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-19 20:13 ` Alan Stern @ 2014-06-20 21:31 ` Kevin Hilman 2014-06-20 21:31 ` Kevin Hilman 1 sibling, 0 replies; 65+ messages in thread From: Kevin Hilman @ 2014-06-20 21:31 UTC (permalink / raw) To: Alan Stern Cc: Allen Yu, Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel Alan Stern <stern@rowland.harvard.edu> writes: > On Thu, 19 Jun 2014, Kevin Hilman wrote: > >> Alan Stern <stern@rowland.harvard.edu> writes: >> >> > On Thu, 19 Jun 2014, Allen Yu wrote: >> > >> >> So what's the exact state of device if dev->power.is_suspended flag >> >> is set and runtime_status is RPM_ACTIVE? Is it a state like >> >> "suspended but still can be accessed"? >> >> >> >> I'm just afraid the existing code would cause a device hang if we >> >> allow it to be accessed even though it's suspended (at this point >> >> RPM_ACTIVE could be meaningless). I don't understand the original >> >> motivation of these code. If it's a valid case, most likely it should >> >> be handled in the specific device driver instead of the PM core. >> > >> > You should read the Changelog for commit 6f3c77b040f (PM / Runtime: >> > let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2). It >> > explains why the code looks the way it does. >> > >> > However, I'm starting to think the reasoning in that commit may not be >> > valid. While perhaps it is okay for some I2C devices (mentioned in the >> > commit log), it probably isn't okay in general. >> >> Why not? > > See below. > >> > Kevin, do have any comments on this matter? What do you think about >> > making the following change to rpm_resume(): >> > >> > repeat: >> > if (dev->power.runtime_error) >> > retval = -EINVAL; >> > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended >> > + else if (dev->power.disable_depth > 0 >> > && dev->power.runtime_status == RPM_ACTIVE) >> > retval = 1; >> > else if (dev->power.disable_depth > 0) >> > >> > Or even: >> > >> > + else if (dev->power.disable_depth > 0 && !dev->power.is_suspended >> > >> > although this would require the I2C driver you mentioned in your commit >> > to change. >> >> My change was introduced to catch a very specific case. Namely, when we >> know that the core has successfully asked the device to do a system suspend >> (dev->power.is_suspended == true) *and* we know that runtime PM was >> disabled *only* by the PM core (disable_depth == 0) while the device was >> still active (runtime_status == RPM_ACTIVE.) > > For a general device, the fact that dev->power.is_suspended is set > means the device _has_ been powered down. Even though the > runtime_status may not have changed, the PM core has to assume the > device is not available for use. This is where things get fuzzy because of the overlap between system PM and runtime PM. It makes sense that from a system PM perspecitve, the core has to assume the device isn't available. But from a runtime PM perspective, we know that it is, so we allow the *runtime PM* requests to succeed. > While your I2C devices may be useable even after the ->suspend callback > returns, for most devices this isn't true. So we shouldn't allow > rpm_resume() to return imediately when is_suspended is set. It's not just my I2C devices, those are just a convenient example. It's true for any device that does a pm_runtime_get*() during its system suspend/resume path. >> In your first idea above, it would allow a _get() to succeed even if >> someone other than the core (including the driver itself) had called >> pm_runtime_disable(). I don't think we want that. > > Why not? The fact that the device is disabled for runtime PM means > that the PM core mustn't try to change its power state. But if the > runtime status is RPM_ACTIVE then the device should already be powered > up, so there's no harm in letting runtime_resume() succeed. Well, if we want pm_runtime_disable() to mean "disable only if !RPM_ACTIVE", I guess that's another question to be debated. However, in my original patch I didn't want to make that generic change, I only was after the very specific case when we know it was only the core which disabled runtime PM. > To put it another way, disabled_depth > 0 means that the PM core isn't > allowed to invoke any of the runtime PM callbacks. But when > runtime_status == RPM_ACTIVE, runtime_resume() can run successfully > without invoking any callbacks. I'd be OK with that more generic change, but I suspect there may be some drivers out there that may be relying on the -EACCESS. >> In the second idea above, we'd completely miss the case where runtime PM >> has been disabled by the core (because the core would have set >> dev->power.is_suspended) > > That's the intention. When is_suspended is set, the PM core assumes > that the device has been powered down in preparation for system > suspend. We don't want to mess that up by performing a runtime resume. This is where I disagree. Some devices really need to be runtime resumed during the suspend/resume process. >> In both cases, we're no longer just checking for that specific condition >> I was after, so I'd have to spend more time thinking about any other >> possible consequences as well. > > Indeed. Hopefully the fallout won't affect more than a few drivers. The fallout for the first one would be minor I suspect. But the second one is a pretty major change that I don't agree with. >> I think part of the confusion here is coming from what >> dev->power.is_suspended means. From the core's perspective, it just >> means that the core has called the ->suspend callback, and didn't detect >> any errors. > > Yes. But the core also has to assume that the ->runtime_resume > callback will undo the effect of ->suspend. Therefore the core should > not call ->runtime_resume if is_suspended is set. I agree with Rafael that it should be up to the bus/subsystem to allow/deny that. >> Depending on the driver though, it doesn't have to mean that the device >> is actually fully suspended. For example, there are several cases of >> "runtime PM centric" drivers are may be needed by other devices during >> the system suspend/resume process and so are runtime PM resumed during >> system suspend. > > At what stage do these devices get powered down? During suspend_late > or suspend_irq? Correct. > At such times the PM core won't invoke the runtime PM callbacks > anyway. The core wont, but the bus/subsystem/pm_domain can. Also, recently the pm_runtime_force* functions were added so that a bus/subsystem could do this easily. > It sounds like what you really want for these devices is to have > dev->power.is_suspended get set at the start of > __device_suspend_late() rather than at the end of __device_suspend(). > Or maybe even not to get set at all. Well, from my PoV, power.is_suspended doesn't have any meaning for runtime PM. It's only for system suspend. Kevin ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. @ 2014-06-20 21:31 ` Kevin Hilman 0 siblings, 0 replies; 65+ messages in thread From: Kevin Hilman @ 2014-06-20 21:31 UTC (permalink / raw) To: Alan Stern Cc: Allen Yu, Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel Alan Stern <stern@rowland.harvard.edu> writes: > On Thu, 19 Jun 2014, Kevin Hilman wrote: > >> Alan Stern <stern@rowland.harvard.edu> writes: >> >> > On Thu, 19 Jun 2014, Allen Yu wrote: >> > >> >> So what's the exact state of device if dev->power.is_suspended flag >> >> is set and runtime_status is RPM_ACTIVE? Is it a state like >> >> "suspended but still can be accessed"? >> >> >> >> I'm just afraid the existing code would cause a device hang if we >> >> allow it to be accessed even though it's suspended (at this point >> >> RPM_ACTIVE could be meaningless). I don't understand the original >> >> motivation of these code. If it's a valid case, most likely it should >> >> be handled in the specific device driver instead of the PM core. >> > >> > You should read the Changelog for commit 6f3c77b040f (PM / Runtime: >> > let rpm_resume() succeed if RPM_ACTIVE, even when disabled, v2). It >> > explains why the code looks the way it does. >> > >> > However, I'm starting to think the reasoning in that commit may not be >> > valid. While perhaps it is okay for some I2C devices (mentioned in the >> > commit log), it probably isn't okay in general. >> >> Why not? > > See below. > >> > Kevin, do have any comments on this matter? What do you think about >> > making the following change to rpm_resume(): >> > >> > repeat: >> > if (dev->power.runtime_error) >> > retval = -EINVAL; >> > - else if (dev->power.disable_depth == 1 && dev->power.is_suspended >> > + else if (dev->power.disable_depth > 0 >> > && dev->power.runtime_status == RPM_ACTIVE) >> > retval = 1; >> > else if (dev->power.disable_depth > 0) >> > >> > Or even: >> > >> > + else if (dev->power.disable_depth > 0 && !dev->power.is_suspended >> > >> > although this would require the I2C driver you mentioned in your commit >> > to change. >> >> My change was introduced to catch a very specific case. Namely, when we >> know that the core has successfully asked the device to do a system suspend >> (dev->power.is_suspended == true) *and* we know that runtime PM was >> disabled *only* by the PM core (disable_depth == 0) while the device was >> still active (runtime_status == RPM_ACTIVE.) > > For a general device, the fact that dev->power.is_suspended is set > means the device _has_ been powered down. Even though the > runtime_status may not have changed, the PM core has to assume the > device is not available for use. This is where things get fuzzy because of the overlap between system PM and runtime PM. It makes sense that from a system PM perspecitve, the core has to assume the device isn't available. But from a runtime PM perspective, we know that it is, so we allow the *runtime PM* requests to succeed. > While your I2C devices may be useable even after the ->suspend callback > returns, for most devices this isn't true. So we shouldn't allow > rpm_resume() to return imediately when is_suspended is set. It's not just my I2C devices, those are just a convenient example. It's true for any device that does a pm_runtime_get*() during its system suspend/resume path. >> In your first idea above, it would allow a _get() to succeed even if >> someone other than the core (including the driver itself) had called >> pm_runtime_disable(). I don't think we want that. > > Why not? The fact that the device is disabled for runtime PM means > that the PM core mustn't try to change its power state. But if the > runtime status is RPM_ACTIVE then the device should already be powered > up, so there's no harm in letting runtime_resume() succeed. Well, if we want pm_runtime_disable() to mean "disable only if !RPM_ACTIVE", I guess that's another question to be debated. However, in my original patch I didn't want to make that generic change, I only was after the very specific case when we know it was only the core which disabled runtime PM. > To put it another way, disabled_depth > 0 means that the PM core isn't > allowed to invoke any of the runtime PM callbacks. But when > runtime_status == RPM_ACTIVE, runtime_resume() can run successfully > without invoking any callbacks. I'd be OK with that more generic change, but I suspect there may be some drivers out there that may be relying on the -EACCESS. >> In the second idea above, we'd completely miss the case where runtime PM >> has been disabled by the core (because the core would have set >> dev->power.is_suspended) > > That's the intention. When is_suspended is set, the PM core assumes > that the device has been powered down in preparation for system > suspend. We don't want to mess that up by performing a runtime resume. This is where I disagree. Some devices really need to be runtime resumed during the suspend/resume process. >> In both cases, we're no longer just checking for that specific condition >> I was after, so I'd have to spend more time thinking about any other >> possible consequences as well. > > Indeed. Hopefully the fallout won't affect more than a few drivers. The fallout for the first one would be minor I suspect. But the second one is a pretty major change that I don't agree with. >> I think part of the confusion here is coming from what >> dev->power.is_suspended means. From the core's perspective, it just >> means that the core has called the ->suspend callback, and didn't detect >> any errors. > > Yes. But the core also has to assume that the ->runtime_resume > callback will undo the effect of ->suspend. Therefore the core should > not call ->runtime_resume if is_suspended is set. I agree with Rafael that it should be up to the bus/subsystem to allow/deny that. >> Depending on the driver though, it doesn't have to mean that the device >> is actually fully suspended. For example, there are several cases of >> "runtime PM centric" drivers are may be needed by other devices during >> the system suspend/resume process and so are runtime PM resumed during >> system suspend. > > At what stage do these devices get powered down? During suspend_late > or suspend_irq? Correct. > At such times the PM core won't invoke the runtime PM callbacks > anyway. The core wont, but the bus/subsystem/pm_domain can. Also, recently the pm_runtime_force* functions were added so that a bus/subsystem could do this easily. > It sounds like what you really want for these devices is to have > dev->power.is_suspended get set at the start of > __device_suspend_late() rather than at the end of __device_suspend(). > Or maybe even not to get set at all. Well, from my PoV, power.is_suspended doesn't have any meaning for runtime PM. It's only for system suspend. Kevin ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-20 21:31 ` Kevin Hilman (?) @ 2014-06-21 13:34 ` Alan Stern 2014-06-22 13:35 ` Rafael J. Wysocki 2014-06-23 18:57 ` Kevin Hilman -1 siblings, 2 replies; 65+ messages in thread From: Alan Stern @ 2014-06-21 13:34 UTC (permalink / raw) To: Kevin Hilman Cc: Allen Yu, Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Fri, 20 Jun 2014, Kevin Hilman wrote: > > For a general device, the fact that dev->power.is_suspended is set > > means the device _has_ been powered down. Even though the > > runtime_status may not have changed, the PM core has to assume the > > device is not available for use. > > This is where things get fuzzy because of the overlap between system PM > and runtime PM. It makes sense that from a system PM perspecitve, the > core has to assume the device isn't available. But from a runtime PM > perspective, we know that it is, so we allow the *runtime PM* requests > to succeed. Well, to be fair, from a runtime PM perspective the core _doesn't_ know that the device is available. For example, if we were talking about a USB device rather than an I2C device, it _wouldn't_ be available. The fact is, after ->suspend returns some devices are still available and some aren't. Currently the PM core doesn't know which are which. > > While your I2C devices may be useable even after the ->suspend callback > > returns, for most devices this isn't true. So we shouldn't allow > > rpm_resume() to return imediately when is_suspended is set. > > It's not just my I2C devices, those are just a convenient example. It's > true for any device that does a pm_runtime_get*() during its system > suspend/resume path. Yes. But what if a pm_runtime_get*() happens duing system suspend but not on the suspend path? For example, what if a workqueue routine happens to run at that time and completely independently calls pm_runtime_get_sync()? The PM core needs to know whether that call should be allowed to succeed. > > That's the intention. When is_suspended is set, the PM core assumes > > that the device has been powered down in preparation for system > > suspend. We don't want to mess that up by performing a runtime resume. > > This is where I disagree. Some devices really need to be runtime > resumed during the suspend/resume process. Okay, I believe you. But do they need to be runtime resumed by a call to pm_runtime_get_*, or could pm_runtime_force_resume() be used instead? > > Yes. But the core also has to assume that the ->runtime_resume > > callback will undo the effect of ->suspend. Therefore the core should > > not call ->runtime_resume if is_suspended is set. > > I agree with Rafael that it should be up to the bus/subsystem to > allow/deny that. How? Wouldn't that mean changing every subsystem that wants to prevent the callbacks? > > At what stage do these devices get powered down? During suspend_late > > or suspend_irq? > > Correct. > > > At such times the PM core won't invoke the runtime PM callbacks > > anyway. > > The core wont, but the bus/subsystem/pm_domain can. Also, recently the > pm_runtime_force* functions were added so that a bus/subsystem could do > this easily. That's okay; if a subsystem calls one of those functions then we know it intends to work around the usual protections. > > It sounds like what you really want for these devices is to have > > dev->power.is_suspended get set at the start of > > __device_suspend_late() rather than at the end of __device_suspend(). > > Or maybe even not to get set at all. > > Well, from my PoV, power.is_suspended doesn't have any meaning for > runtime PM. It's only for system suspend. If it doesn't have any meaning for runtime PM, it shouldn't be tested in runtime.c. But never mind that; the is_suspended flag is a red herring. It doesn't have clearly defined semantics. What we really need to figure out is how to tell the PM core which devices may safely have their runtime callbacks invoked during system suspend. For those devices, the core can avoid calling pm_runtime_disable() during the suspend_late phase. That would address your requirements, right? Here's another way to think about it. At some point during system suspend, a device needs to powered down for the last time. Many subsystems/drivers do this in their ->suspend callback. Some during ->suspend_late or ->suspend_irq. Some may never do it explicitly. Regardless, the PM core needs to know when it has happened, so that it can block ->runtime_resume callbacks from that point onward. Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-21 13:34 ` Alan Stern @ 2014-06-22 13:35 ` Rafael J. Wysocki 2014-06-23 18:57 ` Kevin Hilman 1 sibling, 0 replies; 65+ messages in thread From: Rafael J. Wysocki @ 2014-06-22 13:35 UTC (permalink / raw) To: Alan Stern Cc: Kevin Hilman, Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Saturday, June 21, 2014 09:34:28 AM Alan Stern wrote: > On Fri, 20 Jun 2014, Kevin Hilman wrote: > > > > For a general device, the fact that dev->power.is_suspended is set > > > means the device _has_ been powered down. Even though the > > > runtime_status may not have changed, the PM core has to assume the > > > device is not available for use. > > > > This is where things get fuzzy because of the overlap between system PM > > and runtime PM. It makes sense that from a system PM perspecitve, the > > core has to assume the device isn't available. But from a runtime PM > > perspective, we know that it is, so we allow the *runtime PM* requests > > to succeed. > > Well, to be fair, from a runtime PM perspective the core _doesn't_ > know that the device is available. For example, if we were talking > about a USB device rather than an I2C device, it _wouldn't_ be > available. > > The fact is, after ->suspend returns some devices are still available > and some aren't. Currently the PM core doesn't know which are which. That's correct. As a result of this, the core should not make any assumptions about the device's physical state after ->suspend() has been executed. Well, the core shouldn't actually make any assumptions about the device's physical state at any given time at all, because it never knows what that state is. What it can do (and what the runtime PM rules are for) is to refuse to carry out operations that generally don't make sense, like calling ->runtime_resume() twice in a row, for example. Currently, we have separate rules for runtime PM and for system suspend/resume, but it looks like we really need to establish a set of rules that will cover *both* runtime PM and system suspend/resume at the same time. Of course, it needs to be compatible with the existing rules as far as reasonably possible. Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-21 13:34 ` Alan Stern @ 2014-06-23 18:57 ` Kevin Hilman 2014-06-23 18:57 ` Kevin Hilman 1 sibling, 0 replies; 65+ messages in thread From: Kevin Hilman @ 2014-06-23 18:57 UTC (permalink / raw) To: Alan Stern Cc: Allen Yu, Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel Alan Stern <stern@rowland.harvard.edu> writes: [...] > What we really need to figure out is how to tell the PM core which > devices may safely have their runtime callbacks invoked during system > suspend. For those devices, the core can avoid calling > pm_runtime_disable() during the suspend_late phase. That would address > your requirements, right? Yes, and something I've attempted a few times over the years, most recently during the introduction of the pm_runtime_force* functions[1], which I thought was again an attempt to work around this issue. I don't think Rafael has ever been too thrilled with that idea (including the last time[2]), but I think we're to a point now that we have to manage this somehow. My attempt to let the bus/subsystem/pm_domain set a flag might be too simplistic, but I do agree we do need som way to tell the PM core that runtime PM callbacks are (still) safe. Kevin [1] http://marc.info/?l=linux-pm&m=139343222014989&w=2 [2] http://marc.info/?l=linux-pm&m=139346327619875&w=2 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. @ 2014-06-23 18:57 ` Kevin Hilman 0 siblings, 0 replies; 65+ messages in thread From: Kevin Hilman @ 2014-06-23 18:57 UTC (permalink / raw) To: Alan Stern Cc: Allen Yu, Rafael J. Wysocki, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel Alan Stern <stern@rowland.harvard.edu> writes: [...] > What we really need to figure out is how to tell the PM core which > devices may safely have their runtime callbacks invoked during system > suspend. For those devices, the core can avoid calling > pm_runtime_disable() during the suspend_late phase. That would address > your requirements, right? Yes, and something I've attempted a few times over the years, most recently during the introduction of the pm_runtime_force* functions[1], which I thought was again an attempt to work around this issue. I don't think Rafael has ever been too thrilled with that idea (including the last time[2]), but I think we're to a point now that we have to manage this somehow. My attempt to let the bus/subsystem/pm_domain set a flag might be too simplistic, but I do agree we do need som way to tell the PM core that runtime PM callbacks are (still) safe. Kevin [1] http://marc.info/?l=linux-pm&m=139343222014989&w=2 [2] http://marc.info/?l=linux-pm&m=139346327619875&w=2 ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-18 23:57 ` Rafael J. Wysocki @ 2014-06-19 14:34 ` Alan Stern 2014-06-19 14:34 ` Alan Stern 1 sibling, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-06-19 14:34 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Thu, 19 Jun 2014, Rafael J. Wysocki wrote: > Well, we used to have the notion that runtime_status is not meaningful for > devices with dev->power.disable_depth greater than 0 (except for the special > case in the suspend code path where we know why it is greater than 0). I think > it was useful. :-) Did we really have that notion? My memory is a little cloudy, but I thought we decided that runtime_status would not be meaningful when dev->power.runtime_error was set -- not when dev->power.disable_depth was greater than 0. Am I mixed up? In any case, I think it is reasonable to regard runtime_status as meaningful when disable_depth > 0. The PM core isn't allowed to invoke the runtime callbacks at such times, that's all. This makes perfect sense for a device that doesn't support power management and hence must always be at full power. Or when a driver knows that runtime_status is out of agreement with the device's actual power state and wants to update runtime_status directly. > > So pm_runtime_resume() and pm_request_resume() would still fail, but > > pm_runtime_get() and pm_runtime_get_sync() would work? I'm not sure > > about the reason for this distinction. > > The meaning of pm_runtime_get()/pm_runtime_get_sync() is "prevent the > device from being suspended from now on and resume it if necessary" while > "runtime PM disabled and runtime_status == RPM_ACTIVE" may be interpreted > as "not necessary to resume", so it is reasonable to special case this > particular situation for these particular routines IMHO. By the same reasoning, the meaning of pm_runtime_resume() is "resume the device now if necsesary". Since "runtime PM disabled and runtime_status == RPM_ACTIVE" means "not necessary to resume", isn't it logical for pm_runtime_resume() also to succeed under that condition? Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. @ 2014-06-19 14:34 ` Alan Stern 0 siblings, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-06-19 14:34 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Thu, 19 Jun 2014, Rafael J. Wysocki wrote: > Well, we used to have the notion that runtime_status is not meaningful for > devices with dev->power.disable_depth greater than 0 (except for the special > case in the suspend code path where we know why it is greater than 0). I think > it was useful. :-) Did we really have that notion? My memory is a little cloudy, but I thought we decided that runtime_status would not be meaningful when dev->power.runtime_error was set -- not when dev->power.disable_depth was greater than 0. Am I mixed up? In any case, I think it is reasonable to regard runtime_status as meaningful when disable_depth > 0. The PM core isn't allowed to invoke the runtime callbacks at such times, that's all. This makes perfect sense for a device that doesn't support power management and hence must always be at full power. Or when a driver knows that runtime_status is out of agreement with the device's actual power state and wants to update runtime_status directly. > > So pm_runtime_resume() and pm_request_resume() would still fail, but > > pm_runtime_get() and pm_runtime_get_sync() would work? I'm not sure > > about the reason for this distinction. > > The meaning of pm_runtime_get()/pm_runtime_get_sync() is "prevent the > device from being suspended from now on and resume it if necessary" while > "runtime PM disabled and runtime_status == RPM_ACTIVE" may be interpreted > as "not necessary to resume", so it is reasonable to special case this > particular situation for these particular routines IMHO. By the same reasoning, the meaning of pm_runtime_resume() is "resume the device now if necsesary". Since "runtime PM disabled and runtime_status == RPM_ACTIVE" means "not necessary to resume", isn't it logical for pm_runtime_resume() also to succeed under that condition? Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-19 14:34 ` Alan Stern (?) @ 2014-06-20 13:33 ` Rafael J. Wysocki 2014-06-20 14:43 ` Alan Stern -1 siblings, 1 reply; 65+ messages in thread From: Rafael J. Wysocki @ 2014-06-20 13:33 UTC (permalink / raw) To: Alan Stern Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Thursday, June 19, 2014 10:34:01 AM Alan Stern wrote: > On Thu, 19 Jun 2014, Rafael J. Wysocki wrote: > > > Well, we used to have the notion that runtime_status is not meaningful for > > devices with dev->power.disable_depth greater than 0 (except for the special > > case in the suspend code path where we know why it is greater than 0). I think > > it was useful. :-) > > Did we really have that notion? My memory is a little cloudy, but I > thought we decided that runtime_status would not be meaningful when > dev->power.runtime_error was set -- not when dev->power.disable_depth > was greater than 0. Am I mixed up? Well, we clearly did, because we added things like pm_runtime_set_active() pm_runtime_set_suspended() which allow the status to be changed without doing anything to the device while power.disable_depth is greater than 0. > In any case, I think it is reasonable to regard runtime_status as > meaningful when disable_depth > 0. So we need to remove the above helpers and modify all code using them. > The PM core isn't allowed to invoke the runtime callbacks at such times, > that's all. This makes perfect sense for a device that doesn't support > power management and hence must always be at full power. Or when a driver > knows that runtime_status is out of agreement with the device's actual > power state and wants to update runtime_status directly. That's what it is supposed to use the above helpers for, isn't it? Devices that don't support power management, but that we want to use drivers supporting power management with are clearly a special case, so perhaps we should treat them as such instead of trying to modity the core to silently cover this case too automatically? > > > So pm_runtime_resume() and pm_request_resume() would still fail, but > > > pm_runtime_get() and pm_runtime_get_sync() would work? I'm not sure > > > about the reason for this distinction. > > > > The meaning of pm_runtime_get()/pm_runtime_get_sync() is "prevent the > > device from being suspended from now on and resume it if necessary" while > > "runtime PM disabled and runtime_status == RPM_ACTIVE" may be interpreted > > as "not necessary to resume", so it is reasonable to special case this > > particular situation for these particular routines IMHO. > > By the same reasoning, the meaning of pm_runtime_resume() is "resume > the device now if necsesary". Since "runtime PM disabled and > runtime_status == RPM_ACTIVE" means "not necessary to resume", isn't it > logical for pm_runtime_resume() also to succeed under that condition? My point really is that pm_runtime_get()/pm_runtime_get_sync() actually carry out two operations, (1) incrementing the usage counter and (2) resuming the device if need be. It is not particularly clear if/when the result of (2) should determine the return value of (1) and (2) together, so there is some freedom in that area which we can use to cover special cases. That's all. I'm perfectly fine with leaving the code as is, though. You wanted to change it. :-) Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-20 13:33 ` Rafael J. Wysocki @ 2014-06-20 14:43 ` Alan Stern 0 siblings, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-06-20 14:43 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Fri, 20 Jun 2014, Rafael J. Wysocki wrote: > On Thursday, June 19, 2014 10:34:01 AM Alan Stern wrote: > > On Thu, 19 Jun 2014, Rafael J. Wysocki wrote: > > > > > Well, we used to have the notion that runtime_status is not meaningful for > > > devices with dev->power.disable_depth greater than 0 (except for the special > > > case in the suspend code path where we know why it is greater than 0). I think > > > it was useful. :-) > > > > Did we really have that notion? My memory is a little cloudy, but I > > thought we decided that runtime_status would not be meaningful when > > dev->power.runtime_error was set -- not when dev->power.disable_depth > > was greater than 0. Am I mixed up? > > Well, we clearly did, because we added things like > > pm_runtime_set_active() > pm_runtime_set_suspended() > > which allow the status to be changed without doing anything to the device > while power.disable_depth is greater than 0. Yes. I'm not sure that was thought out as carefully as it could have been. Even with that restriction, it's not possible to guarantee that drivers won't make mistakes. For example, suppose runtime_status is RPM_ACTIVE but the device really is powered down. Before pm_runtime_disable() is called, pm_runtime_resume() will succeed. The driver will think it can use the device, even though it can't. This demonstrates that changing the runtime status can't be done properly unless the subsystem and driver take some of the responsibility. Since that is true, perhaps we ought to allow drivers to call pm_runtime_set_active/suspended() whenever they want, and rely on them not to mess things up. As far as I know, there are only two major use cases for _set_active/_set_suspended: when the device is being initialized during probe, and during system resume. During probe it's pretty safe to assume there won't be any runtime PM calls, and much the same is true during system resume (especially during the resume_irq and resume_early phases). Therefore relying on drivers shouldn't make their jobs any harder. > > In any case, I think it is reasonable to regard runtime_status as > > meaningful when disable_depth > 0. > > So we need to remove the above helpers and modify all code using them. We don't have to remove the helpers. And practically none of the code using them would need to be modified. A certain amount of auditing would be a good idea, though. > > The PM core isn't allowed to invoke the runtime callbacks at such times, > > that's all. This makes perfect sense for a device that doesn't support > > power management and hence must always be at full power. Or when a driver > > knows that runtime_status is out of agreement with the device's actual > > power state and wants to update runtime_status directly. > > That's what it is supposed to use the above helpers for, isn't it? > > Devices that don't support power management, but that we want to use > drivers supporting power management with are clearly a special case, so Are they? I'm not so sure. But never mind... > perhaps we should treat them as such instead of trying to modity the core > to silently cover this case too automatically? How would you treat them specially? Add a "runtime_pm_not_supported" flag? > > > > So pm_runtime_resume() and pm_request_resume() would still fail, but > > > > pm_runtime_get() and pm_runtime_get_sync() would work? I'm not sure > > > > about the reason for this distinction. > > > > > > The meaning of pm_runtime_get()/pm_runtime_get_sync() is "prevent the > > > device from being suspended from now on and resume it if necessary" while > > > "runtime PM disabled and runtime_status == RPM_ACTIVE" may be interpreted > > > as "not necessary to resume", so it is reasonable to special case this > > > particular situation for these particular routines IMHO. > > > > By the same reasoning, the meaning of pm_runtime_resume() is "resume > > the device now if necsesary". Since "runtime PM disabled and > > runtime_status == RPM_ACTIVE" means "not necessary to resume", isn't it > > logical for pm_runtime_resume() also to succeed under that condition? > > My point really is that pm_runtime_get()/pm_runtime_get_sync() actually carry > out two operations, (1) incrementing the usage counter and (2) resuming the > device if need be. It is not particularly clear if/when the result of (2) > should determine the return value of (1) and (2) together, so there is some > freedom in that area which we can use to cover special cases. That's all. Oh. Well, currently those routines are documented as returning the value from their internal pm_request_resume()/pm_runtime_resume() call. > I'm perfectly fine with leaving the code as is, though. You wanted to change it. :-) I'd be equally happy with a "runtime_pm_not_supported" flag, or something equivalent. Implementing it wouldn't be hard: Setting the flag could call pm_runtime_forbid() and make the power/control sysfs attribute return -EPERM for any attempted write. Perhaps also make power/runtime_status show "unsupported" rather than "on". (Hmmm, now that I look at rtpm_status_show(), I see that it already shows "unsupported" whenever disable_depth > 0. Another indication of how confused the situation is?) Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. @ 2014-06-20 14:43 ` Alan Stern 0 siblings, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-06-20 14:43 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Fri, 20 Jun 2014, Rafael J. Wysocki wrote: > On Thursday, June 19, 2014 10:34:01 AM Alan Stern wrote: > > On Thu, 19 Jun 2014, Rafael J. Wysocki wrote: > > > > > Well, we used to have the notion that runtime_status is not meaningful for > > > devices with dev->power.disable_depth greater than 0 (except for the special > > > case in the suspend code path where we know why it is greater than 0). I think > > > it was useful. :-) > > > > Did we really have that notion? My memory is a little cloudy, but I > > thought we decided that runtime_status would not be meaningful when > > dev->power.runtime_error was set -- not when dev->power.disable_depth > > was greater than 0. Am I mixed up? > > Well, we clearly did, because we added things like > > pm_runtime_set_active() > pm_runtime_set_suspended() > > which allow the status to be changed without doing anything to the device > while power.disable_depth is greater than 0. Yes. I'm not sure that was thought out as carefully as it could have been. Even with that restriction, it's not possible to guarantee that drivers won't make mistakes. For example, suppose runtime_status is RPM_ACTIVE but the device really is powered down. Before pm_runtime_disable() is called, pm_runtime_resume() will succeed. The driver will think it can use the device, even though it can't. This demonstrates that changing the runtime status can't be done properly unless the subsystem and driver take some of the responsibility. Since that is true, perhaps we ought to allow drivers to call pm_runtime_set_active/suspended() whenever they want, and rely on them not to mess things up. As far as I know, there are only two major use cases for _set_active/_set_suspended: when the device is being initialized during probe, and during system resume. During probe it's pretty safe to assume there won't be any runtime PM calls, and much the same is true during system resume (especially during the resume_irq and resume_early phases). Therefore relying on drivers shouldn't make their jobs any harder. > > In any case, I think it is reasonable to regard runtime_status as > > meaningful when disable_depth > 0. > > So we need to remove the above helpers and modify all code using them. We don't have to remove the helpers. And practically none of the code using them would need to be modified. A certain amount of auditing would be a good idea, though. > > The PM core isn't allowed to invoke the runtime callbacks at such times, > > that's all. This makes perfect sense for a device that doesn't support > > power management and hence must always be at full power. Or when a driver > > knows that runtime_status is out of agreement with the device's actual > > power state and wants to update runtime_status directly. > > That's what it is supposed to use the above helpers for, isn't it? > > Devices that don't support power management, but that we want to use > drivers supporting power management with are clearly a special case, so Are they? I'm not so sure. But never mind... > perhaps we should treat them as such instead of trying to modity the core > to silently cover this case too automatically? How would you treat them specially? Add a "runtime_pm_not_supported" flag? > > > > So pm_runtime_resume() and pm_request_resume() would still fail, but > > > > pm_runtime_get() and pm_runtime_get_sync() would work? I'm not sure > > > > about the reason for this distinction. > > > > > > The meaning of pm_runtime_get()/pm_runtime_get_sync() is "prevent the > > > device from being suspended from now on and resume it if necessary" while > > > "runtime PM disabled and runtime_status == RPM_ACTIVE" may be interpreted > > > as "not necessary to resume", so it is reasonable to special case this > > > particular situation for these particular routines IMHO. > > > > By the same reasoning, the meaning of pm_runtime_resume() is "resume > > the device now if necsesary". Since "runtime PM disabled and > > runtime_status == RPM_ACTIVE" means "not necessary to resume", isn't it > > logical for pm_runtime_resume() also to succeed under that condition? > > My point really is that pm_runtime_get()/pm_runtime_get_sync() actually carry > out two operations, (1) incrementing the usage counter and (2) resuming the > device if need be. It is not particularly clear if/when the result of (2) > should determine the return value of (1) and (2) together, so there is some > freedom in that area which we can use to cover special cases. That's all. Oh. Well, currently those routines are documented as returning the value from their internal pm_request_resume()/pm_runtime_resume() call. > I'm perfectly fine with leaving the code as is, though. You wanted to change it. :-) I'd be equally happy with a "runtime_pm_not_supported" flag, or something equivalent. Implementing it wouldn't be hard: Setting the flag could call pm_runtime_forbid() and make the power/control sysfs attribute return -EPERM for any attempted write. Perhaps also make power/runtime_status show "unsupported" rather than "on". (Hmmm, now that I look at rtpm_status_show(), I see that it already shows "unsupported" whenever disable_depth > 0. Another indication of how confused the situation is?) Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-20 14:43 ` Alan Stern (?) @ 2014-06-22 13:21 ` Rafael J. Wysocki 2014-06-22 16:45 ` Alan Stern -1 siblings, 1 reply; 65+ messages in thread From: Rafael J. Wysocki @ 2014-06-22 13:21 UTC (permalink / raw) To: Alan Stern Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Friday, June 20, 2014 10:43:11 AM Alan Stern wrote: > On Fri, 20 Jun 2014, Rafael J. Wysocki wrote: > > > On Thursday, June 19, 2014 10:34:01 AM Alan Stern wrote: > > > On Thu, 19 Jun 2014, Rafael J. Wysocki wrote: > > > > > > > Well, we used to have the notion that runtime_status is not meaningful for > > > > devices with dev->power.disable_depth greater than 0 (except for the special > > > > case in the suspend code path where we know why it is greater than 0). I think > > > > it was useful. :-) > > > > > > Did we really have that notion? My memory is a little cloudy, but I > > > thought we decided that runtime_status would not be meaningful when > > > dev->power.runtime_error was set -- not when dev->power.disable_depth > > > was greater than 0. Am I mixed up? > > > > Well, we clearly did, because we added things like > > > > pm_runtime_set_active() > > pm_runtime_set_suspended() > > > > which allow the status to be changed without doing anything to the device > > while power.disable_depth is greater than 0. > > Yes. I'm not sure that was thought out as carefully as it could have > been. > > Even with that restriction, it's not possible to guarantee that drivers > won't make mistakes. For example, suppose runtime_status is RPM_ACTIVE > but the device really is powered down. Before pm_runtime_disable() is > called, pm_runtime_resume() will succeed. The driver will think it can > use the device, even though it can't. > > This demonstrates that changing the runtime status can't be done > properly unless the subsystem and driver take some of the > responsibility. That definitely is the case. > Since that is true, perhaps we ought to allow drivers > to call pm_runtime_set_active/suspended() whenever they want, and rely > on them not to mess things up. We can do that. After all, having to disable runtime PM temporarily just in order to change the status is not convenient after all. IIRC the idea was to discourage drivers from setting the status in arbitrary ways, but if that is too big of a problem ... > As far as I know, there are only two major use cases for > _set_active/_set_suspended: when the device is being initialized during > probe, and during system resume. I think you're right. > During probe it's pretty safe to > assume there won't be any runtime PM calls, and much the same is true > during system resume (especially during the resume_irq and resume_early > phases). Therefore relying on drivers shouldn't make their jobs any > harder. I'm a little afraid of bugs resulting from being unsure how to use the API, however. > > > In any case, I think it is reasonable to regard runtime_status as > > > meaningful when disable_depth > 0. > > > > So we need to remove the above helpers and modify all code using them. > > We don't have to remove the helpers. And practically none of the code > using them would need to be modified. A certain amount of auditing > would be a good idea, though. Auditing would help anyway, but I'm still not sure about the entire idea. For example, what's the meaning of runtime_status for devices that haven't been initialized yet? > > > The PM core isn't allowed to invoke the runtime callbacks at such times, > > > that's all. This makes perfect sense for a device that doesn't support > > > power management and hence must always be at full power. Or when a driver > > > knows that runtime_status is out of agreement with the device's actual > > > power state and wants to update runtime_status directly. > > > > That's what it is supposed to use the above helpers for, isn't it? > > > > Devices that don't support power management, but that we want to use > > drivers supporting power management with are clearly a special case, so > > Are they? I'm not so sure. But never mind... > > > perhaps we should treat them as such instead of trying to modity the core > > to silently cover this case too automatically? > > How would you treat them specially? Add a "runtime_pm_not_supported" > flag? I thought about a "runtime PM has been enabled at least once" flag rather that would be set by pm_runtime_enable() every time it is called and never cleared. That would allow the core to distinguish between "runtime PM disabled temporarily" and "runtime PM not used" which turn out to be sufficiently different cases. > > > > > So pm_runtime_resume() and pm_request_resume() would still fail, but > > > > > pm_runtime_get() and pm_runtime_get_sync() would work? I'm not sure > > > > > about the reason for this distinction. > > > > > > > > The meaning of pm_runtime_get()/pm_runtime_get_sync() is "prevent the > > > > device from being suspended from now on and resume it if necessary" while > > > > "runtime PM disabled and runtime_status == RPM_ACTIVE" may be interpreted > > > > as "not necessary to resume", so it is reasonable to special case this > > > > particular situation for these particular routines IMHO. > > > > > > By the same reasoning, the meaning of pm_runtime_resume() is "resume > > > the device now if necsesary". Since "runtime PM disabled and > > > runtime_status == RPM_ACTIVE" means "not necessary to resume", isn't it > > > logical for pm_runtime_resume() also to succeed under that condition? > > > > My point really is that pm_runtime_get()/pm_runtime_get_sync() actually carry > > out two operations, (1) incrementing the usage counter and (2) resuming the > > device if need be. It is not particularly clear if/when the result of (2) > > should determine the return value of (1) and (2) together, so there is some > > freedom in that area which we can use to cover special cases. That's all. > > Oh. Well, currently those routines are documented as returning the > value from their internal pm_request_resume()/pm_runtime_resume() call. Yes, they are, but that's because we made that particular choice at one point. The freedom is there, though, and we can update the documentation too. :-) > > I'm perfectly fine with leaving the code as is, though. You wanted to change it. :-) > > I'd be equally happy with a "runtime_pm_not_supported" flag, or > something equivalent. Implementing it wouldn't be hard: Setting the > flag could call pm_runtime_forbid() and make the power/control sysfs > attribute return -EPERM for any attempted write. Perhaps also make > power/runtime_status show "unsupported" rather than "on". > > (Hmmm, now that I look at rtpm_status_show(), I see that it already > shows "unsupported" whenever disable_depth > 0. Another indication of > how confused the situation is?) Yes. The core definitely needs to be able to distinguish between the "runtime PM disabled temporarily" and "runtime PM not supported/not used" situations. Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-22 13:21 ` Rafael J. Wysocki @ 2014-06-22 16:45 ` Alan Stern 0 siblings, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-06-22 16:45 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Sun, 22 Jun 2014, Rafael J. Wysocki wrote: > > How would you treat them specially? Add a "runtime_pm_not_supported" > > flag? > > I thought about a "runtime PM has been enabled at least once" flag rather > that would be set by pm_runtime_enable() every time it is called and never > cleared. That would allow the core to distinguish between "runtime PM > disabled temporarily" and "runtime PM not used" which turn out to be > sufficiently different cases. Interesting idea, but it can't tell the difference between "runtime PM not supported" and "runtime PM not enabled yet". I think a simple "not supported" flag will be more straightforward. > Yes. The core definitely needs to be able to distinguish between the > "runtime PM disabled temporarily" and "runtime PM not supported/not used" > situations. Let me work out a patch, and we'll see what you think. For the time being we can stick with our "runtime PM must be disabled (or in error) when the status is changed" approach. Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. @ 2014-06-22 16:45 ` Alan Stern 0 siblings, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-06-22 16:45 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Sun, 22 Jun 2014, Rafael J. Wysocki wrote: > > How would you treat them specially? Add a "runtime_pm_not_supported" > > flag? > > I thought about a "runtime PM has been enabled at least once" flag rather > that would be set by pm_runtime_enable() every time it is called and never > cleared. That would allow the core to distinguish between "runtime PM > disabled temporarily" and "runtime PM not used" which turn out to be > sufficiently different cases. Interesting idea, but it can't tell the difference between "runtime PM not supported" and "runtime PM not enabled yet". I think a simple "not supported" flag will be more straightforward. > Yes. The core definitely needs to be able to distinguish between the > "runtime PM disabled temporarily" and "runtime PM not supported/not used" > situations. Let me work out a patch, and we'll see what you think. For the time being we can stick with our "runtime PM must be disabled (or in error) when the status is changed" approach. Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended. 2014-06-22 16:45 ` Alan Stern (?) @ 2014-06-24 23:38 ` Rafael J. Wysocki 2014-06-27 18:27 ` [RFC] Add "rpm_not_supported" flag Alan Stern -1 siblings, 1 reply; 65+ messages in thread From: Rafael J. Wysocki @ 2014-06-24 23:38 UTC (permalink / raw) To: Alan Stern Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, linux-pm, linux-kernel On Sunday, June 22, 2014 12:45:42 PM Alan Stern wrote: > On Sun, 22 Jun 2014, Rafael J. Wysocki wrote: > > > > How would you treat them specially? Add a "runtime_pm_not_supported" > > > flag? > > > > I thought about a "runtime PM has been enabled at least once" flag rather > > that would be set by pm_runtime_enable() every time it is called and never > > cleared. That would allow the core to distinguish between "runtime PM > > disabled temporarily" and "runtime PM not used" which turn out to be > > sufficiently different cases. > > Interesting idea, but it can't tell the difference between "runtime PM > not supported" and "runtime PM not enabled yet". I think a simple "not > supported" flag will be more straightforward. The question is who will set the "unsupported" flag (think devices without drivers etc.). Or perhaps the idea is that it will be set to start with? > > Yes. The core definitely needs to be able to distinguish between the > > "runtime PM disabled temporarily" and "runtime PM not supported/not used" > > situations. > > Let me work out a patch, and we'll see what you think. For the time > being we can stick with our "runtime PM must be disabled (or in error) > when the status is changed" approach. OK Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* [RFC] Add "rpm_not_supported" flag 2014-06-24 23:38 ` Rafael J. Wysocki @ 2014-06-27 18:27 ` Alan Stern 2014-06-27 19:22 ` Greg Kroah-Hartman 0 siblings, 1 reply; 65+ messages in thread From: Alan Stern @ 2014-06-27 18:27 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Allen Yu, Pavel Machek, Len Brown, Greg Kroah-Hartman, Dan Williams, Linux-pm mailing list, Kernel development list On Wed, 25 Jun 2014, Rafael J. Wysocki wrote: > On Sunday, June 22, 2014 12:45:42 PM Alan Stern wrote: > > On Sun, 22 Jun 2014, Rafael J. Wysocki wrote: > > > > > > How would you treat them specially? Add a "runtime_pm_not_supported" > > > > flag? > > > > > > I thought about a "runtime PM has been enabled at least once" flag rather > > > that would be set by pm_runtime_enable() every time it is called and never > > > cleared. That would allow the core to distinguish between "runtime PM > > > disabled temporarily" and "runtime PM not used" which turn out to be > > > sufficiently different cases. > > > > Interesting idea, but it can't tell the difference between "runtime PM > > not supported" and "runtime PM not enabled yet". I think a simple "not > > supported" flag will be more straightforward. > > The question is who will set the "unsupported" flag (think devices without > drivers etc.). Or perhaps the idea is that it will be set to start with? Drivers or subsystems will set the flag. It should not be set for devices without drivers or subsystems, because the flag means that the hardware doesn't support runtime power management, and the kernel wouldn't know this if there was no driver or subsystem. The flag will not be set to start with. The idea is that you set it when you know for certain that the device cannot be power-managed, but you still want the Runtime PM API to work with the device. In particular, calls to pm_runtime_resume() will succeed. > > > Yes. The core definitely needs to be able to distinguish between the > > > "runtime PM disabled temporarily" and "runtime PM not supported/not used" > > > situations. > > > > Let me work out a patch, and we'll see what you think. For the time > > being we can stick with our "runtime PM must be disabled (or in error) > > when the status is changed" approach. > > OK The patch is below. I haven't tested it with anything meaningful, but it seems straightforward enough. One side point: The patch changes the string displayed for the power/runtime_status attribute file when disable_depth > 0. Instead of "unsupported", it will now say "disabled". The attribute will contain "not supported" when the new flag is set. Is this acceptable? Alan Stern Documentation/power/runtime_pm.txt | 20 +++++++++++++++++++- drivers/base/power/runtime.c | 36 ++++++++++++++++++++++++++++++++++-- drivers/base/power/sysfs.c | 6 ++++-- include/linux/pm.h | 1 + include/linux/pm_runtime.h | 2 ++ 5 files changed, 60 insertions(+), 5 deletions(-) Index: usb-3.16/Documentation/power/runtime_pm.txt =================================================================== --- usb-3.16.orig/Documentation/power/runtime_pm.txt +++ usb-3.16/Documentation/power/runtime_pm.txt @@ -233,6 +233,10 @@ defined in include/linux/pm.h: equal to zero); the initial value of it is 1 (i.e. runtime PM is initially disabled for all devices) + int rpm_not_supported; + - if set, the device does not support runtime power management; attempts to + suspend the device will fail with -EOPNOTSUPP + int runtime_error; - if set, there was a fatal error (one of the callbacks returned error code as described in Section 2), so the helper funtions will not work until @@ -419,6 +423,11 @@ drivers/base/power/runtime.c and include void pm_suspend_ignore_children(struct device *dev, bool enable); - set/unset the power.ignore_children flag of the device + void pm_runtime_not_supported(struct device *dev); + - set the device's 'power.rpm_not_supported' flag, set the device's runtime + status to 'active', and increment the device's usage counter; this action + is irreversible + int pm_runtime_set_active(struct device *dev); - clear the device's 'power.runtime_error' flag, set the device's runtime PM status to 'active' and update its parent's counter of 'active' @@ -432,7 +441,7 @@ drivers/base/power/runtime.c and include PM status to 'suspended' and update its parent's counter of 'active' children as appropriate (it is only valid to use this function if 'power.runtime_error' is set or 'power.disable_depth' is greater than - zero) + zero, and 'power.rpm_not_supported' must be zero) bool pm_runtime_active(struct device *dev); - return true if the device's runtime PM status is 'active' or its @@ -586,6 +595,15 @@ value of /sys/devices/.../power/control manage the device at run time, the driver may confuse it by using pm_runtime_forbid() this way. +If a driver supports runtime PM but one of its devices does not, the driver +can call pm_runtime_not_supported() before calling pm_runtime_enable(). After +that, calls to pm_runtime_resume() and related functions will succeed, but +attempts to suspend the device will fail (much like what happens after calling +pm_runtime_forbid(), except that pm_runtime_not_supported() cannot be undone by +either the kernel or the user). It is not necessary to call +pm_runtime_set_active() for the device; pm_runtime_not_supported() sets the +runtime PM status to 'active'. + 6. Runtime PM and System Sleep Runtime PM and system sleep (i.e., system suspend and hibernation, also known Index: usb-3.16/include/linux/pm.h =================================================================== --- usb-3.16.orig/include/linux/pm.h +++ usb-3.16/include/linux/pm.h @@ -584,6 +584,7 @@ struct dev_pm_info { atomic_t usage_count; atomic_t child_count; unsigned int disable_depth:3; + unsigned int rpm_not_supported:1; unsigned int idle_notification:1; unsigned int request_pending:1; unsigned int deferred_resume:1; Index: usb-3.16/include/linux/pm_runtime.h =================================================================== --- usb-3.16.orig/include/linux/pm_runtime.h +++ usb-3.16/include/linux/pm_runtime.h @@ -47,6 +47,7 @@ extern int __pm_runtime_set_status(struc extern int pm_runtime_barrier(struct device *dev); extern void pm_runtime_enable(struct device *dev); extern void __pm_runtime_disable(struct device *dev, bool check_resume); +extern void pm_runtime_not_supported(struct device *dev); extern void pm_runtime_allow(struct device *dev); extern void pm_runtime_forbid(struct device *dev); extern void pm_runtime_no_callbacks(struct device *dev); @@ -144,6 +145,7 @@ static inline int __pm_runtime_set_statu static inline int pm_runtime_barrier(struct device *dev) { return 0; } static inline void pm_runtime_enable(struct device *dev) {} static inline void __pm_runtime_disable(struct device *dev, bool c) {} +static inline void pm_runtime_not_supported(struct device *dev) {} static inline void pm_runtime_allow(struct device *dev) {} static inline void pm_runtime_forbid(struct device *dev) {} Index: usb-3.16/drivers/base/power/runtime.c =================================================================== --- usb-3.16.orig/drivers/base/power/runtime.c +++ usb-3.16/drivers/base/power/runtime.c @@ -239,7 +239,9 @@ static int rpm_check_suspend_allowed(str { int retval = 0; - if (dev->power.runtime_error) + if (dev->power.rpm_not_supported) + retval = -EOPNOTSUPP; + else if (dev->power.runtime_error) retval = -EINVAL; else if (dev->power.disable_depth > 0) retval = -EACCES; @@ -974,7 +976,9 @@ EXPORT_SYMBOL_GPL(__pm_runtime_resume); * RPM_SUSPENDED, as long as that reflects the actual state of the device. * However, if the device has a parent and the parent is not active, and the * parent's power.ignore_children flag is unset, the device's status cannot be - * set to RPM_ACTIVE, so -EBUSY is returned in that case. + * set to RPM_ACTIVE, so -EBUSY is returned in that case. If the device's + * power.rpm_not_supported flag is set then the device's status cannot be set + * to RPM_SUSPENDED, so -EOPNOTSUPP is returned in that case. * * If successful, __pm_runtime_set_status() clears the power.runtime_error field * and the device parent's counter of unsuspended children is modified to @@ -1002,6 +1006,11 @@ int __pm_runtime_set_status(struct devic goto out_set; if (status == RPM_SUSPENDED) { + if (dev->power.rpm_not_supported) { + error = -EOPNOTSUPP; + goto out; + } + /* It always is possible to set the status to 'suspended'. */ if (parent) { atomic_add_unless(&parent->power.child_count, -1, 0); @@ -1195,6 +1204,26 @@ void pm_runtime_enable(struct device *de EXPORT_SYMBOL_GPL(pm_runtime_enable); /** + * pm_runtime_not_supported - A device doesn't support runtime PM + * @dev: Device to handle. + * + * Set @dev->power.rpm_not_supported, indicating the @dev doesn't support + * runtime PM and consequently must always be in the RPM_ACTIVE state. + * + * For good measure, also set runtime_status to RPM_ACTIVE and increment + * usage_count. + */ +void pm_runtime_not_supported(struct device *dev) +{ + spin_lock_irq(&dev->power.lock); + dev->power.rpm_not_supported = 1; + dev->power.runtime_status = RPM_ACTIVE; + atomic_inc(&dev->power.usage_count); + spin_unlock_irq(&dev->power.lock); +} +EXPORT_SYMBOL_GPL(pm_runtime_not_supported); + +/** * pm_runtime_forbid - Block runtime PM of a device. * @dev: Device to handle. * @@ -1415,6 +1444,9 @@ void pm_runtime_remove(struct device *de * * Typically this function may be invoked from a system suspend callback to make * sure the device is put into low power state. + * + * If the device's power.rpm_not_supported flag is set, the result of calling + * this function is undefined. */ int pm_runtime_force_suspend(struct device *dev) { Index: usb-3.16/drivers/base/power/sysfs.c =================================================================== --- usb-3.16.orig/drivers/base/power/sysfs.c +++ usb-3.16/drivers/base/power/sysfs.c @@ -163,10 +163,12 @@ static ssize_t rtpm_status_show(struct d { const char *p; - if (dev->power.runtime_error) { + if (dev->power.rpm_not_supported) { + p = "not supported\n"; + } else if (dev->power.runtime_error) { p = "error\n"; } else if (dev->power.disable_depth) { - p = "unsupported\n"; + p = "disabled\n"; } else { switch (dev->power.runtime_status) { case RPM_SUSPENDED: ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-06-27 18:27 ` [RFC] Add "rpm_not_supported" flag Alan Stern @ 2014-06-27 19:22 ` Greg Kroah-Hartman 2014-06-27 20:11 ` Alan Stern 0 siblings, 1 reply; 65+ messages in thread From: Greg Kroah-Hartman @ 2014-06-27 19:22 UTC (permalink / raw) To: Alan Stern Cc: Rafael J. Wysocki, Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list On Fri, Jun 27, 2014 at 02:27:28PM -0400, Alan Stern wrote: > On Wed, 25 Jun 2014, Rafael J. Wysocki wrote: > > > On Sunday, June 22, 2014 12:45:42 PM Alan Stern wrote: > > > On Sun, 22 Jun 2014, Rafael J. Wysocki wrote: > > > > > > > > How would you treat them specially? Add a "runtime_pm_not_supported" > > > > > flag? > > > > > > > > I thought about a "runtime PM has been enabled at least once" flag rather > > > > that would be set by pm_runtime_enable() every time it is called and never > > > > cleared. That would allow the core to distinguish between "runtime PM > > > > disabled temporarily" and "runtime PM not used" which turn out to be > > > > sufficiently different cases. > > > > > > Interesting idea, but it can't tell the difference between "runtime PM > > > not supported" and "runtime PM not enabled yet". I think a simple "not > > > supported" flag will be more straightforward. > > > > The question is who will set the "unsupported" flag (think devices without > > drivers etc.). Or perhaps the idea is that it will be set to start with? > > Drivers or subsystems will set the flag. It should not be set for > devices without drivers or subsystems, because the flag means that the > hardware doesn't support runtime power management, and the kernel > wouldn't know this if there was no driver or subsystem. > > The flag will not be set to start with. The idea is that you set it > when you know for certain that the device cannot be power-managed, but > you still want the Runtime PM API to work with the device. In > particular, calls to pm_runtime_resume() will succeed. > > > > > Yes. The core definitely needs to be able to distinguish between the > > > > "runtime PM disabled temporarily" and "runtime PM not supported/not used" > > > > situations. > > > > > > Let me work out a patch, and we'll see what you think. For the time > > > being we can stick with our "runtime PM must be disabled (or in error) > > > when the status is changed" approach. > > > > OK > > The patch is below. I haven't tested it with anything meaningful, but > it seems straightforward enough. > > One side point: The patch changes the string displayed for the > power/runtime_status attribute file when disable_depth > 0. Instead of > "unsupported", it will now say "disabled". The attribute will contain > "not supported" when the new flag is set. > > Is this acceptable? Why change the "unsupported" string? Can't we just leave that one alone? I'd prefer to not break userspace tools... thanks, greg k-h ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-06-27 19:22 ` Greg Kroah-Hartman @ 2014-06-27 20:11 ` Alan Stern 2014-06-27 20:50 ` Greg Kroah-Hartman 0 siblings, 1 reply; 65+ messages in thread From: Alan Stern @ 2014-06-27 20:11 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Rafael J. Wysocki, Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list On Fri, 27 Jun 2014, Greg Kroah-Hartman wrote: > > One side point: The patch changes the string displayed for the > > power/runtime_status attribute file when disable_depth > 0. Instead of > > "unsupported", it will now say "disabled". The attribute will contain > > "not supported" when the new flag is set. > > > > Is this acceptable? > > Why change the "unsupported" string? Can't we just leave that one > alone? I'd prefer to not break userspace tools... I changed it because it's wrong. disable_depth > 0 means that runtime PM has temporarily been disabled, or was never enabled in the first place. It doesn't mean that runtime PM is unsupported. In fact, the word "unsupported" is ambiguous. Does it mean unsupported by the hardware or unsupported by the kernel? The hardware can't change, but the kernel can be altered by loading a module. If that change is too intrusive, I can remove it from the patch. Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-06-27 20:11 ` Alan Stern @ 2014-06-27 20:50 ` Greg Kroah-Hartman 2014-06-28 15:32 ` Alan Stern 0 siblings, 1 reply; 65+ messages in thread From: Greg Kroah-Hartman @ 2014-06-27 20:50 UTC (permalink / raw) To: Alan Stern Cc: Rafael J. Wysocki, Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list On Fri, Jun 27, 2014 at 04:11:35PM -0400, Alan Stern wrote: > On Fri, 27 Jun 2014, Greg Kroah-Hartman wrote: > > > > One side point: The patch changes the string displayed for the > > > power/runtime_status attribute file when disable_depth > 0. Instead of > > > "unsupported", it will now say "disabled". The attribute will contain > > > "not supported" when the new flag is set. > > > > > > Is this acceptable? > > > > Why change the "unsupported" string? Can't we just leave that one > > alone? I'd prefer to not break userspace tools... > > I changed it because it's wrong. disable_depth > 0 means that runtime > PM has temporarily been disabled, or was never enabled in the first > place. It doesn't mean that runtime PM is unsupported. > > In fact, the word "unsupported" is ambiguous. Does it mean unsupported > by the hardware or unsupported by the kernel? The hardware can't > change, but the kernel can be altered by loading a module. > > If that change is too intrusive, I can remove it from the patch. Do you know of any tools that actually look at these files? If there isn't any, then we can try to change it and see who screams :) greg k-h ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-06-27 20:50 ` Greg Kroah-Hartman @ 2014-06-28 15:32 ` Alan Stern 2014-06-30 13:52 ` Rafael J. Wysocki 0 siblings, 1 reply; 65+ messages in thread From: Alan Stern @ 2014-06-28 15:32 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Rafael J. Wysocki, Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list On Fri, 27 Jun 2014, Greg Kroah-Hartman wrote: > On Fri, Jun 27, 2014 at 04:11:35PM -0400, Alan Stern wrote: > > On Fri, 27 Jun 2014, Greg Kroah-Hartman wrote: > > > > > > One side point: The patch changes the string displayed for the > > > > power/runtime_status attribute file when disable_depth > 0. Instead of > > > > "unsupported", it will now say "disabled". The attribute will contain > > > > "not supported" when the new flag is set. > > > > > > > > Is this acceptable? > > > > > > Why change the "unsupported" string? Can't we just leave that one > > > alone? I'd prefer to not break userspace tools... > > > > I changed it because it's wrong. disable_depth > 0 means that runtime > > PM has temporarily been disabled, or was never enabled in the first > > place. It doesn't mean that runtime PM is unsupported. > > > > In fact, the word "unsupported" is ambiguous. Does it mean unsupported > > by the hardware or unsupported by the kernel? The hardware can't > > change, but the kernel can be altered by loading a module. > > > > If that change is too intrusive, I can remove it from the patch. > > Do you know of any tools that actually look at these files? I don't. Of course, that doesn't mean much. > If there isn't any, then we can try to change it and see who screams :) It'll be a learning experience... Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-06-28 15:32 ` Alan Stern @ 2014-06-30 13:52 ` Rafael J. Wysocki 2014-06-30 14:42 ` Alan Stern 0 siblings, 1 reply; 65+ messages in thread From: Rafael J. Wysocki @ 2014-06-30 13:52 UTC (permalink / raw) To: Alan Stern Cc: Greg Kroah-Hartman, Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list On Saturday, June 28, 2014 11:32:21 AM Alan Stern wrote: > On Fri, 27 Jun 2014, Greg Kroah-Hartman wrote: > > > On Fri, Jun 27, 2014 at 04:11:35PM -0400, Alan Stern wrote: > > > On Fri, 27 Jun 2014, Greg Kroah-Hartman wrote: > > > > > > > > One side point: The patch changes the string displayed for the > > > > > power/runtime_status attribute file when disable_depth > 0. Instead of > > > > > "unsupported", it will now say "disabled". The attribute will contain > > > > > "not supported" when the new flag is set. > > > > > > > > > > Is this acceptable? > > > > > > > > Why change the "unsupported" string? Can't we just leave that one > > > > alone? I'd prefer to not break userspace tools... > > > > > > I changed it because it's wrong. disable_depth > 0 means that runtime > > > PM has temporarily been disabled, or was never enabled in the first > > > place. It doesn't mean that runtime PM is unsupported. > > > > > > In fact, the word "unsupported" is ambiguous. Does it mean unsupported > > > by the hardware or unsupported by the kernel? The hardware can't > > > change, but the kernel can be altered by loading a module. > > > > > > If that change is too intrusive, I can remove it from the patch. > > > > Do you know of any tools that actually look at these files? > > I don't. Of course, that doesn't mean much. The only tool I'm aware of that may be looking at them is powertop, so if the change doesn't affect powertop adversely, it should be generally safe. > > If there isn't any, then we can try to change it and see who screams :) > > It'll be a learning experience... Yes, it will. :-) Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-06-30 13:52 ` Rafael J. Wysocki @ 2014-06-30 14:42 ` Alan Stern 2014-07-01 23:18 ` Rafael J. Wysocki 0 siblings, 1 reply; 65+ messages in thread From: Alan Stern @ 2014-06-30 14:42 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Greg Kroah-Hartman, Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list On Mon, 30 Jun 2014, Rafael J. Wysocki wrote: > > > Do you know of any tools that actually look at these files? > > > > I don't. Of course, that doesn't mean much. > > The only tool I'm aware of that may be looking at them is powertop, so > if the change doesn't affect powertop adversely, it should be generally > safe. > > > > If there isn't any, then we can try to change it and see who screams :) > > > > It'll be a learning experience... > > Yes, it will. :-) Then you have no other objections to the patch? Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-06-30 14:42 ` Alan Stern @ 2014-07-01 23:18 ` Rafael J. Wysocki 2014-07-02 14:27 ` Alan Stern 0 siblings, 1 reply; 65+ messages in thread From: Rafael J. Wysocki @ 2014-07-01 23:18 UTC (permalink / raw) To: Alan Stern Cc: Greg Kroah-Hartman, Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list On Monday, June 30, 2014 10:42:19 AM Alan Stern wrote: > On Mon, 30 Jun 2014, Rafael J. Wysocki wrote: > > > > > Do you know of any tools that actually look at these files? > > > > > > I don't. Of course, that doesn't mean much. > > > > The only tool I'm aware of that may be looking at them is powertop, so > > if the change doesn't affect powertop adversely, it should be generally > > safe. > > > > > > If there isn't any, then we can try to change it and see who screams :) > > > > > > It'll be a learning experience... > > > > Yes, it will. :-) > > Then you have no other objections to the patch? My concern still is that it will be confusing, because people won't read the documentation carefully enough and will confuse "runtime PM never used" with "hardware can't do PM". I'm not sure how to make that more clear, though. Also we have the no_callbacks flag and I wonder if/how it is related to the new one. Do we still need both? In addition to that, I think that "hardware can't do PM" should apply to the handling of system suspend resume too. Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-07-01 23:18 ` Rafael J. Wysocki @ 2014-07-02 14:27 ` Alan Stern 2014-07-02 17:56 ` Greg Kroah-Hartman ` (2 more replies) 0 siblings, 3 replies; 65+ messages in thread From: Alan Stern @ 2014-07-02 14:27 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Greg Kroah-Hartman, Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list On Wed, 2 Jul 2014, Rafael J. Wysocki wrote: > > Then you have no other objections to the patch? > > My concern still is that it will be confusing, because people won't read the > documentation carefully enough and will confuse "runtime PM never used" with > "hardware can't do PM". I'm not sure how to make that more clear, though. I could emphasize that distinction a little more strongly in the documentation. > Also we have the no_callbacks flag and I wonder if/how it is related to the > new one. Do we still need both? They mean different things. The no_callbacks flag is used when we want the PM core to think the device can be in RPM_SUSPENDED at times (it is "logically suspended"). rpm_not_supported is used when we want the PM core to think the device must always be in RPM_ACTIVE. > In addition to that, I think that "hardware can't do PM" should apply to the > handling of system suspend resume too. Maybe. For the use case Dan Williams and I are working on, it doesn't matter; for other cases it might matter. That's why I named the flag "rpm_not_supported" -- it applies specifically to runtime PM, not system PM. Here's a brief summary of the story behind this patch... At one point, I suggested to Dan that instead of doing something special for these devices, we could simply have the runtime_suspend() routine always return -EBUSY. He didn't like that idea because then the user would see the device was never powering down but would have no idea why. The rpm_not_supported flag provides this information to the user by causing the power/runtime_status attribute to say "not supported". (Although to be entirely fair, we could just put a message in the kernel log during probe if the hardware doesn't support runtime suspend.) Instead, Dan introduced a messy PM QoS mechanism in commit e3d105055525. I didn't like that approach, but Greg merged it before I objected. Do you have any suggestions? Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-07-02 14:27 ` Alan Stern @ 2014-07-02 17:56 ` Greg Kroah-Hartman 2014-07-03 21:16 ` Rafael J. Wysocki 2014-07-16 22:40 ` Rafael J. Wysocki 2 siblings, 0 replies; 65+ messages in thread From: Greg Kroah-Hartman @ 2014-07-02 17:56 UTC (permalink / raw) To: Alan Stern Cc: Rafael J. Wysocki, Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list On Wed, Jul 02, 2014 at 10:27:06AM -0400, Alan Stern wrote: > On Wed, 2 Jul 2014, Rafael J. Wysocki wrote: > > > > Then you have no other objections to the patch? > > > > My concern still is that it will be confusing, because people won't read the > > documentation carefully enough and will confuse "runtime PM never used" with > > "hardware can't do PM". I'm not sure how to make that more clear, though. > > I could emphasize that distinction a little more strongly in the > documentation. > > > Also we have the no_callbacks flag and I wonder if/how it is related to the > > new one. Do we still need both? > > They mean different things. The no_callbacks flag is used when we want > the PM core to think the device can be in RPM_SUSPENDED at times (it is > "logically suspended"). rpm_not_supported is used when we want the PM > core to think the device must always be in RPM_ACTIVE. > > > In addition to that, I think that "hardware can't do PM" should apply to the > > handling of system suspend resume too. > > Maybe. For the use case Dan Williams and I are working on, it doesn't > matter; for other cases it might matter. That's why I named the flag > "rpm_not_supported" -- it applies specifically to runtime PM, not > system PM. > > Here's a brief summary of the story behind this patch... > > At one point, I suggested to Dan that instead of doing something > special for these devices, we could simply have the runtime_suspend() > routine always return -EBUSY. He didn't like that idea because then > the user would see the device was never powering down but would have no > idea why. The rpm_not_supported flag provides this information to the > user by causing the power/runtime_status attribute to say "not > supported". (Although to be entirely fair, we could just put a message > in the kernel log during probe if the hardware doesn't support runtime > suspend.) > > Instead, Dan introduced a messy PM QoS mechanism in commit > e3d105055525. I didn't like that approach, but Greg merged it before I > objected. Sorry about that, we can always revert it :) greg k-h ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-07-02 14:27 ` Alan Stern 2014-07-02 17:56 ` Greg Kroah-Hartman @ 2014-07-03 21:16 ` Rafael J. Wysocki 2014-07-03 21:17 ` Alan Stern 2014-07-16 22:40 ` Rafael J. Wysocki 2 siblings, 1 reply; 65+ messages in thread From: Rafael J. Wysocki @ 2014-07-03 21:16 UTC (permalink / raw) To: Alan Stern Cc: Greg Kroah-Hartman, Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list On Wednesday, July 02, 2014 10:27:06 AM Alan Stern wrote: > On Wed, 2 Jul 2014, Rafael J. Wysocki wrote: > > > > Then you have no other objections to the patch? > > > > My concern still is that it will be confusing, because people won't read the > > documentation carefully enough and will confuse "runtime PM never used" with > > "hardware can't do PM". I'm not sure how to make that more clear, though. > > I could emphasize that distinction a little more strongly in the > documentation. > > > Also we have the no_callbacks flag and I wonder if/how it is related to the > > new one. Do we still need both? > > They mean different things. The no_callbacks flag is used when we want > the PM core to think the device can be in RPM_SUSPENDED at times (it is > "logically suspended"). rpm_not_supported is used when we want the PM > core to think the device must always be in RPM_ACTIVE. > > > In addition to that, I think that "hardware can't do PM" should apply to the > > handling of system suspend resume too. > > Maybe. For the use case Dan Williams and I are working on, it doesn't > matter; for other cases it might matter. That's why I named the flag > "rpm_not_supported" -- it applies specifically to runtime PM, not > system PM. > > Here's a brief summary of the story behind this patch... > > At one point, I suggested to Dan that instead of doing something > special for these devices, we could simply have the runtime_suspend() > routine always return -EBUSY. He didn't like that idea because then > the user would see the device was never powering down but would have no > idea why. The rpm_not_supported flag provides this information to the > user by causing the power/runtime_status attribute to say "not > supported". (Although to be entirely fair, we could just put a message > in the kernel log during probe if the hardware doesn't support runtime > suspend.) > > Instead, Dan introduced a messy PM QoS mechanism in commit > e3d105055525. I didn't like that approach, but Greg merged it before I > objected. > > Do you have any suggestions? I need some more time to think about that. I'm on vacation till Monday, I should be able to get to this by then. I hope that's not a problem. Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-07-03 21:16 ` Rafael J. Wysocki @ 2014-07-03 21:17 ` Alan Stern 0 siblings, 0 replies; 65+ messages in thread From: Alan Stern @ 2014-07-03 21:17 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Greg Kroah-Hartman, Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list On Thu, 3 Jul 2014, Rafael J. Wysocki wrote: > > Do you have any suggestions? > > I need some more time to think about that. I'm on vacation till Monday, > I should be able to get to this by then. I hope that's not a problem. Tomorrow is a national holiday in the US, so I'm taking some time off too. Don't hurry. Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-07-02 14:27 ` Alan Stern 2014-07-02 17:56 ` Greg Kroah-Hartman 2014-07-03 21:16 ` Rafael J. Wysocki @ 2014-07-16 22:40 ` Rafael J. Wysocki 2014-07-16 23:03 ` Greg Kroah-Hartman 2 siblings, 1 reply; 65+ messages in thread From: Rafael J. Wysocki @ 2014-07-16 22:40 UTC (permalink / raw) To: Alan Stern, Greg Kroah-Hartman Cc: Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list That took me much more time than I had hoped, sorry about that. On Wednesday, July 02, 2014 10:27:06 AM Alan Stern wrote: > On Wed, 2 Jul 2014, Rafael J. Wysocki wrote: > > > > Then you have no other objections to the patch? > > > > My concern still is that it will be confusing, because people won't read the > > documentation carefully enough and will confuse "runtime PM never used" with > > "hardware can't do PM". I'm not sure how to make that more clear, though. > > I could emphasize that distinction a little more strongly in the > documentation. So it looks like we'd like to cover the "enabled but always active" case for runtime PM of a device? > > Also we have the no_callbacks flag and I wonder if/how it is related to the > > new one. Do we still need both? > > They mean different things. The no_callbacks flag is used when we want > the PM core to think the device can be in RPM_SUSPENDED at times (it is > "logically suspended"). rpm_not_supported is used when we want the PM > core to think the device must always be in RPM_ACTIVE. > > > In addition to that, I think that "hardware can't do PM" should apply to the > > handling of system suspend resume too. > > Maybe. For the use case Dan Williams and I are working on, it doesn't > matter; for other cases it might matter. That's why I named the flag > "rpm_not_supported" -- it applies specifically to runtime PM, not > system PM. Maybe it'd be better to call the flag always_active and make rpm_suspend() and rpm_resume() react to it accordingly? So before enabling runtime PM the driver will set always_active and that will mean the device cannot be suspended. And we could add a separate sysfs attribute for that instead of re-purposing the status thing, if needed (I'm not sure about that, however). > Here's a brief summary of the story behind this patch... > > At one point, I suggested to Dan that instead of doing something > special for these devices, we could simply have the runtime_suspend() > routine always return -EBUSY. He didn't like that idea because then > the user would see the device was never powering down but would have no > idea why. The rpm_not_supported flag provides this information to the > user by causing the power/runtime_status attribute to say "not > supported". (Although to be entirely fair, we could just put a message > in the kernel log during probe if the hardware doesn't support runtime > suspend.) > > Instead, Dan introduced a messy PM QoS mechanism in commit > e3d105055525. I didn't like that approach, but Greg merged it before I > objected. That really looks a bit like a hack to me to be honest. Greg, what's your plan toward this? > Do you have any suggestions? We could use an "always_active" type of flag for that, but I'm not sure how much value to the user that will bring. Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-07-16 22:40 ` Rafael J. Wysocki @ 2014-07-16 23:03 ` Greg Kroah-Hartman 2014-07-16 23:27 ` Rafael J. Wysocki 0 siblings, 1 reply; 65+ messages in thread From: Greg Kroah-Hartman @ 2014-07-16 23:03 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Alan Stern, Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list On Thu, Jul 17, 2014 at 12:40:23AM +0200, Rafael J. Wysocki wrote: > On Wednesday, July 02, 2014 10:27:06 AM Alan Stern wrote: > > Here's a brief summary of the story behind this patch... > > > > At one point, I suggested to Dan that instead of doing something > > special for these devices, we could simply have the runtime_suspend() > > routine always return -EBUSY. He didn't like that idea because then > > the user would see the device was never powering down but would have no > > idea why. The rpm_not_supported flag provides this information to the > > user by causing the power/runtime_status attribute to say "not > > supported". (Although to be entirely fair, we could just put a message > > in the kernel log during probe if the hardware doesn't support runtime > > suspend.) > > > > Instead, Dan introduced a messy PM QoS mechanism in commit > > e3d105055525. I didn't like that approach, but Greg merged it before I > > objected. > > That really looks a bit like a hack to me to be honest. > > Greg, what's your plan toward this? If I need to revert something that you all find was wrong, I'll be glad to do so, sorry for merging something too early. thanks, greg k-h ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-07-16 23:03 ` Greg Kroah-Hartman @ 2014-07-16 23:27 ` Rafael J. Wysocki 2014-07-17 14:27 ` Alan Stern 0 siblings, 1 reply; 65+ messages in thread From: Rafael J. Wysocki @ 2014-07-16 23:27 UTC (permalink / raw) To: Alan Stern Cc: Greg Kroah-Hartman, Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list On Wednesday, July 16, 2014 04:03:45 PM Greg Kroah-Hartman wrote: > On Thu, Jul 17, 2014 at 12:40:23AM +0200, Rafael J. Wysocki wrote: > > On Wednesday, July 02, 2014 10:27:06 AM Alan Stern wrote: > > > Here's a brief summary of the story behind this patch... > > > > > > At one point, I suggested to Dan that instead of doing something > > > special for these devices, we could simply have the runtime_suspend() > > > routine always return -EBUSY. He didn't like that idea because then > > > the user would see the device was never powering down but would have no > > > idea why. The rpm_not_supported flag provides this information to the > > > user by causing the power/runtime_status attribute to say "not > > > supported". (Although to be entirely fair, we could just put a message > > > in the kernel log during probe if the hardware doesn't support runtime > > > suspend.) > > > > > > Instead, Dan introduced a messy PM QoS mechanism in commit > > > e3d105055525. I didn't like that approach, but Greg merged it before I > > > objected. > > > > That really looks a bit like a hack to me to be honest. > > > > Greg, what's your plan toward this? > > If I need to revert something that you all find was wrong, I'll be glad > to do so, sorry for merging something too early. Alan, what do you think? I think we're still unsure if the approach taken by that commit is correct, but then I suppose we don't need to revert it at this point and we can fix it later. Is that correct, or would fixing it be difficult for some reason? Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-07-16 23:27 ` Rafael J. Wysocki @ 2014-07-17 14:27 ` Alan Stern 2014-07-18 0:48 ` Rafael J. Wysocki 0 siblings, 1 reply; 65+ messages in thread From: Alan Stern @ 2014-07-17 14:27 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Greg Kroah-Hartman, Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list On Thu, 17 Jul 2014, Rafael J. Wysocki wrote: > On Wednesday, July 16, 2014 04:03:45 PM Greg Kroah-Hartman wrote: > > On Thu, Jul 17, 2014 at 12:40:23AM +0200, Rafael J. Wysocki wrote: > > > On Wednesday, July 02, 2014 10:27:06 AM Alan Stern wrote: > > > > Here's a brief summary of the story behind this patch... > > > > > > > > At one point, I suggested to Dan that instead of doing something > > > > special for these devices, we could simply have the runtime_suspend() > > > > routine always return -EBUSY. He didn't like that idea because then > > > > the user would see the device was never powering down but would have no > > > > idea why. The rpm_not_supported flag provides this information to the > > > > user by causing the power/runtime_status attribute to say "not > > > > supported". (Although to be entirely fair, we could just put a message > > > > in the kernel log during probe if the hardware doesn't support runtime > > > > suspend.) > > > > > > > > Instead, Dan introduced a messy PM QoS mechanism in commit > > > > e3d105055525. I didn't like that approach, but Greg merged it before I > > > > objected. > > > > > > That really looks a bit like a hack to me to be honest. My thought exactly. > > > Greg, what's your plan toward this? > > > > If I need to revert something that you all find was wrong, I'll be glad > > to do so, sorry for merging something too early. > > Alan, what do you think? I'm in favor of reverting that commit and putting something else in its place. But first we have to figure out an approach that will satisfy everyone. I've been hoping that Dan would contribute to this thread. He has spear-headed this whole line of development, and he wrote the commit in question. Maybe he would be happy with the simplest approach: If the device doesn't support runtime PM, write a message in the kernel log and make ->runtime_suspend() always return -EBUSY. > I think we're still unsure if the approach taken by that commit is correct, > but then I suppose we don't need to revert it at this point and we can fix > it later. Is that correct, or would fixing it be difficult for some reason? As far as I can see, it would be okay to wait a while before making any changes. Alan Stern ^ permalink raw reply [flat|nested] 65+ messages in thread
* Re: [RFC] Add "rpm_not_supported" flag 2014-07-17 14:27 ` Alan Stern @ 2014-07-18 0:48 ` Rafael J. Wysocki 0 siblings, 0 replies; 65+ messages in thread From: Rafael J. Wysocki @ 2014-07-18 0:48 UTC (permalink / raw) To: Alan Stern Cc: Greg Kroah-Hartman, Allen Yu, Pavel Machek, Len Brown, Dan Williams, Linux-pm mailing list, Kernel development list On Thursday, July 17, 2014 10:27:44 AM Alan Stern wrote: > On Thu, 17 Jul 2014, Rafael J. Wysocki wrote: > > > On Wednesday, July 16, 2014 04:03:45 PM Greg Kroah-Hartman wrote: > > > On Thu, Jul 17, 2014 at 12:40:23AM +0200, Rafael J. Wysocki wrote: > > > > On Wednesday, July 02, 2014 10:27:06 AM Alan Stern wrote: > > > > > Here's a brief summary of the story behind this patch... > > > > > > > > > > At one point, I suggested to Dan that instead of doing something > > > > > special for these devices, we could simply have the runtime_suspend() > > > > > routine always return -EBUSY. He didn't like that idea because then > > > > > the user would see the device was never powering down but would have no > > > > > idea why. The rpm_not_supported flag provides this information to the > > > > > user by causing the power/runtime_status attribute to say "not > > > > > supported". (Although to be entirely fair, we could just put a message > > > > > in the kernel log during probe if the hardware doesn't support runtime > > > > > suspend.) > > > > > > > > > > Instead, Dan introduced a messy PM QoS mechanism in commit > > > > > e3d105055525. I didn't like that approach, but Greg merged it before I > > > > > objected. > > > > > > > > That really looks a bit like a hack to me to be honest. > > My thought exactly. I'm also wondering why it unregisters the device if dev_pm_qos_add_request() fails instead simply avoiding to enable runtime PM in that case (just a side note). > > > > Greg, what's your plan toward this? > > > > > > If I need to revert something that you all find was wrong, I'll be glad > > > to do so, sorry for merging something too early. > > > > Alan, what do you think? > > I'm in favor of reverting that commit and putting something else in its > place. But first we have to figure out an approach that will satisfy > everyone. > > I've been hoping that Dan would contribute to this thread. He has > spear-headed this whole line of development, and he wrote the commit in > question. > > Maybe he would be happy with the simplest approach: If the device > doesn't support runtime PM, write a message in the kernel log and make > ->runtime_suspend() always return -EBUSY. And I wonder how commit e3d105055525 allows the user to check that the device is in the "always active" mode. I guess by the fact that the PM QoS flags are not visible in sysfs then, but that's far from obvious in my view. Rafael ^ permalink raw reply [flat|nested] 65+ messages in thread
end of thread, other threads:[~2014-07-18 0:30 UTC | newest] Thread overview: 65+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-06-14 10:03 [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended Allen Yu 2014-06-14 10:03 ` Allen Yu 2014-06-14 14:32 ` Alan Stern 2014-06-14 14:32 ` Alan Stern 2014-06-16 3:03 ` Allen Yu 2014-06-16 14:43 ` Alan Stern 2014-06-16 17:40 ` Alan Stern 2014-06-16 17:40 ` Alan Stern 2014-06-16 21:29 ` Rafael J. Wysocki 2014-06-17 14:11 ` Alan Stern 2014-06-17 14:11 ` Alan Stern 2014-06-17 20:26 ` Rafael J. Wysocki 2014-06-17 20:37 ` Rafael J. Wysocki 2014-06-17 20:46 ` Rafael J. Wysocki 2014-06-18 15:30 ` Alan Stern 2014-06-18 15:30 ` Alan Stern 2014-06-18 23:57 ` Rafael J. Wysocki 2014-06-19 8:23 ` Allen Yu 2014-06-19 8:23 ` Allen Yu 2014-06-19 13:55 ` Rafael J. Wysocki 2014-06-19 14:34 ` Allen Yu 2014-06-19 14:34 ` Allen Yu 2014-06-20 14:04 ` Rafael J. Wysocki 2014-06-19 14:56 ` Alan Stern 2014-06-19 19:25 ` Kevin Hilman 2014-06-19 19:25 ` Kevin Hilman 2014-06-19 20:13 ` Alan Stern 2014-06-20 13:20 ` Rafael J. Wysocki 2014-06-20 14:48 ` Alan Stern 2014-06-20 21:34 ` Kevin Hilman 2014-06-20 21:34 ` Kevin Hilman 2014-06-22 13:40 ` Rafael J. Wysocki 2014-06-22 13:24 ` Rafael J. Wysocki 2014-06-20 21:31 ` Kevin Hilman 2014-06-20 21:31 ` Kevin Hilman 2014-06-21 13:34 ` Alan Stern 2014-06-22 13:35 ` Rafael J. Wysocki 2014-06-23 18:57 ` Kevin Hilman 2014-06-23 18:57 ` Kevin Hilman 2014-06-19 14:34 ` Alan Stern 2014-06-19 14:34 ` Alan Stern 2014-06-20 13:33 ` Rafael J. Wysocki 2014-06-20 14:43 ` Alan Stern 2014-06-20 14:43 ` Alan Stern 2014-06-22 13:21 ` Rafael J. Wysocki 2014-06-22 16:45 ` Alan Stern 2014-06-22 16:45 ` Alan Stern 2014-06-24 23:38 ` Rafael J. Wysocki 2014-06-27 18:27 ` [RFC] Add "rpm_not_supported" flag Alan Stern 2014-06-27 19:22 ` Greg Kroah-Hartman 2014-06-27 20:11 ` Alan Stern 2014-06-27 20:50 ` Greg Kroah-Hartman 2014-06-28 15:32 ` Alan Stern 2014-06-30 13:52 ` Rafael J. Wysocki 2014-06-30 14:42 ` Alan Stern 2014-07-01 23:18 ` Rafael J. Wysocki 2014-07-02 14:27 ` Alan Stern 2014-07-02 17:56 ` Greg Kroah-Hartman 2014-07-03 21:16 ` Rafael J. Wysocki 2014-07-03 21:17 ` Alan Stern 2014-07-16 22:40 ` Rafael J. Wysocki 2014-07-16 23:03 ` Greg Kroah-Hartman 2014-07-16 23:27 ` Rafael J. Wysocki 2014-07-17 14:27 ` Alan Stern 2014-07-18 0:48 ` Rafael J. Wysocki
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.