| Message ID | 20250209180624.701140-2-jic23@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: improve handling of direct mode claim and release | expand |
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? > --- 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().
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(). >
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().