[U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

[U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

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

[U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

[Simon Glass]

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
>

[U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

[Masahiro Yamada]

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

[U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

[Oded Gabbay]

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

[U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

[Masahiro Yamada]

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

[U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

[Oded Gabbay]

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

[U-Boot] [PATCH] spl: don't use %.*s with CONFIG_USE_TINY_PRINTF

[Tom Rini]

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>