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

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

[Aditya Garg]

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).

Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
v2 -> Add this patch
 Documentation/core-api/printk-formats.rst | 32 +++++++++++++++++++
 lib/test_printf.c                         | 39 +++++++++++++++++++----
 lib/vsprintf.c                            | 38 ++++++++++++++++++----
 scripts/checkpatch.pl                     |  2 +-
 4 files changed, 97 insertions(+), 14 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index ecccc0473..9982861fa 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -648,6 +648,38 @@ Examples::
 	%p4cc	Y10  little-endian (0x20303159)
 	%p4cc	NV12 big-endian (0xb231564e)
 
+Generic FourCC code
+-------------------
+
+::
+	%p4c[hrbl]	gP00 (0x67503030)
+
+Print a generic FourCC code, as both ASCII characters and its numerical
+value as hexadecimal.
+
+The additional ``h``, ``r``, ``b``, and ``l`` specifiers are used to specify
+host, reversed, big or little endian order data respectively. Host endian
+order means the data is interpreted as a 32-bit integer and the most
+significant byte is printed first; that is, the character code as printed
+matches the byte order stored in memory on big-endian systems, and is reversed
+on little-endian systems.
+
+Passed by reference.
+
+Examples for a little-endian machine, given &(u32)0x67503030::
+
+	%p4ch	gP00 (0x67503030)
+	%p4cr	00Pg (0x30305067)
+	%p4cb	00Pg (0x30305067)
+	%p4cl	gP00 (0x67503030)
+
+Examples for a big-endian machine, given &(u32)0x67503030::
+
+	%p4ch	gP00 (0x67503030)
+	%p4cr	00Pg (0x30305067)
+	%p4cb	gP00 (0x67503030)
+	%p4cl	00Pg (0x30305067)
+
 Rust
 ----
 
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 59dbe4f9a..ee860327e 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -776,21 +776,46 @@ static void __init fwnode_pointer(void)
 	software_node_unregister_node_group(group);
 }
 
+struct fourcc_struct {
+	u32 code;
+	const char *str;
+};
+
+static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n,
+				       const char *fmt)
+{
+	size_t i;
+
+	for (i = 0; i < n; i++)
+		test(fc[i].str, fmt, &fc[i].code);
+}
+
 static void __init fourcc_pointer(void)
 {
-	struct {
-		u32 code;
-		char *str;
-	} const try[] = {
+	struct fourcc_struct const try_cc[] = {
 		{ 0x3231564e, "NV12 little-endian (0x3231564e)", },
 		{ 0xb231564e, "NV12 big-endian (0xb231564e)", },
 		{ 0x10111213, ".... little-endian (0x10111213)", },
 		{ 0x20303159, "Y10  little-endian (0x20303159)", },
 	};
-	unsigned int i;
+	struct fourcc_struct const try_ch = {
+		0x41424344, "ABCD (0x41424344)",
+	};
+	struct fourcc_struct const try_cr = {
+		0x41424344, "DCBA (0x44434241)",
+	};
+	struct fourcc_struct const try_cl = {
+		le32_to_cpu(0x41424344), "ABCD (0x41424344)",
+	};
+	struct fourcc_struct const try_cb = {
+		be32_to_cpu(0x41424344), "ABCD (0x41424344)",
+	};
 
-	for (i = 0; i < ARRAY_SIZE(try); i++)
-		test(try[i].str, "%p4cc", &try[i].code);
+	fourcc_pointer_test(try_cc, ARRAY_SIZE(try_cc), "%p4cc");
+	fourcc_pointer_test(&try_ch, 1, "%p4ch");
+	fourcc_pointer_test(&try_cr, 1, "%p4cr");
+	fourcc_pointer_test(&try_cl, 1, "%p4cl");
+	fourcc_pointer_test(&try_cb, 1, "%p4cb");
 }
 
 static void __init
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 56fe96319..13733a4da 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1781,27 +1781,53 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
 	char output[sizeof("0123 little-endian (0x01234567)")];
 	char *p = output;
 	unsigned int i;
+	bool pixel_fmt = false;
 	u32 orig, val;
 
-	if (fmt[1] != 'c' || fmt[2] != 'c')
+	if (fmt[1] != 'c')
 		return error_string(buf, end, "(%p4?)", spec);
 
 	if (check_pointer(&buf, end, fourcc, spec))
 		return buf;
 
 	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);
+		val = 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++) {
-		unsigned char c = val >> (i * 8);
+		unsigned char c = val >> ((3 - i) * 8);
 
 		/* Print non-control ASCII characters as-is, dot otherwise */
 		*p++ = isascii(c) && isprint(c) ? c : '.';
 	}
 
-	*p++ = ' ';
-	strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
-	p += strlen(p);
+	if (pixel_fmt) {
+		*p++ = ' ';
+		strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
+		p += strlen(p);
+	}
 
 	*p++ = ' ';
 	*p++ = '(';
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7b28ad331..21516f753 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6904,7 +6904,7 @@ sub process {
 					    ($extension eq "f" &&
 					     defined $qualifier && $qualifier !~ /^w/) ||
 					    ($extension eq "4" &&
-					     defined $qualifier && $qualifier !~ /^cc/)) {
+					     defined $qualifier && $qualifier !~ /^c[chlbr]/)) {
 						$bad_specifier = $specifier;
 						last;
 					}
-- 
2.43.0

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

[Rasmus Villemoes]

On Thu, Feb 20 2025, Aditya Garg <gargaditya08@live.com> wrote:

> v2 -> Add this patch
>  Documentation/core-api/printk-formats.rst | 32 +++++++++++++++++++
>  lib/test_printf.c                         | 39 +++++++++++++++++++----
>  lib/vsprintf.c                            | 38 ++++++++++++++++++----

Yay! Thanks for remembering to include test cases.

>  
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 59dbe4f9a..ee860327e 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -776,21 +776,46 @@ static void __init fwnode_pointer(void)
>  	software_node_unregister_node_group(group);
>  }
>  
> +struct fourcc_struct {
> +	u32 code;
> +	const char *str;
> +};
> +
> +static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n,
> +				       const char *fmt)
> +{
> +	size_t i;
> +
> +	for (i = 0; i < n; i++)
> +		test(fc[i].str, fmt, &fc[i].code);
> +}
> +
>  static void __init fourcc_pointer(void)
>  {
> -	struct {
> -		u32 code;
> -		char *str;
> -	} const try[] = {
> +	struct fourcc_struct const try_cc[] = {

I know it matches the code it replaces, but kernel style seems to be
"const struct foo" rather than "struct foo const" (at around 130:1) -
just as you use in the new helper function.

Also, please consider changing the array, and the newly added instances,
to be static instead of automatic (our le32_to_cpu should be usable also
for static initializers).

This will conflict with the conversion-to-kunit which is in flight, but
the conflict should be trivial to resolve.

Rasmus

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

[Aditya Garg]

Hi

> 
>> 
>> diff --git a/lib/test_printf.c b/lib/test_printf.c
>> index 59dbe4f9a..ee860327e 100644
>> --- a/lib/test_printf.c
>> +++ b/lib/test_printf.c
>> @@ -776,21 +776,46 @@ static void __init fwnode_pointer(void)
>> software_node_unregister_node_group(group);
>> }
>> 
>> +struct fourcc_struct {
>> + u32 code;
>> + const char *str;
>> +};
>> +
>> +static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n,
>> +       const char *fmt)
>> +{
>> + size_t i;
>> +
>> + for (i = 0; i < n; i++)
>> + test(fc[i].str, fmt, &fc[i].code);
>> +}
>> +
>> static void __init fourcc_pointer(void)
>> {
>> - struct {
>> - u32 code;
>> - char *str;
>> - } const try[] = {
>> + struct fourcc_struct const try_cc[] = {
> 
> I know it matches the code it replaces, but kernel style seems to be
> "const struct foo" rather than "struct foo const" (at around 130:1) -
> just as you use in the new helper function.
> 
> Also, please consider changing the array, and the newly added instances,
> to be static instead of automatic (our le32_to_cpu should be usable also
> for static initializers).
> 

V3 sent here:
https://lore.kernel.org/dri-devel/98289BC4-D5E1-41B8-AC89-632DBD2C2789@live.com/T/#mfa1dac647c9517674649a50301b122a524cc364c

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

[andriy.shevchenko]

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.

> +		val = 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);
> +	}

...

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

> +	if (pixel_fmt) {

Technically we can avoid a boolean by checking fmt[2] again here, but I'm okay
with a temporary holder.

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

-- 
With Best Regards,
Andy Shevchenko

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

[Aditya Garg]

> 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.

Does this look good now? Made orig a union.

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

union {
u32 raw;
__le32 le;
__be32 be;
} orig;

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

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

orig.raw = get_unaligned(fourcc);

switch (fmt[2]) {
case 'h':
val = orig.raw;
break;
case 'r':
val = swab32(orig.raw);
break;
case 'l':
val = le32_to_cpu(orig.le);
break;
case 'b':
val = be32_to_cpu(orig.be);
break;
case 'c':
val = swab32(orig.raw & ~BIT(31));
pixel_fmt = true;
break;
default:
return error_string(buf, end, "(%p4?)", spec);
}

for (i = 0; i < sizeof(u32); i++) {
unsigned char c = val >> ((3 - i) * 8);
*p++ = isascii(c) && isprint(c) ? c : '.';
}

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

*p++ = ' ';
*p++ = '(';
p += sprintf(p, "0x%08x", orig.raw);
*p++ = ')';
*p = '\0';

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

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

[Aditya Garg]

> Does this look good now? Made orig a union.

Wait, it's messier. Maybe declare data type of val separately in each case?
> 
> char *fourcc_string(char *buf, char *end, const u32 *fourcc, const char *fmt, struct printf_spec spec)
> {
> char output[sizeof("0123 little-endian (0x01234567)")];
> char *p = output;
> unsigned int i;
> bool pixel_fmt = false;
> u32 val;
> 
> union {
> u32 raw;
> __le32 le;
> __be32 be;
> } orig;
> 
> if (fmt[1] != 'c')
> return error_string(buf, end, "(%p4?)", spec);
> 
> if (check_pointer(&buf, end, fourcc, spec))
> return buf;
> 
> orig.raw = get_unaligned(fourcc);
> 
> switch (fmt[2]) {
> case 'h':
> val = orig.raw;
> break;
> case 'r':
> val = swab32(orig.raw);
> break;
> case 'l':
> val = le32_to_cpu(orig.le);
> break;
> case 'b':
> val = be32_to_cpu(orig.be);
> break;
> case 'c':
> val = swab32(orig.raw & ~BIT(31));
> pixel_fmt = true;
> break;
> default:
> return error_string(buf, end, "(%p4?)", spec);
> }
> 
> for (i = 0; i < sizeof(u32); i++) {
> unsigned char c = val >> ((3 - i) * 8);
> *p++ = isascii(c) && isprint(c) ? c : '.';
> }
> 
> if (pixel_fmt) {
> *p++ = ' ';
> strcpy(p, orig.raw & BIT(31) ? "big-endian" : "little-endian");
> p += strlen(p);
> }
> 
> *p++ = ' ';
> *p++ = '(';
> p += sprintf(p, "0x%08x", orig.raw);
> *p++ = ')';
> *p = '\0';
> 
> return string_nocheck(buf, end, output, spec);
> }
> 

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

[andriy.shevchenko]

On Fri, Feb 21, 2025 at 08:06:51PM +0000, Aditya Garg wrote:
> 
> > Does this look good now? Made orig a union.
> 
> Wait, it's messier. Maybe declare data type of val separately in each case?

Yes, this sounds better.

-- 
With Best Regards,
Andy Shevchenko

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

[Aditya Garg]

> 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?

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

[Aditya Garg]

> On 20 Feb 2025, at 10:09 PM, Aditya Garg <gargaditya08@live.com> 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).
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Aditya Garg <gargaditya08@live.com>

BTW, after looking at the comments by Martin [1], its actually better to use existing specifiers for the appletbdrm driver.
The driver needs the host endian as proposed by this patch, so instead of that, we can use %.4s

[1]: https://lore.kernel.org/asahi/E753B391-D2CB-4213-AF82-678ADD5A7644@cutebit.org/

Alternatively we could add a host endian only. Other endians are not really used by any driver AFAIK. The host endian is being used by appletbdrm and Asahi Linux’ SMC driver only.
> 
> 
> ---
> - *p++ = ' ';
> - strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
> - p += strlen(p);
> + if (pixel_fmt) {
> + *p++ = ' ';
> + strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
> + p += strlen(p);
> + }

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

[Aditya Garg]

> On 22 Feb 2025, at 5:41 PM, Aditya Garg <gargaditya08@live.com> wrote:
> 
> 
> 
>> 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?

Looking at the header files, it looks like doing cpu_to_le32 on that variable and doing le32_to_cpu will actually reverse the order twice, on big endian systems, thus technically all way would not swap the order at all.

I'm not really sure how to manage the sparse warnings here.

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

[Aditya Garg]

> 
> Looking at the header files, it looks like doing cpu_to_le32 on that variable and doing le32_to_cpu will actually reverse the order twice, on big endian systems, thus technically all way would not swap the order at all.
> 
> I'm not really sure how to manage the sparse warnings here.



Not sure whether the maintainers would like it, but we can do something like this:

	case 'l’:
#ifdef __LITTLE_ENDIAN
		val = orig;
#else
		orig = swab32(orig);
		val = orig;
#endif
		break;

	case 'b’:
#ifdef __LITTLE_ENDIAN
		orig = swab32(orig);
		val = orig;
#else
		val = orig;
#endif
		break;

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

[andriy.shevchenko]

On Sat, Feb 22, 2025 at 03:46:03PM +0000, Aditya Garg wrote:
> > On 20 Feb 2025, at 10:09 PM, Aditya Garg <gargaditya08@live.com> wrote:
> > 
> > %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).

...

> BTW, after looking at the comments by Martin [1], its actually better to use
> existing specifiers for the appletbdrm driver.  The driver needs the host
> endian as proposed by this patch, so instead of that, we can use %.4s

Do you mean this patch will not be needed? If this a case, that would be the
best solution.

> [1]: https://lore.kernel.org/asahi/E753B391-D2CB-4213-AF82-678ADD5A7644@cutebit.org/
> 
> Alternatively we could add a host endian only. Other endians are not really
> used by any driver AFAIK. The host endian is being used by appletbdrm and
> Asahi Linux’ SMC driver only.

-- 
With Best Regards,
Andy Shevchenko

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

[Aditya Garg]

> On 24 Feb 2025, at 3:28 PM, andriy.shevchenko@linux.intel.com wrote:
> 
> On Sat, Feb 22, 2025 at 03:46:03PM +0000, Aditya Garg wrote:
>>>> On 20 Feb 2025, at 10:09 PM, Aditya Garg <gargaditya08@live.com> wrote:
>>> 
>>> %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).
> 
> ...
> 
>> BTW, after looking at the comments by Martin [1], its actually better to use
>> existing specifiers for the appletbdrm driver.  The driver needs the host
>> endian as proposed by this patch, so instead of that, we can use %.4s
> 
> Do you mean this patch will not be needed? If this a case, that would be the
> best solution.

I tested with %4pE, and the results are different from expected. So this would be preferred. Kindly see my latest email with a proposed workaround for the sparse warnings.
> 
>> [1]: https://lore.kernel.org/asahi/E753B391-D2CB-4213-AF82-678ADD5A7644@cutebit.org/
>> 
>> Alternatively we could add a host endian only. Other endians are not really
>> used by any driver AFAIK. The host endian is being used by appletbdrm and
>> Asahi Linux’ SMC driver only.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

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

[andriy.shevchenko]

On Mon, Feb 24, 2025 at 10:18:48AM +0000, Aditya Garg wrote:
> 
> 
> > On 24 Feb 2025, at 3:28 PM, andriy.shevchenko@linux.intel.com wrote:
> > 
> > On Sat, Feb 22, 2025 at 03:46:03PM +0000, Aditya Garg wrote:
> >>>> On 20 Feb 2025, at 10:09 PM, Aditya Garg <gargaditya08@live.com> wrote:
> >>> 
> >>> %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).
> > 
> > ...
> > 
> >> BTW, after looking at the comments by Martin [1], its actually better to use
> >> existing specifiers for the appletbdrm driver.  The driver needs the host
> >> endian as proposed by this patch, so instead of that, we can use %.4s
> > 
> > Do you mean this patch will not be needed? If this a case, that would be the
> > best solution.
> 
> I tested with %4pE, and the results are different from expected. So this
> would be preferred. Kindly see my latest email with a proposed workaround for
> the sparse warnings.

%.4s sounded okay, but %4pE is always about escaping and the result may occupy
%4x memory (octal escaping of non-printable characters). Of course, you may vary
the escaping classes, but IIRC the octal or hex escaping is unconditional.

> >> [1]: https://lore.kernel.org/asahi/E753B391-D2CB-4213-AF82-678ADD5A7644@cutebit.org/
> >> 
> >> Alternatively we could add a host endian only. Other endians are not really
> >> used by any driver AFAIK. The host endian is being used by appletbdrm and
> >> Asahi Linux’ SMC driver only.

-- 
With Best Regards,
Andy Shevchenko

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

[Aditya Garg]

> On 24 Feb 2025, at 3:54 PM, andriy.shevchenko@linux.intel.com wrote:
> 
> On Mon, Feb 24, 2025 at 10:18:48AM +0000, Aditya Garg wrote:
>> 
>> 
>>>> On 24 Feb 2025, at 3:28 PM, andriy.shevchenko@linux.intel.com wrote:
>>> 
>>> On Sat, Feb 22, 2025 at 03:46:03PM +0000, Aditya Garg wrote:
>>>>>> On 20 Feb 2025, at 10:09 PM, Aditya Garg <gargaditya08@live.com> wrote:
>>>>> 
>>>>> %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).
>>> 
>>> ...
>>> 
>>>> BTW, after looking at the comments by Martin [1], its actually better to use
>>>> existing specifiers for the appletbdrm driver.  The driver needs the host
>>>> endian as proposed by this patch, so instead of that, we can use %.4s
>>> 
>>> Do you mean this patch will not be needed? If this a case, that would be the
>>> best solution.
>> 
>> I tested with %4pE, and the results are different from expected. So this
>> would be preferred. Kindly see my latest email with a proposed workaround for
>> the sparse warnings.
> 
> %.4s sounded okay, but %4pE is always about escaping and the result may occupy
> %4x memory (octal escaping of non-printable characters). Of course, you may vary
> the escaping classes, but IIRC the octal or hex escaping is unconditional.

%.4s is used for unsigned int iirc, here it's __le32.
> 
>>>> [1]: https://lore.kernel.org/asahi/E753B391-D2CB-4213-AF82-678ADD5A7644@cutebit.org/
>>>> 
>>>> Alternatively we could add a host endian only. Other endians are not really
>>>> used by any driver AFAIK. The host endian is being used by appletbdrm and
>>>> Asahi Linux’ SMC driver only.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

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

[andriy.shevchenko]

On Mon, Feb 24, 2025 at 10:32:27AM +0000, Aditya Garg wrote:
> > On 24 Feb 2025, at 3:54 PM, andriy.shevchenko@linux.intel.com wrote:
> > On Mon, Feb 24, 2025 at 10:18:48AM +0000, Aditya Garg wrote:
> >>>> On 24 Feb 2025, at 3:28 PM, andriy.shevchenko@linux.intel.com wrote:
> >>> On Sat, Feb 22, 2025 at 03:46:03PM +0000, Aditya Garg wrote:
> >>>>>> On 20 Feb 2025, at 10:09 PM, Aditya Garg <gargaditya08@live.com> wrote:

...

> >>>>> %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).
> >>> 
> >>> ...
> >>> 
> >>>> BTW, after looking at the comments by Martin [1], its actually better to use
> >>>> existing specifiers for the appletbdrm driver.  The driver needs the host
> >>>> endian as proposed by this patch, so instead of that, we can use %.4s
> >>> 
> >>> Do you mean this patch will not be needed? If this a case, that would be the
> >>> best solution.
> >> 
> >> I tested with %4pE, and the results are different from expected. So this
> >> would be preferred. Kindly see my latest email with a proposed workaround for
> >> the sparse warnings.
> > 
> > %.4s sounded okay, but %4pE is always about escaping and the result may occupy
> > %4x memory (octal escaping of non-printable characters). Of course, you may vary
> > the escaping classes, but IIRC the octal or hex escaping is unconditional.
> 
> %.4s is used for unsigned int iirc, here it's __le32.

No, it's used to 'char *'. in case one may guarantee that it at least is
four characters long.

> >>>> [1]: https://lore.kernel.org/asahi/E753B391-D2CB-4213-AF82-678ADD5A7644@cutebit.org/
> >>>> 
> >>>> Alternatively we could add a host endian only. Other endians are not really
> >>>> used by any driver AFAIK. The host endian is being used by appletbdrm and
> >>>> Asahi Linux’ SMC driver only.

-- 
With Best Regards,
Andy Shevchenko

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

[Aditya Garg]

> On 24 Feb 2025, at 4:11 PM, andriy.shevchenko@linux.intel.com wrote:
> 
> On Mon, Feb 24, 2025 at 10:32:27AM +0000, Aditya Garg wrote:
>>>> On 24 Feb 2025, at 3:54 PM, andriy.shevchenko@linux.intel.com wrote:
>>> On Mon, Feb 24, 2025 at 10:18:48AM +0000, Aditya Garg wrote:
>>>>>> On 24 Feb 2025, at 3:28 PM, andriy.shevchenko@linux.intel.com wrote:
>>>>> On Sat, Feb 22, 2025 at 03:46:03PM +0000, Aditya Garg wrote:
>>>>>>>> On 20 Feb 2025, at 10:09 PM, Aditya Garg <gargaditya08@live.com> wrote:
> 
> ...
> 
>>>>>>> %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).
>>>>> 
>>>>> ...
>>>>> 
>>>>>> BTW, after looking at the comments by Martin [1], its actually better to use
>>>>>> existing specifiers for the appletbdrm driver.  The driver needs the host
>>>>>> endian as proposed by this patch, so instead of that, we can use %.4s
>>>>> 
>>>>> Do you mean this patch will not be needed? If this a case, that would be the
>>>>> best solution.
>>>> 
>>>> I tested with %4pE, and the results are different from expected. So this
>>>> would be preferred. Kindly see my latest email with a proposed workaround for
>>>> the sparse warnings.
>>> 
>>> %.4s sounded okay, but %4pE is always about escaping and the result may occupy
>>> %4x memory (octal escaping of non-printable characters). Of course, you may vary
>>> the escaping classes, but IIRC the octal or hex escaping is unconditional.
>> 
>> %.4s is used for unsigned int iirc, here it's __le32.
> 
> No, it's used to 'char *'. in case one may guarantee that it at least is
> four characters long.

Still the argument here is __le32. %p4h is showing reverse values than what %4pE as well as %.4s shows.
> 
>>>>>> [1]: https://lore.kernel.org/asahi/E753B391-D2CB-4213-AF82-678ADD5A7644@cutebit.org/
>>>>>> 
>>>>>> Alternatively we could add a host endian only. Other endians are not really
>>>>>> used by any driver AFAIK. The host endian is being used by appletbdrm and
>>>>>> Asahi Linux’ SMC driver only.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

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

[andriy.shevchenko]

On Mon, Feb 24, 2025 at 10:43:54AM +0000, Aditya Garg wrote:
> > On 24 Feb 2025, at 4:11 PM, andriy.shevchenko@linux.intel.com wrote:
> > On Mon, Feb 24, 2025 at 10:32:27AM +0000, Aditya Garg wrote:
> >>>> On 24 Feb 2025, at 3:54 PM, andriy.shevchenko@linux.intel.com wrote:
> >>> On Mon, Feb 24, 2025 at 10:18:48AM +0000, Aditya Garg wrote:
> >>>>>> On 24 Feb 2025, at 3:28 PM, andriy.shevchenko@linux.intel.com wrote:
> >>>>> On Sat, Feb 22, 2025 at 03:46:03PM +0000, Aditya Garg wrote:
> >>>>>>>> On 20 Feb 2025, at 10:09 PM, Aditya Garg <gargaditya08@live.com> wrote:

...

> >>>>>>> %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).
> >>>>> 
> >>>>> ...
> >>>>> 
> >>>>>> BTW, after looking at the comments by Martin [1], its actually better to use
> >>>>>> existing specifiers for the appletbdrm driver.  The driver needs the host
> >>>>>> endian as proposed by this patch, so instead of that, we can use %.4s
> >>>>> 
> >>>>> Do you mean this patch will not be needed? If this a case, that would be the
> >>>>> best solution.
> >>>> 
> >>>> I tested with %4pE, and the results are different from expected. So this
> >>>> would be preferred. Kindly see my latest email with a proposed workaround for
> >>>> the sparse warnings.
> >>> 
> >>> %.4s sounded okay, but %4pE is always about escaping and the result may occupy
> >>> %4x memory (octal escaping of non-printable characters). Of course, you may vary
> >>> the escaping classes, but IIRC the octal or hex escaping is unconditional.
> >> 
> >> %.4s is used for unsigned int iirc, here it's __le32.
> > 
> > No, it's used to 'char *'. in case one may guarantee that it at least is
> > four characters long.

> Still the argument here is __le32. %p4h is showing reverse values than what
> %4pE as well as %.4s shows.

For __le32 the %.4s will print from LSB to MSB and otherwise for __be32.
You can provide any conversion you want to have it stable between
the endianesses. In any case looking at the DRM patch it might be that
the entire driver got the endianess wrong. Have you checked my comment
there?

> >>>>>> [1]: https://lore.kernel.org/asahi/E753B391-D2CB-4213-AF82-678ADD5A7644@cutebit.org/
> >>>>>> 
> >>>>>> Alternatively we could add a host endian only. Other endians are not really
> >>>>>> used by any driver AFAIK. The host endian is being used by appletbdrm and
> >>>>>> Asahi Linux’ SMC driver only.

-- 
With Best Regards,
Andy Shevchenko

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

[Aditya Garg]

> On 24 Feb 2025, at 4:19 PM, andriy.shevchenko@linux.intel.com wrote:
> 
> On Mon, Feb 24, 2025 at 10:43:54AM +0000, Aditya Garg wrote:
>>>> On 24 Feb 2025, at 4:11 PM, andriy.shevchenko@linux.intel.com wrote:
>>> On Mon, Feb 24, 2025 at 10:32:27AM +0000, Aditya Garg wrote:
>>>>>> On 24 Feb 2025, at 3:54 PM, andriy.shevchenko@linux.intel.com wrote:
>>>>> On Mon, Feb 24, 2025 at 10:18:48AM +0000, Aditya Garg wrote:
>>>>>>>> On 24 Feb 2025, at 3:28 PM, andriy.shevchenko@linux.intel.com wrote:
>>>>>>> On Sat, Feb 22, 2025 at 03:46:03PM +0000, Aditya Garg wrote:
>>>>>>>>>> On 20 Feb 2025, at 10:09 PM, Aditya Garg <gargaditya08@live.com> wrote:
> 
> ...
> 
>>>>>>>>> %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).
>>>>>>> 
>>>>>>> ...
>>>>>>> 
>>>>>>>> BTW, after looking at the comments by Martin [1], its actually better to use
>>>>>>>> existing specifiers for the appletbdrm driver.  The driver needs the host
>>>>>>>> endian as proposed by this patch, so instead of that, we can use %.4s
>>>>>>> 
>>>>>>> Do you mean this patch will not be needed? If this a case, that would be the
>>>>>>> best solution.
>>>>>> 
>>>>>> I tested with %4pE, and the results are different from expected. So this
>>>>>> would be preferred. Kindly see my latest email with a proposed workaround for
>>>>>> the sparse warnings.
>>>>> 
>>>>> %.4s sounded okay, but %4pE is always about escaping and the result may occupy
>>>>> %4x memory (octal escaping of non-printable characters). Of course, you may vary
>>>>> the escaping classes, but IIRC the octal or hex escaping is unconditional.
>>>> 
>>>> %.4s is used for unsigned int iirc, here it's __le32.
>>> 
>>> No, it's used to 'char *'. in case one may guarantee that it at least is
>>> four characters long.
> 
>> Still the argument here is __le32. %p4h is showing reverse values than what
>> %4pE as well as %.4s shows.
> 
> For __le32 the %.4s will print from LSB to MSB and otherwise for __be32.
> You can provide any conversion you want to have it stable between
> the endianesses. In any case looking at the DRM patch it might be that
> the entire driver got the endianess wrong. Have you checked my comment
> there?

drm drivers have a %p4cc already upstream. I think we need that then.
> 
>>>>>>>> [1]: https://lore.kernel.org/asahi/E753B391-D2CB-4213-AF82-678ADD5A7644@cutebit.org/
>>>>>>>> 
>>>>>>>> Alternatively we could add a host endian only. Other endians are not really
>>>>>>>> used by any driver AFAIK. The host endian is being used by appletbdrm and
>>>>>>>> Asahi Linux’ SMC driver only.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 
> 

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

[andriy.shevchenko]

On Sun, Feb 23, 2025 at 03:16:28PM +0000, Aditya Garg wrote:

> > Looking at the header files, it looks like doing cpu_to_le32 on that variable and doing le32_to_cpu will actually reverse the order twice, on big endian systems, thus technically all way would not swap the order at all.
> > 
> > I'm not really sure how to manage the sparse warnings here.
> 
> Not sure whether the maintainers would like it, but we can do something like this:

This is not what we want, I believe. And this looks like a reinventing a wheel
of cpu_to_*() and *_to_cpu() or similar macros.

> 	case 'l’:
> #ifdef __LITTLE_ENDIAN
> 		val = orig;
> #else
> 		orig = swab32(orig);
> 		val = orig;
> #endif
> 		break;
> 
> 	case 'b’:
> #ifdef __LITTLE_ENDIAN
> 		orig = swab32(orig);
> 		val = orig;
> #else
> 		val = orig;
> #endif
> 		break;

-- 
With Best Regards,
Andy Shevchenko

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

[andriy.shevchenko]

On Sun, Feb 23, 2025 at 06:39:15AM +0000, Aditya Garg wrote:
> > On 22 Feb 2025, at 5:41 PM, Aditya Garg <gargaditya08@live.com> wrote:
> >> 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:

...

> >>> %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;

This doesn't look right. It's basically two conversions from and to orig,
it's like using orig here, but that's wrong for the respective endianess,
'l'/BE should be LE, same for 'b'/LE which should be BE.

> >       }
> > 
> >       /* 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?
> 
> Looking at the header files, it looks like doing cpu_to_le32 on that variable
> and doing le32_to_cpu will actually reverse the order twice, on big endian
> systems, thus technically all way would not swap the order at all.
> 
> I'm not really sure how to manage the sparse warnings here.

-- 
With Best Regards,
Andy Shevchenko

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

[Aditya Garg]

I request the printk maintainers for their views on whether if they are ok with the sparse errors in this original patch.

> On 20 Feb 2025, at 10:09 PM, Aditya Garg <gargaditya08@live.com> 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).
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Aditya Garg <gargaditya08@live.com>
> ---
> v2 -> Add this patch
> Documentation/core-api/printk-formats.rst | 32 +++++++++++++++++++
> lib/test_printf.c                         | 39 +++++++++++++++++++----
> lib/vsprintf.c                            | 38 ++++++++++++++++++----
> scripts/checkpatch.pl                     |  2 +-
> 4 files changed, 97 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index ecccc0473..9982861fa 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -648,6 +648,38 @@ Examples::
>    %p4cc    Y10  little-endian (0x20303159)
>    %p4cc    NV12 big-endian (0xb231564e)
> 
> +Generic FourCC code
> +-------------------
> +
> +::
> +    %p4c[hrbl]    gP00 (0x67503030)
> +
> +Print a generic FourCC code, as both ASCII characters and its numerical
> +value as hexadecimal.
> +
> +The additional ``h``, ``r``, ``b``, and ``l`` specifiers are used to specify
> +host, reversed, big or little endian order data respectively. Host endian
> +order means the data is interpreted as a 32-bit integer and the most
> +significant byte is printed first; that is, the character code as printed
> +matches the byte order stored in memory on big-endian systems, and is reversed
> +on little-endian systems.
> +
> +Passed by reference.
> +
> +Examples for a little-endian machine, given &(u32)0x67503030::
> +
> +    %p4ch    gP00 (0x67503030)
> +    %p4cr    00Pg (0x30305067)
> +    %p4cb    00Pg (0x30305067)
> +    %p4cl    gP00 (0x67503030)
> +
> +Examples for a big-endian machine, given &(u32)0x67503030::
> +
> +    %p4ch    gP00 (0x67503030)
> +    %p4cr    00Pg (0x30305067)
> +    %p4cb    gP00 (0x67503030)
> +    %p4cl    00Pg (0x30305067)
> +
> Rust
> ----
> 
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 59dbe4f9a..ee860327e 100644
> --- a/lib/test_printf.c
> +++ b/lib/test_printf.c
> @@ -776,21 +776,46 @@ static void __init fwnode_pointer(void)
>    software_node_unregister_node_group(group);
> }
> 
> +struct fourcc_struct {
> +    u32 code;
> +    const char *str;
> +};
> +
> +static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n,
> +                       const char *fmt)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < n; i++)
> +        test(fc[i].str, fmt, &fc[i].code);
> +}
> +
> static void __init fourcc_pointer(void)
> {
> -    struct {
> -        u32 code;
> -        char *str;
> -    } const try[] = {
> +    struct fourcc_struct const try_cc[] = {
>        { 0x3231564e, "NV12 little-endian (0x3231564e)", },
>        { 0xb231564e, "NV12 big-endian (0xb231564e)", },
>        { 0x10111213, ".... little-endian (0x10111213)", },
>        { 0x20303159, "Y10  little-endian (0x20303159)", },
>    };
> -    unsigned int i;
> +    struct fourcc_struct const try_ch = {
> +        0x41424344, "ABCD (0x41424344)",
> +    };
> +    struct fourcc_struct const try_cr = {
> +        0x41424344, "DCBA (0x44434241)",
> +    };
> +    struct fourcc_struct const try_cl = {
> +        le32_to_cpu(0x41424344), "ABCD (0x41424344)",
> +    };
> +    struct fourcc_struct const try_cb = {
> +        be32_to_cpu(0x41424344), "ABCD (0x41424344)",
> +    };
> 
> -    for (i = 0; i < ARRAY_SIZE(try); i++)
> -        test(try[i].str, "%p4cc", &try[i].code);
> +    fourcc_pointer_test(try_cc, ARRAY_SIZE(try_cc), "%p4cc");
> +    fourcc_pointer_test(&try_ch, 1, "%p4ch");
> +    fourcc_pointer_test(&try_cr, 1, "%p4cr");
> +    fourcc_pointer_test(&try_cl, 1, "%p4cl");
> +    fourcc_pointer_test(&try_cb, 1, "%p4cb");
> }
> 
> static void __init
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 56fe96319..13733a4da 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1781,27 +1781,53 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
>    char output[sizeof("0123 little-endian (0x01234567)")];
>    char *p = output;
>    unsigned int i;
> +    bool pixel_fmt = false;
>    u32 orig, val;
> 
> -    if (fmt[1] != 'c' || fmt[2] != 'c')
> +    if (fmt[1] != 'c')
>        return error_string(buf, end, "(%p4?)", spec);
> 
>    if (check_pointer(&buf, end, fourcc, spec))
>        return buf;
> 
>    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);
> +        val = 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++) {
> -        unsigned char c = val >> (i * 8);
> +        unsigned char c = val >> ((3 - i) * 8);
> 
>        /* Print non-control ASCII characters as-is, dot otherwise */
>        *p++ = isascii(c) && isprint(c) ? c : '.';
>    }
> 
> -    *p++ = ' ';
> -    strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
> -    p += strlen(p);
> +    if (pixel_fmt) {
> +        *p++ = ' ';
> +        strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
> +        p += strlen(p);
> +    }
> 
>    *p++ = ' ';
>    *p++ = '(';
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7b28ad331..21516f753 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -6904,7 +6904,7 @@ sub process {
>                        ($extension eq "f" &&
>                         defined $qualifier && $qualifier !~ /^w/) ||
>                        ($extension eq "4" &&
> -                         defined $qualifier && $qualifier !~ /^cc/)) {
> +                         defined $qualifier && $qualifier !~ /^c[chlbr]/)) {
>                        $bad_specifier = $specifier;
>                        last;
>                    }
> --
> 2.43.0
> 

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

[Aditya Garg]

> On 24 Feb 2025, at 9:47 PM, Aditya Garg <gargaditya08@live.com> wrote:
> 
> I request the printk maintainers for their views on whether if they are ok with the sparse errors in this original patch.

FWIW, I read a bit about FOURCC and also investigated the cpu_to_le32 and similar macros. I think the v4 I sent should work well, without the sparse warnings. I've also made the v4 separate from the DRM patch set, so as to avoid multiple tree complications and hindering the DRM driver upstream process. For now %p4cc was the best format helper I could find upstream, but I would prefer using %p4cl (little endian) instead for appletbdrm. And this patch imo is needed simply because we need better format helpers, rather than using workarounds to swap bits and using other format helpers.