On Wed, Apr 10 2024 at 17:18, lakshmi.sowjanya.d@xxxxxxxxx wrote:
> @@ -48,6 +49,7 @@ struct module;
> * @archdata: Optional arch-specific data
> * @max_cycles: Maximum safe cycle value which won't overflow on
> * multiplication
> + * @freq_khz: Clocksource frequency in khz.
> * @name: Pointer to clocksource name
> * @list: List head for registration (internal)
> * @rating: Rating value for selection (higher is better)
> @@ -70,6 +72,8 @@ struct module;
> * validate the clocksource from which the snapshot was
> * taken.
> * @flags: Flags describing special properties
> + * @base: Hardware abstraction for clock on which a clocksource
> + * is based
> * @enable: Optional function to enable the clocksource
> * @disable: Optional function to disable the clocksource
> * @suspend: Optional suspend function for the clocksource
> @@ -105,12 +109,14 @@ struct clocksource {
> struct arch_clocksource_data archdata;
> #endif
> u64 max_cycles;
> + u32 freq_khz;
Q: Why is this a bad place to add this member?
A: Because it creates a 4 byte hole in the data structure.
> const char *name;
> struct list_head list;
While adding it here fills a 4 byte hole.
Hint:
pahole -c clocksource kernel/time/clocksource.o
would have told you that.
> int rating;
> enum clocksource_ids id;
> enum vdso_clock_mode vdso_clock_mode;
> unsigned long flags;
> + struct clocksource_base *base;
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index b58dffc58a8f..2542cfefbdee 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -1193,6 +1193,40 @@ 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;
> +
> + convert_clock(&scv->cycles, num, den);
Q: Why does this ignore the return value of convert_clock() ?
A: Because all drivers will correctly fill in everything.
Q: Then why does convert_clock() bother to check and have a return
value?
A: Because drivers will fail to correctly fill in everything
Thanks,
tglx
