Received-SPF: pass (google.com: domain of linux-kernel+bounces-190509-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
From: "D, Lakshmi Sowjanya" <lakshmi.sowjanya.d@intel.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
CC: "tglx@linutronix.de" <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>, "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>, "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 v8 10/12] pps: generators: Add PPS Generator TIO Driver
Thread-Topic: [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driver
Thread-Index: AQHapSHXUIkwGaeDCkGimvlL+UU2cbGVBGIAgBXrFfA=
Date: Mon, 27 May 2024 11:48:54 +0000
Message-ID: <CY8PR11MB7364D1C85099E4337408EBAFC4F02@CY8PR11MB7364.namprd11.prod.outlook.com>
References: <20240513103813.5666-1-lakshmi.sowjanya.d@intel.com>
 <20240513103813.5666-11-lakshmi.sowjanya.d@intel.com>
 <ZkH3GP2b9WTz9W3W@smile.fi.intel.com>
In-Reply-To: <ZkH3GP2b9WTz9W3W@smile.fi.intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?bjjOO8wz9xxq1DFgGMoWH5tb6Tne1BpDXMOz7G6ZS9c2SYuQEPN4Jtk7Byxv?=
 =?us-ascii?Q?fH3cYK0haIU1EOgaSwrCrEVprI/+em1QTq+8GA442jbj0ouH48V/r6Sl9DGI?=
 =?us-ascii?Q?8fzcJYC5rUbRjnlEAe2MMS9T+Kc0IIBBfr8skyiRLfaatbNsnBRat3lpKOSs?=
 =?us-ascii?Q?oq1ZcGDjtwO6sfTLRsRFnV4arfWY3HMMHcYJIAKXVgFX8D/99ZuUGC/bzsaf?=
 =?us-ascii?Q?ivcDhpviELjwDVhrjpwJnQ5n50k+8ISxA2PUfMiSs7SJBkZ1L0/Xq/90u6C2?=
 =?us-ascii?Q?BHb07CG6ILrOgsbs7i308ft9hhQHOvTP8XvjeCn1ZHZiO93zOsTfYOb1QXp2?=
 =?us-ascii?Q?qTvSze9Nb2PlBFHk+HBCy6iuZXJPRU1yRMCltp9/1RjvdEt27iFum9CnmpXH?=
 =?us-ascii?Q?lMkPiODDGmU/8jKZxYN1/M6ylRD5+MdMcB3XbMhbWCfnwb7bKsRoPDb7tRLS?=
 =?us-ascii?Q?PMIGWe0+WCQdTiLo36YVK6zjdA30OVoWlttNH9jbu2Uh1wlejlAYxdbs6GX+?=
 =?us-ascii?Q?BNwt7emC6ocuPuyVnyoHxAmMYiwDp+nqBozIIFe9XAE+qYo4Df3w1Rmi4jk6?=
 =?us-ascii?Q?FX1FZQHpJ8s8RDZVpGu4yZ+u5MfX0mAiUPw8354aPXLBpvtoBYOkZamuq3gL?=
 =?us-ascii?Q?CxxAndg65s6nGKH1Y2c4ACUm8OB0Rv5iRc7R5Ztt8MkEZbYps/QdQdi5nuhp?=
 =?us-ascii?Q?gItFhqs4MSVMHuA3z5viC+MgHattLLuXiJXJ8FPaUgAgneZeow/RteG+CJJd?=
 =?us-ascii?Q?7/Z1JiLDvWrChcO5o+5pVi0fJk3YgaKQJZaXJRn3N9cAvkdZVcUuQMdFtuFv?=
 =?us-ascii?Q?cP3C1GUvadeex0m1NQSS8NQwWXT7b4KJK+zPv5aUGJmzFMSh2skjlAofKDAd?=
 =?us-ascii?Q?XxeLJ33r71z+PbclExWu79/+M0YZiHR5s3FvK7uG60OI77PC8sYqdYj0QSvv?=
 =?us-ascii?Q?wA6n3woko8oU0useM+/QOFWt7/MhlwS3C1zNYOjFQ7vqL1/KNjuUN13c+wsa?=
 =?us-ascii?Q?dkah/OfihRhhcwBXGfmwsX4dhiD0yuQX+Ug0TrUcHH03pB0OMCSNZcXau/pW?=
 =?us-ascii?Q?d6jQI2O6jS5KiO0Ech0b6kberid2FalxxT/DgYNZMqLM0gt+7acLhoYjAbTc?=
 =?us-ascii?Q?9ZZwQrYaTFeLNVrQ2YVnYzUrTvbHpXE3ojV2fO++LXBpsbAp/IA/Zg6x6hzP?=
 =?us-ascii?Q?OY0oMpDyBgBcT5GH9hG95Vv2Ryu0ZH1VoujzDgAOfRfAPtbOHmIYb3Fwfxkb?=
 =?us-ascii?Q?EFO/SNmz4IDrEEQeG+g2uKJelMLLWr8W4i2BiwmrRha4AzIizenzn5MU+awL?=
 =?us-ascii?Q?+D5EWzy396irxIg9PML70bZ4a5izEB6kCwMqvZu51Q/YQvABrOq2fGD+58gJ?=
 =?us-ascii?Q?6CzRKFpJfMThXWpalnioMCmUQVIyfdo8MsNtrBa/ESqOi0yb+X9K7IGY9HC2?=
 =?us-ascii?Q?7fTVQZQs1cRZ0aihKyFdvnIxbDT8avYFrXWo7L/um4H+8XuuYSmpOjotA/Lg?=
 =?us-ascii?Q?1xIGbLMrOkM+5dIaoOVlwM6xWJmPROq3KnlpysFBbyqsakCekJN17ubkB7w7?=
 =?us-ascii?Q?ypqTDWmCKOtyAWe/ghi2wIwrr0AKLBgLQVHiDtVu3GdK4YhQHQBe4GVsrbwL?=
 =?us-ascii?Q?fA=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: 7c090394-6c0c-47bc-091c-08dc7e42fc61
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 May 2024 11:48:54.6334
 (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: o1B3QJYS3NSBASYtWpStprIt9cvT9vioPRtaDKH9UjaOqh1PZBPw7qIwvIQGsMascAGjf1d296hx55vE9z3gc9HHo67fik48FQdF3F3sitA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR11MB7798
X-OriginatorOrg: intel.com


> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Sent: Monday, May 13, 2024 4:49 PM
> To: D, Lakshmi Sowjanya <lakshmi.sowjanya.d@intel.com>
> Cc: tglx@linutronix.de; jstultz@google.com; giometti@enneenne.com;
> corbet@lwn.net; linux-kernel@vger.kernel.org; x86@kernel.org;
> netdev@vger.kernel.org; linux-doc@vger.kernel.org; intel-wired-
> lan@lists.osuosl.org; Dong, Eddie <eddie.dong@intel.com>; Hall, Christoph=
er
> 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>
> Subject: Re: [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driv=
er
>=20
> On Mon, May 13, 2024 at 04:08:11PM +0530, lakshmi.sowjanya.d@intel.com
> wrote:
> > From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
> >
> > The Intel Timed IO PPS generator driver outputs a PPS signal using
> > dedicated hardware that is more accurate than software actuated PPS.
> > The Timed IO hardware generates output events using the ART timer.
> > The ART timer period varies based on platform type, but is less than
> > 100 nanoseconds for all current platforms. Timed IO output accuracy is
> > within 1 ART period.
> >
> > PPS output is enabled by writing '1' the 'enable' sysfs attribute. The
> > driver uses hrtimers to schedule a wake-up 10 ms before each event
> > (edge) target time. At wakeup, the driver converts the target time in
> > terms of CLOCK_REALTIME to ART trigger time and writes this to the
> > Timed IO hardware. The Timed IO hardware generates an event precisely
> > at the requested system time without software involvement.
>=20
> ...
>=20
> > +static ssize_t enable_store(struct device *dev, struct device_attribut=
e
> *attr, const char *buf,
> > +			    size_t count)
> > +{
> > +	struct pps_tio *tio =3D dev_get_drvdata(dev);
> > +	bool enable;
> > +	int err;
>=20
> (1)
>=20
> > +	err =3D kstrtobool(buf, &enable);
> > +	if (err)
> > +		return err;
> > +
> > +	guard(spinlock_irqsave)(&tio->lock);
> > +	if (enable && !tio->enabled) {
>=20
> > +		if (!timekeeping_clocksource_has_base(CSID_X86_ART)) {
> > +			dev_err(tio->dev, "PPS cannot be started as clock is
> not related
> > +to ART");
>=20
> Why not simply dev_err(dev, ...)?
>=20
> > +			return -EPERM;
> > +		}
>=20
> I'm wondering if we can move this check to (1) above.
> Because currently it's a good question if we are able to stop PPS which w=
as
> run by somebody else without this check done.

Do you mean can someone stop the signal without this check?=20
Yes, this check is not required to stop.  So, I feel it need not be moved t=
o (1).

Please, correct me if my understanding is wrong.

>=20
> I.o.w. this sounds too weird to me and reading the code doesn't give any =
hint
> if it's even possible. And if it is, are we supposed to touch that since =
it was
> definitely *not* us who ran it.

Yes, we are not restricting on who can stop/start the signal.=20

>=20
> > +		pps_tio_direction_output(tio);
> > +		hrtimer_start(&tio->timer, first_event(tio),
> HRTIMER_MODE_ABS);
> > +		tio->enabled =3D true;
> > +	} else if (!enable && tio->enabled) {
> > +		hrtimer_cancel(&tio->timer);
> > +		pps_tio_disable(tio);
> > +		tio->enabled =3D false;
> > +	}
> > +	return count;
> > +}
>=20
> ...
>=20
> > +static int pps_tio_probe(struct platform_device *pdev) {
>=20
> 	struct device *dev =3D &pdev->dev;
>=20
> > +	struct pps_tio *tio;
> > +
> > +	if (!(cpu_feature_enabled(X86_FEATURE_TSC_KNOWN_FREQ) &&
> > +	      cpu_feature_enabled(X86_FEATURE_ART))) {
> > +		dev_warn(&pdev->dev, "TSC/ART is not enabled");
>=20
> 		dev_warn(dev, "TSC/ART is not enabled");
>=20
> > +		return -ENODEV;
> > +	}
> > +
> > +	tio =3D devm_kzalloc(&pdev->dev, sizeof(*tio), GFP_KERNEL);
>=20
> 	tio =3D devm_kzalloc(dev, sizeof(*tio), GFP_KERNEL);
>=20
>=20
> > +	if (!tio)
> > +		return -ENOMEM;
> > +
> > +	tio->dev =3D &pdev->dev;
>=20
> 	tio->dev =3D dev;
>=20
> > +	tio->base =3D devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(tio->base))
> > +		return PTR_ERR(tio->base);
>=20
> > +	pps_tio_disable(tio);
>=20
> This...
>=20
> > +	hrtimer_init(&tio->timer, CLOCK_REALTIME, HRTIMER_MODE_ABS);
> > +	tio->timer.function =3D hrtimer_callback;
> > +	spin_lock_init(&tio->lock);
>=20
> > +	tio->enabled =3D false;
>=20
> ...and this should go together, which makes me look at the enabled flag o=
ver
> the code and it seems there are a few places where you missed to sync it
> with the reality.
>=20
> I would think of something like this:
>=20
> 	pps_tio_direction_output() =3D=3D> true
> 	pps_tio_disable(tio) =3D=3D> false
>=20
> where "=3D=3D> X" means assignment of enabled flag.
>=20
> And perhaps this:
>=20
> 	tio->enabled =3D pps_generate_next_pulse(tio, expires +
> SAFE_TIME_NS);
> 	if (!tio->enabled)
> 		...
>=20
> But the above is just thinking out loudly, you may find the better
> approach(es).

Yeah, makes sense.

Will add enable counterpart.
Will update tio->enabled in disable and enable functions.

>=20
> > +	platform_set_drvdata(pdev, tio);
> > +
> > +	return 0;
> > +}
>=20
> --
> With Best Regards,
> Andy Shevchenko
>=20

Regards,
Lakshmi Sowjanya