Re: mainline build failure due to f1e4c916f97f ("drm/edid: add EDID block count and size helpers")

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 30 May 2022, Arnd Bergmann <arnd@arndb.de> wrote:
> On Mon, May 30, 2022 at 3:10 PM Jani Nikula <jani.nikula@intel.com> 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