[U-Boot] [PATCH] logbuff: Prevent an infinite loop for console output

[U-Boot] [PATCH] logbuff: Prevent an infinite loop for console output

[Peter Tyser]

When using 'logbuff' as stdout and the console loglevel is greater
than a message's loglevel it is supposed to be both logged, and printed
to the console.  The logbuff_printk() function is responsible for both
logging and displaying the message.  However, logbuff_printk()
previously used printf() to print the message to the console.  The
printf() call would eventually end up back in logbuff_printk(), and
an infinite loop would occur which would hang a board.

Using serial_puts() instead of printf() in logbuff_printk() avoids the
recursion and resolves the issue.

Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
Reported-by: Dennis Ruffer <daruffer@gmail.com>
---
 common/cmd_log.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/common/cmd_log.c b/common/cmd_log.c
index e3ebe96..baffeb1 100644
--- a/common/cmd_log.c
+++ b/common/cmd_log.c
@@ -313,9 +313,10 @@ static int logbuff_printk(const char *line)
 				break;
 			}
 		}
-		if (msg_level < console_loglevel) {
-			printf("%s", msg);
-		}
+
+		if (msg_level < console_loglevel)
+			serial_puts(msg);
+
 		if (line_feed)
 			msg_level = -1;
 	}
-- 
1.6.2.1

[U-Boot] [PATCH] logbuff: Prevent an infinite loop for console output

[Detlev Zundel]

Hi Peter,

> When using 'logbuff' as stdout and the console loglevel is greater
> than a message's loglevel it is supposed to be both logged, and printed
> to the console.  The logbuff_printk() function is responsible for both
> logging and displaying the message.  However, logbuff_printk()
> previously used printf() to print the message to the console.  The
> printf() call would eventually end up back in logbuff_printk(), and
> an infinite loop would occur which would hang a board.
>
> Using serial_puts() instead of printf() in logbuff_printk() avoids the
> recursion and resolves the issue.
>
> Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
> Reported-by: Dennis Ruffer <daruffer@gmail.com>

Hm.  What if a board has "stdout" set to "lcd" or "nc" or any other
device?  Do we really want the text to be output on the serial console
then?  Doesn't this break the whole "stdout" concept?

Cheers
  Detlev

-- 
Some people unfortunately like  jumping up and down about spaces but not code.
[...]   I'd rather read good poetry written in very bad hand writing than bad
poetry written in beautiful handwriting, and I think the same is true of code.
           -- Alan Cox <20090701130018.115ce0ea@lxorguk.ukuu.org.uk>
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] [PATCH] logbuff: Prevent an infinite loop for console output

[Peter Tyser]

On Fri, 2010-05-07 at 10:23 +0200, Detlev Zundel wrote:
> Hi Peter,
> 
> > When using 'logbuff' as stdout and the console loglevel is greater
> > than a message's loglevel it is supposed to be both logged, and printed
> > to the console.  The logbuff_printk() function is responsible for both
> > logging and displaying the message.  However, logbuff_printk()
> > previously used printf() to print the message to the console.  The
> > printf() call would eventually end up back in logbuff_printk(), and
> > an infinite loop would occur which would hang a board.
> >
> > Using serial_puts() instead of printf() in logbuff_printk() avoids the
> > recursion and resolves the issue.
> >
> > Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
> > Reported-by: Dennis Ruffer <daruffer@gmail.com>
> 
> Hm.  What if a board has "stdout" set to "lcd" or "nc" or any other
> device?  Do we really want the text to be output on the serial console
> then?  Doesn't this break the whole "stdout" concept?

Yes, it does break the stdout concept, but I went with the current patch
for 3 reasons:
1. Most anything is better than the current board lockup that occurs
without the change.

2. For most of the boot process serial_puts() is used to output messages
(ie prior to GD_FLG_DEVINIT is set in gd->flags).  Continuing to use the
same output method after GD_FLG_DEVINIT is set makes some degree of
sense.

3. A proper fix would be more involved.  For example, perhaps allowing
the stdout variable to support multiple devices (eg
"stdout=logbuff,lcd") would be a more elegant solution.  In any case, to
use both the logbuff and non-serial port for output would require much
more extensive changes than this 1-line change.  I don't use the
logbuff, so am not so interested in spending the time to fix it.  The
fact that no one else ran into this bug implies no on else is using it
either.

I agree this fix isn't the best, but its better than the bug in my
opinion.  Ideally someone who uses the logbuff could provide a more
elegant fix.  Any takers?

Best,
Peter

[U-Boot] [PATCH] logbuff: Prevent an infinite loop for console output

[Wolfgang Denk]

Dear Peter Tyser,

In message <1273253668.22784.57.camel@localhost.localdomain> you wrote:
>
> > Hm.  What if a board has "stdout" set to "lcd" or "nc" or any other
> > device?  Do we really want the text to be output on the serial console
> > then?  Doesn't this break the whole "stdout" concept?
> 
> Yes, it does break the stdout concept, but I went with the current patch
> for 3 reasons:

OK, then let's try and come up with a solution that does NOT break
this.

> 1. Most anything is better than the current board lockup that occurs
> without the change.

Even if I was willing to buy this one...

> 2. For most of the boot process serial_puts() is used to output messages
> (ie prior to GD_FLG_DEVINIT is set in gd->flags).  Continuing to use the
> same output method after GD_FLG_DEVINIT is set makes some degree of
> sense.

... this does not make sense to me.  As Detlev pointed out, it breaks
the concept of I/O redirection through stdout.

> I agree this fix isn't the best, but its better than the bug in my
> opinion.  Ideally someone who uses the logbuff could provide a more
> elegant fix.  Any takers?

I don't want to replace one bug (that bites you) with another one (
that bites somebody else).

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I thought my people would grow tired of killing. But you were  right,
they  see it is easier than trading. And it has its pleasures. I feel
it myself. Like the hunt, but with richer rewards.
	-- Apella, "A Private Little War", stardate 4211.8

[U-Boot] [PATCH] logbuff: Prevent an infinite loop for console output

[Peter Tyser]

Hi Wolfgang,

> > I agree this fix isn't the best, but its better than the bug in my
> > opinion.  Ideally someone who uses the logbuff could provide a more
> > elegant fix.  Any takers?
> 
> I don't want to replace one bug (that bites you) with another one (
> that bites somebody else).

I think the bug that I'm introducing (continuing to use serial
output/ignoring the value of stdout) is much better than a hard lockup
(the current bug), but I definitely see your point.  I'm not a user of
the logbuff or LCDs, etc at this point in time, so feel free to do what
you'd like with the patch.

Best,
Peter