On 01/11, David Lechner wrote:
> On 1/11/25 7:35 AM, Jonathan Cameron wrote:
> > 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 applied those two patches to iio testing branch. The diffs appear to be
duplicated in email patches and patch 1 first hunk applies to line 353 of
include/linux/refcount.h instead of line 361. I also didn't re-add
__cond_acquires() which is present in current kernel but not in the patch.
I then applied patch 1 and patch 9 of this series.
> >>>
> >>> 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];
> >
With the patches from [1], patches 1 and 9 of this series, and the change above,
I got the different lock contexts warn as shown above. No change to Sparse.
Tested with both Sparse v0.6.4-66-g0196afe1 from [2] and the Sparse version
I have from Debian (Sparse 0.6.4 (Debian: 0.6.4-5)).
[1]: https://lore.kernel.org/all/20220630135934.1799248-1-aahringo@xxxxxxxxxx/
[2]: git://git.kernel.org/pub/scm/devel/sparse/sparse.git
> > Was the test I ran today.
> >
> > Jonathan
> >
>
> Hmmm... I think maybe I had some other local modifications when I was testing
> previously. But I understand better what is going on now. Your implementation
> is only working because it is static inline. The __cond_acquires() and
> __releases() attributes have no effect and the "different lock contexts for
> basic block" warning is coming from the __acquire() and __release() attributes.
> So it is working correctly, but perhaps not for the reason you thought.
>
> I think what I had done locally is make iio_device_claim_direct() and
> iio_device_release_direct() regular functions instead of static inline so that
> it had to actually make use of __cond_acquires(). In that case, with an
> unpatched sparse, we get "unexpected unlock" warnings for all calls to
> iio_device_release_direct(). With patched sparse, this warning goes away.
>
I think the patches changing to iio_device_claim_direct() are bugy.
I did something similar while working on ad4170 and got deadlock.
In the patch updating ad4000 we had
case IIO_CHAN_INFO_RAW:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
- return ad4000_single_conversion(indio_dev, chan, val);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = ad4000_single_conversion(indio_dev, chan, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
iio_device_claim_direct_mode() returns 0 when the user is able to acquire direct
mode so !iio_device_claim_direct_mode() evaluates to true when we are be able to
acquire iio_dev_opaque mlock, but the ADC driver was returning -EBUSY anyway and
never unlocking mlock. We should do
+ if (iio_device_claim_direct(indio_dev))
It was when I ran Sparse without negating iio_device_claim_direct() that I got
ad4000.c:545:17: warning: context imbalance in 'ad4000_read_raw' - unexpected unlock
Maybe the warn would have been more useful if it was "suspicious error return
after lock acquisition" or something like that?
> So for now, we could take your patch with the __cond_acquires() and
> __releases() attribute removed (since they don't do anything) and leave
> ourselves a note in a comment that sparse needs to be fixed so that we can use
> the __cond_acquires() attribute if/when we get rid of
> iio_device_release_direct_mode() completely and want to make
> iio_device_release_direct() a regular function.
