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

["Michael Walle" <mwalle@kernel.org>]

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