3.8-rc regression with pps-ldisc due to 70ece7a731

3.8-rc regression with pps-ldisc due to 70ece7a731

[George Spelvin]

"TTY: n_tty, add ldisc data to n_tty"

The PPS line discipline has incestuous relations with the n_tty line
discipline, using some hooks to call internal routines.

However, I started noticing violent kernel explosions when testing 3.8-rc,
and after a bit of digging, I think it's due to the fact that the PPS code
assumes that the ->ldisc pointer is available to hold a pointer to a "pps"
structure, but this commit started using it in the core n_tty discipline.

If you look at pps_tty_open in drivers/pps/clients/pps-ldisc.c, you can
see it does:

	pps = pps_register_source(&info, PPS_CAPTUREBOTH | \
				PPS_OFFSETASSERT | PPS_OFFSETCLEAR);
	if (pps == NULL) {
		pr_err("cannot register PPS source \"%s\"\n", info.path);
		return -ENOMEM;
	}
	tty->disc_data = pps;

        /* Should open N_TTY ldisc too */
        ret = alias_n_tty_open(tty);

Where "alias_n_tty_open" is filled in by n_tty_inherit_ops() to be
n_tty_open().  However, in this commit, n_tty_open() now allocates
its own structure and overwrites the disc_data pointer, leading to an
earth-shattering kaboom as NULL pointers are dereferenced in interrupt
handlers.


My first thought is to reserve a pointer in n_tty_data for the
pps structure and update the pps code to chase pointers one more
level.  But I wanted to solicit opinions.

Re: 3.8-rc regression with pps-ldisc due to 70ece7a731

[George Spelvin]

Just FYI, here is the (ugly and appears to crash) sort of patch
I was contemplating.

You may consider this Signed-off-by: in the narrow technical sense that I
can certify the origin of the code, but obviously I do not consider it
a candidate for upstream merging.  It is posted here in the hopes that
its sheer hideousness will inspire someone else to show that they can
do better.

diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index 19083ef..c149c70 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -73,7 +73,10 @@
 #define ECHO_OP_SET_CANON_COL 0x81
 #define ECHO_OP_ERASE_TAB 0x82
 
+struct pps_device;	/* Used by drivers/pps/clients/pps-ldisc.c */
+
 struct n_tty_data {
+	struct pps_device *pps;	/* First so pps-ldisc doesn't have to know offset. */
 	unsigned int column;
 	unsigned long overrun_time;
 	int num_overrun;
@@ -1636,6 +1639,7 @@ static int n_tty_open(struct tty_struct *tty)
 	mutex_init(&ldata->output_lock);
 	mutex_init(&ldata->echo_lock);
 	spin_lock_init(&ldata->read_lock);
+	//tty->pps = NULL;	/* Done by kzalloc */
 
 	/* These are ugly. Currently a malloc failure here can panic */
 	ldata->read_buf = kzalloc(N_TTY_BUF_SIZE, GFP_KERNEL);
@@ -1646,10 +1650,10 @@ static int n_tty_open(struct tty_struct *tty)
 	tty->disc_data = ldata;
 	reset_buffer_flags(tty);
 	tty_unthrottle(tty);
-	ldata->column = 0;
+	//ldata->column = 0;	/* Done by kzalloc */
 	n_tty_set_termios(tty, NULL);
 	tty->minimum_to_wake = 1;
-	tty->closing = 0;
+	//tty->closing = 0;	/* Done by kzalloc */
 
 	return 0;
 err_free_bufs:
diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index 79451f2..b5c6dd8 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -31,9 +31,16 @@
 static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
 				struct pps_event_time *ts)
 {
-	struct pps_device *pps = (struct pps_device *)tty->disc_data;
+	struct pps_device *pps = *(struct pps_device **)tty->disc_data;
 
-	BUG_ON(pps == NULL);
+	/* Because we set the pps field *after* the tty is opened, there's
+	 * a race window during which this can happen.
+	 */
+	if (pps == NULL) {
+		pr_err("Race condition triggered, pps = NULL");
+		return;
+	}
+//	BUG_ON(pps == NULL);
 
 	/* Now do the PPS event report */
 	pps_event(pps, ts, status ? PPS_CAPTUREASSERT :
@@ -67,7 +74,6 @@ static int pps_tty_open(struct tty_struct *tty)
 		pr_err("cannot register PPS source \"%s\"\n", info.path);
 		return -ENOMEM;
 	}
-	tty->disc_data = pps;
 
 	/* Should open N_TTY ldisc too */
 	ret = alias_n_tty_open(tty);
@@ -75,13 +81,13 @@ static int pps_tty_open(struct tty_struct *tty)
 		pr_err("cannot open tty ldisc \"%s\"\n", info.path);
 		goto err_unregister;
 	}
+	*(struct pps_device **)tty->disc_data = pps;
 
 	dev_info(pps->dev, "source \"%s\" added\n", info.path);
 
 	return 0;
 
 err_unregister:
-	tty->disc_data = NULL;
 	pps_unregister_source(pps);
 	return ret;
 }
@@ -90,11 +96,11 @@ static void (*alias_n_tty_close)(struct tty_struct *tty);
 
 static void pps_tty_close(struct tty_struct *tty)
 {
-	struct pps_device *pps = (struct pps_device *)tty->disc_data;
+	struct pps_device *pps = *(struct pps_device **)tty->disc_data;
 
+	*(struct pps_device **)tty->disc_data = NULL;
 	alias_n_tty_close(tty);
 
-	tty->disc_data = NULL;
 	dev_info(pps->dev, "removed\n");
 	pps_unregister_source(pps);
 }

Re: 3.8-rc regression with pps-ldisc due to 70ece7a731

[George Spelvin]

Just a quick update: the previously posted patch *does* work;
the crash I was experiencing was pilot error.

My NTP server is running a 3.8.0-rc6-dirty kernel right now.

I'll research whether that race I talk about in pps_tty_dcd_change
is actually possible or not (can interrupts start arriving before the
->open() method returns?) and work out a finished minimal bugfix patch
if nobody else finds a better solution.

Sorry for the delay tracking this down; I've known about the crash
for a week or so now, but was short of around tuitts to track it down.

(Have I mentioned how ANNOYING it is when the kernel dumps more than
50 lines of crash message to the console screen and then locks
the keyboard so I can't scroll back?)

Re: 3.8-rc regression with pps-ldisc due to 70ece7a731

[Peter Hurley]

Hi George,

On Sun, 2013-02-03 at 20:03 -0500, George Spelvin wrote:
> "TTY: n_tty, add ldisc data to n_tty"
> 
> The PPS line discipline has incestuous relations with the n_tty line
> discipline, using some hooks to call internal routines.
> 
> However, I started noticing violent kernel explosions when testing 3.8-rc,
> and after a bit of digging, I think it's due to the fact that the PPS code
> assumes that the ->ldisc pointer is available to hold a pointer to a "pps"
> structure, but this commit started using it in the core n_tty discipline.
> 
> If you look at pps_tty_open in drivers/pps/clients/pps-ldisc.c, you can
> see it does:
> 
> 	pps = pps_register_source(&info, PPS_CAPTUREBOTH | \
> 				PPS_OFFSETASSERT | PPS_OFFSETCLEAR);
> 	if (pps == NULL) {
> 		pr_err("cannot register PPS source \"%s\"\n", info.path);
> 		return -ENOMEM;
> 	}
> 	tty->disc_data = pps;
> 
>         /* Should open N_TTY ldisc too */
>         ret = alias_n_tty_open(tty);
> 
> Where "alias_n_tty_open" is filled in by n_tty_inherit_ops() to be
> n_tty_open().  However, in this commit, n_tty_open() now allocates
> its own structure and overwrites the disc_data pointer, leading to an
> earth-shattering kaboom as NULL pointers are dereferenced in interrupt
> handlers.

Yuck. Little wonder it broke.

> My first thought is to reserve a pointer in n_tty_data for the
> pps structure and update the pps code to chase pointers one more
> level.  But I wanted to solicit opinions.

Tight coupling is what caused this to break in the first place -- I
don't think tighter coupling is the right answer.

Alternate coming...

Regards,
Peter Hurley

[PATCH 0/4] tty, pps: decouple pps

[Peter Hurley]

The recent breakage of the N_PPS line discipline is due to the tight coupling
pps has created in the tty subsystem. Since the extension of the line discipline
api to accomodate the needs of pps, this level of coupling is unnecessary and
undesirable.

Peter Hurley (4):
  pps: Decouple N_PPS from N_TTY
  pps: Don't crash the machine when exiting will do
  pps: Use lookup list to reduce ldisc coupling
  tty: Remove ancient hardpps()

 drivers/pps/clients/pps-ldisc.c     | 76 ++++++++++++++++++++++++++++++++-----
 drivers/staging/speakup/selection.c |  1 +
 drivers/tty/amiserial.c             |  5 ---
 drivers/tty/n_tty.c                 |  3 +-
 drivers/tty/serial/serial_core.c    | 23 ++++-------
 drivers/tty/tty_buffer.c            |  1 +
 include/linux/serial_core.h         |  1 -
 include/linux/tty_ldisc.h           | 11 ++----
 8 files changed, 83 insertions(+), 38 deletions(-)

-- 
1.8.1.2

[PATCH 1/4] pps: Decouple N_PPS from N_TTY

[Peter Hurley]

The PPS (Pulse-Per-Second) line discipline has developed a number of
unhealthy attachments to core tty data and functions, ultimately leading
to its breakage.

Remove header file linkage.
Make ldisc api extension generic.
Fix attendant build breakage in
    drivers/staging/speakup/selection.c
    drivers/tty/tty_buffer.c
    drivers/tty/n_tty.c

Cc: William Hubbs <w.d.hubbs@gmail.com>
Cc: Chris Brannon <chris@the-brannons.com>
Cc: Kirk Reiser <kirk@braille.uwo.ca>
Cc: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/pps/clients/pps-ldisc.c     |  8 +++++---
 drivers/staging/speakup/selection.c |  1 +
 drivers/tty/n_tty.c                 |  3 ++-
 drivers/tty/serial/serial_core.c    | 20 +++++++++-----------
 drivers/tty/tty_buffer.c            |  1 +
 include/linux/serial_core.h         |  1 -
 include/linux/tty_ldisc.h           | 11 ++++-------
 7 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index 79451f2..27d7ca1 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -28,15 +28,17 @@
 
 #define PPS_TTY_MAGIC		0x0001
 
-static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
-				struct pps_event_time *ts)
+static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status)
 {
+	struct pps_event_time ts;
 	struct pps_device *pps = (struct pps_device *)tty->disc_data;
 
+	pps_get_ts(&ts);
+
 	BUG_ON(pps == NULL);
 
 	/* Now do the PPS event report */
-	pps_event(pps, ts, status ? PPS_CAPTUREASSERT :
+	pps_event(pps, &ts, status ? PPS_CAPTUREASSERT :
 			PPS_CAPTURECLEAR, NULL);
 
 	dev_dbg(pps->dev, "PPS %s at %lu\n",
diff --git a/drivers/staging/speakup/selection.c b/drivers/staging/speakup/selection.c
index d6558fa..72afc05 100644
--- a/drivers/staging/speakup/selection.c
+++ b/drivers/staging/speakup/selection.c
@@ -2,6 +2,7 @@
 #include <linux/consolemap.h>
 #include <linux/interrupt.h>
 #include <linux/sched.h>
+#include <linux/device.h> /* for dev_warn */
 #include <linux/selection.h>
 
 #include "speakup.h"
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
index e269296..911ccb5 100644
--- a/drivers/tty/n_tty.c
+++ b/drivers/tty/n_tty.c
@@ -49,6 +49,7 @@
 #include <linux/file.h>
 #include <linux/uaccess.h>
 #include <linux/module.h>
+#include <linux/ratelimit.h>
 
 
 /* number of characters left in xmit buffer before select has we have room */
@@ -2198,7 +2199,7 @@ struct tty_ldisc_ops tty_ldisc_N_TTY = {
  *	n_tty_inherit_ops	-	inherit N_TTY methods
  *	@ops: struct tty_ldisc_ops where to save N_TTY methods
  *
- *	Used by a generic struct tty_ldisc_ops to easily inherit N_TTY
+ *	Enables 'subclass' line discipline to 'inherit' N_TTY
  *	methods.
  */
 
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index ca98a3f..457f1a6 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2726,31 +2726,29 @@ void uart_handle_dcd_change(struct uart_port *uport, unsigned int status)
 	struct uart_state *state = uport->state;
 	struct tty_port *port = &state->port;
 	struct tty_ldisc *ld = NULL;
-	struct pps_event_time ts;
 	struct tty_struct *tty = port->tty;
 
-	if (tty)
-	        ld = tty_ldisc_ref(tty);
-	if (ld && ld->ops->dcd_change)
-		pps_get_ts(&ts);
-
 	uport->icount.dcd++;
+
 #ifdef CONFIG_HARD_PPS
 	if ((uport->flags & UPF_HARDPPS_CD) && status)
 		hardpps();
 #endif
 
+	if (tty) {
+		ld = tty_ldisc_ref(tty);
+		if (ld && ld->ops->dcd_change)
+			ld->ops->dcd_change(tty, status);
+		if (ld)
+			tty_ldisc_deref(ld);
+	}
+
 	if (port->flags & ASYNC_CHECK_CD) {
 		if (status)
 			wake_up_interruptible(&port->open_wait);
 		else if (tty)
 			tty_hangup(tty);
 	}
-
-	if (ld && ld->ops->dcd_change)
-		ld->ops->dcd_change(tty, status, &ts);
-	if (ld)
-		tty_ldisc_deref(ld);
 }
 EXPORT_SYMBOL_GPL(uart_handle_dcd_change);
 
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 61ec4dd..bb11993 100644
--- a/drivers/tty/tty_buffer.c
+++ b/drivers/tty/tty_buffer.c
@@ -16,6 +16,7 @@
 #include <linux/bitops.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/ratelimit.h>
 
 /**
  *	tty_buffer_free_all		-	free buffers used by a tty
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index d971421..87d4bbc 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -29,7 +29,6 @@
 #include <linux/tty.h>
 #include <linux/mutex.h>
 #include <linux/sysrq.h>
-#include <linux/pps_kernel.h>
 #include <uapi/linux/serial_core.h>
 
 struct uart_port;
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index fb79dd8d..455a0d7 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -100,16 +100,14 @@
  *	seek to perform this action quickly but should wait until
  *	any pending driver I/O is completed.
  *
- * void (*dcd_change)(struct tty_struct *tty, unsigned int status,
- *			struct pps_event_time *ts)
+ * void (*dcd_change)(struct tty_struct *tty, unsigned int status)
  *
- *	Tells the discipline that the DCD pin has changed its status and
- *	the relative timestamp. Pointer ts cannot be NULL.
+ *	Tells the discipline that the DCD pin has changed its status.
+ *	Used exclusively by the N_PPS (Pulse-Per-Second) line discipline.
  */
 
 #include <linux/fs.h>
 #include <linux/wait.h>
-#include <linux/pps_kernel.h>
 #include <linux/wait.h>
 
 struct tty_ldisc_ops {
@@ -144,8 +142,7 @@ struct tty_ldisc_ops {
 	void	(*receive_buf)(struct tty_struct *, const unsigned char *cp,
 			       char *fp, int count);
 	void	(*write_wakeup)(struct tty_struct *);
-	void	(*dcd_change)(struct tty_struct *, unsigned int,
-				struct pps_event_time *);
+	void	(*dcd_change)(struct tty_struct *, unsigned int);
 
 	struct  module *owner;
 	
-- 
1.8.1.2

[PATCH 2/4] pps: Don't crash the machine when exiting will do

[Peter Hurley]

PPS is not really the must-have subsystem that warrants crashing
the machine if the ldisc interface is broken.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/pps/clients/pps-ldisc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index 27d7ca1..0b91d91 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -25,6 +25,7 @@
 #include <linux/serial_core.h>
 #include <linux/tty.h>
 #include <linux/pps_kernel.h>
+#include <linux/bug.h>
 
 #define PPS_TTY_MAGIC		0x0001
 
@@ -35,7 +36,8 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status)
 
 	pps_get_ts(&ts);
 
-	BUG_ON(pps == NULL);
+	if (WARN_ON_ONCE(pps == NULL))
+		return;
 
 	/* Now do the PPS event report */
 	pps_event(pps, &ts, status ? PPS_CAPTUREASSERT :
-- 
1.8.1.2

[PATCH 3/4] pps: Use lookup list to reduce ldisc coupling

[Peter Hurley]

Now that N_TTY uses tty->disc_data for its private data,
'subclass' ldiscs cannot use ->disc_data for their own private data.

Use a lookup list to associate the tty with the pps source.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/pps/clients/pps-ldisc.c | 64 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 59 insertions(+), 5 deletions(-)

diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index 0b91d91..a36d42b 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -25,17 +25,47 @@
 #include <linux/serial_core.h>
 #include <linux/tty.h>
 #include <linux/pps_kernel.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
 #include <linux/bug.h>
 
 #define PPS_TTY_MAGIC		0x0001
 
+struct pps_data {
+	struct pps_device *pps;
+	struct tty_struct *tty;
+	struct list_head link;
+};
+
+DEFINE_SPINLOCK(pps_lock);
+static LIST_HEAD(pps_list);
+
+static struct pps_device *lookup_pps_by_tty(struct tty_struct *tty,
+					    struct pps_data **p)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&pps_lock, flags);
+	list_for_each_entry((*p), &pps_list, link) {
+		if ((*p)->tty == tty) {
+			spin_unlock_irqrestore(&pps_lock, flags);
+			return (*p)->pps;
+		}
+	}
+	spin_unlock_irqrestore(&pps_lock, flags);
+	return NULL;
+}
+
 static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status)
 {
 	struct pps_event_time ts;
-	struct pps_device *pps = (struct pps_device *)tty->disc_data;
+	struct pps_device *pps;
+	struct pps_data *data;
 
 	pps_get_ts(&ts);
 
+	pps = lookup_pps_by_tty(tty, &data);
 	if (WARN_ON_ONCE(pps == NULL))
 		return;
 
@@ -55,6 +85,8 @@ static int pps_tty_open(struct tty_struct *tty)
 	struct tty_driver *drv = tty->driver;
 	int index = tty->index + drv->name_base;
 	struct pps_device *pps;
+	struct pps_data *data;
+	unsigned long flags;
 	int ret;
 
 	info.owner = THIS_MODULE;
@@ -71,7 +103,12 @@ static int pps_tty_open(struct tty_struct *tty)
 		pr_err("cannot register PPS source \"%s\"\n", info.path);
 		return -ENOMEM;
 	}
-	tty->disc_data = pps;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data) {
+		ret = -ENOMEM;
+		goto err_unregister;
+	}
 
 	/* Should open N_TTY ldisc too */
 	ret = alias_n_tty_open(tty);
@@ -80,12 +117,19 @@ static int pps_tty_open(struct tty_struct *tty)
 		goto err_unregister;
 	}
 
+	data->pps = pps;
+	data->tty = tty;
+	INIT_LIST_HEAD(&data->link);
+	spin_lock_irqsave(&pps_lock, flags);
+	list_add(&data->link, &pps_list);
+	spin_unlock_irqrestore(&pps_lock, flags);
+
 	dev_info(pps->dev, "source \"%s\" added\n", info.path);
 
 	return 0;
 
 err_unregister:
-	tty->disc_data = NULL;
+	kfree(data);
 	pps_unregister_source(pps);
 	return ret;
 }
@@ -94,13 +138,23 @@ static void (*alias_n_tty_close)(struct tty_struct *tty);
 
 static void pps_tty_close(struct tty_struct *tty)
 {
-	struct pps_device *pps = (struct pps_device *)tty->disc_data;
+	struct pps_device *pps;
+	struct pps_data *data;
+	unsigned long flags;
 
 	alias_n_tty_close(tty);
 
-	tty->disc_data = NULL;
+	pps = lookup_pps_by_tty(tty, &data);
+	if (!pps)
+		return;
+
 	dev_info(pps->dev, "removed\n");
 	pps_unregister_source(pps);
+
+	spin_lock_irqsave(&pps_lock, flags);
+	list_del(&data->link);
+	spin_unlock_irqrestore(&pps_lock, flags);
+	kfree(data);
 }
 
 static struct tty_ldisc_ops pps_ldisc_ops;
-- 
1.8.1.2

[PATCH 4/4] tty: Remove ancient hardpps()

[Peter Hurley]

hardpps() functionality is provided through the N_PPS line
discipline now. The new function signature was added in commit
025b40ab (2011-01-12). There was no previous macro or
function hardpps(), at least since before the initial commit of
v2.6.12 in 2005. It's unlikely this code has been compiled since.

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
---
 drivers/tty/amiserial.c          | 5 -----
 drivers/tty/serial/serial_core.c | 5 -----
 2 files changed, 10 deletions(-)

diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index 4c7d701..fc70034 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -393,11 +393,6 @@ static void check_modem_status(struct serial_state *info)
 			icount->dsr++;
 		if (dstatus & SER_DCD) {
 			icount->dcd++;
-#ifdef CONFIG_HARD_PPS
-			if ((port->flags & ASYNC_HARDPPS_CD) &&
-			    !(status & SER_DCD))
-				hardpps();
-#endif
 		}
 		if (dstatus & SER_CTS)
 			icount->cts++;
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 457f1a6..b96b1f7 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2730,11 +2730,6 @@ void uart_handle_dcd_change(struct uart_port *uport, unsigned int status)
 
 	uport->icount.dcd++;
 
-#ifdef CONFIG_HARD_PPS
-	if ((uport->flags & UPF_HARDPPS_CD) && status)
-		hardpps();
-#endif
-
 	if (tty) {
 		ld = tty_ldisc_ref(tty);
 		if (ld && ld->ops->dcd_change)
-- 
1.8.1.2

Re: 3.8-rc regression with pps-ldisc due to 70ece7a731

[Peter Hurley]

On Mon, 2013-02-04 at 02:08 -0500, George Spelvin wrote:
> Just a quick update: the previously posted patch *does* work;
> the crash I was experiencing was pilot error.
> 
> My NTP server is running a 3.8.0-rc6-dirty kernel right now.
> 
> I'll research whether that race I talk about in pps_tty_dcd_change
> is actually possible or not (can interrupts start arriving before the
> ->open() method returns?) and work out a finished minimal bugfix patch
> if nobody else finds a better solution.

You are not supposed to receive ldisc->dcd_change() calls outside
the open()/close() pair.

The ldisc is separately enabled/halted which is supposed to prevent
ldisc usage if a ldisc reference cannot be acquired (because it's
halted). The reference is acquired prior to calling the ->dcd_change()
routine.

In the patch series I sent, I changed the BUG_ON() to WARN_ON_ONCE().
Please reply to that patch with the snipped kernel log output if it
warns in your testing and we'll go from there.

> (Have I mentioned how ANNOYING it is when the kernel dumps more than
> 50 lines of crash message to the console screen and then locks
> the keyboard so I can't scroll back?)

netconsole on 2nd machine (see Documentation/networking/netconsole.txt)

Regards,
Peter Hurley

Re: [PATCH 3/4] pps: Use lookup list to reduce ldisc coupling

[Jiri Slaby]

On 02/06/2013 04:55 PM, Peter Hurley wrote:
> --- a/drivers/pps/clients/pps-ldisc.c
> +++ b/drivers/pps/clients/pps-ldisc.c
> @@ -25,17 +25,47 @@
...
> +struct pps_data {
> +	struct pps_device *pps;
> +	struct tty_struct *tty;
> +	struct list_head link;
> +};
> +
> +DEFINE_SPINLOCK(pps_lock);
> +static LIST_HEAD(pps_list);

All 4 look fine, nice cleanup. Except the lock above should be static too.

thanks,
-- 
js
suse labs

Re: [PATCH 3/4] pps: Use lookup list to reduce ldisc coupling

[Peter Hurley]

On Wed, 2013-02-06 at 17:20 +0100, Jiri Slaby wrote:
> On 02/06/2013 04:55 PM, Peter Hurley wrote:
> > --- a/drivers/pps/clients/pps-ldisc.c
> > +++ b/drivers/pps/clients/pps-ldisc.c
> > @@ -25,17 +25,47 @@
> ...
> > +struct pps_data {
> > +	struct pps_device *pps;
> > +	struct tty_struct *tty;
> > +	struct list_head link;
> > +};
> > +
> > +DEFINE_SPINLOCK(pps_lock);
> > +static LIST_HEAD(pps_list);
> 
> All 4 look fine, nice cleanup. Except the lock above should be static too.

Whoops. Thanks for catching that!  (rebasing error... :)

Re: [PATCH 3/4] pps: Use lookup list to reduce ldisc coupling

[George Spelvin]

> Now that N_TTY uses tty->disc_data for its private data,
> 'subclass' ldiscs cannot use ->disc_data for their own private data.
> 
> Use a lookup list to associate the tty with the pps source.

Thanks for the cleanup.  I fully agree my patch was not a good one;
I just wanted someone more experienced to make the call on rearchitecting.

In particular, I was nervous about getting flamed by Linus for something that
was too ambitious.

One thing I'd prefer to do would be to change:

+static struct pps_device *lookup_pps_by_tty(struct tty_struct *tty,
+					    struct pps_data **p)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&pps_lock, flags);
+	list_for_each_entry((*p), &pps_list, link) {
+		if ((*p)->tty == tty) {
+			spin_unlock_irqrestore(&pps_lock, flags);
+			return (*p)->pps;
+		}
+	}
+	spin_unlock_irqrestore(&pps_lock, flags);
+	return NULL;
+}

to:

static struct pps_data *lookup_pps_by_tty(struct tty_struct *tty)
{
	unsigned long flags;

	spin_lock_irqsave(&pps_lock, flags);
	list_for_each_entry(p, &pps_list, link) {
		if (p->tty == tty)
			break;
	}
	spin_unlock_irqrestore(&pps_lock, flags);
	return p;
}

And do the data->pps dereferencing in the caller.


A more ambitious cleanup would use the existing pps_device list
(maintained to allocate minor device numbers) and add an "owner" field
that can be looked up on, without creating a new data structure and
allocation.

(It could either be a generic "void *", or a "struct device *" and
compare it to tty->dev.)

After all, despite the implementation effort to scale, the total number
of pps devices in a system is usually at most 1 (I have a computer where
I run 2, and I doubt there are many others on the planet who do that.)

Re: 3.8-rc regression with pps-ldisc due to 70ece7a731

[George Spelvin]

> Tight coupling is what caused this to break in the first place -- I
> don't think tighter coupling is the right answer.

Agreed.  But given that n_tty already knows there are wrappers, it would
have been possible to find a cleaner way to access an "aux pointer" in
the tty structure, if that's what was desired.

> You are not supposed to receive ldisc->dcd_change() calls outside
> the open()/close() pair.

Yes, I figured that out.  I was wondering because I couldn't see any
way that the serial interrupt hander was blocked or masked, but
then I figured out that it's not, but instead the TTY_LDISC flag
is used.

At the time the open() method is called, the flag is cleared, which makes
tty_ldisc_ref() return NULL, which prevents calling the ldisc methods.

> In the patch series I sent, I changed the BUG_ON() to WARN_ON_ONCE().
> Please reply to that patch with the snipped kernel log output if it
> warns in your testing and we'll go from there.

I really doubt it will.  The entire code change and comment was
just caution on my part.

>> (Have I mentioned how ANNOYING it is when the kernel dumps more than
>> 50 lines of crash message to the console screen and then locks
>> the keyboard so I can't scroll back?)

> netconsole on 2nd machine (see Documentation/networking/netconsole.txt)

Oh, that survives interrupt crashes?  Thank you!

Re: [PATCH 3/4] pps: Use lookup list to reduce ldisc coupling

[Peter Hurley]

On Wed, 2013-02-06 at 14:34 -0500, George Spelvin wrote:
> > Now that N_TTY uses tty->disc_data for its private data,
> > 'subclass' ldiscs cannot use ->disc_data for their own private data.
> > 
> > Use a lookup list to associate the tty with the pps source.
> 
> Thanks for the cleanup.  I fully agree my patch was not a good one;
> I just wanted someone more experienced to make the call on rearchitecting.
> 
> In particular, I was nervous about getting flamed by Linus for something that
> was too ambitious.

No problem and I completely understand. That's why I jumped in -- it
looked like some help was needed, both now and maybe even in iterations
before this.

> One thing I'd prefer to do would be to change:
> 
> +static struct pps_device *lookup_pps_by_tty(struct tty_struct *tty,
> +					    struct pps_data **p)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&pps_lock, flags);
> +	list_for_each_entry((*p), &pps_list, link) {
> +		if ((*p)->tty == tty) {
> +			spin_unlock_irqrestore(&pps_lock, flags);
> +			return (*p)->pps;
> +		}
> +	}
> +	spin_unlock_irqrestore(&pps_lock, flags);
> +	return NULL;
> +}
> 
> to:
> 
> static struct pps_data *lookup_pps_by_tty(struct tty_struct *tty)
> {
> 	unsigned long flags;
> 
> 	spin_lock_irqsave(&pps_lock, flags);
> 	list_for_each_entry(p, &pps_list, link) {
> 		if (p->tty == tty)
> 			break;
> 	}
> 	spin_unlock_irqrestore(&pps_lock, flags);
> 	return p;
> }
> 
> And do the data->pps dereferencing in the caller.

I did this first and it's a mess -- the patch basically ends up looking
like a rewrite. But feel free to use these patches as a base for a
version you do like and submit those instead for review. I just wanted
to show the way.

(Well, actually that was the second version. When I reviewed the
uart_handle_dcd_change() and saw the separate timestamp, I thought that
maybe the latency was going to be a problem. So the first version used
the same approach but with an rcu 'lockless' list instead -- then I went
back and audited the IRQ path and realized there were 5 bus locks and an
i/o port read already. So total overkill.)

Also, I figured maybe it would be best if it was something maintainable
with basic kernel knowledge.

> A more ambitious cleanup would use the existing pps_device list
> (maintained to allocate minor device numbers) and add an "owner" field
> that can be looked up on, without creating a new data structure and
> allocation.

Didn't see where that was (unless you mean the IDR allocation). Probably
best to keep it separate in the event that relative lifetimes change at
some point in the future.

> (It could either be a generic "void *", or a "struct device *" and
> compare it to tty->dev.)
> 
> After all, despite the implementation effort to scale, the total number
> of pps devices in a system is usually at most 1 (I have a computer where
> I run 2, and I doubt there are many others on the planet who do that.)

I thought that was probably the case which is why a lookup list is an
acceptable solution.

Please let us know if you plan to respin the patches, so these patches
don't get pushed.

Regards,
Peter Hurley

Re: 3.8-rc regression with pps-ldisc due to 70ece7a731

[Peter Hurley]

On Wed, 2013-02-06 at 14:45 -0500, George Spelvin wrote:
> > Tight coupling is what caused this to break in the first place -- I
> > don't think tighter coupling is the right answer.
> 
> Agreed.  But given that n_tty already knows there are wrappers, it would
> have been possible to find a cleaner way to access an "aux pointer" in
> the tty structure, if that's what was desired.

I know. Like n_tty_get/set_data() with a void* in struct n_tty_data. But
I'm sure you'd agree that's just an expedient. If there were multiple
uses for this requirement, it'd be better to have this supported by the
base interface for all ldiscs.

> > You are not supposed to receive ldisc->dcd_change() calls outside
> > the open()/close() pair.
> 
> Yes, I figured that out.  I was wondering because I couldn't see any
> way that the serial interrupt hander was blocked or masked, but
> then I figured out that it's not, but instead the TTY_LDISC flag
> is used.
> 
> At the time the open() method is called, the flag is cleared, which makes
> tty_ldisc_ref() return NULL, which prevents calling the ldisc methods.

Exactly.

> > In the patch series I sent, I changed the BUG_ON() to WARN_ON_ONCE().
> > Please reply to that patch with the snipped kernel log output if it
> > warns in your testing and we'll go from there.
> 
> I really doubt it will.  The entire code change and comment was
> just caution on my part.

There were race conditions in the existing ldisc layer which allowed
references past halts. Just 2 days ago, I put up a 23-part
patch series to address it. It's very complicated and it's possible I
missed something.

Regards,
Peter Hurley

Re: [PATCH 3/4] pps: Use lookup list to reduce ldisc coupling

[George Spelvin]

> I did this first and it's a mess -- the patch basically ends up looking
> like a rewrite. But feel free to use these patches as a base for a
> version you do like and submit those instead for review. I just wanted
> to show the way.

I wouldn't think so, but I'll give it a try and see myself.  Thanks!

> (Well, actually that was the second version. When I reviewed the
> uart_handle_dcd_change() and saw the separate timestamp, I thought that
> maybe the latency was going to be a problem. So the first version used
> the same approach but with an rcu 'lockless' list instead -- then I went
> back and audited the IRQ path and realized there were 5 bus locks and an
> i/o port read already. So total overkill.)

Er... but you went and captured the timestamp *before* doing the list
lookup.  It was only moved one jsr later.

Really, what I'd *like* to do is to unconditionally capture a *raw*
timestamp (rdtsc or equivalent) very early in the interrupt handling,
for use by random seeding, pps, network timestamps, and so on.  But the
conversion to a "struct timespec" would be deferred until when and if
it was actually needed.

This is complicated because the conversion has to happen "soon" after
the capture, soon enough that the low-level clock that was read has not
wrapped and become ambiguous.

But that's a lot more complicated.

>> A more ambitious cleanup would use the existing pps_device list
>> (maintained to allocate minor device numbers) and add an "owner" field
>> that can be looked up on, without creating a new data structure and
>> allocation.

> Didn't see where that was (unless you mean the IDR allocation).

Exactly the IDR.  You can just do idr_for_each() until you find
the right one.

> Probably best to keep it separate in the event that relative lifetimes
> change at some point in the future.

I don't see how that could plausibly happen.  Currently, a device
is registered in the IDR immediately after allocation, and is freed
immediately before deallocation.  There is no time that it's permitted to
call any kernel PPS API function with a pps_device * that *not* in
the IDR.

Information with a longer life is segregated in the struct
pps_source_info.  (Which is where I was thinging of adding the
parent_dev field.)

> Please let us know if you plan to respin the patches, so these patches
> don't get pushed.

I do, in the next few hours.  Can you give mt until 0600 UTC,
or should I try for faster?

Re: [PATCH 3/4] pps: Use lookup list to reduce ldisc coupling

[Peter Hurley]

On Wed, 2013-02-06 at 17:19 -0500, George Spelvin wrote:
> > I did this first and it's a mess -- the patch basically ends up looking
> > like a rewrite. But feel free to use these patches as a base for a
> > version you do like and submit those instead for review. I just wanted
> > to show the way.
> 
> I wouldn't think so, but I'll give it a try and see myself.  Thanks!
> 
> > (Well, actually that was the second version. When I reviewed the
> > uart_handle_dcd_change() and saw the separate timestamp, I thought that
> > maybe the latency was going to be a problem. So the first version used
> > the same approach but with an rcu 'lockless' list instead -- then I went
> > back and audited the IRQ path and realized there were 5 bus locks and an
> > i/o port read already. So total overkill.)
> 
> Er... but you went and captured the timestamp *before* doing the list
> lookup.  It was only moved one jsr later.

This was before I moved the dcd_change() call. In the original commit,
the timestamp was acquired in uart_handle_dcd_change() and
only after wake_up/hangup handling did it call the ldisc dcd_change().

So obviously that was misleading.

Also, I wasn't really sure how contended a lock might be. It wasn't
until I'd spent some time with the code to realize that answer was "not
contended".

> Really, what I'd *like* to do is to unconditionally capture a *raw*
> timestamp (rdtsc or equivalent) very early in the interrupt handling,
> for use by random seeding, pps, network timestamps, and so on.  But the
> conversion to a "struct timespec" would be deferred until when and if
> it was actually needed.
> 
> This is complicated because the conversion has to happen "soon" after
> the capture, soon enough that the low-level clock that was read has not
> wrapped and become ambiguous.
> 
> But that's a lot more complicated.

I understand that's a long-term plan. You should approach the RT crowd
about this. I think some might be interested in timestamping interrupts
(at least on certain platforms) for test measurement.

> >> A more ambitious cleanup would use the existing pps_device list
> >> (maintained to allocate minor device numbers) and add an "owner" field
> >> that can be looked up on, without creating a new data structure and
> >> allocation.
> 
> > Didn't see where that was (unless you mean the IDR allocation).
> 
> Exactly the IDR.  You can just do idr_for_each() until you find
> the right one.
> 
> > Probably best to keep it separate in the event that relative lifetimes
> > change at some point in the future.
> 
> I don't see how that could plausibly happen.  Currently, a device
> is registered in the IDR immediately after allocation, and is freed
> immediately before deallocation.  There is no time that it's permitted to
> call any kernel PPS API function with a pps_device * that *not* in
> the IDR.
> 
> Information with a longer life is segregated in the struct
> pps_source_info.  (Which is where I was thinging of adding the
> parent_dev field.)

Ok, so at least someone is thinking about that.

> > Please let us know if you plan to respin the patches, so these patches
> > don't get pushed.
> 
> I do, in the next few hours.  Can you give mt until 0600 UTC,
> or should I try for faster?

What release are you trying to hit? Regardless, nothing's happening
within hours -- days maybe.