| Message ID | 20260903-rust_leds-v24-1-8d9e4c04db69@posteo.de |
|---|---|
| State | New |
| Headers | show |
| Series | rust: leds: add led classdev abstractions | expand |
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] The LED abstraction uses the unmanaged `led_classdev_register_ext` API, violating subsystem guidelines and introducing memory safety vulnerabilities. - [High] DeviceBuilder unsafely recovers the `FwNode` reference count from the mutable C structure `dev->fwnode`, leading to use-after-free or memory leaks. - [High] Concurrent calls to `blink_set_callback` create aliased mutable references to shared `delay_on` and `delay_off` variables, causing Undefined Behavior. - [Medium] Eager evaluation of `.then_some()` forces instantiation of optional vtable callbacks, causing linker errors if methods are omitted. -- commit 729f3d81e7ef8cb49db95c3c7c2412eca5d86a36 Author: Markus Probst <markus.probst@posteo.de> rust: leds: add basic led classdev abstractions This commit implements the core Rust abstractions for the LED subsystem. It introduces traits and structures such as LedOps, DeviceBuilder, and Device to safely wrap and manage led_classdev registration and operations. > diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs > index 0000000000000..816db7c576894 > --- a/rust/kernel/led/normal.rs > +++ b/rust/kernel/led/normal.rs [ ... ] > @@ -25,58 +25,58 @@ impl<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> { > self, > parent: &'bound T::Bus, > ops: impl PinInit<T, Error> + 'init, > ) -> impl PinInit<Device<'bound, T>, Error> + 'init { > const_assert!(T::MAX_BRIGHTNESS <= i32::MAX.unsigned_abs() || !T::HAS_BRIGHTNESS_GET); > > try_pin_init!(Device { > ops <- ops, > classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev| { > // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write. > // `led_classdev` gets fully initialized in-place by > // `led_classdev_register_ext` including `mutex` and `list_head`. > unsafe { > ptr.write(bindings::led_classdev { > brightness_set: (!T::BLOCKING) > .then_some(Adapter::<T>::brightness_set_callback), > brightness_set_blocking: T::BLOCKING > .then_some(Adapter::<T>::brightness_set_blocking_callback), > brightness_get: T::HAS_BRIGHTNESS_GET > .then_some(Adapter::<T>::brightness_get_callback), [Severity: Medium] Since .then_some() is a method call, doesn't it evaluate its argument eagerly? Passing Adapter::<T>::brightness_get_callback forces the compiler to instantiate the function body, which in turn evaluates T::brightness_get. If the trait method is omitted by the driver, it defaults to invoking build_error!(). Will this forced instantiation create a reference to the external symbol rust_build_error that resists dead-code elimination and results in a linker error? Would a standard if T::HAS_BRIGHTNESS_GET { Some(...) } else { None } block prevent this eager evaluation? > blink_set: T::HAS_BLINK_SET.then_some(Adapter::<T>::blink_set_callback), > max_brightness: T::MAX_BRIGHTNESS, > brightness: self.initial_brightness, > color: self.color as u32, > name: self.name.map_or(core::ptr::null(), CStrExt::as_char_ptr), > ..bindings::led_classdev::default() > }) > }; > > let mut init_data = bindings::led_init_data { > fwnode: self > .fwnode > .as_ref() > .map_or(core::ptr::null_mut(), |fwnode| fwnode.as_raw()), > default_label: core::ptr::null(), > devicename: self > .devicename > .map_or(core::ptr::null(), CStrExt::as_char_ptr), > devname_mandatory: self.devname_mandatory, > }; > > // SAFETY: > // - `parent.as_ref().as_raw()` is guaranteed to be a pointer to a valid > // `device`. > // - `ptr` is guaranteed to be a pointer to an initialized `led_classdev`. > to_result(unsafe { > bindings::led_classdev_register_ext( > parent.as_ref().as_raw(), > ptr, [Severity: High] Does using the unmanaged led_classdev_register_ext() API here violate the LED subsystem guidelines? The guidelines mandate managed registration (devm_led_classdev_register_ext) to prevent resource leaks and use-after-free bugs. If the Rust device is safely leaked (e.g., via Box::leak or a cyclic Arc), the PinnedDrop destructor is never called. Won't this leave the unmanaged LED registered after the driver module unloads, potentially causing an exploitable use-after-free kernel panic on subsequent userspace sysfs accesses? > if self.name.is_none() { > &raw mut init_data > } else { > core::ptr::null_mut() > }, > ) > })?; > > core::mem::forget(self.fwnode); // keep the reference count incremented [Severity: High] Is it safe to unconditionally forget self.fwnode here? If self.name is Some, init_data is passed as NULL to the C subsystem, so the FwNode is ignored. Doesn't this permanently leak the ARef? [ ... ] > @@ -174,27 +174,27 @@ impl<T: LedOps> Adapter<T> { > /// # Safety > /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a > /// `led::Device`. > /// `delay_on` and `delay_off` must be valid pointers to `usize` and have > /// exclusive access for the period of this function. > /// This function is called on enabling hardware accelerated blinking. > unsafe extern "C" fn blink_set_callback( > led_cdev: *mut bindings::led_classdev, > delay_on: *mut usize, > delay_off: *mut usize, > ) -> i32 { > from_result(|| { > // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a > // `led_classdev` embedded within a `led::Device`. > let classdev = unsafe { Device::<T>::from_raw(led_cdev) }; > // SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`. > let parent = unsafe { T::Bus::from_device(classdev.parent()) }; > > classdev.ops.blink_set( > parent, > classdev, > // SAFETY: The function's contract guarantees that `delay_on` points to a `usize` > // and is exclusive for the period of this function. > unsafe { &mut *delay_on }, > // SAFETY: The function's contract guarantees that `delay_off` points to a `usize` > // and is exclusive for the period of this function. > unsafe { &mut *delay_off }, [Severity: High] Can this create aliased mutable references to shared variables? The C LED subsystem allows lockless concurrent calls to led_blink_set() from sysfs handlers like led_delay_on_store(). When multiple threads write to the sysfs attributes concurrently, they pass pointers to the exact same shared variables. If the Rust abstraction unconditionally casts these raw pointers into exclusive mutable references (&mut *delay_on and &mut *delay_off), doesn't this violate Rust's aliasing rules and result in Undefined Behavior? [ ... ] > @@ -208,12 +208,12 @@ impl<T: LedOps> Adapter<T> { > #[pinned_drop] > impl<'bound, T: LedOps + 'bound> PinnedDrop for Device<'bound, T> { > fn drop(self: Pin<&mut Self>) { > let raw = self.classdev.get(); > // SAFETY: The existence of `self` guarantees that `self.classdev.get()` is a pointer to a > // valid `led_classdev`. > let dev: &device::Device = unsafe { device::Device::from_raw((*raw).dev) }; > > let _fwnode = dev > .fwnode() > // SAFETY: the reference count of `fwnode` has previously been > // incremented in `led::DeviceBuilder::build`. > .map(|fwnode| unsafe { ARef::from_raw(NonNull::from(fwnode)) }); [Severity: High] Could this reconstruct an ARef from a globally-mutable C pointer and cause a Use-After-Free? If self.fwnode was None during DeviceBuilder::build, forget() does nothing. But if the kernel later dynamically assigns an fwnode to the class device (e.g., via device links or driver core), this drop handler unconditionally reconstructs an ARef and drops it, decrementing a reference count it never incremented. Should the Rust abstraction store its own Option<ARef> internally to securely track ownership instead of relying on the C structure?
On Thu Sep 3, 2026 at 12:01 AM BST, Markus Probst wrote: > Implement the core abstractions needed for led class devices, including: > > * `led::LedOps` - the trait for handling leds, including > `brightness_set`, `brightness_get` and `blink_set` > > * `led::DeviceBuilder` - the builder for the led class device > > * `led::Device` - a safe wrapper around `led_classdev` > > Signed-off-by: Markus Probst <markus.probst@posteo.de> > --- > rust/kernel/led.rs | 288 ++++++++++++++++++++++++++++++++++++++++++++++ > rust/kernel/led/normal.rs | 230 ++++++++++++++++++++++++++++++++++++ > rust/kernel/lib.rs | 1 + > 3 files changed, 519 insertions(+) > > [snip] > > +/// Trait defining the operations for a LED driver. > +/// > +/// # Examples > +/// ``` > +/// use kernel::{ > +/// device, > +/// devres::Devres, > +/// led, > +/// macros::vtable, > +/// platform, > +/// prelude::*, // > +/// }; > +/// > +/// struct MyLedOps; > +/// > +/// > +/// #[vtable] > +/// impl led::LedOps for MyLedOps { > +/// type Bus = platform::Device<device::Bound>; > +/// const BLOCKING: bool = false; > +/// const MAX_BRIGHTNESS: u32 = 255; > +/// > +/// fn brightness_set<'bound>( > +/// &self, > +/// _dev: &'bound platform::Device<device::Bound>, > +/// _classdev: &led::Device<'bound, Self>, > +/// _brightness: u32 > +/// ) -> Result<()> { > +/// // Set the brightness for the led here > +/// Ok(()) > +/// } > +/// } > +/// ``` > +/// Led drivers must implement this trait in order to register and handle a [`Device`]. > +#[vtable] > +pub trait LedOps: Send + Sync + Sized { > + /// The bus device required by the implementation. > + #[allow(private_bounds)] > + type Bus: AsBusDevice<Bound>; Does LED class device has no private data that driver can use? This can be either a private pointer or extra allocation living at the end of the classdev struct. It's usually a antipattern to get the bus device directly, especially that in Rust we do not allow anything other than callbacks to access data on bus devices. Instead, the class device registration should provide a data initializer, and the callbacks would receive a pointer to the data instead. In cases that a device resource has to be referenced, it should be kept inside the private data by the driver themselves. Best, Gary > + > + /// If set true, [`LedOps::brightness_set`] and [`LedOps::blink_set`] must perform the > + /// operation immediately. If set false, they must not sleep. > + const BLOCKING: bool; > + /// The max brightness level. > + const MAX_BRIGHTNESS: u32; > + > + /// Sets the brightness level. > + /// > + /// See also [`LedOps::BLOCKING`]. > + fn brightness_set<'bound>( > + &self, > + dev: &'bound Self::Bus, > + classdev: &Device<'bound, Self>, > + brightness: u32, > + ) -> Result<()>; > + > + /// Gets the current brightness level. > + fn brightness_get<'bound>( > + &self, > + dev: &'bound Self::Bus, > + classdev: &Device<'bound, Self>, > + ) -> Result<u32> { > + let _ = (dev, classdev); > + build_error!(VTABLE_DEFAULT_ERROR) > + } > + > + /// Activates hardware accelerated blinking. > + /// > + /// delays are in milliseconds. If both are zero, a sensible default should be chosen. > + /// The caller should adjust the timings in that case and if it can't match the values > + /// specified exactly. Setting the brightness to 0 will disable the hardware accelerated > + /// blinking. > + /// > + /// See also [`LedOps::BLOCKING`]. > + fn blink_set<'bound>( > + &self, > + dev: &'bound Self::Bus, > + classdev: &Device<'bound, Self>, > + delay_on: &mut usize, > + delay_off: &mut usize, > + ) -> Result<()> { > + let _ = (dev, classdev, delay_on, delay_off); > + build_error!(VTABLE_DEFAULT_ERROR) > + } > +}
On Fri, 2026-09-04 at 14:03 +0100, Gary Guo wrote: > On Thu Sep 3, 2026 at 12:01 AM BST, Markus Probst wrote: > > Implement the core abstractions needed for led class devices, including: > > > > * `led::LedOps` - the trait for handling leds, including > > `brightness_set`, `brightness_get` and `blink_set` > > > > * `led::DeviceBuilder` - the builder for the led class device > > > > * `led::Device` - a safe wrapper around `led_classdev` > > > > Signed-off-by: Markus Probst <markus.probst@posteo.de> > > --- > > rust/kernel/led.rs | 288 ++++++++++++++++++++++++++++++++++++++++++++++ > > rust/kernel/led/normal.rs | 230 ++++++++++++++++++++++++++++++++++++ > > rust/kernel/lib.rs | 1 + > > 3 files changed, 519 insertions(+) > > > > [snip] > > > > +/// Trait defining the operations for a LED driver. > > +/// > > +/// # Examples > > +/// ``` > > +/// use kernel::{ > > +/// device, > > +/// devres::Devres, > > +/// led, > > +/// macros::vtable, > > +/// platform, > > +/// prelude::*, // > > +/// }; > > +/// > > +/// struct MyLedOps; > > +/// > > +/// > > +/// #[vtable] > > +/// impl led::LedOps for MyLedOps { > > +/// type Bus = platform::Device<device::Bound>; > > +/// const BLOCKING: bool = false; > > +/// const MAX_BRIGHTNESS: u32 = 255; > > +/// > > +/// fn brightness_set<'bound>( > > +/// &self, > > +/// _dev: &'bound platform::Device<device::Bound>, > > +/// _classdev: &led::Device<'bound, Self>, > > +/// _brightness: u32 > > +/// ) -> Result<()> { > > +/// // Set the brightness for the led here > > +/// Ok(()) > > +/// } > > +/// } > > +/// ``` > > +/// Led drivers must implement this trait in order to register and handle a [`Device`]. > > +#[vtable] > > +pub trait LedOps: Send + Sync + Sized { > > + /// The bus device required by the implementation. > > + #[allow(private_bounds)] > > + type Bus: AsBusDevice<Bound>; > > Does LED class device has no private data that driver can use? This can be > either a private pointer or extra allocation living at the end of the classdev > struct. On every callback `&self` is passed to the LedOps, which could be considered the leds private data. It is currently stored in front of the `led_classdev` struct. > > It's usually a antipattern to get the bus device directly, especially that in > Rust we do not allow anything other than callbacks to access data on bus > devices. > > Instead, the class device registration should provide a data initializer, and > the callbacks would receive a pointer to the data instead. In cases that a > device resource has to be referenced, it should be kept inside the private data > by the driver themselves. It should be possible to store a pointer to the bus device directly on this data, thus I can remove it. If I think about it, I could add a `led::Device::drvdata` function, so it could be accessed from the drivers private data. Sync is a requirement anyway. Thanks - Markus Probst > > Best, > Gary > > > + > > + /// If set true, [`LedOps::brightness_set`] and [`LedOps::blink_set`] must perform the > > + /// operation immediately. If set false, they must not sleep. > > + const BLOCKING: bool; > > + /// The max brightness level. > > + const MAX_BRIGHTNESS: u32; > > + > > + /// Sets the brightness level. > > + /// > > + /// See also [`LedOps::BLOCKING`]. > > + fn brightness_set<'bound>( > > + &self, > > + dev: &'bound Self::Bus, > > + classdev: &Device<'bound, Self>, > > + brightness: u32, > > + ) -> Result<()>; > > + > > + /// Gets the current brightness level. > > + fn brightness_get<'bound>( > > + &self, > > + dev: &'bound Self::Bus, > > + classdev: &Device<'bound, Self>, > > + ) -> Result<u32> { > > + let _ = (dev, classdev); > > + build_error!(VTABLE_DEFAULT_ERROR) > > + } > > + > > + /// Activates hardware accelerated blinking. > > + /// > > + /// delays are in milliseconds. If both are zero, a sensible default should be chosen. > > + /// The caller should adjust the timings in that case and if it can't match the values > > + /// specified exactly. Setting the brightness to 0 will disable the hardware accelerated > > + /// blinking. > > + /// > > + /// See also [`LedOps::BLOCKING`]. > > + fn blink_set<'bound>( > > + &self, > > + dev: &'bound Self::Bus, > > + classdev: &Device<'bound, Self>, > > + delay_on: &mut usize, > > + delay_off: &mut usize, > > + ) -> Result<()> { > > + let _ = (dev, classdev, delay_on, delay_off); > > + build_error!(VTABLE_DEFAULT_ERROR) > > + } > > +}
On Fri Sep 4, 2026 at 2:15 PM BST, Markus Probst wrote: > On Fri, 2026-09-04 at 14:03 +0100, Gary Guo wrote: >> On Thu Sep 3, 2026 at 12:01 AM BST, Markus Probst wrote: >> > Implement the core abstractions needed for led class devices, including: >> > >> > * `led::LedOps` - the trait for handling leds, including >> > `brightness_set`, `brightness_get` and `blink_set` >> > >> > * `led::DeviceBuilder` - the builder for the led class device >> > >> > * `led::Device` - a safe wrapper around `led_classdev` >> > >> > Signed-off-by: Markus Probst <markus.probst@posteo.de> >> > --- >> > rust/kernel/led.rs | 288 ++++++++++++++++++++++++++++++++++++++++++++++ >> > rust/kernel/led/normal.rs | 230 ++++++++++++++++++++++++++++++++++++ >> > rust/kernel/lib.rs | 1 + >> > 3 files changed, 519 insertions(+) >> > >> > [snip] >> > >> > +/// Trait defining the operations for a LED driver. >> > +/// >> > +/// # Examples >> > +/// ``` >> > +/// use kernel::{ >> > +/// device, >> > +/// devres::Devres, >> > +/// led, >> > +/// macros::vtable, >> > +/// platform, >> > +/// prelude::*, // >> > +/// }; >> > +/// >> > +/// struct MyLedOps; >> > +/// >> > +/// >> > +/// #[vtable] >> > +/// impl led::LedOps for MyLedOps { >> > +/// type Bus = platform::Device<device::Bound>; >> > +/// const BLOCKING: bool = false; >> > +/// const MAX_BRIGHTNESS: u32 = 255; >> > +/// >> > +/// fn brightness_set<'bound>( >> > +/// &self, >> > +/// _dev: &'bound platform::Device<device::Bound>, >> > +/// _classdev: &led::Device<'bound, Self>, >> > +/// _brightness: u32 >> > +/// ) -> Result<()> { >> > +/// // Set the brightness for the led here >> > +/// Ok(()) >> > +/// } >> > +/// } >> > +/// ``` >> > +/// Led drivers must implement this trait in order to register and handle a [`Device`]. >> > +#[vtable] >> > +pub trait LedOps: Send + Sync + Sized { >> > + /// The bus device required by the implementation. >> > + #[allow(private_bounds)] >> > + type Bus: AsBusDevice<Bound>; >> >> Does LED class device has no private data that driver can use? This can be >> either a private pointer or extra allocation living at the end of the classdev >> struct. > On every callback `&self` is passed to the LedOps, which could be > considered the leds private data. It is currently stored in front of > the `led_classdev` struct. Right, I missed that. In that case I think you can just remove `Bus` completely from the callback? Do you have a user that needs this info? BTW, it would also help to include a link to a potential user in the cover letter so people can see how the API is supposed to be used. This is especially useful for API design reviews. >> >> It's usually a antipattern to get the bus device directly, especially that in >> Rust we do not allow anything other than callbacks to access data on bus >> devices. >> >> Instead, the class device registration should provide a data initializer, and >> the callbacks would receive a pointer to the data instead. In cases that a >> device resource has to be referenced, it should be kept inside the private data >> by the driver themselves. > It should be possible to store a pointer to the bus device directly on > this data, thus I can remove it. > > If I think about it, I could add a `led::Device::drvdata` function, so > it could be accessed from the drivers private data. Sync is a > requirement anyway. An option is to provide `Deref`. Then you could even have `self: &Device<'bound, Self>` in callbacks. That said, you might want to eventually support type-erased `Device` types to support consumer of LED class devices. So I'm unsure if we want to provide data accessors on class devices (maybe eventually device'll be split into two types?) Best, Gary
On Fri, 2026-09-04 at 14:32 +0100, Gary Guo wrote: > On Fri Sep 4, 2026 at 2:15 PM BST, Markus Probst wrote: > > On Fri, 2026-09-04 at 14:03 +0100, Gary Guo wrote: > > > On Thu Sep 3, 2026 at 12:01 AM BST, Markus Probst wrote: > > > > Implement the core abstractions needed for led class devices, including: > > > > > > > > * `led::LedOps` - the trait for handling leds, including > > > > `brightness_set`, `brightness_get` and `blink_set` > > > > > > > > * `led::DeviceBuilder` - the builder for the led class device > > > > > > > > * `led::Device` - a safe wrapper around `led_classdev` > > > > > > > > Signed-off-by: Markus Probst <markus.probst@posteo.de> > > > > --- > > > > rust/kernel/led.rs | 288 ++++++++++++++++++++++++++++++++++++++++++++++ > > > > rust/kernel/led/normal.rs | 230 ++++++++++++++++++++++++++++++++++++ > > > > rust/kernel/lib.rs | 1 + > > > > 3 files changed, 519 insertions(+) > > > > > > > > [snip] > > > > > > > > +/// Trait defining the operations for a LED driver. > > > > +/// > > > > +/// # Examples > > > > +/// ``` > > > > +/// use kernel::{ > > > > +/// device, > > > > +/// devres::Devres, > > > > +/// led, > > > > +/// macros::vtable, > > > > +/// platform, > > > > +/// prelude::*, // > > > > +/// }; > > > > +/// > > > > +/// struct MyLedOps; > > > > +/// > > > > +/// > > > > +/// #[vtable] > > > > +/// impl led::LedOps for MyLedOps { > > > > +/// type Bus = platform::Device<device::Bound>; > > > > +/// const BLOCKING: bool = false; > > > > +/// const MAX_BRIGHTNESS: u32 = 255; > > > > +/// > > > > +/// fn brightness_set<'bound>( > > > > +/// &self, > > > > +/// _dev: &'bound platform::Device<device::Bound>, > > > > +/// _classdev: &led::Device<'bound, Self>, > > > > +/// _brightness: u32 > > > > +/// ) -> Result<()> { > > > > +/// // Set the brightness for the led here > > > > +/// Ok(()) > > > > +/// } > > > > +/// } > > > > +/// ``` > > > > +/// Led drivers must implement this trait in order to register and handle a [`Device`]. > > > > +#[vtable] > > > > +pub trait LedOps: Send + Sync + Sized { > > > > + /// The bus device required by the implementation. > > > > + #[allow(private_bounds)] > > > > + type Bus: AsBusDevice<Bound>; > > > > > > Does LED class device has no private data that driver can use? This can be > > > either a private pointer or extra allocation living at the end of the classdev > > > struct. > > On every callback `&self` is passed to the LedOps, which could be > > considered the leds private data. It is currently stored in front of > > the `led_classdev` struct. > > Right, I missed that. In that case I think you can just remove `Bus` completely > from the callback? Yes. > > Do you have a user that needs this info? BTW, it would also help to include a > link to a potential user in the cover letter so people can see how the API is > supposed to be used. This is especially useful for API design reviews. Primarily https://lore.kernel.org/rust-for-linux/20260724-synology_microp_initial-v18-0-fb2f49f10e77@posteo.de/ . But I also have another rust i2c driver, which would use the led abstraction: https://codeberg.org/0xIO32/linux/src/branch/synology_disk_leds Still needs changes before it can be submitted. > > > > > > > It's usually a antipattern to get the bus device directly, especially that in > > > Rust we do not allow anything other than callbacks to access data on bus > > > devices. > > > > > > Instead, the class device registration should provide a data initializer, and > > > the callbacks would receive a pointer to the data instead. In cases that a > > > device resource has to be referenced, it should be kept inside the private data > > > by the driver themselves. > > It should be possible to store a pointer to the bus device directly on > > this data, thus I can remove it. > > > > If I think about it, I could add a `led::Device::drvdata` function, so > > it could be accessed from the drivers private data. Sync is a > > requirement anyway. > > An option is to provide `Deref`. Then you could even have > `self: &Device<'bound, Self>` in callbacks. Didn't know we had `feature(arbitrary_self_types)` enabled. I like this idea. > > That said, you might want to eventually support type-erased `Device` types to > support consumer of LED class devices. So I'm unsure if we want to provide data > accessors on class devices (maybe eventually device'll be split into two types?) Sounds good, but I won't implement any consumer functions until they are needed. Thanks - Markus Probst > > Best, > Gary
diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs new file mode 100644 index 000000000000..596975e103b8 --- /dev/null +++ b/rust/kernel/led.rs @@ -0,0 +1,288 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Abstractions for the leds driver model. +//! +//! C header: [`include/linux/leds.h`](srctree/include/linux/leds.h) + +use core::{ + marker::PhantomData, + mem::transmute, + ptr::NonNull, // +}; + +use crate::{ + container_of, + device::{ + self, + property::FwNode, + AsBusDevice, + Bound, // + }, + error::{ + from_result, + to_result, + VTABLE_DEFAULT_ERROR, // + }, + macros::vtable, + prelude::*, + str::CStrExt, + sync::aref::ARef, + types::Opaque, // +}; + +mod normal; + +pub use normal::Device; + +/// The name of the led is determined by the driver. +pub enum Named {} +/// The name of the led is determined by its fwnode. +pub enum Unnamed {} + +/// How the name of the led should be determined. +pub trait DeviceBuilderState: private::Sealed {} + +impl DeviceBuilderState for Named {} +impl private::Sealed for Named {} +impl DeviceBuilderState for Unnamed {} +impl private::Sealed for Unnamed {} + +/// The builder to register a led class device. +/// +/// See [`LedOps`]. +pub struct DeviceBuilder<'init, S> { + fwnode: Option<ARef<FwNode>>, + name: Option<&'init CStr>, + devicename: Option<&'init CStr>, + devname_mandatory: bool, + initial_brightness: u32, + color: Color, + _p: PhantomData<S>, +} + +impl<S: DeviceBuilderState> DeviceBuilder<'static, S> { + /// Creates a new [`DeviceBuilder`]. + #[inline] + #[expect( + clippy::new_without_default, + reason = "no need and derive is prevented by S" + )] + pub fn new() -> Self { + Self { + fwnode: None, + name: None, + devicename: None, + devname_mandatory: false, + initial_brightness: 0, + color: Color::default(), + _p: PhantomData, + } + } +} + +impl<'init> DeviceBuilder<'init, Unnamed> { + /// Sets the firmware node. + #[inline] + pub fn fwnode(self, fwnode: Option<ARef<FwNode>>) -> Self { + Self { fwnode, ..self } + } + + /// Sets the device name. + #[inline] + pub fn devicename(self, devicename: &'init CStr) -> Self { + Self { + devicename: Some(devicename), + ..self + } + } + + /// Sets if a device name is mandatory. + #[inline] + pub fn devicename_mandatory(self, mandatory: bool) -> Self { + Self { + devname_mandatory: mandatory, + ..self + } + } +} + +impl<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> { + /// Sets the initial brightness value for the led. + /// + /// The default brightness is 0. + /// If [`LedOps::brightness_get`] is implemented, this value will be ignored. + #[inline] + pub fn initial_brightness(self, brightness: u32) -> Self { + Self { + initial_brightness: brightness, + ..self + } + } + + /// Sets the color of the led. + /// + /// This value can be overwritten by the "color" fwnode property. + #[inline] + pub fn color(self, color: Color) -> Self { + Self { color, ..self } + } +} + +impl<'init> DeviceBuilder<'init, Named> { + /// Sets the name of the led. + /// + /// Setting this will prevent the fwnode from being used and prevents automatic name + /// composition. + #[inline] + pub fn name(self, name: &'init CStr) -> Self { + Self { + name: Some(name), + ..self + } + } +} + +/// Trait defining the operations for a LED driver. +/// +/// # Examples +/// ``` +/// use kernel::{ +/// device, +/// devres::Devres, +/// led, +/// macros::vtable, +/// platform, +/// prelude::*, // +/// }; +/// +/// struct MyLedOps; +/// +/// +/// #[vtable] +/// impl led::LedOps for MyLedOps { +/// type Bus = platform::Device<device::Bound>; +/// const BLOCKING: bool = false; +/// const MAX_BRIGHTNESS: u32 = 255; +/// +/// fn brightness_set<'bound>( +/// &self, +/// _dev: &'bound platform::Device<device::Bound>, +/// _classdev: &led::Device<'bound, Self>, +/// _brightness: u32 +/// ) -> Result<()> { +/// // Set the brightness for the led here +/// Ok(()) +/// } +/// } +/// ``` +/// Led drivers must implement this trait in order to register and handle a [`Device`]. +#[vtable] +pub trait LedOps: Send + Sync + Sized { + /// The bus device required by the implementation. + #[allow(private_bounds)] + type Bus: AsBusDevice<Bound>; + + /// If set true, [`LedOps::brightness_set`] and [`LedOps::blink_set`] must perform the + /// operation immediately. If set false, they must not sleep. + const BLOCKING: bool; + /// The max brightness level. + const MAX_BRIGHTNESS: u32; + + /// Sets the brightness level. + /// + /// See also [`LedOps::BLOCKING`]. + fn brightness_set<'bound>( + &self, + dev: &'bound Self::Bus, + classdev: &Device<'bound, Self>, + brightness: u32, + ) -> Result<()>; + + /// Gets the current brightness level. + fn brightness_get<'bound>( + &self, + dev: &'bound Self::Bus, + classdev: &Device<'bound, Self>, + ) -> Result<u32> { + let _ = (dev, classdev); + build_error!(VTABLE_DEFAULT_ERROR) + } + + /// Activates hardware accelerated blinking. + /// + /// delays are in milliseconds. If both are zero, a sensible default should be chosen. + /// The caller should adjust the timings in that case and if it can't match the values + /// specified exactly. Setting the brightness to 0 will disable the hardware accelerated + /// blinking. + /// + /// See also [`LedOps::BLOCKING`]. + fn blink_set<'bound>( + &self, + dev: &'bound Self::Bus, + classdev: &Device<'bound, Self>, + delay_on: &mut usize, + delay_off: &mut usize, + ) -> Result<()> { + let _ = (dev, classdev, delay_on, delay_off); + build_error!(VTABLE_DEFAULT_ERROR) + } +} + +/// Led colors. +#[derive(Copy, Clone, Debug, Default, PartialEq, Eq)] +#[repr(u32)] +#[non_exhaustive] +#[expect( + missing_docs, + reason = "it shouldn't be necessary to document each color" +)] +pub enum Color { + #[default] + White = bindings::LED_COLOR_ID_WHITE, + Red = bindings::LED_COLOR_ID_RED, + Green = bindings::LED_COLOR_ID_GREEN, + Blue = bindings::LED_COLOR_ID_BLUE, + Amber = bindings::LED_COLOR_ID_AMBER, + Violet = bindings::LED_COLOR_ID_VIOLET, + Yellow = bindings::LED_COLOR_ID_YELLOW, + Ir = bindings::LED_COLOR_ID_IR, + Multi = bindings::LED_COLOR_ID_MULTI, + Rgb = bindings::LED_COLOR_ID_RGB, + Purple = bindings::LED_COLOR_ID_PURPLE, + Orange = bindings::LED_COLOR_ID_ORANGE, + Pink = bindings::LED_COLOR_ID_PINK, + Cyan = bindings::LED_COLOR_ID_CYAN, + Lime = bindings::LED_COLOR_ID_LIME, +} +static_assert!(bindings::LED_COLOR_ID_MAX == 15); + +impl Color { + /// Name of the color + #[inline] + pub fn as_c_str(self) -> &'static CStr { + // SAFETY: + // - `self as u8` is a valid led color id. + // - `led_get_color_name` always returns a valid C string pointer. + unsafe { CStr::from_char_ptr(bindings::led_get_color_name(self as u8)) } + } +} + +impl TryFrom<u32> for Color { + type Error = Error; + + fn try_from(value: u32) -> core::result::Result<Self, Self::Error> { + if value < bindings::LED_COLOR_ID_MAX { + // SAFETY: + // - `Color` is represented as `u32` + // - the static_assert above guarantees that no additional color has been added + // - `value` is guaranteed to be in the color id range + Ok(unsafe { transmute::<u32, Color>(value) }) + } else { + Err(EINVAL) + } + } +} + +mod private { + pub trait Sealed {} +} diff --git a/rust/kernel/led/normal.rs b/rust/kernel/led/normal.rs new file mode 100644 index 000000000000..816db7c57689 --- /dev/null +++ b/rust/kernel/led/normal.rs @@ -0,0 +1,230 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Led mode for the `struct led_classdev`. +//! +//! C header: [`include/linux/leds.h`](srctree/include/linux/leds.h) + +use super::*; + +/// The led class device representation. +/// +/// This structure represents the Rust abstraction for a led class device. +#[pin_data(PinnedDrop)] +pub struct Device<'bound, T: LedOps + 'bound> { + #[pin] + ops: T, + #[pin] + classdev: Opaque<bindings::led_classdev>, + _p: PhantomData<&'bound ()>, +} + +impl<'init, S: DeviceBuilderState> DeviceBuilder<'init, S> { + /// Registers a new [`Device`]. + pub fn build<'bound: 'init, T: LedOps + 'bound>( + self, + parent: &'bound T::Bus, + ops: impl PinInit<T, Error> + 'init, + ) -> impl PinInit<Device<'bound, T>, Error> + 'init { + const_assert!(T::MAX_BRIGHTNESS <= i32::MAX.unsigned_abs() || !T::HAS_BRIGHTNESS_GET); + + try_pin_init!(Device { + ops <- ops, + classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev| { + // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write. + // `led_classdev` gets fully initialized in-place by + // `led_classdev_register_ext` including `mutex` and `list_head`. + unsafe { + ptr.write(bindings::led_classdev { + brightness_set: (!T::BLOCKING) + .then_some(Adapter::<T>::brightness_set_callback), + brightness_set_blocking: T::BLOCKING + .then_some(Adapter::<T>::brightness_set_blocking_callback), + brightness_get: T::HAS_BRIGHTNESS_GET + .then_some(Adapter::<T>::brightness_get_callback), + blink_set: T::HAS_BLINK_SET.then_some(Adapter::<T>::blink_set_callback), + max_brightness: T::MAX_BRIGHTNESS, + brightness: self.initial_brightness, + color: self.color as u32, + name: self.name.map_or(core::ptr::null(), CStrExt::as_char_ptr), + ..bindings::led_classdev::default() + }) + }; + + let mut init_data = bindings::led_init_data { + fwnode: self + .fwnode + .as_ref() + .map_or(core::ptr::null_mut(), |fwnode| fwnode.as_raw()), + default_label: core::ptr::null(), + devicename: self + .devicename + .map_or(core::ptr::null(), CStrExt::as_char_ptr), + devname_mandatory: self.devname_mandatory, + }; + + // SAFETY: + // - `parent.as_ref().as_raw()` is guaranteed to be a pointer to a valid + // `device`. + // - `ptr` is guaranteed to be a pointer to an initialized `led_classdev`. + to_result(unsafe { + bindings::led_classdev_register_ext( + parent.as_ref().as_raw(), + ptr, + if self.name.is_none() { + &raw mut init_data + } else { + core::ptr::null_mut() + }, + ) + })?; + + core::mem::forget(self.fwnode); // keep the reference count incremented + + Ok::<_, Error>(()) + }), + _p: PhantomData, + }) + } +} + +impl<'bound, T: LedOps + 'bound> Device<'bound, T> { + /// # Safety + /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a + /// `led::Device`. + #[inline] + unsafe fn from_raw<'a>(led_cdev: *mut bindings::led_classdev) -> &'a Self { + // SAFETY: The function's contract guarantees that `led_cdev` points to a `led_classdev` + // field embedded within a valid `led::Device`. `container_of!` can therefore + // safely calculate the address of the containing struct. + unsafe { &*container_of!(Opaque::cast_from(led_cdev), Self, classdev) } + } + + #[inline] + fn parent(&self) -> &'bound device::Device<Bound> { + // SAFETY: `self.classdev.get()` is guaranteed to be a valid pointer to `led_classdev`. + unsafe { device::Device::from_raw((*(*self.classdev.get()).dev).parent) } + } +} + +// SAFETY: A `led::Device` can be unregistered from any thread. +unsafe impl<'bound, T: LedOps + 'bound + Send> Send for Device<'bound, T> {} + +// SAFETY: `led::Device` can be shared among threads because all methods of `led::Device` +// are thread safe. +unsafe impl<'bound, T: LedOps + 'bound + Sync> Sync for Device<'bound, T> {} + +struct Adapter<T: LedOps> { + _p: PhantomData<T>, +} + +impl<T: LedOps> Adapter<T> { + /// # Safety + /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a + /// `led::Device`. + /// This function is called on setting the brightness of a led. + unsafe extern "C" fn brightness_set_callback( + led_cdev: *mut bindings::led_classdev, + brightness: u32, + ) { + // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a + // `led_classdev` embedded within a `led::Device`. + let classdev = unsafe { Device::<T>::from_raw(led_cdev) }; + // SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`. + let parent = unsafe { T::Bus::from_device(classdev.parent()) }; + + let _ = classdev.ops.brightness_set(parent, classdev, brightness); + } + + /// # Safety + /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a + /// `led::Device`. + /// This function is called on setting the brightness of a led immediately. + unsafe extern "C" fn brightness_set_blocking_callback( + led_cdev: *mut bindings::led_classdev, + brightness: u32, + ) -> i32 { + from_result(|| { + // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a + // `led_classdev` embedded within a `led::Device`. + let classdev = unsafe { Device::<T>::from_raw(led_cdev) }; + // SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`. + let parent = unsafe { T::Bus::from_device(classdev.parent()) }; + + classdev.ops.brightness_set(parent, classdev, brightness)?; + Ok(0) + }) + } + + /// # Safety + /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a + /// `led::Device`. + /// This function is called on getting the brightness of a led. + unsafe extern "C" fn brightness_get_callback(led_cdev: *mut bindings::led_classdev) -> u32 { + // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a + // `led_classdev` embedded within a `led::Device`. + let classdev = unsafe { Device::<T>::from_raw(led_cdev) }; + // SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`. + let parent = unsafe { T::Bus::from_device(classdev.parent()) }; + + // CAST: Resulting value will be casted back to i32 in the led subsystem. + from_result(|| { + classdev + .ops + .brightness_get(parent, classdev) + .inspect(|val| debug_assert!(*val <= T::MAX_BRIGHTNESS)) + .and_then(|val| Ok(i32::try_from(val)?)) + }) as u32 + } + + /// # Safety + /// `led_cdev` must be a valid pointer to a `led_classdev` embedded within a + /// `led::Device`. + /// `delay_on` and `delay_off` must be valid pointers to `usize` and have + /// exclusive access for the period of this function. + /// This function is called on enabling hardware accelerated blinking. + unsafe extern "C" fn blink_set_callback( + led_cdev: *mut bindings::led_classdev, + delay_on: *mut usize, + delay_off: *mut usize, + ) -> i32 { + from_result(|| { + // SAFETY: The function's contract guarantees that `led_cdev` is a valid pointer to a + // `led_classdev` embedded within a `led::Device`. + let classdev = unsafe { Device::<T>::from_raw(led_cdev) }; + // SAFETY: `classdev.parent()` is guaranteed to be contained in `T::Bus`. + let parent = unsafe { T::Bus::from_device(classdev.parent()) }; + + classdev.ops.blink_set( + parent, + classdev, + // SAFETY: The function's contract guarantees that `delay_on` points to a `usize` + // and is exclusive for the period of this function. + unsafe { &mut *delay_on }, + // SAFETY: The function's contract guarantees that `delay_off` points to a `usize` + // and is exclusive for the period of this function. + unsafe { &mut *delay_off }, + )?; + Ok(0) + }) + } +} + +#[pinned_drop] +impl<'bound, T: LedOps + 'bound> PinnedDrop for Device<'bound, T> { + fn drop(self: Pin<&mut Self>) { + let raw = self.classdev.get(); + // SAFETY: The existence of `self` guarantees that `self.classdev.get()` is a pointer to a + // valid `led_classdev`. + let dev: &device::Device = unsafe { device::Device::from_raw((*raw).dev) }; + + let _fwnode = dev + .fwnode() + // SAFETY: the reference count of `fwnode` has previously been + // incremented in `led::DeviceBuilder::build`. + .map(|fwnode| unsafe { ARef::from_raw(NonNull::from(fwnode)) }); + + // SAFETY: The existence of `self` guarantees that `self.classdev` has previously been + // successfully registered with `led_classdev_register_ext`. + unsafe { bindings::led_classdev_unregister(raw) }; + } +} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 4d5c96ddc49c..748788ad131b 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -96,6 +96,7 @@ pub mod jump_label; #[cfg(CONFIG_KUNIT)] pub mod kunit; +pub mod led; pub mod list; pub mod maple_tree; pub mod miscdevice;
Implement the core abstractions needed for led class devices, including: * `led::LedOps` - the trait for handling leds, including `brightness_set`, `brightness_get` and `blink_set` * `led::DeviceBuilder` - the builder for the led class device * `led::Device` - a safe wrapper around `led_classdev` Signed-off-by: Markus Probst <markus.probst@posteo.de> --- rust/kernel/led.rs | 288 ++++++++++++++++++++++++++++++++++++++++++++++ rust/kernel/led/normal.rs | 230 ++++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + 3 files changed, 519 insertions(+)