[PATCH 0/5] hvc_console updates was Re: [BUG] hvc_console WARN() on current upstream

[PATCH 0/5] hvc_console updates was Re: [BUG] hvc_console WARN() on current upstream

[Milton Miller]

Alan Cox wrote:
> Benjamin Herrenschmidt wrote:
>> Seems that we are in interrupt, doing hvc_poll, which does
>> tty_flip_buffer_push
>
>Which means that someone has tty->low_latency set and is calling
>tty_flip_buffer_push in an IRQ. That has never been allowed or safe, and
>now it hurts ;)
>
>/**
> *      tty_flip_buffer_push    -       terminal
> *      @tty: tty to push
> * 
> *      Queue a push of the terminal flip buffers to the line discipline.
>This
> *      function must not be called from IRQ context if tty->low_latency
>is set *
> *      In the event of the queue being busy for flipping the work will be
> *      held off and retried later.
> *
> *      Locking: tty buffer lock. Driver locks in low latency mode.
> */
>
>
>That comment has been there for some years in varying formats
>

I actually was preparing a patch for this problem after I had encountered
the a deadlock due to this.  That is in the first patch.   I then found
and made a few more cleanups, although I might have reordered the rest.

The history for setting low_latency is in the changelog of the first patch..

milton

[PATCH 4/4] hvc_console: comment mb and make it an smp_ one

[Milton Miller]

I remember some history on this barrier.  There was a race between
open via /dev/console and the tty being fully setup.  Its also why
there is a temporary variable and the global is assigned at the end
of the function.

Signed-off-by: Milton Miller <miltonm@bga.com>


Index: work.git/drivers/char/hvc_console.c
===================================================================
--- work.git.orig/drivers/char/hvc_console.c	2009-01-08 04:33:39.000000000 -0600
+++ work.git/drivers/char/hvc_console.c	2009-01-08 04:35:09.000000000 -0600
@@ -875,8 +875,11 @@ static int hvc_init(void)
 		goto stop_thread;
 	}
 
-	/* FIXME: This mb() seems completely random.  Remove it. */
-	mb();
+	/*
+	 * Make sure tty is fully registered before allowing it to be
+	 * found by hvc_console_device.
+	 */
+	smp_mb();
 	hvc_driver = drv;
 	return 0;
 

[PATCH 3/4] hvc_console: free_irq only if request_irq was successful

[Milton Miller]

Only call free_irq if we marked the request_irq has having succeeded
instead of whenever the the sub-driver identified the interrupt to use.

Signed-off-by: Milton Miller <miltonm@bga.com>
---
Appears to be a bugfix but might use a bit of time in -next.
This code was created in 2.6.28 to allow s390 to build without adding ifdefs.
Many hvc-console sub-drivers have a single channel and are not modules.

Index: work.git/drivers/char/hvc_irq.c
===================================================================
--- work.git.orig/drivers/char/hvc_irq.c	2009-01-08 04:07:28.000000000 -0600
+++ work.git/drivers/char/hvc_irq.c	2009-01-08 04:07:47.000000000 -0600
@@ -37,7 +37,7 @@ int notifier_add_irq(struct hvc_struct *
 
 void notifier_del_irq(struct hvc_struct *hp, int irq)
 {
-	if (!irq)
+	if (!hp->irq_requested)
 		return;
 	free_irq(irq, hp);
 	hp->irq_requested = 0;

Re: [PATCH 3/4] hvc_console: free_irq only if request_irq was successful

[Christian Borntraeger]

Am Donnerstag, 8. Januar 2009 schrieb Milton Miller:
> Only call free_irq if we marked the request_irq has having succeeded
> instead of whenever the the sub-driver identified the interrupt to use.
> 
> Signed-off-by: Milton Miller <miltonm@bga.com>
> ---
> Appears to be a bugfix but might use a bit of time in -next.
> This code was created in 2.6.28 to allow s390 to build without adding 
ifdefs.
> Many hvc-console sub-drivers have a single channel and are not modules.
> 
> Index: work.git/drivers/char/hvc_irq.c
> ===================================================================
> --- work.git.orig/drivers/char/hvc_irq.c	2009-01-08 04:07:28.000000000 -0600
> +++ work.git/drivers/char/hvc_irq.c	2009-01-08 04:07:47.000000000 -0600
> @@ -37,7 +37,7 @@ int notifier_add_irq(struct hvc_struct *
> 
>  void notifier_del_irq(struct hvc_struct *hp, int irq)
>  {
> -	if (!irq)
> +	if (!hp->irq_requested)
>  		return;
>  	free_irq(irq, hp);
>  	hp->irq_requested = 0;
> 

Looks sane.

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

[PATCH 1/4] hvc_console: do not set low_latency

[Milton Miller]

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;
 

Re: [PATCH 1/4] hvc_console: do not set low_latency

[Alan Cox]

> v2.6.16-rc1 via 33f0f88f1c51ae5c2d593d26960c760ea154c2e2
>     [PATCH] TTY layer buffering revamp
> 
> added this new api.

No - tty_flip_buffer_push is from 2.1.66 and with the same constraints
from the day it was added.


> 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;

Looks good to me

Re: [PATCH 1/4] hvc_console: do not set low_latency

[Milton Miller]

On Jan 8, 2009, at 6:36 AM, Alan Cox wrote:

>> v2.6.16-rc1 via 33f0f88f1c51ae5c2d593d26960c760ea154c2e2
>>     [PATCH] TTY layer buffering revamp
>>
>> added this new api.
>
> No - tty_flip_buffer_push is from 2.1.66 and with the same constraints
> from the day it was added.
>
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

And they were slightly buggy leading to the buggy workaround.

>
>> 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;
>
> Looks good to me

Thanks, I guess that is an Ack?

milton

Re: [PATCH 1/4] hvc_console: do not set low_latency

[Christian Borntraeger]

Am Donnerstag 08 Januar 2009 schrieb Milton Miller:
> 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;
> 
> 

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

Re: [PATCH 1/4] hvc_console: do not set low_latency

[Christian Borntraeger]

Am Dienstag 13 Januar 2009 schrieb Christian Borntraeger:
>  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
> 
> 

Just in case:

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

Re: [PATCH 1/4] hvc_console: do not set low_latency

[Hendrik Brueckner]

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.

On Tue, Jan 13, 2009 at 10:04:22AM +0100, Christian Borntraeger wrote:
> Am Donnerstag 08 Januar 2009 schrieb Milton Miller:
> > 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.
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>] 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: [<00000000002ceb50>] khvcd+0x58/0x12c

> 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

Acked-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>

Re: [PATCH 1/4] hvc_console: do not set low_latency

[Milton Miller]

On Jan 13, 2009, at 5:35 AM, Hendrik Brueckner wrote:

> 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.
>
> On Tue, Jan 13, 2009 at 10:04:22AM +0100, Christian Borntraeger wrote:
>> Am Donnerstag 08 Januar 2009 schrieb Milton Miller:
>>> 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.
> 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.
>


So the simplest is to never set it.

milton

> 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: [<00000000002ceb50>] 
> khvcd+0x58/0x12c
>
>> 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
>
> Acked-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
>

Re: [PATCH 1/4] hvc_console: do not set low_latency

[Benjamin Herrenschmidt]

> So the simplest is to never set it.

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

Cheers,
Ben.

> milton
> 
> > 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: [<00000000002ceb50>] 
> > khvcd+0x58/0x12c
> >
> >> 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
> >
> > Acked-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
> >

Re: [PATCH 1/4 v2] hvc_console: remove tty->low_latency

[Hendrik Brueckner]

On Wed, Jan 14, 2009 at 08:04:04AM +1100, Benjamin Herrenschmidt wrote:
> > So the simplest is to never set it.
> 
> I already applied your previous patch. Please send an incremental fix.
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: [<00000000002ceb50>] khvcd+0x58/0x12c

Signed-off-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
---
 drivers/char/hvc_console.c |    2 --
 1 file changed, 2 deletions(-)

--- a/drivers/char/hvc_console.c
+++ b/drivers/char/hvc_console.c
@@ -318,8 +318,6 @@ static int hvc_open(struct tty_struct *t
 	} /* else count == 0 */
 
 	tty->driver_data = hp;
-	if (!hp->irq_requested)
-		tty->low_latency = 1; /* Makes flushes to ldisc synchronous. */
 
 	hp->tty = tty;
 

Re: [PATCH 1/4 v2] hvc_console: remove tty->low_latency

[Christian Borntraeger]

> Signed-off-by: Hendrik Brueckner <brueckner@linux.vnet.ibm.com>
> ---
>  drivers/char/hvc_console.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> --- a/drivers/char/hvc_console.c
> +++ b/drivers/char/hvc_console.c
> @@ -318,8 +318,6 @@ static int hvc_open(struct tty_struct *t
>  	} /* else count == 0 */
> 
>  	tty->driver_data = hp;
> -	if (!hp->irq_requested)
> -		tty->low_latency = 1; /* Makes flushes to ldisc synchronous. */
> 
>  	hp->tty = tty;
> 
> 

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

[PATCH 2/4] hvc_console: use kzalloc

[Milton Miller]

Replace kmalloc + memset  with kzalloc.

Signed-off-by: Milton Miller <miltonm@bga.com>

Index: work.git/drivers/char/hvc_console.c
===================================================================
--- work.git.orig/drivers/char/hvc_console.c	2009-01-08 04:24:10.000000000 -0600
+++ work.git/drivers/char/hvc_console.c	2009-01-08 04:24:35.000000000 -0600
@@ -765,13 +765,11 @@ struct hvc_struct __devinit *hvc_alloc(u
 			return ERR_PTR(err);
 	}
 
-	hp = kmalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size,
+	hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size,
 			GFP_KERNEL);
 	if (!hp)
 		return ERR_PTR(-ENOMEM);
 
-	memset(hp, 0x00, sizeof(*hp));
-
 	hp->vtermno = vtermno;
 	hp->data = data;
 	hp->ops = ops;