[PATCH RFC] PM / Runtime: Rework the "runtime idle" helper routine

[PATCH RFC] PM / Runtime: Rework the "runtime idle" helper routine

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The "runtime idle" helper routine, rpm_idle(), currently ignores
return values from .runtime_idle() callbacks executed by it.

However, it turns out that many subsystems use the generic idle
callback routine pm_generic_runtime_idle() which checks the return
value of the driver's callback and executes pm_runtime_suspend() for
the device unless that value is different from 0.  If that logic is
moved to rpm_idle() instead, pm_generic_runtime_idle() can be dropped
and its users will not need any .runtime_idle() callbacks any more.

Moreover, the PCI subsystem's .runtime_idle() routine,
pci_pm_runtime_idle(), works in analogy with the generic one and if
rpm_idle() calls rpm_suspend() after 0 has been returned by the
.runtime_idle() callback executed by it, that routine will not be
necessary any more and may be dropped.

To reduce overall code duplication make the changes described above.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

The patch doesn't break compilation for me, but it hasn't been tested
otherwise.  It applies on top of the linux-pm.git tree's linux-next branch.

Thanks,
Rafael

---
 Documentation/power/runtime_pm.txt |    5 -----
 arch/arm/mach-omap2/omap_device.c  |    7 +------
 drivers/acpi/device_pm.c           |    1 -
 drivers/amba/bus.c                 |    2 +-
 drivers/base/platform.c            |    1 -
 drivers/base/power/domain.c        |    1 -
 drivers/base/power/generic_ops.c   |   23 -----------------------
 drivers/base/power/runtime.c       |   12 +++++-------
 drivers/i2c/i2c-core.c             |    2 +-
 drivers/mmc/core/sdio_bus.c        |    2 +-
 drivers/pci/pci-driver.c           |   27 ---------------------------
 drivers/spi/spi.c                  |    2 +-
 drivers/usb/core/port.c            |    1 -
 include/linux/pm_runtime.h         |    2 --
 14 files changed, 10 insertions(+), 78 deletions(-)

Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -293,11 +293,8 @@ static int rpm_idle(struct device *dev,
 	/* Pending requests need to be canceled. */
 	dev->power.request = RPM_REQ_NONE;
 
-	if (dev->power.no_callbacks) {
-		/* Assume ->runtime_idle() callback would have suspended. */
-		retval = rpm_suspend(dev, rpmflags);
+	if (dev->power.no_callbacks)
 		goto out;
-	}
 
 	/* Carry out an asynchronous or a synchronous idle notification. */
 	if (rpmflags & RPM_ASYNC) {
@@ -306,7 +303,8 @@ static int rpm_idle(struct device *dev,
 			dev->power.request_pending = true;
 			queue_work(pm_wq, &dev->power.work);
 		}
-		goto out;
+		trace_rpm_return_int(dev, _THIS_IP_, 0);
+		return 0;
 	}
 
 	dev->power.idle_notification = true;
@@ -326,14 +324,14 @@ static int rpm_idle(struct device *dev,
 		callback = dev->driver->pm->runtime_idle;
 
 	if (callback)
-		__rpm_callback(callback, dev);
+		retval = __rpm_callback(callback, dev);
 
 	dev->power.idle_notification = false;
 	wake_up_all(&dev->power.wait_queue);
 
  out:
 	trace_rpm_return_int(dev, _THIS_IP_, retval);
-	return retval;
+	return retval ? retval : rpm_suspend(dev, rpmflags);
 }
 
 /**
Index: linux-pm/drivers/base/power/generic_ops.c
===================================================================
--- linux-pm.orig/drivers/base/power/generic_ops.c
+++ linux-pm/drivers/base/power/generic_ops.c
@@ -12,29 +12,6 @@
 
 #ifdef CONFIG_PM_RUNTIME
 /**
- * pm_generic_runtime_idle - Generic runtime idle callback for subsystems.
- * @dev: Device to handle.
- *
- * If PM operations are defined for the @dev's driver and they include
- * ->runtime_idle(), execute it and return its error code, if nonzero.
- * Otherwise, execute pm_runtime_suspend() for the device and return 0.
- */
-int pm_generic_runtime_idle(struct device *dev)
-{
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-
-	if (pm && pm->runtime_idle) {
-		int ret = pm->runtime_idle(dev);
-		if (ret)
-			return ret;
-	}
-
-	pm_runtime_suspend(dev);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(pm_generic_runtime_idle);
-
-/**
  * pm_generic_runtime_suspend - Generic runtime suspend callback for subsystems.
  * @dev: Device to suspend.
  *
Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -37,7 +37,6 @@ extern void pm_runtime_enable(struct dev
 extern void __pm_runtime_disable(struct device *dev, bool check_resume);
 extern void pm_runtime_allow(struct device *dev);
 extern void pm_runtime_forbid(struct device *dev);
-extern int pm_generic_runtime_idle(struct device *dev);
 extern int pm_generic_runtime_suspend(struct device *dev);
 extern int pm_generic_runtime_resume(struct device *dev);
 extern void pm_runtime_no_callbacks(struct device *dev);
@@ -143,7 +142,6 @@ static inline bool pm_runtime_active(str
 static inline bool pm_runtime_status_suspended(struct device *dev) { return false; }
 static inline bool pm_runtime_enabled(struct device *dev) { return false; }
 
-static inline int pm_generic_runtime_idle(struct device *dev) { return 0; }
 static inline int pm_generic_runtime_suspend(struct device *dev) { return 0; }
 static inline int pm_generic_runtime_resume(struct device *dev) { return 0; }
 static inline void pm_runtime_no_callbacks(struct device *dev) {}
Index: linux-pm/arch/arm/mach-omap2/omap_device.c
===================================================================
--- linux-pm.orig/arch/arm/mach-omap2/omap_device.c
+++ linux-pm/arch/arm/mach-omap2/omap_device.c
@@ -591,11 +591,6 @@ static int _od_runtime_suspend(struct de
 	return ret;
 }
 
-static int _od_runtime_idle(struct device *dev)
-{
-	return pm_generic_runtime_idle(dev);
-}
-
 static int _od_runtime_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -653,7 +648,7 @@ static int _od_resume_noirq(struct devic
 struct dev_pm_domain omap_device_pm_domain = {
 	.ops = {
 		SET_RUNTIME_PM_OPS(_od_runtime_suspend, _od_runtime_resume,
-				   _od_runtime_idle)
+				   NULL)
 		USE_PLATFORM_PM_SLEEP_OPS
 		.suspend_noirq = _od_suspend_noirq,
 		.resume_noirq = _od_resume_noirq,
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -886,7 +886,6 @@ static struct dev_pm_domain acpi_general
 #ifdef CONFIG_PM_RUNTIME
 		.runtime_suspend = acpi_subsys_runtime_suspend,
 		.runtime_resume = acpi_subsys_runtime_resume,
-		.runtime_idle = pm_generic_runtime_idle,
 #endif
 #ifdef CONFIG_PM_SLEEP
 		.prepare = acpi_subsys_prepare,
Index: linux-pm/drivers/amba/bus.c
===================================================================
--- linux-pm.orig/drivers/amba/bus.c
+++ linux-pm/drivers/amba/bus.c
@@ -284,7 +284,7 @@ static const struct dev_pm_ops amba_pm =
 	SET_RUNTIME_PM_OPS(
 		amba_pm_runtime_suspend,
 		amba_pm_runtime_resume,
-		pm_generic_runtime_idle
+		NULL
 	)
 };
 
Index: linux-pm/drivers/base/power/domain.c
===================================================================
--- linux-pm.orig/drivers/base/power/domain.c
+++ linux-pm/drivers/base/power/domain.c
@@ -2143,7 +2143,6 @@ void pm_genpd_init(struct generic_pm_dom
 	genpd->max_off_time_changed = true;
 	genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend;
 	genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume;
-	genpd->domain.ops.runtime_idle = pm_generic_runtime_idle;
 	genpd->domain.ops.prepare = pm_genpd_prepare;
 	genpd->domain.ops.suspend = pm_genpd_suspend;
 	genpd->domain.ops.suspend_late = pm_genpd_suspend_late;
Index: linux-pm/drivers/base/platform.c
===================================================================
--- linux-pm.orig/drivers/base/platform.c
+++ linux-pm/drivers/base/platform.c
@@ -888,7 +888,6 @@ int platform_pm_restore(struct device *d
 static const struct dev_pm_ops platform_dev_pm_ops = {
 	.runtime_suspend = pm_generic_runtime_suspend,
 	.runtime_resume = pm_generic_runtime_resume,
-	.runtime_idle = pm_generic_runtime_idle,
 	USE_PLATFORM_PM_SLEEP_OPS
 };
 
Index: linux-pm/drivers/i2c/i2c-core.c
===================================================================
--- linux-pm.orig/drivers/i2c/i2c-core.c
+++ linux-pm/drivers/i2c/i2c-core.c
@@ -435,7 +435,7 @@ static const struct dev_pm_ops i2c_devic
 	SET_RUNTIME_PM_OPS(
 		pm_generic_runtime_suspend,
 		pm_generic_runtime_resume,
-		pm_generic_runtime_idle
+		NULL
 	)
 };
 
Index: linux-pm/drivers/mmc/core/sdio_bus.c
===================================================================
--- linux-pm.orig/drivers/mmc/core/sdio_bus.c
+++ linux-pm/drivers/mmc/core/sdio_bus.c
@@ -211,7 +211,7 @@ static const struct dev_pm_ops sdio_bus_
 	SET_RUNTIME_PM_OPS(
 		pm_generic_runtime_suspend,
 		pm_generic_runtime_resume,
-		pm_generic_runtime_idle
+		NULL
 	)
 };
 
Index: linux-pm/drivers/spi/spi.c
===================================================================
--- linux-pm.orig/drivers/spi/spi.c
+++ linux-pm/drivers/spi/spi.c
@@ -223,7 +223,7 @@ static const struct dev_pm_ops spi_pm =
 	SET_RUNTIME_PM_OPS(
 		pm_generic_runtime_suspend,
 		pm_generic_runtime_resume,
-		pm_generic_runtime_idle
+		NULL
 	)
 };
 
Index: linux-pm/drivers/usb/core/port.c
===================================================================
--- linux-pm.orig/drivers/usb/core/port.c
+++ linux-pm/drivers/usb/core/port.c
@@ -141,7 +141,6 @@ static const struct dev_pm_ops usb_port_
 #ifdef CONFIG_PM_RUNTIME
 	.runtime_suspend =	usb_port_runtime_suspend,
 	.runtime_resume =	usb_port_runtime_resume,
-	.runtime_idle =		pm_generic_runtime_idle,
 #endif
 };
 
Index: linux-pm/Documentation/power/runtime_pm.txt
===================================================================
--- linux-pm.orig/Documentation/power/runtime_pm.txt
+++ linux-pm/Documentation/power/runtime_pm.txt
@@ -660,11 +660,6 @@ Subsystems may wish to conserve code spa
 management callbacks provided by the PM core, defined in
 driver/base/power/generic_ops.c:
 
-  int pm_generic_runtime_idle(struct device *dev);
-    - invoke the ->runtime_idle() callback provided by the driver of this
-      device, if defined, and call pm_runtime_suspend() for this device if the
-      return value is 0 or the callback is not defined
-
   int pm_generic_runtime_suspend(struct device *dev);
     - invoke the ->runtime_suspend() callback provided by the driver of this
       device and return its result, or return -EINVAL if not defined
Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -1046,32 +1046,6 @@ static int pci_pm_runtime_resume(struct
 	return rc;
 }
 
-static int pci_pm_runtime_idle(struct device *dev)
-{
-	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-
-	/*
-	 * If pci_dev->driver is not set (unbound), the device should
-	 * always remain in D0 regardless of the runtime PM status
-	 */
-	if (!pci_dev->driver)
-		goto out;
-
-	if (!pm)
-		return -ENOSYS;
-
-	if (pm->runtime_idle) {
-		int ret = pm->runtime_idle(dev);
-		if (ret)
-			return ret;
-	}
-
-out:
-	pm_runtime_suspend(dev);
-	return 0;
-}
-
 #else /* !CONFIG_PM_RUNTIME */
 
 #define pci_pm_runtime_suspend	NULL
@@ -1099,7 +1073,6 @@ const struct dev_pm_ops pci_dev_pm_ops =
 	.restore_noirq = pci_pm_restore_noirq,
 	.runtime_suspend = pci_pm_runtime_suspend,
 	.runtime_resume = pci_pm_runtime_resume,
-	.runtime_idle = pci_pm_runtime_idle,
 };
 
 #define PCI_PM_OPS_PTR	(&pci_dev_pm_ops)

Re: [PATCH RFC] PM / Runtime: Rework the "runtime idle" helper routine

[Mika Westerberg]

On Wed, May 29, 2013 at 01:29:06AM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The "runtime idle" helper routine, rpm_idle(), currently ignores
> return values from .runtime_idle() callbacks executed by it.
> 
> However, it turns out that many subsystems use the generic idle
> callback routine pm_generic_runtime_idle() which checks the return
> value of the driver's callback and executes pm_runtime_suspend() for
> the device unless that value is different from 0.  If that logic is
> moved to rpm_idle() instead, pm_generic_runtime_idle() can be dropped
> and its users will not need any .runtime_idle() callbacks any more.
> 
> Moreover, the PCI subsystem's .runtime_idle() routine,
> pci_pm_runtime_idle(), works in analogy with the generic one and if
> rpm_idle() calls rpm_suspend() after 0 has been returned by the
> .runtime_idle() callback executed by it, that routine will not be
> necessary any more and may be dropped.
> 
> To reduce overall code duplication make the changes described above.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> The patch doesn't break compilation for me, but it hasn't been tested
> otherwise.  It applies on top of the linux-pm.git tree's linux-next branch.
> 
> Thanks,
> Rafael
> 
> ---
>  Documentation/power/runtime_pm.txt |    5 -----
>  arch/arm/mach-omap2/omap_device.c  |    7 +------
>  drivers/acpi/device_pm.c           |    1 -
>  drivers/amba/bus.c                 |    2 +-
>  drivers/base/platform.c            |    1 -
>  drivers/base/power/domain.c        |    1 -
>  drivers/base/power/generic_ops.c   |   23 -----------------------
>  drivers/base/power/runtime.c       |   12 +++++-------
>  drivers/i2c/i2c-core.c             |    2 +-

i2c-core runtime PM idle still works with this patch :-)

You can add my

Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

for the i2c parts if you like.

>  drivers/mmc/core/sdio_bus.c        |    2 +-
>  drivers/pci/pci-driver.c           |   27 ---------------------------
>  drivers/spi/spi.c                  |    2 +-
>  drivers/usb/core/port.c            |    1 -
>  include/linux/pm_runtime.h         |    2 --
>  14 files changed, 10 insertions(+), 78 deletions(-)

Re: [PATCH RFC] PM / Runtime: Rework the "runtime idle" helper routine

[Alan Stern]

On Wed, 29 May 2013, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The "runtime idle" helper routine, rpm_idle(), currently ignores
> return values from .runtime_idle() callbacks executed by it.
> 
> However, it turns out that many subsystems use the generic idle
> callback routine pm_generic_runtime_idle() which checks the return
> value of the driver's callback and executes pm_runtime_suspend() for
> the device unless that value is different from 0.  If that logic is
> moved to rpm_idle() instead, pm_generic_runtime_idle() can be dropped
> and its users will not need any .runtime_idle() callbacks any more.

Since you're making this change, wouldn't it be a good idea to adopt
Mika's original suggestion and turn on the RPM_AUTO bit in rpmflags
when the use_autosuspend flag is set?

> Moreover, the PCI subsystem's .runtime_idle() routine,
> pci_pm_runtime_idle(), works in analogy with the generic one and if
> rpm_idle() calls rpm_suspend() after 0 has been returned by the
> .runtime_idle() callback executed by it, that routine will not be
> necessary any more and may be dropped.

See below.

What about cases where the runtime-idle callback does
rpm_schedule_suspend or rpm_request_suspend?  You'd have to make sure
that it returns -EBUSY in such cases.  Did you audit for this?

> Index: linux-pm/Documentation/power/runtime_pm.txt
> ===================================================================
> --- linux-pm.orig/Documentation/power/runtime_pm.txt
> +++ linux-pm/Documentation/power/runtime_pm.txt
> @@ -660,11 +660,6 @@ Subsystems may wish to conserve code spa
>  management callbacks provided by the PM core, defined in
>  driver/base/power/generic_ops.c:
>  
> -  int pm_generic_runtime_idle(struct device *dev);
> -    - invoke the ->runtime_idle() callback provided by the driver of this
> -      device, if defined, and call pm_runtime_suspend() for this device if the
> -      return value is 0 or the callback is not defined
> -

The documentation for the runtime-idle callback needs to be updated too.

> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -1046,32 +1046,6 @@ static int pci_pm_runtime_resume(struct
>  	return rc;
>  }
>  
> -static int pci_pm_runtime_idle(struct device *dev)
> -{
> -	struct pci_dev *pci_dev = to_pci_dev(dev);
> -	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> -
> -	/*
> -	 * If pci_dev->driver is not set (unbound), the device should
> -	 * always remain in D0 regardless of the runtime PM status
> -	 */
> -	if (!pci_dev->driver)
> -		goto out;
> -
> -	if (!pm)
> -		return -ENOSYS;
> -
> -	if (pm->runtime_idle) {
> -		int ret = pm->runtime_idle(dev);
> -		if (ret)
> -			return ret;
> -	}
> -
> -out:
> -	pm_runtime_suspend(dev);
> -	return 0;
> -}

This may not be a safe change, because now the behavior is different
in the case where dev->driver is set but pci_dev->driver isn't.  The
difference is that you will now call the driver's runtime-idle
handler, whereas the existing code doesn't.

In fact, this may turn out to be a more widespread problem.  
dev->driver gets set before the probe routine is called, and it gets
cleared after the remove routine is called.  A runtime PM callback to
the driver during these windows isn't a good idea.  Erasing subsystems'
runtime_idle handlers, as this patch does, makes it impossible for the
subsystems to protect against this.

The patch also needs to update
drivers/usb/core/driver.c:usb_runtime_idle().  If you include Mika's
suggestion, the routine can be removed entirely.

Alan Stern

Re: [PATCH RFC] PM / Runtime: Rework the "runtime idle" helper routine

[Rafael J. Wysocki]

On Wednesday, May 29, 2013 10:51:11 AM Alan Stern wrote:
> On Wed, 29 May 2013, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > The "runtime idle" helper routine, rpm_idle(), currently ignores
> > return values from .runtime_idle() callbacks executed by it.
> > 
> > However, it turns out that many subsystems use the generic idle
> > callback routine pm_generic_runtime_idle() which checks the return
> > value of the driver's callback and executes pm_runtime_suspend() for
> > the device unless that value is different from 0.  If that logic is
> > moved to rpm_idle() instead, pm_generic_runtime_idle() can be dropped
> > and its users will not need any .runtime_idle() callbacks any more.
> 
> Since you're making this change, wouldn't it be a good idea to adopt
> Mika's original suggestion and turn on the RPM_AUTO bit in rpmflags
> when the use_autosuspend flag is set?

I'm not actually sure.  It can be done, but I'd prefer to do that as a separate
change in any case.

> > Moreover, the PCI subsystem's .runtime_idle() routine,
> > pci_pm_runtime_idle(), works in analogy with the generic one and if
> > rpm_idle() calls rpm_suspend() after 0 has been returned by the
> > .runtime_idle() callback executed by it, that routine will not be
> > necessary any more and may be dropped.
> 
> See below.
> 
> What about cases where the runtime-idle callback does
> rpm_schedule_suspend or rpm_request_suspend?  You'd have to make sure
> that it returns -EBUSY in such cases.  Did you audit for this?

As far as I could.

I'm not worried about the subsystems modified by this patch, because the
functionality there won't change (except for PCI, that is).

> > Index: linux-pm/Documentation/power/runtime_pm.txt
> > ===================================================================
> > --- linux-pm.orig/Documentation/power/runtime_pm.txt
> > +++ linux-pm/Documentation/power/runtime_pm.txt
> > @@ -660,11 +660,6 @@ Subsystems may wish to conserve code spa
> >  management callbacks provided by the PM core, defined in
> >  driver/base/power/generic_ops.c:
> >  
> > -  int pm_generic_runtime_idle(struct device *dev);
> > -    - invoke the ->runtime_idle() callback provided by the driver of this
> > -      device, if defined, and call pm_runtime_suspend() for this device if the
> > -      return value is 0 or the callback is not defined
> > -
> 
> The documentation for the runtime-idle callback needs to be updated too.

Well, I actually couldn't find the part of it that would need to be updated. :-)

> > Index: linux-pm/drivers/pci/pci-driver.c
> > ===================================================================
> > --- linux-pm.orig/drivers/pci/pci-driver.c
> > +++ linux-pm/drivers/pci/pci-driver.c
> > @@ -1046,32 +1046,6 @@ static int pci_pm_runtime_resume(struct
> >  	return rc;
> >  }
> >  
> > -static int pci_pm_runtime_idle(struct device *dev)
> > -{
> > -	struct pci_dev *pci_dev = to_pci_dev(dev);
> > -	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > -
> > -	/*
> > -	 * If pci_dev->driver is not set (unbound), the device should
> > -	 * always remain in D0 regardless of the runtime PM status
> > -	 */
> > -	if (!pci_dev->driver)
> > -		goto out;
> > -
> > -	if (!pm)
> > -		return -ENOSYS;
> > -
> > -	if (pm->runtime_idle) {
> > -		int ret = pm->runtime_idle(dev);
> > -		if (ret)
> > -			return ret;
> > -	}
> > -
> > -out:
> > -	pm_runtime_suspend(dev);
> > -	return 0;
> > -}
> 
> This may not be a safe change, because now the behavior is different
> in the case where dev->driver is set but pci_dev->driver isn't.

That's a good point.  I think I'll drop the PCI change, then.
Or rather, I'll just remove the pm_runtime_suspend() call from
pci_pm_runtime_idle(). :-)

> The difference is that you will now call the driver's runtime-idle
> handler, whereas the existing code doesn't.
> 
> In fact, this may turn out to be a more widespread problem.  
> dev->driver gets set before the probe routine is called, and it gets
> cleared after the remove routine is called.  A runtime PM callback to
> the driver during these windows isn't a good idea.  Erasing subsystems'
> runtime_idle handlers, as this patch does, makes it impossible for the
> subsystems to protect against this.

Except for PCI it only removes the ones that point to
pm_generic_runtime_idle(), which obviously doesn't check that.

> The patch also needs to update
> drivers/usb/core/driver.c:usb_runtime_idle().

Yes, it does.

> If you include Mika's suggestion, the routine can be removed entirely.

Later. :-)

Thanks,
Rafael


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

Re: [PATCH RFC] PM / Runtime: Rework the "runtime idle" helper routine

[Aaron Lu]

On 05/30/2013 06:18 AM, Rafael J. Wysocki wrote:
> On Wednesday, May 29, 2013 10:51:11 AM Alan Stern wrote:
>> On Wed, 29 May 2013, Rafael J. Wysocki wrote:
>>
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> The "runtime idle" helper routine, rpm_idle(), currently ignores
>>> return values from .runtime_idle() callbacks executed by it.
>>>
>>> However, it turns out that many subsystems use the generic idle
>>> callback routine pm_generic_runtime_idle() which checks the return
>>> value of the driver's callback and executes pm_runtime_suspend() for
>>> the device unless that value is different from 0.  If that logic is
>>> moved to rpm_idle() instead, pm_generic_runtime_idle() can be dropped
>>> and its users will not need any .runtime_idle() callbacks any more.
>>
>> Since you're making this change, wouldn't it be a good idea to adopt
>> Mika's original suggestion and turn on the RPM_AUTO bit in rpmflags
>> when the use_autosuspend flag is set?
> 
> I'm not actually sure.  It can be done, but I'd prefer to do that as a separate
> change in any case.

For SCSI idle callback, that would be a welcome change, where instead of
calling pm_runtime_autosuspend and then return -EBUSY, it can be changed
to simply return 0 after mark_last_busy.

Thanks,
Aaron

Re: [PATCH RFC] PM / Runtime: Rework the "runtime idle" helper routine

[Alan Stern]

On Thu, 30 May 2013, Rafael J. Wysocki wrote:

> > Since you're making this change, wouldn't it be a good idea to adopt
> > Mika's original suggestion and turn on the RPM_AUTO bit in rpmflags
> > when the use_autosuspend flag is set?
> 
> I'm not actually sure.  It can be done, but I'd prefer to do that as a separate
> change in any case.

That makes sense.

> > What about cases where the runtime-idle callback does
> > rpm_schedule_suspend or rpm_request_suspend?  You'd have to make sure
> > that it returns -EBUSY in such cases.  Did you audit for this?
> 
> As far as I could.
> 
> I'm not worried about the subsystems modified by this patch, because the
> functionality there won't change (except for PCI, that is).

Right.  The subsystems that _aren't_ modified are the ones to worry 
about -- like the USB callback.  They are the ones where the behavior 
might change.

> > > Index: linux-pm/Documentation/power/runtime_pm.txt
> > > ===================================================================
> > > --- linux-pm.orig/Documentation/power/runtime_pm.txt
> > > +++ linux-pm/Documentation/power/runtime_pm.txt
> > > @@ -660,11 +660,6 @@ Subsystems may wish to conserve code spa
> > >  management callbacks provided by the PM core, defined in
> > >  driver/base/power/generic_ops.c:
> > >  
> > > -  int pm_generic_runtime_idle(struct device *dev);
> > > -    - invoke the ->runtime_idle() callback provided by the driver of this
> > > -      device, if defined, and call pm_runtime_suspend() for this device if the
> > > -      return value is 0 or the callback is not defined
> > > -
> > 
> > The documentation for the runtime-idle callback needs to be updated too.
> 
> Well, I actually couldn't find the part of it that would need to be updated. :-)

How about this?


Index: usb-3.10/Documentation/power/runtime_pm.txt
===================================================================
--- usb-3.10.orig/Documentation/power/runtime_pm.txt
+++ usb-3.10/Documentation/power/runtime_pm.txt
@@ -144,8 +144,12 @@ The action performed by the idle callbac
 (or driver) in question, but the expected and recommended action is to check
 if the device can be suspended (i.e. if all of the conditions necessary for
 suspending the device are satisfied) and to queue up a suspend request for the
-device in that case.  The value returned by this callback is ignored by the PM
-core.
+device in that case.  If there is no idle callback, or if the callback returns
+0, then the PM core will attempt to carry out a runtime suspend of the device;
+in essence, it will call pm_runtime_suspend() directly.  To prevent this (for
+example, if the callback routine has started a delayed suspend), the routine
+should return a non-zero value.  Negative error return codes are ignored by the
+PM core.
 
 The helper functions provided by the PM core, described in Section 4, guarantee
 that the following constraints are met with respect to runtime PM callbacks for
@@ -301,9 +305,10 @@ drivers/base/power/runtime.c and include
       removing the device from device hierarchy
 
   int pm_runtime_idle(struct device *dev);
-    - execute the subsystem-level idle callback for the device; returns 0 on
-      success or error code on failure, where -EINPROGRESS means that
-      ->runtime_idle() is already being executed
+    - execute the subsystem-level idle callback for the device; returns an
+      error code on failure, where -EINPROGRESS means that ->runtime_idle() is
+      already being executed; if there is no callback or the callback returns 0
+      then run pm_runtime_suspend(dev) and return its result
 
   int pm_runtime_suspend(struct device *dev);
     - execute the subsystem-level suspend callback for the device; returns 0 on


Alan Stern

Re: [PATCH RFC] PM / Runtime: Rework the "runtime idle" helper routine

[Rafael J. Wysocki]

On Thursday, May 30, 2013 01:08:08 PM Alan Stern wrote:
> On Thu, 30 May 2013, Rafael J. Wysocki wrote:
> 
> > > Since you're making this change, wouldn't it be a good idea to adopt
> > > Mika's original suggestion and turn on the RPM_AUTO bit in rpmflags
> > > when the use_autosuspend flag is set?
> > 
> > I'm not actually sure.  It can be done, but I'd prefer to do that as a separate
> > change in any case.
> 
> That makes sense.
> 
> > > What about cases where the runtime-idle callback does
> > > rpm_schedule_suspend or rpm_request_suspend?  You'd have to make sure
> > > that it returns -EBUSY in such cases.  Did you audit for this?
> > 
> > As far as I could.
> > 
> > I'm not worried about the subsystems modified by this patch, because the
> > functionality there won't change (except for PCI, that is).
> 
> Right.  The subsystems that _aren't_ modified are the ones to worry 
> about -- like the USB callback.  They are the ones where the behavior 
> might change.

Right.

> > > > Index: linux-pm/Documentation/power/runtime_pm.txt
> > > > ===================================================================
> > > > --- linux-pm.orig/Documentation/power/runtime_pm.txt
> > > > +++ linux-pm/Documentation/power/runtime_pm.txt
> > > > @@ -660,11 +660,6 @@ Subsystems may wish to conserve code spa
> > > >  management callbacks provided by the PM core, defined in
> > > >  driver/base/power/generic_ops.c:
> > > >  
> > > > -  int pm_generic_runtime_idle(struct device *dev);
> > > > -    - invoke the ->runtime_idle() callback provided by the driver of this
> > > > -      device, if defined, and call pm_runtime_suspend() for this device if the
> > > > -      return value is 0 or the callback is not defined
> > > > -
> > > 
> > > The documentation for the runtime-idle callback needs to be updated too.
> > 
> > Well, I actually couldn't find the part of it that would need to be updated. :-)
> 
> How about this?

Looks good! :-)

May I add your sign-off to it?

Rafael


> Index: usb-3.10/Documentation/power/runtime_pm.txt
> ===================================================================
> --- usb-3.10.orig/Documentation/power/runtime_pm.txt
> +++ usb-3.10/Documentation/power/runtime_pm.txt
> @@ -144,8 +144,12 @@ The action performed by the idle callbac
>  (or driver) in question, but the expected and recommended action is to check
>  if the device can be suspended (i.e. if all of the conditions necessary for
>  suspending the device are satisfied) and to queue up a suspend request for the
> -device in that case.  The value returned by this callback is ignored by the PM
> -core.
> +device in that case.  If there is no idle callback, or if the callback returns
> +0, then the PM core will attempt to carry out a runtime suspend of the device;
> +in essence, it will call pm_runtime_suspend() directly.  To prevent this (for
> +example, if the callback routine has started a delayed suspend), the routine
> +should return a non-zero value.  Negative error return codes are ignored by the
> +PM core.
>  
>  The helper functions provided by the PM core, described in Section 4, guarantee
>  that the following constraints are met with respect to runtime PM callbacks for
> @@ -301,9 +305,10 @@ drivers/base/power/runtime.c and include
>        removing the device from device hierarchy
>  
>    int pm_runtime_idle(struct device *dev);
> -    - execute the subsystem-level idle callback for the device; returns 0 on
> -      success or error code on failure, where -EINPROGRESS means that
> -      ->runtime_idle() is already being executed
> +    - execute the subsystem-level idle callback for the device; returns an
> +      error code on failure, where -EINPROGRESS means that ->runtime_idle() is
> +      already being executed; if there is no callback or the callback returns 0
> +      then run pm_runtime_suspend(dev) and return its result
>  
>    int pm_runtime_suspend(struct device *dev);
>      - execute the subsystem-level suspend callback for the device; returns 0 on
> 
> 
> Alan Stern
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH RFC] PM / Runtime: Rework the "runtime idle" helper routine

[Alan Stern]

On Thu, 30 May 2013, Rafael J. Wysocki wrote:

> > > > > Index: linux-pm/Documentation/power/runtime_pm.txt
> > > > > ===================================================================
> > > > > --- linux-pm.orig/Documentation/power/runtime_pm.txt
> > > > > +++ linux-pm/Documentation/power/runtime_pm.txt
> > > > > @@ -660,11 +660,6 @@ Subsystems may wish to conserve code spa
> > > > >  management callbacks provided by the PM core, defined in
> > > > >  driver/base/power/generic_ops.c:
> > > > >  
> > > > > -  int pm_generic_runtime_idle(struct device *dev);
> > > > > -    - invoke the ->runtime_idle() callback provided by the driver of this
> > > > > -      device, if defined, and call pm_runtime_suspend() for this device if the
> > > > > -      return value is 0 or the callback is not defined
> > > > > -
> > > > 
> > > > The documentation for the runtime-idle callback needs to be updated too.
> > > 
> > > Well, I actually couldn't find the part of it that would need to be updated. :-)
> > 
> > How about this?
> 
> Looks good! :-)
> 
> May I add your sign-off to it?

Go ahead.

Alan Stern

Re: [PATCH RFC] PM / Runtime: Rework the "runtime idle" helper routine

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@sisk.pl> writes:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The "runtime idle" helper routine, rpm_idle(), currently ignores
> return values from .runtime_idle() callbacks executed by it.
>
> However, it turns out that many subsystems use the generic idle
> callback routine pm_generic_runtime_idle() which checks the return
> value of the driver's callback and executes pm_runtime_suspend() for
> the device unless that value is different from 0.  If that logic is
> moved to rpm_idle() instead, pm_generic_runtime_idle() can be dropped
> and its users will not need any .runtime_idle() callbacks any more.
>
> Moreover, the PCI subsystem's .runtime_idle() routine,
> pci_pm_runtime_idle(), works in analogy with the generic one and if
> rpm_idle() calls rpm_suspend() after 0 has been returned by the
> .runtime_idle() callback executed by it, that routine will not be
> necessary any more and may be dropped.
>
> To reduce overall code duplication make the changes described above.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Acked-by: Kevin Hilman <khilman@linaro.org>
Tested-by: Kevin Hilman <khilman@linaro.org> # for OMAP PM domain change

Re: [PATCH RFC] PM / Runtime: Rework the "runtime idle" helper routine

[Ulf Hansson]

On 29 May 2013 01:29, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The "runtime idle" helper routine, rpm_idle(), currently ignores
> return values from .runtime_idle() callbacks executed by it.
>
> However, it turns out that many subsystems use the generic idle
> callback routine pm_generic_runtime_idle() which checks the return
> value of the driver's callback and executes pm_runtime_suspend() for
> the device unless that value is different from 0.  If that logic is
> moved to rpm_idle() instead, pm_generic_runtime_idle() can be dropped
> and its users will not need any .runtime_idle() callbacks any more.
>
> Moreover, the PCI subsystem's .runtime_idle() routine,
> pci_pm_runtime_idle(), works in analogy with the generic one and if
> rpm_idle() calls rpm_suspend() after 0 has been returned by the
> .runtime_idle() callback executed by it, that routine will not be
> necessary any more and may be dropped.
>
> To reduce overall code duplication make the changes described above.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---

This looks good to me!

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>


>
> The patch doesn't break compilation for me, but it hasn't been tested
> otherwise.  It applies on top of the linux-pm.git tree's linux-next branch.
>
> Thanks,
> Rafael
>
> ---
>  Documentation/power/runtime_pm.txt |    5 -----
>  arch/arm/mach-omap2/omap_device.c  |    7 +------
>  drivers/acpi/device_pm.c           |    1 -
>  drivers/amba/bus.c                 |    2 +-
>  drivers/base/platform.c            |    1 -
>  drivers/base/power/domain.c        |    1 -
>  drivers/base/power/generic_ops.c   |   23 -----------------------
>  drivers/base/power/runtime.c       |   12 +++++-------
>  drivers/i2c/i2c-core.c             |    2 +-
>  drivers/mmc/core/sdio_bus.c        |    2 +-
>  drivers/pci/pci-driver.c           |   27 ---------------------------
>  drivers/spi/spi.c                  |    2 +-
>  drivers/usb/core/port.c            |    1 -
>  include/linux/pm_runtime.h         |    2 --
>  14 files changed, 10 insertions(+), 78 deletions(-)
>
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -293,11 +293,8 @@ static int rpm_idle(struct device *dev,
>         /* Pending requests need to be canceled. */
>         dev->power.request = RPM_REQ_NONE;
>
> -       if (dev->power.no_callbacks) {
> -               /* Assume ->runtime_idle() callback would have suspended. */
> -               retval = rpm_suspend(dev, rpmflags);
> +       if (dev->power.no_callbacks)
>                 goto out;
> -       }
>
>         /* Carry out an asynchronous or a synchronous idle notification. */
>         if (rpmflags & RPM_ASYNC) {
> @@ -306,7 +303,8 @@ static int rpm_idle(struct device *dev,
>                         dev->power.request_pending = true;
>                         queue_work(pm_wq, &dev->power.work);
>                 }
> -               goto out;
> +               trace_rpm_return_int(dev, _THIS_IP_, 0);
> +               return 0;
>         }
>
>         dev->power.idle_notification = true;
> @@ -326,14 +324,14 @@ static int rpm_idle(struct device *dev,
>                 callback = dev->driver->pm->runtime_idle;
>
>         if (callback)
> -               __rpm_callback(callback, dev);
> +               retval = __rpm_callback(callback, dev);
>
>         dev->power.idle_notification = false;
>         wake_up_all(&dev->power.wait_queue);
>
>   out:
>         trace_rpm_return_int(dev, _THIS_IP_, retval);
> -       return retval;
> +       return retval ? retval : rpm_suspend(dev, rpmflags);
>  }
>
>  /**
> Index: linux-pm/drivers/base/power/generic_ops.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/generic_ops.c
> +++ linux-pm/drivers/base/power/generic_ops.c
> @@ -12,29 +12,6 @@
>
>  #ifdef CONFIG_PM_RUNTIME
>  /**
> - * pm_generic_runtime_idle - Generic runtime idle callback for subsystems.
> - * @dev: Device to handle.
> - *
> - * If PM operations are defined for the @dev's driver and they include
> - * ->runtime_idle(), execute it and return its error code, if nonzero.
> - * Otherwise, execute pm_runtime_suspend() for the device and return 0.
> - */
> -int pm_generic_runtime_idle(struct device *dev)
> -{
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> -
> -       if (pm && pm->runtime_idle) {
> -               int ret = pm->runtime_idle(dev);
> -               if (ret)
> -                       return ret;
> -       }
> -
> -       pm_runtime_suspend(dev);
> -       return 0;
> -}
> -EXPORT_SYMBOL_GPL(pm_generic_runtime_idle);
> -
> -/**
>   * pm_generic_runtime_suspend - Generic runtime suspend callback for subsystems.
>   * @dev: Device to suspend.
>   *
> Index: linux-pm/include/linux/pm_runtime.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm_runtime.h
> +++ linux-pm/include/linux/pm_runtime.h
> @@ -37,7 +37,6 @@ extern void pm_runtime_enable(struct dev
>  extern void __pm_runtime_disable(struct device *dev, bool check_resume);
>  extern void pm_runtime_allow(struct device *dev);
>  extern void pm_runtime_forbid(struct device *dev);
> -extern int pm_generic_runtime_idle(struct device *dev);
>  extern int pm_generic_runtime_suspend(struct device *dev);
>  extern int pm_generic_runtime_resume(struct device *dev);
>  extern void pm_runtime_no_callbacks(struct device *dev);
> @@ -143,7 +142,6 @@ static inline bool pm_runtime_active(str
>  static inline bool pm_runtime_status_suspended(struct device *dev) { return false; }
>  static inline bool pm_runtime_enabled(struct device *dev) { return false; }
>
> -static inline int pm_generic_runtime_idle(struct device *dev) { return 0; }
>  static inline int pm_generic_runtime_suspend(struct device *dev) { return 0; }
>  static inline int pm_generic_runtime_resume(struct device *dev) { return 0; }
>  static inline void pm_runtime_no_callbacks(struct device *dev) {}
> Index: linux-pm/arch/arm/mach-omap2/omap_device.c
> ===================================================================
> --- linux-pm.orig/arch/arm/mach-omap2/omap_device.c
> +++ linux-pm/arch/arm/mach-omap2/omap_device.c
> @@ -591,11 +591,6 @@ static int _od_runtime_suspend(struct de
>         return ret;
>  }
>
> -static int _od_runtime_idle(struct device *dev)
> -{
> -       return pm_generic_runtime_idle(dev);
> -}
> -
>  static int _od_runtime_resume(struct device *dev)
>  {
>         struct platform_device *pdev = to_platform_device(dev);
> @@ -653,7 +648,7 @@ static int _od_resume_noirq(struct devic
>  struct dev_pm_domain omap_device_pm_domain = {
>         .ops = {
>                 SET_RUNTIME_PM_OPS(_od_runtime_suspend, _od_runtime_resume,
> -                                  _od_runtime_idle)
> +                                  NULL)
>                 USE_PLATFORM_PM_SLEEP_OPS
>                 .suspend_noirq = _od_suspend_noirq,
>                 .resume_noirq = _od_resume_noirq,
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -886,7 +886,6 @@ static struct dev_pm_domain acpi_general
>  #ifdef CONFIG_PM_RUNTIME
>                 .runtime_suspend = acpi_subsys_runtime_suspend,
>                 .runtime_resume = acpi_subsys_runtime_resume,
> -               .runtime_idle = pm_generic_runtime_idle,
>  #endif
>  #ifdef CONFIG_PM_SLEEP
>                 .prepare = acpi_subsys_prepare,
> Index: linux-pm/drivers/amba/bus.c
> ===================================================================
> --- linux-pm.orig/drivers/amba/bus.c
> +++ linux-pm/drivers/amba/bus.c
> @@ -284,7 +284,7 @@ static const struct dev_pm_ops amba_pm =
>         SET_RUNTIME_PM_OPS(
>                 amba_pm_runtime_suspend,
>                 amba_pm_runtime_resume,
> -               pm_generic_runtime_idle
> +               NULL
>         )
>  };
>
> Index: linux-pm/drivers/base/power/domain.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/domain.c
> +++ linux-pm/drivers/base/power/domain.c
> @@ -2143,7 +2143,6 @@ void pm_genpd_init(struct generic_pm_dom
>         genpd->max_off_time_changed = true;
>         genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend;
>         genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume;
> -       genpd->domain.ops.runtime_idle = pm_generic_runtime_idle;
>         genpd->domain.ops.prepare = pm_genpd_prepare;
>         genpd->domain.ops.suspend = pm_genpd_suspend;
>         genpd->domain.ops.suspend_late = pm_genpd_suspend_late;
> Index: linux-pm/drivers/base/platform.c
> ===================================================================
> --- linux-pm.orig/drivers/base/platform.c
> +++ linux-pm/drivers/base/platform.c
> @@ -888,7 +888,6 @@ int platform_pm_restore(struct device *d
>  static const struct dev_pm_ops platform_dev_pm_ops = {
>         .runtime_suspend = pm_generic_runtime_suspend,
>         .runtime_resume = pm_generic_runtime_resume,
> -       .runtime_idle = pm_generic_runtime_idle,
>         USE_PLATFORM_PM_SLEEP_OPS
>  };
>
> Index: linux-pm/drivers/i2c/i2c-core.c
> ===================================================================
> --- linux-pm.orig/drivers/i2c/i2c-core.c
> +++ linux-pm/drivers/i2c/i2c-core.c
> @@ -435,7 +435,7 @@ static const struct dev_pm_ops i2c_devic
>         SET_RUNTIME_PM_OPS(
>                 pm_generic_runtime_suspend,
>                 pm_generic_runtime_resume,
> -               pm_generic_runtime_idle
> +               NULL
>         )
>  };
>
> Index: linux-pm/drivers/mmc/core/sdio_bus.c
> ===================================================================
> --- linux-pm.orig/drivers/mmc/core/sdio_bus.c
> +++ linux-pm/drivers/mmc/core/sdio_bus.c
> @@ -211,7 +211,7 @@ static const struct dev_pm_ops sdio_bus_
>         SET_RUNTIME_PM_OPS(
>                 pm_generic_runtime_suspend,
>                 pm_generic_runtime_resume,
> -               pm_generic_runtime_idle
> +               NULL
>         )
>  };
>
> Index: linux-pm/drivers/spi/spi.c
> ===================================================================
> --- linux-pm.orig/drivers/spi/spi.c
> +++ linux-pm/drivers/spi/spi.c
> @@ -223,7 +223,7 @@ static const struct dev_pm_ops spi_pm =
>         SET_RUNTIME_PM_OPS(
>                 pm_generic_runtime_suspend,
>                 pm_generic_runtime_resume,
> -               pm_generic_runtime_idle
> +               NULL
>         )
>  };
>
> Index: linux-pm/drivers/usb/core/port.c
> ===================================================================
> --- linux-pm.orig/drivers/usb/core/port.c
> +++ linux-pm/drivers/usb/core/port.c
> @@ -141,7 +141,6 @@ static const struct dev_pm_ops usb_port_
>  #ifdef CONFIG_PM_RUNTIME
>         .runtime_suspend =      usb_port_runtime_suspend,
>         .runtime_resume =       usb_port_runtime_resume,
> -       .runtime_idle =         pm_generic_runtime_idle,
>  #endif
>  };
>
> Index: linux-pm/Documentation/power/runtime_pm.txt
> ===================================================================
> --- linux-pm.orig/Documentation/power/runtime_pm.txt
> +++ linux-pm/Documentation/power/runtime_pm.txt
> @@ -660,11 +660,6 @@ Subsystems may wish to conserve code spa
>  management callbacks provided by the PM core, defined in
>  driver/base/power/generic_ops.c:
>
> -  int pm_generic_runtime_idle(struct device *dev);
> -    - invoke the ->runtime_idle() callback provided by the driver of this
> -      device, if defined, and call pm_runtime_suspend() for this device if the
> -      return value is 0 or the callback is not defined
> -
>    int pm_generic_runtime_suspend(struct device *dev);
>      - invoke the ->runtime_suspend() callback provided by the driver of this
>        device and return its result, or return -EINVAL if not defined
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -1046,32 +1046,6 @@ static int pci_pm_runtime_resume(struct
>         return rc;
>  }
>
> -static int pci_pm_runtime_idle(struct device *dev)
> -{
> -       struct pci_dev *pci_dev = to_pci_dev(dev);
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> -
> -       /*
> -        * If pci_dev->driver is not set (unbound), the device should
> -        * always remain in D0 regardless of the runtime PM status
> -        */
> -       if (!pci_dev->driver)
> -               goto out;
> -
> -       if (!pm)
> -               return -ENOSYS;
> -
> -       if (pm->runtime_idle) {
> -               int ret = pm->runtime_idle(dev);
> -               if (ret)
> -                       return ret;
> -       }
> -
> -out:
> -       pm_runtime_suspend(dev);
> -       return 0;
> -}
> -
>  #else /* !CONFIG_PM_RUNTIME */
>
>  #define pci_pm_runtime_suspend NULL
> @@ -1099,7 +1073,6 @@ const struct dev_pm_ops pci_dev_pm_ops =
>         .restore_noirq = pci_pm_restore_noirq,
>         .runtime_suspend = pci_pm_runtime_suspend,
>         .runtime_resume = pci_pm_runtime_resume,
> -       .runtime_idle = pci_pm_runtime_idle,
>  };
>
>  #define PCI_PM_OPS_PTR (&pci_dev_pm_ops)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 0/2] PM / Runtime: Rework the "runtime idle" helper routine (was: Re: [PATCH RFC] PM / Runtime: Rework the "runtime idle" helper routine)

[Rafael J. Wysocki]

On Thursday, May 30, 2013 01:08:08 PM Alan Stern wrote:
> On Thu, 30 May 2013, Rafael J. Wysocki wrote:
> 
> > > Since you're making this change, wouldn't it be a good idea to adopt
> > > Mika's original suggestion and turn on the RPM_AUTO bit in rpmflags
> > > when the use_autosuspend flag is set?
> > 
> > I'm not actually sure.  It can be done, but I'd prefer to do that as a separate
> > change in any case.
> 
> That makes sense.
> 
> > > What about cases where the runtime-idle callback does
> > > rpm_schedule_suspend or rpm_request_suspend?  You'd have to make sure
> > > that it returns -EBUSY in such cases.  Did you audit for this?
> > 
> > As far as I could.
> > 
> > I'm not worried about the subsystems modified by this patch, because the
> > functionality there won't change (except for PCI, that is).
> 
> Right.  The subsystems that _aren't_ modified are the ones to worry 
> about -- like the USB callback.  They are the ones where the behavior 
> might change.

OK, this time I think I've caught all of them. :-)

I've retained the ACKs and Reviewed-by tags in [1/2], because it only makes
more changes in addition to the previously ACKed ones.  The PCI changeset
has been updated and [2/2] is the documentation update.

Thanks,
Rafael


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

[PATCH 1/2, v2] PM / Runtime: Rework the "runtime idle" helper routine

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The "runtime idle" helper routine, rpm_idle(), currently ignores
return values from .runtime_idle() callbacks executed by it.
However, it turns out that many subsystems use
pm_generic_runtime_idle() which checks the return value of the
driver's callback and executes pm_runtime_suspend() for the device
unless that value is not 0.  If that logic is moved to rpm_idle()
instead, pm_generic_runtime_idle() can be dropped and its users
will not need any .runtime_idle() callbacks any more.

Moreover, the PCI, SCSI, and SATA subsystems' .runtime_idle()
routines, pci_pm_runtime_idle(), scsi_runtime_idle(), and
ata_port_runtime_idle(), respectively, as well as a few drivers'
ones may be simplified if rpm_idle() calls rpm_suspend() after 0 has
been returned by the .runtime_idle() callback executed by it.

To reduce overall code bloat, make the changes described above.

Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Tested-by: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Kevin Hilman <khilman@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 Documentation/power/runtime_pm.txt |    5 -----
 arch/arm/mach-omap2/omap_device.c  |    7 +------
 drivers/acpi/device_pm.c           |    1 -
 drivers/amba/bus.c                 |    2 +-
 drivers/ata/libata-core.c          |    2 +-
 drivers/base/platform.c            |    1 -
 drivers/base/power/domain.c        |    1 -
 drivers/base/power/generic_ops.c   |   23 -----------------------
 drivers/base/power/runtime.c       |   12 +++++-------
 drivers/dma/intel_mid_dma.c        |    2 +-
 drivers/gpio/gpio-langwell.c       |    6 +-----
 drivers/i2c/i2c-core.c             |    2 +-
 drivers/mfd/ab8500-gpadc.c         |    8 +-------
 drivers/mmc/core/bus.c             |    2 +-
 drivers/mmc/core/sdio_bus.c        |    2 +-
 drivers/pci/pci-driver.c           |   14 +++++---------
 drivers/scsi/scsi_pm.c             |   11 +++--------
 drivers/sh/pm_runtime.c            |    2 +-
 drivers/spi/spi.c                  |    2 +-
 drivers/tty/serial/mfd.c           |    9 ++-------
 drivers/usb/core/driver.c          |    3 ++-
 drivers/usb/core/port.c            |    1 -
 include/linux/pm_runtime.h         |    2 --
 23 files changed, 28 insertions(+), 92 deletions(-)

Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -293,11 +293,8 @@ static int rpm_idle(struct device *dev,
 	/* Pending requests need to be canceled. */
 	dev->power.request = RPM_REQ_NONE;
 
-	if (dev->power.no_callbacks) {
-		/* Assume ->runtime_idle() callback would have suspended. */
-		retval = rpm_suspend(dev, rpmflags);
+	if (dev->power.no_callbacks)
 		goto out;
-	}
 
 	/* Carry out an asynchronous or a synchronous idle notification. */
 	if (rpmflags & RPM_ASYNC) {
@@ -306,7 +303,8 @@ static int rpm_idle(struct device *dev,
 			dev->power.request_pending = true;
 			queue_work(pm_wq, &dev->power.work);
 		}
-		goto out;
+		trace_rpm_return_int(dev, _THIS_IP_, 0);
+		return 0;
 	}
 
 	dev->power.idle_notification = true;
@@ -326,14 +324,14 @@ static int rpm_idle(struct device *dev,
 		callback = dev->driver->pm->runtime_idle;
 
 	if (callback)
-		__rpm_callback(callback, dev);
+		retval = __rpm_callback(callback, dev);
 
 	dev->power.idle_notification = false;
 	wake_up_all(&dev->power.wait_queue);
 
  out:
 	trace_rpm_return_int(dev, _THIS_IP_, retval);
-	return retval;
+	return retval ? retval : rpm_suspend(dev, rpmflags);
 }
 
 /**
Index: linux-pm/drivers/base/power/generic_ops.c
===================================================================
--- linux-pm.orig/drivers/base/power/generic_ops.c
+++ linux-pm/drivers/base/power/generic_ops.c
@@ -12,29 +12,6 @@
 
 #ifdef CONFIG_PM_RUNTIME
 /**
- * pm_generic_runtime_idle - Generic runtime idle callback for subsystems.
- * @dev: Device to handle.
- *
- * If PM operations are defined for the @dev's driver and they include
- * ->runtime_idle(), execute it and return its error code, if nonzero.
- * Otherwise, execute pm_runtime_suspend() for the device and return 0.
- */
-int pm_generic_runtime_idle(struct device *dev)
-{
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-
-	if (pm && pm->runtime_idle) {
-		int ret = pm->runtime_idle(dev);
-		if (ret)
-			return ret;
-	}
-
-	pm_runtime_suspend(dev);
-	return 0;
-}
-EXPORT_SYMBOL_GPL(pm_generic_runtime_idle);
-
-/**
  * pm_generic_runtime_suspend - Generic runtime suspend callback for subsystems.
  * @dev: Device to suspend.
  *
Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -37,7 +37,6 @@ extern void pm_runtime_enable(struct dev
 extern void __pm_runtime_disable(struct device *dev, bool check_resume);
 extern void pm_runtime_allow(struct device *dev);
 extern void pm_runtime_forbid(struct device *dev);
-extern int pm_generic_runtime_idle(struct device *dev);
 extern int pm_generic_runtime_suspend(struct device *dev);
 extern int pm_generic_runtime_resume(struct device *dev);
 extern void pm_runtime_no_callbacks(struct device *dev);
@@ -143,7 +142,6 @@ static inline bool pm_runtime_active(str
 static inline bool pm_runtime_status_suspended(struct device *dev) { return false; }
 static inline bool pm_runtime_enabled(struct device *dev) { return false; }
 
-static inline int pm_generic_runtime_idle(struct device *dev) { return 0; }
 static inline int pm_generic_runtime_suspend(struct device *dev) { return 0; }
 static inline int pm_generic_runtime_resume(struct device *dev) { return 0; }
 static inline void pm_runtime_no_callbacks(struct device *dev) {}
Index: linux-pm/arch/arm/mach-omap2/omap_device.c
===================================================================
--- linux-pm.orig/arch/arm/mach-omap2/omap_device.c
+++ linux-pm/arch/arm/mach-omap2/omap_device.c
@@ -591,11 +591,6 @@ static int _od_runtime_suspend(struct de
 	return ret;
 }
 
-static int _od_runtime_idle(struct device *dev)
-{
-	return pm_generic_runtime_idle(dev);
-}
-
 static int _od_runtime_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -653,7 +648,7 @@ static int _od_resume_noirq(struct devic
 struct dev_pm_domain omap_device_pm_domain = {
 	.ops = {
 		SET_RUNTIME_PM_OPS(_od_runtime_suspend, _od_runtime_resume,
-				   _od_runtime_idle)
+				   NULL)
 		USE_PLATFORM_PM_SLEEP_OPS
 		.suspend_noirq = _od_suspend_noirq,
 		.resume_noirq = _od_resume_noirq,
Index: linux-pm/drivers/acpi/device_pm.c
===================================================================
--- linux-pm.orig/drivers/acpi/device_pm.c
+++ linux-pm/drivers/acpi/device_pm.c
@@ -886,7 +886,6 @@ static struct dev_pm_domain acpi_general
 #ifdef CONFIG_PM_RUNTIME
 		.runtime_suspend = acpi_subsys_runtime_suspend,
 		.runtime_resume = acpi_subsys_runtime_resume,
-		.runtime_idle = pm_generic_runtime_idle,
 #endif
 #ifdef CONFIG_PM_SLEEP
 		.prepare = acpi_subsys_prepare,
Index: linux-pm/drivers/amba/bus.c
===================================================================
--- linux-pm.orig/drivers/amba/bus.c
+++ linux-pm/drivers/amba/bus.c
@@ -284,7 +284,7 @@ static const struct dev_pm_ops amba_pm =
 	SET_RUNTIME_PM_OPS(
 		amba_pm_runtime_suspend,
 		amba_pm_runtime_resume,
-		pm_generic_runtime_idle
+		NULL
 	)
 };
 
Index: linux-pm/drivers/base/power/domain.c
===================================================================
--- linux-pm.orig/drivers/base/power/domain.c
+++ linux-pm/drivers/base/power/domain.c
@@ -2143,7 +2143,6 @@ void pm_genpd_init(struct generic_pm_dom
 	genpd->max_off_time_changed = true;
 	genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend;
 	genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume;
-	genpd->domain.ops.runtime_idle = pm_generic_runtime_idle;
 	genpd->domain.ops.prepare = pm_genpd_prepare;
 	genpd->domain.ops.suspend = pm_genpd_suspend;
 	genpd->domain.ops.suspend_late = pm_genpd_suspend_late;
Index: linux-pm/drivers/base/platform.c
===================================================================
--- linux-pm.orig/drivers/base/platform.c
+++ linux-pm/drivers/base/platform.c
@@ -888,7 +888,6 @@ int platform_pm_restore(struct device *d
 static const struct dev_pm_ops platform_dev_pm_ops = {
 	.runtime_suspend = pm_generic_runtime_suspend,
 	.runtime_resume = pm_generic_runtime_resume,
-	.runtime_idle = pm_generic_runtime_idle,
 	USE_PLATFORM_PM_SLEEP_OPS
 };
 
Index: linux-pm/drivers/i2c/i2c-core.c
===================================================================
--- linux-pm.orig/drivers/i2c/i2c-core.c
+++ linux-pm/drivers/i2c/i2c-core.c
@@ -435,7 +435,7 @@ static const struct dev_pm_ops i2c_devic
 	SET_RUNTIME_PM_OPS(
 		pm_generic_runtime_suspend,
 		pm_generic_runtime_resume,
-		pm_generic_runtime_idle
+		NULL
 	)
 };
 
Index: linux-pm/drivers/mmc/core/sdio_bus.c
===================================================================
--- linux-pm.orig/drivers/mmc/core/sdio_bus.c
+++ linux-pm/drivers/mmc/core/sdio_bus.c
@@ -211,7 +211,7 @@ static const struct dev_pm_ops sdio_bus_
 	SET_RUNTIME_PM_OPS(
 		pm_generic_runtime_suspend,
 		pm_generic_runtime_resume,
-		pm_generic_runtime_idle
+		NULL
 	)
 };
 
Index: linux-pm/drivers/spi/spi.c
===================================================================
--- linux-pm.orig/drivers/spi/spi.c
+++ linux-pm/drivers/spi/spi.c
@@ -223,7 +223,7 @@ static const struct dev_pm_ops spi_pm =
 	SET_RUNTIME_PM_OPS(
 		pm_generic_runtime_suspend,
 		pm_generic_runtime_resume,
-		pm_generic_runtime_idle
+		NULL
 	)
 };
 
Index: linux-pm/drivers/usb/core/port.c
===================================================================
--- linux-pm.orig/drivers/usb/core/port.c
+++ linux-pm/drivers/usb/core/port.c
@@ -141,7 +141,6 @@ static const struct dev_pm_ops usb_port_
 #ifdef CONFIG_PM_RUNTIME
 	.runtime_suspend =	usb_port_runtime_suspend,
 	.runtime_resume =	usb_port_runtime_resume,
-	.runtime_idle =		pm_generic_runtime_idle,
 #endif
 };
 
Index: linux-pm/Documentation/power/runtime_pm.txt
===================================================================
--- linux-pm.orig/Documentation/power/runtime_pm.txt
+++ linux-pm/Documentation/power/runtime_pm.txt
@@ -660,11 +660,6 @@ Subsystems may wish to conserve code spa
 management callbacks provided by the PM core, defined in
 driver/base/power/generic_ops.c:
 
-  int pm_generic_runtime_idle(struct device *dev);
-    - invoke the ->runtime_idle() callback provided by the driver of this
-      device, if defined, and call pm_runtime_suspend() for this device if the
-      return value is 0 or the callback is not defined
-
   int pm_generic_runtime_suspend(struct device *dev);
     - invoke the ->runtime_suspend() callback provided by the driver of this
       device and return its result, or return -EINVAL if not defined
Index: linux-pm/drivers/usb/core/driver.c
===================================================================
--- linux-pm.orig/drivers/usb/core/driver.c
+++ linux-pm/drivers/usb/core/driver.c
@@ -1765,7 +1765,8 @@ int usb_runtime_idle(struct device *dev)
 	 */
 	if (autosuspend_check(udev) == 0)
 		pm_runtime_autosuspend(dev);
-	return 0;
+	/* Tell the core not to suspend it, though. */
+	return -EBUSY;
 }
 
 int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable)
Index: linux-pm/drivers/pci/pci-driver.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-driver.c
+++ linux-pm/drivers/pci/pci-driver.c
@@ -1050,26 +1050,22 @@ static int pci_pm_runtime_idle(struct de
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	int ret = 0;
 
 	/*
 	 * If pci_dev->driver is not set (unbound), the device should
 	 * always remain in D0 regardless of the runtime PM status
 	 */
 	if (!pci_dev->driver)
-		goto out;
+		return 0;
 
 	if (!pm)
 		return -ENOSYS;
 
-	if (pm->runtime_idle) {
-		int ret = pm->runtime_idle(dev);
-		if (ret)
-			return ret;
-	}
+	if (pm->runtime_idle)
+		ret = pm->runtime_idle(dev);
 
-out:
-	pm_runtime_suspend(dev);
-	return 0;
+	return ret;
 }
 
 #else /* !CONFIG_PM_RUNTIME */
Index: linux-pm/drivers/ata/libata-core.c
===================================================================
--- linux-pm.orig/drivers/ata/libata-core.c
+++ linux-pm/drivers/ata/libata-core.c
@@ -5430,7 +5430,7 @@ static int ata_port_runtime_idle(struct
 				return -EBUSY;
 	}
 
-	return pm_runtime_suspend(dev);
+	return 0;
 }
 
 static int ata_port_runtime_suspend(struct device *dev)
Index: linux-pm/drivers/dma/intel_mid_dma.c
===================================================================
--- linux-pm.orig/drivers/dma/intel_mid_dma.c
+++ linux-pm/drivers/dma/intel_mid_dma.c
@@ -1405,7 +1405,7 @@ static int dma_runtime_idle(struct devic
 			return -EAGAIN;
 	}
 
-	return pm_schedule_suspend(dev, 0);
+	return 0;
 }
 
 /******************************************************************************
Index: linux-pm/drivers/gpio/gpio-langwell.c
===================================================================
--- linux-pm.orig/drivers/gpio/gpio-langwell.c
+++ linux-pm/drivers/gpio/gpio-langwell.c
@@ -305,11 +305,7 @@ static const struct irq_domain_ops lnw_g
 
 static int lnw_gpio_runtime_idle(struct device *dev)
 {
-	int err = pm_schedule_suspend(dev, 500);
-
-	if (!err)
-		return 0;
-
+	pm_schedule_suspend(dev, 500);
 	return -EBUSY;
 }
 
Index: linux-pm/drivers/mfd/ab8500-gpadc.c
===================================================================
--- linux-pm.orig/drivers/mfd/ab8500-gpadc.c
+++ linux-pm/drivers/mfd/ab8500-gpadc.c
@@ -886,12 +886,6 @@ static int ab8500_gpadc_runtime_resume(s
 	return ret;
 }
 
-static int ab8500_gpadc_runtime_idle(struct device *dev)
-{
-	pm_runtime_suspend(dev);
-	return 0;
-}
-
 static int ab8500_gpadc_suspend(struct device *dev)
 {
 	struct ab8500_gpadc *gpadc = dev_get_drvdata(dev);
@@ -1039,7 +1033,7 @@ static int ab8500_gpadc_remove(struct pl
 static const struct dev_pm_ops ab8500_gpadc_pm_ops = {
 	SET_RUNTIME_PM_OPS(ab8500_gpadc_runtime_suspend,
 			   ab8500_gpadc_runtime_resume,
-			   ab8500_gpadc_runtime_idle)
+			   NULL)
 	SET_SYSTEM_SLEEP_PM_OPS(ab8500_gpadc_suspend,
 				ab8500_gpadc_resume)
 
Index: linux-pm/drivers/mmc/core/bus.c
===================================================================
--- linux-pm.orig/drivers/mmc/core/bus.c
+++ linux-pm/drivers/mmc/core/bus.c
@@ -164,7 +164,7 @@ static int mmc_runtime_resume(struct dev
 
 static int mmc_runtime_idle(struct device *dev)
 {
-	return pm_runtime_suspend(dev);
+	return 0;
 }
 
 #endif /* !CONFIG_PM_RUNTIME */
Index: linux-pm/drivers/scsi/scsi_pm.c
===================================================================
--- linux-pm.orig/drivers/scsi/scsi_pm.c
+++ linux-pm/drivers/scsi/scsi_pm.c
@@ -229,8 +229,6 @@ static int scsi_runtime_resume(struct de
 
 static int scsi_runtime_idle(struct device *dev)
 {
-	int err;
-
 	dev_dbg(dev, "scsi_runtime_idle\n");
 
 	/* Insert hooks here for targets, hosts, and transport classes */
@@ -240,14 +238,11 @@ static int scsi_runtime_idle(struct devi
 
 		if (sdev->request_queue->dev) {
 			pm_runtime_mark_last_busy(dev);
-			err = pm_runtime_autosuspend(dev);
-		} else {
-			err = pm_runtime_suspend(dev);
+			pm_runtime_autosuspend(dev);
+			return -EBUSY;
 		}
-	} else {
-		err = pm_runtime_suspend(dev);
 	}
-	return err;
+	return 0;
 }
 
 int scsi_autopm_get_device(struct scsi_device *sdev)
Index: linux-pm/drivers/sh/pm_runtime.c
===================================================================
--- linux-pm.orig/drivers/sh/pm_runtime.c
+++ linux-pm/drivers/sh/pm_runtime.c
@@ -25,7 +25,7 @@
 static int default_platform_runtime_idle(struct device *dev)
 {
 	/* suspend synchronously to disable clocks immediately */
-	return pm_runtime_suspend(dev);
+	return 0;
 }
 
 static struct dev_pm_domain default_pm_domain = {
Index: linux-pm/drivers/tty/serial/mfd.c
===================================================================
--- linux-pm.orig/drivers/tty/serial/mfd.c
+++ linux-pm/drivers/tty/serial/mfd.c
@@ -1248,13 +1248,8 @@ static int serial_hsu_resume(struct pci_
 #ifdef CONFIG_PM_RUNTIME
 static int serial_hsu_runtime_idle(struct device *dev)
 {
-	int err;
-
-	err = pm_schedule_suspend(dev, 500);
-	if (err)
-		return -EBUSY;
-
-	return 0;
+	pm_schedule_suspend(dev, 500);
+	return -EBUSY;
 }
 
 static int serial_hsu_runtime_suspend(struct device *dev)

[PATCH 2/2] PM / Runtime: Update .runtime_idle() callback documentation

[Rafael J. Wysocki]

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

Runtime PM documentation needs to be updated after the previous
change of the rpm_idle() behavior, so modify it as appropriate.

[rjw: Subject and changelog]
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

Index: usb-3.10/Documentation/power/runtime_pm.txt
===================================================================
--- usb-3.10.orig/Documentation/power/runtime_pm.txt
+++ usb-3.10/Documentation/power/runtime_pm.txt
@@ -144,8 +144,12 @@ The action performed by the idle callbac
 (or driver) in question, but the expected and recommended action is to check
 if the device can be suspended (i.e. if all of the conditions necessary for
 suspending the device are satisfied) and to queue up a suspend request for the
-device in that case.  The value returned by this callback is ignored by the PM
-core.
+device in that case.  If there is no idle callback, or if the callback returns
+0, then the PM core will attempt to carry out a runtime suspend of the device;
+in essence, it will call pm_runtime_suspend() directly.  To prevent this (for
+example, if the callback routine has started a delayed suspend), the routine
+should return a non-zero value.  Negative error return codes are ignored by the
+PM core.
 
 The helper functions provided by the PM core, described in Section 4, guarantee
 that the following constraints are met with respect to runtime PM callbacks for
@@ -301,9 +305,10 @@ drivers/base/power/runtime.c and include
       removing the device from device hierarchy
 
   int pm_runtime_idle(struct device *dev);
-    - execute the subsystem-level idle callback for the device; returns 0 on
-      success or error code on failure, where -EINPROGRESS means that
-      ->runtime_idle() is already being executed
+    - execute the subsystem-level idle callback for the device; returns an
+      error code on failure, where -EINPROGRESS means that ->runtime_idle() is
+      already being executed; if there is no callback or the callback returns 0
+      then run pm_runtime_suspend(dev) and return its result
 
   int pm_runtime_suspend(struct device *dev);
     - execute the subsystem-level suspend callback for the device; returns 0 on

Re: [PATCH 1/2, v2] PM / Runtime: Rework the "runtime idle" helper routine

[Alan Stern]

On Sun, 2 Jun 2013, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The "runtime idle" helper routine, rpm_idle(), currently ignores
> return values from .runtime_idle() callbacks executed by it.
> However, it turns out that many subsystems use
> pm_generic_runtime_idle() which checks the return value of the
> driver's callback and executes pm_runtime_suspend() for the device
> unless that value is not 0.  If that logic is moved to rpm_idle()
> instead, pm_generic_runtime_idle() can be dropped and its users
> will not need any .runtime_idle() callbacks any more.
> 
> Moreover, the PCI, SCSI, and SATA subsystems' .runtime_idle()
> routines, pci_pm_runtime_idle(), scsi_runtime_idle(), and
> ata_port_runtime_idle(), respectively, as well as a few drivers'
> ones may be simplified if rpm_idle() calls rpm_suspend() after 0 has
> been returned by the .runtime_idle() callback executed by it.
> 
> To reduce overall code bloat, make the changes described above.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH 1/2, v2] PM / Runtime: Rework the "runtime idle" helper routine

[Rafael J. Wysocki]

On Monday, June 03, 2013 10:33:20 AM Alan Stern wrote:
> On Sun, 2 Jun 2013, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > The "runtime idle" helper routine, rpm_idle(), currently ignores
> > return values from .runtime_idle() callbacks executed by it.
> > However, it turns out that many subsystems use
> > pm_generic_runtime_idle() which checks the return value of the
> > driver's callback and executes pm_runtime_suspend() for the device
> > unless that value is not 0.  If that logic is moved to rpm_idle()
> > instead, pm_generic_runtime_idle() can be dropped and its users
> > will not need any .runtime_idle() callbacks any more.
> > 
> > Moreover, the PCI, SCSI, and SATA subsystems' .runtime_idle()
> > routines, pci_pm_runtime_idle(), scsi_runtime_idle(), and
> > ata_port_runtime_idle(), respectively, as well as a few drivers'
> > ones may be simplified if rpm_idle() calls rpm_suspend() after 0 has
> > been returned by the .runtime_idle() callback executed by it.
> > 
> > To reduce overall code bloat, make the changes described above.
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>

Thanks!

Re: [PATCH 1/2, v2] PM / Runtime: Rework the "runtime idle" helper routine

[Aaron Lu]

On 06/03/2013 05:52 AM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The "runtime idle" helper routine, rpm_idle(), currently ignores
> return values from .runtime_idle() callbacks executed by it.
> However, it turns out that many subsystems use
> pm_generic_runtime_idle() which checks the return value of the
> driver's callback and executes pm_runtime_suspend() for the device
> unless that value is not 0.  If that logic is moved to rpm_idle()
> instead, pm_generic_runtime_idle() can be dropped and its users
> will not need any .runtime_idle() callbacks any more.
> 
> Moreover, the PCI, SCSI, and SATA subsystems' .runtime_idle()
> routines, pci_pm_runtime_idle(), scsi_runtime_idle(), and
> ata_port_runtime_idle(), respectively, as well as a few drivers'
> ones may be simplified if rpm_idle() calls rpm_suspend() after 0 has
> been returned by the .runtime_idle() callback executed by it.

For ATA and SCSI part:
Reviewed-by: Aaron Lu <aaron.lu@intel.com>

Thanks,
Aaron

> 
> To reduce overall code bloat, make the changes described above.
> 
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Tested-by: Kevin Hilman <khilman@linaro.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Acked-by: Kevin Hilman <khilman@linaro.org>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  Documentation/power/runtime_pm.txt |    5 -----
>  arch/arm/mach-omap2/omap_device.c  |    7 +------
>  drivers/acpi/device_pm.c           |    1 -
>  drivers/amba/bus.c                 |    2 +-
>  drivers/ata/libata-core.c          |    2 +-
>  drivers/base/platform.c            |    1 -
>  drivers/base/power/domain.c        |    1 -
>  drivers/base/power/generic_ops.c   |   23 -----------------------
>  drivers/base/power/runtime.c       |   12 +++++-------
>  drivers/dma/intel_mid_dma.c        |    2 +-
>  drivers/gpio/gpio-langwell.c       |    6 +-----
>  drivers/i2c/i2c-core.c             |    2 +-
>  drivers/mfd/ab8500-gpadc.c         |    8 +-------
>  drivers/mmc/core/bus.c             |    2 +-
>  drivers/mmc/core/sdio_bus.c        |    2 +-
>  drivers/pci/pci-driver.c           |   14 +++++---------
>  drivers/scsi/scsi_pm.c             |   11 +++--------
>  drivers/sh/pm_runtime.c            |    2 +-
>  drivers/spi/spi.c                  |    2 +-
>  drivers/tty/serial/mfd.c           |    9 ++-------
>  drivers/usb/core/driver.c          |    3 ++-
>  drivers/usb/core/port.c            |    1 -
>  include/linux/pm_runtime.h         |    2 --
>  23 files changed, 28 insertions(+), 92 deletions(-)
> 
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -293,11 +293,8 @@ static int rpm_idle(struct device *dev,
>  	/* Pending requests need to be canceled. */
>  	dev->power.request = RPM_REQ_NONE;
>  
> -	if (dev->power.no_callbacks) {
> -		/* Assume ->runtime_idle() callback would have suspended. */
> -		retval = rpm_suspend(dev, rpmflags);
> +	if (dev->power.no_callbacks)
>  		goto out;
> -	}
>  
>  	/* Carry out an asynchronous or a synchronous idle notification. */
>  	if (rpmflags & RPM_ASYNC) {
> @@ -306,7 +303,8 @@ static int rpm_idle(struct device *dev,
>  			dev->power.request_pending = true;
>  			queue_work(pm_wq, &dev->power.work);
>  		}
> -		goto out;
> +		trace_rpm_return_int(dev, _THIS_IP_, 0);
> +		return 0;
>  	}
>  
>  	dev->power.idle_notification = true;
> @@ -326,14 +324,14 @@ static int rpm_idle(struct device *dev,
>  		callback = dev->driver->pm->runtime_idle;
>  
>  	if (callback)
> -		__rpm_callback(callback, dev);
> +		retval = __rpm_callback(callback, dev);
>  
>  	dev->power.idle_notification = false;
>  	wake_up_all(&dev->power.wait_queue);
>  
>   out:
>  	trace_rpm_return_int(dev, _THIS_IP_, retval);
> -	return retval;
> +	return retval ? retval : rpm_suspend(dev, rpmflags);
>  }
>  
>  /**
> Index: linux-pm/drivers/base/power/generic_ops.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/generic_ops.c
> +++ linux-pm/drivers/base/power/generic_ops.c
> @@ -12,29 +12,6 @@
>  
>  #ifdef CONFIG_PM_RUNTIME
>  /**
> - * pm_generic_runtime_idle - Generic runtime idle callback for subsystems.
> - * @dev: Device to handle.
> - *
> - * If PM operations are defined for the @dev's driver and they include
> - * ->runtime_idle(), execute it and return its error code, if nonzero.
> - * Otherwise, execute pm_runtime_suspend() for the device and return 0.
> - */
> -int pm_generic_runtime_idle(struct device *dev)
> -{
> -	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> -
> -	if (pm && pm->runtime_idle) {
> -		int ret = pm->runtime_idle(dev);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	pm_runtime_suspend(dev);
> -	return 0;
> -}
> -EXPORT_SYMBOL_GPL(pm_generic_runtime_idle);
> -
> -/**
>   * pm_generic_runtime_suspend - Generic runtime suspend callback for subsystems.
>   * @dev: Device to suspend.
>   *
> Index: linux-pm/include/linux/pm_runtime.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm_runtime.h
> +++ linux-pm/include/linux/pm_runtime.h
> @@ -37,7 +37,6 @@ extern void pm_runtime_enable(struct dev
>  extern void __pm_runtime_disable(struct device *dev, bool check_resume);
>  extern void pm_runtime_allow(struct device *dev);
>  extern void pm_runtime_forbid(struct device *dev);
> -extern int pm_generic_runtime_idle(struct device *dev);
>  extern int pm_generic_runtime_suspend(struct device *dev);
>  extern int pm_generic_runtime_resume(struct device *dev);
>  extern void pm_runtime_no_callbacks(struct device *dev);
> @@ -143,7 +142,6 @@ static inline bool pm_runtime_active(str
>  static inline bool pm_runtime_status_suspended(struct device *dev) { return false; }
>  static inline bool pm_runtime_enabled(struct device *dev) { return false; }
>  
> -static inline int pm_generic_runtime_idle(struct device *dev) { return 0; }
>  static inline int pm_generic_runtime_suspend(struct device *dev) { return 0; }
>  static inline int pm_generic_runtime_resume(struct device *dev) { return 0; }
>  static inline void pm_runtime_no_callbacks(struct device *dev) {}
> Index: linux-pm/arch/arm/mach-omap2/omap_device.c
> ===================================================================
> --- linux-pm.orig/arch/arm/mach-omap2/omap_device.c
> +++ linux-pm/arch/arm/mach-omap2/omap_device.c
> @@ -591,11 +591,6 @@ static int _od_runtime_suspend(struct de
>  	return ret;
>  }
>  
> -static int _od_runtime_idle(struct device *dev)
> -{
> -	return pm_generic_runtime_idle(dev);
> -}
> -
>  static int _od_runtime_resume(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> @@ -653,7 +648,7 @@ static int _od_resume_noirq(struct devic
>  struct dev_pm_domain omap_device_pm_domain = {
>  	.ops = {
>  		SET_RUNTIME_PM_OPS(_od_runtime_suspend, _od_runtime_resume,
> -				   _od_runtime_idle)
> +				   NULL)
>  		USE_PLATFORM_PM_SLEEP_OPS
>  		.suspend_noirq = _od_suspend_noirq,
>  		.resume_noirq = _od_resume_noirq,
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -886,7 +886,6 @@ static struct dev_pm_domain acpi_general
>  #ifdef CONFIG_PM_RUNTIME
>  		.runtime_suspend = acpi_subsys_runtime_suspend,
>  		.runtime_resume = acpi_subsys_runtime_resume,
> -		.runtime_idle = pm_generic_runtime_idle,
>  #endif
>  #ifdef CONFIG_PM_SLEEP
>  		.prepare = acpi_subsys_prepare,
> Index: linux-pm/drivers/amba/bus.c
> ===================================================================
> --- linux-pm.orig/drivers/amba/bus.c
> +++ linux-pm/drivers/amba/bus.c
> @@ -284,7 +284,7 @@ static const struct dev_pm_ops amba_pm =
>  	SET_RUNTIME_PM_OPS(
>  		amba_pm_runtime_suspend,
>  		amba_pm_runtime_resume,
> -		pm_generic_runtime_idle
> +		NULL
>  	)
>  };
>  
> Index: linux-pm/drivers/base/power/domain.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/domain.c
> +++ linux-pm/drivers/base/power/domain.c
> @@ -2143,7 +2143,6 @@ void pm_genpd_init(struct generic_pm_dom
>  	genpd->max_off_time_changed = true;
>  	genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend;
>  	genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume;
> -	genpd->domain.ops.runtime_idle = pm_generic_runtime_idle;
>  	genpd->domain.ops.prepare = pm_genpd_prepare;
>  	genpd->domain.ops.suspend = pm_genpd_suspend;
>  	genpd->domain.ops.suspend_late = pm_genpd_suspend_late;
> Index: linux-pm/drivers/base/platform.c
> ===================================================================
> --- linux-pm.orig/drivers/base/platform.c
> +++ linux-pm/drivers/base/platform.c
> @@ -888,7 +888,6 @@ int platform_pm_restore(struct device *d
>  static const struct dev_pm_ops platform_dev_pm_ops = {
>  	.runtime_suspend = pm_generic_runtime_suspend,
>  	.runtime_resume = pm_generic_runtime_resume,
> -	.runtime_idle = pm_generic_runtime_idle,
>  	USE_PLATFORM_PM_SLEEP_OPS
>  };
>  
> Index: linux-pm/drivers/i2c/i2c-core.c
> ===================================================================
> --- linux-pm.orig/drivers/i2c/i2c-core.c
> +++ linux-pm/drivers/i2c/i2c-core.c
> @@ -435,7 +435,7 @@ static const struct dev_pm_ops i2c_devic
>  	SET_RUNTIME_PM_OPS(
>  		pm_generic_runtime_suspend,
>  		pm_generic_runtime_resume,
> -		pm_generic_runtime_idle
> +		NULL
>  	)
>  };
>  
> Index: linux-pm/drivers/mmc/core/sdio_bus.c
> ===================================================================
> --- linux-pm.orig/drivers/mmc/core/sdio_bus.c
> +++ linux-pm/drivers/mmc/core/sdio_bus.c
> @@ -211,7 +211,7 @@ static const struct dev_pm_ops sdio_bus_
>  	SET_RUNTIME_PM_OPS(
>  		pm_generic_runtime_suspend,
>  		pm_generic_runtime_resume,
> -		pm_generic_runtime_idle
> +		NULL
>  	)
>  };
>  
> Index: linux-pm/drivers/spi/spi.c
> ===================================================================
> --- linux-pm.orig/drivers/spi/spi.c
> +++ linux-pm/drivers/spi/spi.c
> @@ -223,7 +223,7 @@ static const struct dev_pm_ops spi_pm =
>  	SET_RUNTIME_PM_OPS(
>  		pm_generic_runtime_suspend,
>  		pm_generic_runtime_resume,
> -		pm_generic_runtime_idle
> +		NULL
>  	)
>  };
>  
> Index: linux-pm/drivers/usb/core/port.c
> ===================================================================
> --- linux-pm.orig/drivers/usb/core/port.c
> +++ linux-pm/drivers/usb/core/port.c
> @@ -141,7 +141,6 @@ static const struct dev_pm_ops usb_port_
>  #ifdef CONFIG_PM_RUNTIME
>  	.runtime_suspend =	usb_port_runtime_suspend,
>  	.runtime_resume =	usb_port_runtime_resume,
> -	.runtime_idle =		pm_generic_runtime_idle,
>  #endif
>  };
>  
> Index: linux-pm/Documentation/power/runtime_pm.txt
> ===================================================================
> --- linux-pm.orig/Documentation/power/runtime_pm.txt
> +++ linux-pm/Documentation/power/runtime_pm.txt
> @@ -660,11 +660,6 @@ Subsystems may wish to conserve code spa
>  management callbacks provided by the PM core, defined in
>  driver/base/power/generic_ops.c:
>  
> -  int pm_generic_runtime_idle(struct device *dev);
> -    - invoke the ->runtime_idle() callback provided by the driver of this
> -      device, if defined, and call pm_runtime_suspend() for this device if the
> -      return value is 0 or the callback is not defined
> -
>    int pm_generic_runtime_suspend(struct device *dev);
>      - invoke the ->runtime_suspend() callback provided by the driver of this
>        device and return its result, or return -EINVAL if not defined
> Index: linux-pm/drivers/usb/core/driver.c
> ===================================================================
> --- linux-pm.orig/drivers/usb/core/driver.c
> +++ linux-pm/drivers/usb/core/driver.c
> @@ -1765,7 +1765,8 @@ int usb_runtime_idle(struct device *dev)
>  	 */
>  	if (autosuspend_check(udev) == 0)
>  		pm_runtime_autosuspend(dev);
> -	return 0;
> +	/* Tell the core not to suspend it, though. */
> +	return -EBUSY;
>  }
>  
>  int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable)
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -1050,26 +1050,22 @@ static int pci_pm_runtime_idle(struct de
>  {
>  	struct pci_dev *pci_dev = to_pci_dev(dev);
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +	int ret = 0;
>  
>  	/*
>  	 * If pci_dev->driver is not set (unbound), the device should
>  	 * always remain in D0 regardless of the runtime PM status
>  	 */
>  	if (!pci_dev->driver)
> -		goto out;
> +		return 0;
>  
>  	if (!pm)
>  		return -ENOSYS;
>  
> -	if (pm->runtime_idle) {
> -		int ret = pm->runtime_idle(dev);
> -		if (ret)
> -			return ret;
> -	}
> +	if (pm->runtime_idle)
> +		ret = pm->runtime_idle(dev);
>  
> -out:
> -	pm_runtime_suspend(dev);
> -	return 0;
> +	return ret;
>  }
>  
>  #else /* !CONFIG_PM_RUNTIME */
> Index: linux-pm/drivers/ata/libata-core.c
> ===================================================================
> --- linux-pm.orig/drivers/ata/libata-core.c
> +++ linux-pm/drivers/ata/libata-core.c
> @@ -5430,7 +5430,7 @@ static int ata_port_runtime_idle(struct
>  				return -EBUSY;
>  	}
>  
> -	return pm_runtime_suspend(dev);
> +	return 0;
>  }
>  
>  static int ata_port_runtime_suspend(struct device *dev)
> Index: linux-pm/drivers/dma/intel_mid_dma.c
> ===================================================================
> --- linux-pm.orig/drivers/dma/intel_mid_dma.c
> +++ linux-pm/drivers/dma/intel_mid_dma.c
> @@ -1405,7 +1405,7 @@ static int dma_runtime_idle(struct devic
>  			return -EAGAIN;
>  	}
>  
> -	return pm_schedule_suspend(dev, 0);
> +	return 0;
>  }
>  
>  /******************************************************************************
> Index: linux-pm/drivers/gpio/gpio-langwell.c
> ===================================================================
> --- linux-pm.orig/drivers/gpio/gpio-langwell.c
> +++ linux-pm/drivers/gpio/gpio-langwell.c
> @@ -305,11 +305,7 @@ static const struct irq_domain_ops lnw_g
>  
>  static int lnw_gpio_runtime_idle(struct device *dev)
>  {
> -	int err = pm_schedule_suspend(dev, 500);
> -
> -	if (!err)
> -		return 0;
> -
> +	pm_schedule_suspend(dev, 500);
>  	return -EBUSY;
>  }
>  
> Index: linux-pm/drivers/mfd/ab8500-gpadc.c
> ===================================================================
> --- linux-pm.orig/drivers/mfd/ab8500-gpadc.c
> +++ linux-pm/drivers/mfd/ab8500-gpadc.c
> @@ -886,12 +886,6 @@ static int ab8500_gpadc_runtime_resume(s
>  	return ret;
>  }
>  
> -static int ab8500_gpadc_runtime_idle(struct device *dev)
> -{
> -	pm_runtime_suspend(dev);
> -	return 0;
> -}
> -
>  static int ab8500_gpadc_suspend(struct device *dev)
>  {
>  	struct ab8500_gpadc *gpadc = dev_get_drvdata(dev);
> @@ -1039,7 +1033,7 @@ static int ab8500_gpadc_remove(struct pl
>  static const struct dev_pm_ops ab8500_gpadc_pm_ops = {
>  	SET_RUNTIME_PM_OPS(ab8500_gpadc_runtime_suspend,
>  			   ab8500_gpadc_runtime_resume,
> -			   ab8500_gpadc_runtime_idle)
> +			   NULL)
>  	SET_SYSTEM_SLEEP_PM_OPS(ab8500_gpadc_suspend,
>  				ab8500_gpadc_resume)
>  
> Index: linux-pm/drivers/mmc/core/bus.c
> ===================================================================
> --- linux-pm.orig/drivers/mmc/core/bus.c
> +++ linux-pm/drivers/mmc/core/bus.c
> @@ -164,7 +164,7 @@ static int mmc_runtime_resume(struct dev
>  
>  static int mmc_runtime_idle(struct device *dev)
>  {
> -	return pm_runtime_suspend(dev);
> +	return 0;
>  }
>  
>  #endif /* !CONFIG_PM_RUNTIME */
> Index: linux-pm/drivers/scsi/scsi_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/scsi/scsi_pm.c
> +++ linux-pm/drivers/scsi/scsi_pm.c
> @@ -229,8 +229,6 @@ static int scsi_runtime_resume(struct de
>  
>  static int scsi_runtime_idle(struct device *dev)
>  {
> -	int err;
> -
>  	dev_dbg(dev, "scsi_runtime_idle\n");
>  
>  	/* Insert hooks here for targets, hosts, and transport classes */
> @@ -240,14 +238,11 @@ static int scsi_runtime_idle(struct devi
>  
>  		if (sdev->request_queue->dev) {
>  			pm_runtime_mark_last_busy(dev);
> -			err = pm_runtime_autosuspend(dev);
> -		} else {
> -			err = pm_runtime_suspend(dev);
> +			pm_runtime_autosuspend(dev);
> +			return -EBUSY;
>  		}
> -	} else {
> -		err = pm_runtime_suspend(dev);
>  	}
> -	return err;
> +	return 0;
>  }
>  
>  int scsi_autopm_get_device(struct scsi_device *sdev)
> Index: linux-pm/drivers/sh/pm_runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/sh/pm_runtime.c
> +++ linux-pm/drivers/sh/pm_runtime.c
> @@ -25,7 +25,7 @@
>  static int default_platform_runtime_idle(struct device *dev)
>  {
>  	/* suspend synchronously to disable clocks immediately */
> -	return pm_runtime_suspend(dev);
> +	return 0;
>  }
>  
>  static struct dev_pm_domain default_pm_domain = {
> Index: linux-pm/drivers/tty/serial/mfd.c
> ===================================================================
> --- linux-pm.orig/drivers/tty/serial/mfd.c
> +++ linux-pm/drivers/tty/serial/mfd.c
> @@ -1248,13 +1248,8 @@ static int serial_hsu_resume(struct pci_
>  #ifdef CONFIG_PM_RUNTIME
>  static int serial_hsu_runtime_idle(struct device *dev)
>  {
> -	int err;
> -
> -	err = pm_schedule_suspend(dev, 500);
> -	if (err)
> -		return -EBUSY;
> -
> -	return 0;
> +	pm_schedule_suspend(dev, 500);
> +	return -EBUSY;
>  }
>  
>  static int serial_hsu_runtime_suspend(struct device *dev)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/2, v2] PM / Runtime: Rework the "runtime idle" helper routine

[Lan Tianyu]

2013/6/3 Rafael J. Wysocki <rjw@sisk.pl>:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The "runtime idle" helper routine, rpm_idle(), currently ignores
> return values from .runtime_idle() callbacks executed by it.
> However, it turns out that many subsystems use
> pm_generic_runtime_idle() which checks the return value of the
> driver's callback and executes pm_runtime_suspend() for the device
> unless that value is not 0.  If that logic is moved to rpm_idle()
> instead, pm_generic_runtime_idle() can be dropped and its users
> will not need any .runtime_idle() callbacks any more.
>
> Moreover, the PCI, SCSI, and SATA subsystems' .runtime_idle()
> routines, pci_pm_runtime_idle(), scsi_runtime_idle(), and
> ata_port_runtime_idle(), respectively, as well as a few drivers'
> ones may be simplified if rpm_idle() calls rpm_suspend() after 0 has
> been returned by the .runtime_idle() callback executed by it.
>
> To reduce overall code bloat, make the changes described above.
>
Usb runtime pm works normally with this patch.
Tested-by: Lan Tianyu <tianyu.lan@intel.com>

> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Tested-by: Kevin Hilman <khilman@linaro.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Acked-by: Kevin Hilman <khilman@linaro.org>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  Documentation/power/runtime_pm.txt |    5 -----
>  arch/arm/mach-omap2/omap_device.c  |    7 +------
>  drivers/acpi/device_pm.c           |    1 -
>  drivers/amba/bus.c                 |    2 +-
>  drivers/ata/libata-core.c          |    2 +-
>  drivers/base/platform.c            |    1 -
>  drivers/base/power/domain.c        |    1 -
>  drivers/base/power/generic_ops.c   |   23 -----------------------
>  drivers/base/power/runtime.c       |   12 +++++-------
>  drivers/dma/intel_mid_dma.c        |    2 +-
>  drivers/gpio/gpio-langwell.c       |    6 +-----
>  drivers/i2c/i2c-core.c             |    2 +-
>  drivers/mfd/ab8500-gpadc.c         |    8 +-------
>  drivers/mmc/core/bus.c             |    2 +-
>  drivers/mmc/core/sdio_bus.c        |    2 +-
>  drivers/pci/pci-driver.c           |   14 +++++---------
>  drivers/scsi/scsi_pm.c             |   11 +++--------
>  drivers/sh/pm_runtime.c            |    2 +-
>  drivers/spi/spi.c                  |    2 +-
>  drivers/tty/serial/mfd.c           |    9 ++-------
>  drivers/usb/core/driver.c          |    3 ++-
>  drivers/usb/core/port.c            |    1 -
>  include/linux/pm_runtime.h         |    2 --
>  23 files changed, 28 insertions(+), 92 deletions(-)
>
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -293,11 +293,8 @@ static int rpm_idle(struct device *dev,
>         /* Pending requests need to be canceled. */
>         dev->power.request = RPM_REQ_NONE;
>
> -       if (dev->power.no_callbacks) {
> -               /* Assume ->runtime_idle() callback would have suspended. */
> -               retval = rpm_suspend(dev, rpmflags);
> +       if (dev->power.no_callbacks)
>                 goto out;
> -       }
>
>         /* Carry out an asynchronous or a synchronous idle notification. */
>         if (rpmflags & RPM_ASYNC) {
> @@ -306,7 +303,8 @@ static int rpm_idle(struct device *dev,
>                         dev->power.request_pending = true;
>                         queue_work(pm_wq, &dev->power.work);
>                 }
> -               goto out;
> +               trace_rpm_return_int(dev, _THIS_IP_, 0);
> +               return 0;
>         }
>
>         dev->power.idle_notification = true;
> @@ -326,14 +324,14 @@ static int rpm_idle(struct device *dev,
>                 callback = dev->driver->pm->runtime_idle;
>
>         if (callback)
> -               __rpm_callback(callback, dev);
> +               retval = __rpm_callback(callback, dev);
>
>         dev->power.idle_notification = false;
>         wake_up_all(&dev->power.wait_queue);
>
>   out:
>         trace_rpm_return_int(dev, _THIS_IP_, retval);
> -       return retval;
> +       return retval ? retval : rpm_suspend(dev, rpmflags);
>  }
>
>  /**
> Index: linux-pm/drivers/base/power/generic_ops.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/generic_ops.c
> +++ linux-pm/drivers/base/power/generic_ops.c
> @@ -12,29 +12,6 @@
>
>  #ifdef CONFIG_PM_RUNTIME
>  /**
> - * pm_generic_runtime_idle - Generic runtime idle callback for subsystems.
> - * @dev: Device to handle.
> - *
> - * If PM operations are defined for the @dev's driver and they include
> - * ->runtime_idle(), execute it and return its error code, if nonzero.
> - * Otherwise, execute pm_runtime_suspend() for the device and return 0.
> - */
> -int pm_generic_runtime_idle(struct device *dev)
> -{
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> -
> -       if (pm && pm->runtime_idle) {
> -               int ret = pm->runtime_idle(dev);
> -               if (ret)
> -                       return ret;
> -       }
> -
> -       pm_runtime_suspend(dev);
> -       return 0;
> -}
> -EXPORT_SYMBOL_GPL(pm_generic_runtime_idle);
> -
> -/**
>   * pm_generic_runtime_suspend - Generic runtime suspend callback for subsystems.
>   * @dev: Device to suspend.
>   *
> Index: linux-pm/include/linux/pm_runtime.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm_runtime.h
> +++ linux-pm/include/linux/pm_runtime.h
> @@ -37,7 +37,6 @@ extern void pm_runtime_enable(struct dev
>  extern void __pm_runtime_disable(struct device *dev, bool check_resume);
>  extern void pm_runtime_allow(struct device *dev);
>  extern void pm_runtime_forbid(struct device *dev);
> -extern int pm_generic_runtime_idle(struct device *dev);
>  extern int pm_generic_runtime_suspend(struct device *dev);
>  extern int pm_generic_runtime_resume(struct device *dev);
>  extern void pm_runtime_no_callbacks(struct device *dev);
> @@ -143,7 +142,6 @@ static inline bool pm_runtime_active(str
>  static inline bool pm_runtime_status_suspended(struct device *dev) { return false; }
>  static inline bool pm_runtime_enabled(struct device *dev) { return false; }
>
> -static inline int pm_generic_runtime_idle(struct device *dev) { return 0; }
>  static inline int pm_generic_runtime_suspend(struct device *dev) { return 0; }
>  static inline int pm_generic_runtime_resume(struct device *dev) { return 0; }
>  static inline void pm_runtime_no_callbacks(struct device *dev) {}
> Index: linux-pm/arch/arm/mach-omap2/omap_device.c
> ===================================================================
> --- linux-pm.orig/arch/arm/mach-omap2/omap_device.c
> +++ linux-pm/arch/arm/mach-omap2/omap_device.c
> @@ -591,11 +591,6 @@ static int _od_runtime_suspend(struct de
>         return ret;
>  }
>
> -static int _od_runtime_idle(struct device *dev)
> -{
> -       return pm_generic_runtime_idle(dev);
> -}
> -
>  static int _od_runtime_resume(struct device *dev)
>  {
>         struct platform_device *pdev = to_platform_device(dev);
> @@ -653,7 +648,7 @@ static int _od_resume_noirq(struct devic
>  struct dev_pm_domain omap_device_pm_domain = {
>         .ops = {
>                 SET_RUNTIME_PM_OPS(_od_runtime_suspend, _od_runtime_resume,
> -                                  _od_runtime_idle)
> +                                  NULL)
>                 USE_PLATFORM_PM_SLEEP_OPS
>                 .suspend_noirq = _od_suspend_noirq,
>                 .resume_noirq = _od_resume_noirq,
> Index: linux-pm/drivers/acpi/device_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/acpi/device_pm.c
> +++ linux-pm/drivers/acpi/device_pm.c
> @@ -886,7 +886,6 @@ static struct dev_pm_domain acpi_general
>  #ifdef CONFIG_PM_RUNTIME
>                 .runtime_suspend = acpi_subsys_runtime_suspend,
>                 .runtime_resume = acpi_subsys_runtime_resume,
> -               .runtime_idle = pm_generic_runtime_idle,
>  #endif
>  #ifdef CONFIG_PM_SLEEP
>                 .prepare = acpi_subsys_prepare,
> Index: linux-pm/drivers/amba/bus.c
> ===================================================================
> --- linux-pm.orig/drivers/amba/bus.c
> +++ linux-pm/drivers/amba/bus.c
> @@ -284,7 +284,7 @@ static const struct dev_pm_ops amba_pm =
>         SET_RUNTIME_PM_OPS(
>                 amba_pm_runtime_suspend,
>                 amba_pm_runtime_resume,
> -               pm_generic_runtime_idle
> +               NULL
>         )
>  };
>
> Index: linux-pm/drivers/base/power/domain.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/domain.c
> +++ linux-pm/drivers/base/power/domain.c
> @@ -2143,7 +2143,6 @@ void pm_genpd_init(struct generic_pm_dom
>         genpd->max_off_time_changed = true;
>         genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend;
>         genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume;
> -       genpd->domain.ops.runtime_idle = pm_generic_runtime_idle;
>         genpd->domain.ops.prepare = pm_genpd_prepare;
>         genpd->domain.ops.suspend = pm_genpd_suspend;
>         genpd->domain.ops.suspend_late = pm_genpd_suspend_late;
> Index: linux-pm/drivers/base/platform.c
> ===================================================================
> --- linux-pm.orig/drivers/base/platform.c
> +++ linux-pm/drivers/base/platform.c
> @@ -888,7 +888,6 @@ int platform_pm_restore(struct device *d
>  static const struct dev_pm_ops platform_dev_pm_ops = {
>         .runtime_suspend = pm_generic_runtime_suspend,
>         .runtime_resume = pm_generic_runtime_resume,
> -       .runtime_idle = pm_generic_runtime_idle,
>         USE_PLATFORM_PM_SLEEP_OPS
>  };
>
> Index: linux-pm/drivers/i2c/i2c-core.c
> ===================================================================
> --- linux-pm.orig/drivers/i2c/i2c-core.c
> +++ linux-pm/drivers/i2c/i2c-core.c
> @@ -435,7 +435,7 @@ static const struct dev_pm_ops i2c_devic
>         SET_RUNTIME_PM_OPS(
>                 pm_generic_runtime_suspend,
>                 pm_generic_runtime_resume,
> -               pm_generic_runtime_idle
> +               NULL
>         )
>  };
>
> Index: linux-pm/drivers/mmc/core/sdio_bus.c
> ===================================================================
> --- linux-pm.orig/drivers/mmc/core/sdio_bus.c
> +++ linux-pm/drivers/mmc/core/sdio_bus.c
> @@ -211,7 +211,7 @@ static const struct dev_pm_ops sdio_bus_
>         SET_RUNTIME_PM_OPS(
>                 pm_generic_runtime_suspend,
>                 pm_generic_runtime_resume,
> -               pm_generic_runtime_idle
> +               NULL
>         )
>  };
>
> Index: linux-pm/drivers/spi/spi.c
> ===================================================================
> --- linux-pm.orig/drivers/spi/spi.c
> +++ linux-pm/drivers/spi/spi.c
> @@ -223,7 +223,7 @@ static const struct dev_pm_ops spi_pm =
>         SET_RUNTIME_PM_OPS(
>                 pm_generic_runtime_suspend,
>                 pm_generic_runtime_resume,
> -               pm_generic_runtime_idle
> +               NULL
>         )
>  };
>
> Index: linux-pm/drivers/usb/core/port.c
> ===================================================================
> --- linux-pm.orig/drivers/usb/core/port.c
> +++ linux-pm/drivers/usb/core/port.c
> @@ -141,7 +141,6 @@ static const struct dev_pm_ops usb_port_
>  #ifdef CONFIG_PM_RUNTIME
>         .runtime_suspend =      usb_port_runtime_suspend,
>         .runtime_resume =       usb_port_runtime_resume,
> -       .runtime_idle =         pm_generic_runtime_idle,
>  #endif
>  };
>
> Index: linux-pm/Documentation/power/runtime_pm.txt
> ===================================================================
> --- linux-pm.orig/Documentation/power/runtime_pm.txt
> +++ linux-pm/Documentation/power/runtime_pm.txt
> @@ -660,11 +660,6 @@ Subsystems may wish to conserve code spa
>  management callbacks provided by the PM core, defined in
>  driver/base/power/generic_ops.c:
>
> -  int pm_generic_runtime_idle(struct device *dev);
> -    - invoke the ->runtime_idle() callback provided by the driver of this
> -      device, if defined, and call pm_runtime_suspend() for this device if the
> -      return value is 0 or the callback is not defined
> -
>    int pm_generic_runtime_suspend(struct device *dev);
>      - invoke the ->runtime_suspend() callback provided by the driver of this
>        device and return its result, or return -EINVAL if not defined
> Index: linux-pm/drivers/usb/core/driver.c
> ===================================================================
> --- linux-pm.orig/drivers/usb/core/driver.c
> +++ linux-pm/drivers/usb/core/driver.c
> @@ -1765,7 +1765,8 @@ int usb_runtime_idle(struct device *dev)
>          */
>         if (autosuspend_check(udev) == 0)
>                 pm_runtime_autosuspend(dev);
> -       return 0;
> +       /* Tell the core not to suspend it, though. */
> +       return -EBUSY;
>  }
>
>  int usb_set_usb2_hardware_lpm(struct usb_device *udev, int enable)
> Index: linux-pm/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-driver.c
> +++ linux-pm/drivers/pci/pci-driver.c
> @@ -1050,26 +1050,22 @@ static int pci_pm_runtime_idle(struct de
>  {
>         struct pci_dev *pci_dev = to_pci_dev(dev);
>         const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       int ret = 0;
>
>         /*
>          * If pci_dev->driver is not set (unbound), the device should
>          * always remain in D0 regardless of the runtime PM status
>          */
>         if (!pci_dev->driver)
> -               goto out;
> +               return 0;
>
>         if (!pm)
>                 return -ENOSYS;
>
> -       if (pm->runtime_idle) {
> -               int ret = pm->runtime_idle(dev);
> -               if (ret)
> -                       return ret;
> -       }
> +       if (pm->runtime_idle)
> +               ret = pm->runtime_idle(dev);
>
> -out:
> -       pm_runtime_suspend(dev);
> -       return 0;
> +       return ret;
>  }
>
>  #else /* !CONFIG_PM_RUNTIME */
> Index: linux-pm/drivers/ata/libata-core.c
> ===================================================================
> --- linux-pm.orig/drivers/ata/libata-core.c
> +++ linux-pm/drivers/ata/libata-core.c
> @@ -5430,7 +5430,7 @@ static int ata_port_runtime_idle(struct
>                                 return -EBUSY;
>         }
>
> -       return pm_runtime_suspend(dev);
> +       return 0;
>  }
>
>  static int ata_port_runtime_suspend(struct device *dev)
> Index: linux-pm/drivers/dma/intel_mid_dma.c
> ===================================================================
> --- linux-pm.orig/drivers/dma/intel_mid_dma.c
> +++ linux-pm/drivers/dma/intel_mid_dma.c
> @@ -1405,7 +1405,7 @@ static int dma_runtime_idle(struct devic
>                         return -EAGAIN;
>         }
>
> -       return pm_schedule_suspend(dev, 0);
> +       return 0;
>  }
>
>  /******************************************************************************
> Index: linux-pm/drivers/gpio/gpio-langwell.c
> ===================================================================
> --- linux-pm.orig/drivers/gpio/gpio-langwell.c
> +++ linux-pm/drivers/gpio/gpio-langwell.c
> @@ -305,11 +305,7 @@ static const struct irq_domain_ops lnw_g
>
>  static int lnw_gpio_runtime_idle(struct device *dev)
>  {
> -       int err = pm_schedule_suspend(dev, 500);
> -
> -       if (!err)
> -               return 0;
> -
> +       pm_schedule_suspend(dev, 500);
>         return -EBUSY;
>  }
>
> Index: linux-pm/drivers/mfd/ab8500-gpadc.c
> ===================================================================
> --- linux-pm.orig/drivers/mfd/ab8500-gpadc.c
> +++ linux-pm/drivers/mfd/ab8500-gpadc.c
> @@ -886,12 +886,6 @@ static int ab8500_gpadc_runtime_resume(s
>         return ret;
>  }
>
> -static int ab8500_gpadc_runtime_idle(struct device *dev)
> -{
> -       pm_runtime_suspend(dev);
> -       return 0;
> -}
> -
>  static int ab8500_gpadc_suspend(struct device *dev)
>  {
>         struct ab8500_gpadc *gpadc = dev_get_drvdata(dev);
> @@ -1039,7 +1033,7 @@ static int ab8500_gpadc_remove(struct pl
>  static const struct dev_pm_ops ab8500_gpadc_pm_ops = {
>         SET_RUNTIME_PM_OPS(ab8500_gpadc_runtime_suspend,
>                            ab8500_gpadc_runtime_resume,
> -                          ab8500_gpadc_runtime_idle)
> +                          NULL)
>         SET_SYSTEM_SLEEP_PM_OPS(ab8500_gpadc_suspend,
>                                 ab8500_gpadc_resume)
>
> Index: linux-pm/drivers/mmc/core/bus.c
> ===================================================================
> --- linux-pm.orig/drivers/mmc/core/bus.c
> +++ linux-pm/drivers/mmc/core/bus.c
> @@ -164,7 +164,7 @@ static int mmc_runtime_resume(struct dev
>
>  static int mmc_runtime_idle(struct device *dev)
>  {
> -       return pm_runtime_suspend(dev);
> +       return 0;
>  }
>
>  #endif /* !CONFIG_PM_RUNTIME */
> Index: linux-pm/drivers/scsi/scsi_pm.c
> ===================================================================
> --- linux-pm.orig/drivers/scsi/scsi_pm.c
> +++ linux-pm/drivers/scsi/scsi_pm.c
> @@ -229,8 +229,6 @@ static int scsi_runtime_resume(struct de
>
>  static int scsi_runtime_idle(struct device *dev)
>  {
> -       int err;
> -
>         dev_dbg(dev, "scsi_runtime_idle\n");
>
>         /* Insert hooks here for targets, hosts, and transport classes */
> @@ -240,14 +238,11 @@ static int scsi_runtime_idle(struct devi
>
>                 if (sdev->request_queue->dev) {
>                         pm_runtime_mark_last_busy(dev);
> -                       err = pm_runtime_autosuspend(dev);
> -               } else {
> -                       err = pm_runtime_suspend(dev);
> +                       pm_runtime_autosuspend(dev);
> +                       return -EBUSY;
>                 }
> -       } else {
> -               err = pm_runtime_suspend(dev);
>         }
> -       return err;
> +       return 0;
>  }
>
>  int scsi_autopm_get_device(struct scsi_device *sdev)
> Index: linux-pm/drivers/sh/pm_runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/sh/pm_runtime.c
> +++ linux-pm/drivers/sh/pm_runtime.c
> @@ -25,7 +25,7 @@
>  static int default_platform_runtime_idle(struct device *dev)
>  {
>         /* suspend synchronously to disable clocks immediately */
> -       return pm_runtime_suspend(dev);
> +       return 0;
>  }
>
>  static struct dev_pm_domain default_pm_domain = {
> Index: linux-pm/drivers/tty/serial/mfd.c
> ===================================================================
> --- linux-pm.orig/drivers/tty/serial/mfd.c
> +++ linux-pm/drivers/tty/serial/mfd.c
> @@ -1248,13 +1248,8 @@ static int serial_hsu_resume(struct pci_
>  #ifdef CONFIG_PM_RUNTIME
>  static int serial_hsu_runtime_idle(struct device *dev)
>  {
> -       int err;
> -
> -       err = pm_schedule_suspend(dev, 500);
> -       if (err)
> -               return -EBUSY;
> -
> -       return 0;
> +       pm_schedule_suspend(dev, 500);
> +       return -EBUSY;
>  }
>
>  static int serial_hsu_runtime_suspend(struct device *dev)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best regards
Tianyu Lan