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

[Linmao Li]

在 2026/8/11 16:54, Andy Shevchenko 写道:
> On Tue, Aug 11, 2026 at 10:03:45AM +0800, Linmao Li 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.
> ...
>
> >   	/* Sync the regcache again after regulator shutdown. */
> >   	regcache_mark_dirty(st->map);
> > -	ret = regcache_sync(st->map);
> > -	if (ret)
> > +
> I wouldn't add this blank line as these two are quite coupled. OTOH it's a
> better style, so I leave it to Jonathan and others to decide.
>
> > +	return regcache_sync(st->map);
> > +}
> ...
>
> > +static int inv_icm42607_resume(struct device *dev)
> > +{
> > +	struct inv_icm42607_state *st = dev_get_drvdata(dev);
> > +	int resume_ret;
> > +	int ret;
> > +
> > +	ret = inv_icm42607_resume_core(st);
> > +
> > +	resume_ret = pm_runtime_force_resume(dev);
> > +	if (ret) {
> I still don't get the logic here. Shouldn't we rather call the force_suspend()
> last in the .suspend() and force_resume() first here?

pm_runtime_force_suspend() may invoke the .runtime_suspend callback,
which writes PWR_MGMT0 over the bus. It therefore has to run while
vddio is still enabled, before inv_icm42607_disable_vddio_reg().

On resume, vddio and the register cache need to be restored before
pm_runtime_force_resume() re-enables runtime PM. Otherwise runtime PM
could be enabled while the device-side resume has not completed.

This is the ordering already used by the driver; the series does not
change it.

> > +		if (resume_ret)
> > +			dev_warn(dev, "Failed to restore runtime PM state: %d\n",
> > +				 resume_ret);
> > +
> >   		return ret;
> > +	}
> >   
> > -	return pm_runtime_force_resume(dev);
> > +	return resume_ret;
> >   }
> Okay, after reading other drivers I think the above can be written in a bit better form.
Thanks, I have used this form in v3.
> 	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);
>
> // and yes, I would go with longer line here (that's why I renamed variable)
>
> 		return ret;
> 	}
>
> 	return pm_runtime_force_resume(dev);
>
> It seems that if the above analysis is true (I haven't deeply checked that),
> there are many drivers in the kernel suffer from the very same issue (when they
> need to do some work before runtime PM resume may be called from inside the
> system .resume() callback.