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

[Thomas Gleixner <tglx@xxxxxxxxxxxxx>]

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.
  
> +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.

> +{
> +	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.

> +		*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