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

[Christoph Niedermaier <cniedermaier@dh-electronics.com>]

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