Simon Glass <[email protected]> writes: > Hi Rob, > > On 24 October 2014 22:51, Rob Herring <[email protected]> wrote: >> On Sat, Oct 25, 2014 at 1:56 AM, Simon Glass <[email protected]> wrote: >>> +Tom >>> >>> Hi Rob, >>> >>> On 15 October 2014 03:21, Rob Herring <[email protected]> wrote: >>>> From: Rob Herring <[email protected]> >>>> >>>> Commit f18295d3837c282f (fdt_support: fix an endian bug of >>>> fdt_fixup_memory_banks) changed fdt_fixup_memory_banks cell writing from a >>>> byte at a time to casting the buffer pointer to a 64-bit pointer. This can >>>> result in unaligned accesses when there is a mixture of cell sizes of 1 >>>> and 2. >>> >>> Should that be 739a01ed8e0? >> >> Uhh, yes. >> >>> >>>> >>>> Cc: Masahiro Yamada <[email protected]> >>>> Signed-off-by: Rob Herring <[email protected]> >>>> --- >>>> common/fdt_support.c | 11 +++++++---- >>>> 1 file changed, 7 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/common/fdt_support.c b/common/fdt_support.c >>>> index 3f64156..9c41f3a 100644 >>>> --- a/common/fdt_support.c >>>> +++ b/common/fdt_support.c >>>> @@ -388,19 +388,22 @@ static int fdt_pack_reg(const void *fdt, void *buf, >>>> uint64_t *address, >>>> int i; >>>> int address_len = get_cells_len(fdt, "#address-cells"); >>>> int size_len = get_cells_len(fdt, "#size-cells"); >>>> + u64 cell; >>>> char *p = buf; >>>> >>>> for (i = 0; i < n; i++) { >>>> if (address_len == 8) >>>> - *(fdt64_t *)p = cpu_to_fdt64(address[i]); >>>> + cell = cpu_to_fdt64(address[i]); >>>> else >>>> - *(fdt32_t *)p = cpu_to_fdt32(address[i]); >>>> + cell = cpu_to_fdt32(address[i]); >>>> + memcpy(p, &cell, address_len); >>>> p += address_len; >>>> >>>> if (size_len == 8) >>>> - *(fdt64_t *)p = cpu_to_fdt64(size[i]); >>>> + cell = cpu_to_fdt64(size[i]); >>>> else >>>> - *(fdt32_t *)p = cpu_to_fdt32(size[i]); >>>> + cell = cpu_to_fdt32(size[i]); >>>> + memcpy(p, &cell, size_len); >>> >>> What will this do with 32-bit values? Aren't use assuming that the >>> first 32-bit word of 'cell' will contain the least significant 32 >>> bits? Is that always true? I'm not quite sure. >> >> We've already done the endian conversion, so we're just copying a >> string of bytes only changing the alignment potentially. > > Yes I think you are right. Then I suggest adding a comment here, as > memcpy() from a native type is unusual.
Better yet, use put_unaligned(). -- Måns Rullgård [email protected] _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
