Re: [PATCH v1 2/2] rust: Add read_poll_timeout_atomic function

[FUJITA Tomonori]

On Tue, 26 Aug 2025 11:02:18 -0300
Daniel Almeida <daniel.almeida@xxxxxxxxxxxxx> 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.