[PATCH v11 0/4] Allow USB devices to remain runtime-suspended when sleeping

[PATCH v11 0/4] Allow USB devices to remain runtime-suspended when sleeping

[Tomeu Vizoso]

Hi,

this is v11 of an attempt to make it easier for devices to remain in
runtime PM when the system goes to sleep, mainly to reduce the time
spent resuming devices.

For this, we interpret the absence of all PM callback implementations as
it being safe to do direct_complete, so their ancestors aren't prevented
from remaining runtime-suspended.

Additionally, the prepare() callback of USB devices will return 1 if
runtime PM is enabled and the current wakeup settings are correct.

With these changes, a uvcvideo device (for example) stays in runtime
suspend when the system goes to sleep and is left in that state when the
system resumes, not delaying it unnecessarily.

Thanks,

Tomeu

Changes in v11:
- Move calls to dev_pm_domain_set() out from &dev->power.lock as that
  isn't needed for dev->pm_domain.

Changes in v10:
- Remove superfluous call to pm_runtime_enabled() as suggested by Alan

Changes in v9:
- Add docs noting the need for the device lock to be held before calling
  device_is_bound()
- Add docs noting the need for the device lock to be held before calling
  dev_pm_domain_set()
- Move to CONFIG_PM_SLEEP as suggested by Rafael and Ulf.
- Rename from device_check_pm_callbacks to device_pm_check_callbacks to
  follow with the naming convention of existing API.
- Re-add calling to device_pm_check_callbacks from device registration
  and when updating the PM domain of a device.

Changes in v8:
- Add device_is_bound()
- Add dev_pm_domain_set() and update code to use it
- Move no_pm_callbacks field into CONFIG_PM_SLEEP
- Call device_check_pm_callbacks only after a device is bound or unbound

Changes in v7:
- Reduce indentation by adding a label in device_prepare()

Changes in v6:
- Add stub for !CONFIG_PM.
- Move implementation of device_check_pm_callbacks to power/main.c as it
  doesn't belong to CONFIG_PM_SLEEP.
- Take dev->power.lock before modifying flag.

Changes in v5:
- Check for all dev_pm_ops instances associated to a device, updating a
  no_pm_callbacks flag at the times when that could change.

Tomeu Vizoso (4):
  device core: add device_is_bound()
  PM / Domains: add setter for dev.pm_domain
  PM / sleep: Go direct_complete if driver has no callbacks
  USB / PM: Allow USB devices to remain runtime-suspended when sleeping

 arch/arm/mach-omap2/omap_device.c |  7 ++++---
 drivers/acpi/acpi_lpss.c          |  5 +++--
 drivers/acpi/device_pm.c          |  5 +++--
 drivers/base/dd.c                 | 21 +++++++++++++++++++--
 drivers/base/power/clock_ops.c    |  5 +++--
 drivers/base/power/common.c       | 24 ++++++++++++++++++++++++
 drivers/base/power/domain.c       |  8 ++++++--
 drivers/base/power/main.c         | 35 +++++++++++++++++++++++++++++++++++
 drivers/base/power/power.h        |  3 +++
 drivers/gpu/vga/vga_switcheroo.c  | 10 +++++-----
 drivers/misc/mei/pci-me.c         |  5 +++--
 drivers/misc/mei/pci-txe.c        |  5 +++--
 drivers/usb/core/port.c           |  6 ++++++
 drivers/usb/core/usb.c            |  8 +++++++-
 include/linux/device.h            |  2 ++
 include/linux/pm.h                |  1 +
 include/linux/pm_domain.h         |  3 +++
 17 files changed, 130 insertions(+), 23 deletions(-)

-- 
2.5.0

[PATCH v11 1/4] device core: add device_is_bound()

[Tomeu Vizoso]

Adds a function that tells whether a device is already bound to a
driver.

This is needed to warn when there is an attempt to change the PM domain
of a device that has finished probing already. The reason why we want to
enforce that is because in the general case that can cause problems and
also that we can simplify code quite a bit if we can always assume that.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---

Changes in v9:
- Add docs noting the need for the device lock to be held before calling
  device_is_bound()

Changes in v8:
- Add device_is_bound()

 drivers/base/dd.c      | 18 ++++++++++++++++--
 include/linux/device.h |  2 ++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index be0eb4639128..d662ed5d08f4 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -192,9 +192,23 @@ static int deferred_probe_initcall(void)
 }
 late_initcall(deferred_probe_initcall);
 
+/**
+ * device_is_bound() - Check if device is bound to a driver
+ * @dev: device to check
+ *
+ * Returns true if passed device has already finished probing successfully
+ * against a driver.
+ *
+ * This function must be called with the device lock held.
+ */
+bool device_is_bound(struct device *dev)
+{
+	return klist_node_attached(&dev->p->knode_driver);
+}
+
 static void driver_bound(struct device *dev)
 {
-	if (klist_node_attached(&dev->p->knode_driver)) {
+	if (device_is_bound(dev)) {
 		printk(KERN_WARNING "%s: device %s already bound\n",
 			__func__, kobject_name(&dev->kobj));
 		return;
@@ -545,7 +559,7 @@ static int __device_attach(struct device *dev, bool allow_async)
 
 	device_lock(dev);
 	if (dev->driver) {
-		if (klist_node_attached(&dev->p->knode_driver)) {
+		if (device_is_bound(dev)) {
 			ret = 1;
 			goto out_unlock;
 		}
diff --git a/include/linux/device.h b/include/linux/device.h
index 5d7bc6349930..9b41c05fb479 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1035,6 +1035,8 @@ extern int __must_check driver_attach(struct device_driver *drv);
 extern void device_initial_probe(struct device *dev);
 extern int __must_check device_reprobe(struct device *dev);
 
+extern bool device_is_bound(struct device *dev);
+
 /*
  * Easy functions for dynamically creating devices on the fly
  */
-- 
2.5.0

[PATCH v11 2/4] PM / Domains: add setter for dev.pm_domain

[Tomeu Vizoso]

Adds a function that sets the pointer to dev_pm_domain in struct device
and that warns if the device has already finished probing. The reason
why we want to enforce that is because in the general case that can
cause problems and also that we can simplify code quite a bit if we can
always assume that.

This patch also changes all current code that directly sets the
dev.pm_domain pointer.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v11:
- Move calls to dev_pm_domain_set() out from &dev->power.lock as that
  isn't needed for dev->pm_domain.

Changes in v9:
- Add docs noting the need for the device lock to be held before calling
  dev_pm_domain_set()

Changes in v8:
- Add dev_pm_domain_set() and update code to use it

 arch/arm/mach-omap2/omap_device.c |  7 ++++---
 drivers/acpi/acpi_lpss.c          |  5 +++--
 drivers/acpi/device_pm.c          |  5 +++--
 drivers/base/power/clock_ops.c    |  5 +++--
 drivers/base/power/common.c       | 21 +++++++++++++++++++++
 drivers/base/power/domain.c       |  6 ++++--
 drivers/gpu/vga/vga_switcheroo.c  | 10 +++++-----
 drivers/misc/mei/pci-me.c         |  5 +++--
 drivers/misc/mei/pci-txe.c        |  5 +++--
 include/linux/pm_domain.h         |  3 +++
 10 files changed, 52 insertions(+), 20 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index 4cb8fd9f741f..204101d11632 100644
--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -32,6 +32,7 @@
 #include <linux/io.h>
 #include <linux/clk.h>
 #include <linux/clkdev.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
 #include <linux/notifier.h>
@@ -168,7 +169,7 @@ static int omap_device_build_from_dt(struct platform_device *pdev)
 			r->name = dev_name(&pdev->dev);
 	}
 
-	pdev->dev.pm_domain = &omap_device_pm_domain;
+	dev_pm_domain_set(&pdev->dev, &omap_device_pm_domain);
 
 	if (device_active) {
 		omap_device_enable(pdev);
@@ -180,7 +181,7 @@ odbfd_exit1:
 odbfd_exit:
 	/* if data/we are at fault.. load up a fail handler */
 	if (ret)
-		pdev->dev.pm_domain = &omap_device_fail_pm_domain;
+		dev_pm_domain_set(&pdev->dev, &omap_device_fail_pm_domain);
 
 	return ret;
 }
@@ -701,7 +702,7 @@ int omap_device_register(struct platform_device *pdev)
 {
 	pr_debug("omap_device: %s: registering\n", pdev->name);
 
-	pdev->dev.pm_domain = &omap_device_pm_domain;
+	dev_pm_domain_set(&pdev->dev, &omap_device_pm_domain);
 	return platform_device_add(pdev);
 }
 
diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index f51bd0d0bc17..cc6e1abc69b3 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -17,6 +17,7 @@
 #include <linux/io.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/clk-lpss.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/delay.h>
 
@@ -706,7 +707,7 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
 
 	switch (action) {
 	case BUS_NOTIFY_ADD_DEVICE:
-		pdev->dev.pm_domain = &acpi_lpss_pm_domain;
+		dev_pm_domain_set(&pdev->dev, &acpi_lpss_pm_domain);
 		if (pdata->dev_desc->flags & LPSS_LTR)
 			return sysfs_create_group(&pdev->dev.kobj,
 						  &lpss_attr_group);
@@ -714,7 +715,7 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
 	case BUS_NOTIFY_DEL_DEVICE:
 		if (pdata->dev_desc->flags & LPSS_LTR)
 			sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group);
-		pdev->dev.pm_domain = NULL;
+		dev_pm_domain_set(&pdev->dev, NULL);
 		break;
 	default:
 		break;
diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 4806b7f856c4..8c5955bf9bbf 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -22,6 +22,7 @@
 #include <linux/export.h>
 #include <linux/mutex.h>
 #include <linux/pm_qos.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 
 #include "internal.h"
@@ -1076,7 +1077,7 @@ static void acpi_dev_pm_detach(struct device *dev, bool power_off)
 	struct acpi_device *adev = ACPI_COMPANION(dev);
 
 	if (adev && dev->pm_domain == &acpi_general_pm_domain) {
-		dev->pm_domain = NULL;
+		dev_pm_domain_set(dev, NULL);
 		acpi_remove_pm_notifier(adev);
 		if (power_off) {
 			/*
@@ -1128,7 +1129,7 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
 		return -EBUSY;
 
 	acpi_add_pm_notifier(adev, dev, acpi_pm_notify_work_func);
-	dev->pm_domain = &acpi_general_pm_domain;
+	dev_pm_domain_set(dev, &acpi_general_pm_domain);
 	if (power_on) {
 		acpi_dev_pm_full_power(adev);
 		acpi_device_wakeup(adev, ACPI_STATE_S0, false);
diff --git a/drivers/base/power/clock_ops.c b/drivers/base/power/clock_ops.c
index 652b5a367c1f..e80fda6c03a9 100644
--- a/drivers/base/power/clock_ops.c
+++ b/drivers/base/power/clock_ops.c
@@ -15,6 +15,7 @@
 #include <linux/clkdev.h>
 #include <linux/slab.h>
 #include <linux/err.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 
 #ifdef CONFIG_PM
@@ -346,7 +347,7 @@ static int pm_clk_notify(struct notifier_block *nb,
 		if (error)
 			break;
 
-		dev->pm_domain = clknb->pm_domain;
+		dev_pm_domain_set(dev, clknb->pm_domain);
 		if (clknb->con_ids[0]) {
 			for (con_id = clknb->con_ids; *con_id; con_id++)
 				pm_clk_add(dev, *con_id);
@@ -359,7 +360,7 @@ static int pm_clk_notify(struct notifier_block *nb,
 		if (dev->pm_domain != clknb->pm_domain)
 			break;
 
-		dev->pm_domain = NULL;
+		dev_pm_domain_set(dev, NULL);
 		pm_clk_destroy(dev);
 		break;
 	}
diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
index f32b802b98f4..a70f8a5cdfd7 100644
--- a/drivers/base/power/common.c
+++ b/drivers/base/power/common.c
@@ -128,3 +128,24 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
 		dev->pm_domain->detach(dev, power_off);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
+
+/**
+ * dev_pm_domain_set - Set PM domain of a device.
+ * @dev: Device whose PM domain is to be set.
+ * @pd: PM domain to be set, or NULL.
+ *
+ * Sets the PM domain the device belongs to. The PM domain of a device needs
+ * to be set before its probe finishes (it's bound to a driver).
+ *
+ * This function must be called with the device lock held.
+ */
+void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd)
+{
+	if (dev->pm_domain == pd)
+		return;
+
+	WARN(device_is_bound(dev),
+	     "PM domains can only be changed for unbound devices\n");
+	dev->pm_domain = pd;
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_set);
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 16550c63d611..95b1a5c06592 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1240,10 +1240,11 @@ static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev,
 	}
 
 	dev->power.subsys_data->domain_data = &gpd_data->base;
-	dev->pm_domain = &genpd->domain;
 
 	spin_unlock_irq(&dev->power.lock);
 
+	dev_pm_domain_set(dev, &genpd->domain);
+
 	return gpd_data;
 
  err_free:
@@ -1257,9 +1258,10 @@ static struct generic_pm_domain_data *genpd_alloc_dev_data(struct device *dev,
 static void genpd_free_dev_data(struct device *dev,
 				struct generic_pm_domain_data *gpd_data)
 {
+	dev_pm_domain_set(dev, NULL);
+
 	spin_lock_irq(&dev->power.lock);
 
-	dev->pm_domain = NULL;
 	dev->power.subsys_data->domain_data = NULL;
 
 	spin_unlock_irq(&dev->power.lock);
diff --git a/drivers/gpu/vga/vga_switcheroo.c b/drivers/gpu/vga/vga_switcheroo.c
index 21060668fd25..2c3208ea6de4 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -665,17 +665,17 @@ int vga_switcheroo_init_domain_pm_ops(struct device *dev,
 		domain->ops.runtime_suspend = vga_switcheroo_runtime_suspend;
 		domain->ops.runtime_resume = vga_switcheroo_runtime_resume;
 
-		dev->pm_domain = domain;
+		dev_pm_domain_set(dev, domain);
 		return 0;
 	}
-	dev->pm_domain = NULL;
+	dev_pm_domain_set(dev, NULL);
 	return -EINVAL;
 }
 EXPORT_SYMBOL(vga_switcheroo_init_domain_pm_ops);
 
 void vga_switcheroo_fini_domain_pm_ops(struct device *dev)
 {
-	dev->pm_domain = NULL;
+	dev_pm_domain_set(dev, NULL);
 }
 EXPORT_SYMBOL(vga_switcheroo_fini_domain_pm_ops);
 
@@ -719,10 +719,10 @@ vga_switcheroo_init_domain_pm_optimus_hdmi_audio(struct device *dev,
 		domain->ops.runtime_resume =
 			vga_switcheroo_runtime_resume_hdmi_audio;
 
-		dev->pm_domain = domain;
+		dev_pm_domain_set(dev, domain);
 		return 0;
 	}
-	dev->pm_domain = NULL;
+	dev_pm_domain_set(dev, NULL);
 	return -EINVAL;
 }
 EXPORT_SYMBOL(vga_switcheroo_init_domain_pm_optimus_hdmi_audio);
diff --git a/drivers/misc/mei/pci-me.c b/drivers/misc/mei/pci-me.c
index 27678d8154e0..75fc9c688df8 100644
--- a/drivers/misc/mei/pci-me.c
+++ b/drivers/misc/mei/pci-me.c
@@ -31,6 +31,7 @@
 #include <linux/jiffies.h>
 #include <linux/interrupt.h>
 
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 
 #include <linux/mei.h>
@@ -436,7 +437,7 @@ static inline void mei_me_set_pm_domain(struct mei_device *dev)
 		dev->pg_domain.ops.runtime_resume = mei_me_pm_runtime_resume;
 		dev->pg_domain.ops.runtime_idle = mei_me_pm_runtime_idle;
 
-		pdev->dev.pm_domain = &dev->pg_domain;
+		dev_pm_domain_set(&pdev->dev, &dev->pg_domain);
 	}
 }
 
@@ -448,7 +449,7 @@ static inline void mei_me_set_pm_domain(struct mei_device *dev)
 static inline void mei_me_unset_pm_domain(struct mei_device *dev)
 {
 	/* stop using pm callbacks if any */
-	dev->dev->pm_domain = NULL;
+	dev_pm_domain_set(dev->dev, NULL);
 }
 
 static const struct dev_pm_ops mei_me_pm_ops = {
diff --git a/drivers/misc/mei/pci-txe.c b/drivers/misc/mei/pci-txe.c
index 0882c0201907..71f8a7475717 100644
--- a/drivers/misc/mei/pci-txe.c
+++ b/drivers/misc/mei/pci-txe.c
@@ -27,6 +27,7 @@
 #include <linux/jiffies.h>
 #include <linux/interrupt.h>
 #include <linux/workqueue.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 
 #include <linux/mei.h>
@@ -388,7 +389,7 @@ static inline void mei_txe_set_pm_domain(struct mei_device *dev)
 		dev->pg_domain.ops.runtime_resume = mei_txe_pm_runtime_resume;
 		dev->pg_domain.ops.runtime_idle = mei_txe_pm_runtime_idle;
 
-		pdev->dev.pm_domain = &dev->pg_domain;
+		dev_pm_domain_set(&pdev->dev, &dev->pg_domain);
 	}
 }
 
@@ -400,7 +401,7 @@ static inline void mei_txe_set_pm_domain(struct mei_device *dev)
 static inline void mei_txe_unset_pm_domain(struct mei_device *dev)
 {
 	/* stop using pm callbacks if any */
-	dev->dev->pm_domain = NULL;
+	dev_pm_domain_set(dev->dev, NULL);
 }
 
 static const struct dev_pm_ops mei_txe_pm_ops = {
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index b1cf7e797892..4ae4b14fb7b4 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -306,12 +306,15 @@ static inline int of_genpd_add_provider_onecell(struct device_node *np,
 #ifdef CONFIG_PM
 extern int dev_pm_domain_attach(struct device *dev, bool power_on);
 extern void dev_pm_domain_detach(struct device *dev, bool power_off);
+extern void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd);
 #else
 static inline int dev_pm_domain_attach(struct device *dev, bool power_on)
 {
 	return -ENODEV;
 }
 static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {}
+static inline void dev_pm_domain_set(struct device *dev,
+				     struct dev_pm_domain *pd) {}
 #endif
 
 #endif /* _LINUX_PM_DOMAIN_H */
-- 
2.5.0

[PATCH v11 3/4] PM / sleep: Go direct_complete if driver has no callbacks

[Tomeu Vizoso]

If a suitable prepare callback cannot be found for a given device and
its driver has no PM callbacks at all, assume that it can go direct to
complete when the system goes to sleep.

The reason for this is that there's lots of devices in a system that do
no PM at all and there's no reason for them to prevent their ancestors
to do direct_complete if they can support it.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---

Changes in v9:
- Move to CONFIG_PM_SLEEP as suggested by Rafael and Ulf.
- Rename from device_check_pm_callbacks to device_pm_check_callbacks to
  follow with the naming convention of existing API.
- Re-add calling to device_pm_check_callbacks from device registration
  and when updating the PM domain of a device.

Changes in v8:
- Move no_pm_callbacks field into CONFIG_PM_SLEEP
- Call device_check_pm_callbacks only after a device is bound or unbound

Changes in v7:
- Reduce indentation by adding a label in device_prepare()

Changes in v6:
- Add stub for !CONFIG_PM.
- Move implementation of device_check_pm_callbacks to power/main.c as it
  doesn't belong to CONFIG_PM_SLEEP.
- Take dev->power.lock before modifying flag.

Changes in v5:
- Check for all dev_pm_ops instances associated to a device, updating a
  no_pm_callbacks flag at the times when that could change.

 drivers/base/dd.c           |  3 +++
 drivers/base/power/common.c |  3 +++
 drivers/base/power/domain.c |  2 ++
 drivers/base/power/main.c   | 35 +++++++++++++++++++++++++++++++++++
 drivers/base/power/power.h  |  3 +++
 include/linux/pm.h          |  1 +
 6 files changed, 47 insertions(+)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index d662ed5d08f4..15aae412f95b 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -219,6 +219,8 @@ static void driver_bound(struct device *dev)
 
 	klist_add_tail(&dev->p->knode_driver, &dev->driver->p->klist_devices);
 
+	device_pm_check_callbacks(dev);
+
 	/*
 	 * Make sure the device is no longer in one of the deferred lists and
 	 * kick off retrying all pending devices
@@ -709,6 +711,7 @@ static void __device_release_driver(struct device *dev)
 			dev->pm_domain->dismiss(dev);
 
 		klist_remove(&dev->p->knode_driver);
+		device_pm_check_callbacks(dev);
 		if (dev->bus)
 			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 						     BUS_NOTIFY_UNBOUND_DRIVER,
diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
index a70f8a5cdfd7..763eee24cb95 100644
--- a/drivers/base/power/common.c
+++ b/drivers/base/power/common.c
@@ -14,6 +14,8 @@
 #include <linux/acpi.h>
 #include <linux/pm_domain.h>
 
+#include "power.h"
+
 /**
  * dev_pm_get_subsys_data - Create or refcount power.subsys_data for device.
  * @dev: Device to handle.
@@ -147,5 +149,6 @@ void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd)
 	WARN(device_is_bound(dev),
 	     "PM domains can only be changed for unbound devices\n");
 	dev->pm_domain = pd;
+	device_pm_check_callbacks(dev);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_set);
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 95b1a5c06592..cfb6e756b073 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -20,6 +20,8 @@
 #include <linux/suspend.h>
 #include <linux/export.h>
 
+#include "power.h"
+
 #define GENPD_RETRY_MAX_MS	250		/* Approximate */
 
 #define GENPD_DEV_CALLBACK(genpd, type, callback, dev)		\
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 1710c26ba097..d0faac7ab26b 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -125,6 +125,7 @@ void device_pm_add(struct device *dev)
 {
 	pr_debug("PM: Adding info for %s:%s\n",
 		 dev->bus ? dev->bus->name : "No Bus", dev_name(dev));
+	device_pm_check_callbacks(dev);
 	mutex_lock(&dpm_list_mtx);
 	if (dev->parent && dev->parent->power.is_prepared)
 		dev_warn(dev, "parent %s should not be sleeping\n",
@@ -147,6 +148,7 @@ void device_pm_remove(struct device *dev)
 	mutex_unlock(&dpm_list_mtx);
 	device_wakeup_disable(dev);
 	pm_runtime_remove(dev);
+	device_pm_check_callbacks(dev);
 }
 
 /**
@@ -1569,6 +1571,11 @@ static int device_prepare(struct device *dev, pm_message_t state)
 
 	dev->power.wakeup_path = device_may_wakeup(dev);
 
+	if (dev->power.no_pm_callbacks) {
+		ret = 1;	/* Let device go direct_complete */
+		goto unlock;
+	}
+
 	if (dev->pm_domain) {
 		info = "preparing power domain ";
 		callback = dev->pm_domain->ops.prepare;
@@ -1591,6 +1598,7 @@ static int device_prepare(struct device *dev, pm_message_t state)
 	if (callback)
 		ret = callback(dev);
 
+unlock:
 	device_unlock(dev);
 
 	if (ret < 0) {
@@ -1719,3 +1727,30 @@ void dpm_for_each_dev(void *data, void (*fn)(struct device *, void *))
 	device_pm_unlock();
 }
 EXPORT_SYMBOL_GPL(dpm_for_each_dev);
+
+static bool pm_ops_is_empty(const struct dev_pm_ops *ops)
+{
+	if (!ops)
+		return true;
+
+	return !ops->prepare &&
+	       !ops->suspend &&
+	       !ops->suspend_late &&
+	       !ops->suspend_noirq &&
+	       !ops->resume_noirq &&
+	       !ops->resume_early &&
+	       !ops->resume &&
+	       !ops->complete;
+}
+
+void device_pm_check_callbacks(struct device *dev)
+{
+	spin_lock_irq(&dev->power.lock);
+	dev->power.no_pm_callbacks =
+		(!dev->bus || pm_ops_is_empty(dev->bus->pm)) &&
+		(!dev->class || pm_ops_is_empty(dev->class->pm)) &&
+		(!dev->type || pm_ops_is_empty(dev->type->pm)) &&
+		(!dev->pm_domain || pm_ops_is_empty(&dev->pm_domain->ops)) &&
+		(!dev->driver || pm_ops_is_empty(dev->driver->pm));
+	spin_unlock_irq(&dev->power.lock);
+}
diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
index 998fa6b23084..297beae64314 100644
--- a/drivers/base/power/power.h
+++ b/drivers/base/power/power.h
@@ -123,6 +123,7 @@ extern void device_pm_remove(struct device *);
 extern void device_pm_move_before(struct device *, struct device *);
 extern void device_pm_move_after(struct device *, struct device *);
 extern void device_pm_move_last(struct device *);
+extern void device_pm_check_callbacks(struct device *dev);
 
 #else /* !CONFIG_PM_SLEEP */
 
@@ -141,6 +142,8 @@ static inline void device_pm_move_after(struct device *deva,
 					struct device *devb) {}
 static inline void device_pm_move_last(struct device *dev) {}
 
+static inline void device_pm_check_callbacks(struct device *dev) {}
+
 #endif /* !CONFIG_PM_SLEEP */
 
 static inline void device_pm_init(struct device *dev)
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 35d599e7250d..7943acb74ae4 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -573,6 +573,7 @@ struct dev_pm_info {
 	struct wakeup_source	*wakeup;
 	bool			wakeup_path:1;
 	bool			syscore:1;
+	bool			no_pm_callbacks:1;	/* Owned by the PM core */
 #else
 	unsigned int		should_wakeup:1;
 #endif
-- 
2.5.0

[PATCH v11 4/4] USB / PM: Allow USB devices to remain runtime-suspended when sleeping

[Tomeu Vizoso]

Have dev_pm_ops.prepare return 1 for USB devices and ports so that USB
devices can remain runtime-suspended when the system goes to a sleep
state, if their wakeup state is correct and they have runtime PM enabled.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
---

Changes in v10:
- Remove superfluous call to pm_runtime_enabled() as suggested by Alan

 drivers/usb/core/port.c | 6 ++++++
 drivers/usb/core/usb.c  | 8 +++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 210618319f10..f49707d73b5a 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -168,12 +168,18 @@ static int usb_port_runtime_suspend(struct device *dev)
 
 	return retval;
 }
+
+static int usb_port_prepare(struct device *dev)
+{
+	return 1;
+}
 #endif
 
 static const struct dev_pm_ops usb_port_pm_ops = {
 #ifdef CONFIG_PM
 	.runtime_suspend =	usb_port_runtime_suspend,
 	.runtime_resume =	usb_port_runtime_resume,
+	.prepare =		usb_port_prepare,
 #endif
 };
 
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 8d5b2f4113cd..3a55c91b13a1 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -316,7 +316,13 @@ static int usb_dev_uevent(struct device *dev, struct kobj_uevent_env *env)
 
 static int usb_dev_prepare(struct device *dev)
 {
-	return 0;		/* Implement eventually? */
+	struct usb_device *udev = to_usb_device(dev);
+
+	/* Return 0 if the current wakeup setting is wrong, otherwise 1 */
+	if (udev->do_remote_wakeup != device_may_wakeup(dev))
+		return 0;
+
+	return 1;
 }
 
 static void usb_dev_complete(struct device *dev)
-- 
2.5.0

Re: [PATCH v11 0/4] Allow USB devices to remain runtime-suspended when sleeping

[Rafael J. Wysocki]

On Tuesday, October 27, 2015 03:38:47 PM Tomeu Vizoso wrote:
> Hi,
> 
> this is v11 of an attempt to make it easier for devices to remain in
> runtime PM when the system goes to sleep, mainly to reduce the time
> spent resuming devices.
> 
> For this, we interpret the absence of all PM callback implementations as
> it being safe to do direct_complete, so their ancestors aren't prevented
> from remaining runtime-suspended.
> 
> Additionally, the prepare() callback of USB devices will return 1 if
> runtime PM is enabled and the current wakeup settings are correct.
> 
> With these changes, a uvcvideo device (for example) stays in runtime
> suspend when the system goes to sleep and is left in that state when the
> system resumes, not delaying it unnecessarily.
> 
> Thanks,
> 
> Tomeu
> 
> Changes in v11:
> - Move calls to dev_pm_domain_set() out from &dev->power.lock as that
>   isn't needed for dev->pm_domain.
> 
> Changes in v10:
> - Remove superfluous call to pm_runtime_enabled() as suggested by Alan
> 
> Changes in v9:
> - Add docs noting the need for the device lock to be held before calling
>   device_is_bound()
> - Add docs noting the need for the device lock to be held before calling
>   dev_pm_domain_set()
> - Move to CONFIG_PM_SLEEP as suggested by Rafael and Ulf.
> - Rename from device_check_pm_callbacks to device_pm_check_callbacks to
>   follow with the naming convention of existing API.
> - Re-add calling to device_pm_check_callbacks from device registration
>   and when updating the PM domain of a device.
> 
> Changes in v8:
> - Add device_is_bound()
> - Add dev_pm_domain_set() and update code to use it
> - Move no_pm_callbacks field into CONFIG_PM_SLEEP
> - Call device_check_pm_callbacks only after a device is bound or unbound
> 
> Changes in v7:
> - Reduce indentation by adding a label in device_prepare()
> 
> Changes in v6:
> - Add stub for !CONFIG_PM.
> - Move implementation of device_check_pm_callbacks to power/main.c as it
>   doesn't belong to CONFIG_PM_SLEEP.
> - Take dev->power.lock before modifying flag.
> 
> Changes in v5:
> - Check for all dev_pm_ops instances associated to a device, updating a
>   no_pm_callbacks flag at the times when that could change.
> 
> Tomeu Vizoso (4):
>   device core: add device_is_bound()
>   PM / Domains: add setter for dev.pm_domain
>   PM / sleep: Go direct_complete if driver has no callbacks
>   USB / PM: Allow USB devices to remain runtime-suspended when sleeping
> 
>  arch/arm/mach-omap2/omap_device.c |  7 ++++---
>  drivers/acpi/acpi_lpss.c          |  5 +++--
>  drivers/acpi/device_pm.c          |  5 +++--
>  drivers/base/dd.c                 | 21 +++++++++++++++++++--
>  drivers/base/power/clock_ops.c    |  5 +++--
>  drivers/base/power/common.c       | 24 ++++++++++++++++++++++++
>  drivers/base/power/domain.c       |  8 ++++++--
>  drivers/base/power/main.c         | 35 +++++++++++++++++++++++++++++++++++
>  drivers/base/power/power.h        |  3 +++
>  drivers/gpu/vga/vga_switcheroo.c  | 10 +++++-----
>  drivers/misc/mei/pci-me.c         |  5 +++--
>  drivers/misc/mei/pci-txe.c        |  5 +++--
>  drivers/usb/core/port.c           |  6 ++++++
>  drivers/usb/core/usb.c            |  8 +++++++-
>  include/linux/device.h            |  2 ++
>  include/linux/pm.h                |  1 +
>  include/linux/pm_domain.h         |  3 +++
>  17 files changed, 130 insertions(+), 23 deletions(-)

I've queued up this series for the second half of the v4.4 merge window.

Thanks,
Rafael

Re: [PATCH v11 2/4] PM / Domains: add setter for dev.pm_domain

[Daniel Kurtz]

Hi Tomeu,

On Tue, Oct 27, 2015 at 10:38 PM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> Adds a function that sets the pointer to dev_pm_domain in struct device
> and that warns if the device has already finished probing. The reason
> why we want to enforce that is because in the general case that can
> cause problems and also that we can simplify code quite a bit if we can
> always assume that.
>
> This patch also changes all current code that directly sets the
> dev.pm_domain pointer.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---

[snip...]

> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
> index f32b802b98f4..a70f8a5cdfd7 100644
> --- a/drivers/base/power/common.c
> +++ b/drivers/base/power/common.c
> @@ -128,3 +128,24 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
>                 dev->pm_domain->detach(dev, power_off);
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
> +
> +/**
> + * dev_pm_domain_set - Set PM domain of a device.
> + * @dev: Device whose PM domain is to be set.
> + * @pd: PM domain to be set, or NULL.
> + *
> + * Sets the PM domain the device belongs to. The PM domain of a device needs
> + * to be set before its probe finishes (it's bound to a driver).
> + *
> + * This function must be called with the device lock held.
> + */
> +void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd)
> +{
> +       if (dev->pm_domain == pd)
> +               return;
> +
> +       WARN(device_is_bound(dev),
> +            "PM domains can only be changed for unbound devices\n");
> +       dev->pm_domain = pd;
> +}

When testing this patch, I have a platform driver with a device in a
genpd that hits this WARN() during shutdown with a backtrace like:

[  930.476714] ------------[ cut here ]------------
[  930.481323] WARNING: CPU: 3 PID: 10110 at
drivers/base/power/common.c:154 dev_pm_domain_set+0x50/0x60()
[  930.494605] PM domains can only be changed for unbound devices
[  930.541047] Call trace:
[  930.543470] [<ffffffc000208c00>] dump_backtrace+0x0/0x140
[  930.548820] [<ffffffc000208d5c>] show_stack+0x1c/0x28
[  930.553826] [<ffffffc000862f8c>] dump_stack+0x74/0x94
[  930.558831] [<ffffffc000221964>] warn_slowpath_common+0x90/0xb8
[  930.564698] [<ffffffc000221a10>] warn_slowpath_fmt+0x84/0xac
[  930.570305] [<ffffffc000574da8>] dev_pm_domain_set+0x4c/0x60
[  930.575913] [<ffffffc00057f500>] pm_genpd_remove_device+0xc4/0x174
[  930.582038] [<ffffffc00057f62c>] genpd_dev_pm_detach+0x7c/0xd4
[  930.587818] [<ffffffc000574be4>] dev_pm_domain_detach+0x34/0x44
[  930.593685] [<ffffffc00056e0f0>] platform_drv_shutdown+0x30/0x40
[  930.599639] [<ffffffc000569dfc>] device_shutdown+0x12c/0x184
[  930.605247] [<ffffffc000243dbc>] kernel_restart_prepare+0x38/0x44
[  930.611285] [<ffffffc000243eb0>] kernel_restart+0x1c/0x68
[  930.616634] [<ffffffc000244230>] SyS_reboot+0x1b4/0x210
[  930.621810] ---[ end trace 0551d0a7afcd5f6f ]---

The problem appears to be that:
  * On boot, platform_drv_probe() calls dev_pm_domain_attach() before
drv->probe(); thus, it calls dev_pm_domain_attach() while the device
is unbound.

 * However, for a platform_device, the reboot path calls
device_shutdown(), but not __device_release_driver():
device_shutdown()
  dev->driver->shutdown => platform_drv_shutdown()
    dev_pm_domain_detach()
       dev->pm_domain->detach() => genpd_dev_pm_detach()
         pm_genpd_remove_device()
            dev_pm_domain_set(dev, NULL);

So, for a platform_device in a genpd power domain with .shutdown
installed, platform_drv_shutdown() calls dev_pm_domain_detach() while
the device is still bound, which triggers the WARN().

Thanks,
-Dan

Re: Re: [PATCH v11 0/4] Allow USB devices to remain runtime-suspended when sleeping

[Derek Basehore]

On Mon, Nov 02, 2015 at 02:50:40AM +0100, Rafael J. Wysocki wrote:
> 
> I've queued up this series for the second half of the v4.4 merge window.
> 
> Thanks,
> Rafael
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

What's the status of this patch series? I haven't seen the patches
posted for v4.4, plus there's the issue that Dan found that needs to be
addressed.

Is there a new revision of the patch series being worked on?

Re: [PATCH v11 0/4] Allow USB devices to remain runtime-suspended when sleeping

[Rafael J. Wysocki]

On Monday, January 04, 2016 06:27:18 PM Derek Basehore wrote:
> On Mon, Nov 02, 2015 at 02:50:40AM +0100, Rafael J. Wysocki wrote:
> > 
> > I've queued up this series for the second half of the v4.4 merge window.
> > 
> > Thanks,
> > Rafael
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> What's the status of this patch series? I haven't seen the patches
> posted for v4.4, plus there's the issue that Dan found that needs to be
> addressed.
> 
> Is there a new revision of the patch series being worked on?

Tomeu is not working on one AFAICS, but I may just revive his series.

Do you have a pointer to the Dan's report?

Thanks,
Rafael

Re: [PATCH v11 0/4] Allow USB devices to remain runtime-suspended when sleeping

[dbasehore .]

On Tue, Jan 5, 2016 at 4:48 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Monday, January 04, 2016 06:27:18 PM Derek Basehore wrote:
>> On Mon, Nov 02, 2015 at 02:50:40AM +0100, Rafael J. Wysocki wrote:
>> >
>> > I've queued up this series for the second half of the v4.4 merge window.
>> >
>> > Thanks,
>> > Rafael
>> >
>> >
>> > _______________________________________________
>> > linux-arm-kernel mailing list
>> > linux-arm-kernel@lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>> What's the status of this patch series? I haven't seen the patches
>> posted for v4.4, plus there's the issue that Dan found that needs to be
>> addressed.
>>
>> Is there a new revision of the patch series being worked on?
>
> Tomeu is not working on one AFAICS, but I may just revive his series.
>
> Do you have a pointer to the Dan's report?
>
> Thanks,
> Rafael
>

It was a reply to the second patch in the series. Here's a link to it
https://lkml.org/lkml/2015/11/10/107

Re: [PATCH v11 2/4] PM / Domains: add setter for dev.pm_domain

[Tomeu Vizoso]

On 10 November 2015 at 10:33, Daniel Kurtz <djkurtz@chromium.org> wrote:
[snip]
>
> The problem appears to be that:
>   * On boot, platform_drv_probe() calls dev_pm_domain_attach() before
> drv->probe(); thus, it calls dev_pm_domain_attach() while the device
> is unbound.
>
>  * However, for a platform_device, the reboot path calls
> device_shutdown(), but not __device_release_driver():
> device_shutdown()
>   dev->driver->shutdown => platform_drv_shutdown()
>     dev_pm_domain_detach()
>        dev->pm_domain->detach() => genpd_dev_pm_detach()
>          pm_genpd_remove_device()
>             dev_pm_domain_set(dev, NULL);
>
> So, for a platform_device in a genpd power domain with .shutdown
> installed, platform_drv_shutdown() calls dev_pm_domain_detach() while
> the device is still bound, which triggers the WARN().

Hi Rafael, Alan and Ulf,

do you have any suggestion about this? I don't really understand why
the device is detached from the domain on shutdown.

Thanks,

Tomeu

> Thanks,
> -Dan
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v11 2/4] PM / Domains: add setter for dev.pm_domain

[Rafael J. Wysocki]

On Thursday, January 07, 2016 03:47:01 PM Tomeu Vizoso wrote:
> On 10 November 2015 at 10:33, Daniel Kurtz <djkurtz@chromium.org> wrote:
> [snip]
> >
> > The problem appears to be that:
> >   * On boot, platform_drv_probe() calls dev_pm_domain_attach() before
> > drv->probe(); thus, it calls dev_pm_domain_attach() while the device
> > is unbound.
> >
> >  * However, for a platform_device, the reboot path calls
> > device_shutdown(), but not __device_release_driver():
> > device_shutdown()
> >   dev->driver->shutdown => platform_drv_shutdown()
> >     dev_pm_domain_detach()
> >        dev->pm_domain->detach() => genpd_dev_pm_detach()
> >          pm_genpd_remove_device()
> >             dev_pm_domain_set(dev, NULL);
> >
> > So, for a platform_device in a genpd power domain with .shutdown
> > installed, platform_drv_shutdown() calls dev_pm_domain_detach() while
> > the device is still bound, which triggers the WARN().
> 
> Hi Rafael, Alan and Ulf,
> 
> do you have any suggestion about this? I don't really understand why
> the device is detached from the domain on shutdown.

Well, this looks like a bug to me.

Thanks,
Rafael

Re: [PATCH v11 2/4] PM / Domains: add setter for dev.pm_domain

[Fabio Estevam]

On Thu, Jan 7, 2016 at 12:47 PM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> On 10 November 2015 at 10:33, Daniel Kurtz <djkurtz@chromium.org> wrote:
> [snip]
>>
>> The problem appears to be that:
>>   * On boot, platform_drv_probe() calls dev_pm_domain_attach() before
>> drv->probe(); thus, it calls dev_pm_domain_attach() while the device
>> is unbound.
>>
>>  * However, for a platform_device, the reboot path calls
>> device_shutdown(), but not __device_release_driver():
>> device_shutdown()
>>   dev->driver->shutdown => platform_drv_shutdown()
>>     dev_pm_domain_detach()
>>        dev->pm_domain->detach() => genpd_dev_pm_detach()
>>          pm_genpd_remove_device()
>>             dev_pm_domain_set(dev, NULL);
>>
>> So, for a platform_device in a genpd power domain with .shutdown
>> installed, platform_drv_shutdown() calls dev_pm_domain_detach() while
>> the device is still bound, which triggers the WARN().
>
> Hi Rafael, Alan and Ulf,
>
> do you have any suggestion about this? I don't really understand why
> the device is detached from the domain on shutdown.

I am running linux-next and this commit causes the same problem for me
on a mx6 after running a 'reboot' command:

Requesting system reboot
[   15.058782] ------------[ cut here ]------------
[   15.063459] WARNING: CPU: 3 PID: 1122 at
drivers/base/power/common.c:150 dev_pm_domain_set+0x4c/0x58()
[   15.072838] PM domains can only be changed for unbound devices
[   15.078735] Modules linked in:
[   15.081849] CPU: 3 PID: 1122 Comm: init Not tainted
4.4.0-rc8-next-20160111-dirty #207
[   15.089826] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[   15.096375] Backtrace:
[   15.098941] [<c00136d8>] (dump_backtrace) from [<c0013874>]
(show_stack+0x18/0x1c)
[   15.106532]  r6:00000096 r5:00000000 r4:00000000 r3:00000000
[   15.112390] [<c001385c>] (show_stack) from [<c02de89c>]
(dump_stack+0x88/0xa4)
[   15.119708] [<c02de814>] (dump_stack) from [<c002bcac>]
(warn_slowpath_common+0x80/0xbc)
[   15.127860]  r5:c03da810 r4:ee63bd68
[   15.131533] [<c002bc2c>] (warn_slowpath_common) from [<c002bd8c>]
(warn_slowpath_fmt+0x38/0x40)
[   15.140292]  r8:eea01db0 r7:c0b0f87c r6:eea01d80 r5:00000000 r4:ef181410
[   15.147165] [<c002bd58>] (warn_slowpath_fmt) from [<c03da810>]
(dev_pm_domain_set+0x4c/0x58)
[   15.155661]  r3:c0b0f7f0 r2:c09e82dc
[   15.159373] [<c03da7c4>] (dev_pm_domain_set) from [<c03e4e5c>]
(genpd_free_dev_data+0x20/0x50)
[   15.168065]  r5:ef1814a0 r4:ef181410
[   15.171737] [<c03e4e3c>] (genpd_free_dev_data) from [<c03e5fe4>]
(pm_genpd_remove_device+0xb4/0xe0)
[   15.180842]  r6:ef181410 r5:eea01d80 r4:c0b0f7f0 r3:00000000
[   15.186644] [<c03e5f30>] (pm_genpd_remove_device) from [<c03e6040>]
(genpd_dev_pm_detach+0x30/0xac)
[   15.195743]  r8:c0b0f7f0 r7:c1379734 r6:00000001 r5:c0b209c4
r4:ef181410 r3:00000000
[   15.203694] [<c03e6010>] (genpd_dev_pm_detach) from [<c03da7c0>]
(dev_pm_domain_detach+0x28/0x2c)
[   15.212621]  r10:ef181444 r8:c0b6f560 r7:c1379734 r6:ef181410
r5:ef173c10 r4:ef181410
[   15.220683] [<c03da798>] (dev_pm_domain_detach) from [<c03d3ea0>]
(platform_drv_shutdown+0x34/0x38)
[   15.229817] [<c03d3e6c>] (platform_drv_shutdown) from [<c03d05b0>]
(device_shutdown+0x3c/0x194)
[   15.238579]  r4:ef18141c r3:c03d3e6c
[   15.242259] [<c03d0574>] (device_shutdown) from [<c004d51c>]
(kernel_restart_prepare+0x34/0x40)
[   15.251021]  r10:00000000 r8:c0010044 r7:00000000 r6:c0b120cc
r5:4321fedc r4:00000000
[   15.259065] [<c004d4e8>] (kernel_restart_prepare) from [<c004d724>]
(kernel_restart+0x14/0x58)
[   15.267760] [<c004d710>] (kernel_restart) from [<c004d948>]
(SyS_reboot+0x18c/0x1e4)
[   15.275526]  r4:01234567 r3:01234567
[   15.279242] [<c004d7bc>] (SyS_reboot) from [<c000fea0>]
(ret_fast_syscall+0x0/0x1c)
[   15.286920]  r7:00000058 r6:00000000 r5:00000000 r4:00000000
[   15.292754] ---[ end trace 3b419d3cbb367a11 ]---

Re: [PATCH v11 2/4] PM / Domains: add setter for dev.pm_domain

[Rafael J. Wysocki]

On Monday, January 11, 2016 12:35:28 PM Fabio Estevam wrote:
> On Thu, Jan 7, 2016 at 12:47 PM, Tomeu Vizoso
> <tomeu.vizoso@collabora.com> wrote:
> > On 10 November 2015 at 10:33, Daniel Kurtz <djkurtz@chromium.org> wrote:
> > [snip]
> >>
> >> The problem appears to be that:
> >>   * On boot, platform_drv_probe() calls dev_pm_domain_attach() before
> >> drv->probe(); thus, it calls dev_pm_domain_attach() while the device
> >> is unbound.
> >>
> >>  * However, for a platform_device, the reboot path calls
> >> device_shutdown(), but not __device_release_driver():
> >> device_shutdown()
> >>   dev->driver->shutdown => platform_drv_shutdown()
> >>     dev_pm_domain_detach()
> >>        dev->pm_domain->detach() => genpd_dev_pm_detach()
> >>          pm_genpd_remove_device()
> >>             dev_pm_domain_set(dev, NULL);
> >>
> >> So, for a platform_device in a genpd power domain with .shutdown
> >> installed, platform_drv_shutdown() calls dev_pm_domain_detach() while
> >> the device is still bound, which triggers the WARN().
> >
> > Hi Rafael, Alan and Ulf,
> >
> > do you have any suggestion about this? I don't really understand why
> > the device is detached from the domain on shutdown.
> 
> I am running linux-next and this commit causes the same problem for me
> on a mx6 after running a 'reboot' command:
> 
> Requesting system reboot
> [   15.058782] ------------[ cut here ]------------
> [   15.063459] WARNING: CPU: 3 PID: 1122 at
> drivers/base/power/common.c:150 dev_pm_domain_set+0x4c/0x58()
> [   15.072838] PM domains can only be changed for unbound devices
> [   15.078735] Modules linked in:
> [   15.081849] CPU: 3 PID: 1122 Comm: init Not tainted
> 4.4.0-rc8-next-20160111-dirty #207
> [   15.089826] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> [   15.096375] Backtrace:
> [   15.098941] [<c00136d8>] (dump_backtrace) from [<c0013874>]
> (show_stack+0x18/0x1c)
> [   15.106532]  r6:00000096 r5:00000000 r4:00000000 r3:00000000
> [   15.112390] [<c001385c>] (show_stack) from [<c02de89c>]
> (dump_stack+0x88/0xa4)
> [   15.119708] [<c02de814>] (dump_stack) from [<c002bcac>]
> (warn_slowpath_common+0x80/0xbc)
> [   15.127860]  r5:c03da810 r4:ee63bd68
> [   15.131533] [<c002bc2c>] (warn_slowpath_common) from [<c002bd8c>]
> (warn_slowpath_fmt+0x38/0x40)
> [   15.140292]  r8:eea01db0 r7:c0b0f87c r6:eea01d80 r5:00000000 r4:ef181410
> [   15.147165] [<c002bd58>] (warn_slowpath_fmt) from [<c03da810>]
> (dev_pm_domain_set+0x4c/0x58)
> [   15.155661]  r3:c0b0f7f0 r2:c09e82dc
> [   15.159373] [<c03da7c4>] (dev_pm_domain_set) from [<c03e4e5c>]
> (genpd_free_dev_data+0x20/0x50)
> [   15.168065]  r5:ef1814a0 r4:ef181410
> [   15.171737] [<c03e4e3c>] (genpd_free_dev_data) from [<c03e5fe4>]
> (pm_genpd_remove_device+0xb4/0xe0)
> [   15.180842]  r6:ef181410 r5:eea01d80 r4:c0b0f7f0 r3:00000000
> [   15.186644] [<c03e5f30>] (pm_genpd_remove_device) from [<c03e6040>]
> (genpd_dev_pm_detach+0x30/0xac)
> [   15.195743]  r8:c0b0f7f0 r7:c1379734 r6:00000001 r5:c0b209c4
> r4:ef181410 r3:00000000
> [   15.203694] [<c03e6010>] (genpd_dev_pm_detach) from [<c03da7c0>]
> (dev_pm_domain_detach+0x28/0x2c)
> [   15.212621]  r10:ef181444 r8:c0b6f560 r7:c1379734 r6:ef181410
> r5:ef173c10 r4:ef181410
> [   15.220683] [<c03da798>] (dev_pm_domain_detach) from [<c03d3ea0>]
> (platform_drv_shutdown+0x34/0x38)
> [   15.229817] [<c03d3e6c>] (platform_drv_shutdown) from [<c03d05b0>]
> (device_shutdown+0x3c/0x194)

OK, so does the appended patch help?

Rafael

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] platform: Do not detach from PM domains on shutdown

Shutdown is carried out when the driver is still bound to the
device, so it is incorrect to detach it from a PM domain (if any)
at this point.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/base/platform.c |    1 -
 1 file changed, 1 deletion(-)

Index: linux-pm/drivers/base/platform.c
===================================================================
--- linux-pm.orig/drivers/base/platform.c
+++ linux-pm/drivers/base/platform.c
@@ -577,7 +577,6 @@ static void platform_drv_shutdown(struct
 
 	if (drv->shutdown)
 		drv->shutdown(dev);
-	dev_pm_domain_detach(_dev, true);
 }
 
 /**

Re: [PATCH v11 2/4] PM / Domains: add setter for dev.pm_domain

[Fabio Estevam]

On Mon, Jan 11, 2016 at 12:45 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:

> OK, so does the appended patch help?

Yes, thanks. I do not see the warnings after 'reboot' with your patch applied:

Tested-by: Fabio Estevam <fabio.estevam@nxp.com>

Re: [PATCH v11 2/4] PM / Domains: add setter for dev.pm_domain

[Ulf Hansson]

[...]

>> >
>> > Hi Rafael, Alan and Ulf,
>> >
>> > do you have any suggestion about this? I don't really understand why
>> > the device is detached from the domain on shutdown.

Sorry for the delay.

I fully agree with you Rafael, it shouldn't be detached. It's a bug.

>>
>> I am running linux-next and this commit causes the same problem for me
>> on a mx6 after running a 'reboot' command:
>>
>> Requesting system reboot
>> [   15.058782] ------------[ cut here ]------------
>> [   15.063459] WARNING: CPU: 3 PID: 1122 at
>> drivers/base/power/common.c:150 dev_pm_domain_set+0x4c/0x58()
>> [   15.072838] PM domains can only be changed for unbound devices
>> [   15.078735] Modules linked in:
>> [   15.081849] CPU: 3 PID: 1122 Comm: init Not tainted
>> 4.4.0-rc8-next-20160111-dirty #207
>> [   15.089826] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
>> [   15.096375] Backtrace:
>> [   15.098941] [<c00136d8>] (dump_backtrace) from [<c0013874>]
>> (show_stack+0x18/0x1c)
>> [   15.106532]  r6:00000096 r5:00000000 r4:00000000 r3:00000000
>> [   15.112390] [<c001385c>] (show_stack) from [<c02de89c>]
>> (dump_stack+0x88/0xa4)
>> [   15.119708] [<c02de814>] (dump_stack) from [<c002bcac>]
>> (warn_slowpath_common+0x80/0xbc)
>> [   15.127860]  r5:c03da810 r4:ee63bd68
>> [   15.131533] [<c002bc2c>] (warn_slowpath_common) from [<c002bd8c>]
>> (warn_slowpath_fmt+0x38/0x40)
>> [   15.140292]  r8:eea01db0 r7:c0b0f87c r6:eea01d80 r5:00000000 r4:ef181410
>> [   15.147165] [<c002bd58>] (warn_slowpath_fmt) from [<c03da810>]
>> (dev_pm_domain_set+0x4c/0x58)
>> [   15.155661]  r3:c0b0f7f0 r2:c09e82dc
>> [   15.159373] [<c03da7c4>] (dev_pm_domain_set) from [<c03e4e5c>]
>> (genpd_free_dev_data+0x20/0x50)
>> [   15.168065]  r5:ef1814a0 r4:ef181410
>> [   15.171737] [<c03e4e3c>] (genpd_free_dev_data) from [<c03e5fe4>]
>> (pm_genpd_remove_device+0xb4/0xe0)
>> [   15.180842]  r6:ef181410 r5:eea01d80 r4:c0b0f7f0 r3:00000000
>> [   15.186644] [<c03e5f30>] (pm_genpd_remove_device) from [<c03e6040>]
>> (genpd_dev_pm_detach+0x30/0xac)
>> [   15.195743]  r8:c0b0f7f0 r7:c1379734 r6:00000001 r5:c0b209c4
>> r4:ef181410 r3:00000000
>> [   15.203694] [<c03e6010>] (genpd_dev_pm_detach) from [<c03da7c0>]
>> (dev_pm_domain_detach+0x28/0x2c)
>> [   15.212621]  r10:ef181444 r8:c0b6f560 r7:c1379734 r6:ef181410
>> r5:ef173c10 r4:ef181410
>> [   15.220683] [<c03da798>] (dev_pm_domain_detach) from [<c03d3ea0>]
>> (platform_drv_shutdown+0x34/0x38)
>> [   15.229817] [<c03d3e6c>] (platform_drv_shutdown) from [<c03d05b0>]
>> (device_shutdown+0x3c/0x194)
>
> OK, so does the appended patch help?
>
> Rafael
>
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH] platform: Do not detach from PM domains on shutdown
>
> Shutdown is carried out when the driver is still bound to the
> device, so it is incorrect to detach it from a PM domain (if any)
> at this point.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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

> ---
>  drivers/base/platform.c |    1 -
>  1 file changed, 1 deletion(-)
>
> Index: linux-pm/drivers/base/platform.c
> ===================================================================
> --- linux-pm.orig/drivers/base/platform.c
> +++ linux-pm/drivers/base/platform.c
> @@ -577,7 +577,6 @@ static void platform_drv_shutdown(struct
>
>         if (drv->shutdown)
>                 drv->shutdown(dev);
> -       dev_pm_domain_detach(_dev, true);
>  }
>
>  /**
>