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: <CAH5fLgj_eKuo=E7HPgmd1bJNfidGUS37MM1QqRaQ_MJ2kTgCmg@mail.gmail.com>
Date: Wed, 2 Jul 2025 12:35:59 +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 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?

> +/// ## Using [`Regulator<Dynamic>`]
> +///
> +/// This example mimics the behavior of the C API, where the user is in
> +/// control of the enabled reference count. This is useful for drivers that
> +/// might call enable and disable to manage the `enable` reference count at
> +/// runtime, perhaps as a result of `open()` and `close()` calls or whatever
> +/// other driver-specific or subsystem-specific hooks.
> +///
> +/// ```
> +/// # use kernel::prelude::*;
> +/// # use kernel::c_str;
> +/// # use kernel::device::Device;
> +/// # use kernel::regulator::{Regulator, Dynamic};
> +/// struct PrivateData {
> +///     regulator: Regulator<Dynamic>,
> +/// }
> +///
> +/// // A fictictious probe function that obtains a regulator and sets it up.
> +/// fn probe(dev: &Device, data: &mut PrivateData) -> Result<PrivateData> {

The `data` argument is unused. It looks like it should be removed.

> +pub struct Regulator<State = Dynamic>
> +where
> +    State: RegulatorState + 'static,

You can add `: 'static` to the trait itself to avoid having it here.

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