On Wed, 01 Apr 2020, Sakari Ailus sakari.ailus@linux.intel.com wrote:

Add a printk modifier %ppf (for pixel format) for printing V4L2 and DRM pixel formats denoted by 4ccs. The 4cc encoding is the same for both so the same implementation can be used.

I'm not going to take a strong stand in one way or the other regarding the patch at hand, but I do think at some point we have to draw a line what should be included in printk formats. Arguably they should be reserved to things that are generally useful across large parts of the kernel, right?

I think the more specialized you get, the more you should think about just using the plain old %s, and your own helpers. Because frankly, the kernel printk specifiers also start getting more than a little obscure.

Or could we conceive of a way to make this locally extensible yet safe, letting callers use something like %{foo}, as well as providing a locally relevant function to do the conversion?

BR, Jani.

Suggested-by: Mauro Carvalho Chehab mchehab@kernel.org Signed-off-by: Sakari Ailus sakari.ailus@linux.intel.com

---

Documentation/core-api/printk-formats.rst | 11 +++++++++ lib/vsprintf.c | 29 +++++++++++++++++++++++ 2 files changed, 40 insertions(+)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index 8ebe46b1af39..b6249f513c09 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -545,6 +545,17 @@ For printing netdev_features_t.

Passed by reference.

+V4L2 and DRM fourcc code (pixel format) +---------------------------------------

+::

• %ppf

+Print a 4cc code used by V4L2 or DRM.

+Passed by reference.

Thanks

diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 7c488a1ce318..b39f0ac317c5 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1721,6 +1721,32 @@ char *netdev_bits(char *buf, char *end, const void *addr, return special_hex_number(buf, end, num, size); }

+static noinline_for_stack +char *pixel_format_string(char *buf, char *end, const u32 *fourcc,

• 	  struct printf_spec spec, const char *fmt)
  

+{

• char ch[2] = { 0 };
• unsigned int i;
• if (check_pointer(&buf, end, fourcc, spec))

• return buf;
  

• switch (fmt[1]) {
• case 'f':

• for (i = 0; i < sizeof(*fourcc); i++) {
  

• 	ch[0] = *fourcc >> (i << 3);
  

• 	buf = string(buf, end, ch, spec);
  

• }
  

• if (*fourcc & BIT(31))
  

• 	buf = string(buf, end, "-BE", spec);
  

• return buf;
  

• default:

• return error_string(buf, end, "(%pp?)", spec);
  

• }

+}

static noinline_for_stack char *address_val(char *buf, char *end, const void *addr, struct printf_spec spec, const char *fmt) @@ -2131,6 +2157,7 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,

•   correctness of the format string and va_list arguments.
  

• • 'K' For a kernel pointer that should be hidden from unprivileged users
• • 'NF' For a netdev_features_t

• • • 'pf' V4L2 or DRM pixel format.
  • • 'h[CDN]' For a variable-length buffer, it prints it as a hex string with

  •        a certain separator (' ' by default):
    

  •          C colon
    

@@ -2223,6 +2250,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, return restricted_pointer(buf, end, ptr, spec); case 'N': return netdev_bits(buf, end, ptr, spec, fmt);

• case 'p':

• return pixel_format_string(buf, end, ptr, spec, fmt);
  

  case 'a': return address_val(buf, end, ptr, spec, fmt); case 'd':