On Sun, 19 Jan 2025 15:03:33 -0300
Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx> wrote:
> 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.
This needs debugging as there should be no functional change.
> 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
I'm lost. I agree iio_device_claim_direct_mode() returns 0 on success, but
the wrapper in this patch effectively inverts that (see explanation of why, but
in short it is to make it look more like other locks and get more reliable
output 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)
//So if anything other than zero we return false
+ return false;
+
+ __acquire(iio_dev);
+
+ return true;
//return true on sucessfully taking the lock.
+}
Hence the check for we did not get the lock should be that new wrapper returning false.
As it is in your code above.
>
> + 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?
Ok. So it worked, but message unclear? Fair enough but not much we can do
about it as that is deep in sparse and technically that message is correct
as by inverting the logic the unlock is the point were sparse can tell it is backwards.
>
> > 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.
