[U-Boot] [PATCH v2] Add board_panic_no_console to deal with early critical errors

[Simon Glass]

Hi Graeme,

On Mon, Oct 17, 2011 at 8:24 PM, Graeme Russ <graeme.russ at gmail.com> wrote:
> Hi Simon,
>
> On Tue, Oct 18, 2011 at 1:16 PM, Simon Glass <sjg at chromium.org> wrote:
>> This patch adds support for calling panic() before stdio is initialized.
>> At present this will just hang with little or no indication of what the
>> problem is.
>>
>> A new board_panic_no_console() function is added to the board API. If provided
>> by the board it will be called in the event of a panic before the console is
>> ready. This function should turn on all UARTS and spray the message out if
>> it possibly can.
>>
>> The feature is controlled by a new CONFIG_PRE_CONSOLE_PANIC option.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>> Changes in v2:
>> - Made this feature conditional on CONFIG_PRE_CONSOLE_PANIC
>>
>>  README           |   11 +++++++++++
>>  include/common.h |    8 ++++++++
>>  lib/vsprintf.c   |   29 +++++++++++++++++++++++++++++
>>  3 files changed, 48 insertions(+), 0 deletions(-)
>>
>> diff --git a/README b/README
>> index eb9ade9..0748a6f 100644
>> --- a/README
>> +++ b/README
>> @@ -634,6 +634,17 @@ The following options need to be configured:
>>                'Sane' compilers will generate smaller code if
>>                CONFIG_PRE_CON_BUF_SZ is a power of 2
>>
>> +- Pre-console Panic:
>> +               Prior to the console being initialised, since console output
>> +               is silently discarded, a panic() will cause no output and no
>> +               indication of what is wrong. However, if the
>> +               CONFIG_PRE_CONSOLE_PANIC option is defined, then U-Boot will
>> +               call board_panic_no_console() in this case, passing the panic
>> +               message. This function should try to output the message if
>> +               possible, perhaps on all available UARTs (it will need to do
>> +               this directly, since the console code is not functional yet).
>> +               Another option is to flash some LEDs to indicate a panic.
>> +
>>  - Boot Delay:  CONFIG_BOOTDELAY - in seconds
>>                Delay before automatically booting the default image;
>>                set to -1 to disable autoboot.
>> diff --git a/include/common.h b/include/common.h
>> index 4c3e3a6..acc4030 100644
>> --- a/include/common.h
>> +++ b/include/common.h
>> @@ -277,6 +277,14 @@ int        last_stage_init(void);
>>  extern ulong monitor_flash_len;
>>  int mac_read_from_eeprom(void);
>>
>> +/*
>> + * Called during a panic() when no console is available. The board should do
>> + * its best to get a message to the user any way it can. This function should
>> + * return if it can, in which case the system will either hang or reset.
>> + * See panic().
>> + */
>> +void board_panic_no_console(const char *str);
>> +
>>  /* common/flash.c */
>>  void flash_perror (int);
>>
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index 79dead3..56a2aef 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -24,6 +24,8 @@
>>  # define NUM_TYPE long long
>>  #define noinline __attribute__((noinline))
>>
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>>  const char hex_asc[] = "0123456789abcdef";
>>  #define hex_asc_lo(x)   hex_asc[((x) & 0x0f)]
>>  #define hex_asc_hi(x)   hex_asc[((x) & 0xf0) >> 4]
>> @@ -714,12 +716,39 @@ int sprintf(char * buf, const char *fmt, ...)
>>        return i;
>>  }
>>
>> +#ifdef CONFIG_PRE_CONSOLE_PANIC
>> +/* Provide a default function for when no console is available */
>> +static void __board_panic_no_console(const char *msg)
>> +{
>> +       /* Just return since we can't access the console */
>> +}
>> +
>> +void board_panic_no_console(const char *msg) __attribute__((weak,
>> +                                       alias("__board_panic_no_console")));
>> +#endif
>> +
>>  void panic(const char *fmt, ...)
>>  {
>>        va_list args;
>> +
>>        va_start(args, fmt);
>
> No need to add this extra blank line
>
>> +#ifdef CONFIG_PRE_CONSOLE_PANIC
>> +       {
>> +               char str[CONFIG_SYS_PBSIZE];
>> +
>> +               /* TODO)sjg): Move to vsnprintf() when available */
>> +               vsprintf(str, fmt, args);
>> +               if (gd->have_console) {
>> +                       puts(str);
>> +                       putc('\n');
>> +               } else {
>> +                       board_panic_no_console(str);
>> +               }
>> +       }
>> +#else
>>        vprintf(fmt, args);
>>        putc('\n');
>> +#endif
>>        va_end(args);
>>  #if defined (CONFIG_PANIC_HANG)
>>        hang();
>
> Hmm, why not:
>
>
>        char str[CONFIG_SYS_PBSIZE];
>
>        vsprintf(str, fmt, args);
>        puts(str);
>        putc('\n');
>
> #ifdef CONFIG_PRE_CONSOLE_PANIC
>        if (!gd->have_console)
>                board_panic_no_console(str);
> #endif
>
> Since puts() and putc() will, effectively, be NOPs if !gd->have_console

Thanks for looking at this.

I was trying to avoid any code size increase, which is why I just
#ifdefed the whole thing (yuk!). The above code increases code size by
16 bytes on ARM, due to the extra parameter, etc.

I can't test for sure on upstream/master right now but one thing that
causes an panic is trying to write to the console before it is ready -
see the get_console() function in common/console.c. So calling
puts/putc from within panic might again look for a console and again
panic (infinite loop).

>
> Actually, this could be made generic - board_puts_no_console() and move
> into console.c - If a board has a way of generating pre-console output
> then that can be utilised for all pre-console messaging, not just panic
> messages

Yes it could. However the board panic function could actually be quite
destructive. For example it might have to sit there for ages flashing
lights to indicate a panic. It isn't intended as a real printf(). If
you had one of those ready, you would have used it :-)

>
> Now it would be interesting to see what the compiler would do if you
> dropped CONFIG_PRE_CONSOLE_PANIC and just had in console.c:
>
>        if (!gd->have_console)
>                board_puts_no_console(str);
>
>
> with no board_puts_no_console() - Would it see the empty weak function
> and optimise the whole lot out?

The compiler can't do this (sort of by definition of 'weak'). The
linker could, but I don't think it does - you always end up with
something - I suppose since it doesn't know whether your weak function
is doing something or not, so doesn't optimise it out. We could use a
weak function pointer and check if it is zero before calling it (=code
size). I vaguely recall a linker option which can do something clever
hear but can't remember what it was.

So in summary - please let me know if 16 bytes is worth worrying
about. If it is fine to increase code size a little, then I will redo
this patch to clean it up at little as you suggest.

Regards,
Simon

>
> Regards,
>
> Graeme
>