lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CY8PR11MB73648707296EE27BF4CC6F8AC4082@CY8PR11MB7364.namprd11.prod.outlook.com>
Date: Tue, 16 Apr 2024 10:11:42 +0000
From: "D, Lakshmi Sowjanya" <lakshmi.sowjanya.d@...el.com>
To: Thomas Gleixner <tglx@...utronix.de>, "jstultz@...gle.com"
	<jstultz@...gle.com>, "giometti@...eenne.com" <giometti@...eenne.com>,
	"corbet@....net" <corbet@....net>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
CC: "x86@...nel.org" <x86@...nel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>, "intel-wired-lan@...ts.osuosl.org"
	<intel-wired-lan@...ts.osuosl.org>, "andriy.shevchenko@...ux.intel.com"
	<andriy.shevchenko@...ux.intel.com>, "Dong, Eddie" <eddie.dong@...el.com>,
	"Hall, Christopher S" <christopher.s.hall@...el.com>, "Brandeburg, Jesse"
	<jesse.brandeburg@...el.com>, "davem@...emloft.net" <davem@...emloft.net>,
	"alexandre.torgue@...s.st.com" <alexandre.torgue@...s.st.com>,
	"joabreu@...opsys.com" <joabreu@...opsys.com>, "mcoquelin.stm32@...il.com"
	<mcoquelin.stm32@...il.com>, "perex@...ex.cz" <perex@...ex.cz>,
	"linux-sound@...r.kernel.org" <linux-sound@...r.kernel.org>, "Nguyen, Anthony
 L" <anthony.l.nguyen@...el.com>, "peter.hilber@...nsynergy.com"
	<peter.hilber@...nsynergy.com>, "N, Pandith" <pandith.n@...el.com>, "Mohan,
 Subramanian" <subramanian.mohan@...el.com>, "T R, Thejesh Reddy"
	<thejesh.reddy.t.r@...el.com>
Subject: RE: [PATCH v6 01/11] x86/tsc: Add base clock properties in
 clocksource structure



> -----Original Message-----
> From: Thomas Gleixner <tglx@...utronix.de>
> Sent: Thursday, April 11, 2024 3:03 AM
> To: D, Lakshmi Sowjanya <lakshmi.sowjanya.d@...el.com>;
> jstultz@...gle.com; giometti@...eenne.com; corbet@....net; linux-
> kernel@...r.kernel.org
> Cc: x86@...nel.org; netdev@...r.kernel.org; linux-doc@...r.kernel.org; intel-
> wired-lan@...ts.osuosl.org; andriy.shevchenko@...ux.intel.com; Dong, Eddie
> <eddie.dong@...el.com>; Hall, Christopher S <christopher.s.hall@...el.com>;
> Brandeburg, Jesse <jesse.brandeburg@...el.com>; davem@...emloft.net;
> alexandre.torgue@...s.st.com; joabreu@...opsys.com;
> mcoquelin.stm32@...il.com; perex@...ex.cz; linux-sound@...r.kernel.org;
> Nguyen, Anthony L <anthony.l.nguyen@...el.com>;
> peter.hilber@...nsynergy.com; N, Pandith <pandith.n@...el.com>; Mohan,
> Subramanian <subramanian.mohan@...el.com>; T R, Thejesh Reddy
> <thejesh.reddy.t.r@...el.com>; D, Lakshmi Sowjanya
> <lakshmi.sowjanya.d@...el.com>
> Subject: Re: [PATCH v6 01/11] x86/tsc: Add base clock properties in clocksource
> structure
> 
> On Wed, Apr 10 2024 at 17:18, lakshmi.sowjanya.d@...el.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

Agreed.
Will add a check for error case:

	if (!convert_clock(&scv->cycles, num, den))
             		return false;

Thanks,
Sowjanya

> 
> Thanks,
> 
>         tglx

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ