Patchwork [U-Boot,2/2] Add check that console is ready before output

login
register
mail settings
Submitter Simon Glass
Date Aug. 29, 2011, 4:05 p.m.
Message ID <1314633910-8550-3-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/112103/
State New, archived
Headers show

Comments

Simon Glass - Aug. 29, 2011, 4:05 p.m.
If puts() or printf() is called before the console is ready, U-Boot will
either hang or die. This adds a check for this so that debug() can be used
in early code without concern that it will hang.

U-Boot boots properly

Tested-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Simon Glass <sjg@chromium.org>
---
 common/console.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
Graeme Russ - Aug. 29, 2011, 11:16 p.m.
Hi Simon,

On Tue, Aug 30, 2011 at 2:05 AM, Simon Glass <sjg@chromium.org> wrote:
> If puts() or printf() is called before the console is ready, U-Boot will
> either hang or die. This adds a check for this so that debug() can be used
> in early code without concern that it will hang.
>
> U-Boot boots properly
>
> Tested-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  common/console.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/common/console.c b/common/console.c
> index 8c650e0..28ddb95 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -338,7 +338,7 @@ void putc(const char c)
>        if (gd->flags & GD_FLG_DEVINIT) {
>                /* Send to the standard output */
>                fputc(stdout, c);
> -       } else {
> +       } else if (gd->have_console) {
>                /* Send directly to the handler */
>                serial_putc(c);
>        }
> @@ -359,7 +359,7 @@ void puts(const char *s)
>        if (gd->flags & GD_FLG_DEVINIT) {
>                /* Send to the standard output */
>                fputs(stdout, s);
> -       } else {
> +       } else if (gd->have_console) {
>                /* Send directly to the handler */
>                serial_puts(s);
>        }

Please have a look at my console squelching patches

Regards,

Graeme
Wolfgang Denk - Oct. 17, 2011, 8:11 p.m.
Dear Simon Glass,

In message <1314633910-8550-3-git-send-email-sjg@chromium.org> you wrote:
> If puts() or printf() is called before the console is ready, U-Boot will
> either hang or die. This adds a check for this so that debug() can be used
> in early code without concern that it will hang.
> 
> U-Boot boots properly
> 
> Tested-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>  common/console.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)

Isn't this just hushing up implementation errors?

Before, the incorrect call would cause U-Boot to fail - a situation
which cannot be overlooked during the port, so the problem will
quickly be analyzed and fixed.

With your commit, everything appears to work fine, just some
(expected) output will be missing.  This is misleading at beats, and
will cause that buggy code will go undetected for a long time.

I don't think this is a good strategy.

Please comment.

Best regards,

Wolfgang Denk
Simon Glass - Oct. 17, 2011, 8:25 p.m.
Hi Wolfgang,

On Mon, Oct 17, 2011 at 1:11 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1314633910-8550-3-git-send-email-sjg@chromium.org> you wrote:
>> If puts() or printf() is called before the console is ready, U-Boot will
>> either hang or die. This adds a check for this so that debug() can be used
>> in early code without concern that it will hang.
>>
>> U-Boot boots properly
>>
>> Tested-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>  common/console.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> Isn't this just hushing up implementation errors?
>
> Before, the incorrect call would cause U-Boot to fail - a situation
> which cannot be overlooked during the port, so the problem will
> quickly be analyzed and fixed.
>
> With your commit, everything appears to work fine, just some
> (expected) output will be missing.  This is misleading at beats, and
> will cause that buggy code will go undetected for a long time.
>
> I don't think this is a good strategy.
>
> Please comment.

Yes indeed -  I did actually submit an 'early panic' patch which I
should take another look at - it is called 'Add board_panic_no_console
to deal with early critical errors'. I think it was in the same patch
set but I'm not sure.

It would panic when console output was performed before the console
was ready - and therefore give the behaviour you desire.

Regards,
Simon

>
> 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@denx.de
> Compassion -- that's the one things no machine ever had.  Maybe it's
> the one thing that keeps men ahead of them.
>        -- McCoy, "The Ultimate Computer", stardate 4731.3
>
Wolfgang Denk - Oct. 17, 2011, 8:37 p.m.
Dear Simon Glass,

In message <CAPnjgZ3WBmJBs1zCd4_jQsUZcO2BOAOK865CaL6tWeUX9zVVWg@mail.gmail.com> you wrote:
> 
> Yes indeed -  I did actually submit an 'early panic' patch which I
> should take another look at - it is called 'Add board_panic_no_console
> to deal with early critical errors'. I think it was in the same patch
> set but I'm not sure.

It was (I even commented on it).

> It would panic when console output was performed before the console
> was ready - and therefore give the behaviour you desire.

Well, but there will probably be a large number of boards that don't
implement this function (or don;t enable it if we make it
configurable).  And all these will now fail silently.

Best regards,

Wolfgang Denk
Simon Glass - Oct. 17, 2011, 8:38 p.m.
Hi Wolfgang,

On Mon, Oct 17, 2011 at 1:25 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Wolfgang,
>
> On Mon, Oct 17, 2011 at 1:11 PM, Wolfgang Denk <wd@denx.de> wrote:
>> Dear Simon Glass,
>>
>> In message <1314633910-8550-3-git-send-email-sjg@chromium.org> you wrote:
>>> If puts() or printf() is called before the console is ready, U-Boot will
>>> either hang or die. This adds a check for this so that debug() can be used
>>> in early code without concern that it will hang.
>>>
>>> U-Boot boots properly
>>>
>>> Tested-by: Simon Glass <sjg@chromium.org>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>  common/console.c |    4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> Isn't this just hushing up implementation errors?
>>
>> Before, the incorrect call would cause U-Boot to fail - a situation
>> which cannot be overlooked during the port, so the problem will
>> quickly be analyzed and fixed.
>>
>> With your commit, everything appears to work fine, just some
>> (expected) output will be missing.  This is misleading at beats, and
>> will cause that buggy code will go undetected for a long time.
>>
>> I don't think this is a good strategy.
>>
>> Please comment.
>
> Yes indeed -  I did actually submit an 'early panic' patch which I
> should take another look at - it is called 'Add board_panic_no_console
> to deal with early critical errors'. I think it was in the same patch
> set but I'm not sure.
>
> It would panic when console output was performed before the console
> was ready - and therefore give the behaviour you desire.

I took another look at this - in fact this patch has been entirely
superseded by Graeme's:

commit e3e454cd72f319908355427b1a3ae54b3dd53356
Author: Graeme Russ <graeme.russ@gmail.com>
Date:   Mon Aug 29 02:14:05 2011 +0000

    console: Squelch pre-console output in console functions

so there is no need for me to resubmit. The other patch I mentioned,
The board_panic_no_console patch is still of interest though.

Regards,
Simon

>
>>
>> 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@denx.de
>> Compassion -- that's the one things no machine ever had.  Maybe it's
>> the one thing that keeps men ahead of them.
>>        -- McCoy, "The Ultimate Computer", stardate 4731.3
>>
>
Simon Glass - Oct. 17, 2011, 8:42 p.m.
Hi Wolfgang,

On Mon, Oct 17, 2011 at 1:37 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ3WBmJBs1zCd4_jQsUZcO2BOAOK865CaL6tWeUX9zVVWg@mail.gmail.com> you wrote:
>>
>> Yes indeed -  I did actually submit an 'early panic' patch which I
>> should take another look at - it is called 'Add board_panic_no_console
>> to deal with early critical errors'. I think it was in the same patch
>> set but I'm not sure.
>
> It was (I even commented on it).
>
>> It would panic when console output was performed before the console
>> was ready - and therefore give the behaviour you desire.
>
> Well, but there will probably be a large number of boards that don't
> implement this function (or don;t enable it if we make it
> configurable).  And all these will now fail silently.

Sorry, just emailed again on this thread - actually the behaviour of
that patch has already come in due to Graeme's patch.

And I think it is actually the opposite - they will not fail silently,
but in fact ignore the pre-console printf() and continue. Previously
(before Graeme's patch and this one had it been applied) any
pre-console printf() would cause a silent failure / hang. I think this
was the intent of the fix.

The intent of my other patch in the series was to provide a way for a
board to panic before the console is ready, and hopefully get a
message to the user (flashing lights, LCD, init all the UARTs and
blast out a msg, etc.). It is true that most boards will not implement
this. We could perhaps look at implementing a weak function to do this
at the architecture level (but only for SOCs) but it seems risky.

Regards,
Simon

>
> 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@denx.de
> panic: kernel trap (ignored)
>

Patch

diff --git a/common/console.c b/common/console.c
index 8c650e0..28ddb95 100644
--- a/common/console.c
+++ b/common/console.c
@@ -338,7 +338,7 @@  void putc(const char c)
 	if (gd->flags & GD_FLG_DEVINIT) {
 		/* Send to the standard output */
 		fputc(stdout, c);
-	} else {
+	} else if (gd->have_console) {
 		/* Send directly to the handler */
 		serial_putc(c);
 	}
@@ -359,7 +359,7 @@  void puts(const char *s)
 	if (gd->flags & GD_FLG_DEVINIT) {
 		/* Send to the standard output */
 		fputs(stdout, s);
-	} else {
+	} else if (gd->have_console) {
 		/* Send directly to the handler */
 		serial_puts(s);
 	}