Re: [PATCHv6 15/16] pps: add parallel port PPS client

[Alexander Gordeev]

Ð Fri, 17 Dec 2010 16:17:56 -0800
Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> ÐÐÑÐÑ:

> On Fri, 17 Dec 2010 22:54:39 +0300
> Alexander Gordeev <lasaine@xxxxxxxxxxxxx> wrote:
> 
> > Add parallel port PPS client. It uses a standard method for capturing
> > timestamps for assert edge transitions: getting a timestamp soon after
> > an interrupt has happened. This is not a very precise source of time
> > information due to interrupt handling delays. However, timestamps for
> > clear edge transitions are much more precise because the interrupt
> > handler continuously polls hardware port until the transition is done.
> > Hardware port operations require only about 1us so the maximum error
> > should not exceed this value. This was my primary goal when developing
> > this client.
> > Clear edge capture could be disabled using clear_wait parameter.
> > 
> > ...
> >
> > +/* parport interrupt handler */
> > +static void parport_irq(void *handle)
> > +{
> > +	struct pps_event_time ts_assert, ts_clear;
> > +	struct pps_client_pp *dev = handle;
> > +	struct parport *port = dev->pardev->port;
> > +	unsigned int i;
> > +	unsigned long flags;
> > +
> > +	/* first of all we get the time stamp... */
> > +	pps_get_ts(&ts_assert);
> > +
> > +	if (dev->cw == 0)
> > +		/* clear edge capture disabled */
> > +		goto out_assert;
> > +
> > +	/* try capture the clear edge */
> > +	local_irq_save(flags);
> > +	/* check the signal (no signal means the pulse is lost this time) */
> > +	if (!signal_is_set(port)) {
> > +		local_irq_restore(flags);
> > +		dev_err(dev->pps->dev, "lost the signal\n");
> > +		goto out_assert;
> > +	}
> > +
> > +	/* poll the port until the signal is unset */
> > +	for (i = dev->cw; i; i--)
> > +		if (!signal_is_set(port)) {
> > +			pps_get_ts(&ts_clear);
> > +			local_irq_restore(flags);
> > +			dev->cw_err = 0;
> > +			goto out_both;
> > +		}
> > +	local_irq_restore(flags);
> 
> Why is this function paying around with local_irq_save()?  It's unusual
> and looks buggy because local_irq_save() doesn't stop other CPUs from
> taking an interrupt and coming in and playing with the "protected" data.

The idea is to prevent other interrupts on the same processor to
introduce uncontrolled time lags here. Reading from IO port is known to
take approximately 1us while other interrupt handlers can probably take
much more. So I poll the port with locally disabled interrupts to
ensure that the maximum lag here is 1us. All my experiments show that
it is in fact very precise this way given that input signal is precise.

> I have a feeling I asked this before, but you didn't add a comment
> explaining it, so I went and asked it again!  Cause, effect.

No, you didn't ask this before, I think.

> > +static void parport_attach(struct parport *port)
> > +{
> > +	struct pps_client_pp *device;
> > +	struct pps_source_info info = {
> > +		.name		= KBUILD_MODNAME,
> > +		.path		= "",
> > +		.mode		= PPS_CAPTUREBOTH | \
> > +				  PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
> > +				  PPS_ECHOASSERT | PPS_ECHOCLEAR | \
> > +				  PPS_CANWAIT | PPS_TSFMT_TSPEC,
> > +		.echo		= pps_echo,
> > +		.owner		= THIS_MODULE,
> > +		.dev		= NULL
> > +	};
> > +
> > +	device = kzalloc(sizeof(struct pps_client_pp), GFP_KERNEL);
> > +	if (!device) {
> > +		pr_err("memory allocation failed, not attaching\n");
> > +		return;
> 
> And the void-returning parport_driver.attach() still sucks.

Hmm. Do you want me to rewrite the parport subsystem? There are lots of
problems in it now:
 * not part of LDM
 * controlled through /proc
 * detach is called for every port, not only the on being attached to
 * ...

Sure, I think it should be rewritten quite a bit but I cannot do that
right now, sorry.

> > +	}
> > +
> > +	device->pardev = parport_register_device(port, KBUILD_MODNAME,
> > +			NULL, NULL, parport_irq, 0, device);
> > +	if (!device->pardev) {
> > +		pr_err("couldn't register with %s\n", port->name);
> > +		goto err_free;
> > +	}
> > +
> > +	if (parport_claim_or_block(device->pardev) < 0) {
> > +		pr_err("couldn't claim %s\n", port->name);
> > +		goto err_unregister_dev;
> > +	}
> > +
> > +	device->pps = pps_register_source(&info,
> > +			PPS_CAPTUREBOTH | PPS_OFFSETASSERT | PPS_OFFSETCLEAR);
> > +	if (device->pps == NULL) {
> > +		pr_err("couldn't register PPS source\n");
> > +		goto err_release_dev;
> > +	}
> > +
> > +	device->cw = clear_wait;
> > +
> > +	port->ops->enable_irq(port);
> > +
> > +	pr_info("attached to %s\n", port->name);
> > +
> > +	return;
> > +
> > +err_release_dev:
> > +	parport_release(device->pardev);
> > +err_unregister_dev:
> > +	parport_unregister_device(device->pardev);
> > +err_free:
> > +	kfree(device);
> > +}
> > +
> 


-- 
  Alexander

Attachment: signature.asc
Description: PGP signature