| Message ID | 1318904186-12271-1-git-send-email-sjg@chromium.org |
|---|---|
| State | New, archived |
| Headers | show |
Hi Simon, On Tue, Oct 18, 2011 at 1:16 PM, Simon Glass <sjg@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@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 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 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? Regards, Graeme
Hi Graeme, On Mon, Oct 17, 2011 at 8:24 PM, Graeme Russ <graeme.russ@gmail.com> wrote: > Hi Simon, > > On Tue, Oct 18, 2011 at 1:16 PM, Simon Glass <sjg@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@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 >
Hi Simon, On Tue, Oct 18, 2011 at 3:45 PM, Simon Glass <sjg@chromium.org> wrote: > Hi Graeme, > > On Mon, Oct 17, 2011 at 8:24 PM, Graeme Russ <graeme.russ@gmail.com> wrote: >> Hi Simon, >> >> On Tue, Oct 18, 2011 at 1:16 PM, Simon Glass <sjg@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@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 >> 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) > 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
Dear Graeme Russ, In message <CALButCJO6yj5Kw-tx8Qva6qFUBE74Dc877dMpLSsRn_xNyFsVw@mail.gmail.com> you wrote: > > > void panic(const char *fmt, ...) > > { > > va_list args; > > + > > va_start(args, fmt); > > No need to add this extra blank line Oh, yes. We separate declarations and code by a blank line. Best regards, Wolfgang Denk
Dear Graeme Russ, In message <CALButC+LeU05dE0_p+GVeo5cQXkdbFm2hzz_LdyhnTtemiP++Q@mail.gmail.com> you wrote: > > > 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 "A little" like 16 bytes in a single location is OK i it helps to make the code cleaner. 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 Few people do business well who do nothing else. -- Philip Earl of Chesterfield
Hi Graeme, On Mon, Oct 17, 2011 at 10:02 PM, Graeme Russ <graeme.russ@gmail.com> wrote: > Hi Simon, > > On Tue, Oct 18, 2011 at 3:45 PM, Simon Glass <sjg@chromium.org> wrote: >> Hi Graeme, >> >> On Mon, Oct 17, 2011 at 8:24 PM, Graeme Russ <graeme.russ@gmail.com> wrote: >>> Hi Simon, >>> >>> On Tue, Oct 18, 2011 at 1:16 PM, Simon Glass <sjg@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@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 >
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); +#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();
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@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(-)