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.
