[U-Boot] [PATCH 2/5] Add board_panic_no_console() to deal with early critical errors

[Simon Glass]

Hi Wolfgang,

On Wed, Mar 21, 2012 at 2:02 AM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Simon Glass,
>
> In message <1332188824-5447-2-git-send-email-sjg at chromium.org> you wrote:
>> This patch adds support for console output in the event of a panic() before
>> the console is inited. The main purpose of this is to deal with a very early
>> panic() which would otherwise cause a silent hang.
>>
>> A new board_pre_console_panic() 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 available UARTs and
>> output the string (plus a newline) if it possibly can.
>
> In addition to the more constructive comments made already by Grame, I
> object against this patch because I dislike the whole concept.

Well I don't disagree :-)

>
>> +- Pre-console panic():
>> +             Prior to the console being initialised, console output is
>> +             normally silently discarded. This can be annoying if a
>> +             panic() happens in this time. One reason for this might be
>
> True.  This is the reason why it has always been an important design
> rule for U-Boot to initialize the console port as soon as possible.
>
>> +             that you have CONFIG_OF_CONTROL defined but have not
>> +             provided a valid device tree for U-Boot. In general U-Boot's
>> +             console code cannot resolve this problem, since the console
>> +             has not been set up, and it may not be known which UART is
>> +             the console anyway (for example if this information is in
>> +             the device tree).
>
> Please make up your mind:  either you CAN initialize the console, then
> you can use it to output messages in a regular way, or you CANNOT
> initialize it, in which case you cannot print anything.  There is no
> third option.

Well that is very clear - we cannot. We hit panic() before
console_ready() is called.

>
>> +             The solution is to define a function called
>> +             board_pre_console_panic() for your board, which alerts the
>> +             user however it can. Hopefuly this will involve displaying
>> +             a message on available UARTs, or perhaps at least flashing
>
> If you have "available UARTs", you could use this as console, right?

Yes, if we knew which it was.

>
> In the previous discussion you explained the situation differently -
> you were talking about _any_ UARTs that might be present on the
> hardware, without caring about twhat these might actually be used for.

Yes, basically the only difference with this series is that the board
file can control what UARTs are used.

>
> I explained that such an approach is highly dangerous.  I do not want
> you toi set a precedent for such stle and code.

OK

>
>> +             an LED. The function should be very careful to only output
>> +             to UARTs that are known to be unused for peripheral
>> +             interfaces, and change GPIOs that will have no ill effect
>> +             on the board. That said, it is fine to do something
>> +             destructive that will prevent the board from continuing to
>> +             boot, since it isn't going to...
>
> Sorry, but this is bogus. Either you know the console port, or you
> don't.  If there is a free UART, it might be attached to custome
> hardware you have no idea about.  Ditto for GPIOs.  Please do not
> meddle with device state in arbitrary ways.  If there are LED ports on
> that board that are intended to signalize status information then it
> makes sense to use these - but do not use any other ports that might
> be present on the hardware.

Yes that is true. The board file should know what is safe, but when we
share board files among different hardware variants, we have exactly
this problem.

>
>> +             The function will need to output serial data directly,
>> +             since the console code is not functional yet. Note that if
>
> This is broken design.  If you can initialize the UART as console,
> then doi so and use it in the regular way.
>
>> +             the panic happens early enough, then it is possible that
>> +             board_init_f() (or even arch_cpu_init() on ARM) has not
>> +             been called yet. You should init all clocks, GPIOs, etc.
>> +             that are needed to get the string out. Baud rates will need
>> +             to default to something sensible.
>
> Again, this is broken design.  We cannot try to catch errors sooner
> and sooner and soner, and if needed initialization steps have not been
> executed yet, provide additional code for emergency initialization.
> We have regular console code, and now we have pre_console_*() stuff,
> and soon we will have pre_pre_pre_pre_pre_pre_pre_pre_console_*()
> stuff ? NAK.
>
> Just stick to the design principles, and make sure there is as few
> stuff that could possibly go wrong before console initialization as
> possible.  Than all this crap (excuse me, but I don;t find a better
> word) will not be needed.

Fair enough, and agreed. Thanks for looking at this and providing this info.

We know we won't get to console init in this case - there is a panic()
that happens first. So the existing pre-console putc() becomes our
route to displaying a message. The problem with that is only that the
board init hasn't happened yet, so either the pre-console putc() must
init the UARTs or we need a separate init function.

>
>
> I also dislike the modifications to the common code.
>
>
> I think you are trying to solve an unsolvable problem.  If you cannot
> accept that the board gets stuck without printing anything on the
> debug console port because there are situations when you don't know
> which port this is, then you simply should _define_ and _document_ a
> single port as debug console port.  Then initialize and use this in
> the regular way.  If later information (like from a loaded device
> tree) means the console gets switched to anothe rport, that's OK.
> That's what Linux will do as well if you assign another console port
> there.

Yes we are certainly trying to solve an unsolvable problem. There are
some things that will result in a bricked board and we can't solve all
of them. I would be very happy to just accept that. We have the
pre-console stuff which boards can use to do their best, and that
should be good enough for those that care enough. I actually prefer a
pre-console panic to a pre-console putc(), for reasons I went into at
length, but no matter, it's not that important.

So the existing pre-console putc() can be used, if we can sort out how
to make UART init work. Graeme suggested an approach here - I will see
if I can make that work.

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 at denx.de
> Gods don't like people not doing much work. People  who  aren't  busy
> all the time might start to _think_.  - Terry Pratchett, _Small Gods_