On Mon, 30 May 2022, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Mon, May 30, 2022 at 3:10 PM Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
>> >
>> > 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.
>
> The annotations for edid are completely correct and necessary. However
> other driver authors just slap __packed annotations on any structure
> even if the layout is not fixed at all like:
Right. Thanks for the examples.
> struct my_driver_priv {
> struct device dev;
> u8 causes_misalignment;
> spinlock_t lock;
> atomic_t counter;
> } __packed; /* this annotation is harmful because it breaks the atomics */
I wonder if this is something that could be caught with coccinelle. Or
sparse. Are there any cases where this combo is necessary? (I can't
think of any, but it's a low bar. ;)
Cc: Julia.
> or if the annotation does not change the layout like
>
> struct my_dma_descriptor {
> __le64 address;
> __le64 length;
> } __packed; /* does not change layout but makes access slow on some
> architectures */
Why is this the case, though? I'd imagine the compiler could figure this
out.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
