RE: [PATCH 13/15] iio: health: max30100: do not use internal iio_dev lock

["Sa, Nuno" <Nuno.Sa@analog.com>]

> -----Original Message-----
> From: Miquel Raynal <miquel.raynal@bootlin.com>
> Sent: Tuesday, September 20, 2022 2:23 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-arm-kernel@lists.infradead.org; linux-rockchip@lists.infradead.org;
> linux-amlogic@lists.infradead.org; linux-imx@nxp.com; linux-
> iio@vger.kernel.org; Chunyan Zhang <zhang.lyra@gmail.com>; Hennerich,
> Michael <Michael.Hennerich@analog.com>; Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>; Sascha Hauer
> <s.hauer@pengutronix.de>; Cixi Geng <cixi.geng1@unisoc.com>; Kevin
> Hilman <khilman@baylibre.com>; Vladimir Zapolskiy <vz@mleia.com>;
> Pengutronix Kernel Team <kernel@pengutronix.de>; Alexandru Ardelean
> <aardelean@deviqon.com>; Fabio Estevam <festevam@gmail.com>; Andriy
> Tryshnivskyy <andriy.tryshnivskyy@opensynergy.com>; Haibo Chen
> <haibo.chen@nxp.com>; Shawn Guo <shawnguo@kernel.org>; Hans de
> Goede <hdegoede@redhat.com>; Jerome Brunet <jbrunet@baylibre.com>;
> Heiko Stuebner <heiko@sntech.de>; Florian Boor
> <florian.boor@kernelconcepts.de>; Regus, Ciprian
> <Ciprian.Regus@analog.com>; Lars-Peter Clausen <lars@metafoo.de>; Andy
> Shevchenko <andy.shevchenko@gmail.com>; Jonathan Cameron
> <jic23@kernel.org>; Neil Armstrong <narmstrong@baylibre.com>; Baolin
> Wang <baolin.wang@linux.alibaba.com>; Jyoti Bhayana
> <jbhayana@google.com>; Chen-Yu Tsai <wens@csie.org>; Orson Zhai
> <orsonzhai@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@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@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.
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á