Re: [PATCH togreg v3 1/2] iio: imu: inv_icm42607: propagate runtime suspend errors

[Linmao Li]

在 2026/8/24 8:20, Jonathan Cameron 写道:
> On Mon, 17 Aug 2026 17:32:43 +0800
> Linmao Li <lilinmao@xxxxxxxxxx> wrote:
>
> > 在 2026/8/16 4:52, Jonathan Cameron 写道:
> > > On Tue, 11 Aug 2026 18:33:00 +0800
> > > Linmao Li <lilinmao@xxxxxxxxxx> wrote:
> > >   
> > > > The runtime suspend callback always returns success even when updating
> > > > PWR_MGMT0 fails. The PM core can then mark the device suspended while one
> > > > or both sensors remain enabled.
> > > >
> > > > The sibling ICM-42600 driver propagates the corresponding
> > > > inv_icm42600_set_pwr_mgmt0() failure from its runtime suspend callback.
> > > > Make ICM-42607 follow the same behavior by returning the sensor shutdown
> > > > error. Keep a void wrapper for the managed teardown action, where errors
> > > > can only be logged.
> > > What is the practical affect of a sensor remaining enabled?  Bit of
> > > power loss or something more significant?  This info matter when deciding
> > > if we should rush this in during the rc phase, or wait for the next
> > > merge window.
> > As far as I can tell, the practical effect is additional power
> > consumption while the device is idle.  I found no corruption path, but I
> > have no ICM-42607 hardware to reproduce the failure or measure the
> > current.
> >
> > It does not necessarily persist indefinitely.  If the PWR_MGMT0 write
> > fails, regmap may contain the requested OFF state while the hardware
> > remains ON.  A later sensor read requests an enabled mode, so
> > inv_icm42607_set_pwr_mgmt0() retries the write instead of taking its
> > "no change" return.  If the bus error was transient, the cache and
> > hardware are then resynchronized.
> Please capture some of that for the commit description for v2.
I need to correct one detail there.  For the I2C and SPI regmap paths
this driver uses, defer_caching is enabled, and _regmap_raw_write_impl()
drops the affected cache entry when the bus write fails.  So the failure
does not normally leave a cached OFF value.

The conclusion is unchanged, but the mechanism is different: a later
access misses in the cache, so regmap_read() reads PWR_MGMT0 from the
hardware again.  If the bus error was transient, the driver then sees
the actual state and can restore the requested mode if necessary.

I will use the corrected explanation in the next revision.  Sorry for
the confusion.


Thanks,
Linmao
> > I also noticed a cost to this patch: returning an error such as -EIO from
> > .runtime_suspend() makes the PM core set power.runtime_error. Subsequent
> > reads then fail in PM_RUNTIME_ACQUIRE_AUTOSUSPEND() before any sensor
> > register is accessed, until the PM status is reset.  A successful system
> > suspend may clear that state.
> >
> > The patch therefore trades a logged idle power leak that may recover on
> > a later access for a potentially sticky runtime-PM failure.  Since this
> > was found by inspection only, I would not rush it into the rc phase.  I
> > would rather revisit the error handling and target the next merge window.
> That is fair enough.
>
> J
> > Linmao
> >
> > > Jonathan
> > >
> > >   
> > > > Fixes: 3007c1530f96 ("iio: imu: inv_icm42607: Add PM support for icm42607")
> > > > Signed-off-by: Linmao Li <lilinmao@xxxxxxxxxx>
> > > > ---
> > > > Unchanged since v1.
> > > >
> > > >    drivers/iio/imu/inv_icm42607/inv_icm42607_core.c | 15 ++++++++++-----
> > > >    1 file changed, 10 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > > > index 190e998f7b8ef..0da362967f63b 100644
> > > > --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > > > +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > > > @@ -537,9 +537,8 @@ static int inv_icm42607_enable_vddio_reg(struct inv_icm42607_state *st)
> > > >    	return 0;
> > > >    }
> > > >    
> > > > -static void inv_icm42607_sensors_off(void *_data)
> > > > +static int inv_icm42607_sensors_off(struct inv_icm42607_state *st)
> > > >    {
> > > > -	struct inv_icm42607_state *st = _data;
> > > >    	const struct device *dev = regmap_get_device(st->map);
> > > >    	int ret;
> > > >    
> > > > @@ -552,6 +551,13 @@ static void inv_icm42607_sensors_off(void *_data)
> > > >    					 st->conf.accel.mode);
> > > >    	if (ret)
> > > >    		dev_err(dev, "Unable to turn off sensors\n");
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static void inv_icm42607_sensors_off_action(void *data)
> > > > +{
> > > > +	inv_icm42607_sensors_off(data);
> > > >    }
> > > >    
> > > >    static void inv_icm42607_disable_vddio_reg(void *_data)
> > > > @@ -619,7 +625,7 @@ int inv_icm42607_core_probe(struct regmap *regmap,
> > > >    	 * Ensure if sensors get turned on at some point, they're turned off
> > > >    	 * as part of teardown.
> > > >    	 */
> > > > -	ret = devm_add_action_or_reset(dev, inv_icm42607_sensors_off, st);
> > > > +	ret = devm_add_action_or_reset(dev, inv_icm42607_sensors_off_action, st);
> > > >    	if (ret)
> > > >    		return ret;
> > > >    
> > > > @@ -688,8 +694,7 @@ static int inv_icm42607_runtime_suspend(struct device *dev)
> > > >    	 * however the tradeoff is that an unused sensor won't be
> > > >    	 * turned off until the entire chip is no longer in use.
> > > >    	 */
> > > > -	inv_icm42607_sensors_off(st);
> > > > -	return 0;
> > > > +	return inv_icm42607_sensors_off(st);
> > > >    }
> > > >    
> > > >    EXPORT_NS_GPL_DEV_PM_OPS(inv_icm42607_pm_ops, IIO_ICM42607) = {