lkml.org 
[lkml]   [2010]   [Nov]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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]
\
 
 \ /
  Last update: 2010-11-20 23:35    [W:0.096 / U:5.800 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site