[PATCH v1 0/2] rust: Add read_poll_timeout

[PATCH v1 0/2] rust: Add read_poll_timeout

[FUJITA Tomonori]

Add a helper function to poll periodically until a condition is met or
a timeout is reached.

This patch was previously reviewed as part of another patchset [1] but
was removed to expedite merging into the mainline. Now that all the
features it depends on have been merged into the mainline, it is being
reposted as a new independent patchset.

I put this function kernel/time/poll.rs. If we follow the C
implementation, it might go under kernel/iopoll, but since this
function is not necessarily related to I/O, I think it is more
appropriate to maintain it as part of the timekeeping abstraction.

As Andreas pointed out [3], the test to call this function in an
atomic context doesn't work nicely; we can see might_sleep() throws a
warning in an atomic context. If such cases cannot be caught by the
test infra, it would be better to either remove the test or change it
to norun with a TODO, I think. Also, it might be better to move it
next to might_sleep() if we keep it.

After we agree on this patchset, I'll work on
read_poll_timeout_atomic().

v1 (the changes since the last posting [2])
- removed might_sleep() change since it was already merged separately.
- split out cpu_relax() in a separate patch
- make the example code compilable
- update the code to use Clocksource (MONOTONIC)
- call might_sleep() always (even when the function doesn't sleep)
- cosmetic changes to the doc

[1] https://lore.kernel.org/lkml/20250207132623.168854-1-fujita.tomonori@gmail.com/
[2] https://lore.kernel.org/lkml/20250220070611.214262-8-fujita.tomonori@gmail.com/
[3] https://lore.kernel.org/lkml/87y0wx9hpk.fsf@kernel.org/

FUJITA Tomonori (2):
  rust: Add cpu_relax() helper
  rust: Add read_poll_timeout functions

 rust/helpers/helpers.c   |   1 +
 rust/helpers/processor.c |   8 +++
 rust/kernel/lib.rs       |   1 +
 rust/kernel/processor.rs |  13 +++++
 rust/kernel/time.rs      |   1 +
 rust/kernel/time/poll.rs | 104 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 128 insertions(+)
 create mode 100644 rust/helpers/processor.c
 create mode 100644 rust/kernel/processor.rs
 create mode 100644 rust/kernel/time/poll.rs


base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
-- 
2.43.0

[PATCH v1 1/2] rust: Add cpu_relax() helper

[FUJITA Tomonori]

Add cpu_relax() helper in preparation for supporting
read_poll_timeout().

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers/helpers.c   |  1 +
 rust/helpers/processor.c |  8 ++++++++
 rust/kernel/lib.rs       |  1 +
 rust/kernel/processor.rs | 13 +++++++++++++
 4 files changed, 23 insertions(+)
 create mode 100644 rust/helpers/processor.c
 create mode 100644 rust/kernel/processor.rs

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 7cf7fe95e41d..04598665e7c8 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -34,6 +34,7 @@
 #include "pid_namespace.c"
 #include "platform.c"
 #include "poll.c"
+#include "processor.c"
 #include "property.c"
 #include "rbtree.c"
 #include "rcu.c"
diff --git a/rust/helpers/processor.c b/rust/helpers/processor.c
new file mode 100644
index 000000000000..d41355e14d6e
--- /dev/null
+++ b/rust/helpers/processor.c
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/processor.h>
+
+void rust_helper_cpu_relax(void)
+{
+	cpu_relax();
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ed53169e795c..c098c47c1817 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -110,6 +110,7 @@
 pub mod platform;
 pub mod prelude;
 pub mod print;
+pub mod processor;
 pub mod rbtree;
 pub mod regulator;
 pub mod revocable;
diff --git a/rust/kernel/processor.rs b/rust/kernel/processor.rs
new file mode 100644
index 000000000000..d1db85b76243
--- /dev/null
+++ b/rust/kernel/processor.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: Always safe to call.
+    unsafe { bindings::cpu_relax() }
+}
-- 
2.43.0

[PATCH v1 2/2] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

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@kloenk.dev>
Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time.rs      |   1 +
 rust/kernel/time/poll.rs | 104 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+)
 create mode 100644 rust/kernel/time/poll.rs

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 64c8dcf548d6..ec0ec33c838c 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -28,6 +28,7 @@
 
 pub mod delay;
 pub mod hrtimer;
+pub mod poll;
 
 /// The number of nanoseconds per microsecond.
 pub const NSEC_PER_USEC: i64 = bindings::NSEC_PER_USEC as i64;
diff --git a/rust/kernel/time/poll.rs b/rust/kernel/time/poll.rs
new file mode 100644
index 000000000000..9cf0acb1e165
--- /dev/null
+++ b/rust/kernel/time/poll.rs
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! IO polling.
+//!
+//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
+
+use crate::{
+    error::{code::*, Result},
+    processor::cpu_relax,
+    task::might_sleep,
+    time::{delay::fsleep, Delta, Instant, Monotonic},
+};
+
+/// 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),
+///     }
+/// }
+/// ```
+///
+/// ```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)));
+/// 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()`.
+    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

Re: [PATCH v1 1/2] rust: Add cpu_relax() helper

[Alice Ryhl]

On Mon, Aug 11, 2025 at 01:10:37PM +0900, FUJITA Tomonori wrote:
> Add cpu_relax() helper in preparation for supporting
> read_poll_timeout().
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

> +pub fn cpu_relax() {
> +    // SAFETY: Always safe to call.
> +    unsafe { bindings::cpu_relax() }
> +}

Let's mark this #[inline].

Alice

Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

[Alice Ryhl]

On Mon, Aug 11, 2025 at 01:10:38PM +0900, 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.

I would drop this paragraph. You have a call to might_sleep() now.

> +#[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,

I would consider just writing this as:

pub fn read_poll_timeout<T>(
    mut op: impl FnMut() -> Result<T>,
    mut cond: impl FnMut(&T) -> bool,
    sleep_delta: Delta,
    timeout_delta: Option<Delta>,
) -> Result<T>

And I would also consider adding a new error type called TimeoutError
similar to BadFdError in `rust/kernel/fs/file.rs`. That way, we promise
to the caller that we never return error codes other than a timeout.

Another thing is the `timeout_delta` option. I would just have written
it as two methods, one that takes a timeout and one that doesn't. That
way, callers that don't need a timeout do not need to handle timeout
errors. (Do we have any users without a timeout? If not, maybe just
remove the Option.)

> +{
> +    let start: Instant<Monotonic> = Instant::now();
> +    let sleep = !sleep_delta.is_zero();
> +
> +    // Unlike the C version, we always call `might_sleep()`.
> +    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);
> +        }

I would just do:

if !sleep_delta.is_zero() {
    fsleep(sleep_delta);
}

instead of the extra variable.

> +        // fsleep() could be busy-wait loop so we always call cpu_relax().
> +        cpu_relax();
> +    }
> +}
> -- 
> 2.43.0
> 

Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

[Danilo Krummrich]

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@kloenk.dev>
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.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

Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

[Andreas Hindborg]

"Danilo Krummrich" <dakr@kernel.org> writes:

> 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@kloenk.dev>
>> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.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.

I think we should just drop this example.


Best regards,
Andreas Hindborg

Re: [PATCH v1 1/2] rust: Add cpu_relax() helper

[Andreas Hindborg]

"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:

> Add cpu_relax() helper in preparation for supporting
> read_poll_timeout().
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

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


Best regards,
Andreas Hindborg

Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

[Andreas Hindborg]

"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:

> 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@kloenk.dev>
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

With the example removed:

-///
-/// ```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)));
-/// drop(g);
-///
-/// # Ok::<(), Error>(())
-/// ```

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


Best regards,
Andreas Hindborg

Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

[Alexandre Courbot]

On Mon Aug 11, 2025 at 1:10 PM JST, FUJITA Tomonori wrote:
> Add read_poll_timeout functions which poll periodically until a

"functions" should be the singular "function" as this patch only adds
one function.

<snip>
> +/// # 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))) {

Is there a reason for not writing the closures directly inline? I.e.

  match read_poll_timeout(
      // The `op` closure reads the value of a specific status register.
      || io.try_read16(0x1000),
      // The `cond` closure takes a reference to the value returned by `op`
      // and checks whether the hardware is ready.
      |val| *val == HW_READY,
      Delta::from_millis(50),
      Some(Delta::from_secs(3))
  )

I think it is closer to how people will actually use this function, and
the expected types for the closures are available right in the function
definition if they need more details.

Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

On Mon, 11 Aug 2025 09:50:59 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> On Mon, Aug 11, 2025 at 01:10:38PM +0900, 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.
> 
> I would drop this paragraph. You have a call to might_sleep() now.

Do you mean that, since it’s obvious might_sleep() can only be used in
a non-atomic context, the above statement is redundant and can be
dropped?

>> +#[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,
> 
> I would consider just writing this as:
>
> pub fn read_poll_timeout<T>(
>     mut op: impl FnMut() -> Result<T>,
>     mut cond: impl FnMut(&T) -> bool,
>     sleep_delta: Delta,
>     timeout_delta: Option<Delta>,
> ) -> Result<T>

Surely, I'll do.

> And I would also consider adding a new error type called TimeoutError
> similar to BadFdError in `rust/kernel/fs/file.rs`. That way, we promise
> to the caller that we never return error codes other than a timeout.

Understood, I'll.

> Another thing is the `timeout_delta` option. I would just have written
> it as two methods, one that takes a timeout and one that doesn't. That
> way, callers that don't need a timeout do not need to handle timeout
> errors. (Do we have any users without a timeout? If not, maybe just
> remove the Option.)

I'll remove the Option and let's see if we’ll need a version without a
timeout.


>> +{
>> +    let start: Instant<Monotonic> = Instant::now();
>> +    let sleep = !sleep_delta.is_zero();
>> +
>> +    // Unlike the C version, we always call `might_sleep()`.
>> +    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);
>> +        }
> 
> I would just do:
> 
> if !sleep_delta.is_zero() {
>     fsleep(sleep_delta);
> }
> 
> instead of the extra variable.

I'll in the next version.

Re: [PATCH v1 1/2] rust: Add cpu_relax() helper

[FUJITA Tomonori]

On Mon, 11 Aug 2025 09:39:36 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> On Mon, Aug 11, 2025 at 01:10:37PM +0900, FUJITA Tomonori wrote:
>> Add cpu_relax() helper in preparation for supporting
>> read_poll_timeout().
>> 
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> 
>> +pub fn cpu_relax() {
>> +    // SAFETY: Always safe to call.
>> +    unsafe { bindings::cpu_relax() }
>> +}
> 
> Let's mark this #[inline].

Thanks, I'll do in the next version.

Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

On Mon, 11 Aug 2025 11:55:56 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:

> 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@kloenk.dev>
>> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.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.

Either is fine by me.

If there are no other opinions, I’ll go with io/poll.rs in the next
version.


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

I was discussing with Andreas, and I’ll remove this example.

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

// Unlike the C version, we always call `might_sleep()`, because
// conditional calls are error-prone. We clearly separate the
// `read_poll_timeout()` and `read_poll_timeout_atomic()` functions for
// tools like klint.

Looks reasonable?

Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

On Wed, 13 Aug 2025 11:56:26 +0900
"Alexandre Courbot" <acourbot@nvidia.com> wrote:

> On Mon Aug 11, 2025 at 1:10 PM JST, FUJITA Tomonori wrote:
>> Add read_poll_timeout functions which poll periodically until a
> 
> "functions" should be the singular "function" as this patch only adds
> one function.

Oops, thanks. I'll fix.

> <snip>
>> +/// # 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))) {
> 
> Is there a reason for not writing the closures directly inline? I.e.
> 
>   match read_poll_timeout(
>       // The `op` closure reads the value of a specific status register.
>       || io.try_read16(0x1000),
>       // The `cond` closure takes a reference to the value returned by `op`
>       // and checks whether the hardware is ready.
>       |val| *val == HW_READY,
>       Delta::from_millis(50),
>       Some(Delta::from_secs(3))
>   )
> 
> I think it is closer to how people will actually use this function, and
> the expected types for the closures are available right in the function
> definition if they need more details.

Either is fine by me. I thought that not writing directly is more
readable.

Anyone else have an opinion?

Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

[Alice Ryhl]

On Thu, Aug 14, 2025 at 03:11:47PM +0900, FUJITA Tomonori wrote:
> On Mon, 11 Aug 2025 09:50:59 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
> > On Mon, Aug 11, 2025 at 01:10:38PM +0900, 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.
> > 
> > I would drop this paragraph. You have a call to might_sleep() now.
> 
> Do you mean that, since it’s obvious might_sleep() can only be used in
> a non-atomic context, the above statement is redundant and can be
> dropped?

I mean, klint is nice as it's a compile-time check rather than a
runtime check. But might_sleep() still counts as having the
abstractions check it in my book. So you shouldn't say that you are not
checking it, when you are checking it.

Alice

Re: [PATCH v1 2/2] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

On Thu, 14 Aug 2025 15:11:47 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

>>> +#[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,
>> 
>> I would consider just writing this as:
>>
>> pub fn read_poll_timeout<T>(
>>     mut op: impl FnMut() -> Result<T>,
>>     mut cond: impl FnMut(&T) -> bool,
>>     sleep_delta: Delta,
>>     timeout_delta: Option<Delta>,
>> ) -> Result<T>
> 
> Surely, I'll do.

The above change caused the example code to fail to compile with a
"type annotations needed" error, so I kept the original code.


>> And I would also consider adding a new error type called TimeoutError
>> similar to BadFdError in `rust/kernel/fs/file.rs`. That way, we promise
>> to the caller that we never return error codes other than a timeout.
> 
> Understood, I'll.

I was reminded that this function can return errors other than
timeout; Op closure returns an any error like register read failure.