[PATCH net-next v2 0/6] rust: Add IO polling

[PATCH net-next v2 0/6] rust: Add IO polling

[FUJITA Tomonori]

Add Rust version of read_poll_timeout (include/linux/iopoll.h), which
polls periodically until a condition is met or a timeout is reached.
By using the function, the 6th patch fixes QT2025 PHY driver to sleep
until the hardware becomes ready.

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

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

v2:
- Introduce time::Delta instead of core::time::Duration
- Add some trait to Ktime for calculating timeout
- Use read_poll_timeout in QT2025 driver instead of using fsleep directly
v1: https://lore.kernel.org/netdev/20241001112512.4861-1-fujita.tomonori@gmail.com/


FUJITA Tomonori (6):
  rust: time: Implement PartialEq and PartialOrd for Ktime
  rust: time: Introduce Delta type
  rust: time: Implement addition of Ktime and Delta
  rust: time: add wrapper for fsleep function
  rust: Add read_poll_timeout function
  net: phy: qt2025: wait until PHY becomes ready

 drivers/net/phy/qt2025.rs |  11 +++-
 rust/helpers/helpers.c    |   2 +
 rust/helpers/kernel.c     |  13 +++++
 rust/helpers/time.c       |  19 +++++++
 rust/kernel/error.rs      |   1 +
 rust/kernel/io.rs         |   5 ++
 rust/kernel/io/poll.rs    |  70 +++++++++++++++++++++++
 rust/kernel/lib.rs        |   1 +
 rust/kernel/time.rs       | 113 ++++++++++++++++++++++++++++++++++++++
 9 files changed, 234 insertions(+), 1 deletion(-)
 create mode 100644 rust/helpers/kernel.c
 create mode 100644 rust/helpers/time.c
 create mode 100644 rust/kernel/io.rs
 create mode 100644 rust/kernel/io/poll.rs


base-commit: d521db38f339709ccd23c5deb7663904e626c3a6
-- 
2.34.1

[PATCH net-next v2 1/6] rust: time: Implement PartialEq and PartialOrd for Ktime

[FUJITA Tomonori]

Implement PartialEq and PartialOrd trait for Ktime by using C's
ktime_compare function so two Ktime instances can be compared to
determine whether a timeout is met or not.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers/helpers.c |  1 +
 rust/helpers/time.c    |  8 ++++++++
 rust/kernel/time.rs    | 22 ++++++++++++++++++++++
 3 files changed, 31 insertions(+)
 create mode 100644 rust/helpers/time.c

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 30f40149f3a9..c274546bcf78 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -21,6 +21,7 @@
 #include "slab.c"
 #include "spinlock.c"
 #include "task.c"
+#include "time.c"
 #include "uaccess.c"
 #include "wait.c"
 #include "workqueue.c"
diff --git a/rust/helpers/time.c b/rust/helpers/time.c
new file mode 100644
index 000000000000..d6f61affb2c3
--- /dev/null
+++ b/rust/helpers/time.c
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/ktime.h>
+
+int rust_helper_ktime_compare(const ktime_t cmp1, const ktime_t cmp2)
+{
+	return ktime_compare(cmp1, cmp2);
+}
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index e3bb5e89f88d..c40105941a2c 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -81,3 +81,25 @@ fn sub(self, other: Ktime) -> Ktime {
         }
     }
 }
+
+impl PartialEq for Ktime {
+    #[inline]
+    fn eq(&self, other: &Self) -> bool {
+        // SAFETY: FFI call.
+        let ret = unsafe { bindings::ktime_compare(self.inner, other.inner) };
+        ret == 0
+    }
+}
+
+impl PartialOrd for Ktime {
+    #[inline]
+    fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
+        // SAFETY: FFI call.
+        let ret = unsafe { bindings::ktime_compare(self.inner, other.inner) };
+        match ret {
+            0 => Some(core::cmp::Ordering::Equal),
+            x if x < 0 => Some(core::cmp::Ordering::Less),
+            _ => Some(core::cmp::Ordering::Greater),
+        }
+    }
+}
-- 
2.34.1

Re: [PATCH net-next v2 1/6] rust: time: Implement PartialEq and PartialOrd for Ktime

[Fiona Behrens]

On 5 Oct 2024, at 14:25, FUJITA Tomonori wrote:

> Implement PartialEq and PartialOrd trait for Ktime by using C's
> ktime_compare function so two Ktime instances can be compared to
> determine whether a timeout is met or not.

Why is this only PartialEq/PartialOrd? Could we either document why or implement Eq/Ord as well?

>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/helpers/helpers.c |  1 +
>  rust/helpers/time.c    |  8 ++++++++
>  rust/kernel/time.rs    | 22 ++++++++++++++++++++++
>  3 files changed, 31 insertions(+)
>  create mode 100644 rust/helpers/time.c
>
> diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
> index 30f40149f3a9..c274546bcf78 100644
> --- a/rust/helpers/helpers.c
> +++ b/rust/helpers/helpers.c
> @@ -21,6 +21,7 @@
>  #include "slab.c"
>  #include "spinlock.c"
>  #include "task.c"
> +#include "time.c"
>  #include "uaccess.c"
>  #include "wait.c"
>  #include "workqueue.c"
> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
> new file mode 100644
> index 000000000000..d6f61affb2c3
> --- /dev/null
> +++ b/rust/helpers/time.c
> @@ -0,0 +1,8 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/ktime.h>
> +
> +int rust_helper_ktime_compare(const ktime_t cmp1, const ktime_t cmp2)
> +{
> +	return ktime_compare(cmp1, cmp2);
> +}
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index e3bb5e89f88d..c40105941a2c 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -81,3 +81,25 @@ fn sub(self, other: Ktime) -> Ktime {
>          }
>      }
>  }
> +
> +impl PartialEq for Ktime {
> +    #[inline]
> +    fn eq(&self, other: &Self) -> bool {
> +        // SAFETY: FFI call.
> +        let ret = unsafe { bindings::ktime_compare(self.inner, other.inner) };
> +        ret == 0
> +    }
> +}
> +
> +impl PartialOrd for Ktime {
> +    #[inline]
> +    fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
> +        // SAFETY: FFI call.
> +        let ret = unsafe { bindings::ktime_compare(self.inner, other.inner) };
> +        match ret {
> +            0 => Some(core::cmp::Ordering::Equal),
> +            x if x < 0 => Some(core::cmp::Ordering::Less),
> +            _ => Some(core::cmp::Ordering::Greater),
> +        }
> +    }
> +}
> -- 
> 2.34.1

Re: [PATCH net-next v2 1/6] rust: time: Implement PartialEq and PartialOrd for Ktime

[FUJITA Tomonori]

On Sun, 06 Oct 2024 12:28:59 +0200
Fiona Behrens <finn@kloenk.dev> wrote:

>> Implement PartialEq and PartialOrd trait for Ktime by using C's
>> ktime_compare function so two Ktime instances can be compared to
>> determine whether a timeout is met or not.
> 
> Why is this only PartialEq/PartialOrd? Could we either document why or implement Eq/Ord as well?

Because what we need to do is comparing two Ktime instances so we
don't need them?

Re: [PATCH net-next v2 1/6] rust: time: Implement PartialEq and PartialOrd for Ktime

[Fiona Behrens]

On 7 Oct 2024, at 7:37, FUJITA Tomonori wrote:

> On Sun, 06 Oct 2024 12:28:59 +0200
> Fiona Behrens <finn@kloenk.dev> wrote:
>
>>> Implement PartialEq and PartialOrd trait for Ktime by using C's
>>> ktime_compare function so two Ktime instances can be compared to
>>> determine whether a timeout is met or not.
>>
>> Why is this only PartialEq/PartialOrd? Could we either document why or implement Eq/Ord as well?
>
> Because what we need to do is comparing two Ktime instances so we
> don't need them?

Eq is basically just a marker trait, so you could argue we would never need it. I think because those 2 traits mostly just document logic it would make sense to also implement them to not create rethinking if then there is some logic that might want it and then the question is why was it omitted.

 - Fiona

Re: [PATCH net-next v2 1/6] rust: time: Implement PartialEq and PartialOrd for Ktime

[Alice Ryhl]

On Mon, Oct 7, 2024 at 7:37 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Sun, 06 Oct 2024 12:28:59 +0200
> Fiona Behrens <finn@kloenk.dev> wrote:
>
> >> Implement PartialEq and PartialOrd trait for Ktime by using C's
> >> ktime_compare function so two Ktime instances can be compared to
> >> determine whether a timeout is met or not.
> >
> > Why is this only PartialEq/PartialOrd? Could we either document why or implement Eq/Ord as well?
>
> Because what we need to do is comparing two Ktime instances so we
> don't need them?

When you implement PartialEq without Eq, you are telling the reader
that this is a weird type such as floats where there exists values
that are not equal to themselves. That's not the case here, so don't
confuse the reader by leaving out `Eq`.

Alice

Re: [PATCH net-next v2 1/6] rust: time: Implement PartialEq and PartialOrd for Ktime

[FUJITA Tomonori]

On Mon, 7 Oct 2024 10:41:23 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

>> >> Implement PartialEq and PartialOrd trait for Ktime by using C's
>> >> ktime_compare function so two Ktime instances can be compared to
>> >> determine whether a timeout is met or not.
>> >
>> > Why is this only PartialEq/PartialOrd? Could we either document why or implement Eq/Ord as well?
>>
>> Because what we need to do is comparing two Ktime instances so we
>> don't need them?
> 
> When you implement PartialEq without Eq, you are telling the reader
> that this is a weird type such as floats where there exists values
> that are not equal to themselves. That's not the case here, so don't
> confuse the reader by leaving out `Eq`.

Understood. I'll add Eq/Ord in the next version.

Re: [PATCH net-next v2 1/6] rust: time: Implement PartialEq and PartialOrd for Ktime

[Andrew Lunn]

On Mon, Oct 07, 2024 at 10:41:23AM +0200, Alice Ryhl wrote:
> On Mon, Oct 7, 2024 at 7:37 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
> >
> > On Sun, 06 Oct 2024 12:28:59 +0200
> > Fiona Behrens <finn@kloenk.dev> wrote:
> >
> > >> Implement PartialEq and PartialOrd trait for Ktime by using C's
> > >> ktime_compare function so two Ktime instances can be compared to
> > >> determine whether a timeout is met or not.
> > >
> > > Why is this only PartialEq/PartialOrd? Could we either document why or implement Eq/Ord as well?
> >
> > Because what we need to do is comparing two Ktime instances so we
> > don't need them?
> 
> When you implement PartialEq without Eq, you are telling the reader
> that this is a weird type such as floats where there exists values
> that are not equal to themselves. That's not the case here, so don't
> confuse the reader by leaving out `Eq`.
 
This might be one of those areas where there needs to be a difference
between C and Rust in terms of kernel rules. For C, there would need
to be a user. Here you seem to be saying the type system needs it, for
the type to be meaningful, even if there is no user?

Without Eq, would the compiler complain on an == operation, saying it
is not a valid operation? Is there a clear difference between nobody
has implemented this yet, vs such an operation is impossible, such as
your float example?

	Andrew

Re: [PATCH net-next v2 1/6] rust: time: Implement PartialEq and PartialOrd for Ktime

[Alice Ryhl]

On Mon, Oct 7, 2024 at 3:16 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Oct 07, 2024 at 10:41:23AM +0200, Alice Ryhl wrote:
> > On Mon, Oct 7, 2024 at 7:37 AM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:
> > >
> > > On Sun, 06 Oct 2024 12:28:59 +0200
> > > Fiona Behrens <finn@kloenk.dev> wrote:
> > >
> > > >> Implement PartialEq and PartialOrd trait for Ktime by using C's
> > > >> ktime_compare function so two Ktime instances can be compared to
> > > >> determine whether a timeout is met or not.
> > > >
> > > > Why is this only PartialEq/PartialOrd? Could we either document why or implement Eq/Ord as well?
> > >
> > > Because what we need to do is comparing two Ktime instances so we
> > > don't need them?
> >
> > When you implement PartialEq without Eq, you are telling the reader
> > that this is a weird type such as floats where there exists values
> > that are not equal to themselves. That's not the case here, so don't
> > confuse the reader by leaving out `Eq`.
>
> This might be one of those areas where there needs to be a difference
> between C and Rust in terms of kernel rules. For C, there would need
> to be a user. Here you seem to be saying the type system needs it, for
> the type to be meaningful, even if there is no user?
>
> Without Eq, would the compiler complain on an == operation, saying it
> is not a valid operation? Is there a clear difference between nobody
> has implemented this yet, vs such an operation is impossible, such as
> your float example?

Think of it this way: I wrote an implementation of something that
works in situations A and B, but I only use it in situation A. Must I
write my program in a way to make it impossible to use it in situation
B? That's how I see this case. Implementing Eq does not involve adding
any new functions.

Alice

[PATCH net-next v2 2/6] rust: time: Introduce Delta type

[FUJITA Tomonori]

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

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

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

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index c40105941a2c..6c5a1c50c5f1 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -8,9 +8,15 @@
 //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
 //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
 
+/// The number of nanoseconds per microsecond.
+pub const NSEC_PER_USEC: i64 = bindings::NSEC_PER_USEC as i64;
+
 /// The number of nanoseconds per millisecond.
 pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
 
+/// The number of nanoseconds per second.
+pub const NSEC_PER_SEC: i64 = bindings::NSEC_PER_SEC as i64;
+
 /// The time unit of Linux kernel. One jiffy equals (1/HZ) second.
 pub type Jiffies = core::ffi::c_ulong;
 
@@ -103,3 +109,61 @@ fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
         }
     }
 }
+
+/// A span of time.
+#[derive(Copy, Clone)]
+pub struct Delta {
+    nanos: i64,
+}
+
+impl Delta {
+    /// Create a new `Delta` from a number of nanoseconds.
+    #[inline]
+    pub fn from_nanos(nanos: u16) -> Self {
+        Self {
+            nanos: nanos.into(),
+        }
+    }
+
+    /// Create a new `Delta` from a number of microseconds.
+    #[inline]
+    pub fn from_micros(micros: u16) -> Self {
+        Self {
+            nanos: micros as i64 * NSEC_PER_USEC,
+        }
+    }
+
+    /// Create a new `Delta` from a number of milliseconds.
+    #[inline]
+    pub fn from_millis(millis: u16) -> Self {
+        Self {
+            nanos: millis as i64 * NSEC_PER_MSEC,
+        }
+    }
+
+    /// Create a new `Delta` from a number of seconds.
+    #[inline]
+    pub fn from_secs(secs: u16) -> Self {
+        Self {
+            nanos: secs as i64 * NSEC_PER_SEC,
+        }
+    }
+
+    /// Return `true` if the `Detla` spans no time.
+    #[inline]
+    pub fn is_zero(self) -> bool {
+        self.nanos == 0
+    }
+
+    /// Return the number of nanoseconds in the `Delta`.
+    #[inline]
+    pub fn as_nanos(self) -> i64 {
+        self.nanos
+    }
+
+    /// Return the number of microseconds in the `Delta`.
+    #[inline]
+    pub fn as_micros(self) -> i64 {
+        self.nanos / NSEC_PER_USEC
+    }
+}
-- 
2.34.1

Re: [PATCH net-next v2 2/6] rust: time: Introduce Delta type

[Andrew Lunn]

> +/// A span of time.
> +#[derive(Copy, Clone)]
> +pub struct Delta {
> +    nanos: i64,

Is there are use case for negative Deltas ? Should this be u64?

A u64 would allow upto 500 years, if i did my back of an envelope
maths correct. So i suppose 250 years allowing negative delta would
also work.

> +}
> +
> +impl Delta {
> +    /// Create a new `Delta` from a number of nanoseconds.
> +    #[inline]
> +    pub fn from_nanos(nanos: u16) -> Self {

So here you don't allow negative values.

But why limit it to u16, when the base value is a 63 bits? 65535 nS is
not very long.

> +        Self {
> +            nanos: nanos.into(),
> +        }
> +    }
> +
> +    /// Create a new `Delta` from a number of microseconds.
> +    #[inline]
> +    pub fn from_micros(micros: u16) -> Self {

A u32 should not overflow when converted to nS in an i64.

Dumb question. What does Rust in the kernel do if there is an
overflow?

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

So here we are back to signed values. And also you cannot create a
Delta from a Delta because the types are not transitive.

	Andrew

Re: [PATCH net-next v2 2/6] rust: time: Introduce Delta type

[Miguel Ojeda]

On Sat, Oct 5, 2024 at 8:03 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> But why limit it to u16, when the base value is a 63 bits? 65535 nS is
> not very long.

Agreed, for these constructors we should use (at the very least) the
biggest possible type that fits without possible wrapping.

> Dumb question. What does Rust in the kernel do if there is an
> overflow?

It is controlled by `CONFIG_RUST_OVERFLOW_CHECKS` -- it defaults to a
Rust panic, but otherwise it wraps. Either way, it is not UB and
regardless of the setting, it is considered a bug to be fixed.

(I requested upstream Rust a third mode that would allow us to report
the issue (e.g. map it to `WARN_ONCE`) and then continue with wrap.)

Cheers,
Miguel

Re: [PATCH net-next v2 2/6] rust: time: Introduce Delta type

[FUJITA Tomonori]

On Sat, 5 Oct 2024 20:02:55 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +/// A span of time.
>> +#[derive(Copy, Clone)]
>> +pub struct Delta {
>> +    nanos: i64,
> 
> Is there are use case for negative Deltas ? Should this be u64?

I thought that logically Delta could be negative but considering Ktime
APIs like the following, I think that u64 is more appropriate now.

static inline ktime_t ktime_add_us(const ktime_t kt, const u64 usec)
{
        return ktime_add_ns(kt, usec * NSEC_PER_USEC);
}

static inline ktime_t ktime_sub_us(const ktime_t kt, const u64 usec)
{
        return ktime_sub_ns(kt, usec * NSEC_PER_USEC);
}

>> +}
>> +
>> +impl Delta {
>> +    /// Create a new `Delta` from a number of nanoseconds.
>> +    #[inline]
>> +    pub fn from_nanos(nanos: u16) -> Self {
> 
> So here you don't allow negative values.
> 
> But why limit it to u16, when the base value is a 63 bits? 65535 nS is
> not very long.

I thought that from_secs(u16) gives long enough duration but
how about the following APIs?

pub fn from_nanos(nanos: u64)
pub fn from_micros(micros: u32)
pub fn from_millis(millis: u16) 

You can create the maximum via from_nanos. from_micros and from_millis
don't cause wrapping.

Re: [PATCH net-next v2 2/6] rust: time: Introduce Delta type

[Andrew Lunn]

> I thought that from_secs(u16) gives long enough duration but
> how about the following APIs?
> 
> pub fn from_nanos(nanos: u64)
> pub fn from_micros(micros: u32)
> pub fn from_millis(millis: u16) 
> 
> You can create the maximum via from_nanos. from_micros and from_millis
> don't cause wrapping.

When i talked about transitive types, i was meaning that to_nanos(),
to_micros(), to_millis() should have the same type as from_nanos(),
to_micros(), and to_millis().

It is clear these APIs cause discard. millis is a lot less accurate
than nanos. Which is fine, the names make that obvious. But what about
the range? Are there values i can create using from_nanos() which i
cannot then use to_millis() on because it overflows the u16? And i
guess the overflow point is different to to_micros()? This API feels
inconsistent to me. This is why i suggested u64 is used
everywhere. And we avoid the range issues, by artificially clamping to
something which can be represented in all forms, so we have a uniform
behaviour.

But i have little experience of dealing with time in the kernel. I
don't know what the real issues are here, what developers have been
getting wrong for the last 30 years etc.

	Andrew

Re: [PATCH net-next v2 2/6] rust: time: Introduce Delta type

[FUJITA Tomonori]

On Mon, 7 Oct 2024 15:33:13 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> I thought that from_secs(u16) gives long enough duration but
>> how about the following APIs?
>> 
>> pub fn from_nanos(nanos: u64)
>> pub fn from_micros(micros: u32)
>> pub fn from_millis(millis: u16) 
>> 
>> You can create the maximum via from_nanos. from_micros and from_millis
>> don't cause wrapping.
> 
> When i talked about transitive types, i was meaning that to_nanos(),
> to_micros(), to_millis() should have the same type as from_nanos(),
> to_micros(), and to_millis().
> 
> It is clear these APIs cause discard. millis is a lot less accurate
> than nanos. Which is fine, the names make that obvious. But what about
> the range? Are there values i can create using from_nanos() which i
> cannot then use to_millis() on because it overflows the u16? And i
> guess the overflow point is different to to_micros()? This API feels
> inconsistent to me. This is why i suggested u64 is used
> everywhere. And we avoid the range issues, by artificially clamping to
> something which can be represented in all forms, so we have a uniform
> behaviour.

I'll use u64 for all in v3; The range is to u64::MAX in nanoseconds
for all the from_* functions.

Re: [PATCH net-next v2 2/6] rust: time: Introduce Delta type

[Gary Guo]

On Wed, 09 Oct 2024 23:00:15 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> On Mon, 7 Oct 2024 15:33:13 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> >> I thought that from_secs(u16) gives long enough duration but
> >> how about the following APIs?
> >> 
> >> pub fn from_nanos(nanos: u64)
> >> pub fn from_micros(micros: u32)
> >> pub fn from_millis(millis: u16) 
> >> 
> >> You can create the maximum via from_nanos. from_micros and from_millis
> >> don't cause wrapping.  
> > 
> > When i talked about transitive types, i was meaning that to_nanos(),
> > to_micros(), to_millis() should have the same type as from_nanos(),
> > to_micros(), and to_millis().
> > 
> > It is clear these APIs cause discard. millis is a lot less accurate
> > than nanos. Which is fine, the names make that obvious. But what about
> > the range? Are there values i can create using from_nanos() which i
> > cannot then use to_millis() on because it overflows the u16? And i
> > guess the overflow point is different to to_micros()? This API feels
> > inconsistent to me. This is why i suggested u64 is used
> > everywhere. And we avoid the range issues, by artificially clamping to
> > something which can be represented in all forms, so we have a uniform
> > behaviour.  
> 
> I'll use u64 for all in v3; The range is to u64::MAX in nanoseconds
> for all the from_* functions.

If you do, I'd recommend to call it `Duration` rather than `Delta`.
`Delta` sounds to me that it can represent a negative delta, where
`Duration` makes sense to be non-negative.

And it also makes sense that `kernel::time::Duration` is the replacement
of `core::time::Duration`.

Thanks,
Gary

Re: [PATCH net-next v2 2/6] rust: time: Introduce Delta type

[FUJITA Tomonori]

On Sat, 12 Oct 2024 19:56:52 +0100
Gary Guo <gary@garyguo.net> wrote:

>> I'll use u64 for all in v3; The range is to u64::MAX in nanoseconds
>> for all the from_* functions.
> 
> If you do, I'd recommend to call it `Duration` rather than `Delta`.
> `Delta` sounds to me that it can represent a negative delta, where
> `Duration` makes sense to be non-negative.
> 
> And it also makes sense that `kernel::time::Duration` is the replacement
> of `core::time::Duration`.

Ok, `Duration` also works for me. I had kinda impression that it's
better not to use `Duration`.

Re: [PATCH net-next v2 2/6] rust: time: Introduce Delta type

[FUJITA Tomonori]

On Sat, 5 Oct 2024 20:02:55 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

>> +/// A span of time.
>> +#[derive(Copy, Clone)]
>> +pub struct Delta {
>> +    nanos: i64,
> 
> Is there are use case for negative Deltas ? Should this be u64?

After investigating ktime_us_delta() and ktime_ms_delta() users, I
found that ten or so places which use nagative Deltas like:

timedout_ktime = ktime_add_us(ktime_get(), some_usecs);
// Do something that takes time
remaining = ktime_us_delta(timedout_ktime, ktime_get());
if (remaining > 0)
     fsleep(remaining)


Looks straightforward. On second thought, I feel it would be better to
support nagative Deltas. We could use i64 everywhere.

And i64 is more compatible with Ktime Delta APIs; ktime_us_delta and
ktime_ms_delta returns s64; we create Delta without type conversion.

Re: [PATCH net-next v2 2/6] rust: time: Introduce Delta type

[Andrew Lunn]

On Sat, Oct 05, 2024 at 09:25:27PM +0900, FUJITA Tomonori wrote:
> Introduce a type representing a span of time. Define our own type
> because `core::time::Duration` is large and could panic during
> creation.
> 
> We could use time::Ktime for time duration but timestamp and timedelta
> are different so better to use a new type.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/time.rs | 64 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
> 
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index c40105941a2c..6c5a1c50c5f1 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -8,9 +8,15 @@
>  //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
>  //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
>  
> +/// The number of nanoseconds per microsecond.
> +pub const NSEC_PER_USEC: i64 = bindings::NSEC_PER_USEC as i64;
> +
>  /// The number of nanoseconds per millisecond.
>  pub const NSEC_PER_MSEC: i64 = bindings::NSEC_PER_MSEC as i64;
>  
> +/// The number of nanoseconds per second.
> +pub const NSEC_PER_SEC: i64 = bindings::NSEC_PER_SEC as i64;
> +
>  /// The time unit of Linux kernel. One jiffy equals (1/HZ) second.
>  pub type Jiffies = core::ffi::c_ulong;
>  
> @@ -103,3 +109,61 @@ fn partial_cmp(&self, other: &Self) -> Option<core::cmp::Ordering> {
>          }
>      }
>  }
> +
> +/// A span of time.
> +#[derive(Copy, Clone)]
> +pub struct Delta {
> +    nanos: i64,
> +}
> +
> +impl Delta {
> +    /// Create a new `Delta` from a number of nanoseconds.
> +    #[inline]
> +    pub fn from_nanos(nanos: u16) -> Self {
> +        Self {
> +            nanos: nanos.into(),
> +        }
> +    }

Just throwing out an idea:

How about we clamp delay to ~1 year, with a pr_warn() if it needs to
actually clamp. All the APIs take or return a u64.

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

Another dumb rust question. How does the Rust compiler implement 64
bit division on 32 bit systems? GCC with C calls out to a library to
do it, and the kernel does not have that library. So you need to use
the kernel div_u64() function.

Did you compiler this code for a 32 bit system?

	Andrew

[PATCH net-next v2 3/6] rust: time: Implement addition of Ktime and Delta

[FUJITA Tomonori]

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

Ktime = Ktime + Delta

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

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers/time.c |  5 +++++
 rust/kernel/time.rs | 11 +++++++++++
 2 files changed, 16 insertions(+)

diff --git a/rust/helpers/time.c b/rust/helpers/time.c
index d6f61affb2c3..60dee69f4efc 100644
--- a/rust/helpers/time.c
+++ b/rust/helpers/time.c
@@ -2,6 +2,11 @@
 
 #include <linux/ktime.h>
 
+ktime_t rust_helper_ktime_add_ns(const ktime_t kt, const u64 nsec)
+{
+	return ktime_add_ns(kt, nsec);
+}
+
 int rust_helper_ktime_compare(const ktime_t cmp1, const ktime_t cmp2)
 {
 	return ktime_compare(cmp1, cmp2);
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 6c5a1c50c5f1..3e00ad22ed89 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -167,3 +167,14 @@ pub fn as_micros(self) -> i64 {
         self.nanos / NSEC_PER_USEC
     }
 }
+
+impl core::ops::Add<Delta> for Ktime {
+    type Output = Ktime;
+
+    #[inline]
+    fn add(self, delta: Delta) -> Ktime {
+        // SAFETY: FFI call.
+        let t = unsafe { bindings::ktime_add_ns(self.inner, delta.as_nanos() as u64) };
+        Ktime::from_raw(t)
+    }
+}
-- 
2.34.1

Re: [PATCH net-next v2 3/6] rust: time: Implement addition of Ktime and Delta

[Andrew Lunn]

On Sat, Oct 05, 2024 at 09:25:28PM +0900, FUJITA Tomonori wrote:
> Implement Add<Delta> for Ktime to support the operation:
> 
> Ktime = Ktime + Delta
> 
> This is used to calculate the future time when the timeout will occur.

Since Delta can be negative, it could also be a passed time. For a
timeout, that does not make much sense.

> +impl core::ops::Add<Delta> for Ktime {
> +    type Output = Ktime;
> +
> +    #[inline]
> +    fn add(self, delta: Delta) -> Ktime {
> +        // SAFETY: FFI call.
> +        let t = unsafe { bindings::ktime_add_ns(self.inner, delta.as_nanos() as u64) };

So you are throwing away the sign bit. What does Rust in the kernel do
if it was a negative delta?

I think the types being used here need more consideration.

	Andrew

Re: [PATCH net-next v2 3/6] rust: time: Implement addition of Ktime and Delta

[Fiona Behrens]

On 5 Oct 2024, at 20:07, Andrew Lunn wrote:

> On Sat, Oct 05, 2024 at 09:25:28PM +0900, FUJITA Tomonori wrote:
>> Implement Add<Delta> for Ktime to support the operation:
>>
>> Ktime = Ktime + Delta
>>
>> This is used to calculate the future time when the timeout will occur.
>
> Since Delta can be negative, it could also be a passed time. For a
> timeout, that does not make much sense.
>

Are there more usecases than Delta? Would it make sense in that case to also implement Sub as well?

Thanks,
Fiona

>> +impl core::ops::Add<Delta> for Ktime {
>> +    type Output = Ktime;
>> +
>> +    #[inline]
>> +    fn add(self, delta: Delta) -> Ktime {
>> +        // SAFETY: FFI call.
>> +        let t = unsafe { bindings::ktime_add_ns(self.inner, delta.as_nanos() as u64) };
>
> So you are throwing away the sign bit. What does Rust in the kernel do
> if it was a negative delta?
>
> I think the types being used here need more consideration.
>
>     Andrew

Re: [PATCH net-next v2 3/6] rust: time: Implement addition of Ktime and Delta

[FUJITA Tomonori]

On Sun, 06 Oct 2024 12:45:06 +0200
Fiona Behrens <me@kloenk.dev> wrote:

>> On Sat, Oct 05, 2024 at 09:25:28PM +0900, FUJITA Tomonori wrote:
>>> Implement Add<Delta> for Ktime to support the operation:
>>>
>>> Ktime = Ktime + Delta
>>>
>>> This is used to calculate the future time when the timeout will occur.
>>
>> Since Delta can be negative, it could also be a passed time. For a
>> timeout, that does not make much sense.
>>
> 
> Are there more usecases than Delta? Would it make sense in that case to also implement Sub as well?

We might add the api to calculate the elapsed time when it becomes
necessary:

Delta = Ktime - Ktime

Re: [PATCH net-next v2 3/6] rust: time: Implement addition of Ktime and Delta

[Miguel Ojeda]

On Sat, Oct 5, 2024 at 2:26 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> +    fn add(self, delta: Delta) -> Ktime {
> +        // SAFETY: FFI call.
> +        let t = unsafe { bindings::ktime_add_ns(self.inner, delta.as_nanos() as u64) };
> +        Ktime::from_raw(t)
> +    }

I wonder if we want to use the `ktime` macros/operations for this type
or not (even if we still promise it is the same type underneath). It
means having to define helpers, adding `unsafe` code and `SAFETY`
comments, a call penalty in non-LTO, losing overflow checking (if we
want it for these types), and so on.

(And at least C is `-fno-strict-overflow`, otherwise it would be even subtler).

Cheers,
Miguel

Re: [PATCH net-next v2 3/6] rust: time: Implement addition of Ktime and Delta

[FUJITA Tomonori]

On Sat, 5 Oct 2024 20:36:44 +0200
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Sat, Oct 5, 2024 at 2:26 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> +    fn add(self, delta: Delta) -> Ktime {
>> +        // SAFETY: FFI call.
>> +        let t = unsafe { bindings::ktime_add_ns(self.inner, delta.as_nanos() as u64) };
>> +        Ktime::from_raw(t)
>> +    }
> 
> I wonder if we want to use the `ktime` macros/operations for this type
> or not (even if we still promise it is the same type underneath). It
> means having to define helpers, adding `unsafe` code and `SAFETY`
> comments, a call penalty in non-LTO, losing overflow checking (if we
> want it for these types), and so on.

Yeah, if we are allowed to touch ktime_t directly instead of using the
accessors, it's great for the rust side.

The timers maintainers, what do you think?

Re: [PATCH net-next v2 3/6] rust: time: Implement addition of Ktime and Delta

[Alice Ryhl]

On Mon, Oct 7, 2024 at 8:17 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> On Sat, 5 Oct 2024 20:36:44 +0200
> Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
>
> > On Sat, Oct 5, 2024 at 2:26 PM FUJITA Tomonori
> > <fujita.tomonori@gmail.com> wrote:
> >>
> >> +    fn add(self, delta: Delta) -> Ktime {
> >> +        // SAFETY: FFI call.
> >> +        let t = unsafe { bindings::ktime_add_ns(self.inner, delta.as_nanos() as u64) };
> >> +        Ktime::from_raw(t)
> >> +    }
> >
> > I wonder if we want to use the `ktime` macros/operations for this type
> > or not (even if we still promise it is the same type underneath). It
> > means having to define helpers, adding `unsafe` code and `SAFETY`
> > comments, a call penalty in non-LTO, losing overflow checking (if we
> > want it for these types), and so on.
>
> Yeah, if we are allowed to touch ktime_t directly instead of using the
> accessors, it's great for the rust side.
>
> The timers maintainers, what do you think?

We already do that in the existing code. The Ktime::sub method touches
the ktime_t directly and performs a subtraction using the - operator
rather than call a ktime_ method for it.

Alice

Re: [PATCH net-next v2 3/6] rust: time: Implement addition of Ktime and Delta

[FUJITA Tomonori]

On Mon, 7 Oct 2024 16:24:28 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

>> > or not (even if we still promise it is the same type underneath). It
>> > means having to define helpers, adding `unsafe` code and `SAFETY`
>> > comments, a call penalty in non-LTO, losing overflow checking (if we
>> > want it for these types), and so on.
>>
>> Yeah, if we are allowed to touch ktime_t directly instead of using the
>> accessors, it's great for the rust side.
>>
>> The timers maintainers, what do you think?
> 
> We already do that in the existing code. The Ktime::sub method touches
> the ktime_t directly and performs a subtraction using the - operator
> rather than call a ktime_ method for it.

I'll touch ktime_t directly in the next version.

[PATCH net-next v2 4/6] rust: time: add wrapper for fsleep function

[FUJITA Tomonori]

Add a wrapper for fsleep, flexible sleep functions in
`include/linux/delay.h` which 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.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers/time.c |  6 ++++++
 rust/kernel/time.rs | 16 ++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/rust/helpers/time.c b/rust/helpers/time.c
index 60dee69f4efc..0c85bb06af63 100644
--- a/rust/helpers/time.c
+++ b/rust/helpers/time.c
@@ -1,7 +1,13 @@
 // SPDX-License-Identifier: GPL-2.0
 
+#include <linux/delay.h>
 #include <linux/ktime.h>
 
+void rust_helper_fsleep(unsigned long usecs)
+{
+	fsleep(usecs);
+}
+
 ktime_t rust_helper_ktime_add_ns(const ktime_t kt, const u64 nsec)
 {
 	return ktime_add_ns(kt, nsec);
diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 3e00ad22ed89..5cca9c60f74a 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -5,9 +5,12 @@
 //! This module contains the kernel APIs related to time and timers that
 //! have been ported or wrapped for usage by Rust code in the kernel.
 //!
+//! C header: [`include/linux/delay.h`](srctree/include/linux/delay.h).
 //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
 //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
 
+use core::ffi::c_ulong;
+
 /// The number of nanoseconds per microsecond.
 pub const NSEC_PER_USEC: i64 = bindings::NSEC_PER_USEC as i64;
 
@@ -178,3 +181,16 @@ fn add(self, delta: Delta) -> Ktime {
         Ktime::from_raw(t)
     }
 }
+
+/// Sleeps for a given duration.
+///
+/// Equivalent to the kernel's [`fsleep`], flexible sleep function,
+/// which automatically chooses the best sleep method based on a duration.
+///
+/// `Delta` must be longer than one microsecond.
+///
+/// This function can only be used in a nonatomic context.
+pub fn fsleep(delta: Delta) {
+    // SAFETY: FFI call.
+    unsafe { bindings::fsleep(delta.as_micros() as c_ulong) }
+}
-- 
2.34.1

Re: [PATCH net-next v2 4/6] rust: time: add wrapper for fsleep function

[Alice Ryhl]

On Sat, Oct 5, 2024 at 2:26 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Add a wrapper for fsleep, flexible sleep functions in
> `include/linux/delay.h` which 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.
>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/helpers/time.c |  6 ++++++
>  rust/kernel/time.rs | 16 ++++++++++++++++
>  2 files changed, 22 insertions(+)
>
> diff --git a/rust/helpers/time.c b/rust/helpers/time.c
> index 60dee69f4efc..0c85bb06af63 100644
> --- a/rust/helpers/time.c
> +++ b/rust/helpers/time.c
> @@ -1,7 +1,13 @@
>  // SPDX-License-Identifier: GPL-2.0
>
> +#include <linux/delay.h>
>  #include <linux/ktime.h>
>
> +void rust_helper_fsleep(unsigned long usecs)
> +{
> +       fsleep(usecs);
> +}
> +
>  ktime_t rust_helper_ktime_add_ns(const ktime_t kt, const u64 nsec)
>  {
>         return ktime_add_ns(kt, nsec);
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index 3e00ad22ed89..5cca9c60f74a 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -5,9 +5,12 @@
>  //! This module contains the kernel APIs related to time and timers that
>  //! have been ported or wrapped for usage by Rust code in the kernel.
>  //!
> +//! C header: [`include/linux/delay.h`](srctree/include/linux/delay.h).
>  //! C header: [`include/linux/jiffies.h`](srctree/include/linux/jiffies.h).
>  //! C header: [`include/linux/ktime.h`](srctree/include/linux/ktime.h).
>
> +use core::ffi::c_ulong;
> +
>  /// The number of nanoseconds per microsecond.
>  pub const NSEC_PER_USEC: i64 = bindings::NSEC_PER_USEC as i64;
>
> @@ -178,3 +181,16 @@ fn add(self, delta: Delta) -> Ktime {
>          Ktime::from_raw(t)
>      }
>  }
> +
> +/// Sleeps for a given duration.
> +///
> +/// Equivalent to the kernel's [`fsleep`], flexible sleep function,
> +/// which automatically chooses the best sleep method based on a duration.
> +///
> +/// `Delta` must be longer than one microsecond.
> +///
> +/// This function can only be used in a nonatomic context.
> +pub fn fsleep(delta: Delta) {
> +    // SAFETY: FFI call.
> +    unsafe { bindings::fsleep(delta.as_micros() as c_ulong) }
> +}

This rounds down. Should this round it up to the nearest microsecond
instead? It's generally said that fsleep should sleep for at least the
provided duration, but that it may sleep for longer under some
circumstances. By rounding up, you preserve that guarantee.

Also, the note about always sleeping for "at least" the duration may
be a good fit for the docs here as well.

Alice

Re: [PATCH net-next v2 4/6] rust: time: add wrapper for fsleep function

[FUJITA Tomonori]

On Mon, 7 Oct 2024 14:24:03 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

>> +/// Sleeps for a given duration.
>> +///
>> +/// Equivalent to the kernel's [`fsleep`], flexible sleep function,
>> +/// which automatically chooses the best sleep method based on a duration.
>> +///
>> +/// `Delta` must be longer than one microsecond.
>> +///
>> +/// This function can only be used in a nonatomic context.
>> +pub fn fsleep(delta: Delta) {
>> +    // SAFETY: FFI call.
>> +    unsafe { bindings::fsleep(delta.as_micros() as c_ulong) }
>> +}
> 
> This rounds down. Should this round it up to the nearest microsecond
> instead? It's generally said that fsleep should sleep for at least the
> provided duration, but that it may sleep for longer under some
> circumstances. By rounding up, you preserve that guarantee.

I'll round up in the next version.

> Also, the note about always sleeping for "at least" the duration may
> be a good fit for the docs here as well.

I see, will add it.

[PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[FUJITA Tomonori]

Add read_poll_timeout function which polls periodically until a
condition is met or a timeout is reached.

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

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

might_sleep() is called via a wrapper so the __FILE__ and __LINE__
debug info with CONFIG_DEBUG_ATOMIC_SLEEP enabled isn't what we
expect; the wrapper instead of the caller.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/helpers/helpers.c |  1 +
 rust/helpers/kernel.c  | 13 ++++++++
 rust/kernel/error.rs   |  1 +
 rust/kernel/io.rs      |  5 +++
 rust/kernel/io/poll.rs | 70 ++++++++++++++++++++++++++++++++++++++++++
 rust/kernel/lib.rs     |  1 +
 6 files changed, 91 insertions(+)
 create mode 100644 rust/helpers/kernel.c
 create mode 100644 rust/kernel/io.rs
 create mode 100644 rust/kernel/io/poll.rs

diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index c274546bcf78..f9569ff1717e 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -12,6 +12,7 @@
 #include "build_assert.c"
 #include "build_bug.c"
 #include "err.c"
+#include "kernel.c"
 #include "kunit.c"
 #include "mutex.c"
 #include "page.c"
diff --git a/rust/helpers/kernel.c b/rust/helpers/kernel.c
new file mode 100644
index 000000000000..5b9614974c76
--- /dev/null
+++ b/rust/helpers/kernel.c
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/kernel.h>
+
+void rust_helper_cpu_relax(void)
+{
+	cpu_relax();
+}
+
+void rust_helper_might_sleep(void)
+{
+	might_sleep();
+}
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs
index 6f1587a2524e..d571b9587ed6 100644
--- a/rust/kernel/error.rs
+++ b/rust/kernel/error.rs
@@ -58,6 +58,7 @@ macro_rules! declare_err {
     declare_err!(EPIPE, "Broken pipe.");
     declare_err!(EDOM, "Math argument out of domain of func.");
     declare_err!(ERANGE, "Math result not representable.");
+    declare_err!(ETIMEDOUT, "Connection timed out.");
     declare_err!(ERESTARTSYS, "Restart the system call.");
     declare_err!(ERESTARTNOINTR, "System call was interrupted by a signal and will be restarted.");
     declare_err!(ERESTARTNOHAND, "Restart if no handler.");
diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
new file mode 100644
index 000000000000..033f3c4e4adf
--- /dev/null
+++ b/rust/kernel/io.rs
@@ -0,0 +1,5 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Input and Output.
+
+pub mod poll;
diff --git a/rust/kernel/io/poll.rs b/rust/kernel/io/poll.rs
new file mode 100644
index 000000000000..d248a16a7158
--- /dev/null
+++ b/rust/kernel/io/poll.rs
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! IO polling.
+//!
+//! C header: [`include/linux/iopoll.h`](srctree/include/linux/iopoll.h).
+
+use crate::{
+    bindings,
+    error::{code::*, Result},
+    time::{fsleep, Delta, Ktime},
+};
+
+/// Polls periodically until a condition is met or a timeout is reached.
+///
+/// `op` is called repeatedly until `cond` returns `true` or the timeout is
+///  reached. The return value of `op` is passed to `cond`.
+///
+/// `sleep_delta` is the duration to sleep between calls to `op`.
+/// If `sleep_delta` is less than one microsecond, the function will busy-wait.
+///
+/// `timeout_delta` is the maximum time to wait for `cond` to return `true`.
+///
+/// If `sleep_before_read` is `true`, the function will sleep for `sleep_delta`
+/// first.
+///
+/// This function can only be used in a nonatomic context.
+pub fn read_poll_timeout<Op, Cond, T: Copy>(
+    mut op: Op,
+    cond: Cond,
+    sleep_delta: Delta,
+    timeout_delta: Delta,
+    sleep_before_read: bool,
+) -> Result<T>
+where
+    Op: FnMut() -> Result<T>,
+    Cond: Fn(T) -> bool,
+{
+    let timeout = Ktime::ktime_get() + timeout_delta;
+    let sleep = sleep_delta.as_micros() != 0;
+
+    if sleep {
+        // SAFETY: FFI call.
+        unsafe { bindings::might_sleep() }
+    }
+
+    if sleep_before_read && sleep {
+        fsleep(sleep_delta);
+    }
+
+    let val = loop {
+        let val = op()?;
+        if cond(val) {
+            break val;
+        }
+        if !timeout_delta.is_zero() && Ktime::ktime_get() > timeout {
+            break op()?;
+        }
+        if sleep {
+            fsleep(sleep_delta);
+        }
+        // SAFETY: FFI call.
+        unsafe { bindings::cpu_relax() }
+    };
+
+    if cond(val) {
+        Ok(val)
+    } else {
+        Err(ETIMEDOUT)
+    }
+}
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 22a3bfa5a9e9..7b6888723fc4 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -35,6 +35,7 @@
 #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
 pub mod firmware;
 pub mod init;
+pub mod io;
 pub mod ioctl;
 #[cfg(CONFIG_KUNIT)]
 pub mod kunit;
-- 
2.34.1

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Andrew Lunn]

> might_sleep() is called via a wrapper so the __FILE__ and __LINE__
> debug info with CONFIG_DEBUG_ATOMIC_SLEEP enabled isn't what we
> expect; the wrapper instead of the caller.

So not very useful. All we know is that somewhere in Rust something is
sleeping in atomic context. Is it possible to do better? Does __FILE__
and __LINE__ exist in Rust?

> +    if sleep {
> +        // SAFETY: FFI call.
> +        unsafe { bindings::might_sleep() }
> +    }

What is actually unsafe about might_sleep()? It is a void foo(void)
function, so takes no parameters, returns no results. It cannot affect
anything which Rust is managing.

> +        // SAFETY: FFI call.
> +        unsafe { bindings::cpu_relax() }

Same here.

	Andrew

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Boqun Feng]

On Sat, Oct 05, 2024 at 08:32:01PM +0200, Andrew Lunn wrote:
> > might_sleep() is called via a wrapper so the __FILE__ and __LINE__
> > debug info with CONFIG_DEBUG_ATOMIC_SLEEP enabled isn't what we
> > expect; the wrapper instead of the caller.
> 
> So not very useful. All we know is that somewhere in Rust something is
> sleeping in atomic context. Is it possible to do better? Does __FILE__
> and __LINE__ exist in Rust?
> 

Sure, you can use: 

	https://doc.rust-lang.org/core/macro.line.html

> > +    if sleep {
> > +        // SAFETY: FFI call.
> > +        unsafe { bindings::might_sleep() }
> > +    }
> 
> What is actually unsafe about might_sleep()? It is a void foo(void)

Every extern "C" function is by default unsafe, because C doesn't have
the concept of safe/unsafe. If you want to avoid unsafe, you could
introduce a Rust's might_sleep() which calls into
`bindings::might_sleep()`:

	pub fn might_sleep() {
	    // SAFETY: ??
	    unsafe { bindings::might_sleep() }
	}

however, if you call a might_sleep() in a preemption disabled context
when CONFIG_DEBUG_ATOMIC_SLEEP=n and PREEMPT=VOLUNTERY, it could means
an unexpected RCU quiescent state, which results an early RCU grace
period, and that may mean a use-after-free. So it's not that safe as you
may expected.

Regards,
Boqun

> function, so takes no parameters, returns no results. It cannot affect
> anything which Rust is managing.
> 
> > +        // SAFETY: FFI call.
> > +        unsafe { bindings::cpu_relax() }
> 
> Same here.
> 
> 	Andrew
> 

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Andrew Lunn]

On Sat, Oct 05, 2024 at 03:22:23PM -0700, Boqun Feng wrote:
> On Sat, Oct 05, 2024 at 08:32:01PM +0200, Andrew Lunn wrote:
> > > might_sleep() is called via a wrapper so the __FILE__ and __LINE__
> > > debug info with CONFIG_DEBUG_ATOMIC_SLEEP enabled isn't what we
> > > expect; the wrapper instead of the caller.
> > 
> > So not very useful. All we know is that somewhere in Rust something is
> > sleeping in atomic context. Is it possible to do better? Does __FILE__
> > and __LINE__ exist in Rust?
> > 
> 
> Sure, you can use: 
> 
> 	https://doc.rust-lang.org/core/macro.line.html

So i guess might_sleep() needs turning into some sort of macro, calling
__might_sleep(__FILE__, __LINE__); might_resched();

> > > +    if sleep {
> > > +        // SAFETY: FFI call.
> > > +        unsafe { bindings::might_sleep() }
> > > +    }
> > 
> > What is actually unsafe about might_sleep()? It is a void foo(void)
> 
> Every extern "C" function is by default unsafe, because C doesn't have
> the concept of safe/unsafe. If you want to avoid unsafe, you could
> introduce a Rust's might_sleep() which calls into
> `bindings::might_sleep()`:
> 
> 	pub fn might_sleep() {
> 	    // SAFETY: ??
> 	    unsafe { bindings::might_sleep() }
> 	}
> 
> however, if you call a might_sleep() in a preemption disabled context
> when CONFIG_DEBUG_ATOMIC_SLEEP=n and PREEMPT=VOLUNTERY, it could means
> an unexpected RCU quiescent state, which results an early RCU grace
> period, and that may mean a use-after-free. So it's not that safe as you
> may expected.

If you call might_sleep() in a preemption disabled context you code is
already unsafe, since that is the whole point of it, to find bugs
where you use a sleeping function in atomic context. Depending on why
you are in atomic context, it might appear to work, until it does not
actually work, and bad things happen. So it is not might_sleep() which
is unsafe, it is the Rust code calling it.

	Andrew

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[FUJITA Tomonori]

On Sun, 6 Oct 2024 16:45:21 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Sat, Oct 05, 2024 at 03:22:23PM -0700, Boqun Feng wrote:
>> On Sat, Oct 05, 2024 at 08:32:01PM +0200, Andrew Lunn wrote:
>> > > might_sleep() is called via a wrapper so the __FILE__ and __LINE__
>> > > debug info with CONFIG_DEBUG_ATOMIC_SLEEP enabled isn't what we
>> > > expect; the wrapper instead of the caller.
>> > 
>> > So not very useful. All we know is that somewhere in Rust something is
>> > sleeping in atomic context. Is it possible to do better? Does __FILE__
>> > and __LINE__ exist in Rust?
>> > 
>> 
>> Sure, you can use: 
>> 
>> 	https://doc.rust-lang.org/core/macro.line.html
> 
> So i guess might_sleep() needs turning into some sort of macro, calling
> __might_sleep(__FILE__, __LINE__); might_resched();

Yeah, I think we could do such.

Or we could drop the might_sleep call here? We might be able to expect
the improvement C support in Rust in the future.

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Boqun Feng]

On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote:
[...]
> > > > +    if sleep {
> > > > +        // SAFETY: FFI call.
> > > > +        unsafe { bindings::might_sleep() }
> > > > +    }
> > > 
> > > What is actually unsafe about might_sleep()? It is a void foo(void)
> > 
> > Every extern "C" function is by default unsafe, because C doesn't have
> > the concept of safe/unsafe. If you want to avoid unsafe, you could
> > introduce a Rust's might_sleep() which calls into
> > `bindings::might_sleep()`:
> > 
> > 	pub fn might_sleep() {
> > 	    // SAFETY: ??
> > 	    unsafe { bindings::might_sleep() }
> > 	}
> > 
> > however, if you call a might_sleep() in a preemption disabled context
> > when CONFIG_DEBUG_ATOMIC_SLEEP=n and PREEMPT=VOLUNTERY, it could means
> > an unexpected RCU quiescent state, which results an early RCU grace
> > period, and that may mean a use-after-free. So it's not that safe as you
> > may expected.
> 
> If you call might_sleep() in a preemption disabled context you code is
> already unsafe, since that is the whole point of it, to find bugs

Well, in Rust, the rule is: any type-checked (compiled successfully)
code that only calls safe Rust functions cannot be unsafe. So the fact
that calling might_sleep() in a preemption disabled context is unsafe
means that something has to be unsafe.

This eventually can turn into a "blaming game" in the design space: we
can either design the preemption disable function as unsafe or the
might_sleep() function as unsafe. But one of them has to be unsafe
function, otherwise we are breaking the safe code guarantee.

However, this is actually a special case: currently we want to use klint
[1] to detect all context mis-matches at compile time. So the above rule
extends for kernel: any type-checked *and klint-checked* code that only
calls safe Rust functions cannot be unsafe. I.e. we add additional
compile time checking for unsafe code. So if might_sleep() has the
proper klint annotation, and we actually enable klint for kernel code,
then we can make it safe (along with preemption disable functions being
safe).

> where you use a sleeping function in atomic context. Depending on why
> you are in atomic context, it might appear to work, until it does not
> actually work, and bad things happen. So it is not might_sleep() which
> is unsafe, it is the Rust code calling it.

The whole point of unsafe functions is that calling it may result into
unsafe code, so that's why all extern "C" functions are unsafe, so are
might_sleep() (without klint in the picture).


[1]: https://lwn.net/Articles/951550/

Regards,
Boqun

> 
> 	Andrew
> 
> 
> 
> 

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Andrew Lunn]

On Mon, Oct 07, 2024 at 05:28:28AM -0700, Boqun Feng wrote:
> On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote:
> [...]
> > > > > +    if sleep {
> > > > > +        // SAFETY: FFI call.
> > > > > +        unsafe { bindings::might_sleep() }
> > > > > +    }
> > > > 
> > > > What is actually unsafe about might_sleep()? It is a void foo(void)
> > > 
> > > Every extern "C" function is by default unsafe, because C doesn't have
> > > the concept of safe/unsafe. If you want to avoid unsafe, you could
> > > introduce a Rust's might_sleep() which calls into
> > > `bindings::might_sleep()`:
> > > 
> > > 	pub fn might_sleep() {
> > > 	    // SAFETY: ??
> > > 	    unsafe { bindings::might_sleep() }
> > > 	}
> > > 
> > > however, if you call a might_sleep() in a preemption disabled context
> > > when CONFIG_DEBUG_ATOMIC_SLEEP=n and PREEMPT=VOLUNTERY, it could means
> > > an unexpected RCU quiescent state, which results an early RCU grace
> > > period, and that may mean a use-after-free. So it's not that safe as you
> > > may expected.
> > 
> > If you call might_sleep() in a preemption disabled context you code is
> > already unsafe, since that is the whole point of it, to find bugs
> 
> Well, in Rust, the rule is: any type-checked (compiled successfully)
> code that only calls safe Rust functions cannot be unsafe. So the fact
> that calling might_sleep() in a preemption disabled context is unsafe
> means that something has to be unsafe.
> 
> This eventually can turn into a "blaming game" in the design space: we
> can either design the preemption disable function as unsafe or the
> might_sleep() function as unsafe. But one of them has to be unsafe
> function, otherwise we are breaking the safe code guarantee.

Just keep in mind, it could of been C which put you into atomic
context before calling into Rust. An interrupt handler would be a good
example, and i'm sure there are others.

> However, this is actually a special case: currently we want to use klint
> [1] to detect all context mis-matches at compile time. So the above rule
> extends for kernel: any type-checked *and klint-checked* code that only
> calls safe Rust functions cannot be unsafe. I.e. we add additional
> compile time checking for unsafe code. So if might_sleep() has the
> proper klint annotation, and we actually enable klint for kernel code,
> then we can make it safe (along with preemption disable functions being
> safe).
> 
> > where you use a sleeping function in atomic context. Depending on why
> > you are in atomic context, it might appear to work, until it does not
> > actually work, and bad things happen. So it is not might_sleep() which
> > is unsafe, it is the Rust code calling it.
> 
> The whole point of unsafe functions is that calling it may result into
> unsafe code, so that's why all extern "C" functions are unsafe, so are
> might_sleep() (without klint in the picture).

There is a psychological part to this. might_sleep() is a good debug
tool, which costs very little in normal builds, but finds logic bugs
when enabled in debug builds. What we don't want is Rust developers
not scattering it though their code because it adds unsafe code, and
the aim is not to have any unsafe code.

	Andrew

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Boqun Feng]

On Mon, Oct 07, 2024 at 03:48:09PM +0200, Andrew Lunn wrote:
> On Mon, Oct 07, 2024 at 05:28:28AM -0700, Boqun Feng wrote:
> > On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote:
> > [...]
> > > > > > +    if sleep {
> > > > > > +        // SAFETY: FFI call.
> > > > > > +        unsafe { bindings::might_sleep() }
> > > > > > +    }
> > > > > 
> > > > > What is actually unsafe about might_sleep()? It is a void foo(void)
> > > > 
> > > > Every extern "C" function is by default unsafe, because C doesn't have
> > > > the concept of safe/unsafe. If you want to avoid unsafe, you could
> > > > introduce a Rust's might_sleep() which calls into
> > > > `bindings::might_sleep()`:
> > > > 
> > > > 	pub fn might_sleep() {
> > > > 	    // SAFETY: ??
> > > > 	    unsafe { bindings::might_sleep() }
> > > > 	}
> > > > 
> > > > however, if you call a might_sleep() in a preemption disabled context
> > > > when CONFIG_DEBUG_ATOMIC_SLEEP=n and PREEMPT=VOLUNTERY, it could means
> > > > an unexpected RCU quiescent state, which results an early RCU grace
> > > > period, and that may mean a use-after-free. So it's not that safe as you
> > > > may expected.
> > > 
> > > If you call might_sleep() in a preemption disabled context you code is
> > > already unsafe, since that is the whole point of it, to find bugs
> > 
> > Well, in Rust, the rule is: any type-checked (compiled successfully)
> > code that only calls safe Rust functions cannot be unsafe. So the fact
> > that calling might_sleep() in a preemption disabled context is unsafe
> > means that something has to be unsafe.
> > 
> > This eventually can turn into a "blaming game" in the design space: we
> > can either design the preemption disable function as unsafe or the
> > might_sleep() function as unsafe. But one of them has to be unsafe
> > function, otherwise we are breaking the safe code guarantee.
> 
> Just keep in mind, it could of been C which put you into atomic
> context before calling into Rust. An interrupt handler would be a good
> example, and i'm sure there are others.
> 

That's why the klint approach is preferred right now. Without klint, and
if we don't want to mark might_sleep() as unsafe, we probably need to
mark the registration of an interrupt handler unsafe, and the safety
requirement would be "making sure the handler doesn't call schedule()".

> > However, this is actually a special case: currently we want to use klint
> > [1] to detect all context mis-matches at compile time. So the above rule
> > extends for kernel: any type-checked *and klint-checked* code that only
> > calls safe Rust functions cannot be unsafe. I.e. we add additional
> > compile time checking for unsafe code. So if might_sleep() has the
> > proper klint annotation, and we actually enable klint for kernel code,
> > then we can make it safe (along with preemption disable functions being
> > safe).
> > 
> > > where you use a sleeping function in atomic context. Depending on why
> > > you are in atomic context, it might appear to work, until it does not
> > > actually work, and bad things happen. So it is not might_sleep() which
> > > is unsafe, it is the Rust code calling it.
> > 
> > The whole point of unsafe functions is that calling it may result into
> > unsafe code, so that's why all extern "C" functions are unsafe, so are
> > might_sleep() (without klint in the picture).
> 
> There is a psychological part to this. might_sleep() is a good debug
> tool, which costs very little in normal builds, but finds logic bugs
> when enabled in debug builds. What we don't want is Rust developers
> not scattering it though their code because it adds unsafe code, and
> the aim is not to have any unsafe code.
> 

Sure, but my point is these need to be put together into a proper
design. For example, spin_lock() is currently exposed into Rust as a
safe API lock(), so the following code is unsafe:

	let g = lock1.lock(); // lock1 is a spinlock
	might_sleep();
	drop(g);

without the klint rule, if we want to mark might_sleep() as safe, then
we need to mark lock() as unsafe, otherwise, it's an unsafe code block
constructed by pure safe functions. However, compared to might_sleep(),
I think we would like keep lock() as safe since it is used more widely.

Regards,
Boqun

> 	Andrew

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Alice Ryhl]

On Mon, Oct 7, 2024 at 3:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Oct 07, 2024 at 05:28:28AM -0700, Boqun Feng wrote:
> > On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote:
> > However, this is actually a special case: currently we want to use klint
> > [1] to detect all context mis-matches at compile time. So the above rule
> > extends for kernel: any type-checked *and klint-checked* code that only
> > calls safe Rust functions cannot be unsafe. I.e. we add additional
> > compile time checking for unsafe code. So if might_sleep() has the
> > proper klint annotation, and we actually enable klint for kernel code,
> > then we can make it safe (along with preemption disable functions being
> > safe).
> >
> > > where you use a sleeping function in atomic context. Depending on why
> > > you are in atomic context, it might appear to work, until it does not
> > > actually work, and bad things happen. So it is not might_sleep() which
> > > is unsafe, it is the Rust code calling it.
> >
> > The whole point of unsafe functions is that calling it may result into
> > unsafe code, so that's why all extern "C" functions are unsafe, so are
> > might_sleep() (without klint in the picture).
>
> There is a psychological part to this. might_sleep() is a good debug
> tool, which costs very little in normal builds, but finds logic bugs
> when enabled in debug builds. What we don't want is Rust developers
> not scattering it though their code because it adds unsafe code, and
> the aim is not to have any unsafe code.

We can add a safe wrapper for it:

pub fn might_sleep() {
    // SAFETY: Always safe to call.
    unsafe { bindings::might_sleep() };
}

Alice

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Boqun Feng]

On Mon, Oct 07, 2024 at 04:08:48PM +0200, Alice Ryhl wrote:
> On Mon, Oct 7, 2024 at 3:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Mon, Oct 07, 2024 at 05:28:28AM -0700, Boqun Feng wrote:
> > > On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote:
> > > However, this is actually a special case: currently we want to use klint
> > > [1] to detect all context mis-matches at compile time. So the above rule
> > > extends for kernel: any type-checked *and klint-checked* code that only
> > > calls safe Rust functions cannot be unsafe. I.e. we add additional
> > > compile time checking for unsafe code. So if might_sleep() has the
> > > proper klint annotation, and we actually enable klint for kernel code,
> > > then we can make it safe (along with preemption disable functions being
> > > safe).
> > >
> > > > where you use a sleeping function in atomic context. Depending on why
> > > > you are in atomic context, it might appear to work, until it does not
> > > > actually work, and bad things happen. So it is not might_sleep() which
> > > > is unsafe, it is the Rust code calling it.
> > >
> > > The whole point of unsafe functions is that calling it may result into
> > > unsafe code, so that's why all extern "C" functions are unsafe, so are
> > > might_sleep() (without klint in the picture).
> >
> > There is a psychological part to this. might_sleep() is a good debug
> > tool, which costs very little in normal builds, but finds logic bugs
> > when enabled in debug builds. What we don't want is Rust developers
> > not scattering it though their code because it adds unsafe code, and
> > the aim is not to have any unsafe code.
> 
> We can add a safe wrapper for it:
> 
> pub fn might_sleep() {
>     // SAFETY: Always safe to call.
>     unsafe { bindings::might_sleep() };

It's not always safe to call, because might_sleep() has a
might_resched() and in preempt=voluntary kernel, that's a
cond_resched(), which may eventually call __schedule() and report a
quiescent state of RCU. This could means an unexpected early grace
period, and that means a potential use-afer-free.

Regards,
Boqun

> }
> 
> Alice

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Alice Ryhl]

On Mon, Oct 7, 2024 at 4:14 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> On Mon, Oct 07, 2024 at 04:08:48PM +0200, Alice Ryhl wrote:
> > On Mon, Oct 7, 2024 at 3:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Mon, Oct 07, 2024 at 05:28:28AM -0700, Boqun Feng wrote:
> > > > On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote:
> > > > However, this is actually a special case: currently we want to use klint
> > > > [1] to detect all context mis-matches at compile time. So the above rule
> > > > extends for kernel: any type-checked *and klint-checked* code that only
> > > > calls safe Rust functions cannot be unsafe. I.e. we add additional
> > > > compile time checking for unsafe code. So if might_sleep() has the
> > > > proper klint annotation, and we actually enable klint for kernel code,
> > > > then we can make it safe (along with preemption disable functions being
> > > > safe).
> > > >
> > > > > where you use a sleeping function in atomic context. Depending on why
> > > > > you are in atomic context, it might appear to work, until it does not
> > > > > actually work, and bad things happen. So it is not might_sleep() which
> > > > > is unsafe, it is the Rust code calling it.
> > > >
> > > > The whole point of unsafe functions is that calling it may result into
> > > > unsafe code, so that's why all extern "C" functions are unsafe, so are
> > > > might_sleep() (without klint in the picture).
> > >
> > > There is a psychological part to this. might_sleep() is a good debug
> > > tool, which costs very little in normal builds, but finds logic bugs
> > > when enabled in debug builds. What we don't want is Rust developers
> > > not scattering it though their code because it adds unsafe code, and
> > > the aim is not to have any unsafe code.
> >
> > We can add a safe wrapper for it:
> >
> > pub fn might_sleep() {
> >     // SAFETY: Always safe to call.
> >     unsafe { bindings::might_sleep() };
>
> It's not always safe to call, because might_sleep() has a
> might_resched() and in preempt=voluntary kernel, that's a
> cond_resched(), which may eventually call __schedule() and report a
> quiescent state of RCU. This could means an unexpected early grace
> period, and that means a potential use-afer-free.

Atomicity violations are intended to be caught by klint. If you want
to change that decision, you'll have to add unsafe to all functions
that sleep including Mutex::lock, CondVar::wait, and many others.

Alice

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Boqun Feng]

On Mon, Oct 07, 2024 at 04:16:46PM +0200, Alice Ryhl wrote:
> On Mon, Oct 7, 2024 at 4:14 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > On Mon, Oct 07, 2024 at 04:08:48PM +0200, Alice Ryhl wrote:
> > > On Mon, Oct 7, 2024 at 3:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > >
> > > > On Mon, Oct 07, 2024 at 05:28:28AM -0700, Boqun Feng wrote:
> > > > > On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote:
> > > > > However, this is actually a special case: currently we want to use klint
> > > > > [1] to detect all context mis-matches at compile time. So the above rule
> > > > > extends for kernel: any type-checked *and klint-checked* code that only
> > > > > calls safe Rust functions cannot be unsafe. I.e. we add additional
> > > > > compile time checking for unsafe code. So if might_sleep() has the
> > > > > proper klint annotation, and we actually enable klint for kernel code,
> > > > > then we can make it safe (along with preemption disable functions being
> > > > > safe).
> > > > >
> > > > > > where you use a sleeping function in atomic context. Depending on why
> > > > > > you are in atomic context, it might appear to work, until it does not
> > > > > > actually work, and bad things happen. So it is not might_sleep() which
> > > > > > is unsafe, it is the Rust code calling it.
> > > > >
> > > > > The whole point of unsafe functions is that calling it may result into
> > > > > unsafe code, so that's why all extern "C" functions are unsafe, so are
> > > > > might_sleep() (without klint in the picture).
> > > >
> > > > There is a psychological part to this. might_sleep() is a good debug
> > > > tool, which costs very little in normal builds, but finds logic bugs
> > > > when enabled in debug builds. What we don't want is Rust developers
> > > > not scattering it though their code because it adds unsafe code, and
> > > > the aim is not to have any unsafe code.
> > >
> > > We can add a safe wrapper for it:
> > >
> > > pub fn might_sleep() {
> > >     // SAFETY: Always safe to call.
> > >     unsafe { bindings::might_sleep() };
> >
> > It's not always safe to call, because might_sleep() has a
> > might_resched() and in preempt=voluntary kernel, that's a
> > cond_resched(), which may eventually call __schedule() and report a
> > quiescent state of RCU. This could means an unexpected early grace
> > period, and that means a potential use-afer-free.
> 
> Atomicity violations are intended to be caught by klint. If you want

Yes, I already mentioned this to Andrew previously.

> to change that decision, you'll have to add unsafe to all functions
> that sleep including Mutex::lock, CondVar::wait, and many others.

No, I'm not trying to change that decision, just to make it clear that
we can mark might_sleep() as safe because of the decision, not because
it's really safe even without klint...

Regards,
Boqun

> 
> Alice

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Boqun Feng]

On Mon, Oct 07, 2024 at 07:19:56AM -0700, Boqun Feng wrote:
> On Mon, Oct 07, 2024 at 04:16:46PM +0200, Alice Ryhl wrote:
> > On Mon, Oct 7, 2024 at 4:14 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> > >
> > > On Mon, Oct 07, 2024 at 04:08:48PM +0200, Alice Ryhl wrote:
> > > > On Mon, Oct 7, 2024 at 3:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > > >
> > > > > On Mon, Oct 07, 2024 at 05:28:28AM -0700, Boqun Feng wrote:
> > > > > > On Sun, Oct 06, 2024 at 04:45:21PM +0200, Andrew Lunn wrote:
> > > > > > However, this is actually a special case: currently we want to use klint
> > > > > > [1] to detect all context mis-matches at compile time. So the above rule
> > > > > > extends for kernel: any type-checked *and klint-checked* code that only
> > > > > > calls safe Rust functions cannot be unsafe. I.e. we add additional
> > > > > > compile time checking for unsafe code. So if might_sleep() has the
> > > > > > proper klint annotation, and we actually enable klint for kernel code,
> > > > > > then we can make it safe (along with preemption disable functions being
> > > > > > safe).
> > > > > >
> > > > > > > where you use a sleeping function in atomic context. Depending on why
> > > > > > > you are in atomic context, it might appear to work, until it does not
> > > > > > > actually work, and bad things happen. So it is not might_sleep() which
> > > > > > > is unsafe, it is the Rust code calling it.
> > > > > >
> > > > > > The whole point of unsafe functions is that calling it may result into
> > > > > > unsafe code, so that's why all extern "C" functions are unsafe, so are
> > > > > > might_sleep() (without klint in the picture).
> > > > >
> > > > > There is a psychological part to this. might_sleep() is a good debug
> > > > > tool, which costs very little in normal builds, but finds logic bugs
> > > > > when enabled in debug builds. What we don't want is Rust developers
> > > > > not scattering it though their code because it adds unsafe code, and
> > > > > the aim is not to have any unsafe code.
> > > >
> > > > We can add a safe wrapper for it:
> > > >
> > > > pub fn might_sleep() {
> > > >     // SAFETY: Always safe to call.
> > > >     unsafe { bindings::might_sleep() };
> > >
> > > It's not always safe to call, because might_sleep() has a
> > > might_resched() and in preempt=voluntary kernel, that's a
> > > cond_resched(), which may eventually call __schedule() and report a
> > > quiescent state of RCU. This could means an unexpected early grace
> > > period, and that means a potential use-afer-free.
> > 
> > Atomicity violations are intended to be caught by klint. If you want
> 
> Yes, I already mentioned this to Andrew previously.
> 
> > to change that decision, you'll have to add unsafe to all functions
> > that sleep including Mutex::lock, CondVar::wait, and many others.
> 
> No, I'm not trying to change that decision, just to make it clear that
> we can mark might_sleep() as safe because of the decision, not because
> it's really safe even without klint...
> 

Anyway, I think Tomo needs to call __might_sleep() instead of
might_sleep(), and __might_sleep() seems a pure debug function (not
involved with schedule at all). So the wrapper of __might_sleep() can be
perfectly safe.

Regards,
Boqun

> Regards,
> Boqun
> 
> > 
> > Alice

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Andrew Lunn]

> > pub fn might_sleep() {
> >     // SAFETY: Always safe to call.
> >     unsafe { bindings::might_sleep() };
> 
> It's not always safe to call, because might_sleep() has a
> might_resched() and in preempt=voluntary kernel, that's a
> cond_resched(), which may eventually call __schedule() and report a
> quiescent state of RCU. This could means an unexpected early grace
> period, and that means a potential use-afer-free.

How does C handle this?

I'm not an RCU person...

But if you have called might_sleep() you are about to do something
which could sleep. If it does sleep, the scheduler is going to be
called, the grace period has ended, and RCU is going to do its
thing. If that results in a use-after-free, your code is
broken. might_sleep makes no difference here, the code is still
broken, it just happens to light the fuse for the explosion a bit
earlier.

Or, i'm missing something, not being an RCU person.

	Andrew

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Boqun Feng]

On Mon, Oct 07, 2024 at 07:13:40PM +0200, Andrew Lunn wrote:
> > > pub fn might_sleep() {
> > >     // SAFETY: Always safe to call.
> > >     unsafe { bindings::might_sleep() };
> > 
> > It's not always safe to call, because might_sleep() has a
> > might_resched() and in preempt=voluntary kernel, that's a
> > cond_resched(), which may eventually call __schedule() and report a
> > quiescent state of RCU. This could means an unexpected early grace
> > period, and that means a potential use-afer-free.
> 
> How does C handle this?
> 
> I'm not an RCU person...
> 
> But if you have called might_sleep() you are about to do something
> which could sleep. If it does sleep, the scheduler is going to be
> called, the grace period has ended, and RCU is going to do its
> thing. If that results in a use-after-free, your code is
> broken. might_sleep makes no difference here, the code is still
> broken, it just happens to light the fuse for the explosion a bit
> earlier.
> 

Because of the might_resched() in might_sleep(), it will report the
quiescent state of the current CPU, and RCU will pass a grace period if
all CPUs have passed a quiescent state. So for example if someone writes
the following:

    <reader>			<updater>
    rcu_read_lock();
    p = rcu_dereference(gp);
    might_sleep():
      might_resched():
				todo = gp;
				rcu_assign_pointer(gp, NULL);
				synchronize_rcu();

        rcu_all_qs(); // report a quiescent state inside RCU read-side
	              // critical section, which may make a grace period
		      // pass even there is an active RCU reader

				kfree(todo);

    a = READ_ONCE(p->a); // UAF
    rcu_read_unlock();

We probably call the reader side code a "wrong annotation", however,
it's still unsafe code because of the UAF. Also you seems to assume that
might_sleep() is always attached to a sleepable function, which is not
an invalid assumption, but we couldn't use it for reasoning the
safe/unsafe property of Rust functions unless we can encode this in the
type system. For Rust code, without klint rule, might_sleep() needs to
be unsafe. So we have two options for might_sleep().

* Since we rely on klint for atomic context detection, we can mark the
  trivial wrapper (as what Alice presented in the other email) as safe,
  but we need to begin to add klint annotation for that function, unless
  Gary finds a smart way to auto-annotate functions.

* Instead of might_sleep(), we provide the wrapper of __might_sleep(),
  since it doesn't have might_resched() in it, it should be safe. And
  all we care about here is the debugging rather than voluntary context
  switch. (Besides I think preempt=volunatry is eventually going to be
  gone because of PREEMPT_AUTO [1], if that happens I think the
  might_resched() might be dropped entirely).

Does this make sense?

[1]: https://lore.kernel.org/lkml/20240528003521.979836-1-ankur.a.arora@oracle.com/

Regards,
Boqun

> Or, i'm missing something, not being an RCU person.
> 
> 	Andrew

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Andrew Lunn]

> Because of the might_resched() in might_sleep(), it will report the
> quiescent state of the current CPU, and RCU will pass a grace period if
> all CPUs have passed a quiescent state. So for example if someone writes
> the following:
> 
>     <reader>			<updater>
>     rcu_read_lock();
>     p = rcu_dereference(gp);
>     might_sleep():
>       might_resched():
> 				todo = gp;
> 				rcu_assign_pointer(gp, NULL);
> 				synchronize_rcu();
> 
>         rcu_all_qs(); // report a quiescent state inside RCU read-side
> 	              // critical section, which may make a grace period
> 		      // pass even there is an active RCU reader
> 
> 				kfree(todo);
> 

You are obviously missing something here. The call that actually sleeps

      mutex_lock(&lock)

>     a = READ_ONCE(p->a); // UAF
>     rcu_read_unlock();

A might_sleep() should be paired with something which does actually
sleep, under some condition.  At least, that is how it is used in C.
The iopoll being re-implemented here is an example of that.

So take the might_sleep out above, just leaving the mutex_lock. If the
mutex is uncontested, the code does not sleep and everything is O.K?
If it needs to wait for the mutex, it triggers a UAF.

The might_sleep() will also trigger a stack trace, if its is enabled,
because you are not allowed to sleep inside rcu_read_lock(), it is an
example of atomic context.

As far as i see, might_sleep() will cause UAF where there is going to
be a UAF anyway. If you are using it correctly, it does not cause UAF.

> We probably call the reader side code a "wrong annotation", however,
> it's still unsafe code because of the UAF. Also you seems to assume that
> might_sleep() is always attached to a sleepable function, which is not
> an invalid assumption, but we couldn't use it for reasoning the
> safe/unsafe property of Rust functions unless we can encode this in the
> type system.

How are any of the sleeping call encoded in the type system? I assume
any use of a mutex lock, sleep, wait for completion, etc are not all
marked as unsafe? There is some sort of wrapper around them? Why not
just extend that wrapper to might_sleep().

> For Rust code, without klint rule, might_sleep() needs to
> be unsafe. So we have two options for might_sleep().
> 
> * Since we rely on klint for atomic context detection, we can mark the
>   trivial wrapper (as what Alice presented in the other email) as safe,
>   but we need to begin to add klint annotation for that function, unless
>   Gary finds a smart way to auto-annotate functions.

Are there klint annotations for all sleeping functions?

> * Instead of might_sleep(), we provide the wrapper of __might_sleep(),
>   since it doesn't have might_resched() in it, it should be safe. And
>   all we care about here is the debugging rather than voluntary context
>   switch. (Besides I think preempt=volunatry is eventually going to be
>   gone because of PREEMPT_AUTO [1], if that happens I think the
>   might_resched() might be dropped entirely).

__might_sleep() might be safe, but your code is still broken and going
to UAF at some point. Don't you want that UAF to happen more reliably
and faster so you can find the issue? That would be the advantage of
might_sleep() over __might_sleep().

	Andrew

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Boqun Feng]

On Tue, Oct 08, 2024 at 02:12:51PM +0200, Andrew Lunn wrote:
> > Because of the might_resched() in might_sleep(), it will report the
> > quiescent state of the current CPU, and RCU will pass a grace period if
> > all CPUs have passed a quiescent state. So for example if someone writes
> > the following:
> > 
> >     <reader>			<updater>
> >     rcu_read_lock();
> >     p = rcu_dereference(gp);
> >     might_sleep():
> >       might_resched():
> > 				todo = gp;
> > 				rcu_assign_pointer(gp, NULL);
> > 				synchronize_rcu();
> > 
> >         rcu_all_qs(); // report a quiescent state inside RCU read-side
> > 	              // critical section, which may make a grace period
> > 		      // pass even there is an active RCU reader
> > 
> > 				kfree(todo);
> > 
> 
> You are obviously missing something here. The call that actually sleeps
> 
>       mutex_lock(&lock)
> 
> >     a = READ_ONCE(p->a); // UAF
> >     rcu_read_unlock();
> 
> A might_sleep() should be paired with something which does actually
> sleep, under some condition.  At least, that is how it is used in C.

How do you guarantee the "should" part? How can a compiler detect a
might_sleep() that doesn't have a paired "something which does actually
sleep"? I feel like we are just talking through each other, what I was
trying to say is might_sleep() is unsafe because the rule of Rust safe
code (if we don't consider klint) and I'm using an example here to
explain why. And when we are talking about the safe/unsafe attribute of
a function, we cannot use the reasoning "this function should be always
used with another function".

> The iopoll being re-implemented here is an example of that.
> 
> So take the might_sleep out above, just leaving the mutex_lock. If the
> mutex is uncontested, the code does not sleep and everything is O.K?
> If it needs to wait for the mutex, it triggers a UAF.
> 
> The might_sleep() will also trigger a stack trace, if its is enabled,
> because you are not allowed to sleep inside rcu_read_lock(), it is an
> example of atomic context.

These functionalities you mentioned above are also provided by
__might_sleep(), no?

> 
> As far as i see, might_sleep() will cause UAF where there is going to
> be a UAF anyway. If you are using it correctly, it does not cause UAF.
> 

Again, I agree with your assumption that might_sleep() will always be
paired with a sleep function, but we cannot mark might_sleep() as safe
because of that. We can, however, mark might_sleep() as safe because
klint is supposed to cover the detection of atomic context violations.
But we have a better option: __might_sleep().

> > We probably call the reader side code a "wrong annotation", however,
> > it's still unsafe code because of the UAF. Also you seems to assume that
> > might_sleep() is always attached to a sleepable function, which is not
> > an invalid assumption, but we couldn't use it for reasoning the
> > safe/unsafe property of Rust functions unless we can encode this in the
> > type system.
> 
> How are any of the sleeping call encoded in the type system? I assume

There's no easy way, something might work is introducing effect system
[1] into Rust, but that's very complicated and may take years. When
there's no easy way to encode something in the type system, it's usually
the time that unsafe comes to happen, an unsafe function can have a
requirement that cannot be easily detected by compilers, and via unsafe
block and safety comments, programmers provide the reasons why these
requirements are fulfilled.

> any use of a mutex lock, sleep, wait for completion, etc are not all
> marked as unsafe? There is some sort of wrapper around them? Why not

They are marked as safe because of the klint extension of safe Rust rule
I mentioned.

> just extend that wrapper to might_sleep().
> 
> > For Rust code, without klint rule, might_sleep() needs to
> > be unsafe. So we have two options for might_sleep().
> > 
> > * Since we rely on klint for atomic context detection, we can mark the
> >   trivial wrapper (as what Alice presented in the other email) as safe,
> >   but we need to begin to add klint annotation for that function, unless
> >   Gary finds a smart way to auto-annotate functions.
> 
> Are there klint annotations for all sleeping functions?
> 

Not yet, klint is still WIP. But we generally agree that atomic context
violations should be detected by klint (instead of making sleep
functions unsafe or using type system to encode sleep functions).

> > * Instead of might_sleep(), we provide the wrapper of __might_sleep(),
> >   since it doesn't have might_resched() in it, it should be safe. And
> >   all we care about here is the debugging rather than voluntary context
> >   switch. (Besides I think preempt=volunatry is eventually going to be
> >   gone because of PREEMPT_AUTO [1], if that happens I think the
> >   might_resched() might be dropped entirely).
> 
> __might_sleep() might be safe, but your code is still broken and going
> to UAF at some point. Don't you want that UAF to happen more reliably
> and faster so you can find the issue? That would be the advantage of
> might_sleep() over __might_sleep().
> 

Could you give me an example that might_sleep() can detect a bug while
__might_sleep() cannot? IIUC, __might_sleep() is the core of atomic
context detection in might_sleep(), so when CONFIG_DEBUG_ATOMIC_SLEEP=y,
__might_sleep() should detect all bugs that might_sleep() would detect.
Or you are talking about detecting even when
CONFIG_DEBUG_ATOMIC_SLEEP=n?

[1]: https://en.wikipedia.org/wiki/Effect_system

Regards,
Boqun

> 	Andrew

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Miguel Ojeda]

On Tue, Oct 8, 2024 at 2:13 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> As far as i see, might_sleep() will cause UAF where there is going to
> be a UAF anyway. If you are using it correctly, it does not cause UAF.

This already implies that it is an unsafe function (in general, i.e.
modulo klint, or a way to force the user to have to write `unsafe`
somewhere else, or what I call ASHes -- "acknowledged soundness
holes").

If we consider as safe functions that, if used correctly, do not cause
UB, then all functions would be safe.

Cheers,
Miguel

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Andrew Lunn]

On Tue, Oct 08, 2024 at 03:14:05PM +0200, Miguel Ojeda wrote:
> On Tue, Oct 8, 2024 at 2:13 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > As far as i see, might_sleep() will cause UAF where there is going to
> > be a UAF anyway. If you are using it correctly, it does not cause UAF.
> 
> This already implies that it is an unsafe function (in general, i.e.
> modulo klint, or a way to force the user to have to write `unsafe`
> somewhere else, or what I call ASHes -- "acknowledged soundness
> holes").
> 
> If we consider as safe functions that, if used correctly, do not cause
> UB, then all functions would be safe.

From what i hear, klint is still WIP. So we have to accept there will
be bad code out there, which will UAF. We want to find such bad code,
and the easiest way to find it at the moment is to make it UAF as fast
as possible. might_sleep() does that, __might_sleep() does not, and
using neither is the slowest way.

	Andrew

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Boqun Feng]

On Tue, Oct 08, 2024 at 07:16:42PM +0200, Andrew Lunn wrote:
> On Tue, Oct 08, 2024 at 03:14:05PM +0200, Miguel Ojeda wrote:
> > On Tue, Oct 8, 2024 at 2:13 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > As far as i see, might_sleep() will cause UAF where there is going to
> > > be a UAF anyway. If you are using it correctly, it does not cause UAF.
> > 
> > This already implies that it is an unsafe function (in general, i.e.
> > modulo klint, or a way to force the user to have to write `unsafe`
> > somewhere else, or what I call ASHes -- "acknowledged soundness
> > holes").
> > 
> > If we consider as safe functions that, if used correctly, do not cause
> > UB, then all functions would be safe.
> 
> From what i hear, klint is still WIP. So we have to accept there will
> be bad code out there, which will UAF. We want to find such bad code,

If you don't believe in klint, then we need to mark might_sleep() as
unsafe, as I already explain a million times, might_sleep() is unsafe
without the klint compile time check. You have to accept that an unsafe
function should really be marked as unsafe. And yes, in this way, all
sleep functions would be marked as unsafe as well (or we could mark all
preemption disable function as unsafe), but still an unsafe function is
unsafe.

Again, as Miguel mentioned, we can only mark might_sleep() because sleep
in atomic context is an ASH, not because it's really safe.

> and the easiest way to find it at the moment is to make it UAF as
> fast as possible. might_sleep() does that, __might_sleep() does not,
> and using neither is the slowest way.
> 

might_sleep() is useful because it checks preemption count and task
state, which is provided by __might_sleep() as well. I don't think
causing UAF helps we detect atomic context violation faster than what
__might_sleep() already have. Again, could you provide an example that
help me understand your reasoning here?

Regards,
Boqun

> 	Andrew

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Boqun Feng]

On Tue, Oct 08, 2024 at 02:53:56PM -0700, Boqun Feng wrote:
> On Tue, Oct 08, 2024 at 07:16:42PM +0200, Andrew Lunn wrote:
> > On Tue, Oct 08, 2024 at 03:14:05PM +0200, Miguel Ojeda wrote:
> > > On Tue, Oct 8, 2024 at 2:13 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > >
> > > > As far as i see, might_sleep() will cause UAF where there is going to
> > > > be a UAF anyway. If you are using it correctly, it does not cause UAF.
> > > 
> > > This already implies that it is an unsafe function (in general, i.e.
> > > modulo klint, or a way to force the user to have to write `unsafe`
> > > somewhere else, or what I call ASHes -- "acknowledged soundness
> > > holes").
> > > 
> > > If we consider as safe functions that, if used correctly, do not cause
> > > UB, then all functions would be safe.
> > 
> > From what i hear, klint is still WIP. So we have to accept there will
> > be bad code out there, which will UAF. We want to find such bad code,
> 
> If you don't believe in klint, then we need to mark might_sleep() as
> unsafe, as I already explain a million times, might_sleep() is unsafe
> without the klint compile time check. You have to accept that an unsafe
> function should really be marked as unsafe. And yes, in this way, all
> sleep functions would be marked as unsafe as well (or we could mark all
> preemption disable function as unsafe), but still an unsafe function is
> unsafe.
> 
> Again, as Miguel mentioned, we can only mark might_sleep() because sleep
> in atomic context is an ASH, not because it's really safe.
> 
> > and the easiest way to find it at the moment is to make it UAF as
> > fast as possible. might_sleep() does that, __might_sleep() does not,
> > and using neither is the slowest way.
> > 
> 
> might_sleep() is useful because it checks preemption count and task
> state, which is provided by __might_sleep() as well. I don't think
> causing UAF helps we detect atomic context violation faster than what
> __might_sleep() already have. Again, could you provide an example that
> help me understand your reasoning here?
> 

Another advantage of __might_sleep() is that it's already an exported
symbol, so we don't need to introduce a rust helper.

Regards,
Boqun

> Regards,
> Boqun
> 
> > 	Andrew

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Andrew Lunn]

On Tue, Oct 08, 2024 at 02:53:56PM -0700, Boqun Feng wrote:
> On Tue, Oct 08, 2024 at 07:16:42PM +0200, Andrew Lunn wrote:
> > On Tue, Oct 08, 2024 at 03:14:05PM +0200, Miguel Ojeda wrote:
> > > On Tue, Oct 8, 2024 at 2:13 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > >
> > > > As far as i see, might_sleep() will cause UAF where there is going to
> > > > be a UAF anyway. If you are using it correctly, it does not cause UAF.
> > > 
> > > This already implies that it is an unsafe function (in general, i.e.
> > > modulo klint, or a way to force the user to have to write `unsafe`
> > > somewhere else, or what I call ASHes -- "acknowledged soundness
> > > holes").
> > > 
> > > If we consider as safe functions that, if used correctly, do not cause
> > > UB, then all functions would be safe.
> > 
> > From what i hear, klint is still WIP. So we have to accept there will
> > be bad code out there, which will UAF. We want to find such bad code,
> 
> If you don't believe in klint

I did not say that. It is WIP, and without it i assume nothing is
detecting at compile time that the code is broken. Hence we need to
find the problem at runtime, which is what might_sleep() is all about.

> might_sleep() is useful because it checks preemption count and task
> state, which is provided by __might_sleep() as well. I don't think
> causing UAF helps we detect atomic context violation faster than what
> __might_sleep() already have. Again, could you provide an example that
> help me understand your reasoning here?

> > while (1) {
> >     <reader>                        <updater>
> >     rcu_read_lock();
> >     p = rcu_dereference(gp);
> >     mutex_lock(&lock)
> >     a = READ_ONCE(p->a);
> >     mutex_unlock(&lock)
> >     rcu_read_unlock();
> > }

The mutex lock is likely to be uncontested, so you don't sleep, and so
don't trigger the UAF. The code is clearly broken, but you survive.
Until the lock is contested, you do sleep, RCU falls apart, resulting
in a UAF.

Now if you used might_sleep(), every time you go around that loop you
do some of the same processing as actually sleeping, so are much more
likely to trigger the UAF.

might_sleep() as you pointed out, is also active when
CONFIG_DEBUG_ATOMIC_SLEEP is false. Thus it is also going to trigger
the broken code to UAF faster. And i expect a lot of testing is done
without CONFIG_DEBUG_ATOMIC_SLEEP and CONFIG_PROVE_LOCKING.

Once klint is completed, and detects all these problems at compile
time, we can then discard all this might_sleep stuff. But until then,
the faster code explodes, the more likely it is going to be quickly
and cheaply fixed.

	Andrew

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[Boqun Feng]

On Wed, Oct 09, 2024 at 12:26:00AM +0200, Andrew Lunn wrote:
> On Tue, Oct 08, 2024 at 02:53:56PM -0700, Boqun Feng wrote:
> > On Tue, Oct 08, 2024 at 07:16:42PM +0200, Andrew Lunn wrote:
> > > On Tue, Oct 08, 2024 at 03:14:05PM +0200, Miguel Ojeda wrote:
> > > > On Tue, Oct 8, 2024 at 2:13 PM Andrew Lunn <andrew@lunn.ch> wrote:
> > > > >
> > > > > As far as i see, might_sleep() will cause UAF where there is going to
> > > > > be a UAF anyway. If you are using it correctly, it does not cause UAF.
> > > > 
> > > > This already implies that it is an unsafe function (in general, i.e.
> > > > modulo klint, or a way to force the user to have to write `unsafe`
> > > > somewhere else, or what I call ASHes -- "acknowledged soundness
> > > > holes").
> > > > 
> > > > If we consider as safe functions that, if used correctly, do not cause
> > > > UB, then all functions would be safe.
> > > 
> > > From what i hear, klint is still WIP. So we have to accept there will
> > > be bad code out there, which will UAF. We want to find such bad code,
> > 
> > If you don't believe in klint
> 
> I did not say that. It is WIP, and without it i assume nothing is
> detecting at compile time that the code is broken. Hence we need to
> find the problem at runtime, which is what might_sleep() is all about.
> 
> > might_sleep() is useful because it checks preemption count and task
> > state, which is provided by __might_sleep() as well. I don't think
> > causing UAF helps we detect atomic context violation faster than what
> > __might_sleep() already have. Again, could you provide an example that
> > help me understand your reasoning here?
> 
> > > while (1) {
> > >     <reader>                        <updater>
> > >     rcu_read_lock();
> > >     p = rcu_dereference(gp);
> > >     mutex_lock(&lock)
> > >     a = READ_ONCE(p->a);
> > >     mutex_unlock(&lock)
> > >     rcu_read_unlock();
> > > }
> 
> The mutex lock is likely to be uncontested, so you don't sleep, and so
> don't trigger the UAF. The code is clearly broken, but you survive.
> Until the lock is contested, you do sleep, RCU falls apart, resulting
> in a UAF.
> 
> Now if you used might_sleep(), every time you go around that loop you
> do some of the same processing as actually sleeping, so are much more
> likely to trigger the UAF.
> 
> might_sleep() as you pointed out, is also active when
> CONFIG_DEBUG_ATOMIC_SLEEP is false. Thus it is also going to trigger
> the broken code to UAF faster. And i expect a lot of testing is done
> without CONFIG_DEBUG_ATOMIC_SLEEP and CONFIG_PROVE_LOCKING.
> 

Hmm.. but that means we need to quickly detect UAF and track down to the
source, right? In a build without CONFIG_DEBUG_ATOMIC_SLEEP and
CONFIG_PROVE_LOCKING, may I assume memory sanitizer is also not
available? Then how do we detect UAF relatively quickly? Or memory
sanitizer is in fact relatively cheap, so it can still be enabled,
what's the configuration of netdev CI/testing?

Regards,
Boqun

> Once klint is completed, and detects all these problems at compile
> time, we can then discard all this might_sleep stuff. But until then,
> the faster code explodes, the more likely it is going to be quickly
> and cheaply fixed.
> 
> 	Andrew

Re: [PATCH net-next v2 5/6] rust: Add read_poll_timeout function

[FUJITA Tomonori]

On Sat, 5 Oct 2024 15:22:23 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Sat, Oct 05, 2024 at 08:32:01PM +0200, Andrew Lunn wrote:
>> > might_sleep() is called via a wrapper so the __FILE__ and __LINE__
>> > debug info with CONFIG_DEBUG_ATOMIC_SLEEP enabled isn't what we
>> > expect; the wrapper instead of the caller.
>> 
>> So not very useful. All we know is that somewhere in Rust something is
>> sleeping in atomic context. Is it possible to do better? Does __FILE__
>> and __LINE__ exist in Rust?
>> 
> 
> Sure, you can use: 
> 
> 	https://doc.rust-lang.org/core/macro.line.html

To get the proper file name and line number with those macros, we need
to use __might_sleep() instead of might_sleep(). We could also call to
might_resched() there to emulate might_sleep() but fsleep() works
without it so I call only __might_sleep() in the next version.

[PATCH net-next v2 6/6] net: phy: qt2025: wait until PHY becomes ready

[FUJITA Tomonori]

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

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

diff --git a/drivers/net/phy/qt2025.rs b/drivers/net/phy/qt2025.rs
index 1ab065798175..b5925ff9f637 100644
--- a/drivers/net/phy/qt2025.rs
+++ b/drivers/net/phy/qt2025.rs
@@ -12,6 +12,7 @@
 use kernel::c_str;
 use kernel::error::code;
 use kernel::firmware::Firmware;
+use kernel::io::poll::read_poll_timeout;
 use kernel::net::phy::{
     self,
     reg::{Mmd, C45},
@@ -19,6 +20,7 @@
 };
 use kernel::prelude::*;
 use kernel::sizes::{SZ_16K, SZ_8K};
+use kernel::time::Delta;
 
 kernel::module_phy_driver! {
     drivers: [PhyQT2025],
@@ -93,7 +95,14 @@ fn probe(dev: &mut phy::Device) -> Result<()> {
         // The micro-controller will start running from SRAM.
         dev.write(C45::new(Mmd::PCS, 0xe854), 0x0040)?;
 
-        // TODO: sleep here until the hw becomes ready.
+        read_poll_timeout(
+            || dev.read(C45::new(Mmd::PCS, 0xd7fd)),
+            |val| val != 0x00 && val != 0x10,
+            Delta::from_millis(50),
+            Delta::from_secs(3),
+            false,
+        )?;
+
         Ok(())
     }
 
-- 
2.34.1

Re: [PATCH net-next v2 0/6] rust: Add IO polling

[Boqun Feng]

Hi Tomo,

On Sat, Oct 05, 2024 at 09:25:25PM +0900, FUJITA Tomonori wrote:
> Add Rust version of read_poll_timeout (include/linux/iopoll.h), which
> polls periodically until a condition is met or a timeout is reached.
> By using the function, the 6th patch fixes QT2025 PHY driver to sleep
> until the hardware becomes ready.
> 
> As a result of the past discussion, this introduces a new type
> representing a span of time instead of using core::time::Duration or
> time::Ktime.
> 

While, we are at it, I want to suggest that we also add
rust/kernel/time{.rs, /} into the "F:" entries of TIME subsystem like:

diff --git a/MAINTAINERS b/MAINTAINERS
index b77f4495dcf4..09e46a214333 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23376,6 +23376,8 @@ F:      kernel/time/timeconv.c
 F:     kernel/time/timecounter.c
 F:     kernel/time/timekeeping*
 F:     kernel/time/time_test.c
+F:     rust/kernel/time.rs
+F:     rust/kernel/time/
 F:     tools/testing/selftests/timers/

 TIPC NETWORK LAYER

This will help future contributers copy the correct people while
submission. Could you maybe add a patch of this in your series if this
sounds reasonable to you? Thanks!

Regards,
Boqun

> Unlike the old rust branch, This adds a wrapper for fsleep() instead
> of msleep(). fsleep() automatically chooses the best sleep method
> based on a duration.
> 
> v2:
> - Introduce time::Delta instead of core::time::Duration
> - Add some trait to Ktime for calculating timeout
> - Use read_poll_timeout in QT2025 driver instead of using fsleep directly
> v1: https://lore.kernel.org/netdev/20241001112512.4861-1-fujita.tomonori@gmail.com/
> 
> 
> FUJITA Tomonori (6):
>   rust: time: Implement PartialEq and PartialOrd for Ktime
>   rust: time: Introduce Delta type
>   rust: time: Implement addition of Ktime and Delta
>   rust: time: add wrapper for fsleep function
>   rust: Add read_poll_timeout function
>   net: phy: qt2025: wait until PHY becomes ready
> 
>  drivers/net/phy/qt2025.rs |  11 +++-
>  rust/helpers/helpers.c    |   2 +
>  rust/helpers/kernel.c     |  13 +++++
>  rust/helpers/time.c       |  19 +++++++
>  rust/kernel/error.rs      |   1 +
>  rust/kernel/io.rs         |   5 ++
>  rust/kernel/io/poll.rs    |  70 +++++++++++++++++++++++
>  rust/kernel/lib.rs        |   1 +
>  rust/kernel/time.rs       | 113 ++++++++++++++++++++++++++++++++++++++
>  9 files changed, 234 insertions(+), 1 deletion(-)
>  create mode 100644 rust/helpers/kernel.c
>  create mode 100644 rust/helpers/time.c
>  create mode 100644 rust/kernel/io.rs
>  create mode 100644 rust/kernel/io/poll.rs
> 
> 
> base-commit: d521db38f339709ccd23c5deb7663904e626c3a6
> -- 
> 2.34.1
> 
> 

Re: [PATCH net-next v2 0/6] rust: Add IO polling

[FUJITA Tomonori]

On Sat, 12 Oct 2024 08:29:06 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> While, we are at it, I want to suggest that we also add
> rust/kernel/time{.rs, /} into the "F:" entries of TIME subsystem like:
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b77f4495dcf4..09e46a214333 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23376,6 +23376,8 @@ F:      kernel/time/timeconv.c
>  F:     kernel/time/timecounter.c
>  F:     kernel/time/timekeeping*
>  F:     kernel/time/time_test.c
> +F:     rust/kernel/time.rs
> +F:     rust/kernel/time/
>  F:     tools/testing/selftests/timers/
> 
>  TIPC NETWORK LAYER
> 
> This will help future contributers copy the correct people while
> submission. Could you maybe add a patch of this in your series if this
> sounds reasonable to you? Thanks!

Agreed that it's better to have Rust time abstractions in
MAINTAINERS. You add it into the time entry but there are two options
in the file; time and timer?

TIMEKEEPING, CLOCKSOURCE CORE, NTP, ALARMTIMER
M:      John Stultz <jstultz@google.com>
M:      Thomas Gleixner <tglx@linutronix.de>
R:      Stephen Boyd <sboyd@kernel.org>

HIGH-RESOLUTION TIMERS, TIMER WHEEL, CLOCKEVENTS
M:      Anna-Maria Behnsen <anna-maria@linutronix.de>
M:      Frederic Weisbecker <frederic@kernel.org>
M:      Thomas Gleixner <tglx@linutronix.de>

The current Rust abstractions which play mainly with ktimer.h. it's 
not time, timer stuff, I think.

As planned, we'll move *.rs files from rust/kernel in the future,
how we handle time and timer abstractions?

Re: [PATCH net-next v2 0/6] rust: Add IO polling

[FUJITA Tomonori]

On Sun, 13 Oct 2024 10:15:05 +0900 (JST)
FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:

> On Sat, 12 Oct 2024 08:29:06 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
>> While, we are at it, I want to suggest that we also add
>> rust/kernel/time{.rs, /} into the "F:" entries of TIME subsystem like:
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index b77f4495dcf4..09e46a214333 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -23376,6 +23376,8 @@ F:      kernel/time/timeconv.c
>>  F:     kernel/time/timecounter.c
>>  F:     kernel/time/timekeeping*
>>  F:     kernel/time/time_test.c
>> +F:     rust/kernel/time.rs
>> +F:     rust/kernel/time/
>>  F:     tools/testing/selftests/timers/
>> 
>>  TIPC NETWORK LAYER
>> 
>> This will help future contributers copy the correct people while
>> submission. Could you maybe add a patch of this in your series if this
>> sounds reasonable to you? Thanks!
> 
> Agreed that it's better to have Rust time abstractions in
> MAINTAINERS. You add it into the time entry but there are two options
> in the file; time and timer?
> 
> TIMEKEEPING, CLOCKSOURCE CORE, NTP, ALARMTIMER
> M:      John Stultz <jstultz@google.com>
> M:      Thomas Gleixner <tglx@linutronix.de>
> R:      Stephen Boyd <sboyd@kernel.org>
> 
> HIGH-RESOLUTION TIMERS, TIMER WHEEL, CLOCKEVENTS
> M:      Anna-Maria Behnsen <anna-maria@linutronix.de>
> M:      Frederic Weisbecker <frederic@kernel.org>
> M:      Thomas Gleixner <tglx@linutronix.de>
> 
> The current Rust abstractions which play mainly with ktimer.h. it's 
> not time, timer stuff, I think.

Oops, s/ktimer.h/ktime.h/

No entry for ktime.h in MAINTAINERS; used by both time and timer
stuff.

> As planned, we'll move *.rs files from rust/kernel in the future,
> how we handle time and timer abstractions?

Looks like that we'll add rust/kernel/hrtimer/ soon. I feel that it's
better to decide on the layout of time and timer abstractions now
rather than later.

Re: [PATCH net-next v2 0/6] rust: Add IO polling

[Boqun Feng]

On Sun, Oct 13, 2024 at 11:50:33AM +0900, FUJITA Tomonori wrote:
> On Sun, 13 Oct 2024 10:15:05 +0900 (JST)
> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> 
> > On Sat, 12 Oct 2024 08:29:06 -0700
> > Boqun Feng <boqun.feng@gmail.com> wrote:
> > 
> >> While, we are at it, I want to suggest that we also add
> >> rust/kernel/time{.rs, /} into the "F:" entries of TIME subsystem like:
> >> 
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index b77f4495dcf4..09e46a214333 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -23376,6 +23376,8 @@ F:      kernel/time/timeconv.c
> >>  F:     kernel/time/timecounter.c
> >>  F:     kernel/time/timekeeping*
> >>  F:     kernel/time/time_test.c
> >> +F:     rust/kernel/time.rs
> >> +F:     rust/kernel/time/
> >>  F:     tools/testing/selftests/timers/
> >> 
> >>  TIPC NETWORK LAYER
> >> 
> >> This will help future contributers copy the correct people while
> >> submission. Could you maybe add a patch of this in your series if this
> >> sounds reasonable to you? Thanks!
> > 
> > Agreed that it's better to have Rust time abstractions in
> > MAINTAINERS. You add it into the time entry but there are two options
> > in the file; time and timer?
> > 
> > TIMEKEEPING, CLOCKSOURCE CORE, NTP, ALARMTIMER
> > M:      John Stultz <jstultz@google.com>
> > M:      Thomas Gleixner <tglx@linutronix.de>
> > R:      Stephen Boyd <sboyd@kernel.org>
> > 
> > HIGH-RESOLUTION TIMERS, TIMER WHEEL, CLOCKEVENTS
> > M:      Anna-Maria Behnsen <anna-maria@linutronix.de>
> > M:      Frederic Weisbecker <frederic@kernel.org>
> > M:      Thomas Gleixner <tglx@linutronix.de>
> > 
> > The current Rust abstractions which play mainly with ktimer.h. it's 
> > not time, timer stuff, I think.
> 
> Oops, s/ktimer.h/ktime.h/
> 
> No entry for ktime.h in MAINTAINERS; used by both time and timer
> stuff.
> 

I think ktime.h belongs to TIMEKEEPING, since ktime_get() is defined in
kernel/time/timekeeping.c and that's a core function for ktime_t, but
you're not wrong that there is no entry of ktime.h in MAINTAINERS, so if
you want to wait for or check with Thomas, feel free.

> > As planned, we'll move *.rs files from rust/kernel in the future,
> > how we handle time and timer abstractions?

I don't think core stuffs will be moved from rust/kernel, i.e. anything
correspond to the concepts defined in kernel/ directory probably stays
in rust/kernel, so time and timer fall into that category.

> 
> Looks like that we'll add rust/kernel/hrtimer/ soon. I feel that it's
> better to decide on the layout of time and timer abstractions now
> rather than later.

I already suggested we move hrtimer.rs into rust/kernel/time to match
the hierarchy of the counterpart in C (kernel/time/hrtimer.c):

	https://lore.kernel.org/rust-for-linux/ZwqTf-6xaASnIn9l@boqun-archlinux/

Regards,
Boqun

Re: [PATCH net-next v2 0/6] rust: Add IO polling

[FUJITA Tomonori]

On Sat, 12 Oct 2024 20:16:44 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Sun, Oct 13, 2024 at 11:50:33AM +0900, FUJITA Tomonori wrote:
>> On Sun, 13 Oct 2024 10:15:05 +0900 (JST)
>> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
>> 
>> > On Sat, 12 Oct 2024 08:29:06 -0700
>> > Boqun Feng <boqun.feng@gmail.com> wrote:
>> > 
>> >> While, we are at it, I want to suggest that we also add
>> >> rust/kernel/time{.rs, /} into the "F:" entries of TIME subsystem like:
>> >> 
>> >> diff --git a/MAINTAINERS b/MAINTAINERS
>> >> index b77f4495dcf4..09e46a214333 100644
>> >> --- a/MAINTAINERS
>> >> +++ b/MAINTAINERS
>> >> @@ -23376,6 +23376,8 @@ F:      kernel/time/timeconv.c
>> >>  F:     kernel/time/timecounter.c
>> >>  F:     kernel/time/timekeeping*
>> >>  F:     kernel/time/time_test.c
>> >> +F:     rust/kernel/time.rs
>> >> +F:     rust/kernel/time/
>> >>  F:     tools/testing/selftests/timers/
>> >> 
>> >>  TIPC NETWORK LAYER
>> >> 
>> >> This will help future contributers copy the correct people while
>> >> submission. Could you maybe add a patch of this in your series if this
>> >> sounds reasonable to you? Thanks!
>> > 
>> > Agreed that it's better to have Rust time abstractions in
>> > MAINTAINERS. You add it into the time entry but there are two options
>> > in the file; time and timer?
>> > 
>> > TIMEKEEPING, CLOCKSOURCE CORE, NTP, ALARMTIMER
>> > M:      John Stultz <jstultz@google.com>
>> > M:      Thomas Gleixner <tglx@linutronix.de>
>> > R:      Stephen Boyd <sboyd@kernel.org>
>> > 
>> > HIGH-RESOLUTION TIMERS, TIMER WHEEL, CLOCKEVENTS
>> > M:      Anna-Maria Behnsen <anna-maria@linutronix.de>
>> > M:      Frederic Weisbecker <frederic@kernel.org>
>> > M:      Thomas Gleixner <tglx@linutronix.de>
>> > 
>> > The current Rust abstractions which play mainly with ktimer.h. it's 
>> > not time, timer stuff, I think.
>> 
>> Oops, s/ktimer.h/ktime.h/
>> 
>> No entry for ktime.h in MAINTAINERS; used by both time and timer
>> stuff.
>> 
> 
> I think ktime.h belongs to TIMEKEEPING, since ktime_get() is defined in
> kernel/time/timekeeping.c and that's a core function for ktime_t, but

Sounds reasonable.

This patchset adds Delta (also belongs to time, I guess) and fsleep to
rust/kernel/time.rs. I think that fsleep belongs to timer (because
sleep functions in kernel/time/timer.c). It's better to add
rust/kerne/time/timer.rs for fsleep() rather than putting both time
and timer stuff to rust/kernel/time.rs?


> you're not wrong that there is no entry of ktime.h in MAINTAINERS, so if
> you want to wait for or check with Thomas, feel free.
> 
>> > As planned, we'll move *.rs files from rust/kernel in the future,
>> > how we handle time and timer abstractions?
> 
> I don't think core stuffs will be moved from rust/kernel, i.e. anything
> correspond to the concepts defined in kernel/ directory probably stays
> in rust/kernel, so time and timer fall into that category.

Seems that I misunderstood. The plan for the future layout is
documented somewhere?


>> Looks like that we'll add rust/kernel/hrtimer/ soon. I feel that it's
>> better to decide on the layout of time and timer abstractions now
>> rather than later.
> 
> I already suggested we move hrtimer.rs into rust/kernel/time to match
> the hierarchy of the counterpart in C (kernel/time/hrtimer.c):
> 
> 	https://lore.kernel.org/rust-for-linux/ZwqTf-6xaASnIn9l@boqun-archlinux/

Sounds good.

Re: [PATCH net-next v2 0/6] rust: Add IO polling

[Miguel Ojeda]

On Sun, Oct 13, 2024 at 7:15 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Seems that I misunderstood. The plan for the future layout is
> documented somewhere?

Not yet, but I think either would be fine.

Cheers,
Miguel

Re: [PATCH net-next v2 0/6] rust: Add IO polling

[Boqun Feng]

On Sun, Oct 13, 2024 at 02:15:06PM +0900, FUJITA Tomonori wrote:
> On Sat, 12 Oct 2024 20:16:44 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
> 
> > On Sun, Oct 13, 2024 at 11:50:33AM +0900, FUJITA Tomonori wrote:
> >> On Sun, 13 Oct 2024 10:15:05 +0900 (JST)
> >> FUJITA Tomonori <fujita.tomonori@gmail.com> wrote:
> >> 
> >> > On Sat, 12 Oct 2024 08:29:06 -0700
> >> > Boqun Feng <boqun.feng@gmail.com> wrote:
> >> > 
> >> >> While, we are at it, I want to suggest that we also add
> >> >> rust/kernel/time{.rs, /} into the "F:" entries of TIME subsystem like:
> >> >> 
> >> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> >> index b77f4495dcf4..09e46a214333 100644
> >> >> --- a/MAINTAINERS
> >> >> +++ b/MAINTAINERS
> >> >> @@ -23376,6 +23376,8 @@ F:      kernel/time/timeconv.c
> >> >>  F:     kernel/time/timecounter.c
> >> >>  F:     kernel/time/timekeeping*
> >> >>  F:     kernel/time/time_test.c
> >> >> +F:     rust/kernel/time.rs
> >> >> +F:     rust/kernel/time/
> >> >>  F:     tools/testing/selftests/timers/
> >> >> 
> >> >>  TIPC NETWORK LAYER
> >> >> 
> >> >> This will help future contributers copy the correct people while
> >> >> submission. Could you maybe add a patch of this in your series if this
> >> >> sounds reasonable to you? Thanks!
> >> > 
> >> > Agreed that it's better to have Rust time abstractions in
> >> > MAINTAINERS. You add it into the time entry but there are two options
> >> > in the file; time and timer?
> >> > 
> >> > TIMEKEEPING, CLOCKSOURCE CORE, NTP, ALARMTIMER
> >> > M:      John Stultz <jstultz@google.com>
> >> > M:      Thomas Gleixner <tglx@linutronix.de>
> >> > R:      Stephen Boyd <sboyd@kernel.org>
> >> > 
> >> > HIGH-RESOLUTION TIMERS, TIMER WHEEL, CLOCKEVENTS
> >> > M:      Anna-Maria Behnsen <anna-maria@linutronix.de>
> >> > M:      Frederic Weisbecker <frederic@kernel.org>
> >> > M:      Thomas Gleixner <tglx@linutronix.de>
> >> > 
> >> > The current Rust abstractions which play mainly with ktimer.h. it's 
> >> > not time, timer stuff, I think.
> >> 
> >> Oops, s/ktimer.h/ktime.h/
> >> 
> >> No entry for ktime.h in MAINTAINERS; used by both time and timer
> >> stuff.
> >> 
> > 
> > I think ktime.h belongs to TIMEKEEPING, since ktime_get() is defined in
> > kernel/time/timekeeping.c and that's a core function for ktime_t, but
> 
> Sounds reasonable.
> 
> This patchset adds Delta (also belongs to time, I guess) and fsleep to
> rust/kernel/time.rs. I think that fsleep belongs to timer (because
> sleep functions in kernel/time/timer.c). It's better to add
> rust/kerne/time/timer.rs for fsleep() rather than putting both time
> and timer stuff to rust/kernel/time.rs?
> 

Good point. So how about putting fsleep() into rusk/kernel/time/delay.rs
and add that into the "F:" entry of TIMER subsystem? Since "sleep"s are
a set of particular usage of timers which don't directly interact with a
timer or hrtimer struct, so I feel it's better to have their own
file/mod rather than sharing it with timers. Plus this results in less
potential conflicts with Andreas' hrtimer series.

Regards,
Boqun

[...]

Re: [PATCH net-next v2 0/6] rust: Add IO polling

[FUJITA Tomonori]

On Mon, 14 Oct 2024 14:18:57 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

>> This patchset adds Delta (also belongs to time, I guess) and fsleep to
>> rust/kernel/time.rs. I think that fsleep belongs to timer (because
>> sleep functions in kernel/time/timer.c). It's better to add
>> rust/kerne/time/timer.rs for fsleep() rather than putting both time
>> and timer stuff to rust/kernel/time.rs?
>> 
> 
> Good point. So how about putting fsleep() into rusk/kernel/time/delay.rs
> and add that into the "F:" entry of TIMER subsystem? Since "sleep"s are
> a set of particular usage of timers which don't directly interact with a
> timer or hrtimer struct, so I feel it's better to have their own
> file/mod rather than sharing it with timers. Plus this results in less
> potential conflicts with Andreas' hrtimer series.

Sure. I'll go with rust/kernel/time/delay.rs.