[PATCH v1 0/2] Add read_poll_timeout_atomic support

[PATCH v1 0/2] Add read_poll_timeout_atomic support

[FUJITA Tomonori]

Add a helper function, read_poll_timeout_atomic() to poll periodically
until a condition is met or a timeout is reached.

Unlike read_poll_timeout(), read_poll_timeout_atomic() performs
busy-wait so it can be used in an atomic context.

This patchset can be applied on top of the read_poll_timeout patchset (v3) [1].

[1] https://lore.kernel.org/lkml/20250821002055.3654160-1-fujita.tomonori@gmail.com/

FUJITA Tomonori (2):
  rust: add udelay() function
  rust: Add read_poll_timeout_atomic function

 rust/helpers/time.c       |  5 +++
 rust/kernel/io/poll.rs    | 90 ++++++++++++++++++++++++++++++++++++++-
 rust/kernel/time/delay.rs | 34 +++++++++++++++
 3 files changed, 128 insertions(+), 1 deletion(-)


base-commit: a3b971f57db41ef1c68f842cd3c2c00b3df54ce4
-- 
2.43.0

[PATCH v1 1/2] rust: add udelay() function

[FUJITA Tomonori]

Add udelay() function, inserts a delay based on microseconds with busy
waiting, in preparation for supporting read_poll_timeout_atomic().

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers/time.c       |  5 +++++
 rust/kernel/time/delay.rs | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/rust/helpers/time.c b/rust/helpers/time.c
index a318e9fa4408..67a36ccc3ec4 100644
--- a/rust/helpers/time.c
+++ b/rust/helpers/time.c
@@ -33,3 +33,8 @@ s64 rust_helper_ktime_to_ms(const ktime_t kt)
 {
 	return ktime_to_ms(kt);
 }
+
+void rust_helper_udelay(unsigned long usec)
+{
+	udelay(usec);
+}
diff --git a/rust/kernel/time/delay.rs b/rust/kernel/time/delay.rs
index eb8838da62bc..baae3238d419 100644
--- a/rust/kernel/time/delay.rs
+++ b/rust/kernel/time/delay.rs
@@ -47,3 +47,37 @@ pub fn fsleep(delta: Delta) {
         bindings::fsleep(delta.as_micros_ceil() as c_ulong)
     }
 }
+
+/// Inserts a delay based on microseconds with busy waiting.
+///
+/// Equivalent to the C side [`udelay()`], which delays in microseconds.
+///
+/// `delta` must be within `[0, `MAX_UDELAY_MS`]` in milliseconds;
+/// otherwise, it is erroneous behavior. That is, it is considered a bug to
+/// call this function with an out-of-range value, in which case the function
+/// will insert a delay for at least the maximum value in the range and
+/// may warn in the future.
+///
+/// The behavior above differs from the C side [`udelay()`] for which out-of-range
+/// values could lead to an overflow and unexpected behavior.
+///
+/// [`udelay()`]: https://docs.kernel.org/timers/delay_sleep_functions.html#c.udelay
+pub fn udelay(delta: Delta) {
+    const MAX_UDELAY_DELTA: Delta = Delta::from_millis(bindings::MAX_UDELAY_MS as i64);
+
+    let delta = if (Delta::ZERO..=MAX_UDELAY_DELTA).contains(&delta) {
+        delta
+    } else {
+        // TODO: Add WARN_ONCE() when it's supported.
+        MAX_UDELAY_DELTA
+    };
+
+    // SAFETY: It is always safe to call `udelay()` with any duration.
+    unsafe {
+        // Convert the duration to microseconds and round up to preserve
+        // the guarantee; `udelay()` inserts a delay for at least
+        // the provided duration, but that it may delay for longer
+        // under some circumstances.
+        bindings::udelay(delta.as_micros_ceil() as c_ulong)
+    }
+}
-- 
2.43.0

Re: [PATCH v1 1/2] rust: add udelay() function

[Andreas Hindborg]

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

> Add udelay() function, inserts a delay based on microseconds with busy
> waiting, in preparation for supporting read_poll_timeout_atomic().
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/helpers/time.c       |  5 +++++
>  rust/kernel/time/delay.rs | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
>
> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
> index a318e9fa4408..67a36ccc3ec4 100644
> --- a/rust/helpers/time.c
> +++ b/rust/helpers/time.c
> @@ -33,3 +33,8 @@ s64 rust_helper_ktime_to_ms(const ktime_t kt)
>  {
>  	return ktime_to_ms(kt);
>  }
> +
> +void rust_helper_udelay(unsigned long usec)
> +{
> +	udelay(usec);
> +}
> diff --git a/rust/kernel/time/delay.rs b/rust/kernel/time/delay.rs
> index eb8838da62bc..baae3238d419 100644
> --- a/rust/kernel/time/delay.rs
> +++ b/rust/kernel/time/delay.rs
> @@ -47,3 +47,37 @@ pub fn fsleep(delta: Delta) {
>          bindings::fsleep(delta.as_micros_ceil() as c_ulong)
>      }
>  }
> +
> +/// Inserts a delay based on microseconds with busy waiting.
> +///
> +/// Equivalent to the C side [`udelay()`], which delays in microseconds.
> +///
> +/// `delta` must be within `[0, `MAX_UDELAY_MS`]` in milliseconds;
> +/// otherwise, it is erroneous behavior. That is, it is considered a bug to
> +/// call this function with an out-of-range value, in which case the function
> +/// will insert a delay for at least the maximum value in the range and
> +/// may warn in the future.
> +///
> +/// The behavior above differs from the C side [`udelay()`] for which out-of-range
> +/// values could lead to an overflow and unexpected behavior.
> +///
> +/// [`udelay()`]: https://docs.kernel.org/timers/delay_sleep_functions.html#c.udelay
> +pub fn udelay(delta: Delta) {
> +    const MAX_UDELAY_DELTA: Delta = Delta::from_millis(bindings::MAX_UDELAY_MS as i64);
> +
> +    let delta = if (Delta::ZERO..=MAX_UDELAY_DELTA).contains(&delta) {
> +        delta
> +    } else {
> +        // TODO: Add WARN_ONCE() when it's supported.
> +        MAX_UDELAY_DELTA
> +    };
> +
> +    // SAFETY: It is always safe to call `udelay()` with any duration.

Function documentation says it is overflow and unexpected behavior to
call `udelay` with out of range value, but above comment says any
duration is safe. Which is it?


Best regards,
Andreas Hindborg

Re: [PATCH v1 1/2] rust: add udelay() function

[FUJITA Tomonori]

On Tue, 26 Aug 2025 11:09:12 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

>> +/// Inserts a delay based on microseconds with busy waiting.
>> +///
>> +/// Equivalent to the C side [`udelay()`], which delays in microseconds.
>> +///
>> +/// `delta` must be within `[0, `MAX_UDELAY_MS`]` in milliseconds;
>> +/// otherwise, it is erroneous behavior. That is, it is considered a bug to
>> +/// call this function with an out-of-range value, in which case the function
>> +/// will insert a delay for at least the maximum value in the range and
>> +/// may warn in the future.
>> +///
>> +/// The behavior above differs from the C side [`udelay()`] for which out-of-range
>> +/// values could lead to an overflow and unexpected behavior.
>> +///
>> +/// [`udelay()`]: https://docs.kernel.org/timers/delay_sleep_functions.html#c.udelay
>> +pub fn udelay(delta: Delta) {
>> +    const MAX_UDELAY_DELTA: Delta = Delta::from_millis(bindings::MAX_UDELAY_MS as i64);
>> +
>> +    let delta = if (Delta::ZERO..=MAX_UDELAY_DELTA).contains(&delta) {
>> +        delta
>> +    } else {
>> +        // TODO: Add WARN_ONCE() when it's supported.
>> +        MAX_UDELAY_DELTA
>> +    };
>> +
>> +    // SAFETY: It is always safe to call `udelay()` with any duration.
> 
> Function documentation says it is overflow and unexpected behavior to
> call `udelay` with out of range value, but above comment says any
> duration is safe. Which is it?

This can lead to an unexpected delay duration, but it's safe in Rust’s
sense of safety?

If not, how about the followings?

// SAFETY: `delta` is clamped to the range [0, MAX_UDELAY_DELTA],
// so calling `udelay()` with it is always safe.

Re: [PATCH v1 1/2] rust: add udelay() function

[Miguel Ojeda]

On Tue, Aug 26, 2025 at 1:59 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> This can lead to an unexpected delay duration, but it's safe in Rust’s
> sense of safety?

If it is just unexpected behavior, then yeah.

Perhaps Andreas is referring to C overflow UB? If that is the case,
then in the kernel it is actually defined due to
`-fno-strict-overflow`.

Cheers,
Miguel

Re: [PATCH v1 1/2] rust: add udelay() function

[Andreas Hindborg]

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Tue, Aug 26, 2025 at 1:59 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> This can lead to an unexpected delay duration, but it's safe in Rust’s
>> sense of safety?
>
> If it is just unexpected behavior, then yeah.
>
> Perhaps Andreas is referring to C overflow UB? If that is the case,
> then in the kernel it is actually defined due to
> `-fno-strict-overflow`.

OK, cool Then I would suggest that we just add a small note in the docs
about the C behavior that even though passing an invalid value is
considered a bug, it will not cause UB or memory unsafety.


Best regards,
Andreas Hindborg

Re: [PATCH v1 1/2] rust: add udelay() function

[Daniel Almeida]

Hi Fujita,

> On 21 Aug 2025, at 00:57, FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> 
> Add udelay() function, inserts a delay based on microseconds with busy
> waiting, in preparation for supporting read_poll_timeout_atomic().
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
> rust/helpers/time.c       |  5 +++++
> rust/kernel/time/delay.rs | 34 ++++++++++++++++++++++++++++++++++
> 2 files changed, 39 insertions(+)
> 
> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
> index a318e9fa4408..67a36ccc3ec4 100644
> --- a/rust/helpers/time.c
> +++ b/rust/helpers/time.c
> @@ -33,3 +33,8 @@ s64 rust_helper_ktime_to_ms(const ktime_t kt)
> {
> return ktime_to_ms(kt);
> }
> +
> +void rust_helper_udelay(unsigned long usec)
> +{
> + udelay(usec);
> +}
> diff --git a/rust/kernel/time/delay.rs b/rust/kernel/time/delay.rs
> index eb8838da62bc..baae3238d419 100644
> --- a/rust/kernel/time/delay.rs
> +++ b/rust/kernel/time/delay.rs
> @@ -47,3 +47,37 @@ pub fn fsleep(delta: Delta) {
>         bindings::fsleep(delta.as_micros_ceil() as c_ulong)
>     }
> }
> +
> +/// Inserts a delay based on microseconds with busy waiting.
> +///
> +/// Equivalent to the C side [`udelay()`], which delays in microseconds.
> +///
> +/// `delta` must be within `[0, `MAX_UDELAY_MS`]` in milliseconds;
> +/// otherwise, it is erroneous behavior. That is, it is considered a bug to
> +/// call this function with an out-of-range value, in which case the function
> +/// will insert a delay for at least the maximum value in the range and
> +/// may warn in the future.
> +///
> +/// The behavior above differs from the C side [`udelay()`] for which out-of-range
> +/// values could lead to an overflow and unexpected behavior.
> +///
> +/// [`udelay()`]: https://docs.kernel.org/timers/delay_sleep_functions.html#c.udelay
> +pub fn udelay(delta: Delta) {
> +    const MAX_UDELAY_DELTA: Delta = Delta::from_millis(bindings::MAX_UDELAY_MS as i64);

We should perhaps add a build_assert here to make sure this cast is always valid?

> +
> +    let delta = if (Delta::ZERO..=MAX_UDELAY_DELTA).contains(&delta) {
> +        delta
> +    } else {
> +        // TODO: Add WARN_ONCE() when it's supported.
> +        MAX_UDELAY_DELTA
> +    };
> +
> +    // SAFETY: It is always safe to call `udelay()` with any duration.
> +    unsafe {
> +        // Convert the duration to microseconds and round up to preserve
> +        // the guarantee; `udelay()` inserts a delay for at least
> +        // the provided duration, but that it may delay for longer
> +        // under some circumstances.
> +        bindings::udelay(delta.as_micros_ceil() as c_ulong)
> +    }
> +}
> -- 
> 2.43.0
> 
> 

With the change you suggested for the safety comment in udelay:

Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Re: [PATCH v1 1/2] rust: add udelay() function

[FUJITA Tomonori]

On Tue, 26 Aug 2025 09:44:27 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

>> +/// Inserts a delay based on microseconds with busy waiting.
>> +///
>> +/// Equivalent to the C side [`udelay()`], which delays in microseconds.
>> +///
>> +/// `delta` must be within `[0, `MAX_UDELAY_MS`]` in milliseconds;
>> +/// otherwise, it is erroneous behavior. That is, it is considered a bug to
>> +/// call this function with an out-of-range value, in which case the function
>> +/// will insert a delay for at least the maximum value in the range and
>> +/// may warn in the future.
>> +///
>> +/// The behavior above differs from the C side [`udelay()`] for which out-of-range
>> +/// values could lead to an overflow and unexpected behavior.
>> +///
>> +/// [`udelay()`]: https://docs.kernel.org/timers/delay_sleep_functions.html#c.udelay
>> +pub fn udelay(delta: Delta) {
>> +    const MAX_UDELAY_DELTA: Delta = Delta::from_millis(bindings::MAX_UDELAY_MS as i64);
> 
> We should perhaps add a build_assert here to make sure this cast is always valid?

Are you concerned that bindings::MAX_UDELAY_MS might exceed i64::MAX?

If so, such a value seems unrealistic as a delay. While
bindings::MAX_UDELAY_MS is architecture-dependent, its maximum is
currently 10, so I don’t think this will ever become an issue in the
future.

If really necessary, we could add something like the followings?

build_assert!(i128::from(bindings::MAX_UDELAY_MS) < i64::MAX.into());

>> +
>> +    let delta = if (Delta::ZERO..=MAX_UDELAY_DELTA).contains(&delta) {
>> +        delta
>> +    } else {
>> +        // TODO: Add WARN_ONCE() when it's supported.
>> +        MAX_UDELAY_DELTA
>> +    };
>> +
>> +    // SAFETY: It is always safe to call `udelay()` with any duration.
>> +    unsafe {
>> +        // Convert the duration to microseconds and round up to preserve
>> +        // the guarantee; `udelay()` inserts a delay for at least
>> +        // the provided duration, but that it may delay for longer
>> +        // under some circumstances.
>> +        bindings::udelay(delta.as_micros_ceil() as c_ulong)
>> +    }
>> +}
>> -- 
>> 2.43.0
>> 
>> 
> 
> With the change you suggested for the safety comment in udelay:
> 
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

As Miguel wrote, any duration is safe from Rust’s perspective, and it
won’t invoke UB on the C side either.

So I'm not sure the above safety comment needs to be changed. Let’s
ask for Andreas’s opinion.

[PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

[FUJITA Tomonori]

Add read_poll_timeout_atomic function which polls periodically until a
condition is met, an error occurs, or the timeout is reached.

The C's read_poll_timeout_atomic (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 delay_before_read argument isn't supported since there is no user
for now. It's rarely used in the C version.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/io/poll.rs | 90 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 89 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
index 7af1934e397a..71c2c0e0d8b4 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, an error occurs,
@@ -102,3 +105,88 @@ pub fn read_poll_timeout<Op, Cond, T>(
         cpu_relax();
     }
 }
+
+/// Polls periodically until a condition is met, an error occurs,
+/// or the 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 `op` returns an error, `cond`
+/// returns `true`, or the timeout specified by `timeout_delta` is
+/// reached.
+///
+/// # 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_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();
+
+    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 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);
+            left_ns -= delay_ns;
+        }
+
+        cpu_relax();
+        left_ns -= 1;
+    }
+}
-- 
2.43.0

Re: [PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

[Daniel Almeida]

Hi Fujita,

> On 21 Aug 2025, at 00:57, FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> 
> Add read_poll_timeout_atomic function which polls periodically until a
> condition is met, an error occurs, or the timeout is reached.
> 
> The C's read_poll_timeout_atomic (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 delay_before_read argument isn't supported since there is no user
> for now. It's rarely used in the C version.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
> rust/kernel/io/poll.rs | 90 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
> index 7af1934e397a..71c2c0e0d8b4 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, an error occurs,
> @@ -102,3 +105,88 @@ pub fn read_poll_timeout<Op, Cond, T>(
>         cpu_relax();
>     }
> }
> +
> +/// Polls periodically until a condition is met, an error occurs,
> +/// or the 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 `op` returns an error, `cond`
> +/// returns `true`, or the timeout specified by `timeout_delta` is
> +/// reached.
> +///
> +/// # 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_atomic};
> +/// use kernel::time::Delta;
> +///
> +/// const HW_READY: u16 = 0x01;
> +///
> +/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result<()> {

Just “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();
> +
> +    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 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);
> +            left_ns -= delay_ns;
> +        }
> +
> +        cpu_relax();
> +        left_ns -= 1;

A comment on the line above would be nice.

Also, is timeout_delta == 0 an intended use-case?

> +    }
> +}
> -- 
> 2.43.0
> 
> 

Re: [PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

[FUJITA Tomonori]

On Tue, 26 Aug 2025 11:02:18 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

>> +/// Polls periodically until a condition is met, an error occurs,
>> +/// or the 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 `op` returns an error, `cond`
>> +/// returns `true`, or the timeout specified by `timeout_delta` is
>> +/// reached.
>> +///
>> +/// # 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_atomic};
>> +/// use kernel::time::Delta;
>> +///
>> +/// const HW_READY: u16 = 0x01;
>> +///
>> +/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result<()> {
> 
> Just “Result”.

Oops, thanks.

I'll update the example for read_poll_timeout() too.


>> +///     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();
>> +
>> +    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 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);
>> +            left_ns -= delay_ns;
>> +        }
>> +
>> +        cpu_relax();
>> +        left_ns -= 1;
> 
> A comment on the line above would be nice.

As I wrote in another email, the C version was changed to avoid using
ktime, and that’s when the code above was added. I assume the delay is
considered as 1ns as a compromise because ktime can’t be used.

Maybe this comment should be added to the C version instead?


> Also, is timeout_delta == 0 an intended use-case?

I’m not sure if it’s actually used, but I don’t see any reason to
forbid it.

Re: [PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

[FUJITA Tomonori]

On Wed, 27 Aug 2025 09:35:59 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

>>> +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();
>>> +
>>> +    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 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);
>>> +            left_ns -= delay_ns;
>>> +        }
>>> +
>>> +        cpu_relax();
>>> +        left_ns -= 1;
>> 
>> A comment on the line above would be nice.
> 
> As I wrote in another email, the C version was changed to avoid using
> ktime, and that’s when the code above was added. I assume the delay is
> considered as 1ns as a compromise because ktime can’t be used.
> 
> Maybe this comment should be added to the C version instead?

I meant that if we add a comment here, maybe it should be added to the
C version.

Re: [PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

[Danilo Krummrich]

On Thu Aug 21, 2025 at 5:57 AM CEST, FUJITA Tomonori wrote:
> +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();
> +
> +    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 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);
> +            left_ns -= delay_ns;
> +        }
> +
> +        cpu_relax();
> +        left_ns -= 1;

How do we know that each iteration costs 1ns? To make it even more obvious, we
don't control the implementation of cond(). Shouldn't we use ktime for this?

Re: [PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

[Daniel Almeida]

> On 26 Aug 2025, at 11:12, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Thu Aug 21, 2025 at 5:57 AM CEST, FUJITA Tomonori wrote:
>> +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();
>> +
>> +    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 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);
>> +            left_ns -= delay_ns;
>> +        }
>> +
>> +        cpu_relax();
>> +        left_ns -= 1;
> 
> How do we know that each iteration costs 1ns? To make it even more obvious, we
> don't control the implementation of cond(). Shouldn't we use ktime for this?

I don’t think ktime can be used from an atomic context?

— Daniel

Re: [PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

[Danilo Krummrich]

On Tue Aug 26, 2025 at 6:59 PM CEST, Daniel Almeida wrote:
>
>
>> On 26 Aug 2025, at 11:12, Danilo Krummrich <dakr@kernel.org> wrote:
>> 
>> On Thu Aug 21, 2025 at 5:57 AM CEST, FUJITA Tomonori wrote:
>>> +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();
>>> +
>>> +    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 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);
>>> +            left_ns -= delay_ns;
>>> +        }
>>> +
>>> +        cpu_relax();
>>> +        left_ns -= 1;
>> 
>> How do we know that each iteration costs 1ns? To make it even more obvious, we
>> don't control the implementation of cond(). Shouldn't we use ktime for this?
>
> I don’t think ktime can be used from an atomic context?

There's no problem calling things like ktime_get() from atomic context.

Re: [PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

[FUJITA Tomonori]

On Tue, 26 Aug 2025 16:12:44 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:

> On Thu Aug 21, 2025 at 5:57 AM CEST, FUJITA Tomonori wrote:
>> +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();
>> +
>> +    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 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);
>> +            left_ns -= delay_ns;
>> +        }
>> +
>> +        cpu_relax();
>> +        left_ns -= 1;
> 
> How do we know that each iteration costs 1ns? To make it even more obvious, we
> don't control the implementation of cond(). Shouldn't we use ktime for this?

The C version used to use ktime but it has been changed not to:

7349a69cf312 ("iopoll: Do not use timekeeping in read_poll_timeout_atomic()")

https://lore.kernel.org/all/3d2a2f4e553489392d871108797c3be08f88300b.1685692810.git.geert+renesas@glider.be/

I don’t know if the same problem still exists, but I think we should
follow the C implementation. Usually there’s a good reason behind it,
and it has been working so far.

If we want to do it differently in Rust, maybe we should first discuss
the C implementation.

Re: [PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

[Danilo Krummrich]

On Wed Aug 27, 2025 at 2:14 AM CEST, FUJITA Tomonori wrote:
> On Tue, 26 Aug 2025 16:12:44 +0200
> "Danilo Krummrich" <dakr@kernel.org> wrote:
>
>> On Thu Aug 21, 2025 at 5:57 AM CEST, FUJITA Tomonori wrote:
>>> +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();
>>> +
>>> +    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 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);
>>> +            left_ns -= delay_ns;
>>> +        }
>>> +
>>> +        cpu_relax();
>>> +        left_ns -= 1;
>> 
>> How do we know that each iteration costs 1ns? To make it even more obvious, we
>> don't control the implementation of cond(). Shouldn't we use ktime for this?
>
> The C version used to use ktime but it has been changed not to:
>
> 7349a69cf312 ("iopoll: Do not use timekeeping in read_poll_timeout_atomic()")

Ick! That's pretty unfortunate -- no ktime then.

But regardless of that, the current implementation (this and the C one) lack
clarity.

The nanosecond decrement is rather negligible, the real timeout reduction comes
from the delay_delta. Given that, and the fact that we can't use ktime, this
function shouldn't take a raw timeout value, since we can't guarantee the
timeout anyways.

Instead, I think it makes much more sense to provide a retry count as function
argument, such that the user can specify "I want a dealy of 100us, try it 100
times".

This way it is transparent to the caller that the timeout may be significantly
more than 10ms depending on the user's implementation.

As for doing this in C vs Rust: I don't think things have to align in every
implementation detail. If we can improve things on the Rust side from the
get-go, we should not stop ourselves from doing so, just because a similar C
implementation is hard to refactor, due to having a lot of users already.

Re: [PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

[Danilo Krummrich]

On Wed Aug 27, 2025 at 11:00 AM CEST, Danilo Krummrich wrote:
> On Wed Aug 27, 2025 at 2:14 AM CEST, FUJITA Tomonori wrote:
>> On Tue, 26 Aug 2025 16:12:44 +0200
>> "Danilo Krummrich" <dakr@kernel.org> wrote:
>>
>>> On Thu Aug 21, 2025 at 5:57 AM CEST, FUJITA Tomonori wrote:
>>>> +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();
>>>> +
>>>> +    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 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);
>>>> +            left_ns -= delay_ns;
>>>> +        }
>>>> +
>>>> +        cpu_relax();
>>>> +        left_ns -= 1;
>>> 
>>> How do we know that each iteration costs 1ns? To make it even more obvious, we
>>> don't control the implementation of cond(). Shouldn't we use ktime for this?
>>
>> The C version used to use ktime but it has been changed not to:
>>
>> 7349a69cf312 ("iopoll: Do not use timekeeping in read_poll_timeout_atomic()")
>
> Ick! That's pretty unfortunate -- no ktime then.
>
> But regardless of that, the current implementation (this and the C one) lack
> clarity.
>
> The nanosecond decrement is rather negligible, the real timeout reduction comes
> from the delay_delta. Given that, and the fact that we can't use ktime, this
> function shouldn't take a raw timeout value, since we can't guarantee the
> timeout anyways.

Actually, let me put it in other words:

	let val = read_poll_timeout_atomic(
	    || {
	        // Fetch the offset to read from from the HW.
	        let offset = io.read32(0x1000);
	
	        // HW needs a break for some odd reason.
	        udelay(100);
	
	        // Read the actual value.
	        io.try_read32(offset)
	    },
	    |val: &u32| *val == HW_READY,
	    Delta::from_micros(0),      // No delay, keep spinning.
	    Delta::from_millis(10),     // Timeout after 10ms.
	)?;

Seems like a fairly reasonable usage without knowing the implementation details
of read_poll_timeout_atomic(), right?

Except that if the hardware does not become ready, this will spin for 16.67
*minutes* -- in atomic context. Instead of the 10ms the user would expect.

This would be way less error prone if we do not provide a timeout value, but a
retry count.

> Instead, I think it makes much more sense to provide a retry count as function
> argument, such that the user can specify "I want a dealy of 100us, try it 100
> times".
>
> This way it is transparent to the caller that the timeout may be significantly
> more than 10ms depending on the user's implementation.
>
> As for doing this in C vs Rust: I don't think things have to align in every
> implementation detail. If we can improve things on the Rust side from the
> get-go, we should not stop ourselves from doing so, just because a similar C
> implementation is hard to refactor, due to having a lot of users already.

Re: [PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

[Daniel Almeida]

Hi Danilo,

[…}

> 
> Actually, let me put it in other words:
> 
> let val = read_poll_timeout_atomic(
>     || {
>         // Fetch the offset to read from from the HW.
>         let offset = io.read32(0x1000);
> 
>         // HW needs a break for some odd reason.
>         udelay(100);
> 
>         // Read the actual value.
>         io.try_read32(offset)
>     },
>     |val: &u32| *val == HW_READY,
>     Delta::from_micros(0),      // No delay, keep spinning.
>     Delta::from_millis(10),     // Timeout after 10ms.
> )?;
> 
> Seems like a fairly reasonable usage without knowing the implementation details
> of read_poll_timeout_atomic(), right?
> 
> Except that if the hardware does not become ready, this will spin for 16.67
> *minutes* -- in atomic context. Instead of the 10ms the user would expect.
> 
> This would be way less error prone if we do not provide a timeout value, but a
> retry count.
> 
>> Instead, I think it makes much more sense to provide a retry count as function
>> argument, such that the user can specify "I want a dealy of 100us, try it 100
>> times".
>> 
>> This way it is transparent to the caller that the timeout may be significantly
>> more than 10ms depending on the user's implementation.
>> 
>> As for doing this in C vs Rust: I don't think things have to align in every
>> implementation detail. If we can improve things on the Rust side from the
>> get-go, we should not stop ourselves from doing so, just because a similar C
>> implementation is hard to refactor, due to having a lot of users already.

I must say I do not follow. Can you expand yet some more on this?

— Daniel

Re: [PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

[Danilo Krummrich]

On Wed Aug 27, 2025 at 2:14 PM CEST, Daniel Almeida wrote:
> Hi Danilo,
>
> […}
>
>> 
>> Actually, let me put it in other words:
>> 
>> let val = read_poll_timeout_atomic(
>>     || {
>>         // Fetch the offset to read from from the HW.
>>         let offset = io.read32(0x1000);
>> 
>>         // HW needs a break for some odd reason.
>>         udelay(100);
>> 
>>         // Read the actual value.
>>         io.try_read32(offset)
>>     },
>>     |val: &u32| *val == HW_READY,
>>     Delta::from_micros(0),      // No delay, keep spinning.
>>     Delta::from_millis(10),     // Timeout after 10ms.
>> )?;
>> 
>> Seems like a fairly reasonable usage without knowing the implementation details
>> of read_poll_timeout_atomic(), right?
>> 
>> Except that if the hardware does not become ready, this will spin for 16.67
>> *minutes* -- in atomic context. Instead of the 10ms the user would expect.
>> 
>> This would be way less error prone if we do not provide a timeout value, but a
>> retry count.
>> 
>>> Instead, I think it makes much more sense to provide a retry count as function
>>> argument, such that the user can specify "I want a dealy of 100us, try it 100
>>> times".
>>> 
>>> This way it is transparent to the caller that the timeout may be significantly
>>> more than 10ms depending on the user's implementation.
>>> 
>>> As for doing this in C vs Rust: I don't think things have to align in every
>>> implementation detail. If we can improve things on the Rust side from the
>>> get-go, we should not stop ourselves from doing so, just because a similar C
>>> implementation is hard to refactor, due to having a lot of users already.
>
> I must say I do not follow. Can you expand yet some more on this?

Sure, but it would help if you could clarify which aspect you want me to expand
on. :)

Re: [PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

[Daniel Almeida]

> On 27 Aug 2025, at 09:19, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Wed Aug 27, 2025 at 2:14 PM CEST, Daniel Almeida wrote:
>> Hi Danilo,
>> 
>> […}
>> 
>>> 
>>> Actually, let me put it in other words:
>>> 
>>> let val = read_poll_timeout_atomic(
>>>    || {
>>>        // Fetch the offset to read from from the HW.
>>>        let offset = io.read32(0x1000);
>>> 
>>>        // HW needs a break for some odd reason.
>>>        udelay(100);

Why would we have a delay here? Can’t this be broken into two calls to
read_poll_timeout_atomic()? That would be equivalent to what you wrote
IIUC.

>>> 
>>>        // Read the actual value.
>>>        io.try_read32(offset)
>>>    },
>>>    |val: &u32| *val == HW_READY,
>>>    Delta::from_micros(0),      // No delay, keep spinning.
>>>    Delta::from_millis(10),     // Timeout after 10ms.
>>> )?;
>>> 
>>> Seems like a fairly reasonable usage without knowing the implementation details
>>> of read_poll_timeout_atomic(), right?
>>> 
>>> Except that if the hardware does not become ready, this will spin for 16.67
>>> *minutes* -- in atomic context. Instead of the 10ms the user would expect.

This is where you lost me. Where does the 16.67 come from?

>>> 
>>> This would be way less error prone if we do not provide a timeout value, but a
>>> retry count.
>>> 
>>>> Instead, I think it makes much more sense to provide a retry count as function
>>>> argument, such that the user can specify "I want a dealy of 100us, try it 100
>>>> times".
>>>> 
>>>> This way it is transparent to the caller that the timeout may be significantly
>>>> more than 10ms depending on the user's implementation.
>>>> 
>>>> As for doing this in C vs Rust: I don't think things have to align in every
>>>> implementation detail. If we can improve things on the Rust side from the
>>>> get-go, we should not stop ourselves from doing so, just because a similar C
>>>> implementation is hard to refactor, due to having a lot of users already.
>> 
>> I must say I do not follow. Can you expand yet some more on this?
> 
> Sure, but it would help if you could clarify which aspect you want me to expand
> on. :)

Re: [PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

[Danilo Krummrich]

On Wed Aug 27, 2025 at 2:22 PM CEST, Daniel Almeida wrote:
>
>
>> On 27 Aug 2025, at 09:19, Danilo Krummrich <dakr@kernel.org> wrote:
>> 
>> On Wed Aug 27, 2025 at 2:14 PM CEST, Daniel Almeida wrote:
>>> Hi Danilo,
>>> 
>>> […}
>>> 
>>>> 
>>>> Actually, let me put it in other words:
>>>> 
>>>> let val = read_poll_timeout_atomic(
>>>>    || {
>>>>        // Fetch the offset to read from from the HW.
>>>>        let offset = io.read32(0x1000);
>>>> 
>>>>        // HW needs a break for some odd reason.
>>>>        udelay(100);
>
> Why would we have a delay here? Can’t this be broken into two calls to
> read_poll_timeout_atomic()? That would be equivalent to what you wrote
> IIUC.

I'm sure this can somehow be written otherwise as well. But that's not the
point, the point is that this looks like perfectly valid code from a users
perspective.

>>>> 
>>>>        // Read the actual value.
>>>>        io.try_read32(offset)
>>>>    },
>>>>    |val: &u32| *val == HW_READY,
>>>>    Delta::from_micros(0),      // No delay, keep spinning.
>>>>    Delta::from_millis(10),     // Timeout after 10ms.
>>>> )?;
>>>> 
>>>> Seems like a fairly reasonable usage without knowing the implementation details
>>>> of read_poll_timeout_atomic(), right?
>>>> 
>>>> Except that if the hardware does not become ready, this will spin for 16.67
>>>> *minutes* -- in atomic context. Instead of the 10ms the user would expect.
>
> This is where you lost me. Where does the 16.67 come from?

Ah, I see -- let me explain:

Internally read_poll_timeout_atomic() would convert the timeout (10ms) into ns
(let's call it nanos). Then, it would decrement nanos in every iteration of the
internal loop, based on the (wrong) assumption that every loop takes exactly
1ns.

However, since the user executes udelay(100), which is perfectly valid from the
users perspective, in the Op closure, every loop iteration takes at least 100us
instead.

So, the actual timeout calculates as follows.

	Timeout: 10ms = 10.000us = 10.000.000ns

In every iteration this number is decremented by one, hence 10.000.000
iterations.

	100us * 10.000.000 iterations = 16.67 minutes

So, the issue really is that we're not measuring time, but the number of
iterations if delay_delta == 0.

As delay_delta grows the relative eror becomes smaller, yet this is far from
sane behavior.

>>>> 
>>>> This would be way less error prone if we do not provide a timeout value, but a
>>>> retry count.
>>>> 
>>>>> Instead, I think it makes much more sense to provide a retry count as function
>>>>> argument, such that the user can specify "I want a dealy of 100us, try it 100
>>>>> times".
>>>>> 
>>>>> This way it is transparent to the caller that the timeout may be significantly
>>>>> more than 10ms depending on the user's implementation.
>>>>> 
>>>>> As for doing this in C vs Rust: I don't think things have to align in every
>>>>> implementation detail. If we can improve things on the Rust side from the
>>>>> get-go, we should not stop ourselves from doing so, just because a similar C
>>>>> implementation is hard to refactor, due to having a lot of users already.
>>> 
>>> I must say I do not follow. Can you expand yet some more on this?
>> 
>> Sure, but it would help if you could clarify which aspect you want me to expand
>> on. :)