All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 3/7] pps: Don't crash the machine when exiting will do
  2013-02-08  7:05 [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731 George Spelvin
@ 2013-02-06 15:55 ` Peter Hurley
  2013-02-06 15:55 ` [PATCH 4/7] tty: Remove ancient hardpps() Peter Hurley
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: Peter Hurley @ 2013-02-06 15:55 UTC (permalink / raw)
  To: linux-serial, peter; +Cc: linux, linux-kernel, giometti

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>
Signed-off-by: George Spelvin <linux@horizon.com>
---
 drivers/pps/clients/pps-ldisc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index 27d7ca1..e5a702a 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,12 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status)
 
 	pps_get_ts(&ts);
 
-	BUG_ON(pps == NULL);
+	/*
+	 * This should never fail, but the ldisc locking is very
+	 * convoluted, so don't crash just in case.
+	 */
+	if (WARN_ON_ONCE(pps == NULL))
+		return;
 
 	/* Now do the PPS event report */
 	pps_event(pps, &ts, status ? PPS_CAPTUREASSERT :
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 4/7] tty: Remove ancient hardpps()
  2013-02-08  7:05 [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731 George Spelvin
  2013-02-06 15:55 ` [PATCH 3/7] pps: Don't crash the machine when exiting will do Peter Hurley
@ 2013-02-06 15:55 ` Peter Hurley
  2013-02-08  5:45 ` [PATCH 1/7] pps: Decouple N_PPS from N_TTY George Spelvin
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: Peter Hurley @ 2013-02-06 15:55 UTC (permalink / raw)
  To: linux-serial, peter; +Cc: linux, linux-kernel, giometti

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>
Signed-off-by: George Spelvin <linux@horizon.com>
---
 drivers/tty/amiserial.c          | 5 -----
 drivers/tty/serial/serial_core.c | 4 ----
 2 files changed, 9 deletions(-)

diff --git a/drivers/tty/amiserial.c b/drivers/tty/amiserial.c
index 9d7d00c..ba2e09e 100644
--- a/drivers/tty/amiserial.c
+++ b/drivers/tty/amiserial.c
@@ -394,11 +394,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 b3a204b..02be179 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2726,10 +2726,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 (port->flags & ASYNC_CHECK_CD) {
 		if (status)
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 1/7] pps: Decouple N_PPS from N_TTY
  2013-02-08  7:05 [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731 George Spelvin
  2013-02-06 15:55 ` [PATCH 3/7] pps: Don't crash the machine when exiting will do Peter Hurley
  2013-02-06 15:55 ` [PATCH 4/7] tty: Remove ancient hardpps() Peter Hurley
@ 2013-02-08  5:45 ` George Spelvin
  2013-02-08  6:06 ` [PATCH 2/7] pps: Additional cleanups in uart_handle_dcd_change George Spelvin
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: George Spelvin @ 2013-02-08  5:45 UTC (permalink / raw)
  To: linux-serial, peter
  Cc: linux, linux-kernel, giometti, William Hubbs, Chris Brannon,
	Kirk Reiser, Samuel Thibault

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.

This first part eliminates the timestamp parameter from the dcd_change
ldisc method.  This reduces header file linkage and makes the extension
more generic, and the timestamp read is delayed only slightly, from
just before the ldisc->ops->dcd_change method call to just after.

Fix attendant build breakage in
    drivers/tty/n_tty.c
    drivers/tty/tty_buffer.c
    drivers/staging/speakup/selection.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>
Signed-off-by: George Spelvin <linux@horizon.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    |  7 +++----
 drivers/tty/tty_buffer.c            |  1 +
 include/linux/serial_core.h         |  1 -
 include/linux/tty_ldisc.h           | 11 ++++-------
 7 files changed, 16 insertions(+), 16 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 0612df0..2aa2237 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 19083ef..ed66b20 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 */
@@ -2188,7 +2189,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 a '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 2c7230a..8b6bff5 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2718,13 +2718,14 @@ 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)
+	if (ld && ld->ops->dcd_change) {
 		pps_get_ts(&ts);
+		ld->ops->dcd_change(tty, status);
+	}
 
 	uport->icount.dcd++;
 #ifdef CONFIG_HARD_PPS
@@ -2739,8 +2740,6 @@ void uart_handle_dcd_change(struct uart_port *uport, unsigned int status)
 			tty_hangup(tty);
 	}
 
-	if (ld && ld->ops->dcd_change)
-		ld->ops->dcd_change(tty, status, &ts);
 	if (ld)
 		tty_ldisc_deref(ld);
 }
diff --git a/drivers/tty/tty_buffer.c b/drivers/tty/tty_buffer.c
index 45d9161..1bdd896 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 c6690a2..a5f1da9 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


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/7] pps: Additional cleanups in uart_handle_dcd_change
  2013-02-08  7:05 [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731 George Spelvin
                   ` (2 preceding siblings ...)
  2013-02-08  5:45 ` [PATCH 1/7] pps: Decouple N_PPS from N_TTY George Spelvin
@ 2013-02-08  6:06 ` George Spelvin
  2013-02-08  6:38 ` [PATCH 5/7] pps: Add pps_lookup_dev() function George Spelvin
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: George Spelvin @ 2013-02-08  6:06 UTC (permalink / raw)
  To: linux-serial, peter; +Cc: linux, linux-kernel, giometti

An extension of the previous commit, there is no semantic change
here, just 5 fewer lines of source code.

Signed-off-by: George Spelvin <linux@horizon.com>
---
 drivers/tty/serial/serial_core.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 8b6bff5..b3a204b 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2715,16 +2715,14 @@ EXPORT_SYMBOL(uart_match_port);
  */
 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 tty_port *port = &uport->state->port;
 	struct tty_struct *tty = port->tty;
+	struct tty_ldisc *ld = tty ? tty_ldisc_ref(tty) : NULL;
 
-	if (tty)
-	        ld = tty_ldisc_ref(tty);
-	if (ld && ld->ops->dcd_change) {
-		pps_get_ts(&ts);
-		ld->ops->dcd_change(tty, status);
+	if (ld) {
+		if (ld->ops->dcd_change)
+			ld->ops->dcd_change(tty, status);
+		tty_ldisc_deref(ld);
 	}
 
 	uport->icount.dcd++;
@@ -2739,9 +2737,6 @@ void uart_handle_dcd_change(struct uart_port *uport, unsigned int status)
 		else if (tty)
 			tty_hangup(tty);
 	}
-
-	if (ld)
-		tty_ldisc_deref(ld);
 }
 EXPORT_SYMBOL_GPL(uart_handle_dcd_change);
 
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 5/7] pps: Add pps_lookup_dev() function
  2013-02-08  7:05 [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731 George Spelvin
                   ` (3 preceding siblings ...)
  2013-02-08  6:06 ` [PATCH 2/7] pps: Additional cleanups in uart_handle_dcd_change George Spelvin
@ 2013-02-08  6:38 ` George Spelvin
  2013-02-08  6:48 ` [PATCH 6/7] pps: Use pps_lookup_dev to reduce ldisc coupling George Spelvin
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 11+ messages in thread
From: George Spelvin @ 2013-02-08  6:38 UTC (permalink / raw)
  To: linux-serial, peter; +Cc: linux, linux-kernel, giometti

The PPS serial line discipline wants to attach a PPS device to a tty
without changing the tty code to add a struct pps_device * pointer.

Since the number of PPS devices in a typical system is generally very low
(n=1 is by far the most common), it's practical to search the entire list
of allocated pps devices.  (We capture the timestamp before the lookup,
so the timing isn't affected.)

It is a bit ugly that this function, which is part of the in-kernel
PPS API, has to be in pps.c as opposed to kapi,c, but that's not
something that affects users.

Signed-off-by: George Spelvin <linux@horizon.com>
---
 drivers/pps/pps.c          | 35 +++++++++++++++++++++++++++++++++++
 include/linux/pps_kernel.h | 17 ++++++++++++++---
 2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 2420d5a..9a87565 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -357,6 +357,41 @@ void pps_unregister_cdev(struct pps_device *pps)
 }
 
 /*
+ * Look up a pps device by magic cookie.
+ * The cookie is usually a pointer to some enclosing device, but this
+ * code doesn't care; you should never be dereferencing it.
+ *
+ * This is a bit of a kludge that is currently used only by the PPS
+ * serial line discipline.  It may need to be tweaked when a second user
+ * is found.
+ *
+ * There is no function interface for setting the lookup_cookie field.
+ * It's initialized to NULL when the pps device is created, and if a
+ * client wants to use it, just fill it in afterward.
+ *
+ * A very important usage note is that the list used is the same as the
+ * list of assigned minor numbers.  Thus, a pps device can be looked up
+ * after pps_unregister_source time if there are open file handles preventing
+ * it from being deallocated.
+ *
+ * To ensure that this does not lead to duplicate cookies (which Would
+ * Be Bad), also set the cookie to NULL before unregistering.
+ */
+struct pps_device *pps_lookup_dev(void const *cookie)
+{
+	struct pps_device *pps;
+	unsigned id;
+
+	rcu_read_lock();
+	idr_for_each_entry(&pps_idr, pps, id)
+		if (cookie == pps->lookup_cookie)
+			break;
+	rcu_read_unlock();
+	return pps;
+}
+EXPORT_SYMBOL(pps_lookup_dev);
+
+/*
  * Module stuff
  */
 
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 0cc45ae..7db3eb9 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -43,7 +43,7 @@ struct pps_source_info {
 			int event, void *data);	/* PPS echo function */
 
 	struct module *owner;
-	struct device *dev;
+	struct device *dev;		/* Parent device for device_create */
 };
 
 struct pps_event_time {
@@ -69,6 +69,7 @@ struct pps_device {
 	wait_queue_head_t queue;		/* PPS event queue */
 
 	unsigned int id;			/* PPS source unique ID */
+	void const *lookup_cookie;		/* pps_lookup_dev only */
 	struct cdev cdev;
 	struct device *dev;
 	struct fasync_struct *async_queue;	/* fasync method */
@@ -82,16 +83,26 @@ struct pps_device {
 extern struct device_attribute pps_attrs[];
 
 /*
+ * Internal functions.
+ *
+ * These are not actually part of the exported API, but this is a
+ * convenient header file to put them in.
+ */
+
+extern int pps_register_cdev(struct pps_device *pps);
+extern void pps_unregister_cdev(struct pps_device *pps);
+
+/*
  * Exported functions
  */
 
 extern struct pps_device *pps_register_source(
 		struct pps_source_info *info, int default_params);
 extern void pps_unregister_source(struct pps_device *pps);
-extern int pps_register_cdev(struct pps_device *pps);
-extern void pps_unregister_cdev(struct pps_device *pps);
 extern void pps_event(struct pps_device *pps,
 		struct pps_event_time *ts, int event, void *data);
+/* Look up a pps device by magic cookie */
+struct pps_device *pps_lookup_dev(void const *cookie);
 
 static inline void timespec_to_pps_ktime(struct pps_ktime *kt,
 		struct timespec ts)
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 6/7] pps: Use pps_lookup_dev to reduce ldisc coupling
  2013-02-08  7:05 [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731 George Spelvin
                   ` (4 preceding siblings ...)
  2013-02-08  6:38 ` [PATCH 5/7] pps: Add pps_lookup_dev() function George Spelvin
@ 2013-02-08  6:48 ` George Spelvin
  2013-02-08  6:50 ` [PATCH 7/7] tty/tty_ldisc.c: use test_and_clear_bit in tty_ldisc_close George Spelvin
  2013-02-08 23:36 ` [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731 Greg KH
  7 siblings, 0 replies; 11+ messages in thread
From: George Spelvin @ 2013-02-08  6:48 UTC (permalink / raw)
  To: linux-serial, peter; +Cc: linux, linux-kernel, giometti

Now that N_TTY uses tty->disc_data for its private data,
'subclass' ldiscs cannot use ->disc_data for their own private data.
(This is a regression is v3.8-rc1)

Use pps_lookup_dev to associate the tty with the pps source instead.

Signed-off-by: George Spelvin <linux@horizon.com>
---
 drivers/pps/clients/pps-ldisc.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index e5a702a..5c22c5f 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -32,10 +32,12 @@
 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;
 
 	pps_get_ts(&ts);
 
+	pps = pps_lookup_dev(tty);
+
 	/*
 	 * This should never fail, but the ldisc locking is very
 	 * convoluted, so don't crash just in case.
@@ -75,9 +77,9 @@ 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;
+	pps->lookup_cookie = tty;
 
-	/* Should open N_TTY ldisc too */
+	/* Now open the base class N_TTY ldisc */
 	ret = alias_n_tty_open(tty);
 	if (ret < 0) {
 		pr_err("cannot open tty ldisc \"%s\"\n", info.path);
@@ -89,7 +91,7 @@ static int pps_tty_open(struct tty_struct *tty)
 	return 0;
 
 err_unregister:
-	tty->disc_data = NULL;
+	pps->lookup_cookie = NULL;
 	pps_unregister_source(pps);
 	return ret;
 }
@@ -98,12 +100,15 @@ 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 = pps_lookup_dev(tty);
 
 	alias_n_tty_close(tty);
 
-	tty->disc_data = NULL;
+	if (WARN_ON(!pps))
+		return;
+
 	dev_info(pps->dev, "removed\n");
+	pps->lookup_cookie = NULL;
 	pps_unregister_source(pps);
 }
 
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 7/7] tty/tty_ldisc.c: use test_and_clear_bit in tty_ldisc_close
  2013-02-08  7:05 [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731 George Spelvin
                   ` (5 preceding siblings ...)
  2013-02-08  6:48 ` [PATCH 6/7] pps: Use pps_lookup_dev to reduce ldisc coupling George Spelvin
@ 2013-02-08  6:50 ` George Spelvin
  2013-02-08 23:36 ` [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731 Greg KH
  7 siblings, 0 replies; 11+ messages in thread
From: George Spelvin @ 2013-02-08  6:50 UTC (permalink / raw)
  To: linux-serial, peter; +Cc: linux, linux-kernel, giometti

We have a function to test and clear a bit in one step, so use it.

Signed-off-by: George Spelvin <linux@horizon.com>
---
 drivers/tty/tty_ldisc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index c578229..4606ab9 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -453,8 +453,7 @@ static int tty_ldisc_open(struct tty_struct *tty, struct tty_ldisc *ld)
 
 static void tty_ldisc_close(struct tty_struct *tty, struct tty_ldisc *ld)
 {
-	WARN_ON(!test_bit(TTY_LDISC_OPEN, &tty->flags));
-	clear_bit(TTY_LDISC_OPEN, &tty->flags);
+	WARN_ON(!test_and_clear_bit(TTY_LDISC_OPEN, &tty->flags));
 	if (ld->ops->close)
 		ld->ops->close(tty);
 }
-- 
1.8.1.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731
@ 2013-02-08  7:05 George Spelvin
  2013-02-06 15:55 ` [PATCH 3/7] pps: Don't crash the machine when exiting will do Peter Hurley
                   ` (7 more replies)
  0 siblings, 8 replies; 11+ messages in thread
From: George Spelvin @ 2013-02-08  7:05 UTC (permalink / raw)
  To: linux-serial, peter; +Cc: linux, linux-kernel, giometti

The standard N_TTY line discipline used to not use the tty->disc_data
field, so N_PPS felt free to use it.  That has now changed, requiring
that N_PPS use a different method to find its private data.

(In the current, buggy, state, N_PPS follows a wild pointer and explodes
in an interrupt hander as soon as a pulse actually arrives.)

Strictly speaking, only patches 5 and 6 are the bugfix proper; the
rest are various cleanups enabled by the looser coupling of the
line disciplines.

George Spelvin (4):
  pps: Additional cleanups in uart_handle_dcd_change
  pps: Add pps_lookup_dev() function
  pps: Use pps_lookup_dev to reduce ldisc coupling
  tty/tty_ldisc.c: use test_and_clear_bit in tty_ldisc_close

Peter Hurley (3):
  pps: Decouple N_PPS from N_TTY
  pps: Don't crash the machine when exiting will do
  tty: Remove ancient hardpps()

 drivers/pps/clients/pps-ldisc.c     | 33 +++++++++++++++++++++++----------
 drivers/pps/pps.c                   | 35 +++++++++++++++++++++++++++++++++++
 drivers/staging/speakup/selection.c |  1 +
 drivers/tty/amiserial.c             |  5 -----
 drivers/tty/n_tty.c                 |  3 ++-
 drivers/tty/serial/serial_core.c    | 24 +++++++-----------------
 drivers/tty/tty_buffer.c            |  1 +
 drivers/tty/tty_ldisc.c             |  3 +--
 include/linux/pps_kernel.h          | 17 ++++++++++++++---
 include/linux/serial_core.h         |  1 -
 include/linux/tty_ldisc.h           | 11 ++++-------
 11 files changed, 88 insertions(+), 46 deletions(-)

-- 
1.8.1.2


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731
  2013-02-08  7:05 [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731 George Spelvin
                   ` (6 preceding siblings ...)
  2013-02-08  6:50 ` [PATCH 7/7] tty/tty_ldisc.c: use test_and_clear_bit in tty_ldisc_close George Spelvin
@ 2013-02-08 23:36 ` Greg KH
  2013-02-09  0:22   ` George Spelvin
  2013-02-09  7:05   ` George Spelvin
  7 siblings, 2 replies; 11+ messages in thread
From: Greg KH @ 2013-02-08 23:36 UTC (permalink / raw)
  To: George Spelvin; +Cc: linux-serial, peter, linux-kernel, giometti

On Fri, Feb 08, 2013 at 02:05:40AM -0500, George Spelvin wrote:
> The standard N_TTY line discipline used to not use the tty->disc_data
> field, so N_PPS felt free to use it.  That has now changed, requiring
> that N_PPS use a different method to find its private data.
> 
> (In the current, buggy, state, N_PPS follows a wild pointer and explodes
> in an interrupt hander as soon as a pulse actually arrives.)
> 
> Strictly speaking, only patches 5 and 6 are the bugfix proper; the
> rest are various cleanups enabled by the looser coupling of the
> line disciplines.

As this is a breakage in 3.8-rc1, is there any way to pull out patches 5
and 6 in a format that I can apply them now, or in a way that I can get
them into 3.8.1 so that this gets fixed?  I can't apply all of these to
3.8-final now, as it's way too late.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731
  2013-02-08 23:36 ` [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731 Greg KH
@ 2013-02-09  0:22   ` George Spelvin
  2013-02-09  7:05   ` George Spelvin
  1 sibling, 0 replies; 11+ messages in thread
From: George Spelvin @ 2013-02-09  0:22 UTC (permalink / raw)
  To: gregkh, linux; +Cc: giometti, linux-kernel, linux-serial, peter

> As this is a breakage in 3.8-rc1, is there any way to pull out patches 5
> and 6 in a format that I can apply them now, or in a way that I can get
> I can't apply all of these to 3.8-final now, as it's way too late.

I'll reorder them for you.  But Philipp will be bitterly disappointed. :-)

The other cleanups are far smaller and lower risk than the infrastructure
added in #5; I was just being OCD about committing the "least patchable
unit".

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731
  2013-02-08 23:36 ` [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731 Greg KH
  2013-02-09  0:22   ` George Spelvin
@ 2013-02-09  7:05   ` George Spelvin
  1 sibling, 0 replies; 11+ messages in thread
From: George Spelvin @ 2013-02-09  7:05 UTC (permalink / raw)
  To: gregkh, linux; +Cc: giometti, linux-kernel, linux-serial, peter

As a followup, testing my fixes has revealed an old bug in the
PPS driver that I'm trying to figure out how to solve.

Basically, pps_unregister_cdev does

        device_destroy(pps_class, pps->dev->devt);
        cdev_del(&pps->cdev);

And device_destroy ends up calling pps->dev->release,
which is pps_device_destruct, which does

        mutex_lock(&pps_idr_lock);
        idr_remove(&pps_idr, pps->id);
        mutex_unlock(&pps_idr_lock);

        kfree(dev);
        kfree(pps);

Now the problem is that the kfree(pps) happens *before* the
cdev_del(&pps->cdev) call, which is Not Good.

I'm trying to figure out The Right Thing to do in this case and include a
fix for that, too.  It's not a regression, but it is a fairly serious bug.

Advice gratefully received, but I'll figure it out on my own, if not.
The most obvious kludge is to wrap the pps_unregister_cdev operations
in device_get/device_put, to force the release callback to be delayed
until later.

I'm hoping for something prettier, though.  The other option I'm thinking
about is to move one or both deallocations to the (currently stub)
pps_cdev_release function.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2013-02-09  7:06 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-08  7:05 [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731 George Spelvin
2013-02-06 15:55 ` [PATCH 3/7] pps: Don't crash the machine when exiting will do Peter Hurley
2013-02-06 15:55 ` [PATCH 4/7] tty: Remove ancient hardpps() Peter Hurley
2013-02-08  5:45 ` [PATCH 1/7] pps: Decouple N_PPS from N_TTY George Spelvin
2013-02-08  6:06 ` [PATCH 2/7] pps: Additional cleanups in uart_handle_dcd_change George Spelvin
2013-02-08  6:38 ` [PATCH 5/7] pps: Add pps_lookup_dev() function George Spelvin
2013-02-08  6:48 ` [PATCH 6/7] pps: Use pps_lookup_dev to reduce ldisc coupling George Spelvin
2013-02-08  6:50 ` [PATCH 7/7] tty/tty_ldisc.c: use test_and_clear_bit in tty_ldisc_close George Spelvin
2013-02-08 23:36 ` [PATCH 0/7] 3.8-rc regression with pps-ldisc due to 70ece7a731 Greg KH
2013-02-09  0:22   ` George Spelvin
2013-02-09  7:05   ` George Spelvin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.