> -----Original Message-----
> From: Thomas Gleixner <t...@linutronix.de>
> Sent: Thursday, April 11, 2024 3:46 AM
> To: D, Lakshmi Sowjanya <lakshmi.sowjany...@intel.com>;
> jstu...@google.com; giome...@enneenne.com; cor...@lwn.net; linux-
> ker...@vger.kernel.org
> Cc: x...@kernel.org; net...@vger.kernel.org; linux-...@vger.kernel.org; intel-
> wired-...@lists.osuosl.org; andriy.shevche...@linux.intel.com; Dong, Eddie
> <eddie.d...@intel.com>; Hall, Christopher S <christopher.s.h...@intel.com>;
> Brandeburg, Jesse <jesse.brandeb...@intel.com>; da...@davemloft.net;
> alexandre.tor...@foss.st.com; joab...@synopsys.com;
> mcoquelin.st...@gmail.com; pe...@perex.cz; linux-so...@vger.kernel.org;
> Nguyen, Anthony L <anthony.l.ngu...@intel.com>;
> peter.hil...@opensynergy.com; N, Pandith <pandit...@intel.com>; Mohan,
> Subramanian <subramanian.mo...@intel.com>; T R, Thejesh Reddy
> <thejesh.reddy....@intel.com>; D, Lakshmi Sowjanya
> <lakshmi.sowjany...@intel.com>
> Subject: Re: [PATCH v6 08/11] timekeeping: Add function to convert realtime to
> base clock
>
> On Wed, Apr 10 2024 at 17:18, lakshmi.sowjany...@intel.com wrote:
> > From: Lakshmi Sowjanya D <lakshmi.sowjany...@intel.com>
> >
> > PPS(Pulse Per Second) generates signals in realtime, but Timed IO
>
> ... generates signals based on CLOCK_REALTIME, but ...
>
> > hardware understands time in base clock reference.
>
> The hardware does not understand anything.
>
> > Add an interface,
> > ktime_real_to_base_clock() to convert realtime to base clock.
> >
> > Add the helper function timekeeping_clocksource_has_base(), to check
> > whether the current clocksource has the same base clock. This will be
> > used by Timed IO device to check if the base clock is X86_ART(Always
> > Running Timer).
>
> Again this fails to explain the rationale and as this is a core change which
> is
> hardware agnostic the whole Timed IO and ART reference is not really helpful.
> Something like this:
>
> "PPS (Pulse Per Second) generates a hardware pulse every second based
> on CLOCK_REALTIME. This works fine when the pulse is generated in
> software from a hrtimer callback function.
>
> For hardware which generates the pulse by programming a timer it's
> required to convert CLOCK_REALTIME to the underlying hardware clock.
>
> The X86 Timed IO device is based on the Always Running Timer (ART),
> which is the base clock of the TSC, which is usually the system
> clocksource on X86.
>
> The core code already has functionality to convert base clock
> timestamps to system clocksource timestamps, but there is no support
> for converting the other way around.
>
> Provide the required functionality to support such devices in a
> generic way to avoid code duplication in drivers:
>
> 1) ktime_real_to_base_clock() to convert a CLOCK_REALTIME
> timestamp to a base clock timestamp
>
> 2) timekeeping_clocksource_has_base() to allow drivers to validate
> that the system clocksource is based on a particular
> clocksource ID.
Thanks for the commit message.
I will update as suggested.
>
> > +static bool convert_cs_to_base(u64 *cycles, enum clocksource_ids
> > +base_id) {
> > + struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
> > + struct clocksource_base *base = cs->base;
> > +
> > + /* Check whether base_id matches the base clock */
> > + if (!base || base->id != base_id)
> > + return false;
> > +
> > + *cycles -= base->offset;
> > + if (!convert_clock(cycles, base->denominator, base->numerator))
> > + return false;
> > + return true;
> > +}
> > +
> > +static u64 convert_ns_to_cs(u64 delta) {
> > + struct tk_read_base *tkr = &tk_core.timekeeper.tkr_mono;
> > +
> > + return div_u64((delta << tkr->shift) - tkr->xtime_nsec, tkr->mult);
> > +}
>
> > +bool ktime_real_to_base_clock(ktime_t treal, enum clocksource_ids
> > +base_id, u64 *cycles)
>
> As this is a kernel API function it really wants kernel-doc comment to
> explain the
> functionality, the arguments and the return value.
Will add the following documentation:
" ktime_real_to_base_clock()- Convert CLOCK_REALTIME timestamp to a base clock
timestamp.
@treal: CLOCK_REALTIME timestamp to convert.
@base_id: base clocksource id.
@*cycles: pointer to store the converted base clock timestamp.
Converts a supplied, future realtime clock value to the corresponding base
clock value.
Return: true if the conversion is successful, false otherwise."
>
> > +{
> > + struct timekeeper *tk = &tk_core.timekeeper;
> > + unsigned int seq;
> > + u64 delta;
> > +
> > + do {
> > + seq = read_seqcount_begin(&tk_core.seq);
> > + if ((u64)treal < tk->tkr_mono.base_real)
> > + return false;
> > + delta = (u64)treal - tk->tkr_mono.base_real;
>
> In the previous version you had a sanity check on delta:
>
> >>> + if (delta > tk->tkr_mono.clock->max_idle_ns)
> >>> + return false;
>
> And I told you:
>
> >> 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.
>
> So now there is not even a check anymore whether the delta conversion can
> overflow.
>
> There is zero explanation why this conversion is considered to be correct.
Adding the following check for delta overflow in convert_ns_to_cs function.
if (BITS_TO_BYTES(fls64(*delta) + tkr->shift) >= sizeof(*delta))
return false;
>
> > + *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;
> > +}
>
> > +/**
> > + * timekeeping_clocksource_has_base - Check whether the current
> clocksource
> > + * has a base clock
>
> s/has a base clock/is based on a given base clock
>
> > + * @id: The clocksource ID to check for
>
> base clocksource ID
>
> > + *
> > + * Note: The return value is a snapshot which can become invalid right
> > + * after the function returns.
> > + *
> > + * Return: true if the timekeeper clocksource has a base clock with @id,
> > + * false otherwise
> > + */
>
> Thanks,
>
> tglx
