Re: [PATCH 02/15] iio: adc: ad799x: do not use internal iio_dev lock

["Nuno Sá" <noname.nuno@gmail.com>]

On Sat, 2022-09-24 at 15:45 +0100, Jonathan Cameron wrote:
> On Tue, 20 Sep 2022 13:28:08 +0200
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > 'mlock' was being grabbed when setting the device frequency. In
> > order to
> > not introduce any functional change a new lock is added. With that
> > in
> > mind, the lock also needs to be grabbed in the places where 'mlock'
> > is.
> 
> The usage in here is an example of why we originally decided to take
> mlock
> private...  Annoying hard to reason about.  One key thing this
> description
> doesn't mention is protection of st->config vs device state and I
> think
> the original usage of mlock is partly intended to protect that.
> 
> Upshot is I'm not confident enough on this one to be happy taking it
> without
> more head scratching or some review from others!
> 

Yeah, this one is odd enough...

> > 
> > On the other places the lock was being used, we can just drop
> > it since we are only doing one i2c bus read/write which is already
> > safe.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> 
> > ---
> >  drivers/iio/adc/ad799x.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ad799x.c b/drivers/iio/adc/ad799x.c
> > index 262bd7665b33..838ba8e77de1 100644
> > --- a/drivers/iio/adc/ad799x.c
> > +++ b/drivers/iio/adc/ad799x.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/types.h>
> >  #include <linux/err.h>
> >  #include <linux/module.h>
> > +#include <linux/mutex.h>
> >  #include <linux/bitops.h>
> >  
> >  #include <linux/iio/iio.h>
> > @@ -125,6 +126,8 @@ struct ad799x_state {
> >         const struct ad799x_chip_config *chip_config;
> >         struct regulator                *reg;
> >         struct regulator                *vref;
> > +       /* lock to protect against multiple access to the device */
> > +       struct mutex                    lock;
> >         unsigned                        id;
> >         u16                             config;
> >  
> > @@ -290,7 +293,9 @@ static int ad799x_read_raw(struct iio_dev
> > *indio_dev,
> >                 ret = iio_device_claim_direct_mode(indio_dev);
> >                 if (ret)
> >                         return ret;
> > +               mutex_lock(&st->lock);
> 
> If we claim direct mode for the frequency writing we'll avoid racing
> with
> buffers being enabled or other sysfs accesses that are claiming
> direct mode.
> 

As you stated in some other patch, changing the frequency while
buffering is probably not a good idea (possible in some devices though)
but the main reason I haven't used the claim direct approach was
because it would change behavior and could, in theory, break some
userspace apps...

> That made me think we could drop the lock, but the argument gets
> tricker
> around st->config which is used in ad799x_scan_direct() and modified
> in write_event_config() in a fashion that means it could be out of
> sync.
> I'm not sure that matters but it is getting hard to reason about.
> 

The write_event_config() also could use some improvement... Note that
st->config is always written even if ad799x_write_config() fails (which
for some devices is possible). I know that for an i2c write to fail
that probably means we have bigger issues but that does not make it
correct :). We should only update the variable after doing the actual
configuration...

- Nuno Sá
> 

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic