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

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

[FUJITA Tomonori]

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

As a result of the past discussion, this introduces two new types,
Instant and Delta, which represent a specific point in time and a span
of time, respectively.

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

v5:
- set the range of Delta for fsleep function
- update comments
v4: https://lore.kernel.org/lkml/20241025033118.44452-1-fujita.tomonori@gmail.com/
- rebase on the tip tree's timers/core
- add Instant instead of using Ktime
- remove unused basic methods
- add Delta as_micros_ceil method
- use const fn for Delta from_* methods
- add more comments based on the feedback
- add a safe wrapper for cpu_relax()
- add __might_sleep() macro
v3: https://lore.kernel.org/lkml/20241016035214.2229-1-fujita.tomonori@gmail.com/
- Update time::Delta methods (use i64 for everything)
- Fix read_poll_timeout to show the proper debug info (file and line)
- Move fsleep to rust/kernel/time/delay.rs
- Round up delta for fsleep
- Access directly ktime_t instead of using ktime APIs
- Add Eq and Ord with PartialEq and PartialOrd
v2: https://lore.kernel.org/lkml/20241005122531.20298-1-fujita.tomonori@gmail.com/
- Introduce time::Delta instead of core::time::Duration
- Add some trait to Ktime for calculating timeout
- Use read_poll_timeout in QT2025 driver instead of using fsleep directly
v1: https://lore.kernel.org/netdev/20241001112512.4861-1-fujita.tomonori@gmail.com/


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

 MAINTAINERS               |  2 +
 drivers/net/phy/qt2025.rs | 10 +++-
 rust/helpers/helpers.c    |  2 +
 rust/helpers/kernel.c     | 13 ++++++
 rust/helpers/time.c       |  8 ++++
 rust/kernel/error.rs      |  1 +
 rust/kernel/io.rs         |  5 ++
 rust/kernel/io/poll.rs    | 95 ++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs        |  2 +
 rust/kernel/processor.rs  | 13 ++++++
 rust/kernel/time.rs       | 97 ++++++++++++++++++++++++++++-----------
 rust/kernel/time/delay.rs | 43 +++++++++++++++++
 12 files changed, 263 insertions(+), 28 deletions(-)
 create mode 100644 rust/helpers/kernel.c
 create mode 100644 rust/helpers/time.c
 create mode 100644 rust/kernel/io.rs
 create mode 100644 rust/kernel/io/poll.rs
 create mode 100644 rust/kernel/processor.rs
 create mode 100644 rust/kernel/time/delay.rs


base-commit: 1d4199cbbe95efaba51304cfd844bd0ccd224e61
-- 
2.43.0

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

[FUJITA Tomonori]

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

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

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

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

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

[FUJITA Tomonori]

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

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

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

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

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

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

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 4a7c6037c256..dc8289386b41 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 = core::ffi::c_ulong;
 
@@ -81,3 +87,54 @@ fn sub(self, other: Ktime) -> Ktime {
         }
     }
 }
+
+/// A span of time.
+#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
+pub struct Delta {
+    nanos: i64,
+}
+
+impl Delta {
+    /// Create a new `Delta` from a number of microseconds.
+    #[inline]
+    pub const fn from_micros(micros: i64) -> Self {
+        Self {
+            nanos: micros.saturating_mul(NSEC_PER_USEC),
+        }
+    }
+
+    /// Create a new `Delta` from a number of milliseconds.
+    #[inline]
+    pub const fn from_millis(millis: i64) -> Self {
+        Self {
+            nanos: millis.saturating_mul(NSEC_PER_MSEC),
+        }
+    }
+
+    /// Create a new `Delta` from a number of seconds.
+    #[inline]
+    pub const fn from_secs(secs: i64) -> Self {
+        Self {
+            nanos: secs.saturating_mul(NSEC_PER_SEC),
+        }
+    }
+
+    /// Return `true` if the `Detla` spans no time.
+    #[inline]
+    pub fn is_zero(self) -> bool {
+        self.as_nanos() == 0
+    }
+
+    /// Return the number of nanoseconds in the `Delta`.
+    #[inline]
+    pub fn as_nanos(self) -> i64 {
+        self.nanos
+    }
+
+    /// Return the smallest number of microseconds greater than or equal
+    /// to the value in the `Delta`.
+    #[inline]
+    pub fn as_micros_ceil(self) -> i64 {
+        self.as_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
+    }
+}
-- 
2.43.0

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

[FUJITA Tomonori]

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

Rename Ktime to Instant and modify their methods for timestamp.

Implement the subtraction operator for Instant:

Delta = Instant A - Instant B

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

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

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index dc8289386b41..e4f0a0f34d6d 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -31,59 +31,43 @@ pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
     unsafe { bindings::__msecs_to_jiffies(msecs) }
 }
 
-/// A Rust wrapper around a `ktime_t`.
+/// A specific point in time.
 #[repr(transparent)]
 #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord)]
-pub struct Ktime {
+pub struct Instant {
+    // Range from 0 to `KTIME_MAX`.
     inner: bindings::ktime_t,
 }
 
-impl Ktime {
-    /// Create a `Ktime` from a raw `ktime_t`.
+impl Instant {
+    /// Create a `Instant` from a raw `ktime_t`.
     #[inline]
-    pub fn from_raw(inner: bindings::ktime_t) -> Self {
+    fn from_raw(inner: bindings::ktime_t) -> Self {
         Self { inner }
     }
 
     /// Get the current time using `CLOCK_MONOTONIC`.
     #[inline]
-    pub fn ktime_get() -> Self {
+    pub fn now() -> Self {
         // SAFETY: It is always safe to call `ktime_get` outside of NMI context.
         Self::from_raw(unsafe { bindings::ktime_get() })
     }
 
-    /// Divide the number of nanoseconds by a compile-time constant.
     #[inline]
-    fn divns_constant<const DIV: i64>(self) -> i64 {
-        self.to_ns() / DIV
-    }
-
-    /// Returns the number of nanoseconds.
-    #[inline]
-    pub fn to_ns(self) -> i64 {
-        self.inner
-    }
-
-    /// Returns the number of milliseconds.
-    #[inline]
-    pub fn to_ms(self) -> i64 {
-        self.divns_constant::<NSEC_PER_MSEC>()
+    /// Return the amount of time elapsed since the `Instant`.
+    pub fn elapsed(&self) -> Delta {
+        Self::now() - *self
     }
 }
 
-/// Returns the number of milliseconds between two ktimes.
-#[inline]
-pub fn ktime_ms_delta(later: Ktime, earlier: Ktime) -> i64 {
-    (later - earlier).to_ms()
-}
-
-impl core::ops::Sub for Ktime {
-    type Output = Ktime;
+impl core::ops::Sub for Instant {
+    type Output = Delta;
 
+    // never overflows
     #[inline]
-    fn sub(self, other: Ktime) -> Ktime {
-        Self {
-            inner: self.inner - other.inner,
+    fn sub(self, other: Instant) -> Delta {
+        Delta {
+            nanos: self.inner - other.inner,
         }
     }
 }
-- 
2.43.0

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

[FUJITA Tomonori]

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

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

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

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

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

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 30f40149f3a9..c274546bcf78 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -21,6 +21,7 @@
 #include "slab.c"
 #include "spinlock.c"
 #include "task.c"
+#include "time.c"
 #include "uaccess.c"
 #include "wait.c"
 #include "workqueue.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 e4f0a0f34d6d..9395739b51e0 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -2,12 +2,14 @@
 
 //! Time related primitives.
 //!
-//! This module contains the kernel APIs related to time and timers that
+//! This module contains the kernel APIs related to time that
 //! have been ported or wrapped for usage by Rust code in the kernel.
 //!
 //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
 //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
 
+pub mod delay;
+
 /// The number of nanoseconds per microsecond.
 pub const NSEC_PER_USEC: i64 = bindings::NSEC_PER_USEC as i64;
 
diff --git a/rust/kernel/time/delay.rs b/rust/kernel/time/delay.rs
new file mode 100644
index 000000000000..c3c908b72a56
--- /dev/null
+++ b/rust/kernel/time/delay.rs
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Delay and sleep primitives.
+//!
+//! This module contains the kernel APIs related to delay and sleep that
+//! have been ported or wrapped for usage by Rust code in the kernel.
+//!
+//! C header: [`include/linux/delay.h`](srctree/include/linux/delay.h).
+
+use crate::time::Delta;
+use core::ffi::c_ulong;
+
+/// Sleeps for a given duration at least.
+///
+/// Equivalent to the kernel's [`fsleep`], flexible sleep function,
+/// which automatically chooses the best sleep method based on a duration.
+///
+/// `delta` must be 0 or greater and no more than u32::MAX / 2 microseconds.
+/// If a value outside the range is given, the function will sleep
+/// for u32::MAX / 2 microseconds at least.
+///
+/// This function can only be used in a nonatomic context.
+pub fn fsleep(delta: Delta) {
+    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
+    // Considering that fsleep rounds up the duration to the nearest millisecond,
+    // set the maximum value to u32::MAX / 2 microseconds.
+    const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
+
+    let duration = if delta > MAX_DURATION || delta.as_nanos() < 0 {
+        // TODO: add WARN_ONCE() when it's supported.
+        MAX_DURATION
+    } else {
+        delta
+    };
+
+    // SAFETY: FFI call.
+    unsafe {
+        // Convert the duration to microseconds and round up to preserve
+        // the guarantee; fsleep sleeps for at least the provided duration,
+        // but that it may sleep for longer under some circumstances.
+        bindings::fsleep(duration.as_micros_ceil() as c_ulong)
+    }
+}
-- 
2.43.0

[PATCH v5 5/7] MAINTAINERS: rust: Add TIMEKEEPING and TIMER abstractions

[FUJITA Tomonori]

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

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

diff --git a/MAINTAINERS b/MAINTAINERS
index 2250eb10ece1..6fb56965b4c2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10171,6 +10171,7 @@ F:	kernel/time/sleep_timeout.c
 F:	kernel/time/timer.c
 F:	kernel/time/timer_list.c
 F:	kernel/time/timer_migration.*
+F:	rust/kernel/time/delay.rs
 F:	tools/testing/selftests/timers/
 
 HIGH-SPEED SCC DRIVER FOR AX.25
@@ -23366,6 +23367,7 @@ F:	kernel/time/timeconv.c
 F:	kernel/time/timecounter.c
 F:	kernel/time/timekeeping*
 F:	kernel/time/time_test.c
+F:	rust/kernel/time.rs
 F:	tools/testing/selftests/timers/
 
 TIPC NETWORK LAYER
-- 
2.43.0

[PATCH v5 6/7] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

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

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

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

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

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

For the proper debug info, readx_poll_timeout() and __might_sleep()
are implemented as a macro. We could implement them as a normal
function if there is a clean way to get a null-terminated string
without allocation from core::panic::Location::file().

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

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

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index c274546bcf78..f9569ff1717e 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -12,6 +12,7 @@
 #include "build_assert.c"
 #include "build_bug.c"
 #include "err.c"
+#include "kernel.c"
 #include "kunit.c"
 #include "mutex.c"
 #include "page.c"
diff --git a/rust/helpers/kernel.c b/rust/helpers/kernel.c
new file mode 100644
index 000000000000..da847059260b
--- /dev/null
+++ b/rust/helpers/kernel.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/kernel.h>
+
+void rust_helper_cpu_relax(void)
+{
+	cpu_relax();
+}
+
+void rust_helper___might_sleep(const char *file, int line)
+{
+	__might_sleep(file, line);
+}
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 6f1587a2524e..d571b9587ed6 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -58,6 +58,7 @@ macro_rules! declare_err {
     declare_err!(EPIPE, "Broken pipe.");
     declare_err!(EDOM, "Math argument out of domain of func.");
     declare_err!(ERANGE, "Math result not representable.");
+    declare_err!(ETIMEDOUT, "Connection timed out.");
     declare_err!(ERESTARTSYS, "Restart the system call.");
     declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
     declare_err!(ERESTARTNOHAND, "Restart if no handler.");
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
new file mode 100644
index 000000000000..033f3c4e4adf
--- /dev/null
+++ b/rust/kernel/io.rs
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Input and Output.
+
+pub mod poll;
diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
new file mode 100644
index 000000000000..a8caa08f86f2
--- /dev/null
+++ b/rust/kernel/io/poll.rs
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! IO polling.
+//!
+//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
+
+use crate::{
+    error::{code::*, Result},
+    processor::cpu_relax,
+    time::{delay::fsleep, Delta, Instant},
+};
+
+/// Polls periodically until a condition is met or a timeout is reached.
+///
+/// Public but hidden since it should only be used from public macros.
+#[doc(hidden)]
+pub fn read_poll_timeout<Op, Cond, T: Copy>(
+    mut op: Op,
+    cond: Cond,
+    sleep_delta: Delta,
+    timeout_delta: Delta,
+) -> Result<T>
+where
+    Op: FnMut() -> Result<T>,
+    Cond: Fn(T) -> bool,
+{
+    let start = Instant::now();
+    let sleep = !sleep_delta.is_zero();
+    let timeout = !timeout_delta.is_zero();
+
+    let val = loop {
+        let val = op()?;
+        if cond(val) {
+            // Unlike the C version, we immediately return.
+            // We know a condition is met so we don't need to check again.
+            return Ok(val);
+        }
+        if timeout && start.elapsed() > timeout_delta {
+            // Should we return Err(ETIMEDOUT) here instead of call op() again
+            // without a sleep between? But we follow the C version. op() could
+            // take some time so might be worth checking again.
+            break op()?;
+        }
+        if sleep {
+            fsleep(sleep_delta);
+        }
+        // fsleep() could be busy-wait loop so we always call cpu_relax().
+        cpu_relax();
+    };
+
+    if cond(val) {
+        Ok(val)
+    } else {
+        Err(ETIMEDOUT)
+    }
+}
+
+/// Print debug information if it's called inside atomic sections.
+///
+/// Equivalent to the kernel's [`__might_sleep`].
+#[macro_export]
+macro_rules! __might_sleep {
+    () => {
+        #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
+        // SAFETY: FFI call.
+        unsafe {
+            $crate::bindings::__might_sleep(
+                c_str!(::core::file!()).as_char_ptr(),
+                ::core::line!() as i32,
+            )
+        }
+    };
+}
+
+/// Polls periodically until a condition is met or a timeout is reached.
+///
+/// `op` is called repeatedly until `cond` returns `true` or the timeout is
+///  reached. The return value of `op` is passed to `cond`.
+///
+/// `sleep_delta` is the duration to sleep between calls to `op`.
+/// If `sleep_delta` is less than one microsecond, the function will busy-wait.
+///
+/// `timeout_delta` is the maximum time to wait for `cond` to return `true`.
+///
+/// This macro can only be used in a nonatomic context.
+#[macro_export]
+macro_rules! readx_poll_timeout {
+    ($op:expr, $cond:expr, $sleep_delta:expr, $timeout_delta:expr) => {{
+        if !$sleep_delta.is_zero() {
+            $crate::__might_sleep!();
+        }
+
+        $crate::io::poll::read_poll_timeout($op, $cond, $sleep_delta, $timeout_delta)
+    }};
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 22a3bfa5a9e9..b775fd1c9be0 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -35,6 +35,7 @@
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
 pub mod firmware;
 pub mod init;
+pub mod io;
 pub mod ioctl;
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
@@ -44,6 +45,7 @@
 pub mod page;
 pub mod prelude;
 pub mod print;
+pub mod processor;
 pub mod sizes;
 pub mod rbtree;
 mod static_assert;
diff --git a/rust/kernel/processor.rs b/rust/kernel/processor.rs
new file mode 100644
index 000000000000..eeeff4be84fa
--- /dev/null
+++ b/rust/kernel/processor.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() }
+}
-- 
2.43.0

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

[FUJITA Tomonori]

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

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 28d8981f410b..f7480c19d4cc 100644
--- a/drivers/net/phy/qt2025.rs
+++ b/drivers/net/phy/qt2025.rs
@@ -18,7 +18,9 @@
     DeviceId, Driver,
 };
 use kernel::prelude::*;
+use kernel::readx_poll_timeout;
 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.
+        readx_poll_timeout!(
+            || dev.read(C45::new(Mmd::PCS, 0xd7fd)),
+            |val| val != 0x00 && val != 0x10,
+            Delta::from_millis(50),
+            Delta::from_secs(3)
+        )?;
+
         Ok(())
     }
 
-- 
2.43.0

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

[Boqun Feng]

On Fri, Nov 01, 2024 at 10:01:18AM +0900, FUJITA Tomonori wrote:
> Add a wrapper for fsleep, flexible sleep functions in
> `include/linux/delay.h` which typically deals with hardware delays.
> 
> The kernel supports several `sleep` functions to handle various
> lengths of delay. This adds fsleep, automatically chooses the best
> sleep method based on a duration.
> 
> `sleep` functions including `fsleep` belongs to TIMERS, not
> TIMEKEEPING. They are maintained separately. rust/kernel/time.rs is an
> abstraction for TIMEKEEPING. To make Rust abstractions match the C
> side, add rust/kernel/time/delay.rs for this wrapper.
> 
> fsleep() can only be used in a nonatomic context. This requirement is
> not checked by these abstractions, but it is intended that klint [1]
> or a similar tool will be used to check it in the future.
> 
> Link: https://rust-for-linux.com/klint [1]
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/helpers/helpers.c    |  1 +
>  rust/helpers/time.c       |  8 ++++++++
>  rust/kernel/time.rs       |  4 +++-
>  rust/kernel/time/delay.rs | 43 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 55 insertions(+), 1 deletion(-)
>  create mode 100644 rust/helpers/time.c
>  create mode 100644 rust/kernel/time/delay.rs
> 
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 30f40149f3a9..c274546bcf78 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -21,6 +21,7 @@
>  #include "slab.c"
>  #include "spinlock.c"
>  #include "task.c"
> +#include "time.c"
>  #include "uaccess.c"
>  #include "wait.c"
>  #include "workqueue.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 e4f0a0f34d6d..9395739b51e0 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -2,12 +2,14 @@
>  
>  //! Time related primitives.
>  //!
> -//! This module contains the kernel APIs related to time and timers that
> +//! This module contains the kernel APIs related to time that
>  //! have been ported or wrapped for usage by Rust code in the kernel.
>  //!
>  //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
>  //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
>  
> +pub mod delay;
> +
>  /// The number of nanoseconds per microsecond.
>  pub const NSEC_PER_USEC: i64 = bindings::NSEC_PER_USEC as i64;
>  
> diff --git a/rust/kernel/time/delay.rs b/rust/kernel/time/delay.rs
> new file mode 100644
> index 000000000000..c3c908b72a56
> --- /dev/null
> +++ b/rust/kernel/time/delay.rs
> @@ -0,0 +1,43 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Delay and sleep primitives.
> +//!
> +//! This module contains the kernel APIs related to delay and sleep that
> +//! have been ported or wrapped for usage by Rust code in the kernel.
> +//!
> +//! C header: [`include/linux/delay.h`](srctree/include/linux/delay.h).
> +
> +use crate::time::Delta;

Nit: I think it's better to use:

use super::Delta;

here to refer a definition in the super mod.

> +use core::ffi::c_ulong;
> +
> +/// Sleeps for a given duration at least.
> +///
> +/// Equivalent to the kernel's [`fsleep`], flexible sleep function,
> +/// which automatically chooses the best sleep method based on a duration.
> +///
> +/// `delta` must be 0 or greater and no more than u32::MAX / 2 microseconds.

Adding backquotes on "u32::MAX / 2" would make it easier to read and
generates better documentation. For example.

/// `delta` must be 0 or greater and no more than `u32::MAX / 2` microseconds.


> +/// If a value outside the range is given, the function will sleep
> +/// for u32::MAX / 2 microseconds at least.

Same here.

I would also add the converted result in seconds of `u32::MAX / 2`
microseconds to give doc readers some intuitions, like:

the function will sleep for `u32::MAX / 2` (= ~2147 seconds or ~36
minutes) at least.

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

I think it would be helpful if `Delta` has a `is_negative()` function.

The rest looks good to me. Thanks!

Regards,
Boqun

> +        // TODO: add WARN_ONCE() when it's supported.
> +        MAX_DURATION
> +    } else {
> +        delta
> +    };
> +
> +    // SAFETY: FFI call.
> +    unsafe {
> +        // Convert the duration to microseconds and round up to preserve
> +        // the guarantee; fsleep sleeps for at least the provided duration,
> +        // but that it may sleep for longer under some circumstances.
> +        bindings::fsleep(duration.as_micros_ceil() as c_ulong)
> +    }
> +}
> -- 
> 2.43.0
> 
> 

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

[Boqun Feng]

On Fri, Nov 01, 2024 at 10:01:20AM +0900, FUJITA Tomonori wrote:
[...]
> @@ -44,6 +45,7 @@
>  pub mod page;
>  pub mod prelude;
>  pub mod print;
> +pub mod processor;
>  pub mod sizes;
>  pub mod rbtree;
>  mod static_assert;
> diff --git a/rust/kernel/processor.rs b/rust/kernel/processor.rs
> new file mode 100644
> index 000000000000..eeeff4be84fa
> --- /dev/null
> +++ b/rust/kernel/processor.rs

What else would we put into this file? `smp_processor_id()` and IPI
functionality? If so, I would probably want to rename this to cpu.rs.

Regards,
Boqun

> @@ -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() }
> +}
> -- 
> 2.43.0
> 
> 

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

[Boqun Feng]

On Fri, Nov 01, 2024 at 10:01:20AM +0900, FUJITA Tomonori wrote:
> Add read_poll_timeout functions which poll periodically until a
> condition is met or a timeout is reached.
> 
> C's read_poll_timeout (include/linux/iopoll.h) is a complicated macro
> and a simple wrapper for Rust doesn't work. So this implements the
> same functionality in Rust.
> 
> The C version uses usleep_range() while the Rust version uses
> fsleep(), which uses the best sleep method so it works with spans that
> usleep_range() doesn't work nicely with.
> 
> Unlike the C version, __might_sleep() is used instead of might_sleep()
> to show proper debug info; the file name and line
> number. might_resched() could be added to match what the C version
> does but this function works without it.
> 
> The sleep_before_read argument isn't supported since there is no user
> for now. It's rarely used in the C version.
> 
> For the proper debug info, readx_poll_timeout() and __might_sleep()
> are implemented as a macro. We could implement them as a normal
> function if there is a clean way to get a null-terminated string
> without allocation from core::panic::Location::file().
> 

So printk() actually support printing a string with a precison value,
that is: a format string "%.*s" would take two inputs, one for the length
and the other for the pointer to the string, for example you can do:

	char *msg = "hello";

	printk("%.*s\n", 5, msg);

This is similar to printf() in glibc [1].

If we add another __might_sleep_precision() which accepts a file name
length:

	void __might_sleep_precision(const char *file, int len, int line)

then we don't need to use macro here, I've attached a diff based
on your whole patchset, and it seems working.

Cc printk folks to if they know any limitation on using precision.

Regards,
Boqun

[1]: https://www.gnu.org/software/libc/manual/html_node/Output-Conversion-Syntax.html#Output-Conversion-Syntax

> readx_poll_timeout() can only be used in a nonatomic context. This
> requirement is not checked by these abstractions, but it is
> intended that klint [1] or a similar tool will be used to check it
> in the future.
> 
> Link: https://rust-for-linux.com/klint [1]
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---

--------------------------------------------->8
diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
index dabb772c468f..4d368ce80db6 100644
--- a/drivers/net/phy/qt2025.rs
+++ b/drivers/net/phy/qt2025.rs
@@ -18,7 +18,7 @@
     Driver,
 };
 use kernel::prelude::*;
-use kernel::readx_poll_timeout;
+use kernel::read_poll_timeout;
 use kernel::sizes::{SZ_16K, SZ_8K};
 use kernel::time::Delta;
 
@@ -95,7 +95,7 @@ fn probe(dev: &mut phy::Device) -> Result<()> {
         // The micro-controller will start running from SRAM.
         dev.write(C45::new(Mmd::PCS, 0xe854), 0x0040)?;
 
-        readx_poll_timeout!(
+        read_poll_timeout(
             || dev.read(C45::new(Mmd::PCS, 0xd7fd)),
             |val| val != 0x00 && val != 0x10,
             Delta::from_millis(50),
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index be2e8c0a187e..b405b0d19bac 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -87,6 +87,8 @@ 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_resched_precision(const char *file, int len, int line, unsigned int offsets);
+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);
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 43e453ab7e20..f872aa18eaf0 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8543,7 +8543,7 @@ void __init sched_init(void)
 
 #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
 
-void __might_sleep(const char *file, int line)
+void __might_sleep_precision(const char *file, int len, int line)
 {
 	unsigned int state = get_current_state();
 	/*
@@ -8557,7 +8557,14 @@ void __might_sleep(const char *file, int line)
 			(void *)current->task_state_change,
 			(void *)current->task_state_change);
 
-	__might_resched(file, line, 0);
+	__might_resched_precision(file, len, line, 0);
+}
+
+void __might_sleep(const char *file, int line)
+{
+	long len = strlen(file);
+
+	__might_sleep_precision(file, len, line);
 }
 EXPORT_SYMBOL(__might_sleep);
 
@@ -8582,7 +8589,7 @@ static inline bool resched_offsets_ok(unsigned int offsets)
 	return nested == offsets;
 }
 
-void __might_resched(const char *file, int line, unsigned int offsets)
+void __might_resched_precision(const char *file, int len, int line, unsigned int offsets)
 {
 	/* Ratelimiting timestamp: */
 	static unsigned long prev_jiffy;
@@ -8605,8 +8612,8 @@ void __might_resched(const char *file, int line, unsigned int offsets)
 	/* Save this before calling printk(), since that will clobber it: */
 	preempt_disable_ip = get_preempt_disable_ip(current);
 
-	pr_err("BUG: sleeping function called from invalid context at %s:%d\n",
-	       file, line);
+	pr_err("BUG: sleeping function called from invalid context at %.*s:%d\n",
+	       len, file, line);
 	pr_err("in_atomic(): %d, irqs_disabled(): %d, non_block: %d, pid: %d, name: %s\n",
 	       in_atomic(), irqs_disabled(), current->non_block_count,
 	       current->pid, current->comm);
@@ -8631,6 +8638,13 @@ void __might_resched(const char *file, int line, unsigned int offsets)
 	dump_stack();
 	add_taint(TAINT_WARN, LOCKDEP_STILL_OK);
 }
+
+void __might_resched(const char *file, int line, unsigned int offsets)
+{
+	long len = strlen(file);
+
+	__might_resched_precision(file, len, line, offsets);
+}
 EXPORT_SYMBOL(__might_resched);
 
 void __cant_sleep(const char *file, int line, int preempt_offset)
diff --git a/rust/helpers/kernel.c b/rust/helpers/kernel.c
index da847059260b..9dff28f4618e 100644
--- a/rust/helpers/kernel.c
+++ b/rust/helpers/kernel.c
@@ -7,7 +7,7 @@ void rust_helper_cpu_relax(void)
 	cpu_relax();
 }
 
-void rust_helper___might_sleep(const char *file, int line)
+void rust_helper___might_sleep_precision(const char *file, int len, int line)
 {
-	__might_sleep(file, line);
+	__might_sleep_precision(file, len, line);
 }
diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
index a8caa08f86f2..d7e5be162b6e 100644
--- a/rust/kernel/io/poll.rs
+++ b/rust/kernel/io/poll.rs
@@ -10,10 +10,25 @@
     time::{delay::fsleep, Delta, Instant},
 };
 
+use core::panic::Location;
+
 /// Polls periodically until a condition is met or a timeout is reached.
 ///
 /// Public but hidden since it should only be used from public macros.
-#[doc(hidden)]
+///
+/// ```rust
+/// use kernel::io::poll::read_poll_timeout;
+/// use kernel::time::Delta;
+/// use kernel::sync::{SpinLock, new_spinlock};
+///
+/// let lock = KBox::pin_init(new_spinlock!(()), kernel::alloc::flags::GFP_KERNEL)?;
+/// let g = lock.lock();
+/// read_poll_timeout(|| Ok(()), |()| true, Delta::from_micros(42), Delta::from_micros(42));
+/// drop(g);
+///
+/// # Ok::<(), Error>(())
+/// ```
+#[track_caller]
 pub fn read_poll_timeout<Op, Cond, T: Copy>(
     mut op: Op,
     cond: Cond,
@@ -28,6 +43,8 @@ pub fn read_poll_timeout<Op, Cond, T: Copy>(
     let sleep = !sleep_delta.is_zero();
     let timeout = !timeout_delta.is_zero();
 
+    might_sleep(Location::caller());
+
     let val = loop {
         let val = op()?;
         if cond(val) {
@@ -55,41 +72,13 @@ pub fn read_poll_timeout<Op, Cond, T: Copy>(
     }
 }
 
-/// Print debug information if it's called inside atomic sections.
-///
-/// Equivalent to the kernel's [`__might_sleep`].
-#[macro_export]
-macro_rules! __might_sleep {
-    () => {
-        #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
-        // SAFETY: FFI call.
-        unsafe {
-            $crate::bindings::__might_sleep(
-                c_str!(::core::file!()).as_char_ptr(),
-                ::core::line!() as i32,
-            )
-        }
-    };
-}
-
-/// Polls periodically until a condition is met or a timeout is reached.
-///
-/// `op` is called repeatedly until `cond` returns `true` or the timeout is
-///  reached. The return value of `op` is passed to `cond`.
-///
-/// `sleep_delta` is the duration to sleep between calls to `op`.
-/// If `sleep_delta` is less than one microsecond, the function will busy-wait.
-///
-/// `timeout_delta` is the maximum time to wait for `cond` to return `true`.
-///
-/// This macro can only be used in a nonatomic context.
-#[macro_export]
-macro_rules! readx_poll_timeout {
-    ($op:expr, $cond:expr, $sleep_delta:expr, $timeout_delta:expr) => {{
-        if !$sleep_delta.is_zero() {
-            $crate::__might_sleep!();
-        }
-
-        $crate::io::poll::read_poll_timeout($op, $cond, $sleep_delta, $timeout_delta)
-    }};
+fn might_sleep(loc: &Location<'_>) {
+    // SAFETY: FFI call.
+    unsafe {
+        crate::bindings::__might_sleep_precision(
+            loc.file().as_ptr().cast(),
+            loc.file().len() as i32,
+            loc.line() as i32,
+        )
+    }
 }

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

[Petr Mladek]

On Wed 2024-11-06 13:35:09, Boqun Feng wrote:
> On Fri, Nov 01, 2024 at 10:01:20AM +0900, FUJITA Tomonori wrote:
> > Add read_poll_timeout functions which poll periodically until a
> > condition is met or a timeout is reached.
> > 
> > C's read_poll_timeout (include/linux/iopoll.h) is a complicated macro
> > and a simple wrapper for Rust doesn't work. So this implements the
> > same functionality in Rust.
> > 
> > The C version uses usleep_range() while the Rust version uses
> > fsleep(), which uses the best sleep method so it works with spans that
> > usleep_range() doesn't work nicely with.
> > 
> > Unlike the C version, __might_sleep() is used instead of might_sleep()
> > to show proper debug info; the file name and line
> > number. might_resched() could be added to match what the C version
> > does but this function works without it.
> > 
> > The sleep_before_read argument isn't supported since there is no user
> > for now. It's rarely used in the C version.
> > 
> > For the proper debug info, readx_poll_timeout() and __might_sleep()
> > are implemented as a macro. We could implement them as a normal
> > function if there is a clean way to get a null-terminated string
> > without allocation from core::panic::Location::file().
> > 
> 
> So printk() actually support printing a string with a precison value,
> that is: a format string "%.*s" would take two inputs, one for the length
> and the other for the pointer to the string, for example you can do:
> 
> 	char *msg = "hello";
> 
> 	printk("%.*s\n", 5, msg);
> 
> This is similar to printf() in glibc [1].
> 
> If we add another __might_sleep_precision() which accepts a file name
> length:
> 
> 	void __might_sleep_precision(const char *file, int len, int line)
> 
> then we don't need to use macro here, I've attached a diff based
> on your whole patchset, and it seems working.
> 
> Cc printk folks to if they know any limitation on using precision.

I am not aware of any printk() limitation here. The "%.*s" format
should work the same way as in printf() in the userspace.

Best Regards,
Petr

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

[Rasmus Villemoes]

On Fri, Nov 01 2024, FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> For the proper debug info, readx_poll_timeout() and __might_sleep()
> are implemented as a macro. We could implement them as a normal
> function if there is a clean way to get a null-terminated string
> without allocation from core::panic::Location::file().

Would it be too much to hope for either a compiler flag or simply
default behaviour for having the backing, static store of the file!()
&str being guaranteed to be followed by a nul character? (Of course that
nul should not be counted in the slice's length). That would in general
increase interop with C code.

This is hardly the last place where Rust code would pass
Location::file() into C, and having to pass that as a (ptr,len) pair
always and updating the receiving C code to use %.*s seems like an
uphill battle, especially when the C code passes the const char* pointer
through a few layers before it is finally passed to a printf-like
function.

And creating the nul-terminated strings with c_str! needlessly doubles
the storage needed for the file names (unless the rust compiler is smart
enough to then re-use the c_str result for the backing store of the
file!() &str).

Rasmus

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

[Alice Ryhl]

On Thu, Nov 7, 2024 at 1:50 PM Rasmus Villemoes <ravi@prevas.dk> wrote:
>
> On Fri, Nov 01 2024, FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>
> > For the proper debug info, readx_poll_timeout() and __might_sleep()
> > are implemented as a macro. We could implement them as a normal
> > function if there is a clean way to get a null-terminated string
> > without allocation from core::panic::Location::file().
>
> Would it be too much to hope for either a compiler flag or simply
> default behaviour for having the backing, static store of the file!()
> &str being guaranteed to be followed by a nul character? (Of course that
> nul should not be counted in the slice's length). That would in general
> increase interop with C code.
>
> This is hardly the last place where Rust code would pass
> Location::file() into C, and having to pass that as a (ptr,len) pair
> always and updating the receiving C code to use %.*s seems like an
> uphill battle, especially when the C code passes the const char* pointer
> through a few layers before it is finally passed to a printf-like
> function.

This is actively being discussed at:
https://github.com/rust-lang/libs-team/issues/466

> And creating the nul-terminated strings with c_str! needlessly doubles
> the storage needed for the file names (unless the rust compiler is smart
> enough to then re-use the c_str result for the backing store of the
> file!() &str).

For the case of c_str!(file!()), the compiler should do the right
thing. Not via deduplication, but via removal of unused globals.

Alice

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

[Miguel Ojeda]

On Thu, Nov 7, 2024 at 1:50 PM Rasmus Villemoes <ravi@prevas.dk> wrote:
>
> Would it be too much to hope for either a compiler flag or simply
> default behaviour for having the backing, static store of the file!()
> &str being guaranteed to be followed by a nul character? (Of course that
> nul should not be counted in the slice's length). That would in general
> increase interop with C code.

Definitely -- please see the "`c_stringify!`, `c_concat!`, `c_file!`
macros" item in:

    https://github.com/Rust-for-Linux/linux/issues/514

Relatedly, for `Location`, there is "C string equivalents
(nul-terminated) for `core::panic::Location`.", with this ACP:

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

Cheers,
Miguel

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

[FUJITA Tomonori]

On Wed, 6 Nov 2024 10:13:57 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

>> diff --git a/rust/kernel/time/delay.rs b/rust/kernel/time/delay.rs
>> new file mode 100644
>> index 000000000000..c3c908b72a56
>> --- /dev/null
>> +++ b/rust/kernel/time/delay.rs
>> @@ -0,0 +1,43 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Delay and sleep primitives.
>> +//!
>> +//! This module contains the kernel APIs related to delay and sleep that
>> +//! have been ported or wrapped for usage by Rust code in the kernel.
>> +//!
>> +//! C header: [`include/linux/delay.h`](srctree/include/linux/delay.h).
>> +
>> +use crate::time::Delta;
> 
> Nit: I think it's better to use:
> 
> use super::Delta;
> 
> here to refer a definition in the super mod.

Fixed.

>> +use core::ffi::c_ulong;
>> +
>> +/// Sleeps for a given duration at least.
>> +///
>> +/// Equivalent to the kernel's [`fsleep`], flexible sleep function,
>> +/// which automatically chooses the best sleep method based on a duration.
>> +///
>> +/// `delta` must be 0 or greater and no more than u32::MAX / 2 microseconds.
> 
> Adding backquotes on "u32::MAX / 2" would make it easier to read and
> generates better documentation. For example.
> 
> /// `delta` must be 0 or greater and no more than `u32::MAX / 2` microseconds.
>
>
>> +/// If a value outside the range is given, the function will sleep
>> +/// for u32::MAX / 2 microseconds at least.
> 
> Same here.

Updated both.

> I would also add the converted result in seconds of `u32::MAX / 2`
> microseconds to give doc readers some intuitions, like:
> 
> the function will sleep for `u32::MAX / 2` (= ~2147 seconds or ~36
> minutes) at least.

Yeah, looks good. Added.

>> +///
>> +/// This function can only be used in a nonatomic context.
>> +pub fn fsleep(delta: Delta) {
>> +    // The argument of fsleep is an unsigned long, 32-bit on 32-bit architectures.
>> +    // Considering that fsleep rounds up the duration to the nearest millisecond,
>> +    // set the maximum value to u32::MAX / 2 microseconds.
>> +    const MAX_DURATION: Delta = Delta::from_micros(u32::MAX as i64 >> 1);
>> +
>> +    let duration = if delta > MAX_DURATION || delta.as_nanos() < 0 {
> 
> I think it would be helpful if `Delta` has a `is_negative()` function.

Added.

Thanks a lot!

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

[FUJITA Tomonori]

On Wed, 6 Nov 2024 10:18:03 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Fri, Nov 01, 2024 at 10:01:20AM +0900, FUJITA Tomonori wrote:
> [...]
>> @@ -44,6 +45,7 @@
>>  pub mod page;
>>  pub mod prelude;
>>  pub mod print;
>> +pub mod processor;
>>  pub mod sizes;
>>  pub mod rbtree;
>>  mod static_assert;
>> diff --git a/rust/kernel/processor.rs b/rust/kernel/processor.rs
>> new file mode 100644
>> index 000000000000..eeeff4be84fa
>> --- /dev/null
>> +++ b/rust/kernel/processor.rs
> 
> What else would we put into this file? `smp_processor_id()` and IPI
> functionality?

Yeah, we would need smp_processor_id() but not sure about the other
functions. There aren't many processor-related functions that Rust
drivers directly need to call, I guess.

> If so, I would probably want to rename this to cpu.rs.

Fine by me, I'll go with cpu.rs in the next version.

I chose processor.rs just because the C side uses processor.h for
cpu_relax() but cpu.rs also looks good.

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

[FUJITA Tomonori]

On Wed, 6 Nov 2024 13:35:09 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Fri, Nov 01, 2024 at 10:01:20AM +0900, FUJITA Tomonori wrote:
>> Add read_poll_timeout functions which poll periodically until a
>> condition is met or a timeout is reached.
>> 
>> C's read_poll_timeout (include/linux/iopoll.h) is a complicated macro
>> and a simple wrapper for Rust doesn't work. So this implements the
>> same functionality in Rust.
>> 
>> The C version uses usleep_range() while the Rust version uses
>> fsleep(), which uses the best sleep method so it works with spans that
>> usleep_range() doesn't work nicely with.
>> 
>> Unlike the C version, __might_sleep() is used instead of might_sleep()
>> to show proper debug info; the file name and line
>> number. might_resched() could be added to match what the C version
>> does but this function works without it.
>> 
>> The sleep_before_read argument isn't supported since there is no user
>> for now. It's rarely used in the C version.
>> 
>> For the proper debug info, readx_poll_timeout() and __might_sleep()
>> are implemented as a macro. We could implement them as a normal
>> function if there is a clean way to get a null-terminated string
>> without allocation from core::panic::Location::file().
>> 
> 
> So printk() actually support printing a string with a precison value,
> that is: a format string "%.*s" would take two inputs, one for the length
> and the other for the pointer to the string, for example you can do:
> 
> 	char *msg = "hello";
> 
> 	printk("%.*s\n", 5, msg);
> 
> This is similar to printf() in glibc [1].
> 
> If we add another __might_sleep_precision() which accepts a file name
> length:
> 
> 	void __might_sleep_precision(const char *file, int len, int line)
> 
> then we don't need to use macro here, I've attached a diff based
> on your whole patchset, and it seems working.

Ah, I didn't know this.

> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index be2e8c0a187e..b405b0d19bac 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -87,6 +87,8 @@ 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_resched_precision(const char *file, int len, int line, unsigned int offsets);
> +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);
>  
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 43e453ab7e20..f872aa18eaf0 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -8543,7 +8543,7 @@ void __init sched_init(void)
>  
>  #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
>  
> -void __might_sleep(const char *file, int line)
> +void __might_sleep_precision(const char *file, int len, int line)
>  {
>  	unsigned int state = get_current_state();
>  	/*
> @@ -8557,7 +8557,14 @@ void __might_sleep(const char *file, int line)
>  			(void *)current->task_state_change,
>  			(void *)current->task_state_change);
>  
> -	__might_resched(file, line, 0);
> +	__might_resched_precision(file, len, line, 0);
> +}
> +
> +void __might_sleep(const char *file, int line)
> +{
> +	long len = strlen(file);
> +
> +	__might_sleep_precision(file, len, line);
>  }
>  EXPORT_SYMBOL(__might_sleep);
>  
> @@ -8582,7 +8589,7 @@ static inline bool resched_offsets_ok(unsigned int offsets)
>  	return nested == offsets;
>  }
>  
> -void __might_resched(const char *file, int line, unsigned int offsets)
> +void __might_resched_precision(const char *file, int len, int line, unsigned int offsets)
>  {
>  	/* Ratelimiting timestamp: */
>  	static unsigned long prev_jiffy;
> @@ -8605,8 +8612,8 @@ void __might_resched(const char *file, int line, unsigned int offsets)
>  	/* Save this before calling printk(), since that will clobber it: */
>  	preempt_disable_ip = get_preempt_disable_ip(current);
>  
> -	pr_err("BUG: sleeping function called from invalid context at %s:%d\n",
> -	       file, line);
> +	pr_err("BUG: sleeping function called from invalid context at %.*s:%d\n",
> +	       len, file, line);
>  	pr_err("in_atomic(): %d, irqs_disabled(): %d, non_block: %d, pid: %d, name: %s\n",
>  	       in_atomic(), irqs_disabled(), current->non_block_count,
>  	       current->pid, current->comm);
> @@ -8631,6 +8638,13 @@ void __might_resched(const char *file, int line, unsigned int offsets)
>  	dump_stack();
>  	add_taint(TAINT_WARN, LOCKDEP_STILL_OK);
>  }
> +
> +void __might_resched(const char *file, int line, unsigned int offsets)
> +{
> +	long len = strlen(file);
> +
> +	__might_resched_precision(file, len, line, offsets);
> +}
>  EXPORT_SYMBOL(__might_resched);
>  
>  void __cant_sleep(const char *file, int line, int preempt_offset)

Cc scheduler people to ask if they would accept the above change for
Rust or prefer that Rust itself handles the null-terminated string
issue.