| Message ID | 1282329921-24394-1-git-send-email-timur@freescale.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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 <timur@freescale.com> 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. > > 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 <timur@freescale.com> > --- > 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 > > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev >
On Fri, 20 Aug 2010 13:45:21 -0500 Timur Tabi <timur@freescale.com> 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 <akpm@linux-foundation.org> wrote: > On Fri, 20 Aug 2010 13:45:21 -0500 > Timur Tabi <timur@freescale.com> 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 is just for printk(), not user output. This is exactly what printk() has always done for real serial ports. > > 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
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.
> 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: > 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.
On Tue, 14 Sep 2010 14:25:34 -0500 Timur Tabi <timur@freescale.com> wrote: > 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. 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)
On Tue, 14 Sep 2010 20:44:10 +0100 Alan Cox <alan@lxorguk.ukuu.org.uk> wrote: > > 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
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; } } }
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 <timur@freescale.com> --- drivers/char/hvc_console.c | 19 ++++++++++++++++++- 1 files changed, 18 insertions(+), 1 deletions(-)