Message-Id: <DCB831D1-8786-41BC-A95B-44F0BEE71990@collabora.com>
Date: Tue, 5 Aug 2025 11:01:22 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>,
 linux-kernel@...r.kernel.org,
 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 2 Aug 2025, at 08:06, 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.
> 
>>>> +/// 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.
> 
>> 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.

Perhaps it’s worth it to clarify that in the docs for the future versions?

I feel like “sleep_delta == 0 -> this doesn’t sleep -> it’s fine to
use this in an atomic context” is a somewhat expected thought process.
Also, this will lead to the confusion I mentioned, i.e. “why do I need to
use the atomic version if I can pass 0 for sleep_delta?”

I mean, the added context in this thread explains it, but it’s probably
worth it to make sure that the docs also do.

Just my humble opinion.

— Daniel

Powered by blists - more mailing lists