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: <D9VZV8APBYWU.2SWXJLHIQ18ZB@kernel.org>
Date: Wed, 14 May 2025 17:38:40 +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 4:40 PM CEST, Daniel Almeida wrote:
>> 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.

Hmm, so a `struct regulator` might already be enabled and calling
`regulator_enable` doesn't do anything?

> This means that Regulator<Disabled> is a misnomer.

Yeah.

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

You can't specialize the `Drop` impl, you'd have to do this:

    impl<State: RegulatorState> Drop for Regulator<State> {
        fn drop(&mut self) {
            if State::ENABLED {
                regulator_disable();
            }
            regulator_put();
        }
    }

I still think it's an improvement though.

---
Cheers,
Benno