lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250719143250.6799eb5d@jic23-huawei>
Date: Sat, 19 Jul 2025 14:32:50 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Sean Nyekjaer <sean@...nix.com>
Cc: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>, David Lechner
 <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, Andy
 Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/6] iio: imu: inv_icm42600: Simplify pm_runtime setup

On Sat, 19 Jul 2025 12:47:11 +0100
Jonathan Cameron <jic23@...nel.org> wrote:

> On Fri, 18 Jul 2025 06:40:50 +0000
> Sean Nyekjaer <sean@...nix.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@...nix.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@...nix.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
> > 
> >   
> 
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ