Re: [PATCH v2 1/2] PM: domains: Add devres variant for dev_pm_domain_attach()

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

Hi, Dmitry,

On 28.05.2025 00:27, Dmitry Torokhov wrote:
> Hi Claudiu,
> 
> On Mon, May 26, 2025 at 03:20:53PM +0300, Claudiu 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.
> 
> I think you are concentrating too much on runtime PM aspect of this. As
> you mentioned in the last sentence the same issue may happen in the
> absence of runtime PM if the power domain code will shut down the device
> while it is not fully cleaned up.

I see your point. Just wanted to describe the scenario that leads to this
patch.

> 
>>
>> Add devm_pm_domain_attach(). When replacing the dev_pm_domain_attach() and
>> dev_pm_domain_detach() in bus probe and bus remove, it ensures that the
>> device is detached from its PM domain in device_unbind_cleanup(), only
>> after all driver's devres-managed resources have been release.
>>
>> For flexibility, the implemented devm_pm_domain_attach() has 2 state
>> arguments, one for the domain state on attach, one for the domain state on
>> detach.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes in v2:
>> - none; this patch is new
>>
>>  drivers/base/power/common.c | 59 +++++++++++++++++++++++++++++++++++++
>>  include/linux/pm_domain.h   |  8 +++++
>>  2 files changed, 67 insertions(+)
>>
>> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
>> index 781968a128ff..6ef0924efe2e 100644
>> --- a/drivers/base/power/common.c
>> +++ b/drivers/base/power/common.c
>> @@ -115,6 +115,65 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
>>  }
>>  EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>>  
>> +/**
>> + * devm_pm_domain_detach_off - devres action for devm_pm_domain_attach() to
>> + * detach a device and power it off.
>> + * @dev: device to detach.
>> + *
>> + * This function reverse the actions from devm_pm_domain_attach().
>> + * It will be invoked during the remove phase from drivers implicitly.
>> + */
>> +static void devm_pm_domain_detach_off(void *dev)
>> +{
>> +	dev_pm_domain_detach(dev, true);
>> +}
>> +
>> +/**
>> + * devm_pm_domain_detach_on - devres action for devm_pm_domain_attach() to
>> + * detach a device and power it on.
>> + * @dev: device to detach.
>> + *
>> + * This function reverse the actions from devm_pm_domain_attach().
>> + * It will be invoked during the remove phase from drivers implicitly.
>> + */
>> +static void devm_pm_domain_detach_on(void *dev)
>> +{
>> +	dev_pm_domain_detach(dev, false);
>> +}
>> +
>> +/**
>> + * devm_pm_domain_attach - devres-enabled version of dev_pm_domain_attach()
>> + * @dev: Device to attach.
>> + * @attach_power_on: Use to indicate whether we should power on the device
>> + *                   when attaching (true indicates the device is powered on
>> + *                   when attaching).
>> + * @detach_power_off: Used to indicate whether we should power off the device
>> + *                    when detaching (true indicates the device is powered off
>> + *                    when detaching).
>> + *
>> + * NOTE: this will also handle calling dev_pm_domain_detach() for
>> + * you during remove phase.
>> + *
>> + * Returns 0 on successfully attached PM domain, or a negative error code in
>> + * case of a failure.
>> + */
>> +int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
>> +			  bool detach_power_off)
> 
> Do we have examples where we power on a device and leave it powered on
> (or do not power on device on attach but power off it on detach)? I

I haven't found one yet.

> believe devm release should strictly mirror the acquisition, so separate
> flag is not needed.

I was in the middle whether I should do it with 2 flags or only to revert
the acquisition.

> 
> 
>> +{
>> +	int ret;
>> +
>> +	ret = dev_pm_domain_attach(dev, attach_power_on);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (detach_power_off)
>> +		return devm_add_action_or_reset(dev, devm_pm_domain_detach_off,
>> +						dev);
>> +
>> +	return devm_add_action_or_reset(dev, devm_pm_domain_detach_on, dev);
> 
> Instead of 2 separate cleanup methods maybe define dedicated devres:
> 
> struct dev_pm_domain_devres {
> 	struct device *dev;
> 	bool power_off;
> }
> 
> ?

That was the other option I've thought about but I found the one with 2
cleanup methods to be simpler. What would you prefer here?

Ulf: could you please let me know what would you prefer here?

> 
>> +}
>> +EXPORT_SYMBOL_GPL(devm_pm_domain_attach);
>> +
>>  /**
>>   * dev_pm_domain_attach_by_id - Associate a device with one of its PM domains.
>>   * @dev: The device used to lookup the PM domain.
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 0b18160901a2..ee798b090d17 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -509,6 +509,8 @@ struct device *dev_pm_domain_attach_by_name(struct device *dev,
>>  int dev_pm_domain_attach_list(struct device *dev,
>>  			      const struct dev_pm_domain_attach_data *data,
>>  			      struct dev_pm_domain_list **list);
>> +int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
>> +			  bool detach_power_off);
>>  int devm_pm_domain_attach_list(struct device *dev,
>>  			       const struct dev_pm_domain_attach_data *data,
>>  			       struct dev_pm_domain_list **list);
>> @@ -539,6 +541,12 @@ static inline int dev_pm_domain_attach_list(struct device *dev,
>>  	return 0;
>>  }
>>  
>> +static int devm_pm_domain_attach(struct device *dev, bool attach_power_on,
> 
> Needs to be marked "inline".

Will do!

Thank you for your review,
Claudiu

> 
>> +				 bool detach_power_off)
>> +{
>> +	return 0;
>> +}
>> +
>>  static inline int devm_pm_domain_attach_list(struct device *dev,
>>  					     const struct dev_pm_domain_attach_data *data,
>>  					     struct dev_pm_domain_list **list)
> 
> Thanks.
>