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

[Gabbasov, Andrew]

> From: Gabbasov, Andrew
> Sent: Tuesday, May 14, 2013 21:27
> To: Albert ARIBAUD; Marek Vasut
> Cc: u-boot at lists.denx.de; Masahiro Yamada; trini at ti.com
> Subject: RE: [U-Boot] ARM: cfi_flash: Fix unaligned accesses to cfi_qry structure
> 
> 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

Does anybody have any comments on the mentioned updated patch?

Thanks.

Best regards,
Andrew