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

[FUJITA Tomonori]

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

> On Wed, Jun 18, 2025 at 09:13:07PM +0200, Andreas Hindborg wrote:
>> "Boqun Feng" <boqun.feng@xxxxxxxxx> 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@xxxxxxxxx/

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?