[PATCH v2 0/2] Add read_poll_count_atomic support

[PATCH v2 0/2] Add read_poll_count_atomic support

[FUJITA Tomonori]

Add read_poll_count_atomic function which polls periodically until a
condition is met, an error occurs, or the attempt limit is reached.

This helper is used to wait for a condition in atomic context,
mirroring the C's read_poll_timeout_atomic().

In atomic context, the timekeeping infrastructure is unavailable, so
reliable time-based timeouts cannot be implemented. So instead, the
helper accepts a maximum number of attempts and busy-waits (udelay +
cpu_relax) between tries.

v1: https://lore.kernel.org/lkml/20250821035710.3692455-1-fujita.tomonori@gmail.com/
- use the attempt limit instead of timeout
- rename the function to read_poll_count_atomic
- add the comment about C's udelay behavior.


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

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


base-commit: e6901808a3b28d8bdabfa98a618b2eab6f8798e8
-- 
2.43.0

[PATCH v2 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_count_atomic().

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers/time.c       |  5 +++++
 rust/kernel/time/delay.rs | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 42 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..fb7c15dfe186 100644
--- a/rust/kernel/time/delay.rs
+++ b/rust/kernel/time/delay.rs
@@ -47,3 +47,40 @@ 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.
+    // Note that the kernel is compiled with `-fno-strict-overflow`
+    // so any out-of-range value could lead to unexpected behavior
+    // but won't lead to undefined behavior.
+    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 v2 1/2] rust: add udelay() function

[Danilo Krummrich]

On Tue Oct 21, 2025 at 9:11 AM CEST, FUJITA Tomonori wrote:
> Add udelay() function, inserts a delay based on microseconds with busy
> waiting, in preparation for supporting read_poll_count_atomic().
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/helpers/time.c       |  5 +++++
>  rust/kernel/time/delay.rs | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 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..fb7c15dfe186 100644
> --- a/rust/kernel/time/delay.rs
> +++ b/rust/kernel/time/delay.rs
> @@ -47,3 +47,40 @@ 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.

I think clamping the given value to Delta::ZERO..=MAX_UDELAY_DELTA is the
correct thing to do and it should be mentioned by the documentation of the
function (as you do already).

However, if we want to consider it an error if an out of range value is given,
we should just return a Result. (A simple out of range condition, that can be
handled by the user easily shouldn't result into a potential WARN()).

Additionally, we can also have an infallible version of udelay() that evaluates
the delta at build time.

Another alternative would be to just clamp the value and call it a day (i.e. do
not consider out of range values to be an error).

I don't mind one or the other, but I'm against adding a WARN() for something
that can be handled with a Result easily *and* is already handled gracefully.

> +///
> +/// 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.
> +    // Note that the kernel is compiled with `-fno-strict-overflow`
> +    // so any out-of-range value could lead to unexpected behavior
> +    // but won't lead to undefined behavior.
> +    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 v2 1/2] rust: add udelay() function

[Miguel Ojeda]

On Tue, Oct 21, 2025 at 2:08 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> However, if we want to consider it an error if an out of range value is given,
> we should just return a Result. (A simple out of range condition, that can be
> handled by the user easily shouldn't result into a potential WARN()).

I think most callers that will hit this are just buggy, and C is EB
(or worse, UB) as well here, so `Result` isn't great.

I agree that a compile-time one is best, since many (most?) cases will
be constants anyway, so we should definitely have that and avoid
calling the runtime one as much as possible.

Now, for runtime values, since random drivers will call this with
possibly computed values based on who knows what, a warn once may be
too much. A debug assert instead would be less risky if it makes
people more comfortable.

Cheers,
Miguel

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

[Danilo Krummrich]

On 10/21/25 4:39 PM, Miguel Ojeda wrote:
> Now, for runtime values, since random drivers will call this with
> possibly computed values based on who knows what, a warn once may be
> too much. A debug assert instead would be less risky if it makes
> people more comfortable.

Exactly, also consider that MAX_UDELAY_MS depends on the architecture and HZ.

Given that we'd have a WARN() for any value passed that is > MAX_UDELAY_MS, and
given that WARN() is considered a vulnerability if hit (Greg, please correct me
if that's wrong), this would literally become a vulnerability generator. :)

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

[Miguel Ojeda]

On Tue, Oct 21, 2025 at 4:46 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> Given that we'd have a WARN() for any value passed that is > MAX_UDELAY_MS, and
> given that WARN() is considered a vulnerability if hit (Greg, please correct me
> if that's wrong), this would literally become a vulnerability generator. :)

Yes, but only if hit in a way that can be triggered by an attacker,
e.g. user actions, not in general.

i.e. that is why I was saying that someone could end up calculating a
delay value based on something e.g. user controlled, and then gets an
edge case wrong, and hits the `WARN()`, which is the "bad case" and a
CVE would be assigned.

A `pr_warn_once!` plus `debug_assert!` (or similar) should be a fine
way for having EB in functions, which is a useful concept without
leaving it UB liek in C nor going with a full `Result`. I can document
that combo.

Cheers,
Miguel

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

[Danilo Krummrich]

On Tue Oct 21, 2025 at 4:58 PM CEST, Miguel Ojeda wrote:
> On Tue, Oct 21, 2025 at 4:46 PM Danilo Krummrich <dakr@kernel.org> wrote:
>>
>> Given that we'd have a WARN() for any value passed that is > MAX_UDELAY_MS, and
>> given that WARN() is considered a vulnerability if hit (Greg, please correct me
>> if that's wrong), this would literally become a vulnerability generator. :)
>
> Yes, but only if hit in a way that can be triggered by an attacker,
> e.g. user actions, not in general.

Sure, that's what I assumed.

> i.e. that is why I was saying that someone could end up calculating a
> delay value based on something e.g. user controlled, and then gets an
> edge case wrong, and hits the `WARN()`, which is the "bad case" and a
> CVE would be assigned.
>
> A `pr_warn_once!` plus `debug_assert!` (or similar) should be a fine
> way for having EB in functions, which is a useful concept without
> leaving it UB liek in C nor going with a full `Result`. I can document
> that combo.

I'm not even sure we want that necessarily. I'd probably go for just documenting
that the value will be clamped to 0 <= value <= MAX_UDELAY_MS plus an internal
pr_debug!().

This way the function can also explicitly used in cases where the driver isn't
sure whether the value is in range and use it without duplicating the clamp
logic that the function already does internally anyways.

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

[Miguel Ojeda]

On Tue, Oct 21, 2025 at 5:09 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> I'm not even sure we want that necessarily. I'd probably go for just documenting
> that the value will be clamped to 0 <= value <= MAX_UDELAY_MS plus an internal
> pr_debug!().
>
> This way the function can also explicitly used in cases where the driver isn't
> sure whether the value is in range and use it without duplicating the clamp
> logic that the function already does internally anyways.

That would mean we cannot catch bugs in the common case where I would
expect callers to know what the delay range is.

i.e. if they aren't sure what the value is, then I would prefer they
clamp it explicitly on the callee side (or we provide an explicitly
clamped version if it is a common case, but it seems to me runtime
values are already the minority).

i.e. EB is useful because it allows us to catch bugs. If we remove the
EB, then it means things are more ambiguous and thus bugs cannot be
easily be caught anymore because one can actually rely on the
behavior.

Cheers,
Miguel

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

[Danilo Krummrich]

On Tue Oct 21, 2025 at 5:13 PM CEST, Miguel Ojeda wrote:
> i.e. if they aren't sure what the value is, then I would prefer they
> clamp it explicitly on the callee side (or we provide an explicitly
> clamped version if it is a common case, but it seems to me runtime
> values are already the minority).

Absolutely! Especially given the context udelay() is introduced
(read_poll_timeout_atomic()), the compile time checked version is what we really
want.

Maybe we should even defer a runtime checked / clamped version until it is
actually needed.

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

[FUJITA Tomonori]

On Tue, 21 Oct 2025 17:20:41 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:

> On Tue Oct 21, 2025 at 5:13 PM CEST, Miguel Ojeda wrote:
>> i.e. if they aren't sure what the value is, then I would prefer they
>> clamp it explicitly on the callee side (or we provide an explicitly
>> clamped version if it is a common case, but it seems to me runtime
>> values are already the minority).
> 
> Absolutely! Especially given the context udelay() is introduced
> (read_poll_timeout_atomic()), the compile time checked version is what we really
> want.
> 
> Maybe we should even defer a runtime checked / clamped version until it is
> actually needed.

Then perhaps something like this?

#[inline(always)]
pub fn udelay(delta: Delta) {
    build_assert!(
        delta.as_nanos() >= 0 && delta.as_nanos() <= i64::from(bindings::MAX_UDELAY_MS) * 1_000_000
    );

    // SAFETY: It is always safe to call `udelay()` with any duration.
    // Note that the kernel is compiled with `-fno-strict-overflow`
    // so any out-of-range value could lead to unexpected behavior
    // but won't lead to undefined behavior.
    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)
    }
}

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

[Alice Ryhl]

On Wed, Oct 22, 2025 at 07:32:30PM +0900, FUJITA Tomonori wrote:
> On Tue, 21 Oct 2025 17:20:41 +0200
> "Danilo Krummrich" <dakr@kernel.org> wrote:
> 
> > On Tue Oct 21, 2025 at 5:13 PM CEST, Miguel Ojeda wrote:
> >> i.e. if they aren't sure what the value is, then I would prefer they
> >> clamp it explicitly on the callee side (or we provide an explicitly
> >> clamped version if it is a common case, but it seems to me runtime
> >> values are already the minority).
> > 
> > Absolutely! Especially given the context udelay() is introduced
> > (read_poll_timeout_atomic()), the compile time checked version is what we really
> > want.
> > 
> > Maybe we should even defer a runtime checked / clamped version until it is
> > actually needed.
> 
> Then perhaps something like this?
> 
> #[inline(always)]
> pub fn udelay(delta: Delta) {
>     build_assert!(
>         delta.as_nanos() >= 0 && delta.as_nanos() <= i64::from(bindings::MAX_UDELAY_MS) * 1_000_000
>     );

This is a bad idea. Using build_assert! assert for range checks works
poorly, as we found for register index bounds checks.

If you really want to check it at compile-time, you'll need a wrapper
type around Delta that can only be constructed with delays in the right
range.

Alice

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

[FUJITA Tomonori]

On Wed, 22 Oct 2025 14:11:53 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> On Wed, Oct 22, 2025 at 07:32:30PM +0900, FUJITA Tomonori wrote:
>> On Tue, 21 Oct 2025 17:20:41 +0200
>> "Danilo Krummrich" <dakr@kernel.org> wrote:
>> 
>> > On Tue Oct 21, 2025 at 5:13 PM CEST, Miguel Ojeda wrote:
>> >> i.e. if they aren't sure what the value is, then I would prefer they
>> >> clamp it explicitly on the callee side (or we provide an explicitly
>> >> clamped version if it is a common case, but it seems to me runtime
>> >> values are already the minority).
>> > 
>> > Absolutely! Especially given the context udelay() is introduced
>> > (read_poll_timeout_atomic()), the compile time checked version is what we really
>> > want.
>> > 
>> > Maybe we should even defer a runtime checked / clamped version until it is
>> > actually needed.
>> 
>> Then perhaps something like this?
>> 
>> #[inline(always)]
>> pub fn udelay(delta: Delta) {
>>     build_assert!(
>>         delta.as_nanos() >= 0 && delta.as_nanos() <= i64::from(bindings::MAX_UDELAY_MS) * 1_000_000
>>     );
> 
> This is a bad idea. Using build_assert! assert for range checks works
> poorly, as we found for register index bounds checks.

Oh, I didn’t know about that. Do you have a pointer or some details I
could look at?


> If you really want to check it at compile-time, you'll need a wrapper
> type around Delta that can only be constructed with delays in the right
> range.

You meant that introducing a new type like UdelayDelta, right?

read_poll_timeout() and read_poll_timeout_atomic() use different Delta
types... I'm not sure it's a good idea.

Danilo and Miguel, any ideas for other ways we could do the
compile-time check?

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

[Andreas Hindborg]

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

> On Wed, 22 Oct 2025 14:11:53 +0000
> Alice Ryhl <aliceryhl@google.com> wrote:
>
>> On Wed, Oct 22, 2025 at 07:32:30PM +0900, FUJITA Tomonori wrote:
>>> On Tue, 21 Oct 2025 17:20:41 +0200
>>> "Danilo Krummrich" <dakr@kernel.org> wrote:
>>>
>>> > On Tue Oct 21, 2025 at 5:13 PM CEST, Miguel Ojeda wrote:
>>> >> i.e. if they aren't sure what the value is, then I would prefer they
>>> >> clamp it explicitly on the callee side (or we provide an explicitly
>>> >> clamped version if it is a common case, but it seems to me runtime
>>> >> values are already the minority).
>>> >
>>> > Absolutely! Especially given the context udelay() is introduced
>>> > (read_poll_timeout_atomic()), the compile time checked version is what we really
>>> > want.
>>> >
>>> > Maybe we should even defer a runtime checked / clamped version until it is
>>> > actually needed.
>>>
>>> Then perhaps something like this?
>>>
>>> #[inline(always)]
>>> pub fn udelay(delta: Delta) {
>>>     build_assert!(
>>>         delta.as_nanos() >= 0 && delta.as_nanos() <= i64::from(bindings::MAX_UDELAY_MS) * 1_000_000
>>>     );
>>
>> This is a bad idea. Using build_assert! assert for range checks works
>> poorly, as we found for register index bounds checks.
>
> Oh, I didn’t know about that. Do you have a pointer or some details I
> could look at?
>
>
>> If you really want to check it at compile-time, you'll need a wrapper
>> type around Delta that can only be constructed with delays in the right
>> range.
>
> You meant that introducing a new type like UdelayDelta, right?
>
> read_poll_timeout() and read_poll_timeout_atomic() use different Delta
> types... I'm not sure it's a good idea.

I would assume we keep this type private and only construct it in
`udelay`. @Alice, could you give a pointer on this approach?


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Wed, Oct 22, 2025 at 07:32:30PM +0900, FUJITA Tomonori wrote:
>> On Tue, 21 Oct 2025 17:20:41 +0200
>> "Danilo Krummrich" <dakr@kernel.org> wrote:
>>
>> > On Tue Oct 21, 2025 at 5:13 PM CEST, Miguel Ojeda wrote:
>> >> i.e. if they aren't sure what the value is, then I would prefer they
>> >> clamp it explicitly on the callee side (or we provide an explicitly
>> >> clamped version if it is a common case, but it seems to me runtime
>> >> values are already the minority).
>> >
>> > Absolutely! Especially given the context udelay() is introduced
>> > (read_poll_timeout_atomic()), the compile time checked version is what we really
>> > want.
>> >
>> > Maybe we should even defer a runtime checked / clamped version until it is
>> > actually needed.
>>
>> Then perhaps something like this?
>>
>> #[inline(always)]
>> pub fn udelay(delta: Delta) {
>>     build_assert!(
>>         delta.as_nanos() >= 0 && delta.as_nanos() <= i64::from(bindings::MAX_UDELAY_MS) * 1_000_000
>>     );
>
> This is a bad idea. Using build_assert! assert for range checks works
> poorly, as we found for register index bounds checks.

What was the issue you encountered here?


Best regards,
Andreas Hindborg

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

[Alice Ryhl]

On Fri, Oct 24, 2025 at 10:20:56AM +0200, Andreas Hindborg wrote:
> "Alice Ryhl" <aliceryhl@google.com> writes:
> 
> > On Wed, Oct 22, 2025 at 07:32:30PM +0900, FUJITA Tomonori wrote:
> >> On Tue, 21 Oct 2025 17:20:41 +0200
> >> "Danilo Krummrich" <dakr@kernel.org> wrote:
> >>
> >> > On Tue Oct 21, 2025 at 5:13 PM CEST, Miguel Ojeda wrote:
> >> >> i.e. if they aren't sure what the value is, then I would prefer they
> >> >> clamp it explicitly on the callee side (or we provide an explicitly
> >> >> clamped version if it is a common case, but it seems to me runtime
> >> >> values are already the minority).
> >> >
> >> > Absolutely! Especially given the context udelay() is introduced
> >> > (read_poll_timeout_atomic()), the compile time checked version is what we really
> >> > want.
> >> >
> >> > Maybe we should even defer a runtime checked / clamped version until it is
> >> > actually needed.
> >>
> >> Then perhaps something like this?
> >>
> >> #[inline(always)]
> >> pub fn udelay(delta: Delta) {
> >>     build_assert!(
> >>         delta.as_nanos() >= 0 && delta.as_nanos() <= i64::from(bindings::MAX_UDELAY_MS) * 1_000_000
> >>     );
> >
> > This is a bad idea. Using build_assert! assert for range checks works
> > poorly, as we found for register index bounds checks.
> 
> What was the issue you encountered here?

Basically, the problem is that it's too unreliable. The code does not
build unless the compiler can optimize out the build_error() call.
For range checks, seemingly unrelated code changes turn out to affect
these optimizations and break the code.

To make matters worse, the error message when a build_assert!() fails is
terrible. But even if it wasn't, the optimization issue is enough for me
to say we should not use it for range checks.

There have already been at least two instances where someone wasted a
lot of time and had to ask for help trying to debug a failing
build_assert!() used for bounds checks.

Alice

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

[Miguel Ojeda]

On Fri, Oct 24, 2025 at 11:27 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> For range checks, seemingly unrelated code changes turn out to affect
> these optimizations and break the code.

Most of these calls use constants, so in those cases it would be fine
(and otherwise it is really an issue on the optimizer).

But, yes, using `build_assert!` on the "normal" version of `udelay()`
will eventually surprise someone, because someone out there will start
using it with runtime values that happen to work and that later may
not when code gets shuffled around, especially given it shares the
name with C.

So for functions that do `build_assert!` on parameters we may want at
least a suffix with a particular word (e.g. `_const`) or similar, so
that it is clear calling them may have issues if not "obviously
constant for the optimizer", leaving the "normal" name for the runtime
one or the const generics one etc.

Here, I would suggest we do what we did for `fsleep()` and likely move
both to `debug_assert!` plus `pr_warn!` (and likely `pr_warn_once!`
when supported).

Hopefully we will get soon enough const generics that are flexible
enough -- but passing primitives seems bad here, we want the `Delta`.
So we may want the `udelay_const()` here still.

It pains me a bit that the common case would have the longer name, but
such is life.

Cheers,
Miguel

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

[FUJITA Tomonori]

On Fri, 24 Oct 2025 21:05:19 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Fri, Oct 24, 2025 at 11:27 AM Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> For range checks, seemingly unrelated code changes turn out to affect
>> these optimizations and break the code.
> 
> Most of these calls use constants, so in those cases it would be fine
> (and otherwise it is really an issue on the optimizer).
> 
> But, yes, using `build_assert!` on the "normal" version of `udelay()`
> will eventually surprise someone, because someone out there will start
> using it with runtime values that happen to work and that later may
> not when code gets shuffled around, especially given it shares the
> name with C.
> 
> So for functions that do `build_assert!` on parameters we may want at
> least a suffix with a particular word (e.g. `_const`) or similar, so
> that it is clear calling them may have issues if not "obviously
> constant for the optimizer", leaving the "normal" name for the runtime
> one or the const generics one etc.
> 
> Here, I would suggest we do what we did for `fsleep()` and likely move
> both to `debug_assert!` plus `pr_warn!` (and likely `pr_warn_once!`
> when supported).

I've sent v3 with debug_assert! added.

Is anyone currently working on pr_*_once? If I remember correctly,
there were a few proposals in the past, but they didn’t reach a
conclusion on how to implement it and never got merged.

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

[Miguel Ojeda]

On Sun, Oct 26, 2025 at 2:11 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> I've sent v3 with debug_assert! added.
>
> Is anyone currently working on pr_*_once? If I remember correctly,
> there were a few proposals in the past, but they didn’t reach a
> conclusion on how to implement it and never got merged.

Thanks Tomo -- I think the latest was:

    https://lore.kernel.org/rust-for-linux/20241126-pr_once_macros-v4-0-410b8ca9643e@tuta.io/

Cheers,
Miguel

[PATCH v2 2/2] rust: Add read_poll_count_atomic function

[FUJITA Tomonori]

Add read_poll_count_atomic function which polls periodically until a
condition is met, an error occurs, or the attempt limit is reached.

The C's read_poll_timeout_atomic() is used for the similar purpose.
In atomic context the timekeeping infrastructure is unavailable, so
reliable time-based timeouts cannot be implemented. So instead, the
helper accepts a maximum number of attempts and busy-waits (udelay +
cpu_relax) between tries.

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

diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
index 613eb25047ef..c7dd0816205f 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,78 @@ pub fn read_poll_timeout<Op, Cond, T>(
         cpu_relax();
     }
 }
+
+/// Polls periodically until a condition is met, an error occurs,
+/// or the attempt limit 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 attempt limit specified by `count` is reached.
+///
+/// # Errors
+///
+/// If `op` returns an error, then that error is returned directly.
+///
+/// If the attempt limit specified by `count` is reached, then
+/// `Err(ETIMEDOUT)` is returned.
+///
+/// # Examples
+///
+/// ```no_run
+/// use kernel::io::{Io, poll::read_poll_count_atomic};
+/// use kernel::time::Delta;
+///
+/// const HW_READY: u16 = 0x01;
+///
+/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result {
+///     match read_poll_count_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),
+///         1000,
+///     ) {
+///         Ok(_) => {
+///             // The hardware is ready. The returned value of the `op` closure
+///             // isn't used.
+///             Ok(())
+///         }
+///         Err(e) => Err(e),
+///     }
+/// }
+/// ```
+pub fn read_poll_count_atomic<Op, Cond, T>(
+    mut op: Op,
+    mut cond: Cond,
+    delay_delta: Delta,
+    count: usize,
+) -> Result<T>
+where
+    Op: FnMut() -> Result<T>,
+    Cond: FnMut(&T) -> bool,
+{
+    for _ in 0..count {
+        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 !delay_delta.is_zero() {
+            udelay(delay_delta);
+        }
+
+        cpu_relax();
+    }
+
+    Err(ETIMEDOUT)
+}
-- 
2.43.0

Re: [PATCH v2 2/2] rust: Add read_poll_count_atomic function

[Danilo Krummrich]

On Tue Oct 21, 2025 at 9:11 AM CEST, FUJITA Tomonori wrote:
> +/// Polls periodically until a condition is met, an error occurs,
> +/// or the attempt limit 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 attempt limit specified by `count` is reached.
> +///
> +/// # Errors
> +///
> +/// If `op` returns an error, then that error is returned directly.
> +///
> +/// If the attempt limit specified by `count` is reached, then
> +/// `Err(ETIMEDOUT)` is returned.
> +///
> +/// # Examples
> +///
> +/// ```no_run
> +/// use kernel::io::{Io, poll::read_poll_count_atomic};
> +/// use kernel::time::Delta;
> +///
> +/// const HW_READY: u16 = 0x01;
> +///
> +/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result {
> +///     match read_poll_count_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),
> +///         1000,
> +///     ) {
> +///         Ok(_) => {
> +///             // The hardware is ready. The returned value of the `op` closure
> +///             // isn't used.
> +///             Ok(())
> +///         }
> +///         Err(e) => Err(e),
> +///     }

Please replace the match statement with map().

	read_poll_count_atomic(
	    ...
	)
	.map(|_| ())

> +/// }
> +/// ```
> +pub fn read_poll_count_atomic<Op, Cond, T>(

I understand why you renamed the function, but read_poll_timeout_atomic() would
still be accurate -- it does perform a timeout in every iteration. Let's keep
the original name please.

> +    mut op: Op,
> +    mut cond: Cond,
> +    delay_delta: Delta,
> +    count: usize,

Maybe retry would be a slightly better fit compared to count. If we want to be a
bit more verbose, I suggest retry_count. :)

> +) -> Result<T>
> +where
> +    Op: FnMut() -> Result<T>,
> +    Cond: FnMut(&T) -> bool,
> +{
> +    for _ in 0..count {
> +        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: Just like in read_poll_timeout() I think this comment does not carry much
value, but I'm fine if you want to keep it.

> +            return Ok(val);
> +        }
> +
> +        if !delay_delta.is_zero() {
> +            udelay(delay_delta);
> +        }
> +
> +        cpu_relax();
> +    }
> +
> +    Err(ETIMEDOUT)
> +}
> -- 
> 2.43.0

Re: [PATCH v2 2/2] rust: Add read_poll_count_atomic function

[Alice Ryhl]

On Tue, Oct 21, 2025 at 02:35:34PM +0200, Danilo Krummrich wrote:
> On Tue Oct 21, 2025 at 9:11 AM CEST, FUJITA Tomonori wrote:
> > +/// Polls periodically until a condition is met, an error occurs,
> > +/// or the attempt limit 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 attempt limit specified by `count` is reached.
> > +///
> > +/// # Errors
> > +///
> > +/// If `op` returns an error, then that error is returned directly.
> > +///
> > +/// If the attempt limit specified by `count` is reached, then
> > +/// `Err(ETIMEDOUT)` is returned.
> > +///
> > +/// # Examples
> > +///
> > +/// ```no_run
> > +/// use kernel::io::{Io, poll::read_poll_count_atomic};
> > +/// use kernel::time::Delta;
> > +///
> > +/// const HW_READY: u16 = 0x01;
> > +///
> > +/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result {
> > +///     match read_poll_count_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),
> > +///         1000,
> > +///     ) {
> > +///         Ok(_) => {
> > +///             // The hardware is ready. The returned value of the `op` closure
> > +///             // isn't used.
> > +///             Ok(())
> > +///         }
> > +///         Err(e) => Err(e),
> > +///     }
> 
> Please replace the match statement with map().
> 
> 	read_poll_count_atomic(
> 	    ...
> 	)
> 	.map(|_| ())
> 

IMO, this should instead be:

	read_poll_count_atomic(
	    ...
	)?
	Ok(())

Alice

Re: [PATCH v2 2/2] rust: Add read_poll_count_atomic function

[Danilo Krummrich]

On 10/21/25 4:05 PM, Alice Ryhl wrote:
> On Tue, Oct 21, 2025 at 02:35:34PM +0200, Danilo Krummrich wrote:
>> Please replace the match statement with map().
>>
>> 	read_poll_count_atomic(
>> 	    ...
>> 	)
>> 	.map(|_| ())
>>
> 
> IMO, this should instead be:
> 
> 	read_poll_count_atomic(
> 	    ...
> 	)?
> 	Ok(())

I think map() has the advantage that it is a bit more explicit about the fact
that the return value is discarded intentionally.

But I'm fine with either version.

Re: [PATCH v2 2/2] rust: Add read_poll_count_atomic function

[FUJITA Tomonori]

On Tue, 21 Oct 2025 18:02:39 +0200
Danilo Krummrich <dakr@kernel.org> wrote:

> On 10/21/25 4:05 PM, Alice Ryhl wrote:
>> On Tue, Oct 21, 2025 at 02:35:34PM +0200, Danilo Krummrich wrote:
>>> Please replace the match statement with map().
>>>
>>> 	read_poll_count_atomic(
>>> 	    ...
>>> 	)
>>> 	.map(|_| ())
>>>
>> 
>> IMO, this should instead be:
>> 
>> 	read_poll_count_atomic(
>> 	    ...
>> 	)?
>> 	Ok(())
> 
> I think map() has the advantage that it is a bit more explicit about the fact
> that the return value is discarded intentionally.
> 
> But I'm fine with either version.

Then I'll go with 'Ok'.

I'll send to a different patch to update read_poll_wait's example for
the same change.

Re: [PATCH v2 2/2] rust: Add read_poll_count_atomic function

[Andreas Hindborg]

"Alice Ryhl" <aliceryhl@google.com> writes:

> On Tue, Oct 21, 2025 at 02:35:34PM +0200, Danilo Krummrich wrote:
>> On Tue Oct 21, 2025 at 9:11 AM CEST, FUJITA Tomonori wrote:
>> > +/// Polls periodically until a condition is met, an error occurs,
>> > +/// or the attempt limit 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 attempt limit specified by `count` is reached.
>> > +///
>> > +/// # Errors
>> > +///
>> > +/// If `op` returns an error, then that error is returned directly.
>> > +///
>> > +/// If the attempt limit specified by `count` is reached, then
>> > +/// `Err(ETIMEDOUT)` is returned.
>> > +///
>> > +/// # Examples
>> > +///
>> > +/// ```no_run
>> > +/// use kernel::io::{Io, poll::read_poll_count_atomic};
>> > +/// use kernel::time::Delta;
>> > +///
>> > +/// const HW_READY: u16 = 0x01;
>> > +///
>> > +/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result {
>> > +///     match read_poll_count_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),
>> > +///         1000,
>> > +///     ) {
>> > +///         Ok(_) => {
>> > +///             // The hardware is ready. The returned value of the `op` closure
>> > +///             // isn't used.
>> > +///             Ok(())
>> > +///         }
>> > +///         Err(e) => Err(e),
>> > +///     }
>>
>> Please replace the match statement with map().
>>
>> 	read_poll_count_atomic(
>> 	    ...
>> 	)
>> 	.map(|_| ())
>>
>
> IMO, this should instead be:
>
> 	read_poll_count_atomic(
> 	    ...
> 	)?
> 	Ok(())

It does not really matter to me. Why do you prefer one to the other?


Best regards,
Andreas Hindborg

Re: [PATCH v2 2/2] rust: Add read_poll_count_atomic function

[Alice Ryhl]

On Fri, Oct 24, 2025 at 10:25:05AM +0200, Andreas Hindborg wrote:
> "Alice Ryhl" <aliceryhl@google.com> writes:
> 
> > On Tue, Oct 21, 2025 at 02:35:34PM +0200, Danilo Krummrich wrote:
> >> On Tue Oct 21, 2025 at 9:11 AM CEST, FUJITA Tomonori wrote:
> >> > +/// Polls periodically until a condition is met, an error occurs,
> >> > +/// or the attempt limit 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 attempt limit specified by `count` is reached.
> >> > +///
> >> > +/// # Errors
> >> > +///
> >> > +/// If `op` returns an error, then that error is returned directly.
> >> > +///
> >> > +/// If the attempt limit specified by `count` is reached, then
> >> > +/// `Err(ETIMEDOUT)` is returned.
> >> > +///
> >> > +/// # Examples
> >> > +///
> >> > +/// ```no_run
> >> > +/// use kernel::io::{Io, poll::read_poll_count_atomic};
> >> > +/// use kernel::time::Delta;
> >> > +///
> >> > +/// const HW_READY: u16 = 0x01;
> >> > +///
> >> > +/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result {
> >> > +///     match read_poll_count_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),
> >> > +///         1000,
> >> > +///     ) {
> >> > +///         Ok(_) => {
> >> > +///             // The hardware is ready. The returned value of the `op` closure
> >> > +///             // isn't used.
> >> > +///             Ok(())
> >> > +///         }
> >> > +///         Err(e) => Err(e),
> >> > +///     }
> >>
> >> Please replace the match statement with map().
> >>
> >> 	read_poll_count_atomic(
> >> 	    ...
> >> 	)
> >> 	.map(|_| ())
> >>
> >
> > IMO, this should instead be:
> >
> > 	read_poll_count_atomic(
> > 	    ...
> > 	)?
> > 	Ok(())
> 
> It does not really matter to me. Why do you prefer one to the other?

I think it's simpler.

Alice

Re: [PATCH v2 2/2] rust: Add read_poll_count_atomic function

[FUJITA Tomonori]

On Tue, 21 Oct 2025 14:35:34 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:

>> +pub fn read_poll_count_atomic<Op, Cond, T>(
> 
> I understand why you renamed the function, but read_poll_timeout_atomic() would
> still be accurate -- it does perform a timeout in every iteration. Let's keep
> the original name please.

Ok, I'll revert the name.

>> +    mut op: Op,
>> +    mut cond: Cond,
>> +    delay_delta: Delta,
>> +    count: usize,
> 
> Maybe retry would be a slightly better fit compared to count. If we want to be a
> bit more verbose, I suggest retry_count. :)

Sure, I'll go with 'retry'.