[PATCH v2 0/2] rust: leds: add led classdev abstractions

[PATCH v2 0/2] rust: leds: add led classdev abstractions

[Markus Probst]

This patch series has previously been contained in
https://lore.kernel.org/rust-for-linux/20251008181027.662616-1-markus.probst@posteo.de/T/#t
which added a rust written led driver for a microcontroller via i2c.

As the reading and writing to the i2c client via the register!
macro has not been implemented yet [1], the patch series will only
contain the additional abstractions required.

[1] https://lore.kernel.org/rust-for-linux/DDDS2V0V2NVJ.16ZKXCKUA1HUV@kernel.org/

The following changes were made:
* add basic Pin<Vec<T, A>> abstractions, that allow to initialize PinInit items with
  the guarantee that these will never be moved.

* add basic led classdev abstractions to register and unregister leds

Changes since v1:
* fixed typos noticed by Onur Özkan

Markus Probst (2):
  rust: add basic Pin<Vec<T, A>> abstractions
  rust: leds: add basic led classdev abstractions

 rust/kernel/alloc.rs      |   1 +
 rust/kernel/alloc/kvec.rs |  86 +++++++++++
 rust/kernel/led.rs        | 296 ++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs        |   1 +
 rust/kernel/prelude.rs    |   2 +-
 5 files changed, 385 insertions(+), 1 deletion(-)
 create mode 100644 rust/kernel/led.rs

-- 
2.49.1

[PATCH v2 1/2] rust: add basic Pin<Vec<T, A>> abstractions

[Markus Probst]

Implement core Pin<Vec<T, A>> abstractions, including
 * `Vec::pin` and `Vec::into_pin` for constructing a `Pin<Vec<T, A>>`.
   If T does not implement `Unpin`, its values will never be moved.
 * an extension for `Pin<Vec<T, A>>` allowing PinInit to be initialied on a
   Pin<Vec>, as well as truncating and popping values from the Vec

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 rust/kernel/alloc.rs      |  1 +
 rust/kernel/alloc/kvec.rs | 86 +++++++++++++++++++++++++++++++++++++++
 rust/kernel/prelude.rs    |  2 +-
 3 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
index a2c49e5494d3..9c129eaf0625 100644
--- a/rust/kernel/alloc.rs
+++ b/rust/kernel/alloc.rs
@@ -24,6 +24,7 @@
 pub use self::kvec::KVec;
 pub use self::kvec::VVec;
 pub use self::kvec::Vec;
+pub use self::kvec::PinnedVecExt;
 
 /// Indicates an allocation error.
 #[derive(Copy, Clone, PartialEq, Eq, Debug)]
diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
index 3c72e0bdddb8..d5582a7f17e9 100644
--- a/rust/kernel/alloc/kvec.rs
+++ b/rust/kernel/alloc/kvec.rs
@@ -16,11 +16,13 @@
     ops::DerefMut,
     ops::Index,
     ops::IndexMut,
+    pin::Pin,
     ptr,
     ptr::NonNull,
     slice,
     slice::SliceIndex,
 };
+use pin_init::PinInit;
 
 mod errors;
 pub use self::errors::{InsertError, PushError, RemoveError};
@@ -109,6 +111,21 @@ pub struct Vec<T, A: Allocator> {
     _p: PhantomData<A>,
 }
 
+/// Extension for Pin<Vec<T, A>>
+pub trait PinnedVecExt<T> {
+    /// Pin-initializes P and appends it to the back of the [`Vec`] instance without reallocating.
+    fn push_pin_init<E: From<PushError<P>>, P: PinInit<T, E>>(&mut self, init: P) -> Result<(), E>;
+
+    /// Shortens the vector, setting the length to `len` and drops the removed values.
+    /// If `len` is greater than or equal to the current length, this does nothing.
+    ///
+    /// This has no effect on the capacity and will not allocate.
+    fn truncate(&mut self, len: usize);
+
+    /// Removes the last element from a vector and drops it returning true, or false if it is empty.
+    fn pop(&mut self) -> bool;
+}
+
 /// Type alias for [`Vec`] with a [`Kmalloc`] allocator.
 ///
 /// # Examples
@@ -719,6 +736,18 @@ pub fn retain(&mut self, mut f: impl FnMut(&mut T) -> bool) {
         }
         self.truncate(num_kept);
     }
+
+    /// Constructs a new `Pin<Vec<T, A>>`.
+    #[inline]
+    pub fn pin(capacity: usize, flags: Flags) -> Result<Pin<Self>, AllocError> {
+        Self::with_capacity(capacity, flags).map(Pin::<Self>::from)
+    }
+
+    /// Convert a [`Vec<T,A>`] to a [`Pin<Vec<T,A>>`]. If `T` does not implement
+    /// [`Unpin`], then its values will be pinned in memory and can't be moved.
+    pub fn into_pin(this: Self) -> Pin<Self> {
+        this.into()
+    }
 }
 
 impl<T: Clone, A: Allocator> Vec<T, A> {
@@ -1294,6 +1323,63 @@ fn drop(&mut self) {
     }
 }
 
+impl<T, A: Allocator> PinnedVecExt<T> for Pin<Vec<T, A>> {
+    fn truncate(&mut self, len: usize) {
+        // SAFETY: truncate will not reallocate the Vec
+        // CAST: Pin<Ptr> is a transparent wrapper of Ptr
+        unsafe { &mut *core::ptr::from_mut(self).cast::<Vec<T, A>>() }.truncate(len);
+    }
+
+    fn push_pin_init<E: From<PushError<P>>, P: PinInit<T, E>>(&mut self, init: P) -> Result<(), E> {
+        // SAFETY: capacity, spare_capacity_mut and inc_len will not
+        // reallocate the Vec.
+        // CAST: Pin<Ptr> is a transparent wrapper of Ptr
+        let this = unsafe { &mut *core::ptr::from_mut(self).cast::<Vec<T, A>>() };
+
+        if this.len() < this.capacity() {
+            let spare = this.spare_capacity_mut();
+            // SAFETY: the length is less than the capacity, so `spare` is non-empty.
+            unsafe { init.__pinned_init(spare.get_unchecked_mut(0).as_mut_ptr())? };
+            // SAFETY: We just initialised the first spare entry, so it is safe to
+            // increase the length by 1. We also know that the new length is <= capacity.
+            unsafe { this.inc_len(1) };
+            Ok(())
+        } else {
+            Err(E::from(PushError(init)))
+        }
+    }
+
+    fn pop(&mut self) -> bool {
+        if self.is_empty() {
+            return false;
+        }
+
+        // SAFETY:
+        // - We just checked that the length is at least one.
+        // - dec_len will not reallocate the Vec
+        // CAST: Pin<Ptr> is a transparent wrapper of Ptr
+        let ptr: *mut [T] = unsafe { (*core::ptr::from_mut(self).cast::<Vec<T, A>>()).dec_len(1) };
+
+        // SAFETY: the contract of `dec_len` guarantees that the elements in `ptr` are
+        // valid elements whose ownership has been transferred to the caller.
+        unsafe { ptr::drop_in_place(ptr) };
+        true
+    }
+}
+
+impl<T, A: Allocator> From<Vec<T, A>> for Pin<Vec<T, A>> {
+    /// Converts a `Vec<T, A>` into a `Pin<Vec<T, A>>`. If `T` does not implement [`Unpin`], then
+    /// every value in v will be pinned in memory and can't be moved.
+    ///
+    /// This moves `v` into `Pin` without moving any of the values of `v` or allocating and copying
+    /// any memory.
+    fn from(v: Vec<T, A>) -> Self {
+        // SAFETY: The values wrapped inside a `Pin<Vec<T, A>>` cannot be moved or replaced as long
+        // as `T` does not implement `Unpin`.
+        unsafe { Pin::new_unchecked(v) }
+    }
+}
+
 #[macros::kunit_tests(rust_kvec_kunit)]
 mod tests {
     use super::*;
diff --git a/rust/kernel/prelude.rs b/rust/kernel/prelude.rs
index 25fe97aafd02..7179e2ca2a14 100644
--- a/rust/kernel/prelude.rs
+++ b/rust/kernel/prelude.rs
@@ -19,7 +19,7 @@
     c_ushort, c_void,
 };
 
-pub use crate::alloc::{flags::*, Box, KBox, KVBox, KVVec, KVec, VBox, VVec, Vec};
+pub use crate::alloc::{flags::*, Box, KBox, KVBox, KVVec, KVec, PinnedVecExt, VBox, VVec, Vec};
 
 #[doc(no_inline)]
 pub use macros::{export, kunit_tests, module, vtable};
-- 
2.49.1

[PATCH v2 2/2] rust: leds: add basic led classdev abstractions

[Markus Probst]

Implement the core abstractions needed for led class devices, including:

* `led::Handler` - the trait for handling leds, including
  `brightness_set`

* `led::InitData` - data set for the led class device

* `led::Device` - a safe wrapper around `led_classdev`

Signed-off-by: Markus Probst <markus.probst@posteo.de>
---
 rust/kernel/led.rs | 296 +++++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs |   1 +
 2 files changed, 297 insertions(+)
 create mode 100644 rust/kernel/led.rs

diff --git a/rust/kernel/led.rs b/rust/kernel/led.rs
new file mode 100644
index 000000000000..2ceafedaae5a
--- /dev/null
+++ b/rust/kernel/led.rs
@@ -0,0 +1,296 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Abstractions for the leds driver model.
+//!
+//! C header: [`include/linux/leds.h`](srctree/include/linux/leds.h)
+
+use core::pin::Pin;
+
+use pin_init::{pin_data, pinned_drop, PinInit};
+
+use crate::{
+    alloc::KBox,
+    container_of,
+    device::{self, property::FwNode},
+    error::{code::EINVAL, from_result, to_result, Error, Result},
+    prelude::GFP_KERNEL,
+    str::CStr,
+    try_pin_init,
+    types::Opaque,
+};
+
+/// The led class device representation.
+///
+/// This structure represents the Rust abstraction for a C `struct led_classdev`.
+#[pin_data(PinnedDrop)]
+pub struct Device {
+    handler: KBox<dyn HandlerImpl>,
+    #[pin]
+    classdev: Opaque<bindings::led_classdev>,
+}
+
+/// The led init data representation.
+///
+/// This structure represents the Rust abstraction for a C `struct led_init_data`.
+#[derive(Default)]
+pub struct InitData<'a> {
+    fwnode: Option<&'a FwNode>,
+    default_label: Option<&'a CStr>,
+    devicename: Option<&'a CStr>,
+    devname_mandatory: bool,
+}
+
+impl InitData<'static> {
+    /// Creates a new [`InitData`]
+    pub fn new() -> Self {
+        Self::default()
+    }
+}
+
+impl<'a> InitData<'a> {
+    /// Sets the firmware node
+    pub fn fwnode<'b, 'c>(self, fwnode: &'b FwNode) -> InitData<'c>
+    where
+        'a: 'c,
+        'b: 'c,
+    {
+        InitData {
+            fwnode: Some(fwnode),
+            ..self
+        }
+    }
+
+    /// Sets a default label
+    pub fn default_label<'b, 'c>(self, label: &'b CStr) -> InitData<'c>
+    where
+        'a: 'c,
+        'b: 'c,
+    {
+        InitData {
+            default_label: Some(label),
+            ..self
+        }
+    }
+
+    /// Sets the device name
+    pub fn devicename<'b, 'c>(self, devicename: &'b CStr) -> InitData<'c>
+    where
+        'a: 'c,
+        'b: 'c,
+    {
+        InitData {
+            devicename: Some(devicename),
+            ..self
+        }
+    }
+
+    /// Sets if a device name is mandatory
+    pub fn devicename_mandatory(self, mandatory: bool) -> Self {
+        Self {
+            devname_mandatory: mandatory,
+
+            ..self
+        }
+    }
+}
+
+/// The led handler trait.
+///
+/// # Examples
+///
+///```
+/// # use kernel::{c_str, led, alloc::KBox, error::{Result, code::ENOSYS}};
+/// # use core::pin::Pin;
+///
+/// struct MyHandler;
+///
+///
+/// impl led::Handler for MyHandler {
+///     const BLOCKING = false;
+///     const MAX_BRIGHTNESS = 255;
+///
+///     fn brightness_set(&self, _brightness: u32) -> Result<()> {
+///         // Set the brightness for the led here
+///         Ok(())
+///     }
+/// }
+///
+/// fn register_my_led() -> Result<Pin<KBox<led::Device>>> {
+///     let handler = MyHandler;
+///     KBox::pin_init(led::Device::new(
+///         None,
+///         None,
+///         led::InitData::new()
+///             .default_label(c_str!("my_led")),
+///         handler,
+///     ))
+/// }
+///```
+/// Led drivers must implement this trait in order to register and handle a [`Device`].
+pub trait Handler {
+    /// If set true, [`Handler::brightness_set`] and [`Handler::blink_set`] must not sleep
+    /// and perform the operation immediately.
+    const BLOCKING: bool;
+    /// Set this to true, if [`Handler::blink_set`] is implemented.
+    const BLINK: bool = false;
+    /// The max brightness level
+    const MAX_BRIGHTNESS: u32;
+
+    /// Sets the brightness level
+    ///
+    /// See also [`Handler::BLOCKING`]
+    fn brightness_set(&self, brightness: u32) -> Result<()>;
+
+    /// Activates hardware accelerated blinking.
+    ///
+    /// delays are in milliseconds. If both are zero, a sensible default should be chosen.
+    /// The caller should adjust the timings in that case and if it can't match the values
+    /// specified exactly. Setting the brightness to 0 will disable the hardware accelerated
+    /// blinking.
+    ///
+    /// See also [`Handler::BLOCKING`]
+    fn blink_set(&self, _delay_on: &mut usize, _delay_off: &mut usize) -> Result<()> {
+        Err(EINVAL)
+    }
+}
+
+trait HandlerImpl {
+    fn brightness_set(&self, brightness: u32) -> Result<()>;
+    fn blink_set(&self, delay_on: &mut usize, delay_off: &mut usize) -> Result<()>;
+}
+
+impl<T: Handler> HandlerImpl for T {
+    fn brightness_set(&self, brightness: u32) -> Result<()> {
+        T::brightness_set(self, brightness)
+    }
+
+    fn blink_set(&self, delay_on: &mut usize, delay_off: &mut usize) -> Result<()> {
+        T::blink_set(self, delay_on, delay_off)
+    }
+}
+
+// SAFETY: A `led::Device` can be unregistered from any thread.
+unsafe impl Send for Device {}
+
+// SAFETY: `led::Device` can be shared among threads because all methods of `led::Device`
+// are thread safe.
+unsafe impl Sync for Device {}
+
+impl Device {
+    /// Registers a new led classdev.
+    ///
+    /// The [`Device`] will be unregistered and drop.
+    pub fn new<'a, T: Handler + 'static>(
+        parent: Option<&'a device::Device>,
+        init_data: InitData<'a>,
+        handler: T,
+    ) -> impl PinInit<Self, Error> + use<'a, T> {
+        try_pin_init!(Self {
+            handler <- {
+                let handler: KBox<dyn HandlerImpl> = KBox::<T>::new(handler, GFP_KERNEL)?;
+                Ok::<_, Error>(handler)
+            },
+            classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev| {
+                // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
+                unsafe { ptr.write(bindings::led_classdev {
+                    max_brightness: T::MAX_BRIGHTNESS,
+                    brightness_set: T::BLOCKING.then_some(
+                        brightness_set as unsafe extern "C" fn(*mut bindings::led_classdev, u32)
+                    ),
+                    brightness_set_blocking: (!T::BLOCKING).then_some(
+                        brightness_set_blocking
+                            as unsafe extern "C" fn(*mut bindings::led_classdev,u32) -> i32
+                    ),
+                    blink_set: T::BLINK.then_some(
+                        blink_set
+                            as unsafe extern "C" fn(*mut bindings::led_classdev, *mut usize,
+                                                    *mut usize) -> i32
+                    ),
+                    .. bindings::led_classdev::default()
+                }) };
+
+                let mut init_data = bindings::led_init_data {
+                    fwnode: init_data.fwnode.map_or(core::ptr::null_mut(), FwNode::as_raw),
+                    default_label: init_data.default_label
+                                            .map_or(core::ptr::null(), CStr::as_char_ptr),
+                    devicename: init_data.devicename.map_or(core::ptr::null(), CStr::as_char_ptr),
+                    devname_mandatory: init_data.devname_mandatory,
+                };
+
+                let parent = parent
+                    .map_or(core::ptr::null_mut(), device::Device::as_raw);
+
+                // SAFETY:
+                // - `parent` is guaranteed to be a pointer to a valid `device`
+                //    or a null pointer.
+                // - `ptr` is guaranteed to be a pointer to an initialized `led_classdev`.
+                to_result(unsafe {
+                    bindings::led_classdev_register_ext(parent, ptr, &mut init_data)
+                })
+            }),
+        })
+    }
+}
+
+extern "C" fn brightness_set(led_cdev: *mut bindings::led_classdev, brightness: u32) {
+    // SAFETY: `led_cdev` is a valid pointer to a `led_classdev` stored inside a `Device`.
+    let classdev = unsafe {
+        &*container_of!(
+            led_cdev.cast::<Opaque<bindings::led_classdev>>(),
+            Device,
+            classdev
+        )
+    };
+    let _ = classdev.handler.brightness_set(brightness);
+}
+
+extern "C" fn brightness_set_blocking(
+    led_cdev: *mut bindings::led_classdev,
+    brightness: u32,
+) -> i32 {
+    // SAFETY: `led_cdev` is a valid pointer to a `led_classdev` stored inside a `Device`.
+    let classdev = unsafe {
+        &*container_of!(
+            led_cdev.cast::<Opaque<bindings::led_classdev>>(),
+            Device,
+            classdev
+        )
+    };
+    from_result(|| {
+        classdev.handler.brightness_set(brightness)?;
+        Ok(0)
+    })
+}
+
+extern "C" fn blink_set(
+    led_cdev: *mut bindings::led_classdev,
+    delay_on: *mut usize,
+    delay_off: *mut usize,
+) -> i32 {
+    // SAFETY: `led_cdev` is a valid pointer to a `led_classdev` stored inside a `Device`.
+    let classdev = unsafe {
+        &*container_of!(
+            led_cdev.cast::<Opaque<bindings::led_classdev>>(),
+            Device,
+            classdev
+        )
+    };
+    from_result(|| {
+        classdev.handler.blink_set(
+            // SAFETY: `delay_on` is guaranteed to be a valid pointer to usize
+            unsafe { &mut *delay_on },
+            // SAFETY: `delay_on` is guaranteed to be a valid pointer to usize
+            unsafe { &mut *delay_off },
+        )?;
+        Ok(0)
+    })
+}
+
+#[pinned_drop]
+impl PinnedDrop for Device {
+    fn drop(self: Pin<&mut Self>) {
+        // SAFETY: The existence of `self` guarantees that `self.classdev` has previously been
+        // successfully registered with `led_classdev_register_ext`
+        unsafe { bindings::led_classdev_unregister(self.classdev.get()) };
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e5247f584ad2..f42c60da21ae 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -97,6 +97,7 @@
 pub mod jump_label;
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
+pub mod led;
 pub mod list;
 pub mod miscdevice;
 pub mod mm;
-- 
2.49.1

Re: [PATCH v2 1/2] rust: add basic Pin<Vec<T, A>> abstractions

[Alice Ryhl]

On Thu, Oct 09, 2025 at 06:12:33PM +0000, Markus Probst wrote:
> Implement core Pin<Vec<T, A>> abstractions, including
>  * `Vec::pin` and `Vec::into_pin` for constructing a `Pin<Vec<T, A>>`.
>    If T does not implement `Unpin`, its values will never be moved.
>  * an extension for `Pin<Vec<T, A>>` allowing PinInit to be initialied on a
>    Pin<Vec>, as well as truncating and popping values from the Vec
> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>
> ---
>  rust/kernel/alloc.rs      |  1 +
>  rust/kernel/alloc/kvec.rs | 86 +++++++++++++++++++++++++++++++++++++++
>  rust/kernel/prelude.rs    |  2 +-
>  3 files changed, 88 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> index a2c49e5494d3..9c129eaf0625 100644
> --- a/rust/kernel/alloc.rs
> +++ b/rust/kernel/alloc.rs
> @@ -24,6 +24,7 @@
>  pub use self::kvec::KVec;
>  pub use self::kvec::VVec;
>  pub use self::kvec::Vec;
> +pub use self::kvec::PinnedVecExt;
>  
>  /// Indicates an allocation error.
>  #[derive(Copy, Clone, PartialEq, Eq, Debug)]
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index 3c72e0bdddb8..d5582a7f17e9 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -16,11 +16,13 @@
>      ops::DerefMut,
>      ops::Index,
>      ops::IndexMut,
> +    pin::Pin,
>      ptr,
>      ptr::NonNull,
>      slice,
>      slice::SliceIndex,
>  };
> +use pin_init::PinInit;
>  
>  mod errors;
>  pub use self::errors::{InsertError, PushError, RemoveError};
> @@ -109,6 +111,21 @@ pub struct Vec<T, A: Allocator> {
>      _p: PhantomData<A>,
>  }
>  
> +/// Extension for Pin<Vec<T, A>>
> +pub trait PinnedVecExt<T> {
> +    /// Pin-initializes P and appends it to the back of the [`Vec`] instance without reallocating.
> +    fn push_pin_init<E: From<PushError<P>>, P: PinInit<T, E>>(&mut self, init: P) -> Result<(), E>;
> +
> +    /// Shortens the vector, setting the length to `len` and drops the removed values.
> +    /// If `len` is greater than or equal to the current length, this does nothing.
> +    ///
> +    /// This has no effect on the capacity and will not allocate.
> +    fn truncate(&mut self, len: usize);
> +
> +    /// Removes the last element from a vector and drops it returning true, or false if it is empty.
> +    fn pop(&mut self) -> bool;
> +}

Please no extension traits just for this. Just provide `self: Pin<&mut Self>`
methods on Vec.

Alice

Re: [PATCH v2 2/2] rust: leds: add basic led classdev abstractions

[Alice Ryhl]

On Thu, Oct 09, 2025 at 06:12:34PM +0000, Markus Probst wrote:
> Implement the core abstractions needed for led class devices, including:
> 
> * `led::Handler` - the trait for handling leds, including
>   `brightness_set`
> 
> * `led::InitData` - data set for the led class device
> 
> * `led::Device` - a safe wrapper around `led_classdev`
> 
> Signed-off-by: Markus Probst <markus.probst@posteo.de>

> +/// The led class device representation.
> +///
> +/// This structure represents the Rust abstraction for a C `struct led_classdev`.
> +#[pin_data(PinnedDrop)]
> +pub struct Device {
> +    handler: KBox<dyn HandlerImpl>,
> +    #[pin]
> +    classdev: Opaque<bindings::led_classdev>,
> +}

Is it not possible to use Device<T> with a `handler: T` field here? That
avoids the box. I don't think other abstractions have needed that. If
it's needed, then why is LED special?

> +/// The led handler trait.
> +///
> +/// # Examples
> +///
> +///```
> +/// # use kernel::{c_str, led, alloc::KBox, error::{Result, code::ENOSYS}};
> +/// # use core::pin::Pin;
> +///
> +/// struct MyHandler;
> +///
> +///
> +/// impl led::Handler for MyHandler {
> +///     const BLOCKING = false;
> +///     const MAX_BRIGHTNESS = 255;
> +///
> +///     fn brightness_set(&self, _brightness: u32) -> Result<()> {
> +///         // Set the brightness for the led here
> +///         Ok(())
> +///     }
> +/// }
> +///
> +/// fn register_my_led() -> Result<Pin<KBox<led::Device>>> {
> +///     let handler = MyHandler;
> +///     KBox::pin_init(led::Device::new(
> +///         None,
> +///         None,
> +///         led::InitData::new()
> +///             .default_label(c_str!("my_led")),
> +///         handler,
> +///     ))
> +/// }
> +///```
> +/// Led drivers must implement this trait in order to register and handle a [`Device`].
> +pub trait Handler {
> +    /// If set true, [`Handler::brightness_set`] and [`Handler::blink_set`] must not sleep
> +    /// and perform the operation immediately.
> +    const BLOCKING: bool;
> +    /// Set this to true, if [`Handler::blink_set`] is implemented.
> +    const BLINK: bool = false;

We have a macro called #[vtable] that automatically generates this kind
of constant for you. Please use it.

> +impl Device {
> +    /// Registers a new led classdev.
> +    ///
> +    /// The [`Device`] will be unregistered and drop.
> +    pub fn new<'a, T: Handler + 'static>(
> +        parent: Option<&'a device::Device>,
> +        init_data: InitData<'a>,
> +        handler: T,
> +    ) -> impl PinInit<Self, Error> + use<'a, T> {
> +        try_pin_init!(Self {
> +            handler <- {
> +                let handler: KBox<dyn HandlerImpl> = KBox::<T>::new(handler, GFP_KERNEL)?;
> +                Ok::<_, Error>(handler)
> +            },
> +            classdev <- Opaque::try_ffi_init(|ptr: *mut bindings::led_classdev| {
> +                // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
> +                unsafe { ptr.write(bindings::led_classdev {
> +                    max_brightness: T::MAX_BRIGHTNESS,
> +                    brightness_set: T::BLOCKING.then_some(
> +                        brightness_set as unsafe extern "C" fn(*mut bindings::led_classdev, u32)
> +                    ),
> +                    brightness_set_blocking: (!T::BLOCKING).then_some(
> +                        brightness_set_blocking
> +                            as unsafe extern "C" fn(*mut bindings::led_classdev,u32) -> i32
> +                    ),
> +                    blink_set: T::BLINK.then_some(
> +                        blink_set
> +                            as unsafe extern "C" fn(*mut bindings::led_classdev, *mut usize,
> +                                                    *mut usize) -> i32

I think you can just do `blink_set as _`.

> +                    ),
> +                    .. bindings::led_classdev::default()
> +                }) };
> +
> +                let mut init_data = bindings::led_init_data {
> +                    fwnode: init_data.fwnode.map_or(core::ptr::null_mut(), FwNode::as_raw),
> +                    default_label: init_data.default_label
> +                                            .map_or(core::ptr::null(), CStr::as_char_ptr),
> +                    devicename: init_data.devicename.map_or(core::ptr::null(), CStr::as_char_ptr),
> +                    devname_mandatory: init_data.devname_mandatory,
> +                };
> +
> +                let parent = parent
> +                    .map_or(core::ptr::null_mut(), device::Device::as_raw);
> +
> +                // SAFETY:
> +                // - `parent` is guaranteed to be a pointer to a valid `device`
> +                //    or a null pointer.
> +                // - `ptr` is guaranteed to be a pointer to an initialized `led_classdev`.
> +                to_result(unsafe {
> +                    bindings::led_classdev_register_ext(parent, ptr, &mut init_data)

So it's ok that `init_data` is valid for the duration of this call and
no longer? It doesn't stash a pointer to it anywhere?

> +extern "C" fn brightness_set(led_cdev: *mut bindings::led_classdev, brightness: u32) {
> +    // SAFETY: `led_cdev` is a valid pointer to a `led_classdev` stored inside a `Device`.
> +    let classdev = unsafe {
> +        &*container_of!(
> +            led_cdev.cast::<Opaque<bindings::led_classdev>>(),

Please use Opaque::cast_from instead.

> +    // SAFETY: `led_cdev` is a valid pointer to a `led_classdev` stored inside a `Device`.
> +    let classdev = unsafe {
> +        &*container_of!(
> +            led_cdev.cast::<Opaque<bindings::led_classdev>>(),
> +            Device,
> +            classdev
> +        )
> +    };

Instead of repeating this logic many times, I suggest a Device::from_raw
method.

> +extern "C" fn blink_set(
> +    led_cdev: *mut bindings::led_classdev,
> +    delay_on: *mut usize,
> +    delay_off: *mut usize,
> +) -> i32 {
> +    // SAFETY: `led_cdev` is a valid pointer to a `led_classdev` stored inside a `Device`.
> +    let classdev = unsafe {
> +        &*container_of!(
> +            led_cdev.cast::<Opaque<bindings::led_classdev>>(),
> +            Device,
> +            classdev
> +        )
> +    };
> +    from_result(|| {
> +        classdev.handler.blink_set(
> +            // SAFETY: `delay_on` is guaranteed to be a valid pointer to usize
> +            unsafe { &mut *delay_on },
> +            // SAFETY: `delay_on` is guaranteed to be a valid pointer to usize
> +            unsafe { &mut *delay_off },

To create a mutable reference, this safety comment should argue why it
is the case that nobody will access these fields for the duration of
`blink_set`. (For example, from another thread?)

Alice

Re: [PATCH v2 2/2] rust: leds: add basic led classdev abstractions

[Markus Probst]

On Fri, 2025-10-10 at 11:41 +0000, Alice Ryhl wrote:
> On Thu, Oct 09, 2025 at 06:12:34PM +0000, Markus Probst wrote:
> > Implement the core abstractions needed for led class devices,
> > including:
> > 
> > * `led::Handler` - the trait for handling leds, including
> >   `brightness_set`
> > 
> > * `led::InitData` - data set for the led class device
> > 
> > * `led::Device` - a safe wrapper around `led_classdev`
> > 
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> 
> > +/// The led class device representation.
> > +///
> > +/// This structure represents the Rust abstraction for a C `struct
> > led_classdev`.
> > +#[pin_data(PinnedDrop)]
> > +pub struct Device {
> > +    handler: KBox<dyn HandlerImpl>,
> > +    #[pin]
> > +    classdev: Opaque<bindings::led_classdev>,
> > +}
> 
> Is it not possible to use Device<T> with a `handler: T` field here?
> That
> avoids the box. I don't think other abstractions have needed that. If
> it's needed, then why is LED special?
The handler variable is defined, because leds usually store additional
data. If an led driver has multiple leds, it needs to know which one,
without defining for each led its own struct. It also needs access to
resources which allows it to control the leds, e.g. I2cClient (hasn't
been merged yet), Io and so on.

It is possible to store handler: T, but I would not know a way to call
it dynamically, because T is unknown at brightness_set,
brightness_set_blocking and blink_set methods.

I could maybe work it around by creating unsafe extern "C" methods with
method body pre-defined in the Handler trait, which is then used to
reference brightness_set etc. directly though.

> 
> > +/// The led handler trait.
> > +///
> > +/// # Examples
> > +///
> > +///```
> > +/// # use kernel::{c_str, led, alloc::KBox, error::{Result,
> > code::ENOSYS}};
> > +/// # use core::pin::Pin;
> > +///
> > +/// struct MyHandler;
> > +///
> > +///
> > +/// impl led::Handler for MyHandler {
> > +///     const BLOCKING = false;
> > +///     const MAX_BRIGHTNESS = 255;
> > +///
> > +///     fn brightness_set(&self, _brightness: u32) -> Result<()> {
> > +///         // Set the brightness for the led here
> > +///         Ok(())
> > +///     }
> > +/// }
> > +///
> > +/// fn register_my_led() -> Result<Pin<KBox<led::Device>>> {
> > +///     let handler = MyHandler;
> > +///     KBox::pin_init(led::Device::new(
> > +///         None,
> > +///         None,
> > +///         led::InitData::new()
> > +///             .default_label(c_str!("my_led")),
> > +///         handler,
> > +///     ))
> > +/// }
> > +///```
> > +/// Led drivers must implement this trait in order to register and
> > handle a [`Device`].
> > +pub trait Handler {
> > +    /// If set true, [`Handler::brightness_set`] and
> > [`Handler::blink_set`] must not sleep
> > +    /// and perform the operation immediately.
> > +    const BLOCKING: bool;
> > +    /// Set this to true, if [`Handler::blink_set`] is
> > implemented.
> > +    const BLINK: bool = false;
> 
> We have a macro called #[vtable] that automatically generates this
> kind
> of constant for you. Please use it.
> 
> > +impl Device {
> > +    /// Registers a new led classdev.
> > +    ///
> > +    /// The [`Device`] will be unregistered and drop.
> > +    pub fn new<'a, T: Handler + 'static>(
> > +        parent: Option<&'a device::Device>,
> > +        init_data: InitData<'a>,
> > +        handler: T,
> > +    ) -> impl PinInit<Self, Error> + use<'a, T> {
> > +        try_pin_init!(Self {
> > +            handler <- {
> > +                let handler: KBox<dyn HandlerImpl> =
> > KBox::<T>::new(handler, GFP_KERNEL)?;
> > +                Ok::<_, Error>(handler)
> > +            },
> > +            classdev <- Opaque::try_ffi_init(|ptr: *mut
> > bindings::led_classdev| {
> > +                // SAFETY: `try_ffi_init` guarantees that `ptr` is
> > valid for write.
> > +                unsafe { ptr.write(bindings::led_classdev {
> > +                    max_brightness: T::MAX_BRIGHTNESS,
> > +                    brightness_set: T::BLOCKING.then_some(
> > +                        brightness_set as unsafe extern "C"
> > fn(*mut bindings::led_classdev, u32)
> > +                    ),
> > +                    brightness_set_blocking:
> > (!T::BLOCKING).then_some(
> > +                        brightness_set_blocking
> > +                            as unsafe extern "C" fn(*mut
> > bindings::led_classdev,u32) -> i32
> > +                    ),
> > +                    blink_set: T::BLINK.then_some(
> > +                        blink_set
> > +                            as unsafe extern "C" fn(*mut
> > bindings::led_classdev, *mut usize,
> > +                                                    *mut usize) ->
> > i32
> 
> I think you can just do `blink_set as _`.
> 
> > +                    ),
> > +                    .. bindings::led_classdev::default()
> > +                }) };
> > +
> > +                let mut init_data = bindings::led_init_data {
> > +                    fwnode:
> > init_data.fwnode.map_or(core::ptr::null_mut(), FwNode::as_raw),
> > +                    default_label: init_data.default_label
> > +                                           
> > .map_or(core::ptr::null(), CStr::as_char_ptr),
> > +                    devicename:
> > init_data.devicename.map_or(core::ptr::null(), CStr::as_char_ptr),
> > +                    devname_mandatory:
> > init_data.devname_mandatory,
> > +                };
> > +
> > +                let parent = parent
> > +                    .map_or(core::ptr::null_mut(),
> > device::Device::as_raw);
> > +
> > +                // SAFETY:
> > +                // - `parent` is guaranteed to be a pointer to a
> > valid `device`
> > +                //    or a null pointer.
> > +                // - `ptr` is guaranteed to be a pointer to an
> > initialized `led_classdev`.
> > +                to_result(unsafe {
> > +                    bindings::led_classdev_register_ext(parent,
> > ptr, &mut init_data)
> 
> So it's ok that `init_data` is valid for the duration of this call
> and
> no longer? It doesn't stash a pointer to it anywhere?
init_data->parent has a reference count (it exists as long as the
led_classdev)
init_data->devicename will only be used on register (to build the
led_classdev name alongside other parameters)
init_data->fwnode will be accessed on register and stored in
led_classdev. I looked at the function again, to my surpris it does not
increment the reference count of fwnode, so this could be an issue.

> 
> > +extern "C" fn brightness_set(led_cdev: *mut
> > bindings::led_classdev, brightness: u32) {
> > +    // SAFETY: `led_cdev` is a valid pointer to a `led_classdev`
> > stored inside a `Device`.
> > +    let classdev = unsafe {
> > +        &*container_of!(
> > +            led_cdev.cast::<Opaque<bindings::led_classdev>>(),
> 
> Please use Opaque::cast_from instead.
> 
> > +    // SAFETY: `led_cdev` is a valid pointer to a `led_classdev`
> > stored inside a `Device`.
> > +    let classdev = unsafe {
> > +        &*container_of!(
> > +            led_cdev.cast::<Opaque<bindings::led_classdev>>(),
> > +            Device,
> > +            classdev
> > +        )
> > +    };
> 
> Instead of repeating this logic many times, I suggest a
> Device::from_raw
> method.
> 
> > +extern "C" fn blink_set(
> > +    led_cdev: *mut bindings::led_classdev,
> > +    delay_on: *mut usize,
> > +    delay_off: *mut usize,
> > +) -> i32 {
> > +    // SAFETY: `led_cdev` is a valid pointer to a `led_classdev`
> > stored inside a `Device`.
> > +    let classdev = unsafe {
> > +        &*container_of!(
> > +            led_cdev.cast::<Opaque<bindings::led_classdev>>(),
> > +            Device,
> > +            classdev
> > +        )
> > +    };
> > +    from_result(|| {
> > +        classdev.handler.blink_set(
> > +            // SAFETY: `delay_on` is guaranteed to be a valid
> > pointer to usize
> > +            unsafe { &mut *delay_on },
> > +            // SAFETY: `delay_on` is guaranteed to be a valid
> > pointer to usize
> > +            unsafe { &mut *delay_off },
> 
> To create a mutable reference, this safety comment should argue why
> it
> is the case that nobody will access these fields for the duration of
> `blink_set`. (For example, from another thread?)
> 
> Alice

Thanks
- Markus Probst

Re: [PATCH v2 1/2] rust: add basic Pin<Vec<T, A>> abstractions

[Markus Probst]

I might be wrong here, but as far as I know, it is not possible to get
a `Pin<&mut Vec<T, A>>` from a `&mut Pin<Vec<T, A>>`, only a `&mut
Pin<&mut [T, A]>` which is not usable by any of these methods.

Thanks
- Markus Probst

On Fri, 2025-10-10 at 11:33 +0000, Alice Ryhl wrote:
> On Thu, Oct 09, 2025 at 06:12:33PM +0000, Markus Probst wrote:
> > Implement core Pin<Vec<T, A>> abstractions, including
> >  * `Vec::pin` and `Vec::into_pin` for constructing a `Pin<Vec<T,
> > A>>`.
> >    If T does not implement `Unpin`, its values will never be moved.
> >  * an extension for `Pin<Vec<T, A>>` allowing PinInit to be
> > initialied on a
> >    Pin<Vec>, as well as truncating and popping values from the Vec
> > 
> > Signed-off-by: Markus Probst <markus.probst@posteo.de>
> > ---
> >  rust/kernel/alloc.rs      |  1 +
> >  rust/kernel/alloc/kvec.rs | 86
> > +++++++++++++++++++++++++++++++++++++++
> >  rust/kernel/prelude.rs    |  2 +-
> >  3 files changed, 88 insertions(+), 1 deletion(-)
> > 
> > diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
> > index a2c49e5494d3..9c129eaf0625 100644
> > --- a/rust/kernel/alloc.rs
> > +++ b/rust/kernel/alloc.rs
> > @@ -24,6 +24,7 @@
> >  pub use self::kvec::KVec;
> >  pub use self::kvec::VVec;
> >  pub use self::kvec::Vec;
> > +pub use self::kvec::PinnedVecExt;
> >  
> >  /// Indicates an allocation error.
> >  #[derive(Copy, Clone, PartialEq, Eq, Debug)]
> > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > index 3c72e0bdddb8..d5582a7f17e9 100644
> > --- a/rust/kernel/alloc/kvec.rs
> > +++ b/rust/kernel/alloc/kvec.rs
> > @@ -16,11 +16,13 @@
> >      ops::DerefMut,
> >      ops::Index,
> >      ops::IndexMut,
> > +    pin::Pin,
> >      ptr,
> >      ptr::NonNull,
> >      slice,
> >      slice::SliceIndex,
> >  };
> > +use pin_init::PinInit;
> >  
> >  mod errors;
> >  pub use self::errors::{InsertError, PushError, RemoveError};
> > @@ -109,6 +111,21 @@ pub struct Vec<T, A: Allocator> {
> >      _p: PhantomData<A>,
> >  }
> >  
> > +/// Extension for Pin<Vec<T, A>>
> > +pub trait PinnedVecExt<T> {
> > +    /// Pin-initializes P and appends it to the back of the
> > [`Vec`] instance without reallocating.
> > +    fn push_pin_init<E: From<PushError<P>>, P: PinInit<T, E>>(&mut
> > self, init: P) -> Result<(), E>;
> > +
> > +    /// Shortens the vector, setting the length to `len` and drops
> > the removed values.
> > +    /// If `len` is greater than or equal to the current length,
> > this does nothing.
> > +    ///
> > +    /// This has no effect on the capacity and will not allocate.
> > +    fn truncate(&mut self, len: usize);
> > +
> > +    /// Removes the last element from a vector and drops it
> > returning true, or false if it is empty.
> > +    fn pop(&mut self) -> bool;
> > +}
> 
> Please no extension traits just for this. Just provide `self:
> Pin<&mut Self>`
> methods on Vec.
> 
> Alice