[PATCH v2] PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled

[PATCH v2] PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled

[Kevin Hilman]

From: Kevin Hilman <khilman@ti.com>

There are several drivers where the return value of
pm_runtime_get_sync() is used to decide whether or not it is safe to
access hardware and that don't provide .suspend() callbacks for system
suspend (but may use late/noirq callbacks.)  If such a driver happens
to call pm_runtime_get_sync() during system suspend, after the core
has disabled runtime PM, it will get the error code and will decide
that the hardware should not be accessed, although this may be a wrong
conclusion, depending on the state of the device when runtime PM was
disabled.

Drivers might work around this problem by using a test like:

   ret = pm_runtime_get_sync(dev);
   if (!ret || (ret == -EACCES && driver_private_data(dev)->suspended)) {
      /* access hardware */
   }

where driver_private_data(dev)->suspended is a flag set by the
driver's .suspend() method (that would have to be added for this
purpose).  However, that potentially would need to be done by multiple
drivers which means quite a lot of duplicated code and bloat.

To avoid that we can use the observation that the core sets
dev->power.is_suspended before disabling runtime PM and use that
instead of the driver's private flag.  Still, potentially many drivers
would need to repeat that same check in quite a few places, so it's
better to let the core do it.

Then we can be a bit smarter and check whether or not runtime PM was
disabled by the core only (disable_depth == 1) or by someone else in
addition to the core (disable_depth > 1).  In the former case
rpm_resume() can return 1 if the runtime PM status is RPM_ACTIVE,
because it means the device was active when the core disabled runtime
PM.  In the latter case it should still return -EACCES, because it
isn't clear why runtime PM has been disabled.

Tested on AM3730/Beagle-xM where a wakeup IRQ firing during the late
suspend phase triggers runtime PM activity in the I2C driver since the
wakeup IRQ is on an I2C-connected PMIC.

Cc: Rafael J. Wysocki <rjw@sisk.pl>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Kevin Hilman <khilman@ti.com>
---
v2: 
- major changelog rewrite, based largely on input from Rafael 
- add check for disable_depth == 1 and move to separate if statement,
  both suggested by Alan Stern
    

 drivers/base/power/runtime.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 7d9c1cb..d43856b 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -509,6 +509,9 @@ 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;
 	if (retval)
-- 
1.7.9.2

[PATCH v2] PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled

[Rafael J. Wysocki]

On Saturday, September 22, 2012, Kevin Hilman wrote:
> From: Kevin Hilman <khilman@ti.com>
> 
> There are several drivers where the return value of
> pm_runtime_get_sync() is used to decide whether or not it is safe to
> access hardware and that don't provide .suspend() callbacks for system
> suspend (but may use late/noirq callbacks.)  If such a driver happens
> to call pm_runtime_get_sync() during system suspend, after the core
> has disabled runtime PM, it will get the error code and will decide
> that the hardware should not be accessed, although this may be a wrong
> conclusion, depending on the state of the device when runtime PM was
> disabled.
> 
> Drivers might work around this problem by using a test like:
> 
>    ret = pm_runtime_get_sync(dev);
>    if (!ret || (ret == -EACCES && driver_private_data(dev)->suspended)) {
>       /* access hardware */
>    }
> 
> where driver_private_data(dev)->suspended is a flag set by the
> driver's .suspend() method (that would have to be added for this
> purpose).  However, that potentially would need to be done by multiple
> drivers which means quite a lot of duplicated code and bloat.
> 
> To avoid that we can use the observation that the core sets
> dev->power.is_suspended before disabling runtime PM and use that
> instead of the driver's private flag.  Still, potentially many drivers
> would need to repeat that same check in quite a few places, so it's
> better to let the core do it.
> 
> Then we can be a bit smarter and check whether or not runtime PM was
> disabled by the core only (disable_depth == 1) or by someone else in
> addition to the core (disable_depth > 1).  In the former case
> rpm_resume() can return 1 if the runtime PM status is RPM_ACTIVE,
> because it means the device was active when the core disabled runtime
> PM.  In the latter case it should still return -EACCES, because it
> isn't clear why runtime PM has been disabled.
> 
> Tested on AM3730/Beagle-xM where a wakeup IRQ firing during the late
> suspend phase triggers runtime PM activity in the I2C driver since the
> wakeup IRQ is on an I2C-connected PMIC.
> 
> Cc: Rafael J. Wysocki <rjw@sisk.pl>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Signed-off-by: Kevin Hilman <khilman@ti.com>
> ---
> v2: 
> - major changelog rewrite, based largely on input from Rafael 
> - add check for disable_depth == 1 and move to separate if statement,
>   both suggested by Alan Stern

OK, this looks good to me, thanks!

Alan, what do you think?

Rafael


>  drivers/base/power/runtime.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 7d9c1cb..d43856b 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -509,6 +509,9 @@ 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;
>  	if (retval)
> 

[PATCH v2] PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled

[Alan Stern]

On Sat, 22 Sep 2012, Rafael J. Wysocki wrote:

> On Saturday, September 22, 2012, Kevin Hilman wrote:

> OK, this looks good to me, thanks!
> 
> Alan, what do you think?
> 
> Rafael

> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -509,6 +509,9 @@ 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;
> >  	if (retval)

Well, I'd prefer the indentation on the continuation line to be 
different from the indentation of the following line, and I'd prefer 
to have a comment explaining the reason for the exception.

But these are only matters of taste; the implementation itself looks 
good.

Alan Stern

[PATCH v2] PM / Runtime: let rpm_resume() succeed if RPM_ACTIVE, even when disabled

[Rafael J. Wysocki]

On Saturday, September 22, 2012, Alan Stern wrote:
> On Sat, 22 Sep 2012, Rafael J. Wysocki wrote:
> 
> > On Saturday, September 22, 2012, Kevin Hilman wrote:
> 
> > OK, this looks good to me, thanks!
> > 
> > Alan, what do you think?
> > 
> > Rafael
> 
> > > --- a/drivers/base/power/runtime.c
> > > +++ b/drivers/base/power/runtime.c
> > > @@ -509,6 +509,9 @@ 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;
> > >  	if (retval)
> 
> Well, I'd prefer the indentation on the continuation line to be 
> different from the indentation of the following line, and I'd prefer 
> to have a comment explaining the reason for the exception.
> 
> But these are only matters of taste; the implementation itself looks 
> good.

Thanks!

I've applied the patch as v3.7 material (and fixed up the white space).

Rafael