On 01/25, Jonathan Cameron wrote: > 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> > > > >>>>> ... > > > >>>>> --- > > > >>>>> 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 > > > > > > > ... > > > > > 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. The patches in this series are correct. I have messed up while working on ad4170 and probably had replaced _claim_direct_scoped() in that driver with _claim_direct_mode() and a return check similar to the one for _claim_direct(). I've applied all the patches from this series, built, and tested ad4000 driver with AD7687 and it worked fine. Also, Sparse warns about context imbalance when _release_direct() is omitted. Sorry for the false bug alert. > > > > > + 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. >
