.../iio/imu/inv_icm42607/inv_icm42607_core.c | 41 ++++++++++++++----- 1 file changed, 31 insertions(+), 10 deletions(-)
The recently queued ICM-42607 PM support has two error paths that can leave the PM core's state inconsistent with the device. Patch 1 propagates sensor shutdown failures from runtime suspend, matching the behavior of the sibling ICM-42600 driver. Patch 2 ensures that system resume restores runtime PM management on both of its error paths, so that a failed resume does not leave runtime PM disabled for good. Changes since v1: - Patch 2: split the device side of inv_icm42607_resume() into a helper so the PM bookkeeping stays in the wrapper, per Andy's review. No functional change. - Rebased onto the current togreg head. - Patch 1 is unchanged. The Fixes commit is in iio.git togreg and has been included in linux-next. It has not reached mainline. Based on iio.git togreg at 350d1fb9204b. Both patches were compile-tested with W=1 and checked with smatch. No ICM-42607 hardware or fault-injection setup was available. Linmao Li (2): iio: imu: inv_icm42607: propagate runtime suspend errors iio: imu: inv_icm42607: restore runtime PM on system resume errors .../iio/imu/inv_icm42607/inv_icm42607_core.c | 41 ++++++++++++++----- 1 file changed, 31 insertions(+), 10 deletions(-) base-commit: 350d1fb9204b13c5f95e511e98b8bcb47574d425 -- 2.25.1
The recently queued ICM-42607 PM support has two error paths that can leave the PM core's state inconsistent with the device. Patch 1 propagates sensor shutdown failures from runtime suspend, matching the behavior of the sibling ICM-42600 driver. Patch 2 ensures that system resume restores runtime PM management on both of its error paths, so that a failed resume does not leave runtime PM disabled for good. Changes since v2: - Patch 2: restructure inv_icm42607_resume() along the lines Andy suggested. No functional change. - Dropped the redundant hand-written base note; --base already emits base-commit:. - Patch 1 is unchanged. Changes since v1: - Patch 2: split the device side of inv_icm42607_resume() into a helper so the PM bookkeeping stays in the wrapper. No functional change. - Rebased onto the current togreg head. On the ordering question from the v2 review: pm_runtime_force_suspend() runs the .runtime_suspend callback, which writes PWR_MGMT0 over the bus, so it has to happen while vddio is still enabled - that is, before inv_icm42607_disable_vddio_reg() in .suspend(). .resume() then unwinds in the opposite order, which is why pm_runtime_force_resume() comes last there. That ordering is what the driver already does; this series does not change it. The Fixes commit is in iio.git togreg and has been included in linux-next. It has not reached mainline. Both patches were compile-tested with W=1 and checked with smatch. No ICM-42607 hardware or fault-injection setup was available. Linmao Li (2): iio: imu: inv_icm42607: propagate runtime suspend errors iio: imu: inv_icm42607: restore runtime PM on system resume errors .../iio/imu/inv_icm42607/inv_icm42607_core.c | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) base-commit: 350d1fb9204b13c5f95e511e98b8bcb47574d425 -- 2.25.1
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.
Fixes: 3007c1530f96 ("iio: imu: inv_icm42607: Add PM support for icm42607")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
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) = {
--
2.25.1On Tue, 11 Aug 2026 18:33:00 +0800
Linmao Li <lilinmao@kylinos.cn> 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.
Jonathan
>
> Fixes: 3007c1530f96 ("iio: imu: inv_icm42607: Add PM support for icm42607")
> Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
> ---
> 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) = {
在 2026/8/16 4:52, Jonathan Cameron 写道:
> On Tue, 11 Aug 2026 18:33:00 +0800
> Linmao Li <lilinmao@kylinos.cn> 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.
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.
Linmao
>
> Jonathan
>
>
>> Fixes: 3007c1530f96 ("iio: imu: inv_icm42607: Add PM support for icm42607")
>> Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
>> ---
>> 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) = {
On Mon, 17 Aug 2026 17:32:43 +0800
Linmao Li <lilinmao@kylinos.cn> wrote:
> 在 2026/8/16 4:52, Jonathan Cameron 写道:
> > On Tue, 11 Aug 2026 18:33:00 +0800
> > Linmao Li <lilinmao@kylinos.cn> 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 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@kylinos.cn>
> >> ---
> >> 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) = {
在 2026/8/24 8:20, Jonathan Cameron 写道:
> On Mon, 17 Aug 2026 17:32:43 +0800
> Linmao Li <lilinmao@kylinos.cn> wrote:
>
>> 在 2026/8/16 4:52, Jonathan Cameron 写道:
>>> On Tue, 11 Aug 2026 18:33:00 +0800
>>> Linmao Li <lilinmao@kylinos.cn> 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@kylinos.cn>
>>>> ---
>>>> 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) = {
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>
---
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);
+
return ret;
+ }
return pm_runtime_force_resume(dev);
}
--
2.25.1On 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.
> return ret;
> + }
>
> return pm_runtime_force_resume(dev);
> }
在 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);
>> }
On Tue, 11 Aug 2026 10:03:43 +0800 Linmao Li <lilinmao@kylinos.cn> wrote: > The recently queued ICM-42607 PM support has two error paths that can leave > the PM core's state inconsistent with the device. For IIO at least (and I've never come across a subsystem that asks for the style you have here) don't send new versions in reply to older ones. It gets too complex if there is a lot of feedback and generally pushes your patches many screens up from the most recent in potential reviewers inboxes. So you'll probably get fewer reviews. Jonathan
On Tue, Aug 11, 2026 at 10:03:43AM +0800, Linmao Li wrote: > The recently queued ICM-42607 PM support has two error paths that can leave > the PM core's state inconsistent with the device. > > Patch 1 propagates sensor shutdown failures from runtime suspend, matching > the behavior of the sibling ICM-42600 driver. Patch 2 ensures that system > resume restores runtime PM management on both of its error paths, so that > a failed resume does not leave runtime PM disabled for good. > > Changes since v1: > - Patch 2: split the device side of inv_icm42607_resume() into a helper so > the PM bookkeeping stays in the wrapper, per Andy's review. No > functional change. > - Rebased onto the current togreg head. > - Patch 1 is unchanged. > > The Fixes commit is in iio.git togreg and has been included in > linux-next. It has not reached mainline. > Based on iio.git togreg at 350d1fb9204b. Unneeded info since you are correctly used --base and we see that below. > Both patches were compile-tested with W=1 and checked with smatch. No > ICM-42607 hardware or fault-injection setup was available. > > Linmao Li (2): > iio: imu: inv_icm42607: propagate runtime suspend errors > iio: imu: inv_icm42607: restore runtime PM on system resume errors > > .../iio/imu/inv_icm42607/inv_icm42607_core.c | 41 ++++++++++++++----- > 1 file changed, 31 insertions(+), 10 deletions(-) > > > base-commit: 350d1fb9204b13c5f95e511e98b8bcb47574d425 -- With Best Regards, Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.