On Mon, 6 Jan 2025 17:14:12 -0600
David Lechner <dlechner@xxxxxxxxxxxx> wrote:
> On 1/5/25 11:25 AM, 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
>
> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxx/
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.
>
