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

[Miquel Raynal <miquel.raynal@bootlin.com>]

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!

> 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:-)

Thanks,
Miquèl