Message-ID: <2rla4ga3v563gdjdg6fztyh6hardmxnnclfe667gnfs4icsiqo@eho3bcv53h7d>
Date: Mon, 20 Jan 2025 11:35:01 +0100
From: Marek Behún <kabel@...nel.org>
To: Fiona Behrens <me@...enk.dev>
Cc: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>, Jean Delvare <jdelvare@...e.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, Benno Lossin <benno.lossin@...ton.me>, 
	Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, 
	Trevor Gross <tmgross@...ch.edu>, Mark Pearson <mpearson-lenovo@...ebb.ca>, 
	Peter Koch <pkoch@...ovo.com>, rust-for-linux@...r.kernel.org, linux-leds@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/5] rust: leds: Add hardware trigger support for
 hardware-controlled LEDs

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@...enk.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@...hnology.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
> 
> 

Powered by blists - more mailing lists