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

["Danilo Krummrich" <dakr@kernel.org>]

On Wed Aug 27, 2025 at 2:22 PM CEST, Daniel Almeida wrote:
>
>
>> On 27 Aug 2025, at 09:19, Danilo Krummrich <dakr@kernel.org> wrote:
>> 
>> On Wed Aug 27, 2025 at 2:14 PM CEST, Daniel Almeida wrote:
>>> Hi Danilo,
>>> 
>>> […}
>>> 
>>>> 
>>>> Actually, let me put it in other words:
>>>> 
>>>> let val = read_poll_timeout_atomic(
>>>>    || {
>>>>        // Fetch the offset to read from from the HW.
>>>>        let offset = io.read32(0x1000);
>>>> 
>>>>        // HW needs a break for some odd reason.
>>>>        udelay(100);
>
> Why would we have a delay here? Can’t this be broken into two calls to
> read_poll_timeout_atomic()? That would be equivalent to what you wrote
> IIUC.

I'm sure this can somehow be written otherwise as well. But that's not the
point, the point is that this looks like perfectly valid code from a users
perspective.

>>>> 
>>>>        // Read the actual value.
>>>>        io.try_read32(offset)
>>>>    },
>>>>    |val: &u32| *val == HW_READY,
>>>>    Delta::from_micros(0),      // No delay, keep spinning.
>>>>    Delta::from_millis(10),     // Timeout after 10ms.
>>>> )?;
>>>> 
>>>> Seems like a fairly reasonable usage without knowing the implementation details
>>>> of read_poll_timeout_atomic(), right?
>>>> 
>>>> Except that if the hardware does not become ready, this will spin for 16.67
>>>> *minutes* -- in atomic context. Instead of the 10ms the user would expect.
>
> This is where you lost me. Where does the 16.67 come from?

Ah, I see -- let me explain:

Internally read_poll_timeout_atomic() would convert the timeout (10ms) into ns
(let's call it nanos). Then, it would decrement nanos in every iteration of the
internal loop, based on the (wrong) assumption that every loop takes exactly
1ns.

However, since the user executes udelay(100), which is perfectly valid from the
users perspective, in the Op closure, every loop iteration takes at least 100us
instead.

So, the actual timeout calculates as follows.

	Timeout: 10ms = 10.000us = 10.000.000ns

In every iteration this number is decremented by one, hence 10.000.000
iterations.

	100us * 10.000.000 iterations = 16.67 minutes

So, the issue really is that we're not measuring time, but the number of
iterations if delay_delta == 0.

As delay_delta grows the relative eror becomes smaller, yet this is far from
sane behavior.

>>>> 
>>>> This would be way less error prone if we do not provide a timeout value, but a
>>>> retry count.
>>>> 
>>>>> Instead, I think it makes much more sense to provide a retry count as function
>>>>> argument, such that the user can specify "I want a dealy of 100us, try it 100
>>>>> times".
>>>>> 
>>>>> This way it is transparent to the caller that the timeout may be significantly
>>>>> more than 10ms depending on the user's implementation.
>>>>> 
>>>>> As for doing this in C vs Rust: I don't think things have to align in every
>>>>> implementation detail. If we can improve things on the Rust side from the
>>>>> get-go, we should not stop ourselves from doing so, just because a similar C
>>>>> implementation is hard to refactor, due to having a lot of users already.
>>> 
>>> I must say I do not follow. Can you expand yet some more on this?
>> 
>> Sure, but it would help if you could clarify which aspect you want me to expand
>> on. :)