| Message ID | 20250616135357.3929441-2-claudiu.beznea.uj@bp.renesas.com |
|---|---|
| State | New |
| Headers | show |
| Series | PM: domains: Detach on device_unbind_cleanup() | expand |
Hi, Rafael, On 16.06.2025 20:14, Rafael J. Wysocki wrote: > On Mon, Jun 16, 2025 at 3:54 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: >> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> >> The dev_pm_domain_attach() function is typically used in bus code alongside >> dev_pm_domain_detach(), often following patterns like: >> >> static int bus_probe(struct device *_dev) >> { >> struct bus_driver *drv = to_bus_driver(dev->driver); >> struct bus_device *dev = to_bus_device(_dev); >> int ret; >> >> // ... >> >> ret = dev_pm_domain_attach(_dev, true); >> if (ret) >> return ret; >> >> if (drv->probe) >> ret = drv->probe(dev); >> >> // ... >> } >> >> static void bus_remove(struct device *_dev) >> { >> struct bus_driver *drv = to_bus_driver(dev->driver); >> struct bus_device *dev = to_bus_device(_dev); >> >> if (drv->remove) >> drv->remove(dev); >> dev_pm_domain_detach(_dev); >> } >> >> When the driver's probe function uses devres-managed resources that depend >> on the power domain state, those resources are released later during >> device_unbind_cleanup(). >> >> Releasing devres-managed resources that depend on the power domain state >> after detaching the device from its PM domain can cause failures. >> >> For example, if the driver uses devm_pm_runtime_enable() in its probe >> function, and the device's clocks are managed by the PM domain, then >> during removal the runtime PM is disabled in device_unbind_cleanup() after >> the clocks have been removed from the PM domain. It may happen that the >> devm_pm_runtime_enable() action causes the device to be runtime-resumed. >> If the driver specific runtime PM APIs access registers directly, this >> will lead to accessing device registers without clocks being enabled. >> Similar issues may occur with other devres actions that access device >> registers. >> >> Add detach_power_off member to struct dev_pm_info, to be used later in >> device_unbind_cleanup() as the power_off argument for >> dev_pm_domain_detach(). This is a preparatory step toward removing >> dev_pm_domain_detach() calls from bus remove functions. Since the current >> PM domain detach functions (genpd_dev_pm_detach() and acpi_dev_pm_detach()) >> already set dev->pm_domain = NULL, there should be no issues with bus >> drivers that still call dev_pm_domain_detach() in their remove functions. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> >> --- >> >> Changes in v4: >> - save dev->power.detach_power_off in dev_pm_domain_attach() and use >> it in device_unbind_cleanup() when detaching >> - adjusted patch description >> >> Changes in v3: >> - dropped devm_pm_domain_detach_off(), devm_pm_domain_detach_on() >> and use a single function devm_pm_domain_detach() >> >> Changes in v2: >> - none; this patch is new >> >> drivers/base/dd.c | 2 ++ >> drivers/base/power/common.c | 3 +++ >> include/linux/pm.h | 1 + >> 3 files changed, 6 insertions(+) >> >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c >> index b526e0e0f52d..13ab98e033ea 100644 >> --- a/drivers/base/dd.c >> +++ b/drivers/base/dd.c >> @@ -25,6 +25,7 @@ >> #include <linux/kthread.h> >> #include <linux/wait.h> >> #include <linux/async.h> >> +#include <linux/pm_domain.h> >> #include <linux/pm_runtime.h> >> #include <linux/pinctrl/devinfo.h> >> #include <linux/slab.h> >> @@ -552,6 +553,7 @@ static void device_unbind_cleanup(struct device *dev) >> dev->dma_range_map = NULL; >> device_set_driver(dev, NULL); >> dev_set_drvdata(dev, NULL); >> + dev_pm_domain_detach(dev, dev->power.detach_power_off); >> if (dev->pm_domain && dev->pm_domain->dismiss) >> dev->pm_domain->dismiss(dev); >> pm_runtime_reinit(dev); >> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c >> index 781968a128ff..a8f302ed27a5 100644 >> --- a/drivers/base/power/common.c >> +++ b/drivers/base/power/common.c >> @@ -111,6 +111,9 @@ int dev_pm_domain_attach(struct device *dev, bool power_on) >> if (!ret) >> ret = genpd_dev_pm_attach(dev); >> >> + if (dev->pm_domain) >> + dev->power.detach_power_off = power_on; > > I'm assuming that you have checked all of the users of > dev_pm_domain_attach() and verified that the "power off" value is the > same as the "power on" one for all of them. In v2 it has been discussed to just mirror the power_on acquisition. Double checking now, all the current users of dev_pm_domain_attach() follow this rule, except the i2c bus. i2c powers on the domain conditionally: https://elixir.bootlin.com/linux/v6.15.2/source/drivers/i2c/i2c-core-base.c#L575 and powers it off unconditionally: https://elixir.bootlin.com/linux/v6.15.2/source/drivers/i2c/i2c-core-base.c#L638 Should we take this into account ? Thank you, Claudiu > >> + >> return ret < 0 ? ret : 0; >> } >> EXPORT_SYMBOL_GPL(dev_pm_domain_attach); >> diff --git a/include/linux/pm.h b/include/linux/pm.h >> index f0bd8fbae4f2..dcbe2c1ef59b 100644 >> --- a/include/linux/pm.h >> +++ b/include/linux/pm.h >> @@ -720,6 +720,7 @@ struct dev_pm_info { >> struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */ >> void (*set_latency_tolerance)(struct device *, s32); >> struct dev_pm_qos *qos; >> + bool detach_power_off:1; > > Please put the new flag under #ifdef CONFIG_PM after memalloc_noio and > comment it as "Owned by the driver core". OK! Thank you for your review, Claudiu > > Otherwise LGTM. > >> }; >> >> extern int dev_pm_get_subsys_data(struct device *dev); >> -- >> 2.43.0 >>
On Tue, Jun 17, 2025 at 4:41 PM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote: > > Hi, Rafael, > > On 16.06.2025 20:14, Rafael J. Wysocki wrote: > > On Mon, Jun 16, 2025 at 3:54 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > >> > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> > >> The dev_pm_domain_attach() function is typically used in bus code alongside > >> dev_pm_domain_detach(), often following patterns like: > >> > >> static int bus_probe(struct device *_dev) > >> { > >> struct bus_driver *drv = to_bus_driver(dev->driver); > >> struct bus_device *dev = to_bus_device(_dev); > >> int ret; > >> > >> // ... > >> > >> ret = dev_pm_domain_attach(_dev, true); > >> if (ret) > >> return ret; > >> > >> if (drv->probe) > >> ret = drv->probe(dev); > >> > >> // ... > >> } > >> > >> static void bus_remove(struct device *_dev) > >> { > >> struct bus_driver *drv = to_bus_driver(dev->driver); > >> struct bus_device *dev = to_bus_device(_dev); > >> > >> if (drv->remove) > >> drv->remove(dev); > >> dev_pm_domain_detach(_dev); > >> } > >> > >> When the driver's probe function uses devres-managed resources that depend > >> on the power domain state, those resources are released later during > >> device_unbind_cleanup(). > >> > >> Releasing devres-managed resources that depend on the power domain state > >> after detaching the device from its PM domain can cause failures. > >> > >> For example, if the driver uses devm_pm_runtime_enable() in its probe > >> function, and the device's clocks are managed by the PM domain, then > >> during removal the runtime PM is disabled in device_unbind_cleanup() after > >> the clocks have been removed from the PM domain. It may happen that the > >> devm_pm_runtime_enable() action causes the device to be runtime-resumed. > >> If the driver specific runtime PM APIs access registers directly, this > >> will lead to accessing device registers without clocks being enabled. > >> Similar issues may occur with other devres actions that access device > >> registers. > >> > >> Add detach_power_off member to struct dev_pm_info, to be used later in > >> device_unbind_cleanup() as the power_off argument for > >> dev_pm_domain_detach(). This is a preparatory step toward removing > >> dev_pm_domain_detach() calls from bus remove functions. Since the current > >> PM domain detach functions (genpd_dev_pm_detach() and acpi_dev_pm_detach()) > >> already set dev->pm_domain = NULL, there should be no issues with bus > >> drivers that still call dev_pm_domain_detach() in their remove functions. > >> > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > >> --- > >> > >> Changes in v4: > >> - save dev->power.detach_power_off in dev_pm_domain_attach() and use > >> it in device_unbind_cleanup() when detaching > >> - adjusted patch description > >> > >> Changes in v3: > >> - dropped devm_pm_domain_detach_off(), devm_pm_domain_detach_on() > >> and use a single function devm_pm_domain_detach() > >> > >> Changes in v2: > >> - none; this patch is new > >> > >> drivers/base/dd.c | 2 ++ > >> drivers/base/power/common.c | 3 +++ > >> include/linux/pm.h | 1 + > >> 3 files changed, 6 insertions(+) > >> > >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c > >> index b526e0e0f52d..13ab98e033ea 100644 > >> --- a/drivers/base/dd.c > >> +++ b/drivers/base/dd.c > >> @@ -25,6 +25,7 @@ > >> #include <linux/kthread.h> > >> #include <linux/wait.h> > >> #include <linux/async.h> > >> +#include <linux/pm_domain.h> > >> #include <linux/pm_runtime.h> > >> #include <linux/pinctrl/devinfo.h> > >> #include <linux/slab.h> > >> @@ -552,6 +553,7 @@ static void device_unbind_cleanup(struct device *dev) > >> dev->dma_range_map = NULL; > >> device_set_driver(dev, NULL); > >> dev_set_drvdata(dev, NULL); > >> + dev_pm_domain_detach(dev, dev->power.detach_power_off); > >> if (dev->pm_domain && dev->pm_domain->dismiss) > >> dev->pm_domain->dismiss(dev); > >> pm_runtime_reinit(dev); > >> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c > >> index 781968a128ff..a8f302ed27a5 100644 > >> --- a/drivers/base/power/common.c > >> +++ b/drivers/base/power/common.c > >> @@ -111,6 +111,9 @@ int dev_pm_domain_attach(struct device *dev, bool power_on) > >> if (!ret) > >> ret = genpd_dev_pm_attach(dev); > >> > >> + if (dev->pm_domain) > >> + dev->power.detach_power_off = power_on; > > > > I'm assuming that you have checked all of the users of > > dev_pm_domain_attach() and verified that the "power off" value is the > > same as the "power on" one for all of them. > > In v2 it has been discussed to just mirror the power_on acquisition. > > Double checking now, all the current users of dev_pm_domain_attach() follow > this rule, except the i2c bus. i2c powers on the domain conditionally: > > https://elixir.bootlin.com/linux/v6.15.2/source/drivers/i2c/i2c-core-base.c#L575 > > and powers it off unconditionally: > https://elixir.bootlin.com/linux/v6.15.2/source/drivers/i2c/i2c-core-base.c#L638 > > Should we take this into account ? I think so. It is still sufficient to use one device flag to represent the information whether or not to remove power on detach, but I would change the second argument of dev_pm_domain_attach() to a u8 representing a mask of bits: PM_DOMAIN_POWER_ON BIT(0) PM_DOMAIN_POWER_OFF BIT(1) where PM_DOMAIN_POWER_ON will be set to indicate that the device should be turned on right after attaching the PM domain and the value of PM_DOMAIN_POWER_OFF will be stored in the new device flag. The majority of users will set or clear both, but i2c will set PM_DOMAIN_POWER_OFF and either set of clear PM_DOMAIN_POWER_ON depending on the do_power_on value.
On Tue, 17 Jun 2025 at 20:54, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Tue, Jun 17, 2025 at 4:41 PM Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote: > > > > Hi, Rafael, > > > > On 16.06.2025 20:14, Rafael J. Wysocki wrote: > > > On Mon, Jun 16, 2025 at 3:54 PM Claudiu <claudiu.beznea@tuxon.dev> wrote: > > >> > > >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > >> > > >> The dev_pm_domain_attach() function is typically used in bus code alongside > > >> dev_pm_domain_detach(), often following patterns like: > > >> > > >> static int bus_probe(struct device *_dev) > > >> { > > >> struct bus_driver *drv = to_bus_driver(dev->driver); > > >> struct bus_device *dev = to_bus_device(_dev); > > >> int ret; > > >> > > >> // ... > > >> > > >> ret = dev_pm_domain_attach(_dev, true); > > >> if (ret) > > >> return ret; > > >> > > >> if (drv->probe) > > >> ret = drv->probe(dev); > > >> > > >> // ... > > >> } > > >> > > >> static void bus_remove(struct device *_dev) > > >> { > > >> struct bus_driver *drv = to_bus_driver(dev->driver); > > >> struct bus_device *dev = to_bus_device(_dev); > > >> > > >> if (drv->remove) > > >> drv->remove(dev); > > >> dev_pm_domain_detach(_dev); > > >> } > > >> > > >> When the driver's probe function uses devres-managed resources that depend > > >> on the power domain state, those resources are released later during > > >> device_unbind_cleanup(). > > >> > > >> Releasing devres-managed resources that depend on the power domain state > > >> after detaching the device from its PM domain can cause failures. > > >> > > >> For example, if the driver uses devm_pm_runtime_enable() in its probe > > >> function, and the device's clocks are managed by the PM domain, then > > >> during removal the runtime PM is disabled in device_unbind_cleanup() after > > >> the clocks have been removed from the PM domain. It may happen that the > > >> devm_pm_runtime_enable() action causes the device to be runtime-resumed. > > >> If the driver specific runtime PM APIs access registers directly, this > > >> will lead to accessing device registers without clocks being enabled. > > >> Similar issues may occur with other devres actions that access device > > >> registers. > > >> > > >> Add detach_power_off member to struct dev_pm_info, to be used later in > > >> device_unbind_cleanup() as the power_off argument for > > >> dev_pm_domain_detach(). This is a preparatory step toward removing > > >> dev_pm_domain_detach() calls from bus remove functions. Since the current > > >> PM domain detach functions (genpd_dev_pm_detach() and acpi_dev_pm_detach()) > > >> already set dev->pm_domain = NULL, there should be no issues with bus > > >> drivers that still call dev_pm_domain_detach() in their remove functions. > > >> > > >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com> > > >> --- > > >> > > >> Changes in v4: > > >> - save dev->power.detach_power_off in dev_pm_domain_attach() and use > > >> it in device_unbind_cleanup() when detaching > > >> - adjusted patch description > > >> > > >> Changes in v3: > > >> - dropped devm_pm_domain_detach_off(), devm_pm_domain_detach_on() > > >> and use a single function devm_pm_domain_detach() > > >> > > >> Changes in v2: > > >> - none; this patch is new > > >> > > >> drivers/base/dd.c | 2 ++ > > >> drivers/base/power/common.c | 3 +++ > > >> include/linux/pm.h | 1 + > > >> 3 files changed, 6 insertions(+) > > >> > > >> diff --git a/drivers/base/dd.c b/drivers/base/dd.c > > >> index b526e0e0f52d..13ab98e033ea 100644 > > >> --- a/drivers/base/dd.c > > >> +++ b/drivers/base/dd.c > > >> @@ -25,6 +25,7 @@ > > >> #include <linux/kthread.h> > > >> #include <linux/wait.h> > > >> #include <linux/async.h> > > >> +#include <linux/pm_domain.h> > > >> #include <linux/pm_runtime.h> > > >> #include <linux/pinctrl/devinfo.h> > > >> #include <linux/slab.h> > > >> @@ -552,6 +553,7 @@ static void device_unbind_cleanup(struct device *dev) > > >> dev->dma_range_map = NULL; > > >> device_set_driver(dev, NULL); > > >> dev_set_drvdata(dev, NULL); > > >> + dev_pm_domain_detach(dev, dev->power.detach_power_off); > > >> if (dev->pm_domain && dev->pm_domain->dismiss) > > >> dev->pm_domain->dismiss(dev); > > >> pm_runtime_reinit(dev); > > >> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c > > >> index 781968a128ff..a8f302ed27a5 100644 > > >> --- a/drivers/base/power/common.c > > >> +++ b/drivers/base/power/common.c > > >> @@ -111,6 +111,9 @@ int dev_pm_domain_attach(struct device *dev, bool power_on) > > >> if (!ret) > > >> ret = genpd_dev_pm_attach(dev); > > >> > > >> + if (dev->pm_domain) > > >> + dev->power.detach_power_off = power_on; > > > > > > I'm assuming that you have checked all of the users of > > > dev_pm_domain_attach() and verified that the "power off" value is the > > > same as the "power on" one for all of them. > > > > In v2 it has been discussed to just mirror the power_on acquisition. > > > > Double checking now, all the current users of dev_pm_domain_attach() follow > > this rule, except the i2c bus. i2c powers on the domain conditionally: > > > > https://elixir.bootlin.com/linux/v6.15.2/source/drivers/i2c/i2c-core-base.c#L575 > > > > and powers it off unconditionally: > > https://elixir.bootlin.com/linux/v6.15.2/source/drivers/i2c/i2c-core-base.c#L638 > > > > Should we take this into account ? > > I think so. > > It is still sufficient to use one device flag to represent the > information whether or not to remove power on detach, but I would > change the second argument of dev_pm_domain_attach() to a u8 > representing a mask of bits: > > PM_DOMAIN_POWER_ON BIT(0) > PM_DOMAIN_POWER_OFF BIT(1) > > where PM_DOMAIN_POWER_ON will be set to indicate that the device > should be turned on right after attaching the PM domain and the value > of PM_DOMAIN_POWER_OFF will be stored in the new device flag. > > The majority of users will set or clear both, but i2c will set > PM_DOMAIN_POWER_OFF and either set of clear PM_DOMAIN_POWER_ON > depending on the do_power_on value. I am not sure it's needed, unless it's especially targeted for the ACPI PM domain, which I find hard to believe. Also, I find it awkward why the i2c bus should be any different from many other types of buses. It's probably just because of legacy and that someone took a decision when we added it. Wolfram, what's your thinking around this? Kind regards Uffe
diff --git a/drivers/base/dd.c b/drivers/base/dd.c index b526e0e0f52d..13ab98e033ea 100644 --- a/drivers/base/dd.c +++ b/drivers/base/dd.c @@ -25,6 +25,7 @@ #include <linux/kthread.h> #include <linux/wait.h> #include <linux/async.h> +#include <linux/pm_domain.h> #include <linux/pm_runtime.h> #include <linux/pinctrl/devinfo.h> #include <linux/slab.h> @@ -552,6 +553,7 @@ static void device_unbind_cleanup(struct device *dev) dev->dma_range_map = NULL; device_set_driver(dev, NULL); dev_set_drvdata(dev, NULL); + dev_pm_domain_detach(dev, dev->power.detach_power_off); if (dev->pm_domain && dev->pm_domain->dismiss) dev->pm_domain->dismiss(dev); pm_runtime_reinit(dev); diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c index 781968a128ff..a8f302ed27a5 100644 --- a/drivers/base/power/common.c +++ b/drivers/base/power/common.c @@ -111,6 +111,9 @@ int dev_pm_domain_attach(struct device *dev, bool power_on) if (!ret) ret = genpd_dev_pm_attach(dev); + if (dev->pm_domain) + dev->power.detach_power_off = power_on; + return ret < 0 ? ret : 0; } EXPORT_SYMBOL_GPL(dev_pm_domain_attach); diff --git a/include/linux/pm.h b/include/linux/pm.h index f0bd8fbae4f2..dcbe2c1ef59b 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -720,6 +720,7 @@ struct dev_pm_info { struct pm_subsys_data *subsys_data; /* Owned by the subsystem. */ void (*set_latency_tolerance)(struct device *, s32); struct dev_pm_qos *qos; + bool detach_power_off:1; }; extern int dev_pm_get_subsys_data(struct device *dev);