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
    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/

    \
     
     \ /
      Last update: 2010-11-20 16:47    [W:0.042 / U:62.308 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site