On Tue, May 31, 2022 at 8:26 AM Julia Lawall Julia.Lawall@inria.fr wrote:
On 30 May 2022, at 15:27, Arnd Bergmann arnd@arndb.de wrote: On Mon, May 30, 2022 at 4:08 PM Jani Nikula jani.nikula@intel.com wrote:
On Mon, 30 May 2022, Arnd Bergmann arnd@arndb.de 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. ;)
...
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.
Searching for specific types in a packed structure would be easy.
As an experiment: what kind of results would we get when looking for packed structures and unions that contain any of these:
- spinlock_t - atomic_t - dma_addr_t - phys_addr_t - size_t - any pointer - any enum - struct mutex - struct device
This is just a list of common data types that are used in a lot of structures but that one should never find in hardware specific types. If the output from coccinelle is 90% actual bugs, this would be really helpful. OTOH if there is no output at all, or all false-positives, we don't need to look for additional types.
Coccinelle could duplicate the structure without the packed and see if any offsets change, using build bug on, but that would be architecture specific so maybe not useful.
I would consider this a separate issue. The first one above is for identifying structures that are marked as packed but should not be packed at all, regardless of whether that changes the layout.
Arnd