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

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

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

> > > Best regards,
> > > Marek Vasut

Amicalement,
-- 
Albert.