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 --- since v2:
- Add comments to explain why things are being done
- Print characters under 32 (space) as hexadecimals in parenthesis.
- Do not print spaces in the fourcc codes.
- Make use of a loop over the fourcc characters instead of put_unaligned_le32(). This is necessary to omit spaces in the output.
- Use DRM style format instead of V4L2. This provides the precise code as a numerical value as well as explicit endianness information.
- Added WARN_ON_ONCE() sanity checks. Comments on these are welcome; I'd expect them mostly be covered by the tests.
- Added tests for %p4cc in lib/test_printf.c
Documentation/core-api/printk-formats.rst | 12 ++++ lib/test_printf.c | 17 +++++ lib/vsprintf.c | 86 +++++++++++++++++++++++ 3 files changed, 115 insertions(+)
diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index 8ebe46b1af39..7aa0451e06fb 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -545,6 +545,18 @@ 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. + Thanks ======
diff --git a/lib/test_printf.c b/lib/test_printf.c index 2d9f520d2f27..a14754086707 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -624,6 +624,22 @@ static void __init fwnode_pointer(void) software_node_unregister_nodes(softnodes); }
+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) { @@ -668,6 +684,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 7c488a1ce318..02e7906619c0 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1721,6 +1721,89 @@ 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) +{ +#define FOURCC_HEX_CHAR_STR "(xx)" +#define FOURCC_BIG_ENDIAN_STR " big-endian" +#define FOURCC_LITTLE_ENDIAN_STR " little-endian" +#define FOURCC_HEX_NUMBER " (0x01234567)" +#define FOURCC_STRING_MAX \ + FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR \ + FOURCC_HEX_CHAR_STR FOURCC_LITTLE_ENDIAN_STR FOURCC_HEX_NUMBER + struct printf_spec my_spec = { + .type = FORMAT_TYPE_UINT, + .field_width = 2, + .flags = SMALL, + .base = 16, + .precision = -1, + }; + char __s[sizeof(FOURCC_STRING_MAX)]; + char *s = __s; + unsigned int i; + /* + * The 31st bit defines the endianness of the data, so save its printing + * for later. + */ + u32 fourcc = *__fourcc & ~BIT(31); + int ret; + + if (check_pointer(&buf, end, __fourcc, spec)) + return buf; + + if (fmt[1] != 'c' || fmt[2] != 'c') + return error_string(buf, end, "(%p4?)", spec); + + for (i = 0; i < sizeof(fourcc); i++, fourcc >>= 8) { + unsigned char c = fourcc; + + /* Weed out spaces */ + if (c == ' ') + continue; + + /* Print non-control characters as-is */ + if (c > ' ') { + *s = c; + s++; + continue; + } + + if (WARN_ON_ONCE(sizeof(__s) < + (s - __s) + sizeof(FOURCC_HEX_CHAR_STR))) + break; + + *s = '('; + s++; + s = number(s, s + 2, c, my_spec); + *s = ')'; + s++; + } + + ret = strscpy(s, *__fourcc & BIT(31) ? FOURCC_BIG_ENDIAN_STR + : FOURCC_LITTLE_ENDIAN_STR, + sizeof(__s) - (s - __s)); + if (!WARN_ON_ONCE(ret < 0)) + s += ret; + + if (!WARN_ON_ONCE(sizeof(__s) < + (s - __s) + sizeof(FOURCC_HEX_NUMBER))) { + *s = ' '; + s++; + *s = '('; + s++; + /* subtract parentheses and the space from the size */ + special_hex_number(s, s + sizeof(FOURCC_HEX_NUMBER) - 3, + *__fourcc, sizeof(u32)); + s += sizeof(u32) * 2 + 2 /* 0x */; + *s = ')'; + s++; + *s = '\0'; + } + + return string(buf, end, __s, spec); +} + static noinline_for_stack char *address_val(char *buf, char *end, const void *addr, struct printf_spec spec, const char *fmt) @@ -2131,6 +2214,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 @@ -2223,6 +2307,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':
On Mon, Apr 27, 2020 at 05:53:03PM +0300, 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.
4cc is more generic than pixel format.
...
+V4L2 and DRM FourCC code (pixel format) +---------------------------------------
+::
- %p4cc
Missed examples.
+Print a FourCC code used by V4L2 or DRM, including format endianness and +its numerical value as hexadecimal.
...
+static noinline_for_stack +char *fourcc_string(char *buf, char *end, const u32 *__fourcc,
struct printf_spec spec, const char *fmt)
+{ +#define FOURCC_HEX_CHAR_STR "(xx)" +#define FOURCC_BIG_ENDIAN_STR " big-endian" +#define FOURCC_LITTLE_ENDIAN_STR " little-endian" +#define FOURCC_HEX_NUMBER " (0x01234567)"
Where are #undef:s?
+#define FOURCC_STRING_MAX \
- FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR \
- FOURCC_HEX_CHAR_STR FOURCC_LITTLE_ENDIAN_STR FOURCC_HEX_NUMBER
- struct printf_spec my_spec = {
.type = FORMAT_TYPE_UINT,
.field_width = 2,
.flags = SMALL,
.base = 16,
.precision = -1,
- };
Seems like close enough to bus_spec.
- char __s[sizeof(FOURCC_STRING_MAX)];
- char *s = __s;
- unsigned int i;
- /*
* The 31st bit defines the endianness of the data, so save its printing
* for later.
*/
- u32 fourcc = *__fourcc & ~BIT(31);
- int ret;
- if (check_pointer(&buf, end, __fourcc, spec))
return buf;
- if (fmt[1] != 'c' || fmt[2] != 'c')
return error_string(buf, end, "(%p4?)", spec);
- for (i = 0; i < sizeof(fourcc); i++, fourcc >>= 8) {
unsigned char c = fourcc;
/* Weed out spaces */
if (c == ' ')
continue;
/* Print non-control characters as-is */
if (c > ' ') {
*s = c;
s++;
continue;
}
if (WARN_ON_ONCE(sizeof(__s) <
(s - __s) + sizeof(FOURCC_HEX_CHAR_STR)))
Why WARN?!
break;
*s = '(';
s++;
s = number(s, s + 2, c, my_spec);
*s = ')';
s++;
- }
- ret = strscpy(s, *__fourcc & BIT(31) ? FOURCC_BIG_ENDIAN_STR
: FOURCC_LITTLE_ENDIAN_STR,
sizeof(__s) - (s - __s));
- if (!WARN_ON_ONCE(ret < 0))
Ditto.
s += ret;
- if (!WARN_ON_ONCE(sizeof(__s) <
(s - __s) + sizeof(FOURCC_HEX_NUMBER))) {
Ditto.
*s = ' ';
s++;
*s = '(';
s++;
/* subtract parentheses and the space from the size */
special_hex_number(s, s + sizeof(FOURCC_HEX_NUMBER) - 3,
*__fourcc, sizeof(u32));
s += sizeof(u32) * 2 + 2 /* 0x */;
*s = ')';
s++;
*s = '\0';
- }
- return string(buf, end, __s, spec);
This looks overengineered. Why everyone will need so long strings to be printed?
+}
On 27/04/2020 16.53, 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.
Suggested-by: Mauro Carvalho Chehab mchehab@kernel.org Signed-off-by: Sakari Ailus sakari.ailus@linux.intel.com
since v2:
Add comments to explain why things are being done
Print characters under 32 (space) as hexadecimals in parenthesis.
Do not print spaces in the fourcc codes.
Make use of a loop over the fourcc characters instead of put_unaligned_le32(). This is necessary to omit spaces in the output.
Use DRM style format instead of V4L2. This provides the precise code as a numerical value as well as explicit endianness information.
Added WARN_ON_ONCE() sanity checks. Comments on these are welcome; I'd expect them mostly be covered by the tests.
Added tests for %p4cc in lib/test_printf.c
Documentation/core-api/printk-formats.rst | 12 ++++ lib/test_printf.c | 17 +++++ lib/vsprintf.c | 86 +++++++++++++++++++++++ 3 files changed, 115 insertions(+)
diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index 8ebe46b1af39..7aa0451e06fb 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -545,6 +545,18 @@ 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.
Thanks
diff --git a/lib/test_printf.c b/lib/test_printf.c index 2d9f520d2f27..a14754086707 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -624,6 +624,22 @@ static void __init fwnode_pointer(void) software_node_unregister_nodes(softnodes); }
+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) { @@ -668,6 +684,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 7c488a1ce318..02e7906619c0 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1721,6 +1721,89 @@ 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)
+{ +#define FOURCC_HEX_CHAR_STR "(xx)" +#define FOURCC_BIG_ENDIAN_STR " big-endian" +#define FOURCC_LITTLE_ENDIAN_STR " little-endian" +#define FOURCC_HEX_NUMBER " (0x01234567)" +#define FOURCC_STRING_MAX \
- FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR \
- FOURCC_HEX_CHAR_STR FOURCC_LITTLE_ENDIAN_STR FOURCC_HEX_NUMBER
- struct printf_spec my_spec = {
.type = FORMAT_TYPE_UINT,
.field_width = 2,
.flags = SMALL,
.base = 16,
.precision = -1,
- };
- char __s[sizeof(FOURCC_STRING_MAX)];
- char *s = __s;
- unsigned int i;
- /*
* The 31st bit defines the endianness of the data, so save its printing
* for later.
*/
- u32 fourcc = *__fourcc & ~BIT(31);
- int ret;
Dereference __fourcc ...
- if (check_pointer(&buf, end, __fourcc, spec))
return buf;
.. and then sanity check it?
- if (fmt[1] != 'c' || fmt[2] != 'c')
return error_string(buf, end, "(%p4?)", spec);
Doesn't that want to come before everything else, including the check_pointer()?
- for (i = 0; i < sizeof(fourcc); i++, fourcc >>= 8) {
unsigned char c = fourcc;
/* Weed out spaces */
if (c == ' ')
continue;
/* Print non-control characters as-is */
if (c > ' ') {
*s = c;
s++;
continue;
}
Are you sure you want to pass non-ascii characters through?
if (WARN_ON_ONCE(sizeof(__s) <
(s - __s) + sizeof(FOURCC_HEX_CHAR_STR)))
break;
I really don't see the point of these checks. Why check here but not before we output a non-control character? That seems rather arbitrary. Also, assume we do take this break, (to be continued below [*])
*s = '(';
s++;
s = number(s, s + 2, c, my_spec);
You can drop my_spec and just use "s = hex_byte_pack(s, c);".
*s = ')';
s++;
- }
- ret = strscpy(s, *__fourcc & BIT(31) ? FOURCC_BIG_ENDIAN_STR
: FOURCC_LITTLE_ENDIAN_STR,
sizeof(__s) - (s - __s));
- if (!WARN_ON_ONCE(ret < 0))
s += ret;
- if (!WARN_ON_ONCE(sizeof(__s) <
(s - __s) + sizeof(FOURCC_HEX_NUMBER))) {
[*] then AFAICT this WARN_ON_ONCE is guaranteed to also fire [the left-hand side is the same as before, the right-hand side consists of a quantity s-__s that can only be larger or equal than before and a constant sizeof(FOURCC_HEX_NUMBER) that is definitely larger], hence we do not enter this if () and [**]
*s = ' ';
s++;
*s = '(';
s++;
/* subtract parentheses and the space from the size */
special_hex_number(s, s + sizeof(FOURCC_HEX_NUMBER) - 3,
*__fourcc, sizeof(u32));
Urgh. Don't we have an ARRAY_END() macro that let's you call this with (s, ARRAY_END(__s)) as buf, end?
s += sizeof(u32) * 2 + 2 /* 0x */;
Why, when special_hex_number returns a pointer to one-past-its-output?
*s = ')';
s++;
*s = '\0';
- }
- return string(buf, end, __s, spec);
[**] __s does not get nul-terminated, so we call string() with stack garbage.
I'd say just drop those checks, and de-obfuscate the sizing of the __s array so it becomes obviously large enough for what the algorithm can produce. Just
char __s[sizeof("(01)(02)(03)(04) little-endian (0x01020304)")]
or something like that should be sufficient.
Rasmus
Hi Rasmus,
Thanks for the review.
On Mon, Apr 27, 2020 at 05:44:00PM +0200, Rasmus Villemoes wrote:
On 27/04/2020 16.53, 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.
Suggested-by: Mauro Carvalho Chehab mchehab@kernel.org Signed-off-by: Sakari Ailus sakari.ailus@linux.intel.com
since v2:
Add comments to explain why things are being done
Print characters under 32 (space) as hexadecimals in parenthesis.
Do not print spaces in the fourcc codes.
Make use of a loop over the fourcc characters instead of put_unaligned_le32(). This is necessary to omit spaces in the output.
Use DRM style format instead of V4L2. This provides the precise code as a numerical value as well as explicit endianness information.
Added WARN_ON_ONCE() sanity checks. Comments on these are welcome; I'd expect them mostly be covered by the tests.
Added tests for %p4cc in lib/test_printf.c
Documentation/core-api/printk-formats.rst | 12 ++++ lib/test_printf.c | 17 +++++ lib/vsprintf.c | 86 +++++++++++++++++++++++ 3 files changed, 115 insertions(+)
diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index 8ebe46b1af39..7aa0451e06fb 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -545,6 +545,18 @@ 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.
Thanks
diff --git a/lib/test_printf.c b/lib/test_printf.c index 2d9f520d2f27..a14754086707 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -624,6 +624,22 @@ static void __init fwnode_pointer(void) software_node_unregister_nodes(softnodes); }
+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) { @@ -668,6 +684,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 7c488a1ce318..02e7906619c0 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1721,6 +1721,89 @@ 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)
+{ +#define FOURCC_HEX_CHAR_STR "(xx)" +#define FOURCC_BIG_ENDIAN_STR " big-endian" +#define FOURCC_LITTLE_ENDIAN_STR " little-endian" +#define FOURCC_HEX_NUMBER " (0x01234567)" +#define FOURCC_STRING_MAX \
- FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR \
- FOURCC_HEX_CHAR_STR FOURCC_LITTLE_ENDIAN_STR FOURCC_HEX_NUMBER
- struct printf_spec my_spec = {
.type = FORMAT_TYPE_UINT,
.field_width = 2,
.flags = SMALL,
.base = 16,
.precision = -1,
- };
- char __s[sizeof(FOURCC_STRING_MAX)];
- char *s = __s;
- unsigned int i;
- /*
* The 31st bit defines the endianness of the data, so save its printing
* for later.
*/
- u32 fourcc = *__fourcc & ~BIT(31);
- int ret;
Dereference __fourcc ...
- if (check_pointer(&buf, end, __fourcc, spec))
return buf;
.. and then sanity check it?
- if (fmt[1] != 'c' || fmt[2] != 'c')
return error_string(buf, end, "(%p4?)", spec);
Doesn't that want to come before everything else, including the check_pointer()?
Agreed on all three.
- for (i = 0; i < sizeof(fourcc); i++, fourcc >>= 8) {
unsigned char c = fourcc;
/* Weed out spaces */
if (c == ' ')
continue;
/* Print non-control characters as-is */
if (c > ' ') {
*s = c;
s++;
continue;
}
Are you sure you want to pass non-ascii characters through?
I'll print the hexadecimal value in v4.
if (WARN_ON_ONCE(sizeof(__s) <
(s - __s) + sizeof(FOURCC_HEX_CHAR_STR)))
break;
I really don't see the point of these checks. Why check here but not before we output a non-control character? That seems rather arbitrary. Also, assume we do take this break, (to be continued below [*])
Will remove.
*s = '(';
s++;
s = number(s, s + 2, c, my_spec);
You can drop my_spec and just use "s = hex_byte_pack(s, c);".
Ack, this cleans it up indeed.
*s = ')';
s++;
- }
- ret = strscpy(s, *__fourcc & BIT(31) ? FOURCC_BIG_ENDIAN_STR
: FOURCC_LITTLE_ENDIAN_STR,
sizeof(__s) - (s - __s));
- if (!WARN_ON_ONCE(ret < 0))
s += ret;
- if (!WARN_ON_ONCE(sizeof(__s) <
(s - __s) + sizeof(FOURCC_HEX_NUMBER))) {
[*] then AFAICT this WARN_ON_ONCE is guaranteed to also fire [the left-hand side is the same as before, the right-hand side consists of a quantity s-__s that can only be larger or equal than before and a constant sizeof(FOURCC_HEX_NUMBER) that is definitely larger], hence we do not enter this if () and [**]
*s = ' ';
s++;
*s = '(';
s++;
/* subtract parentheses and the space from the size */
special_hex_number(s, s + sizeof(FOURCC_HEX_NUMBER) - 3,
*__fourcc, sizeof(u32));
Urgh. Don't we have an ARRAY_END() macro that let's you call this with (s, ARRAY_END(__s)) as buf, end?
We do, yes. In drivers/block/floppy.c. :-)
s += sizeof(u32) * 2 + 2 /* 0x */;
Why, when special_hex_number returns a pointer to one-past-its-output?
I'll use that for v4.
*s = ')';
s++;
*s = '\0';
- }
- return string(buf, end, __s, spec);
[**] __s does not get nul-terminated, so we call string() with stack garbage.
I'd say just drop those checks, and de-obfuscate the sizing of the __s array so it becomes obviously large enough for what the algorithm can produce. Just
char __s[sizeof("(01)(02)(03)(04) little-endian (0x01020304)")]
or something like that should be sufficient.
Done.
On Mon, 2020-04-27 at 17:53 +0300, 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.
[]
- Added WARN_ON_ONCE() sanity checks. Comments on these are welcome; I'd expect them mostly be covered by the tests.
All the WARN_ON_ONCE uses are not necessary.
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
@@ -1721,6 +1721,89 @@ 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)
There's no reason to use __ prefixed argument names here.
+{ +#define FOURCC_HEX_CHAR_STR "(xx)" +#define FOURCC_BIG_ENDIAN_STR " big-endian" +#define FOURCC_LITTLE_ENDIAN_STR " little-endian"
I don't believe used-once macro defines are useful.
+#define FOURCC_HEX_NUMBER " (0x01234567)"
+#define FOURCC_STRING_MAX \
- FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR FOURCC_HEX_CHAR_STR \
- FOURCC_HEX_CHAR_STR FOURCC_LITTLE_ENDIAN_STR FOURCC_HEX_NUMBER
This is very difficult to read and is size dependent on the size of little-endian > big_endian
I'd write it out
FOURCC_STRING_MAX sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")
and then not have the define at all but use the written out string in the declaration.
- struct printf_spec my_spec = {
.type = FORMAT_TYPE_UINT,
.field_width = 2,
.flags = SMALL,
.base = 16,
.precision = -1,
- };
my_spec isn't usefully named, likely not necessary at all, and if really necessary, it should be static const
- char __s[sizeof(FOURCC_STRING_MAX)];
I'd declare the buffer
char fourcc[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)"];
like all the other specialty functions do.
- char *s = __s;
There's no reason to use __ prefixed variable names here either. All the other specialty function use:
char *p = <output_buffer_name>;
- unsigned int i;
just int i; is typical
- /*
* The 31st bit defines the endianness of the data, so save its printing
* for later.
*/
- u32 fourcc = *__fourcc & ~BIT(31);
bool be = fourcc & BIT(31);
- int ret;
- if (check_pointer(&buf, end, __fourcc, spec))
return buf;
- if (fmt[1] != 'c' || fmt[2] != 'c')
return error_string(buf, end, "(%p4?)", spec);
- for (i = 0; i < sizeof(fourcc); i++, fourcc >>= 8) {
unsigned char c = fourcc >> (i*8);
Please remove the fourcc >>= 8 from the loop and use
unsigned char c = fourcc >> (i*8);
If I understand this correctly, I think this is simpler:
if (isascii(c) && isprint(c)) { *s++ = c; } else { *s++ = '('; s = hex_byte_pack(s, c); *s++ = ')'; }
/* Weed out spaces */
if (c == ' ')
continue;
/* Print non-control characters as-is */
if (c > ' ') {
*s = c;
s++;
continue;
}
if (WARN_ON_ONCE(sizeof(__s) <
(s - __s) + sizeof(FOURCC_HEX_CHAR_STR)))
break;
*s = '(';
s++;
s = number(s, s + 2, c, my_spec);
*s = ')';
s++;
- }
- ret = strscpy(s, *__fourcc & BIT(31) ? FOURCC_BIG_ENDIAN_STR
: FOURCC_LITTLE_ENDIAN_STR,
sizeof(__s) - (s - __s));
If you size the initial buffer correctly, strscpy is unnecessary.
strcpy(s, <bit31> ? "big endian" : "little endian"); s += strlen(s);
- if (!WARN_ON_ONCE(ret < 0))
s += ret;
- if (!WARN_ON_ONCE(sizeof(__s) <
(s - __s) + sizeof(FOURCC_HEX_NUMBER))) {
*s = ' ';
s++;
*s = '(';
s++;
*s++ = ' '; *s++ = '(';
+ /* subtract parentheses and the space from the size */
special_hex_number(s, s + sizeof(FOURCC_HEX_NUMBER) - 3,
*__fourcc, sizeof(u32));
s += sizeof(u32) * 2 + 2 /* 0x */;
*s = ')';
s++;
*s++ = ')';
*s = '\0';
- }
- return string(buf, end, __s, spec);
+}
On Mon, 2020-04-27 at 09:02 -0700, Joe Perches wrote:
On Mon, 2020-04-27 at 17:53 +0300, 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.
[]
- Added WARN_ON_ONCE() sanity checks. Comments on these are welcome; I'd expect them mostly be covered by the tests.
perhaps this is simpler? --- lib/vsprintf.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 7c488a..3e1dbd7 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1721,6 +1721,46 @@ 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; + int i; + u32 val; + + if (check_pointer(&buf, end, fourcc, spec)) + return buf; + + if (fmt[1] != 'c' || fmt[2] != 'c') + return error_string(buf, end, "(%p4?)", spec); + + val = *fourcc & ~BIT(31); + + for (i = 0; i < 4; i++) { + unsigned char c = val >> (i * 8); + + if (isascii(c) && isprint(c)) { + *p++ = c; + } else { + *p++ = '('; + p = hex_byte_pack(p, c); + *p++ = ')'; + } + } + + strcpy(p, *fourcc & BIT(31) ? "big endian" : "little endian"); + p += strlen(p); + *p++ = ' '; + *p++ = '('; + p = special_hex_number(p, p + 10, val, 4); + *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) @@ -2131,6 +2171,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 @@ -2223,6 +2264,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':
dri-devel@lists.freedesktop.org