* Re: [PATCH v2 2/2] rust: Add read_poll_timeout functions
2025-08-17 4:47 ` [PATCH v2 2/2] rust: Add read_poll_timeout functions FUJITA Tomonori
@ 2025-08-19 0:49 ` Alexandre Courbot
2025-08-20 11:11 ` FUJITA Tomonori
2025-08-19 18:30 ` Daniel Almeida
2025-08-20 10:02 ` Alice Ryhl
2 siblings, 1 reply; 9+ messages in thread
From: Alexandre Courbot @ 2025-08-19 0:49 UTC (permalink / raw)
To: FUJITA Tomonori, a.hindborg, alex.gaynor, ojeda
Cc: aliceryhl, anna-maria, bjorn3_gh, boqun.feng, dakr, frederic,
gary, jstultz, linux-kernel, lossin, lyude, rust-for-linux, sboyd,
tglx, tmgross, daniel.almeida, Fiona Behrens
On Sun Aug 17, 2025 at 1:47 PM JST, FUJITA Tomonori wrote:
> Add read_poll_timeout function 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.
>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Reviewed-by: Fiona Behrens <me@kloenk.dev>
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Tested this with nova-core, and it seems to work fine!
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Tested-by: Alexandre Courbot <acourbot@nvidia.com>
Just one last comment about the documentation below.
<snip>
> +/// 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`,
For precision: "This process continues until either `op` returns and
error, `cond` returns `true`, or the timeout specified by
`timeout_delta` is reached."
> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
> +///
> +/// This function can only be used in a nonatomic context.
Here I'd add an errors section:
# Errors
If `op` returns an error, then that error is returned directly.
If the timeout specified by `timeout_delta` is reached, then
`Err(ETIMEDOUT)` is returned.
> +///
> +/// # Examples
> +///
> +/// ```no_run
> +/// use kernel::io::{Io, poll::read_poll_timeout};
> +/// use kernel::time::Delta;
> +///
> +/// const HW_READY: u16 = 0x01;
> +///
> +/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result<()> {
> +/// 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: &u16| *val == HW_READY,
> +/// Delta::from_millis(50),
> +/// Delta::from_secs(3),
> +/// ) {
> +/// Ok(_) => {
> +/// // The hardware is ready. The returned value of the `op` closure
> +/// // isn't used.
> +/// Ok(())
> +/// }
> +/// Err(e) => Err(e),
> +/// }
> +/// }
> +/// ```
> +#[track_caller]
> +pub fn read_poll_timeout<Op, Cond, T>(
> + mut op: Op,
> + mut cond: Cond,
> + sleep_delta: Delta,
> + timeout_delta: Delta,
> +) -> Result<T>
> +where
> + Op: FnMut() -> Result<T>,
> + Cond: FnMut(&T) -> bool,
> +{
> + let start: Instant<Monotonic> = Instant::now();
> +
> + // Unlike the C version, we always call `might_sleep()` unconditionally,
> + // as conditional calls are error-prone. We clearly separate
> + // `read_poll_timeout()` and `read_poll_timeout_atomic()` to aid
> + // tools like klint.
> + 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.
nit: this comment looks superfluous to me, this is a different
implementation from the C version anyway.
> + return Ok(val);
> + }
> +
> + 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.
Same here.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 2/2] rust: Add read_poll_timeout functions
2025-08-19 0:49 ` Alexandre Courbot
@ 2025-08-20 11:11 ` FUJITA Tomonori
0 siblings, 0 replies; 9+ messages in thread
From: FUJITA Tomonori @ 2025-08-20 11:11 UTC (permalink / raw)
To: acourbot
Cc: fujita.tomonori, a.hindborg, alex.gaynor, ojeda, aliceryhl,
anna-maria, bjorn3_gh, boqun.feng, dakr, frederic, gary, jstultz,
linux-kernel, lossin, lyude, rust-for-linux, sboyd, tglx, tmgross,
daniel.almeida, me
On Tue, 19 Aug 2025 09:49:38 +0900
"Alexandre Courbot" <acourbot@nvidia.com> wrote:
> On Sun Aug 17, 2025 at 1:47 PM JST, FUJITA Tomonori wrote:
>> Add read_poll_timeout function 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.
>>
>> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
>> Reviewed-by: Fiona Behrens <me@kloenk.dev>
>> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>
> Tested this with nova-core, and it seems to work fine!
>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> Tested-by: Alexandre Courbot <acourbot@nvidia.com>
Thanks!
> Just one last comment about the documentation below.
>
> <snip>
>> +/// 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`,
>
> For precision: "This process continues until either `op` returns and
> error, `cond` returns `true`, or the timeout specified by
> `timeout_delta` is reached."
Indeed, `op` should be mentioned here.
I think it should be "op returns an error", not "op returns and error",
right?
There are three alternatives, so I’d remove "either". The sentence
would be:
This process continues until either `op` returns an error, `cond`
returns `true`, or the timeout specified by `timeout_delta` is
reached.
I just realized that "If `timeout_delta` is `None`," comment is outdated. I'll update.
>> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
>> +///
>> +/// This function can only be used in a nonatomic context.
>
> Here I'd add an errors section:
>
> # Errors
>
> If `op` returns an error, then that error is returned directly.
>
> If the timeout specified by `timeout_delta` is reached, then
> `Err(ETIMEDOUT)` is returned.
Thanks, looks nice. I'll add it.
>> + 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.
>
> nit: this comment looks superfluous to me, this is a different
> implementation from the C version anyway.
A previous review mentioned that, since this function shares the same
name as the C version, it's good to make the different behavior
explicit. It may be a bit redundant, but I'd prefer to keep it.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] rust: Add read_poll_timeout functions
2025-08-17 4:47 ` [PATCH v2 2/2] rust: Add read_poll_timeout functions FUJITA Tomonori
2025-08-19 0:49 ` Alexandre Courbot
@ 2025-08-19 18:30 ` Daniel Almeida
2025-08-20 6:45 ` FUJITA Tomonori
2025-08-20 10:02 ` Alice Ryhl
2 siblings, 1 reply; 9+ messages in thread
From: Daniel Almeida @ 2025-08-19 18:30 UTC (permalink / raw)
To: FUJITA Tomonori
Cc: a.hindborg, alex.gaynor, ojeda, aliceryhl, anna-maria, bjorn3_gh,
boqun.feng, dakr, frederic, gary, jstultz, linux-kernel, lossin,
lyude, rust-for-linux, sboyd, tglx, tmgross, acourbot,
Fiona Behrens
Hi Fujita,
> On 17 Aug 2025, at 01:47, FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>
> Add read_poll_timeout function 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.
>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> 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/io.rs | 1 +
> rust/kernel/io/poll.rs | 96 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 97 insertions(+)
> create mode 100644 rust/kernel/io/poll.rs
>
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 03b467722b86..ee182b0b5452 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -8,6 +8,7 @@
> use crate::{bindings, build_assert, ffi::c_void};
>
> pub mod mem;
> +pub mod poll;
> pub mod resource;
>
> pub use resource::Resource;
> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
> new file mode 100644
> index 000000000000..e6325725d5a3
> --- /dev/null
> +++ b/rust/kernel/io/poll.rs
> @@ -0,0 +1,96 @@
> +// 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, poll::read_poll_timeout};
> +/// use kernel::time::Delta;
> +///
> +/// const HW_READY: u16 = 0x01;
> +///
> +/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result<()> {
> +/// 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: &u16| *val == HW_READY,
> +/// Delta::from_millis(50),
> +/// Delta::from_secs(3),
> +/// ) {
> +/// Ok(_) => {
> +/// // The hardware is ready. The returned value of the `op` closure
> +/// // isn't used.
> +/// Ok(())
> +/// }
> +/// Err(e) => Err(e),
> +/// }
> +/// }
> +/// ```
> +#[track_caller]
> +pub fn read_poll_timeout<Op, Cond, T>(
> + mut op: Op,
> + mut cond: Cond,
> + sleep_delta: Delta,
> + timeout_delta: Delta,
> +) -> Result<T>
> +where
> + Op: FnMut() -> Result<T>,
> + Cond: FnMut(&T) -> bool,
> +{
> + let start: Instant<Monotonic> = Instant::now();
> +
> + // Unlike the C version, we always call `might_sleep()` unconditionally,
> + // as conditional calls are error-prone. We clearly separate
> + // `read_poll_timeout()` and `read_poll_timeout_atomic()` to aid
> + // tools like klint.
> + 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 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_delta.is_zero() {
> + fsleep(sleep_delta);
> + }
> +
> + // fsleep() could be busy-wait loop so we always call cpu_relax().
> + cpu_relax();
> + }
> +}
> --
> 2.43.0
>
>
Thanks for working on this. Definitely going to be needed by a lot of drivers.
Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
How is the atomic version going to look like? The same, except for
might_sleep() and without the sleep_delta argument?
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2 2/2] rust: Add read_poll_timeout functions
2025-08-19 18:30 ` Daniel Almeida
@ 2025-08-20 6:45 ` FUJITA Tomonori
0 siblings, 0 replies; 9+ messages in thread
From: FUJITA Tomonori @ 2025-08-20 6:45 UTC (permalink / raw)
To: daniel.almeida
Cc: fujita.tomonori, a.hindborg, alex.gaynor, ojeda, aliceryhl,
anna-maria, bjorn3_gh, boqun.feng, dakr, frederic, gary, jstultz,
linux-kernel, lossin, lyude, rust-for-linux, sboyd, tglx, tmgross,
acourbot, me
On Tue, 19 Aug 2025 15:30:51 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:
> Thanks for working on this. Definitely going to be needed by a lot of drivers.
>
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
Thanks!
> How is the atomic version going to look like? The same, except for
> might_sleep() and without the sleep_delta argument?
If we follow the C implementation, it will be different; C's
read_poll_atomic doesn't use ktime to calculate a timeout.
It would look like the following. I think that the read_poll_timeout
patchset is almost complete so I'll send the
read_poll_timeout_atomic() patchset shortly.
diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
index e6325725d5a3..dc4f1ecdf31f 100644
--- a/rust/kernel/io/poll.rs
+++ b/rust/kernel/io/poll.rs
@@ -8,7 +8,10 @@
error::{code::*, Result},
processor::cpu_relax,
task::might_sleep,
- time::{delay::fsleep, Delta, Instant, Monotonic},
+ time::{
+ delay::{fsleep, udelay},
+ Delta, Instant, Monotonic,
+ },
};
/// Polls periodically until a condition is met or a timeout is reached.
@@ -94,3 +97,86 @@ pub fn read_poll_timeout<Op, Cond, T>(
cpu_relax();
}
}
+
+/// 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 performs a busy wait for a duration specified by `delay_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.
+///
+/// # Examples
+///
+/// ```no_run
+/// use kernel::io::{Io, poll::read_poll_timeout_atomic};
+/// use kernel::time::Delta;
+///
+/// const HW_READY: u16 = 0x01;
+///
+/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result<()> {
+/// match read_poll_timeout_atomic(
+/// // 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: &u16| *val == HW_READY,
+/// Delta::from_micros(50),
+/// Delta::from_micros(300),
+/// ) {
+/// Ok(_) => {
+/// // The hardware is ready. The returned value of the `op` closure
+/// // isn't used.
+/// Ok(())
+/// }
+/// Err(e) => Err(e),
+/// }
+/// }
+/// ```
+pub fn read_poll_timeout_atomic<Op, Cond, T>(
+ mut op: Op,
+ mut cond: Cond,
+ delay_delta: Delta,
+ timeout_delta: Delta,
+) -> Result<T>
+where
+ Op: FnMut() -> Result<T>,
+ Cond: FnMut(&T) -> bool,
+{
+ let mut left_ns = timeout_delta.as_nanos();
+ let delay_ns = delay_delta.as_nanos();
+
+ let timeout_is_zero = timeout_delta.is_zero();
+
+ 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 !timeout_is_zero && left_ns < 0 {
+ // 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 !delay_delta.is_zero() {
+ udelay(delay_delta);
+ if !timeout_is_zero {
+ left_ns -= delay_ns;
+ }
+ }
+
+ cpu_relax();
+ if !timeout_is_zero {
+ left_ns -= 1;
+ }
+ }
+}
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] rust: Add read_poll_timeout functions
2025-08-17 4:47 ` [PATCH v2 2/2] rust: Add read_poll_timeout functions FUJITA Tomonori
2025-08-19 0:49 ` Alexandre Courbot
2025-08-19 18:30 ` Daniel Almeida
@ 2025-08-20 10:02 ` Alice Ryhl
2 siblings, 0 replies; 9+ messages in thread
From: Alice Ryhl @ 2025-08-20 10:02 UTC (permalink / raw)
To: FUJITA Tomonori
Cc: a.hindborg, alex.gaynor, ojeda, anna-maria, bjorn3_gh, boqun.feng,
dakr, frederic, gary, jstultz, linux-kernel, lossin, lyude,
rust-for-linux, sboyd, tglx, tmgross, acourbot, daniel.almeida,
Fiona Behrens
On Sun, Aug 17, 2025 at 01:47:23PM +0900, FUJITA Tomonori wrote:
> Add read_poll_timeout function 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.
>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Reviewed-by: Fiona Behrens <me@kloenk.dev>
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
^ permalink raw reply [flat|nested] 9+ messages in thread