lkml.org 
[lkml]   [2011]   [Jun]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] USB: ehci: use packed,aligned(4) instead of removing the packed attribute
    Date
    On Sunday 19 June 2011 21:00:01 Alan Stern wrote:
    > 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:
    > > > 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.

    > > If it does, then the follow-up question is: is a packing on word
    > > boundaries sufficient?
    >
    > > 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

    > > 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


    \
     
     \ /
      Last update: 2011-06-19 22:07    [W:3.770 / U:0.148 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site