Lakshmi! On Tue, Oct 17 2023 at 10:54, lakshmi.sowjanya.d@xxxxxxxxx wrote: 'kernel/time:' is not a proper subsystem prefix. https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-subject > Support system-clock to system-counter conversion. Intel Timed IO > hardware, using system counter as reference to schedule events. This tells me WHAT the patch is doing but not at all WHY this is required and that Intel Timed IO hardware reference is just not cutting it. > +extern int ktime_convert_real_to_system_counter(ktime_t sys_realtime, > + struct system_counterval_t *ret); The changelog talks about system-clock. The function name and the argument tell that this is about real-time. Thats confusing at best. > +static inline int timekeeping_ns_to_delta(const struct tk_read_base *tkr, u64 nsec, > + u64 *cycles) > +{ > + if (BITS_TO_BYTES(fls64(nsec) + tkr->shift) > sizeof(nsec)) > + return -ERANGE; Without a comment you will not be able to grok that check in three months from now. > + *cycles = nsec << tkr->shift; > + *cycles -= tkr->xtime_nsec; > + do_div(*cycles, tkr->mult); > + > + return 0; > +} > + > static inline u64 timekeeping_delta_to_ns(const struct tk_read_base *tkr, u64 delta) > { > u64 nsec; > @@ -1303,6 +1316,47 @@ int get_device_system_crosststamp(int (*get_time_fn) > } > EXPORT_SYMBOL_GPL(get_device_system_crosststamp); > > +/** > + * ktime_convert_real_to_system_counter - Convert system time to system counter System time is _NOT_ the same as clock realtime, really. What's wrong with consistently using clock realtime instead of making up some new terminology? > + * value Pointless line break which makes this unreadable. > + * @sys_realtime: realtime clock value to convert What means sys_realtime? The argument supplies an absolute time value based on clock realtime and not on some magic system realtime. > + * @ret: Computed system counter value with clocksource pointer So @ret is returning the computed value along with a clocksource pointer or what? > + * Converts a supplied, future realtime clock value to the corresponding > + * system counter value. > + * > + * Return: 0 on success, -errno on failure. If this really needs error codes, then please explicitly document them. If not, then make the function return bool and be done with it. > + */ > +int ktime_convert_real_to_system_counter(ktime_t sys_realtime, > + struct system_counterval_t *ret) > +{ > + struct timekeeper *tk = &tk_core.timekeeper; > + ktime_t base_real; > + unsigned int seq; > + u64 ns_delta; > + int err; > + > + do { > + seq = read_seqcount_begin(&tk_core.seq); > + > + base_real = ktime_add(tk->tkr_mono.base, > + tk_core.timekeeper.offs_real); > + if (ktime_compare(sys_realtime, base_real) < 0) > + return -EINVAL; > + > + ret->cs = tk->tkr_mono.clock; > + ns_delta = ktime_to_ns(ktime_sub(sys_realtime, base_real)); > + err = timekeeping_ns_to_delta(&tk->tkr_mono, ns_delta, &ret->cycles); > + if (err < 0) > + return err; This is completely uncomprehensible especially with the hidden range check in the conversion function. All of this can be simplified to: bool ktime_real_to_system_counter(ktime_t treal, struct system_counterval_t *scv) { struct timekeeper *tk = &tk_core.timekeeper; unsigned int seq; u64 delta; do { seq = read_seqcount_begin(&tk_core.seq); delta = (u64)treal - tk->tkr_mono.base_real; if (delta > tk->tkr_mono.clock->max_idle_ns) return false; scv->cycles = timekeeping_ns_to_delta(&tk->tkr_mono, delta); scv->cycles += tk->tkr_mono.cycle_last; scv->cs = tk->tkr_mono.clock; } while (read_seqcount_retry(&tk_core.seq, seq)); return true; } That gets rid of this hideous range check in the conversion function and makes the code simple and straight forward, no? Related question: What guarantee that the supplied value is not in the past? Nothing. It only guarantees that it is not before the realtime base value, which can be up to one second in the past. Maybe it does not matter, but then the 'future realtime clock value' in the function documentation is wishful thinking. Thanks, tglx
