On Mon, 30 May 2022, Arnd Bergmann <arnd@xxxxxxxx> wrote: > On Mon, May 30, 2022 at 11:33 AM Jani Nikula <jani.nikula@xxxxxxxxx> wrote: >> >> That is, for EDID. Makes you wonder about all the other packed structs >> with enum members across the kernel. > > It is not the 'enum' that is special here, it's the 'union' having > unpacked members, Obviously meant union not enum, that was just a -ENOCOFFEE on my part. > and the same thing happens when you have nested structs: both the inner > and the outer aggregate need to be packed, either with __packed at the > end, or on each individual member that is not fully aligned to > max(sizeof(member), 4)). > > I think in general, most __packed annotations we have in the kernel are > completely pointless because they do not change the structure layout on > any architecture but instead just make member access slower on Please explain. They are used quite a bit for parsing blob data, or serialization/deserialization, like in the EDID case at hand. Try removing __attribute__((packed)) from include/drm/drm_edid.h and see the sizeof(struct edid) on any architecture. BR, Jani. > architectures that lack unaligned load/store instructions. There have > definitely been other cases though where a __packed annotation is > not needed on any sane architecture but is needed for OABI ARM. > > Overall I'm not that worried because the only machines running OABI > kernels would be on really old hardware that runs a limited set of > driver code. > > A completely different matter are the extraneous __packed annotations > that lead to possible problems when accessed through a misaligned > pointer. We ignore -Waddress-of-packed-member and -Wcast-align > in the kernel, so these never get caught at build time, but we have > seen bugs from gcc making incorrect assumptions about alignment > even on architectures that have unaligned load/store instructions. > > Arnd -- Jani Nikula, Intel Open Source Graphics Center
