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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Mon, 6 Jan 2025 17:14:12 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 1/5/25 11:25 AM, 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  
> 
> Even if false positives aren't technically wrong, if sparse is having a hard
> time reasoning about the code, then it is probably harder for humans to reason
> about the code as well. So rewriting these false positives anyway could be
> justified beyond just making the static analyzer happy.
> 
> > 
> > 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.  
> 
> I think changing this function to return bool instead of int is nice change
> anyway since it makes writing the code less prone authors to trying to do
> something "clever" with the ret variable. And it also saves one one line of
> code.
> 
> > 
> > To allow a migration path given the rework is now no 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()
> > 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  include/linux/iio/iio.h | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> > index 56161e02f002..4ef2f9893421 100644
> > --- a/include/linux/iio/iio.h
> > +++ b/include/linux/iio/iio.h
> > @@ -662,6 +662,28 @@ 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 false positives from sparse.
> > + */
> > +static inline bool iio_device_claim_direct(struct iio_dev *indio_dev) __cond_acquires(indio_dev)  
> 
> Doesn't __cond_acquires depend on this patch [1] that doesn't look like it was
> ever picked up in sparse?
> 
> [1]: https://lore.kernel.org/all/CAHk-=wjZfO9hGqJ2_hGQG3U_XzSh9_XaXze=HgPdvJbgrvASfA@mail.gmail.com/

I wondered about that. It 'seems' to do the job anyway. I didn't fully
understand that thread so I just blindly tried it instead :)

This case is simpler that that thread, so maybe those acrobatics aren't
needed?

Jonathan

> 
> > +{
> > +	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) __releases(indio_dev)
> > +{
> > +	iio_device_release_direct_mode(indio_dev);
> > +	__release(indio_dev);
> > +}
> > +
> >  /*
> >   * This autocleanup logic is normally used via
> >   * iio_device_claim_direct_scoped().  
> 
> In summary, assuming we get the required changed merged into sparse, I think this
> seems like the best solution.
>