RE: [PATCH v6 08/11] timekeeping: Add function to convert realtime to base clock

[D, Lakshmi Sowjanya]

> -----Original Message-----
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Sent: Thursday, April 11, 2024 3:46 AM
> To: D, Lakshmi Sowjanya <lakshmi.sowjanya.d@xxxxxxxxx>;
> jstultz@xxxxxxxxxx; giometti@xxxxxxxxxxxx; corbet@xxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Cc: x86@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; intel-
> wired-lan@xxxxxxxxxxxxxxxx; andriy.shevchenko@xxxxxxxxxxxxxxx; Dong, Eddie
> <eddie.dong@xxxxxxxxx>; Hall, Christopher S <christopher.s.hall@xxxxxxxxx>;
> Brandeburg, Jesse <jesse.brandeburg@xxxxxxxxx>; davem@xxxxxxxxxxxxx;
> alexandre.torgue@xxxxxxxxxxx; joabreu@xxxxxxxxxxxx;
> mcoquelin.stm32@xxxxxxxxx; perex@xxxxxxxx; linux-sound@xxxxxxxxxxxxxxx;
> Nguyen, Anthony L <anthony.l.nguyen@xxxxxxxxx>;
> peter.hilber@xxxxxxxxxxxxxxx; N, Pandith <pandith.n@xxxxxxxxx>; Mohan,
> Subramanian <subramanian.mohan@xxxxxxxxx>; T R, Thejesh Reddy
> <thejesh.reddy.t.r@xxxxxxxxx>; D, Lakshmi Sowjanya
> <lakshmi.sowjanya.d@xxxxxxxxx>
> Subject: Re: [PATCH v6 08/11] timekeeping: Add function to convert realtime to
> base clock
> 
> On Wed, Apr 10 2024 at 17:18, lakshmi.sowjanya.d@xxxxxxxxx wrote:
> > From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@xxxxxxxxx>
> >
> > 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