Re: [PATCHv4 05/17] pps: access pps device by direct pointer

[Alexander Gordeev <lasaine@lvk.cs.msu.su>]

[-- Attachment #1: Type: text/plain, Size: 9746 bytes --]

В 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 490 bytes --]