Re: [PATCH v2 5/5] iio: imu: inv_icm42600: use guard() to release mutexes

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Fri, Aug 8, 2025 at 5:58 PM Sean Nyekjaer <sean@geanix.com> wrote:
>
> Replace explicit mutex_lock() and mutex_unlock() with the guard() macro
> for cleaner and safer mutex handling.

...

>         pm_runtime_get_sync(dev);
> -       mutex_lock(&st->lock);
> +       guard(mutex)(&st->lock);
>
>         ret = inv_icm42600_set_accel_conf(st, &conf, NULL);
>
> -       mutex_unlock(&st->lock);
>         pm_runtime_mark_last_busy(dev);
>         pm_runtime_put_autosuspend(dev);

This makes PM calls under the mutex. In some cases it may lead to deadlocks.
I think you wanted to use scoped_guard() here and in similar cases.

...

>         struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
>         int ret;
>
> -       mutex_lock(&st->lock);
> +       guard(mutex)(&st->lock);
>
>         st->fifo.watermark.accel = val;
>         ret = inv_icm42600_buffer_update_watermark(st);
>
> -       mutex_unlock(&st->lock);
> -
>         return ret;

Now remove ret and use return directly.

>  }

> -       mutex_lock(&st->lock);
> +       guard(mutex)(&st->lock);
>
>         ret = inv_icm42600_buffer_hwfifo_flush(st, count);
>         if (!ret)
>                 ret = st->fifo.nb.accel;
>
> -       mutex_unlock(&st->lock);
> -
>         return ret;

In the similar way as above.

ret = _flush();
if (ret)
  return ret;

return ...nb.accel;

...

>         struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);

>         int ret;

Now unneeded, just return directly.

> -       mutex_lock(&st->lock);
> +       guard(mutex)(&st->lock);
>
>         if (readval)
>                 ret = regmap_read(st->map, reg, readval);
>         else
>                 ret = regmap_write(st->map, reg, writeval);
>
> -       mutex_unlock(&st->lock);
> -
>         return ret;

...

>         int ret = 0;

Now unneeded assignment.

> -       mutex_lock(&st->lock);
> +       guard(mutex)(&st->lock);
>
>         st->suspended.gyro = st->conf.gyro.mode;
>         st->suspended.accel = st->conf.accel.mode;
>         st->suspended.temp = st->conf.temp_en;
> -       if (pm_runtime_suspended(dev))
> -               goto out_unlock;
> +       ret = pm_runtime_suspended(dev);
> +       if (ret)
> +               return ret;

...

>         /* disable vddio regulator if chip is sleeping */
>         if (!wakeup)
>                 regulator_disable(st->vddio_supply);
>
> -out_unlock:
> -       mutex_unlock(&st->lock);
>         return ret;

Now return 0 to make it clear that this is a success.

...

> @@ -881,10 +878,11 @@ static int inv_icm42600_resume(struct device *dev)
>         bool wakeup;
>         int ret = 0;

Assignment is useless now.

> -       mutex_lock(&st->lock);
> +       guard(mutex)(&st->lock);
>
> -       if (pm_runtime_suspended(dev))
> -               goto out_unlock;
> +       ret = pm_runtime_suspended(dev);
> +       if (ret)
> +               return ret;

...

> -out_unlock:
> -       mutex_unlock(&st->lock);
>         return ret;

  return 0;

?

...

>         regulator_disable(st->vddio_supply);
>
> -error_unlock:
> -       mutex_unlock(&st->lock);
>         return ret;

Ditto.

>  }

...

>         int ret;

Now useless variable.

>
> -       mutex_lock(&st->lock);
> +       guard(mutex)(&st->lock);
>
>         ret = inv_icm42600_enable_regulator_vddio(st);
>
> -       mutex_unlock(&st->lock);
>         return ret;
>  }

...

>         int ret;

Ditto.

> -       mutex_lock(&st->lock);
> +       guard(mutex)(&st->lock);
>
>         st->fifo.watermark.gyro = val;
>         ret = inv_icm42600_buffer_update_watermark(st);
>
> -       mutex_unlock(&st->lock);
> -
>         return ret;
>  }

...

> -       mutex_lock(&st->lock);
> +       guard(mutex)(&st->lock);
>
>         ret = inv_icm42600_buffer_hwfifo_flush(st, count);
>         if (!ret)
>                 ret = st->fifo.nb.gyro;

Invert conditional and return ret directly.

> -       mutex_unlock(&st->lock);
> -
>         return ret;

-- 
With Best Regards,
Andy Shevchenko