linux-kernel - Re: [PATCH v6 1/2] 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: <1DFECD1D-9445-4D59-A727-37C527B5DFF5@collabora.com>
Date: Wed, 2 Jul 2025 10:03:08 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Alice Ryhl <aliceryhl@...gle.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>,
 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 v6 1/2] rust: regulator: add a bare minimum regulator
 abstraction

Hi Alice,

> On 2 Jul 2025, at 07:35, Alice Ryhl <aliceryhl@...gle.com> wrote:
> 
> On Fri, Jun 27, 2025 at 7:11 PM Daniel Almeida
> <daniel.almeida@...labora.com> wrote:
>> 
>> 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 provide the power needed by many hardware blocks and thus are
>> likely to be needed by a lot of drivers.
>> 
>> It was tested on rk3588, where it was used to power up the "mali"
>> regulator in order to power up the GPU.
>> 
>> Signed-off-by: Daniel Almeida <daniel.almeida@...labora.com>
> 
> Overall looks reasonable to me.
> 
>> +/// A trait that abstracts the ability to check if a [`Regulator`] is enabled.
>> +pub trait IsEnabled: RegulatorState {}
>> +impl IsEnabled for Disabled {}
>> +impl IsEnabled for Dynamic {}
> 
> Naming-wise, it's a bit weird that IsEnabled applies to everything
> *but* enabled. And also, the is_enabled() method should probably exist
> for only Dynamic anyway?

I think it's the other way around? Enabled doesn't need this impl precisely
because of the Enabled token. IOW:

Regulator<Enabled>::is_enabled() doesn't make sense.

>  And also, the is_enabled() method should probably exist for only Dynamic anyway?

Also no, because Regulator<Disabled> isn't necessarily disabled. It just was
not enabled by us, but might have been enabled by somebody else in the system.


[…]

> 
> 
>> +impl<T: RegulatorState + 'static> Drop for Regulator<T> {
>> +    fn drop(&mut self) {
>> +        if core::any::TypeId::of::<T>() == core::any::TypeId::of::<Enabled>() {
> 
> I would avoid this kind of logic. Instead, you can add an
> `disable_on_drop()` method or constant to the trait and check it here.
> 
> Alice
> 

Can you expand on this?

— Daniel