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 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
>
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.
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.
