* [RFC PATCH 01/27] iio: core: Rework claim and release of direct mode to work with sparse.
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
@ 2025-01-05 17:25 ` Jonathan Cameron
2025-01-06 23:14 ` David Lechner
2025-01-05 17:25 ` [RFC PATCH 02/27] iio: chemical: scd30: Switch to sparse friendly claim/release_direct() Jonathan Cameron
` (27 subsequent siblings)
28 siblings, 1 reply; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:25 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
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(+)
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().
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [RFC PATCH 01/27] iio: core: Rework claim and release of direct mode to work with sparse.
2025-01-05 17:25 ` [RFC PATCH 01/27] iio: core: Rework claim and release of direct mode to work with sparse Jonathan Cameron
@ 2025-01-06 23:14 ` David Lechner
2025-01-07 14:24 ` Jonathan Cameron
0 siblings, 1 reply; 49+ messages in thread
From: David Lechner @ 2025-01-06 23:14 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio, “Luc Van Oostenryck”
Cc: Jonathan Cameron
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.
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [RFC PATCH 01/27] iio: core: Rework claim and release of direct mode to work with sparse.
2025-01-06 23:14 ` David Lechner
@ 2025-01-07 14:24 ` Jonathan Cameron
2025-01-07 16:09 ` David Lechner
0 siblings, 1 reply; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-07 14:24 UTC (permalink / raw)
To: David Lechner
Cc: Jonathan Cameron, linux-iio, “Luc Van Oostenryck”
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.
>
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [RFC PATCH 01/27] iio: core: Rework claim and release of direct mode to work with sparse.
2025-01-07 14:24 ` Jonathan Cameron
@ 2025-01-07 16:09 ` David Lechner
2025-01-11 13:35 ` Jonathan Cameron
0 siblings, 1 reply; 49+ messages in thread
From: David Lechner @ 2025-01-07 16:09 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, linux-iio, “Luc Van Oostenryck”
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.
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [RFC PATCH 01/27] iio: core: Rework claim and release of direct mode to work with sparse.
2025-01-07 16:09 ` David Lechner
@ 2025-01-11 13:35 ` Jonathan Cameron
2025-01-11 22:28 ` David Lechner
0 siblings, 1 reply; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-11 13:35 UTC (permalink / raw)
To: David Lechner
Cc: Jonathan Cameron, linux-iio, “Luc Van Oostenryck”
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
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [RFC PATCH 01/27] iio: core: Rework claim and release of direct mode to work with sparse.
2025-01-11 13:35 ` Jonathan Cameron
@ 2025-01-11 22:28 ` David Lechner
2025-01-19 18:03 ` Marcelo Schmitt
2025-01-19 19:29 ` Jonathan Cameron
0 siblings, 2 replies; 49+ messages in thread
From: David Lechner @ 2025-01-11 22:28 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, linux-iio, “Luc Van Oostenryck”
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.
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [RFC PATCH 01/27] iio: core: Rework claim and release of direct mode to work with sparse.
2025-01-11 22:28 ` David Lechner
@ 2025-01-19 18:03 ` Marcelo Schmitt
2025-01-25 11:59 ` Jonathan Cameron
2025-01-19 19:29 ` Jonathan Cameron
1 sibling, 1 reply; 49+ messages in thread
From: Marcelo Schmitt @ 2025-01-19 18:03 UTC (permalink / raw)
To: David Lechner
Cc: Jonathan Cameron, Jonathan Cameron, linux-iio,
“Luc Van Oostenryck”
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.
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [RFC PATCH 01/27] iio: core: Rework claim and release of direct mode to work with sparse.
2025-01-19 18:03 ` Marcelo Schmitt
@ 2025-01-25 11:59 ` Jonathan Cameron
2025-01-29 16:34 ` Marcelo Schmitt
0 siblings, 1 reply; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-25 11:59 UTC (permalink / raw)
To: Marcelo Schmitt
Cc: David Lechner, Jonathan Cameron, linux-iio,
“Luc Van Oostenryck”
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.
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [RFC PATCH 01/27] iio: core: Rework claim and release of direct mode to work with sparse.
2025-01-25 11:59 ` Jonathan Cameron
@ 2025-01-29 16:34 ` Marcelo Schmitt
0 siblings, 0 replies; 49+ messages in thread
From: Marcelo Schmitt @ 2025-01-29 16:34 UTC (permalink / raw)
To: Jonathan Cameron
Cc: David Lechner, Jonathan Cameron, linux-iio,
“Luc Van Oostenryck”
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.
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [RFC PATCH 01/27] iio: core: Rework claim and release of direct mode to work with sparse.
2025-01-11 22:28 ` David Lechner
2025-01-19 18:03 ` Marcelo Schmitt
@ 2025-01-19 19:29 ` Jonathan Cameron
2025-01-26 19:23 ` Jonathan Cameron
1 sibling, 1 reply; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-19 19:29 UTC (permalink / raw)
To: David Lechner
Cc: Jonathan Cameron, linux-iio, “Luc Van Oostenryck”
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
>
>
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [RFC PATCH 01/27] iio: core: Rework claim and release of direct mode to work with sparse.
2025-01-19 19:29 ` Jonathan Cameron
@ 2025-01-26 19:23 ` Jonathan Cameron
0 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-26 19:23 UTC (permalink / raw)
To: David Lechner
Cc: Jonathan Cameron, linux-iio, “Luc Van Oostenryck”
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
> >
> >
>
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* [RFC PATCH 02/27] iio: chemical: scd30: Switch to sparse friendly claim/release_direct()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
2025-01-05 17:25 ` [RFC PATCH 01/27] iio: core: Rework claim and release of direct mode to work with sparse Jonathan Cameron
@ 2025-01-05 17:25 ` Jonathan Cameron
2025-01-06 23:22 ` David Lechner
2025-01-05 17:25 ` [RFC PATCH 03/27] iio: temperature: tmp006: Stop using iio_device_claim_direct_scoped() Jonathan Cameron
` (26 subsequent siblings)
28 siblings, 1 reply; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:25 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This driver caused a false positive with __cond_lock() style solution
but is fine with the simple boolean return approach now used.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/chemical/scd30_core.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
index d613c54cb28d..cfbf2f5e9443 100644
--- a/drivers/iio/chemical/scd30_core.c
+++ b/drivers/iio/chemical/scd30_core.c
@@ -211,18 +211,19 @@ static int scd30_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const
break;
}
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
+ if (!iio_device_claim_direct(indio_dev)) {
+ ret = -EBUSY;
break;
+ }
ret = scd30_read(state);
if (ret) {
- iio_device_release_direct_mode(indio_dev);
+ iio_device_release_direct(indio_dev);
break;
}
*val = state->meas[chan->address];
- iio_device_release_direct_mode(indio_dev);
+ iio_device_release_direct(indio_dev);
ret = IIO_VAL_INT;
break;
case IIO_CHAN_INFO_SCALE:
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [RFC PATCH 02/27] iio: chemical: scd30: Switch to sparse friendly claim/release_direct()
2025-01-05 17:25 ` [RFC PATCH 02/27] iio: chemical: scd30: Switch to sparse friendly claim/release_direct() Jonathan Cameron
@ 2025-01-06 23:22 ` David Lechner
0 siblings, 0 replies; 49+ messages in thread
From: David Lechner @ 2025-01-06 23:22 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio, “Luc Van Oostenryck”
Cc: Jonathan Cameron
On 1/5/25 11:25 AM, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> This driver caused a false positive with __cond_lock() style solution
> but is fine with the simple boolean return approach now used.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/iio/chemical/scd30_core.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/chemical/scd30_core.c b/drivers/iio/chemical/scd30_core.c
> index d613c54cb28d..cfbf2f5e9443 100644
> --- a/drivers/iio/chemical/scd30_core.c
> +++ b/drivers/iio/chemical/scd30_core.c
> @@ -211,18 +211,19 @@ static int scd30_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const
> break;
> }
>
> - ret = iio_device_claim_direct_mode(indio_dev);
> - if (ret)
> + if (!iio_device_claim_direct(indio_dev)) {
> + ret = -EBUSY;
> break;
> + }
>
> ret = scd30_read(state);
> if (ret) {
> - iio_device_release_direct_mode(indio_dev);
> + iio_device_release_direct(indio_dev);
> break;
> }
>
> *val = state->meas[chan->address];
> - iio_device_release_direct_mode(indio_dev);
> + iio_device_release_direct(indio_dev);
> ret = IIO_VAL_INT;
> break;
> case IIO_CHAN_INFO_SCALE:
Could do with a precursor patch to use guard(mutex) to allow returning directly
everywhere and avoid break;.
^ permalink raw reply [flat|nested] 49+ messages in thread
* [RFC PATCH 03/27] iio: temperature: tmp006: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
2025-01-05 17:25 ` [RFC PATCH 01/27] iio: core: Rework claim and release of direct mode to work with sparse Jonathan Cameron
2025-01-05 17:25 ` [RFC PATCH 02/27] iio: chemical: scd30: Switch to sparse friendly claim/release_direct() Jonathan Cameron
@ 2025-01-05 17:25 ` Jonathan Cameron
2025-01-05 17:25 ` [RFC PATCH 04/27] iio: imu: st_lsm6dsx: Switch to sparse friendly claim/release_direct() Jonathan Cameron
` (25 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:25 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/temperature/tmp006.c | 33 ++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/temperature/tmp006.c b/drivers/iio/temperature/tmp006.c
index 7beaabc9ecc7..4b3a90538eec 100644
--- a/drivers/iio/temperature/tmp006.c
+++ b/drivers/iio/temperature/tmp006.c
@@ -85,19 +85,25 @@ static int tmp006_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_RAW:
if (channel->type == IIO_VOLTAGE) {
/* LSB is 156.25 nV */
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- ret = tmp006_read_measurement(data, TMP006_VOBJECT);
- if (ret < 0)
- return ret;
- }
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = tmp006_read_measurement(data, TMP006_VOBJECT);
+ iio_device_release_direct(indio_dev);
+ if (ret < 0)
+ return ret;
+
*val = sign_extend32(ret, 15);
} else if (channel->type == IIO_TEMP) {
/* LSB is 0.03125 degrees Celsius */
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- ret = tmp006_read_measurement(data, TMP006_TAMBIENT);
- if (ret < 0)
- return ret;
- }
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = tmp006_read_measurement(data, TMP006_TAMBIENT);
+ iio_device_release_direct(indio_dev);
+ if (ret < 0)
+ return ret;
+
*val = sign_extend32(ret, 15) >> TMP006_TAMBIENT_SHIFT;
} else {
break;
@@ -142,9 +148,8 @@ static int tmp006_write_raw(struct iio_dev *indio_dev,
for (i = 0; i < ARRAY_SIZE(tmp006_freqs); i++)
if ((val == tmp006_freqs[i][0]) &&
(val2 == tmp006_freqs[i][1])) {
- ret = iio_device_claim_direct_mode(indio_dev);
- if (ret)
- return ret;
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
data->config &= ~TMP006_CONFIG_CR_MASK;
data->config |= i << TMP006_CONFIG_CR_SHIFT;
@@ -153,7 +158,7 @@ static int tmp006_write_raw(struct iio_dev *indio_dev,
TMP006_CONFIG,
data->config);
- iio_device_release_direct_mode(indio_dev);
+ iio_device_release_direct(indio_dev);
return ret;
}
return -EINVAL;
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 04/27] iio: imu: st_lsm6dsx: Switch to sparse friendly claim/release_direct()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (2 preceding siblings ...)
2025-01-05 17:25 ` [RFC PATCH 03/27] iio: temperature: tmp006: Stop using iio_device_claim_direct_scoped() Jonathan Cameron
@ 2025-01-05 17:25 ` Jonathan Cameron
2025-01-05 17:25 ` [RFC PATCH 05/27] iio: proximity: sx9310: Stop using iio_device_claim_direct_scoped() Jonathan Cameron
` (24 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:25 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This driver caused a false positive with __cond_lock() style solution
but is fine with the simple boolean return approach now used.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 4fdcc2acc94e..cc7a62198982 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -1804,12 +1804,11 @@ static int st_lsm6dsx_read_raw(struct iio_dev *iio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
- ret = iio_device_claim_direct_mode(iio_dev);
- if (ret)
- break;
+ if (!iio_device_claim_direct(iio_dev))
+ return -EBUSY;
ret = st_lsm6dsx_read_oneshot(sensor, ch->address, val);
- iio_device_release_direct_mode(iio_dev);
+ iio_device_release_direct(iio_dev);
break;
case IIO_CHAN_INFO_SAMP_FREQ:
*val = sensor->odr / 1000;
@@ -1836,9 +1835,8 @@ static int st_lsm6dsx_write_raw(struct iio_dev *iio_dev,
struct st_lsm6dsx_sensor *sensor = iio_priv(iio_dev);
int err;
- err = iio_device_claim_direct_mode(iio_dev);
- if (err)
- return err;
+ if (!iio_device_claim_direct(iio_dev))
+ return -EBUSY;
switch (mask) {
case IIO_CHAN_INFO_SCALE:
@@ -1860,7 +1858,7 @@ static int st_lsm6dsx_write_raw(struct iio_dev *iio_dev,
break;
}
- iio_device_release_direct_mode(iio_dev);
+ iio_device_release_direct(iio_dev);
return err;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 05/27] iio: proximity: sx9310: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (3 preceding siblings ...)
2025-01-05 17:25 ` [RFC PATCH 04/27] iio: imu: st_lsm6dsx: Switch to sparse friendly claim/release_direct() Jonathan Cameron
@ 2025-01-05 17:25 ` Jonathan Cameron
2025-01-05 17:25 ` [RFC PATCH 06/27] iio: proximity: sx9324: " Jonathan Cameron
` (23 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:25 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/proximity/sx9310.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/proximity/sx9310.c b/drivers/iio/proximity/sx9310.c
index 0d7f0518d4fb..b60707eba39d 100644
--- a/drivers/iio/proximity/sx9310.c
+++ b/drivers/iio/proximity/sx9310.c
@@ -337,19 +337,26 @@ static int sx9310_read_raw(struct iio_dev *indio_dev,
int *val2, long mask)
{
struct sx_common_data *data = iio_priv(indio_dev);
+ int ret;
if (chan->type != IIO_PROXIMITY)
return -EINVAL;
switch (mask) {
case IIO_CHAN_INFO_RAW:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
- return sx_common_read_proximity(data, chan, val);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = sx_common_read_proximity(data, chan, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CHAN_INFO_HARDWAREGAIN:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
- return sx9310_read_gain(data, chan, val);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = sx9310_read_gain(data, chan, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CHAN_INFO_SAMP_FREQ:
return sx9310_read_samp_freq(data, val, val2);
default:
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 06/27] iio: proximity: sx9324: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (4 preceding siblings ...)
2025-01-05 17:25 ` [RFC PATCH 05/27] iio: proximity: sx9310: Stop using iio_device_claim_direct_scoped() Jonathan Cameron
@ 2025-01-05 17:25 ` Jonathan Cameron
2025-01-05 17:25 ` [RFC PATCH 07/27] iio: proximity: sx9360: " Jonathan Cameron
` (22 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:25 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/proximity/sx9324.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
index f7819dd2775c..73d972416c01 100644
--- a/drivers/iio/proximity/sx9324.c
+++ b/drivers/iio/proximity/sx9324.c
@@ -429,16 +429,23 @@ static int sx9324_read_raw(struct iio_dev *indio_dev,
int *val, int *val2, long mask)
{
struct sx_common_data *data = iio_priv(indio_dev);
+ int ret;
switch (mask) {
case IIO_CHAN_INFO_RAW:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
- return sx_common_read_proximity(data, chan, val);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = sx_common_read_proximity(data, chan, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CHAN_INFO_HARDWAREGAIN:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
- return sx9324_read_gain(data, chan, val);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = sx9324_read_gain(data, chan, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CHAN_INFO_SAMP_FREQ:
return sx9324_read_samp_freq(data, val, val2);
default:
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 07/27] iio: proximity: sx9360: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (5 preceding siblings ...)
2025-01-05 17:25 ` [RFC PATCH 06/27] iio: proximity: sx9324: " Jonathan Cameron
@ 2025-01-05 17:25 ` Jonathan Cameron
2025-01-05 17:25 ` [RFC PATCH 08/27] iio: accel: adxl367: " Jonathan Cameron
` (21 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:25 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/proximity/sx9360.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/proximity/sx9360.c b/drivers/iio/proximity/sx9360.c
index a6ff16e33c1e..4448988d4e7e 100644
--- a/drivers/iio/proximity/sx9360.c
+++ b/drivers/iio/proximity/sx9360.c
@@ -321,16 +321,23 @@ static int sx9360_read_raw(struct iio_dev *indio_dev,
int *val, int *val2, long mask)
{
struct sx_common_data *data = iio_priv(indio_dev);
+ int ret;
switch (mask) {
case IIO_CHAN_INFO_RAW:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
- return sx_common_read_proximity(data, chan, val);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = sx_common_read_proximity(data, chan, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CHAN_INFO_HARDWAREGAIN:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
- return sx9360_read_gain(data, chan, val);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = sx9360_read_gain(data, chan, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CHAN_INFO_SAMP_FREQ:
return sx9360_read_samp_freq(data, val, val2);
default:
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 08/27] iio: accel: adxl367: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (6 preceding siblings ...)
2025-01-05 17:25 ` [RFC PATCH 07/27] iio: proximity: sx9360: " Jonathan Cameron
@ 2025-01-05 17:25 ` Jonathan Cameron
2025-01-05 17:25 ` [RFC PATCH 09/27] iio: adc: ad4000: " Jonathan Cameron
` (20 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:25 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context
In some cases there is a convenient wrapper function to which
the handling can be moved. Do that instead of introducing
another layer of wrappers. In others an outer wrapper is added
which claims direct mode, runs the original function with the
scoped claim logic removed, releases direct mode and then checks
for errors.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/accel/adxl367.c | 194 ++++++++++++++++++++----------------
1 file changed, 106 insertions(+), 88 deletions(-)
diff --git a/drivers/iio/accel/adxl367.c b/drivers/iio/accel/adxl367.c
index a48ac0d7bd96..add4053e7a02 100644
--- a/drivers/iio/accel/adxl367.c
+++ b/drivers/iio/accel/adxl367.c
@@ -477,45 +477,42 @@ static int adxl367_set_fifo_watermark(struct adxl367_state *st,
static int adxl367_set_range(struct iio_dev *indio_dev,
enum adxl367_range range)
{
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- struct adxl367_state *st = iio_priv(indio_dev);
- int ret;
+ struct adxl367_state *st = iio_priv(indio_dev);
+ int ret;
- guard(mutex)(&st->lock);
+ guard(mutex)(&st->lock);
- ret = adxl367_set_measure_en(st, false);
- if (ret)
- return ret;
+ ret = adxl367_set_measure_en(st, false);
+ if (ret)
+ return ret;
- ret = regmap_update_bits(st->regmap, ADXL367_REG_FILTER_CTL,
- ADXL367_FILTER_CTL_RANGE_MASK,
- FIELD_PREP(ADXL367_FILTER_CTL_RANGE_MASK,
- range));
- if (ret)
- return ret;
+ ret = regmap_update_bits(st->regmap, ADXL367_REG_FILTER_CTL,
+ ADXL367_FILTER_CTL_RANGE_MASK,
+ FIELD_PREP(ADXL367_FILTER_CTL_RANGE_MASK,
+ range));
+ if (ret)
+ return ret;
- adxl367_scale_act_thresholds(st, st->range, range);
+ adxl367_scale_act_thresholds(st, st->range, range);
- /* Activity thresholds depend on range */
- ret = _adxl367_set_act_threshold(st, ADXL367_ACTIVITY,
- st->act_threshold);
- if (ret)
- return ret;
+ /* Activity thresholds depend on range */
+ ret = _adxl367_set_act_threshold(st, ADXL367_ACTIVITY,
+ st->act_threshold);
+ if (ret)
+ return ret;
- ret = _adxl367_set_act_threshold(st, ADXL367_INACTIVITY,
- st->inact_threshold);
- if (ret)
- return ret;
+ ret = _adxl367_set_act_threshold(st, ADXL367_INACTIVITY,
+ st->inact_threshold);
+ if (ret)
+ return ret;
- ret = adxl367_set_measure_en(st, true);
- if (ret)
- return ret;
+ ret = adxl367_set_measure_en(st, true);
+ if (ret)
+ return ret;
- st->range = range;
+ st->range = range;
- return 0;
- }
- unreachable();
+ return 0;
}
static int adxl367_time_ms_to_samples(struct adxl367_state *st, unsigned int ms)
@@ -620,23 +617,20 @@ static int _adxl367_set_odr(struct adxl367_state *st, enum adxl367_odr odr)
static int adxl367_set_odr(struct iio_dev *indio_dev, enum adxl367_odr odr)
{
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- struct adxl367_state *st = iio_priv(indio_dev);
- int ret;
+ struct adxl367_state *st = iio_priv(indio_dev);
+ int ret;
- guard(mutex)(&st->lock);
+ guard(mutex)(&st->lock);
- ret = adxl367_set_measure_en(st, false);
- if (ret)
- return ret;
+ ret = adxl367_set_measure_en(st, false);
+ if (ret)
+ return ret;
- ret = _adxl367_set_odr(st, odr);
- if (ret)
- return ret;
+ ret = _adxl367_set_odr(st, odr);
+ if (ret)
+ return ret;
- return adxl367_set_measure_en(st, true);
- }
- unreachable();
+ return adxl367_set_measure_en(st, true);
}
static int adxl367_set_temp_adc_en(struct adxl367_state *st, unsigned int reg,
@@ -725,32 +719,29 @@ static int adxl367_read_sample(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val)
{
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- struct adxl367_state *st = iio_priv(indio_dev);
- u16 sample;
- int ret;
+ struct adxl367_state *st = iio_priv(indio_dev);
+ u16 sample;
+ int ret;
- guard(mutex)(&st->lock);
+ guard(mutex)(&st->lock);
- ret = adxl367_set_temp_adc_reg_en(st, chan->address, true);
- if (ret)
- return ret;
+ ret = adxl367_set_temp_adc_reg_en(st, chan->address, true);
+ if (ret)
+ return ret;
- ret = regmap_bulk_read(st->regmap, chan->address, &st->sample_buf,
- sizeof(st->sample_buf));
- if (ret)
- return ret;
+ ret = regmap_bulk_read(st->regmap, chan->address, &st->sample_buf,
+ sizeof(st->sample_buf));
+ if (ret)
+ return ret;
- sample = FIELD_GET(ADXL367_DATA_MASK, be16_to_cpu(st->sample_buf));
- *val = sign_extend32(sample, chan->scan_type.realbits - 1);
+ sample = FIELD_GET(ADXL367_DATA_MASK, be16_to_cpu(st->sample_buf));
+ *val = sign_extend32(sample, chan->scan_type.realbits - 1);
- ret = adxl367_set_temp_adc_reg_en(st, chan->address, false);
- if (ret)
- return ret;
+ ret = adxl367_set_temp_adc_reg_en(st, chan->address, false);
+ if (ret)
+ return ret;
- return IIO_VAL_INT;
- }
- unreachable();
+ return IIO_VAL_INT;
}
static int adxl367_get_status(struct adxl367_state *st, u8 *status,
@@ -852,10 +843,15 @@ static int adxl367_read_raw(struct iio_dev *indio_dev,
int *val, int *val2, long info)
{
struct adxl367_state *st = iio_priv(indio_dev);
+ int ret;
switch (info) {
case IIO_CHAN_INFO_RAW:
- return adxl367_read_sample(indio_dev, chan, val);
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = adxl367_read_sample(indio_dev, chan, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CHAN_INFO_SCALE:
switch (chan->type) {
case IIO_ACCEL: {
@@ -912,7 +908,12 @@ static int adxl367_write_raw(struct iio_dev *indio_dev,
if (ret)
return ret;
- return adxl367_set_odr(indio_dev, odr);
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = adxl367_set_odr(indio_dev, odr);
+ iio_device_release_direct(indio_dev);
+ return ret;
}
case IIO_CHAN_INFO_SCALE: {
enum adxl367_range range;
@@ -921,7 +922,12 @@ static int adxl367_write_raw(struct iio_dev *indio_dev,
if (ret)
return ret;
- return adxl367_set_range(indio_dev, range);
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = adxl367_set_range(indio_dev, range);
+ iio_device_release_direct(indio_dev);
+ return ret;
}
default:
return -EINVAL;
@@ -1069,13 +1075,15 @@ static int adxl367_read_event_config(struct iio_dev *indio_dev,
}
}
-static int adxl367_write_event_config(struct iio_dev *indio_dev,
- const struct iio_chan_spec *chan,
- enum iio_event_type type,
- enum iio_event_direction dir,
- bool state)
+static int __adxl367_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ bool state)
{
+ struct adxl367_state *st = iio_priv(indio_dev);
enum adxl367_activity_type act;
+ int ret;
switch (dir) {
case IIO_EV_DIR_RISING:
@@ -1088,28 +1096,38 @@ static int adxl367_write_event_config(struct iio_dev *indio_dev,
return -EINVAL;
}
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- struct adxl367_state *st = iio_priv(indio_dev);
- int ret;
+ guard(mutex)(&st->lock);
+
+ ret = adxl367_set_measure_en(st, false);
+ if (ret)
+ return ret;
- guard(mutex)(&st->lock);
+ ret = adxl367_set_act_interrupt_en(st, act, state);
+ if (ret)
+ return ret;
- ret = adxl367_set_measure_en(st, false);
- if (ret)
- return ret;
+ ret = adxl367_set_act_en(st, act, state ? ADCL367_ACT_REF_ENABLED
+ : ADXL367_ACT_DISABLED);
+ if (ret)
+ return ret;
- ret = adxl367_set_act_interrupt_en(st, act, state);
- if (ret)
- return ret;
+ return adxl367_set_measure_en(st, true);
+}
- ret = adxl367_set_act_en(st, act, state ? ADCL367_ACT_REF_ENABLED
- : ADXL367_ACT_DISABLED);
- if (ret)
- return ret;
+static int adxl367_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ bool state)
+{
+ int ret;
- return adxl367_set_measure_en(st, true);
- }
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = __adxl367_write_event_config(indio_dev, chan, type, dir, state);
+ iio_device_release_direct(indio_dev);
+ return ret;
}
static ssize_t adxl367_get_fifo_enabled(struct device *dev,
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 09/27] iio: adc: ad4000: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (7 preceding siblings ...)
2025-01-05 17:25 ` [RFC PATCH 08/27] iio: accel: adxl367: " Jonathan Cameron
@ 2025-01-05 17:25 ` Jonathan Cameron
2025-01-06 23:19 ` David Lechner
` (2 more replies)
2025-01-05 17:25 ` [RFC PATCH 10/27] iio: adc: ad4130: " Jonathan Cameron
` (19 subsequent siblings)
28 siblings, 3 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:25 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/ad4000.c | 61 +++++++++++++++++++++++++---------------
1 file changed, 38 insertions(+), 23 deletions(-)
diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
index 1d556a842a68..ef0acaafbcdb 100644
--- a/drivers/iio/adc/ad4000.c
+++ b/drivers/iio/adc/ad4000.c
@@ -535,12 +535,16 @@ static int ad4000_read_raw(struct iio_dev *indio_dev,
int *val2, long info)
{
struct ad4000_state *st = iio_priv(indio_dev);
+ int ret;
switch (info) {
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;
case IIO_CHAN_INFO_SCALE:
*val = st->scale_tbl[st->span_comp][0];
*val2 = st->scale_tbl[st->span_comp][1];
@@ -585,36 +589,47 @@ static int ad4000_write_raw_get_fmt(struct iio_dev *indio_dev,
}
}
-static int ad4000_write_raw(struct iio_dev *indio_dev,
- struct iio_chan_spec const *chan, int val, int val2,
- long mask)
+static int __ad4000_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val2)
{
struct ad4000_state *st = iio_priv(indio_dev);
unsigned int reg_val;
bool span_comp_en;
int ret;
- switch (mask) {
- case IIO_CHAN_INFO_SCALE:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- guard(mutex)(&st->lock);
+ guard(mutex)(&st->lock);
- ret = ad4000_read_reg(st, ®_val);
- if (ret < 0)
- return ret;
+ ret = ad4000_read_reg(st, ®_val);
+ if (ret < 0)
+ return ret;
- span_comp_en = val2 == st->scale_tbl[1][1];
- reg_val &= ~AD4000_CFG_SPAN_COMP;
- reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP, span_comp_en);
+ span_comp_en = val2 == st->scale_tbl[1][1];
+ reg_val &= ~AD4000_CFG_SPAN_COMP;
+ reg_val |= FIELD_PREP(AD4000_CFG_SPAN_COMP, span_comp_en);
- ret = ad4000_write_reg(st, reg_val);
- if (ret < 0)
- return ret;
+ ret = ad4000_write_reg(st, reg_val);
+ if (ret < 0)
+ return ret;
- st->span_comp = span_comp_en;
- return 0;
- }
- unreachable();
+ st->span_comp = span_comp_en;
+ return 0;
+}
+
+static int ad4000_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = __ad4000_write_raw(indio_dev, chan, val2);
+ iio_device_release_direct(indio_dev);
+ return ret;
default:
return -EINVAL;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [RFC PATCH 09/27] iio: adc: ad4000: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 ` [RFC PATCH 09/27] iio: adc: ad4000: " Jonathan Cameron
@ 2025-01-06 23:19 ` David Lechner
2025-01-07 11:29 ` Marcelo Schmitt
2025-01-29 16:41 ` Marcelo Schmitt
2 siblings, 0 replies; 49+ messages in thread
From: David Lechner @ 2025-01-06 23:19 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio, “Luc Van Oostenryck”
Cc: Jonathan Cameron
On 1/5/25 11:25 AM, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> This complex cleanup.h use case of conditional guards has proved
> to be more trouble that it is worth in terms of false positive compiler
> warnings and hard to read code.
>
> Move directly to the new claim/release_direct() that allow sparse
> to check for unbalanced context.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/iio/adc/ad4000.c | 61 +++++++++++++++++++++++++---------------
> 1 file changed, 38 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/iio/adc/ad4000.c b/drivers/iio/adc/ad4000.c
> index 1d556a842a68..ef0acaafbcdb 100644
...
> +static int ad4000_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> +
Spurious blank line added.
> + if (!iio_device_claim_direct(indio_dev))
> + return -EBUSY;
> + ret = __ad4000_write_raw(indio_dev, chan, val2);
> + iio_device_release_direct(indio_dev);
> + return ret;
> default:
> return -EINVAL;
> }
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [RFC PATCH 09/27] iio: adc: ad4000: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 ` [RFC PATCH 09/27] iio: adc: ad4000: " Jonathan Cameron
2025-01-06 23:19 ` David Lechner
@ 2025-01-07 11:29 ` Marcelo Schmitt
2025-01-07 14:28 ` Jonathan Cameron
2025-01-29 16:41 ` Marcelo Schmitt
2 siblings, 1 reply; 49+ messages in thread
From: Marcelo Schmitt @ 2025-01-07 11:29 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, “Luc Van Oostenryck”, David Lechner,
Jonathan Cameron
On 01/05, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> This complex cleanup.h use case of conditional guards has proved
> to be more trouble that it is worth in terms of false positive compiler
> warnings and hard to read code.
>
> Move directly to the new claim/release_direct() that allow sparse
> to check for unbalanced context.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/iio/adc/ad4000.c | 61 +++++++++++++++++++++++++---------------
> 1 file changed, 38 insertions(+), 23 deletions(-)
>
Hi Jonathan, aside from the spurious blank line noted by David, the changes for
ad4000 look good to me.
Acked-by: <marcelo.schmitt1@gmail.com>
I also tried running Sparse on IIO subsystem but didn't see any warns for the
drivers being changed (nor prior nor after applying the patches).
make CHECK="path_to_local_sparse_v0.6.4-66-g0196afe1" C=2 drivers/iio/
Did see warns after adding incorrect type in assignments in the driver.
Mind sharing how you are running Sparse?
Thanks,
Marcelo
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [RFC PATCH 09/27] iio: adc: ad4000: Stop using iio_device_claim_direct_scoped()
2025-01-07 11:29 ` Marcelo Schmitt
@ 2025-01-07 14:28 ` Jonathan Cameron
2025-01-11 13:37 ` Jonathan Cameron
0 siblings, 1 reply; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-07 14:28 UTC (permalink / raw)
To: Marcelo Schmitt
Cc: Jonathan Cameron, linux-iio, “Luc Van Oostenryck”,
David Lechner
On Tue, 7 Jan 2025 08:29:36 -0300
Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
> On 01/05, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > This complex cleanup.h use case of conditional guards has proved
> > to be more trouble that it is worth in terms of false positive compiler
> > warnings and hard to read code.
> >
> > Move directly to the new claim/release_direct() that allow sparse
> > to check for unbalanced context.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> > drivers/iio/adc/ad4000.c | 61 +++++++++++++++++++++++++---------------
> > 1 file changed, 38 insertions(+), 23 deletions(-)
> >
> Hi Jonathan, aside from the spurious blank line noted by David, the changes for
> ad4000 look good to me.
>
> Acked-by: <marcelo.schmitt1@gmail.com>
>
> I also tried running Sparse on IIO subsystem but didn't see any warns for the
> drivers being changed (nor prior nor after applying the patches).
>
> make CHECK="path_to_local_sparse_v0.6.4-66-g0196afe1" C=2 drivers/iio/
>
> Did see warns after adding incorrect type in assignments in the driver.
>
> Mind sharing how you are running Sparse?
I just used C=1 but that doesn't really matter for this.
With this series there should be no false positive warnings (or before
it where we didn't have any markings so sparse didn't know to do anything).
Testing wise, I sprinkled in some early returns, breaks etc to add
some broken paths and those triggered context imbalance warnings.
This isn't fixing warnings, it is just about moving to code where we
will get them if we do something silly in the future.
Jonathan
>
> Thanks,
> Marcelo
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [RFC PATCH 09/27] iio: adc: ad4000: Stop using iio_device_claim_direct_scoped()
2025-01-07 14:28 ` Jonathan Cameron
@ 2025-01-11 13:37 ` Jonathan Cameron
2025-01-19 18:18 ` Marcelo Schmitt
0 siblings, 1 reply; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-11 13:37 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Marcelo Schmitt, linux-iio, “Luc Van Oostenryck”,
David Lechner
On Tue, 7 Jan 2025 14:28:54 +0000
Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
> On Tue, 7 Jan 2025 08:29:36 -0300
> Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
>
> > On 01/05, Jonathan Cameron wrote:
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > This complex cleanup.h use case of conditional guards has proved
> > > to be more trouble that it is worth in terms of false positive compiler
> > > warnings and hard to read code.
> > >
> > > Move directly to the new claim/release_direct() that allow sparse
> > > to check for unbalanced context.
> > >
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > ---
> > > drivers/iio/adc/ad4000.c | 61 +++++++++++++++++++++++++---------------
> > > 1 file changed, 38 insertions(+), 23 deletions(-)
> > >
> > Hi Jonathan, aside from the spurious blank line noted by David, the changes for
> > ad4000 look good to me.
> >
> > Acked-by: <marcelo.schmitt1@gmail.com>
> >
> > I also tried running Sparse on IIO subsystem but didn't see any warns for the
> > drivers being changed (nor prior nor after applying the patches).
> >
> > make CHECK="path_to_local_sparse_v0.6.4-66-g0196afe1" C=2 drivers/iio/
> >
> > Did see warns after adding incorrect type in assignments in the driver.
> >
> > Mind sharing how you are running Sparse?
>
> I just used C=1 but that doesn't really matter for this.
> With this series there should be no false positive warnings (or before
> it where we didn't have any markings so sparse didn't know to do anything).
>
> Testing wise, I sprinkled in some early returns, breaks etc to add
> some broken paths and those triggered context imbalance warnings.
>
> This isn't fixing warnings, it is just about moving to code where we
> will get them if we do something silly in the future.
Seems David is also not seeing warnings when he deliberately breaks
the code. See discussion on patch 1. Hopefully we'll soon get to the
bottom of why!
Jonathan
>
> Jonathan
>
> >
> > Thanks,
> > Marcelo
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [RFC PATCH 09/27] iio: adc: ad4000: Stop using iio_device_claim_direct_scoped()
2025-01-11 13:37 ` Jonathan Cameron
@ 2025-01-19 18:18 ` Marcelo Schmitt
0 siblings, 0 replies; 49+ messages in thread
From: Marcelo Schmitt @ 2025-01-19 18:18 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, linux-iio, “Luc Van Oostenryck”,
David Lechner
Hi Jonathan,
Thanks for clarifying the intent of the series and for the hints on how to test
the patches. I think we will need a v2. Didn't look through all drivers being
updated but most of the ones I looked at had a bugy check of
iio_device_claim_direct() return. Please see my comments to the cover letter.
Thanks,
Marcelo
On 01/11, Jonathan Cameron wrote:
> On Tue, 7 Jan 2025 14:28:54 +0000
> Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote:
>
> > On Tue, 7 Jan 2025 08:29:36 -0300
> > Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote:
> >
> > > On 01/05, Jonathan Cameron wrote:
> > > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > >
> > > > This complex cleanup.h use case of conditional guards has proved
> > > > to be more trouble that it is worth in terms of false positive compiler
> > > > warnings and hard to read code.
> > > >
> > > > Move directly to the new claim/release_direct() that allow sparse
> > > > to check for unbalanced context.
> > > >
> > > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > ---
> > > > drivers/iio/adc/ad4000.c | 61 +++++++++++++++++++++++++---------------
> > > > 1 file changed, 38 insertions(+), 23 deletions(-)
> > > >
> > > Hi Jonathan, aside from the spurious blank line noted by David, the changes for
> > > ad4000 look good to me.
> > >
> > > Acked-by: <marcelo.schmitt1@gmail.com>
> > >
> > > I also tried running Sparse on IIO subsystem but didn't see any warns for the
> > > drivers being changed (nor prior nor after applying the patches).
> > >
> > > make CHECK="path_to_local_sparse_v0.6.4-66-g0196afe1" C=2 drivers/iio/
> > >
> > > Did see warns after adding incorrect type in assignments in the driver.
> > >
> > > Mind sharing how you are running Sparse?
> >
> > I just used C=1 but that doesn't really matter for this.
> > With this series there should be no false positive warnings (or before
> > it where we didn't have any markings so sparse didn't know to do anything).
> >
> > Testing wise, I sprinkled in some early returns, breaks etc to add
> > some broken paths and those triggered context imbalance warnings.
> >
> > This isn't fixing warnings, it is just about moving to code where we
> > will get them if we do something silly in the future.
>
> Seems David is also not seeing warnings when he deliberately breaks
> the code. See discussion on patch 1. Hopefully we'll soon get to the
> bottom of why!
>
> Jonathan
>
> >
> > Jonathan
> >
> > >
> > > Thanks,
> > > Marcelo
> >
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [RFC PATCH 09/27] iio: adc: ad4000: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 ` [RFC PATCH 09/27] iio: adc: ad4000: " Jonathan Cameron
2025-01-06 23:19 ` David Lechner
2025-01-07 11:29 ` Marcelo Schmitt
@ 2025-01-29 16:41 ` Marcelo Schmitt
2 siblings, 0 replies; 49+ messages in thread
From: Marcelo Schmitt @ 2025-01-29 16:41 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, “Luc Van Oostenryck”, David Lechner,
Jonathan Cameron
Hi Jonathan,
If you end up not changing this patch much, please consider taking my tags.
On 01/05, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> This complex cleanup.h use case of conditional guards has proved
> to be more trouble that it is worth in terms of false positive compiler
> warnings and hard to read code.
>
> Move directly to the new claim/release_direct() that allow sparse
> to check for unbalanced context.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
Tested-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
^ permalink raw reply [flat|nested] 49+ messages in thread
* [RFC PATCH 10/27] iio: adc: ad4130: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (8 preceding siblings ...)
2025-01-05 17:25 ` [RFC PATCH 09/27] iio: adc: ad4000: " Jonathan Cameron
@ 2025-01-05 17:25 ` Jonathan Cameron
2025-01-05 17:25 ` [RFC PATCH 11/27] " Jonathan Cameron
` (18 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:25 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/ad4130.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/adc/ad4130.c b/drivers/iio/adc/ad4130.c
index de32cc9d18c5..09012f33ff16 100644
--- a/drivers/iio/adc/ad4130.c
+++ b/drivers/iio/adc/ad4130.c
@@ -1060,13 +1060,11 @@ static int _ad4130_read_sample(struct iio_dev *indio_dev, unsigned int channel,
static int ad4130_read_sample(struct iio_dev *indio_dev, unsigned int channel,
int *val)
{
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- struct ad4130_state *st = iio_priv(indio_dev);
+ struct ad4130_state *st = iio_priv(indio_dev);
- guard(mutex)(&st->lock);
- return _ad4130_read_sample(indio_dev, channel, val);
- }
- unreachable();
+ guard(mutex)(&st->lock);
+
+ return _ad4130_read_sample(indio_dev, channel, val);
}
static int ad4130_read_raw(struct iio_dev *indio_dev,
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 11/27] iio: adc: ad4130: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (9 preceding siblings ...)
2025-01-05 17:25 ` [RFC PATCH 10/27] iio: adc: ad4130: " Jonathan Cameron
@ 2025-01-05 17:25 ` Jonathan Cameron
2025-01-05 17:25 ` [RFC PATCH 12/27] iio: adc: ad4695: " Jonathan Cameron
` (17 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:25 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/ad4130.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ad4130.c b/drivers/iio/adc/ad4130.c
index 09012f33ff16..1a38b52abbe4 100644
--- a/drivers/iio/adc/ad4130.c
+++ b/drivers/iio/adc/ad4130.c
@@ -1074,10 +1074,16 @@ static int ad4130_read_raw(struct iio_dev *indio_dev,
struct ad4130_state *st = iio_priv(indio_dev);
unsigned int channel = chan->scan_index;
struct ad4130_setup_info *setup_info = &st->chans_info[channel].setup;
+ int ret;
switch (info) {
case IIO_CHAN_INFO_RAW:
- return ad4130_read_sample(indio_dev, channel, val);
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = ad4130_read_sample(indio_dev, channel, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CHAN_INFO_SCALE: {
guard(mutex)(&st->lock);
*val = st->scale_tbls[setup_info->ref_sel][setup_info->pga][0];
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 12/27] iio: adc: ad4695: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (10 preceding siblings ...)
2025-01-05 17:25 ` [RFC PATCH 11/27] " Jonathan Cameron
@ 2025-01-05 17:25 ` Jonathan Cameron
2025-01-05 17:25 ` [RFC PATCH 13/27] iio: adc: ad7606: " Jonathan Cameron
` (16 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:25 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context. In some cases code is factored
out to utility functions that can do a direect return with the
claim and release around the call.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/ad4695.c | 240 ++++++++++++++++++++++-----------------
1 file changed, 133 insertions(+), 107 deletions(-)
diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
index 3c2c01289fda..e7bf4870099c 100644
--- a/drivers/iio/adc/ad4695.c
+++ b/drivers/iio/adc/ad4695.c
@@ -617,6 +617,25 @@ static int ad4695_read_one_sample(struct ad4695_state *st, unsigned int address)
return spi_sync_transfer(st->spi, xfer, i + 1);
}
+static int __ad4695_read_info_raw(struct ad4695_state *st,
+ struct iio_chan_spec const *chan,
+ int *val)
+{
+ u8 realbits = chan->scan_type.realbits;
+ int ret;
+
+ ret = ad4695_read_one_sample(st, chan->address);
+ if (ret)
+ return ret;
+
+ if (chan->scan_type.sign == 's')
+ *val = sign_extend32(st->raw_data, realbits - 1);
+ else
+ *val = st->raw_data;
+
+ return IIO_VAL_INT;
+}
+
static int ad4695_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
@@ -629,19 +648,12 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- ret = ad4695_read_one_sample(st, chan->address);
- if (ret)
- return ret;
-
- if (chan->scan_type.sign == 's')
- *val = sign_extend32(st->raw_data, realbits - 1);
- else
- *val = st->raw_data;
- return IIO_VAL_INT;
- }
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = __ad4695_read_info_raw(st, chan, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CHAN_INFO_SCALE:
switch (chan->type) {
case IIO_VOLTAGE:
@@ -679,45 +691,45 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
case IIO_CHAN_INFO_CALIBSCALE:
switch (chan->type) {
case IIO_VOLTAGE:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- ret = regmap_read(st->regmap16,
- AD4695_REG_GAIN_IN(chan->scan_index),
- ®_val);
- if (ret)
- return ret;
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = regmap_read(st->regmap16,
+ AD4695_REG_GAIN_IN(chan->scan_index),
+ ®_val);
+ iio_device_release_direct(indio_dev);
+ if (ret)
+ return ret;
- *val = reg_val;
- *val2 = 15;
+ *val = reg_val;
+ *val2 = 15;
- return IIO_VAL_FRACTIONAL_LOG2;
- }
- unreachable();
+ return IIO_VAL_FRACTIONAL_LOG2;
default:
return -EINVAL;
}
case IIO_CHAN_INFO_CALIBBIAS:
switch (chan->type) {
case IIO_VOLTAGE:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- ret = regmap_read(st->regmap16,
- AD4695_REG_OFFSET_IN(chan->scan_index),
- ®_val);
- if (ret)
- return ret;
-
- tmp = sign_extend32(reg_val, 15);
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = regmap_read(st->regmap16,
+ AD4695_REG_OFFSET_IN(chan->scan_index),
+ ®_val);
+ iio_device_release_direct(indio_dev);
+ if (ret)
+ return ret;
- *val = tmp / 4;
- *val2 = abs(tmp) % 4 * MICRO / 4;
+ tmp = sign_extend32(reg_val, 15);
- if (tmp < 0 && *val2) {
- *val *= -1;
- *val2 *= -1;
- }
+ *val = tmp / 4;
+ *val2 = abs(tmp) % 4 * MICRO / 4;
- return IIO_VAL_INT_PLUS_MICRO;
+ if (tmp < 0 && *val2) {
+ *val *= -1;
+ *val2 *= -1;
}
- unreachable();
+
+ return IIO_VAL_INT_PLUS_MICRO;
default:
return -EINVAL;
}
@@ -726,64 +738,75 @@ static int ad4695_read_raw(struct iio_dev *indio_dev,
}
}
-static int ad4695_write_raw(struct iio_dev *indio_dev,
- struct iio_chan_spec const *chan,
- int val, int val2, long mask)
+static int __ad4695_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
{
struct ad4695_state *st = iio_priv(indio_dev);
unsigned int reg_val;
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- switch (mask) {
- case IIO_CHAN_INFO_CALIBSCALE:
- switch (chan->type) {
- case IIO_VOLTAGE:
- if (val < 0 || val2 < 0)
- reg_val = 0;
- else if (val > 1)
- reg_val = U16_MAX;
- else
- reg_val = (val * (1 << 16) +
- mul_u64_u32_div(val2, 1 << 16,
- MICRO)) / 2;
-
- return regmap_write(st->regmap16,
- AD4695_REG_GAIN_IN(chan->scan_index),
- reg_val);
- default:
- return -EINVAL;
- }
- case IIO_CHAN_INFO_CALIBBIAS:
- switch (chan->type) {
- case IIO_VOLTAGE:
- if (val2 >= 0 && val > S16_MAX / 4)
- reg_val = S16_MAX;
- else if ((val2 < 0 ? -val : val) < S16_MIN / 4)
- reg_val = S16_MIN;
- else if (val2 < 0)
- reg_val = clamp_t(int,
- -(val * 4 + -val2 * 4 / MICRO),
- S16_MIN, S16_MAX);
- else if (val < 0)
- reg_val = clamp_t(int,
- val * 4 - val2 * 4 / MICRO,
- S16_MIN, S16_MAX);
- else
- reg_val = clamp_t(int,
- val * 4 + val2 * 4 / MICRO,
- S16_MIN, S16_MAX);
-
- return regmap_write(st->regmap16,
- AD4695_REG_OFFSET_IN(chan->scan_index),
- reg_val);
- default:
- return -EINVAL;
- }
+ switch (mask) {
+ case IIO_CHAN_INFO_CALIBSCALE:
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ if (val < 0 || val2 < 0)
+ reg_val = 0;
+ else if (val > 1)
+ reg_val = U16_MAX;
+ else
+ reg_val = (val * (1 << 16) +
+ mul_u64_u32_div(val2, 1 << 16,
+ MICRO)) / 2;
+
+ return regmap_write(st->regmap16,
+ AD4695_REG_GAIN_IN(chan->scan_index),
+ reg_val);
+ default:
+ return -EINVAL;
+ }
+ case IIO_CHAN_INFO_CALIBBIAS:
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ if (val2 >= 0 && val > S16_MAX / 4)
+ reg_val = S16_MAX;
+ else if ((val2 < 0 ? -val : val) < S16_MIN / 4)
+ reg_val = S16_MIN;
+ else if (val2 < 0)
+ reg_val = clamp_t(int,
+ -(val * 4 + -val2 * 4 / MICRO),
+ S16_MIN, S16_MAX);
+ else if (val < 0)
+ reg_val = clamp_t(int,
+ val * 4 - val2 * 4 / MICRO,
+ S16_MIN, S16_MAX);
+ else
+ reg_val = clamp_t(int,
+ val * 4 + val2 * 4 / MICRO,
+ S16_MIN, S16_MAX);
+
+ return regmap_write(st->regmap16,
+ AD4695_REG_OFFSET_IN(chan->scan_index),
+ reg_val);
default:
return -EINVAL;
}
+ default:
+ return -EINVAL;
}
- unreachable();
+}
+
+static int ad4695_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ int ret;
+
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = __ad4695_write_raw(indio_dev, chan, val, val2, mask);
+ iio_device_release_direct(indio_dev);
+
+ return ret;
}
static int ad4695_read_avail(struct iio_dev *indio_dev,
@@ -833,26 +856,29 @@ static int ad4695_debugfs_reg_access(struct iio_dev *indio_dev,
unsigned int *readval)
{
struct ad4695_state *st = iio_priv(indio_dev);
-
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- if (readval) {
- if (regmap_check_range_table(st->regmap, reg,
- &ad4695_regmap_rd_table))
- return regmap_read(st->regmap, reg, readval);
- if (regmap_check_range_table(st->regmap16, reg,
- &ad4695_regmap16_rd_table))
- return regmap_read(st->regmap16, reg, readval);
- } else {
- if (regmap_check_range_table(st->regmap, reg,
- &ad4695_regmap_wr_table))
- return regmap_write(st->regmap, reg, writeval);
- if (regmap_check_range_table(st->regmap16, reg,
- &ad4695_regmap16_wr_table))
- return regmap_write(st->regmap16, reg, writeval);
- }
+ int ret = -EINVAL;
+
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ if (readval) {
+ if (regmap_check_range_table(st->regmap, reg,
+ &ad4695_regmap_rd_table))
+ ret = regmap_read(st->regmap, reg, readval);
+ if (regmap_check_range_table(st->regmap16, reg,
+ &ad4695_regmap16_rd_table))
+ ret = regmap_read(st->regmap16, reg, readval);
+ } else {
+ if (regmap_check_range_table(st->regmap, reg,
+ &ad4695_regmap_wr_table))
+ ret = regmap_write(st->regmap, reg, writeval);
+ if (regmap_check_range_table(st->regmap16, reg,
+ &ad4695_regmap16_wr_table))
+ ret = regmap_write(st->regmap16, reg, writeval);
}
+ iio_device_release_direct(indio_dev);
- return -EINVAL;
+ return ret;
}
static const struct iio_info ad4695_info = {
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 13/27] iio: adc: ad7606: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (11 preceding siblings ...)
2025-01-05 17:25 ` [RFC PATCH 12/27] iio: adc: ad4695: " Jonathan Cameron
@ 2025-01-05 17:25 ` Jonathan Cameron
2025-01-05 17:25 ` [RFC PATCH 14/27] iio: adc: ad7625: " Jonathan Cameron
` (15 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:25 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/ad7606.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/adc/ad7606.c b/drivers/iio/adc/ad7606.c
index e35d55d03d86..1579cf100569 100644
--- a/drivers/iio/adc/ad7606.c
+++ b/drivers/iio/adc/ad7606.c
@@ -744,13 +744,13 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
switch (m) {
case IIO_CHAN_INFO_RAW:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- ret = ad7606_scan_direct(indio_dev, chan->address, val);
- if (ret < 0)
- return ret;
- return IIO_VAL_INT;
- }
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = ad7606_scan_direct(indio_dev, chan->address, val);
+ iio_device_release_direct(indio_dev);
+ if (ret < 0)
+ return ret;
+ return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
if (st->sw_mode_en)
ch = chan->address;
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 14/27] iio: adc: ad7625: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (12 preceding siblings ...)
2025-01-05 17:25 ` [RFC PATCH 13/27] iio: adc: ad7606: " Jonathan Cameron
@ 2025-01-05 17:25 ` Jonathan Cameron
2025-01-05 17:26 ` [RFC PATCH 15/27] iio: adc: ad7779: " Jonathan Cameron
` (14 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:25 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/ad7625.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/adc/ad7625.c b/drivers/iio/adc/ad7625.c
index afa9bf4ddf3c..a39c970450a2 100644
--- a/drivers/iio/adc/ad7625.c
+++ b/drivers/iio/adc/ad7625.c
@@ -248,12 +248,15 @@ static int ad7625_write_raw(struct iio_dev *indio_dev,
int val, int val2, long info)
{
struct ad7625_state *st = iio_priv(indio_dev);
+ int ret;
switch (info) {
case IIO_CHAN_INFO_SAMP_FREQ:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
- return ad7625_set_sampling_freq(st, val);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = ad7625_set_sampling_freq(st, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
default:
return -EINVAL;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 15/27] iio: adc: ad7779: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (13 preceding siblings ...)
2025-01-05 17:25 ` [RFC PATCH 14/27] iio: adc: ad7625: " Jonathan Cameron
@ 2025-01-05 17:26 ` Jonathan Cameron
2025-01-05 17:26 ` [RFC PATCH 16/27] iio: adc: ad9467: " Jonathan Cameron
` (13 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:26 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/ad7779.c | 103 ++++++++++++++++++++++++---------------
1 file changed, 63 insertions(+), 40 deletions(-)
diff --git a/drivers/iio/adc/ad7779.c b/drivers/iio/adc/ad7779.c
index 2537dab69a35..3b132665d24c 100644
--- a/drivers/iio/adc/ad7779.c
+++ b/drivers/iio/adc/ad7779.c
@@ -467,65 +467,88 @@ static int ad7779_set_calibbias(struct ad7779_state *st, int channel, int val)
calibbias[2]);
}
+static int __ad7779_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long mask)
+{
+ struct ad7779_state *st = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_CALIBSCALE:
+ ret = ad7779_get_calibscale(st, chan->channel);
+ if (ret < 0)
+ return ret;
+ *val = ret;
+ *val2 = GAIN_REL;
+ return IIO_VAL_FRACTIONAL;
+ case IIO_CHAN_INFO_CALIBBIAS:
+ ret = ad7779_get_calibbias(st, chan->channel);
+ if (ret < 0)
+ return ret;
+ *val = ret;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *val = st->sampling_freq;
+ if (*val < 0)
+ return -EINVAL;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
static int ad7779_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan, int *val,
int *val2, long mask)
{
- struct ad7779_state *st = iio_priv(indio_dev);
int ret;
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- switch (mask) {
- case IIO_CHAN_INFO_CALIBSCALE:
- ret = ad7779_get_calibscale(st, chan->channel);
- if (ret < 0)
- return ret;
- *val = ret;
- *val2 = GAIN_REL;
- return IIO_VAL_FRACTIONAL;
- case IIO_CHAN_INFO_CALIBBIAS:
- ret = ad7779_get_calibbias(st, chan->channel);
- if (ret < 0)
- return ret;
- *val = ret;
- return IIO_VAL_INT;
- case IIO_CHAN_INFO_SAMP_FREQ:
- *val = st->sampling_freq;
- if (*val < 0)
- return -EINVAL;
- return IIO_VAL_INT;
- default:
- return -EINVAL;
- }
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = __ad7779_read_raw(indio_dev, chan, val, val2, mask);
+ iio_device_release_direct(indio_dev);
+ return ret;
+}
+
+static int __ad7779_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2,
+ long mask)
+{
+ struct ad7779_state *st = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_CALIBSCALE:
+ return ad7779_set_calibscale(st, chan->channel, val2);
+ case IIO_CHAN_INFO_CALIBBIAS:
+ return ad7779_set_calibbias(st, chan->channel, val);
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ return ad7779_set_sampling_frequency(st, val);
+ default:
+ return -EINVAL;
}
- unreachable();
}
static int ad7779_write_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan, int val, int val2,
long mask)
{
- struct ad7779_state *st = iio_priv(indio_dev);
+ int ret;
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- switch (mask) {
- case IIO_CHAN_INFO_CALIBSCALE:
- return ad7779_set_calibscale(st, chan->channel, val2);
- case IIO_CHAN_INFO_CALIBBIAS:
- return ad7779_set_calibbias(st, chan->channel, val);
- case IIO_CHAN_INFO_SAMP_FREQ:
- return ad7779_set_sampling_frequency(st, val);
- default:
- return -EINVAL;
- }
- }
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = __ad7779_write_raw(indio_dev, chan, val, val2, mask);
+ iio_device_release_direct(indio_dev);
+ return ret;
}
static int ad7779_buffer_preenable(struct iio_dev *indio_dev)
{
- int ret;
struct ad7779_state *st = iio_priv(indio_dev);
+ int ret;
ret = ad7779_spi_write_mask(st,
AD7779_REG_GENERAL_USER_CONFIG_3,
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 16/27] iio: adc: ad9467: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (14 preceding siblings ...)
2025-01-05 17:26 ` [RFC PATCH 15/27] iio: adc: ad7779: " Jonathan Cameron
@ 2025-01-05 17:26 ` Jonathan Cameron
2025-01-05 17:26 ` [RFC PATCH 17/27] iio: adc: max1363: " Jonathan Cameron
` (12 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:26 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/ad9467.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/adc/ad9467.c b/drivers/iio/adc/ad9467.c
index d358958ab310..635c0d10c7bc 100644
--- a/drivers/iio/adc/ad9467.c
+++ b/drivers/iio/adc/ad9467.c
@@ -813,6 +813,18 @@ static int ad9467_read_raw(struct iio_dev *indio_dev,
}
}
+static int __ad9467_update_clock(struct ad9467_state *st, long r_clk)
+{
+ int ret;
+
+ ret = clk_set_rate(st->clk, r_clk);
+ if (ret)
+ return ret;
+
+ guard(mutex)(&st->lock);
+ return ad9467_calibrate(st);
+}
+
static int ad9467_write_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int val, int val2, long mask)
@@ -842,14 +854,11 @@ static int ad9467_write_raw(struct iio_dev *indio_dev,
if (sample_rate == r_clk)
return 0;
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- ret = clk_set_rate(st->clk, r_clk);
- if (ret)
- return ret;
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
- guard(mutex)(&st->lock);
- ret = ad9467_calibrate(st);
- }
+ ret = __ad9467_update_clock(st, r_clk);
+ iio_device_release_direct(indio_dev);
return ret;
default:
return -EINVAL;
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 17/27] iio: adc: max1363: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (15 preceding siblings ...)
2025-01-05 17:26 ` [RFC PATCH 16/27] iio: adc: ad9467: " Jonathan Cameron
@ 2025-01-05 17:26 ` Jonathan Cameron
2025-01-05 17:26 ` [RFC PATCH 18/27] iio: adc: rtq6056: " Jonathan Cameron
` (11 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:26 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/max1363.c | 165 ++++++++++++++++++++------------------
1 file changed, 89 insertions(+), 76 deletions(-)
diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index e8d731bc34e0..35717ec082ce 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -364,55 +364,52 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
int *val,
long m)
{
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- s32 data;
- u8 rxbuf[2];
- struct max1363_state *st = iio_priv(indio_dev);
- struct i2c_client *client = st->client;
-
- guard(mutex)(&st->lock);
-
- /*
- * If monitor mode is enabled, the method for reading a single
- * channel will have to be rather different and has not yet
- * been implemented.
- *
- * Also, cannot read directly if buffered capture enabled.
- */
- if (st->monitor_on)
- return -EBUSY;
+ s32 data;
+ u8 rxbuf[2];
+ struct max1363_state *st = iio_priv(indio_dev);
+ struct i2c_client *client = st->client;
- /* Check to see if current scan mode is correct */
- if (st->current_mode != &max1363_mode_table[chan->address]) {
- int ret;
+ guard(mutex)(&st->lock);
- /* Update scan mode if needed */
- st->current_mode = &max1363_mode_table[chan->address];
- ret = max1363_set_scan_mode(st);
- if (ret < 0)
- return ret;
- }
- if (st->chip_info->bits != 8) {
- /* Get reading */
- data = st->recv(client, rxbuf, 2);
- if (data < 0)
- return data;
-
- data = get_unaligned_be16(rxbuf) &
- ((1 << st->chip_info->bits) - 1);
- } else {
- /* Get reading */
- data = st->recv(client, rxbuf, 1);
- if (data < 0)
- return data;
-
- data = rxbuf[0];
- }
- *val = data;
+ /*
+ * If monitor mode is enabled, the method for reading a single
+ * channel will have to be rather different and has not yet
+ * been implemented.
+ *
+ * Also, cannot read directly if buffered capture enabled.
+ */
+ if (st->monitor_on)
+ return -EBUSY;
+
+ /* Check to see if current scan mode is correct */
+ if (st->current_mode != &max1363_mode_table[chan->address]) {
+ int ret;
+
+ /* Update scan mode if needed */
+ st->current_mode = &max1363_mode_table[chan->address];
+ ret = max1363_set_scan_mode(st);
+ if (ret < 0)
+ return ret;
+ }
+ if (st->chip_info->bits != 8) {
+ /* Get reading */
+ data = st->recv(client, rxbuf, 2);
+ if (data < 0)
+ return data;
+
+ data = get_unaligned_be16(rxbuf) &
+ ((1 << st->chip_info->bits) - 1);
+ } else {
+ /* Get reading */
+ data = st->recv(client, rxbuf, 1);
+ if (data < 0)
+ return data;
- return 0;
+ data = rxbuf[0];
}
- unreachable();
+ *val = data;
+
+ return 0;
}
static int max1363_read_raw(struct iio_dev *indio_dev,
@@ -426,7 +423,11 @@ static int max1363_read_raw(struct iio_dev *indio_dev,
switch (m) {
case IIO_CHAN_INFO_RAW:
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
ret = max1363_read_single_chan(indio_dev, chan, val, m);
+ iio_device_release_direct(indio_dev);
if (ret < 0)
return ret;
return IIO_VAL_INT;
@@ -947,46 +948,58 @@ static inline int __max1363_check_event_mask(int thismask, int checkmask)
return ret;
}
-static int max1363_write_event_config(struct iio_dev *indio_dev,
- const struct iio_chan_spec *chan, enum iio_event_type type,
+static int __max1363_write_event_config(struct max1363_state *st,
+ const struct iio_chan_spec *chan,
enum iio_event_direction dir, bool state)
{
- struct max1363_state *st = iio_priv(indio_dev);
-
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- int number = chan->channel;
- u16 unifiedmask;
- int ret;
+ int number = chan->channel;
+ u16 unifiedmask;
+ int ret;
- guard(mutex)(&st->lock);
+ guard(mutex)(&st->lock);
- unifiedmask = st->mask_low | st->mask_high;
- if (dir == IIO_EV_DIR_FALLING) {
+ unifiedmask = st->mask_low | st->mask_high;
+ if (dir == IIO_EV_DIR_FALLING) {
- if (state == 0)
- st->mask_low &= ~(1 << number);
- else {
- ret = __max1363_check_event_mask((1 << number),
- unifiedmask);
- if (ret)
- return ret;
- st->mask_low |= (1 << number);
- }
- } else {
- if (state == 0)
- st->mask_high &= ~(1 << number);
- else {
- ret = __max1363_check_event_mask((1 << number),
- unifiedmask);
- if (ret)
- return ret;
- st->mask_high |= (1 << number);
- }
+ if (state == 0)
+ st->mask_low &= ~(1 << number);
+ else {
+ ret = __max1363_check_event_mask((1 << number),
+ unifiedmask);
+ if (ret)
+ return ret;
+ st->mask_low |= (1 << number);
+ }
+ } else {
+ if (state == 0)
+ st->mask_high &= ~(1 << number);
+ else {
+ ret = __max1363_check_event_mask((1 << number),
+ unifiedmask);
+ if (ret)
+ return ret;
+ st->mask_high |= (1 << number);
}
}
- max1363_monitor_mode_update(st, !!(st->mask_high | st->mask_low));
return 0;
+
+}
+static int max1363_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan, enum iio_event_type type,
+ enum iio_event_direction dir, bool state)
+{
+ struct max1363_state *st = iio_priv(indio_dev);
+ int ret;
+
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = __max1363_write_event_config(st, chan, dir, state);
+ iio_device_release_direct(indio_dev);
+ max1363_monitor_mode_update(st, !!(st->mask_high | st->mask_low));
+
+ return ret;
}
/*
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 18/27] iio: adc: rtq6056: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (16 preceding siblings ...)
2025-01-05 17:26 ` [RFC PATCH 17/27] iio: adc: max1363: " Jonathan Cameron
@ 2025-01-05 17:26 ` Jonathan Cameron
2025-01-05 17:26 ` [RFC PATCH 19/27] iio: adc: ti-adc161s626: " Jonathan Cameron
` (10 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:26 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/rtq6056.c | 39 +++++++++++++++++++++++++--------------
1 file changed, 25 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/adc/rtq6056.c b/drivers/iio/adc/rtq6056.c
index 337bc8b31b2c..4cb9add4682f 100644
--- a/drivers/iio/adc/rtq6056.c
+++ b/drivers/iio/adc/rtq6056.c
@@ -514,26 +514,37 @@ static int rtq6056_adc_read_avail(struct iio_dev *indio_dev,
}
}
-static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
- struct iio_chan_spec const *chan, int val,
- int val2, long mask)
+static int __rtq6056_adc_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int val,
+ long mask)
{
struct rtq6056_priv *priv = iio_priv(indio_dev);
const struct richtek_dev_data *devdata = priv->devdata;
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- switch (mask) {
- case IIO_CHAN_INFO_SAMP_FREQ:
- if (devdata->fixed_samp_freq)
- return -EINVAL;
- return rtq6056_adc_set_samp_freq(priv, chan, val);
- case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
- return devdata->set_average(priv, val);
- default:
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ if (devdata->fixed_samp_freq)
return -EINVAL;
- }
+ return rtq6056_adc_set_samp_freq(priv, chan, val);
+ case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
+ return devdata->set_average(priv, val);
+ default:
+ return -EINVAL;
}
- unreachable();
+}
+
+static int rtq6056_adc_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int val,
+ int val2, long mask)
+{
+ int ret;
+
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = __rtq6056_adc_write_raw(indio_dev, chan, val, mask);
+ iio_device_release_direct(indio_dev);
+ return ret;
}
static const char *rtq6056_channel_labels[RTQ6056_MAX_CHANNEL] = {
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 19/27] iio: adc: ti-adc161s626: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (17 preceding siblings ...)
2025-01-05 17:26 ` [RFC PATCH 18/27] iio: adc: rtq6056: " Jonathan Cameron
@ 2025-01-05 17:26 ` Jonathan Cameron
2025-01-05 17:26 ` [RFC PATCH 20/27] iio: adc: ti-ads1119: " Jonathan Cameron
` (9 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:26 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/ti-adc161s626.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/adc/ti-adc161s626.c b/drivers/iio/adc/ti-adc161s626.c
index 474e733fb8e0..28aa6b80160c 100644
--- a/drivers/iio/adc/ti-adc161s626.c
+++ b/drivers/iio/adc/ti-adc161s626.c
@@ -137,13 +137,13 @@ static int ti_adc_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- ret = ti_adc_read_measurement(data, chan, val);
- if (ret)
- return ret;
- return IIO_VAL_INT;
- }
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = ti_adc_read_measurement(data, chan, val);
+ iio_device_release_direct(indio_dev);
+ if (ret)
+ return ret;
+ return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
ret = regulator_get_voltage(data->ref);
if (ret < 0)
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 20/27] iio: adc: ti-ads1119: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (18 preceding siblings ...)
2025-01-05 17:26 ` [RFC PATCH 19/27] iio: adc: ti-adc161s626: " Jonathan Cameron
@ 2025-01-05 17:26 ` Jonathan Cameron
2025-01-05 17:26 ` [RFC PATCH 21/27] iio: addac: ad74413r: " Jonathan Cameron
` (8 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:26 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/adc/ti-ads1119.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/adc/ti-ads1119.c b/drivers/iio/adc/ti-ads1119.c
index 0a68ecdea4e6..fe8086622988 100644
--- a/drivers/iio/adc/ti-ads1119.c
+++ b/drivers/iio/adc/ti-ads1119.c
@@ -336,19 +336,24 @@ static int ads1119_read_raw(struct iio_dev *indio_dev,
{
struct ads1119_state *st = iio_priv(indio_dev);
unsigned int index = chan->address;
+ int ret;
if (index >= st->num_channels_cfg)
return -EINVAL;
switch (mask) {
case IIO_CHAN_INFO_RAW:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
- return ads1119_single_conversion(st, chan, val, false);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = ads1119_single_conversion(st, chan, val, false);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CHAN_INFO_OFFSET:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
- return ads1119_single_conversion(st, chan, val, true);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = ads1119_single_conversion(st, chan, val, true);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CHAN_INFO_SCALE:
*val = st->vref_uV / 1000;
*val /= st->channels_cfg[index].gain;
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 21/27] iio: addac: ad74413r: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (19 preceding siblings ...)
2025-01-05 17:26 ` [RFC PATCH 20/27] iio: adc: ti-ads1119: " Jonathan Cameron
@ 2025-01-05 17:26 ` Jonathan Cameron
2025-01-05 17:26 ` [RFC PATCH 22/27] iio: chemical: ens160: " Jonathan Cameron
` (7 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:26 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context. Includes moving a mutex lock
into a function rather than around it to simplify the error handling.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/addac/ad74413r.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/addac/ad74413r.c b/drivers/iio/addac/ad74413r.c
index daea2bde7acf..f14d12b03da6 100644
--- a/drivers/iio/addac/ad74413r.c
+++ b/drivers/iio/addac/ad74413r.c
@@ -826,6 +826,8 @@ static int _ad74413r_get_single_adc_result(struct ad74413r_state *st,
unsigned int uval;
int ret;
+ guard(mutex)(&st->lock);
+
reinit_completion(&st->adc_data_completion);
ret = ad74413r_set_adc_channel_enable(st, channel, true);
@@ -865,12 +867,14 @@ static int ad74413r_get_single_adc_result(struct iio_dev *indio_dev,
unsigned int channel, int *val)
{
struct ad74413r_state *st = iio_priv(indio_dev);
+ int ret;
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- guard(mutex)(&st->lock);
- return _ad74413r_get_single_adc_result(st, channel, val);
- }
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = _ad74413r_get_single_adc_result(st, channel, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
}
static void ad74413r_adc_to_resistance_result(int adc_result, int *val)
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 22/27] iio: chemical: ens160: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (20 preceding siblings ...)
2025-01-05 17:26 ` [RFC PATCH 21/27] iio: addac: ad74413r: " Jonathan Cameron
@ 2025-01-05 17:26 ` Jonathan Cameron
2025-01-05 17:26 ` [RFC PATCH 23/27] iio: dac: ad3552r-hs: " Jonathan Cameron
` (6 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:26 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/chemical/ens160_core.c | 32 ++++++++++++++++++++----------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/iio/chemical/ens160_core.c b/drivers/iio/chemical/ens160_core.c
index 48d5ad2075b6..152f81ff57e3 100644
--- a/drivers/iio/chemical/ens160_core.c
+++ b/drivers/iio/chemical/ens160_core.c
@@ -100,25 +100,35 @@ static const struct iio_chan_spec ens160_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(2),
};
+static int __ens160_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val)
+{
+ struct ens160_data *data = iio_priv(indio_dev);
+ int ret;
+
+ guard(mutex)(&data->mutex);
+ ret = regmap_bulk_read(data->regmap, chan->address,
+ &data->buf, sizeof(data->buf));
+ if (ret)
+ return ret;
+ *val = le16_to_cpu(data->buf);
+ return IIO_VAL_INT;
+}
+
static int ens160_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val, int *val2, long mask)
{
- struct ens160_data *data = iio_priv(indio_dev);
int ret;
switch (mask) {
case IIO_CHAN_INFO_RAW:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- guard(mutex)(&data->mutex);
- ret = regmap_bulk_read(data->regmap, chan->address,
- &data->buf, sizeof(data->buf));
- if (ret)
- return ret;
- *val = le16_to_cpu(data->buf);
- return IIO_VAL_INT;
- }
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = __ens160_read_raw(indio_dev, chan, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CHAN_INFO_SCALE:
switch (chan->channel2) {
case IIO_MOD_CO2:
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 23/27] iio: dac: ad3552r-hs: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (21 preceding siblings ...)
2025-01-05 17:26 ` [RFC PATCH 22/27] iio: chemical: ens160: " Jonathan Cameron
@ 2025-01-05 17:26 ` Jonathan Cameron
2025-01-05 17:26 ` [RFC PATCH 24/27] iio: dac: ad8460: " Jonathan Cameron
` (5 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:26 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/dac/ad3552r-hs.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/dac/ad3552r-hs.c b/drivers/iio/dac/ad3552r-hs.c
index 216c634f3eaf..64899a02ab28 100644
--- a/drivers/iio/dac/ad3552r-hs.c
+++ b/drivers/iio/dac/ad3552r-hs.c
@@ -90,15 +90,18 @@ static int ad3552r_hs_write_raw(struct iio_dev *indio_dev,
int val, int val2, long mask)
{
struct ad3552r_hs_state *st = iio_priv(indio_dev);
+ int ret;
switch (mask) {
case IIO_CHAN_INFO_RAW:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- return st->data->bus_reg_write(st->back,
- AD3552R_REG_ADDR_CH_DAC_16B(chan->channel),
- val, 2);
- }
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = st->data->bus_reg_write(st->back,
+ AD3552R_REG_ADDR_CH_DAC_16B(chan->channel),
+ val, 2);
+
+ iio_device_release_direct(indio_dev);
+ return ret;
default:
return -EINVAL;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 24/27] iio: dac: ad8460: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (22 preceding siblings ...)
2025-01-05 17:26 ` [RFC PATCH 23/27] iio: dac: ad3552r-hs: " Jonathan Cameron
@ 2025-01-05 17:26 ` Jonathan Cameron
2025-01-05 17:26 ` [RFC PATCH 25/27] iio: dummy: " Jonathan Cameron
` (4 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:26 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/dac/ad8460.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c
index 535ee3105af6..6e45686902dd 100644
--- a/drivers/iio/dac/ad8460.c
+++ b/drivers/iio/dac/ad8460.c
@@ -264,9 +264,12 @@ static ssize_t ad8460_write_toggle_en(struct iio_dev *indio_dev, uintptr_t priva
if (ret)
return ret;
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
- return ad8460_enable_apg_mode(state, toggle_en);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = ad8460_enable_apg_mode(state, toggle_en);
+ iio_device_release_direct(indio_dev);
+ return ret;
}
static ssize_t ad8460_read_powerdown(struct iio_dev *indio_dev, uintptr_t private,
@@ -421,14 +424,17 @@ static int ad8460_write_raw(struct iio_dev *indio_dev,
long mask)
{
struct ad8460_state *state = iio_priv(indio_dev);
+ int ret;
switch (mask) {
case IIO_CHAN_INFO_RAW:
switch (chan->type) {
case IIO_VOLTAGE:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
- return ad8460_set_sample(state, val);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = ad8460_set_sample(state, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CURRENT:
return regmap_write(state->regmap, AD8460_CTRL_REG(0x04),
FIELD_PREP(AD8460_QUIESCENT_CURRENT_MSK, val));
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 25/27] iio: dummy: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (23 preceding siblings ...)
2025-01-05 17:26 ` [RFC PATCH 24/27] iio: dac: ad8460: " Jonathan Cameron
@ 2025-01-05 17:26 ` Jonathan Cameron
2025-01-05 17:26 ` [RFC PATCH 26/27] iio: imu: bmi323: " Jonathan Cameron
` (3 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:26 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context. Introduce two new utility functions
to allow for direct returns with claim and release of direct mode
in the caller.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/dummy/iio_simple_dummy.c | 119 ++++++++++++++++-----------
1 file changed, 70 insertions(+), 49 deletions(-)
diff --git a/drivers/iio/dummy/iio_simple_dummy.c b/drivers/iio/dummy/iio_simple_dummy.c
index 09efacaf8f78..8575d4a08963 100644
--- a/drivers/iio/dummy/iio_simple_dummy.c
+++ b/drivers/iio/dummy/iio_simple_dummy.c
@@ -267,6 +267,65 @@ static const struct iio_chan_spec iio_dummy_channels[] = {
},
};
+static int __iio_dummy_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val)
+{
+ struct iio_dummy_state *st = iio_priv(indio_dev);
+
+ guard(mutex)(&st->lock);
+ switch (chan->type) {
+ case IIO_VOLTAGE:
+ if (chan->output) {
+ /* Set integer part to cached value */
+ *val = st->dac_val;
+ return IIO_VAL_INT;
+ } else if (chan->differential) {
+ if (chan->channel == 1)
+ *val = st->differential_adc_val[0];
+ else
+ *val = st->differential_adc_val[1];
+ return IIO_VAL_INT;
+ } else {
+ *val = st->single_ended_adc_val;
+ return IIO_VAL_INT;
+ }
+
+ case IIO_ACCEL:
+ *val = st->accel_val;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int __iio_dummy_read_processed(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val)
+{
+ struct iio_dummy_state *st = iio_priv(indio_dev);
+
+ guard(mutex)(&st->lock);
+ switch (chan->type) {
+ case IIO_STEPS:
+ *val = st->steps;
+ return IIO_VAL_INT;
+ case IIO_ACTIVITY:
+ switch (chan->channel2) {
+ case IIO_MOD_RUNNING:
+ *val = st->activity_running;
+ return IIO_VAL_INT;
+ case IIO_MOD_WALKING:
+ *val = st->activity_walking;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
/**
* iio_dummy_read_raw() - data read function.
* @indio_dev: the struct iio_dev associated with this device instance
@@ -283,59 +342,21 @@ static int iio_dummy_read_raw(struct iio_dev *indio_dev,
long mask)
{
struct iio_dummy_state *st = iio_priv(indio_dev);
+ int ret;
switch (mask) {
case IIO_CHAN_INFO_RAW: /* magic value - channel value read */
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- guard(mutex)(&st->lock);
- switch (chan->type) {
- case IIO_VOLTAGE:
- if (chan->output) {
- /* Set integer part to cached value */
- *val = st->dac_val;
- return IIO_VAL_INT;
- } else if (chan->differential) {
- if (chan->channel == 1)
- *val = st->differential_adc_val[0];
- else
- *val = st->differential_adc_val[1];
- return IIO_VAL_INT;
- } else {
- *val = st->single_ended_adc_val;
- return IIO_VAL_INT;
- }
-
- case IIO_ACCEL:
- *val = st->accel_val;
- return IIO_VAL_INT;
- default:
- return -EINVAL;
- }
- }
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = __iio_dummy_read_raw(indio_dev, chan, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CHAN_INFO_PROCESSED:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- guard(mutex)(&st->lock);
- switch (chan->type) {
- case IIO_STEPS:
- *val = st->steps;
- return IIO_VAL_INT;
- case IIO_ACTIVITY:
- switch (chan->channel2) {
- case IIO_MOD_RUNNING:
- *val = st->activity_running;
- return IIO_VAL_INT;
- case IIO_MOD_WALKING:
- *val = st->activity_walking;
- return IIO_VAL_INT;
- default:
- return -EINVAL;
- }
- default:
- return -EINVAL;
- }
- }
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+ ret = __iio_dummy_read_processed(indio_dev, chan, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_CHAN_INFO_OFFSET:
/* only single ended adc -> 7 */
*val = 7;
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 26/27] iio: imu: bmi323: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (24 preceding siblings ...)
2025-01-05 17:26 ` [RFC PATCH 25/27] iio: dummy: " Jonathan Cameron
@ 2025-01-05 17:26 ` Jonathan Cameron
2025-01-05 17:26 ` [RFC PATCH 27/27] iio: light: bh1745: " Jonathan Cameron
` (2 subsequent siblings)
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:26 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/imu/bmi323/bmi323_core.c | 51 +++++++++++++++++-----------
1 file changed, 32 insertions(+), 19 deletions(-)
diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c
index 7f386c5e58b4..4c404e44a3f4 100644
--- a/drivers/iio/imu/bmi323/bmi323_core.c
+++ b/drivers/iio/imu/bmi323/bmi323_core.c
@@ -1702,26 +1702,36 @@ static int bmi323_write_raw(struct iio_dev *indio_dev,
int val2, long mask)
{
struct bmi323_data *data = iio_priv(indio_dev);
+ int ret;
switch (mask) {
case IIO_CHAN_INFO_SAMP_FREQ:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
- return bmi323_set_odr(data,
- bmi323_iio_to_sensor(chan->type),
- val, val2);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = bmi323_set_odr(data, bmi323_iio_to_sensor(chan->type),
+ val, val2);
+ iio_device_release_direct(indio_dev);
+
+ return ret;
case IIO_CHAN_INFO_SCALE:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
- return bmi323_set_scale(data,
- bmi323_iio_to_sensor(chan->type),
- val, val2);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = bmi323_set_scale(data, bmi323_iio_to_sensor(chan->type),
+ val, val2);
+ iio_device_release_direct(indio_dev);
+
+ return ret;
case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev)
- return bmi323_set_average(data,
- bmi323_iio_to_sensor(chan->type),
- val);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = bmi323_set_average(data, bmi323_iio_to_sensor(chan->type),
+ val);
+ iio_device_release_direct(indio_dev);
+
+ return ret;
case IIO_CHAN_INFO_ENABLE:
return bmi323_enable_steps(data, val);
case IIO_CHAN_INFO_PROCESSED: {
@@ -1747,6 +1757,7 @@ static int bmi323_read_raw(struct iio_dev *indio_dev,
int *val2, long mask)
{
struct bmi323_data *data = iio_priv(indio_dev);
+ int ret;
switch (mask) {
case IIO_CHAN_INFO_PROCESSED:
@@ -1755,10 +1766,12 @@ static int bmi323_read_raw(struct iio_dev *indio_dev,
switch (chan->type) {
case IIO_ACCEL:
case IIO_ANGL_VEL:
- iio_device_claim_direct_scoped(return -EBUSY,
- indio_dev)
- return bmi323_read_axis(data, chan, val);
- unreachable();
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
+
+ ret = bmi323_read_axis(data, chan, val);
+ iio_device_release_direct(indio_dev);
+ return ret;
case IIO_TEMP:
return bmi323_get_temp_data(data, val);
default:
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* [RFC PATCH 27/27] iio: light: bh1745: Stop using iio_device_claim_direct_scoped()
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (25 preceding siblings ...)
2025-01-05 17:26 ` [RFC PATCH 26/27] iio: imu: bmi323: " Jonathan Cameron
@ 2025-01-05 17:26 ` Jonathan Cameron
2025-01-07 13:09 ` [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Nuno Sá
2025-02-02 21:00 ` Jonathan Cameron
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-05 17:26 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
This complex cleanup.h use case of conditional guards has proved
to be more trouble that it is worth in terms of false positive compiler
warnings and hard to read code.
Move directly to the new claim/release_direct() that allow sparse
to check for unbalanced context.
Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/iio/light/bh1745.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/iio/light/bh1745.c b/drivers/iio/light/bh1745.c
index 00a33760893e..a2f3df7f1818 100644
--- a/drivers/iio/light/bh1745.c
+++ b/drivers/iio/light/bh1745.c
@@ -426,16 +426,16 @@ static int bh1745_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
- iio_device_claim_direct_scoped(return -EBUSY, indio_dev) {
- ret = regmap_bulk_read(data->regmap, chan->address,
- &value, 2);
- if (ret)
- return ret;
- *val = value;
+ if (!iio_device_claim_direct(indio_dev))
+ return -EBUSY;
- return IIO_VAL_INT;
- }
- unreachable();
+ ret = regmap_bulk_read(data->regmap, chan->address, &value, 2);
+ iio_device_release_direct(indio_dev);
+ if (ret)
+ return ret;
+ *val = value;
+
+ return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE: {
guard(mutex)(&data->lock);
--
2.47.1
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [RFC PATCH 00/27] iio: improve handling of direct mode claim and release
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (26 preceding siblings ...)
2025-01-05 17:26 ` [RFC PATCH 27/27] iio: light: bh1745: " Jonathan Cameron
@ 2025-01-07 13:09 ` Nuno Sá
2025-01-07 14:31 ` Jonathan Cameron
2025-02-02 21:00 ` Jonathan Cameron
28 siblings, 1 reply; 49+ messages in thread
From: Nuno Sá @ 2025-01-07 13:09 UTC (permalink / raw)
To: Jonathan Cameron, linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
On Sun, 2025-01-05 at 17:25 +0000, Jonathan Cameron wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Note I haven't attempted to CC relevant people for specific drivers.
> I'll do that for a non RFC version if we move forwards.
>
> Effectively two linked things in this series:
>
> 1) Ripping out iio_device_claim_direct_scoped()
> 2) Enabling use of sparse to check the claim is always released.
>
> The iio_device_claim_direct_scoped() was an interesting experiment
> built on conditional scoped guards, but it has been the source of
> a range of esoteric build warnings and is awkward to use.
>
Curious about one thing... David, wouldn't your work on 'if_not_cond_guard()'
help with this messy scoped calls? I saw it was not merged yet though... Was it
dropped for some reason?
Anyways, I do like this approach specially due to 2) which, likely, it would not
be straightforward with automatic cleanups (if feasible at all).
I plan to go over the whole series in the next few days...
- Nuno Sá
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [RFC PATCH 00/27] iio: improve handling of direct mode claim and release
2025-01-07 13:09 ` [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Nuno Sá
@ 2025-01-07 14:31 ` Jonathan Cameron
2025-01-07 16:07 ` Nuno Sá
0 siblings, 1 reply; 49+ messages in thread
From: Jonathan Cameron @ 2025-01-07 14:31 UTC (permalink / raw)
To: Nuno Sá
Cc: Jonathan Cameron, linux-iio, “Luc Van Oostenryck”,
David Lechner
On Tue, 07 Jan 2025 13:09:44 +0000
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Sun, 2025-01-05 at 17:25 +0000, Jonathan Cameron wrote:
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Note I haven't attempted to CC relevant people for specific drivers.
> > I'll do that for a non RFC version if we move forwards.
> >
> > Effectively two linked things in this series:
> >
> > 1) Ripping out iio_device_claim_direct_scoped()
> > 2) Enabling use of sparse to check the claim is always released.
> >
> > The iio_device_claim_direct_scoped() was an interesting experiment
> > built on conditional scoped guards, but it has been the source of
> > a range of esoteric build warnings and is awkward to use.
> >
>
> Curious about one thing... David, wouldn't your work on 'if_not_cond_guard()'
> help with this messy scoped calls? I saw it was not merged yet though... Was it
> dropped for some reason?
Link in cover letter. David's work got merged then reverted :(
https://lore.kernel.org/all/CAHk-=wi8C2yZF_y_T180-v+dSZAhps5QghS_2tKfn-+xAghYPQ@mail.gmail.com/
Basically it seems to be impossible to contrive a way of doing scoped condition
cleanup neatly. I was also hoping we could transition to the if_cond_guard()
approach to solve the scoped problems. :(
Jonathan
>
> Anyways, I do like this approach specially due to 2) which, likely, it would not
> be straightforward with automatic cleanups (if feasible at all).
>
> I plan to go over the whole series in the next few days...
>
> - Nuno Sá
>
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [RFC PATCH 00/27] iio: improve handling of direct mode claim and release
2025-01-07 14:31 ` Jonathan Cameron
@ 2025-01-07 16:07 ` Nuno Sá
0 siblings, 0 replies; 49+ messages in thread
From: Nuno Sá @ 2025-01-07 16:07 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Cameron, linux-iio, “Luc Van Oostenryck”,
David Lechner
On Tue, 2025-01-07 at 14:31 +0000, Jonathan Cameron wrote:
> On Tue, 07 Jan 2025 13:09:44 +0000
> Nuno Sá <noname.nuno@gmail.com> wrote:
>
> > On Sun, 2025-01-05 at 17:25 +0000, Jonathan Cameron wrote:
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > Note I haven't attempted to CC relevant people for specific drivers.
> > > I'll do that for a non RFC version if we move forwards.
> > >
> > > Effectively two linked things in this series:
> > >
> > > 1) Ripping out iio_device_claim_direct_scoped()
> > > 2) Enabling use of sparse to check the claim is always released.
> > >
> > > The iio_device_claim_direct_scoped() was an interesting experiment
> > > built on conditional scoped guards, but it has been the source of
> > > a range of esoteric build warnings and is awkward to use.
> > >
> >
> > Curious about one thing... David, wouldn't your work on
> > 'if_not_cond_guard()'
> > help with this messy scoped calls? I saw it was not merged yet though... Was
> > it
> > dropped for some reason?
>
> Link in cover letter. David's work got merged then reverted :(
>
> https://lore.kernel.org/all/CAHk-=wi8C2yZF_y_T180-v+dSZAhps5QghS_2tKfn-+xAghYPQ@mail.gmail.com/
>
> Basically it seems to be impossible to contrive a way of doing scoped
> condition
> cleanup neatly. I was also hoping we could transition to the if_cond_guard()
> approach to solve the scoped problems. :(
>
Auch, should have read the complete cover. I'll go check that thread.
Thanks!
- Nuno Sá
>
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [RFC PATCH 00/27] iio: improve handling of direct mode claim and release
2025-01-05 17:25 [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Jonathan Cameron
` (27 preceding siblings ...)
2025-01-07 13:09 ` [RFC PATCH 00/27] iio: improve handling of direct mode claim and release Nuno Sá
@ 2025-02-02 21:00 ` Jonathan Cameron
28 siblings, 0 replies; 49+ messages in thread
From: Jonathan Cameron @ 2025-02-02 21:00 UTC (permalink / raw)
To: linux-iio, “Luc Van Oostenryck”
Cc: David Lechner, Jonathan Cameron
On Sun, 5 Jan 2025 17:25:45 +0000
Jonathan Cameron <jic23@kernel.org> wrote:
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Note I haven't attempted to CC relevant people for specific drivers.
> I'll do that for a non RFC version if we move forwards.
I've finished an initial conversion. 'Only' 119 patches
109 files changed, 1990 insertions(+), 1926 deletions(-)
will be getting some poking from 0-day.
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/log/?h=sparse-friendly-direct-mode
I've combined some simple ones though it would only add another 20
or so patches to break them all up by driver, so I might do that just
to keep things bite sized.
Anyhow, once I've had a chance to take one last glance through I'll start
sending them out in groups of 20-30ish.
Quite a few older, less actively maintained drivers in here so I will
be relying on those of you who review beyond your own stuff!
Whilst fairly mechanical, some of the code refactors are a bit complex.
A few driver bugs found and probably a few added by this lot.
To those who have lots of out of tree code, I'm sorry to say this
will hurt a bit, but in the short term you can revert the final patch
that removes the old APIs.
Thanks,
Jonathan
p.s. At least one more similar large refactor on my todo list that
I might get done this cycle.
^ permalink raw reply [flat|nested] 49+ messages in thread