Re: [PATCH 05/10] iio: st_sensors: Use iio_device_claim/release_direct_mode() when relevant

From: Jonathan Cameron <jic23@kernel.org>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Alexandru Ardelean <ardeleanalex@gmail.com>,
	linux-iio <linux-iio@vger.kernel.org>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Denis Ciocca <denis.ciocca@st.com>
Subject: Re: [PATCH 05/10] iio: st_sensors: Use iio_device_claim/release_direct_mode() when relevant
Date: Sat, 15 Jan 2022 16:38:34 +0000	[thread overview]
Message-ID: <20220115163834.1d9ac991@jic23-huawei> (raw)
In-Reply-To: <20211216093243.42fd0c88@xps13>

On Thu, 16 Dec 2021 09:32:43 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hello Alexandru,
> 
> ardeleanalex@gmail.com wrote on Thu, 16 Dec 2021 09:16:34 +0200:
> 
> > On Wed, Dec 15, 2021 at 10:04 PM Miquel Raynal
> > <miquel.raynal@bootlin.com> wrote:  
> > >
> > > The st_sensors_core driver hardcodes the content of the
> > > iio_device_claim_direct_mode() and iio_device_release_direct_mode()
> > > helpers. Let's get rid of this handcrafted implementation and use the
> > > proper core helpers instead. Additionally, this lowers the tab level
> > > (which is always good) and prevents the use of the ->currentmode
> > > variable which is not supposed to be used like this anyway.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  .../iio/common/st_sensors/st_sensors_core.c   | 28 +++++++++----------
> > >  1 file changed, 13 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> > > index 1de395bda03e..e57e85c06f4b 100644
> > > --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> > > +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> > > @@ -549,26 +549,24 @@ int st_sensors_read_info_raw(struct iio_dev *indio_dev,
> > >         int err;
> > >         struct st_sensor_data *sdata = iio_priv(indio_dev);
> > >
> > > -       mutex_lock(&indio_dev->mlock);
> > > -       if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED) {
> > > -               err = -EBUSY;
> > > +       err = iio_device_claim_direct_mode(indio_dev);    
> > 
> > I'm afraid, for this driver, we would first need a cleanup of
> > indio_dev->mlock usage.
> > Or at least that's how I would start it.
> > i.e. remove the indio_dev->mlock and replace it with it's own
> > mutex/lock in all places (except this one).
> > 
> > The whole story about mlock is a bit old.
> > As I was told, it was initially defined in the iio_dev object, but not
> > very strictly controlled during review [of drivers].
> > Drivers kept using it (as a convenience lock).
> > It was later defined to be an IIO framework lock.  
> 
> I see, thanks for the explanation!

That's accurate.  Historical mistakes and all :)
We've been unwinding this for at least 5 years now so most of the simple
cases are now gone, though it seems not all of them!

> 
> > Now, there's a (slow) ongoing work to move mlock inside the
> > iio_dev_opaque struct, and make each driver use it's own lock, OR use
> > iio_device_{claim,release}_direct_mode() where appropriate.
> > 
> > FWIW: this change could go in as-is.  
> 
> /me breathes :-)
> 
> > But there's still the point of implementing another lock on the
> > st_sensor_data type.
> > I would try to split this work into another [parallel] series, because
> > otherwise [if fitted into this series] it would just grow and be
> > slow-to-review series.
> > But ¯\_(ツ)_/¯  
> 
> To be honest, my first goal was to document the modes enumeration.
> Then, I realized currentmodes was also needing a bit of explanations as
> well. Jonathan added that there were misuses with this variable. I then
> tried to reduce it's overall use (when not particularly needed) and I
> ended up doing this much bigger series, because every commit prepares
> the field for the next one.
> 
> The situation in this driver is (as you truly report):
> - the mlock is correctly used in the read_raw hook but everything is
>   hardcoded
> - the mlock is abused everywhere else
> 
> I am very sorry but I am not willing to entirely rework this driver
> because it's not the point of my series, I don't have the hardware and
> I know this would led to yet another series of changes, which I will
> have no time to handle >.<
> 
> My goal here is to reduce the usage count of currentmodes across all
> the device driver. This addresses the former point and I think it's
> completely valid because a series doing exactly what you request would
> definitely do this in two distinct steps as well O:-)

+CC some folks who are active on this driver (there are lots of them
as it covers some very common devices). For now I've +cc Denis but there
are others you should add to a v2.

Hmm. Question is whether doing this change in isolation from the more
general cleanup of mlock is a good move. It won't be obvious what
can race if someone comes along later trying to remove mlock usage...

One perhaps non obvious thing is at that a driver should not rely in
any way on the implementation of iio_device_claim/release_direct_mode().
It's only documented characteristics is it will fail to claim if we
are in buffered mode and that it prevents races with a transition to
buffered modes.

This particular read_raw path doesn't seem to use any shared
state (buffer is allocated in st_sensors_read_axis_data) but it
is turning the power on and off which is potentially fairly nasty
(and interestingly does rely on shared state - see later).

So it should use a local lock as well as mlock to prevent multiple
calls of st_sensors_read_info_raw racing with each other.
(claim_direct doesn't guarantee that because it's an implementation
detail that a driver should rely on - right now as it's open coded
we can know it is safe).

I took a quick look at what other mlock usage we do have...

The other cases in st_sensors_core.c should at most have been
iio_device_claim_direct_mode() etc.  Right now they'll block indefinitely
on a read of relevant _avail which is not good!

However, not clear why they even need to do that as they are simply
listing sdata->sensor_settings->odr.odr_avl etc which is const
data for a give device type.  So I'm fairly sure we could just drop those
two cases.

The mlock use in st_accel_core.c is again dubious as it will block
indefinitely if the buffer is enabled... 

So why is it here?  Probably
1) Avoid changing sampling frequency whilst buffer is running which
should be an iio_device_claim_direct_mode()
2) There is some state that might potentially want keeping in sync - which
would benefit from a local lock.  In particular sdata->odr which is
a local cache of the output data rate is used in st_sensors_set_enable()
which it could race with (if we can't rely on claim_direct())...

This last one is why I don't think we can do this function in isolation.

Sorry :(

On the plus side it's not that hard to fix as:
1) Drop the protection on _avail functions - it seems to be pointless.
2) Add a new lock that actually only ensures device setting for ODR is
   atomic wrt to the cached value.

Plenty of folks to test.   I'm happy to roll the patch if you want me
to, but as it's a precursor to your series perhaps better that you do.

I'm sure one of the people who have made changes to this driver recently
will help with testing.

Jonathan

> 
> Thanks,
> Miquèl
> 