[PATCH 13/15] iio: health: max30100: do not use internal iio_dev lock
Jonathan Cameron
jic23 at kernel.org
Sat Sep 24 08:53:18 PDT 2022
On Tue, 20 Sep 2022 17:10:33 +0200
Miquel Raynal <miquel.raynal at bootlin.com> wrote:
> Hi Nuno,
>
> noname.nuno at gmail.com wrote on Tue, 20 Sep 2022 16:56:01 +0200:
>
> > On Tue, 2022-09-20 at 15:53 +0200, Miquel Raynal wrote:
> > > Hi Nuno,
> > >
> > > Nuno.Sa at analog.com wrote on Tue, 20 Sep 2022 13:15:32 +0000:
> > >
> > > > > -----Original Message-----
> > > > > From: Miquel Raynal <miquel.raynal at bootlin.com>
> > > > > Sent: Tuesday, September 20, 2022 2:56 PM
> > > > > To: Sa, Nuno <Nuno.Sa at analog.com>
> > > > > Cc: linux-arm-kernel at lists.infradead.org;
> > > > > linux-rockchip at lists.infradead.org;
> > > > > linux-amlogic at lists.infradead.org; linux-imx at nxp.com; linux-
> > > > > iio at vger.kernel.org; Chunyan Zhang <zhang.lyra at gmail.com>;
> > > > > Hennerich,
> > > > > Michael <Michael.Hennerich at analog.com>; Martin Blumenstingl
> > > > > <martin.blumenstingl at googlemail.com>; Sascha Hauer
> > > > > <s.hauer at pengutronix.de>; Cixi Geng <cixi.geng1 at unisoc.com>;
> > > > > Kevin
> > > > > Hilman <khilman at baylibre.com>; Vladimir Zapolskiy <vz at mleia.com>;
> > > > > Pengutronix Kernel Team <kernel at pengutronix.de>; Alexandru
> > > > > Ardelean
> > > > > <aardelean at deviqon.com>; Fabio Estevam <festevam at gmail.com>;
> > > > > Andriy
> > > > > Tryshnivskyy <andriy.tryshnivskyy at opensynergy.com>; Haibo Chen
> > > > > <haibo.chen at nxp.com>; Shawn Guo <shawnguo at kernel.org>; Hans de
> > > > > Goede <hdegoede at redhat.com>; Jerome Brunet
> > > > > <jbrunet at baylibre.com>;
> > > > > Heiko Stuebner <heiko at sntech.de>; Florian Boor
> > > > > <florian.boor at kernelconcepts.de>; Regus, Ciprian
> > > > > <Ciprian.Regus at analog.com>; Lars-Peter Clausen <lars at metafoo.de>;
> > > > > Andy
> > > > > Shevchenko <andy.shevchenko at gmail.com>; Jonathan Cameron
> > > > > <jic23 at kernel.org>; Neil Armstrong <narmstrong at baylibre.com>;
> > > > > Baolin
> > > > > Wang <baolin.wang at linux.alibaba.com>; Jyoti Bhayana
> > > > > <jbhayana at google.com>; Chen-Yu Tsai <wens at csie.org>; Orson Zhai
> > > > > <orsonzhai at gmail.com>
> > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use
> > > > > internal iio_dev
> > > > > lock
> > > > >
> > > > > [External]
> > > > >
> > > > > Hi Nuno,
> > > > >
> > > > > Nuno.Sa at analog.com wrote on Tue, 20 Sep 2022 12:44:08 +0000:
> > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Miquel Raynal <miquel.raynal at bootlin.com>
> > > > > > > Sent: Tuesday, September 20, 2022 2:23 PM
> > > > > > > To: Sa, Nuno <Nuno.Sa at analog.com>
> > > > > > > Cc: linux-arm-kernel at lists.infradead.org; linux-
> > > > > rockchip at lists.infradead.org;
> > > > > > > linux-amlogic at lists.infradead.org; linux-imx at nxp.com; linux-
> > > > > > > iio at vger.kernel.org; Chunyan Zhang <zhang.lyra at gmail.com>;
> > > > > Hennerich,
> > > > > > > Michael <Michael.Hennerich at analog.com>; Martin Blumenstingl
> > > > > > > <martin.blumenstingl at googlemail.com>; Sascha Hauer
> > > > > > > <s.hauer at pengutronix.de>; Cixi Geng <cixi.geng1 at unisoc.com>;
> > > > > > > Kevin
> > > > > > > Hilman <khilman at baylibre.com>; Vladimir Zapolskiy
> > > > > > > <vz at mleia.com>;
> > > > > > > Pengutronix Kernel Team <kernel at pengutronix.de>; Alexandru
> > > > > Ardelean
> > > > > > > <aardelean at deviqon.com>; Fabio Estevam <festevam at gmail.com>;
> > > > > Andriy
> > > > > > > Tryshnivskyy <andriy.tryshnivskyy at opensynergy.com>; Haibo
> > > > > > > Chen
> > > > > > > <haibo.chen at nxp.com>; Shawn Guo <shawnguo at kernel.org>; Hans
> > > > > > > de
> > > > > > > Goede <hdegoede at redhat.com>; Jerome Brunet
> > > > > <jbrunet at baylibre.com>;
> > > > > > > Heiko Stuebner <heiko at sntech.de>; Florian Boor
> > > > > > > <florian.boor at kernelconcepts.de>; Regus, Ciprian
> > > > > > > <Ciprian.Regus at analog.com>; Lars-Peter Clausen
> > > > > > > <lars at metafoo.de>;
> > > > > Andy
> > > > > > > Shevchenko <andy.shevchenko at gmail.com>; Jonathan Cameron
> > > > > > > <jic23 at kernel.org>; Neil Armstrong <narmstrong at baylibre.com>;
> > > > > > > Baolin
> > > > > > > Wang <baolin.wang at linux.alibaba.com>; Jyoti Bhayana
> > > > > > > <jbhayana at google.com>; Chen-Yu Tsai <wens at csie.org>; Orson
> > > > > > > Zhai
> > > > > > > <orsonzhai at gmail.com>
> > > > > > > Subject: Re: [PATCH 13/15] iio: health: max30100: do not use
> > > > > > > internal
> > > > > iio_dev
> > > > > > > lock
> > > > > > >
> > > > > > > [External]
> > > > > > >
> > > > > > > Hi Nuno,
> > > > > > >
> > > > > >
> > > > > > Hi Miquel,
> > > > > >
> > > > > > Thanks for reviewing...
> > > > > >
> > > > > > > nuno.sa at analog.com wrote on Tue, 20 Sep 2022 13:28:19 +0200:
> > > > > > >
> > > > > > > > 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
> > > > > > > > allow it. Hence to get rid of the 'mlock' we need to:
> > > > > > > >
> > > > > > > > 1. Use iio_device_claim_direct_mode() to check if direct
> > > > > > > > mode can be
> > > > > > > > claimed and if we can return -EINVAL (as the original
> > > > > > > > code);
> > > > > > > >
> > > > > > > > 2. Make sure that buffering is not disabled while doing a
> > > > > > > > raw read. For
> > > > > > > > that, we can make use of the local lock that already
> > > > > > > > exists.
> > > > > > > >
> > > > > > > > While at it, fixed a minor coding style complain...
> > > > > > > >
> > > > > > > > Signed-off-by: Nuno Sá <nuno.sa at analog.com>
> > > > > > > > ---
> > > > > > > > drivers/iio/health/max30100.c | 24 +++++++++++++++++------
> > > > > > > > -
> > > > > > > > 1 file changed, 17 insertions(+), 7 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/iio/health/max30100.c
> > > > > b/drivers/iio/health/max30100.c
> > > > > > > > index ad5717965223..aa494cad5df0 100644
> > > > > > > > --- a/drivers/iio/health/max30100.c
> > > > > > > > +++ b/drivers/iio/health/max30100.c
> > > > > > > > @@ -185,8 +185,19 @@ static int
> > > > > > > > max30100_buffer_postenable(struct
> > > > > > > iio_dev *indio_dev)
> > > > > > > > static int max30100_buffer_predisable(struct iio_dev
> > > > > > > > *indio_dev)
> > > > > > > > {
> > > > > > > > struct max30100_data *data = iio_priv(indio_dev);
> > > > > > > > + int ret;
> > > > > > > > +
> > > > > > > > + /*
> > > > > > > > + * As stated in the comment in the read_raw()
> > > > > > > > function, temperature
> > > > > > > > + * can only be acquired if the engine is running.
> > > > > > > > As such the mutex
> > > > > > > > + * is used to make sure we do not power down while
> > > > > > > > doing a
> > > > > > > temperature
> > > > > > > > + * reading.
> > > > > > > > + */
> > > > > > > > + mutex_lock(&data->lock);
> > > > > > > > + ret = max30100_set_powermode(data, false);
> > > > > > > > + mutex_unlock(&data->lock);
> > > > > > > >
> > > > > > > > - return max30100_set_powermode(data, false);
> > > > > > > > + return ret;
> > > > > > > > }
> > > > > > > >
> > > > > > > > static const struct iio_buffer_setup_ops
> > > > > > > > max30100_buffer_setup_ops
> > > > > = {
> > > > > > > > @@ -387,18 +398,17 @@ static int max30100_read_raw(struct
> > > > > > > > iio_dev
> > > > > > > *indio_dev,
> > > > > > > > * Temperature reading can only be acquired
> > > > > > > > while engine
> > > > > > > > * is running
> > > > > > > > */
> > > > > > > > - mutex_lock(&indio_dev->mlock);
> > > > > > > > -
> > > > > > > > - if (!iio_buffer_enabled(indio_dev))
> > > > > > > > + if
> > > > > > > > (!iio_device_claim_direct_mode(indio_dev)) {
> > > > > > >
> > > > > > > I wonder if this line change here is really needed. I agree
> > > > > > > the whole
> > > > > > > construction looks like what iio_device_claim_direct_mode()
> > > > > > > does but in
> > > > > > > practice I don't see the point of acquiring any lock here if
> > > > > > > we just
> > > > > > > release it no matter what happens right after.
> > > > > > >
> > > > > >
> > > > > > I can see that this is odd (at the very least) but AFAIK, this
> > > > > > is the only way
> > > > > > to safely infer if buffering is enabled or not.
> > > > > > iio_buffer_enabled() has no
> > > > > > protection against someone concurrently enabling/disabling the
> > > > > > buffer.
> > > > >
> > > > > Yes, but this is only relevant if you want to infer that the
> > > > > "buffers
> > > > > are enabled" and be sure that it cannot be otherwise during the
> > > > > next
> > > > > lines until you release the lock. Acquiring a lock, doing the if
> > > > > and
> > > > > then unconditionally releasing the lock, IMHO, does not make any
> > > > > sense
> > > > > (but I'm not a locking guru) because when you enter the else
> > > > > clause,
> > > > > you are not protected anyway, so in both cases all this is
> > > > > completely
> > > > > racy.
> > > > >
> > > >
> > > > Ahh crap, yes you are right... It is still racy since we can still
> > > > try to read
> > > > the temperature with the device powered off. I'm not really sure
> > > > how to
> > > > address this. One way could be to just use an internal control
> > > > variable
> > > > to reflect the device power state (set/clear on the buffer
> > > > callbacks) and
> > > > only use the local lock (completely ditching the call to
> > > > iio_device_claim_direct_mode())...
> > >
> > > I tend to prefer this option than the one below.
> > >
> > > I guess your implementation already prevents buffer_predisable() to
> > > run
> > > thanks to the local lock being held during the operation. Maybe we
> > > should just verify that buffers are enabled from within the local
> > > lock
> > > being held instead of just acquiring it for the get_temp() measure.
> > > It
> > > would probably solve the situation here.
> > > >
> > Not sure if I understood... You mean something like:
> >
> > mutex_lock(&data->lock);
> > if (!iio_buffer_enabled(indio_dev)) {
> > ret = -EINVAL;
> > } else {
> > ret = max30100_get_temp(data, val);
> > if (!ret)
> > ret = IIO_VAL_INT;
> >
> > }
> > mutex_unlock(&data->lock);
> >
> > If so, I think this is still racy since we release the lock after the
> > predisable which means we could still detect the buffers as enabled (in
> > the above block) and try to get_temp on a powered down device.
>
> True.
>
> >
> > Since we pretty much only care about the power state of the device (and
> > we are using the buffering state to infer that AFAIU), I was thinking
> > in something like:
> >
> >
> > mutex_lock(&data->lock);
> > if (!data->powered) {
> > ret = -EINVAL;
> > } else {
> > ret = max30100_get_temp(data, val);
> > if (!ret)
> > ret = IIO_VAL_INT;
> >
> > }
> > mutex_unlock(&data->lock);
>
> LGTM.
A reference counted power up flag would probably work as we'd want to disable
power only when the reference count goes to 0. Note all checks of that flag
would need to be done under the lock as well.
As an alternative...
Whilst it is a serious oddity, how about flipping the logic and having
an iio_device_claim_buffered_mode() that takes mlock and holds it only
if we are in buffered mode - then holds it until matching release?
Now, I've only done a superficial audit of the buffer removal paths
to check they hold the lock before we call predisable() but it looks
like they do - so this should work.
Just wanted to muddy the waters :)
>
> >
> > Then, in the predisable, something like I have but setting the flag to
> > false and the opposite on the postenable... Naturally we could also
> > just read the registers (and I actually tend to prefer it) instead of a
> > new flag but I guess the flag is enough in this case.
> >
> > - Nuno Sá
> > >
>
>
> Thanks,
> Miquèl
More information about the Linux-rockchip
mailing list