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

[Gary Guo]

On Thu Jul 2, 2026 at 11:47 AM BST, 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.
>
>> 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.
>
> 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.

The lifetime here would probably be better named 'init.

It's the same case for async desugaring, where the lifetime is to capture
everything used in the initializer.

Best,
Gary