linux-kernel - Re: [PATCH v3] 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: <52CFFCA2-F253-49F1-9EA5-2865BD094B25@collabora.com>
Date: Wed, 14 May 2025 11:40:15 -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



> On 14 May 2025, at 10:57, Benno Lossin <lossin@...nel.org> wrote:
> 
> 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.
> 

By the way, IIUC, regulator_disable() does not disable a regulator necessarily.
It just tells the system that you don't care about it being enabled anymore. It can
still remain on if there are other users.

This means that Regulator<Disabled> is a misnomer.

Also, the current solution relies on Regulator being a member of
EnabledRegulator to keep the refcounts sane. I wonder how that is going to work
now that Regulator<Disabled> is obviously not a member of Regulator<Enabled>, i.e.:

impl Drop for Regulator<Enabled> {
 fn drop(&mut self) {
  regulator_disable();  
  
  // We now have to call this explicitly, because no one else will call it for
  // us.
  regulator_put();
 }
}

impl Drop for Regulator<Disabled> {
 fn drop(&mut self) {
  // We now have to repeat this in both destructors.
  regulator_put();  
 }
}

Just to confirm: is that what you have in mind?

— Daniel