On Mon, May 30, 2022 at 4:08 PM Jani Nikula <jani.nikula@xxxxxxxxx> wrote: > On Mon, 30 May 2022, Arnd Bergmann <arnd@xxxxxxxx> wrote: > > 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. I think one would first have to make a list of data types that are not meant to be in a packed structure. It could be a good start to search for any packed aggregates with a pointer, atomic_t or spinlock_t in them, but there are of course many more types that you won't find in hardware structures. > > 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. When you annotate the entire structure as __packed without an extra __aligned() annotation, the compiler has to assume that the structure itself is unaligned as well. On many of the older architectures, this will result in accessing the values one byte at a time. Marking the structure as "__packed __aligned(8)" instead would be harmless. When I have a structure with a few misaligned members, I generally prefer to only annotate the members that are not naturally aligned, but this approach is not very common. Arnd