diff mbox series

[RFC,01/27] iio: core: Rework claim and release of direct mode to work with sparse.

Message ID 20250105172613.1204781-2-jic23@kernel.org (mailing list archive)
State Changes Requested
Headers show
Series iio: improve handling of direct mode claim and release | expand

Commit Message

Jonathan Cameron Jan. 5, 2025, 5:25 p.m. UTC
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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

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.

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@huawei.com>
---
 include/linux/iio/iio.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

David Lechner Jan. 6, 2025, 11:14 p.m. UTC | #1
On 1/5/25 11:25 AM, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 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@huawei.com>
> ---
>  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@mail.gmail.com/

> +{
> +	int ret = iio_device_claim_direct_mode(indio_dev);
> +
> +	if (ret)
> +		return false;
> +
> +	__acquire(iio_dev);
> +
> +	return true;
> +}
> +
> +static inline void iio_device_release_direct(struct iio_dev *indio_dev) __releases(indio_dev)
> +{
> +	iio_device_release_direct_mode(indio_dev);
> +	__release(indio_dev);
> +}
> +
>  /*
>   * This autocleanup logic is normally used via
>   * iio_device_claim_direct_scoped().

In summary, assuming we get the required changed merged into sparse, I think this
seems like the best solution.
Jonathan Cameron Jan. 7, 2025, 2:24 p.m. UTC | #2
On Mon, 6 Jan 2025 17:14:12 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 1/5/25 11:25 AM, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > 
> > 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@huawei.com>
> > ---
> >  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@mail.gmail.com/

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?

Jonathan

> 
> > +{
> > +	int ret = iio_device_claim_direct_mode(indio_dev);
> > +
> > +	if (ret)
> > +		return false;
> > +
> > +	__acquire(iio_dev);
> > +
> > +	return true;
> > +}
> > +
> > +static inline void iio_device_release_direct(struct iio_dev *indio_dev) __releases(indio_dev)
> > +{
> > +	iio_device_release_direct_mode(indio_dev);
> > +	__release(indio_dev);
> > +}
> > +
> >  /*
> >   * This autocleanup logic is normally used via
> >   * iio_device_claim_direct_scoped().  
> 
> In summary, assuming we get the required changed merged into sparse, I think this
> seems like the best solution.
>
David Lechner Jan. 7, 2025, 4:09 p.m. UTC | #3
On 1/7/25 8:24 AM, Jonathan Cameron wrote:
> On Mon, 6 Jan 2025 17:14:12 -0600
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> On 1/5/25 11:25 AM, Jonathan Cameron wrote:
>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>
>>> 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@huawei.com>
>>> ---
>>>  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@mail.gmail.com/
> 
> 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.
Jonathan Cameron Jan. 11, 2025, 1:35 p.m. UTC | #4
On Tue, 7 Jan 2025 10:09:15 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 1/7/25 8:24 AM, Jonathan Cameron wrote:
> > On Mon, 6 Jan 2025 17:14:12 -0600
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> >> On 1/5/25 11:25 AM, Jonathan Cameron wrote:  
> >>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >>>
> >>> 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@huawei.com>
> >>> ---
> >>>  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@mail.gmail.com/  
> > 
> > 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
David Lechner Jan. 11, 2025, 10:28 p.m. UTC | #5
On 1/11/25 7:35 AM, Jonathan Cameron wrote:
> On Tue, 7 Jan 2025 10:09:15 -0600
> David Lechner <dlechner@baylibre.com> wrote:
> 
>> On 1/7/25 8:24 AM, Jonathan Cameron wrote:
>>> On Mon, 6 Jan 2025 17:14:12 -0600
>>> David Lechner <dlechner@baylibre.com> wrote:
>>>   
>>>> On 1/5/25 11:25 AM, Jonathan Cameron wrote:  
>>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>>>
>>>>> 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@huawei.com>
>>>>> ---
>>>>>  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@mail.gmail.com/  
>>>
>>> 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.
Marcelo Schmitt Jan. 19, 2025, 6:03 p.m. UTC | #6
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@baylibre.com> wrote:
> >
> >> On 1/7/25 8:24 AM, Jonathan Cameron wrote:
> >>> On Mon, 6 Jan 2025 17:14:12 -0600
> >>> David Lechner <dlechner@baylibre.com> wrote:
> >>>
> >>>> On 1/5/25 11:25 AM, Jonathan Cameron wrote:
> >>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >>>>>
> >>>>> 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@huawei.com>
> >>>>> ---
> >>>>>  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@mail.gmail.com/

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@redhat.com/
[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.
Jonathan Cameron Jan. 19, 2025, 7:29 p.m. UTC | #7
On Sat, 11 Jan 2025 16:28:02 -0600
David Lechner <dlechner@baylibre.com> wrote:

> On 1/11/25 7:35 AM, Jonathan Cameron wrote:
> > On Tue, 7 Jan 2025 10:09:15 -0600
> > David Lechner <dlechner@baylibre.com> wrote:
> >   
> >> On 1/7/25 8:24 AM, Jonathan Cameron wrote:  
> >>> On Mon, 6 Jan 2025 17:14:12 -0600
> >>> David Lechner <dlechner@baylibre.com> wrote:
> >>>     
> >>>> On 1/5/25 11:25 AM, Jonathan Cameron wrote:    
> >>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >>>>>
> >>>>> 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@huawei.com>
> >>>>> ---
> >>>>>  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@mail.gmail.com/    
> >>>
> >>> 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.

Ah. That indeed explains it.

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

I'm in two minds over whether to keep the __cond_acquires / __releases markings
given there are other in tree uses already.  Mind you I don't care strongly
and I assume sparse at least always obeys inline instructions so what we
probably need is a comment to ensure we never move the code out of the header
unless sparse is fixed.  So I'll go ahead and drop the markings for now.

Even if sparse is never updated, we can keep a little bit of inline magic
in the header to handle the conditional __acquire() and have the real
acquire in an evil looking __iio_device_claim_direct() that no one
should ever touch directly.

We could in theory do similar for the existing iio_device_claim_direct_mode()
but I 'think' that will still give us some false positives + the boolean
return is nicer anyway and tends to give slightly more compact code..

So far making this change to a bunch more drivers has found 2 bugs and led
to a quite a bit of code cleanup. I'll finish doing the 'a's (accel and adc)
then post the result.  One thing I will probably change on this initial set is
splitting the refactors and changes to the new interface into separate
patches as they are different types of change.

Jonathan
> 
>
Jonathan Cameron Jan. 25, 2025, 11:59 a.m. UTC | #8
On Sun, 19 Jan 2025 15:03:33 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> 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@baylibre.com> wrote:
> > >  
> > >> On 1/7/25 8:24 AM, Jonathan Cameron wrote:  
> > >>> On Mon, 6 Jan 2025 17:14:12 -0600
> > >>> David Lechner <dlechner@baylibre.com> wrote:
> > >>>  
> > >>>> On 1/5/25 11:25 AM, Jonathan Cameron wrote:  
> > >>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >>>>>
> > >>>>> 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@huawei.com>
> > >>>>> ---
> > >>>>>  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@mail.gmail.com/  
> 
> 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@redhat.com/
> [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.
Jonathan Cameron Jan. 26, 2025, 7:23 p.m. UTC | #9
On Sun, 19 Jan 2025 19:29:48 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sat, 11 Jan 2025 16:28:02 -0600
> David Lechner <dlechner@baylibre.com> wrote:
> 
> > On 1/11/25 7:35 AM, Jonathan Cameron wrote:  
> > > On Tue, 7 Jan 2025 10:09:15 -0600
> > > David Lechner <dlechner@baylibre.com> wrote:
> > >     
> > >> On 1/7/25 8:24 AM, Jonathan Cameron wrote:    
> > >>> On Mon, 6 Jan 2025 17:14:12 -0600
> > >>> David Lechner <dlechner@baylibre.com> wrote:
> > >>>       
> > >>>> On 1/5/25 11:25 AM, Jonathan Cameron wrote:      
> > >>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >>>>>
> > >>>>> 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@huawei.com>
> > >>>>> ---
> > >>>>>  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@mail.gmail.com/      
> > >>>
> > >>> 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.  
> 
> Ah. That indeed explains it.
> 
> > 
> > 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.  
> 
> I'm in two minds over whether to keep the __cond_acquires / __releases markings
> given there are other in tree uses already.  Mind you I don't care strongly
> and I assume sparse at least always obeys inline instructions so what we
> probably need is a comment to ensure we never move the code out of the header
> unless sparse is fixed.  So I'll go ahead and drop the markings for now.
> 
> Even if sparse is never updated, we can keep a little bit of inline magic
> in the header to handle the conditional __acquire() and have the real
> acquire in an evil looking __iio_device_claim_direct() that no one
> should ever touch directly.
> 
> We could in theory do similar for the existing iio_device_claim_direct_mode()
> but I 'think' that will still give us some false positives + the boolean
> return is nicer anyway and tends to give slightly more compact code..
> 
> So far making this change to a bunch more drivers has found 2 bugs and led
> to a quite a bit of code cleanup. I'll finish doing the 'a's (accel and adc)
> then post the result.  One thing I will probably change on this initial set is
> splitting the refactors and changes to the new interface into separate
> patches as they are different types of change.

I've updated this across the tree (locally). Even whilst making it look just
like a conditional lock there are still a couple of false positives in
light sensors (IIRC vcnl4000 and as73211) that needed more radical surgery
to avoid.  As you note above, maybe that code wasn't that readable anyway
so will benefit.  It seems sparse gets confused with some gotos when mixed
with other control flow stuff like switch statements.

Sometime soon I'll tidy it up into a coherent series - (after working
out what Marcello ran into!)  Exactly how much cleanup and refactoring
I did whilst touching the code is closely correlated to how bored I was
getting so i should probably make that a little more consistent.

Last patch will remove the old infrastructure, which will be a pain for
anyone with out of tree drivers - but easy enough to revert that one for
now.

Jonathan

> 
> Jonathan
> > 
> >   
> 
>
Marcelo Schmitt Jan. 29, 2025, 4:34 p.m. UTC | #10
On 01/25, Jonathan Cameron wrote:
> On Sun, 19 Jan 2025 15:03:33 -0300
> Marcelo Schmitt <marcelo.schmitt1@gmail.com> 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@baylibre.com> wrote:
> > > >  
> > > >> On 1/7/25 8:24 AM, Jonathan Cameron wrote:  
> > > >>> On Mon, 6 Jan 2025 17:14:12 -0600
> > > >>> David Lechner <dlechner@baylibre.com> wrote:
> > > >>>  
> > > >>>> On 1/5/25 11:25 AM, Jonathan Cameron wrote:  
> > > >>>>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > >>>>>
...
> > > >>>>> ---
> > > >>>>>  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@mail.gmail.com/  
> > 
> > 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@redhat.com/
> > [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.  
>
diff mbox series

Patch

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)
+{
+	int ret = iio_device_claim_direct_mode(indio_dev);
+
+	if (ret)
+		return false;
+
+	__acquire(iio_dev);
+
+	return true;
+}
+
+static inline void iio_device_release_direct(struct iio_dev *indio_dev) __releases(indio_dev)
+{
+	iio_device_release_direct_mode(indio_dev);
+	__release(indio_dev);
+}
+
 /*
  * This autocleanup logic is normally used via
  * iio_device_claim_direct_scoped().