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

[Gabbasov, Andrew]

Hello Albert,

> From: Albert ARIBAUD [albert.u.boot at aribaud.net]
> Sent: Monday, May 13, 2013 10:48
> To: Marek Vasut
> Cc: u-boot at lists.denx.de; Masahiro Yamada; trini at ti.com; Gabbasov, Andrew
> Subject: Re: [U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure
> 
> 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.

Thank you for your review and the very useful list of rules.

Although theoretically the cfi_qry structure can be made non-packed and
with properly aligned members (and filled in by individual members assignments
when reading, rather than just copying byte-to-byte, as it is now),
it may make more sense to keep it packed and replicating the actual flash layout
as much as possible. This also makes it more similar to what is used
in Linux kernel (although it seems to rely on native unaligned access ;-)).

So, it seems reasonable to choose to apply rule 5: add comments to cfi_qry strcuture
definition and use get_unaligned/put_unaligned for the necessary fields (only for really
unaligned members, since some 16-bit fields are properly aligned).

I'm sending the version 2 of the patch with this change as a separate message with
Subject: [U-Boot][PATCH v2] cfi_flash: Fix unaligned accesses to cfi_qry structure

Please, let me know if somebody still prefers making non-packed aligned structure,
as described above.

Thanks.

Best regards,
Andrew