Re: [PATCH 14/15] iio: health: max30102: do not use internal iio_dev lock

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

On Sat, 2022-09-24 at 16:54 +0100, Jonathan Cameron wrote:
> On Tue, 20 Sep 2022 13:28:20 +0200
> Nuno Sá <nuno.sa@analog.com> wrote:
> 
> > The pattern used in this device does not quite fit in the
> > iio_device_claim_direct_mode() typical usage. In this case,
> > iio_buffer_enabled() was being used not to prevent the raw access
> > but to decide whether or not the device needs to be powered on
> > before.
> > If buffering, then the device is already on. To guarantee the same
> > behavior, a combination of locks is being used:
> > 
> > 1. Use iio_device_claim_direct_mode() to check if direct mode can
> > be
> > claimed and if we can, then we keep it until the reading is done
> > (which
> > also means the device will be powered on and off);
> > 2. If already buffering, we need to make sure that buffering is not
> > disabled (and hence, powering off the device) while doing a raw
> > read. For
> > that, we can make use of the local lock that already exists.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> Obviously same dance in here as the related previous patch. So same
> solution
> needs adopting.  I just thought I'd reply to make sure we didn't
> forget to
> cover them both :)
> 
> 
Hi Jonathan,

So I was working on v2 in the morning and went with your
iio_device_claim_buffer_mode() approach... And bah, well it works like
a charm in the previous patch, it fails in this one:

-               mutex_lock(&indio_dev->mlock);
-               if (!iio_buffer_enabled(indio_dev))
+               if (iio_device_claim_buffer_mode(indio_dev)) {
                        ret = max30102_get_temp(data, val, true);
-               else
+               } else {
                        ret = max30102_get_temp(data, val, false);
-               mutex_unlock(&indio_dev->mlock);
-               if (ret)
+                       iio_device_release_buffer_mode(indio_dev);
+               }
+               if(ret)
                        return ret;


Note that if we are not in buffered mode we won't get mlock and call
max30102_get_temp(data, val, true) without any lock. While it's very
unlikely for someone, in the meantime, to enable the buffer and then
disable it, it's still racy and possible (at least in theory).

So, I'm thinking again on the flag approach... Just check my comment
(in the previous patch) about it being refcounted. I mean, I might be
missing something, but if we need a refcount, I would say things would
be already (potentially) broken, right?

With this step back, I'm planning on a v2 in the beginning of the week
after we have this sorted out (and I guess we need to settle things
also in the ad799x patch)

- Nuno Sá


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