linux-kernel - Re: [PATCH v1 2/2] rust: Add read_poll_timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20250827.093559.1495790445785541454.fujita.tomonori@gmail.com>
Date: Wed, 27 Aug 2025 09:35:59 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: daniel.almeida@...labora.com
Cc: fujita.tomonori@...il.com, a.hindborg@...nel.org,
 alex.gaynor@...il.com, ojeda@...nel.org, aliceryhl@...gle.com,
 anna-maria@...utronix.de, bjorn3_gh@...tonmail.com, boqun.feng@...il.com,
 dakr@...nel.org, frederic@...nel.org, gary@...yguo.net,
 jstultz@...gle.com, linux-kernel@...r.kernel.org, lossin@...nel.org,
 lyude@...hat.com, rust-for-linux@...r.kernel.org, sboyd@...nel.org,
 tglx@...utronix.de, tmgross@...ch.edu, acourbot@...dia.com
Subject: Re: [PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

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

>> +/// Polls periodically until a condition is met, an error occurs,
>> +/// or the timeout is reached.
>> +///
>> +/// The function repeatedly executes the given operation `op` closure and
>> +/// checks its result using the condition closure `cond`.
>> +///
>> +/// If `cond` returns `true`, the function returns successfully with the result of `op`.
>> +/// Otherwise, it performs a busy wait for a duration specified by `delay_delta`
>> +/// before executing `op` again.
>> +///
>> +/// This process continues until either `op` returns an error, `cond`
>> +/// returns `true`, or the timeout specified by `timeout_delta` is
>> +/// reached.
>> +///
>> +/// # Errors
>> +///
>> +/// If `op` returns an error, then that error is returned directly.
>> +///
>> +/// If the timeout specified by `timeout_delta` is reached, then
>> +/// `Err(ETIMEDOUT)` is returned.
>> +///
>> +/// # Examples
>> +///
>> +/// ```no_run
>> +/// use kernel::io::{Io, poll::read_poll_timeout_atomic};
>> +/// use kernel::time::Delta;
>> +///
>> +/// const HW_READY: u16 = 0x01;
>> +///
>> +/// fn wait_for_hardware<const SIZE: usize>(io: &Io<SIZE>) -> Result<()> {
> 
> Just “Result”.

Oops, thanks.

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


>> +///     match read_poll_timeout_atomic(
>> +///         // The `op` closure reads the value of a specific status register.
>> +///         || io.try_read16(0x1000),
>> +///         // The `cond` closure takes a reference to the value returned by `op`
>> +///         // and checks whether the hardware is ready.
>> +///         |val: &u16| *val == HW_READY,
>> +///         Delta::from_micros(50),
>> +///         Delta::from_micros(300),
>> +///     ) {
>> +///         Ok(_) => {
>> +///             // The hardware is ready. The returned value of the `op` closure
>> +///             // isn't used.
>> +///             Ok(())
>> +///         }
>> +///         Err(e) => Err(e),
>> +///     }
>> +/// }
>> +/// ```
>> +pub fn read_poll_timeout_atomic<Op, Cond, T>(
>> +    mut op: Op,
>> +    mut cond: Cond,
>> +    delay_delta: Delta,
>> +    timeout_delta: Delta,
>> +) -> Result<T>
>> +where
>> +    Op: FnMut() -> Result<T>,
>> +    Cond: FnMut(&T) -> bool,
>> +{
>> +    let mut left_ns = timeout_delta.as_nanos();
>> +    let delay_ns = delay_delta.as_nanos();
>> +
>> +    loop {
>> +        let val = op()?;
>> +        if cond(&val) {
>> +            // Unlike the C version, we immediately return.
>> +            // We know the condition is met so we don't need to check again.
>> +            return Ok(val);
>> +        }
>> +
>> +        if left_ns < 0 {
>> +            // Unlike the C version, we immediately return.
>> +            // We have just called `op()` so we don't need to call it again.
>> +            return Err(ETIMEDOUT);
>> +        }
>> +
>> +        if !delay_delta.is_zero() {
>> +            udelay(delay_delta);
>> +            left_ns -= delay_ns;
>> +        }
>> +
>> +        cpu_relax();
>> +        left_ns -= 1;
> 
> A comment on the line above would be nice.

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

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


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

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