Hi, Rafael,
On 16.06.2025 20:14, Rafael J. Wysocki wrote:
> On Mon, Jun 16, 2025 at 3:54 PM Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
>>
>> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxx>
>> ---
>>
>> 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
>>
