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

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

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.
>