hvc_console is setting low_latency unconditionally, but some clients are
interrupt driven and will call hvc_poll from irq context.  This will cause
tty_flip_buffer_push to be called from irq context, and it very clearly
states it must not be called from IRQ when low_latency is specified.

Looking back through history:
v2.6.16-rc1 via 33f0f88f1c51ae5c2d593d26960c760ea154c2e2
    [PATCH] TTY layer buffering revamp

added this new api.

v2.6.16-rc3 via 8977d929e49021d9a6e031310aab01fa72f849c2
    [PATCH] tty buffering stall fix

claims to fix a stall discovered with hvc_console

v2.6.16-rc5 via fb5c594c2acc441f0d2d8f457484a0e0e9285db3
   [PATCH] Fix race condition in hvc console.

said set this flag to avoid a stall problem, and was merged through
the powerpc arch tree.

Without searching for email discussions, it would appear to be an
overlapping "fix", but one that did not consider all users.

---
This version continues to set low_latency if irqs are not flagged to
be in use, as requested by paulus.   Do all hvc drivers identify the
interrupt this way?  (A quick look at hvc_iucv says they lock to bh
and are not irq driven, the rest would have used the irq before that
patch).

Having the flag set for purely polled drivers will save delaying
the work when receiving input for 1 jiffie.


Index: work.git/drivers/char/hvc_console.c
===================================================================
--- work.git.orig/drivers/char/hvc_console.c	2009-01-08 03:01:24.000000000 -0600
+++ work.git/drivers/char/hvc_console.c	2009-01-08 03:01:51.000000000 -0600
@@ -318,7 +318,8 @@ static int hvc_open(struct tty_struct *t
 	} /* else count == 0 */
 
 	tty->driver_data = hp;
-	tty->low_latency = 1; /* Makes flushes to ldisc synchronous. */
+	if (!hp->irq_requested)
+		tty->low_latency = 1; /* Makes flushes to ldisc synchronous. */
 
 	hp->tty = tty;
 
--

[ message continues ]

No - tty_flip_buffer_push is from 2.1.66 and with the same constraints

Looks good to me
--

[ message continues ]

Yes but wrappers were added and this this and many ohter drivers were  
converted to use them:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git; 
a=commitdiff;h=33f0f88f1c51ae5c2d593d26960c760ea154c2e2


Thanks, I guess that is an Ack?

milton

--

[ message continues ]

This wont work, since the call to notifier_add is done later:
What about:
---
 drivers/char/hvc_console.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/char/hvc_console.c
===================================================================
--- linux-2.6.orig/drivers/char/hvc_console.c
+++ linux-2.6/drivers/char/hvc_console.c
@@ -318,8 +318,6 @@ static int hvc_open(struct tty_struct *t
 	} /* else count == 0 */
 
 	tty->driver_data = hp;
-	tty->low_latency = 1; /* Makes flushes to ldisc synchronous. */
-
 	hp->tty = tty;
 
 	spin_unlock_irqrestore(&hp->lock, flags);
@@ -327,6 +325,9 @@ static int hvc_open(struct tty_struct *t
 	if (hp->ops->notifier_add)
 		rc = hp->ops->notifier_add(hp, hp->data);
 
+	if (!hp->irq_requested)
+		tty->low_latency = 1; /* Makes flushes to ldisc synchronous. */
+
 	/*
 	 * If the notifier fails we return an error.  The tty layer
 	 * will call hvc_close() after a failed open but we don't want to clean


--

[ message continues ]

Just in case:

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
--

[ message continues ]

Here is yet another bug trace related to the low_latency issue, that is
experienced using the hvc_iucv backend.  The hvc_iucv backend now also uses irq
notifiers.
The bug trace below appears if hvc_kick() is called.

It seems that if low_latency is set, tty_flip_buffer_push() should also not be
called within an atomic context, because echo_char_raw() and other echo* calls
might sleep.

Christian's patch solves this problem for irq driven backends.
However, there might be still a problem with polled backends since the khvcd()
thread calls hvc_poll() while hvc_structs_lock is held.

I think the hvc_udbg backend is based on polling.
David, could you check if you experience any problems with your hvc_udbg
backend on latest git.

BUG: sleeping function called from invalid context at /root/cvs/linux-2.6.git/kernel/mutex.c:207
in_atomic(): 1, irqs_disabled(): 0, pid: 748, name: khvcd
1 lock held by khvcd/748:
 #0:  (hvc_structs_lock){--..}, at: [<00000000002ceb50>] khvcd+0x58/0x12c
CPU: 0 Not tainted 2.6.29-rc1git #29
Process khvcd (pid: 748, task: 000000002fb9a480, ksp: 000000002f66bd78)
070000000000000a 000000002f66ba00 0000000000000002 (null) 
       000000002f66baa0 000000002f66ba18 000000002f66ba18 0000000000104f08 
       ffffffffffffc000 000000002f66bd78 (null) (null) 
       000000002f66ba00 000000000000000c 000000002f66ba00 000000002f66ba70 
       0000000000466af8 0000000000104f08 000000002f66ba00 000000002f66ba50 
Call Trace:
([<0000000000104e7c>] show_trace+0x138/0x158)
 [<0000000000104f62>] show_stack+0xc6/0xf8
 [<0000000000105740>] dump_stack+0xb0/0xc0
 [<000000000013144a>] __might_sleep+0x14e/0x17c
 [<000000000045e226>] mutex_lock_nested+0x42/0x3b4
 [<00000000002c443e>] echo_char_raw+0x3a/0x9c
 [<00000000002c688c>] n_tty_receive_buf+0x1154/0x1208
 [<00000000002ca0a2>] flush_to_ldisc+0x152/0x220
 [<00000000002ca1da>] tty_flip_buffer_push+0x6a/0x90
 [<00000000002cea74>] hvc_poll+0x244/0x2c8
 [<00000000002ceb68>] khvcd+0x70/0x12c
 [<000000000015bbd0>] ...
[ message continues ]

So the simplest is to never set it.


--

[ message continues ]

I already applied your previous patch. Please send an incremental fix.

Cheers,

--

[ message continues ]

Here is the incremental patch for your powerpc tree along with a summary
as patch description.

Regards,
Hendrik

--
This patch removes the tty->low_latency setting.

For irq based hvc_console backends the tty->low_latency must be set to 0,
because the tty_flip_buffer_push() function must not be called from IRQ context
(see drivers/char/tty_buffer.c).

For polled backends, the low_latency setting causes the bug trace below, because
tty_flip_buffer_push() is called within an atomic context and subsequent calls
might sleep due to mutex_lock.

BUG: sleeping function called from invalid context at /root/cvs/linux-2.6.git/kernel/mutex.c:207
in_atomic(): 1, irqs_disabled(): 0, pid: 748, name: khvcd
1 lock held by khvcd/748:
 #0:  (hvc_structs_lock){--..}, at: [<00000000002ceb50>] khvcd+0x58/0x12c
CPU: 0 Not tainted 2.6.29-rc1git #29
Process khvcd (pid: 748, task: 000000002fb9a480, ksp: 000000002f66bd78)
070000000000000a 000000002f66ba00 0000000000000002 (null) 
       000000002f66baa0 000000002f66ba18 000000002f66ba18 0000000000104f08 
       ffffffffffffc000 000000002f66bd78 (null) (null) 
       000000002f66ba00 000000000000000c 000000002f66ba00 000000002f66ba70 
       0000000000466af8 0000000000104f08 000000002f66ba00 000000002f66ba50 
Call Trace:
([<0000000000104e7c>] show_trace+0x138/0x158)
 [<0000000000104f62>] show_stack+0xc6/0xf8
 [<0000000000105740>] dump_stack+0xb0/0xc0
 [<000000000013144a>] __might_sleep+0x14e/0x17c
 [<000000000045e226>] mutex_lock_nested+0x42/0x3b4
 [<00000000002c443e>] echo_char_raw+0x3a/0x9c
 [<00000000002c688c>] n_tty_receive_buf+0x1154/0x1208
 [<00000000002ca0a2>] flush_to_ldisc+0x152/0x220
 [<00000000002ca1da>] tty_flip_buffer_push+0x6a/0x90
 [<00000000002cea74>] hvc_poll+0x244/0x2c8
 [<00000000002ceb68>] khvcd+0x70/0x12c
 [<000000000015bbd0>] kthread+0x68/0xa0
 [<0000000000109d5a>] kernel_thread_starter+0x6/0xc
 [<0000000000109d54>] kernel_thread_starter+0x0/0xc
1 lock held by khvcd/748:
 #0:  (hvc_structs_lock){--..}, at: ...
[ message continues ]

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
--

[ message continues ]