[PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[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

[PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

RE: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Allen Yu]

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

RE: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Rafael J. Wysocki]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Rafael J. Wysocki]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Rafael J. Wysocki]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Rafael J. Wysocki]

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)

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Rafael J. Wysocki]

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

RE: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Allen Yu]

[-- 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¥

RE: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Allen Yu]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Rafael J. Wysocki]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

RE: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Allen Yu]

[-- 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¥

RE: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Allen Yu]

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

RE: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Kevin Hilman]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Kevin Hilman]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Rafael J. Wysocki]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Rafael J. Wysocki]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Rafael J. Wysocki]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Kevin Hilman]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Kevin Hilman]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Kevin Hilman]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Kevin Hilman]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Rafael J. Wysocki]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Rafael J. Wysocki]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Rafael J. Wysocki]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Rafael J. Wysocki]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Alan Stern]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Kevin Hilman]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Kevin Hilman]

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

Re: [PATCH 1/1] PM / Runtime: let rpm_resume fail if rpm disabled and device suspended.

[Rafael J. Wysocki]

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

[RFC] Add "rpm_not_supported" flag

[Alan Stern]

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:

Re: [RFC] Add "rpm_not_supported" flag

[Greg Kroah-Hartman]

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

Re: [RFC] Add "rpm_not_supported" flag

[Alan Stern]

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

Re: [RFC] Add "rpm_not_supported" flag

[Greg Kroah-Hartman]

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

Re: [RFC] Add "rpm_not_supported" flag

[Alan Stern]

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

Re: [RFC] Add "rpm_not_supported" flag

[Rafael J. Wysocki]

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

Re: [RFC] Add "rpm_not_supported" flag

[Alan Stern]

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

Re: [RFC] Add "rpm_not_supported" flag

[Rafael J. Wysocki]

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

Re: [RFC] Add "rpm_not_supported" flag

[Alan Stern]

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

Re: [RFC] Add "rpm_not_supported" flag

[Greg Kroah-Hartman]

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

Re: [RFC] Add "rpm_not_supported" flag

[Rafael J. Wysocki]

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

Re: [RFC] Add "rpm_not_supported" flag

[Alan Stern]

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

Re: [RFC] Add "rpm_not_supported" flag

[Rafael J. Wysocki]

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

Re: [RFC] Add "rpm_not_supported" flag

[Greg Kroah-Hartman]

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

Re: [RFC] Add "rpm_not_supported" flag

[Rafael J. Wysocki]

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

Re: [RFC] Add "rpm_not_supported" flag

[Alan Stern]

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

Re: [RFC] Add "rpm_not_supported" flag

[Rafael J. Wysocki]

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