On Sun, 19 Jun 2011, Nicolas Pitre wrote: > On Thu, 16 Jun 2011, Arnd Bergmann wrote: > > > On Thursday 16 June 2011 22:10:53 Alexander Holler wrote: > > > Using packed doesn't seem to be necessary (at least not with those > > > versions of gcc I'm using here), I've tried both versions (on arm, > > > without packed and with packed, aligned(4)) and both are working. I've > > > only posted the patch because I found the usage of packed, aligned(4) > > > much clearer than without packed. And It might help avoiding such > > > discussions like this with people like me who aren't that deep involved > > > in gcc-specific implementation details. ;) > > > > > > Anyway, feel free to nack that patch. I don't really care and just > > > thought I should post it (e.g. as an alternative to removing that packed). > > > > At least I would be happier without the patch. I'm trying to convince > > people to not use these attributes unless required because too much > > harm is done when they are used without understanding the full > > consequences. I also recommend using __packed as localized as possible, > > i.e. set it for the members that need it, not the entire struct. > > > > I agree that your patch is harmless, it's just the opposite of > > a cleanup in my opinion. > > The question is: does the structure really has to be packed? What do you mean? The structure really does need to be allocated without padding between the fields; is that the same thing? So do a bunch of other structures that currently have no annotations at all. > If it does, then the follow-up question is: is a packing on word > boundaries sufficient? Again, what do you mean? The structure contains some 32-bit fields and it must always be allocated at a 4-byte boundary. However there's nothing wrong with stricter allocation -- I don't recall the details but it's entirely possible that some of the fields could be 64 bits on some architectures, in which cases the alignment certainly should be 8-byte. > If the answer is yes in both cases, then having packed,aligned(4) is not > a frivolity but rather a correctness issue. Why so? Current systems work just fine without it. > We can of course provide a > define in include/linux/compiler-gcc.hto hide the ugliness of it > somewhat: > > #define __packed_32 __attribute__((packed,aligned(4))) > > I suspect that the vast majority of the __packed uses in the kernel > would be better with this __packed_32 instead, the actual need and > intent would be more clearly expressed, and the generated code in the > presence of those GCC changes would then be way more efficient and still > correct. What if the intent is that the structure should be 4-byte aligned on 32-bit systems and 8-byte aligned on 64-bit systems? The compiler already does this sort of thing automatically, why mess with it? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
