[PATCH] rust: time: Seal the ClockSource trait

[PATCH] rust: time: Seal the ClockSource trait

[FUJITA Tomonori]

Prevent downstream crates or drivers from implementing `ClockSource`
for arbitrary types, which could otherwise leads to unsupported
behavior.

Introduce a `private::Sealed` trait and implement it for all types
that implement `ClockSource`.

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

diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
index eaa6d9ab5737..b1961652c884 100644
--- a/rust/kernel/time.rs
+++ b/rust/kernel/time.rs
@@ -51,6 +51,15 @@ pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
     unsafe { bindings::__msecs_to_jiffies(msecs) }
 }
 
+mod private {
+    pub trait Sealed {}
+
+    impl Sealed for super::Monotonic {}
+    impl Sealed for super::RealTime {}
+    impl Sealed for super::BootTime {}
+    impl Sealed for super::Tai {}
+}
+
 /// Trait for clock sources.
 ///
 /// Selection of the clock source depends on the use case. In some cases the usage of a
@@ -58,7 +67,7 @@ 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 {
+pub trait ClockSource: private::Sealed {
     /// The kernel clock ID associated with this clock source.
     ///
     /// This constant corresponds to the C side `clockid_t` value.

base-commit: 994393295c89711531583f6de8f296a30b0d944a
-- 
2.43.0

Re: [PATCH] rust: time: Seal the ClockSource trait

[Boqun Feng]

On Wed, Jun 18, 2025 at 08:20:53AM +0900, FUJITA Tomonori wrote:
> Prevent downstream crates or drivers from implementing `ClockSource`
> for arbitrary types, which could otherwise leads to unsupported
> behavior.
> 

Hmm.. I don't think other impl of `ClockSource` is a problem, IIUC, as
long as the ktime_get() can return a value in [0, i64::MAX). Also this
means ClockSource should be an `unsafe` trait, because the correct
implementaion relies on ktime_get() returns the correct value. This is
needed even if you sealed ClockSource trait.

Could you drop this and fix that the ClockSource trait instead? Thanks!

Regards,
Boqun

> Introduce a `private::Sealed` trait and implement it for all types
> that implement `ClockSource`.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> ---
>  rust/kernel/time.rs | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> index eaa6d9ab5737..b1961652c884 100644
> --- a/rust/kernel/time.rs
> +++ b/rust/kernel/time.rs
> @@ -51,6 +51,15 @@ pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
>      unsafe { bindings::__msecs_to_jiffies(msecs) }
>  }
>  
> +mod private {
> +    pub trait Sealed {}
> +
> +    impl Sealed for super::Monotonic {}
> +    impl Sealed for super::RealTime {}
> +    impl Sealed for super::BootTime {}
> +    impl Sealed for super::Tai {}
> +}
> +
>  /// Trait for clock sources.
>  ///
>  /// Selection of the clock source depends on the use case. In some cases the usage of a
> @@ -58,7 +67,7 @@ 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 {
> +pub trait ClockSource: private::Sealed {
>      /// The kernel clock ID associated with this clock source.
>      ///
>      /// This constant corresponds to the C side `clockid_t` value.
> 
> base-commit: 994393295c89711531583f6de8f296a30b0d944a
> -- 
> 2.43.0
> 

Re: [PATCH] rust: time: Seal the ClockSource trait

[Boqun Feng]

On Tue, Jun 17, 2025 at 05:10:42PM -0700, Boqun Feng wrote:
> On Wed, Jun 18, 2025 at 08:20:53AM +0900, FUJITA Tomonori wrote:
> > Prevent downstream crates or drivers from implementing `ClockSource`
> > for arbitrary types, which could otherwise leads to unsupported
> > behavior.
> > 
> 
> Hmm.. I don't think other impl of `ClockSource` is a problem, IIUC, as
> long as the ktime_get() can return a value in [0, i64::MAX). Also this
> means ClockSource should be an `unsafe` trait, because the correct
> implementaion relies on ktime_get() returns the correct value. This is
> needed even if you sealed ClockSource trait.
> 
> Could you drop this and fix that the ClockSource trait instead? Thanks!
> 

For example:

    /// Trait for clock sources.
    ///
    /// ...
    /// # Safety
    /// 
    /// Implementers must ensure `ktime_get()` return a value in [0,
    //  KTIME_MAX (i.e. i64::MAX)).
    pub unsafe trait ClockSource {
        ...
    }

Regards,
Boqun

> Regards,
> Boqun
> 
> > Introduce a `private::Sealed` trait and implement it for all types
> > that implement `ClockSource`.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@gmail.com>
> > ---
> >  rust/kernel/time.rs | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/rust/kernel/time.rs b/rust/kernel/time.rs
> > index eaa6d9ab5737..b1961652c884 100644
> > --- a/rust/kernel/time.rs
> > +++ b/rust/kernel/time.rs
> > @@ -51,6 +51,15 @@ pub fn msecs_to_jiffies(msecs: Msecs) -> Jiffies {
> >      unsafe { bindings::__msecs_to_jiffies(msecs) }
> >  }
> >  
> > +mod private {
> > +    pub trait Sealed {}
> > +
> > +    impl Sealed for super::Monotonic {}
> > +    impl Sealed for super::RealTime {}
> > +    impl Sealed for super::BootTime {}
> > +    impl Sealed for super::Tai {}
> > +}
> > +
> >  /// Trait for clock sources.
> >  ///
> >  /// Selection of the clock source depends on the use case. In some cases the usage of a
> > @@ -58,7 +67,7 @@ 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 {
> > +pub trait ClockSource: private::Sealed {
> >      /// The kernel clock ID associated with this clock source.
> >      ///
> >      /// This constant corresponds to the C side `clockid_t` value.
> > 
> > base-commit: 994393295c89711531583f6de8f296a30b0d944a
> > -- 
> > 2.43.0
> > 

Re: [PATCH] rust: time: Seal the ClockSource trait

[Andreas Hindborg]

"Boqun Feng" <boqun.feng@gmail.com> writes:

> On Tue, Jun 17, 2025 at 05:10:42PM -0700, Boqun Feng wrote:
>> On Wed, Jun 18, 2025 at 08:20:53AM +0900, FUJITA Tomonori wrote:
>> > Prevent downstream crates or drivers from implementing `ClockSource`
>> > for arbitrary types, which could otherwise leads to unsupported
>> > behavior.
>> >
>>
>> Hmm.. I don't think other impl of `ClockSource` is a problem, IIUC, as
>> long as the ktime_get() can return a value in [0, i64::MAX). Also this
>> means ClockSource should be an `unsafe` trait, because the correct
>> implementaion relies on ktime_get() returns the correct value. This is
>> needed even if you sealed ClockSource trait.
>>
>> Could you drop this and fix that the ClockSource trait instead? Thanks!
>>
>
> For example:
>
>     /// Trait for clock sources.
>     ///
>     /// ...
>     /// # Safety
>     ///
>     /// Implementers must ensure `ktime_get()` return a value in [0,
>     //  KTIME_MAX (i.e. i64::MAX)).
>     pub unsafe trait ClockSource {
>         ...
>     }

Nice catch, it definitely needs to be unsafe. We should also require
correlation between ID and the value fetched by `ktime_get`.

But I still think it is fine to seal the trait, why not?


Best regards,
Andreas Hindborg

Re: [PATCH] rust: time: Seal the ClockSource trait

[Boqun Feng]

On Wed, Jun 18, 2025 at 09:13:07PM +0200, Andreas Hindborg wrote:
> "Boqun Feng" <boqun.feng@gmail.com> writes:
> 
> > On Tue, Jun 17, 2025 at 05:10:42PM -0700, Boqun Feng wrote:
> >> On Wed, Jun 18, 2025 at 08:20:53AM +0900, FUJITA Tomonori wrote:
> >> > Prevent downstream crates or drivers from implementing `ClockSource`
> >> > for arbitrary types, which could otherwise leads to unsupported
> >> > behavior.
> >> >
> >>
> >> Hmm.. I don't think other impl of `ClockSource` is a problem, IIUC, as
> >> long as the ktime_get() can return a value in [0, i64::MAX). Also this
> >> means ClockSource should be an `unsafe` trait, because the correct
> >> implementaion relies on ktime_get() returns the correct value. This is
> >> needed even if you sealed ClockSource trait.
> >>
> >> Could you drop this and fix that the ClockSource trait instead? Thanks!
> >>
> >
> > For example:
> >
> >     /// Trait for clock sources.
> >     ///
> >     /// ...
> >     /// # Safety
> >     ///
> >     /// Implementers must ensure `ktime_get()` return a value in [0,
> >     //  KTIME_MAX (i.e. i64::MAX)).
> >     pub unsafe trait ClockSource {
> >         ...
> >     }
> 
> Nice catch, it definitely needs to be unsafe. We should also require
> correlation between ID and the value fetched by `ktime_get`.
> 
> But I still think it is fine to seal the trait, why not?
> 

There could be potential users of a customized clock source, for
example, a device which also has a timestamp register itself:

	https://lore.kernel.org/rust-for-linux/Z9xb1r1x5tOzAIZT@boqun-archlinux/

So I think with ClockSource being unsafe and well documented, making it
not sealed wouldn't be a problem. IMO, sealing is for the cases where we
must not have downstream impls, ClockSource is not such a case.

Regards,
Boqun

> 
> Best regards,
> Andreas Hindborg
> 
> 

Re: [PATCH] rust: time: Seal the ClockSource trait

[FUJITA Tomonori]

On Wed, 18 Jun 2025 12:29:55 -0700
Boqun Feng <boqun.feng@gmail.com> wrote:

> On Wed, Jun 18, 2025 at 09:13:07PM +0200, Andreas Hindborg wrote:
>> "Boqun Feng" <boqun.feng@gmail.com> writes:
>> 
>> > On Tue, Jun 17, 2025 at 05:10:42PM -0700, Boqun Feng wrote:
>> >> On Wed, Jun 18, 2025 at 08:20:53AM +0900, FUJITA Tomonori wrote:
>> >> > Prevent downstream crates or drivers from implementing `ClockSource`
>> >> > for arbitrary types, which could otherwise leads to unsupported
>> >> > behavior.
>> >> >
>> >>
>> >> Hmm.. I don't think other impl of `ClockSource` is a problem, IIUC, as
>> >> long as the ktime_get() can return a value in [0, i64::MAX). Also this
>> >> means ClockSource should be an `unsafe` trait, because the correct
>> >> implementaion relies on ktime_get() returns the correct value. This is
>> >> needed even if you sealed ClockSource trait.
>> >>
>> >> Could you drop this and fix that the ClockSource trait instead? Thanks!
>> >>
>> >
>> > For example:
>> >
>> >     /// Trait for clock sources.
>> >     ///
>> >     /// ...
>> >     /// # Safety
>> >     ///
>> >     /// Implementers must ensure `ktime_get()` return a value in [0,
>> >     //  KTIME_MAX (i.e. i64::MAX)).
>> >     pub unsafe trait ClockSource {
>> >         ...
>> >     }
>> 
>> Nice catch, it definitely needs to be unsafe. We should also require
>> correlation between ID and the value fetched by `ktime_get`.
>> 
>> But I still think it is fine to seal the trait, why not?
>> 
> 
> There could be potential users of a customized clock source, for
> example, a device which also has a timestamp register itself:
> 
> 	https://lore.kernel.org/rust-for-linux/Z9xb1r1x5tOzAIZT@boqun-archlinux/
> 
> So I think with ClockSource being unsafe and well documented, making it
> not sealed wouldn't be a problem. IMO, sealing is for the cases where we
> must not have downstream impls, ClockSource is not such a case.

Ah, I wasn't aware of that kind of use case. Indeed, a customized
clock source seems like a better approach than reinventing Instant and
Delta in a driver.

On the other hand, I think that sealing HrTimerMode trait is the right
approach:

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

Firstly, HrTimerMode needs to be supported on the C side, so
implementing a custom Rust HrTimerMode won't work.

Secondly, if a developer adds a new HrTimerMode on the C side, we
believe that the corresponding Rust support should be added in the
time abstractions, not in drivers or other places.


Does that make sense?

Re: [PATCH] rust: time: Seal the ClockSource trait

[Boqun Feng]

On Wed, Jun 18, 2025, at 5:23 PM, FUJITA Tomonori wrote:
> On Wed, 18 Jun 2025 12:29:55 -0700
> Boqun Feng <boqun.feng@gmail.com> wrote:
>
>> On Wed, Jun 18, 2025 at 09:13:07PM +0200, Andreas Hindborg wrote:
>>> "Boqun Feng" <boqun.feng@gmail.com> writes:
>>> 
>>> > On Tue, Jun 17, 2025 at 05:10:42PM -0700, Boqun Feng wrote:
>>> >> On Wed, Jun 18, 2025 at 08:20:53AM +0900, FUJITA Tomonori wrote:
>>> >> > Prevent downstream crates or drivers from implementing `ClockSource`
>>> >> > for arbitrary types, which could otherwise leads to unsupported
>>> >> > behavior.
>>> >> >
>>> >>
>>> >> Hmm.. I don't think other impl of `ClockSource` is a problem, IIUC, as
>>> >> long as the ktime_get() can return a value in [0, i64::MAX). Also this
>>> >> means ClockSource should be an `unsafe` trait, because the correct
>>> >> implementaion relies on ktime_get() returns the correct value. This is
>>> >> needed even if you sealed ClockSource trait.
>>> >>
>>> >> Could you drop this and fix that the ClockSource trait instead? Thanks!
>>> >>
>>> >
>>> > For example:
>>> >
>>> >     /// Trait for clock sources.
>>> >     ///
>>> >     /// ...
>>> >     /// # Safety
>>> >     ///
>>> >     /// Implementers must ensure `ktime_get()` return a value in [0,
>>> >     //  KTIME_MAX (i.e. i64::MAX)).
>>> >     pub unsafe trait ClockSource {
>>> >         ...
>>> >     }
>>> 
>>> Nice catch, it definitely needs to be unsafe. We should also require
>>> correlation between ID and the value fetched by `ktime_get`.
>>> 
>>> But I still think it is fine to seal the trait, why not?
>>> 
>> 
>> There could be potential users of a customized clock source, for
>> example, a device which also has a timestamp register itself:
>> 
>> 	https://lore.kernel.org/rust-for-linux/Z9xb1r1x5tOzAIZT@boqun-archlinux/
>> 
>> So I think with ClockSource being unsafe and well documented, making it
>> not sealed wouldn't be a problem. IMO, sealing is for the cases where we
>> must not have downstream impls, ClockSource is not such a case.
>
> Ah, I wasn't aware of that kind of use case. Indeed, a customized
> clock source seems like a better approach than reinventing Instant and
> Delta in a driver.
>
> On the other hand, I think that sealing HrTimerMode trait is the right
> approach:
>
> https://lore.kernel.org/rust-for-linux/20250617232806.3950141-1-fujita.tomonori@gmail.com/
>
> Firstly, HrTimerMode needs to be supported on the C side, so
> implementing a custom Rust HrTimerMode won't work.
>
> Secondly, if a developer adds a new HrTimerMode on the C side, we
> believe that the corresponding Rust support should be added in the
> time abstractions, not in drivers or other places.
>
>
> Does that make sense?

Agreed. :)

Regards,
Boqun

Re: [PATCH] rust: time: Seal the ClockSource trait

[FUJITA Tomonori]

On Wed, 18 Jun 2025 21:13:07 +0200
Andreas Hindborg <a.hindborg@kernel.org> wrote:

> "Boqun Feng" <boqun.feng@gmail.com> writes:
> 
>> On Tue, Jun 17, 2025 at 05:10:42PM -0700, Boqun Feng wrote:
>>> On Wed, Jun 18, 2025 at 08:20:53AM +0900, FUJITA Tomonori wrote:
>>> > Prevent downstream crates or drivers from implementing `ClockSource`
>>> > for arbitrary types, which could otherwise leads to unsupported
>>> > behavior.
>>> >
>>>
>>> Hmm.. I don't think other impl of `ClockSource` is a problem, IIUC, as
>>> long as the ktime_get() can return a value in [0, i64::MAX). Also this
>>> means ClockSource should be an `unsafe` trait, because the correct
>>> implementaion relies on ktime_get() returns the correct value. This is
>>> needed even if you sealed ClockSource trait.
>>>
>>> Could you drop this and fix that the ClockSource trait instead? Thanks!
>>>
>>
>> For example:
>>
>>     /// Trait for clock sources.
>>     ///
>>     /// ...
>>     /// # Safety
>>     ///
>>     /// Implementers must ensure `ktime_get()` return a value in [0,
>>     //  KTIME_MAX (i.e. i64::MAX)).
>>     pub unsafe trait ClockSource {
>>         ...
>>     }
> 
> Nice catch, it definitely needs to be unsafe. We should also require
> correlation between ID and the value fetched by `ktime_get`.

What's ID?

Re: [PATCH] rust: time: Seal the ClockSource trait

[Andreas Hindborg]

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

> On Wed, 18 Jun 2025 21:13:07 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> "Boqun Feng" <boqun.feng@gmail.com> writes:
>>
>>> On Tue, Jun 17, 2025 at 05:10:42PM -0700, Boqun Feng wrote:
>>>> On Wed, Jun 18, 2025 at 08:20:53AM +0900, FUJITA Tomonori wrote:
>>>> > Prevent downstream crates or drivers from implementing `ClockSource`
>>>> > for arbitrary types, which could otherwise leads to unsupported
>>>> > behavior.
>>>> >
>>>>
>>>> Hmm.. I don't think other impl of `ClockSource` is a problem, IIUC, as
>>>> long as the ktime_get() can return a value in [0, i64::MAX). Also this
>>>> means ClockSource should be an `unsafe` trait, because the correct
>>>> implementaion relies on ktime_get() returns the correct value. This is
>>>> needed even if you sealed ClockSource trait.
>>>>
>>>> Could you drop this and fix that the ClockSource trait instead? Thanks!
>>>>
>>>
>>> For example:
>>>
>>>     /// Trait for clock sources.
>>>     ///
>>>     /// ...
>>>     /// # Safety
>>>     ///
>>>     /// Implementers must ensure `ktime_get()` return a value in [0,
>>>     //  KTIME_MAX (i.e. i64::MAX)).
>>>     pub unsafe trait ClockSource {
>>>         ...
>>>     }
>>
>> Nice catch, it definitely needs to be unsafe. We should also require
>> correlation between ID and the value fetched by `ktime_get`.
>
> What's ID?


  pub trait ClockSource {
      /// The kernel clock ID associated with this clock source.
      ///
      /// This constant corresponds to the C side `clockid_t` value.
      const ID: bindings::clockid_t;

The constant used to identify the clock source when calling into C APIs.


Best regards,
Andreas Hindborg

Re: [PATCH] rust: time: Seal the ClockSource trait

[FUJITA Tomonori]

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

> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
> 
>> On Wed, 18 Jun 2025 21:13:07 +0200
>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>>> "Boqun Feng" <boqun.feng@gmail.com> writes:
>>>
>>>> On Tue, Jun 17, 2025 at 05:10:42PM -0700, Boqun Feng wrote:
>>>>> On Wed, Jun 18, 2025 at 08:20:53AM +0900, FUJITA Tomonori wrote:
>>>>> > Prevent downstream crates or drivers from implementing `ClockSource`
>>>>> > for arbitrary types, which could otherwise leads to unsupported
>>>>> > behavior.
>>>>> >
>>>>>
>>>>> Hmm.. I don't think other impl of `ClockSource` is a problem, IIUC, as
>>>>> long as the ktime_get() can return a value in [0, i64::MAX). Also this
>>>>> means ClockSource should be an `unsafe` trait, because the correct
>>>>> implementaion relies on ktime_get() returns the correct value. This is
>>>>> needed even if you sealed ClockSource trait.
>>>>>
>>>>> Could you drop this and fix that the ClockSource trait instead? Thanks!
>>>>>
>>>>
>>>> For example:
>>>>
>>>>     /// Trait for clock sources.
>>>>     ///
>>>>     /// ...
>>>>     /// # Safety
>>>>     ///
>>>>     /// Implementers must ensure `ktime_get()` return a value in [0,
>>>>     //  KTIME_MAX (i.e. i64::MAX)).
>>>>     pub unsafe trait ClockSource {
>>>>         ...
>>>>     }
>>>
>>> Nice catch, it definitely needs to be unsafe. We should also require
>>> correlation between ID and the value fetched by `ktime_get`.
>>
>> What's ID?
> 
> 
>   pub trait ClockSource {
>       /// The kernel clock ID associated with this clock source.
>       ///
>       /// This constant corresponds to the C side `clockid_t` value.
>       const ID: bindings::clockid_t;
> 
> The constant used to identify the clock source when calling into C APIs.

Ah, I see. Sorry to ask another question, but can we require
correlation between ID and the value fetched by `ktime_get`? The value
fetched by ktime_get is opaque, isn't it?

Re: [PATCH] rust: time: Seal the ClockSource trait

[Andreas Hindborg]

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

> On Thu, 19 Jun 2025 11:31:08 +0200
> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
>> "FUJITA Tomonori" <fujita.tomonori@gmail.com> writes:
>>
>>> On Wed, 18 Jun 2025 21:13:07 +0200
>>> Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>
>>>> "Boqun Feng" <boqun.feng@gmail.com> writes:
>>>>
>>>>> On Tue, Jun 17, 2025 at 05:10:42PM -0700, Boqun Feng wrote:
>>>>>> On Wed, Jun 18, 2025 at 08:20:53AM +0900, FUJITA Tomonori wrote:
>>>>>> > Prevent downstream crates or drivers from implementing `ClockSource`
>>>>>> > for arbitrary types, which could otherwise leads to unsupported
>>>>>> > behavior.
>>>>>> >
>>>>>>
>>>>>> Hmm.. I don't think other impl of `ClockSource` is a problem, IIUC, as
>>>>>> long as the ktime_get() can return a value in [0, i64::MAX). Also this
>>>>>> means ClockSource should be an `unsafe` trait, because the correct
>>>>>> implementaion relies on ktime_get() returns the correct value. This is
>>>>>> needed even if you sealed ClockSource trait.
>>>>>>
>>>>>> Could you drop this and fix that the ClockSource trait instead? Thanks!
>>>>>>
>>>>>
>>>>> For example:
>>>>>
>>>>>     /// Trait for clock sources.
>>>>>     ///
>>>>>     /// ...
>>>>>     /// # Safety
>>>>>     ///
>>>>>     /// Implementers must ensure `ktime_get()` return a value in [0,
>>>>>     //  KTIME_MAX (i.e. i64::MAX)).
>>>>>     pub unsafe trait ClockSource {
>>>>>         ...
>>>>>     }
>>>>
>>>> Nice catch, it definitely needs to be unsafe. We should also require
>>>> correlation between ID and the value fetched by `ktime_get`.
>>>
>>> What's ID?
>>
>>
>>   pub trait ClockSource {
>>       /// The kernel clock ID associated with this clock source.
>>       ///
>>       /// This constant corresponds to the C side `clockid_t` value.
>>       const ID: bindings::clockid_t;
>>
>> The constant used to identify the clock source when calling into C APIs.
>
> Ah, I see. Sorry to ask another question, but can we require
> correlation between ID and the value fetched by `ktime_get`?

Yes, I think we should. As in, `ClockSource::ktime_get` must return the
time associated with the clock specified by `ClockSource::ID`.

>The value
> fetched by ktime_get is opaque, isn't it?

It is, but the implementation must still fetch the correct counter, right?
Not sure if it could lead to UB if it did not though 🤷


Best regards,
Andreas Hindborg

Re: [PATCH] rust: time: Seal the ClockSource trait

[Boqun Feng]

On Thu, Jun 19, 2025 at 02:57:12PM +0200, Andreas Hindborg wrote:
[...]
> >>
> >>
> >>   pub trait ClockSource {
> >>       /// The kernel clock ID associated with this clock source.
> >>       ///
> >>       /// This constant corresponds to the C side `clockid_t` value.
> >>       const ID: bindings::clockid_t;
> >>
> >> The constant used to identify the clock source when calling into C APIs.
> >
> > Ah, I see. Sorry to ask another question, but can we require
> > correlation between ID and the value fetched by `ktime_get`?
> 
> Yes, I think we should. As in, `ClockSource::ktime_get` must return the
> time associated with the clock specified by `ClockSource::ID`.
> 
> >The value
> > fetched by ktime_get is opaque, isn't it?
> 
> It is, but the implementation must still fetch the correct counter, right?
> Not sure if it could lead to UB if it did not though 🤷
> 

The reason that we need ktime_get() to return value in [0, KTIME_MAX) is
because Instant's type invariants (and Instant's type invariants is for
subtraction not overflowing), so I would say this is not a safety
requirement for impl ClockSource.

Regards,
Boqun

> 
> Best regards,
> Andreas Hindborg
> 
> 
>