[PATCH] PM / Runtime: honor device autosuspend in pm_generic_runtime_idle()

[PATCH] PM / Runtime: honor device autosuspend in pm_generic_runtime_idle()

[Mika Westerberg]

If the device is using autosuspend we should honor that and call
pm_runtime_autosuspend() instead of pm_runtime_suspend(). Failing to do so
causes the device to be suspended immediately even though it expects to be
suspended only when the autosuspend delay is expired.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/base/power/generic_ops.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
index bfd898b..19786ca 100644
--- a/drivers/base/power/generic_ops.c
+++ b/drivers/base/power/generic_ops.c
@@ -29,7 +29,10 @@ int pm_generic_runtime_idle(struct device *dev)
 			return ret;
 	}
 
-	pm_runtime_suspend(dev);
+	if (dev->power.use_autosuspend)
+		pm_runtime_autosuspend(dev);
+	else
+		pm_runtime_suspend(dev);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pm_generic_runtime_idle);
-- 
1.7.10.4

Re: [PATCH] PM / Runtime: honor device autosuspend in pm_generic_runtime_idle()

[Rafael J. Wysocki]

On Monday, May 13, 2013 02:05:27 PM Mika Westerberg wrote:
> If the device is using autosuspend we should honor that and call
> pm_runtime_autosuspend() instead of pm_runtime_suspend(). Failing to do so
> causes the device to be suspended immediately even though it expects to be
> suspended only when the autosuspend delay is expired.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/base/power/generic_ops.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
> index bfd898b..19786ca 100644
> --- a/drivers/base/power/generic_ops.c
> +++ b/drivers/base/power/generic_ops.c
> @@ -29,7 +29,10 @@ int pm_generic_runtime_idle(struct device *dev)
>  			return ret;
>  	}
>  
> -	pm_runtime_suspend(dev);
> +	if (dev->power.use_autosuspend)
> +		pm_runtime_autosuspend(dev);
> +	else
> +		pm_runtime_suspend(dev);
>  	return 0;

First of all, this is racy (power.use_autosuspend shoud be accessed under
power.lock).

Second, this is not the only place we'd need to make this change (the analogous
function for PCI is one example, but there may be others).

Finally, I'm not sure how to address this problem in general.  It may be better
to simply modify rpm_idle() and remove pm_generic_runtime_idle() etc. entirely.

I'll have a look at that, thanks for pointing out the problem.

Rafael


>  }
>  EXPORT_SYMBOL_GPL(pm_generic_runtime_idle);
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] PM / Runtime: honor device autosuspend in pm_generic_runtime_idle()

[Rafael J. Wysocki]

On Monday, May 13, 2013 01:50:18 PM Rafael J. Wysocki wrote:
> On Monday, May 13, 2013 02:05:27 PM Mika Westerberg wrote:
> > If the device is using autosuspend we should honor that and call
> > pm_runtime_autosuspend() instead of pm_runtime_suspend(). Failing to do so
> > causes the device to be suspended immediately even though it expects to be
> > suspended only when the autosuspend delay is expired.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/base/power/generic_ops.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
> > index bfd898b..19786ca 100644
> > --- a/drivers/base/power/generic_ops.c
> > +++ b/drivers/base/power/generic_ops.c
> > @@ -29,7 +29,10 @@ int pm_generic_runtime_idle(struct device *dev)
> >  			return ret;
> >  	}
> >  
> > -	pm_runtime_suspend(dev);
> > +	if (dev->power.use_autosuspend)
> > +		pm_runtime_autosuspend(dev);
> > +	else
> > +		pm_runtime_suspend(dev);
> >  	return 0;
> 
> First of all, this is racy (power.use_autosuspend shoud be accessed under
> power.lock).
> 
> Second, this is not the only place we'd need to make this change (the analogous
> function for PCI is one example, but there may be others).
> 
> Finally, I'm not sure how to address this problem in general.  It may be better
> to simply modify rpm_idle() and remove pm_generic_runtime_idle() etc. entirely.
> 
> I'll have a look at that, thanks for pointing out the problem.

I'm not sure if the core is the right place to address this, because it's
not entirely clear if all drivers using autosuspend will have the same policy
with respect to pm_runtime_idle() (i.e. to avoid suspending immediately if
the suspend delay timer is active).

In my opinion it'd be better to address that in the driver by adding a
.runtime_idle() callback executing pm_runtime_autosuspend(dev) and returning
-EBUSY.

If all (or at least the majority of) drivers using autosuspend end up doing
that, *then* we can move that to the core.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] PM / Runtime: honor device autosuspend in pm_generic_runtime_idle()

[Alan Stern]

On Tue, 28 May 2013, Rafael J. Wysocki wrote:

> On Monday, May 13, 2013 01:50:18 PM Rafael J. Wysocki wrote:
> > On Monday, May 13, 2013 02:05:27 PM Mika Westerberg wrote:
> > > If the device is using autosuspend we should honor that and call
> > > pm_runtime_autosuspend() instead of pm_runtime_suspend(). Failing to do so
> > > causes the device to be suspended immediately even though it expects to be
> > > suspended only when the autosuspend delay is expired.
> > > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > >  drivers/base/power/generic_ops.c |    5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
> > > index bfd898b..19786ca 100644
> > > --- a/drivers/base/power/generic_ops.c
> > > +++ b/drivers/base/power/generic_ops.c
> > > @@ -29,7 +29,10 @@ int pm_generic_runtime_idle(struct device *dev)
> > >  			return ret;
> > >  	}
> > >  
> > > -	pm_runtime_suspend(dev);
> > > +	if (dev->power.use_autosuspend)
> > > +		pm_runtime_autosuspend(dev);
> > > +	else
> > > +		pm_runtime_suspend(dev);
> > >  	return 0;
> > 
> > First of all, this is racy (power.use_autosuspend shoud be accessed under
> > power.lock).
> > 
> > Second, this is not the only place we'd need to make this change (the analogous
> > function for PCI is one example, but there may be others).
> > 
> > Finally, I'm not sure how to address this problem in general.  It may be better
> > to simply modify rpm_idle() and remove pm_generic_runtime_idle() etc. entirely.
> > 
> > I'll have a look at that, thanks for pointing out the problem.
> 
> I'm not sure if the core is the right place to address this, because it's
> not entirely clear if all drivers using autosuspend will have the same policy
> with respect to pm_runtime_idle() (i.e. to avoid suspending immediately if
> the suspend delay timer is active).
> 
> In my opinion it'd be better to address that in the driver by adding a
> .runtime_idle() callback executing pm_runtime_autosuspend(dev) and returning
> -EBUSY.

Remember that the return value from the runtime_idle callback is 
ignored.  Are you suggesting that the PM core should start paying 
attention to it?

Alan Stern

Re: [PATCH] PM / Runtime: honor device autosuspend in pm_generic_runtime_idle()

[Alan Stern]

On Tue, 28 May 2013, Rafael J. Wysocki wrote:

> On Monday, May 13, 2013 01:50:18 PM Rafael J. Wysocki wrote:
> > On Monday, May 13, 2013 02:05:27 PM Mika Westerberg wrote:
> > > If the device is using autosuspend we should honor that and call
> > > pm_runtime_autosuspend() instead of pm_runtime_suspend(). Failing to do so
> > > causes the device to be suspended immediately even though it expects to be
> > > suspended only when the autosuspend delay is expired.
> > > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > >  drivers/base/power/generic_ops.c |    5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
> > > index bfd898b..19786ca 100644
> > > --- a/drivers/base/power/generic_ops.c
> > > +++ b/drivers/base/power/generic_ops.c
> > > @@ -29,7 +29,10 @@ int pm_generic_runtime_idle(struct device *dev)
> > >  			return ret;
> > >  	}
> > >  
> > > -	pm_runtime_suspend(dev);
> > > +	if (dev->power.use_autosuspend)
> > > +		pm_runtime_autosuspend(dev);
> > > +	else
> > > +		pm_runtime_suspend(dev);
> > >  	return 0;
> > 
> > First of all, this is racy (power.use_autosuspend shoud be accessed under
> > power.lock).
> > 
> > Second, this is not the only place we'd need to make this change (the analogous
> > function for PCI is one example, but there may be others).
> > 
> > Finally, I'm not sure how to address this problem in general.  It may be better
> > to simply modify rpm_idle() and remove pm_generic_runtime_idle() etc. entirely.
> > 
> > I'll have a look at that, thanks for pointing out the problem.
> 
> I'm not sure if the core is the right place to address this, because it's
> not entirely clear if all drivers using autosuspend will have the same policy
> with respect to pm_runtime_idle() (i.e. to avoid suspending immediately if
> the suspend delay timer is active).
> 
> In my opinion it'd be better to address that in the driver by adding a
> .runtime_idle() callback executing pm_runtime_autosuspend(dev) and returning
> -EBUSY.

Remember that the return value from the runtime_idle callback is 
ignored.  Are you suggesting that the PM core should start paying 
attention to it?

Alan Stern

Re: [PATCH] PM / Runtime: honor device autosuspend in pm_generic_runtime_idle()

[Rafael J. Wysocki]

On Tuesday, May 28, 2013 10:13:35 AM Alan Stern wrote:
> On Tue, 28 May 2013, Rafael J. Wysocki wrote:
> 
> > On Monday, May 13, 2013 01:50:18 PM Rafael J. Wysocki wrote:
> > > On Monday, May 13, 2013 02:05:27 PM Mika Westerberg wrote:
> > > > If the device is using autosuspend we should honor that and call
> > > > pm_runtime_autosuspend() instead of pm_runtime_suspend(). Failing to do so
> > > > causes the device to be suspended immediately even though it expects to be
> > > > suspended only when the autosuspend delay is expired.
> > > > 
> > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > ---
> > > >  drivers/base/power/generic_ops.c |    5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
> > > > index bfd898b..19786ca 100644
> > > > --- a/drivers/base/power/generic_ops.c
> > > > +++ b/drivers/base/power/generic_ops.c
> > > > @@ -29,7 +29,10 @@ int pm_generic_runtime_idle(struct device *dev)
> > > >  			return ret;
> > > >  	}
> > > >  
> > > > -	pm_runtime_suspend(dev);
> > > > +	if (dev->power.use_autosuspend)
> > > > +		pm_runtime_autosuspend(dev);
> > > > +	else
> > > > +		pm_runtime_suspend(dev);
> > > >  	return 0;
> > > 
> > > First of all, this is racy (power.use_autosuspend shoud be accessed under
> > > power.lock).
> > > 
> > > Second, this is not the only place we'd need to make this change (the analogous
> > > function for PCI is one example, but there may be others).
> > > 
> > > Finally, I'm not sure how to address this problem in general.  It may be better
> > > to simply modify rpm_idle() and remove pm_generic_runtime_idle() etc. entirely.
> > > 
> > > I'll have a look at that, thanks for pointing out the problem.
> > 
> > I'm not sure if the core is the right place to address this, because it's
> > not entirely clear if all drivers using autosuspend will have the same policy
> > with respect to pm_runtime_idle() (i.e. to avoid suspending immediately if
> > the suspend delay timer is active).
> > 
> > In my opinion it'd be better to address that in the driver by adding a
> > .runtime_idle() callback executing pm_runtime_autosuspend(dev) and returning
> > -EBUSY.
> 
> Remember that the return value from the runtime_idle callback is ignored.

It is ignored by the core, but some subsystems (those using
pm_generic_runtime_idle() in particular) take it into account.

> Are you suggesting that the PM core should start paying attention to it?

In fact, I was pondering posting a patch making that change. :-)

Perhaps I'll just post it for discussion later today ...

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH] PM / Runtime: honor device autosuspend in pm_generic_runtime_idle()

[Mika Westerberg]

On Tue, May 28, 2013 at 02:53:47PM +0200, Rafael J. Wysocki wrote:
> On Monday, May 13, 2013 01:50:18 PM Rafael J. Wysocki wrote:
> > On Monday, May 13, 2013 02:05:27 PM Mika Westerberg wrote:
> > > If the device is using autosuspend we should honor that and call
> > > pm_runtime_autosuspend() instead of pm_runtime_suspend(). Failing to do so
> > > causes the device to be suspended immediately even though it expects to be
> > > suspended only when the autosuspend delay is expired.
> > > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > >  drivers/base/power/generic_ops.c |    5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
> > > index bfd898b..19786ca 100644
> > > --- a/drivers/base/power/generic_ops.c
> > > +++ b/drivers/base/power/generic_ops.c
> > > @@ -29,7 +29,10 @@ int pm_generic_runtime_idle(struct device *dev)
> > >  			return ret;
> > >  	}
> > >  
> > > -	pm_runtime_suspend(dev);
> > > +	if (dev->power.use_autosuspend)
> > > +		pm_runtime_autosuspend(dev);
> > > +	else
> > > +		pm_runtime_suspend(dev);
> > >  	return 0;
> > 
> > First of all, this is racy (power.use_autosuspend shoud be accessed under
> > power.lock).
> > 
> > Second, this is not the only place we'd need to make this change (the analogous
> > function for PCI is one example, but there may be others).
> > 
> > Finally, I'm not sure how to address this problem in general.  It may be better
> > to simply modify rpm_idle() and remove pm_generic_runtime_idle() etc. entirely.
> > 
> > I'll have a look at that, thanks for pointing out the problem.
> 
> I'm not sure if the core is the right place to address this, because it's
> not entirely clear if all drivers using autosuspend will have the same policy
> with respect to pm_runtime_idle() (i.e. to avoid suspending immediately if
> the suspend delay timer is active).
> 
> In my opinion it'd be better to address that in the driver by adding a
> .runtime_idle() callback executing pm_runtime_autosuspend(dev) and returning
> -EBUSY.

Makes sense. I did a quick test with following patch and it seems to fix
the issue for i2c-designware driver.

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 966a554..af8be8f 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -267,7 +267,22 @@ static int dw_i2c_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(dw_i2c_dev_pm_ops, dw_i2c_suspend, dw_i2c_resume);
+#ifdef CONFIG_PM_RUNTIME
+static int dw_i2c_runtime_idle(struct device *dev)
+{
+	/*
+	 * Always schedule autosuspend regardless of what runtime PM the
+	 * client devices use.
+	 */
+	pm_runtime_autosuspend(dev);
+	return -EBUSY;
+}
+#endif
+
+static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_suspend, dw_i2c_resume)
+	SET_RUNTIME_PM_OPS(NULL, NULL, dw_i2c_runtime_idle)
+};
 
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:i2c_designware");