Re: [PATCH togreg v3 2/2] iio: imu: inv_icm42607: restore runtime PM on system resume errors

[Linmao Li]

在 2026/8/16 4:58, Jonathan Cameron 写道:
> On Tue, 11 Aug 2026 18:33:01 +0800
> Linmao Li <lilinmao@xxxxxxxxxx> wrote:
>
> > pm_runtime_force_suspend() leaves runtime PM disabled after it succeeds and
> > expects pm_runtime_force_resume() to restore runtime PM management during
> > system resume.
> >
> > The resume callback returns early if enabling the vddio regulator or
> > synchronizing the register cache fails, skipping the matching
> > pm_runtime_force_resume() call. Runtime PM consequently remains disabled
> > after the system has resumed, so runtime autosuspend can no longer turn off
> > sensors enabled afterward.
> >
> > Call pm_runtime_force_resume() on both error paths. Keep the first error as
> > the return value and report a runtime PM restore failure separately.
> >
> > Fixes: 3007c1530f96 ("iio: imu: inv_icm42607: Add PM support for icm42607")
> > Signed-off-by: Linmao Li <lilinmao@xxxxxxxxxx>
> Sashiko has some comments on this:
> https://sashiko.dev/#/patchset/20260811103301.1157404-1-lilinmao%40kylinos.cn
>
> I would note that in some paths error handling is best effort.
> There isn't always a sequence that leaves us in a remotely
> useful state.  So maybe what you have here is the best we can do
> even though it is a bit crazy to expect the driver to do anything
> useful if it can't power the device.
>
> > ---
> > Changes since v2:
> > - Restructure inv_icm42607_resume() along the lines Andy suggested:
> >    handle the error case in its own block and call
> >    pm_runtime_force_resume() directly on the success path.  No
> >    functional change.
> >
> > Changes since v1:
> > - Split the device side of inv_icm42607_resume() into a helper so the
> >    PM bookkeeping stays in the wrapper.  No functional change.
> >
> >   .../iio/imu/inv_icm42607/inv_icm42607_core.c  | 23 +++++++++++++++----
> >   1 file changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > index 0da362967f63b..f4ef75da22c76 100644
> > --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> > @@ -664,9 +664,8 @@ static int inv_icm42607_suspend(struct device *dev)
> >   	return 0;
> >   }
> >   
> > -static int inv_icm42607_resume(struct device *dev)
> > +static int inv_icm42607_resume_core(struct inv_icm42607_state *st)
> >   {
> > -	struct inv_icm42607_state *st = dev_get_drvdata(dev);
> >   	int ret;
> >   
> >   	ret = inv_icm42607_enable_vddio_reg(st);
> > @@ -675,9 +674,25 @@ static int inv_icm42607_resume(struct device *dev)
> >   
> >   	/* Sync the regcache again after regulator shutdown. */
> >   	regcache_mark_dirty(st->map);
> > -	ret = regcache_sync(st->map);
> > -	if (ret)
> > +
> > +	return regcache_sync(st->map);
> > +}
> > +
> > +static int inv_icm42607_resume(struct device *dev)
> > +{
> > +	struct inv_icm42607_state *st = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = inv_icm42607_resume_core(st);
> > +	if (ret) {
> > +		int rc;
> > +
> > +		rc = pm_runtime_force_resume(dev);
> > +		if (rc)
> > +			dev_warn(dev, "Failed to restore runtime PM state: %d\n", rc);
> > +
> There is a question from sashiko on whether this can be reached.
> Even though that may be the case I'd keep the the error print because
> it hardens us against future changes.
Agreed.  I would keep the warning, and I think it is reachable. When
pm_runtime_force_resume() needs to invoke a runtime-resume callback,
GET_CALLBACK() can select one from the PM domain, device type, class or
bus before falling back to the driver.  The NULL runtime_resume in this
driver's PM ops therefore does not mean that the entire callback chain is
empty.  For example, a generic PM domain runtime-resume callback can
fail.

On sashiko's other point, leaving runtime PM disabled would not by itself
block I/O with -EACCES here.  Provided there is no pre-existing
runtime_error, the read paths use PM_RUNTIME_ACQUIRE_AUTOSUSPEND(), which
calls pm_runtime_get_active() with RPM_TRANSPARENT.  The acquisition
therefore succeeds when runtime PM is disabled.

Calling pm_runtime_force_resume() on the error path consequently does not
newly expose accesses after a failed hardware resume; that possibility
already exists without the patch.  Depending on the transport and actual
hardware state, such accesses may either fail or return unusable data.
> >   		return ret;
> > +	}
> >   
> >   	return pm_runtime_force_resume(dev);
> >   }