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] [day] [month] [year] [list]
Message-ID: <f5ef483b-12d8-4ed2-9637-af993c3f8b7d@baylibre.com>
Date: Tue, 23 Dec 2025 11:23:24 -0600
From: David Lechner <dlechner@...libre.com>
To: 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 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.

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

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.

> + *
> + * Tries to acquire the direct mode lock with cleanup ACQUIRE() semantics and
> + * automatically releases it at the end of the scope. It most be always paired
> + * with IIO_DEV_ACQUIRE_ERR(), for example::
> + *
> + *	IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> + *	if (IIO_DEV_ACQUIRE_ERR(&claim))
> + *		return -EBUSY;
> + *
> + * ...or a more common scenario (notice scope the braces)::
> + *
> + *	switch() {
> + *	case IIO_CHAN_INFO_RAW: {
> + *		IIO_DEV_ACQUIRE_DIRECT_MODE(indio_dev, claim);
> + *		if (IIO_DEV_ACQUIRE_ERR(&claim))
> + *			return -EBUSY;
> + *
> + *		...
> + *	}
> + *	case IIO_CHAN_INFO_SCALE:
> + *		...
> + *	...
> + *	}
> + *
> + * Context: Can sleep
> + */
> +#define IIO_DEV_ACQUIRE_DIRECT_MODE(_dev, _var) \
> +	ACQUIRE(__priv__iio_dev_mode_lock_try_direct, _var)(_dev)
> +
> +/**
> + * IIO_DEV_ACQUIRE_BUFFER_MODE(_dev, _var) - Tries to acquire the buffer mode
> + *                                           lock with automatic release
> + * @_dev: IIO device instance
> + * @_var: Dummy variable identifier to store acquire result
> + *
> + * Tries to acquire the direct mode lock and automatically releases it at the
> + * end of the scope. It most be paired with IIO_DEV_ACQUIRE_ERR(), for example::
> + *
> + *	IIO_DEV_ACQUIRE_BUFFER_MODE(indio_dev, claim);
> + *	if (IIO_DEV_ACQUIRE_ERR(&claim))
> + *		return IRQ_HANDLED;
> + *
> + * Context: Can sleep
> + */
> +#define IIO_DEV_ACQUIRE_BUFFER_MODE(_dev, _var) \
> +	ACQUIRE(__priv__iio_dev_mode_lock_try_buffer, _var)(_dev)
> +
> +/**
> + * 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()?

> +
> +/**
> + * 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.

> + *
> + * 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()

> +	guard(__priv__iio_dev_mode_lock)(_dev)
> +
>  extern const struct bus_type iio_bus_type;
>  
>  /**
>