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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Mon, May 13, 2024 at 04:08:11PM +0530, lakshmi.sowjanya.d@xxxxxxxxx wrote:
> From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@xxxxxxxxx>
> 
> 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.

...

> +static ssize_t enable_store(struct device *dev, struct device_attribute *attr, const char *buf,
> +			    size_t count)
> +{
> +	struct pps_tio *tio = dev_get_drvdata(dev);
> +	bool enable;
> +	int err;

(1)

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

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

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

> +			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.

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.

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

...

> +static int pps_tio_probe(struct platform_device *pdev)
> +{

	struct device *dev = &pdev->dev;

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

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

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


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

	tio->dev = dev;

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

> +	pps_tio_disable(tio);

This...

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

> +	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).

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

-- 
With Best Regards,
Andy Shevchenko