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

[David Lechner <dlechner@baylibre.com>]

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

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".

>>  extern const struct bus_type iio_bus_type;
>>  
>>  /**
>>
>