Messages in this thread | | | Date | Sun, 21 Nov 2010 01:33:27 +0300 | From | Alexander Gordeev <> | Subject | Re: [PATCHv4 05/17] pps: access pps device by direct pointer |
| |
В Sat, 20 Nov 2010 16:44:17 +0100 Rodolfo Giometti <giometti@enneenne.com> пишет:
> 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...
Ah, yes, good catch, thank you!
> > + } 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... :-/
No, locking here is necessary. There is only one problem this spinlock protects us from: current tty code neither disables interrupts nor doesn't ensure there are no references to PPS ldisc from uart_handle_dcd_change() before closing it (and removing PPS source). It relies on flushing workqueue and disabling input. It worked good this way until dcd_change() was added which doesn't use workqueues and is called in atomic context so can't lock on mutex.
Imagine that (on SMP system) uart_handle_dcd_change() could obtain a reference to ldisc and call dcd_change() until actually calling pps_event(); then on another processor all the path from tty_ldisc_halt() until tty_ldisc_stop() is executed. And then pps_event() is called with illegal pps pointer.
I just thought you are right that disc_data can be set not NULL by another ldisc and it's a problem. But actually I just realised how to fix it completely. :)
I just have to add a spinlock to tty_struct, lock all the uart_handle_dcd_change() with it and add a "barrier" between tty_ldisc_halt() and tty_ldisc_close() i.e. just that:
... spin_lock_irq(); spin_unlock_irq(); ...
This "barrier" will ensure that there is no references to ldisc from uart_handle_dcd_change(). It won't be able to obtain a new reference after tty_ldisc_halt() so will become completely sane. Not disabling interrupts won't be a problem because it won't be able to obtain an ldisc reference until tty_ldisc_enable() which is called only after the new ldisc is fully functional. If it's our ldisc than it will have both dcd_change defined and a valid pps pointer. If it's not our ldisc it won't have both so uart_handle_dcd_change() won't call dcd_change() at all.
I think I'll do that as a separate patch.
> > /* 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? > ;)
checkpatch is completely happy about all patches. :) But indeed it's not good. I'll fix it, thanks!
> > + 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! ;).
OK. :)
> > - 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 >
-- Alexander [unhandled content-type:application/pgp-signature] | |