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

在 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.
>