linux-kernel - Re: [PATCH 6/6] iio: imu: inv_icm42600: Avoid configuring if already pm

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: <20250716090655.0ba30a2a@jic23-huawei>
Date: Wed, 16 Jul 2025 09:06:55 +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 6/6] iio: imu: inv_icm42600: Avoid configuring if
 already pm_runtime suspended

On Mon, 14 Jul 2025 10:15:59 +0000
Sean Nyekjaer <sean@...nix.com> wrote:

> On Sun, Jul 13, 2025 at 03:32:27PM +0100, Jonathan Cameron wrote:
> > On Wed, 09 Jul 2025 14:35:14 +0200
> > Sean Nyekjaer <sean@...nix.com> wrote:
> >   
> > > Do as in suspend, skip resume configuration steps if the device is already
> > > pm_runtime suspended. This avoids reconfiguring a device that is already
> > > in the correct low-power state and ensures that pm_runtimeM handles the
> > > power state transitions properly.
> > >
> > > Fixes: 31c24c1e93c3 ("iio: imu: inv_icm42600: add core of new inv_icm42600 driver")
> > > Signed-off-by: Sean Nyekjaer <sean@...nix.com>
> > > ---  
> > 
> > Not really related to what you have here, but this code would really
> > benefit from using guard(mutex)()
> > 
> > Jonathan  
> 
> I have converted most of this driver to use guard(mutex).
> 
> Does it make sense to use guard(mutex) in functions that still relies on
> goto error out? Like...

No - general rule is never combine cleanup.h magic like guard with gotos.

If you do want to use it in cases like this, factor out the stuff
> 
> static int inv_icm42600_temp_read(struct inv_icm42600_state *st, s16 *temp)
> {
> 	struct device *dev = regmap_get_device(st->map);
> 	__be16 *raw;
> 	int ret;
> 
> 	pm_runtime_get_sync(dev);
> 	mutex_lock(&st->lock);
> 

from here

> 	ret = inv_icm42600_set_temp_conf(st, true, NULL);
> 	if (ret)
> 		goto exit;
> 
> 	raw = (__be16 *)&st->buffer[0];
> 	ret = regmap_bulk_read(st->map, INV_ICM42600_REG_TEMP_DATA, raw, sizeof(*raw));
> 	if (ret)
> 		goto exit;
> 
> 	*temp = (s16)be16_to_cpup(raw);
> 	if (*temp == INV_ICM42600_DATA_INVALID)
> 		ret = -EINVAL;
> 

to here as a utility function.  Then can use direct returns in that 
function and there are no gotos to worry about.

> exit:
> 	mutex_unlock(&st->lock);
> 	pm_runtime_mark_last_busy(dev);
> 	pm_runtime_put_autosuspend(dev);
> 
> 	return ret;
> }
> 
> 
> If I use guard_scoped(..) it creates a lot of diff lines...
The only way to use that here and avoid the issue would be with breaks
which is ugly.   If we are going to pay the cost of churn, then
factoring out the guts of the function is much cleaner.

May not be worth bothering though!

Jonathan

> 
> /Sean
>