Re: [PATCH v2 04/15] ACPI: table: replace __attribute__((packed)) by __packed

["Rafael J. Wysocki" <rafael@kernel.org>]

On Thu, Apr 1, 2021 at 4:23 PM David Laight <David.Laight@aculab.com> wrote:
>
> From: Rafael J. Wysocki
> > Sent: 01 April 2021 14:50
> ...
> > So what exactly is wrong with using "packed"?  It is way easier to
> > understand for a casual reader of the code.
>
> Because it is usually wrong!
>
> If I have:
>         struct foo {
>                 u64 val;
>         } __packed;
>
> And then have:
> u64 bar(struct foo *foo)
> {
>         return foo->val;
> }
>
> The on some cpu the compiler has to generate the equivalent of:
>         u8 *x = (void *)&foo->val;
>         return x[0] | x[1] << 8 | x[2] << 16 | x[3] << 24 | x[4] << 32 | x[5] << 40 | x[6] << 48 | x[7] << 56;
>
> If you can guarantee that the structure is 32bit aligned
> then it can generate the simpler:
>         u32 *x = (void *)&foo->val;
>         return x[0] | x[1] << 32;
>
> (Yes I've missed out the 64-bit casts)
>
> This is why you should almost never use __packed.
>
> There are historic structures with 64 bit items on 4 byte boundaries
> (and 32 bit values on 2 byte boundaries).
> Typically most of the fields are shorter so can be read directly
> (although they might need a byte-swapping load).

The possible overhead impact is clear to me, but I really don't like
the "local" typedef idea.

It at least would need to be accompanied by a comment explaining why
it is there and why using it is better than using __packed and why
this needs to be defined locally and not in some generic header file.

Also, the FPDT code is just one function that parses the entire table
and there is no object passing between functions in it etc, so is
__packed still problematic in there?