Message-Id: <DBRW63AMB4D8.2HXGYM6FZRX3Z@kernel.org>
Date: Sat, 02 Aug 2025 13:06:04 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "FUJITA Tomonori" <fujita.tomonori@...il.com>
Cc: <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 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.

Powered by blists - more mailing lists