Re: [PATCH v3 3/4] iio: health: max30102: do not use internal iio_dev lock

From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
To: "Nuno Sá" <noname.nuno@gmail.com>
Cc: "Miquel Raynal" <miquel.raynal@bootlin.com>,
	"Nuno Sá" <nuno.sa@analog.com>,
	linux-arm-kernel@lists.infradead.org, linux-iio@vger.kernel.org,
	linux-amlogic@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-imx@nxp.com,
	"Chen-Yu Tsai" <wens@csie.org>,
	"Andriy Tryshnivskyy" <andriy.tryshnivskyy@opensynergy.com>,
	"Ciprian Regus" <ciprian.regus@analog.com>,
	"Vladimir Zapolskiy" <vz@mleia.com>,
	"Cixi Geng" <cixi.geng1@unisoc.com>,
	"Neil Armstrong" <narmstrong@baylibre.com>,
	"Sascha Hauer" <s.hauer@pengutronix.de>,
	"Heiko Stuebner" <heiko@sntech.de>,
	"Fabio Estevam" <festevam@gmail.com>,
	"Jerome Brunet" <jbrunet@baylibre.com>,
	"Martin Blumenstingl" <martin.blumenstingl@googlemail.com>,
	"Pengutronix Kernel Team" <kernel@pengutronix.de>,
	"Baolin Wang" <baolin.wang@linux.alibaba.com>,
	"Hans de Goede" <hdegoede@redhat.com>,
	"Alexandru Ardelean" <aardelean@deviqon.com>,
	"Michael Hennerich" <Michael.Hennerich@analog.com>,
	"Haibo Chen" <haibo.chen@nxp.com>,
	"Lars-Peter Clausen" <lars@metafoo.de>,
	"Jyoti Bhayana" <jbhayana@google.com>,
	"Jonathan Cameron" <jic23@kernel.org>,
	"Andy Shevchenko" <andy.shevchenko@gmail.com>,
	"Florian Boor" <florian.boor@kernelconcepts.de>,
	"Chunyan Zhang" <zhang.lyra@gmail.com>,
	"Orson Zhai" <orsonzhai@gmail.com>,
	"Shawn Guo" <shawnguo@kernel.org>,
	"Kevin Hilman" <khilman@baylibre.com>
Subject: Re: [PATCH v3 3/4] iio: health: max30102: do not use internal iio_dev lock
Date: Fri, 14 Oct 2022 16:21:23 +0100	[thread overview]
Message-ID: <20221014162123.00000420@huawei.com> (raw)
In-Reply-To: <fccf3f41b32e881eb79ee78852fa40a62b134c00.camel@gmail.com>

On Fri, 14 Oct 2022 09:25:59 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Wed, 2022-10-12 at 20:45 +0200, Miquel Raynal wrote:
> > Hi Nuno,
> > 
> > nuno.sa@analog.com wrote on Wed, 12 Oct 2022 17:16:19 +0200:
> >   
> > > The pattern used in this device does not quite fit in the
> > > iio_device_claim_direct_mode() typical usage. In this case, we want
> > > to
> > > know if we are in buffered mode or not to know if the device is
> > > powered
> > > (buffer mode) or not. And depending on that max30102_get_temp()
> > > will
> > > power on the device if needed. Hence, in order to keep the same
> > > functionality, we try to:
> > > 
> > > 1. Claim Buffered mode;
> > > 2: If 1) succeeds call max30102_get_temp() without powering on the
> > >    device;
> > > 3: Release Buffered mode;
> > > 4: If 1) fails, Claim Direct mode;
> > > 5: If 4) succeeds call max30102_get_temp() with powering on the
> > > device;
> > > 6: Release Direct mode;
> > > 7: If 4) fails, goto to 1) and try again.
> > > 
> > > This dance between buffered and direct mode is not particularly
> > > pretty
> > > (as well as the loop introduced by the goto statement) but it does
> > > allow
> > > us to get rid of the mlock usage while keeping the same behavior.  
> > 
> > What about adding a TODO comment saying something like: "this comes
> > from static analysis and helped dropping mlock access, but someone
> > with
> > the device needs to figure out if we can simplify this dance"?
> > Because
> > the reason behind all this is that we don't want to risk breaking the
> > driver, but perhaps a simpler approach would work, right?
> >   
> 
> Hi Miquel,
> 
> AFAIU, either the device is powered (when buffer mode enabled) and we
> can do the reading or it's not and we need to power it on/off
> "manually" while making sure we don't race against enable/disabling
> buffers. This "dance" is needed mainly to make sure that we grab
> 'mlock' one way or another... The other way would be to use some
> specific device lock together with a flag (as discussed) but as
> discussed with Jonathan we decided to go down this road... So,
> honestly, I don't really see the necessity of "marking" this code with
> a TODO but of course if someone comes in with something simpler, great
> :).

Agreed.  I don't expect to see any improvement in this in the future
so a TODO would just be noise and might encourage people to propose
the 'get the lock on it's own function' that we are going through this
dance to avoid adding.

Jonathan

> 
> Anyways, as I said, I'm not really keen in spinning a new version to
> add this comment so I will defer the decision to Jonathan :)  
> 
> Thanks for the help!
> - Nuno Sá
> 

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