Message-Id: <D9VXPNT4HNXP.1PKET0Q1H7O9Y@kernel.org>
Date: Wed, 14 May 2025 15:57:22 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Daniel Almeida" <daniel.almeida@...labora.com>
Cc: "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" <benno.lossin@...ton.me>,
 "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Danilo
 Krummrich" <dakr@...nel.org>, "Boris Brezillon"
 <boris.brezillon@...labora.com>, "Sebastian Reichel"
 <sebastian.reichel@...labora.com>, "Liam Girdwood" <lgirdwood@...il.com>,
 "Mark Brown" <broonie@...nel.org>, <linux-kernel@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v3] rust: regulator: add a bare minimum regulator
 abstraction

On Wed May 14, 2025 at 3:01 PM CEST, Daniel Almeida wrote:
>> On 13 May 2025, at 17:01, Benno Lossin <lossin@...nel.org> wrote:
>> On Tue May 13, 2025 at 5:44 PM CEST, Daniel Almeida wrote:
>>> diff --git a/rust/kernel/regulator.rs b/rust/kernel/regulator.rs
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..7b07b64f61fdd4a84ffb38e9b0f90830d5291ab9
>>> --- /dev/null
>>> +++ b/rust/kernel/regulator.rs
>>> @@ -0,0 +1,211 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>> +//! Regulator abstractions, providing a standard kernel interface to control
>>> +//! voltage and current regulators.
>>> +//!
>>> +//! The intention is to allow systems to dynamically control regulator power
>>> +//! output in order to save power and prolong battery life. This applies to both
>>> +//! voltage regulators (where voltage output is controllable) and current sinks
>>> +//! (where current limit is controllable).
>>> +//!
>>> +//! C header: [`include/linux/regulator/consumer.h`](srctree/include/linux/regulator/consumer.h)
>>> +//!
>>> +//! Regulators are modeled in Rust with two types: [`Regulator`] and
>>> +//! [`EnabledRegulator`].
>> 
>> Would it make sense to store this in a generic variable acting as a type
>> state instead of using two different names? So:
>> 
>>    pub struct Regulator<State: RegulatorState> { /* ... */ }
>> 
>>    pub trait RegulatorState: private::Sealed {}
>> 
>>    pub struct Enabled;
>>    pub struct Disabled;
>> 
>>    impl RegulatorState for Enabled {}
>>    impl RegulatorState for Disabled {}
>> 
>> And then one would use `Regulator<Enabled>` and `Regulator<Disabled>`.
>
> This seems like just another way of doing the same thing.
>
> I have nothing against a typestate, it's an elegant solution really, but so is
> the current one. I'd say let's keep what we have unless there is something
> objectively better about a typestatethat makes it worthy to change this.

I'd say it's cleaner and we already have some APIs that utilize type
states, so I'd prefer we use that where it makes sense.

>>> +/// # Invariants
>>> +///
>>> +/// - [`Regulator`] is a non-null wrapper over a pointer to a `struct
>>> +///   regulator` obtained from [`regulator_get()`](https://docs.kernel.org/driver-api/regulator.html#c.regulator_get).
>> 
>> This should be "`inner` is a pointer obtained from
>> [`regulator_get()`](...)".
>> 
>>> +/// - Each instance of [`Regulator`] is associated with a single count of
>>> +///   [`regulator_get()`](https://docs.kernel.org/driver-api/regulator.html#c.regulator_get).
>> 
>> This is redundant, so we should remove it.
>
> Why is this redundant? It says that we are associated with a *single* refcount.

I'd fold it into the previous bullet point. To me they aren't really
disjoint.

>>> +/// A [`Regulator`] that is known to be enabled.
>>> +///
>>> +/// # Invariants
>>> +///
>>> +/// - [`EnabledRegulator`] is a valid regulator that has been enabled.
>> 
>> This isn't fully clear what it's supposed to mean to me. Maybe mention
>> the `regulator_enable` function?
>> 
>>> +/// - Each instance of [`EnabledRegulator`] is associated with a single count
>>> +///   of [`regulator_enable()`](https://docs.kernel.org/driver-api/regulator.html#c.regulator_enable)
>>> +///   that was obtained from the [`Regulator`] instance once it was enabled.
>> 
>> Ah I see you mention it here, then what is the bullet point above about?
>
> Again, the word _single_ is what is important here.

I don't see the point in stating it in two separate bullet points.

>>> +pub struct EnabledRegulator {
>>> +    inner: Regulator,
>>> +}
>>> +
>>> +impl EnabledRegulator {
>>> +    fn as_ptr(&self) -> *mut bindings::regulator {
>>> +        self.inner.inner.as_ptr()
>>> +    }
>>> +
>>> +    /// Disables the regulator.
>>> +    pub fn disable(self) -> Result<Regulator> {
>>> +        // Keep the count on `regulator_get()`.
>>> +        let regulator = ManuallyDrop::new(self);
>> 
>> Why don't we drop the refcount if the `regulator_disable` call fails?
>
> I am operating under the assumption that regulator_enable() and
> regulator_disable() do not touch the reference count. Note that we do not
> acquire a new reference when we build EnabledRegulator in Regulator::enable(),
> we merely move our instance of Regulator into EnabledRegulator.
>
> disable() takes EnabledRegulator by value in order to convert it to Regulator.
> If we let the destructor run, that will decrement the refcount as it calls
> inner.drop(), so that is why the ManuallyDrop is there in the first place.  
>
> Now if disable() fails, perhaps we should somehow return `self` to the caller.
> That would let them retry the operation, even if it's unlikely to be of any
> help, as Mark said. In this sense, I agree that there is a leak that I
> overlooked.

I see two different options on what to do about the leak:

(1) implement `disable` in terms of a new function `try_disable`
    returning `Result<Regulator, DisableError>` where `DisableError`
    gives access to the `EnabledRegulator`.

(2) document that the `disable` function leaks the refcount if an error
    is returned.

I have a slight preference for (1), but if Mark & you decide that it's
not necessary, since users of regulators won't need it any time soon,
then go with (2).

> On a second note, Benno, do you have a clean way to return both kernel::Error
> and `self` here? I assume that introducing a separate error type just for this
> function is overkill.

It's pretty common in Rust to have a custom error type for those cases.
I think we should also do it here, Alice already used the pattern in the
alloc module [1].

[1]: https://lore.kernel.org/all/20250502-vec-methods-v5-3-06d20ad9366f@google.com

>>> +
>>> +        // SAFETY: Safe as per the type invariants of `Self`.
>>> +        let res = to_result(unsafe { bindings::regulator_disable(regulator.as_ptr()) });
>>> +
>>> +        res.map(|()| Regulator {
>>> +            inner: regulator.inner.inner,
>>> +        })
>>> +    }
>>> +
>>> +    /// Sets the voltage for the regulator.
>>> +    pub fn set_voltage(&self, min_uv: Microvolt, max_uv: Microvolt) -> Result {
>>> +        self.inner.set_voltage(min_uv, max_uv)
>>> +    }
>>> +
>>> +    /// Gets the current voltage of the regulator.
>>> +    pub fn get_voltage(&self) -> Result<Microvolt> {
>>> +        self.inner.get_voltage()
>>> +    }
>>> +}
>>> +
>>> +impl Drop for EnabledRegulator {
>>> +    fn drop(&mut self) {
>>> +        // SAFETY: By the type invariants, we know that `self` owns a reference,
>>> +        // so it is safe to relinquish it now.
>>> +        unsafe { bindings::regulator_disable(self.as_ptr()) };
>> 
>> Same here, what happens to the refcount?
>
> It remains the same, we never acquired one when we enabled, so we are relying
> on inner.drop() to decrement it.

Ah I overlooked that. Then it's fine.

---
Cheers,
Benno

Powered by blists - more mailing lists