[PATCH v24 0/4] rust: leds: add led classdev abstractions

[PATCH v24 0/4] rust: leds: add led classdev abstractions

[Markus Probst]

The following changes were made:
* add basic led classdev abstractions to register and unregister leds

* add basic led classdev abstractions to register and unregister
  multicolor leds

Changes since v23:
* add separate patch for MAINTAINERS file update

Changes since v22:
* readded CStrExt import, because it is imported with `as _` in prelude.
  A `# CONFIG_RUST is not set` sneaked into my .config while
  development, so the compile error was unnoticed.

Changes since v21:
* use 'init for lifetime that is only alive during initialization
* remove unnecessary CStrExt import

Changes since v20:
* resolve Sashiko regressions:
  * fix typo
  * fix fwnode refcount decremented too early

Changes since v19:
* rebase on v7.2-rc1:
  * Add `max_intensity` to `MultiColorSubLed`
* use safer `KBox::pin_slice` instead of `KVec`
  (len might not equal capacity)
* explicitly call `FwNode::dec_ref` instead of dropping a reconstructed
  `ARef<FwNode>`.
* remove direct access to `intensity` and `brightness` fields,
  which may get mutated concurrently by the C side
* fix safety comments pointing to functions from previous revisions

Changes since v18:
* add inlines
* fix invalid documentation
* improve led color duplicate checking

Changes since v17:
* use lifetimes instead of Devres

Changes since v16:
* use for loops for duplicate checking

Changes since v15:
* fix issues reported by Sashiko bot:
  * fix returning error not possible on `brightness_get` callback

Changes since v14:
* fix issues reported by Sashiko bot:
  * add missing inlines
  * add missing Sync trait bound
  * fix vertical import layout for public export of private types
  * fix potential memory leak, if a multicolor led device with over
    `u32::MAX` subleds is registered
* remove default_trigger option
* fix missing CAST doc

Changes since v13:
* rebased onto v7.1-rc1

Changes since v12:
* add `led::DeviceBuilder::name()` and `DeviceBuilderState'
* add `led::Color::as_c_str`

Changes since v11:
* use `led::DeviceBuilder` instead of `led::InitData`
* use static_assert instead of const { assert!(...) }
* restructured patches to avoid moving `led::Device` from
  rust/kernel/led.rs to rust/kernel/led/normal.rs in the 2. patch

Changes since v10:
* allow in-place initialization of `LedOps`
* run rustfmt for code inside `try_pin_init!`

Changes since v9:
* add missing periods in documentation
* duplicate `led::Device` and `led::Adapter` instead of using a complex
  trait
* fix imports not using prelude
* adapt to CStr change
* documented `led::Color::Multi` and `led::Color::Rgb`

Changes since v8:
* accept `Option<ARef<Fwnode>>` in `led::InitData::fwnode()`
* make functions in `MultiColorSubLed` const
* drop the "rust: Add trait to convert a device reference to a bus
  device reference" patch, as it has been picked into driver-core

Changes since v7:
* adjusted import style
* added classdev parameter to callback functions in `LedOps`
* implement `led::Color`
* extend `led::InitData` with
  - initial_brightness
  - default_trigger
  - default_color
* split generic and normal led classdev abstractions up (see patch 3/4)
* add multicolor led class device abstractions (see patch 4/4)
* added MAINTAINERS entry

Changes since v6:
* fixed typos
* improved documentation

Changes since v5:
* rename `IntoBusDevice` trait into `AsBusDevice`
* fix documentation about `LedOps::BLOCKING`
* removed dependency on i2c bindings
* added `AsBusDevice` implementation for `platform::Device`
* removed `device::Device` fallback implementation
* document that `AsBusDevice` must not be used by drivers and is
  intended for bus and class device abstractions only.

Changes since v4:
* add abstraction to convert a device reference to a bus device
  reference
* require the bus device as parent device and provide it in class device
  callbacks
* remove Pin<Vec<_>> abstraction (as not relevant for the led
  abstractions)
* fixed formatting in `led::Device::new`
* fixed `LedOps::BLOCKING` did the inverse effect

Changes since v3:
* fixed kunit tests failing because of example in documentation

Changes since v2:
* return `Devres` on `led::Device` creation
* replace KBox<T> with T in struct definition
* increment and decrement reference-count of fwnode
* make a device parent mandatory for led classdev creation
* rename `led::Handler` to `led::LedOps`
* add optional `brightness_get` function to `led::LedOps`
* use `#[vtable]` instead of `const BLINK: bool`
* use `Opaque::cast_from` instead of casting a pointer
* improve documentation
* improve support for older rust versions
* use `&Device<Bound>` for parent

Changes since v1:
* fixed typos noticed by Onur Özkan

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
Markus Probst (4):
      rust: leds: add basic led classdev abstractions
      rust: leds: add Mode trait
      rust: leds: add multicolor classdev abstractions
      MAINTAINERS: rust: leds: Add rust abstraction entry

 MAINTAINERS                     |   8 +
 rust/bindings/bindings_helper.h |   1 +
 rust/kernel/led.rs              | 339 +++++++++++++++++++++++++++++
 rust/kernel/led/multicolor.rs   | 460 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/led/normal.rs       | 238 +++++++++++++++++++++
 rust/kernel/lib.rs              |   1 +
 6 files changed, 1047 insertions(+)
---
base-commit: e5e04726cdd043e309677071ab1b65a4b18f422b
change-id: 20251114-rust_leds-a959f7c2f7f9

[PATCH v24 1/4] rust: leds: add basic led classdev abstractions

[Markus Probst]

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@posteo.de>
---
 rust/kernel/led.rs        | 288 ++++++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/led/normal.rs | 230 ++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs        |   1 +
 3 files changed, 519 insertions(+)

diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
new file mode 100644
index 000000000000..596975e103b8
--- /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,
+    sync::aref::ARef,
+    types::Opaque, //
+};
+
+mod normal;
+
+pub use normal::Device;
+
+/// The name of the led is determined by the driver.
+pub enum Named {}
+/// The name of the led is determined by its fwnode.
+pub enum Unnamed {}
+
+/// How the name of the led should be determined.
+pub trait DeviceBuilderState: private::Sealed {}
+
+impl DeviceBuilderState for Named {}
+impl private::Sealed for Named {}
+impl DeviceBuilderState for Unnamed {}
+impl private::Sealed for Unnamed {}
+
+/// The builder to register a led class device.
+///
+/// See [`LedOps`].
+pub struct DeviceBuilder<'init, S> {
+    fwnode: Option<ARef<FwNode>>,
+    name: Option<&'init CStr>,
+    devicename: Option<&'init CStr>,
+    devname_mandatory: bool,
+    initial_brightness: u32,
+    color: Color,
+    _p: PhantomData<S>,
+}
+
+impl<S: DeviceBuilderState> DeviceBuilder<'static, S> {
+    /// Creates a new [`DeviceBuilder`].
+    #[inline]
+    #[expect(
+        clippy::new_without_default,
+        reason = "no need and derive is prevented by S"
+    )]
+    pub fn new() -> Self {
+        Self {
+            fwnode: None,
+            name: None,
+            devicename: None,
+            devname_mandatory: false,
+            initial_brightness: 0,
+            color: Color::default(),
+            _p: PhantomData,
+        }
+    }
+}
+
+impl<'init> DeviceBuilder<'init, Unnamed> {
+    /// Sets the firmware node.
+    #[inline]
+    pub fn fwnode(self, fwnode: Option<ARef<FwNode>>) -> Self {
+        Self { fwnode, ..self }
+    }
+
+    /// Sets the device name.
+    #[inline]
+    pub fn devicename(self, devicename: &'init CStr) -> Self {
+        Self {
+            devicename: Some(devicename),
+            ..self
+        }
+    }
+
+    /// Sets if a device name is mandatory.
+    #[inline]
+    pub fn devicename_mandatory(self, mandatory: bool) -> Self {
+        Self {
+            devname_mandatory: mandatory,
+            ..self
+        }
+    }
+}
+
+impl<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> {
+    /// Sets the initial brightness value for the led.
+    ///
+    /// The default brightness is 0.
+    /// If [`LedOps::brightness_get`] is implemented, this value will be ignored.
+    #[inline]
+    pub fn initial_brightness(self, brightness: u32) -> Self {
+        Self {
+            initial_brightness: brightness,
+            ..self
+        }
+    }
+
+    /// Sets the color of the led.
+    ///
+    /// This value can be overwritten by the "color" fwnode property.
+    #[inline]
+    pub fn color(self, color: Color) -> Self {
+        Self { color, ..self }
+    }
+}
+
+impl<'init> DeviceBuilder<'init, Named> {
+    /// Sets the name of the led.
+    ///
+    /// Setting this will prevent the fwnode from being used and prevents automatic name
+    /// composition.
+    #[inline]
+    pub fn name(self, name: &'init CStr) -> Self {
+        Self {
+            name: Some(name),
+            ..self
+        }
+    }
+}
+
+/// Trait defining the operations for a LED driver.
+///
+/// # Examples
+/// ```
+/// use kernel::{
+///      device,
+///      devres::Devres,
+///      led,
+///      macros::vtable,
+///      platform,
+///      prelude::*, //
+///  };
+///
+/// struct MyLedOps;
+///
+///
+/// #[vtable]
+/// impl led::LedOps for MyLedOps {
+///     type Bus = platform::Device<device::Bound>;
+///     const BLOCKING: bool = false;
+///     const MAX_BRIGHTNESS: u32 = 255;
+///
+///     fn brightness_set<'bound>(
+///         &self,
+///         _dev: &'bound platform::Device<device::Bound>,
+///         _classdev: &led::Device<'bound, Self>,
+///         _brightness: u32
+///     ) -> Result<()> {
+///         // Set the brightness for the led here
+///         Ok(())
+///     }
+/// }
+/// ```
+/// Led drivers must implement this trait in order to register and handle a [`Device`].
+#[vtable]
+pub trait LedOps: Send + Sync + Sized {
+    /// The bus device required by the implementation.
+    #[allow(private_bounds)]
+    type Bus: AsBusDevice<Bound>;
+
+    /// If set true, [`LedOps::brightness_set`] and [`LedOps::blink_set`] must perform the
+    /// operation immediately. If set false, they must not sleep.
+    const BLOCKING: bool;
+    /// The max brightness level.
+    const MAX_BRIGHTNESS: u32;
+
+    /// Sets the brightness level.
+    ///
+    /// See also [`LedOps::BLOCKING`].
+    fn brightness_set<'bound>(
+        &self,
+        dev: &'bound Self::Bus,
+        classdev: &Device<'bound, Self>,
+        brightness: u32,
+    ) -> Result<()>;
+
+    /// Gets the current brightness level.
+    fn brightness_get<'bound>(
+        &self,
+        dev: &'bound Self::Bus,
+        classdev: &Device<'bound, Self>,
+    ) -> Result<u32> {
+        let _ = (dev, classdev);
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+
+    /// Activates hardware accelerated blinking.
+    ///
+    /// delays are in milliseconds. If both are zero, a sensible default should be chosen.
+    /// The caller should adjust the timings in that case and if it can't match the values
+    /// specified exactly. Setting the brightness to 0 will disable the hardware accelerated
+    /// blinking.
+    ///
+    /// See also [`LedOps::BLOCKING`].
+    fn blink_set<'bound>(
+        &self,
+        dev: &'bound Self::Bus,
+        classdev: &Device<'bound, Self>,
+        delay_on: &mut usize,
+        delay_off: &mut usize,
+    ) -> Result<()> {
+        let _ = (dev, classdev, delay_on, delay_off);
+        build_error!(VTABLE_DEFAULT_ERROR)
+    }
+}
+
+/// Led colors.
+#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
+#[repr(u32)]
+#[non_exhaustive]
+#[expect(
+    missing_docs,
+    reason = "it shouldn't be necessary to document each color"
+)]
+pub enum Color {
+    #[default]
+    White = bindings::LED_COLOR_ID_WHITE,
+    Red = bindings::LED_COLOR_ID_RED,
+    Green = bindings::LED_COLOR_ID_GREEN,
+    Blue = bindings::LED_COLOR_ID_BLUE,
+    Amber = bindings::LED_COLOR_ID_AMBER,
+    Violet = bindings::LED_COLOR_ID_VIOLET,
+    Yellow = bindings::LED_COLOR_ID_YELLOW,
+    Ir = bindings::LED_COLOR_ID_IR,
+    Multi = bindings::LED_COLOR_ID_MULTI,
+    Rgb = bindings::LED_COLOR_ID_RGB,
+    Purple = bindings::LED_COLOR_ID_PURPLE,
+    Orange = bindings::LED_COLOR_ID_ORANGE,
+    Pink = bindings::LED_COLOR_ID_PINK,
+    Cyan = bindings::LED_COLOR_ID_CYAN,
+    Lime = bindings::LED_COLOR_ID_LIME,
+}
+static_assert!(bindings::LED_COLOR_ID_MAX == 15);
+
+impl Color {
+    /// Name of the color
+    #[inline]
+    pub fn as_c_str(self) -> &'static CStr {
+        // SAFETY:
+        // - `self as u8` is a valid led color id.
+        // - `led_get_color_name` always returns a valid C string pointer.
+        unsafe { CStr::from_char_ptr(bindings::led_get_color_name(self as u8)) }
+    }
+}
+
+impl TryFrom<u32> for Color {
+    type Error = Error;
+
+    fn try_from(value: u32) -> core::result::Result<Self, Self::Error> {
+        if value < bindings::LED_COLOR_ID_MAX {
+            // SAFETY:
+            // - `Color` is represented as `u32`
+            // - the static_assert above guarantees that no additional color has been added
+            // - `value` is guaranteed to be in the color id range
+            Ok(unsafe { transmute::<u32, Color>(value) })
+        } else {
+            Err(EINVAL)
+        }
+    }
+}
+
+mod private {
+    pub trait Sealed {}
+}
diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
new file mode 100644
index 000000000000..816db7c57689
--- /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<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> {
+    /// Registers a new [`Device`].
+    pub fn build<'bound: 'init, T: LedOps + 'bound>(
+        self,
+        parent: &'bound T::Bus,
+        ops: impl PinInit<T, Error> + 'init,
+    ) -> impl PinInit<Device<'bound, T>, Error> + 'init {
+        const_assert!(T::MAX_BRIGHTNESS <= i32::MAX.unsigned_abs() || !T::HAS_BRIGHTNESS_GET);
+
+        try_pin_init!(Device {
+            ops <- ops,
+            classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev| {
+                // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
+                // `led_classdev` gets fully initialized in-place by
+                // `led_classdev_register_ext` including `mutex` and `list_head`.
+                unsafe {
+                    ptr.write(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()
+                    })
+                };
+
+                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`.
+                to_result(unsafe {
+                    bindings::led_classdev_register_ext(
+                        parent.as_ref().as_raw(),
+                        ptr,
+                        if self.name.is_none() {
+                            &raw mut init_data
+                        } else {
+                            core::ptr::null_mut()
+                        },
+                    )
+                })?;
+
+                core::mem::forget(self.fwnode); // keep the reference count incremented
+
+                Ok::<_, Error>(())
+            }),
+            _p: PhantomData,
+        })
+    }
+}
+
+impl<'bound, T: LedOps + 'bound> Device<'bound, T> {
+    /// # Safety
+    /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
+    /// `led::Device`.
+    #[inline]
+    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::Device`. `container_of!` can therefore
+        // safely calculate the address of the containing struct.
+        unsafe { &*container_of!(Opaque::cast_from(led_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`.
+        unsafe { device::Device::from_raw((*(*self.classdev.get()).dev).parent) }
+    }
+}
+
+// SAFETY: A `led::Device` can be unregistered from any thread.
+unsafe impl<'bound, T: LedOps + 'bound + Send> Send for Device<'bound, T> {}
+
+// SAFETY: `led::Device` can be shared among threads because all methods of `led::Device`
+// are thread safe.
+unsafe impl<'bound, T: LedOps + 'bound + Sync> Sync for Device<'bound, T> {}
+
+struct Adapter<T: LedOps> {
+    _p: PhantomData<T>,
+}
+
+impl<T: LedOps> Adapter<T> {
+    /// # Safety
+    /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
+    /// `led::Device`.
+    /// 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::Device`.
+        let classdev = unsafe { Device::<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()) };
+
+        let _ = classdev.ops.brightness_set(parent, classdev, brightness);
+    }
+
+    /// # Safety
+    /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
+    /// `led::Device`.
+    /// 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::Device`.
+            let classdev = unsafe { Device::<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.brightness_set(parent, classdev, brightness)?;
+            Ok(0)
+        })
+    }
+
+    /// # Safety
+    /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
+    /// `led::Device`.
+    /// 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::Device`.
+        let classdev = unsafe { Device::<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::Device`.
+    /// `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::Device`.
+            let classdev = unsafe { Device::<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 + 'bound> PinnedDrop for Device<'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`.
+        let dev: &device::Device = unsafe { device::Device::from_raw((*raw).dev) };
+
+        let _fwnode = dev
+            .fwnode()
+            // SAFETY: the reference count of `fwnode` has previously been
+            // incremented in `led::DeviceBuilder::build`.
+            .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_register_ext`.
+        unsafe { bindings::led_classdev_unregister(raw) };
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 4d5c96ddc49c..748788ad131b 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -96,6 +96,7 @@
 pub mod jump_label;
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
+pub mod led;
 pub mod list;
 pub mod maple_tree;
 pub mod miscdevice;

-- 
2.54.0

[PATCH v24 2/4] rust: leds: add Mode trait

[Markus Probst]

Add the `led::Mode` trait to allow for other types of led class devices
in `led::LedOps`.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 rust/kernel/led.rs        | 27 +++++++++++++++++++++++----
 rust/kernel/led/normal.rs | 22 +++++++++++++++-------
 2 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
index 596975e103b8..5d7e6d08ad2a 100644
--- a/rust/kernel/led.rs
+++ b/rust/kernel/led.rs
@@ -32,7 +32,10 @@
 
 mod normal;
 
-pub use normal::Device;
+pub use normal::{
+    Device,
+    Normal, //
+};
 
 /// The name of the led is determined by the driver.
 pub enum Named {}
@@ -161,6 +164,7 @@ pub fn name(self, name: &'init CStr) -> Self {
 /// #[vtable]
 /// impl led::LedOps for MyLedOps {
 ///     type Bus = platform::Device<device::Bound>;
+///     type Mode = led::Normal;
 ///     const BLOCKING: bool = false;
 ///     const MAX_BRIGHTNESS: u32 = 255;
 ///
@@ -182,6 +186,11 @@ pub trait LedOps: Send + Sync + Sized {
     #[allow(private_bounds)]
     type Bus: AsBusDevice<Bound>;
 
+    /// The led mode to use.
+    ///
+    /// See [`Mode`].
+    type Mode: Mode;
+
     /// If set true, [`LedOps::brightness_set`] and [`LedOps::blink_set`] must perform the
     /// operation immediately. If set false, they must not sleep.
     const BLOCKING: bool;
@@ -194,7 +203,7 @@ pub trait LedOps: Send + Sync + Sized {
     fn brightness_set<'bound>(
         &self,
         dev: &'bound Self::Bus,
-        classdev: &Device<'bound, Self>,
+        classdev: &<Self::Mode as Mode>::Device<'bound, Self>,
         brightness: u32,
     ) -> Result<()>;
 
@@ -202,7 +211,7 @@ fn brightness_set<'bound>(
     fn brightness_get<'bound>(
         &self,
         dev: &'bound Self::Bus,
-        classdev: &Device<'bound, Self>,
+        classdev: &<Self::Mode as Mode>::Device<'bound, Self>,
     ) -> Result<u32> {
         let _ = (dev, classdev);
         build_error!(VTABLE_DEFAULT_ERROR)
@@ -219,7 +228,7 @@ fn brightness_get<'bound>(
     fn blink_set<'bound>(
         &self,
         dev: &'bound Self::Bus,
-        classdev: &Device<'bound, Self>,
+        classdev: &<Self::Mode as Mode>::Device<'bound, Self>,
         delay_on: &mut usize,
         delay_off: &mut usize,
     ) -> Result<()> {
@@ -283,6 +292,16 @@ fn try_from(value: u32) -> core::result::Result<Self, Self::Error> {
     }
 }
 
+/// The led mode.
+///
+/// Each led mode has its own led class device type with different capabilities.
+///
+/// See [`Normal`].
+pub trait Mode: private::Sealed {
+    /// The class device for the led mode.
+    type Device<'bound, T: LedOps<Mode = Self> + 'bound>;
+}
+
 mod private {
     pub trait Sealed {}
 }
diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
index 816db7c57689..25190486a62d 100644
--- a/rust/kernel/led/normal.rs
+++ b/rust/kernel/led/normal.rs
@@ -6,11 +6,19 @@
 
 use super::*;
 
+/// The led mode for the `struct led_classdev`. Leds with this mode can only have a fixed color.
+pub enum Normal {}
+
+impl Mode for Normal {
+    type Device<'bound, T: LedOps<Mode = Self> + 'bound> = Device<'bound, T>;
+}
+impl private::Sealed for Normal {}
+
 /// 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> {
+pub struct Device<'bound, T: LedOps<Mode = Normal> + 'bound> {
     #[pin]
     ops: T,
     #[pin]
@@ -20,7 +28,7 @@ pub struct Device<'bound, T: LedOps + 'bound> {
 
 impl<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> {
     /// Registers a new [`Device`].
-    pub fn build<'bound: 'init, T: LedOps + 'bound>(
+    pub fn build<'bound: 'init, T: LedOps<Mode = Normal> + 'bound>(
         self,
         parent: &'bound T::Bus,
         ops: impl PinInit<T, Error> + 'init,
@@ -87,7 +95,7 @@ pub fn build<'bound: 'init, T: LedOps + 'bound>(
     }
 }
 
-impl<'bound, T: LedOps + 'bound> Device<'bound, T> {
+impl<'bound, T: LedOps<Mode = Normal> + 'bound> Device<'bound, T> {
     /// # Safety
     /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
     /// `led::Device`.
@@ -107,17 +115,17 @@ fn parent(&self) -> &'bound device::Device<Bound> {
 }
 
 // SAFETY: A `led::Device` can be unregistered from any thread.
-unsafe impl<'bound, T: LedOps + 'bound + Send> Send for Device<'bound, T> {}
+unsafe impl<'bound, T: LedOps<Mode = Normal> + 'bound + Send> Send for Device<'bound, T> {}
 
 // SAFETY: `led::Device` can be shared among threads because all methods of `led::Device`
 // are thread safe.
-unsafe impl<'bound, T: LedOps + 'bound + Sync> Sync for Device<'bound, T> {}
+unsafe impl<'bound, T: LedOps<Mode = Normal> + 'bound + Sync> Sync for Device<'bound, T> {}
 
 struct Adapter<T: LedOps> {
     _p: PhantomData<T>,
 }
 
-impl<T: LedOps> Adapter<T> {
+impl<T: LedOps<Mode = Normal>> Adapter<T> {
     /// # Safety
     /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
     /// `led::Device`.
@@ -210,7 +218,7 @@ impl<T: LedOps> Adapter<T> {
 }
 
 #[pinned_drop]
-impl<'bound, T: LedOps + 'bound> PinnedDrop for Device<'bound, T> {
+impl<'bound, T: LedOps<Mode = Normal> + 'bound> PinnedDrop for Device<'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

-- 
2.54.0

[PATCH v24 3/4] rust: leds: add multicolor classdev abstractions

[Markus Probst]

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   | 460 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 494 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 5d7e6d08ad2a..3aa7cdde78f4 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..82fe96fd5273
--- /dev/null
+++ b/rust/kernel/led/multicolor.rs
@@ -0,0 +1,460 @@
+// 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>,
+    /// 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,
+}
+
+// SAFETY: `MultiColorSubLed` can be shared among threads.
+unsafe impl Sync for MultiColorSubLed {}
+
+impl Clone for MultiColorSubLed {
+    fn clone(&self) -> Self {
+        Self {
+            color: self.color,
+            brightness: self.brightness().into(),
+            intensity: self.intensity().into(),
+            max_intensity: self.max_intensity,
+            channel: self.channel,
+        }
+    }
+}
+
+// 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: UnsafeCell::new(0),
+        intensity: UnsafeCell::new(0),
+        max_intensity: None,
+        channel: 0,
+    };
+    let c_zeroed = bindings::mc_subled {
+        color_index: 0,
+        brightness: 0,
+        intensity: 0,
+        max_intensity: 0,
+        channel: 0,
+    };
+
+    assert!(offset_of!(MultiColorSubLed, color) == offset_of!(bindings::mc_subled, color_index));
+    assert!(
+        offset_of!(MultiColorSubLed, brightness) == offset_of!(bindings::mc_subled, brightness)
+    );
+    assert!(offset_of!(MultiColorSubLed, intensity) == offset_of!(bindings::mc_subled, intensity));
+    assert!(
+        offset_of!(MultiColorSubLed, max_intensity)
+            == offset_of!(bindings::mc_subled, max_intensity)
+    );
+    assert!(offset_of!(MultiColorSubLed, channel) == offset_of!(bindings::mc_subled, channel));
+
+    assert_same_type(&0u32, &c_zeroed.color_index);
+    assert_same_type(&rust_zeroed.brightness.into_inner(), &c_zeroed.brightness);
+    assert_same_type(&rust_zeroed.intensity.into_inner(), &c_zeroed.intensity);
+    assert!(size_of_val(&rust_zeroed.max_intensity) == size_of_val(&c_zeroed.max_intensity));
+    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: UnsafeCell::new(0),
+            intensity: UnsafeCell::new(0),
+            max_intensity: None,
+            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 = UnsafeCell::new(intensity);
+        self
+    }
+
+    /// Set the maximum supported intensity of the subled.
+    #[inline]
+    pub const fn max_intensity(mut self, max_intensity: NonZero<u32>) -> Self {
+        self.max_intensity = Some(max_intensity);
+        self
+    }
+
+    /// The intensity of the sub led.
+    #[inline]
+    pub const fn intensity(&self) -> u32 {
+        // SAFETY:
+        // - `self.intensity.get()` is a valid pointer to `u32`.
+        // - We don't have exclusive or immutable access to `self.intensity`,
+        //   but the alignment should prevent "load tearing".
+        unsafe { *self.intensity.get() }
+    }
+
+    /// The brightness of the sub led.
+    #[inline]
+    pub const fn brightness(&self) -> u32 {
+        // SAFETY:
+        // - `self.brightness.get()` is a valid pointer to `u32`.
+        // - We don't have exclusive or immutable access to `self.brightness`,
+        //   but the alignment should prevent "load tearing".
+        unsafe { *self.brightness.get() }
+    }
+}
+
+/// 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<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> {
+    /// Registers a new [`MulticolorDevice`].
+    pub fn build_multicolor<'bound: 'init, T: LedOps<Mode = MultiColor> + 'bound>(
+        self,
+        parent: &'bound T::Bus,
+        ops: impl PinInit<T, Error> + 'init,
+        subleds: &'init [MultiColorSubLed],
+    ) -> impl PinInit<MultiColorDevice<'bound, T>, Error> + 'init {
+        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| {
+                let mut colors = [false; bindings::LED_COLOR_ID_MAX as usize];
+                for subled in subleds {
+                    if colors[subled.color as usize] {
+                        dev_err!(parent.as_ref(), "duplicate color in multicolor led\n");
+                        return Err(EINVAL);
+                    }
+                    colors[subled.color as usize] = true;
+                }
+                let subleds_box = KBox::pin_slice(
+                    |index| Ok::<_, Error>(subleds[index].clone()),
+                    subleds.len(),
+                    GFP_KERNEL,
+                )?;
+                let subleds_box_raw = KBox::into_raw(Pin::into_inner(subleds_box));
+
+                let subled_guard = ScopeGuard::new(|| {
+                    // SAFETY: `subleds_box_raw` is guaranteed to be a valid pointer to
+                    // `[MultiColorSubLed]`.
+                    drop(unsafe { <KBox<[MultiColorSubLed]>>::from_raw(subleds_box_raw) });
+                });
+
+                // 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(subleds_box_raw.len())?,
+                        // CAST: The safeguards in the const block ensure that
+                        // `MultiColorSubLed` has an identical layout to `mc_subled`.
+                        subled_info: subleds_box_raw.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`.
+    #[inline]
+    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::DeviceBuilder::build_multicolor`.
+            .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: `led_cdev.subled_info` is guaranteed to be a valid pointer to
+        // `[MultiColorSubLed]` with length of `led_cdev.num_colors`.
+        // See `led::DeviceBuilder::build_multicolor`.
+        drop(unsafe {
+            KBox::from_raw(ptr::slice_from_raw_parts_mut(
+                led_cdev.subled_info.cast::<MultiColorSubLed>(),
+                // CAST: `num_colors` was originally converted from a `usize` in
+                // `DeviceBuilder::build_multicolor`. Thus it is guaranteed that
+                // `num_colors` fits into an usize.
+                led_cdev.num_colors as usize,
+            ))
+        });
+    }
+}

-- 
2.54.0

[PATCH v24 4/4] MAINTAINERS: rust: leds: Add rust abstraction entry

[Markus Probst]

Add self as maintainer.

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 MAINTAINERS | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3a19da74d00c..54931e850570 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14795,6 +14795,14 @@ F:	drivers/leds/
 F:	include/dt-bindings/leds/
 F:	include/linux/leds.h
 
+LED SUBSYSTEM [RUST]
+M:	Markus Probst <markus.probst@posteo.de>
+L:	linux-leds@vger.kernel.org
+L:	rust-for-linux@vger.kernel.org
+S:	Maintained
+F:	rust/kernel/led.rs
+F:	rust/kernel/led/
+
 LEGO MINDSTORMS EV3
 R:	David Lechner <david@lechnology.com>
 S:	Maintained

-- 
2.54.0

Re: [PATCH v24 2/4] rust: leds: add Mode trait

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The commit message description starts with a lowercase letter, which violates the LED subsystem capitalization guidelines.
--

commit 549c02b13bc8cc96fcda86b2df1afa6c3c034eac
Author: Markus Probst <markus.probst@posteo.de>

rust: leds: add Mode trait

This commit adds the led::Mode trait to allow for other types of led class
devices in led::LedOps.

> rust: leds: add Mode trait

[Severity: Low]
This isn't a bug, but the commit message description starts with a lowercase
letter. The LED subsystem guidelines request capitalizing the description
after the subsystem prefix.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-rust_leds-v24-0-8d9e4c04db69@posteo.de?part=2

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Plain non-atomic reads of `intensity` and `brightness` from `UnsafeCell` in `MultiColorSubLed` cause a data race and Undefined Behavior with concurrent C subsystem modifications.
- [High] Calling `led_mc_calc_color_components` outside of the driver's locking creates a data race and TOCTOU vulnerability.
- [High] Casting concurrent pointers to `&mut usize` in `blink_set_callback` creates an aliasing violation (Undefined Behavior).
- [Low] The commit message description is not capitalized, violating the LED subsystem guidelines.
--

commit 0c1eb1680fe4ed1aa0ee60e6b95decec69575365
Author: Markus Probst <markus.probst@posteo.de>

rust: leds: add multicolor classdev abstractions

This commit introduces the necessary Rust abstractions to support multicolor
LED class devices. It provides wrappers around the C structures mc_subled
and led_classdev_mc, alongside integration with the LED device builder.

> rust: leds: add multicolor classdev abstractions
>
> Implement the abstractions needed for multicolor led class devices,

[Severity: Low]
This isn't a bug, but the LED subsystem guidelines require capitalizing the
description after the subsystem prefix. Should the subject be capitalized as
"Add multicolor classdev abstractions", and similarly the body text?

> diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
> new file mode 100644
> --- /dev/null
> +++ b/rust/kernel/led/multicolor.rs
[ ... ]
> +impl MultiColorSubLed {
[ ... ]
> +    pub const fn intensity(&self) -> u32 {
> +        // SAFETY:
> +        // - self.intensity.get() is a valid pointer to u32.
> +        // - We don't have exclusive or immutable access to self.intensity,
> +        //   but the alignment should prevent "load tearing".
> +        unsafe { *self.intensity.get() }
> +    }

[Severity: High]
Does this plain read cause Undefined Behavior under Rust's concurrency rules?

If a trigger from timer or interrupt context calls intensity() concurrently
while sysfs (via multi_intensity_store) modifies the memory under the
led_access mutex, this creates a data race.

Since plain non-atomic reads of concurrently mutated memory are considered
Undefined Behavior in Rust, should this use an atomic read or read_volatile
instead?

> +
> +    /// The brightness of the sub led.
> +    #[inline]
> +    pub const fn brightness(&self) -> u32 {
> +        // SAFETY:
> +        // - self.brightness.get() is a valid pointer to u32.
> +        // - We don't have exclusive or immutable access to self.brightness,
> +        //   but the alignment should prevent "load tearing".
> +        unsafe { *self.brightness.get() }
> +    }

[Severity: High]
Does this have the same data race and Undefined Behavior issue as the
intensity() read above?

[ ... ]
> +    unsafe extern "C" fn brightness_set_callback(
> +        led_cdev: *mut bindings::led_classdev,
> +        brightness: u32,
> +    ) {
[ ... ]
> +        // 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);
> +    }

[Severity: High]
Can calling bindings::led_mc_calc_color_components() before the driver 
acquires its lock cause data corruption of the hardware state?

Concurrent calls to brightness_set_callback, for example from sysfs and a
hardware trigger, will execute bindings::led_mc_calc_color_components()
outside any locks, racing to overwrite the subled_info array. 

The driver's lock in classdev.ops.brightness_set is acquired too late to
prevent this corruption. Should C drivers invoke
bindings::led_mc_calc_color_components() while holding their own lock
instead?

[ ... ]
> +    unsafe extern "C" fn brightness_set_blocking_callback(
> +        led_cdev: *mut bindings::led_classdev,
> +        brightness: u32,
> +    ) -> i32 {
> +        from_result(|| {
[ ... ]
> +            // 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)
> +        })
> +    }

[Severity: High]
Does this have the same data race vulnerability as brightness_set_callback()
above?

[ ... ]
> +    unsafe extern "C" fn blink_set_callback(
> +        led_cdev: *mut bindings::led_classdev,
> +        delay_on: *mut usize,
> +        delay_off: *mut usize,
> +    ) -> i32 {
> +        from_result(|| {
[ ... ]
> +            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)
> +        })
> +    }

[Severity: High]
Does casting these pointers to exclusive mutable references violate Rust's
aliasing rules and cause Undefined Behavior?

The C subsystem calls blink_set with pointers to led_cdev->blink_delay_on
and blink_delay_off. Concurrently, sysfs handlers like delay_on_store write
to these fields without taking a lock. 
    
Casting these shared, concurrently accessed pointers to exclusive mutable
references falsely asserts exclusive access.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-rust_leds-v24-0-8d9e4c04db69@posteo.de?part=3

Re: [PATCH v24 1/4] rust: leds: add basic led classdev abstractions

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The LED abstraction uses the unmanaged `led_classdev_register_ext` API, violating subsystem guidelines and introducing memory safety vulnerabilities.
- [High] DeviceBuilder unsafely recovers the `FwNode` reference count from the mutable C structure `dev->fwnode`, leading to use-after-free or memory leaks.
- [High] Concurrent calls to `blink_set_callback` create aliased mutable references to shared `delay_on` and `delay_off` variables, causing Undefined Behavior.
- [Medium] Eager evaluation of `.then_some()` forces instantiation of optional vtable callbacks, causing linker errors if methods are omitted.
--

commit 729f3d81e7ef8cb49db95c3c7c2412eca5d86a36
Author: Markus Probst <markus.probst@posteo.de>

rust: leds: add basic led classdev abstractions

This commit implements the core Rust abstractions for the LED subsystem. It
introduces traits and structures such as LedOps, DeviceBuilder, and Device
to safely wrap and manage led_classdev registration and operations.

> diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> index 0000000000000..816db7c576894
> --- a/rust/kernel/led/normal.rs
> +++ b/rust/kernel/led/normal.rs

[ ... ]

> @@ -25,58 +25,58 @@ impl<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> {
>          self,
>          parent: &'bound T::Bus,
>          ops: impl PinInit<T, Error> + 'init,
>      ) -> impl PinInit<Device<'bound, T>, Error> + 'init {
>          const_assert!(T::MAX_BRIGHTNESS <= i32::MAX.unsigned_abs() || !T::HAS_BRIGHTNESS_GET);
>  
>          try_pin_init!(Device {
>              ops <- ops,
>              classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev| {
>                  // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
>                  // `led_classdev` gets fully initialized in-place by
>                  // `led_classdev_register_ext` including `mutex` and `list_head`.
>                  unsafe {
>                      ptr.write(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),

[Severity: Medium]
Since .then_some() is a method call, doesn't it evaluate its argument eagerly?

Passing Adapter::<T>::brightness_get_callback forces the compiler to
instantiate the function body, which in turn evaluates T::brightness_get.
If the trait method is omitted by the driver, it defaults to invoking
build_error!().

Will this forced instantiation create a reference to the external symbol
rust_build_error that resists dead-code elimination and results in a
linker error?

Would a standard if T::HAS_BRIGHTNESS_GET { Some(...) } else { None } block
prevent this eager evaluation?

>                          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()
>                      })
>                  };
>  
>                  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`.
>                  to_result(unsafe {
>                      bindings::led_classdev_register_ext(
>                          parent.as_ref().as_raw(),
>                          ptr,

[Severity: High]
Does using the unmanaged led_classdev_register_ext() API here violate the LED
subsystem guidelines?

The guidelines mandate managed registration (devm_led_classdev_register_ext)
to prevent resource leaks and use-after-free bugs. If the Rust device is
safely leaked (e.g., via Box::leak or a cyclic Arc), the PinnedDrop
destructor is never called.

Won't this leave the unmanaged LED registered after the driver module
unloads, potentially causing an exploitable use-after-free kernel panic on
subsequent userspace sysfs accesses?

>                          if self.name.is_none() {
>                              &raw mut init_data
>                          } else {
>                              core::ptr::null_mut()
>                          },
>                      )
>                  })?;
>  
>                  core::mem::forget(self.fwnode); // keep the reference count incremented

[Severity: High]
Is it safe to unconditionally forget self.fwnode here?

If self.name is Some, init_data is passed as NULL to the C subsystem, so
the FwNode is ignored. Doesn't this permanently leak the ARef?

[ ... ]

> @@ -174,27 +174,27 @@ impl<T: LedOps> Adapter<T> {
>      /// # Safety
>      /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
>      /// `led::Device`.
>      /// `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::Device`.
>              let classdev = unsafe { Device::<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 },

[Severity: High]
Can this create aliased mutable references to shared variables?

The C LED subsystem allows lockless concurrent calls to led_blink_set() from
sysfs handlers like led_delay_on_store(). When multiple threads write to the
sysfs attributes concurrently, they pass pointers to the exact same shared
variables.

If the Rust abstraction unconditionally casts these raw pointers into
exclusive mutable references (&mut *delay_on and &mut *delay_off), doesn't
this violate Rust's aliasing rules and result in Undefined Behavior?

[ ... ]

> @@ -208,12 +208,12 @@ impl<T: LedOps> Adapter<T> {
>  #[pinned_drop]
>  impl<'bound, T: LedOps + 'bound> PinnedDrop for Device<'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`.
>          let dev: &device::Device = unsafe { device::Device::from_raw((*raw).dev) };
>  
>          let _fwnode = dev
>              .fwnode()
>              // SAFETY: the reference count of `fwnode` has previously been
>              // incremented in `led::DeviceBuilder::build`.
>              .map(|fwnode| unsafe { ARef::from_raw(NonNull::from(fwnode)) });

[Severity: High]
Could this reconstruct an ARef from a globally-mutable C pointer and cause a
Use-After-Free?

If self.fwnode was None during DeviceBuilder::build, forget() does nothing.
But if the kernel later dynamically assigns an fwnode to the class device
(e.g., via device links or driver core), this drop handler unconditionally
reconstructs an ARef and drops it, decrementing a reference count it never
incremented.

Should the Rust abstraction store its own Option<ARef> internally to
securely track ownership instead of relying on the C structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903-rust_leds-v24-0-8d9e4c04db69@posteo.de?part=1

Re: [PATCH v24 0/4] rust: leds: add led classdev abstractions

[Markus Probst]

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


@Lee, @Pavel:

Hi,

Regarding the leds rust abstraction patch series.

Lee already mentioned he will take Rust patches into his tree if
"quality Rust-specific Reviewed-by" trailers are present [1].
Will this also apply for any subsequent patches (e.g. bug fixes)?

Secondly, there hasn't been a comment on me taking the maintainer role
for the Rust abstraction yet. If thats fine with you, I kindly ask you
to ACK the patch adding the MAINTAINERS file entry [2].

I was told there have been cases in the past of maintainers leaving the
kernel directly after code has been merged. To take away any fears that
might exist, I can assure you I will take care of the Rust code.

If there is a patch with breaking API changes on the C side, simply add
me to Cc and I will fix the Rust side.

The missing ACK on the patch adding the MAINTAINERS file entry [2] and
the missing Reviewed-By Rust trailers should be the only blockers of
this patch series.

Thanks
- Markus Probst

[1]
https://lore.kernel.org/linux-leds/20251110134058.GC1949330@google.com/

[2]
https://lore.kernel.org/linux-leds/20260903-rust_leds-v24-4-8d9e4c04db69@posteo.de/

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

Re: [PATCH v24 1/4] rust: leds: add basic led classdev abstractions

[Gary Guo]

On Thu Sep 3, 2026 at 12:01 AM BST, 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@posteo.de>
> ---
>  rust/kernel/led.rs        | 288 ++++++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/led/normal.rs | 230 ++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs        |   1 +
>  3 files changed, 519 insertions(+)
>
> [snip]
>
> +/// Trait defining the operations for a LED driver.
> +///
> +/// # Examples
> +/// ```
> +/// use kernel::{
> +///      device,
> +///      devres::Devres,
> +///      led,
> +///      macros::vtable,
> +///      platform,
> +///      prelude::*, //
> +///  };
> +///
> +/// struct MyLedOps;
> +///
> +///
> +/// #[vtable]
> +/// impl led::LedOps for MyLedOps {
> +///     type Bus = platform::Device<device::Bound>;
> +///     const BLOCKING: bool = false;
> +///     const MAX_BRIGHTNESS: u32 = 255;
> +///
> +///     fn brightness_set<'bound>(
> +///         &self,
> +///         _dev: &'bound platform::Device<device::Bound>,
> +///         _classdev: &led::Device<'bound, Self>,
> +///         _brightness: u32
> +///     ) -> Result<()> {
> +///         // Set the brightness for the led here
> +///         Ok(())
> +///     }
> +/// }
> +/// ```
> +/// Led drivers must implement this trait in order to register and handle a [`Device`].
> +#[vtable]
> +pub trait LedOps: Send + Sync + Sized {
> +    /// The bus device required by the implementation.
> +    #[allow(private_bounds)]
> +    type Bus: AsBusDevice<Bound>;

Does LED class device has no private data that driver can use? This can be
either a private pointer or extra allocation living at the end of the classdev
struct.

It's usually a antipattern to get the bus device directly, especially that in
Rust we do not allow anything other than callbacks to access data on bus
devices.

Instead, the class device registration should provide a data initializer, and
the callbacks would receive a pointer to the data instead. In cases that a
device resource has to be referenced, it should be kept inside the private data
by the driver themselves.

Best,
Gary

> +
> +    /// If set true, [`LedOps::brightness_set`] and [`LedOps::blink_set`] must perform the
> +    /// operation immediately. If set false, they must not sleep.
> +    const BLOCKING: bool;
> +    /// The max brightness level.
> +    const MAX_BRIGHTNESS: u32;
> +
> +    /// Sets the brightness level.
> +    ///
> +    /// See also [`LedOps::BLOCKING`].
> +    fn brightness_set<'bound>(
> +        &self,
> +        dev: &'bound Self::Bus,
> +        classdev: &Device<'bound, Self>,
> +        brightness: u32,
> +    ) -> Result<()>;
> +
> +    /// Gets the current brightness level.
> +    fn brightness_get<'bound>(
> +        &self,
> +        dev: &'bound Self::Bus,
> +        classdev: &Device<'bound, Self>,
> +    ) -> Result<u32> {
> +        let _ = (dev, classdev);
> +        build_error!(VTABLE_DEFAULT_ERROR)
> +    }
> +
> +    /// Activates hardware accelerated blinking.
> +    ///
> +    /// delays are in milliseconds. If both are zero, a sensible default should be chosen.
> +    /// The caller should adjust the timings in that case and if it can't match the values
> +    /// specified exactly. Setting the brightness to 0 will disable the hardware accelerated
> +    /// blinking.
> +    ///
> +    /// See also [`LedOps::BLOCKING`].
> +    fn blink_set<'bound>(
> +        &self,
> +        dev: &'bound Self::Bus,
> +        classdev: &Device<'bound, Self>,
> +        delay_on: &mut usize,
> +        delay_off: &mut usize,
> +    ) -> Result<()> {
> +        let _ = (dev, classdev, delay_on, delay_off);
> +        build_error!(VTABLE_DEFAULT_ERROR)
> +    }
> +}

Re: [PATCH v24 1/4] rust: leds: add basic led classdev abstractions

[Markus Probst]

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

On Fri, 2026-09-04 at 14:03 +0100, Gary Guo wrote:
> On Thu Sep 3, 2026 at 12:01 AM BST, 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@posteo.de>
> > ---
> >  rust/kernel/led.rs        | 288 ++++++++++++++++++++++++++++++++++++++++++++++
> >  rust/kernel/led/normal.rs | 230 ++++++++++++++++++++++++++++++++++++
> >  rust/kernel/lib.rs        |   1 +
> >  3 files changed, 519 insertions(+)
> > 
> > [snip]
> > 
> > +/// Trait defining the operations for a LED driver.
> > +///
> > +/// # Examples
> > +/// ```
> > +/// use kernel::{
> > +///      device,
> > +///      devres::Devres,
> > +///      led,
> > +///      macros::vtable,
> > +///      platform,
> > +///      prelude::*, //
> > +///  };
> > +///
> > +/// struct MyLedOps;
> > +///
> > +///
> > +/// #[vtable]
> > +/// impl led::LedOps for MyLedOps {
> > +///     type Bus = platform::Device<device::Bound>;
> > +///     const BLOCKING: bool = false;
> > +///     const MAX_BRIGHTNESS: u32 = 255;
> > +///
> > +///     fn brightness_set<'bound>(
> > +///         &self,
> > +///         _dev: &'bound platform::Device<device::Bound>,
> > +///         _classdev: &led::Device<'bound, Self>,
> > +///         _brightness: u32
> > +///     ) -> Result<()> {
> > +///         // Set the brightness for the led here
> > +///         Ok(())
> > +///     }
> > +/// }
> > +/// ```
> > +/// Led drivers must implement this trait in order to register and handle a [`Device`].
> > +#[vtable]
> > +pub trait LedOps: Send + Sync + Sized {
> > +    /// The bus device required by the implementation.
> > +    #[allow(private_bounds)]
> > +    type Bus: AsBusDevice<Bound>;
> 
> Does LED class device has no private data that driver can use? This can be
> either a private pointer or extra allocation living at the end of the classdev
> struct.
On every callback `&self` is passed to the LedOps, which could be
considered the leds private data. It is currently stored in front of
the `led_classdev` struct.
> 
> It's usually a antipattern to get the bus device directly, especially that in
> Rust we do not allow anything other than callbacks to access data on bus
> devices.
> 
> Instead, the class device registration should provide a data initializer, and
> the callbacks would receive a pointer to the data instead. In cases that a
> device resource has to be referenced, it should be kept inside the private data
> by the driver themselves.
It should be possible to store a pointer to the bus device directly on
this data, thus I can remove it.

If I think about it, I could add a `led::Device::drvdata` function, so
it could be accessed from the drivers private data. Sync is a
requirement anyway.

Thanks
- Markus Probst

> 
> Best,
> Gary
> 
> > +
> > +    /// If set true, [`LedOps::brightness_set`] and [`LedOps::blink_set`] must perform the
> > +    /// operation immediately. If set false, they must not sleep.
> > +    const BLOCKING: bool;
> > +    /// The max brightness level.
> > +    const MAX_BRIGHTNESS: u32;
> > +
> > +    /// Sets the brightness level.
> > +    ///
> > +    /// See also [`LedOps::BLOCKING`].
> > +    fn brightness_set<'bound>(
> > +        &self,
> > +        dev: &'bound Self::Bus,
> > +        classdev: &Device<'bound, Self>,
> > +        brightness: u32,
> > +    ) -> Result<()>;
> > +
> > +    /// Gets the current brightness level.
> > +    fn brightness_get<'bound>(
> > +        &self,
> > +        dev: &'bound Self::Bus,
> > +        classdev: &Device<'bound, Self>,
> > +    ) -> Result<u32> {
> > +        let _ = (dev, classdev);
> > +        build_error!(VTABLE_DEFAULT_ERROR)
> > +    }
> > +
> > +    /// Activates hardware accelerated blinking.
> > +    ///
> > +    /// delays are in milliseconds. If both are zero, a sensible default should be chosen.
> > +    /// The caller should adjust the timings in that case and if it can't match the values
> > +    /// specified exactly. Setting the brightness to 0 will disable the hardware accelerated
> > +    /// blinking.
> > +    ///
> > +    /// See also [`LedOps::BLOCKING`].
> > +    fn blink_set<'bound>(
> > +        &self,
> > +        dev: &'bound Self::Bus,
> > +        classdev: &Device<'bound, Self>,
> > +        delay_on: &mut usize,
> > +        delay_off: &mut usize,
> > +    ) -> Result<()> {
> > +        let _ = (dev, classdev, delay_on, delay_off);
> > +        build_error!(VTABLE_DEFAULT_ERROR)
> > +    }
> > +}

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]

Re: [PATCH v24 1/4] rust: leds: add basic led classdev abstractions

[Gary Guo]

On Fri Sep 4, 2026 at 2:15 PM BST, Markus Probst wrote:
> On Fri, 2026-09-04 at 14:03 +0100, Gary Guo wrote:
>> On Thu Sep 3, 2026 at 12:01 AM BST, 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@posteo.de>
>> > ---
>> >  rust/kernel/led.rs        | 288 ++++++++++++++++++++++++++++++++++++++++++++++
>> >  rust/kernel/led/normal.rs | 230 ++++++++++++++++++++++++++++++++++++
>> >  rust/kernel/lib.rs        |   1 +
>> >  3 files changed, 519 insertions(+)
>> > 
>> > [snip]
>> > 
>> > +/// Trait defining the operations for a LED driver.
>> > +///
>> > +/// # Examples
>> > +/// ```
>> > +/// use kernel::{
>> > +///      device,
>> > +///      devres::Devres,
>> > +///      led,
>> > +///      macros::vtable,
>> > +///      platform,
>> > +///      prelude::*, //
>> > +///  };
>> > +///
>> > +/// struct MyLedOps;
>> > +///
>> > +///
>> > +/// #[vtable]
>> > +/// impl led::LedOps for MyLedOps {
>> > +///     type Bus = platform::Device<device::Bound>;
>> > +///     const BLOCKING: bool = false;
>> > +///     const MAX_BRIGHTNESS: u32 = 255;
>> > +///
>> > +///     fn brightness_set<'bound>(
>> > +///         &self,
>> > +///         _dev: &'bound platform::Device<device::Bound>,
>> > +///         _classdev: &led::Device<'bound, Self>,
>> > +///         _brightness: u32
>> > +///     ) -> Result<()> {
>> > +///         // Set the brightness for the led here
>> > +///         Ok(())
>> > +///     }
>> > +/// }
>> > +/// ```
>> > +/// Led drivers must implement this trait in order to register and handle a [`Device`].
>> > +#[vtable]
>> > +pub trait LedOps: Send + Sync + Sized {
>> > +    /// The bus device required by the implementation.
>> > +    #[allow(private_bounds)]
>> > +    type Bus: AsBusDevice<Bound>;
>> 
>> Does LED class device has no private data that driver can use? This can be
>> either a private pointer or extra allocation living at the end of the classdev
>> struct.
> On every callback `&self` is passed to the LedOps, which could be
> considered the leds private data. It is currently stored in front of
> the `led_classdev` struct.

Right, I missed that. In that case I think you can just remove `Bus` completely
from the callback?

Do you have a user that needs this info? BTW, it would also help to include a
link to a potential user in the cover letter so people can see how the API is
supposed to be used. This is especially useful for API design reviews.

>> 
>> It's usually a antipattern to get the bus device directly, especially that in
>> Rust we do not allow anything other than callbacks to access data on bus
>> devices.
>> 
>> Instead, the class device registration should provide a data initializer, and
>> the callbacks would receive a pointer to the data instead. In cases that a
>> device resource has to be referenced, it should be kept inside the private data
>> by the driver themselves.
> It should be possible to store a pointer to the bus device directly on
> this data, thus I can remove it.
>
> If I think about it, I could add a `led::Device::drvdata` function, so
> it could be accessed from the drivers private data. Sync is a
> requirement anyway.

An option is to provide `Deref`. Then you could even have
`self: &Device<'bound, Self>` in callbacks.

That said, you might want to eventually support type-erased `Device` types to
support consumer of LED class devices. So I'm unsure if we want to provide data
accessors on class devices (maybe eventually device'll be split into two types?)

Best,
Gary

Re: [PATCH v24 1/4] rust: leds: add basic led classdev abstractions

[Markus Probst]

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

On Fri, 2026-09-04 at 14:32 +0100, Gary Guo wrote:
> On Fri Sep 4, 2026 at 2:15 PM BST, Markus Probst wrote:
> > On Fri, 2026-09-04 at 14:03 +0100, Gary Guo wrote:
> > > On Thu Sep 3, 2026 at 12:01 AM BST, 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@posteo.de>
> > > > ---
> > > >  rust/kernel/led.rs        | 288 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  rust/kernel/led/normal.rs | 230 ++++++++++++++++++++++++++++++++++++
> > > >  rust/kernel/lib.rs        |   1 +
> > > >  3 files changed, 519 insertions(+)
> > > > 
> > > > [snip]
> > > > 
> > > > +/// Trait defining the operations for a LED driver.
> > > > +///
> > > > +/// # Examples
> > > > +/// ```
> > > > +/// use kernel::{
> > > > +///      device,
> > > > +///      devres::Devres,
> > > > +///      led,
> > > > +///      macros::vtable,
> > > > +///      platform,
> > > > +///      prelude::*, //
> > > > +///  };
> > > > +///
> > > > +/// struct MyLedOps;
> > > > +///
> > > > +///
> > > > +/// #[vtable]
> > > > +/// impl led::LedOps for MyLedOps {
> > > > +///     type Bus = platform::Device<device::Bound>;
> > > > +///     const BLOCKING: bool = false;
> > > > +///     const MAX_BRIGHTNESS: u32 = 255;
> > > > +///
> > > > +///     fn brightness_set<'bound>(
> > > > +///         &self,
> > > > +///         _dev: &'bound platform::Device<device::Bound>,
> > > > +///         _classdev: &led::Device<'bound, Self>,
> > > > +///         _brightness: u32
> > > > +///     ) -> Result<()> {
> > > > +///         // Set the brightness for the led here
> > > > +///         Ok(())
> > > > +///     }
> > > > +/// }
> > > > +/// ```
> > > > +/// Led drivers must implement this trait in order to register and handle a [`Device`].
> > > > +#[vtable]
> > > > +pub trait LedOps: Send + Sync + Sized {
> > > > +    /// The bus device required by the implementation.
> > > > +    #[allow(private_bounds)]
> > > > +    type Bus: AsBusDevice<Bound>;
> > > 
> > > Does LED class device has no private data that driver can use? This can be
> > > either a private pointer or extra allocation living at the end of the classdev
> > > struct.
> > On every callback `&self` is passed to the LedOps, which could be
> > considered the leds private data. It is currently stored in front of
> > the `led_classdev` struct.
> 
> Right, I missed that. In that case I think you can just remove `Bus` completely
> from the callback?
Yes.

> 
> Do you have a user that needs this info? BTW, it would also help to include a
> link to a potential user in the cover letter so people can see how the API is
> supposed to be used. This is especially useful for API design reviews.
Primarily
https://lore.kernel.org/rust-for-linux/20260724-synology_microp_initial-v18-0-fb2f49f10e77@posteo.de/

.
But I also have another rust i2c driver, which would use the led
abstraction:

https://codeberg.org/0xIO32/linux/src/branch/synology_disk_leds

Still needs changes before it can be submitted.


> 
> > > 
> > > It's usually a antipattern to get the bus device directly, especially that in
> > > Rust we do not allow anything other than callbacks to access data on bus
> > > devices.
> > > 
> > > Instead, the class device registration should provide a data initializer, and
> > > the callbacks would receive a pointer to the data instead. In cases that a
> > > device resource has to be referenced, it should be kept inside the private data
> > > by the driver themselves.
> > It should be possible to store a pointer to the bus device directly on
> > this data, thus I can remove it.
> > 
> > If I think about it, I could add a `led::Device::drvdata` function, so
> > it could be accessed from the drivers private data. Sync is a
> > requirement anyway.
> 
> An option is to provide `Deref`. Then you could even have
> `self: &Device<'bound, Self>` in callbacks.
Didn't know we had `feature(arbitrary_self_types)` enabled.
I like this idea.

> 
> That said, you might want to eventually support type-erased `Device` types to
> support consumer of LED class devices. So I'm unsure if we want to provide data
> accessors on class devices (maybe eventually device'll be split into two types?)
Sounds good, but I won't implement any consumer functions until they
are needed.

Thanks
- Markus Probst

> 
> Best,
> Gary

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 870 bytes --]