* [PATCH v2 0/5] Rust LED Driver Abstractions and Lenovo SE10 Driver with DMI and I/O Port Support
@ 2025-01-13 12:16 Fiona Behrens
2025-01-13 12:16 ` [PATCH v2 1/5] rust: dmi: Add abstractions for DMI Fiona Behrens
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: Fiona Behrens @ 2025-01-13 12:16 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Pavel Machek, Lee Jones, Jean Delvare
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Fiona Behrens,
Mark Pearson, Peter Koch, rust-for-linux, linux-leds,
linux-kernel
This patch series introduces Rust LED abstractions and provides a driver for the Lenovo ThinkEdge SE10 using those abstractions.
1. DMI Abstraction:
- Introduced Rust macros and types for DMI-based system matching, providing functionality similar to the `MODULE_DEVICE_TABLE(dmi, x)` macro in C. This abstraction allows system-specific matching using DMI fields.
2. LED Abstractions:
- Introduced a generic LED abstraction that supports on/off control, brightness levels, and hardware blinking.
- The abstraction is implemented using a vtable, with a struct that holds the generic driver data and is pinned for direct LED registration.
3. I/O Port Abstractions:
- Added abstractions for hardware I/O port memory access, enabling the driver to interact with hardware ports.
4. LED Driver for Lenovo ThinkEdge SE10:
- A driver is implemented for the red LED on the front panel of the Lenovo ThinkEdge SE10, utilizing the previously introduced LED abstractions, I/O port abstractions, and DMI matching.
- The driver supports on/off control and a WWAN hardware trigger.
Additionally, the LED abstraction has been prepared to support multicolor LEDs in the future. While this functionality is planned, it has not yet been implemented, as I do not have a consumer for it at this time.
Would it make sense to add the Rust DMI abstractions to the existing DMI/SMBIOS SUPPORT entry in the MAINTAINERS file?
This series depends on `rust: time: Introduce Delta type` by FUJITA Tomonori[1].
I send this previously without the SE10 driver as an RFC[2], and now have a driver to consume the APIs.
[1]: https://lore.kernel.org/rust-for-linux/20241220061853.2782878-3-fujita.tomonori@gmail.com/
[2]: https://lore.kernel.org/rust-for-linux/20241009105759.579579-1-me@kloenk.dev/
Fiona Behrens (5):
rust: dmi: Add abstractions for DMI
rust: leds: Add Rust bindings for LED subsystem
rust: leds: Add hardware trigger support for hardware-controlled LEDs
rust: add I/O port abstractions with resource management
leds: leds_lenovo_se10: LED driver for Lenovo SE10 platform
MAINTAINERS | 2 +
drivers/leds/Kconfig | 10 +
drivers/leds/Makefile | 1 +
drivers/leds/leds_lenovo_se10.rs | 132 ++++++++
rust/bindings/bindings_helper.h | 1 +
rust/helpers/helpers.c | 1 +
rust/helpers/ioport.c | 42 +++
rust/kernel/dmi.rs | 533 +++++++++++++++++++++++++++++++
rust/kernel/ioport.rs | 169 ++++++++++
rust/kernel/leds.rs | 416 ++++++++++++++++++++++++
rust/kernel/leds/triggers.rs | 128 ++++++++
rust/kernel/lib.rs | 6 +
rust/kernel/time.rs | 7 +
13 files changed, 1448 insertions(+)
create mode 100644 drivers/leds/leds_lenovo_se10.rs
create mode 100644 rust/helpers/ioport.c
create mode 100644 rust/kernel/dmi.rs
create mode 100644 rust/kernel/ioport.rs
create mode 100644 rust/kernel/leds.rs
create mode 100644 rust/kernel/leds/triggers.rs
--
2.47.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/5] rust: dmi: Add abstractions for DMI
2025-01-13 12:16 [PATCH v2 0/5] Rust LED Driver Abstractions and Lenovo SE10 Driver with DMI and I/O Port Support Fiona Behrens
@ 2025-01-13 12:16 ` Fiona Behrens
2025-01-13 12:16 ` [PATCH v2 2/5] rust: leds: Add Rust bindings for LED subsystem Fiona Behrens
` (3 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Fiona Behrens @ 2025-01-13 12:16 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Pavel Machek, Lee Jones, Jean Delvare
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Fiona Behrens,
Mark Pearson, Peter Koch, rust-for-linux, linux-leds,
linux-kernel, Sebastian Walz
Introduce Rust macros and types to support DMI-based system matching,
providing functionality similar to the `MODULE_DEVICE_TABLE(dmi, x)`
macro in C.
- Add the `dmi_system_id!` macro for defining system identifiers and
matching specific DMI fields.
- Add the `dmi_device_table!` macro, which allows module aliases to be
available after a full build when compiled as a module.
- Define the `Field` enum for DMI field IDs, ensuring compatibility
with existing C bindings.
These abstractions enable writing Rust kernel drivers that rely on DMI
data for system-specific behavior and autoloading.
Co-developed-by: Sebastian Walz <sivizius@sivizius.eu>
Signed-off-by: Sebastian Walz <sivizius@sivizius.eu>
Signed-off-by: Fiona Behrens <me@kloenk.dev>
---
rust/bindings/bindings_helper.h | 1 +
rust/kernel/dmi.rs | 533 ++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 +
3 files changed, 536 insertions(+)
create mode 100644 rust/kernel/dmi.rs
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 5c4dfe22f41a..d20b2a6af9c9 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -11,6 +11,7 @@
#include <linux/blk_types.h>
#include <linux/blkdev.h>
#include <linux/cred.h>
+#include <linux/dmi.h>
#include <linux/errname.h>
#include <linux/ethtool.h>
#include <linux/file.h>
diff --git a/rust/kernel/dmi.rs b/rust/kernel/dmi.rs
new file mode 100644
index 000000000000..ba2a010d0e55
--- /dev/null
+++ b/rust/kernel/dmi.rs
@@ -0,0 +1,533 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! DMI support.
+//!
+//! C header: [`inlcude/linux/dmi.h`](srctree/include/linux/dmi.h)
+
+use core::marker::PhantomData;
+use core::num::NonZeroU32;
+use core::ops::Deref;
+
+use crate::prelude::*;
+use crate::str::CStr;
+
+/// Create a new [`SystemId`].
+///
+/// # Examples
+///
+/// Create a [`SystemId`] that matches if the [`BiosVendor`] is `qemu`.
+///
+/// ```
+/// let system_id = kernel::dmi_system_id!("qemu": [
+/// BiosVendor: "qemu" @exact,
+/// ProductFamily: "rust",
+/// ]);
+/// assert_eq!(system_id.matches()[0].slot().unwrap().unwrap(), kernel::dmi::Field::BiosVendor);
+/// assert_eq!(system_id.matches()[0].exact_match(), true);
+/// assert_eq!(system_id.matches()[1].exact_match(), false);
+/// ```
+///
+/// A [`SystemId`] cannot hold more than 4 matches and must not be empty,
+/// otherwise it will fail to compile.
+///
+// TODO: replace with `compile_fail` when supported.
+/// ```ignore
+/// let system_id = kernel::dmi_system_id!("qemu": [
+/// BiosVendor: "qemu",
+/// BiosVersion: "8.0",
+/// ProductName: "doctest",
+/// ProductFamily: "rust",
+/// BoardVendor: "qemu"
+/// ]);
+/// ```
+///
+/// [`BiosVendor`]: Field::BiosVendor
+#[macro_export]
+macro_rules! dmi_system_id {
+ (@strmatch, $slot:ident, $match:literal) => {
+ $crate::dmi::StrMatch::new($crate::dmi::Field::$slot, $match)
+ };
+ (@strmatch, $slot:ident, $match:literal, @exact) => {
+ $crate::dmi::StrMatch::new_exact($crate::dmi::Field::$slot, $match)
+ };
+ ($ident:literal: [$($slot:ident: $match:literal $(@$exact:ident)? $(,)?)+]) => {
+ const {
+ match $crate::dmi::SystemId::new_from_slice($crate::c_str!($ident), &[
+ $($crate::dmi_system_id!(@strmatch, $slot, $match$(, @$exact)?),)+
+ ]) {
+ Some(v) => v,
+ _ => $crate::build_error("Invalid length for matches"),
+ }
+ }
+ };
+}
+
+/// Create a new static [`SystemIdList`] and export it as a device table to generate alias
+/// definitions to load the driver from userspace if compiled as module.
+///
+/// # Examples
+///
+/// Create a device table with the name `QEMU_DMI_TABLE` that loads the driver if the
+/// [`BiosVendor`] is `qemu`.
+///
+/// ```
+/// kernel::dmi_device_table!(1, QEMU_DMI_TABLE, ["qemu": [BiosVendor: "qemu"]]);
+/// # assert_eq!(QEMU_DMI_TABLE.len(), 1);
+/// ```
+///
+/// [`BiosVendor`]: Field::BiosVendor
+#[macro_export]
+macro_rules! dmi_device_table {
+ (
+ $N:literal,
+ $name:ident,
+ [$($ident:literal: [$($slot:ident: $match:literal $(, @$exact:ident)?$(,)?)+]$(,)?)+]
+ ) => {
+ #[cfg_attr(MODULE, export_name = concat!("__mod_device_table__dmi__", stringify!($name)))]
+ #[cfg_attr(MODULE, used)]
+ static $name: $crate::dmi::SystemIdList<'static, $N> =
+ $crate::dmi::SystemIdList::new([$(
+ $crate::dmi_system_id!($ident: [$($slot: $match $(, @$exact)?)+]),
+ )+]);
+ };
+}
+
+/// DMI field id.
+#[derive(Debug, PartialEq, Eq, Copy, Clone)]
+#[repr(u8)]
+pub enum Field {
+ // None
+ /// Bios Vendor
+ BiosVendor = bindings::dmi_field_DMI_BIOS_VENDOR as u8,
+ /// Bios Version
+ BiosVersion = bindings::dmi_field_DMI_BIOS_VERSION as u8,
+ /// Bios Date
+ BiosDate = bindings::dmi_field_DMI_BIOS_DATE as u8,
+ /// Bios Release
+ BiosRelease = bindings::dmi_field_DMI_BIOS_RELEASE as u8,
+ /// Embedded Controller Firmware Release
+ EcFirmwareRelease = bindings::dmi_field_DMI_EC_FIRMWARE_RELEASE as u8,
+ /// System Vendor
+ SysVendor = bindings::dmi_field_DMI_SYS_VENDOR as u8,
+ /// Product Name
+ ProductName = bindings::dmi_field_DMI_PRODUCT_NAME as u8,
+ /// Product Version
+ ProductVersion = bindings::dmi_field_DMI_PRODUCT_VERSION as u8,
+ /// Product Serial
+ ProductSerial = bindings::dmi_field_DMI_PRODUCT_SERIAL as u8,
+ /// Product UUID
+ ProductUuid = bindings::dmi_field_DMI_PRODUCT_UUID as u8,
+ /// Product SKU
+ ProductSku = bindings::dmi_field_DMI_PRODUCT_SKU as u8,
+ /// Product Family
+ ProductFamily = bindings::dmi_field_DMI_PRODUCT_FAMILY as u8,
+ /// Board Vendor
+ BoardVendor = bindings::dmi_field_DMI_BOARD_VENDOR as u8,
+ /// Board Name
+ BoardName = bindings::dmi_field_DMI_BOARD_NAME as u8,
+ /// Board Version
+ BoardVersion = bindings::dmi_field_DMI_BOARD_VERSION as u8,
+ /// Board Serial
+ BoardSerial = bindings::dmi_field_DMI_BOARD_SERIAL as u8,
+ /// Board Asset Tag
+ BoardAssetTag = bindings::dmi_field_DMI_BOARD_ASSET_TAG as u8,
+ /// Chassis Vendor
+ ChassisVendor = bindings::dmi_field_DMI_CHASSIS_VENDOR as u8,
+ /// Chassis Type
+ ChassisType = bindings::dmi_field_DMI_CHASSIS_TYPE as u8,
+ /// Chassis Version
+ ChassisVersion = bindings::dmi_field_DMI_CHASSIS_VERSION as u8,
+ /// Chassis Serial
+ ChassisSerial = bindings::dmi_field_DMI_CHASSIS_SERIAL as u8,
+ /// Chassis Asset Tag
+ ChassisAssetTag = bindings::dmi_field_DMI_CHASSIS_ASSET_TAG as u8,
+ // StringMax,
+ // OemString,
+}
+
+impl Field {
+ /// Return DMI data value.
+ ///
+ /// Returns one DMI data value, can be used to perform complex DMI data checks.
+ pub fn get_system_info(self) -> Option<&'static CStr> {
+ // SAFETY: C call, self is a valid enum
+ let ptr = unsafe { bindings::dmi_get_system_info(self as u8 as _) };
+ if ptr.is_null() {
+ None
+ } else {
+ // SAFETY: ptr checked to be none null above and to be a valid char ptr.
+ Some(unsafe { CStr::from_char_ptr(ptr) })
+ }
+ }
+
+ /// Compare a string to the dmi field (if exists).
+ ///
+ /// Returns true if the requested field equals to the check value (including None).
+ /// wraps the `dmi_match` C function.
+ pub fn compare(self, value: Option<&CStr>) -> bool {
+ // SAFETY: C call, self is valid enum and value is null or a string
+ unsafe {
+ bindings::dmi_match(
+ self as u8 as _,
+ value.map(CStr::as_char_ptr).unwrap_or(core::ptr::null()),
+ )
+ }
+ }
+}
+
+impl TryFrom<u8> for Field {
+ type Error = Error;
+
+ /// Tries to convert a u8 id to a [`Field`].
+ ///
+ /// # Errors
+ ///
+ /// Returns [`EINVAL`] if the id does not match a known field.
+ fn try_from(value: u8) -> Result<Self, Self::Error> {
+ Ok(match value as u32 {
+ bindings::dmi_field_DMI_BIOS_VENDOR => Self::BiosVendor,
+ bindings::dmi_field_DMI_BIOS_VERSION => Self::BiosVersion,
+ bindings::dmi_field_DMI_BIOS_DATE => Self::BiosDate,
+ bindings::dmi_field_DMI_BIOS_RELEASE => Self::BiosRelease,
+ bindings::dmi_field_DMI_EC_FIRMWARE_RELEASE => Self::EcFirmwareRelease,
+ bindings::dmi_field_DMI_SYS_VENDOR => Self::SysVendor,
+ bindings::dmi_field_DMI_PRODUCT_NAME => Self::ProductName,
+ bindings::dmi_field_DMI_PRODUCT_VERSION => Self::ProductVersion,
+ bindings::dmi_field_DMI_PRODUCT_SERIAL => Self::ProductSerial,
+ bindings::dmi_field_DMI_PRODUCT_UUID => Self::ProductUuid,
+ bindings::dmi_field_DMI_PRODUCT_SKU => Self::ProductSku,
+ bindings::dmi_field_DMI_PRODUCT_FAMILY => Self::ProductFamily,
+ bindings::dmi_field_DMI_BOARD_VENDOR => Self::BoardVendor,
+ bindings::dmi_field_DMI_BOARD_NAME => Self::BoardName,
+ bindings::dmi_field_DMI_BOARD_VERSION => Self::BoardVersion,
+ bindings::dmi_field_DMI_BOARD_SERIAL => Self::BoardSerial,
+ bindings::dmi_field_DMI_BOARD_ASSET_TAG => Self::BoardAssetTag,
+ bindings::dmi_field_DMI_CHASSIS_VENDOR => Self::ChassisVendor,
+ bindings::dmi_field_DMI_CHASSIS_TYPE => Self::ChassisType,
+ bindings::dmi_field_DMI_CHASSIS_VERSION => Self::ChassisVersion,
+ bindings::dmi_field_DMI_CHASSIS_SERIAL => Self::ChassisSerial,
+ bindings::dmi_field_DMI_CHASSIS_ASSET_TAG => Self::ChassisAssetTag,
+ _ => return Err(EINVAL),
+ })
+ }
+}
+
+/// String Match entry for DMI.
+///
+/// Wraps the C struct `dmi_strmatch`.
+#[derive(Copy, Clone)]
+#[repr(transparent)]
+pub struct StrMatch(bindings::dmi_strmatch);
+
+impl StrMatch {
+ /// Zeroed match entry.
+ pub const ZERO: Self = {
+ // SAFETY: all zero is a valid match with slot none.
+ unsafe { core::mem::zeroed() }
+ };
+
+ /// Create a new String match entry that is not an exact match.
+ ///
+ /// This function copies the string into it's own allocation.
+ #[inline]
+ pub const fn new(slot: Field, substr: &str) -> Self {
+ Self::new_with_exact(slot, substr, false)
+ }
+
+ /// Create a new String match entry that wants an exact match.
+ ///
+ /// This function copies the string into it's own allocation.
+ #[inline]
+ pub const fn new_exact(slot: Field, substr: &str) -> Self {
+ Self::new_with_exact(slot, substr, true)
+ }
+
+ /// Create a new String match entry.
+ ///
+ /// This function copies the string into it's own allocation.
+ pub const fn new_with_exact(slot: Field, substr: &str, exact: bool) -> Self {
+ #[cfg(target_endian = "little")]
+ let (exactshift, slotshift) = (7, 0);
+ #[cfg(target_endian = "big")]
+ let (exactshift, slotshift) = (0, 1);
+
+ let bitfield = (exact as u8) << exactshift | (slot as u8) << slotshift;
+
+ let mut this = Self(bindings::dmi_strmatch {
+ _bitfield_1: bindings::__BindgenBitfieldUnit::new([bitfield]),
+ ..Self::ZERO.0
+ });
+
+ // copy substr into this
+ // core::ptr::copy_nonoverlapping is not const as mutable ref.
+ let input = substr.as_bytes();
+ let mut index = input.len();
+ let max = this.0.substr.len() - 1;
+ if index > max {
+ // TODO: find a way to generate warning from const
+ index = max;
+ }
+ loop {
+ if index == 0 {
+ break;
+ }
+
+ index -= 1;
+ this.0.substr[index] = input[index] as i8;
+ }
+
+ this
+ }
+
+ /// Return the slot this entry matches.
+ ///
+ /// # Errors
+ ///
+ /// Returns `Ok(None)` if the slot has the value 0, or [`EINVAL`] if the field id is unknown.
+ ///
+ /// # Examples
+ /// ```
+ /// # use kernel::dmi::{StrMatch, Field};
+ /// let strmatch = StrMatch::new(Field::BiosVendor, "qemu");
+ /// assert_eq!(strmatch.slot().unwrap().unwrap(), Field::BiosVendor);
+ /// ```
+ pub fn slot(&self) -> Result<Option<Field>> {
+ let slot = self.0.slot();
+ if slot == 0 {
+ Ok(None)
+ } else {
+ Some(Field::try_from(slot)).transpose()
+ }
+ }
+
+ /// Return if this match wants an exact match.
+ ///
+ /// # Examples
+ /// ```
+ /// # use kernel::dmi::{StrMatch, Field};
+ /// let strmatch = StrMatch::new_exact(Field::BiosVendor, "qemu");
+ /// assert_eq!(strmatch.exact_match(), true);
+ /// ```
+ #[inline]
+ pub fn exact_match(&self) -> bool {
+ self.0.exact_match() == 1
+ }
+
+ /// Return the substring to match for.
+ ///
+ /// # Examples
+ /// ```
+ /// # use kernel::dmi::{StrMatch, Field};
+ /// let strmatch = StrMatch::new(Field::BiosVendor, "qemu");
+ /// assert_eq!(strmatch.substr(), "qemu");
+ /// ```
+ pub fn substr(&self) -> &str {
+ let len = self.0.substr.into_iter().take_while(|x| *x != 0).count();
+ // SAFETY: substr is a valid str for len of len
+ unsafe {
+ core::str::from_utf8_unchecked(core::slice::from_raw_parts(
+ self.0.substr.as_ptr().cast(),
+ len,
+ ))
+ }
+ }
+}
+
+impl core::fmt::Debug for StrMatch {
+ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+ f.debug_struct("StrMatch")
+ .field("slot", &self.slot().ok().flatten())
+ .field("exact_match", &self.exact_match())
+ .field("substr", &self.substr())
+ .finish()
+ }
+}
+
+impl PartialEq for StrMatch {
+ fn eq(&self, other: &Self) -> bool {
+ self.0._bitfield_1 == other.0._bitfield_1 && self.0.substr == other.0.substr
+ }
+}
+
+impl Eq for StrMatch {}
+
+/// DMI system ID.
+///
+/// To create a system ID in a const context the macro [`kernel::dmi_system_id`] can be used.
+///
+/// Wraps the C struct `dmi_system_id`.
+#[repr(transparent)]
+pub struct SystemId<'a> {
+ id: bindings::dmi_system_id,
+ // lifetime anchor for ident, driver data and callback
+ _a: PhantomData<&'a ()>,
+}
+
+impl SystemId<'static> {
+ /// zeroed [`SystemId`] for trailing list.
+ pub const ZERO: Self = {
+ // SAFETY: all fields zeroed is valid.
+ unsafe { core::mem::zeroed() }
+ };
+}
+
+impl<'a> SystemId<'a> {
+ /// Create a new SystemId from a matches array.
+ pub const fn new(ident: &'a CStr, matches: [StrMatch; 4]) -> Self {
+ Self {
+ id: bindings::dmi_system_id {
+ callback: None,
+ ident: ident.as_char_ptr(),
+ // SAFETY: StrMatch is transparent over bindings::dmi_strmatch
+ matches: unsafe {
+ core::mem::transmute::<[StrMatch; 4], [bindings::dmi_strmatch; 4]>(matches)
+ },
+ driver_data: core::ptr::null_mut(),
+ },
+ _a: PhantomData,
+ }
+ }
+
+ /// Createa n new SystemId from a slice of matches, filling missing entries with [`StrMatch::ZERO`].
+ ///
+ /// Copying the matches, only provided to be used in macros.
+ #[doc(hidden)]
+ pub const fn new_from_slice(ident: &'a CStr, matches: &[StrMatch]) -> Option<Self> {
+ if matches.is_empty() || matches.len() > 4 {
+ return None;
+ }
+
+ let mut matches_a = [StrMatch::ZERO; 4];
+ let mut index = matches.len();
+ loop {
+ index -= 1;
+ matches_a[index] = matches[index];
+
+ if index == 0 {
+ break;
+ }
+ }
+ Some(Self::new(ident, matches_a))
+ }
+
+ /// Return the ident of the given SystemId.
+ ///
+ /// Returns a option as the C api allows to not set the ident.
+ pub fn ident_cstr(&self) -> Option<&'a CStr> {
+ let ident_ptr = self.id.ident;
+ if ident_ptr.is_null() {
+ return None;
+ }
+
+ // SAFETY: ident_ptr is valid and non null.
+ Some(unsafe { CStr::from_char_ptr(ident_ptr) })
+ }
+
+ /// Return the ident of the given SystemId as a rust [`str`].
+ ///
+ /// Returns a option as the C api allows to not set the ident.
+ /// See [`ident_cstr`].
+ ///
+ /// [`ident_cstr`]: Self::ident_cstr
+ #[inline]
+ pub fn ident(&self) -> Option<&'a str> {
+ self.ident_cstr().map(CStr::to_str).and_then(Result::ok)
+ }
+
+ /// Set the ident from the given optional string.
+ ///
+ /// Set to None to remove the ident.
+ pub fn set_ident(&mut self, ident: Option<&'a CStr>) {
+ if let Some(ident) = ident {
+ self.id.ident = ident.as_char_ptr();
+ } else {
+ self.id.ident = core::ptr::null();
+ }
+ }
+
+ /// Return the [`StrMatch`] array in the SystemId.
+ pub fn matches(&self) -> &[StrMatch; 4] {
+ // SAFETY: StrMatch is transparent over bindings::dmi_strmatch
+ unsafe { core::mem::transmute(&self.id.matches) }
+ }
+
+ /// Return a mutable reference to the StrMatch array in the SystemId.
+ pub fn matches_mut(&mut self) -> &mut [StrMatch; 4] {
+ // SAFETY: StrMatch is transparent over bindings::dmi_strmatch
+ unsafe { core::mem::transmute(&mut self.id.matches) }
+ }
+}
+
+impl core::fmt::Debug for SystemId<'_> {
+ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+ f.debug_struct("SystemId")
+ .field("ident", &self.ident())
+ .field("matches", &self.matches())
+ .field("callback", &self.id.callback)
+ .field("driver_data", &self.id.driver_data)
+ .finish()
+ }
+}
+
+// SAFETY: pointer to allocation with sufficent lifetime
+unsafe impl<'a> Sync for SystemId<'a> {}
+// SAFETY: pointer to allocation with sufficent lifetime
+unsafe impl<'a> Send for SystemId<'a> {}
+
+/// List of SystemIds, containing a zero sentinel to be used with dmi functions.
+///
+/// Can be exported as a device table with the [`kernel::dmi_device_table`] macro.
+#[repr(C)]
+pub struct SystemIdList<'a, const N: usize> {
+ ids: [SystemId<'a>; N],
+ sentinel: SystemId<'static>,
+}
+
+impl<'a, const N: usize> SystemIdList<'a, N> {
+ /// Create a new list from the given Id list, adding a zero sentinel.
+ pub const fn new(ids: [SystemId<'a>; N]) -> Self {
+ Self {
+ ids,
+ sentinel: SystemId::ZERO,
+ }
+ }
+
+ /// Check system DMI data
+ ///
+ /// Walk the blacklist table running matching functions until someone
+ /// returns non zero or we hit the end. Callback function is called for
+ /// each successful match. Returns the number of matches.
+ pub fn check_system(&self) -> Option<NonZeroU32> {
+ // SAFETY: C call, self has a sentinel
+ NonZeroU32::new(unsafe { bindings::dmi_check_system(self.as_raw_ptr()) as u32 })
+ }
+
+ /// Find the first match.
+ ///
+ /// Walk the blacklist table until the first match is found, and returns it.
+ pub fn first_match(&self) -> Option<&SystemId<'a>> {
+ // SAFETY: C call, self has a sentinel
+ let ptr = unsafe { bindings::dmi_first_match(self.as_raw_ptr()) };
+ if ptr.is_null() {
+ None
+ } else {
+ // SAFETY: ptr is non null. SystemId is transparent
+ Some(unsafe { &*ptr.cast() })
+ }
+ }
+
+ /// Return the raw pointer to the dmi_system_id array, including a terminating zero sentinel.
+ pub fn as_raw_ptr(&self) -> *const bindings::dmi_system_id {
+ // ids is the first element in the struct, safe to use as base pointer.
+ // SystemId is transparent over bindings::dmi_system_id
+ self.ids.as_ptr().cast()
+ }
+}
+
+impl<'a, const N: usize> Deref for SystemIdList<'a, N> {
+ type Target = [SystemId<'a>; N];
+
+ fn deref(&self) -> &Self::Target {
+ &self.ids
+ }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e1065a7551a3..e21f2e607963 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -35,6 +35,8 @@
mod build_assert;
pub mod cred;
pub mod device;
+#[cfg(CONFIG_DMI)]
+pub mod dmi;
pub mod error;
#[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
pub mod firmware;
--
2.47.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/5] rust: leds: Add Rust bindings for LED subsystem
2025-01-13 12:16 [PATCH v2 0/5] Rust LED Driver Abstractions and Lenovo SE10 Driver with DMI and I/O Port Support Fiona Behrens
2025-01-13 12:16 ` [PATCH v2 1/5] rust: dmi: Add abstractions for DMI Fiona Behrens
@ 2025-01-13 12:16 ` Fiona Behrens
2025-01-13 13:10 ` Miguel Ojeda
2025-01-13 12:16 ` [PATCH v2 3/5] rust: leds: Add hardware trigger support for hardware-controlled LEDs Fiona Behrens
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Fiona Behrens @ 2025-01-13 12:16 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Pavel Machek, Lee Jones, Jean Delvare
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Fiona Behrens,
Mark Pearson, Peter Koch, rust-for-linux, linux-leds,
linux-kernel
This patch introduces Rust support for the LED subsystem, adding abstractions
for the `led_classdev` struct and implementing core LED operations such as
`brightness_set`, `brightness_get`, and `blink_set` via a vtable trait.
A `Color` enum is defined to represent various LED colors, and a `LedConfig`
struct is used to configure LED properties such as name and color. The `Led`
struct wraps the C `led_classdev` and ensures proper memory management by being
pinned and automatically unregistered when dropped.
Support for a devm register variant is not included in this patch.
Signed-off-by: Fiona Behrens <me@kloenk.dev>
---
MAINTAINERS | 1 +
rust/kernel/leds.rs | 341 ++++++++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 +
rust/kernel/time.rs | 7 +
4 files changed, 351 insertions(+)
create mode 100644 rust/kernel/leds.rs
diff --git a/MAINTAINERS b/MAINTAINERS
index 30cbc3d44cd5..cef929b57159 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13019,6 +13019,7 @@ F: Documentation/leds/
F: drivers/leds/
F: include/dt-bindings/leds/
F: include/linux/leds.h
+F: rust/kernel/leds.rs
LEGO MINDSTORMS EV3
R: David Lechner <david@lechnology.com>
diff --git a/rust/kernel/leds.rs b/rust/kernel/leds.rs
new file mode 100644
index 000000000000..980af7c405d4
--- /dev/null
+++ b/rust/kernel/leds.rs
@@ -0,0 +1,341 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! LED subsystem.
+//!
+//! C header: [`includes/linux/leds.h`](srctree/include/linux/leds.h)
+
+use core::ptr;
+
+use crate::device::Device;
+use crate::error::from_result;
+use crate::ffi::c_ulong;
+use crate::prelude::*;
+use crate::time::Delta;
+use crate::types::Opaque;
+
+/// Color of an LED.
+#[allow(missing_docs)]
+#[derive(Copy, Clone)]
+#[repr(u32)]
+pub enum Color {
+ 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,
+ 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,
+
+ /// Infrared
+ IR = bindings::LED_COLOR_ID_IR,
+ /// Multicolor LEDs
+ Multi = bindings::LED_COLOR_ID_MULTI,
+ /// Multicolor LEDs that can do arbitrary color,
+ /// so this would include `RGBW` and similar
+ RGB = bindings::LED_COLOR_ID_RGB,
+}
+
+impl Color {
+ /// Get String representation of the Color.
+ pub fn name_cstr(&self) -> Option<&'static CStr> {
+ // SAFETY: C function call, enum is valid argument.
+ let name = unsafe { bindings::led_get_color_name(u32::from(self) as u8) };
+ if name.is_null() {
+ return None;
+ }
+ // SAFETY: pointer from above, valid for static lifetime.
+ Some(unsafe { CStr::from_char_ptr(name) })
+ }
+
+ /// Get String representation of the Color as rust type.
+ #[inline]
+ pub fn name(&self) -> Option<&'static str> {
+ self.name_cstr().map(|name|
+ // SAFETY: name from C name array which is valid UTF-8.
+ unsafe { name.as_str_unchecked() })
+ }
+}
+
+impl core::fmt::Display for Color {
+ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+ f.write_str(self.name().unwrap_or("Invalid color"))
+ }
+}
+
+impl From<Color> for u32 {
+ fn from(color: Color) -> Self {
+ color as u32
+ }
+}
+
+impl From<&Color> for u32 {
+ fn from(color: &Color) -> Self {
+ (*color).into()
+ }
+}
+
+impl TryFrom<u32> for Color {
+ type Error = Error;
+
+ /// Tris to convert a u32 into a [`Color`].
+ ///
+ /// # Errors
+ ///
+ /// Returns [`EINVAL`] if the color is not known.
+ fn try_from(value: u32) -> Result<Self, Self::Error> {
+ Ok(match value {
+ bindings::LED_COLOR_ID_WHITE => Color::White,
+ bindings::LED_COLOR_ID_RED => Color::Red,
+ bindings::LED_COLOR_ID_GREEN => Color::Green,
+ bindings::LED_COLOR_ID_BLUE => Color::Blue,
+ bindings::LED_COLOR_ID_AMBER => Color::Amber,
+ bindings::LED_COLOR_ID_VIOLET => Color::Violet,
+ bindings::LED_COLOR_ID_YELLOW => Color::Yellow,
+ bindings::LED_COLOR_ID_PURPLE => Color::Purple,
+ bindings::LED_COLOR_ID_ORANGE => Color::Orange,
+ bindings::LED_COLOR_ID_PINK => Color::Pink,
+ bindings::LED_COLOR_ID_CYAN => Color::Cyan,
+ bindings::LED_COLOR_ID_LIME => Color::Lime,
+ bindings::LED_COLOR_ID_IR => Color::IR,
+ bindings::LED_COLOR_ID_MULTI => Color::Multi,
+ bindings::LED_COLOR_ID_RGB => Color::RGB,
+ _ => return Err(EINVAL),
+ })
+ }
+}
+
+/// Data used for led registration.
+#[derive(Clone)]
+pub struct LedConfig<'name> {
+ /// Name to give the led.
+ pub name: Option<&'name CStr>,
+ /// Color of the LED.
+ pub color: Color,
+}
+
+/// A Led backed by a C `struct led_classdev`, additionally offering
+/// driver data storage.
+///
+/// The LED is unregistered once this LED struct is dropped.
+// TODO: add devm register variant
+#[pin_data(PinnedDrop)]
+pub struct Led<T> {
+ #[pin]
+ led: Opaque<bindings::led_classdev>,
+ /// Driver private data
+ pub data: T,
+}
+
+impl<T> Led<T>
+where
+ T: Operations<This = Led<T>>,
+{
+ /// Register a new LED.
+ ///
+ /// Copies the name if provided so that the lifetime of the name can end after the init function.
+ #[cfg(CONFIG_LEDS_CLASS)]
+ pub fn register<'a>(
+ device: Option<&'a Device>,
+ config: &'a LedConfig<'a>,
+ data: T,
+ ) -> impl PinInit<Self, Error> + 'a
+ where
+ T: 'a,
+ {
+ try_pin_init!(Self {
+ led <- Opaque::try_ffi_init(move |place: *mut bindings::led_classdev| {
+ // SAFETY: `place` is a pointer to a live allocation, so erasing is valid.
+ unsafe { place.write_bytes(0, 1) };
+
+ if let Some(name) = &config.name {
+ // SAFETY: `place` is a pointer to a live allocation.
+ let name_ptr = unsafe { ptr::addr_of_mut!((*place).name) };
+ // SAFETY: `name_ptr` points to a valid allocation and we have exclusive access.
+ unsafe { ptr::write(name_ptr, name.as_char_ptr()) };
+ }
+
+ // SAFETY: `place` is a pointer to a live allocation.
+ let color_ptr = unsafe { ptr::addr_of_mut!((*place).color) };
+ // SAFETY: `color_ptr` points to a valid allocation and we have exclusive access.
+ unsafe { ptr::write(color_ptr, config.color.into()) };
+
+ // SAFETY: `place` is a pointer to a live allocation.
+ let max_brightness_ptr = unsafe { ptr::addr_of_mut!((*place).max_brightness) };
+ // SAFETY: `max_brightness_ptr` points to a valid allocation and we have exclusive access.
+ unsafe { ptr::write(max_brightness_ptr, T::MAX_BRIGHTNESS as _) };
+
+ // SAFETY: `place` is a pointer to a live allocation.
+ let set_fn_ptr: *mut Option<_> = unsafe { ptr::addr_of_mut!((*place).brightness_set) };
+ // SAFETY: `set_fn_ptr` points to valid allocation and we have exclusive access.
+ unsafe { ptr::write(set_fn_ptr, Some(brightness_set::<T>)) };
+
+ if T::HAS_BRIGHTNESS_GET {
+ // SAFETY: `place` is pointing to a live allocation.
+ let get_fn_ptr: *mut Option<_> = unsafe { ptr::addr_of_mut!((*place).brightness_get) };
+ // SAFETY: `get_fn_ptr` points to a valid allocation and we have exclusive access.
+ unsafe { ptr::write(get_fn_ptr, Some(brightness_get::<T>)) };
+ }
+
+ if T::HAS_BLINK_SET {
+ // SAFETY: `place` is pointing to a live allocation.
+ let set_fn_ptr: *mut Option<_> = unsafe { ptr::addr_of_mut!((*place).blink_set) };
+ // SAFETY: `get_fn_ptr` points to a valid allocation and we have exclusive access.
+ unsafe { ptr::write(set_fn_ptr, Some(blink_set::<T>)) };
+ }
+
+
+ let dev = device.map(|dev| dev.as_raw()).unwrap_or(ptr::null_mut());
+ // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
+ crate::error::to_result(unsafe {
+ bindings::led_classdev_register_ext(dev, place, ptr::null_mut())
+ })
+ }),
+ data: data,
+ })
+ }
+}
+
+impl<T> private::Sealed for Led<T> {}
+
+impl<T> FromLedClassdev for Led<T> {
+ #[inline]
+ unsafe fn led_container_of(ptr: *mut bindings::led_classdev) -> *mut Self {
+ let ptr = ptr.cast::<Opaque<bindings::led_classdev>>();
+
+ // SAFETY: By the safety requirement of this function ptr is embedded in self.
+ unsafe { crate::container_of!(ptr, Self, led).cast_mut() }
+ }
+}
+
+#[pinned_drop]
+impl<T> PinnedDrop for Led<T> {
+ fn drop(self: Pin<&mut Self>) {
+ // SAFETY: led is pointing to a live allocation
+ #[cfg(CONFIG_LEDS_CLASS)]
+ unsafe {
+ bindings::led_classdev_unregister(self.led.get())
+ }
+ }
+}
+
+// SAFETY: `struct led_classdev` holds a mutex.
+unsafe impl<T: Send> Send for Led<T> {}
+// SAFETY: `struct led_classdev` holds a mutex.
+unsafe impl<T: Sync> Sync for Led<T> {}
+
+/// Led operations vtable.
+// TODO: add blocking variant (either via const generic or second trait)
+#[macros::vtable]
+pub trait Operations: Sized {
+ /// The maximum brightnes this led supports.
+ const MAX_BRIGHTNESS: u8;
+
+ /// Type of the container to use as self in the callbacks.
+ ///
+ /// This is most often [`Led<Self>`].
+ type This: FromLedClassdev;
+
+ /// Set LED brightness level.
+ /// This functions must not sleep.
+ fn brightness_set(this: &mut Self::This, brightness: u8);
+
+ /// Get the LED brightness level.
+ fn brightness_get(_this: &mut Self::This) -> u8 {
+ crate::build_error(crate::error::VTABLE_DEFAULT_ERROR)
+ }
+
+ /// Activae hardware accelerated blink, delays are in milliseconds
+ fn blink_set(
+ _this: &mut Self::This,
+ _delay_on: Delta,
+ _delay_off: Delta,
+ ) -> Result<(Delta, Delta)> {
+ crate::build_error(crate::error::VTABLE_DEFAULT_ERROR)
+ }
+}
+
+/// `brightness_set` function pointer
+///
+/// # Safety
+///
+/// `led_cdev` must be passed by the corresponding callback in `led_classdev`
+unsafe extern "C" fn brightness_set<T>(
+ led_cdev: *mut bindings::led_classdev,
+ brightness: bindings::led_brightness,
+) where
+ T: Operations,
+{
+ // SAFETY: By the safety requirement of this function `led_cdev` is embedded inside a `T::This<T>`.
+ let led = unsafe { &mut *(T::This::led_container_of(led_cdev.cast())) };
+ T::brightness_set(led, brightness as _);
+}
+
+/// `brightness_get` function pointer
+///
+/// # Safety
+///
+/// `led_cdev` must be passed by the corresponding callback in `led_classdev`
+unsafe extern "C" fn brightness_get<T>(
+ led_cdev: *mut bindings::led_classdev,
+) -> bindings::led_brightness
+where
+ T: Operations,
+{
+ // SAFETY: By the safety requirement of this function `led_cdev` is embedded inside a `T::This<T>`.
+ let led = unsafe { &mut *(T::This::led_container_of(led_cdev.cast())) };
+ T::brightness_get(led) as _
+}
+
+/// `blink_set` function pointer
+///
+/// # Safety
+///
+/// `led_cdev` must be passed by the corresponding callback in `led_classdev`
+unsafe extern "C" fn blink_set<T>(
+ led_cdev: *mut bindings::led_classdev,
+ delay_on: *mut c_ulong,
+ delay_off: *mut c_ulong,
+) -> i32
+where
+ T: Operations,
+{
+ from_result(|| {
+ // SAFETY: By the safety requirement of this function `led_cdev` is embedded inside a `T::This<T>`.
+ let led = unsafe { &mut *(T::This::led_container_of(led_cdev.cast())) };
+
+ // SAFETY: By the safety requirement of this function `delay_on` is pointing to a valid `c_ulong`.
+ let on = Delta::from_millis(unsafe { *delay_on } as i64);
+ // SAFETY: By the safety requirement of this function `delay_off` is pointing to a valid `c_ulong`.
+ let off = Delta::from_millis(unsafe { *delay_off } as i64);
+
+ let (on, off) = T::blink_set(led, on, off)?;
+
+ // writing back return values
+ // SAFETY: By the safety requirement of this function `delay_on` is pointing to a valid `c_ulong`.
+ unsafe { ptr::write(delay_on, on.as_millis_ceil() as c_ulong) };
+ // SAFETY: By the safety requirement of this function `delay_off` is pointing to a valid `c_ulong`.
+ unsafe { ptr::write(delay_off, off.as_millis_ceil() as c_ulong) };
+
+ Ok(0)
+ })
+}
+
+/// Trait to get the type from a `led_classdev` pointer.
+pub trait FromLedClassdev: private::Sealed {
+ /// Get pointer to the outer type from a `led_classdev` pointer.
+ ///
+ /// # Safety
+ ///
+ /// `ptr` must point to a [`bindings::led_classdev`] field in a struct of type `Self`.
+ unsafe fn led_container_of(ptr: *mut bindings::led_classdev) -> *mut Self;
+}
+
+mod private {
+ /// Marker that a trait cannot be implemented outside of this crate
+ pub trait Sealed {}
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e21f2e607963..8895a1683f82 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -46,6 +46,8 @@
pub mod jump_label;
#[cfg(CONFIG_KUNIT)]
pub mod kunit;
+#[cfg(CONFIG_NEW_LEDS)]
+pub mod leds;
pub mod list;
pub mod miscdevice;
#[cfg(CONFIG_NET)]
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 38da79925586..3c348ce4a7c2 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -143,4 +143,11 @@ pub fn as_nanos(self) -> i64 {
pub fn as_micros_ceil(self) -> i64 {
self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
}
+
+ /// Return the smallest number of milliseconds greater than or equal
+ /// to the value in the `Delta`.
+ #[inline]
+ pub fn as_millis_ceil(self) -> i64 {
+ self.as_nanos().saturating_add(NSEC_PER_MSEC - 1) / NSEC_PER_MSEC
+ }
}
--
2.47.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 3/5] rust: leds: Add hardware trigger support for hardware-controlled LEDs
2025-01-13 12:16 [PATCH v2 0/5] Rust LED Driver Abstractions and Lenovo SE10 Driver with DMI and I/O Port Support Fiona Behrens
2025-01-13 12:16 ` [PATCH v2 1/5] rust: dmi: Add abstractions for DMI Fiona Behrens
2025-01-13 12:16 ` [PATCH v2 2/5] rust: leds: Add Rust bindings for LED subsystem Fiona Behrens
@ 2025-01-13 12:16 ` Fiona Behrens
2025-01-20 10:35 ` Marek Behún
2025-01-13 12:16 ` [PATCH v2 4/5] rust: add I/O port abstractions with resource management Fiona Behrens
2025-01-13 12:16 ` [PATCH v2 5/5] leds: leds_lenovo_se10: LED driver for Lenovo SE10 platform Fiona Behrens
4 siblings, 1 reply; 17+ messages in thread
From: Fiona Behrens @ 2025-01-13 12:16 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Pavel Machek, Lee Jones, Jean Delvare
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Fiona Behrens,
Mark Pearson, Peter Koch, rust-for-linux, linux-leds,
linux-kernel
Adds abstraction for hardware trigger support in LEDs, enabling LEDs to
be controlled by external hardware events.
An `Arc` is embedded within the `led_classdev` to manage the lifecycle
of the hardware trigger, ensuring proper reference counting and cleanup
when the LED is dropped.
Signed-off-by: Fiona Behrens <me@kloenk.dev>
---
MAINTAINERS | 1 +
rust/kernel/leds.rs | 95 +++++++++++++++++++++++---
rust/kernel/leds/triggers.rs | 128 +++++++++++++++++++++++++++++++++++
3 files changed, 214 insertions(+), 10 deletions(-)
create mode 100644 rust/kernel/leds/triggers.rs
diff --git a/MAINTAINERS b/MAINTAINERS
index cef929b57159..954dbd311a55 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13020,6 +13020,7 @@ F: drivers/leds/
F: include/dt-bindings/leds/
F: include/linux/leds.h
F: rust/kernel/leds.rs
+F: rust/kernel/leds/
LEGO MINDSTORMS EV3
R: David Lechner <david@lechnology.com>
diff --git a/rust/kernel/leds.rs b/rust/kernel/leds.rs
index 980af7c405d4..f10a10b56e23 100644
--- a/rust/kernel/leds.rs
+++ b/rust/kernel/leds.rs
@@ -10,9 +10,14 @@
use crate::error::from_result;
use crate::ffi::c_ulong;
use crate::prelude::*;
+#[cfg(CONFIG_LEDS_TRIGGERS)]
+use crate::sync::Arc;
use crate::time::Delta;
use crate::types::Opaque;
+#[cfg(CONFIG_LEDS_TRIGGERS)]
+pub mod triggers;
+
/// Color of an LED.
#[allow(missing_docs)]
#[derive(Copy, Clone)]
@@ -110,12 +115,34 @@ fn try_from(value: u32) -> Result<Self, Self::Error> {
}
/// Data used for led registration.
-#[derive(Clone)]
-pub struct LedConfig<'name> {
+pub struct LedConfig<'name, T> {
/// Name to give the led.
pub name: Option<&'name CStr>,
/// Color of the LED.
pub color: Color,
+ /// Private data of the LED.
+ pub data: T,
+
+ /// Default trigger name.
+ pub default_trigger: Option<&'static CStr>,
+ /// Hardware trigger.
+ ///
+ /// Setting this to some also defaults the default trigger to this hardware trigger.
+ /// Use `default_trigger: Some("none")` to overwrite this.
+ #[cfg(CONFIG_LEDS_TRIGGERS)]
+ pub hardware_trigger: Option<Arc<triggers::Hardware<T>>>,
+}
+
+impl<'name, T> LedConfig<'name, T> {
+ /// Create a new LedConfig
+ pub fn new(color: Color, data: T) -> Self {
+ Self {
+ color,
+ data,
+ // SAFETY: all other fields are valid with zeroes.
+ ..unsafe { core::mem::zeroed() }
+ }
+ }
}
/// A Led backed by a C `struct led_classdev`, additionally offering
@@ -141,8 +168,7 @@ impl<T> Led<T>
#[cfg(CONFIG_LEDS_CLASS)]
pub fn register<'a>(
device: Option<&'a Device>,
- config: &'a LedConfig<'a>,
- data: T,
+ config: LedConfig<'a, T>,
) -> impl PinInit<Self, Error> + 'a
where
T: 'a,
@@ -188,14 +214,46 @@ pub fn register<'a>(
unsafe { ptr::write(set_fn_ptr, Some(blink_set::<T>)) };
}
+ #[cfg(CONFIG_LEDS_TRIGGERS)]
+ if let Some(trigger) = config.hardware_trigger {
+ let trigger = trigger.into_raw();
+ // SAFETY: `place` is pointing to a live allocation.
+ let trigger_type_ptr = unsafe { ptr::addr_of_mut!((*place).trigger_type) };
+ // SAFETY: `trigger` is a valid pointer
+ let hw_trigger = unsafe { ptr::addr_of!((*trigger).hw_type) };
+ // SAFETY: `trigger_type_ptr` points to a valid allocation and we have exclusive access.
+ unsafe { ptr::write(trigger_type_ptr, hw_trigger.cast_mut().cast()) };
+
+ // SAFETY: trigger points to a valid hardware trigger struct.
+ let trigger_name_ptr = unsafe { Opaque::raw_get(ptr::addr_of!( (*trigger).trigger)) };
+ // SAFETY: trigger points to a valid hardware trigger struct.
+ let trigger_name_ptr = unsafe { (*trigger_name_ptr).name };
+ // SAFETY: `place` is pointing to a live allocation.
+ let default_trigger_ptr = unsafe { ptr::addr_of_mut!((*place).default_trigger) };
+ // SAFETY: `default_trigger_ptr` points to a valid allocation and we have exclusive access.
+ unsafe { ptr::write(default_trigger_ptr, trigger_name_ptr) };
+
+ // SAFETY: `place` is pointing to a live allocation.
+ let hw_ctrl_trigger_ptr = unsafe { ptr::addr_of_mut!((*place).hw_control_trigger) };
+ // SAFETY: `hw_ctrl_trigger_ptr` points to a valid allocation and we have exclusive access.
+ unsafe { ptr::write(hw_ctrl_trigger_ptr, trigger_name_ptr) };
+ }
+
+ // After hw trigger impl, to overwrite default trigger
+ if let Some(default_trigger) = config.default_trigger {
+ // SAFETY: `place` is pointing to a live allocation.
+ let default_trigger_ptr = unsafe { ptr::addr_of_mut!((*place).default_trigger) };
+ // SAFETY: `default_trigger_ptr` points to a valid allocation and we have exclusive access.
+ unsafe { ptr::write(default_trigger_ptr, default_trigger.as_char_ptr()) };
+ }
- let dev = device.map(|dev| dev.as_raw()).unwrap_or(ptr::null_mut());
- // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
- crate::error::to_result(unsafe {
- bindings::led_classdev_register_ext(dev, place, ptr::null_mut())
- })
+ let dev = device.map(|dev| dev.as_raw()).unwrap_or(ptr::null_mut());
+ // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
+ crate::error::to_result(unsafe {
+ bindings::led_classdev_register_ext(dev, place, ptr::null_mut())
+ })
}),
- data: data,
+ data: config.data,
})
}
}
@@ -220,6 +278,23 @@ fn drop(self: Pin<&mut Self>) {
unsafe {
bindings::led_classdev_unregister(self.led.get())
}
+
+ // drop trigger if there is a hw trigger defined.
+ #[cfg(CONFIG_LEDS_TRIGGERS)]
+ {
+ // SAFETY: `self.led` is a valid led.
+ let hw_trigger_type =
+ unsafe { ptr::read(ptr::addr_of!((*self.led.get()).trigger_type)) };
+ if !hw_trigger_type.is_null() {
+ // SAFETY: hw_trigger_type is a valid and non null pointer into a Hardware trigger.
+ let hw_trigger_type = unsafe {
+ crate::container_of!(hw_trigger_type, triggers::Hardware<T>, hw_type)
+ };
+ // SAFETY: `hw_trigger_type` is a valid pointer that came from an arc.
+ let hw_trigger_type = unsafe { Arc::from_raw(hw_trigger_type) };
+ drop(hw_trigger_type);
+ }
+ }
}
}
diff --git a/rust/kernel/leds/triggers.rs b/rust/kernel/leds/triggers.rs
new file mode 100644
index 000000000000..d5f2b8252645
--- /dev/null
+++ b/rust/kernel/leds/triggers.rs
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! LED trigger abstractions.
+
+use core::marker::PhantomData;
+use core::ptr;
+
+use crate::error::{from_result, to_result};
+use crate::prelude::*;
+use crate::types::Opaque;
+
+use super::FromLedClassdev;
+
+/// LED Hardware trigger.
+///
+/// Used to impement a hardware operation mode for an LED.
+#[pin_data(PinnedDrop)]
+pub struct Hardware<T> {
+ #[pin]
+ pub(crate) hw_type: Opaque<bindings::led_hw_trigger_type>,
+ #[pin]
+ pub(crate) trigger: Opaque<bindings::led_trigger>,
+ _t: PhantomData<T>,
+}
+
+impl<T> Hardware<T>
+where
+ T: HardwareOperations,
+{
+ /// Register a new hardware Trigger with a given name.
+ pub fn register(name: &'static CStr) -> impl PinInit<Self, Error> {
+ try_pin_init!( Self {
+ // SAFETY: `led_hw_trigger_type` is valid with all zeroes.
+ hw_type: Opaque::new(unsafe { core::mem::zeroed() }),
+ trigger <- Opaque::try_ffi_init(move |place: *mut bindings::led_trigger| {
+ // SAFETY: `place` is a pointer to a live allocation, so erasing is valid.
+ unsafe { place.write_bytes(0, 1) };
+
+ // Add name
+ // SAFETY: `place` is pointing to a live allocation, so the deref is safe.
+ let name_ptr = unsafe { ptr::addr_of_mut!((*place).name) };
+ // SAFETY: `name_ptr` points to a valid allocation and we have exclusive access.
+ unsafe { ptr::write(name_ptr, name.as_char_ptr()) };
+
+ // Add fn pointers
+ // SAFETY: `place` is pointing to a live allocation, so the deref is safe.
+ let activate_fn_ptr: *mut Option<_> = unsafe { ptr::addr_of_mut!((*place).activate) };
+ // SAFETY: `activate_fn_ptr` points to a valid allocation and we have exclusive access.
+ unsafe { ptr::write(activate_fn_ptr, Some(trigger_activate::<T>)) };
+
+ if T::HAS_DEACTIVATE {
+ // SAFETY: `place` is pointing to a live allocation, so the deref is safe.
+ let deactivate_fn_ptr: *mut Option<_> = unsafe { ptr::addr_of_mut!((*place).deactivate) };
+ // SAFETY: `deactivate_fn_ptr` points to a valid allocation and we have exclusive access.
+ unsafe { ptr::write(deactivate_fn_ptr, Some(trigger_deactivate::<T>)) };
+ }
+
+ // Add hardware trigger
+ // SAFETY: `place` is pointing to a live allocation, so the deref is safe.
+ let trigger_type_ptr = unsafe { ptr::addr_of_mut!((*place).trigger_type) };
+ // SAFETY: `place` is pointing to a live allocation, so the deref is safe.
+ let trigger_type = unsafe { crate::container_of!(place, Self, trigger).cast_mut() };
+ // SAFETY: `trigger_type` is pointing to a live allocation of Self.
+ let trigger_type = unsafe { ptr::addr_of!((*trigger_type).hw_type) };
+ // SAFETY: `trigger_type_ptr` points to a valid allocation and we have exclusive access.
+ unsafe{ ptr::write(trigger_type_ptr, Opaque::raw_get(trigger_type)) };
+
+ // SAFETY: ffi call, `place` is sufficently filled with data at this point
+ to_result(unsafe {
+ bindings::led_trigger_register(place)
+ })
+ }),
+ _t: PhantomData,
+ })
+ }
+}
+
+#[pinned_drop]
+impl<T> PinnedDrop for Hardware<T> {
+ fn drop(self: Pin<&mut Self>) {
+ // SAFETY: trigger is pointing to a live and registered allocation
+ unsafe {
+ bindings::led_trigger_unregister(self.trigger.get());
+ }
+ }
+}
+
+/// Operations for the Hardware trigger
+#[macros::vtable]
+pub trait HardwareOperations: super::Operations {
+ /// Activate the hardware trigger.
+ fn activate(this: &mut Self::This) -> Result;
+ /// Deactivate the hardware trigger.
+ fn deactivate(_this: &mut Self::This) {
+ crate::build_error(crate::error::VTABLE_DEFAULT_ERROR)
+ }
+}
+
+/// `trigger_activate` function pointer
+///
+/// # Safety
+///
+/// `led_cdev` must be passed by the corresponding callback in `led_trigger`.
+unsafe extern "C" fn trigger_activate<T>(led_cdev: *mut bindings::led_classdev) -> i32
+where
+ T: HardwareOperations,
+{
+ from_result(|| {
+ // SAFETY: By the safety requirement of this function `led_cdev` is embedded inside a `T::This<T>`.
+ let led = unsafe { &mut *(T::This::led_container_of(led_cdev.cast())) };
+ T::activate(led)?;
+ Ok(0)
+ })
+}
+
+/// `trigger_deactivate` function pointer
+///
+/// # Safety
+///
+/// `led_cdev` must be passed by the corresponding callback in `led_trigger`.
+unsafe extern "C" fn trigger_deactivate<T>(led_cdev: *mut bindings::led_classdev)
+where
+ T: HardwareOperations,
+{
+ // SAFETY: By the safety requirement of this function `led_cdev` is embedded inside a `T::This<T>`.
+ let led = unsafe { &mut *(T::This::led_container_of(led_cdev.cast())) };
+ T::deactivate(led)
+}
--
2.47.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 4/5] rust: add I/O port abstractions with resource management
2025-01-13 12:16 [PATCH v2 0/5] Rust LED Driver Abstractions and Lenovo SE10 Driver with DMI and I/O Port Support Fiona Behrens
` (2 preceding siblings ...)
2025-01-13 12:16 ` [PATCH v2 3/5] rust: leds: Add hardware trigger support for hardware-controlled LEDs Fiona Behrens
@ 2025-01-13 12:16 ` Fiona Behrens
2025-01-13 13:15 ` Daniel Almeida
2025-01-13 12:16 ` [PATCH v2 5/5] leds: leds_lenovo_se10: LED driver for Lenovo SE10 platform Fiona Behrens
4 siblings, 1 reply; 17+ messages in thread
From: Fiona Behrens @ 2025-01-13 12:16 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Pavel Machek, Lee Jones, Jean Delvare
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Fiona Behrens,
Mark Pearson, Peter Koch, rust-for-linux, linux-leds,
linux-kernel
This patch introduces abstractions for working with I/O port resources,
including the `Resource` and `Region` types. These abstractions facilitate
interaction with `ioport_resource`, enabling safe management of resource
reservations and memory accesses.
Additionally, helper functions such as `outb`, `outw`, and `outl` have been
provided to write values to these resources, with matching read functions
such as `inb`, `inw`, and `inl` for accessing the port memory.
Signed-off-by: Fiona Behrens <me@kloenk.dev>
---
rust/helpers/helpers.c | 1 +
rust/helpers/ioport.c | 42 ++++++++++
rust/kernel/ioport.rs | 169 +++++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 2 +
4 files changed, 214 insertions(+)
create mode 100644 rust/helpers/ioport.c
create mode 100644 rust/kernel/ioport.rs
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index dcf827a61b52..b40aee82fa0f 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -14,6 +14,7 @@
#include "cred.c"
#include "err.c"
#include "fs.c"
+#include "ioport.c"
#include "jump_label.c"
#include "kunit.c"
#include "mutex.c"
diff --git a/rust/helpers/ioport.c b/rust/helpers/ioport.c
new file mode 100644
index 000000000000..d9c9e2093b98
--- /dev/null
+++ b/rust/helpers/ioport.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/ioport.h>
+#include <linux/io.h>
+
+struct resource *rust_helper_request_region(resource_size_t start,
+ resource_size_t n, const char *name)
+{
+ return request_region(start, n, name);
+}
+
+struct resource *rust_helper_request_muxed_region(resource_size_t start,
+ resource_size_t n,
+ const char *name)
+{
+ return request_muxed_region(start, n, name);
+}
+
+void rust_helper_release_region(resource_size_t start, resource_size_t n)
+{
+ release_region(start, n);
+}
+
+#define define_rust_helper_out(name, type) \
+ void rust_helper_##name(type value, unsigned long addr) \
+ { \
+ (name)(value, addr); \
+ }
+
+define_rust_helper_out(outb, u8);
+define_rust_helper_out(outw, u16);
+define_rust_helper_out(outl, u32);
+
+#define define_rust_helper_in(name, type) \
+ type rust_helper_##name(unsigned long addr) \
+ { \
+ return (name)(addr); \
+ }
+
+define_rust_helper_in(inb, u8);
+define_rust_helper_in(inw, u16);
+define_rust_helper_in(inl, u32);
diff --git a/rust/kernel/ioport.rs b/rust/kernel/ioport.rs
new file mode 100644
index 000000000000..9bc342cb4663
--- /dev/null
+++ b/rust/kernel/ioport.rs
@@ -0,0 +1,169 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstractions of routines for detecting, reserving and
+//! allocating system resources.
+//!
+//! C header: [`include/linux/ioport.h`](srctree/include/linux/ioport.h)
+
+use core::ops::Deref;
+use core::ptr;
+
+use crate::prelude::*;
+use crate::types::Opaque;
+
+/// Resource Size type.
+/// This is a type alias to `u64`
+/// depending on the config option `CONFIG_PHYS_ADDR_T_64BIT`.
+#[cfg(CONFIG_PHYS_ADDR_T_64BIT)]
+pub type ResourceSize = u64;
+
+/// Resource Size type.
+/// This is a type alias to `u32`
+/// depending on the config option `CONFIG_PHYS_ADDR_T_64BIT`.
+#[cfg(not(CONFIG_PHYS_ADDR_T_64BIT))]
+pub type ResourceSize = u32;
+
+/// Resource node.
+#[repr(transparent)]
+pub struct Resource(Opaque<bindings::resource>);
+
+impl Resource {
+ /// Convert a raw C `struct resource` pointer to a `&'a Resource`.
+ ///
+ /// # Safety
+ ///
+ /// Callers must ensure that `ptr` is valid, non-null.
+ pub unsafe fn as_ref<'a>(ptr: *mut bindings::resource) -> &'a Self {
+ // SAFETY: Guaranteed by the safety requirements of the function.
+ unsafe { &*ptr.cast() }
+ }
+
+ /// Return raw pointer to the resource.
+ pub fn as_raw(&self) -> *mut bindings::resource {
+ self.0.get()
+ }
+
+ /// Get name of the resource.
+ pub fn name(&self) -> &CStr {
+ // SAFETY: self.get is valid and of type `bindings::resource`
+ let name_ptr = unsafe { ptr::read(ptr::addr_of!((*self.as_raw()).name)) };
+ // SAFETY: `name_ptr` is a valid char pointer from the resource
+ unsafe { CStr::from_char_ptr(name_ptr) }
+ }
+
+ /// Get the start of the [`Resource`].
+ #[inline]
+ pub fn start(&self) -> ResourceSize {
+ // SAFETY: self.get is valid and of type `bindings::resource`
+ unsafe { ptr::read(ptr::addr_of!((*self.as_raw()).start)) }
+ }
+
+ /// Get the end of the [`Resource`].
+ #[inline]
+ pub fn end(&self) -> ResourceSize {
+ // SAFETY: self.get is valid and of type `bindings::resource`
+ unsafe { ptr::read(ptr::addr_of!((*self.as_raw()).start)) }
+ }
+
+ /// Get the length of the Resource.
+ // Empty resource (len = 0) does not make any sense.`
+ #[allow(clippy::len_without_is_empty)]
+ pub fn len(&self) -> ResourceSize {
+ self.end() - self.start() + 1
+ }
+}
+
+/// Requested region using `request_region`. This will release the region when dropped.
+///
+/// This uses port memory from the `ioport_resource` parent resource.
+pub struct Region<'name>(&'name Resource);
+
+impl<'name> Region<'name> {
+ /// Request a new muxed region from the `ioport_resource` region.
+ #[inline]
+ pub fn request_muxed(start: ResourceSize, n: ResourceSize, name: &'name CStr) -> Option<Self> {
+ // SAFETY: C ffi call. `name` is valid for lifetime of `Self`
+ unsafe { Self::from_resource(bindings::request_muxed_region(start, n, name.as_char_ptr())) }
+ }
+
+ /// Request a new region from the `ioport_resource` region.
+ #[inline]
+ pub fn request(start: ResourceSize, n: ResourceSize, name: &'name CStr) -> Option<Self> {
+ // SAFETY: C ffi call. `name` is valid for lifetime of `Self`
+ unsafe { Self::from_resource(bindings::request_region(start, n, name.as_char_ptr())) }
+ }
+
+ /// Get a resource pointer and return a resource if the pointer is non-null.
+ ///
+ /// Helper for `request` and `request_muxed`.
+ ///
+ /// # Safety
+ ///
+ /// `resource` has to be a valid or null pointer to a resource.
+ unsafe fn from_resource(resource: *mut bindings::resource) -> Option<Self> {
+ if resource.is_null() {
+ None
+ } else {
+ // SAFETY: resource is a valid resource by the function requirements and non-null.
+ Some(Self(unsafe { Resource::as_ref(resource) }))
+ }
+ }
+}
+
+macro_rules! define_out {
+ ($name:ident => $type:ty) => {
+ #[doc = concat!("Write [`", stringify!($type), "`] value into port memory region at the given offset.")]
+ pub fn $name(&self, value: $type, offset: ResourceSize) {
+ let address = self.start() + offset;
+ debug_assert!((address + (core::mem::size_of::<$type>() as ResourceSize)) <= (self.end() + 1));
+ // SAFETY: ffi call, address is in the region
+ unsafe { bindings::$name(value, address) };
+ }
+ };
+ ($($name:ident => $type:ty;)*) => {
+ $(define_out! { $name => $type })*
+ };
+}
+
+macro_rules! define_in {
+ ($name:ident => $type:ty) => {
+ #[doc = concat!("Read [`", stringify!($type), "`] value from port memory region at the given offset.")]
+ pub fn $name(&self, offset: ResourceSize) -> $type {
+ let address = self.start() + offset;
+ debug_assert!((address + (core::mem::size_of::<$type>() as ResourceSize)) <= (self.end() + 1));
+ // SAFETY: ffi call, address is in the region
+ unsafe { bindings::$name(address) }
+ }
+ };
+ ($($name:ident => $type:ty;)*) => {
+ $(define_in! { $name => $type })*
+ };
+}
+
+impl Region<'_> {
+ define_out! {
+ outb => u8;
+ outw => u16;
+ outl => u32;
+ }
+ define_in! {
+ inb => u8;
+ inw => u16;
+ inl => u32;
+ }
+}
+
+impl Drop for Region<'_> {
+ fn drop(&mut self) {
+ // SAFETY: ffi call, resource is valid.
+ unsafe { bindings::release_region(self.start(), self.len()) };
+ }
+}
+
+impl Deref for Region<'_> {
+ type Target = Resource;
+
+ fn deref(&self) -> &Self::Target {
+ self.0
+ }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 8895a1683f82..99cd706c40e7 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -43,6 +43,8 @@
pub mod fs;
pub mod init;
pub mod ioctl;
+#[cfg(CONFIG_HAS_IOPORT)]
+pub mod ioport;
pub mod jump_label;
#[cfg(CONFIG_KUNIT)]
pub mod kunit;
--
2.47.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 5/5] leds: leds_lenovo_se10: LED driver for Lenovo SE10 platform
2025-01-13 12:16 [PATCH v2 0/5] Rust LED Driver Abstractions and Lenovo SE10 Driver with DMI and I/O Port Support Fiona Behrens
` (3 preceding siblings ...)
2025-01-13 12:16 ` [PATCH v2 4/5] rust: add I/O port abstractions with resource management Fiona Behrens
@ 2025-01-13 12:16 ` Fiona Behrens
2025-01-17 17:21 ` Mark Pearson
2025-01-20 10:47 ` Marek Behún
4 siblings, 2 replies; 17+ messages in thread
From: Fiona Behrens @ 2025-01-13 12:16 UTC (permalink / raw)
To: Miguel Ojeda, Alex Gaynor, Pavel Machek, Lee Jones, Jean Delvare
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Fiona Behrens,
Mark Pearson, Peter Koch, rust-for-linux, linux-leds,
linux-kernel
Add driver for the Lenovo ThinkEdge SE10 LED.
This driver supports controlling the red LED located on the front panel of the
Lenovo SE10 hardware. Additionally, it supports the hardware-triggered
functionality of the LED, which by default is tied to the WWAN trigger.
The driver is written in Rust and adds basic LED support for the SE10 platform.
Signed-off-by: Fiona Behrens <me@kloenk.dev>
---
drivers/leds/Kconfig | 10 +++
drivers/leds/Makefile | 1 +
drivers/leds/leds_lenovo_se10.rs | 132 +++++++++++++++++++++++++++++++
3 files changed, 143 insertions(+)
create mode 100644 drivers/leds/leds_lenovo_se10.rs
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b784bb74a837..89d9e98189d6 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -223,6 +223,16 @@ config LEDS_TURRIS_OMNIA
side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the
front panel.
+config LEDS_LENOVO_SE10
+ tristate "LED support for Lenovo ThinkEdge SE10"
+ depends on RUST
+ depends on (X86 && DMI) || COMPILE_TEST
+ depends on HAS_IOPORT
+ imply LEDS_TRIGGERS
+ help
+ This option enables basic support for the LED found on the front of
+ Lenovo's SE10 ThinkEdge. There is one user controlable LED on the front panel.
+
config LEDS_LM3530
tristate "LCD Backlight driver for LM3530"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 18afbb5a23ee..2cff22cbafcf 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -37,6 +37,7 @@ obj-$(CONFIG_LEDS_IP30) += leds-ip30.o
obj-$(CONFIG_LEDS_IPAQ_MICRO) += leds-ipaq-micro.o
obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o
obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o
+obj-$(CONFIG_LEDS_LENOVO_SE10) += leds_lenovo_se10.o
obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o
obj-$(CONFIG_LEDS_LM3532) += leds-lm3532.o
obj-$(CONFIG_LEDS_LM3533) += leds-lm3533.o
diff --git a/drivers/leds/leds_lenovo_se10.rs b/drivers/leds/leds_lenovo_se10.rs
new file mode 100644
index 000000000000..d704125610a4
--- /dev/null
+++ b/drivers/leds/leds_lenovo_se10.rs
@@ -0,0 +1,132 @@
+// SPDX-License-Identifier: GPL-2.0
+//! LED driver for Lenovo ThinkEdge SE10.
+
+use kernel::ioport::{Region, ResourceSize};
+#[cfg(CONFIG_LEDS_TRIGGERS)]
+use kernel::leds::triggers;
+use kernel::leds::{Led, LedConfig, Operations};
+use kernel::prelude::*;
+use kernel::time::Delta;
+use kernel::{c_str, dmi_device_table};
+
+module! {
+ type: SE10,
+ name: "leds_lenovo_se10",
+ author: "Fiona Behrens <me@kloenk.dev>",
+ description: "LED driver for Lenovo ThinkEdge SE10",
+ license: "GPL",
+}
+
+dmi_device_table!(5, SE10_DMI_TABLE, [
+ "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NH"],
+ "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NJ"],
+ "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NK"],
+ "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NL"],
+ "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NM"],
+]);
+
+struct SE10 {
+ /// Led registration
+ _led: Pin<KBox<Led<LedSE10>>>,
+}
+
+impl kernel::Module for SE10 {
+ fn init(_module: &'static ThisModule) -> Result<Self> {
+ if SE10_DMI_TABLE.check_system().is_none() {
+ return Err(ENODEV);
+ }
+
+ let led = KBox::try_pin_init(
+ Led::register(
+ None,
+ LedConfig {
+ name: Some(c_str!("platform:red:user")),
+ #[cfg(CONFIG_LEDS_TRIGGERS)]
+ hardware_trigger: Some(kernel::sync::Arc::pin_init(
+ triggers::Hardware::register(c_str!("wwan")),
+ GFP_KERNEL,
+ )?),
+ ..LedConfig::new(kernel::leds::Color::Red, LedSE10)
+ },
+ ),
+ GFP_KERNEL,
+ )?;
+
+ Ok(Self { _led: led })
+ }
+}
+
+/// Valid led commands.
+#[repr(u8)]
+#[allow(missing_docs)]
+enum LedCommand {
+ #[cfg(CONFIG_LEDS_TRIGGERS)]
+ Trigger = 0xB2,
+ Off = 0xB3,
+ On = 0xB4,
+ Blink = 0xB5,
+}
+
+struct LedSE10;
+
+impl LedSE10 {
+ /// Base address of the command port.
+ const CMD_PORT: ResourceSize = 0x6C;
+ /// Length of the command port.
+ const CMD_LEN: ResourceSize = 1;
+ /// Blink duration the hardware supports.
+ const HW_DURATION: Delta = Delta::from_millis(1000);
+
+ /// Request led region.
+ fn request_cmd_region(&self) -> Result<Region<'static>> {
+ Region::request_muxed(Self::CMD_PORT, Self::CMD_LEN, c_str!("leds_lenovo_se10"))
+ .ok_or(EBUSY)
+ }
+
+ /// Send command.
+ fn send_cmd(&self, cmd: LedCommand) -> Result {
+ let region = self.request_cmd_region()?;
+ region.outb(cmd as u8, 0);
+ Ok(())
+ }
+}
+
+#[vtable]
+impl Operations for LedSE10 {
+ type This = Led<LedSE10>;
+
+ const MAX_BRIGHTNESS: u8 = 1;
+
+ fn brightness_set(this: &mut Self::This, brightness: u8) {
+ if let Err(e) = if brightness == 0 {
+ this.data.send_cmd(LedCommand::Off)
+ } else {
+ this.data.send_cmd(LedCommand::On)
+ } {
+ pr_warn!("Failed to set led: {e:?}\n)")
+ }
+ }
+
+ fn blink_set(
+ this: &mut Self::This,
+ delay_on: Delta,
+ delay_off: Delta,
+ ) -> Result<(Delta, Delta)> {
+ if !(delay_on.is_zero() && delay_off.is_zero()
+ || delay_on == Self::HW_DURATION && delay_off == Self::HW_DURATION)
+ {
+ return Err(EINVAL);
+ }
+
+ this.data.send_cmd(LedCommand::Blink)?;
+ Ok((Self::HW_DURATION, Self::HW_DURATION))
+ }
+}
+
+#[vtable]
+#[cfg(CONFIG_LEDS_TRIGGERS)]
+impl triggers::HardwareOperations for LedSE10 {
+ fn activate(this: &mut Self::This) -> Result {
+ this.data.send_cmd(LedCommand::Trigger)
+ }
+}
--
2.47.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/5] rust: leds: Add Rust bindings for LED subsystem
2025-01-13 12:16 ` [PATCH v2 2/5] rust: leds: Add Rust bindings for LED subsystem Fiona Behrens
@ 2025-01-13 13:10 ` Miguel Ojeda
0 siblings, 0 replies; 17+ messages in thread
From: Miguel Ojeda @ 2025-01-13 13:10 UTC (permalink / raw)
To: Fiona Behrens
Cc: Miguel Ojeda, Alex Gaynor, Pavel Machek, Lee Jones, Jean Delvare,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Mark Pearson,
Peter Koch, rust-for-linux, linux-leds, linux-kernel
Hi Fiona,
Thanks for this! I noticed a procedural thing (the last diff chunk,
please see below), so I took the chance to give a few quick
surface-level comments I noticed while scrolling, mostly on docs :)
On Mon, Jan 13, 2025 at 1:16 PM Fiona Behrens <me@kloenk.dev> wrote:
>
> + /// Get String representation of the Color.
[`String`], [`Color`]
i.e. please format, and use intra-doc links where possible/reasonable.
> + // SAFETY: pointer from above, valid for static lifetime.
In general, try to explain why, not just what, in the safety comments,
i.e. why it is valid for a static lifetime? e.g. does the C API
guarantee it?
> + /// Get String representation of the Color as rust type.
> + #[inline]
> + pub fn name(&self) -> Option<&'static str> {
"Rust type"
However, I am not sure what it is trying to say. I would have thought
it is a custom newtype of something or perhaps something strange is
going on, but I guess you are referring to `str` vs. `CStr`? In
general, I don't think we need to say "as a Rust type" -- it may
confuse more than help.
> + // SAFETY: name from C name array which is valid UTF-8.
What guarantees this? i.e. I imagine you looked into all the cases
from the static table, so I would for instance say: "All values in the
C name array are valid UTF-8." or similar (perhaps mention which
array, so that people can e.g. grep).
> +impl core::fmt::Display for Color {
> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> + f.write_str(self.name().unwrap_or("Invalid color"))
> + }
> +}
Can this happen? If not, should `Color` have an `# Invariant` and
would we need the `Option<...>` for names then?
In any case, here, shouldn't the error be bubbled up?
> + /// Tris to convert a u32 into a [`Color`].
Typo: Tries
Format: [`u32`]
There are also other typos ("brightnes", "Activae") -- I recommend
`checkpatch.pl` with the `--codespell` flag which may help catching
some of these.
> + /// Get the LED brightness level.
> + fn brightness_get(_this: &mut Self::This) -> u8 {
> + crate::build_error(crate::error::VTABLE_DEFAULT_ERROR)
> + }
Please use the macro instead (see 15f2f9313a39 ("rust: use the
`build_error!` macro, not the hidden function")). Soon we will have it
in the prelude too (see 4401565fe92b ("rust: add `build_error!` to the
prelude")), by the way.
> + // SAFETY: By the safety requirement of this function `delay_on` is pointing to a valid `c_ulong`.
> + let on = Delta::from_millis(unsafe { *delay_on } as i64);
I didn't look into most comments, but e.g. I noticed this one does not
look correct -- the safety requirements for this function do not
mention anything about `delay_on`, no?
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 38da79925586..3c348ce4a7c2 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -143,4 +143,11 @@ pub fn as_nanos(self) -> i64 {
> pub fn as_micros_ceil(self) -> i64 {
> self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
> }
> +
> + /// Return the smallest number of milliseconds greater than or equal
> + /// to the value in the `Delta`.
> + #[inline]
> + pub fn as_millis_ceil(self) -> i64 {
> + self.as_nanos().saturating_add(NSEC_PER_MSEC - 1) / NSEC_PER_MSEC
> + }
This should probably be its own patch, with Cc to the timekeeping
maintainers etc.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/5] rust: add I/O port abstractions with resource management
2025-01-13 12:16 ` [PATCH v2 4/5] rust: add I/O port abstractions with resource management Fiona Behrens
@ 2025-01-13 13:15 ` Daniel Almeida
2025-01-13 13:28 ` Fiona Behrens
0 siblings, 1 reply; 17+ messages in thread
From: Daniel Almeida @ 2025-01-13 13:15 UTC (permalink / raw)
To: Fiona Behrens
Cc: Miguel Ojeda, Alex Gaynor, Pavel Machek, Lee Jones, Jean Delvare,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Mark Pearson,
Peter Koch, rust-for-linux, linux-leds, linux-kernel
Hi Fiona,
> On 13 Jan 2025, at 09:16, Fiona Behrens <me@kloenk.dev> wrote:
>
> This patch introduces abstractions for working with I/O port resources,
> including the `Resource` and `Region` types. These abstractions facilitate
> interaction with `ioport_resource`, enabling safe management of resource
> reservations and memory accesses.
>
> Additionally, helper functions such as `outb`, `outw`, and `outl` have been
> provided to write values to these resources, with matching read functions
> such as `inb`, `inw`, and `inl` for accessing the port memory.
>
> Signed-off-by: Fiona Behrens <me@kloenk.dev>
> ---
> rust/helpers/helpers.c | 1 +
> rust/helpers/ioport.c | 42 ++++++++++
> rust/kernel/ioport.rs | 169 +++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 2 +
> 4 files changed, 214 insertions(+)
> create mode 100644 rust/helpers/ioport.c
> create mode 100644 rust/kernel/ioport.rs
>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index dcf827a61b52..b40aee82fa0f 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -14,6 +14,7 @@
> #include "cred.c"
> #include "err.c"
> #include "fs.c"
> +#include "ioport.c"
> #include "jump_label.c"
> #include "kunit.c"
> #include "mutex.c"
> diff --git a/rust/helpers/ioport.c b/rust/helpers/ioport.c
> new file mode 100644
> index 000000000000..d9c9e2093b98
> --- /dev/null
> +++ b/rust/helpers/ioport.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/ioport.h>
> +#include <linux/io.h>
> +
> +struct resource *rust_helper_request_region(resource_size_t start,
> + resource_size_t n, const char *name)
> +{
> + return request_region(start, n, name);
> +}
> +
> +struct resource *rust_helper_request_muxed_region(resource_size_t start,
> + resource_size_t n,
> + const char *name)
> +{
> + return request_muxed_region(start, n, name);
> +}
> +
> +void rust_helper_release_region(resource_size_t start, resource_size_t n)
> +{
> + release_region(start, n);
> +}
> +
> +#define define_rust_helper_out(name, type) \
> + void rust_helper_##name(type value, unsigned long addr) \
> + { \
> + (name)(value, addr); \
> + }
> +
> +define_rust_helper_out(outb, u8);
> +define_rust_helper_out(outw, u16);
> +define_rust_helper_out(outl, u32);
> +
> +#define define_rust_helper_in(name, type) \
> + type rust_helper_##name(unsigned long addr) \
> + { \
> + return (name)(addr); \
> + }
> +
> +define_rust_helper_in(inb, u8);
> +define_rust_helper_in(inw, u16);
> +define_rust_helper_in(inl, u32);
> diff --git a/rust/kernel/ioport.rs b/rust/kernel/ioport.rs
> new file mode 100644
> index 000000000000..9bc342cb4663
> --- /dev/null
> +++ b/rust/kernel/ioport.rs
> @@ -0,0 +1,169 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Abstractions of routines for detecting, reserving and
> +//! allocating system resources.
> +//!
> +//! C header: [`include/linux/ioport.h`](srctree/include/linux/ioport.h)
> +
> +use core::ops::Deref;
> +use core::ptr;
> +
> +use crate::prelude::*;
> +use crate::types::Opaque;
> +
> +/// Resource Size type.
> +/// This is a type alias to `u64`
> +/// depending on the config option `CONFIG_PHYS_ADDR_T_64BIT`.
> +#[cfg(CONFIG_PHYS_ADDR_T_64BIT)]
> +pub type ResourceSize = u64;
> +
> +/// Resource Size type.
> +/// This is a type alias to `u32`
> +/// depending on the config option `CONFIG_PHYS_ADDR_T_64BIT`.
> +#[cfg(not(CONFIG_PHYS_ADDR_T_64BIT))]
> +pub type ResourceSize = u32;
> +
> +/// Resource node.
> +#[repr(transparent)]
> +pub struct Resource(Opaque<bindings::resource>);
This is a conflict with [0]
[0]: https://lore.kernel.org/rust-for-linux/20250109133057.243751-1-daniel.almeida@collabora.com/
— Daniel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/5] rust: add I/O port abstractions with resource management
2025-01-13 13:15 ` Daniel Almeida
@ 2025-01-13 13:28 ` Fiona Behrens
0 siblings, 0 replies; 17+ messages in thread
From: Fiona Behrens @ 2025-01-13 13:28 UTC (permalink / raw)
To: Daniel Almeida
Cc: Miguel Ojeda, Alex Gaynor, Pavel Machek, Lee Jones, Jean Delvare,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Mark Pearson,
Peter Koch, rust-for-linux, linux-leds, linux-kernel
Hi,
On 13 Jan 2025, at 14:15, Daniel Almeida wrote:
> Hi Fiona,
>
>> On 13 Jan 2025, at 09:16, Fiona Behrens <me@kloenk.dev> wrote:
>>
>> This patch introduces abstractions for working with I/O port resources,
>> including the `Resource` and `Region` types. These abstractions facilitate
>> interaction with `ioport_resource`, enabling safe management of resource
>> reservations and memory accesses.
>>
>> Additionally, helper functions such as `outb`, `outw`, and `outl` have been
>> provided to write values to these resources, with matching read functions
>> such as `inb`, `inw`, and `inl` for accessing the port memory.
>>
>> Signed-off-by: Fiona Behrens <me@kloenk.dev>
>> ---
>> rust/helpers/helpers.c | 1 +
>> rust/helpers/ioport.c | 42 ++++++++++
>> rust/kernel/ioport.rs | 169 +++++++++++++++++++++++++++++++++++++++++
>> rust/kernel/lib.rs | 2 +
>> 4 files changed, 214 insertions(+)
>> create mode 100644 rust/helpers/ioport.c
>> create mode 100644 rust/kernel/ioport.rs
>>
>> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
>> index dcf827a61b52..b40aee82fa0f 100644
>> --- a/rust/helpers/helpers.c
>> +++ b/rust/helpers/helpers.c
>> @@ -14,6 +14,7 @@
>> #include "cred.c"
>> #include "err.c"
>> #include "fs.c"
>> +#include "ioport.c"
>> #include "jump_label.c"
>> #include "kunit.c"
>> #include "mutex.c"
>> diff --git a/rust/helpers/ioport.c b/rust/helpers/ioport.c
>> new file mode 100644
>> index 000000000000..d9c9e2093b98
>> --- /dev/null
>> +++ b/rust/helpers/ioport.c
>> @@ -0,0 +1,42 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/ioport.h>
>> +#include <linux/io.h>
>> +
>> +struct resource *rust_helper_request_region(resource_size_t start,
>> + resource_size_t n, const char *name)
>> +{
>> + return request_region(start, n, name);
>> +}
>> +
>> +struct resource *rust_helper_request_muxed_region(resource_size_t start,
>> + resource_size_t n,
>> + const char *name)
>> +{
>> + return request_muxed_region(start, n, name);
>> +}
>> +
>> +void rust_helper_release_region(resource_size_t start, resource_size_t n)
>> +{
>> + release_region(start, n);
>> +}
>> +
>> +#define define_rust_helper_out(name, type) \
>> + void rust_helper_##name(type value, unsigned long addr) \
>> + { \
>> + (name)(value, addr); \
>> + }
>> +
>> +define_rust_helper_out(outb, u8);
>> +define_rust_helper_out(outw, u16);
>> +define_rust_helper_out(outl, u32);
>> +
>> +#define define_rust_helper_in(name, type) \
>> + type rust_helper_##name(unsigned long addr) \
>> + { \
>> + return (name)(addr); \
>> + }
>> +
>> +define_rust_helper_in(inb, u8);
>> +define_rust_helper_in(inw, u16);
>> +define_rust_helper_in(inl, u32);
>> diff --git a/rust/kernel/ioport.rs b/rust/kernel/ioport.rs
>> new file mode 100644
>> index 000000000000..9bc342cb4663
>> --- /dev/null
>> +++ b/rust/kernel/ioport.rs
>> @@ -0,0 +1,169 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Abstractions of routines for detecting, reserving and
>> +//! allocating system resources.
>> +//!
>> +//! C header: [`include/linux/ioport.h`](srctree/include/linux/ioport.h)
>> +
>> +use core::ops::Deref;
>> +use core::ptr;
>> +
>> +use crate::prelude::*;
>> +use crate::types::Opaque;
>> +
>> +/// Resource Size type.
>> +/// This is a type alias to `u64`
>> +/// depending on the config option `CONFIG_PHYS_ADDR_T_64BIT`.
>> +#[cfg(CONFIG_PHYS_ADDR_T_64BIT)]
>> +pub type ResourceSize = u64;
>> +
>> +/// Resource Size type.
>> +/// This is a type alias to `u32`
>> +/// depending on the config option `CONFIG_PHYS_ADDR_T_64BIT`.
>> +#[cfg(not(CONFIG_PHYS_ADDR_T_64BIT))]
>> +pub type ResourceSize = u32;
>> +
>> +/// Resource node.
>> +#[repr(transparent)]
>> +pub struct Resource(Opaque<bindings::resource>);
>
> This is a conflict with [0]
Ah missed that, only saw the IO abstraction in the same series but it did not work for my use case, and then somehow missed this, will rework to use that as base in the next version
Thanks
Fiona
>
> [0]: https://lore.kernel.org/rust-for-linux/20250109133057.243751-1-daniel.almeida@collabora.com/
>
> — Daniel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/5] leds: leds_lenovo_se10: LED driver for Lenovo SE10 platform
2025-01-13 12:16 ` [PATCH v2 5/5] leds: leds_lenovo_se10: LED driver for Lenovo SE10 platform Fiona Behrens
@ 2025-01-17 17:21 ` Mark Pearson
2025-01-17 17:31 ` Fiona Behrens
2025-01-17 17:43 ` Miguel Ojeda
2025-01-20 10:47 ` Marek Behún
1 sibling, 2 replies; 17+ messages in thread
From: Mark Pearson @ 2025-01-17 17:21 UTC (permalink / raw)
To: Fiona Behrens, Miguel Ojeda, Alex Gaynor, Pavel Machek, Lee Jones,
Jean Delvare
Cc: Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Peter Koch,
rust-for-linux, linux-leds, linux-kernel
Hi Fiona,
On Mon, Jan 13, 2025, at 7:16 AM, Fiona Behrens wrote:
> Add driver for the Lenovo ThinkEdge SE10 LED.
>
> This driver supports controlling the red LED located on the front panel of the
> Lenovo SE10 hardware. Additionally, it supports the hardware-triggered
> functionality of the LED, which by default is tied to the WWAN trigger.
>
> The driver is written in Rust and adds basic LED support for the SE10 platform.
>
> Signed-off-by: Fiona Behrens <me@kloenk.dev>
> ---
> drivers/leds/Kconfig | 10 +++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds_lenovo_se10.rs | 132 +++++++++++++++++++++++++++++++
All the other files are called leds-<name>. Should this be leds-lenovo-se10.rs?
> 3 files changed, 143 insertions(+)
> create mode 100644 drivers/leds/leds_lenovo_se10.rs
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index b784bb74a837..89d9e98189d6 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -223,6 +223,16 @@ config LEDS_TURRIS_OMNIA
> side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the
> front panel.
>
> +config LEDS_LENOVO_SE10
> + tristate "LED support for Lenovo ThinkEdge SE10"
> + depends on RUST
> + depends on (X86 && DMI) || COMPILE_TEST
> + depends on HAS_IOPORT
> + imply LEDS_TRIGGERS
> + help
> + This option enables basic support for the LED found on the front of
> + Lenovo's SE10 ThinkEdge. There is one user controlable LED on the
> front panel.
> +
> config LEDS_LM3530
> tristate "LCD Backlight driver for LM3530"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 18afbb5a23ee..2cff22cbafcf 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_LEDS_IP30) += leds-ip30.o
> obj-$(CONFIG_LEDS_IPAQ_MICRO) += leds-ipaq-micro.o
> obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o
> obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o
> +obj-$(CONFIG_LEDS_LENOVO_SE10) += leds_lenovo_se10.o
Same note above on name formatting.
> obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o
> obj-$(CONFIG_LEDS_LM3532) += leds-lm3532.o
> obj-$(CONFIG_LEDS_LM3533) += leds-lm3533.o
> diff --git a/drivers/leds/leds_lenovo_se10.rs
> b/drivers/leds/leds_lenovo_se10.rs
> new file mode 100644
> index 000000000000..d704125610a4
> --- /dev/null
> +++ b/drivers/leds/leds_lenovo_se10.rs
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//! LED driver for Lenovo ThinkEdge SE10.
> +
> +use kernel::ioport::{Region, ResourceSize};
> +#[cfg(CONFIG_LEDS_TRIGGERS)]
> +use kernel::leds::triggers;
> +use kernel::leds::{Led, LedConfig, Operations};
> +use kernel::prelude::*;
> +use kernel::time::Delta;
> +use kernel::{c_str, dmi_device_table};
> +
> +module! {
> + type: SE10,
> + name: "leds_lenovo_se10",
> + author: "Fiona Behrens <me@kloenk.dev>",
> + description: "LED driver for Lenovo ThinkEdge SE10",
> + license: "GPL",
> +}
> +
> +dmi_device_table!(5, SE10_DMI_TABLE, [
> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NH"],
> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NJ"],
> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NK"],
> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NL"],
> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NM"],
> +]);
> +
> +struct SE10 {
> + /// Led registration
> + _led: Pin<KBox<Led<LedSE10>>>,
> +}
> +
> +impl kernel::Module for SE10 {
> + fn init(_module: &'static ThisModule) -> Result<Self> {
> + if SE10_DMI_TABLE.check_system().is_none() {
> + return Err(ENODEV);
> + }
> +
> + let led = KBox::try_pin_init(
> + Led::register(
> + None,
> + LedConfig {
> + name: Some(c_str!("platform:red:user")),
> + #[cfg(CONFIG_LEDS_TRIGGERS)]
> + hardware_trigger: Some(kernel::sync::Arc::pin_init(
> + triggers::Hardware::register(c_str!("wwan")),
I was curious as to why the "wwan" in here.
> + GFP_KERNEL,
> + )?),
> + ..LedConfig::new(kernel::leds::Color::Red, LedSE10)
> + },
> + ),
> + GFP_KERNEL,
> + )?;
> +
> + Ok(Self { _led: led })
> + }
> +}
> +
> +/// Valid led commands.
> +#[repr(u8)]
> +#[allow(missing_docs)]
> +enum LedCommand {
> + #[cfg(CONFIG_LEDS_TRIGGERS)]
> + Trigger = 0xB2,
> + Off = 0xB3,
> + On = 0xB4,
> + Blink = 0xB5,
> +}
> +
> +struct LedSE10;
> +
> +impl LedSE10 {
> + /// Base address of the command port.
> + const CMD_PORT: ResourceSize = 0x6C;
> + /// Length of the command port.
> + const CMD_LEN: ResourceSize = 1;
> + /// Blink duration the hardware supports.
> + const HW_DURATION: Delta = Delta::from_millis(1000);
> +
> + /// Request led region.
> + fn request_cmd_region(&self) -> Result<Region<'static>> {
> + Region::request_muxed(Self::CMD_PORT, Self::CMD_LEN,
> c_str!("leds_lenovo_se10"))
> + .ok_or(EBUSY)
> + }
> +
> + /// Send command.
> + fn send_cmd(&self, cmd: LedCommand) -> Result {
> + let region = self.request_cmd_region()?;
> + region.outb(cmd as u8, 0);
> + Ok(())
> + }
> +}
> +
> +#[vtable]
> +impl Operations for LedSE10 {
> + type This = Led<LedSE10>;
> +
> + const MAX_BRIGHTNESS: u8 = 1;
> +
> + fn brightness_set(this: &mut Self::This, brightness: u8) {
> + if let Err(e) = if brightness == 0 {
> + this.data.send_cmd(LedCommand::Off)
> + } else {
> + this.data.send_cmd(LedCommand::On)
> + } {
> + pr_warn!("Failed to set led: {e:?}\n)")
> + }
> + }
> +
> + fn blink_set(
> + this: &mut Self::This,
> + delay_on: Delta,
> + delay_off: Delta,
> + ) -> Result<(Delta, Delta)> {
> + if !(delay_on.is_zero() && delay_off.is_zero()
> + || delay_on == Self::HW_DURATION && delay_off ==
> Self::HW_DURATION)
> + {
> + return Err(EINVAL);
> + }
> +
> + this.data.send_cmd(LedCommand::Blink)?;
> + Ok((Self::HW_DURATION, Self::HW_DURATION))
> + }
> +}
> +
> +#[vtable]
> +#[cfg(CONFIG_LEDS_TRIGGERS)]
> +impl triggers::HardwareOperations for LedSE10 {
> + fn activate(this: &mut Self::This) -> Result {
> + this.data.send_cmd(LedCommand::Trigger)
> + }
> +}
> --
> 2.47.0
I don't have the competence to review the rust code I'm afraid - so my limited feedback above is the best I can do. Not sure it's really worth a reviewed-by tag, but I did read the code and learnt a little about rust in the process (which was fun).
I did test your changes on my SE10 system and it works well.
Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Thanks!
Mark
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/5] leds: leds_lenovo_se10: LED driver for Lenovo SE10 platform
2025-01-17 17:21 ` Mark Pearson
@ 2025-01-17 17:31 ` Fiona Behrens
2025-01-17 18:02 ` Mark Pearson
2025-01-17 17:43 ` Miguel Ojeda
1 sibling, 1 reply; 17+ messages in thread
From: Fiona Behrens @ 2025-01-17 17:31 UTC (permalink / raw)
To: Mark Pearson, Miguel Ojeda
Cc: Alex Gaynor, Pavel Machek, Lee Jones, Jean Delvare, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Peter Koch, rust-for-linux, linux-leds,
linux-kernel
Hi,
On 17 Jan 2025, at 18:21, Mark Pearson wrote:
> Hi Fiona,
>
> On Mon, Jan 13, 2025, at 7:16 AM, Fiona Behrens wrote:
>> Add driver for the Lenovo ThinkEdge SE10 LED.
>>
>> This driver supports controlling the red LED located on the front panel of the
>> Lenovo SE10 hardware. Additionally, it supports the hardware-triggered
>> functionality of the LED, which by default is tied to the WWAN trigger.
>>
>> The driver is written in Rust and adds basic LED support for the SE10 platform.
>>
>> Signed-off-by: Fiona Behrens <me@kloenk.dev>
>> ---
>> drivers/leds/Kconfig | 10 +++
>> drivers/leds/Makefile | 1 +
>> drivers/leds/leds_lenovo_se10.rs | 132 +++++++++++++++++++++++++++++++
>
> All the other files are called leds-<name>. Should this be leds-lenovo-se10.rs?
This does not work with rust, as the rust makefile converts this filename to a rust crate name, and those crate name cannot have dashes in them.
Not sure if we should fix this to hold to the file name conventions, maybe something for @Miguel to decide
>
>> 3 files changed, 143 insertions(+)
>> create mode 100644 drivers/leds/leds_lenovo_se10.rs
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index b784bb74a837..89d9e98189d6 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -223,6 +223,16 @@ config LEDS_TURRIS_OMNIA
>> side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the
>> front panel.
>>
>> +config LEDS_LENOVO_SE10
>> + tristate "LED support for Lenovo ThinkEdge SE10"
>> + depends on RUST
>> + depends on (X86 && DMI) || COMPILE_TEST
>> + depends on HAS_IOPORT
>> + imply LEDS_TRIGGERS
>> + help
>> + This option enables basic support for the LED found on the front of
>> + Lenovo's SE10 ThinkEdge. There is one user controlable LED on the
>> front panel.
>> +
>> config LEDS_LM3530
>> tristate "LCD Backlight driver for LM3530"
>> depends on LEDS_CLASS
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index 18afbb5a23ee..2cff22cbafcf 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -37,6 +37,7 @@ obj-$(CONFIG_LEDS_IP30) += leds-ip30.o
>> obj-$(CONFIG_LEDS_IPAQ_MICRO) += leds-ipaq-micro.o
>> obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o
>> obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o
>> +obj-$(CONFIG_LEDS_LENOVO_SE10) += leds_lenovo_se10.o
>
> Same note above on name formatting.
>
>> obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o
>> obj-$(CONFIG_LEDS_LM3532) += leds-lm3532.o
>> obj-$(CONFIG_LEDS_LM3533) += leds-lm3533.o
>> diff --git a/drivers/leds/leds_lenovo_se10.rs
>> b/drivers/leds/leds_lenovo_se10.rs
>> new file mode 100644
>> index 000000000000..d704125610a4
>> --- /dev/null
>> +++ b/drivers/leds/leds_lenovo_se10.rs
>> @@ -0,0 +1,132 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +//! LED driver for Lenovo ThinkEdge SE10.
>> +
>> +use kernel::ioport::{Region, ResourceSize};
>> +#[cfg(CONFIG_LEDS_TRIGGERS)]
>> +use kernel::leds::triggers;
>> +use kernel::leds::{Led, LedConfig, Operations};
>> +use kernel::prelude::*;
>> +use kernel::time::Delta;
>> +use kernel::{c_str, dmi_device_table};
>> +
>> +module! {
>> + type: SE10,
>> + name: "leds_lenovo_se10",
>> + author: "Fiona Behrens <me@kloenk.dev>",
>> + description: "LED driver for Lenovo ThinkEdge SE10",
>> + license: "GPL",
>> +}
>> +
>> +dmi_device_table!(5, SE10_DMI_TABLE, [
>> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NH"],
>> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NJ"],
>> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NK"],
>> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NL"],
>> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NM"],
>> +]);
>> +
>> +struct SE10 {
>> + /// Led registration
>> + _led: Pin<KBox<Led<LedSE10>>>,
>> +}
>> +
>> +impl kernel::Module for SE10 {
>> + fn init(_module: &'static ThisModule) -> Result<Self> {
>> + if SE10_DMI_TABLE.check_system().is_none() {
>> + return Err(ENODEV);
>> + }
>> +
>> + let led = KBox::try_pin_init(
>> + Led::register(
>> + None,
>> + LedConfig {
>> + name: Some(c_str!("platform:red:user")),
>> + #[cfg(CONFIG_LEDS_TRIGGERS)]
>> + hardware_trigger: Some(kernel::sync::Arc::pin_init(
>> + triggers::Hardware::register(c_str!("wwan")),
>
> I was curious as to why the "wwan" in here.
This is the hardware trigger, as to the documentation I found from Lenovo the trigger mode gives hardware control to the wwan module if installed in the hardware.
>
>> + GFP_KERNEL,
>> + )?),
>> + ..LedConfig::new(kernel::leds::Color::Red, LedSE10)
>> + },
>> + ),
>> + GFP_KERNEL,
>> + )?;
>> +
>> + Ok(Self { _led: led })
>> + }
>> +}
>> +
>> +/// Valid led commands.
>> +#[repr(u8)]
>> +#[allow(missing_docs)]
>> +enum LedCommand {
>> + #[cfg(CONFIG_LEDS_TRIGGERS)]
>> + Trigger = 0xB2,
>> + Off = 0xB3,
>> + On = 0xB4,
>> + Blink = 0xB5,
>> +}
>> +
>> +struct LedSE10;
>> +
>> +impl LedSE10 {
>> + /// Base address of the command port.
>> + const CMD_PORT: ResourceSize = 0x6C;
>> + /// Length of the command port.
>> + const CMD_LEN: ResourceSize = 1;
>> + /// Blink duration the hardware supports.
>> + const HW_DURATION: Delta = Delta::from_millis(1000);
>> +
>> + /// Request led region.
>> + fn request_cmd_region(&self) -> Result<Region<'static>> {
>> + Region::request_muxed(Self::CMD_PORT, Self::CMD_LEN,
>> c_str!("leds_lenovo_se10"))
>> + .ok_or(EBUSY)
>> + }
>> +
>> + /// Send command.
>> + fn send_cmd(&self, cmd: LedCommand) -> Result {
>> + let region = self.request_cmd_region()?;
>> + region.outb(cmd as u8, 0);
>> + Ok(())
>> + }
>> +}
>> +
>> +#[vtable]
>> +impl Operations for LedSE10 {
>> + type This = Led<LedSE10>;
>> +
>> + const MAX_BRIGHTNESS: u8 = 1;
>> +
>> + fn brightness_set(this: &mut Self::This, brightness: u8) {
>> + if let Err(e) = if brightness == 0 {
>> + this.data.send_cmd(LedCommand::Off)
>> + } else {
>> + this.data.send_cmd(LedCommand::On)
>> + } {
>> + pr_warn!("Failed to set led: {e:?}\n)")
>> + }
>> + }
>> +
>> + fn blink_set(
>> + this: &mut Self::This,
>> + delay_on: Delta,
>> + delay_off: Delta,
>> + ) -> Result<(Delta, Delta)> {
>> + if !(delay_on.is_zero() && delay_off.is_zero()
>> + || delay_on == Self::HW_DURATION && delay_off ==
>> Self::HW_DURATION)
>> + {
>> + return Err(EINVAL);
>> + }
>> +
>> + this.data.send_cmd(LedCommand::Blink)?;
>> + Ok((Self::HW_DURATION, Self::HW_DURATION))
>> + }
>> +}
>> +
>> +#[vtable]
>> +#[cfg(CONFIG_LEDS_TRIGGERS)]
>> +impl triggers::HardwareOperations for LedSE10 {
>> + fn activate(this: &mut Self::This) -> Result {
>> + this.data.send_cmd(LedCommand::Trigger)
>> + }
>> +}
>> --
>> 2.47.0
>
> I don't have the competence to review the rust code I'm afraid - so my limited feedback above is the best I can do. Not sure it's really worth a reviewed-by tag, but I did read the code and learnt a little about rust in the process (which was fun).
>
> I did test your changes on my SE10 system and it works well.
> Tested-by: Mark Pearson <mpearson-lenovo@squebb.ca>
Thanks a lot,
Fiona
>
> Thanks!
> Mark
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/5] leds: leds_lenovo_se10: LED driver for Lenovo SE10 platform
2025-01-17 17:21 ` Mark Pearson
2025-01-17 17:31 ` Fiona Behrens
@ 2025-01-17 17:43 ` Miguel Ojeda
1 sibling, 0 replies; 17+ messages in thread
From: Miguel Ojeda @ 2025-01-17 17:43 UTC (permalink / raw)
To: Mark Pearson
Cc: Fiona Behrens, Miguel Ojeda, Alex Gaynor, Pavel Machek, Lee Jones,
Jean Delvare, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Peter Koch, rust-for-linux, linux-leds, linux-kernel
On Fri, Jan 17, 2025 at 6:22 PM Mark Pearson <mpearson-lenovo@squebb.ca> wrote:
>
> All the other files are called leds-<name>. Should this be leds-lenovo-se10.rs?
Rust requires underscores for the crate name, so we started with just
using the same name for both since it is simpler and to avoid ending
up with the usual mix of dashes/underscores in the future.
But we can allow them if subsystems think it is important to maintain
consistency within their folder etc.
Cheers,
Miguel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/5] leds: leds_lenovo_se10: LED driver for Lenovo SE10 platform
2025-01-17 17:31 ` Fiona Behrens
@ 2025-01-17 18:02 ` Mark Pearson
0 siblings, 0 replies; 17+ messages in thread
From: Mark Pearson @ 2025-01-17 18:02 UTC (permalink / raw)
To: Fiona Behrens, Miguel Ojeda
Cc: Alex Gaynor, Pavel Machek, Lee Jones, Jean Delvare, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Peter Koch, rust-for-linux, linux-leds,
linux-kernel
Hi
On Fri, Jan 17, 2025, at 12:31 PM, Fiona Behrens wrote:
> Hi,
>
> On 17 Jan 2025, at 18:21, Mark Pearson wrote:
>
>> Hi Fiona,
>>
>> On Mon, Jan 13, 2025, at 7:16 AM, Fiona Behrens wrote:
>>> Add driver for the Lenovo ThinkEdge SE10 LED.
>>>
>>> This driver supports controlling the red LED located on the front panel of the
>>> Lenovo SE10 hardware. Additionally, it supports the hardware-triggered
>>> functionality of the LED, which by default is tied to the WWAN trigger.
>>>
>>> The driver is written in Rust and adds basic LED support for the SE10 platform.
>>>
>>> Signed-off-by: Fiona Behrens <me@kloenk.dev>
>>> ---
>>> drivers/leds/Kconfig | 10 +++
>>> drivers/leds/Makefile | 1 +
>>> drivers/leds/leds_lenovo_se10.rs | 132 +++++++++++++++++++++++++++++++
>>
>> All the other files are called leds-<name>. Should this be leds-lenovo-se10.rs?
>
> This does not work with rust, as the rust makefile converts this
> filename to a rust crate name, and those crate name cannot have dashes
> in them.
> Not sure if we should fix this to hold to the file name conventions,
> maybe something for @Miguel to decide
Ah - thanks for the clarification (and to Miguel in the follow up)
>
>>
>>> 3 files changed, 143 insertions(+)
>>> create mode 100644 drivers/leds/leds_lenovo_se10.rs
>>>
>>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>>> index b784bb74a837..89d9e98189d6 100644
>>> --- a/drivers/leds/Kconfig
>>> +++ b/drivers/leds/Kconfig
>>> @@ -223,6 +223,16 @@ config LEDS_TURRIS_OMNIA
>>> side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the
>>> front panel.
>>>
>>> +config LEDS_LENOVO_SE10
>>> + tristate "LED support for Lenovo ThinkEdge SE10"
>>> + depends on RUST
>>> + depends on (X86 && DMI) || COMPILE_TEST
>>> + depends on HAS_IOPORT
>>> + imply LEDS_TRIGGERS
>>> + help
>>> + This option enables basic support for the LED found on the front of
>>> + Lenovo's SE10 ThinkEdge. There is one user controlable LED on the
>>> front panel.
>>> +
>>> config LEDS_LM3530
>>> tristate "LCD Backlight driver for LM3530"
>>> depends on LEDS_CLASS
>>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>>> index 18afbb5a23ee..2cff22cbafcf 100644
>>> --- a/drivers/leds/Makefile
>>> +++ b/drivers/leds/Makefile
>>> @@ -37,6 +37,7 @@ obj-$(CONFIG_LEDS_IP30) += leds-ip30.o
>>> obj-$(CONFIG_LEDS_IPAQ_MICRO) += leds-ipaq-micro.o
>>> obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o
>>> obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o
>>> +obj-$(CONFIG_LEDS_LENOVO_SE10) += leds_lenovo_se10.o
>>
>> Same note above on name formatting.
>>
>>> obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o
>>> obj-$(CONFIG_LEDS_LM3532) += leds-lm3532.o
>>> obj-$(CONFIG_LEDS_LM3533) += leds-lm3533.o
>>> diff --git a/drivers/leds/leds_lenovo_se10.rs
>>> b/drivers/leds/leds_lenovo_se10.rs
>>> new file mode 100644
>>> index 000000000000..d704125610a4
>>> --- /dev/null
>>> +++ b/drivers/leds/leds_lenovo_se10.rs
>>> @@ -0,0 +1,132 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +//! LED driver for Lenovo ThinkEdge SE10.
>>> +
>>> +use kernel::ioport::{Region, ResourceSize};
>>> +#[cfg(CONFIG_LEDS_TRIGGERS)]
>>> +use kernel::leds::triggers;
>>> +use kernel::leds::{Led, LedConfig, Operations};
>>> +use kernel::prelude::*;
>>> +use kernel::time::Delta;
>>> +use kernel::{c_str, dmi_device_table};
>>> +
>>> +module! {
>>> + type: SE10,
>>> + name: "leds_lenovo_se10",
>>> + author: "Fiona Behrens <me@kloenk.dev>",
>>> + description: "LED driver for Lenovo ThinkEdge SE10",
>>> + license: "GPL",
>>> +}
>>> +
>>> +dmi_device_table!(5, SE10_DMI_TABLE, [
>>> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NH"],
>>> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NJ"],
>>> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NK"],
>>> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NL"],
>>> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NM"],
>>> +]);
>>> +
>>> +struct SE10 {
>>> + /// Led registration
>>> + _led: Pin<KBox<Led<LedSE10>>>,
>>> +}
>>> +
>>> +impl kernel::Module for SE10 {
>>> + fn init(_module: &'static ThisModule) -> Result<Self> {
>>> + if SE10_DMI_TABLE.check_system().is_none() {
>>> + return Err(ENODEV);
>>> + }
>>> +
>>> + let led = KBox::try_pin_init(
>>> + Led::register(
>>> + None,
>>> + LedConfig {
>>> + name: Some(c_str!("platform:red:user")),
>>> + #[cfg(CONFIG_LEDS_TRIGGERS)]
>>> + hardware_trigger: Some(kernel::sync::Arc::pin_init(
>>> + triggers::Hardware::register(c_str!("wwan")),
>>
>> I was curious as to why the "wwan" in here.
>
> This is the hardware trigger, as to the documentation I found from
> Lenovo the trigger mode gives hardware control to the wwan module if
> installed in the hardware.
>
Ah - I should probably have known that :) All good.
Mark
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/5] rust: leds: Add hardware trigger support for hardware-controlled LEDs
2025-01-13 12:16 ` [PATCH v2 3/5] rust: leds: Add hardware trigger support for hardware-controlled LEDs Fiona Behrens
@ 2025-01-20 10:35 ` Marek Behún
2025-01-20 10:59 ` Fiona Behrens
0 siblings, 1 reply; 17+ messages in thread
From: Marek Behún @ 2025-01-20 10:35 UTC (permalink / raw)
To: Fiona Behrens
Cc: Miguel Ojeda, Alex Gaynor, Pavel Machek, Lee Jones, Jean Delvare,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Mark Pearson,
Peter Koch, rust-for-linux, linux-leds, linux-kernel
On Mon, Jan 13, 2025 at 01:16:18PM +0100, Fiona Behrens wrote:
> Adds abstraction for hardware trigger support in LEDs, enabling LEDs to
> be controlled by external hardware events.
>
> An `Arc` is embedded within the `led_classdev` to manage the lifecycle
> of the hardware trigger, ensuring proper reference counting and cleanup
> when the LED is dropped.
>
> Signed-off-by: Fiona Behrens <me@kloenk.dev>
> ---
> MAINTAINERS | 1 +
> rust/kernel/leds.rs | 95 +++++++++++++++++++++++---
> rust/kernel/leds/triggers.rs | 128 +++++++++++++++++++++++++++++++++++
> 3 files changed, 214 insertions(+), 10 deletions(-)
> create mode 100644 rust/kernel/leds/triggers.rs
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cef929b57159..954dbd311a55 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13020,6 +13020,7 @@ F: drivers/leds/
> F: include/dt-bindings/leds/
> F: include/linux/leds.h
> F: rust/kernel/leds.rs
> +F: rust/kernel/leds/
>
> LEGO MINDSTORMS EV3
> R: David Lechner <david@lechnology.com>
> diff --git a/rust/kernel/leds.rs b/rust/kernel/leds.rs
> index 980af7c405d4..f10a10b56e23 100644
> --- a/rust/kernel/leds.rs
> +++ b/rust/kernel/leds.rs
> @@ -10,9 +10,14 @@
> use crate::error::from_result;
> use crate::ffi::c_ulong;
> use crate::prelude::*;
> +#[cfg(CONFIG_LEDS_TRIGGERS)]
> +use crate::sync::Arc;
> use crate::time::Delta;
> use crate::types::Opaque;
>
> +#[cfg(CONFIG_LEDS_TRIGGERS)]
> +pub mod triggers;
> +
> /// Color of an LED.
> #[allow(missing_docs)]
> #[derive(Copy, Clone)]
> @@ -110,12 +115,34 @@ fn try_from(value: u32) -> Result<Self, Self::Error> {
> }
>
> /// Data used for led registration.
> -#[derive(Clone)]
> -pub struct LedConfig<'name> {
> +pub struct LedConfig<'name, T> {
> /// Name to give the led.
> pub name: Option<&'name CStr>,
> /// Color of the LED.
> pub color: Color,
> + /// Private data of the LED.
> + pub data: T,
> +
> + /// Default trigger name.
> + pub default_trigger: Option<&'static CStr>,
> + /// Hardware trigger.
> + ///
> + /// Setting this to some also defaults the default trigger to this hardware trigger.
> + /// Use `default_trigger: Some("none")` to overwrite this.
> + #[cfg(CONFIG_LEDS_TRIGGERS)]
> + pub hardware_trigger: Option<Arc<triggers::Hardware<T>>>,
> +}
> +
> +impl<'name, T> LedConfig<'name, T> {
> + /// Create a new LedConfig
> + pub fn new(color: Color, data: T) -> Self {
> + Self {
> + color,
> + data,
> + // SAFETY: all other fields are valid with zeroes.
> + ..unsafe { core::mem::zeroed() }
> + }
> + }
> }
>
> /// A Led backed by a C `struct led_classdev`, additionally offering
> @@ -141,8 +168,7 @@ impl<T> Led<T>
> #[cfg(CONFIG_LEDS_CLASS)]
> pub fn register<'a>(
> device: Option<&'a Device>,
> - config: &'a LedConfig<'a>,
> - data: T,
> + config: LedConfig<'a, T>,
> ) -> impl PinInit<Self, Error> + 'a
> where
> T: 'a,
> @@ -188,14 +214,46 @@ pub fn register<'a>(
> unsafe { ptr::write(set_fn_ptr, Some(blink_set::<T>)) };
> }
>
> + #[cfg(CONFIG_LEDS_TRIGGERS)]
> + if let Some(trigger) = config.hardware_trigger {
> + let trigger = trigger.into_raw();
> + // SAFETY: `place` is pointing to a live allocation.
> + let trigger_type_ptr = unsafe { ptr::addr_of_mut!((*place).trigger_type) };
> + // SAFETY: `trigger` is a valid pointer
> + let hw_trigger = unsafe { ptr::addr_of!((*trigger).hw_type) };
> + // SAFETY: `trigger_type_ptr` points to a valid allocation and we have exclusive access.
> + unsafe { ptr::write(trigger_type_ptr, hw_trigger.cast_mut().cast()) };
> +
> + // SAFETY: trigger points to a valid hardware trigger struct.
> + let trigger_name_ptr = unsafe { Opaque::raw_get(ptr::addr_of!( (*trigger).trigger)) };
> + // SAFETY: trigger points to a valid hardware trigger struct.
> + let trigger_name_ptr = unsafe { (*trigger_name_ptr).name };
> + // SAFETY: `place` is pointing to a live allocation.
> + let default_trigger_ptr = unsafe { ptr::addr_of_mut!((*place).default_trigger) };
> + // SAFETY: `default_trigger_ptr` points to a valid allocation and we have exclusive access.
> + unsafe { ptr::write(default_trigger_ptr, trigger_name_ptr) };
> +
> + // SAFETY: `place` is pointing to a live allocation.
> + let hw_ctrl_trigger_ptr = unsafe { ptr::addr_of_mut!((*place).hw_control_trigger) };
> + // SAFETY: `hw_ctrl_trigger_ptr` points to a valid allocation and we have exclusive access.
> + unsafe { ptr::write(hw_ctrl_trigger_ptr, trigger_name_ptr) };
> + }
> +
> + // After hw trigger impl, to overwrite default trigger
> + if let Some(default_trigger) = config.default_trigger {
> + // SAFETY: `place` is pointing to a live allocation.
> + let default_trigger_ptr = unsafe { ptr::addr_of_mut!((*place).default_trigger) };
> + // SAFETY: `default_trigger_ptr` points to a valid allocation and we have exclusive access.
> + unsafe { ptr::write(default_trigger_ptr, default_trigger.as_char_ptr()) };
> + }
>
> - let dev = device.map(|dev| dev.as_raw()).unwrap_or(ptr::null_mut());
> - // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
> - crate::error::to_result(unsafe {
> - bindings::led_classdev_register_ext(dev, place, ptr::null_mut())
> - })
> + let dev = device.map(|dev| dev.as_raw()).unwrap_or(ptr::null_mut());
> + // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
> + crate::error::to_result(unsafe {
> + bindings::led_classdev_register_ext(dev, place, ptr::null_mut())
> + })
> }),
> - data: data,
> + data: config.data,
> })
> }
> }
> @@ -220,6 +278,23 @@ fn drop(self: Pin<&mut Self>) {
> unsafe {
> bindings::led_classdev_unregister(self.led.get())
> }
> +
> + // drop trigger if there is a hw trigger defined.
> + #[cfg(CONFIG_LEDS_TRIGGERS)]
> + {
> + // SAFETY: `self.led` is a valid led.
> + let hw_trigger_type =
> + unsafe { ptr::read(ptr::addr_of!((*self.led.get()).trigger_type)) };
> + if !hw_trigger_type.is_null() {
> + // SAFETY: hw_trigger_type is a valid and non null pointer into a Hardware trigger.
> + let hw_trigger_type = unsafe {
> + crate::container_of!(hw_trigger_type, triggers::Hardware<T>, hw_type)
> + };
> + // SAFETY: `hw_trigger_type` is a valid pointer that came from an arc.
> + let hw_trigger_type = unsafe { Arc::from_raw(hw_trigger_type) };
> + drop(hw_trigger_type);
> + }
> + }
> }
> }
>
> diff --git a/rust/kernel/leds/triggers.rs b/rust/kernel/leds/triggers.rs
> new file mode 100644
> index 000000000000..d5f2b8252645
> --- /dev/null
> +++ b/rust/kernel/leds/triggers.rs
> @@ -0,0 +1,128 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! LED trigger abstractions.
> +
> +use core::marker::PhantomData;
> +use core::ptr;
> +
> +use crate::error::{from_result, to_result};
> +use crate::prelude::*;
> +use crate::types::Opaque;
> +
> +use super::FromLedClassdev;
> +
> +/// LED Hardware trigger.
> +///
> +/// Used to impement a hardware operation mode for an LED.
> +#[pin_data(PinnedDrop)]
> +pub struct Hardware<T> {
> + #[pin]
> + pub(crate) hw_type: Opaque<bindings::led_hw_trigger_type>,
This should probably be called trigger_type instead of hw_type,
as it is in the C version of the code.
> + #[pin]
> + pub(crate) trigger: Opaque<bindings::led_trigger>,
> + _t: PhantomData<T>,
> +}
> +
> +impl<T> Hardware<T>
> +where
> + T: HardwareOperations,
> +{
> + /// Register a new hardware Trigger with a given name.
> + pub fn register(name: &'static CStr) -> impl PinInit<Self, Error> {
> + try_pin_init!( Self {
> + // SAFETY: `led_hw_trigger_type` is valid with all zeroes.
> + hw_type: Opaque::new(unsafe { core::mem::zeroed() }),
> + trigger <- Opaque::try_ffi_init(move |place: *mut bindings::led_trigger| {
> + // SAFETY: `place` is a pointer to a live allocation, so erasing is valid.
> + unsafe { place.write_bytes(0, 1) };
> +
> + // Add name
> + // SAFETY: `place` is pointing to a live allocation, so the deref is safe.
> + let name_ptr = unsafe { ptr::addr_of_mut!((*place).name) };
> + // SAFETY: `name_ptr` points to a valid allocation and we have exclusive access.
> + unsafe { ptr::write(name_ptr, name.as_char_ptr()) };
> +
> + // Add fn pointers
> + // SAFETY: `place` is pointing to a live allocation, so the deref is safe.
> + let activate_fn_ptr: *mut Option<_> = unsafe { ptr::addr_of_mut!((*place).activate) };
> + // SAFETY: `activate_fn_ptr` points to a valid allocation and we have exclusive access.
> + unsafe { ptr::write(activate_fn_ptr, Some(trigger_activate::<T>)) };
> +
> + if T::HAS_DEACTIVATE {
> + // SAFETY: `place` is pointing to a live allocation, so the deref is safe.
> + let deactivate_fn_ptr: *mut Option<_> = unsafe { ptr::addr_of_mut!((*place).deactivate) };
> + // SAFETY: `deactivate_fn_ptr` points to a valid allocation and we have exclusive access.
> + unsafe { ptr::write(deactivate_fn_ptr, Some(trigger_deactivate::<T>)) };
> + }
> +
> + // Add hardware trigger
> + // SAFETY: `place` is pointing to a live allocation, so the deref is safe.
> + let trigger_type_ptr = unsafe { ptr::addr_of_mut!((*place).trigger_type) };
> + // SAFETY: `place` is pointing to a live allocation, so the deref is safe.
> + let trigger_type = unsafe { crate::container_of!(place, Self, trigger).cast_mut() };
> + // SAFETY: `trigger_type` is pointing to a live allocation of Self.
> + let trigger_type = unsafe { ptr::addr_of!((*trigger_type).hw_type) };
> + // SAFETY: `trigger_type_ptr` points to a valid allocation and we have exclusive access.
> + unsafe{ ptr::write(trigger_type_ptr, Opaque::raw_get(trigger_type)) };
> +
> + // SAFETY: ffi call, `place` is sufficently filled with data at this point
> + to_result(unsafe {
> + bindings::led_trigger_register(place)
> + })
> + }),
> + _t: PhantomData,
> + })
> + }
> +}
> +
> +#[pinned_drop]
> +impl<T> PinnedDrop for Hardware<T> {
> + fn drop(self: Pin<&mut Self>) {
> + // SAFETY: trigger is pointing to a live and registered allocation
> + unsafe {
> + bindings::led_trigger_unregister(self.trigger.get());
> + }
> + }
> +}
> +
> +/// Operations for the Hardware trigger
> +#[macros::vtable]
> +pub trait HardwareOperations: super::Operations {
> + /// Activate the hardware trigger.
> + fn activate(this: &mut Self::This) -> Result;
> + /// Deactivate the hardware trigger.
> + fn deactivate(_this: &mut Self::This) {
> + crate::build_error(crate::error::VTABLE_DEFAULT_ERROR)
> + }
> +}
This looks as if you are doing a Rust binding for struct led_trigger.
But you keep calling it Hardware trigger, which makes me thing that
you are confused about what is a LED trigger and what is a hardware
trigger.
Why do you keep putting "Hardware" into the names of these symbols?
I fear that you may be confused about some stuff here. In order to
determine whether this is the case, could you answer the following
questions please?
- What is the purpose of `struct led_hw_trigger_type`?
- What is the purpose of the `dummy` member of this struct? What
value should be assigned to it?
- If a LED class device (LED cdev) has the `trigger_type` member set
to NULL, which LED triggers will be listed in the sysfs `trigger`
file for this LED cdev? And which triggers will be listed if the
`trigger_type` member is not NULL?
- Why does both `struct led_classdev` and `struct led_trigger` have
the `trigger_type` member?
> +/// `trigger_activate` function pointer
> +///
> +/// # Safety
> +///
> +/// `led_cdev` must be passed by the corresponding callback in `led_trigger`.
> +unsafe extern "C" fn trigger_activate<T>(led_cdev: *mut bindings::led_classdev) -> i32
> +where
> + T: HardwareOperations,
> +{
> + from_result(|| {
> + // SAFETY: By the safety requirement of this function `led_cdev` is embedded inside a `T::This<T>`.
> + let led = unsafe { &mut *(T::This::led_container_of(led_cdev.cast())) };
> + T::activate(led)?;
> + Ok(0)
> + })
> +}
> +
> +/// `trigger_deactivate` function pointer
> +///
> +/// # Safety
> +///
> +/// `led_cdev` must be passed by the corresponding callback in `led_trigger`.
> +unsafe extern "C" fn trigger_deactivate<T>(led_cdev: *mut bindings::led_classdev)
> +where
> + T: HardwareOperations,
> +{
> + // SAFETY: By the safety requirement of this function `led_cdev` is embedded inside a `T::This<T>`.
> + let led = unsafe { &mut *(T::This::led_container_of(led_cdev.cast())) };
> + T::deactivate(led)
> +}
> --
> 2.47.0
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/5] leds: leds_lenovo_se10: LED driver for Lenovo SE10 platform
2025-01-13 12:16 ` [PATCH v2 5/5] leds: leds_lenovo_se10: LED driver for Lenovo SE10 platform Fiona Behrens
2025-01-17 17:21 ` Mark Pearson
@ 2025-01-20 10:47 ` Marek Behún
1 sibling, 0 replies; 17+ messages in thread
From: Marek Behún @ 2025-01-20 10:47 UTC (permalink / raw)
To: Fiona Behrens
Cc: Miguel Ojeda, Alex Gaynor, Pavel Machek, Lee Jones, Jean Delvare,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Mark Pearson,
Peter Koch, rust-for-linux, linux-leds, linux-kernel
On Mon, Jan 13, 2025 at 01:16:20PM +0100, Fiona Behrens wrote:
> Add driver for the Lenovo ThinkEdge SE10 LED.
>
> This driver supports controlling the red LED located on the front panel of the
> Lenovo SE10 hardware. Additionally, it supports the hardware-triggered
> functionality of the LED, which by default is tied to the WWAN trigger.
>
> The driver is written in Rust and adds basic LED support for the SE10 platform.
>
> Signed-off-by: Fiona Behrens <me@kloenk.dev>
> ---
> drivers/leds/Kconfig | 10 +++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds_lenovo_se10.rs | 132 +++++++++++++++++++++++++++++++
> 3 files changed, 143 insertions(+)
> create mode 100644 drivers/leds/leds_lenovo_se10.rs
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index b784bb74a837..89d9e98189d6 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -223,6 +223,16 @@ config LEDS_TURRIS_OMNIA
> side of CZ.NIC's Turris Omnia router. There are 12 RGB LEDs on the
> front panel.
>
> +config LEDS_LENOVO_SE10
> + tristate "LED support for Lenovo ThinkEdge SE10"
> + depends on RUST
> + depends on (X86 && DMI) || COMPILE_TEST
> + depends on HAS_IOPORT
> + imply LEDS_TRIGGERS
> + help
> + This option enables basic support for the LED found on the front of
> + Lenovo's SE10 ThinkEdge. There is one user controlable LED on the front panel.
> +
> config LEDS_LM3530
> tristate "LCD Backlight driver for LM3530"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 18afbb5a23ee..2cff22cbafcf 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -37,6 +37,7 @@ obj-$(CONFIG_LEDS_IP30) += leds-ip30.o
> obj-$(CONFIG_LEDS_IPAQ_MICRO) += leds-ipaq-micro.o
> obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o
> obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o
> +obj-$(CONFIG_LEDS_LENOVO_SE10) += leds_lenovo_se10.o
> obj-$(CONFIG_LEDS_LM3530) += leds-lm3530.o
> obj-$(CONFIG_LEDS_LM3532) += leds-lm3532.o
> obj-$(CONFIG_LEDS_LM3533) += leds-lm3533.o
> diff --git a/drivers/leds/leds_lenovo_se10.rs b/drivers/leds/leds_lenovo_se10.rs
> new file mode 100644
> index 000000000000..d704125610a4
> --- /dev/null
> +++ b/drivers/leds/leds_lenovo_se10.rs
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//! LED driver for Lenovo ThinkEdge SE10.
> +
> +use kernel::ioport::{Region, ResourceSize};
> +#[cfg(CONFIG_LEDS_TRIGGERS)]
> +use kernel::leds::triggers;
> +use kernel::leds::{Led, LedConfig, Operations};
> +use kernel::prelude::*;
> +use kernel::time::Delta;
> +use kernel::{c_str, dmi_device_table};
> +
> +module! {
> + type: SE10,
> + name: "leds_lenovo_se10",
> + author: "Fiona Behrens <me@kloenk.dev>",
> + description: "LED driver for Lenovo ThinkEdge SE10",
> + license: "GPL",
> +}
> +
> +dmi_device_table!(5, SE10_DMI_TABLE, [
> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NH"],
> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NJ"],
> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NK"],
> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NL"],
> + "LENOVO-SE10": [SysVendor: "LENOVO", ProductName: "12NM"],
> +]);
> +
> +struct SE10 {
> + /// Led registration
> + _led: Pin<KBox<Led<LedSE10>>>,
> +}
> +
> +impl kernel::Module for SE10 {
> + fn init(_module: &'static ThisModule) -> Result<Self> {
> + if SE10_DMI_TABLE.check_system().is_none() {
> + return Err(ENODEV);
> + }
> +
> + let led = KBox::try_pin_init(
> + Led::register(
> + None,
> + LedConfig {
> + name: Some(c_str!("platform:red:user")),
> + #[cfg(CONFIG_LEDS_TRIGGERS)]
> + hardware_trigger: Some(kernel::sync::Arc::pin_init(
> + triggers::Hardware::register(c_str!("wwan")),
There are currently two LED drivers utilizing the led_hw_trigger_type
mechanism to make certain triggers available only for certain LEDs:
- the leds-cros_ec.c driver, which registers the trigger under the name
"chromeos-auto", to suggest that activating the trigger on this LED
will make it blink automatically by hardware and that it is ChromeOS
specific,
- the leds-turris-omnia.c driver, which registers the trigger under the
name "omnia-mcu", to suggest that activating the trigger will make the
LED blinking be controller by the MCU on Turris Omnia.
Using the name "wwan" for this trigger is too general. In the future
someone may want to create a software "wwan" trigger that will be
available for any LED class device, for example...
Please change the name of this LED-private trigger.
> + GFP_KERNEL,
> + )?),
> + ..LedConfig::new(kernel::leds::Color::Red, LedSE10)
> + },
> + ),
> + GFP_KERNEL,
> + )?;
> +
> + Ok(Self { _led: led })
> + }
> +}
> +
> +/// Valid led commands.
> +#[repr(u8)]
> +#[allow(missing_docs)]
> +enum LedCommand {
> + #[cfg(CONFIG_LEDS_TRIGGERS)]
> + Trigger = 0xB2,
> + Off = 0xB3,
> + On = 0xB4,
> + Blink = 0xB5,
> +}
> +
> +struct LedSE10;
> +
> +impl LedSE10 {
> + /// Base address of the command port.
> + const CMD_PORT: ResourceSize = 0x6C;
> + /// Length of the command port.
> + const CMD_LEN: ResourceSize = 1;
> + /// Blink duration the hardware supports.
> + const HW_DURATION: Delta = Delta::from_millis(1000);
> +
> + /// Request led region.
> + fn request_cmd_region(&self) -> Result<Region<'static>> {
> + Region::request_muxed(Self::CMD_PORT, Self::CMD_LEN, c_str!("leds_lenovo_se10"))
> + .ok_or(EBUSY)
> + }
> +
> + /// Send command.
> + fn send_cmd(&self, cmd: LedCommand) -> Result {
> + let region = self.request_cmd_region()?;
> + region.outb(cmd as u8, 0);
> + Ok(())
> + }
> +}
> +
> +#[vtable]
> +impl Operations for LedSE10 {
> + type This = Led<LedSE10>;
> +
> + const MAX_BRIGHTNESS: u8 = 1;
> +
> + fn brightness_set(this: &mut Self::This, brightness: u8) {
> + if let Err(e) = if brightness == 0 {
> + this.data.send_cmd(LedCommand::Off)
> + } else {
> + this.data.send_cmd(LedCommand::On)
> + } {
> + pr_warn!("Failed to set led: {e:?}\n)")
> + }
> + }
> +
> + fn blink_set(
> + this: &mut Self::This,
> + delay_on: Delta,
> + delay_off: Delta,
> + ) -> Result<(Delta, Delta)> {
> + if !(delay_on.is_zero() && delay_off.is_zero()
> + || delay_on == Self::HW_DURATION && delay_off == Self::HW_DURATION)
> + {
> + return Err(EINVAL);
> + }
> +
> + this.data.send_cmd(LedCommand::Blink)?;
> + Ok((Self::HW_DURATION, Self::HW_DURATION))
> + }
> +}
> +
> +#[vtable]
> +#[cfg(CONFIG_LEDS_TRIGGERS)]
> +impl triggers::HardwareOperations for LedSE10 {
> + fn activate(this: &mut Self::This) -> Result {
> + this.data.send_cmd(LedCommand::Trigger)
> + }
No deactivation method for the trigger? NACK.
The driver must implement the deactivation method, since LED core
always allows disabling LED triggers. See led-trigger.c function
led_trigger_write(): if "none" is written to the sysfs `trigger`
file, the trigger is removed and the `trigger` file will afterwards
report that no trigger is activated on the LED.
Since you did not implement the deactivation method, this will result
in the system thinking that no LED trigger is selected on the LED,
but in fact your LED's blinking will still be controlled by hardware.
Marek
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/5] rust: leds: Add hardware trigger support for hardware-controlled LEDs
2025-01-20 10:35 ` Marek Behún
@ 2025-01-20 10:59 ` Fiona Behrens
2025-01-20 14:18 ` Marek Behún
0 siblings, 1 reply; 17+ messages in thread
From: Fiona Behrens @ 2025-01-20 10:59 UTC (permalink / raw)
To: Marek Behún
Cc: Miguel Ojeda, Alex Gaynor, Pavel Machek, Lee Jones, Jean Delvare,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Alice Ryhl, Trevor Gross, Mark Pearson,
Peter Koch, rust-for-linux, linux-leds, linux-kernel
Hi,
On 20 Jan 2025, at 11:35, Marek Behún wrote:
> On Mon, Jan 13, 2025 at 01:16:18PM +0100, Fiona Behrens wrote:
>> Adds abstraction for hardware trigger support in LEDs, enabling LEDs to
>> be controlled by external hardware events.
>>
>> An `Arc` is embedded within the `led_classdev` to manage the lifecycle
>> of the hardware trigger, ensuring proper reference counting and cleanup
>> when the LED is dropped.
>>
>> Signed-off-by: Fiona Behrens <me@kloenk.dev>
>> ---
>> MAINTAINERS | 1 +
>> rust/kernel/leds.rs | 95 +++++++++++++++++++++++---
>> rust/kernel/leds/triggers.rs | 128 +++++++++++++++++++++++++++++++++++
>> 3 files changed, 214 insertions(+), 10 deletions(-)
>> create mode 100644 rust/kernel/leds/triggers.rs
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index cef929b57159..954dbd311a55 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -13020,6 +13020,7 @@ F: drivers/leds/
>> F: include/dt-bindings/leds/
>> F: include/linux/leds.h
>> F: rust/kernel/leds.rs
>> +F: rust/kernel/leds/
>>
>> LEGO MINDSTORMS EV3
>> R: David Lechner <david@lechnology.com>
>> diff --git a/rust/kernel/leds.rs b/rust/kernel/leds.rs
>> index 980af7c405d4..f10a10b56e23 100644
>> --- a/rust/kernel/leds.rs
>> +++ b/rust/kernel/leds.rs
>> @@ -10,9 +10,14 @@
>> use crate::error::from_result;
>> use crate::ffi::c_ulong;
>> use crate::prelude::*;
>> +#[cfg(CONFIG_LEDS_TRIGGERS)]
>> +use crate::sync::Arc;
>> use crate::time::Delta;
>> use crate::types::Opaque;
>>
>> +#[cfg(CONFIG_LEDS_TRIGGERS)]
>> +pub mod triggers;
>> +
>> /// Color of an LED.
>> #[allow(missing_docs)]
>> #[derive(Copy, Clone)]
>> @@ -110,12 +115,34 @@ fn try_from(value: u32) -> Result<Self, Self::Error> {
>> }
>>
>> /// Data used for led registration.
>> -#[derive(Clone)]
>> -pub struct LedConfig<'name> {
>> +pub struct LedConfig<'name, T> {
>> /// Name to give the led.
>> pub name: Option<&'name CStr>,
>> /// Color of the LED.
>> pub color: Color,
>> + /// Private data of the LED.
>> + pub data: T,
>> +
>> + /// Default trigger name.
>> + pub default_trigger: Option<&'static CStr>,
>> + /// Hardware trigger.
>> + ///
>> + /// Setting this to some also defaults the default trigger to this hardware trigger.
>> + /// Use `default_trigger: Some("none")` to overwrite this.
>> + #[cfg(CONFIG_LEDS_TRIGGERS)]
>> + pub hardware_trigger: Option<Arc<triggers::Hardware<T>>>,
>> +}
>> +
>> +impl<'name, T> LedConfig<'name, T> {
>> + /// Create a new LedConfig
>> + pub fn new(color: Color, data: T) -> Self {
>> + Self {
>> + color,
>> + data,
>> + // SAFETY: all other fields are valid with zeroes.
>> + ..unsafe { core::mem::zeroed() }
>> + }
>> + }
>> }
>>
>> /// A Led backed by a C `struct led_classdev`, additionally offering
>> @@ -141,8 +168,7 @@ impl<T> Led<T>
>> #[cfg(CONFIG_LEDS_CLASS)]
>> pub fn register<'a>(
>> device: Option<&'a Device>,
>> - config: &'a LedConfig<'a>,
>> - data: T,
>> + config: LedConfig<'a, T>,
>> ) -> impl PinInit<Self, Error> + 'a
>> where
>> T: 'a,
>> @@ -188,14 +214,46 @@ pub fn register<'a>(
>> unsafe { ptr::write(set_fn_ptr, Some(blink_set::<T>)) };
>> }
>>
>> + #[cfg(CONFIG_LEDS_TRIGGERS)]
>> + if let Some(trigger) = config.hardware_trigger {
>> + let trigger = trigger.into_raw();
>> + // SAFETY: `place` is pointing to a live allocation.
>> + let trigger_type_ptr = unsafe { ptr::addr_of_mut!((*place).trigger_type) };
>> + // SAFETY: `trigger` is a valid pointer
>> + let hw_trigger = unsafe { ptr::addr_of!((*trigger).hw_type) };
>> + // SAFETY: `trigger_type_ptr` points to a valid allocation and we have exclusive access.
>> + unsafe { ptr::write(trigger_type_ptr, hw_trigger.cast_mut().cast()) };
>> +
>> + // SAFETY: trigger points to a valid hardware trigger struct.
>> + let trigger_name_ptr = unsafe { Opaque::raw_get(ptr::addr_of!( (*trigger).trigger)) };
>> + // SAFETY: trigger points to a valid hardware trigger struct.
>> + let trigger_name_ptr = unsafe { (*trigger_name_ptr).name };
>> + // SAFETY: `place` is pointing to a live allocation.
>> + let default_trigger_ptr = unsafe { ptr::addr_of_mut!((*place).default_trigger) };
>> + // SAFETY: `default_trigger_ptr` points to a valid allocation and we have exclusive access.
>> + unsafe { ptr::write(default_trigger_ptr, trigger_name_ptr) };
>> +
>> + // SAFETY: `place` is pointing to a live allocation.
>> + let hw_ctrl_trigger_ptr = unsafe { ptr::addr_of_mut!((*place).hw_control_trigger) };
>> + // SAFETY: `hw_ctrl_trigger_ptr` points to a valid allocation and we have exclusive access.
>> + unsafe { ptr::write(hw_ctrl_trigger_ptr, trigger_name_ptr) };
>> + }
>> +
>> + // After hw trigger impl, to overwrite default trigger
>> + if let Some(default_trigger) = config.default_trigger {
>> + // SAFETY: `place` is pointing to a live allocation.
>> + let default_trigger_ptr = unsafe { ptr::addr_of_mut!((*place).default_trigger) };
>> + // SAFETY: `default_trigger_ptr` points to a valid allocation and we have exclusive access.
>> + unsafe { ptr::write(default_trigger_ptr, default_trigger.as_char_ptr()) };
>> + }
>>
>> - let dev = device.map(|dev| dev.as_raw()).unwrap_or(ptr::null_mut());
>> - // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
>> - crate::error::to_result(unsafe {
>> - bindings::led_classdev_register_ext(dev, place, ptr::null_mut())
>> - })
>> + let dev = device.map(|dev| dev.as_raw()).unwrap_or(ptr::null_mut());
>> + // SAFETY: `place` is a pointer to a live allocation of `bindings::led_classdev`.
>> + crate::error::to_result(unsafe {
>> + bindings::led_classdev_register_ext(dev, place, ptr::null_mut())
>> + })
>> }),
>> - data: data,
>> + data: config.data,
>> })
>> }
>> }
>> @@ -220,6 +278,23 @@ fn drop(self: Pin<&mut Self>) {
>> unsafe {
>> bindings::led_classdev_unregister(self.led.get())
>> }
>> +
>> + // drop trigger if there is a hw trigger defined.
>> + #[cfg(CONFIG_LEDS_TRIGGERS)]
>> + {
>> + // SAFETY: `self.led` is a valid led.
>> + let hw_trigger_type =
>> + unsafe { ptr::read(ptr::addr_of!((*self.led.get()).trigger_type)) };
>> + if !hw_trigger_type.is_null() {
>> + // SAFETY: hw_trigger_type is a valid and non null pointer into a Hardware trigger.
>> + let hw_trigger_type = unsafe {
>> + crate::container_of!(hw_trigger_type, triggers::Hardware<T>, hw_type)
>> + };
>> + // SAFETY: `hw_trigger_type` is a valid pointer that came from an arc.
>> + let hw_trigger_type = unsafe { Arc::from_raw(hw_trigger_type) };
>> + drop(hw_trigger_type);
>> + }
>> + }
>> }
>> }
>>
>> diff --git a/rust/kernel/leds/triggers.rs b/rust/kernel/leds/triggers.rs
>> new file mode 100644
>> index 000000000000..d5f2b8252645
>> --- /dev/null
>> +++ b/rust/kernel/leds/triggers.rs
>> @@ -0,0 +1,128 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! LED trigger abstractions.
>> +
>> +use core::marker::PhantomData;
>> +use core::ptr;
>> +
>> +use crate::error::{from_result, to_result};
>> +use crate::prelude::*;
>> +use crate::types::Opaque;
>> +
>> +use super::FromLedClassdev;
>> +
>> +/// LED Hardware trigger.
>> +///
>> +/// Used to impement a hardware operation mode for an LED.
>> +#[pin_data(PinnedDrop)]
>> +pub struct Hardware<T> {
>> + #[pin]
>> + pub(crate) hw_type: Opaque<bindings::led_hw_trigger_type>,
>
> This should probably be called trigger_type instead of hw_type,
> as it is in the C version of the code.
>
>> + #[pin]
>> + pub(crate) trigger: Opaque<bindings::led_trigger>,
>> + _t: PhantomData<T>,
>> +}
>> +
>> +impl<T> Hardware<T>
>> +where
>> + T: HardwareOperations,
>> +{
>> + /// Register a new hardware Trigger with a given name.
>> + pub fn register(name: &'static CStr) -> impl PinInit<Self, Error> {
>> + try_pin_init!( Self {
>> + // SAFETY: `led_hw_trigger_type` is valid with all zeroes.
>> + hw_type: Opaque::new(unsafe { core::mem::zeroed() }),
>> + trigger <- Opaque::try_ffi_init(move |place: *mut bindings::led_trigger| {
>> + // SAFETY: `place` is a pointer to a live allocation, so erasing is valid.
>> + unsafe { place.write_bytes(0, 1) };
>> +
>> + // Add name
>> + // SAFETY: `place` is pointing to a live allocation, so the deref is safe.
>> + let name_ptr = unsafe { ptr::addr_of_mut!((*place).name) };
>> + // SAFETY: `name_ptr` points to a valid allocation and we have exclusive access.
>> + unsafe { ptr::write(name_ptr, name.as_char_ptr()) };
>> +
>> + // Add fn pointers
>> + // SAFETY: `place` is pointing to a live allocation, so the deref is safe.
>> + let activate_fn_ptr: *mut Option<_> = unsafe { ptr::addr_of_mut!((*place).activate) };
>> + // SAFETY: `activate_fn_ptr` points to a valid allocation and we have exclusive access.
>> + unsafe { ptr::write(activate_fn_ptr, Some(trigger_activate::<T>)) };
>> +
>> + if T::HAS_DEACTIVATE {
>> + // SAFETY: `place` is pointing to a live allocation, so the deref is safe.
>> + let deactivate_fn_ptr: *mut Option<_> = unsafe { ptr::addr_of_mut!((*place).deactivate) };
>> + // SAFETY: `deactivate_fn_ptr` points to a valid allocation and we have exclusive access.
>> + unsafe { ptr::write(deactivate_fn_ptr, Some(trigger_deactivate::<T>)) };
>> + }
>> +
>> + // Add hardware trigger
>> + // SAFETY: `place` is pointing to a live allocation, so the deref is safe.
>> + let trigger_type_ptr = unsafe { ptr::addr_of_mut!((*place).trigger_type) };
>> + // SAFETY: `place` is pointing to a live allocation, so the deref is safe.
>> + let trigger_type = unsafe { crate::container_of!(place, Self, trigger).cast_mut() };
>> + // SAFETY: `trigger_type` is pointing to a live allocation of Self.
>> + let trigger_type = unsafe { ptr::addr_of!((*trigger_type).hw_type) };
>> + // SAFETY: `trigger_type_ptr` points to a valid allocation and we have exclusive access.
>> + unsafe{ ptr::write(trigger_type_ptr, Opaque::raw_get(trigger_type)) };
>> +
>> + // SAFETY: ffi call, `place` is sufficently filled with data at this point
>> + to_result(unsafe {
>> + bindings::led_trigger_register(place)
>> + })
>> + }),
>> + _t: PhantomData,
>> + })
>> + }
>> +}
>> +
>> +#[pinned_drop]
>> +impl<T> PinnedDrop for Hardware<T> {
>> + fn drop(self: Pin<&mut Self>) {
>> + // SAFETY: trigger is pointing to a live and registered allocation
>> + unsafe {
>> + bindings::led_trigger_unregister(self.trigger.get());
>> + }
>> + }
>> +}
>> +
>> +/// Operations for the Hardware trigger
>> +#[macros::vtable]
>> +pub trait HardwareOperations: super::Operations {
>> + /// Activate the hardware trigger.
>> + fn activate(this: &mut Self::This) -> Result;
>> + /// Deactivate the hardware trigger.
>> + fn deactivate(_this: &mut Self::This) {
>> + crate::build_error(crate::error::VTABLE_DEFAULT_ERROR)
>> + }
>> +}
>
> This looks as if you are doing a Rust binding for struct led_trigger.
> But you keep calling it Hardware trigger, which makes me thing that
> you are confused about what is a LED trigger and what is a hardware
> trigger.
>
> Why do you keep putting "Hardware" into the names of these symbols?
The idea was to create a abstraction specific to writing a hardware trigger (or my understanding of what that is) and deal with the other
trigger types later, to more separate the things on the rust side with e.g. the vtables for those.
But my understanding might be wrong.
(My broad understanding is what I did in the SE10 driver later, to tell the hardware to not present the LED to the kernel, but some other hardware wiring to a hardware thing that then drives the LED)
>
> I fear that you may be confused about some stuff here. In order to
> determine whether this is the case, could you answer the following
> questions please?
That might be right, thanks for trying to clear it up if that is the case.
> - What is the purpose of `struct led_hw_trigger_type`?
Marking a led that it has a private trigger that gives control of the LED to some hardware driver.
> - What is the purpose of the `dummy` member of this struct? What
> value should be assigned to it?
From my understanding this is just to give the struct a size, so that it has a unique address in memory so the pointer value can be compared.
> - If a LED class device (LED cdev) has the `trigger_type` member set
> to NULL, which LED triggers will be listed in the sysfs `trigger`
> file for this LED cdev? And which triggers will be listed if the
> `trigger_type` member is not NULL?
For null all generic triggers will be listed, and for some value all generic plus the specific trigger.
> - Why does both `struct led_classdev` and `struct led_trigger` have
> the `trigger_type` member?
led_classdev has it to declare that it does have a led private trigger mode, and the led_trigger has it so the activate/deactive functions can be found.
My research so far into how triggers work was mostly so that I can use the wwan module trigger on the SE10 board I have here, and therefore I did not look into how to write a generic led trigger usable on more then a specific led.
Thanks a lot for clearing up possible misunderstandings,
Fiona
>
>> +/// `trigger_activate` function pointer
>> +///
>> +/// # Safety
>> +///
>> +/// `led_cdev` must be passed by the corresponding callback in `led_trigger`.
>> +unsafe extern "C" fn trigger_activate<T>(led_cdev: *mut bindings::led_classdev) -> i32
>> +where
>> + T: HardwareOperations,
>> +{
>> + from_result(|| {
>> + // SAFETY: By the safety requirement of this function `led_cdev` is embedded inside a `T::This<T>`.
>> + let led = unsafe { &mut *(T::This::led_container_of(led_cdev.cast())) };
>> + T::activate(led)?;
>> + Ok(0)
>> + })
>> +}
>> +
>> +/// `trigger_deactivate` function pointer
>> +///
>> +/// # Safety
>> +///
>> +/// `led_cdev` must be passed by the corresponding callback in `led_trigger`.
>> +unsafe extern "C" fn trigger_deactivate<T>(led_cdev: *mut bindings::led_classdev)
>> +where
>> + T: HardwareOperations,
>> +{
>> + // SAFETY: By the safety requirement of this function `led_cdev` is embedded inside a `T::This<T>`.
>> + let led = unsafe { &mut *(T::This::led_container_of(led_cdev.cast())) };
>> + T::deactivate(led)
>> +}
>> --
>> 2.47.0
>>
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/5] rust: leds: Add hardware trigger support for hardware-controlled LEDs
2025-01-20 10:59 ` Fiona Behrens
@ 2025-01-20 14:18 ` Marek Behún
0 siblings, 0 replies; 17+ messages in thread
From: Marek Behún @ 2025-01-20 14:18 UTC (permalink / raw)
To: Fiona Behrens
Cc: Marek Behún, Miguel Ojeda, Alex Gaynor, Pavel Machek,
Lee Jones, Jean Delvare, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Mark Pearson, Peter Koch, rust-for-linux,
linux-leds, linux-kernel
On Mon, Jan 20, 2025 at 11:59:03AM +0100, Fiona Behrens wrote:
> >
> > This looks as if you are doing a Rust binding for struct led_trigger.
> > But you keep calling it Hardware trigger, which makes me thing that
> > you are confused about what is a LED trigger and what is a hardware
> > trigger.
> >
> > Why do you keep putting "Hardware" into the names of these symbols?
>
> The idea was to create a abstraction specific to writing a hardware trigger (or my understanding of what that is) and deal with the other
> trigger types later, to more separate the things on the rust side with e.g. the vtables for those.
> But my understanding might be wrong.
>
> (My broad understanding is what I did in the SE10 driver later, to tell the hardware to not present the LED to the kernel, but some other hardware wiring to a hardware thing that then drives the LED)
Looking at patch 5, you do:
#[vtable]
#[cfg(CONFIG_LEDS_TRIGGERS)]
impl triggers::HardwareOperations for LedSE10 {
fn activate(this: &mut Self::This) -> Result {
this.data.send_cmd(LedCommand::Trigger)
}
}
I think this naming "triggers::HardwareOperations" may cause confusion
in the future. I think that what you implement here should be called
LED Private Trigger, or something derived from that.
Using the work "hardware" may lead people to think that it means the
other mechanism, wherein we offload software LED triggers to hardware
(which is implemented via the `hw_control_*` members of
`struct led_classdev`).
>
> >
> > I fear that you may be confused about some stuff here. In order to
> > determine whether this is the case, could you answer the following
> > questions please?
>
> That might be right, thanks for trying to clear it up if that is the case.
>
> > - What is the purpose of `struct led_hw_trigger_type`?
> Marking a led that it has a private trigger that gives control of the LED to some hardware driver.
> > - What is the purpose of the `dummy` member of this struct? What
> > value should be assigned to it?
> From my understanding this is just to give the struct a size, so that it has a unique address in memory so the pointer value can be compared.
> > - If a LED class device (LED cdev) has the `trigger_type` member set
> > to NULL, which LED triggers will be listed in the sysfs `trigger`
> > file for this LED cdev? And which triggers will be listed if the
> > `trigger_type` member is not NULL?
> For null all generic triggers will be listed, and for some value all generic plus the specific trigger.
> > - Why does both `struct led_classdev` and `struct led_trigger` have
> > the `trigger_type` member?
> led_classdev has it to declare that it does have a led private trigger mode, and the led_trigger has it so the activate/deactive functions can be found.
>
> My research so far into how triggers work was mostly so that I can use the wwan module trigger on the SE10 board I have here, and therefore I did not look into how to write a generic led trigger usable on more then a specific led.
>
> Thanks a lot for clearing up possible misunderstandings,
> Fiona
OK it seems that you do indeed understand these.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2025-01-20 14:19 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-13 12:16 [PATCH v2 0/5] Rust LED Driver Abstractions and Lenovo SE10 Driver with DMI and I/O Port Support Fiona Behrens
2025-01-13 12:16 ` [PATCH v2 1/5] rust: dmi: Add abstractions for DMI Fiona Behrens
2025-01-13 12:16 ` [PATCH v2 2/5] rust: leds: Add Rust bindings for LED subsystem Fiona Behrens
2025-01-13 13:10 ` Miguel Ojeda
2025-01-13 12:16 ` [PATCH v2 3/5] rust: leds: Add hardware trigger support for hardware-controlled LEDs Fiona Behrens
2025-01-20 10:35 ` Marek Behún
2025-01-20 10:59 ` Fiona Behrens
2025-01-20 14:18 ` Marek Behún
2025-01-13 12:16 ` [PATCH v2 4/5] rust: add I/O port abstractions with resource management Fiona Behrens
2025-01-13 13:15 ` Daniel Almeida
2025-01-13 13:28 ` Fiona Behrens
2025-01-13 12:16 ` [PATCH v2 5/5] leds: leds_lenovo_se10: LED driver for Lenovo SE10 platform Fiona Behrens
2025-01-17 17:21 ` Mark Pearson
2025-01-17 17:31 ` Fiona Behrens
2025-01-17 18:02 ` Mark Pearson
2025-01-17 17:43 ` Miguel Ojeda
2025-01-20 10:47 ` Marek Behún
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox