[PATCH] tiny-printf: Add support for upper case hex values

[PATCH] tiny-printf: Add support for upper case hex values

[Christoph Niedermaier]

If tiny printf is used with 0x%08X (upper case X) the output is
always 0x00000000. It could be confusing if upper case instead
of lower case is used intentionally or accidentally because the
actual value is not output. To avoid this confusion, tiny printf
is extended to support also the formatting with %X.

Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
---
Cc: Marek Vasut <marex@denx.de>
Cc: Tom Rini <trini@konsulko.com>
Cc: Benedikt Spranger <b.spranger@linutronix.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: John Ogness <john.ogness@linutronix.de>
Cc: Jerome Forissier <jerome.forissier@linaro.org>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/tiny-printf.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
index 0503c17341f..81c51d12f6c 100644
--- a/lib/tiny-printf.c
+++ b/lib/tiny-printf.c
@@ -13,6 +13,11 @@
 #include <stdarg.h>
 #include <linux/ctype.h>
 
+enum case_style {
+	LOWERCASE = 0,
+	UPPERCASE = 1,
+};
+
 struct printf_info {
 	char *bf;	/* Digit buffer */
 	char zs;	/* non-zero if a digit has been written */
@@ -27,14 +32,19 @@ static void out(struct printf_info *info, char c)
 	*info->bf++ = c;
 }
 
-static void out_dgt(struct printf_info *info, char dgt)
+static void out_dgt_case(struct printf_info *info, char dgt, enum case_style style)
 {
-	out(info, dgt + (dgt < 10 ? '0' : 'a' - 10));
+	out(info, dgt + (dgt < 10 ? '0' : (style ? 'A' : 'a') - 10));
 	info->zs = 1;
 }
 
-static void div_out(struct printf_info *info, unsigned long *num,
-		    unsigned long div)
+static void out_dgt(struct printf_info *info, char dgt)
+{
+	out_dgt_case(info, dgt, LOWERCASE);
+}
+
+static void div_out_case(struct printf_info *info, unsigned long *num,
+			 unsigned long div, enum case_style style)
 {
 	unsigned char dgt = 0;
 
@@ -44,7 +54,13 @@ static void div_out(struct printf_info *info, unsigned long *num,
 	}
 
 	if (info->zs || dgt > 0)
-		out_dgt(info, dgt);
+		out_dgt_case(info, dgt, style);
+}
+
+static void div_out(struct printf_info *info, unsigned long *num,
+		    unsigned long div)
+{
+	div_out_case(info, num, div, LOWERCASE);
 }
 
 #ifdef CONFIG_SPL_NET
@@ -203,6 +219,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va)
 	unsigned long num;
 	char buf[12];
 	unsigned long div;
+	enum case_style style = LOWERCASE;
 
 	while ((ch = *(fmt++))) {
 		if (ch != '%') {
@@ -283,6 +300,8 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va)
 				}
 				islong = true;
 				/* no break */
+			case 'X':
+				style = UPPERCASE;
 			case 'x':
 				if (islong) {
 					num = va_arg(va, unsigned long);
@@ -295,7 +314,7 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va)
 					out_dgt(info, 0);
 				} else {
 					for (; div; div /= 0x10)
-						div_out(info, &num, div);
+						div_out_case(info, &num, div, style);
 				}
 				break;
 			case 'c':
-- 
2.30.2

Re: [PATCH] tiny-printf: Add support for upper case hex values

[Marek Vasut]

On 3/20/25 11:23 AM, Christoph Niedermaier wrote:
> If tiny printf is used with 0x%08X (upper case X) the output is
> always 0x00000000. It could be confusing if upper case instead
> of lower case is used intentionally or accidentally because the
> actual value is not output. To avoid this confusion, tiny printf
> is extended to support also the formatting with %X.
> 
> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
TINY_PRINTF is meant to be tiny, i.e. not consume a lot of space, at the 
expense of functionality. This is meant to be used in size constrained 
environments, like the SPL. If you need full vsprintf() formatting 
support, disable TINY_PRINTF in your config and use the regular 
vsprintf() implementation.

Re: [PATCH] tiny-printf: Add support for upper case hex values

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

On Thu, Mar 20, 2025 at 12:49:17PM +0100, Marek Vasut wrote:
> On 3/20/25 11:23 AM, Christoph Niedermaier wrote:
> > If tiny printf is used with 0x%08X (upper case X) the output is
> > always 0x00000000. It could be confusing if upper case instead
> > of lower case is used intentionally or accidentally because the
> > actual value is not output. To avoid this confusion, tiny printf
> > is extended to support also the formatting with %X.
> > 
> > Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> TINY_PRINTF is meant to be tiny, i.e. not consume a lot of space, at the
> expense of functionality. This is meant to be used in size constrained
> environments, like the SPL. If you need full vsprintf() formatting support,
> disable TINY_PRINTF in your config and use the regular vsprintf()
> implementation.

Right. I could accept a patch to tiny-printf that treats %X like %x (so
that the value shown is correct) as that would be a minimal size change.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] tiny-printf: Add support for upper case hex values

[Quentin Schulz]

Hi Marek,

On 3/20/25 12:49 PM, Marek Vasut wrote:
> On 3/20/25 11:23 AM, Christoph Niedermaier wrote:
>> If tiny printf is used with 0x%08X (upper case X) the output is
>> always 0x00000000. It could be confusing if upper case instead
>> of lower case is used intentionally or accidentally because the
>> actual value is not output. To avoid this confusion, tiny printf
>> is extended to support also the formatting with %X.
>>
>> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> TINY_PRINTF is meant to be tiny, i.e. not consume a lot of space, at the 
> expense of functionality. This is meant to be used in size constrained 
> environments, like the SPL. If you need full vsprintf() formatting 
> support, disable TINY_PRINTF in your config and use the regular 
> vsprintf() implementation.

The issue is that disabling TINY_PRINTF may not be possible (size 
constraints) and some code is compiled for different stages and people 
typically don't check whether the format used in printf is valid with 
tiny_printf. I've had this issue already in the past, I vaguely recall 
"complaining" about it on IRC.

Maybe there's something we can do to verify that the code is working how 
we expect it to work, regardless of tiny_printf/full printf selection? 
checkpatch or a compile-time check for the formats maybe?

But yeah, essentially the whole thing is... if we continue like this, 
we'll just end up getting closer and closer to the full printf which is 
not something we want :)

Cheers,
Quentin

Re: [PATCH] tiny-printf: Add support for upper case hex values

[Marek Vasut]

On 3/20/25 3:00 PM, Quentin Schulz wrote:
> Hi Marek,
> 
> On 3/20/25 12:49 PM, Marek Vasut wrote:
>> On 3/20/25 11:23 AM, Christoph Niedermaier wrote:
>>> If tiny printf is used with 0x%08X (upper case X) the output is
>>> always 0x00000000. It could be confusing if upper case instead
>>> of lower case is used intentionally or accidentally because the
>>> actual value is not output. To avoid this confusion, tiny printf
>>> is extended to support also the formatting with %X.
>>>
>>> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
>> TINY_PRINTF is meant to be tiny, i.e. not consume a lot of space, at 
>> the expense of functionality. This is meant to be used in size 
>> constrained environments, like the SPL. If you need full vsprintf() 
>> formatting support, disable TINY_PRINTF in your config and use the 
>> regular vsprintf() implementation.
> 
> The issue is that disabling TINY_PRINTF may not be possible (size 
> constraints) and some code is compiled for different stages and people 
> typically don't check whether the format used in printf is valid with 
> tiny_printf. I've had this issue already in the past, I vaguely recall 
> "complaining" about it on IRC.
> 
> Maybe there's something we can do to verify that the code is working how 
> we expect it to work, regardless of tiny_printf/full printf selection? 
> checkpatch or a compile-time check for the formats maybe?
> 
> But yeah, essentially the whole thing is... if we continue like this, 
> we'll just end up getting closer and closer to the full printf which is 
> not something we want :)
Shall we maybe patch tiny printf to print '?' on unsupported formatting 
characters, or outright complain that users should fix their code ?

For the %x/%X thing, we could technically fall back from %X to %x , 
which would do the printing with minimum footprint increase, albeit 
slightly malformed:

diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
index 0503c17341f..48db7b1f78f 100644
--- a/lib/tiny-printf.c
+++ b/lib/tiny-printf.c
@@ -284,6 +284,7 @@ static int _vprintf(struct printf_info *info, const 
char *fmt, va_list va)
                                 islong = true;
                                 /* no break */
                         case 'x':
+                       case 'X':
                                 if (islong) {
                                         num = va_arg(va, unsigned long);
                                         div = 1UL << (sizeof(long) * 8 
- 4);

Re: [PATCH] tiny-printf: Add support for upper case hex values

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2573 bytes --]

On Thu, Mar 20, 2025 at 03:14:03PM +0100, Marek Vasut wrote:
> On 3/20/25 3:00 PM, Quentin Schulz wrote:
> > Hi Marek,
> > 
> > On 3/20/25 12:49 PM, Marek Vasut wrote:
> > > On 3/20/25 11:23 AM, Christoph Niedermaier wrote:
> > > > If tiny printf is used with 0x%08X (upper case X) the output is
> > > > always 0x00000000. It could be confusing if upper case instead
> > > > of lower case is used intentionally or accidentally because the
> > > > actual value is not output. To avoid this confusion, tiny printf
> > > > is extended to support also the formatting with %X.
> > > > 
> > > > Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> > > TINY_PRINTF is meant to be tiny, i.e. not consume a lot of space, at
> > > the expense of functionality. This is meant to be used in size
> > > constrained environments, like the SPL. If you need full vsprintf()
> > > formatting support, disable TINY_PRINTF in your config and use the
> > > regular vsprintf() implementation.
> > 
> > The issue is that disabling TINY_PRINTF may not be possible (size
> > constraints) and some code is compiled for different stages and people
> > typically don't check whether the format used in printf is valid with
> > tiny_printf. I've had this issue already in the past, I vaguely recall
> > "complaining" about it on IRC.
> > 
> > Maybe there's something we can do to verify that the code is working how
> > we expect it to work, regardless of tiny_printf/full printf selection?
> > checkpatch or a compile-time check for the formats maybe?
> > 
> > But yeah, essentially the whole thing is... if we continue like this,
> > we'll just end up getting closer and closer to the full printf which is
> > not something we want :)
> Shall we maybe patch tiny printf to print '?' on unsupported formatting
> characters, or outright complain that users should fix their code ?

This sounds good to me, adding ? in the output.

> For the %x/%X thing, we could technically fall back from %X to %x , which
> would do the printing with minimum footprint increase, albeit slightly
> malformed:

There's 109 hits on "%X" and another 489 on "%0.X", so I think it's
reasonable to do either of:
- Misprint A-F as a-f (in other words, treat it like 'x'
- Audit the callers and change them to 'x' from 'X'. We normally don't
  capitalize the output and there's all sorts of patches over the years
  changing them to lowercase in other places.

We have done both for other format specifiers and tiny-printf before in
the past.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

RE: [PATCH] tiny-printf: Add support for upper case hex values

[Christoph Niedermaier]

From: Tom Rini <trini@konsulko.com>
Sent: Thursday, March 20, 2025 3:19 PM
> On Thu, Mar 20, 2025 at 03:14:03PM +0100, Marek Vasut wrote:
> > On 3/20/25 3:00 PM, Quentin Schulz wrote:
> > > Hi Marek,
> > >
> > > On 3/20/25 12:49 PM, Marek Vasut wrote:
> > > > On 3/20/25 11:23 AM, Christoph Niedermaier wrote:
> > > > > If tiny printf is used with 0x%08X (upper case X) the output is
> > > > > always 0x00000000. It could be confusing if upper case instead
> > > > > of lower case is used intentionally or accidentally because the
> > > > > actual value is not output. To avoid this confusion, tiny printf
> > > > > is extended to support also the formatting with %X.
> > > > >
> > > > > Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> > > > TINY_PRINTF is meant to be tiny, i.e. not consume a lot of space, at
> > > > the expense of functionality. This is meant to be used in size
> > > > constrained environments, like the SPL. If you need full vsprintf()
> > > > formatting support, disable TINY_PRINTF in your config and use the
> > > > regular vsprintf() implementation.
> > >
> > > The issue is that disabling TINY_PRINTF may not be possible (size
> > > constraints) and some code is compiled for different stages and people
> > > typically don't check whether the format used in printf is valid with
> > > tiny_printf. I've had this issue already in the past, I vaguely recall
> > > "complaining" about it on IRC.
> > >
> > > Maybe there's something we can do to verify that the code is working how
> > > we expect it to work, regardless of tiny_printf/full printf selection?
> > > checkpatch or a compile-time check for the formats maybe?
> > >
> > > But yeah, essentially the whole thing is... if we continue like this,
> > > we'll just end up getting closer and closer to the full printf which is
> > > not something we want :)
> > Shall we maybe patch tiny printf to print '?' on unsupported formatting
> > characters, or outright complain that users should fix their code ?
> 
> This sounds good to me, adding ? in the output.
> 
> > For the %x/%X thing, we could technically fall back from %X to %x , which
> > would do the printing with minimum footprint increase, albeit slightly
> > malformed:
> 
> There's 109 hits on "%X" and another 489 on "%0.X", so I think it's
> reasonable to do either of:
> - Misprint A-F as a-f (in other words, treat it like 'x'
> - Audit the callers and change them to 'x' from 'X'. We normally don't
>   capitalize the output and there's all sorts of patches over the years
>   changing them to lowercase in other places.
> 
> We have done both for other format specifiers and tiny-printf before in
> the past.

If we taking about adding feature, I think that the patch don't really
add a new feature, it's just a variant of %x. I reuse part of the %x code.
So if size matters here the size of the object file (not stripped):

Before:
-rw-r--r-- 1 developer developer 19340 Mar 20 15:32 tiny-printf.o

After with current patch:
-rw-r--r-- 1 developer developer 21212 Mar 20 15:38 tiny-printf.o
=> Diff: 1872 Bytes (+9,67%)

I have another patch, where I don't introduce two new function and
don't use an enum. Then it looks like this:
-rw-r--r-- 1 developer developer 19888 Mar 20 16:53 tiny-printf.o
=> Diff: 548 Bytes (+2,83%)

Would this increase in size be OK for %X?
So there will be no misprint.

Otherwise, a misprint for %X would be fine with me, because I still
get the correct value.


Regards
Christoph

Re: [PATCH] tiny-printf: Add support for upper case hex values

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3907 bytes --]

On Thu, Mar 20, 2025 at 05:58:56PM +0000, Christoph Niedermaier wrote:
> From: Tom Rini <trini@konsulko.com>
> Sent: Thursday, March 20, 2025 3:19 PM
> > On Thu, Mar 20, 2025 at 03:14:03PM +0100, Marek Vasut wrote:
> > > On 3/20/25 3:00 PM, Quentin Schulz wrote:
> > > > Hi Marek,
> > > >
> > > > On 3/20/25 12:49 PM, Marek Vasut wrote:
> > > > > On 3/20/25 11:23 AM, Christoph Niedermaier wrote:
> > > > > > If tiny printf is used with 0x%08X (upper case X) the output is
> > > > > > always 0x00000000. It could be confusing if upper case instead
> > > > > > of lower case is used intentionally or accidentally because the
> > > > > > actual value is not output. To avoid this confusion, tiny printf
> > > > > > is extended to support also the formatting with %X.
> > > > > >
> > > > > > Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> > > > > TINY_PRINTF is meant to be tiny, i.e. not consume a lot of space, at
> > > > > the expense of functionality. This is meant to be used in size
> > > > > constrained environments, like the SPL. If you need full vsprintf()
> > > > > formatting support, disable TINY_PRINTF in your config and use the
> > > > > regular vsprintf() implementation.
> > > >
> > > > The issue is that disabling TINY_PRINTF may not be possible (size
> > > > constraints) and some code is compiled for different stages and people
> > > > typically don't check whether the format used in printf is valid with
> > > > tiny_printf. I've had this issue already in the past, I vaguely recall
> > > > "complaining" about it on IRC.
> > > >
> > > > Maybe there's something we can do to verify that the code is working how
> > > > we expect it to work, regardless of tiny_printf/full printf selection?
> > > > checkpatch or a compile-time check for the formats maybe?
> > > >
> > > > But yeah, essentially the whole thing is... if we continue like this,
> > > > we'll just end up getting closer and closer to the full printf which is
> > > > not something we want :)
> > > Shall we maybe patch tiny printf to print '?' on unsupported formatting
> > > characters, or outright complain that users should fix their code ?
> > 
> > This sounds good to me, adding ? in the output.
> > 
> > > For the %x/%X thing, we could technically fall back from %X to %x , which
> > > would do the printing with minimum footprint increase, albeit slightly
> > > malformed:
> > 
> > There's 109 hits on "%X" and another 489 on "%0.X", so I think it's
> > reasonable to do either of:
> > - Misprint A-F as a-f (in other words, treat it like 'x'
> > - Audit the callers and change them to 'x' from 'X'. We normally don't
> >   capitalize the output and there's all sorts of patches over the years
> >   changing them to lowercase in other places.
> > 
> > We have done both for other format specifiers and tiny-printf before in
> > the past.
> 
> If we taking about adding feature, I think that the patch don't really
> add a new feature, it's just a variant of %x. I reuse part of the %x code.
> So if size matters here the size of the object file (not stripped):
> 
> Before:
> -rw-r--r-- 1 developer developer 19340 Mar 20 15:32 tiny-printf.o
> 
> After with current patch:
> -rw-r--r-- 1 developer developer 21212 Mar 20 15:38 tiny-printf.o
> => Diff: 1872 Bytes (+9,67%)
> 
> I have another patch, where I don't introduce two new function and
> don't use an enum. Then it looks like this:
> -rw-r--r-- 1 developer developer 19888 Mar 20 16:53 tiny-printf.o
> => Diff: 548 Bytes (+2,83%)
> 
> Would this increase in size be OK for %X?
> So there will be no misprint.
> 
> Otherwise, a misprint for %X would be fine with me, because I still
> get the correct value.

Tiny really does mean tiny in this case so yes, I would prefer the
single digit byte increase of adding 'X' to the 'x' case. Thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

RE: [PATCH] tiny-printf: Add support for upper case hex values

[Christoph Niedermaier]

From: Tom Rini <trini@konsulko.com>
Sent: Thursday, March 20, 2025 7:28 PM
> On Thu, Mar 20, 2025 at 05:58:56PM +0000, Christoph Niedermaier wrote:
> > From: Tom Rini <trini@konsulko.com>
> > Sent: Thursday, March 20, 2025 3:19 PM
> > > On Thu, Mar 20, 2025 at 03:14:03PM +0100, Marek Vasut wrote:
> > > > On 3/20/25 3:00 PM, Quentin Schulz wrote:
> > > > > Hi Marek,
> > > > >
> > > > > On 3/20/25 12:49 PM, Marek Vasut wrote:
> > > > > > On 3/20/25 11:23 AM, Christoph Niedermaier wrote:
> > > > > > > If tiny printf is used with 0x%08X (upper case X) the output is
> > > > > > > always 0x00000000. It could be confusing if upper case instead
> > > > > > > of lower case is used intentionally or accidentally because the
> > > > > > > actual value is not output. To avoid this confusion, tiny printf
> > > > > > > is extended to support also the formatting with %X.
> > > > > > >
> > > > > > > Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> > > > > > TINY_PRINTF is meant to be tiny, i.e. not consume a lot of space, at
> > > > > > the expense of functionality. This is meant to be used in size
> > > > > > constrained environments, like the SPL. If you need full vsprintf()
> > > > > > formatting support, disable TINY_PRINTF in your config and use the
> > > > > > regular vsprintf() implementation.
> > > > >
> > > > > The issue is that disabling TINY_PRINTF may not be possible (size
> > > > > constraints) and some code is compiled for different stages and people
> > > > > typically don't check whether the format used in printf is valid with
> > > > > tiny_printf. I've had this issue already in the past, I vaguely recall
> > > > > "complaining" about it on IRC.
> > > > >
> > > > > Maybe there's something we can do to verify that the code is working how
> > > > > we expect it to work, regardless of tiny_printf/full printf selection?
> > > > > checkpatch or a compile-time check for the formats maybe?
> > > > >
> > > > > But yeah, essentially the whole thing is... if we continue like this,
> > > > > we'll just end up getting closer and closer to the full printf which is
> > > > > not something we want :)
> > > > Shall we maybe patch tiny printf to print '?' on unsupported formatting
> > > > characters, or outright complain that users should fix their code ?
> > >
> > > This sounds good to me, adding ? in the output.
> > >
> > > > For the %x/%X thing, we could technically fall back from %X to %x , which
> > > > would do the printing with minimum footprint increase, albeit slightly
> > > > malformed:
> > >
> > > There's 109 hits on "%X" and another 489 on "%0.X", so I think it's
> > > reasonable to do either of:
> > > - Misprint A-F as a-f (in other words, treat it like 'x'
> > > - Audit the callers and change them to 'x' from 'X'. We normally don't
> > >   capitalize the output and there's all sorts of patches over the years
> > >   changing them to lowercase in other places.
> > >
> > > We have done both for other format specifiers and tiny-printf before in
> > > the past.
> >
> > If we taking about adding feature, I think that the patch don't really
> > add a new feature, it's just a variant of %x. I reuse part of the %x code.
> > So if size matters here the size of the object file (not stripped):
> >
> > Before:
> > -rw-r--r-- 1 developer developer 19340 Mar 20 15:32 tiny-printf.o
> >
> > After with current patch:
> > -rw-r--r-- 1 developer developer 21212 Mar 20 15:38 tiny-printf.o
> > => Diff: 1872 Bytes (+9,67%)
> >
> > I have another patch, where I don't introduce two new function and
> > don't use an enum. Then it looks like this:
> > -rw-r--r-- 1 developer developer 19888 Mar 20 16:53 tiny-printf.o
> > => Diff: 548 Bytes (+2,83%)
> >
> > Would this increase in size be OK for %X?
> > So there will be no misprint.
> >
> > Otherwise, a misprint for %X would be fine with me, because I still
> > get the correct value.
> 
> Tiny really does mean tiny in this case so yes, I would prefer the
> single digit byte increase of adding 'X' to the 'x' case. Thanks!

OK, I will make a V2 with this changes.


Regards
Christoph

Re: [PATCH] tiny-printf: Add support for upper case hex values

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 553 bytes --]

Hi,

> The issue is that disabling TINY_PRINTF may not be possible (size 
> constraints) and some code is compiled for different stages and people 
> typically don't check whether the format used in printf is valid with 
> tiny_printf. I've had this issue already in the past, I vaguely recall 
> "complaining" about it on IRC.

Yes, I've stumbled on "%pa" with tiny printf (i.e. in
drivers/pinctrl/pinctrl-single.c) which is printing the very wrong
value, actually :) So printing anything unknown as '?' would really
help here.

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

RE: [PATCH] tiny-printf: Add support for upper case hex values

[Christoph Niedermaier]

From: Michael Walle <mwalle@kernel.org>
Sent: Tuesday, April 1, 2025 10:56 AM
> 
> Hi,
> 
>> The issue is that disabling TINY_PRINTF may not be possible (size
>> constraints) and some code is compiled for different stages and people
>> typically don't check whether the format used in printf is valid with
>> tiny_printf. I've had this issue already in the past, I vaguely recall
>> "complaining" about it on IRC.
> 
> Yes, I've stumbled on "%pa" with tiny printf (i.e. in
> drivers/pinctrl/pinctrl-single.c) which is printing the very wrong
> value, actually :) So printing anything unknown as '?' would really
> help here.

Something like that would help:
diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
index 48db7b1f78f..b918d6d7386 100644
--- a/lib/tiny-printf.c
+++ b/lib/tiny-printf.c
@@ -280,6 +280,12 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va)
                                        while (isalnum(fmt[0]))
                                                fmt++;
                                        break;
+                               } else {
+                                       if (fmt[0] != '\0' && (fmt[0] == 'a' || fmt[0] == 'm' ||
+                                                              fmt[0] == 'M' || fmt[0] == 'I')) {
+                                               fmt++;
+                                               goto unsupported;
+                                       }
                                }
                                islong = true;
                                /* no break */
@@ -308,6 +314,8 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va)
                        case '%':
                                out(info, '%');
                        default:
+unsupported:
+                               out(info, '?');
                                break;
                        }

But maybe it is too much for tiny printf, because tiny means
tiny. It's a question of either gain or small code size.

Regards
Christoph

Re: [PATCH] tiny-printf: Add support for upper case hex values

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 3426 bytes --]

Hi,

> >> The issue is that disabling TINY_PRINTF may not be possible (size
> >> constraints) and some code is compiled for different stages and people
> >> typically don't check whether the format used in printf is valid with
> >> tiny_printf. I've had this issue already in the past, I vaguely recall
> >> "complaining" about it on IRC.
> > 
> > Yes, I've stumbled on "%pa" with tiny printf (i.e. in
> > drivers/pinctrl/pinctrl-single.c) which is printing the very wrong
> > value, actually :) So printing anything unknown as '?' would really
> > help here.
>
> Something like that would help:
> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
> index 48db7b1f78f..b918d6d7386 100644
> --- a/lib/tiny-printf.c
> +++ b/lib/tiny-printf.c
> @@ -280,6 +280,12 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va)
>                                         while (isalnum(fmt[0]))
>                                                 fmt++;
>                                         break;
> +                               } else {
> +                                       if (fmt[0] != '\0' && (fmt[0] == 'a' || fmt[0] == 'm' ||
> +                                                              fmt[0] == 'M' || fmt[0] == 'I')) {
> +                                               fmt++;
> +                                               goto unsupported;
> +                                       }

I wouldn't mind printing the pointer for %p[mMI], but %pa prints the
*content* of the pointer which is really confusing. I.e. in
pinctrl-single.c the reg value pairs are printed like

  dev_dbg(dev, "reg/val %pa/0x%08x\n", &reg, val);

with reg being a pointer to a physical address. So with tiny_printf
the address of reg (which is a pointer to the stack) is printed in
this case.

I don't think we can print %p without putting more logic into the
decoding. I think the culprit here is the fallthrough to %x, which
then leads to the confusing behavior shown above. IMHO if we want to
avoid that, we'd have to make %p entirely unsupported.

diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
index faf55d7f327..8147ffa2c1b 100644
--- a/lib/tiny-printf.c
+++ b/lib/tiny-printf.c
@@ -269,21 +269,18 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va)
 						div_out(info, &num, div);
 				}
 				break;
+#if CONFIG_IS_ENABLED(NET) || CONFIG_IS_ENABLED(NET_LWIP) || _DEBUG
 			case 'p':
-				if (CONFIG_IS_ENABLED(NET) ||
-				    CONFIG_IS_ENABLED(NET_LWIP) || _DEBUG) {
-					pointer(info, fmt, va_arg(va, void *));
-					/*
-					 * Skip this because it pulls in _ctype which is
-					 * 256 bytes, and we don't generally implement
-					 * pointer anyway
-					 */
-					while (isalnum(fmt[0]))
-						fmt++;
-					break;
-				}
-				islong = true;
-				/* no break */
+				pointer(info, fmt, va_arg(va, void *));
+				/*
+				 * Skip this because it pulls in _ctype which is
+				 * 256 bytes, and we don't generally implement
+				 * pointer anyway
+				 */
+				while (isalnum(fmt[0]))
+					fmt++;
+				break;
+#endif
 			case 'x':
 				if (islong) {
 					num = va_arg(va, unsigned long);
@@ -310,6 +307,8 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va)
 			case '%':
 				out(info, '%');
 			default:
+				out(info, '?');
 				break;
 			}

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

RE: [PATCH] tiny-printf: Add support for upper case hex values

[Christoph Niedermaier]

From: Michael Walle <mwalle@kernel.org>
Sent: Wednesday, April 2, 2025 9:02 AM
>>>> The issue is that disabling TINY_PRINTF may not be possible (size
>>>> constraints) and some code is compiled for different stages and people
>>>> typically don't check whether the format used in printf is valid with
>>>> tiny_printf. I've had this issue already in the past, I vaguely recall
>>>> "complaining" about it on IRC.
>>>
>>> Yes, I've stumbled on "%pa" with tiny printf (i.e. in
>>> drivers/pinctrl/pinctrl-single.c) which is printing the very wrong
>>> value, actually :) So printing anything unknown as '?' would really
>>> help here.
>>
>> Something like that would help:
>> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
>> index 48db7b1f78f..b918d6d7386 100644
>> --- a/lib/tiny-printf.c
>> +++ b/lib/tiny-printf.c
>> @@ -280,6 +280,12 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va)
>>                                         while (isalnum(fmt[0]))
>>                                                 fmt++;
>>                                         break;
>> +                               } else {
>> +                                       if (fmt[0] != '\0' && (fmt[0] == 'a' || fmt[0] == 'm' ||
>> +                                                              fmt[0] == 'M' || fmt[0] == 'I')) {
>> +                                               fmt++;
>> +                                               goto unsupported;
>> +                                       }
> 
> I wouldn't mind printing the pointer for %p[mMI], but %pa prints the
> *content* of the pointer which is really confusing. I.e. in
> pinctrl-single.c the reg value pairs are printed like
> 
>   dev_dbg(dev, "reg/val %pa/0x%08x\n", &reg, val);
> 
> with reg being a pointer to a physical address. So with tiny_printf
> the address of reg (which is a pointer to the stack) is printed in
> this case.
> 
> I don't think we can print %p without putting more logic into the
> decoding. I think the culprit here is the fallthrough to %x, which
> then leads to the confusing behavior shown above. IMHO if we want to
> avoid that, we'd have to make %p entirely unsupported.
> 
> diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
> index faf55d7f327..8147ffa2c1b 100644
> --- a/lib/tiny-printf.c
> +++ b/lib/tiny-printf.c
> @@ -269,21 +269,18 @@ static int _vprintf(struct printf_info *info, const char *fmt,
> va_list va)
>  						div_out(info, &num, div);
>  				}
>  				break;
> +#if CONFIG_IS_ENABLED(NET) || CONFIG_IS_ENABLED(NET_LWIP) || _DEBUG

What if we fine-tune tinyprinf via config here?
For example SPL_USE_TINY_PRINTF_POINTER_SUPPORT and
select it by NET or NET_LWIP. If someone needs it,
the pointer output can be enabled, otherwise '?' for
unsupported is output.

>  			case 'p':
> -				if (CONFIG_IS_ENABLED(NET) ||
> -				    CONFIG_IS_ENABLED(NET_LWIP) || _DEBUG) {
> -					pointer(info, fmt, va_arg(va, void *));
> -					/*
> -					 * Skip this because it pulls in _ctype which is
> -					 * 256 bytes, and we don't generally implement
> -					 * pointer anyway
> -					 */
> -					while (isalnum(fmt[0]))
> -						fmt++;
> -					break;
> -				}
> -				islong = true;
> -				/* no break */
> +				pointer(info, fmt, va_arg(va, void *));
> +				/*
> +				 * Skip this because it pulls in _ctype which is
> +				 * 256 bytes, and we don't generally implement
> +				 * pointer anyway
> +				 */
> +				while (isalnum(fmt[0]))
> +					fmt++;
> +				break;
> +#endif
>  			case 'x':
>  				if (islong) {
>  					num = va_arg(va, unsigned long);
> @@ -310,6 +307,8 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va)
>  			case '%':
>  				out(info, '%');
>  			default:
> +				out(info, '?');
>  				break;
>  			}

Regards
Christoph

Re: [PATCH] tiny-printf: Add support for upper case hex values

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]

Hi,

> > I wouldn't mind printing the pointer for %p[mMI], but %pa prints the
> > *content* of the pointer which is really confusing. I.e. in
> > pinctrl-single.c the reg value pairs are printed like
> > 
> >   dev_dbg(dev, "reg/val %pa/0x%08x\n", &reg, val);
> > 
> > with reg being a pointer to a physical address. So with tiny_printf
> > the address of reg (which is a pointer to the stack) is printed in
> > this case.
> > 
> > I don't think we can print %p without putting more logic into the
> > decoding. I think the culprit here is the fallthrough to %x, which
> > then leads to the confusing behavior shown above. IMHO if we want to
> > avoid that, we'd have to make %p entirely unsupported.
> > 
> > diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
> > index faf55d7f327..8147ffa2c1b 100644
> > --- a/lib/tiny-printf.c
> > +++ b/lib/tiny-printf.c
> > @@ -269,21 +269,18 @@ static int _vprintf(struct printf_info *info, const char *fmt,
> > va_list va)
> >  						div_out(info, &num, div);
> >  				}
> >  				break;
> > +#if CONFIG_IS_ENABLED(NET) || CONFIG_IS_ENABLED(NET_LWIP) || _DEBUG
>
> What if we fine-tune tinyprinf via config here?
> For example SPL_USE_TINY_PRINTF_POINTER_SUPPORT and
> select it by NET or NET_LWIP. If someone needs it,
> the pointer output can be enabled, otherwise '?' for
> unsupported is output.

Yeah I had a similar idea, but I'm not sure if yet another config
symbol is worth it. That's up to the maintainer to decide :)

In any case, we have a different behavior to what is printed
right now, as we drop the fallthrough to %x. Tom? Simon?

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

Re: [PATCH] tiny-printf: Add support for upper case hex values

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1873 bytes --]

On Fri, Apr 04, 2025 at 10:33:25AM +0200, Michael Walle wrote:
> Hi,
> 
> > > I wouldn't mind printing the pointer for %p[mMI], but %pa prints the
> > > *content* of the pointer which is really confusing. I.e. in
> > > pinctrl-single.c the reg value pairs are printed like
> > > 
> > >   dev_dbg(dev, "reg/val %pa/0x%08x\n", &reg, val);
> > > 
> > > with reg being a pointer to a physical address. So with tiny_printf
> > > the address of reg (which is a pointer to the stack) is printed in
> > > this case.
> > > 
> > > I don't think we can print %p without putting more logic into the
> > > decoding. I think the culprit here is the fallthrough to %x, which
> > > then leads to the confusing behavior shown above. IMHO if we want to
> > > avoid that, we'd have to make %p entirely unsupported.
> > > 
> > > diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
> > > index faf55d7f327..8147ffa2c1b 100644
> > > --- a/lib/tiny-printf.c
> > > +++ b/lib/tiny-printf.c
> > > @@ -269,21 +269,18 @@ static int _vprintf(struct printf_info *info, const char *fmt,
> > > va_list va)
> > >  						div_out(info, &num, div);
> > >  				}
> > >  				break;
> > > +#if CONFIG_IS_ENABLED(NET) || CONFIG_IS_ENABLED(NET_LWIP) || _DEBUG
> >
> > What if we fine-tune tinyprinf via config here?
> > For example SPL_USE_TINY_PRINTF_POINTER_SUPPORT and
> > select it by NET or NET_LWIP. If someone needs it,
> > the pointer output can be enabled, otherwise '?' for
> > unsupported is output.
> 
> Yeah I had a similar idea, but I'm not sure if yet another config
> symbol is worth it. That's up to the maintainer to decide :)
> 
> In any case, we have a different behavior to what is printed
> right now, as we drop the fallthrough to %x. Tom? Simon?

A Kconfig symbol that NET||NET_LWIP select seems fine, and fall through
to printing "?".

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH] tiny-printf: Add support for upper case hex values

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]

Hi,
> > > > I wouldn't mind printing the pointer for %p[mMI], but %pa prints the
> > > > *content* of the pointer which is really confusing. I.e. in
> > > > pinctrl-single.c the reg value pairs are printed like
> > > > 
> > > >   dev_dbg(dev, "reg/val %pa/0x%08x\n", &reg, val);
> > > > 
> > > > with reg being a pointer to a physical address. So with tiny_printf
> > > > the address of reg (which is a pointer to the stack) is printed in
> > > > this case.
> > > > 
> > > > I don't think we can print %p without putting more logic into the
> > > > decoding. I think the culprit here is the fallthrough to %x, which
> > > > then leads to the confusing behavior shown above. IMHO if we want to
> > > > avoid that, we'd have to make %p entirely unsupported.
> > > > 
> > > > diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
> > > > index faf55d7f327..8147ffa2c1b 100644
> > > > --- a/lib/tiny-printf.c
> > > > +++ b/lib/tiny-printf.c
> > > > @@ -269,21 +269,18 @@ static int _vprintf(struct printf_info *info, const char *fmt,
> > > > va_list va)
> > > >  						div_out(info, &num, div);
> > > >  				}
> > > >  				break;
> > > > +#if CONFIG_IS_ENABLED(NET) || CONFIG_IS_ENABLED(NET_LWIP) || _DEBUG
> > >
> > > What if we fine-tune tinyprinf via config here?
> > > For example SPL_USE_TINY_PRINTF_POINTER_SUPPORT and
> > > select it by NET or NET_LWIP. If someone needs it,
> > > the pointer output can be enabled, otherwise '?' for
> > > unsupported is output.
> > 
> > Yeah I had a similar idea, but I'm not sure if yet another config
> > symbol is worth it. That's up to the maintainer to decide :)
> > 
> > In any case, we have a different behavior to what is printed
> > right now, as we drop the fallthrough to %x. Tom? Simon?
>
> A Kconfig symbol that NET||NET_LWIP select seems fine, and fall through
> to printing "?".

Great! Christoph, will you prepare a patch or should I?

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

RE: [PATCH] tiny-printf: Add support for upper case hex values

[Christoph Niedermaier]

From: Michael Walle <mwalle@kernel.org>
Sent: Monday, April 7, 2025 7:28 AM
> Hi,
> > > > > I wouldn't mind printing the pointer for %p[mMI], but %pa prints the
> > > > > *content* of the pointer which is really confusing. I.e. in
> > > > > pinctrl-single.c the reg value pairs are printed like
> > > > >
> > > > >   dev_dbg(dev, "reg/val %pa/0x%08x\n", &reg, val);
> > > > >
> > > > > with reg being a pointer to a physical address. So with tiny_printf
> > > > > the address of reg (which is a pointer to the stack) is printed in
> > > > > this case.
> > > > >
> > > > > I don't think we can print %p without putting more logic into the
> > > > > decoding. I think the culprit here is the fallthrough to %x, which
> > > > > then leads to the confusing behavior shown above. IMHO if we want to
> > > > > avoid that, we'd have to make %p entirely unsupported.
> > > > >
> > > > > diff --git a/lib/tiny-printf.c b/lib/tiny-printf.c
> > > > > index faf55d7f327..8147ffa2c1b 100644
> > > > > --- a/lib/tiny-printf.c
> > > > > +++ b/lib/tiny-printf.c
> > > > > @@ -269,21 +269,18 @@ static int _vprintf(struct printf_info *info, const char *fmt, va_list va)
> > > > >  						div_out(info, &num, div);
> > > > >  				}
> > > > >  				break;
> > > > > +#if CONFIG_IS_ENABLED(NET) || CONFIG_IS_ENABLED(NET_LWIP) || _DEBUG
> > > >
> > > > What if we fine-tune tinyprinf via config here?
> > > > For example SPL_USE_TINY_PRINTF_POINTER_SUPPORT and
> > > > select it by NET or NET_LWIP. If someone needs it,
> > > > the pointer output can be enabled, otherwise '?' for
> > > > unsupported is output.
> > >
> > > Yeah I had a similar idea, but I'm not sure if yet another config
> > > symbol is worth it. That's up to the maintainer to decide :)
> > >
> > > In any case, we have a different behavior to what is printed
> > > right now, as we drop the fallthrough to %x. Tom? Simon?
> >
> > A Kconfig symbol that NET||NET_LWIP select seems fine, and fall through
> > to printing "?".
> 
> Great! Christoph, will you prepare a patch or should I?

OK, I will prepare a patch.

Regards
Christoph