On Tue, Mar 19 2024 at 18:35, lakshmi.sowjanya.d@xxxxxxxxx wrote:
> +bool ktime_real_to_base_clock(ktime_t treal, enum clocksource_ids base_id, u64 *cycles)
> +{
> + 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;
I don't think this cutoff is valid. There is no guarantee that this is
linear unless:
Treal[last timekeeper update] <= treal < Treal[next timekeeper update]
Look at the dance in get_device_system_crosststamp() and
adjust_historical_crosststamp() to see why.
> + *cycles = tk->tkr_mono.cycle_last + convert_ns_to_cs(delta);
> + if (!convert_cs_to_base(cycles, base_id))
> + return false;
> + } while (read_seqcount_retry(&tk_core.seq, seq));
> +
> + return true;
> +}
> +EXPORT_SYMBOL_GPL(ktime_real_to_base_clock);
Looking at the usage site:
> +static bool pps_generate_next_pulse(struct pps_tio *tio, ktime_t expires)
> +{
> + u64 art;
> +
> + if (!ktime_real_to_base_clock(expires, CSID_X86_ART, &art)) {
> + pps_tio_disable(tio);
I'm pretty sure this can happen when there is sufficient delay between
the check for (now - expires < SAFE_TIME_NS) and the delta computation
in ktime_real_to_base_clock() if there is a timerkeeper update
interleaving which brings tkr_mono.base_real in front of expires.
Is that intentional and correct?
If so, then it's inconsistent with the behaviour of the hrtimer
callback:
> + return false;
> + }
> +
> + pps_compv_write(tio, art - ART_HW_DELAY_CYCLES);
> + return true;
> +}
> +
> +static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer)
> +{
> + struct pps_tio *tio = container_of(timer, struct pps_tio, timer);
> + ktime_t expires, now;
> +
> + guard(spinlock)(&tio->lock);
> +
> + expires = hrtimer_get_expires(timer);
> + now = ktime_get_real();
> +
> + if (now - expires < SAFE_TIME_NS) {
> + if (!pps_generate_next_pulse(tio, expires + SAFE_TIME_NS))
> + return HRTIMER_NORESTART;
> + }
This safe guard does not care about time being set. I'm not familiar
with the PPS logic, but is it expected that the pulse pattern will be
like this:
---|-----|-----|-----|----------------->
P P ^ P
|
clock_settime(CLOCK_REALTIME, now - 2 seconds)
Obviously the pulse gap will be as big as the time is set
backwards, which might be way more than 2 seconds.
---|-----|-----|-----|----------------->
P P ^ P P
|
clock_settime(CLOCK_REALTIME, now + 2 seconds)
I don't see anything in this code which cares about CLOCK_REALTIME being
set via clock_settime() or adjtimex().
Aside of that I have a question about how the TIO hardware treats this
case:
ktime_real_to_base_clock(expires, &art);
-> GAP which makes @art get into the past
pps_compv_write(tio, art - ART_HW_DELAY_CYCLES);
Will the hardware ignore that already expired value or just emit a pulse
immediately? In the latter case the pulse will be at a random point in
time, which does not sound correct.
Thanks,
tglx
