[PATCH] hvc_console: fix dropping of characters when output byte channel is full

hvc_console_print() calls the HVC client driver's put_chars() callback
to write some characters to the console.  If the callback returns 0, that
indicates that no characters were written (perhaps the output buffer is
full), but hvc_console_print() treats that as an error and discards the
rest of the buffer.

So change hvc_console_print() to just loop and call put_chars() again if it
returns a 0 return code.

This change makes hvc_console_print() behave more like hvc_push(), which
does check for a 0 return code and re-schedules itself.

Signed-off-by: Timur Tabi <redacted>
---
 drivers/char/hvc_console.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/drivers/char/hvc_console.c b/drivers/char/hvc_console.c
index fa27d16..b4deffd 100644
--- a/drivers/char/hvc_console.c
+++ b/drivers/char/hvc_console.c

@@ -3,6 +3,7 @@
  * Copyright (C) 2001 Paul Mackerras <paulus@au.ibm.com>, IBM
  * Copyright (C) 2004 Benjamin Herrenschmidt <benh@kernel.crashing.org>, IBM Corp.
  * Copyright (C) 2004 IBM Corporation
+ * Copyright 2009 Freescale Semiconductor, Inc.
  *
  * Additional Author(s):
  *  Ryan S. Arnold <rsa@us.ibm.com>

@@ -141,6 +142,7 @@ static void hvc_console_print(struct console *co, const char *b,
 	char c[N_OUTBUF] __ALIGNED__;
 	unsigned i = 0, n = 0;
 	int r, donecr = 0, index = co->index;
+	unsigned int timeout = 1000000; /* Keep trying for up to one second */
 
 	/* Console access attempt outside of acceptable console range. */
 	if (index >= MAX_NR_HVC_CONSOLES)

@@ -152,6 +154,10 @@ static void hvc_console_print(struct console *co, const char *b,
 
 	while (count > 0 || i > 0) {
 		if (count > 0 && i < sizeof(c)) {
+			/* If the local buffer (c) is not full, then copy some
+			 * bytes from the input buffer to it. We stop when the
+			 * local buffer is full. \n is converted to \r\n.
+			 */
 			if (b[n] == '\n' && !donecr) {
 				c[i++] = '\r';
 				donecr = 1;

@@ -162,14 +168,25 @@ static void hvc_console_print(struct console *co, const char *b,
 			}
 		} else {
 			r = cons_ops[index]->put_chars(vtermnos[index], c, i);
-			if (r <= 0) {
+			if (r < 0) {
 				/* throw away chars on error */
 				i = 0;
 			} else if (r > 0) {
 				i -= r;
 				if (i > 0)
 					memmove(c, c+r, i);
+			} else {
+				/* If r == 0, then the client driver didn't do
+				 * anything, so wait 1us and try again.  If we
+				 * time out, then just exit.
+				 */
+				if (!--timeout)
+					return;
+				udelay(1);
+				continue;
 			}
+			/* Reset the timeout */
+			timeout = 1000000;
 		}
 	}
 }

-- 
1.7.0.1

Andrew,

Would you please take a moment to review this patch I sent last month?
I'd like for it to be incorporated into 2.6.37, but I'm having a hard
time finding someone to consider it.

Thanks.

On Fri, Aug 20, 2010 at 1:45 PM, Timur Tabi [off-list ref] wrote:

hvc_console_print() calls the HVC client driver's put_chars() callback
to write some characters to the console. =A0If the callback returns 0, th=

at

indicates that no characters were written (perhaps the output buffer is
full), but hvc_console_print() treats that as an error and discards the
rest of the buffer.

So change hvc_console_print() to just loop and call put_chars() again if =

it

, IBM Corp.

st char *b,

=A0 =A0 =A0 =A0char c[N_OUTBUF] __ALIGNED__;
=A0 =A0 =A0 =A0unsigned i =3D 0, n =3D 0;
=A0 =A0 =A0 =A0int r, donecr =3D 0, index =3D co->index;
+ =A0 =A0 =A0 unsigned int timeout =3D 1000000; /* Keep trying for up to =

one second */

=A0 =A0 =A0 =A0/* Console access attempt outside of acceptable console ra=

nge. */

nst char *b,

=A0 =A0 =A0 =A0while (count > 0 || i > 0) {
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (count > 0 && i < sizeof(c)) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* If the local buffer (c) =

is not full, then copy some

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* bytes from the input b=

uffer to it. We stop when the

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* local buffer is full. =

\n is converted to \r\n.

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (b[n] =3D=3D '\n' && !d=

onecr) {

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0c[i++] =3D=

 '\r';

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0donecr =3D=

 1;

onst char *b,

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else {
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0r =3D cons_ops[index]->put=

_chars(vtermnos[index], c, i);

- =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (r <=3D 0) {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (r < 0) {
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* throw a=

way chars on error */

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0i =3D 0;
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else if (r > 0) {
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0i -=3D r;
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (i > 0)
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =

=A0 =A0memmove(c, c+r, i);

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 } else {
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* If r =3D=

=3D 0, then the client driver didn't do

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* anythi=

ng, so wait 1us and try again. =A0If we

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* time o=

ut, then just exit.

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!--time=

out)

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=

 =A0 return;

+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(1);
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 continue;
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Reset the timeout */
+ =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 timeout =3D 1000000;
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
=A0 =A0 =A0 =A0}
=A0}
--
1.7.0.1


_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

--=20
Timur Tabi
Linux kernel developer at Freescale

On Fri, 20 Aug 2010 13:45:21 -0500
Timur Tabi [off-list ref] wrote:

hvc_console_print() calls the HVC client driver's put_chars() callback
to write some characters to the console.  If the callback returns 0, that
indicates that no characters were written (perhaps the output buffer is
full), but hvc_console_print() treats that as an error and discards the
rest of the buffer.

So change hvc_console_print() to just loop and call put_chars() again if it
returns a 0 return code.

Seems rather dangerous.  The upper layer will sit there chewing 100%
CPU for as long as the lower layer is congested.

This change makes hvc_console_print() behave more like hvc_push(), which
does check for a 0 return code and re-schedules itself.

Yes, hvc_push() reschedules.  It doesn't sit in a tight loop burning
electrons!

Can we do something safer&smarter here?

On Tue, 14 Sep 2010 12:17:21 -0700
Andrew Morton [off-list ref] wrote:

On Fri, 20 Aug 2010 13:45:21 -0500
Timur Tabi [off-list ref] wrote:

quoted

hvc_console_print() calls the HVC client driver's put_chars() callback
to write some characters to the console.  If the callback returns 0, that
indicates that no characters were written (perhaps the output buffer is
full), but hvc_console_print() treats that as an error and discards the
rest of the buffer.

So change hvc_console_print() to just loop and call put_chars() again if it
returns a 0 return code.

Seems rather dangerous.  The upper layer will sit there chewing 100%
CPU for as long as the lower layer is congested.

This is just for printk(), not user output.  This is exactly what
printk() has always done for real serial ports.

quoted

This change makes hvc_console_print() behave more like hvc_push(), which
does check for a 0 return code and re-schedules itself.

Yes, hvc_push() reschedules.

hvc_push() is not relevant to kernel console output.

hvc_console_write() currently does not reschedule anything.  It just
drops characters when busy.

-Scott

Yes, hvc_push() reschedules.  It doesn't sit in a tight loop burning
electrons!

Can we do something safer&smarter here?

Its a printk handler - better to lose the bytes than hang the box. I
think the current code is probably right.

Alan Cox wrote:

Its a printk handler - better to lose the bytes than hang the box. I
think the current code is probably right.

What do you think about this change:

http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-October/thread.html#76830

This is the original version of my patch.  In it, I simply allow drivers to
return 0 to indicate that they're too busy to accept data.  It works great
on the hvc driver that we have in-house today, but it might break other
drivers that return 0 to indicate error.

-- 
Timur Tabi
Linux kernel developer at Freescale

On Tue, 14 Sep 2010 14:25:34 -0500
Timur Tabi [off-list ref] wrote:

Alan Cox wrote:

quoted

Its a printk handler - better to lose the bytes than hang the box. I
think the current code is probably right.

What do you think about this change:

http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-October/thread.html#76830

This is the original version of my patch.  In it, I simply allow drivers to
return 0 to indicate that they're too busy to accept data.  It works great
on the hvc driver that we have in-house today, but it might break other
drivers that return 0 to indicate error.

If you want to make that change then I guess you need to audit every
other hvc driver first.

But I never understood the point of the hvc layer anyway 8)

Alan Cox wrote:

If you want to make that change then I guess you need to audit every
other hvc driver first.

But I never understood the point of the hvc layer anyway 8)

We've come to the same conclusion here.  I think we're just going to drop
the hvc layer and make the driver a standard console driver.

-- 
Timur Tabi
Linux kernel developer at Freescale

On Tue, 14 Sep 2010 20:44:10 +0100
Alan Cox [off-list ref] wrote:

quoted

Yes, hvc_push() reschedules.  It doesn't sit in a tight loop burning
electrons!

Can we do something safer&smarter here?

Its a printk handler - better to lose the bytes than hang the box. I
think the current code is probably right.

Losing the bytes is unacceptable.  Even if an hvc backend erroneously
returns zero on a permanent error, the timeout should prevent hanging
the box for too long.

Though I suspect the right answer for us may be "don't use the hvc
layer".

-Scott