linux-kernel - Re: [PATCH v9 1/3] rust: leds: add basic led classdev abstractions

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: <aR78ywVnpWaOEeJ-@google.com>
Date: Thu, 20 Nov 2025 11:34:35 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: Markus Probst <markus.probst@...teo.de>
Cc: Lee Jones <lee@...nel.org>, Pavel Machek <pavel@...nel.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Dave Ertman <david.m.ertman@...el.com>, 
	Ira Weiny <ira.weiny@...el.com>, Leon Romanovsky <leon@...nel.org>, 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 <lossin@...nel.org>, 
	Andreas Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>, 
	Danilo Krummrich <dakr@...nel.org>, "Rafael J. Wysocki" <rafael@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	"Krzysztof Wilczyński" <kwilczynski@...nel.org>, rust-for-linux@...r.kernel.org, 
	linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-pci@...r.kernel.org
Subject: Re: [PATCH v9 1/3] rust: leds: add basic led classdev abstractions

On Wed, Nov 19, 2025 at 02:11:21PM +0000, Markus Probst wrote:
> Implement the core abstractions needed for led class devices, including:
> 
> * `led::LedOps` - the trait for handling leds, including
>   `brightness_set`, `brightness_get` and `blink_set`
> 
> * `led::InitData` - data set for the led class device
> 
> * `led::Device` - a safe wrapper around `led_classdev`
> 
> Signed-off-by: Markus Probst <markus.probst@...teo.de>
> ---
>  MAINTAINERS        |   7 +
>  rust/kernel/led.rs | 472 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs |   1 +
>  3 files changed, 480 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b71ea515240a..80cb030911b7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14112,6 +14112,13 @@ F:	drivers/leds/
>  F:	include/dt-bindings/leds/
>  F:	include/linux/leds.h
>  
> +LED SUBSYSTEM [RUST]
> +M:	Markus Probst <markus.probst@...teo.de>
> +L:	linux-leds@...r.kernel.org
> +L:	rust-for-linux@...r.kernel.org
> +S:	Maintained
> +F:	rust/kernel/led.rs
> +
>  LEGO MINDSTORMS EV3
>  R:	David Lechner <david@...hnology.com>
>  S:	Maintained
> diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
> new file mode 100644
> index 000000000000..fca55f02be8d
> --- /dev/null
> +++ b/rust/kernel/led.rs
> @@ -0,0 +1,472 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Abstractions for the leds driver model.
> +//!
> +//! C header: [`include/linux/leds.h`](srctree/include/linux/leds.h)
> +
> +use core::{
> +    marker::PhantomData,
> +    mem::transmute,
> +    pin::Pin,
> +    ptr::NonNull, //
> +};
> +
> +use pin_init::{
> +    pin_data,
> +    pinned_drop,
> +    PinInit, //
> +};
> +
> +use crate::{
> +    build_error,
> +    container_of,
> +    device::{
> +        self,
> +        property::FwNode,
> +        AsBusDevice,
> +        Bound, //
> +    },
> +    devres::Devres,
> +    error::{
> +        code::EINVAL,
> +        from_result,
> +        to_result,
> +        Error,
> +        Result,
> +        VTABLE_DEFAULT_ERROR, //
> +    },
> +    macros::vtable,
> +    str::CStr,
> +    try_pin_init,
> +    types::{
> +        ARef,
> +        Opaque, //
> +    }, //
> +};

Please import kernel::prelude::* and remove all the imports that are
available from the prelude.

> +impl<'a> InitData<'a> {
> +    /// Sets the firmware node
> +    pub fn fwnode(self, fwnode: Option<ARef<FwNode>>) -> Self {

I'm thinking that perhaps this should just be a `&'a FwNode` instead?
That way, you can increment the refcount in Device::new() if
registration is successful.

> +        Self { fwnode, ..self }
> +    }
> +
> +    /// Sets a default label

There are many missing periods in doc-comments.

> +/// Trait defining the operations for a LED driver.
> +///
> +/// # Examples
> +///
> +///```
> +/// # use kernel::{
> +/// #     c_str, device, devres::Devres,
> +/// #     error::Result, led,
> +/// #     macros::vtable, platform, prelude::*,
> +/// # };
> +/// # use core::pin::Pin;
> +///
> +/// struct MyLedOps;

When using # in examples, please do not have an empty line before
beginning the example. It shows up as a weird extra empty line in the
rendered docs.

You could consider just making the imports displayed here also.

Also the ``` both above and below the example usually has a space:

/// ```

rather than

///```

> +                    // SAFETY:
> +                    // - `parent.as_raw()` is guaranteed to be a pointer to a valid `device`
> +                    //    or a null pointer.
> +                    // - `ptr` is guaranteed to be a pointer to an initialized `led_classdev`.
> +                    to_result(unsafe {
> +                        bindings::led_classdev_register_ext(
> +                            parent.as_ref().as_raw(),
> +                            ptr,
> +                            &mut init_data_raw,
> +                        )
> +                    })?;
> +
> +                    core::mem::forget(init_data.fwnode); // keep the reference count incremented

This led abstraction implicitly takes a refcount on the fwnode and then
drops it when the device is unbound.

Lee, can you confirm that taking a refcount on the fwnode is the right
way to use the LED subsytem?

Alice