[PATCH net-next v1 0/2] add delay abstraction (sleep functions)

[PATCH net-next v1 0/2] add delay abstraction (sleep functions)

[FUJITA Tomonori]

Add an abstraction for sleep functions in `include/linux/delay.h` for
dealing with hardware delays. `delay.h` supports sleep and delay (busy
wait). This adds support for sleep functions used by QT2025 PHY driver
to sleep until a PHY becomes ready.

The old rust branch has the delay abstraction which supports msleep()
with a helper function which rounds a `Duration` up to the nearest
milliseconds.

This adds fsleep() support instead of msleep(). fsleep() can handle
various lengths of delay by internally calling an appropriate sleep
function including msleep().


FUJITA Tomonori (2):
  rust: add delay abstraction
  net: phy: qt2025: wait until PHY becomes ready

 drivers/net/phy/qt2025.rs       | 11 +++++++++--
 rust/bindings/bindings_helper.h |  1 +
 rust/helpers/delay.c            |  8 ++++++++
 rust/helpers/helpers.c          |  1 +
 rust/kernel/delay.rs            | 18 ++++++++++++++++++
 rust/kernel/lib.rs              |  1 +
 6 files changed, 38 insertions(+), 2 deletions(-)
 create mode 100644 rust/helpers/delay.c
 create mode 100644 rust/kernel/delay.rs


base-commit: c824deb1a89755f70156b5cdaf569fca80698719
-- 
2.34.1

[PATCH net-next v1 1/2] rust: add delay abstraction

[FUJITA Tomonori]

Add an abstraction for sleep functions in `include/linux/delay.h` for
dealing with hardware delays.

The kernel supports several `sleep` functions for handles various
lengths of delay. This adds fsleep helper function, internally calls
an appropriate sleep function.

This is used by QT2025 PHY driver to wait until a PHY becomes ready.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/bindings/bindings_helper.h |  1 +
 rust/helpers/delay.c            |  8 ++++++++
 rust/helpers/helpers.c          |  1 +
 rust/kernel/delay.rs            | 18 ++++++++++++++++++
 rust/kernel/lib.rs              |  1 +
 5 files changed, 29 insertions(+)
 create mode 100644 rust/helpers/delay.c
 create mode 100644 rust/kernel/delay.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index ae82e9c941af..29a2f59294ba 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -10,6 +10,7 @@
 #include <linux/blk-mq.h>
 #include <linux/blk_types.h>
 #include <linux/blkdev.h>
+#include <linux/delay.h>
 #include <linux/errname.h>
 #include <linux/ethtool.h>
 #include <linux/firmware.h>
diff --git a/rust/helpers/delay.c b/rust/helpers/delay.c
new file mode 100644
index 000000000000..7ae64ad8141d
--- /dev/null
+++ b/rust/helpers/delay.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/helpers/helpers.c b/rust/helpers/helpers.c
index 30f40149f3a9..279ea662ee3b 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 "delay.c"
 #include "kunit.c"
 #include "mutex.c"
 #include "page.c"
diff --git a/rust/kernel/delay.rs b/rust/kernel/delay.rs
new file mode 100644
index 000000000000..79f51a9608b5
--- /dev/null
+++ b/rust/kernel/delay.rs
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Delay and sleep routines.
+//!
+//! C headers: [`include/linux/delay.h`](srctree/include/linux/delay.h).
+
+use core::{ffi::c_ulong, time::Duration};
+
+/// Sleeps for a given duration.
+///
+/// Equivalent to the kernel's [`fsleep`] function, internally calls `udelay`,
+/// `usleep_range`, or `msleep`.
+///
+/// This function can only be used in a nonatomic context.
+pub fn sleep(duration: Duration) {
+    // SAFETY: FFI call.
+    unsafe { bindings::fsleep(duration.as_micros() as c_ulong) }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 22a3bfa5a9e9..c08cb5509c82 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -30,6 +30,7 @@
 #[cfg(CONFIG_BLOCK)]
 pub mod block;
 mod build_assert;
+pub mod delay;
 pub mod device;
 pub mod error;
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
-- 
2.34.1

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[Alice Ryhl]

On Tue, Oct 1, 2024 at 1:27 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Add an abstraction for sleep functions in `include/linux/delay.h` for
> dealing with hardware delays.
>
> The kernel supports several `sleep` functions for handles various
> lengths of delay. This adds fsleep helper function, internally calls
> an appropriate sleep function.
>
> This is used by QT2025 PHY driver to wait until a PHY becomes ready.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/bindings/bindings_helper.h |  1 +
>  rust/helpers/delay.c            |  8 ++++++++
>  rust/helpers/helpers.c          |  1 +
>  rust/kernel/delay.rs            | 18 ++++++++++++++++++
>  rust/kernel/lib.rs              |  1 +

There is already a rust/kernel/time.rs file that this can go in.

> +/// Sleeps for a given duration.
> +///
> +/// Equivalent to the kernel's [`fsleep`] function, internally calls `udelay`,
> +/// `usleep_range`, or `msleep`.
> +///
> +/// This function can only be used in a nonatomic context.
> +pub fn sleep(duration: Duration) {
> +    // SAFETY: FFI call.
> +    unsafe { bindings::fsleep(duration.as_micros() as c_ulong) }
> +}

We should probably call this `fsleep` to match the C side.

Alice

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[Andrew Lunn]

On Tue, Oct 01, 2024 at 11:25:11AM +0000, FUJITA Tomonori wrote:
> Add an abstraction for sleep functions in `include/linux/delay.h` for
> dealing with hardware delays.
> 
> The kernel supports several `sleep` functions for handles various

s/for/which

> lengths of delay. This adds fsleep helper function, internally calls
> an appropriate sleep function.
> 
> This is used by QT2025 PHY driver to wait until a PHY becomes ready.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/bindings/bindings_helper.h |  1 +
>  rust/helpers/delay.c            |  8 ++++++++
>  rust/helpers/helpers.c          |  1 +
>  rust/kernel/delay.rs            | 18 ++++++++++++++++++
>  rust/kernel/lib.rs              |  1 +
>  5 files changed, 29 insertions(+)
>  create mode 100644 rust/helpers/delay.c
>  create mode 100644 rust/kernel/delay.rs
> 
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index ae82e9c941af..29a2f59294ba 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -10,6 +10,7 @@
>  #include <linux/blk-mq.h>
>  #include <linux/blk_types.h>
>  #include <linux/blkdev.h>
> +#include <linux/delay.h>
>  #include <linux/errname.h>
>  #include <linux/ethtool.h>
>  #include <linux/firmware.h>
> diff --git a/rust/helpers/delay.c b/rust/helpers/delay.c
> new file mode 100644
> index 000000000000..7ae64ad8141d
> --- /dev/null
> +++ b/rust/helpers/delay.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/helpers/helpers.c b/rust/helpers/helpers.c
> index 30f40149f3a9..279ea662ee3b 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 "delay.c"
>  #include "kunit.c"
>  #include "mutex.c"
>  #include "page.c"
> diff --git a/rust/kernel/delay.rs b/rust/kernel/delay.rs
> new file mode 100644
> index 000000000000..79f51a9608b5
> --- /dev/null
> +++ b/rust/kernel/delay.rs
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Delay and sleep routines.
> +//!
> +//! C headers: [`include/linux/delay.h`](srctree/include/linux/delay.h).
> +
> +use core::{ffi::c_ulong, time::Duration};
> +
> +/// Sleeps for a given duration.
> +///
> +/// Equivalent to the kernel's [`fsleep`] function, internally calls `udelay`,
> +/// `usleep_range`, or `msleep`.

Is it possible to cross reference
Documentation/timers/timers-howto.rst ?  fsleep() points to it, so it
would e good if the Rust version also did.

I would also document the units for the parameter. Is it picoseconds
or centuries?

	Andrew

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[Miguel Ojeda]

On Tue, Oct 1, 2024 at 2:31 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Is it possible to cross reference
> Documentation/timers/timers-howto.rst ?  fsleep() points to it, so it
> would e good if the Rust version also did.

We currently use links like:

    //! Reference: <https://docs.kernel.org/core-api/rbtree.html>

Cheers,
Miguel

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[FUJITA Tomonori]

On Tue, 1 Oct 2024 14:31:39 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Tue, Oct 01, 2024 at 11:25:11AM +0000, FUJITA Tomonori wrote:
>> Add an abstraction for sleep functions in `include/linux/delay.h` for
>> dealing with hardware delays.
>> 
>> The kernel supports several `sleep` functions for handles various
> 
> s/for/which

Oops, thanks.

>> +/// Sleeps for a given duration.
>> +///
>> +/// Equivalent to the kernel's [`fsleep`] function, internally calls `udelay`,
>> +/// `usleep_range`, or `msleep`.
> 
> Is it possible to cross reference
> Documentation/timers/timers-howto.rst ?  fsleep() points to it, so it
> would e good if the Rust version also did.
> 
> I would also document the units for the parameter. Is it picoseconds
> or centuries?

Rust's Duration is created from seconds and nanoseconds.

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[Andrew Lunn]

> > I would also document the units for the parameter. Is it picoseconds
> > or centuries?
> 
> Rust's Duration is created from seconds and nanoseconds.

How well know is that? And is there a rust-for-linux wide preference
to use Duration for time? Are we going to get into a situation that
some abstractions use Duration, others seconds, some milliseconds,
etc, just like C code?

Anyway, i would still document the parameter is a Duration, since it
is different to how C fsleep() works.

	Andrew

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[Miguel Ojeda]

On Wed, Oct 2, 2024 at 2:19 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> How well know is that? And is there a rust-for-linux wide preference
> to use Duration for time? Are we going to get into a situation that
> some abstractions use Duration, others seconds, some milliseconds,
> etc, just like C code?

We have `Ktime` that wraps `ktime_t`.

However, note that something like `Ktime` or `Duration` are types, not
a typedef, i.e. it is not an integer where you may confuse the unit.

So, for instance, the implementation here calls `as_micros()` to get
the actual integer. And whoever constructs e.g. a `Duration` has
several constructors to do so, not just the one that takes seconds and
nanoseconds, e.g. there is also `from_millis()`.

Perhaps we may end up needing different abstractions depending on what
we need (Cc'ing Thomas), but we will almost certainly want to still
use types like this, rather than plain integers or typedefs where
units can be confused.

> Anyway, i would still document the parameter is a Duration, since it
> is different to how C fsleep() works.

That is in the signature itself -- so taking into account what I
mentioned above, does it make sense that seeing the type in the
signature would be enough?

Cheers,
Miguel

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[Andrew Lunn]

On Wed, Oct 02, 2024 at 02:35:57PM +0200, Miguel Ojeda wrote:
> On Wed, Oct 2, 2024 at 2:19 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > How well know is that? And is there a rust-for-linux wide preference
> > to use Duration for time? Are we going to get into a situation that
> > some abstractions use Duration, others seconds, some milliseconds,
> > etc, just like C code?
> 
> We have `Ktime` that wraps `ktime_t`.
> 
> However, note that something like `Ktime` or `Duration` are types, not
> a typedef, i.e. it is not an integer where you may confuse the unit.
> 
> So, for instance, the implementation here calls `as_micros()` to get
> the actual integer. And whoever constructs e.g. a `Duration` has
> several constructors to do so, not just the one that takes seconds and
> nanoseconds, e.g. there is also `from_millis()`.
> 
> Perhaps we may end up needing different abstractions depending on what
> we need (Cc'ing Thomas), but we will almost certainly want to still
> use types like this, rather than plain integers or typedefs where
> units can be confused.
> 
> > Anyway, i would still document the parameter is a Duration, since it
> > is different to how C fsleep() works.
> 
> That is in the signature itself -- so taking into account what I
> mentioned above, does it make sense that seeing the type in the
> signature would be enough?

Which is better, the Rust type system catching the error, or not
making the error in the first place because you read the documentation
and it pointed you in the right direction?

Maybe this is my background as a C programmer, with its sloppy type
system, but i prefer to have this very clear, maybe redundantly
stating it in words in addition to the signature.

	Andrew

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[Miguel Ojeda]

On Wed, Oct 2, 2024 at 2:51 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> Which is better, the Rust type system catching the error, or not
> making the error in the first place because you read the documentation
> and it pointed you in the right direction?

The former, because we don't want to duplicate documentation of every
type in every function that uses a type. It does not scale, and it
would be worse for the reader of the docs as soon as you become
familiar with a given type.

Of course, there may be exceptions, but just using a type normally
should not require explaining the type itself in every function.

Also note that in e.g. the rendered docs you can jump to another type
with a single click on top of the name. In some editors, you may be
able to e.g. hover it perhaps.

> Maybe this is my background as a C programmer, with its sloppy type
> system, but i prefer to have this very clear, maybe redundantly
> stating it in words in addition to the signature.

The second part of my message was about the signature point you made,
i.e. not about the units. So I am not sure if you are asking here to
re-state the types of parameters in every function's docs -- what do
you gain from that in common cases?

We also don't repeat every parameter in a bullet list inside the
documentation to explain each parameter, unless a parameter needs it
for a particular reason. In general, the stronger/stricter your
types/signatures are, and the better documented your types are, the
less "extra notes" in every function you need.

For instance, if you see `int` in a signature, then you very likely
need documentation to understand how the argument works because `int`
is way too general (e.g. it is likely it admits cases you are not
supposed to use). However, if you see `Duration`, then you already
know the answer to the units question.

Thus, in a way, we are factoring out documentation to a single place,
thus making them simpler/easier/lighter -- so you can see it as a way
to scale writing docs! :)

That is also why carrying as much information in the signature also
helps with docs, and not just with having the compiler catch mistakes
for us.

I hope this gives some context on why we are approaching it like this.

Cheers,
Miguel

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[Thomas Gleixner]

On Wed, Oct 02 2024 at 15:21, Miguel Ojeda wrote:
> On Wed, Oct 2, 2024 at 2:51 PM Andrew Lunn <andrew@lunn.ch> wrote:
>> Maybe this is my background as a C programmer, with its sloppy type
>> system, but i prefer to have this very clear, maybe redundantly
>> stating it in words in addition to the signature.
>
> The second part of my message was about the signature point you made,
> i.e. not about the units. So I am not sure if you are asking here to
> re-state the types of parameters in every function's docs -- what do
> you gain from that in common cases?
>
> We also don't repeat every parameter in a bullet list inside the
> documentation to explain each parameter, unless a parameter needs it
> for a particular reason. In general, the stronger/stricter your
> types/signatures are, and the better documented your types are, the
> less "extra notes" in every function you need.
>
> For instance, if you see `int` in a signature, then you very likely
> need documentation to understand how the argument works because `int`
> is way too general (e.g. it is likely it admits cases you are not
> supposed to use). However, if you see `Duration`, then you already
> know the answer to the units question.
>
> Thus, in a way, we are factoring out documentation to a single place,
> thus making them simpler/easier/lighter -- so you can see it as a way
> to scale writing docs! :)
>
> That is also why carrying as much information in the signature also
> helps with docs, and not just with having the compiler catch mistakes
> for us.

I completely agree.

'delay(Duration d)' does not need explanation for @d unless there is a
restriction for the valid range of @d. @d is explained in the
documentation of Duration.

That's not any different in C, when the function argument is a pointer
to a complex struct. Nobody would even think about explaining the struct
members in the documentation of the function which has that struct
pointer argument. No, you need to go to the struct declaration to figure
it out.

Redundant documentation is actually bad, because any changes to the type
will inevitably lead to stale documentation at the usage site. The
kernel is full of this.

I havent's seen the actual patches as they were sent to netdev for
whatever reason. There is a larger rework of delay.h going on:

  https://lore.kernel.org/all/20240911-devel-anna-maria-b4-timers-flseep-v2-0-b0d3f33ccfe0@linutronix.de/

V3 will be coming early next week. So please look at V2 if you have any
constraints vs. the rust implementation.

Thanks,

        tglx

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[Alice Ryhl]

On Wed, Oct 2, 2024 at 2:19 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > I would also document the units for the parameter. Is it picoseconds
> > > or centuries?
> >
> > Rust's Duration is created from seconds and nanoseconds.
>
> How well know is that? And is there a rust-for-linux wide preference
> to use Duration for time? Are we going to get into a situation that
> some abstractions use Duration, others seconds, some milliseconds,
> etc, just like C code?
>
> Anyway, i would still document the parameter is a Duration, since it
> is different to how C fsleep() works.

I'm not necessarily convinced we want to use the Rust Duration type.
Similar questions came up when I added the Ktime type. The Rust
Duration type is rather large.

Alice

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[FUJITA Tomonori]

On Wed, 2 Oct 2024 14:37:55 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

> On Wed, Oct 2, 2024 at 2:19 PM Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> > > I would also document the units for the parameter. Is it picoseconds
>> > > or centuries?
>> >
>> > Rust's Duration is created from seconds and nanoseconds.
>>
>> How well know is that? And is there a rust-for-linux wide preference
>> to use Duration for time? Are we going to get into a situation that
>> some abstractions use Duration, others seconds, some milliseconds,
>> etc, just like C code?
>>
>> Anyway, i would still document the parameter is a Duration, since it
>> is different to how C fsleep() works.
> 
> I'm not necessarily convinced we want to use the Rust Duration type.
> Similar questions came up when I added the Ktime type. The Rust
> Duration type is rather large.

core::mem::size_of::<core::time::Duration>() says 16 bytes.

You prefer to add a simpler Duration structure to kernel/time.rs?
Something like:

struct Duration {
    nanos: u64,
}

u64 in nanoseconds is enough for delay in the kernel, I think.


btw, core::time::Duration::new() could panic if a driver does
something stupid; for example,

let d = core::time::Duration::new(u64::MAX, 1_000_000_000);

Is this acceptable?

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[Alice Ryhl]

On Wed, Oct 2, 2024 at 3:58 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Wed, 2 Oct 2024 14:37:55 +0200
> Alice Ryhl <aliceryhl@google.com> wrote:
>
> > On Wed, Oct 2, 2024 at 2:19 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >>
> >> > > I would also document the units for the parameter. Is it picoseconds
> >> > > or centuries?
> >> >
> >> > Rust's Duration is created from seconds and nanoseconds.
> >>
> >> How well know is that? And is there a rust-for-linux wide preference
> >> to use Duration for time? Are we going to get into a situation that
> >> some abstractions use Duration, others seconds, some milliseconds,
> >> etc, just like C code?
> >>
> >> Anyway, i would still document the parameter is a Duration, since it
> >> is different to how C fsleep() works.
> >
> > I'm not necessarily convinced we want to use the Rust Duration type.
> > Similar questions came up when I added the Ktime type. The Rust
> > Duration type is rather large.
>
> core::mem::size_of::<core::time::Duration>() says 16 bytes.
>
> You prefer to add a simpler Duration structure to kernel/time.rs?
> Something like:
>
> struct Duration {
>     nanos: u64,
> }
>
> u64 in nanoseconds is enough for delay in the kernel, I think.

That type already exists. It's called kernel::time::Ktime.

Alice

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[FUJITA Tomonori]

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

>> You prefer to add a simpler Duration structure to kernel/time.rs?
>> Something like:
>>
>> struct Duration {
>>     nanos: u64,
>> }
>>
>> u64 in nanoseconds is enough for delay in the kernel, I think.
> 
> That type already exists. It's called kernel::time::Ktime.

Sure. Some code use ktime_t to represent duration so using Ktime for
the delay functions makes sense. I'll add some methods to Ktime and
use it.

Thanks!

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[Miguel Ojeda]

On Wed, Oct 2, 2024 at 4:40 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Sure. Some code use ktime_t to represent duration so using Ktime for
> the delay functions makes sense. I'll add some methods to Ktime and
> use it.

We really should still use different types to represent points and
deltas, even if internally they happen to end up using/being the
"same" thing.

If we start mixing those two up, then it will be harder to unravel later.

I think Thomas also wanted to have two types, please see this thread:
https://lore.kernel.org/rust-for-linux/87h6vfnh0f.ffs@tglx/ (we also
discussed clocks).

Cheers,
Miguel

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[Thomas Gleixner]

On Wed, Oct 02 2024 at 16:52, Miguel Ojeda wrote:
> On Wed, Oct 2, 2024 at 4:40 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> Sure. Some code use ktime_t to represent duration so using Ktime for
>> the delay functions makes sense. I'll add some methods to Ktime and
>> use it.
>
> We really should still use different types to represent points and
> deltas, even if internally they happen to end up using/being the
> "same" thing.
>
> If we start mixing those two up, then it will be harder to unravel later.
>
> I think Thomas also wanted to have two types, please see this thread:
> https://lore.kernel.org/rust-for-linux/87h6vfnh0f.ffs@tglx/ (we also
> discussed clocks).

Correct. They are distinct.

Btw, why is this sent to netdev and not to LKML? delay is generic code
and has nothing to do with networking.

Thanks,

        tglx

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[FUJITA Tomonori]

On Wed, 02 Oct 2024 21:40:45 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Wed, Oct 02 2024 at 16:52, Miguel Ojeda wrote:
>> On Wed, Oct 2, 2024 at 4:40 PM FUJITA Tomonori
>> <fujita.tomonori@gmail.com> wrote:
>>>
>>> Sure. Some code use ktime_t to represent duration so using Ktime for
>>> the delay functions makes sense. I'll add some methods to Ktime and
>>> use it.
>>
>> We really should still use different types to represent points and
>> deltas, even if internally they happen to end up using/being the
>> "same" thing.
>>
>> If we start mixing those two up, then it will be harder to unravel later.
>>
>> I think Thomas also wanted to have two types, please see this thread:
>> https://lore.kernel.org/rust-for-linux/87h6vfnh0f.ffs@tglx/ (we also
>> discussed clocks).
> 
> Correct. They are distinct.

Understood. I'll add a new type, time::Delta. Any alternative name
suggestions?

> Btw, why is this sent to netdev and not to LKML? delay is generic code
> and has nothing to do with networking.

Rust abstractions are typically merged with their users. I'm trying to
push the delay abstractions with a fix for QT2025 PHY driver
(drivers/net/phy/qt2025.rs), which uses delay.

Sorry, I'll send v2 to timers maintainers too.

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[Miguel Ojeda]

On Thu, Oct 3, 2024 at 3:24 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Rust abstractions are typically merged with their users. I'm trying to
> push the delay abstractions with a fix for QT2025 PHY driver
> (drivers/net/phy/qt2025.rs), which uses delay.

To clarify, in case it helps: users indeed drive the need for
abstractions (i.e. we don't merge abstractions without an expected
user), and it can happen that they go together in the same patch
series for convenience, that is true.

However, I don't think I would say "typically", since most
abstractions went in on their own so far, and each patch still needs
to Cc the relevant maintainers/lists and the respective maintainers
should say they are OK moving it through another tree.

In other words, the "default" is that the abstractions go through
their tree, i.e. delay wouldn't go through netdev, unless the
maintainers are OK with that (e.g. perhaps because it is simpler in a
given case).

I have some more notes at
https://rust-for-linux.com/contributing#the-rust-subsystem.

Of course, in most cases to review abstractions it helps seeing the
expected user, so sometimes it may help to show the users in the same
patch series, and sometimes it may make more sense to just add a link
to Lore to the user, or to a branch; and sometimes examples in the
commit message or in the abstractions' docs themselves are enough.

Cheers,
Miguel

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[Andrew Lunn]

On Thu, Oct 03, 2024 at 12:50:51PM +0200, Miguel Ojeda wrote:
> On Thu, Oct 3, 2024 at 3:24 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
> >
> > Rust abstractions are typically merged with their users. I'm trying to
> > push the delay abstractions with a fix for QT2025 PHY driver
> > (drivers/net/phy/qt2025.rs), which uses delay.
> 
> To clarify, in case it helps: users indeed drive the need for
> abstractions (i.e. we don't merge abstractions without an expected
> user), and it can happen that they go together in the same patch
> series for convenience, that is true.
> 
> However, I don't think I would say "typically", since most
> abstractions went in on their own so far

Looking at the kernel as a whole, i would say that is actually
atypical. Rust is being somewhat special here. But it also seems to be
agreed on that this is O.K.

> In other words, the "default" is that the abstractions go through
> their tree, i.e. delay wouldn't go through netdev, unless the
> maintainers are OK with that (e.g. perhaps because it is simpler in a
> given case).

In this case, the fdelay() binding should be simple enough that i
think we can use the normal mechanism of merging it via netdev, so
long as the other subsystem Maintainer gives an Acked-by: But we can
also pull a stable branch into netdev if we need to.

A Rust equivalent of iopoll.h is going to be a bit more interesting.

./scripts/get_maintainer.pl -f include/linux/iopoll.h 
linux-kernel@vger.kernel.org (open list)

i.e. it does not have a Maintainer!

Looking at the Acked-by:s i would keep Arnd Bergmann <arnd@arndb.de>
in the loop.

	Andrew

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[FUJITA Tomonori]

On Tue, 1 Oct 2024 14:31:39 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +/// Sleeps for a given duration.
>> +///
>> +/// Equivalent to the kernel's [`fsleep`] function, internally calls `udelay`,
>> +/// `usleep_range`, or `msleep`.
> 
> Is it possible to cross reference
> Documentation/timers/timers-howto.rst ?  fsleep() points to it, so it
> would e good if the Rust version also did.

Looks like the pointer to Documentation/timers/timers-howto.rst in
fsleep will be removed soon.

https://lore.kernel.org/all/20240911-devel-anna-maria-b4-timers-flseep-v2-0-b0d3f33ccfe0@linutronix.de/

Re: [PATCH net-next v1 1/2] rust: add delay abstraction

[Andrew Lunn]

On Fri, Oct 04, 2024 at 09:08:19PM +0900, FUJITA Tomonori wrote:
> On Tue, 1 Oct 2024 14:31:39 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> >> +/// Sleeps for a given duration.
> >> +///
> >> +/// Equivalent to the kernel's [`fsleep`] function, internally calls `udelay`,
> >> +/// `usleep_range`, or `msleep`.
> > 
> > Is it possible to cross reference
> > Documentation/timers/timers-howto.rst ?  fsleep() points to it, so it
> > would e good if the Rust version also did.
> 
> Looks like the pointer to Documentation/timers/timers-howto.rst in
> fsleep will be removed soon.
> 
> https://lore.kernel.org/all/20240911-devel-anna-maria-b4-timers-flseep-v2-0-b0d3f33ccfe0@linutronix.de/

It would be more accurate to say it gets replaced with a new document:

Documentation/timers/delay_sleep_functions.rst

So please reference that.

	Andrew

[PATCH net-next v1 2/2] net: phy: qt2025: wait until PHY becomes ready

[FUJITA Tomonori]

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

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 drivers/net/phy/qt2025.rs | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
index 28d8981f410b..3a8ef9f73642 100644
--- a/drivers/net/phy/qt2025.rs
+++ b/drivers/net/phy/qt2025.rs
@@ -93,8 +93,15 @@ 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.
-        Ok(())
+        // sleep here until the hw becomes ready.
+        for _ in 0..60 {
+            kernel::delay::sleep(core::time::Duration::from_millis(50));
+            let val = dev.read(C45::new(Mmd::PCS, 0xd7fd))?;
+            if val != 0x00 && val != 0x10 {
+                return Ok(());
+            }
+        }
+        Err(code::ENODEV)
     }
 
     fn read_status(dev: &mut phy::Device) -> Result<u16> {
-- 
2.34.1

Re: [PATCH net-next v1 2/2] net: phy: qt2025: wait until PHY becomes ready

[Alice Ryhl]

On Tue, Oct 1, 2024 at 1:27 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Wait until a PHY becomes ready in the probe callback by using a sleep
> function.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  drivers/net/phy/qt2025.rs | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
> index 28d8981f410b..3a8ef9f73642 100644
> --- a/drivers/net/phy/qt2025.rs
> +++ b/drivers/net/phy/qt2025.rs
> @@ -93,8 +93,15 @@ 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.
> -        Ok(())
> +        // sleep here until the hw becomes ready.
> +        for _ in 0..60 {
> +            kernel::delay::sleep(core::time::Duration::from_millis(50));
> +            let val = dev.read(C45::new(Mmd::PCS, 0xd7fd))?;
> +            if val != 0x00 && val != 0x10 {
> +                return Ok(());
> +            }

Why not place the sleep after this check? That way, we don't need to
sleep if the check succeeds immediately.

> +        }
> +        Err(code::ENODEV)

This sleeps for up to 3 seconds. Is that the right amount to sleep?

Alice

Re: [PATCH net-next v1 2/2] net: phy: qt2025: wait until PHY becomes ready

[Andrew Lunn]

On Tue, Oct 01, 2024 at 01:36:41PM +0200, Alice Ryhl wrote:
> On Tue, Oct 1, 2024 at 1:27 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
> >
> > Wait until a PHY becomes ready in the probe callback by using a sleep
> > function.
> >
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > ---
> >  drivers/net/phy/qt2025.rs | 11 +++++++++--
> >  1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
> > index 28d8981f410b..3a8ef9f73642 100644
> > --- a/drivers/net/phy/qt2025.rs
> > +++ b/drivers/net/phy/qt2025.rs
> > @@ -93,8 +93,15 @@ 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.
> > -        Ok(())
> > +        // sleep here until the hw becomes ready.
> > +        for _ in 0..60 {
> > +            kernel::delay::sleep(core::time::Duration::from_millis(50));
> > +            let val = dev.read(C45::new(Mmd::PCS, 0xd7fd))?;
> > +            if val != 0x00 && val != 0x10 {
> > +                return Ok(());
> > +            }
> 
> Why not place the sleep after this check? That way, we don't need to
> sleep if the check succeeds immediately.

Nice, you just made my point :-)

I generally point developers at iopoll.h, because developers nearly
always get this sort of polling for something to happen wrong. 

The kernel sleep functions guarantee the minimum sleep time. They say
nothing about the maximum sleep time. You can ask it to sleep for 1ms,
and in reality, due to something stealing the CPU and not being RT
friendly, it actually sleeps for 10ms. This extra long sleep time
blows straight past your timeout, if you have a time based timeout.
What most developers do is after the sleep() returns they check to see
if the timeout has been reached and then exit with -ETIMEDOUT. They
don't check the state of the hardware, which given its had a long time
to do its thing, probably is now in a good state. But the function
returns -ETIMEDOUT.

There should always be a check of the hardware state after the sleep,
in order to determine ETIMEDOUT vs 0.

As i said, most C developers get this wrong. So i don't really see why
Rust developers also will not get this wrong. So i like to discourage
this sort of code, and have Rust implementations of iopoll.h.

	Andrew

iopoll abstraction (was: Re: [PATCH net-next v1 2/2] net: phy: qt2025: wait until PHY becomes ready)

[Dirk Behme]

On 01.10.2024 14:48, Andrew Lunn wrote:
> On Tue, Oct 01, 2024 at 01:36:41PM +0200, Alice Ryhl wrote:
>> On Tue, Oct 1, 2024 at 1:27 PM FUJITA Tomonori
>> <fujita.tomonori@gmail.com> wrote:
>>>
>>> Wait until a PHY becomes ready in the probe callback by using a sleep
>>> function.
>>>
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>> ---
>>>   drivers/net/phy/qt2025.rs | 11 +++++++++--
>>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
>>> index 28d8981f410b..3a8ef9f73642 100644
>>> --- a/drivers/net/phy/qt2025.rs
>>> +++ b/drivers/net/phy/qt2025.rs
>>> @@ -93,8 +93,15 @@ 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.
>>> -        Ok(())
>>> +        // sleep here until the hw becomes ready.
>>> +        for _ in 0..60 {
>>> +            kernel::delay::sleep(core::time::Duration::from_millis(50));
>>> +            let val = dev.read(C45::new(Mmd::PCS, 0xd7fd))?;
>>> +            if val != 0x00 && val != 0x10 {
>>> +                return Ok(());
>>> +            }
>>
>> Why not place the sleep after this check? That way, we don't need to
>> sleep if the check succeeds immediately.
> 
> Nice, you just made my point :-)
> 
> I generally point developers at iopoll.h, because developers nearly
> always get this sort of polling for something to happen wrong.
> 
> The kernel sleep functions guarantee the minimum sleep time. They say
> nothing about the maximum sleep time. You can ask it to sleep for 1ms,
> and in reality, due to something stealing the CPU and not being RT
> friendly, it actually sleeps for 10ms. This extra long sleep time
> blows straight past your timeout, if you have a time based timeout.
> What most developers do is after the sleep() returns they check to see
> if the timeout has been reached and then exit with -ETIMEDOUT. They
> don't check the state of the hardware, which given its had a long time
> to do its thing, probably is now in a good state. But the function
> returns -ETIMEDOUT.
> 
> There should always be a check of the hardware state after the sleep,
> in order to determine ETIMEDOUT vs 0.
> 
> As i said, most C developers get this wrong. So i don't really see why
> Rust developers also will not get this wrong. So i like to discourage
> this sort of code, and have Rust implementations of iopoll.h.


Do we talk about some simple Rust wrappers for the macros in iopoll.h? 
E.g. something like [1]?

Or are we talking about some more complex (safety) dependencies which 
need some more complex abstraction handling?

Best regards

Dirk

[1]

int rust_helper_readb_poll_timeout(const volatile void * addr,
                                   u64 val, u64 cond, u64 delay_us,
                                   u64 timeout_us)
{
        return readb_poll_timeout(addr, val, cond, delay_us, timeout_us);
}

Re: iopoll abstraction

[FUJITA Tomonori]

On Wed, 2 Oct 2024 06:39:48 +0200
Dirk Behme <dirk.behme@de.bosch.com> wrote:

>> I generally point developers at iopoll.h, because developers nearly
>> always get this sort of polling for something to happen wrong.
>> The kernel sleep functions guarantee the minimum sleep time. They say
>> nothing about the maximum sleep time. You can ask it to sleep for 1ms,
>> and in reality, due to something stealing the CPU and not being RT
>> friendly, it actually sleeps for 10ms. This extra long sleep time
>> blows straight past your timeout, if you have a time based timeout.
>> What most developers do is after the sleep() returns they check to see
>> if the timeout has been reached and then exit with -ETIMEDOUT. They
>> don't check the state of the hardware, which given its had a long time
>> to do its thing, probably is now in a good state. But the function
>> returns -ETIMEDOUT.
>> There should always be a check of the hardware state after the sleep,
>> in order to determine ETIMEDOUT vs 0.
>> As i said, most C developers get this wrong. So i don't really see why
>> Rust developers also will not get this wrong. So i like to discourage
>> this sort of code, and have Rust implementations of iopoll.h.
> 
> 
> Do we talk about some simple Rust wrappers for the macros in iopoll.h?
> E.g. something like [1]?
> 
> Or are we talking about some more complex (safety) dependencies which
> need some more complex abstraction handling?

(snip)

> int rust_helper_readb_poll_timeout(const volatile void * addr,
>                                   u64 val, u64 cond, u64 delay_us,
>                                   u64 timeout_us)
> {
>        return readb_poll_timeout(addr, val, cond, delay_us, timeout_us);
> }

I'm not sure a simple wrapper for iopoll.h works. We need to pass a
function. I'm testing a macro like the following (not added ktime
timeout yet):

macro_rules! read_poll_timeout {
    ($op:expr, $val:expr, $cond:expr, $sleep:expr, $timeout:expr, $($args:expr),*) => {{
        let _ = $val;
        loop {
            $val = $op($($args),*);
            if $cond {
                break;
            }
            kernel::delay::sleep($sleep);
        }
        if $cond {
            Ok(())
        } else {
            Err(kernel::error::code::ETIMEDOUT)
        }
    }};
}

Re: iopoll abstraction

[Boqun Feng]

On Wed, Oct 02, 2024 at 09:56:36AM +0000, FUJITA Tomonori wrote:
> On Wed, 2 Oct 2024 06:39:48 +0200
> Dirk Behme <dirk.behme@de.bosch.com> wrote:
> 
> >> I generally point developers at iopoll.h, because developers nearly
> >> always get this sort of polling for something to happen wrong.
> >> The kernel sleep functions guarantee the minimum sleep time. They say
> >> nothing about the maximum sleep time. You can ask it to sleep for 1ms,
> >> and in reality, due to something stealing the CPU and not being RT
> >> friendly, it actually sleeps for 10ms. This extra long sleep time
> >> blows straight past your timeout, if you have a time based timeout.
> >> What most developers do is after the sleep() returns they check to see
> >> if the timeout has been reached and then exit with -ETIMEDOUT. They
> >> don't check the state of the hardware, which given its had a long time
> >> to do its thing, probably is now in a good state. But the function
> >> returns -ETIMEDOUT.
> >> There should always be a check of the hardware state after the sleep,
> >> in order to determine ETIMEDOUT vs 0.
> >> As i said, most C developers get this wrong. So i don't really see why
> >> Rust developers also will not get this wrong. So i like to discourage
> >> this sort of code, and have Rust implementations of iopoll.h.
> > 
> > 
> > Do we talk about some simple Rust wrappers for the macros in iopoll.h?
> > E.g. something like [1]?
> > 
> > Or are we talking about some more complex (safety) dependencies which
> > need some more complex abstraction handling?
> 
> (snip)
> 
> > int rust_helper_readb_poll_timeout(const volatile void * addr,
> >                                   u64 val, u64 cond, u64 delay_us,
> >                                   u64 timeout_us)
> > {
> >        return readb_poll_timeout(addr, val, cond, delay_us, timeout_us);
> > }
> 
> I'm not sure a simple wrapper for iopoll.h works. We need to pass a
> function. I'm testing a macro like the following (not added ktime
> timeout yet):

You could use closure as a parameter to avoid macro interface, something
like:

	fn read_poll_timeout<Op, Cond, T>(
	    op: Op,
	    cond: Cond,
	    sleep: Delta,
	    timeout: Delta,
	) -> Result<T> where
	    Op: Fn() -> T,
	    cond: Fn() -> bool {

	    let __timeout = kernel::Ktime::ktime_get() + timeout;

	    let val = loop {
		let val = op();
		if cond() {
		    break Some(val);
		}
		kernel::delay::sleep(sleep);

		if __timeout.after(kernel::Ktime::ktime_get()) {
		    break None;
		}
	    };

	    if cond() {
		val
	    } else {
		Err(kernel::error::code::ETIMEDOUT)
	    }
	}

note that you don't need the args part, because `op` is a closure that
can capature value, so for example, if in C code you need to call foo(a,
b), with closure, you can do:

	<a and b are defined in the caller>
	read_poll_timeout(|| { foo(a, b) }, ...);

with above API.

Regards,
Boqun

> 
> macro_rules! read_poll_timeout {
>     ($op:expr, $val:expr, $cond:expr, $sleep:expr, $timeout:expr, $($args:expr),*) => {{
>         let _ = $val;
>         loop {
>             $val = $op($($args),*);
>             if $cond {
>                 break;
>             }
>             kernel::delay::sleep($sleep);
>         }
>         if $cond {
>             Ok(())
>         } else {
>             Err(kernel::error::code::ETIMEDOUT)
>         }
>     }};
> }
> 

Re: iopoll abstraction

[FUJITA Tomonori]

On Thu, 3 Oct 2024 04:52:48 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> You could use closure as a parameter to avoid macro interface, something
> like:
> 
> 	fn read_poll_timeout<Op, Cond, T>(
> 	    op: Op,
> 	    cond: Cond,
> 	    sleep: Delta,
> 	    timeout: Delta,
> 	) -> Result<T> where
> 	    Op: Fn() -> T,
> 	    cond: Fn() -> bool {
> 
> 	    let __timeout = kernel::Ktime::ktime_get() + timeout;
> 
> 	    let val = loop {
> 		let val = op();
> 		if cond() {
> 		    break Some(val);
> 		}
> 		kernel::delay::sleep(sleep);
> 
> 		if __timeout.after(kernel::Ktime::ktime_get()) {
> 		    break None;
> 		}
> 	    };
> 
> 	    if cond() {
> 		val
> 	    } else {
> 		Err(kernel::error::code::ETIMEDOUT)
> 	    }
> 	}

Great! I changed couple of things.

1. Op typically reads a value from a register and could need mut objects. So I use FnMut.
2. reading from hardware could fail so Op had better to return an error [1].
3. Cond needs val; typically check the value from a register.

[1] https://lore.kernel.org/netdev/ec7267b5-ae77-4c4a-94f8-aa933c87a9a2@lunn.ch

Seems that the following works QT2025 driver. How does it look?

fn read_poll_timeout<Op, Cond, T: Copy>(
    mut op: Op,
    cond: Cond,
    sleep: Delta,
    timeout: Delta,
) -> Result<T>
where
    Op: FnMut() -> Result<T>,
    Cond: Fn(T) -> bool,
{
    let timeout = Ktime::ktime_get() + timeout;
    let ret = loop {
        let val = op()?;
        if cond(val) {
            break Ok(val);
        }
        kernel::delay::sleep(sleep);

        if Ktime::ktime_get() > timeout {
            break Err(code::ETIMEDOUT);
        }
    };

    ret
}

Re: iopoll abstraction

[Boqun Feng]

On Thu, Oct 03, 2024 at 01:45:18PM +0000, FUJITA Tomonori wrote:
> On Thu, 3 Oct 2024 04:52:48 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > You could use closure as a parameter to avoid macro interface, something
> > like:
> > 
> > 	fn read_poll_timeout<Op, Cond, T>(
> > 	    op: Op,
> > 	    cond: Cond,
> > 	    sleep: Delta,
> > 	    timeout: Delta,
> > 	) -> Result<T> where
> > 	    Op: Fn() -> T,
> > 	    cond: Fn() -> bool {
> > 
> > 	    let __timeout = kernel::Ktime::ktime_get() + timeout;
> > 
> > 	    let val = loop {
> > 		let val = op();
> > 		if cond() {
> > 		    break Some(val);
> > 		}
> > 		kernel::delay::sleep(sleep);
> > 
> > 		if __timeout.after(kernel::Ktime::ktime_get()) {
> > 		    break None;
> > 		}
> > 	    };
> > 
> > 	    if cond() {
> > 		val
> > 	    } else {
> > 		Err(kernel::error::code::ETIMEDOUT)
> > 	    }
> > 	}
> 
> Great! I changed couple of things.
> 
> 1. Op typically reads a value from a register and could need mut objects. So I use FnMut.
> 2. reading from hardware could fail so Op had better to return an error [1].
> 3. Cond needs val; typically check the value from a register.
> 
> [1] https://lore.kernel.org/netdev/ec7267b5-ae77-4c4a-94f8-aa933c87a9a2@lunn.ch
> 
> Seems that the following works QT2025 driver. How does it look?
> 

Makes sense to me. Of course, more users will probably tell us whether
we cover all the cases, but this is a good starting point. I would put
the function at kernel::io::poll, but that's my personal preference ;-)

Regards,
Boqun

> fn read_poll_timeout<Op, Cond, T: Copy>(
>     mut op: Op,
>     cond: Cond,
>     sleep: Delta,
>     timeout: Delta,
> ) -> Result<T>
> where
>     Op: FnMut() -> Result<T>,
>     Cond: Fn(T) -> bool,
> {
>     let timeout = Ktime::ktime_get() + timeout;
>     let ret = loop {
>         let val = op()?;
>         if cond(val) {
>             break Ok(val);
>         }
>         kernel::delay::sleep(sleep);
> 
>         if Ktime::ktime_get() > timeout {
>             break Err(code::ETIMEDOUT);
>         }
>     };
> 
>     ret
> }

Re: iopoll abstraction

[Andrew Lunn]

> > fn read_poll_timeout<Op, Cond, T: Copy>(
> >     mut op: Op,
> >     cond: Cond,
> >     sleep: Delta,
> >     timeout: Delta,
> > ) -> Result<T>
> > where
> >     Op: FnMut() -> Result<T>,
> >     Cond: Fn(T) -> bool,
> > {
> >     let timeout = Ktime::ktime_get() + timeout;
> >     let ret = loop {
> >         let val = op()?;
> >         if cond(val) {
> >             break Ok(val);
> >         }
> >         kernel::delay::sleep(sleep);
> > 
> >         if Ktime::ktime_get() > timeout {
> >             break Err(code::ETIMEDOUT);
> >         }
> >     };
> > 
> >     ret
> > }

This appears to have the usual bug when people implement it themselves
and i then point them at iopoll.h, which so far as been bug free.

kernel::delay::sleep(sleep) can sleep for an arbitrary amount of time
greater than sleep, if the system is busy doing other things. You
might only get around this loop once, and exit with ETIMEOUT, but
while you have been sleeping a long time the hardware has completed
its operation, but you never check.

There must be a call to cond() after the timeout to handle this
condition.

And this is not theoretical. I had a very reproducible case of this
during the boot of a device. It is less likely today, with SMP
systems, and all the RT patches, but if it does happen, it will be
very hard to track down.

	Andrew

Re: iopoll abstraction

[FUJITA Tomonori]

On Thu, 3 Oct 2024 18:00:36 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> > fn read_poll_timeout<Op, Cond, T: Copy>(
>> >     mut op: Op,
>> >     cond: Cond,
>> >     sleep: Delta,
>> >     timeout: Delta,
>> > ) -> Result<T>
>> > where
>> >     Op: FnMut() -> Result<T>,
>> >     Cond: Fn(T) -> bool,
>> > {
>> >     let timeout = Ktime::ktime_get() + timeout;
>> >     let ret = loop {
>> >         let val = op()?;
>> >         if cond(val) {
>> >             break Ok(val);
>> >         }
>> >         kernel::delay::sleep(sleep);
>> > 
>> >         if Ktime::ktime_get() > timeout {
>> >             break Err(code::ETIMEDOUT);
>> >         }
>> >     };
>> > 
>> >     ret
>> > }
> 
> This appears to have the usual bug when people implement it themselves
> and i then point them at iopoll.h, which so far as been bug free.

Ah, in the next submission, I'll try to ensure that the Rust version
works the same way as the C version.

Re: iopoll abstraction

[Andrew Lunn]

> we cover all the cases, but this is a good starting point. I would put
> the function at kernel::io::poll, but that's my personal preference ;-)

iopoll.h has a few variants. The variant being implemented here can
only be used in thread context where it can sleep. There is also
read_poll_timeout_atomic() which can be used in atomic context, which
uses udelay() rather than sleeping, since you are not allowed to sleep
in atomic context.

So we should keep the naming open to allow for the atomic variant
sometime in the future.

We probably also want a comment that this helper cannot be used in
atomic context.

Do we have a Rust equivalent of might_sleep()?

https://elixir.bootlin.com/linux/v6.12-rc1/source/include/linux/kernel.h#L93

	Andrew

Re: iopoll abstraction

[FUJITA Tomonori]

On Thu, 3 Oct 2024 18:09:15 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> We probably also want a comment that this helper cannot be used in
> atomic context.

Yeah, I'll add such.

> Do we have a Rust equivalent of might_sleep()?
> 
> https://elixir.bootlin.com/linux/v6.12-rc1/source/include/linux/kernel.h#L93

No. I'll add bindings for might_sleep() and cpu_relax().

Re: iopoll abstraction

[Andrew Lunn]

On Fri, Oct 04, 2024 at 08:48:03PM +0900, FUJITA Tomonori wrote:
> On Thu, 3 Oct 2024 18:09:15 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > We probably also want a comment that this helper cannot be used in
> > atomic context.
> 
> Yeah, I'll add such.
> 
> > Do we have a Rust equivalent of might_sleep()?
> > 
> > https://elixir.bootlin.com/linux/v6.12-rc1/source/include/linux/kernel.h#L93
> 
> No. I'll add bindings for might_sleep() and cpu_relax().

Please make sure you involve the scheduler people. This is now well
outside of networking, same as the discussion around time has little
to do with networking.

The might_sleep() is not a strong requirement for iopoll, so you might
want to get the basic functionality merged first, and then once
might_sleep() is agreed on, add it to iopoll. It is just a debug
feature.

	Andrew

Re: [PATCH net-next v1 2/2] net: phy: qt2025: wait until PHY becomes ready

[FUJITA Tomonori]

On Tue, 1 Oct 2024 14:48:06 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> I generally point developers at iopoll.h, because developers nearly
> always get this sort of polling for something to happen wrong. 

Ah, I had forgotten about iopoll.h. Make senses. I'll try implement an
equivalent in Rust.

Re: [PATCH net-next v1 2/2] net: phy: qt2025: wait until PHY becomes ready

[Andrew Lunn]

On Wed, Oct 02, 2024 at 10:13:39AM +0000, FUJITA Tomonori wrote:
> On Tue, 1 Oct 2024 14:48:06 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > I generally point developers at iopoll.h, because developers nearly
> > always get this sort of polling for something to happen wrong. 
> 
> Ah, I had forgotten about iopoll.h. Make senses. I'll try implement an
> equivalent in Rust.

There are some subtleties involved with PHYs, which is why we have our
own wrapper around the macros in iopoll.h:

https://elixir.bootlin.com/linux/v6.11.1/source/include/linux/phy.h#L1288

Normally an IO operation cannot fail. But PHYs are different, a read
could return -EOPNOTSUPP, -EIO, -ETIMEDOUT etc. That needs to be take
into account and checked before evaluating the condition.

So you might need to think about something similar for Rust. A generic
read_poll_timeout() and a phy_read_poll_timeout() built on top of it.

This is a worthwhile set of helpers to have, since the bugs in this
are nothing to do with memory safety, but plain simple logic bugs,
which Rust itself is unlikely to help with.

	Andrew

Re: [PATCH net-next v1 2/2] net: phy: qt2025: wait until PHY becomes ready

[FUJITA Tomonori]

On Wed, 2 Oct 2024 14:31:07 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> There are some subtleties involved with PHYs, which is why we have our
> own wrapper around the macros in iopoll.h:
> 
> https://elixir.bootlin.com/linux/v6.11.1/source/include/linux/phy.h#L1288
> 
> Normally an IO operation cannot fail. But PHYs are different, a read
> could return -EOPNOTSUPP, -EIO, -ETIMEDOUT etc. That needs to be take
> into account and checked before evaluating the condition.

Thanks, I didn't know this function. I think that a generic
read_poll_timeout in Rust can handle such case.

#define read_poll_timeout(op, val, cond, sleep_us, timeout_us, \
				sleep_before_read, args...) \
({ \
	u64 __timeout_us = (timeout_us); \
	unsigned long __sleep_us = (sleep_us); \
	ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
	might_sleep_if((__sleep_us) != 0); \
	if (sleep_before_read && __sleep_us) \
		usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
	for (;;) { \
		(val) = op(args); \
		if (cond) \
			break; \

The reason why the C version cannot handle such case is that after
call `op` function, read_poll_timeout macro can't know whether `val`
is an error or not.

In Rust version, 'op' function can return Result type, explicitly
tells success or an error. It can return an error before evaluating
the condition.

Re: [PATCH net-next v1 2/2] net: phy: qt2025: wait until PHY becomes ready

[FUJITA Tomonori]

On Tue, 1 Oct 2024 13:36:41 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

>> diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
>> index 28d8981f410b..3a8ef9f73642 100644
>> --- a/drivers/net/phy/qt2025.rs
>> +++ b/drivers/net/phy/qt2025.rs
>> @@ -93,8 +93,15 @@ 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.
>> -        Ok(())
>> +        // sleep here until the hw becomes ready.
>> +        for _ in 0..60 {
>> +            kernel::delay::sleep(core::time::Duration::from_millis(50));
>> +            let val = dev.read(C45::new(Mmd::PCS, 0xd7fd))?;
>> +            if val != 0x00 && val != 0x10 {
>> +                return Ok(());
>> +            }
> 
> Why not place the sleep after this check? That way, we don't need to
> sleep if the check succeeds immediately.

Yeah, that's better. I copied the logic in the vendor driver without
thinking.

As Andrew pointed out, the code like this in C drivers isn't
recommended; iopoll.h should be used. So I'll try to implement such.


>> +        }
>> +        Err(code::ENODEV)
> 
> This sleeps for up to 3 seconds. Is that the right amount to sleep?

That's what the vendor driver does.

Re: [PATCH net-next v1 0/2] add delay abstraction (sleep functions)

[Alice Ryhl]

On Tue, Oct 1, 2024 at 1:27 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Add an abstraction for sleep functions in `include/linux/delay.h` for
> dealing with hardware delays. `delay.h` supports sleep and delay (busy
> wait). This adds support for sleep functions used by QT2025 PHY driver
> to sleep until a PHY becomes ready.
>
> The old rust branch has the delay abstraction which supports msleep()
> with a helper function which rounds a `Duration` up to the nearest
> milliseconds.
>
> This adds fsleep() support instead of msleep(). fsleep() can handle
> various lengths of delay by internally calling an appropriate sleep
> function including msleep().

Add time keeping maintainers to CC.