Re: [PATCH v3 1/3] iio: imu: inv_mpu6050: fix unbalanced regulator_disable calls, when probe fails

Re: [PATCH v3 1/3] iio: imu: inv_mpu6050: fix unbalanced regulator_disable calls, when probe fails

[Andrey Skvortsov]

On 26-04-26 15:43, Jonathan Cameron wrote:
> On Sun, 26 Apr 2026 14:23:32 +0300
> Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> 
> > During a probe functions after all regulators are enabled, runtime pm
> > is enabled. Before probe function finishes, runtime pm triggers and
> > disables vddio regulator. When probe function fails after that,
> > inv_mpu_core_disable_regulator_action tries to disable already
> > disabled by runtime pm vddio regulator causing following backtrace:
> > 
> >   inv-mpu6050-i2c 1-0068: trigger probe fail -19
> >   WARNING: drivers/regulator/core.c:3244 at _regulator_disable+0x2ac/0x600
> >   Call trace:
> >    _regulator_disable+0x2ac/0x600 (P)
> >    regulator_disable+0xac/0x148
> >    inv_mpu_core_disable_regulator_vddio_action+0x3c/0xb0 [inv_mpu6050]
> >    devm_action_release+0x4c/0x88
> >    release_nodes+0xd8/0x178
> >    devres_release_group+0x214/0x3c8
> >    i2c_device_probe+0x6fc/0x9b0
> >   ...
> >   inv-mpu6050-i2c 1-0068: Failed to disable vddio regulator: -5
> > 
> > vddio state is handled in two places: pm_runtime and
> > inv_mpu_core_disable_regulator_vddio_action devm action.
> > inv_mpu_core_disable_regulator_vddio_action has to check, whether
> > regulator is disabled by pm_runtime already. But this information is
> > available only after pm_runtime state is initialized by
> > pm_runtime_set_active during a probe. So
> > inv_mpu_core_disable_regulator_vddio_action has to be called only
> > after pm_runtime is properly initialized. To handle cases, when probe
> > fails before pm_runtime is enabled, explicitly disable regulators
> > in error paths.
> Why not drag pm_runtime_set_active() earlier?

Do you mean something like proposed in v2? [1]

1. https://lore.kernel.org/linux-iio/20260401082737.781018-4-andrej.skvortzov@gmail.com/#t

> Whilst we currently
> check for an error on that call I'm not sure there is any reason
> to do so.  It's a narrow set of conditions than can cause such an
> error. 
> 
> If you have that between the regulator enable and registering the
> devm action I think that would mean you don't need to manually clean
> up the regulators.
> 
> Maybe I'm missing something.
> 
> J
> 
> > 
> > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> > Fixes: 07c12b1c007c ("iio: imu: mpu6050: add support for regulator framework")
> > ---
> > 
> > Changes in v3:
> >  - make patch to be the first patch of the patchset for easier backporting
> >  - update commit message to fix Andy's comment
> >  - don't move pm_runtime_set_active from pm_runtime initialization,
> >    move activation of inv_mpu_core_disable_regulator_action instead
> > 
> > Changes in v2:
> >  - minimize call trace in commit message
> >  - include specific driver name in patch subject
> > 

-- 
Best regards,
Andrey Skvortsov

Re: [PATCH v3 1/3] iio: imu: inv_mpu6050: fix unbalanced regulator_disable calls, when probe fails

[Jonathan Cameron]

On Sun, 26 Apr 2026 23:25:42 +0300
Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:

> On 26-04-26 15:43, Jonathan Cameron wrote:
> > On Sun, 26 Apr 2026 14:23:32 +0300
> > Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> >   
> > > During a probe functions after all regulators are enabled, runtime pm
> > > is enabled. Before probe function finishes, runtime pm triggers and
> > > disables vddio regulator. When probe function fails after that,
> > > inv_mpu_core_disable_regulator_action tries to disable already
> > > disabled by runtime pm vddio regulator causing following backtrace:
> > > 
> > >   inv-mpu6050-i2c 1-0068: trigger probe fail -19
> > >   WARNING: drivers/regulator/core.c:3244 at _regulator_disable+0x2ac/0x600
> > >   Call trace:
> > >    _regulator_disable+0x2ac/0x600 (P)
> > >    regulator_disable+0xac/0x148
> > >    inv_mpu_core_disable_regulator_vddio_action+0x3c/0xb0 [inv_mpu6050]
> > >    devm_action_release+0x4c/0x88
> > >    release_nodes+0xd8/0x178
> > >    devres_release_group+0x214/0x3c8
> > >    i2c_device_probe+0x6fc/0x9b0
> > >   ...
> > >   inv-mpu6050-i2c 1-0068: Failed to disable vddio regulator: -5
> > > 
> > > vddio state is handled in two places: pm_runtime and
> > > inv_mpu_core_disable_regulator_vddio_action devm action.
> > > inv_mpu_core_disable_regulator_vddio_action has to check, whether
> > > regulator is disabled by pm_runtime already. But this information is
> > > available only after pm_runtime state is initialized by
> > > pm_runtime_set_active during a probe. So
> > > inv_mpu_core_disable_regulator_vddio_action has to be called only
> > > after pm_runtime is properly initialized. To handle cases, when probe
> > > fails before pm_runtime is enabled, explicitly disable regulators
> > > in error paths.  
> > Why not drag pm_runtime_set_active() earlier?  
> 
> Do you mean something like proposed in v2? [1]
> 
> 1. https://lore.kernel.org/linux-iio/20260401082737.781018-4-andrej.skvortzov@gmail.com/#t
Ah yes. Sorry, I missed that discussion.

To me moving this earlier is less hacky.  We are brining the runtime
state inline with what we have set the power to at the point of doing
so.

Nuno, having seen the complexity that moving that state change late
causes, I don't suppose you are convinced that v2 solution is the way
to go?

Thanks,

Jonathan

> 
> > Whilst we currently
> > check for an error on that call I'm not sure there is any reason
> > to do so.  It's a narrow set of conditions than can cause such an
> > error. 
> > 
> > If you have that between the regulator enable and registering the
> > devm action I think that would mean you don't need to manually clean
> > up the regulators.
> > 
> > Maybe I'm missing something.
> > 
> > J
> >   
> > > 
> > > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@gmail.com>
> > > Fixes: 07c12b1c007c ("iio: imu: mpu6050: add support for regulator framework")
> > > ---
> > > 
> > > Changes in v3:
> > >  - make patch to be the first patch of the patchset for easier backporting
> > >  - update commit message to fix Andy's comment
> > >  - don't move pm_runtime_set_active from pm_runtime initialization,
> > >    move activation of inv_mpu_core_disable_regulator_action instead
> > > 
> > > Changes in v2:
> > >  - minimize call trace in commit message
> > >  - include specific driver name in patch subject
> > >   
> 

Re: [PATCH v3 1/3] iio: imu: inv_mpu6050: fix unbalanced regulator_disable calls, when probe fails

[Andy Shevchenko]

On Sun, Apr 26, 2026 at 02:23:32PM +0300, Andrey Skvortsov wrote:
> During a probe functions after all regulators are enabled, runtime pm
> is enabled. Before probe function finishes, runtime pm triggers and
> disables vddio regulator. When probe function fails after that,
> inv_mpu_core_disable_regulator_action tries to disable already

Please, use () when we refer to the functions or callbacks:
 — func()
 — .callback()

> disabled by runtime pm vddio regulator causing following backtrace:

PM

>   inv-mpu6050-i2c 1-0068: trigger probe fail -19
>   WARNING: drivers/regulator/core.c:3244 at _regulator_disable+0x2ac/0x600
>   Call trace:
>    _regulator_disable+0x2ac/0x600 (P)
>    regulator_disable+0xac/0x148
>    inv_mpu_core_disable_regulator_vddio_action+0x3c/0xb0 [inv_mpu6050]
>    devm_action_release+0x4c/0x88
>    release_nodes+0xd8/0x178
>    devres_release_group+0x214/0x3c8
>    i2c_device_probe+0x6fc/0x9b0
>   ...
>   inv-mpu6050-i2c 1-0068: Failed to disable vddio regulator: -5
> 
> vddio state is handled in two places: pm_runtime and
> inv_mpu_core_disable_regulator_vddio_action devm action.
> inv_mpu_core_disable_regulator_vddio_action has to check, whether
> regulator is disabled by pm_runtime already. But this information is
> available only after pm_runtime state is initialized by
> pm_runtime_set_active during a probe. So
> inv_mpu_core_disable_regulator_vddio_action has to be called only
> after pm_runtime is properly initialized. To handle cases, when probe
> fails before pm_runtime is enabled, explicitly disable regulators
> in error paths.

As per previous comments.

...

> +	result = devm_add_action_or_reset(dev, inv_mpu_core_disable_regulator_action,
> +					  st);

I would go with a single line despite being long.


> +	if (result)
> +		return dev_err_probe(dev, result,
> +				     "Failed to setup regulator cleanup action\n");

-- 
With Best Regards,
Andy Shevchenko