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

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

[FUJITA Tomonori]

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

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

The remaining patches are for the Rust portion and updates to the
MAINTAINERS file.

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

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

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

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

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

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


base-commit: beeb78d46249cab8b2b8359a2ce8fa5376b5ad2d
-- 
2.43.0

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

[FUJITA Tomonori]

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

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

Modify __might_sleep() to call __might_sleep_precision() but the
impact should be negligible. When printing the error (sleeping
function called from invalid context), the precision string format is
used instead of the simple string format; the precision specifies the
the maximum length of the displayed string.

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

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

Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
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    | 61 +++++++++++++++++++++++++++---------------
 2 files changed, 42 insertions(+), 21 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index be2e8c0a187e..086ee1dc447e 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -87,6 +87,7 @@ extern int dynamic_might_resched(void);
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
 extern void __might_resched(const char *file, int line, unsigned int offsets);
 extern void __might_sleep(const char *file, int line);
+extern void __might_sleep_precision(const char *file, int len, int line);
 extern void __cant_sleep(const char *file, int line, int preempt_offset);
 extern void __cant_migrate(const char *file, int line);
 
@@ -145,6 +146,7 @@ extern void __cant_migrate(const char *file, int line);
   static inline void __might_resched(const char *file, int line,
 				     unsigned int offsets) { }
 static inline void __might_sleep(const char *file, int line) { }
+static inline void __might_sleep_precision(const char *file, int len, int line) { }
 # define might_sleep() do { might_resched(); } while (0)
 # define cant_sleep() do { } while (0)
 # define cant_migrate()		do { } while (0)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 165c90ba64ea..6643e03eafa4 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8678,24 +8678,6 @@ void __init sched_init(void)
 
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
 
-void __might_sleep(const char *file, int line)
-{
-	unsigned int state = get_current_state();
-	/*
-	 * Blocking primitives will set (and therefore destroy) current->state,
-	 * since we will exit with TASK_RUNNING make sure we enter with it,
-	 * otherwise we will destroy state.
-	 */
-	WARN_ONCE(state != TASK_RUNNING && current->task_state_change,
-			"do not call blocking ops when !TASK_RUNNING; "
-			"state=%x set at [<%p>] %pS\n", state,
-			(void *)current->task_state_change,
-			(void *)current->task_state_change);
-
-	__might_resched(file, line, 0);
-}
-EXPORT_SYMBOL(__might_sleep);
-
 static void print_preempt_disable_ip(int preempt_offset, unsigned long ip)
 {
 	if (!IS_ENABLED(CONFIG_DEBUG_PREEMPT))
@@ -8717,7 +8699,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 file_len, int line,
+				      unsigned int offsets)
 {
 	/* Ratelimiting timestamp: */
 	static unsigned long prev_jiffy;
@@ -8740,8 +8723,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",
+	       file_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);
@@ -8766,8 +8749,44 @@ void __might_resched(const char *file, int line, unsigned int offsets)
 	dump_stack();
 	add_taint(TAINT_WARN, LOCKDEP_STILL_OK);
 }
+
+/*
+ * The precision in vsnprintf() specifies the maximum length of the
+ * displayed string. The precision needs to be larger than the actual
+ * length of the string, so a sufficiently large value should be used
+ * for the filename length.
+ */
+#define MAX_FILENAME_LEN (1<<14)
+
+void __might_resched(const char *file, int line, unsigned int offsets)
+{
+	__might_resched_precision(file, MAX_FILENAME_LEN, line, offsets);
+}
 EXPORT_SYMBOL(__might_resched);
 
+void __might_sleep_precision(const char *file, int len, int line)
+{
+	unsigned int state = get_current_state();
+	/*
+	 * Blocking primitives will set (and therefore destroy) current->state,
+	 * since we will exit with TASK_RUNNING make sure we enter with it,
+	 * otherwise we will destroy state.
+	 */
+	WARN_ONCE(state != TASK_RUNNING && current->task_state_change,
+			"do not call blocking ops when !TASK_RUNNING; "
+			"state=%x set at [<%p>] %pS\n", state,
+			(void *)current->task_state_change,
+			(void *)current->task_state_change);
+
+	__might_resched_precision(file, len, line, 0);
+}
+
+void __might_sleep(const char *file, int line)
+{
+	__might_sleep_precision(file, MAX_FILENAME_LEN, line);
+}
+EXPORT_SYMBOL(__might_sleep);
+
 void __cant_sleep(const char *file, int line, int preempt_offset)
 {
 	static unsigned long prev_jiffy;
-- 
2.43.0

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

[FUJITA Tomonori]

On Thu, 20 Feb 2025 16:06:03 +0900
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> Add __might_sleep_precision(), Rust friendly version of
> __might_sleep(), which takes a pointer to a string with the length
> instead of a null-terminated string.
> 
> Rust's core::panic::Location::file(), which gives the file name of a
> caller, doesn't provide a null-terminated
> string. __might_sleep_precision() uses a precision specifier in the
> printk format, which specifies the length of a string; a string
> doesn't need to be a null-terminated.
> 
> Modify __might_sleep() to call __might_sleep_precision() but the
> impact should be negligible. When printing the error (sleeping
> function called from invalid context), the precision string format is
> used instead of the simple string format; the precision specifies the
> the maximum length of the displayed string.
> 
> Note that Location::file() providing a null-terminated string for
> better C interoperability is under discussion [1].
> 
> [1]: https://github.com/rust-lang/libs-team/issues/466
> 
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> 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    | 61 +++++++++++++++++++++++++++---------------
>  2 files changed, 42 insertions(+), 21 deletions(-)

SCHEDULER maintainers,

Can I get an ack for this? Or you prefer a different approach?

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

[FUJITA Tomonori]

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

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

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

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

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

[Andreas Hindborg]

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:

> 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).
>
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Reviewed-by: Trevor Gross <tmgross@umich.edu>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Gary Guo <gary@garyguo.net>
> Reviewed-by: Fiona Behrens <me@kloenk.dev>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

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

[FUJITA Tomonori]

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

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

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

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

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

Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Fiona Behrens <me@kloenk.dev>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time.rs | 88 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

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

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

[Andreas Hindborg]

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:

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


Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>

Two suggestions below, take or leave.

> ---
>  rust/kernel/time.rs | 88 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 88 insertions(+)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 48b71e6641ce..622cd01e24d7 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -8,9 +8,15 @@
>  //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
>  //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
>  
> +/// The number of nanoseconds per microsecond.
> +pub const NSEC_PER_USEC: i64 = bindings::NSEC_PER_USEC as i64;
> +
>  /// The number of nanoseconds per millisecond.
>  pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
>  
> +/// The number of nanoseconds per second.
> +pub const NSEC_PER_SEC: i64 = bindings::NSEC_PER_SEC as i64;
> +
>  /// The time unit of Linux kernel. One jiffy equals (1/HZ) second.
>  pub type Jiffies = crate::ffi::c_ulong;
>  
> @@ -81,3 +87,85 @@ fn sub(self, other: Ktime) -> Ktime {
>          }
>      }
>  }
> +
> +/// A span of time.
> +///
> +/// This struct represents a span of time, with its value stored as nanoseconds.
> +/// The value can represent any valid i64 value, including negative, zero, and
> +/// positive numbers.
> +#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
> +pub struct Delta {
> +    nanos: i64,
> +}
> +
> +impl Delta {
> +    /// A span of time equal to zero.
> +    pub const ZERO: Self = Self { nanos: 0 };
> +
> +    /// Create a new [`Delta`] from a number of microseconds.
> +    ///
> +    /// The `micros` can range from -9_223_372_036_854_775 to 9_223_372_036_854_775.

To make these numbers truly useful, it would be nice to have them as
constants, for example `Delta::MAX_MICROS`, `Delta::MIN_MICROS`.

> +    /// If `micros` is outside this range, `i64::MIN` is used for negative values,
> +    /// and `i64::MAX` is used for positive values due to saturation.
> +    #[inline]
> +    pub const fn from_micros(micros: i64) -> Self {
> +        Self {
> +            nanos: micros.saturating_mul(NSEC_PER_USEC),
> +        }
> +    }
> +
> +    /// Create a new [`Delta`] from a number of milliseconds.
> +    ///
> +    /// The `millis` can range from -9_223_372_036_854 to 9_223_372_036_854.
> +    /// If `millis` is outside this range, `i64::MIN` is used for negative values,
> +    /// and `i64::MAX` is used for positive values due to saturation.
> +    #[inline]
> +    pub const fn from_millis(millis: i64) -> Self {
> +        Self {
> +            nanos: millis.saturating_mul(NSEC_PER_MSEC),
> +        }
> +    }
> +
> +    /// Create a new [`Delta`] from a number of seconds.
> +    ///
> +    /// The `secs` can range from -9_223_372_036 to 9_223_372_036.
> +    /// If `secs` is outside this range, `i64::MIN` is used for negative values,
> +    /// and `i64::MAX` is used for positive values due to saturation.
> +    #[inline]
> +    pub const fn from_secs(secs: i64) -> Self {
> +        Self {
> +            nanos: secs.saturating_mul(NSEC_PER_SEC),
> +        }
> +    }
> +
> +    /// Return `true` if the [`Delta`] spans no time.
> +    #[inline]
> +    pub fn is_zero(self) -> bool {
> +        self.as_nanos() == 0
> +    }
> +
> +    /// Return `true` if the [`Delta`] spans a negative amount of time.
> +    #[inline]
> +    pub fn is_negative(self) -> bool {
> +        self.as_nanos() < 0
> +    }
> +
> +    /// Return the number of nanoseconds in the [`Delta`].
> +    #[inline]
> +    pub const fn as_nanos(self) -> i64 {
> +        self.nanos
> +    }
> +
> +    /// Return the smallest number of microseconds greater than or equal
> +    /// to the value in the [`Delta`].
> +    #[inline]
> +    pub const fn as_micros_ceil(self) -> i64 {
> +        self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
> +    }
> +
> +    /// Return the number of milliseconds in the [`Delta`].

We might consider adding "rounded towards zero" to this doc string.


Best regards,
Andreas Hindborg

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

[FUJITA Tomonori]

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

Rename Ktime to Instant and modify their methods for timestamp.

Implement the subtraction operator for Instant:

Delta = Instant A - Instant B

Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Fiona Behrens <me@kloenk.dev>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time.rs | 77 +++++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 38 deletions(-)

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

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

[Andreas Hindborg]

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:

> 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
>
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Reviewed-by: Gary Guo <gary@garyguo.net>
> Reviewed-by: Fiona Behrens <me@kloenk.dev>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>


Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>


As Boqun mentioned, we should make this generic over `ClockId` when the
hrtimer patches land.

One question regarding overflow below.

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

If this never overflows by invariant, would it make sense to use
`unchecked_sub` or `wraping_sub`? That would remove the overflow check.


Best regards,
Andreas Hindborg

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

[FUJITA Tomonori]

On Sat, 22 Mar 2025 14:58:16 +0100
Andreas Hindborg <a.hindborg@kernel.org> wrote:

> FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
> 
>> 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
>>
>> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
>> Reviewed-by: Gary Guo <gary@garyguo.net>
>> Reviewed-by: Fiona Behrens <me@kloenk.dev>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> 
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> 
> 
> As Boqun mentioned, we should make this generic over `ClockId` when the
> hrtimer patches land.

Seems that I overlooked his mail. Can you give me a pointer?

I assume that you want the Instance type to vary depending on the
clock source.


>> -/// Returns the number of milliseconds between two ktimes.
>> -#[inline]
>> -pub fn ktime_ms_delta(later: Ktime, earlier: Ktime) -> i64 {
>> -    (later - earlier).to_ms()
>> -}
>> -
>> -impl core::ops::Sub for Ktime {
>> -    type Output = Ktime;
>> +impl core::ops::Sub for Instant {
>> +    type Output = Delta;
>>  
>> +    // By the type invariant, it never overflows.
>>      #[inline]
>> -    fn sub(self, other: Ktime) -> Ktime {
>> -        Self {
>> -            inner: self.inner - other.inner,
>> +    fn sub(self, other: Instant) -> Delta {
>> +        Delta {
>> +            nanos: self.inner - other.inner,
> 
> If this never overflows by invariant, would it make sense to use
> `unchecked_sub` or `wraping_sub`? That would remove the overflow check.

Yeah, I think that it can. But I prefer to keep the code as is to
catch a bug.

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

[Andreas Hindborg]

"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:

> On Sat, 22 Mar 2025 14:58:16 +0100
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> FUJITA Tomonori <fujita.tomonori@gmail.com> writes:
>>
>>> 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
>>>
>>> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
>>> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
>>> Reviewed-by: Gary Guo <gary@garyguo.net>
>>> Reviewed-by: Fiona Behrens <me@kloenk.dev>
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>
>>
>> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
>>
>>
>> As Boqun mentioned, we should make this generic over `ClockId` when the
>> hrtimer patches land.
>
> Seems that I overlooked his mail. Can you give me a pointer?
>
> I assume that you want the Instance type to vary depending on the
> clock source.

Yea, basically it is only okay to subtract instants if they are derived
from the same clock source. Boqun suggested here [1] before hrtimer
patches landed I think.

At any rate, now we have `kernel::time::ClockId`. It is an enum though,
so I am not sure how to go about it in practice. But we would want
`Instant<RealTime> - Instant<BootTime>` to give a compiler error.


Best regards,
Andreas Hindborg


[1] https://lore.kernel.org/all/ZxwFyl0xIje5gv7J@Boquns-Mac-mini.local

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

[FUJITA Tomonori]

On Thu, 03 Apr 2025 12:41:52 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

>>> As Boqun mentioned, we should make this generic over `ClockId` when the
>>> hrtimer patches land.
>>
>> Seems that I overlooked his mail. Can you give me a pointer?
>>
>> I assume that you want the Instance type to vary depending on the
>> clock source.
> 
> Yea, basically it is only okay to subtract instants if they are derived
> from the same clock source. Boqun suggested here [1] before hrtimer
> patches landed I think.
> 
> At any rate, now we have `kernel::time::ClockId`. It is an enum though,
> so I am not sure how to go about it in practice. But we would want
> `Instant<RealTime> - Instant<BootTime>` to give a compiler error.

Understood, thanks!

If we need a compile error for this, I don't think that the enum
works; I guess that we need a distinct type for each of RealTime,
BootTime, and so on.

Once the current patchset is merged, I'll work on that.

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

[FUJITA Tomonori]

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

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

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

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

Link: https://rust-for-linux.com/klint [1]
Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Fiona Behrens <me@kloenk.dev>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers/helpers.c    |  1 +
 rust/helpers/time.c       |  8 +++++++
 rust/kernel/time.rs       |  2 ++
 rust/kernel/time/delay.rs | 49 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 60 insertions(+)
 create mode 100644 rust/helpers/time.c
 create mode 100644 rust/kernel/time/delay.rs

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

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

[Frederic Weisbecker]

Le Thu, Feb 20, 2025 at 04:06:07PM +0900, FUJITA Tomonori a écrit :
> 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]
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Reviewed-by: Gary Guo <gary@garyguo.net>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Fiona Behrens <me@kloenk.dev>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Sorry to make a late review. I don't mean to delay that any further
but:

> +/// `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.

And very important: the behaviour also differ in that the C side takes
usecs while this takes nsecs. We should really disambiguate the situation
as that might create confusion or misusage.

Either this should be renamed to fsleep_ns() or fsleep_nsecs(), or this should
take microseconds directly.

Thanks.

> +///
> +/// This function can only be used in a nonatomic context.
> +///
> +/// [`fsleep`]: https://docs.kernel.org/timers/delay_sleep_functions.html#c.fsleep
> +pub fn fsleep(delta: Delta) {
> +    // The maximum value is set to `i32::MAX` microseconds to prevent integer
> +    // overflow inside fsleep, which could lead to unintentional infinite sleep.
> +    const MAX_DELTA: Delta = Delta::from_micros(i32::MAX as i64);
> +
> +    let delta = if (Delta::ZERO..=MAX_DELTA).contains(&delta) {
> +        delta
> +    } else {
> +        // TODO: Add WARN_ONCE() when it's supported.
> +        MAX_DELTA
> +    };
> +
> +    // SAFETY: It is always safe to call `fsleep()` with any duration.
> +    unsafe {
> +        // Convert the duration to microseconds and round up to preserve
> +        // the guarantee; `fsleep()` sleeps for at least the provided duration,
> +        // but that it may sleep for longer under some circumstances.
> +        bindings::fsleep(delta.as_micros_ceil() as c_ulong)
> +    }
> +}
> -- 
> 2.43.0
> 

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

[FUJITA Tomonori]

On Fri, 21 Mar 2025 23:05:23 +0100
Frederic Weisbecker <frederic@kernel.org> wrote:

> Le Thu, Feb 20, 2025 at 04:06:07PM +0900, FUJITA Tomonori a écrit :
>> 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]
>> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Reviewed-by: Gary Guo <gary@garyguo.net>
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> Reviewed-by: Fiona Behrens <me@kloenk.dev>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Sorry to make a late review. I don't mean to delay that any further
> but:

No problem at all. Thanks for reviewing!


>> +/// `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.
> 
> And very important: the behaviour also differ in that the C side takes
> usecs while this takes nsecs. We should really disambiguate the situation
> as that might create confusion or misusage.
> 
> Either this should be renamed to fsleep_ns() or fsleep_nsecs(), or this should
> take microseconds directly.

You meant that `Delta` type internally tracks time in nanoseconds?

It's true but Delta type is a unit-agnostic time abstraction, designed
to represent durations across different granularities — seconds,
milliseconds, microseconds, nanoseconds. The Rust abstraction always
tries to us Delta type to represent durations.

Rust's fsleep takes Delta, internally converts it in usecs, and calls
C's fsleep.

Usually, drivers convert from a certain time unit to Delta before
calling fsleep like the following, so misuse or confusion is unlikely
to occur, I think.

fsleep(Delta::from_micros(50));

However, as you pointed out, there is a difference; C's fsleep takes
usecs while Rust's fsleep takes a unit-agnostic time type. Taking this
difference into account, if we were to rename fsleep for Rust, I think
that a name that is agnostic to the time unit would seem more
appropriate. Simply sleep(), perhaps?

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

[Andreas Hindborg]

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:

> On Fri, 21 Mar 2025 23:05:23 +0100
> Frederic Weisbecker <frederic@kernel.org> wrote:
>

[...]

>>> +/// `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.
>> 
>> And very important: the behaviour also differ in that the C side takes
>> usecs while this takes nsecs. We should really disambiguate the situation
>> as that might create confusion or misusage.
>> 
>> Either this should be renamed to fsleep_ns() or fsleep_nsecs(), or this should
>> take microseconds directly.
>
> You meant that `Delta` type internally tracks time in nanoseconds?
>
> It's true but Delta type is a unit-agnostic time abstraction, designed
> to represent durations across different granularities — seconds,
> milliseconds, microseconds, nanoseconds. The Rust abstraction always
> tries to us Delta type to represent durations.
>
> Rust's fsleep takes Delta, internally converts it in usecs, and calls
> C's fsleep.
>
> Usually, drivers convert from a certain time unit to Delta before
> calling fsleep like the following, so misuse or confusion is unlikely
> to occur, I think.
>
> fsleep(Delta::from_micros(50));
>
> However, as you pointed out, there is a difference; C's fsleep takes
> usecs while Rust's fsleep takes a unit-agnostic time type. Taking this
> difference into account, if we were to rename fsleep for Rust, I think
> that a name that is agnostic to the time unit would seem more
> appropriate. Simply sleep(), perhaps?

I would prefer to keep the same name. But if we must change it, how
about `flexible_sleep` to indicate the behavior?

And just to reiterate what Tomonori already said, it is not possible to
call this method with an integer,

  fsleep(500)

will not compile. A unit based conversion into `Duration` is required.


Best regards,
Andreas Hindborg

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

[Andreas Hindborg]

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:

> 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]
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Reviewed-by: Gary Guo <gary@garyguo.net>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Fiona Behrens <me@kloenk.dev>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

[PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[FUJITA Tomonori]

Add new sections for DELAY/SLEEP and TIMEKEEPING abstractions
respectively. It was possible to include both abstractions in a single
section, but following precedent, two sections were created to
correspond with the entries for the C language APIs.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index c8d9e8187eb0..775ea845f011 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10355,6 +10355,13 @@ F:	kernel/time/timer_list.c
 F:	kernel/time/timer_migration.*
 F:	tools/testing/selftests/timers/
 
+DELAY AND SLEEP API [RUST]
+M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
+L:	rust-for-linux@vger.kernel.org
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	rust/kernel/time/delay.rs
+
 HIGH-SPEED SCC DRIVER FOR AX.25
 L:	linux-hams@vger.kernel.org
 S:	Orphan
@@ -23854,6 +23861,13 @@ F:	kernel/time/timekeeping*
 F:	kernel/time/time_test.c
 F:	tools/testing/selftests/timers/
 
+TIMEKEEPING API [RUST]
+M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
+L:	rust-for-linux@vger.kernel.org
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	rust/kernel/time.rs
+
 TIPC NETWORK LAYER
 M:	Jon Maloy <jmaloy@redhat.com>
 L:	netdev@vger.kernel.org (core kernel code)
-- 
2.43.0

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[Boqun Feng]

Hi Tomo,

On Thu, Feb 20, 2025 at 04:06:08PM +0900, FUJITA Tomonori wrote:
> Add new sections for DELAY/SLEEP and TIMEKEEPING abstractions
> respectively. It was possible to include both abstractions in a single
> section, but following precedent, two sections were created to
> correspond with the entries for the C language APIs.
> 

Could you add me as a reviewer in these entries?

Thanks!

Regards,
Boqun

> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  MAINTAINERS | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c8d9e8187eb0..775ea845f011 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10355,6 +10355,13 @@ F:	kernel/time/timer_list.c
>  F:	kernel/time/timer_migration.*
>  F:	tools/testing/selftests/timers/
>  
> +DELAY AND SLEEP API [RUST]
> +M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
+R:Boqun Feng <boqun.feng@gmail.com>
> +L:	rust-for-linux@vger.kernel.org
> +L:	linux-kernel@vger.kernel.org
> +S:	Maintained
> +F:	rust/kernel/time/delay.rs
> +
>  HIGH-SPEED SCC DRIVER FOR AX.25
>  L:	linux-hams@vger.kernel.org
>  S:	Orphan
> @@ -23854,6 +23861,13 @@ F:	kernel/time/timekeeping*
>  F:	kernel/time/time_test.c
>  F:	tools/testing/selftests/timers/
>  
> +TIMEKEEPING API [RUST]
> +M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
+R:Boqun Feng <boqun.feng@gmail.com>
> +L:	rust-for-linux@vger.kernel.org
> +L:	linux-kernel@vger.kernel.org
> +S:	Maintained
> +F:	rust/kernel/time.rs
> +
>  TIPC NETWORK LAYER
>  M:	Jon Maloy <jmaloy@redhat.com>
>  L:	netdev@vger.kernel.org (core kernel code)
> -- 
> 2.43.0
> 
> 

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[Andreas Hindborg]

Boqun Feng <boqun.feng@gmail.com> writes:

> Hi Tomo,
>
> On Thu, Feb 20, 2025 at 04:06:08PM +0900, FUJITA Tomonori wrote:
>> Add new sections for DELAY/SLEEP and TIMEKEEPING abstractions
>> respectively. It was possible to include both abstractions in a single
>> section, but following precedent, two sections were created to
>> correspond with the entries for the C language APIs.
>> 
>
> Could you add me as a reviewer in these entries?
>

I would like to be added as well.


Best regards,
Andreas Hindborg

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[Thomas Gleixner]

On Fri, Mar 21 2025 at 20:18, Andreas Hindborg wrote:
>> Could you add me as a reviewer in these entries?
>>
>
> I would like to be added as well.

Please add the relevant core code maintainers (Anna-Maria, Frederic,
John Stultz and myself) as well to the reviewers list, so that this does
not end up with changes going in opposite directions.

Thanks,

        tglx

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[Boqun Feng]

On Fri, Mar 21, 2025 at 09:38:46PM +0100, Thomas Gleixner wrote:
> On Fri, Mar 21 2025 at 20:18, Andreas Hindborg wrote:
> >> Could you add me as a reviewer in these entries?
> >>
> >
> > I would like to be added as well.
> 
> Please add the relevant core code maintainers (Anna-Maria, Frederic,
> John Stultz and myself) as well to the reviewers list, so that this does
> not end up with changes going in opposite directions.
> 

Make sense, I assume you want this to go via rust then (althought we
would like it to go via your tree if possible ;-))?

Regards,
Boqun

> Thanks,
> 
>         tglx

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[FUJITA Tomonori]

Thank you all!

On Fri, 21 Mar 2025 14:00:52 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Fri, Mar 21, 2025 at 09:38:46PM +0100, Thomas Gleixner wrote:
>> On Fri, Mar 21 2025 at 20:18, Andreas Hindborg wrote:
>> >> Could you add me as a reviewer in these entries?
>> >>
>> >
>> > I would like to be added as well.
>> 
>> Please add the relevant core code maintainers (Anna-Maria, Frederic,
>> John Stultz and myself) as well to the reviewers list, so that this does
>> not end up with changes going in opposite directions.
>> 
> 
> Make sense, I assume you want this to go via rust then (althought we
> would like it to go via your tree if possible ;-))?

Once the following review regarding fsleep() is complete, I will submit
patches #2 through #6 as v12 for rust-next:

https://lore.kernel.org/linux-kernel/20250322.102449.895174336060649075.fujita.tomonori@gmail.com/

The updated MAINTAINERS file will look like the following.

diff --git a/MAINTAINERS b/MAINTAINERS
index cbf84690c495..858e0b34422f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10370,6 +10370,18 @@ F:	kernel/time/timer_list.c
 F:	kernel/time/timer_migration.*
 F:	tools/testing/selftests/timers/
 
+DELAY AND SLEEP API [RUST]
+M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
+R:	Boqun Feng <boqun.feng@gmail.com>
+R:	Andreas Hindborg <a.hindborg@kernel.org>
+R:	Anna-Maria Behnsen <anna-maria@linutronix.de>
+R:	Frederic Weisbecker <frederic@kernel.org>
+R:	Thomas Gleixner <tglx@linutronix.de>
+L:	rust-for-linux@vger.kernel.org
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	rust/kernel/time/delay.rs
+
 HIGH-SPEED SCC DRIVER FOR AX.25
 L:	linux-hams@vger.kernel.org
 S:	Orphan
@@ -23944,6 +23956,17 @@ F:	kernel/time/timekeeping*
 F:	kernel/time/time_test.c
 F:	tools/testing/selftests/timers/
 
+TIMEKEEPING API [RUST]
+M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
+R:	Boqun Feng <boqun.feng@gmail.com>
+R:	Andreas Hindborg <a.hindborg@kernel.org>
+R:	John Stultz <jstultz@google.com>
+R:	Thomas Gleixner <tglx@linutronix.de>
+L:	rust-for-linux@vger.kernel.org
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+F:	rust/kernel/time.rs
+
 TIPC NETWORK LAYER
 M:	Jon Maloy <jmaloy@redhat.com>
 L:	netdev@vger.kernel.org (core kernel code)

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[Boqun Feng]

On Sat, Mar 22, 2025 at 11:07:03AM +0900, FUJITA Tomonori wrote:
> Thank you all!
> 
> On Fri, 21 Mar 2025 14:00:52 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Fri, Mar 21, 2025 at 09:38:46PM +0100, Thomas Gleixner wrote:
> >> On Fri, Mar 21 2025 at 20:18, Andreas Hindborg wrote:
> >> >> Could you add me as a reviewer in these entries?
> >> >>
> >> >
> >> > I would like to be added as well.
> >> 
> >> Please add the relevant core code maintainers (Anna-Maria, Frederic,
> >> John Stultz and myself) as well to the reviewers list, so that this does
> >> not end up with changes going in opposite directions.
> >> 
> > 
> > Make sense, I assume you want this to go via rust then (althought we
> > would like it to go via your tree if possible ;-))?
> 

Given Andreas is already preparing the pull request of the hrtimer
abstraction to Miguel, and delay, timekeeping and hrtimer are related,
these timekeeping/delay patches should go via Andreas (i.e.
rust/hrtimer-next into rust/rust-next) if Thomas and Miguel are OK with
it. Works for you, Andreas? If so...

> Once the following review regarding fsleep() is complete, I will submit
> patches #2 through #6 as v12 for rust-next:
> 
> https://lore.kernel.org/linux-kernel/20250322.102449.895174336060649075.fujita.tomonori@gmail.com/
> 
> The updated MAINTAINERS file will look like the following.
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cbf84690c495..858e0b34422f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10370,6 +10370,18 @@ F:	kernel/time/timer_list.c
>  F:	kernel/time/timer_migration.*
>  F:	tools/testing/selftests/timers/
>  
> +DELAY AND SLEEP API [RUST]
> +M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
> +R:	Boqun Feng <boqun.feng@gmail.com>
> +R:	Andreas Hindborg <a.hindborg@kernel.org>

... this "R:" entry would be "M:",

> +R:	Anna-Maria Behnsen <anna-maria@linutronix.de>
> +R:	Frederic Weisbecker <frederic@kernel.org>
> +R:	Thomas Gleixner <tglx@linutronix.de>
> +L:	rust-for-linux@vger.kernel.org
> +L:	linux-kernel@vger.kernel.org

+T:	git https://github.com/Rust-for-Linux/linux.git hrtimer-next

> +S:	Maintained

I will let Andreas decide whether this is a "Supported" entry ;-)

> +F:	rust/kernel/time/delay.rs
> +
>  HIGH-SPEED SCC DRIVER FOR AX.25
>  L:	linux-hams@vger.kernel.org
>  S:	Orphan
> @@ -23944,6 +23956,17 @@ F:	kernel/time/timekeeping*
>  F:	kernel/time/time_test.c
>  F:	tools/testing/selftests/timers/
>  
> +TIMEKEEPING API [RUST]

and similar things for this entry as well.

> +M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
> +R:	Boqun Feng <boqun.feng@gmail.com>
> +R:	Andreas Hindborg <a.hindborg@kernel.org>
> +R:	John Stultz <jstultz@google.com>
> +R:	Thomas Gleixner <tglx@linutronix.de>

+R:      Stephen Boyd <sboyd@kernel.org>

?

> +L:	rust-for-linux@vger.kernel.org
> +L:	linux-kernel@vger.kernel.org
> +S:	Maintained
> +F:	rust/kernel/time.rs
> +

Tomo, let's wait for Andreas' rely and decide how to change these
entries. Thanks!

Regards,
Boqun

>  TIPC NETWORK LAYER
>  M:	Jon Maloy <jmaloy@redhat.com>
>  L:	netdev@vger.kernel.org (core kernel code)

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[Andreas Hindborg]

Hi All,

"Boqun Feng" <boqun.feng@gmail.com> writes:

> On Sat, Mar 22, 2025 at 11:07:03AM +0900, FUJITA Tomonori wrote:
>> Thank you all!
>>
>> On Fri, 21 Mar 2025 14:00:52 -0700
>> Boqun Feng <boqun.feng@gmail.com> wrote:
>>
>> > On Fri, Mar 21, 2025 at 09:38:46PM +0100, Thomas Gleixner wrote:
>> >> On Fri, Mar 21 2025 at 20:18, Andreas Hindborg wrote:
>> >> >> Could you add me as a reviewer in these entries?
>> >> >>
>> >> >
>> >> > I would like to be added as well.
>> >>
>> >> Please add the relevant core code maintainers (Anna-Maria, Frederic,
>> >> John Stultz and myself) as well to the reviewers list, so that this does
>> >> not end up with changes going in opposite directions.
>> >>
>> >
>> > Make sense, I assume you want this to go via rust then (althought we
>> > would like it to go via your tree if possible ;-))?
>>
>
> Given Andreas is already preparing the pull request of the hrtimer
> abstraction to Miguel, and delay, timekeeping and hrtimer are related,
> these timekeeping/delay patches should go via Andreas (i.e.
> rust/hrtimer-next into rust/rust-next) if Thomas and Miguel are OK with
> it. Works for you, Andreas? If so...
>
>> Once the following review regarding fsleep() is complete, I will submit
>> patches #2 through #6 as v12 for rust-next:
>>
>> https://lore.kernel.org/linux-kernel/20250322.102449.895174336060649075.fujita.tomonori@gmail.com/
>>
>> The updated MAINTAINERS file will look like the following.
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index cbf84690c495..858e0b34422f 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -10370,6 +10370,18 @@ F:	kernel/time/timer_list.c
>>  F:	kernel/time/timer_migration.*
>>  F:	tools/testing/selftests/timers/
>>
>> +DELAY AND SLEEP API [RUST]
>> +M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
>> +R:	Boqun Feng <boqun.feng@gmail.com>
>> +R:	Andreas Hindborg <a.hindborg@kernel.org>
>
> ... this "R:" entry would be "M:",
>
>> +R:	Anna-Maria Behnsen <anna-maria@linutronix.de>
>> +R:	Frederic Weisbecker <frederic@kernel.org>
>> +R:	Thomas Gleixner <tglx@linutronix.de>
>> +L:	rust-for-linux@vger.kernel.org
>> +L:	linux-kernel@vger.kernel.org
>
> +T:	git https://github.com/Rust-for-Linux/linux.git hrtimer-next
>
>> +S:	Maintained
>
> I will let Andreas decide whether this is a "Supported" entry ;-)
>
>> +F:	rust/kernel/time/delay.rs
>> +
>>  HIGH-SPEED SCC DRIVER FOR AX.25
>>  L:	linux-hams@vger.kernel.org
>>  S:	Orphan
>> @@ -23944,6 +23956,17 @@ F:	kernel/time/timekeeping*
>>  F:	kernel/time/time_test.c
>>  F:	tools/testing/selftests/timers/
>>
>> +TIMEKEEPING API [RUST]
>
> and similar things for this entry as well.
>
>> +M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
>> +R:	Boqun Feng <boqun.feng@gmail.com>
>> +R:	Andreas Hindborg <a.hindborg@kernel.org>
>> +R:	John Stultz <jstultz@google.com>
>> +R:	Thomas Gleixner <tglx@linutronix.de>
>
> +R:      Stephen Boyd <sboyd@kernel.org>
>
> ?
>
>> +L:	rust-for-linux@vger.kernel.org
>> +L:	linux-kernel@vger.kernel.org
>> +S:	Maintained
>> +F:	rust/kernel/time.rs
>> +
>
> Tomo, let's wait for Andreas' rely and decide how to change these
> entries. Thanks!

My recommendation would be to take all of `rust/kernel/time` under one
entry for now. I suggest the following, folding in the hrtimer entry as
well:

DELAY, SLEEP, TIMEKEEPING, TIMERS [RUST]
M:	Andreas Hindborg <a.hindborg@kernel.org>
R:	Boqun Feng <boqun.feng@gmail.com>
R:	FUJITA Tomonori <fujita.tomonori@gmail.com>
R:	Lyude Paul <lyude@redhat.com>
R:	Frederic Weisbecker <frederic@kernel.org>
R:	Thomas Gleixner <tglx@linutronix.de>
R:	Anna-Maria Behnsen <anna-maria@linutronix.de>
R:	John Stultz <jstultz@google.com>
L:	rust-for-linux@vger.kernel.org
S:	Supported
W:	https://rust-for-linux.com
B:	https://github.com/Rust-for-Linux/linux/issues
T:	git https://github.com/Rust-for-Linux/linux.git rust-timekeeping-next
F:	rust/kernel/time.rs
F:	rust/kernel/time/

If that is acceptable to everyone, it is very likely that I can pick 2-6
for v6.16.

I assume patch 1 will go through the sched/core tree, and then Miguel
can pick 7.

Best regards,
Andreas Hindborg

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[Boqun Feng]

On Sat, Mar 22, 2025 at 11:40:21PM +0100, Andreas Hindborg wrote:
> Hi All,
> 
> "Boqun Feng" <boqun.feng@gmail.com> writes:
> 
> > On Sat, Mar 22, 2025 at 11:07:03AM +0900, FUJITA Tomonori wrote:
> >> Thank you all!
> >>
> >> On Fri, 21 Mar 2025 14:00:52 -0700
> >> Boqun Feng <boqun.feng@gmail.com> wrote:
> >>
> >> > On Fri, Mar 21, 2025 at 09:38:46PM +0100, Thomas Gleixner wrote:
> >> >> On Fri, Mar 21 2025 at 20:18, Andreas Hindborg wrote:
> >> >> >> Could you add me as a reviewer in these entries?
> >> >> >>
> >> >> >
> >> >> > I would like to be added as well.
> >> >>
> >> >> Please add the relevant core code maintainers (Anna-Maria, Frederic,
> >> >> John Stultz and myself) as well to the reviewers list, so that this does
> >> >> not end up with changes going in opposite directions.
> >> >>
> >> >
> >> > Make sense, I assume you want this to go via rust then (althought we
> >> > would like it to go via your tree if possible ;-))?
> >>
> >
> > Given Andreas is already preparing the pull request of the hrtimer
> > abstraction to Miguel, and delay, timekeeping and hrtimer are related,
> > these timekeeping/delay patches should go via Andreas (i.e.
> > rust/hrtimer-next into rust/rust-next) if Thomas and Miguel are OK with
> > it. Works for you, Andreas? If so...
> >
> >> Once the following review regarding fsleep() is complete, I will submit
> >> patches #2 through #6 as v12 for rust-next:
> >>
> >> https://lore.kernel.org/linux-kernel/20250322.102449.895174336060649075.fujita.tomonori@gmail.com/
> >>
> >> The updated MAINTAINERS file will look like the following.
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index cbf84690c495..858e0b34422f 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -10370,6 +10370,18 @@ F:	kernel/time/timer_list.c
> >>  F:	kernel/time/timer_migration.*
> >>  F:	tools/testing/selftests/timers/
> >>
> >> +DELAY AND SLEEP API [RUST]
> >> +M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
> >> +R:	Boqun Feng <boqun.feng@gmail.com>
> >> +R:	Andreas Hindborg <a.hindborg@kernel.org>
> >
> > ... this "R:" entry would be "M:",
> >
> >> +R:	Anna-Maria Behnsen <anna-maria@linutronix.de>
> >> +R:	Frederic Weisbecker <frederic@kernel.org>
> >> +R:	Thomas Gleixner <tglx@linutronix.de>
> >> +L:	rust-for-linux@vger.kernel.org
> >> +L:	linux-kernel@vger.kernel.org
> >
> > +T:	git https://github.com/Rust-for-Linux/linux.git hrtimer-next
> >
> >> +S:	Maintained
> >
> > I will let Andreas decide whether this is a "Supported" entry ;-)
> >
> >> +F:	rust/kernel/time/delay.rs
> >> +
> >>  HIGH-SPEED SCC DRIVER FOR AX.25
> >>  L:	linux-hams@vger.kernel.org
> >>  S:	Orphan
> >> @@ -23944,6 +23956,17 @@ F:	kernel/time/timekeeping*
> >>  F:	kernel/time/time_test.c
> >>  F:	tools/testing/selftests/timers/
> >>
> >> +TIMEKEEPING API [RUST]
> >
> > and similar things for this entry as well.
> >
> >> +M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
> >> +R:	Boqun Feng <boqun.feng@gmail.com>
> >> +R:	Andreas Hindborg <a.hindborg@kernel.org>
> >> +R:	John Stultz <jstultz@google.com>
> >> +R:	Thomas Gleixner <tglx@linutronix.de>
> >
> > +R:      Stephen Boyd <sboyd@kernel.org>
> >
> > ?
> >
> >> +L:	rust-for-linux@vger.kernel.org
> >> +L:	linux-kernel@vger.kernel.org
> >> +S:	Maintained
> >> +F:	rust/kernel/time.rs
> >> +
> >
> > Tomo, let's wait for Andreas' rely and decide how to change these
> > entries. Thanks!
> 
> My recommendation would be to take all of `rust/kernel/time` under one
> entry for now. I suggest the following, folding in the hrtimer entry as
> well:
> 
> DELAY, SLEEP, TIMEKEEPING, TIMERS [RUST]
> M:	Andreas Hindborg <a.hindborg@kernel.org>

Given you're the one who would handle the patches, I think this make
more sense.

> R:	Boqun Feng <boqun.feng@gmail.com>
> R:	FUJITA Tomonori <fujita.tomonori@gmail.com>

Tomo, does this look good to you?

> R:	Lyude Paul <lyude@redhat.com>
> R:	Frederic Weisbecker <frederic@kernel.org>
> R:	Thomas Gleixner <tglx@linutronix.de>
> R:	Anna-Maria Behnsen <anna-maria@linutronix.de>
> R:	John Stultz <jstultz@google.com>

We should add:

R:      Stephen Boyd <sboyd@kernel.org>

If Stephen is not against it.

> L:	rust-for-linux@vger.kernel.org
> S:	Supported
> W:	https://rust-for-linux.com
> B:	https://github.com/Rust-for-Linux/linux/issues
> T:	git https://github.com/Rust-for-Linux/linux.git rust-timekeeping-next
> F:	rust/kernel/time.rs
> F:	rust/kernel/time/
> 
> If that is acceptable to everyone, it is very likely that I can pick 2-6
> for v6.16.
> 

You will need to fix something because patch 2-6 removes `Ktime` ;-)

> I assume patch 1 will go through the sched/core tree, and then Miguel
> can pick 7.
> 

Patch 1 & 7 probably should go together, but we can decide it later.

Regards,
Boqun

> Best regards,
> Andreas Hindborg
> 
> 
> 

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[Andreas Hindborg]

"Boqun Feng" <boqun.feng@gmail.com> writes:

> On Sat, Mar 22, 2025 at 11:40:21PM +0100, Andreas Hindborg wrote:
>> Hi All,
>>
>> "Boqun Feng" <boqun.feng@gmail.com> writes:
>>
>> > On Sat, Mar 22, 2025 at 11:07:03AM +0900, FUJITA Tomonori wrote:
>> >> Thank you all!
>> >>
>> >> On Fri, 21 Mar 2025 14:00:52 -0700
>> >> Boqun Feng <boqun.feng@gmail.com> wrote:
>> >>
>> >> > On Fri, Mar 21, 2025 at 09:38:46PM +0100, Thomas Gleixner wrote:
>> >> >> On Fri, Mar 21 2025 at 20:18, Andreas Hindborg wrote:
>> >> >> >> Could you add me as a reviewer in these entries?
>> >> >> >>
>> >> >> >
>> >> >> > I would like to be added as well.
>> >> >>
>> >> >> Please add the relevant core code maintainers (Anna-Maria, Frederic,
>> >> >> John Stultz and myself) as well to the reviewers list, so that this does
>> >> >> not end up with changes going in opposite directions.
>> >> >>
>> >> >
>> >> > Make sense, I assume you want this to go via rust then (althought we
>> >> > would like it to go via your tree if possible ;-))?
>> >>
>> >
>> > Given Andreas is already preparing the pull request of the hrtimer
>> > abstraction to Miguel, and delay, timekeeping and hrtimer are related,
>> > these timekeeping/delay patches should go via Andreas (i.e.
>> > rust/hrtimer-next into rust/rust-next) if Thomas and Miguel are OK with
>> > it. Works for you, Andreas? If so...
>> >
>> >> Once the following review regarding fsleep() is complete, I will submit
>> >> patches #2 through #6 as v12 for rust-next:
>> >>
>> >> https://lore.kernel.org/linux-kernel/20250322.102449.895174336060649075.fujita.tomonori@gmail.com/
>> >>
>> >> The updated MAINTAINERS file will look like the following.
>> >>
>> >> diff --git a/MAINTAINERS b/MAINTAINERS
>> >> index cbf84690c495..858e0b34422f 100644
>> >> --- a/MAINTAINERS
>> >> +++ b/MAINTAINERS
>> >> @@ -10370,6 +10370,18 @@ F:	kernel/time/timer_list.c
>> >>  F:	kernel/time/timer_migration.*
>> >>  F:	tools/testing/selftests/timers/
>> >>
>> >> +DELAY AND SLEEP API [RUST]
>> >> +M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
>> >> +R:	Boqun Feng <boqun.feng@gmail.com>
>> >> +R:	Andreas Hindborg <a.hindborg@kernel.org>
>> >
>> > ... this "R:" entry would be "M:",
>> >
>> >> +R:	Anna-Maria Behnsen <anna-maria@linutronix.de>
>> >> +R:	Frederic Weisbecker <frederic@kernel.org>
>> >> +R:	Thomas Gleixner <tglx@linutronix.de>
>> >> +L:	rust-for-linux@vger.kernel.org
>> >> +L:	linux-kernel@vger.kernel.org
>> >
>> > +T:	git https://github.com/Rust-for-Linux/linux.git hrtimer-next
>> >
>> >> +S:	Maintained
>> >
>> > I will let Andreas decide whether this is a "Supported" entry ;-)
>> >
>> >> +F:	rust/kernel/time/delay.rs
>> >> +
>> >>  HIGH-SPEED SCC DRIVER FOR AX.25
>> >>  L:	linux-hams@vger.kernel.org
>> >>  S:	Orphan
>> >> @@ -23944,6 +23956,17 @@ F:	kernel/time/timekeeping*
>> >>  F:	kernel/time/time_test.c
>> >>  F:	tools/testing/selftests/timers/
>> >>
>> >> +TIMEKEEPING API [RUST]
>> >
>> > and similar things for this entry as well.
>> >
>> >> +M:	FUJITA Tomonori <fujita.tomonori@gmail.com>
>> >> +R:	Boqun Feng <boqun.feng@gmail.com>
>> >> +R:	Andreas Hindborg <a.hindborg@kernel.org>
>> >> +R:	John Stultz <jstultz@google.com>
>> >> +R:	Thomas Gleixner <tglx@linutronix.de>
>> >
>> > +R:      Stephen Boyd <sboyd@kernel.org>
>> >
>> > ?
>> >
>> >> +L:	rust-for-linux@vger.kernel.org
>> >> +L:	linux-kernel@vger.kernel.org
>> >> +S:	Maintained
>> >> +F:	rust/kernel/time.rs
>> >> +
>> >
>> > Tomo, let's wait for Andreas' rely and decide how to change these
>> > entries. Thanks!
>>
>> My recommendation would be to take all of `rust/kernel/time` under one
>> entry for now. I suggest the following, folding in the hrtimer entry as
>> well:
>>
>> DELAY, SLEEP, TIMEKEEPING, TIMERS [RUST]
>> M:	Andreas Hindborg <a.hindborg@kernel.org>
>
> Given you're the one who would handle the patches, I think this make
> more sense.
>
>> R:	Boqun Feng <boqun.feng@gmail.com>
>> R:	FUJITA Tomonori <fujita.tomonori@gmail.com>
>
> Tomo, does this look good to you?
>
>> R:	Lyude Paul <lyude@redhat.com>
>> R:	Frederic Weisbecker <frederic@kernel.org>
>> R:	Thomas Gleixner <tglx@linutronix.de>
>> R:	Anna-Maria Behnsen <anna-maria@linutronix.de>
>> R:	John Stultz <jstultz@google.com>
>
> We should add:
>
> R:      Stephen Boyd <sboyd@kernel.org>
>
> If Stephen is not against it.

Yes 👍

>
>> L:	rust-for-linux@vger.kernel.org
>> S:	Supported
>> W:	https://rust-for-linux.com
>> B:	https://github.com/Rust-for-Linux/linux/issues
>> T:	git https://github.com/Rust-for-Linux/linux.git rust-timekeeping-next
>> F:	rust/kernel/time.rs
>> F:	rust/kernel/time/
>>
>> If that is acceptable to everyone, it is very likely that I can pick 2-6
>> for v6.16.
>>
>
> You will need to fix something because patch 2-6 removes `Ktime` ;-)

Yea, but `Instant` is almost a direct substitution, right? Anyway, Tomo
can send a new spin and change all the uses of Ktime, or I can do it. It
should be straight forward. Either way is fine with me.


Best regards,
Andreas Hindborg

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[FUJITA Tomonori]

On Mon, 31 Mar 2025 21:43:50 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

>>> If that is acceptable to everyone, it is very likely that I can pick 2-6
>>> for v6.16.
>>>
>>
>> You will need to fix something because patch 2-6 removes `Ktime` ;-)
> 
> Yea, but `Instant` is almost a direct substitution, right? Anyway, Tomo
> can send a new spin and change all the uses of Ktime, or I can do it. It
> should be straight forward. Either way is fine with me.

`Delta`? Not `Instant`.

All Ktime in hrtimer are passed to hrtimer_start_range_ns(), right?

I'll send a new version shortly.

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[Andreas Hindborg]

"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:

> On Mon, 31 Mar 2025 21:43:50 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>>>> If that is acceptable to everyone, it is very likely that I can pick 2-6
>>>> for v6.16.
>>>>
>>>
>>> You will need to fix something because patch 2-6 removes `Ktime` ;-)
>>
>> Yea, but `Instant` is almost a direct substitution, right? Anyway, Tomo
>> can send a new spin and change all the uses of Ktime, or I can do it. It
>> should be straight forward. Either way is fine with me.
>
> `Delta`? Not `Instant`.

It depends. Current hrtimer takes `Ktime` and supports
`HrTimerMode::Absolute` and `HrTimerMode::Relative`. With `Delta` and
`Instant` we should take `Instant` for `HrTimerMode::Absolute` and
`Delta` for `HrTimerMode::Relative`. The API needs to be modified a bit
to make that work though. Probably we need to make the start function
generic over the expiration type or something.

If you want to, you can fix that. If not, you can use `Instant` for the
relative case as well, and we shall interpret it as duration. Then I
will fix it up later. Your decision.

> All Ktime in hrtimer are passed to hrtimer_start_range_ns(), right?

Yes, that is where they end up.

> I'll send a new version shortly.

Great :)


Best regards,
Andreas Hindborg

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[FUJITA Tomonori]

On Thu, 03 Apr 2025 12:54:40 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

>>>> You will need to fix something because patch 2-6 removes `Ktime` ;-)
>>>
>>> Yea, but `Instant` is almost a direct substitution, right? Anyway, Tomo
>>> can send a new spin and change all the uses of Ktime, or I can do it. It
>>> should be straight forward. Either way is fine with me.
>>
>> `Delta`? Not `Instant`.
> 
> It depends. Current hrtimer takes `Ktime` and supports
> `HrTimerMode::Absolute` and `HrTimerMode::Relative`. With `Delta` and
> `Instant` we should take `Instant` for `HrTimerMode::Absolute` and
> `Delta` for `HrTimerMode::Relative`. The API needs to be modified a bit
> to make that work though. Probably we need to make the start function
> generic over the expiration type or something.
> 
> If you want to, you can fix that. If not, you can use `Instant` for the
> relative case as well, and we shall interpret it as duration. Then I
> will fix it up later. Your decision.
> 
>> All Ktime in hrtimer are passed to hrtimer_start_range_ns(), right?
> 
> Yes, that is where they end up.

Ah, I found that __hrtimer_start_range_ns() handles ktime_t
differently in HRTIMER_MODE_REL mode.

As you said, looks like the API needs to be updated and I think that
it's best to leave that to you. I'll just use `Instant` for all cases.

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[Boqun Feng]

On Thu, Apr 03, 2025 at 09:57:45PM +0900, FUJITA Tomonori wrote:
> On Thu, 03 Apr 2025 12:54:40 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
> 
> >>>> You will need to fix something because patch 2-6 removes `Ktime` ;-)
> >>>
> >>> Yea, but `Instant` is almost a direct substitution, right? Anyway, Tomo
> >>> can send a new spin and change all the uses of Ktime, or I can do it. It
> >>> should be straight forward. Either way is fine with me.
> >>
> >> `Delta`? Not `Instant`.
> > 
> > It depends. Current hrtimer takes `Ktime` and supports
> > `HrTimerMode::Absolute` and `HrTimerMode::Relative`. With `Delta` and
> > `Instant` we should take `Instant` for `HrTimerMode::Absolute` and
> > `Delta` for `HrTimerMode::Relative`. The API needs to be modified a bit
> > to make that work though. Probably we need to make the start function
> > generic over the expiration type or something.
> > 

If we make `HrTimerMode` a trait:

    pub trait HrTimerMode {
        type Expires; /* either Delta or Instant */
    }

and `HrTimerPointer` generic over `HrTimerMode`:

    pub trait HrTimerPointer<Mode: HrTimerMode> {
        fn start(self, expires: Mode::Expires) -> HrTimerHandler<Mode>
    }

then we can disallow that a Relative timer accidentally uses an Instant
or an Absolute timer accidentally uses an Delta. Of course a few other
places need to be generic, but the end result looks pretty good to me.

Regards,
Boqun

> > If you want to, you can fix that. If not, you can use `Instant` for the
> > relative case as well, and we shall interpret it as duration. Then I
> > will fix it up later. Your decision.
> > 
> >> All Ktime in hrtimer are passed to hrtimer_start_range_ns(), right?
> > 
> > Yes, that is where they end up.
> 
> Ah, I found that __hrtimer_start_range_ns() handles ktime_t
> differently in HRTIMER_MODE_REL mode.
> 
> As you said, looks like the API needs to be updated and I think that
> it's best to leave that to you. I'll just use `Instant` for all cases.
> 
> 

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[FUJITA Tomonori]

On Mon, 31 Mar 2025 07:03:15 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

>> My recommendation would be to take all of `rust/kernel/time` under one
>> entry for now. I suggest the following, folding in the hrtimer entry as
>> well:
>> 
>> DELAY, SLEEP, TIMEKEEPING, TIMERS [RUST]
>> M:	Andreas Hindborg <a.hindborg@kernel.org>
> 
> Given you're the one who would handle the patches, I think this make
> more sense.
> 
>> R:	Boqun Feng <boqun.feng@gmail.com>
>> R:	FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Tomo, does this look good to you?

Fine by me.

So a single entry for all the Rust time stuff, which isn't aligned
with C's MAINTAINERS entries. It's just for now?


>> R:	Lyude Paul <lyude@redhat.com>
>> R:	Frederic Weisbecker <frederic@kernel.org>
>> R:	Thomas Gleixner <tglx@linutronix.de>
>> R:	Anna-Maria Behnsen <anna-maria@linutronix.de>
>> R:	John Stultz <jstultz@google.com>
> 
> We should add:
> 
> R:      Stephen Boyd <sboyd@kernel.org>
> 
> If Stephen is not against it.
> 
>> L:	rust-for-linux@vger.kernel.org
>> S:	Supported
>> W:	https://rust-for-linux.com
>> B:	https://github.com/Rust-for-Linux/linux/issues
>> T:	git https://github.com/Rust-for-Linux/linux.git rust-timekeeping-next
>> F:	rust/kernel/time.rs
>> F:	rust/kernel/time/
>> 
>> If that is acceptable to everyone, it is very likely that I can pick 2-6
>> for v6.16.
>> 
> 
> You will need to fix something because patch 2-6 removes `Ktime` ;-)

I'll take care of it in the next version.

>> I assume patch 1 will go through the sched/core tree, and then Miguel
>> can pick 7.
>> 
> 
> Patch 1 & 7 probably should go together, but we can decide it later.

Since nothing has moved forward for quite a while, maybe it's time to
drop patch 1.

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[Boqun Feng]

On Wed, Apr 02, 2025 at 11:16:27PM +0900, FUJITA Tomonori wrote:
> On Mon, 31 Mar 2025 07:03:15 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> >> My recommendation would be to take all of `rust/kernel/time` under one
> >> entry for now. I suggest the following, folding in the hrtimer entry as
> >> well:
> >> 
> >> DELAY, SLEEP, TIMEKEEPING, TIMERS [RUST]
> >> M:	Andreas Hindborg <a.hindborg@kernel.org>
> > 
> > Given you're the one who would handle the patches, I think this make
> > more sense.
> > 
> >> R:	Boqun Feng <boqun.feng@gmail.com>
> >> R:	FUJITA Tomonori <fujita.tomonori@gmail.com>
> > 
> > Tomo, does this look good to you?
> 
> Fine by me.
> 
> So a single entry for all the Rust time stuff, which isn't aligned
> with C's MAINTAINERS entries. It's just for now?
> 

Given Andreas is the one who's going to handle the PRs, and he will put
all the things in one branch. I think it's fine even for long term, and
we got all relevant reviewers covered. If the Rust timekeeping + hrtimer
community expands in the future, we can also add more entries. We don't
necessarily need to copy all maintainer structures from C ;-)

> 
> >> R:	Lyude Paul <lyude@redhat.com>
> >> R:	Frederic Weisbecker <frederic@kernel.org>
> >> R:	Thomas Gleixner <tglx@linutronix.de>
> >> R:	Anna-Maria Behnsen <anna-maria@linutronix.de>
> >> R:	John Stultz <jstultz@google.com>
> > 
> > We should add:
> > 
> > R:      Stephen Boyd <sboyd@kernel.org>
> > 
> > If Stephen is not against it.
> > 
> >> L:	rust-for-linux@vger.kernel.org
> >> S:	Supported
> >> W:	https://rust-for-linux.com
> >> B:	https://github.com/Rust-for-Linux/linux/issues
> >> T:	git https://github.com/Rust-for-Linux/linux.git rust-timekeeping-next
> >> F:	rust/kernel/time.rs
> >> F:	rust/kernel/time/
> >> 
> >> If that is acceptable to everyone, it is very likely that I can pick 2-6
> >> for v6.16.
> >> 
> > 
> > You will need to fix something because patch 2-6 removes `Ktime` ;-)
> 
> I'll take care of it in the next version.
> 

Thanks!

> >> I assume patch 1 will go through the sched/core tree, and then Miguel
> >> can pick 7.
> >> 
> > 
> > Patch 1 & 7 probably should go together, but we can decide it later.
> 
> Since nothing has moved forward for quite a while, maybe it's time to
> drop patch 1.

No, I think we should keep it. Because otherwise we will use a macro
version of read_poll_timeout(), which is strictly worse. I'm happy to
collect patch #1 and the cpu_relax() patch of patch #7, and send an PR
to tip. Could you split them a bit:

* Move the Rust might_sleep() in patch #7 to patch #1 and put it at
  kernel::task, also if we EXPORT_SYMBOL(__might_sleep_precision), we
  don't need the rust_helper for it.

* Have a separate containing the cpu_relax() bit.

* Also you may want to put #[inline] at cpu_relax() and might_resched().

and we can start from there. Sounds good?

Regards,
Boqun

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[FUJITA Tomonori]

On Wed, 2 Apr 2025 09:29:18 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Wed, Apr 02, 2025 at 11:16:27PM +0900, FUJITA Tomonori wrote:
>> On Mon, 31 Mar 2025 07:03:15 -0700
>> Boqun Feng <boqun.feng@gmail.com> wrote:
>> 
>> >> My recommendation would be to take all of `rust/kernel/time` under one
>> >> entry for now. I suggest the following, folding in the hrtimer entry as
>> >> well:
>> >> 
>> >> DELAY, SLEEP, TIMEKEEPING, TIMERS [RUST]
>> >> M:	Andreas Hindborg <a.hindborg@kernel.org>
>> > 
>> > Given you're the one who would handle the patches, I think this make
>> > more sense.
>> > 
>> >> R:	Boqun Feng <boqun.feng@gmail.com>
>> >> R:	FUJITA Tomonori <fujita.tomonori@gmail.com>
>> > 
>> > Tomo, does this look good to you?
>> 
>> Fine by me.
>> 
>> So a single entry for all the Rust time stuff, which isn't aligned
>> with C's MAINTAINERS entries. It's just for now?
>> 
> 
> Given Andreas is the one who's going to handle the PRs, and he will put
> all the things in one branch. I think it's fine even for long term, and
> we got all relevant reviewers covered. If the Rust timekeeping + hrtimer
> community expands in the future, we can also add more entries. We don't
> necessarily need to copy all maintainer structures from C ;-)

It seems I was mistaken. I had thought that the ideal goal was for the
same team to maintain both the C code and the corresponding Rust code.


>> >> I assume patch 1 will go through the sched/core tree, and then Miguel
>> >> can pick 7.
>> >> 
>> > 
>> > Patch 1 & 7 probably should go together, but we can decide it later.
>> 
>> Since nothing has moved forward for quite a while, maybe it's time to
>> drop patch 1.
> 
> No, I think we should keep it. Because otherwise we will use a macro

Yeah, I know. the first version of this uses a macro.


> version of read_poll_timeout(), which is strictly worse. I'm happy to
> collect patch #1 and the cpu_relax() patch of patch #7, and send an PR
> to tip. Could you split them a bit:
> 
> * Move the Rust might_sleep() in patch #7 to patch #1 and put it at
>   kernel::task, also if we EXPORT_SYMBOL(__might_sleep_precision), we
>   don't need the rust_helper for it.
> 
> * Have a separate containing the cpu_relax() bit.
> 
> * Also you may want to put #[inline] at cpu_relax() and might_resched().
> 
> and we can start from there. Sounds good?

I can do whatever but I don't think these matters. The problem is that
we haven't received a response from the scheduler maintainers for a
long time. We don't even know if the implementation is actually an
issue.

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[Boqun Feng]

On Thu, Apr 03, 2025 at 08:03:34AM +0900, FUJITA Tomonori wrote:
> On Wed, 2 Apr 2025 09:29:18 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Wed, Apr 02, 2025 at 11:16:27PM +0900, FUJITA Tomonori wrote:
> >> On Mon, 31 Mar 2025 07:03:15 -0700
> >> Boqun Feng <boqun.feng@gmail.com> wrote:
> >> 
> >> >> My recommendation would be to take all of `rust/kernel/time` under one
> >> >> entry for now. I suggest the following, folding in the hrtimer entry as
> >> >> well:
> >> >> 
> >> >> DELAY, SLEEP, TIMEKEEPING, TIMERS [RUST]
> >> >> M:	Andreas Hindborg <a.hindborg@kernel.org>
> >> > 
> >> > Given you're the one who would handle the patches, I think this make
> >> > more sense.
> >> > 
> >> >> R:	Boqun Feng <boqun.feng@gmail.com>
> >> >> R:	FUJITA Tomonori <fujita.tomonori@gmail.com>
> >> > 
> >> > Tomo, does this look good to you?
> >> 
> >> Fine by me.
> >> 
> >> So a single entry for all the Rust time stuff, which isn't aligned
> >> with C's MAINTAINERS entries. It's just for now?
> >> 
> > 
> > Given Andreas is the one who's going to handle the PRs, and he will put
> > all the things in one branch. I think it's fine even for long term, and
> > we got all relevant reviewers covered. If the Rust timekeeping + hrtimer
> > community expands in the future, we can also add more entries. We don't
> > necessarily need to copy all maintainer structures from C ;-)
> 
> It seems I was mistaken. I had thought that the ideal goal was for the
> same team to maintain both the C code and the corresponding Rust code.
> 

Yeah, that was the ideal goal, but Frederic said in the hrtimer series:

	https://lore.kernel.org/rust-for-linux/Z8iLIyofy6KGOsq5@localhost.localdomain/

, to me it's clear that hrtimer maintainers want hrtimer Rust patches to
go to rust tree via Andreas, and given timekeeping maintainers are
basically the same group of people, and Thomas explicitly asked to be
added as reviewers:

	https://lore.kernel.org/rust-for-linux/87o6xu15m1.ffs@tglx/

It's a clear signal that timekeeping and hrtimer Rust patches are
preferred to go to rust tree, and Andreas had nicely accepted to handle
timekeeping and sleep/delay patches, so it makes sense to use one entry
if he prefers. Hope this clarifies things.

> 
> >> >> I assume patch 1 will go through the sched/core tree, and then Miguel
> >> >> can pick 7.
> >> >> 
> >> > 
> >> > Patch 1 & 7 probably should go together, but we can decide it later.
> >> 
> >> Since nothing has moved forward for quite a while, maybe it's time to
> >> drop patch 1.
> > 
> > No, I think we should keep it. Because otherwise we will use a macro
> 
> Yeah, I know. the first version of this uses a macro.
> 
> 
> > version of read_poll_timeout(), which is strictly worse. I'm happy to
> > collect patch #1 and the cpu_relax() patch of patch #7, and send an PR
> > to tip. Could you split them a bit:
> > 
> > * Move the Rust might_sleep() in patch #7 to patch #1 and put it at
> >   kernel::task, also if we EXPORT_SYMBOL(__might_sleep_precision), we
> >   don't need the rust_helper for it.
> > 
> > * Have a separate containing the cpu_relax() bit.
> > 
> > * Also you may want to put #[inline] at cpu_relax() and might_resched().
> > 
> > and we can start from there. Sounds good?
> 
> I can do whatever but I don't think these matters. The problem is that

Confused. I said I would do a PR, that means if no objection, the
patches will get merged. Isn't this a way to move forward? Or you're
against that I'm doing a PR?

> we haven't received a response from the scheduler maintainers for a
> long time. We don't even know if the implementation is actually an
> issue.
> 

If there's an issue, I can fix it. After all, printk confirmed that
".*s" format works for this case:

	https://lore.kernel.org/rust-for-linux/ZyyAsjsz05AlkOBd@pathway.suse.cz/

and Peter sort of confirmed he's not against the idea:

	https://lore.kernel.org/rust-for-linux/20250201121613.GC8256@noisy.programming.kicks-ass.net/

I don't see any major issue blocking this. But of course, I'm happy to
resolve if there is one.


Regards,
Boqun

> 
> 

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[FUJITA Tomonori]

On Wed, 2 Apr 2025 17:51:41 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Thu, Apr 03, 2025 at 08:03:34AM +0900, FUJITA Tomonori wrote:
>> On Wed, 2 Apr 2025 09:29:18 -0700
>> Boqun Feng <boqun.feng@gmail.com> wrote:
>> 
>> > On Wed, Apr 02, 2025 at 11:16:27PM +0900, FUJITA Tomonori wrote:
>> >> On Mon, 31 Mar 2025 07:03:15 -0700
>> >> Boqun Feng <boqun.feng@gmail.com> wrote:
>> >> 
>> >> >> My recommendation would be to take all of `rust/kernel/time` under one
>> >> >> entry for now. I suggest the following, folding in the hrtimer entry as
>> >> >> well:
>> >> >> 
>> >> >> DELAY, SLEEP, TIMEKEEPING, TIMERS [RUST]
>> >> >> M:	Andreas Hindborg <a.hindborg@kernel.org>
>> >> > 
>> >> > Given you're the one who would handle the patches, I think this make
>> >> > more sense.
>> >> > 
>> >> >> R:	Boqun Feng <boqun.feng@gmail.com>
>> >> >> R:	FUJITA Tomonori <fujita.tomonori@gmail.com>
>> >> > 
>> >> > Tomo, does this look good to you?
>> >> 
>> >> Fine by me.
>> >> 
>> >> So a single entry for all the Rust time stuff, which isn't aligned
>> >> with C's MAINTAINERS entries. It's just for now?
>> >> 
>> > 
>> > Given Andreas is the one who's going to handle the PRs, and he will put
>> > all the things in one branch. I think it's fine even for long term, and
>> > we got all relevant reviewers covered. If the Rust timekeeping + hrtimer
>> > community expands in the future, we can also add more entries. We don't
>> > necessarily need to copy all maintainer structures from C ;-)
>> 
>> It seems I was mistaken. I had thought that the ideal goal was for the
>> same team to maintain both the C code and the corresponding Rust code.
>> 
> 
> Yeah, that was the ideal goal, but Frederic said in the hrtimer series:
> 
> 	https://lore.kernel.org/rust-for-linux/Z8iLIyofy6KGOsq5@localhost.localdomain/
> 
> , to me it's clear that hrtimer maintainers want hrtimer Rust patches to
> go to rust tree via Andreas, and given timekeeping maintainers are
> basically the same group of people, and Thomas explicitly asked to be
> added as reviewers:
> 
> 	https://lore.kernel.org/rust-for-linux/87o6xu15m1.ffs@tglx/
> 
> It's a clear signal that timekeeping and hrtimer Rust patches are
> preferred to go to rust tree, and Andreas had nicely accepted to handle
> timekeeping and sleep/delay patches, so it makes sense to use one entry
> if he prefers. Hope this clarifies things.

Yeah, I see your point.

>> >> >> I assume patch 1 will go through the sched/core tree, and then Miguel
>> >> >> can pick 7.
>> >> >> 
>> >> > 
>> >> > Patch 1 & 7 probably should go together, but we can decide it later.
>> >> 
>> >> Since nothing has moved forward for quite a while, maybe it's time to
>> >> drop patch 1.
>> > 
>> > No, I think we should keep it. Because otherwise we will use a macro
>> 
>> Yeah, I know. the first version of this uses a macro.
>> 
>> 
>> > version of read_poll_timeout(), which is strictly worse. I'm happy to
>> > collect patch #1 and the cpu_relax() patch of patch #7, and send an PR
>> > to tip. Could you split them a bit:
>> > 
>> > * Move the Rust might_sleep() in patch #7 to patch #1 and put it at
>> >   kernel::task, also if we EXPORT_SYMBOL(__might_sleep_precision), we
>> >   don't need the rust_helper for it.
>> > 
>> > * Have a separate containing the cpu_relax() bit.
>> > 
>> > * Also you may want to put #[inline] at cpu_relax() and might_resched().
>> > 
>> > and we can start from there. Sounds good?
>> 
>> I can do whatever but I don't think these matters. The problem is that
> 
> Confused. I said I would do a PR, that means if no objection, the
> patches will get merged. Isn't this a way to move forward? Or you're
> against that I'm doing a PR?

I don't object to you doing a PR.

I meant that it's unclear whether we can move forward with the
approach, as we haven't received much response from the maintainers
and we don't know what the blockers are.

>> we haven't received a response from the scheduler maintainers for a
>> long time. We don't even know if the implementation is actually an
>> issue.
>> 
> 
> If there's an issue, I can fix it. After all, printk confirmed that
> ".*s" format works for this case:
> 
> 	https://lore.kernel.org/rust-for-linux/ZyyAsjsz05AlkOBd@pathway.suse.cz/
> 
> and Peter sort of confirmed he's not against the idea:
> 
> 	https://lore.kernel.org/rust-for-linux/20250201121613.GC8256@noisy.programming.kicks-ass.net/
> 
> I don't see any major issue blocking this. But of course, I'm happy to
> resolve if there is one.

I know but this patch adds a workaround to the C code for Rust’s sake,
I would understand if the maintainers reject it and prefer having Rust
work around the issue instead.

Anyway, let's try one more time. I'll modify the patch #1 as you
suggested and send a new patchset.


Thanks!

Re: [PATCH v11 6/8] MAINTAINERS: rust: Add new sections for DELAY/SLEEP and TIMEKEEPING API

[Boqun Feng]

On Thu, Apr 03, 2025 at 12:02:00PM +0900, FUJITA Tomonori wrote:
[...]
> >> > version of read_poll_timeout(), which is strictly worse. I'm happy to
> >> > collect patch #1 and the cpu_relax() patch of patch #7, and send an PR
> >> > to tip. Could you split them a bit:
> >> > 
> >> > * Move the Rust might_sleep() in patch #7 to patch #1 and put it at
> >> >   kernel::task, also if we EXPORT_SYMBOL(__might_sleep_precision), we
> >> >   don't need the rust_helper for it.
> >> > 
> >> > * Have a separate containing the cpu_relax() bit.
> >> > 
> >> > * Also you may want to put #[inline] at cpu_relax() and might_resched().
> >> > 
> >> > and we can start from there. Sounds good?
> >> 
> >> I can do whatever but I don't think these matters. The problem is that
> > 
> > Confused. I said I would do a PR, that means if no objection, the
> > patches will get merged. Isn't this a way to move forward? Or you're
> > against that I'm doing a PR?
> 
> I don't object to you doing a PR.
> 
> I meant that it's unclear whether we can move forward with the
> approach, as we haven't received much response from the maintainers
> and we don't know what the blockers are.
> 
> >> we haven't received a response from the scheduler maintainers for a
> >> long time. We don't even know if the implementation is actually an
> >> issue.
> >> 
> > 
> > If there's an issue, I can fix it. After all, printk confirmed that
> > ".*s" format works for this case:
> > 
> > 	https://lore.kernel.org/rust-for-linux/ZyyAsjsz05AlkOBd@pathway.suse.cz/
> > 
> > and Peter sort of confirmed he's not against the idea:
> > 
> > 	https://lore.kernel.org/rust-for-linux/20250201121613.GC8256@noisy.programming.kicks-ass.net/
> > 
> > I don't see any major issue blocking this. But of course, I'm happy to
> > resolve if there is one.
> 
> I know but this patch adds a workaround to the C code for Rust´s sake,

The workaround at C is a helper function and the original C API
still works as it should be.

> I would understand if the maintainers reject it and prefer having Rust
> work around the issue instead.
> 

The workaround at Rust means we could have a function interface but we
use a macro interface.

I say we pick the C workaround, because read_poll_timeout() is a public
API, it's worth the effect making it better.

> Anyway, let's try one more time. I'll modify the patch #1 as you
> suggested and send a new patchset.
> 

Thanks!

Regards,
Boqun

> 
> Thanks!

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

[FUJITA Tomonori]

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

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

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

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

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

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

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 9565485a1a54..16d256897ccb 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -14,6 +14,7 @@
 #include "cred.c"
 #include "device.c"
 #include "err.c"
+#include "kernel.c"
 #include "fs.c"
 #include "io.c"
 #include "jump_label.c"
diff --git a/rust/helpers/kernel.c b/rust/helpers/kernel.c
new file mode 100644
index 000000000000..f04c04d4cc4f
--- /dev/null
+++ b/rust/helpers/kernel.c
@@ -0,0 +1,18 @@
+// 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);
+}
+
+void rust_helper_might_resched(void)
+{
+	might_resched();
+}
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
index d4a73e52e3ee..be63742f517b 100644
--- a/rust/kernel/io.rs
+++ b/rust/kernel/io.rs
@@ -7,6 +7,8 @@
 use crate::error::{code::EINVAL, Result};
 use crate::{bindings, build_assert};
 
+pub mod poll;
+
 /// Raw representation of an MMIO region.
 ///
 /// By itself, the existence of an instance of this structure does not provide any guarantees that
diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
new file mode 100644
index 000000000000..5977b2082cc6
--- /dev/null
+++ b/rust/kernel/io/poll.rs
@@ -0,0 +1,120 @@
+// 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},
+};
+
+/// Polls periodically until a condition is met or a timeout is reached.
+///
+/// The function repeatedly executes the given operation `op` closure and
+/// checks its result using the condition closure `cond`.
+/// If `cond` returns `true`, the function returns successfully with the result of `op`.
+/// Otherwise, it waits for a duration specified by `sleep_delta`
+/// before executing `op` again.
+/// This process continues until either `cond` returns `true` or the timeout,
+/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
+/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
+///
+/// # Examples
+///
+/// ```rust,ignore
+/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
+///     // The `op` closure reads the value of a specific status register.
+///     let op = || -> Result<u16> { dev.read_ready_register() };
+///
+///     // The `cond` closure takes a reference to the value returned by `op`
+///     // and checks whether the hardware is ready.
+///     let cond = |val: &u16| *val == HW_READY;
+///
+///     match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
+///         Ok(_) => {
+///             // The hardware is ready. The returned value of the `op`` closure isn't used.
+///             Ok(())
+///         }
+///         Err(e) => Err(e),
+///     }
+/// }
+/// ```
+///
+/// ```rust
+/// use kernel::io::poll::read_poll_timeout;
+/// use kernel::time::Delta;
+/// use kernel::sync::{SpinLock, new_spinlock};
+///
+/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
+/// let g = lock.lock();
+/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
+/// drop(g);
+///
+/// # Ok::<(), Error>(())
+/// ```
+#[track_caller]
+pub fn read_poll_timeout<Op, Cond, T>(
+    mut op: Op,
+    mut cond: Cond,
+    sleep_delta: Delta,
+    timeout_delta: Option<Delta>,
+) -> Result<T>
+where
+    Op: FnMut() -> Result<T>,
+    Cond: FnMut(&T) -> bool,
+{
+    let start = Instant::now();
+    let sleep = !sleep_delta.is_zero();
+
+    if sleep {
+        might_sleep();
+    }
+
+    loop {
+        let val = op()?;
+        if cond(&val) {
+            // Unlike the C version, we immediately return.
+            // We know the condition is met so we don't need to check again.
+            return Ok(val);
+        }
+        if let Some(timeout_delta) = timeout_delta {
+            if start.elapsed() > timeout_delta {
+                // Unlike the C version, we immediately return.
+                // We have just called `op()` so we don't need to call it again.
+                return Err(ETIMEDOUT);
+            }
+        }
+        if sleep {
+            fsleep(sleep_delta);
+        }
+        // fsleep() could be busy-wait loop so we always call cpu_relax().
+        cpu_relax();
+    }
+}
+
+/// Annotation for functions that can sleep.
+///
+/// Equivalent to the C side [`might_sleep()`], this function serves as
+/// a debugging aid and a potential scheduling point.
+///
+/// This function can only be used in a nonatomic context.
+#[track_caller]
+fn might_sleep() {
+    #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
+    {
+        let loc = core::panic::Location::caller();
+        // SAFETY: FFI call.
+        unsafe {
+            crate::bindings::__might_sleep_precision(
+                loc.file().as_ptr().cast(),
+                loc.file().len() as i32,
+                loc.line() as i32,
+            )
+        }
+    }
+
+    // SAFETY: FFI call.
+    unsafe { crate::bindings::might_resched() }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0911..415c500212dd 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -40,6 +40,7 @@
 pub mod block;
 #[doc(hidden)]
 pub mod build_assert;
+pub mod cpu;
 pub mod cred;
 pub mod device;
 pub mod device_id;
-- 
2.43.0

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

[Fiona Behrens]

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:

> Add read_poll_timeout functions which poll periodically until a
> condition is met or a timeout is reached.
>
> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
> macro and a simple wrapper for Rust doesn't work. So this implements
> the same functionality in Rust.
>
> The C version uses usleep_range() while the Rust version uses
> fsleep(), which uses the best sleep method so it works with spans that
> usleep_range() doesn't work nicely with.
>
> The sleep_before_read argument isn't supported since there is no user
> for now. It's rarely used in the C version.
>
> read_poll_timeout() can only be used in a nonatomic context. This
> requirement is not checked by these abstractions, but it is intended
> that klint [1] or a similar tool will be used to check it in the
> future.
>
> Link: https://rust-for-linux.com/klint [1]
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

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

> ---
>  rust/helpers/helpers.c |   1 +
>  rust/helpers/kernel.c  |  18 +++++++
>  rust/kernel/cpu.rs     |  13 +++++
>  rust/kernel/error.rs   |   1 +
>  rust/kernel/io.rs      |   2 +
>  rust/kernel/io/poll.rs | 120 +++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs     |   1 +
>  7 files changed, 156 insertions(+)
>  create mode 100644 rust/helpers/kernel.c
>  create mode 100644 rust/kernel/cpu.rs
>  create mode 100644 rust/kernel/io/poll.rs
>
> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
> new file mode 100644
> index 000000000000..5977b2082cc6
> --- /dev/null
> +++ b/rust/kernel/io/poll.rs
> @@ -0,0 +1,120 @@
> +// 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},
> +};
> +
> +/// Polls periodically until a condition is met or a timeout is reached.
> +///
> +/// The function repeatedly executes the given operation `op` closure and
> +/// checks its result using the condition closure `cond`.
> +/// If `cond` returns `true`, the function returns successfully with the result of `op`.
> +/// Otherwise, it waits for a duration specified by `sleep_delta`
> +/// before executing `op` again.
> +/// This process continues until either `cond` returns `true` or the timeout,
> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
> +///
> +/// # Examples
> +///
> +/// ```rust,ignore
> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
> +///     // The `op` closure reads the value of a specific status register.
> +///     let op = || -> Result<u16> { dev.read_ready_register() };
> +///
> +///     // The `cond` closure takes a reference to the value returned by `op`
> +///     // and checks whether the hardware is ready.
> +///     let cond = |val: &u16| *val == HW_READY;
> +///
> +///     match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
> +///         Ok(_) => {
> +///             // The hardware is ready. The returned value of the `op`` closure isn't used.
> +///             Ok(())
> +///         }
> +///         Err(e) => Err(e),
> +///     }
> +/// }
> +/// ```
> +///
> +/// ```rust
> +/// use kernel::io::poll::read_poll_timeout;
> +/// use kernel::time::Delta;
> +/// use kernel::sync::{SpinLock, new_spinlock};
> +///
> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
> +/// let g = lock.lock();
> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
> +/// drop(g);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +#[track_caller]
> +pub fn read_poll_timeout<Op, Cond, T>(
> +    mut op: Op,
> +    mut cond: Cond,
> +    sleep_delta: Delta,
> +    timeout_delta: Option<Delta>,

Fun idea I just had, though not sure it is of actuall use (probably not).
Instead of `Option<Delta> we could use `impl Into<Option<Delta>>`,
that enables to use both, so not having to write Some if we have a value.

> +) -> Result<T>
> +where
> +    Op: FnMut() -> Result<T>,
> +    Cond: FnMut(&T) -> bool,
> +{
> +    let start = Instant::now();
> +    let sleep = !sleep_delta.is_zero();
> +
> +    if sleep {
> +        might_sleep();
> +    }
> +
> +    loop {
> +        let val = op()?;
> +        if cond(&val) {
> +            // Unlike the C version, we immediately return.
> +            // We know the condition is met so we don't need to check again.
> +            return Ok(val);
> +        }
> +        if let Some(timeout_delta) = timeout_delta {
> +            if start.elapsed() > timeout_delta {
> +                // Unlike the C version, we immediately return.
> +                // We have just called `op()` so we don't need to call it again.
> +                return Err(ETIMEDOUT);
> +            }
> +        }
> +        if sleep {
> +            fsleep(sleep_delta);
> +        }
> +        // fsleep() could be busy-wait loop so we always call cpu_relax().
> +        cpu_relax();
> +    }
> +}
> +
> +/// Annotation for functions that can sleep.
> +///
> +/// Equivalent to the C side [`might_sleep()`], this function serves as
> +/// a debugging aid and a potential scheduling point.
> +///
> +/// This function can only be used in a nonatomic context.
> +#[track_caller]
> +fn might_sleep() {
> +    #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
> +    {
> +        let loc = core::panic::Location::caller();
> +        // SAFETY: FFI call.
> +        unsafe {
> +            crate::bindings::__might_sleep_precision(
> +                loc.file().as_ptr().cast(),
> +                loc.file().len() as i32,
> +                loc.line() as i32,
> +            )
> +        }
> +    }
> +
> +    // SAFETY: FFI call.
> +    unsafe { crate::bindings::might_resched() }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 496ed32b0911..415c500212dd 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -40,6 +40,7 @@
>  pub mod block;
>  #[doc(hidden)]
>  pub mod build_assert;
> +pub mod cpu;
>  pub mod cred;
>  pub mod device;
>  pub mod device_id;

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

[FUJITA Tomonori]

On Thu, 20 Feb 2025 16:04:50 +0100
Fiona Behrens <me@kloenk.dev> wrote:

>> Add read_poll_timeout functions which poll periodically until a
>> condition is met or a timeout is reached.
>>
>> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
>> macro and a simple wrapper for Rust doesn't work. So this implements
>> the same functionality in Rust.
>>
>> The C version uses usleep_range() while the Rust version uses
>> fsleep(), which uses the best sleep method so it works with spans that
>> usleep_range() doesn't work nicely with.
>>
>> The sleep_before_read argument isn't supported since there is no user
>> for now. It's rarely used in the C version.
>>
>> read_poll_timeout() can only be used in a nonatomic context. This
>> requirement is not checked by these abstractions, but it is intended
>> that klint [1] or a similar tool will be used to check it in the
>> future.
>>
>> Link: https://rust-for-linux.com/klint [1]
>> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Reviewed-by: Fiona Behrens <me@kloenk.dev>

Thanks!


>> +#[track_caller]
>> +pub fn read_poll_timeout<Op, Cond, T>(
>> +    mut op: Op,
>> +    mut cond: Cond,
>> +    sleep_delta: Delta,
>> +    timeout_delta: Option<Delta>,
> 
> Fun idea I just had, though not sure it is of actuall use (probably not).
> Instead of `Option<Delta> we could use `impl Into<Option<Delta>>`,
> that enables to use both, so not having to write Some if we have a value.

Either is fine by me. I couldn't find any functions under the
rust/kernel that use impl Into<Option<T>> as an argument. Any
rules regarding this?

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

[Andreas Hindborg]

Hi Tomonori,

FUJITA Tomonori <fujita.tomonori@gmail.com> writes:

> Add read_poll_timeout functions which poll periodically until a
> condition is met or a timeout is reached.
>
> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
> macro and a simple wrapper for Rust doesn't work. So this implements
> the same functionality in Rust.
>
> The C version uses usleep_range() while the Rust version uses
> fsleep(), which uses the best sleep method so it works with spans that
> usleep_range() doesn't work nicely with.
>
> The sleep_before_read argument isn't supported since there is no user
> for now. It's rarely used in the C version.
>
> read_poll_timeout() can only be used in a nonatomic context. This
> requirement is not checked by these abstractions, but it is intended
> that klint [1] or a similar tool will be used to check it in the
> future.
>
> Link: https://rust-for-linux.com/klint [1]
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/helpers/helpers.c |   1 +
>  rust/helpers/kernel.c  |  18 +++++++
>  rust/kernel/cpu.rs     |  13 +++++
>  rust/kernel/error.rs   |   1 +
>  rust/kernel/io.rs      |   2 +
>  rust/kernel/io/poll.rs | 120 +++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs     |   1 +
>  7 files changed, 156 insertions(+)
>  create mode 100644 rust/helpers/kernel.c
>  create mode 100644 rust/kernel/cpu.rs
>  create mode 100644 rust/kernel/io/poll.rs
>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 9565485a1a54..16d256897ccb 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -14,6 +14,7 @@
>  #include "cred.c"
>  #include "device.c"
>  #include "err.c"
> +#include "kernel.c"
>  #include "fs.c"
>  #include "io.c"
>  #include "jump_label.c"
> diff --git a/rust/helpers/kernel.c b/rust/helpers/kernel.c
> new file mode 100644
> index 000000000000..f04c04d4cc4f
> --- /dev/null
> +++ b/rust/helpers/kernel.c
> @@ -0,0 +1,18 @@
> +// 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);
> +}
> +
> +void rust_helper_might_resched(void)
> +{
> +	might_resched();
> +}
> 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.

I don't think this safety comment is sufficient. There are two other
similar comments further down.

> +    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
> index d4a73e52e3ee..be63742f517b 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -7,6 +7,8 @@
>  use crate::error::{code::EINVAL, Result};
>  use crate::{bindings, build_assert};
>  
> +pub mod poll;
> +
>  /// Raw representation of an MMIO region.
>  ///
>  /// By itself, the existence of an instance of this structure does not provide any guarantees that
> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
> new file mode 100644
> index 000000000000..5977b2082cc6
> --- /dev/null
> +++ b/rust/kernel/io/poll.rs
> @@ -0,0 +1,120 @@
> +// 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},
> +};
> +
> +/// Polls periodically until a condition is met or a timeout is reached.
> +///
> +/// The function repeatedly executes the given operation `op` closure and
> +/// checks its result using the condition closure `cond`.
> +/// If `cond` returns `true`, the function returns successfully with the result of `op`.
> +/// Otherwise, it waits for a duration specified by `sleep_delta`
> +/// before executing `op` again.
> +/// This process continues until either `cond` returns `true` or the timeout,
> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
> +///
> +/// # Examples
> +///
> +/// ```rust,ignore
> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
> +///     // The `op` closure reads the value of a specific status register.
> +///     let op = || -> Result<u16> { dev.read_ready_register() };
> +///
> +///     // The `cond` closure takes a reference to the value returned by `op`
> +///     // and checks whether the hardware is ready.
> +///     let cond = |val: &u16| *val == HW_READY;
> +///
> +///     match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
> +///         Ok(_) => {
> +///             // The hardware is ready. The returned value of the `op`` closure isn't used.
> +///             Ok(())
> +///         }
> +///         Err(e) => Err(e),
> +///     }
> +/// }
> +/// ```
> +///
> +/// ```rust
> +/// use kernel::io::poll::read_poll_timeout;
> +/// use kernel::time::Delta;
> +/// use kernel::sync::{SpinLock, new_spinlock};
> +///
> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
> +/// let g = lock.lock();
> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
> +/// drop(g);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```

I am guessing this example is present to test the call to `might_sleep`.
Could you document the reason for the test. As an example, this code is
not really usable. `#[test]` was staged for 6.15, so perhaps move this
to a unit test instead?

The test throws this BUG, which is what I think is also your intention:

  BUG: sleeping function called from invalid context at rust/doctests_kernel_generated.rs:3523
  in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 171, name: kunit_try_catch
  preempt_count: 1, expected: 0
  RCU nest depth: 0, expected: 0
  1 lock held by kunit_try_catch/171:
  #0: ffff8881003ce598 (rust/doctests_kernel_generated.rs:3521){+.+.}-{3:3}, at: rust_helper_spin_lock+0xd/0x10
  CPU: 0 UID: 0 PID: 171 Comm: kunit_try_catch Tainted: G                 N 6.14.0-rc7+ #14
  Tainted: [N]=TEST
  Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/01/2014
  Call Trace:
  <TASK>
  dump_stack_lvl+0x7b/0xa0
  dump_stack+0x14/0x16
  __might_resched_precision+0x22f/0x240
  __might_sleep_precision+0x39/0x70
  _RNvNtNtCs1cdwasc6FUb_6kernel2io4poll11might_sleep+0x19/0x20
  rust_doctest_kernel_io_poll_rs_0+0xa5/0x1f0
  kunit_try_run_case+0x73/0x150
  ? trace_hardirqs_on+0x5a/0x90
  kunit_generic_run_threadfn_adapter+0x1a/0x30
  kthread+0x219/0x230
  ? kunit_try_catch_run+0x230/0x230
  ? __do_trace_sched_kthread_stop_ret+0x50/0x50
  ret_from_fork+0x35/0x40
  ? __do_trace_sched_kthread_stop_ret+0x50/0x50
  ret_from_fork_asm+0x11/0x20
  </TASK>

Kunit does not pick this up as a failure, but it _should_, and hopefully
it will soon (TM).

So, we should probably expect failure when we get that fixed. And
perhaps for now disable the test or add a TODO to change to expect fail
when we fix kunit.


Best regards,
Andreas Hindborg

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

[FUJITA Tomonori]

Sorry, I somehow missed this email.

On Sat, 22 Mar 2025 17:02:31 +0100
Andreas Hindborg <a.hindborg@kernel.org> wrote:

>> +/// 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.
> 
> I don't think this safety comment is sufficient. There are two other
> similar comments further down.

Updated the comment.

>> +/// ```rust
>> +/// use kernel::io::poll::read_poll_timeout;
>> +/// use kernel::time::Delta;
>> +/// use kernel::sync::{SpinLock, new_spinlock};
>> +///
>> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
>> +/// let g = lock.lock();
>> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
>> +/// drop(g);
>> +///
>> +/// # Ok::<(), Error>(())
>> +/// ```
> 
> I am guessing this example is present to test the call to `might_sleep`.

I also guess so. Boqun wrote this test, IIRC.

> Could you document the reason for the test. As an example, this code is
> not really usable. `#[test]` was staged for 6.15, so perhaps move this
> to a unit test instead?
> 
> The test throws this BUG, which is what I think is also your intention:

might_sleep() doesn't throw BUG(), just a warning. Can the test
infrastructure handle such?

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

[Andreas Hindborg]

"FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:

> Sorry, I somehow missed this email.
>
> On Sat, 22 Mar 2025 17:02:31 +0100
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>>> +/// 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.
>>
>> I don't think this safety comment is sufficient. There are two other
>> similar comments further down.
>
> Updated the comment.
>
>>> +/// ```rust
>>> +/// use kernel::io::poll::read_poll_timeout;
>>> +/// use kernel::time::Delta;
>>> +/// use kernel::sync::{SpinLock, new_spinlock};
>>> +///
>>> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
>>> +/// let g = lock.lock();
>>> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
>>> +/// drop(g);
>>> +///
>>> +/// # Ok::<(), Error>(())
>>> +/// ```
>>
>> I am guessing this example is present to test the call to `might_sleep`.
>
> I also guess so. Boqun wrote this test, IIRC.
>
>> Could you document the reason for the test. As an example, this code is
>> not really usable. `#[test]` was staged for 6.15, so perhaps move this
>> to a unit test instead?
>>
>> The test throws this BUG, which is what I think is also your intention:
>
> might_sleep() doesn't throw BUG(), just a warning. Can the test
> infrastructure handle such?

As I wrote, kunit does not handle this. But I am confused about the
bug/warn comment. The trace I pasted clearly says "BUG"?

I think we should just remove this test for now.


Best regards,
Andreas Hindborg

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

[FUJITA Tomonori]

On Mon, 11 Aug 2025 11:42:46 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

>>> Could you document the reason for the test. As an example, this code is
>>> not really usable. `#[test]` was staged for 6.15, so perhaps move this
>>> to a unit test instead?
>>>
>>> The test throws this BUG, which is what I think is also your intention:
>>
>> might_sleep() doesn't throw BUG(), just a warning. Can the test
>> infrastructure handle such?
> 
> As I wrote, kunit does not handle this. But I am confused about the
> bug/warn comment. The trace I pasted clearly says "BUG"?

Yeah, might_sleep() says "BUG" like BUG() macros but they are
different. might_sleep() simply prints debug information.


> I think we should just remove this test for now.

I'll do in the next version.

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

[Daniel Almeida]

Hi Fujita,

> On 20 Feb 2025, at 04:06, FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> 
> Add read_poll_timeout functions which poll periodically until a
> condition is met or a timeout is reached.
> 
> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
> macro and a simple wrapper for Rust doesn't work. So this implements
> the same functionality in Rust.
> 
> The C version uses usleep_range() while the Rust version uses
> fsleep(), which uses the best sleep method so it works with spans that
> usleep_range() doesn't work nicely with.
> 
> The sleep_before_read argument isn't supported since there is no user
> for now. It's rarely used in the C version.
> 
> read_poll_timeout() can only be used in a nonatomic context. This
> requirement is not checked by these abstractions, but it is intended
> that klint [1] or a similar tool will be used to check it in the
> future.
> 
> Link: https://rust-for-linux.com/klint [1]
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

This appears to be the last version of this patch. Do you have any plans to
keep working on it? Is there anything I can do to help? :)

If you don’t have the time to work on this, I can pick it up for you.

— Daniel

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

[FUJITA Tomonori]

On Mon, 28 Jul 2025 09:44:39 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

>> Add read_poll_timeout functions which poll periodically until a
>> condition is met or a timeout is reached.
>> 
>> The C's read_poll_timeout (include/linux/iopoll.h) is a complicated
>> macro and a simple wrapper for Rust doesn't work. So this implements
>> the same functionality in Rust.
>> 
>> The C version uses usleep_range() while the Rust version uses
>> fsleep(), which uses the best sleep method so it works with spans that
>> usleep_range() doesn't work nicely with.
>> 
>> The sleep_before_read argument isn't supported since there is no user
>> for now. It's rarely used in the C version.
>> 
>> read_poll_timeout() can only be used in a nonatomic context. This
>> requirement is not checked by these abstractions, but it is intended
>> that klint [1] or a similar tool will be used to check it in the
>> future.
>> 
>> Link: https://rust-for-linux.com/klint [1]
>> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> This appears to be the last version of this patch. Do you have any plans to
> keep working on it? Is there anything I can do to help? :)
> 
> If you don’t have the time to work on this, I can pick it up for you.

All the dependencies for this patch (timer, fsleep, might_sleep, etc)
are planned to be merged in 6.17-rc1, and I'll submit the updated
version after the rc1 release.

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

[Danilo Krummrich]

On 7/28/25 2:52 PM, FUJITA Tomonori wrote:
> All the dependencies for this patch (timer, fsleep, might_sleep, etc)
> are planned to be merged in 6.17-rc1, and I'll submit the updated
> version after the rc1 release.

Can you please Cc Alex and me on this?

Thanks,
Danilo

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

[FUJITA Tomonori]

On Mon, 28 Jul 2025 14:57:16 +0200
Danilo Krummrich <kernel@dakr.org> wrote:

> On 7/28/25 2:52 PM, FUJITA Tomonori wrote:
>> All the dependencies for this patch (timer, fsleep, might_sleep, etc)
>> are planned to be merged in 6.17-rc1, and I'll submit the updated
>> version after the rc1 release.
> 
> Can you please Cc Alex and me on this?

Sure.

read_poll_timeout() works for drm drivers? read_poll_timeout_atomic()
are necessary too?

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

[Danilo Krummrich]

On Mon Jul 28, 2025 at 3:08 PM CEST, FUJITA Tomonori wrote:
> On Mon, 28 Jul 2025 14:57:16 +0200
> Danilo Krummrich <kernel@dakr.org> wrote:
>
>> On 7/28/25 2:52 PM, FUJITA Tomonori wrote:
>>> All the dependencies for this patch (timer, fsleep, might_sleep, etc)
>>> are planned to be merged in 6.17-rc1, and I'll submit the updated
>>> version after the rc1 release.
>> 
>> Can you please Cc Alex and me on this?
>
> Sure.
>
> read_poll_timeout() works for drm drivers? read_poll_timeout_atomic()
> are necessary too?

Please see my other reply on this patch.

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

[Daniel Almeida]

> On 28 Jul 2025, at 10:08, FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> 
> On Mon, 28 Jul 2025 14:57:16 +0200
> Danilo Krummrich <kernel@dakr.org> wrote:
> 
>> On 7/28/25 2:52 PM, FUJITA Tomonori wrote:
>>> All the dependencies for this patch (timer, fsleep, might_sleep, etc)
>>> are planned to be merged in 6.17-rc1, and I'll submit the updated
>>> version after the rc1 release.
>> 
>> Can you please Cc Alex and me on this?
> 
> Sure.
> 
> read_poll_timeout() works for drm drivers? read_poll_timeout_atomic()
> are necessary too?

Tyr needs read_poll_timeout_atomic() too, but I never really understood
if that's not achievable by setting sleep_delta==0, which skips the
call to fsleep().

— Daniel

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

[Danilo Krummrich]

On Mon Jul 28, 2025 at 4:13 PM CEST, Daniel Almeida wrote:
> Tyr needs read_poll_timeout_atomic() too, but I never really understood
> if that's not achievable by setting sleep_delta==0, which skips the
> call to fsleep().

read_poll_timeout_atomic() uses udelay() instead of fsleep(), where the latter
may sleep.

In [1] I explicitly asked for not having the

	if sleep_delta != 0 {
	   might_sleep();
	}

conditional.

It hides bugs and callers should be explicit about what they want. Either they
want something that actually may sleep or they want something that can be used
from atomic context.

Even better, in Rust we can use Klint to validate correct usage at compile time
with this separation.

[1] https://lore.kernel.org/lkml/DBNPR4KQZXY5.279JBMO315A12@kernel.org/

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

[Danilo Krummrich]

On Thu Feb 20, 2025 at 8:06 AM CET, FUJITA Tomonori wrote:
> 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() }
> +}

Please split this out in a separate patch.

> 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
> index d4a73e52e3ee..be63742f517b 100644
> --- a/rust/kernel/io.rs
> +++ b/rust/kernel/io.rs
> @@ -7,6 +7,8 @@
>  use crate::error::{code::EINVAL, Result};
>  use crate::{bindings, build_assert};
>  
> +pub mod poll;
> +
>  /// Raw representation of an MMIO region.
>  ///
>  /// By itself, the existence of an instance of this structure does not provide any guarantees that
> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
> new file mode 100644
> index 000000000000..5977b2082cc6
> --- /dev/null
> +++ b/rust/kernel/io/poll.rs
> @@ -0,0 +1,120 @@
> +// 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},
> +};
> +
> +/// Polls periodically until a condition is met or a timeout is reached.
> +///
> +/// The function repeatedly executes the given operation `op` closure and
> +/// checks its result using the condition closure `cond`.

I'd add an empty line here,

> +/// If `cond` returns `true`, the function returns successfully with the result of `op`.
> +/// Otherwise, it waits for a duration specified by `sleep_delta`
> +/// before executing `op` again.

and here.

> +/// This process continues until either `cond` returns `true` or the timeout,
> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
> +///
> +/// # Examples
> +///
> +/// ```rust,ignore

Why ignore? This should be possible to compile test.

> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {

I think the parameter here can just be `&Io<SIZE>`.

> +///     // The `op` closure reads the value of a specific status register.
> +///     let op = || -> Result<u16> { dev.read_ready_register() };
> +///
> +///     // The `cond` closure takes a reference to the value returned by `op`
> +///     // and checks whether the hardware is ready.
> +///     let cond = |val: &u16| *val == HW_READY;
> +///
> +///     match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
> +///         Ok(_) => {
> +///             // The hardware is ready. The returned value of the `op`` closure isn't used.
> +///             Ok(())
> +///         }
> +///         Err(e) => Err(e),
> +///     }
> +/// }
> +/// ```
> +///
> +/// ```rust
> +/// use kernel::io::poll::read_poll_timeout;
> +/// use kernel::time::Delta;
> +/// use kernel::sync::{SpinLock, new_spinlock};
> +///
> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
> +/// let g = lock.lock();
> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
> +/// drop(g);
> +///
> +/// # Ok::<(), Error>(())
> +/// ```
> +#[track_caller]
> +pub fn read_poll_timeout<Op, Cond, T>(
> +    mut op: Op,
> +    mut cond: Cond,
> +    sleep_delta: Delta,
> +    timeout_delta: Option<Delta>,
> +) -> Result<T>
> +where
> +    Op: FnMut() -> Result<T>,
> +    Cond: FnMut(&T) -> bool,
> +{
> +    let start = Instant::now();
> +    let sleep = !sleep_delta.is_zero();
> +
> +    if sleep {
> +        might_sleep();
> +    }

I think a conditional might_sleep() is not great.

I also think we can catch this at compile time, if we add two different variants
of read_poll_timeout() instead and be explicit about it. We could get Klint to
catch such issues for us at compile time.

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

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

[FUJITA Tomonori]

On Mon, 28 Jul 2025 15:13:45 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:

> On Thu Feb 20, 2025 at 8:06 AM CET, FUJITA Tomonori wrote:
>> 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() }
>> +}
> 
> Please split this out in a separate patch.

I will.

>> 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
>> index d4a73e52e3ee..be63742f517b 100644
>> --- a/rust/kernel/io.rs
>> +++ b/rust/kernel/io.rs
>> @@ -7,6 +7,8 @@
>>  use crate::error::{code::EINVAL, Result};
>>  use crate::{bindings, build_assert};
>>  
>> +pub mod poll;
>> +
>>  /// Raw representation of an MMIO region.
>>  ///
>>  /// By itself, the existence of an instance of this structure does not provide any guarantees that
>> diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
>> new file mode 100644
>> index 000000000000..5977b2082cc6
>> --- /dev/null
>> +++ b/rust/kernel/io/poll.rs
>> @@ -0,0 +1,120 @@
>> +// 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},
>> +};
>> +
>> +/// Polls periodically until a condition is met or a timeout is reached.
>> +///
>> +/// The function repeatedly executes the given operation `op` closure and
>> +/// checks its result using the condition closure `cond`.
> 
> I'd add an empty line here,
>
>> +/// If `cond` returns `true`, the function returns successfully with the result of `op`.
>> +/// Otherwise, it waits for a duration specified by `sleep_delta`
>> +/// before executing `op` again.
> 
> and here.

Sure, I'll add at both places.

>> +/// This process continues until either `cond` returns `true` or the timeout,
>> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
>> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
>> +///
>> +/// # Examples
>> +///
>> +/// ```rust,ignore
> 
> Why ignore? This should be possible to compile test.

https://lore.kernel.org/rust-for-linux/CEF87294-8580-4C84-BEA3-EB72E63ED7DF@collabora.com/

>> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
> 
> I think the parameter here can just be `&Io<SIZE>`.
> 
>> +///     // The `op` closure reads the value of a specific status register.
>> +///     let op = || -> Result<u16> { dev.read_ready_register() };
>> +///
>> +///     // The `cond` closure takes a reference to the value returned by `op`
>> +///     // and checks whether the hardware is ready.
>> +///     let cond = |val: &u16| *val == HW_READY;
>> +///
>> +///     match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
>> +///         Ok(_) => {
>> +///             // The hardware is ready. The returned value of the `op`` closure isn't used.
>> +///             Ok(())
>> +///         }
>> +///         Err(e) => Err(e),
>> +///     }
>> +/// }
>> +/// ```
>> +///
>> +/// ```rust
>> +/// use kernel::io::poll::read_poll_timeout;
>> +/// use kernel::time::Delta;
>> +/// use kernel::sync::{SpinLock, new_spinlock};
>> +///
>> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
>> +/// let g = lock.lock();
>> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
>> +/// drop(g);
>> +///
>> +/// # Ok::<(), Error>(())
>> +/// ```
>> +#[track_caller]
>> +pub fn read_poll_timeout<Op, Cond, T>(
>> +    mut op: Op,
>> +    mut cond: Cond,
>> +    sleep_delta: Delta,
>> +    timeout_delta: Option<Delta>,
>> +) -> Result<T>
>> +where
>> +    Op: FnMut() -> Result<T>,
>> +    Cond: FnMut(&T) -> bool,
>> +{
>> +    let start = Instant::now();
>> +    let sleep = !sleep_delta.is_zero();
>> +
>> +    if sleep {
>> +        might_sleep();
>> +    }
> 
> I think a conditional might_sleep() is not great.
> 
> I also think we can catch this at compile time, if we add two different variants
> of read_poll_timeout() instead and be explicit about it. We could get Klint to
> catch such issues for us at compile time.

Your point is that functions which cannot be used in atomic context
should be clearly separated into different ones. Then Klint might be
able to detect such usage at compile time, right?

How about dropping the conditional might_sleep() and making
read_poll_timeout return an error with zero sleep_delta?

Drivers which need busy-loop (without even udelay) can
call read_poll_timeout_atomic() with zero delay.

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

[Danilo Krummrich]

On Sat Aug 2, 2025 at 3:42 AM CEST, FUJITA Tomonori wrote:
> On Mon, 28 Jul 2025 15:13:45 +0200
> "Danilo Krummrich" <dakr@kernel.org> wrote:
>> On Thu Feb 20, 2025 at 8:06 AM CET, FUJITA Tomonori wrote:
>>> +/// This process continues until either `cond` returns `true` or the timeout,
>>> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
>>> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
>>> +///
>>> +/// # Examples
>>> +///
>>> +/// ```rust,ignore
>> 
>> Why ignore? This should be possible to compile test.
>
> https://lore.kernel.org/rust-for-linux/CEF87294-8580-4C84-BEA3-EB72E63ED7DF@collabora.com/

I disagree with that. 'ignore' should only be used if we can't make it compile.

In this case we can make it compile, we just can't run it, since there's no real
HW underneath that we can read registers from.

An example that isn't compiled will eventually be forgotten to be updated when
things are changed.

>>> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
>> 
>> I think the parameter here can just be `&Io<SIZE>`.
>> 
>>> +///     // The `op` closure reads the value of a specific status register.
>>> +///     let op = || -> Result<u16> { dev.read_ready_register() };
>>> +///
>>> +///     // The `cond` closure takes a reference to the value returned by `op`
>>> +///     // and checks whether the hardware is ready.
>>> +///     let cond = |val: &u16| *val == HW_READY;
>>> +///
>>> +///     match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
>>> +///         Ok(_) => {
>>> +///             // The hardware is ready. The returned value of the `op`` closure isn't used.
>>> +///             Ok(())
>>> +///         }
>>> +///         Err(e) => Err(e),
>>> +///     }
>>> +/// }
>>> +/// ```
>>> +///
>>> +/// ```rust
>>> +/// use kernel::io::poll::read_poll_timeout;
>>> +/// use kernel::time::Delta;
>>> +/// use kernel::sync::{SpinLock, new_spinlock};
>>> +///
>>> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
>>> +/// let g = lock.lock();
>>> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
>>> +/// drop(g);
>>> +///
>>> +/// # Ok::<(), Error>(())
>>> +/// ```
>>> +#[track_caller]
>>> +pub fn read_poll_timeout<Op, Cond, T>(
>>> +    mut op: Op,
>>> +    mut cond: Cond,
>>> +    sleep_delta: Delta,
>>> +    timeout_delta: Option<Delta>,
>>> +) -> Result<T>
>>> +where
>>> +    Op: FnMut() -> Result<T>,
>>> +    Cond: FnMut(&T) -> bool,
>>> +{
>>> +    let start = Instant::now();
>>> +    let sleep = !sleep_delta.is_zero();
>>> +
>>> +    if sleep {
>>> +        might_sleep();
>>> +    }
>> 
>> I think a conditional might_sleep() is not great.
>> 
>> I also think we can catch this at compile time, if we add two different variants
>> of read_poll_timeout() instead and be explicit about it. We could get Klint to
>> catch such issues for us at compile time.
>
> Your point is that functions which cannot be used in atomic context
> should be clearly separated into different ones. Then Klint might be
> able to detect such usage at compile time, right?
>
> How about dropping the conditional might_sleep() and making
> read_poll_timeout return an error with zero sleep_delta?

Yes, let's always call might_sleep(), the conditional is very error prone. We
want to see the warning splat whenever someone calls read_poll_timeout() from
atomic context.

Yes, with zero sleep_delta it could be called from atomic context technically,
but if drivers rely on this and wrap this into higher level helpers it's very
easy to miss a subtle case and end up with non-zero sleep_delta within an atomic
context for some rare condition that then is hard to debug.

As for making read_poll_timeout() return a error with zero sleep_delta, I don't
see a reason to do that. If a driver wraps read_poll_timeout() in its own
function that sometimes sleeps and sometimes does not, based on some condition,
but is never called from atomic context, that's fine.

> Drivers which need busy-loop (without even udelay) can
> call read_poll_timeout_atomic() with zero delay.

It's not the zero delay or zero sleep_delta that makes the difference  it's
really the fact the one can be called from atomic context and one can't be.

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

[FUJITA Tomonori]

On Sat, 02 Aug 2025 13:06:04 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:

> On Sat Aug 2, 2025 at 3:42 AM CEST, FUJITA Tomonori wrote:
>> On Mon, 28 Jul 2025 15:13:45 +0200
>> "Danilo Krummrich" <dakr@kernel.org> wrote:
>>> On Thu Feb 20, 2025 at 8:06 AM CET, FUJITA Tomonori wrote:
>>>> +/// This process continues until either `cond` returns `true` or the timeout,
>>>> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
>>>> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
>>>> +///
>>>> +/// # Examples
>>>> +///
>>>> +/// ```rust,ignore
>>> 
>>> Why ignore? This should be possible to compile test.
>>
>> https://lore.kernel.org/rust-for-linux/CEF87294-8580-4C84-BEA3-EB72E63ED7DF@collabora.com/
> 
> I disagree with that. 'ignore' should only be used if we can't make it compile.
> 
> In this case we can make it compile, we just can't run it, since there's no real
> HW underneath that we can read registers from.
> 
> An example that isn't compiled will eventually be forgotten to be updated when
> things are changed.

I also prefer the example that can be compiled however I can't think
of a compilable example that is similar to actual use cases (for
example, waiting for some hardware condition). Do you have any ideas?

>>>> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
>>> 
>>> I think the parameter here can just be `&Io<SIZE>`.
>>> 
>>>> +///     // The `op` closure reads the value of a specific status register.
>>>> +///     let op = || -> Result<u16> { dev.read_ready_register() };
>>>> +///
>>>> +///     // The `cond` closure takes a reference to the value returned by `op`
>>>> +///     // and checks whether the hardware is ready.
>>>> +///     let cond = |val: &u16| *val == HW_READY;
>>>> +///
>>>> +///     match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
>>>> +///         Ok(_) => {
>>>> +///             // The hardware is ready. The returned value of the `op`` closure isn't used.
>>>> +///             Ok(())
>>>> +///         }
>>>> +///         Err(e) => Err(e),
>>>> +///     }
>>>> +/// }
>>>> +/// ```
>>>> +///
>>>> +/// ```rust
>>>> +/// use kernel::io::poll::read_poll_timeout;
>>>> +/// use kernel::time::Delta;
>>>> +/// use kernel::sync::{SpinLock, new_spinlock};
>>>> +///
>>>> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
>>>> +/// let g = lock.lock();
>>>> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
>>>> +/// drop(g);
>>>> +///
>>>> +/// # Ok::<(), Error>(())
>>>> +/// ```
>>>> +#[track_caller]
>>>> +pub fn read_poll_timeout<Op, Cond, T>(
>>>> +    mut op: Op,
>>>> +    mut cond: Cond,
>>>> +    sleep_delta: Delta,
>>>> +    timeout_delta: Option<Delta>,
>>>> +) -> Result<T>
>>>> +where
>>>> +    Op: FnMut() -> Result<T>,
>>>> +    Cond: FnMut(&T) -> bool,
>>>> +{
>>>> +    let start = Instant::now();
>>>> +    let sleep = !sleep_delta.is_zero();
>>>> +
>>>> +    if sleep {
>>>> +        might_sleep();
>>>> +    }
>>> 
>>> I think a conditional might_sleep() is not great.
>>> 
>>> I also think we can catch this at compile time, if we add two different variants
>>> of read_poll_timeout() instead and be explicit about it. We could get Klint to
>>> catch such issues for us at compile time.
>>
>> Your point is that functions which cannot be used in atomic context
>> should be clearly separated into different ones. Then Klint might be
>> able to detect such usage at compile time, right?
>>
>> How about dropping the conditional might_sleep() and making
>> read_poll_timeout return an error with zero sleep_delta?
> 
> Yes, let's always call might_sleep(), the conditional is very error prone. We
> want to see the warning splat whenever someone calls read_poll_timeout() from
> atomic context.
> 
> Yes, with zero sleep_delta it could be called from atomic context technically,
> but if drivers rely on this and wrap this into higher level helpers it's very
> easy to miss a subtle case and end up with non-zero sleep_delta within an atomic
> context for some rare condition that then is hard to debug.
> 
> As for making read_poll_timeout() return a error with zero sleep_delta, I don't
> see a reason to do that. If a driver wraps read_poll_timeout() in its own
> function that sometimes sleeps and sometimes does not, based on some condition,
> but is never called from atomic context, that's fine.

Ok, let's always call might_sleep, even when there's no possibility of
sleeping.

>> Drivers which need busy-loop (without even udelay) can
>> call read_poll_timeout_atomic() with zero delay.
> 
> It's not the zero delay or zero sleep_delta that makes the difference  it's
> really the fact the one can be called from atomic context and one can't be.

That's what I meant.

Since calling read_poll_timeout_atomic() with zero delay can achieve
the same thing as calling read_poll_timeout with zero sleep delta, I
thought the zero sleep delta in read_poll_timeout is unncessary.

But as in the use case you mentioned above, a driver could use the
sleep delta that is not const and calls the function with both
non-zero and zero delta values so let's keep zero sleep delta support.

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

[Danilo Krummrich]

On Tue Aug 5, 2025 at 3:37 PM CEST, FUJITA Tomonori wrote:
> On Sat, 02 Aug 2025 13:06:04 +0200
> "Danilo Krummrich" <dakr@kernel.org> wrote:
>
>> On Sat Aug 2, 2025 at 3:42 AM CEST, FUJITA Tomonori wrote:
>>> On Mon, 28 Jul 2025 15:13:45 +0200
>>> "Danilo Krummrich" <dakr@kernel.org> wrote:
>>>> On Thu Feb 20, 2025 at 8:06 AM CET, FUJITA Tomonori wrote:
>>>>> +/// This process continues until either `cond` returns `true` or the timeout,
>>>>> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
>>>>> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
>>>>> +///
>>>>> +/// # Examples
>>>>> +///
>>>>> +/// ```rust,ignore
>>>> 
>>>> Why ignore? This should be possible to compile test.
>>>
>>> https://lore.kernel.org/rust-for-linux/CEF87294-8580-4C84-BEA3-EB72E63ED7DF@collabora.com/
>> 
>> I disagree with that. 'ignore' should only be used if we can't make it compile.
>> 
>> In this case we can make it compile, we just can't run it, since there's no real
>> HW underneath that we can read registers from.
>> 
>> An example that isn't compiled will eventually be forgotten to be updated when
>> things are changed.
>
> I also prefer the example that can be compiled however I can't think
> of a compilable example that is similar to actual use cases (for
> example, waiting for some hardware condition). Do you have any ideas?

With my suggestion below, it should be compilable.

When you just take a &Io<SIZE> as argument in wait_for_hardware() you can call
io.read(). Then define HW_READY as some random value and it should compile.

>>>>> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
>>>> 
>>>> I think the parameter here can just be `&Io<SIZE>`.
>>>> 
>>>>> +///     // The `op` closure reads the value of a specific status register.
>>>>> +///     let op = || -> Result<u16> { dev.read_ready_register() };
>>>>> +///
>>>>> +///     // The `cond` closure takes a reference to the value returned by `op`
>>>>> +///     // and checks whether the hardware is ready.
>>>>> +///     let cond = |val: &u16| *val == HW_READY;
>>>>> +///
>>>>> +///     match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
>>>>> +///         Ok(_) => {
>>>>> +///             // The hardware is ready. The returned value of the `op`` closure isn't used.
>>>>> +///             Ok(())
>>>>> +///         }
>>>>> +///         Err(e) => Err(e),
>>>>> +///     }
>>>>> +/// }
>>>>> +/// ```

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

[Andrew Lunn]

> I also prefer the example that can be compiled however I can't think
> of a compilable example that is similar to actual use cases (for
> example, waiting for some hardware condition). Do you have any ideas?

Does it have to be successfully runnable? Just read from address
0x42. If it actually gets run, it will trigger an Opps, but it should
be obvious why.

	Andrew

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

[Danilo Krummrich]

On Tue Aug 5, 2025 at 3:53 PM CEST, Andrew Lunn wrote:
>> I also prefer the example that can be compiled however I can't think
>> of a compilable example that is similar to actual use cases (for
>> example, waiting for some hardware condition). Do you have any ideas?
>
> Does it have to be successfully runnable? Just read from address
> 0x42. If it actually gets run, it will trigger an Opps, but it should
> be obvious why.

No, we have all three options.

  (1) Don't do anything with the code.
  (2) Compile it, but don't run it.
  (3) Compile it and run it as kunit test.

In this case we want to compile it, but not actually run it.

- Danilo

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

[Daniel Almeida]

> On 2 Aug 2025, at 08:06, Danilo Krummrich <dakr@kernel.org> wrote:
> 
> On Sat Aug 2, 2025 at 3:42 AM CEST, FUJITA Tomonori wrote:
>> On Mon, 28 Jul 2025 15:13:45 +0200
>> "Danilo Krummrich" <dakr@kernel.org> wrote:
>>> On Thu Feb 20, 2025 at 8:06 AM CET, FUJITA Tomonori wrote:
>>>> +/// This process continues until either `cond` returns `true` or the timeout,
>>>> +/// specified by `timeout_delta`, is reached. If `timeout_delta` is `None`,
>>>> +/// polling continues indefinitely until `cond` evaluates to `true` or an error occurs.
>>>> +///
>>>> +/// # Examples
>>>> +///
>>>> +/// ```rust,ignore
>>> 
>>> Why ignore? This should be possible to compile test.
>> 
>> https://lore.kernel.org/rust-for-linux/CEF87294-8580-4C84-BEA3-EB72E63ED7DF@collabora.com/
> 
> I disagree with that. 'ignore' should only be used if we can't make it compile.
> 
> In this case we can make it compile, we just can't run it, since there's no real
> HW underneath that we can read registers from.
> 
> An example that isn't compiled will eventually be forgotten to be updated when
> things are changed.
> 
>>>> +/// fn wait_for_hardware(dev: &mut Device) -> Result<()> {
>>> 
>>> I think the parameter here can just be `&Io<SIZE>`.
>>> 
>>>> +///     // The `op` closure reads the value of a specific status register.
>>>> +///     let op = || -> Result<u16> { dev.read_ready_register() };
>>>> +///
>>>> +///     // The `cond` closure takes a reference to the value returned by `op`
>>>> +///     // and checks whether the hardware is ready.
>>>> +///     let cond = |val: &u16| *val == HW_READY;
>>>> +///
>>>> +///     match read_poll_timeout(op, cond, Delta::from_millis(50), Some(Delta::from_secs(3))) {
>>>> +///         Ok(_) => {
>>>> +///             // The hardware is ready. The returned value of the `op`` closure isn't used.
>>>> +///             Ok(())
>>>> +///         }
>>>> +///         Err(e) => Err(e),
>>>> +///     }
>>>> +/// }
>>>> +/// ```
>>>> +///
>>>> +/// ```rust
>>>> +/// use kernel::io::poll::read_poll_timeout;
>>>> +/// use kernel::time::Delta;
>>>> +/// use kernel::sync::{SpinLock, new_spinlock};
>>>> +///
>>>> +/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
>>>> +/// let g = lock.lock();
>>>> +/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Some(Delta::from_micros(42)));
>>>> +/// drop(g);
>>>> +///
>>>> +/// # Ok::<(), Error>(())
>>>> +/// ```
>>>> +#[track_caller]
>>>> +pub fn read_poll_timeout<Op, Cond, T>(
>>>> +    mut op: Op,
>>>> +    mut cond: Cond,
>>>> +    sleep_delta: Delta,
>>>> +    timeout_delta: Option<Delta>,
>>>> +) -> Result<T>
>>>> +where
>>>> +    Op: FnMut() -> Result<T>,
>>>> +    Cond: FnMut(&T) -> bool,
>>>> +{
>>>> +    let start = Instant::now();
>>>> +    let sleep = !sleep_delta.is_zero();
>>>> +
>>>> +    if sleep {
>>>> +        might_sleep();
>>>> +    }
>>> 
>>> I think a conditional might_sleep() is not great.
>>> 
>>> I also think we can catch this at compile time, if we add two different variants
>>> of read_poll_timeout() instead and be explicit about it. We could get Klint to
>>> catch such issues for us at compile time.
>> 
>> Your point is that functions which cannot be used in atomic context
>> should be clearly separated into different ones. Then Klint might be
>> able to detect such usage at compile time, right?
>> 
>> How about dropping the conditional might_sleep() and making
>> read_poll_timeout return an error with zero sleep_delta?
> 
> Yes, let's always call might_sleep(), the conditional is very error prone. We
> want to see the warning splat whenever someone calls read_poll_timeout() from
> atomic context.
> 
> Yes, with zero sleep_delta it could be called from atomic context technically,
> but if drivers rely on this and wrap this into higher level helpers it's very
> easy to miss a subtle case and end up with non-zero sleep_delta within an atomic
> context for some rare condition that then is hard to debug.
> 
> As for making read_poll_timeout() return a error with zero sleep_delta, I don't
> see a reason to do that. If a driver wraps read_poll_timeout() in its own
> function that sometimes sleeps and sometimes does not, based on some condition,
> but is never called from atomic context, that's fine.
> 
>> Drivers which need busy-loop (without even udelay) can
>> call read_poll_timeout_atomic() with zero delay.
> 
> It's not the zero delay or zero sleep_delta that makes the difference  it's
> really the fact the one can be called from atomic context and one can't be.

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

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

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

Just my humble opinion.

— Daniel

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

[Danilo Krummrich]

On Tue Aug 5, 2025 at 4:01 PM CEST, Daniel Almeida wrote:
> Perhaps it’s worth it to clarify that in the docs for the future versions?

If you mean the documentation that is given out to users of the API I disagree.

We should not explain the implmentation details or the reasons for them to
users. Instead we should explain the constraints and what users can expect from
the API and what's the best practice to use it.

If you meant to add a comment to the implementation itself, that's fine of
course.

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

[FUJITA Tomonori]

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

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

diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
index 1ab065798175..cdf0540f0a98 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),
+            Some(Delta::from_secs(3)),
+        )?;
+
         Ok(())
     }
 
-- 
2.43.0

Re: [PATCH v11 0/8] rust: Add IO polling

[Daniel Almeida]

Hi Fujita,

Would you be interested in working on read_poll_timeout_atomic() as well?

There would be a user for that.

— Daniel 

Re: [PATCH v11 0/8] rust: Add IO polling

[FUJITA Tomonori]

On Thu, 27 Feb 2025 14:29:45 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

> Would you be interested in working on read_poll_timeout_atomic() as well?
> 
> There would be a user for that.

I think that I can add it.

But I prefer to push the current patchset into mainline first. I need
an ack or nack for the first patch from SCHEDULER maintainers.

Re: [PATCH v11 0/8] rust: Add IO polling

[Boqun Feng]

On Thu, Feb 20, 2025 at 04:06:02PM +0900, FUJITA Tomonori wrote:
> Add a helper function to poll periodically until a condition is met or
> a timeout is reached. By using the function, the 8th patch fixes
> QT2025 PHY driver to sleep until the hardware becomes ready.
> 
> The first patch is for sched/core, which adds
> __might_sleep_precision(), rust friendly version of __might_sleep(),
> which takes a pointer to a string with the length instead of a
> null-terminated string. Rust's core::panic::Location::file(), which
> gives the file name of a caller, doesn't provide a null-terminated
> string. __might_sleep_precision() uses a precision specifier in the
> printk format, which specifies the length of a string; a string
> doesn't need to be a null-terminated.
> 
> The remaining patches are for the Rust portion and updates to the
> MAINTAINERS file.
> 
> This introduces two new types, Instant and Delta, which represent a
> specific point in time and a span of time, respectively.
> 

I propose we should make forward-progress by merging patch #2 to #6 in
mainline first. These are relatively trivial and only affect Rust side,
and the whole patchest does show that they have potential users.

Thomas, John, Stephen, Anna-Maria and Frederic, does this sound good to
you? If so, could any of you provide Acked-by/Reviewed-by and suggest
how should we route these patches? Thanks a lot!

Regards,
Boqun

> 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.

[...]