pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Rabin Vincent]

Hello,

If an i2c driver uses dev_pm_ops and pm_runtime_suspended() returns true
for the device,  the i2c core will not call the driver's pm->suspend()
routine.  Similar behaviour (except for the if dev_pm_ops check) is
present in the generic PM ops provided in
drivers/base/power/generic_ops.c.

Since pm_runtime_suspended() returns true if the relevant driver did not
call any pm_runtime functions, this means that any driver which does not
use pm_runtime APIs will not get its pm->suspend() callback called
during system sleep, if CONFIG_PM_RUNTIME is enabled.

For the i2c case, there are several such drivers (in drivers/input/*,
etc) lacking these calls.  How is this to be handled?  Do all of these
drivers need to be patched to use the pm_runtime API if they are to be
used on a kernel with PM_RUNTIME enabled?

thanks,
Rabin

Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Alan Stern]

On Tue, 14 Dec 2010, Rabin Vincent wrote:

> Hello,
> 
> If an i2c driver uses dev_pm_ops and pm_runtime_suspended() returns true
> for the device,  the i2c core will not call the driver's pm->suspend()
> routine.  Similar behaviour (except for the if dev_pm_ops check) is
> present in the generic PM ops provided in
> drivers/base/power/generic_ops.c.
> 
> Since pm_runtime_suspended() returns true if the relevant driver did not
> call any pm_runtime functions, this means that any driver which does not
> use pm_runtime APIs will not get its pm->suspend() callback called
> during system sleep, if CONFIG_PM_RUNTIME is enabled.
> 
> For the i2c case, there are several such drivers (in drivers/input/*,
> etc) lacking these calls.  How is this to be handled?  Do all of these
> drivers need to be patched to use the pm_runtime API if they are to be
> used on a kernel with PM_RUNTIME enabled?

I'm not familiar with the details of how the i2c subsystem works.  But
in general, the subsystem code should call pm_runtime_set_active()  
for every device before registering it.  Then if a driver doesn't use
any runtime-PM functions, pm_runtime_suspended() will return false.

Alan Stern

Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Mark Brown]

On Tue, Dec 14, 2010 at 11:16:45AM -0500, Alan Stern wrote:

> I'm not familiar with the details of how the i2c subsystem works.  But
> in general, the subsystem code should call pm_runtime_set_active()  
> for every device before registering it.  Then if a driver doesn't use
> any runtime-PM functions, pm_runtime_suspended() will return false.

Hrm, if that's the case then we also need to update at least the
platform and SPI buses.  Though looking at the documentation this is
going to get a bit interesting as there's a requirement that the parent
has runtime PM enabled on it...  It's certainly not terribly apparent
from the documentation.

It'd be really helpful if it were clearer what noop buses like this were
supposed to do to get runtime PM working.

Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Alan Stern]

On Tue, 14 Dec 2010, Mark Brown wrote:

> On Tue, Dec 14, 2010 at 11:16:45AM -0500, Alan Stern wrote:
> 
> > I'm not familiar with the details of how the i2c subsystem works.  But
> > in general, the subsystem code should call pm_runtime_set_active()  
> > for every device before registering it.  Then if a driver doesn't use
> > any runtime-PM functions, pm_runtime_suspended() will return false.
> 
> Hrm, if that's the case then we also need to update at least the
> platform and SPI buses.  Though looking at the documentation this is
> going to get a bit interesting as there's a requirement that the parent
> has runtime PM enabled on it...

The parent can either be set to the active state or set to ignore its 
children.  The parent does not have to be enabled for runtime PM.

>  It's certainly not terribly apparent
> from the documentation.

What part isn't clear from the documentation?  I think the description 
of pm_runtime_set_active() in Documentation/power/runtime_pm.txt is 
pretty straightforward.

> It'd be really helpful if it were clearer what noop buses like this were
> supposed to do to get runtime PM working.

I'm a little confused.  When you say this is a "noop" bus, do you mean
that it can't do any power management?  If so, why does it need to get
runtime PM working?

Alan Stern

Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Mark Brown]

On Tue, Dec 14, 2010 at 12:44:24PM -0500, Alan Stern wrote:
> On Tue, 14 Dec 2010, Mark Brown wrote:
> > On Tue, Dec 14, 2010 at 11:16:45AM -0500, Alan Stern wrote:

> > > I'm not familiar with the details of how the i2c subsystem works.  But
> > > in general, the subsystem code should call pm_runtime_set_active()  
> > > for every device before registering it.  Then if a driver doesn't use
> > > any runtime-PM functions, pm_runtime_suspended() will return false.

> > Hrm, if that's the case then we also need to update at least the
> > platform and SPI buses.  Though looking at the documentation this is
> > going to get a bit interesting as there's a requirement that the parent
> > has runtime PM enabled on it...

> The parent can either be set to the active state or set to ignore its 
> children.  The parent does not have to be enabled for runtime PM.

Both of those require that the parent is set up to know something about
runtime PM to some extent - in the case of buses like I2C the parent is
a largely unrelated thing on a different bus which may or may not have
runtime PM implemented.

> >  It's certainly not terribly apparent
> > from the documentation.

> What part isn't clear from the documentation?  I think the description 
> of pm_runtime_set_active() in Documentation/power/runtime_pm.txt is 
> pretty straightforward.

It's not clear to me that one needs to do this in order to avoid
breaking the suspend and resume calls of drivers that aren't doing
anything with runtime PM.  It's clear what it does, but it's less clear
that the bus should do it or that not doing it will have an impact on
stuff that isn't using runtime PM.

> > It'd be really helpful if it were clearer what noop buses like this were
> > supposed to do to get runtime PM working.

> I'm a little confused.  When you say this is a "noop" bus, do you mean
> that it can't do any power management?  If so, why does it need to get
> runtime PM working?

The bus as a whole may not be able to do anything useful with runtime PM
but individual devices on it may be able to do so - for example, a multi
function device provides a parent device and a bunch of children for
that device.  Runtime PM provides a nice way for the children to
individually suspend themselves and let the parent implement extra power
savings if all the children suspend.  It also provides a userspace API
for controlling runtime suspend behaviour which drivers may find useful,
and stuff like the autosuspend delays might be useful to some.

Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Alan Stern]

On Tue, 14 Dec 2010, Mark Brown wrote:

> On Tue, Dec 14, 2010 at 12:44:24PM -0500, Alan Stern wrote:
> > On Tue, 14 Dec 2010, Mark Brown wrote:
> > > On Tue, Dec 14, 2010 at 11:16:45AM -0500, Alan Stern wrote:
> 
> > > > I'm not familiar with the details of how the i2c subsystem works.  But
> > > > in general, the subsystem code should call pm_runtime_set_active()  
> > > > for every device before registering it.  Then if a driver doesn't use
> > > > any runtime-PM functions, pm_runtime_suspended() will return false.
> 
> > > Hrm, if that's the case then we also need to update at least the
> > > platform and SPI buses.  Though looking at the documentation this is
> > > going to get a bit interesting as there's a requirement that the parent
> > > has runtime PM enabled on it...
> 
> > The parent can either be set to the active state or set to ignore its 
> > children.  The parent does not have to be enabled for runtime PM.
> 
> Both of those require that the parent is set up to know something about
> runtime PM to some extent - in the case of buses like I2C the parent is
> a largely unrelated thing on a different bus which may or may not have
> runtime PM implemented.
> 
> > >  It's certainly not terribly apparent
> > > from the documentation.
> 
> > What part isn't clear from the documentation?  I think the description 
> > of pm_runtime_set_active() in Documentation/power/runtime_pm.txt is 
> > pretty straightforward.
> 
> It's not clear to me that one needs to do this in order to avoid
> breaking the suspend and resume calls of drivers that aren't doing
> anything with runtime PM.  It's clear what it does, but it's less clear
> that the bus should do it or that not doing it will have an impact on
> stuff that isn't using runtime PM.
> 
> > > It'd be really helpful if it were clearer what noop buses like this were
> > > supposed to do to get runtime PM working.
> 
> > I'm a little confused.  When you say this is a "noop" bus, do you mean
> > that it can't do any power management?  If so, why does it need to get
> > runtime PM working?
> 
> The bus as a whole may not be able to do anything useful with runtime PM
> but individual devices on it may be able to do so - for example, a multi
> function device provides a parent device and a bunch of children for
> that device.  Runtime PM provides a nice way for the children to
> individually suspend themselves and let the parent implement extra power
> savings if all the children suspend.  It also provides a userspace API
> for controlling runtime suspend behaviour which drivers may find useful,
> and stuff like the autosuspend delays might be useful to some.

I see.  It sounds like you're really saying that new devices default to 
the wrong runtime state.  If pm_runtime_init() would set new devices to 
RPM_ACTIVE instead of RPM_SUSPENDED then this problem wouldn't arise.  
Children could do whatever they want, and even if the parent's driver 
was totally ignorant of runtime PM, it would work out.

Alan Stern

Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Mark Brown]

On Tue, Dec 14, 2010 at 02:10:58PM -0500, Alan Stern wrote:

> I see.  It sounds like you're really saying that new devices default to 
> the wrong runtime state.  If pm_runtime_init() would set new devices to 
> RPM_ACTIVE instead of RPM_SUSPENDED then this problem wouldn't arise.  
> Children could do whatever they want, and even if the parent's driver 
> was totally ignorant of runtime PM, it would work out.

In this case that's the desired behaviour but I worry that it'd break
some other use case (eg, keeping a parent awake because there was an
unclaimed device sitting around).  

When I've been working with the runtime PM subsystem before I've thought
that it might be nice to have a specific RPM_UNINITIAZLIZED state which
would generally get out of the way.  It might be a bit clearer than the
current situation conceptually but I've never felt I had a firm enough
grasp on what was going on to really say, everything always feels more
complicated than I feel it should if I understood it properly.

Thinking about it in the case of I2C we probably also want to mark the
device as ignoring its children when it registers as an I2C controller,
while an I2C controller that does runtime PM will probably figure this
out for itself it would save a bit of effort.

Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Alan Stern]

On Tue, 14 Dec 2010, Mark Brown wrote:

> On Tue, Dec 14, 2010 at 02:10:58PM -0500, Alan Stern wrote:
> 
> > I see.  It sounds like you're really saying that new devices default to 
> > the wrong runtime state.  If pm_runtime_init() would set new devices to 
> > RPM_ACTIVE instead of RPM_SUSPENDED then this problem wouldn't arise.  
> > Children could do whatever they want, and even if the parent's driver 
> > was totally ignorant of runtime PM, it would work out.
> 
> In this case that's the desired behaviour but I worry that it'd break
> some other use case (eg, keeping a parent awake because there was an
> unclaimed device sitting around).  
> 
> When I've been working with the runtime PM subsystem before I've thought
> that it might be nice to have a specific RPM_UNINITIAZLIZED state which
> would generally get out of the way.  It might be a bit clearer than the
> current situation conceptually but I've never felt I had a firm enough
> grasp on what was going on to really say, everything always feels more
> complicated than I feel it should if I understood it properly.

That's an interesting suggestion.  In general the PM core can't tell 
what power state a device is really in when it is first discovered and 
registered.

I don't know if it would really solve your problem, though.  What we 
really need is a better way to tell when a device shouldn't prevent its 
parent from suspending.  Something like: If a device has no driver and 
no children, it should automatically be put in the RPM_SUSPENDED state.

> Thinking about it in the case of I2C we probably also want to mark the
> device as ignoring its children when it registers as an I2C controller,
> while an I2C controller that does runtime PM will probably figure this
> out for itself it would save a bit of effort.

That might work, at least for your purposes.

Alan Stern

Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Mark Brown]

On Tue, Dec 14, 2010 at 06:03:06PM -0500, Alan Stern wrote:
> On Tue, 14 Dec 2010, Mark Brown wrote:

> > When I've been working with the runtime PM subsystem before I've thought
> > that it might be nice to have a specific RPM_UNINITIAZLIZED state which
> > would generally get out of the way.  It might be a bit clearer than the

> That's an interesting suggestion.  In general the PM core can't tell 
> what power state a device is really in when it is first discovered and 
> registered.

> I don't know if it would really solve your problem, though.  What we 
> really need is a better way to tell when a device shouldn't prevent its 
> parent from suspending.  Something like: If a device has no driver and 
> no children, it should automatically be put in the RPM_SUSPENDED state.

Yes, there'd need to be a bunch of code implementing behaviours that
look like what you suggest above and I'm not clear if it'd be worth the
hassle of implementing it - like I say, I'm not generally comfortable
enough with my understanding in this area to have a strong opinion on
the best approach.

Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Rafael J. Wysocki]

On Tuesday, December 14, 2010, Alan Stern wrote:
> On Tue, 14 Dec 2010, Mark Brown wrote:
> 
> > On Tue, Dec 14, 2010 at 12:44:24PM -0500, Alan Stern wrote:
> > > On Tue, 14 Dec 2010, Mark Brown wrote:
> > > > On Tue, Dec 14, 2010 at 11:16:45AM -0500, Alan Stern wrote:
> > 
> > > > > I'm not familiar with the details of how the i2c subsystem works.  But
> > > > > in general, the subsystem code should call pm_runtime_set_active()  
> > > > > for every device before registering it.  Then if a driver doesn't use
> > > > > any runtime-PM functions, pm_runtime_suspended() will return false.
> > 
> > > > Hrm, if that's the case then we also need to update at least the
> > > > platform and SPI buses.  Though looking at the documentation this is
> > > > going to get a bit interesting as there's a requirement that the parent
> > > > has runtime PM enabled on it...
> > 
> > > The parent can either be set to the active state or set to ignore its 
> > > children.  The parent does not have to be enabled for runtime PM.
> > 
> > Both of those require that the parent is set up to know something about
> > runtime PM to some extent - in the case of buses like I2C the parent is
> > a largely unrelated thing on a different bus which may or may not have
> > runtime PM implemented.
> > 
> > > >  It's certainly not terribly apparent
> > > > from the documentation.
> > 
> > > What part isn't clear from the documentation?  I think the description 
> > > of pm_runtime_set_active() in Documentation/power/runtime_pm.txt is 
> > > pretty straightforward.
> > 
> > It's not clear to me that one needs to do this in order to avoid
> > breaking the suspend and resume calls of drivers that aren't doing
> > anything with runtime PM.  It's clear what it does, but it's less clear
> > that the bus should do it or that not doing it will have an impact on
> > stuff that isn't using runtime PM.
> > 
> > > > It'd be really helpful if it were clearer what noop buses like this were
> > > > supposed to do to get runtime PM working.
> > 
> > > I'm a little confused.  When you say this is a "noop" bus, do you mean
> > > that it can't do any power management?  If so, why does it need to get
> > > runtime PM working?
> > 
> > The bus as a whole may not be able to do anything useful with runtime PM
> > but individual devices on it may be able to do so - for example, a multi
> > function device provides a parent device and a bunch of children for
> > that device.  Runtime PM provides a nice way for the children to
> > individually suspend themselves and let the parent implement extra power
> > savings if all the children suspend.  It also provides a userspace API
> > for controlling runtime suspend behaviour which drivers may find useful,
> > and stuff like the autosuspend delays might be useful to some.
> 
> I see.  It sounds like you're really saying that new devices default to 
> the wrong runtime state.  If pm_runtime_init() would set new devices to 
> RPM_ACTIVE instead of RPM_SUSPENDED then this problem wouldn't arise.  
> Children could do whatever they want, and even if the parent's driver 
> was totally ignorant of runtime PM, it would work out.

Actaully, pm_runtime_init() also sets power.disable_depth to 1, which disables
runtime PM and (with the pm_runtime_suspended() change I proposed earlier)
that should be sufficient, unless someone blindly calls pm_runtime_enable()
for the device without changing its status as appropriate beforehand.  That,
however, is a bug in the code doing this IMnsHO.

Thanks,
Rafael

Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Rafael J. Wysocki]

On Tuesday, December 14, 2010, Alan Stern wrote:
> On Tue, 14 Dec 2010, Rabin Vincent wrote:
> 
> > Hello,
> > 
> > If an i2c driver uses dev_pm_ops and pm_runtime_suspended() returns true
> > for the device,  the i2c core will not call the driver's pm->suspend()
> > routine.  Similar behaviour (except for the if dev_pm_ops check) is
> > present in the generic PM ops provided in
> > drivers/base/power/generic_ops.c.
> > 
> > Since pm_runtime_suspended() returns true if the relevant driver did not
> > call any pm_runtime functions, this means that any driver which does not
> > use pm_runtime APIs will not get its pm->suspend() callback called
> > during system sleep, if CONFIG_PM_RUNTIME is enabled.
> > 
> > For the i2c case, there are several such drivers (in drivers/input/*,
> > etc) lacking these calls.  How is this to be handled?  Do all of these
> > drivers need to be patched to use the pm_runtime API if they are to be
> > used on a kernel with PM_RUNTIME enabled?
> 
> I'm not familiar with the details of how the i2c subsystem works.  But
> in general, the subsystem code should call pm_runtime_set_active()  
> for every device before registering it.  Then if a driver doesn't use
> any runtime-PM functions, pm_runtime_suspended() will return false.

I rather think that our current definition of pm_runtime_suspended() is
not really adequate.

Namely, it shouldn't really return true if runtime PM is not enabled
(ie. power.disable_depth > 1).  That change would fix the issue at hand,
wouldn't it?

Rafael

Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Rafael J. Wysocki]

On Wednesday, December 15, 2010, Rafael J. Wysocki wrote:
> On Tuesday, December 14, 2010, Alan Stern wrote:
> > On Tue, 14 Dec 2010, Rabin Vincent wrote:
> > 
> > > Hello,
> > > 
> > > If an i2c driver uses dev_pm_ops and pm_runtime_suspended() returns true
> > > for the device,  the i2c core will not call the driver's pm->suspend()
> > > routine.  Similar behaviour (except for the if dev_pm_ops check) is
> > > present in the generic PM ops provided in
> > > drivers/base/power/generic_ops.c.
> > > 
> > > Since pm_runtime_suspended() returns true if the relevant driver did not
> > > call any pm_runtime functions, this means that any driver which does not
> > > use pm_runtime APIs will not get its pm->suspend() callback called
> > > during system sleep, if CONFIG_PM_RUNTIME is enabled.
> > > 
> > > For the i2c case, there are several such drivers (in drivers/input/*,
> > > etc) lacking these calls.  How is this to be handled?  Do all of these
> > > drivers need to be patched to use the pm_runtime API if they are to be
> > > used on a kernel with PM_RUNTIME enabled?
> > 
> > I'm not familiar with the details of how the i2c subsystem works.  But
> > in general, the subsystem code should call pm_runtime_set_active()  
> > for every device before registering it.  Then if a driver doesn't use
> > any runtime-PM functions, pm_runtime_suspended() will return false.
> 
> I rather think that our current definition of pm_runtime_suspended() is
> not really adequate.
> 
> Namely, it shouldn't really return true if runtime PM is not enabled
> (ie. power.disable_depth > 1).

That should have been power.disable_depth > 0, of course.  Sorry.

> That change would fix the issue at hand,  wouldn't it?

Rafael

Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Rafael J. Wysocki]

On Wednesday, December 15, 2010, Rafael J. Wysocki wrote:
> On Wednesday, December 15, 2010, Rafael J. Wysocki wrote:
> > On Tuesday, December 14, 2010, Alan Stern wrote:
> > > On Tue, 14 Dec 2010, Rabin Vincent wrote:
> > > 
> > > > Hello,
> > > > 
> > > > If an i2c driver uses dev_pm_ops and pm_runtime_suspended() returns true
> > > > for the device,  the i2c core will not call the driver's pm->suspend()
> > > > routine.  Similar behaviour (except for the if dev_pm_ops check) is
> > > > present in the generic PM ops provided in
> > > > drivers/base/power/generic_ops.c.
> > > > 
> > > > Since pm_runtime_suspended() returns true if the relevant driver did not
> > > > call any pm_runtime functions, this means that any driver which does not
> > > > use pm_runtime APIs will not get its pm->suspend() callback called
> > > > during system sleep, if CONFIG_PM_RUNTIME is enabled.
> > > > 
> > > > For the i2c case, there are several such drivers (in drivers/input/*,
> > > > etc) lacking these calls.  How is this to be handled?  Do all of these
> > > > drivers need to be patched to use the pm_runtime API if they are to be
> > > > used on a kernel with PM_RUNTIME enabled?
> > > 
> > > I'm not familiar with the details of how the i2c subsystem works.  But
> > > in general, the subsystem code should call pm_runtime_set_active()  
> > > for every device before registering it.  Then if a driver doesn't use
> > > any runtime-PM functions, pm_runtime_suspended() will return false.
> > 
> > I rather think that our current definition of pm_runtime_suspended() is
> > not really adequate.
> > 
> > Namely, it shouldn't really return true if runtime PM is not enabled
> > (ie. power.disable_depth > 1).
> 
> That should have been power.disable_depth > 0, of course.  Sorry.
> 
> > That change would fix the issue at hand,  wouldn't it?

Below is a patch.  I don't think it needs to be done under the lock at
the moment if the callers of pm_runtime_enable() always remember to set
the appropriate device status before calling it.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
Subject: PM / Runtime: Fix pm_runtime_suspended()

pm_runtime_suspended() shouldn't return true if the runtime PM of the
given device is disabled.

Signed-off-by: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
---
 include/linux/pm_runtime.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6/include/linux/pm_runtime.h
===================================================================
--- linux-2.6.orig/include/linux/pm_runtime.h
+++ linux-2.6/include/linux/pm_runtime.h
@@ -78,7 +78,8 @@ static inline void device_set_run_wake(s
 
 static inline bool pm_runtime_suspended(struct device *dev)
 {
-	return dev->power.runtime_status == RPM_SUSPENDED;
+	return dev->power.runtime_status == RPM_SUSPENDED
+		&& !dev->power.disable_depth;
 }
 
 static inline void pm_runtime_mark_last_busy(struct device *dev)

Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Alan Stern]

On Wed, 15 Dec 2010, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> Subject: PM / Runtime: Fix pm_runtime_suspended()
> 
> pm_runtime_suspended() shouldn't return true if the runtime PM of the
> given device is disabled.
> 
> Signed-off-by: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> ---
>  include/linux/pm_runtime.h |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/include/linux/pm_runtime.h
> ===================================================================
> --- linux-2.6.orig/include/linux/pm_runtime.h
> +++ linux-2.6/include/linux/pm_runtime.h
> @@ -78,7 +78,8 @@ static inline void device_set_run_wake(s
>  
>  static inline bool pm_runtime_suspended(struct device *dev)
>  {
> -	return dev->power.runtime_status == RPM_SUSPENDED;
> +	return dev->power.runtime_status == RPM_SUSPENDED
> +		&& !dev->power.disable_depth;
>  }

You need to update the documentation entry for pm_runtime_suspended as 
well.

I think this is okay.  If a driver or subsystem uses
pm_runtime_suspended() then it must be runtime-PM-aware, so it wouldn't
leave a device disabled for runtime PM.

So in theory the only place this would matter is if the function is
called in a generic setting, and AFAICT the only place that happens is
in generic_ops.c, where the change is correct.

Alan Stern

Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Rafael J. Wysocki]

On Wednesday, December 15, 2010, Alan Stern wrote:
> On Wed, 15 Dec 2010, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> > Subject: PM / Runtime: Fix pm_runtime_suspended()
> > 
> > pm_runtime_suspended() shouldn't return true if the runtime PM of the
> > given device is disabled.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> > ---
> >  include/linux/pm_runtime.h |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6/include/linux/pm_runtime.h
> > ===================================================================
> > --- linux-2.6.orig/include/linux/pm_runtime.h
> > +++ linux-2.6/include/linux/pm_runtime.h
> > @@ -78,7 +78,8 @@ static inline void device_set_run_wake(s
> >  
> >  static inline bool pm_runtime_suspended(struct device *dev)
> >  {
> > -	return dev->power.runtime_status == RPM_SUSPENDED;
> > +	return dev->power.runtime_status == RPM_SUSPENDED
> > +		&& !dev->power.disable_depth;
> >  }
> 
> You need to update the documentation entry for pm_runtime_suspended as 
> well.

Yes, I also need to rework the changelog to explain what exactly the problem is.

> I think this is okay.  If a driver or subsystem uses
> pm_runtime_suspended() then it must be runtime-PM-aware, so it wouldn't
> leave a device disabled for runtime PM.
> 
> So in theory the only place this would matter is if the function is
> called in a generic setting, and AFAICT the only place that happens is
> in generic_ops.c, where the change is correct.

Indeed.

Thanks,
Rafael

Re: [linux-pm] pm_runtime_suspended() and non-pm_runtime-using (i2c) drivers

[Rafael J. Wysocki]

On Wednesday, December 15, 2010, Rafael J. Wysocki wrote:
> On Wednesday, December 15, 2010, Alan Stern wrote:
> > On Wed, 15 Dec 2010, Rafael J. Wysocki wrote:
> > 
> > > From: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> > > Subject: PM / Runtime: Fix pm_runtime_suspended()
> > > 
> > > pm_runtime_suspended() shouldn't return true if the runtime PM of the
> > > given device is disabled.
> > > 
> > > Signed-off-by: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
> > > ---
> > >  include/linux/pm_runtime.h |    3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > Index: linux-2.6/include/linux/pm_runtime.h
> > > ===================================================================
> > > --- linux-2.6.orig/include/linux/pm_runtime.h
> > > +++ linux-2.6/include/linux/pm_runtime.h
> > > @@ -78,7 +78,8 @@ static inline void device_set_run_wake(s
> > >  
> > >  static inline bool pm_runtime_suspended(struct device *dev)
> > >  {
> > > -	return dev->power.runtime_status == RPM_SUSPENDED;
> > > +	return dev->power.runtime_status == RPM_SUSPENDED
> > > +		&& !dev->power.disable_depth;
> > >  }
> > 
> > You need to update the documentation entry for pm_runtime_suspended as 
> > well.
> 
> Yes, I also need to rework the changelog to explain what exactly the problem is.

Updated patch is appended.

Thanks,
Rafael

---
From: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
Subject: PM / Runtime: Fix pm_runtime_suspended()

There are some situations (e.g. in __pm_generic_call()), where
pm_runtime_suspended() is used to decide whether or not to execute
a device's (system) ->suspend() callback.  The callback is not
executed if pm_runtime_suspended() returns true, but it does so
for devices that don't even support runtime PM, because the
power.disable_depth device field is ignored by it.  This leads to
problems (i.e. devices are not suspened when they should), so rework
pm_runtime_suspended() so that it returns false if the device's
power.disable_depth field is different from zero.

Signed-off-by: Rafael J. Wysocki <rjw-KKrjLPT3xs0@public.gmane.org>
---
 Documentation/power/runtime_pm.txt |    4 ++--
 include/linux/pm_runtime.h         |    3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

Index: linux-2.6/include/linux/pm_runtime.h
===================================================================
--- linux-2.6.orig/include/linux/pm_runtime.h
+++ linux-2.6/include/linux/pm_runtime.h
@@ -78,7 +78,8 @@ static inline void device_set_run_wake(s
 
 static inline bool pm_runtime_suspended(struct device *dev)
 {
-	return dev->power.runtime_status == RPM_SUSPENDED;
+	return dev->power.runtime_status == RPM_SUSPENDED
+		&& !dev->power.disable_depth;
 }
 
 static inline void pm_runtime_mark_last_busy(struct device *dev)
Index: linux-2.6/Documentation/power/runtime_pm.txt
===================================================================
--- linux-2.6.orig/Documentation/power/runtime_pm.txt
+++ linux-2.6/Documentation/power/runtime_pm.txt
@@ -396,8 +396,8 @@ drivers/base/power/runtime.c and include
       zero)
 
   bool pm_runtime_suspended(struct device *dev);
-    - return true if the device's runtime PM status is 'suspended', or false
-      otherwise
+    - return true if the device's runtime PM status is 'suspended' and its
+      'power.disable_depth' field is equal to zero, or false otherwise
 
   void pm_runtime_allow(struct device *dev);
     - set the power.runtime_auto flag for the device and decrease its usage