[PATCH net-next v3 0/8] rust: Add IO polling

[PATCH net-next v3 0/8] rust: Add IO polling

[FUJITA Tomonori]

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

As a result of the past discussion, this introduces a new type
representing a span of time instead of using core::time::Duration or
time::Ktime.

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.

v3:
- 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):
  rust: time: Add PartialEq/Eq/PartialOrd/Ord trait to Ktime
  rust: time: Introduce Delta type
  rust: time: Change output of Ktime's sub operation to Delta
  rust: time: Implement addition of Ktime and Delta
  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    | 84 +++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs        |  1 +
 rust/kernel/time.rs       | 99 ++++++++++++++++++++++++++++++++++++---
 rust/kernel/time/delay.rs | 31 ++++++++++++
 11 files changed, 249 insertions(+), 7 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/time/delay.rs


base-commit: 068f3b34c5c2be5fe7923a9966c1c16f992a2f9c
-- 
2.43.0

[PATCH net-next v3 1/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 more efficient and we already do in
the existing code (Ktime::sub).

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

Re: [PATCH net-next v3 1/8] rust: time: Add PartialEq/Eq/PartialOrd/Ord trait to Ktime

[Alice Ryhl]

On Wed, Oct 16, 2024 at 5:53 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Add PartialEq/Eq/PartialOrd/Ord trait to Ktime so two Ktime instances
> can be compared to determine whether a timeout is met or not.
>
> Use the derive implements; we directly touch C's ktime_t rather than
> using the C's accessors because more efficient and we already do in
> the existing code (Ktime::sub).
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

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

[PATCH net-next v3 2/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.

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

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time.rs | 74 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 4a7c6037c256..38a70dc98083 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,71 @@ fn sub(self, other: Ktime) -> Ktime {
         }
     }
 }
+
+/// A span of time.
+#[derive(Copy, Clone)]
+pub struct Delta {
+    nanos: i64,
+}
+
+impl Delta {
+    /// Create a new `Delta` from a number of nanoseconds.
+    #[inline]
+    pub fn from_nanos(nanos: i64) -> Self {
+        Self { nanos }
+    }
+
+    /// Create a new `Delta` from a number of microseconds.
+    #[inline]
+    pub 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 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 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.nanos == 0
+    }
+
+    /// Return the number of nanoseconds in the `Delta`.
+    #[inline]
+    pub fn as_nanos(self) -> i64 {
+        self.nanos
+    }
+
+    /// Return the number of microseconds in the `Delta`.
+    #[inline]
+    pub fn as_micros(self) -> i64 {
+        self.nanos / NSEC_PER_USEC
+    }
+
+    /// Return the number of milliseconds in the `Delta`.
+    #[inline]
+    pub fn as_millis(self) -> i64 {
+        self.nanos / NSEC_PER_MSEC
+    }
+
+    /// Return the number of seconds in the `Delta`.
+    #[inline]
+    pub fn as_secs(self) -> i64 {
+        self.nanos / NSEC_PER_SEC
+    }
+}
-- 
2.43.0

Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

[Alice Ryhl]

On Wed, Oct 16, 2024 at 5:53 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Introduce a type representing a span of time. Define our own type
> because `core::time::Duration` is large and could panic during
> creation.
>
> time::Ktime could be also used for time duration but timestamp and
> timedelta are different so better to use a new type.
>
> i64 is used instead of u64 to represent a span of time; some C drivers
> uses negative Deltas and i64 is more compatible with Ktime using i64
> too (e.g., ktime_[us|ms]_delta() APIs return i64 so we create Delta
> object without type conversion.

Is there a reason that Delta wraps i64 directly instead of wrapping
the Ktime type? I'd like to see the reason in the commit message.

Alice

Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

[FUJITA Tomonori]

On Wed, 16 Oct 2024 10:23:49 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

> On Wed, Oct 16, 2024 at 5:53 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> Introduce a type representing a span of time. Define our own type
>> because `core::time::Duration` is large and could panic during
>> creation.
>>
>> time::Ktime could be also used for time duration but timestamp and
>> timedelta are different so better to use a new type.
>>
>> i64 is used instead of u64 to represent a span of time; some C drivers
>> uses negative Deltas and i64 is more compatible with Ktime using i64
>> too (e.g., ktime_[us|ms]_delta() APIs return i64 so we create Delta
>> object without type conversion.
> 
> Is there a reason that Delta wraps i64 directly instead of wrapping
> the Ktime type? I'd like to see the reason in the commit message.

You meant that bindings::ktime_t is used instead of i64 like:

pub struct Delta {
    nanos: bindings::ktime_t,
}

? I've not considered but might make sense.

Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

[Fiona Behrens]

On 16 Oct 2024, at 5:52, FUJITA Tomonori wrote:

> Introduce a type representing a span of time. Define our own type
> because `core::time::Duration` is large and could panic during
> creation.
>
> time::Ktime could be also used for time duration but timestamp and
> timedelta are different so better to use a new type.
>
> i64 is used instead of u64 to represent a span of time; some C drivers
> uses negative Deltas and i64 is more compatible with Ktime using i64
> too (e.g., ktime_[us|ms]_delta() APIs return i64 so we create Delta
> object without type conversion.
>
> Delta::from_[micro|millis|secs] APIs take i64. When a span of time
> overflows, i64::MAX is used.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/time.rs | 74 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 4a7c6037c256..38a70dc98083 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,71 @@ fn sub(self, other: Ktime) -> Ktime {
>          }
>      }
>  }
> +
> +/// A span of time.
> +#[derive(Copy, Clone)]

Could we also derive PartialEq and Eq (maybe also PartialOrd and Ord)? Would need that to compare deltas in my LED driver.

> +pub struct Delta {
> +    nanos: i64,
> +}
> +

I think all this functions could be const (need from_millis as const for LED, but when at it we could probably make all those const?)

 - Fiona

> +impl Delta {
> +    /// Create a new `Delta` from a number of nanoseconds.
> +    #[inline]
> +    pub fn from_nanos(nanos: i64) -> Self {
> +        Self { nanos }
> +    }
> +
> +    /// Create a new `Delta` from a number of microseconds.
> +    #[inline]
> +    pub 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 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 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.nanos == 0
> +    }
> +
> +    /// Return the number of nanoseconds in the `Delta`.
> +    #[inline]
> +    pub fn as_nanos(self) -> i64 {
> +        self.nanos
> +    }
> +
> +    /// Return the number of microseconds in the `Delta`.
> +    #[inline]
> +    pub fn as_micros(self) -> i64 {
> +        self.nanos / NSEC_PER_USEC
> +    }
> +
> +    /// Return the number of milliseconds in the `Delta`.
> +    #[inline]
> +    pub fn as_millis(self) -> i64 {
> +        self.nanos / NSEC_PER_MSEC
> +    }
> +
> +    /// Return the number of seconds in the `Delta`.
> +    #[inline]
> +    pub fn as_secs(self) -> i64 {
> +        self.nanos / NSEC_PER_SEC
> +    }
> +}
> -- 
> 2.43.0

Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

[FUJITA Tomonori]

On Thu, 17 Oct 2024 12:17:18 +0200
Fiona Behrens <me@kloenk.dev> wrote:

>> +/// A span of time.
>> +#[derive(Copy, Clone)]
>
> Could we also derive PartialEq and Eq (maybe also PartialOrd and
> Ord)? Would need that to compare deltas in my LED driver.

Sure, I'll add.

>> +pub struct Delta {
>> +    nanos: i64,
>> +}
>> +
>
> I think all this functions could be const (need from_millis as const
> for LED, but when at it we could probably make all those const?)

I think that making all the from_* methods const fn make sense. I'll
do.

Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

[Andrew Lunn]

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

Wasn't there a comment that the code should always round up? Delaying
for 0 uS is probably not what the user wants.

    Andrew

Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

[Miguel Ojeda]

On Fri, Oct 18, 2024 at 3:50 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Wasn't there a comment that the code should always round up? Delaying
> for 0 uS is probably not what the user wants.

This is not delaying, though, so I don't see why a method with this
name should return a rounded up value.

Moreover, `ktime_to_us` doesn't round up. The standard library one
doesn't, either.

Cheers,
Miguel

Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

[Andrew Lunn]

On Fri, Oct 18, 2024 at 04:31:34PM +0200, Miguel Ojeda wrote:
> On Fri, Oct 18, 2024 at 3:50 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > Wasn't there a comment that the code should always round up? Delaying
> > for 0 uS is probably not what the user wants.
> 
> This is not delaying, though, so I don't see why a method with this
> name should return a rounded up value.
> 
> Moreover, `ktime_to_us` doesn't round up. The standard library one
> doesn't, either.

Did you see my other comment, about not actually using these helpers?
I _guess_ it was not used because it does not in fact round up. So at
least for this patchset, the helpers are useless. Should we be adding
useless helpers? Or should we be adding useful helpers? Maybe these
helpers need a different name, and do actually round up?

	Andrew

Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

[Miguel Ojeda]

On Fri, Oct 18, 2024 at 6:55 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Did you see my other comment, about not actually using these helpers?
> I _guess_ it was not used because it does not in fact round up. So at
> least for this patchset, the helpers are useless. Should we be adding
> useless helpers? Or should we be adding useful helpers? Maybe these
> helpers need a different name, and do actually round up?

Yeah, I saw that -- if I understand you correctly, you were asking why
`as_nanos()` is used and not `as_secs()` directly (did you mean
`as_micros()`?) by adding rounding on `Delta`'s `as_*()` methods.

So my point here was that a method with a name like `as_*()` has
nothing to do with rounding, so I wouldn't add rounding here (it would
be misleading).

Now, we can of course have rounding methods in `Delta` for convenience
if that is something commonly needed by `Delta`'s consumers like
`fsleep()`, that is fine, but those would need to be called
differently, e.g. `to_micros_ceil`: `to_` since it is not "free"
(following e.g. `to_radians`) and + `_ceil` to follow `div_ceil` from
the `int_roundings` feature (and shorter than something like
`_rounded_up`).

In other words, I think you see these small `as_*()` functions as
"helpers" to do something else, and thus why you say we should provide
those that we need (only) and make them do what we need (the
rounding). That is fair.

However, another way of viewing these is as typical conversion methods
of `Delta`, i.e. the very basic interface for a type like this. Thus
Tomonori implemented the very basic interface, and since there was no
rounding, then he added it in `fsleep()`.

I agree having rounding methods here could be useful, but I am
ambivalent as to whether following the "no unused code" rule that far
as to remove very basic APIs for a type like this.

Cheers,
Miguel

Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

[Miguel Ojeda]

On Sat, Oct 19, 2024 at 2:21 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> differently, e.g. `to_micros_ceil`: `to_` since it is not "free"

Well, it is sufficiently free, I guess, considering other methods. I
noticed `as_*()` in this patch take `self` rather than `&self`, though.

Cheers,
Miguel

Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

[FUJITA Tomonori]

On Sat, 19 Oct 2024 14:41:07 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Sat, Oct 19, 2024 at 2:21 PM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> differently, e.g. `to_micros_ceil`: `to_` since it is not "free"
> 
> Well, it is sufficiently free, I guess, considering other methods. I
> noticed `as_*()` in this patch take `self` rather than `&self`, though.

Should use &self for as_*() instead?

Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

[Miguel Ojeda]

On Wed, Oct 23, 2024 at 1:53 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Should use &self for as_*() instead?

I don't think there is a hard rule, so taking `self` is fine even if uncommon.

But probably we should discuss eventually if we want more concrete
guidelines here (i.e. more concrete than Rust usual ones).

Thanks!

Cheers,
Miguel

Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

[Andrew Lunn]

On Sat, Oct 19, 2024 at 02:21:45PM +0200, Miguel Ojeda wrote:
> On Fri, Oct 18, 2024 at 6:55 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > Did you see my other comment, about not actually using these helpers?
> > I _guess_ it was not used because it does not in fact round up. So at
> > least for this patchset, the helpers are useless. Should we be adding
> > useless helpers? Or should we be adding useful helpers? Maybe these
> > helpers need a different name, and do actually round up?
> 
> Yeah, I saw that -- if I understand you correctly, you were asking why
> `as_nanos()` is used and not `as_secs()` directly (did you mean
> `as_micros()`?) by adding rounding on `Delta`'s `as_*()` methods.
> 
> So my point here was that a method with a name like `as_*()` has
> nothing to do with rounding, so I wouldn't add rounding here (it would
> be misleading).
> 
> Now, we can of course have rounding methods in `Delta` for convenience
> if that is something commonly needed by `Delta`'s consumers like
> `fsleep()`, that is fine, but those would need to be called
> differently, e.g. `to_micros_ceil`: `to_` since it is not "free"
> (following e.g. `to_radians`) and + `_ceil` to follow `div_ceil` from
> the `int_roundings` feature (and shorter than something like
> `_rounded_up`).
> 
> In other words, I think you see these small `as_*()` functions as
> "helpers" to do something else, and thus why you say we should provide
> those that we need (only) and make them do what we need (the
> rounding). That is fair.
> 
> However, another way of viewing these is as typical conversion methods
> of `Delta`, i.e. the very basic interface for a type like this. Thus
> Tomonori implemented the very basic interface, and since there was no
> rounding, then he added it in `fsleep()`.
> 
> I agree having rounding methods here could be useful, but I am
> ambivalent as to whether following the "no unused code" rule that far
> as to remove very basic APIs for a type like this.

I would say ignoring the rule about an API always having a user has
led to badly designed code, which is actually going to lead to bugs.

It is clearly a philosophical point, what the base of the type is, and
what are helpers. For me, the base is a i64 representing nano seconds,
operations too add, subtract and compare, and a way to get the number
of nanoseconds in and out.

I see being able to create such a type from microseconds, millisecond,
seconds and decades as helpers on top of this base. Also, being able
to extract the number of nanoseconds from the type but expressed in
microseconds, milliseconds, seconds and months are lossy helpers on
top of the base.

So far, we only have one use case for this type, holding a duration to
be passed to fsleep(). Rounding down what you pass to fsleep() is
generally not what the user wants to do, and we should try to design
the code to avoid this. The current helpers actually encourage such
bugs, because they round down. Because of this they are currently
unused. But they are a trap waiting for somebody to fall into. What
the current users of this type really want is lossy helpers which
round up. And by reviewing the one user against the API, it is clear
the current API is wrong.

So i say, throw away the round down helpers until somebody really
needs them. That avoids a class of bugs, passing a too low value to
sleep. Add the one helper which is actually needed right now.

There is potentially a better option. Make the actual sleep operation
part of the type. All the rounding up then becomes part of the core,
and the developer gets core code which just works correctly, with an
API which is hard to make do the wrong thing.

	Andrew

Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

[Miguel Ojeda]

On Sat, Oct 19, 2024 at 8:41 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> So far, we only have one use case for this type, holding a duration to
> be passed to fsleep(). Rounding down what you pass to fsleep() is
> generally not what the user wants to do, and we should try to design
> the code to avoid this. The current helpers actually encourage such
> bugs, because they round down. Because of this they are currently
> unused. But they are a trap waiting for somebody to fall into. What
> the current users of this type really want is lossy helpers which
> round up. And by reviewing the one user against the API, it is clear
> the current API is wrong.

If you are talking about Rust's `fsleep()`, then "users" are not the
ones calling the rounding up operation (the implementation of
`fsleep()` is -- just like in the C side).

If you are talking about C's `fsleep()`, then users are not supposed
to use `bindings::`.

> So i say, throw away the round down helpers until somebody really
> needs them. That avoids a class of bugs, passing a too low value to
> sleep. Add the one helper which is actually needed right now.

Eventually this type will likely get other methods for one reason or
another, including the non-rounding ones. Thus we should be careful
about the names we pick, which is why I was saying a method like
`as_micros()` should not be rounding up. That would be confusing, and
thus potentially end creating bugs, even if it is the only method you
add today.

Again, if you want to throw away all the unused methods and only have
the rounding up one, then that is reasonable, but please let's not add
misleading methods that could add more bugs than the ones you are
trying to avoid. Please use `as_micros_ceil()` or similar.

> There is potentially a better option. Make the actual sleep operation
> part of the type. All the rounding up then becomes part of the core,
> and the developer gets core code which just works correctly, with an
> API which is hard to make do the wrong thing.

That is orthogonal: whether the sleeping function is in `Delta` or
not, users would not be the ones calling the rounding up operation
(please see above).

Anyway, by moving more things into a type, you are increasing its
complexity. And, depending how modules are organized, you could be
also giving visibility into the internals of that type, thus
increasing the possibility of "implementation-side bugs" compared to
the other way around (it does not really matter here, though).

If you want it as a method for user ergonomics though, that is a
different topic.

Cheers,
Miguel

Re: [PATCH net-next v3 2/8] rust: time: Introduce Delta type

[FUJITA Tomonori]

On Sun, 20 Oct 2024 15:05:36 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> Again, if you want to throw away all the unused methods and only have
> the rounding up one, then that is reasonable, but please let's not add
> misleading methods that could add more bugs than the ones you are
> trying to avoid. Please use `as_micros_ceil()` or similar.

as_micros_ceil() looks good. I'll add it in the next version.

I'll drop the unused methods in the next version. I think that adding
a type with the unused, very basic interface isn't bad though; it
could give more info about the type.

[PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta

[FUJITA Tomonori]

Change the output type of Ktime's subtraction operation from Ktime to
Delta. Currently, the output is Ktime:

Ktime = Ktime - Ktime

It means that Ktime is used to represent timedelta. Delta is
introduced so use it. A typical example is calculating the elapsed
time:

Delta = current Ktime - past Ktime;

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time.rs | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 38a70dc98083..8c00854db58c 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -74,16 +74,16 @@ pub fn to_ms(self) -> i64 {
 /// Returns the number of milliseconds between two ktimes.
 #[inline]
 pub fn ktime_ms_delta(later: Ktime, earlier: Ktime) -> i64 {
-    (later - earlier).to_ms()
+    (later - earlier).as_millis()
 }
 
 impl core::ops::Sub for Ktime {
-    type Output = Ktime;
+    type Output = Delta;
 
     #[inline]
-    fn sub(self, other: Ktime) -> Ktime {
-        Self {
-            inner: self.inner - other.inner,
+    fn sub(self, other: Ktime) -> Delta {
+        Delta {
+            nanos: self.inner - other.inner,
         }
     }
 }
-- 
2.43.0

Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta

[Alice Ryhl]

On Wed, Oct 16, 2024 at 5:53 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Change the output type of Ktime's subtraction operation from Ktime to
> Delta. Currently, the output is Ktime:
>
> Ktime = Ktime - Ktime
>
> It means that Ktime is used to represent timedelta. Delta is
> introduced so use it. A typical example is calculating the elapsed
> time:
>
> Delta = current Ktime - past Ktime;
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

So this means that you are repurposing Ktime as a replacement for
Instant rather than making both a Delta and Instant type? Okay. That
seems reasonable enough.

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

Alice

Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta

[Boqun Feng]

On Wed, Oct 16, 2024 at 10:25:11AM +0200, Alice Ryhl wrote:
> On Wed, Oct 16, 2024 at 5:53 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
> >
> > Change the output type of Ktime's subtraction operation from Ktime to
> > Delta. Currently, the output is Ktime:
> >
> > Ktime = Ktime - Ktime
> >
> > It means that Ktime is used to represent timedelta. Delta is
> > introduced so use it. A typical example is calculating the elapsed
> > time:
> >
> > Delta = current Ktime - past Ktime;
> >
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> So this means that you are repurposing Ktime as a replacement for
> Instant rather than making both a Delta and Instant type? Okay. That
> seems reasonable enough.
> 

I think it's still reasonable to introduce a `Instant<ClockSource>` type
(based on `Ktime`) to avoid some mis-uses, but we can do that in the
future.

Regards,
Boqun

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

Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta

[FUJITA Tomonori]

On Wed, 16 Oct 2024 10:25:11 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

> On Wed, Oct 16, 2024 at 5:53 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> Change the output type of Ktime's subtraction operation from Ktime to
>> Delta. Currently, the output is Ktime:
>>
>> Ktime = Ktime - Ktime
>>
>> It means that Ktime is used to represent timedelta. Delta is
>> introduced so use it. A typical example is calculating the elapsed
>> time:
>>
>> Delta = current Ktime - past Ktime;
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> So this means that you are repurposing Ktime as a replacement for
> Instant rather than making both a Delta and Instant type? Okay. That
> seems reasonable enough.

Yes.

Surely, we could create both Delta and Instant. What is Ktime used
for? Both can simply use bindings::ktime_t like the followings?

pub struct Instant {
    inner: bindings::ktime_t,
}

pub struct Delta {
    inner: bindings::ktime_t,
}

Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta

[Alice Ryhl]

On Thu, Oct 17, 2024 at 9:11 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Wed, 16 Oct 2024 10:25:11 +0200
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > On Wed, Oct 16, 2024 at 5:53 AM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:
> >>
> >> Change the output type of Ktime's subtraction operation from Ktime to
> >> Delta. Currently, the output is Ktime:
> >>
> >> Ktime = Ktime - Ktime
> >>
> >> It means that Ktime is used to represent timedelta. Delta is
> >> introduced so use it. A typical example is calculating the elapsed
> >> time:
> >>
> >> Delta = current Ktime - past Ktime;
> >>
> >> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> >
> > So this means that you are repurposing Ktime as a replacement for
> > Instant rather than making both a Delta and Instant type? Okay. That
> > seems reasonable enough.
>
> Yes.
>
> Surely, we could create both Delta and Instant. What is Ktime used
> for? Both can simply use bindings::ktime_t like the followings?
>
> pub struct Instant {
>     inner: bindings::ktime_t,
> }
>
> pub struct Delta {
>     inner: bindings::ktime_t,
> }

What you're doing is okay. No complaints.

Alice

Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta

[Miguel Ojeda]

On Thu, Oct 17, 2024 at 9:11 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Surely, we could create both Delta and Instant. What is Ktime used
> for? Both can simply use bindings::ktime_t like the followings?

I think it may help having 2 (public) types, rather than reusing the
`Ktime` name for one of them, because people may associate several
concepts to `ktime_t` which is what they know already, but I would
suggest mentioning in the docs clearly that these maps to usecase
subsets of `ktime_t` (whether we mention or not that they are
supposed to be `ktime_t`s is another thing, even if they are).

Whether we have a third private type internally for `Ktime` or not
does not matter much, so whatever is best for implementation purposes.
And if we do have a private `Ktime`, I would avoid making it public
unless there is a good reason for doing so.

Cheers,
Miguel

Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta

[FUJITA Tomonori]

On Thu, 17 Oct 2024 18:45:13 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

>> Surely, we could create both Delta and Instant. What is Ktime used
>> for? Both can simply use bindings::ktime_t like the followings?
> 
> I think it may help having 2 (public) types, rather than reusing the
> `Ktime` name for one of them, because people may associate several
> concepts to `ktime_t` which is what they know already, but I would
> suggest mentioning in the docs clearly that these maps to usecase
> subsets of `ktime_t` (whether we mention or not that they are
> supposed to be `ktime_t`s is another thing, even if they are).

Sounds good. I'll create both `Delta` and `Instant`.

> Whether we have a third private type internally for `Ktime` or not
> does not matter much, so whatever is best for implementation purposes.
> And if we do have a private `Ktime`, I would avoid making it public
> unless there is a good reason for doing so.

I don't think implementing `Delta` and `Instant` types on the top of a
private Ktime makes sense. I'll just rename the current `Ktime` type to
`Instant` type.

Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta

[Boqun Feng]

On Wed, Oct 16, 2024 at 12:52:08PM +0900, FUJITA Tomonori wrote:
> Change the output type of Ktime's subtraction operation from Ktime to
> Delta. Currently, the output is Ktime:
> 
> Ktime = Ktime - Ktime
> 
> It means that Ktime is used to represent timedelta. Delta is
> introduced so use it. A typical example is calculating the elapsed
> time:
> 
> Delta = current Ktime - past Ktime;
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/time.rs | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 38a70dc98083..8c00854db58c 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -74,16 +74,16 @@ pub fn to_ms(self) -> i64 {
>  /// Returns the number of milliseconds between two ktimes.
>  #[inline]
>  pub fn ktime_ms_delta(later: Ktime, earlier: Ktime) -> i64 {
> -    (later - earlier).to_ms()
> +    (later - earlier).as_millis()
>  }
>  
>  impl core::ops::Sub for Ktime {
> -    type Output = Ktime;
> +    type Output = Delta;
>  
>      #[inline]
> -    fn sub(self, other: Ktime) -> Ktime {
> -        Self {
> -            inner: self.inner - other.inner,
> +    fn sub(self, other: Ktime) -> Delta {
> +        Delta {
> +            nanos: self.inner - other.inner,

My understanding is that ktime_t, when used as a timestamp, can only
have a value in range [0, i64::MAX], so this substraction here never
overflows, maybe we want add a comment here?

We should really differ two cases: 1) user inputs may cause overflow vs
2) implementation errors or unexpected hardware errors cause overflow, I
think.

Regards,
Boqun

>          }
>      }
>  }
> -- 
> 2.43.0
> 
> 

Re: [PATCH net-next v3 3/8] rust: time: Change output of Ktime's sub operation to Delta

[FUJITA Tomonori]

On Wed, 16 Oct 2024 13:03:48 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

>>  impl core::ops::Sub for Ktime {
>> -    type Output = Ktime;
>> +    type Output = Delta;
>>  
>>      #[inline]
>> -    fn sub(self, other: Ktime) -> Ktime {
>> -        Self {
>> -            inner: self.inner - other.inner,
>> +    fn sub(self, other: Ktime) -> Delta {
>> +        Delta {
>> +            nanos: self.inner - other.inner,
> 
> My understanding is that ktime_t, when used as a timestamp, can only
> have a value in range [0, i64::MAX], so this substraction here never
> overflows, maybe we want add a comment here?

Sure, I'll add.

[PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

[FUJITA Tomonori]

Implement Add<Delta> for Ktime to support the operation:

Ktime = Ktime + Delta

This is typically used to calculate the future time when the timeout
will occur.

timeout Ktime = current Ktime (via ktime_get()) + Delta;
// do something
if (ktime_get() > timeout Ktime) {
    // timed out
}

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time.rs | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 8c00854db58c..9b0537b63cf7 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -155,3 +155,14 @@ pub fn as_secs(self) -> i64 {
         self.nanos / NSEC_PER_SEC
     }
 }
+
+impl core::ops::Add<Delta> for Ktime {
+    type Output = Ktime;
+
+    #[inline]
+    fn add(self, delta: Delta) -> Ktime {
+        Ktime {
+            inner: self.inner + delta.as_nanos(),
+        }
+    }
+}
-- 
2.43.0

Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

[Alice Ryhl]

On Wed, Oct 16, 2024 at 5:53 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Implement Add<Delta> for Ktime to support the operation:
>
> Ktime = Ktime + Delta
>
> This is typically used to calculate the future time when the timeout
> will occur.
>
> timeout Ktime = current Ktime (via ktime_get()) + Delta;
> // do something
> if (ktime_get() > timeout Ktime) {
>     // timed out
> }
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

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

>  rust/kernel/time.rs | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 8c00854db58c..9b0537b63cf7 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -155,3 +155,14 @@ pub fn as_secs(self) -> i64 {
>          self.nanos / NSEC_PER_SEC
>      }
>  }
> +
> +impl core::ops::Add<Delta> for Ktime {
> +    type Output = Ktime;
> +
> +    #[inline]
> +    fn add(self, delta: Delta) -> Ktime {
> +        Ktime {
> +            inner: self.inner + delta.as_nanos(),

You don't want to do `delta.inner` here?

Alice

Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

[Boqun Feng]

On Wed, Oct 16, 2024 at 12:52:09PM +0900, FUJITA Tomonori wrote:
> Implement Add<Delta> for Ktime to support the operation:
> 
> Ktime = Ktime + Delta
> 
> This is typically used to calculate the future time when the timeout
> will occur.
> 
> timeout Ktime = current Ktime (via ktime_get()) + Delta;
> // do something
> if (ktime_get() > timeout Ktime) {
>     // timed out
> }
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/time.rs | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 8c00854db58c..9b0537b63cf7 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -155,3 +155,14 @@ pub fn as_secs(self) -> i64 {
>          self.nanos / NSEC_PER_SEC
>      }
>  }
> +
> +impl core::ops::Add<Delta> for Ktime {
> +    type Output = Ktime;
> +
> +    #[inline]
> +    fn add(self, delta: Delta) -> Ktime {
> +        Ktime {
> +            inner: self.inner + delta.as_nanos(),

What if overflow happens in this addition? Is the expectation that user
should avoid overflows? I asked because we have ktime_add_safe() which
saturate at KTIME_SEC_MAX.

Regards,
Boqun

> +        }
> +    }
> +}
> -- 
> 2.43.0
> 
> 

Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

[FUJITA Tomonori]

On Wed, 16 Oct 2024 12:54:07 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

>> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
>> index 8c00854db58c..9b0537b63cf7 100644
>> --- a/rust/kernel/time.rs
>> +++ b/rust/kernel/time.rs
>> @@ -155,3 +155,14 @@ pub fn as_secs(self) -> i64 {
>>          self.nanos / NSEC_PER_SEC
>>      }
>>  }
>> +
>> +impl core::ops::Add<Delta> for Ktime {
>> +    type Output = Ktime;
>> +
>> +    #[inline]
>> +    fn add(self, delta: Delta) -> Ktime {
>> +        Ktime {
>> +            inner: self.inner + delta.as_nanos(),
> 
> What if overflow happens in this addition? Is the expectation that user
> should avoid overflows?

Yes, I'll add a comment.

> I asked because we have ktime_add_safe() which saturate at
> KTIME_SEC_MAX.

We could add the Rust version of add_safe method. But looks like
ktime_add_safe() is used by only some core systems so we don't need to
add it now?

Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

[Alice Ryhl]

On Thu, Oct 17, 2024 at 11:31 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Wed, 16 Oct 2024 12:54:07 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
>
> >> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> >> index 8c00854db58c..9b0537b63cf7 100644
> >> --- a/rust/kernel/time.rs
> >> +++ b/rust/kernel/time.rs
> >> @@ -155,3 +155,14 @@ pub fn as_secs(self) -> i64 {
> >>          self.nanos / NSEC_PER_SEC
> >>      }
> >>  }
> >> +
> >> +impl core::ops::Add<Delta> for Ktime {
> >> +    type Output = Ktime;
> >> +
> >> +    #[inline]
> >> +    fn add(self, delta: Delta) -> Ktime {
> >> +        Ktime {
> >> +            inner: self.inner + delta.as_nanos(),
> >
> > What if overflow happens in this addition? Is the expectation that user
> > should avoid overflows?
>
> Yes, I'll add a comment.
>
> > I asked because we have ktime_add_safe() which saturate at
> > KTIME_SEC_MAX.
>
> We could add the Rust version of add_safe method. But looks like
> ktime_add_safe() is used by only some core systems so we don't need to
> add it now?

I think it makes sense to follow the standard Rust addition
conventions here. Rust normally treats + as addition that BUGs on
overflow (with the appropriate configs set), and then there's a
saturating_add function for when you want it to saturate.

Alice

Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

[Miguel Ojeda]

On Thu, Oct 17, 2024 at 11:31 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> We could add the Rust version of add_safe method. But looks like
> ktime_add_safe() is used by only some core systems so we don't need to
> add it now?

There was some discussion in the past about this -- I wrote there a
summary of the `add` variants:

    https://lore.kernel.org/rust-for-linux/CANiq72ka4UvJzb4dN12fpA1WirgDHXcvPurvc7B9t+iPUfWnew@mail.gmail.com/

I think this is a case where following the naming of the C side would
be worse, i.e. where it is worth not applying our usual guideline.
Calling something `_safe`/`_unsafe` like the C macros would be quite
confusing for Rust.

Personally, I would prefer that we stay consistent, which will help
when dealing with more code. That is (from the message above):

  - No suffix: not supposed to wrap. So, in Rust, map it to operators.
  - `_unsafe()`: wraps. So, in Rust, map it to `wrapping` methods.
  - `_safe()`: saturates. So, in Rust, map it to `saturating` methods.

(assuming I read the C code correctly back then.)

And if there are any others that are Rust-unsafe, then map it to
`unchecked` methods, of course.

Cheers,
Miguel

Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

[Boqun Feng]

On Thu, Oct 17, 2024 at 06:33:23PM +0200, Miguel Ojeda wrote:
> On Thu, Oct 17, 2024 at 11:31 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
> >
> > We could add the Rust version of add_safe method. But looks like
> > ktime_add_safe() is used by only some core systems so we don't need to
> > add it now?
> 
> There was some discussion in the past about this -- I wrote there a
> summary of the `add` variants:
> 
>     https://lore.kernel.org/rust-for-linux/CANiq72ka4UvJzb4dN12fpA1WirgDHXcvPurvc7B9t+iPUfWnew@mail.gmail.com/
> 
> I think this is a case where following the naming of the C side would
> be worse, i.e. where it is worth not applying our usual guideline.
> Calling something `_safe`/`_unsafe` like the C macros would be quite
> confusing for Rust.
> 
> Personally, I would prefer that we stay consistent, which will help
> when dealing with more code. That is (from the message above):
> 
>   - No suffix: not supposed to wrap. So, in Rust, map it to operators.
>   - `_unsafe()`: wraps. So, in Rust, map it to `wrapping` methods.
>   - `_safe()`: saturates. So, in Rust, map it to `saturating` methods.
> 
> (assuming I read the C code correctly back then.)
> 
> And if there are any others that are Rust-unsafe, then map it to
> `unchecked` methods, of course.
> 

The point I tried to make is that `+` operator of Ktime can cause
overflow because of *user inputs*, unlike the `-` operator of Ktime,
which cannot cause overflow as long as Ktime is implemented correctly
(as a timestamp). Because the overflow possiblity is exposed to users,
then we need to 1) document it and 2) provide saturating_add() (maybe
also checked_add() and overflowing_add()) so that users won't need to do
the saturating themselves:

	let mut kt = Ktime::ktime_get();
	let d: Delta = <maybe a userspace input>;

	// kt + d may overflow, so checking
	if let Some(_) = kt.as_ns().checked_add(d.as_nanos()) {
	    // not overflow, can add
	    kt = kt + d;
	} else {
	    // set kt to KTIME_SEC_MAX
	}

instead, they can do:

	let kt = Ktime::ktime_get();
	let d: Delta = <maybe a userspace input>;

	kt = kt.saturating_add(d);

but one thing I'm not sure is since it looks like saturating to
KTIME_SEC_MAX is the current C choice, if we want to do the same, should
we use the name `add_safe()` instead of `saturating_add()`? FWIW, it
seems harmless to saturate at KTIME_MAX to me. So personally, I like
what Alice suggested.

Hope these make sense.

Regards,
Boqun

> Cheers,
> Miguel

Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

[Miguel Ojeda]

On Thu, Oct 17, 2024 at 7:03 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> The point I tried to make is that `+` operator of Ktime can cause
> overflow because of *user inputs*, unlike the `-` operator of Ktime,
> which cannot cause overflow as long as Ktime is implemented correctly
> (as a timestamp). Because the overflow possiblity is exposed to users,
> then we need to 1) document it and 2) provide saturating_add() (maybe
> also checked_add() and overflowing_add()) so that users won't need to do
> the saturating themselves:

Generally, operators are expected to possibly wrap/panic, so that
would be fine, no?

It may be surprising to `ktime_t` users, and that is bad. It is one
more reason to avoid using the same name for the type, too.

My only concern is that people may expect this sort of thing (timing
related) to "usually/just work" and not expect panics. That is bad,
but if we remain consistent, it may be best to pay the cost now. In
other words, when someone sees an operator, it is saying it is never
supposed to wrap, and that is easy to remember. Perhaps some types
could avoid providing the operators if it is too dangerous to use
them.

The other option is be inconsistent in our use of operators, and
instead give operators the "best" semantics for each type. That can
definitely provide better ergonomics in some cases, but there is a
high risk of forgetting what each operator does for each type --
operator overloading can be quite risky.

So that is why I think it may be best/easier to generally say "we
don't do operator overloading in general unless the operator really
behaves like a core one", especially early on.

>         kt = kt.saturating_add(d);

Yeah, that is what we want (I may be missing something in your argument though).

> but one thing I'm not sure is since it looks like saturating to
> KTIME_SEC_MAX is the current C choice, if we want to do the same, should
> we use the name `add_safe()` instead of `saturating_add()`? FWIW, it
> seems harmless to saturate at KTIME_MAX to me. So personally, I like
> what Alice suggested.

Do you mean it would be confusing to not saturate to the highest the
lower/inner level type can hold?

I mean, nothing says we need to saturate to the highest -- we could
have a type invariant. (One more reason to avoid the same name).

Worst case, we can have two saturating methods, or two types, if really needed.

Thomas can probably tell us what are the most important use cases
needed, and whether it is a good opportunity to clean/redesign this in
Rust differently.

Cheers,
Miguel

Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

[Boqun Feng]

On Thu, Oct 17, 2024 at 08:10:17PM +0200, Miguel Ojeda wrote:
> On Thu, Oct 17, 2024 at 7:03 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > The point I tried to make is that `+` operator of Ktime can cause
> > overflow because of *user inputs*, unlike the `-` operator of Ktime,
> > which cannot cause overflow as long as Ktime is implemented correctly
> > (as a timestamp). Because the overflow possiblity is exposed to users,
> > then we need to 1) document it and 2) provide saturating_add() (maybe
> > also checked_add() and overflowing_add()) so that users won't need to do
> > the saturating themselves:
> 
> Generally, operators are expected to possibly wrap/panic, so that
> would be fine, no?
> 

Yes, but I guess I need to make it clear: the current `+` operator
implemention is fine for me. What I'm trying to say is: since we have a
`+` that expects users to not provide inputs that causes overflow, then
it makes sense to provide a saturating_add() at the same time for the
API completeness.

> It may be surprising to `ktime_t` users, and that is bad. It is one
> more reason to avoid using the same name for the type, too.
> 
> My only concern is that people may expect this sort of thing (timing
> related) to "usually/just work" and not expect panics. That is bad,
> but if we remain consistent, it may be best to pay the cost now. In
> other words, when someone sees an operator, it is saying it is never
> supposed to wrap, and that is easy to remember. Perhaps some types
> could avoid providing the operators if it is too dangerous to use
> them.
> 

Sounds reasonable to me.

> The other option is be inconsistent in our use of operators, and
> instead give operators the "best" semantics for each type. That can
> definitely provide better ergonomics in some cases, but there is a
> high risk of forgetting what each operator does for each type --
> operator overloading can be quite risky.
> 

Agreed.

> So that is why I think it may be best/easier to generally say "we
> don't do operator overloading in general unless the operator really
> behaves like a core one", especially early on.
> 
> >         kt = kt.saturating_add(d);
> 
> Yeah, that is what we want (I may be missing something in your argument though).
> 
> > but one thing I'm not sure is since it looks like saturating to
> > KTIME_SEC_MAX is the current C choice, if we want to do the same, should
> > we use the name `add_safe()` instead of `saturating_add()`? FWIW, it
> > seems harmless to saturate at KTIME_MAX to me. So personally, I like
> > what Alice suggested.
> 
> Do you mean it would be confusing to not saturate to the highest the
> lower/inner level type can hold?
> 

Yes.

> I mean, nothing says we need to saturate to the highest -- we could
> have a type invariant. (One more reason to avoid the same name).
> 
> Worst case, we can have two saturating methods, or two types, if really needed.
> 
> Thomas can probably tell us what are the most important use cases
> needed, and whether it is a good opportunity to clean/redesign this in
> Rust differently.
> 

Works for me!

Regards,
Boqun

> Cheers,
> Miguel

Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

[Miguel Ojeda]

On Thu, Oct 17, 2024 at 7:03 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> but one thing I'm not sure is since it looks like saturating to
> KTIME_SEC_MAX is the current C choice, if we want to do the same, should
> we use the name `add_safe()` instead of `saturating_add()`? FWIW, it
> seems harmless to saturate at KTIME_MAX to me. So personally, I like

Wait -- `ktime_add_safe()` calls `ktime_set(KTIME_SEC_MAX, 0)` which
goes into the conditional that returns `KTIME_MAX`, not `KTIME_SEC_MAX
* NSEC_PER_SEC` (which is what I guess you were saying).

So I am confused -- it doesn't saturate to `KTIME_SEC_MAX` (scaled)
anyway. Which is confusing in itself.

In fact, it means that `ktime_add_safe()` allows you to get any value
whatsoever as long as you don't overflow, but `ktime_set` does not
allow you to -- unless you use enough nanoseconds to get you there
(i.e. over a second in nanoseconds).

Cheers,
Miguel

Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

[Boqun Feng]

On Thu, Oct 17, 2024 at 08:58:48PM +0200, Miguel Ojeda wrote:
> On Thu, Oct 17, 2024 at 7:03 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > but one thing I'm not sure is since it looks like saturating to
> > KTIME_SEC_MAX is the current C choice, if we want to do the same, should
> > we use the name `add_safe()` instead of `saturating_add()`? FWIW, it
> > seems harmless to saturate at KTIME_MAX to me. So personally, I like
> 
> Wait -- `ktime_add_safe()` calls `ktime_set(KTIME_SEC_MAX, 0)` which
> goes into the conditional that returns `KTIME_MAX`, not `KTIME_SEC_MAX
> * NSEC_PER_SEC` (which is what I guess you were saying).
> 

Yeah.. this is very interesting ;-) I missed that.

> So I am confused -- it doesn't saturate to `KTIME_SEC_MAX` (scaled)
> anyway. Which is confusing in itself.
> 

Then I think it suffices to say ktime_add_safe() is just a
saturating_add() for i64? ;-)

> In fact, it means that `ktime_add_safe()` allows you to get any value
> whatsoever as long as you don't overflow, but `ktime_set` does not
> allow you to -- unless you use enough nanoseconds to get you there
> (i.e. over a second in nanoseconds).
> 

That seems to the be case.

Regards,
Boqun

> Cheers,
> Miguel
> 

Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

[FUJITA Tomonori]

On Thu, 17 Oct 2024 18:33:23 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Thu, Oct 17, 2024 at 11:31 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> We could add the Rust version of add_safe method. But looks like
>> ktime_add_safe() is used by only some core systems so we don't need to
>> add it now?
> 
> There was some discussion in the past about this -- I wrote there a
> summary of the `add` variants:
> 
>     https://lore.kernel.org/rust-for-linux/CANiq72ka4UvJzb4dN12fpA1WirgDHXcvPurvc7B9t+iPUfWnew@mail.gmail.com/
> 
> I think this is a case where following the naming of the C side would
> be worse, i.e. where it is worth not applying our usual guideline.
> Calling something `_safe`/`_unsafe` like the C macros would be quite
> confusing for Rust.
> 
> Personally, I would prefer that we stay consistent, which will help
> when dealing with more code. That is (from the message above):
> 
>   - No suffix: not supposed to wrap. So, in Rust, map it to operators.
>   - `_unsafe()`: wraps. So, in Rust, map it to `wrapping` methods.
>   - `_safe()`: saturates. So, in Rust, map it to `saturating` methods.

Can we add the above to Documentation/rust/coding-guidelines.rst?

I think that it's better than adding the similar comment to every
function that performs arithmetic.

Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

[Miguel Ojeda]

On Wed, Oct 23, 2024 at 8:51 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Can we add the above to Documentation/rust/coding-guidelines.rst?

Sounds good to me -- I will send a patch.

Just to confirm, do you mean the whole operators overloading guideline
that I mentioned elsewhere and what semantics the arithmetic operators
should have (i.e.to avoid having to repeatedly document why operator
do "not supposed to wrap" and why we relegate saturating/wrapping/...
to methods), or something else?

Thanks!

Cheers,
Miguel

Re: [PATCH net-next v3 4/8] rust: time: Implement addition of Ktime and Delta

[FUJITA Tomonori]

On Wed, 23 Oct 2024 12:59:03 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Wed, Oct 23, 2024 at 8:51 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> Can we add the above to Documentation/rust/coding-guidelines.rst?
> 
> Sounds good to me -- I will send a patch.

Great, thanks!

> Just to confirm, do you mean the whole operators overloading guideline
> that I mentioned elsewhere and what semantics the arithmetic operators
> should have (i.e.to avoid having to repeatedly document why operator
> do "not supposed to wrap" and why we relegate saturating/wrapping/...
> to methods), or something else?

I was only thinking about the guideline for naming (at least as a
starter); your mail in this thread:

https://lore.kernel.org/netdev/CANiq72mbWVVCA_EjV_7DtMYHH_RF9P9Br=sRdyLtPFkythST1w@mail.gmail.com/

[PATCH net-next v3 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.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
Link: https://rust-for-linux.com/klint [1]
---
 rust/helpers/helpers.c    |  1 +
 rust/helpers/time.c       |  8 ++++++++
 rust/kernel/time.rs       |  4 +++-
 rust/kernel/time/delay.rs | 31 +++++++++++++++++++++++++++++++
 4 files changed, 43 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 9b0537b63cf7..d58daff6f928 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..dc7e2b3a0ab2
--- /dev/null
+++ b/rust/kernel/time/delay.rs
@@ -0,0 +1,31 @@
+// 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;
+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 longer than one microsecond.
+///
+/// This function can only be used in a nonatomic context.
+pub fn fsleep(delta: time::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(
+            ((delta.as_nanos() + time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC) as c_ulong,
+        )
+    }
+}
-- 
2.43.0

Re: [PATCH net-next v3 5/8] rust: time: Add wrapper for fsleep function

[Alice Ryhl]

On Wed, Oct 16, 2024 at 5:54 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Add a wrapper for fsleep, flexible sleep functions in
> `include/linux/delay.h` which typically deals with hardware delays.
>
> The kernel supports several `sleep` functions to handle various
> lengths of delay. This adds fsleep, automatically chooses the best
> sleep method based on a duration.
>
> `sleep` functions including `fsleep` belongs to TIMERS, not
> TIMEKEEPING. They are maintained separately. rust/kernel/time.rs is an
> abstraction for TIMEKEEPING. To make Rust abstractions match the C
> side, add rust/kernel/time/delay.rs for this wrapper.
>
> fsleep() can only be used in a nonatomic context. This requirement is
> not checked by these abstractions, but it is intended that klint [1]
> or a similar tool will be used to check it in the future.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Link: https://rust-for-linux.com/klint [1]
> ---
>  rust/helpers/helpers.c    |  1 +
>  rust/helpers/time.c       |  8 ++++++++
>  rust/kernel/time.rs       |  4 +++-
>  rust/kernel/time/delay.rs | 31 +++++++++++++++++++++++++++++++
>  4 files changed, 43 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 9b0537b63cf7..d58daff6f928 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..dc7e2b3a0ab2
> --- /dev/null
> +++ b/rust/kernel/time/delay.rs
> @@ -0,0 +1,31 @@
> +// 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;
> +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 longer than one microsecond.

Why is this required? Right now you just round up to one microsecond,
which seems okay.

> +/// This function can only be used in a nonatomic context.
> +pub fn fsleep(delta: time::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(
> +            ((delta.as_nanos() + time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC) as c_ulong,

You probably want this:

delta.as_nanos().saturating_add(time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC

This would avoid a crash if someone passes i64::MAX nanoseconds and
CONFIG_RUST_OVERFLOW_CHECKS is enabled.

Alice

Re: [PATCH net-next v3 5/8] rust: time: Add wrapper for fsleep function

[Miguel Ojeda]

On Wed, Oct 16, 2024 at 10:29 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> You probably want this:
>
> delta.as_nanos().saturating_add(time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC
>
> This would avoid a crash if someone passes i64::MAX nanoseconds and
> CONFIG_RUST_OVERFLOW_CHECKS is enabled.

I think we should document whether `fsleep` is expected to be usable
for "forever" values.

It sounds like that, given "too large" values in `msecs_to_jiffies`
mean "infinite timeout".

Cheers,
Miguel

Re: [PATCH net-next v3 5/8] rust: time: Add wrapper for fsleep function

[FUJITA Tomonori]

On Wed, 16 Oct 2024 11:42:07 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Wed, Oct 16, 2024 at 10:29 AM Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> You probably want this:
>>
>> delta.as_nanos().saturating_add(time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC
>>
>> This would avoid a crash if someone passes i64::MAX nanoseconds and
>> CONFIG_RUST_OVERFLOW_CHECKS is enabled.
> 
> I think we should document whether `fsleep` is expected to be usable
> for "forever" values.
> 
> It sounds like that, given "too large" values in `msecs_to_jiffies`
> mean "infinite timeout".

Do you mean msecs_to_jiffies() returns MAX_JIFFY_OFFSET ((LONG_MAX >>
1)-1) with a value exceeding i32::MAX milliseconds?

Re: [PATCH net-next v3 5/8] rust: time: Add wrapper for fsleep function

[Miguel Ojeda]

On Thu, Oct 24, 2024 at 2:22 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Do you mean msecs_to_jiffies() returns MAX_JIFFY_OFFSET ((LONG_MAX >>
> 1)-1) with a value exceeding i32::MAX milliseconds?

Yeah -- well, I was referring to its docs:

 * - negative values mean 'infinite timeout' (MAX_JIFFY_OFFSET)
 *
 * - 'too large' values [that would result in larger than
 *   MAX_JIFFY_OFFSET values] mean 'infinite timeout' too.

So I assume `fsleep` is meant to inherit that.

Cheers,
Miguel

Re: [PATCH net-next v3 5/8] rust: time: Add wrapper for fsleep function

[FUJITA Tomonori]

On Wed, 16 Oct 2024 10:29:12 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

>> diff --git a/rust/kernel/time/delay.rs b/rust/kernel/time/delay.rs
>> new file mode 100644
>> index 000000000000..dc7e2b3a0ab2
>> --- /dev/null
>> +++ b/rust/kernel/time/delay.rs
>> @@ -0,0 +1,31 @@
>> +// 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;
>> +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 longer than one microsecond.
> 
> Why is this required? Right now you just round up to one microsecond,
> which seems okay.

Oops, it should have been removed.

>> +/// This function can only be used in a nonatomic context.
>> +pub fn fsleep(delta: time::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(
>> +            ((delta.as_nanos() + time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC) as c_ulong,
> 
> You probably want this:
> 
> delta.as_nanos().saturating_add(time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC
> 
> This would avoid a crash if someone passes i64::MAX nanoseconds and
> CONFIG_RUST_OVERFLOW_CHECKS is enabled.

Ah, I'll fix.

Thanks!

Re: [PATCH net-next v3 5/8] rust: time: Add wrapper for fsleep function

[Andrew Lunn]

> +/// This function can only be used in a nonatomic context.
> +pub fn fsleep(delta: time::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(
> +            ((delta.as_nanos() + time::NSEC_PER_USEC - 1) / time::NSEC_PER_USEC) as c_ulong,

You add a as_sec() helper, but then don't use it? If the helper does
not do what you want, maybe the helper is wrong? This is part of why
we say all APIs should have a user.

	Andrew

[PATCH net-next v3 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 d678a58c0205..faef059c16b1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10172,6 +10172,7 @@ F:	kernel/time/hrtimer.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
@@ -23382,6 +23383,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 net-next v3 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.

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.

For the proper debug info, readx_poll_timeout() is implemented as a
macro.

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.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
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 | 84 ++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |  1 +
 6 files changed, 105 insertions(+)
 create mode 100644 rust/helpers/kernel.c
 create mode 100644 rust/kernel/io.rs
 create mode 100644 rust/kernel/io/poll.rs

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 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..d4bb791f665b
--- /dev/null
+++ b/rust/kernel/io/poll.rs
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! IO polling.
+//!
+//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
+
+use crate::{
+    bindings,
+    error::{code::*, Result},
+    time::{delay::fsleep, Delta, Ktime},
+};
+
+/// 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,
+    sleep_before_read: bool,
+) -> Result<T>
+where
+    Op: FnMut() -> Result<T>,
+    Cond: Fn(T) -> bool,
+{
+    let timeout = Ktime::ktime_get() + timeout_delta;
+    let sleep = !sleep_delta.is_zero();
+
+    if sleep_before_read && sleep {
+        fsleep(sleep_delta);
+    }
+
+    let val = loop {
+        let val = op()?;
+        if cond(val) {
+            break val;
+        }
+        if !timeout_delta.is_zero() && Ktime::ktime_get() > timeout {
+            break op()?;
+        }
+        if sleep {
+            fsleep(sleep_delta);
+        }
+        // SAFETY: FFI call.
+        unsafe { bindings::cpu_relax() }
+    };
+
+    if cond(val) {
+        Ok(val)
+    } else {
+        Err(ETIMEDOUT)
+    }
+}
+
+/// 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) => {{
+        #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
+        if !$sleep_delta.is_zero() {
+            // SAFETY: FFI call.
+            unsafe {
+                $crate::bindings::__might_sleep(
+                    ::core::file!().as_ptr() as *const i8,
+                    ::core::line!() as i32,
+                )
+            }
+        }
+
+        $crate::io::poll::read_poll_timeout($op, $cond, $sleep_delta, $timeout_delta, false)
+    }};
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index b5f4b3ce6b48..d5fd6aeb24ca 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;
-- 
2.43.0

Re: [PATCH net-next v3 7/8] rust: Add read_poll_timeout functions

[Alice Ryhl]

On Wed, Oct 16, 2024 at 5:54 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Add read_poll_timeout functions which poll periodically until a
> condition is met or a timeout is reached.
>
> C's read_poll_timeout (include/linux/iopoll.h) is a complicated macro
> and a simple wrapper for Rust doesn't work. So this implements the
> same functionality in Rust.
>
> The C version uses usleep_range() while the Rust version uses
> fsleep(), which uses the best sleep method so it works with spans that
> usleep_range() doesn't work nicely with.
>
> Unlike the C version, __might_sleep() is used instead of might_sleep()
> to show proper debug info; the file name and line
> number. might_resched() could be added to match what the C version
> does but this function works without it.
>
> For the proper debug info, readx_poll_timeout() is implemented as a
> macro.
>
> 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.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> Link: https://rust-for-linux.com/klint [1]
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Duplicated SOB? This should just be:

Link: https://rust-for-linux.com/klint [1]
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

> +/// 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,
> +    sleep_before_read: bool,
> +) -> Result<T>
> +where
> +    Op: FnMut() -> Result<T>,
> +    Cond: Fn(T) -> bool,
> +{
> +    let timeout = Ktime::ktime_get() + timeout_delta;
> +    let sleep = !sleep_delta.is_zero();
> +
> +    if sleep_before_read && sleep {
> +        fsleep(sleep_delta);
> +    }

You always pass `false` for `sleep_before_read` so perhaps just remove
this and the argument entirely?

> +        if cond(val) {
> +            break val;
> +        }

This breaks out to another cond(val) check below. Perhaps just `return
Ok(val)` here to avoid the double condition check?

> +        if !timeout_delta.is_zero() && Ktime::ktime_get() > timeout {
> +            break op()?;
> +        }

Shouldn't you just return `Err(ETIMEDOUT)` here? I don't think you're
supposed to call `op` twice without a sleep in between.

> +        if sleep {
> +            fsleep(sleep_delta);
> +        }
> +        // SAFETY: FFI call.
> +        unsafe { bindings::cpu_relax() }

Should cpu_relax() be in an else branch? Also, please add a safe
wrapper function around cpu_relax.

> +macro_rules! readx_poll_timeout {
> +    ($op:expr, $cond:expr, $sleep_delta:expr, $timeout_delta:expr) => {{
> +        #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
> +        if !$sleep_delta.is_zero() {
> +            // SAFETY: FFI call.
> +            unsafe {
> +                $crate::bindings::__might_sleep(
> +                    ::core::file!().as_ptr() as *const i8,
> +                    ::core::line!() as i32,
> +                )
> +            }
> +        }

It could be nice to introduce a might_sleep macro that does this
internally? Then we can reuse this logic in other places.

Alice

Re: [PATCH net-next v3 7/8] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

On Wed, 16 Oct 2024 10:37:46 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

> On Wed, Oct 16, 2024 at 5:54 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> Add read_poll_timeout functions which poll periodically until a
>> condition is met or a timeout is reached.
>>
>> C's read_poll_timeout (include/linux/iopoll.h) is a complicated macro
>> and a simple wrapper for Rust doesn't work. So this implements the
>> same functionality in Rust.
>>
>> The C version uses usleep_range() while the Rust version uses
>> fsleep(), which uses the best sleep method so it works with spans that
>> usleep_range() doesn't work nicely with.
>>
>> Unlike the C version, __might_sleep() is used instead of might_sleep()
>> to show proper debug info; the file name and line
>> number. might_resched() could be added to match what the C version
>> does but this function works without it.
>>
>> For the proper debug info, readx_poll_timeout() is implemented as a
>> macro.
>>
>> 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.
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> Link: https://rust-for-linux.com/klint [1]
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Duplicated SOB? This should just be:
> 
> Link: https://rust-for-linux.com/klint [1]
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Oops, I'll fix.

>> +/// 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,
>> +    sleep_before_read: bool,
>> +) -> Result<T>
>> +where
>> +    Op: FnMut() -> Result<T>,
>> +    Cond: Fn(T) -> bool,
>> +{
>> +    let timeout = Ktime::ktime_get() + timeout_delta;
>> +    let sleep = !sleep_delta.is_zero();
>> +
>> +    if sleep_before_read && sleep {
>> +        fsleep(sleep_delta);
>> +    }
> 
> You always pass `false` for `sleep_before_read` so perhaps just remove
> this and the argument entirely?

Most of users of C's this function use false buf some use true. It
would be better to provide this option?

Would be better to provide only read_poll_timeout() which takes the
sleep_before_read argument?; No readx_poll_timeout wrapper.

>> +        if cond(val) {
>> +            break val;
>> +        }
> 
> This breaks out to another cond(val) check below. Perhaps just `return
> Ok(val)` here to avoid the double condition check?

I think that that's how the C version works. But `return Ok(val)` here
is fine, I guess.

>> +        if !timeout_delta.is_zero() && Ktime::ktime_get() > timeout {
>> +            break op()?;
>> +        }
> 
> Shouldn't you just return `Err(ETIMEDOUT)` here? I don't think you're
> supposed to call `op` twice without a sleep in between.

I think that it's how the C version works.

>> +        if sleep {
>> +            fsleep(sleep_delta);
>> +        }
>> +        // SAFETY: FFI call.
>> +        unsafe { bindings::cpu_relax() }
> 
> Should cpu_relax() be in an else branch?

I think that we could. I'll remove if you prefer. The C version always
calls cpu_relax(). I think that the following commit explains why:

commit b407460ee99033503993ac7437d593451fcdfe44
Author: Geert Uytterhoeven <geert+renesas@glider.be>
Date:   Fri Jun 2 10:50:36 2023 +0200

    iopoll: Call cpu_relax() in busy loops


> Also, please add a safe wrapper function around cpu_relax.

Sure, which file do you think is best to add the wrapper to?

rust/kernel/processor.rs

?

>> +macro_rules! readx_poll_timeout {
>> +    ($op:expr, $cond:expr, $sleep_delta:expr, $timeout_delta:expr) => {{
>> +        #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
>> +        if !$sleep_delta.is_zero() {
>> +            // SAFETY: FFI call.
>> +            unsafe {
>> +                $crate::bindings::__might_sleep(
>> +                    ::core::file!().as_ptr() as *const i8,
>> +                    ::core::line!() as i32,
>> +                )
>> +            }
>> +        }
> 
> It could be nice to introduce a might_sleep macro that does this
> internally? Then we can reuse this logic in other places.

I think that with #[track_caller], we can use a normal function
instead of a macro.

Using might_sleep name for a wrapper of __might_sleep is confusing
since the C side has also might_sleep. __foo function name is
acceptable?

Re: [PATCH net-next v3 7/8] rust: Add read_poll_timeout functions

[Alice Ryhl]

On Wed, Oct 16, 2024 at 5:54 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
> +/// 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) => {{
> +        #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
> +        if !$sleep_delta.is_zero() {
> +            // SAFETY: FFI call.
> +            unsafe {
> +                $crate::bindings::__might_sleep(
> +                    ::core::file!().as_ptr() as *const i8,
> +                    ::core::line!() as i32,
> +                )
> +            }
> +        }

I wonder if we can use #[track_caller] and
core::panic::Location::caller [1] to do this without having to use a
macro? I don't know whether it would work, but if it does, that would
be super cool.

#[track_caller]
fn might_sleep() {
    let location = core::panic::Location::caller();
    // SAFETY: FFI call.
    unsafe {
        $crate::bindings::__might_sleep(
            location.file().as_char_ptr(),
            location.line() as i32,
        )
    }
}

Alice

[1]: https://doc.rust-lang.org/nightly/core/panic/struct.Location.html#method.caller

Re: [PATCH net-next v3 7/8] rust: Add read_poll_timeout functions

[Alice Ryhl]

On Wed, Oct 16, 2024 at 10:45 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Wed, Oct 16, 2024 at 5:54 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
> > +/// 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) => {{
> > +        #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
> > +        if !$sleep_delta.is_zero() {
> > +            // SAFETY: FFI call.
> > +            unsafe {
> > +                $crate::bindings::__might_sleep(
> > +                    ::core::file!().as_ptr() as *const i8,
> > +                    ::core::line!() as i32,
> > +                )
> > +            }
> > +        }
>
> I wonder if we can use #[track_caller] and
> core::panic::Location::caller [1] to do this without having to use a
> macro? I don't know whether it would work, but if it does, that would
> be super cool.
>
> #[track_caller]
> fn might_sleep() {
>     let location = core::panic::Location::caller();
>     // SAFETY: FFI call.
>     unsafe {
>         $crate::bindings::__might_sleep(
>             location.file().as_char_ptr(),
>             location.line() as i32,
>         )
>     }
> }

Actually, this raises a problem ... core::panic::Location doesn't give
us a nul-terminated string, so we probably can't pass it to
`__might_sleep`. The thing is, `::core::file!()` doesn't give us a
nul-terminated string either, so this code is probably incorrect
as-is.

Alice

Re: [PATCH net-next v3 7/8] rust: Add read_poll_timeout functions

[Miguel Ojeda]

On Wed, Oct 16, 2024 at 10:52 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> `__might_sleep`. The thing is, `::core::file!()` doesn't give us a
> nul-terminated string either, so this code is probably incorrect
> as-is.

Yeah, elsewhere we use `c_str!`. It would be nice to get
`core::c_file!` too, though that one is less critical since we can do
it ourselves. Perhaps we should also consider using Clippy's
`disallowed_macros` too if we don't expect people to often need the
normal one. Added to our list:

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

Cheers,
Miguel

Re: [PATCH net-next v3 7/8] rust: Add read_poll_timeout functions

[FUJITA Tomonori]

On Wed, 16 Oct 2024 10:52:17 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

> On Wed, Oct 16, 2024 at 10:45 AM Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> On Wed, Oct 16, 2024 at 5:54 AM FUJITA Tomonori
>> <fujita.tomonori@gmail.com> wrote:
>> > +/// 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) => {{
>> > +        #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
>> > +        if !$sleep_delta.is_zero() {
>> > +            // SAFETY: FFI call.
>> > +            unsafe {
>> > +                $crate::bindings::__might_sleep(
>> > +                    ::core::file!().as_ptr() as *const i8,
>> > +                    ::core::line!() as i32,
>> > +                )
>> > +            }
>> > +        }
>>
>> I wonder if we can use #[track_caller] and
>> core::panic::Location::caller [1] to do this without having to use a
>> macro? I don't know whether it would work, but if it does, that would
>> be super cool.

Seems it works, no need to use macro. Thanks a lot!

>> #[track_caller]
>> fn might_sleep() {
>>     let location = core::panic::Location::caller();
>>     // SAFETY: FFI call.
>>     unsafe {
>>         $crate::bindings::__might_sleep(
>>             location.file().as_char_ptr(),
>>             location.line() as i32,
>>         )
>>     }
>> }
> 
> Actually, this raises a problem ... core::panic::Location doesn't give
> us a nul-terminated string, so we probably can't pass it to
> `__might_sleep`. The thing is, `::core::file!()` doesn't give us a
> nul-terminated string either, so this code is probably incorrect
> as-is.

Ah, what's the recommended way to get a null-terminated string from
&str?

Re: [PATCH net-next v3 7/8] rust: Add read_poll_timeout functions

[Alice Ryhl]

On Fri, Oct 18, 2024 at 10:10 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Wed, 16 Oct 2024 10:52:17 +0200
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > On Wed, Oct 16, 2024 at 10:45 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >>
> >> On Wed, Oct 16, 2024 at 5:54 AM FUJITA Tomonori
> >> <fujita.tomonori@gmail.com> wrote:
> >> > +/// 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) => {{
> >> > +        #[cfg(CONFIG_DEBUG_ATOMIC_SLEEP)]
> >> > +        if !$sleep_delta.is_zero() {
> >> > +            // SAFETY: FFI call.
> >> > +            unsafe {
> >> > +                $crate::bindings::__might_sleep(
> >> > +                    ::core::file!().as_ptr() as *const i8,
> >> > +                    ::core::line!() as i32,
> >> > +                )
> >> > +            }
> >> > +        }
> >>
> >> I wonder if we can use #[track_caller] and
> >> core::panic::Location::caller [1] to do this without having to use a
> >> macro? I don't know whether it would work, but if it does, that would
> >> be super cool.
>
> Seems it works, no need to use macro. Thanks a lot!
>
> >> #[track_caller]
> >> fn might_sleep() {
> >>     let location = core::panic::Location::caller();
> >>     // SAFETY: FFI call.
> >>     unsafe {
> >>         $crate::bindings::__might_sleep(
> >>             location.file().as_char_ptr(),
> >>             location.line() as i32,
> >>         )
> >>     }
> >> }
> >
> > Actually, this raises a problem ... core::panic::Location doesn't give
> > us a nul-terminated string, so we probably can't pass it to
> > `__might_sleep`. The thing is, `::core::file!()` doesn't give us a
> > nul-terminated string either, so this code is probably incorrect
> > as-is.
>
> Ah, what's the recommended way to get a null-terminated string from
> &str?

In this case, you should be able to use the `c_str!` macro.

`kernel::c_str!(core::file!()).as_char_ptr()`

Alice

Re: [PATCH net-next v3 7/8] rust: Add read_poll_timeout functions

[Andrew Lunn]

> > Ah, what's the recommended way to get a null-terminated string from
> > &str?
> 
> In this case, you should be able to use the `c_str!` macro.
> 
> `kernel::c_str!(core::file!()).as_char_ptr()`

Does this allocate memory? In this case, that would be O.K, but at
some point i expect somebody is going to want the atomic version of
this poll helper. You then need to pass additional flags to kalloc()
if you call it in atomic context.

	Andrew

Re: [PATCH net-next v3 7/8] rust: Add read_poll_timeout functions

[Miguel Ojeda]

On Fri, Oct 18, 2024 at 4:15 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Does this allocate memory? In this case, that would be O.K, but at

No, it does not -- it is all done at compile-time.

Cheers,
Miguel

Re: [PATCH net-next v3 7/8] rust: Add read_poll_timeout functions

[Andrew Lunn]

> +            // SAFETY: FFI call.
> +            unsafe {
> +                $crate::bindings::__might_sleep(
> +                    ::core::file!().as_ptr() as *const i8,
> +                    ::core::line!() as i32,
> +                )

Can this be pulled out into an easy to share macro? I expect to see
this to be scattered in a number of files, so making it easy to use
would be good.

	Andrew

[PATCH net-next v3 8/8] 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 1ab065798175..dabb772c468f 100644
--- a/drivers/net/phy/qt2025.rs
+++ b/drivers/net/phy/qt2025.rs
@@ -18,7 +18,9 @@
     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 net-next v3 8/8] net: phy: qt2025: Wait until PHY becomes ready

[Alice Ryhl]

On Wed, Oct 16, 2024 at 5:54 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> 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>

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

Re: [PATCH net-next v3 0/8] rust: Add IO polling

[Andrew Lunn]

On Wed, Oct 16, 2024 at 12:52:05PM +0900, FUJITA Tomonori wrote:
> polls 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.
> 
> As a result of the past discussion, this introduces a new type
> representing a span of time instead of using core::time::Duration or
> time::Ktime.
> 
> 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.

This patchset is > 95% time handling, and only a small part
networking. So i'm not sure netdev is the correct subsystem to merge
this.

I can give an Acked-by for the last patch, and i don't expect any
merge conflicts.

    Andrew

---
pw-bot: cr

Re: [PATCH net-next v3 0/8] rust: Add IO polling

[FUJITA Tomonori]

On Fri, 18 Oct 2024 16:26:54 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Wed, Oct 16, 2024 at 12:52:05PM +0900, FUJITA Tomonori wrote:
>> polls 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.
>> 
>> As a result of the past discussion, this introduces a new type
>> representing a span of time instead of using core::time::Duration or
>> time::Ktime.
>> 
>> 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.
> 
> This patchset is > 95% time handling, and only a small part
> networking. So i'm not sure netdev is the correct subsystem to merge
> this.

The time handling code became much bigger than I expected.

I'll send the next version for the tip tree.

TIME-KEEPING/TIMERS maintainers, would you prefer this to go through
the rust tree?