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

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

[Tomeu Vizoso]

Hi,

this is v12 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 v12:
- Include linux/pm_domain.h in vga_switcheroo.c for dev_pm_domain_set()

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  | 11 ++++++-----
 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, 132 insertions(+), 22 deletions(-)

-- 
2.5.0

[PATCH v12 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 v12:
- Include linux/pm_domain.h in vga_switcheroo.c for dev_pm_domain_set()

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  | 11 ++++++-----
 drivers/misc/mei/pci-me.c         |  5 +++--
 drivers/misc/mei/pci-txe.c        |  5 +++--
 include/linux/pm_domain.h         |  3 +++
 10 files changed, 54 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index 3750ed14f8c5..0437537751bc 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 047281a6ae11..c570b1d9f094 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -18,6 +18,7 @@
 #include <linux/mutex.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>
 
@@ -875,13 +876,14 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
 
 	switch (action) {
 	case BUS_NOTIFY_BIND_DRIVER:
-		pdev->dev.pm_domain = &acpi_lpss_pm_domain;
+		dev_pm_domain_set(&pdev->dev, &acpi_lpss_pm_domain);
 		break;
 	case BUS_NOTIFY_DRIVER_NOT_BOUND:
 	case BUS_NOTIFY_UNBOUND_DRIVER:
 		pdev->dev.pm_domain = NULL;
 		break;
 	case BUS_NOTIFY_ADD_DEVICE:
+		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);
@@ -889,6 +891,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);
+		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 08a02cdc737c..cd2c3d6d40e0 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"
@@ -1059,7 +1060,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) {
 			/*
@@ -1111,7 +1112,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 60ee5591ee8f..b36d01e0589d 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_CLK
@@ -348,7 +349,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);
@@ -361,7 +362,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 f48e33385b3e..02812bcabcac 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 b80379012840..e8b526faec1f 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1188,10 +1188,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:
@@ -1205,9 +1206,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 d64d9058bce5..665ab9fd0e01 100644
--- a/drivers/gpu/vga/vga_switcheroo.c
+++ b/drivers/gpu/vga/vga_switcheroo.c
@@ -36,6 +36,7 @@
 #include <linux/fs.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/seq_file.h>
 #include <linux/uaccess.h>
@@ -918,17 +919,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);
 
@@ -989,10 +990,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 ba4ced38efae..db21d3995f7e 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -240,12 +240,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

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

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

[Rafael J. Wysocki]

On Thursday, January 07, 2016 04:46:11 PM Tomeu Vizoso wrote:
> Hi,
> 
> this is v12 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 v12:
> - Include linux/pm_domain.h in vga_switcheroo.c for dev_pm_domain_set()
> 
> 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  | 11 ++++++-----
>  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, 132 insertions(+), 22 deletions(-)

Series applied, thanks!


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

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

[Andy Shevchenko]

On Thu, Jan 7, 2016 at 5:46 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.


> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c

> @@ -875,13 +876,14 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>
>         switch (action) {
>         case BUS_NOTIFY_BIND_DRIVER:
> -               pdev->dev.pm_domain = &acpi_lpss_pm_domain;
> +               dev_pm_domain_set(&pdev->dev, &acpi_lpss_pm_domain);
>                 break;
>         case BUS_NOTIFY_DRIVER_NOT_BOUND:
>         case BUS_NOTIFY_UNBOUND_DRIVER:
>                 pdev->dev.pm_domain = NULL;

Missed?

>                 break;
>         case BUS_NOTIFY_ADD_DEVICE:
> +               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);
> @@ -889,6 +891,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);
> +               dev_pm_domain_set(&pdev->dev, NULL);
>                 break;
>         default:
>                 break;

This looks wrong. I didn't test yet, but I have concerns here. Why did
you add those calls?

Have you been aware about the commit b5f88dd1d6ef ("Revert "ACPI /
LPSS: allow to use specific PM domain during ->probe()") ?

Sorry for being too late with this.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jan 29, 2016 at 5:38 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Jan 7, 2016 at 5:46 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.
>
>
>> --- a/drivers/acpi/acpi_lpss.c
>> +++ b/drivers/acpi/acpi_lpss.c
>
>> @@ -875,13 +876,14 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>>
>>         switch (action) {
>>         case BUS_NOTIFY_BIND_DRIVER:
>> -               pdev->dev.pm_domain = &acpi_lpss_pm_domain;
>> +               dev_pm_domain_set(&pdev->dev, &acpi_lpss_pm_domain);
>>                 break;
>>         case BUS_NOTIFY_DRIVER_NOT_BOUND:
>>         case BUS_NOTIFY_UNBOUND_DRIVER:
>>                 pdev->dev.pm_domain = NULL;
>
> Missed?
>
>>                 break;
>>         case BUS_NOTIFY_ADD_DEVICE:
>> +               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);
>> @@ -889,6 +891,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);
>> +               dev_pm_domain_set(&pdev->dev, NULL);
>>                 break;
>>         default:
>>                 break;
>
> This looks wrong. I didn't test yet, but I have concerns here. Why did
> you add those calls?


Okay, for first glance it seems working on Intel Braswell. But can
yoyu point me out what was going on before this patch?

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jan 29, 2016 at 5:51 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jan 29, 2016 at 5:38 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Thu, Jan 7, 2016 at 5:46 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.
>>
>>
>>> --- a/drivers/acpi/acpi_lpss.c
>>> +++ b/drivers/acpi/acpi_lpss.c
>>
>>> @@ -875,13 +876,14 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>>>
>>>         switch (action) {
>>>         case BUS_NOTIFY_BIND_DRIVER:
>>> -               pdev->dev.pm_domain = &acpi_lpss_pm_domain;
>>> +               dev_pm_domain_set(&pdev->dev, &acpi_lpss_pm_domain);
>>>                 break;
>>>         case BUS_NOTIFY_DRIVER_NOT_BOUND:
>>>         case BUS_NOTIFY_UNBOUND_DRIVER:
>>>                 pdev->dev.pm_domain = NULL;
>>
>> Missed?
>>
>>>                 break;
>>>         case BUS_NOTIFY_ADD_DEVICE:
>>> +               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);
>>> @@ -889,6 +891,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);
>>> +               dev_pm_domain_set(&pdev->dev, NULL);
>>>                 break;
>>>         default:
>>>                 break;
>>
>> This looks wrong. I didn't test yet, but I have concerns here. Why did
>> you add those calls?
>
>
> Okay, for first glance it seems working on Intel Braswell.

One more test unveils this one

# modprobe -r sdhci-acpi
[ 1289.909441] ------------[ cut here ]------------
[ 1289.918205] WARNING: CPU: 1 PID: 4374 at
/home/andy/prj/linux-otc/drivers/base/power/common.c:150
dev_pm_domain_set+0x51/0x60()
[ 1289.934681] PM domains can only be changed for unbound devices
[ 1289.944843] Modules linked in: sdhci_acpi(-) sdhci mmc_core
led_class [last unloaded: dw_dmac_core]
[ 1289.958802] CPU: 1 PID: 4374 Comm: modprobe Not tainted 4.5.0-rc1+ #3
[ 1289.969736]  ffffffff81c38330 ffff88007bb53d18 ffffffff8133162f
ffff88007bb53d60
[ 1289.981844]  ffff88007bb53d50 ffffffff8105cd12 ffff88017a007410
0000000000000000
[ 1289.993996]  0000000000000001 0000000000000080 0000000000000000
ffff88007bb53db0
[ 1290.006123] Call Trace:
[ 1290.012600]  [<ffffffff8133162f>] dump_stack+0x44/0x55
[ 1290.022052]  [<ffffffff8105cd12>] warn_slowpath_common+0x82/0xc0
[ 1290.032462]  [<ffffffff8105cd9c>] warn_slowpath_fmt+0x4c/0x50
[ 1290.042589]  [<ffffffff814867c1>] dev_pm_domain_set+0x51/0x60
[ 1290.052695]  [<ffffffff813a62a9>] acpi_dev_pm_detach+0x3f/0x84
[ 1290.062910]  [<ffffffff81486747>] dev_pm_domain_detach+0x27/0x30
[ 1290.073294]  [<ffffffff81480188>] platform_drv_remove+0x38/0x40
[ 1290.083585]  [<ffffffff8147da51>] __device_release_driver+0xa1/0x160
[ 1290.094335]  [<ffffffff8147e5a6>] driver_detach+0xa6/0xb0
[ 1290.104009]  [<ffffffff8147d435>] bus_remove_driver+0x55/0xd0
[ 1290.114053]  [<ffffffff8147eccc>] driver_unregister+0x2c/0x50
[ 1290.124076]  [<ffffffff81480232>] platform_driver_unregister+0x12/0x20
[ 1290.134957]  [<ffffffffa003681c>] sdhci_acpi_driver_exit+0x10/0x12
[sdhci_acpi]
[ 1290.146730]  [<ffffffff810ce0e9>] SyS_delete_module+0x199/0x240
[ 1290.156907]  [<ffffffff8100219b>] ? exit_to_usermode_loop+0x8b/0x90
[ 1290.167478]  [<ffffffff81002b83>] do_syscall_32_irqs_off+0x53/0xa0
[ 1290.177949]  [<ffffffff818b7f9a>] entry_INT80_compat+0x2a/0x40
[ 1290.187986] ---[ end trace a8351c0506ee239b ]---
[ 1290.213855] mmc0: card 0001 removed

Which function is guilty?

> But can
> yoyu point me out what was going on before this patch?

-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Rafael J. Wysocki]

On Friday, January 29, 2016 06:35:49 PM Andy Shevchenko wrote:
> On Fri, Jan 29, 2016 at 5:51 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jan 29, 2016 at 5:38 PM, Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> >> On Thu, Jan 7, 2016 at 5:46 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.
> >>
> >>
> >>> --- a/drivers/acpi/acpi_lpss.c
> >>> +++ b/drivers/acpi/acpi_lpss.c
> >>
> >>> @@ -875,13 +876,14 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
> >>>
> >>>         switch (action) {
> >>>         case BUS_NOTIFY_BIND_DRIVER:
> >>> -               pdev->dev.pm_domain = &acpi_lpss_pm_domain;
> >>> +               dev_pm_domain_set(&pdev->dev, &acpi_lpss_pm_domain);
> >>>                 break;
> >>>         case BUS_NOTIFY_DRIVER_NOT_BOUND:
> >>>         case BUS_NOTIFY_UNBOUND_DRIVER:
> >>>                 pdev->dev.pm_domain = NULL;
> >>
> >> Missed?
> >>
> >>>                 break;
> >>>         case BUS_NOTIFY_ADD_DEVICE:
> >>> +               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);
> >>> @@ -889,6 +891,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);
> >>> +               dev_pm_domain_set(&pdev->dev, NULL);
> >>>                 break;
> >>>         default:
> >>>                 break;
> >>
> >> This looks wrong. I didn't test yet, but I have concerns here. Why did
> >> you add those calls?
> >
> >
> > Okay, for first glance it seems working on Intel Braswell.
> 
> One more test unveils this one
> 
> # modprobe -r sdhci-acpi
> [ 1289.909441] ------------[ cut here ]------------
> [ 1289.918205] WARNING: CPU: 1 PID: 4374 at
> /home/andy/prj/linux-otc/drivers/base/power/common.c:150
> dev_pm_domain_set+0x51/0x60()
> [ 1289.934681] PM domains can only be changed for unbound devices
> [ 1289.944843] Modules linked in: sdhci_acpi(-) sdhci mmc_core
> led_class [last unloaded: dw_dmac_core]
> [ 1289.958802] CPU: 1 PID: 4374 Comm: modprobe Not tainted 4.5.0-rc1+ #3
> [ 1289.969736]  ffffffff81c38330 ffff88007bb53d18 ffffffff8133162f
> ffff88007bb53d60
> [ 1289.981844]  ffff88007bb53d50 ffffffff8105cd12 ffff88017a007410
> 0000000000000000
> [ 1289.993996]  0000000000000001 0000000000000080 0000000000000000
> ffff88007bb53db0
> [ 1290.006123] Call Trace:
> [ 1290.012600]  [<ffffffff8133162f>] dump_stack+0x44/0x55
> [ 1290.022052]  [<ffffffff8105cd12>] warn_slowpath_common+0x82/0xc0
> [ 1290.032462]  [<ffffffff8105cd9c>] warn_slowpath_fmt+0x4c/0x50
> [ 1290.042589]  [<ffffffff814867c1>] dev_pm_domain_set+0x51/0x60
> [ 1290.052695]  [<ffffffff813a62a9>] acpi_dev_pm_detach+0x3f/0x84
> [ 1290.062910]  [<ffffffff81486747>] dev_pm_domain_detach+0x27/0x30
> [ 1290.073294]  [<ffffffff81480188>] platform_drv_remove+0x38/0x40
> [ 1290.083585]  [<ffffffff8147da51>] __device_release_driver+0xa1/0x160
> [ 1290.094335]  [<ffffffff8147e5a6>] driver_detach+0xa6/0xb0
> [ 1290.104009]  [<ffffffff8147d435>] bus_remove_driver+0x55/0xd0
> [ 1290.114053]  [<ffffffff8147eccc>] driver_unregister+0x2c/0x50
> [ 1290.124076]  [<ffffffff81480232>] platform_driver_unregister+0x12/0x20
> [ 1290.134957]  [<ffffffffa003681c>] sdhci_acpi_driver_exit+0x10/0x12
> [sdhci_acpi]
> [ 1290.146730]  [<ffffffff810ce0e9>] SyS_delete_module+0x199/0x240
> [ 1290.156907]  [<ffffffff8100219b>] ? exit_to_usermode_loop+0x8b/0x90
> [ 1290.167478]  [<ffffffff81002b83>] do_syscall_32_irqs_off+0x53/0xa0
> [ 1290.177949]  [<ffffffff818b7f9a>] entry_INT80_compat+0x2a/0x40
> [ 1290.187986] ---[ end trace a8351c0506ee239b ]---
> [ 1290.213855] mmc0: card 0001 removed
> 
> Which function is guilty?

acpi_dev_pm_detach() calls dev_pm_domain_set() to clear the pm_domain pointer.

One might argue that this is happening too early.

Thanks,
Rafael

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

[Valdis.Kletnieks]

On Fri, 29 Jan 2016 22:35:18 +0100, "Rafael J. Wysocki" said:
> > One more test unveils this one
> > 
> > # modprobe -r sdhci-acpi
> > [ 1289.909441] ------------[ cut here ]------------
> > [ 1289.918205] WARNING: CPU: 1 PID: 4374 at
> > /home/andy/prj/linux-otc/drivers/base/power/common.c:150
> > dev_pm_domain_set+0x51/0x60()
> > [ 1289.934681] PM domains can only be changed for unbound devices
> > [ 1289.944843] Modules linked in: sdhci_acpi(-) sdhci mmc_core
> > led_class [last unloaded: dw_dmac_core]
> > [ 1289.958802] CPU: 1 PID: 4374 Comm: modprobe Not tainted 4.5.0-rc1+ #3
> > [ 1289.969736]  ffffffff81c38330 ffff88007bb53d18 ffffffff8133162f
> > ffff88007bb53d60
> > [ 1289.981844]  ffff88007bb53d50 ffffffff8105cd12 ffff88017a007410
> > 0000000000000000
> > [ 1289.993996]  0000000000000001 0000000000000080 0000000000000000
> > ffff88007bb53db0
> > [ 1290.006123] Call Trace:
> > [ 1290.012600]  [<ffffffff8133162f>] dump_stack+0x44/0x55
> > [ 1290.022052]  [<ffffffff8105cd12>] warn_slowpath_common+0x82/0xc0
> > [ 1290.032462]  [<ffffffff8105cd9c>] warn_slowpath_fmt+0x4c/0x50
> > [ 1290.042589]  [<ffffffff814867c1>] dev_pm_domain_set+0x51/0x60

My Dell Latitude laptopi on next-20160201 is throwing a similar error
at shutdown, except the traceback continues:

mei_me_remove+0xbd/0xc0
pci_device_shutdown+0x32/0x50
device_shutdown+0xe2/0x210

(sorry, that's all I have, transcribing from one frame of video shot while
the system was shutting down....)

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

[Rafael J. Wysocki]

On Mon, Feb 8, 2016 at 10:44 PM,  <Valdis.Kletnieks@vt.edu> wrote:
> On Fri, 29 Jan 2016 22:35:18 +0100, "Rafael J. Wysocki" said:
>> > One more test unveils this one
>> >
>> > # modprobe -r sdhci-acpi
>> > [ 1289.909441] ------------[ cut here ]------------
>> > [ 1289.918205] WARNING: CPU: 1 PID: 4374 at
>> > /home/andy/prj/linux-otc/drivers/base/power/common.c:150
>> > dev_pm_domain_set+0x51/0x60()
>> > [ 1289.934681] PM domains can only be changed for unbound devices
>> > [ 1289.944843] Modules linked in: sdhci_acpi(-) sdhci mmc_core
>> > led_class [last unloaded: dw_dmac_core]
>> > [ 1289.958802] CPU: 1 PID: 4374 Comm: modprobe Not tainted 4.5.0-rc1+ #3
>> > [ 1289.969736]  ffffffff81c38330 ffff88007bb53d18 ffffffff8133162f
>> > ffff88007bb53d60
>> > [ 1289.981844]  ffff88007bb53d50 ffffffff8105cd12 ffff88017a007410
>> > 0000000000000000
>> > [ 1289.993996]  0000000000000001 0000000000000080 0000000000000000
>> > ffff88007bb53db0
>> > [ 1290.006123] Call Trace:
>> > [ 1290.012600]  [<ffffffff8133162f>] dump_stack+0x44/0x55
>> > [ 1290.022052]  [<ffffffff8105cd12>] warn_slowpath_common+0x82/0xc0
>> > [ 1290.032462]  [<ffffffff8105cd9c>] warn_slowpath_fmt+0x4c/0x50
>> > [ 1290.042589]  [<ffffffff814867c1>] dev_pm_domain_set+0x51/0x60
>
> My Dell Latitude laptopi on next-20160201 is throwing a similar error
> at shutdown, except the traceback continues:
>
> mei_me_remove+0xbd/0xc0
> pci_device_shutdown+0x32/0x50
> device_shutdown+0xe2/0x210
>
> (sorry, that's all I have, transcribing from one frame of video shot while
> the system was shutting down....)

This should be fixed in 4.5-rc3, so please retest.

Thanks,
Rafael

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

[Valdis.Kletnieks]

[-- Attachment #1.1: Type: text/plain, Size: 613 bytes --]

On Mon, 08 Feb 2016 22:50:39 +0100, "Rafael J. Wysocki" said:
> On Mon, Feb 8, 2016 at 10:44 PM,  <Valdis.Kletnieks@vt.edu> wrote:

> > My Dell Latitude laptopi on next-20160201 is throwing a similar error
> > at shutdown, except the traceback continues:
> >
> > mei_me_remove+0xbd/0xc0
> > pci_device_shutdown+0x32/0x50
> > device_shutdown+0xe2/0x210
> >
> > (sorry, that's all I have, transcribing from one frame of video shot while
> > the system was shutting down....)
>
> This should be fixed in 4.5-rc3, so please retest.

Confirming that next-20160201 has the problem, but it is fixed as of next-20160208.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 848 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel