* [PATCH RESEND v23 3/3] rust: leds: add multicolor classdev abstractions
2026-08-05 19:34 [PATCH RESEND v23 0/3] rust: leds: add led " Markus Probst via B4 Relay
@ 2026-08-05 19:34 ` Markus Probst via B4 Relay
2026-08-05 20:52 ` sashiko-bot
0 siblings, 1 reply; 9+ messages in thread
From: Markus Probst via B4 Relay @ 2026-08-05 19:34 UTC (permalink / raw)
To: Lee Jones, Pavel Machek, Greg Kroah-Hartman, Dave Ertman,
Leon Romanovsky, Miguel Ojeda, Alex Gaynor, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Rafael J. Wysocki, Bjorn Helgaas,
Krzysztof Wilczyński, Boqun Feng, Daniel Almeida,
Tamir Duberstein, Alexandre Courbot, Onur Özkan, Ira Weiny,
Boqun Feng
Cc: rust-for-linux, linux-leds, linux-kernel, linux-pci,
Markus Probst
From: Markus Probst <markus.probst@posteo.de>
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 1124785e210b..821f539e597e 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -68,6 +68,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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND v23 3/3] rust: leds: add multicolor classdev abstractions
2026-08-05 19:34 ` [PATCH RESEND v23 3/3] rust: leds: add multicolor " Markus Probst via B4 Relay
@ 2026-08-05 20:52 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-05 20:52 UTC (permalink / raw)
To: Markus Probst; +Cc: ojeda, linux-leds, linux-pci, lee
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unsound fwnode reference counting and recovery logic leads to a memory leak or use-after-free.
- [High] Unsynchronized concurrent reads of `brightness` and `intensity` create a data race, resulting in Undefined Behavior.
- [Low] Commit message description is not capitalized, violating the LED subsystem guidelines.
--
--- Patch [3]: [PATCH RESEND v23 3/3] rust: leds: add multicolor classdev abstractions ---
commit 31ed8e2758580675a7a667a24dbbce311465d001
Author: Markus Probst <markus.probst@posteo.de>
rust: leds: add multicolor classdev abstractions
Implement the abstractions needed for multicolor led class devices,
including led::MultiColor, MultiColorSubLed, and led::MultiColorDevice.
> rust: leds: add multicolor classdev abstractions
[Severity: Low]
This isn't a bug, but the commit message description is not capitalized,
violating the LED subsystem guidelines. Should this be "Add multicolor
classdev abstractions"?
> diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
> new file mode 100644
> index 0000000000000..82fe96fd5273b
> --- /dev/null
> +++ b/rust/kernel/led/multicolor.rs
[ ... ]
> + /// 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() }
> + }
[Severity: High]
Does this code introduce undefined behavior due to unsynchronized concurrent
reads?
The C subsystem updates these fields concurrently when user-space writes to
sysfs. In Rust's memory model, plain pointer dereferences on non-atomic
variables that can be concurrently mutated constitute a data race and strict
undefined behavior.
Should this use core::ptr::read_volatile or atomics instead?
[ ... ]
> + // 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>(())
> + }),
[Severity: High]
Can this code leak the fwnode reference?
If self.name is present, null_mut() is passed for init_data, meaning
the fwnode is never passed to the C subsystem. However, core::mem::forget()
is called unconditionally, leaking the reference since the device will not
have the fwnode for recovery during drop.
[ ... ]
> +#[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)) });
[Severity: High]
Can this cause a use-after-free?
If self.fwnode was originally None, but the C subsystem matches and
assigns an fwnode from elsewhere (e.g. the parent device), this drop logic
will wrongly assume ownership of it and drop an unowned ARef.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-rust_leds-v23-0-b2fea4290ee6@posteo.de?part=3
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH RESEND v23 0/3] rust: leds: add led classdev abstractions
@ 2026-08-19 18:58 Markus Probst
2026-08-19 18:58 ` [PATCH RESEND v23 1/3] rust: leds: add basic " Markus Probst
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Markus Probst @ 2026-08-19 18:58 UTC (permalink / raw)
To: Lee Jones, Pavel Machek, Greg Kroah-Hartman, Dave Ertman,
Leon Romanovsky, Miguel Ojeda, Alex Gaynor, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Rafael J. Wysocki, Bjorn Helgaas,
Krzysztof Wilczyński, Boqun Feng, Daniel Almeida,
Tamir Duberstein, Alexandre Courbot, Onur Özkan, Ira Weiny,
Boqun Feng
Cc: rust-for-linux, linux-leds, linux-kernel, linux-pci,
Markus Probst
This patch series has previously been contained in
https://lore.kernel.org/rust-for-linux/20251008181027.662616-1-markus.probst@posteo.de/T/#t
which added a rust written led driver for a microcontroller via i2c.
As the reading and writing to the i2c client via the register!
macro has not been implemented yet [1], the patch series will only
contain the additional abstractions required.
This series depends on [1].
[1] https://lore.kernel.org/rust-for-linux/20260525202921.124698-1-dakr@kernel.org/
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 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 (3):
rust: leds: add basic led classdev abstractions
rust: leds: add Mode trait
rust: leds: add multicolor classdev abstractions
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: 1137d8b5df06137fb49513cc923b3b24d94cb809
change-id: 20251114-rust_leds-a959f7c2f7f9
-----BEGIN PGP SIGNATURE-----
iQJPBAABCAA5FiEEgnQYxPSsWOdyMMRzNHYf+OetQ9IFAmpZGhcbFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMiwyAAoJEDR2H/jnrUPSIFkQAIJHfAe6S+v8u4v29fke
uLDop7Ga9Ujz38DD9iMIdnRhgDeh3eN7boFgMab3XM2JhFEuseCkDwIGdybF+zPx
EEoXxpXZp8KcTR26ryN4RuFax/q4nwZaXnZt133COFMPSiHswyn0ZjjlZQb5pK4L
ikKXcCCTWouQkdVD/WB0bTW1Bo065QpCvMP5NTWWQ109EnFTGOofbWv9FV+YzOy0
/baQI/20IRW8CU4s3FG8/LKc5F7yHCYkX89hmUtK9+xiuoNgZgCyt1kuXASRB1ol
jVP1TGauphC0WC/DqL0U4OQcJfMF4vSFblAaLiEnp+YkTj/A/e3T+Y8sLGWI876T
9juYGeGbX8Lupsa3OzFe2csfYJwrMZzL7iOkugJQlVLWxsrTFAZsdStwZDx6mV7k
g1oeux9d9P+lM3gG6HT2jobC7Y+7hD3Z9qFnxQ4vP2tSpcQb0s9emO26JE7hvh9x
oK9C3XUa7Yan3umqeqU9Iz3fJ0w2ZFDNTK5P8LtBJnlagqlRWDCDVu+nx9rsaxXf
S3wh5LktdUB8Xi+qigybTwqjV1SMocfBciVmYJrP4xjhkz8z32WCZdLHY8rpFZ1h
kcVy33DBzv8wPK+fCntQVm9rDC5qmrVpfNOYrg1ZYDCG3Yd/ilL/7VxnR0boQ/5W
qvNRlYVg/i4TsPhwgFacMPzv
=wKUV
-----END PGP SIGNATURE-----
--
Markus Probst <markus.probst@posteo.de>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH RESEND v23 1/3] rust: leds: add basic led classdev abstractions
2026-08-19 18:58 [PATCH RESEND v23 0/3] rust: leds: add led classdev abstractions Markus Probst
@ 2026-08-19 18:58 ` Markus Probst
2026-08-19 19:14 ` sashiko-bot
2026-08-19 18:58 ` [PATCH RESEND v23 2/3] rust: leds: add Mode trait Markus Probst
2026-08-19 18:58 ` [PATCH RESEND v23 3/3] rust: leds: add multicolor classdev abstractions Markus Probst
2 siblings, 1 reply; 9+ messages in thread
From: Markus Probst @ 2026-08-19 18:58 UTC (permalink / raw)
To: Lee Jones, Pavel Machek, Greg Kroah-Hartman, Dave Ertman,
Leon Romanovsky, Miguel Ojeda, Alex Gaynor, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Rafael J. Wysocki, Bjorn Helgaas,
Krzysztof Wilczyński, Boqun Feng, Daniel Almeida,
Tamir Duberstein, Alexandre Courbot, Onur Özkan, Ira Weiny,
Boqun Feng
Cc: rust-for-linux, linux-leds, linux-kernel, linux-pci,
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>
---
MAINTAINERS | 8 ++
rust/kernel/led.rs | 288 ++++++++++++++++++++++++++++++++++++++++++++++
rust/kernel/led/normal.rs | 230 ++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +
4 files changed, 527 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 806bd2d80d15..70989c4c693c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14662,6 +14662,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
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 9512af7156df..09e4cc7a14db 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -90,6 +90,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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH RESEND v23 2/3] rust: leds: add Mode trait
2026-08-19 18:58 [PATCH RESEND v23 0/3] rust: leds: add led classdev abstractions Markus Probst
2026-08-19 18:58 ` [PATCH RESEND v23 1/3] rust: leds: add basic " Markus Probst
@ 2026-08-19 18:58 ` Markus Probst
2026-08-19 19:11 ` sashiko-bot
2026-08-19 18:58 ` [PATCH RESEND v23 3/3] rust: leds: add multicolor classdev abstractions Markus Probst
2 siblings, 1 reply; 9+ messages in thread
From: Markus Probst @ 2026-08-19 18:58 UTC (permalink / raw)
To: Lee Jones, Pavel Machek, Greg Kroah-Hartman, Dave Ertman,
Leon Romanovsky, Miguel Ojeda, Alex Gaynor, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Rafael J. Wysocki, Bjorn Helgaas,
Krzysztof Wilczyński, Boqun Feng, Daniel Almeida,
Tamir Duberstein, Alexandre Courbot, Onur Özkan, Ira Weiny,
Boqun Feng
Cc: rust-for-linux, linux-leds, linux-kernel, linux-pci,
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH RESEND v23 3/3] rust: leds: add multicolor classdev abstractions
2026-08-19 18:58 [PATCH RESEND v23 0/3] rust: leds: add led classdev abstractions Markus Probst
2026-08-19 18:58 ` [PATCH RESEND v23 1/3] rust: leds: add basic " Markus Probst
2026-08-19 18:58 ` [PATCH RESEND v23 2/3] rust: leds: add Mode trait Markus Probst
@ 2026-08-19 18:58 ` Markus Probst
2026-08-19 19:10 ` sashiko-bot
2 siblings, 1 reply; 9+ messages in thread
From: Markus Probst @ 2026-08-19 18:58 UTC (permalink / raw)
To: Lee Jones, Pavel Machek, Greg Kroah-Hartman, Dave Ertman,
Leon Romanovsky, Miguel Ojeda, Alex Gaynor, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Rafael J. Wysocki, Bjorn Helgaas,
Krzysztof Wilczyński, Boqun Feng, Daniel Almeida,
Tamir Duberstein, Alexandre Courbot, Onur Özkan, Ira Weiny,
Boqun Feng
Cc: rust-for-linux, linux-leds, linux-kernel, linux-pci,
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 1124785e210b..821f539e597e 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -68,6 +68,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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND v23 3/3] rust: leds: add multicolor classdev abstractions
2026-08-19 18:58 ` [PATCH RESEND v23 3/3] rust: leds: add multicolor classdev abstractions Markus Probst
@ 2026-08-19 19:10 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-19 19:10 UTC (permalink / raw)
To: Markus Probst; +Cc: lee, linux-pci, ojeda, linux-leds
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect manual reference count management for `fwnode` causes memory leaks and potential Use-After-Free (UAF).
- [High] Plain, unsynchronized reads of `intensity` and `brightness` from `UnsafeCell` cause data races and Undefined Behavior.
- [Low] The commit message violates formatting conventions (capitalization) and contains a typo.
--
--- Patch [3]: [PATCH RESEND v23 3/3] rust: leds: add multicolor classdev abstractions ---
commit 2486b1ac4294e9421b47197f76e5b346cbfb370a
Author: Markus Probst <markus.probst@posteo.de>
rust: leds: add multicolor classdev abstractions
This patch implements the abstractions needed for multicolor led class devices.
It introduces the MultiColor mode, MultiColorSubLed wrapper, MultiColorDevice
wrapper, and device builder functions.
> rust: leds: add multicolor classdev abstractions
[Severity: Low]
This isn't a bug, but the commit message subject violates the subsystem
capitalization convention. Could this be capitalized as "Add multicolor..."?
> * `MultiColorSubLed` - a safe wrapper arround `mc_subled`
[Severity: Low]
This isn't a bug, but there is a typo (arround) in the commit message.
> 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
[ ... ]
> + 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() }
> + }
[Severity: High]
Can these plain reads cause data races and undefined behavior?
Since MultiColorDevice implements Sync, references can be safely shared across
threads. At the same time, the C LED subsystem can concurrently mutate these
fields asynchronously, for example via sysfs writes handled by
multi_intensity_store() or background state calculations in
led_mc_calc_color_components().
Concurrent unsynchronized read/write accesses to the same memory location,
where at least one is a write, are formal data races in Rust. Could these be
wrapped with proper atomic accesses or synchronization?
[ ... ]
> + // 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>(())
[Severity: High]
Can this sequence cause a reference leak or a use-after-free for the fwnode?
If a device name is provided, led_classdev_multicolor_register_ext() is called
with a null pointer for init_data to force the use of the provided name.
Consequently, the C LED subsystem does not copy the fwnode from init_data into
the device.
However, this code unconditionally calls core::mem::forget(self.fwnode).
If self.fwnode was Some, its reference count is leaked because
MultiColorDevice::drop() will later attempt to recover it using dev.fwnode(),
which will return None.
[ ... ]
> +#[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)) });
[Severity: High]
Following up on the previous observation, if self.fwnode was initially None,
but the C driver core later sets dev->fwnode (for example, via device_set_node
from an ACPI table or parent), could this code wrap that unowned pointer in
ARef::from_raw and improperly decrement its refcount upon destruction, causing
a use-after-free?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-rust_leds-v23-0-2716a4ef89a4@posteo.de?part=3
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND v23 2/3] rust: leds: add Mode trait
2026-08-19 18:58 ` [PATCH RESEND v23 2/3] rust: leds: add Mode trait Markus Probst
@ 2026-08-19 19:11 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-19 19:11 UTC (permalink / raw)
To: Markus Probst; +Cc: linux-pci, ojeda, lee, linux-leds
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unsound conversion of potentially aliased C pointers to `&mut usize` in `blink_set_callback`.
- [High] Relying on PinnedDrop and unmanaged led_classdev_unregister introduces a Use-After-Free (UAF) vulnerability on driver unbind if the Device instance outlives the parent device.
- [Low] The commit message description starts with a lowercase letter, violating LED subsystem conventions.
--
--- Patch [2]: [PATCH RESEND v23 2/3] rust: leds: add Mode trait ---
commit 8b1c998241db82c99cdf6f28eac6bffb4a27cef4
Author: Markus Probst <markus.probst@posteo.de>
rust: leds: add Mode trait
Add the led::Mode trait to allow for other types of led class devices
in led::LedOps.
> rust: leds: add Mode trait
[Severity: Low]
Is this commit subject formatted correctly? The LED subsystem guidelines
require capitalizing the description after the subsystem prefix.
> diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> index 816db7c576894..25190486a62d0 100644
> --- a/rust/kernel/led/normal.rs
> +++ b/rust/kernel/led/normal.rs
[ ... ]
> @@ -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,
[Severity: High]
Can relying on PinnedDrop to unregister the LED device result in a
use-after-free regression?
In this build function, the code calls bindings::led_classdev_register_ext()
and later relies on bindings::led_classdev_unregister() in PinnedDrop:
rust/kernel/led/normal.rs:DeviceBuilder::build() {
to_result(unsafe {
bindings::led_classdev_register_ext( ... )
})?;
...
rust/kernel/led/normal.rs:PinnedDrop for Device {
fn drop(self: Pin<&mut Self>) {
...
unsafe { bindings::led_classdev_unregister(raw) };
}
If the Device instance outlives the parent device (for example, if it is
wrapped in an Arc and kept alive by another thread), the Drop
implementation will not execute at driver unbind time.
Because led_classdev_unregister is not called during unbind, the LED sysfs
attributes would remain active. Subsequent sysfs accesses could then invoke
callbacks that dereference the freed parent device pointer.
Should devm_led_classdev_register_ext() be used to explicitly tie the
teardown to the unbind lifecycle?
[ ... ]
> @@ -107,17 +115,17 @@ fn parent(&self) -> &'bound device::Device<Bound> {
> }
>
[ ... ]
> 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`.
[Severity: High]
Does this Adapter contain an unsound conversion of potentially aliased
C pointers?
In blink_set_callback(), the function unconditionally converts delay_on and
delay_off pointers into mutable references:
rust/kernel/led/normal.rs:blink_set_callback()
unsafe { &mut *delay_on },
...
unsafe { &mut *delay_off },
The C function led_blink_set() and its callback signature accept these
pointers without the restrict keyword. This means C callers are legally
permitted to pass the same pointer for both arguments.
Could a C caller aliasing these pointers create overlapping mutable
references and trigger undefined behavior?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-rust_leds-v23-0-2716a4ef89a4@posteo.de?part=2
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RESEND v23 1/3] rust: leds: add basic led classdev abstractions
2026-08-19 18:58 ` [PATCH RESEND v23 1/3] rust: leds: add basic " Markus Probst
@ 2026-08-19 19:14 ` sashiko-bot
0 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-08-19 19:14 UTC (permalink / raw)
To: Markus Probst; +Cc: linux-leds, linux-pci, lee, ojeda
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `blink_set_callback` creates aliased mutable references if `delay_on` and `delay_off` point to the same memory location.
- [High] The lifetime bound `'bound: 'init` on `DeviceBuilder::build` is inverted, allowing the Device to outlive its borrowed C strings.
- [Medium] The commit message description is not capitalized after the subsystem prefix.
- [Low] Small functions in abstractions must be annotated with `#[inline]`.
--
--- Patch [1]: [PATCH RESEND v23 1/3] rust: leds: add basic led classdev abstractions ---
commit 0bee73d85d7c6052f28b0588d95b4632e4bdc662
Author: Markus Probst <markus.probst@posteo.de>
rust: leds: add basic led classdev abstractions
Implement the core abstractions needed for led class devices, including
the Device builder and LedOps trait.
> rust: leds: add basic led classdev abstractions
[Severity: Medium]
This isn't a bug, but the subsystem guidelines request capitalizing the
description after the subsystem prefix. Should this be "rust: leds: Add
basic led classdev abstractions" instead?
> diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
> index 596975e103b8b.....
> --- /dev/null
> +++ b/rust/kernel/led.rs
[ ... ]
> +impl TryFrom<u32> for Color {
> + type Error = Error;
> +
> + fn try_from(value: u32) -> core::result::Result<Self, Self::Error> {
[Severity: Low]
This isn't a bug, but the Rust subsystem guidelines specify that small
functions in abstractions should be annotated with #[inline]. Could we add
that attribute to this try_from() implementation?
> diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> index 816db7c576894.....
> --- /dev/null
> +++ b/rust/kernel/led/normal.rs
[ ... ]
> +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 {
[Severity: High]
Is the lifetime bound 'bound: 'init inverted here?
DeviceBuilder captures string references with the 'init lifetime. If the
returned Device has a 'bound lifetime that outlives 'init, a caller could
create a Device that escapes the scope of the borrowed strings.
Since the underlying C led_classdev stores raw pointers to these strings,
could this lead to a use-after-free when accessed via sysfs if the strings
are dropped?
[ ... ]
> + unsafe extern "C" fn blink_set_callback(
> + led_cdev: *mut bindings::led_classdev,
> + delay_on: *mut usize,
> + delay_off: *mut usize,
> + ) -> i32 {
[ ... ]
> + 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]
Does this create aliased mutable references if the C subsystem passes the
same pointer for delay_on and delay_off?
The C LED subsystem explicitly permits and frequently passes the exact same
pointer for both arguments. For example, led_blink_set_oneshot() in
ledtrig-netdev.c passes identical pointers.
If both pointers are identical, converting them concurrently into two
&mut usize references triggers undefined behavior in Rust. Could we handle
this aliasing safely?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260819-rust_leds-v23-0-2716a4ef89a4@posteo.de?part=1
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-08-19 19:14 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-19 18:58 [PATCH RESEND v23 0/3] rust: leds: add led classdev abstractions Markus Probst
2026-08-19 18:58 ` [PATCH RESEND v23 1/3] rust: leds: add basic " Markus Probst
2026-08-19 19:14 ` sashiko-bot
2026-08-19 18:58 ` [PATCH RESEND v23 2/3] rust: leds: add Mode trait Markus Probst
2026-08-19 19:11 ` sashiko-bot
2026-08-19 18:58 ` [PATCH RESEND v23 3/3] rust: leds: add multicolor classdev abstractions Markus Probst
2026-08-19 19:10 ` sashiko-bot
-- strict thread matches above, loose matches on Subject: below --
2026-08-05 19:34 [PATCH RESEND v23 0/3] rust: leds: add led " Markus Probst via B4 Relay
2026-08-05 19:34 ` [PATCH RESEND v23 3/3] rust: leds: add multicolor " Markus Probst via B4 Relay
2026-08-05 20:52 ` sashiko-bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox