[PATCH v8 00/14] hrtimer Rust API

[PATCH v8 00/14] hrtimer Rust API

[Andreas Hindborg]

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

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

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

Link: https://git.kernel.org/pub/scm/linux/kernel/git/a.hindborg/linux.git/log/?h=rnull-v6.11-rc2 [1]
Link: https://gitlab.freedesktop.org/lyudess/linux/-/tree/rvkms-wip [2]
Link: https://lore.kernel.org/rust-for-linux/20240612223025.1158537-1-boqun.feng@gmail.com/ [3]
Link: https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@redhat.com/ [4]
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
Changes in 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 `ClockSource`
      rust: hrtimer: add maintainer entry

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

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

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

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

[Andreas Hindborg]

From: Lyude Paul <lyude@redhat.com>

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

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

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

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

-- 
2.47.0

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

[Andreas Hindborg]

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

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

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

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 87e47f2f5618d..2cf365cfb412e 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -10,6 +10,8 @@
 
 use core::convert::Into;
 
+pub mod hrtimer;
+
 /// The number of nanoseconds per millisecond.
 pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
 
diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
new file mode 100644
index 0000000000000..a6332924efabd
--- /dev/null
+++ b/rust/kernel/time/hrtimer.rs
@@ -0,0 +1,312 @@
+// 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
+//! * Running
+//!
+//! Operations:
+//!
+//! * Start
+//! * Cancel
+//! * Stop
+//! * Restart
+//!
+//! Events:
+//!
+//! * Expire
+//!
+//! ## State Diagram
+//!
+//! ```text
+//!                  <-- Stop ----
+//!                  <-- Cancel --
+//!                  --- Start -->
+//!        +---------+        +---------+
+//!   O--->| Stopped |        | Running |---o
+//!        +---------+        +---------+   |
+//!                                  ^      |
+//!                  <- Expire --    |      |
+//!                                  o------o
+//!                                   Restart
+//! ```
+//!
+//! A timer is initialized in the **stopped** state. A stopped timer can be
+//! **started** with an **expiry** time. After the timer is started, it is
+//! **running**. When the timer **expires**, the timer handler is executed.
+//! After the handler has executed, the timer may be **restarted** or
+//! **stopped**. A running timer can be **canceled** before it's handler is
+//! executed. A timer that is cancelled enters the **stopped** state.
+//!
+
+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 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 {
+            // INVARIANTS: We initialize `timer` with `hrtimer_setup` below.
+            timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
+                // SAFETY: By design of `pin_init!`, `place` is a pointer 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::CallbackTarget::run),
+                        bindings::CLOCK_MONOTONIC as i32,
+                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
+                    );
+                }
+            }),
+            _t: PhantomData,
+        })
+    }
+
+    /// Get a pointer to the contained `bindings::hrtimer`.
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must point to a live allocation of at least the size of `Self`.
+    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
+        // SAFETY: The field projection to `timer` does not go out of bounds,
+        // because the caller of this function promises that `ptr` points to an
+        // allocation of at least the size of `Self`.
+        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
+    }
+
+    /// Cancel an initialized and potentially running timer.
+    ///
+    /// If the timer handler is running, this will block until the handler is
+    /// finished.
+    ///
+    /// Users of the `HrTimer` API would not usually call this method directly.
+    /// Instead they would use the safe `cancel` method on the [`HrTimerHandle`]
+    /// returned when the timer was started.
+    ///
+    /// # Safety
+    ///
+    /// `self_ptr` must point to a valid `Self`.
+    #[allow(dead_code)]
+    pub(crate) unsafe fn raw_cancel(self_ptr: *const Self) -> bool {
+        // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
+        let c_timer_ptr = unsafe { HrTimer::raw_get(self_ptr) };
+
+        // If the handler is running, this will wait for the handler to finish
+        // before returning.
+        // SAFETY: `c_timer_ptr` is initialized and valid. Synchronization is
+        // handled on C side.
+        unsafe { bindings::hrtimer_cancel(c_timer_ptr) != 0 }
+    }
+}
+
+/// Implemented by pointer types that point to structs that embed a [`HrTimer`].
+///
+/// Target (pointee) must be [`Sync`] because timer callbacks happen in another
+/// thread of execution (hard or soft interrupt context).
+///
+/// Starting a timer returns a [`HrTimerHandle`] that can be used to manipulate
+/// the timer. Note that it is OK to call the start function repeatedly, and
+/// that more than one [`HrTimerHandle`] associated with a [`HrTimerPointer`] may
+/// exist. A timer can be manipulated through any of the handles, and a handle
+/// may represent a cancelled timer.
+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 {
+    /// Callback to be called from C when timer fires.
+    ///
+    /// # Safety
+    ///
+    /// Only to be called by C code in `hrtimer` subsystem. `ptr` must point to
+    /// the `bindings::hrtimer` structure that was used to start the timer.
+    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
+}
+
+/// Implemented by structs that can be the target of a timer callback.
+pub trait HrTimerCallback {
+    /// The type whose [`RawHrTimerCallback::run`] method will be invoked when
+    /// the timer expires.
+    type CallbackTarget<'a>: RawHrTimerCallback;
+
+    /// This type is passed to the timer callback function. It may be a borrow
+    /// of [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
+    /// implementation can guarantee exclusive access to the target during timer
+    /// handler execution.
+    type CallbackTargetParameter<'a>;
+
+    /// Called by the timer logic when the timer fires.
+    fn run(this: Self::CallbackTargetParameter<'_>)
+    where
+        Self: Sized;
+}
+
+/// A handle representing a potentially running timer.
+///
+/// More than one handle representing the same timer might exist.
+///
+/// # Safety
+///
+/// When dropped, the timer represented by this handle must be cancelled, if it
+/// is running. If the timer handler is running when the handle is dropped, the
+/// drop method must wait for the handler to finish before returning.
+pub unsafe trait HrTimerHandle {
+    /// Cancel the timer, if it is running. If the timer handler is running, block
+    /// till the handler has finished.
+    fn cancel(&mut self) -> bool;
+}
+
+/// Implemented by structs that contain timer nodes.
+///
+/// Clients of the timer API would usually safely implement this trait by using
+/// the [`crate::impl_has_hr_timer`] macro.
+///
+/// # Safety
+///
+/// Implementers of this trait must ensure that the implementer has a [`HrTimer`]
+/// field at the offset specified by `OFFSET` and that all trait methods are
+/// implemented according to their documentation.
+///
+/// [`impl_has_timer`]: crate::impl_has_timer
+pub unsafe trait HasHrTimer<T> {
+    /// Offset of the [`HrTimer`] field within `Self`
+    const OFFSET: usize;
+
+    /// Return a pointer to the [`HrTimer`] within `Self`.
+    ///
+    /// # Safety
+    ///
+    /// `ptr` must point to a valid struct of type `Self`.
+    unsafe fn raw_get_timer(ptr: *const Self) -> *const HrTimer<T> {
+        // SAFETY: By the safety requirement of this trait, the trait
+        // implementor will have a `HrTimer` field at the specified offset.
+        unsafe { ptr.cast::<u8>().add(Self::OFFSET).cast::<HrTimer<T>>() }
+    }
+
+    /// Return a pointer to the struct that is embedding the [`HrTimer`] pointed
+    /// to by `ptr`.
+    ///
+    /// # 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,
+    {
+        // SAFETY: By the safety requirement of this function and the `HasHrTimer`
+        // trait, the following expression will yield a pointer to the `Self`
+        // containing the timer addressed by `ptr`.
+        unsafe { ptr.cast::<u8>().sub(Self::OFFSET).cast::<Self>() }
+    }
+
+    /// Get pointer to embedded `bindings::hrtimer` struct.
+    ///
+    /// # Safety
+    ///
+    /// `self_ptr` must point to a valid `Self`.
+    unsafe fn c_timer_ptr(self_ptr: *const Self) -> *const bindings::hrtimer {
+        // SAFETY: `self_ptr` is a valid pointer to a `Self`.
+        let timer_ptr = unsafe { Self::raw_get_timer(self_ptr) };
+
+        // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
+        unsafe { HrTimer::raw_get(timer_ptr) }
+    }
+
+    /// Start the timer contained in the `Self` pointed to by `self_ptr`. If
+    /// it is already running it is removed and inserted.
+    ///
+    /// # Safety
+    ///
+    /// `self_ptr` must point to a valid `Self`.
+    unsafe fn start(self_ptr: *const Self, expires: Ktime) {
+        // SAFETY: By function safety requirement, `self_ptr`is a valid `Self`.
+        unsafe {
+            bindings::hrtimer_start_range_ns(
+                Self::c_timer_ptr(self_ptr).cast_mut(),
+                expires.to_ns(),
+                0,
+                bindings::hrtimer_mode_HRTIMER_MODE_REL,
+            );
+        }
+    }
+}
+
+/// Use to implement the [`HasHrTimer<T>`] trait.
+///
+/// See [`module`] documentation for an example.
+///
+/// [`module`]: crate::time::hrtimer
+#[macro_export]
+macro_rules! impl_has_hr_timer {
+    (
+        impl$({$($generics:tt)*})?
+            HasHrTimer<$timer_type:ty>
+            for $self:ty
+        { self.$field:ident }
+        $($rest:tt)*
+    ) => {
+        // SAFETY: This implementation of `raw_get_timer` only compiles if the
+        // field has the right type.
+        unsafe impl$(<$($generics)*>)? $crate::time::hrtimer::HasHrTimer<$timer_type> for $self {
+            const OFFSET: usize = ::core::mem::offset_of!(Self, $field) as usize;
+
+            #[inline]
+            unsafe fn raw_get_timer(ptr: *const Self) ->
+                *const $crate::time::hrtimer::HrTimer<$timer_type>
+            {
+                // SAFETY: The caller promises that the pointer is not dangling.
+                unsafe {
+                    ::core::ptr::addr_of!((*ptr).$field)
+                }
+            }
+        }
+    }
+}

-- 
2.47.0

[PATCH v8 03/14] 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>
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 3cefda7a43725..1dfa75714f9d6 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 v8 04/14] rust: hrtimer: implement `HrTimerPointer` for `Arc`

[Andreas Hindborg]

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

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

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index a6332924efabd..b2a3130bc21d7 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -119,7 +119,6 @@ unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
     /// # Safety
     ///
     /// `self_ptr` must point to a valid `Self`.
-    #[allow(dead_code)]
     pub(crate) unsafe fn raw_cancel(self_ptr: *const Self) -> bool {
         // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
         let c_timer_ptr = unsafe { HrTimer::raw_get(self_ptr) };
@@ -310,3 +309,6 @@ unsafe fn raw_get_timer(ptr: *const Self) ->
         }
     }
 }
+
+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 0000000000000..d1c90631d0036
--- /dev/null
+++ b/rust/kernel/time/hrtimer/arc.rs
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::HasHrTimer;
+use super::HrTimer;
+use super::HrTimerCallback;
+use super::HrTimerHandle;
+use super::HrTimerPointer;
+use super::RawHrTimerCallback;
+use crate::sync::Arc;
+use crate::sync::ArcBorrow;
+use crate::time::Ktime;
+
+/// A handle for an `Arc<HasHrTimer<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: Send + Sync,
+    T: HasHrTimer<T>,
+    T: for<'a> HrTimerCallback<CallbackTarget<'a> = Self>,
+{
+    type TimerHandle = ArcHrTimerHandle<T>;
+
+    fn start(self, expires: Ktime) -> ArcHrTimerHandle<T> {
+        // SAFETY: 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: HasHrTimer<T>,
+    T: for<'a> HrTimerCallback<CallbackTarget<'a> = Self>,
+    T: for<'a> HrTimerCallback<CallbackTargetParameter<'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 v8 05/14] rust: hrtimer: allow timer restart from timer handler

[Andreas Hindborg]

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

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

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

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index b2a3130bc21d7..e342193f985eb 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -183,7 +183,7 @@ pub trait HrTimerCallback {
     type CallbackTargetParameter<'a>;
 
     /// Called by the timer logic when the timer fires.
-    fn run(this: Self::CallbackTargetParameter<'_>)
+    fn run(this: Self::CallbackTargetParameter<'_>) -> HrTimerRestart
     where
         Self: Sized;
 }
@@ -278,6 +278,32 @@ unsafe fn start(self_ptr: *const Self, expires: Ktime) {
     }
 }
 
+/// Restart policy for timers.
+pub enum HrTimerRestart {
+    /// Timer should not be restarted.
+    NoRestart,
+    /// Timer should be restarted.
+    Restart,
+}
+
+impl From<bindings::hrtimer_restart> for HrTimerRestart {
+    fn from(value: u32) -> Self {
+        match value {
+            bindings::hrtimer_restart_HRTIMER_NORESTART => Self::NoRestart,
+            _ => Self::Restart,
+        }
+    }
+}
+
+impl From<HrTimerRestart> for bindings::hrtimer_restart {
+    fn from(value: HrTimerRestart) -> Self {
+        match value {
+            HrTimerRestart::NoRestart => bindings::hrtimer_restart_HRTIMER_NORESTART,
+            HrTimerRestart::Restart => bindings::hrtimer_restart_HRTIMER_RESTART,
+        }
+    }
+}
+
 /// Use to implement the [`HasHrTimer<T>`] trait.
 ///
 /// See [`module`] documentation for an example.
diff --git a/rust/kernel/time/hrtimer/arc.rs b/rust/kernel/time/hrtimer/arc.rs
index d1c90631d0036..109eded0e73be 100644
--- a/rust/kernel/time/hrtimer/arc.rs
+++ b/rust/kernel/time/hrtimer/arc.rs
@@ -82,8 +82,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()
     }
 }

-- 
2.47.0

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

[Andreas Hindborg]

Add a trait to allow unsafely queuing stack allocated timers.

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

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index e342193f985eb..196794089f033 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -157,6 +157,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 v8 07/14] rust: hrtimer: add `hrtimer::ScopedHrTimerPointer`

[Andreas Hindborg]

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

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

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

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 196794089f033..666bcd6e46a11 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -188,6 +188,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 v8 08/14] rust: hrtimer: implement `UnsafeHrTimerPointer` for `Pin<&T>`

[Andreas Hindborg]

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

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

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 666bcd6e46a11..3228ed916b4fe 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -402,3 +402,5 @@ unsafe fn raw_get_timer(ptr: *const Self) ->
 
 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 0000000000000..ecab39118662e
--- /dev/null
+++ b/rust/kernel/time/hrtimer/pin.rs
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::HasHrTimer;
+use super::HrTimer;
+use super::HrTimerCallback;
+use super::HrTimerHandle;
+use super::RawHrTimerCallback;
+use super::UnsafeHrTimerPointer;
+use crate::time::Ktime;
+use core::pin::Pin;
+
+/// A handle for a `Pin<&HasHrTimer>`. When the handle exists, the timer might be
+/// running.
+pub struct PinHrTimerHandle<'a, 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<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`.
+        unsafe { T::start(self_ptr, expires) };
+
+        PinHrTimerHandle { inner: self }
+    }
+}
+
+impl<'a, T> RawHrTimerCallback for Pin<&'a T>
+where
+    T: HasHrTimer<T>,
+    T: HrTimerCallback<CallbackTarget<'a> = Self>,
+    T: HrTimerCallback<CallbackTargetParameter<'a> = Self>,
+{
+    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+        // `HrTimer` is `repr(C)`
+        let timer_ptr = ptr as *mut HrTimer<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()
+    }
+}

-- 
2.47.0

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

[Andreas Hindborg]

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

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

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index 3228ed916b4fe..f36513cba4351 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -404,3 +404,5 @@ unsafe fn raw_get_timer(ptr: *const Self) ->
 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 0000000000000..153e89b86caf2
--- /dev/null
+++ b/rust/kernel/time/hrtimer/pin_mut.rs
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::HasHrTimer;
+use super::HrTimer;
+use super::HrTimerCallback;
+use super::HrTimerHandle;
+use super::RawHrTimerCallback;
+use super::UnsafeHrTimerPointer;
+use crate::time::Ktime;
+use core::pin::Pin;
+
+/// A handle for a `Pin<&mut HasHrTimer>`. When the handle exists, the timer might
+/// be running.
+pub struct PinMutHrTimerHandle<'a, 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<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`.
+        unsafe { T::start(self_ptr, expires) };
+
+        PinMutHrTimerHandle { inner: self }
+    }
+}
+
+impl<'a, T> RawHrTimerCallback for Pin<&'a mut T>
+where
+    T: HasHrTimer<T>,
+    T: HrTimerCallback<CallbackTarget<'a> = Self>,
+    T: HrTimerCallback<CallbackTargetParameter<'a> = Self>,
+{
+    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+        // `HrTimer` is `repr(C)`
+        let timer_ptr = ptr as *mut HrTimer<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()
+    }
+}

-- 
2.47.0

[PATCH v8 10/14] 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>
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 cb4ebea3b0742..9da4a32e60bc3 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 v8 11/14] rust: hrtimer: implement `HrTimerPointer` for `Pin<Box<T>>`

[Andreas Hindborg]

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

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

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index f36513cba4351..de9edf52ebb07 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -406,3 +406,6 @@ unsafe fn raw_get_timer(ptr: *const Self) ->
 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 0000000000000..39db96de023a8
--- /dev/null
+++ b/rust/kernel/time/hrtimer/tbox.rs
@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: GPL-2.0
+
+use super::HasHrTimer;
+use super::HrTimer;
+use super::HrTimerCallback;
+use super::HrTimerHandle;
+use super::HrTimerPointer;
+use super::RawHrTimerCallback;
+use crate::prelude::*;
+use crate::time::Ktime;
+use core::mem::ManuallyDrop;
+
+/// A handle for a [`Box<HasHrTimer<T>>`] returned by a call to
+/// [`HrTimerPointer::start`].
+pub struct BoxHrTimerHandle<T, A>
+where
+    T: HasHrTimer<T>,
+    A: crate::alloc::Allocator,
+{
+    pub(crate) inner: *mut 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) };
+
+        // 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) })
+    }
+}
+
+impl<T, A> HrTimerPointer for Pin<Box<T, A>>
+where
+    T: Send + Sync,
+    T: HasHrTimer<T>,
+    T: for<'a> HrTimerCallback<CallbackTarget<'a> = Pin<Box<T, A>>>,
+    T: for<'a> HrTimerCallback<CallbackTargetParameter<'a> = Pin<&'a T>>,
+    A: crate::alloc::Allocator,
+{
+    type TimerHandle = BoxHrTimerHandle<T, A>;
+
+    fn start(self, expires: Ktime) -> Self::TimerHandle {
+        let self_ptr: *const T = <Self as core::ops::Deref>::deref(&self);
+
+        // SAFETY: Since we generate the pointer passed to `start` from a valid
+        // reference, it is a valid pointer.
+        unsafe { T::start(self_ptr, expires) };
+
+        // SAFETY: We will not move out of this box during timer callback (we
+        // pass an immutable reference to the callback).
+        let inner = unsafe { Pin::into_inner_unchecked(self) };
+
+        BoxHrTimerHandle {
+            inner: Box::into_raw(inner),
+            _p: core::marker::PhantomData,
+        }
+    }
+}
+
+impl<T, A> RawHrTimerCallback for Pin<Box<T, A>>
+where
+    T: HasHrTimer<T>,
+    T: for<'a> HrTimerCallback<CallbackTarget<'a> = Pin<Box<T, A>>>,
+    T: for<'a> HrTimerCallback<CallbackTargetParameter<'a> = Pin<&'a T>>,
+    A: crate::alloc::Allocator,
+{
+    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart {
+        // `HrTimer` is `repr(C)`
+        let timer_ptr = ptr.cast::<super::HrTimer<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()
+    }
+}

-- 
2.47.0

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

[Andreas Hindborg]

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

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

diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
index de9edf52ebb07..db49061f830c3 100644
--- a/rust/kernel/time/hrtimer.rs
+++ b/rust/kernel/time/hrtimer.rs
@@ -59,6 +59,7 @@
 pub struct HrTimer<T> {
     #[pin]
     timer: Opaque<bindings::hrtimer>,
+    mode: HrTimerMode,
     _t: PhantomData<T>,
 }
 
@@ -72,7 +73,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,
     {
@@ -87,10 +88,11 @@ pub fn new() -> impl PinInit<Self>
                         place,
                         Some(T::CallbackTarget::run),
                         bindings::CLOCK_MONOTONIC as i32,
-                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
+                        mode.into(),
                     );
                 }
             }),
+            mode: mode,
             _t: PhantomData,
         })
     }
@@ -336,7 +338,7 @@ unsafe fn start(self_ptr: *const Self, expires: Ktime) {
                 Self::c_timer_ptr(self_ptr).cast_mut(),
                 expires.to_ns(),
                 0,
-                bindings::hrtimer_mode_HRTIMER_MODE_REL,
+                (*Self::raw_get_timer(self_ptr)).mode.into(),
             );
         }
     }
@@ -368,6 +370,84 @@ fn from(value: HrTimerRestart) -> Self {
     }
 }
 
+/// Operational mode of [`HrTimer`].
+#[derive(Clone, Copy)]
+pub enum HrTimerMode {
+    /// Timer expires at the given expiration time.
+    Absolute,
+    /// Timer expires after the given expiration time interpreted as a duration from now.
+    Relative,
+    /// Timer does not move between CPU cores.
+    Pinned,
+    /// Timer handler is executed in soft irq context.
+    Soft,
+    /// Timer handler is executed in hard irq context.
+    Hard,
+    /// Timer expires at the given expiration time.
+    /// Timer does not move between CPU cores.
+    AbsolutePinned,
+    /// Timer expires after the given expiration time interpreted as a duration from now.
+    /// Timer does not move between CPU cores.
+    RelativePinned,
+    /// Timer expires at the given expiration time.
+    /// Timer handler is executed in soft irq context.
+    AbsoluteSoft,
+    /// Timer expires after the given expiration time interpreted as a duration from now.
+    /// Timer handler is executed in soft irq context.
+    RelativeSoft,
+    /// Timer expires at the given expiration time.
+    /// Timer does not move between CPU cores.
+    /// Timer handler is executed in soft irq context.
+    AbsolutePinnedSoft,
+    /// Timer expires after the given expiration time interpreted as a duration from now.
+    /// Timer does not move between CPU cores.
+    /// Timer handler is executed in soft irq context.
+    RelativePinnedSoft,
+    /// Timer expires at the given expiration time.
+    /// Timer handler is executed in hard irq context.
+    AbsoluteHard,
+    /// Timer expires after the given expiration time interpreted as a duration from now.
+    /// Timer handler is executed in hard irq context.
+    RelativeHard,
+    /// Timer expires at the given expiration time.
+    /// Timer does not move between CPU cores.
+    /// Timer handler is executed in hard irq context.
+    AbsolutePinnedHard,
+    /// Timer expires after the given expiration time interpreted as a duration from now.
+    /// Timer does not move between CPU cores.
+    /// Timer handler is executed in hard irq context.
+    RelativePinnedHard,
+}
+
+impl From<HrTimerMode> for bindings::hrtimer_mode {
+    fn from(value: HrTimerMode) -> Self {
+        use bindings::*;
+        match value {
+            HrTimerMode::Absolute => hrtimer_mode_HRTIMER_MODE_ABS,
+            HrTimerMode::Relative => hrtimer_mode_HRTIMER_MODE_REL,
+            HrTimerMode::Pinned => hrtimer_mode_HRTIMER_MODE_PINNED,
+            HrTimerMode::Soft => hrtimer_mode_HRTIMER_MODE_SOFT,
+            HrTimerMode::Hard => hrtimer_mode_HRTIMER_MODE_HARD,
+            HrTimerMode::AbsolutePinned => hrtimer_mode_HRTIMER_MODE_ABS_PINNED,
+            HrTimerMode::RelativePinned => hrtimer_mode_HRTIMER_MODE_REL_PINNED,
+            HrTimerMode::AbsoluteSoft => hrtimer_mode_HRTIMER_MODE_ABS_SOFT,
+            HrTimerMode::RelativeSoft => hrtimer_mode_HRTIMER_MODE_REL_SOFT,
+            HrTimerMode::AbsolutePinnedSoft => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_SOFT,
+            HrTimerMode::RelativePinnedSoft => hrtimer_mode_HRTIMER_MODE_REL_PINNED_SOFT,
+            HrTimerMode::AbsoluteHard => hrtimer_mode_HRTIMER_MODE_ABS_HARD,
+            HrTimerMode::RelativeHard => hrtimer_mode_HRTIMER_MODE_REL_HARD,
+            HrTimerMode::AbsolutePinnedHard => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_HARD,
+            HrTimerMode::RelativePinnedHard => hrtimer_mode_HRTIMER_MODE_REL_PINNED_HARD,
+        }
+    }
+}
+
+impl From<HrTimerMode> for u64 {
+    fn from(value: HrTimerMode) -> Self {
+        Into::<bindings::hrtimer_mode>::into(value) as u64
+    }
+}
+
 /// Use to implement the [`HasHrTimer<T>`] trait.
 ///
 /// See [`module`] documentation for an example.

-- 
2.47.0

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

[Andreas Hindborg]

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

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

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

-- 
2.47.0

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

[Andreas Hindborg]

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

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

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

-- 
2.47.0

Re: [PATCH v8 00/14] hrtimer Rust API

[Andreas Hindborg]

"Andreas Hindborg" <a.hindborg@kernel.org> writes:

> This series adds support for using the `hrtimer` subsystem from Rust code.
>
> The series adds support for timer mode and clock source configuration during
> timer initialization. Examples and functionality to execute closures at timer
> expiration has been removed, as these depend on either atomics [3] or
> `SpinLockIrq` [4], which are still being worked on.
>
> This series is a dependency for unmerged features of the Rust null block driver
> [1], and for rkvms [2].
>

@ timer subsystem maintainers: did you discuss how you want to set up
maintenance for this yet? As mentioned, I'm happy stepping up to
maintain this, but if you want to handle it with existing resources that
is perfectly fine as well.

I was hoping we could merge the patches in the near future. The patches
have been on list for quite a while now, and I am happy with the shape
of them. They are in my critical path for merging dependent code in the
rust null block driver.

Let me know if there is anything I can do to help move the process
forward.


Best regards,
Andreas Hindborg

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

[Benno Lossin]

On 18.02.25 14:27, Andreas Hindborg wrote:
> From: Lyude Paul <lyude@redhat.com>
> 
> A simple function to turn the provided value in nanoseconds into a Ktime
> value. We allow any type which implements Into<bindings::ktime_t>, which
> resolves to Into<i64>.
> 
> This is useful for some of the older DRM APIs that never got moved to
> Ktime.

Are these older DRM APIs on the C side, or on the Rust side? If they are
on the Rust side, we should just move them to Ktime, no?

> Signed-off-by: Lyude Paul <lyude@redhat.com>
> Reviewed-by: Lyude Paul <lyude@redhat.com>

This seems wrong considering the `From Lyude Paul` line above :) (or is
it possible to review your own patch?)

The patch itself looks good, so iff the above question is answered with
"the older DRM APIs are on the C side" then:

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

---
Cheers,
Benno

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

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

[Andreas Hindborg]

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

> On 18.02.25 14:27, Andreas Hindborg wrote:
>> From: Lyude Paul <lyude@redhat.com>
>>
>> A simple function to turn the provided value in nanoseconds into a Ktime
>> value. We allow any type which implements Into<bindings::ktime_t>, which
>> resolves to Into<i64>.
>>
>> This is useful for some of the older DRM APIs that never got moved to
>> Ktime.
>
> Are these older DRM APIs on the C side, or on the Rust side? If they are
> on the Rust side, we should just move them to Ktime, no?
>
>> Signed-off-by: Lyude Paul <lyude@redhat.com>
>> Reviewed-by: Lyude Paul <lyude@redhat.com>
>
> This seems wrong considering the `From Lyude Paul` line above :) (or is
> it possible to review your own patch?)
>
> The patch itself looks good, so iff the above question is answered with
> "the older DRM APIs are on the C side" then:
>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>

It occurs to me that I am actually not relying on this patch for this
series any longer, so I will just drop it.


Best regards,
Andreas Hindborg

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

[Tamir Duberstein]

On Tue, Feb 18, 2025 at 8:29 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> This patch adds support for intrusive use of the hrtimer system. For now,
> only one timer can be embedded in a Rust struct.
>
> The hrtimer Rust API is based on the intrusive style pattern introduced by
> the Rust workqueue API.
>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/time.rs         |   2 +
>  rust/kernel/time/hrtimer.rs | 312 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 314 insertions(+)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 87e47f2f5618d..2cf365cfb412e 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -10,6 +10,8 @@
>
>  use core::convert::Into;
>
> +pub mod hrtimer;
> +
>  /// The number of nanoseconds per millisecond.
>  pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
>
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> new file mode 100644
> index 0000000000000..a6332924efabd
> --- /dev/null
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -0,0 +1,312 @@
> +// 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
> +//! * Running
> +//!
> +//! Operations:
> +//!
> +//! * Start
> +//! * Cancel
> +//! * Stop
> +//! * Restart
> +//!
> +//! Events:
> +//!
> +//! * Expire
> +//!
> +//! ## State Diagram
> +//!
> +//! ```text
> +//!                  <-- Stop ----
> +//!                  <-- Cancel --
> +//!                  --- Start -->
> +//!        +---------+        +---------+
> +//!   O--->| Stopped |        | Running |---o
> +//!        +---------+        +---------+   |
> +//!                                  ^      |
> +//!                  <- Expire --    |      |
> +//!                                  o------o
> +//!                                   Restart
> +//! ```
> +//!
> +//! A timer is initialized in the **stopped** state. A stopped timer can be
> +//! **started** with an **expiry** time. After the timer is started, it is
> +//! **running**. When the timer **expires**, the timer handler is executed.
> +//! After the handler has executed, the timer may be **restarted** or
> +//! **stopped**. A running timer can be **canceled** before it's handler is

"it's" = it is. This should be "its" (possessive).

Just to be clear, after the handler has executed and before the timer
has been **restarted** or **stopped** the timer is still in the
**running** state?


> +//! executed. A timer that is cancelled enters the **stopped** state.
> +//!
> +
> +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 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 {
> +            // INVARIANTS: We initialize `timer` with `hrtimer_setup` below.
> +            timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
> +                // SAFETY: By design of `pin_init!`, `place` is a pointer 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::CallbackTarget::run),
> +                        bindings::CLOCK_MONOTONIC as i32,
> +                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
> +                    );
> +                }
> +            }),
> +            _t: PhantomData,
> +        })
> +    }
> +
> +    /// Get a pointer to the contained `bindings::hrtimer`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must point to a live allocation of at least the size of `Self`.
> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
> +        // SAFETY: The field projection to `timer` does not go out of bounds,
> +        // because the caller of this function promises that `ptr` points to an
> +        // allocation of at least the size of `Self`.
> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
> +    }

Can you help me understand why the various functions here operate on
*const Self? I understand the need to obtain a C pointer to interact
with bindings, but I don't understand why we're dealing in raw
pointers to the abstraction rather than references. This extends to
HrTimerPointer, which is intended to be implemented by *pointers to*
structs that embed `HrTimer`; why isn't it implemented on by the
embedder itself?

I realize we discussed this on v6, sorry for not keeping up there.

> +
> +    /// Cancel an initialized and potentially running timer.
> +    ///
> +    /// If the timer handler is running, this will block until the handler is
> +    /// finished.
> +    ///
> +    /// Users of the `HrTimer` API would not usually call this method directly.
> +    /// Instead they would use the safe `cancel` method on the [`HrTimerHandle`]
> +    /// returned when the timer was started.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `self_ptr` must point to a valid `Self`.
> +    #[allow(dead_code)]
> +    pub(crate) unsafe fn raw_cancel(self_ptr: *const Self) -> bool {
> +        // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
> +        let c_timer_ptr = unsafe { HrTimer::raw_get(self_ptr) };
> +
> +        // If the handler is running, this will wait for the handler to finish
> +        // before returning.
> +        // SAFETY: `c_timer_ptr` is initialized and valid. Synchronization is
> +        // handled on C side.
> +        unsafe { bindings::hrtimer_cancel(c_timer_ptr) != 0 }
> +    }
> +}
> +
> +/// Implemented by pointer types that point to structs that embed a [`HrTimer`].
> +///
> +/// Target (pointee) must be [`Sync`] because timer callbacks happen in another
> +/// thread of execution (hard or soft interrupt context).
> +///
> +/// Starting a timer returns a [`HrTimerHandle`] that can be used to manipulate
> +/// the timer. Note that it is OK to call the start function repeatedly, and
> +/// that more than one [`HrTimerHandle`] associated with a [`HrTimerPointer`] may
> +/// exist. A timer can be manipulated through any of the handles, and a handle
> +/// may represent a cancelled timer.
> +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 {
> +    /// Callback to be called from C when timer fires.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Only to be called by C code in `hrtimer` subsystem. `ptr` must point to
> +    /// the `bindings::hrtimer` structure that was used to start the timer.
> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
> +}
> +
> +/// Implemented by structs that can be the target of a timer callback.
> +pub trait HrTimerCallback {
> +    /// The type whose [`RawHrTimerCallback::run`] method will be invoked when
> +    /// the timer expires.
> +    type CallbackTarget<'a>: RawHrTimerCallback;
> +
> +    /// This type is passed to the timer callback function. It may be a borrow
> +    /// of [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
> +    /// implementation can guarantee exclusive access to the target during timer
> +    /// handler execution.
> +    type CallbackTargetParameter<'a>;
> +
> +    /// Called by the timer logic when the timer fires.
> +    fn run(this: Self::CallbackTargetParameter<'_>)
> +    where
> +        Self: Sized;
> +}
> +
> +/// A handle representing a potentially running timer.
> +///
> +/// More than one handle representing the same timer might exist.
> +///
> +/// # Safety
> +///
> +/// When dropped, the timer represented by this handle must be cancelled, if it
> +/// is running. If the timer handler is running when the handle is dropped, the
> +/// drop method must wait for the handler to finish before returning.

Between this comment and the comment on cancel we say "if it is
running" 3 times. Can we say it just once, on the method, and here say
that cancel must be called in Drop?

> +pub unsafe trait HrTimerHandle {
> +    /// Cancel the timer, if it is running. If the timer handler is running, block
> +    /// till the handler has finished.
> +    fn cancel(&mut self) -> bool;
> +}
> +
> +/// Implemented by structs that contain timer nodes.
> +///
> +/// Clients of the timer API would usually safely implement this trait by using
> +/// the [`crate::impl_has_hr_timer`] macro.
> +///
> +/// # Safety
> +///
> +/// Implementers of this trait must ensure that the implementer has a [`HrTimer`]
> +/// field at the offset specified by `OFFSET` and that all trait methods are
> +/// implemented according to their documentation.
> +///
> +/// [`impl_has_timer`]: crate::impl_has_timer
> +pub unsafe trait HasHrTimer<T> {
> +    /// Offset of the [`HrTimer`] field within `Self`
> +    const OFFSET: usize;

Does this need to be part of the trait? As an alternative the provided
methods could be generated in the macro below and reduce the
opportunity to implement this trait incorrectly.

> +
> +    /// Return a pointer to the [`HrTimer`] within `Self`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must point to a valid struct of type `Self`.
> +    unsafe fn raw_get_timer(ptr: *const Self) -> *const HrTimer<T> {
> +        // SAFETY: By the safety requirement of this trait, the trait
> +        // implementor will have a `HrTimer` field at the specified offset.
> +        unsafe { ptr.cast::<u8>().add(Self::OFFSET).cast::<HrTimer<T>>() }
> +    }
> +
> +    /// Return a pointer to the struct that is embedding the [`HrTimer`] pointed
> +    /// to by `ptr`.
> +    ///
> +    /// # 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,
> +    {
> +        // SAFETY: By the safety requirement of this function and the `HasHrTimer`
> +        // trait, the following expression will yield a pointer to the `Self`
> +        // containing the timer addressed by `ptr`.
> +        unsafe { ptr.cast::<u8>().sub(Self::OFFSET).cast::<Self>() }
> +    }
> +
> +    /// Get pointer to embedded `bindings::hrtimer` struct.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `self_ptr` must point to a valid `Self`.
> +    unsafe fn c_timer_ptr(self_ptr: *const Self) -> *const bindings::hrtimer {
> +        // SAFETY: `self_ptr` is a valid pointer to a `Self`.
> +        let timer_ptr = unsafe { Self::raw_get_timer(self_ptr) };
> +
> +        // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
> +        unsafe { HrTimer::raw_get(timer_ptr) }
> +    }
> +
> +    /// Start the timer contained in the `Self` pointed to by `self_ptr`. If
> +    /// it is already running it is removed and inserted.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `self_ptr` must point to a valid `Self`.
> +    unsafe fn start(self_ptr: *const Self, expires: Ktime) {
> +        // SAFETY: By function safety requirement, `self_ptr`is a valid `Self`.
> +        unsafe {
> +            bindings::hrtimer_start_range_ns(
> +                Self::c_timer_ptr(self_ptr).cast_mut(),
> +                expires.to_ns(),
> +                0,
> +                bindings::hrtimer_mode_HRTIMER_MODE_REL,
> +            );
> +        }
> +    }
> +}
> +
> +/// Use to implement the [`HasHrTimer<T>`] trait.
> +///
> +/// See [`module`] documentation for an example.
> +///
> +/// [`module`]: crate::time::hrtimer
> +#[macro_export]
> +macro_rules! impl_has_hr_timer {
> +    (
> +        impl$({$($generics:tt)*})?
> +            HasHrTimer<$timer_type:ty>
> +            for $self:ty
> +        { self.$field:ident }
> +        $($rest:tt)*
> +    ) => {
> +        // SAFETY: This implementation of `raw_get_timer` only compiles if the
> +        // field has the right type.
> +        unsafe impl$(<$($generics)*>)? $crate::time::hrtimer::HasHrTimer<$timer_type> for $self {
> +            const OFFSET: usize = ::core::mem::offset_of!(Self, $field) as usize;
> +
> +            #[inline]
> +            unsafe fn raw_get_timer(ptr: *const Self) ->
> +                *const $crate::time::hrtimer::HrTimer<$timer_type>
> +            {
> +                // SAFETY: The caller promises that the pointer is not dangling.
> +                unsafe {
> +                    ::core::ptr::addr_of!((*ptr).$field)
> +                }
> +            }
> +        }
> +    }
> +}
>
> --
> 2.47.0
>
>

Re: [PATCH v8 00/14] hrtimer Rust API

[Frederic Weisbecker]

Le Wed, Feb 19, 2025 at 12:02:50PM +0100, Andreas Hindborg a écrit :
> "Andreas Hindborg" <a.hindborg@kernel.org> writes:
> 
> > This series adds support for using the `hrtimer` subsystem from Rust code.
> >
> > The series adds support for timer mode and clock source configuration during
> > timer initialization. Examples and functionality to execute closures at timer
> > expiration has been removed, as these depend on either atomics [3] or
> > `SpinLockIrq` [4], which are still being worked on.
> >
> > This series is a dependency for unmerged features of the Rust null block driver
> > [1], and for rkvms [2].
> >
> 
> @ timer subsystem maintainers: did you discuss how you want to set up
> maintenance for this yet? As mentioned, I'm happy stepping up to
> maintain this, but if you want to handle it with existing resources that
> is perfectly fine as well.

You're the best candidate to maintain this code since you wrote it :-)

Also I personally have near zero skills in Rust as of today so all I can do
is to vaguely keep an eye on the binding's interface and keep in touch
with the changes.

So I suggest you to add a new entry with you as a maintainer (I suggested
something similar to Fujita for some other timer related things) but please
keep us Cc'ed for future changes.

> I was hoping we could merge the patches in the near future. The patches
> have been on list for quite a while now, and I am happy with the shape
> of them. They are in my critical path for merging dependent code in the
> rust null block driver.
> 
> Let me know if there is anything I can do to help move the process
> forward.

Let me have a last look...

Thanks.


> 
> 
> Best regards,
> Andreas Hindborg
> 
> 
> 
> 

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

[Andreas Hindborg]

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

> On Tue, Feb 18, 2025 at 8:29 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>

[...]

>> +//! ## State Diagram
>> +//!
>> +//! ```text
>> +//!                  <-- Stop ----
>> +//!                  <-- Cancel --
>> +//!                  --- Start -->
>> +//!        +---------+        +---------+
>> +//!   O--->| Stopped |        | Running |---o
>> +//!        +---------+        +---------+   |
>> +//!                                  ^      |
>> +//!                  <- Expire --    |      |
>> +//!                                  o------o
>> +//!                                   Restart
>> +//! ```
>> +//!
>> +//! A timer is initialized in the **stopped** state. A stopped timer can be
>> +//! **started** with an **expiry** time. After the timer is started, it is
>> +//! **running**. When the timer **expires**, the timer handler is executed.
>> +//! After the handler has executed, the timer may be **restarted** or
>> +//! **stopped**. A running timer can be **canceled** before it's handler is
>
> "it's" = it is. This should be "its" (possessive).

Thanks 👍

> Just to be clear, after the handler has executed and before the timer
> has been **restarted** or **stopped** the timer is still in the
> **running** state?

It depends on the return value of the handler. If the handler returns restart,
the timer the timer does not enter the stopped state. If the handler
returns stop, the timer enters the stopped state.

The timer is still considered to be in running state the handler is
running.

I can add this info to the section.

>
>
>> +//! executed. A timer that is cancelled enters the **stopped** state.
>> +//!
>> +
>> +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 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 {
>> +            // INVARIANTS: We initialize `timer` with `hrtimer_setup` below.
>> +            timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
>> +                // SAFETY: By design of `pin_init!`, `place` is a pointer 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::CallbackTarget::run),
>> +                        bindings::CLOCK_MONOTONIC as i32,
>> +                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
>> +                    );
>> +                }
>> +            }),
>> +            _t: PhantomData,
>> +        })
>> +    }
>> +
>> +    /// Get a pointer to the contained `bindings::hrtimer`.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// `ptr` must point to a live allocation of at least the size of `Self`.
>> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
>> +        // SAFETY: The field projection to `timer` does not go out of bounds,
>> +        // because the caller of this function promises that `ptr` points to an
>> +        // allocation of at least the size of `Self`.
>> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
>> +    }
>
> Can you help me understand why the various functions here operate on
> *const Self? I understand the need to obtain a C pointer to interact
> with bindings, but I don't understand why we're dealing in raw
> pointers to the abstraction rather than references.

We cannot reference the `bindings::hrtimer` without wrapping it in
`Opaque`. This would be the primary reason. At other times, we cannot
produce references because we might not be able to prove that we satisfy
the safety requirements for turning a pointer into a reference. If we
are just doing offset arithmetic anyway, we don't need a reference.


> This extends to
> HrTimerPointer, which is intended to be implemented by *pointers to*
> structs that embed `HrTimer`; why isn't it implemented on by the
> embedder itself?

Not sure what you mean here. If you refer to for instance the
implementation of `HrTimerPointer for Arc<T>`, I get why you might
wonder, why does `HasHrTimer::start` not take a reference instead of a
pointer? We could do that, but we would just immediately break it down
again in the implementation of `HasHrTimer::start`. Might still be a
good idea though.

>
> I realize we discussed this on v6, sorry for not keeping up there.

No worries, it is good that we discuss this.

[...]

>> +
>> +/// A handle representing a potentially running timer.
>> +///
>> +/// More than one handle representing the same timer might exist.
>> +///
>> +/// # Safety
>> +///
>> +/// When dropped, the timer represented by this handle must be cancelled, if it
>> +/// is running. If the timer handler is running when the handle is dropped, the
>> +/// drop method must wait for the handler to finish before returning.
>
> Between this comment and the comment on cancel we say "if it is
> running" 3 times. Can we say it just once, on the method, and here say
> that cancel must be called in Drop?

Well, the comment on `cancel` is just a description of what the function
does. This piece of text is a safety requirement.

We could make the safety requirement for implementing the trait "Implement
the methods according to their documentation". But that would not help with
the drop requirement.

>
>> +pub unsafe trait HrTimerHandle {
>> +    /// Cancel the timer, if it is running. If the timer handler is running, block
>> +    /// till the handler has finished.
>> +    fn cancel(&mut self) -> bool;
>> +}
>> +
>> +/// Implemented by structs that contain timer nodes.
>> +///
>> +/// Clients of the timer API would usually safely implement this trait by using
>> +/// the [`crate::impl_has_hr_timer`] macro.
>> +///
>> +/// # Safety
>> +///
>> +/// Implementers of this trait must ensure that the implementer has a [`HrTimer`]
>> +/// field at the offset specified by `OFFSET` and that all trait methods are
>> +/// implemented according to their documentation.
>> +///
>> +/// [`impl_has_timer`]: crate::impl_has_timer
>> +pub unsafe trait HasHrTimer<T> {
>> +    /// Offset of the [`HrTimer`] field within `Self`
>> +    const OFFSET: usize;
>
> Does this need to be part of the trait? As an alternative the provided
> methods could be generated in the macro below and reduce the
> opportunity to implement this trait incorrectly.

There is no risk of implementing the trait wrong, because it is usually
derived by a macro.

We need at least one of the methods to be able to have the type system
verify that the type for which we implement `HasHrTImer` actually has a
field with the name we specify, and that this field has the right type.
And to have that, we need the OFFSET.



Best regards,
Andreas Hindborg

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

[Tamir Duberstein]

On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Tamir Duberstein" <tamird@gmail.com> writes:
>
> > On Tue, Feb 18, 2025 at 8:29 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
>
> [...]
>
> >> +//! ## State Diagram
> >> +//!
> >> +//! ```text
> >> +//!                  <-- Stop ----
> >> +//!                  <-- Cancel --
> >> +//!                  --- Start -->
> >> +//!        +---------+        +---------+
> >> +//!   O--->| Stopped |        | Running |---o
> >> +//!        +---------+        +---------+   |
> >> +//!                                  ^      |
> >> +//!                  <- Expire --    |      |
> >> +//!                                  o------o
> >> +//!                                   Restart
> >> +//! ```
> >> +//!
> >> +//! A timer is initialized in the **stopped** state. A stopped timer can be
> >> +//! **started** with an **expiry** time. After the timer is started, it is
> >> +//! **running**. When the timer **expires**, the timer handler is executed.
> >> +//! After the handler has executed, the timer may be **restarted** or
> >> +//! **stopped**. A running timer can be **canceled** before it's handler is
> >
> > "it's" = it is. This should be "its" (possessive).
>
> Thanks 👍
>
> > Just to be clear, after the handler has executed and before the timer
> > has been **restarted** or **stopped** the timer is still in the
> > **running** state?
>
> It depends on the return value of the handler. If the handler returns restart,
> the timer the timer does not enter the stopped state. If the handler
> returns stop, the timer enters the stopped state.
>
> The timer is still considered to be in running state the handler is
> running.
>
> I can add this info to the section.

Yeah, some clarification here would be useful.

> >
> >
> >> +//! executed. A timer that is cancelled enters the **stopped** state.
> >> +//!
> >> +
> >> +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 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 {
> >> +            // INVARIANTS: We initialize `timer` with `hrtimer_setup` below.
> >> +            timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
> >> +                // SAFETY: By design of `pin_init!`, `place` is a pointer 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::CallbackTarget::run),
> >> +                        bindings::CLOCK_MONOTONIC as i32,
> >> +                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
> >> +                    );
> >> +                }
> >> +            }),
> >> +            _t: PhantomData,
> >> +        })
> >> +    }
> >> +
> >> +    /// Get a pointer to the contained `bindings::hrtimer`.
> >> +    ///
> >> +    /// # Safety
> >> +    ///
> >> +    /// `ptr` must point to a live allocation of at least the size of `Self`.
> >> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
> >> +        // SAFETY: The field projection to `timer` does not go out of bounds,
> >> +        // because the caller of this function promises that `ptr` points to an
> >> +        // allocation of at least the size of `Self`.
> >> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
> >> +    }
> >
> > Can you help me understand why the various functions here operate on
> > *const Self? I understand the need to obtain a C pointer to interact
> > with bindings, but I don't understand why we're dealing in raw
> > pointers to the abstraction rather than references.
>
> We cannot reference the `bindings::hrtimer` without wrapping it in
> `Opaque`. This would be the primary reason. At other times, we cannot
> produce references because we might not be able to prove that we satisfy
> the safety requirements for turning a pointer into a reference. If we
> are just doing offset arithmetic anyway, we don't need a reference.

Why do we have a pointer, rather than a reference, to Self in the
first place? I think this is the key thing I don't understand.

>
>
> > This extends to
> > HrTimerPointer, which is intended to be implemented by *pointers to*
> > structs that embed `HrTimer`; why isn't it implemented on by the
> > embedder itself?
>
> Not sure what you mean here. If you refer to for instance the
> implementation of `HrTimerPointer for Arc<T>`, I get why you might
> wonder, why does `HasHrTimer::start` not take a reference instead of a
> pointer? We could do that, but we would just immediately break it down
> again in the implementation of `HasHrTimer::start`. Might still be a
> good idea though.

I was trying to say that my question (which I clarified above,
hopefully) extends to the description and name of this trait.
Specifically for this trait I don't understand why its semantics are
described in terms of pointers rather than references (and AsRef, to
allow for Arc and friends).

> >
> > I realize we discussed this on v6, sorry for not keeping up there.
>
> No worries, it is good that we discuss this.
>
> [...]
>
> >> +
> >> +/// A handle representing a potentially running timer.
> >> +///
> >> +/// More than one handle representing the same timer might exist.
> >> +///
> >> +/// # Safety
> >> +///
> >> +/// When dropped, the timer represented by this handle must be cancelled, if it
> >> +/// is running. If the timer handler is running when the handle is dropped, the
> >> +/// drop method must wait for the handler to finish before returning.
> >
> > Between this comment and the comment on cancel we say "if it is
> > running" 3 times. Can we say it just once, on the method, and here say
> > that cancel must be called in Drop?
>
> Well, the comment on `cancel` is just a description of what the function
> does. This piece of text is a safety requirement.
>
> We could make the safety requirement for implementing the trait "Implement
> the methods according to their documentation". But that would not help with
> the drop requirement.

My suggestion is that the safety comment read: when dropped,
[Self::cancel] must be called. Something like that.

>
> >
> >> +pub unsafe trait HrTimerHandle {
> >> +    /// Cancel the timer, if it is running. If the timer handler is running, block
> >> +    /// till the handler has finished.
> >> +    fn cancel(&mut self) -> bool;
> >> +}
> >> +
> >> +/// Implemented by structs that contain timer nodes.
> >> +///
> >> +/// Clients of the timer API would usually safely implement this trait by using
> >> +/// the [`crate::impl_has_hr_timer`] macro.
> >> +///
> >> +/// # Safety
> >> +///
> >> +/// Implementers of this trait must ensure that the implementer has a [`HrTimer`]
> >> +/// field at the offset specified by `OFFSET` and that all trait methods are
> >> +/// implemented according to their documentation.
> >> +///
> >> +/// [`impl_has_timer`]: crate::impl_has_timer
> >> +pub unsafe trait HasHrTimer<T> {
> >> +    /// Offset of the [`HrTimer`] field within `Self`
> >> +    const OFFSET: usize;
> >
> > Does this need to be part of the trait? As an alternative the provided
> > methods could be generated in the macro below and reduce the
> > opportunity to implement this trait incorrectly.
>
> There is no risk of implementing the trait wrong, because it is usually
> derived by a macro.

There's no risk when it's implemented by the macro, but you used the
word usually,  which means there is a risk.

> We need at least one of the methods to be able to have the type system
> verify that the type for which we implement `HasHrTImer` actually has a
> field with the name we specify, and that this field has the right type.
> And to have that, we need the OFFSET.

I don't follow this logic. OFFSET is calculated in the body of the
macro. I'm suggesting that the macro generate the method
implementations (which would no longer be provided). In effect I'm
saying: keep OFFSET private.

I'm also noticing now that the macro generates an implementation of
raw_get_timer *in addition to* the provided implementation. Why are
both needed?

>
>
>
> Best regards,
> Andreas Hindborg
>
>

Re: [PATCH v8 00/14] hrtimer Rust API

[Frederic Weisbecker]

Le Tue, Feb 18, 2025 at 02:27:05PM +0100, Andreas Hindborg a écrit :
> This series adds support for using the `hrtimer` subsystem from Rust code.
> 
> The series adds support for timer mode and clock source configuration during
> timer initialization. Examples and functionality to execute closures at timer
> expiration has been removed, as these depend on either atomics [3] or
> `SpinLockIrq` [4], which are still being worked on.
> 
> This series is a dependency for unmerged features of the Rust null block driver
> [1], and for rkvms [2].
> 
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/a.hindborg/linux.git/log/?h=rnull-v6.11-rc2 [1]
> Link: https://gitlab.freedesktop.org/lyudess/linux/-/tree/rvkms-wip [2]
> Link: https://lore.kernel.org/rust-for-linux/20240612223025.1158537-1-boqun.feng@gmail.com/ [3]
> Link: https://lore.kernel.org/rust-for-linux/20240916213025.477225-1-lyude@redhat.com/ [4]
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

Let me repeat again that I have extremely limited understanding of this.
But let's say I haven't spotted a major issue on the interface...

	    Acked-by: Frederic Weisbecker <frederic@kernel.org>

Thanks!

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

[Benno Lossin]

On 18.02.25 14:27, Andreas Hindborg wrote:
> Add a trait to allow unsafely queuing stack allocated timers.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

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

> ---
>  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 e342193f985eb..196794089f033 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -157,6 +157,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;

I have an interesting idea to make this function safe, but I don't want
to block this series.

How about we have the following signature:

    fn start(self, expires: Ktime) -> impl PinInit<Self::TimerHandle>;

And in the safety requirements of the trait we ask that `TimerHandle` be
`!Unpin`. Then the TimerHandle cannot be forgotten because it is pinned
and pinned values have the drop guarantee [1].

You can then use `stack_pin_init!` to pin the value directly on the
stack (dropping it by leaving its scope will then cancel the timer &
possibly wait for the timer callback to finish running).

[1]: https://doc.rust-lang.org/std/pin/index.html#drop-guarantee

---
Cheers,
Benno

> +}
> +
>  /// 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
> 
> 

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

[Benno Lossin]

On 18.02.25 14:27, Andreas Hindborg wrote:
> 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>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---

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

---
Cheers,
Benno

> 
> 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 3cefda7a43725..1dfa75714f9d6 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
> 
> 

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

[Benno Lossin]

On 18.02.25 14:27, Andreas Hindborg wrote:
> Add an associated function to convert a `Box<T>` into a `Pin<Box<T>>`.

Why don't you use `into()` directly where you need it? Do you want the
function call to be more descriptive?
(To be clear, I'm not against the addition, just wanting to check if our
motivation is the same)

> Acked-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>

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

---
Cheers,
Benno

> ---
>  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 cb4ebea3b0742..9da4a32e60bc3 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
> 
> 

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

[Benno Lossin]

On 18.02.25 14:27, Andreas Hindborg wrote:
> +/// Operational mode of [`HrTimer`].
> +#[derive(Clone, Copy)]
> +pub enum HrTimerMode {
> +    /// Timer expires at the given expiration time.
> +    Absolute,
> +    /// Timer expires after the given expiration time interpreted as a duration from now.
> +    Relative,
> +    /// Timer does not move between CPU cores.
> +    Pinned,
> +    /// Timer handler is executed in soft irq context.
> +    Soft,
> +    /// Timer handler is executed in hard irq context.
> +    Hard,
> +    /// Timer expires at the given expiration time.
> +    /// Timer does not move between CPU cores.
> +    AbsolutePinned,
> +    /// Timer expires after the given expiration time interpreted as a duration from now.
> +    /// Timer does not move between CPU cores.
> +    RelativePinned,
> +    /// Timer expires at the given expiration time.
> +    /// Timer handler is executed in soft irq context.
> +    AbsoluteSoft,
> +    /// Timer expires after the given expiration time interpreted as a duration from now.
> +    /// Timer handler is executed in soft irq context.
> +    RelativeSoft,
> +    /// Timer expires at the given expiration time.
> +    /// Timer does not move between CPU cores.
> +    /// Timer handler is executed in soft irq context.
> +    AbsolutePinnedSoft,
> +    /// Timer expires after the given expiration time interpreted as a duration from now.
> +    /// Timer does not move between CPU cores.
> +    /// Timer handler is executed in soft irq context.
> +    RelativePinnedSoft,
> +    /// Timer expires at the given expiration time.
> +    /// Timer handler is executed in hard irq context.
> +    AbsoluteHard,
> +    /// Timer expires after the given expiration time interpreted as a duration from now.
> +    /// Timer handler is executed in hard irq context.
> +    RelativeHard,
> +    /// Timer expires at the given expiration time.
> +    /// Timer does not move between CPU cores.
> +    /// Timer handler is executed in hard irq context.
> +    AbsolutePinnedHard,
> +    /// Timer expires after the given expiration time interpreted as a duration from now.
> +    /// Timer does not move between CPU cores.
> +    /// Timer handler is executed in hard irq context.
> +    RelativePinnedHard,
> +}

At some point we probably want to move this to bitfields, or do you
think it's better to keep it like this?

> +
> +impl From<HrTimerMode> for bindings::hrtimer_mode {
> +    fn from(value: HrTimerMode) -> Self {
> +        use bindings::*;
> +        match value {
> +            HrTimerMode::Absolute => hrtimer_mode_HRTIMER_MODE_ABS,
> +            HrTimerMode::Relative => hrtimer_mode_HRTIMER_MODE_REL,
> +            HrTimerMode::Pinned => hrtimer_mode_HRTIMER_MODE_PINNED,
> +            HrTimerMode::Soft => hrtimer_mode_HRTIMER_MODE_SOFT,
> +            HrTimerMode::Hard => hrtimer_mode_HRTIMER_MODE_HARD,
> +            HrTimerMode::AbsolutePinned => hrtimer_mode_HRTIMER_MODE_ABS_PINNED,
> +            HrTimerMode::RelativePinned => hrtimer_mode_HRTIMER_MODE_REL_PINNED,
> +            HrTimerMode::AbsoluteSoft => hrtimer_mode_HRTIMER_MODE_ABS_SOFT,
> +            HrTimerMode::RelativeSoft => hrtimer_mode_HRTIMER_MODE_REL_SOFT,
> +            HrTimerMode::AbsolutePinnedSoft => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_SOFT,
> +            HrTimerMode::RelativePinnedSoft => hrtimer_mode_HRTIMER_MODE_REL_PINNED_SOFT,
> +            HrTimerMode::AbsoluteHard => hrtimer_mode_HRTIMER_MODE_ABS_HARD,
> +            HrTimerMode::RelativeHard => hrtimer_mode_HRTIMER_MODE_REL_HARD,
> +            HrTimerMode::AbsolutePinnedHard => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_HARD,
> +            HrTimerMode::RelativePinnedHard => hrtimer_mode_HRTIMER_MODE_REL_PINNED_HARD,
> +        }
> +    }
> +}
> +
> +impl From<HrTimerMode> for u64 {
> +    fn from(value: HrTimerMode) -> Self {
> +        Into::<bindings::hrtimer_mode>::into(value) as u64
> +    }
> +}

Hmm do drivers really need these impls? If not, then you could also just
have a private function that does the conversion and use it only in the
abstraction layer.

---
Cheers,
Benno

> +
>  /// Use to implement the [`HasHrTimer<T>`] trait.
>  ///
>  /// See [`module`] documentation for an example.
> 
> --
> 2.47.0
> 
> 

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

[Benno Lossin]

On 18.02.25 14:27, Andreas Hindborg wrote:
> Allow selecting a clock source for timers by passing a `ClockSource`
> variant to `HrTimer::new`.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/time/hrtimer.rs | 52 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 50 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index db49061f830c3..2b46d66eaa313 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -73,7 +73,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: ClockSource) -> impl PinInit<Self>
>      where
>          T: HrTimerCallback,
>      {
> @@ -87,7 +87,7 @@ pub fn new(mode: HrTimerMode) -> impl PinInit<Self>
>                      bindings::hrtimer_setup(
>                          place,
>                          Some(T::CallbackTarget::run),
> -                        bindings::CLOCK_MONOTONIC as i32,
> +                        clock.into(),
>                          mode.into(),
>                      );
>                  }
> @@ -448,6 +448,54 @@ fn from(value: HrTimerMode) -> Self {
>      }
>  }
> 
> +/// The clock source to use for a [`HrTimer`].
> +pub enum ClockSource {
> +    /// A settable system-wide clock that measures real (i.e., wall-clock) time.

Missing newline here to separate the short one-line description and the
rest of the docs. (also below)

> +    /// Setting this clock requires appropriate privileges. This clock is
> +    /// affected by discontinuous jumps in the system time (e.g., if the system
> +    /// administrator manually changes the clock), and by frequency adjustments
> +    /// performed by NTP and similar applications via adjtime(3), adjtimex(2),
> +    /// clock_adjtime(2), and ntp_adjtime(3). This clock normally counts the
> +    /// number of seconds since 1970-01-01 00:00:00 Coordinated Universal Time
> +    /// (UTC) except that it ignores leap seconds; near a leap second it is
> +    /// typically adjusted by NTP to stay roughly in sync with UTC.

Thanks for the extensive descriptions of the various clock sources!

> +    RealTime,
> +    /// A nonsettable system-wide clock that represents monotonic time since—as
> +    /// described by POSIX—"some unspecified point in the past". On Linux, that
> +    /// point corresponds to the number of seconds that the system has been
> +    /// running since it was booted.
> +    ///
> +    /// The CLOCK_MONOTONIC clock is not affected by discontinuous jumps in the
> +    /// system time (e.g., if the system administrator manually changes the
> +    /// clock), but is affected by frequency adjustments. This clock does not
> +    /// count time that the system is suspended.
> +    Monotonic,
> +    /// A nonsettable system-wide clock that is identical to CLOCK_MONOTONIC,
> +    /// except that it also includes any time that the system is suspended. This
> +    /// allows applications to get a suspend-aware monotonic clock without
> +    /// having to deal with the complications of CLOCK_REALTIME, which may have
> +    /// discontinuities if the time is changed using settimeofday(2) or similar.
> +    BootTime,
> +    /// A nonsettable system-wide clock derived from wall-clock time but
> +    /// counting leap seconds. This clock does not experience discontinuities or
> +    /// frequency adjustments caused by inserting leap seconds as CLOCK_REALTIME
> +    /// does.
> +    ///
> +    /// The acronym TAI refers to International Atomic Time.

In that case, I would expect the acronym to be `IAT`.

> +    TAI,
> +}
> +
> +impl From<ClockSource> for bindings::clockid_t {
> +    fn from(value: ClockSource) -> Self {
> +        match value {
> +            ClockSource::RealTime => bindings::CLOCK_REALTIME as i32,
> +            ClockSource::Monotonic => bindings::CLOCK_MONOTONIC as i32,
> +            ClockSource::BootTime => bindings::CLOCK_BOOTTIME as i32,
> +            ClockSource::TAI => bindings::CLOCK_TAI as i32,
> +        }
> +    }
> +}

Same question here as for the `HrTimerMode`, do drivers need this impl?
If not, then I think a private conversion function is a better fit.

---
Cheers,
Benno

> +
>  /// Use to implement the [`HasHrTimer<T>`] trait.
>  ///
>  /// See [`module`] documentation for an example.
> 
> --
> 2.47.0
> 
> 

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

[Benno Lossin]

On 18.02.25 14:27, Andreas Hindborg wrote:
> This patch adds support for intrusive use of the hrtimer system. For now,
> only one timer can be embedded in a Rust struct.
> 
> The hrtimer Rust API is based on the intrusive style pattern introduced by
> the Rust workqueue API.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/time.rs         |   2 +
>  rust/kernel/time/hrtimer.rs | 312 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 314 insertions(+)
> 
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 87e47f2f5618d..2cf365cfb412e 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -10,6 +10,8 @@
> 
>  use core::convert::Into;
> 
> +pub mod hrtimer;
> +
>  /// The number of nanoseconds per millisecond.
>  pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
> 
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> new file mode 100644
> index 0000000000000..a6332924efabd
> --- /dev/null
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -0,0 +1,312 @@
> +// 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
> +//! * Running
> +//!
> +//! Operations:
> +//!
> +//! * Start
> +//! * Cancel
> +//! * Stop
> +//! * Restart
> +//!
> +//! Events:
> +//!
> +//! * Expire
> +//!
> +//! ## State Diagram
> +//!
> +//! ```text
> +//!                  <-- Stop ----
> +//!                  <-- Cancel --
> +//!                  --- Start -->
> +//!        +---------+        +---------+
> +//!   O--->| Stopped |        | Running |---o
> +//!        +---------+        +---------+   |
> +//!                                  ^      |
> +//!                  <- Expire --    |      |
> +//!                                  o------o
> +//!                                   Restart
> +//! ```
> +//!
> +//! A timer is initialized in the **stopped** state. A stopped timer can be
> +//! **started** with an **expiry** time. After the timer is started, it is
> +//! **running**. When the timer **expires**, the timer handler is executed.
> +//! After the handler has executed, the timer may be **restarted** or
> +//! **stopped**. A running timer can be **canceled** before it's handler is

This confuses me a bit, in the other thread you wrote that the handler
decides if the timer should restart or be stopped. But What happens when
I call `cancel` on the `HrTimerHandle` while the handler is running, but
finishes shortly after with a restart request? Will it be canceled?

I also have a bit of a wording issue with "the timer is running" IIUC,
this means that the timer subsystem keeps track of the expiry time and
when the time is elapsed, it fires the code that you registered prior.
At first, I thought that "the timer is running" meant that the
registered code is running. Maybe we should have two different terms for
that? I personally would prefer "active timer" for "the timer subsystem
is currently tracking the time and when it is elapsed, it will run the
code" and "running timer" for "the timer's expiry time has elapsed and
the timer callback is currently being executed".

> +//! executed. A timer that is cancelled enters the **stopped** state.
> +//!
> +
> +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 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 {
> +            // INVARIANTS: We initialize `timer` with `hrtimer_setup` below.
> +            timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
> +                // SAFETY: By design of `pin_init!`, `place` is a pointer 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::CallbackTarget::run),
> +                        bindings::CLOCK_MONOTONIC as i32,
> +                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
> +                    );
> +                }
> +            }),
> +            _t: PhantomData,
> +        })
> +    }
> +
> +    /// Get a pointer to the contained `bindings::hrtimer`.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `ptr` must point to a live allocation of at least the size of `Self`.
> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
> +        // SAFETY: The field projection to `timer` does not go out of bounds,
> +        // because the caller of this function promises that `ptr` points to an
> +        // allocation of at least the size of `Self`.
> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
> +    }
> +
> +    /// Cancel an initialized and potentially running timer.
> +    ///
> +    /// If the timer handler is running, this will block until the handler is
> +    /// finished.
> +    ///
> +    /// Users of the `HrTimer` API would not usually call this method directly.
> +    /// Instead they would use the safe `cancel` method on the [`HrTimerHandle`]

Can you link to the `cancel` function?

> +    /// returned when the timer was started.
> +    ///
> +    /// # Safety
> +    ///
> +    /// `self_ptr` must point to a valid `Self`.
> +    #[allow(dead_code)]
> +    pub(crate) unsafe fn raw_cancel(self_ptr: *const Self) -> bool {
> +        // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
> +        let c_timer_ptr = unsafe { HrTimer::raw_get(self_ptr) };
> +
> +        // If the handler is running, this will wait for the handler to finish
> +        // before returning.
> +        // SAFETY: `c_timer_ptr` is initialized and valid. Synchronization is
> +        // handled on C side.
> +        unsafe { bindings::hrtimer_cancel(c_timer_ptr) != 0 }
> +    }
> +}
> +
> +/// Implemented by pointer types that point to structs that embed a [`HrTimer`].
> +///
> +/// Target (pointee) must be [`Sync`] because timer callbacks happen in another
> +/// thread of execution (hard or soft interrupt context).
> +///
> +/// Starting a timer returns a [`HrTimerHandle`] that can be used to manipulate
> +/// the timer. Note that it is OK to call the start function repeatedly, and
> +/// that more than one [`HrTimerHandle`] associated with a [`HrTimerPointer`] may
> +/// exist. A timer can be manipulated through any of the handles, and a handle
> +/// may represent a cancelled timer.
> +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.

I don't understand this argument. The bounds in the other patches seem
like they could easily be combined. Then this trait could be merged into
the one above.

> +pub trait RawHrTimerCallback {
> +    /// Callback to be called from C when timer fires.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Only to be called by C code in `hrtimer` subsystem. `ptr` must point to
> +    /// the `bindings::hrtimer` structure that was used to start the timer.
> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
> +}
> +
> +/// Implemented by structs that can be the target of a timer callback.
> +pub trait HrTimerCallback {
> +    /// The type whose [`RawHrTimerCallback::run`] method will be invoked when
> +    /// the timer expires.
> +    type CallbackTarget<'a>: RawHrTimerCallback;
> +
> +    /// This type is passed to the timer callback function. It may be a borrow
> +    /// of [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
> +    /// implementation can guarantee exclusive access to the target during timer

Technically "exclusive" access is correct if the `CallbackTarget` is
`Pin<&Self>`, since you will get exclusive access to a `Pin<&Self>`, but
it might confuse people, because there can be multiple `Pin<&Self>`. So
I would just drop the word "exclusive" here.

> +    /// handler execution.
> +    type CallbackTargetParameter<'a>;

Also why can't this type be an associated type of `HrTimerPointer`?
Since this seems to always be constrained in the impls of
`RawHrTimerCallback`.

---
Cheers,
Benno

> +
> +    /// Called by the timer logic when the timer fires.
> +    fn run(this: Self::CallbackTargetParameter<'_>)
> +    where
> +        Self: Sized;
> +}

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

[Benno Lossin]

On 18.02.25 14:27, Andreas Hindborg wrote:
> This patch allows timer handlers to report that they want a timer to be
> restarted after the timer handler has finished executing.
> 
> Also update the `hrtimer` documentation to showcase the new feature.
> 
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/time/hrtimer.rs     | 28 +++++++++++++++++++++++++++-
>  rust/kernel/time/hrtimer/arc.rs |  4 +---
>  2 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index b2a3130bc21d7..e342193f985eb 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -183,7 +183,7 @@ pub trait HrTimerCallback {
>      type CallbackTargetParameter<'a>;
> 
>      /// Called by the timer logic when the timer fires.
> -    fn run(this: Self::CallbackTargetParameter<'_>)
> +    fn run(this: Self::CallbackTargetParameter<'_>) -> HrTimerRestart
>      where
>          Self: Sized;
>  }
> @@ -278,6 +278,32 @@ unsafe fn start(self_ptr: *const Self, expires: Ktime) {
>      }
>  }
> 
> +/// Restart policy for timers.
> +pub enum HrTimerRestart {
> +    /// Timer should not be restarted.
> +    NoRestart,
> +    /// Timer should be restarted.
> +    Restart,
> +}
> +
> +impl From<bindings::hrtimer_restart> for HrTimerRestart {
> +    fn from(value: u32) -> Self {
> +        match value {
> +            bindings::hrtimer_restart_HRTIMER_NORESTART => Self::NoRestart,
> +            _ => Self::Restart,
> +        }
> +    }
> +}
> +
> +impl From<HrTimerRestart> for bindings::hrtimer_restart {
> +    fn from(value: HrTimerRestart) -> Self {
> +        match value {
> +            HrTimerRestart::NoRestart => bindings::hrtimer_restart_HRTIMER_NORESTART,
> +            HrTimerRestart::Restart => bindings::hrtimer_restart_HRTIMER_RESTART,
> +        }
> +    }
> +}

Do drivers need these impls? If no, I would prefer a private conversion
function just for the abstraction.

---
Cheers,
Benno

> +
>  /// 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 d1c90631d0036..109eded0e73be 100644
> --- a/rust/kernel/time/hrtimer/arc.rs
> +++ b/rust/kernel/time/hrtimer/arc.rs
> @@ -82,8 +82,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()
>      }
>  }
> 
> --
> 2.47.0
> 
> 

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

[Andreas Hindborg]

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

> On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:

[...]

>> >> +pub unsafe trait HrTimerHandle {
>> >> +    /// Cancel the timer, if it is running. If the timer handler is running, block
>> >> +    /// till the handler has finished.
>> >> +    fn cancel(&mut self) -> bool;
>> >> +}
>> >> +
>> >> +/// Implemented by structs that contain timer nodes.
>> >> +///
>> >> +/// Clients of the timer API would usually safely implement this trait by using
>> >> +/// the [`crate::impl_has_hr_timer`] macro.
>> >> +///
>> >> +/// # Safety
>> >> +///
>> >> +/// Implementers of this trait must ensure that the implementer has a [`HrTimer`]
>> >> +/// field at the offset specified by `OFFSET` and that all trait methods are
>> >> +/// implemented according to their documentation.
>> >> +///
>> >> +/// [`impl_has_timer`]: crate::impl_has_timer
>> >> +pub unsafe trait HasHrTimer<T> {
>> >> +    /// Offset of the [`HrTimer`] field within `Self`
>> >> +    const OFFSET: usize;
>> >
>> > Does this need to be part of the trait? As an alternative the provided
>> > methods could be generated in the macro below and reduce the
>> > opportunity to implement this trait incorrectly.
>>
>> There is no risk of implementing the trait wrong, because it is usually
>> derived by a macro.
>
> There's no risk when it's implemented by the macro, but you used the
> word usually,  which means there is a risk.
>
>> We need at least one of the methods to be able to have the type system
>> verify that the type for which we implement `HasHrTImer` actually has a
>> field with the name we specify, and that this field has the right type.
>> And to have that, we need the OFFSET.
>
> I don't follow this logic. OFFSET is calculated in the body of the
> macro. I'm suggesting that the macro generate the method
> implementations (which would no longer be provided). In effect I'm
> saying: keep OFFSET private.
>
> I'm also noticing now that the macro generates an implementation of
> raw_get_timer *in addition to* the provided implementation. Why are
> both needed?

HasHrTimer is unsafe, because it would be unsound to implement, if the
type it is implemented on does not have a `Timer` at the specified
offset.

To be able to implement it safely with a macro, the macro must verify
that the type we implement the trait on satisfies the safety
requirement. That is, we have to have the macro verify that the type
indeed has a field of type `Timer` with the given name. If that is the
case, the macro can calculate OFFSET.

The way we achieve this is we re-implement on of the trait methods in
such a way that it only compiles if the type we reimplement trait
on actually have the field of the right type.

I want to generate as little code as possible in the macro, and I would
rather rely on the default implementations given in the trait, than have
the macro generate implementations for all the methods. Generated code
are more difficult to reason about.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

> On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Tamir Duberstein" <tamird@gmail.com> writes:
>>
>> > On Tue, Feb 18, 2025 at 8:29 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >>
>>
>> [...]
>>
>> >> +//! ## State Diagram
>> >> +//!
>> >> +//! ```text
>> >> +//!                  <-- Stop ----
>> >> +//!                  <-- Cancel --
>> >> +//!                  --- Start -->
>> >> +//!        +---------+        +---------+
>> >> +//!   O--->| Stopped |        | Running |---o
>> >> +//!        +---------+        +---------+   |
>> >> +//!                                  ^      |
>> >> +//!                  <- Expire --    |      |
>> >> +//!                                  o------o
>> >> +//!                                   Restart
>> >> +//! ```
>> >> +//!
>> >> +//! A timer is initialized in the **stopped** state. A stopped timer can be
>> >> +//! **started** with an **expiry** time. After the timer is started, it is
>> >> +//! **running**. When the timer **expires**, the timer handler is executed.
>> >> +//! After the handler has executed, the timer may be **restarted** or
>> >> +//! **stopped**. A running timer can be **canceled** before it's handler is
>> >
>> > "it's" = it is. This should be "its" (possessive).
>>
>> Thanks 👍
>>
>> > Just to be clear, after the handler has executed and before the timer
>> > has been **restarted** or **stopped** the timer is still in the
>> > **running** state?
>>
>> It depends on the return value of the handler. If the handler returns restart,
>> the timer the timer does not enter the stopped state. If the handler
>> returns stop, the timer enters the stopped state.
>>
>> The timer is still considered to be in running state the handler is
>> running.
>>
>> I can add this info to the section.
>
> Yeah, some clarification here would be useful.

I'll add a paragraph 👍

[...]

>> >> +    /// Get a pointer to the contained `bindings::hrtimer`.
>> >> +    ///
>> >> +    /// # Safety
>> >> +    ///
>> >> +    /// `ptr` must point to a live allocation of at least the size of `Self`.
>> >> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
>> >> +        // SAFETY: The field projection to `timer` does not go out of bounds,
>> >> +        // because the caller of this function promises that `ptr` points to an
>> >> +        // allocation of at least the size of `Self`.
>> >> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
>> >> +    }
>> >
>> > Can you help me understand why the various functions here operate on
>> > *const Self? I understand the need to obtain a C pointer to interact
>> > with bindings, but I don't understand why we're dealing in raw
>> > pointers to the abstraction rather than references.
>>
>> We cannot reference the `bindings::hrtimer` without wrapping it in
>> `Opaque`. This would be the primary reason. At other times, we cannot
>> produce references because we might not be able to prove that we satisfy
>> the safety requirements for turning a pointer into a reference. If we
>> are just doing offset arithmetic anyway, we don't need a reference.
>
> Why do we have a pointer, rather than a reference, to Self in the
> first place? I think this is the key thing I don't understand.

Perhaps it makes more sense if you look at the context. One of the entry
points to `HrTimer::raw_get` is via `<ArcHrTimerHandle as
HrTimerHandle>::cancel`. This user facing method takes `&mut self`. The
handle contains an arc to a type that contains a `Timer` and implements
`HasHrTImer`. To get to the timer, we need to do pointer manipulation.
We only know how to get the `Timer` field via the `OFFSET`. The natural
return value from the offset operation is a raw pointer. Rather than
convert back to a reference, we stay in pointer land when we call
`HrTimer::raw_cancel`, because we need a pointer to the
`bindings::hrtimer` anyway, not a reference.

>
>>
>>
>> > This extends to
>> > HrTimerPointer, which is intended to be implemented by *pointers to*
>> > structs that embed `HrTimer`; why isn't it implemented on by the
>> > embedder itself?
>>
>> Not sure what you mean here. If you refer to for instance the
>> implementation of `HrTimerPointer for Arc<T>`, I get why you might
>> wonder, why does `HasHrTimer::start` not take a reference instead of a
>> pointer? We could do that, but we would just immediately break it down
>> again in the implementation of `HasHrTimer::start`. Might still be a
>> good idea though.
>
> I was trying to say that my question (which I clarified above,
> hopefully) extends to the description and name of this trait.
> Specifically for this trait I don't understand why its semantics are
> described in terms of pointers rather than references (and AsRef, to
> allow for Arc and friends).

All user facing APIs use references, not pointers. The raw pointer
interfaces are for internal use only. I don't think we would gain
anything from using `AsRef` internally. Perhaps you could clarify a bit more?

>
>> >
>> > I realize we discussed this on v6, sorry for not keeping up there.
>>
>> No worries, it is good that we discuss this.
>>
>> [...]
>>
>> >> +
>> >> +/// A handle representing a potentially running timer.
>> >> +///
>> >> +/// More than one handle representing the same timer might exist.
>> >> +///
>> >> +/// # Safety
>> >> +///
>> >> +/// When dropped, the timer represented by this handle must be cancelled, if it
>> >> +/// is running. If the timer handler is running when the handle is dropped, the
>> >> +/// drop method must wait for the handler to finish before returning.
>> >
>> > Between this comment and the comment on cancel we say "if it is
>> > running" 3 times. Can we say it just once, on the method, and here say
>> > that cancel must be called in Drop?
>>
>> Well, the comment on `cancel` is just a description of what the function
>> does. This piece of text is a safety requirement.
>>
>> We could make the safety requirement for implementing the trait "Implement
>> the methods according to their documentation". But that would not help with
>> the drop requirement.
>
> My suggestion is that the safety comment read: when dropped,
> [Self::cancel] must be called. Something like that.

We don't care how the timer is canceled, it just has to be canceled. It
does not have to be by calling `Self::cancel`.


Best regards,
Andreas Hindborg

Re: [PATCH v8 00/14] hrtimer Rust API

[Andreas Hindborg]

"Frederic Weisbecker" <frederic@kernel.org> writes:

> Le Wed, Feb 19, 2025 at 12:02:50PM +0100, Andreas Hindborg a écrit :
>> "Andreas Hindborg" <a.hindborg@kernel.org> writes:
>>
>> > This series adds support for using the `hrtimer` subsystem from Rust code.
>> >
>> > The series adds support for timer mode and clock source configuration during
>> > timer initialization. Examples and functionality to execute closures at timer
>> > expiration has been removed, as these depend on either atomics [3] or
>> > `SpinLockIrq` [4], which are still being worked on.
>> >
>> > This series is a dependency for unmerged features of the Rust null block driver
>> > [1], and for rkvms [2].
>> >
>>
>> @ timer subsystem maintainers: did you discuss how you want to set up
>> maintenance for this yet? As mentioned, I'm happy stepping up to
>> maintain this, but if you want to handle it with existing resources that
>> is perfectly fine as well.
>
> You're the best candidate to maintain this code since you wrote it :-)
>
> Also I personally have near zero skills in Rust as of today so all I can do
> is to vaguely keep an eye on the binding's interface and keep in touch
> with the changes.
>
> So I suggest you to add a new entry with you as a maintainer (I suggested
> something similar to Fujita for some other timer related things) but please
> keep us Cc'ed for future changes.

Alright, lets do that.

Do you want to pick future changes to this directly form list or would
you prefer that I send you a PR with changes?

We are probably going to have a new iteration anyway, as discussion
picked up again.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

> On 18.02.25 14:27, Andreas Hindborg wrote:
>> This patch adds support for intrusive use of the hrtimer system. For now,
>> only one timer can be embedded in a Rust struct.
>>
>> The hrtimer Rust API is based on the intrusive style pattern introduced by
>> the Rust workqueue API.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/time.rs         |   2 +
>>  rust/kernel/time/hrtimer.rs | 312 ++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 314 insertions(+)
>>
>> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
>> index 87e47f2f5618d..2cf365cfb412e 100644
>> --- a/rust/kernel/time.rs
>> +++ b/rust/kernel/time.rs
>> @@ -10,6 +10,8 @@
>>
>>  use core::convert::Into;
>>
>> +pub mod hrtimer;
>> +
>>  /// The number of nanoseconds per millisecond.
>>  pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
>>
>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>> new file mode 100644
>> index 0000000000000..a6332924efabd
>> --- /dev/null
>> +++ b/rust/kernel/time/hrtimer.rs
>> @@ -0,0 +1,312 @@
>> +// 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
>> +//! * Running
>> +//!
>> +//! Operations:
>> +//!
>> +//! * Start
>> +//! * Cancel
>> +//! * Stop
>> +//! * Restart
>> +//!
>> +//! Events:
>> +//!
>> +//! * Expire
>> +//!
>> +//! ## State Diagram
>> +//!
>> +//! ```text
>> +//!                  <-- Stop ----
>> +//!                  <-- Cancel --
>> +//!                  --- Start -->
>> +//!        +---------+        +---------+
>> +//!   O--->| Stopped |        | Running |---o
>> +//!        +---------+        +---------+   |
>> +//!                                  ^      |
>> +//!                  <- Expire --    |      |
>> +//!                                  o------o
>> +//!                                   Restart
>> +//! ```
>> +//!
>> +//! A timer is initialized in the **stopped** state. A stopped timer can be
>> +//! **started** with an **expiry** time. After the timer is started, it is
>> +//! **running**. When the timer **expires**, the timer handler is executed.
>> +//! After the handler has executed, the timer may be **restarted** or
>> +//! **stopped**. A running timer can be **canceled** before it's handler is
>
> This confuses me a bit, in the other thread you wrote that the handler
> decides if the timer should restart or be stopped. But What happens when
> I call `cancel` on the `HrTimerHandle` while the handler is running, but
> finishes shortly after with a restart request? Will it be canceled?

Tamir had a somewhat similar question and Is added the following


//! A timer is still considered to be running when the handler is executing. The
//! return value of the handler determines if the timer will stop or restart
//! when the handler finishes executing.
//!

If a timer is canceled while the handler is running, the call to cancel
will block. After the handler has finished executing and the timer has
been restarted or stopped, the cancel call will unblock. If at the timer
is stopped cancel has no effect. If the timer was restarted because of
the handler return value, it will be canceled.

I'll add a note on that as well.


> I also have a bit of a wording issue with "the timer is running" IIUC,
> this means that the timer subsystem keeps track of the expiry time and
> when the time is elapsed, it fires the code that you registered prior.
> At first, I thought that "the timer is running" meant that the
> registered code is running. Maybe we should have two different terms for
> that? I personally would prefer "active timer" for "the timer subsystem
> is currently tracking the time and when it is elapsed, it will run the
> code" and "running timer" for "the timer's expiry time has elapsed and
> the timer callback is currently being executed".

I understand your confusion and I experienced a similar confusion when I
first dove in. Early iterations of this patch set used another
terminology, but C maintainers requested I use the terminology used in C
hrtimer subsystem. Which of course is the right thing to do.

And so in this terminology a timer is `running` when it is armed and
will run the handler when expiration occurs.

I tried to explain with the paragraph under the diagram, but perhaps it
is not enough. I can try to expand it further.

>
>> +//! executed. A timer that is cancelled enters the **stopped** state.
>> +//!
>> +
>> +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 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 {
>> +            // INVARIANTS: We initialize `timer` with `hrtimer_setup` below.
>> +            timer <- Opaque::ffi_init(move |place: *mut bindings::hrtimer| {
>> +                // SAFETY: By design of `pin_init!`, `place` is a pointer 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::CallbackTarget::run),
>> +                        bindings::CLOCK_MONOTONIC as i32,
>> +                        bindings::hrtimer_mode_HRTIMER_MODE_REL,
>> +                    );
>> +                }
>> +            }),
>> +            _t: PhantomData,
>> +        })
>> +    }
>> +
>> +    /// Get a pointer to the contained `bindings::hrtimer`.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// `ptr` must point to a live allocation of at least the size of `Self`.
>> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
>> +        // SAFETY: The field projection to `timer` does not go out of bounds,
>> +        // because the caller of this function promises that `ptr` points to an
>> +        // allocation of at least the size of `Self`.
>> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
>> +    }
>> +
>> +    /// Cancel an initialized and potentially running timer.
>> +    ///
>> +    /// If the timer handler is running, this will block until the handler is
>> +    /// finished.
>> +    ///
>> +    /// Users of the `HrTimer` API would not usually call this method directly.
>> +    /// Instead they would use the safe `cancel` method on the [`HrTimerHandle`]
>
> Can you link to the `cancel` function?
>
>> +    /// returned when the timer was started.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// `self_ptr` must point to a valid `Self`.
>> +    #[allow(dead_code)]
>> +    pub(crate) unsafe fn raw_cancel(self_ptr: *const Self) -> bool {
>> +        // SAFETY: timer_ptr points to an allocation of at least `HrTimer` size.
>> +        let c_timer_ptr = unsafe { HrTimer::raw_get(self_ptr) };
>> +
>> +        // If the handler is running, this will wait for the handler to finish
>> +        // before returning.
>> +        // SAFETY: `c_timer_ptr` is initialized and valid. Synchronization is
>> +        // handled on C side.
>> +        unsafe { bindings::hrtimer_cancel(c_timer_ptr) != 0 }
>> +    }
>> +}
>> +
>> +/// Implemented by pointer types that point to structs that embed a [`HrTimer`].
>> +///
>> +/// Target (pointee) must be [`Sync`] because timer callbacks happen in another
>> +/// thread of execution (hard or soft interrupt context).
>> +///
>> +/// Starting a timer returns a [`HrTimerHandle`] that can be used to manipulate
>> +/// the timer. Note that it is OK to call the start function repeatedly, and
>> +/// that more than one [`HrTimerHandle`] associated with a [`HrTimerPointer`] may
>> +/// exist. A timer can be manipulated through any of the handles, and a handle
>> +/// may represent a cancelled timer.
>> +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.
>
> I don't understand this argument. The bounds in the other patches seem
> like they could easily be combined. Then this trait could be merged into
> the one above.

I initially wrote `HrTimerPointer` and `RawHrTimerCallback` as a single
trait. But I ran into problems when specifying trait bounds. I got some
loops in the type graph that the compiler could not figure out.

>
>> +pub trait RawHrTimerCallback {
>> +    /// Callback to be called from C when timer fires.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// Only to be called by C code in `hrtimer` subsystem. `ptr` must point to
>> +    /// the `bindings::hrtimer` structure that was used to start the timer.
>> +    unsafe extern "C" fn run(ptr: *mut bindings::hrtimer) -> bindings::hrtimer_restart;
>> +}
>> +
>> +/// Implemented by structs that can be the target of a timer callback.
>> +pub trait HrTimerCallback {
>> +    /// The type whose [`RawHrTimerCallback::run`] method will be invoked when
>> +    /// the timer expires.
>> +    type CallbackTarget<'a>: RawHrTimerCallback;
>> +
>> +    /// This type is passed to the timer callback function. It may be a borrow
>> +    /// of [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
>> +    /// implementation can guarantee exclusive access to the target during timer
>
> Technically "exclusive" access is correct if the `CallbackTarget` is
> `Pin<&Self>`, since you will get exclusive access to a `Pin<&Self>`, but
> it might confuse people, because there can be multiple `Pin<&Self>`. So
> I would just drop the word "exclusive" here.

Yes, maybe it should be "shared or exclusive access, depending on the type"?

>
>> +    /// handler execution.
>> +    type CallbackTargetParameter<'a>;
>
> Also why can't this type be an associated type of `HrTimerPointer`?
> Since this seems to always be constrained in the impls of
> `RawHrTimerCallback`.

That might be a nice improvement, I'll try that out.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

> On 18.02.25 14:27, Andreas Hindborg wrote:
>> +
>> +    /// Cancel an initialized and potentially running timer.
>> +    ///
>> +    /// If the timer handler is running, this will block until the handler is
>> +    /// finished.
>> +    ///
>> +    /// Users of the `HrTimer` API would not usually call this method directly.
>> +    /// Instead they would use the safe `cancel` method on the [`HrTimerHandle`]
>
> Can you link to the `cancel` function?

Yes 👍


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

> On 18.02.25 14:27, Andreas Hindborg wrote:
>> This patch allows timer handlers to report that they want a timer to be
>> restarted after the timer handler has finished executing.
>>
>> Also update the `hrtimer` documentation to showcase the new feature.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/time/hrtimer.rs     | 28 +++++++++++++++++++++++++++-
>>  rust/kernel/time/hrtimer/arc.rs |  4 +---
>>  2 files changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>> index b2a3130bc21d7..e342193f985eb 100644
>> --- a/rust/kernel/time/hrtimer.rs
>> +++ b/rust/kernel/time/hrtimer.rs
>> @@ -183,7 +183,7 @@ pub trait HrTimerCallback {
>>      type CallbackTargetParameter<'a>;
>>
>>      /// Called by the timer logic when the timer fires.
>> -    fn run(this: Self::CallbackTargetParameter<'_>)
>> +    fn run(this: Self::CallbackTargetParameter<'_>) -> HrTimerRestart
>>      where
>>          Self: Sized;
>>  }
>> @@ -278,6 +278,32 @@ unsafe fn start(self_ptr: *const Self, expires: Ktime) {
>>      }
>>  }
>>
>> +/// Restart policy for timers.
>> +pub enum HrTimerRestart {
>> +    /// Timer should not be restarted.
>> +    NoRestart,
>> +    /// Timer should be restarted.
>> +    Restart,
>> +}
>> +
>> +impl From<bindings::hrtimer_restart> for HrTimerRestart {
>> +    fn from(value: u32) -> Self {
>> +        match value {
>> +            bindings::hrtimer_restart_HRTIMER_NORESTART => Self::NoRestart,
>> +            _ => Self::Restart,
>> +        }
>> +    }
>> +}
>> +
>> +impl From<HrTimerRestart> for bindings::hrtimer_restart {
>> +    fn from(value: HrTimerRestart) -> Self {
>> +        match value {
>> +            HrTimerRestart::NoRestart => bindings::hrtimer_restart_HRTIMER_NORESTART,
>> +            HrTimerRestart::Restart => bindings::hrtimer_restart_HRTIMER_RESTART,
>> +        }
>> +    }
>> +}
>
> Do drivers need these impls? If no, I would prefer a private conversion
> function just for the abstraction.

No, drivers would not need these. I can hide them.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

> On 18.02.25 14:27, Andreas Hindborg wrote:
>> Add an associated function to convert a `Box<T>` into a `Pin<Box<T>>`.
>
> Why don't you use `into()` directly where you need it? Do you want the
> function call to be more descriptive?
> (To be clear, I'm not against the addition, just wanting to check if our
> motivation is the same)

Yes, I prefer the explicit name in a line like this:

        // 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) }));


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

> On 18.02.25 14:27, Andreas Hindborg wrote:
>> +/// Operational mode of [`HrTimer`].
>> +#[derive(Clone, Copy)]
>> +pub enum HrTimerMode {
>> +    /// Timer expires at the given expiration time.
>> +    Absolute,
>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>> +    Relative,
>> +    /// Timer does not move between CPU cores.
>> +    Pinned,
>> +    /// Timer handler is executed in soft irq context.
>> +    Soft,
>> +    /// Timer handler is executed in hard irq context.
>> +    Hard,
>> +    /// Timer expires at the given expiration time.
>> +    /// Timer does not move between CPU cores.
>> +    AbsolutePinned,
>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>> +    /// Timer does not move between CPU cores.
>> +    RelativePinned,
>> +    /// Timer expires at the given expiration time.
>> +    /// Timer handler is executed in soft irq context.
>> +    AbsoluteSoft,
>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>> +    /// Timer handler is executed in soft irq context.
>> +    RelativeSoft,
>> +    /// Timer expires at the given expiration time.
>> +    /// Timer does not move between CPU cores.
>> +    /// Timer handler is executed in soft irq context.
>> +    AbsolutePinnedSoft,
>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>> +    /// Timer does not move between CPU cores.
>> +    /// Timer handler is executed in soft irq context.
>> +    RelativePinnedSoft,
>> +    /// Timer expires at the given expiration time.
>> +    /// Timer handler is executed in hard irq context.
>> +    AbsoluteHard,
>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>> +    /// Timer handler is executed in hard irq context.
>> +    RelativeHard,
>> +    /// Timer expires at the given expiration time.
>> +    /// Timer does not move between CPU cores.
>> +    /// Timer handler is executed in hard irq context.
>> +    AbsolutePinnedHard,
>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>> +    /// Timer does not move between CPU cores.
>> +    /// Timer handler is executed in hard irq context.
>> +    RelativePinnedHard,
>> +}
>
> At some point we probably want to move this to bitfields, or do you
> think it's better to keep it like this?

Yes, eventually the would transition. The main difficulty is that not
all flag combinations are legal, and the zero value is also a flag.
There was some promising work being shared on Zulip for this [1], but I
don't think it is completed yet. Added Timothy to CC.

[1] https://rust-for-linux.zulipchat.com/#narrow/channel/291565-Help/topic/Best.20way.20to.20handle.20enum.2Fflags.20situation

>> +
>> +impl From<HrTimerMode> for bindings::hrtimer_mode {
>> +    fn from(value: HrTimerMode) -> Self {
>> +        use bindings::*;
>> +        match value {
>> +            HrTimerMode::Absolute => hrtimer_mode_HRTIMER_MODE_ABS,
>> +            HrTimerMode::Relative => hrtimer_mode_HRTIMER_MODE_REL,
>> +            HrTimerMode::Pinned => hrtimer_mode_HRTIMER_MODE_PINNED,
>> +            HrTimerMode::Soft => hrtimer_mode_HRTIMER_MODE_SOFT,
>> +            HrTimerMode::Hard => hrtimer_mode_HRTIMER_MODE_HARD,
>> +            HrTimerMode::AbsolutePinned => hrtimer_mode_HRTIMER_MODE_ABS_PINNED,
>> +            HrTimerMode::RelativePinned => hrtimer_mode_HRTIMER_MODE_REL_PINNED,
>> +            HrTimerMode::AbsoluteSoft => hrtimer_mode_HRTIMER_MODE_ABS_SOFT,
>> +            HrTimerMode::RelativeSoft => hrtimer_mode_HRTIMER_MODE_REL_SOFT,
>> +            HrTimerMode::AbsolutePinnedSoft => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_SOFT,
>> +            HrTimerMode::RelativePinnedSoft => hrtimer_mode_HRTIMER_MODE_REL_PINNED_SOFT,
>> +            HrTimerMode::AbsoluteHard => hrtimer_mode_HRTIMER_MODE_ABS_HARD,
>> +            HrTimerMode::RelativeHard => hrtimer_mode_HRTIMER_MODE_REL_HARD,
>> +            HrTimerMode::AbsolutePinnedHard => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_HARD,
>> +            HrTimerMode::RelativePinnedHard => hrtimer_mode_HRTIMER_MODE_REL_PINNED_HARD,
>> +        }
>> +    }
>> +}
>> +
>> +impl From<HrTimerMode> for u64 {
>> +    fn from(value: HrTimerMode) -> Self {
>> +        Into::<bindings::hrtimer_mode>::into(value) as u64
>> +    }
>> +}
>
> Hmm do drivers really need these impls? If not, then you could also just
> have a private function that does the conversion and use it only in the
> abstraction layer.

Similar to the other impls you commented on, I can move them private. I
would prefer using `From` rather than some other function.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

> On 18.02.25 14:27, Andreas Hindborg wrote:
>> Allow selecting a clock source for timers by passing a `ClockSource`
>> variant to `HrTimer::new`.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>>  rust/kernel/time/hrtimer.rs | 52 +++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 50 insertions(+), 2 deletions(-)
>>
>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>> index db49061f830c3..2b46d66eaa313 100644
>> --- a/rust/kernel/time/hrtimer.rs
>> +++ b/rust/kernel/time/hrtimer.rs
>> @@ -73,7 +73,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: ClockSource) -> impl PinInit<Self>
>>      where
>>          T: HrTimerCallback,
>>      {
>> @@ -87,7 +87,7 @@ pub fn new(mode: HrTimerMode) -> impl PinInit<Self>
>>                      bindings::hrtimer_setup(
>>                          place,
>>                          Some(T::CallbackTarget::run),
>> -                        bindings::CLOCK_MONOTONIC as i32,
>> +                        clock.into(),
>>                          mode.into(),
>>                      );
>>                  }
>> @@ -448,6 +448,54 @@ fn from(value: HrTimerMode) -> Self {
>>      }
>>  }
>>
>> +/// The clock source to use for a [`HrTimer`].
>> +pub enum ClockSource {
>> +    /// A settable system-wide clock that measures real (i.e., wall-clock) time.
>
> Missing newline here to separate the short one-line description and the
> rest of the docs. (also below)

Right. It is copy pasta from the C code, but I guess we can add some newlines.

>
>> +    /// Setting this clock requires appropriate privileges. This clock is
>> +    /// affected by discontinuous jumps in the system time (e.g., if the system
>> +    /// administrator manually changes the clock), and by frequency adjustments
>> +    /// performed by NTP and similar applications via adjtime(3), adjtimex(2),
>> +    /// clock_adjtime(2), and ntp_adjtime(3). This clock normally counts the
>> +    /// number of seconds since 1970-01-01 00:00:00 Coordinated Universal Time
>> +    /// (UTC) except that it ignores leap seconds; near a leap second it is
>> +    /// typically adjusted by NTP to stay roughly in sync with UTC.
>
> Thanks for the extensive descriptions of the various clock sources!

Just FYI, I pulled these from other documentation sources, I didn't
author all of this.

>
>> +    RealTime,
>> +    /// A nonsettable system-wide clock that represents monotonic time since—as
>> +    /// described by POSIX—"some unspecified point in the past" On Linux, that
>> +    /// point corresponds to the number of seconds that the system has been
>> +    /// running since it was booted.
>> +    ///
>> +    /// The CLOCK_MONOTONIC clock is not affected by discontinuous jumps in the
>> +    /// system time (e.g., if the system administrator manually changes the
>> +    /// clock), but is affected by frequency adjustments. This clock does not
>> +    /// count time that the system is suspended.
>> +    Monotonic,
>> +    /// A nonsettable system-wide clock that is identical to CLOCK_MONOTONIC,
>> +    /// except that it also includes any time that the system is suspended. This
>> +    /// allows applications to get a suspend-aware monotonic clock without
>> +    /// having to deal with the complications of CLOCK_REALTIME, which may have
>> +    /// discontinuities if the time is changed using settimeofday(2) or similar.
>> +    BootTime,
>> +    /// A nonsettable system-wide clock derived from wall-clock time but
>> +    /// counting leap seconds. This clock does not experience discontinuities or
>> +    /// frequency adjustments caused by inserting leap seconds as CLOCK_REALTIME
>> +    /// does.
>> +    ///
>> +    /// The acronym TAI refers to International Atomic Time.
>
> In that case, I would expect the acronym to be `IAT`.

Yea, this one is funny. The abbreviation apparently is French [1] and
TAI is the correct and agreed upon (beyond Linux) abbreviation to use.

[1] https://en.wikipedia.org/wiki/International_Atomic_Time

>
>> +    TAI,
>> +}
>> +
>> +impl From<ClockSource> for bindings::clockid_t {
>> +    fn from(value: ClockSource) -> Self {
>> +        match value {
>> +            ClockSource::RealTime => bindings::CLOCK_REALTIME as i32,
>> +            ClockSource::Monotonic => bindings::CLOCK_MONOTONIC as i32,
>> +            ClockSource::BootTime => bindings::CLOCK_BOOTTIME as i32,
>> +            ClockSource::TAI => bindings::CLOCK_TAI as i32,
>> +        }
>> +    }
>> +}
>
> Same question here as for the `HrTimerMode`, do drivers need this impl?
> If not, then I think a private conversion function is a better fit.

I will hide it.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

Andreas Hindborg <a.hindborg@kernel.org> writes:

> "Benno Lossin" <benno.lossin@proton.me> writes:
>
>> On 18.02.25 14:27, Andreas Hindborg wrote:
>>> +pub trait HrTimerCallback {
>>> +    /// The type whose [`RawHrTimerCallback::run`] method will be invoked when
>>> +    /// the timer expires.
>>> +    type CallbackTarget<'a>: RawHrTimerCallback;
>>> +
>>> +    /// This type is passed to the timer callback function. It may be a borrow
>>> +    /// of [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
>>> +    /// implementation can guarantee exclusive access to the target during timer
>>
>> Technically "exclusive" access is correct if the `CallbackTarget` is
>> `Pin<&Self>`, since you will get exclusive access to a `Pin<&Self>`, but
>> it might confuse people, because there can be multiple `Pin<&Self>`. So
>> I would just drop the word "exclusive" here.
>
> Yes, maybe it should be "shared or exclusive access, depending on the type"?
>
>>
>>> +    /// handler execution.
>>> +    type CallbackTargetParameter<'a>;
>>
>> Also why can't this type be an associated type of `HrTimerPointer`?
>> Since this seems to always be constrained in the impls of
>> `RawHrTimerCallback`.
>
> That might be a nice improvement, I'll try that out.

Looking closer at this, I don't see how to achieve this. We need access
to the type here, because it is used in the signature of `run`.
`HrTimerCallback` has no bounds on it, and that is nice. If we want to
move these associated types, we have to introduce a bound here.

We need to be generic over the type of the parameter to `run`, and by
the time the user implements this trait, the type must be known and so
the user has to specify somehow.


Best regards,
Andreas Hindborg

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

[Benno Lossin]

On 21.02.25 10:09, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
> 
>> On 18.02.25 14:27, Andreas Hindborg wrote:
>>> This patch allows timer handlers to report that they want a timer to be
>>> restarted after the timer handler has finished executing.
>>>
>>> Also update the `hrtimer` documentation to showcase the new feature.
>>>
>>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>> ---
>>>  rust/kernel/time/hrtimer.rs     | 28 +++++++++++++++++++++++++++-
>>>  rust/kernel/time/hrtimer/arc.rs |  4 +---
>>>  2 files changed, 28 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>>> index b2a3130bc21d7..e342193f985eb 100644
>>> --- a/rust/kernel/time/hrtimer.rs
>>> +++ b/rust/kernel/time/hrtimer.rs
>>> @@ -183,7 +183,7 @@ pub trait HrTimerCallback {
>>>      type CallbackTargetParameter<'a>;
>>>
>>>      /// Called by the timer logic when the timer fires.
>>> -    fn run(this: Self::CallbackTargetParameter<'_>)
>>> +    fn run(this: Self::CallbackTargetParameter<'_>) -> HrTimerRestart
>>>      where
>>>          Self: Sized;
>>>  }
>>> @@ -278,6 +278,32 @@ unsafe fn start(self_ptr: *const Self, expires: Ktime) {
>>>      }
>>>  }
>>>
>>> +/// Restart policy for timers.
>>> +pub enum HrTimerRestart {
>>> +    /// Timer should not be restarted.
>>> +    NoRestart,
>>> +    /// Timer should be restarted.
>>> +    Restart,
>>> +}
>>> +
>>> +impl From<bindings::hrtimer_restart> for HrTimerRestart {
>>> +    fn from(value: u32) -> Self {
>>> +        match value {
>>> +            bindings::hrtimer_restart_HRTIMER_NORESTART => Self::NoRestart,
>>> +            _ => Self::Restart,
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +impl From<HrTimerRestart> for bindings::hrtimer_restart {
>>> +    fn from(value: HrTimerRestart) -> Self {
>>> +        match value {
>>> +            HrTimerRestart::NoRestart => bindings::hrtimer_restart_HRTIMER_NORESTART,
>>> +            HrTimerRestart::Restart => bindings::hrtimer_restart_HRTIMER_RESTART,
>>> +        }
>>> +    }
>>> +}
>>
>> Do drivers need these impls? If no, I would prefer a private conversion
>> function just for the abstraction.
> 
> No, drivers would not need these. I can hide them.

Sounds good, you can add:

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

---
Cheers,
Benno

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

[Benno Lossin]

On 21.02.25 10:17, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
> 
>> On 18.02.25 14:27, Andreas Hindborg wrote:
>>> +/// Operational mode of [`HrTimer`].
>>> +#[derive(Clone, Copy)]
>>> +pub enum HrTimerMode {
>>> +    /// Timer expires at the given expiration time.
>>> +    Absolute,
>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>> +    Relative,
>>> +    /// Timer does not move between CPU cores.
>>> +    Pinned,
>>> +    /// Timer handler is executed in soft irq context.
>>> +    Soft,
>>> +    /// Timer handler is executed in hard irq context.
>>> +    Hard,
>>> +    /// Timer expires at the given expiration time.
>>> +    /// Timer does not move between CPU cores.
>>> +    AbsolutePinned,
>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>> +    /// Timer does not move between CPU cores.
>>> +    RelativePinned,
>>> +    /// Timer expires at the given expiration time.
>>> +    /// Timer handler is executed in soft irq context.
>>> +    AbsoluteSoft,
>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>> +    /// Timer handler is executed in soft irq context.
>>> +    RelativeSoft,
>>> +    /// Timer expires at the given expiration time.
>>> +    /// Timer does not move between CPU cores.
>>> +    /// Timer handler is executed in soft irq context.
>>> +    AbsolutePinnedSoft,
>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>> +    /// Timer does not move between CPU cores.
>>> +    /// Timer handler is executed in soft irq context.
>>> +    RelativePinnedSoft,
>>> +    /// Timer expires at the given expiration time.
>>> +    /// Timer handler is executed in hard irq context.
>>> +    AbsoluteHard,
>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>> +    /// Timer handler is executed in hard irq context.
>>> +    RelativeHard,
>>> +    /// Timer expires at the given expiration time.
>>> +    /// Timer does not move between CPU cores.
>>> +    /// Timer handler is executed in hard irq context.
>>> +    AbsolutePinnedHard,
>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>> +    /// Timer does not move between CPU cores.
>>> +    /// Timer handler is executed in hard irq context.
>>> +    RelativePinnedHard,
>>> +}
>>
>> At some point we probably want to move this to bitfields, or do you
>> think it's better to keep it like this?
> 
> Yes, eventually the would transition. The main difficulty is that not
> all flag combinations are legal, and the zero value is also a flag.
> There was some promising work being shared on Zulip for this [1], but I
> don't think it is completed yet. Added Timothy to CC.
> 
> [1] https://rust-for-linux.zulipchat.com/#narrow/channel/291565-Help/topic/Best.20way.20to.20handle.20enum.2Fflags.20situation

Ah yeah I remember that. And also the complication about certain
combinations not being allowed.

>>> +
>>> +impl From<HrTimerMode> for bindings::hrtimer_mode {
>>> +    fn from(value: HrTimerMode) -> Self {
>>> +        use bindings::*;
>>> +        match value {
>>> +            HrTimerMode::Absolute => hrtimer_mode_HRTIMER_MODE_ABS,
>>> +            HrTimerMode::Relative => hrtimer_mode_HRTIMER_MODE_REL,
>>> +            HrTimerMode::Pinned => hrtimer_mode_HRTIMER_MODE_PINNED,
>>> +            HrTimerMode::Soft => hrtimer_mode_HRTIMER_MODE_SOFT,
>>> +            HrTimerMode::Hard => hrtimer_mode_HRTIMER_MODE_HARD,
>>> +            HrTimerMode::AbsolutePinned => hrtimer_mode_HRTIMER_MODE_ABS_PINNED,
>>> +            HrTimerMode::RelativePinned => hrtimer_mode_HRTIMER_MODE_REL_PINNED,
>>> +            HrTimerMode::AbsoluteSoft => hrtimer_mode_HRTIMER_MODE_ABS_SOFT,
>>> +            HrTimerMode::RelativeSoft => hrtimer_mode_HRTIMER_MODE_REL_SOFT,
>>> +            HrTimerMode::AbsolutePinnedSoft => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_SOFT,
>>> +            HrTimerMode::RelativePinnedSoft => hrtimer_mode_HRTIMER_MODE_REL_PINNED_SOFT,
>>> +            HrTimerMode::AbsoluteHard => hrtimer_mode_HRTIMER_MODE_ABS_HARD,
>>> +            HrTimerMode::RelativeHard => hrtimer_mode_HRTIMER_MODE_REL_HARD,
>>> +            HrTimerMode::AbsolutePinnedHard => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_HARD,
>>> +            HrTimerMode::RelativePinnedHard => hrtimer_mode_HRTIMER_MODE_REL_PINNED_HARD,
>>> +        }
>>> +    }
>>> +}
>>> +
>>> +impl From<HrTimerMode> for u64 {
>>> +    fn from(value: HrTimerMode) -> Self {
>>> +        Into::<bindings::hrtimer_mode>::into(value) as u64
>>> +    }
>>> +}
>>
>> Hmm do drivers really need these impls? If not, then you could also just
>> have a private function that does the conversion and use it only in the
>> abstraction layer.
> 
> Similar to the other impls you commented on, I can move them private. I
> would prefer using `From` rather than some other function.

What's the reason for you preferring `From`? I don't think it's
important to forbid access from the drivers, but if it's unnecessary,
why would we give them access in the first place?

---
Cheers,
Benno

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

[Benno Lossin]

On 21.02.25 10:29, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
> 
>> On 18.02.25 14:27, Andreas Hindborg wrote:
>>> Allow selecting a clock source for timers by passing a `ClockSource`
>>> variant to `HrTimer::new`.
>>>
>>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>>> ---
>>>  rust/kernel/time/hrtimer.rs | 52 +++++++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 50 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>>> index db49061f830c3..2b46d66eaa313 100644
>>> --- a/rust/kernel/time/hrtimer.rs
>>> +++ b/rust/kernel/time/hrtimer.rs
>>> @@ -73,7 +73,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: ClockSource) -> impl PinInit<Self>
>>>      where
>>>          T: HrTimerCallback,
>>>      {
>>> @@ -87,7 +87,7 @@ pub fn new(mode: HrTimerMode) -> impl PinInit<Self>
>>>                      bindings::hrtimer_setup(
>>>                          place,
>>>                          Some(T::CallbackTarget::run),
>>> -                        bindings::CLOCK_MONOTONIC as i32,
>>> +                        clock.into(),
>>>                          mode.into(),
>>>                      );
>>>                  }
>>> @@ -448,6 +448,54 @@ fn from(value: HrTimerMode) -> Self {
>>>      }
>>>  }
>>>
>>> +/// The clock source to use for a [`HrTimer`].
>>> +pub enum ClockSource {
>>> +    /// A settable system-wide clock that measures real (i.e., wall-clock) time.
>>
>> Missing newline here to separate the short one-line description and the
>> rest of the docs. (also below)
> 
> Right. It is copy pasta from the C code, but I guess we can add some newlines.
> 
>>
>>> +    /// Setting this clock requires appropriate privileges. This clock is
>>> +    /// affected by discontinuous jumps in the system time (e.g., if the system
>>> +    /// administrator manually changes the clock), and by frequency adjustments
>>> +    /// performed by NTP and similar applications via adjtime(3), adjtimex(2),
>>> +    /// clock_adjtime(2), and ntp_adjtime(3). This clock normally counts the
>>> +    /// number of seconds since 1970-01-01 00:00:00 Coordinated Universal Time
>>> +    /// (UTC) except that it ignores leap seconds; near a leap second it is
>>> +    /// typically adjusted by NTP to stay roughly in sync with UTC.
>>
>> Thanks for the extensive descriptions of the various clock sources!
> 
> Just FYI, I pulled these from other documentation sources, I didn't
> author all of this.

I still find it useful for you to copy these, I've seen a patch in the
past where the C documentation gave more information than the Rust
abstraction.

>>> +    RealTime,
>>> +    /// A nonsettable system-wide clock that represents monotonic time since—as
>>> +    /// described by POSIX—"some unspecified point in the past" On Linux, that
>>> +    /// point corresponds to the number of seconds that the system has been
>>> +    /// running since it was booted.
>>> +    ///
>>> +    /// The CLOCK_MONOTONIC clock is not affected by discontinuous jumps in the
>>> +    /// system time (e.g., if the system administrator manually changes the
>>> +    /// clock), but is affected by frequency adjustments. This clock does not
>>> +    /// count time that the system is suspended.
>>> +    Monotonic,
>>> +    /// A nonsettable system-wide clock that is identical to CLOCK_MONOTONIC,
>>> +    /// except that it also includes any time that the system is suspended. This
>>> +    /// allows applications to get a suspend-aware monotonic clock without
>>> +    /// having to deal with the complications of CLOCK_REALTIME, which may have
>>> +    /// discontinuities if the time is changed using settimeofday(2) or similar.
>>> +    BootTime,
>>> +    /// A nonsettable system-wide clock derived from wall-clock time but
>>> +    /// counting leap seconds. This clock does not experience discontinuities or
>>> +    /// frequency adjustments caused by inserting leap seconds as CLOCK_REALTIME
>>> +    /// does.
>>> +    ///
>>> +    /// The acronym TAI refers to International Atomic Time.
>>
>> In that case, I would expect the acronym to be `IAT`.
> 
> Yea, this one is funny. The abbreviation apparently is French [1] and
> TAI is the correct and agreed upon (beyond Linux) abbreviation to use.

What a world we have... :)

---
Cheers,
Benno

> [1] https://en.wikipedia.org/wiki/International_Atomic_Time
> 
>>
>>> +    TAI,
>>> +}
>>> +
>>> +impl From<ClockSource> for bindings::clockid_t {
>>> +    fn from(value: ClockSource) -> Self {
>>> +        match value {
>>> +            ClockSource::RealTime => bindings::CLOCK_REALTIME as i32,
>>> +            ClockSource::Monotonic => bindings::CLOCK_MONOTONIC as i32,
>>> +            ClockSource::BootTime => bindings::CLOCK_BOOTTIME as i32,
>>> +            ClockSource::TAI => bindings::CLOCK_TAI as i32,
>>> +        }
>>> +    }
>>> +}
>>
>> Same question here as for the `HrTimerMode`, do drivers need this impl?
>> If not, then I think a private conversion function is a better fit.
> 
> I will hide it.
> 
> 
> Best regards,
> Andreas Hindborg
> 
> 

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

[Benno Lossin]

On 21.02.25 11:15, Andreas Hindborg wrote:
> Andreas Hindborg <a.hindborg@kernel.org> writes:
> 
>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>
>>> On 18.02.25 14:27, Andreas Hindborg wrote:
>>>> +pub trait HrTimerCallback {
>>>> +    /// The type whose [`RawHrTimerCallback::run`] method will be invoked when
>>>> +    /// the timer expires.
>>>> +    type CallbackTarget<'a>: RawHrTimerCallback;
>>>> +
>>>> +    /// This type is passed to the timer callback function. It may be a borrow
>>>> +    /// of [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
>>>> +    /// implementation can guarantee exclusive access to the target during timer
>>>
>>> Technically "exclusive" access is correct if the `CallbackTarget` is
>>> `Pin<&Self>`, since you will get exclusive access to a `Pin<&Self>`, but
>>> it might confuse people, because there can be multiple `Pin<&Self>`. So
>>> I would just drop the word "exclusive" here.
>>
>> Yes, maybe it should be "shared or exclusive access, depending on the type"?
>>
>>>
>>>> +    /// handler execution.
>>>> +    type CallbackTargetParameter<'a>;
>>>
>>> Also why can't this type be an associated type of `HrTimerPointer`?
>>> Since this seems to always be constrained in the impls of
>>> `RawHrTimerCallback`.
>>
>> That might be a nice improvement, I'll try that out.
> 
> Looking closer at this, I don't see how to achieve this. We need access
> to the type here, because it is used in the signature of `run`.
> `HrTimerCallback` has no bounds on it, and that is nice. If we want to
> move these associated types, we have to introduce a bound here.
> 
> We need to be generic over the type of the parameter to `run`, and by
> the time the user implements this trait, the type must be known and so
> the user has to specify somehow.

I think if you put the associated type on the `RawHrTimerCallback`
trait, it should work.

---
Cheers,
Benno

Re: [PATCH v8 00/14] hrtimer Rust API

[Frederic Weisbecker]

Le Fri, Feb 21, 2025 at 09:40:41AM +0100, Andreas Hindborg a écrit :
> "Frederic Weisbecker" <frederic@kernel.org> writes:
> 
> > Le Wed, Feb 19, 2025 at 12:02:50PM +0100, Andreas Hindborg a écrit :
> >> "Andreas Hindborg" <a.hindborg@kernel.org> writes:
> >>
> >> > This series adds support for using the `hrtimer` subsystem from Rust code.
> >> >
> >> > The series adds support for timer mode and clock source configuration during
> >> > timer initialization. Examples and functionality to execute closures at timer
> >> > expiration has been removed, as these depend on either atomics [3] or
> >> > `SpinLockIrq` [4], which are still being worked on.
> >> >
> >> > This series is a dependency for unmerged features of the Rust null block driver
> >> > [1], and for rkvms [2].
> >> >
> >>
> >> @ timer subsystem maintainers: did you discuss how you want to set up
> >> maintenance for this yet? As mentioned, I'm happy stepping up to
> >> maintain this, but if you want to handle it with existing resources that
> >> is perfectly fine as well.
> >
> > You're the best candidate to maintain this code since you wrote it :-)
> >
> > Also I personally have near zero skills in Rust as of today so all I can do
> > is to vaguely keep an eye on the binding's interface and keep in touch
> > with the changes.
> >
> > So I suggest you to add a new entry with you as a maintainer (I suggested
> > something similar to Fujita for some other timer related things) but please
> > keep us Cc'ed for future changes.
> 
> Alright, lets do that.
> 
> Do you want to pick future changes to this directly form list or would
> you prefer that I send you a PR with changes?

I was thinking the patchset would be better routed towards the Rust tree?

How do you guys proceed usually with bindings tree maintainance?

I read yesterday Linus saying that Rust bindings are users of existing
kernel infrastructure just like any other driver. I personally think it's
more than that, and probably he does to, but anyway he has a point in that
they are not changing the core infrastructures. Functionally they are
eventually new API users.

Adding to that, Rust bindings require quite some specific knowledge that
is mostly to be found among the Rust tree developers for now (and not much
shared elsewhere I suspect), I think it's a better idea that you guys handle
this hrtimer binding within the Rust tree. You'll be more flexible and people
applying the related patches will know what they are doing.

What do you think?

> 
> We are probably going to have a new iteration anyway, as discussion
> picked up again.

Ok.

Thanks.

> 
> 
> Best regards,
> Andreas Hindborg
> 
> 
> 

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

[Frederic Weisbecker]

Le Thu, Feb 20, 2025 at 11:46:10PM +0000, Benno Lossin a écrit :
> On 18.02.25 14:27, Andreas Hindborg wrote:
> > This patch adds support for intrusive use of the hrtimer system. For now,
> > only one timer can be embedded in a Rust struct.
> > 
> > The hrtimer Rust API is based on the intrusive style pattern introduced by
> > the Rust workqueue API.
> > 
> > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> > ---
> >  rust/kernel/time.rs         |   2 +
> >  rust/kernel/time/hrtimer.rs | 312 ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 314 insertions(+)
> > 
> > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> > index 87e47f2f5618d..2cf365cfb412e 100644
> > --- a/rust/kernel/time.rs
> > +++ b/rust/kernel/time.rs
> > @@ -10,6 +10,8 @@
> > 
> >  use core::convert::Into;
> > 
> > +pub mod hrtimer;
> > +
> >  /// The number of nanoseconds per millisecond.
> >  pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
> > 
> > diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> > new file mode 100644
> > index 0000000000000..a6332924efabd
> > --- /dev/null
> > +++ b/rust/kernel/time/hrtimer.rs
> > @@ -0,0 +1,312 @@
> > +// 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
> > +//! * Running
> > +//!
> > +//! Operations:
> > +//!
> > +//! * Start
> > +//! * Cancel
> > +//! * Stop
> > +//! * Restart
> > +//!
> > +//! Events:
> > +//!
> > +//! * Expire
> > +//!
> > +//! ## State Diagram
> > +//!
> > +//! ```text
> > +//!                  <-- Stop ----
> > +//!                  <-- Cancel --
> > +//!                  --- Start -->
> > +//!        +---------+        +---------+
> > +//!   O--->| Stopped |        | Running |---o
> > +//!        +---------+        +---------+   |
> > +//!                                  ^      |
> > +//!                  <- Expire --    |      |
> > +//!                                  o------o
> > +//!                                   Restart
> > +//! ```
> > +//!
> > +//! A timer is initialized in the **stopped** state. A stopped timer can be
> > +//! **started** with an **expiry** time. After the timer is started, it is
> > +//! **running**. When the timer **expires**, the timer handler is executed.
> > +//! After the handler has executed, the timer may be **restarted** or
> > +//! **stopped**. A running timer can be **canceled** before it's handler is
> 
> This confuses me a bit, in the other thread you wrote that the handler
> decides if the timer should restart or be stopped. But What happens when
> I call `cancel` on the `HrTimerHandle` while the handler is running, but
> finishes shortly after with a restart request? Will it be canceled?
> 
> I also have a bit of a wording issue with "the timer is running" IIUC,
> this means that the timer subsystem keeps track of the expiry time and
> when the time is elapsed, it fires the code that you registered prior.
> At first, I thought that "the timer is running" meant that the
> registered code is running. Maybe we should have two different terms for
> that? I personally would prefer "active timer" for "the timer subsystem
> is currently tracking the time and when it is elapsed, it will run the
> code" and "running timer" for "the timer's expiry time has elapsed and
> the timer callback is currently being executed".

Good point. "Running" in the hrtimer terminology is usually to
describe a running callback and not just an elapsing (or say started) timer.

I would rather have:

Stopped: initialized but not started, or cancelled, or not restarted
Started: initialized and started or restarted
Running: executing the callback

Thanks.

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

[Andreas Hindborg]

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

> On 21.02.25 10:17, Andreas Hindborg wrote:
>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>
>>> On 18.02.25 14:27, Andreas Hindborg wrote:
>>>> +/// Operational mode of [`HrTimer`].
>>>> +#[derive(Clone, Copy)]
>>>> +pub enum HrTimerMode {
>>>> +    /// Timer expires at the given expiration time.
>>>> +    Absolute,
>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>> +    Relative,
>>>> +    /// Timer does not move between CPU cores.
>>>> +    Pinned,
>>>> +    /// Timer handler is executed in soft irq context.
>>>> +    Soft,
>>>> +    /// Timer handler is executed in hard irq context.
>>>> +    Hard,
>>>> +    /// Timer expires at the given expiration time.
>>>> +    /// Timer does not move between CPU cores.
>>>> +    AbsolutePinned,
>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>> +    /// Timer does not move between CPU cores.
>>>> +    RelativePinned,
>>>> +    /// Timer expires at the given expiration time.
>>>> +    /// Timer handler is executed in soft irq context.
>>>> +    AbsoluteSoft,
>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>> +    /// Timer handler is executed in soft irq context.
>>>> +    RelativeSoft,
>>>> +    /// Timer expires at the given expiration time.
>>>> +    /// Timer does not move between CPU cores.
>>>> +    /// Timer handler is executed in soft irq context.
>>>> +    AbsolutePinnedSoft,
>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>> +    /// Timer does not move between CPU cores.
>>>> +    /// Timer handler is executed in soft irq context.
>>>> +    RelativePinnedSoft,
>>>> +    /// Timer expires at the given expiration time.
>>>> +    /// Timer handler is executed in hard irq context.
>>>> +    AbsoluteHard,
>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>> +    /// Timer handler is executed in hard irq context.
>>>> +    RelativeHard,
>>>> +    /// Timer expires at the given expiration time.
>>>> +    /// Timer does not move between CPU cores.
>>>> +    /// Timer handler is executed in hard irq context.
>>>> +    AbsolutePinnedHard,
>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>> +    /// Timer does not move between CPU cores.
>>>> +    /// Timer handler is executed in hard irq context.
>>>> +    RelativePinnedHard,
>>>> +}
>>>
>>> At some point we probably want to move this to bitfields, or do you
>>> think it's better to keep it like this?
>>
>> Yes, eventually the would transition. The main difficulty is that not
>> all flag combinations are legal, and the zero value is also a flag.
>> There was some promising work being shared on Zulip for this [1], but I
>> don't think it is completed yet. Added Timothy to CC.
>>
>> [1] https://rust-for-linux.zulipchat.com/#narrow/channel/291565-Help/topic/Best.20way.20to.20handle.20enum.2Fflags.20situation
>
> Ah yeah I remember that. And also the complication about certain
> combinations not being allowed.
>
>>>> +
>>>> +impl From<HrTimerMode> for bindings::hrtimer_mode {
>>>> +    fn from(value: HrTimerMode) -> Self {
>>>> +        use bindings::*;
>>>> +        match value {
>>>> +            HrTimerMode::Absolute => hrtimer_mode_HRTIMER_MODE_ABS,
>>>> +            HrTimerMode::Relative => hrtimer_mode_HRTIMER_MODE_REL,
>>>> +            HrTimerMode::Pinned => hrtimer_mode_HRTIMER_MODE_PINNED,
>>>> +            HrTimerMode::Soft => hrtimer_mode_HRTIMER_MODE_SOFT,
>>>> +            HrTimerMode::Hard => hrtimer_mode_HRTIMER_MODE_HARD,
>>>> +            HrTimerMode::AbsolutePinned => hrtimer_mode_HRTIMER_MODE_ABS_PINNED,
>>>> +            HrTimerMode::RelativePinned => hrtimer_mode_HRTIMER_MODE_REL_PINNED,
>>>> +            HrTimerMode::AbsoluteSoft => hrtimer_mode_HRTIMER_MODE_ABS_SOFT,
>>>> +            HrTimerMode::RelativeSoft => hrtimer_mode_HRTIMER_MODE_REL_SOFT,
>>>> +            HrTimerMode::AbsolutePinnedSoft => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_SOFT,
>>>> +            HrTimerMode::RelativePinnedSoft => hrtimer_mode_HRTIMER_MODE_REL_PINNED_SOFT,
>>>> +            HrTimerMode::AbsoluteHard => hrtimer_mode_HRTIMER_MODE_ABS_HARD,
>>>> +            HrTimerMode::RelativeHard => hrtimer_mode_HRTIMER_MODE_REL_HARD,
>>>> +            HrTimerMode::AbsolutePinnedHard => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_HARD,
>>>> +            HrTimerMode::RelativePinnedHard => hrtimer_mode_HRTIMER_MODE_REL_PINNED_HARD,
>>>> +        }
>>>> +    }
>>>> +}
>>>> +
>>>> +impl From<HrTimerMode> for u64 {
>>>> +    fn from(value: HrTimerMode) -> Self {
>>>> +        Into::<bindings::hrtimer_mode>::into(value) as u64
>>>> +    }
>>>> +}
>>>
>>> Hmm do drivers really need these impls? If not, then you could also just
>>> have a private function that does the conversion and use it only in the
>>> abstraction layer.
>>
>> Similar to the other impls you commented on, I can move them private. I
>> would prefer using `From` rather than some other function.
>
> What's the reason for you preferring `From`? I don't think it's
> important to forbid access from the drivers, but if it's unnecessary,
> why would we give them access in the first place?

TIL trait implementations cannot be hidden. I like `From` because every
single rust developer seeing a `From` implementation will immediately know
what it does. It is more idiomatic in that way than having another
conversion function. I think using existing traits with well defined
semantics is preferable to a local function.

But since driver implementer do not need it, I'm undecided. If you want
them converted to a private function I can do that, for all the ones you
called out. But I am also OK with keeping as is. You decide.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

"Frederic Weisbecker" <frederic@kernel.org> writes:

> Le Thu, Feb 20, 2025 at 11:46:10PM +0000, Benno Lossin a écrit :
>> On 18.02.25 14:27, Andreas Hindborg wrote:
>> > This patch adds support for intrusive use of the hrtimer system. For now,
>> > only one timer can be embedded in a Rust struct.
>> >
>> > The hrtimer Rust API is based on the intrusive style pattern introduced by
>> > the Rust workqueue API.
>> >
>> > Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> > ---
>> >  rust/kernel/time.rs         |   2 +
>> >  rust/kernel/time/hrtimer.rs | 312 ++++++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 314 insertions(+)
>> >
>> > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
>> > index 87e47f2f5618d..2cf365cfb412e 100644
>> > --- a/rust/kernel/time.rs
>> > +++ b/rust/kernel/time.rs
>> > @@ -10,6 +10,8 @@
>> >
>> >  use core::convert::Into;
>> >
>> > +pub mod hrtimer;
>> > +
>> >  /// The number of nanoseconds per millisecond.
>> >  pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
>> >
>> > diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
>> > new file mode 100644
>> > index 0000000000000..a6332924efabd
>> > --- /dev/null
>> > +++ b/rust/kernel/time/hrtimer.rs
>> > @@ -0,0 +1,312 @@
>> > +// 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
>> > +//! * Running
>> > +//!
>> > +//! Operations:
>> > +//!
>> > +//! * Start
>> > +//! * Cancel
>> > +//! * Stop
>> > +//! * Restart
>> > +//!
>> > +//! Events:
>> > +//!
>> > +//! * Expire
>> > +//!
>> > +//! ## State Diagram
>> > +//!
>> > +//! ```text
>> > +//!                  <-- Stop ----
>> > +//!                  <-- Cancel --
>> > +//!                  --- Start -->
>> > +//!        +---------+        +---------+
>> > +//!   O--->| Stopped |        | Running |---o
>> > +//!        +---------+        +---------+   |
>> > +//!                                  ^      |
>> > +//!                  <- Expire --    |      |
>> > +//!                                  o------o
>> > +//!                                   Restart
>> > +//! ```
>> > +//!
>> > +//! A timer is initialized in the **stopped** state. A stopped timer can be
>> > +//! **started** with an **expiry** time. After the timer is started, it is
>> > +//! **running**. When the timer **expires**, the timer handler is executed.
>> > +//! After the handler has executed, the timer may be **restarted** or
>> > +//! **stopped**. A running timer can be **canceled** before it's handler is
>>
>> This confuses me a bit, in the other thread you wrote that the handler
>> decides if the timer should restart or be stopped. But What happens when
>> I call `cancel` on the `HrTimerHandle` while the handler is running, but
>> finishes shortly after with a restart request? Will it be canceled?
>>
>> I also have a bit of a wording issue with "the timer is running" IIUC,
>> this means that the timer subsystem keeps track of the expiry time and
>> when the time is elapsed, it fires the code that you registered prior.
>> At first, I thought that "the timer is running" meant that the
>> registered code is running. Maybe we should have two different terms for
>> that? I personally would prefer "active timer" for "the timer subsystem
>> is currently tracking the time and when it is elapsed, it will run the
>> code" and "running timer" for "the timer's expiry time has elapsed and
>> the timer callback is currently being executed".
>
> Good point. "Running" in the hrtimer terminology is usually to
> describe a running callback and not just an elapsing (or say started) timer.
>
> I would rather have:
>
> Stopped: initialized but not started, or cancelled, or not restarted
> Started: initialized and started or restarted
> Running: executing the callback

Alright, we can do that. That makes sense 👍



Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

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

> On 21.02.25 11:15, Andreas Hindborg wrote:
>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>
>>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>>
>>>> On 18.02.25 14:27, Andreas Hindborg wrote:
>>>>> +pub trait HrTimerCallback {
>>>>> +    /// The type whose [`RawHrTimerCallback::run`] method will be invoked when
>>>>> +    /// the timer expires.
>>>>> +    type CallbackTarget<'a>: RawHrTimerCallback;
>>>>> +
>>>>> +    /// This type is passed to the timer callback function. It may be a borrow
>>>>> +    /// of [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
>>>>> +    /// implementation can guarantee exclusive access to the target during timer
>>>>
>>>> Technically "exclusive" access is correct if the `CallbackTarget` is
>>>> `Pin<&Self>`, since you will get exclusive access to a `Pin<&Self>`, but
>>>> it might confuse people, because there can be multiple `Pin<&Self>`. So
>>>> I would just drop the word "exclusive" here.
>>>
>>> Yes, maybe it should be "shared or exclusive access, depending on the type"?
>>>
>>>>
>>>>> +    /// handler execution.
>>>>> +    type CallbackTargetParameter<'a>;
>>>>
>>>> Also why can't this type be an associated type of `HrTimerPointer`?
>>>> Since this seems to always be constrained in the impls of
>>>> `RawHrTimerCallback`.
>>>
>>> That might be a nice improvement, I'll try that out.
>>
>> Looking closer at this, I don't see how to achieve this. We need access
>> to the type here, because it is used in the signature of `run`.
>> `HrTimerCallback` has no bounds on it, and that is nice. If we want to
>> move these associated types, we have to introduce a bound here.
>>
>> We need to be generic over the type of the parameter to `run`, and by
>> the time the user implements this trait, the type must be known and so
>> the user has to specify somehow.
>
> I think if you put the associated type on the `RawHrTimerCallback`
> trait, it should work.

What would be the signature of `HrTimerCallback::run` in that case?


Best regards,
Andreas Hindborg

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

[Tamir Duberstein]

On Fri, Feb 21, 2025 at 3:20 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Tamir Duberstein" <tamird@gmail.com> writes:
>
> > On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> [...]
>
> >> >> +pub unsafe trait HrTimerHandle {
> >> >> +    /// Cancel the timer, if it is running. If the timer handler is running, block
> >> >> +    /// till the handler has finished.
> >> >> +    fn cancel(&mut self) -> bool;
> >> >> +}
> >> >> +
> >> >> +/// Implemented by structs that contain timer nodes.
> >> >> +///
> >> >> +/// Clients of the timer API would usually safely implement this trait by using
> >> >> +/// the [`crate::impl_has_hr_timer`] macro.
> >> >> +///
> >> >> +/// # Safety
> >> >> +///
> >> >> +/// Implementers of this trait must ensure that the implementer has a [`HrTimer`]
> >> >> +/// field at the offset specified by `OFFSET` and that all trait methods are
> >> >> +/// implemented according to their documentation.
> >> >> +///
> >> >> +/// [`impl_has_timer`]: crate::impl_has_timer
> >> >> +pub unsafe trait HasHrTimer<T> {
> >> >> +    /// Offset of the [`HrTimer`] field within `Self`
> >> >> +    const OFFSET: usize;
> >> >
> >> > Does this need to be part of the trait? As an alternative the provided
> >> > methods could be generated in the macro below and reduce the
> >> > opportunity to implement this trait incorrectly.
> >>
> >> There is no risk of implementing the trait wrong, because it is usually
> >> derived by a macro.
> >
> > There's no risk when it's implemented by the macro, but you used the
> > word usually,  which means there is a risk.
> >
> >> We need at least one of the methods to be able to have the type system
> >> verify that the type for which we implement `HasHrTImer` actually has a
> >> field with the name we specify, and that this field has the right type.
> >> And to have that, we need the OFFSET.
> >
> > I don't follow this logic. OFFSET is calculated in the body of the
> > macro. I'm suggesting that the macro generate the method
> > implementations (which would no longer be provided). In effect I'm
> > saying: keep OFFSET private.
> >
> > I'm also noticing now that the macro generates an implementation of
> > raw_get_timer *in addition to* the provided implementation. Why are
> > both needed?
>
> HasHrTimer is unsafe, because it would be unsound to implement, if the
> type it is implemented on does not have a `Timer` at the specified
> offset.
>
> To be able to implement it safely with a macro, the macro must verify
> that the type we implement the trait on satisfies the safety
> requirement. That is, we have to have the macro verify that the type
> indeed has a field of type `Timer` with the given name. If that is the
> case, the macro can calculate OFFSET.
>
> The way we achieve this is we re-implement on of the trait methods in
> such a way that it only compiles if the type we reimplement trait
> on actually have the field of the right type.
>
> I want to generate as little code as possible in the macro, and I would
> rather rely on the default implementations given in the trait, than have
> the macro generate implementations for all the methods. Generated code
> are more difficult to reason about.

Again, I don't follow. The provided implementation of raw_get_timer is
either not used (in the presence of the macro) or it relies on the
implementer correctly setting OFFSET, which the compiler cannot check
and which can break at a distance.

Wouldn't it be simpler to just generate both functions that rely on
OFFSET? They're both one-liners that delegate to other existing
macros.

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

[Tamir Duberstein]

On Fri, Feb 21, 2025 at 3:37 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Tamir Duberstein" <tamird@gmail.com> writes:
>
> > On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> "Tamir Duberstein" <tamird@gmail.com> writes:
> >>
> >> > On Tue, Feb 18, 2025 at 8:29 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >> >>
> >>
> >> [...]
> >>
> >> >> +//! ## State Diagram
> >> >> +//!
> >> >> +//! ```text
> >> >> +//!                  <-- Stop ----
> >> >> +//!                  <-- Cancel --
> >> >> +//!                  --- Start -->
> >> >> +//!        +---------+        +---------+
> >> >> +//!   O--->| Stopped |        | Running |---o
> >> >> +//!        +---------+        +---------+   |
> >> >> +//!                                  ^      |
> >> >> +//!                  <- Expire --    |      |
> >> >> +//!                                  o------o
> >> >> +//!                                   Restart
> >> >> +//! ```
> >> >> +//!
> >> >> +//! A timer is initialized in the **stopped** state. A stopped timer can be
> >> >> +//! **started** with an **expiry** time. After the timer is started, it is
> >> >> +//! **running**. When the timer **expires**, the timer handler is executed.
> >> >> +//! After the handler has executed, the timer may be **restarted** or
> >> >> +//! **stopped**. A running timer can be **canceled** before it's handler is
> >> >
> >> > "it's" = it is. This should be "its" (possessive).
> >>
> >> Thanks 👍
> >>
> >> > Just to be clear, after the handler has executed and before the timer
> >> > has been **restarted** or **stopped** the timer is still in the
> >> > **running** state?
> >>
> >> It depends on the return value of the handler. If the handler returns restart,
> >> the timer the timer does not enter the stopped state. If the handler
> >> returns stop, the timer enters the stopped state.
> >>
> >> The timer is still considered to be in running state the handler is
> >> running.
> >>
> >> I can add this info to the section.
> >
> > Yeah, some clarification here would be useful.
>
> I'll add a paragraph 👍
>
> [...]
>
> >> >> +    /// Get a pointer to the contained `bindings::hrtimer`.
> >> >> +    ///
> >> >> +    /// # Safety
> >> >> +    ///
> >> >> +    /// `ptr` must point to a live allocation of at least the size of `Self`.
> >> >> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
> >> >> +        // SAFETY: The field projection to `timer` does not go out of bounds,
> >> >> +        // because the caller of this function promises that `ptr` points to an
> >> >> +        // allocation of at least the size of `Self`.
> >> >> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
> >> >> +    }
> >> >
> >> > Can you help me understand why the various functions here operate on
> >> > *const Self? I understand the need to obtain a C pointer to interact
> >> > with bindings, but I don't understand why we're dealing in raw
> >> > pointers to the abstraction rather than references.
> >>
> >> We cannot reference the `bindings::hrtimer` without wrapping it in
> >> `Opaque`. This would be the primary reason. At other times, we cannot
> >> produce references because we might not be able to prove that we satisfy
> >> the safety requirements for turning a pointer into a reference. If we
> >> are just doing offset arithmetic anyway, we don't need a reference.
> >
> > Why do we have a pointer, rather than a reference, to Self in the
> > first place? I think this is the key thing I don't understand.
>
> Perhaps it makes more sense if you look at the context. One of the entry
> points to `HrTimer::raw_get` is via `<ArcHrTimerHandle as
> HrTimerHandle>::cancel`. This user facing method takes `&mut self`. The
> handle contains an arc to a type that contains a `Timer` and implements
> `HasHrTImer`. To get to the timer, we need to do pointer manipulation.

This is the part I don't entirely understand. Why can't we chase
references all the way to the Timer? AFAICT the reason is that the
HasHrTimer trait is in terms of pointers, but couldn't it be in terms
of references? At least for getting the timer, not necessarily for
getting the timer's container.

> We only know how to get the `Timer` field via the `OFFSET`. The natural
> return value from the offset operation is a raw pointer. Rather than
> convert back to a reference, we stay in pointer land when we call
> `HrTimer::raw_cancel`, because we need a pointer to the
> `bindings::hrtimer` anyway, not a reference.

Sure, but this is breaking encapsulation in a way. Rather than having
a Timer type that owns the raw pointer and is responsible for all
operations on it, a variety of callers are allowed to get the raw
pointer and interact with the bindings.

>
> >
> >>
> >>
> >> > This extends to
> >> > HrTimerPointer, which is intended to be implemented by *pointers to*
> >> > structs that embed `HrTimer`; why isn't it implemented on by the
> >> > embedder itself?
> >>
> >> Not sure what you mean here. If you refer to for instance the
> >> implementation of `HrTimerPointer for Arc<T>`, I get why you might
> >> wonder, why does `HasHrTimer::start` not take a reference instead of a
> >> pointer? We could do that, but we would just immediately break it down
> >> again in the implementation of `HasHrTimer::start`. Might still be a
> >> good idea though.
> >
> > I was trying to say that my question (which I clarified above,
> > hopefully) extends to the description and name of this trait.
> > Specifically for this trait I don't understand why its semantics are
> > described in terms of pointers rather than references (and AsRef, to
> > allow for Arc and friends).
>
> All user facing APIs use references, not pointers. The raw pointer
> interfaces are for internal use only. I don't think we would gain
> anything from using `AsRef` internally. Perhaps you could clarify a bit more?

It's hard to prove the counter-factual, I don't know exactly what the
code would look like if we went with references everywhere. It just
seems to me that we're writing quite C-like Rust here, which implies
to me that we're not leveraging the compiler as much as we could.

>
> >
> >> >
> >> > I realize we discussed this on v6, sorry for not keeping up there.
> >>
> >> No worries, it is good that we discuss this.
> >>
> >> [...]
> >>
> >> >> +
> >> >> +/// A handle representing a potentially running timer.
> >> >> +///
> >> >> +/// More than one handle representing the same timer might exist.
> >> >> +///
> >> >> +/// # Safety
> >> >> +///
> >> >> +/// When dropped, the timer represented by this handle must be cancelled, if it
> >> >> +/// is running. If the timer handler is running when the handle is dropped, the
> >> >> +/// drop method must wait for the handler to finish before returning.
> >> >
> >> > Between this comment and the comment on cancel we say "if it is
> >> > running" 3 times. Can we say it just once, on the method, and here say
> >> > that cancel must be called in Drop?
> >>
> >> Well, the comment on `cancel` is just a description of what the function
> >> does. This piece of text is a safety requirement.
> >>
> >> We could make the safety requirement for implementing the trait "Implement
> >> the methods according to their documentation". But that would not help with
> >> the drop requirement.
> >
> > My suggestion is that the safety comment read: when dropped,
> > [Self::cancel] must be called. Something like that.
>
> We don't care how the timer is canceled, it just has to be canceled. It
> does not have to be by calling `Self::cancel`.

I understand. I'm only suggesting that we could reduce the repetition a bit.

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

[Andreas Hindborg]

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

> On Fri, Feb 21, 2025 at 3:20 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Tamir Duberstein" <tamird@gmail.com> writes:
>>
>> > On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> [...]
>>
>> >> >> +pub unsafe trait HrTimerHandle {
>> >> >> +    /// Cancel the timer, if it is running. If the timer handler is running, block
>> >> >> +    /// till the handler has finished.
>> >> >> +    fn cancel(&mut self) -> bool;
>> >> >> +}
>> >> >> +
>> >> >> +/// Implemented by structs that contain timer nodes.
>> >> >> +///
>> >> >> +/// Clients of the timer API would usually safely implement this trait by using
>> >> >> +/// the [`crate::impl_has_hr_timer`] macro.
>> >> >> +///
>> >> >> +/// # Safety
>> >> >> +///
>> >> >> +/// Implementers of this trait must ensure that the implementer has a [`HrTimer`]
>> >> >> +/// field at the offset specified by `OFFSET` and that all trait methods are
>> >> >> +/// implemented according to their documentation.
>> >> >> +///
>> >> >> +/// [`impl_has_timer`]: crate::impl_has_timer
>> >> >> +pub unsafe trait HasHrTimer<T> {
>> >> >> +    /// Offset of the [`HrTimer`] field within `Self`
>> >> >> +    const OFFSET: usize;
>> >> >
>> >> > Does this need to be part of the trait? As an alternative the provided
>> >> > methods could be generated in the macro below and reduce the
>> >> > opportunity to implement this trait incorrectly.
>> >>
>> >> There is no risk of implementing the trait wrong, because it is usually
>> >> derived by a macro.
>> >
>> > There's no risk when it's implemented by the macro, but you used the
>> > word usually,  which means there is a risk.
>> >
>> >> We need at least one of the methods to be able to have the type system
>> >> verify that the type for which we implement `HasHrTImer` actually has a
>> >> field with the name we specify, and that this field has the right type.
>> >> And to have that, we need the OFFSET.
>> >
>> > I don't follow this logic. OFFSET is calculated in the body of the
>> > macro. I'm suggesting that the macro generate the method
>> > implementations (which would no longer be provided). In effect I'm
>> > saying: keep OFFSET private.
>> >
>> > I'm also noticing now that the macro generates an implementation of
>> > raw_get_timer *in addition to* the provided implementation. Why are
>> > both needed?
>>
>> HasHrTimer is unsafe, because it would be unsound to implement, if the
>> type it is implemented on does not have a `Timer` at the specified
>> offset.
>>
>> To be able to implement it safely with a macro, the macro must verify
>> that the type we implement the trait on satisfies the safety
>> requirement. That is, we have to have the macro verify that the type
>> indeed has a field of type `Timer` with the given name. If that is the
>> case, the macro can calculate OFFSET.
>>
>> The way we achieve this is we re-implement on of the trait methods in
>> such a way that it only compiles if the type we reimplement trait
>> on actually have the field of the right type.
>>
>> I want to generate as little code as possible in the macro, and I would
>> rather rely on the default implementations given in the trait, than have
>> the macro generate implementations for all the methods. Generated code
>> are more difficult to reason about.
>
> Again, I don't follow. The provided implementation of raw_get_timer is
> either not used (in the presence of the macro) or it relies on the
> implementer correctly setting OFFSET, which the compiler cannot check
> and which can break at a distance.
>
> Wouldn't it be simpler to just generate both functions that rely on
> OFFSET? They're both one-liners that delegate to other existing
> macros.

No, I would rather generate as little code as possible. The only reason
I am generating `raw_get_timer` is to be able to type check that the
field name given to the macro has the right type.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

Andreas Hindborg <a.hindborg@kernel.org> writes:

> "Tamir Duberstein" <tamird@gmail.com> writes:
>
>> On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>
>>> "Tamir Duberstein" <tamird@gmail.com> writes:
>>>
>>> > On Tue, Feb 18, 2025 at 8:29 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:

[...]

>>> >> +    /// Get a pointer to the contained `bindings::hrtimer`.
>>> >> +    ///
>>> >> +    /// # Safety
>>> >> +    ///
>>> >> +    /// `ptr` must point to a live allocation of at least the size of `Self`.
>>> >> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
>>> >> +        // SAFETY: The field projection to `timer` does not go out of bounds,
>>> >> +        // because the caller of this function promises that `ptr` points to an
>>> >> +        // allocation of at least the size of `Self`.
>>> >> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
>>> >> +    }
>>> >
>>> > Can you help me understand why the various functions here operate on
>>> > *const Self? I understand the need to obtain a C pointer to interact
>>> > with bindings, but I don't understand why we're dealing in raw
>>> > pointers to the abstraction rather than references.
>>>
>>> We cannot reference the `bindings::hrtimer` without wrapping it in
>>> `Opaque`. This would be the primary reason. At other times, we cannot
>>> produce references because we might not be able to prove that we satisfy
>>> the safety requirements for turning a pointer into a reference. If we
>>> are just doing offset arithmetic anyway, we don't need a reference.
>>
>> Why do we have a pointer, rather than a reference, to Self in the
>> first place? I think this is the key thing I don't understand.
>
> Perhaps it makes more sense if you look at the context. One of the entry
> points to `HrTimer::raw_get` is via `<ArcHrTimerHandle as
> HrTimerHandle>::cancel`. This user facing method takes `&mut self`. The
> handle contains an arc to a type that contains a `Timer` and implements
> `HasHrTImer`. To get to the timer, we need to do pointer manipulation.
> We only know how to get the `Timer` field via the `OFFSET`. The natural
> return value from the offset operation is a raw pointer. Rather than
> convert back to a reference, we stay in pointer land when we call
> `HrTimer::raw_cancel`, because we need a pointer to the
> `bindings::hrtimer` anyway, not a reference.

I changed `HasHrTimer::start` to take a reference, and I think that
makes sense 👍 Taking an `impl AsRef` does not work out when `Self` is
`Pin<&T>`. I'll go over the whole thing and see of other places could
benefit.

Best regards,
Andreas Hindborg

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

[Tamir Duberstein]

On Fri, Feb 21, 2025 at 8:17 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Tamir Duberstein" <tamird@gmail.com> writes:
>
> > On Fri, Feb 21, 2025 at 3:20 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> "Tamir Duberstein" <tamird@gmail.com> writes:
> >>
> >> > On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> [...]
> >>
> >> >> >> +pub unsafe trait HrTimerHandle {
> >> >> >> +    /// Cancel the timer, if it is running. If the timer handler is running, block
> >> >> >> +    /// till the handler has finished.
> >> >> >> +    fn cancel(&mut self) -> bool;
> >> >> >> +}
> >> >> >> +
> >> >> >> +/// Implemented by structs that contain timer nodes.
> >> >> >> +///
> >> >> >> +/// Clients of the timer API would usually safely implement this trait by using
> >> >> >> +/// the [`crate::impl_has_hr_timer`] macro.
> >> >> >> +///
> >> >> >> +/// # Safety
> >> >> >> +///
> >> >> >> +/// Implementers of this trait must ensure that the implementer has a [`HrTimer`]
> >> >> >> +/// field at the offset specified by `OFFSET` and that all trait methods are
> >> >> >> +/// implemented according to their documentation.
> >> >> >> +///
> >> >> >> +/// [`impl_has_timer`]: crate::impl_has_timer
> >> >> >> +pub unsafe trait HasHrTimer<T> {
> >> >> >> +    /// Offset of the [`HrTimer`] field within `Self`
> >> >> >> +    const OFFSET: usize;
> >> >> >
> >> >> > Does this need to be part of the trait? As an alternative the provided
> >> >> > methods could be generated in the macro below and reduce the
> >> >> > opportunity to implement this trait incorrectly.
> >> >>
> >> >> There is no risk of implementing the trait wrong, because it is usually
> >> >> derived by a macro.
> >> >
> >> > There's no risk when it's implemented by the macro, but you used the
> >> > word usually,  which means there is a risk.
> >> >
> >> >> We need at least one of the methods to be able to have the type system
> >> >> verify that the type for which we implement `HasHrTImer` actually has a
> >> >> field with the name we specify, and that this field has the right type.
> >> >> And to have that, we need the OFFSET.
> >> >
> >> > I don't follow this logic. OFFSET is calculated in the body of the
> >> > macro. I'm suggesting that the macro generate the method
> >> > implementations (which would no longer be provided). In effect I'm
> >> > saying: keep OFFSET private.
> >> >
> >> > I'm also noticing now that the macro generates an implementation of
> >> > raw_get_timer *in addition to* the provided implementation. Why are
> >> > both needed?
> >>
> >> HasHrTimer is unsafe, because it would be unsound to implement, if the
> >> type it is implemented on does not have a `Timer` at the specified
> >> offset.
> >>
> >> To be able to implement it safely with a macro, the macro must verify
> >> that the type we implement the trait on satisfies the safety
> >> requirement. That is, we have to have the macro verify that the type
> >> indeed has a field of type `Timer` with the given name. If that is the
> >> case, the macro can calculate OFFSET.
> >>
> >> The way we achieve this is we re-implement on of the trait methods in
> >> such a way that it only compiles if the type we reimplement trait
> >> on actually have the field of the right type.
> >>
> >> I want to generate as little code as possible in the macro, and I would
> >> rather rely on the default implementations given in the trait, than have
> >> the macro generate implementations for all the methods. Generated code
> >> are more difficult to reason about.
> >
> > Again, I don't follow. The provided implementation of raw_get_timer is
> > either not used (in the presence of the macro) or it relies on the
> > implementer correctly setting OFFSET, which the compiler cannot check
> > and which can break at a distance.
> >
> > Wouldn't it be simpler to just generate both functions that rely on
> > OFFSET? They're both one-liners that delegate to other existing
> > macros.
>
> No, I would rather generate as little code as possible. The only reason
> I am generating `raw_get_timer` is to be able to type check that the
> field name given to the macro has the right type.

At a minimum this code is inconsistent because it includes a provided
implementation which is never used.

But I continue to disagree that generating OFFSET and then doing raw
pointer manipulation is better than just generating a one-line method.
Perhaps someone else could weigh in.

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

[Tamir Duberstein]

On Fri, Feb 21, 2025 at 8:29 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Andreas Hindborg <a.hindborg@kernel.org> writes:
>
> > "Tamir Duberstein" <tamird@gmail.com> writes:
> >
> >> On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>>
> >>> "Tamir Duberstein" <tamird@gmail.com> writes:
> >>>
> >>> > On Tue, Feb 18, 2025 at 8:29 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> [...]
>
> >>> >> +    /// Get a pointer to the contained `bindings::hrtimer`.
> >>> >> +    ///
> >>> >> +    /// # Safety
> >>> >> +    ///
> >>> >> +    /// `ptr` must point to a live allocation of at least the size of `Self`.
> >>> >> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
> >>> >> +        // SAFETY: The field projection to `timer` does not go out of bounds,
> >>> >> +        // because the caller of this function promises that `ptr` points to an
> >>> >> +        // allocation of at least the size of `Self`.
> >>> >> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
> >>> >> +    }
> >>> >
> >>> > Can you help me understand why the various functions here operate on
> >>> > *const Self? I understand the need to obtain a C pointer to interact
> >>> > with bindings, but I don't understand why we're dealing in raw
> >>> > pointers to the abstraction rather than references.
> >>>
> >>> We cannot reference the `bindings::hrtimer` without wrapping it in
> >>> `Opaque`. This would be the primary reason. At other times, we cannot
> >>> produce references because we might not be able to prove that we satisfy
> >>> the safety requirements for turning a pointer into a reference. If we
> >>> are just doing offset arithmetic anyway, we don't need a reference.
> >>
> >> Why do we have a pointer, rather than a reference, to Self in the
> >> first place? I think this is the key thing I don't understand.
> >
> > Perhaps it makes more sense if you look at the context. One of the entry
> > points to `HrTimer::raw_get` is via `<ArcHrTimerHandle as
> > HrTimerHandle>::cancel`. This user facing method takes `&mut self`. The
> > handle contains an arc to a type that contains a `Timer` and implements
> > `HasHrTImer`. To get to the timer, we need to do pointer manipulation.
> > We only know how to get the `Timer` field via the `OFFSET`. The natural
> > return value from the offset operation is a raw pointer. Rather than
> > convert back to a reference, we stay in pointer land when we call
> > `HrTimer::raw_cancel`, because we need a pointer to the
> > `bindings::hrtimer` anyway, not a reference.
>
> I changed `HasHrTimer::start` to take a reference, and I think that
> makes sense 👍 Taking an `impl AsRef` does not work out when `Self` is
> `Pin<&T>`. I'll go over the whole thing and see of other places could
> benefit.

Can you elaborate please? Pin<&T>::get_ref returns &T, which gets you
to the AsRef you need, no?

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

[Boqun Feng]

On Fri, Feb 21, 2025 at 02:28:58PM +0100, Andreas Hindborg wrote:
> Andreas Hindborg <a.hindborg@kernel.org> writes:
> 
> > "Tamir Duberstein" <tamird@gmail.com> writes:
> >
> >> On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>>
> >>> "Tamir Duberstein" <tamird@gmail.com> writes:
> >>>
> >>> > On Tue, Feb 18, 2025 at 8:29 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> [...]
> 
> >>> >> +    /// Get a pointer to the contained `bindings::hrtimer`.
> >>> >> +    ///
> >>> >> +    /// # Safety
> >>> >> +    ///
> >>> >> +    /// `ptr` must point to a live allocation of at least the size of `Self`.
> >>> >> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
> >>> >> +        // SAFETY: The field projection to `timer` does not go out of bounds,
> >>> >> +        // because the caller of this function promises that `ptr` points to an
> >>> >> +        // allocation of at least the size of `Self`.
> >>> >> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
> >>> >> +    }
> >>> >
> >>> > Can you help me understand why the various functions here operate on
> >>> > *const Self? I understand the need to obtain a C pointer to interact
> >>> > with bindings, but I don't understand why we're dealing in raw
> >>> > pointers to the abstraction rather than references.
> >>>
> >>> We cannot reference the `bindings::hrtimer` without wrapping it in
> >>> `Opaque`. This would be the primary reason. At other times, we cannot
> >>> produce references because we might not be able to prove that we satisfy
> >>> the safety requirements for turning a pointer into a reference. If we
> >>> are just doing offset arithmetic anyway, we don't need a reference.
> >>
> >> Why do we have a pointer, rather than a reference, to Self in the
> >> first place? I think this is the key thing I don't understand.
> >
> > Perhaps it makes more sense if you look at the context. One of the entry
> > points to `HrTimer::raw_get` is via `<ArcHrTimerHandle as
> > HrTimerHandle>::cancel`. This user facing method takes `&mut self`. The
> > handle contains an arc to a type that contains a `Timer` and implements
> > `HasHrTImer`. To get to the timer, we need to do pointer manipulation.
> > We only know how to get the `Timer` field via the `OFFSET`. The natural
> > return value from the offset operation is a raw pointer. Rather than
> > convert back to a reference, we stay in pointer land when we call
> > `HrTimer::raw_cancel`, because we need a pointer to the
> > `bindings::hrtimer` anyway, not a reference.
> 
> I changed `HasHrTimer::start` to take a reference, and I think that
> makes sense 👍 Taking an `impl AsRef` does not work out when `Self` is
> `Pin<&T>`. I'll go over the whole thing and see of other places could
> benefit.
> 

Hmm... if you mean:

trait HasHrTimer {
    unsafe fn start(&self, expires: Ktime) {
        ...
    }
}

Then it'll be problematic because the pointer derived from `&self`
doesn't have write provenance, therefore in a timer callback, the
pointer cannot be used for write, which means for example you cannot
convert the pointer back into a `Pin<Box<HasTimer>>`.

To answer Tamir's question, pointers are heavily used here because we
need to preserve the provenance.

Regards,
Boqun

> Best regards,
> Andreas Hindborg
> 
> 
> 

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

[Tamir Duberstein]

On Fri, Feb 21, 2025 at 9:40 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Hmm... if you mean:
>
> trait HasHrTimer {
>     unsafe fn start(&self, expires: Ktime) {
>         ...
>     }
> }
>
> Then it'll be problematic because the pointer derived from `&self`
> doesn't have write provenance, therefore in a timer callback, the
> pointer cannot be used for write, which means for example you cannot
> convert the pointer back into a `Pin<Box<HasTimer>>`.
>
> To answer Tamir's question, pointers are heavily used here because we
> need to preserve the provenance.

Wouldn't the natural implication be that &mut self is needed? Maybe
you can help me understand why pointers can express a contract that
references can't?

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

[Boqun Feng]

On Fri, Feb 21, 2025 at 09:46:08AM -0500, Tamir Duberstein wrote:
> On Fri, Feb 21, 2025 at 9:40 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > Hmm... if you mean:
> >
> > trait HasHrTimer {
> >     unsafe fn start(&self, expires: Ktime) {
> >         ...
> >     }
> > }
> >
> > Then it'll be problematic because the pointer derived from `&self`
> > doesn't have write provenance, therefore in a timer callback, the
> > pointer cannot be used for write, which means for example you cannot
> > convert the pointer back into a `Pin<Box<HasTimer>>`.
> >
> > To answer Tamir's question, pointers are heavily used here because we
> > need to preserve the provenance.
> 
> Wouldn't the natural implication be that &mut self is needed? Maybe

For an `Arc<HasTimer>`, you cannot get `&mut self`.

> you can help me understand why pointers can express a contract that
> references can't?

I assume you already know what a pointer provenance is?

	http://doc.rust-lang.org/std/ptr/index.html#provenance

Passing a pointer (including offset operation on it) preserves the
provenance (determined as derive time), however, deriving a pointer from
a reference gives the pointer a provenance based on the reference type.
For example, let's say we have an `Arc<i32>` and a clone:

	let arc = Arc::new(42);
	let clone = arc.clone();

you can obviously do a into_raw() + from_raw() pair:

	let ptr = Arc::into_raw(arc);
	let arc = unsafe { Arc::from_raw(arc) };

however, if you create a reference based on `Arc::into_raw()`, and then
derive a pointer from that, you change the provenance, therefore the
below code would generate UB:

	// cannot mutably borrow because of clone.
	let ptr = unsafe { &*Arc::into_raw(arc) } as *const i32;
    
	let arc = unsafe { Arc::from_raw(ptr) };


(playground code snippet for this example)

	https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=15e051db46c3886b29ed02e579562278

As you already know, the whole thing about pointers/references here is
passing the value to the callback and the callback can "reconstruct" the
data, in such a case, reborrowing in the middle of the chain into a
reference is not necessary, and as the above shows, it can be
problematic.

Hope this could be helpful.

Regards,
Boqun

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

[Tamir Duberstein]

On Fri, Feb 21, 2025 at 10:19 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Fri, Feb 21, 2025 at 09:46:08AM -0500, Tamir Duberstein wrote:
> > On Fri, Feb 21, 2025 at 9:40 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> > >
> > > Hmm... if you mean:
> > >
> > > trait HasHrTimer {
> > >     unsafe fn start(&self, expires: Ktime) {
> > >         ...
> > >     }
> > > }
> > >
> > > Then it'll be problematic because the pointer derived from `&self`
> > > doesn't have write provenance, therefore in a timer callback, the
> > > pointer cannot be used for write, which means for example you cannot
> > > convert the pointer back into a `Pin<Box<HasTimer>>`.
> > >
> > > To answer Tamir's question, pointers are heavily used here because we
> > > need to preserve the provenance.
> >
> > Wouldn't the natural implication be that &mut self is needed? Maybe
>
> For an `Arc<HasTimer>`, you cannot get `&mut self`.
>
> > you can help me understand why pointers can express a contract that
> > references can't?
>
> I assume you already know what a pointer provenance is?
>
>         http://doc.rust-lang.org/std/ptr/index.html#provenance
>
> Passing a pointer (including offset operation on it) preserves the
> provenance (determined as derive time), however, deriving a pointer from
> a reference gives the pointer a provenance based on the reference type.
> For example, let's say we have an `Arc<i32>` and a clone:
>
>         let arc = Arc::new(42);
>         let clone = arc.clone();
>
> you can obviously do a into_raw() + from_raw() pair:
>
>         let ptr = Arc::into_raw(arc);
>         let arc = unsafe { Arc::from_raw(arc) };
>
> however, if you create a reference based on `Arc::into_raw()`, and then
> derive a pointer from that, you change the provenance, therefore the
> below code would generate UB:
>
>         // cannot mutably borrow because of clone.
>         let ptr = unsafe { &*Arc::into_raw(arc) } as *const i32;
>
>         let arc = unsafe { Arc::from_raw(ptr) };
>
>
> (playground code snippet for this example)
>
>         https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=15e051db46c3886b29ed02e579562278
>
> As you already know, the whole thing about pointers/references here is
> passing the value to the callback and the callback can "reconstruct" the
> data, in such a case, reborrowing in the middle of the chain into a
> reference is not necessary, and as the above shows, it can be
> problematic.
>
> Hope this could be helpful.
>
> Regards,
> Boqun

Thanks for the explanation. I think where I'm still confused is that
provenance is to pointers as the type system is to references. In
other words, it's not clear to me how using pointers solves the
problem of wanting to write through an Arc. Is the idea that the
pointer inside the Arc has write provenance, and that by doing pointer
offsets instead of going through references we get to break rules
about mutability?

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

[Boqun Feng]

On Fri, Feb 21, 2025 at 02:46:07PM -0500, Tamir Duberstein wrote:
> On Fri, Feb 21, 2025 at 10:19 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Fri, Feb 21, 2025 at 09:46:08AM -0500, Tamir Duberstein wrote:
> > > On Fri, Feb 21, 2025 at 9:40 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > >
> > > > Hmm... if you mean:
> > > >
> > > > trait HasHrTimer {
> > > >     unsafe fn start(&self, expires: Ktime) {
> > > >         ...
> > > >     }
> > > > }
> > > >
> > > > Then it'll be problematic because the pointer derived from `&self`
> > > > doesn't have write provenance, therefore in a timer callback, the
> > > > pointer cannot be used for write, which means for example you cannot
> > > > convert the pointer back into a `Pin<Box<HasTimer>>`.
> > > >
> > > > To answer Tamir's question, pointers are heavily used here because we
> > > > need to preserve the provenance.
> > >
> > > Wouldn't the natural implication be that &mut self is needed? Maybe
> >
> > For an `Arc<HasTimer>`, you cannot get `&mut self`.
> >
> > > you can help me understand why pointers can express a contract that
> > > references can't?
> >
> > I assume you already know what a pointer provenance is?
> >
> >         http://doc.rust-lang.org/std/ptr/index.html#provenance
> >
> > Passing a pointer (including offset operation on it) preserves the
> > provenance (determined as derive time), however, deriving a pointer from
> > a reference gives the pointer a provenance based on the reference type.
> > For example, let's say we have an `Arc<i32>` and a clone:
> >
> >         let arc = Arc::new(42);
> >         let clone = arc.clone();
> >
> > you can obviously do a into_raw() + from_raw() pair:
> >
> >         let ptr = Arc::into_raw(arc);
> >         let arc = unsafe { Arc::from_raw(arc) };
> >
> > however, if you create a reference based on `Arc::into_raw()`, and then
> > derive a pointer from that, you change the provenance, therefore the
> > below code would generate UB:
> >
> >         // cannot mutably borrow because of clone.
> >         let ptr = unsafe { &*Arc::into_raw(arc) } as *const i32;
> >
> >         let arc = unsafe { Arc::from_raw(ptr) };
> >
> >
> > (playground code snippet for this example)
> >
> >         https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=15e051db46c3886b29ed02e579562278
> >
> > As you already know, the whole thing about pointers/references here is
> > passing the value to the callback and the callback can "reconstruct" the
> > data, in such a case, reborrowing in the middle of the chain into a
> > reference is not necessary, and as the above shows, it can be
> > problematic.
> >
> > Hope this could be helpful.
> >
> > Regards,
> > Boqun
> 
> Thanks for the explanation. I think where I'm still confused is that
> provenance is to pointers as the type system is to references. In
> other words, it's not clear to me how using pointers solves the
> problem of wanting to write through an Arc. Is the idea that the
> pointer inside the Arc has write provenance, and that by doing pointer
> offsets instead of going through references we get to break rules
> about mutability?

Sort of, but we don't actually break any rule here, because pointer are
supposed to be unsafe to dereference ;-)

Regards,
Boqun

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

[Tamir Duberstein]

On Fri, Feb 21, 2025 at 5:37 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Fri, Feb 21, 2025 at 02:46:07PM -0500, Tamir Duberstein wrote:
> > On Fri, Feb 21, 2025 at 10:19 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> > >
> > > On Fri, Feb 21, 2025 at 09:46:08AM -0500, Tamir Duberstein wrote:
> > > > On Fri, Feb 21, 2025 at 9:40 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > > >
> > > > > Hmm... if you mean:
> > > > >
> > > > > trait HasHrTimer {
> > > > >     unsafe fn start(&self, expires: Ktime) {
> > > > >         ...
> > > > >     }
> > > > > }
> > > > >
> > > > > Then it'll be problematic because the pointer derived from `&self`
> > > > > doesn't have write provenance, therefore in a timer callback, the
> > > > > pointer cannot be used for write, which means for example you cannot
> > > > > convert the pointer back into a `Pin<Box<HasTimer>>`.
> > > > >
> > > > > To answer Tamir's question, pointers are heavily used here because we
> > > > > need to preserve the provenance.
> > > >
> > > > Wouldn't the natural implication be that &mut self is needed? Maybe
> > >
> > > For an `Arc<HasTimer>`, you cannot get `&mut self`.
> > >
> > > > you can help me understand why pointers can express a contract that
> > > > references can't?
> > >
> > > I assume you already know what a pointer provenance is?
> > >
> > >         http://doc.rust-lang.org/std/ptr/index.html#provenance
> > >
> > > Passing a pointer (including offset operation on it) preserves the
> > > provenance (determined as derive time), however, deriving a pointer from
> > > a reference gives the pointer a provenance based on the reference type.
> > > For example, let's say we have an `Arc<i32>` and a clone:
> > >
> > >         let arc = Arc::new(42);
> > >         let clone = arc.clone();
> > >
> > > you can obviously do a into_raw() + from_raw() pair:
> > >
> > >         let ptr = Arc::into_raw(arc);
> > >         let arc = unsafe { Arc::from_raw(arc) };
> > >
> > > however, if you create a reference based on `Arc::into_raw()`, and then
> > > derive a pointer from that, you change the provenance, therefore the
> > > below code would generate UB:
> > >
> > >         // cannot mutably borrow because of clone.
> > >         let ptr = unsafe { &*Arc::into_raw(arc) } as *const i32;
> > >
> > >         let arc = unsafe { Arc::from_raw(ptr) };
> > >
> > >
> > > (playground code snippet for this example)
> > >
> > >         https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=15e051db46c3886b29ed02e579562278
> > >
> > > As you already know, the whole thing about pointers/references here is
> > > passing the value to the callback and the callback can "reconstruct" the
> > > data, in such a case, reborrowing in the middle of the chain into a
> > > reference is not necessary, and as the above shows, it can be
> > > problematic.
> > >
> > > Hope this could be helpful.
> > >
> > > Regards,
> > > Boqun
> >
> > Thanks for the explanation. I think where I'm still confused is that
> > provenance is to pointers as the type system is to references. In
> > other words, it's not clear to me how using pointers solves the
> > problem of wanting to write through an Arc. Is the idea that the
> > pointer inside the Arc has write provenance, and that by doing pointer
> > offsets instead of going through references we get to break rules
> > about mutability?
>
> Sort of, but we don't actually break any rule here, because pointer are
> supposed to be unsafe to dereference ;-)
>
> Regards,
> Boqun

Doesn't this mean that any holder of Arc<HasHrTimer> can obtain a
pointer to the timer, and interact with it concurrently with other
holders? Isn't this a problem?

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

[Boqun Feng]

On Fri, Feb 21, 2025 at 08:08:09PM -0500, Tamir Duberstein wrote:
> On Fri, Feb 21, 2025 at 5:37 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Fri, Feb 21, 2025 at 02:46:07PM -0500, Tamir Duberstein wrote:
> > > On Fri, Feb 21, 2025 at 10:19 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > >
> > > > On Fri, Feb 21, 2025 at 09:46:08AM -0500, Tamir Duberstein wrote:
> > > > > On Fri, Feb 21, 2025 at 9:40 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> > > > > >
> > > > > > Hmm... if you mean:
> > > > > >
> > > > > > trait HasHrTimer {
> > > > > >     unsafe fn start(&self, expires: Ktime) {
> > > > > >         ...
> > > > > >     }
> > > > > > }
> > > > > >
> > > > > > Then it'll be problematic because the pointer derived from `&self`
> > > > > > doesn't have write provenance, therefore in a timer callback, the
> > > > > > pointer cannot be used for write, which means for example you cannot
> > > > > > convert the pointer back into a `Pin<Box<HasTimer>>`.
> > > > > >
> > > > > > To answer Tamir's question, pointers are heavily used here because we
> > > > > > need to preserve the provenance.
> > > > >
> > > > > Wouldn't the natural implication be that &mut self is needed? Maybe
> > > >
> > > > For an `Arc<HasTimer>`, you cannot get `&mut self`.
> > > >
> > > > > you can help me understand why pointers can express a contract that
> > > > > references can't?
> > > >
> > > > I assume you already know what a pointer provenance is?
> > > >
> > > >         http://doc.rust-lang.org/std/ptr/index.html#provenance
> > > >
> > > > Passing a pointer (including offset operation on it) preserves the
> > > > provenance (determined as derive time), however, deriving a pointer from
> > > > a reference gives the pointer a provenance based on the reference type.
> > > > For example, let's say we have an `Arc<i32>` and a clone:
> > > >
> > > >         let arc = Arc::new(42);
> > > >         let clone = arc.clone();
> > > >
> > > > you can obviously do a into_raw() + from_raw() pair:
> > > >
> > > >         let ptr = Arc::into_raw(arc);
> > > >         let arc = unsafe { Arc::from_raw(arc) };
> > > >
> > > > however, if you create a reference based on `Arc::into_raw()`, and then
> > > > derive a pointer from that, you change the provenance, therefore the
> > > > below code would generate UB:
> > > >
> > > >         // cannot mutably borrow because of clone.
> > > >         let ptr = unsafe { &*Arc::into_raw(arc) } as *const i32;
> > > >
> > > >         let arc = unsafe { Arc::from_raw(ptr) };
> > > >
> > > >
> > > > (playground code snippet for this example)
> > > >
> > > >         https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=15e051db46c3886b29ed02e579562278
> > > >
> > > > As you already know, the whole thing about pointers/references here is
> > > > passing the value to the callback and the callback can "reconstruct" the
> > > > data, in such a case, reborrowing in the middle of the chain into a
> > > > reference is not necessary, and as the above shows, it can be
> > > > problematic.
> > > >
> > > > Hope this could be helpful.
> > > >
> > > > Regards,
> > > > Boqun
> > >
> > > Thanks for the explanation. I think where I'm still confused is that
> > > provenance is to pointers as the type system is to references. In
> > > other words, it's not clear to me how using pointers solves the
> > > problem of wanting to write through an Arc. Is the idea that the
> > > pointer inside the Arc has write provenance, and that by doing pointer
> > > offsets instead of going through references we get to break rules
> > > about mutability?
> >
> > Sort of, but we don't actually break any rule here, because pointer are
> > supposed to be unsafe to dereference ;-)
> >
> > Regards,
> > Boqun
> 
> Doesn't this mean that any holder of Arc<HasHrTimer> can obtain a
> pointer to the timer, and interact with it concurrently with other
> holders? Isn't this a problem?

If so, do you have a case in mind? I.e. could you show two concurrent
user of `Arc<HasHrTimer>` end up racing with each other? Note that
hrtimer function on C side usually provides some locking mechanism
therefore concurrently calling won't cause a problem.

Regards,
Boqun

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

[Andreas Hindborg]

"Boqun Feng" <boqun.feng@gmail.com> writes:

> On Fri, Feb 21, 2025 at 09:46:08AM -0500, Tamir Duberstein wrote:
>> On Fri, Feb 21, 2025 at 9:40 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>> >
>> > Hmm... if you mean:
>> >
>> > trait HasHrTimer {
>> >     unsafe fn start(&self, expires: Ktime) {
>> >         ...
>> >     }
>> > }
>> >
>> > Then it'll be problematic because the pointer derived from `&self`
>> > doesn't have write provenance, therefore in a timer callback, the
>> > pointer cannot be used for write, which means for example you cannot
>> > convert the pointer back into a `Pin<Box<HasTimer>>`.
>> >
>> > To answer Tamir's question, pointers are heavily used here because we
>> > need to preserve the provenance.
>>
>> Wouldn't the natural implication be that &mut self is needed? Maybe
>
> For an `Arc<HasTimer>`, you cannot get `&mut self`.
>
>> you can help me understand why pointers can express a contract that
>> references can't?
>
> I assume you already know what a pointer provenance is?
>
> 	http://doc.rust-lang.org/std/ptr/index.html#provenance
>
> Passing a pointer (including offset operation on it) preserves the
> provenance (determined as derive time), however, deriving a pointer from
> a reference gives the pointer a provenance based on the reference type.
> For example, let's say we have an `Arc<i32>` and a clone:
>
> 	let arc = Arc::new(42);
> 	let clone = arc.clone();
>
> you can obviously do a into_raw() + from_raw() pair:
>
> 	let ptr = Arc::into_raw(arc);
> 	let arc = unsafe { Arc::from_raw(arc) };
>
> however, if you create a reference based on `Arc::into_raw()`, and then
> derive a pointer from that, you change the provenance,

In this case, the pointer will have the pointer of `Arc::into_raw()`
will have the provenance of the original reference. When you turn that
pointer back into a reference, won't the reference inherit the
provenance of the pointer, which is the same as the original reference?

As I read the docs, getting a reference to a `Timer` from a reference to
a `<MyType as HasHrTimer>` by converting `&MyType` to a `*const MyType`,
doing a `ptr.cast::<u8>().add(offset).cast::<HrTimer<T>>()` and
converting that pointer to a reference should be fine? The final pointer
before converting back to a reference will still have provenance of the
original reference. Converting to a reference at the end will shrink the
provenance, but it is still fine.

Going from a `&HrTimer<T>` to a `&T` is a problem, because that would
require offset outside spatial permission of pointer provenance, and it
would require increasing the size of the spatial permission.

Is this correctly understood?

> therefore the
> below code would generate UB:
>
> 	// cannot mutably borrow because of clone.
> 	let ptr = unsafe { &*Arc::into_raw(arc) } as *const i32;
>
> 	let arc = unsafe { Arc::from_raw(ptr) };
>
>
> (playground code snippet for this example)
>
> 	https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=15e051db46c3886b29ed02e579562278
>
> As you already know, the whole thing about pointers/references here is
> passing the value to the callback and the callback can "reconstruct" the
> data, in such a case, reborrowing in the middle of the chain into a
> reference is not necessary, and as the above shows, it can be
> problematic.

Thanks for bringing this up, I forgot about it.


Best regards,
Andreas Hindborg

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

[Benno Lossin]

On 21.02.25 12:39, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
> 
>> On 21.02.25 10:17, Andreas Hindborg wrote:
>>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>>
>>>> On 18.02.25 14:27, Andreas Hindborg wrote:
>>>>> +/// Operational mode of [`HrTimer`].
>>>>> +#[derive(Clone, Copy)]
>>>>> +pub enum HrTimerMode {
>>>>> +    /// Timer expires at the given expiration time.
>>>>> +    Absolute,
>>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>>> +    Relative,
>>>>> +    /// Timer does not move between CPU cores.
>>>>> +    Pinned,
>>>>> +    /// Timer handler is executed in soft irq context.
>>>>> +    Soft,
>>>>> +    /// Timer handler is executed in hard irq context.
>>>>> +    Hard,
>>>>> +    /// Timer expires at the given expiration time.
>>>>> +    /// Timer does not move between CPU cores.
>>>>> +    AbsolutePinned,
>>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>>> +    /// Timer does not move between CPU cores.
>>>>> +    RelativePinned,
>>>>> +    /// Timer expires at the given expiration time.
>>>>> +    /// Timer handler is executed in soft irq context.
>>>>> +    AbsoluteSoft,
>>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>>> +    /// Timer handler is executed in soft irq context.
>>>>> +    RelativeSoft,
>>>>> +    /// Timer expires at the given expiration time.
>>>>> +    /// Timer does not move between CPU cores.
>>>>> +    /// Timer handler is executed in soft irq context.
>>>>> +    AbsolutePinnedSoft,
>>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>>> +    /// Timer does not move between CPU cores.
>>>>> +    /// Timer handler is executed in soft irq context.
>>>>> +    RelativePinnedSoft,
>>>>> +    /// Timer expires at the given expiration time.
>>>>> +    /// Timer handler is executed in hard irq context.
>>>>> +    AbsoluteHard,
>>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>>> +    /// Timer handler is executed in hard irq context.
>>>>> +    RelativeHard,
>>>>> +    /// Timer expires at the given expiration time.
>>>>> +    /// Timer does not move between CPU cores.
>>>>> +    /// Timer handler is executed in hard irq context.
>>>>> +    AbsolutePinnedHard,
>>>>> +    /// Timer expires after the given expiration time interpreted as a duration from now.
>>>>> +    /// Timer does not move between CPU cores.
>>>>> +    /// Timer handler is executed in hard irq context.
>>>>> +    RelativePinnedHard,
>>>>> +}
>>>>
>>>> At some point we probably want to move this to bitfields, or do you
>>>> think it's better to keep it like this?
>>>
>>> Yes, eventually the would transition. The main difficulty is that not
>>> all flag combinations are legal, and the zero value is also a flag.
>>> There was some promising work being shared on Zulip for this [1], but I
>>> don't think it is completed yet. Added Timothy to CC.
>>>
>>> [1] https://rust-for-linux.zulipchat.com/#narrow/channel/291565-Help/topic/Best.20way.20to.20handle.20enum.2Fflags.20situation
>>
>> Ah yeah I remember that. And also the complication about certain
>> combinations not being allowed.
>>
>>>>> +
>>>>> +impl From<HrTimerMode> for bindings::hrtimer_mode {
>>>>> +    fn from(value: HrTimerMode) -> Self {
>>>>> +        use bindings::*;
>>>>> +        match value {
>>>>> +            HrTimerMode::Absolute => hrtimer_mode_HRTIMER_MODE_ABS,
>>>>> +            HrTimerMode::Relative => hrtimer_mode_HRTIMER_MODE_REL,
>>>>> +            HrTimerMode::Pinned => hrtimer_mode_HRTIMER_MODE_PINNED,
>>>>> +            HrTimerMode::Soft => hrtimer_mode_HRTIMER_MODE_SOFT,
>>>>> +            HrTimerMode::Hard => hrtimer_mode_HRTIMER_MODE_HARD,
>>>>> +            HrTimerMode::AbsolutePinned => hrtimer_mode_HRTIMER_MODE_ABS_PINNED,
>>>>> +            HrTimerMode::RelativePinned => hrtimer_mode_HRTIMER_MODE_REL_PINNED,
>>>>> +            HrTimerMode::AbsoluteSoft => hrtimer_mode_HRTIMER_MODE_ABS_SOFT,
>>>>> +            HrTimerMode::RelativeSoft => hrtimer_mode_HRTIMER_MODE_REL_SOFT,
>>>>> +            HrTimerMode::AbsolutePinnedSoft => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_SOFT,
>>>>> +            HrTimerMode::RelativePinnedSoft => hrtimer_mode_HRTIMER_MODE_REL_PINNED_SOFT,
>>>>> +            HrTimerMode::AbsoluteHard => hrtimer_mode_HRTIMER_MODE_ABS_HARD,
>>>>> +            HrTimerMode::RelativeHard => hrtimer_mode_HRTIMER_MODE_REL_HARD,
>>>>> +            HrTimerMode::AbsolutePinnedHard => hrtimer_mode_HRTIMER_MODE_ABS_PINNED_HARD,
>>>>> +            HrTimerMode::RelativePinnedHard => hrtimer_mode_HRTIMER_MODE_REL_PINNED_HARD,
>>>>> +        }
>>>>> +    }
>>>>> +}
>>>>> +
>>>>> +impl From<HrTimerMode> for u64 {
>>>>> +    fn from(value: HrTimerMode) -> Self {
>>>>> +        Into::<bindings::hrtimer_mode>::into(value) as u64
>>>>> +    }
>>>>> +}
>>>>
>>>> Hmm do drivers really need these impls? If not, then you could also just
>>>> have a private function that does the conversion and use it only in the
>>>> abstraction layer.
>>>
>>> Similar to the other impls you commented on, I can move them private. I
>>> would prefer using `From` rather than some other function.
>>
>> What's the reason for you preferring `From`? I don't think it's
>> important to forbid access from the drivers, but if it's unnecessary,
>> why would we give them access in the first place?
> 
> TIL trait implementations cannot be hidden.

Yeah trait impls are always public.

> I like `From` because every single rust developer seeing a `From`
> implementation will immediately know what it does. It is more
> idiomatic in that way than having another conversion function. I think
> using existing traits with well defined semantics is preferable to a
> local function.

Well yes, but that only holds if other people need to use it.

> But since driver implementer do not need it, I'm undecided. If you want
> them converted to a private function I can do that, for all the ones you
> called out. But I am also OK with keeping as is. You decide.

I think it's better to keep these enums somewhat opaque.

---
Cheers,
Benno

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

[Benno Lossin]

On 21.02.25 13:17, Andreas Hindborg wrote:
> "Benno Lossin" <benno.lossin@proton.me> writes:
> 
>> On 21.02.25 11:15, Andreas Hindborg wrote:
>>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>>
>>>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>>>
>>>>> On 18.02.25 14:27, Andreas Hindborg wrote:
>>>>>> +pub trait HrTimerCallback {
>>>>>> +    /// The type whose [`RawHrTimerCallback::run`] method will be invoked when
>>>>>> +    /// the timer expires.
>>>>>> +    type CallbackTarget<'a>: RawHrTimerCallback;
>>>>>> +
>>>>>> +    /// This type is passed to the timer callback function. It may be a borrow
>>>>>> +    /// of [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
>>>>>> +    /// implementation can guarantee exclusive access to the target during timer
>>>>>
>>>>> Technically "exclusive" access is correct if the `CallbackTarget` is
>>>>> `Pin<&Self>`, since you will get exclusive access to a `Pin<&Self>`, but
>>>>> it might confuse people, because there can be multiple `Pin<&Self>`. So
>>>>> I would just drop the word "exclusive" here.
>>>>
>>>> Yes, maybe it should be "shared or exclusive access, depending on the type"?
>>>>
>>>>>
>>>>>> +    /// handler execution.
>>>>>> +    type CallbackTargetParameter<'a>;
>>>>>
>>>>> Also why can't this type be an associated type of `HrTimerPointer`?
>>>>> Since this seems to always be constrained in the impls of
>>>>> `RawHrTimerCallback`.
>>>>
>>>> That might be a nice improvement, I'll try that out.
>>>
>>> Looking closer at this, I don't see how to achieve this. We need access
>>> to the type here, because it is used in the signature of `run`.
>>> `HrTimerCallback` has no bounds on it, and that is nice. If we want to
>>> move these associated types, we have to introduce a bound here.
>>>
>>> We need to be generic over the type of the parameter to `run`, and by
>>> the time the user implements this trait, the type must be known and so
>>> the user has to specify somehow.
>>
>> I think if you put the associated type on the `RawHrTimerCallback`
>> trait, it should work.
> 
> What would be the signature of `HrTimerCallback::run` in that case?

    /// 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 CallbackTarget: RawHrTimerCallback;
    
        /// Called by the timer logic when the timer fires.
        fn run(this: <Self::CallbackTarget as RawHrTimerCallback>::CallbackTargetParameter<'_>)
        where
            Self: Sized;
        // also, why does this Sized bound exist here?
    }

That should work, but now the names seem a bit long... How about:
- CallbackTarget -> Pointer
  - and then you can also call this the "smart pointer whose `run`
    method will be invoked when..." in the docs
- CallbackTargetParameter -> CallbackParameter

---
Cheers,
Benno

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

[Andreas Hindborg]

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

> On 21.02.25 13:17, Andreas Hindborg wrote:
>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>
>>> On 21.02.25 11:15, Andreas Hindborg wrote:
>>>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>>>
>>>>> "Benno Lossin" <benno.lossin@proton.me> writes:
>>>>>
>>>>>> On 18.02.25 14:27, Andreas Hindborg wrote:
>>>>>>> +pub trait HrTimerCallback {
>>>>>>> +    /// The type whose [`RawHrTimerCallback::run`] method will be invoked when
>>>>>>> +    /// the timer expires.
>>>>>>> +    type CallbackTarget<'a>: RawHrTimerCallback;
>>>>>>> +
>>>>>>> +    /// This type is passed to the timer callback function. It may be a borrow
>>>>>>> +    /// of [`Self::CallbackTarget`], or it may be `Self::CallbackTarget` if the
>>>>>>> +    /// implementation can guarantee exclusive access to the target during timer
>>>>>>
>>>>>> Technically "exclusive" access is correct if the `CallbackTarget` is
>>>>>> `Pin<&Self>`, since you will get exclusive access to a `Pin<&Self>`, but
>>>>>> it might confuse people, because there can be multiple `Pin<&Self>`. So
>>>>>> I would just drop the word "exclusive" here.
>>>>>
>>>>> Yes, maybe it should be "shared or exclusive access, depending on the type"?
>>>>>
>>>>>>
>>>>>>> +    /// handler execution.
>>>>>>> +    type CallbackTargetParameter<'a>;
>>>>>>
>>>>>> Also why can't this type be an associated type of `HrTimerPointer`?
>>>>>> Since this seems to always be constrained in the impls of
>>>>>> `RawHrTimerCallback`.
>>>>>
>>>>> That might be a nice improvement, I'll try that out.
>>>>
>>>> Looking closer at this, I don't see how to achieve this. We need access
>>>> to the type here, because it is used in the signature of `run`.
>>>> `HrTimerCallback` has no bounds on it, and that is nice. If we want to
>>>> move these associated types, we have to introduce a bound here.
>>>>
>>>> We need to be generic over the type of the parameter to `run`, and by
>>>> the time the user implements this trait, the type must be known and so
>>>> the user has to specify somehow.
>>>
>>> I think if you put the associated type on the `RawHrTimerCallback`
>>> trait, it should work.
>>
>> What would be the signature of `HrTimerCallback::run` in that case?
>
>     /// 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 CallbackTarget: RawHrTimerCallback;
>
>         /// Called by the timer logic when the timer fires.
>         fn run(this: <Self::CallbackTarget as RawHrTimerCallback>::CallbackTargetParameter<'_>)
>         where
>             Self: Sized;
>         // also, why does this Sized bound exist here?
>     }
>
> That should work, but now the names seem a bit long... How about:
> - CallbackTarget -> Pointer
>   - and then you can also call this the "smart pointer whose `run`
>     method will be invoked when..." in the docs
> - CallbackTargetParameter -> CallbackParameter

Right, that works. I thought you wanted to move both the associated
types.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

Andreas Hindborg <a.hindborg@kernel.org> writes:

> "Boqun Feng" <boqun.feng@gmail.com> writes:
>
>> On Fri, Feb 21, 2025 at 09:46:08AM -0500, Tamir Duberstein wrote:
>>> On Fri, Feb 21, 2025 at 9:40 AM Boqun Feng <boqun.feng@gmail.com> wrote:
>>> >
>>> > Hmm... if you mean:
>>> >
>>> > trait HasHrTimer {
>>> >     unsafe fn start(&self, expires: Ktime) {
>>> >         ...
>>> >     }
>>> > }
>>> >
>>> > Then it'll be problematic because the pointer derived from `&self`
>>> > doesn't have write provenance, therefore in a timer callback, the
>>> > pointer cannot be used for write, which means for example you cannot
>>> > convert the pointer back into a `Pin<Box<HasTimer>>`.
>>> >
>>> > To answer Tamir's question, pointers are heavily used here because we
>>> > need to preserve the provenance.
>>>
>>> Wouldn't the natural implication be that &mut self is needed? Maybe
>>
>> For an `Arc<HasTimer>`, you cannot get `&mut self`.
>>
>>> you can help me understand why pointers can express a contract that
>>> references can't?
>>
>> I assume you already know what a pointer provenance is?
>>
>> 	http://doc.rust-lang.org/std/ptr/index.html#provenance
>>
>> Passing a pointer (including offset operation on it) preserves the
>> provenance (determined as derive time), however, deriving a pointer from
>> a reference gives the pointer a provenance based on the reference type.
>> For example, let's say we have an `Arc<i32>` and a clone:
>>
>> 	let arc = Arc::new(42);
>> 	let clone = arc.clone();
>>
>> you can obviously do a into_raw() + from_raw() pair:
>>
>> 	let ptr = Arc::into_raw(arc);
>> 	let arc = unsafe { Arc::from_raw(arc) };
>>
>> however, if you create a reference based on `Arc::into_raw()`, and then
>> derive a pointer from that, you change the provenance,
>
> In this case, the pointer will have the pointer of `Arc::into_raw()`
> will have the provenance of the original reference. When you turn that
> pointer back into a reference, won't the reference inherit the
> provenance of the pointer, which is the same as the original reference?
>
> As I read the docs, getting a reference to a `Timer` from a reference to
> a `<MyType as HasHrTimer>` by converting `&MyType` to a `*const MyType`,
> doing a `ptr.cast::<u8>().add(offset).cast::<HrTimer<T>>()` and
> converting that pointer to a reference should be fine? The final pointer
> before converting back to a reference will still have provenance of the
> original reference. Converting to a reference at the end will shrink the
> provenance, but it is still fine.
>
> Going from a `&HrTimer<T>` to a `&T` is a problem, because that would
> require offset outside spatial permission of pointer provenance, and it
> would require increasing the size of the spatial permission.
>
> Is this correctly understood?

How does provenance work across language boundaries? Should we actually
use `with_addr` [1] when we get pointers from C round trips?


Best regards,
Andreas Hindborg



[1] https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.with_addr

Re: [PATCH v8 00/14] hrtimer Rust API

[Miguel Ojeda]

On Fri, Feb 21, 2025 at 12:20 PM Frederic Weisbecker
<frederic@kernel.org> wrote:
>
> I was thinking the patchset would be better routed towards the Rust tree?
>
> How do you guys proceed usually with bindings tree maintainance?

So far, what we have been doing is ask maintainers, first, if they
would be willing take the patches themselves -- they are the experts
of the subsystem, know what changes are incoming, etc. Some subsystems
have done this (e.g. KUnit). That is ideal, because the goal is to
scale and allows maintainers to be in full control.

Of course, sometimes maintainers are not fully comfortable doing that,
since they may not have the bandwidth, or the setup, or the Rust
knowledge. In those cases, we typically ask if they would be willing
to have a co-maintainer (i.e. in their entry, e.g. like locking did),
or a sub-maintainer (i.e. in a new entry, e.g. like block did), that
would take care of the bulk of the work from them.

I think that is a nice middle-ground -- the advantage of doing it like
that is that you get the benefits of knowing best what is going on
without too much work (hopefully), and it may allow you to get more
and more involved over time and confident on what is going on with the
Rust callers, typical issues that appear, etc. Plus the sub-maintainer
gets to learn more about the subsystem, its timelines, procedures,
etc., which you may welcome (if you are looking for new people to get
involved).

I think that would be a nice middle-ground. As far as I understand,
Andreas would be happy to commit to maintain the Rust side as a
sub-maintainer (for instance). He would also need to make sure the
tree builds properly with Rust enabled and so on. He already does
something similar for Jens. Would that work for you?

You could take the patches directly with his RoBs or Acked-bys, for
instance. Or perhaps it makes more sense to take PRs from him (on the
Rust code only, of course), to save you more work. Andreas does not
send PRs to anyone yet, but I think it would be a good time for him to
start learning how to apply patches himself etc.

If not, then the last fallback would be putting it in the Rust tree as
a sub-entry or similar.

I hope that clarifies (and thanks whatever you decide!).

Cheers,
Miguel

Re: [PATCH v8 00/14] hrtimer Rust API

[Miguel Ojeda]

On Sat, Feb 22, 2025 at 2:04 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> send PRs to anyone yet, but I think it would be a good time for him to
> start learning how to apply patches himself etc.

By this, I meant taking care of the usual maintainer bits, not just
the actual "applying" (which he of course knows how to do!).

If you decide to go for PRs from him to you, I am happy to help
getting him up to speed if needed, too.

Cheers,
Miguel

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

[Andreas Hindborg]

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

> On Fri, Feb 21, 2025 at 8:29 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Andreas Hindborg <a.hindborg@kernel.org> writes:
>>
>> > "Tamir Duberstein" <tamird@gmail.com> writes:
>> >
>> >> On Thu, Feb 20, 2025 at 4:19 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >>>
>> >>> "Tamir Duberstein" <tamird@gmail.com> writes:
>> >>>
>> >>> > On Tue, Feb 18, 2025 at 8:29 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> [...]
>>
>> >>> >> +    /// Get a pointer to the contained `bindings::hrtimer`.
>> >>> >> +    ///
>> >>> >> +    /// # Safety
>> >>> >> +    ///
>> >>> >> +    /// `ptr` must point to a live allocation of at least the size of `Self`.
>> >>> >> +    unsafe fn raw_get(ptr: *const Self) -> *mut bindings::hrtimer {
>> >>> >> +        // SAFETY: The field projection to `timer` does not go out of bounds,
>> >>> >> +        // because the caller of this function promises that `ptr` points to an
>> >>> >> +        // allocation of at least the size of `Self`.
>> >>> >> +        unsafe { Opaque::raw_get(core::ptr::addr_of!((*ptr).timer)) }
>> >>> >> +    }
>> >>> >
>> >>> > Can you help me understand why the various functions here operate on
>> >>> > *const Self? I understand the need to obtain a C pointer to interact
>> >>> > with bindings, but I don't understand why we're dealing in raw
>> >>> > pointers to the abstraction rather than references.
>> >>>
>> >>> We cannot reference the `bindings::hrtimer` without wrapping it in
>> >>> `Opaque`. This would be the primary reason. At other times, we cannot
>> >>> produce references because we might not be able to prove that we satisfy
>> >>> the safety requirements for turning a pointer into a reference. If we
>> >>> are just doing offset arithmetic anyway, we don't need a reference.
>> >>
>> >> Why do we have a pointer, rather than a reference, to Self in the
>> >> first place? I think this is the key thing I don't understand.
>> >
>> > Perhaps it makes more sense if you look at the context. One of the entry
>> > points to `HrTimer::raw_get` is via `<ArcHrTimerHandle as
>> > HrTimerHandle>::cancel`. This user facing method takes `&mut self`. The
>> > handle contains an arc to a type that contains a `Timer` and implements
>> > `HasHrTImer`. To get to the timer, we need to do pointer manipulation.
>> > We only know how to get the `Timer` field via the `OFFSET`. The natural
>> > return value from the offset operation is a raw pointer. Rather than
>> > convert back to a reference, we stay in pointer land when we call
>> > `HrTimer::raw_cancel`, because we need a pointer to the
>> > `bindings::hrtimer` anyway, not a reference.
>>
>> I changed `HasHrTimer::start` to take a reference, and I think that
>> makes sense 👍 Taking an `impl AsRef` does not work out when `Self` is
>> `Pin<&T>`. I'll go over the whole thing and see of other places could
>> benefit.
>
> Can you elaborate please? Pin<&T>::get_ref returns &T, which gets you
> to the AsRef you need, no?

Right, that works. But if I pass in &self it's not really needed. You
can see for yourself in next version.

Hopefully I did not break any provenance rules.


Best regards,
Andreas Hindborg

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

[Tamir Duberstein]

On Sat, Feb 22, 2025 at 1:26 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Right, that works. But if I pass in &self it's not really needed. You
> can see for yourself in next version.
>
> Hopefully I did not break any provenance rules.

Thank you for being willing to iterate!
Tamir

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

[Boqun Feng]

(we had some discussion on zulip: 

	https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Timer.20support/near/501291973

, replying to restate what I've said for list discussion)

On Sat, Feb 22, 2025 at 12:40:23PM +0100, Andreas Hindborg wrote:
> Andreas Hindborg <a.hindborg@kernel.org> writes:
> 
> > "Boqun Feng" <boqun.feng@gmail.com> writes:
> >
> >> On Fri, Feb 21, 2025 at 09:46:08AM -0500, Tamir Duberstein wrote:
> >>> On Fri, Feb 21, 2025 at 9:40 AM Boqun Feng <boqun.feng@gmail.com> wrote:
> >>> >
> >>> > Hmm... if you mean:
> >>> >
> >>> > trait HasHrTimer {
> >>> >     unsafe fn start(&self, expires: Ktime) {
> >>> >         ...
> >>> >     }
> >>> > }
> >>> >
> >>> > Then it'll be problematic because the pointer derived from `&self`
> >>> > doesn't have write provenance, therefore in a timer callback, the
> >>> > pointer cannot be used for write, which means for example you cannot
> >>> > convert the pointer back into a `Pin<Box<HasTimer>>`.
> >>> >
> >>> > To answer Tamir's question, pointers are heavily used here because we
> >>> > need to preserve the provenance.
> >>>
> >>> Wouldn't the natural implication be that &mut self is needed? Maybe
> >>
> >> For an `Arc<HasTimer>`, you cannot get `&mut self`.
> >>
> >>> you can help me understand why pointers can express a contract that
> >>> references can't?
> >>
> >> I assume you already know what a pointer provenance is?
> >>
> >> 	http://doc.rust-lang.org/std/ptr/index.html#provenance
> >>
> >> Passing a pointer (including offset operation on it) preserves the
> >> provenance (determined as derive time), however, deriving a pointer from
> >> a reference gives the pointer a provenance based on the reference type.
> >> For example, let's say we have an `Arc<i32>` and a clone:
> >>
> >> 	let arc = Arc::new(42);
> >> 	let clone = arc.clone();
> >>
> >> you can obviously do a into_raw() + from_raw() pair:
> >>
> >> 	let ptr = Arc::into_raw(arc);
> >> 	let arc = unsafe { Arc::from_raw(arc) };
> >>
> >> however, if you create a reference based on `Arc::into_raw()`, and then
> >> derive a pointer from that, you change the provenance,
> >
> > In this case, the pointer will have the pointer of `Arc::into_raw()`
> > will have the provenance of the original reference. When you turn that
> > pointer back into a reference, won't the reference inherit the
> > provenance of the pointer, which is the same as the original reference?

If the reference is an immutable one, then it won't have the write
provenance, and if the reference is only to the `Timer` field, it won't
have the provenance for the whole `HasHrTimer` struct.

> >
> > As I read the docs, getting a reference to a `Timer` from a reference to
> > a `<MyType as HasHrTimer>` by converting `&MyType` to a `*const MyType`,
> > doing a `ptr.cast::<u8>().add(offset).cast::<HrTimer<T>>()` and
> > converting that pointer to a reference should be fine? The final pointer
> > before converting back to a reference will still have provenance of the
> > original reference. Converting to a reference at the end will shrink the
> > provenance, but it is still fine.
> >
> > Going from a `&HrTimer<T>` to a `&T` is a problem, because that would
> > require offset outside spatial permission of pointer provenance, and it
> > would require increasing the size of the spatial permission.
> >
> > Is this correctly understood?

Right. Also in this case, since eventually, we need to convert to a
`Arc<T>`, the pointer from `&HrTimer` or `&T` doesn't work because write
provenance is required.

> 
> How does provenance work across language boundaries? Should we actually
> use `with_addr` [1] when we get pointers from C round trips?
> 

I would suggest we assume that C can pass the provenance via pointers,
that is, if we pass a pointer value to C and eventually get it back from
C, we should treat it as having the same provenance. The rationale is 1)
considering LTO, C may also have the same or similar provenance model
(e.g [1]), and 2) the C part may get rewritten into Rust in the future,
the correctness about provenance should not be changed by that
possibility. `with_addr()` is not necessary then, because pointers
should have the proper provenance when it passes Rust/C boundary.

[1]: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3005.pdf

Regards,
Boqun

> 
> Best regards,
> Andreas Hindborg
> 
> 
> 
> [1] https://doc.rust-lang.org/stable/std/primitive.pointer.html#method.with_addr
> 

Re: [PATCH v8 00/14] hrtimer Rust API

[Frederic Weisbecker]

Le Sat, Feb 22, 2025 at 02:04:16PM +0100, Miguel Ojeda a écrit :
> On Fri, Feb 21, 2025 at 12:20 PM Frederic Weisbecker
> <frederic@kernel.org> wrote:
> >
> > I was thinking the patchset would be better routed towards the Rust tree?
> >
> > How do you guys proceed usually with bindings tree maintainance?
> 
> So far, what we have been doing is ask maintainers, first, if they
> would be willing take the patches themselves -- they are the experts
> of the subsystem, know what changes are incoming, etc. Some subsystems
> have done this (e.g. KUnit). That is ideal, because the goal is to
> scale and allows maintainers to be in full control.
> 
> Of course, sometimes maintainers are not fully comfortable doing that,
> since they may not have the bandwidth, or the setup, or the Rust
> knowledge. In those cases, we typically ask if they would be willing
> to have a co-maintainer (i.e. in their entry, e.g. like locking did),
> or a sub-maintainer (i.e. in a new entry, e.g. like block did), that
> would take care of the bulk of the work from them.
> 
> I think that is a nice middle-ground -- the advantage of doing it like
> that is that you get the benefits of knowing best what is going on
> without too much work (hopefully), and it may allow you to get more
> and more involved over time and confident on what is going on with the
> Rust callers, typical issues that appear, etc. Plus the sub-maintainer
> gets to learn more about the subsystem, its timelines, procedures,
> etc., which you may welcome (if you are looking for new people to get
> involved).
> 
> I think that would be a nice middle-ground. As far as I understand,
> Andreas would be happy to commit to maintain the Rust side as a
> sub-maintainer (for instance). He would also need to make sure the
> tree builds properly with Rust enabled and so on. He already does
> something similar for Jens. Would that work for you?
> 
> You could take the patches directly with his RoBs or Acked-bys, for
> instance. Or perhaps it makes more sense to take PRs from him (on the
> Rust code only, of course), to save you more work. Andreas does not
> send PRs to anyone yet, but I think it would be a good time for him to
> start learning how to apply patches himself etc.
> 
> If not, then the last fallback would be putting it in the Rust tree as
> a sub-entry or similar.
> 
> I hope that clarifies (and thanks whatever you decide!).

Yes this clarifies a lot, thanks for the detailed options.
I think it's preferrable that you guys maintain it because you
are the experts with these Rust bindings and you'll be much
more flexible committing and pushing to your own tree instead
of waiting on us lagging to comprehend the content of each
pull requests.

Just keep us in Cc so we stay in touch with what's going on.

Thanks a lot!

> 
> Cheers,
> Miguel