Received-SPF: pass (google.com: domain of linux-kernel+bounces-139374-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Thomas Gleixner <tglx@linutronix.de>
To: lakshmi.sowjanya.d@intel.com, jstultz@google.com, giometti@enneenne.com,
 corbet@lwn.net, linux-kernel@vger.kernel.org
Cc: x86@kernel.org, netdev@vger.kernel.org, linux-doc@vger.kernel.org,
 intel-wired-lan@lists.osuosl.org, andriy.shevchenko@linux.intel.com,
 eddie.dong@intel.com, christopher.s.hall@intel.com,
 jesse.brandeburg@intel.com, davem@davemloft.net,
 alexandre.torgue@foss.st.com, joabreu@synopsys.com,
 mcoquelin.stm32@gmail.com, perex@perex.cz, linux-sound@vger.kernel.org,
 anthony.l.nguyen@intel.com, peter.hilber@opensynergy.com,
 pandith.n@intel.com, subramanian.mohan@intel.com,
 thejesh.reddy.t.r@intel.com, lakshmi.sowjanya.d@intel.com
Subject: Re: [PATCH v6 01/11] x86/tsc: Add base clock properties in
 clocksource structure
In-Reply-To: <20240410114828.25581-2-lakshmi.sowjanya.d@intel.com>
References: <20240410114828.25581-1-lakshmi.sowjanya.d@intel.com>
 <20240410114828.25581-2-lakshmi.sowjanya.d@intel.com>
Date: Wed, 10 Apr 2024 23:32:46 +0200
Message-ID: <87cyqw3nup.ffs@tglx>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

On Wed, Apr 10 2024 at 17:18, lakshmi.sowjanya.d@intel.com 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