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

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

[-- Attachment #1: Type: text/plain, Size: 8550 bytes --]

On Fri, 2026-10-09 at 12:34 -0700, Boqun Feng wrote:
> On Fri, Oct 09, 2026 at 07:18:16PM +0000, Markus Probst wrote:
> > 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@posteo.de>
> > > > ---
> > > >  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.
> > 
> 
> Then the C side has a data race that needs some fix (or they use
> READ_ONCE() or WRITE_ONCE() which are *atomic* to avoid the data race).
They don't use READ_ONCE or WRITE_ONCE.

Writes to "intensity" can happen at anytime by `multi_intensity_store`.
It does lock the `led_access` mutex on write. It is not locked on read
and `grep "READ_ONCE" -r drivers/leds/` has no matches in drivers
either.

Writes to "brightness" are on the C-side handled by the driver by
calling `led_mc_calc_color_components`. This rust abstraction always
calles it in `brightness_set_callback`. So on the C-side, this at least
is less of an issue, as writes and reads are controlled by the C
driver.

> 
> The general rule is: if C side has a data race, they should fix it, if C
> side doesn't care ("the compiler should not data race on this code"),
> then the Rust side treat it as atomic operations. This is the only way
> to better code regarding data races.
It probably should use WRITE_ONCE and READ_ONCE, but it also shouldn't
create any issues if its not used. There is no load tearing on a 32-bit
integer and memory ordering is not required. Not sure if its worth the
trouble changing every existing multicolor led driver.

> 
> > 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.
> > 
> 
> No, please don't over-use read_volatile(). The reason that READ_ONCE()
> and WRITE_ONCE() are safe to use for synchronization is because
> semantics-wise they are atomic on certain types (if aligned), and the
> them being volatile is just an implementation detail.
Ok.

I will need to make .get_mut() const for this.

> 
> Regards,
> Boqun

Thanks
- Markus Probst

> 
> > 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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]