Mark the ClockSource trait as unsafe and document its safety
requirements. Specifically, implementers must guarantee that their
`ktime_get()` implementation returns a value in the inclusive range
[0, KTIME_MAX].

Update all existing implementations to use `unsafe impl` with
corresponding safety comments.

Note that there could be potential users of a customized clock source [1]
so we don't seal the trait.

Link: https://lore.kernel.org/rust-for-linux/Z9xb1r1x5tOzAIZT@boqun-archlinux/ [1]
Suggested-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
---
 rust/kernel/time.rs | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index 64c8dcf548d6..a90c386dda3a 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -59,7 +59,13 @@ pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
 /// cases the user of the clock has to decide which clock is best suited for the
 /// purpose. In most scenarios clock [`Monotonic`] is the best choice as it
 /// provides a accurate monotonic notion of time (leap second smearing ignored).
-pub trait ClockSource {
+///
+/// # Safety
+///
+/// Implementers must ensure that `ktime_get()` returns a value in the inclusive range
+/// [0, KTIME_MAX] (i.e., greater than or equal to 0 and less than or equal to
+/// `KTIME_MAX`, where `KTIME_MAX` equals `i64::MAX`).
+pub unsafe trait ClockSource {
     /// The kernel clock ID associated with this clock source.
     ///
     /// This constant corresponds to the C side `clockid_t` value.
@@ -67,7 +73,7 @@ pub trait ClockSource {
 
     /// Get the current time from the clock source.
     ///
-    /// The function must return a value in the range from 0 to `KTIME_MAX`.
+    /// The function must return a value in the range [0, KTIME_MAX].
     fn ktime_get() -> bindings::ktime_t;
 }
 
@@ -84,7 +90,9 @@ pub trait ClockSource {
 /// count time that the system is suspended.
 pub struct Monotonic;
 
-impl ClockSource for Monotonic {
+// SAFETY: The kernel's `ktime_get()` is guaranteed to return a value
+// in [0, KTIME_MAX].
+unsafe impl ClockSource for Monotonic {
     const ID: bindings::clockid_t = bindings::CLOCK_MONOTONIC as bindings::clockid_t;
 
     fn ktime_get() -> bindings::ktime_t {
@@ -109,7 +117,9 @@ fn ktime_get() -> bindings::ktime_t {
 /// the clock will experience discontinuity around leap second adjustment.
 pub struct RealTime;
 
-impl ClockSource for RealTime {
+// SAFETY: The kernel's `ktime_get_real()` is guaranteed to return a value
+// in [0, KTIME_MAX].
+unsafe impl ClockSource for RealTime {
     const ID: bindings::clockid_t = bindings::CLOCK_REALTIME as bindings::clockid_t;
 
     fn ktime_get() -> bindings::ktime_t {
@@ -127,7 +137,9 @@ fn ktime_get() -> bindings::ktime_t {
 /// discontinuities if the time is changed using settimeofday(2) or similar.
 pub struct BootTime;
 
-impl ClockSource for BootTime {
+// SAFETY: The kernel's `ktime_get_boottime()` is guaranteed to return a value
+// in [0, KTIME_MAX].
+unsafe impl ClockSource for BootTime {
     const ID: bindings::clockid_t = bindings::CLOCK_BOOTTIME as bindings::clockid_t;
 
     fn ktime_get() -> bindings::ktime_t {
@@ -149,7 +161,9 @@ fn ktime_get() -> bindings::ktime_t {
 /// The acronym TAI refers to International Atomic Time.
 pub struct Tai;
 
-impl ClockSource for Tai {
+// SAFETY: The kernel's `ktime_get_clocktai()` is guaranteed to return a value
+// in [0, KTIME_MAX].
+unsafe impl ClockSource for Tai {
     const ID: bindings::clockid_t = bindings::CLOCK_TAI as bindings::clockid_t;
 
     fn ktime_get() -> bindings::ktime_t {

base-commit: d4b29ddf82a458935f1bd4909b8a7a13df9d3bdc
-- 
2.43.0

On Mon, Jun 30, 2025 at 3:10 PM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> Mark the ClockSource trait as unsafe and document its safety
> requirements. Specifically, implementers must guarantee that their
> `ktime_get()` implementation returns a value in the inclusive range
> [0, KTIME_MAX].
>
> Update all existing implementations to use `unsafe impl` with
> corresponding safety comments.
>
> Note that there could be potential users of a customized clock source [1]
> so we don't seal the trait.
>
> Link: https://lore.kernel.org/rust-for-linux/Z9xb1r1x5tOzAIZT@boqun-archlinux/ [1]
> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>

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

Though you're missing `` around [0; KTIME_MAX] in some places, which
may be worth adding.

Alice

On Mon, 30 Jun 2025 15:33:31 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

> On Mon, Jun 30, 2025 at 3:10 PM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> Mark the ClockSource trait as unsafe and document its safety
>> requirements. Specifically, implementers must guarantee that their
>> `ktime_get()` implementation returns a value in the inclusive range
>> [0, KTIME_MAX].
>>
>> Update all existing implementations to use `unsafe impl` with
>> corresponding safety comments.
>>
>> Note that there could be potential users of a customized clock source [1]
>> so we don't seal the trait.
>>
>> Link: https://lore.kernel.org/rust-for-linux/Z9xb1r1x5tOzAIZT@boqun-archlinux/ [1]
>> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> 
> LGTM:
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Thanks!

> Though you're missing `` around [0; KTIME_MAX] in some places, which
> may be worth adding.

Andreas, would you like me to send v2 with the above changes?

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

> On Mon, 30 Jun 2025 15:33:31 +0200
> Alice Ryhl <aliceryhl@google.com> wrote:
>
>> On Mon, Jun 30, 2025 at 3:10 PM FUJITA Tomonori
>> <fujita.tomonori@gmail.com> wrote:
>>>
>>> Mark the ClockSource trait as unsafe and document its safety
>>> requirements. Specifically, implementers must guarantee that their
>>> `ktime_get()` implementation returns a value in the inclusive range
>>> [0, KTIME_MAX].
>>>
>>> Update all existing implementations to use `unsafe impl` with
>>> corresponding safety comments.
>>>
>>> Note that there could be potential users of a customized clock source [1]
>>> so we don't seal the trait.
>>>
>>> Link: https://lore.kernel.org/rust-for-linux/Z9xb1r1x5tOzAIZT@boqun-archlinux/ [1]
>>> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>
>> LGTM:
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>
> Thanks!
>
>> Though you're missing `` around [0; KTIME_MAX] in some places, which
>> may be worth adding.
>
> Andreas, would you like me to send v2 with the above changes?

Perhaps we should use rust ranges instead [1]? Like this, no brackets: `0..=KTIME_MAX`.


Best regards,
Andreas Hindborg


[1] https://doc.rust-lang.org/reference/expressions/range-expr.html

On Wed, Jul 2, 2025 at 11:17 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>
> > On Mon, 30 Jun 2025 15:33:31 +0200
> > Alice Ryhl <aliceryhl@google.com> wrote:
> >
> >> On Mon, Jun 30, 2025 at 3:10 PM FUJITA Tomonori
> >> <fujita.tomonori@gmail.com> wrote:
> >>>
> >>> Mark the ClockSource trait as unsafe and document its safety
> >>> requirements. Specifically, implementers must guarantee that their
> >>> `ktime_get()` implementation returns a value in the inclusive range
> >>> [0, KTIME_MAX].
> >>>
> >>> Update all existing implementations to use `unsafe impl` with
> >>> corresponding safety comments.
> >>>
> >>> Note that there could be potential users of a customized clock source [1]
> >>> so we don't seal the trait.
> >>>
> >>> Link: https://lore.kernel.org/rust-for-linux/Z9xb1r1x5tOzAIZT@boqun-archlinux/ [1]
> >>> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> >>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> >>
> >> LGTM:
> >> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> >
> > Thanks!
> >
> >> Though you're missing `` around [0; KTIME_MAX] in some places, which
> >> may be worth adding.
> >
> > Andreas, would you like me to send v2 with the above changes?
>
> Perhaps we should use rust ranges instead [1]? Like this, no brackets: `0..=KTIME_MAX`.

Well, maybe. But I think it's also worth considering just using
english to describe it:

Implementers must ensure that `ktime_get()` returns a positive value
less than or equal to `KTIME_MAX`.

Alice

On Wed, 2 Jul 2025 11:50:35 +0200
Alice Ryhl <aliceryhl@google.com> wrote:

>> >>> Mark the ClockSource trait as unsafe and document its safety
>> >>> requirements. Specifically, implementers must guarantee that their
>> >>> `ktime_get()` implementation returns a value in the inclusive range
>> >>> [0, KTIME_MAX].
>> >>>
>> >>> Update all existing implementations to use `unsafe impl` with
>> >>> corresponding safety comments.
>> >>>
>> >>> Note that there could be potential users of a customized clock source [1]
>> >>> so we don't seal the trait.
>> >>>
>> >>> Link: https://lore.kernel.org/rust-for-linux/Z9xb1r1x5tOzAIZT@boqun-archlinux/ [1]
>> >>> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
>> >>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>> >>
>> >> LGTM:
>> >> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>> >
>> > Thanks!
>> >
>> >> Though you're missing `` around [0; KTIME_MAX] in some places, which
>> >> may be worth adding.
>> >
>> > Andreas, would you like me to send v2 with the above changes?
>>
>> Perhaps we should use rust ranges instead [1]? Like this, no brackets: `0..=KTIME_MAX`.
> 
> Well, maybe. But I think it's also worth considering just using
> english to describe it:

I had a quick look at the official Rust documentation, and I think I
agree with this opinion.

> Implementers must ensure that `ktime_get()` returns a positive value
> less than or equal to `KTIME_MAX`.

Would it make sense to explicitly mention that zero is included? Also
it would be nice to explain the value of KTIME_MAX?


Implementers must ensure that ktime_get() returns a non-negative value
less than or equal to KTIME_MAX, where KTIME_MAX equals i64::MAX.

On Fri, Jul 4, 2025 at 2:08 AM FUJITA Tomonori
<fujita.tomonori@gmail.com> wrote:
>
> I had a quick look at the official Rust documentation, and I think I
> agree with this opinion.

My personal take: I agree that having both English and something else
is a bit redundant -- some redundancy is good when something (like a
notation) may be non-obvious, but I think a math integer interval or a
Rust range or a condition like 0 <= v <= `KTIME_MAX` are all
understandable in this context.

Now, whether to use English or any of the other options, it is hard to
say what is best for most readers. Personally, I prefer to just see
one of the expressions, which makes it also closer to other forms,
e.g. a debug assert somewhere, or a contract, or other tooling or
formalization efforts.

Cheers,
Miguel

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

> On Fri, Jul 4, 2025 at 2:08 AM FUJITA Tomonori
> <fujita.tomonori@gmail.com> wrote:
>>
>> I had a quick look at the official Rust documentation, and I think I
>> agree with this opinion.
>
> My personal take: I agree that having both English and something else
> is a bit redundant -- some redundancy is good when something (like a
> notation) may be non-obvious, but I think a math integer interval or a
> Rust range or a condition like 0 <= v <= `KTIME_MAX` are all
> understandable in this context.
>
> Now, whether to use English or any of the other options, it is hard to
> say what is best for most readers. Personally, I prefer to just see
> one of the expressions, which makes it also closer to other forms,
> e.g. a debug assert somewhere, or a contract, or other tooling or
> formalization efforts.

I would prefer `0..=KTIME_MAX` or `0 <= v <= KTIME_MAX`. English prose
is also OK and I won't object to that, but I prefer the others.


Best regards,
Andreas Hindborg

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

> On Mon, 30 Jun 2025 15:33:31 +0200
> Alice Ryhl <aliceryhl@google.com> wrote:
>
>> On Mon, Jun 30, 2025 at 3:10 PM FUJITA Tomonori
>> <fujita.tomonori@gmail.com> wrote:
>>>
>>> Mark the ClockSource trait as unsafe and document its safety
>>> requirements. Specifically, implementers must guarantee that their
>>> `ktime_get()` implementation returns a value in the inclusive range
>>> [0, KTIME_MAX].
>>>
>>> Update all existing implementations to use `unsafe impl` with
>>> corresponding safety comments.
>>>
>>> Note that there could be potential users of a customized clock source [1]
>>> so we don't seal the trait.
>>>
>>> Link: https://lore.kernel.org/rust-for-linux/Z9xb1r1x5tOzAIZT@boqun-archlinux/ [1]
>>> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
>>
>> LGTM:
>> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>
> Thanks!
>
>> Though you're missing `` around [0; KTIME_MAX] in some places, which
>> may be worth adding.
>
> Andreas, would you like me to send v2 with the above changes?

I can add the ticks.


Best regards,
Andreas Hindborg