Re: [PATCH v4 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
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
> > _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.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
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
> _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.
--
Kind regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
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.
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
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);
> +}
--
With Best Regards,
Andy Shevchenko
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
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
_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?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
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);
> > +}
>
--
Regards,
Sakari Ailus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
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
Signed-off-by: Sakari Ailus
---
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(+)
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 v
