linux-kernel - Re: [PATCH RESEND v2] rust: regulator: add a bare minimum regulator abstraction

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: <0698A75E-D43C-4D02-B734-BFE1B3CC5D34@collabora.com>
Date: Wed, 26 Mar 2025 16:49:26 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Mark Brown <broonie@...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>,
 lgirdwood@...il.com,
 linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH RESEND v2] rust: regulator: add a bare minimum regulator
 abstraction



> On 26 Mar 2025, at 15:56, Mark Brown <broonie@...nel.org> wrote:
> 
> On Wed, Mar 26, 2025 at 03:39:33PM -0300, Daniel Almeida wrote:
> 
> This is flagged as a resend but appears to be the first copy I've
> seen...

Hi Mark, you were not on cc earlier this morning, which is why this is being
resent. I left a comment about this, but apparently b4 did not pick it up.

>> Add a bare minimum regulator abstraction to be used by Rust drivers.
>> This abstraction adds a small subset of the regulator API, which is
>> thought to be sufficient for the drivers we have now.
> 
>> +//! Regulators are modeled with two types: [`Regulator`] and
>> +//! [`EnabledRegulator`].
> 
> It would be helpful to see what the client code actually looks like
> here.

Ack, I'll include examples in v3.

> 
>> +    /// 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 })
>> +    }
> 
> I assume this map does soemthing to report errors?

map() returns the error to the caller, if any. Notice that the return type is
Result<EnabledRegulator>.

> 
>> +impl EnabledRegulator {
> 
>> +    /// Disables the regulator.
>> +    pub fn disable(self) -> Result<Regulator> {
>> +        // Keep the count on `regulator_get()`.
>> +        let regulator = ManuallyDrop::new(self);
>> +
>> +        // 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,
>> +        })
>> +    }
> 
> This looks like user code could manually call it which feels like asking
> for trouble?

Yes, user code can call this. My understanding is that drivers may want to
disable the regulator at runtime, possibly to save power when the device is
idle?

What trouble are you referring to?


>> +    /// Sets the voltage for the regulator.
>> +    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.as_ptr(), min_uv.0, max_uv.0) })
>> +    }
> 
> set_voltage() is only implemented for enabled regulators.  It is
> reasonable for a driver to want to set the voltage for a regulator prior
> to enabling it in order to ensure that the device powers up cleanly,
> there may be different requirements for power on from on and idle so the
> constraints may not be enough to ensure that a device can power on
> cleanly.
> 
>> +    /// 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.as_ptr()) };
>> +        if voltage < 0 {
>> +            Err(Error::from_errno(voltage))
>> +        } else {
>> +            Ok(Microvolt(voltage))
>> +        }
>> +    }
> 
> Same issue here.

Ok, we can move the implementation to Regulator then.

— Daniel