[LinuxPPS] [PATCH 02/11] Streamline interrupt handling; get rid of a lock and idr_find()
Rodolfo Giometti
giometti at enneenne.com
Mon Feb 9 22:51:11 CET 2009
On Fri, Feb 06, 2009 at 08:28:25AM -0500, George Spelvin wrote:
> This speeds up some code by passing and returninf a struct pps_device *, rather than a
> numeric ID that has to be looked up.
>
> ---
> Documentation/pps/pps.txt | 8 ++-
> drivers/pps/clients/ktimer.c | 19 ++++----
> drivers/pps/clients/pps-ldisc.c | 26 ++++++-----
> drivers/pps/kapi.c | 91 +++++++++++++++++++--------------------
> drivers/pps/pps.c | 48 +++++++++++----------
> drivers/pps/sysfs.c | 14 ++++--
> include/linux/pps.h | 11 +++--
> 7 files changed, 115 insertions(+), 102 deletions(-)
>
> diff --git a/Documentation/pps/pps.txt b/Documentation/pps/pps.txt
> index 125f4ab..8470a69 100644
> --- a/Documentation/pps/pps.txt
> +++ b/Documentation/pps/pps.txt
> @@ -85,12 +85,12 @@ pps_source_info_s as follows:
> and then calling the function pps_register_source() in your
> intialization routine as follows:
>
> - source = pps_register_source(&pps_ktimer_info,
> + pps = pps_register_source(&pps_ktimer_info,
> PPS_CAPTUREASSERT | PPS_OFFSETASSERT);
>
> The pps_register_source() prototype is:
>
> - int pps_register_source(struct pps_source_info_s *info, int default_params)
> + struct pps_source *pps_register_source(struct pps_source_info_s *info, int default_params)
>
> where "info" is a pointer to a structure that describes a particular
> PPS source, "default_params" tells the system what the initial default
> @@ -98,11 +98,13 @@ parameters for the device should be (it is obvious that these parameters
> must be a subset of ones defined in the struct
> pps_source_info_s which describe the capabilities of the driver).
>
> +The return value is either a pointer to a pps struct or an ERR_PTR.
> +
> Once you have registered a new PPS source into the system you can
> signal an assert event (for example in the interrupt handler routine)
> just using:
>
> - pps_event(source, &ts, PPS_CAPTUREASSERT, ptr)
> + pps_event(pps, &ts, PPS_CAPTUREASSERT, ptr)
>
> where "ts" is the event's timestamp.
>
> diff --git a/drivers/pps/clients/ktimer.c b/drivers/pps/clients/ktimer.c
> index 259baa7..64719e0 100644
> --- a/drivers/pps/clients/ktimer.c
> +++ b/drivers/pps/clients/ktimer.c
> @@ -25,6 +25,7 @@
> #include <linux/init.h>
> #include <linux/time.h>
> #include <linux/timer.h>
> +#include <linux/err.h>
>
> #include <linux/pps.h>
>
> @@ -32,7 +33,7 @@
> * Global variables
> */
>
> -static int source;
> +static struct pps_device *source;
> static struct timer_list ktimer;
>
> /*
> @@ -62,12 +63,12 @@ static void pps_ktimer_event(unsigned long ptr)
> * The echo function
> */
>
> -static void pps_ktimer_echo(int source, int event, void *data)
> +static void pps_ktimer_echo(int id, int event, void *data)
> {
> pr_info("echo %s %s for source %d\n",
> event & PPS_CAPTUREASSERT ? "assert" : "",
> event & PPS_CAPTURECLEAR ? "clear" : "",
> - source);
> + id);
> }
>
> /*
> @@ -98,20 +99,20 @@ static void __exit pps_ktimer_exit(void)
>
> static int __init pps_ktimer_init(void)
> {
> - int ret;
> + struct pps_device *pps;
>
> - ret = pps_register_source(&pps_ktimer_info,
> + pps = pps_register_source(&pps_ktimer_info,
> PPS_CAPTUREASSERT | PPS_OFFSETASSERT);
> - if (ret < 0) {
> + if (IS_ERR(pps)) {
> printk(KERN_ERR "cannot register ktimer source\n");
> - return ret;
> + return PTR_ERR(pps);
> }
> - source = ret;
> + source = pps;
>
> setup_timer(&ktimer, pps_ktimer_event, 0);
> mod_timer(&ktimer, jiffies + HZ);
>
> - pr_info("ktimer PPS source registered at %d\n", source);
> + pr_info("ktimer PPS source registered at %d\n", pps->id);
>
> return 0;
> }
> diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
> index 0406f40..bc8b56a 100644
> --- a/drivers/pps/clients/pps-ldisc.c
> +++ b/drivers/pps/clients/pps-ldisc.c
> @@ -29,7 +29,7 @@
> static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
> struct timespec *ts)
> {
> - long id = (long) tty->disc_data;
> + struct pps_device *pps = tty->disc_data;
> struct timespec __ts;
> struct pps_ktime pps_ts;
>
> @@ -42,11 +42,11 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
> pps_ts.nsec = ts->tv_nsec;
>
> /* Now do the PPS event report */
> - pps_event(id, &pps_ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR,
> + pps_event(pps, &pps_ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR,
> NULL);
>
> pr_debug("PPS %s at %lu on source #%d\n",
> - status ? "assert" : "clear", jiffies, (int) id);
> + status ? "assert" : "clear", jiffies, pps->id);
> }
>
> static int pps_tty_open(struct tty_struct *tty)
> @@ -54,7 +54,8 @@ static int pps_tty_open(struct tty_struct *tty)
> struct pps_source_info info;
> struct tty_driver *drv = tty->driver;
> int index = tty->index + drv->name_base;
> - long ret;
> + struct pps_device *pps;
> + int ret;
>
> info.owner = THIS_MODULE;
> info.dev = NULL;
> @@ -64,30 +65,31 @@ static int pps_tty_open(struct tty_struct *tty)
> PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
> PPS_CANWAIT | PPS_TSFMT_TSPEC;
>
> - ret = pps_register_source(&info, PPS_CAPTUREBOTH | \
> + pps = pps_register_source(&info, PPS_CAPTUREBOTH | \
> PPS_OFFSETASSERT | PPS_OFFSETCLEAR);
> - if (ret < 0) {
> + if (IS_ERR(pps)) {
> pr_err("cannot register PPS source \"%s\"\n", info.path);
> - return ret;
> + return PTR_ERR(pps);
> }
> - tty->disc_data = (void *) ret;
> + tty->disc_data = pps;
>
> /* Should open N_TTY ldisc too */
> ret = n_tty_open(tty);
> if (ret < 0)
> - pps_unregister_source((long) tty->disc_data);
> + pps_unregister_source(tty->disc_data);
>
> - pr_info("PPS source #%d \"%s\" added\n", (int) ret, info.path);
> + pr_info("PPS source #%d \"%s\" added\n", pps->id, info.path);
>
> return 0;
> }
>
> static void pps_tty_close(struct tty_struct *tty)
> {
> - long id = (long) tty->disc_data;
> + struct pps_device *pps = tty->disc_data;
> + int id = pps->id;
>
> - pps_unregister_source(id);
> n_tty_close(tty);
> + pps_unregister_source(pps);
Why?
>
> pr_info("PPS source #%d removed\n", (int) id);
> }
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index 917b379..c1129f5 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -124,7 +124,8 @@ exit:
> * The function returns, in case of success, the PPS source ID.
> */
>
> -int pps_register_source(struct pps_source_info *info, int default_params)
> +struct pps_device *
> +pps_register_source(struct pps_source_info *info, int default_params)
> {
> struct pps_device *pps;
> int id;
> @@ -198,7 +199,7 @@ int pps_register_source(struct pps_source_info *info, int default_params)
>
> pr_info("new PPS source %s at ID %d\n", info->name, id);
>
> - return id;
> + return pps;
>
> free_idr:
> spin_lock_irq(&pps_idr_lock);
> @@ -211,7 +212,7 @@ kfree_pps:
> pps_register_source_exit:
> printk(KERN_ERR "pps: %s: unable to register source\n", info->name);
>
> - return err;
> + return ERR_PTR(err);
> }
> EXPORT_SYMBOL(pps_register_source);
>
> @@ -222,27 +223,19 @@ EXPORT_SYMBOL(pps_register_source);
> * the system.
> */
>
> -void pps_unregister_source(int source)
> +void pps_unregister_source(struct pps_device *pps)
> {
> - struct pps_device *pps;
> -
> - spin_lock_irq(&pps_idr_lock);
> - pps = idr_find(&pps_idr, source);
> -
> if (!pps) {
> BUG();
> - spin_unlock_irq(&pps_idr_lock);
> return;
> }
> - spin_unlock_irq(&pps_idr_lock);
> -
> pps_unregister_cdev(pps);
> pps_put_source(pps);
> }
> EXPORT_SYMBOL(pps_unregister_source);
I don't think this is correct, see below.
>
> /* pps_event - register a PPS event into the system
> - * @source: the PPS source ID
> + * @pps: the pps_device structure
> * @ts: the event timestamp
> * @event: the event type
> * @data: userdef pointer
> @@ -252,66 +245,72 @@ EXPORT_SYMBOL(pps_unregister_source);
> *
> * If an echo function is associated with the PPS source it will be called
> * as:
> - * pps->info.echo(source, event, data);
> + * pps->info.echo(pps->id, event, data);
> + *
> + * NOTE: This function does no locking. The caller must ensure that
> + * a) calls are serialized, and
> + * b) do not occur after pps_deregister_source is called.
> + * For current PPS clients, this is taken care of by:
> + * serial: a) struct uart_port's lock field, b) ldisc refcount
> + * ktimer: a) struct tvec_base's lock, b) del_timer_sync()
> */
>
> -void pps_event(int source, struct pps_ktime *ts, int event, void *data)
> +void
> +pps_event(struct pps_device *pps, struct pps_ktime *ts, int event, void *data)
> {
> - struct pps_device *pps;
> - unsigned long flags;
> + int mode;
> + u32 seq;
> +
> + if (!pps)
> + return;
I don't understand your locking scheme here, what about if a
pps_unregister_source() is executed after this point?
>
> if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
> printk(KERN_ERR "pps: unknown event (%x) for source %d\n",
> - event, source);
> + event, pps->id);
> return;
> }
> -
> - pps = pps_get_source(source);
Why are you removing pps_get_source() here and not pps_put_source()
into pps_unregister_source()?
> - if (!pps)
> - return;
> + event &= PPS_CAPTUREASSERT | PPS_CAPTURECLEAR;
>
> pr_debug("PPS event on source %d at %llu.%06u\n",
> pps->id, (unsigned long long) ts->sec, ts->nsec);
>
> - spin_lock_irqsave(&pps->lock, flags);
> -
> - /* Must call the echo function? */
> - if ((pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)))
> - pps->info.echo(source, event, data);
> -
> /* Check the event */
> - pps->current_mode = pps->params.mode;
> + mode = pps->params.mode;
> if (event & PPS_CAPTUREASSERT) {
> + if (mode & PPS_ECHOASSERT)
> + pps->info.echo(pps->id, PPS_CAPTUREASSERT, data);
Keep in mind that RFC says that echo function should be called as soon
as possible.
> /* We have to add an offset? */
> - if (pps->params.mode & PPS_OFFSETASSERT)
> + if (mode & PPS_OFFSETASSERT)
> pps_add_offset(ts, &pps->params.assert_off_tu);
>
> /* Save the time stamp */
> - pps->assert_tu = *ts;
> - pps->assert_sequence++;
> + seq = pps->assert_sequence + 1;
> + pps->assert_tu[seq % 4] = *ts;
> + wmb();
> + pps->assert_sequence = seq;
Why are you doing like this? If not related with the patch's title,
please split the patch.
> pr_debug("capture assert seq #%u for source %d\n",
> - pps->assert_sequence, source);
> + seq, pps->id);
> }
> if (event & PPS_CAPTURECLEAR) {
> + if (mode & PPS_ECHOCLEAR)
> + pps->info.echo(pps->id, PPS_CAPTURECLEAR, data);
Ditto.
> /* We have to add an offset? */
> - if (pps->params.mode & PPS_OFFSETCLEAR)
> + if (mode & PPS_OFFSETCLEAR)
> pps_add_offset(ts, &pps->params.clear_off_tu);
>
> /* Save the time stamp */
> - pps->clear_tu = *ts;
> - pps->clear_sequence++;
> + seq = pps->clear_sequence + 1;
> + pps->clear_tu[seq % 4] = *ts;
> + wmb();
> + pps->clear_sequence = seq;
Ditto.
> pr_debug("capture clear seq #%u for source %d\n",
> - pps->clear_sequence, source);
> + seq, pps->id);
> }
>
> - pps->go = ~0;
> - wake_up_interruptible(&pps->queue);
> -
> - kill_fasync(&pps->async_queue, SIGIO, POLL_IN);
> -
> - spin_unlock_irqrestore(&pps->lock, flags);
> -
> - /* Now we can release the PPS source for (possible) deregistration */
> - pps_put_source(pps);
> + if (event & mode & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) {
> + pps->go = ~0;
> + wake_up_interruptible(&pps->queue);
> + kill_fasync(&pps->async_queue, SIGIO, POLL_IN);
> + }
> }
> EXPORT_SYMBOL(pps_event);
> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> index aeaa52e..9f6fa02 100644
> --- a/drivers/pps/pps.c
> +++ b/drivers/pps/pps.c
> @@ -65,6 +65,7 @@ static long pps_cdev_ioctl(struct file *file,
> unsigned long ticks;
> void __user *uarg = (void __user *) arg;
> int __user *iuarg = (int __user *) arg;
> + u32 seq1, seq2;
> int err;
>
> switch (cmd) {
> @@ -78,8 +79,9 @@ static long pps_cdev_ioctl(struct file *file,
> pr_debug("PPS_GETPARAMS: source %d\n", pps->id);
>
> /* Return current parameters */
> - err = copy_to_user(uarg, &pps->params,
> - sizeof(struct pps_kparams));
> + spin_lock(&pps->lock);
> + err = copy_to_user(uarg, &pps->params, sizeof pps->params);
> + spin_unlock(&pps->lock);
Here you are adding a lock, if not related with patch's title, please
split patch.
> if (err)
> return -EFAULT;
>
> @@ -92,7 +94,10 @@ static long pps_cdev_ioctl(struct file *file,
> if (!capable(CAP_SYS_TIME))
> return -EPERM;
>
> - err = copy_from_user(¶ms, uarg, sizeof(struct pps_kparams));
> + /* Sanity checks */
> + if (!uarg)
> + return -EINVAL;
> + err = copy_from_user(¶ms, uarg, sizeof params);
copy_from_user() should already do the necessary checking. Again, this
is not related to patch's title, please, split the patch.
> if (err)
> return -EFAULT;
> if (!(params.mode & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR))) {
> @@ -108,23 +113,21 @@ static long pps_cdev_ioctl(struct file *file,
> return -EINVAL;
> }
>
> - spin_lock_irq(&pps->lock);
> -
> - /* Save the new parameters */
> - pps->params = params;
> -
> /* Restore the read only parameters */
> if ((params.mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
> /* section 3.3 of RFC 2783 interpreted */
> pr_debug("time format unspecified (%x)\n",
> params.mode);
> - pps->params.mode |= PPS_TSFMT_TSPEC;
> + params.mode |= PPS_TSFMT_TSPEC;
> }
> if (pps->info.mode & PPS_CANWAIT)
> - pps->params.mode |= PPS_CANWAIT;
> - pps->params.api_version = PPS_API_VERS;
> + params.mode |= PPS_CANWAIT;
> + params.api_version = PPS_API_VERS;
>
> - spin_unlock_irq(&pps->lock);
> + /* Save the new parameters */
> + spin_lock(&pps->lock);
> + pps->params = params;
> + spin_unlock(&pps->lock);
Ditto.
> break;
>
> @@ -140,7 +143,9 @@ static long pps_cdev_ioctl(struct file *file,
> case PPS_FETCH:
> pr_debug("PPS_FETCH: source %d\n", pps->id);
>
> - err = copy_from_user(&fdata, uarg, sizeof(struct pps_fdata));
> + if (!uarg)
> + return -EINVAL;
> + err = copy_from_user(&fdata, uarg, sizeof fdata);
Ditto.
> if (err)
> return -EFAULT;
>
> @@ -171,17 +176,14 @@ static long pps_cdev_ioctl(struct file *file,
> }
>
> /* Return the fetched timestamp */
> - spin_lock_irq(&pps->lock);
> -
> - fdata.info.assert_sequence = pps->assert_sequence;
> - fdata.info.clear_sequence = pps->clear_sequence;
> - fdata.info.assert_tu = pps->assert_tu;
> - fdata.info.clear_tu = pps->clear_tu;
> + fdata.info.assert_sequence = seq1 = pps->assert_sequence;
> + fdata.info.clear_sequence = seq2 = pps->clear_sequence;
> + read_barrier_depends();
> + fdata.info.assert_tu = pps->assert_tu[seq1 % 4];
> + fdata.info.clear_tu = pps->clear_tu[seq2 % 4];
Not related with patch's title, please split the patch.
> fdata.info.current_mode = pps->current_mode;
>
> - spin_unlock_irq(&pps->lock);
> -
> - err = copy_to_user(uarg, &fdata, sizeof(struct pps_fdata));
> + err = copy_to_user(uarg, &fdata, sizeof fdata);
> if (err)
> return -EFAULT;
>
> @@ -201,7 +203,7 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
> struct pps_device, cdev);
> int found;
>
> - found = pps_get_source(pps->id) != 0;
> + found = pps_get_source(pps->id) != 0; /* Bump refcount */
Ditto.
> if (!found)
> return -ENODEV;
>
> diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c
> index b4cc850..84a5634 100644
> --- a/drivers/pps/sysfs.c
> +++ b/drivers/pps/sysfs.c
> @@ -33,26 +33,32 @@ static ssize_t pps_show_assert(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct pps_device *pps = dev_get_drvdata(dev);
> + u32 seq;
>
> if (!(pps->info.mode & PPS_CAPTUREASSERT))
> return 0;
>
> + seq = pps->assert_sequence;
> + rmb();
> return sprintf(buf, "%lld.%09d#%d\n",
> - (long long) pps->assert_tu.sec, pps->assert_tu.nsec,
> - pps->assert_sequence);
> + (long long) pps->assert_tu[seq % 4].sec,
> + pps->assert_tu[seq % 4].nsec, (unsigned)seq);
> }
>
> static ssize_t pps_show_clear(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> struct pps_device *pps = dev_get_drvdata(dev);
> + u32 seq;
>
> if (!(pps->info.mode & PPS_CAPTURECLEAR))
> return 0;
>
> + seq = pps->clear_sequence;
> + rmb();
> return sprintf(buf, "%lld.%09d#%d\n",
> - (long long) pps->clear_tu.sec, pps->clear_tu.nsec,
> - pps->clear_sequence);
> + (long long) pps->clear_tu[seq % 4].sec,
> + pps->clear_tu[seq % 4].nsec, (unsigned)seq);
> }
>
> static ssize_t pps_show_mode(struct device *dev,
> diff --git a/include/linux/pps.h b/include/linux/pps.h
> index d7719b3..2609708 100644
> --- a/include/linux/pps.h
> +++ b/include/linux/pps.h
> @@ -151,8 +151,8 @@ struct pps_device {
>
> __u32 assert_sequence; /* PPS' assert event seq # */
> __u32 clear_sequence; /* PPS' clear event seq # */
> - struct pps_ktime assert_tu;
> - struct pps_ktime clear_tu;
> + struct pps_ktime assert_tu[4];
> + struct pps_ktime clear_tu[4];
Ditto.
> int current_mode; /* PPS mode at event time */
>
> int go; /* PPS event is arrived? */
> @@ -184,12 +184,13 @@ extern struct device_attribute pps_attrs[];
>
> struct pps_device *pps_get_source(int source);
> extern void pps_put_source(struct pps_device *pps);
> -extern int pps_register_source(struct pps_source_info *info,
> +extern struct pps_device *pps_register_source(struct pps_source_info *info,
> int default_params);
> -extern void pps_unregister_source(int source);
> +extern void pps_unregister_source(struct pps_device *pps);
> extern int pps_register_cdev(struct pps_device *pps);
> extern void pps_unregister_cdev(struct pps_device *pps);
> -extern void pps_event(int source, struct pps_ktime *ts, int event, void *data);
> +extern void pps_event(struct pps_device *pps, struct pps_ktime *ts,
> + int event, void *data);
>
> #endif /* __KERNEL__ */
>
> --
> 1.6.0.6
Please, solve (or explain) the locking issue during
pps_unregister_source() and pps_event(), other things are cosmetics!
:)
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti at enneenne.com
Linux Device Driver giometti at linux.it
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
More information about the LinuxPPS
mailing list