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

[Boqun Feng <boqun.feng@gmail.com>]

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
> 
>