Re: [PATCH v2 01/27] iio: core: Rework claim and release of direct mode to work with sparse.

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 17 Feb 2025 10:38:11 +0000
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sun, 2025-02-09 at 18:05 +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > Initial thought was to do something similar to __cond_lock()
> > 
> > 	do_iio_device_claim_direct_mode(iio_dev) ? : ({ __acquire(iio_dev);
> > 0; })
> > + Appropriate static inline iio_device_release_direct_mode()
> > 
> > However with that, sparse generates false positives. E.g.
> > 
> > drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c:1811:17: warning: context
> > imbalance in 'st_lsm6dsx_read_raw' - unexpected unlock
> > 
> > So instead, this patch rethinks the return type and makes it more
> > 'conditional lock like' (which is part of what is going on under the hood
> > anyway) and return a boolean - true for successfully acquired, false for
> > did not acquire.
> > 
> > To allow a migration path given the rework is now non trivial, take a leaf
> > out of the naming of the conditional guard we currently have for IIO
> > device direct mode and drop the _mode postfix from the new functions giving
> > iio_device_claim_direct() and iio_device_release_direct()
> > 
> > Whilst the kernel supports __cond_acquires() upstream sparse does not
> > yet do so.  Hence rely on sparse expanding a static inline wrapper
> > to explicitly see whether __acquire() is called.
> > 
> > Note that even with the solution here, sparse sometimes gives false
> > positives. However in the few cases seen they were complex code
> > structures that benefited from simplification anyway.
> > 
> > Reviewed-by: David Lechner <dlechner@baylibre.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> > v2: include linux/compiler_types.h (David)  
> 
> UhU, I'm not seeing it?
> 
Fixed up.  Thanks!
> > ---  
> 
> With the above,
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> 
> >  include/linux/iio/iio.h | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 56161e02f002..fe33835b19cf 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -662,6 +662,31 @@ int iio_push_event(struct iio_dev *indio_dev, u64
> > ev_code, s64 timestamp);
> >  int iio_device_claim_direct_mode(struct iio_dev *indio_dev);
> >  void iio_device_release_direct_mode(struct iio_dev *indio_dev);
> >  
> > +/*
> > + * Helper functions that allow claim and release of direct mode
> > + * in a fashion that doesn't generate many false positives from sparse.
> > + * Note this must remain static inline in the header so that sparse
> > + * can see the __acquire() marking. Revisit when sparse supports
> > + * __cond_acquires()
> > + */
> > +static inline bool iio_device_claim_direct(struct iio_dev *indio_dev)
> > +{
> > +	int ret = iio_device_claim_direct_mode(indio_dev);
> > +
> > +	if (ret)
> > +		return false;
> > +
> > +	__acquire(iio_dev);
> > +
> > +	return true;
> > +}
> > +
> > +static inline void iio_device_release_direct(struct iio_dev *indio_dev)
> > +{
> > +	iio_device_release_direct_mode(indio_dev);
> > +	__release(indio_dev);
> > +}
> > +
> >  /*
> >   * This autocleanup logic is normally used via
> >   * iio_device_claim_direct_scoped().  
>