[1/2] iio: adc: rzg2l_adc: Drop devm_pm_runtime_enable()

Commit Message

Claudiu Beznea Jan. 3, 2025, 2 p.m. UTC

From: Claudiu Beznea <[e-mail]>

On all systems where the rzg2l_adc driver is used, the ADC clocks are part
of a PM domain. The code that implements the PM domains support is in
drivers/clk/renesas/rzg2l-cpg.c, the functions of interest for this commit
being rzg2l_cpg_attach_dev() and rzg2l_cpg_deattach_dev(). The PM
domains support is registered with GENPD_FLAG_PM_CLK which, according to
the documentation, instructs genpd to use the PM clk framework while
powering on/off attached devices.

During probe, the ADC device is attached to the PM domain
controlling the ADC clocks. Similarly, during removal, the ADC device is
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 ADC 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 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 runtime resume the ADC device. However,
at the point it is called, the ADC device is no longer part of the PM
domain (which manages the ADC clocks). Since the rzg2l_adc runtime PM
APIs directly modifies hardware registers, the
rzg2l_adc_pm_runtime_resume() function is invoked without the ADC clocks
being enabled. This is because the PM domain no longer resumes along with
the ADC device. As a result, this leads to system aborts.

Drop the devres API for runtime PM enable.

Fixes: 89ee8174e8c8 ("iio: adc: rzg2l_adc: Simplify the runtime PM code")
Signed-off-by: Claudiu Beznea <[e-mail]>
---
 drivers/iio/adc/rzg2l_adc.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

Comments

Jonathan Cameron Jan. 4, 2025, 1:52 p.m. UTC | #1

On Fri,  3 Jan 2025 16:00:41 +0200
Claudiu <[e-mail]> wrote:

> From: Claudiu Beznea <[e-mail]>
+CC Rafael and linux-pm

> 
> On all systems where the rzg2l_adc driver is used, the ADC clocks are part
> of a PM domain. The code that implements the PM domains support is in
> drivers/clk/renesas/rzg2l-cpg.c, the functions of interest for this commit
> being rzg2l_cpg_attach_dev() and rzg2l_cpg_deattach_dev(). The PM
> domains support is registered with GENPD_FLAG_PM_CLK which, according to
> the documentation, instructs genpd to use the PM clk framework while
> powering on/off attached devices.
> 
> During probe, the ADC device is attached to the PM domain
> controlling the ADC clocks. Similarly, during removal, the ADC device is
> 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 ADC 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 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() ->

So is the only real difference that in the code below you disable runtime pm
before autosuspend?  Can you still do that with a devm callback just not
the standard one?


>     __pm_runtime_use_autosuspend() ->
>       update_autosuspend() ->
>         rpm_idle()
> 
> The rpm_idle() function attempts to runtime resume the ADC device.

Can you give a little more on that path. I'm not immediately spotting
how rpm_idle() is causing a resume

> However,
> at the point it is called, the ADC device is no longer part of the PM
> domain (which manages the ADC clocks). Since the rzg2l_adc runtime PM
> APIs directly modifies hardware registers, the
> rzg2l_adc_pm_runtime_resume() function is invoked without the ADC clocks
> being enabled. This is because the PM domain no longer resumes along with
> the ADC device. As a result, this leads to system aborts.
> 
> Drop the devres API for runtime PM enable.
> 
> Fixes: 89ee8174e8c8 ("iio: adc: rzg2l_adc: Simplify the runtime PM code")
> Signed-off-by: Claudiu Beznea <[e-mail]>

See below. I'm doubtful in general about the sequence changes and
specifically you can't just remove one devm callback from a driver without
modifying a lot of other code / leaving really fragile ordering.

Jonathan

> ---
>  drivers/iio/adc/rzg2l_adc.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> index 883c167c0670..f12f3daf08cc 100644
> --- a/drivers/iio/adc/rzg2l_adc.c
> +++ b/drivers/iio/adc/rzg2l_adc.c
> @@ -464,25 +464,26 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>  
>  	pm_runtime_set_autosuspend_delay(dev, 300);
>  	pm_runtime_use_autosuspend(dev);
> -	ret = devm_pm_runtime_enable(dev);
> -	if (ret)
> -		return ret;
> +	pm_runtime_enable(dev);
>  
>  	platform_set_drvdata(pdev, indio_dev);
>  
>  	ret = rzg2l_adc_hw_init(dev, adc);
> -	if (ret)
> -		return dev_err_probe(&pdev->dev, ret,
> -				     "failed to initialize ADC HW\n");
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "failed to initialize ADC HW\n");
> +		goto rpm_disable;
> +	}
>  
>  	irq = platform_get_irq(pdev, 0);
> -	if (irq < 0)
> -		return irq;
> +	if (irq < 0) {
> +		ret = irq;
> +		goto rpm_disable;
> +	}
>  
>  	ret = devm_request_irq(dev, irq, rzg2l_adc_isr,
>  			       0, dev_name(dev), adc);
>  	if (ret < 0)
> -		return ret;
> +		goto rpm_disable;
>  
>  	init_completion(&adc->completion);
>  
> @@ -493,6 +494,19 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>  	indio_dev->num_channels = adc->data->num_channels;
>  
>  	return devm_iio_device_register(dev, indio_dev);
> +
> +rpm_disable:
> +	pm_runtime_disable(dev);
> +	pm_runtime_dont_use_autosuspend(dev);
> +	return ret;
If you have to move away from devm you must do it for all calls after
the first thing that is manually cleaned up.
As you have it here the userspace interfaces are left available at a point
well after power down.

> +}
> +
> +static void rzg2l_adc_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	pm_runtime_disable(dev);
> +	pm_runtime_dont_use_autosuspend(dev);
>  }
>  
>  static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
> @@ -614,6 +628,7 @@ static const struct dev_pm_ops rzg2l_adc_pm_ops = {
>  
>  static struct platform_driver rzg2l_adc_driver = {
>  	.probe		= rzg2l_adc_probe,
> +	.remove		= rzg2l_adc_remove,
>  	.driver		= {
>  		.name		= DRIVER_NAME,
>  		.of_match_table = rzg2l_adc_match,

Claudiu Beznea Jan. 6, 2025, 9:18 a.m. UTC | #2

Hi, Jonathan,


On 04.01.2025 15:52, Jonathan Cameron wrote:
> On Fri,  3 Jan 2025 16:00:41 +0200
> Claudiu <[e-mail]> wrote:
> 
>> From: Claudiu Beznea <[e-mail]>
> +CC Rafael and linux-pm
> 
>>
>> On all systems where the rzg2l_adc driver is used, the ADC clocks are part
>> of a PM domain. The code that implements the PM domains support is in
>> drivers/clk/renesas/rzg2l-cpg.c, the functions of interest for this commit
>> being rzg2l_cpg_attach_dev() and rzg2l_cpg_deattach_dev(). The PM
>> domains support is registered with GENPD_FLAG_PM_CLK which, according to
>> the documentation, instructs genpd to use the PM clk framework while
>> powering on/off attached devices.
>>
>> During probe, the ADC device is attached to the PM domain
>> controlling the ADC clocks. Similarly, during removal, the ADC device is
>> 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 ADC 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 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() ->
> 
> So is the only real difference that in the code below you disable runtime pm
> before autosuspend?

No, the difference is that now, the driver specific runtime PM APIs are not
called anymore (through the pointed call stack) after the ADC was removed
from it's PM domain.


>  Can you still do that with a devm callback just not
> the standard one?

No. It doesn't matter if we call the standard devm callback or driver
specific one. As long as it is devm it will impact us as long as the driver
specific runtime PM APIs are called through devres_release_all() after
dev_pm_domain_detach(). And at that time the PM domain may be off along
with its clocks disabled.

> 
> 
>>     __pm_runtime_use_autosuspend() ->
>>       update_autosuspend() ->
>>         rpm_idle()
>>
>> The rpm_idle() function attempts to runtime resume the ADC device.
> 
> Can you give a little more on that path. I'm not immediately spotting
> how rpm_idle() is causing a resume

It is not in particular about the resume. Runtime suspend/resume after
devres_release_all() will be affected.

In particular, the rpm_idle() can call rpm_suspend() (called on the out
label of rpm_idle()) and rpm_suspend() may call the driver specific
runtime_suspend API through the following code (from the rpm_suspend()
function):

        callback = RPM_GET_CALLBACK(dev, runtime_suspend);



        dev_pm_enable_wake_irq_check(dev, true);

        retval = rpm_callback(callback, dev);

        if (retval)

                goto fail;



The full stack generated when running:
# cd /sys/bus/platform/drivers/rzg2l-adc
# while :; do echo 10058000.adc > unbind ; echo 10058000.adc > bind; done

is as follows:

[   24.801195] Call trace:
[   24.803633]  rzg2l_adc_pm_runtime_suspend+0x18/0x54 (P)
[   24.808847]  pm_generic_runtime_suspend+0x2c/0x44 (L)
[   24.813887]  pm_generic_runtime_suspend+0x2c/0x44
[   24.818580]  __rpm_callback+0x48/0x198
[   24.822319]  rpm_callback+0x68/0x74
[   24.825798]  rpm_suspend+0x100/0x578
[   24.829362]  rpm_idle+0xd0/0x17c
[   24.832582]  update_autosuspend+0x30/0xc4
[   24.836580]  pm_runtime_disable_action+0x40/0x64
[   24.841184]  devm_action_release+0x14/0x20
[   24.845274]  devres_release_all+0xa0/0x100
[   24.849361]  device_unbind_cleanup+0x18/0x60
[   24.853618]  device_release_driver_internal+0x1ec/0x228
[   24.858828]  device_driver_detach+0x18/0x24
[   24.862998]  unbind_store+0xb4/0xb8
[   24.866478]  drv_attr_store+0x24/0x38
[   24.870135]  sysfs_kf_write+0x44/0x54
[   24.873795]  kernfs_fop_write_iter+0x118/0x1a8
[   24.878229]  vfs_write+0x2ac/0x358
[   24.881627]  ksys_write+0x68/0xfc
[   24.884934]  __arm64_sys_write+0x1c/0x28
[   24.888846]  invoke_syscall+0x48/0x110
[   24.892592]  el0_svc_common.constprop.0+0xc0/0xe0
[   24.897285]  do_el0_svc+0x1c/0x28
[   24.900593]  el0_svc+0x30/0xd0
[   24.903647]  el0t_64_sync_handler+0xc8/0xcc
[   24.907821]  el0t_64_sync+0x198/0x19c
[   24.911481] Code: 910003fd f9403c00 f941bc01 f9400020 (b9400000)


Digging it further today: on the Renesas RZ/G3S we implement the power
domain on/off and we register the PM domain with GENPD_FLAG_PM_CLK. The
on/off PM domain functionality is implemented though the clock controller
MSTOP functionality which blocks the bus access to the specific IP (in this
particular case to the ADC).

The issue is reproducible when doing:
# cd /sys/bus/platform/drivers/rzg2l-adc
# while :; do echo 10058000.adc > unbind ; echo 10058000.adc > bind; done

I noticed today that doing single manual unbind+bind doesn't always leads
to aborts. It may be related to the fact that, as I noticed, the genpd
power off is called asynchronously as a work (through
genpd_power_off_work_fn()).

I also noticed today what when there is no on/off functionality implemented
on the PM domain we have no failures (as the MSTOP is not implemented and
the bus access to the specific IP is not blocked as there is no PM domain
on/off available).



> 
>> However,
>> at the point it is called, the ADC device is no longer part of the PM
>> domain (which manages the ADC clocks). Since the rzg2l_adc runtime PM
>> APIs directly modifies hardware registers, the
>> rzg2l_adc_pm_runtime_resume() function is invoked without the ADC clocks
>> being enabled. This is because the PM domain no longer resumes along with
>> the ADC device. As a result, this leads to system aborts.
>>
>> Drop the devres API for runtime PM enable.
>>
>> Fixes: 89ee8174e8c8 ("iio: adc: rzg2l_adc: Simplify the runtime PM code")
>> Signed-off-by: Claudiu Beznea <[e-mail]>
> 
> See below. I'm doubtful in general about the sequence changes and
> specifically you can't just remove one devm callback from a driver without
> modifying a lot of other code / leaving really fragile ordering.
> 
> Jonathan
> 
>> ---
>>  drivers/iio/adc/rzg2l_adc.c | 33 ++++++++++++++++++++++++---------
>>  1 file changed, 24 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
>> index 883c167c0670..f12f3daf08cc 100644
>> --- a/drivers/iio/adc/rzg2l_adc.c
>> +++ b/drivers/iio/adc/rzg2l_adc.c
>> @@ -464,25 +464,26 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>>  
>>  	pm_runtime_set_autosuspend_delay(dev, 300);
>>  	pm_runtime_use_autosuspend(dev);
>> -	ret = devm_pm_runtime_enable(dev);
>> -	if (ret)
>> -		return ret;
>> +	pm_runtime_enable(dev);
>>  
>>  	platform_set_drvdata(pdev, indio_dev);
>>  
>>  	ret = rzg2l_adc_hw_init(dev, adc);
>> -	if (ret)
>> -		return dev_err_probe(&pdev->dev, ret,
>> -				     "failed to initialize ADC HW\n");
>> +	if (ret) {
>> +		dev_err_probe(&pdev->dev, ret, "failed to initialize ADC HW\n");
>> +		goto rpm_disable;
>> +	}
>>  
>>  	irq = platform_get_irq(pdev, 0);
>> -	if (irq < 0)
>> -		return irq;
>> +	if (irq < 0) {
>> +		ret = irq;
>> +		goto rpm_disable;
>> +	}
>>  
>>  	ret = devm_request_irq(dev, irq, rzg2l_adc_isr,
>>  			       0, dev_name(dev), adc);
>>  	if (ret < 0)
>> -		return ret;
>> +		goto rpm_disable;
>>  
>>  	init_completion(&adc->completion);
>>  
>> @@ -493,6 +494,19 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>>  	indio_dev->num_channels = adc->data->num_channels;
>>  
>>  	return devm_iio_device_register(dev, indio_dev);
>> +
>> +rpm_disable:
>> +	pm_runtime_disable(dev);
>> +	pm_runtime_dont_use_autosuspend(dev);
>> +	return ret;
> If you have to move away from devm you must do it for all calls after
> the first thing that is manually cleaned up.
> As you have it here the userspace interfaces are left available at a point
> well after power down.

I see, thank you for pointing it.

And thank you for checking this,
Claudiu

> 
>> +}
>> +
>> +static void rzg2l_adc_remove(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +
>> +	pm_runtime_disable(dev);
>> +	pm_runtime_dont_use_autosuspend(dev);
>>  }
>>  
>>  static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
>> @@ -614,6 +628,7 @@ static const struct dev_pm_ops rzg2l_adc_pm_ops = {
>>  
>>  static struct platform_driver rzg2l_adc_driver = {
>>  	.probe		= rzg2l_adc_probe,
>> +	.remove		= rzg2l_adc_remove,
>>  	.driver		= {
>>  		.name		= DRIVER_NAME,
>>  		.of_match_table = rzg2l_adc_match,
>

Jonathan Cameron Jan. 11, 2025, 1:14 p.m. UTC | #3

On Mon, 6 Jan 2025 11:18:41 +0200
Claudiu Beznea <[e-mail]> wrote:

> Hi, Jonathan,
> 
> 
> On 04.01.2025 15:52, Jonathan Cameron wrote:
> > On Fri,  3 Jan 2025 16:00:41 +0200
> > Claudiu <[e-mail]> wrote:
> >   
> >> From: Claudiu Beznea <[e-mail]>  
> > +CC Rafael and linux-pm
> >   
> >>
> >> On all systems where the rzg2l_adc driver is used, the ADC clocks are part
> >> of a PM domain. The code that implements the PM domains support is in
> >> drivers/clk/renesas/rzg2l-cpg.c, the functions of interest for this commit
> >> being rzg2l_cpg_attach_dev() and rzg2l_cpg_deattach_dev(). The PM
> >> domains support is registered with GENPD_FLAG_PM_CLK which, according to
> >> the documentation, instructs genpd to use the PM clk framework while
> >> powering on/off attached devices.
> >>
> >> During probe, the ADC device is attached to the PM domain
> >> controlling the ADC clocks. Similarly, during removal, the ADC device is
> >> 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 ADC 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 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() ->  
> > 
> > So is the only real difference that in the code below you disable runtime pm
> > before autosuspend?  
> 
> No, the difference is that now, the driver specific runtime PM APIs are not
> called anymore (through the pointed call stack) after the ADC was removed
> from it's PM domain.

By my reading they are only not called now because you turn autosuspend off
after disabling runtime PM.

> 
> 
> >  Can you still do that with a devm callback just not
> > the standard one?  
> 
> No. It doesn't matter if we call the standard devm callback or driver
> specific one. As long as it is devm it will impact us as long as the driver
> specific runtime PM APIs are called through devres_release_all() after
> dev_pm_domain_detach(). And at that time the PM domain may be off along
> with its clocks disabled.

As above, I think that this is only the case because of the reordering
of those two calls, not something more fundamental.

In driver remove flow, device_unbind_cleanup9() is called
just after device_remove() which is calling the dev->driver_remove()
callback. There are no runtime pm related calls in between that I can see.

So you are moving these calls a little earlier but not in a fashion that
seems to have any involvement with anything else.


Call stack being
device_release_driver_internal()
-> __device_release_driver()
  -> device_remove() where you are now calling the disables
   .. some dma stuff
  -> device_unbind_cleanup() where the calling of disables previously was.

other than that unrelated DMA stuff there is nothing between the
two calls. 

There is runtime PM stuff before any of this, but not in the crucial
portion this code affects.

So I am thinking the only change that actually matters is the trivial
reorder mentioned above.





> 
> > 
> >   
> >>     __pm_runtime_use_autosuspend() ->
> >>       update_autosuspend() ->
> >>         rpm_idle()
> >>
> >> The rpm_idle() function attempts to runtime resume the ADC device.  
> > 
> > Can you give a little more on that path. I'm not immediately spotting
> > how rpm_idle() is causing a resume  
> 
> It is not in particular about the resume. Runtime suspend/resume after
> devres_release_all() will be affected.
> 
> In particular, the rpm_idle() can call rpm_suspend() (called on the out
> label of rpm_idle()) and rpm_suspend() may call the driver specific
> runtime_suspend API through the following code (from the rpm_suspend()
> function):
> 
>         callback = RPM_GET_CALLBACK(dev, runtime_suspend);
> 
> 
> 
>         dev_pm_enable_wake_irq_check(dev, true);
> 
>         retval = rpm_callback(callback, dev);
> 
>         if (retval)
> 
>                 goto fail;
> 
> 
> 
> The full stack generated when running:
> # cd /sys/bus/platform/drivers/rzg2l-adc
> # while :; do echo 10058000.adc > unbind ; echo 10058000.adc > bind; done
> 
> is as follows:
> 
> [   24.801195] Call trace:
> [   24.803633]  rzg2l_adc_pm_runtime_suspend+0x18/0x54 (P)
> [   24.808847]  pm_generic_runtime_suspend+0x2c/0x44 (L)
> [   24.813887]  pm_generic_runtime_suspend+0x2c/0x44
> [   24.818580]  __rpm_callback+0x48/0x198
> [   24.822319]  rpm_callback+0x68/0x74
> [   24.825798]  rpm_suspend+0x100/0x578
> [   24.829362]  rpm_idle+0xd0/0x17c
> [   24.832582]  update_autosuspend+0x30/0xc4
> [   24.836580]  pm_runtime_disable_action+0x40/0x64
> [   24.841184]  devm_action_release+0x14/0x20
> [   24.845274]  devres_release_all+0xa0/0x100
> [   24.849361]  device_unbind_cleanup+0x18/0x60
> [   24.853618]  device_release_driver_internal+0x1ec/0x228
> [   24.858828]  device_driver_detach+0x18/0x24
> [   24.862998]  unbind_store+0xb4/0xb8
> [   24.866478]  drv_attr_store+0x24/0x38
> [   24.870135]  sysfs_kf_write+0x44/0x54
> [   24.873795]  kernfs_fop_write_iter+0x118/0x1a8
> [   24.878229]  vfs_write+0x2ac/0x358
> [   24.881627]  ksys_write+0x68/0xfc
> [   24.884934]  __arm64_sys_write+0x1c/0x28
> [   24.888846]  invoke_syscall+0x48/0x110
> [   24.892592]  el0_svc_common.constprop.0+0xc0/0xe0
> [   24.897285]  do_el0_svc+0x1c/0x28
> [   24.900593]  el0_svc+0x30/0xd0
> [   24.903647]  el0t_64_sync_handler+0xc8/0xcc
> [   24.907821]  el0t_64_sync+0x198/0x19c
> [   24.911481] Code: 910003fd f9403c00 f941bc01 f9400020 (b9400000)

Thanks, that was helpful.

> 
> 
> Digging it further today: on the Renesas RZ/G3S we implement the power
> domain on/off and we register the PM domain with GENPD_FLAG_PM_CLK. The
> on/off PM domain functionality is implemented though the clock controller
> MSTOP functionality which blocks the bus access to the specific IP (in this
> particular case to the ADC).
> 
> The issue is reproducible when doing:
> # cd /sys/bus/platform/drivers/rzg2l-adc
> # while :; do echo 10058000.adc > unbind ; echo 10058000.adc > bind; done
> 
> I noticed today that doing single manual unbind+bind doesn't always leads
> to aborts. It may be related to the fact that, as I noticed, the genpd
> power off is called asynchronously as a work (through
> genpd_power_off_work_fn()).
> 
> I also noticed today what when there is no on/off functionality implemented
> on the PM domain we have no failures (as the MSTOP is not implemented and
> the bus access to the specific IP is not blocked as there is no PM domain
> on/off available).

The PM domain stuff is only called later in device_unbind_cleanup()
so I don't see the relevance. All the code you are modifying is
done before that happens.

Jonathan


> 
> 
> 
> >   
> >> However,
> >> at the point it is called, the ADC device is no longer part of the PM
> >> domain (which manages the ADC clocks). Since the rzg2l_adc runtime PM
> >> APIs directly modifies hardware registers, the
> >> rzg2l_adc_pm_runtime_resume() function is invoked without the ADC clocks
> >> being enabled. This is because the PM domain no longer resumes along with
> >> the ADC device. As a result, this leads to system aborts.
> >>
> >> Drop the devres API for runtime PM enable.
> >>
> >> Fixes: 89ee8174e8c8 ("iio: adc: rzg2l_adc: Simplify the runtime PM code")
> >> Signed-off-by: Claudiu Beznea <[e-mail]>  
> > 
> > See below. I'm doubtful in general about the sequence changes and
> > specifically you can't just remove one devm callback from a driver without
> > modifying a lot of other code / leaving really fragile ordering.
> > 
> > Jonathan
> >   
> >> ---
> >>  drivers/iio/adc/rzg2l_adc.c | 33 ++++++++++++++++++++++++---------
> >>  1 file changed, 24 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> >> index 883c167c0670..f12f3daf08cc 100644
> >> --- a/drivers/iio/adc/rzg2l_adc.c
> >> +++ b/drivers/iio/adc/rzg2l_adc.c
> >> @@ -464,25 +464,26 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
> >>  
> >>  	pm_runtime_set_autosuspend_delay(dev, 300);
> >>  	pm_runtime_use_autosuspend(dev);
> >> -	ret = devm_pm_runtime_enable(dev);
> >> -	if (ret)
> >> -		return ret;
> >> +	pm_runtime_enable(dev);
> >>  
> >>  	platform_set_drvdata(pdev, indio_dev);
> >>  
> >>  	ret = rzg2l_adc_hw_init(dev, adc);
> >> -	if (ret)
> >> -		return dev_err_probe(&pdev->dev, ret,
> >> -				     "failed to initialize ADC HW\n");
> >> +	if (ret) {
> >> +		dev_err_probe(&pdev->dev, ret, "failed to initialize ADC HW\n");
> >> +		goto rpm_disable;
> >> +	}
> >>  
> >>  	irq = platform_get_irq(pdev, 0);
> >> -	if (irq < 0)
> >> -		return irq;
> >> +	if (irq < 0) {
> >> +		ret = irq;
> >> +		goto rpm_disable;
> >> +	}
> >>  
> >>  	ret = devm_request_irq(dev, irq, rzg2l_adc_isr,
> >>  			       0, dev_name(dev), adc);
> >>  	if (ret < 0)
> >> -		return ret;
> >> +		goto rpm_disable;
> >>  
> >>  	init_completion(&adc->completion);
> >>  
> >> @@ -493,6 +494,19 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
> >>  	indio_dev->num_channels = adc->data->num_channels;
> >>  
> >>  	return devm_iio_device_register(dev, indio_dev);
> >> +
> >> +rpm_disable:
> >> +	pm_runtime_disable(dev);
> >> +	pm_runtime_dont_use_autosuspend(dev);
> >> +	return ret;  
> > If you have to move away from devm you must do it for all calls after
> > the first thing that is manually cleaned up.
> > As you have it here the userspace interfaces are left available at a point
> > well after power down.  
> 
> I see, thank you for pointing it.
> 
> And thank you for checking this,
> Claudiu
> 
> >   
> >> +}
> >> +
> >> +static void rzg2l_adc_remove(struct platform_device *pdev)
> >> +{
> >> +	struct device *dev = &pdev->dev;
> >> +
> >> +	pm_runtime_disable(dev);
> >> +	pm_runtime_dont_use_autosuspend(dev);
> >>  }
> >>  
> >>  static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
> >> @@ -614,6 +628,7 @@ static const struct dev_pm_ops rzg2l_adc_pm_ops = {
> >>  
> >>  static struct platform_driver rzg2l_adc_driver = {
> >>  	.probe		= rzg2l_adc_probe,
> >> +	.remove		= rzg2l_adc_remove,
> >>  	.driver		= {
> >>  		.name		= DRIVER_NAME,
> >>  		.of_match_table = rzg2l_adc_match,  
> >   
> 
>

Claudiu Beznea Jan. 15, 2025, 1:37 p.m. UTC | #4

Hi, Jonathan,

Thank you for your input!

On 11.01.2025 15:14, Jonathan Cameron wrote:
> On Mon, 6 Jan 2025 11:18:41 +0200
> Claudiu Beznea <[e-mail]> wrote:
> 
>> Hi, Jonathan,
>>
>>
>> On 04.01.2025 15:52, Jonathan Cameron wrote:
>>> On Fri,  3 Jan 2025 16:00:41 +0200
>>> Claudiu <[e-mail]> wrote:
>>>   
>>>> From: Claudiu Beznea <[e-mail]>  
>>> +CC Rafael and linux-pm
>>>   
>>>>
>>>> On all systems where the rzg2l_adc driver is used, the ADC clocks are part
>>>> of a PM domain. The code that implements the PM domains support is in
>>>> drivers/clk/renesas/rzg2l-cpg.c, the functions of interest for this commit
>>>> being rzg2l_cpg_attach_dev() and rzg2l_cpg_deattach_dev(). The PM
>>>> domains support is registered with GENPD_FLAG_PM_CLK which, according to
>>>> the documentation, instructs genpd to use the PM clk framework while
>>>> powering on/off attached devices.
>>>>
>>>> During probe, the ADC device is attached to the PM domain
>>>> controlling the ADC clocks. Similarly, during removal, the ADC device is
>>>> 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 ADC 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 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() ->  
>>>
>>> So is the only real difference that in the code below you disable runtime pm
>>> before autosuspend?  
>>
>> No, the difference is that now, the driver specific runtime PM APIs are not
>> called anymore (through the pointed call stack) after the ADC was removed
>> from it's PM domain.
> 
> By my reading they are only not called now because you turn autosuspend off
> after disabling runtime PM.

Sorry, I wanted to say that the runtime PM APIs are not called anymore from
devm_action_release(), though this call stack:

[   24.801195] Call trace:
[   24.803633]  rzg2l_adc_pm_runtime_suspend+0x18/0x54 (P)
[   24.808847]  pm_generic_runtime_suspend+0x2c/0x44 (L)
[   24.813887]  pm_generic_runtime_suspend+0x2c/0x44
[   24.818580]  __rpm_callback+0x48/0x198
[   24.822319]  rpm_callback+0x68/0x74
[   24.825798]  rpm_suspend+0x100/0x578
[   24.829362]  rpm_idle+0xd0/0x17c
[   24.832582]  update_autosuspend+0x30/0xc4
[   24.836580]  pm_runtime_disable_action+0x40/0x64
[   24.841184]  devm_action_release+0x14/0x20
[   24.845274]  devres_release_all+0xa0/0x100
[   24.849361]  device_unbind_cleanup+0x18/0x60

This is because I dropped the devm_pm_runtime_enable() which registers the
pm_runtime_disable_action(), which is called at the time the
device_unbind_cleanup() is called, which is called when the ADC is not
anymore part of its PM domain.

If I change the order in remove function proposed in this patch, thus do:

+static void rzg2l_adc_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	pm_runtime_dont_use_autosuspend(dev);
+	pm_runtime_disable(dev);
 }

nothing changes with the behavior of this patch. There will be no issue if
the device is runtime suspended/resumed through the
pm_runtime_dont_use_autosuspend() because at the time the
rzg2l_adc_remove() is called the ADC is still part of the PM domain.



> 
>>
>>
>>>  Can you still do that with a devm callback just not
>>> the standard one?  
>>
>> No. It doesn't matter if we call the standard devm callback or driver
>> specific one. As long as it is devm it will impact us as long as the driver
>> specific runtime PM APIs are called through devres_release_all() after
>> dev_pm_domain_detach(). And at that time the PM domain may be off along
>> with its clocks disabled.
> 
> As above, I think that this is only the case because of the reordering
> of those two calls, not something more fundamental.

I tried having a local devm function (the following diff applied with this
patch reverted) identical with pm_runtime_disable_action():

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index 22a581c894f8..459cc9c67eec 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -423,6 +423,12 @@ static int rzg2l_adc_hw_init(struct device *dev,
struct rzg2l_adc *adc)
        return ret;
 }

+static void rzg2l_pm_runtime_disable(void *data)
+{
+       pm_runtime_dont_use_autosuspend(data);
+       pm_runtime_disable(data);
+}
+
 static int rzg2l_adc_probe(struct platform_device *pdev)
 {
        struct device *dev = &pdev->dev;
@@ -463,7 +469,9 @@ static int rzg2l_adc_probe(struct platform_device *pdev)

        pm_runtime_set_autosuspend_delay(dev, 300);
        pm_runtime_use_autosuspend(dev);
-       ret = devm_pm_runtime_enable(dev);
+       pm_runtime_enable(dev);
+
+       ret = devm_add_action_or_reset(dev, rzg2l_pm_runtime_disable, dev);
        if (ret)
                return ret;

With this the issue is still reproducible.

However, changing the order of functions in rzg2l_pm_runtime_disable() and
having it like:

+static void rzg2l_pm_runtime_disable(void *data)
+{
+       pm_runtime_disable(data);
+       pm_runtime_dont_use_autosuspend(data);
+}


leads to no failure when doing unbind/bind.

However, I see the pm_runtime_disable() can still call rpm_resume() under
certain conditions. It can still lead to failures if it is called after the
device was remove from its PM domain.

> 
> In driver remove flow, device_unbind_cleanup9() is called
> just after device_remove() which is calling the dev->driver_remove()
> callback. There are no runtime pm related calls in between that I can see.

On my side the device_remove() is calling dev->bus->remove() which is
platform_remove(), which calls the dev_pm_domain_detach(). The
dev_pm_domain_detach() detaches the ADC from it's PM domain. Because of
this, accessing now the ADC registers after a runtime resume leads to
failures pointed in this patch (as of my investigation) (as the ADC is not
anymore part of its PM domain and its PM domain is not started anymore
though runtime PM APIs).

A similar issue was found while I was adding thermal support for RZ/G3S,
explained in
https://lore.kernel.org/all/[e-mail]


Jonathan, Rafael, Ulf, all,

Do consider OK to change the order in pm_runtime_disable_action() to get
rid of these issues, e.g.:

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 2ee45841486b..f27d311d2619 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);

 static void pm_runtime_disable_action(void *data)
 {
-       pm_runtime_dont_use_autosuspend(data);
        pm_runtime_disable(data);
+       pm_runtime_dont_use_autosuspend(data);
 }

though I see a rpm_resume() call is still possible though pm_runtime_disable().

Thank you,
Claudiu

> 
> So you are moving these calls a little earlier but not in a fashion that
> seems to have any involvement with anything else.
> 
> 
> Call stack being
> device_release_driver_internal()
> -> __device_release_driver()
>   -> device_remove() where you are now calling the disables
>    .. some dma stuff
>   -> device_unbind_cleanup() where the calling of disables previously was.
> 
> other than that unrelated DMA stuff there is nothing between the
> two calls. 
> 
> There is runtime PM stuff before any of this, but not in the crucial
> portion this code affects.
> 
> So I am thinking the only change that actually matters is the trivial
> reorder mentioned above.
> 
> 
> 
> 
> 
>>
>>>
>>>   
>>>>     __pm_runtime_use_autosuspend() ->
>>>>       update_autosuspend() ->
>>>>         rpm_idle()
>>>>
>>>> The rpm_idle() function attempts to runtime resume the ADC device.  
>>>
>>> Can you give a little more on that path. I'm not immediately spotting
>>> how rpm_idle() is causing a resume  
>>
>> It is not in particular about the resume. Runtime suspend/resume after
>> devres_release_all() will be affected.
>>
>> In particular, the rpm_idle() can call rpm_suspend() (called on the out
>> label of rpm_idle()) and rpm_suspend() may call the driver specific
>> runtime_suspend API through the following code (from the rpm_suspend()
>> function):
>>
>>         callback = RPM_GET_CALLBACK(dev, runtime_suspend);
>>
>>
>>
>>         dev_pm_enable_wake_irq_check(dev, true);
>>
>>         retval = rpm_callback(callback, dev);
>>
>>         if (retval)
>>
>>                 goto fail;
>>
>>
>>
>> The full stack generated when running:
>> # cd /sys/bus/platform/drivers/rzg2l-adc
>> # while :; do echo 10058000.adc > unbind ; echo 10058000.adc > bind; done
>>
>> is as follows:
>>
>> [   24.801195] Call trace:
>> [   24.803633]  rzg2l_adc_pm_runtime_suspend+0x18/0x54 (P)
>> [   24.808847]  pm_generic_runtime_suspend+0x2c/0x44 (L)
>> [   24.813887]  pm_generic_runtime_suspend+0x2c/0x44
>> [   24.818580]  __rpm_callback+0x48/0x198
>> [   24.822319]  rpm_callback+0x68/0x74
>> [   24.825798]  rpm_suspend+0x100/0x578
>> [   24.829362]  rpm_idle+0xd0/0x17c
>> [   24.832582]  update_autosuspend+0x30/0xc4
>> [   24.836580]  pm_runtime_disable_action+0x40/0x64
>> [   24.841184]  devm_action_release+0x14/0x20
>> [   24.845274]  devres_release_all+0xa0/0x100
>> [   24.849361]  device_unbind_cleanup+0x18/0x60
>> [   24.853618]  device_release_driver_internal+0x1ec/0x228
>> [   24.858828]  device_driver_detach+0x18/0x24
>> [   24.862998]  unbind_store+0xb4/0xb8
>> [   24.866478]  drv_attr_store+0x24/0x38
>> [   24.870135]  sysfs_kf_write+0x44/0x54
>> [   24.873795]  kernfs_fop_write_iter+0x118/0x1a8
>> [   24.878229]  vfs_write+0x2ac/0x358
>> [   24.881627]  ksys_write+0x68/0xfc
>> [   24.884934]  __arm64_sys_write+0x1c/0x28
>> [   24.888846]  invoke_syscall+0x48/0x110
>> [   24.892592]  el0_svc_common.constprop.0+0xc0/0xe0
>> [   24.897285]  do_el0_svc+0x1c/0x28
>> [   24.900593]  el0_svc+0x30/0xd0
>> [   24.903647]  el0t_64_sync_handler+0xc8/0xcc
>> [   24.907821]  el0t_64_sync+0x198/0x19c
>> [   24.911481] Code: 910003fd f9403c00 f941bc01 f9400020 (b9400000)
> 
> Thanks, that was helpful.
> 
>>
>>
>> Digging it further today: on the Renesas RZ/G3S we implement the power
>> domain on/off and we register the PM domain with GENPD_FLAG_PM_CLK. The
>> on/off PM domain functionality is implemented though the clock controller
>> MSTOP functionality which blocks the bus access to the specific IP (in this
>> particular case to the ADC).
>>
>> The issue is reproducible when doing:
>> # cd /sys/bus/platform/drivers/rzg2l-adc
>> # while :; do echo 10058000.adc > unbind ; echo 10058000.adc > bind; done
>>
>> I noticed today that doing single manual unbind+bind doesn't always leads
>> to aborts. It may be related to the fact that, as I noticed, the genpd
>> power off is called asynchronously as a work (through
>> genpd_power_off_work_fn()).
>>
>> I also noticed today what when there is no on/off functionality implemented
>> on the PM domain we have no failures (as the MSTOP is not implemented and
>> the bus access to the specific IP is not blocked as there is no PM domain
>> on/off available).
> 
> The PM domain stuff is only called later in device_unbind_cleanup()
> so I don't see the relevance. All the code you are modifying is
> done before that happens.
> 
> Jonathan
> 
> 
>>
>>
>>
>>>   
>>>> However,
>>>> at the point it is called, the ADC device is no longer part of the PM
>>>> domain (which manages the ADC clocks). Since the rzg2l_adc runtime PM
>>>> APIs directly modifies hardware registers, the
>>>> rzg2l_adc_pm_runtime_resume() function is invoked without the ADC clocks
>>>> being enabled. This is because the PM domain no longer resumes along with
>>>> the ADC device. As a result, this leads to system aborts.
>>>>
>>>> Drop the devres API for runtime PM enable.
>>>>
>>>> Fixes: 89ee8174e8c8 ("iio: adc: rzg2l_adc: Simplify the runtime PM code")
>>>> Signed-off-by: Claudiu Beznea <[e-mail]>  
>>>
>>> See below. I'm doubtful in general about the sequence changes and
>>> specifically you can't just remove one devm callback from a driver without
>>> modifying a lot of other code / leaving really fragile ordering.
>>>
>>> Jonathan
>>>   
>>>> ---
>>>>  drivers/iio/adc/rzg2l_adc.c | 33 ++++++++++++++++++++++++---------
>>>>  1 file changed, 24 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
>>>> index 883c167c0670..f12f3daf08cc 100644
>>>> --- a/drivers/iio/adc/rzg2l_adc.c
>>>> +++ b/drivers/iio/adc/rzg2l_adc.c
>>>> @@ -464,25 +464,26 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>>>>  
>>>>  	pm_runtime_set_autosuspend_delay(dev, 300);
>>>>  	pm_runtime_use_autosuspend(dev);
>>>> -	ret = devm_pm_runtime_enable(dev);
>>>> -	if (ret)
>>>> -		return ret;
>>>> +	pm_runtime_enable(dev);
>>>>  
>>>>  	platform_set_drvdata(pdev, indio_dev);
>>>>  
>>>>  	ret = rzg2l_adc_hw_init(dev, adc);
>>>> -	if (ret)
>>>> -		return dev_err_probe(&pdev->dev, ret,
>>>> -				     "failed to initialize ADC HW\n");
>>>> +	if (ret) {
>>>> +		dev_err_probe(&pdev->dev, ret, "failed to initialize ADC HW\n");
>>>> +		goto rpm_disable;
>>>> +	}
>>>>  
>>>>  	irq = platform_get_irq(pdev, 0);
>>>> -	if (irq < 0)
>>>> -		return irq;
>>>> +	if (irq < 0) {
>>>> +		ret = irq;
>>>> +		goto rpm_disable;
>>>> +	}
>>>>  
>>>>  	ret = devm_request_irq(dev, irq, rzg2l_adc_isr,
>>>>  			       0, dev_name(dev), adc);
>>>>  	if (ret < 0)
>>>> -		return ret;
>>>> +		goto rpm_disable;
>>>>  
>>>>  	init_completion(&adc->completion);
>>>>  
>>>> @@ -493,6 +494,19 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>>>>  	indio_dev->num_channels = adc->data->num_channels;
>>>>  
>>>>  	return devm_iio_device_register(dev, indio_dev);
>>>> +
>>>> +rpm_disable:
>>>> +	pm_runtime_disable(dev);
>>>> +	pm_runtime_dont_use_autosuspend(dev);
>>>> +	return ret;  
>>> If you have to move away from devm you must do it for all calls after
>>> the first thing that is manually cleaned up.
>>> As you have it here the userspace interfaces are left available at a point
>>> well after power down.  
>>
>> I see, thank you for pointing it.
>>
>> And thank you for checking this,
>> Claudiu
>>
>>>   
>>>> +}
>>>> +
>>>> +static void rzg2l_adc_remove(struct platform_device *pdev)
>>>> +{
>>>> +	struct device *dev = &pdev->dev;
>>>> +
>>>> +	pm_runtime_disable(dev);
>>>> +	pm_runtime_dont_use_autosuspend(dev);
>>>>  }
>>>>  
>>>>  static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
>>>> @@ -614,6 +628,7 @@ static const struct dev_pm_ops rzg2l_adc_pm_ops = {
>>>>  
>>>>  static struct platform_driver rzg2l_adc_driver = {
>>>>  	.probe		= rzg2l_adc_probe,
>>>> +	.remove		= rzg2l_adc_remove,
>>>>  	.driver		= {
>>>>  		.name		= DRIVER_NAME,
>>>>  		.of_match_table = rzg2l_adc_match,  
>>>   
>>
>>
>

Ulf Hansson Jan. 15, 2025, 3:23 p.m. UTC | #5

On Fri, 3 Jan 2025 at 15:00, Claudiu <[e-mail]> wrote:
>
> From: Claudiu Beznea <[e-mail]>
>
> On all systems where the rzg2l_adc driver is used, the ADC clocks are part
> of a PM domain. The code that implements the PM domains support is in
> drivers/clk/renesas/rzg2l-cpg.c, the functions of interest for this commit
> being rzg2l_cpg_attach_dev() and rzg2l_cpg_deattach_dev(). The PM
> domains support is registered with GENPD_FLAG_PM_CLK which, according to
> the documentation, instructs genpd to use the PM clk framework while
> powering on/off attached devices.
>
> During probe, the ADC device is attached to the PM domain
> controlling the ADC clocks. Similarly, during removal, the ADC device is
> 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 ADC 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 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 runtime resume the ADC device. However,
> at the point it is called, the ADC device is no longer part of the PM
> domain (which manages the ADC clocks). Since the rzg2l_adc runtime PM
> APIs directly modifies hardware registers, the
> rzg2l_adc_pm_runtime_resume() function is invoked without the ADC clocks
> being enabled. This is because the PM domain no longer resumes along with
> the ADC device. As a result, this leads to system aborts.
>
> Drop the devres API for runtime PM enable.
>
> Fixes: 89ee8174e8c8 ("iio: adc: rzg2l_adc: Simplify the runtime PM code")
> Signed-off-by: Claudiu Beznea <[e-mail]>

Reviewed-by: Ulf Hansson <[e-mail]>

Kind regards
Uffe

> ---
>  drivers/iio/adc/rzg2l_adc.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> index 883c167c0670..f12f3daf08cc 100644
> --- a/drivers/iio/adc/rzg2l_adc.c
> +++ b/drivers/iio/adc/rzg2l_adc.c
> @@ -464,25 +464,26 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>
>         pm_runtime_set_autosuspend_delay(dev, 300);
>         pm_runtime_use_autosuspend(dev);
> -       ret = devm_pm_runtime_enable(dev);
> -       if (ret)
> -               return ret;
> +       pm_runtime_enable(dev);
>
>         platform_set_drvdata(pdev, indio_dev);
>
>         ret = rzg2l_adc_hw_init(dev, adc);
> -       if (ret)
> -               return dev_err_probe(&pdev->dev, ret,
> -                                    "failed to initialize ADC HW\n");
> +       if (ret) {
> +               dev_err_probe(&pdev->dev, ret, "failed to initialize ADC HW\n");
> +               goto rpm_disable;
> +       }
>
>         irq = platform_get_irq(pdev, 0);
> -       if (irq < 0)
> -               return irq;
> +       if (irq < 0) {
> +               ret = irq;
> +               goto rpm_disable;
> +       }
>
>         ret = devm_request_irq(dev, irq, rzg2l_adc_isr,
>                                0, dev_name(dev), adc);
>         if (ret < 0)
> -               return ret;
> +               goto rpm_disable;
>
>         init_completion(&adc->completion);
>
> @@ -493,6 +494,19 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>         indio_dev->num_channels = adc->data->num_channels;
>
>         return devm_iio_device_register(dev, indio_dev);
> +
> +rpm_disable:
> +       pm_runtime_disable(dev);
> +       pm_runtime_dont_use_autosuspend(dev);
> +       return ret;
> +}
> +
> +static void rzg2l_adc_remove(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +
> +       pm_runtime_disable(dev);
> +       pm_runtime_dont_use_autosuspend(dev);
>  }
>
>  static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
> @@ -614,6 +628,7 @@ static const struct dev_pm_ops rzg2l_adc_pm_ops = {
>
>  static struct platform_driver rzg2l_adc_driver = {
>         .probe          = rzg2l_adc_probe,
> +       .remove         = rzg2l_adc_remove,
>         .driver         = {
>                 .name           = DRIVER_NAME,
>                 .of_match_table = rzg2l_adc_match,
> --
> 2.43.0
>

Ulf Hansson Jan. 15, 2025, 3:29 p.m. UTC | #6

On Wed, 15 Jan 2025 at 14:37, Claudiu Beznea <[e-mail]> wrote:
>
> Hi, Jonathan,
>
> Thank you for your input!
>
> On 11.01.2025 15:14, Jonathan Cameron wrote:
> > On Mon, 6 Jan 2025 11:18:41 +0200
> > Claudiu Beznea <[e-mail]> wrote:
> >
> >> Hi, Jonathan,
> >>
> >>
> >> On 04.01.2025 15:52, Jonathan Cameron wrote:
> >>> On Fri,  3 Jan 2025 16:00:41 +0200
> >>> Claudiu <[e-mail]> wrote:
> >>>
> >>>> From: Claudiu Beznea <[e-mail]>
> >>> +CC Rafael and linux-pm
> >>>
> >>>>
> >>>> On all systems where the rzg2l_adc driver is used, the ADC clocks are part
> >>>> of a PM domain. The code that implements the PM domains support is in
> >>>> drivers/clk/renesas/rzg2l-cpg.c, the functions of interest for this commit
> >>>> being rzg2l_cpg_attach_dev() and rzg2l_cpg_deattach_dev(). The PM
> >>>> domains support is registered with GENPD_FLAG_PM_CLK which, according to
> >>>> the documentation, instructs genpd to use the PM clk framework while
> >>>> powering on/off attached devices.
> >>>>
> >>>> During probe, the ADC device is attached to the PM domain
> >>>> controlling the ADC clocks. Similarly, during removal, the ADC device is
> >>>> 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 ADC 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 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() ->
> >>>
> >>> So is the only real difference that in the code below you disable runtime pm
> >>> before autosuspend?
> >>
> >> No, the difference is that now, the driver specific runtime PM APIs are not
> >> called anymore (through the pointed call stack) after the ADC was removed
> >> from it's PM domain.
> >
> > By my reading they are only not called now because you turn autosuspend off
> > after disabling runtime PM.
>
> Sorry, I wanted to say that the runtime PM APIs are not called anymore from
> devm_action_release(), though this call stack:
>
> [   24.801195] Call trace:
> [   24.803633]  rzg2l_adc_pm_runtime_suspend+0x18/0x54 (P)
> [   24.808847]  pm_generic_runtime_suspend+0x2c/0x44 (L)
> [   24.813887]  pm_generic_runtime_suspend+0x2c/0x44
> [   24.818580]  __rpm_callback+0x48/0x198
> [   24.822319]  rpm_callback+0x68/0x74
> [   24.825798]  rpm_suspend+0x100/0x578
> [   24.829362]  rpm_idle+0xd0/0x17c
> [   24.832582]  update_autosuspend+0x30/0xc4
> [   24.836580]  pm_runtime_disable_action+0x40/0x64
> [   24.841184]  devm_action_release+0x14/0x20
> [   24.845274]  devres_release_all+0xa0/0x100
> [   24.849361]  device_unbind_cleanup+0x18/0x60
>
> This is because I dropped the devm_pm_runtime_enable() which registers the
> pm_runtime_disable_action(), which is called at the time the
> device_unbind_cleanup() is called, which is called when the ADC is not
> anymore part of its PM domain.
>
> If I change the order in remove function proposed in this patch, thus do:
>
> +static void rzg2l_adc_remove(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +
> +       pm_runtime_dont_use_autosuspend(dev);
> +       pm_runtime_disable(dev);
>  }
>
> nothing changes with the behavior of this patch. There will be no issue if
> the device is runtime suspended/resumed through the
> pm_runtime_dont_use_autosuspend() because at the time the
> rzg2l_adc_remove() is called the ADC is still part of the PM domain.
>
>
>
> >
> >>
> >>
> >>>  Can you still do that with a devm callback just not
> >>> the standard one?
> >>
> >> No. It doesn't matter if we call the standard devm callback or driver
> >> specific one. As long as it is devm it will impact us as long as the driver
> >> specific runtime PM APIs are called through devres_release_all() after
> >> dev_pm_domain_detach(). And at that time the PM domain may be off along
> >> with its clocks disabled.
> >
> > As above, I think that this is only the case because of the reordering
> > of those two calls, not something more fundamental.
>
> I tried having a local devm function (the following diff applied with this
> patch reverted) identical with pm_runtime_disable_action():
>
> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> index 22a581c894f8..459cc9c67eec 100644
> --- a/drivers/iio/adc/rzg2l_adc.c
> +++ b/drivers/iio/adc/rzg2l_adc.c
> @@ -423,6 +423,12 @@ static int rzg2l_adc_hw_init(struct device *dev,
> struct rzg2l_adc *adc)
>         return ret;
>  }
>
> +static void rzg2l_pm_runtime_disable(void *data)
> +{
> +       pm_runtime_dont_use_autosuspend(data);
> +       pm_runtime_disable(data);
> +}
> +
>  static int rzg2l_adc_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> @@ -463,7 +469,9 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>
>         pm_runtime_set_autosuspend_delay(dev, 300);
>         pm_runtime_use_autosuspend(dev);
> -       ret = devm_pm_runtime_enable(dev);
> +       pm_runtime_enable(dev);
> +
> +       ret = devm_add_action_or_reset(dev, rzg2l_pm_runtime_disable, dev);
>         if (ret)
>                 return ret;
>
> With this the issue is still reproducible.
>
> However, changing the order of functions in rzg2l_pm_runtime_disable() and
> having it like:
>
> +static void rzg2l_pm_runtime_disable(void *data)
> +{
> +       pm_runtime_disable(data);
> +       pm_runtime_dont_use_autosuspend(data);
> +}
>
>
> leads to no failure when doing unbind/bind.
>
> However, I see the pm_runtime_disable() can still call rpm_resume() under
> certain conditions. It can still lead to failures if it is called after the
> device was remove from its PM domain.
>
> >
> > In driver remove flow, device_unbind_cleanup9() is called
> > just after device_remove() which is calling the dev->driver_remove()
> > callback. There are no runtime pm related calls in between that I can see.
>
> On my side the device_remove() is calling dev->bus->remove() which is
> platform_remove(), which calls the dev_pm_domain_detach(). The
> dev_pm_domain_detach() detaches the ADC from it's PM domain. Because of
> this, accessing now the ADC registers after a runtime resume leads to
> failures pointed in this patch (as of my investigation) (as the ADC is not
> anymore part of its PM domain and its PM domain is not started anymore
> though runtime PM APIs).
>
> A similar issue was found while I was adding thermal support for RZ/G3S,
> explained in
> https://lore.kernel.org/all/[e-mail]
>
>
> Jonathan, Rafael, Ulf, all,
>
> Do consider OK to change the order in pm_runtime_disable_action() to get
> rid of these issues, e.g.:
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 2ee45841486b..f27d311d2619 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
>
>  static void pm_runtime_disable_action(void *data)
>  {
> -       pm_runtime_dont_use_autosuspend(data);
>         pm_runtime_disable(data);
> +       pm_runtime_dont_use_autosuspend(data);
>  }
>
> though I see a rpm_resume() call is still possible though pm_runtime_disable().

I am still worried about keeping the device runtime enabled during a
window when we have turned off all resources for the device. Typically
we want to leave the device in a low power state after unbind.

That said, I would rather just drop the devm_pm_runtime_enable() API
altogether and convert all users of it into
pm_runtime_enable|disable(), similar to what your patch does.

>
> Thank you,
> Claudiu

[...]

Kind regards
Uffe

Jonathan Cameron Jan. 15, 2025, 3:42 p.m. UTC | #7

On Wed, 15 Jan 2025 15:37:57 +0200
Claudiu Beznea <[e-mail]> wrote:

> Hi, Jonathan,
> 
> Thank you for your input!
> 
> On 11.01.2025 15:14, Jonathan Cameron wrote:
> > On Mon, 6 Jan 2025 11:18:41 +0200
> > Claudiu Beznea <[e-mail]> wrote:
> >   
> >> Hi, Jonathan,
> >>
> >>
> >> On 04.01.2025 15:52, Jonathan Cameron wrote:  
> >>> On Fri,  3 Jan 2025 16:00:41 +0200
> >>> Claudiu <[e-mail]> wrote:
> >>>     
> >>>> From: Claudiu Beznea <[e-mail]>    
> >>> +CC Rafael and linux-pm
> >>>     
> >>>>
> >>>> On all systems where the rzg2l_adc driver is used, the ADC clocks are part
> >>>> of a PM domain. The code that implements the PM domains support is in
> >>>> drivers/clk/renesas/rzg2l-cpg.c, the functions of interest for this commit
> >>>> being rzg2l_cpg_attach_dev() and rzg2l_cpg_deattach_dev(). The PM
> >>>> domains support is registered with GENPD_FLAG_PM_CLK which, according to
> >>>> the documentation, instructs genpd to use the PM clk framework while
> >>>> powering on/off attached devices.
> >>>>
> >>>> During probe, the ADC device is attached to the PM domain
> >>>> controlling the ADC clocks. Similarly, during removal, the ADC device is
> >>>> 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 ADC 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 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() ->    
> >>>
> >>> So is the only real difference that in the code below you disable runtime pm
> >>> before autosuspend?    
> >>
> >> No, the difference is that now, the driver specific runtime PM APIs are not
> >> called anymore (through the pointed call stack) after the ADC was removed
> >> from it's PM domain.  
> > 
> > By my reading they are only not called now because you turn autosuspend off
> > after disabling runtime PM.  
> 
> Sorry, I wanted to say that the runtime PM APIs are not called anymore from
> devm_action_release(), though this call stack:
> 
> [   24.801195] Call trace:
> [   24.803633]  rzg2l_adc_pm_runtime_suspend+0x18/0x54 (P)
> [   24.808847]  pm_generic_runtime_suspend+0x2c/0x44 (L)
> [   24.813887]  pm_generic_runtime_suspend+0x2c/0x44
> [   24.818580]  __rpm_callback+0x48/0x198
> [   24.822319]  rpm_callback+0x68/0x74
> [   24.825798]  rpm_suspend+0x100/0x578
> [   24.829362]  rpm_idle+0xd0/0x17c
> [   24.832582]  update_autosuspend+0x30/0xc4
> [   24.836580]  pm_runtime_disable_action+0x40/0x64
> [   24.841184]  devm_action_release+0x14/0x20
> [   24.845274]  devres_release_all+0xa0/0x100
> [   24.849361]  device_unbind_cleanup+0x18/0x60
> 
> This is because I dropped the devm_pm_runtime_enable() which registers the
> pm_runtime_disable_action(), which is called at the time the
> device_unbind_cleanup() is called, which is called when the ADC is not
> anymore part of its PM domain.
> 
> If I change the order in remove function proposed in this patch, thus do:
> 
> +static void rzg2l_adc_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_disable(dev);
>  }
> 
> nothing changes with the behavior of this patch. There will be no issue if
> the device is runtime suspended/resumed through the
> pm_runtime_dont_use_autosuspend() because at the time the
> rzg2l_adc_remove() is called the ADC is still part of the PM domain.
> 
> 
> 
> >   
> >>
> >>  
> >>>  Can you still do that with a devm callback just not
> >>> the standard one?    
> >>
> >> No. It doesn't matter if we call the standard devm callback or driver
> >> specific one. As long as it is devm it will impact us as long as the driver
> >> specific runtime PM APIs are called through devres_release_all() after
> >> dev_pm_domain_detach(). And at that time the PM domain may be off along
> >> with its clocks disabled.  
> > 
> > As above, I think that this is only the case because of the reordering
> > of those two calls, not something more fundamental.  
> 
> I tried having a local devm function (the following diff applied with this
> patch reverted) identical with pm_runtime_disable_action():
> 
> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> index 22a581c894f8..459cc9c67eec 100644
> --- a/drivers/iio/adc/rzg2l_adc.c
> +++ b/drivers/iio/adc/rzg2l_adc.c
> @@ -423,6 +423,12 @@ static int rzg2l_adc_hw_init(struct device *dev,
> struct rzg2l_adc *adc)
>         return ret;
>  }
> 
> +static void rzg2l_pm_runtime_disable(void *data)
> +{
> +       pm_runtime_dont_use_autosuspend(data);
> +       pm_runtime_disable(data);
> +}
> +
>  static int rzg2l_adc_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
> @@ -463,7 +469,9 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
> 
>         pm_runtime_set_autosuspend_delay(dev, 300);
>         pm_runtime_use_autosuspend(dev);
> -       ret = devm_pm_runtime_enable(dev);
> +       pm_runtime_enable(dev);
> +
> +       ret = devm_add_action_or_reset(dev, rzg2l_pm_runtime_disable, dev);
>         if (ret)
>                 return ret;
> 
> With this the issue is still reproducible.
> 
> However, changing the order of functions in rzg2l_pm_runtime_disable() and
> having it like:
> 
> +static void rzg2l_pm_runtime_disable(void *data)
> +{
> +       pm_runtime_disable(data);
> +       pm_runtime_dont_use_autosuspend(data);
> +}
> 
> 
> leads to no failure when doing unbind/bind.
> 
> However, I see the pm_runtime_disable() can still call rpm_resume() under
> certain conditions. It can still lead to failures if it is called after the
> device was remove from its PM domain.
> 
> > 
> > In driver remove flow, device_unbind_cleanup9() is called
> > just after device_remove() which is calling the dev->driver_remove()
> > callback. There are no runtime pm related calls in between that I can see.  
> 
> On my side the device_remove() is calling dev->bus->remove() which is
> platform_remove(), which calls the dev_pm_domain_detach(). The
> dev_pm_domain_detach() detaches the ADC from it's PM domain. Because of
> this, accessing now the ADC registers after a runtime resume leads to
> failures pointed in this patch (as of my investigation) (as the ADC is not
> anymore part of its PM domain and its PM domain is not started anymore
> though runtime PM APIs).
> 
> A similar issue was found while I was adding thermal support for RZ/G3S,
> explained in
> https://lore.kernel.org/all/[e-mail]
> 
> 
> Jonathan, Rafael, Ulf, all,
> 
> Do consider OK to change the order in pm_runtime_disable_action() to get
> rid of these issues, e.g.:
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 2ee45841486b..f27d311d2619 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> 
>  static void pm_runtime_disable_action(void *data)
>  {
> -       pm_runtime_dont_use_autosuspend(data);
>         pm_runtime_disable(data);
> +       pm_runtime_dont_use_autosuspend(data);
>  }
> 
> though I see a rpm_resume() call is still possible though pm_runtime_disable().

This isn't the right fix.  I was just trying to get to the bottom of why
your fix worked and reordering was a false path. If we go ahead with this
patch, then put them in the same order as in pm_runtime_disable_action()
and add a nice big comment on why we have to do them manually.

Now you've talked me through the call of platform_remove() I can see the
dev_pm_domain_detach(). It seems odd that is called before 
the devres manged part of device tear down.

The change here to me smells like a hack to get around that and looks
like a bad idea from a maintenance point of view.

Rafael/all, right approach or should we do something else?

Jonathan


> 
> Thank you,
> Claudiu
> 
> > 
> > So you are moving these calls a little earlier but not in a fashion that
> > seems to have any involvement with anything else.
> > 
> > 
> > Call stack being
> > device_release_driver_internal()  
> > -> __device_release_driver()
> >   -> device_remove() where you are now calling the disables  
> >    .. some dma stuff  
> >   -> device_unbind_cleanup() where the calling of disables previously was.  
> > 
> > other than that unrelated DMA stuff there is nothing between the
> > two calls. 
> > 
> > There is runtime PM stuff before any of this, but not in the crucial
> > portion this code affects.
> > 
> > So I am thinking the only change that actually matters is the trivial
> > reorder mentioned above.
> > 
> > 
> > 
> > 
> >   
> >>  
> >>>
> >>>     
> >>>>     __pm_runtime_use_autosuspend() ->
> >>>>       update_autosuspend() ->
> >>>>         rpm_idle()
> >>>>
> >>>> The rpm_idle() function attempts to runtime resume the ADC device.    
> >>>
> >>> Can you give a little more on that path. I'm not immediately spotting
> >>> how rpm_idle() is causing a resume    
> >>
> >> It is not in particular about the resume. Runtime suspend/resume after
> >> devres_release_all() will be affected.
> >>
> >> In particular, the rpm_idle() can call rpm_suspend() (called on the out
> >> label of rpm_idle()) and rpm_suspend() may call the driver specific
> >> runtime_suspend API through the following code (from the rpm_suspend()
> >> function):
> >>
> >>         callback = RPM_GET_CALLBACK(dev, runtime_suspend);
> >>
> >>
> >>
> >>         dev_pm_enable_wake_irq_check(dev, true);
> >>
> >>         retval = rpm_callback(callback, dev);
> >>
> >>         if (retval)
> >>
> >>                 goto fail;
> >>
> >>
> >>
> >> The full stack generated when running:
> >> # cd /sys/bus/platform/drivers/rzg2l-adc
> >> # while :; do echo 10058000.adc > unbind ; echo 10058000.adc > bind; done
> >>
> >> is as follows:
> >>
> >> [   24.801195] Call trace:
> >> [   24.803633]  rzg2l_adc_pm_runtime_suspend+0x18/0x54 (P)
> >> [   24.808847]  pm_generic_runtime_suspend+0x2c/0x44 (L)
> >> [   24.813887]  pm_generic_runtime_suspend+0x2c/0x44
> >> [   24.818580]  __rpm_callback+0x48/0x198
> >> [   24.822319]  rpm_callback+0x68/0x74
> >> [   24.825798]  rpm_suspend+0x100/0x578
> >> [   24.829362]  rpm_idle+0xd0/0x17c
> >> [   24.832582]  update_autosuspend+0x30/0xc4
> >> [   24.836580]  pm_runtime_disable_action+0x40/0x64
> >> [   24.841184]  devm_action_release+0x14/0x20
> >> [   24.845274]  devres_release_all+0xa0/0x100
> >> [   24.849361]  device_unbind_cleanup+0x18/0x60
> >> [   24.853618]  device_release_driver_internal+0x1ec/0x228
> >> [   24.858828]  device_driver_detach+0x18/0x24
> >> [   24.862998]  unbind_store+0xb4/0xb8
> >> [   24.866478]  drv_attr_store+0x24/0x38
> >> [   24.870135]  sysfs_kf_write+0x44/0x54
> >> [   24.873795]  kernfs_fop_write_iter+0x118/0x1a8
> >> [   24.878229]  vfs_write+0x2ac/0x358
> >> [   24.881627]  ksys_write+0x68/0xfc
> >> [   24.884934]  __arm64_sys_write+0x1c/0x28
> >> [   24.888846]  invoke_syscall+0x48/0x110
> >> [   24.892592]  el0_svc_common.constprop.0+0xc0/0xe0
> >> [   24.897285]  do_el0_svc+0x1c/0x28
> >> [   24.900593]  el0_svc+0x30/0xd0
> >> [   24.903647]  el0t_64_sync_handler+0xc8/0xcc
> >> [   24.907821]  el0t_64_sync+0x198/0x19c
> >> [   24.911481] Code: 910003fd f9403c00 f941bc01 f9400020 (b9400000)  
> > 
> > Thanks, that was helpful.
> >   
> >>
> >>
> >> Digging it further today: on the Renesas RZ/G3S we implement the power
> >> domain on/off and we register the PM domain with GENPD_FLAG_PM_CLK. The
> >> on/off PM domain functionality is implemented though the clock controller
> >> MSTOP functionality which blocks the bus access to the specific IP (in this
> >> particular case to the ADC).
> >>
> >> The issue is reproducible when doing:
> >> # cd /sys/bus/platform/drivers/rzg2l-adc
> >> # while :; do echo 10058000.adc > unbind ; echo 10058000.adc > bind; done
> >>
> >> I noticed today that doing single manual unbind+bind doesn't always leads
> >> to aborts. It may be related to the fact that, as I noticed, the genpd
> >> power off is called asynchronously as a work (through
> >> genpd_power_off_work_fn()).
> >>
> >> I also noticed today what when there is no on/off functionality implemented
> >> on the PM domain we have no failures (as the MSTOP is not implemented and
> >> the bus access to the specific IP is not blocked as there is no PM domain
> >> on/off available).  
> > 
> > The PM domain stuff is only called later in device_unbind_cleanup()
> > so I don't see the relevance. All the code you are modifying is
> > done before that happens.
> > 
> > Jonathan
> > 
> >   
> >>
> >>
> >>  
> >>>     
> >>>> However,
> >>>> at the point it is called, the ADC device is no longer part of the PM
> >>>> domain (which manages the ADC clocks). Since the rzg2l_adc runtime PM
> >>>> APIs directly modifies hardware registers, the
> >>>> rzg2l_adc_pm_runtime_resume() function is invoked without the ADC clocks
> >>>> being enabled. This is because the PM domain no longer resumes along with
> >>>> the ADC device. As a result, this leads to system aborts.
> >>>>
> >>>> Drop the devres API for runtime PM enable.
> >>>>
> >>>> Fixes: 89ee8174e8c8 ("iio: adc: rzg2l_adc: Simplify the runtime PM code")
> >>>> Signed-off-by: Claudiu Beznea <[e-mail]>    
> >>>
> >>> See below. I'm doubtful in general about the sequence changes and
> >>> specifically you can't just remove one devm callback from a driver without
> >>> modifying a lot of other code / leaving really fragile ordering.
> >>>
> >>> Jonathan
> >>>     
> >>>> ---
> >>>>  drivers/iio/adc/rzg2l_adc.c | 33 ++++++++++++++++++++++++---------
> >>>>  1 file changed, 24 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> >>>> index 883c167c0670..f12f3daf08cc 100644
> >>>> --- a/drivers/iio/adc/rzg2l_adc.c
> >>>> +++ b/drivers/iio/adc/rzg2l_adc.c
> >>>> @@ -464,25 +464,26 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
> >>>>  
> >>>>  	pm_runtime_set_autosuspend_delay(dev, 300);
> >>>>  	pm_runtime_use_autosuspend(dev);
> >>>> -	ret = devm_pm_runtime_enable(dev);
> >>>> -	if (ret)
> >>>> -		return ret;
> >>>> +	pm_runtime_enable(dev);
> >>>>  
> >>>>  	platform_set_drvdata(pdev, indio_dev);
> >>>>  
> >>>>  	ret = rzg2l_adc_hw_init(dev, adc);
> >>>> -	if (ret)
> >>>> -		return dev_err_probe(&pdev->dev, ret,
> >>>> -				     "failed to initialize ADC HW\n");
> >>>> +	if (ret) {
> >>>> +		dev_err_probe(&pdev->dev, ret, "failed to initialize ADC HW\n");
> >>>> +		goto rpm_disable;
> >>>> +	}
> >>>>  
> >>>>  	irq = platform_get_irq(pdev, 0);
> >>>> -	if (irq < 0)
> >>>> -		return irq;
> >>>> +	if (irq < 0) {
> >>>> +		ret = irq;
> >>>> +		goto rpm_disable;
> >>>> +	}
> >>>>  
> >>>>  	ret = devm_request_irq(dev, irq, rzg2l_adc_isr,
> >>>>  			       0, dev_name(dev), adc);
> >>>>  	if (ret < 0)
> >>>> -		return ret;
> >>>> +		goto rpm_disable;
> >>>>  
> >>>>  	init_completion(&adc->completion);
> >>>>  
> >>>> @@ -493,6 +494,19 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
> >>>>  	indio_dev->num_channels = adc->data->num_channels;
> >>>>  
> >>>>  	return devm_iio_device_register(dev, indio_dev);
> >>>> +
> >>>> +rpm_disable:
> >>>> +	pm_runtime_disable(dev);
> >>>> +	pm_runtime_dont_use_autosuspend(dev);
> >>>> +	return ret;    
> >>> If you have to move away from devm you must do it for all calls after
> >>> the first thing that is manually cleaned up.
> >>> As you have it here the userspace interfaces are left available at a point
> >>> well after power down.    
> >>
> >> I see, thank you for pointing it.
> >>
> >> And thank you for checking this,
> >> Claudiu
> >>  
> >>>     
> >>>> +}
> >>>> +
> >>>> +static void rzg2l_adc_remove(struct platform_device *pdev)
> >>>> +{
> >>>> +	struct device *dev = &pdev->dev;
> >>>> +
> >>>> +	pm_runtime_disable(dev);
> >>>> +	pm_runtime_dont_use_autosuspend(dev);
> >>>>  }
> >>>>  
> >>>>  static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
> >>>> @@ -614,6 +628,7 @@ static const struct dev_pm_ops rzg2l_adc_pm_ops = {
> >>>>  
> >>>>  static struct platform_driver rzg2l_adc_driver = {
> >>>>  	.probe		= rzg2l_adc_probe,
> >>>> +	.remove		= rzg2l_adc_remove,
> >>>>  	.driver		= {
> >>>>  		.name		= DRIVER_NAME,
> >>>>  		.of_match_table = rzg2l_adc_match,    
> >>>     
> >>
> >>  
> >   
> 
> 
>

Jonathan Cameron Jan. 17, 2025, 3:52 p.m. UTC | #8

On Wed, 15 Jan 2025 16:29:15 +0100
Ulf Hansson <[e-mail]> wrote:

> On Wed, 15 Jan 2025 at 14:37, Claudiu Beznea <[e-mail]> wrote:
> >
> > Hi, Jonathan,
> >
> > Thank you for your input!
> >
> > On 11.01.2025 15:14, Jonathan Cameron wrote:  
> > > On Mon, 6 Jan 2025 11:18:41 +0200
> > > Claudiu Beznea <[e-mail]> wrote:
> > >  
> > >> Hi, Jonathan,
> > >>
> > >>
> > >> On 04.01.2025 15:52, Jonathan Cameron wrote:  
> > >>> On Fri,  3 Jan 2025 16:00:41 +0200
> > >>> Claudiu <[e-mail]> wrote:
> > >>>  
> > >>>> From: Claudiu Beznea <[e-mail]>  
> > >>> +CC Rafael and linux-pm
> > >>>  
> > >>>>
> > >>>> On all systems where the rzg2l_adc driver is used, the ADC clocks are part
> > >>>> of a PM domain. The code that implements the PM domains support is in
> > >>>> drivers/clk/renesas/rzg2l-cpg.c, the functions of interest for this commit
> > >>>> being rzg2l_cpg_attach_dev() and rzg2l_cpg_deattach_dev(). The PM
> > >>>> domains support is registered with GENPD_FLAG_PM_CLK which, according to
> > >>>> the documentation, instructs genpd to use the PM clk framework while
> > >>>> powering on/off attached devices.
> > >>>>
> > >>>> During probe, the ADC device is attached to the PM domain
> > >>>> controlling the ADC clocks. Similarly, during removal, the ADC device is
> > >>>> 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 ADC 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 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() ->  
> > >>>
> > >>> So is the only real difference that in the code below you disable runtime pm
> > >>> before autosuspend?  
> > >>
> > >> No, the difference is that now, the driver specific runtime PM APIs are not
> > >> called anymore (through the pointed call stack) after the ADC was removed
> > >> from it's PM domain.  
> > >
> > > By my reading they are only not called now because you turn autosuspend off
> > > after disabling runtime PM.  
> >
> > Sorry, I wanted to say that the runtime PM APIs are not called anymore from
> > devm_action_release(), though this call stack:
> >
> > [   24.801195] Call trace:
> > [   24.803633]  rzg2l_adc_pm_runtime_suspend+0x18/0x54 (P)
> > [   24.808847]  pm_generic_runtime_suspend+0x2c/0x44 (L)
> > [   24.813887]  pm_generic_runtime_suspend+0x2c/0x44
> > [   24.818580]  __rpm_callback+0x48/0x198
> > [   24.822319]  rpm_callback+0x68/0x74
> > [   24.825798]  rpm_suspend+0x100/0x578
> > [   24.829362]  rpm_idle+0xd0/0x17c
> > [   24.832582]  update_autosuspend+0x30/0xc4
> > [   24.836580]  pm_runtime_disable_action+0x40/0x64
> > [   24.841184]  devm_action_release+0x14/0x20
> > [   24.845274]  devres_release_all+0xa0/0x100
> > [   24.849361]  device_unbind_cleanup+0x18/0x60
> >
> > This is because I dropped the devm_pm_runtime_enable() which registers the
> > pm_runtime_disable_action(), which is called at the time the
> > device_unbind_cleanup() is called, which is called when the ADC is not
> > anymore part of its PM domain.
> >
> > If I change the order in remove function proposed in this patch, thus do:
> >
> > +static void rzg2l_adc_remove(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +
> > +       pm_runtime_dont_use_autosuspend(dev);
> > +       pm_runtime_disable(dev);
> >  }
> >
> > nothing changes with the behavior of this patch. There will be no issue if
> > the device is runtime suspended/resumed through the
> > pm_runtime_dont_use_autosuspend() because at the time the
> > rzg2l_adc_remove() is called the ADC is still part of the PM domain.
> >
> >
> >  
> > >  
> > >>
> > >>  
> > >>>  Can you still do that with a devm callback just not
> > >>> the standard one?  
> > >>
> > >> No. It doesn't matter if we call the standard devm callback or driver
> > >> specific one. As long as it is devm it will impact us as long as the driver
> > >> specific runtime PM APIs are called through devres_release_all() after
> > >> dev_pm_domain_detach(). And at that time the PM domain may be off along
> > >> with its clocks disabled.  
> > >
> > > As above, I think that this is only the case because of the reordering
> > > of those two calls, not something more fundamental.  
> >
> > I tried having a local devm function (the following diff applied with this
> > patch reverted) identical with pm_runtime_disable_action():
> >
> > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> > index 22a581c894f8..459cc9c67eec 100644
> > --- a/drivers/iio/adc/rzg2l_adc.c
> > +++ b/drivers/iio/adc/rzg2l_adc.c
> > @@ -423,6 +423,12 @@ static int rzg2l_adc_hw_init(struct device *dev,
> > struct rzg2l_adc *adc)
> >         return ret;
> >  }
> >
> > +static void rzg2l_pm_runtime_disable(void *data)
> > +{
> > +       pm_runtime_dont_use_autosuspend(data);
> > +       pm_runtime_disable(data);
> > +}
> > +
> >  static int rzg2l_adc_probe(struct platform_device *pdev)
> >  {
> >         struct device *dev = &pdev->dev;
> > @@ -463,7 +469,9 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
> >
> >         pm_runtime_set_autosuspend_delay(dev, 300);
> >         pm_runtime_use_autosuspend(dev);
> > -       ret = devm_pm_runtime_enable(dev);
> > +       pm_runtime_enable(dev);
> > +
> > +       ret = devm_add_action_or_reset(dev, rzg2l_pm_runtime_disable, dev);
> >         if (ret)
> >                 return ret;
> >
> > With this the issue is still reproducible.
> >
> > However, changing the order of functions in rzg2l_pm_runtime_disable() and
> > having it like:
> >
> > +static void rzg2l_pm_runtime_disable(void *data)
> > +{
> > +       pm_runtime_disable(data);
> > +       pm_runtime_dont_use_autosuspend(data);
> > +}
> >
> >
> > leads to no failure when doing unbind/bind.
> >
> > However, I see the pm_runtime_disable() can still call rpm_resume() under
> > certain conditions. It can still lead to failures if it is called after the
> > device was remove from its PM domain.
> >  
> > >
> > > In driver remove flow, device_unbind_cleanup9() is called
> > > just after device_remove() which is calling the dev->driver_remove()
> > > callback. There are no runtime pm related calls in between that I can see.  
> >
> > On my side the device_remove() is calling dev->bus->remove() which is
> > platform_remove(), which calls the dev_pm_domain_detach(). The
> > dev_pm_domain_detach() detaches the ADC from it's PM domain. Because of
> > this, accessing now the ADC registers after a runtime resume leads to
> > failures pointed in this patch (as of my investigation) (as the ADC is not
> > anymore part of its PM domain and its PM domain is not started anymore
> > though runtime PM APIs).
> >
> > A similar issue was found while I was adding thermal support for RZ/G3S,
> > explained in
> > https://lore.kernel.org/all/[e-mail]
> >
> >
> > Jonathan, Rafael, Ulf, all,
> >
> > Do consider OK to change the order in pm_runtime_disable_action() to get
> > rid of these issues, e.g.:
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index 2ee45841486b..f27d311d2619 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> >
> >  static void pm_runtime_disable_action(void *data)
> >  {
> > -       pm_runtime_dont_use_autosuspend(data);
> >         pm_runtime_disable(data);
> > +       pm_runtime_dont_use_autosuspend(data);
> >  }
> >
> > though I see a rpm_resume() call is still possible though pm_runtime_disable().  
> 
> I am still worried about keeping the device runtime enabled during a
> window when we have turned off all resources for the device. Typically
> we want to leave the device in a low power state after unbind.
> 
> That said, I would rather just drop the devm_pm_runtime_enable() API
> altogether and convert all users of it into
> pm_runtime_enable|disable(), similar to what your patch does.

That is making a mess of a lot of automated cleanup for a strange
runtime pm related path.  This is pain a driver should not have
to deal with, though I'm not clear what the right solution is!

Key is that drivers should not mix devm managed cleanup and not, so
that means that anything that happens after runtime pm is enabled
has to be torn down manually.  One solution to this might be to
always enable it late assuming that is safe to do so there is
never anything else done after it in the probe path of a driver.

Jonathan

> 
> >
> > Thank you,
> > Claudiu  
> 
> [...]
> 
> Kind regards
> Uffe
>

Ulf Hansson Jan. 20, 2025, 2:59 p.m. UTC | #9

On Fri, 17 Jan 2025 at 16:52, Jonathan Cameron
<[e-mail]> wrote:
>
> On Wed, 15 Jan 2025 16:29:15 +0100
> Ulf Hansson <[e-mail]> wrote:
>
> > On Wed, 15 Jan 2025 at 14:37, Claudiu Beznea <[e-mail]> wrote:
> > >
> > > Hi, Jonathan,
> > >
> > > Thank you for your input!
> > >
> > > On 11.01.2025 15:14, Jonathan Cameron wrote:
> > > > On Mon, 6 Jan 2025 11:18:41 +0200
> > > > Claudiu Beznea <[e-mail]> wrote:
> > > >
> > > >> Hi, Jonathan,
> > > >>
> > > >>
> > > >> On 04.01.2025 15:52, Jonathan Cameron wrote:
> > > >>> On Fri,  3 Jan 2025 16:00:41 +0200
> > > >>> Claudiu <[e-mail]> wrote:
> > > >>>
> > > >>>> From: Claudiu Beznea <[e-mail]>
> > > >>> +CC Rafael and linux-pm
> > > >>>
> > > >>>>
> > > >>>> On all systems where the rzg2l_adc driver is used, the ADC clocks are part
> > > >>>> of a PM domain. The code that implements the PM domains support is in
> > > >>>> drivers/clk/renesas/rzg2l-cpg.c, the functions of interest for this commit
> > > >>>> being rzg2l_cpg_attach_dev() and rzg2l_cpg_deattach_dev(). The PM
> > > >>>> domains support is registered with GENPD_FLAG_PM_CLK which, according to
> > > >>>> the documentation, instructs genpd to use the PM clk framework while
> > > >>>> powering on/off attached devices.
> > > >>>>
> > > >>>> During probe, the ADC device is attached to the PM domain
> > > >>>> controlling the ADC clocks. Similarly, during removal, the ADC device is
> > > >>>> 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 ADC 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 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() ->
> > > >>>
> > > >>> So is the only real difference that in the code below you disable runtime pm
> > > >>> before autosuspend?
> > > >>
> > > >> No, the difference is that now, the driver specific runtime PM APIs are not
> > > >> called anymore (through the pointed call stack) after the ADC was removed
> > > >> from it's PM domain.
> > > >
> > > > By my reading they are only not called now because you turn autosuspend off
> > > > after disabling runtime PM.
> > >
> > > Sorry, I wanted to say that the runtime PM APIs are not called anymore from
> > > devm_action_release(), though this call stack:
> > >
> > > [   24.801195] Call trace:
> > > [   24.803633]  rzg2l_adc_pm_runtime_suspend+0x18/0x54 (P)
> > > [   24.808847]  pm_generic_runtime_suspend+0x2c/0x44 (L)
> > > [   24.813887]  pm_generic_runtime_suspend+0x2c/0x44
> > > [   24.818580]  __rpm_callback+0x48/0x198
> > > [   24.822319]  rpm_callback+0x68/0x74
> > > [   24.825798]  rpm_suspend+0x100/0x578
> > > [   24.829362]  rpm_idle+0xd0/0x17c
> > > [   24.832582]  update_autosuspend+0x30/0xc4
> > > [   24.836580]  pm_runtime_disable_action+0x40/0x64
> > > [   24.841184]  devm_action_release+0x14/0x20
> > > [   24.845274]  devres_release_all+0xa0/0x100
> > > [   24.849361]  device_unbind_cleanup+0x18/0x60
> > >
> > > This is because I dropped the devm_pm_runtime_enable() which registers the
> > > pm_runtime_disable_action(), which is called at the time the
> > > device_unbind_cleanup() is called, which is called when the ADC is not
> > > anymore part of its PM domain.
> > >
> > > If I change the order in remove function proposed in this patch, thus do:
> > >
> > > +static void rzg2l_adc_remove(struct platform_device *pdev)
> > > +{
> > > +       struct device *dev = &pdev->dev;
> > > +
> > > +       pm_runtime_dont_use_autosuspend(dev);
> > > +       pm_runtime_disable(dev);
> > >  }
> > >
> > > nothing changes with the behavior of this patch. There will be no issue if
> > > the device is runtime suspended/resumed through the
> > > pm_runtime_dont_use_autosuspend() because at the time the
> > > rzg2l_adc_remove() is called the ADC is still part of the PM domain.
> > >
> > >
> > >
> > > >
> > > >>
> > > >>
> > > >>>  Can you still do that with a devm callback just not
> > > >>> the standard one?
> > > >>
> > > >> No. It doesn't matter if we call the standard devm callback or driver
> > > >> specific one. As long as it is devm it will impact us as long as the driver
> > > >> specific runtime PM APIs are called through devres_release_all() after
> > > >> dev_pm_domain_detach(). And at that time the PM domain may be off along
> > > >> with its clocks disabled.
> > > >
> > > > As above, I think that this is only the case because of the reordering
> > > > of those two calls, not something more fundamental.
> > >
> > > I tried having a local devm function (the following diff applied with this
> > > patch reverted) identical with pm_runtime_disable_action():
> > >
> > > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> > > index 22a581c894f8..459cc9c67eec 100644
> > > --- a/drivers/iio/adc/rzg2l_adc.c
> > > +++ b/drivers/iio/adc/rzg2l_adc.c
> > > @@ -423,6 +423,12 @@ static int rzg2l_adc_hw_init(struct device *dev,
> > > struct rzg2l_adc *adc)
> > >         return ret;
> > >  }
> > >
> > > +static void rzg2l_pm_runtime_disable(void *data)
> > > +{
> > > +       pm_runtime_dont_use_autosuspend(data);
> > > +       pm_runtime_disable(data);
> > > +}
> > > +
> > >  static int rzg2l_adc_probe(struct platform_device *pdev)
> > >  {
> > >         struct device *dev = &pdev->dev;
> > > @@ -463,7 +469,9 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
> > >
> > >         pm_runtime_set_autosuspend_delay(dev, 300);
> > >         pm_runtime_use_autosuspend(dev);
> > > -       ret = devm_pm_runtime_enable(dev);
> > > +       pm_runtime_enable(dev);
> > > +
> > > +       ret = devm_add_action_or_reset(dev, rzg2l_pm_runtime_disable, dev);
> > >         if (ret)
> > >                 return ret;
> > >
> > > With this the issue is still reproducible.
> > >
> > > However, changing the order of functions in rzg2l_pm_runtime_disable() and
> > > having it like:
> > >
> > > +static void rzg2l_pm_runtime_disable(void *data)
> > > +{
> > > +       pm_runtime_disable(data);
> > > +       pm_runtime_dont_use_autosuspend(data);
> > > +}
> > >
> > >
> > > leads to no failure when doing unbind/bind.
> > >
> > > However, I see the pm_runtime_disable() can still call rpm_resume() under
> > > certain conditions. It can still lead to failures if it is called after the
> > > device was remove from its PM domain.
> > >
> > > >
> > > > In driver remove flow, device_unbind_cleanup9() is called
> > > > just after device_remove() which is calling the dev->driver_remove()
> > > > callback. There are no runtime pm related calls in between that I can see.
> > >
> > > On my side the device_remove() is calling dev->bus->remove() which is
> > > platform_remove(), which calls the dev_pm_domain_detach(). The
> > > dev_pm_domain_detach() detaches the ADC from it's PM domain. Because of
> > > this, accessing now the ADC registers after a runtime resume leads to
> > > failures pointed in this patch (as of my investigation) (as the ADC is not
> > > anymore part of its PM domain and its PM domain is not started anymore
> > > though runtime PM APIs).
> > >
> > > A similar issue was found while I was adding thermal support for RZ/G3S,
> > > explained in
> > > https://lore.kernel.org/all/[e-mail]
> > >
> > >
> > > Jonathan, Rafael, Ulf, all,
> > >
> > > Do consider OK to change the order in pm_runtime_disable_action() to get
> > > rid of these issues, e.g.:
> > >
> > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > index 2ee45841486b..f27d311d2619 100644
> > > --- a/drivers/base/power/runtime.c
> > > +++ b/drivers/base/power/runtime.c
> > > @@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> > >
> > >  static void pm_runtime_disable_action(void *data)
> > >  {
> > > -       pm_runtime_dont_use_autosuspend(data);
> > >         pm_runtime_disable(data);
> > > +       pm_runtime_dont_use_autosuspend(data);
> > >  }
> > >
> > > though I see a rpm_resume() call is still possible though pm_runtime_disable().
> >
> > I am still worried about keeping the device runtime enabled during a
> > window when we have turned off all resources for the device. Typically
> > we want to leave the device in a low power state after unbind.
> >
> > That said, I would rather just drop the devm_pm_runtime_enable() API
> > altogether and convert all users of it into
> > pm_runtime_enable|disable(), similar to what your patch does.
>
> That is making a mess of a lot of automated cleanup for a strange
> runtime pm related path.  This is pain a driver should not have
> to deal with, though I'm not clear what the right solution is!
>
> Key is that drivers should not mix devm managed cleanup and not, so
> that means that anything that happens after runtime pm is enabled
> has to be torn down manually.  One solution to this might be to
> always enable it late assuming that is safe to do so there is
> never anything else done after it in the probe path of a driver.

The problem is that runtime PM isn't really comparable to other
resources that we are managing through devm* functions.

Enabling runtime PM for a device changes the behaviour for how
power-mgmt is handled for the device. Enabling/disabling of runtime PM
really needs to be explicitly controlled by the driver for the device.

If there are cleanups to be made for runtime PM, beyond disabling
runtime PM, we could instead consider adding that code in
pm_runtime_reinit().

[...]

Kind regards
Uffe

Jonathan Cameron Jan. 24, 2025, 6:41 p.m. UTC | #10

On Mon, 20 Jan 2025 15:59:02 +0100
Ulf Hansson <[e-mail]> wrote:

> On Fri, 17 Jan 2025 at 16:52, Jonathan Cameron
> <[e-mail]> wrote:
> >
> > On Wed, 15 Jan 2025 16:29:15 +0100
> > Ulf Hansson <[e-mail]> wrote:
> >  
> > > On Wed, 15 Jan 2025 at 14:37, Claudiu Beznea <[e-mail]> wrote:  
> > > >
> > > > Hi, Jonathan,
> > > >
> > > > Thank you for your input!
> > > >
> > > > On 11.01.2025 15:14, Jonathan Cameron wrote:  
> > > > > On Mon, 6 Jan 2025 11:18:41 +0200
> > > > > Claudiu Beznea <[e-mail]> wrote:
> > > > >  
> > > > >> Hi, Jonathan,
> > > > >>
> > > > >>
> > > > >> On 04.01.2025 15:52, Jonathan Cameron wrote:  
> > > > >>> On Fri,  3 Jan 2025 16:00:41 +0200
> > > > >>> Claudiu <[e-mail]> wrote:
> > > > >>>  
> > > > >>>> From: Claudiu Beznea <[e-mail]>  
> > > > >>> +CC Rafael and linux-pm
> > > > >>>  
> > > > >>>>
> > > > >>>> On all systems where the rzg2l_adc driver is used, the ADC clocks are part
> > > > >>>> of a PM domain. The code that implements the PM domains support is in
> > > > >>>> drivers/clk/renesas/rzg2l-cpg.c, the functions of interest for this commit
> > > > >>>> being rzg2l_cpg_attach_dev() and rzg2l_cpg_deattach_dev(). The PM
> > > > >>>> domains support is registered with GENPD_FLAG_PM_CLK which, according to
> > > > >>>> the documentation, instructs genpd to use the PM clk framework while
> > > > >>>> powering on/off attached devices.
> > > > >>>>
> > > > >>>> During probe, the ADC device is attached to the PM domain
> > > > >>>> controlling the ADC clocks. Similarly, during removal, the ADC device is
> > > > >>>> 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 ADC 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 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() ->  
> > > > >>>
> > > > >>> So is the only real difference that in the code below you disable runtime pm
> > > > >>> before autosuspend?  
> > > > >>
> > > > >> No, the difference is that now, the driver specific runtime PM APIs are not
> > > > >> called anymore (through the pointed call stack) after the ADC was removed
> > > > >> from it's PM domain.  
> > > > >
> > > > > By my reading they are only not called now because you turn autosuspend off
> > > > > after disabling runtime PM.  
> > > >
> > > > Sorry, I wanted to say that the runtime PM APIs are not called anymore from
> > > > devm_action_release(), though this call stack:
> > > >
> > > > [   24.801195] Call trace:
> > > > [   24.803633]  rzg2l_adc_pm_runtime_suspend+0x18/0x54 (P)
> > > > [   24.808847]  pm_generic_runtime_suspend+0x2c/0x44 (L)
> > > > [   24.813887]  pm_generic_runtime_suspend+0x2c/0x44
> > > > [   24.818580]  __rpm_callback+0x48/0x198
> > > > [   24.822319]  rpm_callback+0x68/0x74
> > > > [   24.825798]  rpm_suspend+0x100/0x578
> > > > [   24.829362]  rpm_idle+0xd0/0x17c
> > > > [   24.832582]  update_autosuspend+0x30/0xc4
> > > > [   24.836580]  pm_runtime_disable_action+0x40/0x64
> > > > [   24.841184]  devm_action_release+0x14/0x20
> > > > [   24.845274]  devres_release_all+0xa0/0x100
> > > > [   24.849361]  device_unbind_cleanup+0x18/0x60
> > > >
> > > > This is because I dropped the devm_pm_runtime_enable() which registers the
> > > > pm_runtime_disable_action(), which is called at the time the
> > > > device_unbind_cleanup() is called, which is called when the ADC is not
> > > > anymore part of its PM domain.
> > > >
> > > > If I change the order in remove function proposed in this patch, thus do:
> > > >
> > > > +static void rzg2l_adc_remove(struct platform_device *pdev)
> > > > +{
> > > > +       struct device *dev = &pdev->dev;
> > > > +
> > > > +       pm_runtime_dont_use_autosuspend(dev);
> > > > +       pm_runtime_disable(dev);
> > > >  }
> > > >
> > > > nothing changes with the behavior of this patch. There will be no issue if
> > > > the device is runtime suspended/resumed through the
> > > > pm_runtime_dont_use_autosuspend() because at the time the
> > > > rzg2l_adc_remove() is called the ADC is still part of the PM domain.
> > > >
> > > >
> > > >  
> > > > >  
> > > > >>
> > > > >>  
> > > > >>>  Can you still do that with a devm callback just not
> > > > >>> the standard one?  
> > > > >>
> > > > >> No. It doesn't matter if we call the standard devm callback or driver
> > > > >> specific one. As long as it is devm it will impact us as long as the driver
> > > > >> specific runtime PM APIs are called through devres_release_all() after
> > > > >> dev_pm_domain_detach(). And at that time the PM domain may be off along
> > > > >> with its clocks disabled.  
> > > > >
> > > > > As above, I think that this is only the case because of the reordering
> > > > > of those two calls, not something more fundamental.  
> > > >
> > > > I tried having a local devm function (the following diff applied with this
> > > > patch reverted) identical with pm_runtime_disable_action():
> > > >
> > > > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> > > > index 22a581c894f8..459cc9c67eec 100644
> > > > --- a/drivers/iio/adc/rzg2l_adc.c
> > > > +++ b/drivers/iio/adc/rzg2l_adc.c
> > > > @@ -423,6 +423,12 @@ static int rzg2l_adc_hw_init(struct device *dev,
> > > > struct rzg2l_adc *adc)
> > > >         return ret;
> > > >  }
> > > >
> > > > +static void rzg2l_pm_runtime_disable(void *data)
> > > > +{
> > > > +       pm_runtime_dont_use_autosuspend(data);
> > > > +       pm_runtime_disable(data);
> > > > +}
> > > > +
> > > >  static int rzg2l_adc_probe(struct platform_device *pdev)
> > > >  {
> > > >         struct device *dev = &pdev->dev;
> > > > @@ -463,7 +469,9 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
> > > >
> > > >         pm_runtime_set_autosuspend_delay(dev, 300);
> > > >         pm_runtime_use_autosuspend(dev);
> > > > -       ret = devm_pm_runtime_enable(dev);
> > > > +       pm_runtime_enable(dev);
> > > > +
> > > > +       ret = devm_add_action_or_reset(dev, rzg2l_pm_runtime_disable, dev);
> > > >         if (ret)
> > > >                 return ret;
> > > >
> > > > With this the issue is still reproducible.
> > > >
> > > > However, changing the order of functions in rzg2l_pm_runtime_disable() and
> > > > having it like:
> > > >
> > > > +static void rzg2l_pm_runtime_disable(void *data)
> > > > +{
> > > > +       pm_runtime_disable(data);
> > > > +       pm_runtime_dont_use_autosuspend(data);
> > > > +}
> > > >
> > > >
> > > > leads to no failure when doing unbind/bind.
> > > >
> > > > However, I see the pm_runtime_disable() can still call rpm_resume() under
> > > > certain conditions. It can still lead to failures if it is called after the
> > > > device was remove from its PM domain.
> > > >  
> > > > >
> > > > > In driver remove flow, device_unbind_cleanup9() is called
> > > > > just after device_remove() which is calling the dev->driver_remove()
> > > > > callback. There are no runtime pm related calls in between that I can see.  
> > > >
> > > > On my side the device_remove() is calling dev->bus->remove() which is
> > > > platform_remove(), which calls the dev_pm_domain_detach(). The
> > > > dev_pm_domain_detach() detaches the ADC from it's PM domain. Because of
> > > > this, accessing now the ADC registers after a runtime resume leads to
> > > > failures pointed in this patch (as of my investigation) (as the ADC is not
> > > > anymore part of its PM domain and its PM domain is not started anymore
> > > > though runtime PM APIs).
> > > >
> > > > A similar issue was found while I was adding thermal support for RZ/G3S,
> > > > explained in
> > > > https://lore.kernel.org/all/[e-mail]
> > > >
> > > >
> > > > Jonathan, Rafael, Ulf, all,
> > > >
> > > > Do consider OK to change the order in pm_runtime_disable_action() to get
> > > > rid of these issues, e.g.:
> > > >
> > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > index 2ee45841486b..f27d311d2619 100644
> > > > --- a/drivers/base/power/runtime.c
> > > > +++ b/drivers/base/power/runtime.c
> > > > @@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> > > >
> > > >  static void pm_runtime_disable_action(void *data)
> > > >  {
> > > > -       pm_runtime_dont_use_autosuspend(data);
> > > >         pm_runtime_disable(data);
> > > > +       pm_runtime_dont_use_autosuspend(data);
> > > >  }
> > > >
> > > > though I see a rpm_resume() call is still possible though pm_runtime_disable().  
> > >
> > > I am still worried about keeping the device runtime enabled during a
> > > window when we have turned off all resources for the device. Typically
> > > we want to leave the device in a low power state after unbind.
> > >
> > > That said, I would rather just drop the devm_pm_runtime_enable() API
> > > altogether and convert all users of it into
> > > pm_runtime_enable|disable(), similar to what your patch does.  
> >
> > That is making a mess of a lot of automated cleanup for a strange
> > runtime pm related path.  This is pain a driver should not have
> > to deal with, though I'm not clear what the right solution is!
> >
> > Key is that drivers should not mix devm managed cleanup and not, so
> > that means that anything that happens after runtime pm is enabled
> > has to be torn down manually.  One solution to this might be to
> > always enable it late assuming that is safe to do so there is
> > never anything else done after it in the probe path of a driver.  
> 
> The problem is that runtime PM isn't really comparable to other
> resources that we are managing through devm* functions.
> 
> Enabling runtime PM for a device changes the behaviour for how
> power-mgmt is handled for the device. Enabling/disabling of runtime PM
> really needs to be explicitly controlled by the driver for the device.

I'm sorry to say I'm not yet convinced.

Devm callbacks are explicitly registered by the driver so that they
are unwound in a specific order.  Many other parts of driver
registration rely on this ordering.  This does not seem different
for runtime PM than anything else.

Superficially the issue here looks to me to be that a non devm
cleanup is inserted by the bus->remove() callback into drivers
that are otherwise relying on explicit ordering provided
by managed cleanup.

I appreciate there may be no trivial solution.

Maybe we can minimize that impact by always doing runtime pm last
in any probe() function?  Can that work here?


> 
> If there are cleanups to be made for runtime PM, beyond disabling
> runtime PM, we could instead consider adding that code in
> pm_runtime_reinit().

I'm not familiar enough to comment on this option beyond it being
after devres_release_all() so, maybe?  It seems superficially
a better point in the sequence.

Jonathan
> 
> [...]
> 
> Kind regards
> Uffe

Claudiu Beznea Jan. 27, 2025, 8:30 a.m. UTC | #11

On 24.01.2025 20:41, Jonathan Cameron wrote:
> On Mon, 20 Jan 2025 15:59:02 +0100
> Ulf Hansson <[e-mail]> wrote:
> 
>> On Fri, 17 Jan 2025 at 16:52, Jonathan Cameron
>> <[e-mail]> wrote:
>>>
>>> On Wed, 15 Jan 2025 16:29:15 +0100
>>> Ulf Hansson <[e-mail]> wrote:
>>>  
>>>> On Wed, 15 Jan 2025 at 14:37, Claudiu Beznea <[e-mail]> wrote:  
>>>>>
>>>>> Hi, Jonathan,
>>>>>
>>>>> Thank you for your input!
>>>>>
>>>>> On 11.01.2025 15:14, Jonathan Cameron wrote:  
>>>>>> On Mon, 6 Jan 2025 11:18:41 +0200
>>>>>> Claudiu Beznea <[e-mail]> wrote:
>>>>>>  
>>>>>>> Hi, Jonathan,
>>>>>>>
>>>>>>>
>>>>>>> On 04.01.2025 15:52, Jonathan Cameron wrote:  
>>>>>>>> On Fri,  3 Jan 2025 16:00:41 +0200
>>>>>>>> Claudiu <[e-mail]> wrote:
>>>>>>>>  
>>>>>>>>> From: Claudiu Beznea <[e-mail]>  
>>>>>>>> +CC Rafael and linux-pm
>>>>>>>>  
>>>>>>>>>
>>>>>>>>> On all systems where the rzg2l_adc driver is used, the ADC clocks are part
>>>>>>>>> of a PM domain. The code that implements the PM domains support is in
>>>>>>>>> drivers/clk/renesas/rzg2l-cpg.c, the functions of interest for this commit
>>>>>>>>> being rzg2l_cpg_attach_dev() and rzg2l_cpg_deattach_dev(). The PM
>>>>>>>>> domains support is registered with GENPD_FLAG_PM_CLK which, according to
>>>>>>>>> the documentation, instructs genpd to use the PM clk framework while
>>>>>>>>> powering on/off attached devices.
>>>>>>>>>
>>>>>>>>> During probe, the ADC device is attached to the PM domain
>>>>>>>>> controlling the ADC clocks. Similarly, during removal, the ADC device is
>>>>>>>>> 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 ADC 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 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() ->  
>>>>>>>>
>>>>>>>> So is the only real difference that in the code below you disable runtime pm
>>>>>>>> before autosuspend?  
>>>>>>>
>>>>>>> No, the difference is that now, the driver specific runtime PM APIs are not
>>>>>>> called anymore (through the pointed call stack) after the ADC was removed
>>>>>>> from it's PM domain.  
>>>>>>
>>>>>> By my reading they are only not called now because you turn autosuspend off
>>>>>> after disabling runtime PM.  
>>>>>
>>>>> Sorry, I wanted to say that the runtime PM APIs are not called anymore from
>>>>> devm_action_release(), though this call stack:
>>>>>
>>>>> [   24.801195] Call trace:
>>>>> [   24.803633]  rzg2l_adc_pm_runtime_suspend+0x18/0x54 (P)
>>>>> [   24.808847]  pm_generic_runtime_suspend+0x2c/0x44 (L)
>>>>> [   24.813887]  pm_generic_runtime_suspend+0x2c/0x44
>>>>> [   24.818580]  __rpm_callback+0x48/0x198
>>>>> [   24.822319]  rpm_callback+0x68/0x74
>>>>> [   24.825798]  rpm_suspend+0x100/0x578
>>>>> [   24.829362]  rpm_idle+0xd0/0x17c
>>>>> [   24.832582]  update_autosuspend+0x30/0xc4
>>>>> [   24.836580]  pm_runtime_disable_action+0x40/0x64
>>>>> [   24.841184]  devm_action_release+0x14/0x20
>>>>> [   24.845274]  devres_release_all+0xa0/0x100
>>>>> [   24.849361]  device_unbind_cleanup+0x18/0x60
>>>>>
>>>>> This is because I dropped the devm_pm_runtime_enable() which registers the
>>>>> pm_runtime_disable_action(), which is called at the time the
>>>>> device_unbind_cleanup() is called, which is called when the ADC is not
>>>>> anymore part of its PM domain.
>>>>>
>>>>> If I change the order in remove function proposed in this patch, thus do:
>>>>>
>>>>> +static void rzg2l_adc_remove(struct platform_device *pdev)
>>>>> +{
>>>>> +       struct device *dev = &pdev->dev;
>>>>> +
>>>>> +       pm_runtime_dont_use_autosuspend(dev);
>>>>> +       pm_runtime_disable(dev);
>>>>>  }
>>>>>
>>>>> nothing changes with the behavior of this patch. There will be no issue if
>>>>> the device is runtime suspended/resumed through the
>>>>> pm_runtime_dont_use_autosuspend() because at the time the
>>>>> rzg2l_adc_remove() is called the ADC is still part of the PM domain.
>>>>>
>>>>>
>>>>>  
>>>>>>  
>>>>>>>
>>>>>>>  
>>>>>>>>  Can you still do that with a devm callback just not
>>>>>>>> the standard one?  
>>>>>>>
>>>>>>> No. It doesn't matter if we call the standard devm callback or driver
>>>>>>> specific one. As long as it is devm it will impact us as long as the driver
>>>>>>> specific runtime PM APIs are called through devres_release_all() after
>>>>>>> dev_pm_domain_detach(). And at that time the PM domain may be off along
>>>>>>> with its clocks disabled.  
>>>>>>
>>>>>> As above, I think that this is only the case because of the reordering
>>>>>> of those two calls, not something more fundamental.  
>>>>>
>>>>> I tried having a local devm function (the following diff applied with this
>>>>> patch reverted) identical with pm_runtime_disable_action():
>>>>>
>>>>> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
>>>>> index 22a581c894f8..459cc9c67eec 100644
>>>>> --- a/drivers/iio/adc/rzg2l_adc.c
>>>>> +++ b/drivers/iio/adc/rzg2l_adc.c
>>>>> @@ -423,6 +423,12 @@ static int rzg2l_adc_hw_init(struct device *dev,
>>>>> struct rzg2l_adc *adc)
>>>>>         return ret;
>>>>>  }
>>>>>
>>>>> +static void rzg2l_pm_runtime_disable(void *data)
>>>>> +{
>>>>> +       pm_runtime_dont_use_autosuspend(data);
>>>>> +       pm_runtime_disable(data);
>>>>> +}
>>>>> +
>>>>>  static int rzg2l_adc_probe(struct platform_device *pdev)
>>>>>  {
>>>>>         struct device *dev = &pdev->dev;
>>>>> @@ -463,7 +469,9 @@ static int rzg2l_adc_probe(struct platform_device *pdev)
>>>>>
>>>>>         pm_runtime_set_autosuspend_delay(dev, 300);
>>>>>         pm_runtime_use_autosuspend(dev);
>>>>> -       ret = devm_pm_runtime_enable(dev);
>>>>> +       pm_runtime_enable(dev);
>>>>> +
>>>>> +       ret = devm_add_action_or_reset(dev, rzg2l_pm_runtime_disable, dev);
>>>>>         if (ret)
>>>>>                 return ret;
>>>>>
>>>>> With this the issue is still reproducible.
>>>>>
>>>>> However, changing the order of functions in rzg2l_pm_runtime_disable() and
>>>>> having it like:
>>>>>
>>>>> +static void rzg2l_pm_runtime_disable(void *data)
>>>>> +{
>>>>> +       pm_runtime_disable(data);
>>>>> +       pm_runtime_dont_use_autosuspend(data);
>>>>> +}
>>>>>
>>>>>
>>>>> leads to no failure when doing unbind/bind.
>>>>>
>>>>> However, I see the pm_runtime_disable() can still call rpm_resume() under
>>>>> certain conditions. It can still lead to failures if it is called after the
>>>>> device was remove from its PM domain.
>>>>>  
>>>>>>
>>>>>> In driver remove flow, device_unbind_cleanup9() is called
>>>>>> just after device_remove() which is calling the dev->driver_remove()
>>>>>> callback. There are no runtime pm related calls in between that I can see.  
>>>>>
>>>>> On my side the device_remove() is calling dev->bus->remove() which is
>>>>> platform_remove(), which calls the dev_pm_domain_detach(). The
>>>>> dev_pm_domain_detach() detaches the ADC from it's PM domain. Because of
>>>>> this, accessing now the ADC registers after a runtime resume leads to
>>>>> failures pointed in this patch (as of my investigation) (as the ADC is not
>>>>> anymore part of its PM domain and its PM domain is not started anymore
>>>>> though runtime PM APIs).
>>>>>
>>>>> A similar issue was found while I was adding thermal support for RZ/G3S,
>>>>> explained in
>>>>> https://lore.kernel.org/all/[e-mail]
>>>>>
>>>>>
>>>>> Jonathan, Rafael, Ulf, all,
>>>>>
>>>>> Do consider OK to change the order in pm_runtime_disable_action() to get
>>>>> rid of these issues, e.g.:
>>>>>
>>>>> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
>>>>> index 2ee45841486b..f27d311d2619 100644
>>>>> --- a/drivers/base/power/runtime.c
>>>>> +++ b/drivers/base/power/runtime.c
>>>>> @@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
>>>>>
>>>>>  static void pm_runtime_disable_action(void *data)
>>>>>  {
>>>>> -       pm_runtime_dont_use_autosuspend(data);
>>>>>         pm_runtime_disable(data);
>>>>> +       pm_runtime_dont_use_autosuspend(data);
>>>>>  }
>>>>>
>>>>> though I see a rpm_resume() call is still possible though pm_runtime_disable().  
>>>>
>>>> I am still worried about keeping the device runtime enabled during a
>>>> window when we have turned off all resources for the device. Typically
>>>> we want to leave the device in a low power state after unbind.
>>>>
>>>> That said, I would rather just drop the devm_pm_runtime_enable() API
>>>> altogether and convert all users of it into
>>>> pm_runtime_enable|disable(), similar to what your patch does.  
>>>
>>> That is making a mess of a lot of automated cleanup for a strange
>>> runtime pm related path.  This is pain a driver should not have
>>> to deal with, though I'm not clear what the right solution is!
>>>
>>> Key is that drivers should not mix devm managed cleanup and not, so
>>> that means that anything that happens after runtime pm is enabled
>>> has to be torn down manually.  One solution to this might be to
>>> always enable it late assuming that is safe to do so there is
>>> never anything else done after it in the probe path of a driver.  
>>
>> The problem is that runtime PM isn't really comparable to other
>> resources that we are managing through devm* functions.
>>
>> Enabling runtime PM for a device changes the behaviour for how
>> power-mgmt is handled for the device. Enabling/disabling of runtime PM
>> really needs to be explicitly controlled by the driver for the device.
> 
> I'm sorry to say I'm not yet convinced.
> 
> Devm callbacks are explicitly registered by the driver so that they
> are unwound in a specific order.  Many other parts of driver
> registration rely on this ordering.  This does not seem different
> for runtime PM than anything else.
> 
> Superficially the issue here looks to me to be that a non devm
> cleanup is inserted by the bus->remove() callback into drivers
> that are otherwise relying on explicit ordering provided
> by managed cleanup.
> 
> I appreciate there may be no trivial solution.
> 
> Maybe we can minimize that impact by always doing runtime pm last
> in any probe() function?  Can that work here?

It will not work, AFAICT. E.g., some hardware need initialization before,
e.g., registering a device for it in the proper subsystem. Runtime PM may
help with enabling, e.g., clocks, for that hardware initialization to work.

> 
> 
>>
>> If there are cleanups to be made for runtime PM, beyond disabling
>> runtime PM, we could instead consider adding that code in
>> pm_runtime_reinit().
> 
> I'm not familiar enough to comment on this option beyond it being
> after devres_release_all() so, maybe?  It seems superficially
> a better point in the sequence.

I only need disabling runtime PM in my usecase.

One thing that I've experimented with is to drop the dev_pm_domain_detach()
call from platform_remove() and add it in device_unbind_cleanup(). I
checked with this diff:

1/

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index f0e4b4aba885..e658c2d642be 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -544,6 +544,7 @@ static ssize_t state_synced_show(struct device *dev,
 }
 static DEVICE_ATTR_RW(state_synced);

+extern void dev_pm_domain_detach(struct device *dev, bool power_off);
 static void device_unbind_cleanup(struct device *dev)
 {
        devres_release_all(dev);
@@ -554,6 +555,7 @@ static void device_unbind_cleanup(struct device *dev)
        dev_set_drvdata(dev, NULL);
        if (dev->pm_domain && dev->pm_domain->dismiss)
                dev->pm_domain->dismiss(dev);
+       dev_pm_domain_detach(dev, true);
        pm_runtime_reinit(dev);
        dev_pm_set_driver_flags(dev, 0);
 }
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 6f2a33722c52..bf2f8d39d184 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1422,7 +1422,6 @@ static void platform_remove(struct device *_dev)

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

And also, with this one:

2/

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index f0e4b4aba885..216d765fbfcd 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -544,9 +544,11 @@ static ssize_t state_synced_show(struct device *dev,
 }
 static DEVICE_ATTR_RW(state_synced);

+extern void dev_pm_domain_detach(struct device *dev, bool power_off);
 static void device_unbind_cleanup(struct device *dev)
 {
        devres_release_all(dev);
+       dev_pm_domain_detach(dev, true);
        arch_teardown_dma_ops(dev);
        kfree(dev->dma_range_map);
        dev->dma_range_map = NULL;
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 6f2a33722c52..bf2f8d39d184 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1422,7 +1422,6 @@ static void platform_remove(struct device *_dev)

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

Both of these work on my side (when doing continuous unbind/bind) but I'm
not sure what are the implications of it. If all good, maybe we can add a
devm helper for dev_pm_domain_attach() and use it in platform_probe(). With
this it will be guaranteed that the devm_pm_domain_detach() will be the
last cleanup helper that will be called on remove for the platform bus.
What do you think?

Thank you,
Claudiu

> 
> Jonathan
>>
>> [...]
>>
>> Kind regards
>> Uffe
>

Ulf Hansson Jan. 27, 2025, 10:47 a.m. UTC | #12

[...]

> > > > > Do consider OK to change the order in pm_runtime_disable_action() to get
> > > > > rid of these issues, e.g.:
> > > > >
> > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > index 2ee45841486b..f27d311d2619 100644
> > > > > --- a/drivers/base/power/runtime.c
> > > > > +++ b/drivers/base/power/runtime.c
> > > > > @@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> > > > >
> > > > >  static void pm_runtime_disable_action(void *data)
> > > > >  {
> > > > > -       pm_runtime_dont_use_autosuspend(data);
> > > > >         pm_runtime_disable(data);
> > > > > +       pm_runtime_dont_use_autosuspend(data);
> > > > >  }
> > > > >
> > > > > though I see a rpm_resume() call is still possible though pm_runtime_disable().
> > > >
> > > > I am still worried about keeping the device runtime enabled during a
> > > > window when we have turned off all resources for the device. Typically
> > > > we want to leave the device in a low power state after unbind.
> > > >
> > > > That said, I would rather just drop the devm_pm_runtime_enable() API
> > > > altogether and convert all users of it into
> > > > pm_runtime_enable|disable(), similar to what your patch does.
> > >
> > > That is making a mess of a lot of automated cleanup for a strange
> > > runtime pm related path.  This is pain a driver should not have
> > > to deal with, though I'm not clear what the right solution is!
> > >
> > > Key is that drivers should not mix devm managed cleanup and not, so
> > > that means that anything that happens after runtime pm is enabled
> > > has to be torn down manually.  One solution to this might be to
> > > always enable it late assuming that is safe to do so there is
> > > never anything else done after it in the probe path of a driver.
> >
> > The problem is that runtime PM isn't really comparable to other
> > resources that we are managing through devm* functions.
> >
> > Enabling runtime PM for a device changes the behaviour for how
> > power-mgmt is handled for the device. Enabling/disabling of runtime PM
> > really needs to be explicitly controlled by the driver for the device.
>
> I'm sorry to say I'm not yet convinced.

Okay, let me try one more time. :-)

>
> Devm callbacks are explicitly registered by the driver so that they
> are unwound in a specific order.  Many other parts of driver
> registration rely on this ordering.  This does not seem different
> for runtime PM than anything else.

If you compare clocks, for example. It's the driver that is in full
control of the clock gating/ungating. When the ->remove() callback
runs, the driver typically makes sure that it leaves the clock gated.
Then it doesn't really matter when the clock resource gets released.
The point is, the driver is in full control of the resource.

If runtime PM would remain enabled beyond the call to the ->remove()
callback, it would mean that the driver's runtime PM callbacks could
be called too. For example, userspace via sysfs may at any point
decide to runtime resume the device. In other words, we may end up
calling the runtime PM callbacks in the driver, when they are not
intended to be called. In the worst case, I guess we could even end up
trying to control resources (like a clock) from the ->runtime
_resume() callback, when the references to these resources may already
have been released.

>
> Superficially the issue here looks to me to be that a non devm
> cleanup is inserted by the bus->remove() callback into drivers
> that are otherwise relying on explicit ordering provided
> by managed cleanup.
>
> I appreciate there may be no trivial solution.
>
> Maybe we can minimize that impact by always doing runtime pm last
> in any probe() function?  Can that work here?
>
>
> >
> > If there are cleanups to be made for runtime PM, beyond disabling
> > runtime PM, we could instead consider adding that code in
> > pm_runtime_reinit().
>
> I'm not familiar enough to comment on this option beyond it being
> after devres_release_all() so, maybe?  It seems superficially
> a better point in the sequence.
>
> Jonathan
> >
> > [...]

Kind regards
Uffe

Jonathan Cameron Jan. 27, 2025, 12:32 p.m. UTC | #13

On Mon, 27 Jan 2025 11:47:44 +0100
Ulf Hansson <[e-mail]> wrote:

> [...]
> 
> > > > > > Do consider OK to change the order in pm_runtime_disable_action() to get
> > > > > > rid of these issues, e.g.:
> > > > > >
> > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > index 2ee45841486b..f27d311d2619 100644
> > > > > > --- a/drivers/base/power/runtime.c
> > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > @@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> > > > > >
> > > > > >  static void pm_runtime_disable_action(void *data)
> > > > > >  {
> > > > > > -       pm_runtime_dont_use_autosuspend(data);
> > > > > >         pm_runtime_disable(data);
> > > > > > +       pm_runtime_dont_use_autosuspend(data);
> > > > > >  }
> > > > > >
> > > > > > though I see a rpm_resume() call is still possible though pm_runtime_disable().  
> > > > >
> > > > > I am still worried about keeping the device runtime enabled during a
> > > > > window when we have turned off all resources for the device. Typically
> > > > > we want to leave the device in a low power state after unbind.
> > > > >
> > > > > That said, I would rather just drop the devm_pm_runtime_enable() API
> > > > > altogether and convert all users of it into
> > > > > pm_runtime_enable|disable(), similar to what your patch does.  
> > > >
> > > > That is making a mess of a lot of automated cleanup for a strange
> > > > runtime pm related path.  This is pain a driver should not have
> > > > to deal with, though I'm not clear what the right solution is!
> > > >
> > > > Key is that drivers should not mix devm managed cleanup and not, so
> > > > that means that anything that happens after runtime pm is enabled
> > > > has to be torn down manually.  One solution to this might be to
> > > > always enable it late assuming that is safe to do so there is
> > > > never anything else done after it in the probe path of a driver.  
> > >
> > > The problem is that runtime PM isn't really comparable to other
> > > resources that we are managing through devm* functions.
> > >
> > > Enabling runtime PM for a device changes the behaviour for how
> > > power-mgmt is handled for the device. Enabling/disabling of runtime PM
> > > really needs to be explicitly controlled by the driver for the device.  
> >
> > I'm sorry to say I'm not yet convinced.  
> 
> Okay, let me try one more time. :-)

+CC Greg as the disagreement here is really a philosophy of what
devm cleanup is relative to remove.  Perhaps Greg or Rafael can
given some guidance on the intent there.

Mind you I think I found another subsystem working around this
and in a somewhat more elegant, general way (to my eyes anyway!)

https://elixir.bootlin.com/linux/v6.12.6/source/drivers/i2c/i2c-core-base.c#L630
https://lore.kernel.org/all/[e-mail]/

+CC Dmitry.

I2C creates an extra devres group and releases it before devm_pm_domain_detach()
As all devm calls from the driver end up in that group, they are released
before dev_pm_domain_detach()


> 
> >
> > Devm callbacks are explicitly registered by the driver so that they
> > are unwound in a specific order.  Many other parts of driver
> > registration rely on this ordering.  This does not seem different
> > for runtime PM than anything else.  
> 
> If you compare clocks, for example. It's the driver that is in full
> control of the clock gating/ungating. When the ->remove() callback
> runs, the driver typically makes sure that it leaves the clock gated.
> Then it doesn't really matter when the clock resource gets released.
> The point is, the driver is in full control of the resource.

Not a good example. devm_clk_get_enabled() does not gate the clock until
the devm cleanup. The assumption being that nothing that affects
it runs between the remove() and devm cleanup.  So pretty much identical
to the runtime pm case.  They being that you have to obey ordering so
that if you need to run something after the clock is disabled then
you register that callback before you call devm_clk_get_enabled()

> 
> If runtime PM would remain enabled beyond the call to the ->remove()
> callback, it would mean that the driver's runtime PM callbacks could
> be called too. For example, userspace via sysfs may at any point
> decide to runtime resume the device. In other words, we may end up
> calling the runtime PM callbacks in the driver, when they are not
> intended to be called. In the worst case, I guess we could even end up
> trying to control resources (like a clock) from the ->runtime
> _resume() callback, when the references to these resources may already
> have been released.

This is all about what we consider remove. To me, with devm_ manged cleanup
in place, both remove() and devm_ cleanup count as parts of that remove
process.  

One option I did wonder about was having a devm_pm_domain_attach()
A little cheeky but I think the call in platform_probe() is late enough
that we don't run into the checks on no devm_ calls before driver probe.

That would shuffle the dev_pm_domain_detach() to the end of the
devm_ cleanup.  Mind you the i2c approach above seems better.

Jonathan

> 
> >
> > Superficially the issue here looks to me to be that a non devm
> > cleanup is inserted by the bus->remove() callback into drivers
> > that are otherwise relying on explicit ordering provided
> > by managed cleanup.
> >
> > I appreciate there may be no trivial solution.
> >
> > Maybe we can minimize that impact by always doing runtime pm last
> > in any probe() function?  Can that work here?
> >
> >  
> > >
> > > If there are cleanups to be made for runtime PM, beyond disabling
> > > runtime PM, we could instead consider adding that code in
> > > pm_runtime_reinit().  
> >
> > I'm not familiar enough to comment on this option beyond it being
> > after devres_release_all() so, maybe?  It seems superficially
> > a better point in the sequence.
> >
> > Jonathan  
> > >
> > > [...]  
> 
> Kind regards
> Uffe

Ulf Hansson Jan. 27, 2025, 3:02 p.m. UTC | #14

On Mon, 27 Jan 2025 at 13:32, Jonathan Cameron
<[e-mail]> wrote:
>
> On Mon, 27 Jan 2025 11:47:44 +0100
> Ulf Hansson <[e-mail]> wrote:
>
> > [...]
> >
> > > > > > > Do consider OK to change the order in pm_runtime_disable_action() to get
> > > > > > > rid of these issues, e.g.:
> > > > > > >
> > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > > index 2ee45841486b..f27d311d2619 100644
> > > > > > > --- a/drivers/base/power/runtime.c
> > > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > > @@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> > > > > > >
> > > > > > >  static void pm_runtime_disable_action(void *data)
> > > > > > >  {
> > > > > > > -       pm_runtime_dont_use_autosuspend(data);
> > > > > > >         pm_runtime_disable(data);
> > > > > > > +       pm_runtime_dont_use_autosuspend(data);
> > > > > > >  }
> > > > > > >
> > > > > > > though I see a rpm_resume() call is still possible though pm_runtime_disable().
> > > > > >
> > > > > > I am still worried about keeping the device runtime enabled during a
> > > > > > window when we have turned off all resources for the device. Typically
> > > > > > we want to leave the device in a low power state after unbind.
> > > > > >
> > > > > > That said, I would rather just drop the devm_pm_runtime_enable() API
> > > > > > altogether and convert all users of it into
> > > > > > pm_runtime_enable|disable(), similar to what your patch does.
> > > > >
> > > > > That is making a mess of a lot of automated cleanup for a strange
> > > > > runtime pm related path.  This is pain a driver should not have
> > > > > to deal with, though I'm not clear what the right solution is!
> > > > >
> > > > > Key is that drivers should not mix devm managed cleanup and not, so
> > > > > that means that anything that happens after runtime pm is enabled
> > > > > has to be torn down manually.  One solution to this might be to
> > > > > always enable it late assuming that is safe to do so there is
> > > > > never anything else done after it in the probe path of a driver.
> > > >
> > > > The problem is that runtime PM isn't really comparable to other
> > > > resources that we are managing through devm* functions.
> > > >
> > > > Enabling runtime PM for a device changes the behaviour for how
> > > > power-mgmt is handled for the device. Enabling/disabling of runtime PM
> > > > really needs to be explicitly controlled by the driver for the device.
> > >
> > > I'm sorry to say I'm not yet convinced.
> >
> > Okay, let me try one more time. :-)
>
> +CC Greg as the disagreement here is really a philosophy of what
> devm cleanup is relative to remove.  Perhaps Greg or Rafael can
> given some guidance on the intent there.
>
> Mind you I think I found another subsystem working around this
> and in a somewhat more elegant, general way (to my eyes anyway!)
>
> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/i2c/i2c-core-base.c#L630
> https://lore.kernel.org/all/[e-mail]/
>
> +CC Dmitry.
>
> I2C creates an extra devres group and releases it before devm_pm_domain_detach()
> As all devm calls from the driver end up in that group, they are released
> before dev_pm_domain_detach()
>

How would that address the problem I pointed out with runtime PM
below? This problem isn't limited to attaching/detaching PM domains.

>
> >
> > >
> > > Devm callbacks are explicitly registered by the driver so that they
> > > are unwound in a specific order.  Many other parts of driver
> > > registration rely on this ordering.  This does not seem different
> > > for runtime PM than anything else.
> >
> > If you compare clocks, for example. It's the driver that is in full
> > control of the clock gating/ungating. When the ->remove() callback
> > runs, the driver typically makes sure that it leaves the clock gated.
> > Then it doesn't really matter when the clock resource gets released.
> > The point is, the driver is in full control of the resource.
>
> Not a good example. devm_clk_get_enabled() does not gate the clock until

I was not referring to devm_clk_get_enable(), but rather just devm_clk_get().

To me devm_clk_get_enable() is another interface that we should avoid.
For example, what if the clock is already gated when the ->remove()
callback runs? Then we need to ungate the clock just to make the
devres path happy so it doesn't gate an already gated clock. And this,
just to save one or two lines of code.

Don't get me wrong, I certainly like the devm* functions in general,
but it's not a good fit for everything.

> the devm cleanup. The assumption being that nothing that affects
> it runs between the remove() and devm cleanup.  So pretty much identical
> to the runtime pm case.  They being that you have to obey ordering so
> that if you need to run something after the clock is disabled then
> you register that callback before you call devm_clk_get_enabled()
>
> >
> > If runtime PM would remain enabled beyond the call to the ->remove()
> > callback, it would mean that the driver's runtime PM callbacks could
> > be called too. For example, userspace via sysfs may at any point
> > decide to runtime resume the device. In other words, we may end up
> > calling the runtime PM callbacks in the driver, when they are not
> > intended to be called. In the worst case, I guess we could even end up
> > trying to control resources (like a clock) from the ->runtime
> > _resume() callback, when the references to these resources may already
> > have been released.
>
> This is all about what we consider remove. To me, with devm_ manged cleanup
> in place, both remove() and devm_ cleanup count as parts of that remove
> process.

There is no straightforward process here, if you would keep runtime PM
enabled beyond ->remove(). Things can happen in parallel.

In that case, drivers would need to extend their runtime PM callbacks
to cope with more complicated conditions, as resources that those use
may have been released. Moreover, how can we make sure that the device
is put into a low power state after the ->remove() has been called?

>
> One option I did wonder about was having a devm_pm_domain_attach()
> A little cheeky but I think the call in platform_probe() is late enough
> that we don't run into the checks on no devm_ calls before driver probe.
>
> That would shuffle the dev_pm_domain_detach() to the end of the
> devm_ cleanup.  Mind you the i2c approach above seems better.

Again, this isn't limited to PM domains.

[...]

Kind regards
Uffe

Jonathan Cameron Jan. 27, 2025, 6:24 p.m. UTC | #15

On Mon, 27 Jan 2025 16:02:32 +0100
Ulf Hansson <[e-mail]> wrote:

> On Mon, 27 Jan 2025 at 13:32, Jonathan Cameron
> <[e-mail]> wrote:
> >
> > On Mon, 27 Jan 2025 11:47:44 +0100
> > Ulf Hansson <[e-mail]> wrote:
> >  
> > > [...]
> > >  
> > > > > > > > Do consider OK to change the order in pm_runtime_disable_action() to get
> > > > > > > > rid of these issues, e.g.:
> > > > > > > >
> > > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > > > index 2ee45841486b..f27d311d2619 100644
> > > > > > > > --- a/drivers/base/power/runtime.c
> > > > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > > > @@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> > > > > > > >
> > > > > > > >  static void pm_runtime_disable_action(void *data)
> > > > > > > >  {
> > > > > > > > -       pm_runtime_dont_use_autosuspend(data);
> > > > > > > >         pm_runtime_disable(data);
> > > > > > > > +       pm_runtime_dont_use_autosuspend(data);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > though I see a rpm_resume() call is still possible though pm_runtime_disable().  
> > > > > > >
> > > > > > > I am still worried about keeping the device runtime enabled during a
> > > > > > > window when we have turned off all resources for the device. Typically
> > > > > > > we want to leave the device in a low power state after unbind.
> > > > > > >
> > > > > > > That said, I would rather just drop the devm_pm_runtime_enable() API
> > > > > > > altogether and convert all users of it into
> > > > > > > pm_runtime_enable|disable(), similar to what your patch does.  
> > > > > >
> > > > > > That is making a mess of a lot of automated cleanup for a strange
> > > > > > runtime pm related path.  This is pain a driver should not have
> > > > > > to deal with, though I'm not clear what the right solution is!
> > > > > >
> > > > > > Key is that drivers should not mix devm managed cleanup and not, so
> > > > > > that means that anything that happens after runtime pm is enabled
> > > > > > has to be torn down manually.  One solution to this might be to
> > > > > > always enable it late assuming that is safe to do so there is
> > > > > > never anything else done after it in the probe path of a driver.  
> > > > >
> > > > > The problem is that runtime PM isn't really comparable to other
> > > > > resources that we are managing through devm* functions.
> > > > >
> > > > > Enabling runtime PM for a device changes the behaviour for how
> > > > > power-mgmt is handled for the device. Enabling/disabling of runtime PM
> > > > > really needs to be explicitly controlled by the driver for the device.  
> > > >
> > > > I'm sorry to say I'm not yet convinced.  
> > >
> > > Okay, let me try one more time. :-)  
> >
> > +CC Greg as the disagreement here is really a philosophy of what
> > devm cleanup is relative to remove.  Perhaps Greg or Rafael can
> > given some guidance on the intent there.
> >
> > Mind you I think I found another subsystem working around this
> > and in a somewhat more elegant, general way (to my eyes anyway!)
> >
> > https://elixir.bootlin.com/linux/v6.12.6/source/drivers/i2c/i2c-core-base.c#L630
> > https://lore.kernel.org/all/[e-mail]/
> >
> > +CC Dmitry.
> >
> > I2C creates an extra devres group and releases it before devm_pm_domain_detach()
> > As all devm calls from the driver end up in that group, they are released
> > before dev_pm_domain_detach()
> >  
> 
> How would that address the problem I pointed out with runtime PM
> below? This problem isn't limited to attaching/detaching PM domains.

It's associated with anything that happens after a driver remove is done.
We just disagree on when that remove is finished. There is nothing special about
the remove() callback, that is just part of remove process.
No magic transition of state that allows new things to happen follows
the device driver remove finishing. Sure you can get the remove
handling ordering wrong whether devm is in use or not.  The trick is
almost always to never mix devm and not.  Once you need a single bit of
manual unwinding stop with the devm and do everything beyond that point
by hand (in probe order, before that point in remove order)

> 
> >  
> > >  
> > > >
> > > > Devm callbacks are explicitly registered by the driver so that they
> > > > are unwound in a specific order.  Many other parts of driver
> > > > registration rely on this ordering.  This does not seem different
> > > > for runtime PM than anything else.  
> > >
> > > If you compare clocks, for example. It's the driver that is in full
> > > control of the clock gating/ungating. When the ->remove() callback
> > > runs, the driver typically makes sure that it leaves the clock gated.
> > > Then it doesn't really matter when the clock resource gets released.
> > > The point is, the driver is in full control of the resource.  
> >
> > Not a good example. devm_clk_get_enabled() does not gate the clock until  
> 
> I was not referring to devm_clk_get_enable(), but rather just devm_clk_get().
> 
> To me devm_clk_get_enable() is another interface that we should avoid.
> For example, what if the clock is already gated when the ->remove()
> callback runs? Then we need to ungate the clock just to make the
> devres path happy so it doesn't gate an already gated clock. And this,
> just to save one or two lines of code.

If someone is using a clock that is gated by other calls in the driver
that then indeed the use of devm_clk_get_enabled() is inappropriate
or they turn they do need to enable the clock and turn it off again
as you mention which is a mess but often needs doing anyway as we
commonly need some clocks at least to put a device into a low power
state.

> 
> Don't get me wrong, I certainly like the devm* functions in general,
> but it's not a good fit for everything.

For anything that a driver otherwise calls in remove() they are
a direct equivalent that is just called automatically. (though apparently
not quite in platform drivers!) If you have a sequence that is
sufficiently complex they may not be a good fit. I just don't think
that the sequence in this driver (and many others) is complex.
The driver code (with the above change from i2c ported to platform code
to fix the specific problem) is a lot simpler before Claudia's v2 to
change the handling. 32 lines added to work around this...

> 
> > the devm cleanup. The assumption being that nothing that affects
> > it runs between the remove() and devm cleanup.  So pretty much identical
> > to the runtime pm case.  They being that you have to obey ordering so
> > that if you need to run something after the clock is disabled then
> > you register that callback before you call devm_clk_get_enabled()
> >  
> > >
> > > If runtime PM would remain enabled beyond the call to the ->remove()
> > > callback, it would mean that the driver's runtime PM callbacks could
> > > be called too. For example, userspace via sysfs may at any point
> > > decide to runtime resume the device. In other words, we may end up
> > > calling the runtime PM callbacks in the driver, when they are not
> > > intended to be called. In the worst case, I guess we could even end up
> > > trying to control resources (like a clock) from the ->runtime
> > > _resume() callback, when the references to these resources may already
> > > have been released.  
> >
> > This is all about what we consider remove. To me, with devm_ manged cleanup
> > in place, both remove() and devm_ cleanup count as parts of that remove
> > process.  
> 
> There is no straightforward process here, if you would keep runtime PM
> enabled beyond ->remove(). Things can happen in parallel.

How?  Nothing magic happens when a driver remove() ends.
So there is no difference at all in a driver calling runtime pm disable
in that code or in devres cleanup that happens immediately after that
in the bus driver remove.

> 
> In that case, drivers would need to extend their runtime PM callbacks
> to cope with more complicated conditions, as resources that those use
> may have been released. Moreover, how can we make sure that the device
> is put into a low power state after the ->remove() has been called?

This is a fair question.  Also one commonly handled by drivers using devm
though perhaps not for the reason you are thinking.  Key is that most
drivers should not rely at all on runtime PM being built let alone
enabled. 

Solution is normally a devm_add_action_or_reset() call that
registers a call to put the device into a low power state.

Sequence is normally something like:

1) enable clks etc.
2) Turn the power on.
3) device setup
4) enable runtime PM after setting the state to active. Let autosuspend
   do it's work.

On remove (all devm_ managed).
5) disable runtime PM.
6) Turn the power off (may involve a check on whether it is already
   off though in many cases it's idempotent so we don't bother checking).
7) disable clocks.

in this particular driver I'm assuming that the low power state is
handled via the reset lines being put back into reset by the
the unwind of the two
devm_reset_control_get_exclusive_deasserted() calls.


> 
> >
> > One option I did wonder about was having a devm_pm_domain_attach()
> > A little cheeky but I think the call in platform_probe() is late enough
> > that we don't run into the checks on no devm_ calls before driver probe.
> >
> > That would shuffle the dev_pm_domain_detach() to the end of the
> > devm_ cleanup.  Mind you the i2c approach above seems better.  
> 
> Again, this isn't limited to PM domains.

All comes down to when you think driver remove is finished.
I think the answer to that in the above i2c scheme is after the
release of the devres group, not after end of remove call.

Anything that the bus driver does will then occur after the
device driver devres cleanup is done.  That puts all cleanup
in device driver remove() immediately before the devres cleanup
of anything setup in probe(), making manual unwinding and automatic
unwinding effective the same.

So for me the right fix here is to make the same thing happen
for platform devices as for i2c ones.

Jonathan


> 
> [...]
> 
> Kind regards
> Uffe

Dmitry Torokhov Jan. 28, 2025, 5:40 a.m. UTC | #16

On Mon, Jan 27, 2025 at 06:24:23PM +0000, Jonathan Cameron wrote:
> On Mon, 27 Jan 2025 16:02:32 +0100
> Ulf Hansson <[e-mail]> wrote:
> 
> > On Mon, 27 Jan 2025 at 13:32, Jonathan Cameron
> > <[e-mail]> wrote:
> > >
> > > On Mon, 27 Jan 2025 11:47:44 +0100
> > > Ulf Hansson <[e-mail]> wrote:
> > >  
> > > > [...]
> > > >  
> > > > > > > > > Do consider OK to change the order in pm_runtime_disable_action() to get
> > > > > > > > > rid of these issues, e.g.:
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > > > > index 2ee45841486b..f27d311d2619 100644
> > > > > > > > > --- a/drivers/base/power/runtime.c
> > > > > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > > > > @@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> > > > > > > > >
> > > > > > > > >  static void pm_runtime_disable_action(void *data)
> > > > > > > > >  {
> > > > > > > > > -       pm_runtime_dont_use_autosuspend(data);
> > > > > > > > >         pm_runtime_disable(data);
> > > > > > > > > +       pm_runtime_dont_use_autosuspend(data);
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > though I see a rpm_resume() call is still possible though pm_runtime_disable().  
> > > > > > > >
> > > > > > > > I am still worried about keeping the device runtime enabled during a
> > > > > > > > window when we have turned off all resources for the device. Typically
> > > > > > > > we want to leave the device in a low power state after unbind.
> > > > > > > >
> > > > > > > > That said, I would rather just drop the devm_pm_runtime_enable() API
> > > > > > > > altogether and convert all users of it into
> > > > > > > > pm_runtime_enable|disable(), similar to what your patch does.  
> > > > > > >
> > > > > > > That is making a mess of a lot of automated cleanup for a strange
> > > > > > > runtime pm related path.  This is pain a driver should not have
> > > > > > > to deal with, though I'm not clear what the right solution is!
> > > > > > >
> > > > > > > Key is that drivers should not mix devm managed cleanup and not, so
> > > > > > > that means that anything that happens after runtime pm is enabled
> > > > > > > has to be torn down manually.  One solution to this might be to
> > > > > > > always enable it late assuming that is safe to do so there is
> > > > > > > never anything else done after it in the probe path of a driver.  
> > > > > >
> > > > > > The problem is that runtime PM isn't really comparable to other
> > > > > > resources that we are managing through devm* functions.
> > > > > >
> > > > > > Enabling runtime PM for a device changes the behaviour for how
> > > > > > power-mgmt is handled for the device. Enabling/disabling of runtime PM
> > > > > > really needs to be explicitly controlled by the driver for the device.  
> > > > >
> > > > > I'm sorry to say I'm not yet convinced.  
> > > >
> > > > Okay, let me try one more time. :-)  
> > >
> > > +CC Greg as the disagreement here is really a philosophy of what
> > > devm cleanup is relative to remove.  Perhaps Greg or Rafael can
> > > given some guidance on the intent there.
> > >
> > > Mind you I think I found another subsystem working around this
> > > and in a somewhat more elegant, general way (to my eyes anyway!)
> > >
> > > https://elixir.bootlin.com/linux/v6.12.6/source/drivers/i2c/i2c-core-base.c#L630
> > > https://lore.kernel.org/all/[e-mail]/
> > >
> > > +CC Dmitry.
> > >
> > > I2C creates an extra devres group and releases it before devm_pm_domain_detach()
> > > As all devm calls from the driver end up in that group, they are released
> > > before dev_pm_domain_detach()

There is also a similar fix in HID core.

> > >  
> > 
> > How would that address the problem I pointed out with runtime PM
> > below? This problem isn't limited to attaching/detaching PM domains.
> 
> It's associated with anything that happens after a driver remove is done.
> We just disagree on when that remove is finished. There is nothing special about
> the remove() callback, that is just part of remove process.
> No magic transition of state that allows new things to happen follows
> the device driver remove finishing. Sure you can get the remove
> handling ordering wrong whether devm is in use or not.  The trick is
> almost always to never mix devm and not.  Once you need a single bit of
> manual unwinding stop with the devm and do everything beyond that point
> by hand (in probe order, before that point in remove order)

Right, this is a classic problem of mixing devm-managed resources and
ordinary ones. Every time we have a bus remove() method that is not
trivial we need to make sure the resources are released in the right
order, which is:

1. Driver-allocated resources
2. Bus-allocated resources
3. Driver-core allocated resources.

Establishing a devres group before calling into drivers' probe() methods
(and releasing it before doing the rest of the cleanup in remove()) is
one such way.

Thanks.

Geert Uytterhoeven Jan. 28, 2025, 7:59 a.m. UTC | #17

Hi Jonathan,

On Mon, 27 Jan 2025 at 19:24, Jonathan Cameron
<[e-mail]> wrote:
> On Mon, 27 Jan 2025 16:02:32 +0100
> Ulf Hansson <[e-mail]> wrote:
> > On Mon, 27 Jan 2025 at 13:32, Jonathan Cameron
> > <[e-mail]> wrote:
> > > On Mon, 27 Jan 2025 11:47:44 +0100
> > > Ulf Hansson <[e-mail]> wrote:
> > > > > > > > > Do consider OK to change the order in pm_runtime_disable_action() to get
> > > > > > > > > rid of these issues, e.g.:
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > > > > index 2ee45841486b..f27d311d2619 100644
> > > > > > > > > --- a/drivers/base/power/runtime.c
> > > > > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > > > > @@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> > > > > > > > >
> > > > > > > > >  static void pm_runtime_disable_action(void *data)
> > > > > > > > >  {
> > > > > > > > > -       pm_runtime_dont_use_autosuspend(data);
> > > > > > > > >         pm_runtime_disable(data);
> > > > > > > > > +       pm_runtime_dont_use_autosuspend(data);
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > though I see a rpm_resume() call is still possible though pm_runtime_disable().
> > > > > > > >
> > > > > > > > I am still worried about keeping the device runtime enabled during a
> > > > > > > > window when we have turned off all resources for the device. Typically
> > > > > > > > we want to leave the device in a low power state after unbind.
> > > > > > > >
> > > > > > > > That said, I would rather just drop the devm_pm_runtime_enable() API
> > > > > > > > altogether and convert all users of it into
> > > > > > > > pm_runtime_enable|disable(), similar to what your patch does.
> > > > > > >
> > > > > > > That is making a mess of a lot of automated cleanup for a strange
> > > > > > > runtime pm related path.  This is pain a driver should not have
> > > > > > > to deal with, though I'm not clear what the right solution is!
> > > > > > >
> > > > > > > Key is that drivers should not mix devm managed cleanup and not, so
> > > > > > > that means that anything that happens after runtime pm is enabled
> > > > > > > has to be torn down manually.  One solution to this might be to
> > > > > > > always enable it late assuming that is safe to do so there is
> > > > > > > never anything else done after it in the probe path of a driver.
> > > > > >
> > > > > > The problem is that runtime PM isn't really comparable to other
> > > > > > resources that we are managing through devm* functions.
> > > > > >
> > > > > > Enabling runtime PM for a device changes the behaviour for how
> > > > > > power-mgmt is handled for the device. Enabling/disabling of runtime PM
> > > > > > really needs to be explicitly controlled by the driver for the device.
> > > > >
> > > > > I'm sorry to say I'm not yet convinced.
> > > >
> > > > Okay, let me try one more time. :-)
> > >
> > > +CC Greg as the disagreement here is really a philosophy of what
> > > devm cleanup is relative to remove.  Perhaps Greg or Rafael can
> > > given some guidance on the intent there.
> > >
> > > Mind you I think I found another subsystem working around this
> > > and in a somewhat more elegant, general way (to my eyes anyway!)
> > >
> > > https://elixir.bootlin.com/linux/v6.12.6/source/drivers/i2c/i2c-core-base.c#L630
> > > https://lore.kernel.org/all/[e-mail]/
> > >
> > > +CC Dmitry.
> > >
> > > I2C creates an extra devres group and releases it before devm_pm_domain_detach()
> > > As all devm calls from the driver end up in that group, they are released
> > > before dev_pm_domain_detach()
> >
> > How would that address the problem I pointed out with runtime PM
> > below? This problem isn't limited to attaching/detaching PM domains.
>
> It's associated with anything that happens after a driver remove is done.
> We just disagree on when that remove is finished. There is nothing special about
> the remove() callback, that is just part of remove process.
> No magic transition of state that allows new things to happen follows
> the device driver remove finishing. Sure you can get the remove
> handling ordering wrong whether devm is in use or not.  The trick is
> almost always to never mix devm and not.  Once you need a single bit of
> manual unwinding stop with the devm and do everything beyond that point
> by hand (in probe order, before that point in remove order)
>
> > > > > Devm callbacks are explicitly registered by the driver so that they
> > > > > are unwound in a specific order.  Many other parts of driver
> > > > > registration rely on this ordering.  This does not seem different
> > > > > for runtime PM than anything else.
> > > >
> > > > If you compare clocks, for example. It's the driver that is in full
> > > > control of the clock gating/ungating. When the ->remove() callback
> > > > runs, the driver typically makes sure that it leaves the clock gated.
> > > > Then it doesn't really matter when the clock resource gets released.
> > > > The point is, the driver is in full control of the resource.
> > >
> > > Not a good example. devm_clk_get_enabled() does not gate the clock until
> >
> > I was not referring to devm_clk_get_enable(), but rather just devm_clk_get().
> >
> > To me devm_clk_get_enable() is another interface that we should avoid.
> > For example, what if the clock is already gated when the ->remove()
> > callback runs? Then we need to ungate the clock just to make the
> > devres path happy so it doesn't gate an already gated clock. And this,
> > just to save one or two lines of code.
>
> If someone is using a clock that is gated by other calls in the driver
> that then indeed the use of devm_clk_get_enabled() is inappropriate
> or they turn they do need to enable the clock and turn it off again
> as you mention which is a mess but often needs doing anyway as we
> commonly need some clocks at least to put a device into a low power
> state.
>
> >
> > Don't get me wrong, I certainly like the devm* functions in general,
> > but it's not a good fit for everything.
>
> For anything that a driver otherwise calls in remove() they are
> a direct equivalent that is just called automatically. (though apparently
> not quite in platform drivers!) If you have a sequence that is
> sufficiently complex they may not be a good fit. I just don't think
> that the sequence in this driver (and many others) is complex.
> The driver code (with the above change from i2c ported to platform code
> to fix the specific problem) is a lot simpler before Claudia's v2 to
> change the handling. 32 lines added to work around this...
>
> >
> > > the devm cleanup. The assumption being that nothing that affects
> > > it runs between the remove() and devm cleanup.  So pretty much identical
> > > to the runtime pm case.  They being that you have to obey ordering so
> > > that if you need to run something after the clock is disabled then
> > > you register that callback before you call devm_clk_get_enabled()
> > >
> > > > If runtime PM would remain enabled beyond the call to the ->remove()
> > > > callback, it would mean that the driver's runtime PM callbacks could
> > > > be called too. For example, userspace via sysfs may at any point
> > > > decide to runtime resume the device. In other words, we may end up
> > > > calling the runtime PM callbacks in the driver, when they are not
> > > > intended to be called. In the worst case, I guess we could even end up
> > > > trying to control resources (like a clock) from the ->runtime
> > > > _resume() callback, when the references to these resources may already
> > > > have been released.
> > >
> > > This is all about what we consider remove. To me, with devm_ manged cleanup
> > > in place, both remove() and devm_ cleanup count as parts of that remove
> > > process.
> >
> > There is no straightforward process here, if you would keep runtime PM
> > enabled beyond ->remove(). Things can happen in parallel.
>
> How?  Nothing magic happens when a driver remove() ends.
> So there is no difference at all in a driver calling runtime pm disable
> in that code or in devres cleanup that happens immediately after that
> in the bus driver remove.
>
> > In that case, drivers would need to extend their runtime PM callbacks
> > to cope with more complicated conditions, as resources that those use
> > may have been released. Moreover, how can we make sure that the device
> > is put into a low power state after the ->remove() has been called?
>
> This is a fair question.  Also one commonly handled by drivers using devm
> though perhaps not for the reason you are thinking.  Key is that most
> drivers should not rely at all on runtime PM being built let alone
> enabled.

Drivers for components on SoCs that use PM Domains must use
Runtime PM, as that is the only available method to control the PM Domain.

> Solution is normally a devm_add_action_or_reset() call that
> registers a call to put the device into a low power state.
>
> Sequence is normally something like:
>
> 1) enable clks etc.

Can be done explicitly, or using pm_runtime_resume_and_get() in case
of a clock domain.

> 2) Turn the power on.

I assume you mean through a regulator, GPIO, or (deasserted) reset
signal?
In case of a PM Domain, that is done using pm_runtime_resume_and_get()
again.

> 3) device setup
> 4) enable runtime PM after setting the state to active. Let autosuspend
>    do it's work.
>
> On remove (all devm_ managed).
> 5) disable runtime PM.
> 6) Turn the power off (may involve a check on whether it is already
>    off though in many cases it's idempotent so we don't bother checking).
> 7) disable clocks.
>
> in this particular driver I'm assuming that the low power state is
> handled via the reset lines being put back into reset by the
> the unwind of the two
> devm_reset_control_get_exclusive_deasserted() calls.

No, power control is handled by the PM (Power and Clock!) Domain, which
controls both the power area (R9A08G045_PD_ADC) and the module clocks
(R9A08G045_ADC_ADCLK, R9A08G045_ADC_PCLK).

Gr{oetje,eeting}s,

                        Geert

Jonathan Cameron Jan. 28, 2025, 10:59 a.m. UTC | #18

On Tue, 28 Jan 2025 08:59:33 +0100
Geert Uytterhoeven <[e-mail]> wrote:

> Hi Jonathan,
> 
> On Mon, 27 Jan 2025 at 19:24, Jonathan Cameron
> <[e-mail]> wrote:
> > On Mon, 27 Jan 2025 16:02:32 +0100
> > Ulf Hansson <[e-mail]> wrote:  
> > > On Mon, 27 Jan 2025 at 13:32, Jonathan Cameron
> > > <[e-mail]> wrote:  
> > > > On Mon, 27 Jan 2025 11:47:44 +0100
> > > > Ulf Hansson <[e-mail]> wrote:  
> > > > > > > > > > Do consider OK to change the order in pm_runtime_disable_action() to get
> > > > > > > > > > rid of these issues, e.g.:
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > > > > > > > > > index 2ee45841486b..f27d311d2619 100644
> > > > > > > > > > --- a/drivers/base/power/runtime.c
> > > > > > > > > > +++ b/drivers/base/power/runtime.c
> > > > > > > > > > @@ -1547,8 +1547,8 @@ EXPORT_SYMBOL_GPL(pm_runtime_enable);
> > > > > > > > > >
> > > > > > > > > >  static void pm_runtime_disable_action(void *data)
> > > > > > > > > >  {
> > > > > > > > > > -       pm_runtime_dont_use_autosuspend(data);
> > > > > > > > > >         pm_runtime_disable(data);
> > > > > > > > > > +       pm_runtime_dont_use_autosuspend(data);
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > though I see a rpm_resume() call is still possible though pm_runtime_disable().  
> > > > > > > > >
> > > > > > > > > I am still worried about keeping the device runtime enabled during a
> > > > > > > > > window when we have turned off all resources for the device. Typically
> > > > > > > > > we want to leave the device in a low power state after unbind.
> > > > > > > > >
> > > > > > > > > That said, I would rather just drop the devm_pm_runtime_enable() API
> > > > > > > > > altogether and convert all users of it into
> > > > > > > > > pm_runtime_enable|disable(), similar to what your patch does.  
> > > > > > > >
> > > > > > > > That is making a mess of a lot of automated cleanup for a strange
> > > > > > > > runtime pm related path.  This is pain a driver should not have
> > > > > > > > to deal with, though I'm not clear what the right solution is!
> > > > > > > >
> > > > > > > > Key is that drivers should not mix devm managed cleanup and not, so
> > > > > > > > that means that anything that happens after runtime pm is enabled
> > > > > > > > has to be torn down manually.  One solution to this might be to
> > > > > > > > always enable it late assuming that is safe to do so there is
> > > > > > > > never anything else done after it in the probe path of a driver.  
> > > > > > >
> > > > > > > The problem is that runtime PM isn't really comparable to other
> > > > > > > resources that we are managing through devm* functions.
> > > > > > >
> > > > > > > Enabling runtime PM for a device changes the behaviour for how
> > > > > > > power-mgmt is handled for the device. Enabling/disabling of runtime PM
> > > > > > > really needs to be explicitly controlled by the driver for the device.  
> > > > > >
> > > > > > I'm sorry to say I'm not yet convinced.  
> > > > >
> > > > > Okay, let me try one more time. :-)  
> > > >
> > > > +CC Greg as the disagreement here is really a philosophy of what
> > > > devm cleanup is relative to remove.  Perhaps Greg or Rafael can
> > > > given some guidance on the intent there.
> > > >
> > > > Mind you I think I found another subsystem working around this
> > > > and in a somewhat more elegant, general way (to my eyes anyway!)
> > > >
> > > > https://elixir.bootlin.com/linux/v6.12.6/source/drivers/i2c/i2c-core-base.c#L630
> > > > https://lore.kernel.org/all/[e-mail]/
> > > >
> > > > +CC Dmitry.
> > > >
> > > > I2C creates an extra devres group and releases it before devm_pm_domain_detach()
> > > > As all devm calls from the driver end up in that group, they are released
> > > > before dev_pm_domain_detach()  
> > >
> > > How would that address the problem I pointed out with runtime PM
> > > below? This problem isn't limited to attaching/detaching PM domains.  
> >
> > It's associated with anything that happens after a driver remove is done.
> > We just disagree on when that remove is finished. There is nothing special about
> > the remove() callback, that is just part of remove process.
> > No magic transition of state that allows new things to happen follows
> > the device driver remove finishing. Sure you can get the remove
> > handling ordering wrong whether devm is in use or not.  The trick is
> > almost always to never mix devm and not.  Once you need a single bit of
> > manual unwinding stop with the devm and do everything beyond that point
> > by hand (in probe order, before that point in remove order)
> >  
> > > > > > Devm callbacks are explicitly registered by the driver so that they
> > > > > > are unwound in a specific order.  Many other parts of driver
> > > > > > registration rely on this ordering.  This does not seem different
> > > > > > for runtime PM than anything else.  
> > > > >
> > > > > If you compare clocks, for example. It's the driver that is in full
> > > > > control of the clock gating/ungating. When the ->remove() callback
> > > > > runs, the driver typically makes sure that it leaves the clock gated.
> > > > > Then it doesn't really matter when the clock resource gets released.
> > > > > The point is, the driver is in full control of the resource.  
> > > >
> > > > Not a good example. devm_clk_get_enabled() does not gate the clock until  
> > >
> > > I was not referring to devm_clk_get_enable(), but rather just devm_clk_get().
> > >
> > > To me devm_clk_get_enable() is another interface that we should avoid.
> > > For example, what if the clock is already gated when the ->remove()
> > > callback runs? Then we need to ungate the clock just to make the
> > > devres path happy so it doesn't gate an already gated clock. And this,
> > > just to save one or two lines of code.  
> >
> > If someone is using a clock that is gated by other calls in the driver
> > that then indeed the use of devm_clk_get_enabled() is inappropriate
> > or they turn they do need to enable the clock and turn it off again
> > as you mention which is a mess but often needs doing anyway as we
> > commonly need some clocks at least to put a device into a low power
> > state.
> >  
> > >
> > > Don't get me wrong, I certainly like the devm* functions in general,
> > > but it's not a good fit for everything.  
> >
> > For anything that a driver otherwise calls in remove() they are
> > a direct equivalent that is just called automatically. (though apparently
> > not quite in platform drivers!) If you have a sequence that is
> > sufficiently complex they may not be a good fit. I just don't think
> > that the sequence in this driver (and many others) is complex.
> > The driver code (with the above change from i2c ported to platform code
> > to fix the specific problem) is a lot simpler before Claudia's v2 to
> > change the handling. 32 lines added to work around this...
> >  
> > >  
> > > > the devm cleanup. The assumption being that nothing that affects
> > > > it runs between the remove() and devm cleanup.  So pretty much identical
> > > > to the runtime pm case.  They being that you have to obey ordering so
> > > > that if you need to run something after the clock is disabled then
> > > > you register that callback before you call devm_clk_get_enabled()
> > > >  
> > > > > If runtime PM would remain enabled beyond the call to the ->remove()
> > > > > callback, it would mean that the driver's runtime PM callbacks could
> > > > > be called too. For example, userspace via sysfs may at any point
> > > > > decide to runtime resume the device. In other words, we may end up
> > > > > calling the runtime PM callbacks in the driver, when they are not
> > > > > intended to be called. In the worst case, I guess we could even end up
> > > > > trying to control resources (like a clock) from the ->runtime
> > > > > _resume() callback, when the references to these resources may already
> > > > > have been released.  
> > > >
> > > > This is all about what we consider remove. To me, with devm_ manged cleanup
> > > > in place, both remove() and devm_ cleanup count as parts of that remove
> > > > process.  
> > >
> > > There is no straightforward process here, if you would keep runtime PM
> > > enabled beyond ->remove(). Things can happen in parallel.  
> >
> > How?  Nothing magic happens when a driver remove() ends.
> > So there is no difference at all in a driver calling runtime pm disable
> > in that code or in devres cleanup that happens immediately after that
> > in the bus driver remove.
> >  
> > > In that case, drivers would need to extend their runtime PM callbacks
> > > to cope with more complicated conditions, as resources that those use
> > > may have been released. Moreover, how can we make sure that the device
> > > is put into a low power state after the ->remove() has been called?  
> >
> > This is a fair question.  Also one commonly handled by drivers using devm
> > though perhaps not for the reason you are thinking.  Key is that most
> > drivers should not rely at all on runtime PM being built let alone
> > enabled.  
> 
> Drivers for components on SoCs that use PM Domains must use
> Runtime PM, as that is the only available method to control the PM Domain.

Ok. So you explicitly disable the interfaces to turn runtime pm off. Fair
enough.  Though in the driver I would want to see explicit statement of
that. I don't want to go dig in the parent to find out.  My assumption
will always be that runtime PM might be disabled unless I see
a dependency or comment telling me otherwise.

> 
> > Solution is normally a devm_add_action_or_reset() call that
> > registers a call to put the device into a low power state.
> >
> > Sequence is normally something like:
> >
> > 1) enable clks etc.  
> 
> Can be done explicitly, or using pm_runtime_resume_and_get() in case
> of a clock domain.
> 
> > 2) Turn the power on.  
> 
> I assume you mean through a regulator, GPIO, or (deasserted) reset
> signal?
> In case of a PM Domain, that is done using pm_runtime_resume_and_get()
> again.

Often a register write or similar to bring a device out of a low
power state.  That register usually sits in the space of the ADC etc.
Can be done via runtime PM callbacks in the ADC driver.  Often is, though
common sequence is to enable directly in probe() then enable runtime
pm to turn it off again.  Here where you have clocks etc in the pm domain
I guess an early runtime pm resume does most of that, but I'd assume
the reset deasserts in the driver are also part of that.
The reset is not done in the PM domain code here. It is being
done in this driver.

> 
> > 3) device setup
> > 4) enable runtime PM after setting the state to active. Let autosuspend
> >    do it's work.
> >
> > On remove (all devm_ managed).
> > 5) disable runtime PM.
> > 6) Turn the power off (may involve a check on whether it is already
> >    off though in many cases it's idempotent so we don't bother checking).
> > 7) disable clocks.
> >
> > in this particular driver I'm assuming that the low power state is
> > handled via the reset lines being put back into reset by the
> > the unwind of the two
> > devm_reset_control_get_exclusive_deasserted() calls.  
> 
> No, power control is handled by the PM (Power and Clock!) Domain, which
> controls both the power area (R9A08G045_PD_ADC) and the module clocks
> (R9A08G045_ADC_ADCLK, R9A08G045_ADC_PCLK).

Ok, though putting the device into reset is also a common way to put
something into power saving, just not here.

Aim of this illustration wasn't so much about this specific example or
indeed anything using runtime pm and pm domains.  It was more to make
the point that the flow is a general problem and easily handled in
a driver.  We can do 'anything' in devm calls.  The balance of
what makes sense is not driven by the complexity of a specific call it is
driven by the rule of thumb that everything after the first non devm
cleanup thing (that needs code in remove) must not use devm.

Sometimes it is safe, but it makes reasoning about ordering much
harder so I will reject any driver doing it from a maintainability
point of view.  I have simply seen too many subtle bugs as a result
of mixing and matching devres cleanup and driver remove calls.

I reiterate that (to me) there is nothing special about this aspect of
how we should handle pm domains.  They should not be complicating
the driver code I see. If anything they should be making it simpler!
In the proposed changes in this series they make the driver more
complex.

As per the earlier reply and Dmitry's follow up seems there is
a common solution to this problem used by other buses.
If there are problems with that then absolutely fine to consider
something else, but so far I've not understood there to be one.

Basically I'm asking for someone to point me to specific real
driver code that solution breaks. I think that can only happen
if devm_ cleanup in a driver is relying on the pm domain being
down.   Superficially I'd argue any case that does that is
dubious and probably wants improving, but obviously need real
examples to consider.  Maybe there is something subtle.

Jonathan




> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>

Patch

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index 883c167c0670..f12f3daf08cc 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -464,25 +464,26 @@  static int rzg2l_adc_probe(struct platform_device *pdev)
 
 	pm_runtime_set_autosuspend_delay(dev, 300);
 	pm_runtime_use_autosuspend(dev);
-	ret = devm_pm_runtime_enable(dev);
-	if (ret)
-		return ret;
+	pm_runtime_enable(dev);
 
 	platform_set_drvdata(pdev, indio_dev);
 
 	ret = rzg2l_adc_hw_init(dev, adc);
-	if (ret)
-		return dev_err_probe(&pdev->dev, ret,
-				     "failed to initialize ADC HW\n");
+	if (ret) {
+		dev_err_probe(&pdev->dev, ret, "failed to initialize ADC HW\n");
+		goto rpm_disable;
+	}
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq < 0)
-		return irq;
+	if (irq < 0) {
+		ret = irq;
+		goto rpm_disable;
+	}
 
 	ret = devm_request_irq(dev, irq, rzg2l_adc_isr,
 			       0, dev_name(dev), adc);
 	if (ret < 0)
-		return ret;
+		goto rpm_disable;
 
 	init_completion(&adc->completion);
 
@@ -493,6 +494,19 @@  static int rzg2l_adc_probe(struct platform_device *pdev)
 	indio_dev->num_channels = adc->data->num_channels;
 
 	return devm_iio_device_register(dev, indio_dev);
+
+rpm_disable:
+	pm_runtime_disable(dev);
+	pm_runtime_dont_use_autosuspend(dev);
+	return ret;
+}
+
+static void rzg2l_adc_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	pm_runtime_disable(dev);
+	pm_runtime_dont_use_autosuspend(dev);
 }
 
 static const struct rzg2l_adc_hw_params rzg2l_hw_params = {
@@ -614,6 +628,7 @@  static const struct dev_pm_ops rzg2l_adc_pm_ops = {
 
 static struct platform_driver rzg2l_adc_driver = {
 	.probe		= rzg2l_adc_probe,
+	.remove		= rzg2l_adc_remove,
 	.driver		= {
 		.name		= DRIVER_NAME,
 		.of_match_table = rzg2l_adc_match,