| 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
| ||
|
Message-ID: <bf1c82f7-da25-47b6-846d-9f8427ee5790@baylibre.com>
Date: Wed, 3 Dec 2025 16:34:22 -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 RFC 3/6] iio: core: Add cleanup.h support for
iio_device_claim_*()
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@...el.com>
>> Signed-off-by: Kurt Borja <kuurtb@...il.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;
>>
>> /**
>>
>
Powered by blists - more mailing lists