Re: [PATCH v6 4/7] rust: hrtimer: Add HrTimerCallbackContext and ::forward()

[Andreas Hindborg <a.hindborg@kernel.org>]

"Lyude Paul" <lyude@redhat.com> writes:

> With Linux's hrtimer API, there's a number of methods that can only be
> called in two situations:
>
> * When we have exclusive access to the hrtimer and it is not currently
>   active
> * When we're within the context of an hrtimer callback context
>
> This commit handles the second situation and implements hrtimer_forward()
> support in the context of a timer callback. We do this by introducing a
> HrTimerCallbackContext type which is provided to users during the
> RawHrTimerCallback::run() callback, and then add a forward() function to
> the type.
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
>
> ---
> V2:
> * Improve SAFETY comments for HrTimerCallbackContext uses (I forgot to
>   mention that we're within RawHrTimerCallback::run()
> * Split forward into forward() and raw_forward() since we're going to have
>   two contexts that we can call forward() from now.
> * Clarify contexts in which certain hrtimer methods can be called.
> * Make sure that we use a mutable reference for forward() here - just in
>   case :).
> * Rename interval to duration
> V3:
> * Rename duration -back- to interval (now that I actually have read
>   hrtimer_forward's source, interval does make more sense than duration
>   considering the fact we return the number of overruns that occurred
>   according to the given interval).
> * Rewrite documentation a bit (re: Andreas)
> V6:
> * Move reference to HrTimerCallbackContext::forward() in HrTimer::forward()
>   comments into this commit so rustdoc doesn't fail.
> * Deduplicate documentation for HrTimerCallbackContext::forward()
>
> Signed-off-by: Lyude Paul <lyude@redhat.com>
> ---
>  rust/kernel/time/hrtimer.rs         | 63 +++++++++++++++++++++++++++--
>  rust/kernel/time/hrtimer/arc.rs     |  9 ++++-
>  rust/kernel/time/hrtimer/pin.rs     |  9 ++++-
>  rust/kernel/time/hrtimer/pin_mut.rs | 12 ++++--
>  rust/kernel/time/hrtimer/tbox.rs    |  9 ++++-
>  5 files changed, 93 insertions(+), 9 deletions(-)
>
> diff --git a/rust/kernel/time/hrtimer.rs b/rust/kernel/time/hrtimer.rs
> index 5ccdd1b0b8021..a8fb176e599e8 100644
> --- a/rust/kernel/time/hrtimer.rs
> +++ b/rust/kernel/time/hrtimer.rs
> @@ -69,7 +69,7 @@
>
>  use super::{ClockSource, Delta, Instant};
>  use crate::{prelude::*, types::Opaque};
> -use core::marker::PhantomData;
> +use core::{marker::PhantomData, ptr::NonNull};
>  use pin_init::PinInit;
>
>  /// A type-alias to refer to the [`Instant<C>`] for a given `T` from [`HrTimer<T>`].
> @@ -196,6 +196,10 @@ unsafe fn raw_forward(self_ptr: *mut Self, now: HrTimerInstant<T>, interval: Del
>      /// expires after `now` and then returns the number of times the timer was forwarded by
>      /// `interval`.
>      ///
> +    /// This function is mainly useful for timer types which can provide exclusive access to the
> +    /// timer when the timer is not running. For forwarding the timer from within the timer callback
> +    /// context, see [`HrTimerCallbackContext::forward()`].
> +    ///
>      /// Returns the number of overruns that occurred as a result of the timer expiry change.
>      pub fn forward(self: Pin<&mut Self>, now: HrTimerInstant<T>, interval: Delta) -> u64
>      where
> @@ -345,9 +349,13 @@ pub trait HrTimerCallback {
>      type Pointer<'a>: RawHrTimerCallback;
>
>      /// Called by the timer logic when the timer fires.
> -    fn run(this: <Self::Pointer<'_> as RawHrTimerCallback>::CallbackTarget<'_>) -> HrTimerRestart
> +    fn run(
> +        this: <Self::Pointer<'_> as RawHrTimerCallback>::CallbackTarget<'_>,
> +        ctx: HrTimerCallbackContext<'_, Self>,
> +    ) -> HrTimerRestart
>      where
> -        Self: Sized;
> +        Self: Sized,
> +        Self: HasHrTimer<Self>;
>  }

You fixed a bug here since v5 (unbounded T). Please remember to put it in the change
log, it makes review easier.


Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg