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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] System suspend and resume callbacks will self-deadlock due to recursive acquisition of a non-recursive mutex via `pm_runtime_force_suspend()`.
- [High] Early returns in system sleep callbacks bypass `pm_runtime_force_resume`, leaving runtime PM permanently disabled.
- [Low] System suspend and resume callbacks redundantly duplicate runtime PM operations.
--

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

iio: imu: inv_icm42607: Add PM support for icm42607

Add 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 3dd4cc8c6b1d3..cfdce70ce25dc 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c

[ ... ]

> +static int inv_icm42607_suspend(struct device *dev)
> +{
> +	struct inv_icm42607_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +
> +	ret = pm_runtime_force_suspend(dev);

[Severity: High]
Does this code self-deadlock when system suspend is triggered?

inv_icm42607_suspend() acquires st->lock via guard(), then calls
pm_runtime_force_suspend(). If the device is active, this synchronously
invokes inv_icm42607_runtime_suspend() which attempts to acquire the
same non-recursive st->lock:

inv_icm42607_suspend()
  guard(mutex)(&st->lock);
  pm_runtime_force_suspend()
    inv_icm42607_runtime_suspend()
      guard(mutex)(&st->lock);

Does the same deadlock pattern apply to inv_icm42607_resume() and
inv_icm42607_runtime_resume()?

> +	if (ret)
> +		return ret;
> +
> +	ret = inv_icm42607_set_pwr_mgmt0(st, INV_ICM42607_SENSOR_MODE_OFF,
> +					 INV_ICM42607_SENSOR_MODE_OFF,
> +					 false, NULL);
> +	if (ret)
> +		return ret;
> +
> +	inv_icm42607_disable_vddio_reg(st);

[Severity: Low]
Are these manual hardware shutdown operations redundant?

pm_runtime_force_suspend() invokes inv_icm42607_runtime_suspend() which
already performs these exact same sensor power-down and regulator disable
operations.

> +
> +	return 0;
> +}
> +
> +static int inv_icm42607_resume(struct device *dev)
> +{
> +	struct inv_icm42607_state *st = dev_get_drvdata(dev);
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +
> +	ret = pm_runtime_force_resume(dev);

[Severity: High]
Does this early return leave runtime PM permanently disabled for the device?

If the device was active before system suspend, pm_runtime_force_suspend()
suspends it, updates its status to RPM_SUSPENDED, and calls
pm_runtime_disable(). 

During system resume, the pm_runtime_suspended() check above evaluates
to true, causing the function to return early. This bypasses the
pm_runtime_force_resume() call, so its internal call to pm_runtime_enable()
is never executed.

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