[PATCH] common/console.c: use CONFIG_VAL() with PRE_CON_BUF_* variables

[PATCH] common/console.c: use CONFIG_VAL() with PRE_CON_BUF_* variables

[Rasmus Villemoes]

There is currently no support for PRE_CONSOLE_BUFFER in SPL, but if
and when that gets implemented, one would almost certainly want to use
a different address and/or size for the buffer (e.g., U-Boot proper
might specify an address in DRAM and a generous buffer, while SPL
would be much more constrained).

So a prerequisite for adding SPL_PRE_CONSOLE_BUFFER is to make the
code use SPL_-specific values. No functional change.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 common/console.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/common/console.c b/common/console.c
index 92b1c93be1..dc071f1ed6 100644
--- a/common/console.c
+++ b/common/console.c
@@ -593,13 +593,13 @@ int tstc(void)
 #define PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL	1
 
 #if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER)
-#define CIRC_BUF_IDX(idx) ((idx) % (unsigned long)CONFIG_PRE_CON_BUF_SZ)
+#define CIRC_BUF_IDX(idx) ((idx) % (unsigned long)CONFIG_VAL(PRE_CON_BUF_SZ))
 
 static void pre_console_putc(const char c)
 {
 	char *buffer;
 
-	buffer = map_sysmem(CONFIG_PRE_CON_BUF_ADDR, CONFIG_PRE_CON_BUF_SZ);
+	buffer = map_sysmem(CONFIG_VAL(PRE_CON_BUF_ADDR), CONFIG_VAL(PRE_CON_BUF_SZ));
 
 	buffer[CIRC_BUF_IDX(gd->precon_buf_idx++)] = c;
 
@@ -615,15 +615,15 @@ static void pre_console_puts(const char *s)
 static void print_pre_console_buffer(int flushpoint)
 {
 	unsigned long in = 0, out = 0;
-	char buf_out[CONFIG_PRE_CON_BUF_SZ + 1];
+	char buf_out[CONFIG_VAL(PRE_CON_BUF_SZ) + 1];
 	char *buf_in;
 
 	if (IS_ENABLED(CONFIG_SILENT_CONSOLE) && (gd->flags & GD_FLG_SILENT))
 		return;
 
-	buf_in = map_sysmem(CONFIG_PRE_CON_BUF_ADDR, CONFIG_PRE_CON_BUF_SZ);
-	if (gd->precon_buf_idx > CONFIG_PRE_CON_BUF_SZ)
-		in = gd->precon_buf_idx - CONFIG_PRE_CON_BUF_SZ;
+	buf_in = map_sysmem(CONFIG_VAL(PRE_CON_BUF_ADDR), CONFIG_VAL(PRE_CON_BUF_SZ));
+	if (gd->precon_buf_idx > CONFIG_VAL(PRE_CON_BUF_SZ))
+		in = gd->precon_buf_idx - CONFIG_VAL(PRE_CON_BUF_SZ);
 
 	while (in < gd->precon_buf_idx)
 		buf_out[out++] = buf_in[CIRC_BUF_IDX(in++)];
-- 
2.31.1

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

[Rasmus Villemoes]

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@prevas.dk>
---
 common/console.c                  | 10 +++++++++-
 include/asm-generic/global_data.h | 12 ++++++++----
 2 files changed, 17 insertions(+), 5 deletions(-)

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
-- 
2.31.1

Re: [PATCH] common/console.c: use CONFIG_VAL() with PRE_CON_BUF_* variables

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 637 bytes --]

On Tue, May 03, 2022 at 02:37:39PM +0200, Rasmus Villemoes wrote:

> There is currently no support for PRE_CONSOLE_BUFFER in SPL, but if
> and when that gets implemented, one would almost certainly want to use
> a different address and/or size for the buffer (e.g., U-Boot proper
> might specify an address in DRAM and a generous buffer, while SPL
> would be much more constrained).
> 
> So a prerequisite for adding SPL_PRE_CONSOLE_BUFFER is to make the
> code use SPL_-specific values. No functional change.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Rasmus Villemoes]

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@prevas.dk>
> ---
>  common/console.c                  | 10 +++++++++-
>  include/asm-generic/global_data.h | 12 ++++++++----
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> 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

Re: [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@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@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@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
>

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

On Tue, May 03, 2022 at 03:13:27PM +0200, 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@prevas.dk>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]