Messages in this thread | | | Date | Sat, 20 Nov 2010 16:44:17 +0100 | From | Rodolfo Giometti <> | Subject | Re: [PATCHv4 05/17] pps: access pps device by direct pointer |
| |
On Thu, Nov 18, 2010 at 07:00:58PM +0300, Alexander Gordeev wrote: > Using device index as a pointer needs some unnecessary work to be done > every time the pointer is needed (in irq handler for example). > Using a direct pointer is much more easy (and safe as well). > > Signed-off-by: Alexander Gordeev <lasaine@lvk.cs.msu.su> > --- > drivers/pps/clients/pps-ktimer.c | 30 ++++----- > drivers/pps/clients/pps-ldisc.c | 52 ++++++++++------ > drivers/pps/kapi.c | 124 +++++++++----------------------------- > drivers/pps/pps.c | 22 ++----- > include/linux/pps_kernel.h | 23 +++---- > 5 files changed, 90 insertions(+), 161 deletions(-) > > diff --git a/drivers/pps/clients/pps-ktimer.c b/drivers/pps/clients/pps-ktimer.c > index e1bdd8b..966ead1 100644 > --- a/drivers/pps/clients/pps-ktimer.c > +++ b/drivers/pps/clients/pps-ktimer.c > @@ -31,7 +31,7 @@ > * Global variables > */ > > -static int source; > +static struct pps_device *pps; > static struct timer_list ktimer; > > /* > @@ -47,7 +47,7 @@ static void pps_ktimer_event(unsigned long ptr) > > pr_info("PPS event at %lu\n", jiffies); > > - pps_event(source, &ts, PPS_CAPTUREASSERT, NULL); > + pps_event(pps, &ts, PPS_CAPTUREASSERT, NULL); > > mod_timer(&ktimer, jiffies + HZ); > } > @@ -56,12 +56,11 @@ 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(struct pps_device *pps, int event, void *data) > { > - pr_info("echo %s %s for source %d\n", > + dev_info(pps->dev, "echo %s %s\n", > event & PPS_CAPTUREASSERT ? "assert" : "", > - event & PPS_CAPTURECLEAR ? "clear" : "", > - source); > + event & PPS_CAPTURECLEAR ? "clear" : ""); > } > > /* > @@ -84,30 +83,27 @@ static struct pps_source_info pps_ktimer_info = { > > static void __exit pps_ktimer_exit(void) > { > - del_timer_sync(&ktimer); > - pps_unregister_source(source); > + dev_info(pps->dev, "ktimer PPS source unregistered\n"); > > - pr_info("ktimer PPS source unregistered\n"); > + del_timer_sync(&ktimer); > + pps_unregister_source(pps); > } > > static int __init pps_ktimer_init(void) > { > - int ret; > - > - ret = pps_register_source(&pps_ktimer_info, > + pps = pps_register_source(&pps_ktimer_info, > PPS_CAPTUREASSERT | PPS_OFFSETASSERT); > - if (ret < 0) { > + if (pps == NULL) { > printk(KERN_ERR "cannot register ktimer source\n"); > - return ret; > + return -ENOMEM; > } > - source = ret; > > setup_timer(&ktimer, pps_ktimer_event, 0); > mod_timer(&ktimer, jiffies + HZ); > > - pr_info("ktimer PPS source registered at %d\n", source); > + dev_info(pps->dev, "ktimer PPS source registered\n"); > > - return 0; > + return 0; > } > > module_init(pps_ktimer_init); > diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c > index 20fc9f7..1950b15 100644 > --- a/drivers/pps/clients/pps-ldisc.c > +++ b/drivers/pps/clients/pps-ldisc.c > @@ -23,14 +23,18 @@ > #include <linux/serial_core.h> > #include <linux/tty.h> > #include <linux/pps_kernel.h> > +#include <linux/spinlock.h> > > #define PPS_TTY_MAGIC 0x0001 > > +static DEFINE_SPINLOCK(pps_ldisc_lock); > + > static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status, > struct pps_event_time *ts) > { > - int id = (long)tty->disc_data; > + struct pps_device *pps; > struct pps_event_time __ts; > + unsigned long flags; > > /* First of all we get the time stamp... */ > pps_get_ts(&__ts); > @@ -39,12 +43,18 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status, > if (!ts) /* No. Do it ourself! */ > ts = &__ts; > > - /* Now do the PPS event report */ > - pps_event(id, ts, status ? PPS_CAPTUREASSERT : PPS_CAPTURECLEAR, > - NULL); > + spin_lock_irqsave(&pps_ldisc_lock, flags); > > - pr_debug("PPS %s at %lu on source #%d\n", > - status ? "assert" : "clear", jiffies, id); > + /* Now do the PPS event report */ > + pps = (struct pps_device *)tty->disc_data; > + if (pps != NULL) { > + pps_event(pps, ts, status ? PPS_CAPTUREASSERT : > + PPS_CAPTURECLEAR, NULL); > + spin_unlock_irqrestore(&pps_ldisc_lock, flags); > + dev_dbg(pps->dev, "PPS %s at %lu\n", > + status ? "assert" : "clear", jiffies);
I think you should swap these two lines because after you release the lock the pps pointer may be not valid...
> + } else > + spin_unlock_irqrestore(&pps_ldisc_lock, flags); > } > > static int (*alias_n_tty_open)(struct tty_struct *tty); > @@ -54,7 +64,7 @@ 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; > - int ret; > + struct pps_device *pps; > > info.owner = THIS_MODULE; > info.dev = NULL; > @@ -64,20 +74,22 @@ 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 (pps == NULL) { > pr_err("cannot register PPS source \"%s\"\n", info.path); > - return ret; > + return -ENOMEM; > } > - tty->disc_data = (void *)(long)ret; > + > + spin_lock_irq(&pps_ldisc_lock); > + tty->disc_data = pps; > + spin_unlock_irq(&pps_ldisc_lock);
Maybe this lock is useless... however, are we sure that before setting tty->disc_data to pps its value is null? Otherwise the dcd_change may be called with an oops! We cannot control serial port IRQ generation... :-/
> /* Should open N_TTY ldisc too */ > - ret = alias_n_tty_open(tty); > - if (ret < 0) > - pps_unregister_source((long)tty->disc_data); > + if (alias_n_tty_open(tty) < 0)
Are you sure this code style is «canonical»? What does checkpatch say? ;)
> + pps_unregister_source(pps);
If the above function fails I suppose you should invalidate tty->disc_data, then unregister the pps source and, in the end, return error (I know old code returns 0 anywhere, but I think it can be fixed right now! ;).
> - pr_info("PPS source #%d \"%s\" added\n", ret, info.path); > + dev_info(pps->dev, "source \"%s\" added\n", info.path); > > return 0; > } > @@ -86,12 +98,16 @@ static void (*alias_n_tty_close)(struct tty_struct *tty); > > static void pps_tty_close(struct tty_struct *tty) > { > - int id = (long)tty->disc_data; > + struct pps_device *pps = (struct pps_device *)tty->disc_data; > > - pps_unregister_source(id); > alias_n_tty_close(tty); > > - pr_info("PPS source #%d removed\n", id); > + spin_lock_irq(&pps_ldisc_lock); > + tty->disc_data = NULL; > + spin_unlock_irq(&pps_ldisc_lock); > + > + dev_info(pps->dev, "removed\n"); > + pps_unregister_source(pps); > } > > static struct tty_ldisc_ops pps_ldisc_ops;
I suppose you can solve all your problems if you do the lock into pps_tty_init...
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti@enneenne.com Linux Device Driver giometti@linux.it Embedded Systems phone: +39 349 2432127 UNIX programming skype: rodolfo.giometti Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |