On Mon, 17 Feb 2025 10:38:11 +0000
Nuno Sá <noname.nuno@xxxxxxxxx> wrote:
> On Sun, 2025-02-09 at 18:05 +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxx>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxx>
>
> > 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().
>
