[PATCH v2 7/9] tty: Remove ancient hardpps()

[PATCH v2 7/9] 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>
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.3

[PATCH v2 9/9] tty/tty_ldisc.c: use test_and_clear_bit in tty_ldisc_close

[George Spelvin]

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.3

[PATCH v2 1/9] pps: Add pps_lookup_dev() function

[George Spelvin]

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          | 33 +++++++++++++++++++++++++++++++++
 include/linux/pps_kernel.h | 17 ++++++++++++++---
 2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 2420d5a..a70e384 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -352,11 +352,44 @@ free_idr:
 
 void pps_unregister_cdev(struct pps_device *pps)
 {
+	pps->lookup_cookie = NULL;
 	device_destroy(pps_class, pps->dev->devt);
 	cdev_del(&pps->cdev);
 }
 
 /*
+ * 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.
+ *
+ * The cookie is automatically set to NULL in pps_unregister_source()
+ * so that it will not be used again, even if the pps device cannot
+ * be removed from the idr due to pending references holding the minor
+ * number in use.
+ */
+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.3

[PATCH v2 2/9] pps: Use pps_lookup_dev 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.
(This is a regression is v3.8-rc1)

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

This fixes a crashing regression in 3.8-rc1.

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

diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index 79451f2..60cee9e 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -31,7 +31,7 @@
 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_lookup_dev(tty);
 
 	BUG_ON(pps == NULL);
 
@@ -67,9 +67,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);
@@ -81,7 +81,6 @@ static int pps_tty_open(struct tty_struct *tty)
 	return 0;
 
 err_unregister:
-	tty->disc_data = NULL;
 	pps_unregister_source(pps);
 	return ret;
 }
@@ -90,11 +89,10 @@ 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;
 	dev_info(pps->dev, "removed\n");
 	pps_unregister_source(pps);
 }
-- 
1.8.1.3

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

[George Spelvin]

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 | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index 60cee9e..a94f73e 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
 
@@ -33,7 +34,12 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
 {
 	struct pps_device *pps = pps_lookup_dev(tty);
 
-	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 :
@@ -93,6 +99,9 @@ static void pps_tty_close(struct tty_struct *tty)
 
 	alias_n_tty_close(tty);
 
+	if (WARN_ON(!pps))
+		return;
+
 	dev_info(pps->dev, "removed\n");
 	pps_unregister_source(pps);
 }
-- 
1.8.1.3

[PATCH v2 6/9] pps: Additional cleanups in uart_handle_dcd_change

[George Spelvin]

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

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

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 23fc494..b3a204b 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2715,15 +2715,15 @@ 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)
-		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++;
 #ifdef CONFIG_HARD_PPS
@@ -2737,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.3

[PATCH v2 5/9] pps: Move timestamp read into PPS code proper

[George Spelvin]

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.

The previous patches fixed the crashing.  This one reduces coupling further
by eliminating 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     | 11 +++++++----
 drivers/staging/speakup/selection.c |  1 +
 drivers/tty/n_tty.c                 |  3 ++-
 drivers/tty/serial/serial_core.c    |  5 +----
 drivers/tty/tty_buffer.c            |  1 +
 include/linux/serial_core.h         |  1 -
 include/linux/tty_ldisc.h           | 11 ++++-------
 7 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index a94f73e..73bd3bb 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -29,11 +29,14 @@
 
 #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_device *pps = pps_lookup_dev(tty);
+	struct pps_device *pps;
+	struct pps_event_time ts;
+
+	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.
@@ -42,7 +45,7 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status,
 		return;
 
 	/* 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..23fc494 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2718,13 +2718,12 @@ 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);
+		ld->ops->dcd_change(tty, status);
 
 	uport->icount.dcd++;
 #ifdef CONFIG_HARD_PPS
@@ -2739,8 +2738,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.3

[PATCH v2 8/9] pps: Use a single cdev

[George Spelvin]

One per device just seems wasteful, when we already manintain a
data structure to map minor numbers to devices, and we already have
a PPS_MAX_SOURCES #define.

This is also a more comprehensive fix to the use-after-free bug
that has already received a minimal patch.
---
 drivers/pps/pps.c          | 66 ++++++++++++++++++++++++----------------------
 include/linux/pps_kernel.h |  1 -
 2 files changed, 34 insertions(+), 33 deletions(-)

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 6437703..754b0b5 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -41,6 +41,8 @@
 
 static dev_t pps_devt;
 static struct class *pps_class;
+static struct cdev pps_cdev;
+
 
 static DEFINE_MUTEX(pps_idr_lock);
 static DEFINE_IDR(pps_idr);
@@ -244,17 +246,23 @@ static long pps_cdev_ioctl(struct file *file,
 
 static int pps_cdev_open(struct inode *inode, struct file *file)
 {
-	struct pps_device *pps = container_of(inode->i_cdev,
-						struct pps_device, cdev);
-	file->private_data = pps;
-	kobject_get(&pps->dev->kobj);
-	return 0;
+	int err = -ENXIO;
+	struct pps_device *pps;
+
+	rcu_read_lock();
+	pps = idr_find(&pps_idr, iminor(inode));
+	if (pps) {
+		file->private_data = pps;
+		kobject_get(&pps->dev->kobj);
+		err = 0;
+	}
+	rcu_read_unlock();
+	return err;
 }
 
 static int pps_cdev_release(struct inode *inode, struct file *file)
 {
-	struct pps_device *pps = container_of(inode->i_cdev,
-						struct pps_device, cdev);
+	struct pps_device *pps = file->private_data;
 	kobject_put(&pps->dev->kobj);
 	return 0;
 }
@@ -277,8 +285,6 @@ static void pps_device_destruct(struct device *dev)
 {
 	struct pps_device *pps = dev_get_drvdata(dev);
 
-	cdev_del(&pps->cdev);
-
 	/* Now we can release the ID for re-use */
 	pr_debug("deallocating pps%d\n", pps->id);
 	mutex_lock(&pps_idr_lock);
@@ -295,17 +301,14 @@ int pps_register_cdev(struct pps_device *pps)
 	dev_t devt;
 
 	mutex_lock(&pps_idr_lock);
-	/* Get new ID for the new PPS source */
-	if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0) {
-		mutex_unlock(&pps_idr_lock);
-		return -ENOMEM;
-	}
-
-	/* Now really allocate the PPS source.
+	/* Get new ID for the new PPS source.
 	 * After idr_get_new() calling the new source will be freely available
 	 * into the kernel.
 	 */
-	err = idr_get_new(&pps_idr, pps, &pps->id);
+	if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0)
+		err = -ENOMEM;
+	else
+		err = idr_get_new(&pps_idr, pps, &pps->id);
 	mutex_unlock(&pps_idr_lock);
 
 	if (err < 0)
@@ -321,33 +324,21 @@ int pps_register_cdev(struct pps_device *pps)
 
 	devt = MKDEV(MAJOR(pps_devt), pps->id);
 
-	cdev_init(&pps->cdev, &pps_cdev_fops);
-	pps->cdev.owner = pps->info.owner;
-
-	err = cdev_add(&pps->cdev, devt, 1);
-	if (err) {
-		pr_err("%s: failed to add char device %d:%d\n",
-				pps->info.name, MAJOR(pps_devt), pps->id);
-		goto free_idr;
-	}
 	pps->dev = device_create(pps_class, pps->info.dev, devt, pps,
 							"pps%d", pps->id);
 	if (IS_ERR(pps->dev)) {
 		err = PTR_ERR(pps->dev);
-		goto del_cdev;
+		goto free_idr;
 	}
 
 	/* Override the release function with our own */
 	pps->dev->release = pps_device_destruct;
 
 	pr_debug("source %s got cdev (%d:%d)\n", pps->info.name,
-			MAJOR(pps_devt), pps->id);
+			MAJOR(devt), MINOR(devt));
 
 	return 0;
 
-del_cdev:
-	cdev_del(&pps->cdev);
-
 free_idr:
 	mutex_lock(&pps_idr_lock);
 	idr_remove(&pps_idr, pps->id);
@@ -401,8 +392,9 @@ EXPORT_SYMBOL(pps_lookup_dev);
 
 static void __exit pps_exit(void)
 {
-	class_destroy(pps_class);
+	cdev_del(&pps_cdev);
 	unregister_chrdev_region(pps_devt, PPS_MAX_SOURCES);
+	class_destroy(pps_class);
 }
 
 static int __init pps_init(void)
@@ -422,12 +414,22 @@ static int __init pps_init(void)
 		goto remove_class;
 	}
 
+	cdev_init(&pps_cdev, &pps_cdev_fops);
+	pps_cdev.owner = THIS_MODULE;
+	err = cdev_add(&pps_cdev, pps_devt, PPS_MAX_SOURCES);
+	if (err < 0) {
+		pr_err("failed to register struct cdev\n");
+		goto remove_region;
+	}
+
 	pr_info("LinuxPPS API ver. %d registered\n", PPS_API_VERS);
 	pr_info("Software ver. %s - Copyright 2005-2007 Rodolfo Giometti "
 		"<giometti@linux.it>\n", PPS_VERSION);
 
 	return 0;
 
+remove_region:
+	unregister_chrdev_region(pps_devt, PPS_MAX_SOURCES);
 remove_class:
 	class_destroy(pps_class);
 
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 7db3eb9..caca565 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -70,7 +70,6 @@ struct pps_device {
 
 	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 */
 	spinlock_t lock;
-- 
1.8.1.3

[PATCH v2 3/9] pps: Fix a use-after free bug when unregistering a source.

[George Spelvin]

Remove the cdev from the system (with cdev_del) *before* deallocating it
(in pps_device_destruct, called via kobject_put from device_destroy).

Also prevent deallocating a device with open file handles.

A better long-term fix is probably to remove the cdev from the pps_device
entirely, and instead have all devices reference one global cdev.  Then
the deallocation ordering becomes simpler.

But that's more complex and invasive change, so we leave that
for later.

Signed-off-by: George Spelvin <linux@horizon.com>
Cc: stable@kernel.org
---
 drivers/pps/pps.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index a70e384..6437703 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -247,12 +247,15 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
 	struct pps_device *pps = container_of(inode->i_cdev,
 						struct pps_device, cdev);
 	file->private_data = pps;
-
+	kobject_get(&pps->dev->kobj);
 	return 0;
 }
 
 static int pps_cdev_release(struct inode *inode, struct file *file)
 {
+	struct pps_device *pps = container_of(inode->i_cdev,
+						struct pps_device, cdev);
+	kobject_put(&pps->dev->kobj);
 	return 0;
 }
 
@@ -274,8 +277,10 @@ static void pps_device_destruct(struct device *dev)
 {
 	struct pps_device *pps = dev_get_drvdata(dev);
 
-	/* release id here to protect others from using it while it's
-	 * still in use */
+	cdev_del(&pps->cdev);
+
+	/* Now we can release the ID for re-use */
+	pr_debug("deallocating pps%d\n", pps->id);
 	mutex_lock(&pps_idr_lock);
 	idr_remove(&pps_idr, pps->id);
 	mutex_unlock(&pps_idr_lock);
@@ -332,6 +337,7 @@ int pps_register_cdev(struct pps_device *pps)
 		goto del_cdev;
 	}
 
+	/* Override the release function with our own */
 	pps->dev->release = pps_device_destruct;
 
 	pr_debug("source %s got cdev (%d:%d)\n", pps->info.name,
@@ -352,9 +358,9 @@ free_idr:
 
 void pps_unregister_cdev(struct pps_device *pps)
 {
+	pr_debug("unregistering pps%d\n", pps->id);
 	pps->lookup_cookie = NULL;
 	device_destroy(pps_class, pps->dev->devt);
-	cdev_del(&pps->cdev);
 }
 
 /*
-- 
1.8.1.3

[PATCH v2 0/9] 3.8-rc regression with pps-ldisc due to 70ece7a731

[George Spelvin]

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.)

Compared to v1, this has been rearranges to the first three patches are
the minimial bugfixes:

* 1/9 "Add pps_lookup_dev() function"
  This adds the infrastructure necessary to bypass disc_data use.

* 2/9 "Use pps_lookup_dev to reduce ldisc coupling"
  This actually fixes the bug.

* 3/9 "Fix a use-after free bug when unregistering a source."
  This is actually an old bug, present before 3.7.  I'd like to solicit
  feedback from folks who know device drivers better to ask if I did
  things right.  I'd also appreciate a look at patch 8/9 which is a more
  aggressive cleanup of the same bug.

The remaining ones are various cleanups enabled or inspired by the
above.

* 4/9 "Don't crash the machine when exiting will do"
  This downgrades some BUG() checks do future bugs will be less deadly.
* 5/9 "Move timestamp read into PPS code proper"
  This changes the internal kernel DCD change interface to capture the
  timestamp inside the called function, cleaning up the generic tty code.
* 6/9 "Additional cleanups in uart_handle_dcd_change"
  Just a slight code reorganization to shrink the source.
* 7/9 "Remove ancient hardpps()"
  Eliminate dead code.
* 8/9 "Use a single cdev"
  This is a more ambitious overhaul of the cdev business that patch
  #3 fixes.
* 9/9 "use test_and_clear_bit in tty_ldisc_close"
  Just a two-liner I noticed while working on the above.

George Spelvin (8):
  pps: Add pps_lookup_dev() function
  pps: Use pps_lookup_dev to reduce ldisc coupling
  pps: Fix a use-after free bug when unregistering a source.
  pps: Additional cleanups in uart_handle_dcd_change
  pps: Use a single cdev
  tty/tty_ldisc.c: use test_and_clear_bit in tty_ldisc_close

Peter Hurley (1):
  pps: Move timestamp read into PPS code proper
  pps: Don't crash the machine when exiting will do
  tty: Remove ancient hardpps()

 drivers/pps/clients/pps-ldisc.c     |  30 +++++++----
 drivers/pps/pps.c                   | 103 +++++++++++++++++++++++++-----------
 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          |  18 +++++--
 include/linux/serial_core.h         |   1 -
 include/linux/tty_ldisc.h           |  11 ++--
 11 files changed, 122 insertions(+), 78 deletions(-)

-- 
1.8.1.3

Re: [PATCH v2 0/9] 3.8-rc regression with pps-ldisc due to 70ece7a731

[Greg KH]

On Tue, Feb 12, 2013 at 08:56:07AM -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.)
> 
> Compared to v1, this has been rearranges to the first three patches are
> the minimial bugfixes:
> 
> * 1/9 "Add pps_lookup_dev() function"
>   This adds the infrastructure necessary to bypass disc_data use.
> 
> * 2/9 "Use pps_lookup_dev to reduce ldisc coupling"
>   This actually fixes the bug.
> 
> * 3/9 "Fix a use-after free bug when unregistering a source."
>   This is actually an old bug, present before 3.7.  I'd like to solicit
>   feedback from folks who know device drivers better to ask if I did
>   things right.  I'd also appreciate a look at patch 8/9 which is a more
>   aggressive cleanup of the same bug.

I can take these through my tty tree, but it would be very good if I
actually had the ack from the PPS maintainer...

Rodolfo, any objection for me taking these?

thanks,

greg k-h

Re: [PATCH v2 0/9] 3.8-rc regression with pps-ldisc due to 70ece7a731

[Rodolfo Giometti]

On Wed, Feb 13, 2013 at 08:45:47AM -0800, Greg KH wrote:
> 
> I can take these through my tty tree, but it would be very good if I
> actually had the ack from the PPS maintainer...
> 
> Rodolfo, any objection for me taking these?

No. It's ok for me. If you wish you can add my «Acked-by» sign.

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

Re: [PATCH v2 0/9] 3.8-rc regression with pps-ldisc due to 70ece7a731

[Rodolfo Giometti]

On Wed, Feb 13, 2013 at 08:45:47AM -0800, Greg KH wrote:
> 
> I can take these through my tty tree, but it would be very good if I
> actually had the ack from the PPS maintainer...
> 
> Rodolfo, any objection for me taking these?

No. It's ok for me. If you wish you can add my «Acked-by» sign.

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-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/9] pps: Move timestamp read into PPS code proper

[Greg KH]

On Tue, Feb 12, 2013 at 02:00:43AM -0500, George Spelvin wrote:
> 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.
> 
> The previous patches fixed the crashing.  This one reduces coupling further
> by eliminating 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

I also had to fix up:
	drivers/staging/dgrp/dgrp_*.c

to get things to build properly.

thanks,

greg k-h

Re: [PATCH v2 8/9] pps: Use a single cdev

[Greg KH]

On Tue, Feb 12, 2013 at 02:02:42AM -0500, George Spelvin wrote:
> One per device just seems wasteful, when we already manintain a
> data structure to map minor numbers to devices, and we already have
> a PPS_MAX_SOURCES #define.
> 
> This is also a more comprehensive fix to the use-after-free bug
> that has already received a minimal patch.
> ---
>  drivers/pps/pps.c          | 66 ++++++++++++++++++++++++----------------------
>  include/linux/pps_kernel.h |  1 -
>  2 files changed, 34 insertions(+), 33 deletions(-)

You forgot a Signed-off-by: line for this patch, so I can't apply it, or
the 9/9 patch :(

Care to resend just these two after fixing this up?

thanks,

greg k-h

Re: [PATCH v2 8/9] pps: Use a single cdev

[George Spelvin]

> You forgot a Signed-off-by: line for this patch, so I can't apply it, or
> the 9/9 patch :(

Oops, fixed.  I don't see why the 9/9 patch depends on it,
though.  They're not related or interdependent in any way.

If you want to check the logic, I'd appreciate it.  I'm not
really sure about the RCU stuff.  My understanding is that:
- the idr code does the appropriate write locking when
  modifying itself, so I don't need to do any.
- The pps_device returned from idr_find is itself refcounted,
  so it can't go away, and the accesses don't have bo be
  inside the RCU read "lock".  It's only the IDR's internal
  index nodes that might get reallocated by modificaitons of
  a different part of the tree.

> Care to resend just these two after fixing this up?

I can, but if you think you need 9/9 resent (which *did* have a S-o-b),
I'm confused and wonder why...

Re: [PATCH v2 8/9] pps: Use a single cdev

[Greg KH]

On Wed, Feb 13, 2013 at 01:35:29PM -0500, George Spelvin wrote:
> > You forgot a Signed-off-by: line for this patch, so I can't apply it, or
> > the 9/9 patch :(
> 
> Oops, fixed.  I don't see why the 9/9 patch depends on it,
> though.  They're not related or interdependent in any way.
> 
> If you want to check the logic, I'd appreciate it.  I'm not
> really sure about the RCU stuff.  My understanding is that:
> - the idr code does the appropriate write locking when
>   modifying itself, so I don't need to do any.
> - The pps_device returned from idr_find is itself refcounted,
>   so it can't go away, and the accesses don't have bo be
>   inside the RCU read "lock".  It's only the IDR's internal
>   index nodes that might get reallocated by modificaitons of
>   a different part of the tree.
> 
> > Care to resend just these two after fixing this up?
> 
> I can, but if you think you need 9/9 resent (which *did* have a S-o-b),
> I'm confused and wonder why...

I stopped at that point in the series, that's the only reason why, I
didn't "check" to see if there was a dependancy, I just assumed there
was...

So please resend, thanks.

greg k-h

Re: [PATCH v2 8/9] pps: Use a single cdev

[Peter Hurley]

On Tue, 2013-02-12 at 02:02 -0500, George Spelvin wrote:
> One per device just seems wasteful, when we already manintain a
> data structure to map minor numbers to devices, and we already have
> a PPS_MAX_SOURCES #define.
> 
> This is also a more comprehensive fix to the use-after-free bug
> that has already received a minimal patch.
> ---
>  drivers/pps/pps.c          | 66 ++++++++++++++++++++++++----------------------
>  include/linux/pps_kernel.h |  1 -
>  2 files changed, 34 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> index 6437703..754b0b5 100644
> --- a/drivers/pps/pps.c
> +++ b/drivers/pps/pps.c
> @@ -41,6 +41,8 @@
>  
>  static dev_t pps_devt;
>  static struct class *pps_class;
> +static struct cdev pps_cdev;
> +
>  
>  static DEFINE_MUTEX(pps_idr_lock);
>  static DEFINE_IDR(pps_idr);
> @@ -244,17 +246,23 @@ static long pps_cdev_ioctl(struct file *file,
>  
>  static int pps_cdev_open(struct inode *inode, struct file *file)
>  {
> -	struct pps_device *pps = container_of(inode->i_cdev,
> -						struct pps_device, cdev);
> -	file->private_data = pps;
> -	kobject_get(&pps->dev->kobj);
> -	return 0;
> +	int err = -ENXIO;
> +	struct pps_device *pps;
> +
> +	rcu_read_lock();
> +	pps = idr_find(&pps_idr, iminor(inode));
> +	if (pps) {
> +		file->private_data = pps;
> +		kobject_get(&pps->dev->kobj);
> +		err = 0;
> +	}
> +	rcu_read_unlock();

This should be:
	rcu_read_lock();
	pps = idr_find(&pps_idr, iminor(inode));
	rcu_read_unlock();
	if (pps) {
		file->private_data = pps;
		kobject_get(&pps->dev->kobj);
		err = 0;
	}

It's only the internal structures of idr that need rcu barriers.

> +	return err;
>  }
>  
>  static int pps_cdev_release(struct inode *inode, struct file *file)
>  {
> -	struct pps_device *pps = container_of(inode->i_cdev,
> -						struct pps_device, cdev);
> +	struct pps_device *pps = file->private_data;
>  	kobject_put(&pps->dev->kobj);
>  	return 0;
>  }
> @@ -277,8 +285,6 @@ static void pps_device_destruct(struct device *dev)
>  {
>  	struct pps_device *pps = dev_get_drvdata(dev);
>  
> -	cdev_del(&pps->cdev);
> -
>  	/* Now we can release the ID for re-use */
>  	pr_debug("deallocating pps%d\n", pps->id);
>  	mutex_lock(&pps_idr_lock);
> @@ -295,17 +301,14 @@ int pps_register_cdev(struct pps_device *pps)
>  	dev_t devt;
>  
>  	mutex_lock(&pps_idr_lock);
> -	/* Get new ID for the new PPS source */
> -	if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0) {
> -		mutex_unlock(&pps_idr_lock);
> -		return -ENOMEM;
> -	}
> -
> -	/* Now really allocate the PPS source.
> +	/* Get new ID for the new PPS source.
>  	 * After idr_get_new() calling the new source will be freely available
>  	 * into the kernel.
>  	 */
> -	err = idr_get_new(&pps_idr, pps, &pps->id);
> +	if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0)
> +		err = -ENOMEM;
> +	else
> +		err = idr_get_new(&pps_idr, pps, &pps->id);

Your maintainer should be letting you know about this:

-------- Forwarded Message --------
From: Tejun Heo <tj@kernel.org>
To: akpm@linux-foundation.org
Cc: linux-kernel@vger.kernel.org, rusty@rustcorp.com.au, bfields@fieldses.org,
skinsbursky@parallels.com, ebiederm@xmission.com, jmorris@namei.org, axboe@kernel.dk,
Tejun Heo <tj@kernel.org>, Rodolfo Giometti <giometti@enneenne.com>
Subject: [PATCH 41/62] pps: convert to idr_alloc()
Date: Sat, 2 Feb 2013 17:20:42 -0800

Convert to the much saner new idr interface.

Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Rodolfo Giometti <giometti@enneenne.com>
---
This patch depends on an earlier idr changes and I think it would be
best to route these together through -mm.  Please holler if there's
any objection.  Thanks.

 drivers/pps/kapi.c |  2 +-
 drivers/pps/pps.c  | 36 ++++++++++++++----------------------
 2 files changed, 15 insertions(+), 23 deletions(-)

diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index f197e8e..cdad4d9 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -102,7 +102,7 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
 		goto pps_register_source_exit;
 	}
 
-	/* These initializations must be done before calling idr_get_new()
+	/* These initializations must be done before calling idr_alloc()
 	 * in order to avoid reces into pps_event().
 	 */
 	pps->params.api_version = PPS_API_VERS;
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 2420d5a..de8e663 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -290,29 +290,21 @@ int pps_register_cdev(struct pps_device *pps)
 	dev_t devt;
 
 	mutex_lock(&pps_idr_lock);
-	/* Get new ID for the new PPS source */
-	if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0) {
-		mutex_unlock(&pps_idr_lock);
-		return -ENOMEM;
-	}
-
-	/* Now really allocate the PPS source.
-	 * After idr_get_new() calling the new source will be freely available
-	 * into the kernel.
+	/*
+	 * Get new ID for the new PPS source.  After idr_alloc() calling
+	 * the new source will be freely available into the kernel.
 	 */
-	err = idr_get_new(&pps_idr, pps, &pps->id);
-	mutex_unlock(&pps_idr_lock);
-
-	if (err < 0)
-		return err;
-
-	pps->id &= MAX_IDR_MASK;
-	if (pps->id >= PPS_MAX_SOURCES) {
-		pr_err("%s: too many PPS sources in the system\n",
-					pps->info.name);
-		err = -EBUSY;
-		goto free_idr;
+	err = idr_alloc(&pps_idr, pps, 0, PPS_MAX_SOURCES, GFP_KERNEL);
+	if (err < 0) {
+		if (err == -ENOSPC) {
+			pr_err("%s: too many PPS sources in the system\n",
+			       pps->info.name);
+			err = -EBUSY;
+		}
+		goto out_unlock;
 	}
+	pps->id = err;
+	mutex_unlock(&pps_idr_lock);
 
 	devt = MKDEV(MAJOR(pps_devt), pps->id);
 
@@ -345,8 +337,8 @@ del_cdev:
 free_idr:
 	mutex_lock(&pps_idr_lock);
 	idr_remove(&pps_idr, pps->id);
+out_unlock:
 	mutex_unlock(&pps_idr_lock);
-
 	return err;
 }
 
-- 
1.8.1


>  	mutex_unlock(&pps_idr_lock);
>  
>  	if (err < 0)
> @@ -321,33 +324,21 @@ int pps_register_cdev(struct pps_device *pps)
>  
>  	devt = MKDEV(MAJOR(pps_devt), pps->id);
>  
> -	cdev_init(&pps->cdev, &pps_cdev_fops);
> -	pps->cdev.owner = pps->info.owner;
> -
> -	err = cdev_add(&pps->cdev, devt, 1);
> -	if (err) {
> -		pr_err("%s: failed to add char device %d:%d\n",
> -				pps->info.name, MAJOR(pps_devt), pps->id);
> -		goto free_idr;
> -	}
>  	pps->dev = device_create(pps_class, pps->info.dev, devt, pps,
>  							"pps%d", pps->id);
>  	if (IS_ERR(pps->dev)) {
>  		err = PTR_ERR(pps->dev);
> -		goto del_cdev;
> +		goto free_idr;
>  	}
>  
>  	/* Override the release function with our own */
>  	pps->dev->release = pps_device_destruct;
>  
>  	pr_debug("source %s got cdev (%d:%d)\n", pps->info.name,
> -			MAJOR(pps_devt), pps->id);
> +			MAJOR(devt), MINOR(devt));
>  
>  	return 0;
>  
> -del_cdev:
> -	cdev_del(&pps->cdev);
> -
>  free_idr:
>  	mutex_lock(&pps_idr_lock);
>  	idr_remove(&pps_idr, pps->id);
> @@ -401,8 +392,9 @@ EXPORT_SYMBOL(pps_lookup_dev);
>  
>  static void __exit pps_exit(void)
>  {
> -	class_destroy(pps_class);
> +	cdev_del(&pps_cdev);
>  	unregister_chrdev_region(pps_devt, PPS_MAX_SOURCES);
> +	class_destroy(pps_class);
>  }
>  
>  static int __init pps_init(void)
> @@ -422,12 +414,22 @@ static int __init pps_init(void)
>  		goto remove_class;
>  	}
>  
> +	cdev_init(&pps_cdev, &pps_cdev_fops);
> +	pps_cdev.owner = THIS_MODULE;
> +	err = cdev_add(&pps_cdev, pps_devt, PPS_MAX_SOURCES);
> +	if (err < 0) {
> +		pr_err("failed to register struct cdev\n");
> +		goto remove_region;
> +	}
> +
>  	pr_info("LinuxPPS API ver. %d registered\n", PPS_API_VERS);
>  	pr_info("Software ver. %s - Copyright 2005-2007 Rodolfo Giometti "
>  		"<giometti@linux.it>\n", PPS_VERSION);
>  
>  	return 0;
>  
> +remove_region:
> +	unregister_chrdev_region(pps_devt, PPS_MAX_SOURCES);
>  remove_class:
>  	class_destroy(pps_class);
>  
> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> index 7db3eb9..caca565 100644
> --- a/include/linux/pps_kernel.h
> +++ b/include/linux/pps_kernel.h
> @@ -70,7 +70,6 @@ struct pps_device {
>  
>  	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 */
>  	spinlock_t lock;