RE: [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driver

-----Original Message-----
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Sent: Monday, May 13, 2024 4:49 PM
To: D, Lakshmi Sowjanya <redacted>
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 [off-list ref]; Hall, Christopher
S [off-list ref]; Brandeburg, Jesse
[off-list ref]; 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 [off-list ref];
peter.hilber@opensynergy.com; N, Pandith [off-list ref]; Mohan,
Subramanian [off-list ref]; T R, Thejesh Reddy
[off-list ref]
Subject: Re: [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driver

On Mon, May 13, 2024 at 04:08:11PM +0530, lakshmi.sowjanya.d@intel.com
wrote:

quoted

From: Lakshmi Sowjanya D <redacted>

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.

...

quoted

+static ssize_t enable_store(struct device *dev, struct device_attribute

*attr, const char *buf,

quoted

+			    size_t count)
+{
+	struct pps_tio *tio = dev_get_drvdata(dev);
+	bool enable;
+	int err;

(1)

quoted

+	err = kstrtobool(buf, &enable);
+	if (err)
+		return err;
+
+	guard(spinlock_irqsave)(&tio->lock);
+	if (enable && !tio->enabled) {

quoted

+		if (!timekeeping_clocksource_has_base(CSID_X86_ART)) {
+			dev_err(tio->dev, "PPS cannot be started as clock is

not related

quoted

+to ART");

Why not simply dev_err(dev, ...)?

quoted

+			return -EPERM;
+		}

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 was
run by somebody else without this check done.

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

Please, correct me if my understanding is wrong.

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. 

quoted

+		pps_tio_direction_output(tio);
+		hrtimer_start(&tio->timer, first_event(tio),

HRTIMER_MODE_ABS);

quoted

+		tio->enabled = true;
+	} else if (!enable && tio->enabled) {
+		hrtimer_cancel(&tio->timer);
+		pps_tio_disable(tio);
+		tio->enabled = false;
+	}
+	return count;
+}

...

quoted

+static int pps_tio_probe(struct platform_device *pdev) {

	struct device *dev = &pdev->dev;

quoted

+	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");

		dev_warn(dev, "TSC/ART is not enabled");

quoted

+		return -ENODEV;
+	}
+
+	tio = devm_kzalloc(&pdev->dev, sizeof(*tio), GFP_KERNEL);

	tio = devm_kzalloc(dev, sizeof(*tio), GFP_KERNEL);

quoted

+	if (!tio)
+		return -ENOMEM;
+
+	tio->dev = &pdev->dev;

	tio->dev = dev;

quoted

+	tio->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(tio->base))
+		return PTR_ERR(tio->base);

quoted

+	pps_tio_disable(tio);

This...

quoted

+	hrtimer_init(&tio->timer, CLOCK_REALTIME, HRTIMER_MODE_ABS);
+	tio->timer.function = hrtimer_callback;
+	spin_lock_init(&tio->lock);

quoted

+	tio->enabled = false;

...and this should go together, which makes me look at the enabled flag over
the code and it seems there are a few places where you missed to sync it
with the reality.

I would think of something like this:

	pps_tio_direction_output() ==> true
	pps_tio_disable(tio) ==> false

where "==> X" means assignment of enabled flag.

And perhaps this:

	tio->enabled = pps_generate_next_pulse(tio, expires +
SAFE_TIME_NS);
	if (!tio->enabled)
		...

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.

quoted

+	platform_set_drvdata(pdev, tio);
+
+	return 0;
+}

--
With Best Regards,
Andy Shevchenko

Regards,
Lakshmi Sowjanya