* [U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF @ 2016-12-28 7:36 Oded Gabbay 2017-01-12 5:07 ` Simon Glass 2017-01-17 7:30 ` Masahiro Yamada 0 siblings, 2 replies; 7+ messages in thread From: Oded Gabbay @ 2016-12-28 7:36 UTC (permalink / raw) To: u-boot 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 /* -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF 2016-12-28 7:36 [U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF Oded Gabbay @ 2017-01-12 5:07 ` Simon Glass 2017-01-17 7:30 ` Masahiro Yamada 1 sibling, 0 replies; 7+ messages in thread From: Simon Glass @ 2017-01-12 5:07 UTC (permalink / raw) To: u-boot 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF 2016-12-28 7:36 [U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF Oded Gabbay 2017-01-12 5:07 ` Simon Glass @ 2017-01-17 7:30 ` Masahiro Yamada 2017-01-17 8:06 ` Oded Gabbay 1 sibling, 1 reply; 7+ messages in thread From: Masahiro Yamada @ 2017-01-17 7:30 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF 2017-01-17 7:30 ` Masahiro Yamada @ 2017-01-17 8:06 ` Oded Gabbay 2017-01-17 9:18 ` Masahiro Yamada 2017-01-19 3:03 ` Tom Rini 0 siblings, 2 replies; 7+ messages in thread From: Oded Gabbay @ 2017-01-17 8:06 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF 2017-01-17 8:06 ` Oded Gabbay @ 2017-01-17 9:18 ` Masahiro Yamada 2017-01-17 9:25 ` Oded Gabbay 2017-01-19 3:03 ` Tom Rini 1 sibling, 1 reply; 7+ messages in thread From: Masahiro Yamada @ 2017-01-17 9:18 UTC (permalink / raw) To: u-boot 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 ? > -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF 2017-01-17 9:18 ` Masahiro Yamada @ 2017-01-17 9:25 ` Oded Gabbay 0 siblings, 0 replies; 7+ messages in thread From: Oded Gabbay @ 2017-01-17 9:25 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF 2017-01-17 8:06 ` Oded Gabbay 2017-01-17 9:18 ` Masahiro Yamada @ 2017-01-19 3:03 ` Tom Rini 1 sibling, 0 replies; 7+ messages in thread From: Tom Rini @ 2017-01-19 3:03 UTC (permalink / raw) To: u-boot 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. -- 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> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-01-19 3:03 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-28 7:36 [U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF Oded Gabbay 2017-01-12 5:07 ` Simon Glass 2017-01-17 7:30 ` Masahiro Yamada 2017-01-17 8:06 ` Oded Gabbay 2017-01-17 9:18 ` Masahiro Yamada 2017-01-17 9:25 ` Oded Gabbay 2017-01-19 3:03 ` Tom Rini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox