> -----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.
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á
