linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] rust: Add read_poll_timeout
@ 2025-08-17  4:47 FUJITA Tomonori
  2025-08-17  4:47 ` [PATCH v2 1/2] rust: Add cpu_relax() helper FUJITA Tomonori
  2025-08-17  4:47 ` [PATCH v2 2/2] rust: Add read_poll_timeout functions FUJITA Tomonori
  0 siblings, 2 replies; 9+ messages in thread
From: FUJITA Tomonori @ 2025-08-17  4:47 UTC (permalink / raw)
  To: a.hindborg, alex.gaynor, ojeda
  Cc: aliceryhl, anna-maria, bjorn3_gh, boqun.feng, dakr, frederic,
	gary, jstultz, linux-kernel, lossin, lyude, rust-for-linux, sboyd,
	tglx, tmgross, acourbot, daniel.almeida

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

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

I put this function kernel/io/poll.rs. This function is not
necessarily related to I/O though. I don't think this function
perfectly matches the existing abstraction. Suggestions for a more
appropriate place are welcome.

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

v2
- make cpu_relax() inline
- remove the example code to call might_sleep with a lock hold
- move kernel/time/poll.rs to kernel/io/poll.rs
- remove the Option for timeout argument
- avoid the extra variable, sleep.
- update the comment and commit message
- writing closures directly inline in the example code
v1: https://lore.kernel.org/lkml/20250811041039.3231548-1-fujita.tomonori@gmail.com/
The changes since the last posting [2] are
- removed might_sleep() change since it was already merged separately.
- split out cpu_relax() in a separate patch
- make the example code compilable
- update the code to use Clocksource (MONOTONIC)
- call might_sleep() always (even when the function doesn't sleep)
- cosmetic changes to the doc

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

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

 rust/helpers/helpers.c   |  1 +
 rust/helpers/processor.c |  8 ++++
 rust/kernel/io.rs        |  1 +
 rust/kernel/io/poll.rs   | 96 ++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs       |  1 +
 rust/kernel/processor.rs | 14 ++++++
 6 files changed, 121 insertions(+)
 create mode 100644 rust/helpers/processor.c
 create mode 100644 rust/kernel/io/poll.rs
 create mode 100644 rust/kernel/processor.rs


base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
-- 
2.43.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/2] rust: Add cpu_relax() helper
  2025-08-17  4:47 [PATCH v2 0/2] rust: Add read_poll_timeout FUJITA Tomonori
@ 2025-08-17  4:47 ` FUJITA Tomonori
  2025-08-19 18:32   ` Daniel Almeida
  2025-08-17  4:47 ` [PATCH v2 2/2] rust: Add read_poll_timeout functions FUJITA Tomonori
  1 sibling, 1 reply; 9+ messages in thread
From: FUJITA Tomonori @ 2025-08-17  4:47 UTC (permalink / raw)
  To: a.hindborg, alex.gaynor, ojeda
  Cc: aliceryhl, anna-maria, bjorn3_gh, boqun.feng, dakr, frederic,
	gary, jstultz, linux-kernel, lossin, lyude, rust-for-linux, sboyd,
	tglx, tmgross, acourbot, daniel.almeida

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

Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers/helpers.c   |  1 +
 rust/helpers/processor.c |  8 ++++++++
 rust/kernel/lib.rs       |  1 +
 rust/kernel/processor.rs | 14 ++++++++++++++
 4 files changed, 24 insertions(+)
 create mode 100644 rust/helpers/processor.c
 create mode 100644 rust/kernel/processor.rs

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 7cf7fe95e41d..04598665e7c8 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -34,6 +34,7 @@
 #include "pid_namespace.c"
 #include "platform.c"
 #include "poll.c"
+#include "processor.c"
 #include "property.c"
 #include "rbtree.c"
 #include "rcu.c"
diff --git a/rust/helpers/processor.c b/rust/helpers/processor.c
new file mode 100644
index 000000000000..d41355e14d6e
--- /dev/null
+++ b/rust/helpers/processor.c
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/processor.h>
+
+void rust_helper_cpu_relax(void)
+{
+	cpu_relax();
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ed53169e795c..c098c47c1817 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -110,6 +110,7 @@
 pub mod platform;
 pub mod prelude;
 pub mod print;
+pub mod processor;
 pub mod rbtree;
 pub mod regulator;
 pub mod revocable;
diff --git a/rust/kernel/processor.rs b/rust/kernel/processor.rs
new file mode 100644
index 000000000000..85b49b3614dd
--- /dev/null
+++ b/rust/kernel/processor.rs
@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Processor related primitives.
+//!
+//! C header: [`include/linux/processor.h`](srctree/include/linux/processor.h)
+
+/// Lower CPU power consumption or yield to a hyperthreaded twin processor.
+///
+/// It also happens to serve as a compiler barrier.
+#[inline]
+pub fn cpu_relax() {
+    // SAFETY: Always safe to call.
+    unsafe { bindings::cpu_relax() }
+}
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/2] rust: Add read_poll_timeout functions
  2025-08-17  4:47 [PATCH v2 0/2] rust: Add read_poll_timeout FUJITA Tomonori
  2025-08-17  4:47 ` [PATCH v2 1/2] rust: Add cpu_relax() helper FUJITA Tomonori
@ 2025-08-17  4:47 ` FUJITA Tomonori
  2025-08-19  0:49   ` Alexandre Courbot
                     ` (2 more replies)
  1 sibling, 3 replies; 9+ messages in thread
From: FUJITA Tomonori @ 2025-08-17  4:47 UTC (permalink / raw)
  To: a.hindborg, alex.gaynor, ojeda
  Cc: aliceryhl, anna-maria, bjorn3_gh, boqun.feng, dakr, frederic,
	gary, jstultz, linux-kernel, lossin, lyude, rust-for-linux, sboyd,
	tglx, tmgross, acourbot, daniel.almeida, Fiona Behrens

Add read_poll_timeout function which poll periodically until a
condition is met or a timeout is reached.

The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
macro and a simple wrapper for Rust doesn't work. So this implements
the same functionality in Rust.

The C version uses usleep_range() while the Rust version uses
fsleep(), which uses the best sleep method so it works with spans that
usleep_range() doesn't work nicely with.

The sleep_before_read argument isn't supported since there is no user
for now. It's rarely used in the C version.

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Reviewed-by: Fiona Behrens <me@kloenk.dev>
Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/io.rs      |  1 +
 rust/kernel/io/poll.rs | 96 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 97 insertions(+)
 create mode 100644 rust/kernel/io/poll.rs

diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index 03b467722b86..ee182b0b5452 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -8,6 +8,7 @@
 use crate::{bindings, build_assert, ffi::c_void};
 
 pub mod mem;
+pub mod poll;
 pub mod resource;
 
 pub use resource::Resource;
diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
new file mode 100644
index 000000000000..e6325725d5a3
--- /dev/null
+++ b/rust/kernel/io/poll.rs
@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! IO polling.
+//!
+//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
+
+use crate::{
+    error::{code::*, Result},
+    processor::cpu_relax,
+    task::might_sleep,
+    time::{delay::fsleep, Delta, Instant, Monotonic},
+};
+
+/// Polls periodically until a condition is met or a timeout is reached.
+///
+/// The function repeatedly executes the given operation `op` closure and
+/// checks its result using the condition closure `cond`.
+///
+/// If `cond` returns `true`, the function returns successfully with the result of `op`.
+/// Otherwise, it waits for a duration specified by `sleep_delta`
+/// before executing `op` again.
+///
+/// This process continues until either `cond` returns `true` or the timeout,
+/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
+/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
+///
+/// This function can only be used in a nonatomic context.
+///
+/// # Examples
+///
+/// ```no_run
+/// use kernel::io::{Io, poll::read_poll_timeout};
+/// use kernel::time::Delta;
+///
+/// const HW_READY: u16 = 0x01;
+///
+/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result<()> {
+///     match read_poll_timeout(
+///         // The `op` closure reads the value of a specific status register.
+///         || io.try_read16(0x1000),
+///         // The `cond` closure takes a reference to the value returned by `op`
+///         // and checks whether the hardware is ready.
+///         |val: &u16| *val == HW_READY,
+///         Delta::from_millis(50),
+///         Delta::from_secs(3),
+///     ) {
+///         Ok(_) => {
+///             // The hardware is ready. The returned value of the `op` closure
+///             // isn't used.
+///             Ok(())
+///         }
+///         Err(e) => Err(e),
+///     }
+/// }
+/// ```
+#[track_caller]
+pub fn read_poll_timeout<Op, Cond, T>(
+    mut op: Op,
+    mut cond: Cond,
+    sleep_delta: Delta,
+    timeout_delta: Delta,
+) -> Result<T>
+where
+    Op: FnMut() -> Result<T>,
+    Cond: FnMut(&T) -> bool,
+{
+    let start: Instant<Monotonic> = Instant::now();
+
+    // Unlike the C version, we always call `might_sleep()` unconditionally,
+    // as conditional calls are error-prone. We clearly separate
+    // `read_poll_timeout()` and `read_poll_timeout_atomic()` to aid
+    // tools like klint.
+    might_sleep();
+
+    loop {
+        let val = op()?;
+        if cond(&val) {
+            // Unlike the C version, we immediately return.
+            // We know the condition is met so we don't need to check again.
+            return Ok(val);
+        }
+
+        if start.elapsed() > timeout_delta {
+            // Unlike the C version, we immediately return.
+            // We have just called `op()` so we don't need to call it again.
+            return Err(ETIMEDOUT);
+        }
+
+        if !sleep_delta.is_zero() {
+            fsleep(sleep_delta);
+        }
+
+        // fsleep() could be busy-wait loop so we always call cpu_relax().
+        cpu_relax();
+    }
+}
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] rust: Add read_poll_timeout functions
  2025-08-17  4:47 ` [PATCH v2 2/2] rust: Add read_poll_timeout functions FUJITA Tomonori
@ 2025-08-19  0:49   ` Alexandre Courbot
  2025-08-20 11:11     ` FUJITA Tomonori
  2025-08-19 18:30   ` Daniel Almeida
  2025-08-20 10:02   ` Alice Ryhl
  2 siblings, 1 reply; 9+ messages in thread
From: Alexandre Courbot @ 2025-08-19  0:49 UTC (permalink / raw)
  To: FUJITA Tomonori, a.hindborg, alex.gaynor, ojeda
  Cc: aliceryhl, anna-maria, bjorn3_gh, boqun.feng, dakr, frederic,
	gary, jstultz, linux-kernel, lossin, lyude, rust-for-linux, sboyd,
	tglx, tmgross, daniel.almeida, Fiona Behrens

On Sun Aug 17, 2025 at 1:47 PM JST, FUJITA Tomonori wrote:
> Add read_poll_timeout function which poll periodically until a
> condition is met or a timeout is reached.
>
> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
> macro and a simple wrapper for Rust doesn't work. So this implements
> the same functionality in Rust.
>
> The C version uses usleep_range() while the Rust version uses
> fsleep(), which uses the best sleep method so it works with spans that
> usleep_range() doesn't work nicely with.
>
> The sleep_before_read argument isn't supported since there is no user
> for now. It's rarely used in the C version.
>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Reviewed-by: Fiona Behrens <me@kloenk.dev>
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Tested this with nova-core, and it seems to work fine!

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Tested-by: Alexandre Courbot <acourbot@nvidia.com>

Just one last comment about the documentation below.

<snip>
> +/// Polls periodically until a condition is met or a timeout is reached.
> +///
> +/// The function repeatedly executes the given operation `op` closure and
> +/// checks its result using the condition closure `cond`.
> +///
> +/// If `cond` returns `true`, the function returns successfully with the result of `op`.
> +/// Otherwise, it waits for a duration specified by `sleep_delta`
> +/// before executing `op` again.
> +///
> +/// This process continues until either `cond` returns `true` or the timeout,
> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,

For precision: "This process continues until either `op` returns and
error, `cond` returns `true`, or the timeout specified by
`timeout_delta` is reached."

> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
> +///
> +/// This function can only be used in a nonatomic context.

Here I'd add an errors section:

# Errors

If `op` returns an error, then that error is returned directly.

If the timeout specified by `timeout_delta` is reached, then
`Err(ETIMEDOUT)` is returned.

> +///
> +/// # Examples
> +///
> +/// ```no_run
> +/// use kernel::io::{Io, poll::read_poll_timeout};
> +/// use kernel::time::Delta;
> +///
> +/// const HW_READY: u16 = 0x01;
> +///
> +/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result<()> {
> +///     match read_poll_timeout(
> +///         // The `op` closure reads the value of a specific status register.
> +///         || io.try_read16(0x1000),
> +///         // The `cond` closure takes a reference to the value returned by `op`
> +///         // and checks whether the hardware is ready.
> +///         |val: &u16| *val == HW_READY,
> +///         Delta::from_millis(50),
> +///         Delta::from_secs(3),
> +///     ) {
> +///         Ok(_) => {
> +///             // The hardware is ready. The returned value of the `op` closure
> +///             // isn't used.
> +///             Ok(())
> +///         }
> +///         Err(e) => Err(e),
> +///     }
> +/// }
> +/// ```
> +#[track_caller]
> +pub fn read_poll_timeout<Op, Cond, T>(
> +    mut op: Op,
> +    mut cond: Cond,
> +    sleep_delta: Delta,
> +    timeout_delta: Delta,
> +) -> Result<T>
> +where
> +    Op: FnMut() -> Result<T>,
> +    Cond: FnMut(&T) -> bool,
> +{
> +    let start: Instant<Monotonic> = Instant::now();
> +
> +    // Unlike the C version, we always call `might_sleep()` unconditionally,
> +    // as conditional calls are error-prone. We clearly separate
> +    // `read_poll_timeout()` and `read_poll_timeout_atomic()` to aid
> +    // tools like klint.
> +    might_sleep();
> +
> +    loop {
> +        let val = op()?;
> +        if cond(&val) {
> +            // Unlike the C version, we immediately return.
> +            // We know the condition is met so we don't need to check again.

nit: this comment looks superfluous to me, this is a different
implementation from the C version anyway.

> +            return Ok(val);
> +        }
> +
> +        if start.elapsed() > timeout_delta {
> +            // Unlike the C version, we immediately return.
> +            // We have just called `op()` so we don't need to call it again.

Same here.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] rust: Add read_poll_timeout functions
  2025-08-17  4:47 ` [PATCH v2 2/2] rust: Add read_poll_timeout functions FUJITA Tomonori
  2025-08-19  0:49   ` Alexandre Courbot
@ 2025-08-19 18:30   ` Daniel Almeida
  2025-08-20  6:45     ` FUJITA Tomonori
  2025-08-20 10:02   ` Alice Ryhl
  2 siblings, 1 reply; 9+ messages in thread
From: Daniel Almeida @ 2025-08-19 18:30 UTC (permalink / raw)
  To: FUJITA Tomonori
  Cc: a.hindborg, alex.gaynor, ojeda, aliceryhl, anna-maria, bjorn3_gh,
	boqun.feng, dakr, frederic, gary, jstultz, linux-kernel, lossin,
	lyude, rust-for-linux, sboyd, tglx, tmgross, acourbot,
	Fiona Behrens

Hi Fujita,

> On 17 Aug 2025, at 01:47, FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> 
> Add read_poll_timeout function which poll periodically until a
> condition is met or a timeout is reached.
> 
> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
> macro and a simple wrapper for Rust doesn't work. So this implements
> the same functionality in Rust.
> 
> The C version uses usleep_range() while the Rust version uses
> fsleep(), which uses the best sleep method so it works with spans that
> usleep_range() doesn't work nicely with.
> 
> The sleep_before_read argument isn't supported since there is no user
> for now. It's rarely used in the C version.
> 
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Reviewed-by: Fiona Behrens <me@kloenk.dev>
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
> rust/kernel/io.rs      |  1 +
> rust/kernel/io/poll.rs | 96 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 97 insertions(+)
> create mode 100644 rust/kernel/io/poll.rs
> 
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index 03b467722b86..ee182b0b5452 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -8,6 +8,7 @@
> use crate::{bindings, build_assert, ffi::c_void};
> 
> pub mod mem;
> +pub mod poll;
> pub mod resource;
> 
> pub use resource::Resource;
> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
> new file mode 100644
> index 000000000000..e6325725d5a3
> --- /dev/null
> +++ b/rust/kernel/io/poll.rs
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! IO polling.
> +//!
> +//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
> +
> +use crate::{
> +    error::{code::*, Result},
> +    processor::cpu_relax,
> +    task::might_sleep,
> +    time::{delay::fsleep, Delta, Instant, Monotonic},
> +};
> +
> +/// Polls periodically until a condition is met or a timeout is reached.
> +///
> +/// The function repeatedly executes the given operation `op` closure and
> +/// checks its result using the condition closure `cond`.
> +///
> +/// If `cond` returns `true`, the function returns successfully with the result of `op`.
> +/// Otherwise, it waits for a duration specified by `sleep_delta`
> +/// before executing `op` again.
> +///
> +/// This process continues until either `cond` returns `true` or the timeout,
> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
> +///
> +/// This function can only be used in a nonatomic context.
> +///
> +/// # Examples
> +///
> +/// ```no_run
> +/// use kernel::io::{Io, poll::read_poll_timeout};
> +/// use kernel::time::Delta;
> +///
> +/// const HW_READY: u16 = 0x01;
> +///
> +/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result<()> {
> +///     match read_poll_timeout(
> +///         // The `op` closure reads the value of a specific status register.
> +///         || io.try_read16(0x1000),
> +///         // The `cond` closure takes a reference to the value returned by `op`
> +///         // and checks whether the hardware is ready.
> +///         |val: &u16| *val == HW_READY,
> +///         Delta::from_millis(50),
> +///         Delta::from_secs(3),
> +///     ) {
> +///         Ok(_) => {
> +///             // The hardware is ready. The returned value of the `op` closure
> +///             // isn't used.
> +///             Ok(())
> +///         }
> +///         Err(e) => Err(e),
> +///     }
> +/// }
> +/// ```
> +#[track_caller]
> +pub fn read_poll_timeout<Op, Cond, T>(
> +    mut op: Op,
> +    mut cond: Cond,
> +    sleep_delta: Delta,
> +    timeout_delta: Delta,
> +) -> Result<T>
> +where
> +    Op: FnMut() -> Result<T>,
> +    Cond: FnMut(&T) -> bool,
> +{
> +    let start: Instant<Monotonic> = Instant::now();
> +
> +    // Unlike the C version, we always call `might_sleep()` unconditionally,
> +    // as conditional calls are error-prone. We clearly separate
> +    // `read_poll_timeout()` and `read_poll_timeout_atomic()` to aid
> +    // tools like klint.
> +    might_sleep();
> +
> +    loop {
> +        let val = op()?;
> +        if cond(&val) {
> +            // Unlike the C version, we immediately return.
> +            // We know the condition is met so we don't need to check again.
> +            return Ok(val);
> +        }
> +
> +        if start.elapsed() > timeout_delta {
> +            // Unlike the C version, we immediately return.
> +            // We have just called `op()` so we don't need to call it again.
> +            return Err(ETIMEDOUT);
> +        }
> +
> +        if !sleep_delta.is_zero() {
> +            fsleep(sleep_delta);
> +        }
> +
> +        // fsleep() could be busy-wait loop so we always call cpu_relax().
> +        cpu_relax();
> +    }
> +}
> -- 
> 2.43.0
> 
> 

Thanks for working on this. Definitely going to be needed by a lot of drivers.

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

How is the atomic version going to look like? The same, except for
might_sleep() and without the sleep_delta argument?


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/2] rust: Add cpu_relax() helper
  2025-08-17  4:47 ` [PATCH v2 1/2] rust: Add cpu_relax() helper FUJITA Tomonori
@ 2025-08-19 18:32   ` Daniel Almeida
  0 siblings, 0 replies; 9+ messages in thread
From: Daniel Almeida @ 2025-08-19 18:32 UTC (permalink / raw)
  To: FUJITA Tomonori
  Cc: a.hindborg, alex.gaynor, ojeda, aliceryhl, anna-maria, bjorn3_gh,
	boqun.feng, dakr, frederic, gary, jstultz, linux-kernel, lossin,
	lyude, rust-for-linux, sboyd, tglx, tmgross, acourbot



> On 17 Aug 2025, at 01:47, FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> 
> Add cpu_relax() helper in preparation for supporting
> read_poll_timeout().
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
> rust/helpers/helpers.c   |  1 +
> rust/helpers/processor.c |  8 ++++++++
> rust/kernel/lib.rs       |  1 +
> rust/kernel/processor.rs | 14 ++++++++++++++
> 4 files changed, 24 insertions(+)
> create mode 100644 rust/helpers/processor.c
> create mode 100644 rust/kernel/processor.rs
> 
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 7cf7fe95e41d..04598665e7c8 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -34,6 +34,7 @@
> #include "pid_namespace.c"
> #include "platform.c"
> #include "poll.c"
> +#include "processor.c"
> #include "property.c"
> #include "rbtree.c"
> #include "rcu.c"
> diff --git a/rust/helpers/processor.c b/rust/helpers/processor.c
> new file mode 100644
> index 000000000000..d41355e14d6e
> --- /dev/null
> +++ b/rust/helpers/processor.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/processor.h>
> +
> +void rust_helper_cpu_relax(void)
> +{
> + cpu_relax();
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index ed53169e795c..c098c47c1817 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -110,6 +110,7 @@
> pub mod platform;
> pub mod prelude;
> pub mod print;
> +pub mod processor;
> pub mod rbtree;
> pub mod regulator;
> pub mod revocable;
> diff --git a/rust/kernel/processor.rs b/rust/kernel/processor.rs
> new file mode 100644
> index 000000000000..85b49b3614dd
> --- /dev/null
> +++ b/rust/kernel/processor.rs
> @@ -0,0 +1,14 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Processor related primitives.
> +//!
> +//! C header: [`include/linux/processor.h`](srctree/include/linux/processor.h)
> +
> +/// Lower CPU power consumption or yield to a hyperthreaded twin processor.
> +///
> +/// It also happens to serve as a compiler barrier.
> +#[inline]
> +pub fn cpu_relax() {
> +    // SAFETY: Always safe to call.
> +    unsafe { bindings::cpu_relax() }
> +}
> -- 
> 2.43.0
> 
> 

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] rust: Add read_poll_timeout functions
  2025-08-19 18:30   ` Daniel Almeida
@ 2025-08-20  6:45     ` FUJITA Tomonori
  0 siblings, 0 replies; 9+ messages in thread
From: FUJITA Tomonori @ 2025-08-20  6:45 UTC (permalink / raw)
  To: daniel.almeida
  Cc: fujita.tomonori, a.hindborg, alex.gaynor, ojeda, aliceryhl,
	anna-maria, bjorn3_gh, boqun.feng, dakr, frederic, gary, jstultz,
	linux-kernel, lossin, lyude, rust-for-linux, sboyd, tglx, tmgross,
	acourbot, me

On Tue, 19 Aug 2025 15:30:51 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

> Thanks for working on this. Definitely going to be needed by a lot of drivers.
> 
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>

Thanks!

> How is the atomic version going to look like? The same, except for
> might_sleep() and without the sleep_delta argument?

If we follow the C implementation, it will be different; C's
read_poll_atomic doesn't use ktime to calculate a timeout.

It would look like the following. I think that the read_poll_timeout
patchset is almost complete so I'll send the
read_poll_timeout_atomic() patchset shortly.

diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
index e6325725d5a3..dc4f1ecdf31f 100644
--- a/rust/kernel/io/poll.rs
+++ b/rust/kernel/io/poll.rs
@@ -8,7 +8,10 @@
     error::{code::*, Result},
     processor::cpu_relax,
     task::might_sleep,
-    time::{delay::fsleep, Delta, Instant, Monotonic},
+    time::{
+        delay::{fsleep, udelay},
+        Delta, Instant, Monotonic,
+    },
 };
 
 /// Polls periodically until a condition is met or a timeout is reached.
@@ -94,3 +97,86 @@ pub fn read_poll_timeout<Op, Cond, T>(
         cpu_relax();
     }
 }
+
+/// Polls periodically until a condition is met or a timeout is reached.
+///
+/// The function repeatedly executes the given operation `op` closure and
+/// checks its result using the condition closure `cond`.
+///
+/// If `cond` returns `true`, the function returns successfully with the result of `op`.
+/// Otherwise, it performs a busy wait for a duration specified by `delay_delta`
+/// before executing `op` again.
+///
+/// This process continues until either `cond` returns `true` or the timeout,
+/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
+/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
+///
+/// # Examples
+///
+/// ```no_run
+/// use kernel::io::{Io, poll::read_poll_timeout_atomic};
+/// use kernel::time::Delta;
+///
+/// const HW_READY: u16 = 0x01;
+///
+/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result<()> {
+///     match read_poll_timeout_atomic(
+///         // The `op` closure reads the value of a specific status register.
+///         || io.try_read16(0x1000),
+///         // The `cond` closure takes a reference to the value returned by `op`
+///         // and checks whether the hardware is ready.
+///         |val: &u16| *val == HW_READY,
+///         Delta::from_micros(50),
+///         Delta::from_micros(300),
+///     ) {
+///         Ok(_) => {
+///             // The hardware is ready. The returned value of the `op` closure
+///             // isn't used.
+///             Ok(())
+///         }
+///         Err(e) => Err(e),
+///     }
+/// }
+/// ```
+pub fn read_poll_timeout_atomic<Op, Cond, T>(
+    mut op: Op,
+    mut cond: Cond,
+    delay_delta: Delta,
+    timeout_delta: Delta,
+) -> Result<T>
+where
+    Op: FnMut() -> Result<T>,
+    Cond: FnMut(&T) -> bool,
+{
+    let mut left_ns = timeout_delta.as_nanos();
+    let delay_ns = delay_delta.as_nanos();
+
+    let timeout_is_zero = timeout_delta.is_zero();
+
+    loop {
+        let val = op()?;
+        if cond(&val) {
+            // Unlike the C version, we immediately return.
+            // We know the condition is met so we don't need to check again.
+            return Ok(val);
+        }
+
+        if !timeout_is_zero && left_ns < 0 {
+            // Unlike the C version, we immediately return.
+            // We have just called `op()` so we don't need to call it again.
+            return Err(ETIMEDOUT);
+        }
+
+        if !delay_delta.is_zero() {
+            udelay(delay_delta);
+            if !timeout_is_zero {
+                left_ns -= delay_ns;
+            }
+        }
+
+        cpu_relax();
+        if !timeout_is_zero {
+            left_ns -= 1;
+        }
+    }
+}
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] rust: Add read_poll_timeout functions
  2025-08-17  4:47 ` [PATCH v2 2/2] rust: Add read_poll_timeout functions FUJITA Tomonori
  2025-08-19  0:49   ` Alexandre Courbot
  2025-08-19 18:30   ` Daniel Almeida
@ 2025-08-20 10:02   ` Alice Ryhl
  2 siblings, 0 replies; 9+ messages in thread
From: Alice Ryhl @ 2025-08-20 10:02 UTC (permalink / raw)
  To: FUJITA Tomonori
  Cc: a.hindborg, alex.gaynor, ojeda, anna-maria, bjorn3_gh, boqun.feng,
	dakr, frederic, gary, jstultz, linux-kernel, lossin, lyude,
	rust-for-linux, sboyd, tglx, tmgross, acourbot, daniel.almeida,
	Fiona Behrens

On Sun, Aug 17, 2025 at 01:47:23PM +0900, FUJITA Tomonori wrote:
> Add read_poll_timeout function which poll periodically until a
> condition is met or a timeout is reached.
> 
> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
> macro and a simple wrapper for Rust doesn't work. So this implements
> the same functionality in Rust.
> 
> The C version uses usleep_range() while the Rust version uses
> fsleep(), which uses the best sleep method so it works with spans that
> usleep_range() doesn't work nicely with.
> 
> The sleep_before_read argument isn't supported since there is no user
> for now. It's rarely used in the C version.
> 
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Reviewed-by: Fiona Behrens <me@kloenk.dev>
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/2] rust: Add read_poll_timeout functions
  2025-08-19  0:49   ` Alexandre Courbot
@ 2025-08-20 11:11     ` FUJITA Tomonori
  0 siblings, 0 replies; 9+ messages in thread
From: FUJITA Tomonori @ 2025-08-20 11:11 UTC (permalink / raw)
  To: acourbot
  Cc: fujita.tomonori, a.hindborg, alex.gaynor, ojeda, aliceryhl,
	anna-maria, bjorn3_gh, boqun.feng, dakr, frederic, gary, jstultz,
	linux-kernel, lossin, lyude, rust-for-linux, sboyd, tglx, tmgross,
	daniel.almeida, me

On Tue, 19 Aug 2025 09:49:38 +0900
"Alexandre Courbot" <acourbot@nvidia.com> wrote:

> On Sun Aug 17, 2025 at 1:47 PM JST, FUJITA Tomonori wrote:
>> Add read_poll_timeout function which poll periodically until a
>> condition is met or a timeout is reached.
>>
>> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
>> macro and a simple wrapper for Rust doesn't work. So this implements
>> the same functionality in Rust.
>>
>> The C version uses usleep_range() while the Rust version uses
>> fsleep(), which uses the best sleep method so it works with spans that
>> usleep_range() doesn't work nicely with.
>>
>> The sleep_before_read argument isn't supported since there is no user
>> for now. It's rarely used in the C version.
>>
>> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
>> Reviewed-by: Fiona Behrens <me@kloenk.dev>
>> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Tested this with nova-core, and it seems to work fine!
> 
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
> Tested-by: Alexandre Courbot <acourbot@nvidia.com>

Thanks!

> Just one last comment about the documentation below.
> 
> <snip>
>> +/// Polls periodically until a condition is met or a timeout is reached.
>> +///
>> +/// The function repeatedly executes the given operation `op` closure and
>> +/// checks its result using the condition closure `cond`.
>> +///
>> +/// If `cond` returns `true`, the function returns successfully with the result of `op`.
>> +/// Otherwise, it waits for a duration specified by `sleep_delta`
>> +/// before executing `op` again.
>> +///
>> +/// This process continues until either `cond` returns `true` or the timeout,
>> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
> 
> For precision: "This process continues until either `op` returns and
> error, `cond` returns `true`, or the timeout specified by
> `timeout_delta` is reached."

Indeed, `op` should be mentioned here.

I think it should be "op returns an error", not "op returns and error",
right?

There are three alternatives, so I’d remove "either". The sentence
would be:

This process continues until either `op` returns an error, `cond`
returns `true`, or the timeout specified by `timeout_delta` is
reached.

I just realized that "If `timeout_delta` is `None`," comment is outdated. I'll update.


>> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
>> +///
>> +/// This function can only be used in a nonatomic context.
> 
> Here I'd add an errors section:
> 
> # Errors
> 
> If `op` returns an error, then that error is returned directly.
> 
> If the timeout specified by `timeout_delta` is reached, then
> `Err(ETIMEDOUT)` is returned.

Thanks, looks nice. I'll add it.


>> +    loop {
>> +        let val = op()?;
>> +        if cond(&val) {
>> +            // Unlike the C version, we immediately return.
>> +            // We know the condition is met so we don't need to check again.
> 
> nit: this comment looks superfluous to me, this is a different
> implementation from the C version anyway.

A previous review mentioned that, since this function shares the same
name as the C version, it's good to make the different behavior
explicit. It may be a bit redundant, but I'd prefer to keep it.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2025-08-20 11:11 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-17  4:47 [PATCH v2 0/2] rust: Add read_poll_timeout FUJITA Tomonori
2025-08-17  4:47 ` [PATCH v2 1/2] rust: Add cpu_relax() helper FUJITA Tomonori
2025-08-19 18:32   ` Daniel Almeida
2025-08-17  4:47 ` [PATCH v2 2/2] rust: Add read_poll_timeout functions FUJITA Tomonori
2025-08-19  0:49   ` Alexandre Courbot
2025-08-20 11:11     ` FUJITA Tomonori
2025-08-19 18:30   ` Daniel Almeida
2025-08-20  6:45     ` FUJITA Tomonori
2025-08-20 10:02   ` Alice Ryhl

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).