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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CY8PR11MB7364D1C85099E4337408EBAFC4F02@CY8PR11MB7364.namprd11.prod.outlook.com>
Date: Mon, 27 May 2024 11:48:54 +0000
From: "D, Lakshmi Sowjanya" <lakshmi.sowjanya.d@...el.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC: "tglx@...utronix.de" <tglx@...utronix.de>, "jstultz@...gle.com"
	<jstultz@...gle.com>, "giometti@...eenne.com" <giometti@...eenne.com>,
	"corbet@....net" <corbet@....net>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>, "Dong,
 Eddie" <eddie.dong@...el.com>, "Hall, Christopher S"
	<christopher.s.hall@...el.com>, "Brandeburg, Jesse"
	<jesse.brandeburg@...el.com>, "davem@...emloft.net" <davem@...emloft.net>,
	"alexandre.torgue@...s.st.com" <alexandre.torgue@...s.st.com>,
	"joabreu@...opsys.com" <joabreu@...opsys.com>, "mcoquelin.stm32@...il.com"
	<mcoquelin.stm32@...il.com>, "perex@...ex.cz" <perex@...ex.cz>,
	"linux-sound@...r.kernel.org" <linux-sound@...r.kernel.org>, "Nguyen, Anthony
 L" <anthony.l.nguyen@...el.com>, "peter.hilber@...nsynergy.com"
	<peter.hilber@...nsynergy.com>, "N, Pandith" <pandith.n@...el.com>, "Mohan,
 Subramanian" <subramanian.mohan@...el.com>, "T R, Thejesh Reddy"
	<thejesh.reddy.t.r@...el.com>
Subject: RE: [PATCH v8 10/12] pps: generators: Add PPS Generator TIO Driver



> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> Sent: Monday, May 13, 2024 4:49 PM
> To: D, Lakshmi Sowjanya <lakshmi.sowjanya.d@...el.com>
> Cc: tglx@...utronix.de; jstultz@...gle.com; giometti@...eenne.com;
> corbet@....net; linux-kernel@...r.kernel.org; x86@...nel.org;
> netdev@...r.kernel.org; linux-doc@...r.kernel.org; intel-wired-
> lan@...ts.osuosl.org; Dong, Eddie <eddie.dong@...el.com>; Hall, Christopher
> S <christopher.s.hall@...el.com>; Brandeburg, Jesse
> <jesse.brandeburg@...el.com>; davem@...emloft.net;
> alexandre.torgue@...s.st.com; joabreu@...opsys.com;
> mcoquelin.stm32@...il.com; perex@...ex.cz; linux-
> sound@...r.kernel.org; Nguyen, Anthony L <anthony.l.nguyen@...el.com>;
> peter.hilber@...nsynergy.com; N, Pandith <pandith.n@...el.com>; Mohan,
> Subramanian <subramanian.mohan@...el.com>; T R, Thejesh Reddy
> <thejesh.reddy.t.r@...el.com>
> 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@...el.com
> wrote:
> > From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@...el.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.
> 
> ...
> 
> > +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.

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. 

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

Yeah, makes sense.

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

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

Regards,
Lakshmi Sowjanya