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

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

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