Re: [PATCH V5 05/11] iio: imu: inv_icm42607: Add PM support for icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unbalanced regulator disable upon driver unbind due to incorrect devres ordering and PM status check.
--

commit bd7f7b2e2bfc8853252ca783dc08bb6dfb4d8df7
Author: Chris Morgan <macromorgan@hotmail.com>

iio: imu: inv_icm42607: Add PM support for icm42607

This commit adds power management support for the ICM42607 device driver.

> diff --git a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> index 8ff51711ab1fe..047de1b405507 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
[ ... ]
> @@ -192,8 +249,10 @@ static int inv_icm42607_enable_vddio_reg(struct inv_icm42607_state *st)
>  static void inv_icm42607_disable_vddio_reg(void *_data)
>  {
>  	struct inv_icm42607_state *st = _data;
> +	struct device *dev = regmap_get_device(st->map);
>  
> -	regulator_disable(st->vddio_supply);
> +	if (!pm_runtime_suspended(dev))
> +		regulator_disable(st->vddio_supply);
>  }

Can this lead to an unbalanced regulator disable upon driver unbind?

During driver unbind, devres actions execute in reverse order. The PM runtime
cleanup added by devm_pm_runtime_set_active_enabled() runs first, calling
pm_runtime_disable() which increments disable_depth, and
pm_runtime_set_suspended() which overwrites runtime_status to RPM_SUSPENDED.

Next, this devm action executes and checks pm_runtime_suspended(dev). Because
pm_runtime_suspended() requires !dev->power.disable_depth, it will
unconditionally return false here, causing the action to always call
regulator_disable().

If the device was already runtime-suspended prior to unbind, its regulator
was already disabled by inv_icm42607_runtime_suspend(), resulting in an
unbalanced double-disable that can shut off power to shared devices.

Simply changing the check to pm_runtime_status_suspended() will not work
either, because pm_runtime_set_suspended() has already forced the status to
RPM_SUSPENDED, which would cause the regulator to leak if the device was
active.

Does the driver need to ensure the device is resumed before unbind, or use a
custom remove function, to correctly balance the regulator?

[ ... ]
> @@ -251,10 +310,97 @@ int inv_icm42607_core_probe(struct regmap *regmap, const struct inv_icm42607_hw
>  	if (ret)
>  		return ret;
>  
> +	ret = devm_pm_runtime_set_active_enabled(dev);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_set_autosuspend_delay(dev, INV_ICM42607_SUSPEND_DELAY_MS);
> +	pm_runtime_use_autosuspend(dev);
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509191907.24734-1-macroalpha82@gmail.com?part=5