Hi David, Just for reference here, I've posted a v2 of this patch now titled: [PATCH v2 02/11] media: vsp1: Remove packed attributes from aligned structures which removes the attributes instead of modifying them. Thanks for the pointers! Regards Kieran On 13/03/18 15:03, Kieran Bingham wrote: > Hi David, > > On 13/03/18 13:38, David Laight wrote: >> From: Kieran Bingham [mailto:kieran.bingham+renesas@xxxxxxxxxxxxxxxx] >>> On 13/03/18 11:20, David Laight wrote: >>>> From: Kieran Bingham >>>>> Sent: 09 March 2018 22:04 >>>>> The kernel provides a __packed definition to abstract away from the >>>>> compiler specific attributes tag. >>>>> >>>>> Convert all packed structures in VSP1 to use it. >>>>> >>>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx> >>>>> --- >>>>> drivers/media/platform/vsp1/vsp1_dl.c | 6 +++--- >>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c >>>>> index 37e2c984fbf3..382e45c2054e 100644 >>>>> --- a/drivers/media/platform/vsp1/vsp1_dl.c >>>>> +++ b/drivers/media/platform/vsp1/vsp1_dl.c >>>>> @@ -29,19 +29,19 @@ >>>>> struct vsp1_dl_header_list { >>>>> u32 num_bytes; >>>>> u32 addr; >>>>> -} __attribute__((__packed__)); >>>>> +} __packed; >>>>> >>>>> struct vsp1_dl_header { >>>>> u32 num_lists; >>>>> struct vsp1_dl_header_list lists[8]; >>>>> u32 next_header; >>>>> u32 flags; >>>>> -} __attribute__((__packed__)); >>>>> +} __packed; >>>>> >>>>> struct vsp1_dl_entry { >>>>> u32 addr; >>>>> u32 data; >>>>> -} __attribute__((__packed__)); >>>>> +} __packed; >>>> >>>> Do these structures ever actually appear in misaligned memory? >>>> If they don't then they shouldn't be marked 'packed'. >>> >>> I believe the declaration is to ensure that the struct definition is not altered >>> by the compiler as these structures specifically define the layout of how the >>> memory is read by the VSP1 hardware. >> >> The C language and ABI define structure layouts. >> >>> Certainly 2 u32's sequentially stored in a struct are unlikely to be moved or >>> rearranged by the compiler (though I believe it would be free to do so if it >>> chose without this attribute), but I think the declaration shows the intent of >>> the memory structure. >> >> The language requires the fields be in order and the ABI stops the compiler >> adding 'random' padding. >> >>> Isn't this a common approach throughout the kernel when dealing with hardware >>> defined memory structures ? >> >> Absolutely not. >> __packed tells the compiler that the structure might be on any address boundary. >> On many architectures this means the compiler must use byte accesses with shifts >> and ors for every access. >> The most a hardware defined structure will have is a compile-time assert >> that it is the correct size (to avoid silly errors from changes). > > > > Ok - interesting, I see what you're saying - and certainly don't want the > compiler to be performing byte accesses on the structures unnecessarily. > > I'm trying to distinguish the difference here. Is the single point that > __packed > > causes byte-access, where as > > __attribute__((__packed__)); > > does not? > > Looking at the GCC docs [0]: I see that __attribute__((__packed__)) tells the > compiler that the "structure or union is placed to minimize the memory required". > > However, the keil compiler docs[1] do certainly declare that __packed causes > byte alignment. > > I was confused/thrown off here by the Kernel defining __packed as > __attribute__((packed)) at [2]. > > Do __attribute__((packed)) and __attribute__((__packed__)) differ ? > > In which case, from what I've read so far I wish "__packed" was "__unaligned"... > > > [0] > https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#index-packed-type-attribute > > [1] http://www.keil.com/support/man/docs/armcc/armcc_chr1359124230195.htm > > [2] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/compiler-gcc.h?h=v4.16-rc5#n92 > > > Regards > > Kieran > > >> David >>
