[GIT PATCH] TTY patches for 2.6.31-rc5-git

[GIT PATCH] TTY patches for 2.6.31-rc5-git

[Greg KH]

Here are some the TTY ldisc patches for your kernel tree.

As you wrote these, I think you know what they fix :)

Please pull from:
	master.kernel.org:/pub/scm/linux/kernel/git/gregkh/tty-2.6.git/

The full patches will be sent to the linux-kernel mailing list, if anyone
wants to see them.

thanks,

greg k-h

------------

 drivers/char/tty_ldisc.c  |  152 ++++++++++++++++-----------------------------
 include/linux/tty_ldisc.h |    2 +-
 2 files changed, 54 insertions(+), 100 deletions(-)

---------------

Linus Torvalds (3):
      tty-ldisc: make refcount be atomic_t 'users' count
      tty-ldisc: turn ldisc user count into a proper refcount
      tty-ldisc: be more careful in 'put_ldisc' locking

[PATCH 1/3] tty-ldisc: make refcount be atomic_t 'users' count

[Greg Kroah-Hartman]

From: Linus Torvalds <torvalds@linux-foundation.org>

This is pure preparation of changing the ldisc reference counting to be
a true refcount that defines the lifetime of the ldisc.  But this is a
purely syntactic change for now to make the next steps easier.

This patch should make no semantic changes at all. But I wanted to make
the ldisc refcount be an atomic (I will be touching it without locks
soon enough), and I wanted to rename it so that there isn't quite as
much confusion between 'ldo->refcount' (ldisk operations refcount) and
'ld->refcount' (ldisc refcount itself) in the same file.

So it's now an atomic 'ld->users' count. It still starts at zero,
despite having a reference from 'tty->ldisc', but that will change once
we turn it into a _real_ refcount.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Tested-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Tested-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
Acked-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/char/tty_ldisc.c  |   18 ++++++++----------
 include/linux/tty_ldisc.h |    2 +-
 2 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/char/tty_ldisc.c b/drivers/char/tty_ldisc.c
index acd76b7..fd175e6 100644
--- a/drivers/char/tty_ldisc.c
+++ b/drivers/char/tty_ldisc.c
@@ -142,7 +142,7 @@ static struct tty_ldisc *tty_ldisc_try_get(int disc)
 			/* lock it */
 			ldops->refcount++;
 			ld->ops = ldops;
-			ld->refcount = 0;
+			atomic_set(&ld->users, 0);
 			err = 0;
 		}
 	}
@@ -206,7 +206,7 @@ static void tty_ldisc_put(struct tty_ldisc *ld)
 	ldo->refcount--;
 	module_put(ldo->owner);
 	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
-	WARN_ON(ld->refcount);
+	WARN_ON(atomic_read(&ld->users));
 	kfree(ld);
 }
 
@@ -297,7 +297,7 @@ static int tty_ldisc_try(struct tty_struct *tty)
 	spin_lock_irqsave(&tty_ldisc_lock, flags);
 	ld = tty->ldisc;
 	if (test_bit(TTY_LDISC, &tty->flags)) {
-		ld->refcount++;
+		atomic_inc(&ld->users);
 		ret = 1;
 	}
 	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
@@ -324,7 +324,7 @@ struct tty_ldisc *tty_ldisc_ref_wait(struct tty_struct *tty)
 {
 	/* wait_event is a macro */
 	wait_event(tty_ldisc_wait, tty_ldisc_try(tty));
-	WARN_ON(tty->ldisc->refcount == 0);
+	WARN_ON(atomic_read(&tty->ldisc->users) == 0);
 	return tty->ldisc;
 }
 EXPORT_SYMBOL_GPL(tty_ldisc_ref_wait);
@@ -365,11 +365,9 @@ void tty_ldisc_deref(struct tty_ldisc *ld)
 	BUG_ON(ld == NULL);
 
 	spin_lock_irqsave(&tty_ldisc_lock, flags);
-	if (ld->refcount == 0)
+	if (atomic_read(&ld->users) == 0)
 		printk(KERN_ERR "tty_ldisc_deref: no references.\n");
-	else
-		ld->refcount--;
-	if (ld->refcount == 0)
+	else if (atomic_dec_and_test(&ld->users))
 		wake_up(&tty_ldisc_wait);
 	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
 }
@@ -536,10 +534,10 @@ static int tty_ldisc_wait_idle(struct tty_struct *tty)
 {
 	unsigned long flags;
 	spin_lock_irqsave(&tty_ldisc_lock, flags);
-	while (tty->ldisc->refcount) {
+	while (atomic_read(&tty->ldisc->users)) {
 		spin_unlock_irqrestore(&tty_ldisc_lock, flags);
 		if (wait_event_timeout(tty_ldisc_wait,
-				tty->ldisc->refcount == 0, 5 * HZ) == 0)
+				atomic_read(&tty->ldisc->users) == 0, 5 * HZ) == 0)
 			return -EBUSY;
 		spin_lock_irqsave(&tty_ldisc_lock, flags);
 	}
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index 40f38d8..0c4ee9b 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -144,7 +144,7 @@ struct tty_ldisc_ops {
 
 struct tty_ldisc {
 	struct tty_ldisc_ops *ops;
-	int refcount;
+	atomic_t users;
 };
 
 #define TTY_LDISC_MAGIC	0x5403
-- 
1.6.3.2

[PATCH 2/3] tty-ldisc: turn ldisc user count into a proper refcount

[Greg Kroah-Hartman]

From: Linus Torvalds <torvalds@linux-foundation.org>

By using the user count for the actual lifetime rules, we can get rid of
the silly "wait_for_idle" logic, because any busy ldisc will
automatically stay around until the last user releases it.  This avoids
a host of odd issues, and simplifies the code.

So now, when the last ldisc reference is dropped, we just release the
ldisc operations struct reference, and free the ldisc.

It looks obvious enough, and it does work for me, but the counting
_could_ be off. It probably isn't (bad counting in the new version would
generally imply that the old code did something really bad, like free an
ldisc with a non-zero count), but it does need some testing, and
preferably somebody looking at it.

With this change, both 'tty_ldisc_put()' and 'tty_ldisc_deref()' are
just aliases for the new ref-counting 'put_ldisc()'. Both of them
decrement the ldisc user count and free it if it goes down to zero.
They're identical functions, in other words.

But the reason they still exist as sepate functions is that one of them
was exported (tty_ldisc_deref) and had a stupid name (so I don't want to
use it as the main name), and the other one was used in multiple places
(and I didn't want to make the patch larger just to rename the users).

In addition to the refcounting, I did do some minimal cleanup. For
example, now "tty_ldisc_try()" actually returns the ldisc it got under
the lock, rather than returning true/false and then the caller would
look up the ldisc again (now without the protection of the lock).

That said, there's tons of dubious use of 'tty->ldisc' without obviously
proper locking or refcounting left. I expressly did _not_ want to try to
fix it all, keeping the patch minimal. There may or may not be bugs in
that kind of code, but they wouldn't be _new_ bugs.

That said, even if the bugs aren't new, the timing and lifetime will
change. For example, some silly code may depend on the 'tty->ldisc'
pointer not changing because they hold a refcount on the 'ldisc'. And
that's no longer true - if you hold a ref on the ldisc, the 'ldisc'
itself is safe, but tty->ldisc may change.

So the proper locking (remains) to hold tty->ldisc_mutex if you expect
tty->ldisc to be stable. That's not really a _new_ rule, but it's an
example of something that the old code might have unintentionally
depended on and hidden bugs.

Whatever. The patch _looks_ sensible to me. The only users of
ldisc->users are:
 - get_ldisc() - atomically increment the count

 - put_ldisc() - atomically decrements the count and releases if zero

 - tty_ldisc_try_get() - creates the ldisc, and sets the count to 1.
   The ldisc should then either be released, or be attached to a tty.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Tested-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Tested-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
Acked-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/char/tty_ldisc.c |  143 +++++++++++++++-------------------------------
 1 files changed, 46 insertions(+), 97 deletions(-)

diff --git a/drivers/char/tty_ldisc.c b/drivers/char/tty_ldisc.c
index fd175e6..be55dfc 100644
--- a/drivers/char/tty_ldisc.c
+++ b/drivers/char/tty_ldisc.c
@@ -48,6 +48,34 @@ static DECLARE_WAIT_QUEUE_HEAD(tty_ldisc_wait);
 /* Line disc dispatch table */
 static struct tty_ldisc_ops *tty_ldiscs[NR_LDISCS];
 
+static inline struct tty_ldisc *get_ldisc(struct tty_ldisc *ld)
+{
+	if (ld)
+		atomic_inc(&ld->users);
+	return ld;
+}
+
+static inline void put_ldisc(struct tty_ldisc *ld)
+{
+	if (WARN_ON_ONCE(!ld))
+		return;
+
+	/*
+	 * If this is the last user, free the ldisc, and
+	 * release the ldisc ops.
+	 */
+	if (atomic_dec_and_test(&ld->users)) {
+		unsigned long flags;
+		struct tty_ldisc_ops *ldo = ld->ops;
+
+		kfree(ld);
+		spin_lock_irqsave(&tty_ldisc_lock, flags);
+		ldo->refcount--;
+		module_put(ldo->owner);
+		spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+	}
+}
+
 /**
  *	tty_register_ldisc	-	install a line discipline
  *	@disc: ldisc number
@@ -142,7 +170,7 @@ static struct tty_ldisc *tty_ldisc_try_get(int disc)
 			/* lock it */
 			ldops->refcount++;
 			ld->ops = ldops;
-			atomic_set(&ld->users, 0);
+			atomic_set(&ld->users, 1);
 			err = 0;
 		}
 	}
@@ -181,35 +209,6 @@ static struct tty_ldisc *tty_ldisc_get(int disc)
 	return ld;
 }
 
-/**
- *	tty_ldisc_put		-	drop ldisc reference
- *	@ld: ldisc
- *
- *	Drop a reference to a line discipline. Manage refcounts and
- *	module usage counts. Free the ldisc once the recount hits zero.
- *
- *	Locking:
- *		takes tty_ldisc_lock to guard against ldisc races
- */
-
-static void tty_ldisc_put(struct tty_ldisc *ld)
-{
-	unsigned long flags;
-	int disc = ld->ops->num;
-	struct tty_ldisc_ops *ldo;
-
-	BUG_ON(disc < N_TTY || disc >= NR_LDISCS);
-
-	spin_lock_irqsave(&tty_ldisc_lock, flags);
-	ldo = tty_ldiscs[disc];
-	BUG_ON(ldo->refcount == 0);
-	ldo->refcount--;
-	module_put(ldo->owner);
-	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
-	WARN_ON(atomic_read(&ld->users));
-	kfree(ld);
-}
-
 static void *tty_ldiscs_seq_start(struct seq_file *m, loff_t *pos)
 {
 	return (*pos < NR_LDISCS) ? pos : NULL;
@@ -234,7 +233,7 @@ static int tty_ldiscs_seq_show(struct seq_file *m, void *v)
 	if (IS_ERR(ld))
 		return 0;
 	seq_printf(m, "%-10s %2d\n", ld->ops->name ? ld->ops->name : "???", i);
-	tty_ldisc_put(ld);
+	put_ldisc(ld);
 	return 0;
 }
 
@@ -288,20 +287,17 @@ static void tty_ldisc_assign(struct tty_struct *tty, struct tty_ldisc *ld)
  *	Locking: takes tty_ldisc_lock
  */
 
-static int tty_ldisc_try(struct tty_struct *tty)
+static struct tty_ldisc *tty_ldisc_try(struct tty_struct *tty)
 {
 	unsigned long flags;
 	struct tty_ldisc *ld;
-	int ret = 0;
 
 	spin_lock_irqsave(&tty_ldisc_lock, flags);
-	ld = tty->ldisc;
-	if (test_bit(TTY_LDISC, &tty->flags)) {
-		atomic_inc(&ld->users);
-		ret = 1;
-	}
+	ld = NULL;
+	if (test_bit(TTY_LDISC, &tty->flags))
+		ld = get_ldisc(tty->ldisc);
 	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
-	return ret;
+	return ld;
 }
 
 /**
@@ -322,10 +318,11 @@ static int tty_ldisc_try(struct tty_struct *tty)
 
 struct tty_ldisc *tty_ldisc_ref_wait(struct tty_struct *tty)
 {
+	struct tty_ldisc *ld;
+
 	/* wait_event is a macro */
-	wait_event(tty_ldisc_wait, tty_ldisc_try(tty));
-	WARN_ON(atomic_read(&tty->ldisc->users) == 0);
-	return tty->ldisc;
+	wait_event(tty_ldisc_wait, (ld = tty_ldisc_try(tty)) != NULL);
+	return ld;
 }
 EXPORT_SYMBOL_GPL(tty_ldisc_ref_wait);
 
@@ -342,9 +339,7 @@ EXPORT_SYMBOL_GPL(tty_ldisc_ref_wait);
 
 struct tty_ldisc *tty_ldisc_ref(struct tty_struct *tty)
 {
-	if (tty_ldisc_try(tty))
-		return tty->ldisc;
-	return NULL;
+	return tty_ldisc_try(tty);
 }
 EXPORT_SYMBOL_GPL(tty_ldisc_ref);
 
@@ -360,19 +355,15 @@ EXPORT_SYMBOL_GPL(tty_ldisc_ref);
 
 void tty_ldisc_deref(struct tty_ldisc *ld)
 {
-	unsigned long flags;
-
-	BUG_ON(ld == NULL);
-
-	spin_lock_irqsave(&tty_ldisc_lock, flags);
-	if (atomic_read(&ld->users) == 0)
-		printk(KERN_ERR "tty_ldisc_deref: no references.\n");
-	else if (atomic_dec_and_test(&ld->users))
-		wake_up(&tty_ldisc_wait);
-	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+	put_ldisc(ld);
 }
 EXPORT_SYMBOL_GPL(tty_ldisc_deref);
 
+static inline void tty_ldisc_put(struct tty_ldisc *ld)
+{
+	put_ldisc(ld);
+}
+
 /**
  *	tty_ldisc_enable	-	allow ldisc use
  *	@tty: terminal to activate ldisc on
@@ -521,31 +512,6 @@ static int tty_ldisc_halt(struct tty_struct *tty)
 }
 
 /**
- *	tty_ldisc_wait_idle	-	wait for the ldisc to become idle
- *	@tty: tty to wait for
- *
- *	Wait for the line discipline to become idle. The discipline must
- *	have been halted for this to guarantee it remains idle.
- *
- *	tty_ldisc_lock protects the ref counts currently.
- */
-
-static int tty_ldisc_wait_idle(struct tty_struct *tty)
-{
-	unsigned long flags;
-	spin_lock_irqsave(&tty_ldisc_lock, flags);
-	while (atomic_read(&tty->ldisc->users)) {
-		spin_unlock_irqrestore(&tty_ldisc_lock, flags);
-		if (wait_event_timeout(tty_ldisc_wait,
-				atomic_read(&tty->ldisc->users) == 0, 5 * HZ) == 0)
-			return -EBUSY;
-		spin_lock_irqsave(&tty_ldisc_lock, flags);
-	}
-	spin_unlock_irqrestore(&tty_ldisc_lock, flags);
-	return 0;
-}
-
-/**
  *	tty_set_ldisc		-	set line discipline
  *	@tty: the terminal to set
  *	@ldisc: the line discipline
@@ -640,14 +606,6 @@ int tty_set_ldisc(struct tty_struct *tty, int ldisc)
 
 	flush_scheduled_work();
 
-	/* Let any existing reference holders finish */
-	retval = tty_ldisc_wait_idle(tty);
-	if (retval < 0) {
-		clear_bit(TTY_LDISC_CHANGING, &tty->flags);
-		tty_ldisc_put(new_ldisc);
-		return retval;
-	}
-
 	mutex_lock(&tty->ldisc_mutex);
 	if (test_bit(TTY_HUPPED, &tty->flags)) {
 		/* We were raced by the hangup method. It will have stomped
@@ -793,7 +751,6 @@ void tty_ldisc_hangup(struct tty_struct *tty)
 		if (tty->ldisc) {	/* Not yet closed */
 			/* Switch back to N_TTY */
 			tty_ldisc_halt(tty);
-			tty_ldisc_wait_idle(tty);
 			tty_ldisc_reinit(tty);
 			/* At this point we have a closed ldisc and we want to
 			   reopen it. We could defer this to the next open but
@@ -858,14 +815,6 @@ void tty_ldisc_release(struct tty_struct *tty, struct tty_struct *o_tty)
 	tty_ldisc_halt(tty);
 	flush_scheduled_work();
 
-	/*
-	 * Wait for any short term users (we know they are just driver
-	 * side waiters as the file is closing so user count on the file
-	 * side is zero.
-	 */
-
-	tty_ldisc_wait_idle(tty);
-
 	mutex_lock(&tty->ldisc_mutex);
 	/*
 	 * Now kill off the ldisc
-- 
1.6.3.2

[PATCH 3/3] tty-ldisc: be more careful in 'put_ldisc' locking

[Greg Kroah-Hartman]

From: Linus Torvalds <torvalds@linux-foundation.org>

Use 'atomic_dec_and_lock()' to make sure that we always hold the
tty_ldisc_lock when the ldisc count goes to zero. That way we can never
race against 'tty_ldisc_try()' increasing the count again.

Reported-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Tested-by: Sergey Senozhatsky <sergey.senozhatsky@mail.by>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/char/tty_ldisc.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/char/tty_ldisc.c b/drivers/char/tty_ldisc.c
index be55dfc..1733d34 100644
--- a/drivers/char/tty_ldisc.c
+++ b/drivers/char/tty_ldisc.c
@@ -55,25 +55,32 @@ static inline struct tty_ldisc *get_ldisc(struct tty_ldisc *ld)
 	return ld;
 }
 
-static inline void put_ldisc(struct tty_ldisc *ld)
+static void put_ldisc(struct tty_ldisc *ld)
 {
+	unsigned long flags;
+
 	if (WARN_ON_ONCE(!ld))
 		return;
 
 	/*
 	 * If this is the last user, free the ldisc, and
 	 * release the ldisc ops.
+	 *
+	 * We really want an "atomic_dec_and_lock_irqsave()",
+	 * but we don't have it, so this does it by hand.
 	 */
-	if (atomic_dec_and_test(&ld->users)) {
-		unsigned long flags;
+	local_irq_save(flags);
+	if (atomic_dec_and_lock(&ld->users, &tty_ldisc_lock)) {
 		struct tty_ldisc_ops *ldo = ld->ops;
 
-		kfree(ld);
-		spin_lock_irqsave(&tty_ldisc_lock, flags);
 		ldo->refcount--;
 		module_put(ldo->owner);
 		spin_unlock_irqrestore(&tty_ldisc_lock, flags);
+
+		kfree(ld);
+		return;
 	}
+	local_irq_restore(flags);
 }
 
 /**
-- 
1.6.3.2