[PATCH v10 00/13] hrtimer Rust API

[PATCH v10 00/13] hrtimer Rust API

[Andreas Hindborg]

Add support for using the `hrtimer` subsystem from Rust code.

Add support for timer mode and clock source configuration during timer
initialization. Do not add examples and functionality to execute closures at
timer expiration , 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 v10:
- Use imperative language for all commit messages.
- Spelling and grammar fixes for documentation.
- Consistently use `this` as argument name for functions taking a pointer to
  `Self`.
- Correct module documentation describing states.
- Derive some standard traits for configuration enums.
- Rephrase documentation for function that refrain from crating references.
- Use "has returned" rather than "has finished executing" when describing
  handler termination.
- Simplify documentation of `HrTimer::cancel`.
- Fix a documentation bug in the description of `Sync` requirement for
  `HrTimerPointer`.
- Consistently use the wording "contains" about types that contain another type.
- Remove `OFFSET` from `HasHrTimer`.
- Use direct mapping for enumerations where applicable.
- Remove `RUST_HRTIMER` kconfig.
- Add Frederic Weisbecker and Lyude Paul as reviewers.
- Add Thomas Gleixner and Anna-Maria Behnsen as reviewers.
- Change `ClockSource` to `ClockId` and move to `time` module.
- Add scm tree to maintainer entry.
- Add a note about effects of timer operations concurrent with cancel operation.
- Update documentation for CLOCK_REALTIME and CLOCK_TAI.
- Link to v9: https://lore.kernel.org/r/20250224-hrtimer-v3-v6-12-rc2-v9-0-5bd3bf0ce6cc@kernel.org

Changes in v9:
- Hide `From` conversions for opaque enums.
- Add kconfig entry for rust hrtimer API.
- Move `CallbackTargetParameter` to `RawHrTimerPointer`
- Shorten first paragraphs for clock source descriptions.
- Link `HrTimerHandle::cancel` in docs.
- Clarify exclusive/shared access to callback parameter in docs.
- Improve documentation for functions that avoid creating references.
- Expand safety requirement for `HasHrTimer::start`.
- Update module level documentation and diagram.
- Use `NonNull` to store pointer in BoxHrTimerHandle.
- Add a note to `HrTimerHandle` safety requirement.
- Link to v8: https://lore.kernel.org/r/20250218-hrtimer-v3-v6-12-rc2-v8-0-48dedb015eb3@kernel.org

Changes in v8:
- Publicly expose timer handles.
- Link to v7: https://lore.kernel.org/r/20250203-hrtimer-v3-v6-12-rc2-v7-0-189144725399@kernel.org

Changes in v7:
- fix a typo in commit message for "rust: time: Add Ktime::from_ns()"
- fix a typo in safety comment in `HrTimer::new`
- fix a typo in `HrTimer::raw_cancel`
- fix a typo in the vocabulary
- fix a typo in `HrTimerCallback` docs
- refactor module documentation
- add an ascii state diagram to module documentation
- specify reason for adding `Arc::as_ptr`'
- change `boxed` to `this` in `Box::into_pin`
- change `from_ns` to `from_nanos` to align with std
- imporove safety comment for `impl Send for HrTimer`
- remove useless paragraph in docs for `HrTimerPointer`
- rephrase docs for `HrTimerPointer::TimerHandle`
- update docs for `HrTimerCallback::CallbackTarget`
- explain how users should use safe functions for cancelling a timer
- rename generics for consistency
- remove a note about storing mode in `HrTimer` - this is still required
- rebase on v6.14-rc1
- Link to v6: https://lore.kernel.org/r/20250110-hrtimer-v3-v6-12-rc2-v6-0-f71d50f16482@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 `ClockId`
      rust: hrtimer: add maintainer entry

 MAINTAINERS                         |  15 ++
 rust/kernel/alloc/kbox.rs           |   6 +
 rust/kernel/sync/arc.rs             |  13 +-
 rust/kernel/time.rs                 |  68 +++++
 rust/kernel/time/hrtimer.rs         | 525 ++++++++++++++++++++++++++++++++++++
 rust/kernel/time/hrtimer/arc.rs     |  92 +++++++
 rust/kernel/time/hrtimer/pin.rs     |  99 +++++++
 rust/kernel/time/hrtimer/pin_mut.rs | 101 +++++++
 rust/kernel/time/hrtimer/tbox.rs    | 109 ++++++++
 9 files changed, 1026 insertions(+), 2 deletions(-)
---
base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
change-id: 20241017-hrtimer-v3-v6-12-rc2-215dc6b169bf

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

[PATCH v10 01/13] rust: hrtimer: introduce hrtimer support

[Andreas Hindborg]

Add support for intrusive use of the hrtimer system. For now,
only add support for embedding one timer per Rust struct.

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

Acked-by: Frederic Weisbecker <frederic@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time.rs         |   2 +
 rust/kernel/time/hrtimer.rs | 359 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 361 insertions(+)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 379c0f5772e5..fab1dadfa589 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).
 
+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 000000000000..7d7d490f8b6f
--- /dev/null
+++ b/rust/kernel/time/hrtimer.rs
@@ -0,0 +1,359 @@
+// 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
+//!
+//! States:
+//!
+//! - Stopped: initialized but not started, or cancelled, or not restarted.
+//! - Started: initialized and started or restarted.
+//! - Running: executing the callback.
+//!
+//! Operations:
+//!
+//! * Start
+//! * Cancel
+//! * Restart
+//!
+//! Events:
+//!
+//! * Expire
+//!
+//! ## State Diagram
+//!
+//! ```text
+//!                                                   Return NoRestart
+//!                       +---------------------------------------------------------------------+
+//!                       |                                                                     |
+//!                       |                                                                     |
+//!                       |                                                                     |
+//!                       |                                         Return Restart              |
+//!                       |                                      +------------------------+     |
+//!                       |                                      |                        |     |
+//!                       |                                      |                        |     |
+//!                       v                                      v                        |     |
+//!           +-----------------+      Start      +------------------+           +--------+-----+--+
+//!           |                 +---------------->|                  |           |                 |
+//! Init      |                 |                 |                  |  Expire   |                 |
+//! --------->|    Stopped      |                 |      Started     +---------->|     Running     |
+//!           |                 |     Cancel      |                  |           |                 |
+//!           |                 |<----------------+                  |           |                 |
+//!           +-----------------+                 +---------------+--+           +-----------------+
+//!                                                     ^         |
+//!                                                     |         |
+//!                                                     +---------+
+//!                                                      Restart
+//! ```
+//!
+//!
+//! A timer is initialized in the **stopped** state. A stopped timer can be
+//! **started** by the `start` operation, with an **expiry** time. After the
+//! `start` operation, the timer is in the **started** state. When the timer
+//! **expires**, the timer enters the **running** state and the handler is
+//! executed. After the handler has returned, the timer may enter the
+//! **started* or **stopped** state, depending on the return value of the
+//! handler. A timer in the **started** or **running** state may be **canceled**
+//! by the `cancel` operation. A timer that is cancelled enters the **stopped**
+//! state.
+//!
+//! A `cancel` or `restart` operation on a timer in the **running** state takes
+//! effect after the handler has returned and the timer has transitioned
+//! out of the **running** state.
+//!
+//! A `restart` operation on a timer in the **stopped** state is equivalent to a
+//! `start` operation.
+
+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<T> {
+    #[pin]
+    timer: Opaque<bindings::hrtimer>,
+    _t: PhantomData<T>,
+}
+
+// SAFETY: Ownership of an `HrTimer` can be moved to other threads and
+// used/dropped from there.
+unsafe impl<T> Send for HrTimer<T> {}
+
+// SAFETY: Timer operations are locked on the C side, so it is safe to operate
+// on a timer from multiple threads.
+unsafe impl<T> Sync for HrTimer<T> {}
+
+impl<T> HrTimer<T> {
+    /// Return an initializer for a new timer instance.
+    pub fn new() -> impl PinInit<Self>
+    where
+        T: HrTimerCallback,
+    {
+        pin_init!(Self {
+            // INVARIANT: 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 to a
+                // 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::Pointer::run),
+                        bindings::CLOCK_MONOTONIC as i32,
+                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
+                    );
+                }
+            }),
+            _t: PhantomData,
+        })
+    }
+
+    /// Get a pointer to the contained `bindings::hrtimer`.
+    ///
+    /// This function is useful to get access to the value without creating
+    /// intermediate references.
+    ///
+    /// # Safety
+    ///
+    /// `this` must point to a live allocation of at least the size of `Self`.
+    unsafe fn raw_get(this: *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 `this` points to an
+        // allocation of at least the size of `Self`.
+        unsafe { Opaque::raw_get(core::ptr::addr_of!((*this).timer)) }
+    }
+
+    /// Cancel an initialized and potentially running timer.
+    ///
+    /// If the timer handler is running, this function will block until the
+    /// handler returns.
+    ///
+    /// Note that the timer might be started by a concurrent start operation. If
+    /// so, the timer might not be in the **stopped** state when this function
+    /// returns.
+    ///
+    /// Users of the `HrTimer` API would not usually call this method directly.
+    /// Instead they would use the safe [`HrTimerHandle::cancel`] on the handle
+    /// returned when the timer was started.
+    ///
+    /// This function is useful to get access to the value without creating
+    /// intermediate references.
+    ///
+    /// # Safety
+    ///
+    /// `this` must point to a valid `Self`.
+    #[allow(dead_code)]
+    pub(crate) unsafe fn raw_cancel(this: *const Self) -> bool {
+        // SAFETY: `this` points to an allocation of at least `HrTimer` size.
+        let c_timer_ptr = unsafe { HrTimer::raw_get(this) };
+
+        // If the handler is running, this will wait for the handler to return
+        // before returning.
+        // SAFETY: `c_timer_ptr` is initialized and valid. Synchronization is
+        // handled on the C side.
+        unsafe { bindings::hrtimer_cancel(c_timer_ptr) != 0 }
+    }
+}
+
+/// Implemented by pointer types that point to structs that contain a [`HrTimer`].
+///
+/// `Self` must be [`Sync`] because it is passed to timer callbacks 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.
+pub trait HrTimerPointer: Sync + Sized {
+    /// 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: When implementing this trait, consider that it is not unsafe 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 {
+    /// This type is passed to [`HrTimerCallback::run`]. It may be a borrow of
+    /// [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
+    /// implementation can guarantee correct access (exclusive or shared
+    /// depending on the type) to the target during timer handler execution.
+    type CallbackTarget<'a>;
+
+    /// Callback to be called from C when timer fires.
+    ///
+    /// # Safety
+    ///
+    /// Only to be called by C code in the `hrtimer` subsystem. `this` must point
+    /// to the `bindings::hrtimer` structure that was used to start the timer.
+    unsafe extern "C" fn run(this: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
+}
+
+/// Implemented by structs that can be the target of a timer callback.
+pub trait HrTimerCallback {
+    /// The type whose [`RawHrTimerCallback::run`] method will be invoked when
+    /// the timer expires.
+    type Pointer<'a>: RawHrTimerCallback;
+
+    /// Called by the timer logic when the timer fires.
+    fn run(this: <Self::Pointer<'_> as RawHrTimerCallback>::CallbackTarget<'_>)
+    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 return before returning.
+///
+/// Note: One way to satisfy the safety requirement is to call `Self::cancel` in
+/// the drop implementation for `Self.`
+pub unsafe trait HrTimerHandle {
+    /// Cancel the timer. If the timer is in the running state, block till the
+    /// handler has returned.
+    ///
+    /// Note that the timer might be started by a concurrent start operation. If
+    /// so, the timer might not be in the **stopped** state when this function
+    /// returns.
+    ///
+    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<T> {
+    /// Return a pointer to the [`HrTimer`] within `Self`.
+    ///
+    /// This function is useful to get access to the value without creating
+    /// intermediate references.
+    ///
+    /// # Safety
+    ///
+    /// `this` must point to a valid struct of type `Self`.
+    unsafe fn raw_get_timer(this: *const Self) -> *const HrTimer<T>;
+
+    /// Return a pointer to the struct that is containing the [`HrTimer`] pointed
+    /// to by `ptr`.
+    ///
+    /// This function is useful to get access to the value without creating
+    /// intermediate references.
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must point to a [`HrTimer<T>`] field in a struct of type `Self`.
+    unsafe fn timer_container_of(ptr: *mut HrTimer<T>) -> *mut Self
+    where
+        Self: Sized;
+
+    /// Get pointer to the contained `bindings::hrtimer` struct.
+    ///
+    /// This function is useful to get access to the value without creating
+    /// intermediate references.
+    ///
+    /// # Safety
+    ///
+    /// `this` must point to a valid `Self`.
+    unsafe fn c_timer_ptr(this: *const Self) -> *const bindings::hrtimer {
+        // SAFETY: `this` is a valid pointer to a `Self`.
+        let timer_ptr = unsafe { Self::raw_get_timer(this) };
+
+        // 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
+    ///
+    /// - `this` must point to a valid `Self`.
+    /// - Caller must ensure that `self` lives until the timer fires or is
+    ///   canceled.
+    unsafe fn start(this: *const Self, expires: Ktime) {
+        // SAFETY: By function safety requirement, `this`is a valid `Self`.
+        unsafe {
+            bindings::hrtimer_start_range_ns(
+                Self::c_timer_ptr(this).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 {
+
+            #[inline]
+            unsafe fn raw_get_timer(this: *const Self) ->
+                *const $crate::time::hrtimer::HrTimer<$timer_type>
+            {
+                // SAFETY: The caller promises that the pointer is not dangling.
+                unsafe {
+                    ::core::ptr::addr_of!((*this).$field)
+                }
+            }
+
+            #[inline]
+            unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_type>) ->
+                *mut Self
+            {
+                // SAFETY: As per the safety requirement of this function, `ptr`
+                // is pointing inside a `$timer_type`.
+                unsafe {
+                    ::kernel::container_of!(ptr, $timer_type, $field).cast_mut()
+                }
+            }
+        }
+    }
+}

-- 
2.47.0

[PATCH v10 02/13] rust: sync: add `Arc::as_ptr`

[Andreas Hindborg]

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

Reviewed-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---

This is a dependency for:

rust: hrtimer: implement `HrTimerPointer` for `Arc`
---
 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 3cefda7a4372..1dfa75714f9d 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -246,6 +246,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
@@ -539,11 +548,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

[PATCH v10 03/13] rust: hrtimer: implement `HrTimerPointer` for `Arc`

[Andreas Hindborg]

Allow the use of intrusive `hrtimer` fields in structs that are managed by
an `Arc` by implementing `HrTimerPointer` and `RawTimerCallbck` for `Arc`.

Acked-by: Frederic Weisbecker <frederic@kernel.org>
Reviewed-by: Lyude Paul <lyude@redhat.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs     |  4 +-
 rust/kernel/time/hrtimer/arc.rs | 94 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 7d7d490f8b6f..bb46602a7749 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -150,7 +150,6 @@ unsafe fn raw_get(this: *const Self) -> *mut bindings::hrtimer {
     /// # Safety
     ///
     /// `this` must point to a valid `Self`.
-    #[allow(dead_code)]
     pub(crate) unsafe fn raw_cancel(this: *const Self) -> bool {
         // SAFETY: `this` points to an allocation of at least `HrTimer` size.
         let c_timer_ptr = unsafe { HrTimer::raw_get(this) };
@@ -357,3 +356,6 @@ unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_typ
         }
     }
 }
+
+mod arc;
+pub use arc::ArcHrTimerHandle;
diff --git a/rust/kernel/time/hrtimer/arc.rs b/rust/kernel/time/hrtimer/arc.rs
new file mode 100644
index 000000000000..5c916489fc13
--- /dev/null
+++ b/rust/kernel/time/hrtimer/arc.rs
@@ -0,0 +1,94 @@
+// 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<T>>` returned by a call to
+/// [`HrTimerPointer::start`].
+pub struct ArcHrTimerHandle<T>
+where
+    T: HasHrTimer<T>,
+{
+    pub(crate) inner: Arc<T>,
+}
+
+// SAFETY: We implement drop below, and we cancel the timer in the drop
+// implementation.
+unsafe impl<T> HrTimerHandle for ArcHrTimerHandle<T>
+where
+    T: HasHrTimer<T>,
+{
+    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 `T`.
+        let timer_ptr = unsafe { <T as HasHrTimer<T>>::raw_get_timer(self_ptr) };
+
+        // SAFETY: As `timer_ptr` points into `T` and `T` is valid, `timer_ptr`
+        // must point to a valid `HrTimer` instance.
+        unsafe { HrTimer::<T>::raw_cancel(timer_ptr) }
+    }
+}
+
+impl<T> Drop for ArcHrTimerHandle<T>
+where
+    T: HasHrTimer<T>,
+{
+    fn drop(&mut self) {
+        self.cancel();
+    }
+}
+
+impl<T> HrTimerPointer for Arc<T>
+where
+    T: 'static,
+    T: Send + Sync,
+    T: HasHrTimer<T>,
+    T: for<'a> HrTimerCallback<Pointer<'a> = Self>,
+    Arc<T>: for<'a> RawHrTimerCallback<CallbackTarget<'a> = ArcBorrow<'a, T>>,
+{
+    type TimerHandle = ArcHrTimerHandle<T>;
+
+    fn start(self, expires: Ktime) -> ArcHrTimerHandle<T> {
+        // SAFETY:
+        //  - We keep `self` alive by wrapping it in a handle below.
+        //  - Since we generate the pointer passed to `start` from a valid
+        //    reference, it is a valid pointer.
+        unsafe { T::start(Arc::as_ptr(&self), expires) };
+        ArcHrTimerHandle { inner: self }
+    }
+}
+
+impl<T> RawHrTimerCallback for Arc<T>
+where
+    T: 'static,
+    T: HasHrTimer<T>,
+    T: for<'a> HrTimerCallback<Pointer<'a> = Self>,
+{
+    type CallbackTarget<'a> = ArcBorrow<'a, T>;
+
+    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+        // `HrTimer` is `repr(C)`
+        let timer_ptr = ptr.cast::<super::HrTimer<T>>();
+
+        // 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 { T::timer_container_of(timer_ptr) };
+
+        // SAFETY: `data_ptr` points to the `T` that was used to queue the
+        // timer. This `T` is contained in an `Arc`.
+        let receiver = unsafe { ArcBorrow::from_raw(data_ptr) };
+
+        T::run(receiver);
+
+        bindings::hrtimer_restart_HRTIMER_NORESTART
+    }
+}

-- 
2.47.0

[PATCH v10 04/13] rust: hrtimer: allow timer restart from timer handler

[Andreas Hindborg]

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

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

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index bb46602a7749..7317ba4f71c4 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -214,7 +214,7 @@ pub trait HrTimerCallback {
     type Pointer<'a>: RawHrTimerCallback;
 
     /// Called by the timer logic when the timer fires.
-    fn run(this: <Self::Pointer<'_> as RawHrTimerCallback>::CallbackTarget<'_>)
+    fn run(this: <Self::Pointer<'_> as RawHrTimerCallback>::CallbackTarget<'_>) -> HrTimerRestart
     where
         Self: Sized;
 }
@@ -315,6 +315,22 @@ unsafe fn start(this: *const Self, expires: Ktime) {
     }
 }
 
+/// Restart policy for timers.
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+#[repr(u32)]
+pub enum HrTimerRestart {
+    /// Timer should not be restarted.
+    NoRestart = bindings::hrtimer_restart_HRTIMER_NORESTART,
+    /// Timer should be restarted.
+    Restart = bindings::hrtimer_restart_HRTIMER_RESTART,
+}
+
+impl HrTimerRestart {
+    fn into_c(self) -> bindings::hrtimer_restart {
+        self as bindings::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 5c916489fc13..7152fa414b37 100644
--- a/rust/kernel/time/hrtimer/arc.rs
+++ b/rust/kernel/time/hrtimer/arc.rs
@@ -87,8 +87,6 @@ impl<T> RawHrTimerCallback for Arc<T>
         // timer. This `T` is contained in an `Arc`.
         let receiver = unsafe { ArcBorrow::from_raw(data_ptr) };
 
-        T::run(receiver);
-
-        bindings::hrtimer_restart_HRTIMER_NORESTART
+        T::run(receiver).into_c()
     }
 }

-- 
2.47.0

[PATCH v10 05/13] rust: hrtimer: add `UnsafeHrTimerPointer`

[Andreas Hindborg]

Add a trait to allow unsafely queuing stack allocated timers.

Acked-by: Frederic Weisbecker <frederic@kernel.org>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Lyude Paul <lyude@redhat.com>
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 7317ba4f71c4..1fe25c271e0d 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -188,6 +188,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 v10 06/13] 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`.

Acked-by: Frederic Weisbecker <frederic@kernel.org>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Lyude Paul <lyude@redhat.com>
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 1fe25c271e0d..d90a25785f87 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -219,6 +219,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<T> ScopedHrTimerPointer for T
+where
+    T: UnsafeHrTimerPointer,
+{
+    fn start_scoped<U, F>(self, expires: Ktime, f: F) -> U
+    where
+        F: FnOnce() -> U,
+    {
+        // 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 v10 07/13] 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.

Acked-by: Frederic Weisbecker <frederic@kernel.org>
Reviewed-by: Lyude Paul <lyude@redhat.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs     |  2 +
 rust/kernel/time/hrtimer/pin.rs | 99 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index d90a25785f87..2ca56397eade 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -439,3 +439,5 @@ unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_typ
 
 mod arc;
 pub use arc::ArcHrTimerHandle;
+mod pin;
+pub use pin::PinHrTimerHandle;
diff --git a/rust/kernel/time/hrtimer/pin.rs b/rust/kernel/time/hrtimer/pin.rs
new file mode 100644
index 000000000000..6c9f2190f8e1
--- /dev/null
+++ b/rust/kernel/time/hrtimer/pin.rs
@@ -0,0 +1,99 @@
+// 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, T>
+where
+    T: HasHrTimer<T>,
+{
+    pub(crate) inner: Pin<&'a T>,
+}
+
+// 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, T> HrTimerHandle for PinHrTimerHandle<'a, T>
+where
+    T: HasHrTimer<T>,
+{
+    fn cancel(&mut self) -> bool {
+        let self_ptr: *const T = self.inner.get_ref();
+
+        // SAFETY: As we got `self_ptr` from a reference above, it must point to
+        // a valid `T`.
+        let timer_ptr = unsafe { <T as HasHrTimer<T>>::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::<T>::raw_cancel(timer_ptr) }
+    }
+}
+
+impl<'a, T> Drop for PinHrTimerHandle<'a, T>
+where
+    T: HasHrTimer<T>,
+{
+    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, T> UnsafeHrTimerPointer for Pin<&'a T>
+where
+    T: Send + Sync,
+    T: HasHrTimer<T>,
+    T: HrTimerCallback<Pointer<'a> = Self>,
+    Pin<&'a T>: RawHrTimerCallback<CallbackTarget<'a> = Self>,
+{
+    type TimerHandle = PinHrTimerHandle<'a, T>;
+
+    unsafe fn start(self, expires: Ktime) -> Self::TimerHandle {
+        // Cast to pointer
+        let self_ptr: *const T = <Self as core::ops::Deref>::deref(&self);
+
+        // SAFETY:
+        //  - As we derive `self_ptr` from a reference above, it must point to a
+        //    valid `T`.
+        //  - We keep `self` alive by wrapping it in a handle below.
+        unsafe { T::start(self_ptr, expires) };
+
+        PinHrTimerHandle { inner: self }
+    }
+}
+
+impl<'a, T> RawHrTimerCallback for Pin<&'a T>
+where
+    T: HasHrTimer<T>,
+    T: HrTimerCallback<Pointer<'a> = Self>,
+{
+    type CallbackTarget<'b> = Self;
+
+    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+        // `HrTimer` is `repr(C)`
+        let timer_ptr = ptr as *mut HrTimer<T>;
+
+        // SAFETY: By the safety requirement of this function, `timer_ptr`
+        // points to a `HrTimer<T>` contained in an `T`.
+        let receiver_ptr = unsafe { T::timer_container_of(timer_ptr) };
+
+        // SAFETY: By the safety requirement of this function, `timer_ptr`
+        // points to a `HrTimer<T>` contained in an `T`.
+        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) };
+
+        T::run(receiver_pin).into_c()
+    }
+}

-- 
2.47.0

[PATCH v10 08/13] 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.

Acked-by: Frederic Weisbecker <frederic@kernel.org>
Reviewed-by: Lyude Paul <lyude@redhat.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs         |   2 +
 rust/kernel/time/hrtimer/pin_mut.rs | 101 ++++++++++++++++++++++++++++++++++++
 2 files changed, 103 insertions(+)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 2ca56397eade..d2791fd624b7 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -441,3 +441,5 @@ unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_typ
 pub use arc::ArcHrTimerHandle;
 mod pin;
 pub use pin::PinHrTimerHandle;
+mod pin_mut;
+pub use pin_mut::PinMutHrTimerHandle;
diff --git a/rust/kernel/time/hrtimer/pin_mut.rs b/rust/kernel/time/hrtimer/pin_mut.rs
new file mode 100644
index 000000000000..4f4a9e9602d8
--- /dev/null
+++ b/rust/kernel/time/hrtimer/pin_mut.rs
@@ -0,0 +1,101 @@
+// 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, T>
+where
+    T: HasHrTimer<T>,
+{
+    pub(crate) inner: Pin<&'a mut T>,
+}
+
+// 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, T> HrTimerHandle for PinMutHrTimerHandle<'a, T>
+where
+    T: HasHrTimer<T>,
+{
+    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 T };
+
+        // SAFETY: As we got `self_ptr` from a reference above, it must point to
+        // a valid `T`.
+        let timer_ptr = unsafe { <T as HasHrTimer<T>>::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::<T>::raw_cancel(timer_ptr) }
+    }
+}
+
+impl<'a, T> Drop for PinMutHrTimerHandle<'a, T>
+where
+    T: HasHrTimer<T>,
+{
+    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, T> UnsafeHrTimerPointer for Pin<&'a mut T>
+where
+    T: Send + Sync,
+    T: HasHrTimer<T>,
+    T: HrTimerCallback<Pointer<'a> = Self>,
+    Pin<&'a mut T>: RawHrTimerCallback<CallbackTarget<'a> = Self>,
+{
+    type TimerHandle = PinMutHrTimerHandle<'a, T>;
+
+    unsafe fn start(self, expires: Ktime) -> Self::TimerHandle {
+        // Cast to pointer
+        let self_ptr: *const T = <Self as core::ops::Deref>::deref(&self);
+
+        // SAFETY:
+        //  - As we derive `self_ptr` from a reference above, it must point to a
+        //    valid `T`.
+        //  - We keep `self` alive by wrapping it in a handle below.
+        unsafe { T::start(self_ptr, expires) };
+
+        PinMutHrTimerHandle { inner: self }
+    }
+}
+
+impl<'a, T> RawHrTimerCallback for Pin<&'a mut T>
+where
+    T: HasHrTimer<T>,
+    T: HrTimerCallback<Pointer<'a> = Self>,
+{
+    type CallbackTarget<'b> = Self;
+
+    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+        // `HrTimer` is `repr(C)`
+        let timer_ptr = ptr as *mut HrTimer<T>;
+
+        // SAFETY: By the safety requirement of this function, `timer_ptr`
+        // points to a `HrTimer<T>` contained in an `T`.
+        let receiver_ptr = unsafe { T::timer_container_of(timer_ptr) };
+
+        // SAFETY: By the safety requirement of this function, `timer_ptr`
+        // points to a `HrTimer<T>` contained in an `T`.
+        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) };
+
+        T::run(receiver_pin).into_c()
+    }
+}

-- 
2.47.0

[PATCH v10 09/13] rust: alloc: add `Box::into_pin`

[Andreas Hindborg]

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

Acked-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Lyude Paul <lyude@redhat.com>
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 cb4ebea3b074..9da4a32e60bc 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(this: Self) -> Pin<Self> {
+        this.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

[PATCH v10 10/13] rust: hrtimer: implement `HrTimerPointer` for `Pin<Box<T>>`

[Andreas Hindborg]

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

Acked-by: Frederic Weisbecker <frederic@kernel.org>
Reviewed-by: Lyude Paul <lyude@redhat.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs      |   3 ++
 rust/kernel/time/hrtimer/tbox.rs | 109 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index d2791fd624b7..991d37b0524a 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -443,3 +443,6 @@ unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_typ
 pub use pin::PinHrTimerHandle;
 mod pin_mut;
 pub use pin_mut::PinMutHrTimerHandle;
+// `box` is a reserved keyword, so prefix with `t` for timer
+mod tbox;
+pub use tbox::BoxHrTimerHandle;
diff --git a/rust/kernel/time/hrtimer/tbox.rs b/rust/kernel/time/hrtimer/tbox.rs
new file mode 100644
index 000000000000..a3b2ed849050
--- /dev/null
+++ b/rust/kernel/time/hrtimer/tbox.rs
@@ -0,0 +1,109 @@
+// 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;
+use core::ptr::NonNull;
+
+/// A handle for a [`Box<HasHrTimer<T>>`] returned by a call to
+/// [`HrTimerPointer::start`].
+pub struct BoxHrTimerHandle<T, A>
+where
+    T: HasHrTimer<T>,
+    A: crate::alloc::Allocator,
+{
+    pub(crate) inner: NonNull<T>,
+    _p: core::marker::PhantomData<A>,
+}
+
+// SAFETY: We implement drop below, and we cancel the timer in the drop
+// implementation.
+unsafe impl<T, A> HrTimerHandle for BoxHrTimerHandle<T, A>
+where
+    T: HasHrTimer<T>,
+    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 `T`.
+        let timer_ptr = unsafe { <T as HasHrTimer<T>>::raw_get_timer(self.inner.as_ptr()) };
+
+        // SAFETY: As `timer_ptr` points into `T` and `T` is valid, `timer_ptr`
+        // must point to a valid `HrTimer` instance.
+        unsafe { HrTimer::<T>::raw_cancel(timer_ptr) }
+    }
+}
+
+impl<T, A> Drop for BoxHrTimerHandle<T, A>
+where
+    T: HasHrTimer<T>,
+    A: crate::alloc::Allocator,
+{
+    fn drop(&mut self) {
+        self.cancel();
+        // SAFETY: `self.inner` came from a `Box::into_raw` call
+        drop(unsafe { Box::<T, A>::from_raw(self.inner.as_ptr()) })
+    }
+}
+
+impl<T, A> HrTimerPointer for Pin<Box<T, A>>
+where
+    T: 'static,
+    T: Send + Sync,
+    T: HasHrTimer<T>,
+    T: for<'a> HrTimerCallback<Pointer<'a> = Pin<Box<T, A>>>,
+    Pin<Box<T, A>>: for<'a> RawHrTimerCallback<CallbackTarget<'a> = Pin<&'a T>>,
+    A: crate::alloc::Allocator,
+{
+    type TimerHandle = BoxHrTimerHandle<T, A>;
+
+    fn start(self, expires: Ktime) -> Self::TimerHandle {
+        // SAFETY:
+        //  - We will not move out of this box during timer callback (we pass an
+        //    immutable reference to the callback).
+        //  - `Box::into_raw` is guaranteed to return a valid pointer.
+        let inner =
+            unsafe { NonNull::new_unchecked(Box::into_raw(Pin::into_inner_unchecked(self))) };
+
+        // SAFETY:
+        //  - We keep `self` alive by wrapping it in a handle below.
+        //  - Since we generate the pointer passed to `start` from a valid
+        //    reference, it is a valid pointer.
+        unsafe { T::start(inner.as_ptr(), expires) };
+
+        BoxHrTimerHandle {
+            inner,
+            _p: core::marker::PhantomData,
+        }
+    }
+}
+
+impl<T, A> RawHrTimerCallback for Pin<Box<T, A>>
+where
+    T: 'static,
+    T: HasHrTimer<T>,
+    T: for<'a> HrTimerCallback<Pointer<'a> = Pin<Box<T, A>>>,
+    A: crate::alloc::Allocator,
+{
+    type CallbackTarget<'a> = Pin<&'a T>;
+
+    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+        // `HrTimer` is `repr(C)`
+        let timer_ptr = ptr.cast::<super::HrTimer<T>>();
+
+        // 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 { T::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::<T, A>::from_raw(data_ptr) }));
+
+        T::run(tbox.as_ref()).into_c()
+    }
+}

-- 
2.47.0

[PATCH v10 11/13] rust: hrtimer: add `HrTimerMode`

[Andreas Hindborg]

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

Acked-by: Frederic Weisbecker <frederic@kernel.org>
Reviewed-by: Lyude Paul <lyude@redhat.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time/hrtimer.rs | 82 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 79 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 991d37b0524a..d06be922d8d0 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -80,6 +80,7 @@
 pub struct HrTimer<T> {
     #[pin]
     timer: Opaque<bindings::hrtimer>,
+    mode: HrTimerMode,
     _t: PhantomData<T>,
 }
 
@@ -93,7 +94,7 @@ unsafe impl<T> Sync for HrTimer<T> {}
 
 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,
     {
@@ -108,10 +109,11 @@ pub fn new() -> impl PinInit<Self>
                         place,
                         Some(T::Pointer::run),
                         bindings::CLOCK_MONOTONIC as i32,
-                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
+                        mode.into_c(),
                     );
                 }
             }),
+            mode: mode,
             _t: PhantomData,
         })
     }
@@ -373,7 +375,7 @@ unsafe fn start(this: *const Self, expires: Ktime) {
                 Self::c_timer_ptr(this).cast_mut(),
                 expires.to_ns(),
                 0,
-                bindings::hrtimer_mode_HRTIMER_MODE_REL,
+                (*Self::raw_get_timer(this)).mode.into_c(),
             );
         }
     }
@@ -395,6 +397,80 @@ fn into_c(self) -> bindings::hrtimer_restart {
     }
 }
 
+/// Operational mode of [`HrTimer`].
+// NOTE: Some of these have the same encoding on the C side, so we keep
+// `repr(Rust)` and convert elsewhere.
+#[derive(Clone, Copy, PartialEq, Eq, Debug)]
+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 HrTimerMode {
+    fn into_c(self) -> bindings::hrtimer_mode {
+        use bindings::*;
+        match self {
+            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,
+        }
+    }
+}
+
 /// Use to implement the [`HasHrTimer<T>`] trait.
 ///
 /// See [`module`] documentation for an example.

-- 
2.47.0

[PATCH v10 12/13] rust: hrtimer: add clocksource selection through `ClockId`

[Andreas Hindborg]

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

Acked-by: Frederic Weisbecker <frederic@kernel.org>
Reviewed-by: Lyude Paul <lyude@redhat.com>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/time.rs         | 66 +++++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/time/hrtimer.rs |  5 ++--
 2 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index fab1dadfa589..f509cb0eb71e 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -83,3 +83,69 @@ fn sub(self, other: Ktime) -> Ktime {
         }
     }
 }
+
+/// An identifier for a clock. Used when specifying clock sources.
+///
+///
+/// Selection of the clock depends on the use case. In some cases the usage of a
+/// particular clock is mandatory, e.g. in network protocols, filesystems.In other
+/// cases the user of the clock has to decide which clock is best suited for the
+/// purpose. In most scenarios clock [`ClockId::Monotonic`] is the best choice as it
+/// provides a accurate monotonic notion of time (leap second smearing ignored).
+#[derive(Clone, Copy, PartialEq, Eq, Debug)]
+#[repr(u32)]
+pub enum ClockId {
+    /// 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 may be
+    /// adjusted by leap second smearing to stay roughly in sync with UTC. Leap
+    /// second smearing applies frequency adjustments to the clock to speed up
+    /// or slow down the clock to account for the leap second without
+    /// discontinuities in the clock. If leap second smearing is not applied,
+    /// the clock will experience discontinuity around leap second adjustment.
+    RealTime = bindings::CLOCK_REALTIME,
+    /// A monotonically increasing clock.
+    ///
+    /// 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
+    /// CLOCK_REAL (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 = bindings::CLOCK_MONOTONIC,
+    /// A monotonic that ticks while system is suspended.
+    ///
+    /// 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 = bindings::CLOCK_BOOTTIME,
+    /// International Atomic Time.
+    ///
+    /// A system-wide clock derived from wall-clock time but counting leap seconds.
+    ///
+    /// This clock is coupled to CLOCK_REALTIME and will be set when CLOCK_REALTIME is
+    /// set, or when the offset to CLOCK_REALTIME is changed via adjtimex(2). This
+    /// usually happens during boot and **should** not happen during normal operations.
+    /// However, if NTP or another application adjusts CLOCK_REALTIME by leap second
+    /// smearing, this clock will not be precise during leap second smearing.
+    ///
+    /// The acronym TAI refers to International Atomic Time.
+    TAI = bindings::CLOCK_TAI,
+}
+
+impl ClockId {
+    fn into_c(self) -> bindings::clockid_t {
+        self as bindings::clockid_t
+    }
+}
diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index d06be922d8d0..f700c479cd40 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -67,6 +67,7 @@
 //! A `restart` operation on a timer in the **stopped** state is equivalent to a
 //! `start` operation.
 
+use super::ClockId;
 use crate::{init::PinInit, prelude::*, time::Ktime, types::Opaque};
 use core::marker::PhantomData;
 
@@ -94,7 +95,7 @@ unsafe impl<T> Sync for HrTimer<T> {}
 
 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: ClockId) -> impl PinInit<Self>
     where
         T: HrTimerCallback,
     {
@@ -108,7 +109,7 @@ pub fn new(mode: HrTimerMode) -> impl PinInit<Self>
                     bindings::hrtimer_setup(
                         place,
                         Some(T::Pointer::run),
-                        bindings::CLOCK_MONOTONIC as i32,
+                        clock.into_c(),
                         mode.into_c(),
                     );
                 }

-- 
2.47.0

[PATCH v10 13/13] rust: hrtimer: add maintainer entry

[Andreas Hindborg]

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

Acked-by: Boqun Feng <boqun.feng@gmail.com>
Acked-by: Frederic Weisbecker <frederic@kernel.org>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 MAINTAINERS | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 896a307fa065..ba8e802faabf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10355,6 +10355,21 @@ 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>
+R:	Frederic Weisbecker <frederic@kernel.org>
+R:	Lyude Paul <lyude@redhat.com>
+R:	Thomas Gleixner <tglx@linutronix.de>
+R:	Anna-Maria Behnsen <anna-maria@linutronix.de>
+L:	rust-for-linux@vger.kernel.org
+S:	Supported
+W:	https://rust-for-linux.com
+B:	https://github.com/Rust-for-Linux/linux/issues
+T:	git https://github.com/Rust-for-Linux/linux.git hrtimer-next
+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

Re: [PATCH v10 01/13] rust: hrtimer: introduce hrtimer support

[Benno Lossin]

On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
> Add support for intrusive use of the hrtimer system. For now,
> only add support for embedding one timer per Rust struct.
>
> The hrtimer Rust API is based on the intrusive style pattern introduced by
> the Rust workqueue API.
>
> Acked-by: Frederic Weisbecker <frederic@kernel.org>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Some smaller changes below, with those fixed:

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

> ---
>  rust/kernel/time.rs         |   2 +
>  rust/kernel/time/hrtimer.rs | 359 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 361 insertions(+)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 379c0f5772e5..fab1dadfa589 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).
>
> +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 000000000000..7d7d490f8b6f
> --- /dev/null
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -0,0 +1,359 @@
> +// 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
> +//!
> +//! States:
> +//!
> +//! - Stopped: initialized but not started, or cancelled, or not restarted.
> +//! - Started: initialized and started or restarted.
> +//! - Running: executing the callback.
> +//!
> +//! Operations:
> +//!
> +//! * Start
> +//! * Cancel
> +//! * Restart
> +//!
> +//! Events:
> +//!
> +//! * Expire
> +//!
> +//! ## State Diagram
> +//!
> +//! ```text
> +//!                                                   Return NoRestart
> +//!                       +---------------------------------------------------------------------+
> +//!                       |                                                                     |
> +//!                       |                                                                     |
> +//!                       |                                                                     |
> +//!                       |                                         Return Restart              |
> +//!                       |                                      +------------------------+     |
> +//!                       |                                      |                        |     |
> +//!                       |                                      |                        |     |
> +//!                       v                                      v                        |     |
> +//!           +-----------------+      Start      +------------------+           +--------+-----+--+
> +//!           |                 +---------------->|                  |           |                 |
> +//! Init      |                 |                 |                  |  Expire   |                 |
> +//! --------->|    Stopped      |                 |      Started     +---------->|     Running     |
> +//!           |                 |     Cancel      |                  |           |                 |
> +//!           |                 |<----------------+                  |           |                 |
> +//!           +-----------------+                 +---------------+--+           +-----------------+
> +//!                                                     ^         |
> +//!                                                     |         |
> +//!                                                     +---------+
> +//!                                                      Restart
> +//! ```
> +//!
> +//!
> +//! A timer is initialized in the **stopped** state. A stopped timer can be
> +//! **started** by the `start` operation, with an **expiry** time. After the
> +//! `start` operation, the timer is in the **started** state. When the timer
> +//! **expires**, the timer enters the **running** state and the handler is
> +//! executed. After the handler has returned, the timer may enter the
> +//! **started* or **stopped** state, depending on the return value of the
> +//! handler. A timer in the **started** or **running** state may be **canceled**
> +//! by the `cancel` operation. A timer that is cancelled enters the **stopped**
> +//! state.

This looks very nice, thanks!

> +//!
> +//! A `cancel` or `restart` operation on a timer in the **running** state takes
> +//! effect after the handler has returned and the timer has transitioned
> +//! out of the **running** state.
> +//!
> +//! A `restart` operation on a timer in the **stopped** state is equivalent to a
> +//! `start` operation.
> +
> +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<T> {
> +    #[pin]
> +    timer: Opaque<bindings::hrtimer>,
> +    _t: PhantomData<T>,
> +}
> +
> +// SAFETY: Ownership of an `HrTimer` can be moved to other threads and
> +// used/dropped from there.
> +unsafe impl<T> Send for HrTimer<T> {}
> +
> +// SAFETY: Timer operations are locked on the C side, so it is safe to operate
> +// on a timer from multiple threads.
> +unsafe impl<T> Sync for HrTimer<T> {}
> +
> +impl<T> HrTimer<T> {
> +    /// Return an initializer for a new timer instance.
> +    pub fn new() -> impl PinInit<Self>
> +    where
> +        T: HrTimerCallback,
> +    {
> +        pin_init!(Self {
> +            // INVARIANT: 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 to a
> +                // 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::Pointer::run),
> +                        bindings::CLOCK_MONOTONIC as i32,
> +                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
> +                    );
> +                }
> +            }),
> +            _t: PhantomData,
> +        })
> +    }
> +
> +    /// Get a pointer to the contained `bindings::hrtimer`.
> +    ///
> +    /// This function is useful to get access to the value without creating
> +    /// intermediate references.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `this` must point to a live allocation of at least the size of `Self`.
> +    unsafe fn raw_get(this: *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 `this` points to an
> +        // allocation of at least the size of `Self`.
> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*this).timer)) }
> +    }
> +
> +    /// Cancel an initialized and potentially running timer.
> +    ///
> +    /// If the timer handler is running, this function will block until the
> +    /// handler returns.
> +    ///
> +    /// Note that the timer might be started by a concurrent start operation. If
> +    /// so, the timer might not be in the **stopped** state when this function
> +    /// returns.
> +    ///
> +    /// Users of the `HrTimer` API would not usually call this method directly.
> +    /// Instead they would use the safe [`HrTimerHandle::cancel`] on the handle
> +    /// returned when the timer was started.
> +    ///
> +    /// This function is useful to get access to the value without creating
> +    /// intermediate references.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `this` must point to a valid `Self`.
> +    #[allow(dead_code)]
> +    pub(crate) unsafe fn raw_cancel(this: *const Self) -> bool {
> +        // SAFETY: `this` points to an allocation of at least `HrTimer` size.
> +        let c_timer_ptr = unsafe { HrTimer::raw_get(this) };
> +
> +        // If the handler is running, this will wait for the handler to return
> +        // before returning.
> +        // SAFETY: `c_timer_ptr` is initialized and valid. Synchronization is
> +        // handled on the C side.
> +        unsafe { bindings::hrtimer_cancel(c_timer_ptr) != 0 }
> +    }
> +}
> +
> +/// Implemented by pointer types that point to structs that contain a [`HrTimer`].
> +///
> +/// `Self` must be [`Sync`] because it is passed to timer callbacks 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.
> +pub trait HrTimerPointer: Sync + Sized {
> +    /// 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: When implementing this trait, consider that it is not unsafe 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 {
> +    /// This type is passed to [`HrTimerCallback::run`]. It may be a borrow of
> +    /// [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the

This part of the docs no longer makes sense. You probably mean to say
`Self` instead, right?

> +    /// implementation can guarantee correct access (exclusive or shared
> +    /// depending on the type) to the target during timer handler execution.
> +    type CallbackTarget<'a>;
> +
> +    /// Callback to be called from C when timer fires.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Only to be called by C code in the `hrtimer` subsystem. `this` must point
> +    /// to the `bindings::hrtimer` structure that was used to start the timer.
> +    unsafe extern "C" fn run(this: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
> +}
> +
> +/// Implemented by structs that can be the target of a timer callback.
> +pub trait HrTimerCallback {
> +    /// The type whose [`RawHrTimerCallback::run`] method will be invoked when
> +    /// the timer expires.
> +    type Pointer<'a>: RawHrTimerCallback;
> +
> +    /// Called by the timer logic when the timer fires.
> +    fn run(this: <Self::Pointer<'_> as RawHrTimerCallback>::CallbackTarget<'_>)
> +    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 return before returning.
> +///
> +/// Note: One way to satisfy the safety requirement is to call `Self::cancel` in
> +/// the drop implementation for `Self.`
> +pub unsafe trait HrTimerHandle {
> +    /// Cancel the timer. If the timer is in the running state, block till the
> +    /// handler has returned.
> +    ///
> +    /// Note that the timer might be started by a concurrent start operation. If
> +    /// so, the timer might not be in the **stopped** state when this function
> +    /// returns.
> +    ///
> +    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

This link is unused.

> +pub unsafe trait HasHrTimer<T> {
> +    /// Return a pointer to the [`HrTimer`] within `Self`.
> +    ///
> +    /// This function is useful to get access to the value without creating
> +    /// intermediate references.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `this` must point to a valid struct of type `Self`.

I don't think that this is the correct requirement. The pointer `this`
must be valid (i.e. dereferenceable), but the value that we're pointing
at doesn't have to be valid, right?

Same below.

> +    unsafe fn raw_get_timer(this: *const Self) -> *const HrTimer<T>;
> +
> +    /// Return a pointer to the struct that is containing the [`HrTimer`] pointed
> +    /// to by `ptr`.
> +    ///
> +    /// This function is useful to get access to the value without creating
> +    /// intermediate references.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must point to a [`HrTimer<T>`] field in a struct of type `Self`.
> +    unsafe fn timer_container_of(ptr: *mut HrTimer<T>) -> *mut Self
> +    where
> +        Self: Sized;
> +
> +    /// Get pointer to the contained `bindings::hrtimer` struct.
> +    ///
> +    /// This function is useful to get access to the value without creating
> +    /// intermediate references.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `this` must point to a valid `Self`.
> +    unsafe fn c_timer_ptr(this: *const Self) -> *const bindings::hrtimer {
> +        // SAFETY: `this` is a valid pointer to a `Self`.
> +        let timer_ptr = unsafe { Self::raw_get_timer(this) };
> +
> +        // 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
> +    ///
> +    /// - `this` must point to a valid `Self`.

Here the requirement is correct, since you need that for
`hrtimer_start_range_ns`.

> +    /// - Caller must ensure that `self` lives until the timer fires or is

There is no `self`, do you mean the value living behind `this`?

> +    ///   canceled.
> +    unsafe fn start(this: *const Self, expires: Ktime) {
> +        // SAFETY: By function safety requirement, `this`is a valid `Self`.
> +        unsafe {
> +            bindings::hrtimer_start_range_ns(
> +                Self::c_timer_ptr(this).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 {
> +
> +            #[inline]
> +            unsafe fn raw_get_timer(this: *const Self) ->
> +                *const $crate::time::hrtimer::HrTimer<$timer_type>
> +            {
> +                // SAFETY: The caller promises that the pointer is not dangling.
> +                unsafe {
> +                    ::core::ptr::addr_of!((*this).$field)
> +                }
> +            }
> +
> +            #[inline]
> +            unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_type>) ->
> +                *mut Self

This formatting looks a bit weird, (macro formatting is annoying, I
know).

---
Cheers,
Benno

> +            {
> +                // SAFETY: As per the safety requirement of this function, `ptr`
> +                // is pointing inside a `$timer_type`.
> +                unsafe {
> +                    ::kernel::container_of!(ptr, $timer_type, $field).cast_mut()
> +                }
> +            }
> +        }
> +    }
> +}
>
> --
> 2.47.0

Re: [PATCH v10 03/13] rust: hrtimer: implement `HrTimerPointer` for `Arc`

[Benno Lossin]

On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
> +impl<T> HrTimerPointer for Arc<T>
> +where
> +    T: 'static,
> +    T: Send + Sync,
> +    T: HasHrTimer<T>,
> +    T: for<'a> HrTimerCallback<Pointer<'a> = Self>,
> +    Arc<T>: for<'a> RawHrTimerCallback<CallbackTarget<'a> = ArcBorrow<'a, T>>,

I don't understand why you need this bound here.

> +{
> +    type TimerHandle = ArcHrTimerHandle<T>;
> +
> +    fn start(self, expires: Ktime) -> ArcHrTimerHandle<T> {
> +        // SAFETY:
> +        //  - We keep `self` alive by wrapping it in a handle below.
> +        //  - Since we generate the pointer passed to `start` from a valid
> +        //    reference, it is a valid pointer.
> +        unsafe { T::start(Arc::as_ptr(&self), expires) };
> +        ArcHrTimerHandle { inner: self }
> +    }
> +}
> +
> +impl<T> RawHrTimerCallback for Arc<T>
> +where
> +    T: 'static,
> +    T: HasHrTimer<T>,
> +    T: for<'a> HrTimerCallback<Pointer<'a> = Self>,
> +{
> +    type CallbackTarget<'a> = ArcBorrow<'a, T>;
> +
> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
> +        // `HrTimer` is `repr(C)`
> +        let timer_ptr = ptr.cast::<super::HrTimer<T>>();
> +
> +        // 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 { T::timer_container_of(timer_ptr) };
> +
> +        // SAFETY: `data_ptr` points to the `T` that was used to queue the
> +        // timer. This `T` is contained in an `Arc`.

You're not justifying all safety requirements of `ArcBorrow::from_raw`.

---
Cheers,
Benno

> +        let receiver = unsafe { ArcBorrow::from_raw(data_ptr) };
> +
> +        T::run(receiver);
> +
> +        bindings::hrtimer_restart_HRTIMER_NORESTART
> +    }
> +}
>
> --
> 2.47.0

Re: [PATCH v10 04/13] rust: hrtimer: allow timer restart from timer handler

[Benno Lossin]

On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
> Allow timer handlers to report that they want a timer to be restarted after
> the timer handler has finished executing.
>
> Acked-by: Frederic Weisbecker <frederic@kernel.org>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

---
Cheers,
Benno

Re: [PATCH v10 01/13] rust: hrtimer: introduce hrtimer support

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>> Add support for intrusive use of the hrtimer system. For now,
>> only add support for embedding one timer per Rust struct.
>>
>> The hrtimer Rust API is based on the intrusive style pattern introduced by
>> the Rust workqueue API.
>>
>> Acked-by: Frederic Weisbecker <frederic@kernel.org>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> Some smaller changes below, with those fixed:
>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>

Thanks!

>
>> ---
>>  rust/kernel/time.rs         |   2 +
>>  rust/kernel/time/hrtimer.rs | 359 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 361 insertions(+)
>>
>> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
>> index 379c0f5772e5..fab1dadfa589 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).
>>
>> +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 000000000000..7d7d490f8b6f
>> --- /dev/null
>> +++ b/rust/kernel/time/hrtimer.rs
>> @@ -0,0 +1,359 @@
>> +// 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
>> +//!
>> +//! States:
>> +//!
>> +//! - Stopped: initialized but not started, or cancelled, or not restarted.
>> +//! - Started: initialized and started or restarted.
>> +//! - Running: executing the callback.
>> +//!
>> +//! Operations:
>> +//!
>> +//! * Start
>> +//! * Cancel
>> +//! * Restart
>> +//!
>> +//! Events:
>> +//!
>> +//! * Expire
>> +//!
>> +//! ## State Diagram
>> +//!
>> +//! ```text
>> +//!                                                   Return NoRestart
>> +//!                       +---------------------------------------------------------------------+
>> +//!                       |                                                                     |
>> +//!                       |                                                                     |
>> +//!                       |                                                                     |
>> +//!                       |                                         Return Restart              |
>> +//!                       |                                      +------------------------+     |
>> +//!                       |                                      |                        |     |
>> +//!                       |                                      |                        |     |
>> +//!                       v                                      v                        |     |
>> +//!           +-----------------+      Start      +------------------+           +--------+-----+--+
>> +//!           |                 +---------------->|                  |           |                 |
>> +//! Init      |                 |                 |                  |  Expire   |                 |
>> +//! --------->|    Stopped      |                 |      Started     +---------->|     Running     |
>> +//!           |                 |     Cancel      |                  |           |                 |
>> +//!           |                 |<----------------+                  |           |                 |
>> +//!           +-----------------+                 +---------------+--+           +-----------------+
>> +//!                                                     ^         |
>> +//!                                                     |         |
>> +//!                                                     +---------+
>> +//!                                                      Restart
>> +//! ```
>> +//!
>> +//!
>> +//! A timer is initialized in the **stopped** state. A stopped timer can be
>> +//! **started** by the `start` operation, with an **expiry** time. After the
>> +//! `start` operation, the timer is in the **started** state. When the timer
>> +//! **expires**, the timer enters the **running** state and the handler is
>> +//! executed. After the handler has returned, the timer may enter the
>> +//! **started* or **stopped** state, depending on the return value of the
>> +//! handler. A timer in the **started** or **running** state may be **canceled**
>> +//! by the `cancel` operation. A timer that is cancelled enters the **stopped**
>> +//! state.
>
> This looks very nice, thanks!
>
>> +//!
>> +//! A `cancel` or `restart` operation on a timer in the **running** state takes
>> +//! effect after the handler has returned and the timer has transitioned
>> +//! out of the **running** state.
>> +//!
>> +//! A `restart` operation on a timer in the **stopped** state is equivalent to a
>> +//! `start` operation.
>> +
>> +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<T> {
>> +    #[pin]
>> +    timer: Opaque<bindings::hrtimer>,
>> +    _t: PhantomData<T>,
>> +}
>> +
>> +// SAFETY: Ownership of an `HrTimer` can be moved to other threads and
>> +// used/dropped from there.
>> +unsafe impl<T> Send for HrTimer<T> {}
>> +
>> +// SAFETY: Timer operations are locked on the C side, so it is safe to operate
>> +// on a timer from multiple threads.
>> +unsafe impl<T> Sync for HrTimer<T> {}
>> +
>> +impl<T> HrTimer<T> {
>> +    /// Return an initializer for a new timer instance.
>> +    pub fn new() -> impl PinInit<Self>
>> +    where
>> +        T: HrTimerCallback,
>> +    {
>> +        pin_init!(Self {
>> +            // INVARIANT: 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 to a
>> +                // 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::Pointer::run),
>> +                        bindings::CLOCK_MONOTONIC as i32,
>> +                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
>> +                    );
>> +                }
>> +            }),
>> +            _t: PhantomData,
>> +        })
>> +    }
>> +
>> +    /// Get a pointer to the contained `bindings::hrtimer`.
>> +    ///
>> +    /// This function is useful to get access to the value without creating
>> +    /// intermediate references.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// `this` must point to a live allocation of at least the size of `Self`.
>> +    unsafe fn raw_get(this: *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 `this` points to an
>> +        // allocation of at least the size of `Self`.
>> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*this).timer)) }
>> +    }
>> +
>> +    /// Cancel an initialized and potentially running timer.
>> +    ///
>> +    /// If the timer handler is running, this function will block until the
>> +    /// handler returns.
>> +    ///
>> +    /// Note that the timer might be started by a concurrent start operation. If
>> +    /// so, the timer might not be in the **stopped** state when this function
>> +    /// returns.
>> +    ///
>> +    /// Users of the `HrTimer` API would not usually call this method directly.
>> +    /// Instead they would use the safe [`HrTimerHandle::cancel`] on the handle
>> +    /// returned when the timer was started.
>> +    ///
>> +    /// This function is useful to get access to the value without creating
>> +    /// intermediate references.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// `this` must point to a valid `Self`.
>> +    #[allow(dead_code)]
>> +    pub(crate) unsafe fn raw_cancel(this: *const Self) -> bool {
>> +        // SAFETY: `this` points to an allocation of at least `HrTimer` size.
>> +        let c_timer_ptr = unsafe { HrTimer::raw_get(this) };
>> +
>> +        // If the handler is running, this will wait for the handler to return
>> +        // before returning.
>> +        // SAFETY: `c_timer_ptr` is initialized and valid. Synchronization is
>> +        // handled on the C side.
>> +        unsafe { bindings::hrtimer_cancel(c_timer_ptr) != 0 }
>> +    }
>> +}
>> +
>> +/// Implemented by pointer types that point to structs that contain a [`HrTimer`].
>> +///
>> +/// `Self` must be [`Sync`] because it is passed to timer callbacks 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.
>> +pub trait HrTimerPointer: Sync + Sized {
>> +    /// 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: When implementing this trait, consider that it is not unsafe 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 {
>> +    /// This type is passed to [`HrTimerCallback::run`]. It may be a borrow of
>> +    /// [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
>
> This part of the docs no longer makes sense. You probably mean to say
> `Self` instead, right?

Yes:

    /// This passed passed to [`HrTimerCallback::run`]. It may be [`Self`], or a
    /// pointer type derived from [`Self`].

>
>> +    /// implementation can guarantee correct access (exclusive or shared
>> +    /// depending on the type) to the target during timer handler execution.
>> +    type CallbackTarget<'a>;
>> +
>> +    /// Callback to be called from C when timer fires.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// Only to be called by C code in the `hrtimer` subsystem. `this` must point
>> +    /// to the `bindings::hrtimer` structure that was used to start the timer.
>> +    unsafe extern "C" fn run(this: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
>> +}
>> +
>> +/// Implemented by structs that can be the target of a timer callback.
>> +pub trait HrTimerCallback {
>> +    /// The type whose [`RawHrTimerCallback::run`] method will be invoked when
>> +    /// the timer expires.
>> +    type Pointer<'a>: RawHrTimerCallback;
>> +
>> +    /// Called by the timer logic when the timer fires.
>> +    fn run(this: <Self::Pointer<'_> as RawHrTimerCallback>::CallbackTarget<'_>)
>> +    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 return before returning.
>> +///
>> +/// Note: One way to satisfy the safety requirement is to call `Self::cancel` in
>> +/// the drop implementation for `Self.`
>> +pub unsafe trait HrTimerHandle {
>> +    /// Cancel the timer. If the timer is in the running state, block till the
>> +    /// handler has returned.
>> +    ///
>> +    /// Note that the timer might be started by a concurrent start operation. If
>> +    /// so, the timer might not be in the **stopped** state when this function
>> +    /// returns.
>> +    ///
>> +    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
>
> This link is unused.

Thanks.

>
>> +pub unsafe trait HasHrTimer<T> {
>> +    /// Return a pointer to the [`HrTimer`] within `Self`.
>> +    ///
>> +    /// This function is useful to get access to the value without creating
>> +    /// intermediate references.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// `this` must point to a valid struct of type `Self`.
>
> I don't think that this is the correct requirement. The pointer `this`
> must be valid (i.e. dereferenceable), but the value that we're pointing
> at doesn't have to be valid, right?

You are right:


    /// `this` must be a valid pointer.


>
> Same below.

Right.

I shall not update the safety requirement at the call sites, because "`this`
must point to a valid `Self`" is a stronger requirement, so those are
all fine.

>
>> +    unsafe fn raw_get_timer(this: *const Self) -> *const HrTimer<T>;
>> +
>> +    /// Return a pointer to the struct that is containing the [`HrTimer`] pointed
>> +    /// to by `ptr`.
>> +    ///
>> +    /// This function is useful to get access to the value without creating
>> +    /// intermediate references.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// `ptr` must point to a [`HrTimer<T>`] field in a struct of type `Self`.
>> +    unsafe fn timer_container_of(ptr: *mut HrTimer<T>) -> *mut Self
>> +    where
>> +        Self: Sized;
>> +
>> +    /// Get pointer to the contained `bindings::hrtimer` struct.
>> +    ///
>> +    /// This function is useful to get access to the value without creating
>> +    /// intermediate references.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// `this` must point to a valid `Self`.
>> +    unsafe fn c_timer_ptr(this: *const Self) -> *const bindings::hrtimer {
>> +        // SAFETY: `this` is a valid pointer to a `Self`.
>> +        let timer_ptr = unsafe { Self::raw_get_timer(this) };
>> +
>> +        // 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
>> +    ///
>> +    /// - `this` must point to a valid `Self`.
>
> Here the requirement is correct, since you need that for
> `hrtimer_start_range_ns`.

Yes.

>
>> +    /// - Caller must ensure that `self` lives until the timer fires or is
>
> There is no `self`, do you mean the value living behind `this`?

Yes, will change:

    /// - Caller must ensure that the pointee of `this` lives until the timer
    ///   fires or is canceled.

>
>> +    ///   canceled.
>> +    unsafe fn start(this: *const Self, expires: Ktime) {
>> +        // SAFETY: By function safety requirement, `this`is a valid `Self`.
>> +        unsafe {
>> +            bindings::hrtimer_start_range_ns(
>> +                Self::c_timer_ptr(this).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 {
>> +
>> +            #[inline]
>> +            unsafe fn raw_get_timer(this: *const Self) ->
>> +                *const $crate::time::hrtimer::HrTimer<$timer_type>
>> +            {
>> +                // SAFETY: The caller promises that the pointer is not dangling.
>> +                unsafe {
>> +                    ::core::ptr::addr_of!((*this).$field)
>> +                }
>> +            }
>> +
>> +            #[inline]
>> +            unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_type>) ->
>> +                *mut Self
>
> This formatting looks a bit weird, (macro formatting is annoying, I
> know).

How would you change it?


Best regards,
Andreas Hindborg

Re: [PATCH v10 07/13] rust: hrtimer: implement `UnsafeHrTimerPointer` for `Pin<&T>`

[Benno Lossin]

On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
> Allow pinned references to structs that contain a `HrTimer` node to be
> scheduled with the `hrtimer` subsystem.
>
> Acked-by: Frederic Weisbecker <frederic@kernel.org>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/time/hrtimer.rs     |  2 +
>  rust/kernel/time/hrtimer/pin.rs | 99 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 101 insertions(+)
>
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index d90a25785f87..2ca56397eade 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -439,3 +439,5 @@ unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_typ
>
>  mod arc;
>  pub use arc::ArcHrTimerHandle;
> +mod pin;
> +pub use pin::PinHrTimerHandle;
> diff --git a/rust/kernel/time/hrtimer/pin.rs b/rust/kernel/time/hrtimer/pin.rs
> new file mode 100644
> index 000000000000..6c9f2190f8e1
> --- /dev/null
> +++ b/rust/kernel/time/hrtimer/pin.rs
> @@ -0,0 +1,99 @@
> +// 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, T>
> +where
> +    T: HasHrTimer<T>,
> +{
> +    pub(crate) inner: Pin<&'a T>,
> +}
> +
> +// 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, T> HrTimerHandle for PinHrTimerHandle<'a, T>
> +where
> +    T: HasHrTimer<T>,
> +{
> +    fn cancel(&mut self) -> bool {
> +        let self_ptr: *const T = self.inner.get_ref();
> +
> +        // SAFETY: As we got `self_ptr` from a reference above, it must point to
> +        // a valid `T`.
> +        let timer_ptr = unsafe { <T as HasHrTimer<T>>::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::<T>::raw_cancel(timer_ptr) }
> +    }
> +}
> +
> +impl<'a, T> Drop for PinHrTimerHandle<'a, T>
> +where
> +    T: HasHrTimer<T>,
> +{
> +    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, T> UnsafeHrTimerPointer for Pin<&'a T>
> +where
> +    T: Send + Sync,
> +    T: HasHrTimer<T>,
> +    T: HrTimerCallback<Pointer<'a> = Self>,
> +    Pin<&'a T>: RawHrTimerCallback<CallbackTarget<'a> = Self>,
> +{
> +    type TimerHandle = PinHrTimerHandle<'a, T>;
> +
> +    unsafe fn start(self, expires: Ktime) -> Self::TimerHandle {
> +        // Cast to pointer
> +        let self_ptr: *const T = <Self as core::ops::Deref>::deref(&self);

Why use deref? `get_ref` seems much cleaner.

> +
> +        // SAFETY:
> +        //  - As we derive `self_ptr` from a reference above, it must point to a
> +        //    valid `T`.
> +        //  - We keep `self` alive by wrapping it in a handle below.
> +        unsafe { T::start(self_ptr, expires) };
> +
> +        PinHrTimerHandle { inner: self }
> +    }
> +}
> +
> +impl<'a, T> RawHrTimerCallback for Pin<&'a T>
> +where
> +    T: HasHrTimer<T>,
> +    T: HrTimerCallback<Pointer<'a> = Self>,
> +{
> +    type CallbackTarget<'b> = Self;
> +
> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
> +        // `HrTimer` is `repr(C)`
> +        let timer_ptr = ptr as *mut HrTimer<T>;
> +
> +        // SAFETY: By the safety requirement of this function, `timer_ptr`
> +        // points to a `HrTimer<T>` contained in an `T`.
> +        let receiver_ptr = unsafe { T::timer_container_of(timer_ptr) };
> +
> +        // SAFETY: By the safety requirement of this function, `timer_ptr`
> +        // points to a `HrTimer<T>` contained in an `T`.

This justification seems wrong it talks about `HrTimer<T>`, but here we
have a `*const T`... Also see [1] (I am mainly interested in your
justification for the lifetime).

[1]: https://doc.rust-lang.org/std/ptr/index.html#pointer-to-reference-conversion

---
Cheers,
Benno

> +        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) };
> +
> +        T::run(receiver_pin).into_c()
> +    }
> +}
>
> --
> 2.47.0

Re: [PATCH v10 08/13] rust: hrtimer: implement `UnsafeHrTimerPointer` for `Pin<&mut T>`

[Benno Lossin]

On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
> Allow pinned mutable references to structs that contain a `HrTimer` node to
> be scheduled with the `hrtimer` subsystem.
>
> Acked-by: Frederic Weisbecker <frederic@kernel.org>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/time/hrtimer.rs         |   2 +
>  rust/kernel/time/hrtimer/pin_mut.rs | 101 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 103 insertions(+)
>
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index 2ca56397eade..d2791fd624b7 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -441,3 +441,5 @@ unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_typ
>  pub use arc::ArcHrTimerHandle;
>  mod pin;
>  pub use pin::PinHrTimerHandle;
> +mod pin_mut;
> +pub use pin_mut::PinMutHrTimerHandle;
> diff --git a/rust/kernel/time/hrtimer/pin_mut.rs b/rust/kernel/time/hrtimer/pin_mut.rs
> new file mode 100644
> index 000000000000..4f4a9e9602d8
> --- /dev/null
> +++ b/rust/kernel/time/hrtimer/pin_mut.rs
> @@ -0,0 +1,101 @@
> +// 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, T>
> +where
> +    T: HasHrTimer<T>,
> +{
> +    pub(crate) inner: Pin<&'a mut T>,
> +}
> +
> +// 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, T> HrTimerHandle for PinMutHrTimerHandle<'a, T>
> +where
> +    T: HasHrTimer<T>,
> +{
> +    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 T };
> +
> +        // SAFETY: As we got `self_ptr` from a reference above, it must point to
> +        // a valid `T`.
> +        let timer_ptr = unsafe { <T as HasHrTimer<T>>::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::<T>::raw_cancel(timer_ptr) }
> +    }
> +}
> +
> +impl<'a, T> Drop for PinMutHrTimerHandle<'a, T>
> +where
> +    T: HasHrTimer<T>,
> +{
> +    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, T> UnsafeHrTimerPointer for Pin<&'a mut T>
> +where
> +    T: Send + Sync,
> +    T: HasHrTimer<T>,
> +    T: HrTimerCallback<Pointer<'a> = Self>,
> +    Pin<&'a mut T>: RawHrTimerCallback<CallbackTarget<'a> = Self>,
> +{
> +    type TimerHandle = PinMutHrTimerHandle<'a, T>;
> +
> +    unsafe fn start(self, expires: Ktime) -> Self::TimerHandle {
> +        // Cast to pointer
> +        let self_ptr: *const T = <Self as core::ops::Deref>::deref(&self);

You cannot go through a shared reference here, since you convert the
pointer obtained here in the `run` function later back into a mutable
reference. You will have to use `get_unchecked_mut` or
`into_inner_unchecked`.

> +
> +        // SAFETY:
> +        //  - As we derive `self_ptr` from a reference above, it must point to a
> +        //    valid `T`.
> +        //  - We keep `self` alive by wrapping it in a handle below.
> +        unsafe { T::start(self_ptr, expires) };
> +
> +        PinMutHrTimerHandle { inner: self }
> +    }
> +}
> +
> +impl<'a, T> RawHrTimerCallback for Pin<&'a mut T>
> +where
> +    T: HasHrTimer<T>,
> +    T: HrTimerCallback<Pointer<'a> = Self>,
> +{
> +    type CallbackTarget<'b> = Self;
> +
> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
> +        // `HrTimer` is `repr(C)`
> +        let timer_ptr = ptr as *mut HrTimer<T>;
> +
> +        // SAFETY: By the safety requirement of this function, `timer_ptr`
> +        // points to a `HrTimer<T>` contained in an `T`.
> +        let receiver_ptr = unsafe { T::timer_container_of(timer_ptr) };
> +
> +        // SAFETY: By the safety requirement of this function, `timer_ptr`
> +        // points to a `HrTimer<T>` contained in an `T`.

Same question here as for the `Pin<&T>` patch.

---
Cheers,
Benno

> +        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) };
> +
> +        T::run(receiver_pin).into_c()
> +    }
> +}
>
> --
> 2.47.0

Re: [PATCH v10 10/13] rust: hrtimer: implement `HrTimerPointer` for `Pin<Box<T>>`

[Benno Lossin]

On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
> Allow `Pin<Box<T>>` to be the target of a timer callback.
>
> Acked-by: Frederic Weisbecker <frederic@kernel.org>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/time/hrtimer.rs      |   3 ++
>  rust/kernel/time/hrtimer/tbox.rs | 109 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 112 insertions(+)
>
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index d2791fd624b7..991d37b0524a 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -443,3 +443,6 @@ unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_typ
>  pub use pin::PinHrTimerHandle;
>  mod pin_mut;
>  pub use pin_mut::PinMutHrTimerHandle;
> +// `box` is a reserved keyword, so prefix with `t` for timer
> +mod tbox;
> +pub use tbox::BoxHrTimerHandle;
> diff --git a/rust/kernel/time/hrtimer/tbox.rs b/rust/kernel/time/hrtimer/tbox.rs
> new file mode 100644
> index 000000000000..a3b2ed849050
> --- /dev/null
> +++ b/rust/kernel/time/hrtimer/tbox.rs
> @@ -0,0 +1,109 @@
> +// 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;
> +use core::ptr::NonNull;
> +
> +/// A handle for a [`Box<HasHrTimer<T>>`] returned by a call to
> +/// [`HrTimerPointer::start`].
> +pub struct BoxHrTimerHandle<T, A>

Should this type implement `Send` and `Sync` depending on `T`?

> +where
> +    T: HasHrTimer<T>,
> +    A: crate::alloc::Allocator,
> +{
> +    pub(crate) inner: NonNull<T>,
> +    _p: core::marker::PhantomData<A>,
> +}
> +
> +// SAFETY: We implement drop below, and we cancel the timer in the drop
> +// implementation.
> +unsafe impl<T, A> HrTimerHandle for BoxHrTimerHandle<T, A>
> +where
> +    T: HasHrTimer<T>,
> +    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 `T`.
> +        let timer_ptr = unsafe { <T as HasHrTimer<T>>::raw_get_timer(self.inner.as_ptr()) };
> +
> +        // SAFETY: As `timer_ptr` points into `T` and `T` is valid, `timer_ptr`
> +        // must point to a valid `HrTimer` instance.
> +        unsafe { HrTimer::<T>::raw_cancel(timer_ptr) }
> +    }
> +}
> +
> +impl<T, A> Drop for BoxHrTimerHandle<T, A>
> +where
> +    T: HasHrTimer<T>,
> +    A: crate::alloc::Allocator,
> +{
> +    fn drop(&mut self) {
> +        self.cancel();
> +        // SAFETY: `self.inner` came from a `Box::into_raw` call

Please add this as an invariant to `Self`.

> +        drop(unsafe { Box::<T, A>::from_raw(self.inner.as_ptr()) })
> +    }
> +}
> +
> +impl<T, A> HrTimerPointer for Pin<Box<T, A>>
> +where
> +    T: 'static,
> +    T: Send + Sync,
> +    T: HasHrTimer<T>,
> +    T: for<'a> HrTimerCallback<Pointer<'a> = Pin<Box<T, A>>>,
> +    Pin<Box<T, A>>: for<'a> RawHrTimerCallback<CallbackTarget<'a> = Pin<&'a T>>,

I don't think this is necessary.

> +    A: crate::alloc::Allocator,
> +{
> +    type TimerHandle = BoxHrTimerHandle<T, A>;
> +
> +    fn start(self, expires: Ktime) -> Self::TimerHandle {
> +        // SAFETY:
> +        //  - We will not move out of this box during timer callback (we pass an
> +        //    immutable reference to the callback).
> +        //  - `Box::into_raw` is guaranteed to return a valid pointer.
> +        let inner =
> +            unsafe { NonNull::new_unchecked(Box::into_raw(Pin::into_inner_unchecked(self))) };
> +
> +        // SAFETY:
> +        //  - We keep `self` alive by wrapping it in a handle below.
> +        //  - Since we generate the pointer passed to `start` from a valid
> +        //    reference, it is a valid pointer.
> +        unsafe { T::start(inner.as_ptr(), expires) };
> +
> +        BoxHrTimerHandle {
> +            inner,
> +            _p: core::marker::PhantomData,
> +        }
> +    }
> +}
> +
> +impl<T, A> RawHrTimerCallback for Pin<Box<T, A>>
> +where
> +    T: 'static,
> +    T: HasHrTimer<T>,
> +    T: for<'a> HrTimerCallback<Pointer<'a> = Pin<Box<T, A>>>,
> +    A: crate::alloc::Allocator,
> +{
> +    type CallbackTarget<'a> = Pin<&'a T>;

Why isn't this `Pin<&'a mut T>`?

> +
> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
> +        // `HrTimer` is `repr(C)`
> +        let timer_ptr = ptr.cast::<super::HrTimer<T>>();
> +
> +        // 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 { T::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::<T, A>::from_raw(data_ptr) }));

Since you turn this into a reference below and never run the drop, why
not turn the pointer directly into a reference?

---
Cheers,
Benno

> +
> +        T::run(tbox.as_ref()).into_c()
> +    }
> +}
>
> --
> 2.47.0

Re: [PATCH v10 11/13] rust: hrtimer: add `HrTimerMode`

[Benno Lossin]

On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
> Allow selection of timer mode by passing a `HrTimerMode` variant to
> `HrTimer::new`.
>
> Acked-by: Frederic Weisbecker <frederic@kernel.org>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

---
Cheers,
Benno

Re: [PATCH v10 12/13] rust: hrtimer: add clocksource selection through `ClockId`

[Benno Lossin]

On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
> Allow selecting a clock source for timers by passing a `ClockId`
> variant to `HrTimer::new`.
>
> Acked-by: Frederic Weisbecker <frederic@kernel.org>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

---
Cheers,
Benno

Re: [PATCH v10 03/13] rust: hrtimer: implement `HrTimerPointer` for `Arc`

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>> +impl<T> HrTimerPointer for Arc<T>
>> +where
>> +    T: 'static,
>> +    T: Send + Sync,
>> +    T: HasHrTimer<T>,
>> +    T: for<'a> HrTimerCallback<Pointer<'a> = Self>,
>> +    Arc<T>: for<'a> RawHrTimerCallback<CallbackTarget<'a> = ArcBorrow<'a, T>>,
>
> I don't understand why you need this bound here.

This impl is applicable only when `Arc<T> has an implementation of
`RawTimerCallback` where CallbackTarget<'a> = ArcBorrow<'a, T>. I don't
want the impl to be available if that is not the case.

It's just an additional check.

>
>> +{
>> +    type TimerHandle = ArcHrTimerHandle<T>;
>> +
>> +    fn start(self, expires: Ktime) -> ArcHrTimerHandle<T> {
>> +        // SAFETY:
>> +        //  - We keep `self` alive by wrapping it in a handle below.
>> +        //  - Since we generate the pointer passed to `start` from a valid
>> +        //    reference, it is a valid pointer.
>> +        unsafe { T::start(Arc::as_ptr(&self), expires) };
>> +        ArcHrTimerHandle { inner: self }
>> +    }
>> +}
>> +
>> +impl<T> RawHrTimerCallback for Arc<T>
>> +where
>> +    T: 'static,
>> +    T: HasHrTimer<T>,
>> +    T: for<'a> HrTimerCallback<Pointer<'a> = Self>,
>> +{
>> +    type CallbackTarget<'a> = ArcBorrow<'a, T>;
>> +
>> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
>> +        // `HrTimer` is `repr(C)`
>> +        let timer_ptr = ptr.cast::<super::HrTimer<T>>();
>> +
>> +        // 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 { T::timer_container_of(timer_ptr) };
>> +
>> +        // SAFETY: `data_ptr` points to the `T` that was used to queue the
>> +        // timer. This `T` is contained in an `Arc`.
>
> You're not justifying all safety requirements of `ArcBorrow::from_raw`.

How is this:

        // SAFETY:
        //  - `data_ptr` is derived form the pointer to the `T` that was used to
        //    queue the timer.
        //  - The `ArcTimerHandle` associated with this timer is guaranteed to
        //    be alive for the duration of the lifetime of `receiver`, so the
        //    refcount of the underlying `Arc` is guaranteed to be nonzero for
        //    the duration.
        //  - We own one refcount in the `ArcTimerHandle` associted with this
        //    timer, so it is not possible to get a `UniqueArc` to this
        //    allocation from other `Arc` clones.


Best regards,
Andreas Hindborg

Re: [PATCH v10 13/13] rust: hrtimer: add maintainer entry

[Benno Lossin]

On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
> Add Andreas Hindborg as maintainer for Rust `hrtimer` abstractions. Also
> add Boqun Feng as reviewer.
>
> Acked-by: Boqun Feng <boqun.feng@gmail.com>
> Acked-by: Frederic Weisbecker <frederic@kernel.org>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>

I don't recall adding my reviewed by for this patch (I normally don't
review MAINTAINERS changes, since there isn't anything to review). And I
don't find it in lore, how did this end up here?

---
Cheers,
Benno

> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  MAINTAINERS | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 896a307fa065..ba8e802faabf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10355,6 +10355,21 @@ 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>
> +R:	Frederic Weisbecker <frederic@kernel.org>
> +R:	Lyude Paul <lyude@redhat.com>
> +R:	Thomas Gleixner <tglx@linutronix.de>
> +R:	Anna-Maria Behnsen <anna-maria@linutronix.de>
> +L:	rust-for-linux@vger.kernel.org
> +S:	Supported
> +W:	https://rust-for-linux.com
> +B:	https://github.com/Rust-for-Linux/linux/issues
> +T:	git https://github.com/Rust-for-Linux/linux.git hrtimer-next
> +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

Re: [PATCH v10 03/13] rust: hrtimer: implement `HrTimerPointer` for `Arc`

[Benno Lossin]

On Fri Mar 7, 2025 at 2:27 PM CET, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
>
>> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>>> +impl<T> HrTimerPointer for Arc<T>
>>> +where
>>> +    T: 'static,
>>> +    T: Send + Sync,
>>> +    T: HasHrTimer<T>,
>>> +    T: for<'a> HrTimerCallback<Pointer<'a> = Self>,
>>> +    Arc<T>: for<'a> RawHrTimerCallback<CallbackTarget<'a> = ArcBorrow<'a, T>>,
>>
>> I don't understand why you need this bound here.
>
> This impl is applicable only when `Arc<T> has an implementation of
> `RawTimerCallback` where CallbackTarget<'a> = ArcBorrow<'a, T>. I don't
> want the impl to be available if that is not the case.

The impl below has less strict other bounds than this one, so this bound
doesn't change anything.

> It's just an additional check.

To me it's just additional noise.

>>> +{
>>> +    type TimerHandle = ArcHrTimerHandle<T>;
>>> +
>>> +    fn start(self, expires: Ktime) -> ArcHrTimerHandle<T> {
>>> +        // SAFETY:
>>> +        //  - We keep `self` alive by wrapping it in a handle below.
>>> +        //  - Since we generate the pointer passed to `start` from a valid
>>> +        //    reference, it is a valid pointer.
>>> +        unsafe { T::start(Arc::as_ptr(&self), expires) };
>>> +        ArcHrTimerHandle { inner: self }
>>> +    }
>>> +}
>>> +
>>> +impl<T> RawHrTimerCallback for Arc<T>
>>> +where
>>> +    T: 'static,
>>> +    T: HasHrTimer<T>,
>>> +    T: for<'a> HrTimerCallback<Pointer<'a> = Self>,
>>> +{
>>> +    type CallbackTarget<'a> = ArcBorrow<'a, T>;
>>> +
>>> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
>>> +        // `HrTimer` is `repr(C)`
>>> +        let timer_ptr = ptr.cast::<super::HrTimer<T>>();
>>> +
>>> +        // 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 { T::timer_container_of(timer_ptr) };
>>> +
>>> +        // SAFETY: `data_ptr` points to the `T` that was used to queue the
>>> +        // timer. This `T` is contained in an `Arc`.
>>
>> You're not justifying all safety requirements of `ArcBorrow::from_raw`.
>
> How is this:
>
>         // SAFETY:
>         //  - `data_ptr` is derived form the pointer to the `T` that was used to
>         //    queue the timer.
>         //  - The `ArcTimerHandle` associated with this timer is guaranteed to
>         //    be alive for the duration of the lifetime of `receiver`, so the

There is no `receiver` in this context?

Is the reason for the handle staying alive that when it is dropped, it
calls `cancel` and that waits until the callback finishes? If so, did
you write that down somewhere here?

>         //    refcount of the underlying `Arc` is guaranteed to be nonzero for
>         //    the duration.
>         //  - We own one refcount in the `ArcTimerHandle` associted with this
>         //    timer, so it is not possible to get a `UniqueArc` to this
>         //    allocation from other `Arc` clones.

Otherwise this sounds good.

---
Cheers,
Benno

Re: [PATCH v10 07/13] rust: hrtimer: implement `UnsafeHrTimerPointer` for `Pin<&T>`

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>> Allow pinned references to structs that contain a `HrTimer` node to be
>> scheduled with the `hrtimer` subsystem.
>>
>> Acked-by: Frederic Weisbecker <frederic@kernel.org>
>> Reviewed-by: Lyude Paul <lyude@redhat.com>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/time/hrtimer.rs     |  2 +
>>  rust/kernel/time/hrtimer/pin.rs | 99 +++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 101 insertions(+)
>>
>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>> index d90a25785f87..2ca56397eade 100644
>> --- a/rust/kernel/time/hrtimer.rs
>> +++ b/rust/kernel/time/hrtimer.rs
>> @@ -439,3 +439,5 @@ unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_typ
>>
>>  mod arc;
>>  pub use arc::ArcHrTimerHandle;
>> +mod pin;
>> +pub use pin::PinHrTimerHandle;
>> diff --git a/rust/kernel/time/hrtimer/pin.rs b/rust/kernel/time/hrtimer/pin.rs
>> new file mode 100644
>> index 000000000000..6c9f2190f8e1
>> --- /dev/null
>> +++ b/rust/kernel/time/hrtimer/pin.rs
>> @@ -0,0 +1,99 @@
>> +// 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, T>
>> +where
>> +    T: HasHrTimer<T>,
>> +{
>> +    pub(crate) inner: Pin<&'a T>,
>> +}
>> +
>> +// 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, T> HrTimerHandle for PinHrTimerHandle<'a, T>
>> +where
>> +    T: HasHrTimer<T>,
>> +{
>> +    fn cancel(&mut self) -> bool {
>> +        let self_ptr: *const T = self.inner.get_ref();
>> +
>> +        // SAFETY: As we got `self_ptr` from a reference above, it must point to
>> +        // a valid `T`.
>> +        let timer_ptr = unsafe { <T as HasHrTimer<T>>::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::<T>::raw_cancel(timer_ptr) }
>> +    }
>> +}
>> +
>> +impl<'a, T> Drop for PinHrTimerHandle<'a, T>
>> +where
>> +    T: HasHrTimer<T>,
>> +{
>> +    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, T> UnsafeHrTimerPointer for Pin<&'a T>
>> +where
>> +    T: Send + Sync,
>> +    T: HasHrTimer<T>,
>> +    T: HrTimerCallback<Pointer<'a> = Self>,
>> +    Pin<&'a T>: RawHrTimerCallback<CallbackTarget<'a> = Self>,
>> +{
>> +    type TimerHandle = PinHrTimerHandle<'a, T>;
>> +
>> +    unsafe fn start(self, expires: Ktime) -> Self::TimerHandle {
>> +        // Cast to pointer
>> +        let self_ptr: *const T = <Self as core::ops::Deref>::deref(&self);
>
> Why use deref? `get_ref` seems much cleaner.

Sure.

>
>> +
>> +        // SAFETY:
>> +        //  - As we derive `self_ptr` from a reference above, it must point to a
>> +        //    valid `T`.
>> +        //  - We keep `self` alive by wrapping it in a handle below.
>> +        unsafe { T::start(self_ptr, expires) };
>> +
>> +        PinHrTimerHandle { inner: self }
>> +    }
>> +}
>> +
>> +impl<'a, T> RawHrTimerCallback for Pin<&'a T>
>> +where
>> +    T: HasHrTimer<T>,
>> +    T: HrTimerCallback<Pointer<'a> = Self>,
>> +{
>> +    type CallbackTarget<'b> = Self;
>> +
>> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
>> +        // `HrTimer` is `repr(C)`
>> +        let timer_ptr = ptr as *mut HrTimer<T>;
>> +
>> +        // SAFETY: By the safety requirement of this function, `timer_ptr`
>> +        // points to a `HrTimer<T>` contained in an `T`.
>> +        let receiver_ptr = unsafe { T::timer_container_of(timer_ptr) };
>> +
>> +        // SAFETY: By the safety requirement of this function, `timer_ptr`
>> +        // points to a `HrTimer<T>` contained in an `T`.
>
> This justification seems wrong it talks about `HrTimer<T>`, but here we
> have a `*const T`... Also see [1] (I am mainly interested in your
> justification for the lifetime).
>
> [1]: https://doc.rust-lang.org/std/ptr/index.html#pointer-to-reference-conversion

How is this:

        // SAFETY:
        //  - By the safety requirement of this function, `timer_ptr`
        //    points to a `HrTimer<T>` contained in an `T`.
        //  - The `PinHrTimerHandle` associated with this timer is guaranteed to
        //    be alive until this method returns. As the handle borrows from
        //    `T`, `T` is also guaranteed to be alive for the duration of this
        //    function.


Best regards,
Andreas Hindborg

Re: [PATCH v10 08/13] rust: hrtimer: implement `UnsafeHrTimerPointer` for `Pin<&mut T>`

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>> Allow pinned mutable references to structs that contain a `HrTimer` node to
>> be scheduled with the `hrtimer` subsystem.
>>
>> Acked-by: Frederic Weisbecker <frederic@kernel.org>
>> Reviewed-by: Lyude Paul <lyude@redhat.com>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/time/hrtimer.rs         |   2 +
>>  rust/kernel/time/hrtimer/pin_mut.rs | 101 ++++++++++++++++++++++++++++++++++++
>>  2 files changed, 103 insertions(+)
>>
>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>> index 2ca56397eade..d2791fd624b7 100644
>> --- a/rust/kernel/time/hrtimer.rs
>> +++ b/rust/kernel/time/hrtimer.rs
>> @@ -441,3 +441,5 @@ unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_typ
>>  pub use arc::ArcHrTimerHandle;
>>  mod pin;
>>  pub use pin::PinHrTimerHandle;
>> +mod pin_mut;
>> +pub use pin_mut::PinMutHrTimerHandle;
>> diff --git a/rust/kernel/time/hrtimer/pin_mut.rs b/rust/kernel/time/hrtimer/pin_mut.rs
>> new file mode 100644
>> index 000000000000..4f4a9e9602d8
>> --- /dev/null
>> +++ b/rust/kernel/time/hrtimer/pin_mut.rs
>> @@ -0,0 +1,101 @@
>> +// 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, T>
>> +where
>> +    T: HasHrTimer<T>,
>> +{
>> +    pub(crate) inner: Pin<&'a mut T>,
>> +}
>> +
>> +// 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, T> HrTimerHandle for PinMutHrTimerHandle<'a, T>
>> +where
>> +    T: HasHrTimer<T>,
>> +{
>> +    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 T };
>> +
>> +        // SAFETY: As we got `self_ptr` from a reference above, it must point to
>> +        // a valid `T`.
>> +        let timer_ptr = unsafe { <T as HasHrTimer<T>>::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::<T>::raw_cancel(timer_ptr) }
>> +    }
>> +}
>> +
>> +impl<'a, T> Drop for PinMutHrTimerHandle<'a, T>
>> +where
>> +    T: HasHrTimer<T>,
>> +{
>> +    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, T> UnsafeHrTimerPointer for Pin<&'a mut T>
>> +where
>> +    T: Send + Sync,
>> +    T: HasHrTimer<T>,
>> +    T: HrTimerCallback<Pointer<'a> = Self>,
>> +    Pin<&'a mut T>: RawHrTimerCallback<CallbackTarget<'a> = Self>,
>> +{
>> +    type TimerHandle = PinMutHrTimerHandle<'a, T>;
>> +
>> +    unsafe fn start(self, expires: Ktime) -> Self::TimerHandle {
>> +        // Cast to pointer
>> +        let self_ptr: *const T = <Self as core::ops::Deref>::deref(&self);
>
> You cannot go through a shared reference here, since you convert the
> pointer obtained here in the `run` function later back into a mutable
> reference. You will have to use `get_unchecked_mut` or
> `into_inner_unchecked`.

Thanks, will fix.


Best regards,
Andreas Hindborg

Re: [PATCH v10 01/13] rust: hrtimer: introduce hrtimer support

[Benno Lossin]

On Fri Mar 7, 2025 at 2:10 PM CET, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
>> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>>> +/// 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 {
>>> +
>>> +            #[inline]
>>> +            unsafe fn raw_get_timer(this: *const Self) ->
>>> +                *const $crate::time::hrtimer::HrTimer<$timer_type>
>>> +            {
>>> +                // SAFETY: The caller promises that the pointer is not dangling.
>>> +                unsafe {
>>> +                    ::core::ptr::addr_of!((*this).$field)
>>> +                }
>>> +            }
>>> +
>>> +            #[inline]
>>> +            unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_type>) ->
>>> +                *mut Self
>>
>> This formatting looks a bit weird, (macro formatting is annoying, I
>> know).
>
> How would you change it?

Just use `rustfmt` (copy the macro arm, replace all `$` with `_` in
names and remove repetitions, `rustfmt` and then revert. You're lucky,
since you only have one repetition that doesn't cause the line to go
over the 100 column threshold):

        // 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 {

            #[inline]
            unsafe fn raw_get_timer(
                this: *const Self,
            ) -> *const $crate::time::hrtimer::HrTimer<$timer_type> {
                // SAFETY: The caller promises that the pointer is not dangling.
                unsafe { ::core::ptr::addr_of!((*this).$field) }
            }

            #[inline]
            unsafe fn timer_container_of(
                ptr: *mut $crate::time::hrtimer::HrTimer<$timer_type>,
            ) -> *mut Self {
                // SAFETY: As per the safety requirement of this function, `ptr`
                // is pointing inside a `$timer_type`.
                unsafe { ::kernel::container_of!(ptr, $timer_type, $field).cast_mut() }
            }
        }

---
Cheers,
Benno

Re: [PATCH v10 08/13] rust: hrtimer: implement `UnsafeHrTimerPointer` for `Pin<&mut T>`

[Benno Lossin]

On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
> Allow pinned mutable references to structs that contain a `HrTimer` node to
> be scheduled with the `hrtimer` subsystem.
>
> Acked-by: Frederic Weisbecker <frederic@kernel.org>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/time/hrtimer.rs         |   2 +
>  rust/kernel/time/hrtimer/pin_mut.rs | 101 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 103 insertions(+)
>
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index 2ca56397eade..d2791fd624b7 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -441,3 +441,5 @@ unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_typ
>  pub use arc::ArcHrTimerHandle;
>  mod pin;
>  pub use pin::PinHrTimerHandle;
> +mod pin_mut;
> +pub use pin_mut::PinMutHrTimerHandle;
> diff --git a/rust/kernel/time/hrtimer/pin_mut.rs b/rust/kernel/time/hrtimer/pin_mut.rs
> new file mode 100644
> index 000000000000..4f4a9e9602d8
> --- /dev/null
> +++ b/rust/kernel/time/hrtimer/pin_mut.rs
> @@ -0,0 +1,101 @@
> +// 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, T>
> +where
> +    T: HasHrTimer<T>,
> +{
> +    pub(crate) inner: Pin<&'a mut T>,

I just noticed, if `T: Unpin`, this is unsound in combination with you
creating another `Pin<&mut T>` reference below for the callback, since
then we have two `&mut T` pointing to the same value. So you should
store a raw pointer instead.

---
Cheers,
Benno

Re: [PATCH v10 07/13] rust: hrtimer: implement `UnsafeHrTimerPointer` for `Pin<&T>`

[Benno Lossin]

On Fri Mar 7, 2025 at 2:37 PM CET, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
>> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>>> +impl<'a, T> RawHrTimerCallback for Pin<&'a T>
>>> +where
>>> +    T: HasHrTimer<T>,
>>> +    T: HrTimerCallback<Pointer<'a> = Self>,
>>> +{
>>> +    type CallbackTarget<'b> = Self;
>>> +
>>> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
>>> +        // `HrTimer` is `repr(C)`
>>> +        let timer_ptr = ptr as *mut HrTimer<T>;
>>> +
>>> +        // SAFETY: By the safety requirement of this function, `timer_ptr`
>>> +        // points to a `HrTimer<T>` contained in an `T`.
>>> +        let receiver_ptr = unsafe { T::timer_container_of(timer_ptr) };
>>> +
>>> +        // SAFETY: By the safety requirement of this function, `timer_ptr`
>>> +        // points to a `HrTimer<T>` contained in an `T`.
>>
>> This justification seems wrong it talks about `HrTimer<T>`, but here we
>> have a `*const T`... Also see [1] (I am mainly interested in your
>> justification for the lifetime).
>>
>> [1]: https://doc.rust-lang.org/std/ptr/index.html#pointer-to-reference-conversion
>
> How is this:
>
>         // SAFETY:
>         //  - By the safety requirement of this function, `timer_ptr`
>         //    points to a `HrTimer<T>` contained in an `T`.
>         //  - The `PinHrTimerHandle` associated with this timer is guaranteed to
>         //    be alive until this method returns. As the handle borrows from
>         //    `T`, `T` is also guaranteed to be alive for the duration of this
>         //    function.

Sounds good, if you can also explain (probably somewhere else, as every
`RawHrTimerCallback` implementer will rely on this) why the handle lives
for the duration of the callback.

---
Cheers,
Benno

Re: [PATCH v10 10/13] rust: hrtimer: implement `HrTimerPointer` for `Pin<Box<T>>`

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>> Allow `Pin<Box<T>>` to be the target of a timer callback.
>>
>> Acked-by: Frederic Weisbecker <frederic@kernel.org>
>> Reviewed-by: Lyude Paul <lyude@redhat.com>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/time/hrtimer.rs      |   3 ++
>>  rust/kernel/time/hrtimer/tbox.rs | 109 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 112 insertions(+)
>>
>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>> index d2791fd624b7..991d37b0524a 100644
>> --- a/rust/kernel/time/hrtimer.rs
>> +++ b/rust/kernel/time/hrtimer.rs
>> @@ -443,3 +443,6 @@ unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_typ
>>  pub use pin::PinHrTimerHandle;
>>  mod pin_mut;
>>  pub use pin_mut::PinMutHrTimerHandle;
>> +// `box` is a reserved keyword, so prefix with `t` for timer
>> +mod tbox;
>> +pub use tbox::BoxHrTimerHandle;
>> diff --git a/rust/kernel/time/hrtimer/tbox.rs b/rust/kernel/time/hrtimer/tbox.rs
>> new file mode 100644
>> index 000000000000..a3b2ed849050
>> --- /dev/null
>> +++ b/rust/kernel/time/hrtimer/tbox.rs
>> @@ -0,0 +1,109 @@
>> +// 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;
>> +use core::ptr::NonNull;
>> +
>> +/// A handle for a [`Box<HasHrTimer<T>>`] returned by a call to
>> +/// [`HrTimerPointer::start`].
>> +pub struct BoxHrTimerHandle<T, A>
>
> Should this type implement `Send` and `Sync` depending on `T`?

Yes. In practice `T` will always be `Send` and `Sync` because of bounds
on other traits.

I don't think we have to require `T: Sync`, because the handle does not ever
create shared references to the underlying `T`?

>
>> +where
>> +    T: HasHrTimer<T>,
>> +    A: crate::alloc::Allocator,
>> +{
>> +    pub(crate) inner: NonNull<T>,
>> +    _p: core::marker::PhantomData<A>,
>> +}
>> +
>> +// SAFETY: We implement drop below, and we cancel the timer in the drop
>> +// implementation.
>> +unsafe impl<T, A> HrTimerHandle for BoxHrTimerHandle<T, A>
>> +where
>> +    T: HasHrTimer<T>,
>> +    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 `T`.
>> +        let timer_ptr = unsafe { <T as HasHrTimer<T>>::raw_get_timer(self.inner.as_ptr()) };
>> +
>> +        // SAFETY: As `timer_ptr` points into `T` and `T` is valid, `timer_ptr`
>> +        // must point to a valid `HrTimer` instance.
>> +        unsafe { HrTimer::<T>::raw_cancel(timer_ptr) }
>> +    }
>> +}
>> +
>> +impl<T, A> Drop for BoxHrTimerHandle<T, A>
>> +where
>> +    T: HasHrTimer<T>,
>> +    A: crate::alloc::Allocator,
>> +{
>> +    fn drop(&mut self) {
>> +        self.cancel();
>> +        // SAFETY: `self.inner` came from a `Box::into_raw` call
>
> Please add this as an invariant to `Self`.

OK.

>
>> +        drop(unsafe { Box::<T, A>::from_raw(self.inner.as_ptr()) })
>> +    }
>> +}
>> +
>> +impl<T, A> HrTimerPointer for Pin<Box<T, A>>
>> +where
>> +    T: 'static,
>> +    T: Send + Sync,
>> +    T: HasHrTimer<T>,
>> +    T: for<'a> HrTimerCallback<Pointer<'a> = Pin<Box<T, A>>>,
>> +    Pin<Box<T, A>>: for<'a> RawHrTimerCallback<CallbackTarget<'a> = Pin<&'a T>>,
>
> I don't think this is necessary.

Should I remove it? I feel like it communicates intent.

>
>> +    A: crate::alloc::Allocator,
>> +{
>> +    type TimerHandle = BoxHrTimerHandle<T, A>;
>> +
>> +    fn start(self, expires: Ktime) -> Self::TimerHandle {
>> +        // SAFETY:
>> +        //  - We will not move out of this box during timer callback (we pass an
>> +        //    immutable reference to the callback).
>> +        //  - `Box::into_raw` is guaranteed to return a valid pointer.
>> +        let inner =
>> +            unsafe { NonNull::new_unchecked(Box::into_raw(Pin::into_inner_unchecked(self))) };
>> +
>> +        // SAFETY:
>> +        //  - We keep `self` alive by wrapping it in a handle below.
>> +        //  - Since we generate the pointer passed to `start` from a valid
>> +        //    reference, it is a valid pointer.
>> +        unsafe { T::start(inner.as_ptr(), expires) };
>> +
>> +        BoxHrTimerHandle {
>> +            inner,
>> +            _p: core::marker::PhantomData,
>> +        }
>> +    }
>> +}
>> +
>> +impl<T, A> RawHrTimerCallback for Pin<Box<T, A>>
>> +where
>> +    T: 'static,
>> +    T: HasHrTimer<T>,
>> +    T: for<'a> HrTimerCallback<Pointer<'a> = Pin<Box<T, A>>>,
>> +    A: crate::alloc::Allocator,
>> +{
>> +    type CallbackTarget<'a> = Pin<&'a T>;
>
> Why isn't this `Pin<&'a mut T>`?

I don't think it matters much? There can be no other mutable references
while the callback is running, so why not a shared ref?

>
>> +
>> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
>> +        // `HrTimer` is `repr(C)`
>> +        let timer_ptr = ptr.cast::<super::HrTimer<T>>();
>> +
>> +        // 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 { T::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::<T, A>::from_raw(data_ptr) }));
>
> Since you turn this into a reference below and never run the drop, why
> not turn the pointer directly into a reference?

You mean replace with `unsafe {&*data_ptr};`? I guess that could work,
but it hinges on `Box` being transparent which is more subtle than going
through the API.



Best regards,
Andreas Hindborg

Re: [PATCH v10 13/13] rust: hrtimer: add maintainer entry

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>> Add Andreas Hindborg as maintainer for Rust `hrtimer` abstractions. Also
>> add Boqun Feng as reviewer.
>>
>> Acked-by: Boqun Feng <boqun.feng@gmail.com>
>> Acked-by: Frederic Weisbecker <frederic@kernel.org>
>> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
>
> I don't recall adding my reviewed by for this patch (I normally don't
> review MAINTAINERS changes, since there isn't anything to review). And I
> don't find it in lore, how did this end up here?
>

Thanks for spotting this. It looks like I added your tag between v8 and
v9, and you are right, your tag for this patch does not appear on lore.

I had some trouble with b4 being too generous with the tags and I had to
revert to manual tagging. I think I messed this one up or did not manage
to revert the actions of b4.

I'll remove it.


Best regards,
Andreas Hindborg

Re: [PATCH v10 13/13] rust: hrtimer: add maintainer entry

[Benno Lossin]

On Fri Mar 7, 2025 at 3:10 PM CET, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
>
>> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>>> Add Andreas Hindborg as maintainer for Rust `hrtimer` abstractions. Also
>>> add Boqun Feng as reviewer.
>>>
>>> Acked-by: Boqun Feng <boqun.feng@gmail.com>
>>> Acked-by: Frederic Weisbecker <frederic@kernel.org>
>>> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
>>
>> I don't recall adding my reviewed by for this patch (I normally don't
>> review MAINTAINERS changes, since there isn't anything to review). And I
>> don't find it in lore, how did this end up here?
>>
>
> Thanks for spotting this. It looks like I added your tag between v8 and
> v9, and you are right, your tag for this patch does not appear on lore.
>
> I had some trouble with b4 being too generous with the tags and I had to
> revert to manual tagging. I think I messed this one up or did not manage
> to revert the actions of b4.

I also had that take place in the past... b4 picking non-existent tags
or tags from old versions... I always add my tags manually...

> I'll remove it.

Thanks.

---
Cheers,
Benno

Re: [PATCH v10 03/13] rust: hrtimer: implement `HrTimerPointer` for `Arc`

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On Fri Mar 7, 2025 at 2:27 PM CET, Andreas Hindborg wrote:
>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>
>>> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>>>> +impl<T> HrTimerPointer for Arc<T>
>>>> +where
>>>> +    T: 'static,
>>>> +    T: Send + Sync,
>>>> +    T: HasHrTimer<T>,
>>>> +    T: for<'a> HrTimerCallback<Pointer<'a> = Self>,
>>>> +    Arc<T>: for<'a> RawHrTimerCallback<CallbackTarget<'a> = ArcBorrow<'a, T>>,
>>>
>>> I don't understand why you need this bound here.
>>
>> This impl is applicable only when `Arc<T> has an implementation of
>> `RawTimerCallback` where CallbackTarget<'a> = ArcBorrow<'a, T>. I don't
>> want the impl to be available if that is not the case.
>
> The impl below has less strict other bounds than this one, so this bound
> doesn't change anything.
>
>> It's just an additional check.
>
> To me it's just additional noise.

I'll drop it then.

>
>>>> +{
>>>> +    type TimerHandle = ArcHrTimerHandle<T>;
>>>> +
>>>> +    fn start(self, expires: Ktime) -> ArcHrTimerHandle<T> {
>>>> +        // SAFETY:
>>>> +        //  - We keep `self` alive by wrapping it in a handle below.
>>>> +        //  - Since we generate the pointer passed to `start` from a valid
>>>> +        //    reference, it is a valid pointer.
>>>> +        unsafe { T::start(Arc::as_ptr(&self), expires) };
>>>> +        ArcHrTimerHandle { inner: self }
>>>> +    }
>>>> +}
>>>> +
>>>> +impl<T> RawHrTimerCallback for Arc<T>
>>>> +where
>>>> +    T: 'static,
>>>> +    T: HasHrTimer<T>,
>>>> +    T: for<'a> HrTimerCallback<Pointer<'a> = Self>,
>>>> +{
>>>> +    type CallbackTarget<'a> = ArcBorrow<'a, T>;
>>>> +
>>>> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
>>>> +        // `HrTimer` is `repr(C)`
>>>> +        let timer_ptr = ptr.cast::<super::HrTimer<T>>();
>>>> +
>>>> +        // 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 { T::timer_container_of(timer_ptr) };
>>>> +
>>>> +        // SAFETY: `data_ptr` points to the `T` that was used to queue the
>>>> +        // timer. This `T` is contained in an `Arc`.
>>>
>>> You're not justifying all safety requirements of `ArcBorrow::from_raw`.
>>
>> How is this:
>>
>>         // SAFETY:
>>         //  - `data_ptr` is derived form the pointer to the `T` that was used to
>>         //    queue the timer.
>>         //  - The `ArcTimerHandle` associated with this timer is guaranteed to
>>         //    be alive for the duration of the lifetime of `receiver`, so the
>
> There is no `receiver` in this context?

It's the value returned from the call, same line.

>
> Is the reason for the handle staying alive that when it is dropped, it
> calls `cancel` and that waits until the callback finishes? If so, did
> you write that down somewhere here?

Yes, it is in the safety requirement of the `HrTimerHandle` trait.
Should I add that? It becomes quite a story.

>
>>         //    refcount of the underlying `Arc` is guaranteed to be nonzero for
>>         //    the duration.
>>         //  - We own one refcount in the `ArcTimerHandle` associted with this
>>         //    timer, so it is not possible to get a `UniqueArc` to this
>>         //    allocation from other `Arc` clones.
>
> Otherwise this sounds good.

Cool.


Best regards,
Andreas Hindborg

Re: [PATCH v10 01/13] rust: hrtimer: introduce hrtimer support

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On Fri Mar 7, 2025 at 2:10 PM CET, Andreas Hindborg wrote:
>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>>>> +/// 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 {
>>>> +
>>>> +            #[inline]
>>>> +            unsafe fn raw_get_timer(this: *const Self) ->
>>>> +                *const $crate::time::hrtimer::HrTimer<$timer_type>
>>>> +            {
>>>> +                // SAFETY: The caller promises that the pointer is not dangling.
>>>> +                unsafe {
>>>> +                    ::core::ptr::addr_of!((*this).$field)
>>>> +                }
>>>> +            }
>>>> +
>>>> +            #[inline]
>>>> +            unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_type>) ->
>>>> +                *mut Self
>>>
>>> This formatting looks a bit weird, (macro formatting is annoying, I
>>> know).
>>
>> How would you change it?
>
> Just use `rustfmt` (copy the macro arm, replace all `$` with `_` in
> names and remove repetitions, `rustfmt` and then revert. You're lucky,
> since you only have one repetition that doesn't cause the line to go
> over the 100 column threshold):
>
>         // 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 {
>
>             #[inline]
>             unsafe fn raw_get_timer(
>                 this: *const Self,
>             ) -> *const $crate::time::hrtimer::HrTimer<$timer_type> {
>                 // SAFETY: The caller promises that the pointer is not dangling.
>                 unsafe { ::core::ptr::addr_of!((*this).$field) }
>             }
>
>             #[inline]
>             unsafe fn timer_container_of(
>                 ptr: *mut $crate::time::hrtimer::HrTimer<$timer_type>,
>             ) -> *mut Self {
>                 // SAFETY: As per the safety requirement of this function, `ptr`
>                 // is pointing inside a `$timer_type`.
>                 unsafe { ::kernel::container_of!(ptr, $timer_type, $field).cast_mut() }
>             }
>         }

That is better.


Best regards,
Andreas Hindborg

Re: [PATCH v10 08/13] rust: hrtimer: implement `UnsafeHrTimerPointer` for `Pin<&mut T>`

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>> Allow pinned mutable references to structs that contain a `HrTimer` node to
>> be scheduled with the `hrtimer` subsystem.
>>
>> Acked-by: Frederic Weisbecker <frederic@kernel.org>
>> Reviewed-by: Lyude Paul <lyude@redhat.com>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/time/hrtimer.rs         |   2 +
>>  rust/kernel/time/hrtimer/pin_mut.rs | 101 ++++++++++++++++++++++++++++++++++++
>>  2 files changed, 103 insertions(+)
>>
>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>> index 2ca56397eade..d2791fd624b7 100644
>> --- a/rust/kernel/time/hrtimer.rs
>> +++ b/rust/kernel/time/hrtimer.rs
>> @@ -441,3 +441,5 @@ unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_typ
>>  pub use arc::ArcHrTimerHandle;
>>  mod pin;
>>  pub use pin::PinHrTimerHandle;
>> +mod pin_mut;
>> +pub use pin_mut::PinMutHrTimerHandle;
>> diff --git a/rust/kernel/time/hrtimer/pin_mut.rs b/rust/kernel/time/hrtimer/pin_mut.rs
>> new file mode 100644
>> index 000000000000..4f4a9e9602d8
>> --- /dev/null
>> +++ b/rust/kernel/time/hrtimer/pin_mut.rs
>> @@ -0,0 +1,101 @@
>> +// 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, T>
>> +where
>> +    T: HasHrTimer<T>,
>> +{
>> +    pub(crate) inner: Pin<&'a mut T>,
>
> I just noticed, if `T: Unpin`, this is unsound in combination with you
> creating another `Pin<&mut T>` reference below for the callback, since
> then we have two `&mut T` pointing to the same value. So you should
> store a raw pointer instead.

Will fix.


Best regards,
Andreas Hindborg

Re: [PATCH v10 07/13] rust: hrtimer: implement `UnsafeHrTimerPointer` for `Pin<&T>`

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On Fri Mar 7, 2025 at 2:37 PM CET, Andreas Hindborg wrote:
>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>>>> +impl<'a, T> RawHrTimerCallback for Pin<&'a T>
>>>> +where
>>>> +    T: HasHrTimer<T>,
>>>> +    T: HrTimerCallback<Pointer<'a> = Self>,
>>>> +{
>>>> +    type CallbackTarget<'b> = Self;
>>>> +
>>>> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
>>>> +        // `HrTimer` is `repr(C)`
>>>> +        let timer_ptr = ptr as *mut HrTimer<T>;
>>>> +
>>>> +        // SAFETY: By the safety requirement of this function, `timer_ptr`
>>>> +        // points to a `HrTimer<T>` contained in an `T`.
>>>> +        let receiver_ptr = unsafe { T::timer_container_of(timer_ptr) };
>>>> +
>>>> +        // SAFETY: By the safety requirement of this function, `timer_ptr`
>>>> +        // points to a `HrTimer<T>` contained in an `T`.
>>>
>>> This justification seems wrong it talks about `HrTimer<T>`, but here we
>>> have a `*const T`... Also see [1] (I am mainly interested in your
>>> justification for the lifetime).
>>>
>>> [1]: https://doc.rust-lang.org/std/ptr/index.html#pointer-to-reference-conversion
>>
>> How is this:
>>
>>         // SAFETY:
>>         //  - By the safety requirement of this function, `timer_ptr`
>>         //    points to a `HrTimer<T>` contained in an `T`.
>>         //  - The `PinHrTimerHandle` associated with this timer is guaranteed to
>>         //    be alive until this method returns. As the handle borrows from
>>         //    `T`, `T` is also guaranteed to be alive for the duration of this
>>         //    function.
>
> Sounds good, if you can also explain (probably somewhere else, as every
> `RawHrTimerCallback` implementer will rely on this) why the handle lives
> for the duration of the callback.

It is in the safety requirement for the `HrTimerHandle` trait already.
Should I reference it here?


Best regards,
Andreas Hindborg

Re: [PATCH v10 03/13] rust: hrtimer: implement `HrTimerPointer` for `Arc`

[Benno Lossin]

On Fri Mar 7, 2025 at 3:16 PM CET, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
>> On Fri Mar 7, 2025 at 2:27 PM CET, Andreas Hindborg wrote:
>>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>>> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>>>>> +impl<T> RawHrTimerCallback for Arc<T>
>>>>> +where
>>>>> +    T: 'static,
>>>>> +    T: HasHrTimer<T>,
>>>>> +    T: for<'a> HrTimerCallback<Pointer<'a> = Self>,
>>>>> +{
>>>>> +    type CallbackTarget<'a> = ArcBorrow<'a, T>;
>>>>> +
>>>>> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
>>>>> +        // `HrTimer` is `repr(C)`
>>>>> +        let timer_ptr = ptr.cast::<super::HrTimer<T>>();
>>>>> +
>>>>> +        // 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 { T::timer_container_of(timer_ptr) };
>>>>> +
>>>>> +        // SAFETY: `data_ptr` points to the `T` that was used to queue the
>>>>> +        // timer. This `T` is contained in an `Arc`.
>>>>
>>>> You're not justifying all safety requirements of `ArcBorrow::from_raw`.
>>>
>>> How is this:
>>>
>>>         // SAFETY:
>>>         //  - `data_ptr` is derived form the pointer to the `T` that was used to
>>>         //    queue the timer.
>>>         //  - The `ArcTimerHandle` associated with this timer is guaranteed to
>>>         //    be alive for the duration of the lifetime of `receiver`, so the
>>
>> There is no `receiver` in this context?
>
> It's the value returned from the call, same line.

Ah my bad.

>
>> Is the reason for the handle staying alive that when it is dropped, it
>> calls `cancel` and that waits until the callback finishes? If so, did
>> you write that down somewhere here?
>
> Yes, it is in the safety requirement of the `HrTimerHandle` trait.
> Should I add that? It becomes quite a story.

Yeah, I think you should add it.

---
Cheers,
Benno

>>>         //    refcount of the underlying `Arc` is guaranteed to be nonzero for
>>>         //    the duration.
>>>         //  - We own one refcount in the `ArcTimerHandle` associted with this
>>>         //    timer, so it is not possible to get a `UniqueArc` to this
>>>         //    allocation from other `Arc` clones.
>>
>> Otherwise this sounds good.
>
> Cool.
>
>
> Best regards,
> Andreas Hindborg

Re: [PATCH v10 07/13] rust: hrtimer: implement `UnsafeHrTimerPointer` for `Pin<&T>`

[Benno Lossin]

On Fri Mar 7, 2025 at 3:21 PM CET, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
>
>> On Fri Mar 7, 2025 at 2:37 PM CET, Andreas Hindborg wrote:
>>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>>> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>>>>> +impl<'a, T> RawHrTimerCallback for Pin<&'a T>
>>>>> +where
>>>>> +    T: HasHrTimer<T>,
>>>>> +    T: HrTimerCallback<Pointer<'a> = Self>,
>>>>> +{
>>>>> +    type CallbackTarget<'b> = Self;
>>>>> +
>>>>> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
>>>>> +        // `HrTimer` is `repr(C)`
>>>>> +        let timer_ptr = ptr as *mut HrTimer<T>;
>>>>> +
>>>>> +        // SAFETY: By the safety requirement of this function, `timer_ptr`
>>>>> +        // points to a `HrTimer<T>` contained in an `T`.
>>>>> +        let receiver_ptr = unsafe { T::timer_container_of(timer_ptr) };
>>>>> +
>>>>> +        // SAFETY: By the safety requirement of this function, `timer_ptr`
>>>>> +        // points to a `HrTimer<T>` contained in an `T`.
>>>>
>>>> This justification seems wrong it talks about `HrTimer<T>`, but here we
>>>> have a `*const T`... Also see [1] (I am mainly interested in your
>>>> justification for the lifetime).
>>>>
>>>> [1]: https://doc.rust-lang.org/std/ptr/index.html#pointer-to-reference-conversion
>>>
>>> How is this:
>>>
>>>         // SAFETY:
>>>         //  - By the safety requirement of this function, `timer_ptr`
>>>         //    points to a `HrTimer<T>` contained in an `T`.
>>>         //  - The `PinHrTimerHandle` associated with this timer is guaranteed to
>>>         //    be alive until this method returns. As the handle borrows from
>>>         //    `T`, `T` is also guaranteed to be alive for the duration of this
>>>         //    function.
>>
>> Sounds good, if you can also explain (probably somewhere else, as every
>> `RawHrTimerCallback` implementer will rely on this) why the handle lives
>> for the duration of the callback.
>
> It is in the safety requirement for the `HrTimerHandle` trait already.
> Should I reference it here?

Yes please!

---
Cheers,
Benno

Re: [PATCH v10 10/13] rust: hrtimer: implement `HrTimerPointer` for `Pin<Box<T>>`

[Benno Lossin]

On Fri Mar 7, 2025 at 3:01 PM CET, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
>
>> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>>> Allow `Pin<Box<T>>` to be the target of a timer callback.
>>>
>>> Acked-by: Frederic Weisbecker <frederic@kernel.org>
>>> Reviewed-by: Lyude Paul <lyude@redhat.com>
>>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>> ---
>>>  rust/kernel/time/hrtimer.rs      |   3 ++
>>>  rust/kernel/time/hrtimer/tbox.rs | 109 +++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 112 insertions(+)
>>>
>>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>>> index d2791fd624b7..991d37b0524a 100644
>>> --- a/rust/kernel/time/hrtimer.rs
>>> +++ b/rust/kernel/time/hrtimer.rs
>>> @@ -443,3 +443,6 @@ unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_typ
>>>  pub use pin::PinHrTimerHandle;
>>>  mod pin_mut;
>>>  pub use pin_mut::PinMutHrTimerHandle;
>>> +// `box` is a reserved keyword, so prefix with `t` for timer
>>> +mod tbox;
>>> +pub use tbox::BoxHrTimerHandle;
>>> diff --git a/rust/kernel/time/hrtimer/tbox.rs b/rust/kernel/time/hrtimer/tbox.rs
>>> new file mode 100644
>>> index 000000000000..a3b2ed849050
>>> --- /dev/null
>>> +++ b/rust/kernel/time/hrtimer/tbox.rs
>>> @@ -0,0 +1,109 @@
>>> +// 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;
>>> +use core::ptr::NonNull;
>>> +
>>> +/// A handle for a [`Box<HasHrTimer<T>>`] returned by a call to
>>> +/// [`HrTimerPointer::start`].
>>> +pub struct BoxHrTimerHandle<T, A>
>>
>> Should this type implement `Send` and `Sync` depending on `T`?
>
> Yes. In practice `T` will always be `Send` and `Sync` because of bounds
> on other traits.
>
> I don't think we have to require `T: Sync`, because the handle does not ever
> create shared references to the underlying `T`?

Oh I meant to do:

    unsafe impl<T: Send + Sync, A> Send for BoxHrTimerHandle<T, A> {}

But since you don't have it, it might be unnecessary.

>>> +where
>>> +    T: HasHrTimer<T>,
>>> +    A: crate::alloc::Allocator,
>>> +{
>>> +    pub(crate) inner: NonNull<T>,
>>> +    _p: core::marker::PhantomData<A>,
>>> +}
>>> +
>>> +// SAFETY: We implement drop below, and we cancel the timer in the drop
>>> +// implementation.
>>> +unsafe impl<T, A> HrTimerHandle for BoxHrTimerHandle<T, A>
>>> +where
>>> +    T: HasHrTimer<T>,
>>> +    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 `T`.
>>> +        let timer_ptr = unsafe { <T as HasHrTimer<T>>::raw_get_timer(self.inner.as_ptr()) };
>>> +
>>> +        // SAFETY: As `timer_ptr` points into `T` and `T` is valid, `timer_ptr`
>>> +        // must point to a valid `HrTimer` instance.
>>> +        unsafe { HrTimer::<T>::raw_cancel(timer_ptr) }
>>> +    }
>>> +}
>>> +
>>> +impl<T, A> Drop for BoxHrTimerHandle<T, A>
>>> +where
>>> +    T: HasHrTimer<T>,
>>> +    A: crate::alloc::Allocator,
>>> +{
>>> +    fn drop(&mut self) {
>>> +        self.cancel();
>>> +        // SAFETY: `self.inner` came from a `Box::into_raw` call
>>
>> Please add this as an invariant to `Self`.
>
> OK.
>
>>
>>> +        drop(unsafe { Box::<T, A>::from_raw(self.inner.as_ptr()) })
>>> +    }
>>> +}
>>> +
>>> +impl<T, A> HrTimerPointer for Pin<Box<T, A>>
>>> +where
>>> +    T: 'static,
>>> +    T: Send + Sync,
>>> +    T: HasHrTimer<T>,
>>> +    T: for<'a> HrTimerCallback<Pointer<'a> = Pin<Box<T, A>>>,
>>> +    Pin<Box<T, A>>: for<'a> RawHrTimerCallback<CallbackTarget<'a> = Pin<&'a T>>,
>>
>> I don't think this is necessary.
>
> Should I remove it? I feel like it communicates intent.

What intent?

>>> +    A: crate::alloc::Allocator,
>>> +{
>>> +    type TimerHandle = BoxHrTimerHandle<T, A>;
>>> +
>>> +    fn start(self, expires: Ktime) -> Self::TimerHandle {
>>> +        // SAFETY:
>>> +        //  - We will not move out of this box during timer callback (we pass an
>>> +        //    immutable reference to the callback).
>>> +        //  - `Box::into_raw` is guaranteed to return a valid pointer.
>>> +        let inner =
>>> +            unsafe { NonNull::new_unchecked(Box::into_raw(Pin::into_inner_unchecked(self))) };
>>> +
>>> +        // SAFETY:
>>> +        //  - We keep `self` alive by wrapping it in a handle below.
>>> +        //  - Since we generate the pointer passed to `start` from a valid
>>> +        //    reference, it is a valid pointer.
>>> +        unsafe { T::start(inner.as_ptr(), expires) };
>>> +
>>> +        BoxHrTimerHandle {
>>> +            inner,
>>> +            _p: core::marker::PhantomData,
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +impl<T, A> RawHrTimerCallback for Pin<Box<T, A>>
>>> +where
>>> +    T: 'static,
>>> +    T: HasHrTimer<T>,
>>> +    T: for<'a> HrTimerCallback<Pointer<'a> = Pin<Box<T, A>>>,
>>> +    A: crate::alloc::Allocator,
>>> +{
>>> +    type CallbackTarget<'a> = Pin<&'a T>;
>>
>> Why isn't this `Pin<&'a mut T>`?
>
> I don't think it matters much? There can be no other mutable references
> while the callback is running, so why not a shared ref?

IIUC there can be no references to the value, since the user used a
`Pin<Box<T>>` to schedule the timer.

I thought it might make sense to give a pinned mutable reference, since
you explicitly implement the `RawHrTimerCallback` for `Pin<&mut T>`.
Which made me believe one sometimes needs to modify the `T` from the
callback.

Since we're able to do that when the user used a `Box`, I think we
should just do it.

>>> +
>>> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
>>> +        // `HrTimer` is `repr(C)`
>>> +        let timer_ptr = ptr.cast::<super::HrTimer<T>>();
>>> +
>>> +        // 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 { T::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::<T, A>::from_raw(data_ptr) }));
>>
>> Since you turn this into a reference below and never run the drop, why
>> not turn the pointer directly into a reference?
>
> You mean replace with `unsafe {&*data_ptr};`? I guess that could work,
> but it hinges on `Box` being transparent which is more subtle than going
> through the API.

I think that's cleaner. Also why does that rely on `Box` being
transparent?

---
Cheers,
Benno

Re: [PATCH v10 10/13] rust: hrtimer: implement `HrTimerPointer` for `Pin<Box<T>>`

[Andreas Hindborg]

"Benno Lossin" <benno.lossin@proton.me> writes:

> On Fri Mar 7, 2025 at 3:01 PM CET, Andreas Hindborg wrote:
>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>
>>> On Fri Mar 7, 2025 at 11:11 AM CET, Andreas Hindborg wrote:
>>>> Allow `Pin<Box<T>>` to be the target of a timer callback.
>>>>
>>>> Acked-by: Frederic Weisbecker <frederic@kernel.org>
>>>> Reviewed-by: Lyude Paul <lyude@redhat.com>
>>>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>>> ---
>>>>  rust/kernel/time/hrtimer.rs      |   3 ++
>>>>  rust/kernel/time/hrtimer/tbox.rs | 109 +++++++++++++++++++++++++++++++++++++++
>>>>  2 files changed, 112 insertions(+)
>>>>
>>>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>>>> index d2791fd624b7..991d37b0524a 100644
>>>> --- a/rust/kernel/time/hrtimer.rs
>>>> +++ b/rust/kernel/time/hrtimer.rs
>>>> @@ -443,3 +443,6 @@ unsafe fn timer_container_of(ptr: *mut $crate::time::hrtimer::HrTimer<$timer_typ
>>>>  pub use pin::PinHrTimerHandle;
>>>>  mod pin_mut;
>>>>  pub use pin_mut::PinMutHrTimerHandle;
>>>> +// `box` is a reserved keyword, so prefix with `t` for timer
>>>> +mod tbox;
>>>> +pub use tbox::BoxHrTimerHandle;
>>>> diff --git a/rust/kernel/time/hrtimer/tbox.rs b/rust/kernel/time/hrtimer/tbox.rs
>>>> new file mode 100644
>>>> index 000000000000..a3b2ed849050
>>>> --- /dev/null
>>>> +++ b/rust/kernel/time/hrtimer/tbox.rs
>>>> @@ -0,0 +1,109 @@
>>>> +// 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;
>>>> +use core::ptr::NonNull;
>>>> +
>>>> +/// A handle for a [`Box<HasHrTimer<T>>`] returned by a call to
>>>> +/// [`HrTimerPointer::start`].
>>>> +pub struct BoxHrTimerHandle<T, A>
>>>
>>> Should this type implement `Send` and `Sync` depending on `T`?
>>
>> Yes. In practice `T` will always be `Send` and `Sync` because of bounds
>> on other traits.
>>
>> I don't think we have to require `T: Sync`, because the handle does not ever
>> create shared references to the underlying `T`?
>
> Oh I meant to do:
>
>     unsafe impl<T: Send + Sync, A> Send for BoxHrTimerHandle<T, A> {}
>
> But since you don't have it, it might be unnecessary.
>
>>>> +where
>>>> +    T: HasHrTimer<T>,
>>>> +    A: crate::alloc::Allocator,
>>>> +{
>>>> +    pub(crate) inner: NonNull<T>,
>>>> +    _p: core::marker::PhantomData<A>,
>>>> +}
>>>> +
>>>> +// SAFETY: We implement drop below, and we cancel the timer in the drop
>>>> +// implementation.
>>>> +unsafe impl<T, A> HrTimerHandle for BoxHrTimerHandle<T, A>
>>>> +where
>>>> +    T: HasHrTimer<T>,
>>>> +    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 `T`.
>>>> +        let timer_ptr = unsafe { <T as HasHrTimer<T>>::raw_get_timer(self.inner.as_ptr()) };
>>>> +
>>>> +        // SAFETY: As `timer_ptr` points into `T` and `T` is valid, `timer_ptr`
>>>> +        // must point to a valid `HrTimer` instance.
>>>> +        unsafe { HrTimer::<T>::raw_cancel(timer_ptr) }
>>>> +    }
>>>> +}
>>>> +
>>>> +impl<T, A> Drop for BoxHrTimerHandle<T, A>
>>>> +where
>>>> +    T: HasHrTimer<T>,
>>>> +    A: crate::alloc::Allocator,
>>>> +{
>>>> +    fn drop(&mut self) {
>>>> +        self.cancel();
>>>> +        // SAFETY: `self.inner` came from a `Box::into_raw` call
>>>
>>> Please add this as an invariant to `Self`.
>>
>> OK.
>>
>>>
>>>> +        drop(unsafe { Box::<T, A>::from_raw(self.inner.as_ptr()) })
>>>> +    }
>>>> +}
>>>> +
>>>> +impl<T, A> HrTimerPointer for Pin<Box<T, A>>
>>>> +where
>>>> +    T: 'static,
>>>> +    T: Send + Sync,
>>>> +    T: HasHrTimer<T>,
>>>> +    T: for<'a> HrTimerCallback<Pointer<'a> = Pin<Box<T, A>>>,
>>>> +    Pin<Box<T, A>>: for<'a> RawHrTimerCallback<CallbackTarget<'a> = Pin<&'a T>>,
>>>
>>> I don't think this is necessary.
>>
>> Should I remove it? I feel like it communicates intent.
>
> What intent?
>
>>>> +    A: crate::alloc::Allocator,
>>>> +{
>>>> +    type TimerHandle = BoxHrTimerHandle<T, A>;
>>>> +
>>>> +    fn start(self, expires: Ktime) -> Self::TimerHandle {
>>>> +        // SAFETY:
>>>> +        //  - We will not move out of this box during timer callback (we pass an
>>>> +        //    immutable reference to the callback).
>>>> +        //  - `Box::into_raw` is guaranteed to return a valid pointer.
>>>> +        let inner =
>>>> +            unsafe { NonNull::new_unchecked(Box::into_raw(Pin::into_inner_unchecked(self))) };
>>>> +
>>>> +        // SAFETY:
>>>> +        //  - We keep `self` alive by wrapping it in a handle below.
>>>> +        //  - Since we generate the pointer passed to `start` from a valid
>>>> +        //    reference, it is a valid pointer.
>>>> +        unsafe { T::start(inner.as_ptr(), expires) };
>>>> +
>>>> +        BoxHrTimerHandle {
>>>> +            inner,
>>>> +            _p: core::marker::PhantomData,
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>> +impl<T, A> RawHrTimerCallback for Pin<Box<T, A>>
>>>> +where
>>>> +    T: 'static,
>>>> +    T: HasHrTimer<T>,
>>>> +    T: for<'a> HrTimerCallback<Pointer<'a> = Pin<Box<T, A>>>,
>>>> +    A: crate::alloc::Allocator,
>>>> +{
>>>> +    type CallbackTarget<'a> = Pin<&'a T>;
>>>
>>> Why isn't this `Pin<&'a mut T>`?
>>
>> I don't think it matters much? There can be no other mutable references
>> while the callback is running, so why not a shared ref?
>
> IIUC there can be no references to the value, since the user used a
> `Pin<Box<T>>` to schedule the timer.
>
> I thought it might make sense to give a pinned mutable reference, since
> you explicitly implement the `RawHrTimerCallback` for `Pin<&mut T>`.
> Which made me believe one sometimes needs to modify the `T` from the
> callback.
>
> Since we're able to do that when the user used a `Box`, I think we
> should just do it.

I see your point, I will change it.

>
>>>> +
>>>> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
>>>> +        // `HrTimer` is `repr(C)`
>>>> +        let timer_ptr = ptr.cast::<super::HrTimer<T>>();
>>>> +
>>>> +        // 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 { T::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::<T, A>::from_raw(data_ptr) }));
>>>
>>> Since you turn this into a reference below and never run the drop, why
>>> not turn the pointer directly into a reference?
>>
>> You mean replace with `unsafe {&*data_ptr};`? I guess that could work,
>> but it hinges on `Box` being transparent which is more subtle than going
>> through the API.
>
> I think that's cleaner. Also why does that rely on `Box` being
> transparent?

I did not think of `Box::into_raw` as returning a pointer to the
underlying `T`, but looking at the signature, it does. I thought it was
returning a pointer to a `Box`.

I will change it as you suggest.


Best regards,
Andreas Hindborg