> -----Original Message-----
> From: Peter Hilber <peter.hil...@opensynergy.com>
> Sent: Tuesday, April 30, 2024 7:58 PM
> To: D, Lakshmi Sowjanya <lakshmi.sowjany...@intel.com>;
> t...@linutronix.de; 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>;
> N, Pandith <pandit...@intel.com>; Mohan, Subramanian
> <subramanian.mo...@intel.com>; T R, Thejesh Reddy
> <thejesh.reddy....@intel.com>
> Subject: Re: [PATCH v7 01/12] timekeeping: Add base clock properties in
> clocksource structure
>
> On 30.04.24 10:52, lakshmi.sowjany...@intel.com wrote:
> > From: Lakshmi Sowjanya D <lakshmi.sowjany...@intel.com>
> >
> > Add base clock hardware abstraction in clocksource structure.
> >
> > Provide generic functionality in convert_base_to_cs() to convert base
> > clock timestamps to system clocksource without requiring architecture
> > specific parameters.
> >
> > This is intended to replace convert_art_to_tsc() and
> > convert_art_ns_to_tsc() functions which are specific to convert ART
> > (Always Running Timer) time to the corresponding TSC value.
> >
> > Add the infrastructure in get_device_system_crosststamp().
> >
> > Co-developed-by: Thomas Gleixner <t...@linutronix.de>
> > Signed-off-by: Thomas Gleixner <t...@linutronix.de>
> > Co-developed-by: Christopher S. Hall <christopher.s.h...@intel.com>
> > Signed-off-by: Christopher S. Hall <christopher.s.h...@intel.com>
> > Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjany...@intel.com>
> > ---
> > include/linux/clocksource.h | 27 +++++++++++++++++++++++++
> > include/linux/timekeeping.h | 2 ++
> > kernel/time/timekeeping.c | 39
> +++++++++++++++++++++++++++++++++++--
> > 3 files changed, 66 insertions(+), 2 deletions(-)
> >
> [...]
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index b58dffc58a8f..4e5e931e766a 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -1193,6 +1193,42 @@ static bool timestamp_in_interval(u64 start, u64
> end, u64 ts)
> > return false;
> > }
> >
> > +static bool convert_clock(u64 *val, u32 numerator, u32 denominator) {
> > + u64 rem, res;
> > +
> > + if (!numerator || !denominator)
> > + return false;
> > +
> > + res = div64_u64_rem(*val, denominator, &rem) * numerator;
> > + *val = res + div_u64(rem * numerator, denominator);
> > + return true;
> > +}
> > +
> > +static bool convert_base_to_cs(struct system_counterval_t *scv) {
> > + struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
> > + struct clocksource_base *base = cs->base;
> > + u32 num, den;
> > +
> > + /* The timestamp was taken from the time keeper clock source */
> > + if (cs->id == scv->cs_id)
> > + return true;
> > +
> > + /* Check whether cs_id matches the base clock */
> > + if (!base || base->id != scv->cs_id)
> > + return false;
> > +
> > + num = scv->use_nsecs ? cs->freq_khz : base->numerator;
> > + den = scv->use_nsecs ? USEC_PER_SEC : base->denominator;
> > +
> > + if (!convert_clock(&scv->cycles, num, den))
> > + return false;
> > +
> > + scv->cycles += base->offset;
> > + return true;
> > +}
> > +
> > /**
> > * get_device_system_crosststamp - Synchronously capture
> system/device timestamp
> > * @get_time_fn: Callback to get simultaneous device time and
> > @@ -1238,8 +1274,7 @@ int get_device_system_crosststamp(int
> (*get_time_fn)
> > * system counter value is the same as for the currently
> > * installed timekeeper clocksource
> > */
> > - if (system_counterval.cs_id == CSID_GENERIC ||
> > - tk->tkr_mono.clock->id != system_counterval.cs_id)
> > + if (!convert_base_to_cs(&system_counterval))
> > return -ENODEV;
>
> AFAIU the error condition
>
> system_counterval.cs_id == CSID_GENERIC
>
> is silently dropped by this patch, but shouldn't be.
>
> get_device_system_crosststamp() can only check the identity of a
> clocksource (base) for non-generic ids.
>
> Regards,
>
> Peter
Thanks Peter,
Noted. The check will be added as below:
if (system_counterval.cs_id == CSID_GENERIC ||
!convert_base_to_cs(&system_counterval))
Regards,
Sowjanya
