* [PATCH v21 1/3] rust: leds: add basic led classdev abstractions
2026-06-29 13:10 [PATCH v21 0/3] rust: leds: add led classdev abstractions Markus Probst
@ 2026-06-29 13:10 ` Markus Probst
2026-06-29 13:36 ` sashiko-bot
2026-06-29 13:10 ` [PATCH v21 2/3] rust: leds: add Mode trait Markus Probst
2026-06-29 13:10 ` [PATCH v21 3/3] rust: leds: add multicolor classdev abstractions Markus Probst
2 siblings, 1 reply; 7+ messages in thread
From: Markus Probst @ 2026-06-29 13:10 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 15011f5752a9..ceb2285366ff 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..c92d99d68497
--- /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<'a, S> {
+ fwnode: Option<ARef<FwNode>>,
+ name: Option<&'a CStr>,
+ devicename: Option<&'a 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<'a> DeviceBuilder<'a, 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: &'a 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<'a, S: DeviceBuilderState> DeviceBuilder<'a, 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<'a> DeviceBuilder<'a, 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: &'a 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..2769f690bb24
--- /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<'a, S: DeviceBuilderState> DeviceBuilder<'a, S> {
+ /// Registers a new [`Device`].
+ pub fn build<'bound: 'a, T: LedOps + 'bound>(
+ self,
+ parent: &'bound T::Bus,
+ ops: impl PinInit<T, Error> + 'a,
+ ) -> impl PinInit<Device<'bound, T>, Error> + 'a {
+ 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.53.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v21 1/3] rust: leds: add basic led classdev abstractions
2026-06-29 13:10 ` [PATCH v21 1/3] rust: leds: add basic " Markus Probst
@ 2026-06-29 13:36 ` sashiko-bot
0 siblings, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-06-29 13:36 UTC (permalink / raw)
To: Markus Probst; +Cc: linux-pci, ojeda, linux-leds, lee
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `blink_set_callback` creates multiple mutable references (`&mut usize`) to the same shared memory concurrently, violating Rust's strict aliasing rules.
- [High] The `DeviceBuilder::build` method erases the lifetime of the `name` string, allowing the `led_classdev.name` pointer to outlive the string and causing a Use-After-Free.
- [High] Unsound `FwNode` reference count management in `Device::drop` retrieves the `fwnode` directly from the mutable C structure, leading to refcount underflow and Use-After-Free.
--
commit d59e3e50bab36d2a8936041aa09ed5823876969f
Author: Markus Probst <markus.probst@posteo.de>
rust: leds: add basic led classdev abstractions
Implement the core abstractions needed for led class devices, including
LedOps, DeviceBuilder, and Device.
> diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
> new file mode 100644
> index 0000000000000..2769f690bb245
> --- /dev/null
> +++ b/rust/kernel/led/normal.rs
[ ... ]
> +impl<'a, S: DeviceBuilderState> DeviceBuilder<'a, S> {
> + /// Registers a new [`Device`].
> + pub fn build<'bound: 'a, T: LedOps + 'bound>(
> + self,
> + parent: &'bound T::Bus,
> + ops: impl PinInit<T, Error> + 'a,
> + ) -> impl PinInit<Device<'bound, T>, Error> + 'a {
[Severity: High]
Is the lifetime bound <'bound: 'a> backwards here?
By specifying 'bound: 'a, the returned Device ('bound) is explicitly permitted
to outlive the builder's string lifetime ('a).
Because the raw pointer to this string is written directly into the
led_classdev.name field, a driver could initialize a Device with a temporary
string, which drops the string while leaving led_cdev->name dangling. Can
this lead to a use-after-free?
[ ... ]
> + 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]
Could this lead to a strict aliasing violation by creating multiple mutable
references to the same shared memory concurrently?
In the C LED subsystem, sysfs write operations to delay_on and delay_off can
invoke led_blink_set() concurrently across different threads without holding
locks on the device.
This passes pointers to the shared led_cdev->blink_delay_on and
led_cdev->blink_delay_off fields directly into the blink_set callback. By
unconditionally casting these raw C pointers to &mut usize in Rust, the
callback can create overlapping mutable references to the exact same memory
locations.
[ ... ]
> +#[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]
Does this unsoundly steal a reference count from the C field dev->fwnode?
In DeviceBuilder::build, self.fwnode is passed to C and its refcount is
leaked via core::mem::forget. However, if self.fwnode was initially None, no
reference count is leaked.
If dev->fwnode is subsequently populated by external C code, this
PinnedDrop implementation blindly constructs an ARef and drops it. Since
Rust never incremented this refcount, could this result in an underflow
and an eventual use-after-free of the FwNode?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-rust_leds-v21-0-4f0f19579db5@posteo.de?part=1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v21 2/3] rust: leds: add Mode trait
2026-06-29 13:10 [PATCH v21 0/3] rust: leds: add led classdev abstractions Markus Probst
2026-06-29 13:10 ` [PATCH v21 1/3] rust: leds: add basic " Markus Probst
@ 2026-06-29 13:10 ` Markus Probst
2026-06-29 13:40 ` sashiko-bot
2026-06-29 13:10 ` [PATCH v21 3/3] rust: leds: add multicolor classdev abstractions Markus Probst
2 siblings, 1 reply; 7+ messages in thread
From: Markus Probst @ 2026-06-29 13:10 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 c92d99d68497..6ee337008db7 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: &'a 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 2769f690bb24..9fdef9b18918 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<'a, S: DeviceBuilderState> DeviceBuilder<'a, S> {
/// Registers a new [`Device`].
- pub fn build<'bound: 'a, T: LedOps + 'bound>(
+ pub fn build<'bound: 'a, T: LedOps<Mode = Normal> + 'bound>(
self,
parent: &'bound T::Bus,
ops: impl PinInit<T, Error> + 'a,
@@ -87,7 +95,7 @@ pub fn build<'bound: 'a, 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.53.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v21 3/3] rust: leds: add multicolor classdev abstractions
2026-06-29 13:10 [PATCH v21 0/3] rust: leds: add led classdev abstractions Markus Probst
2026-06-29 13:10 ` [PATCH v21 1/3] rust: leds: add basic " Markus Probst
2026-06-29 13:10 ` [PATCH v21 2/3] rust: leds: add Mode trait Markus Probst
@ 2026-06-29 13:10 ` Markus Probst
2026-06-29 13:55 ` sashiko-bot
2 siblings, 1 reply; 7+ messages in thread
From: Markus Probst @ 2026-06-29 13:10 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 6ee337008db7..4540c4e5c9d7 100644
--- a/rust/kernel/led.rs
+++ b/rust/kernel/led.rs
@@ -30,8 +30,16 @@
types::Opaque, //
};
+#[cfg(CONFIG_LEDS_CLASS_MULTICOLOR)]
+mod multicolor;
mod normal;
+#[cfg(CONFIG_LEDS_CLASS_MULTICOLOR)]
+pub use multicolor::{
+ MultiColor,
+ MultiColorDevice,
+ MultiColorSubLed, //
+};
pub use normal::{
Device,
Normal, //
@@ -255,7 +263,24 @@ pub enum Color {
Violet = bindings::LED_COLOR_ID_VIOLET,
Yellow = bindings::LED_COLOR_ID_YELLOW,
Ir = bindings::LED_COLOR_ID_IR,
+ #[cfg_attr(
+ CONFIG_LEDS_CLASS_MULTICOLOR,
+ doc = "Use this color for a [`MultiColor`] led."
+ )]
+ #[cfg_attr(
+ not(CONFIG_LEDS_CLASS_MULTICOLOR),
+ doc = "Use this color for a `MultiColor` led."
+ )]
+ /// If the led supports RGB, use [`Color::Rgb`] instead.
Multi = bindings::LED_COLOR_ID_MULTI,
+ #[cfg_attr(
+ CONFIG_LEDS_CLASS_MULTICOLOR,
+ doc = "Use this color for a [`MultiColor`] led with rgb support."
+ )]
+ #[cfg_attr(
+ not(CONFIG_LEDS_CLASS_MULTICOLOR),
+ doc = "Use this color for a `MultiColor` led with rgb support."
+ )]
Rgb = bindings::LED_COLOR_ID_RGB,
Purple = bindings::LED_COLOR_ID_PURPLE,
Orange = bindings::LED_COLOR_ID_ORANGE,
@@ -296,7 +321,14 @@ fn try_from(value: u32) -> core::result::Result<Self, Self::Error> {
///
/// Each led mode has its own led class device type with different capabilities.
///
-/// See [`Normal`].
+#[cfg_attr(
+ CONFIG_LEDS_CLASS_MULTICOLOR,
+ doc = "See [`Normal`] and [`MultiColor`]."
+)]
+#[cfg_attr(
+ not(CONFIG_LEDS_CLASS_MULTICOLOR),
+ doc = "See [`Normal`] and `MultiColor`."
+)]
pub trait Mode: private::Sealed {
/// The class device for the led mode.
type Device<'bound, T: LedOps<Mode = Self> + 'bound>;
diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
new file mode 100644
index 000000000000..2c55140dd37e
--- /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<'a, S: DeviceBuilderState> DeviceBuilder<'a, S> {
+ /// Registers a new [`MulticolorDevice`].
+ pub fn build_multicolor<'bound: 'a, T: LedOps<Mode = MultiColor> + 'bound>(
+ self,
+ parent: &'bound T::Bus,
+ ops: impl PinInit<T, Error> + 'a,
+ subleds: &'a [MultiColorSubLed],
+ ) -> impl PinInit<MultiColorDevice<'bound, T>, Error> + 'a {
+ const_assert!(T::MAX_BRIGHTNESS <= i32::MAX.unsigned_abs() || !T::HAS_BRIGHTNESS_GET);
+
+ try_pin_init!(MultiColorDevice {
+ ops <- ops,
+ classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev_mc| {
+ 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.53.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v21 3/3] rust: leds: add multicolor classdev abstractions
2026-06-29 13:10 ` [PATCH v21 3/3] rust: leds: add multicolor classdev abstractions Markus Probst
@ 2026-06-29 13:55 ` sashiko-bot
0 siblings, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-06-29 13:55 UTC (permalink / raw)
To: Markus Probst; +Cc: linux-pci, linux-leds, lee, ojeda
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unsynchronized reads of concurrently modified `intensity` and `brightness` fields in `MultiColorSubLed` cause a data race and Undefined Behavior in safe Rust.
- [High] Flawed `fwnode` reference counting ownership in `MultiColorDevice` leads to potential Use-After-Free or memory leak.
--
commit 53cb84fc2ecc8ded772da6376e83ccc60f89c50f
Author: Markus Probst <markus.probst@posteo.de>
rust: leds: add multicolor classdev abstractions
Implement the abstractions needed for multicolor led class devices.
> diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
> new file mode 100644
> index 0000000000000..2c55140dd37ed
> --- /dev/null
> +++ b/rust/kernel/led/multicolor.rs
> @@ -0,0 +1,460 @@
[ ... ]
> + /// 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() }
[Severity: High]
Does this unsynchronized raw pointer read risk causing a data race and
Undefined Behavior in safe Rust?
The C multicolor LED core modifies subled_info[i].intensity and brightness
concurrently when a user writes to sysfs attributes. Meanwhile, this safe Rust
getter method performs an unsynchronized raw pointer read. Since
MultiColorDevice implements Sync, multiple threads can share
MultiColorSubLed and read these fields at any time.
Should this use core::ptr::read_volatile to ensure soundness?
> + }
> +
> + /// 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]
Similarly, does this unsynchronized read of brightness risk causing a data
race when the C side modifies it concurrently?
Should core::ptr::read_volatile be used here as well?
[ ... ]
> + 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
[Severity: High]
Does leaking the fwnode reference count to the C subsystem and then trying
to reclaim it in drop() by reading dev.fwnode() risk a Use-After-Free?
If the kernel (e.g. ACPI) assigns a different fwnode to the device after
registration, dev.fwnode() will return this new fwnode. The drop()
implementation below would then incorrectly take ownership of the new
fwnode's reference count and drop it, while permanently leaking the
originally passed fwnode.
Should MultiColorDevice store Option<ARef<FwNode>> locally to ensure it
only manages the exact reference it uniquely owns?
> +
> + Ok::<_, Error>(())
> + }),
> + _p: PhantomData,
> + })
[ ... ]
> +#[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]
Is it safe to unconditionally reconstruct an ARef from dev.fwnode() here?
As mentioned above, if dev.fwnode() returns a different fwnode than the one
originally passed to build_multicolor, this will drop a reference it does not
own, potentially leading to a Use-After-Free, while the originally leaked
reference remains un-dropped.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-rust_leds-v21-0-4f0f19579db5@posteo.de?part=3
^ permalink raw reply [flat|nested] 7+ messages in thread