From: Michael Walle <mwa...@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", ®, 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
