linux-kernel - Re: [PATCH v2 2/2] rust: Add read_poll

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 for Android: free password hash cracker in your pocket

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

Message-Id: <20250820.201139.2145357080225408024.fujita.tomonori@gmail.com>
Date: Wed, 20 Aug 2025 20:11:39 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: acourbot@...dia.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, daniel.almeida@...labora.com,
 me@...enk.dev
Subject: Re: [PATCH v2 2/2] rust: Add read_poll_timeout functions

On Tue, 19 Aug 2025 09:49:38 +0900
"Alexandre Courbot" <acourbot@...dia.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@...nel.org>
>> Reviewed-by: Fiona Behrens <me@...enk.dev>
>> Tested-by: Daniel Almeida <daniel.almeida@...labora.com>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
> 
> Tested this with nova-core, and it seems to work fine!
> 
> Reviewed-by: Alexandre Courbot <acourbot@...dia.com>
> Tested-by: Alexandre Courbot <acourbot@...dia.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.