linux-kernel - Re: [PATCH v2 4/7] iio: core: Add cleanup.h support for iio_device_claim

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DF974CHWQ5BL.MW4NYZU499S7@gmail.com>
Date: Sat, 27 Dec 2025 13:04:14 -0500
From: "Kurt Borja" <kuurtb@...il.com>
To: "David Lechner" <dlechner@...libre.com>, "Kurt Borja"
 <kuurtb@...il.com>, "Andy Shevchenko" <andriy.shevchenko@...el.com>,
 "Lars-Peter Clausen" <lars@...afoo.de>, "Michael Hennerich"
 <Michael.Hennerich@...log.com>, "Jonathan Cameron" <jic23@...nel.org>,
 "Benson Leung" <bleung@...omium.org>, "Antoniu Miclaus"
 <antoniu.miclaus@...log.com>, "Gwendal Grignou" <gwendal@...omium.org>,
 "Shrikant Raskar" <raskar.shree97@...il.com>, "Per-Daniel Olsson"
 <perdaniel.olsson@...s.com>
Cc: Nuno Sá <nuno.sa@...log.com>, "Andy Shevchenko"
 <andy@...nel.org>, "Guenter Roeck" <groeck@...omium.org>, "Jonathan
 Cameron" <Jonathan.Cameron@...wei.com>, <linux-iio@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <chrome-platform@...ts.linux.dev>
Subject: Re: [PATCH v2 4/7] iio: core: Add cleanup.h support for
 iio_device_claim_*()

On Tue Dec 23, 2025 at 12:23 PM -05, David Lechner wrote:
> On 12/11/25 8:45 PM, Kurt Borja wrote:
>> Add guard classes for iio_device_claim_*() conditional locks. This will
>> aid drivers write safer and cleaner code when dealing with some common
>> patterns.
>> 
>
>
>> These classes are not meant to be used directly by drivers (hence the
>> __priv__ prefix). Instead, documented wrapper macros are provided to
>> enforce the use of ACQUIRE() or guard() semantics and avoid the
>> problematic scoped guard.
>
> Would be useful to repeat this in a comment in the code.

Sure!

>
>> 
>> Suggested-by: Andy Shevchenko <andriy.shevchenko@...el.com>
>> Signed-off-by: Kurt Borja <kuurtb@...il.com>
>> ---
>>  include/linux/iio/iio.h | 83 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 83 insertions(+)
>> 
>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>> index f8a7ef709210..c84853c7a37f 100644
>> --- a/include/linux/iio/iio.h
>> +++ b/include/linux/iio/iio.h
>> @@ -10,6 +10,7 @@
>>  #include <linux/align.h>
>>  #include <linux/device.h>
>>  #include <linux/cdev.h>
>> +#include <linux/cleanup.h>
>>  #include <linux/compiler_types.h>
>>  #include <linux/minmax.h>
>>  #include <linux/slab.h>
>> @@ -739,6 +740,88 @@ static inline void iio_device_release_buffer_mode(struct iio_dev *indio_dev)
>>  	__iio_dev_mode_unlock(indio_dev);
>>  }
>>  
>> +DEFINE_GUARD(__priv__iio_dev_mode_lock, struct iio_dev *,
>> +	     __iio_dev_mode_lock(_T), __iio_dev_mode_unlock(_T));
>> +DEFINE_GUARD_COND(__priv__iio_dev_mode_lock, _try_buffer,
>> +		  iio_device_claim_buffer_mode(_T));
>> +DEFINE_GUARD_COND(__priv__iio_dev_mode_lock, _try_direct,
>> +		  iio_device_claim_direct(_T));
>> +
>> +/**
>> + * IIO_DEV_ACQUIRE_DIRECT_MODE(_dev, _var) - Tries to acquire the direct mode
>> + *                                           lock with automatic release
>> + * @_dev: IIO device instance
>> + * @_var: Dummy variable identifier to store acquire result
>
> It's not a dummy if it does something. :-) (so we can drop that word)

That's true :).

>
> Also, I would call it _claim instead of _var to to match the example
> and encourage people to use the same name everywhere.
>
> And for that matter, we don't really need the leading underscores in either
> parameter since there are no name conflicts.

Ack.

...

>> +/**
>> + * IIO_DEV_ACQUIRE_ERR() - ACQUIRE_ERR() wrapper
>> + * @_var: Dummy variable passed to IIO_DEV_ACQUIRE_*_MODE()
>> + *
>> + * Return: true on success, false on error
>
> This could be clarified more. Based on the example, this sounds
> backwards.
>
> 	Returns: true if acquiring the mode failed, otherwise false.
>
>> + */
>> +#define IIO_DEV_ACQUIRE_ERR(_var_ptr) \
>> +	ACQUIRE_ERR(__priv__iio_dev_mode_lock_try_buffer, _var_ptr)
>
> There is no error code here, so calling it "ERR" seems wrong.
> Maybe IIO_DEV_ACQUIRE_FAILED()?

Here I'd prefer to keep it as _ERR so users can make the immediate
association. But I don't feel strongly about it.

>
>> +
>> +/**
>> + * IIO_DEV_GUARD_ANY_MODE - Acquires the mode lock with automatic release
>> + * @_dev: IIO device instance
>
> It would be helpful to explain more about the use case here and that this
> is used rarely.
>
> The point to get across is that it is used when we need to do something
> that doesn't depend on the current mode, but would be affected by a mode
> switch. So it guards against changing the mode without caring what the
> current mode is. If it is in buffer mode, it stays in buffer mode, otherwise
> direct mode is claimed.

I'll add a comment on this.

>
>> + *
>> + * Acquires the mode lock with cleanup guard() semantics. It is usually paired
>> + * with iio_buffer_enabled().
>> + *
>> + * This should *not* be used to protect internal driver state and it's use in
>> + * general is *strongly* discouraged. Use any of the IIO_DEV_ACQUIRE_*_MODE()
>> + * variants.
>
> Might as well add Context: here like the others.
>
>> + */
>> +#define IIO_DEV_GUARD_ANY_MODE(_dev) \
>
> Accordingly, I would be inclined to call it IIO_DEV_GUARD_CURRENT_MODE()

I like this better.

Thank you for your comments!

-- 
 ~ Kurt