Re: [PATCH] tiny-printf: Handle formatting of %p with an extra Kconfig

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

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

On Wed Apr 9, 2025 at 5:22 PM CEST, Tom Rini wrote:
> On Wed, Apr 09, 2025 at 02:33:08PM +0200, Michael Walle wrote:
> > Hi,
> > 
> > > >> The formatting with %pa / %pap behaves like %x, which results in an
> > > >> incorrect value being output. To improve this, a new fine-tuning
> > > >> Kconfig XPL_USE_TINY_PRINTF_POINTER_SUPPORT for pointer formatting
> > > >> has been added. If it is enabled, the output of %pa / %pap should
> > > >> be correct, and if it is disabled, the pointer formatting is
> > > >> completely unsupported. In addition to indicate unsupported formatting,
> > > >> '?' will be output. This allows enabling pointer formatting only
> > > >> when needed. For SPL_NET and NET_LWIP it is selected by default.
> > > >> Then it also supports the formatting with %pm, %pM and %pI4.
> > > >>
> > > >> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> > > >> ---
> > > >> Cc: Tom Rini <trini@konsulko.com>
> > > >> Cc: Simon Glass <sjg@chromium.org>
> > > >> Cc: Michael Walle <mwalle@kernel.org>
> > > >> Cc: Quentin Schulz <quentin.schulz@cherry.de>
> > > >> Cc: Marek Vasut <marex@denx.de>
> > > >> Cc: Benedikt Spranger <b.spranger@linutronix.de>
> > > >> Cc: Jerome Forissier <jerome.forissier@linaro.org>
> > > >> Cc: John Ogness <john.ogness@linutronix.de>
> > > >> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > >> ---
> > > >>  Kconfig            |  1 +
> > > >>  common/spl/Kconfig |  1 +
> > > >>  lib/Kconfig        |  8 ++++++++
> > > >>  lib/tiny-printf.c  | 45 +++++++++++++++++++--------------------------
> > > >>  4 files changed, 29 insertions(+), 26 deletions(-)
> > > >>
> > > >> diff --git a/Kconfig b/Kconfig
> > > >> index 6379a454166..4d13717294c 100644
> > > >> --- a/Kconfig
> > > >> +++ b/Kconfig
> > > >> @@ -757,6 +757,7 @@ config NET
> > > >>
> > > >>  config NET_LWIP
> > > >>  	bool "Use lwIP for networking stack"
> > > >> +	select XPL_USE_TINY_PRINTF_POINTER_SUPPORT if SPL_USE_TINY_PRINTF || TPL_USE_TINY_PRINTF || VPL_USE_TINY_PRINTF
> > > >>  	imply NETDEVICES
> > > >>  	help
> > > >>  	  Include networking support based on the lwIP (lightweight IP)
> > > >> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> > > >> index 94e118f8465..72736dbecf5 100644
> > > >> --- a/common/spl/Kconfig
> > > >> +++ b/common/spl/Kconfig
> > > >> @@ -1096,6 +1096,7 @@ config SPL_DM_SPI_FLASH
> > > >>  config SPL_NET
> > > >>  	bool "Support networking"
> > > >>  	depends on !NET_LWIP
> > > >> +	select XPL_USE_TINY_PRINTF_POINTER_SUPPORT if SPL_USE_TINY_PRINTF || TPL_USE_TINY_PRINTF || VPL_USE_TINY_PRINTF
> > > >>  	help
> > > >>  	  Enable support for network devices (such as Ethernet) in SPL.
> > > >>  	  This permits SPL to load U-Boot over a network link rather than
> > > >> diff --git a/lib/Kconfig b/lib/Kconfig
> > > >> index 1a683dea670..62e28d4a1f3 100644
> > > >> --- a/lib/Kconfig
> > > >> +++ b/lib/Kconfig
> > > >> @@ -253,6 +253,14 @@ config VPL_USE_TINY_PRINTF
> > > >>
> > > >>  	  The supported format specifiers are %c, %s, %u/%d and %x.
> > > >>
> > > >> +config XPL_USE_TINY_PRINTF_POINTER_SUPPORT
> > > >> +	bool "Extend tiny printf with the pointer formatting %p"
> > > >> +	depends on SPL_USE_TINY_PRINTF || TPL_USE_TINY_PRINTF || VPL_USE_TINY_PRINTF
> > > >> +	help
> > > >> +	  This option enables the formatting of pointers %p. It supports
> > > >> +	  %p and %pa / %pap. If this option is selected by SPL_NET or NET_LWIP
> > > >> +	  it also supports the formatting with %pm, %pM and %pI4.
> > > > 
> > > > This isn't quite what I'd like to see. I don't want to start using the
> > > > literal XPL namespace as that will lead to confusion down the line.
> > >
> > > OK, in V2 I will only support SPL.
> > >
> > > > Since we only have SPL_NET, I think we should name this symbol
> > > > SPL_USE_TINY_PRINTF_POINTER_SUPPORT, not ask about it (so bool without
> > > > "prompt text" following), and select from SPL_NET if
> > > > SPL_USE_TINY_PRINTF.
> > 
> > IIRC, the old one also enabled the pointer support if DEBUG is
> > enabled. I don't think this will work with Kconfig.
>
> I was looking around for, but didn't quite see, a good existing option
> to "if .." around the prompt text for.

What I meant was that you normally do -DDEBUG on a file (or
equally a #define DEBUG). Thus you cannot add it to Kconfig.

> > > Now you will get the output '?' when using formatting with %p or %pa.
> > > If someone wants to use the pointer support e.g. %pa in pinctrl-single.c
> > > and is restricted to use tiny printf, then it would be good to have
> > > the option to enable it manually and not be forced to enable SPL_NET or
> > > NET_LWIP to have the pointer support enabled. In this case, it makes
> > > sense to allow switching it on in menuconfig.
> > 
> > FWIW, I'm also fine with enabling full printf support as long as the
> > tiny one doesn't print misleading values.
>
> I'm not sure if the one non-debug %pa print in pinctrl-single.c is
> really triggered within SPL, and I do hope that the way this patch is
> otherwise done will make it easier if someone needs %pa to work when
> debugging a problem in SPL, and can't enable full printf due to space.

Yes, but the dev_dbg() triggered it because of the same reason as
above, DEBUG isn't defined if you do it on a per-file basis. Or I'm
getting that logic wrong, as I don't understand why _DEBUG (commit
c091f65234cf introduced it but doesn't tell the reason).

-michael

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