Re: [PATCH v2 2/3] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Aditya Garg <gargaditya08@live.com>]

> On 21 Feb 2025, at 8:57 PM, andriy.shevchenko@linux.intel.com wrote:
> 
> On Thu, Feb 20, 2025 at 04:39:23PM +0000, Aditya Garg wrote:
>> From: Hector Martin <marcan@marcan.st>
>> 
>> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
>> it's useful to be able to print generic 4-character codes formatted as
>> an integer. Extend it to add format specifiers for printing generic
>> 32-bit FOURCCs with various endian semantics:
>> 
>> %p4ch   Host-endian
>> %p4cl Little-endian
>> %p4cb Big-endian
>> %p4cr Reverse-endian
>> 
>> The endianness determines how bytes are interpreted as a u32, and the
>> FOURCC is then always printed MSByte-first (this is the opposite of
>> V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would
>> allow printing LSByte-first FOURCCs stored in host endian order
>> (other than the hex form being in character order, not the integer
>> value).
> 
> ...
> 
>> orig = get_unaligned(fourcc);
>> - val = orig & ~BIT(31);
>> + switch (fmt[2]) {
>> + case 'h':
>> + val = orig;
>> + break;
>> + case 'r':
>> + orig = swab32(orig);
>> + val = orig;
>> + break;
>> + case 'l':
> 
>> + orig = le32_to_cpu(orig);
>> + val = orig;
>> + break;
>> + case 'b':
>> + orig = be32_to_cpu(orig);
> 
> I do not see that orig is a union of different types. Have you run sparse?
> It will definitely complain on this code.

After messing around with this, what I’ve noticed is that orig and val used in this struct should be u32. Now in case of little endian and big endian, that things are messy. The original code by Hector was using le32_to_cpu on orig, which itself is declared as a u32 here (maybe was done with the intention to convert le32 orig to u32 orig?).

Anyways, what I have done is that:

1. Declare new variable, orig_le which is __le32.
2. Instead of doing orig = le32_to_cpu(orig); , we can do orig_le = cpu_to_le32(orig). This fixes the sparse warning: cast to restricted __le32
3. Now the original code was intending to use val=orig=le32_to_cpu(orig) at the bottom part of this struct. Those parts also require val and orig to be u32. For that, we are now using le32_to_cpu(orig_le). Since val is same as orig, in case these cases, instead of making a val_le, I’ve simply used orig_le there as well.

Similar changes done for big endian.

So, the struct looks like this now:

static noinline_for_stack
char *fourcc_string(char *buf, char *end, const u32 *fourcc,
		    struct printf_spec spec, const char *fmt)
{
	char output[sizeof("0123 little-endian (0x01234567)")];
	char *p = output;
	unsigned int i;
	unsigned char c;
	bool pixel_fmt = false;
	u32 orig, val;
	__le32 orig_le;
	__be32 orig_be;

	if (fmt[1] != 'c')
		return error_string(buf, end, "(%p4?)", spec);

	if (check_pointer(&buf, end, fourcc, spec))
		return buf;

	orig = get_unaligned(fourcc);
	switch (fmt[2]) {
	case 'h':
		val = orig;
		break;
	case 'r':
		orig = swab32(orig);
		val = orig;
		break;
	case 'l':
		orig_le = cpu_to_le32(orig);
		break;
	case 'b':
		orig_be = cpu_to_be32(orig);
		break;
	case 'c':
		/* Pixel formats are printed LSB-first */
		val = swab32(orig & ~BIT(31));
		pixel_fmt = true;
		break;
	default:
		return error_string(buf, end, "(%p4?)", spec);
	}

	for (i = 0; i < sizeof(u32); i++) {
		switch (fmt[2]) {
		case 'h':
		case 'r':
		case 'c':
			c = val >> ((3 - i) * 8);
			break;
		case 'l':
			c = le32_to_cpu(orig_le) >> ((3 - i) * 8);
			break;
		case 'b':
			c = be32_to_cpu(orig_be) >> ((3 - i) * 8);
			break;
		}

		/* Print non-control ASCII characters as-is, dot otherwise */
		*p++ = isascii(c) && isprint(c) ? c : '.';
	}

	if (pixel_fmt) {
		*p++ = ' ';
		strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
		p += strlen(p);
	}

	*p++ = ' ';
	*p++ = '(';

	switch (fmt[2]) {
	case 'h':
	case 'r':
	case 'c':
		p = special_hex_number(p, output + sizeof(output) - 2, orig, sizeof(u32));
		break;
	case 'l':
		p = special_hex_number(p, output + sizeof(output) - 2, le32_to_cpu(orig_le), sizeof(u32));
		break;
	case 'b':
		p = special_hex_number(p, output + sizeof(output) - 2, be32_to_cpu(orig_be), sizeof(u32));
		break;
	}

	*p++ = ')';
	*p = '\0';

	return string(buf, end, output, spec);
}

Andy, could you verify this?