Re: [PATCH v10 7/8] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

On Fri, 7 Feb 2025 22:50:37 -0300
Daniel Almeida <daniel.almeida@xxxxxxxxxxxxx> wrote:

>> +/// Polls periodically until a condition is met or a timeout is reached.
>> +///
>> +/// ```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>(())
> 
> IMHO, the example section here needs to be improved.

Do you have any specific ideas?

Generally, this function is used to wait for the hardware to be
ready. So I can't think of a nice example.


>> +/// ```
>> +#[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(Location::caller());
>> +    }
>> +
>> +    loop {
>> +        let val = op()?;
> 
> I.e.: it’s not clear that `op` computes `val` until you read this.
> 
>> +        if cond(&val) {
> 
> It’s a bit unclear how `cond` works, until you see this line here.
> 
> It’s even more important to explain this a tad better, since it differs slightly from the C API.

ok, I'll try to add some docs in v11.

> Also, it doesn’t help that `Op` and `Cond` take and return the unit type in the
> only example provided.
> 
> ― Daniel
> 
>