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: <20251227145312.0f1c5f87@jic23-huawei>
Date: Sat, 27 Dec 2025 14:53:12 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Kurt Borja <kuurtb@...il.com>, Andy Shevchenko
 <andriy.shevchenko@...el.com>, Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>, 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>,
 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, 23 Dec 2025 11:23:24 -0600
David Lechner <dlechner@...libre.com> 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.
> 
Given David did a much more thorough review than me I've just
added a few comments on top.

Jonathan

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

This one doesn't actually have any users I think?  Do we want to introduce
it without them? I'm not sure we do.

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

Agreed. That's a clearer name. 

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