Re: [PATCH] iio: chemical: scd30: Replace manual locking with RAII locking

[Maxwell Doose]

On Fri, May 15, 2026 at 9:13 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Thu, 14 May 2026 19:26:03 -0500
> Maxwell Doose <m32285159@xxxxxxxxx> wrote:
>
> > scd30_core.c currently uses manual mutex_lock() and mutex_unlock()
> > calls. Replace them with the newer guard(mutex)() for cleaner RAII
> > patterns and to improve maintainability.
> >
> > In addition, minor refactors in control logic to remove gotos where the
> > guard(mutex)() calls would be used, and replace the "?:" operator with
> > regular if/else returns.
> >
> > Signed-off-by: Maxwell Doose <m32285159@xxxxxxxxx>
> https://sashiko.dev/#/patchset/20260515002603.19240-1-m32285159%40gmail.com
>
> It caught the main issue. Given that is a fairly common bad pattern
> I might have noticed it. But then maybe not. Another win to the bot ;)

Gah, (of course its the callbacks!) will fix for v2. At least it's a
pattern that I can recognize now (I still need to take a deep deep
dive into the IIO API).

> > ---
> >  drivers/iio/chemical/scd30_core.c | 53 +++++++++++++++++--------------
> >  1 file changed, 29 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
> > index a665fcb78806..a24c95874fd3 100644
> > --- a/drivers/iio/chemical/scd30_core.c
> > +++ b/drivers/iio/chemical/scd30_core.c
>
> >
> >  static IIO_DEVICE_ATTR_RO(sampling_frequency_available, 0);
> > @@ -590,19 +598,16 @@ static irqreturn_t scd30_trigger_handler(int irq, void *p)
> >       } scan = { };
> >       int ret;
> >
> > -     mutex_lock(&state->lock);
> > +     guard(mutex)(&state->lock);
>
> I agree with sashiko here (I might have noticed) that it is in appropriate
> to extend the lock scope so far. In particular over the iio_trigger_notify_done().
> Depending on exactly what happens in there we might end up with a deadlock.
>
> For this one I'd factor out the reading code and memcpy + lock into a helper
> - you can use guard() in that safely without expanding the scope and keep
> to the goto pattern for the outer function.
>

Will get that helper in for the v2. TBH would've used scoped_guard()
but in my opinion indentation would get screwed up at that point so
I'll go with the helper.

>
> Or just leave this one alone as too complex.
>
> > +
> >       if (!iio_trigger_using_own(indio_dev))
> >               ret = scd30_read_poll(state);
> >       else
> >               ret = scd30_read_meas(state);
> >       memcpy(scan.data, state->meas, sizeof(state->meas));
> > -     mutex_unlock(&state->lock);
> > -     if (ret)
> > -             goto out;
> > -
> > -     iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan),
> > -                                 iio_get_time_ns(indio_dev));
> > -out:
> > +     if (!ret)
> Very rarely will I be happy with a change that flips from having the
> error path out of line to one where the good path is out of line.
> If you do decide to change this one via a helper, keep the goto pattern
> for this error handling.
>

Alrighty, sounds good.

best regards,
max

(ps, will be away for weekend so unfortunately I won't be able to
respond to email, as I won't have access to my computer, and my
phone's email client is crap :( )

>
> > +             iio_push_to_buffers_with_ts(indio_dev, &scan, sizeof(scan),
> > +                                         iio_get_time_ns(indio_dev));
> >       iio_trigger_notify_done(indio_dev->trig);
> >       return IRQ_HANDLED;
> >  }
>