[PATCH v1 0/2] rust: time: Add fsleep()

[PATCH v1 0/2] rust: time: Add fsleep()

[FUJITA Tomonori]

Add wrapper for fsleep() function.

The first patch renames from the Delta's methods as_micros_ceil() and
as_millis() to into_micros_ceil() and into_millis() respectively to
maintain consistency with the other function names. I think that the
commit 2ed94606a0fe ("rust: time: Rename Delta's methods from as_* to
into_*"), wasn't applied as expected, due to the conflict with the
commit 1b7bbd597527 ("rust: time: Avoid 64-bit integer division on
32-bit architectures").

The second patch is effectively a resend of a previous patch in the
iopoll patchset. The last post was:

[PATCH v15 5/6] rust: time: Add wrapper for fsleep() function

https://lore.kernel.org/lkml/20250423192857.199712-6-fujita.tomonori@gmail.com/

This patch has already been reviewed by many developers. It was not
merged previously due to issues with 64-bit division on 32-bit
architectures. A patch addressing the division issue has already been
merged, so this patch can now be merged as well.


FUJITA Tomonori (2):
  rust: time: Rename Delta's as_micros_ceil and as_millis
  rust: time: Add wrapper for fsleep() function

 rust/helpers/time.c       |  6 +++++
 rust/kernel/time.rs       |  5 ++--
 rust/kernel/time/delay.rs | 49 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 2 deletions(-)
 create mode 100644 rust/kernel/time/delay.rs


base-commit: 994393295c89711531583f6de8f296a30b0d944a
-- 
2.43.0

[PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[FUJITA Tomonori]

Rename the Delta struct's methods as_micros_ceil() and as_millis() to
into_micros_ceil() and into_millis() respectively, for consistency
with the naming of other methods.

Fix the commit 2ed94606a0fe ("rust: time: Rename Delta's methods from
as_* to into_*"), wasn't applied as expected, due to the conflict with
the commit 1b7bbd597527 ("rust: time: Avoid 64-bit integer division on
32-bit architectures").

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

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index eaa6d9ab5737..445877039395 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -284,7 +284,7 @@ pub const fn into_nanos(self) -> i64 {
     /// Return the smallest number of microseconds greater than or equal
     /// to the value in the [`Delta`].
     #[inline]
-    pub fn as_micros_ceil(self) -> i64 {
+    pub fn into_micros_ceil(self) -> i64 {
         #[cfg(CONFIG_64BIT)]
         {
             self.into_nanos().saturating_add(NSEC_PER_USEC - 1) / NSEC_PER_USEC
@@ -299,7 +299,7 @@ pub fn as_micros_ceil(self) -> i64 {
 
     /// Return the number of milliseconds in the [`Delta`].
     #[inline]
-    pub fn as_millis(self) -> i64 {
+    pub fn into_millis(self) -> i64 {
         #[cfg(CONFIG_64BIT)]
         {
             self.into_nanos() / NSEC_PER_MSEC
-- 
2.43.0

[PATCH v1 2/2] 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.

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

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

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

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[Alice Ryhl]

On Tue, Jun 17, 2025 at 11:41:54PM +0900, FUJITA Tomonori wrote:
> Rename the Delta struct's methods as_micros_ceil() and as_millis() to
> into_micros_ceil() and into_millis() respectively, for consistency
> with the naming of other methods.
> 
> Fix the commit 2ed94606a0fe ("rust: time: Rename Delta's methods from
> as_* to into_*"), wasn't applied as expected, due to the conflict with
> the commit 1b7bbd597527 ("rust: time: Avoid 64-bit integer division on
> 32-bit architectures").
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Why are we renaming them? The stdlib always uses as_* or to_* for copy
types. In my mind, into_* means that you want to emphasize that you are
performing a transformation that consumes self and transfers ownership
of some resource in the process.

See the api guidelines:
https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

Alice

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[Miguel Ojeda]

On Wed, Jun 18, 2025 at 10:05 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> Why are we renaming them? The stdlib always uses as_* or to_* for copy
> types. In my mind, into_* means that you want to emphasize that you are
> performing a transformation that consumes self and transfers ownership
> of some resource in the process.
>
> See the api guidelines:
> https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv

We may be going in circles here... I think the confusion is all on
what to do for "owned -> owned" `Copy` non-expensive conversions.

I think Tomo sent a patch to change the `as_` and `is_` methods to
take `&self` to be consistent with the guidelines, since they say
there is no "owned -> owned" case for `as_`. Then you mentioned that
`self` is OK and Andreas agreed, and I guess Tomo ended up with
`into_` since `to_` is only for the expensive case, even though it is
not meant for `Copy` types.

In other words, either we say in the kernel we are OK with `as_` for
"owned -> owned" too, or we take `&self`.

Did I get that right, everyone?

Thanks!

Cheers,
Miguel

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[Alice Ryhl]

On Wed, Jun 18, 2025 at 11:29:26AM +0200, Miguel Ojeda wrote:
> On Wed, Jun 18, 2025 at 10:05 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > Why are we renaming them? The stdlib always uses as_* or to_* for copy
> > types. In my mind, into_* means that you want to emphasize that you are
> > performing a transformation that consumes self and transfers ownership
> > of some resource in the process.
> >
> > See the api guidelines:
> > https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv
> 
> We may be going in circles here... I think the confusion is all on
> what to do for "owned -> owned" `Copy` non-expensive conversions.
> 
> I think Tomo sent a patch to change the `as_` and `is_` methods to
> take `&self` to be consistent with the guidelines, since they say
> there is no "owned -> owned" case for `as_`. Then you mentioned that
> `self` is OK and Andreas agreed, and I guess Tomo ended up with
> `into_` since `to_` is only for the expensive case, even though it is
> not meant for `Copy` types.
> 
> In other words, either we say in the kernel we are OK with `as_` for
> "owned -> owned" too, or we take `&self`.
> 
> Did I get that right, everyone?

Yeah I think using as_* naming for cases other than borrowed->borrowed
is relatively common for Copy types. Looking at the stdlib, the rules
I'm seeing are more or less these:

First, if the method is expensive the naming is to_* or a verb without a
prefix. We only use into_* for expensive operations if the call also
transfers ownership of some resource. Example: CStr::to_bytes()

Otherwise, if the method is something that looks inside the type (i.e.
it peels away a layer of abstraction), then we using as_* naming. Or we
might use a noun with no prefix if we want it to feel like a field
access. Example: Duration::as_millis()

On the other hand, if the method transforms the value while staying at
the same layer of abstraction, then we may call the method to_* (or just
use a verb without a prefix). Example: f64::to_radians()

Alice

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[Miguel Ojeda]

On Wed, Jun 18, 2025 at 11:52 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> Yeah I think using as_* naming for cases other than borrowed->borrowed
> is relatively common for Copy types.

Hmm... from a quick look, I only see a few: on raw pointers, `NonNull`
and `Component`. But maybe I am grepping wrong.

There are a bunch on unstable and private stuff, e.g.
`ptr::Alignment`, so it may become a bit more common over time.

So far it seems most of them take `&self`, which seems to align with
the guidelines.

Either way, I agree that what is important is that it is a "free"
conversion/access/...

Cheers,
Miguel

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[Alice Ryhl]

On Wed, Jun 18, 2025 at 1:03 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Wed, Jun 18, 2025 at 11:52 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >
> > Yeah I think using as_* naming for cases other than borrowed->borrowed
> > is relatively common for Copy types.
>
> Hmm... from a quick look, I only see a few: on raw pointers, `NonNull`
> and `Component`. But maybe I am grepping wrong.
>
> There are a bunch on unstable and private stuff, e.g.
> `ptr::Alignment`, so it may become a bit more common over time.
>
> So far it seems most of them take `&self`, which seems to align with
> the guidelines.
>
> Either way, I agree that what is important is that it is a "free"
> conversion/access/...

There are also methods such as Duration::as_millis(). Yes, those take
&self but &self is equivalent to self for Copy types, so there is no
difference. And even if we did treat them differently,
Duration::as_millis() is actually borrowed->owned as the return type
is not a reference, so ...

Alice

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[Miguel Ojeda]

On Wed, Jun 18, 2025 at 3:17 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> There are also methods such as Duration::as_millis(). Yes, those take
> &self but &self is equivalent to self for Copy types, so there is no
> difference. And even if we did treat them differently,
> Duration::as_millis() is actually borrowed->owned as the return type
> is not a reference, so ...

In most cases it may not matter, but even if taking either was exactly
the same, the point of the discussion(s) was what is more idiomatic,
i.e. how to spell those signatures.

I understand you are saying that `Duration::as_millis()` is already a
stable example from the standard library of something that is not
borrowed -> borrowed, and thus the guidelines should be understood as
implying it is fine either way. It is still confusing, as shown by
these repeated discussions, and on the parameter's side of things,
they still seem to prefer `&self`, including in the equivalent methods
of this patch.

Personally, I would use `self`, and clarify the guidelines.

Cheers,
Miguel

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[FUJITA Tomonori]

On Wed, 18 Jun 2025 17:47:27 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Wed, Jun 18, 2025 at 3:17 PM Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> There are also methods such as Duration::as_millis(). Yes, those take
>> &self but &self is equivalent to self for Copy types, so there is no
>> difference. And even if we did treat them differently,
>> Duration::as_millis() is actually borrowed->owned as the return type
>> is not a reference, so ...
> 
> In most cases it may not matter, but even if taking either was exactly
> the same, the point of the discussion(s) was what is more idiomatic,
> i.e. how to spell those signatures.
> 
> I understand you are saying that `Duration::as_millis()` is already a
> stable example from the standard library of something that is not
> borrowed -> borrowed, and thus the guidelines should be understood as
> implying it is fine either way. It is still confusing, as shown by
> these repeated discussions, and on the parameter's side of things,
> they still seem to prefer `&self`, including in the equivalent methods
> of this patch.
> 
> Personally, I would use `self`, and clarify the guidelines.

So would the function be defined like this?

fn as_nanos(self) -> i64;

If that's the case, then we've come full circle back to the original
problem; Clippy warns against using as_* names for trait methods that
take self as follows:

warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
   --> /home/fujita/git/linux-rust/rust/kernel/time/hrtimer.rs:430:17
    |
430 |     fn as_nanos(self) -> i64;
    |                 ^^^^
    |
    = help: consider choosing a less ambiguous name
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
    = note: `-W clippy::wrong-self-convention` implied by `-W clippy::all`
    = help: to override `-W clippy::all` add `#[allow(clippy::wrong_self_convention)]`

https://lore.kernel.org/rust-for-linux/20250610132823.3457263-2-fujita.tomonori@gmail.com/


HrTimerExpires trait needs as_nanos() method and Instant and Delta
need to implement HrTimerExpires trait.

We need a consistent definition of as_nanos() across the
HrTimerExpires trait, and the Instant and Delta structures.

And it would be better if the definition of as_nanos were consistent
with the other as_* methods.

It looks like the conversion from Delta to i64 doesn’t quite fit any
of the categories in the API guidelines. How should it be defined?

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[Miguel Ojeda]

On Thu, Jun 19, 2025 at 9:08 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> So would the function be defined like this?
>
> fn as_nanos(self) -> i64;
>
> If that's the case, then we've come full circle back to the original
> problem; Clippy warns against using as_* names for trait methods that
> take self as follows:
>
> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference

Yeah, the Clippy warning is indeed one more data point that the
guidelines are confusing to the point of having Clippy complain or,
more likely, the guidelines' intention is that we should just pick
`&self`.

If we decide to be OK with `self`s in the kernel for these cases, we
can simply disable the lint. Doing so means we lose the rest of the
checking for that lint, sadly.

And, yeah, we are indeed going in circles.

What I would normally suggest for cases like this is answering: what
would be the best for the kernel's particular case, regardless of
existing guidelines/lints? Then, if we think it is better to be
different, and there is enough justification to do so, then try to
mitigate the lose of the lints, talk to upstream, write our own
variation of the guidelines, etc.

So I would like to hear if anybody feels strongly about either
direction, i.e. any other pros/cons that we haven't thought of.

Thanks!

Cheers,
Miguel

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[Andreas Hindborg]

Hi Fujita,

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

> Rename the Delta struct's methods as_micros_ceil() and as_millis() to
> into_micros_ceil() and into_millis() respectively, for consistency
> with the naming of other methods.
>
> Fix the commit 2ed94606a0fe ("rust: time: Rename Delta's methods from
> as_* to into_*"), wasn't applied as expected, due to the conflict with
> the commit 1b7bbd597527 ("rust: time: Avoid 64-bit integer division on
> 32-bit architectures").
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

Sorry for messing up your patch. Since we have no rules against rebasing
rust-timekeeping and the PR is a few weeks down the road, I will just go
back and fix the original patch. Keep commit history clean.

Best regards,
Andreas Hindborg

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[Andreas Hindborg]

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Thu, Jun 19, 2025 at 9:08 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> So would the function be defined like this?
>>
>> fn as_nanos(self) -> i64;
>>
>> If that's the case, then we've come full circle back to the original
>> problem; Clippy warns against using as_* names for trait methods that
>> take self as follows:
>>
>> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
>
> Yeah, the Clippy warning is indeed one more data point that the
> guidelines are confusing to the point of having Clippy complain or,
> more likely, the guidelines' intention is that we should just pick
> `&self`.
>
> If we decide to be OK with `self`s in the kernel for these cases, we
> can simply disable the lint. Doing so means we lose the rest of the
> checking for that lint, sadly.
>
> And, yeah, we are indeed going in circles.
>
> What I would normally suggest for cases like this is answering: what
> would be the best for the kernel's particular case, regardless of
> existing guidelines/lints? Then, if we think it is better to be
> different, and there is enough justification to do so, then try to
> mitigate the lose of the lints, talk to upstream, write our own
> variation of the guidelines, etc.
>
> So I would like to hear if anybody feels strongly about either
> direction, i.e. any other pros/cons that we haven't thought of.

The table at [1] seems to suggest `to_*` or `into_*` being the right
prefix for this situation. It does not fully match `to_*`, as the
conversion is not expensive. It does not match `into_*` as the type is
`Copy`.

I am leaning towards `to_*`, but no strong feelings against `into_*`.

I would not go with `as_*`, I would expect that to borrow.


Best regards,
Andreas Hindborg

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[FUJITA Tomonori]

On Thu, 19 Jun 2025 11:12:00 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

>> Rename the Delta struct's methods as_micros_ceil() and as_millis() to
>> into_micros_ceil() and into_millis() respectively, for consistency
>> with the naming of other methods.
>>
>> Fix the commit 2ed94606a0fe ("rust: time: Rename Delta's methods from
>> as_* to into_*"), wasn't applied as expected, due to the conflict with
>> the commit 1b7bbd597527 ("rust: time: Avoid 64-bit integer division on
>> 32-bit architectures").
>>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> Sorry for messing up your patch. Since we have no rules against rebasing
> rust-timekeeping and the PR is a few weeks down the road, I will just go
> back and fix the original patch. Keep commit history clean.

I think it would be easier to resolve conflicts by applying the typed
clock sources patchset and the patchset to convert hrtimer first, and
then applying the division patch.

Then please ignore this and apply only the second patch in this
patchset.

Let me know if you need any help.


Thanks!

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[Miguel Ojeda]

On Thu, Jun 19, 2025 at 11:28 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> The table at [1] seems to suggest `to_*` or `into_*` being the right
> prefix for this situation. It does not fully match `to_*`, as the
> conversion is not expensive. It does not match `into_*` as the type is
> `Copy`.
>
> I am leaning towards `to_*`, but no strong feelings against `into_*`.
>
> I would not go with `as_*`, I would expect that to borrow.

It is an integer division by compile-time constant, so likely just a
multiplication and some adjustment, so it depends on whether we
consider that "expensive".

However, even if we consider that "expensive", we will still have the
same question when we have a really cheap method.

The root issue is that the table just doesn't say what to do in some
of the "free" cases, and it is generally confusing.

Since I am asking for opinions: why do you consider `as_*` as
expecting to borrow? The standard does take `&self` the majority of
the time (but not always), and Clippy also expects a borrow, but you
also said in a previous iteration that you don't want to take a
pointer just to pass an integer, which makes sense: we wouldn't pass a
reference if we were using the integer.

Thanks!

(I am tempted to propose a new table...)

Cheers,
Miguel

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[Andreas Hindborg]

"Miguel Ojeda" <miguel.ojeda.sandonis@gmail.com> writes:

> On Thu, Jun 19, 2025 at 11:28 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> The table at [1] seems to suggest `to_*` or `into_*` being the right
>> prefix for this situation. It does not fully match `to_*`, as the
>> conversion is not expensive. It does not match `into_*` as the type is
>> `Copy`.
>>
>> I am leaning towards `to_*`, but no strong feelings against `into_*`.
>>
>> I would not go with `as_*`, I would expect that to borrow.
>
> It is an integer division by compile-time constant, so likely just a
> multiplication and some adjustment, so it depends on whether we
> consider that "expensive".
>
> However, even if we consider that "expensive", we will still have the
> same question when we have a really cheap method.
>
> The root issue is that the table just doesn't say what to do in some
> of the "free" cases, and it is generally confusing.
>
> Since I am asking for opinions: why do you consider `as_*` as
> expecting to borrow?

1) I **feel** that is usually the case. I did not check `std` if this
also the case in practice.
2) The table at [1] says `as_*` is borrowed -> borrowed.
3) To me, the wording "as" indicates a view into something.

> The standard does take `&self` the majority of
> the time (but not always), and Clippy also expects a borrow, but you
> also said in a previous iteration that you don't want to take a
> pointer just to pass an integer, which makes sense: we wouldn't pass a
> reference if we were using the integer.

Yes, I would prefer taking by value. I think Alice mentioned earlier in
this thread that the compiler will be smart about this and just pass the
value. But it still feels wrong to me.


Best regards,
Andreas Hindborg

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[Miguel Ojeda]

On Thu, Jun 19, 2025 at 2:51 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Yes, I would prefer taking by value. I think Alice mentioned earlier in
> this thread that the compiler will be smart about this and just pass the
> value. But it still feels wrong to me.

If inlined/private, yes; but not always.

So for small ("free") functions like this, it should generally not
matter, since they should be inlined whether by manual marking or by
the compiler.

But, in general, it is not the same, and you can see cases where the
compiler will still pass a pointer, and thus dereferences and writes
to memory to take an address to pass it.

Which means that, outside small things like `as_*`, one should still
probably take by value. Which creates an inconsistency.

Cheers,
Miguel

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[Alice Ryhl]

On Thu, Jun 19, 2025 at 8:08 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Wed, 18 Jun 2025 17:47:27 +0200
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>
> > On Wed, Jun 18, 2025 at 3:17 PM Alice Ryhl <aliceryhl@google.com> wrote:
> >>
> >> There are also methods such as Duration::as_millis(). Yes, those take
> >> &self but &self is equivalent to self for Copy types, so there is no
> >> difference. And even if we did treat them differently,
> >> Duration::as_millis() is actually borrowed->owned as the return type
> >> is not a reference, so ...
> >
> > In most cases it may not matter, but even if taking either was exactly
> > the same, the point of the discussion(s) was what is more idiomatic,
> > i.e. how to spell those signatures.
> >
> > I understand you are saying that `Duration::as_millis()` is already a
> > stable example from the standard library of something that is not
> > borrowed -> borrowed, and thus the guidelines should be understood as
> > implying it is fine either way. It is still confusing, as shown by
> > these repeated discussions, and on the parameter's side of things,
> > they still seem to prefer `&self`, including in the equivalent methods
> > of this patch.
> >
> > Personally, I would use `self`, and clarify the guidelines.
>
> So would the function be defined like this?
>
> fn as_nanos(self) -> i64;
>
> If that's the case, then we've come full circle back to the original
> problem; Clippy warns against using as_* names for trait methods that
> take self as follows:
>
> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
>    --> /home/fujita/git/linux-rust/rust/kernel/time/hrtimer.rs:430:17
>     |
> 430 |     fn as_nanos(self) -> i64;
>     |                 ^^^^
>     |
>     = help: consider choosing a less ambiguous name
>     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
>     = note: `-W clippy::wrong-self-convention` implied by `-W clippy::all`
>     = help: to override `-W clippy::all` add `#[allow(clippy::wrong_self_convention)]`
>
> https://lore.kernel.org/rust-for-linux/20250610132823.3457263-2-fujita.tomonori@gmail.com/

Are we missing a derive(Copy) for this type? Clippy does not emit that
lint if the type is Copy:
https://github.com/rust-lang/rust-clippy/issues/273

Alice

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[FUJITA Tomonori]

On Tue, 24 Jun 2025 13:15:32 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

>> So would the function be defined like this?
>>
>> fn as_nanos(self) -> i64;
>>
>> If that's the case, then we've come full circle back to the original
>> problem; Clippy warns against using as_* names for trait methods that
>> take self as follows:
>>
>> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
>>    --> /home/fujita/git/linux-rust/rust/kernel/time/hrtimer.rs:430:17
>>     |
>> 430 |     fn as_nanos(self) -> i64;
>>     |                 ^^^^
>>     |
>>     = help: consider choosing a less ambiguous name
>>     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
>>     = note: `-W clippy::wrong-self-convention` implied by `-W clippy::all`
>>     = help: to override `-W clippy::all` add `#[allow(clippy::wrong_self_convention)]`
>>
>> https://lore.kernel.org/rust-for-linux/20250610132823.3457263-2-fujita.tomonori@gmail.com/
> 
> Are we missing a derive(Copy) for this type? Clippy does not emit that
> lint if the type is Copy:
> https://github.com/rust-lang/rust-clippy/issues/273

I think that both Delta and Instant structs implement Copy.

#[repr(transparent)]
#[derive(PartialEq, PartialOrd, Eq, Ord)]
pub struct Instant<C: ClockSource> {
    inner: bindings::ktime_t,
    _c: PhantomData<C>,
}

impl<C: ClockSource> Clone for Instant<C> {
    fn clone(&self) -> Self {
        *self
    }
}

impl<C: ClockSource> Copy for Instant<C> {}

#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
pub struct Delta {
    nanos: i64,
}


The above warning is about the trait method.

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[Alice Ryhl]

On Tue, Jun 24, 2025 at 2:49 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Tue, 24 Jun 2025 13:15:32 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> >> So would the function be defined like this?
> >>
> >> fn as_nanos(self) -> i64;
> >>
> >> If that's the case, then we've come full circle back to the original
> >> problem; Clippy warns against using as_* names for trait methods that
> >> take self as follows:
> >>
> >> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
> >>    --> /home/fujita/git/linux-rust/rust/kernel/time/hrtimer.rs:430:17
> >>     |
> >> 430 |     fn as_nanos(self) -> i64;
> >>     |                 ^^^^
> >>     |
> >>     = help: consider choosing a less ambiguous name
> >>     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
> >>     = note: `-W clippy::wrong-self-convention` implied by `-W clippy::all`
> >>     = help: to override `-W clippy::all` add `#[allow(clippy::wrong_self_convention)]`
> >>
> >> https://lore.kernel.org/rust-for-linux/20250610132823.3457263-2-fujita.tomonori@gmail.com/
> >
> > Are we missing a derive(Copy) for this type? Clippy does not emit that
> > lint if the type is Copy:
> > https://github.com/rust-lang/rust-clippy/issues/273
>
> I think that both Delta and Instant structs implement Copy.
>
> #[repr(transparent)]
> #[derive(PartialEq, PartialOrd, Eq, Ord)]
> pub struct Instant<C: ClockSource> {
>     inner: bindings::ktime_t,
>     _c: PhantomData<C>,
> }
>
> impl<C: ClockSource> Clone for Instant<C> {
>     fn clone(&self) -> Self {
>         *self
>     }
> }
>
> impl<C: ClockSource> Copy for Instant<C> {}
>
> #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
> pub struct Delta {
>     nanos: i64,
> }
>
> The above warning is about the trait method.

Wait, it's a trait method!?

Alice

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[FUJITA Tomonori]

On Tue, 24 Jun 2025 14:54:24 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

>> >> So would the function be defined like this?
>> >>
>> >> fn as_nanos(self) -> i64;
>> >>
>> >> If that's the case, then we've come full circle back to the original
>> >> problem; Clippy warns against using as_* names for trait methods that
>> >> take self as follows:
>> >>
>> >> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
>> >>    --> /home/fujita/git/linux-rust/rust/kernel/time/hrtimer.rs:430:17
>> >>     |
>> >> 430 |     fn as_nanos(self) -> i64;
>> >>     |                 ^^^^
>> >>     |
>> >>     = help: consider choosing a less ambiguous name
>> >>     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
>> >>     = note: `-W clippy::wrong-self-convention` implied by `-W clippy::all`
>> >>     = help: to override `-W clippy::all` add `#[allow(clippy::wrong_self_convention)]`
>> >>
>> >> https://lore.kernel.org/rust-for-linux/20250610132823.3457263-2-fujita.tomonori@gmail.com/
>> >
>> > Are we missing a derive(Copy) for this type? Clippy does not emit that
>> > lint if the type is Copy:
>> > https://github.com/rust-lang/rust-clippy/issues/273
>>
>> I think that both Delta and Instant structs implement Copy.
>>
>> #[repr(transparent)]
>> #[derive(PartialEq, PartialOrd, Eq, Ord)]
>> pub struct Instant<C: ClockSource> {
>>     inner: bindings::ktime_t,
>>     _c: PhantomData<C>,
>> }
>>
>> impl<C: ClockSource> Clone for Instant<C> {
>>     fn clone(&self) -> Self {
>>         *self
>>     }
>> }
>>
>> impl<C: ClockSource> Copy for Instant<C> {}
>>
>> #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
>> pub struct Delta {
>>     nanos: i64,
>> }
>>
>> The above warning is about the trait method.
> 
> Wait, it's a trait method!?

Yes. Clippy warns the following implementation:

pub trait HrTimerExpires {
    fn as_nanos(self) -> i64;
}

Clippy doesn't warn when the methods on Delta and Instant are written
similarly. So Clippy is happy about the followings:

pub trait HrTimerExpires {
    fn as_nanos(&self) -> i64;
}

impl HrTimerExpires for Delta {
    fn as_nanos(&self) -> i64 {
        Delta::as_nanos(*self)
    }
}

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[Alice Ryhl]

On Tue, Jun 24, 2025 at 3:14 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Tue, 24 Jun 2025 14:54:24 +0100
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> >> >> So would the function be defined like this?
> >> >>
> >> >> fn as_nanos(self) -> i64;
> >> >>
> >> >> If that's the case, then we've come full circle back to the original
> >> >> problem; Clippy warns against using as_* names for trait methods that
> >> >> take self as follows:
> >> >>
> >> >> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
> >> >>    --> /home/fujita/git/linux-rust/rust/kernel/time/hrtimer.rs:430:17
> >> >>     |
> >> >> 430 |     fn as_nanos(self) -> i64;
> >> >>     |                 ^^^^
> >> >>     |
> >> >>     = help: consider choosing a less ambiguous name
> >> >>     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
> >> >>     = note: `-W clippy::wrong-self-convention` implied by `-W clippy::all`
> >> >>     = help: to override `-W clippy::all` add `#[allow(clippy::wrong_self_convention)]`
> >> >>
> >> >> https://lore.kernel.org/rust-for-linux/20250610132823.3457263-2-fujita.tomonori@gmail.com/
> >> >
> >> > Are we missing a derive(Copy) for this type? Clippy does not emit that
> >> > lint if the type is Copy:
> >> > https://github.com/rust-lang/rust-clippy/issues/273
> >>
> >> I think that both Delta and Instant structs implement Copy.
> >>
> >> #[repr(transparent)]
> >> #[derive(PartialEq, PartialOrd, Eq, Ord)]
> >> pub struct Instant<C: ClockSource> {
> >>     inner: bindings::ktime_t,
> >>     _c: PhantomData<C>,
> >> }
> >>
> >> impl<C: ClockSource> Clone for Instant<C> {
> >>     fn clone(&self) -> Self {
> >>         *self
> >>     }
> >> }
> >>
> >> impl<C: ClockSource> Copy for Instant<C> {}
> >>
> >> #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
> >> pub struct Delta {
> >>     nanos: i64,
> >> }
> >>
> >> The above warning is about the trait method.
> >
> > Wait, it's a trait method!?
>
> Yes. Clippy warns the following implementation:
>
> pub trait HrTimerExpires {
>     fn as_nanos(self) -> i64;
> }
>
> Clippy doesn't warn when the methods on Delta and Instant are written
> similarly. So Clippy is happy about the followings:
>
> pub trait HrTimerExpires {
>     fn as_nanos(&self) -> i64;
> }
>
> impl HrTimerExpires for Delta {
>     fn as_nanos(&self) -> i64 {
>         Delta::as_nanos(*self)
>     }
> }

If it's a trait method, then it should take &self because you don't
know whether it's Copy.

Alice

Re: [PATCH v1 1/2] rust: time: Rename Delta's methods as_micros_ceil and as_millis

[FUJITA Tomonori]

On Tue, 24 Jun 2025 15:45:45 +0100
Alice Ryhl <aliceryhl@google.com> wrote:

> On Tue, Jun 24, 2025 at 3:14 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> On Tue, 24 Jun 2025 14:54:24 +0100
>> Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> >> >> So would the function be defined like this?
>> >> >>
>> >> >> fn as_nanos(self) -> i64;
>> >> >>
>> >> >> If that's the case, then we've come full circle back to the original
>> >> >> problem; Clippy warns against using as_* names for trait methods that
>> >> >> take self as follows:
>> >> >>
>> >> >> warning: methods called `as_*` usually take `self` by reference or `self` by mutable reference
>> >> >>    --> /home/fujita/git/linux-rust/rust/kernel/time/hrtimer.rs:430:17
>> >> >>     |
>> >> >> 430 |     fn as_nanos(self) -> i64;
>> >> >>     |                 ^^^^
>> >> >>     |
>> >> >>     = help: consider choosing a less ambiguous name
>> >> >>     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
>> >> >>     = note: `-W clippy::wrong-self-convention` implied by `-W clippy::all`
>> >> >>     = help: to override `-W clippy::all` add `#[allow(clippy::wrong_self_convention)]`
>> >> >>
>> >> >> https://lore.kernel.org/rust-for-linux/20250610132823.3457263-2-fujita.tomonori@gmail.com/
>> >> >
>> >> > Are we missing a derive(Copy) for this type? Clippy does not emit that
>> >> > lint if the type is Copy:
>> >> > https://github.com/rust-lang/rust-clippy/issues/273
>> >>
>> >> I think that both Delta and Instant structs implement Copy.
>> >>
>> >> #[repr(transparent)]
>> >> #[derive(PartialEq, PartialOrd, Eq, Ord)]
>> >> pub struct Instant<C: ClockSource> {
>> >>     inner: bindings::ktime_t,
>> >>     _c: PhantomData<C>,
>> >> }
>> >>
>> >> impl<C: ClockSource> Clone for Instant<C> {
>> >>     fn clone(&self) -> Self {
>> >>         *self
>> >>     }
>> >> }
>> >>
>> >> impl<C: ClockSource> Copy for Instant<C> {}
>> >>
>> >> #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Debug)]
>> >> pub struct Delta {
>> >>     nanos: i64,
>> >> }
>> >>
>> >> The above warning is about the trait method.
>> >
>> > Wait, it's a trait method!?
>>
>> Yes. Clippy warns the following implementation:
>>
>> pub trait HrTimerExpires {
>>     fn as_nanos(self) -> i64;
>> }
>>
>> Clippy doesn't warn when the methods on Delta and Instant are written
>> similarly. So Clippy is happy about the followings:
>>
>> pub trait HrTimerExpires {
>>     fn as_nanos(&self) -> i64;
>> }
>>
>> impl HrTimerExpires for Delta {
>>     fn as_nanos(&self) -> i64 {
>>         Delta::as_nanos(*self)
>>     }
>> }
> 
> If it's a trait method, then it should take &self because you don't
> know whether it's Copy.

A trait method as_* should take &self. The same name of a method in a
structure which implements that trait can take self (if the structure
implements Copy and the cost is free) instead of &self, although it
doesn't match the same name of the trait method.

Also, the conversions table says that the ownership of as_* is
borrowed -> borrowed so neither of them matches the table.

Right?

Re: [PATCH v1 2/2] rust: time: Add wrapper for fsleep() function

[Andreas Hindborg]

On Tue, 17 Jun 2025 23:41:55 +0900, FUJITA Tomonori wrote:
> Add a wrapper for fsleep(), flexible sleep functions in
> include/linux/delay.h which typically deals with hardware delays.
> 
> The kernel supports several sleep functions to handle various lengths
> of delay. This adds fsleep(), automatically chooses the best sleep
> method based on a duration.
> 
> [...]

Applied, thanks!

[2/2] rust: time: Add wrapper for fsleep() function
      commit: d4b29ddf82a458935f1bd4909b8a7a13df9d3bdc

Best regards,
-- 
Andreas Hindborg <a.hindborg@kernel.org>