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@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> > ---
> > 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á
>
