Re: [PATCH 1/8] kernel: add common infrastructure for unaligned access

[Harvey Harrison <harvey.harrison-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Thu, 2008-04-10 at 22:43 +0100, David Howells wrote:
> Harvey Harrison <harvey.harrison-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > +static inline u16 __get_unaligned_le16(const u8 *p)
> > +{
> > +	return (u16)(p[0] | p[1] << 8);
> > +}
> 
> You shouldn't need these casts.  return is going to cast it anyway.
> 
> Actually, you probably _ought_ to have casts, but it should look like this:
> 
> 	return (u16)p[0] | (u16)p[1] << 8;

I've been looking at that thinking I needed something different, I
believe it is ok as u8 will expand to int when shifted... correct?  Or
do I actually need the cast on each p[] term...anyone?

> 
> You are shifting an 8-bit value left by 8 bits, so the compiler may be at
> liberty to instruct the RHS to end up zero.
> 
> I presume the compiler is guaranteed not to merge the two memory accesses?  It
> can't seem to make it do so, though I seem to remember there were cases where
> it did, though I can't reproduce them.  I assume that's why you're passing in
> a u8 pointer and not a u16/u32/u64 pointer.

Yes, that is the reason.  The implementation is nearly identical to the
existing arm version in-tree (minus the register keywords of course).

Harvey