On Fri, Aug 25, 2023 6:18 John Stultz <jstultz@xxxxxxxxxx> wrote: > On Thu, Aug 17, 2023 at 6:13 PM Peter Hilber > <peter.hilber@xxxxxxxxxxxxxxx> wrote: >> >> This patch series changes struct system_counterval_t to identify the >> clocksource through enum clocksource_ids, rather than through struct >> clocksource *. The net effect of the patch series is that >> get_device_system_crosststamp() callers can supply clocksource ids instead >> of clocksource pointers, which can be problematic to get hold of. > > Hey Peter, > Thanks for sending this out. I'm a little curious though, can you > expand a bit on how clocksource pointers can be problematic to get a > hold of? What exactly is the problem that is motivating this change? > Hi John, I'm very sorry for the late reply; there was some unexpected delay. Thank you for the remark; I'll expand on the motivation in the next patch series iteration, similar to the explanation below. The immediate motivation for this patch series is to enable the virtio_rtc RFC v2 driver [4] to refer to the Arm Generic Timer without requiring new helper functions in the arm_arch_timer driver. Other future get_device_system_crosststamp() users may profit from this change as well. Clocksource structs are normally private to clocksource drivers. Therefore, get_device_system_crosststamp() callers require that clocksource drivers expose the clocksource of interest in some way. Drivers such as virtio_rtc [4] could obtain all information for calling get_device_system_crosststamp() from their bound device, except for clocksource identification. Such drivers' only direct relation with the clocksource driver is clocksource identification. So using the clocksource enum, rather than obtaining pointers in a clocksource driver specific way, would reduce the coupling between the get_device_system_crosststamp() callers and clocksource drivers. Next, I provide some details to support the low coupling argument. There are two sorts of get_device_system_crosststamp() callers in the current kernel: 1) On Intel platforms, some PTP hardware clocks obtain the clocksource pointer for get_device_system_crosststamp() using convert_art_to_tsc() or convert_art_ns_to_tsc() from arch/x86. 2) The ptp_kvm driver uses kvm_arch_ptp_get_crosststamp(), which is implemented for platforms with kvm_clock or arm_arch_timer. Amongst other things, kvm_arch_ptp_get_crosststamp() returns a clocksource pointer. The Arm implementation is in the arm_arch_timer driver. When I proposed in the virtio_rtc RFC v1 patch series [3] to obtain the clocksource pointer of the arm_arch_timer driver through a generic helper function, one of the maintainers wasn't very enthusiastic about it and suggested reusing kvm_arch_ptp_get_crosststamp() somehow [1]. But to me there seems not to be much in common [2]. Quoting myself from [2]: > If[!] &clocksource_counter should not be exposed, then I can see two > alternatives: > > Alternative 1: Put a function of type > > int (*get_time_fn) (ktime_t *device_time, > struct system_counterval_t *sys_counterval, > void *ctx) > > into arm_arch_timer.c, as required by get_device_system_crosststamp() > (and include a virtio_rtc header). This looks inelegant, since it would require virtio_rtc to put part of its code into arm_arch_timer.c, and would require including a virtio_rtc header in arm_arch_timer.c. The second alternative is using this patch series to expand the use of the clocksource enum to get_device_system_crosststamp(). This should also make it easy to use get_device_system_crosststamp() with other clocksources in the future, by just extending the clocksource enum. > I just worry that switching to an enumeration solution might be > eventually exposing more than we would like to userland. ATM the enum is not in a UAPI header. So IMHO exposing this to userland in the future would require a pretty explicit change. Thanks for the review, Peter [1] https://lore.kernel.org/all/87ila4qwuw.wl-maz@xxxxxxxxxx/ [2] https://lore.kernel.org/all/151befb2-8fbc-b796-47bb-39960a979065@xxxxxxxxxxxxxxx/ [3] https://lore.kernel.org/all/20230630171052.985577-1-peter.hilber@xxxxxxxxxxxxxxx/ [4] https://lore.kernel.org/all/20230818012014.212155-1-peter.hilber@xxxxxxxxxxxxxxx/
