Message-ID: <CANZih_Tm2w5C58tg36LzEMoLrqWgSGaJZQ_nxoDVUXFMJnZBXQ@mail.gmail.com>
Date: Sat, 5 Jul 2025 00:37:45 -0300
From: Andrew Ijano <andrew.ijano@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: andrew.lopes@...mni.usp.br, gustavobastos@....br, dlechner@...libre.com, 
	nuno.sa@...log.com, andy@...nel.org, jstephan@...libre.com, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 3/4] iio: accel: sca3000: use lock guards

On Sat, Jun 21, 2025 at 2:55 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> Please add extra description for where you have pushed locks up
> a level in the call tree and why that is fine to do.
>
Ok! I'll do that.

> >       switch (mask) {
> > -     case IIO_CHAN_INFO_SAMP_FREQ:
> > +     case IIO_CHAN_INFO_SAMP_FREQ: {
> > +             guard(mutex)(&st->lock);
> As below
>
> >               if (val2)
> >                       return -EINVAL;
> > -             mutex_lock(&st->lock);
> > -             ret = sca3000_write_raw_samp_freq(st, val);
> > -             mutex_unlock(&st->lock);
> > -             return ret;
> > -     case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> > +             return sca3000_write_raw_samp_freq(st, val);
> > +     }
> > +     case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY: {
> > +             guard(mutex)(&st->lock);
> >               if (val2)
> >                       return -EINVAL;
> > -             mutex_lock(&st->lock);
>
> You can keep the old ordering here.  It doesn't matter much but
> easier to be sure about a patch that makes no functional change.
Ok!


> > -     mutex_lock(&st->lock);
> > +     guard(mutex)(&st->lock);
>
> This is a very large increase in scope.  Use scoped_guard() here instead to avoid
> holding the lock over a whole load of code that doesn't need it.
>
> >       ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_STATUS_ADDR));
> > -     mutex_unlock(&st->lock);

That makes sense! I don't know why I didn't use scoped_guard() in this
case before.


> >       if (state) {
> >               dev_info(&indio_dev->dev, "supposedly enabling ring buffer\n");
> > -             ret = sca3000_write_reg(st,
> > +             return sca3000_write_reg(st,
> >                       SCA3000_REG_MODE_ADDR,
> >                       ret | SCA3000_REG_MODE_RING_BUF_ENABLE);
>
> This indent was already nasty so as we are touching the code good to clean it up.
> For cases like this we can be more relaxed and if it helps readability go a little
> over 80 chars (I think this will be 80 ish)

Great! I'll pay attention to that.

> >
> >
> >  static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
> > @@ -1307,22 +1259,18 @@ static int sca3000_hw_ring_postdisable(struct iio_dev *indio_dev)
> >       int ret;
> >       struct sca3000_state *st = iio_priv(indio_dev);
> >
> > +     guard(mutex)(&st->lock);
>
> See comment at the top - Making this change is fine but call it out in the patch
> description as it isn't simple change to using guards, but instead to holding
> the lock for longer.  Change is fine but point it out as it needs
> more review than the mechanical changes.

Ok!

>
> >       ret = __sca3000_hw_ring_state_set(indio_dev, 0);
> >       if (ret)
> >               return ret;
> >
> >       /* Disable the 50% full interrupt */
> > -     mutex_lock(&st->lock);
> > -
> >       ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> >       if (ret)
> > -             goto unlock;
> > -     ret = sca3000_write_reg(st,
> > +             return ret;
> > +     return sca3000_write_reg(st,
> >                               SCA3000_REG_INT_MASK_ADDR,
> >                               ret & ~SCA3000_REG_INT_MASK_RING_HALF);
>
> As below.
>
> > -unlock:
> > -     mutex_unlock(&st->lock);
> > -     return ret;
> >  }
>
> > @@ -1386,13 +1334,9 @@ static int sca3000_clean_setup(struct sca3000_state *st)
> >        */
> >       ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_MODE_ADDR));
> >       if (ret)
> > -             goto error_ret;
> > -     ret = sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> > +             return ret;
> > +     return sca3000_write_reg(st, SCA3000_REG_MODE_ADDR,
> >                               ret & SCA3000_MODE_PROT_MASK);
>
> As below.
>
> > -
> > -error_ret:
> > -     mutex_unlock(&st->lock);
> > -     return ret;
> >  }
> >
> >  static const struct iio_info sca3000_info = {
> > @@ -1470,19 +1414,16 @@ static int sca3000_stop_all_interrupts(struct sca3000_state *st)
> >  {
> >       int ret;
> >
> > -     mutex_lock(&st->lock);
> > +     guard(mutex)(&st->lock);
> >       ret = spi_w8r8(st->us, SCA3000_READ_REG(SCA3000_REG_INT_MASK_ADDR));
> >       if (ret)
> > -             goto error_ret;
> > +             return ret;
> >
> > -     ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> > +     return sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> This adds a character to this line which means that either the indent of
> the following lines was previously wrong, or it is now.
> I think you need to add a space to the following lines.
>
> Check for other similar cases.
>
> >                               ret &
> >                               ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
> >                                 SCA3000_REG_INT_MASK_RING_HALF |
> >                                 SCA3000_REG_INT_MASK_ALL_INTS));

Hm, correct me if I'm mistaken but I couldn't find this extra
character, I used the same number of tabs in both cases. Even in this
email it shows as having the same number of white spaces.

Thanks,
Andrew

Powered by blists - more mailing lists