[PATCH 1/7] Char: nozomi, use GFP_KERNEL for kfifo allocation

[PATCH 1/7] Char: nozomi, use GFP_KERNEL for kfifo allocation

[Jiri Slaby]

The allocation was moved to probe function in 9842c38e9176. And we can
sleep there. So allocate the 4*8192 bytes as GFP_KERNEL to mitigate
the allocation failure.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Tested-by: Gerald Pfeifer <gerald@pfeifer.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/tty/nozomi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/nozomi.c b/drivers/tty/nozomi.c
index fd0a9852..acaecc1 100644
--- a/drivers/tty/nozomi.c
+++ b/drivers/tty/nozomi.c
@@ -1431,8 +1431,8 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 	}
 
 	for (i = PORT_MDM; i < MAX_PORT; i++) {
-		if (kfifo_alloc(&dc->port[i].fifo_ul,
-		      FIFO_BUFFER_SIZE_UL, GFP_ATOMIC)) {
+		if (kfifo_alloc(&dc->port[i].fifo_ul, FIFO_BUFFER_SIZE_UL,
+					GFP_KERNEL)) {
 			dev_err(&pdev->dev,
 					"Could not allocate kfifo buffer\n");
 			ret = -ENOMEM;
-- 
1.7.4.2

[PATCH 2/7] Char: nozomi, remove port.count checks

[Jiri Slaby]

Before 33dd474a, these were some kind of protection against race with
HUP. They were protected with port->tty_sem at the same time.

By that commit, the counting was switched to tty_port's one, but the
locking remained the old one. So the count was not protected by
any lock anymore.

The driver should not test whether it raced with HUP or not anyways.
With the new refcounted tty model, it just should proceed as nothing
happened because all needed info is still there. In respect to this,
let's drop the useless and unprotected tests (tty_port->count is
protected by tty_port->lock).

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Tested-by: Gerald Pfeifer <gerald@pfeifer.com>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/tty/nozomi.c |   13 +------------
 1 files changed, 1 insertions(+), 12 deletions(-)

diff --git a/drivers/tty/nozomi.c b/drivers/tty/nozomi.c
index acaecc1..c34d622 100644
--- a/drivers/tty/nozomi.c
+++ b/drivers/tty/nozomi.c
@@ -1690,11 +1690,6 @@ static int ntty_write(struct tty_struct *tty, const unsigned char *buffer,
 
 	mutex_lock(&port->tty_sem);
 
-	if (unlikely(!port->port.count)) {
-		DBG1(" ");
-		goto exit;
-	}
-
 	rval = kfifo_in(&port->fifo_ul, (unsigned char *)buffer, count);
 
 	/* notify card */
@@ -1740,8 +1735,7 @@ static int ntty_write_room(struct tty_struct *tty)
 
 	if (dc) {
 		mutex_lock(&port->tty_sem);
-		if (port->port.count)
-			room = kfifo_avail(&port->fifo_ul);
+		room = kfifo_avail(&port->fifo_ul);
 		mutex_unlock(&port->tty_sem);
 	}
 	return room;
@@ -1889,11 +1883,6 @@ static s32 ntty_chars_in_buffer(struct tty_struct *tty)
 		goto exit_in_buffer;
 	}
 
-	if (unlikely(!port->port.count)) {
-		dev_err(&dc->pdev->dev, "No tty open?\n");
-		goto exit_in_buffer;
-	}
-
 	rval = kfifo_len(&port->fifo_ul);
 
 exit_in_buffer:
-- 
1.7.4.2

[PATCH 3/7] Char: nozomi, remove useless tty_sem

[Jiri Slaby]

tty_sem used to protect tty open count. This was removed in 33dd474a
but the lock remained in place.

So remove it completely as it protects nothing now.

Also this solves Mac's problem with inatomic operation called from
atomic context (ppp):
BUG: scheduling while atomic: firefox-bin/1992/0x10000800
Modules linked in: ...
Pid: 1992, comm: firefox-bin Not tainted 2.6.38 #1
Call Trace:
...
 [] ? mutex_lock+0xe/0x21
 [] ? ntty_write+0x5d/0x192 [nozomi]
 [] ? __mod_timer.clone.30+0xbe/0xcc
 [] ? check_preempt_curr+0x60/0x6d
 [] ? __nf_ct_refresh_acct+0x75/0xbe
 [] ? ppp_async_push+0xa9/0x3bd [ppp_async]
 [] ? ppp_async_send+0x34/0x40 [ppp_async]
 [] ? ppp_push+0x6c/0x4f9 [ppp_generic]
...

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Reported-by: Mac <kmac@poczta.fm>
Tested-by: Gerald Pfeifer <gerald@pfeifer.com>
Cc: Jack Stone <jwjstone@fastmail.fm>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/tty/nozomi.c |   12 ++----------
 1 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/nozomi.c b/drivers/tty/nozomi.c
index c34d622..b1aecc7 100644
--- a/drivers/tty/nozomi.c
+++ b/drivers/tty/nozomi.c
@@ -364,8 +364,6 @@ struct port {
 	u8 toggle_ul;
 	u16 token_dl;
 
-	/* mutex to ensure one access patch to this port */
-	struct mutex tty_sem;
 	wait_queue_head_t tty_wait;
 	struct async_icount tty_icount;
 
@@ -1474,7 +1472,6 @@ static int __devinit nozomi_card_init(struct pci_dev *pdev,
 		struct device *tty_dev;
 		struct port *port = &dc->port[i];
 		port->dc = dc;
-		mutex_init(&port->tty_sem);
 		tty_port_init(&port->port);
 		port->port.ops = &noz_tty_port_ops;
 		tty_dev = tty_register_device(ntty_driver, dc->index_start + i,
@@ -1688,8 +1685,6 @@ static int ntty_write(struct tty_struct *tty, const unsigned char *buffer,
 	if (!dc || !port)
 		return -ENODEV;
 
-	mutex_lock(&port->tty_sem);
-
 	rval = kfifo_in(&port->fifo_ul, (unsigned char *)buffer, count);
 
 	/* notify card */
@@ -1714,7 +1709,6 @@ static int ntty_write(struct tty_struct *tty, const unsigned char *buffer,
 	spin_unlock_irqrestore(&dc->spin_mutex, flags);
 
 exit:
-	mutex_unlock(&port->tty_sem);
 	return rval;
 }
 
@@ -1733,11 +1727,9 @@ static int ntty_write_room(struct tty_struct *tty)
 	int room = 4096;
 	const struct nozomi *dc = get_dc_by_tty(tty);
 
-	if (dc) {
-		mutex_lock(&port->tty_sem);
+	if (dc)
 		room = kfifo_avail(&port->fifo_ul);
-		mutex_unlock(&port->tty_sem);
-	}
+
 	return room;
 }
 
-- 
1.7.4.2

Re: [PATCH 3/7] Char: nozomi, remove useless tty_sem

[Jack Stone]

On 20/04/2011 09:43, Jiri Slaby wrote:
> tty_sem used to protect tty open count. This was removed in 33dd474a
> but the lock remained in place.
> 
> So remove it completely as it protects nothing now.
> 
> Also this solves Mac's problem with inatomic operation called from
> atomic context (ppp):
> BUG: scheduling while atomic: firefox-bin/1992/0x10000800
> Modules linked in: ...
> Pid: 1992, comm: firefox-bin Not tainted 2.6.38 #1
> Call Trace:
> ...
>  [] ? mutex_lock+0xe/0x21
>  [] ? ntty_write+0x5d/0x192 [nozomi]
>  [] ? __mod_timer.clone.30+0xbe/0xcc
>  [] ? check_preempt_curr+0x60/0x6d
>  [] ? __nf_ct_refresh_acct+0x75/0xbe
>  [] ? ppp_async_push+0xa9/0x3bd [ppp_async]
>  [] ? ppp_async_send+0x34/0x40 [ppp_async]
>  [] ? ppp_push+0x6c/0x4f9 [ppp_generic]
> ...
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Reported-by: Mac <kmac@poczta.fm>
> Tested-by: Gerald Pfeifer <gerald@pfeifer.com>
> Cc: Jack Stone <jwjstone@fastmail.fm>
Reviewed-by: Jack Stone <jwjstone@fastmail.fm>
> Cc: Alan Cox <alan@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@suse.de>

Thanks for sorting this Jiri.

Jack

[PATCH 4/7] Char: moxa, fix locking in moxa_write

[Jiri Slaby]

moxa_write can be called from atomic context with irqs disabled (from
ppp_async_push). Don't enable interrupts by spin_unlock_bh as this
might cause deadlocks in the ppp layer.

Instead, use irqsave/irqrestore spin_lock functions.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/tty/moxa.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/moxa.c b/drivers/tty/moxa.c
index a290e9e..6255561 100644
--- a/drivers/tty/moxa.c
+++ b/drivers/tty/moxa.c
@@ -1202,14 +1202,15 @@ static int moxa_write(struct tty_struct *tty,
 		      const unsigned char *buf, int count)
 {
 	struct moxa_port *ch = tty->driver_data;
+	unsigned long flags;
 	int len;
 
 	if (ch == NULL)
 		return 0;
 
-	spin_lock_bh(&moxa_lock);
+	spin_lock_irqsave(&moxa_lock, flags);
 	len = MoxaPortWriteData(tty, buf, count);
-	spin_unlock_bh(&moxa_lock);
+	spin_unlock_irqrestore(&moxa_lock, flags);
 
 	set_bit(LOWWAIT, &ch->statusflags);
 	return len;
-- 
1.7.4.2

[PATCH 5/7] TTY: serial_core, remove invalid test

[Jiri Slaby]

tty->index (named here as line) is set up in initialize_tty_struct.
The value is checked in get_tty_driver for the found driver as:
	if (device < base || device >= base + p->num)
		continue;
	*index = device - base;

So index/line can never be more than driver->num. Hence remove this
test from uart_open.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/tty/serial/serial_core.c |    9 ---------
 1 files changed, 0 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index d4bd465..848fd13 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1543,15 +1543,6 @@ static int uart_open(struct tty_struct *tty, struct file *filp)
 	pr_debug("uart_open(%d) called\n", line);
 
 	/*
-	 * tty->driver->num won't change, so we won't fail here with
-	 * tty->driver_data set to something non-NULL (and therefore
-	 * we won't get caught by uart_close()).
-	 */
-	retval = -ENODEV;
-	if (line >= tty->driver->num)
-		goto fail;
-
-	/*
 	 * We take the semaphore inside uart_get to guarantee that we won't
 	 * be re-entered while allocating the state structure, or while we
 	 * request any IRQs that the driver may need.  This also has the nice
-- 
1.7.4.2

[PATCH 6/7] TTY: serial_core, remove superfluous set_task_state

[Jiri Slaby]

msleep* is guaranteed to return with TASK_RUNNING task state. And
since there is no other set_task_state in the paths of
uart_wait_until_sent, we need not to set_task_state to TASK_RUNNING.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Alan Cox <alan@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/tty/serial/serial_core.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 848fd13..cda1089 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1427,7 +1427,6 @@ static void __uart_wait_until_sent(struct uart_port *port, int timeout)
 		if (time_after(jiffies, expire))
 			break;
 	}
-	set_current_state(TASK_RUNNING); /* might not be needed */
 }
 
 static void uart_wait_until_sent(struct tty_struct *tty, int timeout)
-- 
1.7.4.2

[PATCH 7/7] TTY: tty_io, annotate locking functions

[Jiri Slaby]

tty_write_lock and tty_write_unlock contain imbalanced locking. But
this is intentional, so mark them appropriately by
__acquires/__releases.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/tty/tty_io.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 0bcf388..7721d6d 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -964,12 +964,14 @@ static ssize_t tty_read(struct file *file, char __user *buf, size_t count,
 }
 
 void tty_write_unlock(struct tty_struct *tty)
+	__releases(&tty->atomic_write_lock)
 {
 	mutex_unlock(&tty->atomic_write_lock);
 	wake_up_interruptible_poll(&tty->write_wait, POLLOUT);
 }
 
 int tty_write_lock(struct tty_struct *tty, int ndelay)
+	__acquires(&tty->atomic_write_lock)
 {
 	if (!mutex_trylock(&tty->atomic_write_lock)) {
 		if (ndelay)
-- 
1.7.4.2