Re: [PATCH v26 3/4] rust: leds: Add multicolor classdev abstractions

[Markus Probst]

On Fri, 2026-10-09 at 11:52 -0700, Boqun Feng wrote:
> On Wed, Sep 30, 2026 at 01:05:32PM +0000, Markus Probst 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@xxxxxxxxx>
> > ---
> >  rust/bindings/bindings_helper.h |   1 +
> >  rust/kernel/led.rs              |  34 ++-
> >  rust/kernel/led/multicolor.rs   | 445 ++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 479 insertions(+), 1 deletion(-)
> > 
> > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> > index 4b31aa7f432f..81a03985322a 100644
> > --- a/rust/bindings/bindings_helper.h
> > +++ b/rust/bindings/bindings_helper.h
> > @@ -69,6 +69,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 c17f8ef75006..4b66fe41a80c 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, //
> > @@ -233,7 +241,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,
> > @@ -274,7 +299,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>: Deref<Target = T>;
> > diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
> > new file mode 100644
> > index 000000000000..309487bdf38a
> > --- /dev/null
> > +++ b/rust/kernel/led/multicolor.rs
> > @@ -0,0 +1,445 @@
> > +// 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 core::{
> > +    cell::UnsafeCell,
> > +    num::NonZero,
> > +    ptr, //
> > +};
> > +
> > +use crate::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(Debug)]
> > +#[non_exhaustive]
> > +pub struct MultiColorSubLed {
> > +    /// The color of the sub led
> > +    pub color: Color,
> > +    brightness: UnsafeCell<u32>,
> > +    intensity: UnsafeCell<u32>,
> 
> These should be `Atomic<u32>`, or am I missing something here? Using
> `Atomic<u32>` should resolve sashiko's comment on this patch.
Snippet of the `Atomic::from_ptr` rustdoc:

"
For the duration of 'a, other accesses to *ptr must not cause data
races (defined by LKMM) against atomic operations on the returned
reference. Note that if all other accesses are atomic, then this safety
requirement is trivially fulfilled.
"

This safety requirement is likely not met if I see this correctly,
because the led subsystem does not use atomic accesses.

Ofc, this function won't be used, but I think given that the same
struct is also accessed by the C-side, it should also apply here.

Like Sashiko suggests, "core::ptr::read_volatile()" might be a better
option to prevent certain compiler optimizations.

Thanks
- Markus Probst

> 
> Regards,
> Boqun
> 
> > +    /// The maximum supported intensity value.
> > +    ///
> > +    /// If None the maximum intensity equals to [`LedOps::MAX_BRIGHTNESS`].
> > +    pub max_intensity: Option<NonZero<u32>>,
> > +    /// Arbitrary data for the driver to store.
> > +    pub channel: u32,
> > +}
> > +
> [...]

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