[PATCH] common/console.c: prevent pre-console buffer contents from being added to itself

[Simon Glass]

Hi Rasmus,

On Tue, 23 Aug 2022 at 05:38, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> Ping.
>
> The previous patch has already been applied [cff29636933a,
> common/console.c: use CONFIG_VAL() with PRE_CON_BUF_* variables], and I
> realize this one might be more "controversial" in how it makes
> gd->precon_buf_idx serve as a "flag" when negative, but I'd really like
> to make progress towards making pre-console-buffering usable in SPL
> (since I'm debugging yet another issue where I need to know what
> happened very early).
>
> On 03/05/2022 15.13, Rasmus Villemoes wrote:
> > I do not have any non-serial output devices, so a
> > print_pre_console_buffer(PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL)
> > does nothing for me.
> >
> > However, I was manually inspected the pre-console buffer using md.b,
> > and I noticed that the early part of it was repeated. The reason is
> > that the first call of print_pre_console_buffer(), from
> > console_init_f(), ends up invoking puts() with the contents of the
> > buffer at that point, and puts() at that point ends up in the else
> > branch of
> >
> >       if (gd->flags & GD_FLG_DEVINIT) {
> >               /* Send to the standard output */
> >               fputs(stdout, s);
> >       } else {
> >               /* Send directly to the handler */
> >               pre_console_puts(s);
> >               serial_puts(s);
> >       }
> >
> > so indeed the contents is added again.
> >
> > That can be somewhat confusing (both when reading the buffer manually,
> > but also if it did actually come out on some device). So disable all
> > use of the pre-console buffer while print_pre_console_buffer() is
> > emitting it.
> >
> > Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
> > ---
> >  common/console.c                  | 10 +++++++++-
> >  include/asm-generic/global_data.h | 12 ++++++++----
> >  2 files changed, 17 insertions(+), 5 deletions(-)

Reviewed-by: Simon Glass <sjg at chromium.org>

Looking at the logic here, I wonder if we should use membuf, but I
suspect it would increase code size, s probably note.


> >
> > diff --git a/common/console.c b/common/console.c
> > index dc071f1ed6..c5a72d9a2a 100644
> > --- a/common/console.c
> > +++ b/common/console.c
> > @@ -599,6 +599,9 @@ static void pre_console_putc(const char c)
> >  {
> >       char *buffer;
> >
> > +     if (gd->precon_buf_idx < 0)
> > +             return;
> > +
> >       buffer = map_sysmem(CONFIG_VAL(PRE_CON_BUF_ADDR), CONFIG_VAL(PRE_CON_BUF_SZ));
> >
> >       buffer[CIRC_BUF_IDX(gd->precon_buf_idx++)] = c;
> > @@ -608,13 +611,16 @@ static void pre_console_putc(const char c)
> >
> >  static void pre_console_puts(const char *s)
> >  {
> > +     if (gd->precon_buf_idx < 0)
> > +             return;
> > +
> >       while (*s)
> >               pre_console_putc(*s++);
> >  }
> >
> >  static void print_pre_console_buffer(int flushpoint)
> >  {
> > -     unsigned long in = 0, out = 0;
> > +     long in = 0, out = 0;
> >       char buf_out[CONFIG_VAL(PRE_CON_BUF_SZ) + 1];
> >       char *buf_in;
> >
> > @@ -631,6 +637,7 @@ static void print_pre_console_buffer(int flushpoint)
> >
> >       buf_out[out] = 0;
> >
> > +     gd->precon_buf_idx = -1;
> >       switch (flushpoint) {
> >       case PRE_CONSOLE_FLUSHPOINT1_SERIAL:
> >               puts(buf_out);
> > @@ -639,6 +646,7 @@ static void print_pre_console_buffer(int flushpoint)
> >               console_puts_select(stdout, false, buf_out);
> >               break;
> >       }
> > +     gd->precon_buf_idx = in;
> >  }
> >  #else
> >  static inline void pre_console_putc(const char c) {}
> > diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> > index 805a6fd679..2a747d91e1 100644
> > --- a/include/asm-generic/global_data.h
> > +++ b/include/asm-generic/global_data.h
> > @@ -115,10 +115,14 @@ struct global_data {
> >       /**
> >        * @precon_buf_idx: pre-console buffer index
> >        *
> > -      * @precon_buf_idx indicates the current position of the buffer used to
> > -      * collect output before the console becomes available
> > -      */
> > -     unsigned long precon_buf_idx;
> > +      * @precon_buf_idx indicates the current position of the
> > +      * buffer used to collect output before the console becomes
> > +      * available. When negative, the pre-console buffer is
> > +      * temporarily disabled (used when the pre-console buffer is
> > +      * being written out, to prevent adding its contents to
> > +      * itself).
> > +      */
> > +     long precon_buf_idx;
> >  #endif
> >       /**
> >        * @env_addr: address of environment structure
>