Received-SPF: pass (google.com: domain of linux-kernel+bounces-146604-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: "D, Lakshmi Sowjanya" <lakshmi.sowjanya.d@intel.com>
To: Thomas Gleixner <tglx@linutronix.de>, "jstultz@google.com"
	<jstultz@google.com>, "giometti@enneenne.com" <giometti@enneenne.com>,
	"corbet@lwn.net" <corbet@lwn.net>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
CC: "x86@kernel.org" <x86@kernel.org>, "netdev@vger.kernel.org"
	<netdev@vger.kernel.org>, "linux-doc@vger.kernel.org"
	<linux-doc@vger.kernel.org>, "intel-wired-lan@lists.osuosl.org"
	<intel-wired-lan@lists.osuosl.org>, "andriy.shevchenko@linux.intel.com"
	<andriy.shevchenko@linux.intel.com>, "Dong, Eddie" <eddie.dong@intel.com>,
	"Hall, Christopher S" <christopher.s.hall@intel.com>, "Brandeburg, Jesse"
	<jesse.brandeburg@intel.com>, "davem@davemloft.net" <davem@davemloft.net>,
	"alexandre.torgue@foss.st.com" <alexandre.torgue@foss.st.com>,
	"joabreu@synopsys.com" <joabreu@synopsys.com>, "mcoquelin.stm32@gmail.com"
	<mcoquelin.stm32@gmail.com>, "perex@perex.cz" <perex@perex.cz>,
	"linux-sound@vger.kernel.org" <linux-sound@vger.kernel.org>, "Nguyen, Anthony
 L" <anthony.l.nguyen@intel.com>, "peter.hilber@opensynergy.com"
	<peter.hilber@opensynergy.com>, "N, Pandith" <pandith.n@intel.com>, "Mohan,
 Subramanian" <subramanian.mohan@intel.com>, "T R, Thejesh Reddy"
	<thejesh.reddy.t.r@intel.com>
Subject: RE: [PATCH v6 01/11] x86/tsc: Add base clock properties in
 clocksource structure
Thread-Topic: [PATCH v6 01/11] x86/tsc: Add base clock properties in
 clocksource structure
Thread-Index: AQHaiz0PazTrRHQXt0yKbKUcSArdGrFiBscAgAiuGPA=
Date: Tue, 16 Apr 2024 10:11:42 +0000
Message-ID: <CY8PR11MB73648707296EE27BF4CC6F8AC4082@CY8PR11MB7364.namprd11.prod.outlook.com>
References: <20240410114828.25581-1-lakshmi.sowjanya.d@intel.com>
 <20240410114828.25581-2-lakshmi.sowjanya.d@intel.com> <87cyqw3nup.ffs@tglx>
In-Reply-To: <87cyqw3nup.ffs@tglx>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ZA7M4/oYxkSo/ZjYlnBDW24roWcLAAX91UsF31ABUeCtSAlKpOTkhbHaAj3B?=
 =?us-ascii?Q?P42UqtpJWALNFSricGdcMbXZSY3XQ3EekK4bPwkWOKUdeDz9q0bPXEzlHdXI?=
 =?us-ascii?Q?8BFUPBOgTtJfRsE3himz03BSp/kKSFecUMh2Y77IiKaCnvc186Kfva+J+xho?=
 =?us-ascii?Q?Wkv1VvGxGINhSKzJUUDDNrDBHp1N2tKOUFdZgweKZYjr4gjwqFuL5GiEpEvk?=
 =?us-ascii?Q?Und6MS5fuk7ZrxxUkQmB0VPx6W2znjERI6LBGIh31C7JwZ8uxgRzQhNPX5z+?=
 =?us-ascii?Q?S96DhQw+Wu3bFvgxuaK2hElCyYpy/hxSKxw/gDGhfBcE1GRkJeNgsz7Bw6Rq?=
 =?us-ascii?Q?IPATcH2gf9Q/+UTE0u0IegOgNRoepmO5Q3pMfLuLmzloBJjNmjMyQ5HLoVeQ?=
 =?us-ascii?Q?+Vi2cbAd9PgAydwmZffHeF80lMSGeOvTx5yKu3gqIsmToWjg1QvukJnI52aj?=
 =?us-ascii?Q?wmKJuq38ADYj5IWqOcwUvpwnx5WwNp5JK1vb6Rxxw9bM9TcfSsUTn1hXQIo7?=
 =?us-ascii?Q?tzWMNnf4BK17Xe+/dd7IubdqPuTtkUAmldrYw2STb+Y+cLReUXEQxPj59Gzx?=
 =?us-ascii?Q?Uwx76sDmmxtxgo7S3nXg5dK9ZisB2s2fTfHhCoOGNX+/7lSmwxz9yGhm/Zym?=
 =?us-ascii?Q?teZtarUdUTDKhcrAySplq5r0JoZ+tPS+vbKIb3RylJufn5ghA72SMn55DmbC?=
 =?us-ascii?Q?jYDo+ysohf6tv1KTP7sqX7U/tXx0im4Qaz6h0/k5viyatvg2iT3xIiauKJym?=
 =?us-ascii?Q?ruNHeKPoC7gUEibZKsNdB9aa+j4LwcBseb4AiTVbpNRQZQtqhPkd+i2S3t3L?=
 =?us-ascii?Q?/Tq2tU1+fn89WZR+6LbHz9Vm/L5XaXPA+cWq1SsShCUyGkxL4UwQMXTNndHM?=
 =?us-ascii?Q?/UdGRurFQvlpZ0oywfQTXQ0kKr92XFcSAt3U1T1JtBu3ftNBvuu/I0MBdGB3?=
 =?us-ascii?Q?+agolXdAqMsdnICcRD6sZ1BO5yF0NnmqKZ/OkO/LkUDUKqs6YuXEGKokNlTk?=
 =?us-ascii?Q?5ugwB2W+hFkqlVkebv4eCcAl31VYlXNZ7N42Jfpfm28Q8EJcA316oZPlrtiO?=
 =?us-ascii?Q?niLtlQoUx0icjUQaJGmdhibz0MBrF2c7hhC7DfuT9IYgR0Fg+rmL5oFdXj87?=
 =?us-ascii?Q?mbb4SLrKBedVyM27lQ5XbcRhD2XkyAqqNClIgRLd5EBK2iGgUDBnIOwqqn7B?=
 =?us-ascii?Q?RnBBITAMH8zh9v0cm5cHsazFxqqTQsUJa3BFUhOZAD1RNOALPyctu8oeR2fk?=
 =?us-ascii?Q?peHrer49RYKNl5Lmwc2+R0jM6Afth6K15EVJuyYTGG5TVO7psLBSPlzotkTR?=
 =?us-ascii?Q?T+ryndD+ZVMyEh6OTIMAmhGzoGlIae96EWTWtozXliaUR9I4rZlLY71KmOAX?=
 =?us-ascii?Q?Y5W3YQ0LNvEuu/6hg8D2Zgs/ygUrooL5EcigW1UZLxopw8PT+ATKQsDCbTtv?=
 =?us-ascii?Q?D0N7XCfgcZoHXgiUY6RkcLGiiblXLbL1JoYEMQ4vnhmBUd7vHq2/rPx3uAiM?=
 =?us-ascii?Q?pFnhBfFxfgExitHT+bDOjM5FAcsuSyNOY6Efezu/Z0EG5R27jtNLY2lwE3co?=
 =?us-ascii?Q?tThszknHmA5NQdumfFjM2wpsb4fZWOuTDzGEPqzKURcnoD3BK84F/zpuT5l8?=
 =?us-ascii?Q?Kw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CY8PR11MB7364.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 07d6207c-e559-43cd-87fe-08dc5dfd9d16
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Apr 2024 10:11:42.2892
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: WB3gGtc2km70pDzq5wp+V4BWeEVKAY2cOsxaCrpznaISjYZYs5+zvStj3LB0shAsKH8VsBi+9Q7I6lSbOd7NFahowjvVhK5rxgt3Pvfu+LY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR11MB6617
X-OriginatorOrg: intel.com


> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Thursday, April 11, 2024 3:03 AM
> To: D, Lakshmi Sowjanya <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; in=
tel-
> wired-lan@lists.osuosl.org; andriy.shevchenko@linux.intel.com; Dong, Eddi=
e
> <eddie.dong@intel.com>; Hall, Christopher S <christopher.s.hall@intel.com=
>;
> Brandeburg, Jesse <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;
> Nguyen, Anthony L <anthony.l.nguyen@intel.com>;
> peter.hilber@opensynergy.com; N, Pandith <pandith.n@intel.com>; Mohan,
> Subramanian <subramanian.mohan@intel.com>; T R, Thejesh Reddy
> <thejesh.reddy.t.r@intel.com>; D, Lakshmi Sowjanya
> <lakshmi.sowjanya.d@intel.com>
> Subject: Re: [PATCH v6 01/11] x86/tsc: Add base clock properties in clock=
source
> structure
>=20
> 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;
>=20
> Q: Why is this a bad place to add this member?
>=20
> A: Because it creates a 4 byte hole in the data structure.
>=20
> >  	const char		*name;
> >  	struct list_head	list;
>=20
> While adding it here fills a 4 byte hole.
>=20
> Hint:
>=20
>   pahole -c clocksource kernel/time/clocksource.o
>=20
> would have told you that.
>=20
> >  	int			rating;
> >  	enum clocksource_ids	id;
> >  	enum vdso_clock_mode	vdso_clock_mode;
> >  	unsigned long		flags;
> > +	struct clocksource_base *base;
>=20
> > 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 =3D div64_u64_rem(*val, denominator, &rem) * numerator;
> > +	*val =3D res + div_u64(rem * numerator, denominator);
> > +	return true;
> > +}
> > +
> > +static bool convert_base_to_cs(struct system_counterval_t *scv) {
> > +	struct clocksource *cs =3D tk_core.timekeeper.tkr_mono.clock;
> > +	struct clocksource_base *base =3D cs->base;
> > +	u32 num, den;
> > +
> > +	/* The timestamp was taken from the time keeper clock source */
> > +	if (cs->id =3D=3D scv->cs_id)
> > +		return true;
> > +
> > +	/* Check whether cs_id matches the base clock */
> > +	if (!base || base->id !=3D scv->cs_id)
> > +		return false;
> > +
> > +	num =3D scv->use_nsecs ? cs->freq_khz : base->numerator;
> > +	den =3D scv->use_nsecs ? USEC_PER_SEC : base->denominator;
> > +
> > +	convert_clock(&scv->cycles, num, den);
>=20
> Q: Why does this ignore the return value of convert_clock() ?
>=20
> A: Because all drivers will correctly fill in everything.
>=20
> Q: Then why does convert_clock() bother to check and have a return
>    value?
>=20
> 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

>=20
> Thanks,
>=20
>         tglx