[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 10:02 PM, Graeme Russ <graeme.russ at gmail.com> wrote:
> Hi Simon,
>
> On Tue, Oct 18, 2011 at 3:45 PM, Simon Glass <sjg at chromium.org> wrote:
>> 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>
>
> [snip]
>
>>>
>>> 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).
>
> My squelch pre-console output and CONFIG_PRE_CONSOLE_BUFFER patches have
> fixed all that

Yes you are right, it is fine now,

>
>>> 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 :-)
>
> True, but console is meant to come up ASAP so pre-console output would (in
> theory) be minimal and only in abnormal conditions
>
>>> 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.
>
> Ah, I see - Well still, we could do something like the following in
> pre_console_putc()
>
> static void pre_console_putc(const char c)
> {
>        char *buffer = (char *)CONFIG_PRE_CON_BUF_ADDR;
>
>        buffer[CIRC_BUF_IDX(gd->precon_buf_idx++)] = c;
>
> #ifdef CONFIG_PRE_CONSOLE_PUTC
>        if (!gd->have_console)
>                board_pre_console_putc(str);
> #endif
> }
>
> It looks to me like there could be even smoother integration with
> CONFIG_PRE_CONSOLE_BUFFER (i.e. allow CONFIG_PRE_CONSOLE_PUTC
> with or without CONFIG_PRE_CONSOLE_BUFFER)

Well ok. This changes the patch pretty drastically but I'm fine with
that. The main point is to avoid a silent hang and it still does that.
I will send out a v3 patch along the lines you describe.

Thanks very much for looking at this. Between this and the pre-console
buffer I am now very happy with how we deal with early failures.

Regards,
Simon

>
>> 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.
>
> Thats a Wolfgang question, but I think it can be done without the overhead
>
> Regards,
>
> Graeme
>