[PATCH v6 00/14] hrtimer Rust API

[PATCH v6 00/14] hrtimer Rust API

[Andreas Hindborg]

This series adds support for using the `hrtimer` subsystem from Rust code.

The series adds support for timer mode and clock source configuration during
timer initialization. Examples and functionality to execute closures at timer
expiration has been removed, as these depend on either atomics [3] or
`SpinLockIrq` [4], which are still being worked on.

This series is a dependency for unmerged features of the Rust null block driver
[1], and for rkvms [2].

Link: https://git.kernel.org/pub/scm/linux/kernel/git/a.hindborg/linux.git/log/?h=rnull-v6.11-rc2 [1]
Link: https://gitlab.freedesktop.org/lyudess/linux/-/tree/rvkms-wip [2]
Link: https://lore.kernel.org/rust-for-linux/20240612223025.1158537-1-boqun.feng@gmail.com/ [3]
Link: https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@redhat.com/ [4]
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
Changes in v6:
- prefix all hrtimer related type names with `Hr`
- add a few links for type names in the documentation
- Link to v5: https://lore.kernel.org/r/20241217-hrtimer-v3-v6-12-rc2-v5-0-b34c20ac2cb7@kernel.org

Changes in v5:
- Fix a typo in `impl_has_timer`
- Implement `Box::into_pin` in terms of `impl From<Box> for Pin<Box>`
- Link to v4: https://lore.kernel.org/r/20241206-hrtimer-v3-v6-12-rc2-v4-0-6cb8c3673682@kernel.org

Changes in v4:
- rebase on v6.13-rc1 and adapt to kernel `Box`
- add a missing safety comment to `hrtimer::start`
- use `hrtimer_setup`
- fix a build issue when `bindings::hrtimer_restart` is signed
- fix a memory leak where box was not destroyed
- fix a documentation typo
- remove `as` coercion at multiple locations
- use fully qualified syntax when invoking `deref`
- move `hrtimer` into `time` module
- Link to v3: https://lore.kernel.org/r/20241017-hrtimer-v3-v6-12-rc2-v3-0-59a75cbb44da@kernel.org

Changes in v3:
- support timer mode selection
- support clock source selection
- eliminate `Arc::clone_from_raw` in favor of using `ArcBorrow`
- make `Arc::as_ptr` an associated method
- update safety requirement for `ArcBorrow::from_raw`
- remove examples (pending `SpinLockIrq` and `CondVar` patches)
- remove `start_function` (v2 `schedule_function`, pending `SpinLockIrq` and `CondVar` patches)
- change function naming from schedule/armed to start/running
- add vocabulary to documentation
- update safety comment in `Arc::as_ptr`
- Link to v2: https://lore.kernel.org/r/20240917222739.1298275-1-a.hindborg@kernel.org

Changes in v2:
- use a handle to own the timer callback target
- add ability to for callback to reschedule timer
- improve `impl_has_timer` to allow generics
- add support for stack allocated timers
- add support for scheduling closures
- use `Ktime` for setting expiration
- use `CondVar` instead of `AtomicBool` in examples
- rebase on 6.11
- improve documentation
- Link to v1: https://lore.kernel.org/r/20240425094634.262674-1-nmi@metaspace.dk

---
Andreas Hindborg (13):
      rust: hrtimer: introduce hrtimer support
      rust: sync: add `Arc::as_ptr`
      rust: hrtimer: implement `HrTimerPointer` for `Arc`
      rust: hrtimer: allow timer restart from timer handler
      rust: hrtimer: add `UnsafeHrTimerPointer`
      rust: hrtimer: add `hrtimer::ScopedHrTimerPointer`
      rust: hrtimer: implement `UnsafeHrTimerPointer` for `Pin<&T>`
      rust: hrtimer: implement `UnsafeHrTimerPointer` for `Pin<&mut T>`
      rust: alloc: add `Box::into_pin`
      rust: hrtimer: implement `HrTimerPointer` for `Pin<Box<T>>`
      rust: hrtimer: add `HrTimerMode`
      rust: hrtimer: add clocksource selection through `ClockSource`
      rust: hrtimer: add maintainer entry

Lyude Paul (1):
      rust: time: Add Ktime::from_ns()

 MAINTAINERS                         |  10 +
 rust/kernel/alloc/kbox.rs           |   6 +
 rust/kernel/sync/arc.rs             |  13 +-
 rust/kernel/time.rs                 |  10 +
 rust/kernel/time/hrtimer.rs         | 529 ++++++++++++++++++++++++++++++++++++
 rust/kernel/time/hrtimer/arc.rs     |  87 ++++++
 rust/kernel/time/hrtimer/pin.rs     |  95 +++++++
 rust/kernel/time/hrtimer/pin_mut.rs |  97 +++++++
 rust/kernel/time/hrtimer/tbox.rs    | 102 +++++++
 9 files changed, 947 insertions(+), 2 deletions(-)
---
base-commit: 8093380ff7bf546cf202531729705208911b87f1
change-id: 20241017-hrtimer-v3-v6-12-rc2-215dc6b169bf

Best regards,
-- 
Andreas Hindborg <a.hindborg@kernel.org>

[PATCH v6 01/14] rust: time: Add Ktime::from_ns()

[Andreas Hindborg]

From: Lyude Paul <lyude@redhat.com>

A simple function to turn the provided value in nanoseconds into a Ktime
value. We allow any type which implements Into<bindings::ktime_t>, which
resolves to Into<i64>.

This is useful for some of the older DRM APIs that never got moved to Ktime

Signed-off-by: Lyude Paul <lyude@redhat.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time.rs | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 379c0f5772e575c9ceacb9c85255b13501db8f30..f59e0fea79d3acfddd922f601f569353609aeec1 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -8,6 +8,8 @@
 //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
 //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
 
+use core::convert::Into;
+
 /// The number of nanoseconds per millisecond.
 pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
 
@@ -63,6 +65,12 @@ pub fn to_ns(self) -> i64 {
     pub fn to_ms(self) -> i64 {
         self.divns_constant::<NSEC_PER_MSEC>()
     }
+
+    /// Creates a new Ktime from the given duration in nanoseconds
+    #[inline]
+    pub fn from_ns(ns: impl Into<bindings::ktime_t>) -> Self {
+        Self { inner: ns.into() }
+    }
 }
 
 /// Returns the number of milliseconds between two ktimes.

-- 
2.47.0

Re: [PATCH v6 01/14] rust: time: Add Ktime::from_ns()

[Tamir Duberstein]

On Fri, Jan 10, 2025 at 3:20 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> From: Lyude Paul <lyude@redhat.com>
>
> A simple function to turn the provided value in nanoseconds into a Ktime
> value. We allow any type which implements Into<bindings::ktime_t>, which
> resolves to Into<i64>.
>
> This is useful for some of the older DRM APIs that never got moved to Ktime

Missing period.

> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/time.rs | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 379c0f5772e575c9ceacb9c85255b13501db8f30..f59e0fea79d3acfddd922f601f569353609aeec1 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -8,6 +8,8 @@
>  //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
>  //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
>
> +use core::convert::Into;
> +
>  /// The number of nanoseconds per millisecond.
>  pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
>
> @@ -63,6 +65,12 @@ pub fn to_ns(self) -> i64 {
>      pub fn to_ms(self) -> i64 {
>          self.divns_constant::<NSEC_PER_MSEC>()
>      }
> +
> +    /// Creates a new Ktime from the given duration in nanoseconds
> +    #[inline]
> +    pub fn from_ns(ns: impl Into<bindings::ktime_t>) -> Self {
> +        Self { inner: ns.into() }
> +    }
>  }

Should this be called `from_nanos` to be consistent with
`core::time::Duration::from_nanos`?

>
>  /// Returns the number of milliseconds between two ktimes.
>
> --
> 2.47.0

Re: [PATCH v6 01/14] rust: time: Add Ktime::from_ns()

[Andreas Hindborg]

"Tamir Duberstein" <tamird@gmail.com> writes:

> On Fri, Jan 10, 2025 at 3:20 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> From: Lyude Paul <lyude@redhat.com>
>>
>> A simple function to turn the provided value in nanoseconds into a Ktime
>> value. We allow any type which implements Into<bindings::ktime_t>, which
>> resolves to Into<i64>.
>>
>> This is useful for some of the older DRM APIs that never got moved to Ktime
>
> Missing period.
>
>> Signed-off-by: Lyude Paul <lyude@redhat.com>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/time.rs | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
>> index 379c0f5772e575c9ceacb9c85255b13501db8f30..f59e0fea79d3acfddd922f601f569353609aeec1 100644
>> --- a/rust/kernel/time.rs
>> +++ b/rust/kernel/time.rs
>> @@ -8,6 +8,8 @@
>>  //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffiesh).
>>  //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
>>
>> +use core::convert::Into;
>> +
>>  /// The number of nanoseconds per millisecond.
>>  pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
>>
>> @@ -63,6 +65,12 @@ pub fn to_ns(self) -> i64 {
>>      pub fn to_ms(self) -> i64 {
>>          self.divns_constant::<NSEC_PER_MSEC>()
>>      }
>> +
>> +    /// Creates a new Ktime from the given duration in nanoseconds
>> +    #[inline]
>> +    pub fn from_ns(ns: impl Into<bindings::ktime_t>) -> Self {
>> +        Self { inner: ns.into() }
>> +    }
>>  }
>
> Should this be called `from_nanos` to be consistent with
> `core::time::Duration::from_nanos`?

I am OK with that. @Lyude you OK with me changing this?


Best regards,
Andreas Hindborg

[PATCH v6 02/14] rust: hrtimer: introduce hrtimer support

[Andreas Hindborg]

This patch adds support for intrusive use of the hrtimer system. For now,
only one timer can be embedded in a Rust struct.

The hrtimer Rust API is based on the intrusive style pattern introduced by
the Rust workqueue API.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time.rs         |   2 +
 rust/kernel/time/hrtimer.rs | 296 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 298 insertions(+)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index f59e0fea79d3acfddd922f601f569353609aeec1..51c3532eee0184495ed5b7d717860c9980ff2a43 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -10,6 +10,8 @@
 
 use core::convert::Into;
 
+pub mod hrtimer;
+
 /// The number of nanoseconds per millisecond.
 pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
 
diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
new file mode 100644
index 0000000000000000000000000000000000000000..31c461ddd2c377101ed6c1c7e009f7dccf458ebc
--- /dev/null
+++ b/rust/kernel/time/hrtimer.rs
@@ -0,0 +1,296 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Intrusive high resolution timers.
+//!
+//! Allows running timer callbacks without doing allocations at the time of
+//! starting the timer. For now, only one timer per type is allowed.
+//!
+//! # Vocabulary
+//!
+//! A timer is initialized in the **stopped** state. A stopped timer can be
+//! **started** with an **expiry** time. After the timer is started, it is
+//! **running**. When the timer **expires**, the timer handler is executed.
+//! After the handler has executed, the timer may be **restarted** or
+//! **stopped**. A running timer can be **cancelled** before it's handler is
+//! executed. A timer that is cancelled enters the **stopped** state.
+//!
+//! States:
+//!
+//! * Stopped
+//! * Running
+//!
+//! Operations:
+//!
+//! * Start
+//! * Cancel
+//! * Stop
+//! * Restart
+//!
+//! Events:
+//!
+//! * Expire
+
+use crate::{init::PinInit, prelude::*, time::Ktime, types::Opaque};
+use core::marker::PhantomData;
+
+/// A timer backed by a C `struct hrtimer`.
+///
+/// # Invariants
+///
+/// * `self.timer` is initialized by `bindings::hrtimer_setup`.
+#[pin_data]
+#[repr(C)]
+pub struct HrTimer<U> {
+    #[pin]
+    timer: Opaque<bindings::hrtimer>,
+    _t: PhantomData<U>,
+}
+
+// SAFETY: A `HrTimer` can be moved to other threads and used/dropped from there.
+unsafe impl<U> Send for HrTimer<U> {}
+
+// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
+// timer from multiple threads
+unsafe impl<U> Sync for HrTimer<U> {}
+
+impl<T> HrTimer<T> {
+    /// Return an initializer for a new timer instance.
+    pub fn new() -> impl PinInit<Self>
+    where
+        T: HrTimerCallback,
+    {
+        pin_init!(Self {
+            // INVARIANTS: We initialize `timer` with `hrtimer_setup` below.
+            timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
+                // SAFETY: By design of `pin_init!`, `place` is a pointer live
+                // allocation. hrtimer_setup will initialize `place` and does
+                // not require `place` to be initialized prior to the call.
+                unsafe {
+                    bindings::hrtimer_setup(
+                        place,
+                        Some(T::CallbackTarget::run),
+                        bindings::CLOCK_MONOTONIC as i32,
+                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
+                    );
+                }
+            }),
+            _t: PhantomData,
+        })
+    }
+
+    /// Get a pointer to the contained `bindings::hrtimer`.
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must point to a live allocation of at least the size of `Self`.
+    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
+        // SAFETY: The field projection to `timer` does not go out of bounds,
+        // because the caller of this function promises that `ptr` points to an
+        // allocation of at least the size of `Self`.
+        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
+    }
+
+    /// Cancel an initialized and potentially running timer.
+    ///
+    /// If the timer handler is running, this will block until the handler is
+    /// finished.
+    ///
+    /// # Safety
+    ///
+    /// `self_ptr` must point to a valid `Self`.
+    #[allow(dead_code)]
+    pub(crate) unsafe fn raw_cancel(self_ptr: *const Self) -> bool {
+        // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
+        let c_timer_ptr = unsafe { HrTimer::raw_get(self_ptr) };
+
+        // If handler is running, this will wait for handler to finish before
+        // returning.
+        // SAFETY: `c_timer_ptr` is initialized and valid. Synchronization is
+        // handled on C side.
+        unsafe { bindings::hrtimer_cancel(c_timer_ptr) != 0 }
+    }
+}
+
+/// Implemented by pointer types that point to structs that embed a [`HrTimer`].
+///
+/// Typical implementers would be [`Box<T>`], [`Arc<T>`], [`ARef<T>`] where `T`
+/// has a field of type [`HrTimer`].
+///
+/// Target must be [`Sync`] because timer callbacks happen in another thread of
+/// execution (hard or soft interrupt context).
+///
+/// Starting a timer returns a [`HrTimerHandle`] that can be used to manipulate
+/// the timer. Note that it is OK to call the start function repeatedly, and
+/// that more than one [`HrTimerHandle`] associated with a [`HrTimerPointer`] may
+/// exist. A timer can be manipulated through any of the handles, and a handle
+/// may represent a cancelled timer.
+///
+/// [`Box<T>`]: Box
+/// [`Arc<T>`]: crate::sync::Arc
+/// [`ARef<T>`]: crate::types::ARef
+pub trait HrTimerPointer: Sync + Sized {
+    /// A handle representing a running timer.
+    ///
+    /// If the timer is running or if the timer callback is executing when the
+    /// handle is dropped, the drop method of [`HrTimerHandle`] should not return
+    /// until the timer is stopped and the callback has completed.
+    ///
+    /// Note: It must be safe to leak the handle.
+    type TimerHandle: HrTimerHandle;
+
+    /// Start the timer with expiry after `expires` time units. If the timer was
+    /// already running, it is restarted with the new expiry time.
+    fn start(self, expires: Ktime) -> Self::TimerHandle;
+}
+
+/// Implemented by [`HrTimerPointer`] implementers to give the C timer callback a
+/// function to call.
+// This is split from `HrTimerPointer` to make it easier to specify trait bounds.
+pub trait RawHrTimerCallback {
+    /// Callback to be called from C when timer fires.
+    ///
+    /// # Safety
+    ///
+    /// Only to be called by C code in `hrtimer` subsystem. `ptr` must point to
+    /// the `bindings::hrtimer` structure that was used to start the timer.
+    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
+}
+
+/// Implemented by structs that can the target of a timer callback.
+pub trait HrTimerCallback {
+    /// The type that was used for starting the timer.
+    type CallbackTarget<'a>: RawHrTimerCallback;
+
+    /// This type is passed to the timer callback function. It may be a borrow
+    /// of [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
+    /// implementation can guarantee exclusive access to the target during timer
+    /// handler execution.
+    type CallbackTargetParameter<'a>;
+
+    /// Called by the timer logic when the timer fires.
+    fn run(this: Self::CallbackTargetParameter<'_>)
+    where
+        Self: Sized;
+}
+
+/// A handle representing a potentially running timer.
+///
+/// More than one handle representing the same timer might exist.
+///
+/// # Safety
+///
+/// When dropped, the timer represented by this handle must be cancelled, if it
+/// is running. If the timer handler is running when the handle is dropped, the
+/// drop method must wait for the handler to finish before returning.
+pub unsafe trait HrTimerHandle {
+    /// Cancel the timer, if it is running. If the timer handler is running, block
+    /// till the handler has finished.
+    fn cancel(&mut self) -> bool;
+}
+
+/// Implemented by structs that contain timer nodes.
+///
+/// Clients of the timer API would usually safely implement this trait by using
+/// the [`crate::impl_has_hr_timer`] macro.
+///
+/// # Safety
+///
+/// Implementers of this trait must ensure that the implementer has a [`HrTimer`]
+/// field at the offset specified by `OFFSET` and that all trait methods are
+/// implemented according to their documentation.
+///
+/// [`impl_has_timer`]: crate::impl_has_timer
+pub unsafe trait HasHrTimer<U> {
+    /// Offset of the [`HrTimer`] field within `Self`
+    const OFFSET: usize;
+
+    /// Return a pointer to the [`HrTimer`] within `Self`.
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must point to a valid struct of type `Self`.
+    unsafe fn raw_get_timer(ptr: *const Self) -> *const HrTimer<U> {
+        // SAFETY: By the safety requirement of this trait, the trait
+        // implementor will have a `HrTimer` field at the specified offset.
+        unsafe { ptr.cast::<u8>().add(Self::OFFSET).cast::<HrTimer<U>>() }
+    }
+
+    /// Return a pointer to the struct that is embedding the [`HrTimer`] pointed
+    /// to by `ptr`.
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must point to a [`HrTimer<U>`] field in a struct of type `Self`.
+    unsafe fn timer_container_of(ptr: *mut HrTimer<U>) -> *mut Self
+    where
+        Self: Sized,
+    {
+        // SAFETY: By the safety requirement of this function and the `HasHrTimer`
+        // trait, the following expression will yield a pointer to the `Self`
+        // containing the timer addressed by `ptr`.
+        unsafe { ptr.cast::<u8>().sub(Self::OFFSET).cast::<Self>() }
+    }
+
+    /// Get pointer to embedded `bindings::hrtimer` struct.
+    ///
+    /// # Safety
+    ///
+    /// `self_ptr` must point to a valid `Self`.
+    unsafe fn c_timer_ptr(self_ptr: *const Self) -> *const bindings::hrtimer {
+        // SAFETY: `self_ptr` is a valid pointer to a `Self`.
+        let timer_ptr = unsafe { Self::raw_get_timer(self_ptr) };
+
+        // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
+        unsafe { HrTimer::raw_get(timer_ptr) }
+    }
+
+    /// Start the timer contained in the `Self` pointed to by `self_ptr`. If
+    /// it is already running it is removed and inserted.
+    ///
+    /// # Safety
+    ///
+    /// `self_ptr` must point to a valid `Self`.
+    unsafe fn start(self_ptr: *const Self, expires: Ktime) {
+        // SAFETY: By function safety requirement, `self_ptr`is a valid `Self`.
+        unsafe {
+            bindings::hrtimer_start_range_ns(
+                Self::c_timer_ptr(self_ptr).cast_mut(),
+                expires.to_ns(),
+                0,
+                bindings::hrtimer_mode_HRTIMER_MODE_REL,
+            );
+        }
+    }
+}
+
+/// Use to implement the [`HasHrTimer<T>`] trait.
+///
+/// See [`module`] documentation for an example.
+///
+/// [`module`]: crate::time::hrtimer
+#[macro_export]
+macro_rules! impl_has_hr_timer {
+    (
+        impl$({$($generics:tt)*})?
+            HasHrTimer<$timer_type:ty>
+            for $self:ty
+        { self.$field:ident }
+        $($rest:tt)*
+    ) => {
+        // SAFETY: This implementation of `raw_get_timer` only compiles if the
+        // field has the right type.
+        unsafe impl$(<$($generics)*>)? $crate::time::hrtimer::HasHrTimer<$timer_type> for $self {
+            const OFFSET: usize = ::core::mem::offset_of!(Self, $field) as usize;
+
+            #[inline]
+            unsafe fn raw_get_timer(ptr: *const Self) ->
+                *const $crate::time::hrtimer::HrTimer<$timer_type>
+            {
+                // SAFETY: The caller promises that the pointer is not dangling.
+                unsafe {
+                    ::core::ptr::addr_of!((*ptr).$field)
+                }
+            }
+        }
+    }
+}

-- 
2.47.0

Re: [PATCH v6 02/14] rust: hrtimer: introduce hrtimer support

[Tamir Duberstein]

On Fri, Jan 10, 2025 at 3:18 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> This patch adds support for intrusive use of the hrtimer system. For now,
> only one timer can be embedded in a Rust struct.
>
> The hrtimer Rust API is based on the intrusive style pattern introduced by
> the Rust workqueue API.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/time.rs         |   2 +
>  rust/kernel/time/hrtimer.rs | 296 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 298 insertions(+)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index f59e0fea79d3acfddd922f601f569353609aeec1..51c3532eee0184495ed5b7d717860c9980ff2a43 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -10,6 +10,8 @@
>
>  use core::convert::Into;
>
> +pub mod hrtimer;
> +
>  /// The number of nanoseconds per millisecond.
>  pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
>
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..31c461ddd2c377101ed6c1c7e009f7dccf458ebc
> --- /dev/null
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -0,0 +1,296 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Intrusive high resolution timers.
> +//!
> +//! Allows running timer callbacks without doing allocations at the time of
> +//! starting the timer. For now, only one timer per type is allowed.
> +//!
> +//! # Vocabulary
> +//!
> +//! A timer is initialized in the **stopped** state. A stopped timer can be
> +//! **started** with an **expiry** time. After the timer is started, it is
> +//! **running**. When the timer **expires**, the timer handler is executed.
> +//! After the handler has executed, the timer may be **restarted** or
> +//! **stopped**. A running timer can be **cancelled** before it's handler is
> +//! executed. A timer that is cancelled enters the **stopped** state.

s/it's/its/

I find this pretty hard to read because it contains a mix of
descriptions of states and descriptions of state transitions. Can we
state all the states first, and then all the transitions? Diagrams are
always helpful.

> +//!
> +//! States:
> +//!
> +//! * Stopped
> +//! * Running
> +//!
> +//! Operations:
> +//!
> +//! * Start
> +//! * Cancel
> +//! * Stop
> +//! * Restart
> +//!
> +//! Events:
> +//!
> +//! * Expire
> +
> +use crate::{init::PinInit, prelude::*, time::Ktime, types::Opaque};
> +use core::marker::PhantomData;
> +
> +/// A timer backed by a C `struct hrtimer`.
> +///
> +/// # Invariants
> +///
> +/// * `self.timer` is initialized by `bindings::hrtimer_setup`.
> +#[pin_data]
> +#[repr(C)]
> +pub struct HrTimer<U> {
> +    #[pin]
> +    timer: Opaque<bindings::hrtimer>,
> +    _t: PhantomData<U>,
> +}
> +
> +// SAFETY: A `HrTimer` can be moved to other threads and used/dropped from there.

Is that because it's on the heap on the C side, and the Rust type is
roughly a pointer to it?

> +unsafe impl<U> Send for HrTimer<U> {}
> +
> +// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
> +// timer from multiple threads
> +unsafe impl<U> Sync for HrTimer<U> {}
> +
> +impl<T> HrTimer<T> {
> +    /// Return an initializer for a new timer instance.

What's an initializer?

> +    pub fn new() -> impl PinInit<Self>
> +    where
> +        T: HrTimerCallback,
> +    {
> +        pin_init!(Self {
> +            // INVARIANTS: We initialize `timer` with `hrtimer_setup` below.
> +            timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
> +                // SAFETY: By design of `pin_init!`, `place` is a pointer live

pointer *to a* live allocation?

> +                // allocation. hrtimer_setup will initialize `place` and does
> +                // not require `place` to be initialized prior to the call.
> +                unsafe {
> +                    bindings::hrtimer_setup(
> +                        place,
> +                        Some(T::CallbackTarget::run),
> +                        bindings::CLOCK_MONOTONIC as i32,

Is it possible to avoid this as cast?

> +                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
> +                    );
> +                }
> +            }),
> +            _t: PhantomData,
> +        })
> +    }
> +
> +    /// Get a pointer to the contained `bindings::hrtimer`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must point to a live allocation of at least the size of `Self`.

This function is private, this doc comment won't ever be rendered by rustdoc.

> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
> +        // SAFETY: The field projection to `timer` does not go out of bounds,
> +        // because the caller of this function promises that `ptr` points to an
> +        // allocation of at least the size of `Self`.
> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
> +    }
> +
> +    /// Cancel an initialized and potentially running timer.
> +    ///
> +    /// If the timer handler is running, this will block until the handler is
> +    /// finished.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `self_ptr` must point to a valid `Self`.
> +    #[allow(dead_code)]

Why does this need to be raw? This can be explained in the commit
message or the cover letter IMO.

> +    pub(crate) unsafe fn raw_cancel(self_ptr: *const Self) -> bool {
> +        // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
> +        let c_timer_ptr = unsafe { HrTimer::raw_get(self_ptr) };
> +
> +        // If handler is running, this will wait for handler to finish before
> +        // returning.

Missing article in both halves; s/handler/the handler/.

> +        // SAFETY: `c_timer_ptr` is initialized and valid. Synchronization is
> +        // handled on C side.
> +        unsafe { bindings::hrtimer_cancel(c_timer_ptr) != 0 }
> +    }
> +}
> +
> +/// Implemented by pointer types that point to structs that embed a [`HrTimer`].
> +///
> +/// Typical implementers would be [`Box<T>`], [`Arc<T>`], [`ARef<T>`] where `T`
> +/// has a field of type [`HrTimer`].

nit: this paragraph is over specifying things.

> +///
> +/// Target must be [`Sync`] because timer callbacks happen in another thread of
> +/// execution (hard or soft interrupt context).

What is "Target"?

> +///
> +/// Starting a timer returns a [`HrTimerHandle`] that can be used to manipulate
> +/// the timer. Note that it is OK to call the start function repeatedly, and
> +/// that more than one [`HrTimerHandle`] associated with a [`HrTimerPointer`] may
> +/// exist. A timer can be manipulated through any of the handles, and a handle
> +/// may represent a cancelled timer.

"Cancel" is a transition, not a state, so I think this should say "a
stopped timer".

> +///
> +/// [`Box<T>`]: Box
> +/// [`Arc<T>`]: crate::sync::Arc
> +/// [`ARef<T>`]: crate::types::ARef
> +pub trait HrTimerPointer: Sync + Sized {
> +    /// A handle representing a running timer.

The comment just above says that a handle can represent a stopped
timer. Which is it?

> +    ///
> +    /// If the timer is running or if the timer callback is executing when the
> +    /// handle is dropped, the drop method of [`HrTimerHandle`] should not return
> +    /// until the timer is stopped and the callback has completed.
> +    ///
> +    /// Note: It must be safe to leak the handle.

What does "safe" mean exactly? Also, this comment seems to be
describing the trait `HrTimerHandle` rather than the associated type
`TimerHandle`.

> +    type TimerHandle: HrTimerHandle;
> +
> +    /// Start the timer with expiry after `expires` time units. If the timer was
> +    /// already running, it is restarted with the new expiry time.
> +    fn start(self, expires: Ktime) -> Self::TimerHandle;
> +}
> +
> +/// Implemented by [`HrTimerPointer`] implementers to give the C timer callback a
> +/// function to call.
> +// This is split from `HrTimerPointer` to make it easier to specify trait bounds.
> +pub trait RawHrTimerCallback {
> +    /// Callback to be called from C when timer fires.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Only to be called by C code in `hrtimer` subsystem. `ptr` must point to
> +    /// the `bindings::hrtimer` structure that was used to start the timer.
> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
> +}
> +
> +/// Implemented by structs that can the target of a timer callback.

Missing verb: s/can the/can be the/

> +pub trait HrTimerCallback {
> +    /// The type that was used for starting the timer.

This comment isn't very helpful, is it? I think it should say "The
type whose `run` method will be invoked when the timer expires.".

> +    type CallbackTarget<'a>: RawHrTimerCallback;
> +
> +    /// This type is passed to the timer callback function. It may be a borrow
> +    /// of [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
> +    /// implementation can guarantee exclusive access to the target during timer
> +    /// handler execution.
> +    type CallbackTargetParameter<'a>;
> +
> +    /// Called by the timer logic when the timer fires.
> +    fn run(this: Self::CallbackTargetParameter<'_>)
> +    where
> +        Self: Sized;
> +}
> +
> +/// A handle representing a potentially running timer.
> +///
> +/// More than one handle representing the same timer might exist.
> +///
> +/// # Safety
> +///
> +/// When dropped, the timer represented by this handle must be cancelled, if it
> +/// is running. If the timer handler is running when the handle is dropped, the
> +/// drop method must wait for the handler to finish before returning.

See my note above anchored on `type TimerHandle: HrTimerHandle`; it
repeats this comment.

> +pub unsafe trait HrTimerHandle {
> +    /// Cancel the timer, if it is running. If the timer handler is running, block
> +    /// till the handler has finished.
> +    fn cancel(&mut self) -> bool;
> +}
> +
> +/// Implemented by structs that contain timer nodes.
> +///
> +/// Clients of the timer API would usually safely implement this trait by using
> +/// the [`crate::impl_has_hr_timer`] macro.
> +///
> +/// # Safety
> +///
> +/// Implementers of this trait must ensure that the implementer has a [`HrTimer`]
> +/// field at the offset specified by `OFFSET` and that all trait methods are
> +/// implemented according to their documentation.
> +///
> +/// [`impl_has_timer`]: crate::impl_has_timer
> +pub unsafe trait HasHrTimer<U> {
> +    /// Offset of the [`HrTimer`] field within `Self`
> +    const OFFSET: usize;
> +
> +    /// Return a pointer to the [`HrTimer`] within `Self`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must point to a valid struct of type `Self`.
> +    unsafe fn raw_get_timer(ptr: *const Self) -> *const HrTimer<U> {
> +        // SAFETY: By the safety requirement of this trait, the trait
> +        // implementor will have a `HrTimer` field at the specified offset.
> +        unsafe { ptr.cast::<u8>().add(Self::OFFSET).cast::<HrTimer<U>>() }
> +    }
> +
> +    /// Return a pointer to the struct that is embedding the [`HrTimer`] pointed
> +    /// to by `ptr`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must point to a [`HrTimer<U>`] field in a struct of type `Self`.
> +    unsafe fn timer_container_of(ptr: *mut HrTimer<U>) -> *mut Self
> +    where
> +        Self: Sized,
> +    {
> +        // SAFETY: By the safety requirement of this function and the `HasHrTimer`
> +        // trait, the following expression will yield a pointer to the `Self`
> +        // containing the timer addressed by `ptr`.
> +        unsafe { ptr.cast::<u8>().sub(Self::OFFSET).cast::<Self>() }
> +    }
> +
> +    /// Get pointer to embedded `bindings::hrtimer` struct.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `self_ptr` must point to a valid `Self`.
> +    unsafe fn c_timer_ptr(self_ptr: *const Self) -> *const bindings::hrtimer {
> +        // SAFETY: `self_ptr` is a valid pointer to a `Self`.
> +        let timer_ptr = unsafe { Self::raw_get_timer(self_ptr) };
> +
> +        // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
> +        unsafe { HrTimer::raw_get(timer_ptr) }
> +    }
> +
> +    /// Start the timer contained in the `Self` pointed to by `self_ptr`. If
> +    /// it is already running it is removed and inserted.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `self_ptr` must point to a valid `Self`.
> +    unsafe fn start(self_ptr: *const Self, expires: Ktime) {
> +        // SAFETY: By function safety requirement, `self_ptr`is a valid `Self`.
> +        unsafe {
> +            bindings::hrtimer_start_range_ns(
> +                Self::c_timer_ptr(self_ptr).cast_mut(),
> +                expires.to_ns(),
> +                0,
> +                bindings::hrtimer_mode_HRTIMER_MODE_REL,
> +            );
> +        }
> +    }
> +}
> +
> +/// Use to implement the [`HasHrTimer<T>`] trait.
> +///
> +/// See [`module`] documentation for an example.
> +///
> +/// [`module`]: crate::time::hrtimer
> +#[macro_export]
> +macro_rules! impl_has_hr_timer {
> +    (
> +        impl$({$($generics:tt)*})?
> +            HasHrTimer<$timer_type:ty>
> +            for $self:ty
> +        { self.$field:ident }
> +        $($rest:tt)*
> +    ) => {
> +        // SAFETY: This implementation of `raw_get_timer` only compiles if the
> +        // field has the right type.
> +        unsafe impl$(<$($generics)*>)? $crate::time::hrtimer::HasHrTimer<$timer_type> for $self {
> +            const OFFSET: usize = ::core::mem::offset_of!(Self, $field) as usize;
> +
> +            #[inline]
> +            unsafe fn raw_get_timer(ptr: *const Self) ->
> +                *const $crate::time::hrtimer::HrTimer<$timer_type>
> +            {
> +                // SAFETY: The caller promises that the pointer is not dangling.
> +                unsafe {
> +                    ::core::ptr::addr_of!((*ptr).$field)
> +                }
> +            }
> +        }
> +    }
> +}

It'd be helpful to understand these design choices; why is so much of
this written in terms of raw pointers?

Tamir

Re: [PATCH v6 02/14] rust: hrtimer: introduce hrtimer support

[Andreas Hindborg]

Hi Tamir,

Thanks for the comments!

"Tamir Duberstein" <tamird@gmail.com> writes:

> On Fri, Jan 10, 2025 at 3:18 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:

[cut]

>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..31c461ddd2c377101ed6c1c7e009f7dccf458ebc
>> --- /dev/null
>> +++ b/rust/kernel/time/hrtimer.rs
>> @@ -0,0 +1,296 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Intrusive high resolution timers.
>> +//!
>> +//! Allows running timer callbacks without doing allocations at the time of
>> +//! starting the timer. For now, only one timer per type is allowed.
>> +//!
>> +//! # Vocabulary
>> +//!
>> +//! A timer is initialized in the **stopped** state. A stopped timer can be
>> +//! **started** with an **expiry** time. After the timer is started, it is
>> +//! **running**. When the timer **expires**, the timer handler is executed.
>> +//! After the handler has executed, the timer may be **restarted** or
>> +//! **stopped**. A running timer can be **cancelled** before it's handler is
>> +//! executed. A timer that is cancelled enters the **stopped** state.
>
> s/it's/its/
>
> I find this pretty hard to read because it contains a mix of
> descriptions of states and descriptions of state transitions. Can we
> state all the states first, and then all the transitions? Diagrams are
> always helpful.

You mean start the section with the state list from below?

>
>> +//!
>> +//! States:
>> +//!
>> +//! * Stopped
>> +//! * Running
>> +//!
>> +//! Operations:
>> +//!
>> +//! * Start
>> +//! * Cancel
>> +//! * Stop
>> +//! * Restart
>> +//!
>> +//! Events:
>> +//!
>> +//! * Expire
>> +
>> +use crate::{init::PinInit, prelude::*, time::Ktime, types::Opaque};
>> +use core::marker::PhantomData;
>> +
>> +/// A timer backed by a C `struct hrtimer`.
>> +///
>> +/// # Invariants
>> +///
>> +/// * `self.timer` is initialized by `bindings::hrtimer_setup`.
>> +#[pin_data]
>> +#[repr(C)]
>> +pub struct HrTimer<U> {
>> +    #[pin]
>> +    timer: Opaque<bindings::hrtimer>,
>> +    _t: PhantomData<U>,
>> +}
>> +
>> +// SAFETY: A `HrTimer` can be moved to other threads and used/dropped from there.
>
> Is that because it's on the heap on the C side, and the Rust type is
> roughly a pointer to it?

No it's unrelated to stack/heap allocation. It's just that the data
structure, and the methods on it, are constructed in such a way that there
is nothing that makes it unsafe to transfer ownership to another thread.
The `Opaque` field is removing this marker so we are just adding it
back.

`HrTimer` is not a pointer to a timer, it _is_ (contains) a
`bindings::hrtimer` struct.

>
>> +unsafe impl<U> Send for HrTimer<U> {}
>> +
>> +// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
>> +// timer from multiple threads
>> +unsafe impl<U> Sync for HrTimer<U> {}
>> +
>> +impl<T> HrTimer<T> {
>> +    /// Return an initializer for a new timer instance.
>
> What's an initializer?

A function that initialize a data structure. See
https://rust.docs.kernel.org/kernel/init/trait.PinInit.html . It is a
concept widely used in kernel rust. Perhaps I should add a link to the
return type in the text [initializer]: `kernel::init::PinInit`?

>
>> +    pub fn new() -> impl PinInit<Self>
>> +    where
>> +        T: HrTimerCallback,
>> +    {
>> +        pin_init!(Self {
>> +            // INVARIANTS: We initialize `timer` with `hrtimer_setup` below.
>> +            timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
>> +                // SAFETY: By design of `pin_init!`, `place` is a pointer live
>
> pointer *to a* live allocation?

👍

>
>> +                // allocation. hrtimer_setup will initialize `place` and does
>> +                // not require `place` to be initialized prior to the call.
>> +                unsafe {
>> +                    bindings::hrtimer_setup(
>> +                        place,
>> +                        Some(T::CallbackTarget::run),
>> +                        bindings::CLOCK_MONOTONIC as i32,
>
> Is it possible to avoid this as cast?

I don't think so, because bindgen generates CLOCK_MONOTONIC as u32. We
could `try_into`, but i don't think that makes sense.

>
>> +                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
>> +                    );
>> +                }
>> +            }),
>> +            _t: PhantomData,
>> +        })
>> +    }
>> +
>> +    /// Get a pointer to the contained `bindings::hrtimer`.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// `ptr` must point to a live allocation of at least the size of `Self`.
>
> This function is private, this doc comment won't ever be rendered by rustdoc.

But that is OK, right? I think there is an option for rustdoc to render private
functions as well.

>
>> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
>> +        // SAFETY: The field projection to `timer` does not go out of bounds,
>> +        // because the caller of this function promises that `ptr` points to an
>> +        // allocation of at least the size of `Self`.
>> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
>> +    }
>> +
>> +    /// Cancel an initialized and potentially running timer.
>> +    ///
>> +    /// If the timer handler is running, this will block until the handler is
>> +    /// finished.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// `self_ptr` must point to a valid `Self`.
>> +    #[allow(dead_code)]
>
> Why does this need to be raw? This can be explained in the commit
> message or the cover letter IMO.

I named it `raw_` to indicate that this is not what a user of the API
would call. They would call some safe code that wraps this method in a
way that is always safe. Some people use double underscore prefix to
same effect. I wanted to convey the message "this is not the cancel you
are looking for (jedi hand wave)".

>
>> +    pub(crate) unsafe fn raw_cancel(self_ptr: *const Self) -> bool {
>> +        // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
>> +        let c_timer_ptr = unsafe { HrTimer::raw_get(self_ptr) };
>> +
>> +        // If handler is running, this will wait for handler to finish before
>> +        // returning.
>
> Missing article in both halves; s/handler/the handler/.

👍

>
>> +        // SAFETY: `c_timer_ptr` is initialized and valid. Synchronization is
>> +        // handled on C side.
>> +        unsafe { bindings::hrtimer_cancel(c_timer_ptr) != 0 }
>> +    }
>> +}
>> +
>> +/// Implemented by pointer types that point to structs that embed a [`HrTimer`].
>> +///
>> +/// Typical implementers would be [`Box<T>`], [`Arc<T>`], [`ARef<T>`] where `T`
>> +/// has a field of type [`HrTimer`].
>
> nit: this paragraph is over specifying things.

I can remove the paragraph.

>
>> +///
>> +/// Target must be [`Sync`] because timer callbacks happen in another thread of
>> +/// execution (hard or soft interrupt context).
>
> What is "Target"?

That would be the pointee of the pointer<T: HasHrTimer>. Should I
replace "Target" with "The pointee"?

>
>> +///
>> +/// Starting a timer returns a [`HrTimerHandle`] that can be used to manipulate
>> +/// the timer. Note that it is OK to call the start function repeatedly, and
>> +/// that more than one [`HrTimerHandle`] associated with a [`HrTimerPointer`] may
>> +/// exist. A timer can be manipulated through any of the handles, and a handle
>> +/// may represent a cancelled timer.
>
> "Cancel" is a transition, not a state, so I think this should say "a
> stopped timer".

The vocabulary says:

> A running timer can be **cancelled** before it's handler is executed. A
> timer that is canceled enters the **stopped** state.

So I guess it should be fine as is? Alternatively it should be
"... represent a timer in the stopped state". Either is fine for me. Which
one do you like better?

>
>> +///
>> +/// [`Box<T>`]: Box
>> +/// [`Arc<T>`]: crate::sync::Arc
>> +/// [`ARef<T>`]: crate::types::ARef
>> +pub trait HrTimerPointer: Sync + Sized {
>> +    /// A handle representing a running timer.
>
> The comment just above says that a handle can represent a stopped
> timer. Which is it?

It will be a handle that completed the start or restart operation. When
the handle is initially returned, the timer will probably be running.
But it could be canceled or it could expire at any point and then the
handle no longer represents a running timer. So how about:

"A handle representing a started or restarted timer."

>
>> +    ///
>> +    /// If the timer is running or if the timer callback is executing when the
>> +    /// handle is dropped, the drop method of [`HrTimerHandle`] should not return
>> +    /// until the timer is stopped and the callback has completed.
>> +    ///
>> +    /// Note: It must be safe to leak the handle.
>
> What does "safe" mean exactly?

It means that the implementer of `HrTimerPointer` should consider that
leaking the handle does not require unsafe code and that it can be done
in safe rust. It might be better put as this:

    /// Note: When implementing this trait, consider that it is not unsafe to
    /// leak the handle.

> Also, this comment seems to be
> describing the trait `HrTimerHandle` rather than the associated type
> `TimerHandle`.

The note is for implementers of `HrTimerPointer`. They have to select a
type for `TimerHandle`.

>
>> +    type TimerHandle: HrTimerHandle;
>> +
>> +    /// Start the timer with expiry after `expires` time units. If the timer was
>> +    /// already running, it is restarted with the new expiry time.
>> +    fn start(self, expires: Ktime) -> Self::TimerHandle;
>> +}
>> +
>> +/// Implemented by [`HrTimerPointer`] implementers to give the C timer callback a
>> +/// function to call.
>> +// This is split from `HrTimerPointer` to make it easier to specify trait bounds.
>> +pub trait RawHrTimerCallback {
>> +    /// Callback to be called from C when timer fires.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// Only to be called by C code in `hrtimer` subsystem. `ptr` must point to
>> +    /// the `bindings::hrtimer` structure that was used to start the timer.
>> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
>> +}
>> +
>> +/// Implemented by structs that can the target of a timer callback.
>
> Missing verb: s/can the/can be the/

👍

>
>> +pub trait HrTimerCallback {
>> +    /// The type that was used for starting the timer.
>
> This comment isn't very helpful, is it? I think it should say "The
> type whose `run` method will be invoked when the timer expires.".

Yes, that is better.

>
>> +    type CallbackTarget<'a>: RawHrTimerCallback;
>> +
>> +    /// This type is passed to the timer callback function. It may be a borrow
>> +    /// of [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
>> +    /// implementation can guarantee exclusive access to the target during timer
>> +    /// handler execution.
>> +    type CallbackTargetParameter<'a>;
>> +
>> +    /// Called by the timer logic when the timer fires.
>> +    fn run(this: Self::CallbackTargetParameter<'_>)
>> +    where
>> +        Self: Sized;
>> +}
>> +
>> +/// A handle representing a potentially running timer.
>> +///
>> +/// More than one handle representing the same timer might exist.
>> +///
>> +/// # Safety
>> +///
>> +/// When dropped, the timer represented by this handle must be cancelled, if it
>> +/// is running. If the timer handler is running when the handle is dropped, the
>> +/// drop method must wait for the handler to finish before returning.
>
> See my note above anchored on `type TimerHandle: HrTimerHandle`; it
> repeats this comment.

Oh, I see your point regarding the paragraph above the note. Hmm, do you
think I should remove it from the associated type of
`HrTimerPointer::TimerHandle`? Is it a problem to refer to the safety
requirement from the associated type docs?

[cut]

>> +/// Use to implement the [`HasHrTimer<T>`] trait.
>> +///
>> +/// See [`module`] documentation for an example.
>> +///
>> +/// [`module`]: crate::time::hrtimer
>> +#[macro_export]
>> +macro_rules! impl_has_hr_timer {
>> +    (
>> +        impl$({$($generics:tt)*})?
>> +            HasHrTimer<$timer_type:ty>
>> +            for $self:ty
>> +        { self.$field:ident }
>> +        $($rest:tt)*
>> +    ) => {
>> +        // SAFETY: This implementation of `raw_get_timer` only compiles if the
>> +        // field has the right type.
>> +        unsafe impl$(<$($generics)*>)? $crate::time::hrtimer::HasHrTimer<$timer_type> for $self {
>> +            const OFFSET: usize = ::core::mem::offset_of!(Self, $field) as usize;
>> +
>> +            #[inline]
>> +            unsafe fn raw_get_timer(ptr: *const Self) ->
>> +                *const $crate::time::hrtimer::HrTimer<$timer_type>
>> +            {
>> +                // SAFETY: The caller promises that the pointer is not dangling.
>> +                unsafe {
>> +                    ::core::ptr::addr_of!((*ptr).$field)
>> +                }
>> +            }
>> +        }
>> +    }
>> +}
>
> It'd be helpful to understand these design choices; why is so much of
> this written in terms of raw pointers?

A lot of this design is dictated by the underlying C API. We have a C
structure that is embedded into a rust structure (the timer structure).
This C structure is used to place the Rust structure into trees and
linked lists on the C side. To start (arm) a timer we have to pass a raw
pointer to this embedded C structure to the C API. When the timer fires,
the C API notifies us by calling a repr(C) function with a pointer to
the embedded C struct. So we have to do some offset calculations to get
back to the rust struct that embeds the timer struct. That is not
possible without raw pointers.

The design borrows most of its ideas from the worktree abstractions,
which have almost the same challenges.


Best regards,
Andreas Hindborg

Re: [PATCH v6 02/14] rust: hrtimer: introduce hrtimer support

[Miguel Ojeda]

On Thu, Jan 16, 2025 at 8:06 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> But that is OK, right? I think there is an option for rustdoc to render private
> functions as well.

Yeah, documenting private functions is fine and encouraged.

We may not be able to require it for everything, but it would be nice
to achieve it e.g. for core abstractions.

Cheers,
Miguel

Re: [PATCH v6 02/14] rust: hrtimer: introduce hrtimer support

[Alice Ryhl]

On Fri, Jan 10, 2025 at 9:17 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> This patch adds support for intrusive use of the hrtimer system. For now,
> only one timer can be embedded in a Rust struct.
>
> The hrtimer Rust API is based on the intrusive style pattern introduced by
> the Rust workqueue API.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> +/// A timer backed by a C `struct hrtimer`.
> +///
> +/// # Invariants
> +///
> +/// * `self.timer` is initialized by `bindings::hrtimer_setup`.
> +#[pin_data]
> +#[repr(C)]
> +pub struct HrTimer<U> {

nit: We usually use repr(transparent) instead.

> +    #[pin]
> +    timer: Opaque<bindings::hrtimer>,
> +    _t: PhantomData<U>,
> +}
> +
> +// SAFETY: A `HrTimer` can be moved to other threads and used/dropped from there.
> +unsafe impl<U> Send for HrTimer<U> {}
> +
> +// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
> +// timer from multiple threads
> +unsafe impl<U> Sync for HrTimer<U> {}
> +
> +impl<T> HrTimer<T> {

You are inconsistent with whether the generic parameter is called T or U.

Alice

Re: [PATCH v6 02/14] rust: hrtimer: introduce hrtimer support

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Fri, Jan 10, 2025 at 9:17 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> This patch adds support for intrusive use of the hrtimer system. For now,
>> only one timer can be embedded in a Rust struct.
>>
>> The hrtimer Rust API is based on the intrusive style pattern introduced by
>> the Rust workqueue API.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> +/// A timer backed by a C `struct hrtimer`.
>> +///
>> +/// # Invariants
>> +///
>> +/// * `self.timer` is initialized by `bindings::hrtimer_setup`.
>> +#[pin_data]
>> +#[repr(C)]
>> +pub struct HrTimer<U> {
>
> nit: We usually use repr(transparent) instead.

`HrTimer` will have an additional field later in the series, and
`transparent` will not work.

>
>> +    #[pin]
>> +    timer: Opaque<bindings::hrtimer>,
>> +    _t: PhantomData<U>,
>> +}
>> +
>> +// SAFETY: A `HrTimer` can be moved to other threads and used/dropped from there.
>> +unsafe impl<U> Send for HrTimer<U> {}
>> +
>> +// SAFETY: Timer operations are locked on C side, so it is safe to operate on a
>> +// timer from multiple threads
>> +unsafe impl<U> Sync for HrTimer<U> {}
>> +
>> +impl<T> HrTimer<T> {
>
> You are inconsistent with whether the generic parameter is called T or U.

Will fix.


Best regards,
Andreas Hindborg

[PATCH v6 03/14] rust: sync: add `Arc::as_ptr`

[Andreas Hindborg]

Add a method to get a pointer to the data contained in an `Arc`.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/sync/arc.rs | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index fa4509406ee909ca0677b78d5ece966089ce6366..3d6111ddb007285b26eca2177a412033f4ac5dcb 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -233,6 +233,15 @@ pub fn into_raw(self) -> *const T {
         unsafe { core::ptr::addr_of!((*ptr).data) }
     }
 
+    /// Return a raw pointer to the data in this arc.
+    pub fn as_ptr(this: &Self) -> *const T {
+        let ptr = this.ptr.as_ptr();
+
+        // SAFETY: As `ptr` points to a valid allocation of type `ArcInner`,
+        // field projection to `data`is within bounds of the allocation.
+        unsafe { core::ptr::addr_of!((*ptr).data) }
+    }
+
     /// Recreates an [`Arc`] instance previously deconstructed via [`Arc::into_raw`].
     ///
     /// # Safety
@@ -508,11 +517,11 @@ unsafe fn new(inner: NonNull<ArcInner<T>>) -> Self {
     }
 
     /// Creates an [`ArcBorrow`] to an [`Arc`] that has previously been deconstructed with
-    /// [`Arc::into_raw`].
+    /// [`Arc::into_raw`] or [`Arc::as_ptr`].
     ///
     /// # Safety
     ///
-    /// * The provided pointer must originate from a call to [`Arc::into_raw`].
+    /// * The provided pointer must originate from a call to [`Arc::into_raw`] or [`Arc::as_ptr`].
     /// * For the duration of the lifetime annotated on this `ArcBorrow`, the reference count must
     ///   not hit zero.
     /// * For the duration of the lifetime annotated on this `ArcBorrow`, there must not be a

-- 
2.47.0

Re: [PATCH v6 03/14] rust: sync: add `Arc::as_ptr`

[Tamir Duberstein]

On Fri, Jan 10, 2025 at 3:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Add a method to get a pointer to the data contained in an `Arc`.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Can this mention why it is needed?

Re: [PATCH v6 03/14] rust: sync: add `Arc::as_ptr`

[Andreas Hindborg]

"Tamir Duberstein" <tamird@gmail.com> writes:

> On Fri, Jan 10, 2025 at 3:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Add a method to get a pointer to the data contained in an `Arc`.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> Can this mention why it is needed?

I can add that after the cut. It is for impleminting `HrTimerPointer`
for `Arc`.


Best regards,
Andreas Hindborg

Re: [PATCH v6 03/14] rust: sync: add `Arc::as_ptr`

[Tamir Duberstein]

On Thu, Jan 16, 2025 at 2:23 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Tamir Duberstein" <tamird@gmail.com> writes:
>
> > On Fri, Jan 10, 2025 at 3:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> Add a method to get a pointer to the data contained in an `Arc`.
> >>
> >> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> >
> > Can this mention why it is needed?
>
> I can add that after the cut. It is for impleminting `HrTimerPointer`
> for `Arc`.

The broader question is: why do we need to implement `/HrTimerPointer
` for `Arc`? Even broader is: why do we need to use raw pointers so
much?

Re: [PATCH v6 03/14] rust: sync: add `Arc::as_ptr`

[Boqun Feng]

On Mon, Feb 03, 2025 at 05:46:42PM -0500, Tamir Duberstein wrote:
> On Thu, Jan 16, 2025 at 2:23 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >
> > "Tamir Duberstein" <tamird@gmail.com> writes:
> >
> > > On Fri, Jan 10, 2025 at 3:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> > >>
> > >> Add a method to get a pointer to the data contained in an `Arc`.
> > >>
> > >> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> > >
> > > Can this mention why it is needed?
> >
> > I can add that after the cut. It is for impleminting `HrTimerPointer`
> > for `Arc`.
> 
> The broader question is: why do we need to implement `/HrTimerPointer
> ` for `Arc`? Even broader is: why do we need to use raw pointers so
> much?

Because to use the C function for hrtimer operation (start, cancel,
etc.), we need to pass a raw pointer of "struct hrtimer", which is
embedded in the object in an `Arc`.

Regards,
Boqun

Re: [PATCH v6 03/14] rust: sync: add `Arc::as_ptr`

[Andreas Hindborg]

"Tamir Duberstein" <tamird@gmail.com> writes:

> On Thu, Jan 16, 2025 at 2:23 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Tamir Duberstein" <tamird@gmail.com> writes:
>>
>> > On Fri, Jan 10, 2025 at 3:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >>
>> >> Add a method to get a pointer to the data contained in an `Arc`.
>> >>
>> >> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> >
>> > Can this mention why it is needed?
>>
>> I can add that after the cut. It is for impleminting `HrTimerPointer`
>> for `Arc`.
>
> The broader question is: why do we need to implement `/HrTimerPointer
> ` for `Arc`? Even broader is: why do we need to use raw pointers so
> much?

The C APIs work with raw pointers. One important detail here is that the
consumers of the Rust APIs will never work with raw pointers. That is
all encapsulated within the abstractions.


Best regards,
Andreas Hindborg

Re: [PATCH v6 03/14] rust: sync: add `Arc::as_ptr`

[Alice Ryhl]

On Fri, Jan 10, 2025 at 9:17 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Add a method to get a pointer to the data contained in an `Arc`.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

A reason for why it's needed would be nice, but:

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

[PATCH v6 04/14] rust: hrtimer: implement `HrTimerPointer` for `Arc`

[Andreas Hindborg]

This patch allows the use of intrusive `hrtimer` fields in structs that are
managed by an `Arc`.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs     |  3 +-
 rust/kernel/time/hrtimer/arc.rs | 89 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 31c461ddd2c377101ed6c1c7e009f7dccf458ebc..d0842c7c4c6ddffeef9a79cbf9727819060e4333 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -98,7 +98,6 @@ unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
     /// # Safety
     ///
     /// `self_ptr` must point to a valid `Self`.
-    #[allow(dead_code)]
     pub(crate) unsafe fn raw_cancel(self_ptr: *const Self) -> bool {
         // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
         let c_timer_ptr = unsafe { HrTimer::raw_get(self_ptr) };
@@ -294,3 +293,5 @@ unsafe fn raw_get_timer(ptr: *const Self) ->
         }
     }
 }
+
+mod arc;
diff --git a/rust/kernel/time/hrtimer/arc.rs b/rust/kernel/time/hrtimer/arc.rs
new file mode 100644
index 0000000000000000000000000000000000000000..4c1812f190bc7a9e0b2fcd5ea2e320f45ba584bc
--- /dev/null
+++ b/rust/kernel/time/hrtimer/arc.rs
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::HasHrTimer;
+use super::HrTimer;
+use super::HrTimerCallback;
+use super::HrTimerHandle;
+use super::HrTimerPointer;
+use super::RawHrTimerCallback;
+use crate::sync::Arc;
+use crate::sync::ArcBorrow;
+use crate::time::Ktime;
+
+/// A handle for an `Arc<HasHrTimer<U>>` returned by a call to
+/// [`HrTimerPointer::start`].
+pub struct ArcHrTimerHandle<U>
+where
+    U: HasHrTimer<U>,
+{
+    pub(crate) inner: Arc<U>,
+}
+
+// SAFETY: We implement drop below, and we cancel the timer in the drop
+// implementation.
+unsafe impl<U> HrTimerHandle for ArcHrTimerHandle<U>
+where
+    U: HasHrTimer<U>,
+{
+    fn cancel(&mut self) -> bool {
+        let self_ptr = Arc::as_ptr(&self.inner);
+
+        // SAFETY: As we obtained `self_ptr` from a valid reference above, it
+        // must point to a valid `U`.
+        let timer_ptr = unsafe { <U as HasHrTimer<U>>::raw_get_timer(self_ptr) };
+
+        // SAFETY: As `timer_ptr` points into `U` and `U` is valid, `timer_ptr`
+        // must point to a valid `HrTimer` instance.
+        unsafe { HrTimer::<U>::raw_cancel(timer_ptr) }
+    }
+}
+
+impl<U> Drop for ArcHrTimerHandle<U>
+where
+    U: HasHrTimer<U>,
+{
+    fn drop(&mut self) {
+        self.cancel();
+    }
+}
+
+impl<U> HrTimerPointer for Arc<U>
+where
+    U: Send + Sync,
+    U: HasHrTimer<U>,
+    U: for<'a> HrTimerCallback<CallbackTarget<'a> = Self>,
+{
+    type TimerHandle = ArcHrTimerHandle<U>;
+
+    fn start(self, expires: Ktime) -> ArcHrTimerHandle<U> {
+        // SAFETY: Since we generate the pointer passed to `start` from a
+        // valid reference, it is a valid pointer.
+        unsafe { U::start(Arc::as_ptr(&self), expires) };
+
+        ArcHrTimerHandle { inner: self }
+    }
+}
+
+impl<U> RawHrTimerCallback for Arc<U>
+where
+    U: HasHrTimer<U>,
+    U: for<'a> HrTimerCallback<CallbackTarget<'a> = Self>,
+    U: for<'a> HrTimerCallback<CallbackTargetParameter<'a> = ArcBorrow<'a, U>>,
+{
+    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+        // `HrTimer` is `repr(C)`
+        let timer_ptr = ptr.cast::<super::HrTimer<U>>();
+
+        // SAFETY: By C API contract `ptr` is the pointer we passed when
+        // queuing the timer, so it is a `HrTimer<T>` embedded in a `T`.
+        let data_ptr = unsafe { U::timer_container_of(timer_ptr) };
+
+        // SAFETY: `data_ptr` points to the `U` that was used to queue the
+        // timer. This `U` is contained in an `Arc`.
+        let receiver = unsafe { ArcBorrow::from_raw(data_ptr) };
+
+        U::run(receiver);
+
+        bindings::hrtimer_restart_HRTIMER_NORESTART
+    }
+}

-- 
2.47.0

[PATCH v6 05/14] rust: hrtimer: allow timer restart from timer handler

[Andreas Hindborg]

This patch allows timer handlers to report that they want a timer to be
restarted after the timer handler has finished executing.

Also update the `hrtimer` documentation to showcase the new feature.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs     | 37 ++++++++++++++++++++++++++++++++++++-
 rust/kernel/time/hrtimer/arc.rs |  4 +---
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index d0842c7c4c6ddffeef9a79cbf9727819060e4333..50e8c23578b5cf7196893ac88d9547fc027f1f04 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -167,7 +167,7 @@ pub trait HrTimerCallback {
     type CallbackTargetParameter<'a>;
 
     /// Called by the timer logic when the timer fires.
-    fn run(this: Self::CallbackTargetParameter<'_>)
+    fn run(this: Self::CallbackTargetParameter<'_>) -> HrTimerRestart
     where
         Self: Sized;
 }
@@ -262,6 +262,41 @@ unsafe fn start(self_ptr: *const Self, expires: Ktime) {
     }
 }
 
+/// Restart policy for timers.
+pub enum HrTimerRestart {
+    /// Timer should not be restarted.
+    NoRestart,
+    /// Timer should be restarted.
+    Restart,
+}
+
+impl From<u32> for HrTimerRestart {
+    fn from(value: u32) -> Self {
+        match value {
+            0 => Self::NoRestart,
+            _ => Self::Restart,
+        }
+    }
+}
+
+impl From<i32> for HrTimerRestart {
+    fn from(value: i32) -> Self {
+        match value {
+            0 => Self::NoRestart,
+            _ => Self::Restart,
+        }
+    }
+}
+
+impl From<HrTimerRestart> for bindings::hrtimer_restart {
+    fn from(value: HrTimerRestart) -> Self {
+        match value {
+            HrTimerRestart::NoRestart => bindings::hrtimer_restart_HRTIMER_NORESTART,
+            HrTimerRestart::Restart => bindings::hrtimer_restart_HRTIMER_RESTART,
+        }
+    }
+}
+
 /// Use to implement the [`HasHrTimer<T>`] trait.
 ///
 /// See [`module`] documentation for an example.
diff --git a/rust/kernel/time/hrtimer/arc.rs b/rust/kernel/time/hrtimer/arc.rs
index 4c1812f190bc7a9e0b2fcd5ea2e320f45ba584bc..cee143b04aa21fa6c60c3363cd95f7a67360cbf8 100644
--- a/rust/kernel/time/hrtimer/arc.rs
+++ b/rust/kernel/time/hrtimer/arc.rs
@@ -82,8 +82,6 @@ impl<U> RawHrTimerCallback for Arc<U>
         // timer. This `U` is contained in an `Arc`.
         let receiver = unsafe { ArcBorrow::from_raw(data_ptr) };
 
-        U::run(receiver);
-
-        bindings::hrtimer_restart_HRTIMER_NORESTART
+        U::run(receiver).into()
     }
 }

-- 
2.47.0

Re: [PATCH v6 05/14] rust: hrtimer: allow timer restart from timer handler

[Tamir Duberstein]

On Fri, Jan 10, 2025 at 3:17 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> This patch allows timer handlers to report that they want a timer to be
> restarted after the timer handler has finished executing.
>
> Also update the `hrtimer` documentation to showcase the new feature.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/time/hrtimer.rs     | 37 ++++++++++++++++++++++++++++++++++++-
>  rust/kernel/time/hrtimer/arc.rs |  4 +---
>  2 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index d0842c7c4c6ddffeef9a79cbf9727819060e4333..50e8c23578b5cf7196893ac88d9547fc027f1f04 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -167,7 +167,7 @@ pub trait HrTimerCallback {
>      type CallbackTargetParameter<'a>;
>
>      /// Called by the timer logic when the timer fires.
> -    fn run(this: Self::CallbackTargetParameter<'_>)
> +    fn run(this: Self::CallbackTargetParameter<'_>) -> HrTimerRestart
>      where
>          Self: Sized;
>  }
> @@ -262,6 +262,41 @@ unsafe fn start(self_ptr: *const Self, expires: Ktime) {
>      }
>  }
>
> +/// Restart policy for timers.
> +pub enum HrTimerRestart {
> +    /// Timer should not be restarted.
> +    NoRestart,
> +    /// Timer should be restarted.
> +    Restart,
> +}
> +
> +impl From<u32> for HrTimerRestart {
> +    fn from(value: u32) -> Self {
> +        match value {
> +            0 => Self::NoRestart,
> +            _ => Self::Restart,
> +        }
> +    }
> +}
> +
> +impl From<i32> for HrTimerRestart {
> +    fn from(value: i32) -> Self {
> +        match value {
> +            0 => Self::NoRestart,
> +            _ => Self::Restart,
> +        }
> +    }
> +}

These are for converting from bindings to our enum, right? Why do we
need both signed and unsigned, and why do we hardcode 0 rather than
using the generated constants?

> +
> +impl From<HrTimerRestart> for bindings::hrtimer_restart {
> +    fn from(value: HrTimerRestart) -> Self {
> +        match value {
> +            HrTimerRestart::NoRestart => bindings::hrtimer_restart_HRTIMER_NORESTART,
> +            HrTimerRestart::Restart => bindings::hrtimer_restart_HRTIMER_RESTART,
> +        }
> +    }
> +}
> +
>  /// Use to implement the [`HasHrTimer<T>`] trait.
>  ///
>  /// See [`module`] documentation for an example.
> diff --git a/rust/kernel/time/hrtimer/arc.rs b/rust/kernel/time/hrtimer/arc.rs
> index 4c1812f190bc7a9e0b2fcd5ea2e320f45ba584bc..cee143b04aa21fa6c60c3363cd95f7a67360cbf8 100644
> --- a/rust/kernel/time/hrtimer/arc.rs
> +++ b/rust/kernel/time/hrtimer/arc.rs
> @@ -82,8 +82,6 @@ impl<U> RawHrTimerCallback for Arc<U>
>          // timer. This `U` is contained in an `Arc`.
>          let receiver = unsafe { ArcBorrow::from_raw(data_ptr) };
>
> -        U::run(receiver);
> -
> -        bindings::hrtimer_restart_HRTIMER_NORESTART
> +        U::run(receiver).into()
>      }
>  }
>
> --
> 2.47.0
>
>
>

Re: [PATCH v6 05/14] rust: hrtimer: allow timer restart from timer handler

[Andreas Hindborg]

"Tamir Duberstein" <tamird@gmail.com> writes:

> On Fri, Jan 10, 2025 at 3:17 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> This patch allows timer handlers to report that they want a timer to be
>> restarted after the timer handler has finished executing.
>>
>> Also update the `hrtimer` documentation to showcase the new feature.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/time/hrtimer.rs     | 37 ++++++++++++++++++++++++++++++++++++-
>>  rust/kernel/time/hrtimer/arc.rs |  4 +---
>>  2 files changed, 37 insertions(+), 4 deletions(-)
>>
>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>> index d0842c7c4c6ddffeef9a79cbf9727819060e4333..50e8c23578b5cf7196893ac88d9547fc027f1f04 100644
>> --- a/rust/kernel/time/hrtimer.rs
>> +++ b/rust/kernel/time/hrtimer.rs
>> @@ -167,7 +167,7 @@ pub trait HrTimerCallback {
>>      type CallbackTargetParameter<'a>;
>>
>>      /// Called by the timer logic when the timer fires.
>> -    fn run(this: Self::CallbackTargetParameter<'_>)
>> +    fn run(this: Self::CallbackTargetParameter<'_>) -> HrTimerRestart
>>      where
>>          Self: Sized;
>>  }
>> @@ -262,6 +262,41 @@ unsafe fn start(self_ptr: *const Self, expires: Ktime) {
>>      }
>>  }
>>
>> +/// Restart policy for timers.
>> +pub enum HrTimerRestart {
>> +    /// Timer should not be restarted.
>> +    NoRestart,
>> +    /// Timer should be restarted.
>> +    Restart,
>> +}
>> +
>> +impl From<u32> for HrTimerRestart {
>> +    fn from(value: u32) -> Self {
>> +        match value {
>> +            0 => Self::NoRestart,
>> +            _ => Self::Restart,
>> +        }
>> +    }
>> +}
>> +
>> +impl From<i32> for HrTimerRestart {
>> +    fn from(value: i32) -> Self {
>> +        match value {
>> +            0 => Self::NoRestart,
>> +            _ => Self::Restart,
>> +        }
>> +    }
>> +}
>
> These are for converting from bindings to our enum, right? Why do we
> need both signed and unsigned,

Depending on kernel (target arch) configuration, the enum type will be
either signed or unsigned on C side.

I guess I could try to figure out what kernel configs effect the change
and then do conditional compilation on Rust side, but it seems overkill.

> and why do we hardcode 0 rather than
> using the generated constants?

We should use the constants - thanks.


Best regards,
Andreas Hindborg

Re: [PATCH v6 05/14] rust: hrtimer: allow timer restart from timer handler

[Tamir Duberstein]

On Wed, Jan 15, 2025 at 6:21 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Tamir Duberstein" <tamird@gmail.com> writes:
>
> > On Fri, Jan 10, 2025 at 3:17 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> This patch allows timer handlers to report that they want a timer to be
> >> restarted after the timer handler has finished executing.
> >>
> >> Also update the `hrtimer` documentation to showcase the new feature.
> >>
> >> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> >> ---
> >>  rust/kernel/time/hrtimer.rs     | 37 ++++++++++++++++++++++++++++++++++++-
> >>  rust/kernel/time/hrtimer/arc.rs |  4 +---
> >>  2 files changed, 37 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> >> index d0842c7c4c6ddffeef9a79cbf9727819060e4333..50e8c23578b5cf7196893ac88d9547fc027f1f04 100644
> >> --- a/rust/kernel/time/hrtimer.rs
> >> +++ b/rust/kernel/time/hrtimer.rs
> >> @@ -167,7 +167,7 @@ pub trait HrTimerCallback {
> >>      type CallbackTargetParameter<'a>;
> >>
> >>      /// Called by the timer logic when the timer fires.
> >> -    fn run(this: Self::CallbackTargetParameter<'_>)
> >> +    fn run(this: Self::CallbackTargetParameter<'_>) -> HrTimerRestart
> >>      where
> >>          Self: Sized;
> >>  }
> >> @@ -262,6 +262,41 @@ unsafe fn start(self_ptr: *const Self, expires: Ktime) {
> >>      }
> >>  }
> >>
> >> +/// Restart policy for timers.
> >> +pub enum HrTimerRestart {
> >> +    /// Timer should not be restarted.
> >> +    NoRestart,
> >> +    /// Timer should be restarted.
> >> +    Restart,
> >> +}
> >> +
> >> +impl From<u32> for HrTimerRestart {
> >> +    fn from(value: u32) -> Self {
> >> +        match value {
> >> +            0 => Self::NoRestart,
> >> +            _ => Self::Restart,
> >> +        }
> >> +    }
> >> +}
> >> +
> >> +impl From<i32> for HrTimerRestart {
> >> +    fn from(value: i32) -> Self {
> >> +        match value {
> >> +            0 => Self::NoRestart,
> >> +            _ => Self::Restart,
> >> +        }
> >> +    }
> >> +}
> >
> > These are for converting from bindings to our enum, right? Why do we
> > need both signed and unsigned,
>
> Depending on kernel (target arch) configuration, the enum type will be
> either signed or unsigned on C side.
>
> I guess I could try to figure out what kernel configs effect the change
> and then do conditional compilation on Rust side, but it seems overkill.

The bindings seem to contain a type alias:

pub const hrtimer_restart_HRTIMER_NORESTART: hrtimer_restart = 0;
pub const hrtimer_restart_HRTIMER_RESTART: hrtimer_restart = 1;
pub type hrtimer_restart = ffi::c_uint;

Perhaps you could impl `From<bindings:: hrtimer_restart>` instead?

Tamir

Re: [PATCH v6 05/14] rust: hrtimer: allow timer restart from timer handler

[Andreas Hindborg]

"Tamir Duberstein" <tamird@gmail.com> writes:

> On Wed, Jan 15, 2025 at 6:21 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Tamir Duberstein" <tamird@gmail.com> writes:
>>
>> > On Fri, Jan 10, 2025 at 3:17 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >>
>> >> This patch allows timer handlers to report that they want a timer to be
>> >> restarted after the timer handler has finished executing.
>> >>
>> >> Also update the `hrtimer` documentation to showcase the new feature.
>> >>
>> >> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> >> ---
>> >>  rust/kernel/time/hrtimer.rs     | 37 ++++++++++++++++++++++++++++++++++++-
>> >>  rust/kernel/time/hrtimer/arc.rs |  4 +---
>> >>  2 files changed, 37 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>> >> index d0842c7c4c6ddffeef9a79cbf9727819060e4333..50e8c23578b5cf7196893ac88d9547fc027f1f04 100644
>> >> --- a/rust/kernel/time/hrtimer.rs
>> >> +++ b/rust/kernel/time/hrtimer.rs
>> >> @@ -167,7 +167,7 @@ pub trait HrTimerCallback {
>> >>      type CallbackTargetParameter<'a>;
>> >>
>> >>      /// Called by the timer logic when the timer fires.
>> >> -    fn run(this: Self::CallbackTargetParameter<'_>)
>> >> +    fn run(this: Self::CallbackTargetParameter<'_>) -> HrTimerRestart
>> >>      where
>> >>          Self: Sized;
>> >>  }
>> >> @@ -262,6 +262,41 @@ unsafe fn start(self_ptr: *const Self, expires: Ktime) {
>> >>      }
>> >>  }
>> >>
>> >> +/// Restart policy for timers.
>> >> +pub enum HrTimerRestart {
>> >> +    /// Timer should not be restarted.
>> >> +    NoRestart,
>> >> +    /// Timer should be restarted.
>> >> +    Restart,
>> >> +}
>> >> +
>> >> +impl From<u32> for HrTimerRestart {
>> >> +    fn from(value: u32) -> Self {
>> >> +        match value {
>> >> +            0 => Self::NoRestart,
>> >> +            _ => Self::Restart,
>> >> +        }
>> >> +    }
>> >> +}
>> >> +
>> >> +impl From<i32> for HrTimerRestart {
>> >> +    fn from(value: i32) -> Self {
>> >> +        match value {
>> >> +            0 => Self::NoRestart,
>> >> +            _ => Self::Restart,
>> >> +        }
>> >> +    }
>> >> +}
>> >
>> > These are for converting from bindings to our enum, right? Why do we
>> > need both signed and unsigned,
>>
>> Depending on kernel (target arch) configuration, the enum type will be
>> either signed or unsigned on C side.
>>
>> I guess I could try to figure out what kernel configs effect the change
>> and then do conditional compilation on Rust side, but it seems overkill.
>
> The bindings seem to contain a type alias:
>
> pub const hrtimer_restart_HRTIMER_NORESTART: hrtimer_restart = 0;
> pub const hrtimer_restart_HRTIMER_RESTART: hrtimer_restart = 1;
> pub type hrtimer_restart = ffi::c_uint;
>
> Perhaps you could impl `From<bindings:: hrtimer_restart>` instead?

Yes - good idea!


Best regards,
Andreas Hindborg

[PATCH v6 06/14] rust: hrtimer: add `UnsafeHrTimerPointer`

[Andreas Hindborg]

Add a trait to allow unsafely queuing stack allocated timers.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 50e8c23578b5cf7196893ac88d9547fc027f1f04..aab998b02f19b774d5b04ae2c89b669f13c4b0c7 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -142,6 +142,37 @@ pub trait HrTimerPointer: Sync + Sized {
     fn start(self, expires: Ktime) -> Self::TimerHandle;
 }
 
+/// Unsafe version of [`HrTimerPointer`] for situations where leaking the
+/// [`HrTimerHandle`] returned by `start` would be unsound. This is the case for
+/// stack allocated timers.
+///
+/// Typical implementers are pinned references such as [`Pin<&T>`].
+///
+/// # Safety
+///
+/// Implementers of this trait must ensure that instances of types implementing
+/// [`UnsafeHrTimerPointer`] outlives any associated [`HrTimerPointer::TimerHandle`]
+/// instances.
+pub unsafe trait UnsafeHrTimerPointer: Sync + Sized {
+    /// A handle representing a running timer.
+    ///
+    /// # Safety
+    ///
+    /// If the timer is running, or if the timer callback is executing when the
+    /// handle is dropped, the drop method of [`Self::TimerHandle`] must not return
+    /// until the timer is stopped and the callback has completed.
+    type TimerHandle: HrTimerHandle;
+
+    /// Start the timer after `expires` time units. If the timer was already
+    /// running, it is restarted at the new expiry time.
+    ///
+    /// # Safety
+    ///
+    /// Caller promises keep the timer structure alive until the timer is dead.
+    /// Caller can ensure this by not leaking the returned [`Self::TimerHandle`].
+    unsafe fn start(self, expires: Ktime) -> Self::TimerHandle;
+}
+
 /// Implemented by [`HrTimerPointer`] implementers to give the C timer callback a
 /// function to call.
 // This is split from `HrTimerPointer` to make it easier to specify trait bounds.

-- 
2.47.0

[PATCH v6 07/14] rust: hrtimer: add `hrtimer::ScopedHrTimerPointer`

[Andreas Hindborg]

Add the trait `ScopedHrTimerPointer` to allow safe use of stack allocated
timers. Safety is achieved by pinning the stack in place while timers are
running.

Implement the trait for all types that implement `UnsafeHrTimerPointer`.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index aab998b02f19b774d5b04ae2c89b669f13c4b0c7..3cf81b5bb4c7071e095948f3220232a15116ed5b 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -173,6 +173,39 @@ pub unsafe trait UnsafeHrTimerPointer: Sync + Sized {
     unsafe fn start(self, expires: Ktime) -> Self::TimerHandle;
 }
 
+/// A trait for stack allocated timers.
+///
+/// # Safety
+///
+/// Implementers must ensure that `start_scoped` does not return until the
+/// timer is dead and the timer handler is not running.
+pub unsafe trait ScopedHrTimerPointer {
+    /// Start the timer to run after `expires` time units and immediately
+    /// after call `f`. When `f` returns, the timer is cancelled.
+    fn start_scoped<T, F>(self, expires: Ktime, f: F) -> T
+    where
+        F: FnOnce() -> T;
+}
+
+// SAFETY: By the safety requirement of [`UnsafeHrTimerPointer`], dropping the
+// handle returned by [`UnsafeHrTimerPointer::start`] ensures that the timer is
+// killed.
+unsafe impl<U> ScopedHrTimerPointer for U
+where
+    U: UnsafeHrTimerPointer,
+{
+    fn start_scoped<T, F>(self, expires: Ktime, f: F) -> T
+    where
+        F: FnOnce() -> T,
+    {
+        // SAFETY: We drop the timer handle below before returning.
+        let handle = unsafe { UnsafeHrTimerPointer::start(self, expires) };
+        let t = f();
+        drop(handle);
+        t
+    }
+}
+
 /// Implemented by [`HrTimerPointer`] implementers to give the C timer callback a
 /// function to call.
 // This is split from `HrTimerPointer` to make it easier to specify trait bounds.

-- 
2.47.0

[PATCH v6 08/14] rust: hrtimer: implement `UnsafeHrTimerPointer` for `Pin<&T>`

[Andreas Hindborg]

Allow pinned references to structs that contain a `HrTimer` node to be
scheduled with the `hrtimer` subsystem.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs     |  1 +
 rust/kernel/time/hrtimer/pin.rs | 95 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 3cf81b5bb4c7071e095948f3220232a15116ed5b..d776e93584a87dfdc646816a16bacf19e444217d 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -394,3 +394,4 @@ unsafe fn raw_get_timer(ptr: *const Self) ->
 }
 
 mod arc;
+mod pin;
diff --git a/rust/kernel/time/hrtimer/pin.rs b/rust/kernel/time/hrtimer/pin.rs
new file mode 100644
index 0000000000000000000000000000000000000000..2f2d1312566c5b8a763fdb8e0283dedb0ca3e231
--- /dev/null
+++ b/rust/kernel/time/hrtimer/pin.rs
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::HasHrTimer;
+use super::HrTimer;
+use super::HrTimerCallback;
+use super::HrTimerHandle;
+use super::RawHrTimerCallback;
+use super::UnsafeHrTimerPointer;
+use crate::time::Ktime;
+use core::pin::Pin;
+
+/// A handle for a `Pin<&HasHrTimer>`. When the handle exists, the timer might be
+/// running.
+pub struct PinHrTimerHandle<'a, U>
+where
+    U: HasHrTimer<U>,
+{
+    pub(crate) inner: Pin<&'a U>,
+}
+
+// SAFETY: We cancel the timer when the handle is dropped. The implementation of
+// the `cancel` method will block if the timer handler is running.
+unsafe impl<'a, U> HrTimerHandle for PinHrTimerHandle<'a, U>
+where
+    U: HasHrTimer<U>,
+{
+    fn cancel(&mut self) -> bool {
+        let self_ptr: *const U = self.inner.get_ref();
+
+        // SAFETY: As we got `self_ptr` from a reference above, it must point to
+        // a valid `U`.
+        let timer_ptr = unsafe { <U as HasHrTimer<U>>::raw_get_timer(self_ptr) };
+
+        // SAFETY: As `timer_ptr` is derived from a reference, it must point to
+        // a valid and initialized `HrTimer`.
+        unsafe { HrTimer::<U>::raw_cancel(timer_ptr) }
+    }
+}
+
+impl<'a, U> Drop for PinHrTimerHandle<'a, U>
+where
+    U: HasHrTimer<U>,
+{
+    fn drop(&mut self) {
+        self.cancel();
+    }
+}
+
+// SAFETY: We capture the lifetime of `Self` when we create a `PinHrTimerHandle`,
+// so `Self` will outlive the handle.
+unsafe impl<'a, U> UnsafeHrTimerPointer for Pin<&'a U>
+where
+    U: Send + Sync,
+    U: HasHrTimer<U>,
+    U: HrTimerCallback<CallbackTarget<'a> = Self>,
+{
+    type TimerHandle = PinHrTimerHandle<'a, U>;
+
+    unsafe fn start(self, expires: Ktime) -> Self::TimerHandle {
+        // Cast to pointer
+        let self_ptr: *const U = <Self as core::ops::Deref>::deref(&self);
+
+        // SAFETY: As we derive `self_ptr` from a reference above, it must point
+        // to a valid `U`.
+        unsafe { U::start(self_ptr, expires) };
+
+        PinHrTimerHandle { inner: self }
+    }
+}
+
+impl<'a, U> RawHrTimerCallback for Pin<&'a U>
+where
+    U: HasHrTimer<U>,
+    U: HrTimerCallback<CallbackTarget<'a> = Self>,
+    U: HrTimerCallback<CallbackTargetParameter<'a> = Self>,
+{
+    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+        // `HrTimer` is `repr(C)`
+        let timer_ptr = ptr as *mut HrTimer<U>;
+
+        // SAFETY: By the safety requirement of this function, `timer_ptr`
+        // points to a `HrTimer<U>` contained in an `U`.
+        let receiver_ptr = unsafe { U::timer_container_of(timer_ptr) };
+
+        // SAFETY: By the safety requirement of this function, `timer_ptr`
+        // points to a `HrTimer<U>` contained in an `U`.
+        let receiver_ref = unsafe { &*receiver_ptr };
+
+        // SAFETY: `receiver_ref` only exists as pinned, so it is safe to pin it
+        // here.
+        let receiver_pin = unsafe { Pin::new_unchecked(receiver_ref) };
+
+        U::run(receiver_pin).into()
+    }
+}

-- 
2.47.0

[PATCH v6 09/14] rust: hrtimer: implement `UnsafeHrTimerPointer` for `Pin<&mut T>`

[Andreas Hindborg]

Allow pinned mutable references to structs that contain a `HrTimer` node to
be scheduled with the `hrtimer` subsystem.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs         |  1 +
 rust/kernel/time/hrtimer/pin_mut.rs | 97 +++++++++++++++++++++++++++++++++++++
 2 files changed, 98 insertions(+)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index d776e93584a87dfdc646816a16bacf19e444217d..9220a8f559271c7eda8d457dbd52cf9e4800ec14 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -395,3 +395,4 @@ unsafe fn raw_get_timer(ptr: *const Self) ->
 
 mod arc;
 mod pin;
+mod pin_mut;
diff --git a/rust/kernel/time/hrtimer/pin_mut.rs b/rust/kernel/time/hrtimer/pin_mut.rs
new file mode 100644
index 0000000000000000000000000000000000000000..adcbba2b5ca9759fbdcc235ba75e006e35724d10
--- /dev/null
+++ b/rust/kernel/time/hrtimer/pin_mut.rs
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::HasHrTimer;
+use super::HrTimer;
+use super::HrTimerCallback;
+use super::HrTimerHandle;
+use super::RawHrTimerCallback;
+use super::UnsafeHrTimerPointer;
+use crate::time::Ktime;
+use core::pin::Pin;
+
+/// A handle for a `Pin<&mut HasHrTimer>`. When the handle exists, the timer might
+/// be running.
+pub struct PinMutHrTimerHandle<'a, U>
+where
+    U: HasHrTimer<U>,
+{
+    pub(crate) inner: Pin<&'a mut U>,
+}
+
+// SAFETY: We cancel the timer when the handle is dropped. The implementation of
+// the `cancel` method will block if the timer handler is running.
+unsafe impl<'a, U> HrTimerHandle for PinMutHrTimerHandle<'a, U>
+where
+    U: HasHrTimer<U>,
+{
+    fn cancel(&mut self) -> bool {
+        // SAFETY: We are not moving out of `self` or handing out mutable
+        // references to `self`.
+        let self_ptr = unsafe { self.inner.as_mut().get_unchecked_mut() as *mut U };
+
+        // SAFETY: As we got `self_ptr` from a reference above, it must point to
+        // a valid `U`.
+        let timer_ptr = unsafe { <U as HasHrTimer<U>>::raw_get_timer(self_ptr) };
+
+        // SAFETY: As `timer_ptr` is derived from a reference, it must point to
+        // a valid and initialized `HrTimer`.
+        unsafe { HrTimer::<U>::raw_cancel(timer_ptr) }
+    }
+}
+
+impl<'a, U> Drop for PinMutHrTimerHandle<'a, U>
+where
+    U: HasHrTimer<U>,
+{
+    fn drop(&mut self) {
+        self.cancel();
+    }
+}
+
+// SAFETY: We capture the lifetime of `Self` when we create a
+// `PinMutHrTimerHandle`, so `Self` will outlive the handle.
+unsafe impl<'a, U> UnsafeHrTimerPointer for Pin<&'a mut U>
+where
+    U: Send + Sync,
+    U: HasHrTimer<U>,
+    U: HrTimerCallback<CallbackTarget<'a> = Self>,
+{
+    type TimerHandle = PinMutHrTimerHandle<'a, U>;
+
+    unsafe fn start(self, expires: Ktime) -> Self::TimerHandle {
+        // Cast to pointer
+        let self_ptr: *const U = <Self as core::ops::Deref>::deref(&self);
+
+        // SAFETY: As we derive `self_ptr` from a reference above, it must point
+        // to a valid `U`.
+        unsafe { U::start(self_ptr, expires) };
+
+        PinMutHrTimerHandle { inner: self }
+    }
+}
+
+impl<'a, U> RawHrTimerCallback for Pin<&'a mut U>
+where
+    U: HasHrTimer<U>,
+    U: HrTimerCallback<CallbackTarget<'a> = Self>,
+    U: HrTimerCallback<CallbackTargetParameter<'a> = Self>,
+{
+    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+        // `HrTimer` is `repr(C)`
+        let timer_ptr = ptr as *mut HrTimer<U>;
+
+        // SAFETY: By the safety requirement of this function, `timer_ptr`
+        // points to a `HrTimer<U>` contained in an `U`.
+        let receiver_ptr = unsafe { U::timer_container_of(timer_ptr) };
+
+        // SAFETY: By the safety requirement of this function, `timer_ptr`
+        // points to a `HrTimer<U>` contained in an `U`.
+        let receiver_ref = unsafe { &mut *receiver_ptr };
+
+        // SAFETY: `receiver_ref` only exists as pinned, so it is safe to pin it
+        // here.
+        let receiver_pin = unsafe { Pin::new_unchecked(receiver_ref) };
+
+        U::run(receiver_pin).into()
+    }
+}

-- 
2.47.0

[PATCH v6 10/14] rust: alloc: add `Box::into_pin`

[Andreas Hindborg]

Add an associated function to convert a `Box<T>` into a `Pin<Box<T>>`.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/alloc/kbox.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
index 9ce414361c2c6dd8eea09b11041f6c307cbc7864..76f29e2ac085e19871f18653cfddf11d2594682c 100644
--- a/rust/kernel/alloc/kbox.rs
+++ b/rust/kernel/alloc/kbox.rs
@@ -245,6 +245,12 @@ pub fn pin(x: T, flags: Flags) -> Result<Pin<Box<T, A>>, AllocError>
         Ok(Self::new(x, flags)?.into())
     }
 
+    /// Convert a [`Box<T,A>`] to a [`Pin<Box<T,A>>`]. If `T` does not implement
+    /// [`Unpin`], then `x` will be pinned in memory and can't be moved.
+    pub fn into_pin(boxed: Self) -> Pin<Self> {
+        boxed.into()
+    }
+
     /// Forgets the contents (does not run the destructor), but keeps the allocation.
     fn forget_contents(this: Self) -> Box<MaybeUninit<T>, A> {
         let ptr = Self::into_raw(this);

-- 
2.47.0

Re: [PATCH v6 10/14] rust: alloc: add `Box::into_pin`

[Danilo Krummrich]

On 10.01.2025 21:15, Andreas Hindborg wrote:
> Add an associated function to convert a `Box<T>` into a `Pin<Box<T>>`.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/alloc/kbox.rs | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
> index 9ce414361c2c6dd8eea09b11041f6c307cbc7864..76f29e2ac085e19871f18653cfddf11d2594682c 100644
> --- a/rust/kernel/alloc/kbox.rs
> +++ b/rust/kernel/alloc/kbox.rs
> @@ -245,6 +245,12 @@ pub fn pin(x: T, flags: Flags) -> Result<Pin<Box<T, A>>, AllocError>
>          Ok(Self::new(x, flags)?.into())
>      }
>  
> +    /// Convert a [`Box<T,A>`] to a [`Pin<Box<T,A>>`]. If `T` does not implement
> +    /// [`Unpin`], then `x` will be pinned in memory and can't be moved.
> +    pub fn into_pin(boxed: Self) -> Pin<Self> {

Personally, I prefer `this`, as below. But I don't think it really matters.

Acked-by: Danilo Krummrich <dakr@kernel.org>

> +        boxed.into()
> +    }
> +
>      /// Forgets the contents (does not run the destructor), but keeps the allocation.
>      fn forget_contents(this: Self) -> Box<MaybeUninit<T>, A> {
>          let ptr = Self::into_raw(this);
> 

[PATCH v6 11/14] rust: hrtimer: implement `HrTimerPointer` for `Pin<Box<T>>`

[Andreas Hindborg]

Allow `Pin<Box<T>>` to be the target of a timer callback.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs      |   2 +
 rust/kernel/time/hrtimer/tbox.rs | 102 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 9220a8f559271c7eda8d457dbd52cf9e4800ec14..885d1772f7466725d2a421990f1ae154290ccf60 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -396,3 +396,5 @@ unsafe fn raw_get_timer(ptr: *const Self) ->
 mod arc;
 mod pin;
 mod pin_mut;
+// `box` is a reserved keyword, so prefix with `t` for timer
+mod tbox;
diff --git a/rust/kernel/time/hrtimer/tbox.rs b/rust/kernel/time/hrtimer/tbox.rs
new file mode 100644
index 0000000000000000000000000000000000000000..8dec40f7313921cd9f5bf357bb84962480385359
--- /dev/null
+++ b/rust/kernel/time/hrtimer/tbox.rs
@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::HasHrTimer;
+use super::HrTimer;
+use super::HrTimerCallback;
+use super::HrTimerHandle;
+use super::HrTimerPointer;
+use super::RawHrTimerCallback;
+use crate::prelude::*;
+use crate::time::Ktime;
+use core::mem::ManuallyDrop;
+
+/// A handle for a [`Box<HasHrTimer<U>>`] returned by a call to
+/// [`HrTimerPointer::start`].
+pub struct BoxHrTimerHandle<U, A>
+where
+    U: HasHrTimer<U>,
+    A: crate::alloc::Allocator,
+{
+    pub(crate) inner: *mut U,
+    _p: core::marker::PhantomData<A>,
+}
+
+// SAFETY: We implement drop below, and we cancel the timer in the drop
+// implementation.
+unsafe impl<U, A> HrTimerHandle for BoxHrTimerHandle<U, A>
+where
+    U: HasHrTimer<U>,
+    A: crate::alloc::Allocator,
+{
+    fn cancel(&mut self) -> bool {
+        // SAFETY: As we obtained `self.inner` from a valid reference when we
+        // created `self`, it must point to a valid `U`.
+        let timer_ptr = unsafe { <U as HasHrTimer<U>>::raw_get_timer(self.inner) };
+
+        // SAFETY: As `timer_ptr` points into `U` and `U` is valid, `timer_ptr`
+        // must point to a valid `HrTimer` instance.
+        unsafe { HrTimer::<U>::raw_cancel(timer_ptr) }
+    }
+}
+
+impl<U, A> Drop for BoxHrTimerHandle<U, A>
+where
+    U: HasHrTimer<U>,
+    A: crate::alloc::Allocator,
+{
+    fn drop(&mut self) {
+        self.cancel();
+        // SAFETY: `self.inner` came from a `Box::into_raw` call
+        drop(unsafe { Box::<U, A>::from_raw(self.inner) })
+    }
+}
+
+impl<U, A> HrTimerPointer for Pin<Box<U, A>>
+where
+    U: Send + Sync,
+    U: HasHrTimer<U>,
+    U: for<'a> HrTimerCallback<CallbackTarget<'a> = Pin<Box<U, A>>>,
+    U: for<'a> HrTimerCallback<CallbackTargetParameter<'a> = Pin<&'a U>>,
+    A: crate::alloc::Allocator,
+{
+    type TimerHandle = BoxHrTimerHandle<U, A>;
+
+    fn start(self, expires: Ktime) -> Self::TimerHandle {
+        let self_ptr: *const U = <Self as core::ops::Deref>::deref(&self);
+
+        // SAFETY: Since we generate the pointer passed to `start` from a valid
+        // reference, it is a valid pointer.
+        unsafe { U::start(self_ptr, expires) };
+
+        // SAFETY: We will not move out of this box during timer callback (we
+        // pass an immutable reference to the callback).
+        let inner = unsafe { Pin::into_inner_unchecked(self) };
+
+        BoxHrTimerHandle {
+            inner: Box::into_raw(inner),
+            _p: core::marker::PhantomData,
+        }
+    }
+}
+
+impl<U, A> RawHrTimerCallback for Pin<Box<U, A>>
+where
+    U: HasHrTimer<U>,
+    U: for<'a> HrTimerCallback<CallbackTarget<'a> = Pin<Box<U, A>>>,
+    U: for<'a> HrTimerCallback<CallbackTargetParameter<'a> = Pin<&'a U>>,
+    A: crate::alloc::Allocator,
+{
+    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+        // `HrTimer` is `repr(C)`
+        let timer_ptr = ptr.cast::<super::HrTimer<U>>();
+
+        // SAFETY: By C API contract `ptr` is the pointer we passed when
+        // queuing the timer, so it is a `HrTimer<T>` embedded in a `T`.
+        let data_ptr = unsafe { U::timer_container_of(timer_ptr) };
+
+        // SAFETY: We called `Box::into_raw` when we queued the timer.
+        let tbox = ManuallyDrop::new(Box::into_pin(unsafe { Box::<U, A>::from_raw(data_ptr) }));
+
+        U::run(tbox.as_ref()).into()
+    }
+}

-- 
2.47.0

[PATCH v6 12/14] rust: hrtimer: add `HrTimerMode`

[Andreas Hindborg]

Allow selection of timer mode by passing a `HrTimerMode` variant to
`HrTimer::new`.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs | 87 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 84 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 885d1772f7466725d2a421990f1ae154290ccf60..232af60f7c38370a4b2414a4c3780e06d2aa0c7d 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -43,6 +43,8 @@
 pub struct HrTimer<U> {
     #[pin]
     timer: Opaque<bindings::hrtimer>,
+    // This field goes away when `bindings::hrtimer_setup` is added.
+    mode: HrTimerMode,
     _t: PhantomData<U>,
 }
 
@@ -55,7 +57,7 @@ unsafe impl<U> Sync for HrTimer<U> {}
 
 impl<T> HrTimer<T> {
     /// Return an initializer for a new timer instance.
-    pub fn new() -> impl PinInit<Self>
+    pub fn new(mode: HrTimerMode) -> impl PinInit<Self>
     where
         T: HrTimerCallback,
     {
@@ -70,10 +72,11 @@ pub fn new() -> impl PinInit<Self>
                         place,
                         Some(T::CallbackTarget::run),
                         bindings::CLOCK_MONOTONIC as i32,
-                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
+                        mode.into(),
                     );
                 }
             }),
+            mode: mode,
             _t: PhantomData,
         })
     }
@@ -320,7 +323,7 @@ unsafe fn start(self_ptr: *const Self, expires: Ktime) {
                 Self::c_timer_ptr(self_ptr).cast_mut(),
                 expires.to_ns(),
                 0,
-                bindings::hrtimer_mode_HRTIMER_MODE_REL,
+                (*Self::raw_get_timer(self_ptr)).mode.into(),
             );
         }
     }
@@ -361,6 +364,84 @@ fn from(value: HrTimerRestart) -> Self {
     }
 }
 
+/// Operational mode of [`HrTimer`].
+#[derive(Clone, Copy)]
+pub enum HrTimerMode {
+    /// Timer expires at the given expiration time.
+    Absolute,
+    /// Timer expires after the given expiration time interpreted as a duration from now.
+    Relative,
+    /// Timer does not move between CPU cores.
+    Pinned,
+    /// Timer handler is executed in soft irq context.
+    Soft,
+    /// Timer handler is executed in hard irq context.
+    Hard,
+    /// Timer expires at the given expiration time.
+    /// Timer does not move between CPU cores.
+    AbsolutePinned,
+    /// Timer expires after the given expiration time interpreted as a duration from now.
+    /// Timer does not move between CPU cores.
+    RelativePinned,
+    /// Timer expires at the given expiration time.
+    /// Timer handler is executed in soft irq context.
+    AbsoluteSoft,
+    /// Timer expires after the given expiration time interpreted as a duration from now.
+    /// Timer handler is executed in soft irq context.
+    RelativeSoft,
+    /// Timer expires at the given expiration time.
+    /// Timer does not move between CPU cores.
+    /// Timer handler is executed in soft irq context.
+    AbsolutePinnedSoft,
+    /// Timer expires after the given expiration time interpreted as a duration from now.
+    /// Timer does not move between CPU cores.
+    /// Timer handler is executed in soft irq context.
+    RelativePinnedSoft,
+    /// Timer expires at the given expiration time.
+    /// Timer handler is executed in hard irq context.
+    AbsoluteHard,
+    /// Timer expires after the given expiration time interpreted as a duration from now.
+    /// Timer handler is executed in hard irq context.
+    RelativeHard,
+    /// Timer expires at the given expiration time.
+    /// Timer does not move between CPU cores.
+    /// Timer handler is executed in hard irq context.
+    AbsolutePinnedHard,
+    /// Timer expires after the given expiration time interpreted as a duration from now.
+    /// Timer does not move between CPU cores.
+    /// Timer handler is executed in hard irq context.
+    RelativePinnedHard,
+}
+
+impl From<HrTimerMode> for bindings::hrtimer_mode {
+    fn from(value: HrTimerMode) -> Self {
+        use bindings::*;
+        match value {
+            HrTimerMode::Absolute => hrtimer_mode_HRTIMER_MODE_ABS,
+            HrTimerMode::Relative => hrtimer_mode_HRTIMER_MODE_REL,
+            HrTimerMode::Pinned => hrtimer_mode_HRTIMER_MODE_PINNED,
+            HrTimerMode::Soft => hrtimer_mode_HRTIMER_MODE_SOFT,
+            HrTimerMode::Hard => hrtimer_mode_HRTIMER_MODE_HARD,
+            HrTimerMode::AbsolutePinned => hrtimer_mode_HRTIMER_MODE_ABS_PINNED,
+            HrTimerMode::RelativePinned => hrtimer_mode_HRTIMER_MODE_REL_PINNED,
+            HrTimerMode::AbsoluteSoft => hrtimer_mode_HRTIMER_MODE_ABS_SOFT,
+            HrTimerMode::RelativeSoft => hrtimer_mode_HRTIMER_MODE_REL_SOFT,
+            HrTimerMode::AbsolutePinnedSoft => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_SOFT,
+            HrTimerMode::RelativePinnedSoft => hrtimer_mode_HRTIMER_MODE_REL_PINNED_SOFT,
+            HrTimerMode::AbsoluteHard => hrtimer_mode_HRTIMER_MODE_ABS_HARD,
+            HrTimerMode::RelativeHard => hrtimer_mode_HRTIMER_MODE_REL_HARD,
+            HrTimerMode::AbsolutePinnedHard => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_HARD,
+            HrTimerMode::RelativePinnedHard => hrtimer_mode_HRTIMER_MODE_REL_PINNED_HARD,
+        }
+    }
+}
+
+impl From<HrTimerMode> for u64 {
+    fn from(value: HrTimerMode) -> Self {
+        Into::<bindings::hrtimer_mode>::into(value) as u64
+    }
+}
+
 /// Use to implement the [`HasHrTimer<T>`] trait.
 ///
 /// See [`module`] documentation for an example.

-- 
2.47.0

[PATCH v6 13/14] rust: hrtimer: add clocksource selection through `ClockSource`

[Andreas Hindborg]

Allow selecting a clock source for timers by passing a `ClockSource`
variant to `HrTimer::new`.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs | 52 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 232af60f7c38370a4b2414a4c3780e06d2aa0c7d..f759e4b1aa9f9fa6d55d5c144deedccc5d0590ae 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -57,7 +57,7 @@ unsafe impl<U> Sync for HrTimer<U> {}
 
 impl<T> HrTimer<T> {
     /// Return an initializer for a new timer instance.
-    pub fn new(mode: HrTimerMode) -> impl PinInit<Self>
+    pub fn new(mode: HrTimerMode, clock: ClockSource) -> impl PinInit<Self>
     where
         T: HrTimerCallback,
     {
@@ -71,7 +71,7 @@ pub fn new(mode: HrTimerMode) -> impl PinInit<Self>
                     bindings::hrtimer_setup(
                         place,
                         Some(T::CallbackTarget::run),
-                        bindings::CLOCK_MONOTONIC as i32,
+                        clock.into(),
                         mode.into(),
                     );
                 }
@@ -442,6 +442,54 @@ fn from(value: HrTimerMode) -> Self {
     }
 }
 
+/// The clock source to use for a [`HrTimer`].
+pub enum ClockSource {
+    /// A settable system-wide clock that measures real (i.e., wall-clock) time.
+    /// Setting this clock requires appropriate privileges. This clock is
+    /// affected by discontinuous jumps in the system time (e.g., if the system
+    /// administrator manually changes the clock), and by frequency adjustments
+    /// performed by NTP and similar applications via adjtime(3), adjtimex(2),
+    /// clock_adjtime(2), and ntp_adjtime(3). This clock normally counts the
+    /// number of seconds since 1970-01-01 00:00:00 Coordinated Universal Time
+    /// (UTC) except that it ignores leap seconds; near a leap second it is
+    /// typically adjusted by NTP to stay roughly in sync with UTC.
+    RealTime,
+    /// A nonsettable system-wide clock that represents monotonic time since—as
+    /// described by POSIX—"some unspecified point in the past". On Linux, that
+    /// point corresponds to the number of seconds that the system has been
+    /// running since it was booted.
+    ///
+    /// The CLOCK_MONOTONIC clock is not affected by discontinuous jumps in the
+    /// system time (e.g., if the system administrator manually changes the
+    /// clock), but is affected by frequency adjustments. This clock does not
+    /// count time that the system is suspended.
+    Monotonic,
+    /// A nonsettable system-wide clock that is identical to CLOCK_MONOTONIC,
+    /// except that it also includes any time that the system is suspended. This
+    /// allows applications to get a suspend-aware monotonic clock without
+    /// having to deal with the complications of CLOCK_REALTIME, which may have
+    /// discontinuities if the time is changed using settimeofday(2) or similar.
+    BootTime,
+    /// A nonsettable system-wide clock derived from wall-clock time but
+    /// counting leap seconds. This clock does not experience discontinuities or
+    /// frequency adjustments caused by inserting leap seconds as CLOCK_REALTIME
+    /// does.
+    ///
+    /// The acronym TAI refers to International Atomic Time.
+    TAI,
+}
+
+impl From<ClockSource> for bindings::clockid_t {
+    fn from(value: ClockSource) -> Self {
+        match value {
+            ClockSource::RealTime => bindings::CLOCK_REALTIME as i32,
+            ClockSource::Monotonic => bindings::CLOCK_MONOTONIC as i32,
+            ClockSource::BootTime => bindings::CLOCK_BOOTTIME as i32,
+            ClockSource::TAI => bindings::CLOCK_TAI as i32,
+        }
+    }
+}
+
 /// Use to implement the [`HasHrTimer<T>`] trait.
 ///
 /// See [`module`] documentation for an example.

-- 
2.47.0

[PATCH v6 14/14] rust: hrtimer: add maintainer entry

[Andreas Hindborg]

Add Andreas Hindborg as maintainer for Rust `hrtimer` abstractions. Also
add Boqun Feng as reviewer.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
Acked-by: Boqun Feng <boqun.feng@gmail.com>
---
 MAINTAINERS | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 30cbc3d44cd53e6b1a81d56161004d7ab825d7a9..926e1b4c548494ad84eab218167a3075c125908b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10253,6 +10253,16 @@ F:	kernel/time/timer_list.c
 F:	kernel/time/timer_migration.*
 F:	tools/testing/selftests/timers/
 
+HIGH-RESOLUTION TIMERS [RUST]
+M:	Andreas Hindborg <a.hindborg@kernel.org>
+R:	Boqun Feng <boqun.feng@gmail.com>
+L:	rust-for-linux@vger.kernel.org
+S:	Supported
+W:	https://rust-for-linux.com
+B:	https://github.com/Rust-for-Linux/linux/issues
+F:	rust/kernel/time/hrtimer.rs
+F:	rust/kernel/time/hrtimer/
+
 HIGH-SPEED SCC DRIVER FOR AX.25
 L:	linux-hams@vger.kernel.org
 S:	Orphan

-- 
2.47.0