[U-Boot] [PATCH] logbuff: Prevent an infinite loop for console output
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; }
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
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
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.
- Most anything is better than the current board lockup that occurs
without the change.
Even if I was willing to buy this one...
- 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
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
participants (3)
-
Detlev Zundel -
Peter Tyser -
Wolfgang Denk