From: arnd@arndb•de (Arnd Bergmann)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH] USB: ehci: use packed, aligned(4) instead of removing the packed attribute
Date: Sun, 19 Jun 2011 22:02:23 +0200 [thread overview]
Message-ID: <201106192202.23989.arnd@arndb.de> (raw)
In-Reply-To: <Pine.LNX.4.44L0.1106191453200.14586-100000@netrider.rowland.org>
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
next prev parent reply other threads:[~2011-06-19 20:02 UTC|newest]
Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-04-27 14:34 [PATCH] echi: remove structure packing from ehci_def Rabin Vincent
2011-04-27 15:15 ` Sergei Shtylyov
2011-04-27 15:37 ` [PATCHv2] " Rabin Vincent
2011-06-16 16:17 ` [PATCH] USB: ehci: use packed, aligned(4) instead of removing the packed attribute Alexander Holler
2011-06-16 17:09 ` [PATCH] USB: ehci: use packed,aligned(4) " Alan Stern
2011-06-16 17:55 ` [PATCH] USB: ehci: use packed, aligned(4) " Arnd Bergmann
2011-06-16 19:25 ` [PATCH] USB: ehci: use packed,aligned(4) " Alexander Holler
2011-06-16 19:46 ` Alan Stern
2011-06-16 20:10 ` Alexander Holler
2011-06-16 20:20 ` [PATCH] USB: ehci: use packed, aligned(4) " Arnd Bergmann
2011-06-19 15:02 ` Nicolas Pitre
2011-06-19 19:00 ` [PATCH] USB: ehci: use packed,aligned(4) " Alan Stern
2011-06-19 20:02 ` Arnd Bergmann [this message]
2011-06-19 20:11 ` [PATCH] USB: ehci: use packed, aligned(4) " Arnd Bergmann
2011-06-19 21:39 ` Nicolas Pitre
2011-06-19 21:27 ` Nicolas Pitre
2011-06-20 15:03 ` [PATCH] USB: ehci: use packed,aligned(4) " Alan Stern
2011-06-20 16:16 ` [PATCH] USB: ehci: use packed, aligned(4) " Nicolas Pitre
2011-06-20 16:48 ` [PATCH] USB: ehci: use packed,aligned(4) " Alan Stern
2011-06-20 16:58 ` [PATCH] USB: ehci: use packed, aligned(4) " Arnd Bergmann
2011-06-20 19:02 ` Russell King - ARM Linux
2011-06-20 19:20 ` Nicolas Pitre
2011-06-20 19:29 ` Nicolas Pitre
2011-06-20 17:10 ` Nicolas Pitre
2011-06-20 17:35 ` [PATCH] USB: ehci: use packed,aligned(4) " Alan Stern
2011-06-20 18:48 ` Russell King - ARM Linux
2011-06-20 20:26 ` [PATCH] USB: ehci: use packed, aligned(4) " Arnd Bergmann
2011-06-20 20:50 ` Nicolas Pitre
2011-06-20 20:55 ` [PATCH] USB: ehci: use packed,aligned(4) " Russell King - ARM Linux
2011-06-20 21:23 ` [PATCH] USB: ehci: use packed, aligned(4) " Arnd Bergmann
2011-06-20 22:23 ` Nicolas Pitre
2011-06-21 11:25 ` Arnd Bergmann
2011-06-25 1:25 ` Nicolas Pitre
2011-06-25 8:09 ` Arnd Bergmann
2011-06-28 18:51 ` Nicolas Pitre
2011-06-29 10:56 ` Arnd Bergmann
2011-06-20 19:14 ` Nicolas Pitre
2011-06-20 19:32 ` Russell King - ARM Linux
2011-06-20 20:14 ` Arnd Bergmann
2011-06-20 20:42 ` [PATCH] USB: ehci: use packed,aligned(4) " Alan Stern
2011-06-20 22:36 ` [PATCH] USB: ehci: use packed, aligned(4) " Nicolas Pitre
2011-06-21 15:06 ` [PATCH] USB: ehci: use packed,aligned(4) " Alan Stern
2011-06-20 17:39 ` Alexander Holler
2011-06-20 18:39 ` Alan Stern
2011-06-20 18:46 ` Alexander Holler
2011-06-20 18:57 ` Alan Stern
2011-06-20 19:56 ` [PATCH] USB: ehci: use packed, aligned(4) " Nicolas Pitre
2011-06-20 21:04 ` [PATCH] USB: ehci: use packed,aligned(4) " Alan Stern
2011-06-20 22:31 ` [PATCH] USB: ehci: use packed, aligned(4) " Nicolas Pitre
2011-06-21 14:58 ` [PATCH] USB: ehci: use packed,aligned(4) " Alan Stern
2011-06-21 20:41 ` [PATCH] USB: ehci: use packed, aligned(4) " Nicolas Pitre
2011-06-22 6:23 ` [PATCH] USB: ehci: use packed,aligned(4) " Alexander Holler
2011-06-20 20:09 ` [PATCH] USB: ehci: use packed, aligned(4) " Arnd Bergmann
2011-06-20 21:05 ` [PATCH] USB: ehci: use packed,aligned(4) " Alan Stern
2011-06-20 20:07 ` [PATCH] USB: ehci: use packed, aligned(4) " Arnd Bergmann
2011-06-20 20:28 ` Nicolas Pitre
2011-06-20 20:39 ` Arnd Bergmann
2011-06-20 21:03 ` Nicolas Pitre
2011-06-23 9:47 ` Alexander Holler
2011-06-23 14:25 ` Alan Stern
2011-06-24 11:40 ` Alexander Holler
2011-06-20 16:26 ` Arnd Bergmann
2011-06-16 20:30 ` [PATCH] USB: ehci: use packed,aligned(4) " Alan Stern
2011-06-16 18:16 ` Alexander Holler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201106192202.23989.arnd@arndb.de \
--to=arnd@arndb$(echo .)de \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox