linux-kernel - Re: [PATCH 2/2] rust: regulator: add devm_regulator_get

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] [thread-next>] [day] [month] [year] [list]
Message-Id: <DB27F561-30AD-400E-811B-19CEAE511CFA@collabora.com>
Date: Sat, 30 Aug 2025 10:13:45 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: Liam Girdwood <lgirdwood@...il.com>,
 Mark Brown <broonie@...nel.org>,
 Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>,
 Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 Benno Lossin <lossin@...nel.org>,
 Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>,
 Trevor Gross <tmgross@...ch.edu>,
 Danilo Krummrich <dakr@...nel.org>,
 linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 2/2] rust: regulator: add devm_regulator_get_enable API

Hi Alex,

> On 30 Aug 2025, at 02:20, Alexandre Courbot <acourbot@...dia.com> wrote:
> 
> On Sat Aug 30, 2025 at 6:11 AM JST, Daniel Almeida wrote:
>> A lot of drivers only care about enabling the regulator for as long as
>> the underlying Device is bound. This can be easily observed due to the
>> extensive use of `devm_regulator_get_enable` and
>> `devm_regulator_get_enable_optional` throughout the kernel.
>> 
>> Therefore, make this helper available in Rust. Also add an example
>> noting how it should be the default API unless the driver needs more
>> fine-grained control over the regulator.
>> 
>> Suggested-by: Danilo Krummrich <dakr@...nel.org>
>> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>
>> ---
>> rust/helpers/regulator.c | 10 ++++++++++
>> rust/kernel/regulator.rs | 42 +++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 51 insertions(+), 1 deletion(-)
>> 
>> diff --git a/rust/helpers/regulator.c b/rust/helpers/regulator.c
>> index cd8b7ba648ee33dd14326c9242fb6c96ab8e32a7..11bc332443bd064f4b5afd350ffc045badff9076 100644
>> --- a/rust/helpers/regulator.c
>> +++ b/rust/helpers/regulator.c
>> @@ -40,4 +40,14 @@ int rust_helper_regulator_is_enabled(struct regulator *regulator)
>> return regulator_is_enabled(regulator);
>> }
>> 
>> +int rust_helper_devm_regulator_get_enable(struct device *dev, const char *id)
>> +{
>> + return devm_regulator_get_enable(dev, id);
>> +}
>> +
>> +int rust_helper_devm_regulator_get_enable_optional(struct device *dev, const char *id)
>> +{
>> + return devm_regulator_get_enable_optional(dev, id);
>> +}
>> +
>> #endif
>> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
>> index 60993373f4d911f4f0cbec2510f0c67efa24a51b..73d4c9b56dca9c676793d78e35e5758d18eef3e8 100644
>> --- a/rust/kernel/regulator.rs
>> +++ b/rust/kernel/regulator.rs
>> @@ -18,7 +18,7 @@
>> 
>> use crate::{
>>     bindings,
>> -    device::Device,
>> +    device::{Bound, Device},
>>     error::{from_err_ptr, to_result, Result},
>>     prelude::*,
>> };
>> @@ -70,6 +70,26 @@ pub struct Error<State: RegulatorState> {
>>     pub regulator: Regulator<State>,
>> }
>> 
>> +/// Enables a regulator whose lifetime is tied to the lifetime of `dev`.
>> +///
>> +/// This calls `regulator_disable()` and `regulator_put()` automatically on
>> +/// driver detach.
>> +///
>> +/// This API is identical to `devm_regulator_get_enable()`, and should be
>> +/// preferred if the caller only cares about the regulator being on.
>> +pub fn enable(dev: &Device<Bound>, name: &CStr) -> Result {
> 
> The name `enable` sounds like it just enables a regulator, which is a bit
> confusing IMHO. Maybe `get_enable` or `get_enable_for`? Not sure what
> would be idiomatic here.

So I thought about get_enabled, but I thought the "get" nomenclature was
confusing. For example, "get" acquires a refcount, but for the devm_ version
the refcount management is transparent. In this sense, I thought that just
"enable" would convey the idea better, i.e. "enable this and forget about any
lifetime management at all".

If you still think that using the "get" prefix is better, I can change it no
worries :)

> 
>> +    // SAFETY: `dev` is a valid and bound device, while `name` is a valid C
>> +    // string.
>> +    to_result(unsafe { bindings::devm_regulator_get_enable(dev.as_raw(), name.as_ptr()) })
>> +}
>> +
>> +/// Same as [`enable`], but calls `devm_regulator_get_enable_optional` instead.
> 
> Maybe explain in one sentence what `devm_regulator_get_enable_optional`
> as it might not be completely obvious.

Perhaps adding a link?

> 
>> +pub fn enable_optional(dev: &Device<Bound>, name: &CStr) -> Result {
>> +    // SAFETY: `dev` is a valid and bound device, while `name` is a valid C
>> +    // string.
>> +    to_result(unsafe { bindings::devm_regulator_get_enable_optional(dev.as_raw(), name.as_ptr()) })
>> +}
>> +
>> /// A `struct regulator` abstraction.
>> ///
>> /// # Examples
>> @@ -146,6 +166,26 @@ pub struct Error<State: RegulatorState> {
>> /// }
>> /// ```
>> ///
>> +/// If a driver only cares about the regulator being on for as long it is bound
>> +/// to a device, then it should use [`regulator::get_enabled`] or
>> +/// [`regulator::get_enabled_optional`]. This should be the default use-case
> 
> I suppose you mean `enable` and `enable_optional` instead of
> `get_enabled` and `get_enabled_optional` (although I personally would
> favor the latter :)).

Hmm, something happened here. I always make sure to run rustdoc before
submitting, and it did not error out even though this function does not exist.

In any case, my bad.

— Daniel