[PATCH v8 0/7] rust: Add IO polling

[PATCH v8 0/7] 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 7th patch fixes
QT2025 PHY driver to sleep until the hardware becomes ready.

As a result of the past discussion, 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.

Add __might_sleep_precision(), rust friendly version of
__might_sleep(), which takes a pointer to a string with the length.
core::panic::Location::file() doesn't provide a null-terminated string
so a work around is necessary to use __might_sleep(). Providing a
null-terminated string for better C interoperability is under
discussion [1].

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

v8:
- 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 (7):
  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       |  28 +++++++++--
 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    |  84 +++++++++++++++++++++++++++++++
 rust/kernel/lib.rs        |   2 +
 rust/kernel/time.rs       | 103 ++++++++++++++++++++++++++++----------
 rust/kernel/time/delay.rs |  43 ++++++++++++++++
 14 files changed, 283 insertions(+), 33 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: ceff0757f5dafb5be5205988171809c877b1d3e3
-- 
2.43.0

[PATCH v8 1/7] 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>
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

[PATCH v8 2/7] 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 different from Delta.

Delta::from_[millis|secs] APIs take i64. When a span of time
overflows, i64::MAX is used.

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

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 48b71e6641ce..55a365af85a3 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,60 @@ fn sub(self, other: Ktime) -> Ktime {
         }
     }
 }
+
+/// A span of time.
+#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
+pub struct Delta {
+    nanos: i64,
+}
+
+impl Delta {
+    /// Create a new `Delta` from a number of microseconds.
+    #[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.
+    #[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.
+    #[inline]
+    pub const fn from_secs(secs: i64) -> Self {
+        Self {
+            nanos: secs.saturating_mul(NSEC_PER_SEC),
+        }
+    }
+
+    /// Return `true` if the `Detla` spans no time.
+    #[inline]
+    pub fn is_zero(self) -> bool {
+        self.as_nanos() == 0
+    }
+
+    /// Return `true` if the `Detla` 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 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 fn as_micros_ceil(self) -> i64 {
+        self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
+    }
+}
-- 
2.43.0

[PATCH v8 3/7] 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

The operation never overflows (Instant ranges from 0 to
`KTIME_MAX`).

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

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 55a365af85a3..da54a70f8f1f 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -31,59 +31,43 @@ 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.
 #[repr(transparent)]
 #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
-pub struct Ktime {
+pub struct Instant {
+    // Range from 0 to `KTIME_MAX`.
     inner: bindings::ktime_t,
 }
 
-impl Ktime {
-    /// Create a `Ktime` from a raw `ktime_t`.
+impl Instant {
+    /// Create a `Instant` from a raw `ktime_t`.
     #[inline]
-    pub fn from_raw(inner: bindings::ktime_t) -> Self {
+    fn from_raw(inner: bindings::ktime_t) -> Self {
         Self { inner }
     }
 
     /// Get the current time using `CLOCK_MONOTONIC`.
     #[inline]
-    pub fn ktime_get() -> Self {
+    pub fn now() -> 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
-    }
-
-    /// Returns the number of milliseconds.
-    #[inline]
-    pub fn to_ms(self) -> i64 {
-        self.divns_constant::<NSEC_PER_MSEC>()
+    /// Return the amount of time elapsed since the `Instant`.
+    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;
 
+    // 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

[PATCH v8 4/7] 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       |  4 +++-
 rust/kernel/time/delay.rs | 43 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 55 insertions(+), 1 deletion(-)
 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 da54a70f8f1f..3be2bf578519 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -2,12 +2,14 @@
 
 //! Time related primitives.
 //!
-//! This module contains the kernel APIs related to time and timers that
+//! This module contains the kernel APIs related to time that
 //! have been ported or wrapped for usage by Rust code in the kernel.
 //!
 //! 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..db5c08b0f230
--- /dev/null
+++ b/rust/kernel/time/delay.rs
@@ -0,0 +1,43 @@
+// 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 kernel's [`fsleep`], flexible sleep function,
+/// which automatically chooses the best sleep method based on a duration.
+///
+/// `delta` must be 0 or greater and no more than `u32::MAX / 2` microseconds.
+/// If a value outside the range is given, the function will sleep
+/// for `u32::MAX / 2` microseconds (= ~2147 seconds or ~36 minutes) at least.
+///
+/// This function can only be used in a nonatomic context.
+pub fn fsleep(delta: Delta) {
+    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
+    // Considering that fsleep rounds up the duration to the nearest millisecond,
+    // set the maximum value to u32::MAX / 2 microseconds.
+    const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
+
+    let duration = if delta > MAX_DURATION || delta.is_negative() {
+        // TODO: add WARN_ONCE() when it's supported.
+        MAX_DURATION
+    } else {
+        delta
+    };
+
+    // SAFETY: FFI call.
+    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(duration.as_micros_ceil() as c_ulong)
+    }
+}
-- 
2.43.0

[PATCH v8 5/7] 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 baf0eeb9a355..77bf1d2e6173 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10251,6 +10251,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
@@ -23643,6 +23644,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 v8 6/7] 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.

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.

core::panic::Location::file() doesn't provide a null-terminated string
so add __might_sleep_precision() helper function, which takes a
pointer to a string with its length.

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]
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    | 28 +++++++++++---
 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 | 84 ++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |  2 +
 9 files changed, 144 insertions(+), 5 deletions(-)
 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/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 3e5a6bf587f9..d9ac66dc66d3 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8670,7 +8670,10 @@ void __init sched_init(void)
 
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
 
-void __might_sleep(const char *file, int line)
+static void __might_resched_precision(const char *file, int len,
+				      int line, unsigned int offsets);
+
+void __might_sleep_precision(const char *file, int len, int line)
 {
 	unsigned int state = get_current_state();
 	/*
@@ -8684,7 +8687,14 @@ void __might_sleep(const char *file, int line)
 			(void *)current->task_state_change,
 			(void *)current->task_state_change);
 
-	__might_resched(file, line, 0);
+	__might_resched_precision(file, len, line, 0);
+}
+
+void __might_sleep(const char *file, int line)
+{
+	long len = strlen(file);
+
+	__might_sleep_precision(file, len, line);
 }
 EXPORT_SYMBOL(__might_sleep);
 
@@ -8709,7 +8719,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;
@@ -8732,8 +8743,8 @@ 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);
+	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);
@@ -8758,6 +8769,13 @@ 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)
+{
+	long len = strlen(file);
+
+	__might_resched_precision(file, len, line, offsets);
+}
 EXPORT_SYMBOL(__might_resched);
 
 void __cant_sleep(const char *file, int line, int preempt_offset)
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..da8e975d8e50
--- /dev/null
+++ b/rust/kernel/io/poll.rs
@@ -0,0 +1,84 @@
+// 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,
+    cond: Cond,
+    sleep_delta: Delta,
+    timeout_delta: Delta,
+) -> Result<T>
+where
+    Op: FnMut() -> Result<T>,
+    Cond: Fn(T) -> bool,
+{
+    let start = Instant::now();
+    let sleep = !sleep_delta.is_zero();
+    let timeout = !timeout_delta.is_zero();
+
+    might_sleep(Location::caller());
+
+    let val = loop {
+        let val = op()?;
+        if cond(val) {
+            // Unlike the C version, we immediately return.
+            // We know a condition is met so we don't need to check again.
+            return Ok(val);
+        }
+        if timeout && start.elapsed() > timeout_delta {
+            // Should we return Err(ETIMEDOUT) here instead of call op() again
+            // without a sleep between? But we follow the C version. op() could
+            // take some time so might be worth checking again.
+            break op()?;
+        }
+        if sleep {
+            fsleep(sleep_delta);
+        }
+        // fsleep() could be busy-wait loop so we always call cpu_relax().
+        cpu_relax();
+    };
+
+    if cond(val) {
+        Ok(val)
+    } else {
+        Err(ETIMEDOUT)
+    }
+}
+
+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

[PATCH v8 7/7] 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>
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..f642831519ca 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

Re: [PATCH v8 7/7] net: phy: qt2025: Wait until PHY becomes ready

[Alice Ryhl]

On Thu, Jan 16, 2025 at 5:43 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Wait until a PHY becomes ready in the probe callback by
> using read_poll_timeout function.
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

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

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[Alice Ryhl]

On Thu, Jan 16, 2025 at 5:42 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>
> ---
>  rust/helpers/helpers.c    |  1 +
>  rust/helpers/time.c       |  8 ++++++++
>  rust/kernel/time.rs       |  4 +++-
>  rust/kernel/time/delay.rs | 43 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 55 insertions(+), 1 deletion(-)
>  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 da54a70f8f1f..3be2bf578519 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -2,12 +2,14 @@
>
>  //! Time related primitives.
>  //!
> -//! This module contains the kernel APIs related to time and timers that
> +//! This module contains the kernel APIs related to time that
>  //! have been ported or wrapped for usage by Rust code in the kernel.
>  //!
>  //! 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..db5c08b0f230
> --- /dev/null
> +++ b/rust/kernel/time/delay.rs
> @@ -0,0 +1,43 @@
> +// 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 kernel's [`fsleep`], flexible sleep function,
> +/// which automatically chooses the best sleep method based on a duration.
> +///
> +/// `delta` must be 0 or greater and no more than `u32::MAX / 2` microseconds.
> +/// If a value outside the range is given, the function will sleep
> +/// for `u32::MAX / 2` microseconds (= ~2147 seconds or ~36 minutes) at least.
> +///
> +/// This function can only be used in a nonatomic context.
> +pub fn fsleep(delta: Delta) {
> +    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
> +    // Considering that fsleep rounds up the duration to the nearest millisecond,
> +    // set the maximum value to u32::MAX / 2 microseconds.
> +    const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);

Hmm, is this value correct on 64-bit platforms?

> +    let duration = if delta > MAX_DURATION || delta.is_negative() {
> +        // TODO: add WARN_ONCE() when it's supported.
> +        MAX_DURATION
> +    } else {
> +        delta
> +    };
> +
> +    // SAFETY: FFI call.
> +    unsafe {

This safety comment is incomplete. You can say this:

// SAFETY: It is always safe to call fsleep with any duration.

Alice

Re: [PATCH v8 3/7] rust: time: Introduce Instant type

[Alice Ryhl]

On Thu, Jan 16, 2025 at 5:42 AM 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
>
> The operation never overflows (Instant ranges from 0 to
> `KTIME_MAX`).
>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/time.rs | 48 +++++++++++++++------------------------------
>  1 file changed, 16 insertions(+), 32 deletions(-)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 55a365af85a3..da54a70f8f1f 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -31,59 +31,43 @@ 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.
>  #[repr(transparent)]
>  #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
> -pub struct Ktime {
> +pub struct Instant {
> +    // Range from 0 to `KTIME_MAX`.
>      inner: bindings::ktime_t,
>  }
>
> -impl Ktime {
> -    /// Create a `Ktime` from a raw `ktime_t`.
> +impl Instant {
> +    /// Create a `Instant` from a raw `ktime_t`.
>      #[inline]
> -    pub fn from_raw(inner: bindings::ktime_t) -> Self {
> +    fn from_raw(inner: bindings::ktime_t) -> Self {
>          Self { inner }
>      }

Please keep this function public.

>      /// Get the current time using `CLOCK_MONOTONIC`.
>      #[inline]
> -    pub fn ktime_get() -> Self {
> +    pub fn now() -> 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
> -    }
> -
> -    /// Returns the number of milliseconds.
> -    #[inline]
> -    pub fn to_ms(self) -> i64 {
> -        self.divns_constant::<NSEC_PER_MSEC>()
> +    /// Return the amount of time elapsed since the `Instant`.
> +    pub fn elapsed(&self) -> Delta {

Nit: This places the #[inline] marker before the documentation. Please
move it after to be consistent.

Alice

Re: [PATCH v8 2/7] rust: time: Introduce Delta type

[Alice Ryhl]

On Thu, Jan 16, 2025 at 5:42 AM 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 different from Delta.
>
> Delta::from_[millis|secs] APIs take i64. When a span of time
> overflows, i64::MAX is used.
>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

One nit below, otherwise LGTM

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

> +    /// Return the number of nanoseconds in the `Delta`.
> +    #[inline]
> +    pub fn as_nanos(self) -> i64 {
> +        self.nanos
> +    }

I added the ktime_ms_delta() function because I was going to use it.
Can you add an `as_millis()` function too? That way I can use
start_time.elapsed().as_millis() for my use-case.

Alice

Re: [PATCH v8 6/7] rust: Add read_poll_timeout functions

[Alice Ryhl]

On Thu, Jan 16, 2025 at 5:43 AM 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.
>
> 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.
>
> core::panic::Location::file() doesn't provide a null-terminated string
> so add __might_sleep_precision() helper function, which takes a
> pointer to a string with its length.
>
> 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]
> 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>

[...]

> +void __might_sleep(const char *file, int line)
> +{
> +       long len = strlen(file);
> +
> +       __might_sleep_precision(file, len, line);
>  }
>  EXPORT_SYMBOL(__might_sleep);

I think these strlen() calls could be pretty expensive. You run them
every time might_sleep() runs even if the check does not fail.

How about changing __might_resched_precision() to accept a length of
-1 for nul-terminated strings, and having it compute the length with
strlen only *if* we know that we actually need the length?

if (len < 0) len = strlen(file);
pr_err("BUG: sleeping function called from invalid context at %.*s:%d\n",
       len, file, line);

Another option might be to compile the lengths at compile-time by
having the macros use sizeof on __FILE__, but that sounds more tricky
to get right.

Alice

Re: [PATCH v8 6/7] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

On Thu, 16 Jan 2025 10:45:00 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

>> +void __might_sleep(const char *file, int line)
>> +{
>> +       long len = strlen(file);
>> +
>> +       __might_sleep_precision(file, len, line);
>>  }
>>  EXPORT_SYMBOL(__might_sleep);
> 
> I think these strlen() calls could be pretty expensive. You run them
> every time might_sleep() runs even if the check does not fail.

Ah, yes.

> How about changing __might_resched_precision() to accept a length of
> -1 for nul-terminated strings, and having it compute the length with
> strlen only *if* we know that we actually need the length?
> 
> if (len < 0) len = strlen(file);
> pr_err("BUG: sleeping function called from invalid context at %.*s:%d\n",
>        len, file, line);

Works for me.

> Another option might be to compile the lengths at compile-time by
> having the macros use sizeof on __FILE__, but that sounds more tricky
> to get right.

Yeah.

By the way, from what I saw in the discussion about Location::file(),
I got the impression that the feature for a null-terminated string
seems likely to be supported in the near future. Am I correct?

If so, rather than adding a Rust-specific helper function to the C
side, it would be better to solve the problem on the Rust side like
the previous versions with c_str()! and file()! for now?

Re: [PATCH v8 6/7] rust: Add read_poll_timeout functions

[Alice Ryhl]

On Thu, Jan 16, 2025 at 12:32 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Thu, 16 Jan 2025 10:45:00 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> >> +void __might_sleep(const char *file, int line)
> >> +{
> >> +       long len = strlen(file);
> >> +
> >> +       __might_sleep_precision(file, len, line);
> >>  }
> >>  EXPORT_SYMBOL(__might_sleep);
> >
> > I think these strlen() calls could be pretty expensive. You run them
> > every time might_sleep() runs even if the check does not fail.
>
> Ah, yes.
>
> > How about changing __might_resched_precision() to accept a length of
> > -1 for nul-terminated strings, and having it compute the length with
> > strlen only *if* we know that we actually need the length?
> >
> > if (len < 0) len = strlen(file);
> > pr_err("BUG: sleeping function called from invalid context at %.*s:%d\n",
> >        len, file, line);
>
> Works for me.
>
> > Another option might be to compile the lengths at compile-time by
> > having the macros use sizeof on __FILE__, but that sounds more tricky
> > to get right.
>
> Yeah.
>
> By the way, from what I saw in the discussion about Location::file(),
> I got the impression that the feature for a null-terminated string
> seems likely to be supported in the near future. Am I correct?

There's a good chance, but it's also not guaranteed.

> If so, rather than adding a Rust-specific helper function to the C
> side, it would be better to solve the problem on the Rust side like
> the previous versions with c_str()! and file()! for now?

I would be okay with a scenario where older compilers just reference
the read_poll_timeout() function in the error message, and only newer
compilers reference the location of the caller. Of course, right now,
only older compilers exist. But if we don't get nul-terminated
location strings, then I do think we should make the change you're
currently making.

Alice

Re: [PATCH v8 6/7] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

On Thu, 16 Jan 2025 12:42:57 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

>> If so, rather than adding a Rust-specific helper function to the C
>> side, it would be better to solve the problem on the Rust side like
>> the previous versions with c_str()! and file()! for now?
> 
> I would be okay with a scenario where older compilers just reference
> the read_poll_timeout() function in the error message, and only newer
> compilers reference the location of the caller. Of course, right now,
> only older compilers exist. But if we don't get nul-terminated
> location strings, then I do think we should make the change you're
> currently making.

Okay, let's see if we can get ACK from the scheduler maintainers with
your version, which has less impact on the C code.

Re: [PATCH v8 6/7] rust: Add read_poll_timeout functions

[Alice Ryhl]

On Thu, Jan 16, 2025 at 12:49 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Thu, 16 Jan 2025 12:42:57 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> >> If so, rather than adding a Rust-specific helper function to the C
> >> side, it would be better to solve the problem on the Rust side like
> >> the previous versions with c_str()! and file()! for now?
> >
> > I would be okay with a scenario where older compilers just reference
> > the read_poll_timeout() function in the error message, and only newer
> > compilers reference the location of the caller. Of course, right now,
> > only older compilers exist. But if we don't get nul-terminated
> > location strings, then I do think we should make the change you're
> > currently making.
>
> Okay, let's see if we can get ACK from the scheduler maintainers with
> your version, which has less impact on the C code.

You might want to split the might_sleep() changes into its own commit
to make it harder to miss. Right now, the title looks like something
that isn't changing the C side.

Alice

Re: [PATCH v8 2/7] rust: time: Introduce Delta type

[FUJITA Tomonori]

On Thu, 16 Jan 2025 10:36:07 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

> On Thu, Jan 16, 2025 at 5:42 AM 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 different from Delta.
>>
>> Delta::from_[millis|secs] APIs take i64. When a span of time
>> overflows, i64::MAX is used.
>>
>> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> One nit below, otherwise LGTM
> 
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Thanks!

>> +    /// Return the number of nanoseconds in the `Delta`.
>> +    #[inline]
>> +    pub fn as_nanos(self) -> i64 {
>> +        self.nanos
>> +    }
> 
> I added the ktime_ms_delta() function because I was going to use it.
> Can you add an `as_millis()` function too? That way I can use
> start_time.elapsed().as_millis() for my use-case.

Surely, I'll in the next version.

I dropped as_millis() method in v4 because I followed the rule, don't
add an API that may not be used.

Re: [PATCH v8 3/7] rust: time: Introduce Instant type

[FUJITA Tomonori]

On Thu, 16 Jan 2025 10:32:45 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

>> -impl Ktime {
>> -    /// Create a `Ktime` from a raw `ktime_t`.
>> +impl Instant {
>> +    /// Create a `Instant` from a raw `ktime_t`.
>>      #[inline]
>> -    pub fn from_raw(inner: bindings::ktime_t) -> Self {
>> +    fn from_raw(inner: bindings::ktime_t) -> Self {
>>          Self { inner }
>>      }
> 
> Please keep this function public.

Surely, your driver uses from_raw()?

>>      /// Get the current time using `CLOCK_MONOTONIC`.
>>      #[inline]
>> -    pub fn ktime_get() -> Self {
>> +    pub fn now() -> 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
>> -    }
>> -
>> -    /// Returns the number of milliseconds.
>> -    #[inline]
>> -    pub fn to_ms(self) -> i64 {
>> -        self.divns_constant::<NSEC_PER_MSEC>()
>> +    /// Return the amount of time elapsed since the `Instant`.
>> +    pub fn elapsed(&self) -> Delta {
> 
> Nit: This places the #[inline] marker before the documentation. Please
> move it after to be consistent.

Oops, I'll fix.

Re: [PATCH v8 3/7] rust: time: Introduce Instant type

[Miguel Ojeda]

On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> -/// A Rust wrapper around a `ktime_t`.
> +/// A specific point in time.
>  #[repr(transparent)]
>  #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
> -pub struct Ktime {
> +pub struct Instant {
> +    // Range from 0 to `KTIME_MAX`.

On top of what Tom mentioned: is this intended as an invariant? If
yes, then please document it publicly in the `Instant` docs in a `#
Invariants` section. Otherwise, I would clarify this comment somehow,
since it seems ambiguous.

Thanks!

Cheers,
Miguel

Re: [PATCH v8 2/7] rust: time: Introduce Delta type

[Miguel Ojeda]

On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> +    /// Return `true` if the `Detla` spans no time.

Typo: `Delta` (here and another one).

By the way, please try to use intra-doc links (i.e. unless they don't
work for some reason).

Thanks!

Cheers,
Miguel

Re: [PATCH v8 2/7] rust: time: Introduce Delta type

[Miguel Ojeda]

On Thu, Jan 16, 2025 at 1:00 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> I dropped as_millis() method in v4 because I followed the rule, don't
> add an API that may not be used.

Yeah, please mention the intended/expected use case in the commit
message so that it is clear, and then it should be OK (at least for
something simple like `as_millis()` here).

Thanks!

Cheers,
Miguel

Re: [PATCH v8 3/7] rust: time: Introduce Instant type

[FUJITA Tomonori]

On Thu, 16 Jan 2025 12:17:15 +0000
Tom Gundersen <tgunders@redhat.com> wrote:

>> -/// A Rust wrapper around a `ktime_t`.
>> +/// A specific point in time.
>>  #[repr(transparent)]
>>  #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
>> -pub struct Ktime {
>> +pub struct Instant {
>> +    // Range from 0 to `KTIME_MAX`.
>>      inner: bindings::ktime_t,
>>  }
>>
>> -impl Ktime {
>> -    /// Create a `Ktime` from a raw `ktime_t`.
>> +impl Instant {
>> +    /// Create a `Instant` from a raw `ktime_t`.
>>      #[inline]
>> -    pub fn from_raw(inner: bindings::ktime_t) -> Self {
>> +    fn from_raw(inner: bindings::ktime_t) -> Self {
>>
> 
> How do we know inner is between 0 and KTIME_MAX?

In my understanding, the kernel assumes that the range of the ktime_t
type is from 0 to KTIME_MAX. The ktime APIs guarantees to give a valid
ktime_t. The Rust abstraction creates ktime_t via ktime_get() so it's
fine now.

However, if we makes from_raw() public, a caller can create invalid
ktime_t by not using ktime APIs. Then from_raw() needs ceiling like
C's ktime_set(), I think.


>          Self { inner }
>>      }
>>
>>      /// Get the current time using `CLOCK_MONOTONIC`.
>>      #[inline]
>> -    pub fn ktime_get() -> Self {
>> +    pub fn now() -> Self {
>>          // SAFETY: It is always safe to call `ktime_get` outside of NMI
>> context.
>>
> 
> Similarly, should there be a comment here about the range being guaranteed
> to be correct?
> 
>          Self::from_raw(unsafe { bindings::ktime_get() })

We could add something like "The ktime API guarantees a valid
ktime_t". But adding similar comments to all the places where the
ktime API is called is redundant?

The comment on Instant struct must be improved instead, I think.

Re: [PATCH v8 3/7] rust: time: Introduce Instant type

[FUJITA Tomonori]

On Thu, 16 Jan 2025 13:37:42 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> -/// A Rust wrapper around a `ktime_t`.
>> +/// A specific point in time.
>>  #[repr(transparent)]
>>  #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
>> -pub struct Ktime {
>> +pub struct Instant {
>> +    // Range from 0 to `KTIME_MAX`.
> 
> On top of what Tom mentioned: is this intended as an invariant? If
> yes, then please document it publicly in the `Instant` docs in a `#
> Invariants` section. Otherwise, I would clarify this comment somehow,
> since it seems ambiguous.

As I wrote to Tom, that's the kernel's assumption. Do we need to make
it an invariant too?

Or improving the above "Range from 0 to `KTIME_MAX.`" is enough?

The kernel assumes that the range of the ktime_t type is from 0 to
KTIME_MAX. The ktime APIs guarantees to give a valid ktime_t.

Re: [PATCH v8 2/7] rust: time: Introduce Delta type

[FUJITA Tomonori]

On Thu, 16 Jan 2025 13:43:36 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> +    /// Return `true` if the `Detla` spans no time.
> 
> Typo: `Delta` (here and another one).

Oops, sorry. I'll fix.

> By the way, please try to use intra-doc links (i.e. unless they don't
> work for some reason).

Surely, I'll try.

Thanks,

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[FUJITA Tomonori]

On Thu, 16 Jan 2025 10:27:02 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

>> +/// This function can only be used in a nonatomic context.
>> +pub fn fsleep(delta: Delta) {
>> +    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>> +    // Considering that fsleep rounds up the duration to the nearest millisecond,
>> +    // set the maximum value to u32::MAX / 2 microseconds.
>> +    const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
> 
> Hmm, is this value correct on 64-bit platforms?

You meant that the maximum can be longer on 64-bit platforms? 2147484
milliseconds is long enough for fsleep's duration?

If you prefer, I use different maximum durations for 64-bit and 32-bit
platforms, respectively.


>> +    let duration = if delta > MAX_DURATION || delta.is_negative() {
>> +        // TODO: add WARN_ONCE() when it's supported.
>> +        MAX_DURATION
>> +    } else {
>> +        delta
>> +    };
>> +
>> +    // SAFETY: FFI call.
>> +    unsafe {
> 
> This safety comment is incomplete. You can say this:
> 
> // SAFETY: It is always safe to call fsleep with any duration.

Thanks, I'll use your safety comment.

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[FUJITA Tomonori]

On Fri, 17 Jan 2025 16:53:26 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> On Thu, 16 Jan 2025 10:27:02 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
>>> +/// This function can only be used in a nonatomic context.
>>> +pub fn fsleep(delta: Delta) {
>>> +    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>>> +    // Considering that fsleep rounds up the duration to the nearest millisecond,
>>> +    // set the maximum value to u32::MAX / 2 microseconds.
>>> +    const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
>> 
>> Hmm, is this value correct on 64-bit platforms?
> 
> You meant that the maximum can be longer on 64-bit platforms? 2147484
> milliseconds is long enough for fsleep's duration?
> 
> If you prefer, I use different maximum durations for 64-bit and 32-bit
> platforms, respectively.

How about the following?

const MAX_DURATION: Delta = Delta::from_micros(usize::MAX as i64 >> 1);

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[Alice Ryhl]

On Fri, Jan 17, 2025 at 10:01 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Fri, 17 Jan 2025 16:53:26 +0900 (JST)
> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>
> > On Thu, 16 Jan 2025 10:27:02 +0100
> > Alice Ryhl <aliceryhl@google.com> wrote:
> >
> >>> +/// This function can only be used in a nonatomic context.
> >>> +pub fn fsleep(delta: Delta) {
> >>> +    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
> >>> +    // Considering that fsleep rounds up the duration to the nearest millisecond,
> >>> +    // set the maximum value to u32::MAX / 2 microseconds.
> >>> +    const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
> >>
> >> Hmm, is this value correct on 64-bit platforms?
> >
> > You meant that the maximum can be longer on 64-bit platforms? 2147484
> > milliseconds is long enough for fsleep's duration?
> >
> > If you prefer, I use different maximum durations for 64-bit and 32-bit
> > platforms, respectively.
>
> How about the following?
>
> const MAX_DURATION: Delta = Delta::from_micros(usize::MAX as i64 >> 1);

Why is there a maximum in the first place? Are you worried about
overflow on the C side?

Alice

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[FUJITA Tomonori]

On Fri, 17 Jan 2025 10:13:08 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

> On Fri, Jan 17, 2025 at 10:01 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> On Fri, 17 Jan 2025 16:53:26 +0900 (JST)
>> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>>
>> > On Thu, 16 Jan 2025 10:27:02 +0100
>> > Alice Ryhl <aliceryhl@google.com> wrote:
>> >
>> >>> +/// This function can only be used in a nonatomic context.
>> >>> +pub fn fsleep(delta: Delta) {
>> >>> +    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>> >>> +    // Considering that fsleep rounds up the duration to the nearest millisecond,
>> >>> +    // set the maximum value to u32::MAX / 2 microseconds.
>> >>> +    const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
>> >>
>> >> Hmm, is this value correct on 64-bit platforms?
>> >
>> > You meant that the maximum can be longer on 64-bit platforms? 2147484
>> > milliseconds is long enough for fsleep's duration?
>> >
>> > If you prefer, I use different maximum durations for 64-bit and 32-bit
>> > platforms, respectively.
>>
>> How about the following?
>>
>> const MAX_DURATION: Delta = Delta::from_micros(usize::MAX as i64 >> 1);
> 
> Why is there a maximum in the first place? Are you worried about
> overflow on the C side?

Yeah, Boqun is concerned that an incorrect input (a negative value or
an overflow on the C side) leads to unintentional infinite sleep:

https://lore.kernel.org/lkml/ZxwVuceNORRAI7FV@Boquns-Mac-mini.local/

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[Alice Ryhl]

On Fri, Jan 17, 2025 at 10:55 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Fri, 17 Jan 2025 10:13:08 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > On Fri, Jan 17, 2025 at 10:01 AM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:
> >>
> >> On Fri, 17 Jan 2025 16:53:26 +0900 (JST)
> >> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> >>
> >> > On Thu, 16 Jan 2025 10:27:02 +0100
> >> > Alice Ryhl <aliceryhl@google.com> wrote:
> >> >
> >> >>> +/// This function can only be used in a nonatomic context.
> >> >>> +pub fn fsleep(delta: Delta) {
> >> >>> +    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
> >> >>> +    // Considering that fsleep rounds up the duration to the nearest millisecond,
> >> >>> +    // set the maximum value to u32::MAX / 2 microseconds.
> >> >>> +    const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
> >> >>
> >> >> Hmm, is this value correct on 64-bit platforms?
> >> >
> >> > You meant that the maximum can be longer on 64-bit platforms? 2147484
> >> > milliseconds is long enough for fsleep's duration?
> >> >
> >> > If you prefer, I use different maximum durations for 64-bit and 32-bit
> >> > platforms, respectively.
> >>
> >> How about the following?
> >>
> >> const MAX_DURATION: Delta = Delta::from_micros(usize::MAX as i64 >> 1);
> >
> > Why is there a maximum in the first place? Are you worried about
> > overflow on the C side?
>
> Yeah, Boqun is concerned that an incorrect input (a negative value or
> an overflow on the C side) leads to unintentional infinite sleep:
>
> https://lore.kernel.org/lkml/ZxwVuceNORRAI7FV@Boquns-Mac-mini.local/

Okay, can you explain in the comment that this maximum value prevents
integer overflow inside fsleep?

Alice

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[FUJITA Tomonori]

On Fri, 17 Jan 2025 14:05:52 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

> On Fri, Jan 17, 2025 at 10:55 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> On Fri, 17 Jan 2025 10:13:08 +0100
>> Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> > On Fri, Jan 17, 2025 at 10:01 AM FUJITA Tomonori
>> > <fujita.tomonori@gmail.com> wrote:
>> >>
>> >> On Fri, 17 Jan 2025 16:53:26 +0900 (JST)
>> >> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>> >>
>> >> > On Thu, 16 Jan 2025 10:27:02 +0100
>> >> > Alice Ryhl <aliceryhl@google.com> wrote:
>> >> >
>> >> >>> +/// This function can only be used in a nonatomic context.
>> >> >>> +pub fn fsleep(delta: Delta) {
>> >> >>> +    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>> >> >>> +    // Considering that fsleep rounds up the duration to the nearest millisecond,
>> >> >>> +    // set the maximum value to u32::MAX / 2 microseconds.
>> >> >>> +    const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
>> >> >>
>> >> >> Hmm, is this value correct on 64-bit platforms?
>> >> >
>> >> > You meant that the maximum can be longer on 64-bit platforms? 2147484
>> >> > milliseconds is long enough for fsleep's duration?
>> >> >
>> >> > If you prefer, I use different maximum durations for 64-bit and 32-bit
>> >> > platforms, respectively.
>> >>
>> >> How about the following?
>> >>
>> >> const MAX_DURATION: Delta = Delta::from_micros(usize::MAX as i64 >> 1);
>> >
>> > Why is there a maximum in the first place? Are you worried about
>> > overflow on the C side?
>>
>> Yeah, Boqun is concerned that an incorrect input (a negative value or
>> an overflow on the C side) leads to unintentional infinite sleep:
>>
>> https://lore.kernel.org/lkml/ZxwVuceNORRAI7FV@Boquns-Mac-mini.local/
> 
> Okay, can you explain in the comment that this maximum value prevents
> integer overflow inside fsleep?

Surely, how about the following?

pub fn fsleep(delta: Delta) {
    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
    // Considering that fsleep rounds up the duration to the nearest millisecond,
    // set the maximum value to u32::MAX / 2 microseconds to prevent integer
    // overflow inside fsleep, which could lead to unintentional infinite sleep.
    const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[Alice Ryhl]

On Fri, Jan 17, 2025 at 3:20 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Fri, 17 Jan 2025 14:05:52 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > On Fri, Jan 17, 2025 at 10:55 AM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:
> >>
> >> On Fri, 17 Jan 2025 10:13:08 +0100
> >> Alice Ryhl <aliceryhl@google.com> wrote:
> >>
> >> > On Fri, Jan 17, 2025 at 10:01 AM FUJITA Tomonori
> >> > <fujita.tomonori@gmail.com> wrote:
> >> >>
> >> >> On Fri, 17 Jan 2025 16:53:26 +0900 (JST)
> >> >> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> >> >>
> >> >> > On Thu, 16 Jan 2025 10:27:02 +0100
> >> >> > Alice Ryhl <aliceryhl@google.com> wrote:
> >> >> >
> >> >> >>> +/// This function can only be used in a nonatomic context.
> >> >> >>> +pub fn fsleep(delta: Delta) {
> >> >> >>> +    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
> >> >> >>> +    // Considering that fsleep rounds up the duration to the nearest millisecond,
> >> >> >>> +    // set the maximum value to u32::MAX / 2 microseconds.
> >> >> >>> +    const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
> >> >> >>
> >> >> >> Hmm, is this value correct on 64-bit platforms?
> >> >> >
> >> >> > You meant that the maximum can be longer on 64-bit platforms? 2147484
> >> >> > milliseconds is long enough for fsleep's duration?
> >> >> >
> >> >> > If you prefer, I use different maximum durations for 64-bit and 32-bit
> >> >> > platforms, respectively.
> >> >>
> >> >> How about the following?
> >> >>
> >> >> const MAX_DURATION: Delta = Delta::from_micros(usize::MAX as i64 >> 1);
> >> >
> >> > Why is there a maximum in the first place? Are you worried about
> >> > overflow on the C side?
> >>
> >> Yeah, Boqun is concerned that an incorrect input (a negative value or
> >> an overflow on the C side) leads to unintentional infinite sleep:
> >>
> >> https://lore.kernel.org/lkml/ZxwVuceNORRAI7FV@Boquns-Mac-mini.local/
> >
> > Okay, can you explain in the comment that this maximum value prevents
> > integer overflow inside fsleep?
>
> Surely, how about the following?
>
> pub fn fsleep(delta: Delta) {
>     // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>     // Considering that fsleep rounds up the duration to the nearest millisecond,
>     // set the maximum value to u32::MAX / 2 microseconds to prevent integer
>     // overflow inside fsleep, which could lead to unintentional infinite sleep.
>     const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);

Hmm ... this is phrased as-if the problem is on 32-bit machines, but
the problem is that fsleep casts an `unsigned long` to `unsigned int`
which can overflow on 64-bit machines. I would instead say this
prevents overflow on 64-bit machines when casting to an int.

Also, it might be cleaner to just use `i32::MAX as i64` instead of u32.

Alice

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[Miguel Ojeda]

On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> +/// `delta` must be 0 or greater and no more than `u32::MAX / 2` microseconds.
> +/// If a value outside the range is given, the function will sleep
> +/// for `u32::MAX / 2` microseconds (= ~2147 seconds or ~36 minutes) at least.

I would emphasize with something like:

    `delta` must be within [0, `u32::MAX / 2`] 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.

In addition, I would add a new paragraph how the behavior differs
w.r.t. the C `fsleep()`, i.e. IIRC from the past discussions,
`fsleep()` would do an infinite sleep instead. So I think it is
important to highlight that.

> +    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
> +    // Considering that fsleep rounds up the duration to the nearest millisecond,
> +    // set the maximum value to u32::MAX / 2 microseconds.

Nit: please use Markdown code spans in normal comments (no need for
intra-doc links there).

> +    let duration = if delta > MAX_DURATION || delta.is_negative() {
> +        // TODO: add WARN_ONCE() when it's supported.

Ditto (also "Add").

By the way, can this be written differently maybe? e.g. using a range
since it is `const`?

You can probably reuse `delta` as the new bindings name, since we
don't need the old one after this step.

Thanks!

Cheers,
Miguel

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[FUJITA Tomonori]

On Fri, 17 Jan 2025 15:31:07 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

> On Fri, Jan 17, 2025 at 3:20 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> On Fri, 17 Jan 2025 14:05:52 +0100
>> Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> > On Fri, Jan 17, 2025 at 10:55 AM FUJITA Tomonori
>> > <fujita.tomonori@gmail.com> wrote:
>> >>
>> >> On Fri, 17 Jan 2025 10:13:08 +0100
>> >> Alice Ryhl <aliceryhl@google.com> wrote:
>> >>
>> >> > On Fri, Jan 17, 2025 at 10:01 AM FUJITA Tomonori
>> >> > <fujita.tomonori@gmail.com> wrote:
>> >> >>
>> >> >> On Fri, 17 Jan 2025 16:53:26 +0900 (JST)
>> >> >> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>> >> >>
>> >> >> > On Thu, 16 Jan 2025 10:27:02 +0100
>> >> >> > Alice Ryhl <aliceryhl@google.com> wrote:
>> >> >> >
>> >> >> >>> +/// This function can only be used in a nonatomic context.
>> >> >> >>> +pub fn fsleep(delta: Delta) {
>> >> >> >>> +    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>> >> >> >>> +    // Considering that fsleep rounds up the duration to the nearest millisecond,
>> >> >> >>> +    // set the maximum value to u32::MAX / 2 microseconds.
>> >> >> >>> +    const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
>> >> >> >>
>> >> >> >> Hmm, is this value correct on 64-bit platforms?
>> >> >> >
>> >> >> > You meant that the maximum can be longer on 64-bit platforms? 2147484
>> >> >> > milliseconds is long enough for fsleep's duration?
>> >> >> >
>> >> >> > If you prefer, I use different maximum durations for 64-bit and 32-bit
>> >> >> > platforms, respectively.
>> >> >>
>> >> >> How about the following?
>> >> >>
>> >> >> const MAX_DURATION: Delta = Delta::from_micros(usize::MAX as i64 >> 1);
>> >> >
>> >> > Why is there a maximum in the first place? Are you worried about
>> >> > overflow on the C side?
>> >>
>> >> Yeah, Boqun is concerned that an incorrect input (a negative value or
>> >> an overflow on the C side) leads to unintentional infinite sleep:
>> >>
>> >> https://lore.kernel.org/lkml/ZxwVuceNORRAI7FV@Boquns-Mac-mini.local/
>> >
>> > Okay, can you explain in the comment that this maximum value prevents
>> > integer overflow inside fsleep?
>>
>> Surely, how about the following?
>>
>> pub fn fsleep(delta: Delta) {
>>     // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>>     // Considering that fsleep rounds up the duration to the nearest millisecond,
>>     // set the maximum value to u32::MAX / 2 microseconds to prevent integer
>>     // overflow inside fsleep, which could lead to unintentional infinite sleep.
>>     const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
> 
> Hmm ... this is phrased as-if the problem is on 32-bit machines, but
> the problem is that fsleep casts an `unsigned long` to `unsigned int`
> which can overflow on 64-bit machines. I would instead say this
> prevents overflow on 64-bit machines when casting to an int.

Yeah, but DIV_ROUND_UP in fsync() could also cause overflow before
casting ulong to uint for calling msleep() (it could happen on both
32-bit and 64-bit).

The following looks ok?

The maximum value is set to `u32::MAX / 2` microseconds to prevent integer
overflow inside fsleep, which could lead to unintentional infinite sleep.


> Also, it might be cleaner to just use `i32::MAX as i64` instead of u32.

You meant that using i32::MAX instead of u32::MAX / 2 (and u32::MAX >>
1) might be cleaner? I might think so too.

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[FUJITA Tomonori]

On Fri, 17 Jan 2025 19:59:15 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> +/// `delta` must be 0 or greater and no more than `u32::MAX / 2` microseconds.
>> +/// If a value outside the range is given, the function will sleep
>> +/// for `u32::MAX / 2` microseconds (= ~2147 seconds or ~36 minutes) at least.
> 
> I would emphasize with something like:
> 
>     `delta` must be within [0, `u32::MAX / 2`] 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.

Thanks, I'll use the above instead.

> In addition, I would add a new paragraph how the behavior differs
> w.r.t. the C `fsleep()`, i.e. IIRC from the past discussions,
> `fsleep()` would do an infinite sleep instead. So I think it is
> important to highlight that.

/// The above behavior differs from the kernel's [`fsleep`], which could sleep
/// infinitely (for [`MAX_JIFFY_OFFSET`] jiffies).

Looks ok?

>> +    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>> +    // Considering that fsleep rounds up the duration to the nearest millisecond,
>> +    // set the maximum value to u32::MAX / 2 microseconds.
> 
> Nit: please use Markdown code spans in normal comments (no need for
> intra-doc links there).

Understood.

>> +    let duration = if delta > MAX_DURATION || delta.is_negative() {
>> +        // TODO: add WARN_ONCE() when it's supported.
> 
> Ditto (also "Add").

Oops, I'll fix.

> By the way, can this be written differently maybe? e.g. using a range
> since it is `const`?

A range can be used for a custom type?

> You can probably reuse `delta` as the new bindings name, since we
> don't need the old one after this step.

Do you mean something like the following?

const MAX_DELTA: Delta = Delta::from_micros(i32::MAX as i64);

let delta = if delta > MAX_DELTA || delta.is_negative() {
    // TODO: Add WARN_ONCE() when it's supported.
    MAX_DELTA
} else {
    delta
};

Re: [PATCH v8 3/7] rust: time: Introduce Instant type

[Miguel Ojeda]

On Fri, Jan 17, 2025 at 12:31 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> As I wrote to Tom, that's the kernel's assumption. Do we need to make
> it an invariant too?
>
> Or improving the above "Range from 0 to `KTIME_MAX.`" is enough?
>
> The kernel assumes that the range of the ktime_t type is from 0 to
> KTIME_MAX. The ktime APIs guarantees to give a valid ktime_t.

It depends on what is best for users, i.e. if there are no use cases
where this needs to be negative, then why wouldn't we have the
invariant documented? Or do we want to make it completely opaque?

Generally speaking, I think we should pick whatever makes the most
sense for the future, since we have a chance to "do the right thing",
even if the C side is a bit different (we already use a different
name, anyway).

Thomas et al. probably know what makes the most sense here.

Cheers,
Miguel

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[Miguel Ojeda]

On Sat, Jan 18, 2025 at 9:02 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> /// The above behavior differs from the kernel's [`fsleep`], which could sleep
> /// infinitely (for [`MAX_JIFFY_OFFSET`] jiffies).
>
> Looks ok?

I think if that is meant as an intra-doc link, it would link to this
function, rather than the C side one, so please add a link target to
e.g. https://docs.kernel.org/timers/delay_sleep_functions.html#c.fsleep.

I would also say "the C side [`fsleep()`] or similar"; in other words,
both are "kernel's" at this point.

And perhaps I would simplify and say something like "The behavior
above differs from the C side [`fsleep()`] for which out-of-range
values mean "infinite timeout" instead."

> A range can be used for a custom type?

I was thinking of doing it through `as_nanos()`, but it may read
worse, so please ignore it if so.

> Do you mean something like the following?

Yeah, I just meant using `let delta`.

Cheers,
Miguel

Re: [PATCH v8 2/7] rust: time: Introduce Delta type

[Miguel Ojeda]

Hi Tomonori,

Since you started getting Reviewed-bys, I don't want to delay this
more (pun unintended :), but a couple quick notes...

I can create "good first issues" for these in our tracker if you
prefer, since these should be easy and can be done later (even if, in
general, I think we should require examples and good docs for new
abstractions).

On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> 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 different from Delta.

Typo: "is different ..."?

> Delta::from_[millis|secs] APIs take i64. When a span of time
> overflows, i64::MAX is used.

This behavior should be part of the docs of the methods below.

> +/// A span of time.
> +#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
> +pub struct Delta {
> +    nanos: i64,
> +}

Some more docs here in `Delta` would be good, e.g. some questions
readers may have could be: What range of values can it hold? Can they
be negative?

Also some module-level docs would be nice relating all the types (you
mention some of that in the commit message for `Instant`, but it would
be nice to put it as docs, rather than in the commit message).

> +    /// Create a new `Delta` from a number of microseconds.
> +    #[inline]
> +    pub const fn from_micros(micros: i64) -> Self {
> +        Self {
> +            nanos: micros.saturating_mul(NSEC_PER_USEC),
> +        }
> +    }

For each of these, I would mention that they saturate and I would
mention the range of input values that would be kept as-is without
loss.

And it would be nice to add some examples, which you can take the
chance to show how it saturates, and they would double as tests, too.

Thanks!

Cheers,
Miguel

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[FUJITA Tomonori]

On Sat, 18 Jan 2025 13:17:29 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Sat, Jan 18, 2025 at 9:02 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> /// The above behavior differs from the kernel's [`fsleep`], which could sleep
>> /// infinitely (for [`MAX_JIFFY_OFFSET`] jiffies).
>>
>> Looks ok?
> 
> I think if that is meant as an intra-doc link, it would link to this
> function, rather than the C side one, so please add a link target to
> e.g. https://docs.kernel.org/timers/delay_sleep_functions.html#c.fsleep.

Added.

> I would also say "the C side [`fsleep()`] or similar"; in other words,
> both are "kernel's" at this point.

Agreed that "the C side" is better and updated the comment. I copied
that expression from the existing code; there are many "kernel's" in
rust/kernel/. "good first issues" for them?

You prefer "[`fsleep()`]" rather than "[`fsleep`]"? I can't find any
precedent for the C side functions.

> And perhaps I would simplify and say something like "The behavior
> above differs from the C side [`fsleep()`] for which out-of-range
> values mean "infinite timeout" instead."

Yeah, simpler is better. After applying the above changes, it ended up
as follows.

/// 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) {


>> A range can be used for a custom type?
> 
> I was thinking of doing it through `as_nanos()`, but it may read
> worse, so please ignore it if so.

Ah, it might work. The following doesn't work. Seems that we need to
add another const like MAX_DELTA_NANOS or something. No strong
preference but I feel the current is simpler.

let delta = match delta.as_nanos() {
    0..=MAX_DELTA.as_nanos() as i32 => delta,
    _ => MAX_DELTA,
};

Re: [PATCH v8 2/7] rust: time: Introduce Delta type

[FUJITA Tomonori]

On Sat, 18 Jan 2025 13:19:21 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> Since you started getting Reviewed-bys, I don't want to delay this
> more (pun unintended :), but a couple quick notes...
> 
> I can create "good first issues" for these in our tracker if you
> prefer, since these should be easy and can be done later (even if, in
> general, I think we should require examples and good docs for new
> abstractions).

Yes, please create such. I'll add more docs but I'm sure that there
will be room for improvement.

> On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> 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 different from Delta.
> 
> Typo: "is different ..."?

Oops, will fix.

>> Delta::from_[millis|secs] APIs take i64. When a span of time
>> overflows, i64::MAX is used.
>
> This behavior should be part of the docs of the methods below.

You want to add the above explanation to all the
Delta::from_[millis|micro|secs], right?

>> +/// A span of time.
>> +#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
>> +pub struct Delta {
>> +    nanos: i64,
>> +}
> 
> Some more docs here in `Delta` would be good, e.g. some questions
> readers may have could be: What range of values can it hold? Can they
> be negative?

Okay, I'll add.

> Also some module-level docs would be nice relating all the types (you
> mention some of that in the commit message for `Instant`, but it would
> be nice to put it as docs, rather than in the commit message).

Is there any existing source code I can refer to? I'm not sure
how "module-level docs" should be written.

>> +    /// Create a new `Delta` from a number of microseconds.
>> +    #[inline]
>> +    pub const fn from_micros(micros: i64) -> Self {
>> +        Self {
>> +            nanos: micros.saturating_mul(NSEC_PER_USEC),
>> +        }
>> +    }
> 
> For each of these, I would mention that they saturate and I would
> mention the range of input values that would be kept as-is without
> loss.
> 
> And it would be nice to add some examples, which you can take the
> chance to show how it saturates, and they would double as tests, too.

I'll try to improve.

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[Alice Ryhl]

On Wed, Jan 22, 2025 at 7:57 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Sat, 18 Jan 2025 13:17:29 +0100
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>
> > On Sat, Jan 18, 2025 at 9:02 AM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:
> >>
> >> /// The above behavior differs from the kernel's [`fsleep`], which could sleep
> >> /// infinitely (for [`MAX_JIFFY_OFFSET`] jiffies).
> >>
> >> Looks ok?
> >
> > I think if that is meant as an intra-doc link, it would link to this
> > function, rather than the C side one, so please add a link target to
> > e.g. https://docs.kernel.org/timers/delay_sleep_functions.html#c.fsleep.
>
> Added.
>
> > I would also say "the C side [`fsleep()`] or similar"; in other words,
> > both are "kernel's" at this point.
>
> Agreed that "the C side" is better and updated the comment. I copied
> that expression from the existing code; there are many "kernel's" in
> rust/kernel/. "good first issues" for them?
>
> You prefer "[`fsleep()`]" rather than "[`fsleep`]"? I can't find any
> precedent for the C side functions.

I think that's a matter of taste. In the Rust ecosystem, fsleep is
more common, in the kernel ecosystem, fsleep() is more common. I've
seen both in Rust code at this point.

> > And perhaps I would simplify and say something like "The behavior
> > above differs from the C side [`fsleep()`] for which out-of-range
> > values mean "infinite timeout" instead."
>
> Yeah, simpler is better. After applying the above changes, it ended up
> as follows.
>
> /// 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;

I'd do `[0, i32::MAX]` instead for better rendering.

> /// 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) {
>
>
> >> A range can be used for a custom type?
> >
> > I was thinking of doing it through `as_nanos()`, but it may read
> > worse, so please ignore it if so.
>
> Ah, it might work. The following doesn't work. Seems that we need to
> add another const like MAX_DELTA_NANOS or something. No strong
> preference but I feel the current is simpler.
>
> let delta = match delta.as_nanos() {
>     0..=MAX_DELTA.as_nanos() as i32 => delta,
>     _ => MAX_DELTA,
> };

Could you do Delta::min(delta, MAX_DELTA).as_nanos() ?

Alice

Re: [PATCH v8 2/7] rust: time: Introduce Delta type

[Miguel Ojeda]

On Wed, Jan 22, 2025 at 8:37 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Yes, please create such. I'll add more docs but I'm sure that there
> will be room for improvement.

Sounds good, will do!

> You want to add the above explanation to all the
> Delta::from_[millis|micro|secs], right?

Yeah, I meant to add the saturating note to each of them.

> Is there any existing source code I can refer to? I'm not sure
> how "module-level docs" should be written.

You can see e.g.

    rust/kernel/block/mq.rs
    rust/kernel/init.rs
    rust/kernel/workqueue.rs

(grep `^//!` for others).

In general, you can use the module-level docs to talk about how things
relate between items of that module. For instance, when I saw in your
commit message this note:

    Implement the subtraction operator for Instant:

    Delta = Instant A - Instant B

I thought something like: "It would be nice to explain how `Delta` and
`Instant` fit together / how they are related / how all fits together
in the `time` module".

> I'll try to improve.

Thanks a lot! (really -- I personally appreciate good docs)

Cheers,
Miguel

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[Miguel Ojeda]

On Wed, Jan 22, 2025 at 7:57 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Agreed that "the C side" is better and updated the comment. I copied
> that expression from the existing code; there are many "kernel's" in
> rust/kernel/. "good first issues" for them?

Yeah, will do.

> You prefer "[`fsleep()`]" rather than "[`fsleep`]"? I can't find any
> precedent for the C side functions.

There is no preference yet. It would be nice to be consistent, though.
The option of removing the `()` in all cases may be easier to check
for than the other, though the `()` give a bit of (possibly redundant)
information to the reader.

> Yeah, simpler is better. After applying the above changes, it ended up
> as follows.

Looks good, thanks!

Not sure if we should say "Equivalent" given it is not exactly the
same, but I am not a native speaker: I think it does not necessarily
need to be exactly the same to be "equivalent", but perhaps "Similar
to" or "Counterpart of" or something like that is better.

> Ah, it might work. The following doesn't work. Seems that we need to
> add another const like MAX_DELTA_NANOS or something. No strong
> preference but I feel the current is simpler.
>
> let delta = match delta.as_nanos() {
>     0..=MAX_DELTA.as_nanos() as i32 => delta,
>     _ => MAX_DELTA,
> };

Yeah, don't worry about it too much :)

[ The language may get `const { ... }` to work there (it does in
nightly) though it wouldn't look good either. I think the `as i32`
would not be needed. ]

By the way, speaking of something related, do we want to make some of
the methods `fn`s be `const`?

Cheers,
Miguel

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[FUJITA Tomonori]

On Wed, 22 Jan 2025 09:23:33 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

>> > I would also say "the C side [`fsleep()`] or similar"; in other words,
>> > both are "kernel's" at this point.
>>
>> Agreed that "the C side" is better and updated the comment. I copied
>> that expression from the existing code; there are many "kernel's" in
>> rust/kernel/. "good first issues" for them?
>>
>> You prefer "[`fsleep()`]" rather than "[`fsleep`]"? I can't find any
>> precedent for the C side functions.
> 
> I think that's a matter of taste. In the Rust ecosystem, fsleep is
> more common, in the kernel ecosystem, fsleep() is more common. I've
> seen both in Rust code at this point.

Understood, I'll go with [`fsleep`].


>> > And perhaps I would simplify and say something like "The behavior
>> > above differs from the C side [`fsleep()`] for which out-of-range
>> > values mean "infinite timeout" instead."
>>
>> Yeah, simpler is better. After applying the above changes, it ended up
>> as follows.
>>
>> /// 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;
> 
> I'd do `[0, i32::MAX]` instead for better rendering.

Yeah, looks better after redering. I'll update it.


>> /// 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) {
>>
>>
>> >> A range can be used for a custom type?
>> >
>> > I was thinking of doing it through `as_nanos()`, but it may read
>> > worse, so please ignore it if so.
>>
>> Ah, it might work. The following doesn't work. Seems that we need to
>> add another const like MAX_DELTA_NANOS or something. No strong
>> preference but I feel the current is simpler.
>>
>> let delta = match delta.as_nanos() {
>>     0..=MAX_DELTA.as_nanos() as i32 => delta,
>>     _ => MAX_DELTA,
>> };
> 
> Could you do Delta::min(delta, MAX_DELTA).as_nanos() ?

We need Delta type here so you meant:

let delta = std::cmp::min(delta, MAX_DELTA);

?

We also need to convert a negative delta to MAX_DELTA so we could do:

let delta = if delta.is_negative() {
    MAX_DELTA
} else {
    min(delta, MAX_DELTA)
};

looks a bit readable than the original code?

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[Alice Ryhl]

On Wed, Jan 22, 2025 at 11:44 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> >> >> A range can be used for a custom type?
> >> >
> >> > I was thinking of doing it through `as_nanos()`, but it may read
> >> > worse, so please ignore it if so.
> >>
> >> Ah, it might work. The following doesn't work. Seems that we need to
> >> add another const like MAX_DELTA_NANOS or something. No strong
> >> preference but I feel the current is simpler.
> >>
> >> let delta = match delta.as_nanos() {
> >>     0..=MAX_DELTA.as_nanos() as i32 => delta,
> >>     _ => MAX_DELTA,
> >> };
> >
> > Could you do Delta::min(delta, MAX_DELTA).as_nanos() ?
>
> We need Delta type here so you meant:
>
> let delta = std::cmp::min(delta, MAX_DELTA);

If `Delta` implements the `Ord` trait, then you can write `Delta::min`
to take the minimum of two deltas. It's equivalent to `std::cmp::min`,
of course.

> We also need to convert a negative delta to MAX_DELTA so we could do:
>
> let delta = if delta.is_negative() {
>     MAX_DELTA
> } else {
>     min(delta, MAX_DELTA)
> };
>
> looks a bit readable than the original code?

At that point we might as well write

    if delta.is_negative() || delta > MAX_DELTA

and skip the call to `min`.

Alice

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[FUJITA Tomonori]

On Wed, 22 Jan 2025 11:47:38 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

>> >> Ah, it might work. The following doesn't work. Seems that we need to
>> >> add another const like MAX_DELTA_NANOS or something. No strong
>> >> preference but I feel the current is simpler.
>> >>
>> >> let delta = match delta.as_nanos() {
>> >>     0..=MAX_DELTA.as_nanos() as i32 => delta,
>> >>     _ => MAX_DELTA,
>> >> };
>> >
>> > Could you do Delta::min(delta, MAX_DELTA).as_nanos() ?
>>
>> We need Delta type here so you meant:
>>
>> let delta = std::cmp::min(delta, MAX_DELTA);
> 
> If `Delta` implements the `Ord` trait, then you can write `Delta::min`
> to take the minimum of two deltas. It's equivalent to `std::cmp::min`,
> of course.

Ah, nice.

>> We also need to convert a negative delta to MAX_DELTA so we could do:
>>
>> let delta = if delta.is_negative() {
>>     MAX_DELTA
>> } else {
>>     min(delta, MAX_DELTA)
>> };
>>
>> looks a bit readable than the original code?
> 
> At that point we might as well write
> 
>     if delta.is_negative() || delta > MAX_DELTA
> 
> and skip the call to `min`.

Yeah, it's what the original code does. I'll leave this code alone.

Re: [PATCH v8 3/7] rust: time: Introduce Instant type

[FUJITA Tomonori]

On Thu, 16 Jan 2025 21:06:44 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> On Thu, 16 Jan 2025 10:32:45 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
>>> -impl Ktime {
>>> -    /// Create a `Ktime` from a raw `ktime_t`.
>>> +impl Instant {
>>> +    /// Create a `Instant` from a raw `ktime_t`.
>>>      #[inline]
>>> -    pub fn from_raw(inner: bindings::ktime_t) -> Self {
>>> +    fn from_raw(inner: bindings::ktime_t) -> Self {
>>>          Self { inner }
>>>      }
>> 
>> Please keep this function public.
> 
> Surely, your driver uses from_raw()?

I checked out the C version of Binder driver and it doesn't seem like
the driver needs from_raw function. The Rust version [1] also doesn't
seem to need the function. Do you have a different use case?

https://r.android.com/3004103 [1]

Re: [PATCH v8 3/7] rust: time: Introduce Instant type

[Alice Ryhl]

On Wed, Jan 22, 2025 at 1:49 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Thu, 16 Jan 2025 21:06:44 +0900 (JST)
> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>
> > On Thu, 16 Jan 2025 10:32:45 +0100
> > Alice Ryhl <aliceryhl@google.com> wrote:
> >
> >>> -impl Ktime {
> >>> -    /// Create a `Ktime` from a raw `ktime_t`.
> >>> +impl Instant {
> >>> +    /// Create a `Instant` from a raw `ktime_t`.
> >>>      #[inline]
> >>> -    pub fn from_raw(inner: bindings::ktime_t) -> Self {
> >>> +    fn from_raw(inner: bindings::ktime_t) -> Self {
> >>>          Self { inner }
> >>>      }
> >>
> >> Please keep this function public.
> >
> > Surely, your driver uses from_raw()?
>
> I checked out the C version of Binder driver and it doesn't seem like
> the driver needs from_raw function. The Rust version [1] also doesn't
> seem to need the function. Do you have a different use case?

Not for this particular function, but I've changed functions called
from_raw and similar from private to public so many times at this
point that I think it should be the default.


Alice

Re: [PATCH v8 3/7] rust: time: Introduce Instant type

[FUJITA Tomonori]

On Wed, 22 Jan 2025 13:51:21 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

> On Wed, Jan 22, 2025 at 1:49 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> On Thu, 16 Jan 2025 21:06:44 +0900 (JST)
>> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>>
>> > On Thu, 16 Jan 2025 10:32:45 +0100
>> > Alice Ryhl <aliceryhl@google.com> wrote:
>> >
>> >>> -impl Ktime {
>> >>> -    /// Create a `Ktime` from a raw `ktime_t`.
>> >>> +impl Instant {
>> >>> +    /// Create a `Instant` from a raw `ktime_t`.
>> >>>      #[inline]
>> >>> -    pub fn from_raw(inner: bindings::ktime_t) -> Self {
>> >>> +    fn from_raw(inner: bindings::ktime_t) -> Self {
>> >>>          Self { inner }
>> >>>      }
>> >>
>> >> Please keep this function public.
>> >
>> > Surely, your driver uses from_raw()?
>>
>> I checked out the C version of Binder driver and it doesn't seem like
>> the driver needs from_raw function. The Rust version [1] also doesn't
>> seem to need the function. Do you have a different use case?
> 
> Not for this particular function, but I've changed functions called
> from_raw and similar from private to public so many times at this
> point that I think it should be the default.

Then can we remove from_raw()?

We don't use Instant to represent both a specific point in time and a
span of time (we add Delta) so a device driver don't need to create an
Instant from an arbitrary value, I think.

If we allow a device driver to create Instant via from_raw(), we need
to validate a value from the driver. If we create ktime_t only via
ktime_get(), we don't need the details of ktime like a valid range of
ktime_t.

Re: [PATCH v8 3/7] rust: time: Introduce Instant type

[Gary Guo]

On Wed, 22 Jan 2025 22:46:35 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> On Wed, 22 Jan 2025 13:51:21 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
> > On Wed, Jan 22, 2025 at 1:49 PM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:  
> >>
> >> On Thu, 16 Jan 2025 21:06:44 +0900 (JST)
> >> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> >>  
> >> > On Thu, 16 Jan 2025 10:32:45 +0100
> >> > Alice Ryhl <aliceryhl@google.com> wrote:
> >> >  
> >> >>> -impl Ktime {
> >> >>> -    /// Create a `Ktime` from a raw `ktime_t`.
> >> >>> +impl Instant {
> >> >>> +    /// Create a `Instant` from a raw `ktime_t`.
> >> >>>      #[inline]
> >> >>> -    pub fn from_raw(inner: bindings::ktime_t) -> Self {
> >> >>> +    fn from_raw(inner: bindings::ktime_t) -> Self {
> >> >>>          Self { inner }
> >> >>>      }  
> >> >>
> >> >> Please keep this function public.  
> >> >
> >> > Surely, your driver uses from_raw()?  
> >>
> >> I checked out the C version of Binder driver and it doesn't seem like
> >> the driver needs from_raw function. The Rust version [1] also doesn't
> >> seem to need the function. Do you have a different use case?  
> > 
> > Not for this particular function, but I've changed functions called
> > from_raw and similar from private to public so many times at this
> > point that I think it should be the default.  
> 
> Then can we remove from_raw()?
> 
> We don't use Instant to represent both a specific point in time and a
> span of time (we add Delta) so a device driver don't need to create an
> Instant from an arbitrary value, I think.
> 
> If we allow a device driver to create Instant via from_raw(), we need
> to validate a value from the driver. If we create ktime_t only via
> ktime_get(), we don't need the details of ktime like a valid range of
> ktime_t.

Yeah, I agree on this. If we specify the range 0..KTIME_MAX as
invariant then this either has to check or has to be unsafe. I don't
think that's worth adding unless there's a concrete need for it.

Best,
Gary

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[Gary Guo]

On Sat, 18 Jan 2025 17:02:24 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> On Fri, 17 Jan 2025 19:59:15 +0100
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> > On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:  
> >>
> >> +/// `delta` must be 0 or greater and no more than `u32::MAX / 2` microseconds.
> >> +/// If a value outside the range is given, the function will sleep
> >> +/// for `u32::MAX / 2` microseconds (= ~2147 seconds or ~36 minutes) at least.  
> > 
> > I would emphasize with something like:
> > 
> >     `delta` must be within [0, `u32::MAX / 2`] 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.  
> 
> Thanks, I'll use the above instead.
> 
> > In addition, I would add a new paragraph how the behavior differs
> > w.r.t. the C `fsleep()`, i.e. IIRC from the past discussions,
> > `fsleep()` would do an infinite sleep instead. So I think it is
> > important to highlight that.  
> 
> /// The above behavior differs from the kernel's [`fsleep`], which could sleep
> /// infinitely (for [`MAX_JIFFY_OFFSET`] jiffies).
> 
> Looks ok?
> 
> >> +    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
> >> +    // Considering that fsleep rounds up the duration to the nearest millisecond,
> >> +    // set the maximum value to u32::MAX / 2 microseconds.  
> > 
> > Nit: please use Markdown code spans in normal comments (no need for
> > intra-doc links there).  
> 
> Understood.
> 
> >> +    let duration = if delta > MAX_DURATION || delta.is_negative() {
> >> +        // TODO: add WARN_ONCE() when it's supported.  
> > 
> > Ditto (also "Add").  
> 
> Oops, I'll fix.
> 
> > By the way, can this be written differently maybe? e.g. using a range
> > since it is `const`?  
> 
> A range can be used for a custom type?

Yes, you can say `!(Delta::ZERO..MAX_DURATION).contains(&delta)`.
(You'll need to add `Delta::ZERO`).

The `start..end` syntax is a fancy way of constructing a
`core::ops::Range` which has `contains` as long as `PartialOrd` is
implemented.

> 
> > You can probably reuse `delta` as the new bindings name, since we
> > don't need the old one after this step.  
> 
> Do you mean something like the following?
> 
> const MAX_DELTA: Delta = Delta::from_micros(i32::MAX as i64);
> 
> let delta = if delta > MAX_DELTA || delta.is_negative() {
>     // TODO: Add WARN_ONCE() when it's supported.
>     MAX_DELTA
> } else {
>     delta
> };

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[Alice Ryhl]

On Wed, Jan 22, 2025 at 6:05 PM Gary Guo <gary@garyguo.net> wrote:
>
> On Sat, 18 Jan 2025 17:02:24 +0900 (JST)
> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>
> > On Fri, 17 Jan 2025 19:59:15 +0100
> > Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> >
> > > On Thu, Jan 16, 2025 at 5:42 AM FUJITA Tomonori
> > > <fujita.tomonori@gmail.com> wrote:
> > >>
> > >> +/// `delta` must be 0 or greater and no more than `u32::MAX / 2` microseconds.
> > >> +/// If a value outside the range is given, the function will sleep
> > >> +/// for `u32::MAX / 2` microseconds (= ~2147 seconds or ~36 minutes) at least.
> > >
> > > I would emphasize with something like:
> > >
> > >     `delta` must be within [0, `u32::MAX / 2`] 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.
> >
> > Thanks, I'll use the above instead.
> >
> > > In addition, I would add a new paragraph how the behavior differs
> > > w.r.t. the C `fsleep()`, i.e. IIRC from the past discussions,
> > > `fsleep()` would do an infinite sleep instead. So I think it is
> > > important to highlight that.
> >
> > /// The above behavior differs from the kernel's [`fsleep`], which could sleep
> > /// infinitely (for [`MAX_JIFFY_OFFSET`] jiffies).
> >
> > Looks ok?
> >
> > >> +    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
> > >> +    // Considering that fsleep rounds up the duration to the nearest millisecond,
> > >> +    // set the maximum value to u32::MAX / 2 microseconds.
> > >
> > > Nit: please use Markdown code spans in normal comments (no need for
> > > intra-doc links there).
> >
> > Understood.
> >
> > >> +    let duration = if delta > MAX_DURATION || delta.is_negative() {
> > >> +        // TODO: add WARN_ONCE() when it's supported.
> > >
> > > Ditto (also "Add").
> >
> > Oops, I'll fix.
> >
> > > By the way, can this be written differently maybe? e.g. using a range
> > > since it is `const`?
> >
> > A range can be used for a custom type?
>
> Yes, you can say `!(Delta::ZERO..MAX_DURATION).contains(&delta)`.
> (You'll need to add `Delta::ZERO`).

It would need to use ..= instead of .. to match the current check.

Alice

Re: [PATCH v8 6/7] rust: Add read_poll_timeout functions

[Gary Guo]

On Thu, 16 Jan 2025 13:40:58 +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.
> 
> 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.
> 
> core::panic::Location::file() doesn't provide a null-terminated string
> so add __might_sleep_precision() helper function, which takes a
> pointer to a string with its length.
> 
> 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]
> 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    | 28 +++++++++++---
>  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 | 84 ++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs     |  2 +
>  9 files changed, 144 insertions(+), 5 deletions(-)
>  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.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..da8e975d8e50
> --- /dev/null
> +++ b/rust/kernel/io/poll.rs
> @@ -0,0 +1,84 @@
> +// 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>(

I wonder if we can lift the `T: Copy` restriction and have `Cond` take
`&T` instead. I can see this being useful in many cases.

I know that quite often `T` is just an integer so you'd want to pass by
value, but I think almost always `Cond` is a very simple closure so
inlining would take place and they won't make a performance difference.

> +    mut op: Op,
> +    cond: Cond,
> +    sleep_delta: Delta,
> +    timeout_delta: Delta,
> +) -> Result<T>
> +where
> +    Op: FnMut() -> Result<T>,
> +    Cond: Fn(T) -> bool,
> +{
> +    let start = Instant::now();
> +    let sleep = !sleep_delta.is_zero();
> +    let timeout = !timeout_delta.is_zero();
> +
> +    might_sleep(Location::caller());

This should only be called if `timeout` is true?

> +
> +    let val = loop {
> +        let val = op()?;
> +        if cond(val) {
> +            // Unlike the C version, we immediately return.
> +            // We know a condition is met so we don't need to check again.
> +            return Ok(val);
> +        }
> +        if timeout && start.elapsed() > timeout_delta {
> +            // Should we return Err(ETIMEDOUT) here instead of call op() again
> +            // without a sleep between? But we follow the C version. op() could
> +            // take some time so might be worth checking again.
> +            break op()?;

Maybe the reason is `ktime_get` can take some time (due to its use of
seqlock and thus may require retrying?) Although this logic breaks down
when `read_poll_timeout_atomic` also has this extra `op(args)` despite
the condition being trivial.

So I really can't convince myself that this additional `op()` call is
needed. I can't think of any case where this behaviour would be
depended on by a driver, so I'd be tempted just to return ETIMEOUT
straight.

Regardless, even if you decide to keep it, you can hoist the `if
cond(val)` block below up or move the `op()` down, given that this is
the only place to break from the loop without returning.

> +        }
> +        if sleep {
> +            fsleep(sleep_delta);
> +        }
> +        // fsleep() could be busy-wait loop so we always call cpu_relax().
> +        cpu_relax();
> +    };
> +
> +    if cond(val) {
> +        Ok(val)
> +    } else {
> +        Err(ETIMEDOUT)
> +    }
> +}
> +
> +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 v8 7/7] net: phy: qt2025: Wait until PHY becomes ready

[Gary Guo]

On Thu, 16 Jan 2025 13:40:59 +0900
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> Wait until a PHY becomes ready in the probe callback by
> using read_poll_timeout function.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

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

> ---
>  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..f642831519ca 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(())
>      }
>  

Re: [PATCH v8 1/7] rust: time: Add PartialEq/Eq/PartialOrd/Ord trait to Ktime

[Gary Guo]

On Thu, 16 Jan 2025 13:40:53 +0900
FUJITA Tomonori <fujita.tomonori@gmail.com> 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>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

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

> ---
>  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,
>  }

Re: [PATCH v8 6/7] rust: Add read_poll_timeout functions

[Alice Ryhl]

On Wed, Jan 22, 2025 at 7:36 PM Gary Guo <gary@garyguo.net> wrote:
>
> On Thu, 16 Jan 2025 13:40:58 +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.
> >
> > 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.
> >
> > core::panic::Location::file() doesn't provide a null-terminated string
> > so add __might_sleep_precision() helper function, which takes a
> > pointer to a string with its length.
> >
> > 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]
> > 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    | 28 +++++++++++---
> >  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 | 84 ++++++++++++++++++++++++++++++++++++++++++
> >  rust/kernel/lib.rs     |  2 +
> >  9 files changed, 144 insertions(+), 5 deletions(-)
> >  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.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..da8e975d8e50
> > --- /dev/null
> > +++ b/rust/kernel/io/poll.rs
> > @@ -0,0 +1,84 @@
> > +// 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>(
>
> I wonder if we can lift the `T: Copy` restriction and have `Cond` take
> `&T` instead. I can see this being useful in many cases.
>
> I know that quite often `T` is just an integer so you'd want to pass by
> value, but I think almost always `Cond` is a very simple closure so
> inlining would take place and they won't make a performance difference.

Yeah, I think it should be

Cond: FnMut(&T) -> bool,

with FnMut as well.

Alice

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[FUJITA Tomonori]

On Wed, 22 Jan 2025 18:06:58 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

>> > >> +    let duration = if delta > MAX_DURATION || delta.is_negative() {
>> > >> +        // TODO: add WARN_ONCE() when it's supported.
>> > >
>> > > Ditto (also "Add").
>> >
>> > Oops, I'll fix.
>> >
>> > > By the way, can this be written differently maybe? e.g. using a range
>> > > since it is `const`?
>> >
>> > A range can be used for a custom type?
>>
>> Yes, you can say `!(Delta::ZERO..MAX_DURATION).contains(&delta)`.
>> (You'll need to add `Delta::ZERO`).
> 
> It would need to use ..= instead of .. to match the current check.

Neat, it works as follows.

let delta = if (Delta::ZERO..=MAX_DELTA).contains(&delta) {
    delta
} else {
    MAX_DELTA
};

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[FUJITA Tomonori]

On Wed, 22 Jan 2025 11:21:51 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> Not sure if we should say "Equivalent" given it is not exactly the
> same, but I am not a native speaker: I think it does not necessarily
> need to be exactly the same to be "equivalent", but perhaps "Similar
> to" or "Counterpart of" or something like that is better.

I'm not a native speaker either, but seems that "equivalent" can be
used as "functionally equivalent". The official Rust docs also use
"equivalent" in this sense, "Equivalent to C’s unsigned char type"

https://doc.rust-lang.org/std/os/raw/type.c_uchar.html

There are many places where "equivalent" is used in this sense in
rust/kernel/. Seems that only rust/kernel/block/mq.rs uses a different
word, "counterpart" in this sense.

Possibly another "good first issue" to make this expression in the
tree consistent?

>> Ah, it might work. The following doesn't work. Seems that we need to
>> add another const like MAX_DELTA_NANOS or something. No strong
>> preference but I feel the current is simpler.
>>
>> let delta = match delta.as_nanos() {
>>     0..=MAX_DELTA.as_nanos() as i32 => delta,
>>     _ => MAX_DELTA,
>> };
> 
> Yeah, don't worry about it too much :)
> 
> [ The language may get `const { ... }` to work there (it does in
> nightly) though it wouldn't look good either. I think the `as i32`
> would not be needed. ]
>
> By the way, speaking of something related, do we want to make some of
> the methods `fn`s be `const`?

Yeah. All the from_* functions are already const. Seems that making
as_* funcations (as_nanos, etc) const too make sense.

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[Boqun Feng]

On Wed, Jan 22, 2025 at 07:44:05PM +0900, FUJITA Tomonori wrote:
> On Wed, 22 Jan 2025 09:23:33 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
> 
> >> > I would also say "the C side [`fsleep()`] or similar"; in other words,
> >> > both are "kernel's" at this point.
> >>
> >> Agreed that "the C side" is better and updated the comment. I copied
> >> that expression from the existing code; there are many "kernel's" in
> >> rust/kernel/. "good first issues" for them?
> >>
> >> You prefer "[`fsleep()`]" rather than "[`fsleep`]"? I can't find any
> >> precedent for the C side functions.
> > 
> > I think that's a matter of taste. In the Rust ecosystem, fsleep is
> > more common, in the kernel ecosystem, fsleep() is more common. I've
> > seen both in Rust code at this point.
> 
> Understood, I'll go with [`fsleep`].
> 

I would suggest using [`fsleep()`], in the same spirit of this paragraph
in Documentation/process/maintainer-tip.rst:

"""
When a function is mentioned in the changelog, either the text body or the
subject line, please use the format 'function_name()'. Omitting the
brackets after the function name can be ambiguous::

  Subject: subsys/component: Make reservation_count static

  reservation_count is only used in reservation_stats. Make it static.

The variant with brackets is more precise::

  Subject: subsys/component: Make reservation_count() static

  reservation_count() is only called from reservation_stats(). Make it
  static.
"""

, since fsleep() falls into the areas of tip tree.

Regards,
Boqun

[...]

Re: [PATCH v8 6/7] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

On Wed, 22 Jan 2025 18:36:12 +0000
Gary Guo <gary@garyguo.net> wrote:

>> +#[track_caller]
>> +pub fn read_poll_timeout<Op, Cond, T: Copy>(
> 
> I wonder if we can lift the `T: Copy` restriction and have `Cond` take
> `&T` instead. I can see this being useful in many cases.
> 
> I know that quite often `T` is just an integer so you'd want to pass by
> value, but I think almost always `Cond` is a very simple closure so
> inlining would take place and they won't make a performance difference.

Yeah, we can. More handy for the users of this function. I'll do.

>> +    mut op: Op,
>> +    cond: Cond,
>> +    sleep_delta: Delta,
>> +    timeout_delta: Delta,
>> +) -> Result<T>
>> +where
>> +    Op: FnMut() -> Result<T>,
>> +    Cond: Fn(T) -> bool,
>> +{
>> +    let start = Instant::now();
>> +    let sleep = !sleep_delta.is_zero();
>> +    let timeout = !timeout_delta.is_zero();
>> +
>> +    might_sleep(Location::caller());
> 
> This should only be called if `timeout` is true?

Oops, I messed up this in v6 somehow. I'll fix.

>> +    let val = loop {
>> +        let val = op()?;
>> +        if cond(val) {
>> +            // Unlike the C version, we immediately return.
>> +            // We know a condition is met so we don't need to check again.
>> +            return Ok(val);
>> +        }
>> +        if timeout && start.elapsed() > timeout_delta {
>> +            // Should we return Err(ETIMEDOUT) here instead of call op() again
>> +            // without a sleep between? But we follow the C version. op() could
>> +            // take some time so might be worth checking again.
>> +            break op()?;
> 
> Maybe the reason is `ktime_get` can take some time (due to its use of
> seqlock and thus may require retrying?) Although this logic breaks down
> when `read_poll_timeout_atomic` also has this extra `op(args)` despite
> the condition being trivial.

ktime_get() might do retrying (read_seqcount) but compared to the op
function, I think that ktime_get() is fast (usually an op function
waits for hardware).

> So I really can't convince myself that this additional `op()` call is
> needed. I can't think of any case where this behaviour would be
> depended on by a driver, so I'd be tempted just to return ETIMEOUT
> straight.

As I commented in the code, I just mimic the logic of the C version,
which has been used for a long time. But as you said, looks like we
can return Err(ETIMEOUT) immediately here. I'll do that in the next
version.

Re: [PATCH v8 6/7] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

On Wed, 22 Jan 2025 21:14:00 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

>> > +#[track_caller]
>> > +pub fn read_poll_timeout<Op, Cond, T: Copy>(
>>
>> I wonder if we can lift the `T: Copy` restriction and have `Cond` take
>> `&T` instead. I can see this being useful in many cases.
>>
>> I know that quite often `T` is just an integer so you'd want to pass by
>> value, but I think almost always `Cond` is a very simple closure so
>> inlining would take place and they won't make a performance difference.
> 
> Yeah, I think it should be
> 
> Cond: FnMut(&T) -> bool,
> 
> with FnMut as well.

Yeah, less restriction is better. I changed the code as follows:

#[track_caller]
pub fn read_poll_timeout<Op, Cond, T>(
    mut op: Op,
    mut cond: Cond,
    sleep_delta: Delta,
    timeout_delta: Delta,
) -> Result<T>
where
    Op: FnMut() -> Result<T>,
    Cond: FnMut(&T) -> bool,
{

			  

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[FUJITA Tomonori]

On Wed, 22 Jan 2025 23:03:24 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Wed, Jan 22, 2025 at 07:44:05PM +0900, FUJITA Tomonori wrote:
>> On Wed, 22 Jan 2025 09:23:33 +0100
>> Alice Ryhl <aliceryhl@google.com> wrote:
>> 
>> >> > I would also say "the C side [`fsleep()`] or similar"; in other words,
>> >> > both are "kernel's" at this point.
>> >>
>> >> Agreed that "the C side" is better and updated the comment. I copied
>> >> that expression from the existing code; there are many "kernel's" in
>> >> rust/kernel/. "good first issues" for them?
>> >>
>> >> You prefer "[`fsleep()`]" rather than "[`fsleep`]"? I can't find any
>> >> precedent for the C side functions.
>> > 
>> > I think that's a matter of taste. In the Rust ecosystem, fsleep is
>> > more common, in the kernel ecosystem, fsleep() is more common. I've
>> > seen both in Rust code at this point.
>> 
>> Understood, I'll go with [`fsleep`].
>> 
> 
> I would suggest using [`fsleep()`], in the same spirit of this paragraph
> in Documentation/process/maintainer-tip.rst:
> 
> """
> When a function is mentioned in the changelog, either the text body or the
> subject line, please use the format 'function_name()'. Omitting the
> brackets after the function name can be ambiguous::
> 
>   Subject: subsys/component: Make reservation_count static
> 
>   reservation_count is only used in reservation_stats. Make it static.
> 
> The variant with brackets is more precise::
> 
>   Subject: subsys/component: Make reservation_count() static
> 
>   reservation_count() is only called from reservation_stats(). Make it
>   static.
> """
> 
> , since fsleep() falls into the areas of tip tree.

Thanks, I overlooked this. I had better to go with [`fsleep()`] then.

I think that using two styles under rust/kernel isn't ideal. Should we
make the [`function_name()`] style the preference under rust/kernel?

Unless there's a subsystem that prefers the [`function_name`] style,
that would be a different story.

Re: [PATCH v8 4/7] rust: time: Add wrapper for fsleep function

[Miguel Ojeda]

On Thu, Jan 23, 2025 at 9:40 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Thanks, I overlooked this. I had better to go with [`fsleep()`] then.
>
> I think that using two styles under rust/kernel isn't ideal. Should we
> make the [`function_name()`] style the preference under rust/kernel?

Sounds good to me. I have a guidelines series on the backlog, so I can
add it there.

Though there is a question about whether we should do the same for
Rust ones or not. For Rust, it is a bit clearer already since it uses
e.g. CamelCase for types.

> Unless there's a subsystem that prefers the [`function_name`] style,
> that would be a different story.

I think we have a good chance to try to make things consistent across
the kernel (for Rust) wherever possible, like for formatting, since
all the code is new, i.e. to try to avoid variations between
subsystems, even if in the C side had them originally.

Exceptions as usual may be needed, but unless there is a good reason,
I think we should try to stay consistent.

Cheers,
Miguel

Re: [PATCH v8 3/7] rust: time: Introduce Instant type

[FUJITA Tomonori]

On Sat, 18 Jan 2025 13:15:42 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Fri, Jan 17, 2025 at 12:31 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> As I wrote to Tom, that's the kernel's assumption. Do we need to make
>> it an invariant too?
>>
>> Or improving the above "Range from 0 to `KTIME_MAX.`" is enough?
>>
>> The kernel assumes that the range of the ktime_t type is from 0 to
>> KTIME_MAX. The ktime APIs guarantees to give a valid ktime_t.
> 
> It depends on what is best for users, i.e. if there are no use cases
> where this needs to be negative, then why wouldn't we have the
> invariant documented? Or do we want to make it completely opaque?

Instant object is always created via ktime_get() so it shouldn't be
negative. ktime_t is opaque for users. However, we support creating a
Delta object from the difference between two Instance objects:

Delta = Instant1 - Instant2

It's a subtraction of two s64 types so to prevent overflow, the range
of ktime_t needs to be limited.

I'll add the invariant doc. I'm not sure if an invariant document
is the best choice, but in any case, the above information should be
documented.