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

[Linmao Li <lilinmao@kylinos.cn>]

在 2026/8/16 4:58, Jonathan Cameron 写道:
> On Tue, 11 Aug 2026 18:33:01 +0800
> Linmao Li <lilinmao@kylinos.cn> 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@kylinos.cn>
> 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);
>>   }