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

[Benno Lossin <benno.lossin@proton.me>]

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