drivers/iio/adc/ade9000.c | 4 +- .../common/cros_ec_sensors/cros_ec_sensors_core.c | 7 +-- drivers/iio/health/max30100.c | 4 +- drivers/iio/health/max30102.c | 24 +++------- drivers/iio/industrialio-core.c | 34 +++++++++----- drivers/iio/light/opt4060.c | 52 +++++++--------------- drivers/iio/light/vcnl4000.c | 24 ++++------ include/linux/iio/iio.h | 24 +++++++++- 8 files changed, 83 insertions(+), 90 deletions(-)
Hi,
In a recent driver review discussion [1], Andy Shevchenko suggested we
add cleanup.h support for the lock API:
iio_device_claim_{direct,buffer_mode}().
Which would allow some nice code simplification* in many places. Some
examples are given as patches, but the last two are the biggest
differences.
Although I was never entirely sure if Andy meant cleanup classes for
locks or for iio_trigger_notify_done(), I still think this is a great
addition to the API :).
Thanks for taking a look!
* It's important to mention that David Lechner expressed some concerns
about this [2], hence why this is an RFC series.
[1] https://lore.kernel.org/linux-iio/aSsBdJZDWcadxEHC@smile.fi.intel.com/
[2] https://lore.kernel.org/linux-iio/248b009e-0401-4531-b9f0-56771e16bdef@baylibre.com/
Signed-off-by: Kurt Borja <kuurtb@gmail.com>
---
Kurt Borja (6):
iio: core: Match iio_device_claim_*() return semantics
iio: core: Match iio_device_claim_*() naming
iio: core: Add cleanup.h support for iio_device_claim_*()
iio: light: vcnl4000: Use cleanup.h for IIO locks
iio: health: max30102: Use cleanup.h for IIO locks
iio: light: opt4060: Use cleanup.h for IIO locks
drivers/iio/adc/ade9000.c | 4 +-
.../common/cros_ec_sensors/cros_ec_sensors_core.c | 7 +--
drivers/iio/health/max30100.c | 4 +-
drivers/iio/health/max30102.c | 24 +++-------
drivers/iio/industrialio-core.c | 34 +++++++++-----
drivers/iio/light/opt4060.c | 52 +++++++---------------
drivers/iio/light/vcnl4000.c | 24 ++++------
include/linux/iio/iio.h | 24 +++++++++-
8 files changed, 83 insertions(+), 90 deletions(-)
---
base-commit: f9e05791642810a0cf6237d39fafd6fec5e0b4bb
change-id: 20251130-lock-impr-6f22748c15e8
--
~ Kurt
On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
> Hi,
>
> In a recent driver review discussion [1], Andy Shevchenko suggested we
> add cleanup.h support for the lock API:
>
> iio_device_claim_{direct,buffer_mode}().
We already went this patch and then reverted it. I guess before we did not had
ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
annotations much make sense if we go down this path. Unless we want to use both
approaches which is also questionable.
- Nuno Sá
On Thu Dec 4, 2025 at 9:36 AM -05, Nuno Sá wrote:
> On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
>> Hi,
>>
>> In a recent driver review discussion [1], Andy Shevchenko suggested we
>> add cleanup.h support for the lock API:
>>
>> iio_device_claim_{direct,buffer_mode}().
>
> We already went this patch and then reverted it. I guess before we did not had
> ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
> last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
>
> Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
> annotations much make sense if we go down this path. Unless we want to use both
> approaches which is also questionable.
I think if we add iio_device_claim() or whatever the final name may be,
we can annotate that instead with acquires(&mlock) and maybe it could
work?
I will test that.
>
> - Nuno Sá
--
~ Kurt
On Thu, Dec 4, 2025 at 4:35 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
> >
> > In a recent driver review discussion [1], Andy Shevchenko suggested we
> > add cleanup.h support for the lock API:
> >
> > iio_device_claim_{direct,buffer_mode}().
>
> We already went this patch and then reverted it. I guess before we did not had
> ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
> last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
>
> Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
> annotations much make sense if we go down this path. Unless we want to use both
> approaches which is also questionable.
This, indeed, needs a (broader) discussion and I appreciate that Kurt
sent this RFC. Jonathan, what's your thoughts?
--
With Best Regards,
Andy Shevchenko
On Thu, 4 Dec 2025 17:07:28 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Thu, Dec 4, 2025 at 4:35 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
> > >
> > > In a recent driver review discussion [1], Andy Shevchenko suggested we
> > > add cleanup.h support for the lock API:
> > >
> > > iio_device_claim_{direct,buffer_mode}().
> >
> > We already went this patch and then reverted it. I guess before we did not had
> > ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
> > last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
> >
> > Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
> > annotations much make sense if we go down this path. Unless we want to use both
> > approaches which is also questionable.
>
> This, indeed, needs a (broader) discussion and I appreciate that Kurt
> sent this RFC. Jonathan, what's your thoughts?
I was pretty heavily involved in discussions around ACQUIRE() and it's use
in CXL and runtime PM (though that's still evolving with Rafael trying
to improve the syntax a little). As you might guess I did have this use
in mind during those discussions.
As far as I know by avoiding the for loop complexity of the previous
try we made and looking (under the hood) like guard() it should be much
easier and safer to use. Looking at this was on my list, so I'm very happy
to see this series from Kurt exploring how it would be done.
Sparse wise there is no support for now for any of the cleanup.h magic
other than ignoring it. That doesn't bother me that much though as these
macros create more or less hidden local variables that are hard to mess
with in incorrect ways.
So in general I'm very much in favour of this for same reasons I jumped
in last time (which turned out to be premature!)
This will be particularly useful in avoiding the need for helper functions
in otherwise simple code flows.
Jonathan
On Sat, 2025-12-06 at 18:46 +0000, Jonathan Cameron wrote:
> On Thu, 4 Dec 2025 17:07:28 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > On Thu, Dec 4, 2025 at 4:35 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
> > > >
> > > > In a recent driver review discussion [1], Andy Shevchenko suggested we
> > > > add cleanup.h support for the lock API:
> > > >
> > > > iio_device_claim_{direct,buffer_mode}().
> > >
> > > We already went this patch and then reverted it. I guess before we did not had
> > > ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
> > > last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
> > >
> > > Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
> > > annotations much make sense if we go down this path. Unless we want to use both
> > > approaches which is also questionable.
> >
> > This, indeed, needs a (broader) discussion and I appreciate that Kurt
> > sent this RFC. Jonathan, what's your thoughts?
>
> I was pretty heavily involved in discussions around ACQUIRE() and it's use
> in CXL and runtime PM (though that's still evolving with Rafael trying
> to improve the syntax a little). As you might guess I did have this use
> in mind during those discussions.
>
> As far as I know by avoiding the for loop complexity of the previous
> try we made and looking (under the hood) like guard() it should be much
> easier and safer to use. Looking at this was on my list, so I'm very happy
> to see this series from Kurt exploring how it would be done.
>
> Sparse wise there is no support for now for any of the cleanup.h magic
> other than ignoring it. That doesn't bother me that much though as these
> macros create more or less hidden local variables that are hard to mess
> with in incorrect ways.
>
> So in general I'm very much in favour of this for same reasons I jumped
> in last time (which turned out to be premature!)
>
> This will be particularly useful in avoiding the need for helper functions
> in otherwise simple code flows.
>
Ok, it seems we are going down the path to introduce this again. I do agree the new ACQUIRE()
macros make things better (btw, I would be in favor of something similar to pm runtime). Though
I'm still a bit worried about the device lock helper (the iio_device_claim one). We went through
some significant work in order to make mlock private (given historical abuse of it) and this
is basically making it public again. So I would like to either think a bit harder to see if we
can avoid it or just keep the code in patches 5 and 6 as is (even though the dance in there is
really not pretty).
At the very least I would like to see a big, fat comment stating that lock is not to be randomly
used by drivers to protect their own internal data structures and state.
- Nuno Sá
On 12/9/25 4:34 AM, Nuno Sá wrote:
> On Sat, 2025-12-06 at 18:46 +0000, Jonathan Cameron wrote:
>> On Thu, 4 Dec 2025 17:07:28 +0200
>> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>
>>> On Thu, Dec 4, 2025 at 4:35 PM Nuno Sá <noname.nuno@gmail.com> wrote:
>>>> On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
>>>>>
>>>>> In a recent driver review discussion [1], Andy Shevchenko suggested we
>>>>> add cleanup.h support for the lock API:
>>>>>
>>>>> iio_device_claim_{direct,buffer_mode}().
>>>>
>>>> We already went this patch and then reverted it. I guess before we did not had
>>>> ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
>>>> last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
>>>>
>>>> Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
>>>> annotations much make sense if we go down this path. Unless we want to use both
>>>> approaches which is also questionable.
>>>
>>> This, indeed, needs a (broader) discussion and I appreciate that Kurt
>>> sent this RFC. Jonathan, what's your thoughts?
>>
>> I was pretty heavily involved in discussions around ACQUIRE() and it's use
>> in CXL and runtime PM (though that's still evolving with Rafael trying
>> to improve the syntax a little). As you might guess I did have this use
>> in mind during those discussions.
>>
>> As far as I know by avoiding the for loop complexity of the previous
>> try we made and looking (under the hood) like guard() it should be much
>> easier and safer to use. Looking at this was on my list, so I'm very happy
>> to see this series from Kurt exploring how it would be done.
>>
>> Sparse wise there is no support for now for any of the cleanup.h magic
>> other than ignoring it. That doesn't bother me that much though as these
>> macros create more or less hidden local variables that are hard to mess
>> with in incorrect ways.
>>
>> So in general I'm very much in favour of this for same reasons I jumped
>> in last time (which turned out to be premature!)
>>
>> This will be particularly useful in avoiding the need for helper functions
>> in otherwise simple code flows.
>>
>
> Ok, it seems we are going down the path to introduce this again. I do agree the new ACQUIRE()
> macros make things better (btw, I would be in favor of something similar to pm runtime). Though
> I'm still a bit worried about the device lock helper (the iio_device_claim one). We went through
> some significant work in order to make mlock private (given historical abuse of it) and this
> is basically making it public again. So I would like to either think a bit harder to see if we
> can avoid it or just keep the code in patches 5 and 6 as is (even though the dance in there is
> really not pretty).
>
> At the very least I would like to see a big, fat comment stating that lock is not to be randomly
> used by drivers to protect their own internal data structures and state.
>
> - Nuno Sá
Due to the way that conditional guards only extend regular guards, I don't
think there is a way to not expose the basic mlock wrapper. So "don't use this
unless you really know what you are doing" docs seem like the best option.
On Tue, 2025-12-09 at 11:05 -0600, David Lechner wrote:
> On 12/9/25 4:34 AM, Nuno Sá wrote:
> > On Sat, 2025-12-06 at 18:46 +0000, Jonathan Cameron wrote:
> > > On Thu, 4 Dec 2025 17:07:28 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > >
> > > > On Thu, Dec 4, 2025 at 4:35 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > > On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
> > > > > >
> > > > > > In a recent driver review discussion [1], Andy Shevchenko suggested we
> > > > > > add cleanup.h support for the lock API:
> > > > > >
> > > > > > iio_device_claim_{direct,buffer_mode}().
> > > > >
> > > > > We already went this patch and then reverted it. I guess before we did not had
> > > > > ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
> > > > > last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
> > > > >
> > > > > Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
> > > > > annotations much make sense if we go down this path. Unless we want to use both
> > > > > approaches which is also questionable.
> > > >
> > > > This, indeed, needs a (broader) discussion and I appreciate that Kurt
> > > > sent this RFC. Jonathan, what's your thoughts?
> > >
> > > I was pretty heavily involved in discussions around ACQUIRE() and it's use
> > > in CXL and runtime PM (though that's still evolving with Rafael trying
> > > to improve the syntax a little). As you might guess I did have this use
> > > in mind during those discussions.
> > >
> > > As far as I know by avoiding the for loop complexity of the previous
> > > try we made and looking (under the hood) like guard() it should be much
> > > easier and safer to use. Looking at this was on my list, so I'm very happy
> > > to see this series from Kurt exploring how it would be done.
> > >
> > > Sparse wise there is no support for now for any of the cleanup.h magic
> > > other than ignoring it. That doesn't bother me that much though as these
> > > macros create more or less hidden local variables that are hard to mess
> > > with in incorrect ways.
> > >
> > > So in general I'm very much in favour of this for same reasons I jumped
> > > in last time (which turned out to be premature!)
> > >
> > > This will be particularly useful in avoiding the need for helper functions
> > > in otherwise simple code flows.
> > >
> >
> > Ok, it seems we are going down the path to introduce this again. I do agree the new ACQUIRE()
> > macros make things better (btw, I would be in favor of something similar to pm runtime). Though
> > I'm still a bit worried about the device lock helper (the iio_device_claim one). We went through
> > some significant work in order to make mlock private (given historical abuse of it) and this
> > is basically making it public again. So I would like to either think a bit harder to see if we
> > can avoid it or just keep the code in patches 5 and 6 as is (even though the dance in there is
> > really not pretty).
> >
> > At the very least I would like to see a big, fat comment stating that lock is not to be randomly
> > used by drivers to protect their own internal data structures and state.
> >
> > - Nuno Sá
>
> Due to the way that conditional guards only extend regular guards, I don't
> think there is a way to not expose the basic mlock wrapper. So "don't use this
> unless you really know what you are doing" docs seem like the best option.
Right! I figured my first option would be very unlikely... But for the comment I hope we can
elaborate a bit more. Like "don't use this lock to protect your own driver state/data ... you might
need this together iio_buffer_enabled() and if for some reason you cannot use the claim helpers).
- Nuno Sá
On Wed, 10 Dec 2025 09:17:17 +0000
Nuno Sá <noname.nuno@gmail.com> wrote:
> On Tue, 2025-12-09 at 11:05 -0600, David Lechner wrote:
> > On 12/9/25 4:34 AM, Nuno Sá wrote:
> > > On Sat, 2025-12-06 at 18:46 +0000, Jonathan Cameron wrote:
> > > > On Thu, 4 Dec 2025 17:07:28 +0200
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > >
> > > > > On Thu, Dec 4, 2025 at 4:35 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> > > > > > On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
> > > > > > >
> > > > > > > In a recent driver review discussion [1], Andy Shevchenko suggested we
> > > > > > > add cleanup.h support for the lock API:
> > > > > > >
> > > > > > > iio_device_claim_{direct,buffer_mode}().
> > > > > >
> > > > > > We already went this patch and then reverted it. I guess before we did not had
> > > > > > ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
> > > > > > last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
> > > > > >
> > > > > > Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
> > > > > > annotations much make sense if we go down this path. Unless we want to use both
> > > > > > approaches which is also questionable.
> > > > >
> > > > > This, indeed, needs a (broader) discussion and I appreciate that Kurt
> > > > > sent this RFC. Jonathan, what's your thoughts?
> > > >
> > > > I was pretty heavily involved in discussions around ACQUIRE() and it's use
> > > > in CXL and runtime PM (though that's still evolving with Rafael trying
> > > > to improve the syntax a little). As you might guess I did have this use
> > > > in mind during those discussions.
> > > >
> > > > As far as I know by avoiding the for loop complexity of the previous
> > > > try we made and looking (under the hood) like guard() it should be much
> > > > easier and safer to use. Looking at this was on my list, so I'm very happy
> > > > to see this series from Kurt exploring how it would be done.
> > > >
> > > > Sparse wise there is no support for now for any of the cleanup.h magic
> > > > other than ignoring it. That doesn't bother me that much though as these
> > > > macros create more or less hidden local variables that are hard to mess
> > > > with in incorrect ways.
> > > >
> > > > So in general I'm very much in favour of this for same reasons I jumped
> > > > in last time (which turned out to be premature!)
> > > >
> > > > This will be particularly useful in avoiding the need for helper functions
> > > > in otherwise simple code flows.
> > > >
> > >
> > > Ok, it seems we are going down the path to introduce this again. I do agree the new ACQUIRE()
> > > macros make things better (btw, I would be in favor of something similar to pm runtime). Though
> > > I'm still a bit worried about the device lock helper (the iio_device_claim one). We went through
> > > some significant work in order to make mlock private (given historical abuse of it) and this
> > > is basically making it public again. So I would like to either think a bit harder to see if we
> > > can avoid it or just keep the code in patches 5 and 6 as is (even though the dance in there is
> > > really not pretty).
> > >
> > > At the very least I would like to see a big, fat comment stating that lock is not to be randomly
> > > used by drivers to protect their own internal data structures and state.
> > >
> > > - Nuno Sá
> >
> > Due to the way that conditional guards only extend regular guards, I don't
> > think there is a way to not expose the basic mlock wrapper. So "don't use this
> > unless you really know what you are doing" docs seem like the best option.
>
> Right! I figured my first option would be very unlikely... But for the comment I hope we can
> elaborate a bit more. Like "don't use this lock to protect your own driver state/data ... you might
> need this together iio_buffer_enabled() and if for some reason you cannot use the claim helpers).
I think this problem is possible to work around.
We use wrapper macros rather than ever using the ACQUIRE macro directly
(and obscure function names to make it obvious they aren't normal :)
The underlying lock guards might be there but they'll smell so bad that anyone
using them should have at least checked why and hence seen the 'do not use this'
documentation.
Jonathan
>
> - Nuno Sá
>
>
On Sat Dec 6, 2025 at 1:46 PM -05, Jonathan Cameron wrote:
> On Thu, 4 Dec 2025 17:07:28 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
>> On Thu, Dec 4, 2025 at 4:35 PM Nuno Sá <noname.nuno@gmail.com> wrote:
>> > On Wed, 2025-12-03 at 14:18 -0500, Kurt Borja wrote:
>> > >
>> > > In a recent driver review discussion [1], Andy Shevchenko suggested we
>> > > add cleanup.h support for the lock API:
>> > >
>> > > iio_device_claim_{direct,buffer_mode}().
>> >
>> > We already went this patch and then reverted it. I guess before we did not had
>> > ACQUIRE() and ACQUIRE_ERR() but I'm not sure that makes it much better. Looking at the
>> > last two patches on how we are handling the buffer mode stuff, I'm really not convinced...
>> >
>> > Also, I have doubts sparse can keep up with the __cleanup stuff so I'm not sure the
>> > annotations much make sense if we go down this path. Unless we want to use both
>> > approaches which is also questionable.
>>
>> This, indeed, needs a (broader) discussion and I appreciate that Kurt
>> sent this RFC. Jonathan, what's your thoughts?
>
> I was pretty heavily involved in discussions around ACQUIRE() and it's use
> in CXL and runtime PM (though that's still evolving with Rafael trying
> to improve the syntax a little). As you might guess I did have this use
> in mind during those discussions.
>
> As far as I know by avoiding the for loop complexity of the previous
> try we made and looking (under the hood) like guard() it should be much
> easier and safer to use. Looking at this was on my list, so I'm very happy
> to see this series from Kurt exploring how it would be done.
>
> Sparse wise there is no support for now for any of the cleanup.h magic
> other than ignoring it. That doesn't bother me that much though as these
> macros create more or less hidden local variables that are hard to mess
> with in incorrect ways.
>
> So in general I'm very much in favour of this for same reasons I jumped
> in last time (which turned out to be premature!)
>
> This will be particularly useful in avoiding the need for helper functions
> in otherwise simple code flows.
Good to hear!
Next version, after we agree on the naming approach, I'll drop the RFC
and thake all suggestions for the next version.
Thank you all for your suggestions and comments :)
>
> Jonathan
--
~ Kurt
© 2016 - 2025 Red Hat, Inc.