On Sun, Feb 23, 2025 at 06:39:15AM +0000, Aditya Garg wrote:
> > On 22 Feb 2025, at 5:41 PM, Aditya Garg <gargaditya08@xxxxxxxx> wrote:
> >> On 21 Feb 2025, at 8:57 PM, andriy.shevchenko@xxxxxxxxxxxxxxx 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
