* [PATCH v9 1/3] rust: leds: add basic led classdev abstractions
2025-11-19 14:11 [PATCH v9 0/3] rust: leds: add led classdev abstractions Markus Probst
@ 2025-11-19 14:11 ` Markus Probst
2025-11-20 11:34 ` Alice Ryhl
2025-11-19 14:11 ` [PATCH v9 2/3] rust: leds: split generic and normal led classdev abstractions up Markus Probst
2025-11-19 14:11 ` [PATCH v9 3/3] rust: leds: add multicolor classdev abstractions Markus Probst
2 siblings, 1 reply; 8+ messages in thread
From: Markus Probst @ 2025-11-19 14:11 UTC (permalink / raw)
To: Lee Jones, Pavel Machek, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Danilo Krummrich, Rafael J. Wysocki,
Bjorn Helgaas, Krzysztof Wilczyński
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::InitData` - data set for the led class device
* `led::Device` - a safe wrapper around `led_classdev`
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
MAINTAINERS | 7 +
rust/kernel/led.rs | 472 +++++++++++++++++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +
3 files changed, 480 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index b71ea515240a..80cb030911b7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14112,6 +14112,13 @@ 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
+
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..fca55f02be8d
--- /dev/null
+++ b/rust/kernel/led.rs
@@ -0,0 +1,472 @@
+// 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,
+ pin::Pin,
+ ptr::NonNull, //
+};
+
+use pin_init::{
+ pin_data,
+ pinned_drop,
+ PinInit, //
+};
+
+use crate::{
+ build_error,
+ container_of,
+ device::{
+ self,
+ property::FwNode,
+ AsBusDevice,
+ Bound, //
+ },
+ devres::Devres,
+ error::{
+ code::EINVAL,
+ from_result,
+ to_result,
+ Error,
+ Result,
+ VTABLE_DEFAULT_ERROR, //
+ },
+ macros::vtable,
+ str::CStr,
+ try_pin_init,
+ types::{
+ ARef,
+ Opaque, //
+ }, //
+};
+
+/// The led class device representation.
+///
+/// This structure represents the Rust abstraction for a C `struct led_classdev`.
+#[pin_data(PinnedDrop)]
+pub struct Device<T: LedOps> {
+ ops: T,
+ #[pin]
+ classdev: Opaque<bindings::led_classdev>,
+}
+
+/// The led init data representation.
+///
+/// This structure represents the Rust abstraction for a C `struct led_init_data` with additional
+/// fields from `struct led_classdev`.
+#[derive(Default)]
+pub struct InitData<'a> {
+ fwnode: Option<ARef<FwNode>>,
+ default_label: Option<&'a CStr>,
+ devicename: Option<&'a CStr>,
+ devname_mandatory: bool,
+ initial_brightness: u32,
+ default_trigger: Option<&'a CStr>,
+ color: Color,
+}
+
+impl InitData<'static> {
+ /// Creates a new [`InitData`]
+ pub fn new() -> Self {
+ Self::default()
+ }
+}
+
+impl<'a> InitData<'a> {
+ /// Sets the firmware node
+ pub fn fwnode(self, fwnode: Option<ARef<FwNode>>) -> Self {
+ Self { fwnode, ..self }
+ }
+
+ /// Sets a default label
+ pub fn default_label(self, label: &'a CStr) -> Self {
+ Self {
+ default_label: Some(label),
+ ..self
+ }
+ }
+
+ /// Sets the device name
+ pub fn devicename(self, devicename: &'a CStr) -> Self {
+ Self {
+ devicename: Some(devicename),
+ ..self
+ }
+ }
+
+ /// Sets if a device name is mandatory
+ pub fn devicename_mandatory(self, mandatory: bool) -> Self {
+ Self {
+ devname_mandatory: mandatory,
+ ..self
+ }
+ }
+
+ /// Sets the initial brightness value for the led
+ ///
+ /// The default brightness is 0.
+ /// If [`LedOps::brightness_get`] is implemented, this value will be ignored.
+ pub fn initial_brightness(self, brightness: u32) -> Self {
+ Self {
+ initial_brightness: brightness,
+ ..self
+ }
+ }
+
+ /// Set the default led trigger
+ ///
+ /// This value can be overwritten by the "linux,default-trigger" fwnode property.
+ pub fn default_trigger(self, trigger: &'a CStr) -> Self {
+ Self {
+ default_trigger: Some(trigger),
+ ..self
+ }
+ }
+
+ /// Sets the color of the led
+ ///
+ /// This value can be overwritten by the "color" fwnode property.
+ pub fn color(self, color: Color) -> Self {
+ Self { color, ..self }
+ }
+}
+
+/// Trait defining the operations for a LED driver.
+///
+/// # Examples
+///
+///```
+/// # use kernel::{
+/// # c_str, device, devres::Devres,
+/// # error::Result, led,
+/// # macros::vtable, platform, prelude::*,
+/// # };
+/// # use core::pin::Pin;
+///
+/// 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(
+/// &self,
+/// _dev: &platform::Device<device::Bound>,
+/// _classdev: &led::Device<Self>,
+/// _brightness: u32
+/// ) -> Result<()> {
+/// // Set the brightness for the led here
+/// Ok(())
+/// }
+/// }
+///
+/// fn register_my_led(
+/// parent: &platform::Device<device::Bound>,
+/// ) -> Result<Pin<KBox<Devres<led::Device<MyLedOps>>>>> {
+/// KBox::pin_init(led::Device::new(
+/// parent,
+/// led::InitData::new()
+/// .default_label(c_str!("my_led")),
+/// MyLedOps,
+/// ), GFP_KERNEL)
+/// }
+///```
+/// Led drivers must implement this trait in order to register and handle a [`Device`].
+#[vtable]
+pub trait LedOps: Send + 'static + 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(
+ &self,
+ dev: &Self::Bus,
+ classdev: &Device<Self>,
+ brightness: u32,
+ ) -> Result<()>;
+
+ /// Gets the current brightness level.
+ fn brightness_get(&self, _dev: &Self::Bus, _classdev: &Device<Self>) -> u32 {
+ 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(
+ &self,
+ _dev: &Self::Bus,
+ _classdev: &Device<Self>,
+ _delay_on: &mut usize,
+ _delay_off: &mut usize,
+ ) -> Result<()> {
+ build_error!(VTABLE_DEFAULT_ERROR)
+ }
+}
+
+/// Led colors.
+#[derive(Copy, Clone, Debug, Default)]
+#[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,
+}
+
+impl TryFrom<u32> for Color {
+ type Error = Error;
+
+ fn try_from(value: u32) -> core::result::Result<Self, Self::Error> {
+ const _: () = {
+ assert!(bindings::LED_COLOR_ID_MAX == 15);
+ };
+ if value < bindings::LED_COLOR_ID_MAX {
+ // SAFETY:
+ // - `Color` is represented as `u32`
+ // - the const block 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)
+ }
+ }
+}
+
+// SAFETY: A `led::Device` can be unregistered from any thread.
+unsafe impl<T: LedOps + Send> Send for Device<T> {}
+
+// SAFETY: `led::Device` can be shared among threads because all methods of `led::Device`
+// are thread safe.
+unsafe impl<T: LedOps + Sync> Sync for Device<T> {}
+
+impl<T: LedOps> Device<T> {
+ /// Registers a new led classdev.
+ ///
+ /// The [`Device`] will be unregistered on drop.
+ pub fn new<'a>(
+ parent: &'a T::Bus,
+ init_data: InitData<'a>,
+ ops: T,
+ ) -> impl PinInit<Devres<Self>, Error> + 'a {
+ Devres::new(
+ parent.as_ref(),
+ try_pin_init!(Self {
+ 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: init_data.initial_brightness,
+ default_trigger: init_data.default_trigger
+ .map_or(core::ptr::null(), CStr::as_char_ptr),
+ color: init_data.color as u32,
+ ..bindings::led_classdev::default()
+ })
+ };
+
+ let mut init_data_raw = bindings::led_init_data {
+ fwnode: init_data.fwnode
+ .as_ref()
+ .map_or(core::ptr::null_mut(), |fwnode| fwnode.as_raw()),
+ default_label: init_data
+ .default_label
+ .map_or(core::ptr::null(), CStr::as_char_ptr),
+ devicename: init_data
+ .devicename
+ .map_or(core::ptr::null(), CStr::as_char_ptr),
+ devname_mandatory: init_data.devname_mandatory,
+ };
+
+ // SAFETY:
+ // - `parent.as_raw()` is guaranteed to be a pointer to a valid `device`
+ // or a null pointer.
+ // - `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,
+ &mut init_data_raw,
+ )
+ })?;
+
+ core::mem::forget(init_data.fwnode); // keep the reference count incremented
+
+ Ok::<_, Error>(())
+ }),
+ }),
+ )
+ }
+
+ /// # Safety
+ /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a
+ /// `led::Device`.
+ 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) }
+ }
+
+ fn parent(&self) -> &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) }
+ }
+}
+
+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()) };
+
+ classdev.ops.brightness_get(parent, classdev)
+ }
+
+ /// # 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<T: LedOps> PinnedDrop for Device<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 `struct 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::Device::new`.
+ .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(self.classdev.get()) };
+ }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 8c0070a8029e..a8f49ce78f8d 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -105,6 +105,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.51.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v9 1/3] rust: leds: add basic led classdev abstractions
2025-11-19 14:11 ` [PATCH v9 1/3] rust: leds: add basic " Markus Probst
@ 2025-11-20 11:34 ` Alice Ryhl
2025-11-20 13:00 ` Markus Probst
0 siblings, 1 reply; 8+ messages in thread
From: Alice Ryhl @ 2025-11-20 11:34 UTC (permalink / raw)
To: Markus Probst
Cc: Lee Jones, Pavel Machek, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Rafael J. Wysocki, Bjorn Helgaas,
Krzysztof Wilczyński, rust-for-linux, linux-leds,
linux-kernel, linux-pci
On Wed, Nov 19, 2025 at 02:11:21PM +0000, Markus Probst wrote:
> Implement the core abstractions needed for led class devices, including:
>
> * `led::LedOps` - the trait for handling leds, including
> `brightness_set`, `brightness_get` and `blink_set`
>
> * `led::InitData` - data set for the led class device
>
> * `led::Device` - a safe wrapper around `led_classdev`
>
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
> MAINTAINERS | 7 +
> rust/kernel/led.rs | 472 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 1 +
> 3 files changed, 480 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b71ea515240a..80cb030911b7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14112,6 +14112,13 @@ 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
> +
> 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..fca55f02be8d
> --- /dev/null
> +++ b/rust/kernel/led.rs
> @@ -0,0 +1,472 @@
> +// 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,
> + pin::Pin,
> + ptr::NonNull, //
> +};
> +
> +use pin_init::{
> + pin_data,
> + pinned_drop,
> + PinInit, //
> +};
> +
> +use crate::{
> + build_error,
> + container_of,
> + device::{
> + self,
> + property::FwNode,
> + AsBusDevice,
> + Bound, //
> + },
> + devres::Devres,
> + error::{
> + code::EINVAL,
> + from_result,
> + to_result,
> + Error,
> + Result,
> + VTABLE_DEFAULT_ERROR, //
> + },
> + macros::vtable,
> + str::CStr,
> + try_pin_init,
> + types::{
> + ARef,
> + Opaque, //
> + }, //
> +};
Please import kernel::prelude::* and remove all the imports that are
available from the prelude.
> +impl<'a> InitData<'a> {
> + /// Sets the firmware node
> + pub fn fwnode(self, fwnode: Option<ARef<FwNode>>) -> Self {
I'm thinking that perhaps this should just be a `&'a FwNode` instead?
That way, you can increment the refcount in Device::new() if
registration is successful.
> + Self { fwnode, ..self }
> + }
> +
> + /// Sets a default label
There are many missing periods in doc-comments.
> +/// Trait defining the operations for a LED driver.
> +///
> +/// # Examples
> +///
> +///```
> +/// # use kernel::{
> +/// # c_str, device, devres::Devres,
> +/// # error::Result, led,
> +/// # macros::vtable, platform, prelude::*,
> +/// # };
> +/// # use core::pin::Pin;
> +///
> +/// struct MyLedOps;
When using # in examples, please do not have an empty line before
beginning the example. It shows up as a weird extra empty line in the
rendered docs.
You could consider just making the imports displayed here also.
Also the ``` both above and below the example usually has a space:
/// ```
rather than
///```
> + // SAFETY:
> + // - `parent.as_raw()` is guaranteed to be a pointer to a valid `device`
> + // or a null pointer.
> + // - `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,
> + &mut init_data_raw,
> + )
> + })?;
> +
> + core::mem::forget(init_data.fwnode); // keep the reference count incremented
This led abstraction implicitly takes a refcount on the fwnode and then
drops it when the device is unbound.
Lee, can you confirm that taking a refcount on the fwnode is the right
way to use the LED subsytem?
Alice
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v9 1/3] rust: leds: add basic led classdev abstractions
2025-11-20 11:34 ` Alice Ryhl
@ 2025-11-20 13:00 ` Markus Probst
0 siblings, 0 replies; 8+ messages in thread
From: Markus Probst @ 2025-11-20 13:00 UTC (permalink / raw)
To: Alice Ryhl
Cc: Lee Jones, Pavel Machek, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Rafael J. Wysocki, Bjorn Helgaas,
Krzysztof Wilczyński, rust-for-linux, linux-leds,
linux-kernel, linux-pci
On Thu, 2025-11-20 at 11:34 +0000, Alice Ryhl wrote:
> On Wed, Nov 19, 2025 at 02:11:21PM +0000, Markus Probst wrote:
> > Implement the core abstractions needed for led class devices, including:
> >
> > * `led::LedOps` - the trait for handling leds, including
> > `brightness_set`, `brightness_get` and `blink_set`
> >
> > * `led::InitData` - data set for the led class device
> >
> > * `led::Device` - a safe wrapper around `led_classdev`
> >
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> > MAINTAINERS | 7 +
> > rust/kernel/led.rs | 472 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > rust/kernel/lib.rs | 1 +
> > 3 files changed, 480 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index b71ea515240a..80cb030911b7 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -14112,6 +14112,13 @@ 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
> > +
> > 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..fca55f02be8d
> > --- /dev/null
> > +++ b/rust/kernel/led.rs
> > @@ -0,0 +1,472 @@
> > +// 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,
> > + pin::Pin,
> > + ptr::NonNull, //
> > +};
> > +
> > +use pin_init::{
> > + pin_data,
> > + pinned_drop,
> > + PinInit, //
> > +};
> > +
> > +use crate::{
> > + build_error,
> > + container_of,
> > + device::{
> > + self,
> > + property::FwNode,
> > + AsBusDevice,
> > + Bound, //
> > + },
> > + devres::Devres,
> > + error::{
> > + code::EINVAL,
> > + from_result,
> > + to_result,
> > + Error,
> > + Result,
> > + VTABLE_DEFAULT_ERROR, //
> > + },
> > + macros::vtable,
> > + str::CStr,
> > + try_pin_init,
> > + types::{
> > + ARef,
> > + Opaque, //
> > + }, //
> > +};
>
> Please import kernel::prelude::* and remove all the imports that are
> available from the prelude.
>
> > +impl<'a> InitData<'a> {
> > + /// Sets the firmware node
> > + pub fn fwnode(self, fwnode: Option<ARef<FwNode>>) -> Self {
>
> I'm thinking that perhaps this should just be a `&'a FwNode` instead?
> That way, you can increment the refcount in Device::new() if
> registration is successful.
This was the way I have done it in v8. I issue with this approch is, if
the fwnode is optional, you have to do this ugly code:
let mut init_data = InitData::new()
.default_label(c_str!(":label"))
.devicename(c_str!("devicename"));
let child_fwnode = fwnode.child_by_name(c_str!("led"));
if let Some(child_fwnode_ref) = &child_fwnode {
init_data = init_data.fwnode(child_fwnode_ref);
}
Instead of
let mut init_data = InitData::new()
.fwnode(fwnode.child_by_name(c_str!("led")))
.default_label(c_str!(":label"))
.devicename(c_str!("devicename"));
Furthermore, most of the functions in the rust abstractions return a
`ARef<FwNode>` anyway. The only exception I found is
`device::Device::fwnode()`, but an led driver usually has more than one
led to its unlikely that the root fwnode will be directly passed to an
led classdev. As a result with the `&'a FwNode` approach, the led
abstraction would increment the reference count once, and the driver
will then decrement it once (dropping of the ARef<FwNode>).
Sounds like a tiny overhead to me.
>
> > + Self { fwnode, ..self }
> > + }
> > +
> > + /// Sets a default label
>
> There are many missing periods in doc-comments.
>
> > +/// Trait defining the operations for a LED driver.
> > +///
> > +/// # Examples
> > +///
> > +///```
> > +/// # use kernel::{
> > +/// # c_str, device, devres::Devres,
> > +/// # error::Result, led,
> > +/// # macros::vtable, platform, prelude::*,
> > +/// # };
> > +/// # use core::pin::Pin;
> > +///
> > +/// struct MyLedOps;
>
> When using # in examples, please do not have an empty line before
> beginning the example. It shows up as a weird extra empty line in the
> rendered docs.
>
> You could consider just making the imports displayed here also.
>
> Also the ``` both above and below the example usually has a space:
>
> /// ```
>
> rather than
>
> ///```
>
> > + // SAFETY:
> > + // - `parent.as_raw()` is guaranteed to be a pointer to a valid `device`
> > + // or a null pointer.
> > + // - `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,
> > + &mut init_data_raw,
> > + )
> > + })?;
> > +
> > + core::mem::forget(init_data.fwnode); // keep the reference count incremented
>
> This led abstraction implicitly takes a refcount on the fwnode and then
> drops it when the device is unbound.
I did look through the led classdev code and noticed that the fwnode
refcount isn't incremented. From what I can tell, the driver is
responsible to ensure the fwnode refcount never hits 0 while the led
classdev is registered. Thats why I incremented the refcount of the
fwnode to ensure the safety requirement is met.
Thanks
- Markus Probst
>
> Lee, can you confirm that taking a refcount on the fwnode is the right
> way to use the LED subsytem?
>
> Alice
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v9 2/3] rust: leds: split generic and normal led classdev abstractions up
2025-11-19 14:11 [PATCH v9 0/3] rust: leds: add led classdev abstractions Markus Probst
2025-11-19 14:11 ` [PATCH v9 1/3] rust: leds: add basic " Markus Probst
@ 2025-11-19 14:11 ` Markus Probst
2025-11-20 10:54 ` Alice Ryhl
2025-11-19 14:11 ` [PATCH v9 3/3] rust: leds: add multicolor classdev abstractions Markus Probst
2 siblings, 1 reply; 8+ messages in thread
From: Markus Probst @ 2025-11-19 14:11 UTC (permalink / raw)
To: Lee Jones, Pavel Machek, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Danilo Krummrich, Rafael J. Wysocki,
Bjorn Helgaas, Krzysztof Wilczyński
Cc: rust-for-linux, linux-leds, linux-kernel, linux-pci,
Markus Probst
Move code specific to normal led class devices into a separate file and
introduce the `led::Mode` trait to allow for other types of led class
devices.
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
MAINTAINERS | 1 +
rust/kernel/device.rs | 2 +-
rust/kernel/led.rs | 128 +++++++++++++++++++++++++++++++++++-----------
rust/kernel/led/normal.rs | 39 ++++++++++++++
4 files changed, 139 insertions(+), 31 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 80cb030911b7..ca11b9064e3f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14118,6 +14118,7 @@ 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>
diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs
index 660cb2b48c07..d330248ff2aa 100644
--- a/rust/kernel/device.rs
+++ b/rust/kernel/device.rs
@@ -333,7 +333,7 @@ pub(crate) fn as_raw(&self) -> *mut bindings::device {
}
/// Returns a reference to the parent device, if any.
- #[cfg_attr(not(CONFIG_AUXILIARY_BUS), expect(dead_code))]
+ #[cfg_attr(not(any(CONFIG_AUXILIARY_BUS, CONFIG_LEDS_CLASS)), expect(dead_code))]
pub(crate) fn parent(&self) -> Option<&Device> {
// SAFETY:
// - By the type invariant `self.as_raw()` is always valid.
diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
index fca55f02be8d..d51735322093 100644
--- a/rust/kernel/led.rs
+++ b/rust/kernel/led.rs
@@ -44,14 +44,18 @@
}, //
};
+mod normal;
+
+pub use normal::Normal;
+
/// The led class device representation.
///
-/// This structure represents the Rust abstraction for a C `struct led_classdev`.
+/// This structure represents the Rust abstraction for a led class device.
#[pin_data(PinnedDrop)]
pub struct Device<T: LedOps> {
ops: T,
#[pin]
- classdev: Opaque<bindings::led_classdev>,
+ classdev: Opaque<<T::Mode as private::Mode>::Type>,
}
/// The led init data representation.
@@ -153,6 +157,7 @@ pub fn color(self, color: Color) -> 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;
///
@@ -184,6 +189,12 @@ pub trait LedOps: Send + 'static + Sized {
/// The bus device required by the implementation.
#[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;
@@ -270,6 +281,42 @@ 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::Mode {}
+
+impl<T: private::Mode> Mode for T {}
+
+type RegisterFunc<T> =
+ unsafe extern "C" fn(*mut bindings::device, *mut T, *mut bindings::led_init_data) -> i32;
+
+type UnregisterFunc<T> = unsafe extern "C" fn(*mut T);
+
+mod private {
+ pub trait Mode {
+ type Type;
+ const REGISTER: super::RegisterFunc<Self::Type>;
+ const UNREGISTER: super::UnregisterFunc<Self::Type>;
+
+ /// # Safety
+ /// `raw` must be a valid pointer to [`Self::Type`].
+ unsafe fn device<'a>(raw: *mut Self::Type) -> &'a crate::device::Device;
+
+ /// # Safety
+ /// `led_cdev` must be a valid pointer to `led_classdev` embedded within [`Self::Type`].
+ unsafe fn from_classdev(led_cdev: *mut bindings::led_classdev) -> *mut Self::Type;
+
+ /// # Safety
+ /// `raw` must be a valid pointer to [`Self::Type`].
+ unsafe fn pre_brightness_set(_raw: *mut Self::Type, _brightness: u32) {}
+
+ fn release(_led_cdev: &mut Self::Type) {}
+ }
+}
+
// SAFETY: A `led::Device` can be unregistered from any thread.
unsafe impl<T: LedOps + Send> Send for Device<T> {}
@@ -278,24 +325,22 @@ unsafe impl<T: LedOps + Send> Send for Device<T> {}
unsafe impl<T: LedOps + Sync> Sync for Device<T> {}
impl<T: LedOps> Device<T> {
- /// Registers a new led classdev.
- ///
- /// The [`Device`] will be unregistered on drop.
- pub fn new<'a>(
+ fn __new<'a>(
parent: &'a T::Bus,
init_data: InitData<'a>,
ops: T,
+ func: impl FnOnce(bindings::led_classdev) -> Result<<T::Mode as private::Mode>::Type> + 'a,
) -> impl PinInit<Devres<Self>, Error> + 'a {
Devres::new(
parent.as_ref(),
try_pin_init!(Self {
ops,
- classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev| {
+ classdev <- Opaque::try_ffi_init(|ptr: *mut <T::Mode as private::Mode>::Type| {
// 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`.
+ // `T::Mode::Type` (and the embedded led_classdev) gets fully initialized
+ // in-place by `T::Mode::REGISTER` including `mutex` and `list_head`.
unsafe {
- ptr.write(bindings::led_classdev {
+ ptr.write((func)(bindings::led_classdev {
brightness_set: (!T::BLOCKING)
.then_some(Adapter::<T>::brightness_set_callback),
brightness_set_blocking: T::BLOCKING
@@ -309,7 +354,7 @@ pub fn new<'a>(
.map_or(core::ptr::null(), CStr::as_char_ptr),
color: init_data.color as u32,
..bindings::led_classdev::default()
- })
+ })?)
};
let mut init_data_raw = bindings::led_init_data {
@@ -326,11 +371,11 @@ pub fn new<'a>(
};
// SAFETY:
- // - `parent.as_raw()` is guaranteed to be a pointer to a valid `device`
- // or a null pointer.
- // - `ptr` is guaranteed to be a pointer to an initialized `led_classdev`.
+ // - `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 `T::Mode::Type`.
to_result(unsafe {
- bindings::led_classdev_register_ext(
+ (<T::Mode as private::Mode>::REGISTER)(
parent.as_ref().as_raw(),
ptr,
&mut init_data_raw,
@@ -350,15 +395,22 @@ pub fn new<'a>(
/// `led::Device`.
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) }
+ // embedded within a `led::Device` and thus is embedded within `T::Mode::Type`.
+ let raw = unsafe { <T::Mode as private::Mode>::from_classdev(led_cdev) };
+
+ // SAFETY: The function's contract guarantees that `raw` 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(raw), Self, classdev) }
}
fn parent(&self) -> &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: `self.classdev.get()` is guaranteed to be a valid pointer to `T::Mode::Type`.
+ let device = unsafe { <T::Mode as private::Mode>::device(self.classdev.get()) };
+ // SAFETY: `led::Device::__new` doesn't allow to register a class device without an parent.
+ let parent = unsafe { device.parent().unwrap_unchecked() };
+ // SAFETY: the existence of `self` guarantees that `parent` is bound to a driver.
+ unsafe { parent.as_bound() }
}
}
@@ -376,11 +428,17 @@ impl<T: LedOps> Adapter<T> {
brightness: u32,
) {
// SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a
- // `led_classdev` embedded within a `led::Device`.
+ // `T::Mode::Type` 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()) };
+ // SAFETY: `classdev.classdev.get()` is guaranteed to be a valid pointer to a
+ // `T::Mode::Type`.
+ unsafe {
+ <T::Mode as private::Mode>::pre_brightness_set(classdev.classdev.get(), brightness);
+ }
+
let _ = classdev.ops.brightness_set(parent, classdev, brightness);
}
@@ -394,11 +452,17 @@ impl<T: LedOps> Adapter<T> {
) -> 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`.
+ // `T::Mode::Type` 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()) };
+ // SAFETY: `classdev.classdev.get()` is guaranteed to be a valid pointer to a
+ // `T::Mode::Type`.
+ unsafe {
+ <T::Mode as private::Mode>::pre_brightness_set(classdev.classdev.get(), brightness);
+ }
+
classdev.ops.brightness_set(parent, classdev, brightness)?;
Ok(0)
})
@@ -410,7 +474,7 @@ impl<T: LedOps> Adapter<T> {
/// 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`.
+ // `T::Mode::Type` 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()) };
@@ -431,7 +495,7 @@ impl<T: LedOps> Adapter<T> {
) -> 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`.
+ // `T::Mode::Type` 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()) };
@@ -456,17 +520,21 @@ impl<T: LedOps> PinnedDrop for Device<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 `struct led_classdev`.
- let dev: &device::Device = unsafe { device::Device::from_raw((*raw).dev) };
+ // valid `T::Mode::Type`.
+ let dev: &device::Device = unsafe { <T::Mode as private::Mode>::device(raw) };
let _fwnode = dev
.fwnode()
// SAFETY: the reference count of `fwnode` has previously been
- // incremented in `led::Device::new`.
+ // incremented in `led::Device::__new`.
.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(self.classdev.get()) };
+ // successfully registered with `T::Mode::REGISTER`.
+ unsafe { (<T::Mode as private::Mode>::UNREGISTER)(raw) };
+
+ // SAFETY: The existence of `self` guarantees that `self.classdev.get()` is a pointer to a
+ // valid `T::Mode::Type`.
+ <T::Mode as private::Mode>::release(unsafe { &mut *raw });
}
}
diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs
new file mode 100644
index 000000000000..13feeb3256f3
--- /dev/null
+++ b/rust/kernel/led/normal.rs
@@ -0,0 +1,39 @@
+// 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 mode for the `struct led_classdev`. Leds with this mode can only have a fixed color.
+pub enum Normal {}
+
+impl private::Mode for Normal {
+ type Type = bindings::led_classdev;
+ const REGISTER: RegisterFunc<Self::Type> = bindings::led_classdev_register_ext;
+ const UNREGISTER: UnregisterFunc<Self::Type> = bindings::led_classdev_unregister;
+
+ unsafe fn device<'a>(raw: *mut Self::Type) -> &'a device::Device {
+ // SAFETY:
+ // - The function's contract guarantees that `raw` is a valid pointer to `led_classdev`.
+ unsafe { device::Device::from_raw((*raw).dev) }
+ }
+
+ unsafe fn from_classdev(led_cdev: *mut bindings::led_classdev) -> *mut Self::Type {
+ led_cdev
+ }
+}
+
+impl<T: LedOps<Mode = Normal>> Device<T> {
+ /// Registers a new led classdev.
+ ///
+ /// The [`Device`] will be unregistered on drop.
+ pub fn new<'a>(
+ parent: &'a T::Bus,
+ init_data: InitData<'a>,
+ ops: T,
+ ) -> impl PinInit<Devres<Self>, Error> + 'a {
+ Self::__new(parent, init_data, ops, Ok)
+ }
+}
--
2.51.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v9 2/3] rust: leds: split generic and normal led classdev abstractions up
2025-11-19 14:11 ` [PATCH v9 2/3] rust: leds: split generic and normal led classdev abstractions up Markus Probst
@ 2025-11-20 10:54 ` Alice Ryhl
2025-11-20 13:21 ` Markus Probst
0 siblings, 1 reply; 8+ messages in thread
From: Alice Ryhl @ 2025-11-20 10:54 UTC (permalink / raw)
To: Markus Probst
Cc: Lee Jones, Pavel Machek, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Rafael J. Wysocki, Bjorn Helgaas,
Krzysztof Wilczyński, rust-for-linux, linux-leds,
linux-kernel, linux-pci
On Wed, Nov 19, 2025 at 02:11:24PM +0000, Markus Probst wrote:
> Move code specific to normal led class devices into a separate file and
> introduce the `led::Mode` trait to allow for other types of led class
> devices.
>
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
So it seems like the goal of this trait is to support both normal led
and multicolor led under the same code. However, it seems the traits
involved with this are pretty complex.
My primary feedback here is: please consider if we can avoid these
complex traits. How much duplication would it really take to just have
two Device structs and two LedOps traits? I think a few pieces of
duplication would be far better than what this patch does.
In fact, I'm not sure you even need two LedOps traits if you did that;
it seems like you could even get away with reusing the trait for both
cases.
Alice
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v9 2/3] rust: leds: split generic and normal led classdev abstractions up
2025-11-20 10:54 ` Alice Ryhl
@ 2025-11-20 13:21 ` Markus Probst
0 siblings, 0 replies; 8+ messages in thread
From: Markus Probst @ 2025-11-20 13:21 UTC (permalink / raw)
To: Alice Ryhl
Cc: Lee Jones, Pavel Machek, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Danilo Krummrich, Rafael J. Wysocki, Bjorn Helgaas,
Krzysztof Wilczyński, rust-for-linux, linux-leds,
linux-kernel, linux-pci
On Thu, 2025-11-20 at 10:54 +0000, Alice Ryhl wrote:
> On Wed, Nov 19, 2025 at 02:11:24PM +0000, Markus Probst wrote:
> > Move code specific to normal led class devices into a separate file and
> > introduce the `led::Mode` trait to allow for other types of led class
> > devices.
> >
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
>
> So it seems like the goal of this trait is to support both normal led
> and multicolor led under the same code. However, it seems the traits
> involved with this are pretty complex.
>
> My primary feedback here is: please consider if we can avoid these
> complex traits. How much duplication would it really take to just have
> two Device structs and two LedOps traits? I think a few pieces of
> duplication would be far better than what this patch does.
I counted over 221 lines of code for each classdev type (not counting
LedOps). If we want to support `led_classdev_flash` later down the line
(if a Rust driver would need it), that would be over 663 lines of code.
It doesn't look too complex to me, but thats maybe because I wrote it?
>
> In fact, I'm not sure you even need two LedOps traits if you did that;
> it seems like you could even get away with reusing the trait for both
> cases.
Currently it only uses one LedOps trait. The implementation in the
multicolor needs access to `Device::subleds()`. `Device` gets passed as
a reference to every LedOps function. If there would be two `Device`
structs, I am not sure how we can preserve one LedOps trait. Possibly a
type declaration on `Mode`? (assuming we still want the `Mode` trait)
Thanks
- Markus Probst
>
> Alice
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v9 3/3] rust: leds: add multicolor classdev abstractions
2025-11-19 14:11 [PATCH v9 0/3] rust: leds: add led classdev abstractions Markus Probst
2025-11-19 14:11 ` [PATCH v9 1/3] rust: leds: add basic " Markus Probst
2025-11-19 14:11 ` [PATCH v9 2/3] rust: leds: split generic and normal led classdev abstractions up Markus Probst
@ 2025-11-19 14:11 ` Markus Probst
2 siblings, 0 replies; 8+ messages in thread
From: Markus Probst @ 2025-11-19 14:11 UTC (permalink / raw)
To: Lee Jones, Pavel Machek, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Miguel Ojeda, Alex Gaynor, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Danilo Krummrich, Rafael J. Wysocki,
Bjorn Helgaas, Krzysztof Wilczyński
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::Device::new_multicolor()` - the function to register a multicolor
led class device
* `led::Device::subleds()` - the function to access the brightness and
intensity of the individual sub leds
Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
rust/bindings/bindings_helper.h | 1 +
rust/kernel/led.rs | 13 ++-
rust/kernel/led/multicolor.rs | 195 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 208 insertions(+), 1 deletion(-)
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index f92abb578b56..8825b6df9c9e 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -62,6 +62,7 @@
#include <linux/ioport.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 d51735322093..7c1f6252605f 100644
--- a/rust/kernel/led.rs
+++ b/rust/kernel/led.rs
@@ -44,8 +44,12 @@
}, //
};
+#[cfg(CONFIG_LEDS_CLASS_MULTICOLOR)]
+mod multicolor;
mod normal;
+#[cfg(CONFIG_LEDS_CLASS_MULTICOLOR)]
+pub use multicolor::{MultiColor, MultiColorSubLed};
pub use normal::Normal;
/// The led class device representation.
@@ -285,7 +289,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::Mode {}
impl<T: private::Mode> Mode for T {}
diff --git a/rust/kernel/led/multicolor.rs b/rust/kernel/led/multicolor.rs
new file mode 100644
index 000000000000..db5935448bd7
--- /dev/null
+++ b/rust/kernel/led/multicolor.rs
@@ -0,0 +1,195 @@
+// 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 crate::{
+ alloc::KVec,
+ error::code::EINVAL,
+ prelude::*, //
+};
+
+use super::*;
+
+/// The led mode for the `struct led_classdev_mc`. Leds with this mode can have multiple colors.
+pub enum MultiColor {}
+
+/// The multicolor sub led info representation.
+///
+/// This structure represents the Rust abstraction for a C `struct mc_subled`.
+#[repr(C)]
+#[derive(Copy, Clone, Debug)]
+pub struct MultiColorSubLed {
+ /// the color of the sub led
+ pub color: Color,
+ /// the brightness of the sub led.
+ ///
+ /// The value will be automatically calculated.
+ /// See `MultiColor::pre_brightness_set`.
+ pub brightness: u32,
+ /// the intensity of the sub led.
+ pub intensity: u32,
+ /// arbitrary data for the driver to store.
+ pub channel: u32,
+ _p: PhantomData<()>, // Only allow creation with `MultiColorSubLed::new`.
+}
+
+// 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: 0,
+ intensity: 0,
+ channel: 0,
+ _p: PhantomData,
+ };
+ let c_zeroed = bindings::mc_subled {
+ color_index: 0,
+ brightness: 0,
+ intensity: 0,
+ channel: 0,
+ };
+
+ assert!(offset_of!(MultiColorSubLed, color) == offset_of!(bindings::mc_subled, color_index));
+ assert_same_type(&0u32, &c_zeroed.color_index);
+
+ assert!(
+ offset_of!(MultiColorSubLed, brightness) == offset_of!(bindings::mc_subled, brightness)
+ );
+ assert_same_type(&rust_zeroed.brightness, &c_zeroed.brightness);
+
+ assert!(offset_of!(MultiColorSubLed, intensity) == offset_of!(bindings::mc_subled, intensity));
+ assert_same_type(&rust_zeroed.intensity, &c_zeroed.intensity);
+
+ assert!(offset_of!(MultiColorSubLed, channel) == offset_of!(bindings::mc_subled, channel));
+ 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.
+ pub const fn new(color: Color) -> Self {
+ Self {
+ color,
+ brightness: 0,
+ intensity: 0,
+ channel: 0,
+ _p: PhantomData,
+ }
+ }
+
+ /// Set arbitrary data for the driver.
+ pub const fn channel(mut self, channel: u32) -> Self {
+ self.channel = channel;
+ self
+ }
+
+ /// Set the initial intensity of the subled.
+ pub const fn initial_intensity(mut self, intensity: u32) -> Self {
+ self.intensity = intensity;
+ self
+ }
+}
+
+impl private::Mode for MultiColor {
+ type Type = bindings::led_classdev_mc;
+ const REGISTER: RegisterFunc<Self::Type> = bindings::led_classdev_multicolor_register_ext;
+ const UNREGISTER: UnregisterFunc<Self::Type> = bindings::led_classdev_multicolor_unregister;
+
+ unsafe fn device<'a>(raw: *mut Self::Type) -> &'a device::Device {
+ // SAFETY:
+ // - The function's contract guarantees that `raw` is a valid pointer to `led_classdev`.
+ unsafe { device::Device::from_raw((*raw).led_cdev.dev) }
+ }
+
+ unsafe fn from_classdev(led_cdev: *mut bindings::led_classdev) -> *mut Self::Type {
+ // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to
+ // `led_classdev` embedded within a `Self::Type`.
+ unsafe { container_of!(led_cdev, bindings::led_classdev_mc, led_cdev) }
+ }
+
+ unsafe fn pre_brightness_set(raw: *mut Self::Type, brightness: u32) {
+ // SAFETY: The function's contract guarantees that `raw` is a valid pointer to
+ // `led_classdev_mc`.
+ unsafe { bindings::led_mc_calc_color_components(raw, brightness) };
+ }
+
+ fn release(led_cdev: &mut Self::Type) {
+ // SAFETY: `subled_info` is guaranteed to be a valid array pointer to `mc_subled` with the
+ // length and capacity of `led_cdev.num_colors`. See `led::Device::new_multicolor`.
+ let _subleds_vec = unsafe {
+ KVec::from_raw_parts(
+ led_cdev.subled_info,
+ led_cdev.num_colors as usize,
+ led_cdev.num_colors as usize,
+ )
+ };
+ }
+}
+
+impl<T: LedOps<Mode = MultiColor>> Device<T> {
+ /// Registers a new multicolor led classdev.
+ ///
+ /// The [`Device`] will be unregistered on drop.
+ pub fn new_multicolor<'a>(
+ parent: &'a T::Bus,
+ init_data: InitData<'a>,
+ ops: T,
+ subleds: &'a [MultiColorSubLed],
+ ) -> impl PinInit<Devres<Self>, Error> + 'a {
+ assert!(subleds.len() <= u32::MAX as usize);
+ Self::__new(parent, init_data, ops, |led_cdev| {
+ let mut subleds_vec = KVec::new();
+ subleds_vec.extend_from_slice(subleds, GFP_KERNEL)?;
+ let (subled_info, num_colors, capacity) = subleds_vec.into_raw_parts();
+ debug_assert_eq!(num_colors, capacity);
+
+ let mut used = 0;
+ if subleds.iter().any(|subled| {
+ let bit = 1 << (subled.color as u32);
+ if (used & bit) != 0 {
+ true
+ } else {
+ used |= bit;
+ false
+ }
+ }) {
+ dev_err!(parent.as_ref(), "duplicate color in multicolor led\n");
+ return Err(EINVAL);
+ }
+
+ Ok(bindings::led_classdev_mc {
+ led_cdev,
+ // CAST: We checked above that the length of subleds fits into a u32.
+ num_colors: num_colors as u32,
+ // CAST: The safeguards in the const block ensure that `MultiColorSubLed` has an
+ // identical layout to `mc_subled`.
+ subled_info: subled_info.cast::<bindings::mc_subled>(),
+ })
+ })
+ }
+
+ /// Returns the subleds passed to [`Device::new_multicolor`].
+ 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>(),
+ raw.num_colors as usize,
+ )
+ }
+ }
+}
--
2.51.0
^ permalink raw reply related [flat|nested] 8+ messages in thread