Message-ID: <Zd4qrLVWcacza747@intel.com>
Date: Tue, 27 Feb 2024 20:32:12 +0200
From: Ville Syrjälä <ville.syrjala@...ux.intel.com>
To: Rasmus Villemoes <linux@...musvillemoes.dk>
Cc: Jani Nikula <jani.nikula@...ux.intel.com>,
	Rodrigo Vivi <rodrigo.vivi@...el.com>,
	intel-gfx@...ts.freedesktop.org, Petr Mladek <pmladek@...e.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 01/12] drm/i915: Indicate which pipe failed the fastset
 check overall

On Tue, Feb 27, 2024 at 10:38:10AM +0100, Rasmus Villemoes wrote:
> On 26/02/2024 15.57, Jani Nikula wrote:
> 
> > Personally I suck at remembering even the standard printf conversion
> > specifiers, let alone all the kernel extensions. I basically have to
> > look them up every time. I'd really love some %{name} format for named
> > pointer things. And indeed preferrably without the %p. Just %{name}.
> 
> Sorry to spoil the fun, but that's a non-starter.
> 
> foo.c: In function ‘foo’:
> foo.c:5:24: warning: unknown conversion type character ‘{’ in format
> [-Wformat=]
>     5 |         printf("Hello %{function} World\n", &foo);
>       |                        ^
> 
> You can't start accepting stuff that -Wformat will warn about. We're not
> going to start building with Wno-format.

Are there any sensible looking characters we could use for
this? Ideally I'd like to have something to bracket the
outsides, and perhaps a namespace separator in the middle.

Or are we really forced into having essentially a random set
of characters following just a %p/etc.?

> 
> > And then we could discuss adding support for drm specific things. I
> > guess one downside is that the functions to do this would have to be in
> > vsprintf.c instead of drm. Unless we add some code in drm for this
> > that's always built-in.
> 
> If people can be trusted to write callbacks with the proper semantics
> for snprintf [1], we could do a generic

Yeah, I was at some point thinking that having a version of
register_printf_function() for printk() might be nice. The dangers
being that we get conflicts between subsystems (*), or that it gets
totally out of hand, or as you point out below people will start
to do questionable things in there.

(*) My earlier "include a subsystem namespace in the format" 
idea was basically how I was thinking of avoiding conflicts.

> 
> typedef char * (*printf_callback)(char *buf, char *end, void *ctx);
> 
> struct printf_ext {
>   printf_callback cb;
>   void *ctx;
> };
> 
> #define PRINTF_EXT(callback, context) &(struct printf_ext){ .cb =
> callback, .ctx = context }
> 
> // in drm-land
> 
> char* my_drm_gizmo_formatter(char *buf, char *end, void *ctx)
> {
>   struct drm_gizmo *dg = ctx;
>   ....
>   return buf;
> }
> #define pX_gizmo(dg) PRINTF_EXT(my_drm_gizmo_formatter, dg)
> 
>    printk("error: gizmo %pX in wrong state!\n", pX_gizmo(dg));
> 
> Then vsprintf.c doesn't need to know anything about any particular
> subsystem. And if a subsystem breaks snprintf semantics, they get to
> keep the pieces. With a little more macro magic, one might even be able
> to throw in some type safety checks.
> 
> Rasmus
> 
> [1] You can't sleep, you can't allocate memory, you probably can't even
> take any raw spinlocks, you must attempt to do the full formatting so
> you can tell how much room would be needed, but you must of course not
> write anything beyond end. Calling vsnprintf() to format various integer
> members is probably ok, but recursively using %pX to print full
> subobjects is likely a bad idea.

-- 
Ville Syrjälä
Intel

Powered by blists - more mailing lists