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

[Markus Probst]

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 <markus.probst@xxxxxxxxx>
> > ---
> >  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 <markus.probst@xxxxxxxxx>
> > +L:	linux-leds@xxxxxxxxxxxxxxx
> > +L:	rust-for-linux@xxxxxxxxxxxxxxx
> > +S:	Maintained
> > +F:	rust/kernel/led.rs
> > +F:	rust/kernel/led/
> > +
> >  LEGO MINDSTORMS EV3
> >  R:	David Lechner <david@xxxxxxxxxxxxxx>
> >  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.
> 
> > 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.
> 
> 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`.

> 
> > +        const_assert!(T::MAX_BRIGHTNESS <= i32::MAX.unsigned_abs() || !T::HAS_BRIGHTNESS_GET);
> > +
> > +        try_pin_init!(Device {
> > +            ops <- ops,
> > +            classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev| {
> > +                // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
> > +                // `led_classdev` gets fully initialized in-place by
> > +                // `led_classdev_register_ext` including `mutex` and `list_head`.
> > +                unsafe {
> > +                    ptr.write(bindings::led_classdev {
> > +                        brightness_set: (!T::BLOCKING)
> > +                            .then_some(Adapter::<T>::brightness_set_callback),
> > +                        brightness_set_blocking: T::BLOCKING
> > +                            .then_some(Adapter::<T>::brightness_set_blocking_callback),
> > +                        brightness_get: T::HAS_BRIGHTNESS_GET
> > +                            .then_some(Adapter::<T>::brightness_get_callback),
> > +                        blink_set: T::HAS_BLINK_SET.then_some(Adapter::<T>::blink_set_callback),
> > +                        max_brightness: T::MAX_BRIGHTNESS,
> > +                        brightness: self.initial_brightness,
> > +                        color: self.color as u32,
> > +                        name: self.name.map_or(core::ptr::null(), CStrExt::as_char_ptr),
> > +                        ..bindings::led_classdev::default()
> > +                    })
> > +                };
> > +
> > +                let mut init_data = bindings::led_init_data {
> > +                    fwnode: self
> > +                        .fwnode
> > +                        .as_ref()
> > +                        .map_or(core::ptr::null_mut(), |fwnode| fwnode.as_raw()),
> > +                    default_label: core::ptr::null(),
> > +                    devicename: self
> > +                        .devicename
> > +                        .map_or(core::ptr::null(), CStrExt::as_char_ptr),
> > +                    devname_mandatory: self.devname_mandatory,
> > +                };
> > +
> > +                // SAFETY:
> > +                // - `parent.as_ref().as_raw()` is guaranteed to be a pointer to a valid
> > +                //    `device`.
> > +                // - `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,
> > +                        if self.name.is_none() {
> > +                            &raw mut init_data
> > +                        } else {
> > +                            core::ptr::null_mut()
> > +                        },
> > +                    )
> > +                })?;
> > +
> > +                core::mem::forget(self.fwnode); // keep the reference count incremented
> > +
> > +                Ok::<_, Error>(())
> > +            }),
> > +            _p: PhantomData,
> > +        })
> > +    }
> > +}
Thanks
- Markus Probst


> 
> Alice

Attachment: signature.asc
Description: This is a digitally signed message part