[U-Boot] [PATCH] 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 oded.gabbay@gmail.com Cc: Simon Glass sjg@chromium.org --- common/spl/spl.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/common/spl/spl.c b/common/spl/spl.c index f7df834..7c4744d 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -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 /*
On 28 December 2016 at 00:36, Oded Gabbay oded.gabbay@gmail.com wrote:
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 oded.gabbay@gmail.com Cc: Simon Glass sjg@chromium.org
common/spl/spl.c | 5 +++++ 1 file changed, 5 insertions(+)
Reviewed-by: Simon Glass sjg@chromium.org
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.
diff --git a/common/spl/spl.c b/common/spl/spl.c index f7df834..7c4744d 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -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
2016-12-28 16:36 GMT+09:00 Oded Gabbay oded.gabbay@gmail.com:
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 oded.gabbay@gmail.com Cc: Simon Glass sjg@chromium.org
common/spl/spl.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/common/spl/spl.c b/common/spl/spl.c index f7df834..7c4744d 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -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
Same here.
Please do not patch around with CONFIG_USE_TINY_PRINTF.
What you need to do is to fix tiny_printf() implementation.
On Tue, Jan 17, 2017 at 9:30 AM, Masahiro Yamada yamada.masahiro@socionext.com wrote:
2016-12-28 16:36 GMT+09:00 Oded Gabbay oded.gabbay@gmail.com:
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 oded.gabbay@gmail.com Cc: Simon Glass sjg@chromium.org
common/spl/spl.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/common/spl/spl.c b/common/spl/spl.c index f7df834..7c4744d 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -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
Same here.
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
2017-01-17 17:06 GMT+09:00 Oded Gabbay oded.gabbay@gmail.com:
On Tue, Jan 17, 2017 at 9:30 AM, Masahiro Yamada yamada.masahiro@socionext.com wrote:
2016-12-28 16:36 GMT+09:00 Oded Gabbay oded.gabbay@gmail.com:
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 oded.gabbay@gmail.com Cc: Simon Glass sjg@chromium.org
common/spl/spl.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/common/spl/spl.c b/common/spl/spl.c index f7df834..7c4744d 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -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
Same here.
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 ?
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()?
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 ?
On Tue, Jan 17, 2017 at 11:18 AM, Masahiro Yamada yamada.masahiro@socionext.com wrote:
2017-01-17 17:06 GMT+09:00 Oded Gabbay oded.gabbay@gmail.com:
On Tue, Jan 17, 2017 at 9:30 AM, Masahiro Yamada yamada.masahiro@socionext.com wrote:
2016-12-28 16:36 GMT+09:00 Oded Gabbay oded.gabbay@gmail.com:
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 oded.gabbay@gmail.com Cc: Simon Glass sjg@chromium.org
common/spl/spl.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/common/spl/spl.c b/common/spl/spl.c index f7df834..7c4744d 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -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
Same here.
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 ?
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
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
On Tue, Jan 17, 2017 at 10:06:26AM +0200, Oded Gabbay wrote:
On Tue, Jan 17, 2017 at 9:30 AM, Masahiro Yamada yamada.masahiro@socionext.com wrote:
2016-12-28 16:36 GMT+09:00 Oded Gabbay oded.gabbay@gmail.com:
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 oded.gabbay@gmail.com Cc: Simon Glass sjg@chromium.org
common/spl/spl.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/common/spl/spl.c b/common/spl/spl.c index f7df834..7c4744d 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -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
Same here.
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 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.
participants (4)
-
Masahiro Yamada -
Oded Gabbay -
Simon Glass -
Tom Rini