Hi Graeme,
On Mon, Oct 17, 2011 at 8:24 PM, Graeme Russ <graeme.r...@gmail.com> wrote:
> Hi Simon,
>
> On Tue, Oct 18, 2011 at 1:16 PM, Simon Glass <s...@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 <s...@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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
