| 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: <DEPLQT84HBAO.2GAY5BHP05HNL@gmail.com>
Date: Thu, 04 Dec 2025 12:18:31 -0500
From: "Kurt Borja" <kuurtb@...il.com>
To: "David Lechner" <dlechner@...libre.com>, "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 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@...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 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
Powered by blists - more mailing lists