[U-Boot] [PATCH] usb: do explicit unaligned accesses

[Marek Vasut]

Dear Albert ARIBAUD,

> Hi Marek,
> 
> On Sat, 1 Sep 2012 17:12:29 +0200, Marek Vasut <marex at denx.de> wrote:
> > > > > One place where lumping and misalignement prevention did clash
> > > > > was raised in the previous discussion: a 7+1 bytes
> > > > > function-local char array was allocated on a non-aligned
> > > > > address (which is possible and normal because it is a char) and
> > > > > was initialized with some content. The compiler lumped the
> > > > > initialization as two misaligned 32-byte native accesses,
> > > > > despite misaligned native accesses being forbidden by compiler
> > > > > command line options. This was a compiler bug.
> > > > 
> > > > But that'd mean that instead of fixing a compiler, we'd be doing a
> > > > workaround in our code?
> > > 
> > > Not exactly.
> > > 
> > > First, in this instance, a fix to the compiler has been at least
> > > requested, if not already applied (I would need to check this). The
> > > fix causes the compiler to still generate misaligned 32-bit
> > > accesses *if* misaligned native accesses are allowed, and to use
> > > only allowed accesses otherwise.
> > 
> > But then again, this is compiler bug we exposed, no need to hide it.
> > I'm firm on this one.
> 
> I guess I was not clear: this issue with an 8-char local array was
> *not* in U-Boot. So we exposed nothing there, and none of the
> discussion led to any conclusion that we should hide anything under the
> carpet. Actually, if/when we meet a compiler issue, my suggestion is
> always to explicitly and lavishly comment the 'fix', whatever it is,
> with warnings such as /* CAUTION! BRAINDEAD COMPILER! There is an issue
> with compiler X versions Y and up where [...] */. And keep an eye on the
> compiler.

Agreed

> > > Second, I do not ask U-Boot contributors to mark code as explicitly
> > > unaligned when the misaligned access is caused by a compiler or
> > > code error; I ask them to mark code as unaligned when the misaligned
> > > access is *unavoidable* because the HW or some standard imposes it.
> > 
> > I see, I'm starting to see your point. Maybe because I've missed the
> > previous discussion.
> 
> I think so.
> 
> > > Here, the specification from which the USB struc is derived imposes
> > > a misaligned field. This, rather than any compiler bug, makes the
> > > misaligned access *unavoidable*. And because it is, I ask that it be
> > > marked so, by the explicit use of unaligned accessors.
> > 
> > Still, this is unaligned only on ARM, not on maybe some other arches,
> > right?
> 
> The notion of 'misaligned' (rather than unaligned) is pretty much
> architecture-independent: the fact that a 32-bit int is not on a
> 4-byte boundary makes it misaligned. Now, depending on arches, this
> misalignment may be irrelevant because the arch can do native misaligned
> accesses with little or no penalty, or may be a worry because the arch
> can (be made to) do native misaligned accesses but at a performance
> cost, or it may be a blocker because the arch cannot do native
> misaligned accesses.
> 
> So maybe on some other arches misalignment is 'not a problem', or
> maybe it is 'a serious issue'. Anyway, for ARM is ranges from one ed
> to the other, and for any arch, on a given system the seriousness of the
> issue may be set by the designers of the system.
> 
> > > Yes. However, letting the compiler generate workarounds may end up
> > > letting it generate workarounds for misaligned accesses caused by
> > > errors or bugs also. Marking the code explicitly helps telling
> > > which is which too.
> > 
> > Does this work across architectures too? Like, on arm it's
> > misaligned, on intel it isnt.
> 
> Each architecture has its own capabilites regarding native misaligned
> accesses... This is why I consider that as a general rule U-Boot should
> always align its data properly, because (hopefully) all architectures
> can do aligned native accesses; OTOH, if we accept misaligned code on
> the grounds that 'it works on such and suh arches' or that 'any normal
> arch should be able to handle misaligned accesses some way' or 'no one
> in their right mind would physically forbit misaligned accesses', then
> we're just giving Murphy a chance to kick us at some point.
> 
> Consider this an application of Postel's principle: we liberally
> accept architectures that maybe allow misaligned accesses and maybe
> handle them well; and we conservatively do not do such accesses unless
> we have no other choice.
> 
> > > Here it is barely an ad hoc solution, as the alternative would be
> > > fixing the hardware or worse, spec (can someone tell us where this
> > > misaligned struct field originates from exactly, hw or USB spec?)
> > 
> > http://www.intel.com/technology/usb/download/ehci-r10.pdf I think
> > you're looking around 3.6 .
> 
> I see section 3.6 (Queue Head) but I don't readily see the link
> between the header and the patch we're discussiong (but I'm not an
> USB expert either). Can you connect e few more dots for me? Thanks in
> advance.

Nevermind, I took a second look and noticed how the structure looks.

Anyway, I see USB is pioneering this new rule, nice :-) I now basically see the 
issue at hand, sorry for being a blind ass. Remind me next time to look first 
and talk afterwards.

361 struct usb_hub_descriptor {
362         unsigned char  bLength;
363         unsigned char  bDescriptorType;
364         unsigned char  bNbrPorts;
365         unsigned short wHubCharacteristics;

Let's apply this patch once (if?) WD pulls my USB tree.

> > > > Best regards,
> > > > Marek Vasut
> 
> Amicalement,