Re: [PATCH RFC 3/6] iio: core: Add cleanup.h support for iio_device_claim_*()

["Kurt Borja" <kuurtb@gmail.com>]

On Wed Dec 3, 2025 at 5:34 PM -05, David Lechner wrote:
> On 12/3/25 3:50 PM, David Lechner wrote:
>> On 12/3/25 1:18 PM, Kurt Borja wrote:
>>> Add guard() and ACQUIRE() support for iio_device_claim_*() lock.
>>>
>>> This involves exporting iio_device_{claim, release}() wrappers to define
>>> a general GUARD class, and then defining the _direct and _buffer
>>> conditional ones.
>> 
>> Commit messages should say why we need this.
>> 
>> Also, this seems like two separate things. Adding a new claim/release pair
>> and adding the conditional guard support to the existing ones. So perhaps
>> better as two separate patches.
>> 
>>>
>>> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>>> Signed-off-by: Kurt Borja <kuurtb@gmail.com>
>>> ---
>>>  drivers/iio/industrialio-core.c | 12 ++++++++++++
>>>  include/linux/iio/iio.h         | 20 ++++++++++++++++++++
>>>  2 files changed, 32 insertions(+)
>>>
>>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>>> index adf0142d0300..da090c993fe8 100644
>>> --- a/drivers/iio/industrialio-core.c
>>> +++ b/drivers/iio/industrialio-core.c
>>> @@ -2171,6 +2171,18 @@ int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>>>  }
>>>  EXPORT_SYMBOL_GPL(__devm_iio_device_register);
>>>  
>>> +void __iio_device_claim(struct iio_dev *indio_dev)
>>> +{
>>> +	mutex_lock(&to_iio_dev_opaque(indio_dev)->mlock);
>>> +}
>>> +EXPORT_SYMBOL_GPL(__iio_device_claim);
>>> +
>>> +void __iio_device_release(struct iio_dev *indio_dev)
>>> +{
>>> +	mutex_unlock(&to_iio_dev_opaque(indio_dev)->mlock);
>>> +}
>>> +EXPORT_SYMBOL_GPL(__iio_device_release);
>>> +
>>>  /**
>>>   * __iio_device_claim_direct - Keep device in direct mode
>>>   * @indio_dev:	the iio_dev associated with the device
>>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>>> index 27da9af67c47..472b13ec28d3 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>
>>> @@ -661,9 +662,23 @@ void iio_device_unregister(struct iio_dev *indio_dev);
>>>  int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev,
>>>  			       struct module *this_mod);
>>>  int iio_push_event(struct iio_dev *indio_dev, u64 ev_code, s64 timestamp);
>>> +void __iio_device_claim(struct iio_dev *indio_dev);
>>> +void __iio_device_release(struct iio_dev *indio_dev);
>>>  bool __iio_device_claim_direct(struct iio_dev *indio_dev);
>>>  void __iio_device_release_direct(struct iio_dev *indio_dev);
>>>  
>>> +static inline void iio_device_claim(struct iio_dev *indio_dev)
>>> +	__acquires(indio_dev)
>>> +{
>>> +	__iio_device_claim(indio_dev);
>>> +}
>>> +
>>> +static inline void iio_device_release(struct iio_dev *indio_dev)
>>> +	__releases(indio_dev)
>>> +{
>>> +	__iio_device_release(indio_dev);
>>> +}
>> 
>> It was unfortunate that we had to drop "mode" from iio_device_claim_direct_mode()
>> during the recent API change, but at least it is fairly obvious that "direct"
>> is a mode. Here, dropping "mode" from the name hurts the understanding. These
>> could also use some documentation comments to explain what these are for and
>> when it is appropriate to use them. I had to really dig around the code to
>> come to the understanding that these mean "don't allow switching modes until
>> we release the claim".

I agree.

>> 
>> I would call it something like iio_device_{claim,release}_current_mode().
>> 
>> 
>>> +
>>>  /*
>>>   * Helper functions that allow claim and release of direct mode
>>>   * in a fashion that doesn't generate many false positives from sparse.
>>> @@ -690,6 +705,11 @@ static inline void iio_device_release_direct(struct iio_dev *indio_dev)
>>>  bool iio_device_claim_buffer(struct iio_dev *indio_dev);
>>>  void iio_device_release_buffer(struct iio_dev *indio_dev);
>>>  
>>> +DEFINE_GUARD(iio_device_claim, struct iio_dev *, iio_device_claim(_T),
>>> +	     iio_device_release(_T));
>>> +DEFINE_GUARD_COND(iio_device_claim, _buffer, iio_device_claim_buffer(_T));
>>> +DEFINE_GUARD_COND(iio_device_claim, _direct, iio_device_claim_direct(_T));
>>> +
>
> When I made the comments about keeping "mode" in the name, I forgot
> that DEFINE_GUARD_COND() only extends a DEFINE_GUARD(). So I understand
> if we need to make names that fit a certain pattern rather than what
> I suggested.
>
> Still would be nice to have:
>
> iio_device_claim_mode()
> iio_device_claim_mode_direct()
> iio_device_claim_mode_buffer()
> iio_device_release_mode()
>
> Just really annoying to rename iio_device_{claim,release}_direct()
> everywhere since we just did that. We could keep both names around
> for a while though to avoid the churn.

If we rename iio_device_claim_direct() (which is huge), maybe we can
pick shorter names and more descriptive names while at it? I was
thinking something like:

	iio_mode_lock()
	iio_mode_lock_direct()
	iio_mode_lock_buffer()
	iio_mode_unlock()

Shorter names will also keep lines short when using guards.

>
> It also means that we should remove __iio_device_release_direct() and
> iio_device_release_buffer_mode() to make it clear that there is only
> a single "release" function used by all variants of "claim".

I agree.

-- 
 ~ Kurt