| Message ID | 20201103133400.24805-1-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs | expand |
On Tue, Nov 03, 2020 at 03:34:00PM +0200, Sakari Ailus wrote: > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM > pixel formats denoted by fourccs. The fourcc encoding is the same for both > so the same implementation can be used. ... > +static noinline_for_stack > +char *fourcc_string(char *buf, char *end, const u32 *fourcc, > + struct printf_spec spec, const char *fmt) > +{ > + char output[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")]; I would add a comment that there is another possibility, i.e. big-endian, but it occupies less space. > + char *p = output; > + unsigned int i; > + u32 val; > + > + if (fmt[1] != 'c' || fmt[2] != 'c') > + return error_string(output, end, "(%p4?)", spec); > + > + if (check_pointer(&buf, end, fourcc, spec)) > + return buf; > + > + val = *fourcc & ~BIT(31); > + > + for (i = 0; i < sizeof(*fourcc); i++) { > + unsigned char c = val >> (i * 8); > + > + /* Weed out spaces */ > + if (c == ' ') > + continue; > + > + /* Print non-control ASCII characters as-is */ > + if (isascii(c) && isprint(c)) { > + *p++ = c; > + continue; > + } > + > + *p++ = '('; > + p = hex_byte_pack(p, c); > + *p++ = ')'; > + } I guess above loop can be still optimized, but I have to think about it. > + strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian"); > + p += strlen(p); > + > + *p++ = ' '; > + *p++ = '('; > + /* subtract parenthesis and the space from the size */ This comment misleading. What you are subtracting is a room for closing parenthesis and NUL. > + p = special_hex_number(p, output + sizeof(output) - 2, *fourcc, > + sizeof(u32)); I would go with one line here. The (theoretical) problem is here that the case when buffer size is not enough to print a value will be like '(0xabc)' but should be rather '(0xabcd' like snprintf() does in general. > + *p++ = ')'; > + *p = '\0'; > + > + return string(buf, end, output, spec); > +}
Hi Andy, On Tue, Nov 03, 2020 at 04:47:47PM +0200, Andy Shevchenko wrote: > On Tue, Nov 03, 2020 at 03:34:00PM +0200, Sakari Ailus wrote: > > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM > > pixel formats denoted by fourccs. The fourcc encoding is the same for both > > so the same implementation can be used. > > ... > > > +static noinline_for_stack > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc, > > + struct printf_spec spec, const char *fmt) > > +{ > > + char output[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")]; > > I would add a comment that there is another possibility, i.e. big-endian, but > it occupies less space. This string is here to represent the longest possible output of the function. Most of the time it's less. Saying that might make sense but it's fairly clear already. Either way works for me though. > > > + char *p = output; > > + unsigned int i; > > + u32 val; > > + > > + if (fmt[1] != 'c' || fmt[2] != 'c') > > + return error_string(output, end, "(%p4?)", spec); > > + > > + if (check_pointer(&buf, end, fourcc, spec)) > > + return buf; > > + > > + val = *fourcc & ~BIT(31); > > + > > + for (i = 0; i < sizeof(*fourcc); i++) { > > + unsigned char c = val >> (i * 8); > > + > > + /* Weed out spaces */ > > + if (c == ' ') > > + continue; > > + > > + /* Print non-control ASCII characters as-is */ > > + if (isascii(c) && isprint(c)) { > > + *p++ = c; > > + continue; > > + } > > + > > + *p++ = '('; > > + p = hex_byte_pack(p, c); > > + *p++ = ')'; > > + } > > I guess above loop can be still optimized, but I have to think about it. > > > + strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian"); > > + p += strlen(p); > > + > > + *p++ = ' '; > > + *p++ = '('; > > + /* subtract parenthesis and the space from the size */ > > This comment misleading. What you are subtracting is a room for closing > parenthesis and NUL. Agreed, I'll update it for v5. > > > + p = special_hex_number(p, output + sizeof(output) - 2, *fourcc, > > + sizeof(u32)); > > I would go with one line here. It's wrapped since the result would be over 80 otherwise. > > The (theoretical) problem is here that the case when buffer size is not enough > to print a value will be like '(0xabc)' but should be rather '(0xabcd' like > snprintf() does in general. > > > + *p++ = ')'; > > + *p = '\0'; > > + > > + return string(buf, end, output, spec); > > +} >
On Tue, Nov 03, 2020 at 04:56:16PM +0200, Sakari Ailus wrote: > On Tue, Nov 03, 2020 at 04:47:47PM +0200, Andy Shevchenko wrote: > > On Tue, Nov 03, 2020 at 03:34:00PM +0200, Sakari Ailus wrote: > > > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM > > > pixel formats denoted by fourccs. The fourcc encoding is the same for both > > > so the same implementation can be used. ... > > > + char output[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")]; > > > > I would add a comment that there is another possibility, i.e. big-endian, but > > it occupies less space. > > This string is here to represent the longest possible output of the > function. Most of the time it's less. Saying that might make sense but it's > fairly clear already. Either way works for me though. It's not known by reading the above line. I would add a comment. ... > > > + p = special_hex_number(p, output + sizeof(output) - 2, *fourcc, > > > + sizeof(u32)); > > > > I would go with one line here. > > It's wrapped since the result would be over 80 otherwise. It will be not the first one breaking the limit in this module for the sake of readability. ... > > The (theoretical) problem is here that the case when buffer size is not enough > > to print a value will be like '(0xabc)' but should be rather '(0xabcd' like > > snprintf() does in general. Any comments here? Perhaps you need to add a comment to explain that the overflow is impossible.
On Tue, 2020-11-03 at 16:56 +0200, Sakari Ailus wrote: > On Tue, Nov 03, 2020 at 04:47:47PM +0200, Andy Shevchenko wrote: > > On Tue, Nov 03, 2020 at 03:34:00PM +0200, Sakari Ailus wrote: > > > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM > > > pixel formats denoted by fourccs. The fourcc encoding is the same for both > > > so the same implementation can be used. > > > > ... > > > > > +static noinline_for_stack > > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc, > > > + struct printf_spec spec, const char *fmt) > > > +{ > > > + char output[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")]; > > > > I would add a comment that there is another possibility, i.e. big-endian, but > > it occupies less space. I think it's unnecessary as it's obvious and similarly done in other <foo>_string type functions. > > > + p = special_hex_number(p, output + sizeof(output) - 2, *fourcc, > > > + sizeof(u32)); > > > > I would go with one line here. > > It's wrapped since the result would be over 80 otherwise. Perhaps simpler as p = special_hex_number(p, p + 10, *fourcc, sizeof(u32)); > > The (theoretical) problem is here that the case when buffer size is not enough > > to print a value will be like '(0xabc)' but should be rather '(0xabcd' like > > snprintf() does in general. Isn't the stack buffer known to be large enough? > > > + *p++ = ')'; > > > + *p = '\0'; > > > + > > > + return string(buf, end, output, spec); Isn't the actual output buffer used here truncating output? If the general problem is someone using a limited length pointer output like %10p4cc, then all the output is getting truncated no?
Hi Joe, On Tue, Nov 03, 2020 at 08:49:36AM -0800, Joe Perches wrote: > On Tue, 2020-11-03 at 16:56 +0200, Sakari Ailus wrote: > > On Tue, Nov 03, 2020 at 04:47:47PM +0200, Andy Shevchenko wrote: > > > On Tue, Nov 03, 2020 at 03:34:00PM +0200, Sakari Ailus wrote: > > > > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM > > > > pixel formats denoted by fourccs. The fourcc encoding is the same for both > > > > so the same implementation can be used. > > > > > > ... > > > > > > > +static noinline_for_stack > > > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc, > > > > + struct printf_spec spec, const char *fmt) > > > > +{ > > > > + char output[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")]; > > > > > > I would add a comment that there is another possibility, i.e. big-endian, but > > > it occupies less space. > > I think it's unnecessary as it's obvious and similarly done in other > <foo>_string type functions. > > > > > + p = special_hex_number(p, output + sizeof(output) - 2, *fourcc, > > > > + sizeof(u32)); > > > > > > I would go with one line here. > > > > It's wrapped since the result would be over 80 otherwise. > > Perhaps simpler as > > p = special_hex_number(p, p + 10, *fourcc, sizeof(u32)); Yes. But having bugs elsewhere would have a magnified effect. I wouldn't be afraid of a newline here. > > > > The (theoretical) problem is here that the case when buffer size is not enough > > > to print a value will be like '(0xabc)' but should be rather '(0xabcd' like > > > snprintf() does in general. > > Isn't the stack buffer known to be large enough? Yes. If there are no bugs, that is. > > > > > + *p++ = ')'; > > > > + *p = '\0'; > > > > + > > > > + return string(buf, end, output, spec); > > Isn't the actual output buffer used here truncating output? > > If the general problem is someone using a limited length pointer > output like %10p4cc, then all the output is getting truncated no? Correct. But I'd also be surprised if anyone ever used this feature.
On Fri, 2020-11-13 at 12:54 +0200, Sakari Ailus wrote: > Hi Joe, > > On Tue, Nov 03, 2020 at 08:49:36AM -0800, Joe Perches wrote: > > On Tue, 2020-11-03 at 16:56 +0200, Sakari Ailus wrote: > > > On Tue, Nov 03, 2020 at 04:47:47PM +0200, Andy Shevchenko wrote: > > > > On Tue, Nov 03, 2020 at 03:34:00PM +0200, Sakari Ailus wrote: > > > > > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM > > > > > pixel formats denoted by fourccs. The fourcc encoding is the same for both > > > > > so the same implementation can be used. > > > > > > > > ... > > > > > > > > > +static noinline_for_stack > > > > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc, > > > > > + struct printf_spec spec, const char *fmt) > > > > > +{ > > > > > + char output[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")]; > > > > > > > > I would add a comment that there is another possibility, i.e. big-endian, but > > > > it occupies less space. > > > > I think it's unnecessary as it's obvious and similarly done in other > > <foo>_string type functions. > > > > > > > + p = special_hex_number(p, output + sizeof(output) - 2, *fourcc, > > > > > + sizeof(u32)); > > > > > > > > I would go with one line here. > > > > > > It's wrapped since the result would be over 80 otherwise. > > > > Perhaps simpler as > > > > p = special_hex_number(p, p + 10, *fourcc, sizeof(u32)); > > Yes. But having bugs elsewhere would have a magnified effect. How's that? Where would "elsewhere" be? > I wouldn't be afraid of a newline here. I'd prefer obvious code instead of indirected p vs output and having to lookup whatever output is again. special_hex_number is already known to fit in the buffer.
diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index 6d26c5c6ac48..080262d2e030 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -565,6 +565,22 @@ For printing netdev_features_t. Passed by reference. +V4L2 and DRM FourCC code (pixel format) +--------------------------------------- + +:: + + %p4cc + +Print a FourCC code used by V4L2 or DRM, including format endianness and +its numerical value as hexadecimal. + +Passed by reference. + +Examples:: + + %p4cc BG12 little-endian (0x32314742) + Thanks ====== diff --git a/lib/test_printf.c b/lib/test_printf.c index 7ac87f18a10f..7fb042542660 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -649,6 +649,22 @@ static void __init fwnode_pointer(void) software_node_unregister(&softnodes[0]); } +static void __init fourcc_pointer(void) +{ + struct { + u32 code; + char *str; + } const try[] = { + { 0x20104646, "FF(10) little-endian (0x20104646)", }, + { 0xa0104646, "FF(10) big-endian (0xa0104646)", }, + { 0x10111213, "(13)(12)(11)(10) little-endian (0x10111213)", }, + }; + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(try); i++) + test(try[i].str, "%p4cc", &try[i].code); +} + static void __init errptr(void) { @@ -694,6 +710,7 @@ test_pointer(void) flags(); errptr(); fwnode_pointer(); + fourcc_pointer(); } static void __init selftest(void) diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 14c9a6af1b23..2be86b302c88 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1733,6 +1733,55 @@ char *netdev_bits(char *buf, char *end, const void *addr, return special_hex_number(buf, end, num, size); } +static noinline_for_stack +char *fourcc_string(char *buf, char *end, const u32 *fourcc, + struct printf_spec spec, const char *fmt) +{ + char output[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")]; + char *p = output; + unsigned int i; + u32 val; + + if (fmt[1] != 'c' || fmt[2] != 'c') + return error_string(output, end, "(%p4?)", spec); + + if (check_pointer(&buf, end, fourcc, spec)) + return buf; + + val = *fourcc & ~BIT(31); + + for (i = 0; i < sizeof(*fourcc); i++) { + unsigned char c = val >> (i * 8); + + /* Weed out spaces */ + if (c == ' ') + continue; + + /* Print non-control ASCII characters as-is */ + if (isascii(c) && isprint(c)) { + *p++ = c; + continue; + } + + *p++ = '('; + p = hex_byte_pack(p, c); + *p++ = ')'; + } + + strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian"); + p += strlen(p); + + *p++ = ' '; + *p++ = '('; + /* subtract parenthesis and the space from the size */ + p = special_hex_number(p, output + sizeof(output) - 2, *fourcc, + sizeof(u32)); + *p++ = ')'; + *p = '\0'; + + return string(buf, end, output, spec); +} + static noinline_for_stack char *address_val(char *buf, char *end, const void *addr, struct printf_spec spec, const char *fmt) @@ -2162,6 +2211,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 + * - '4cc' V4L2 or DRM FourCC code, with endianness and raw numerical value. * - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with * a certain separator (' ' by default): * C colon @@ -2259,6 +2309,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 '4': + return fourcc_string(buf, end, ptr, spec, fmt); case 'a': return address_val(buf, end, ptr, spec, fmt); case 'd':
Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM pixel formats denoted by fourccs. The fourcc encoding is the same for both so the same implementation can be used. Suggested-by: Mauro Carvalho Chehab <mchehab@kernel.org> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- Hi folks, I believe I've addressed comments from Rasmus, Joe and Andy --- variables that don't need to be negative have remained unsigned though. Thanks for the suggestions, I believe this is much cleaner than v3. since v3: - Remove use of WARN_ON, assume the code is correct instead. A side effect of this is the code is much easier to understand. - Check the modifier first before validating the buf pointer. - Use isascii() and isprint() functions to weed out non-printable characters. - Use hex_byte_pack() for printing 8-bit hexadecimal numbers. - Remove macros, and instead use plain strings. - Use strcpy() to copy the endianness string, and then strlen() to add its length. - Strip the MSB (endianness bit), then use the value as such. - Assign characters to the buffer and increment active pointer at the same time. - Drop __ from variable names. - Add example to documentation. Documentation/core-api/printk-formats.rst | 16 +++++++ lib/test_printf.c | 17 ++++++++ lib/vsprintf.c | 52 +++++++++++++++++++++++ 3 files changed, 85 insertions(+)