Message-Id: <B288AFB1-BA0A-4383-9823-EAC9E5DCA59F@collabora.com>
Date: Wed, 14 May 2025 10:01:26 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Benno Lossin <lossin@...nel.org>
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

Hi Benno,

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

> 
>> +//!
>> +//! The transition between these types is done by calling
>> +//! [`Regulator::enable()`] and [`EnabledRegulator::disable()`] respectively.
>> +//!
>> +//! Use an enum or [`kernel::types::Either`] to gracefully transition between
>> +//! the two states at runtime if needed. Store [`EnabledRegulator`] directly
>> +//! otherwise.
>> +//!
>> +//! See [`Voltage and current regulator API`]("https://docs.kernel.org/driver-api/regulator.html")
>> +//! for more information.
>> +
>> +use crate::{
>> +    bindings,
>> +    device::Device,
>> +    error::{from_err_ptr, to_result, Result},
>> +    prelude::*,
>> +};
>> +
>> +use core::{mem::ManuallyDrop, ptr::NonNull};
>> +
>> +/// A `struct regulator` abstraction.
>> +///
>> +/// # Examples
>> +///
>> +/// Enabling a regulator:
>> +///
>> +/// ```
>> +/// # use kernel::prelude::*;
>> +/// # use kernel::c_str;
>> +/// # use kernel::device::Device;
>> +/// # use kernel::regulator::{Microvolt, Regulator, EnabledRegulator};
>> +/// fn enable(dev: &Device, min_uv: Microvolt, max_uv: Microvolt) -> Result {
>> +///    // Obtain a reference to a (fictitious) regulator.
>> +///    let regulator: Regulator = Regulator::get(dev, c_str!("vcc"))?;
>> +///
>> +///    // The voltage can be set before enabling the regulator if needed, e.g.:
>> +///    regulator.set_voltage(min_uv, max_uv)?;
>> +///
>> +///    // The same applies for `get_voltage()`, i.e.:
>> +///    let voltage: Microvolt = regulator.get_voltage()?;
>> +///
>> +///    // Enables the regulator, consuming the previous value.
>> +///    //
>> +///    // From now on, the regulator is known to be enabled because of the type
>> +///    // `EnabledRegulator`.
>> +///    let regulator: EnabledRegulator = regulator.enable()?;
>> +///
>> +///    // The voltage can also be set after enabling the regulator, e.g.:
>> +///    regulator.set_voltage(min_uv, max_uv)?;
>> +///
>> +///    // The same applies for `get_voltage()`, i.e.:
>> +///    let voltage: Microvolt = regulator.get_voltage()?;
>> +///
>> +///    // Dropping an enabled regulator will disable it. The refcount will be
>> +///    // decremented.
> 
> Where would you normally store an enabled regulator to keep it alive?
> Maybe adjust the example to do just that?

In your driver’s private data.

> 
>> +///    drop(regulator);
>> +///    // ...
>> +///    # Ok::<(), Error>(())
>> +/// }
>> +///```
>> +///
>> +/// Disabling a regulator:
>> +///
>> +///```
>> +/// # use kernel::prelude::*;
>> +/// # use kernel::c_str;
>> +/// # use kernel::device::Device;
>> +/// # use kernel::regulator::{Microvolt, Regulator, EnabledRegulator};
>> +/// fn disable(dev: &Device, regulator: EnabledRegulator) -> Result {
>> +///    // We can also disable an enabled regulator without reliquinshing our
>> +///    // refcount:
>> +///    let regulator: Regulator = regulator.disable()?;
>> +///
>> +///    // The refcount will be decremented when `regulator` is dropped.
>> +///    drop(regulator);
>> +///    // ...
>> +///    # Ok::<(), Error>(())
>> +/// }
>> +/// ```
>> +///
>> +/// # 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.

> 
>> +pub struct Regulator {
>> +    inner: NonNull<bindings::regulator>,
>> +}
>> +
>> +impl Regulator {
>> +    /// Obtains a [`Regulator`] instance from the system.
>> +    pub fn get(dev: &Device, name: &CStr) -> Result<Self> {
>> +        // SAFETY: It is safe to call `regulator_get()`, on a device pointer
>> +        // received from the C code.
>> +        let inner = from_err_ptr(unsafe { bindings::regulator_get(dev.as_raw(), name.as_ptr()) })?;
>> +
>> +        // SAFETY: We can safely trust `inner` to be a pointer to a valid
>> +        // regulator if `ERR_PTR` was not returned.
>> +        let inner = unsafe { NonNull::new_unchecked(inner) };
>> +
>> +        Ok(Self { inner })
>> +    }
>> +
>> +    /// Enables the regulator.
>> +    pub fn enable(self) -> Result<EnabledRegulator> {
>> +        // SAFETY: Safe as per the type invariants of `Regulator`.
>> +        let res = to_result(unsafe { bindings::regulator_enable(self.inner.as_ptr()) });
>> +        res.map(|()| EnabledRegulator { inner: self })
>> +    }
>> +
>> +    /// Sets the voltage for the regulator.
>> +    ///
>> +    /// This can be used to ensure that the device powers up cleanly.
>> +    pub fn set_voltage(&self, min_uv: Microvolt, max_uv: Microvolt) -> Result {
>> +        // SAFETY: Safe as per the type invariants of `Regulator`.
>> +        to_result(unsafe {
>> +            bindings::regulator_set_voltage(self.inner.as_ptr(), min_uv.0, max_uv.0)
>> +        })
>> +    }
>> +
>> +    /// Gets the current voltage of the regulator.
>> +    pub fn get_voltage(&self) -> Result<Microvolt> {
>> +        // SAFETY: Safe as per the type invariants of `Regulator`.
>> +        let voltage = unsafe { bindings::regulator_get_voltage(self.inner.as_ptr()) };
>> +        if voltage < 0 {
>> +            Err(Error::from_errno(voltage))
>> +        } else {
>> +            Ok(Microvolt(voltage))
>> +        }
>> +    }
>> +}
>> +
>> +impl Drop for Regulator {
>> +    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_put(self.inner.as_ptr()) };
>> +    }
>> +}
>> +
>> +/// 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.

> 
> Also, please refer to `inner` instead of [`EnabledRegulator`].
> 
>> +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.

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.


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

> 
> ---
> Cheers,
> Benno
> 
>> +    }
>> +}
>> +
>> +/// A voltage in microvolts.
>> +///
>> +/// The explicit type is used to avoid confusion with other multiples of the
>> +/// volt, which can be desastrous.
>> +#[repr(transparent)]
>> +#[derive(Copy, Clone, PartialEq, Eq)]
>> +pub struct Microvolt(pub i32);
>> 
>> ---
>> base-commit: edc5e6e019c99b529b3d1f2801d5cce9924ae79b
>> change-id: 20250326-topics-tyr-regulator-e8b98f6860d7
>> 
>> Best regards,
> 
> 


— Daniel

Powered by blists - more mailing lists