Re: [PATCH v2 2/2] rust: leds: add basic led classdev abstractions

[Markus Probst <markus.probst@posteo.de>]

On Fri, 2025-10-10 at 11:41 +0000, Alice Ryhl wrote:
> On Thu, Oct 09, 2025 at 06:12:34PM +0000, Markus Probst wrote:
> > Implement the core abstractions needed for led class devices,
> > including:
> > 
> > * `led::Handler` - the trait for handling leds, including
> >   `brightness_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@posteo.de>
> 
> > +/// The led class device representation.
> > +///
> > +/// This structure represents the Rust abstraction for a C `struct
> > led_classdev`.
> > +#[pin_data(PinnedDrop)]
> > +pub struct Device {
> > +    handler: KBox<dyn HandlerImpl>,
> > +    #[pin]
> > +    classdev: Opaque<bindings::led_classdev>,
> > +}
> 
> Is it not possible to use Device<T> with a `handler: T` field here?
> That
> avoids the box. I don't think other abstractions have needed that. If
> it's needed, then why is LED special?
The handler variable is defined, because leds usually store additional
data. If an led driver has multiple leds, it needs to know which one,
without defining for each led its own struct. It also needs access to
resources which allows it to control the leds, e.g. I2cClient (hasn't
been merged yet), Io and so on.

It is possible to store handler: T, but I would not know a way to call
it dynamically, because T is unknown at brightness_set,
brightness_set_blocking and blink_set methods.

I could maybe work it around by creating unsafe extern "C" methods with
method body pre-defined in the Handler trait, which is then used to
reference brightness_set etc. directly though.

> 
> > +/// The led handler trait.
> > +///
> > +/// # Examples
> > +///
> > +///```
> > +/// # use kernel::{c_str, led, alloc::KBox, error::{Result,
> > code::ENOSYS}};
> > +/// # use core::pin::Pin;
> > +///
> > +/// struct MyHandler;
> > +///
> > +///
> > +/// impl led::Handler for MyHandler {
> > +///     const BLOCKING = false;
> > +///     const MAX_BRIGHTNESS = 255;
> > +///
> > +///     fn brightness_set(&self, _brightness: u32) -> Result<()> {
> > +///         // Set the brightness for the led here
> > +///         Ok(())
> > +///     }
> > +/// }
> > +///
> > +/// fn register_my_led() -> Result<Pin<KBox<led::Device>>> {
> > +///     let handler = MyHandler;
> > +///     KBox::pin_init(led::Device::new(
> > +///         None,
> > +///         None,
> > +///         led::InitData::new()
> > +///             .default_label(c_str!("my_led")),
> > +///         handler,
> > +///     ))
> > +/// }
> > +///```
> > +/// Led drivers must implement this trait in order to register and
> > handle a [`Device`].
> > +pub trait Handler {
> > +    /// If set true, [`Handler::brightness_set`] and
> > [`Handler::blink_set`] must not sleep
> > +    /// and perform the operation immediately.
> > +    const BLOCKING: bool;
> > +    /// Set this to true, if [`Handler::blink_set`] is
> > implemented.
> > +    const BLINK: bool = false;
> 
> We have a macro called #[vtable] that automatically generates this
> kind
> of constant for you. Please use it.
> 
> > +impl Device {
> > +    /// Registers a new led classdev.
> > +    ///
> > +    /// The [`Device`] will be unregistered and drop.
> > +    pub fn new<'a, T: Handler + 'static>(
> > +        parent: Option<&'a device::Device>,
> > +        init_data: InitData<'a>,
> > +        handler: T,
> > +    ) -> impl PinInit<Self, Error> + use<'a, T> {
> > +        try_pin_init!(Self {
> > +            handler <- {
> > +                let handler: KBox<dyn HandlerImpl> =
> > KBox::<T>::new(handler, GFP_KERNEL)?;
> > +                Ok::<_, Error>(handler)
> > +            },
> > +            classdev <- Opaque::try_ffi_init(|ptr: *mut
> > bindings::led_classdev| {
> > +                // SAFETY: `try_ffi_init` guarantees that `ptr` is
> > valid for write.
> > +                unsafe { ptr.write(bindings::led_classdev {
> > +                    max_brightness: T::MAX_BRIGHTNESS,
> > +                    brightness_set: T::BLOCKING.then_some(
> > +                        brightness_set as unsafe extern "C"
> > fn(*mut bindings::led_classdev, u32)
> > +                    ),
> > +                    brightness_set_blocking:
> > (!T::BLOCKING).then_some(
> > +                        brightness_set_blocking
> > +                            as unsafe extern "C" fn(*mut
> > bindings::led_classdev,u32) -> i32
> > +                    ),
> > +                    blink_set: T::BLINK.then_some(
> > +                        blink_set
> > +                            as unsafe extern "C" fn(*mut
> > bindings::led_classdev, *mut usize,
> > +                                                    *mut usize) ->
> > i32
> 
> I think you can just do `blink_set as _`.
> 
> > +                    ),
> > +                    .. bindings::led_classdev::default()
> > +                }) };
> > +
> > +                let mut init_data = bindings::led_init_data {
> > +                    fwnode:
> > init_data.fwnode.map_or(core::ptr::null_mut(), FwNode::as_raw),
> > +                    default_label: init_data.default_label
> > +                                           
> > .map_or(core::ptr::null(), CStr::as_char_ptr),
> > +                    devicename:
> > init_data.devicename.map_or(core::ptr::null(), CStr::as_char_ptr),
> > +                    devname_mandatory:
> > init_data.devname_mandatory,
> > +                };
> > +
> > +                let parent = parent
> > +                    .map_or(core::ptr::null_mut(),
> > device::Device::as_raw);
> > +
> > +                // SAFETY:
> > +                // - `parent` 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,
> > ptr, &mut init_data)
> 
> So it's ok that `init_data` is valid for the duration of this call
> and
> no longer? It doesn't stash a pointer to it anywhere?
init_data->parent has a reference count (it exists as long as the
led_classdev)
init_data->devicename will only be used on register (to build the
led_classdev name alongside other parameters)
init_data->fwnode will be accessed on register and stored in
led_classdev. I looked at the function again, to my surpris it does not
increment the reference count of fwnode, so this could be an issue.

> 
> > +extern "C" fn brightness_set(led_cdev: *mut
> > bindings::led_classdev, brightness: u32) {
> > +    // SAFETY: `led_cdev` is a valid pointer to a `led_classdev`
> > stored inside a `Device`.
> > +    let classdev = unsafe {
> > +        &*container_of!(
> > +            led_cdev.cast::<Opaque<bindings::led_classdev>>(),
> 
> Please use Opaque::cast_from instead.
> 
> > +    // SAFETY: `led_cdev` is a valid pointer to a `led_classdev`
> > stored inside a `Device`.
> > +    let classdev = unsafe {
> > +        &*container_of!(
> > +            led_cdev.cast::<Opaque<bindings::led_classdev>>(),
> > +            Device,
> > +            classdev
> > +        )
> > +    };
> 
> Instead of repeating this logic many times, I suggest a
> Device::from_raw
> method.
> 
> > +extern "C" fn blink_set(
> > +    led_cdev: *mut bindings::led_classdev,
> > +    delay_on: *mut usize,
> > +    delay_off: *mut usize,
> > +) -> i32 {
> > +    // SAFETY: `led_cdev` is a valid pointer to a `led_classdev`
> > stored inside a `Device`.
> > +    let classdev = unsafe {
> > +        &*container_of!(
> > +            led_cdev.cast::<Opaque<bindings::led_classdev>>(),
> > +            Device,
> > +            classdev
> > +        )
> > +    };
> > +    from_result(|| {
> > +        classdev.handler.blink_set(
> > +            // SAFETY: `delay_on` is guaranteed to be a valid
> > pointer to usize
> > +            unsafe { &mut *delay_on },
> > +            // SAFETY: `delay_on` is guaranteed to be a valid
> > pointer to usize
> > +            unsafe { &mut *delay_off },
> 
> To create a mutable reference, this safety comment should argue why
> it
> is the case that nobody will access these fields for the duration of
> `blink_set`. (For example, from another thread?)
> 
> Alice

Thanks
- Markus Probst