Messages in this thread | | | Date | Sun, 21 Nov 2010 02:23:02 +0300 | From | Alexander Gordeev <> | Subject | Re: [PATCHv4 08/17] pps: add async PPS event handler |
| |
В Sat, 20 Nov 2010 17:08:51 +0100 Rodolfo Giometti <giometti@enneenne.com> пишет:
> On Thu, Nov 18, 2010 at 07:01:01PM +0300, Alexander Gordeev wrote: > > This handler should be called from an IRQ handler. It uses per-device > > workqueue internally. > > Can you please explain to me why do you need this patch? Maybe you can > add a verbose patch's description? :)
Well, it's all about optimizing latencies on rt-preempt kernel: if everything is done in a process context than we can use mutexes (haven't done that yet) and don't disable interrupts, which is better for hard real time. I measured that pps_event with kernel consumer enabled takes 1-2us on my test machine. Not a big deal, of course...
Sorry, I've completely forgotten about an echo function. However quick look to current clients shows that it is only used in pps-ktimer.c for debug printing... Maybe it's not needed at all? I mean, have you ever got any user request for this feature? If yes, it can be removed IMHO since RFC-2783 says that it's optional. I can handle the removal in the next version of the patchset.
If you don't want it I can have this patch on my rt branch only and don't try to push it into mainline.
> > Signed-off-by: Alexander Gordeev <lasaine@lvk.cs.msu.su> > > --- > > drivers/pps/clients/pps-ktimer.c | 2 +- > > drivers/pps/clients/pps-ldisc.c | 2 +- > > drivers/pps/kapi.c | 95 ++++++++++++++++++++++++++++++++++++-- > > drivers/pps/pps.c | 14 +++++- > > include/linux/pps_kernel.h | 12 +++++ > > 5 files changed, 117 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c > > index 2728469..26ed7a2 100644 > > --- a/drivers/pps/clients/pps-ktimer.c > > +++ b/drivers/pps/clients/pps-ktimer.c > > @@ -48,7 +48,7 @@ static void pps_ktimer_event(unsigned long ptr) > > > > dev_info(pps->dev, "PPS event at %lu\n", jiffies); > > > > - pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL); > > + pps_event_irq(pps, &ts, PPS_CAPTUREASSERT, NULL); > > > > mod_timer(&ktimer, jiffies + HZ); > > } > > diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c > > index 30789fa..7006f85 100644 > > --- a/drivers/pps/clients/pps-ldisc.c > > +++ b/drivers/pps/clients/pps-ldisc.c > > @@ -50,7 +50,7 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status, > > /* Now do the PPS event report */ > > pps = (struct pps_device *)tty->disc_data; > > if (pps != NULL) { > > - pps_event(pps, ts, status ? PPS_CAPTUREASSERT : > > + pps_event_irq(pps, ts, status ? PPS_CAPTUREASSERT : > > PPS_CAPTURECLEAR, NULL); > > spin_unlock_irqrestore(&pps_ldisc_lock, flags); > > dev_dbg(pps->dev, "PPS %s at %lu\n", > > diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c > > index f5b2b78..ca3b4f8 100644 > > --- a/drivers/pps/kapi.c > > +++ b/drivers/pps/kapi.c > > @@ -32,9 +32,19 @@ > > #include <linux/slab.h> > > > > /* > > + * Global variables > > + */ > > + > > +/* PPS event workqueue */ > > +struct workqueue_struct *pps_event_workqueue; > > + > > +/* > > * Local functions > > */ > > > > +static void assert_work_func(struct work_struct *work); > > +static void clear_work_func(struct work_struct *work); > > + > > static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset) > > { > > ts->nsec += offset->nsec; > > @@ -108,6 +118,9 @@ struct pps_device *pps_register_source(struct pps_source_info *info, > > init_waitqueue_head(&pps->queue); > > spin_lock_init(&pps->lock); > > > > + INIT_WORK(&pps->assert_work, assert_work_func); > > + INIT_WORK(&pps->clear_work, clear_work_func); > > + > > /* Create the char device */ > > err = pps_register_cdev(pps); > > if (err < 0) { > > @@ -152,11 +165,12 @@ EXPORT_SYMBOL(pps_unregister_source); > > * @event: the event type > > * @data: userdef pointer > > * > > - * This function is used by each PPS client in order to register a new > > - * PPS event into the system (it's usually called inside an IRQ handler). > > + * This function is used by PPS clients in order to register a new > > + * PPS event into the system. It should not be called from an IRQ > > + * handler. Use pps_event_irq instead. > > * > > - * If an echo function is associated with the PPS device it will be called > > - * as: > > + * If an echo function is associated with the PPS device it will be > > + * called as: > > * pps->info.echo(pps, event, data); > > */ > > void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event, > > @@ -226,3 +240,76 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event, > > spin_unlock_irqrestore(&pps->lock, flags); > > } > > EXPORT_SYMBOL(pps_event); > > + > > +/* Async event handlers */ > > + > > +static void assert_work_func(struct work_struct *work) > > +{ > > + struct pps_device *pps = container_of(work, > > + struct pps_device, assert_work); > > + > > + pps_event(pps, &pps->assert_work_ts, PPS_CAPTUREASSERT, > > + pps->assert_work_data); > > +} > > + > > +static void clear_work_func(struct work_struct *work) > > +{ > > + struct pps_device *pps = container_of(work, > > + struct pps_device, clear_work); > > + > > + pps_event(pps, &pps->clear_work_ts, PPS_CAPTURECLEAR, > > + pps->clear_work_data); > > +} > > The RFC-2783 says that (see 3.1 PPS abstraction): > > The API optionally supports an "echo" feature, in which events on > the > incoming PPS signal may be reflected through software, after the > capture of the corresponding timestamp, to an output signal pin. > This feature may be used to discover an upper bound on the actual > delay between the edges of the PPS signal and the capture of the > timestamps; such information may be useful in precise calibration > of > the system. > > The designation of an output pin for the echo signal, and sense and > shape of the output transition, is outside the scope of this > specification, but SHOULD be documented for each implementation. > The > output pin MAY also undergo transitions at other times besides > those > caused by PPS input events. > > By applying this patch the echo function is called inside a work queue > so it depends to the scheduler. I suppose this is not acceptable, > otherwise we should drop the echo function support. > > > +/* pps_event_irq - register a PPS event for deffered handling using > > + * workqueue > > + * > > + * @pps: the PPS device > > + * @ts: the event timestamp > > + * @event: the event type > > + * @data: userdef pointer > > + * > > + * This function is used by PPS clients in order to register a new > > + * PPS event into the system. It should be called from an IRQ handler > > + * only. > > + * > > + * If an echo function is associated with the PPS device it will be > > + * called as: > > + * pps->info.echo(pps, event, data); > > + */ > > The above comment talks about the echo function but you removed it > from the code below...
Well, I meant that it will be called from pps_event(). :) Actually just copied this comment from pps_event() and thought that it's still right somehow. Do you want me to remove this comment?
> > +void pps_event_irq(struct pps_device *pps, struct pps_event_time *ts, > > + int event, void *data) > > +{ > > + /* check event type */ > > + BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0); > > + > > + if (event & PPS_CAPTUREASSERT) { > > + if (work_pending(&pps->assert_work)) { > > + dev_err(pps->dev, "deferred assert edge work haven't" > > + " been handled within a second\n"); > > + /* FIXME: do something more intelligent > > + * then just exit */ > > + } else { > > + /* now we can copy data safely */ > > + pps->assert_work_ts = *ts; > > + pps->assert_work_data = data; > > + > > + queue_work(pps_event_workqueue, &pps->assert_work); > > + } > > + } > > + if (event & PPS_CAPTURECLEAR) { > > + if (work_pending(&pps->clear_work)) { > > + dev_err(pps->dev, "deferred clear edge work haven't" > > + " been handled within a second\n"); > > + /* FIXME: do something more intelligent > > + * then just exit */ > > + } else { > > + /* now we can copy data safely */ > > + pps->clear_work_ts = *ts; > > + pps->clear_work_data = data; > > + > > + queue_work(pps_event_workqueue, &pps->clear_work); > > + } > > + } > > +} > > +EXPORT_SYMBOL(pps_event_irq); > > diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c > > index 79b4455..f642558 100644 > > --- a/drivers/pps/pps.c > > +++ b/drivers/pps/pps.c > > @@ -321,18 +321,26 @@ void pps_unregister_cdev(struct pps_device *pps) > > > > static void __exit pps_exit(void) > > { > > - class_destroy(pps_class); > > unregister_chrdev_region(pps_devt, PPS_MAX_SOURCES); > > + class_destroy(pps_class); > > + destroy_workqueue(pps_event_workqueue); > > } > > > > static int __init pps_init(void) > > { > > int err; > > > > + pps_event_workqueue = create_workqueue("pps"); > > + if (!pps_event_workqueue) { > > + pr_err("failed to create workqueue\n"); > > + return -ENOMEM; > > + } > > + > > pps_class = class_create(THIS_MODULE, "pps"); > > if (!pps_class) { > > pr_err("failed to allocate class\n"); > > - return -ENOMEM; > > + err = -ENOMEM; > > + goto destroy_workqueue; > > } > > pps_class->dev_attrs = pps_attrs; > > > > @@ -350,6 +358,8 @@ static int __init pps_init(void) > > > > remove_class: > > class_destroy(pps_class); > > +destroy_workqueue: > > + destroy_workqueue(pps_event_workqueue); > > > > return err; > > } > > diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h > > index 1aedf50..5af0498 100644 > > --- a/include/linux/pps_kernel.h > > +++ b/include/linux/pps_kernel.h > > @@ -26,6 +26,7 @@ > > #include <linux/cdev.h> > > #include <linux/device.h> > > #include <linux/time.h> > > +#include <linux/workqueue.h> > > > > /* > > * Global defines > > @@ -70,6 +71,13 @@ struct pps_device { > > struct device *dev; > > struct fasync_struct *async_queue; /* fasync method */ > > spinlock_t lock; > > + > > + struct work_struct assert_work; > > + struct work_struct clear_work; > > + struct pps_event_time assert_work_ts; > > + struct pps_event_time clear_work_ts; > > + void *assert_work_data; > > + void *clear_work_data; > > }; > > > > /* > > @@ -78,6 +86,8 @@ struct pps_device { > > > > extern struct device_attribute pps_attrs[]; > > > > +extern struct workqueue_struct *pps_event_workqueue; > > + > > /* > > * Exported functions > > */ > > @@ -89,6 +99,8 @@ extern int pps_register_cdev(struct pps_device *pps); > > extern void pps_unregister_cdev(struct pps_device *pps); > > extern void pps_event(struct pps_device *pps, > > struct pps_event_time *ts, int event, void *data); > > +extern void pps_event_irq(struct pps_device *pps, > > + struct pps_event_time *ts, int event, void *data); > > > > static inline void timespec_to_pps_ktime(struct pps_ktime *kt, > > struct timespec ts) > > -- > > 1.7.2.3 > > > > Ciao, > > Rodolfo >
-- Alexander [unhandled content-type:application/pgp-signature] | |