| Message ID | 20230818011256.211078-5-peter.hilber@opensynergy.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | treewide: Use clocksource id for get_device_system_crosststamp() | expand |
Peter! On Fri, Aug 18 2023 at 03:12, Peter Hilber wrote: > --- a/arch/x86/kernel/tsc.c > +++ b/arch/x86/kernel/tsc.c > @@ -1313,7 +1313,7 @@ struct system_counterval_t convert_art_to_tsc(u64 art) > res += tmp + art_to_tsc_offset; > > return (struct system_counterval_t) { > - .cs = have_art ? &clocksource_tsc : NULL, > + .cs_id = have_art ? CSID_TSC : CSID_GENERIC, > .cycles = res Can you please change all of this so that: patch 1: Adds cs_id to struct system_counterval_t patch 2-4: Add the clocksource ID and set the cs_id field patch 5: Switches the core to evaluate cs_id patch 6: Remove the cs field from system_counterval_t > --- a/include/linux/timekeeping.h > +++ b/include/linux/timekeeping.h > @@ -270,12 +270,12 @@ struct system_device_crosststamp { > * struct system_counterval_t - system counter value with the pointer to the > * corresponding clocksource > * @cycles: System counter value > - * @cs: Clocksource corresponding to system counter value. Used by > + * @cs_id: Clocksource corresponding to system counter value. Used by > * timekeeping code to verify comparibility of two cycle values That comment is inaccurate. It's not longer the clocksource itself. It's the ID which is used for validation. Thanks, tglx
On 15.09.23 15:30, Thomas Gleixner wrote: > Peter! > > On Fri, Aug 18 2023 at 03:12, Peter Hilber wrote: >> --- a/arch/x86/kernel/tsc.c >> +++ b/arch/x86/kernel/tsc.c >> @@ -1313,7 +1313,7 @@ struct system_counterval_t convert_art_to_tsc(u64 art) >> res += tmp + art_to_tsc_offset; >> >> return (struct system_counterval_t) { >> - .cs = have_art ? &clocksource_tsc : NULL, >> + .cs_id = have_art ? CSID_TSC : CSID_GENERIC, >> .cycles = res > > Can you please change all of this so that: > > patch 1: Adds cs_id to struct system_counterval_t > patch 2-4: Add the clocksource ID and set the cs_id field > patch 5: Switches the core to evaluate cs_id > patch 6: Remove the cs field from system_counterval_t OK. For 2-4, I assume split x86/tsc, x86/kvm, drivers/ptp (which also handles the CSID_ARM_ARCH_COUNTER case). >> --- a/include/linux/timekeeping.h >> +++ b/include/linux/timekeeping.h >> @@ -270,12 +270,12 @@ struct system_device_crosststamp { >> * struct system_counterval_t - system counter value with the pointer to the >> * corresponding clocksource >> * @cycles: System counter value >> - * @cs: Clocksource corresponding to system counter value. Used by >> + * @cs_id: Clocksource corresponding to system counter value. Used by >> * timekeeping code to verify comparibility of two cycle values > > That comment is inaccurate. It's not longer the clocksource itself. It's > the ID which is used for validation. I will change the comment to refer to "Clocksource ID". Thanks for the advice! Peter
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c index e56cc4e97d0d..eadfb819a0b5 100644 --- a/arch/x86/kernel/tsc.c +++ b/arch/x86/kernel/tsc.c @@ -1313,7 +1313,7 @@ struct system_counterval_t convert_art_to_tsc(u64 art) res += tmp + art_to_tsc_offset; return (struct system_counterval_t) { - .cs = have_art ? &clocksource_tsc : NULL, + .cs_id = have_art ? CSID_TSC : CSID_GENERIC, .cycles = res }; } @@ -1335,7 +1335,7 @@ EXPORT_SYMBOL(convert_art_to_tsc); * struct system_counterval_t - system counter value with the pointer to the * corresponding clocksource * @cycles: System counter value - * @cs: Clocksource corresponding to system counter value. Used + * @cs_id: Clocksource corresponding to system counter value. Used * by timekeeping code to verify comparability of two cycle * values. */ @@ -1353,7 +1353,7 @@ struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns) res += tmp; return (struct system_counterval_t) { - .cs = have_art ? &clocksource_tsc : NULL, + .cs_id = have_art ? CSID_TSC : CSID_GENERIC, .cycles = res }; } diff --git a/drivers/ptp/ptp_kvm_common.c b/drivers/ptp/ptp_kvm_common.c index 2418977989be..d72e6ed71b7a 100644 --- a/drivers/ptp/ptp_kvm_common.c +++ b/drivers/ptp/ptp_kvm_common.c @@ -4,6 +4,7 @@ * * Copyright (C) 2017 Red Hat Inc. */ +#include <linux/clocksource.h> #include <linux/device.h> #include <linux/err.h> #include <linux/init.h> @@ -46,7 +47,7 @@ static int ptp_kvm_get_time_fn(ktime_t *device_time, preempt_enable_notrace(); system_counter->cycles = cycle; - system_counter->cs = cs; + system_counter->cs_id = cs->id; *device_time = timespec64_to_ktime(tspec); diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h index fe1e467ba046..2e7ae0f7f19e 100644 --- a/include/linux/timekeeping.h +++ b/include/linux/timekeeping.h @@ -270,12 +270,12 @@ struct system_device_crosststamp { * struct system_counterval_t - system counter value with the pointer to the * corresponding clocksource * @cycles: System counter value - * @cs: Clocksource corresponding to system counter value. Used by + * @cs_id: Clocksource corresponding to system counter value. Used by * timekeeping code to verify comparibility of two cycle values */ struct system_counterval_t { u64 cycles; - struct clocksource *cs; + enum clocksource_ids cs_id; }; /* diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index 266d02809dbb..56428eadf4c1 100644 --- a/kernel/time/timekeeping.c +++ b/kernel/time/timekeeping.c @@ -1236,7 +1236,8 @@ int get_device_system_crosststamp(int (*get_time_fn) * system counter value is the same as the currently installed * timekeeper clocksource */ - if (tk->tkr_mono.clock != system_counterval.cs) + if (system_counterval.cs_id == CSID_GENERIC || + tk->tkr_mono.clock->id != system_counterval.cs_id) return -ENODEV; cycles = system_counterval.cycles;
Use enum clocksource_ids, rather than struct clocksource *, in struct system_counterval_t. This allows drivers to use get_device_system_crosststamp() without referring to clocksource structs, which are clocksource driver implementation details. As a "treewide" change, this modifies code in timekeeping, ptp/kvm, and x86/tsc. So far, get_device_system_crosststamp() requires the caller to provide a clocksource * as part of the cross-timestamp (through the system_counterval_t * parameter in the get_time_fn() callback). But clocksource * are not easily accessible to most code. As a workaround, custom functions returning the clocksource * may be put into the clocksource driver code (cf. kvm_arch_ptp_get_crosststamp() in arm_arch_timer.c). To avoid more interference with clocksource driver implementation details from future code, change system_counterval_t to use enum clocksource_ids, which serves as a unique identifier for relevant clocksources. This change will allow the virtio_rtc driver, available as an RFC patch series, to work without modifying the arm_arch_timer implementation. This change should not alter any behavior. For the x86 ART related code, .cs == NULL corresponds to system_counterval_t.cs_id == CSID_GENERIC (0). The one drawback of this change is that clocksources now need a custom id to work with get_device_system_crosststamp(), since they cannot be distinguished otherwise. Signed-off-by: Peter Hilber <peter.hilber@opensynergy.com> --- arch/x86/kernel/tsc.c | 6 +++--- drivers/ptp/ptp_kvm_common.c | 3 ++- include/linux/timekeeping.h | 4 ++-- kernel/time/timekeeping.c | 3 ++- 4 files changed, 9 insertions(+), 7 deletions(-)