| Message ID | 20250105172613.1204781-2-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: improve handling of direct mode claim and release | expand |
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/ > +{ > + 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.
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. >
On 1/7/25 8:24 AM, Jonathan Cameron wrote: > 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? I was not able to get a sparse warning without applying that patch to sparse first. My test method was to apply this series to my Linux tree and then comment out a iio_device_release_direct() line in a random driver. And looking at the way the check works, this is exactly what I would expect. The negative output argument in __attribute__((context,x,0,-1)) means something different (check = 0) without the spare patch applied.
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) +{ + 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().