Message-Id: <20250802.104249.1482605492526656971.fujita.tomonori@gmail.com>
Date: Sat, 02 Aug 2025 10:42:49 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: dakr@...nel.org
Cc: fujita.tomonori@...il.com, linux-kernel@...r.kernel.org,
 daniel.almeida@...labora.com, rust-for-linux@...r.kernel.org,
 netdev@...r.kernel.org, andrew@...n.ch, hkallweit1@...il.com,
 tmgross@...ch.edu, ojeda@...nel.org, alex.gaynor@...il.com,
 gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
 a.hindborg@...sung.com, aliceryhl@...gle.com, anna-maria@...utronix.de,
 frederic@...nel.org, tglx@...utronix.de, arnd@...db.de,
 jstultz@...gle.com, sboyd@...nel.org, mingo@...hat.com,
 peterz@...radead.org, juri.lelli@...hat.com, vincent.guittot@...aro.org,
 dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
 mgorman@...e.de, vschneid@...hat.com, tgunders@...hat.com, me@...enk.dev,
 david.laight.linux@...il.com
Subject: Re: [PATCH v11 7/8] rust: Add read_poll_timeout functions

On Mon, 28 Jul 2025 15:13:45 +0200
"Danilo Krummrich" <dakr@...nel.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.

Powered by blists - more mailing lists