Received-SPF: pass (google.com: domain of linux-kernel+bounces-126667-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
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>,
	"Sangannavar, Mallikarjunappa" <mallikarjunappa.sangannavar@intel.com>,
	"Mohan, Subramanian" <subramanian.mohan@intel.com>, "Goudar, Basavaraj"
	<basavaraj.goudar@intel.com>, "T R, Thejesh Reddy"
	<thejesh.reddy.t.r@intel.com>
Subject: RE: [PATCH v5 02/11] timekeeping: Add function to convert realtime to
 base clock
Thread-Topic: [PATCH v5 02/11] timekeeping: Add function to convert realtime
 to base clock
Thread-Index: AQHaef5Bjihn85FGzE6EBTZu2xn8QrFCSw0AgBErO+A=
Date: Mon, 1 Apr 2024 13:07:56 +0000
Message-ID: <CY8PR11MB736402AC980408384A213960C43F2@CY8PR11MB7364.namprd11.prod.outlook.com>
References: <20240319130547.4195-1-lakshmi.sowjanya.d@intel.com>
 <20240319130547.4195-3-lakshmi.sowjanya.d@intel.com> <87le6bhc0t.ffs@tglx>
In-Reply-To: <87le6bhc0t.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?dcJ9dT2VPI7ZTp3iMRK0/RXvUlboM6zrVYxCYZIAmgtiUYPaXKi6b6y1w4uz?=
 =?us-ascii?Q?qLeB805pnowAHgU6BVd1cY34aucuFSv4Ua2nFN9+Ikm8XqIjbignKPLK3hHc?=
 =?us-ascii?Q?PIJfU5LE5vmxAYk4+hW0NL8gFyd4jZ31NE+e2zp/hu1FibkvyvVZc5iavU3O?=
 =?us-ascii?Q?+jph27MQB3kvfqN22HZDFmklOvAV/PhIZuAgYPO7lyYSnRVqj74N1CN6ncs9?=
 =?us-ascii?Q?Y8cQ8eRH0jOn6cDv0mzplIy9NeT/KSxetNVu7np5NOOFnfwzrGlYk4G/wZtV?=
 =?us-ascii?Q?ic566cVq6hhpFScmbgn2zbtkH0izzPNH9i8EIASx1SW99MSBPhIEwHQ+eRWF?=
 =?us-ascii?Q?cPR1rwcBDkmzB0wYhHNCzjRoJjIULOx7xTosZA8FsBABwzJbDbYWJ43u3kL8?=
 =?us-ascii?Q?8RdeY/R/fCr9vf6vwm4qGI7a9bJNrdPQnHJ676aj24nOH+AYdtWOpxrxhKJw?=
 =?us-ascii?Q?OO82Dhesbl+7uy3Sfa5kOuLPmOBHWr5GtbXzbu6Pq6rPl/7fRxalI7iTg89f?=
 =?us-ascii?Q?eoDddcu3mx7T8qgtM+pPOy422SYIG5wrLnJhbe8DUMR4H572m2+EtlNuX8pf?=
 =?us-ascii?Q?WJjP7exS187a5PSjkbzWD8jXqhCCdKh2WtN19it2yt6SckVwodUjRihbrhxK?=
 =?us-ascii?Q?TWoTBkr61/PVyTON/aonIN+/3bGR5XCtnLi9c/I761Vk5NJtVCsS4tcL8x74?=
 =?us-ascii?Q?yULPCQxL36kjkyMKpzchakx0KV+7cggwYsfgWI0ljaMG4DTweWzMbAkKq5mg?=
 =?us-ascii?Q?A51D83HRkr+ZJBVErJe+dIMiTAh19dffGKtR17U6aB7dU4/+z3pDNgRPnB33?=
 =?us-ascii?Q?dtSsaQlD9UxQPcqJXyt0KAi1IB0pCW3k02LVEtPuIB38D+12MmmXviHr3qtR?=
 =?us-ascii?Q?i+ewcZsn+ew9f5hVchjptZsEWqk4GML7riSvFKFmp0fBxru9I03FCirZ8/vO?=
 =?us-ascii?Q?YJQNaRhMRNOqhaO90IsShX9aDWzOkfm+52Xn5qz9m0AlFIOUxqls2K4nKNnw?=
 =?us-ascii?Q?UZK1uFLVyZCUTKaqZDReopyp4WtIkMeCEC0KqIu5mVyw7qJVbID98lHICzAd?=
 =?us-ascii?Q?8I/AmMEuvGEvZ9a4JoA/ytpDUquQkKrLcR9tAUqh8/uMVgYNGSKh1kshGNKl?=
 =?us-ascii?Q?75tlXJWlHcx7GpzvjYhGRsGc4VleBKljzXveck++YryNS8dUcERuiMN6mjo3?=
 =?us-ascii?Q?T1Oo98RAhdrTNxNb39FGQPXBf5RWNqj7RP6qYq+tKxd+J6y0XD6sc6zBFJVd?=
 =?us-ascii?Q?9JU/WfVhf2tZJHSM6guPzzUm0GFNzRpNj3wF8ORt1A7++HPU3TgcEDTE6hcp?=
 =?us-ascii?Q?0j3mTZd7l+8ASkckBOfgbjWL7xXjS/HRm3GjYO82Dftx2V5yHrzeLjX/CiLu?=
 =?us-ascii?Q?PxnoUDpn1hO+1PteWxmvV/8VEC6sW4vBQol7LTzbgQxqBFrCU3FosRuYaZyF?=
 =?us-ascii?Q?f8TQryeXluiubYKAr/z6yytCiNhuNGrZjpdQ0I0cVZVL86uOFowiZSED2Whl?=
 =?us-ascii?Q?AbXZXjSQOSs/E1ovZsXL34Ls5XWnAV+d/MStIbwdreOyixBeDnaXzFCpI4Z7?=
 =?us-ascii?Q?NZjiM1qJps6YJKR35LGDJJPdFoWa5kBdFXM6qAQSSmvuRwB5HBui2i2EzSPT?=
 =?us-ascii?Q?Wg=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: 3cb76629-8962-4b45-899c-08dc524cbf6c
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Apr 2024 13:07:56.1709
 (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: GEObS1EXnqmW5Lma6Qbb+5SD3Jmbe8tEfPTU4Q8Zl/lAdmvHceciDxS9jUHSj3hAMcvAAsUJK/3DVk7CzUjXuFqbuENIxfOes6ygQILYLM8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ1PR11MB6131
X-OriginatorOrg: intel.com


> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Thursday, March 21, 2024 8:23 PM
> 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>;
> Sangannavar, Mallikarjunappa <mallikarjunappa.sangannavar@intel.com>;
> Mohan, Subramanian <subramanian.mohan@intel.com>; Goudar, Basavaraj
> <basavaraj.goudar@intel.com>; T R, Thejesh Reddy
> <thejesh.reddy.t.r@intel.com>; D, Lakshmi Sowjanya
> <lakshmi.sowjanya.d@intel.com>
> Subject: Re: [PATCH v5 02/11] timekeeping: Add function to convert realti=
me to
> base clock
>=20
> On Tue, Mar 19 2024 at 18:35, lakshmi.sowjanya.d@intel.com wrote:
> > +bool ktime_real_to_base_clock(ktime_t treal, enum clocksource_ids
> > +base_id, u64 *cycles) {
> > +	struct timekeeper *tk =3D &tk_core.timekeeper;
> > +	unsigned int seq;
> > +	u64 delta;
> > +
> > +	do {
> > +		seq =3D read_seqcount_begin(&tk_core.seq);
> > +		delta =3D (u64)treal - tk->tkr_mono.base_real;
> > +		if (delta > tk->tkr_mono.clock->max_idle_ns)
> > +			return false;
>=20
> I don't think this cutoff is valid. There is no guarantee that this is li=
near unless:
>=20
>        Treal[last timekeeper update] <=3D treal < Treal[next timekeeper u=
pdate]
>=20
> Look at the dance in get_device_system_crosststamp() and
> adjust_historical_crosststamp() to see why.
>=20
> > +		*cycles =3D tk->tkr_mono.cycle_last + convert_ns_to_cs(delta);
> > +		if (!convert_cs_to_base(cycles, base_id))
> > +			return false;
> > +	} while (read_seqcount_retry(&tk_core.seq, seq));
> > +
> > +	return true;
> > +}
> > +EXPORT_SYMBOL_GPL(ktime_real_to_base_clock);
>=20
> Looking at the usage site:
>=20
> > +static bool pps_generate_next_pulse(struct pps_tio *tio, ktime_t
> > +expires) {
> > +	u64 art;
> > +
> > +	if (!ktime_real_to_base_clock(expires, CSID_X86_ART, &art)) {
> > +		pps_tio_disable(tio);
>=20
> I'm pretty sure this can happen when there is sufficient delay between th=
e check
> for (now - expires < SAFE_TIME_NS) and the delta computation in
> ktime_real_to_base_clock() if there is a timerkeeper update interleaving =
which
> brings tkr_mono.base_real in front of expires.
>=20
> Is that intentional and correct?
>=20
> If so, then it's inconsistent with the behaviour of the hrtimer
> callback:
>=20
> > +		return false;
> > +	}
> > +
> > +	pps_compv_write(tio, art - ART_HW_DELAY_CYCLES);
> > +	return true;
> > +}
> > +
> > +static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer) {
> > +	struct pps_tio *tio =3D container_of(timer, struct pps_tio, timer);
> > +	ktime_t expires, now;
> > +
> > +	guard(spinlock)(&tio->lock);
> > +
> > +	expires =3D hrtimer_get_expires(timer);
> > +	now =3D ktime_get_real();
> > +
> > +	if (now - expires < SAFE_TIME_NS) {
> > +		if (!pps_generate_next_pulse(tio, expires + SAFE_TIME_NS))
> > +			return HRTIMER_NORESTART;
> > +	}
>=20
> This safe guard does not care about time being set. I'm not familiar with=
 the PPS
> logic, but is it expected that the pulse pattern will be like this:
>=20
>=20
>=20
>     ---|-----|-----|-----|----------------->
>        P     P  ^        P
>                 |
>         clock_settime(CLOCK_REALTIME, now - 2 seconds)
>=20
>         Obviously the pulse gap will be as big as the time is set
>         backwards, which might be way more than 2 seconds.
>=20
>=20
>     ---|-----|-----|-----|----------------->
>        P     P  ^  P     P
>                 |
>         clock_settime(CLOCK_REALTIME, now + 2 seconds)
>=20
> I don't see anything in this code which cares about CLOCK_REALTIME being =
set
> via clock_settime() or adjtimex().
>=20
> Aside of that I have a question about how the TIO hardware treats this
> case:
>=20
>    ktime_real_to_base_clock(expires, &art);
>=20
> -> GAP which makes @art get into the past
>=20
>    pps_compv_write(tio, art - ART_HW_DELAY_CYCLES);
>=20
> Will the hardware ignore that already expired value or just emit a pulse
> immediately? In the latter case the pulse will be at a random point in ti=
me, which
> does not sound correct.

Thanks for the review Thomas,

If an expired time is programmed to hardware, it ignores and no pulse is ge=
nerated.
There is an event counter feature in hardware, this increments on every pul=
se sent. In next version of driver, we will be monitoring this counter to d=
etect any missed pulse and disable the hardware.

Regards,
Sowjanya

>=20
> Thanks,
>=20
>         tglx