[PATCH] USB: ehci: use packed, aligned(4) instead of removing the packed attribute

On Sunday 19 June 2011 21:00:01 Alan Stern wrote:

On Sun, 19 Jun 2011, Nicolas Pitre wrote:

quoted

On Thu, 16 Jun 2011, Arnd Bergmann wrote:

quoted

On Thursday 16 June 2011 22:10:53 Alexander Holler wrote:
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.

I guess the issue is that some ABIs actually require a minimum alignment,
like the old ARM ABI that you can still use to build the kernel.

If a structure is not a multiple of four bytes in size, that ABI
will add padding at the end, e.g. in

struct s {
	char c[2];
};

struct t {
	struct s t1;
	unsigned short t2[3];
};

On most architectures, struct s will be two bytes in size and one byte
aligned, while struct t is eight bytes and two byte aligned.

On ARM oABI, struct s ends up with four byte size and alignment while
struct t is twelve bytes long. All this is ok for regular structures,
but not when they are used to describe memory layout of hardware
registers on on-wire packets.

quoted

If it does, then the follow-up question is: is a packing on word 
boundaries sufficient?

quoted

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.

I think Nicolas got it backwards here, adding both packed and
aligned(4) would make a structure like the one above consistently
incorrect when used to describe a tightly packed hardware structure.

In this case, we would have to do

struct s {
	char c[2];
} __packed;

struct t {
	struct s t1;
	unsigned short t2[3] __aligned(2);
} __packed;

To tell the compiler that t2 is indeed aligned, while struct t
is packed to include no padding around t.
 
I actually recently stumbled over code that gets this wrong,
see

http://git.kernel.org/?p=linux/kernel/git/arnd/playground.git;a=commit;h=284cef173aafd531a708f48e71a9cc7249fc8a98

quoted

 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?

Different issue.

	Arnd