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

[PATCH v10 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

v10:
- 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 TIMEKEEPING and TIMER abstractions
  rust: Add read_poll_timeout functions
  net: phy: qt2025: Wait until PHY becomes ready

 MAINTAINERS               |   2 +
 drivers/net/phy/qt2025.rs |  10 ++-
 include/linux/kernel.h    |   2 +
 kernel/sched/core.c       |  55 ++++++++------
 rust/helpers/helpers.c    |   2 +
 rust/helpers/kernel.c     |  13 ++++
 rust/helpers/time.c       |   8 ++
 rust/kernel/cpu.rs        |  13 ++++
 rust/kernel/error.rs      |   1 +
 rust/kernel/io.rs         |   2 +
 rust/kernel/io/poll.rs    |  78 +++++++++++++++++++
 rust/kernel/lib.rs        |   1 +
 rust/kernel/time.rs       | 155 ++++++++++++++++++++++++++++++--------
 rust/kernel/time/delay.rs |  49 ++++++++++++
 14 files changed, 337 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 v10 1/8] sched/core: Add __might_sleep_precision()

[FUJITA Tomonori]

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

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

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

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

Link: https://github.com/rust-lang/libs-team/issues/466 [1]
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    | 55 ++++++++++++++++++++++++++----------------
 2 files changed, 36 insertions(+), 21 deletions(-)

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

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

[David Laight]

On Fri,  7 Feb 2025 22:26:16 +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. strlen() isn't called in a normal case;
> it's called only when printing the error (sleeping function called
> from invalid context).
> 
> Note that Location::file() providing a null-terminated string for
> better C interoperability is under discussion [1].
> 
> Link: https://github.com/rust-lang/libs-team/issues/466 [1]
> 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    | 55 ++++++++++++++++++++++++++----------------
>  2 files changed, 36 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index be2e8c0a187e..086ee1dc447e 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -87,6 +87,7 @@ extern int dynamic_might_resched(void);
>  #ifdef CONFIG_DEBUG_ATOMIC_SLEEP
>  extern void __might_resched(const char *file, int line, unsigned int offsets);
>  extern void __might_sleep(const char *file, int line);
> +extern void __might_sleep_precision(const char *file, int len, int line);
>  extern void __cant_sleep(const char *file, int line, int preempt_offset);
>  extern void __cant_migrate(const char *file, int line);
>  
> @@ -145,6 +146,7 @@ extern void __cant_migrate(const char *file, int line);
>    static inline void __might_resched(const char *file, int line,
>  				     unsigned int offsets) { }
>  static inline void __might_sleep(const char *file, int line) { }
> +static inline void __might_sleep_precision(const char *file, int len, int line) { }
>  # define might_sleep() do { might_resched(); } while (0)
>  # define cant_sleep() do { } while (0)
>  # define cant_migrate()		do { } while (0)
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 165c90ba64ea..d308f2a8692e 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 len, int line,

For clarity that ought to be file_len.

> +				      unsigned int offsets)
>  {
>  	/* Ratelimiting timestamp: */
>  	static unsigned long prev_jiffy;
> @@ -8740,8 +8723,10 @@ void __might_resched(const char *file, int line, unsigned int offsets)
>  	/* Save this before calling printk(), since that will clobber it: */
>  	preempt_disable_ip = get_preempt_disable_ip(current);
>  
> -	pr_err("BUG: sleeping function called from invalid context at %s:%d\n",
> -	       file, line);
> +	if (len < 0)
> +		len = strlen(file);

No need for strlen(), just use a big number instead of -1.
Anything bigger than a sane upper limit on the filename length will do.

	David

> +	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);
> @@ -8766,8 +8751,36 @@ void __might_resched(const char *file, int line, unsigned int offsets)
>  	dump_stack();
>  	add_taint(TAINT_WARN, LOCKDEP_STILL_OK);
>  }
> +
> +void __might_resched(const char *file, int line, unsigned int offsets)
> +{
> +	__might_resched_precision(file, -1, line, offsets);
> +}
>  EXPORT_SYMBOL(__might_resched);
>  
> +void __might_sleep_precision(const char *file, int len, int line)
> +{
> +	unsigned int state = get_current_state();
> +	/*
> +	 * Blocking primitives will set (and therefore destroy) current->state,
> +	 * since we will exit with TASK_RUNNING make sure we enter with it,
> +	 * otherwise we will destroy state.
> +	 */
> +	WARN_ONCE(state != TASK_RUNNING && current->task_state_change,
> +			"do not call blocking ops when !TASK_RUNNING; "
> +			"state=%x set at [<%p>] %pS\n", state,
> +			(void *)current->task_state_change,
> +			(void *)current->task_state_change);
> +
> +	__might_resched_precision(file, len, line, 0);
> +}
> +
> +void __might_sleep(const char *file, int line)
> +{
> +	__might_sleep_precision(file, -1, line);
> +}
> +EXPORT_SYMBOL(__might_sleep);
> +
>  void __cant_sleep(const char *file, int line, int preempt_offset)
>  {
>  	static unsigned long prev_jiffy;

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

[FUJITA Tomonori]

On Fri, 7 Feb 2025 18:12:58 +0000
David Laight <david.laight.linux@gmail.com> wrote:

>>  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 len, int line,
> 
> For clarity that ought to be file_len.

Yeah, I'll update.

>> +				      unsigned int offsets)
>>  {
>>  	/* Ratelimiting timestamp: */
>>  	static unsigned long prev_jiffy;
>> @@ -8740,8 +8723,10 @@ void __might_resched(const char *file, int line, unsigned int offsets)
>>  	/* Save this before calling printk(), since that will clobber it: */
>>  	preempt_disable_ip = get_preempt_disable_ip(current);
>>  
>> -	pr_err("BUG: sleeping function called from invalid context at %s:%d\n",
>> -	       file, line);
>> +	if (len < 0)
>> +		len = strlen(file);
> 
> No need for strlen(), just use a big number instead of -1.
> Anything bigger than a sane upper limit on the filename length will do.

Ah, that's right. Just passing the maximum precision (1<<15-1) works.

The precision specifies the maximum length. vsnprintf() always
iterates through a string until it reaches the maximum length or
encounters the null terminator. So strlen() here is useless.

Alice and Boqun, the above change is fine? Can I keep the tags?

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

[Alice Ryhl]

On Sat, Feb 8, 2025 at 4:01 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Fri, 7 Feb 2025 18:12:58 +0000
> David Laight <david.laight.linux@gmail.com> wrote:
>
> >>  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 len, int line,
> >
> > For clarity that ought to be file_len.
>
> Yeah, I'll update.
>
> >> +                                  unsigned int offsets)
> >>  {
> >>      /* Ratelimiting timestamp: */
> >>      static unsigned long prev_jiffy;
> >> @@ -8740,8 +8723,10 @@ void __might_resched(const char *file, int line, unsigned int offsets)
> >>      /* Save this before calling printk(), since that will clobber it: */
> >>      preempt_disable_ip = get_preempt_disable_ip(current);
> >>
> >> -    pr_err("BUG: sleeping function called from invalid context at %s:%d\n",
> >> -           file, line);
> >> +    if (len < 0)
> >> +            len = strlen(file);
> >
> > No need for strlen(), just use a big number instead of -1.
> > Anything bigger than a sane upper limit on the filename length will do.
>
> Ah, that's right. Just passing the maximum precision (1<<15-1) works.
>
> The precision specifies the maximum length. vsnprintf() always
> iterates through a string until it reaches the maximum length or
> encounters the null terminator. So strlen() here is useless.
>
> Alice and Boqun, the above change is fine? Can I keep the tags?

I'd probably like a comment explaining the meaning of this constant
somewhere, but sure ok with me.

Alice

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

[Boqun Feng]

On Mon, Feb 10, 2025 at 10:41:00AM +0100, Alice Ryhl wrote:
> On Sat, Feb 8, 2025 at 4:01 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
> >
> > On Fri, 7 Feb 2025 18:12:58 +0000
> > David Laight <david.laight.linux@gmail.com> wrote:
> >
> > >>  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 len, int line,
> > >
> > > For clarity that ought to be file_len.
> >
> > Yeah, I'll update.
> >
> > >> +                                  unsigned int offsets)
> > >>  {
> > >>      /* Ratelimiting timestamp: */
> > >>      static unsigned long prev_jiffy;
> > >> @@ -8740,8 +8723,10 @@ void __might_resched(const char *file, int line, unsigned int offsets)
> > >>      /* Save this before calling printk(), since that will clobber it: */
> > >>      preempt_disable_ip = get_preempt_disable_ip(current);
> > >>
> > >> -    pr_err("BUG: sleeping function called from invalid context at %s:%d\n",
> > >> -           file, line);
> > >> +    if (len < 0)
> > >> +            len = strlen(file);
> > >
> > > No need for strlen(), just use a big number instead of -1.
> > > Anything bigger than a sane upper limit on the filename length will do.
> >
> > Ah, that's right. Just passing the maximum precision (1<<15-1) works.
> >
> > The precision specifies the maximum length. vsnprintf() always
> > iterates through a string until it reaches the maximum length or
> > encounters the null terminator. So strlen() here is useless.
> >
> > Alice and Boqun, the above change is fine? Can I keep the tags?
> 
> I'd probably like a comment explaining the meaning of this constant
> somewhere, but sure ok with me.
> 

Agreed. The code should be fine but need some comments.

Regards,
Boqun

> Alice

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

[FUJITA Tomonori]

On Sun, 16 Feb 2025 17:51:32 -0800
Boqun Feng <boqun.feng@gmail.com> wrote:

>> > >> +                                  unsigned int offsets)
>> > >>  {
>> > >>      /* Ratelimiting timestamp: */
>> > >>      static unsigned long prev_jiffy;
>> > >> @@ -8740,8 +8723,10 @@ void __might_resched(const char *file, int line, unsigned int offsets)
>> > >>      /* Save this before calling printk(), since that will clobber it: */
>> > >>      preempt_disable_ip = get_preempt_disable_ip(current);
>> > >>
>> > >> -    pr_err("BUG: sleeping function called from invalid context at %s:%d\n",
>> > >> -           file, line);
>> > >> +    if (len < 0)
>> > >> +            len = strlen(file);
>> > >
>> > > No need for strlen(), just use a big number instead of -1.
>> > > Anything bigger than a sane upper limit on the filename length will do.
>> >
>> > Ah, that's right. Just passing the maximum precision (1<<15-1) works.
>> >
>> > The precision specifies the maximum length. vsnprintf() always
>> > iterates through a string until it reaches the maximum length or
>> > encounters the null terminator. So strlen() here is useless.
>> >
>> > Alice and Boqun, the above change is fine? Can I keep the tags?
>> 
>> I'd probably like a comment explaining the meaning of this constant
>> somewhere, but sure ok with me.
>> 
> 
> Agreed. The code should be fine but need some comments.

Of course, I'll add some in the next version.

Thanks!

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

[FUJITA Tomonori]

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

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

Reviewed-by: Trevor Gross <tmgross@umich.edu>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
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

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

[FUJITA Tomonori]

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

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

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

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

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

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
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

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

[FUJITA Tomonori]

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

Rename Ktime to Instant and modify their methods for timestamp.

Implement the subtraction operator for Instant:

Delta = Instant A - Instant B

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
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

[PATCH v10 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]
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

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

[FUJITA Tomonori]

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

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

diff --git a/MAINTAINERS b/MAINTAINERS
index c8d9e8187eb0..987a25550853 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10353,6 +10353,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
@@ -23852,6 +23853,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

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

[FUJITA Tomonori]

On Fri,  7 Feb 2025 22:26:21 +0900
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> 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 c8d9e8187eb0..987a25550853 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10353,6 +10353,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
> @@ -23852,6 +23853,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/

TIMERS and TIMEKEEPING maintainers,

You would prefer to add rust files to a separate entry for Rust? Or
you prefer a different option?

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

[Frederic Weisbecker]

Le Mon, Feb 17, 2025 at 09:10:08AM +0900, FUJITA Tomonori a écrit :
> On Fri,  7 Feb 2025 22:26:21 +0900
> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> 
> > 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 c8d9e8187eb0..987a25550853 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -10353,6 +10353,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
> > @@ -23852,6 +23853,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/
> 
> TIMERS and TIMEKEEPING maintainers,
> 
> You would prefer to add rust files to a separate entry for Rust? Or
> you prefer a different option?

It's probably a better idea to keep those rust entries to their own sections.
This code will be better handled into your more capable hands.

Thanks.

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

[FUJITA Tomonori]

On Mon, 17 Feb 2025 14:39:44 +0100
Frederic Weisbecker <frederic@kernel.org> wrote:

>> > diff --git a/MAINTAINERS b/MAINTAINERS
>> > index c8d9e8187eb0..987a25550853 100644
>> > --- a/MAINTAINERS
>> > +++ b/MAINTAINERS
>> > @@ -10353,6 +10353,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
>> > @@ -23852,6 +23853,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/
>> 
>> TIMERS and TIMEKEEPING maintainers,
>> 
>> You would prefer to add rust files to a separate entry for Rust? Or
>> you prefer a different option?
> 
> It's probably a better idea to keep those rust entries to their own sections.
> This code will be better handled into your more capable hands.

Got it. I'll create new sections for Rust's TIMERS and TIMEKEEPING.

Thanks!

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

[FUJITA Tomonori]

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

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

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

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

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

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

Link: https://rust-for-linux.com/klint [1]
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers/helpers.c |  1 +
 rust/helpers/kernel.c  | 13 +++++++
 rust/kernel/cpu.rs     | 13 +++++++
 rust/kernel/error.rs   |  1 +
 rust/kernel/io.rs      |  2 ++
 rust/kernel/io/poll.rs | 78 ++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |  1 +
 7 files changed, 109 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..9dff28f4618e
--- /dev/null
+++ b/rust/helpers/kernel.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/kernel.h>
+
+void rust_helper_cpu_relax(void)
+{
+	cpu_relax();
+}
+
+void rust_helper___might_sleep_precision(const char *file, int len, int line)
+{
+	__might_sleep_precision(file, len, line);
+}
diff --git a/rust/kernel/cpu.rs b/rust/kernel/cpu.rs
new file mode 100644
index 000000000000..eeeff4be84fa
--- /dev/null
+++ b/rust/kernel/cpu.rs
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Processor related primitives.
+//!
+//! C header: [`include/linux/processor.h`](srctree/include/linux/processor.h).
+
+/// Lower CPU power consumption or yield to a hyperthreaded twin processor.
+///
+/// It also happens to serve as a compiler barrier.
+pub fn cpu_relax() {
+    // SAFETY: FFI call.
+    unsafe { bindings::cpu_relax() }
+}
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index f6ecf09cb65f..8858eb13b3df 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -64,6 +64,7 @@ macro_rules! declare_err {
     declare_err!(EPIPE, "Broken pipe.");
     declare_err!(EDOM, "Math argument out of domain of func.");
     declare_err!(ERANGE, "Math result not representable.");
+    declare_err!(ETIMEDOUT, "Connection timed out.");
     declare_err!(ERESTARTSYS, "Restart the system call.");
     declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
     declare_err!(ERESTARTNOHAND, "Restart if no handler.");
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
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..bed5b693402e
--- /dev/null
+++ b/rust/kernel/io/poll.rs
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! IO polling.
+//!
+//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
+
+use crate::{
+    cpu::cpu_relax,
+    error::{code::*, Result},
+    time::{delay::fsleep, Delta, Instant},
+};
+
+use core::panic::Location;
+
+/// Polls periodically until a condition is met or a timeout is reached.
+///
+/// ```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(Location::caller());
+    }
+
+    loop {
+        let val = op()?;
+        if cond(&val) {
+            // Unlike the C version, we immediately return.
+            // We know the condition is met so we don't need to check again.
+            return Ok(val);
+        }
+        if let Some(timeout_delta) = timeout_delta {
+            if start.elapsed() > timeout_delta {
+                // Unlike the C version, we immediately return.
+                // We have just called `op()` so we don't need to call it again.
+                return Err(ETIMEDOUT);
+            }
+        }
+        if sleep {
+            fsleep(sleep_delta);
+        }
+        // fsleep() could be busy-wait loop so we always call cpu_relax().
+        cpu_relax();
+    }
+}
+
+fn might_sleep(loc: &Location<'_>) {
+    // SAFETY: FFI call.
+    unsafe {
+        crate::bindings::__might_sleep_precision(
+            loc.file().as_ptr().cast(),
+            loc.file().len() as i32,
+            loc.line() as i32,
+        )
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 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 v10 7/8] rust: Add read_poll_timeout functions

[Daniel Almeida]

Hi,

> +
> +/// Polls periodically until a condition is met or a timeout is reached.
> +///
> +/// ```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>(())

IMHO, the example section here needs to be improved.

> +/// ```
> +#[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(Location::caller());
> +    }
> +
> +    loop {
> +        let val = op()?;

I.e.: it’s not clear that `op` computes `val` until you read this.

> +        if cond(&val) {

It’s a bit unclear how `cond` works, until you see this line here.

It’s even more important to explain this a tad better, since it differs slightly from the C API.

Also, it doesn’t help that `Op` and `Cond` take and return the unit type in the
only example provided.

— Daniel

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

[FUJITA Tomonori]

On Fri, 7 Feb 2025 22:50:37 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

>> +/// Polls periodically until a condition is met or a timeout is reached.
>> +///
>> +/// ```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>(())
> 
> IMHO, the example section here needs to be improved.

Do you have any specific ideas?

Generally, this function is used to wait for the hardware to be
ready. So I can't think of a nice example.


>> +/// ```
>> +#[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(Location::caller());
>> +    }
>> +
>> +    loop {
>> +        let val = op()?;
> 
> I.e.: it’s not clear that `op` computes `val` until you read this.
> 
>> +        if cond(&val) {
> 
> It’s a bit unclear how `cond` works, until you see this line here.
> 
> It’s even more important to explain this a tad better, since it differs slightly from the C API.

ok, I'll try to add some docs in v11.

> Also, it doesn’t help that `Op` and `Cond` take and return the unit type in the
> only example provided.
> 
> ― Daniel
> 
> 

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

[Daniel Almeida]

Sorry, ended up replying privately by mistake, resending with everybody else on cc:

——————


> On 14 Feb 2025, at 01:13, FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> 
> On Fri, 7 Feb 2025 22:50:37 -0300
> Daniel Almeida <daniel.almeida@collabora.com> wrote:
> 
>>> +/// Polls periodically until a condition is met or a timeout is reached.
>>> +///
>>> +/// ```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>(())
>> 
>> IMHO, the example section here needs to be improved.
> 
> Do you have any specific ideas?
> 
> Generally, this function is used to wait for the hardware to be
> ready. So I can't think of a nice example.

Just pretend that you’re polling some mmio address that indicates whether some hardware
block is ready, for example.

You can use “ignore” if you want, the example just has to illustrate how this function works, really.

Something like

```ignore
 /* R is a fictional type that abstracts a memory-mapped register where `read()` returns Result<u32> */
 fn wait_for_hardware(ready_register: R) {
     let op = || ready_register.read()?

     // `READY` is some device-specific constant that we are waiting for.
     let cond =  |value: &u32| { *value == READY }

     let res = io::poll::read_poll_timeout (/* fill this with the right arguments */);

     /* show how `res` works, is -ETIMEDOUT returned on Err? */

     match res {
       Ok(<what is here?>) => { /* hardware is ready */}
       Err(e) => { /* explain that *value != READY here? */ }

     /* sleep is Option<Delta>, how does this work? i.e.: show both None, and Some(…) with some comments. */
 } 
```

That’s just a rough draft, but I think it's going to be helpful for users.

— Daniel

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

[FUJITA Tomonori]

On Sun, 16 Feb 2025 09:19:02 -0300
Daniel Almeida <daniel.almeida@collabora.com> wrote:

>>>> +/// Polls periodically until a condition is met or a timeout is reached.
>>>> +///
>>>> +/// ```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>(())
>>> 
>>> IMHO, the example section here needs to be improved.
>> 
>> Do you have any specific ideas?
>> 
>> Generally, this function is used to wait for the hardware to be
>> ready. So I can't think of a nice example.
> 
> Just pretend that you’re polling some mmio address that indicates whether some hardware
> block is ready, for example.
> 
> You can use “ignore” if you want, the example just has to illustrate how this function works, really.

Ah, you use `ignore` comment. I was only considering examples that
would actually be tested.


> Something like
> 
> ```ignore
>  /* R is a fictional type that abstracts a memory-mapped register where `read()` returns Result<u32> */
>  fn wait_for_hardware(ready_register: R) {
>      let op = || ready_register.read()?
> 
>      // `READY` is some device-specific constant that we are waiting for.
>      let cond =  |value: &u32| { *value == READY }
> 
>      let res = io::poll::read_poll_timeout (/* fill this with the right arguments */);
> 
>      /* show how `res` works, is -ETIMEDOUT returned on Err? */
> 
>      match res {
>        Ok(<what is here?>) => { /* hardware is ready */}
>        Err(e) => { /* explain that *value != READY here? */ }
> 
>      /* sleep is Option<Delta>, how does this work? i.e.: show both None, and Some(…) with some comments. */
>  } 
> ```
> 
> That’s just a rough draft, but I think it's going to be helpful for users.

I'll add an example based on the code QT2025 PHY (8th patch). It's
similar to the above.

Thanks,

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

[Gary Guo]

On Fri,  7 Feb 2025 22:26:22 +0900
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

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

One last Q: why isn't `might_sleep` marked as `track_caller` and then
have `Location::caller` be called internally?

It would make the API same as the C macro.

Also -- perhaps this function can be public (though I guess you'd need
to put it in a new module).

Best,
Gary

> 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 v10 7/8] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

On Sun, 9 Feb 2025 16:20:48 +0000
Gary Guo <gary@garyguo.net> wrote:

>> +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,
>> +        )
>> +    }
>> +}
> 
> One last Q: why isn't `might_sleep` marked as `track_caller` and then
> have `Location::caller` be called internally?
>
> It would make the API same as the C macro.

Equivalent to the C side __might_sleep(), not might_sleep(). To avoid
confusion, it might be better to change the name of this function.

The reason why __might_sleep() is used instead of might_sleep() is
might_sleep() can't always be called. It was discussed in v2:

https://lore.kernel.org/all/ZwPT7HZvG1aYONkQ@boqun-archlinux/

> Also -- perhaps this function can be public (though I guess you'd need
> to put it in a new module).

Wouldn't it be better to keep it private until actual users appear?

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

[Gary Guo]

On Fri, 14 Feb 2025 13:05:30 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> On Sun, 9 Feb 2025 16:20:48 +0000
> Gary Guo <gary@garyguo.net> wrote:
> 
> >> +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,
> >> +        )
> >> +    }
> >> +}  
> > 
> > One last Q: why isn't `might_sleep` marked as `track_caller` and then
> > have `Location::caller` be called internally?
> >
> > It would make the API same as the C macro.  
> 
> Equivalent to the C side __might_sleep(), not might_sleep(). To avoid
> confusion, it might be better to change the name of this function.
> 
> The reason why __might_sleep() is used instead of might_sleep() is
> might_sleep() can't always be called. It was discussed in v2:
> 
> https://lore.kernel.org/all/ZwPT7HZvG1aYONkQ@boqun-archlinux/

I don't follow. `__might_sleep` or `might_sleep` wouldn't make a
difference here, given that this function may actually sleep.

- Gary

> 
> > Also -- perhaps this function can be public (though I guess you'd need
> > to put it in a new module).  

> 
> Wouldn't it be better to keep it private until actual users appear?

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

[FUJITA Tomonori]

On Fri, 14 Feb 2025 11:37:40 +0000
Gary Guo <gary@garyguo.net> wrote:

> On Fri, 14 Feb 2025 13:05:30 +0900 (JST)
> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> 
>> On Sun, 9 Feb 2025 16:20:48 +0000
>> Gary Guo <gary@garyguo.net> wrote:
>> 
>> >> +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,
>> >> +        )
>> >> +    }
>> >> +}  
>> > 
>> > One last Q: why isn't `might_sleep` marked as `track_caller` and then
>> > have `Location::caller` be called internally?
>> >
>> > It would make the API same as the C macro.  
>> 
>> Equivalent to the C side __might_sleep(), not might_sleep(). To avoid
>> confusion, it might be better to change the name of this function.
>> 
>> The reason why __might_sleep() is used instead of might_sleep() is
>> might_sleep() can't always be called. It was discussed in v2:
>> 
>> https://lore.kernel.org/all/ZwPT7HZvG1aYONkQ@boqun-archlinux/
> 
> I don't follow. `__might_sleep` or `might_sleep` wouldn't make a
> difference here, given that this function may actually sleep.

Yeah, it doesn't matter here. If I understand correctly, the
discussion is about whether might_sleep() itself should be unsafe
considering the case where it is called from other functions. I simply
chose uncontroversial __might_sleep().

After reviewing the code again, I realized that I made a mistake;
__might_sleep() should only be executed when CONFIG_DEBUG_ATOMIC_SLEEP
is enabled. I also think that it is confusing that might_sleep() calls
C's __might_sleep().

How about implementing the equivalent to might_sleep()?

/// 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() }
}


>> > Also -- perhaps this function can be public (though I guess you'd need
>> > to put it in a new module).  
>> 
>> Wouldn't it be better to keep it private until actual users appear?

I'll make the above public if you think that is the better approach.

C's might_sleep() is defined in linux/kernel.h but kernel/kernel.rs
isn't a good choice, I guess. kernel/sched.rs or other options?

[PATCH v10 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 v10 0/8] rust: Add IO polling

[Daniel Almeida]

Hi Fujita,

> On 7 Feb 2025, at 10:26, FUJITA Tomonori <fujita.tomonori@gmail.com> 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.

I tested this on a driver I’ve been working on. This is working as intended.

Tested-by: Daniel Almeida <daniel.almeida@collabora.com>

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

[FUJITA Tomonori]

On Sat, 15 Feb 2025 18:28:30 -0300
Daniel Almeida <daniel.almeida@collabora.com> 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.
> 
> I tested this on a driver I’ve been working on. This is working as intended.
> 
> Tested-by: Daniel Almeida <daniel.almeida@collabora.com>

Great, thanks for testing!