| Message ID | 1445956731-6304-3-git-send-email-tomeu.vizoso@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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 ]---
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 */