[U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure

[Albert ARIBAUD]

Hi Marek,

On Fri, 10 May 2013 14:36:00 +0200, Marek Vasut <marex at denx.de> wrote:

> Hello Masahiro-san,

> > By the way, I also had this unalignment access problem for my board.
> > Before finding your patch, I was thinking another way to fix this problem.
> > 
> > My idea is to just use 'get_unaligned' and 'put_unaligned' functions
> > instead of introducing special macros.
> > With this way, we don't need to change the fields of struct cfi_qry.
> 
> I think we should make sure to use natural alignment as much as possible, 
> really. I'm keeping Albert in CC because this is his turf.

Marek, you invoked me; next time be careful what you wish for. :)

My rules (not 'of thumb', as they also apply to ARM) regarding
alignment are as follows (yes, it's a more general answer than your
question called for, but the last rules are more directly related):

0) Never assume that U-Boot can use native unaligned accesses. Yes,
ARMv7+ can do native unaligned accesses with almost no performance
cost, but U-Boot runs on all sorts of targets (not only ARM), some
allowing unaligned access but with a penalty, and some unable to
perform unaligned access at all. We always run the risk that some code
in U-Boot ends up running on target which will die horribly on some
unaligned access, so to avoid this we must assume and enforce strict
alignment, even for architectures which do not require it, such as
ARMv7+.

1) As per rule 0, always enable alignment check -- again, even on
targets which could do without it. This allows catching any unaligned
access, be they accidental (bad pointer arithmetic) or by design
(misaligned field in an otherwise aligned struct).

2) Despite rules 0 and 1, always enable native unaligned accesses (IOW,
always use option -munaligned-accesses). That is because without this
option (or more precisely, when -mno-unaligned-accesses is in effect),
the ARM compiler may silently detect misaligned accesses and fix them
using smaller aligned accesses, thus hiding a potential alignment
issue until it bites on some later and unrelated target run.

3) always size fields in a structure to their natural size, i.e., if a
field is 16-bits, make it u16 or s16.

4) always align fields in a structure to their natural boundaries,
i.e., if a field is 16-bits, align it to an even address.

5) if a field absolutely has to be unaligned because of hardware or
standard, then a) document that! and b) access it with explicit
unaligned access macros.

So in essence, I am opposed to changing fields from 16-bit to 2 x 8-bit
just 'because unaligned'. Either fix the fields' alignment, if at all
possible; and if not, then apply rule 5: document the issue and fix it
using explicit unaligned access macros.

> Best regards,
> Marek Vasut

Amicalement,
-- 
Albert.