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: <CAH5fLgg1-K6x0wRO79mt4fGvJeNanr9Km75=1Z9G0gy-WbeU2g@mail.gmail.com>
Date: Wed, 2 Jul 2025 15:12:08 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
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>, 
	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

On Wed, Jul 2, 2025 at 3:03 PM Daniel Almeida
<daniel.almeida@...labora.com> wrote:
>
> 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.

Okay.

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

Along these lines:

pub trait RegulatorState: 'static {
    const DISABLE_ON_DROP: bool;
}

impl RegulatorState for Enabled {
    const DISABLE_ON_DROP: bool = true;
}
impl RegulatorState for Disabled {
    const DISABLE_ON_DROP: bool = false;
}
impl RegulatorState for Dynamic {
    const DISABLE_ON_DROP: bool = false;
}

impl<T: RegulatorState> Drop for Regulator<T> {
    fn drop(&mut self) {
        if T::DISABLE_ON_DROP {
            unsafe { bindings::regulator_disable(self.inner.as_ptr()) };
        }
        unsafe { bindings::regulator_put(self.inner.as_ptr()) };
    }
}