Re: [PATCH v21 1/3] rust: leds: add basic led classdev abstractions

On Thu, Jul 02, 2026 at 11:00:30AM +0000, Markus Probst wrote:
> On Thu, 2026-07-02 at 10:47 +0000, Alice Ryhl wrote:
> > On Mon, Jun 29, 2026 at 01:10:28PM +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::DeviceBuilder` - the builder for the led class device
> > > 
> > > * `led::Device` - a safe wrapper around `led_classdev`
> > > 
> > > Signed-off-by: Markus Probst <[email protected]>
> > > ---
> > >  MAINTAINERS               |   8 ++
> > >  rust/kernel/led.rs        | 288 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  rust/kernel/led/normal.rs | 230 ++++++++++++++++++++++++++++++++++++
> > >  rust/kernel/lib.rs        |   1 +
> > >  4 files changed, 527 insertions(+)
> > > 
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 15011f5752a9..ceb2285366ff 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -14662,6 +14662,14 @@ F:	drivers/leds/
> > >  F:	include/dt-bindings/leds/
> > >  F:	include/linux/leds.h
> > >  
> > > +LED SUBSYSTEM [RUST]
> > > +M:	Markus Probst <[email protected]>
> > > +L:	[email protected]
> > > +L:	[email protected]
> > > +S:	Maintained
> > > +F:	rust/kernel/led.rs
> > > +F:	rust/kernel/led/
> > > +
> > >  LEGO MINDSTORMS EV3
> > >  R:	David Lechner <[email protected]>
> > >  S:	Maintained
> > > diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
> > > new file mode 100644
> > > index 000000000000..c92d99d68497
> > > --- /dev/null
> > > +++ b/rust/kernel/led.rs
> > > @@ -0,0 +1,288 @@
> > > +// 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,
> > > +    ptr::NonNull, //
> > > +};
> > > +
> > > +use crate::{
> > > +    container_of,
> > > +    device::{
> > > +        self,
> > > +        property::FwNode,
> > > +        AsBusDevice,
> > > +        Bound, //
> > > +    },
> > > +    error::{
> > > +        from_result,
> > > +        to_result,
> > > +        VTABLE_DEFAULT_ERROR, //
> > > +    },
> > > +    macros::vtable,
> > > +    prelude::*,
> > > +    str::CStrExt,
> > 
> > CStrExt is in the prelude. Please check for unnecessary imports.
> There is a
> `use super::*;` in rust/kernel/led/normal.rs and
> rust/kernel/led/multicolor.rs, which both make use of CStrExt.
> The latter being in patch 3.

I understand that you are using it. However, it's still unnecessary
because you also imported prelude::*.

> > > diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> > > new file mode 100644
> > > index 000000000000..2769f690bb24
> > > --- /dev/null
> > > +++ b/rust/kernel/led/normal.rs
> > > @@ -0,0 +1,230 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +//! Led mode for the `struct led_classdev`.
> > > +//!
> > > +//! C header: [`include/linux/leds.h`](srctree/include/linux/leds.h)
> > > +
> > > +use super::*;
> > > +
> > > +/// The led class device representation.
> > > +///
> > > +/// This structure represents the Rust abstraction for a led class device.
> > > +#[pin_data(PinnedDrop)]
> > > +pub struct Device<'bound, T: LedOps + 'bound> {
> > > +    #[pin]
> > > +    ops: T,
> > > +    #[pin]
> > > +    classdev: Opaque<bindings::led_classdev>,
> > > +    _p: PhantomData<&'bound ()>,
> > > +}
> > > +
> > > +impl<'a, S: DeviceBuilderState> DeviceBuilder<'a, S> {
> > > +    /// Registers a new [`Device`].
> > > +    pub fn build<'bound: 'a, T: LedOps + 'bound>(
> > > +        self,
> > > +        parent: &'bound T::Bus,
> > > +        ops: impl PinInit<T, Error> + 'a,
> > > +    ) -> impl PinInit<Device<'bound, T>, Error> + 'a {
> > 
> > I think it would be useful to separate out the two lifetimes more
> > clearly. You have two sets of lifetimes:
> > 
> > * 'bound which is the duration in which the bus device is bound.
> > * 'a which is the duration in which the `name`/`devicename` fields are
> >   valid.
> > 
> > And these have different constraints because 'bound is much larger than
> > 'a. The 'bound lifetime is longer than the entire Device struct, but the
> > 'a lifetime only needs to last for the duration of the initialization
> > because (I assume) the strings are copied by `led_classdev_register_ext`
> > 
> > So under that logic, I would rename 'a to 'name or something like that
> > to indicate what it's the lifetime of.
> Preferably 'init, which can be used in other class device abstractions
> as well if needed.

Sounds good.

> > 
> > Note that if I'm wrong about the lifetime of the name strings, then this
> > code should be changed accordingly. It looks like you're actually
> > stashing the pointers in the led_classdev, and if that outlives this
> > initializer, then the current lifetimes are wrong, and Device must also
> > be annotated with 'name to indicate this additional lifetime.
> name and devicename only need to be valid for the duration of
> `led_classdev_register_ext`.

Great.

Alice