Message-Id: <DBZIBAUIBYNH.3I8AZG4I8I59E@kernel.org>
Date: Mon, 11 Aug 2025 11:55:56 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "FUJITA Tomonori" <fujita.tomonori@...il.com>
Cc: <a.hindborg@...nel.org>, <alex.gaynor@...il.com>, <ojeda@...nel.org>,
 <aliceryhl@...gle.com>, <anna-maria@...utronix.de>,
 <bjorn3_gh@...tonmail.com>, <boqun.feng@...il.com>, <frederic@...nel.org>,
 <gary@...yguo.net>, <jstultz@...gle.com>, <linux-kernel@...r.kernel.org>,
 <lossin@...nel.org>, <lyude@...hat.com>, <rust-for-linux@...r.kernel.org>,
 <sboyd@...nel.org>, <tglx@...utronix.de>, <tmgross@...ch.edu>,
 <acourbot@...dia.com>, <daniel.almeida@...labora.com>, "Fiona Behrens"
 <me@...enk.dev>
Subject: Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

On Mon Aug 11, 2025 at 6:10 AM CEST, FUJITA Tomonori wrote:
> Add read_poll_timeout functions which poll periodically until a
> condition is met or a timeout is reached.
>
> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
> macro and a simple wrapper for Rust doesn't work. So this implements
> the same functionality in Rust.
>
> The C version uses usleep_range() while the Rust version uses
> fsleep(), which uses the best sleep method so it works with spans that
> usleep_range() doesn't work nicely with.
>
> The sleep_before_read argument isn't supported since there is no user
> for now. It's rarely used in the C version.
>
> read_poll_timeout() can only be used in a nonatomic context. This
> requirement is not checked by these abstractions, but it is intended
> that klint [1] or a similar tool will be used to check it in the
> future.
>
> Link: https://rust-for-linux.com/klint [1]
> Reviewed-by: Fiona Behrens <me@...enk.dev>
> Tested-by: Daniel Almeida <daniel.almeida@...labora.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
> ---
>  rust/kernel/time.rs      |   1 +
>  rust/kernel/time/poll.rs | 104 +++++++++++++++++++++++++++++++++++++++

Hm, are we should this should go in the time module? I does use timekeeping
stuff, but not every user of timekeeping stuff should go under the time module.

This is rather I/O stuff and I'd expect it in rust/kernel/io/poll.rs instead.

> +/// Polls periodically until a condition is met or a timeout is reached.
> +///
> +/// The function repeatedly executes the given operation `op` closure and
> +/// checks its result using the condition closure `cond`.
> +///
> +/// If `cond` returns `true`, the function returns successfully with the result of `op`.
> +/// Otherwise, it waits for a duration specified by `sleep_delta`
> +/// before executing `op` again.
> +///
> +/// This process continues until either `cond` returns `true` or the timeout,
> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
> +///
> +/// This function can only be used in a nonatomic context.
> +///
> +/// # Examples
> +///
> +/// ```no_run
> +/// use kernel::io::Io;
> +/// use kernel::time::{poll::read_poll_timeout, Delta};
> +///
> +/// const HW_READY: u16 = 0x01;
> +///
> +/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result<()> {
> +///     // The `op` closure reads the value of a specific status register.
> +///     let op = || -> Result<u16> { io.try_read16(0x1000) };
> +///
> +///     // The `cond` closure takes a reference to the value returned by `op`
> +///     // and checks whether the hardware is ready.
> +///     let cond = |val: &u16| *val == HW_READY;
> +///
> +///     match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
> +///         Ok(_) => {
> +///             // The hardware is ready. The returned value of the `op`` closure isn't used.
> +///             Ok(())
> +///         }
> +///         Err(e) => Err(e),
> +///     }
> +/// }
> +/// ```

This is exactly what I had in mind, thanks!

> +/// ```rust
> +/// use kernel::sync::{SpinLock, new_spinlock};
> +/// use kernel::time::Delta;
> +/// use kernel::time::poll::read_poll_timeout;
> +///
> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
> +/// let g = lock.lock();
> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));

I assume you want to demonstrate misuse from atomic contex here? I'd rather not
do so. But if we really want that, there should be a *very* obvious comment
about this being wrong somewhere.

> +/// drop(g);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +#[track_caller]
> +pub fn read_poll_timeout<Op, Cond, T>(
> +    mut op: Op,
> +    mut cond: Cond,
> +    sleep_delta: Delta,
> +    timeout_delta: Option<Delta>,
> +) -> Result<T>
> +where
> +    Op: FnMut() -> Result<T>,
> +    Cond: FnMut(&T) -> bool,
> +{
> +    let start: Instant<Monotonic> = Instant::now();
> +    let sleep = !sleep_delta.is_zero();
> +
> +    // Unlike the C version, we always call `might_sleep()`.

I think we should explain why, i.e. the argument about being error prone, clear
separation of read_poll_timeout() and read_poll_timeout_atomic() for klint, etc.
(I also think the C version should not have done this conditionally to begin
with.)

> +    might_sleep();
> +
> +    loop {
> +        let val = op()?;
> +        if cond(&val) {
> +            // Unlike the C version, we immediately return.
> +            // We know the condition is met so we don't need to check again.
> +            return Ok(val);
> +        }
> +        if let Some(timeout_delta) = timeout_delta {
> +            if start.elapsed() > timeout_delta {
> +                // Unlike the C version, we immediately return.
> +                // We have just called `op()` so we don't need to call it again.
> +                return Err(ETIMEDOUT);
> +            }
> +        }
> +        if sleep {
> +            fsleep(sleep_delta);
> +        }
> +        // fsleep() could be busy-wait loop so we always call cpu_relax().
> +        cpu_relax();
> +    }
> +}
> -- 
> 2.43.0

Powered by blists - more mailing lists