On Tue, 7 Jan 2025 10:09:15 -0600
David Lechner <dlechner@xxxxxxxxxxxx> wrote:
> On 1/7/25 8:24 AM, Jonathan Cameron wrote:
> > 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?
>
> 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.
>
Curious. I wasn't being remotely careful with what sparse version
i was running so just went with what Arch is carrying which turns out to be
a bit old.
Same test as you describe gives me:
CHECK drivers/iio/adc/ad4000.c
drivers/iio/adc/ad4000.c:533:12: warning: context imbalance in 'ad4000_read_raw' - different lock contexts for basic block
So I tried that with latest sparse from kernel.org and I still get that warning
which is what I'd expect to see.
Simple make C=1 W=1 build
I wonder what we have different? Maybe it is missing some cases?
diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
index ef0acaafbcdb..6785d55ff53a 100644
--- a/drivers/iio/adc/ad4000.c
+++ b/drivers/iio/adc/ad4000.c
@@ -543,7 +543,7 @@ static int ad4000_read_raw(struct iio_dev *indio_dev,
return -EBUSY;
ret = ad4000_single_conversion(indio_dev, chan, val);
- iio_device_release_direct(indio_dev);
+// iio_device_release_direct(indio_dev);
return ret;
case IIO_CHAN_INFO_SCALE:
*val = st->scale_tbl[st->span_comp][0];
Was the test I ran today.
Jonathan
