Add read_poll_timeout functions which poll periodically until a
condition is met or a timeout is reached.
The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
macro and a simple wrapper for Rust doesn't work. So this implements
the same functionality in Rust.
The C version uses usleep_range() while the Rust version uses
fsleep(), which uses the best sleep method so it works with spans that
usleep_range() doesn't work nicely with.
The sleep_before_read argument isn't supported since there is no user
for now. It's rarely used in the C version.
read_poll_timeout() can only be used in a nonatomic context. This
requirement is not checked by these abstractions, but it is intended
that klint [1] or a similar tool will be used to check it in the
future.
Link: https://rust-for-linux.com/klint [1]
Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
rust/helpers/helpers.c | 1 +
rust/helpers/kernel.c | 18 +++++++
rust/kernel/cpu.rs | 13 +++++
rust/kernel/error.rs | 1 +
rust/kernel/io.rs | 2 +
rust/kernel/io/poll.rs | 120 +++++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +
7 files changed, 156 insertions(+)
create mode 100644 rust/helpers/kernel.c
create mode 100644 rust/kernel/cpu.rs
create mode 100644 rust/kernel/io/poll.rs
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 9565485a1a54..16d256897ccb 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -14,6 +14,7 @@
#include "cred.c"
#include "device.c"
#include "err.c"
+#include "kernel.c"
#include "fs.c"
#include "io.c"
#include "jump_label.c"
diff --git a/rust/helpers/kernel.c b/rust/helpers/kernel.c
new file mode 100644
index 000000000000..f04c04d4cc4f
--- /dev/null
+++ b/rust/helpers/kernel.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/kernel.h>
+
+void rust_helper_cpu_relax(void)
+{
+ cpu_relax();
+}
+
+void rust_helper___might_sleep_precision(const char *file, int len, int line)
+{
+ __might_sleep_precision(file, len, line);
+}
+
+void rust_helper_might_resched(void)
+{
+ might_resched();
+}
diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs
new file mode 100644
index 000000000000..eeeff4be84fa
--- /dev/null
+++ b/rust/kernel/cpu.rs
@@ -0,0 +1,13 @@
+// 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.
+pub fn cpu_relax() {
+ // SAFETY: FFI call.
+ unsafe { bindings::cpu_relax() }
+}
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index f6ecf09cb65f..8858eb13b3df 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -64,6 +64,7 @@ macro_rules! declare_err {
declare_err!(EPIPE, "Broken pipe.");
declare_err!(EDOM, "Math argument out of domain of func.");
declare_err!(ERANGE, "Math result not representable.");
+ declare_err!(ETIMEDOUT, "Connection timed out.");
declare_err!(ERESTARTSYS, "Restart the system call.");
declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
declare_err!(ERESTARTNOHAND, "Restart if no handler.");
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
index d4a73e52e3ee..be63742f517b 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -7,6 +7,8 @@
use crate::error::{code::EINVAL, Result};
use crate::{bindings, build_assert};
+pub mod poll;
+
/// Raw representation of an MMIO region.
///
/// By itself, the existence of an instance of this structure does not provide any guarantees that
diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
new file mode 100644
index 000000000000..5977b2082cc6
--- /dev/null
+++ b/rust/kernel/io/poll.rs
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! IO polling.
+//!
+//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
+
+use crate::{
+ cpu::cpu_relax,
+ error::{code::*, Result},
+ time::{delay::fsleep, Delta, Instant},
+};
+
+/// 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.
+///
+/// # Examples
+///
+/// ```rust,ignore
+/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
+/// // The `op` closure reads the value of a specific status register.
+/// let op = || -> Result<u16> { dev.read_ready_register() };
+///
+/// // The `cond` closure takes a reference to the value returned by `op`
+/// // and checks whether the hardware is ready.
+/// let cond = |val: &u16| *val == HW_READY;
+///
+/// match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
+/// Ok(_) => {
+/// // The hardware is ready. The returned value of the `op`` closure isn't used.
+/// Ok(())
+/// }
+/// Err(e) => Err(e),
+/// }
+/// }
+/// ```
+///
+/// ```rust
+/// use kernel::io::poll::read_poll_timeout;
+/// use kernel::time::Delta;
+/// use kernel::sync::{SpinLock, new_spinlock};
+///
+/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
+/// let g = lock.lock();
+/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
+/// drop(g);
+///
+/// # Ok::<(), Error>(())
+/// ```
+#[track_caller]
+pub fn read_poll_timeout<Op, Cond, T>(
+ mut op: Op,
+ mut cond: Cond,
+ sleep_delta: Delta,
+ timeout_delta: Option<Delta>,
+) -> Result<T>
+where
+ Op: FnMut() -> Result<T>,
+ Cond: FnMut(&T) -> bool,
+{
+ let start = Instant::now();
+ let sleep = !sleep_delta.is_zero();
+
+ if sleep {
+ might_sleep();
+ }
+
+ loop {
+ let val = op()?;
+ if cond(&val) {
+ // Unlike the C version, we immediately return.
+ // We know the condition is met so we don't need to check again.
+ return Ok(val);
+ }
+ if let Some(timeout_delta) = timeout_delta {
+ if start.elapsed() > timeout_delta {
+ // Unlike the C version, we immediately return.
+ // We have just called `op()` so we don't need to call it again.
+ return Err(ETIMEDOUT);
+ }
+ }
+ if sleep {
+ fsleep(sleep_delta);
+ }
+ // fsleep() could be busy-wait loop so we always call cpu_relax().
+ cpu_relax();
+ }
+}
+
+/// Annotation for functions that can sleep.
+///
+/// Equivalent to the C side [`might_sleep()`], this function serves as
+/// a debugging aid and a potential scheduling point.
+///
+/// This function can only be used in a nonatomic context.
+#[track_caller]
+fn might_sleep() {
+ #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
+ {
+ let loc = core::panic::Location::caller();
+ // SAFETY: FFI call.
+ unsafe {
+ crate::bindings::__might_sleep_precision(
+ loc.file().as_ptr().cast(),
+ loc.file().len() as i32,
+ loc.line() as i32,
+ )
+ }
+ }
+
+ // SAFETY: FFI call.
+ unsafe { crate::bindings::might_resched() }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0911..415c500212dd 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -40,6 +40,7 @@
pub mod block;
#[doc(hidden)]
pub mod build_assert;
+pub mod cpu;
pub mod cred;
pub mod device;
pub mod device_id;
--
2.43.0On Thu Feb 20, 2025 at 8:06 AM CET, FUJITA Tomonori wrote:
> diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs
> new file mode 100644
> index 000000000000..eeeff4be84fa
> --- /dev/null
> +++ b/rust/kernel/cpu.rs
> @@ -0,0 +1,13 @@
> +// 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.
> +pub fn cpu_relax() {
> + // SAFETY: FFI call.
> + unsafe { bindings::cpu_relax() }
> +}
Please split this out in a separate patch.
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index f6ecf09cb65f..8858eb13b3df 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -64,6 +64,7 @@ macro_rules! declare_err {
> declare_err!(EPIPE, "Broken pipe.");
> declare_err!(EDOM, "Math argument out of domain of func.");
> declare_err!(ERANGE, "Math result not representable.");
> + declare_err!(ETIMEDOUT, "Connection timed out.");
> declare_err!(ERESTARTSYS, "Restart the system call.");
> declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
> declare_err!(ERESTARTNOHAND, "Restart if no handler.");
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index d4a73e52e3ee..be63742f517b 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -7,6 +7,8 @@
> use crate::error::{code::EINVAL, Result};
> use crate::{bindings, build_assert};
>
> +pub mod poll;
> +
> /// Raw representation of an MMIO region.
> ///
> /// By itself, the existence of an instance of this structure does not provide any guarantees that
> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
> new file mode 100644
> index 000000000000..5977b2082cc6
> --- /dev/null
> +++ b/rust/kernel/io/poll.rs
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! IO polling.
> +//!
> +//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
> +
> +use crate::{
> + cpu::cpu_relax,
> + error::{code::*, Result},
> + time::{delay::fsleep, Delta, Instant},
> +};
> +
> +/// 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`.
I'd add an empty line here,
> +/// 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.
and here.
> +/// 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
> +///
> +/// ```rust,ignore
Why ignore? This should be possible to compile test.
> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
I think the parameter here can just be `&Io<SIZE>`.
> +/// // The `op` closure reads the value of a specific status register.
> +/// let op = || -> Result<u16> { dev.read_ready_register() };
> +///
> +/// // The `cond` closure takes a reference to the value returned by `op`
> +/// // and checks whether the hardware is ready.
> +/// let cond = |val: &u16| *val == HW_READY;
> +///
> +/// match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
> +/// Ok(_) => {
> +/// // The hardware is ready. The returned value of the `op`` closure isn't used.
> +/// Ok(())
> +/// }
> +/// Err(e) => Err(e),
> +/// }
> +/// }
> +/// ```
> +///
> +/// ```rust
> +/// use kernel::io::poll::read_poll_timeout;
> +/// use kernel::time::Delta;
> +/// use kernel::sync::{SpinLock, new_spinlock};
> +///
> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
> +/// let g = lock.lock();
> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
> +/// drop(g);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +#[track_caller]
> +pub fn read_poll_timeout<Op, Cond, T>(
> + mut op: Op,
> + mut cond: Cond,
> + sleep_delta: Delta,
> + timeout_delta: Option<Delta>,
> +) -> Result<T>
> +where
> + Op: FnMut() -> Result<T>,
> + Cond: FnMut(&T) -> bool,
> +{
> + let start = Instant::now();
> + let sleep = !sleep_delta.is_zero();
> +
> + if sleep {
> + might_sleep();
> + }
I think a conditional might_sleep() is not great.
I also think we can catch this at compile time, if we add two different variants
of read_poll_timeout() instead and be explicit about it. We could get Klint to
catch such issues for us at compile time.
> +
> + loop {
> + let val = op()?;
> + if cond(&val) {
> + // Unlike the C version, we immediately return.
> + // We know the condition is met so we don't need to check again.
> + return Ok(val);
> + }
> + if let Some(timeout_delta) = timeout_delta {
> + if start.elapsed() > timeout_delta {
> + // Unlike the C version, we immediately return.
> + // We have just called `op()` so we don't need to call it again.
> + return Err(ETIMEDOUT);
> + }
> + }
> + if sleep {
> + fsleep(sleep_delta);
> + }
> + // fsleep() could be busy-wait loop so we always call cpu_relax().
> + cpu_relax();
> + }
> +}
On Mon, 28 Jul 2025 15:13:45 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:
> On Thu Feb 20, 2025 at 8:06 AM CET, FUJITA Tomonori wrote:
>> diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs
>> new file mode 100644
>> index 000000000000..eeeff4be84fa
>> --- /dev/null
>> +++ b/rust/kernel/cpu.rs
>> @@ -0,0 +1,13 @@
>> +// 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.
>> +pub fn cpu_relax() {
>> + // SAFETY: FFI call.
>> + unsafe { bindings::cpu_relax() }
>> +}
>
> Please split this out in a separate patch.
I will.
>> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
>> index f6ecf09cb65f..8858eb13b3df 100644
>> --- a/rust/kernel/error.rs
>> +++ b/rust/kernel/error.rs
>> @@ -64,6 +64,7 @@ macro_rules! declare_err {
>> declare_err!(EPIPE, "Broken pipe.");
>> declare_err!(EDOM, "Math argument out of domain of func.");
>> declare_err!(ERANGE, "Math result not representable.");
>> + declare_err!(ETIMEDOUT, "Connection timed out.");
>> declare_err!(ERESTARTSYS, "Restart the system call.");
>> declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
>> declare_err!(ERESTARTNOHAND, "Restart if no handler.");
>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
>> index d4a73e52e3ee..be63742f517b 100644
>> --- a/rust/kernel/io.rs
>> +++ b/rust/kernel/io.rs
>> @@ -7,6 +7,8 @@
>> use crate::error::{code::EINVAL, Result};
>> use crate::{bindings, build_assert};
>>
>> +pub mod poll;
>> +
>> /// Raw representation of an MMIO region.
>> ///
>> /// By itself, the existence of an instance of this structure does not provide any guarantees that
>> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
>> new file mode 100644
>> index 000000000000..5977b2082cc6
>> --- /dev/null
>> +++ b/rust/kernel/io/poll.rs
>> @@ -0,0 +1,120 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! IO polling.
>> +//!
>> +//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
>> +
>> +use crate::{
>> + cpu::cpu_relax,
>> + error::{code::*, Result},
>> + time::{delay::fsleep, Delta, Instant},
>> +};
>> +
>> +/// 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`.
>
> I'd add an empty line here,
>
>> +/// 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.
>
> and here.
Sure, I'll add at both places.
>> +/// 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
>> +///
>> +/// ```rust,ignore
>
> Why ignore? This should be possible to compile test.
https://lore.kernel.org/rust-for-linux/CEF87294-8580-4C84-BEA3-EB72E63ED7DF@collabora.com/
>> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
>
> I think the parameter here can just be `&Io<SIZE>`.
>
>> +/// // The `op` closure reads the value of a specific status register.
>> +/// let op = || -> Result<u16> { dev.read_ready_register() };
>> +///
>> +/// // The `cond` closure takes a reference to the value returned by `op`
>> +/// // and checks whether the hardware is ready.
>> +/// let cond = |val: &u16| *val == HW_READY;
>> +///
>> +/// match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
>> +/// Ok(_) => {
>> +/// // The hardware is ready. The returned value of the `op`` closure isn't used.
>> +/// Ok(())
>> +/// }
>> +/// Err(e) => Err(e),
>> +/// }
>> +/// }
>> +/// ```
>> +///
>> +/// ```rust
>> +/// use kernel::io::poll::read_poll_timeout;
>> +/// use kernel::time::Delta;
>> +/// use kernel::sync::{SpinLock, new_spinlock};
>> +///
>> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
>> +/// let g = lock.lock();
>> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
>> +/// drop(g);
>> +///
>> +/// # Ok::<(), Error>(())
>> +/// ```
>> +#[track_caller]
>> +pub fn read_poll_timeout<Op, Cond, T>(
>> + mut op: Op,
>> + mut cond: Cond,
>> + sleep_delta: Delta,
>> + timeout_delta: Option<Delta>,
>> +) -> Result<T>
>> +where
>> + Op: FnMut() -> Result<T>,
>> + Cond: FnMut(&T) -> bool,
>> +{
>> + let start = Instant::now();
>> + let sleep = !sleep_delta.is_zero();
>> +
>> + if sleep {
>> + might_sleep();
>> + }
>
> I think a conditional might_sleep() is not great.
>
> I also think we can catch this at compile time, if we add two different variants
> of read_poll_timeout() instead and be explicit about it. We could get Klint to
> catch such issues for us at compile time.
Your point is that functions which cannot be used in atomic context
should be clearly separated into different ones. Then Klint might be
able to detect such usage at compile time, right?
How about dropping the conditional might_sleep() and making
read_poll_timeout return an error with zero sleep_delta?
Drivers which need busy-loop (without even udelay) can
call read_poll_timeout_atomic() with zero delay.
On Sat Aug 2, 2025 at 3:42 AM CEST, FUJITA Tomonori wrote:
> On Mon, 28 Jul 2025 15:13:45 +0200
> "Danilo Krummrich" <dakr@kernel.org> wrote:
>> On Thu Feb 20, 2025 at 8:06 AM CET, FUJITA Tomonori wrote:
>>> +/// 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
>>> +///
>>> +/// ```rust,ignore
>>
>> Why ignore? This should be possible to compile test.
>
> https://lore.kernel.org/rust-for-linux/CEF87294-8580-4C84-BEA3-EB72E63ED7DF@collabora.com/
I disagree with that. 'ignore' should only be used if we can't make it compile.
In this case we can make it compile, we just can't run it, since there's no real
HW underneath that we can read registers from.
An example that isn't compiled will eventually be forgotten to be updated when
things are changed.
>>> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
>>
>> I think the parameter here can just be `&Io<SIZE>`.
>>
>>> +/// // The `op` closure reads the value of a specific status register.
>>> +/// let op = || -> Result<u16> { dev.read_ready_register() };
>>> +///
>>> +/// // The `cond` closure takes a reference to the value returned by `op`
>>> +/// // and checks whether the hardware is ready.
>>> +/// let cond = |val: &u16| *val == HW_READY;
>>> +///
>>> +/// match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
>>> +/// Ok(_) => {
>>> +/// // The hardware is ready. The returned value of the `op`` closure isn't used.
>>> +/// Ok(())
>>> +/// }
>>> +/// Err(e) => Err(e),
>>> +/// }
>>> +/// }
>>> +/// ```
>>> +///
>>> +/// ```rust
>>> +/// use kernel::io::poll::read_poll_timeout;
>>> +/// use kernel::time::Delta;
>>> +/// use kernel::sync::{SpinLock, new_spinlock};
>>> +///
>>> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
>>> +/// let g = lock.lock();
>>> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
>>> +/// drop(g);
>>> +///
>>> +/// # Ok::<(), Error>(())
>>> +/// ```
>>> +#[track_caller]
>>> +pub fn read_poll_timeout<Op, Cond, T>(
>>> + mut op: Op,
>>> + mut cond: Cond,
>>> + sleep_delta: Delta,
>>> + timeout_delta: Option<Delta>,
>>> +) -> Result<T>
>>> +where
>>> + Op: FnMut() -> Result<T>,
>>> + Cond: FnMut(&T) -> bool,
>>> +{
>>> + let start = Instant::now();
>>> + let sleep = !sleep_delta.is_zero();
>>> +
>>> + if sleep {
>>> + might_sleep();
>>> + }
>>
>> I think a conditional might_sleep() is not great.
>>
>> I also think we can catch this at compile time, if we add two different variants
>> of read_poll_timeout() instead and be explicit about it. We could get Klint to
>> catch such issues for us at compile time.
>
> Your point is that functions which cannot be used in atomic context
> should be clearly separated into different ones. Then Klint might be
> able to detect such usage at compile time, right?
>
> How about dropping the conditional might_sleep() and making
> read_poll_timeout return an error with zero sleep_delta?
Yes, let's always call might_sleep(), the conditional is very error prone. We
want to see the warning splat whenever someone calls read_poll_timeout() from
atomic context.
Yes, with zero sleep_delta it could be called from atomic context technically,
but if drivers rely on this and wrap this into higher level helpers it's very
easy to miss a subtle case and end up with non-zero sleep_delta within an atomic
context for some rare condition that then is hard to debug.
As for making read_poll_timeout() return a error with zero sleep_delta, I don't
see a reason to do that. If a driver wraps read_poll_timeout() in its own
function that sometimes sleeps and sometimes does not, based on some condition,
but is never called from atomic context, that's fine.
> Drivers which need busy-loop (without even udelay) can
> call read_poll_timeout_atomic() with zero delay.
It's not the zero delay or zero sleep_delta that makes the difference it's
really the fact the one can be called from atomic context and one can't be.
On Sat, 02 Aug 2025 13:06:04 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:
> On Sat Aug 2, 2025 at 3:42 AM CEST, FUJITA Tomonori wrote:
>> On Mon, 28 Jul 2025 15:13:45 +0200
>> "Danilo Krummrich" <dakr@kernel.org> wrote:
>>> On Thu Feb 20, 2025 at 8:06 AM CET, FUJITA Tomonori wrote:
>>>> +/// 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
>>>> +///
>>>> +/// ```rust,ignore
>>>
>>> Why ignore? This should be possible to compile test.
>>
>> https://lore.kernel.org/rust-for-linux/CEF87294-8580-4C84-BEA3-EB72E63ED7DF@collabora.com/
>
> I disagree with that. 'ignore' should only be used if we can't make it compile.
>
> In this case we can make it compile, we just can't run it, since there's no real
> HW underneath that we can read registers from.
>
> An example that isn't compiled will eventually be forgotten to be updated when
> things are changed.
I also prefer the example that can be compiled however I can't think
of a compilable example that is similar to actual use cases (for
example, waiting for some hardware condition). Do you have any ideas?
>>>> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
>>>
>>> I think the parameter here can just be `&Io<SIZE>`.
>>>
>>>> +/// // The `op` closure reads the value of a specific status register.
>>>> +/// let op = || -> Result<u16> { dev.read_ready_register() };
>>>> +///
>>>> +/// // The `cond` closure takes a reference to the value returned by `op`
>>>> +/// // and checks whether the hardware is ready.
>>>> +/// let cond = |val: &u16| *val == HW_READY;
>>>> +///
>>>> +/// match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
>>>> +/// Ok(_) => {
>>>> +/// // The hardware is ready. The returned value of the `op`` closure isn't used.
>>>> +/// Ok(())
>>>> +/// }
>>>> +/// Err(e) => Err(e),
>>>> +/// }
>>>> +/// }
>>>> +/// ```
>>>> +///
>>>> +/// ```rust
>>>> +/// use kernel::io::poll::read_poll_timeout;
>>>> +/// use kernel::time::Delta;
>>>> +/// use kernel::sync::{SpinLock, new_spinlock};
>>>> +///
>>>> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
>>>> +/// let g = lock.lock();
>>>> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
>>>> +/// drop(g);
>>>> +///
>>>> +/// # Ok::<(), Error>(())
>>>> +/// ```
>>>> +#[track_caller]
>>>> +pub fn read_poll_timeout<Op, Cond, T>(
>>>> + mut op: Op,
>>>> + mut cond: Cond,
>>>> + sleep_delta: Delta,
>>>> + timeout_delta: Option<Delta>,
>>>> +) -> Result<T>
>>>> +where
>>>> + Op: FnMut() -> Result<T>,
>>>> + Cond: FnMut(&T) -> bool,
>>>> +{
>>>> + let start = Instant::now();
>>>> + let sleep = !sleep_delta.is_zero();
>>>> +
>>>> + if sleep {
>>>> + might_sleep();
>>>> + }
>>>
>>> I think a conditional might_sleep() is not great.
>>>
>>> I also think we can catch this at compile time, if we add two different variants
>>> of read_poll_timeout() instead and be explicit about it. We could get Klint to
>>> catch such issues for us at compile time.
>>
>> Your point is that functions which cannot be used in atomic context
>> should be clearly separated into different ones. Then Klint might be
>> able to detect such usage at compile time, right?
>>
>> How about dropping the conditional might_sleep() and making
>> read_poll_timeout return an error with zero sleep_delta?
>
> Yes, let's always call might_sleep(), the conditional is very error prone. We
> want to see the warning splat whenever someone calls read_poll_timeout() from
> atomic context.
>
> Yes, with zero sleep_delta it could be called from atomic context technically,
> but if drivers rely on this and wrap this into higher level helpers it's very
> easy to miss a subtle case and end up with non-zero sleep_delta within an atomic
> context for some rare condition that then is hard to debug.
>
> As for making read_poll_timeout() return a error with zero sleep_delta, I don't
> see a reason to do that. If a driver wraps read_poll_timeout() in its own
> function that sometimes sleeps and sometimes does not, based on some condition,
> but is never called from atomic context, that's fine.
Ok, let's always call might_sleep, even when there's no possibility of
sleeping.
>> Drivers which need busy-loop (without even udelay) can
>> call read_poll_timeout_atomic() with zero delay.
>
> It's not the zero delay or zero sleep_delta that makes the difference it's
> really the fact the one can be called from atomic context and one can't be.
That's what I meant.
Since calling read_poll_timeout_atomic() with zero delay can achieve
the same thing as calling read_poll_timeout with zero sleep delta, I
thought the zero sleep delta in read_poll_timeout is unncessary.
But as in the use case you mentioned above, a driver could use the
sleep delta that is not const and calls the function with both
non-zero and zero delta values so let's keep zero sleep delta support.
On Tue Aug 5, 2025 at 3:37 PM CEST, FUJITA Tomonori wrote:
> On Sat, 02 Aug 2025 13:06:04 +0200
> "Danilo Krummrich" <dakr@kernel.org> wrote:
>
>> On Sat Aug 2, 2025 at 3:42 AM CEST, FUJITA Tomonori wrote:
>>> On Mon, 28 Jul 2025 15:13:45 +0200
>>> "Danilo Krummrich" <dakr@kernel.org> wrote:
>>>> On Thu Feb 20, 2025 at 8:06 AM CET, FUJITA Tomonori wrote:
>>>>> +/// 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
>>>>> +///
>>>>> +/// ```rust,ignore
>>>>
>>>> Why ignore? This should be possible to compile test.
>>>
>>> https://lore.kernel.org/rust-for-linux/CEF87294-8580-4C84-BEA3-EB72E63ED7DF@collabora.com/
>>
>> I disagree with that. 'ignore' should only be used if we can't make it compile.
>>
>> In this case we can make it compile, we just can't run it, since there's no real
>> HW underneath that we can read registers from.
>>
>> An example that isn't compiled will eventually be forgotten to be updated when
>> things are changed.
>
> I also prefer the example that can be compiled however I can't think
> of a compilable example that is similar to actual use cases (for
> example, waiting for some hardware condition). Do you have any ideas?
With my suggestion below, it should be compilable.
When you just take a &Io<SIZE> as argument in wait_for_hardware() you can call
io.read(). Then define HW_READY as some random value and it should compile.
>>>>> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
>>>>
>>>> I think the parameter here can just be `&Io<SIZE>`.
>>>>
>>>>> +/// // The `op` closure reads the value of a specific status register.
>>>>> +/// let op = || -> Result<u16> { dev.read_ready_register() };
>>>>> +///
>>>>> +/// // The `cond` closure takes a reference to the value returned by `op`
>>>>> +/// // and checks whether the hardware is ready.
>>>>> +/// let cond = |val: &u16| *val == HW_READY;
>>>>> +///
>>>>> +/// match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
>>>>> +/// Ok(_) => {
>>>>> +/// // The hardware is ready. The returned value of the `op`` closure isn't used.
>>>>> +/// Ok(())
>>>>> +/// }
>>>>> +/// Err(e) => Err(e),
>>>>> +/// }
>>>>> +/// }
>>>>> +/// ```
> I also prefer the example that can be compiled however I can't think > of a compilable example that is similar to actual use cases (for > example, waiting for some hardware condition). Do you have any ideas? Does it have to be successfully runnable? Just read from address 0x42. If it actually gets run, it will trigger an Opps, but it should be obvious why. Andrew
On Tue Aug 5, 2025 at 3:53 PM CEST, Andrew Lunn wrote: >> I also prefer the example that can be compiled however I can't think >> of a compilable example that is similar to actual use cases (for >> example, waiting for some hardware condition). Do you have any ideas? > > Does it have to be successfully runnable? Just read from address > 0x42. If it actually gets run, it will trigger an Opps, but it should > be obvious why. No, we have all three options. (1) Don't do anything with the code. (2) Compile it, but don't run it. (3) Compile it and run it as kunit test. In this case we want to compile it, but not actually run it. - Danilo
> On 2 Aug 2025, at 08:06, Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Sat Aug 2, 2025 at 3:42 AM CEST, FUJITA Tomonori wrote:
>> On Mon, 28 Jul 2025 15:13:45 +0200
>> "Danilo Krummrich" <dakr@kernel.org> wrote:
>>> On Thu Feb 20, 2025 at 8:06 AM CET, FUJITA Tomonori wrote:
>>>> +/// 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
>>>> +///
>>>> +/// ```rust,ignore
>>>
>>> Why ignore? This should be possible to compile test.
>>
>> https://lore.kernel.org/rust-for-linux/CEF87294-8580-4C84-BEA3-EB72E63ED7DF@collabora.com/
>
> I disagree with that. 'ignore' should only be used if we can't make it compile.
>
> In this case we can make it compile, we just can't run it, since there's no real
> HW underneath that we can read registers from.
>
> An example that isn't compiled will eventually be forgotten to be updated when
> things are changed.
>
>>>> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
>>>
>>> I think the parameter here can just be `&Io<SIZE>`.
>>>
>>>> +/// // The `op` closure reads the value of a specific status register.
>>>> +/// let op = || -> Result<u16> { dev.read_ready_register() };
>>>> +///
>>>> +/// // The `cond` closure takes a reference to the value returned by `op`
>>>> +/// // and checks whether the hardware is ready.
>>>> +/// let cond = |val: &u16| *val == HW_READY;
>>>> +///
>>>> +/// match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
>>>> +/// Ok(_) => {
>>>> +/// // The hardware is ready. The returned value of the `op`` closure isn't used.
>>>> +/// Ok(())
>>>> +/// }
>>>> +/// Err(e) => Err(e),
>>>> +/// }
>>>> +/// }
>>>> +/// ```
>>>> +///
>>>> +/// ```rust
>>>> +/// use kernel::io::poll::read_poll_timeout;
>>>> +/// use kernel::time::Delta;
>>>> +/// use kernel::sync::{SpinLock, new_spinlock};
>>>> +///
>>>> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
>>>> +/// let g = lock.lock();
>>>> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
>>>> +/// drop(g);
>>>> +///
>>>> +/// # Ok::<(), Error>(())
>>>> +/// ```
>>>> +#[track_caller]
>>>> +pub fn read_poll_timeout<Op, Cond, T>(
>>>> + mut op: Op,
>>>> + mut cond: Cond,
>>>> + sleep_delta: Delta,
>>>> + timeout_delta: Option<Delta>,
>>>> +) -> Result<T>
>>>> +where
>>>> + Op: FnMut() -> Result<T>,
>>>> + Cond: FnMut(&T) -> bool,
>>>> +{
>>>> + let start = Instant::now();
>>>> + let sleep = !sleep_delta.is_zero();
>>>> +
>>>> + if sleep {
>>>> + might_sleep();
>>>> + }
>>>
>>> I think a conditional might_sleep() is not great.
>>>
>>> I also think we can catch this at compile time, if we add two different variants
>>> of read_poll_timeout() instead and be explicit about it. We could get Klint to
>>> catch such issues for us at compile time.
>>
>> Your point is that functions which cannot be used in atomic context
>> should be clearly separated into different ones. Then Klint might be
>> able to detect such usage at compile time, right?
>>
>> How about dropping the conditional might_sleep() and making
>> read_poll_timeout return an error with zero sleep_delta?
>
> Yes, let's always call might_sleep(), the conditional is very error prone. We
> want to see the warning splat whenever someone calls read_poll_timeout() from
> atomic context.
>
> Yes, with zero sleep_delta it could be called from atomic context technically,
> but if drivers rely on this and wrap this into higher level helpers it's very
> easy to miss a subtle case and end up with non-zero sleep_delta within an atomic
> context for some rare condition that then is hard to debug.
>
> As for making read_poll_timeout() return a error with zero sleep_delta, I don't
> see a reason to do that. If a driver wraps read_poll_timeout() in its own
> function that sometimes sleeps and sometimes does not, based on some condition,
> but is never called from atomic context, that's fine.
>
>> Drivers which need busy-loop (without even udelay) can
>> call read_poll_timeout_atomic() with zero delay.
>
> It's not the zero delay or zero sleep_delta that makes the difference it's
> really the fact the one can be called from atomic context and one can't be.
Perhaps it’s worth it to clarify that in the docs for the future versions?
I feel like “sleep_delta == 0 -> this doesn’t sleep -> it’s fine to
use this in an atomic context” is a somewhat expected thought process.
Also, this will lead to the confusion I mentioned, i.e. “why do I need to
use the atomic version if I can pass 0 for sleep_delta?”
I mean, the added context in this thread explains it, but it’s probably
worth it to make sure that the docs also do.
Just my humble opinion.
— Daniel
On Tue Aug 5, 2025 at 4:01 PM CEST, Daniel Almeida wrote: > Perhaps it’s worth it to clarify that in the docs for the future versions? If you mean the documentation that is given out to users of the API I disagree. We should not explain the implmentation details or the reasons for them to users. Instead we should explain the constraints and what users can expect from the API and what's the best practice to use it. If you meant to add a comment to the implementation itself, that's fine of course.
Hi Fujita, > On 20 Feb 2025, at 04:06, FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > Add read_poll_timeout functions which poll periodically until a > condition is met or a timeout is reached. > > The C's read_poll_timeout (include/linux/iopoll.h) is a complicated > macro and a simple wrapper for Rust doesn't work. So this implements > the same functionality in Rust. > > The C version uses usleep_range() while the Rust version uses > fsleep(), which uses the best sleep method so it works with spans that > usleep_range() doesn't work nicely with. > > The sleep_before_read argument isn't supported since there is no user > for now. It's rarely used in the C version. > > read_poll_timeout() can only be used in a nonatomic context. This > requirement is not checked by these abstractions, but it is intended > that klint [1] or a similar tool will be used to check it in the > future. > > Link: https://rust-for-linux.com/klint [1] > Tested-by: Daniel Almeida <daniel.almeida@collabora.com> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> This appears to be the last version of this patch. Do you have any plans to keep working on it? Is there anything I can do to help? :) If you don’t have the time to work on this, I can pick it up for you. — Daniel
On Mon, 28 Jul 2025 09:44:39 -0300 Daniel Almeida <daniel.almeida@collabora.com> wrote: >> Add read_poll_timeout functions which poll periodically until a >> condition is met or a timeout is reached. >> >> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated >> macro and a simple wrapper for Rust doesn't work. So this implements >> the same functionality in Rust. >> >> The C version uses usleep_range() while the Rust version uses >> fsleep(), which uses the best sleep method so it works with spans that >> usleep_range() doesn't work nicely with. >> >> The sleep_before_read argument isn't supported since there is no user >> for now. It's rarely used in the C version. >> >> read_poll_timeout() can only be used in a nonatomic context. This >> requirement is not checked by these abstractions, but it is intended >> that klint [1] or a similar tool will be used to check it in the >> future. >> >> Link: https://rust-for-linux.com/klint [1] >> Tested-by: Daniel Almeida <daniel.almeida@collabora.com> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > > This appears to be the last version of this patch. Do you have any plans to > keep working on it? Is there anything I can do to help? :) > > If you don’t have the time to work on this, I can pick it up for you. All the dependencies for this patch (timer, fsleep, might_sleep, etc) are planned to be merged in 6.17-rc1, and I'll submit the updated version after the rc1 release.
On 7/28/25 2:52 PM, FUJITA Tomonori wrote: > All the dependencies for this patch (timer, fsleep, might_sleep, etc) > are planned to be merged in 6.17-rc1, and I'll submit the updated > version after the rc1 release. Can you please Cc Alex and me on this? Thanks, Danilo
On Mon, 28 Jul 2025 14:57:16 +0200 Danilo Krummrich <kernel@dakr.org> wrote: > On 7/28/25 2:52 PM, FUJITA Tomonori wrote: >> All the dependencies for this patch (timer, fsleep, might_sleep, etc) >> are planned to be merged in 6.17-rc1, and I'll submit the updated >> version after the rc1 release. > > Can you please Cc Alex and me on this? Sure. read_poll_timeout() works for drm drivers? read_poll_timeout_atomic() are necessary too?
> On 28 Jul 2025, at 10:08, FUJITA Tomonori <fujita.tomonori@gmail.com> wrote: > > On Mon, 28 Jul 2025 14:57:16 +0200 > Danilo Krummrich <kernel@dakr.org> wrote: > >> On 7/28/25 2:52 PM, FUJITA Tomonori wrote: >>> All the dependencies for this patch (timer, fsleep, might_sleep, etc) >>> are planned to be merged in 6.17-rc1, and I'll submit the updated >>> version after the rc1 release. >> >> Can you please Cc Alex and me on this? > > Sure. > > read_poll_timeout() works for drm drivers? read_poll_timeout_atomic() > are necessary too? Tyr needs read_poll_timeout_atomic() too, but I never really understood if that's not achievable by setting sleep_delta==0, which skips the call to fsleep(). — Daniel
On Mon Jul 28, 2025 at 4:13 PM CEST, Daniel Almeida wrote:
> Tyr needs read_poll_timeout_atomic() too, but I never really understood
> if that's not achievable by setting sleep_delta==0, which skips the
> call to fsleep().
read_poll_timeout_atomic() uses udelay() instead of fsleep(), where the latter
may sleep.
In [1] I explicitly asked for not having the
if sleep_delta != 0 {
might_sleep();
}
conditional.
It hides bugs and callers should be explicit about what they want. Either they
want something that actually may sleep or they want something that can be used
from atomic context.
Even better, in Rust we can use Klint to validate correct usage at compile time
with this separation.
[1] https://lore.kernel.org/lkml/DBNPR4KQZXY5.279JBMO315A12@kernel.org/
On Mon Jul 28, 2025 at 3:08 PM CEST, FUJITA Tomonori wrote: > On Mon, 28 Jul 2025 14:57:16 +0200 > Danilo Krummrich <kernel@dakr.org> wrote: > >> On 7/28/25 2:52 PM, FUJITA Tomonori wrote: >>> All the dependencies for this patch (timer, fsleep, might_sleep, etc) >>> are planned to be merged in 6.17-rc1, and I'll submit the updated >>> version after the rc1 release. >> >> Can you please Cc Alex and me on this? > > Sure. > > read_poll_timeout() works for drm drivers? read_poll_timeout_atomic() > are necessary too? Please see my other reply on this patch.
Hi Tomonori,
FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
> Add read_poll_timeout functions which poll periodically until a
> condition is met or a timeout is reached.
>
> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
> macro and a simple wrapper for Rust doesn't work. So this implements
> the same functionality in Rust.
>
> The C version uses usleep_range() while the Rust version uses
> fsleep(), which uses the best sleep method so it works with spans that
> usleep_range() doesn't work nicely with.
>
> The sleep_before_read argument isn't supported since there is no user
> for now. It's rarely used in the C version.
>
> read_poll_timeout() can only be used in a nonatomic context. This
> requirement is not checked by these abstractions, but it is intended
> that klint [1] or a similar tool will be used to check it in the
> future.
>
> Link: https://rust-for-linux.com/klint [1]
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
> rust/helpers/helpers.c | 1 +
> rust/helpers/kernel.c | 18 +++++++
> rust/kernel/cpu.rs | 13 +++++
> rust/kernel/error.rs | 1 +
> rust/kernel/io.rs | 2 +
> rust/kernel/io/poll.rs | 120 +++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 1 +
> 7 files changed, 156 insertions(+)
> create mode 100644 rust/helpers/kernel.c
> create mode 100644 rust/kernel/cpu.rs
> create mode 100644 rust/kernel/io/poll.rs
>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 9565485a1a54..16d256897ccb 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -14,6 +14,7 @@
> #include "cred.c"
> #include "device.c"
> #include "err.c"
> +#include "kernel.c"
> #include "fs.c"
> #include "io.c"
> #include "jump_label.c"
> diff --git a/rust/helpers/kernel.c b/rust/helpers/kernel.c
> new file mode 100644
> index 000000000000..f04c04d4cc4f
> --- /dev/null
> +++ b/rust/helpers/kernel.c
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/kernel.h>
> +
> +void rust_helper_cpu_relax(void)
> +{
> + cpu_relax();
> +}
> +
> +void rust_helper___might_sleep_precision(const char *file, int len, int line)
> +{
> + __might_sleep_precision(file, len, line);
> +}
> +
> +void rust_helper_might_resched(void)
> +{
> + might_resched();
> +}
> diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs
> new file mode 100644
> index 000000000000..eeeff4be84fa
> --- /dev/null
> +++ b/rust/kernel/cpu.rs
> @@ -0,0 +1,13 @@
> +// 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.
> +pub fn cpu_relax() {
> + // SAFETY: FFI call.
I don't think this safety comment is sufficient. There are two other
similar comments further down.
> + unsafe { bindings::cpu_relax() }
> +}
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index f6ecf09cb65f..8858eb13b3df 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -64,6 +64,7 @@ macro_rules! declare_err {
> declare_err!(EPIPE, "Broken pipe.");
> declare_err!(EDOM, "Math argument out of domain of func.");
> declare_err!(ERANGE, "Math result not representable.");
> + declare_err!(ETIMEDOUT, "Connection timed out.");
> declare_err!(ERESTARTSYS, "Restart the system call.");
> declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
> declare_err!(ERESTARTNOHAND, "Restart if no handler.");
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> index d4a73e52e3ee..be63742f517b 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -7,6 +7,8 @@
> use crate::error::{code::EINVAL, Result};
> use crate::{bindings, build_assert};
>
> +pub mod poll;
> +
> /// Raw representation of an MMIO region.
> ///
> /// By itself, the existence of an instance of this structure does not provide any guarantees that
> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
> new file mode 100644
> index 000000000000..5977b2082cc6
> --- /dev/null
> +++ b/rust/kernel/io/poll.rs
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! IO polling.
> +//!
> +//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
> +
> +use crate::{
> + cpu::cpu_relax,
> + error::{code::*, Result},
> + time::{delay::fsleep, Delta, Instant},
> +};
> +
> +/// 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.
> +///
> +/// # Examples
> +///
> +/// ```rust,ignore
> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
> +/// // The `op` closure reads the value of a specific status register.
> +/// let op = || -> Result<u16> { dev.read_ready_register() };
> +///
> +/// // The `cond` closure takes a reference to the value returned by `op`
> +/// // and checks whether the hardware is ready.
> +/// let cond = |val: &u16| *val == HW_READY;
> +///
> +/// match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
> +/// Ok(_) => {
> +/// // The hardware is ready. The returned value of the `op`` closure isn't used.
> +/// Ok(())
> +/// }
> +/// Err(e) => Err(e),
> +/// }
> +/// }
> +/// ```
> +///
> +/// ```rust
> +/// use kernel::io::poll::read_poll_timeout;
> +/// use kernel::time::Delta;
> +/// use kernel::sync::{SpinLock, new_spinlock};
> +///
> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
> +/// let g = lock.lock();
> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
> +/// drop(g);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
I am guessing this example is present to test the call to `might_sleep`.
Could you document the reason for the test. As an example, this code is
not really usable. `#[test]` was staged for 6.15, so perhaps move this
to a unit test instead?
The test throws this BUG, which is what I think is also your intention:
BUG: sleeping function called from invalid context at rust/doctests_kernel_generated.rs:3523
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 171, name: kunit_try_catch
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
1 lock held by kunit_try_catch/171:
#0: ffff8881003ce598 (rust/doctests_kernel_generated.rs:3521){+.+.}-{3:3}, at: rust_helper_spin_lock+0xd/0x10
CPU: 0 UID: 0 PID: 171 Comm: kunit_try_catch Tainted: G N 6.14.0-rc7+ #14
Tainted: [N]=TEST
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
Call Trace:
<TASK>
dump_stack_lvl+0x7b/0xa0
dump_stack+0x14/0x16
__might_resched_precision+0x22f/0x240
__might_sleep_precision+0x39/0x70
_RNvNtNtCs1cdwasc6FUb_6kernel2io4poll11might_sleep+0x19/0x20
rust_doctest_kernel_io_poll_rs_0+0xa5/0x1f0
kunit_try_run_case+0x73/0x150
? trace_hardirqs_on+0x5a/0x90
kunit_generic_run_threadfn_adapter+0x1a/0x30
kthread+0x219/0x230
? kunit_try_catch_run+0x230/0x230
? __do_trace_sched_kthread_stop_ret+0x50/0x50
ret_from_fork+0x35/0x40
? __do_trace_sched_kthread_stop_ret+0x50/0x50
ret_from_fork_asm+0x11/0x20
</TASK>
Kunit does not pick this up as a failure, but it _should_, and hopefully
it will soon (TM).
So, we should probably expect failure when we get that fixed. And
perhaps for now disable the test or add a TODO to change to expect fail
when we fix kunit.
Best regards,
Andreas Hindborg
Sorry, I somehow missed this email.
On Sat, 22 Mar 2025 17:02:31 +0100
Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> +/// Lower CPU power consumption or yield to a hyperthreaded twin processor.
>> +///
>> +/// It also happens to serve as a compiler barrier.
>> +pub fn cpu_relax() {
>> + // SAFETY: FFI call.
>
> I don't think this safety comment is sufficient. There are two other
> similar comments further down.
Updated the comment.
>> +/// ```rust
>> +/// use kernel::io::poll::read_poll_timeout;
>> +/// use kernel::time::Delta;
>> +/// use kernel::sync::{SpinLock, new_spinlock};
>> +///
>> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
>> +/// let g = lock.lock();
>> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
>> +/// drop(g);
>> +///
>> +/// # Ok::<(), Error>(())
>> +/// ```
>
> I am guessing this example is present to test the call to `might_sleep`.
I also guess so. Boqun wrote this test, IIRC.
> Could you document the reason for the test. As an example, this code is
> not really usable. `#[test]` was staged for 6.15, so perhaps move this
> to a unit test instead?
>
> The test throws this BUG, which is what I think is also your intention:
might_sleep() doesn't throw BUG(), just a warning. Can the test
infrastructure handle such?
"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
> Sorry, I somehow missed this email.
>
> On Sat, 22 Mar 2025 17:02:31 +0100
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>>> +/// Lower CPU power consumption or yield to a hyperthreaded twin processor.
>>> +///
>>> +/// It also happens to serve as a compiler barrier.
>>> +pub fn cpu_relax() {
>>> + // SAFETY: FFI call.
>>
>> I don't think this safety comment is sufficient. There are two other
>> similar comments further down.
>
> Updated the comment.
>
>>> +/// ```rust
>>> +/// use kernel::io::poll::read_poll_timeout;
>>> +/// use kernel::time::Delta;
>>> +/// use kernel::sync::{SpinLock, new_spinlock};
>>> +///
>>> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
>>> +/// let g = lock.lock();
>>> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
>>> +/// drop(g);
>>> +///
>>> +/// # Ok::<(), Error>(())
>>> +/// ```
>>
>> I am guessing this example is present to test the call to `might_sleep`.
>
> I also guess so. Boqun wrote this test, IIRC.
>
>> Could you document the reason for the test. As an example, this code is
>> not really usable. `#[test]` was staged for 6.15, so perhaps move this
>> to a unit test instead?
>>
>> The test throws this BUG, which is what I think is also your intention:
>
> might_sleep() doesn't throw BUG(), just a warning. Can the test
> infrastructure handle such?
As I wrote, kunit does not handle this. But I am confused about the
bug/warn comment. The trace I pasted clearly says "BUG"?
I think we should just remove this test for now.
Best regards,
Andreas Hindborg
On Mon, 11 Aug 2025 11:42:46 +0200 Andreas Hindborg <a.hindborg@kernel.org> wrote: >>> Could you document the reason for the test. As an example, this code is >>> not really usable. `#[test]` was staged for 6.15, so perhaps move this >>> to a unit test instead? >>> >>> The test throws this BUG, which is what I think is also your intention: >> >> might_sleep() doesn't throw BUG(), just a warning. Can the test >> infrastructure handle such? > > As I wrote, kunit does not handle this. But I am confused about the > bug/warn comment. The trace I pasted clearly says "BUG"? Yeah, might_sleep() says "BUG" like BUG() macros but they are different. might_sleep() simply prints debug information. > I think we should just remove this test for now. I'll do in the next version.
FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
> Add read_poll_timeout functions which poll periodically until a
> condition is met or a timeout is reached.
>
> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
> macro and a simple wrapper for Rust doesn't work. So this implements
> the same functionality in Rust.
>
> The C version uses usleep_range() while the Rust version uses
> fsleep(), which uses the best sleep method so it works with spans that
> usleep_range() doesn't work nicely with.
>
> The sleep_before_read argument isn't supported since there is no user
> for now. It's rarely used in the C version.
>
> read_poll_timeout() can only be used in a nonatomic context. This
> requirement is not checked by these abstractions, but it is intended
> that klint [1] or a similar tool will be used to check it in the
> future.
>
> Link: https://rust-for-linux.com/klint [1]
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Reviewed-by: Fiona Behrens <me@kloenk.dev>
> ---
> rust/helpers/helpers.c | 1 +
> rust/helpers/kernel.c | 18 +++++++
> rust/kernel/cpu.rs | 13 +++++
> rust/kernel/error.rs | 1 +
> rust/kernel/io.rs | 2 +
> rust/kernel/io/poll.rs | 120 +++++++++++++++++++++++++++++++++++++++++
> rust/kernel/lib.rs | 1 +
> 7 files changed, 156 insertions(+)
> create mode 100644 rust/helpers/kernel.c
> create mode 100644 rust/kernel/cpu.rs
> create mode 100644 rust/kernel/io/poll.rs
>
> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
> new file mode 100644
> index 000000000000..5977b2082cc6
> --- /dev/null
> +++ b/rust/kernel/io/poll.rs
> @@ -0,0 +1,120 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! IO polling.
> +//!
> +//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
> +
> +use crate::{
> + cpu::cpu_relax,
> + error::{code::*, Result},
> + time::{delay::fsleep, Delta, Instant},
> +};
> +
> +/// 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.
> +///
> +/// # Examples
> +///
> +/// ```rust,ignore
> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
> +/// // The `op` closure reads the value of a specific status register.
> +/// let op = || -> Result<u16> { dev.read_ready_register() };
> +///
> +/// // The `cond` closure takes a reference to the value returned by `op`
> +/// // and checks whether the hardware is ready.
> +/// let cond = |val: &u16| *val == HW_READY;
> +///
> +/// match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
> +/// Ok(_) => {
> +/// // The hardware is ready. The returned value of the `op`` closure isn't used.
> +/// Ok(())
> +/// }
> +/// Err(e) => Err(e),
> +/// }
> +/// }
> +/// ```
> +///
> +/// ```rust
> +/// use kernel::io::poll::read_poll_timeout;
> +/// use kernel::time::Delta;
> +/// use kernel::sync::{SpinLock, new_spinlock};
> +///
> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
> +/// let g = lock.lock();
> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
> +/// drop(g);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +#[track_caller]
> +pub fn read_poll_timeout<Op, Cond, T>(
> + mut op: Op,
> + mut cond: Cond,
> + sleep_delta: Delta,
> + timeout_delta: Option<Delta>,
Fun idea I just had, though not sure it is of actuall use (probably not).
Instead of `Option<Delta> we could use `impl Into<Option<Delta>>`,
that enables to use both, so not having to write Some if we have a value.
> +) -> Result<T>
> +where
> + Op: FnMut() -> Result<T>,
> + Cond: FnMut(&T) -> bool,
> +{
> + let start = Instant::now();
> + let sleep = !sleep_delta.is_zero();
> +
> + if sleep {
> + might_sleep();
> + }
> +
> + loop {
> + let val = op()?;
> + if cond(&val) {
> + // Unlike the C version, we immediately return.
> + // We know the condition is met so we don't need to check again.
> + return Ok(val);
> + }
> + if let Some(timeout_delta) = timeout_delta {
> + if start.elapsed() > timeout_delta {
> + // Unlike the C version, we immediately return.
> + // We have just called `op()` so we don't need to call it again.
> + return Err(ETIMEDOUT);
> + }
> + }
> + if sleep {
> + fsleep(sleep_delta);
> + }
> + // fsleep() could be busy-wait loop so we always call cpu_relax().
> + cpu_relax();
> + }
> +}
> +
> +/// Annotation for functions that can sleep.
> +///
> +/// Equivalent to the C side [`might_sleep()`], this function serves as
> +/// a debugging aid and a potential scheduling point.
> +///
> +/// This function can only be used in a nonatomic context.
> +#[track_caller]
> +fn might_sleep() {
> + #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
> + {
> + let loc = core::panic::Location::caller();
> + // SAFETY: FFI call.
> + unsafe {
> + crate::bindings::__might_sleep_precision(
> + loc.file().as_ptr().cast(),
> + loc.file().len() as i32,
> + loc.line() as i32,
> + )
> + }
> + }
> +
> + // SAFETY: FFI call.
> + unsafe { crate::bindings::might_resched() }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 496ed32b0911..415c500212dd 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -40,6 +40,7 @@
> pub mod block;
> #[doc(hidden)]
> pub mod build_assert;
> +pub mod cpu;
> pub mod cred;
> pub mod device;
> pub mod device_id;
On Thu, 20 Feb 2025 16:04:50 +0100 Fiona Behrens <me@kloenk.dev> wrote: >> Add read_poll_timeout functions which poll periodically until a >> condition is met or a timeout is reached. >> >> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated >> macro and a simple wrapper for Rust doesn't work. So this implements >> the same functionality in Rust. >> >> The C version uses usleep_range() while the Rust version uses >> fsleep(), which uses the best sleep method so it works with spans that >> usleep_range() doesn't work nicely with. >> >> The sleep_before_read argument isn't supported since there is no user >> for now. It's rarely used in the C version. >> >> read_poll_timeout() can only be used in a nonatomic context. This >> requirement is not checked by these abstractions, but it is intended >> that klint [1] or a similar tool will be used to check it in the >> future. >> >> Link: https://rust-for-linux.com/klint [1] >> Tested-by: Daniel Almeida <daniel.almeida@collabora.com> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com> > > Reviewed-by: Fiona Behrens <me@kloenk.dev> Thanks! >> +#[track_caller] >> +pub fn read_poll_timeout<Op, Cond, T>( >> + mut op: Op, >> + mut cond: Cond, >> + sleep_delta: Delta, >> + timeout_delta: Option<Delta>, > > Fun idea I just had, though not sure it is of actuall use (probably not). > Instead of `Option<Delta> we could use `impl Into<Option<Delta>>`, > that enables to use both, so not having to write Some if we have a value. Either is fine by me. I couldn't find any functions under the rust/kernel that use impl Into<Option<T>> as an argument. Any rules regarding this?
© 2016 - 2025 Red Hat, Inc.