Re: [PATCH v11 7/8] rust: Add read_poll_timeout functions

Danilo Krummrich
2025-07-28T13:13:53Z

On Thu Feb 20, 2025 at 8:06 AM CET, FUJITA Tomonori wrote:
> diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs
> new file mode 100644
> index 000000000000..eeeff4be84fa
> --- /dev/null
> +++ b/rust/kernel/cpu.rs
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Processor related primitives.
> +//!
> +//! C header: [`include/linux/processor.h`](srctree/include/linux/processor.h).
> +
> +/// Lower CPU power consumption or yield to a hyperthreaded twin processor.
> +///
> +/// It also happens to serve as a compiler barrier.
> +pub fn cpu_relax() {
> + // SAFETY: FFI call.
> + unsafe { bindings::cpu_relax() }
> +}

Please split this out in a separate patch.

> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index f6ecf09cb65f..8858eb13b3df 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -64,6 +64,7 @@ macro_rules! declare_err {
> declare_err!(EPIPE, "Broken pipe.");
> declare_err!(EDOM, "Math argument out of domain of func.");
> declare_err!(ERANGE, "Math result not representable.");
> + declare_err!(ETIMEDOUT, "Connection timed out.");
> declare_err!(ERESTARTSYS, "Restart the system call.");
> declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
> declare_err!(ERESTARTNOHAND, "Restart if no handler.");
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index d4a73e52e3ee..be63742f517b 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -7,6 +7,8 @@
> use crate::error::{code::EINVAL, Result};
> use crate::{bindings, build_assert};
>
> +pub mod poll;
> +
> /// Raw representation of an MMIO region.
> ///
> /// By itself, the existence of an instance of this structure does not provide any guarantees that
> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
> new file mode 100644
> index 000000000000..5977b2082cc6
> --- /dev/null
> +++ b/rust/kernel/io/poll.rs
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! IO polling.
> +//!
> +//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
> +
> +use crate::{
> + cpu::cpu_relax,
> + error::{code::*, Result},
> + time::{delay::fsleep, Delta, Instant},
> +};
> +
> +/// 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`.

I'd add an empty line here,

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

and here.

> +/// 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.
> +///
> +/// # Examples
> +///
> +/// ```rust,ignore

Why ignore? This should be possible to compile test.

> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {

I think the parameter here can just be `&Io<SIZE>`.

> +/// // The `op` closure reads the value of a specific status register.
> +/// let op = || -> Result<u16> { dev.read_ready_register() };
> +///
> +/// // 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),
> +/// }
> +/// }
> +/// ```
> +///
> +/// ```rust
> +/// use kernel::io::poll::read_poll_timeout;
> +/// use kernel::time::Delta;
> +/// use kernel::sync::{SpinLock, new_spinlock};
> +///
> +/// 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)));
> +/// 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::now();
> + let sleep = !sleep_delta.is_zero();
> +
> + if sleep {
> + might_sleep();
> + }

I think a conditional might_sleep() is not great.

I also think we can catch this at compile time, if we add two different variants
of read_poll_timeout() instead and be explicit about it. We could get Klint to
catch such issues for us at compile time.

> +
> + 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();
> + }
> +}