Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Jonathan Cameron]

On Wed, 19 Feb 2025 14:45:07 +0200
Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:

> Hi, Daniel, Jonathan,
> 
> On 15.02.2025 15:51, Claudiu Beznea wrote:
> > Hi, Greg,
> > 
> > On 15.02.2025 15:25, Greg KH wrote:  
> >> On Sat, Feb 15, 2025 at 03:08:49PM +0200, Claudiu wrote:  
> >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>
> >>> On the Renesas RZ/G3S (and other Renesas SoCs, e.g., RZ/G2{L, LC, UL}),
> >>> clocks are managed through PM domains. These PM domains, registered on
> >>> behalf of the clock controller driver, are configured with
> >>> GENPD_FLAG_PM_CLK. In most of the Renesas drivers used by RZ SoCs, the
> >>> clocks are enabled/disabled using runtime PM APIs. The power domains may
> >>> also have power_on/power_off support implemented. After the device PM
> >>> domain is powered off any CPU accesses to these domains leads to system
> >>> aborts.
> >>>
> >>> During probe, devices are attached to the PM domain controlling their
> >>> clocks and power. Similarly, during removal, devices are detached from the
> >>> PM domain.
> >>>
> >>> The detachment call stack is as follows:
> >>>
> >>> device_driver_detach() ->
> >>>   device_release_driver_internal() ->
> >>>     __device_release_driver() ->
> >>>       device_remove() ->
> >>>         platform_remove() ->
> >>> 	  dev_pm_domain_detach()
> >>>
> >>> During driver unbind, after the device is detached from its PM domain,
> >>> the device_unbind_cleanup() function is called, which subsequently invokes
> >>> devres_release_all(). This function handles devres resource cleanup.
> >>>
> >>> If runtime PM is enabled in driver probe via devm_pm_runtime_enable(), the
> >>> cleanup process triggers the action or reset function for disabling runtime
> >>> PM. This function is pm_runtime_disable_action(), which leads to the
> >>> following call stack of interest when called:
> >>>
> >>> pm_runtime_disable_action() ->
> >>>   pm_runtime_dont_use_autosuspend() ->
> >>>     __pm_runtime_use_autosuspend() ->
> >>>       update_autosuspend() ->
> >>>         rpm_idle()
> >>>
> >>> The rpm_idle() function attempts to resume the device at runtime. However,
> >>> at the point it is called, the device is no longer part of a PM domain
> >>> (which manages clocks and power states). If the driver implements its own
> >>> runtime PM APIs for specific functionalities - such as the rzg2l_adc
> >>> driver - while also relying on the power domain subsystem for power
> >>> management, rpm_idle() will invoke the driver's runtime PM API. However,
> >>> since the device is no longer part of a PM domain at this point, the PM
> >>> domain's runtime PM APIs will not be called. This leads to system aborts on
> >>> Renesas SoCs.
> >>>
> >>> Another identified case is when a subsystem performs various cleanups
> >>> using device_unbind_cleanup(), calling driver-specific APIs in the process.
> >>> A known example is the thermal subsystem, which may call driver-specific
> >>> APIs to disable the thermal device. The relevant call stack in this case
> >>> is:
> >>>
> >>> device_driver_detach() ->
> >>>   device_release_driver_internal() ->
> >>>     device_unbind_cleanup() ->
> >>>       devres_release_all() ->
> >>>         devm_thermal_of_zone_release() ->
> >>> 	  thermal_zone_device_disable() ->
> >>> 	    thermal_zone_device_set_mode() ->
> >>> 	      struct thermal_zone_device_ops::change_mode()
> >>>
> >>> At the moment the driver-specific change_mode() API is called, the device
> >>> is no longer part of its PM domain. Accessing its registers without proper
> >>> power management leads to system aborts.
> >>>
> >>> Open a devres group before calling the driver probe, and close it
> >>> immediately after the driver remove function is called and before
> >>> dev_pm_domain_detach(). This ensures that driver-specific devm actions or
> >>> reset functions are executed immediately after the driver remove function
> >>> completes. Additionally, it prevents driver-specific runtime PM APIs from
> >>> being called when the device is no longer part of its power domain.
> >>>
> >>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>> ---
> >>>
> >>> Hi,

Hi Claudiu, Greg,

Sorry, I missed this thread whilst travelling and only saw it because
of reference from the in driver solution.

> >>>
> >>> Although Ulf gave its green light for the approaches on both IIO [1],
> >>> [2] and thermal subsystems [3], Jonathan considered unacceptable the
> >>> approaches in [1], [2] as he considered it may lead to dificult to
> >>> maintain code and code opened to subtle bugs (due to the potential of
> >>> mixing devres and non-devres calls). He pointed out a similar approach
> >>> that was done for the I2C bus [4], [5].
> >>>
> >>> As the discussions in [1], [2] stopped w/o a clear conclusion, this
> >>> patch tries to revive it by proposing a similar approach that was done
> >>> for the I2C bus.
> >>>
> >>> Please let me know you input.  
> >>
> >> I'm with Jonathan here, the devres stuff is getting crazy here and you
> >> have drivers mixing them and side affects happening and lots of
> >> confusion.  Your change here is only going to make it even more
> >> confusing, and shouldn't actually solve it for other busses (i.e. what
> >> about iio devices NOT on the platform bus?)  

In some cases they are already carrying the support as per the link
above covering all i2c drivers.  I'd like to see a generic solution and
I suspect pushing it to the device drivers rather than the bus code
will explode badly and leave us with subtle bugs where people don't
realise it is necessary. 

https://lore.kernel.org/all/20250224120608.1769039-1-claudiu.beznea.uj@bp.renesas.com/
is a lot nastier looking than what we have here. I'll review that in a minute
to show that it need not be that bad, but none the less not pleasant.

+CC linux-iio to join up threads and Dmitry wrt to i2c case (and HID that does
similar)

> > 
> > You're right, other busses will still have this problem.
> >   
> >>
> >> Why can't your individual driver handle this instead?  

In my mind because it's the bus code that is doing the unexpected part by
making calls in the remove path that are effectively not in the same order
as probe because they occur between driver remove and related devres cleanup
for stuff registered in probe.

> > 
> > Initially I tried it at the driver level by using non-devres PM runtime
> > enable API but wasn't considered OK by all parties.
> > 
> > I haven't thought about having devres_open_group()/devres_close_group() in
> > the driver itself but it should work.  
> 
> Are you OK with having the devres_open_group()/devres_close_group() in the
> currently known affected drivers (drivers/iio/adc/rzg2l_adc.c and the
> proposed drivers/thermal/renesas/rzg3s_thermal.c [1]) ?

I guess it may be the best of a bunch of not particularly nasty solutions...

Jonathan

> 
> Thank you,
> Claudiu
> 
> [1]
> https://lore.kernel.org/all/20250103163805.1775705-5-claudiu.beznea.uj@bp.renesas.com
> 
> > 
> > Thank you,
> > Claudiu
> >   
> >>
> >> thanks,
> >>
> >> greg k-h  
> >   
> 

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Dmitry Torokhov]

On Wed, Mar 05, 2025 at 02:03:09PM +0000, Jonathan Cameron wrote:
> On Wed, 19 Feb 2025 14:45:07 +0200
> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> 
> > Hi, Daniel, Jonathan,
> > 
> > On 15.02.2025 15:51, Claudiu Beznea wrote:
> > > Hi, Greg,
> > > 
> > > On 15.02.2025 15:25, Greg KH wrote:  
> > >> On Sat, Feb 15, 2025 at 03:08:49PM +0200, Claudiu wrote:  
> > >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > >>>
> > >>> On the Renesas RZ/G3S (and other Renesas SoCs, e.g., RZ/G2{L, LC, UL}),
> > >>> clocks are managed through PM domains. These PM domains, registered on
> > >>> behalf of the clock controller driver, are configured with
> > >>> GENPD_FLAG_PM_CLK. In most of the Renesas drivers used by RZ SoCs, the
> > >>> clocks are enabled/disabled using runtime PM APIs. The power domains may
> > >>> also have power_on/power_off support implemented. After the device PM
> > >>> domain is powered off any CPU accesses to these domains leads to system
> > >>> aborts.
> > >>>
> > >>> During probe, devices are attached to the PM domain controlling their
> > >>> clocks and power. Similarly, during removal, devices are detached from the
> > >>> PM domain.
> > >>>
> > >>> The detachment call stack is as follows:
> > >>>
> > >>> device_driver_detach() ->
> > >>>   device_release_driver_internal() ->
> > >>>     __device_release_driver() ->
> > >>>       device_remove() ->
> > >>>         platform_remove() ->
> > >>> 	  dev_pm_domain_detach()
> > >>>
> > >>> During driver unbind, after the device is detached from its PM domain,
> > >>> the device_unbind_cleanup() function is called, which subsequently invokes
> > >>> devres_release_all(). This function handles devres resource cleanup.
> > >>>
> > >>> If runtime PM is enabled in driver probe via devm_pm_runtime_enable(), the
> > >>> cleanup process triggers the action or reset function for disabling runtime
> > >>> PM. This function is pm_runtime_disable_action(), which leads to the
> > >>> following call stack of interest when called:
> > >>>
> > >>> pm_runtime_disable_action() ->
> > >>>   pm_runtime_dont_use_autosuspend() ->
> > >>>     __pm_runtime_use_autosuspend() ->
> > >>>       update_autosuspend() ->
> > >>>         rpm_idle()
> > >>>
> > >>> The rpm_idle() function attempts to resume the device at runtime. However,
> > >>> at the point it is called, the device is no longer part of a PM domain
> > >>> (which manages clocks and power states). If the driver implements its own
> > >>> runtime PM APIs for specific functionalities - such as the rzg2l_adc
> > >>> driver - while also relying on the power domain subsystem for power
> > >>> management, rpm_idle() will invoke the driver's runtime PM API. However,
> > >>> since the device is no longer part of a PM domain at this point, the PM
> > >>> domain's runtime PM APIs will not be called. This leads to system aborts on
> > >>> Renesas SoCs.
> > >>>
> > >>> Another identified case is when a subsystem performs various cleanups
> > >>> using device_unbind_cleanup(), calling driver-specific APIs in the process.
> > >>> A known example is the thermal subsystem, which may call driver-specific
> > >>> APIs to disable the thermal device. The relevant call stack in this case
> > >>> is:
> > >>>
> > >>> device_driver_detach() ->
> > >>>   device_release_driver_internal() ->
> > >>>     device_unbind_cleanup() ->
> > >>>       devres_release_all() ->
> > >>>         devm_thermal_of_zone_release() ->
> > >>> 	  thermal_zone_device_disable() ->
> > >>> 	    thermal_zone_device_set_mode() ->
> > >>> 	      struct thermal_zone_device_ops::change_mode()
> > >>>
> > >>> At the moment the driver-specific change_mode() API is called, the device
> > >>> is no longer part of its PM domain. Accessing its registers without proper
> > >>> power management leads to system aborts.
> > >>>
> > >>> Open a devres group before calling the driver probe, and close it
> > >>> immediately after the driver remove function is called and before
> > >>> dev_pm_domain_detach(). This ensures that driver-specific devm actions or
> > >>> reset functions are executed immediately after the driver remove function
> > >>> completes. Additionally, it prevents driver-specific runtime PM APIs from
> > >>> being called when the device is no longer part of its power domain.
> > >>>
> > >>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > >>> ---
> > >>>
> > >>> Hi,
> 
> Hi Claudiu, Greg,
> 
> Sorry, I missed this thread whilst travelling and only saw it because
> of reference from the in driver solution.
> 
> > >>>
> > >>> Although Ulf gave its green light for the approaches on both IIO [1],
> > >>> [2] and thermal subsystems [3], Jonathan considered unacceptable the
> > >>> approaches in [1], [2] as he considered it may lead to dificult to
> > >>> maintain code and code opened to subtle bugs (due to the potential of
> > >>> mixing devres and non-devres calls). He pointed out a similar approach
> > >>> that was done for the I2C bus [4], [5].
> > >>>
> > >>> As the discussions in [1], [2] stopped w/o a clear conclusion, this
> > >>> patch tries to revive it by proposing a similar approach that was done
> > >>> for the I2C bus.
> > >>>
> > >>> Please let me know you input.  
> > >>
> > >> I'm with Jonathan here, the devres stuff is getting crazy here and you
> > >> have drivers mixing them and side affects happening and lots of
> > >> confusion.  Your change here is only going to make it even more
> > >> confusing, and shouldn't actually solve it for other busses (i.e. what
> > >> about iio devices NOT on the platform bus?)  
> 
> In some cases they are already carrying the support as per the link
> above covering all i2c drivers.  I'd like to see a generic solution and
> I suspect pushing it to the device drivers rather than the bus code
> will explode badly and leave us with subtle bugs where people don't
> realise it is necessary. 
> 
> https://lore.kernel.org/all/20250224120608.1769039-1-claudiu.beznea.uj@bp.renesas.com/
> is a lot nastier looking than what we have here. I'll review that in a minute
> to show that it need not be that bad, but none the less not pleasant.
> 
> +CC linux-iio to join up threads and Dmitry wrt to i2c case (and HID that does
> similar)

We should not expect individual drivers handle this, because this is a
layering violation: they need to know implementation details of the bus
code to know if the bus is using non-devres managed resources, and
adjust their behavior. Moving this into driver core is also not
feasible, as not all buses need it. So IMO this should belong to
individual bus code.

Instead of using devres group a bus may opt to use
devm_add_action_or_reset() and other devm APIs to make sure bus'
resource unwinding is carried in the correct order relative to freeing
driver-owned resources.

> 
> > > 
> > > You're right, other busses will still have this problem.
> > >   
> > >>
> > >> Why can't your individual driver handle this instead?  
> 
> In my mind because it's the bus code that is doing the unexpected part by
> making calls in the remove path that are effectively not in the same order
> as probe because they occur between driver remove and related devres cleanup
> for stuff registered in probe.
> 
> > > 
> > > Initially I tried it at the driver level by using non-devres PM runtime
> > > enable API but wasn't considered OK by all parties.
> > > 
> > > I haven't thought about having devres_open_group()/devres_close_group() in
> > > the driver itself but it should work.  
> > 
> > Are you OK with having the devres_open_group()/devres_close_group() in the
> > currently known affected drivers (drivers/iio/adc/rzg2l_adc.c and the
> > proposed drivers/thermal/renesas/rzg3s_thermal.c [1]) ?
> 
> I guess it may be the best of a bunch of not particularly nasty solutions...

We need to update _ALL_ platform drivers using devm then, and this is
clearly not scalable.

Thanks.

-- 
Dmitry

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Claudiu Beznea]

Hi, Rafael,

On 06.03.2025 08:11, Dmitry Torokhov wrote:
> On Wed, Mar 05, 2025 at 02:03:09PM +0000, Jonathan Cameron wrote:
>> On Wed, 19 Feb 2025 14:45:07 +0200
>> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>
>>> Hi, Daniel, Jonathan,
>>>
>>> On 15.02.2025 15:51, Claudiu Beznea wrote:
>>>> Hi, Greg,
>>>>
>>>> On 15.02.2025 15:25, Greg KH wrote:  
>>>>> On Sat, Feb 15, 2025 at 03:08:49PM +0200, Claudiu wrote:  
>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>
>>>>>> On the Renesas RZ/G3S (and other Renesas SoCs, e.g., RZ/G2{L, LC, UL}),
>>>>>> clocks are managed through PM domains. These PM domains, registered on
>>>>>> behalf of the clock controller driver, are configured with
>>>>>> GENPD_FLAG_PM_CLK. In most of the Renesas drivers used by RZ SoCs, the
>>>>>> clocks are enabled/disabled using runtime PM APIs. The power domains may
>>>>>> also have power_on/power_off support implemented. After the device PM
>>>>>> domain is powered off any CPU accesses to these domains leads to system
>>>>>> aborts.
>>>>>>
>>>>>> During probe, devices are attached to the PM domain controlling their
>>>>>> clocks and power. Similarly, during removal, devices are detached from the
>>>>>> PM domain.
>>>>>>
>>>>>> The detachment call stack is as follows:
>>>>>>
>>>>>> device_driver_detach() ->
>>>>>>   device_release_driver_internal() ->
>>>>>>     __device_release_driver() ->
>>>>>>       device_remove() ->
>>>>>>         platform_remove() ->
>>>>>> 	  dev_pm_domain_detach()
>>>>>>
>>>>>> During driver unbind, after the device is detached from its PM domain,
>>>>>> the device_unbind_cleanup() function is called, which subsequently invokes
>>>>>> devres_release_all(). This function handles devres resource cleanup.
>>>>>>
>>>>>> If runtime PM is enabled in driver probe via devm_pm_runtime_enable(), the
>>>>>> cleanup process triggers the action or reset function for disabling runtime
>>>>>> PM. This function is pm_runtime_disable_action(), which leads to the
>>>>>> following call stack of interest when called:
>>>>>>
>>>>>> pm_runtime_disable_action() ->
>>>>>>   pm_runtime_dont_use_autosuspend() ->
>>>>>>     __pm_runtime_use_autosuspend() ->
>>>>>>       update_autosuspend() ->
>>>>>>         rpm_idle()
>>>>>>
>>>>>> The rpm_idle() function attempts to resume the device at runtime. However,
>>>>>> at the point it is called, the device is no longer part of a PM domain
>>>>>> (which manages clocks and power states). If the driver implements its own
>>>>>> runtime PM APIs for specific functionalities - such as the rzg2l_adc
>>>>>> driver - while also relying on the power domain subsystem for power
>>>>>> management, rpm_idle() will invoke the driver's runtime PM API. However,
>>>>>> since the device is no longer part of a PM domain at this point, the PM
>>>>>> domain's runtime PM APIs will not be called. This leads to system aborts on
>>>>>> Renesas SoCs.
>>>>>>
>>>>>> Another identified case is when a subsystem performs various cleanups
>>>>>> using device_unbind_cleanup(), calling driver-specific APIs in the process.
>>>>>> A known example is the thermal subsystem, which may call driver-specific
>>>>>> APIs to disable the thermal device. The relevant call stack in this case
>>>>>> is:
>>>>>>
>>>>>> device_driver_detach() ->
>>>>>>   device_release_driver_internal() ->
>>>>>>     device_unbind_cleanup() ->
>>>>>>       devres_release_all() ->
>>>>>>         devm_thermal_of_zone_release() ->
>>>>>> 	  thermal_zone_device_disable() ->
>>>>>> 	    thermal_zone_device_set_mode() ->
>>>>>> 	      struct thermal_zone_device_ops::change_mode()
>>>>>>
>>>>>> At the moment the driver-specific change_mode() API is called, the device
>>>>>> is no longer part of its PM domain. Accessing its registers without proper
>>>>>> power management leads to system aborts.
>>>>>>
>>>>>> Open a devres group before calling the driver probe, and close it
>>>>>> immediately after the driver remove function is called and before
>>>>>> dev_pm_domain_detach(). This ensures that driver-specific devm actions or
>>>>>> reset functions are executed immediately after the driver remove function
>>>>>> completes. Additionally, it prevents driver-specific runtime PM APIs from
>>>>>> being called when the device is no longer part of its power domain.
>>>>>>
>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>> ---
>>>>>>
>>>>>> Hi,
>>
>> Hi Claudiu, Greg,
>>
>> Sorry, I missed this thread whilst travelling and only saw it because
>> of reference from the in driver solution.
>>
>>>>>>
>>>>>> Although Ulf gave its green light for the approaches on both IIO [1],
>>>>>> [2] and thermal subsystems [3], Jonathan considered unacceptable the
>>>>>> approaches in [1], [2] as he considered it may lead to dificult to
>>>>>> maintain code and code opened to subtle bugs (due to the potential of
>>>>>> mixing devres and non-devres calls). He pointed out a similar approach
>>>>>> that was done for the I2C bus [4], [5].
>>>>>>
>>>>>> As the discussions in [1], [2] stopped w/o a clear conclusion, this
>>>>>> patch tries to revive it by proposing a similar approach that was done
>>>>>> for the I2C bus.
>>>>>>
>>>>>> Please let me know you input.  
>>>>>
>>>>> I'm with Jonathan here, the devres stuff is getting crazy here and you
>>>>> have drivers mixing them and side affects happening and lots of
>>>>> confusion.  Your change here is only going to make it even more
>>>>> confusing, and shouldn't actually solve it for other busses (i.e. what
>>>>> about iio devices NOT on the platform bus?)  
>>
>> In some cases they are already carrying the support as per the link
>> above covering all i2c drivers.  I'd like to see a generic solution and
>> I suspect pushing it to the device drivers rather than the bus code
>> will explode badly and leave us with subtle bugs where people don't
>> realise it is necessary. 
>>
>> https://lore.kernel.org/all/20250224120608.1769039-1-claudiu.beznea.uj@bp.renesas.com/
>> is a lot nastier looking than what we have here. I'll review that in a minute
>> to show that it need not be that bad, but none the less not pleasant.
>>
>> +CC linux-iio to join up threads and Dmitry wrt to i2c case (and HID that does
>> similar)
> 
> We should not expect individual drivers handle this, because this is a
> layering violation: they need to know implementation details of the bus
> code to know if the bus is using non-devres managed resources, and
> adjust their behavior. Moving this into driver core is also not
> feasible, as not all buses need it. So IMO this should belong to
> individual bus code.
> 
> Instead of using devres group a bus may opt to use
> devm_add_action_or_reset() and other devm APIs to make sure bus'
> resource unwinding is carried in the correct order relative to freeing
> driver-owned resources.

Can you please let us know your input on the approach proposed in this
patch? Or if you would prefer devm_add_action_or_reset() as suggested by
Dmitry? Or if you consider another approach would fit better?

Currently there were issues identified with the rzg2l-adc driver (driver
based solution proposed in [1]) and with the rzg3s thermal driver (solved
by function rzg3s_thermal_probe() from [2]).

As expressed previously by Jonathan and Dimitry this is a common problem
and as the issue is due to a call in the bus driver, would be better and
simpler to handle it in the bus driver. Otherwise, individual drivers would
have to be adjusted in a similar way.

Thank you,
Claudiu

[1]
https://lore.kernel.org/all/20250324122627.32336-2-claudiu.beznea.uj@bp.renesas.com/
[2]
https://lore.kernel.org/all/20250324135701.179827-3-claudiu.beznea.uj@bp.renesas.com/

> 
>>
>>>>
>>>> You're right, other busses will still have this problem.
>>>>   
>>>>>
>>>>> Why can't your individual driver handle this instead?  
>>
>> In my mind because it's the bus code that is doing the unexpected part by
>> making calls in the remove path that are effectively not in the same order
>> as probe because they occur between driver remove and related devres cleanup
>> for stuff registered in probe.
>>
>>>>
>>>> Initially I tried it at the driver level by using non-devres PM runtime
>>>> enable API but wasn't considered OK by all parties.
>>>>
>>>> I haven't thought about having devres_open_group()/devres_close_group() in
>>>> the driver itself but it should work.  
>>>
>>> Are you OK with having the devres_open_group()/devres_close_group() in the
>>> currently known affected drivers (drivers/iio/adc/rzg2l_adc.c and the
>>> proposed drivers/thermal/renesas/rzg3s_thermal.c [1]) ?
>>
>> I guess it may be the best of a bunch of not particularly nasty solutions...
> 
> We need to update _ALL_ platform drivers using devm then, and this is
> clearly not scalable.
> 
> Thanks.
> 

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Jonathan Cameron]

On Thu, 27 Mar 2025 18:47:53 +0200
Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:

> Hi, Rafael,
> 
> On 06.03.2025 08:11, Dmitry Torokhov wrote:
> > On Wed, Mar 05, 2025 at 02:03:09PM +0000, Jonathan Cameron wrote:  
> >> On Wed, 19 Feb 2025 14:45:07 +0200
> >> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >>  
> >>> Hi, Daniel, Jonathan,
> >>>
> >>> On 15.02.2025 15:51, Claudiu Beznea wrote:  
> >>>> Hi, Greg,
> >>>>
> >>>> On 15.02.2025 15:25, Greg KH wrote:    
> >>>>> On Sat, Feb 15, 2025 at 03:08:49PM +0200, Claudiu wrote:    
> >>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>>>
> >>>>>> On the Renesas RZ/G3S (and other Renesas SoCs, e.g., RZ/G2{L, LC, UL}),
> >>>>>> clocks are managed through PM domains. These PM domains, registered on
> >>>>>> behalf of the clock controller driver, are configured with
> >>>>>> GENPD_FLAG_PM_CLK. In most of the Renesas drivers used by RZ SoCs, the
> >>>>>> clocks are enabled/disabled using runtime PM APIs. The power domains may
> >>>>>> also have power_on/power_off support implemented. After the device PM
> >>>>>> domain is powered off any CPU accesses to these domains leads to system
> >>>>>> aborts.
> >>>>>>
> >>>>>> During probe, devices are attached to the PM domain controlling their
> >>>>>> clocks and power. Similarly, during removal, devices are detached from the
> >>>>>> PM domain.
> >>>>>>
> >>>>>> The detachment call stack is as follows:
> >>>>>>
> >>>>>> device_driver_detach() ->
> >>>>>>   device_release_driver_internal() ->
> >>>>>>     __device_release_driver() ->
> >>>>>>       device_remove() ->
> >>>>>>         platform_remove() ->
> >>>>>> 	  dev_pm_domain_detach()
> >>>>>>
> >>>>>> During driver unbind, after the device is detached from its PM domain,
> >>>>>> the device_unbind_cleanup() function is called, which subsequently invokes
> >>>>>> devres_release_all(). This function handles devres resource cleanup.
> >>>>>>
> >>>>>> If runtime PM is enabled in driver probe via devm_pm_runtime_enable(), the
> >>>>>> cleanup process triggers the action or reset function for disabling runtime
> >>>>>> PM. This function is pm_runtime_disable_action(), which leads to the
> >>>>>> following call stack of interest when called:
> >>>>>>
> >>>>>> pm_runtime_disable_action() ->
> >>>>>>   pm_runtime_dont_use_autosuspend() ->
> >>>>>>     __pm_runtime_use_autosuspend() ->
> >>>>>>       update_autosuspend() ->
> >>>>>>         rpm_idle()
> >>>>>>
> >>>>>> The rpm_idle() function attempts to resume the device at runtime. However,
> >>>>>> at the point it is called, the device is no longer part of a PM domain
> >>>>>> (which manages clocks and power states). If the driver implements its own
> >>>>>> runtime PM APIs for specific functionalities - such as the rzg2l_adc
> >>>>>> driver - while also relying on the power domain subsystem for power
> >>>>>> management, rpm_idle() will invoke the driver's runtime PM API. However,
> >>>>>> since the device is no longer part of a PM domain at this point, the PM
> >>>>>> domain's runtime PM APIs will not be called. This leads to system aborts on
> >>>>>> Renesas SoCs.
> >>>>>>
> >>>>>> Another identified case is when a subsystem performs various cleanups
> >>>>>> using device_unbind_cleanup(), calling driver-specific APIs in the process.
> >>>>>> A known example is the thermal subsystem, which may call driver-specific
> >>>>>> APIs to disable the thermal device. The relevant call stack in this case
> >>>>>> is:
> >>>>>>
> >>>>>> device_driver_detach() ->
> >>>>>>   device_release_driver_internal() ->
> >>>>>>     device_unbind_cleanup() ->
> >>>>>>       devres_release_all() ->
> >>>>>>         devm_thermal_of_zone_release() ->
> >>>>>> 	  thermal_zone_device_disable() ->
> >>>>>> 	    thermal_zone_device_set_mode() ->
> >>>>>> 	      struct thermal_zone_device_ops::change_mode()
> >>>>>>
> >>>>>> At the moment the driver-specific change_mode() API is called, the device
> >>>>>> is no longer part of its PM domain. Accessing its registers without proper
> >>>>>> power management leads to system aborts.
> >>>>>>
> >>>>>> Open a devres group before calling the driver probe, and close it
> >>>>>> immediately after the driver remove function is called and before
> >>>>>> dev_pm_domain_detach(). This ensures that driver-specific devm actions or
> >>>>>> reset functions are executed immediately after the driver remove function
> >>>>>> completes. Additionally, it prevents driver-specific runtime PM APIs from
> >>>>>> being called when the device is no longer part of its power domain.
> >>>>>>
> >>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>>> ---
> >>>>>>
> >>>>>> Hi,  
> >>
> >> Hi Claudiu, Greg,
> >>
> >> Sorry, I missed this thread whilst travelling and only saw it because
> >> of reference from the in driver solution.
> >>  
> >>>>>>
> >>>>>> Although Ulf gave its green light for the approaches on both IIO [1],
> >>>>>> [2] and thermal subsystems [3], Jonathan considered unacceptable the
> >>>>>> approaches in [1], [2] as he considered it may lead to dificult to
> >>>>>> maintain code and code opened to subtle bugs (due to the potential of
> >>>>>> mixing devres and non-devres calls). He pointed out a similar approach
> >>>>>> that was done for the I2C bus [4], [5].
> >>>>>>
> >>>>>> As the discussions in [1], [2] stopped w/o a clear conclusion, this
> >>>>>> patch tries to revive it by proposing a similar approach that was done
> >>>>>> for the I2C bus.
> >>>>>>
> >>>>>> Please let me know you input.    
> >>>>>
> >>>>> I'm with Jonathan here, the devres stuff is getting crazy here and you
> >>>>> have drivers mixing them and side affects happening and lots of
> >>>>> confusion.  Your change here is only going to make it even more
> >>>>> confusing, and shouldn't actually solve it for other busses (i.e. what
> >>>>> about iio devices NOT on the platform bus?)    
> >>
> >> In some cases they are already carrying the support as per the link
> >> above covering all i2c drivers.  I'd like to see a generic solution and
> >> I suspect pushing it to the device drivers rather than the bus code
> >> will explode badly and leave us with subtle bugs where people don't
> >> realise it is necessary. 
> >>
> >> https://lore.kernel.org/all/20250224120608.1769039-1-claudiu.beznea.uj@bp.renesas.com/
> >> is a lot nastier looking than what we have here. I'll review that in a minute
> >> to show that it need not be that bad, but none the less not pleasant.
> >>
> >> +CC linux-iio to join up threads and Dmitry wrt to i2c case (and HID that does
> >> similar)  
> > 
> > We should not expect individual drivers handle this, because this is a
> > layering violation: they need to know implementation details of the bus
> > code to know if the bus is using non-devres managed resources, and
> > adjust their behavior. Moving this into driver core is also not
> > feasible, as not all buses need it. So IMO this should belong to
> > individual bus code.
> > 
> > Instead of using devres group a bus may opt to use
> > devm_add_action_or_reset() and other devm APIs to make sure bus'
> > resource unwinding is carried in the correct order relative to freeing
> > driver-owned resources.  
> 
> Can you please let us know your input on the approach proposed in this
> patch? Or if you would prefer devm_add_action_or_reset() as suggested by
> Dmitry? Or if you consider another approach would fit better?
> 
> Currently there were issues identified with the rzg2l-adc driver (driver
> based solution proposed in [1]) and with the rzg3s thermal driver (solved
> by function rzg3s_thermal_probe() from [2]).
> 
> As expressed previously by Jonathan and Dimitry this is a common problem
> and as the issue is due to a call in the bus driver, would be better and
> simpler to handle it in the bus driver. Otherwise, individual drivers would
> have to be adjusted in a similar way.
> 

Rafael,

Greg suggested we ask for your input on the right option:

https://lore.kernel.org/all/2025032703-genre-excitable-9473@gregkh/
(that thread has the other option).

Jonathan

> Thank you,
> Claudiu
> 
> [1]
> https://lore.kernel.org/all/20250324122627.32336-2-claudiu.beznea.uj@bp.renesas.com/
> [2]
> https://lore.kernel.org/all/20250324135701.179827-3-claudiu.beznea.uj@bp.renesas.com/
> 
> >   
> >>  
> >>>>
> >>>> You're right, other busses will still have this problem.
> >>>>     
> >>>>>
> >>>>> Why can't your individual driver handle this instead?    
> >>
> >> In my mind because it's the bus code that is doing the unexpected part by
> >> making calls in the remove path that are effectively not in the same order
> >> as probe because they occur between driver remove and related devres cleanup
> >> for stuff registered in probe.
> >>  
> >>>>
> >>>> Initially I tried it at the driver level by using non-devres PM runtime
> >>>> enable API but wasn't considered OK by all parties.
> >>>>
> >>>> I haven't thought about having devres_open_group()/devres_close_group() in
> >>>> the driver itself but it should work.    
> >>>
> >>> Are you OK with having the devres_open_group()/devres_close_group() in the
> >>> currently known affected drivers (drivers/iio/adc/rzg2l_adc.c and the
> >>> proposed drivers/thermal/renesas/rzg3s_thermal.c [1]) ?  
> >>
> >> I guess it may be the best of a bunch of not particularly nasty solutions...  
> > 
> > We need to update _ALL_ platform drivers using devm then, and this is
> > clearly not scalable.
> > 
> > Thanks.
> >   
> 

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Claudiu Beznea]

Hi, Rafael,

On 30.03.2025 18:31, Jonathan Cameron wrote:
> On Thu, 27 Mar 2025 18:47:53 +0200
> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> 
>> Hi, Rafael,
>>
>> On 06.03.2025 08:11, Dmitry Torokhov wrote:
>>> On Wed, Mar 05, 2025 at 02:03:09PM +0000, Jonathan Cameron wrote:  
>>>> On Wed, 19 Feb 2025 14:45:07 +0200
>>>> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>>>  
>>>>> Hi, Daniel, Jonathan,
>>>>>
>>>>> On 15.02.2025 15:51, Claudiu Beznea wrote:  
>>>>>> Hi, Greg,
>>>>>>
>>>>>> On 15.02.2025 15:25, Greg KH wrote:    
>>>>>>> On Sat, Feb 15, 2025 at 03:08:49PM +0200, Claudiu wrote:    
>>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>>>
>>>>>>>> On the Renesas RZ/G3S (and other Renesas SoCs, e.g., RZ/G2{L, LC, UL}),
>>>>>>>> clocks are managed through PM domains. These PM domains, registered on
>>>>>>>> behalf of the clock controller driver, are configured with
>>>>>>>> GENPD_FLAG_PM_CLK. In most of the Renesas drivers used by RZ SoCs, the
>>>>>>>> clocks are enabled/disabled using runtime PM APIs. The power domains may
>>>>>>>> also have power_on/power_off support implemented. After the device PM
>>>>>>>> domain is powered off any CPU accesses to these domains leads to system
>>>>>>>> aborts.
>>>>>>>>
>>>>>>>> During probe, devices are attached to the PM domain controlling their
>>>>>>>> clocks and power. Similarly, during removal, devices are detached from the
>>>>>>>> PM domain.
>>>>>>>>
>>>>>>>> The detachment call stack is as follows:
>>>>>>>>
>>>>>>>> device_driver_detach() ->
>>>>>>>>   device_release_driver_internal() ->
>>>>>>>>     __device_release_driver() ->
>>>>>>>>       device_remove() ->
>>>>>>>>         platform_remove() ->
>>>>>>>> 	  dev_pm_domain_detach()
>>>>>>>>
>>>>>>>> During driver unbind, after the device is detached from its PM domain,
>>>>>>>> the device_unbind_cleanup() function is called, which subsequently invokes
>>>>>>>> devres_release_all(). This function handles devres resource cleanup.
>>>>>>>>
>>>>>>>> If runtime PM is enabled in driver probe via devm_pm_runtime_enable(), the
>>>>>>>> cleanup process triggers the action or reset function for disabling runtime
>>>>>>>> PM. This function is pm_runtime_disable_action(), which leads to the
>>>>>>>> following call stack of interest when called:
>>>>>>>>
>>>>>>>> pm_runtime_disable_action() ->
>>>>>>>>   pm_runtime_dont_use_autosuspend() ->
>>>>>>>>     __pm_runtime_use_autosuspend() ->
>>>>>>>>       update_autosuspend() ->
>>>>>>>>         rpm_idle()
>>>>>>>>
>>>>>>>> The rpm_idle() function attempts to resume the device at runtime. However,
>>>>>>>> at the point it is called, the device is no longer part of a PM domain
>>>>>>>> (which manages clocks and power states). If the driver implements its own
>>>>>>>> runtime PM APIs for specific functionalities - such as the rzg2l_adc
>>>>>>>> driver - while also relying on the power domain subsystem for power
>>>>>>>> management, rpm_idle() will invoke the driver's runtime PM API. However,
>>>>>>>> since the device is no longer part of a PM domain at this point, the PM
>>>>>>>> domain's runtime PM APIs will not be called. This leads to system aborts on
>>>>>>>> Renesas SoCs.
>>>>>>>>
>>>>>>>> Another identified case is when a subsystem performs various cleanups
>>>>>>>> using device_unbind_cleanup(), calling driver-specific APIs in the process.
>>>>>>>> A known example is the thermal subsystem, which may call driver-specific
>>>>>>>> APIs to disable the thermal device. The relevant call stack in this case
>>>>>>>> is:
>>>>>>>>
>>>>>>>> device_driver_detach() ->
>>>>>>>>   device_release_driver_internal() ->
>>>>>>>>     device_unbind_cleanup() ->
>>>>>>>>       devres_release_all() ->
>>>>>>>>         devm_thermal_of_zone_release() ->
>>>>>>>> 	  thermal_zone_device_disable() ->
>>>>>>>> 	    thermal_zone_device_set_mode() ->
>>>>>>>> 	      struct thermal_zone_device_ops::change_mode()
>>>>>>>>
>>>>>>>> At the moment the driver-specific change_mode() API is called, the device
>>>>>>>> is no longer part of its PM domain. Accessing its registers without proper
>>>>>>>> power management leads to system aborts.
>>>>>>>>
>>>>>>>> Open a devres group before calling the driver probe, and close it
>>>>>>>> immediately after the driver remove function is called and before
>>>>>>>> dev_pm_domain_detach(). This ensures that driver-specific devm actions or
>>>>>>>> reset functions are executed immediately after the driver remove function
>>>>>>>> completes. Additionally, it prevents driver-specific runtime PM APIs from
>>>>>>>> being called when the device is no longer part of its power domain.
>>>>>>>>
>>>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Hi,  
>>>>
>>>> Hi Claudiu, Greg,
>>>>
>>>> Sorry, I missed this thread whilst travelling and only saw it because
>>>> of reference from the in driver solution.
>>>>  
>>>>>>>>
>>>>>>>> Although Ulf gave its green light for the approaches on both IIO [1],
>>>>>>>> [2] and thermal subsystems [3], Jonathan considered unacceptable the
>>>>>>>> approaches in [1], [2] as he considered it may lead to dificult to
>>>>>>>> maintain code and code opened to subtle bugs (due to the potential of
>>>>>>>> mixing devres and non-devres calls). He pointed out a similar approach
>>>>>>>> that was done for the I2C bus [4], [5].
>>>>>>>>
>>>>>>>> As the discussions in [1], [2] stopped w/o a clear conclusion, this
>>>>>>>> patch tries to revive it by proposing a similar approach that was done
>>>>>>>> for the I2C bus.
>>>>>>>>
>>>>>>>> Please let me know you input.    
>>>>>>>
>>>>>>> I'm with Jonathan here, the devres stuff is getting crazy here and you
>>>>>>> have drivers mixing them and side affects happening and lots of
>>>>>>> confusion.  Your change here is only going to make it even more
>>>>>>> confusing, and shouldn't actually solve it for other busses (i.e. what
>>>>>>> about iio devices NOT on the platform bus?)    
>>>>
>>>> In some cases they are already carrying the support as per the link
>>>> above covering all i2c drivers.  I'd like to see a generic solution and
>>>> I suspect pushing it to the device drivers rather than the bus code
>>>> will explode badly and leave us with subtle bugs where people don't
>>>> realise it is necessary. 
>>>>
>>>> https://lore.kernel.org/all/20250224120608.1769039-1-claudiu.beznea.uj@bp.renesas.com/
>>>> is a lot nastier looking than what we have here. I'll review that in a minute
>>>> to show that it need not be that bad, but none the less not pleasant.
>>>>
>>>> +CC linux-iio to join up threads and Dmitry wrt to i2c case (and HID that does
>>>> similar)  
>>>
>>> We should not expect individual drivers handle this, because this is a
>>> layering violation: they need to know implementation details of the bus
>>> code to know if the bus is using non-devres managed resources, and
>>> adjust their behavior. Moving this into driver core is also not
>>> feasible, as not all buses need it. So IMO this should belong to
>>> individual bus code.
>>>
>>> Instead of using devres group a bus may opt to use
>>> devm_add_action_or_reset() and other devm APIs to make sure bus'
>>> resource unwinding is carried in the correct order relative to freeing
>>> driver-owned resources.  
>>
>> Can you please let us know your input on the approach proposed in this
>> patch? Or if you would prefer devm_add_action_or_reset() as suggested by
>> Dmitry? Or if you consider another approach would fit better?
>>
>> Currently there were issues identified with the rzg2l-adc driver (driver
>> based solution proposed in [1]) and with the rzg3s thermal driver (solved
>> by function rzg3s_thermal_probe() from [2]).
>>
>> As expressed previously by Jonathan and Dimitry this is a common problem
>> and as the issue is due to a call in the bus driver, would be better and
>> simpler to handle it in the bus driver. Otherwise, individual drivers would
>> have to be adjusted in a similar way.
>>
> 
> Rafael,
> 
> Greg suggested we ask for your input on the right option:
> 
> https://lore.kernel.org/all/2025032703-genre-excitable-9473@gregkh/
> (that thread has the other option).

Can you please let us know your opinion on this?

Thank you,
Claudiu

> 
> Jonathan
> 
>> Thank you,
>> Claudiu
>>
>> [1]
>> https://lore.kernel.org/all/20250324122627.32336-2-claudiu.beznea.uj@bp.renesas.com/
>> [2]
>> https://lore.kernel.org/all/20250324135701.179827-3-claudiu.beznea.uj@bp.renesas.com/
>>
>>>   
>>>>  
>>>>>>
>>>>>> You're right, other busses will still have this problem.
>>>>>>     
>>>>>>>
>>>>>>> Why can't your individual driver handle this instead?    
>>>>
>>>> In my mind because it's the bus code that is doing the unexpected part by
>>>> making calls in the remove path that are effectively not in the same order
>>>> as probe because they occur between driver remove and related devres cleanup
>>>> for stuff registered in probe.
>>>>  
>>>>>>
>>>>>> Initially I tried it at the driver level by using non-devres PM runtime
>>>>>> enable API but wasn't considered OK by all parties.
>>>>>>
>>>>>> I haven't thought about having devres_open_group()/devres_close_group() in
>>>>>> the driver itself but it should work.    
>>>>>
>>>>> Are you OK with having the devres_open_group()/devres_close_group() in the
>>>>> currently known affected drivers (drivers/iio/adc/rzg2l_adc.c and the
>>>>> proposed drivers/thermal/renesas/rzg3s_thermal.c [1]) ?  
>>>>
>>>> I guess it may be the best of a bunch of not particularly nasty solutions...  
>>>
>>> We need to update _ALL_ platform drivers using devm then, and this is
>>> clearly not scalable.
>>>
>>> Thanks.
>>>   
>>
> 

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Claudiu Beznea]

Hi, Rafael, Ulf, PM list,


On 09.04.2025 19:12, Claudiu Beznea wrote:
> Hi, Rafael,
> 
> On 30.03.2025 18:31, Jonathan Cameron wrote:
>> On Thu, 27 Mar 2025 18:47:53 +0200
>> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>
>>> Hi, Rafael,
>>>
>>> On 06.03.2025 08:11, Dmitry Torokhov wrote:
>>>> On Wed, Mar 05, 2025 at 02:03:09PM +0000, Jonathan Cameron wrote:  
>>>>> On Wed, 19 Feb 2025 14:45:07 +0200
>>>>> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>>>>  
>>>>>> Hi, Daniel, Jonathan,
>>>>>>
>>>>>> On 15.02.2025 15:51, Claudiu Beznea wrote:  
>>>>>>> Hi, Greg,
>>>>>>>
>>>>>>> On 15.02.2025 15:25, Greg KH wrote:    
>>>>>>>> On Sat, Feb 15, 2025 at 03:08:49PM +0200, Claudiu wrote:    
>>>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>>>>
>>>>>>>>> On the Renesas RZ/G3S (and other Renesas SoCs, e.g., RZ/G2{L, LC, UL}),
>>>>>>>>> clocks are managed through PM domains. These PM domains, registered on
>>>>>>>>> behalf of the clock controller driver, are configured with
>>>>>>>>> GENPD_FLAG_PM_CLK. In most of the Renesas drivers used by RZ SoCs, the
>>>>>>>>> clocks are enabled/disabled using runtime PM APIs. The power domains may
>>>>>>>>> also have power_on/power_off support implemented. After the device PM
>>>>>>>>> domain is powered off any CPU accesses to these domains leads to system
>>>>>>>>> aborts.
>>>>>>>>>
>>>>>>>>> During probe, devices are attached to the PM domain controlling their
>>>>>>>>> clocks and power. Similarly, during removal, devices are detached from the
>>>>>>>>> PM domain.
>>>>>>>>>
>>>>>>>>> The detachment call stack is as follows:
>>>>>>>>>
>>>>>>>>> device_driver_detach() ->
>>>>>>>>>   device_release_driver_internal() ->
>>>>>>>>>     __device_release_driver() ->
>>>>>>>>>       device_remove() ->
>>>>>>>>>         platform_remove() ->
>>>>>>>>> 	  dev_pm_domain_detach()
>>>>>>>>>
>>>>>>>>> During driver unbind, after the device is detached from its PM domain,
>>>>>>>>> the device_unbind_cleanup() function is called, which subsequently invokes
>>>>>>>>> devres_release_all(). This function handles devres resource cleanup.
>>>>>>>>>
>>>>>>>>> If runtime PM is enabled in driver probe via devm_pm_runtime_enable(), the
>>>>>>>>> cleanup process triggers the action or reset function for disabling runtime
>>>>>>>>> PM. This function is pm_runtime_disable_action(), which leads to the
>>>>>>>>> following call stack of interest when called:
>>>>>>>>>
>>>>>>>>> pm_runtime_disable_action() ->
>>>>>>>>>   pm_runtime_dont_use_autosuspend() ->
>>>>>>>>>     __pm_runtime_use_autosuspend() ->
>>>>>>>>>       update_autosuspend() ->
>>>>>>>>>         rpm_idle()
>>>>>>>>>
>>>>>>>>> The rpm_idle() function attempts to resume the device at runtime. However,
>>>>>>>>> at the point it is called, the device is no longer part of a PM domain
>>>>>>>>> (which manages clocks and power states). If the driver implements its own
>>>>>>>>> runtime PM APIs for specific functionalities - such as the rzg2l_adc
>>>>>>>>> driver - while also relying on the power domain subsystem for power
>>>>>>>>> management, rpm_idle() will invoke the driver's runtime PM API. However,
>>>>>>>>> since the device is no longer part of a PM domain at this point, the PM
>>>>>>>>> domain's runtime PM APIs will not be called. This leads to system aborts on
>>>>>>>>> Renesas SoCs.
>>>>>>>>>
>>>>>>>>> Another identified case is when a subsystem performs various cleanups
>>>>>>>>> using device_unbind_cleanup(), calling driver-specific APIs in the process.
>>>>>>>>> A known example is the thermal subsystem, which may call driver-specific
>>>>>>>>> APIs to disable the thermal device. The relevant call stack in this case
>>>>>>>>> is:
>>>>>>>>>
>>>>>>>>> device_driver_detach() ->
>>>>>>>>>   device_release_driver_internal() ->
>>>>>>>>>     device_unbind_cleanup() ->
>>>>>>>>>       devres_release_all() ->
>>>>>>>>>         devm_thermal_of_zone_release() ->
>>>>>>>>> 	  thermal_zone_device_disable() ->
>>>>>>>>> 	    thermal_zone_device_set_mode() ->
>>>>>>>>> 	      struct thermal_zone_device_ops::change_mode()
>>>>>>>>>
>>>>>>>>> At the moment the driver-specific change_mode() API is called, the device
>>>>>>>>> is no longer part of its PM domain. Accessing its registers without proper
>>>>>>>>> power management leads to system aborts.
>>>>>>>>>
>>>>>>>>> Open a devres group before calling the driver probe, and close it
>>>>>>>>> immediately after the driver remove function is called and before
>>>>>>>>> dev_pm_domain_detach(). This ensures that driver-specific devm actions or
>>>>>>>>> reset functions are executed immediately after the driver remove function
>>>>>>>>> completes. Additionally, it prevents driver-specific runtime PM APIs from
>>>>>>>>> being called when the device is no longer part of its power domain.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Hi,  
>>>>>
>>>>> Hi Claudiu, Greg,
>>>>>
>>>>> Sorry, I missed this thread whilst travelling and only saw it because
>>>>> of reference from the in driver solution.
>>>>>  
>>>>>>>>>
>>>>>>>>> Although Ulf gave its green light for the approaches on both IIO [1],
>>>>>>>>> [2] and thermal subsystems [3], Jonathan considered unacceptable the
>>>>>>>>> approaches in [1], [2] as he considered it may lead to dificult to
>>>>>>>>> maintain code and code opened to subtle bugs (due to the potential of
>>>>>>>>> mixing devres and non-devres calls). He pointed out a similar approach
>>>>>>>>> that was done for the I2C bus [4], [5].
>>>>>>>>>
>>>>>>>>> As the discussions in [1], [2] stopped w/o a clear conclusion, this
>>>>>>>>> patch tries to revive it by proposing a similar approach that was done
>>>>>>>>> for the I2C bus.
>>>>>>>>>
>>>>>>>>> Please let me know you input.    
>>>>>>>>
>>>>>>>> I'm with Jonathan here, the devres stuff is getting crazy here and you
>>>>>>>> have drivers mixing them and side affects happening and lots of
>>>>>>>> confusion.  Your change here is only going to make it even more
>>>>>>>> confusing, and shouldn't actually solve it for other busses (i.e. what
>>>>>>>> about iio devices NOT on the platform bus?)    
>>>>>
>>>>> In some cases they are already carrying the support as per the link
>>>>> above covering all i2c drivers.  I'd like to see a generic solution and
>>>>> I suspect pushing it to the device drivers rather than the bus code
>>>>> will explode badly and leave us with subtle bugs where people don't
>>>>> realise it is necessary. 
>>>>>
>>>>> https://lore.kernel.org/all/20250224120608.1769039-1-claudiu.beznea.uj@bp.renesas.com/
>>>>> is a lot nastier looking than what we have here. I'll review that in a minute
>>>>> to show that it need not be that bad, but none the less not pleasant.
>>>>>
>>>>> +CC linux-iio to join up threads and Dmitry wrt to i2c case (and HID that does
>>>>> similar)  
>>>>
>>>> We should not expect individual drivers handle this, because this is a
>>>> layering violation: they need to know implementation details of the bus
>>>> code to know if the bus is using non-devres managed resources, and
>>>> adjust their behavior. Moving this into driver core is also not
>>>> feasible, as not all buses need it. So IMO this should belong to
>>>> individual bus code.
>>>>
>>>> Instead of using devres group a bus may opt to use
>>>> devm_add_action_or_reset() and other devm APIs to make sure bus'
>>>> resource unwinding is carried in the correct order relative to freeing
>>>> driver-owned resources.  
>>>
>>> Can you please let us know your input on the approach proposed in this
>>> patch? Or if you would prefer devm_add_action_or_reset() as suggested by
>>> Dmitry? Or if you consider another approach would fit better?
>>>
>>> Currently there were issues identified with the rzg2l-adc driver (driver
>>> based solution proposed in [1]) and with the rzg3s thermal driver (solved
>>> by function rzg3s_thermal_probe() from [2]).
>>>
>>> As expressed previously by Jonathan and Dimitry this is a common problem
>>> and as the issue is due to a call in the bus driver, would be better and
>>> simpler to handle it in the bus driver. Otherwise, individual drivers would
>>> have to be adjusted in a similar way.
>>>
>>
>> Rafael,
>>
>> Greg suggested we ask for your input on the right option:
>>
>> https://lore.kernel.org/all/2025032703-genre-excitable-9473@gregkh/
>> (that thread has the other option).
> 
> Can you please let us know your opinion on this?
Can you please let us know if you have any suggestions for this?

Thank you,
Claudiu

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Ulf Hansson]

On Fri, 9 May 2025 at 13:51, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>
> Hi, Rafael, Ulf, PM list,
>
>
> On 09.04.2025 19:12, Claudiu Beznea wrote:
> > Hi, Rafael,
> >
> > On 30.03.2025 18:31, Jonathan Cameron wrote:
> >> On Thu, 27 Mar 2025 18:47:53 +0200
> >> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >>
> >>> Hi, Rafael,
> >>>
> >>> On 06.03.2025 08:11, Dmitry Torokhov wrote:
> >>>> On Wed, Mar 05, 2025 at 02:03:09PM +0000, Jonathan Cameron wrote:
> >>>>> On Wed, 19 Feb 2025 14:45:07 +0200
> >>>>> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >>>>>
> >>>>>> Hi, Daniel, Jonathan,
> >>>>>>
> >>>>>> On 15.02.2025 15:51, Claudiu Beznea wrote:
> >>>>>>> Hi, Greg,
> >>>>>>>
> >>>>>>> On 15.02.2025 15:25, Greg KH wrote:
> >>>>>>>> On Sat, Feb 15, 2025 at 03:08:49PM +0200, Claudiu wrote:
> >>>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>>>>>>
> >>>>>>>>> On the Renesas RZ/G3S (and other Renesas SoCs, e.g., RZ/G2{L, LC, UL}),
> >>>>>>>>> clocks are managed through PM domains. These PM domains, registered on
> >>>>>>>>> behalf of the clock controller driver, are configured with
> >>>>>>>>> GENPD_FLAG_PM_CLK. In most of the Renesas drivers used by RZ SoCs, the
> >>>>>>>>> clocks are enabled/disabled using runtime PM APIs. The power domains may
> >>>>>>>>> also have power_on/power_off support implemented. After the device PM
> >>>>>>>>> domain is powered off any CPU accesses to these domains leads to system
> >>>>>>>>> aborts.
> >>>>>>>>>
> >>>>>>>>> During probe, devices are attached to the PM domain controlling their
> >>>>>>>>> clocks and power. Similarly, during removal, devices are detached from the
> >>>>>>>>> PM domain.
> >>>>>>>>>
> >>>>>>>>> The detachment call stack is as follows:
> >>>>>>>>>
> >>>>>>>>> device_driver_detach() ->
> >>>>>>>>>   device_release_driver_internal() ->
> >>>>>>>>>     __device_release_driver() ->
> >>>>>>>>>       device_remove() ->
> >>>>>>>>>         platform_remove() ->
> >>>>>>>>>         dev_pm_domain_detach()
> >>>>>>>>>
> >>>>>>>>> During driver unbind, after the device is detached from its PM domain,
> >>>>>>>>> the device_unbind_cleanup() function is called, which subsequently invokes
> >>>>>>>>> devres_release_all(). This function handles devres resource cleanup.
> >>>>>>>>>
> >>>>>>>>> If runtime PM is enabled in driver probe via devm_pm_runtime_enable(), the
> >>>>>>>>> cleanup process triggers the action or reset function for disabling runtime
> >>>>>>>>> PM. This function is pm_runtime_disable_action(), which leads to the
> >>>>>>>>> following call stack of interest when called:
> >>>>>>>>>
> >>>>>>>>> pm_runtime_disable_action() ->
> >>>>>>>>>   pm_runtime_dont_use_autosuspend() ->
> >>>>>>>>>     __pm_runtime_use_autosuspend() ->
> >>>>>>>>>       update_autosuspend() ->
> >>>>>>>>>         rpm_idle()
> >>>>>>>>>
> >>>>>>>>> The rpm_idle() function attempts to resume the device at runtime. However,
> >>>>>>>>> at the point it is called, the device is no longer part of a PM domain
> >>>>>>>>> (which manages clocks and power states). If the driver implements its own
> >>>>>>>>> runtime PM APIs for specific functionalities - such as the rzg2l_adc
> >>>>>>>>> driver - while also relying on the power domain subsystem for power
> >>>>>>>>> management, rpm_idle() will invoke the driver's runtime PM API. However,
> >>>>>>>>> since the device is no longer part of a PM domain at this point, the PM
> >>>>>>>>> domain's runtime PM APIs will not be called. This leads to system aborts on
> >>>>>>>>> Renesas SoCs.
> >>>>>>>>>
> >>>>>>>>> Another identified case is when a subsystem performs various cleanups
> >>>>>>>>> using device_unbind_cleanup(), calling driver-specific APIs in the process.
> >>>>>>>>> A known example is the thermal subsystem, which may call driver-specific
> >>>>>>>>> APIs to disable the thermal device. The relevant call stack in this case
> >>>>>>>>> is:
> >>>>>>>>>
> >>>>>>>>> device_driver_detach() ->
> >>>>>>>>>   device_release_driver_internal() ->
> >>>>>>>>>     device_unbind_cleanup() ->
> >>>>>>>>>       devres_release_all() ->
> >>>>>>>>>         devm_thermal_of_zone_release() ->
> >>>>>>>>>         thermal_zone_device_disable() ->
> >>>>>>>>>           thermal_zone_device_set_mode() ->
> >>>>>>>>>             struct thermal_zone_device_ops::change_mode()
> >>>>>>>>>
> >>>>>>>>> At the moment the driver-specific change_mode() API is called, the device
> >>>>>>>>> is no longer part of its PM domain. Accessing its registers without proper
> >>>>>>>>> power management leads to system aborts.
> >>>>>>>>>
> >>>>>>>>> Open a devres group before calling the driver probe, and close it
> >>>>>>>>> immediately after the driver remove function is called and before
> >>>>>>>>> dev_pm_domain_detach(). This ensures that driver-specific devm actions or
> >>>>>>>>> reset functions are executed immediately after the driver remove function
> >>>>>>>>> completes. Additionally, it prevents driver-specific runtime PM APIs from
> >>>>>>>>> being called when the device is no longer part of its power domain.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>>>>>> ---
> >>>>>>>>>
> >>>>>>>>> Hi,
> >>>>>
> >>>>> Hi Claudiu, Greg,
> >>>>>
> >>>>> Sorry, I missed this thread whilst travelling and only saw it because
> >>>>> of reference from the in driver solution.
> >>>>>
> >>>>>>>>>
> >>>>>>>>> Although Ulf gave its green light for the approaches on both IIO [1],
> >>>>>>>>> [2] and thermal subsystems [3], Jonathan considered unacceptable the
> >>>>>>>>> approaches in [1], [2] as he considered it may lead to dificult to
> >>>>>>>>> maintain code and code opened to subtle bugs (due to the potential of
> >>>>>>>>> mixing devres and non-devres calls). He pointed out a similar approach
> >>>>>>>>> that was done for the I2C bus [4], [5].
> >>>>>>>>>
> >>>>>>>>> As the discussions in [1], [2] stopped w/o a clear conclusion, this
> >>>>>>>>> patch tries to revive it by proposing a similar approach that was done
> >>>>>>>>> for the I2C bus.
> >>>>>>>>>
> >>>>>>>>> Please let me know you input.
> >>>>>>>>
> >>>>>>>> I'm with Jonathan here, the devres stuff is getting crazy here and you
> >>>>>>>> have drivers mixing them and side affects happening and lots of
> >>>>>>>> confusion.  Your change here is only going to make it even more
> >>>>>>>> confusing, and shouldn't actually solve it for other busses (i.e. what
> >>>>>>>> about iio devices NOT on the platform bus?)
> >>>>>
> >>>>> In some cases they are already carrying the support as per the link
> >>>>> above covering all i2c drivers.  I'd like to see a generic solution and
> >>>>> I suspect pushing it to the device drivers rather than the bus code
> >>>>> will explode badly and leave us with subtle bugs where people don't
> >>>>> realise it is necessary.
> >>>>>
> >>>>> https://lore.kernel.org/all/20250224120608.1769039-1-claudiu.beznea.uj@bp.renesas.com/
> >>>>> is a lot nastier looking than what we have here. I'll review that in a minute
> >>>>> to show that it need not be that bad, but none the less not pleasant.
> >>>>>
> >>>>> +CC linux-iio to join up threads and Dmitry wrt to i2c case (and HID that does
> >>>>> similar)
> >>>>
> >>>> We should not expect individual drivers handle this, because this is a
> >>>> layering violation: they need to know implementation details of the bus
> >>>> code to know if the bus is using non-devres managed resources, and
> >>>> adjust their behavior. Moving this into driver core is also not
> >>>> feasible, as not all buses need it. So IMO this should belong to
> >>>> individual bus code.
> >>>>
> >>>> Instead of using devres group a bus may opt to use
> >>>> devm_add_action_or_reset() and other devm APIs to make sure bus'
> >>>> resource unwinding is carried in the correct order relative to freeing
> >>>> driver-owned resources.
> >>>
> >>> Can you please let us know your input on the approach proposed in this
> >>> patch? Or if you would prefer devm_add_action_or_reset() as suggested by
> >>> Dmitry? Or if you consider another approach would fit better?
> >>>
> >>> Currently there were issues identified with the rzg2l-adc driver (driver
> >>> based solution proposed in [1]) and with the rzg3s thermal driver (solved
> >>> by function rzg3s_thermal_probe() from [2]).
> >>>
> >>> As expressed previously by Jonathan and Dimitry this is a common problem
> >>> and as the issue is due to a call in the bus driver, would be better and
> >>> simpler to handle it in the bus driver. Otherwise, individual drivers would
> >>> have to be adjusted in a similar way.
> >>>
> >>
> >> Rafael,
> >>
> >> Greg suggested we ask for your input on the right option:
> >>
> >> https://lore.kernel.org/all/2025032703-genre-excitable-9473@gregkh/
> >> (that thread has the other option).
> >
> > Can you please let us know your opinion on this?
> Can you please let us know if you have any suggestions for this?

It's been a while since I looked at this. Although as I understand it,
the main issue comes from using devm_pm_runtime_enable().

As I have tried to argue before, I think devm_pm_runtime_enable()
should *not* be used. Not here, not at all. Runtime PM isn't like any
other resources that we fetch/release. Instead, it's a behaviour that
you turn on and off, which needs to be managed more carefully, rather
than relying on fetch/release ordering from devres.

That said, I would convert the driver to use pm_runtime_enable() and
pm_runtime_disable() instead.

Kind regards
Uffe

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Claudiu Beznea]

Hi, Ulf,

Thank you for your input!

On 09.05.2025 16:07, Ulf Hansson wrote:
> On Fri, 9 May 2025 at 13:51, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>
>> Hi, Rafael, Ulf, PM list,
>>
>>
>> On 09.04.2025 19:12, Claudiu Beznea wrote:
>>> Hi, Rafael,
>>>
>>> On 30.03.2025 18:31, Jonathan Cameron wrote:
>>>> On Thu, 27 Mar 2025 18:47:53 +0200
>>>> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>>>
>>>>> Hi, Rafael,
>>>>>
>>>>> On 06.03.2025 08:11, Dmitry Torokhov wrote:
>>>>>> On Wed, Mar 05, 2025 at 02:03:09PM +0000, Jonathan Cameron wrote:
>>>>>>> On Wed, 19 Feb 2025 14:45:07 +0200
>>>>>>> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>>>>>>
>>>>>>>> Hi, Daniel, Jonathan,
>>>>>>>>
>>>>>>>> On 15.02.2025 15:51, Claudiu Beznea wrote:
>>>>>>>>> Hi, Greg,
>>>>>>>>>
>>>>>>>>> On 15.02.2025 15:25, Greg KH wrote:
>>>>>>>>>> On Sat, Feb 15, 2025 at 03:08:49PM +0200, Claudiu wrote:
>>>>>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>>>>>>
>>>>>>>>>>> On the Renesas RZ/G3S (and other Renesas SoCs, e.g., RZ/G2{L, LC, UL}),
>>>>>>>>>>> clocks are managed through PM domains. These PM domains, registered on
>>>>>>>>>>> behalf of the clock controller driver, are configured with
>>>>>>>>>>> GENPD_FLAG_PM_CLK. In most of the Renesas drivers used by RZ SoCs, the
>>>>>>>>>>> clocks are enabled/disabled using runtime PM APIs. The power domains may
>>>>>>>>>>> also have power_on/power_off support implemented. After the device PM
>>>>>>>>>>> domain is powered off any CPU accesses to these domains leads to system
>>>>>>>>>>> aborts.
>>>>>>>>>>>
>>>>>>>>>>> During probe, devices are attached to the PM domain controlling their
>>>>>>>>>>> clocks and power. Similarly, during removal, devices are detached from the
>>>>>>>>>>> PM domain.
>>>>>>>>>>>
>>>>>>>>>>> The detachment call stack is as follows:
>>>>>>>>>>>
>>>>>>>>>>> device_driver_detach() ->
>>>>>>>>>>>   device_release_driver_internal() ->
>>>>>>>>>>>     __device_release_driver() ->
>>>>>>>>>>>       device_remove() ->
>>>>>>>>>>>         platform_remove() ->
>>>>>>>>>>>         dev_pm_domain_detach()
>>>>>>>>>>>
>>>>>>>>>>> During driver unbind, after the device is detached from its PM domain,
>>>>>>>>>>> the device_unbind_cleanup() function is called, which subsequently invokes
>>>>>>>>>>> devres_release_all(). This function handles devres resource cleanup.
>>>>>>>>>>>
>>>>>>>>>>> If runtime PM is enabled in driver probe via devm_pm_runtime_enable(), the
>>>>>>>>>>> cleanup process triggers the action or reset function for disabling runtime
>>>>>>>>>>> PM. This function is pm_runtime_disable_action(), which leads to the
>>>>>>>>>>> following call stack of interest when called:
>>>>>>>>>>>
>>>>>>>>>>> pm_runtime_disable_action() ->
>>>>>>>>>>>   pm_runtime_dont_use_autosuspend() ->
>>>>>>>>>>>     __pm_runtime_use_autosuspend() ->
>>>>>>>>>>>       update_autosuspend() ->
>>>>>>>>>>>         rpm_idle()
>>>>>>>>>>>
>>>>>>>>>>> The rpm_idle() function attempts to resume the device at runtime. However,
>>>>>>>>>>> at the point it is called, the device is no longer part of a PM domain
>>>>>>>>>>> (which manages clocks and power states). If the driver implements its own
>>>>>>>>>>> runtime PM APIs for specific functionalities - such as the rzg2l_adc
>>>>>>>>>>> driver - while also relying on the power domain subsystem for power
>>>>>>>>>>> management, rpm_idle() will invoke the driver's runtime PM API. However,
>>>>>>>>>>> since the device is no longer part of a PM domain at this point, the PM
>>>>>>>>>>> domain's runtime PM APIs will not be called. This leads to system aborts on
>>>>>>>>>>> Renesas SoCs.
>>>>>>>>>>>
>>>>>>>>>>> Another identified case is when a subsystem performs various cleanups
>>>>>>>>>>> using device_unbind_cleanup(), calling driver-specific APIs in the process.
>>>>>>>>>>> A known example is the thermal subsystem, which may call driver-specific
>>>>>>>>>>> APIs to disable the thermal device. The relevant call stack in this case
>>>>>>>>>>> is:
>>>>>>>>>>>
>>>>>>>>>>> device_driver_detach() ->
>>>>>>>>>>>   device_release_driver_internal() ->
>>>>>>>>>>>     device_unbind_cleanup() ->
>>>>>>>>>>>       devres_release_all() ->
>>>>>>>>>>>         devm_thermal_of_zone_release() ->
>>>>>>>>>>>         thermal_zone_device_disable() ->
>>>>>>>>>>>           thermal_zone_device_set_mode() ->
>>>>>>>>>>>             struct thermal_zone_device_ops::change_mode()
>>>>>>>>>>>
>>>>>>>>>>> At the moment the driver-specific change_mode() API is called, the device
>>>>>>>>>>> is no longer part of its PM domain. Accessing its registers without proper
>>>>>>>>>>> power management leads to system aborts.
>>>>>>>>>>>
>>>>>>>>>>> Open a devres group before calling the driver probe, and close it
>>>>>>>>>>> immediately after the driver remove function is called and before
>>>>>>>>>>> dev_pm_domain_detach(). This ensures that driver-specific devm actions or
>>>>>>>>>>> reset functions are executed immediately after the driver remove function
>>>>>>>>>>> completes. Additionally, it prevents driver-specific runtime PM APIs from
>>>>>>>>>>> being called when the device is no longer part of its power domain.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>> Hi,
>>>>>>>
>>>>>>> Hi Claudiu, Greg,
>>>>>>>
>>>>>>> Sorry, I missed this thread whilst travelling and only saw it because
>>>>>>> of reference from the in driver solution.
>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Although Ulf gave its green light for the approaches on both IIO [1],
>>>>>>>>>>> [2] and thermal subsystems [3], Jonathan considered unacceptable the
>>>>>>>>>>> approaches in [1], [2] as he considered it may lead to dificult to
>>>>>>>>>>> maintain code and code opened to subtle bugs (due to the potential of
>>>>>>>>>>> mixing devres and non-devres calls). He pointed out a similar approach
>>>>>>>>>>> that was done for the I2C bus [4], [5].
>>>>>>>>>>>
>>>>>>>>>>> As the discussions in [1], [2] stopped w/o a clear conclusion, this
>>>>>>>>>>> patch tries to revive it by proposing a similar approach that was done
>>>>>>>>>>> for the I2C bus.
>>>>>>>>>>>
>>>>>>>>>>> Please let me know you input.
>>>>>>>>>>
>>>>>>>>>> I'm with Jonathan here, the devres stuff is getting crazy here and you
>>>>>>>>>> have drivers mixing them and side affects happening and lots of
>>>>>>>>>> confusion.  Your change here is only going to make it even more
>>>>>>>>>> confusing, and shouldn't actually solve it for other busses (i.e. what
>>>>>>>>>> about iio devices NOT on the platform bus?)
>>>>>>>
>>>>>>> In some cases they are already carrying the support as per the link
>>>>>>> above covering all i2c drivers.  I'd like to see a generic solution and
>>>>>>> I suspect pushing it to the device drivers rather than the bus code
>>>>>>> will explode badly and leave us with subtle bugs where people don't
>>>>>>> realise it is necessary.
>>>>>>>
>>>>>>> https://lore.kernel.org/all/20250224120608.1769039-1-claudiu.beznea.uj@bp.renesas.com/
>>>>>>> is a lot nastier looking than what we have here. I'll review that in a minute
>>>>>>> to show that it need not be that bad, but none the less not pleasant.
>>>>>>>
>>>>>>> +CC linux-iio to join up threads and Dmitry wrt to i2c case (and HID that does
>>>>>>> similar)
>>>>>>
>>>>>> We should not expect individual drivers handle this, because this is a
>>>>>> layering violation: they need to know implementation details of the bus
>>>>>> code to know if the bus is using non-devres managed resources, and
>>>>>> adjust their behavior. Moving this into driver core is also not
>>>>>> feasible, as not all buses need it. So IMO this should belong to
>>>>>> individual bus code.
>>>>>>
>>>>>> Instead of using devres group a bus may opt to use
>>>>>> devm_add_action_or_reset() and other devm APIs to make sure bus'
>>>>>> resource unwinding is carried in the correct order relative to freeing
>>>>>> driver-owned resources.
>>>>>
>>>>> Can you please let us know your input on the approach proposed in this
>>>>> patch? Or if you would prefer devm_add_action_or_reset() as suggested by
>>>>> Dmitry? Or if you consider another approach would fit better?
>>>>>
>>>>> Currently there were issues identified with the rzg2l-adc driver (driver
>>>>> based solution proposed in [1]) and with the rzg3s thermal driver (solved
>>>>> by function rzg3s_thermal_probe() from [2]).
>>>>>
>>>>> As expressed previously by Jonathan and Dimitry this is a common problem
>>>>> and as the issue is due to a call in the bus driver, would be better and
>>>>> simpler to handle it in the bus driver. Otherwise, individual drivers would
>>>>> have to be adjusted in a similar way.
>>>>>
>>>>
>>>> Rafael,
>>>>
>>>> Greg suggested we ask for your input on the right option:
>>>>
>>>> https://lore.kernel.org/all/2025032703-genre-excitable-9473@gregkh/
>>>> (that thread has the other option).
>>>
>>> Can you please let us know your opinion on this?
>> Can you please let us know if you have any suggestions for this?
> 
> It's been a while since I looked at this. Although as I understand it,
> the main issue comes from using devm_pm_runtime_enable().

Yes, it comes from the usage of devm_pm_runtime_enable() in drivers and the
dev_pm_domain_detach() call in platform_remove() right after calling
driver's remove function.

On the platform I experienced issues with, the dev_pm_domain_detach() drops
the clocks from the device power domain and any subsequent PM runtime
resume calls (that may happen in the devres cleanup phase) have no effect
on enabling the clocks. If driver has functions registered (e.g. through
devm_add_action_or_reset()), or driver specific runtime PM functions that
access directly registers in the devres cleanup phase this leads to system
aborts.


> 
> As I have tried to argue before, I think devm_pm_runtime_enable()
> should *not* be used. Not here, not at all. Runtime PM isn't like any
> other resources that we fetch/release. Instead, it's a behaviour that
> you turn on and off, which needs to be managed more carefully, rather
> than relying on fetch/release ordering from devres.
> 
> That said, I would convert the driver to use pm_runtime_enable() and
> pm_runtime_disable() instead.

I've tried this approach previously but it resulted in more complicated
code and thus, Jonathan wasn't happy with it [1].

Another approach I've tried was to have devres group opened/closed in the
driver itself [2], [3] but it was postponed as this approach may have a chance.

At the moment I have 2 drivers waiting for a resolution on this [2], [3]
and I recently posted a new one [4] that uses driver specific local devres
group to avoid this issue.


Thank you,
Claudiu

[1]
https://lore.kernel.org/all/20250224120608.1769039-2-claudiu.beznea.uj@bp.renesas.com
[2]
https://lore.kernel.org/all/20250324122627.32336-2-claudiu.beznea.uj@bp.renesas.com
[3]
https://lore.kernel.org/all/20250324135701.179827-3-claudiu.beznea.uj@bp.renesas.com/
[4]
https://lore.kernel.org/all/20250430103236.3511989-6-claudiu.beznea.uj@bp.renesas.com

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Jonathan Cameron]

Rafael,

Sorry for top posting but this issue needs your input.


On Fri, 9 May 2025 17:12:19 +0300
Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:

> Hi, Ulf,
> 
> Thank you for your input!
> 
> On 09.05.2025 16:07, Ulf Hansson wrote:
> > On Fri, 9 May 2025 at 13:51, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:  
> >>
> >> Hi, Rafael, Ulf, PM list,
> >>
> >>
> >> On 09.04.2025 19:12, Claudiu Beznea wrote:  
> >>> Hi, Rafael,
> >>>
> >>> On 30.03.2025 18:31, Jonathan Cameron wrote:  
> >>>> On Thu, 27 Mar 2025 18:47:53 +0200
> >>>> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >>>>  
> >>>>> Hi, Rafael,
> >>>>>
> >>>>> On 06.03.2025 08:11, Dmitry Torokhov wrote:  
> >>>>>> On Wed, Mar 05, 2025 at 02:03:09PM +0000, Jonathan Cameron wrote:  
> >>>>>>> On Wed, 19 Feb 2025 14:45:07 +0200
> >>>>>>> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >>>>>>>  
> >>>>>>>> Hi, Daniel, Jonathan,
> >>>>>>>>
> >>>>>>>> On 15.02.2025 15:51, Claudiu Beznea wrote:  
> >>>>>>>>> Hi, Greg,
> >>>>>>>>>
> >>>>>>>>> On 15.02.2025 15:25, Greg KH wrote:  
> >>>>>>>>>> On Sat, Feb 15, 2025 at 03:08:49PM +0200, Claudiu wrote:  
> >>>>>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>>>>>>>>
> >>>>>>>>>>> On the Renesas RZ/G3S (and other Renesas SoCs, e.g., RZ/G2{L, LC, UL}),
> >>>>>>>>>>> clocks are managed through PM domains. These PM domains, registered on
> >>>>>>>>>>> behalf of the clock controller driver, are configured with
> >>>>>>>>>>> GENPD_FLAG_PM_CLK. In most of the Renesas drivers used by RZ SoCs, the
> >>>>>>>>>>> clocks are enabled/disabled using runtime PM APIs. The power domains may
> >>>>>>>>>>> also have power_on/power_off support implemented. After the device PM
> >>>>>>>>>>> domain is powered off any CPU accesses to these domains leads to system
> >>>>>>>>>>> aborts.
> >>>>>>>>>>>
> >>>>>>>>>>> During probe, devices are attached to the PM domain controlling their
> >>>>>>>>>>> clocks and power. Similarly, during removal, devices are detached from the
> >>>>>>>>>>> PM domain.
> >>>>>>>>>>>
> >>>>>>>>>>> The detachment call stack is as follows:
> >>>>>>>>>>>
> >>>>>>>>>>> device_driver_detach() ->
> >>>>>>>>>>>   device_release_driver_internal() ->
> >>>>>>>>>>>     __device_release_driver() ->
> >>>>>>>>>>>       device_remove() ->
> >>>>>>>>>>>         platform_remove() ->
> >>>>>>>>>>>         dev_pm_domain_detach()
> >>>>>>>>>>>
> >>>>>>>>>>> During driver unbind, after the device is detached from its PM domain,
> >>>>>>>>>>> the device_unbind_cleanup() function is called, which subsequently invokes
> >>>>>>>>>>> devres_release_all(). This function handles devres resource cleanup.
> >>>>>>>>>>>
> >>>>>>>>>>> If runtime PM is enabled in driver probe via devm_pm_runtime_enable(), the
> >>>>>>>>>>> cleanup process triggers the action or reset function for disabling runtime
> >>>>>>>>>>> PM. This function is pm_runtime_disable_action(), which leads to the
> >>>>>>>>>>> following call stack of interest when called:
> >>>>>>>>>>>
> >>>>>>>>>>> pm_runtime_disable_action() ->
> >>>>>>>>>>>   pm_runtime_dont_use_autosuspend() ->
> >>>>>>>>>>>     __pm_runtime_use_autosuspend() ->
> >>>>>>>>>>>       update_autosuspend() ->
> >>>>>>>>>>>         rpm_idle()
> >>>>>>>>>>>
> >>>>>>>>>>> The rpm_idle() function attempts to resume the device at runtime. However,
> >>>>>>>>>>> at the point it is called, the device is no longer part of a PM domain
> >>>>>>>>>>> (which manages clocks and power states). If the driver implements its own
> >>>>>>>>>>> runtime PM APIs for specific functionalities - such as the rzg2l_adc
> >>>>>>>>>>> driver - while also relying on the power domain subsystem for power
> >>>>>>>>>>> management, rpm_idle() will invoke the driver's runtime PM API. However,
> >>>>>>>>>>> since the device is no longer part of a PM domain at this point, the PM
> >>>>>>>>>>> domain's runtime PM APIs will not be called. This leads to system aborts on
> >>>>>>>>>>> Renesas SoCs.
> >>>>>>>>>>>
> >>>>>>>>>>> Another identified case is when a subsystem performs various cleanups
> >>>>>>>>>>> using device_unbind_cleanup(), calling driver-specific APIs in the process.
> >>>>>>>>>>> A known example is the thermal subsystem, which may call driver-specific
> >>>>>>>>>>> APIs to disable the thermal device. The relevant call stack in this case
> >>>>>>>>>>> is:
> >>>>>>>>>>>
> >>>>>>>>>>> device_driver_detach() ->
> >>>>>>>>>>>   device_release_driver_internal() ->
> >>>>>>>>>>>     device_unbind_cleanup() ->
> >>>>>>>>>>>       devres_release_all() ->
> >>>>>>>>>>>         devm_thermal_of_zone_release() ->
> >>>>>>>>>>>         thermal_zone_device_disable() ->
> >>>>>>>>>>>           thermal_zone_device_set_mode() ->
> >>>>>>>>>>>             struct thermal_zone_device_ops::change_mode()
> >>>>>>>>>>>
> >>>>>>>>>>> At the moment the driver-specific change_mode() API is called, the device
> >>>>>>>>>>> is no longer part of its PM domain. Accessing its registers without proper
> >>>>>>>>>>> power management leads to system aborts.
> >>>>>>>>>>>
> >>>>>>>>>>> Open a devres group before calling the driver probe, and close it
> >>>>>>>>>>> immediately after the driver remove function is called and before
> >>>>>>>>>>> dev_pm_domain_detach(). This ensures that driver-specific devm actions or
> >>>>>>>>>>> reset functions are executed immediately after the driver remove function
> >>>>>>>>>>> completes. Additionally, it prevents driver-specific runtime PM APIs from
> >>>>>>>>>>> being called when the device is no longer part of its power domain.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>>
> >>>>>>>>>>> Hi,  
> >>>>>>>
> >>>>>>> Hi Claudiu, Greg,
> >>>>>>>
> >>>>>>> Sorry, I missed this thread whilst travelling and only saw it because
> >>>>>>> of reference from the in driver solution.
> >>>>>>>  
> >>>>>>>>>>>
> >>>>>>>>>>> Although Ulf gave its green light for the approaches on both IIO [1],
> >>>>>>>>>>> [2] and thermal subsystems [3], Jonathan considered unacceptable the
> >>>>>>>>>>> approaches in [1], [2] as he considered it may lead to dificult to
> >>>>>>>>>>> maintain code and code opened to subtle bugs (due to the potential of
> >>>>>>>>>>> mixing devres and non-devres calls). He pointed out a similar approach
> >>>>>>>>>>> that was done for the I2C bus [4], [5].
> >>>>>>>>>>>
> >>>>>>>>>>> As the discussions in [1], [2] stopped w/o a clear conclusion, this
> >>>>>>>>>>> patch tries to revive it by proposing a similar approach that was done
> >>>>>>>>>>> for the I2C bus.
> >>>>>>>>>>>
> >>>>>>>>>>> Please let me know you input.  
> >>>>>>>>>>
> >>>>>>>>>> I'm with Jonathan here, the devres stuff is getting crazy here and you
> >>>>>>>>>> have drivers mixing them and side affects happening and lots of
> >>>>>>>>>> confusion.  Your change here is only going to make it even more
> >>>>>>>>>> confusing, and shouldn't actually solve it for other busses (i.e. what
> >>>>>>>>>> about iio devices NOT on the platform bus?)  
> >>>>>>>
> >>>>>>> In some cases they are already carrying the support as per the link
> >>>>>>> above covering all i2c drivers.  I'd like to see a generic solution and
> >>>>>>> I suspect pushing it to the device drivers rather than the bus code
> >>>>>>> will explode badly and leave us with subtle bugs where people don't
> >>>>>>> realise it is necessary.
> >>>>>>>
> >>>>>>> https://lore.kernel.org/all/20250224120608.1769039-1-claudiu.beznea.uj@bp.renesas.com/
> >>>>>>> is a lot nastier looking than what we have here. I'll review that in a minute
> >>>>>>> to show that it need not be that bad, but none the less not pleasant.
> >>>>>>>
> >>>>>>> +CC linux-iio to join up threads and Dmitry wrt to i2c case (and HID that does
> >>>>>>> similar)  
> >>>>>>
> >>>>>> We should not expect individual drivers handle this, because this is a
> >>>>>> layering violation: they need to know implementation details of the bus
> >>>>>> code to know if the bus is using non-devres managed resources, and
> >>>>>> adjust their behavior. Moving this into driver core is also not
> >>>>>> feasible, as not all buses need it. So IMO this should belong to
> >>>>>> individual bus code.
> >>>>>>
> >>>>>> Instead of using devres group a bus may opt to use
> >>>>>> devm_add_action_or_reset() and other devm APIs to make sure bus'
> >>>>>> resource unwinding is carried in the correct order relative to freeing
> >>>>>> driver-owned resources.  
> >>>>>
> >>>>> Can you please let us know your input on the approach proposed in this
> >>>>> patch? Or if you would prefer devm_add_action_or_reset() as suggested by
> >>>>> Dmitry? Or if you consider another approach would fit better?
> >>>>>
> >>>>> Currently there were issues identified with the rzg2l-adc driver (driver
> >>>>> based solution proposed in [1]) and with the rzg3s thermal driver (solved
> >>>>> by function rzg3s_thermal_probe() from [2]).
> >>>>>
> >>>>> As expressed previously by Jonathan and Dimitry this is a common problem
> >>>>> and as the issue is due to a call in the bus driver, would be better and
> >>>>> simpler to handle it in the bus driver. Otherwise, individual drivers would
> >>>>> have to be adjusted in a similar way.
> >>>>>  
> >>>>
> >>>> Rafael,
> >>>>
> >>>> Greg suggested we ask for your input on the right option:
> >>>>
> >>>> https://lore.kernel.org/all/2025032703-genre-excitable-9473@gregkh/
> >>>> (that thread has the other option).  
> >>>
> >>> Can you please let us know your opinion on this?  
> >> Can you please let us know if you have any suggestions for this?  
> > 
> > It's been a while since I looked at this. Although as I understand it,
> > the main issue comes from using devm_pm_runtime_enable().  
> 
> Yes, it comes from the usage of devm_pm_runtime_enable() in drivers and the
> dev_pm_domain_detach() call in platform_remove() right after calling
> driver's remove function.
> 
> On the platform I experienced issues with, the dev_pm_domain_detach() drops
> the clocks from the device power domain and any subsequent PM runtime
> resume calls (that may happen in the devres cleanup phase) have no effect
> on enabling the clocks. If driver has functions registered (e.g. through
> devm_add_action_or_reset()), or driver specific runtime PM functions that
> access directly registers in the devres cleanup phase this leads to system
> aborts.
> 
> 
> > 
> > As I have tried to argue before, I think devm_pm_runtime_enable()
> > should *not* be used. Not here, not at all. Runtime PM isn't like any
> > other resources that we fetch/release. Instead, it's a behaviour that
> > you turn on and off, which needs to be managed more carefully, rather
> > than relying on fetch/release ordering from devres.
> > 
> > That said, I would convert the driver to use pm_runtime_enable() and
> > pm_runtime_disable() instead.  
> 
> I've tried this approach previously but it resulted in more complicated
> code and thus, Jonathan wasn't happy with it [1].

Yes. I (and others as per the various threads) fundamentally disagree that
runtime pm is special (in this aspect - it's special in many other good
ways).

In the common, simple use case for runtime PM at the point or probe and
remove it is exactly like any other resource that we enable then disable.
What happens between those stages isn't particularly relevant here. We
have other resources also go through state changes between probe and
remove and those sequences are easy to manage.  Where more complex
management is necessary then indeed not using this call is of course the
way to go. Those just aren't the common cases I see.

Removing this call adds considerable potential code complexity to many
drivers which may be trivially solved through the approaches that have
been suggested and have been in use for some time on several bus types.

That code complexity has nothing to with this call at all but to do with
needing to drop lots of other devm management because of the potential
for races that any mix and match of devm and other entails.

I'd approximate that every time we stop using devm_pm_runtime_enable()
we add 10-20 lines of code that is just there because that call can't
be used. That code will come with a review burden and related bugs and
potentially applies in 100-1000s of drivers.

Fundamentally we need a clear statement on which way to go from Rafael
and either to proceed with a common solution across all buses that encounter
this issue, or rip out the existing uses in other buses. (or a 3rd way?)
Mix and match depending on bus type is not a sensible way to proceed.

> 
> Another approach I've tried was to have devres group opened/closed in the
> driver itself [2], [3] but it was postponed as this approach may have a chance.

The in driver approach is unfortunately a sticking plaster, working around
it in each driver.  Maybe that's the best we can do.  Pushing the burden
of infrastructure a driver can't see (because is in the bus layer) down
to drivers is messy at best and most like a source of future bugs.
Removing a specific devm_ helper doesn't stop people rolling their own with
no awareness of this weird subtlety.

More fundamentally I don't personally think a bus driver should ever
be doing things that in it's own probe and remove that can result in races
wrt to devm handling in the drivers.  We should not be making driver writers
jobs hard.  The solutions Dmitry used for other buses resolve that in a more
general fashion, at the point where we know if it is necessary (next to
the bus driver calls that are at issue) so to me look like a good way to go.
(this is one of the approaches Claudiu has posted for platform devices).

Jonathan


> 
> At the moment I have 2 drivers waiting for a resolution on this [2], [3]
> and I recently posted a new one [4] that uses driver specific local devres
> group to avoid this issue.
> 
> 
> Thank you,
> Claudiu
> 
> [1]
> https://lore.kernel.org/all/20250224120608.1769039-2-claudiu.beznea.uj@bp.renesas.com
> [2]
> https://lore.kernel.org/all/20250324122627.32336-2-claudiu.beznea.uj@bp.renesas.com
> [3]
> https://lore.kernel.org/all/20250324135701.179827-3-claudiu.beznea.uj@bp.renesas.com/
> [4]
> https://lore.kernel.org/all/20250430103236.3511989-6-claudiu.beznea.uj@bp.renesas.com

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Dmitry Torokhov]

On Sun, May 11, 2025 at 12:01:57PM +0100, Jonathan Cameron wrote:
> Rafael,
> 
> Sorry for top posting but this issue needs your input.
> 
> 
> On Fri, 9 May 2025 17:12:19 +0300
> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> 
> > Hi, Ulf,
> > 
> > Thank you for your input!
> > 
> > On 09.05.2025 16:07, Ulf Hansson wrote:
> > > On Fri, 9 May 2025 at 13:51, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:  
> > >>
> > >> Hi, Rafael, Ulf, PM list,
> > >>
> > >>
> > >> On 09.04.2025 19:12, Claudiu Beznea wrote:  
> > >>> Hi, Rafael,
> > >>>
> > >>> On 30.03.2025 18:31, Jonathan Cameron wrote:  
> > >>>> On Thu, 27 Mar 2025 18:47:53 +0200
> > >>>> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > >>>>  
> > >>>>> Hi, Rafael,
> > >>>>>
> > >>>>> On 06.03.2025 08:11, Dmitry Torokhov wrote:  
> > >>>>>> On Wed, Mar 05, 2025 at 02:03:09PM +0000, Jonathan Cameron wrote:  
> > >>>>>>> On Wed, 19 Feb 2025 14:45:07 +0200
> > >>>>>>> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > >>>>>>>  
> > >>>>>>>> Hi, Daniel, Jonathan,
> > >>>>>>>>
> > >>>>>>>> On 15.02.2025 15:51, Claudiu Beznea wrote:  
> > >>>>>>>>> Hi, Greg,
> > >>>>>>>>>
> > >>>>>>>>> On 15.02.2025 15:25, Greg KH wrote:  
> > >>>>>>>>>> On Sat, Feb 15, 2025 at 03:08:49PM +0200, Claudiu wrote:  
> > >>>>>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > >>>>>>>>>>>
> > >>>>>>>>>>> On the Renesas RZ/G3S (and other Renesas SoCs, e.g., RZ/G2{L, LC, UL}),
> > >>>>>>>>>>> clocks are managed through PM domains. These PM domains, registered on
> > >>>>>>>>>>> behalf of the clock controller driver, are configured with
> > >>>>>>>>>>> GENPD_FLAG_PM_CLK. In most of the Renesas drivers used by RZ SoCs, the
> > >>>>>>>>>>> clocks are enabled/disabled using runtime PM APIs. The power domains may
> > >>>>>>>>>>> also have power_on/power_off support implemented. After the device PM
> > >>>>>>>>>>> domain is powered off any CPU accesses to these domains leads to system
> > >>>>>>>>>>> aborts.
> > >>>>>>>>>>>
> > >>>>>>>>>>> During probe, devices are attached to the PM domain controlling their
> > >>>>>>>>>>> clocks and power. Similarly, during removal, devices are detached from the
> > >>>>>>>>>>> PM domain.
> > >>>>>>>>>>>
> > >>>>>>>>>>> The detachment call stack is as follows:
> > >>>>>>>>>>>
> > >>>>>>>>>>> device_driver_detach() ->
> > >>>>>>>>>>>   device_release_driver_internal() ->
> > >>>>>>>>>>>     __device_release_driver() ->
> > >>>>>>>>>>>       device_remove() ->
> > >>>>>>>>>>>         platform_remove() ->
> > >>>>>>>>>>>         dev_pm_domain_detach()
> > >>>>>>>>>>>
> > >>>>>>>>>>> During driver unbind, after the device is detached from its PM domain,
> > >>>>>>>>>>> the device_unbind_cleanup() function is called, which subsequently invokes
> > >>>>>>>>>>> devres_release_all(). This function handles devres resource cleanup.
> > >>>>>>>>>>>
> > >>>>>>>>>>> If runtime PM is enabled in driver probe via devm_pm_runtime_enable(), the
> > >>>>>>>>>>> cleanup process triggers the action or reset function for disabling runtime
> > >>>>>>>>>>> PM. This function is pm_runtime_disable_action(), which leads to the
> > >>>>>>>>>>> following call stack of interest when called:
> > >>>>>>>>>>>
> > >>>>>>>>>>> pm_runtime_disable_action() ->
> > >>>>>>>>>>>   pm_runtime_dont_use_autosuspend() ->
> > >>>>>>>>>>>     __pm_runtime_use_autosuspend() ->
> > >>>>>>>>>>>       update_autosuspend() ->
> > >>>>>>>>>>>         rpm_idle()
> > >>>>>>>>>>>
> > >>>>>>>>>>> The rpm_idle() function attempts to resume the device at runtime. However,
> > >>>>>>>>>>> at the point it is called, the device is no longer part of a PM domain
> > >>>>>>>>>>> (which manages clocks and power states). If the driver implements its own
> > >>>>>>>>>>> runtime PM APIs for specific functionalities - such as the rzg2l_adc
> > >>>>>>>>>>> driver - while also relying on the power domain subsystem for power
> > >>>>>>>>>>> management, rpm_idle() will invoke the driver's runtime PM API. However,
> > >>>>>>>>>>> since the device is no longer part of a PM domain at this point, the PM
> > >>>>>>>>>>> domain's runtime PM APIs will not be called. This leads to system aborts on
> > >>>>>>>>>>> Renesas SoCs.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Another identified case is when a subsystem performs various cleanups
> > >>>>>>>>>>> using device_unbind_cleanup(), calling driver-specific APIs in the process.
> > >>>>>>>>>>> A known example is the thermal subsystem, which may call driver-specific
> > >>>>>>>>>>> APIs to disable the thermal device. The relevant call stack in this case
> > >>>>>>>>>>> is:
> > >>>>>>>>>>>
> > >>>>>>>>>>> device_driver_detach() ->
> > >>>>>>>>>>>   device_release_driver_internal() ->
> > >>>>>>>>>>>     device_unbind_cleanup() ->
> > >>>>>>>>>>>       devres_release_all() ->
> > >>>>>>>>>>>         devm_thermal_of_zone_release() ->
> > >>>>>>>>>>>         thermal_zone_device_disable() ->
> > >>>>>>>>>>>           thermal_zone_device_set_mode() ->
> > >>>>>>>>>>>             struct thermal_zone_device_ops::change_mode()
> > >>>>>>>>>>>
> > >>>>>>>>>>> At the moment the driver-specific change_mode() API is called, the device
> > >>>>>>>>>>> is no longer part of its PM domain. Accessing its registers without proper
> > >>>>>>>>>>> power management leads to system aborts.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Open a devres group before calling the driver probe, and close it
> > >>>>>>>>>>> immediately after the driver remove function is called and before
> > >>>>>>>>>>> dev_pm_domain_detach(). This ensures that driver-specific devm actions or
> > >>>>>>>>>>> reset functions are executed immediately after the driver remove function
> > >>>>>>>>>>> completes. Additionally, it prevents driver-specific runtime PM APIs from
> > >>>>>>>>>>> being called when the device is no longer part of its power domain.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > >>>>>>>>>>> ---
> > >>>>>>>>>>>
> > >>>>>>>>>>> Hi,  
> > >>>>>>>
> > >>>>>>> Hi Claudiu, Greg,
> > >>>>>>>
> > >>>>>>> Sorry, I missed this thread whilst travelling and only saw it because
> > >>>>>>> of reference from the in driver solution.
> > >>>>>>>  
> > >>>>>>>>>>>
> > >>>>>>>>>>> Although Ulf gave its green light for the approaches on both IIO [1],
> > >>>>>>>>>>> [2] and thermal subsystems [3], Jonathan considered unacceptable the
> > >>>>>>>>>>> approaches in [1], [2] as he considered it may lead to dificult to
> > >>>>>>>>>>> maintain code and code opened to subtle bugs (due to the potential of
> > >>>>>>>>>>> mixing devres and non-devres calls). He pointed out a similar approach
> > >>>>>>>>>>> that was done for the I2C bus [4], [5].
> > >>>>>>>>>>>
> > >>>>>>>>>>> As the discussions in [1], [2] stopped w/o a clear conclusion, this
> > >>>>>>>>>>> patch tries to revive it by proposing a similar approach that was done
> > >>>>>>>>>>> for the I2C bus.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Please let me know you input.  
> > >>>>>>>>>>
> > >>>>>>>>>> I'm with Jonathan here, the devres stuff is getting crazy here and you
> > >>>>>>>>>> have drivers mixing them and side affects happening and lots of
> > >>>>>>>>>> confusion.  Your change here is only going to make it even more
> > >>>>>>>>>> confusing, and shouldn't actually solve it for other busses (i.e. what
> > >>>>>>>>>> about iio devices NOT on the platform bus?)  
> > >>>>>>>
> > >>>>>>> In some cases they are already carrying the support as per the link
> > >>>>>>> above covering all i2c drivers.  I'd like to see a generic solution and
> > >>>>>>> I suspect pushing it to the device drivers rather than the bus code
> > >>>>>>> will explode badly and leave us with subtle bugs where people don't
> > >>>>>>> realise it is necessary.
> > >>>>>>>
> > >>>>>>> https://lore.kernel.org/all/20250224120608.1769039-1-claudiu.beznea.uj@bp.renesas.com/
> > >>>>>>> is a lot nastier looking than what we have here. I'll review that in a minute
> > >>>>>>> to show that it need not be that bad, but none the less not pleasant.
> > >>>>>>>
> > >>>>>>> +CC linux-iio to join up threads and Dmitry wrt to i2c case (and HID that does
> > >>>>>>> similar)  
> > >>>>>>
> > >>>>>> We should not expect individual drivers handle this, because this is a
> > >>>>>> layering violation: they need to know implementation details of the bus
> > >>>>>> code to know if the bus is using non-devres managed resources, and
> > >>>>>> adjust their behavior. Moving this into driver core is also not
> > >>>>>> feasible, as not all buses need it. So IMO this should belong to
> > >>>>>> individual bus code.
> > >>>>>>
> > >>>>>> Instead of using devres group a bus may opt to use
> > >>>>>> devm_add_action_or_reset() and other devm APIs to make sure bus'
> > >>>>>> resource unwinding is carried in the correct order relative to freeing
> > >>>>>> driver-owned resources.  
> > >>>>>
> > >>>>> Can you please let us know your input on the approach proposed in this
> > >>>>> patch? Or if you would prefer devm_add_action_or_reset() as suggested by
> > >>>>> Dmitry? Or if you consider another approach would fit better?
> > >>>>>
> > >>>>> Currently there were issues identified with the rzg2l-adc driver (driver
> > >>>>> based solution proposed in [1]) and with the rzg3s thermal driver (solved
> > >>>>> by function rzg3s_thermal_probe() from [2]).
> > >>>>>
> > >>>>> As expressed previously by Jonathan and Dimitry this is a common problem
> > >>>>> and as the issue is due to a call in the bus driver, would be better and
> > >>>>> simpler to handle it in the bus driver. Otherwise, individual drivers would
> > >>>>> have to be adjusted in a similar way.
> > >>>>>  
> > >>>>
> > >>>> Rafael,
> > >>>>
> > >>>> Greg suggested we ask for your input on the right option:
> > >>>>
> > >>>> https://lore.kernel.org/all/2025032703-genre-excitable-9473@gregkh/
> > >>>> (that thread has the other option).  
> > >>>
> > >>> Can you please let us know your opinion on this?  
> > >> Can you please let us know if you have any suggestions for this?  
> > > 
> > > It's been a while since I looked at this. Although as I understand it,
> > > the main issue comes from using devm_pm_runtime_enable().  
> > 
> > Yes, it comes from the usage of devm_pm_runtime_enable() in drivers and the
> > dev_pm_domain_detach() call in platform_remove() right after calling
> > driver's remove function.
> > 
> > On the platform I experienced issues with, the dev_pm_domain_detach() drops
> > the clocks from the device power domain and any subsequent PM runtime
> > resume calls (that may happen in the devres cleanup phase) have no effect
> > on enabling the clocks. If driver has functions registered (e.g. through
> > devm_add_action_or_reset()), or driver specific runtime PM functions that
> > access directly registers in the devres cleanup phase this leads to system
> > aborts.
> > 
> > 
> > > 
> > > As I have tried to argue before, I think devm_pm_runtime_enable()
> > > should *not* be used. Not here, not at all. Runtime PM isn't like any
> > > other resources that we fetch/release. Instead, it's a behaviour that
> > > you turn on and off, which needs to be managed more carefully, rather
> > > than relying on fetch/release ordering from devres.
> > > 
> > > That said, I would convert the driver to use pm_runtime_enable() and
> > > pm_runtime_disable() instead.  
> > 
> > I've tried this approach previously but it resulted in more complicated
> > code and thus, Jonathan wasn't happy with it [1].
> 
> Yes. I (and others as per the various threads) fundamentally disagree that
> runtime pm is special (in this aspect - it's special in many other good
> ways).
> 
> In the common, simple use case for runtime PM at the point or probe and
> remove it is exactly like any other resource that we enable then disable.
> What happens between those stages isn't particularly relevant here. We
> have other resources also go through state changes between probe and
> remove and those sequences are easy to manage.  Where more complex
> management is necessary then indeed not using this call is of course the
> way to go. Those just aren't the common cases I see.
> 
> Removing this call adds considerable potential code complexity to many
> drivers which may be trivially solved through the approaches that have
> been suggested and have been in use for some time on several bus types.
> 
> That code complexity has nothing to with this call at all but to do with
> needing to drop lots of other devm management because of the potential
> for races that any mix and match of devm and other entails.
> 
> I'd approximate that every time we stop using devm_pm_runtime_enable()
> we add 10-20 lines of code that is just there because that call can't
> be used. That code will come with a review burden and related bugs and
> potentially applies in 100-1000s of drivers.
> 
> Fundamentally we need a clear statement on which way to go from Rafael
> and either to proceed with a common solution across all buses that encounter
> this issue, or rip out the existing uses in other buses. (or a 3rd way?)
> Mix and match depending on bus type is not a sensible way to proceed.
> 
> > 
> > Another approach I've tried was to have devres group opened/closed in the
> > driver itself [2], [3] but it was postponed as this approach may have a chance.
> 
> The in driver approach is unfortunately a sticking plaster, working around
> it in each driver.  Maybe that's the best we can do.  Pushing the burden
> of infrastructure a driver can't see (because is in the bus layer) down
> to drivers is messy at best and most like a source of future bugs.
> Removing a specific devm_ helper doesn't stop people rolling their own with
> no awareness of this weird subtlety.
> 
> More fundamentally I don't personally think a bus driver should ever
> be doing things that in it's own probe and remove that can result in races
> wrt to devm handling in the drivers.  We should not be making driver writers
> jobs hard.  The solutions Dmitry used for other buses resolve that in a more
> general fashion, at the point where we know if it is necessary (next to
> the bus driver calls that are at issue) so to me look like a good way to go.
> (this is one of the approaches Claudiu has posted for platform devices).

We should stop thinking about this as a particular problem with
devm_pm_runtime_enable(). Any non-trivial bus probe()/remove() code
(going beyond simply calling driver's probe and remove) will run into
this problem. In HID we had an issue with bus core closing the
underlying transport device and releasing transfer buffers (memory
buffers) too early, before the driver had a chance to clean up properly,
resulting in UAF.

Thanks.

-- 
Dmitry

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Ulf Hansson]

On Fri, 9 May 2025 at 16:12, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>
> Hi, Ulf,
>
> Thank you for your input!
>
> On 09.05.2025 16:07, Ulf Hansson wrote:
> > On Fri, 9 May 2025 at 13:51, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >>
> >> Hi, Rafael, Ulf, PM list,
> >>
> >>
> >> On 09.04.2025 19:12, Claudiu Beznea wrote:
> >>> Hi, Rafael,
> >>>
> >>> On 30.03.2025 18:31, Jonathan Cameron wrote:
> >>>> On Thu, 27 Mar 2025 18:47:53 +0200
> >>>> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >>>>
> >>>>> Hi, Rafael,
> >>>>>
> >>>>> On 06.03.2025 08:11, Dmitry Torokhov wrote:
> >>>>>> On Wed, Mar 05, 2025 at 02:03:09PM +0000, Jonathan Cameron wrote:
> >>>>>>> On Wed, 19 Feb 2025 14:45:07 +0200
> >>>>>>> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >>>>>>>
> >>>>>>>> Hi, Daniel, Jonathan,
> >>>>>>>>
> >>>>>>>> On 15.02.2025 15:51, Claudiu Beznea wrote:
> >>>>>>>>> Hi, Greg,
> >>>>>>>>>
> >>>>>>>>> On 15.02.2025 15:25, Greg KH wrote:
> >>>>>>>>>> On Sat, Feb 15, 2025 at 03:08:49PM +0200, Claudiu wrote:
> >>>>>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>>>>>>>>
> >>>>>>>>>>> On the Renesas RZ/G3S (and other Renesas SoCs, e.g., RZ/G2{L, LC, UL}),
> >>>>>>>>>>> clocks are managed through PM domains. These PM domains, registered on
> >>>>>>>>>>> behalf of the clock controller driver, are configured with
> >>>>>>>>>>> GENPD_FLAG_PM_CLK. In most of the Renesas drivers used by RZ SoCs, the
> >>>>>>>>>>> clocks are enabled/disabled using runtime PM APIs. The power domains may
> >>>>>>>>>>> also have power_on/power_off support implemented. After the device PM
> >>>>>>>>>>> domain is powered off any CPU accesses to these domains leads to system
> >>>>>>>>>>> aborts.
> >>>>>>>>>>>
> >>>>>>>>>>> During probe, devices are attached to the PM domain controlling their
> >>>>>>>>>>> clocks and power. Similarly, during removal, devices are detached from the
> >>>>>>>>>>> PM domain.
> >>>>>>>>>>>
> >>>>>>>>>>> The detachment call stack is as follows:
> >>>>>>>>>>>
> >>>>>>>>>>> device_driver_detach() ->
> >>>>>>>>>>>   device_release_driver_internal() ->
> >>>>>>>>>>>     __device_release_driver() ->
> >>>>>>>>>>>       device_remove() ->
> >>>>>>>>>>>         platform_remove() ->
> >>>>>>>>>>>         dev_pm_domain_detach()
> >>>>>>>>>>>
> >>>>>>>>>>> During driver unbind, after the device is detached from its PM domain,
> >>>>>>>>>>> the device_unbind_cleanup() function is called, which subsequently invokes
> >>>>>>>>>>> devres_release_all(). This function handles devres resource cleanup.
> >>>>>>>>>>>
> >>>>>>>>>>> If runtime PM is enabled in driver probe via devm_pm_runtime_enable(), the
> >>>>>>>>>>> cleanup process triggers the action or reset function for disabling runtime
> >>>>>>>>>>> PM. This function is pm_runtime_disable_action(), which leads to the
> >>>>>>>>>>> following call stack of interest when called:
> >>>>>>>>>>>
> >>>>>>>>>>> pm_runtime_disable_action() ->
> >>>>>>>>>>>   pm_runtime_dont_use_autosuspend() ->
> >>>>>>>>>>>     __pm_runtime_use_autosuspend() ->
> >>>>>>>>>>>       update_autosuspend() ->
> >>>>>>>>>>>         rpm_idle()
> >>>>>>>>>>>
> >>>>>>>>>>> The rpm_idle() function attempts to resume the device at runtime. However,
> >>>>>>>>>>> at the point it is called, the device is no longer part of a PM domain
> >>>>>>>>>>> (which manages clocks and power states). If the driver implements its own
> >>>>>>>>>>> runtime PM APIs for specific functionalities - such as the rzg2l_adc
> >>>>>>>>>>> driver - while also relying on the power domain subsystem for power
> >>>>>>>>>>> management, rpm_idle() will invoke the driver's runtime PM API. However,
> >>>>>>>>>>> since the device is no longer part of a PM domain at this point, the PM
> >>>>>>>>>>> domain's runtime PM APIs will not be called. This leads to system aborts on
> >>>>>>>>>>> Renesas SoCs.
> >>>>>>>>>>>
> >>>>>>>>>>> Another identified case is when a subsystem performs various cleanups
> >>>>>>>>>>> using device_unbind_cleanup(), calling driver-specific APIs in the process.
> >>>>>>>>>>> A known example is the thermal subsystem, which may call driver-specific
> >>>>>>>>>>> APIs to disable the thermal device. The relevant call stack in this case
> >>>>>>>>>>> is:
> >>>>>>>>>>>
> >>>>>>>>>>> device_driver_detach() ->
> >>>>>>>>>>>   device_release_driver_internal() ->
> >>>>>>>>>>>     device_unbind_cleanup() ->
> >>>>>>>>>>>       devres_release_all() ->
> >>>>>>>>>>>         devm_thermal_of_zone_release() ->
> >>>>>>>>>>>         thermal_zone_device_disable() ->
> >>>>>>>>>>>           thermal_zone_device_set_mode() ->
> >>>>>>>>>>>             struct thermal_zone_device_ops::change_mode()
> >>>>>>>>>>>
> >>>>>>>>>>> At the moment the driver-specific change_mode() API is called, the device
> >>>>>>>>>>> is no longer part of its PM domain. Accessing its registers without proper
> >>>>>>>>>>> power management leads to system aborts.
> >>>>>>>>>>>
> >>>>>>>>>>> Open a devres group before calling the driver probe, and close it
> >>>>>>>>>>> immediately after the driver remove function is called and before
> >>>>>>>>>>> dev_pm_domain_detach(). This ensures that driver-specific devm actions or
> >>>>>>>>>>> reset functions are executed immediately after the driver remove function
> >>>>>>>>>>> completes. Additionally, it prevents driver-specific runtime PM APIs from
> >>>>>>>>>>> being called when the device is no longer part of its power domain.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>>
> >>>>>>>>>>> Hi,
> >>>>>>>
> >>>>>>> Hi Claudiu, Greg,
> >>>>>>>
> >>>>>>> Sorry, I missed this thread whilst travelling and only saw it because
> >>>>>>> of reference from the in driver solution.
> >>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Although Ulf gave its green light for the approaches on both IIO [1],
> >>>>>>>>>>> [2] and thermal subsystems [3], Jonathan considered unacceptable the
> >>>>>>>>>>> approaches in [1], [2] as he considered it may lead to dificult to
> >>>>>>>>>>> maintain code and code opened to subtle bugs (due to the potential of
> >>>>>>>>>>> mixing devres and non-devres calls). He pointed out a similar approach
> >>>>>>>>>>> that was done for the I2C bus [4], [5].
> >>>>>>>>>>>
> >>>>>>>>>>> As the discussions in [1], [2] stopped w/o a clear conclusion, this
> >>>>>>>>>>> patch tries to revive it by proposing a similar approach that was done
> >>>>>>>>>>> for the I2C bus.
> >>>>>>>>>>>
> >>>>>>>>>>> Please let me know you input.
> >>>>>>>>>>
> >>>>>>>>>> I'm with Jonathan here, the devres stuff is getting crazy here and you
> >>>>>>>>>> have drivers mixing them and side affects happening and lots of
> >>>>>>>>>> confusion.  Your change here is only going to make it even more
> >>>>>>>>>> confusing, and shouldn't actually solve it for other busses (i.e. what
> >>>>>>>>>> about iio devices NOT on the platform bus?)
> >>>>>>>
> >>>>>>> In some cases they are already carrying the support as per the link
> >>>>>>> above covering all i2c drivers.  I'd like to see a generic solution and
> >>>>>>> I suspect pushing it to the device drivers rather than the bus code
> >>>>>>> will explode badly and leave us with subtle bugs where people don't
> >>>>>>> realise it is necessary.
> >>>>>>>
> >>>>>>> https://lore.kernel.org/all/20250224120608.1769039-1-claudiu.beznea.uj@bp.renesas.com/
> >>>>>>> is a lot nastier looking than what we have here. I'll review that in a minute
> >>>>>>> to show that it need not be that bad, but none the less not pleasant.
> >>>>>>>
> >>>>>>> +CC linux-iio to join up threads and Dmitry wrt to i2c case (and HID that does
> >>>>>>> similar)
> >>>>>>
> >>>>>> We should not expect individual drivers handle this, because this is a
> >>>>>> layering violation: they need to know implementation details of the bus
> >>>>>> code to know if the bus is using non-devres managed resources, and
> >>>>>> adjust their behavior. Moving this into driver core is also not
> >>>>>> feasible, as not all buses need it. So IMO this should belong to
> >>>>>> individual bus code.
> >>>>>>
> >>>>>> Instead of using devres group a bus may opt to use
> >>>>>> devm_add_action_or_reset() and other devm APIs to make sure bus'
> >>>>>> resource unwinding is carried in the correct order relative to freeing
> >>>>>> driver-owned resources.
> >>>>>
> >>>>> Can you please let us know your input on the approach proposed in this
> >>>>> patch? Or if you would prefer devm_add_action_or_reset() as suggested by
> >>>>> Dmitry? Or if you consider another approach would fit better?
> >>>>>
> >>>>> Currently there were issues identified with the rzg2l-adc driver (driver
> >>>>> based solution proposed in [1]) and with the rzg3s thermal driver (solved
> >>>>> by function rzg3s_thermal_probe() from [2]).
> >>>>>
> >>>>> As expressed previously by Jonathan and Dimitry this is a common problem
> >>>>> and as the issue is due to a call in the bus driver, would be better and
> >>>>> simpler to handle it in the bus driver. Otherwise, individual drivers would
> >>>>> have to be adjusted in a similar way.
> >>>>>
> >>>>
> >>>> Rafael,
> >>>>
> >>>> Greg suggested we ask for your input on the right option:
> >>>>
> >>>> https://lore.kernel.org/all/2025032703-genre-excitable-9473@gregkh/
> >>>> (that thread has the other option).
> >>>
> >>> Can you please let us know your opinion on this?
> >> Can you please let us know if you have any suggestions for this?
> >
> > It's been a while since I looked at this. Although as I understand it,
> > the main issue comes from using devm_pm_runtime_enable().
>
> Yes, it comes from the usage of devm_pm_runtime_enable() in drivers and the
> dev_pm_domain_detach() call in platform_remove() right after calling
> driver's remove function.

Okay.

>
> On the platform I experienced issues with, the dev_pm_domain_detach() drops
> the clocks from the device power domain and any subsequent PM runtime
> resume calls (that may happen in the devres cleanup phase) have no effect
> on enabling the clocks. If driver has functions registered (e.g. through
> devm_add_action_or_reset()), or driver specific runtime PM functions that
> access directly registers in the devres cleanup phase this leads to system
> aborts.

So if you move away from using devm_pm_runtime_enable() things would
be easier to manage and there is no additional new devres-management
needed.

>
>
> >
> > As I have tried to argue before, I think devm_pm_runtime_enable()
> > should *not* be used. Not here, not at all. Runtime PM isn't like any
> > other resources that we fetch/release. Instead, it's a behaviour that
> > you turn on and off, which needs to be managed more carefully, rather
> > than relying on fetch/release ordering from devres.
> >
> > That said, I would convert the driver to use pm_runtime_enable() and
> > pm_runtime_disable() instead.
>
> I've tried this approach previously but it resulted in more complicated
> code and thus, Jonathan wasn't happy with it [1].

I understand that you have been trying to move forward to address
people's opinions. It's not always easy to keep everybody happy. :-)

That said, I still think this is the most viable option as it's how
the vast majority of drivers do it today. A few lines of additional
code shouldn't really be a big problem in my opinion.

>
> Another approach I've tried was to have devres group opened/closed in the
> driver itself [2], [3] but it was postponed as this approach may have a chance.
>
> At the moment I have 2 drivers waiting for a resolution on this [2], [3]
> and I recently posted a new one [4] that uses driver specific local devres
> group to avoid this issue.
>

Kind regards
Uffe

>
> Thank you,
> Claudiu
>
> [1]
> https://lore.kernel.org/all/20250224120608.1769039-2-claudiu.beznea.uj@bp.renesas.com
> [2]
> https://lore.kernel.org/all/20250324122627.32336-2-claudiu.beznea.uj@bp.renesas.com
> [3]
> https://lore.kernel.org/all/20250324135701.179827-3-claudiu.beznea.uj@bp.renesas.com/
> [4]
> https://lore.kernel.org/all/20250430103236.3511989-6-claudiu.beznea.uj@bp.renesas.com

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Dmitry Torokhov]

Hi Ulf,

On Mon, May 19, 2025 at 05:02:05PM +0200, Ulf Hansson wrote:
> On Fri, 9 May 2025 at 16:12, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >
> > Hi, Ulf,
> >
> > Thank you for your input!
> >
> > On 09.05.2025 16:07, Ulf Hansson wrote:
> > > On Fri, 9 May 2025 at 13:51, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > >>
> > >> Hi, Rafael, Ulf, PM list,
> > >>
> > >>
> > >> On 09.04.2025 19:12, Claudiu Beznea wrote:
> > >>> Hi, Rafael,
> > >>>
> > >>> On 30.03.2025 18:31, Jonathan Cameron wrote:
> > >>>> On Thu, 27 Mar 2025 18:47:53 +0200
> > >>>> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > >>>>
> > >>>>> Hi, Rafael,
> > >>>>>
> > >>>>> On 06.03.2025 08:11, Dmitry Torokhov wrote:
> > >>>>>> On Wed, Mar 05, 2025 at 02:03:09PM +0000, Jonathan Cameron wrote:
> > >>>>>>> On Wed, 19 Feb 2025 14:45:07 +0200
> > >>>>>>> Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > >>>>>>>
> > >>>>>>>> Hi, Daniel, Jonathan,
> > >>>>>>>>
> > >>>>>>>> On 15.02.2025 15:51, Claudiu Beznea wrote:
> > >>>>>>>>> Hi, Greg,
> > >>>>>>>>>
> > >>>>>>>>> On 15.02.2025 15:25, Greg KH wrote:
> > >>>>>>>>>> On Sat, Feb 15, 2025 at 03:08:49PM +0200, Claudiu wrote:
> > >>>>>>>>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > >>>>>>>>>>>
> > >>>>>>>>>>> On the Renesas RZ/G3S (and other Renesas SoCs, e.g., RZ/G2{L, LC, UL}),
> > >>>>>>>>>>> clocks are managed through PM domains. These PM domains, registered on
> > >>>>>>>>>>> behalf of the clock controller driver, are configured with
> > >>>>>>>>>>> GENPD_FLAG_PM_CLK. In most of the Renesas drivers used by RZ SoCs, the
> > >>>>>>>>>>> clocks are enabled/disabled using runtime PM APIs. The power domains may
> > >>>>>>>>>>> also have power_on/power_off support implemented. After the device PM
> > >>>>>>>>>>> domain is powered off any CPU accesses to these domains leads to system
> > >>>>>>>>>>> aborts.
> > >>>>>>>>>>>
> > >>>>>>>>>>> During probe, devices are attached to the PM domain controlling their
> > >>>>>>>>>>> clocks and power. Similarly, during removal, devices are detached from the
> > >>>>>>>>>>> PM domain.
> > >>>>>>>>>>>
> > >>>>>>>>>>> The detachment call stack is as follows:
> > >>>>>>>>>>>
> > >>>>>>>>>>> device_driver_detach() ->
> > >>>>>>>>>>>   device_release_driver_internal() ->
> > >>>>>>>>>>>     __device_release_driver() ->
> > >>>>>>>>>>>       device_remove() ->
> > >>>>>>>>>>>         platform_remove() ->
> > >>>>>>>>>>>         dev_pm_domain_detach()
> > >>>>>>>>>>>
> > >>>>>>>>>>> During driver unbind, after the device is detached from its PM domain,
> > >>>>>>>>>>> the device_unbind_cleanup() function is called, which subsequently invokes
> > >>>>>>>>>>> devres_release_all(). This function handles devres resource cleanup.
> > >>>>>>>>>>>
> > >>>>>>>>>>> If runtime PM is enabled in driver probe via devm_pm_runtime_enable(), the
> > >>>>>>>>>>> cleanup process triggers the action or reset function for disabling runtime
> > >>>>>>>>>>> PM. This function is pm_runtime_disable_action(), which leads to the
> > >>>>>>>>>>> following call stack of interest when called:
> > >>>>>>>>>>>
> > >>>>>>>>>>> pm_runtime_disable_action() ->
> > >>>>>>>>>>>   pm_runtime_dont_use_autosuspend() ->
> > >>>>>>>>>>>     __pm_runtime_use_autosuspend() ->
> > >>>>>>>>>>>       update_autosuspend() ->
> > >>>>>>>>>>>         rpm_idle()
> > >>>>>>>>>>>
> > >>>>>>>>>>> The rpm_idle() function attempts to resume the device at runtime. However,
> > >>>>>>>>>>> at the point it is called, the device is no longer part of a PM domain
> > >>>>>>>>>>> (which manages clocks and power states). If the driver implements its own
> > >>>>>>>>>>> runtime PM APIs for specific functionalities - such as the rzg2l_adc
> > >>>>>>>>>>> driver - while also relying on the power domain subsystem for power
> > >>>>>>>>>>> management, rpm_idle() will invoke the driver's runtime PM API. However,
> > >>>>>>>>>>> since the device is no longer part of a PM domain at this point, the PM
> > >>>>>>>>>>> domain's runtime PM APIs will not be called. This leads to system aborts on
> > >>>>>>>>>>> Renesas SoCs.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Another identified case is when a subsystem performs various cleanups
> > >>>>>>>>>>> using device_unbind_cleanup(), calling driver-specific APIs in the process.
> > >>>>>>>>>>> A known example is the thermal subsystem, which may call driver-specific
> > >>>>>>>>>>> APIs to disable the thermal device. The relevant call stack in this case
> > >>>>>>>>>>> is:
> > >>>>>>>>>>>
> > >>>>>>>>>>> device_driver_detach() ->
> > >>>>>>>>>>>   device_release_driver_internal() ->
> > >>>>>>>>>>>     device_unbind_cleanup() ->
> > >>>>>>>>>>>       devres_release_all() ->
> > >>>>>>>>>>>         devm_thermal_of_zone_release() ->
> > >>>>>>>>>>>         thermal_zone_device_disable() ->
> > >>>>>>>>>>>           thermal_zone_device_set_mode() ->
> > >>>>>>>>>>>             struct thermal_zone_device_ops::change_mode()
> > >>>>>>>>>>>
> > >>>>>>>>>>> At the moment the driver-specific change_mode() API is called, the device
> > >>>>>>>>>>> is no longer part of its PM domain. Accessing its registers without proper
> > >>>>>>>>>>> power management leads to system aborts.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Open a devres group before calling the driver probe, and close it
> > >>>>>>>>>>> immediately after the driver remove function is called and before
> > >>>>>>>>>>> dev_pm_domain_detach(). This ensures that driver-specific devm actions or
> > >>>>>>>>>>> reset functions are executed immediately after the driver remove function
> > >>>>>>>>>>> completes. Additionally, it prevents driver-specific runtime PM APIs from
> > >>>>>>>>>>> being called when the device is no longer part of its power domain.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > >>>>>>>>>>> ---
> > >>>>>>>>>>>
> > >>>>>>>>>>> Hi,
> > >>>>>>>
> > >>>>>>> Hi Claudiu, Greg,
> > >>>>>>>
> > >>>>>>> Sorry, I missed this thread whilst travelling and only saw it because
> > >>>>>>> of reference from the in driver solution.
> > >>>>>>>
> > >>>>>>>>>>>
> > >>>>>>>>>>> Although Ulf gave its green light for the approaches on both IIO [1],
> > >>>>>>>>>>> [2] and thermal subsystems [3], Jonathan considered unacceptable the
> > >>>>>>>>>>> approaches in [1], [2] as he considered it may lead to dificult to
> > >>>>>>>>>>> maintain code and code opened to subtle bugs (due to the potential of
> > >>>>>>>>>>> mixing devres and non-devres calls). He pointed out a similar approach
> > >>>>>>>>>>> that was done for the I2C bus [4], [5].
> > >>>>>>>>>>>
> > >>>>>>>>>>> As the discussions in [1], [2] stopped w/o a clear conclusion, this
> > >>>>>>>>>>> patch tries to revive it by proposing a similar approach that was done
> > >>>>>>>>>>> for the I2C bus.
> > >>>>>>>>>>>
> > >>>>>>>>>>> Please let me know you input.
> > >>>>>>>>>>
> > >>>>>>>>>> I'm with Jonathan here, the devres stuff is getting crazy here and you
> > >>>>>>>>>> have drivers mixing them and side affects happening and lots of
> > >>>>>>>>>> confusion.  Your change here is only going to make it even more
> > >>>>>>>>>> confusing, and shouldn't actually solve it for other busses (i.e. what
> > >>>>>>>>>> about iio devices NOT on the platform bus?)
> > >>>>>>>
> > >>>>>>> In some cases they are already carrying the support as per the link
> > >>>>>>> above covering all i2c drivers.  I'd like to see a generic solution and
> > >>>>>>> I suspect pushing it to the device drivers rather than the bus code
> > >>>>>>> will explode badly and leave us with subtle bugs where people don't
> > >>>>>>> realise it is necessary.
> > >>>>>>>
> > >>>>>>> https://lore.kernel.org/all/20250224120608.1769039-1-claudiu.beznea.uj@bp.renesas.com/
> > >>>>>>> is a lot nastier looking than what we have here. I'll review that in a minute
> > >>>>>>> to show that it need not be that bad, but none the less not pleasant.
> > >>>>>>>
> > >>>>>>> +CC linux-iio to join up threads and Dmitry wrt to i2c case (and HID that does
> > >>>>>>> similar)
> > >>>>>>
> > >>>>>> We should not expect individual drivers handle this, because this is a
> > >>>>>> layering violation: they need to know implementation details of the bus
> > >>>>>> code to know if the bus is using non-devres managed resources, and
> > >>>>>> adjust their behavior. Moving this into driver core is also not
> > >>>>>> feasible, as not all buses need it. So IMO this should belong to
> > >>>>>> individual bus code.
> > >>>>>>
> > >>>>>> Instead of using devres group a bus may opt to use
> > >>>>>> devm_add_action_or_reset() and other devm APIs to make sure bus'
> > >>>>>> resource unwinding is carried in the correct order relative to freeing
> > >>>>>> driver-owned resources.
> > >>>>>
> > >>>>> Can you please let us know your input on the approach proposed in this
> > >>>>> patch? Or if you would prefer devm_add_action_or_reset() as suggested by
> > >>>>> Dmitry? Or if you consider another approach would fit better?
> > >>>>>
> > >>>>> Currently there were issues identified with the rzg2l-adc driver (driver
> > >>>>> based solution proposed in [1]) and with the rzg3s thermal driver (solved
> > >>>>> by function rzg3s_thermal_probe() from [2]).
> > >>>>>
> > >>>>> As expressed previously by Jonathan and Dimitry this is a common problem
> > >>>>> and as the issue is due to a call in the bus driver, would be better and
> > >>>>> simpler to handle it in the bus driver. Otherwise, individual drivers would
> > >>>>> have to be adjusted in a similar way.
> > >>>>>
> > >>>>
> > >>>> Rafael,
> > >>>>
> > >>>> Greg suggested we ask for your input on the right option:
> > >>>>
> > >>>> https://lore.kernel.org/all/2025032703-genre-excitable-9473@gregkh/
> > >>>> (that thread has the other option).
> > >>>
> > >>> Can you please let us know your opinion on this?
> > >> Can you please let us know if you have any suggestions for this?
> > >
> > > It's been a while since I looked at this. Although as I understand it,
> > > the main issue comes from using devm_pm_runtime_enable().
> >
> > Yes, it comes from the usage of devm_pm_runtime_enable() in drivers and the
> > dev_pm_domain_detach() call in platform_remove() right after calling
> > driver's remove function.
> 
> Okay.

This is not the root of the problem though. There is nothing really
special about power domain and runtime power management. The root of the
problem is that current code violates the order of releasing resources
by mixing devm- and normal resource management together. Usually it is
individual driver's fault, but in this case it is bus code that uses the
manual release (dev_om_domain_detach) that violates the "release in
opposite order to acquisition" rule.

> 
> >
> > On the platform I experienced issues with, the dev_pm_domain_detach() drops
> > the clocks from the device power domain and any subsequent PM runtime
> > resume calls (that may happen in the devres cleanup phase) have no effect
> > on enabling the clocks. If driver has functions registered (e.g. through
> > devm_add_action_or_reset()), or driver specific runtime PM functions that
> > access directly registers in the devres cleanup phase this leads to system
> > aborts.
> 
> So if you move away from using devm_pm_runtime_enable() things would
> be easier to manage and there is no additional new devres-management
> needed.

How exactly will it improve the situation? You still need to make sure
that you are not disabling things out of the order. You simply moving
the complexity to the driver, essentially forbidding it (and any other
driver on platform bus) from using any devm APIs. 

> 
> >
> >
> > >
> > > As I have tried to argue before, I think devm_pm_runtime_enable()
> > > should *not* be used. Not here, not at all. Runtime PM isn't like any
> > > other resources that we fetch/release. Instead, it's a behaviour that
> > > you turn on and off, which needs to be managed more carefully, rather
> > > than relying on fetch/release ordering from devres.

I disagree. It is a resource that you turn on and off, same as clocks,
regulators, interrupts, etc. We manage those during lifetime of the
device, disable them when going into low power mode/suspend, reenable
them upon resume, may disable and reenable them for other reasons.

PM is not any more special here. As long as you keep the proper order of
operations it works as well.

> > >
> > > That said, I would convert the driver to use pm_runtime_enable() and
> > > pm_runtime_disable() instead.
> >
> > I've tried this approach previously but it resulted in more complicated
> > code and thus, Jonathan wasn't happy with it [1].
> 
> I understand that you have been trying to move forward to address
> people's opinions. It's not always easy to keep everybody happy. :-)
> 
> That said, I still think this is the most viable option as it's how
> the vast majority of drivers do it today. A few lines of additional
> code shouldn't really be a big problem in my opinion.

Have you tried making such change? Again, you will need to abandon use
of most other devm APIs so that you keep the order of releasing
resources. The only devm that you can still use is devm_k*alloc(), the
rest has to be converted into unmanaged.

Thanks.

-- 
Dmitry

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Ulf Hansson]

[...]

> > > >>>>>
> > > >>>>
> > > >>>> Rafael,
> > > >>>>
> > > >>>> Greg suggested we ask for your input on the right option:
> > > >>>>
> > > >>>> https://lore.kernel.org/all/2025032703-genre-excitable-9473@gregkh/
> > > >>>> (that thread has the other option).
> > > >>>
> > > >>> Can you please let us know your opinion on this?
> > > >> Can you please let us know if you have any suggestions for this?
> > > >
> > > > It's been a while since I looked at this. Although as I understand it,
> > > > the main issue comes from using devm_pm_runtime_enable().
> > >
> > > Yes, it comes from the usage of devm_pm_runtime_enable() in drivers and the
> > > dev_pm_domain_detach() call in platform_remove() right after calling
> > > driver's remove function.
> >
> > Okay.
>
> This is not the root of the problem though. There is nothing really
> special about power domain and runtime power management. The root of the
> problem is that current code violates the order of releasing resources
> by mixing devm- and normal resource management together. Usually it is
> individual driver's fault, but in this case it is bus code that uses the
> manual release (dev_om_domain_detach) that violates the "release in
> opposite order to acquisition" rule.

As I said before, runtime PM is not a regular resource, but a
behaviour that we turn on/off for a device. Enabling and disabling
runtime PM needs to be managed more carefully in my opinion.

For example, even if the order is made correctly, suppose a driver's
->remove() callback completes by turning off the resources for its
device and leaves runtime PM enabled, as it relies on devres to do it
some point later. Beyond this point, nothing would prevent userspace
for runtime resuming/suspending the device via sysfs. I would be quite
worried if that happens as it certainly would lead to undefined
behaviour.

>
> >
> > >
> > > On the platform I experienced issues with, the dev_pm_domain_detach() drops
> > > the clocks from the device power domain and any subsequent PM runtime
> > > resume calls (that may happen in the devres cleanup phase) have no effect
> > > on enabling the clocks. If driver has functions registered (e.g. through
> > > devm_add_action_or_reset()), or driver specific runtime PM functions that
> > > access directly registers in the devres cleanup phase this leads to system
> > > aborts.
> >
> > So if you move away from using devm_pm_runtime_enable() things would
> > be easier to manage and there is no additional new devres-management
> > needed.
>
> How exactly will it improve the situation? You still need to make sure
> that you are not disabling things out of the order. You simply moving
> the complexity to the driver, essentially forbidding it (and any other
> driver on platform bus) from using any devm APIs.

The driver can still use the devres APIs to "get" all resources and
then rely on devres to "put" them. There is nothing that prevents
that, right?

Or maybe I didn't understand the problem correctly?

>
> >
> > >
> > >
> > > >
> > > > As I have tried to argue before, I think devm_pm_runtime_enable()
> > > > should *not* be used. Not here, not at all. Runtime PM isn't like any
> > > > other resources that we fetch/release. Instead, it's a behaviour that
> > > > you turn on and off, which needs to be managed more carefully, rather
> > > > than relying on fetch/release ordering from devres.
>
> I disagree. It is a resource that you turn on and off, same as clocks,
> regulators, interrupts, etc. We manage those during lifetime of the
> device, disable them when going into low power mode/suspend, reenable
> them upon resume, may disable and reenable them for other reasons.
>
> PM is not any more special here. As long as you keep the proper order of
> operations it works as well.

How would you solve the issue I pointed out above?

>
> > > >
> > > > That said, I would convert the driver to use pm_runtime_enable() and
> > > > pm_runtime_disable() instead.
> > >
> > > I've tried this approach previously but it resulted in more complicated
> > > code and thus, Jonathan wasn't happy with it [1].
> >
> > I understand that you have been trying to move forward to address
> > people's opinions. It's not always easy to keep everybody happy. :-)
> >
> > That said, I still think this is the most viable option as it's how
> > the vast majority of drivers do it today. A few lines of additional
> > code shouldn't really be a big problem in my opinion.
>
> Have you tried making such change? Again, you will need to abandon use
> of most other devm APIs so that you keep the order of releasing
> resources. The only devm that you can still use is devm_k*alloc(), the
> rest has to be converted into unmanaged.

I guess I need to take a stab at this particular use case.

Looking closer, could it be that it's really the combination of
turning on/off resources using devres (not just get/put if them) like
clocks - and using devm_pm_runtime_enable()?

Kind regards
Uffe

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Dmitry Torokhov]

On Tue, May 20, 2025 at 02:09:36PM +0200, Ulf Hansson wrote:
> [...]
> 
> > > > >>>>>
> > > > >>>>
> > > > >>>> Rafael,
> > > > >>>>
> > > > >>>> Greg suggested we ask for your input on the right option:
> > > > >>>>
> > > > >>>> https://lore.kernel.org/all/2025032703-genre-excitable-9473@gregkh/
> > > > >>>> (that thread has the other option).
> > > > >>>
> > > > >>> Can you please let us know your opinion on this?
> > > > >> Can you please let us know if you have any suggestions for this?
> > > > >
> > > > > It's been a while since I looked at this. Although as I understand it,
> > > > > the main issue comes from using devm_pm_runtime_enable().
> > > >
> > > > Yes, it comes from the usage of devm_pm_runtime_enable() in drivers and the
> > > > dev_pm_domain_detach() call in platform_remove() right after calling
> > > > driver's remove function.
> > >
> > > Okay.
> >
> > This is not the root of the problem though. There is nothing really
> > special about power domain and runtime power management. The root of the
> > problem is that current code violates the order of releasing resources
> > by mixing devm- and normal resource management together. Usually it is
> > individual driver's fault, but in this case it is bus code that uses the
> > manual release (dev_om_domain_detach) that violates the "release in
> > opposite order to acquisition" rule.
> 
> As I said before, runtime PM is not a regular resource, but a
> behaviour that we turn on/off for a device. Enabling and disabling
> runtime PM needs to be managed more carefully in my opinion.

You also need to be careful enabling and disabling interrupts, be
careful with power supplies to make sure the chip you want to talk to is
powered up, and has clocks running.

But I think you are concentrating too much on runtime PM aspect and are
missing the bigger picture: even in the absence of runtime PM we should
not detach the device from its power domain out of order compared to
releasing other resources.

We have:

static void platform_remove(struct device *_dev)
{
	struct platform_driver *drv = to_platform_driver(_dev->driver);
	struct platform_device *dev = to_platform_device(_dev);

	if (drv->remove)
		drv->remove(dev);
	dev_pm_domain_detach(_dev, true);
}

That "dev_pm_domain_detach(_dev, true)" means that device may get
powered off. If you look at for example ACPI power domain it will end up
calling acpi_dev_pm_low_power() which may place the device into D3Cold
state. At this point you can't talk to the device.

Now, if the driver used devm APIs, all the resources acquired will be
released *after* we detach from the domain. That means that interrupts
may still be firing, custom devm callbacks may still try to communicate
with the device, etc. This can cause multitude of errors, from
relatively benign logs in dmesg to system hangs. 

To solve this is simple: make sure we take devm into account and
release all devm resources acquired after the device was attached to a
power domain before we detach the device from that power domain.

If we do this we can also safely use devm_pm_runtime_enable() and it
will be disabled at the right time and everyone will be happy.

> 
> For example, even if the order is made correctly, suppose a driver's
> ->remove() callback completes by turning off the resources for its
> device and leaves runtime PM enabled, as it relies on devres to do it
> some point later. Beyond this point, nothing would prevent userspace
> for runtime resuming/suspending the device via sysfs. I would be quite
> worried if that happens as it certainly would lead to undefined
> behaviour.

If you have this situation that means that you have logic error in the
driver. You should not run off resources too early. devm usually helps
with that. If you do:

devm_regulator_get_enaled()
devm_clk_get_enabled()
devm_request_threaded_irq()
devm_pm_runtime_enable()

then at remove time you should see:

runtime_pm_disable()
free_irq()
clk_disable()
regulator_disable/put()

in this particular order, which should be safe. But if you start mixing
things up and let's say use clk_get() (not devm variant) and explicitly
call clk_put() in remove() you will end up with clocks disabled but
interrupts still active and trouble will ensue.

The point of devm is so that you do not simply "leave runtime PM
enabled" with other resources shut off.

> 
> >
> > >
> > > >
> > > > On the platform I experienced issues with, the dev_pm_domain_detach() drops
> > > > the clocks from the device power domain and any subsequent PM runtime
> > > > resume calls (that may happen in the devres cleanup phase) have no effect
> > > > on enabling the clocks. If driver has functions registered (e.g. through
> > > > devm_add_action_or_reset()), or driver specific runtime PM functions that
> > > > access directly registers in the devres cleanup phase this leads to system
> > > > aborts.
> > >
> > > So if you move away from using devm_pm_runtime_enable() things would
> > > be easier to manage and there is no additional new devres-management
> > > needed.
> >
> > How exactly will it improve the situation? You still need to make sure
> > that you are not disabling things out of the order. You simply moving
> > the complexity to the driver, essentially forbidding it (and any other
> > driver on platform bus) from using any devm APIs.
> 
> The driver can still use the devres APIs to "get" all resources and
> then rely on devres to "put" them. There is nothing that prevents
> that, right?

*Order* is important. You need to release everything in reverse order.

> 
> Or maybe I didn't understand the problem correctly?

If you mix devm and regular APIs it is nearly impossible to maintain the
proper order of releasing the resources.

I hope I was able to explain the issue above.

> 
> >
> > >
> > > >
> > > >
> > > > >
> > > > > As I have tried to argue before, I think devm_pm_runtime_enable()
> > > > > should *not* be used. Not here, not at all. Runtime PM isn't like any
> > > > > other resources that we fetch/release. Instead, it's a behaviour that
> > > > > you turn on and off, which needs to be managed more carefully, rather
> > > > > than relying on fetch/release ordering from devres.
> >
> > I disagree. It is a resource that you turn on and off, same as clocks,
> > regulators, interrupts, etc. We manage those during lifetime of the
> > device, disable them when going into low power mode/suspend, reenable
> > them upon resume, may disable and reenable them for other reasons.
> >
> > PM is not any more special here. As long as you keep the proper order of
> > operations it works as well.
> 
> How would you solve the issue I pointed out above?

The proper order is to disable runtime PM at the right time (which is
typically early enough as it is typically enabled late in probe()) so
there is no concern with "leaving it enabled". And again, the issue is
not limited to runtime PM, even devices that do not use runtime PM
should not be detached from their power domain too early.

> 
> >
> > > > >
> > > > > That said, I would convert the driver to use pm_runtime_enable() and
> > > > > pm_runtime_disable() instead.
> > > >
> > > > I've tried this approach previously but it resulted in more complicated
> > > > code and thus, Jonathan wasn't happy with it [1].
> > >
> > > I understand that you have been trying to move forward to address
> > > people's opinions. It's not always easy to keep everybody happy. :-)
> > >
> > > That said, I still think this is the most viable option as it's how
> > > the vast majority of drivers do it today. A few lines of additional
> > > code shouldn't really be a big problem in my opinion.
> >
> > Have you tried making such change? Again, you will need to abandon use
> > of most other devm APIs so that you keep the order of releasing
> > resources. The only devm that you can still use is devm_k*alloc(), the
> > rest has to be converted into unmanaged.
> 
> I guess I need to take a stab at this particular use case.
> 
> Looking closer, could it be that it's really the combination of
> turning on/off resources using devres (not just get/put if them) like
> clocks - and using devm_pm_runtime_enable()?

It is mixing devres and non-devres resources, yes. Not specifically
runtime PM.

Thanks.

-- 
Dmitry

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Claudiu Beznea]

Hi, Ulf,

On 20.05.2025 15:09, Ulf Hansson wrote:
> For example, even if the order is made correctly, suppose a driver's
> ->remove() callback completes by turning off the resources for its
> device and leaves runtime PM enabled, as it relies on devres to do it
> some point later. Beyond this point, nothing would prevent userspace
> for runtime resuming/suspending the device via sysfs. 

If I'm not wrong, that can't happen? The driver_sysfs_remove() is called
before device_remove() (which calls the driver remove) is called, this
being the call path:

device_driver_detach() ->
  device_release_driver_internal() ->
    __device_release_driver() ->
      driver_sysfs_remove()
      // ...
      device_remove()

And the driver_sysfs_remove() calls in the end __kernfs_remove() which
looks to me like the place that actually drops the entries from sysfs, this
being a call path for it:

driver_sysfs_remove() ->
  sysfs_remove_link() ->
    kernfs_remove_by_name() ->
      kernfs_remove_by_name_ns() ->
        __kernfs_remove() ->

activating the following line in __kernfs_remove():

pr_debug("kernfs %s: removing\n", kernfs_rcu_name(kn));

leads to the following prints when unbinding the watchdog device from its
watchdog driver (attached to platform bus) on my board:
https://p.fr33tux.org/935252

Thank you,
Claudiu

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Ulf Hansson]

On Wed, 21 May 2025 at 07:41, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>
> Hi, Ulf,
>
> On 20.05.2025 15:09, Ulf Hansson wrote:
> > For example, even if the order is made correctly, suppose a driver's
> > ->remove() callback completes by turning off the resources for its
> > device and leaves runtime PM enabled, as it relies on devres to do it
> > some point later. Beyond this point, nothing would prevent userspace
> > for runtime resuming/suspending the device via sysfs.
>
> If I'm not wrong, that can't happen? The driver_sysfs_remove() is called
> before device_remove() (which calls the driver remove) is called, this
> being the call path:
>
> device_driver_detach() ->
>   device_release_driver_internal() ->
>     __device_release_driver() ->
>       driver_sysfs_remove()
>       // ...
>       device_remove()
>
> And the driver_sysfs_remove() calls in the end __kernfs_remove() which
> looks to me like the place that actually drops the entries from sysfs, this
> being a call path for it:
>
> driver_sysfs_remove() ->
>   sysfs_remove_link() ->
>     kernfs_remove_by_name() ->
>       kernfs_remove_by_name_ns() ->
>         __kernfs_remove() ->
>
> activating the following line in __kernfs_remove():
>
> pr_debug("kernfs %s: removing\n", kernfs_rcu_name(kn));
>
> leads to the following prints when unbinding the watchdog device from its
> watchdog driver (attached to platform bus) on my board:
> https://p.fr33tux.org/935252

Indeed this is a very good point you make! I completely overlooked
this fact, thanks a lot for clarifying this!

However, my main point still stands.

In the end, there is nothing preventing rpm_suspend|resume|idle() in
drivers/base/power/runtime.c from running (don't forget runtime PM is
asynchronous too) for the device in question. This could lead to that
a ->runtime_suspend|resume|idle() callback becomes executed at any
point in time, as long as we haven't called pm_runtime_disable() for
the device.

That's why the devm_pm_runtime_enable() should be avoided as it simply
introduces a race-condition. Drivers need to be more careful and use
pm_runtime_enable|disable() explicitly to control the behaviour.

Kind regards
Uffe

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Dmitry Torokhov]

On Wed, May 21, 2025 at 02:37:08PM +0200, Ulf Hansson wrote:
> On Wed, 21 May 2025 at 07:41, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >
> > Hi, Ulf,
> >
> > On 20.05.2025 15:09, Ulf Hansson wrote:
> > > For example, even if the order is made correctly, suppose a driver's
> > > ->remove() callback completes by turning off the resources for its
> > > device and leaves runtime PM enabled, as it relies on devres to do it
> > > some point later. Beyond this point, nothing would prevent userspace
> > > for runtime resuming/suspending the device via sysfs.
> >
> > If I'm not wrong, that can't happen? The driver_sysfs_remove() is called
> > before device_remove() (which calls the driver remove) is called, this
> > being the call path:
> >
> > device_driver_detach() ->
> >   device_release_driver_internal() ->
> >     __device_release_driver() ->
> >       driver_sysfs_remove()
> >       // ...
> >       device_remove()
> >
> > And the driver_sysfs_remove() calls in the end __kernfs_remove() which
> > looks to me like the place that actually drops the entries from sysfs, this
> > being a call path for it:
> >
> > driver_sysfs_remove() ->
> >   sysfs_remove_link() ->
> >     kernfs_remove_by_name() ->
> >       kernfs_remove_by_name_ns() ->
> >         __kernfs_remove() ->
> >
> > activating the following line in __kernfs_remove():
> >
> > pr_debug("kernfs %s: removing\n", kernfs_rcu_name(kn));
> >
> > leads to the following prints when unbinding the watchdog device from its
> > watchdog driver (attached to platform bus) on my board:
> > https://p.fr33tux.org/935252
> 
> Indeed this is a very good point you make! I completely overlooked
> this fact, thanks a lot for clarifying this!
> 
> However, my main point still stands.
> 
> In the end, there is nothing preventing rpm_suspend|resume|idle() in
> drivers/base/power/runtime.c from running (don't forget runtime PM is
> asynchronous too) for the device in question. This could lead to that
> a ->runtime_suspend|resume|idle() callback becomes executed at any
> point in time, as long as we haven't called pm_runtime_disable() for
> the device.

So exactly the same may happen if you enter driver->remove() and
something calls runtime API before pm_runtime_disable() is called.
The driver has (as they should be doing currently) be prepared for this.

> 
> That's why the devm_pm_runtime_enable() should be avoided as it simply
> introduces a race-condition. Drivers need to be more careful and use
> pm_runtime_enable|disable() explicitly to control the behaviour.

You make it sound like we are dealing with some non-deterministic
process, like garbage collector, where runtime disable done by devm
happens at some unspecified point in the future. However we are dealing
with very well defined order of operations, all happening within
__device_release_driver() call. It is the same scope as when using
manual pm_runtime_disable(). Just the order is wrong, that is it.

Thanks.

-- 
Dmitry

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Claudiu Beznea]

Hi, Ulf,

On 21.05.2025 17:57, Dmitry Torokhov wrote:
> On Wed, May 21, 2025 at 02:37:08PM +0200, Ulf Hansson wrote:
>> On Wed, 21 May 2025 at 07:41, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>>
>>> Hi, Ulf,
>>>
>>> On 20.05.2025 15:09, Ulf Hansson wrote:
>>>> For example, even if the order is made correctly, suppose a driver's
>>>> ->remove() callback completes by turning off the resources for its
>>>> device and leaves runtime PM enabled, as it relies on devres to do it
>>>> some point later. Beyond this point, nothing would prevent userspace
>>>> for runtime resuming/suspending the device via sysfs.
>>>
>>> If I'm not wrong, that can't happen? The driver_sysfs_remove() is called
>>> before device_remove() (which calls the driver remove) is called, this
>>> being the call path:
>>>
>>> device_driver_detach() ->
>>>   device_release_driver_internal() ->
>>>     __device_release_driver() ->
>>>       driver_sysfs_remove()
>>>       // ...
>>>       device_remove()
>>>
>>> And the driver_sysfs_remove() calls in the end __kernfs_remove() which
>>> looks to me like the place that actually drops the entries from sysfs, this
>>> being a call path for it:
>>>
>>> driver_sysfs_remove() ->
>>>   sysfs_remove_link() ->
>>>     kernfs_remove_by_name() ->
>>>       kernfs_remove_by_name_ns() ->
>>>         __kernfs_remove() ->
>>>
>>> activating the following line in __kernfs_remove():
>>>
>>> pr_debug("kernfs %s: removing\n", kernfs_rcu_name(kn));
>>>
>>> leads to the following prints when unbinding the watchdog device from its
>>> watchdog driver (attached to platform bus) on my board:
>>> https://p.fr33tux.org/935252
>>
>> Indeed this is a very good point you make! I completely overlooked
>> this fact, thanks a lot for clarifying this!
>>
>> However, my main point still stands.
>>
>> In the end, there is nothing preventing rpm_suspend|resume|idle() in
>> drivers/base/power/runtime.c from running (don't forget runtime PM is
>> asynchronous too) for the device in question. This could lead to that
>> a ->runtime_suspend|resume|idle() callback becomes executed at any
>> point in time, as long as we haven't called pm_runtime_disable() for
>> the device.
> 
> So exactly the same may happen if you enter driver->remove() and
> something calls runtime API before pm_runtime_disable() is called.
> The driver has (as they should be doing currently) be prepared for this.

I took the time and tried to do a comparison of the current solutions
(describing the bad and good things I see), trying to understand your
concerns with regards to RPM suspend|resume|idle while unbinding a device
from its driver.

I see the following cases:

Case 1/ the current approach when devm_pm_runtime_enable() is used in
driver's ->probe() with the current code base:

- right after driver ->remove() finish its execution clocks are detached
  from the PM domain, through dev_pm_domain_detach() call in
  platform_remove()

- any subsequent RPM resume|suspend|idle will lead to failure if the driver
  specific RPM APIs access directly registers and counts on PM domain to
  enable/disable the clocks

- at this point, if the IRQs are shared (but not only) and devm requested
  the driver's IRQ handler can still be called asynchronously; driver
  should be prepared for such events and should be written to work for such
  scenarios; but as the clocks are not in the PM domain anymore and RPM is
  still enabled at this point, if the driver don't run runtime suspend on
  probe (and runtime resume/suspend on runtime), I think (because I haven't
  investigated this yet) it can't rely on pm_runtime_active()/
  pm_runtime_suspended() checks in interrupt handlers
  and can't decide if it can interrogate registers or not; interrogating
  should lead to failure at this stage as the clocks are disabled; drivers
  should work in such scenario and the CONFIG_DEBUG_SHIRQ is a way to check
  they can; I previously debugged a similar issue on drivers/net/ethernet/
  renesas/ravb driver where using devm_pm_runtime_enable() in probe and
  pm_runtime_suspended() checks in IRQ handlers was the way to make this
  scenario happy; at that time I wasn't able to find that
  dev_pm_domain_detach() have the impact discussed in this thread

Case 2/ What is proposed in this patch: devm_pm_runtime_enable() used +
open devres group after dev_pm_domain_attach() (in probe) and close the
devres group before dev_pm_domain_attach() (in remove):

- right after the driver ->remove() is executed only the driver allocated
  devres resources are freed; this happens before dev_pm_domain_deattach()
  is called, though the proposed devres_release_group() call in this patch

- while doing this, driver can still get async RPM suspend|resume|idle
  requests; is like the execution is in the driver ->remove()
  but the pm_runtime_disable() hasn't been called yet

- as the runtime PM is enabled in driver's ->probe() mostly after the HW is
  prepared to take requests and all the other devm resources are allocated,
  the RPM disable is going to be among the first things to be called by the
  devres_release_group()

- then, after RPM disable, all the devres resources allocated only in the
  driver's ->probe() are cleaned up in reverse order, just like
  device_unbind_cleanup() -> devres_release_all() call in
  __device_release_driver() is doing, but limited only to the resources
  allocated by the driver itself; I personally see this like manually
  allocating and freeing resources in the driver itself w/o relying on
  devres

- then it comes the turn of dev_pm_domain_detach() call in
  platform_remove(): at the time dev_pm_domain_detach() is executed the
  runtime PM is disabled and all the devres resources allocated by driver
  are freed as well

- after the dev_pm_domain_detach() is executed all the driver resources
  are cleaned up, the driver can't get IRQs as it's handler was already
  unregistered, no other user can execute rpm suspend|resume|idle
  as the RPM is disabled at this time

Case 3/ devm_pm_runtime_enabled() dropped and replaced by manual cleanup:
- the driver code is going be complicated, difficult to maintain and error
  prone

I may have missed considering things when describing the case 2 (which is
what is proposed by this patch) as I don't have the full picture behind the
dev_pm_domain_detach() call in platform bus remove. If so, please correct me.

Thank you,
Claudiu

> 
>>
>> That's why the devm_pm_runtime_enable() should be avoided as it simply
>> introduces a race-condition. Drivers need to be more careful and use
>> pm_runtime_enable|disable() explicitly to control the behaviour.
> 
> You make it sound like we are dealing with some non-deterministic
> process, like garbage collector, where runtime disable done by devm
> happens at some unspecified point in the future. However we are dealing
> with very well defined order of operations, all happening within
> __device_release_driver() call. It is the same scope as when using
> manual pm_runtime_disable(). Just the order is wrong, that is it.
> 
> Thanks.
> 

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Ulf Hansson]

On Thu, 22 May 2025 at 11:48, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>
> Hi, Ulf,
>
> On 21.05.2025 17:57, Dmitry Torokhov wrote:
> > On Wed, May 21, 2025 at 02:37:08PM +0200, Ulf Hansson wrote:
> >> On Wed, 21 May 2025 at 07:41, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >>>
> >>> Hi, Ulf,
> >>>
> >>> On 20.05.2025 15:09, Ulf Hansson wrote:
> >>>> For example, even if the order is made correctly, suppose a driver's
> >>>> ->remove() callback completes by turning off the resources for its
> >>>> device and leaves runtime PM enabled, as it relies on devres to do it
> >>>> some point later. Beyond this point, nothing would prevent userspace
> >>>> for runtime resuming/suspending the device via sysfs.
> >>>
> >>> If I'm not wrong, that can't happen? The driver_sysfs_remove() is called
> >>> before device_remove() (which calls the driver remove) is called, this
> >>> being the call path:
> >>>
> >>> device_driver_detach() ->
> >>>   device_release_driver_internal() ->
> >>>     __device_release_driver() ->
> >>>       driver_sysfs_remove()
> >>>       // ...
> >>>       device_remove()
> >>>
> >>> And the driver_sysfs_remove() calls in the end __kernfs_remove() which
> >>> looks to me like the place that actually drops the entries from sysfs, this
> >>> being a call path for it:
> >>>
> >>> driver_sysfs_remove() ->
> >>>   sysfs_remove_link() ->
> >>>     kernfs_remove_by_name() ->
> >>>       kernfs_remove_by_name_ns() ->
> >>>         __kernfs_remove() ->
> >>>
> >>> activating the following line in __kernfs_remove():
> >>>
> >>> pr_debug("kernfs %s: removing\n", kernfs_rcu_name(kn));
> >>>
> >>> leads to the following prints when unbinding the watchdog device from its
> >>> watchdog driver (attached to platform bus) on my board:
> >>> https://p.fr33tux.org/935252
> >>
> >> Indeed this is a very good point you make! I completely overlooked
> >> this fact, thanks a lot for clarifying this!
> >>
> >> However, my main point still stands.
> >>
> >> In the end, there is nothing preventing rpm_suspend|resume|idle() in
> >> drivers/base/power/runtime.c from running (don't forget runtime PM is
> >> asynchronous too) for the device in question. This could lead to that
> >> a ->runtime_suspend|resume|idle() callback becomes executed at any
> >> point in time, as long as we haven't called pm_runtime_disable() for
> >> the device.
> >
> > So exactly the same may happen if you enter driver->remove() and
> > something calls runtime API before pm_runtime_disable() is called.
> > The driver has (as they should be doing currently) be prepared for this.
>
> I took the time and tried to do a comparison of the current solutions
> (describing the bad and good things I see), trying to understand your
> concerns with regards to RPM suspend|resume|idle while unbinding a device
> from its driver.
>
> I see the following cases:
>
> Case 1/ the current approach when devm_pm_runtime_enable() is used in
> driver's ->probe() with the current code base:
>
> - right after driver ->remove() finish its execution clocks are detached
>   from the PM domain, through dev_pm_domain_detach() call in
>   platform_remove()
>
> - any subsequent RPM resume|suspend|idle will lead to failure if the driver
>   specific RPM APIs access directly registers and counts on PM domain to
>   enable/disable the clocks
>
> - at this point, if the IRQs are shared (but not only) and devm requested
>   the driver's IRQ handler can still be called asynchronously; driver
>   should be prepared for such events and should be written to work for such
>   scenarios; but as the clocks are not in the PM domain anymore and RPM is
>   still enabled at this point, if the driver don't run runtime suspend on
>   probe (and runtime resume/suspend on runtime), I think (because I haven't
>   investigated this yet) it can't rely on pm_runtime_active()/
>   pm_runtime_suspended() checks in interrupt handlers
>   and can't decide if it can interrogate registers or not; interrogating
>   should lead to failure at this stage as the clocks are disabled; drivers
>   should work in such scenario and the CONFIG_DEBUG_SHIRQ is a way to check
>   they can; I previously debugged a similar issue on drivers/net/ethernet/
>   renesas/ravb driver where using devm_pm_runtime_enable() in probe and
>   pm_runtime_suspended() checks in IRQ handlers was the way to make this
>   scenario happy; at that time I wasn't able to find that
>   dev_pm_domain_detach() have the impact discussed in this thread
>
> Case 2/ What is proposed in this patch: devm_pm_runtime_enable() used +
> open devres group after dev_pm_domain_attach() (in probe) and close the
> devres group before dev_pm_domain_attach() (in remove):
>
> - right after the driver ->remove() is executed only the driver allocated
>   devres resources are freed; this happens before dev_pm_domain_deattach()
>   is called, though the proposed devres_release_group() call in this patch
>
> - while doing this, driver can still get async RPM suspend|resume|idle
>   requests; is like the execution is in the driver ->remove()
>   but the pm_runtime_disable() hasn't been called yet
>
> - as the runtime PM is enabled in driver's ->probe() mostly after the HW is
>   prepared to take requests and all the other devm resources are allocated,
>   the RPM disable is going to be among the first things to be called by the
>   devres_release_group()
>
> - then, after RPM disable, all the devres resources allocated only in the
>   driver's ->probe() are cleaned up in reverse order, just like
>   device_unbind_cleanup() -> devres_release_all() call in
>   __device_release_driver() is doing, but limited only to the resources
>   allocated by the driver itself; I personally see this like manually
>   allocating and freeing resources in the driver itself w/o relying on
>   devres
>
> - then it comes the turn of dev_pm_domain_detach() call in
>   platform_remove(): at the time dev_pm_domain_detach() is executed the
>   runtime PM is disabled and all the devres resources allocated by driver
>   are freed as well
>
> - after the dev_pm_domain_detach() is executed all the driver resources
>   are cleaned up, the driver can't get IRQs as it's handler was already
>   unregistered, no other user can execute rpm suspend|resume|idle
>   as the RPM is disabled at this time
>
> Case 3/ devm_pm_runtime_enabled() dropped and replaced by manual cleanup:
> - the driver code is going be complicated, difficult to maintain and error
>   prone

Yes, the driver's code would become slightly more complicated, but
more importantly it would be correct.

To me it sounds like the driver's ->remove() callback could do this:

pm_runtime_get_sync()
pm_runtime_disable()
pm_runtime_put_noidle()

In this way, the driver will runtime resume its device, allowing
devres to drop/turn-off resources in the order we want. Except for the
clocks, as those would be turned off via dev_pm_domain_detach() before
the IRQ handler is freed (via devres), right?

To avoid getting the IRQ handler to be called when it can't access
registers, we could do one of the below:
*) Look for a condition in the IRQ handler and bail-out when we know
we should not manage IRQs. Is using pm_runtime_enabled() sufficient,
you think? Otherwise we need a driver specific flag, which should be
set in ->remove().
*) Don't use devm* when registering the IRQ handler.

Yes, both options further contribute to making the driver code
slightly more complicated, but if you want to solve the problem sooner
than later, I think this is what you need to do. Yet, I think there is
another option too, see below.

>
> I may have missed considering things when describing the case 2 (which is
> what is proposed by this patch) as I don't have the full picture behind the
> dev_pm_domain_detach() call in platform bus remove. If so, please correct me.

The dev_pm_domain_attach|detach() calls in bus level code
(probe/remove) were added there a long time ago, way before devres was
being used like today.

Currently we also have devm_pm_domain_attach_list(), which is used
when devices have multiple PM domains to attach too. This is *not*
called by bus-level code, but by the driver themselves. For these
cases, we would not encounter the problems you have been facing with
clocks/IRQ-handler, I think - because the devres order is maintained
for PM domains too.

That said, I think adding a devm_pm_domain_attach() interface would
make perfect sense. Then we can try to replace
dev_pm_domain_attach|detach() in bus level code, with just a call to
devm_pm_domain_attach(). In this way, we should preserve the
expectation for drivers around devres for PM domains. Even if it would
change the behaviour for some drivers, it still sounds like the
correct thing to do in my opinion.

[...]

Kind regards
Uffe

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Claudiu Beznea]

Hi, Ulf,

On 22.05.2025 14:53, Ulf Hansson wrote:
> On Thu, 22 May 2025 at 11:48, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>
>> Hi, Ulf,
>>
>> On 21.05.2025 17:57, Dmitry Torokhov wrote:
>>> On Wed, May 21, 2025 at 02:37:08PM +0200, Ulf Hansson wrote:
>>>> On Wed, 21 May 2025 at 07:41, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>>>>
>>>>> Hi, Ulf,
>>>>>
>>>>> On 20.05.2025 15:09, Ulf Hansson wrote:
>>>>>> For example, even if the order is made correctly, suppose a driver's
>>>>>> ->remove() callback completes by turning off the resources for its
>>>>>> device and leaves runtime PM enabled, as it relies on devres to do it
>>>>>> some point later. Beyond this point, nothing would prevent userspace
>>>>>> for runtime resuming/suspending the device via sysfs.
>>>>>
>>>>> If I'm not wrong, that can't happen? The driver_sysfs_remove() is called
>>>>> before device_remove() (which calls the driver remove) is called, this
>>>>> being the call path:
>>>>>
>>>>> device_driver_detach() ->
>>>>>   device_release_driver_internal() ->
>>>>>     __device_release_driver() ->
>>>>>       driver_sysfs_remove()
>>>>>       // ...
>>>>>       device_remove()
>>>>>
>>>>> And the driver_sysfs_remove() calls in the end __kernfs_remove() which
>>>>> looks to me like the place that actually drops the entries from sysfs, this
>>>>> being a call path for it:
>>>>>
>>>>> driver_sysfs_remove() ->
>>>>>   sysfs_remove_link() ->
>>>>>     kernfs_remove_by_name() ->
>>>>>       kernfs_remove_by_name_ns() ->
>>>>>         __kernfs_remove() ->
>>>>>
>>>>> activating the following line in __kernfs_remove():
>>>>>
>>>>> pr_debug("kernfs %s: removing\n", kernfs_rcu_name(kn));
>>>>>
>>>>> leads to the following prints when unbinding the watchdog device from its
>>>>> watchdog driver (attached to platform bus) on my board:
>>>>> https://p.fr33tux.org/935252
>>>>
>>>> Indeed this is a very good point you make! I completely overlooked
>>>> this fact, thanks a lot for clarifying this!
>>>>
>>>> However, my main point still stands.
>>>>
>>>> In the end, there is nothing preventing rpm_suspend|resume|idle() in
>>>> drivers/base/power/runtime.c from running (don't forget runtime PM is
>>>> asynchronous too) for the device in question. This could lead to that
>>>> a ->runtime_suspend|resume|idle() callback becomes executed at any
>>>> point in time, as long as we haven't called pm_runtime_disable() for
>>>> the device.
>>>
>>> So exactly the same may happen if you enter driver->remove() and
>>> something calls runtime API before pm_runtime_disable() is called.
>>> The driver has (as they should be doing currently) be prepared for this.
>>
>> I took the time and tried to do a comparison of the current solutions
>> (describing the bad and good things I see), trying to understand your
>> concerns with regards to RPM suspend|resume|idle while unbinding a device
>> from its driver.
>>
>> I see the following cases:
>>
>> Case 1/ the current approach when devm_pm_runtime_enable() is used in
>> driver's ->probe() with the current code base:
>>
>> - right after driver ->remove() finish its execution clocks are detached
>>   from the PM domain, through dev_pm_domain_detach() call in
>>   platform_remove()
>>
>> - any subsequent RPM resume|suspend|idle will lead to failure if the driver
>>   specific RPM APIs access directly registers and counts on PM domain to
>>   enable/disable the clocks
>>
>> - at this point, if the IRQs are shared (but not only) and devm requested
>>   the driver's IRQ handler can still be called asynchronously; driver
>>   should be prepared for such events and should be written to work for such
>>   scenarios; but as the clocks are not in the PM domain anymore and RPM is
>>   still enabled at this point, if the driver don't run runtime suspend on
>>   probe (and runtime resume/suspend on runtime), I think (because I haven't
>>   investigated this yet) it can't rely on pm_runtime_active()/
>>   pm_runtime_suspended() checks in interrupt handlers
>>   and can't decide if it can interrogate registers or not; interrogating
>>   should lead to failure at this stage as the clocks are disabled; drivers
>>   should work in such scenario and the CONFIG_DEBUG_SHIRQ is a way to check
>>   they can; I previously debugged a similar issue on drivers/net/ethernet/
>>   renesas/ravb driver where using devm_pm_runtime_enable() in probe and
>>   pm_runtime_suspended() checks in IRQ handlers was the way to make this
>>   scenario happy; at that time I wasn't able to find that
>>   dev_pm_domain_detach() have the impact discussed in this thread
>>
>> Case 2/ What is proposed in this patch: devm_pm_runtime_enable() used +
>> open devres group after dev_pm_domain_attach() (in probe) and close the
>> devres group before dev_pm_domain_attach() (in remove):
>>
>> - right after the driver ->remove() is executed only the driver allocated
>>   devres resources are freed; this happens before dev_pm_domain_deattach()
>>   is called, though the proposed devres_release_group() call in this patch
>>
>> - while doing this, driver can still get async RPM suspend|resume|idle
>>   requests; is like the execution is in the driver ->remove()
>>   but the pm_runtime_disable() hasn't been called yet
>>
>> - as the runtime PM is enabled in driver's ->probe() mostly after the HW is
>>   prepared to take requests and all the other devm resources are allocated,
>>   the RPM disable is going to be among the first things to be called by the
>>   devres_release_group()
>>
>> - then, after RPM disable, all the devres resources allocated only in the
>>   driver's ->probe() are cleaned up in reverse order, just like
>>   device_unbind_cleanup() -> devres_release_all() call in
>>   __device_release_driver() is doing, but limited only to the resources
>>   allocated by the driver itself; I personally see this like manually
>>   allocating and freeing resources in the driver itself w/o relying on
>>   devres
>>
>> - then it comes the turn of dev_pm_domain_detach() call in
>>   platform_remove(): at the time dev_pm_domain_detach() is executed the
>>   runtime PM is disabled and all the devres resources allocated by driver
>>   are freed as well
>>
>> - after the dev_pm_domain_detach() is executed all the driver resources
>>   are cleaned up, the driver can't get IRQs as it's handler was already
>>   unregistered, no other user can execute rpm suspend|resume|idle
>>   as the RPM is disabled at this time
>>
>> Case 3/ devm_pm_runtime_enabled() dropped and replaced by manual cleanup:
>> - the driver code is going be complicated, difficult to maintain and error
>>   prone
> 
> Yes, the driver's code would become slightly more complicated, but
> more importantly it would be correct.
> 
> To me it sounds like the driver's ->remove() callback could do this:
> 
> pm_runtime_get_sync()
> pm_runtime_disable()
> pm_runtime_put_noidle()

In my case it was just pm_runtime_disable() at the end of driver ->remove()
and pm_runtime_active() checks in IRQ handlers which didn't worked after
driver ->remove() finished execution due to disable_depth being incremented
in pm_runtime_disable(). The IRQs were devm requested.

The solution found at the that time was to use devm_pm_runtime_enable() in
probe and pm_runtime_suspended() calls in IRQ handlers.

> 
> In this way, the driver will runtime resume its device, allowing
> devres to drop/turn-off resources in the order we want.
> Except for the
> clocks, as those would be turned off via dev_pm_domain_detach() before
> the IRQ handler is freed (via devres), right?
> 
> To avoid getting the IRQ handler to be called when it can't access
> registers, we could do one of the below:
> *) Look for a condition in the IRQ handler and bail-out when we know
> we should not manage IRQs. Is using pm_runtime_enabled() sufficient,
> you think? Otherwise we need a driver specific flag, which should be
> set in ->remove().
> *) Don't use devm* when registering the IRQ handler.

That's true.

> 
> Yes, both options further contribute to making the driver code
> slightly more complicated, but if you want to solve the problem sooner
> than later, I think this is what you need to do. Yet, I think there is
> another option too, see below.
> 
>>
>> I may have missed considering things when describing the case 2 (which is
>> what is proposed by this patch) as I don't have the full picture behind the
>> dev_pm_domain_detach() call in platform bus remove. If so, please correct me.
> 
> The dev_pm_domain_attach|detach() calls in bus level code
> (probe/remove) were added there a long time ago, way before devres was
> being used like today.
> 
> Currently we also have devm_pm_domain_attach_list(), which is used
> when devices have multiple PM domains to attach too. This is *not*
> called by bus-level code, but by the driver themselves. For these
> cases, we would not encounter the problems you have been facing with
> clocks/IRQ-handler, I think - because the devres order is maintained
> for PM domains too.
> 
> That said, I think adding a devm_pm_domain_attach() interface would
> make perfect sense. Then we can try to replace
> dev_pm_domain_attach|detach() in bus level code, with just a call to
> devm_pm_domain_attach(). In this way, we should preserve the
> expectation for drivers around devres for PM domains. Even if it would
> change the behaviour for some drivers, it still sounds like the
> correct thing to do in my opinion.

This looks good to me, as well. I did prototype it on my side and tested on
all my failure cases and it works.

Thank you,
Claudiu

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Ulf Hansson]

On Thu, 22 May 2025 at 16:08, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>
> Hi, Ulf,
>
> On 22.05.2025 14:53, Ulf Hansson wrote:
> > On Thu, 22 May 2025 at 11:48, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >>
> >> Hi, Ulf,
> >>
> >> On 21.05.2025 17:57, Dmitry Torokhov wrote:
> >>> On Wed, May 21, 2025 at 02:37:08PM +0200, Ulf Hansson wrote:
> >>>> On Wed, 21 May 2025 at 07:41, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >>>>>
> >>>>> Hi, Ulf,
> >>>>>
> >>>>> On 20.05.2025 15:09, Ulf Hansson wrote:
> >>>>>> For example, even if the order is made correctly, suppose a driver's
> >>>>>> ->remove() callback completes by turning off the resources for its
> >>>>>> device and leaves runtime PM enabled, as it relies on devres to do it
> >>>>>> some point later. Beyond this point, nothing would prevent userspace
> >>>>>> for runtime resuming/suspending the device via sysfs.
> >>>>>
> >>>>> If I'm not wrong, that can't happen? The driver_sysfs_remove() is called
> >>>>> before device_remove() (which calls the driver remove) is called, this
> >>>>> being the call path:
> >>>>>
> >>>>> device_driver_detach() ->
> >>>>>   device_release_driver_internal() ->
> >>>>>     __device_release_driver() ->
> >>>>>       driver_sysfs_remove()
> >>>>>       // ...
> >>>>>       device_remove()
> >>>>>
> >>>>> And the driver_sysfs_remove() calls in the end __kernfs_remove() which
> >>>>> looks to me like the place that actually drops the entries from sysfs, this
> >>>>> being a call path for it:
> >>>>>
> >>>>> driver_sysfs_remove() ->
> >>>>>   sysfs_remove_link() ->
> >>>>>     kernfs_remove_by_name() ->
> >>>>>       kernfs_remove_by_name_ns() ->
> >>>>>         __kernfs_remove() ->
> >>>>>
> >>>>> activating the following line in __kernfs_remove():
> >>>>>
> >>>>> pr_debug("kernfs %s: removing\n", kernfs_rcu_name(kn));
> >>>>>
> >>>>> leads to the following prints when unbinding the watchdog device from its
> >>>>> watchdog driver (attached to platform bus) on my board:
> >>>>> https://p.fr33tux.org/935252
> >>>>
> >>>> Indeed this is a very good point you make! I completely overlooked
> >>>> this fact, thanks a lot for clarifying this!
> >>>>
> >>>> However, my main point still stands.
> >>>>
> >>>> In the end, there is nothing preventing rpm_suspend|resume|idle() in
> >>>> drivers/base/power/runtime.c from running (don't forget runtime PM is
> >>>> asynchronous too) for the device in question. This could lead to that
> >>>> a ->runtime_suspend|resume|idle() callback becomes executed at any
> >>>> point in time, as long as we haven't called pm_runtime_disable() for
> >>>> the device.
> >>>
> >>> So exactly the same may happen if you enter driver->remove() and
> >>> something calls runtime API before pm_runtime_disable() is called.
> >>> The driver has (as they should be doing currently) be prepared for this.
> >>
> >> I took the time and tried to do a comparison of the current solutions
> >> (describing the bad and good things I see), trying to understand your
> >> concerns with regards to RPM suspend|resume|idle while unbinding a device
> >> from its driver.
> >>
> >> I see the following cases:
> >>
> >> Case 1/ the current approach when devm_pm_runtime_enable() is used in
> >> driver's ->probe() with the current code base:
> >>
> >> - right after driver ->remove() finish its execution clocks are detached
> >>   from the PM domain, through dev_pm_domain_detach() call in
> >>   platform_remove()
> >>
> >> - any subsequent RPM resume|suspend|idle will lead to failure if the driver
> >>   specific RPM APIs access directly registers and counts on PM domain to
> >>   enable/disable the clocks
> >>
> >> - at this point, if the IRQs are shared (but not only) and devm requested
> >>   the driver's IRQ handler can still be called asynchronously; driver
> >>   should be prepared for such events and should be written to work for such
> >>   scenarios; but as the clocks are not in the PM domain anymore and RPM is
> >>   still enabled at this point, if the driver don't run runtime suspend on
> >>   probe (and runtime resume/suspend on runtime), I think (because I haven't
> >>   investigated this yet) it can't rely on pm_runtime_active()/
> >>   pm_runtime_suspended() checks in interrupt handlers
> >>   and can't decide if it can interrogate registers or not; interrogating
> >>   should lead to failure at this stage as the clocks are disabled; drivers
> >>   should work in such scenario and the CONFIG_DEBUG_SHIRQ is a way to check
> >>   they can; I previously debugged a similar issue on drivers/net/ethernet/
> >>   renesas/ravb driver where using devm_pm_runtime_enable() in probe and
> >>   pm_runtime_suspended() checks in IRQ handlers was the way to make this
> >>   scenario happy; at that time I wasn't able to find that
> >>   dev_pm_domain_detach() have the impact discussed in this thread
> >>
> >> Case 2/ What is proposed in this patch: devm_pm_runtime_enable() used +
> >> open devres group after dev_pm_domain_attach() (in probe) and close the
> >> devres group before dev_pm_domain_attach() (in remove):
> >>
> >> - right after the driver ->remove() is executed only the driver allocated
> >>   devres resources are freed; this happens before dev_pm_domain_deattach()
> >>   is called, though the proposed devres_release_group() call in this patch
> >>
> >> - while doing this, driver can still get async RPM suspend|resume|idle
> >>   requests; is like the execution is in the driver ->remove()
> >>   but the pm_runtime_disable() hasn't been called yet
> >>
> >> - as the runtime PM is enabled in driver's ->probe() mostly after the HW is
> >>   prepared to take requests and all the other devm resources are allocated,
> >>   the RPM disable is going to be among the first things to be called by the
> >>   devres_release_group()
> >>
> >> - then, after RPM disable, all the devres resources allocated only in the
> >>   driver's ->probe() are cleaned up in reverse order, just like
> >>   device_unbind_cleanup() -> devres_release_all() call in
> >>   __device_release_driver() is doing, but limited only to the resources
> >>   allocated by the driver itself; I personally see this like manually
> >>   allocating and freeing resources in the driver itself w/o relying on
> >>   devres
> >>
> >> - then it comes the turn of dev_pm_domain_detach() call in
> >>   platform_remove(): at the time dev_pm_domain_detach() is executed the
> >>   runtime PM is disabled and all the devres resources allocated by driver
> >>   are freed as well
> >>
> >> - after the dev_pm_domain_detach() is executed all the driver resources
> >>   are cleaned up, the driver can't get IRQs as it's handler was already
> >>   unregistered, no other user can execute rpm suspend|resume|idle
> >>   as the RPM is disabled at this time
> >>
> >> Case 3/ devm_pm_runtime_enabled() dropped and replaced by manual cleanup:
> >> - the driver code is going be complicated, difficult to maintain and error
> >>   prone
> >
> > Yes, the driver's code would become slightly more complicated, but
> > more importantly it would be correct.
> >
> > To me it sounds like the driver's ->remove() callback could do this:
> >
> > pm_runtime_get_sync()
> > pm_runtime_disable()
> > pm_runtime_put_noidle()
>
> In my case it was just pm_runtime_disable() at the end of driver ->remove()
> and pm_runtime_active() checks in IRQ handlers which didn't worked after
> driver ->remove() finished execution due to disable_depth being incremented
> in pm_runtime_disable(). The IRQs were devm requested.
>
> The solution found at the that time was to use devm_pm_runtime_enable() in
> probe and pm_runtime_suspended() calls in IRQ handlers.
>
> >
> > In this way, the driver will runtime resume its device, allowing
> > devres to drop/turn-off resources in the order we want.
> > Except for the
> > clocks, as those would be turned off via dev_pm_domain_detach() before
> > the IRQ handler is freed (via devres), right?
> >
> > To avoid getting the IRQ handler to be called when it can't access
> > registers, we could do one of the below:
> > *) Look for a condition in the IRQ handler and bail-out when we know
> > we should not manage IRQs. Is using pm_runtime_enabled() sufficient,
> > you think? Otherwise we need a driver specific flag, which should be
> > set in ->remove().
> > *) Don't use devm* when registering the IRQ handler.
>
> That's true.
>
> >
> > Yes, both options further contribute to making the driver code
> > slightly more complicated, but if you want to solve the problem sooner
> > than later, I think this is what you need to do. Yet, I think there is
> > another option too, see below.
> >
> >>
> >> I may have missed considering things when describing the case 2 (which is
> >> what is proposed by this patch) as I don't have the full picture behind the
> >> dev_pm_domain_detach() call in platform bus remove. If so, please correct me.
> >
> > The dev_pm_domain_attach|detach() calls in bus level code
> > (probe/remove) were added there a long time ago, way before devres was
> > being used like today.
> >
> > Currently we also have devm_pm_domain_attach_list(), which is used
> > when devices have multiple PM domains to attach too. This is *not*
> > called by bus-level code, but by the driver themselves. For these
> > cases, we would not encounter the problems you have been facing with
> > clocks/IRQ-handler, I think - because the devres order is maintained
> > for PM domains too.
> >
> > That said, I think adding a devm_pm_domain_attach() interface would
> > make perfect sense. Then we can try to replace
> > dev_pm_domain_attach|detach() in bus level code, with just a call to
> > devm_pm_domain_attach(). In this way, we should preserve the
> > expectation for drivers around devres for PM domains. Even if it would
> > change the behaviour for some drivers, it still sounds like the
> > correct thing to do in my opinion.
>
> This looks good to me, as well. I did prototype it on my side and tested on
> all my failure cases and it works.

That's great! I am happy to help review, if/when you decide to post it.

Kind regards
Uffe

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Dmitry Torokhov]

On Thu, May 22, 2025 at 06:28:44PM +0200, Ulf Hansson wrote:
> On Thu, 22 May 2025 at 16:08, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >
> > Hi, Ulf,
> >
> > On 22.05.2025 14:53, Ulf Hansson wrote:
> > >
> > > That said, I think adding a devm_pm_domain_attach() interface would
> > > make perfect sense. Then we can try to replace
> > > dev_pm_domain_attach|detach() in bus level code, with just a call to
> > > devm_pm_domain_attach(). In this way, we should preserve the
> > > expectation for drivers around devres for PM domains. Even if it would
> > > change the behaviour for some drivers, it still sounds like the
> > > correct thing to do in my opinion.
> >
> > This looks good to me, as well. I did prototype it on my side and tested on
> > all my failure cases and it works.
> 
> That's great! I am happy to help review, if/when you decide to post it.

So you are saying you'd be OK with essentially the following (with
devm_pm_domain_attach() actually being elsewhere in a real patch and not
necessarily mimicked by devm_add_action_or_reset()):

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index cfccf3ff36e7..1e017bfa5caf 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1376,6 +1376,27 @@ static int platform_uevent(const struct device *dev, struct kobj_uevent_env *env
 	return 0;
 }
 
+
+static void platform_pm_domain_detach(void *d)
+{
+	dev_pm_domain_detach(d, true);
+}
+
+static int devm_pm_domain_attach(struct device *dev)
+{
+	int error;
+
+	error = dev_pm_domain_attach(dev, true);
+	if (error)
+		return error;
+
+	error = devm_add_action_or_reset(dev, platform_pm_domain_detach, dev);
+	if (error)
+		return error;
+
+	return 0;
+}
+
 static int platform_probe(struct device *_dev)
 {
 	struct platform_driver *drv = to_platform_driver(_dev->driver);
@@ -1396,15 +1417,12 @@ static int platform_probe(struct device *_dev)
 	if (ret < 0)
 		return ret;
 
-	ret = dev_pm_domain_attach(_dev, true);
+	ret = devm_pm_domain_attach(_dev);
 	if (ret)
 		goto out;
 
-	if (drv->probe) {
+	if (drv->probe)
 		ret = drv->probe(dev);
-		if (ret)
-			dev_pm_domain_detach(_dev, true);
-	}
 
 out:
 	if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
@@ -1422,7 +1440,6 @@ static void platform_remove(struct device *_dev)
 
 	if (drv->remove)
 		drv->remove(dev);
-	dev_pm_domain_detach(_dev, true);
 }
 
 static void platform_shutdown(struct device *_dev)


If so, then OK, it will work for me as well. This achieves the
same behavior as with using devres group. The only difference is that if
we ever need to extend the platform bus to acquire/release more
resources they will also have to use devm API and not the regular one.

Thanks.

-- 
Dmitry

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Ulf Hansson]

On Thu, 22 May 2025 at 20:47, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> On Thu, May 22, 2025 at 06:28:44PM +0200, Ulf Hansson wrote:
> > On Thu, 22 May 2025 at 16:08, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > >
> > > Hi, Ulf,
> > >
> > > On 22.05.2025 14:53, Ulf Hansson wrote:
> > > >
> > > > That said, I think adding a devm_pm_domain_attach() interface would
> > > > make perfect sense. Then we can try to replace
> > > > dev_pm_domain_attach|detach() in bus level code, with just a call to
> > > > devm_pm_domain_attach(). In this way, we should preserve the
> > > > expectation for drivers around devres for PM domains. Even if it would
> > > > change the behaviour for some drivers, it still sounds like the
> > > > correct thing to do in my opinion.
> > >
> > > This looks good to me, as well. I did prototype it on my side and tested on
> > > all my failure cases and it works.
> >
> > That's great! I am happy to help review, if/when you decide to post it.
>
> So you are saying you'd be OK with essentially the following (with
> devm_pm_domain_attach() actually being elsewhere in a real patch and not
> necessarily mimicked by devm_add_action_or_reset()):

Correct!

>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index cfccf3ff36e7..1e017bfa5caf 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1376,6 +1376,27 @@ static int platform_uevent(const struct device *dev, struct kobj_uevent_env *env
>         return 0;
>  }
>
> +
> +static void platform_pm_domain_detach(void *d)
> +{
> +       dev_pm_domain_detach(d, true);
> +}

Well, I would not limit this to the platform bus, even if that is the
most widely used.

Let's add the new generic interface along with
dev_pm_domain_attach|detach* and friends instead.

Then we can convert bus level code (and others), such as the platform
bus to use it, in a step-by-step approach.

> +
> +static int devm_pm_domain_attach(struct device *dev)
> +{
> +       int error;
> +
> +       error = dev_pm_domain_attach(dev, true);
> +       if (error)
> +               return error;
> +
> +       error = devm_add_action_or_reset(dev, platform_pm_domain_detach, dev);
> +       if (error)
> +               return error;
> +
> +       return 0;
> +}
> +
>  static int platform_probe(struct device *_dev)
>  {
>         struct platform_driver *drv = to_platform_driver(_dev->driver);
> @@ -1396,15 +1417,12 @@ static int platform_probe(struct device *_dev)
>         if (ret < 0)
>                 return ret;
>
> -       ret = dev_pm_domain_attach(_dev, true);
> +       ret = devm_pm_domain_attach(_dev);
>         if (ret)
>                 goto out;
>
> -       if (drv->probe) {
> +       if (drv->probe)
>                 ret = drv->probe(dev);
> -               if (ret)
> -                       dev_pm_domain_detach(_dev, true);
> -       }
>
>  out:
>         if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> @@ -1422,7 +1440,6 @@ static void platform_remove(struct device *_dev)
>
>         if (drv->remove)
>                 drv->remove(dev);
> -       dev_pm_domain_detach(_dev, true);
>  }
>
>  static void platform_shutdown(struct device *_dev)
>
>
> If so, then OK, it will work for me as well. This achieves the
> same behavior as with using devres group. The only difference is that if
> we ever need to extend the platform bus to acquire/release more
> resources they will also have to use devm API and not the regular one.

Sounds reasonable to me! Thanks for a nice discussion!

When it comes to the devm_pm_runtime_enable() API, I think we
seriously should consider removing it. Let me have a closer look at
that.

Kind regards
Uffe

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Dmitry Torokhov]

On Fri, May 23, 2025 at 12:09:08AM +0200, Ulf Hansson wrote:
> On Thu, 22 May 2025 at 20:47, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >
> > On Thu, May 22, 2025 at 06:28:44PM +0200, Ulf Hansson wrote:
> > > On Thu, 22 May 2025 at 16:08, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > > >
> > > > Hi, Ulf,
> > > >
> > > > On 22.05.2025 14:53, Ulf Hansson wrote:
> > > > >
> > > > > That said, I think adding a devm_pm_domain_attach() interface would
> > > > > make perfect sense. Then we can try to replace
> > > > > dev_pm_domain_attach|detach() in bus level code, with just a call to
> > > > > devm_pm_domain_attach(). In this way, we should preserve the
> > > > > expectation for drivers around devres for PM domains. Even if it would
> > > > > change the behaviour for some drivers, it still sounds like the
> > > > > correct thing to do in my opinion.
> > > >
> > > > This looks good to me, as well. I did prototype it on my side and tested on
> > > > all my failure cases and it works.
> > >
> > > That's great! I am happy to help review, if/when you decide to post it.
> >
> > So you are saying you'd be OK with essentially the following (with
> > devm_pm_domain_attach() actually being elsewhere in a real patch and not
> > necessarily mimicked by devm_add_action_or_reset()):
> 
> Correct!
> 
> >
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index cfccf3ff36e7..1e017bfa5caf 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -1376,6 +1376,27 @@ static int platform_uevent(const struct device *dev, struct kobj_uevent_env *env
> >         return 0;
> >  }
> >
> > +
> > +static void platform_pm_domain_detach(void *d)
> > +{
> > +       dev_pm_domain_detach(d, true);
> > +}
> 
> Well, I would not limit this to the platform bus, even if that is the
> most widely used.
> 
> Let's add the new generic interface along with
> dev_pm_domain_attach|detach* and friends instead.
> 
> Then we can convert bus level code (and others), such as the platform
> bus to use it, in a step-by-step approach.

Right, this was only a draft:

"... with devm_pm_domain_attach() actually being elsewhere in a real
patch and not necessarily mimicked by devm_add_action_or_reset() ..."

> 
> > +
> > +static int devm_pm_domain_attach(struct device *dev)
> > +{
> > +       int error;
> > +
> > +       error = dev_pm_domain_attach(dev, true);
> > +       if (error)
> > +               return error;
> > +
> > +       error = devm_add_action_or_reset(dev, platform_pm_domain_detach, dev);
> > +       if (error)
> > +               return error;
> > +
> > +       return 0;
> > +}
> > +
> >  static int platform_probe(struct device *_dev)
> >  {
> >         struct platform_driver *drv = to_platform_driver(_dev->driver);
> > @@ -1396,15 +1417,12 @@ static int platform_probe(struct device *_dev)
> >         if (ret < 0)
> >                 return ret;
> >
> > -       ret = dev_pm_domain_attach(_dev, true);
> > +       ret = devm_pm_domain_attach(_dev);
> >         if (ret)
> >                 goto out;
> >
> > -       if (drv->probe) {
> > +       if (drv->probe)
> >                 ret = drv->probe(dev);
> > -               if (ret)
> > -                       dev_pm_domain_detach(_dev, true);
> > -       }
> >
> >  out:
> >         if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> > @@ -1422,7 +1440,6 @@ static void platform_remove(struct device *_dev)
> >
> >         if (drv->remove)
> >                 drv->remove(dev);
> > -       dev_pm_domain_detach(_dev, true);
> >  }
> >
> >  static void platform_shutdown(struct device *_dev)
> >
> >
> > If so, then OK, it will work for me as well. This achieves the
> > same behavior as with using devres group. The only difference is that if
> > we ever need to extend the platform bus to acquire/release more
> > resources they will also have to use devm API and not the regular one.
> 
> Sounds reasonable to me! Thanks for a nice discussion!
> 
> When it comes to the devm_pm_runtime_enable() API, I think we
> seriously should consider removing it. Let me have a closer look at
> that.

I think once we sort out the power domain detach being out of order with
regard to other devm-managed resources in bus code you need to analyze
this again and you will find out that much as with IRQs, devm API for
runtime PM is useful for majority of cases. Of course there will be
exceptions, but by and large it will cut down on boilerplate code.

Thanks.

-- 
Dmitry

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Ulf Hansson]

On Fri, 23 May 2025 at 01:06, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> On Fri, May 23, 2025 at 12:09:08AM +0200, Ulf Hansson wrote:
> > On Thu, 22 May 2025 at 20:47, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > >
> > > On Thu, May 22, 2025 at 06:28:44PM +0200, Ulf Hansson wrote:
> > > > On Thu, 22 May 2025 at 16:08, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > > > >
> > > > > Hi, Ulf,
> > > > >
> > > > > On 22.05.2025 14:53, Ulf Hansson wrote:
> > > > > >
> > > > > > That said, I think adding a devm_pm_domain_attach() interface would
> > > > > > make perfect sense. Then we can try to replace
> > > > > > dev_pm_domain_attach|detach() in bus level code, with just a call to
> > > > > > devm_pm_domain_attach(). In this way, we should preserve the
> > > > > > expectation for drivers around devres for PM domains. Even if it would
> > > > > > change the behaviour for some drivers, it still sounds like the
> > > > > > correct thing to do in my opinion.
> > > > >
> > > > > This looks good to me, as well. I did prototype it on my side and tested on
> > > > > all my failure cases and it works.
> > > >
> > > > That's great! I am happy to help review, if/when you decide to post it.
> > >
> > > So you are saying you'd be OK with essentially the following (with
> > > devm_pm_domain_attach() actually being elsewhere in a real patch and not
> > > necessarily mimicked by devm_add_action_or_reset()):
> >
> > Correct!
> >
> > >
> > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > > index cfccf3ff36e7..1e017bfa5caf 100644
> > > --- a/drivers/base/platform.c
> > > +++ b/drivers/base/platform.c
> > > @@ -1376,6 +1376,27 @@ static int platform_uevent(const struct device *dev, struct kobj_uevent_env *env
> > >         return 0;
> > >  }
> > >
> > > +
> > > +static void platform_pm_domain_detach(void *d)
> > > +{
> > > +       dev_pm_domain_detach(d, true);
> > > +}
> >
> > Well, I would not limit this to the platform bus, even if that is the
> > most widely used.
> >
> > Let's add the new generic interface along with
> > dev_pm_domain_attach|detach* and friends instead.
> >
> > Then we can convert bus level code (and others), such as the platform
> > bus to use it, in a step-by-step approach.
>
> Right, this was only a draft:
>
> "... with devm_pm_domain_attach() actually being elsewhere in a real
> patch and not necessarily mimicked by devm_add_action_or_reset() ..."
>
> >
> > > +
> > > +static int devm_pm_domain_attach(struct device *dev)
> > > +{
> > > +       int error;
> > > +
> > > +       error = dev_pm_domain_attach(dev, true);
> > > +       if (error)
> > > +               return error;
> > > +
> > > +       error = devm_add_action_or_reset(dev, platform_pm_domain_detach, dev);
> > > +       if (error)
> > > +               return error;
> > > +
> > > +       return 0;
> > > +}
> > > +
> > >  static int platform_probe(struct device *_dev)
> > >  {
> > >         struct platform_driver *drv = to_platform_driver(_dev->driver);
> > > @@ -1396,15 +1417,12 @@ static int platform_probe(struct device *_dev)
> > >         if (ret < 0)
> > >                 return ret;
> > >
> > > -       ret = dev_pm_domain_attach(_dev, true);
> > > +       ret = devm_pm_domain_attach(_dev);
> > >         if (ret)
> > >                 goto out;
> > >
> > > -       if (drv->probe) {
> > > +       if (drv->probe)
> > >                 ret = drv->probe(dev);
> > > -               if (ret)
> > > -                       dev_pm_domain_detach(_dev, true);
> > > -       }
> > >
> > >  out:
> > >         if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> > > @@ -1422,7 +1440,6 @@ static void platform_remove(struct device *_dev)
> > >
> > >         if (drv->remove)
> > >                 drv->remove(dev);
> > > -       dev_pm_domain_detach(_dev, true);
> > >  }
> > >
> > >  static void platform_shutdown(struct device *_dev)
> > >
> > >
> > > If so, then OK, it will work for me as well. This achieves the
> > > same behavior as with using devres group. The only difference is that if
> > > we ever need to extend the platform bus to acquire/release more
> > > resources they will also have to use devm API and not the regular one.
> >
> > Sounds reasonable to me! Thanks for a nice discussion!
> >
> > When it comes to the devm_pm_runtime_enable() API, I think we
> > seriously should consider removing it. Let me have a closer look at
> > that.
>
> I think once we sort out the power domain detach being out of order with
> regard to other devm-managed resources in bus code you need to analyze
> this again and you will find out that much as with IRQs, devm API for
> runtime PM is useful for majority of cases. Of course there will be
> exceptions, but by and large it will cut down on boilerplate code.

Well, the problem is that the interface is just too difficult to
understand how to use correctly.

A quick look for deployments in drivers confirms my worries.

Kind regards
Uffe

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Claudiu Beznea]

Hi, Ulf,

On 23.05.2025 12:47, Ulf Hansson wrote:
> On Fri, 23 May 2025 at 01:06, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>>
>> On Fri, May 23, 2025 at 12:09:08AM +0200, Ulf Hansson wrote:
>>> On Thu, 22 May 2025 at 20:47, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>>>>
>>>> On Thu, May 22, 2025 at 06:28:44PM +0200, Ulf Hansson wrote:
>>>>> On Thu, 22 May 2025 at 16:08, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>>>>>>
>>>>>> Hi, Ulf,
>>>>>>
>>>>>> On 22.05.2025 14:53, Ulf Hansson wrote:
>>>>>>>
>>>>>>> That said, I think adding a devm_pm_domain_attach() interface would
>>>>>>> make perfect sense. Then we can try to replace
>>>>>>> dev_pm_domain_attach|detach() in bus level code, with just a call to
>>>>>>> devm_pm_domain_attach(). In this way, we should preserve the
>>>>>>> expectation for drivers around devres for PM domains. Even if it would
>>>>>>> change the behaviour for some drivers, it still sounds like the
>>>>>>> correct thing to do in my opinion.
>>>>>>
>>>>>> This looks good to me, as well. I did prototype it on my side and tested on
>>>>>> all my failure cases and it works.
>>>>>
>>>>> That's great! I am happy to help review, if/when you decide to post it.
>>>>
>>>> So you are saying you'd be OK with essentially the following (with
>>>> devm_pm_domain_attach() actually being elsewhere in a real patch and not
>>>> necessarily mimicked by devm_add_action_or_reset()):
>>>
>>> Correct!
>>>
>>>>
>>>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>>>> index cfccf3ff36e7..1e017bfa5caf 100644
>>>> --- a/drivers/base/platform.c
>>>> +++ b/drivers/base/platform.c
>>>> @@ -1376,6 +1376,27 @@ static int platform_uevent(const struct device *dev, struct kobj_uevent_env *env
>>>>         return 0;
>>>>  }
>>>>
>>>> +
>>>> +static void platform_pm_domain_detach(void *d)
>>>> +{
>>>> +       dev_pm_domain_detach(d, true);
>>>> +}
>>>
>>> Well, I would not limit this to the platform bus, even if that is the
>>> most widely used.
>>>
>>> Let's add the new generic interface along with
>>> dev_pm_domain_attach|detach* and friends instead.
>>>
>>> Then we can convert bus level code (and others), such as the platform
>>> bus to use it, in a step-by-step approach.
>>
>> Right, this was only a draft:
>>
>> "... with devm_pm_domain_attach() actually being elsewhere in a real
>> patch and not necessarily mimicked by devm_add_action_or_reset() ..."
>>
>>>
>>>> +
>>>> +static int devm_pm_domain_attach(struct device *dev)
>>>> +{
>>>> +       int error;
>>>> +
>>>> +       error = dev_pm_domain_attach(dev, true);
>>>> +       if (error)
>>>> +               return error;
>>>> +
>>>> +       error = devm_add_action_or_reset(dev, platform_pm_domain_detach, dev);
>>>> +       if (error)
>>>> +               return error;
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>>  static int platform_probe(struct device *_dev)
>>>>  {
>>>>         struct platform_driver *drv = to_platform_driver(_dev->driver);
>>>> @@ -1396,15 +1417,12 @@ static int platform_probe(struct device *_dev)
>>>>         if (ret < 0)
>>>>                 return ret;
>>>>
>>>> -       ret = dev_pm_domain_attach(_dev, true);
>>>> +       ret = devm_pm_domain_attach(_dev);
>>>>         if (ret)
>>>>                 goto out;
>>>>
>>>> -       if (drv->probe) {
>>>> +       if (drv->probe)
>>>>                 ret = drv->probe(dev);
>>>> -               if (ret)
>>>> -                       dev_pm_domain_detach(_dev, true);
>>>> -       }
>>>>
>>>>  out:
>>>>         if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
>>>> @@ -1422,7 +1440,6 @@ static void platform_remove(struct device *_dev)
>>>>
>>>>         if (drv->remove)
>>>>                 drv->remove(dev);
>>>> -       dev_pm_domain_detach(_dev, true);
>>>>  }
>>>>
>>>>  static void platform_shutdown(struct device *_dev)
>>>>
>>>>
>>>> If so, then OK, it will work for me as well. This achieves the
>>>> same behavior as with using devres group. The only difference is that if
>>>> we ever need to extend the platform bus to acquire/release more
>>>> resources they will also have to use devm API and not the regular one.
>>>
>>> Sounds reasonable to me! Thanks for a nice discussion!
>>>
>>> When it comes to the devm_pm_runtime_enable() API, I think we
>>> seriously should consider removing it. Let me have a closer look at
>>> that.
>>
>> I think once we sort out the power domain detach being out of order with
>> regard to other devm-managed resources in bus code you need to analyze
>> this again and you will find out that much as with IRQs, devm API for
>> runtime PM is useful for majority of cases. Of course there will be
>> exceptions, but by and large it will cut down on boilerplate code.
> 
> Well, the problem is that the interface is just too difficult to
> understand how to use correctly.
> 
> A quick look for deployments in drivers confirms my worries.

Maybe we can add something like:

diff --git a/MAINTAINERS b/MAINTAINERS
index 96e64f3d7b47..568a8307863b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10100,6 +10100,7 @@ F:
Documentation/devicetree/bindings/power/power?domain*
 T:     git git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/linux-pm.git
 F:     drivers/pmdomain/
 F:     include/linux/pm_domain.h
+K:      \bpm_runtime_\w+\b

in MAINTAINERS file so that any new patch using the RPM will also be sent
to PM maintainers and checked accordingly?

Thank you,
Claudiu

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Ulf Hansson]

On Fri, 23 May 2025 at 12:52, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>
> Hi, Ulf,
>
> On 23.05.2025 12:47, Ulf Hansson wrote:
> > On Fri, 23 May 2025 at 01:06, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >>
> >> On Fri, May 23, 2025 at 12:09:08AM +0200, Ulf Hansson wrote:
> >>> On Thu, 22 May 2025 at 20:47, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> >>>>
> >>>> On Thu, May 22, 2025 at 06:28:44PM +0200, Ulf Hansson wrote:
> >>>>> On Thu, 22 May 2025 at 16:08, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >>>>>>
> >>>>>> Hi, Ulf,
> >>>>>>
> >>>>>> On 22.05.2025 14:53, Ulf Hansson wrote:
> >>>>>>>
> >>>>>>> That said, I think adding a devm_pm_domain_attach() interface would
> >>>>>>> make perfect sense. Then we can try to replace
> >>>>>>> dev_pm_domain_attach|detach() in bus level code, with just a call to
> >>>>>>> devm_pm_domain_attach(). In this way, we should preserve the
> >>>>>>> expectation for drivers around devres for PM domains. Even if it would
> >>>>>>> change the behaviour for some drivers, it still sounds like the
> >>>>>>> correct thing to do in my opinion.
> >>>>>>
> >>>>>> This looks good to me, as well. I did prototype it on my side and tested on
> >>>>>> all my failure cases and it works.
> >>>>>
> >>>>> That's great! I am happy to help review, if/when you decide to post it.
> >>>>
> >>>> So you are saying you'd be OK with essentially the following (with
> >>>> devm_pm_domain_attach() actually being elsewhere in a real patch and not
> >>>> necessarily mimicked by devm_add_action_or_reset()):
> >>>
> >>> Correct!
> >>>
> >>>>
> >>>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> >>>> index cfccf3ff36e7..1e017bfa5caf 100644
> >>>> --- a/drivers/base/platform.c
> >>>> +++ b/drivers/base/platform.c
> >>>> @@ -1376,6 +1376,27 @@ static int platform_uevent(const struct device *dev, struct kobj_uevent_env *env
> >>>>         return 0;
> >>>>  }
> >>>>
> >>>> +
> >>>> +static void platform_pm_domain_detach(void *d)
> >>>> +{
> >>>> +       dev_pm_domain_detach(d, true);
> >>>> +}
> >>>
> >>> Well, I would not limit this to the platform bus, even if that is the
> >>> most widely used.
> >>>
> >>> Let's add the new generic interface along with
> >>> dev_pm_domain_attach|detach* and friends instead.
> >>>
> >>> Then we can convert bus level code (and others), such as the platform
> >>> bus to use it, in a step-by-step approach.
> >>
> >> Right, this was only a draft:
> >>
> >> "... with devm_pm_domain_attach() actually being elsewhere in a real
> >> patch and not necessarily mimicked by devm_add_action_or_reset() ..."
> >>
> >>>
> >>>> +
> >>>> +static int devm_pm_domain_attach(struct device *dev)
> >>>> +{
> >>>> +       int error;
> >>>> +
> >>>> +       error = dev_pm_domain_attach(dev, true);
> >>>> +       if (error)
> >>>> +               return error;
> >>>> +
> >>>> +       error = devm_add_action_or_reset(dev, platform_pm_domain_detach, dev);
> >>>> +       if (error)
> >>>> +               return error;
> >>>> +
> >>>> +       return 0;
> >>>> +}
> >>>> +
> >>>>  static int platform_probe(struct device *_dev)
> >>>>  {
> >>>>         struct platform_driver *drv = to_platform_driver(_dev->driver);
> >>>> @@ -1396,15 +1417,12 @@ static int platform_probe(struct device *_dev)
> >>>>         if (ret < 0)
> >>>>                 return ret;
> >>>>
> >>>> -       ret = dev_pm_domain_attach(_dev, true);
> >>>> +       ret = devm_pm_domain_attach(_dev);
> >>>>         if (ret)
> >>>>                 goto out;
> >>>>
> >>>> -       if (drv->probe) {
> >>>> +       if (drv->probe)
> >>>>                 ret = drv->probe(dev);
> >>>> -               if (ret)
> >>>> -                       dev_pm_domain_detach(_dev, true);
> >>>> -       }
> >>>>
> >>>>  out:
> >>>>         if (drv->prevent_deferred_probe && ret == -EPROBE_DEFER) {
> >>>> @@ -1422,7 +1440,6 @@ static void platform_remove(struct device *_dev)
> >>>>
> >>>>         if (drv->remove)
> >>>>                 drv->remove(dev);
> >>>> -       dev_pm_domain_detach(_dev, true);
> >>>>  }
> >>>>
> >>>>  static void platform_shutdown(struct device *_dev)
> >>>>
> >>>>
> >>>> If so, then OK, it will work for me as well. This achieves the
> >>>> same behavior as with using devres group. The only difference is that if
> >>>> we ever need to extend the platform bus to acquire/release more
> >>>> resources they will also have to use devm API and not the regular one.
> >>>
> >>> Sounds reasonable to me! Thanks for a nice discussion!
> >>>
> >>> When it comes to the devm_pm_runtime_enable() API, I think we
> >>> seriously should consider removing it. Let me have a closer look at
> >>> that.
> >>
> >> I think once we sort out the power domain detach being out of order with
> >> regard to other devm-managed resources in bus code you need to analyze
> >> this again and you will find out that much as with IRQs, devm API for
> >> runtime PM is useful for majority of cases. Of course there will be
> >> exceptions, but by and large it will cut down on boilerplate code.
> >
> > Well, the problem is that the interface is just too difficult to
> > understand how to use correctly.
> >
> > A quick look for deployments in drivers confirms my worries.
>
> Maybe we can add something like:
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 96e64f3d7b47..568a8307863b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10100,6 +10100,7 @@ F:
> Documentation/devicetree/bindings/power/power?domain*
>  T:     git git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/linux-pm.git
>  F:     drivers/pmdomain/
>  F:     include/linux/pm_domain.h
> +K:      \bpm_runtime_\w+\b
>
> in MAINTAINERS file so that any new patch using the RPM will also be sent
> to PM maintainers and checked accordingly?

Well, I like the idea, but I am worried that it may be too much for me
to review. :-)

Although, perhaps I should help Rafael, more officially, to helpt
review code under "POWER MANAGEMENT CORE". Runtime PM is part of it.

Rafael, what do you think?

Kind regards
Uffe

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Geert Uytterhoeven]

Hi Ulf,

On Thu, 22 May 2025 at 13:54, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On Thu, 22 May 2025 at 11:48, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > I may have missed considering things when describing the case 2 (which is
> > what is proposed by this patch) as I don't have the full picture behind the
> > dev_pm_domain_detach() call in platform bus remove. If so, please correct me.
>
> The dev_pm_domain_attach|detach() calls in bus level code
> (probe/remove) were added there a long time ago, way before devres was
> being used like today.
>
> Currently we also have devm_pm_domain_attach_list(), which is used
> when devices have multiple PM domains to attach too. This is *not*
> called by bus-level code, but by the driver themselves. For these
> cases, we would not encounter the problems you have been facing with
> clocks/IRQ-handler, I think - because the devres order is maintained
> for PM domains too.
>
> That said, I think adding a devm_pm_domain_attach() interface would
> make perfect sense. Then we can try to replace
> dev_pm_domain_attach|detach() in bus level code, with just a call to
> devm_pm_domain_attach(). In this way, we should preserve the
> expectation for drivers around devres for PM domains. Even if it would
> change the behaviour for some drivers, it still sounds like the
> correct thing to do in my opinion.

IMO that sounds like going in the wrong direction.  Why would a driver
need to care if the device it manages is not located in a PM domain,
located in a single PM domain, or located in multiple PM domains?
All of this depends on SoC integration, not on the device that's
being driven. The nice thing about doing all this in the bus level
code is that it is abstracted away for the device driver (modulo using
pm_runtime_*() calls).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Ulf Hansson]

On Thu, 22 May 2025 at 16:56, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Ulf,
>
> On Thu, 22 May 2025 at 13:54, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > On Thu, 22 May 2025 at 11:48, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > > I may have missed considering things when describing the case 2 (which is
> > > what is proposed by this patch) as I don't have the full picture behind the
> > > dev_pm_domain_detach() call in platform bus remove. If so, please correct me.
> >
> > The dev_pm_domain_attach|detach() calls in bus level code
> > (probe/remove) were added there a long time ago, way before devres was
> > being used like today.
> >
> > Currently we also have devm_pm_domain_attach_list(), which is used
> > when devices have multiple PM domains to attach too. This is *not*
> > called by bus-level code, but by the driver themselves. For these
> > cases, we would not encounter the problems you have been facing with
> > clocks/IRQ-handler, I think - because the devres order is maintained
> > for PM domains too.
> >
> > That said, I think adding a devm_pm_domain_attach() interface would
> > make perfect sense. Then we can try to replace
> > dev_pm_domain_attach|detach() in bus level code, with just a call to
> > devm_pm_domain_attach(). In this way, we should preserve the
> > expectation for drivers around devres for PM domains. Even if it would
> > change the behaviour for some drivers, it still sounds like the
> > correct thing to do in my opinion.
>
> IMO that sounds like going in the wrong direction.  Why would a driver
> need to care if the device it manages is not located in a PM domain,
> located in a single PM domain, or located in multiple PM domains?

Before we added support for multiple PM domains for a device, it was
more or less transparent for drivers. It still is, as long as there
are not multiple PM domains to consider for the device in question.

The problem with making this transparent/common for the multiple PM
domain case too, was in principle that it could not be done flexible
enough from bus level code. It was discussed at conferences and at
LKML, sorry but I don't have the references to those chats at hand.
Anyway, that was the conclusion back when we introduced this around
2018. See the below commits:

82e12d9e0bd5 PM / Domains: Add dev_pm_domain_attach_by_id() to manage
multi PM domains
3c095f32a92b PM / Domains: Add support for multi PM domains per device to genpd

> All of this depends on SoC integration, not on the device that's
> being driven. The nice thing about doing all this in the bus level
> code is that it is abstracted away for the device driver (modulo using
> pm_runtime_*() calls).

Right, I would have been nice, but we couldn't make it work back then.

[...]

Kind regards
Uffe

Re: [PATCH] driver core: platform: Use devres group to free driver probe resources

[Ulf Hansson]

On Wed, 21 May 2025 at 16:58, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
> On Wed, May 21, 2025 at 02:37:08PM +0200, Ulf Hansson wrote:
> > On Wed, 21 May 2025 at 07:41, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > >
> > > Hi, Ulf,
> > >
> > > On 20.05.2025 15:09, Ulf Hansson wrote:
> > > > For example, even if the order is made correctly, suppose a driver's
> > > > ->remove() callback completes by turning off the resources for its
> > > > device and leaves runtime PM enabled, as it relies on devres to do it
> > > > some point later. Beyond this point, nothing would prevent userspace
> > > > for runtime resuming/suspending the device via sysfs.
> > >
> > > If I'm not wrong, that can't happen? The driver_sysfs_remove() is called
> > > before device_remove() (which calls the driver remove) is called, this
> > > being the call path:
> > >
> > > device_driver_detach() ->
> > >   device_release_driver_internal() ->
> > >     __device_release_driver() ->
> > >       driver_sysfs_remove()
> > >       // ...
> > >       device_remove()
> > >
> > > And the driver_sysfs_remove() calls in the end __kernfs_remove() which
> > > looks to me like the place that actually drops the entries from sysfs, this
> > > being a call path for it:
> > >
> > > driver_sysfs_remove() ->
> > >   sysfs_remove_link() ->
> > >     kernfs_remove_by_name() ->
> > >       kernfs_remove_by_name_ns() ->
> > >         __kernfs_remove() ->
> > >
> > > activating the following line in __kernfs_remove():
> > >
> > > pr_debug("kernfs %s: removing\n", kernfs_rcu_name(kn));
> > >
> > > leads to the following prints when unbinding the watchdog device from its
> > > watchdog driver (attached to platform bus) on my board:
> > > https://p.fr33tux.org/935252
> >
> > Indeed this is a very good point you make! I completely overlooked
> > this fact, thanks a lot for clarifying this!
> >
> > However, my main point still stands.
> >
> > In the end, there is nothing preventing rpm_suspend|resume|idle() in
> > drivers/base/power/runtime.c from running (don't forget runtime PM is
> > asynchronous too) for the device in question. This could lead to that
> > a ->runtime_suspend|resume|idle() callback becomes executed at any
> > point in time, as long as we haven't called pm_runtime_disable() for
> > the device.
>
> So exactly the same may happen if you enter driver->remove() and
> something calls runtime API before pm_runtime_disable() is called.
> The driver has (as they should be doing currently) be prepared for this.
>
> >
> > That's why the devm_pm_runtime_enable() should be avoided as it simply
> > introduces a race-condition. Drivers need to be more careful and use
> > pm_runtime_enable|disable() explicitly to control the behaviour.
>
> You make it sound like we are dealing with some non-deterministic
> process, like garbage collector, where runtime disable done by devm
> happens at some unspecified point in the future. However we are dealing
> with very well defined order of operations, all happening within
> __device_release_driver() call. It is the same scope as when using
> manual pm_runtime_disable(). Just the order is wrong, that is it.

I understand that devres is deterministic, the order to manage things
is ofcourse specified how we use it during ->probe() etc. My apologies
if it has sounded different to you.

What I have been trying to say is, because how the runtime PM works,
drivers must be careful about calling pm_runtime_enable|disable() as
relying on the order from devres is in many cases not sufficient.

Kind regards
Uffe