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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2g4hn5nulrstqdo4rjd2uyniyuoupbvwbuf34ng47wz6d5hih3@67mtm2t5yuhu>
Date: Mon, 11 Aug 2025 11:47:23 +0000
From: Sean Nyekjaer <sean@...nix.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>, Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, Jean-Baptiste Maneyrol <jmaneyrol@...ensense.com>, Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH v2 5/5] iio: imu: inv_icm42600: use guard() to release mutexes

On Fri, Aug 08, 2025 at 11:52:35PM +0100, Andy Shevchenko wrote:
> On Fri, Aug 8, 2025 at 5:58 PM Sean Nyekjaer <sean@...nix.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.

Oh, good catch :)

> 
> ...
> 
> >         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

Thanks for the review.

/Sean