spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

PATCH New Oded Gabbay 4 participants 7 messages Dec 28, 2016 – Jan 19, 2017

Share & tools

Oded Gabbay

author

2016-12-28 07:36 UTC #0 lore raw

PATCH 1/1 spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

In the tiny-printf implementation, there is no support for %.*s. This patch checks if CONFIG_USE_TINY_PRINTF is defined and if so, prints a different debug statement which doesn't use %.*s

Signed-off-by Oded Gabbay <[email protected]>

Cc Simon Glass <[email protected]>

---
 common/spl/spl.c | 5 +++++
 1 file changed, 5 insertions(+)

common/spl/spl.c +5 -0

index f7df834..7c4744d 100644
@@ -115,9 +115,14 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
 		}
 		spl_image->os = image_get_os(header);
 		spl_image->name = image_get_name(header);
+#ifdef CONFIG_USE_TINY_PRINTF
+		debug("spl: payload image: %s load addr: 0x%x size: %d\n",
+			spl_image->name, spl_image->load_addr, spl_image->size);
+#else
 		debug("spl: payload image: %.*s load addr: 0x%x size: %d\n",
 			(int)sizeof(spl_image->name), spl_image->name,
 			spl_image->load_addr, spl_image->size);
+#endif
 	} else {
 #ifdef CONFIG_SPL_PANIC_ON_RAW_IMAGE
 		/*

Signature · 1 line

2.7.4

Simon Glass

2017-01-12 05:07 UTC #1 lore raw

PATCH 1/1 spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

Quoted 9 lines · Oded Gabbay

In the tiny-printf implementation, there is no support for %.*s. This patch checks if CONFIG_USE_TINY_PRINTF is defined and if so, prints a different debug statement which doesn't use %.*s
Signed-off-by Oded Gabbay <[email protected]>
Cc Simon Glass <[email protected]>
---
 common/spl/spl.c | 5 +++++
 1 file changed, 5 insertions(+)

Reviewed-by Simon Glass <[email protected]>

I did consider whether we should have some #defines for this (one which expands to either "*" or "" and one which drops the (int) parameter when USE_TINY_PRINTF is enabled), but it might be a bit clumsy.

I think what you have is the simplest solution for now.

Quoted 23 lines

common/spl/spl.c +5 -1

index f7df834..7c4744d 100644
@@ -115,9 +115,14 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
                }
                spl_image->os = image_get_os(header);
                spl_image->name = image_get_name(header);
+#ifdef CONFIG_USE_TINY_PRINTF
+               debug("spl: payload image: %s load addr: 0x%x size: %d\n",
+                       spl_image->name, spl_image->load_addr, spl_image->size);
+#else
                debug("spl: payload image: %.*s load addr: 0x%x size: %d\n",
                        (int)sizeof(spl_image->name), spl_image->name,
                        spl_image->load_addr, spl_image->size);
+#endif
        } else {
 #ifdef CONFIG_SPL_PANIC_ON_RAW_IMAGE
                /*
--

2.7.4

Masahiro Yamada

2017-01-17 07:30 UTC #2 lore raw

PATCH 1/1 spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

In reply to Oded Gabbay

2016-12-28 16:36 GMT+09:00 Oded Gabbay <[email protected]>:

Quoted 18 lines

In the tiny-printf implementation, there is no support for %.*s. This patch checks if CONFIG_USE_TINY_PRINTF is defined and if so, prints a different debug statement which doesn't use %.*s
Signed-off-by Oded Gabbay <[email protected]>
Cc Simon Glass <[email protected]>
---
 common/spl/spl.c | 5 +++++
 1 file changed, 5 insertions(+)
common/spl/spl.c +0 -0
index f7df834..7c4744d 100644
@@ -115,9 +115,14 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
                }
                spl_image->os = image_get_os(header);
                spl_image->name = image_get_name(header);

common/spl/spl.c int spl_parse_image_header(struct spl_image_info *spl_image, · line 127

Quoted 10 lines

common/spl/spl.c +5 -0

+#ifdef CONFIG_USE_TINY_PRINTF
+               debug("spl: payload image: %s load addr: 0x%x size: %d\n",
+                       spl_image->name, spl_image->load_addr, spl_image->size);
+#else
                debug("spl: payload image: %.*s load addr: 0x%x size: %d\n",
                        (int)sizeof(spl_image->name), spl_image->name,
                        spl_image->load_addr, spl_image->size);
+#endif
        } else {
 #ifdef CONFIG_SPL_PANIC_ON_RAW_IMAGE

Same here.

Please do not patch around with CONFIG_USE_TINY_PRINTF.

What you need to do is to fix tiny_printf() implementation.

Signature · 2 lines

Best Regards
Masahiro Yamada

Oded Gabbay

author

2017-01-17 08:06 UTC #3 lore raw

PATCH 1/1 spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

Quoted 33 lines · Masahiro Yamada

2016-12-28 16:36 GMT+09:00 Oded Gabbay <[email protected]>:
Quoted 18 lines
In the tiny-printf implementation, there is no support for %.*s. This patch checks if CONFIG_USE_TINY_PRINTF is defined and if so, prints a different debug statement which doesn't use %.*s
Signed-off-by Oded Gabbay <[email protected]>
Cc Simon Glass <[email protected]>
---
 common/spl/spl.c | 5 +++++
 1 file changed, 5 insertions(+)
common/spl/spl.c +0 -0
index f7df834..7c4744d 100644
@@ -115,9 +115,14 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
                }
                spl_image->os = image_get_os(header);
                spl_image->name = image_get_name(header);
common/spl/spl.c int spl_parse_image_header(struct spl_image_info *spl_image, · line 127
Quoted 10 lines
common/spl/spl.c +5 -0
+#ifdef CONFIG_USE_TINY_PRINTF
+               debug("spl: payload image: %s load addr: 0x%x size: %d\n",
+                       spl_image->name, spl_image->load_addr, spl_image->size);
+#else
                debug("spl: payload image: %.*s load addr: 0x%x size: %d\n",
                        (int)sizeof(spl_image->name), spl_image->name,
                        spl_image->load_addr, spl_image->size);
+#endif
        } else {
 #ifdef CONFIG_SPL_PANIC_ON_RAW_IMAGE
Same here.

Quoted 8 lines

Please do not patch around with CONFIG_USE_TINY_PRINTF.
What you need to do is to fix tiny_printf() implementation.
--
Best Regards
Masahiro Yamada

ok, I can accept that but how would you like to fix it ? I guess you don't want to copy the entire printf implementation, otherwise what's the point in tiny-printf ?

Do you think we should just silently treat %.s as %s inside tiny-printf ? And take that approach to the other two patches I sent ?

Thanks,

Oded

Masahiro Yamada

2017-01-17 09:18 UTC #4 lore raw

PATCH 1/1 spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

2017-01-17 17:06 GMT+09:00 Oded Gabbay <[email protected]>:

Quoted 40 lines

Quoted 38 lines · Masahiro Yamada
2016-12-28 16:36 GMT+09:00 Oded Gabbay <[email protected]>:
Quoted 18 lines
In the tiny-printf implementation, there is no support for %.*s. This patch checks if CONFIG_USE_TINY_PRINTF is defined and if so, prints a different debug statement which doesn't use %.*s
Signed-off-by Oded Gabbay <[email protected]>
Cc Simon Glass <[email protected]>
---
 common/spl/spl.c | 5 +++++
 1 file changed, 5 insertions(+)
common/spl/spl.c +0 -0
index f7df834..7c4744d 100644
@@ -115,9 +115,14 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
                }
                spl_image->os = image_get_os(header);
                spl_image->name = image_get_name(header);
common/spl/spl.c int spl_parse_image_header(struct spl_image_info *spl_image, · line 127
Quoted 10 lines
common/spl/spl.c +5 -0
+#ifdef CONFIG_USE_TINY_PRINTF
+               debug("spl: payload image: %s load addr: 0x%x size: %d\n",
+                       spl_image->name, spl_image->load_addr, spl_image->size);
+#else
                debug("spl: payload image: %.*s load addr: 0x%x size: %d\n",
                        (int)sizeof(spl_image->name), spl_image->name,
                        spl_image->load_addr, spl_image->size);
+#endif
        } else {
 #ifdef CONFIG_SPL_PANIC_ON_RAW_IMAGE
Same here.
Please do not patch around with CONFIG_USE_TINY_PRINTF.
What you need to do is to fix tiny_printf() implementation.

Quoted 7 lines

Quoted 3 lines
--
Best Regards
Masahiro Yamada
ok, I can accept that but how would you like to fix it ? I guess you don't want to copy the entire printf implementation, otherwise what's the point in tiny-printf ?

Right. Supporting all the format specifiers will make
tiny-printf much bigger, then we will lose the point of this function.

Can we silently ignore specifiers that are not recognized by tiny_printf()?

Quoted 3 lines

Do you think we should just silently treat %.s as %s inside tiny-printf ? And take that approach to the other two patches I sent ?

Signature · 2 lines

Best Regards
Masahiro Yamada

Oded Gabbay

author

2017-01-17 09:25 UTC #5 lore raw

PATCH 1/1 spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

Quoted 50 lines · Masahiro Yamada

2017-01-17 17:06 GMT+09:00 Oded Gabbay <[email protected]>:
Quoted 40 lines
Quoted 38 lines · Masahiro Yamada
2016-12-28 16:36 GMT+09:00 Oded Gabbay <[email protected]>:
Quoted 18 lines
In the tiny-printf implementation, there is no support for %.*s. This patch checks if CONFIG_USE_TINY_PRINTF is defined and if so, prints a different debug statement which doesn't use %.*s
Signed-off-by Oded Gabbay <[email protected]>
Cc Simon Glass <[email protected]>
---
 common/spl/spl.c | 5 +++++
 1 file changed, 5 insertions(+)
common/spl/spl.c +0 -0
index f7df834..7c4744d 100644
@@ -115,9 +115,14 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
                }
                spl_image->os = image_get_os(header);
                spl_image->name = image_get_name(header);
common/spl/spl.c int spl_parse_image_header(struct spl_image_info *spl_image, · line 127
Quoted 10 lines
common/spl/spl.c +5 -0
+#ifdef CONFIG_USE_TINY_PRINTF
+               debug("spl: payload image: %s load addr: 0x%x size: %d\n",
+                       spl_image->name, spl_image->load_addr, spl_image->size);
+#else
                debug("spl: payload image: %.*s load addr: 0x%x size: %d\n",
                        (int)sizeof(spl_image->name), spl_image->name,
                        spl_image->load_addr, spl_image->size);
+#endif
        } else {
 #ifdef CONFIG_SPL_PANIC_ON_RAW_IMAGE
Same here.
Please do not patch around with CONFIG_USE_TINY_PRINTF.
What you need to do is to fix tiny_printf() implementation.
Quoted 7 lines
Quoted 3 lines
--
Best Regards
Masahiro Yamada
ok, I can accept that but how would you like to fix it ? I guess you don't want to copy the entire printf implementation, otherwise what's the point in tiny-printf ?

Quoted 5 lines

Right. Supporting all the format specifiers will make
tiny-printf much bigger, then we will lose the point of this function.
Can we silently ignore specifiers that are not recognized by tiny_printf()?

That is the default behavior today, but the result of that behavior is that many prints look awful, with a lot of missing information, such as : "address () " , or "payload image: load addr" (between payload
image: and load addr should be the name of the image). It really makes the whole thing totally unusable.

I didn't like that behavior and that's why I wanted to change it. I think there are two options:
1. Use only implemented specifiers like in my original patches.
2. Silently replace un-implemented specifiers with the most similar implemented specifiers, like %X to %x, or %.s to %s

Thanks,
Oded

Quoted 9 lines

Quoted 4 lines
Do you think we should just silently treat %.s as %s inside tiny-printf ? And take that approach to the other two patches I sent ?
--
Best Regards
Masahiro Yamada

Tom Rini

2017-01-19 03:03 UTC #6 lore raw

PATCH 1/1 spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

In reply to Oded Gabbay

Quoted 37 lines · Oded Gabbay

Quoted 35 lines · Masahiro Yamada
2016-12-28 16:36 GMT+09:00 Oded Gabbay <[email protected]>:
Quoted 18 lines
In the tiny-printf implementation, there is no support for %.*s. This patch checks if CONFIG_USE_TINY_PRINTF is defined and if so, prints a different debug statement which doesn't use %.*s
Signed-off-by Oded Gabbay <[email protected]>
Cc Simon Glass <[email protected]>
---
 common/spl/spl.c | 5 +++++
 1 file changed, 5 insertions(+)
common/spl/spl.c +0 -0
index f7df834..7c4744d 100644
@@ -115,9 +115,14 @@ int spl_parse_image_header(struct spl_image_info *spl_image,
                }
                spl_image->os = image_get_os(header);
                spl_image->name = image_get_name(header);
common/spl/spl.c int spl_parse_image_header(struct spl_image_info *spl_image, · line 127
Quoted 10 lines
common/spl/spl.c +5 -0
+#ifdef CONFIG_USE_TINY_PRINTF
+               debug("spl: payload image: %s load addr: 0x%x size: %d\n",
+                       spl_image->name, spl_image->load_addr, spl_image->size);
+#else
                debug("spl: payload image: %.*s load addr: 0x%x size: %d\n",
                        (int)sizeof(spl_image->name), spl_image->name,
                        spl_image->load_addr, spl_image->size);
+#endif
        } else {
 #ifdef CONFIG_SPL_PANIC_ON_RAW_IMAGE
Same here.
Please do not patch around with CONFIG_USE_TINY_PRINTF.

Quoted 8 lines

Quoted 6 lines
What you need to do is to fix tiny_printf() implementation.
--
Best Regards
Masahiro Yamada
ok, I can accept that but how would you like to fix it ?

I would suggest un-winding things such that we get useful information in both cases with USE_TINY_PRINTF being the limiting factor. I am of the opinion that USE_TINY_PRINTF should be the default within SPL.

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20170118/50205fda/attachment.sig>