Re: [PATCH V10 5/9] iio: imu: inv_icm42607: Add PM support for icm42607

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing `dev_set_drvdata()` causes a guaranteed kernel panic on PM transitions.
- [Medium] Unnecessary start-up delays applied when disabling sensors during power management transitions.
--

commit 095604e6b5564d3e2f59fa7bf159761bbf791c56
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 4448d63e95fd7..39620da92bd43 100644
> --- a/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
> +++ b/drivers/iio/imu/inv_icm42607/inv_icm42607_core.c
[ ... ]
> @@ -72,6 +73,51 @@ const struct inv_icm42607_hw inv_icm42607p_hw_data = {
>  };
>  EXPORT_SYMBOL_NS_GPL(inv_icm42607p_hw_data, "IIO_ICM42607");
>  
> +static int inv_icm42607_set_pwr_mgmt0(struct inv_icm42607_state *st,
> +				      enum inv_icm42607_sensor_mode gyro,
> +				      enum inv_icm42607_sensor_mode accel,
> +				      bool temp, unsigned int *sleep_ms)
> +{
[ ... ]
> +	sleepval = 0;
> +	if (temp && !oldtemp)
> +		sleepval = max(sleepval, INV_ICM42607_TEMP_STARTUP_TIME_MS);
> +
> +	if (accel != oldaccel)
> +		sleepval = max(sleepval, INV_ICM42607_ACCEL_STARTUP_TIME_MS);
> +
> +	if (gyro != oldgyro)
> +		sleepval = max(sleepval, INV_ICM42607_GYRO_STARTUP_TIME_MS);

[Severity: Medium]
Does this unnecessarily delay suspend?

When inv_icm42607_runtime_suspend() turns the sensors off by passing
INV_ICM42607_SENSOR_MODE_OFF, the state changes (accel != oldaccel and
gyro != oldgyro).

Because of this, it seems the function will wait for the maximum startup
time before suspending, rather than only sleeping when the sensors are
actually turning on.

> +
> +	if (sleep_ms)
> +		*sleep_ms = sleepval;
> +	else if (sleepval)
> +		fsleep(sleepval * 1000);
> +
> +	return 0;
> +}
[ ... ]
> +static int inv_icm42607_runtime_suspend(struct device *dev)
> +{
> +	struct inv_icm42607_state *st = dev_get_drvdata(dev);
> +
> +	guard(mutex)(&st->lock);

[Severity: Critical]
Will this cause a NULL pointer dereference?

The PM callbacks retrieve the driver state using dev_get_drvdata(), but it
doesn't appear that dev_set_drvdata() is ever called during probe to
associate the state with the device.

Since pm_runtime_use_autosuspend() is enabled with a 2000ms delay in probe,
won't this reliably crash the kernel exactly two seconds after the device is
probed?

> +
> +	/* save sensors state */
> +	st->suspended.gyro = st->conf.gyro.mode;
> +	st->suspended.accel = st->conf.accel.mode;
> +	st->suspended.temp = st->conf.temp_en;
> +
> +	return inv_icm42607_set_pwr_mgmt0(st, INV_ICM42607_SENSOR_MODE_OFF,
> +					  INV_ICM42607_SENSOR_MODE_OFF, false,
> +					  NULL);
> +}

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