debug uart: don't print before initialization

PATCH New Simon Goldschmidt 2 participants 8 messages Oct 7, 2018 – Oct 19, 2018 Generic include/asm header files

Share & tools

Simon Goldschmidt

author

2018-10-07 17:52 UTC #0 lore raw

PATCH 1/1 debug uart: don't print before initialization

At least on socfpga gen5, _debug_uart_putc() can be called before debug_uart_init(), which leaves us stuck in an infinite loop in the ns16550 debug uart driver.

Since this prevents debugging startup problems instead of helping, let's add a field to 'gd' that prevents calling the _debug_uart_putc() until debug_uart_init() has been called.

Signed-off-by Simon Goldschmidt <[email protected]>

---

 include/asm-generic/global_data.h |  3 +++
 include/debug_uart.h              | 19 ++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

include/asm-generic/global_data.h +3 -0

index c83fc01b76..9de7f48476 100644
@@ -122,6 +122,9 @@ typedef struct global_data {
 	struct list_head log_head;	/* List of struct log_device */
 	int log_fmt;			/* Mask containing log format info */
 #endif
+#ifdef CONFIG_DEBUG_UART
+	int debug_uart_initialized;	/* No print before debug_uart_init */
+#endif
 } gd_t;
 #endif

include/debug_uart.h +14 -5

index 34e8b2fc81..4133c5612c 100644
@@ -128,9 +128,12 @@ void printhex8(uint value);
 \
 	static inline void _printch(int ch) \
 	{ \
-		if (ch == '\n') \
-			_debug_uart_putc('\r'); \
-		_debug_uart_putc(ch); \
+		DECLARE_GLOBAL_DATA_PTR; \
+		if (gd->debug_uart_initialized) { \
+			if (ch == '\n') \
+				_debug_uart_putc('\r'); \
+			_debug_uart_putc(ch); \
+		} \
 	} \
 \
 	void printch(int ch) \

@@ -146,8 +149,12 @@ void printhex8(uint value);
 \
 	static inline void printhex1(uint digit) \
 	{ \
-		digit &= 0xf; \
-		_debug_uart_putc(digit > 9 ? digit - 10 + 'a' : digit + '0'); \
+		DECLARE_GLOBAL_DATA_PTR; \
+		if (gd->debug_uart_initialized) { \
+			digit &= 0xf; \
+			_debug_uart_putc(digit > 9 ? digit - 10 + 'a' : \
+					 digit + '0'); \
+		} \
 	} \
 \
 	static inline void printhex(uint value, int digits) \

@@ -173,8 +180,10 @@ void printhex8(uint value);
 \
 	void debug_uart_init(void) \
 	{ \
+		DECLARE_GLOBAL_DATA_PTR; \
 		board_debug_uart_init(); \
 		_debug_uart_init(); \
+		gd->debug_uart_initialized = 1; \
 		_DEBUG_UART_ANNOUNCE \
 	} \

Signature · 1 line

2.17.1

Simon Glass

2018-10-09 03:40 UTC #1 lore raw

PATCH 1/1 debug uart: don't print before initialization

Hi,

Quoted 3 lines · Simon Goldschmidt

At least on socfpga gen5, _debug_uart_putc() can be called before debug_uart_init(), which leaves us stuck in an infinite loop in the ns16550 debug uart driver.

Can you fix that? That is a bug.

Quoted 18 lines

Since this prevents debugging startup problems instead of helping, let's add a field to 'gd' that prevents calling the _debug_uart_putc() until debug_uart_init() has been called.
Signed-off-by Simon Goldschmidt <[email protected]>
---
 include/asm-generic/global_data.h |  3 +++
 include/debug_uart.h              | 19 ++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)
include/asm-generic/global_data.h +0 -0
index c83fc01b76..9de7f48476 100644

include/asm-generic/global_data.h typedef struct global_data { · line 127

Quoted 6 lines

include/asm-generic/global_data.h +3 -0

@@ -122,6 +122,9 @@ typedef struct global_data {
        struct list_head log_head;      /* List of struct log_device */
        int log_fmt;                    /* Mask containing log format info */
 #endif
+#ifdef CONFIG_DEBUG_UART
+       int debug_uart_initialized;     /* No print before debug_uart_init */
+#endif

There is no requirement that gd be set up before the debug UART is running. It certainly isn't on the few platforms I know about.

Regards,
Simon

Simon Goldschmidt

author

2018-10-09 05:06 UTC #2 lore raw

PATCH 1/1 debug uart: don't print before initialization

Quoted 10 lines · Simon Glass

Hi,
Quoted 3 lines · Simon Goldschmidt
At least on socfpga gen5, _debug_uart_putc() can be called before debug_uart_init(), which leaves us stuck in an infinite loop in the ns16550 debug uart driver.
Can you fix that? That is a bug.

I already posted a patch for that but it was rejected:
http://patchwork.ozlabs.org/patch/955765/

As patman automatically choses the CC addresses, you weren't on the CC list back then, since that patch covered different filfes.

Simon

Quoted 31 lines

Quoted 19 lines
Since this prevents debugging startup problems instead of helping, let's add a field to 'gd' that prevents calling the _debug_uart_putc() until debug_uart_init() has been called.
Signed-off-by Simon Goldschmidt <[email protected]>
---
 include/asm-generic/global_data.h |  3 +++
 include/debug_uart.h              | 19 ++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)
include/asm-generic/global_data.h +0 -0
index c83fc01b76..9de7f48476 100644
include/asm-generic/global_data.h typedef struct global_data { · line 127
Quoted 6 lines
include/asm-generic/global_data.h +3 -0
@@ -122,6 +122,9 @@ typedef struct global_data {
        struct list_head log_head;      /* List of struct log_device */
        int log_fmt;                    /* Mask containing log format info */
 #endif
+#ifdef CONFIG_DEBUG_UART
+       int debug_uart_initialized;     /* No print before debug_uart_init */
+#endif
There is no requirement that gd be set up before the debug UART is running. It certainly isn't on the few platforms I know about.
Regards,
Simon

Simon Goldschmidt

author

2018-10-10 13:28 UTC #3 lore raw

PATCH 1/1 debug uart: don't print before initialization

+ Marek (as he commented on the original patch
http://patchwork.ozlabs.org/patch/955765/)

Quoted 11 lines · Simon Goldschmidt

Quoted 8 lines · Simon Glass
Hi,
Quoted 3 lines · Simon Goldschmidt
At least on socfpga gen5, _debug_uart_putc() can be called before debug_uart_init(), which leaves us stuck in an infinite loop in the ns16550 debug uart driver.
Can you fix that? That is a bug.
I already posted a patch for that but it was rejected:
http://patchwork.ozlabs.org/patch/955765/

I'd have to add I still thing that that patch is good to fix this:
It checks that the baudrate divisor is set, which effectively is an 'enable' bit for this hardware.

There were comments about the style (we can talk about that) and Marek rejected it because he wanted a generic solution. But honestly, given your idea that some platforms init the debug uart before setting up gd, I don't think I can find a solution for this.

So I'd really like to get my original patch applied (see above).

Simon

Quoted 35 lines

As patman automatically choses the CC addresses, you weren't on the CC list back then, since that patch covered different filfes.
Simon
Quoted 28 lines
Quoted 17 lines
Since this prevents debugging startup problems instead of helping, let's add a field to 'gd' that prevents calling the _debug_uart_putc() until debug_uart_init() has been called.
Signed-off-by Simon Goldschmidt <[email protected]>
---
  include/asm-generic/global_data.h |  3 +++
  include/debug_uart.h              | 19 ++++++++++++++-----
  2 files changed, 17 insertions(+), 5 deletions(-)
include/asm-generic/global_data.h +0 -0
index c83fc01b76..9de7f48476 100644
include/asm-generic/global_data.h typedef struct global_data { · line 127
Quoted 6 lines
include/asm-generic/global_data.h +3 -0
@@ -122,6 +122,9 @@ typedef struct global_data {
         struct list_head log_head;      /* List of struct log_device */
         int log_fmt;                    /* Mask containing log format info */
  #endif
+#ifdef CONFIG_DEBUG_UART
+       int debug_uart_initialized;     /* No print before debug_uart_init */
+#endif
There is no requirement that gd be set up before the debug UART is running. It certainly isn't on the few platforms I know about.
Regards,
Simon

Simon Glass

2018-10-10 20:03 UTC #4 lore raw

PATCH 1/1 debug uart: don't print before initialization

Hi Simon,

Quoted 26 lines · Simon Goldschmidt

+ Marek (as he commented on the original patch http://patchwork.ozlabs.org/patch/955765/)
Quoted 8 lines · Simon Goldschmidt
Quoted 6 lines · Simon Glass
Hi,
Quoted 1 line · Simon Goldschmidt
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
At least on socfpga gen5, _debug_uart_putc() can be called before debug_uart_init(), which leaves us stuck in an infinite loop in the ns16550 debug uart driver.
Can you fix that? That is a bug.
I already posted a patch for that but it was rejected:
http://patchwork.ozlabs.org/patch/955765/
I'd have to add I still thing that that patch is good to fix this:
It checks that the baudrate divisor is set, which effectively is an 'enable' bit for this hardware.

Quoted 7 lines

There were comments about the style (we can talk about that) and Marek rejected it because he wanted a generic solution. But honestly, given your idea that some platforms init the debug uart before setting up gd, I don't think I can find a solution for this.
So I'd really like to get my original patch applied (see above).

Yes I think your patch is best - it is specific to the hardware which is the only way you can safely detect whether the UART is enabled.

We cannot rely on state like global_data to tell if the debug UART is enabled. We could use a flag in the DATA section on some devices, but that is also pretty nasty and we've been trying to avoid adding such things and using global_data instead.

So I am OK with the original patch.

However before going further I'd like to understand your thoughts on this question: I feel that you are checking for something that should not happen - i.e. the debug UART must be inited before use, just like any other device. We don't in general put these sorts of checks into other drivers. Why is the debug UART different?

Regards,
Simon

Quoted 43 lines

Simon
Quoted 36 lines
As patman automatically choses the CC addresses, you weren't on the CC list back then, since that patch covered different filfes.
Simon
Quoted 29 lines
Quoted 17 lines
Since this prevents debugging startup problems instead of helping, let's add a field to 'gd' that prevents calling the _debug_uart_putc() until debug_uart_init() has been called.
Signed-off-by Simon Goldschmidt <[email protected]>
---
  include/asm-generic/global_data.h |  3 +++
  include/debug_uart.h              | 19 ++++++++++++++-----
  2 files changed, 17 insertions(+), 5 deletions(-)
include/asm-generic/global_data.h +0 -0
index c83fc01b76..9de7f48476 100644
include/asm-generic/global_data.h typedef struct global_data { · line 127
Quoted 6 lines
include/asm-generic/global_data.h +3 -0
@@ -122,6 +122,9 @@ typedef struct global_data {
         struct list_head log_head;      /* List of struct log_device */
         int log_fmt;                    /* Mask containing log format info */
  #endif
+#ifdef CONFIG_DEBUG_UART
+       int debug_uart_initialized;     /* No print before debug_uart_init */
+#endif
There is no requirement that gd be set up before the debug UART is running. It certainly isn't on the few platforms I know about.
Regards,
Simon

Simon Goldschmidt

author

2018-10-10 20:16 UTC #5 lore raw

PATCH 1/1 debug uart: don't print before initialization

Quoted 47 lines · Simon Glass

Hi Simon,
Quoted 26 lines · Simon Goldschmidt
+ Marek (as he commented on the original patch http://patchwork.ozlabs.org/patch/955765/)
Quoted 8 lines · Simon Goldschmidt
Quoted 6 lines · Simon Glass
Hi,
Quoted 1 line · Simon Goldschmidt
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
At least on socfpga gen5, _debug_uart_putc() can be called before debug_uart_init(), which leaves us stuck in an infinite loop in the ns16550 debug uart driver.
Can you fix that? That is a bug.
I already posted a patch for that but it was rejected:
http://patchwork.ozlabs.org/patch/955765/
I'd have to add I still thing that that patch is good to fix this:
It checks that the baudrate divisor is set, which effectively is an 'enable' bit for this hardware.
Quoted 7 lines
There were comments about the style (we can talk about that) and Marek rejected it because he wanted a generic solution. But honestly, given your idea that some platforms init the debug uart before setting up gd, I don't think I can find a solution for this.
So I'd really like to get my original patch applied (see above).
Yes I think your patch is best - it is specific to the hardware which is the only way you can safely detect whether the UART is enabled.
We cannot rely on state like global_data to tell if the debug UART is enabled. We could use a flag in the DATA section on some devices, but that is also pretty nasty and we've been trying to avoid adding such things and using global_data instead.

Quoted 7 lines

So I am OK with the original patch.
However before going further I'd like to understand your thoughts on this question: I feel that you are checking for something that should not happen - i.e. the debug UART must be inited before use, just like any other device. We don't in general put these sorts of checks into other drivers. Why is the debug UART different?

On this specific platform (socfpga cylcone5/gen5), the preloader calls a function before initializing the debug uart that is also called later and that should emit an error in one case. And if this is the case (which it can be depending on the boot source), then the system will hang when debug uart is enabled (loop forever in the debug uart output code).

Now you can say we can initialize the debug uart earlier on this platform, but that doesn't really work as it could be that the above function enables a bus that is required for the debug uart (on socfpga, I could well imagine the debug being implemented in FPGA and the offending function enables the FPGA bridge).

Then you could argue to remove the offending printf, but I feel that debug uart should help in debugging, not prevent it by entering an infinite loop when printf is called at the wrong time.

I just feel it's better to lose some printf messages at the start than adding a hang that's not present without debug uart.

And as much as I'd love to solve this in a general way, the multitudes of different startup code does not seem to leave an easy way to solve this other than I did...?

Simon

Quoted 47 lines

Regards,
Simon
Quoted 43 lines
Simon
Quoted 36 lines
As patman automatically choses the CC addresses, you weren't on the CC list back then, since that patch covered different filfes.
Simon
Quoted 29 lines
Quoted 17 lines
Since this prevents debugging startup problems instead of helping, let's add a field to 'gd' that prevents calling the _debug_uart_putc() until debug_uart_init() has been called.
Signed-off-by Simon Goldschmidt <[email protected]>
---
  include/asm-generic/global_data.h |  3 +++
  include/debug_uart.h              | 19 ++++++++++++++-----
  2 files changed, 17 insertions(+), 5 deletions(-)
include/asm-generic/global_data.h +0 -0
index c83fc01b76..9de7f48476 100644
include/asm-generic/global_data.h typedef struct global_data { · line 127
Quoted 6 lines
include/asm-generic/global_data.h +3 -0
@@ -122,6 +122,9 @@ typedef struct global_data {
         struct list_head log_head;      /* List of struct log_device */
         int log_fmt;                    /* Mask containing log format info */
  #endif
+#ifdef CONFIG_DEBUG_UART
+       int debug_uart_initialized;     /* No print before debug_uart_init */
+#endif
There is no requirement that gd be set up before the debug UART is running. It certainly isn't on the few platforms I know about.
Regards,
Simon

Simon Goldschmidt

author

2018-10-17 20:02 UTC #6 lore raw

PATCH 1/1 debug uart: don't print before initialization

Simon Goldschmidt <[email protected]> schrieb am Mi., 10. Okt. 2018, 22:16:

Quoted 73 lines · Simon Glass

Quoted 7 lines
Hi Simon,
Quoted 2 lines · Simon Goldschmidt
+ Marek (as he commented on the original patch
http://patchwork.ozlabs.org/patch/955765/)
Quoted 40 lines
Quoted 24 lines · Simon Goldschmidt
Quoted 8 lines · Simon Glass
Quoted 6 lines
Hi,
Quoted 1 line · Simon Goldschmidt
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
At least on socfpga gen5, _debug_uart_putc() can be called before debug_uart_init(), which leaves us stuck in an infinite loop in the ns16550 debug uart driver.
Can you fix that? That is a bug.
I already posted a patch for that but it was rejected:
http://patchwork.ozlabs.org/patch/955765/
I'd have to add I still thing that that patch is good to fix this:
It checks that the baudrate divisor is set, which effectively is an 'enable' bit for this hardware.
Quoted 7 lines
There were comments about the style (we can talk about that) and Marek rejected it because he wanted a generic solution. But honestly, given your idea that some platforms init the debug uart before setting up gd, I don't think I can find a solution for this.
So I'd really like to get my original patch applied (see above).
Yes I think your patch is best - it is specific to the hardware which is the only way you can safely detect whether the UART is enabled.
We cannot rely on state like global_data to tell if the debug UART is enabled. We could use a flag in the DATA section on some devices, but that is also pretty nasty and we've been trying to avoid adding such things and using global_data instead.
Quoted 7 lines
So I am OK with the original patch.
However before going further I'd like to understand your thoughts on this question: I feel that you are checking for something that should not happen - i.e. the debug UART must be inited before use, just like any other device. We don't in general put these sorts of checks into other drivers. Why is the debug UART different?
On this specific platform (socfpga cylcone5/gen5), the preloader calls a function before initializing the debug uart that is also called later and that should emit an error in one case. And if this is the case (which it can be depending on the boot source), then the system will hang when debug uart is enabled (loop forever in the debug uart output code).
Now you can say we can initialize the debug uart earlier on this platform, but that doesn't really work as it could be that the above function enables a bus that is required for the debug uart (on socfpga, I could well imagine the debug being implemented in FPGA and the offending function enables the FPGA bridge).
Then you could argue to remove the offending printf, but I feel that debug uart should help in debugging, not prevent it by entering an infinite loop when printf is called at the wrong time.

Quoted 8 lines

I just feel it's better to lose some printf messages at the start than adding
a hang that's not present without debug uart.
And as much as I'd love to solve this in a general way, the multitudes of different startup code does not seem to leave an easy way to solve this other than I did...?

So, without further answers, can we push the original patch? (
http://patchwork.ozlabs.org/patch/955765/ )

Simon

Quoted 56 lines

Simon
Quoted 20 lines
Regards,
Simon
Quoted 16 lines
Simon
Quoted 12 lines
As patman automatically choses the CC addresses, you weren't on the CC list back then, since that patch covered different filfes.
Simon
Quoted 5 lines
Quoted 5 lines
Since this prevents debugging startup problems instead of helping, let's add a field to 'gd' that prevents calling the _debug_uart_putc() until debug_uart_init() has been called.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Review
Quoted 8 lines
Signed-off-by Simon Goldschmidt <[email protected]>
---
  include/asm-generic/global_data.h |  3 +++
  include/debug_uart.h              | 19 ++++++++++++++-----
  2 files changed, 17 insertions(+), 5 deletions(-)
(quoted diff) +0 -0
b/include/asm-generic/global_data.h
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
include/asm-generic/global_data.h typedef struct global_data { · line 122
Quoted 5 lines
index c83fc01b76..9de7f48476 100644
include/asm-generic/global_data.h +0 -0
@@ -122,6 +122,9 @@ typedef struct global_data {
         struct list_head log_head;      /* List of struct
log_device */
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
include/asm-generic/global_data.h typedef struct global_data { · line 123
Quoted 1 line
include/asm-generic/global_data.h +0 -0
         int log_fmt;                    /* Mask containing log
format info */
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
include/asm-generic/global_data.h typedef struct global_data {
Quoted 3 lines
include/asm-generic/global_data.h +2 -0
  #endif
+#ifdef CONFIG_DEBUG_UART
+       int debug_uart_initialized;     /* No print before
debug_uart_init */
Quoted 12 lines
Quoted 9 lines
Quoted 7 lines
Quoted 7 lines
include/asm-generic/global_data.h typedef struct global_data { · line 127
Quoted 1 line
include/asm-generic/global_data.h +1 -0
+#endif
There is no requirement that gd be set up before the debug UART is running. It certainly isn't on the few platforms I know about.
Regards,
Simon

Simon Glass

2018-10-19 03:25 UTC #7 lore raw

PATCH 1/1 debug uart: don't print before initialization

Hi Simon,

Quoted 76 lines · Simon Goldschmidt

Simon Goldschmidt <[email protected]> schrieb am Mi., 10. Okt. 2018, 22:16:
Quoted 72 lines · Simon Glass
Quoted 48 lines
Hi Simon,
Quoted 27 lines · Simon Goldschmidt
+ Marek (as he commented on the original patch
http://patchwork.ozlabs.org/patch/955765/)
Quoted 8 lines · Simon Goldschmidt
Quoted 6 lines · Simon Glass
Hi,
Quoted 1 line · Simon Goldschmidt
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
At least on socfpga gen5, _debug_uart_putc() can be called before debug_uart_init(), which leaves us stuck in an infinite loop in the ns16550 debug uart driver.
Can you fix that? That is a bug.
I already posted a patch for that but it was rejected:
http://patchwork.ozlabs.org/patch/955765/
I'd have to add I still thing that that patch is good to fix this:
It checks that the baudrate divisor is set, which effectively is an 'enable' bit for this hardware.
Quoted 7 lines
There were comments about the style (we can talk about that) and Marek rejected it because he wanted a generic solution. But honestly, given your idea that some platforms init the debug uart before setting up gd, I don't think I can find a solution for this.
So I'd really like to get my original patch applied (see above).
Yes I think your patch is best - it is specific to the hardware which is the only way you can safely detect whether the UART is enabled.
We cannot rely on state like global_data to tell if the debug UART is enabled. We could use a flag in the DATA section on some devices, but that is also pretty nasty and we've been trying to avoid adding such things and using global_data instead.
Quoted 7 lines
So I am OK with the original patch.
However before going further I'd like to understand your thoughts on this question: I feel that you are checking for something that should not happen - i.e. the debug UART must be inited before use, just like any other device. We don't in general put these sorts of checks into other drivers. Why is the debug UART different?
On this specific platform (socfpga cylcone5/gen5), the preloader calls a function before initializing the debug uart that is also called later and
that should emit an error in one case. And if this is the case (which it can be depending on the boot source), then the system will hang when debug uart is enabled (loop forever in the debug uart output code).
Now you can say we can initialize the debug uart earlier on this platform, but that doesn't really work as it could be that the above function enables
a bus that is required for the debug uart (on socfpga, I could well imagine
the debug being implemented in FPGA and the offending function enables the FPGA bridge).

Don't put your debug UART on device that needs lots of init. Only Intel does that :-)

Quoted 9 lines

Quoted 9 lines
Then you could argue to remove the offending printf, but I feel that debug uart should help in debugging, not prevent it by entering an infinite loop when printf is called at the wrong time.
I just feel it's better to lose some printf messages at the start than adding
a hang that's not present without debug uart.

Quoted 7 lines

Quoted 3 lines
And as much as I'd love to solve this in a general way, the multitudes of different startup code does not seem to leave an easy way to solve this other than I did...?
So, without further answers, can we push the original patch? (
http://patchwork.ozlabs.org/patch/955765/ )

I think so.

Regards,
Simon