Re: [PATCH v4 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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