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

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

[Rafael J. Wysocki]

On Thursday, September 20, 2012, Kevin Hilman wrote:
> From: Kevin Hilman <khilman@ti.com>
> 
> When runtime PM is disabled, what we want is for callbacks not to be
> called from then on.  However, currently, when runtime PM is disabled,
> operations such as 'get' will also fail even if the device is
> currently active.
> 
> Since calling 'get' on a device that is already RPM_ACTIVE does not
> involve calling the callbacks, it should be allowed to succeed, even
> if runtime PM is disabled.
> 
> This is particularily useful in runtime PM enabled drivers that are
> used during system suspend.  Because runtime PM is disabled during
> system suspend, currently any driver's use of pm_runtime_get* will
> fail with -EACCES.  This is expected if the device was already runtime
> suspended, but if the device is actually active (due to recent usage,
> autosuspend timeout not expired, or pm_runtime_resume() called in
> ->suspend() method), the pm_runtime_get*() call should actually
> succeed.

I'd say the problem is when the drive in question uses the return value of
pm_runtime_get_sync(), for example, to decide whether or not it is safe
to access the hardware.  In that case it may decide that accessing the
hardware is unsafe during system suspend, although that's not really the
case.  So the change you're proposing allows drivers of this kind  (and there
may be a substantial number of them) to be simplified slightly.

> To permit the usage described above, add a check to rpm_resume() so
> that success is returned in the case where a driver is suspended (it's
> ->suspend callback has been called) but is still RPM_ACTIVE.
> 
> This patch was developed in close collaboration with Rafael J. Wysocki
> <rjw@sisk.pl>
> 
> Tested on AM3730/Beagle-xM where 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>
> Signed-off-by: Kevin Hilman <khilman@ti.com>

OK, I wonder if anyone has any objections against this.  Alan?

Rafael


> ---
> This version is just a resend with the vger address for linux-pm.
> 
>  drivers/base/power/runtime.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 7d9c1cb..dafa5ec 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -510,7 +510,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
>  	if (dev->power.runtime_error)
>  		retval = -EINVAL;
>  	else if (dev->power.disable_depth > 0)
> -		retval = -EACCES;
> +		retval = dev->power.is_suspended && 
> +			dev->power.runtime_status == RPM_ACTIVE ? 1 : -EACCES;
>  	if (retval)
>  		goto out;
>  
> 

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

[Alan Stern]

On Thu, 20 Sep 2012, Rafael J. Wysocki wrote:

> On Thursday, September 20, 2012, Kevin Hilman wrote:
> > From: Kevin Hilman <khilman@ti.com>
> > 
> > When runtime PM is disabled, what we want is for callbacks not to be
> > called from then on.  However, currently, when runtime PM is disabled,
> > operations such as 'get' will also fail even if the device is
> > currently active.
> > 
> > Since calling 'get' on a device that is already RPM_ACTIVE does not
> > involve calling the callbacks, it should be allowed to succeed, even
> > if runtime PM is disabled.
> > 
> > This is particularily useful in runtime PM enabled drivers that are
> > used during system suspend.  Because runtime PM is disabled during
> > system suspend, currently any driver's use of pm_runtime_get* will
> > fail with -EACCES.  This is expected if the device was already runtime
> > suspended, but if the device is actually active (due to recent usage,
> > autosuspend timeout not expired, or pm_runtime_resume() called in
> > ->suspend() method), the pm_runtime_get*() call should actually
> > succeed.
> 
> I'd say the problem is when the drive in question uses the return value of
> pm_runtime_get_sync(), for example, to decide whether or not it is safe
> to access the hardware.  In that case it may decide that accessing the
> hardware is unsafe during system suspend, although that's not really the
> case.  So the change you're proposing allows drivers of this kind  (and there
> may be a substantial number of them) to be simplified slightly.

Kevin makes a good case that pm_runtime_resume() and related functions 
should succeed even when runtime PM is disabled, if the device is 
already in the desired state.

The same may be true for pm_runtime_suspend().  What do you think?

> > To permit the usage described above, add a check to rpm_resume() so
> > that success is returned in the case where a driver is suspended (it's
> > ->suspend callback has been called) but is still RPM_ACTIVE.
> > 
> > This patch was developed in close collaboration with Rafael J. Wysocki
> > <rjw@sisk.pl>
> > 
> > Tested on AM3730/Beagle-xM where 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>
> > Signed-off-by: Kevin Hilman <khilman@ti.com>
> 
> OK, I wonder if anyone has any objections against this.  Alan?

The way the patch is written contradicts the documentation:

  * A request to execute ->runtime_resume() will cancel any pending or
    scheduled requests to execute the other callbacks for the same device,
    except for scheduled autosuspends.

> > @@ -510,7 +510,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
> >  	if (dev->power.runtime_error)
> >  		retval = -EINVAL;
> >  	else if (dev->power.disable_depth > 0)
> > -		retval = -EACCES;
> > +		retval = dev->power.is_suspended && 
> > +			dev->power.runtime_status == RPM_ACTIVE ? 1 : -EACCES;
> >  	if (retval)
> >  		goto out;

Also, the is_suspended test seems irrelevant in general -- it makes 
sense in terms of the scenario Kevin described but that's not the 
stated purpose of the patch.

Both of these problems can be addressed by writing the code as follows:

	if (dev->power.runtime_error)
		retval = -EINVAL;
	else if (dev->power.disable_depth > 0) {

		/* Special case: allow this if the device is already active */
		if (dev->power.runtime_status != RPM_ACTIVE)
			retval = -EACCES;
	}
	if (retval)
		goto out;

Alan Stern

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

[Rafael J. Wysocki]

On Friday, September 21, 2012, Alan Stern wrote:
> On Thu, 20 Sep 2012, Rafael J. Wysocki wrote:
> 
> > On Thursday, September 20, 2012, Kevin Hilman wrote:
> > > From: Kevin Hilman <khilman@ti.com>
> > > 
> > > When runtime PM is disabled, what we want is for callbacks not to be
> > > called from then on.  However, currently, when runtime PM is disabled,
> > > operations such as 'get' will also fail even if the device is
> > > currently active.
> > > 
> > > Since calling 'get' on a device that is already RPM_ACTIVE does not
> > > involve calling the callbacks, it should be allowed to succeed, even
> > > if runtime PM is disabled.
> > > 
> > > This is particularily useful in runtime PM enabled drivers that are
> > > used during system suspend.  Because runtime PM is disabled during
> > > system suspend, currently any driver's use of pm_runtime_get* will
> > > fail with -EACCES.  This is expected if the device was already runtime
> > > suspended, but if the device is actually active (due to recent usage,
> > > autosuspend timeout not expired, or pm_runtime_resume() called in
> > > ->suspend() method), the pm_runtime_get*() call should actually
> > > succeed.
> > 
> > I'd say the problem is when the drive in question uses the return value of
> > pm_runtime_get_sync(), for example, to decide whether or not it is safe
> > to access the hardware.  In that case it may decide that accessing the
> > hardware is unsafe during system suspend, although that's not really the
> > case.  So the change you're proposing allows drivers of this kind  (and there
> > may be a substantial number of them) to be simplified slightly.
> 
> Kevin makes a good case that pm_runtime_resume() and related functions 
> should succeed even when runtime PM is disabled, if the device is 
> already in the desired state.
> 
> The same may be true for pm_runtime_suspend().  What do you think?

I've discussed that with Kevin.  The problem is that the runtime PM
status may be changed at will when runtime PM is disabled by using
__pm_runtime_set_status(), so the status generally cannod be trusted
if power.disable_depth > 0.

During system suspend, however, runtime PM is disabled by the core and
if neither the driver nor the subsystem has disabled it in the meantime,
the status should be actually valid.

> > > To permit the usage described above, add a check to rpm_resume() so
> > > that success is returned in the case where a driver is suspended (it's
> > > ->suspend callback has been called) but is still RPM_ACTIVE.
> > > 
> > > This patch was developed in close collaboration with Rafael J. Wysocki
> > > <rjw@sisk.pl>
> > > 
> > > Tested on AM3730/Beagle-xM where 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>
> > > Signed-off-by: Kevin Hilman <khilman@ti.com>
> > 
> > OK, I wonder if anyone has any objections against this.  Alan?
> 
> The way the patch is written contradicts the documentation:
> 
>   * A request to execute ->runtime_resume() will cancel any pending or
>     scheduled requests to execute the other callbacks for the same device,
>     except for scheduled autosuspends.

I'm not sure where the contradiction is.  The patch simply changes the
behavior for disabled runtime PM, which is to return a nonzero value immediately
anyway.

> > > @@ -510,7 +510,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
> > >  	if (dev->power.runtime_error)
> > >  		retval = -EINVAL;
> > >  	else if (dev->power.disable_depth > 0)
> > > -		retval = -EACCES;
> > > +		retval = dev->power.is_suspended && 
> > > +			dev->power.runtime_status == RPM_ACTIVE ? 1 : -EACCES;
> > >  	if (retval)
> > >  		goto out;
> 
> Also, the is_suspended test seems irrelevant in general -- it makes 
> sense in terms of the scenario Kevin described but that's not the 
> stated purpose of the patch.

Well, it is, although the changelog doesn't state it sufficiently clearly. :-)

It fact, it is an optimization, because the driver can have a "suspended"
flag that will be set in its .suspend() routine and then do something like:

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

but if the above patch is applied, the driver won't need to do that (and
that's something multiple drivers may need to do, so it seems to be worth
optimizing).

> Both of these problems can be addressed by writing the code as follows:
> 
> 	if (dev->power.runtime_error)
> 		retval = -EINVAL;
> 	else if (dev->power.disable_depth > 0) {
> 
> 		/* Special case: allow this if the device is already active */
> 		if (dev->power.runtime_status != RPM_ACTIVE)
> 			retval = -EACCES;
> 	}
> 	if (retval)
> 		goto out;

We could do that too, but I'm a bit concerned about the situations where
runtime PM is disabled by the driver itself or by the subsystem, because
in those cases whoever disables runtime PM would have to make sure that the
status still reflects the actual hardware state, but that's what the runtime
PM framework is for (among other things).

Thanks,
Rafael

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

[Alan Stern]

On Fri, 21 Sep 2012, Rafael J. Wysocki wrote:

> > Kevin makes a good case that pm_runtime_resume() and related functions 
> > should succeed even when runtime PM is disabled, if the device is 
> > already in the desired state.
> > 
> > The same may be true for pm_runtime_suspend().  What do you think?
> 
> I've discussed that with Kevin.  The problem is that the runtime PM
> status may be changed at will when runtime PM is disabled by using
> __pm_runtime_set_status(), so the status generally cannod be trusted
> if power.disable_depth > 0.

Hmmm.  That same argument applies even when is_suspended is true.  
Runtime PM might have been disabled beforehand by the driver, so you 
still don't know whether to believe the status.

> During system suspend, however, runtime PM is disabled by the core and
> if neither the driver nor the subsystem has disabled it in the meantime,
> the status should be actually valid.

I suppose you could check that .disable_depth == 1.  That would mean 
only the core had disabled runtime PM.

> > The way the patch is written contradicts the documentation:
> > 
> >   * A request to execute ->runtime_resume() will cancel any pending or
> >     scheduled requests to execute the other callbacks for the same device,
> >     except for scheduled autosuspends.
> 
> I'm not sure where the contradiction is.  The patch simply changes the
> behavior for disabled runtime PM, which is to return a nonzero value immediately
> anyway.

It changes an error return to a non-error return.

However, if we limit the effects to times when the system is 
suspending then there shouldn't be any pending or scheduled requests 
anyway.  So okay, this isn't an issue.

> > > > @@ -510,7 +510,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
> > > >  	if (dev->power.runtime_error)
> > > >  		retval = -EINVAL;
> > > >  	else if (dev->power.disable_depth > 0)
> > > > -		retval = -EACCES;
> > > > +		retval = dev->power.is_suspended && 
> > > > +			dev->power.runtime_status == RPM_ACTIVE ? 1 : -EACCES;
> > > >  	if (retval)
> > > >  		goto out;
> > 
> > Also, the is_suspended test seems irrelevant in general -- it makes 
> > sense in terms of the scenario Kevin described but that's not the 
> > stated purpose of the patch.
> 
> Well, it is, although the changelog doesn't state it sufficiently clearly. :-)

Good point.  The changelog needs to be improved.

> > Both of these problems can be addressed by writing the code as follows:
> > 
> > 	if (dev->power.runtime_error)
> > 		retval = -EINVAL;
> > 	else if (dev->power.disable_depth > 0) {
> > 
> > 		/* Special case: allow this if the device is already active */
> > 		if (dev->power.runtime_status != RPM_ACTIVE)
> > 			retval = -EACCES;
> > 	}
> > 	if (retval)
> > 		goto out;
> 
> We could do that too, but I'm a bit concerned about the situations where
> runtime PM is disabled by the driver itself or by the subsystem, because
> in those cases whoever disables runtime PM would have to make sure that the
> status still reflects the actual hardware state, but that's what the runtime
> PM framework is for (among other things).

All right, let's use Kevin's original scheme but add a test for 
disable_depth == 1.  I suggest changing the ?: operator to a regular 
"if" statement, because the new condition will be even longer than the 
old one (which I found a little hard to read in the first place).

And of course, a comment should be added to explain the reason for the
exception.

Kevin, how does this sound?

Alan Stern

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

[Kevin Hilman]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Fri, 21 Sep 2012, Rafael J. Wysocki wrote:
>
>> > Kevin makes a good case that pm_runtime_resume() and related functions 
>> > should succeed even when runtime PM is disabled, if the device is 
>> > already in the desired state.
>> > 
>> > The same may be true for pm_runtime_suspend().  What do you think?
>> 
>> I've discussed that with Kevin.  The problem is that the runtime PM
>> status may be changed at will when runtime PM is disabled by using
>> __pm_runtime_set_status(), so the status generally cannod be trusted
>> if power.disable_depth > 0.
>
> Hmmm.  That same argument applies even when is_suspended is true.  
> Runtime PM might have been disabled beforehand by the driver, so you 
> still don't know whether to believe the status.
>
>> During system suspend, however, runtime PM is disabled by the core and
>> if neither the driver nor the subsystem has disabled it in the meantime,
>> the status should be actually valid.
>
> I suppose you could check that .disable_depth == 1.  That would mean 
> only the core had disabled runtime PM.
>
>> > The way the patch is written contradicts the documentation:
>> > 
>> >   * A request to execute ->runtime_resume() will cancel any pending or
>> >     scheduled requests to execute the other callbacks for the same device,
>> >     except for scheduled autosuspends.
>> 
>> I'm not sure where the contradiction is.  The patch simply changes the
>> behavior for disabled runtime PM, which is to return a nonzero value immediately
>> anyway.
>
> It changes an error return to a non-error return.
>
> However, if we limit the effects to times when the system is 
> suspending then there shouldn't be any pending or scheduled requests 
> anyway.  So okay, this isn't an issue.
>
>> > > > @@ -510,7 +510,8 @@ static int rpm_resume(struct device *dev, int rpmflags)
>> > > >  	if (dev->power.runtime_error)
>> > > >  		retval = -EINVAL;
>> > > >  	else if (dev->power.disable_depth > 0)
>> > > > -		retval = -EACCES;
>> > > > +		retval = dev->power.is_suspended && 
>> > > > +			dev->power.runtime_status == RPM_ACTIVE ? 1 : -EACCES;
>> > > >  	if (retval)
>> > > >  		goto out;
>> > 
>> > Also, the is_suspended test seems irrelevant in general -- it makes 
>> > sense in terms of the scenario Kevin described but that's not the 
>> > stated purpose of the patch.
>> 
>> Well, it is, although the changelog doesn't state it sufficiently clearly. :-)
>
> Good point.  The changelog needs to be improved.
>
>> > Both of these problems can be addressed by writing the code as follows:
>> > 
>> > 	if (dev->power.runtime_error)
>> > 		retval = -EINVAL;
>> > 	else if (dev->power.disable_depth > 0) {
>> > 
>> > 		/* Special case: allow this if the device is already active */
>> > 		if (dev->power.runtime_status != RPM_ACTIVE)
>> > 			retval = -EACCES;
>> > 	}
>> > 	if (retval)
>> > 		goto out;
>> 
>> We could do that too, but I'm a bit concerned about the situations where
>> runtime PM is disabled by the driver itself or by the subsystem, because
>> in those cases whoever disables runtime PM would have to make sure that the
>> status still reflects the actual hardware state, but that's what the runtime
>> PM framework is for (among other things).
>
> All right, let's use Kevin's original scheme but add a test for 
> disable_depth == 1.  I suggest changing the ?: operator to a regular 
> "if" statement, because the new condition will be even longer than the 
> old one (which I found a little hard to read in the first place).
>
> And of course, a comment should be added to explain the reason for the
> exception.
>
> Kevin, how does this sound?
>

Sounds good to me.

I'll respin and try to make the changelog more clear.

Thanks,

Kevin