| Message ID | 1348267654-30697-1-git-send-email-khilman@deeprootsystems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) >
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
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
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)