Message-Id: <20250805.223721.524503114987740782.fujita.tomonori@gmail.com>
Date: Tue, 05 Aug 2025 22:37:21 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: dakr@...nel.org
Cc: fujita.tomonori@...il.com, linux-kernel@...r.kernel.org,
 daniel.almeida@...labora.com, rust-for-linux@...r.kernel.org,
 netdev@...r.kernel.org, andrew@...n.ch, hkallweit1@...il.com,
 tmgross@...ch.edu, ojeda@...nel.org, alex.gaynor@...il.com,
 gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
 a.hindborg@...sung.com, aliceryhl@...gle.com, anna-maria@...utronix.de,
 frederic@...nel.org, tglx@...utronix.de, arnd@...db.de,
 jstultz@...gle.com, sboyd@...nel.org, mingo@...hat.com,
 peterz@...radead.org, juri.lelli@...hat.com, vincent.guittot@...aro.org,
 dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
 mgorman@...e.de, vschneid@...hat.com, tgunders@...hat.com, me@...enk.dev,
 david.laight.linux@...il.com
Subject: Re: [PATCH v11 7/8] rust: Add read_poll_timeout functions

On Sat, 02 Aug 2025 13:06:04 +0200
"Danilo Krummrich" <dakr@...nel.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@...nel.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.

Powered by blists - more mailing lists