[PATCH v9 0/8] rust: Add IO polling

[PATCH v9 0/8] rust: Add IO polling

[FUJITA Tomonori]

Add a helper function to poll periodically until a condition is met or
a timeout is reached. By using the function, the 8th patch fixes
QT2025 PHY driver to sleep until the hardware becomes ready.

The first patch is for sched/core, which adds
__might_sleep_precision(), rust friendly version of __might_sleep(),
which takes a pointer to a string with the length instead of a
null-terminated string. Rust's core::panic::Location::file(), which
gives the file name of a caller, doesn't provide a null-terminated
string. __might_sleep_precision() uses a precision specifier in the
printk format, which specifies the length of a string; a string
doesn't need to be a null-terminated.

The remaining patches are for the Rust side.

This introduces two new types, Instant and Delta, which represent a
specific point in time and a span of time, respectively.

Unlike the old rust branch, This adds a wrapper for fsleep() instead
of msleep(). fsleep() automatically chooses the best sleep method
based on a duration.

[1]: https://github.com/rust-lang/libs-team/issues/466

v9:
- make the might_sleep() changes into as a separate patch
- add as_millis() method to Delta for Binder driver
- make Delta's as_*() methods const (useful in some use cases)
- add Delta::ZERO const; used in fsleep()
- fix typos
- use intra-doc links
- place the #[inline] marker before the documentation
- remove Instant's from_raw() method
- add Invariants to Instant type
- improve Delta's methods documents
- fix fsleep() SAFETY comment
- improve fsleep() documents
- lift T:Copy restriction in read_poll_timeout()
- use MutFn for Cond in read_poll_timeout() instead of Fn
- fix might_sleep() call in read_poll_timeout()
- simplify read_poll_timeout() logic
v8: https://lore.kernel.org/lkml/20250116044100.80679-1-fujita.tomonori@gmail.com/
- fix compile warnings
v7: https://lore.kernel.org/lkml/20241220061853.2782878-1-fujita.tomonori@gmail.com/
- rebased on rust-next
- use crate::ffi instead of core::ffi
v6: https://lore.kernel.org/lkml/20241114070234.116329-1-fujita.tomonori@gmail.com/
- use super::Delta in delay.rs
- improve the comments
- add Delta's is_negative() method
- rename processor.rs to cpu.rs for cpu_relax()
- add __might_sleep_precision() taking pointer to a string with the length
- implement read_poll_timeout as normal function instead of macro
v5: https://lore.kernel.org/lkml/20241101010121.69221-1-fujita.tomonori@gmail.com/
- set the range of Delta for fsleep function
- update comments
v4: https://lore.kernel.org/lkml/20241025033118.44452-1-fujita.tomonori@gmail.com/
- rebase on the tip tree's timers/core
- add Instant instead of using Ktime
- remove unused basic methods
- add Delta as_micros_ceil method
- use const fn for Delta from_* methods
- add more comments based on the feedback
- add a safe wrapper for cpu_relax()
- add __might_sleep() macro
v3: https://lore.kernel.org/lkml/20241016035214.2229-1-fujita.tomonori@gmail.com/
- Update time::Delta methods (use i64 for everything)
- Fix read_poll_timeout to show the proper debug info (file and line)
- Move fsleep to rust/kernel/time/delay.rs
- Round up delta for fsleep
- Access directly ktime_t instead of using ktime APIs
- Add Eq and Ord with PartialEq and PartialOrd
v2: https://lore.kernel.org/lkml/20241005122531.20298-1-fujita.tomonori@gmail.com/
- Introduce time::Delta instead of core::time::Duration
- Add some trait to Ktime for calculating timeout
- Use read_poll_timeout in QT2025 driver instead of using fsleep directly
v1: https://lore.kernel.org/netdev/20241001112512.4861-1-fujita.tomonori@gmail.com/

FUJITA Tomonori (8):
  sched: Add __might_sleep_precision()
  rust: time: Add PartialEq/Eq/PartialOrd/Ord trait to Ktime
  rust: time: Introduce Delta type
  rust: time: Introduce Instant type
  rust: time: Add wrapper for fsleep() function
  MAINTAINERS: rust: Add TIMEKEEPING and TIMER abstractions
  rust: Add read_poll_timeout functions
  net: phy: qt2025: Wait until PHY becomes ready

 MAINTAINERS               |   2 +
 drivers/net/phy/qt2025.rs |  10 ++-
 include/linux/kernel.h    |   2 +
 kernel/sched/core.c       |  55 ++++++++------
 rust/helpers/helpers.c    |   2 +
 rust/helpers/kernel.c     |  13 ++++
 rust/helpers/time.c       |   8 ++
 rust/kernel/cpu.rs        |  13 ++++
 rust/kernel/error.rs      |   1 +
 rust/kernel/io.rs         |   5 ++
 rust/kernel/io/poll.rs    |  79 +++++++++++++++++++
 rust/kernel/lib.rs        |   2 +
 rust/kernel/time.rs       | 155 ++++++++++++++++++++++++++++++--------
 rust/kernel/time/delay.rs |  49 ++++++++++++
 14 files changed, 342 insertions(+), 54 deletions(-)
 create mode 100644 rust/helpers/kernel.c
 create mode 100644 rust/helpers/time.c
 create mode 100644 rust/kernel/cpu.rs
 create mode 100644 rust/kernel/io.rs
 create mode 100644 rust/kernel/io/poll.rs
 create mode 100644 rust/kernel/time/delay.rs


base-commit: 606489dbfa979dce53797f24840c512d0e7510f9
-- 
2.43.0

[PATCH v9 1/8] sched/core: Add __might_sleep_precision()

[FUJITA Tomonori]

Add __might_sleep_precision(), Rust friendly version of
__might_sleep(), which takes a pointer to a string with the length
instead of a null-terminated string.

Rust's core::panic::Location::file(), which gives the file name of a
caller, doesn't provide a null-terminated
string. __might_sleep_precision() uses a precision specifier in the
printk format, which specifies the length of a string; a string
doesn't need to be a null-terminated.

Modify __might_sleep() to call __might_sleep_precision() but the
impact should be negligible. strlen() isn't called in a normal case;
it's called only when printing the error (sleeping function called
from invalid context).

Note that Location::file() providing a null-terminated string for
better C interoperability is under discussion [1].

[1]: https://github.com/rust-lang/libs-team/issues/466
Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 include/linux/kernel.h |  2 ++
 kernel/sched/core.c    | 55 ++++++++++++++++++++++++++----------------
 2 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index be2e8c0a187e..086ee1dc447e 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -87,6 +87,7 @@ extern int dynamic_might_resched(void);
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
 extern void __might_resched(const char *file, int line, unsigned int offsets);
 extern void __might_sleep(const char *file, int line);
+extern void __might_sleep_precision(const char *file, int len, int line);
 extern void __cant_sleep(const char *file, int line, int preempt_offset);
 extern void __cant_migrate(const char *file, int line);
 
@@ -145,6 +146,7 @@ extern void __cant_migrate(const char *file, int line);
   static inline void __might_resched(const char *file, int line,
 				     unsigned int offsets) { }
 static inline void __might_sleep(const char *file, int line) { }
+static inline void __might_sleep_precision(const char *file, int len, int line) { }
 # define might_sleep() do { might_resched(); } while (0)
 # define cant_sleep() do { } while (0)
 # define cant_migrate()		do { } while (0)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 88a9a515b2ba..69d77c14ad33 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8666,24 +8666,6 @@ void __init sched_init(void)
 
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
 
-void __might_sleep(const char *file, int line)
-{
-	unsigned int state = get_current_state();
-	/*
-	 * Blocking primitives will set (and therefore destroy) current->state,
-	 * since we will exit with TASK_RUNNING make sure we enter with it,
-	 * otherwise we will destroy state.
-	 */
-	WARN_ONCE(state != TASK_RUNNING && current->task_state_change,
-			"do not call blocking ops when !TASK_RUNNING; "
-			"state=%x set at [<%p>] %pS\n", state,
-			(void *)current->task_state_change,
-			(void *)current->task_state_change);
-
-	__might_resched(file, line, 0);
-}
-EXPORT_SYMBOL(__might_sleep);
-
 static void print_preempt_disable_ip(int preempt_offset, unsigned long ip)
 {
 	if (!IS_ENABLED(CONFIG_DEBUG_PREEMPT))
@@ -8705,7 +8687,8 @@ static inline bool resched_offsets_ok(unsigned int offsets)
 	return nested == offsets;
 }
 
-void __might_resched(const char *file, int line, unsigned int offsets)
+static void __might_resched_precision(const char *file, int len, int line,
+				      unsigned int offsets)
 {
 	/* Ratelimiting timestamp: */
 	static unsigned long prev_jiffy;
@@ -8728,8 +8711,10 @@ void __might_resched(const char *file, int line, unsigned int offsets)
 	/* Save this before calling printk(), since that will clobber it: */
 	preempt_disable_ip = get_preempt_disable_ip(current);
 
-	pr_err("BUG: sleeping function called from invalid context at %s:%d\n",
-	       file, line);
+	if (len < 0)
+		len = strlen(file);
+	pr_err("BUG: sleeping function called from invalid context at %.*s:%d\n",
+	       len, file, line);
 	pr_err("in_atomic(): %d, irqs_disabled(): %d, non_block: %d, pid: %d, name: %s\n",
 	       in_atomic(), irqs_disabled(), current->non_block_count,
 	       current->pid, current->comm);
@@ -8754,8 +8739,36 @@ void __might_resched(const char *file, int line, unsigned int offsets)
 	dump_stack();
 	add_taint(TAINT_WARN, LOCKDEP_STILL_OK);
 }
+
+void __might_resched(const char *file, int line, unsigned int offsets)
+{
+	__might_resched_precision(file, -1, line, offsets);
+}
 EXPORT_SYMBOL(__might_resched);
 
+void __might_sleep_precision(const char *file, int len, int line)
+{
+	unsigned int state = get_current_state();
+	/*
+	 * Blocking primitives will set (and therefore destroy) current->state,
+	 * since we will exit with TASK_RUNNING make sure we enter with it,
+	 * otherwise we will destroy state.
+	 */
+	WARN_ONCE(state != TASK_RUNNING && current->task_state_change,
+			"do not call blocking ops when !TASK_RUNNING; "
+			"state=%x set at [<%p>] %pS\n", state,
+			(void *)current->task_state_change,
+			(void *)current->task_state_change);
+
+	__might_resched_precision(file, len, line, 0);
+}
+
+void __might_sleep(const char *file, int line)
+{
+	__might_sleep_precision(file, -1, line);
+}
+EXPORT_SYMBOL(__might_sleep);
+
 void __cant_sleep(const char *file, int line, int preempt_offset)
 {
 	static unsigned long prev_jiffy;
-- 
2.43.0

Re: [PATCH v9 1/8] sched/core: Add __might_sleep_precision()

[Alice Ryhl]

On Sat, Jan 25, 2025 at 11:20 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Add __might_sleep_precision(), Rust friendly version of
> __might_sleep(), which takes a pointer to a string with the length
> instead of a null-terminated string.
>
> Rust's core::panic::Location::file(), which gives the file name of a
> caller, doesn't provide a null-terminated
> string. __might_sleep_precision() uses a precision specifier in the
> printk format, which specifies the length of a string; a string
> doesn't need to be a null-terminated.
>
> Modify __might_sleep() to call __might_sleep_precision() but the
> impact should be negligible. strlen() isn't called in a normal case;
> it's called only when printing the error (sleeping function called
> from invalid context).
>
> Note that Location::file() providing a null-terminated string for
> better C interoperability is under discussion [1].
>
> [1]: https://github.com/rust-lang/libs-team/issues/466
> Co-developed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v9 1/8] sched/core: Add __might_sleep_precision()

[Peter Zijlstra]

On Sat, Jan 25, 2025 at 07:18:46PM +0900, FUJITA Tomonori wrote:
> Add __might_sleep_precision(), Rust friendly version of
> __might_sleep(), which takes a pointer to a string with the length
> instead of a null-terminated string.
> 
> Rust's core::panic::Location::file(), which gives the file name of a
> caller, doesn't provide a null-terminated
> string. __might_sleep_precision() uses a precision specifier in the
> printk format, which specifies the length of a string; a string
> doesn't need to be a null-terminated.
> 
> Modify __might_sleep() to call __might_sleep_precision() but the
> impact should be negligible. strlen() isn't called in a normal case;
> it's called only when printing the error (sleeping function called
> from invalid context).
> 
> Note that Location::file() providing a null-terminated string for
> better C interoperability is under discussion [1].

Urgh :/

Re: [PATCH v9 1/8] sched/core: Add __might_sleep_precision()

[FUJITA Tomonori]

On Tue, 28 Jan 2025 12:37:38 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Sat, Jan 25, 2025 at 07:18:46PM +0900, FUJITA Tomonori wrote:
>> Add __might_sleep_precision(), Rust friendly version of
>> __might_sleep(), which takes a pointer to a string with the length
>> instead of a null-terminated string.
>> 
>> Rust's core::panic::Location::file(), which gives the file name of a
>> caller, doesn't provide a null-terminated
>> string. __might_sleep_precision() uses a precision specifier in the
>> printk format, which specifies the length of a string; a string
>> doesn't need to be a null-terminated.
>> 
>> Modify __might_sleep() to call __might_sleep_precision() but the
>> impact should be negligible. strlen() isn't called in a normal case;
>> it's called only when printing the error (sleeping function called
>> from invalid context).
>> 
>> Note that Location::file() providing a null-terminated string for
>> better C interoperability is under discussion [1].
> 
> Urgh :/

Yeah... so not acceptable?

Then I switch to the implementation with Rust macros, which gives a
null terminated string.

Re: [PATCH v9 1/8] sched/core: Add __might_sleep_precision()

[Boqun Feng]

On Thu, Jan 30, 2025 at 08:56:44AM +0900, FUJITA Tomonori wrote:
> On Tue, 28 Jan 2025 12:37:38 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Sat, Jan 25, 2025 at 07:18:46PM +0900, FUJITA Tomonori wrote:
> >> Add __might_sleep_precision(), Rust friendly version of
> >> __might_sleep(), which takes a pointer to a string with the length
> >> instead of a null-terminated string.
> >> 
> >> Rust's core::panic::Location::file(), which gives the file name of a
> >> caller, doesn't provide a null-terminated
> >> string. __might_sleep_precision() uses a precision specifier in the
> >> printk format, which specifies the length of a string; a string
> >> doesn't need to be a null-terminated.
> >> 
> >> Modify __might_sleep() to call __might_sleep_precision() but the
> >> impact should be negligible. strlen() isn't called in a normal case;
> >> it's called only when printing the error (sleeping function called
> >> from invalid context).
> >> 
> >> Note that Location::file() providing a null-terminated string for
> >> better C interoperability is under discussion [1].
> > 
> > Urgh :/
> 
> Yeah... so not acceptable?
> 

I would like to see some concrete and technical reasons for why it's not
acceptable ;-) I'm not sure whether Peter was against this patch or just
not happy about Location::file() providing a null-terminated string is a
WIP.

To me, null-terminated string literals don't provide much benefits
other than you can pass it via only one pointer value, the cost is that
you will always need to calculate the length of the string when needed,
so hard to say it's a straightforward win.

Regards,
Boqun

> Then I switch to the implementation with Rust macros, which gives a
> null terminated string.

Re: [PATCH v9 1/8] sched/core: Add __might_sleep_precision()

[Peter Zijlstra]

On Wed, Jan 29, 2025 at 05:14:54PM -0800, Boqun Feng wrote:
> On Thu, Jan 30, 2025 at 08:56:44AM +0900, FUJITA Tomonori wrote:
> > On Tue, 28 Jan 2025 12:37:38 +0100
> > Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > On Sat, Jan 25, 2025 at 07:18:46PM +0900, FUJITA Tomonori wrote:
> > >> Add __might_sleep_precision(), Rust friendly version of
> > >> __might_sleep(), which takes a pointer to a string with the length
> > >> instead of a null-terminated string.
> > >> 
> > >> Rust's core::panic::Location::file(), which gives the file name of a
> > >> caller, doesn't provide a null-terminated
> > >> string. __might_sleep_precision() uses a precision specifier in the
> > >> printk format, which specifies the length of a string; a string
> > >> doesn't need to be a null-terminated.
> > >> 
> > >> Modify __might_sleep() to call __might_sleep_precision() but the
> > >> impact should be negligible. strlen() isn't called in a normal case;
> > >> it's called only when printing the error (sleeping function called
> > >> from invalid context).
> > >> 
> > >> Note that Location::file() providing a null-terminated string for
> > >> better C interoperability is under discussion [1].
> > > 
> > > Urgh :/
> > 
> > Yeah... so not acceptable?

Just frustrated we 'need' more ugly to deal with Rust being stupid.

> I would like to see some concrete and technical reasons for why it's not
> acceptable ;-) I'm not sure whether Peter was against this patch or just
> not happy about Location::file() providing a null-terminated string is a
> WIP.

The latter.

I just hate on Rust for being designed by a bunch of C haters, not
wanting to acknowledge the whole frigging world runs on C and they
*have* to deal with interoperability.

That got us a whole pile of ugly including this.

[PATCH v9 2/8] rust: time: Add PartialEq/Eq/PartialOrd/Ord trait to Ktime

[FUJITA Tomonori]

Add PartialEq/Eq/PartialOrd/Ord trait to Ktime so two Ktime instances
can be compared to determine whether a timeout is met or not.

Use the derive implements; we directly touch C's ktime_t rather than
using the C's accessors because it is more efficient and we already do
in the existing code (Ktime::sub).

Reviewed-by: Trevor Gross <tmgross@umich.edu>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 379c0f5772e5..48b71e6641ce 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -27,7 +27,7 @@ pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
 
 /// A Rust wrapper around a `ktime_t`.
 #[repr(transparent)]
-#[derive(Copy, Clone)]
+#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
 pub struct Ktime {
     inner: bindings::ktime_t,
 }
-- 
2.43.0

Re: [PATCH v9 2/8] rust: time: Add PartialEq/Eq/PartialOrd/Ord trait to Ktime

[Fiona Behrens]

On 25 Jan 2025, at 11:18, FUJITA Tomonori wrote:

> Add PartialEq/Eq/PartialOrd/Ord trait to Ktime so two Ktime instances
> can be compared to determine whether a timeout is met or not.
>
> Use the derive implements; we directly touch C's ktime_t rather than
> using the C's accessors because it is more efficient and we already do
> in the existing code (Ktime::sub).
>
> Reviewed-by: Trevor Gross <tmgross@umich.edu>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Gary Guo <gary@garyguo.net>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Fiona Behrens <me@kloenk.dev>

[PATCH v9 3/8] rust: time: Introduce Delta type

[FUJITA Tomonori]

Introduce a type representing a span of time. Define our own type
because `core::time::Duration` is large and could panic during
creation.

time::Ktime could be also used for time duration but timestamp and
timedelta are different so better to use a new type.

i64 is used instead of u64 to represent a span of time; some C drivers
uses negative Deltas and i64 is more compatible with Ktime using i64
too (e.g., ktime_[us|ms]_delta() APIs return i64 so we create Delta
object without type conversion.

i64 is used instead of bindings::ktime_t because when the ktime_t
type is used as timestamp, it represents values from 0 to
KTIME_MAX, which is different from Delta.

as_millis() method isn't used in this patchset. It's planned to be
used in Binder driver.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time.rs | 88 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 48b71e6641ce..622cd01e24d7 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -8,9 +8,15 @@
 //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
 //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
 
+/// The number of nanoseconds per microsecond.
+pub const NSEC_PER_USEC: i64 = bindings::NSEC_PER_USEC as i64;
+
 /// The number of nanoseconds per millisecond.
 pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
 
+/// The number of nanoseconds per second.
+pub const NSEC_PER_SEC: i64 = bindings::NSEC_PER_SEC as i64;
+
 /// The time unit of Linux kernel. One jiffy equals (1/HZ) second.
 pub type Jiffies = crate::ffi::c_ulong;
 
@@ -81,3 +87,85 @@ fn sub(self, other: Ktime) -> Ktime {
         }
     }
 }
+
+/// A span of time.
+///
+/// This struct represents a span of time, with its value stored as nanoseconds.
+/// The value can represent any valid i64 value, including negative, zero, and
+/// positive numbers.
+#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
+pub struct Delta {
+    nanos: i64,
+}
+
+impl Delta {
+    /// A span of time equal to zero.
+    pub const ZERO: Self = Self { nanos: 0 };
+
+    /// Create a new [`Delta`] from a number of microseconds.
+    ///
+    /// The `micros` can range from -9_223_372_036_854_775 to 9_223_372_036_854_775.
+    /// If `micros` is outside this range, `i64::MIN` is used for negative values,
+    /// and `i64::MAX` is used for positive values due to saturation.
+    #[inline]
+    pub const fn from_micros(micros: i64) -> Self {
+        Self {
+            nanos: micros.saturating_mul(NSEC_PER_USEC),
+        }
+    }
+
+    /// Create a new [`Delta`] from a number of milliseconds.
+    ///
+    /// The `millis` can range from -9_223_372_036_854 to 9_223_372_036_854.
+    /// If `millis` is outside this range, `i64::MIN` is used for negative values,
+    /// and `i64::MAX` is used for positive values due to saturation.
+    #[inline]
+    pub const fn from_millis(millis: i64) -> Self {
+        Self {
+            nanos: millis.saturating_mul(NSEC_PER_MSEC),
+        }
+    }
+
+    /// Create a new [`Delta`] from a number of seconds.
+    ///
+    /// The `secs` can range from -9_223_372_036 to 9_223_372_036.
+    /// If `secs` is outside this range, `i64::MIN` is used for negative values,
+    /// and `i64::MAX` is used for positive values due to saturation.
+    #[inline]
+    pub const fn from_secs(secs: i64) -> Self {
+        Self {
+            nanos: secs.saturating_mul(NSEC_PER_SEC),
+        }
+    }
+
+    /// Return `true` if the [`Delta`] spans no time.
+    #[inline]
+    pub fn is_zero(self) -> bool {
+        self.as_nanos() == 0
+    }
+
+    /// Return `true` if the [`Delta`] spans a negative amount of time.
+    #[inline]
+    pub fn is_negative(self) -> bool {
+        self.as_nanos() < 0
+    }
+
+    /// Return the number of nanoseconds in the [`Delta`].
+    #[inline]
+    pub const fn as_nanos(self) -> i64 {
+        self.nanos
+    }
+
+    /// Return the smallest number of microseconds greater than or equal
+    /// to the value in the [`Delta`].
+    #[inline]
+    pub const fn as_micros_ceil(self) -> i64 {
+        self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
+    }
+
+    /// Return the number of milliseconds in the [`Delta`].
+    #[inline]
+    pub const fn as_millis(self) -> i64 {
+        self.as_nanos() / NSEC_PER_MSEC
+    }
+}
-- 
2.43.0

Re: [PATCH v9 3/8] rust: time: Introduce Delta type

[Gary Guo]

On Sat, 25 Jan 2025 19:18:48 +0900
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> Introduce a type representing a span of time. Define our own type
> because `core::time::Duration` is large and could panic during
> creation.
> 
> time::Ktime could be also used for time duration but timestamp and
> timedelta are different so better to use a new type.
> 
> i64 is used instead of u64 to represent a span of time; some C drivers
> uses negative Deltas and i64 is more compatible with Ktime using i64
> too (e.g., ktime_[us|ms]_delta() APIs return i64 so we create Delta
> object without type conversion.
> 
> i64 is used instead of bindings::ktime_t because when the ktime_t
> type is used as timestamp, it represents values from 0 to
> KTIME_MAX, which is different from Delta.
> 
> as_millis() method isn't used in this patchset. It's planned to be
> used in Binder driver.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/time.rs | 88 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 88 insertions(+)

Re: [PATCH v9 3/8] rust: time: Introduce Delta type

[Fiona Behrens]

On 25 Jan 2025, at 11:18, FUJITA Tomonori wrote:

> Introduce a type representing a span of time. Define our own type
> because `core::time::Duration` is large and could panic during
> creation.
>
> time::Ktime could be also used for time duration but timestamp and
> timedelta are different so better to use a new type.
>
> i64 is used instead of u64 to represent a span of time; some C drivers
> uses negative Deltas and i64 is more compatible with Ktime using i64
> too (e.g., ktime_[us|ms]_delta() APIs return i64 so we create Delta
> object without type conversion.
>
> i64 is used instead of bindings::ktime_t because when the ktime_t
> type is used as timestamp, it represents values from 0 to
> KTIME_MAX, which is different from Delta.
>
> as_millis() method isn't used in this patchset. It's planned to be
> used in Binder driver.

Thanks for adding millis, also will use that in my led patch :)

>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Fiona Behrens <me@kloenk.dev>

[PATCH v9 4/8] rust: time: Introduce Instant type

[FUJITA Tomonori]

Introduce a type representing a specific point in time. We could use
the Ktime type but C's ktime_t is used for both timestamp and
timedelta. To avoid confusion, introduce a new Instant type for
timestamp.

Rename Ktime to Instant and modify their methods for timestamp.

Implement the subtraction operator for Instant:

Delta = Instant A - Instant B

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time.rs | 77 +++++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 38 deletions(-)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 622cd01e24d7..d64a05a4f4d1 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -5,6 +5,22 @@
 //! This module contains the kernel APIs related to time and timers that
 //! have been ported or wrapped for usage by Rust code in the kernel.
 //!
+//! There are two types in this module:
+//!
+//! - The [`Instant`] type represents a specific point in time.
+//! - The [`Delta`] type represents a span of time.
+//!
+//! Note that the C side uses `ktime_t` type to represent both. However, timestamp
+//! and timedelta are different. To avoid confusion, we use two different types.
+//!
+//! A [`Instant`] object can be created by calling the [`Instant::now()`] function.
+//! It represents a point in time at which the object was created.
+//! By calling the [`Instant::elapsed()`] method, a [`Delta`] object representing
+//! the elapsed time can be created. The [`Delta`] object can also be created
+//! by subtracting two [`Instant`] objects.
+//!
+//! A [`Delta`] type supports methods to retrieve the duration in various units.
+//!
 //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
 //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
 
@@ -31,59 +47,44 @@ pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
     unsafe { bindings::__msecs_to_jiffies(msecs) }
 }
 
-/// A Rust wrapper around a `ktime_t`.
+/// A specific point in time.
+///
+/// # Invariants
+///
+/// The `inner` value is in the range from 0 to `KTIME_MAX`.
 #[repr(transparent)]
 #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
-pub struct Ktime {
+pub struct Instant {
     inner: bindings::ktime_t,
 }
 
-impl Ktime {
-    /// Create a `Ktime` from a raw `ktime_t`.
-    #[inline]
-    pub fn from_raw(inner: bindings::ktime_t) -> Self {
-        Self { inner }
-    }
-
+impl Instant {
     /// Get the current time using `CLOCK_MONOTONIC`.
     #[inline]
-    pub fn ktime_get() -> Self {
-        // SAFETY: It is always safe to call `ktime_get` outside of NMI context.
-        Self::from_raw(unsafe { bindings::ktime_get() })
-    }
-
-    /// Divide the number of nanoseconds by a compile-time constant.
-    #[inline]
-    fn divns_constant<const DIV: i64>(self) -> i64 {
-        self.to_ns() / DIV
-    }
-
-    /// Returns the number of nanoseconds.
-    #[inline]
-    pub fn to_ns(self) -> i64 {
-        self.inner
+    pub fn now() -> Self {
+        // INVARIANT: The `ktime_get()` function returns a value in the range
+        // from 0 to `KTIME_MAX`.
+        Self {
+            // SAFETY: It is always safe to call `ktime_get()` outside of NMI context.
+            inner: unsafe { bindings::ktime_get() },
+        }
     }
 
-    /// Returns the number of milliseconds.
+    /// Return the amount of time elapsed since the [`Instant`].
     #[inline]
-    pub fn to_ms(self) -> i64 {
-        self.divns_constant::<NSEC_PER_MSEC>()
+    pub fn elapsed(&self) -> Delta {
+        Self::now() - *self
     }
 }
 
-/// Returns the number of milliseconds between two ktimes.
-#[inline]
-pub fn ktime_ms_delta(later: Ktime, earlier: Ktime) -> i64 {
-    (later - earlier).to_ms()
-}
-
-impl core::ops::Sub for Ktime {
-    type Output = Ktime;
+impl core::ops::Sub for Instant {
+    type Output = Delta;
 
+    // By the type invariant, it never overflows.
     #[inline]
-    fn sub(self, other: Ktime) -> Ktime {
-        Self {
-            inner: self.inner - other.inner,
+    fn sub(self, other: Instant) -> Delta {
+        Delta {
+            nanos: self.inner - other.inner,
         }
     }
 }
-- 
2.43.0

Re: [PATCH v9 4/8] rust: time: Introduce Instant type

[Gary Guo]

On Sat, 25 Jan 2025 19:18:49 +0900
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> Introduce a type representing a specific point in time. We could use
> the Ktime type but C's ktime_t is used for both timestamp and
> timedelta. To avoid confusion, introduce a new Instant type for
> timestamp.
> 
> Rename Ktime to Instant and modify their methods for timestamp.
> 
> Implement the subtraction operator for Instant:
> 
> Delta = Instant A - Instant B
> 
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/time.rs | 77 +++++++++++++++++++++++----------------------
>  1 file changed, 39 insertions(+), 38 deletions(-)

Re: [PATCH v9 4/8] rust: time: Introduce Instant type

[Fiona Behrens]

On 25 Jan 2025, at 11:18, FUJITA Tomonori wrote:

> Introduce a type representing a specific point in time. We could use
> the Ktime type but C's ktime_t is used for both timestamp and
> timedelta. To avoid confusion, introduce a new Instant type for
> timestamp.
>
> Rename Ktime to Instant and modify their methods for timestamp.
>
> Implement the subtraction operator for Instant:
>
> Delta = Instant A - Instant B
>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Fiona Behrens <me@kloenk.dev>

[PATCH v9 5/8] rust: time: Add wrapper for fsleep() function

[FUJITA Tomonori]

Add a wrapper for fsleep(), flexible sleep functions in
include/linux/delay.h which typically deals with hardware delays.

The kernel supports several sleep functions to handle various lengths
of delay. This adds fsleep(), automatically chooses the best sleep
method based on a duration.

sleep functions including fsleep() belongs to TIMERS, not
TIMEKEEPING. They are maintained separately. rust/kernel/time.rs is an
abstraction for TIMEKEEPING. To make Rust abstractions match the C
side, add rust/kernel/time/delay.rs for this wrapper.

fsleep() can only be used in a nonatomic context. This requirement is
not checked by these abstractions, but it is intended that klint [1]
or a similar tool will be used to check it in the future.

Link: https://rust-for-linux.com/klint [1]
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers/helpers.c    |  1 +
 rust/helpers/time.c       |  8 +++++++
 rust/kernel/time.rs       |  2 ++
 rust/kernel/time/delay.rs | 49 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 60 insertions(+)
 create mode 100644 rust/helpers/time.c
 create mode 100644 rust/kernel/time/delay.rs

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index dcf827a61b52..d16aeda7a558 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -26,6 +26,7 @@
 #include "slab.c"
 #include "spinlock.c"
 #include "task.c"
+#include "time.c"
 #include "uaccess.c"
 #include "vmalloc.c"
 #include "wait.c"
diff --git a/rust/helpers/time.c b/rust/helpers/time.c
new file mode 100644
index 000000000000..7ae64ad8141d
--- /dev/null
+++ b/rust/helpers/time.c
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/delay.h>
+
+void rust_helper_fsleep(unsigned long usecs)
+{
+	fsleep(usecs);
+}
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index d64a05a4f4d1..eeb0f6a7e5d4 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -24,6 +24,8 @@
 //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
 //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
 
+pub mod delay;
+
 /// The number of nanoseconds per microsecond.
 pub const NSEC_PER_USEC: i64 = bindings::NSEC_PER_USEC as i64;
 
diff --git a/rust/kernel/time/delay.rs b/rust/kernel/time/delay.rs
new file mode 100644
index 000000000000..02b8731433c7
--- /dev/null
+++ b/rust/kernel/time/delay.rs
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Delay and sleep primitives.
+//!
+//! This module contains the kernel APIs related to delay and sleep that
+//! have been ported or wrapped for usage by Rust code in the kernel.
+//!
+//! C header: [`include/linux/delay.h`](srctree/include/linux/delay.h).
+
+use super::Delta;
+use crate::ffi::c_ulong;
+
+/// Sleeps for a given duration at least.
+///
+/// Equivalent to the C side [`fsleep()`], flexible sleep function,
+/// which automatically chooses the best sleep method based on a duration.
+///
+/// `delta` must be within `[0, i32::MAX]` microseconds;
+/// otherwise, it is erroneous behavior. That is, it is considered a bug
+/// to call this function with an out-of-range value, in which case the function
+/// will sleep for at least the maximum value in the range and may warn
+/// in the future.
+///
+/// The behavior above differs from the C side [`fsleep()`] for which out-of-range
+/// values mean "infinite timeout" instead.
+///
+/// This function can only be used in a nonatomic context.
+///
+/// [`fsleep`]: https://docs.kernel.org/timers/delay_sleep_functions.html#c.fsleep
+pub fn fsleep(delta: Delta) {
+    // The maximum value is set to `i32::MAX` microseconds to prevent integer
+    // overflow inside fsleep, which could lead to unintentional infinite sleep.
+    const MAX_DELTA: Delta = Delta::from_micros(i32::MAX as i64);
+
+    let delta = if (Delta::ZERO..=MAX_DELTA).contains(&delta) {
+        delta
+    } else {
+        // TODO: Add WARN_ONCE() when it's supported.
+        MAX_DELTA
+    };
+
+    // SAFETY: It is always safe to call `fsleep()` with any duration.
+    unsafe {
+        // Convert the duration to microseconds and round up to preserve
+        // the guarantee; `fsleep()` sleeps for at least the provided duration,
+        // but that it may sleep for longer under some circumstances.
+        bindings::fsleep(delta.as_micros_ceil() as c_ulong)
+    }
+}
-- 
2.43.0

Re: [PATCH v9 5/8] rust: time: Add wrapper for fsleep() function

[Gary Guo]

On Sat, 25 Jan 2025 19:18:50 +0900
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> Add a wrapper for fsleep(), flexible sleep functions in
> include/linux/delay.h which typically deals with hardware delays.
> 
> The kernel supports several sleep functions to handle various lengths
> of delay. This adds fsleep(), automatically chooses the best sleep
> method based on a duration.
> 
> sleep functions including fsleep() belongs to TIMERS, not
> TIMEKEEPING. They are maintained separately. rust/kernel/time.rs is an
> abstraction for TIMEKEEPING. To make Rust abstractions match the C
> side, add rust/kernel/time/delay.rs for this wrapper.
> 
> fsleep() can only be used in a nonatomic context. This requirement is
> not checked by these abstractions, but it is intended that klint [1]
> or a similar tool will be used to check it in the future.
> 
> Link: https://rust-for-linux.com/klint [1]
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/helpers/helpers.c    |  1 +
>  rust/helpers/time.c       |  8 +++++++
>  rust/kernel/time.rs       |  2 ++
>  rust/kernel/time/delay.rs | 49 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 60 insertions(+)
>  create mode 100644 rust/helpers/time.c
>  create mode 100644 rust/kernel/time/delay.rs
> 
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index dcf827a61b52..d16aeda7a558 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -26,6 +26,7 @@
>  #include "slab.c"
>  #include "spinlock.c"
>  #include "task.c"
> +#include "time.c"
>  #include "uaccess.c"
>  #include "vmalloc.c"
>  #include "wait.c"
> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
> new file mode 100644
> index 000000000000..7ae64ad8141d
> --- /dev/null
> +++ b/rust/helpers/time.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/delay.h>
> +
> +void rust_helper_fsleep(unsigned long usecs)
> +{
> +	fsleep(usecs);
> +}
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index d64a05a4f4d1..eeb0f6a7e5d4 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -24,6 +24,8 @@
>  //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
>  //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
>  
> +pub mod delay;
> +
>  /// The number of nanoseconds per microsecond.
>  pub const NSEC_PER_USEC: i64 = bindings::NSEC_PER_USEC as i64;
>  
> diff --git a/rust/kernel/time/delay.rs b/rust/kernel/time/delay.rs
> new file mode 100644
> index 000000000000..02b8731433c7
> --- /dev/null
> +++ b/rust/kernel/time/delay.rs
> @@ -0,0 +1,49 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Delay and sleep primitives.
> +//!
> +//! This module contains the kernel APIs related to delay and sleep that
> +//! have been ported or wrapped for usage by Rust code in the kernel.
> +//!
> +//! C header: [`include/linux/delay.h`](srctree/include/linux/delay.h).
> +
> +use super::Delta;
> +use crate::ffi::c_ulong;
> +
> +/// Sleeps for a given duration at least.
> +///
> +/// Equivalent to the C side [`fsleep()`], flexible sleep function,
> +/// which automatically chooses the best sleep method based on a duration.
> +///
> +/// `delta` must be within `[0, i32::MAX]` microseconds;
> +/// otherwise, it is erroneous behavior. That is, it is considered a bug
> +/// to call this function with an out-of-range value, in which case the function
> +/// will sleep for at least the maximum value in the range and may warn
> +/// in the future.
> +///
> +/// The behavior above differs from the C side [`fsleep()`] for which out-of-range
> +/// values mean "infinite timeout" instead.
> +///
> +/// This function can only be used in a nonatomic context.
> +///
> +/// [`fsleep`]: https://docs.kernel.org/timers/delay_sleep_functions.html#c.fsleep
> +pub fn fsleep(delta: Delta) {
> +    // The maximum value is set to `i32::MAX` microseconds to prevent integer
> +    // overflow inside fsleep, which could lead to unintentional infinite sleep.
> +    const MAX_DELTA: Delta = Delta::from_micros(i32::MAX as i64);
> +
> +    let delta = if (Delta::ZERO..=MAX_DELTA).contains(&delta) {
> +        delta
> +    } else {
> +        // TODO: Add WARN_ONCE() when it's supported.
> +        MAX_DELTA
> +    };
> +
> +    // SAFETY: It is always safe to call `fsleep()` with any duration.
> +    unsafe {
> +        // Convert the duration to microseconds and round up to preserve
> +        // the guarantee; `fsleep()` sleeps for at least the provided duration,
> +        // but that it may sleep for longer under some circumstances.
> +        bindings::fsleep(delta.as_micros_ceil() as c_ulong)
> +    }
> +}

Re: [PATCH v9 5/8] rust: time: Add wrapper for fsleep() function

[Alice Ryhl]

On Sat, Jan 25, 2025 at 11:20 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Add a wrapper for fsleep(), flexible sleep functions in
> include/linux/delay.h which typically deals with hardware delays.
>
> The kernel supports several sleep functions to handle various lengths
> of delay. This adds fsleep(), automatically chooses the best sleep
> method based on a duration.
>
> sleep functions including fsleep() belongs to TIMERS, not
> TIMEKEEPING. They are maintained separately. rust/kernel/time.rs is an
> abstraction for TIMEKEEPING. To make Rust abstractions match the C
> side, add rust/kernel/time/delay.rs for this wrapper.
>
> fsleep() can only be used in a nonatomic context. This requirement is
> not checked by these abstractions, but it is intended that klint [1]
> or a similar tool will be used to check it in the future.
>
> Link: https://rust-for-linux.com/klint [1]
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v9 5/8] rust: time: Add wrapper for fsleep() function

[Fiona Behrens]

On 25 Jan 2025, at 11:18, FUJITA Tomonori wrote:

> Add a wrapper for fsleep(), flexible sleep functions in
> include/linux/delay.h which typically deals with hardware delays.
>
> The kernel supports several sleep functions to handle various lengths
> of delay. This adds fsleep(), automatically chooses the best sleep
> method based on a duration.
>
> sleep functions including fsleep() belongs to TIMERS, not
> TIMEKEEPING. They are maintained separately. rust/kernel/time.rs is an
> abstraction for TIMEKEEPING. To make Rust abstractions match the C
> side, add rust/kernel/time/delay.rs for this wrapper.
>
> fsleep() can only be used in a nonatomic context. This requirement is
> not checked by these abstractions, but it is intended that klint [1]
> or a similar tool will be used to check it in the future.
>
> Link: https://rust-for-linux.com/klint [1]
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

One question below, but fine with this as well

Reviewed-by: Fiona Behrens <me@kloenk.dev>

> ---
>  rust/helpers/helpers.c    |  1 +
>  rust/helpers/time.c       |  8 +++++++
>  rust/kernel/time.rs       |  2 ++
>  rust/kernel/time/delay.rs | 49 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 60 insertions(+)
>  create mode 100644 rust/helpers/time.c
>  create mode 100644 rust/kernel/time/delay.rs
>
(..)
> diff --git a/rust/kernel/time/delay.rs b/rust/kernel/time/delay.rs
> new file mode 100644
> index 000000000000..02b8731433c7
> --- /dev/null
> +++ b/rust/kernel/time/delay.rs
> @@ -0,0 +1,49 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Delay and sleep primitives.
> +//!
> +//! This module contains the kernel APIs related to delay and sleep that
> +//! have been ported or wrapped for usage by Rust code in the kernel.
> +//!
> +//! C header: [`include/linux/delay.h`](srctree/include/linux/delay.h).
> +
> +use super::Delta;
> +use crate::ffi::c_ulong;
> +
> +/// Sleeps for a given duration at least.
> +///
> +/// Equivalent to the C side [`fsleep()`], flexible sleep function,
> +/// which automatically chooses the best sleep method based on a duration.
> +///
> +/// `delta` must be within `[0, i32::MAX]` microseconds;
> +/// otherwise, it is erroneous behavior. That is, it is considered a bug
> +/// to call this function with an out-of-range value, in which case the function
> +/// will sleep for at least the maximum value in the range and may warn
> +/// in the future.
> +///
> +/// The behavior above differs from the C side [`fsleep()`] for which out-of-range
> +/// values mean "infinite timeout" instead.
> +///
> +/// This function can only be used in a nonatomic context.
> +///
> +/// [`fsleep`]: https://docs.kernel.org/timers/delay_sleep_functions.html#c.fsleep
> +pub fn fsleep(delta: Delta) {
> +    // The maximum value is set to `i32::MAX` microseconds to prevent integer
> +    // overflow inside fsleep, which could lead to unintentional infinite sleep.
> +    const MAX_DELTA: Delta = Delta::from_micros(i32::MAX as i64);
> +
> +    let delta = if (Delta::ZERO..=MAX_DELTA).contains(&delta) {
> +        delta
> +    } else {
> +        // TODO: Add WARN_ONCE() when it's supported.
> +        MAX_DELTA
> +    };

Did you try that with std::cmp::Ord you derived on Delta? This `.contains` looks a bit weird, maybe it also works with `delta <= MAX_DELTA`?

> +
> +    // SAFETY: It is always safe to call `fsleep()` with any duration.
> +    unsafe {
> +        // Convert the duration to microseconds and round up to preserve
> +        // the guarantee; `fsleep()` sleeps for at least the provided duration,
> +        // but that it may sleep for longer under some circumstances.
> +        bindings::fsleep(delta.as_micros_ceil() as c_ulong)
> +    }
> +}
> -- 
> 2.43.0

Re: [PATCH v9 5/8] rust: time: Add wrapper for fsleep() function

[FUJITA Tomonori]

On Tue, 28 Jan 2025 11:37:41 +0100
Fiona Behrens <me@kloenk.dev> wrote:

>> Add a wrapper for fsleep(), flexible sleep functions in
>> include/linux/delay.h which typically deals with hardware delays.
>>
>> The kernel supports several sleep functions to handle various lengths
>> of delay. This adds fsleep(), automatically chooses the best sleep
>> method based on a duration.
>>
>> sleep functions including fsleep() belongs to TIMERS, not
>> TIMEKEEPING. They are maintained separately. rust/kernel/time.rs is an
>> abstraction for TIMEKEEPING. To make Rust abstractions match the C
>> side, add rust/kernel/time/delay.rs for this wrapper.
>>
>> fsleep() can only be used in a nonatomic context. This requirement is
>> not checked by these abstractions, but it is intended that klint [1]
>> or a similar tool will be used to check it in the future.
>>
>> Link: https://rust-for-linux.com/klint [1]
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> One question below, but fine with this as well
> 
> Reviewed-by: Fiona Behrens <me@kloenk.dev>

Thanks!

>> +pub fn fsleep(delta: Delta) {
>> +    // The maximum value is set to `i32::MAX` microseconds to prevent integer
>> +    // overflow inside fsleep, which could lead to unintentional infinite sleep.
>> +    const MAX_DELTA: Delta = Delta::from_micros(i32::MAX as i64);
>> +
>> +    let delta = if (Delta::ZERO..=MAX_DELTA).contains(&delta) {
>> +        delta
>> +    } else {
>> +        // TODO: Add WARN_ONCE() when it's supported.
>> +        MAX_DELTA
>> +    };
> 
> Did you try that with std::cmp::Ord you derived on Delta? This `.contains` looks a bit weird, maybe it also works with `delta <= MAX_DELTA`?

Do you mean using either if or match?

The link to the discussion that led to the current implementation is:

https://lore.kernel.org/lkml/CANiq72nNsmuQz1mEx2ov8SXj_UAEURDZFtLotf4qP2pf+r97eQ@mail.gmail.com/#t

[PATCH v9 6/8] MAINTAINERS: rust: Add TIMEKEEPING and TIMER abstractions

[FUJITA Tomonori]

Add Rust TIMEKEEPING and TIMER abstractions to the maintainers entry
respectively.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8e047e20fbd8..faa4081e9e76 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10307,6 +10307,7 @@ F:	kernel/time/sleep_timeout.c
 F:	kernel/time/timer.c
 F:	kernel/time/timer_list.c
 F:	kernel/time/timer_migration.*
+F:	rust/kernel/time/delay.rs
 F:	tools/testing/selftests/timers/
 
 HIGH-SPEED SCC DRIVER FOR AX.25
@@ -23741,6 +23742,7 @@ F:	kernel/time/timeconv.c
 F:	kernel/time/timecounter.c
 F:	kernel/time/timekeeping*
 F:	kernel/time/time_test.c
+F:	rust/kernel/time.rs
 F:	tools/testing/selftests/timers/
 
 TIPC NETWORK LAYER
-- 
2.43.0

[PATCH v9 7/8] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

Add read_poll_timeout functions which poll periodically until a
condition is met or a timeout is reached.

The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
macro and a simple wrapper for Rust doesn't work. So this implements
the same functionality in Rust.

The C version uses usleep_range() while the Rust version uses
fsleep(), which uses the best sleep method so it works with spans that
usleep_range() doesn't work nicely with.

Unlike the C version, __might_sleep() is used instead of might_sleep()
to show proper debug info; the file name and line
number. might_resched() could be added to match what the C version
does but this function works without it.

The sleep_before_read argument isn't supported since there is no user
for now. It's rarely used in the C version.

read_poll_timeout() can only be used in a nonatomic context. This
requirement is not checked by these abstractions, but it is intended
that klint [1] or a similar tool will be used to check it in the
future.

Link: https://rust-for-linux.com/klint [1]
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers/helpers.c |  1 +
 rust/helpers/kernel.c  | 13 +++++++
 rust/kernel/cpu.rs     | 13 +++++++
 rust/kernel/error.rs   |  1 +
 rust/kernel/io.rs      |  5 +++
 rust/kernel/io/poll.rs | 79 ++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |  2 ++
 7 files changed, 114 insertions(+)
 create mode 100644 rust/helpers/kernel.c
 create mode 100644 rust/kernel/cpu.rs
 create mode 100644 rust/kernel/io.rs
 create mode 100644 rust/kernel/io/poll.rs

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index d16aeda7a558..7ab71a6d4603 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -13,6 +13,7 @@
 #include "build_bug.c"
 #include "cred.c"
 #include "err.c"
+#include "kernel.c"
 #include "fs.c"
 #include "jump_label.c"
 #include "kunit.c"
diff --git a/rust/helpers/kernel.c b/rust/helpers/kernel.c
new file mode 100644
index 000000000000..9dff28f4618e
--- /dev/null
+++ b/rust/helpers/kernel.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/kernel.h>
+
+void rust_helper_cpu_relax(void)
+{
+	cpu_relax();
+}
+
+void rust_helper___might_sleep_precision(const char *file, int len, int line)
+{
+	__might_sleep_precision(file, len, line);
+}
diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs
new file mode 100644
index 000000000000..eeeff4be84fa
--- /dev/null
+++ b/rust/kernel/cpu.rs
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Processor related primitives.
+//!
+//! C header: [`include/linux/processor.h`](srctree/include/linux/processor.h).
+
+/// Lower CPU power consumption or yield to a hyperthreaded twin processor.
+///
+/// It also happens to serve as a compiler barrier.
+pub fn cpu_relax() {
+    // SAFETY: FFI call.
+    unsafe { bindings::cpu_relax() }
+}
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index f6ecf09cb65f..8858eb13b3df 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -64,6 +64,7 @@ macro_rules! declare_err {
     declare_err!(EPIPE, "Broken pipe.");
     declare_err!(EDOM, "Math argument out of domain of func.");
     declare_err!(ERANGE, "Math result not representable.");
+    declare_err!(ETIMEDOUT, "Connection timed out.");
     declare_err!(ERESTARTSYS, "Restart the system call.");
     declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
     declare_err!(ERESTARTNOHAND, "Restart if no handler.");
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
new file mode 100644
index 000000000000..033f3c4e4adf
--- /dev/null
+++ b/rust/kernel/io.rs
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Input and Output.
+
+pub mod poll;
diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
new file mode 100644
index 000000000000..7a503cf643a1
--- /dev/null
+++ b/rust/kernel/io/poll.rs
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! IO polling.
+//!
+//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
+
+use crate::{
+    cpu::cpu_relax,
+    error::{code::*, Result},
+    time::{delay::fsleep, Delta, Instant},
+};
+
+use core::panic::Location;
+
+/// Polls periodically until a condition is met or a timeout is reached.
+///
+/// Public but hidden since it should only be used from public macros.
+///
+/// ```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), Delta::from_micros(42));
+/// drop(g);
+///
+/// # Ok::<(), Error>(())
+/// ```
+#[track_caller]
+pub fn read_poll_timeout<Op, Cond, T: Copy>(
+    mut op: Op,
+    mut cond: Cond,
+    sleep_delta: Delta,
+    timeout_delta: Delta,
+) -> Result<T>
+where
+    Op: FnMut() -> Result<T>,
+    Cond: FnMut(&T) -> bool,
+{
+    let start = Instant::now();
+    let sleep = !sleep_delta.is_zero();
+    let timeout = !timeout_delta.is_zero();
+
+    if sleep {
+        might_sleep(Location::caller());
+    }
+
+    loop {
+        let val = op()?;
+        if cond(&val) {
+            // Unlike the C version, we immediately return.
+            // We know the condition is met so we don't need to check again.
+            return Ok(val);
+        }
+        if timeout && start.elapsed() > timeout_delta {
+            // Unlike the C version, we immediately return.
+            // We have just called `op()` so we don't need to call it again.
+            return Err(ETIMEDOUT);
+        }
+        if sleep {
+            fsleep(sleep_delta);
+        }
+        // fsleep() could be busy-wait loop so we always call cpu_relax().
+        cpu_relax();
+    }
+}
+
+fn might_sleep(loc: &Location<'_>) {
+    // SAFETY: FFI call.
+    unsafe {
+        crate::bindings::__might_sleep_precision(
+            loc.file().as_ptr().cast(),
+            loc.file().len() as i32,
+            loc.line() as i32,
+        )
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 545d1170ee63..c477701b2efa 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -35,6 +35,7 @@
 pub mod block;
 #[doc(hidden)]
 pub mod build_assert;
+pub mod cpu;
 pub mod cred;
 pub mod device;
 pub mod error;
@@ -42,6 +43,7 @@
 pub mod firmware;
 pub mod fs;
 pub mod init;
+pub mod io;
 pub mod ioctl;
 pub mod jump_label;
 #[cfg(CONFIG_KUNIT)]
-- 
2.43.0

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

[Gary Guo]

On Sat, 25 Jan 2025 19:18:52 +0900
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> Add read_poll_timeout functions which poll periodically until a
> condition is met or a timeout is reached.
> 
> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
> macro and a simple wrapper for Rust doesn't work. So this implements
> the same functionality in Rust.
> 
> The C version uses usleep_range() while the Rust version uses
> fsleep(), which uses the best sleep method so it works with spans that
> usleep_range() doesn't work nicely with.
> 
> Unlike the C version, __might_sleep() is used instead of might_sleep()
> to show proper debug info; the file name and line
> number. might_resched() could be added to match what the C version
> does but this function works without it.
> 
> The sleep_before_read argument isn't supported since there is no user
> for now. It's rarely used in the C version.
> 
> read_poll_timeout() can only be used in a nonatomic context. This
> requirement is not checked by these abstractions, but it is intended
> that klint [1] or a similar tool will be used to check it in the
> future.
> 
> Link: https://rust-for-linux.com/klint [1]
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/helpers/helpers.c |  1 +
>  rust/helpers/kernel.c  | 13 +++++++
>  rust/kernel/cpu.rs     | 13 +++++++
>  rust/kernel/error.rs   |  1 +
>  rust/kernel/io.rs      |  5 +++
>  rust/kernel/io/poll.rs | 79 ++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs     |  2 ++
>  7 files changed, 114 insertions(+)
>  create mode 100644 rust/helpers/kernel.c
>  create mode 100644 rust/kernel/cpu.rs
>  create mode 100644 rust/kernel/io.rs
>  create mode 100644 rust/kernel/io/poll.rs
> 
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index d16aeda7a558..7ab71a6d4603 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -13,6 +13,7 @@
>  #include "build_bug.c"
>  #include "cred.c"
>  #include "err.c"
> +#include "kernel.c"
>  #include "fs.c"
>  #include "jump_label.c"
>  #include "kunit.c"
> diff --git a/rust/helpers/kernel.c b/rust/helpers/kernel.c
> new file mode 100644
> index 000000000000..9dff28f4618e
> --- /dev/null
> +++ b/rust/helpers/kernel.c
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/kernel.h>
> +
> +void rust_helper_cpu_relax(void)
> +{
> +	cpu_relax();
> +}
> +
> +void rust_helper___might_sleep_precision(const char *file, int len, int line)
> +{
> +	__might_sleep_precision(file, len, line);
> +}
> diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs
> new file mode 100644
> index 000000000000..eeeff4be84fa
> --- /dev/null
> +++ b/rust/kernel/cpu.rs
> @@ -0,0 +1,13 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Processor related primitives.
> +//!
> +//! C header: [`include/linux/processor.h`](srctree/include/linux/processor.h).
> +
> +/// Lower CPU power consumption or yield to a hyperthreaded twin processor.
> +///
> +/// It also happens to serve as a compiler barrier.
> +pub fn cpu_relax() {
> +    // SAFETY: FFI call.
> +    unsafe { bindings::cpu_relax() }
> +}
> diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
> index f6ecf09cb65f..8858eb13b3df 100644
> --- a/rust/kernel/error.rs
> +++ b/rust/kernel/error.rs
> @@ -64,6 +64,7 @@ macro_rules! declare_err {
>      declare_err!(EPIPE, "Broken pipe.");
>      declare_err!(EDOM, "Math argument out of domain of func.");
>      declare_err!(ERANGE, "Math result not representable.");
> +    declare_err!(ETIMEDOUT, "Connection timed out.");
>      declare_err!(ERESTARTSYS, "Restart the system call.");
>      declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
>      declare_err!(ERESTARTNOHAND, "Restart if no handler.");
> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
> new file mode 100644
> index 000000000000..033f3c4e4adf
> --- /dev/null
> +++ b/rust/kernel/io.rs
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Input and Output.
> +
> +pub mod poll;
> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
> new file mode 100644
> index 000000000000..7a503cf643a1
> --- /dev/null
> +++ b/rust/kernel/io/poll.rs
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! IO polling.
> +//!
> +//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
> +
> +use crate::{
> +    cpu::cpu_relax,
> +    error::{code::*, Result},
> +    time::{delay::fsleep, Delta, Instant},
> +};
> +
> +use core::panic::Location;
> +
> +/// Polls periodically until a condition is met or a timeout is reached.
> +///
> +/// Public but hidden since it should only be used from public macros.
> +///
> +/// ```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), Delta::from_micros(42));
> +/// drop(g);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +#[track_caller]
> +pub fn read_poll_timeout<Op, Cond, T: Copy>(
> +    mut op: Op,
> +    mut cond: Cond,
> +    sleep_delta: Delta,
> +    timeout_delta: Delta,
> +) -> Result<T>
> +where
> +    Op: FnMut() -> Result<T>,
> +    Cond: FnMut(&T) -> bool,
> +{
> +    let start = Instant::now();
> +    let sleep = !sleep_delta.is_zero();
> +    let timeout = !timeout_delta.is_zero();
> +
> +    if sleep {
> +        might_sleep(Location::caller());
> +    }
> +
> +    loop {
> +        let val = op()?;
> +        if cond(&val) {
> +            // Unlike the C version, we immediately return.
> +            // We know the condition is met so we don't need to check again.
> +            return Ok(val);
> +        }
> +        if timeout && start.elapsed() > timeout_delta {

Re-reading this again I wonder if this is the desired behaviour? Maybe
a timeout of 0 should mean check-once instead of no timeout. The
special-casing of 0 makes sense in C but in Rust we should use `None`
to mean it instead?

For `sleep_delta` it's fine to not use `Option`.

> +            // Unlike the C version, we immediately return.
> +            // We have just called `op()` so we don't need to call it again.
> +            return Err(ETIMEDOUT);
> +        }
> +        if sleep {
> +            fsleep(sleep_delta);
> +        }
> +        // fsleep() could be busy-wait loop so we always call cpu_relax().
> +        cpu_relax();
> +    }
> +}
> +
> +fn might_sleep(loc: &Location<'_>) {
> +    // SAFETY: FFI call.
> +    unsafe {
> +        crate::bindings::__might_sleep_precision(
> +            loc.file().as_ptr().cast(),
> +            loc.file().len() as i32,
> +            loc.line() as i32,
> +        )
> +    }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 545d1170ee63..c477701b2efa 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -35,6 +35,7 @@
>  pub mod block;
>  #[doc(hidden)]
>  pub mod build_assert;
> +pub mod cpu;
>  pub mod cred;
>  pub mod device;
>  pub mod error;
> @@ -42,6 +43,7 @@
>  pub mod firmware;
>  pub mod fs;
>  pub mod init;
> +pub mod io;
>  pub mod ioctl;
>  pub mod jump_label;
>  #[cfg(CONFIG_KUNIT)]

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

[FUJITA Tomonori]

On Mon, 27 Jan 2025 11:46:46 +0800
Gary Guo <gary@garyguo.net> wrote:

>> +#[track_caller]
>> +pub fn read_poll_timeout<Op, Cond, T: Copy>(
>> +    mut op: Op,
>> +    mut cond: Cond,
>> +    sleep_delta: Delta,
>> +    timeout_delta: Delta,
>> +) -> Result<T>
>> +where
>> +    Op: FnMut() -> Result<T>,
>> +    Cond: FnMut(&T) -> bool,
>> +{
>> +    let start = Instant::now();
>> +    let sleep = !sleep_delta.is_zero();
>> +    let timeout = !timeout_delta.is_zero();
>> +
>> +    if sleep {
>> +        might_sleep(Location::caller());
>> +    }
>> +
>> +    loop {
>> +        let val = op()?;
>> +        if cond(&val) {
>> +            // Unlike the C version, we immediately return.
>> +            // We know the condition is met so we don't need to check again.
>> +            return Ok(val);
>> +        }
>> +        if timeout && start.elapsed() > timeout_delta {
> 
> Re-reading this again I wonder if this is the desired behaviour? Maybe
> a timeout of 0 should mean check-once instead of no timeout. The
> special-casing of 0 makes sense in C but in Rust we should use `None`
> to mean it instead?

It's the behavior of the C version; the comment of this function says:

* @timeout_us: Timeout in us, 0 means never timeout

You meant that waiting for a condition without a timeout is generally
a bad idea? If so, can we simply return EINVAL for zero Delta?

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

[Gary Guo]

On Mon, 27 Jan 2025 15:31:47 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> On Mon, 27 Jan 2025 11:46:46 +0800
> Gary Guo <gary@garyguo.net> wrote:
> 
> >> +#[track_caller]
> >> +pub fn read_poll_timeout<Op, Cond, T: Copy>(
> >> +    mut op: Op,
> >> +    mut cond: Cond,
> >> +    sleep_delta: Delta,
> >> +    timeout_delta: Delta,
> >> +) -> Result<T>
> >> +where
> >> +    Op: FnMut() -> Result<T>,
> >> +    Cond: FnMut(&T) -> bool,
> >> +{
> >> +    let start = Instant::now();
> >> +    let sleep = !sleep_delta.is_zero();
> >> +    let timeout = !timeout_delta.is_zero();
> >> +
> >> +    if sleep {
> >> +        might_sleep(Location::caller());
> >> +    }
> >> +
> >> +    loop {
> >> +        let val = op()?;
> >> +        if cond(&val) {
> >> +            // Unlike the C version, we immediately return.
> >> +            // We know the condition is met so we don't need to check again.
> >> +            return Ok(val);
> >> +        }
> >> +        if timeout && start.elapsed() > timeout_delta {  
> > 
> > Re-reading this again I wonder if this is the desired behaviour? Maybe
> > a timeout of 0 should mean check-once instead of no timeout. The
> > special-casing of 0 makes sense in C but in Rust we should use `None`
> > to mean it instead?  
> 
> It's the behavior of the C version; the comment of this function says:
> 
> * @timeout_us: Timeout in us, 0 means never timeout
> 
> You meant that waiting for a condition without a timeout is generally
> a bad idea? If so, can we simply return EINVAL for zero Delta?
> 

No, I think we should still keep the ability to represent indefinite
wait (no timeout) but we should use `None` to represent this rather
than `Delta::ZERO`.

I know that we use 0 to mean indefinite wait in C, I am saying that
it's not the most intuitive way to represent in Rust.

Intuitively, a timeout of 0 should be closer to a timeout of 1 and thus
should mean "return with ETIMEDOUT immedidately" rather than "wait
forever".

In C since we don't have a very good sum type support, so we
special case 0 to be the special value to represent indefinite wait,
but I don't think we need to repeat this in Rust.

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

[FUJITA Tomonori]

On Tue, 28 Jan 2025 08:49:37 +0800
Gary Guo <gary@garyguo.net> wrote:

> On Mon, 27 Jan 2025 15:31:47 +0900 (JST)
> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> 
>> On Mon, 27 Jan 2025 11:46:46 +0800
>> Gary Guo <gary@garyguo.net> wrote:
>> 
>> >> +#[track_caller]
>> >> +pub fn read_poll_timeout<Op, Cond, T: Copy>(
>> >> +    mut op: Op,
>> >> +    mut cond: Cond,
>> >> +    sleep_delta: Delta,
>> >> +    timeout_delta: Delta,
>> >> +) -> Result<T>
>> >> +where
>> >> +    Op: FnMut() -> Result<T>,
>> >> +    Cond: FnMut(&T) -> bool,
>> >> +{
>> >> +    let start = Instant::now();
>> >> +    let sleep = !sleep_delta.is_zero();
>> >> +    let timeout = !timeout_delta.is_zero();
>> >> +
>> >> +    if sleep {
>> >> +        might_sleep(Location::caller());
>> >> +    }
>> >> +
>> >> +    loop {
>> >> +        let val = op()?;
>> >> +        if cond(&val) {
>> >> +            // Unlike the C version, we immediately return.
>> >> +            // We know the condition is met so we don't need to check again.
>> >> +            return Ok(val);
>> >> +        }
>> >> +        if timeout && start.elapsed() > timeout_delta {  
>> > 
>> > Re-reading this again I wonder if this is the desired behaviour? Maybe
>> > a timeout of 0 should mean check-once instead of no timeout. The
>> > special-casing of 0 makes sense in C but in Rust we should use `None`
>> > to mean it instead?  
>> 
>> It's the behavior of the C version; the comment of this function says:
>> 
>> * @timeout_us: Timeout in us, 0 means never timeout
>> 
>> You meant that waiting for a condition without a timeout is generally
>> a bad idea? If so, can we simply return EINVAL for zero Delta?
>> 
> 
> No, I think we should still keep the ability to represent indefinite
> wait (no timeout) but we should use `None` to represent this rather
> than `Delta::ZERO`.
> 
> I know that we use 0 to mean indefinite wait in C, I am saying that
> it's not the most intuitive way to represent in Rust.
> 
> Intuitively, a timeout of 0 should be closer to a timeout of 1 and thus
> should mean "return with ETIMEDOUT immedidately" rather than "wait
> forever".
> 
> In C since we don't have a very good sum type support, so we
> special case 0 to be the special value to represent indefinite wait,
> but I don't think we need to repeat this in Rust.

Understood, thanks. How about the following code?

+#[track_caller]
+pub fn read_poll_timeout<Op, Cond, T: Copy>(
+    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(Location::caller());
+    }
+
+    loop {
+        let val = op()?;
+        if cond(&val) {
+            // Unlike the C version, we immediately return.
+            // We know the condition is met so we don't need to check again.
+            return Ok(val);
+        }
+        if let Some(timeout_delta) = timeout_delta {
+            if start.elapsed() > timeout_delta {
+                // Unlike the C version, we immediately return.
+                // We have just called `op()` so we don't need to call it again.
+                return Err(ETIMEDOUT);
+            }
+        }
+        if sleep {
+            fsleep(sleep_delta);
+        }
+        // fsleep() could be busy-wait loop so we always call cpu_relax().
+        cpu_relax();
+    }
+}

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

[Fiona Behrens]

On 28 Jan 2025, at 7:29, FUJITA Tomonori wrote:

> On Tue, 28 Jan 2025 08:49:37 +0800
> Gary Guo <gary@garyguo.net> wrote:
>
>> On Mon, 27 Jan 2025 15:31:47 +0900 (JST)
>> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>>
>>> On Mon, 27 Jan 2025 11:46:46 +0800
>>> Gary Guo <gary@garyguo.net> wrote:
>>>
>>>>> +#[track_caller]
>>>>> +pub fn read_poll_timeout<Op, Cond, T: Copy>(
>>>>> +    mut op: Op,
>>>>> +    mut cond: Cond,
>>>>> +    sleep_delta: Delta,
>>>>> +    timeout_delta: Delta,
>>>>> +) -> Result<T>
>>>>> +where
>>>>> +    Op: FnMut() -> Result<T>,
>>>>> +    Cond: FnMut(&T) -> bool,
>>>>> +{
>>>>> +    let start = Instant::now();
>>>>> +    let sleep = !sleep_delta.is_zero();
>>>>> +    let timeout = !timeout_delta.is_zero();
>>>>> +
>>>>> +    if sleep {
>>>>> +        might_sleep(Location::caller());
>>>>> +    }
>>>>> +
>>>>> +    loop {
>>>>> +        let val = op()?;
>>>>> +        if cond(&val) {
>>>>> +            // Unlike the C version, we immediately return.
>>>>> +            // We know the condition is met so we don't need to check again.
>>>>> +            return Ok(val);
>>>>> +        }
>>>>> +        if timeout && start.elapsed() > timeout_delta {
>>>>
>>>> Re-reading this again I wonder if this is the desired behaviour? Maybe
>>>> a timeout of 0 should mean check-once instead of no timeout. The
>>>> special-casing of 0 makes sense in C but in Rust we should use `None`
>>>> to mean it instead?
>>>
>>> It's the behavior of the C version; the comment of this function says:
>>>
>>> * @timeout_us: Timeout in us, 0 means never timeout
>>>
>>> You meant that waiting for a condition without a timeout is generally
>>> a bad idea? If so, can we simply return EINVAL for zero Delta?
>>>
>>
>> No, I think we should still keep the ability to represent indefinite
>> wait (no timeout) but we should use `None` to represent this rather
>> than `Delta::ZERO`.
>>
>> I know that we use 0 to mean indefinite wait in C, I am saying that
>> it's not the most intuitive way to represent in Rust.
>>
>> Intuitively, a timeout of 0 should be closer to a timeout of 1 and thus
>> should mean "return with ETIMEDOUT immedidately" rather than "wait
>> forever".
>>
>> In C since we don't have a very good sum type support, so we
>> special case 0 to be the special value to represent indefinite wait,
>> but I don't think we need to repeat this in Rust.
>
> Understood, thanks. How about the following code?
>
> +#[track_caller]
> +pub fn read_poll_timeout<Op, Cond, T: Copy>(
> +    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(Location::caller());
> +    }
> +
> +    loop {
> +        let val = op()?;
> +        if cond(&val) {
> +            // Unlike the C version, we immediately return.
> +            // We know the condition is met so we don't need to check again.
> +            return Ok(val);
> +        }
> +        if let Some(timeout_delta) = timeout_delta {
> +            if start.elapsed() > timeout_delta {
> +                // Unlike the C version, we immediately return.
> +                // We have just called `op()` so we don't need to call it again.
> +                return Err(ETIMEDOUT);
> +            }
> +        }
> +        if sleep {
> +            fsleep(sleep_delta);
> +        }
> +        // fsleep() could be busy-wait loop so we always call cpu_relax().
> +        cpu_relax();
> +    }
> +}

I wonder if it makes sense to then switch `Delta` to wrap a  `NonZeroI64` and forbid deltas with 0 nanoseconds with that and use the niche optimization. Not sure if we make other apis horrible by that, but this would prevent deltas that encode no time passing.

Thanks,
Fiona

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

[FUJITA Tomonori]

On Tue, 28 Jan 2025 11:49:48 +0100
Fiona Behrens <me@kloenk.dev> wrote:

>> +#[track_caller]
>> +pub fn read_poll_timeout<Op, Cond, T: Copy>(
>> +    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(Location::caller());
>> +    }
>> +
>> +    loop {
>> +        let val = op()?;
>> +        if cond(&val) {
>> +            // Unlike the C version, we immediately return.
>> +            // We know the condition is met so we don't need to check again.
>> +            return Ok(val);
>> +        }
>> +        if let Some(timeout_delta) = timeout_delta {
>> +            if start.elapsed() > timeout_delta {
>> +                // Unlike the C version, we immediately return.
>> +                // We have just called `op()` so we don't need to call it again.
>> +                return Err(ETIMEDOUT);
>> +            }
>> +        }
>> +        if sleep {
>> +            fsleep(sleep_delta);
>> +        }
>> +        // fsleep() could be busy-wait loop so we always call cpu_relax().
>> +        cpu_relax();
>> +    }
>> +}
> 
> I wonder if it makes sense to then switch `Delta` to wrap a  `NonZeroI64` and forbid deltas with 0 nanoseconds with that and use the niche optimization. Not sure if we make other apis horrible by that, but this would prevent deltas that encode no time passing.

I think that there are valid use casese for a zero Delta type.

About this function, using zero Delta for sleep_delta means busy
polling. Sevaral drivers use the C version of this function in that
manner.

Using zero Delta for timeout_delta means checking a condition only
once.

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

[FUJITA Tomonori]

On Tue, 28 Jan 2025 15:29:57 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> On Tue, 28 Jan 2025 08:49:37 +0800
> Gary Guo <gary@garyguo.net> wrote:
> 
>> On Mon, 27 Jan 2025 15:31:47 +0900 (JST)
>> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>> 
>>> On Mon, 27 Jan 2025 11:46:46 +0800
>>> Gary Guo <gary@garyguo.net> wrote:
>>> 
>>> >> +#[track_caller]
>>> >> +pub fn read_poll_timeout<Op, Cond, T: Copy>(
>>> >> +    mut op: Op,
>>> >> +    mut cond: Cond,
>>> >> +    sleep_delta: Delta,
>>> >> +    timeout_delta: Delta,
>>> >> +) -> Result<T>
>>> >> +where
>>> >> +    Op: FnMut() -> Result<T>,
>>> >> +    Cond: FnMut(&T) -> bool,
>>> >> +{
>>> >> +    let start = Instant::now();
>>> >> +    let sleep = !sleep_delta.is_zero();
>>> >> +    let timeout = !timeout_delta.is_zero();
>>> >> +
>>> >> +    if sleep {
>>> >> +        might_sleep(Location::caller());
>>> >> +    }
>>> >> +
>>> >> +    loop {
>>> >> +        let val = op()?;
>>> >> +        if cond(&val) {
>>> >> +            // Unlike the C version, we immediately return.
>>> >> +            // We know the condition is met so we don't need to check again.
>>> >> +            return Ok(val);
>>> >> +        }
>>> >> +        if timeout && start.elapsed() > timeout_delta {  
>>> > 
>>> > Re-reading this again I wonder if this is the desired behaviour? Maybe
>>> > a timeout of 0 should mean check-once instead of no timeout. The
>>> > special-casing of 0 makes sense in C but in Rust we should use `None`
>>> > to mean it instead?  
>>> 
>>> It's the behavior of the C version; the comment of this function says:
>>> 
>>> * @timeout_us: Timeout in us, 0 means never timeout
>>> 
>>> You meant that waiting for a condition without a timeout is generally
>>> a bad idea? If so, can we simply return EINVAL for zero Delta?
>>> 
>> 
>> No, I think we should still keep the ability to represent indefinite
>> wait (no timeout) but we should use `None` to represent this rather
>> than `Delta::ZERO`.
>> 
>> I know that we use 0 to mean indefinite wait in C, I am saying that
>> it's not the most intuitive way to represent in Rust.
>> 
>> Intuitively, a timeout of 0 should be closer to a timeout of 1 and thus
>> should mean "return with ETIMEDOUT immedidately" rather than "wait
>> forever".
>> 
>> In C since we don't have a very good sum type support, so we
>> special case 0 to be the special value to represent indefinite wait,
>> but I don't think we need to repeat this in Rust.
> 
> Understood, thanks. How about the following code?
> 
> +#[track_caller]
> +pub fn read_poll_timeout<Op, Cond, T: Copy>(

Oops, `Copy` should be dropped:

+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(Location::caller());
> +    }
> +
> +    loop {
> +        let val = op()?;
> +        if cond(&val) {
> +            // Unlike the C version, we immediately return.
> +            // We know the condition is met so we don't need to check again.
> +            return Ok(val);
> +        }
> +        if let Some(timeout_delta) = timeout_delta {
> +            if start.elapsed() > timeout_delta {
> +                // Unlike the C version, we immediately return.
> +                // We have just called `op()` so we don't need to call it again.
> +                return Err(ETIMEDOUT);
> +            }
> +        }
> +        if sleep {
> +            fsleep(sleep_delta);
> +        }
> +        // fsleep() could be busy-wait loop so we always call cpu_relax().
> +        cpu_relax();
> +    }
> +}
> 

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

[Fiona Behrens]

On 25 Jan 2025, at 11:18, FUJITA Tomonori wrote:

> Add read_poll_timeout functions which poll periodically until a
> condition is met or a timeout is reached.
>
> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
> macro and a simple wrapper for Rust doesn't work. So this implements
> the same functionality in Rust.
>
> The C version uses usleep_range() while the Rust version uses
> fsleep(), which uses the best sleep method so it works with spans that
> usleep_range() doesn't work nicely with.
>
> Unlike the C version, __might_sleep() is used instead of might_sleep()
> to show proper debug info; the file name and line
> number. might_resched() could be added to match what the C version
> does but this function works without it.
>
> The sleep_before_read argument isn't supported since there is no user
> for now. It's rarely used in the C version.
>
> read_poll_timeout() can only be used in a nonatomic context. This
> requirement is not checked by these abstractions, but it is intended
> that klint [1] or a similar tool will be used to check it in the
> future.
>
> Link: https://rust-for-linux.com/klint [1]
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/helpers/helpers.c |  1 +
>  rust/helpers/kernel.c  | 13 +++++++
>  rust/kernel/cpu.rs     | 13 +++++++
>  rust/kernel/error.rs   |  1 +
>  rust/kernel/io.rs      |  5 +++
>  rust/kernel/io/poll.rs | 79 ++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs     |  2 ++
>  7 files changed, 114 insertions(+)
>  create mode 100644 rust/helpers/kernel.c
>  create mode 100644 rust/kernel/cpu.rs
>  create mode 100644 rust/kernel/io.rs
>  create mode 100644 rust/kernel/io/poll.rs
>
(..)
> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
> new file mode 100644
> index 000000000000..7a503cf643a1
> --- /dev/null
> +++ b/rust/kernel/io/poll.rs
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! IO polling.
> +//!
> +//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
> +
> +use crate::{
> +    cpu::cpu_relax,
> +    error::{code::*, Result},
> +    time::{delay::fsleep, Delta, Instant},
> +};
> +
> +use core::panic::Location;
> +
> +/// Polls periodically until a condition is met or a timeout is reached.
> +///
> +/// Public but hidden since it should only be used from public macros.

This states the function should be hidden, but I don’t see a `#[doc(hidden)]` in here so bit confused by that comment what part now is hidden.

Thanks,
Fiona

> +///
> +/// ```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), Delta::from_micros(42));
> +/// drop(g);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +#[track_caller]
> +pub fn read_poll_timeout<Op, Cond, T: Copy>(
> +    mut op: Op,
> +    mut cond: Cond,
> +    sleep_delta: Delta,
> +    timeout_delta: Delta,
> +) -> Result<T>
> +where
> +    Op: FnMut() -> Result<T>,
> +    Cond: FnMut(&T) -> bool,
> +{
> +    let start = Instant::now();
> +    let sleep = !sleep_delta.is_zero();
> +    let timeout = !timeout_delta.is_zero();
> +
> +    if sleep {
> +        might_sleep(Location::caller());
> +    }
> +
> +    loop {
> +        let val = op()?;
> +        if cond(&val) {
> +            // Unlike the C version, we immediately return.
> +            // We know the condition is met so we don't need to check again.
> +            return Ok(val);
> +        }
> +        if timeout && start.elapsed() > timeout_delta {
> +            // Unlike the C version, we immediately return.
> +            // We have just called `op()` so we don't need to call it again.
> +            return Err(ETIMEDOUT);
> +        }
> +        if sleep {
> +            fsleep(sleep_delta);
> +        }
> +        // fsleep() could be busy-wait loop so we always call cpu_relax().
> +        cpu_relax();
> +    }
> +}
> +
> +fn might_sleep(loc: &Location<'_>) {
> +    // SAFETY: FFI call.
> +    unsafe {
> +        crate::bindings::__might_sleep_precision(
> +            loc.file().as_ptr().cast(),
> +            loc.file().len() as i32,
> +            loc.line() as i32,
> +        )
> +    }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 545d1170ee63..c477701b2efa 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -35,6 +35,7 @@
>  pub mod block;
>  #[doc(hidden)]
>  pub mod build_assert;
> +pub mod cpu;
>  pub mod cred;
>  pub mod device;
>  pub mod error;
> @@ -42,6 +43,7 @@
>  pub mod firmware;
>  pub mod fs;
>  pub mod init;
> +pub mod io;
>  pub mod ioctl;
>  pub mod jump_label;
>  #[cfg(CONFIG_KUNIT)]
> -- 
> 2.43.0

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

[FUJITA Tomonori]

On Tue, 28 Jan 2025 11:52:42 +0100
Fiona Behrens <me@kloenk.dev> wrote:

>> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
>> new file mode 100644
>> index 000000000000..7a503cf643a1
>> --- /dev/null
>> +++ b/rust/kernel/io/poll.rs
>> @@ -0,0 +1,79 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! IO polling.
>> +//!
>> +//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
>> +
>> +use crate::{
>> +    cpu::cpu_relax,
>> +    error::{code::*, Result},
>> +    time::{delay::fsleep, Delta, Instant},
>> +};
>> +
>> +use core::panic::Location;
>> +
>> +/// Polls periodically until a condition is met or a timeout is reached.
>> +///
>> +/// Public but hidden since it should only be used from public macros.
> 
> This states the function should be hidden, but I don’t see a `#[doc(hidden)]` in here so bit confused by that comment what part now is hidden.

Oops, this comment is the older implementation; it should have been
deleted. I'll fix.

[PATCH v9 8/8] net: phy: qt2025: Wait until PHY becomes ready

[FUJITA Tomonori]

Wait until a PHY becomes ready in the probe callback by
using read_poll_timeout function.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 drivers/net/phy/qt2025.rs | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
index 1ab065798175..b6a63eaa6ef5 100644
--- a/drivers/net/phy/qt2025.rs
+++ b/drivers/net/phy/qt2025.rs
@@ -12,6 +12,7 @@
 use kernel::c_str;
 use kernel::error::code;
 use kernel::firmware::Firmware;
+use kernel::io::poll::read_poll_timeout;
 use kernel::net::phy::{
     self,
     reg::{Mmd, C45},
@@ -19,6 +20,7 @@
 };
 use kernel::prelude::*;
 use kernel::sizes::{SZ_16K, SZ_8K};
+use kernel::time::Delta;
 
 kernel::module_phy_driver! {
     drivers: [PhyQT2025],
@@ -93,7 +95,13 @@ fn probe(dev: &mut phy::Device) -> Result<()> {
         // The micro-controller will start running from SRAM.
         dev.write(C45::new(Mmd::PCS, 0xe854), 0x0040)?;
 
-        // TODO: sleep here until the hw becomes ready.
+        read_poll_timeout(
+            || dev.read(C45::new(Mmd::PCS, 0xd7fd)),
+            |val| *val != 0x00 && *val != 0x10,
+            Delta::from_millis(50),
+            Delta::from_secs(3),
+        )?;
+
         Ok(())
     }
 
-- 
2.43.0