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

FUJITA Tomonori
2025-08-05T13:37:40Z

On Sat, 02 Aug 2025 13:06:04 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:

> On Sat Aug 2, 2025 at 3:42 AM CEST, FUJITA Tomonori wrote:
>> On Mon, 28 Jul 2025 15:13:45 +0200
>> "Danilo Krummrich" <dakr@kernel.org> wrote:
>>> On Thu Feb 20, 2025 at 8:06 AM CET, FUJITA Tomonori wrote:
>>>> +/// This process continues until either `cond` returns `true` or the timeout,
>>>> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
>>>> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
>>>> +///
>>>> +/// # Examples
>>>> +///
>>>> +/// ```rust,ignore
>>>
>>> Why ignore? This should be possible to compile test.
>>
>> https://lore.kernel.org/rust-for-linux/CEF87294-8580-4C84-BEA3-EB72E63ED7DF@collabora.com/
>
> I disagree with that. 'ignore' should only be used if we can't make it compile.
>
> In this case we can make it compile, we just can't run it, since there's no real
> HW underneath that we can read registers from.
>
> An example that isn't compiled will eventually be forgotten to be updated when
> things are changed.

I also prefer the example that can be compiled however I can't think
of a compilable example that is similar to actual use cases (for
example, waiting for some hardware condition). Do you have any ideas?

>>>> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
>>>
>>> I think the parameter here can just be `&Io<SIZE>`.
>>>
>>>> +/// // The `op` closure reads the value of a specific status register.
>>>> +/// let op = || -> Result<u16> { dev.read_ready_register() };
>>>> +///
>>>> +/// // The `cond` closure takes a reference to the value returned by `op`
>>>> +/// // and checks whether the hardware is ready.
>>>> +/// let cond = |val: &u16| *val == HW_READY;
>>>> +///
>>>> +/// match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
>>>> +/// Ok(_) => {
>>>> +/// // The hardware is ready. The returned value of the `op`` closure isn't used.
>>>> +/// Ok(())
>>>> +/// }
>>>> +/// Err(e) => Err(e),
>>>> +/// }
>>>> +/// }
>>>> +/// ```
>>>> +///
>>>> +/// ```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>(())
>>>> +/// ```
>>>> +#[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();
>>>> + }
>>>
>>> I think a conditional might_sleep() is not great.
>>>
>>> I also think we can catch this at compile time, if we add two different variants
>>> of read_poll_timeout() instead and be explicit about it. We could get Klint to
>>> catch such issues for us at compile time.
>>
>> Your point is that functions which cannot be used in atomic context
>> should be clearly separated into different ones. Then Klint might be
>> able to detect such usage at compile time, right?
>>
>> How about dropping the conditional might_sleep() and making
>> read_poll_timeout return an error with zero sleep_delta?
>
> Yes, let's always call might_sleep(), the conditional is very error prone. We
> want to see the warning splat whenever someone calls read_poll_timeout() from
> atomic context.
>
> Yes, with zero sleep_delta it could be called from atomic context technically,
> but if drivers rely on this and wrap this into higher level helpers it's very
> easy to miss a subtle case and end up with non-zero sleep_delta within an atomic
> context for some rare condition that then is hard to debug.
>
> As for making read_poll_timeout() return a error with zero sleep_delta, I don't
> see a reason to do that. If a driver wraps read_poll_timeout() in its own
> function that sometimes sleeps and sometimes does not, based on some condition,
> but is never called from atomic context, that's fine.

Ok, let's always call might_sleep, even when there's no possibility of
sleeping.

>> Drivers which need busy-loop (without even udelay) can
>> call read_poll_timeout_atomic() with zero delay.
>
> It's not the zero delay or zero sleep_delta that makes the difference it's
> really the fact the one can be called from atomic context and one can't be.

That's what I meant.

Since calling read_poll_timeout_atomic() with zero delay can achieve
the same thing as calling read_poll_timeout with zero sleep delta, I
thought the zero sleep delta in read_poll_timeout is unncessary.

But as in the use case you mentioned above, a driver could use the
sleep delta that is not const and calls the function with both
non-zero and zero delta values so let's keep zero sleep delta support.