Hi Brian, On 3 May 2017 at 15:03, Brian Starkey <brian.starkey@xxxxxxx> wrote: > On Wed, May 03, 2017 at 02:51:18PM +0100, Daniel Stone wrote: >> formats_offset is the end of the fixed-size element, which is >> currently aligned to 32 bytes, and practically speaking would always >> have to be anyway. As it is an array of uint32_t, this gives natural >> alignment. > > Why must it always be? The __packed attribute means it'll have no > padding - so any u16 or u8 added to the end will break it - putting > the formats array on a non-aligned boundary. > > If the assumption is that the struct will always be made of only > u32/u64 members (and the implementation will break otherwise) then > there had better be a really big comment saying so, and preferably a > compile-time assertion too. Indeed that's the case, for most ioctls at least. A comment would definitely be in order. > I'm missing the reason for it being __packed in the first place - > perhaps that's just a left over and needs to be removed. > > Either way, this line aligns to 8: > > + blob_size = ALIGN(sizeof(struct drm_format_modifier_blob), 8); > > ...and the rest of the blob_size calculation looks like it assumes the > formats array starts at that 8-byte boundary. So, for clarity and > consistency I reckon the blob_size code and the code that builds the > blob should do the same thing. All this is correct - the __packed declaration is unnecessary, and so is the rounding up when calculating the size. And with that fixed, I believe it should be correct, no? Cheers, Daniel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
