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

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.

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?

"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

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

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?

"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

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
>

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.

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.

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