Hi Nuno,
Nuno.Sa@xxxxxxxxxx wrote on Tue, 20 Sep 2022 12:44:08 +0000:
> > -----Original Message-----
> > From: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> > Sent: Tuesday, September 20, 2022 2:23 PM
> > To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-rockchip@xxxxxxxxxxxxxxxxxxx;
> > linux-amlogic@xxxxxxxxxxxxxxxxxxx; linux-imx@xxxxxxx; linux-
> > iio@xxxxxxxxxxxxxxx; Chunyan Zhang <zhang.lyra@xxxxxxxxx>; Hennerich,
> > Michael <Michael.Hennerich@xxxxxxxxxx>; Martin Blumenstingl
> > <martin.blumenstingl@xxxxxxxxxxxxxx>; Sascha Hauer
> > <s.hauer@xxxxxxxxxxxxxx>; Cixi Geng <cixi.geng1@xxxxxxxxxx>; Kevin
> > Hilman <khilman@xxxxxxxxxxxx>; Vladimir Zapolskiy <vz@xxxxxxxxx>;
> > Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>; Alexandru Ardelean
> > <aardelean@xxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>; Andriy
> > Tryshnivskyy <andriy.tryshnivskyy@xxxxxxxxxxxxxxx>; Haibo Chen
> > <haibo.chen@xxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Hans de
> > Goede <hdegoede@xxxxxxxxxx>; Jerome Brunet <jbrunet@xxxxxxxxxxxx>;
> > Heiko Stuebner <heiko@xxxxxxxxx>; Florian Boor
> > <florian.boor@xxxxxxxxxxxxxxxxx>; Regus, Ciprian
> > <Ciprian.Regus@xxxxxxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>; Andy
> > Shevchenko <andy.shevchenko@xxxxxxxxx>; Jonathan Cameron
> > <jic23@xxxxxxxxxx>; Neil Armstrong <narmstrong@xxxxxxxxxxxx>; Baolin
> > Wang <baolin.wang@xxxxxxxxxxxxxxxxx>; Jyoti Bhayana
> > <jbhayana@xxxxxxxxxx>; Chen-Yu Tsai <wens@xxxxxxxx>; Orson Zhai
> > <orsonzhai@xxxxxxxxx>
> > 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@xxxxxxxxxx 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@xxxxxxxxxx>
> > > ---
> > > 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.
> So the call is needed to make sure 'mlock' is internally grabbed before
> calling iio_buffer_enabled().
>
> > Unless of course if there is a hidden goal like "stop exporting
> > iio_buffer_enabled()" or something like that.
> >
> > At least I would separate this from the main change which targets the
> > removal of mlock because I don't see how it is directly related.
>
> In a sense both changes are needed to ultimately get rid of mlock. I'm
> also not sure how could I do the separation... Do you have something
> in mind?
>
> - Nuno Sá
Thanks,
Miquèl
