Re: [PATCH v18 3/3] rust: leds: add multicolor classdev abstractions

["Onur Özkan" <work@onurozkan.dev>]

On Sat, 30 May 2026 22:49:58 +0000
Markus Probst <markus.probst@posteo.de> wrote:

> Implement the abstractions needed for multicolor led class devices,
> including:
> 
> * `led::MultiColor` - the led mode implementation
> 
> * `MultiColorSubLed` - a safe wrapper arround `mc_subled`
> 
> * `led::MultiColorDevice` - a safe wrapper around `led_classdev_mc`
> 
> * `led::DeviceBuilder::build_multicolor` - a function to register a new
>   multicolor led class device
> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
>  rust/bindings/bindings_helper.h |   1 +
>  rust/kernel/led.rs              |  34 +++-
>  rust/kernel/led/multicolor.rs   | 405 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 439 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 446dbeaf0866..17b5461453e0 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -67,6 +67,7 @@
>  #include <linux/iosys-map.h>
>  #include <linux/jiffies.h>
>  #include <linux/jump_label.h>
> +#include <linux/led-class-multicolor.h>
>  #include <linux/mdio.h>
>  #include <linux/mm.h>
>  #include <linux/miscdevice.h>
> diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
> index 6ee337008db7..4540c4e5c9d7 100644
> --- a/rust/kernel/led.rs
> +++ b/rust/kernel/led.rs
> @@ -30,8 +30,16 @@
>      types::Opaque, //
>  };
>  
> +#[cfg(CONFIG_LEDS_CLASS_MULTICOLOR)]
> +mod multicolor;
>  mod normal;
>  
> +#[cfg(CONFIG_LEDS_CLASS_MULTICOLOR)]
> +pub use multicolor::{
> +    MultiColor,
> +    MultiColorDevice,
> +    MultiColorSubLed, //
> +};
>  pub use normal::{
>      Device,
>      Normal, //
> @@ -255,7 +263,24 @@ pub enum Color {
>      Violet = bindings::LED_COLOR_ID_VIOLET,
>      Yellow = bindings::LED_COLOR_ID_YELLOW,
>      Ir = bindings::LED_COLOR_ID_IR,
> +    #[cfg_attr(
> +        CONFIG_LEDS_CLASS_MULTICOLOR,
> +        doc = "Use this color for a [`MultiColor`] led."
> +    )]
> +    #[cfg_attr(
> +        not(CONFIG_LEDS_CLASS_MULTICOLOR),
> +        doc = "Use this color for a `MultiColor` led."
> +    )]
> +    /// If the led supports RGB, use [`Color::Rgb`] instead.
>      Multi = bindings::LED_COLOR_ID_MULTI,
> +    #[cfg_attr(
> +        CONFIG_LEDS_CLASS_MULTICOLOR,
> +        doc = "Use this color for a [`MultiColor`] led with rgb support."
> +    )]
> +    #[cfg_attr(
> +        not(CONFIG_LEDS_CLASS_MULTICOLOR),
> +        doc = "Use this color for a `MultiColor` led with rgb support."
> +    )]
>      Rgb = bindings::LED_COLOR_ID_RGB,
>      Purple = bindings::LED_COLOR_ID_PURPLE,
>      Orange = bindings::LED_COLOR_ID_ORANGE,
> @@ -296,7 +321,14 @@ fn try_from(value: u32) -> core::result::Result<Self, Self::Error> {
>  ///
>  /// Each led mode has its own led class device type with different capabilities.
>  ///
> -/// See [`Normal`].
> +#[cfg_attr(
> +    CONFIG_LEDS_CLASS_MULTICOLOR,
> +    doc = "See [`Normal`] and [`MultiColor`]."
> +)]
> +#[cfg_attr(
> +    not(CONFIG_LEDS_CLASS_MULTICOLOR),
> +    doc = "See [`Normal`] and `MultiColor`."
> +)]
>  pub trait Mode: private::Sealed {
>      /// The class device for the led mode.
>      type Device<'bound, T: LedOps<Mode = Self> + 'bound>;
> diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
> new file mode 100644
> index 000000000000..18c9d36221f1
> --- /dev/null
> +++ b/rust/kernel/led/multicolor.rs
> @@ -0,0 +1,405 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Led mode for the `struct led_classdev_mc`.
> +//!
> +//! C header: [`include/linux/led-class-multicolor.h`](srctree/include/linux/led-class-multicolor.h)
> +
> +use crate::{
> +    alloc::KVec,
> +    types::ScopeGuard, //
> +};
> +
> +use super::*;
> +
> +/// The led mode for the `struct led_classdev_mc`. Leds with this mode can have multiple colors.
> +pub enum MultiColor {}
> +impl Mode for MultiColor {
> +    type Device<'bound, T: LedOps<Mode = Self> + 'bound> = MultiColorDevice<'bound, T>;
> +}
> +impl private::Sealed for MultiColor {}
> +
> +/// The multicolor sub led info representation.
> +///
> +/// This structure represents the Rust abstraction for a C `struct mc_subled`.
> +#[repr(C)]
> +#[derive(Copy, Clone, Debug)]
> +#[non_exhaustive]
> +pub struct MultiColorSubLed {
> +    /// the color of the sub led
> +    pub color: Color,
> +    /// the brightness of the sub led.
> +    ///
> +    /// The value will be automatically calculated.
> +    /// See `MultiColor::pre_brightness_set`.
> +    pub brightness: u32,
> +    /// the intensity of the sub led.
> +    pub intensity: u32,
> +    /// arbitrary data for the driver to store.
> +    pub channel: u32,
> +}
> +
> +// We directly pass a reference to the `subled_info` field in `led_classdev_mc` to the driver via
> +// `Device::subleds()`.
> +// We need safeguards to ensure `MultiColorSubLed` and `mc_subled` stay identical.
> +const _: () = {
> +    use core::mem::offset_of;
> +
> +    const fn assert_same_type<T>(_: &T, _: &T) {}
> +
> +    let rust_zeroed = MultiColorSubLed {
> +        color: Color::White,
> +        brightness: 0,
> +        intensity: 0,
> +        channel: 0,
> +    };
> +    let c_zeroed = bindings::mc_subled {
> +        color_index: 0,
> +        brightness: 0,
> +        intensity: 0,
> +        channel: 0,
> +    };
> +
> +    assert!(offset_of!(MultiColorSubLed, color) == offset_of!(bindings::mc_subled, color_index));
> +    assert_same_type(&0u32, &c_zeroed.color_index);
> +
> +    assert!(
> +        offset_of!(MultiColorSubLed, brightness) == offset_of!(bindings::mc_subled, brightness)
> +    );
> +    assert_same_type(&rust_zeroed.brightness, &c_zeroed.brightness);
> +
> +    assert!(offset_of!(MultiColorSubLed, intensity) == offset_of!(bindings::mc_subled, intensity));
> +    assert_same_type(&rust_zeroed.intensity, &c_zeroed.intensity);
> +
> +    assert!(offset_of!(MultiColorSubLed, channel) == offset_of!(bindings::mc_subled, channel));
> +    assert_same_type(&rust_zeroed.channel, &c_zeroed.channel);
> +
> +    assert!(size_of::<MultiColorSubLed>() == size_of::<bindings::mc_subled>());
> +};
> +
> +impl MultiColorSubLed {
> +    /// Create a new multicolor sub led info.
> +    #[inline]
> +    pub const fn new(color: Color) -> Self {
> +        Self {
> +            color,
> +            brightness: 0,
> +            intensity: 0,
> +            channel: 0,
> +        }
> +    }
> +
> +    /// Set arbitrary data for the driver.
> +    #[inline]
> +    pub const fn channel(mut self, channel: u32) -> Self {
> +        self.channel = channel;
> +        self
> +    }
> +
> +    /// Set the initial intensity of the subled.
> +    #[inline]
> +    pub const fn initial_intensity(mut self, intensity: u32) -> Self {
> +        self.intensity = intensity;
> +        self
> +    }
> +}
> +
> +/// The multicolor led class device representation.
> +///
> +/// This structure represents the Rust abstraction for a multicolor led class device.
> +#[pin_data(PinnedDrop)]
> +pub struct MultiColorDevice<'bound, T: LedOps<Mode = MultiColor> + 'bound> {
> +    #[pin]
> +    ops: T,
> +    #[pin]
> +    classdev: Opaque<bindings::led_classdev_mc>,
> +    _p: PhantomData<&'bound ()>,
> +}
> +
> +impl<'a, S: DeviceBuilderState> DeviceBuilder<'a, S> {
> +    /// Registers a new [`MulticolorDevice`].
> +    pub fn build_multicolor<'bound: 'a, T: LedOps<Mode = MultiColor> + 'bound>(
> +        self,
> +        parent: &'bound T::Bus,
> +        ops: impl PinInit<T, Error> + 'a,
> +        subleds: &'a [MultiColorSubLed],
> +    ) -> impl PinInit<MultiColorDevice<'bound, T>, Error> + 'a {
> +        const_assert!(T::MAX_BRIGHTNESS <= i32::MAX.unsigned_abs() || !T::HAS_BRIGHTNESS_GET);
> +
> +        try_pin_init!(MultiColorDevice {
> +            ops <- ops,
> +            classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev_mc| {
> +                for (index_a, subled_a) in subleds.iter().enumerate() {
> +                    for (index_b, subled_b) in subleds.iter().enumerate() {
> +                        if index_a != index_b && subled_a.color == subled_b.color {
> +                            dev_err!(parent.as_ref(), "duplicate color in multicolor led\n");
> +                            return Err(EINVAL);

If we are failing on the first duplicate color then we can easily avoid O(n2)
here.

Instead of doing that, what I would do is:

	let mut list = [false; LED_COLOR_ID_MAX as usize];
	for subled in subleds {
		if list[subled.color as usize] {
			// duplication found, fail here.
		}

		list[subled.color as usize] = true;
	}

I know that we only have 15 colors today (I don't know if it that list ever
grows) but I think we should always avoid landing that kind of *potentially*
resource exhausting flows to kernel no matter how large/small the input is.
I think it's just not a good practice.

The latter should also be easier to read/follow at a glance.

Thanks,
Onur

> +                        }
> +                    }
> +                }
> +                let mut subleds_vec = KVec::new();
> +                subleds_vec.extend_from_slice(subleds, GFP_KERNEL)?;
> +                let (subled_info, num_colors, capacity) = subleds_vec.into_raw_parts();
> +                debug_assert_eq!(num_colors, capacity);
> +
> +                let subled_guard = ScopeGuard::new(|| {
> +                    // SAFETY: `subled_info` is guaranteed to be a valid array pointer to
> +                    // `mc_subled` with the length and capacity of `num_colors`.
> +                    drop(unsafe { KVec::from_raw_parts(subled_info, num_colors, num_colors) });
> +                });
> +
> +                // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
> +                // `led_classdev_mc` gets fully initialized in-place by
> +                // `led_classdev_multicolor_register_ext` including `mutex` and `list_head`.
> +                unsafe {
> +                    ptr.write(bindings::led_classdev_mc {
> +                        led_cdev: 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()
> +                        },
> +                        num_colors: u32::try_from(num_colors)?,
> +                        // CAST: The safeguards in the const block ensure that
> +                        // `MultiColorSubLed` has an identical layout to `mc_subled`.
> +                        subled_info: subled_info.cast::<bindings::mc_subled>(),
> +                    })
> +                };
> +
> +                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_mc`.
> +                to_result(unsafe {
> +                    bindings::led_classdev_multicolor_register_ext(
> +                        parent.as_ref().as_raw(),
> +                        ptr,
> +                        if self.name.is_none() {
> +                            &raw mut init_data
> +                        } else {
> +                            core::ptr::null_mut()
> +                        },
> +                    )
> +                })?;
> +
> +                subled_guard.dismiss();
> +
> +                core::mem::forget(self.fwnode); // keep the reference count incremented
> +
> +                Ok::<_, Error>(())
> +            }),
> +            _p: PhantomData,
> +        })
> +    }
> +}
> +
> +impl<'bound, T: LedOps<Mode = MultiColor> + 'bound> MultiColorDevice<'bound, T> {
> +    /// # Safety
> +    /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
> +    /// `led::MultiColorDevice`.
> +    unsafe fn from_raw<'a>(led_cdev: *mut bindings::led_classdev) -> &'a Self {
> +        // SAFETY: The function's contract guarantees that `led_cdev` points to a `led_classdev`
> +        // field embedded within a valid `led::MultiColorDevice`. `container_of!` can therefore
> +        // safely calculate the address of the containing struct.
> +        let led_mc_cdev = unsafe { container_of!(led_cdev, bindings::led_classdev_mc, led_cdev) };
> +
> +        // SAFETY: It is guaranteed that `led_mc_cdev` points to a `led_classdev_mc`
> +        // field embedded within a valid `led::MultiColorDevice`. `container_of!` can therefore
> +        // safely calculate the address of the containing struct.
> +        unsafe { &*container_of!(Opaque::cast_from(led_mc_cdev), Self, classdev) }
> +    }
> +
> +    #[inline]
> +    fn parent(&self) -> &'bound device::Device<Bound> {
> +        // SAFETY: `self.classdev.get()` is guaranteed to be a valid pointer to `led_classdev_mc`.
> +        unsafe { device::Device::from_raw((*(*self.classdev.get()).led_cdev.dev).parent) }
> +    }
> +
> +    /// Returns the subleds passed to [`Device::new_multicolor`].
> +    #[inline]
> +    pub fn subleds(&self) -> &[MultiColorSubLed] {
> +        // SAFETY: The existence of `self` guarantees that `self.classdev.get()` is a pointer to a
> +        // valid `led_classdev_mc`.
> +        let raw = unsafe { &*self.classdev.get() };
> +        // SAFETY: `raw.subled_info` is a valid pointer to `mc_subled[num_colors]`.
> +        // CAST: The safeguards in the const block ensure that `MultiColorSubLed` has an identical
> +        // layout to `mc_subled`.
> +        unsafe {
> +            core::slice::from_raw_parts(
> +                raw.subled_info.cast::<MultiColorSubLed>(),
> +                // CAST: It is guaranteed that `num_colors` fits into an `usize`.
> +                raw.num_colors as usize,
> +            )
> +        }
> +    }
> +}
> +
> +// SAFETY: A `led::MultiColorDevice` can be unregistered from any thread.
> +unsafe impl<'bound, T: LedOps<Mode = MultiColor> + 'bound + Send> Send
> +    for MultiColorDevice<'bound, T>
> +{
> +}
> +
> +// SAFETY: `led::MultiColorDevice` can be shared among threads because all methods of `led::Device`
> +// are thread safe.
> +unsafe impl<'bound, T: LedOps<Mode = MultiColor> + 'bound + Sync> Sync
> +    for MultiColorDevice<'bound, T>
> +{
> +}
> +
> +struct Adapter<T: LedOps<Mode = MultiColor>> {
> +    _p: PhantomData<T>,
> +}
> +
> +impl<T: LedOps<Mode = MultiColor>> Adapter<T> {
> +    /// # Safety
> +    /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
> +    /// `led::MultiColorDevice`.
> +    /// This function is called on setting the brightness of a led.
> +    unsafe extern "C" fn brightness_set_callback(
> +        led_cdev: *mut bindings::led_classdev,
> +        brightness: u32,
> +    ) {
> +        // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
> +        // `led_classdev` embedded within a `led::MultiColorDevice`.
> +        let classdev = unsafe { MultiColorDevice::<T>::from_raw(led_cdev) };
> +        // SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`.
> +        let parent = unsafe { T::Bus::from_device(classdev.parent()) };
> +
> +        // SAFETY: `classdev.classdev.get()` is guaranteed to be a pointer to a valid
> +        // `led_classdev_mc`.
> +        unsafe { bindings::led_mc_calc_color_components(classdev.classdev.get(), brightness) };
> +
> +        let _ = classdev.ops.brightness_set(parent, classdev, brightness);
> +    }
> +
> +    /// # Safety
> +    /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
> +    /// `led::MultiColorDevice`.
> +    /// This function is called on setting the brightness of a led immediately.
> +    unsafe extern "C" fn brightness_set_blocking_callback(
> +        led_cdev: *mut bindings::led_classdev,
> +        brightness: u32,
> +    ) -> i32 {
> +        from_result(|| {
> +            // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
> +            // `led_classdev` embedded within a `led::MultiColorDevice`.
> +            let classdev = unsafe { MultiColorDevice::<T>::from_raw(led_cdev) };
> +            // SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`.
> +            let parent = unsafe { T::Bus::from_device(classdev.parent()) };
> +
> +            // SAFETY: `classdev.classdev.get()` is guaranteed to be a pointer to a valid
> +            // `led_classdev_mc`.
> +            unsafe { bindings::led_mc_calc_color_components(classdev.classdev.get(), brightness) };
> +
> +            classdev.ops.brightness_set(parent, classdev, brightness)?;
> +            Ok(0)
> +        })
> +    }
> +
> +    /// # Safety
> +    /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
> +    /// `led::MultiColorDevice`.
> +    /// This function is called on getting the brightness of a led.
> +    unsafe extern "C" fn brightness_get_callback(led_cdev: *mut bindings::led_classdev) -> u32 {
> +        // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
> +        // `led_classdev` embedded within a `led::MultiColorDevice`.
> +        let classdev = unsafe { MultiColorDevice::<T>::from_raw(led_cdev) };
> +        // SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`.
> +        let parent = unsafe { T::Bus::from_device(classdev.parent()) };
> +
> +        // CAST: Resulting value will be casted back to i32 in the led subsystem.
> +        from_result(|| {
> +            classdev
> +                .ops
> +                .brightness_get(parent, classdev)
> +                .inspect(|val| debug_assert!(*val <= T::MAX_BRIGHTNESS))
> +                .and_then(|val| Ok(i32::try_from(val)?))
> +        }) as u32
> +    }
> +
> +    /// # Safety
> +    /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
> +    /// `led::MultiColorDevice`.
> +    /// `delay_on` and `delay_off` must be valid pointers to `usize` and have
> +    /// exclusive access for the period of this function.
> +    /// This function is called on enabling hardware accelerated blinking.
> +    unsafe extern "C" fn blink_set_callback(
> +        led_cdev: *mut bindings::led_classdev,
> +        delay_on: *mut usize,
> +        delay_off: *mut usize,
> +    ) -> i32 {
> +        from_result(|| {
> +            // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
> +            // `led_classdev` embedded within a `led::MultiColorDevice`.
> +            let classdev = unsafe { MultiColorDevice::<T>::from_raw(led_cdev) };
> +            // SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`.
> +            let parent = unsafe { T::Bus::from_device(classdev.parent()) };
> +
> +            classdev.ops.blink_set(
> +                parent,
> +                classdev,
> +                // SAFETY: The function's contract guarantees that `delay_on` points to a `usize`
> +                // and is exclusive for the period of this function.
> +                unsafe { &mut *delay_on },
> +                // SAFETY: The function's contract guarantees that `delay_off` points to a `usize`
> +                // and is exclusive for the period of this function.
> +                unsafe { &mut *delay_off },
> +            )?;
> +            Ok(0)
> +        })
> +    }
> +}
> +
> +#[pinned_drop]
> +impl<'bound, T: LedOps<Mode = MultiColor> + 'bound> PinnedDrop for MultiColorDevice<'bound, T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        let raw = self.classdev.get();
> +        // SAFETY: The existence of `self` guarantees that `self.classdev.get()` is a pointer to a
> +        // valid `led_classdev_mc`.
> +        let dev: &device::Device = unsafe { device::Device::from_raw((*raw).led_cdev.dev) };
> +
> +        let _fwnode = dev
> +            .fwnode()
> +            // SAFETY: the reference count of `fwnode` has previously been
> +            // incremented in `led::Device::new`.
> +            .map(|fwnode| unsafe { ARef::from_raw(NonNull::from(fwnode)) });
> +
> +        // SAFETY: The existence of `self` guarantees that `self.classdev` has previously been
> +        // successfully registered with `led_classdev_multicolor_register_ext`.
> +        unsafe { bindings::led_classdev_multicolor_unregister(raw) };
> +
> +        // SAFETY: `raw` is guaranteed to be a valid pointer to `led_classdev_mc`.
> +        let led_cdev = unsafe { &*raw };
> +
> +        // SAFETY: `subled_info` is guaranteed to be a valid array pointer to `mc_subled` with the
> +        // length and capacity of `led_cdev.num_colors`. See `led::MulticolorDevice::new`.
> +        drop(unsafe {
> +            KVec::from_raw_parts(
> +                led_cdev.subled_info,
> +                led_cdev.num_colors as usize,
> +                led_cdev.num_colors as usize,
> +            )
> +        });
> +    }
> +}
> 
> -- 
> 2.53.0
>