Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
calls during probe. These are not required when the device is marked
active via pm_runtime_set_active() before enabling pm_runtime with
pm_runtime_enable().
Also remove the redundant pm_runtime_put_sync() call from the cleanup
path, since the core is not incrementing the usage count beforehand.
This simplifies the PM setup and avoids manipulating the usage counter
unnecessarily.
Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
index 55a29b1e2b11355598b0ede7af22857aed3ae134..1072bea11c73d09a9a0e6ea9d4a5c7a72248dca7 100644
--- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
+++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
@@ -708,7 +708,6 @@ static void inv_icm42600_disable_pm(void *_data)
{
struct device *dev = _data;
- pm_runtime_put_sync(dev);
pm_runtime_disable(dev);
}
@@ -806,11 +805,10 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,
ret = pm_runtime_set_active(dev);
if (ret)
return ret;
- pm_runtime_get_noresume(dev);
+
pm_runtime_enable(dev);
pm_runtime_set_autosuspend_delay(dev, INV_ICM42600_SUSPEND_DELAY_MS);
pm_runtime_use_autosuspend(dev);
- pm_runtime_put(dev);
return devm_add_action_or_reset(dev, inv_icm42600_disable_pm, dev);
}
--
2.50.0On Wed, 09 Jul 2025 14:35:12 +0200
Sean Nyekjaer <sean@geanix.com> wrote:
> Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> calls during probe. These are not required when the device is marked
> active via pm_runtime_set_active() before enabling pm_runtime with
> pm_runtime_enable().
>
> Also remove the redundant pm_runtime_put_sync() call from the cleanup
> path, since the core is not incrementing the usage count beforehand.
>
> This simplifies the PM setup and avoids manipulating the usage counter
> unnecessarily.
Could we switch directly to using devm_pm_runtime_enable() for this driver?
At first glance looks like this code is missing the disable of autosuspend
that should be there (which devm_pm_runtime_enable() will also handle).
>
> Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 55a29b1e2b11355598b0ede7af22857aed3ae134..1072bea11c73d09a9a0e6ea9d4a5c7a72248dca7 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -708,7 +708,6 @@ static void inv_icm42600_disable_pm(void *_data)
> {
> struct device *dev = _data;
>
> - pm_runtime_put_sync(dev);
> pm_runtime_disable(dev);
> }
>
> @@ -806,11 +805,10 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> ret = pm_runtime_set_active(dev);
> if (ret)
> return ret;
> - pm_runtime_get_noresume(dev);
> +
> pm_runtime_enable(dev);
> pm_runtime_set_autosuspend_delay(dev, INV_ICM42600_SUSPEND_DELAY_MS);
> pm_runtime_use_autosuspend(dev);
> - pm_runtime_put(dev);
>
> return devm_add_action_or_reset(dev, inv_icm42600_disable_pm, dev);
> }
>
On Sun, Jul 13, 2025 at 03:28:10PM +0100, Jonathan Cameron wrote:
> On Wed, 09 Jul 2025 14:35:12 +0200
> Sean Nyekjaer <sean@geanix.com> wrote:
>
> > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> > calls during probe. These are not required when the device is marked
> > active via pm_runtime_set_active() before enabling pm_runtime with
> > pm_runtime_enable().
> >
> > Also remove the redundant pm_runtime_put_sync() call from the cleanup
> > path, since the core is not incrementing the usage count beforehand.
> >
> > This simplifies the PM setup and avoids manipulating the usage counter
> > unnecessarily.
>
> Could we switch directly to using devm_pm_runtime_enable() for this driver?
>
> At first glance looks like this code is missing the disable of autosuspend
> that should be there (which devm_pm_runtime_enable() will also handle).
>
I have tried to use devm_pm_runtime_enable() but on rmmod it warns
"unbalanced disables for regulator"
If I remove this:
- ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vddio_reg, st);
- if (ret)
- return ret;
Everything seems okay again. I have checked with printk's that
inv_icm42600_disable_vddio_reg() is called twice with
devm_pm_runtime_enable() used.
Does it make sense?
/Sean
>
> >
> > Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> > ---
> > drivers/iio/imu/inv_icm42600/inv_icm42600_core.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > index 55a29b1e2b11355598b0ede7af22857aed3ae134..1072bea11c73d09a9a0e6ea9d4a5c7a72248dca7 100644
> > --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> > @@ -708,7 +708,6 @@ static void inv_icm42600_disable_pm(void *_data)
> > {
> > struct device *dev = _data;
> >
> > - pm_runtime_put_sync(dev);
> > pm_runtime_disable(dev);
> > }
> >
> > @@ -806,11 +805,10 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> > ret = pm_runtime_set_active(dev);
> > if (ret)
> > return ret;
> > - pm_runtime_get_noresume(dev);
> > +
> > pm_runtime_enable(dev);
> > pm_runtime_set_autosuspend_delay(dev, INV_ICM42600_SUSPEND_DELAY_MS);
> > pm_runtime_use_autosuspend(dev);
> > - pm_runtime_put(dev);
> >
> > return devm_add_action_or_reset(dev, inv_icm42600_disable_pm, dev);
> > }
> >
>
On Mon, Jul 14, 2025 at 07:24:57AM +0100, Sean Nyekjaer wrote: > On Sun, Jul 13, 2025 at 03:28:10PM +0100, Jonathan Cameron wrote: > > On Wed, 09 Jul 2025 14:35:12 +0200 > > Sean Nyekjaer <sean@geanix.com> wrote: > > > > > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put() > > > calls during probe. These are not required when the device is marked > > > active via pm_runtime_set_active() before enabling pm_runtime with > > > pm_runtime_enable(). > > > > > > Also remove the redundant pm_runtime_put_sync() call from the cleanup > > > path, since the core is not incrementing the usage count beforehand. > > > > > > This simplifies the PM setup and avoids manipulating the usage counter > > > unnecessarily. > > > > Could we switch directly to using devm_pm_runtime_enable() for this driver? > > > > At first glance looks like this code is missing the disable of autosuspend > > that should be there (which devm_pm_runtime_enable() will also handle). > > > > I have tried to use devm_pm_runtime_enable() but on rmmod it warns > "unbalanced disables for regulator" > > If I remove this: > - ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vddio_reg, st); > - if (ret) > - return ret; > > Everything seems okay again. I have checked with printk's that > inv_icm42600_disable_vddio_reg() is called twice with > devm_pm_runtime_enable() used. > Does it make sense? > > /Sean with pm_runtime_enable(): root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko [ 3793.713077] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt [ 3793.727728] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity... [ 3793.737660] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator [ 3793.856891] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator [ 3793.866872] inv_icm42600_enable_regulator_vddio() enable vddio [ 3793.920739] inv_icm42600_runtime_suspend() disable vddio root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600 [ 3796.954850] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio() [ 3796.954910] inv_icm42600_enable_regulator_vddio() enable vddio [ 3796.985140] inv_icm42600_disable_vddio_reg() disable vddio with devm_pm_runtime_enable(): root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko [ 3852.873887] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt [ 3852.888715] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity... [ 3852.898514] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator [ 3853.016890] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator [ 3853.026860] inv_icm42600_enable_regulator_vddio() enable vddio [ 3853.080835] inv_icm42600_runtime_suspend() disable vddio root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600 [ 3854.448461] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio() [ 3854.448540] inv_icm42600_enable_regulator_vddio() enable vddio [ 3854.467061] inv_icm42600_runtime_suspend() disable vddio [ 3854.477170] inv_icm42600_disable_vddio_reg() disable vddio [ 3854.483835] ------------[ cut here ]------------ [ 3854.483912] WARNING: CPU: 0 PID: 582 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0 [ 3854.497853] unbalanced disables for regulator Is the way from here to remove the devm_add_action_or_reset(dev, inv_icm42600_disable_vddio_reg... ? /Sean
On Mon, 14 Jul 2025 07:42:43 +0000 Sean Nyekjaer <sean@geanix.com> wrote: > On Mon, Jul 14, 2025 at 07:24:57AM +0100, Sean Nyekjaer wrote: > > On Sun, Jul 13, 2025 at 03:28:10PM +0100, Jonathan Cameron wrote: > > > On Wed, 09 Jul 2025 14:35:12 +0200 > > > Sean Nyekjaer <sean@geanix.com> wrote: > > > > > > > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put() > > > > calls during probe. These are not required when the device is marked > > > > active via pm_runtime_set_active() before enabling pm_runtime with > > > > pm_runtime_enable(). > > > > > > > > Also remove the redundant pm_runtime_put_sync() call from the cleanup > > > > path, since the core is not incrementing the usage count beforehand. > > > > > > > > This simplifies the PM setup and avoids manipulating the usage counter > > > > unnecessarily. > > > > > > Could we switch directly to using devm_pm_runtime_enable() for this driver? > > > > > > At first glance looks like this code is missing the disable of autosuspend > > > that should be there (which devm_pm_runtime_enable() will also handle). > > > > > > > I have tried to use devm_pm_runtime_enable() but on rmmod it warns > > "unbalanced disables for regulator" > > > > If I remove this: > > - ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vddio_reg, st); > > - if (ret) > > - return ret; > > > > Everything seems okay again. I have checked with printk's that > > inv_icm42600_disable_vddio_reg() is called twice with > > devm_pm_runtime_enable() used. > > Does it make sense? > > > > /Sean > > with pm_runtime_enable(): > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko > [ 3793.713077] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt > [ 3793.727728] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity... > [ 3793.737660] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator > [ 3793.856891] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator > [ 3793.866872] inv_icm42600_enable_regulator_vddio() enable vddio > [ 3793.920739] inv_icm42600_runtime_suspend() disable vddio > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600 > [ 3796.954850] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio() > [ 3796.954910] inv_icm42600_enable_regulator_vddio() enable vddio > [ 3796.985140] inv_icm42600_disable_vddio_reg() disable vddio > > with devm_pm_runtime_enable(): > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko > [ 3852.873887] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt > [ 3852.888715] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity... > [ 3852.898514] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator > [ 3853.016890] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator > [ 3853.026860] inv_icm42600_enable_regulator_vddio() enable vddio > [ 3853.080835] inv_icm42600_runtime_suspend() disable vddio > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600 > [ 3854.448461] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio() > [ 3854.448540] inv_icm42600_enable_regulator_vddio() enable vddio > [ 3854.467061] inv_icm42600_runtime_suspend() disable vddio As below What is the call path for this final suspend? Is it coming from update_autosuspend()? > [ 3854.477170] inv_icm42600_disable_vddio_reg() disable vddio > [ 3854.483835] ------------[ cut here ]------------ > [ 3854.483912] WARNING: CPU: 0 PID: 582 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0 > [ 3854.497853] unbalanced disables for regulator > > Is the way from here to remove the devm_add_action_or_reset(dev, > inv_icm42600_disable_vddio_reg... ? That will make a mess if runtime PM is not built into the kernel which is why an PM state in remove should return to the state before it was enabled in the first place (i.e. on!). That final runtime suspend surprises me. > > /Sean >
On Wed, Jul 16, 2025 at 09:00:10AM +0100, Jonathan Cameron wrote: > On Mon, 14 Jul 2025 07:42:43 +0000 > Sean Nyekjaer <sean@geanix.com> wrote: > > > On Mon, Jul 14, 2025 at 07:24:57AM +0100, Sean Nyekjaer wrote: > > > On Sun, Jul 13, 2025 at 03:28:10PM +0100, Jonathan Cameron wrote: > > > > On Wed, 09 Jul 2025 14:35:12 +0200 > > > > Sean Nyekjaer <sean@geanix.com> wrote: > > > > > > > > > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put() > > > > > calls during probe. These are not required when the device is marked > > > > > active via pm_runtime_set_active() before enabling pm_runtime with > > > > > pm_runtime_enable(). > > > > > > > > > > Also remove the redundant pm_runtime_put_sync() call from the cleanup > > > > > path, since the core is not incrementing the usage count beforehand. > > > > > > > > > > This simplifies the PM setup and avoids manipulating the usage counter > > > > > unnecessarily. > > > > > > > > Could we switch directly to using devm_pm_runtime_enable() for this driver? > > > > > > > > At first glance looks like this code is missing the disable of autosuspend > > > > that should be there (which devm_pm_runtime_enable() will also handle). > > > > > > > > > > I have tried to use devm_pm_runtime_enable() but on rmmod it warns > > > "unbalanced disables for regulator" > > > > > > If I remove this: > > > - ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vddio_reg, st); > > > - if (ret) > > > - return ret; > > > > > > Everything seems okay again. I have checked with printk's that > > > inv_icm42600_disable_vddio_reg() is called twice with > > > devm_pm_runtime_enable() used. > > > Does it make sense? > > > > > > /Sean > > > > with pm_runtime_enable(): > > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko > > [ 3793.713077] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt > > [ 3793.727728] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity... > > [ 3793.737660] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator > > [ 3793.856891] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator > > [ 3793.866872] inv_icm42600_enable_regulator_vddio() enable vddio > > [ 3793.920739] inv_icm42600_runtime_suspend() disable vddio > > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600 > > [ 3796.954850] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio() > > [ 3796.954910] inv_icm42600_enable_regulator_vddio() enable vddio > > [ 3796.985140] inv_icm42600_disable_vddio_reg() disable vddio > > > > with devm_pm_runtime_enable(): > > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko > > [ 3852.873887] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt > > [ 3852.888715] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity... > > [ 3852.898514] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator > > [ 3853.016890] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator > > [ 3853.026860] inv_icm42600_enable_regulator_vddio() enable vddio > > [ 3853.080835] inv_icm42600_runtime_suspend() disable vddio > > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600 > > [ 3854.448461] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio() > > [ 3854.448540] inv_icm42600_enable_regulator_vddio() enable vddio > > [ 3854.467061] inv_icm42600_runtime_suspend() disable vddio > > As below What is the call path for this final suspend? > Is it coming from update_autosuspend()? Yeah it looks like pm_runtime_dont_use_autosuspend() is calling runtime_suspend(). root@v4:/data/root# rmmod inv-icm42600-i2c inv-icm42600 [ 291.511085] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio() [ 291.511165] inv_icm42600_enable_regulator_vddio() enable vddio [ 291.532398] inv_icm42600_runtime_suspend() disable vddio [ 291.538517] CPU: 0 UID: 0 PID: 331 Comm: rmmod Tainted: G W 6.16.0-rc1-00202-g7fe6e564b5c9-dirty #146 VOLUNTARY [ 291.538559] Tainted: [W]=WARN [ 291.538566] Hardware name: Freescale i.MX6 Ultralite (Device Tree) [ 291.538575] Call trace: [ 291.538590] unwind_backtrace from show_stack+0x10/0x14 [ 291.538643] show_stack from dump_stack_lvl+0x54/0x68 [ 291.538679] dump_stack_lvl from inv_icm42600_runtime_suspend+0x68/0x6c [inv_icm42600] [ 291.538728] inv_icm42600_runtime_suspend [inv_icm42600] from __rpm_callback+0x48/0x18c [ 291.538775] __rpm_callback from rpm_callback+0x5c/0x68 [ 291.538815] rpm_callback from rpm_suspend+0xdc/0x584 [ 291.538853] rpm_suspend from pm_runtime_disable_action+0x30/0x5c [ 291.538885] pm_runtime_disable_action from devres_release_group+0x180/0x1a0 [ 291.538917] devres_release_group from i2c_device_remove+0x34/0x84 [ 291.538949] i2c_device_remove from device_release_driver_internal+0x180/0x1f4 [ 291.538976] device_release_driver_internal from driver_detach+0x54/0xa0 [ 291.538998] driver_detach from bus_remove_driver+0x58/0xa4 [ 291.539030] bus_remove_driver from sys_delete_module+0x16c/0x250 [ 291.539065] sys_delete_module from ret_fast_syscall+0x0/0x54 [ 291.539089] Exception stack(0xd0ab1fa8 to 0xd0ab1ff0) [ 291.539108] 1fa0: be94fe46 be94fd1c 017ccfa4 00000800 0000000a 017ccf68 [ 291.539126] 1fc0: be94fe46 be94fd1c 017ccf68 00000081 00000000 00000001 00000003 017cc190 [ 291.539139] 1fe0: b6c8be41 be94fadc 000179cb b6c8be48 [ 291.685102] inv_icm42600_disable_vddio_reg() disable vddio [ 291.694566] ------------[ cut here ]------------ [ 291.694621] WARNING: CPU: 0 PID: 331 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0 [ 291.708496] unbalanced disables for regulator-dummy [ 291.713391] Modules linked in: inv_icm42600_i2c(-) inv_icm42600 inv_sensors_timestamp [last unloaded: inv_icm42600] [ 291.723939] CPU: 0 UID: 0 PID: 331 Comm: rmmod Tainted: G W 6.16.0-rc1-00202-g7fe6e564b5c9-dirty #146 VOLUNTARY [ 291.735620] Tainted: [W]=WARN [ 291.738598] Hardware name: Freescale i.MX6 Ultralite (Device Tree) [ 291.744789] Call trace: [ 291.744807] unwind_backtrace from show_stack+0x10/0x14 [ 291.752614] show_stack from dump_stack_lvl+0x54/0x68 [ 291.757704] dump_stack_lvl from __warn+0x7c/0xe0 [ 291.762437] __warn from warn_slowpath_fmt+0x124/0x18c [ 291.767602] warn_slowpath_fmt from _regulator_disable+0x140/0x1a0 [ 291.773812] _regulator_disable from regulator_disable+0x48/0x80 [ 291.779843] regulator_disable from devres_release_group+0x180/0x1a0 [ 291.786231] devres_release_group from i2c_device_remove+0x34/0x84 [ 291.792446] i2c_device_remove from device_release_driver_internal+0x180/0x1f4 [ 291.799699] device_release_driver_internal from driver_detach+0x54/0xa0 [ 291.806427] driver_detach from bus_remove_driver+0x58/0xa4 [ 291.812033] bus_remove_driver from sys_delete_module+0x16c/0x250 [ 291.818160] sys_delete_module from ret_fast_syscall+0x0/0x54 [ 291.823934] Exception stack(0xd0ab1fa8 to 0xd0ab1ff0) [ 291.829007] 1fa0: be94fe46 be94fd1c 017ccfa4 00000800 0000000a 017ccf68 [ 291.837205] 1fc0: be94fe46 be94fd1c 017ccf68 00000081 00000000 00000001 00000003 017cc190 [ 291.845397] 1fe0: b6c8be41 be94fadc 000179cb b6c8be48 [ 291.850632] ---[ end trace 0000000000000000 ]---0 > > > [ 3854.477170] inv_icm42600_disable_vddio_reg() disable vddio > > [ 3854.483835] ------------[ cut here ]------------ > > [ 3854.483912] WARNING: CPU: 0 PID: 582 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0 > > [ 3854.497853] unbalanced disables for regulator > > > > Is the way from here to remove the devm_add_action_or_reset(dev, > > inv_icm42600_disable_vddio_reg... ? > > That will make a mess if runtime PM is not built into > the kernel which is why an PM state in remove should return to the state > before it was enabled in the first place (i.e. on!). > That final runtime suspend surprises me. Got it :) /Sean
On Fri, 18 Jul 2025 06:40:50 +0000 Sean Nyekjaer <sean@geanix.com> wrote: > On Wed, Jul 16, 2025 at 09:00:10AM +0100, Jonathan Cameron wrote: > > On Mon, 14 Jul 2025 07:42:43 +0000 > > Sean Nyekjaer <sean@geanix.com> wrote: > > > > > On Mon, Jul 14, 2025 at 07:24:57AM +0100, Sean Nyekjaer wrote: > > > > On Sun, Jul 13, 2025 at 03:28:10PM +0100, Jonathan Cameron wrote: > > > > > On Wed, 09 Jul 2025 14:35:12 +0200 > > > > > Sean Nyekjaer <sean@geanix.com> wrote: > > > > > > > > > > > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put() > > > > > > calls during probe. These are not required when the device is marked > > > > > > active via pm_runtime_set_active() before enabling pm_runtime with > > > > > > pm_runtime_enable(). > > > > > > > > > > > > Also remove the redundant pm_runtime_put_sync() call from the cleanup > > > > > > path, since the core is not incrementing the usage count beforehand. > > > > > > > > > > > > This simplifies the PM setup and avoids manipulating the usage counter > > > > > > unnecessarily. > > > > > > > > > > Could we switch directly to using devm_pm_runtime_enable() for this driver? > > > > > > > > > > At first glance looks like this code is missing the disable of autosuspend > > > > > that should be there (which devm_pm_runtime_enable() will also handle). > > > > > > > > > > > > > I have tried to use devm_pm_runtime_enable() but on rmmod it warns > > > > "unbalanced disables for regulator" > > > > > > > > If I remove this: > > > > - ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vddio_reg, st); > > > > - if (ret) > > > > - return ret; > > > > > > > > Everything seems okay again. I have checked with printk's that > > > > inv_icm42600_disable_vddio_reg() is called twice with > > > > devm_pm_runtime_enable() used. > > > > Does it make sense? > > > > > > > > /Sean > > > > > > with pm_runtime_enable(): > > > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko > > > [ 3793.713077] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt > > > [ 3793.727728] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity... > > > [ 3793.737660] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator > > > [ 3793.856891] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator > > > [ 3793.866872] inv_icm42600_enable_regulator_vddio() enable vddio > > > [ 3793.920739] inv_icm42600_runtime_suspend() disable vddio > > > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600 > > > [ 3796.954850] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio() > > > [ 3796.954910] inv_icm42600_enable_regulator_vddio() enable vddio > > > [ 3796.985140] inv_icm42600_disable_vddio_reg() disable vddio > > > > > > with devm_pm_runtime_enable(): > > > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko > > > [ 3852.873887] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt > > > [ 3852.888715] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity... > > > [ 3852.898514] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator > > > [ 3853.016890] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator > > > [ 3853.026860] inv_icm42600_enable_regulator_vddio() enable vddio > > > [ 3853.080835] inv_icm42600_runtime_suspend() disable vddio > > > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600 > > > [ 3854.448461] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio() > > > [ 3854.448540] inv_icm42600_enable_regulator_vddio() enable vddio > > > [ 3854.467061] inv_icm42600_runtime_suspend() disable vddio > > > > As below What is the call path for this final suspend? > > Is it coming from update_autosuspend()? > > Yeah it looks like pm_runtime_dont_use_autosuspend() is calling runtime_suspend(). ok. So how are we supposed to handle this? Seems like it would be a fairly common situation. devm_pm_runtime_set_active_enabled() might be relevant. I get confused by all the reference counter complexity in runtime pm but is devm_pm_runtime_get_noresume() what we want? That will decrement the usage count with no action just before we hit the point were the suspend or not decision is made. So I think that will mean the device things it is already suspended when you hit the path here and so not do it again? There seems to be only one user of this stuff though: https://elixir.bootlin.com/linux/v6.15.6/source/drivers/spi/atmel-quadspi.c#L1440 > > root@v4:/data/root# rmmod inv-icm42600-i2c inv-icm42600 > [ 291.511085] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio() > [ 291.511165] inv_icm42600_enable_regulator_vddio() enable vddio > [ 291.532398] inv_icm42600_runtime_suspend() disable vddio > [ 291.538517] CPU: 0 UID: 0 PID: 331 Comm: rmmod Tainted: G W 6.16.0-rc1-00202-g7fe6e564b5c9-dirty #146 VOLUNTARY > [ 291.538559] Tainted: [W]=WARN > [ 291.538566] Hardware name: Freescale i.MX6 Ultralite (Device Tree) > [ 291.538575] Call trace: > [ 291.538590] unwind_backtrace from show_stack+0x10/0x14 > [ 291.538643] show_stack from dump_stack_lvl+0x54/0x68 > [ 291.538679] dump_stack_lvl from inv_icm42600_runtime_suspend+0x68/0x6c [inv_icm42600] > [ 291.538728] inv_icm42600_runtime_suspend [inv_icm42600] from __rpm_callback+0x48/0x18c > [ 291.538775] __rpm_callback from rpm_callback+0x5c/0x68 > [ 291.538815] rpm_callback from rpm_suspend+0xdc/0x584 > [ 291.538853] rpm_suspend from pm_runtime_disable_action+0x30/0x5c > [ 291.538885] pm_runtime_disable_action from devres_release_group+0x180/0x1a0 > [ 291.538917] devres_release_group from i2c_device_remove+0x34/0x84 > [ 291.538949] i2c_device_remove from device_release_driver_internal+0x180/0x1f4 > [ 291.538976] device_release_driver_internal from driver_detach+0x54/0xa0 > [ 291.538998] driver_detach from bus_remove_driver+0x58/0xa4 > [ 291.539030] bus_remove_driver from sys_delete_module+0x16c/0x250 > [ 291.539065] sys_delete_module from ret_fast_syscall+0x0/0x54 > [ 291.539089] Exception stack(0xd0ab1fa8 to 0xd0ab1ff0) > [ 291.539108] 1fa0: be94fe46 be94fd1c 017ccfa4 00000800 0000000a 017ccf68 > [ 291.539126] 1fc0: be94fe46 be94fd1c 017ccf68 00000081 00000000 00000001 00000003 017cc190 > [ 291.539139] 1fe0: b6c8be41 be94fadc 000179cb b6c8be48 > [ 291.685102] inv_icm42600_disable_vddio_reg() disable vddio > [ 291.694566] ------------[ cut here ]------------ > [ 291.694621] WARNING: CPU: 0 PID: 331 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0 > [ 291.708496] unbalanced disables for regulator-dummy > [ 291.713391] Modules linked in: inv_icm42600_i2c(-) inv_icm42600 inv_sensors_timestamp [last unloaded: inv_icm42600] > [ 291.723939] CPU: 0 UID: 0 PID: 331 Comm: rmmod Tainted: G W 6.16.0-rc1-00202-g7fe6e564b5c9-dirty #146 VOLUNTARY > [ 291.735620] Tainted: [W]=WARN > [ 291.738598] Hardware name: Freescale i.MX6 Ultralite (Device Tree) > [ 291.744789] Call trace: > [ 291.744807] unwind_backtrace from show_stack+0x10/0x14 > [ 291.752614] show_stack from dump_stack_lvl+0x54/0x68 > [ 291.757704] dump_stack_lvl from __warn+0x7c/0xe0 > [ 291.762437] __warn from warn_slowpath_fmt+0x124/0x18c > [ 291.767602] warn_slowpath_fmt from _regulator_disable+0x140/0x1a0 > [ 291.773812] _regulator_disable from regulator_disable+0x48/0x80 > [ 291.779843] regulator_disable from devres_release_group+0x180/0x1a0 > [ 291.786231] devres_release_group from i2c_device_remove+0x34/0x84 > [ 291.792446] i2c_device_remove from device_release_driver_internal+0x180/0x1f4 > [ 291.799699] device_release_driver_internal from driver_detach+0x54/0xa0 > [ 291.806427] driver_detach from bus_remove_driver+0x58/0xa4 > [ 291.812033] bus_remove_driver from sys_delete_module+0x16c/0x250 > [ 291.818160] sys_delete_module from ret_fast_syscall+0x0/0x54 > [ 291.823934] Exception stack(0xd0ab1fa8 to 0xd0ab1ff0) > [ 291.829007] 1fa0: be94fe46 be94fd1c 017ccfa4 00000800 0000000a 017ccf68 > [ 291.837205] 1fc0: be94fe46 be94fd1c 017ccf68 00000081 00000000 00000001 00000003 017cc190 > [ 291.845397] 1fe0: b6c8be41 be94fadc 000179cb b6c8be48 > [ 291.850632] ---[ end trace 0000000000000000 ]---0 > > > > > > [ 3854.477170] inv_icm42600_disable_vddio_reg() disable vddio > > > [ 3854.483835] ------------[ cut here ]------------ > > > [ 3854.483912] WARNING: CPU: 0 PID: 582 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0 > > > [ 3854.497853] unbalanced disables for regulator > > > > > > Is the way from here to remove the devm_add_action_or_reset(dev, > > > inv_icm42600_disable_vddio_reg... ? > > > > That will make a mess if runtime PM is not built into > > the kernel which is why an PM state in remove should return to the state > > before it was enabled in the first place (i.e. on!). > > That final runtime suspend surprises me. > > Got it :) > > /Sean > >
On Sat, 19 Jul 2025 12:47:11 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 18 Jul 2025 06:40:50 +0000 > Sean Nyekjaer <sean@geanix.com> wrote: > > > On Wed, Jul 16, 2025 at 09:00:10AM +0100, Jonathan Cameron wrote: > > > On Mon, 14 Jul 2025 07:42:43 +0000 > > > Sean Nyekjaer <sean@geanix.com> wrote: > > > > > > > On Mon, Jul 14, 2025 at 07:24:57AM +0100, Sean Nyekjaer wrote: > > > > > On Sun, Jul 13, 2025 at 03:28:10PM +0100, Jonathan Cameron wrote: > > > > > > On Wed, 09 Jul 2025 14:35:12 +0200 > > > > > > Sean Nyekjaer <sean@geanix.com> wrote: > > > > > > > > > > > > > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put() > > > > > > > calls during probe. These are not required when the device is marked > > > > > > > active via pm_runtime_set_active() before enabling pm_runtime with > > > > > > > pm_runtime_enable(). > > > > > > > > > > > > > > Also remove the redundant pm_runtime_put_sync() call from the cleanup > > > > > > > path, since the core is not incrementing the usage count beforehand. > > > > > > > > > > > > > > This simplifies the PM setup and avoids manipulating the usage counter > > > > > > > unnecessarily. > > > > > > > > > > > > Could we switch directly to using devm_pm_runtime_enable() for this driver? > > > > > > > > > > > > At first glance looks like this code is missing the disable of autosuspend > > > > > > that should be there (which devm_pm_runtime_enable() will also handle). > > > > > > > > > > > > > > > > I have tried to use devm_pm_runtime_enable() but on rmmod it warns > > > > > "unbalanced disables for regulator" > > > > > > > > > > If I remove this: > > > > > - ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vddio_reg, st); > > > > > - if (ret) > > > > > - return ret; > > > > > > > > > > Everything seems okay again. I have checked with printk's that > > > > > inv_icm42600_disable_vddio_reg() is called twice with > > > > > devm_pm_runtime_enable() used. > > > > > Does it make sense? > > > > > > > > > > /Sean > > > > > > > > with pm_runtime_enable(): > > > > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko > > > > [ 3793.713077] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt > > > > [ 3793.727728] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity... > > > > [ 3793.737660] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator > > > > [ 3793.856891] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator > > > > [ 3793.866872] inv_icm42600_enable_regulator_vddio() enable vddio > > > > [ 3793.920739] inv_icm42600_runtime_suspend() disable vddio > > > > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600 > > > > [ 3796.954850] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio() > > > > [ 3796.954910] inv_icm42600_enable_regulator_vddio() enable vddio > > > > [ 3796.985140] inv_icm42600_disable_vddio_reg() disable vddio > > > > > > > > with devm_pm_runtime_enable(): > > > > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko > > > > [ 3852.873887] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt > > > > [ 3852.888715] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity... > > > > [ 3852.898514] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator > > > > [ 3853.016890] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator > > > > [ 3853.026860] inv_icm42600_enable_regulator_vddio() enable vddio > > > > [ 3853.080835] inv_icm42600_runtime_suspend() disable vddio > > > > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600 > > > > [ 3854.448461] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio() > > > > [ 3854.448540] inv_icm42600_enable_regulator_vddio() enable vddio > > > > [ 3854.467061] inv_icm42600_runtime_suspend() disable vddio > > > > > > As below What is the call path for this final suspend? > > > Is it coming from update_autosuspend()? > > > > Yeah it looks like pm_runtime_dont_use_autosuspend() is calling runtime_suspend(). > > ok. So how are we supposed to handle this? Seems like it would be a fairly common > situation. devm_pm_runtime_set_active_enabled() might be relevant. > I get confused by all the reference counter > complexity in runtime pm but is devm_pm_runtime_get_noresume() what we want? > That will decrement the usage count with no action just before we hit the point > were the suspend or not decision is made. So I think that will mean the device > things it is already suspended when you hit the path here and so not do it again? > > There seems to be only one user of this stuff though: > https://elixir.bootlin.com/linux/v6.15.6/source/drivers/spi/atmel-quadspi.c#L1440 Note that example does a get in the remove callback(). Seems odd but maybe we need a devm_pm_runtime_*put() that results in an a get in the remove path to ensure counts all match. > > > > root@v4:/data/root# rmmod inv-icm42600-i2c inv-icm42600 > > [ 291.511085] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio() > > [ 291.511165] inv_icm42600_enable_regulator_vddio() enable vddio > > [ 291.532398] inv_icm42600_runtime_suspend() disable vddio > > [ 291.538517] CPU: 0 UID: 0 PID: 331 Comm: rmmod Tainted: G W 6.16.0-rc1-00202-g7fe6e564b5c9-dirty #146 VOLUNTARY > > [ 291.538559] Tainted: [W]=WARN > > [ 291.538566] Hardware name: Freescale i.MX6 Ultralite (Device Tree) > > [ 291.538575] Call trace: > > [ 291.538590] unwind_backtrace from show_stack+0x10/0x14 > > [ 291.538643] show_stack from dump_stack_lvl+0x54/0x68 > > [ 291.538679] dump_stack_lvl from inv_icm42600_runtime_suspend+0x68/0x6c [inv_icm42600] > > [ 291.538728] inv_icm42600_runtime_suspend [inv_icm42600] from __rpm_callback+0x48/0x18c > > [ 291.538775] __rpm_callback from rpm_callback+0x5c/0x68 > > [ 291.538815] rpm_callback from rpm_suspend+0xdc/0x584 > > [ 291.538853] rpm_suspend from pm_runtime_disable_action+0x30/0x5c > > [ 291.538885] pm_runtime_disable_action from devres_release_group+0x180/0x1a0 > > [ 291.538917] devres_release_group from i2c_device_remove+0x34/0x84 > > [ 291.538949] i2c_device_remove from device_release_driver_internal+0x180/0x1f4 > > [ 291.538976] device_release_driver_internal from driver_detach+0x54/0xa0 > > [ 291.538998] driver_detach from bus_remove_driver+0x58/0xa4 > > [ 291.539030] bus_remove_driver from sys_delete_module+0x16c/0x250 > > [ 291.539065] sys_delete_module from ret_fast_syscall+0x0/0x54 > > [ 291.539089] Exception stack(0xd0ab1fa8 to 0xd0ab1ff0) > > [ 291.539108] 1fa0: be94fe46 be94fd1c 017ccfa4 00000800 0000000a 017ccf68 > > [ 291.539126] 1fc0: be94fe46 be94fd1c 017ccf68 00000081 00000000 00000001 00000003 017cc190 > > [ 291.539139] 1fe0: b6c8be41 be94fadc 000179cb b6c8be48 > > [ 291.685102] inv_icm42600_disable_vddio_reg() disable vddio > > [ 291.694566] ------------[ cut here ]------------ > > [ 291.694621] WARNING: CPU: 0 PID: 331 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0 > > [ 291.708496] unbalanced disables for regulator-dummy > > [ 291.713391] Modules linked in: inv_icm42600_i2c(-) inv_icm42600 inv_sensors_timestamp [last unloaded: inv_icm42600] > > [ 291.723939] CPU: 0 UID: 0 PID: 331 Comm: rmmod Tainted: G W 6.16.0-rc1-00202-g7fe6e564b5c9-dirty #146 VOLUNTARY > > [ 291.735620] Tainted: [W]=WARN > > [ 291.738598] Hardware name: Freescale i.MX6 Ultralite (Device Tree) > > [ 291.744789] Call trace: > > [ 291.744807] unwind_backtrace from show_stack+0x10/0x14 > > [ 291.752614] show_stack from dump_stack_lvl+0x54/0x68 > > [ 291.757704] dump_stack_lvl from __warn+0x7c/0xe0 > > [ 291.762437] __warn from warn_slowpath_fmt+0x124/0x18c > > [ 291.767602] warn_slowpath_fmt from _regulator_disable+0x140/0x1a0 > > [ 291.773812] _regulator_disable from regulator_disable+0x48/0x80 > > [ 291.779843] regulator_disable from devres_release_group+0x180/0x1a0 > > [ 291.786231] devres_release_group from i2c_device_remove+0x34/0x84 > > [ 291.792446] i2c_device_remove from device_release_driver_internal+0x180/0x1f4 > > [ 291.799699] device_release_driver_internal from driver_detach+0x54/0xa0 > > [ 291.806427] driver_detach from bus_remove_driver+0x58/0xa4 > > [ 291.812033] bus_remove_driver from sys_delete_module+0x16c/0x250 > > [ 291.818160] sys_delete_module from ret_fast_syscall+0x0/0x54 > > [ 291.823934] Exception stack(0xd0ab1fa8 to 0xd0ab1ff0) > > [ 291.829007] 1fa0: be94fe46 be94fd1c 017ccfa4 00000800 0000000a 017ccf68 > > [ 291.837205] 1fc0: be94fe46 be94fd1c 017ccf68 00000081 00000000 00000001 00000003 017cc190 > > [ 291.845397] 1fe0: b6c8be41 be94fadc 000179cb b6c8be48 > > [ 291.850632] ---[ end trace 0000000000000000 ]---0 > > > > > > > > > [ 3854.477170] inv_icm42600_disable_vddio_reg() disable vddio > > > > [ 3854.483835] ------------[ cut here ]------------ > > > > [ 3854.483912] WARNING: CPU: 0 PID: 582 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0 > > > > [ 3854.497853] unbalanced disables for regulator > > > > > > > > Is the way from here to remove the devm_add_action_or_reset(dev, > > > > inv_icm42600_disable_vddio_reg... ? > > > > > > That will make a mess if runtime PM is not built into > > > the kernel which is why an PM state in remove should return to the state > > > before it was enabled in the first place (i.e. on!). > > > That final runtime suspend surprises me. > > > > Got it :) > > > > /Sean > > > > > >
On Sat, Jul 19, 2025 at 02:32:50PM +0100, Jonathan Cameron wrote: > On Sat, 19 Jul 2025 12:47:11 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Fri, 18 Jul 2025 06:40:50 +0000 > > Sean Nyekjaer <sean@geanix.com> wrote: > > > > > On Wed, Jul 16, 2025 at 09:00:10AM +0100, Jonathan Cameron wrote: > > > > On Mon, 14 Jul 2025 07:42:43 +0000 > > > > Sean Nyekjaer <sean@geanix.com> wrote: > > > > > > > > > On Mon, Jul 14, 2025 at 07:24:57AM +0100, Sean Nyekjaer wrote: > > > > > > On Sun, Jul 13, 2025 at 03:28:10PM +0100, Jonathan Cameron wrote: > > > > > > > On Wed, 09 Jul 2025 14:35:12 +0200 > > > > > > > Sean Nyekjaer <sean@geanix.com> wrote: > > > > > > > > > > > > > > > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put() > > > > > > > > calls during probe. These are not required when the device is marked > > > > > > > > active via pm_runtime_set_active() before enabling pm_runtime with > > > > > > > > pm_runtime_enable(). > > > > > > > > > > > > > > > > Also remove the redundant pm_runtime_put_sync() call from the cleanup > > > > > > > > path, since the core is not incrementing the usage count beforehand. > > > > > > > > > > > > > > > > This simplifies the PM setup and avoids manipulating the usage counter > > > > > > > > unnecessarily. > > > > > > > > > > > > > > Could we switch directly to using devm_pm_runtime_enable() for this driver? > > > > > > > > > > > > > > At first glance looks like this code is missing the disable of autosuspend > > > > > > > that should be there (which devm_pm_runtime_enable() will also handle). > > > > > > > > > > > > > > > > > > > I have tried to use devm_pm_runtime_enable() but on rmmod it warns > > > > > > "unbalanced disables for regulator" > > > > > > > > > > > > If I remove this: > > > > > > - ret = devm_add_action_or_reset(dev, inv_icm42600_disable_vddio_reg, st); > > > > > > - if (ret) > > > > > > - return ret; > > > > > > > > > > > > Everything seems okay again. I have checked with printk's that > > > > > > inv_icm42600_disable_vddio_reg() is called twice with > > > > > > devm_pm_runtime_enable() used. > > > > > > Does it make sense? > > > > > > > > > > > > /Sean > > > > > > > > > > with pm_runtime_enable(): > > > > > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko > > > > > [ 3793.713077] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt > > > > > [ 3793.727728] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity... > > > > > [ 3793.737660] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator > > > > > [ 3793.856891] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator > > > > > [ 3793.866872] inv_icm42600_enable_regulator_vddio() enable vddio > > > > > [ 3793.920739] inv_icm42600_runtime_suspend() disable vddio > > > > > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600 > > > > > [ 3796.954850] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio() > > > > > [ 3796.954910] inv_icm42600_enable_regulator_vddio() enable vddio > > > > > [ 3796.985140] inv_icm42600_disable_vddio_reg() disable vddio > > > > > > > > > > with devm_pm_runtime_enable(): > > > > > root@v4:/data/root# insmod /tmp/inv-icm42600.ko; insmod /tmp/inv-icm42600-i2c.ko > > > > > [ 3852.873887] inv-icm42600-i2c 1-0068: no INT1 interrupt defined, fallback to first interrupt > > > > > [ 3852.888715] inv-icm42600-i2c 1-0068: mounting matrix not found: using identity... > > > > > [ 3852.898514] inv-icm42600-i2c 1-0068: supply vdd not found, using dummy regulator > > > > > [ 3853.016890] inv-icm42600-i2c 1-0068: supply vddio not found, using dummy regulator > > > > > [ 3853.026860] inv_icm42600_enable_regulator_vddio() enable vddio > > > > > [ 3853.080835] inv_icm42600_runtime_suspend() disable vddio > > > > > root@v4:/data/root# rmmod inv_icm42600_i2c inv_icm42600 > > > > > [ 3854.448461] inv_icm42600_runtime_resume() -> inv_icm42600_enable_regulator_vddio() > > > > > [ 3854.448540] inv_icm42600_enable_regulator_vddio() enable vddio > > > > > [ 3854.467061] inv_icm42600_runtime_suspend() disable vddio > > > > > > > > As below What is the call path for this final suspend? > > > > Is it coming from update_autosuspend()? > > > > > > Yeah it looks like pm_runtime_dont_use_autosuspend() is calling runtime_suspend(). > > > > ok. So how are we supposed to handle this? Seems like it would be a fairly common > > situation. devm_pm_runtime_set_active_enabled() might be relevant. > > I get confused by all the reference counter > > complexity in runtime pm but is devm_pm_runtime_get_noresume() what we want? > > That will decrement the usage count with no action just before we hit the point > > were the suspend or not decision is made. So I think that will mean the device > > things it is already suspended when you hit the path here and so not do it again? > > > > There seems to be only one user of this stuff though: > > https://elixir.bootlin.com/linux/v6.15.6/source/drivers/spi/atmel-quadspi.c#L1440 > Note that example does a get in the remove callback(). Seems odd but maybe > we need a devm_pm_runtime_*put() that results in an a get in the remove path to ensure counts > all match. > Hi, For what is worth; It looks like drivers/iio/adc/ti-ads1100.c does same as we are trying to do here. It also fails in rmmod: root@v4:/data/root# insmod /tmp/ti-ads1100.ko [ 65.778107] ads1100 1-0049: supply vdd not found, using dummy regulator root@v4:/data/root# rmmod /tmp/ti-ads1100.ko [ 73.625923] ------------[ cut here ]------------ [ 73.625976] WARNING: CPU: 0 PID: 299 at drivers/regulator/core.c:3016 _regulator_disable+0x140/0x1a0 [ 73.639939] unbalanced disables for regulator-dummy [ 73.644833] Modules linked in: ti_ads1100(-) [ 73.649178] CPU: 0 UID: 0 PID: 299 Comm: rmmod Not tainted 6.16.0-rc1-00203-ge84dc7fb9038-dirty #160 VOLUNTARY [ 73.659289] Hardware name: Freescale i.MX6 Ultralite (Device Tree) [ 73.665479] Call trace: [ 73.665499] unwind_backtrace from show_stack+0x10/0x14 [ 73.673301] show_stack from dump_stack_lvl+0x54/0x68 [ 73.678389] dump_stack_lvl from __warn+0x7c/0xe0 [ 73.683120] __warn from warn_slowpath_fmt+0x124/0x18c [ 73.688285] warn_slowpath_fmt from _regulator_disable+0x140/0x1a0 [ 73.694494] _regulator_disable from regulator_disable+0x48/0x80 [ 73.700529] regulator_disable from devres_release_group+0x180/0x1a0 [ 73.706925] devres_release_group from i2c_device_remove+0x34/0x84 [ 73.713146] i2c_device_remove from device_release_driver_internal+0x180/0x1f4 [ 73.720402] device_release_driver_internal from driver_detach+0x54/0xa0 [ 73.727130] driver_detach from bus_remove_driver+0x58/0xa4 [ 73.732735] bus_remove_driver from sys_delete_module+0x16c/0x250 [ 73.738857] sys_delete_module from ret_fast_syscall+0x0/0x54 [ 73.744627] Exception stack(0xd0d5dfa8 to 0xd0d5dff0) [ 73.749701] dfa0: be970e51 be970d2c 00233fbc 00000800 0000000a 00233f80 [ 73.757896] dfc0: be970e51 be970d2c 00233f80 00000081 00000000 00000001 00000002 00233190 [ 73.766086] dfe0: b6c87e41 be970aec 000179cb b6c87e48 [ 73.771228] ---[ end trace 0000000000000000 ]--- I will try something similar to this: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=4154e767354140db7804207117e7238fb337b0e7 /Sean
On Wed, Jul 09, 2025 at 02:35:12PM +0200, Sean Nyekjaer wrote:
> Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> calls during probe. These are not required when the device is marked
> active via pm_runtime_set_active() before enabling pm_runtime with
> pm_runtime_enable().
Hmm... What will happen if the autosuspend triggers just before going out from
the probe when this change is applied?
> Also remove the redundant pm_runtime_put_sync() call from the cleanup
> path, since the core is not incrementing the usage count beforehand.
This is interesting. Have anybody actually tried to see refcount WARN about this?
> This simplifies the PM setup and avoids manipulating the usage counter
> unnecessarily.
> Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
This should be the first, or close to the beginning of the series, patch.
--
With Best Regards,
Andy Shevchenko
Hi Andy,
On Thu, Jul 10, 2025 at 12:08:40PM +0100, Andy Shevchenko wrote:
> On Wed, Jul 09, 2025 at 02:35:12PM +0200, Sean Nyekjaer wrote:
> > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> > calls during probe. These are not required when the device is marked
> > active via pm_runtime_set_active() before enabling pm_runtime with
> > pm_runtime_enable().
>
> Hmm... What will happen if the autosuspend triggers just before going out from
> the probe when this change is applied?
Nothing, as pm_runtime is enabled as the last step in probe.
>
> > Also remove the redundant pm_runtime_put_sync() call from the cleanup
> > path, since the core is not incrementing the usage count beforehand.
>
> This is interesting. Have anybody actually tried to see refcount WARN about this?
>
> > This simplifies the PM setup and avoids manipulating the usage counter
> > unnecessarily.
>
> > Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
> > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
>
> This should be the first, or close to the beginning of the series, patch.
Ok, but help me understand why?
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thanks for the review :)
/Sean
On Thu, Jul 10, 2025 at 10:45:43AM +0000, Sean Nyekjaer wrote:
> On Thu, Jul 10, 2025 at 12:08:40PM +0100, Andy Shevchenko wrote:
> > On Wed, Jul 09, 2025 at 02:35:12PM +0200, Sean Nyekjaer wrote:
> > > Remove unnecessary pm_runtime_get_noresume() and pm_runtime_put()
> > > calls during probe. These are not required when the device is marked
> > > active via pm_runtime_set_active() before enabling pm_runtime with
> > > pm_runtime_enable().
> >
> > Hmm... What will happen if the autosuspend triggers just before going out from
> > the probe when this change is applied?
>
> Nothing, as pm_runtime is enabled as the last step in probe.
Note, that PM runtime can be enabled by userspace or disabled.
> > > Also remove the redundant pm_runtime_put_sync() call from the cleanup
> > > path, since the core is not incrementing the usage count beforehand.
> >
> > This is interesting. Have anybody actually tried to see refcount WARN about this?
> >
> > > This simplifies the PM setup and avoids manipulating the usage counter
> > > unnecessarily.
> >
> > > Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
> > > Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> >
> > This should be the first, or close to the beginning of the series, patch.
>
> Ok, but help me understand why?
The commits that are marked as Fixes should be easy to backport. When
the series is started with the refactoring, it's not good as potentially
backporting material might (even undeliberately) get a dependency on the
refactoring.
--
With Best Regards,
Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.