| Message ID | 1413364860-23866-1-git-send-email-robherring2@gmail.com |
|---|---|
| State | Deferred |
| Delegated to: | Jerry Van Baren |
| Headers | show |
+Tom Hi Rob, On 15 October 2014 03:21, Rob Herring <robherring2@gmail.com> wrote: > From: Rob Herring <robh@kernel.org> > > 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? > > Cc: Masahiro Yamada <yamada.m@jp.panasonic.com> > Signed-off-by: Rob Herring <robh@kernel.org> > --- > 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. Also I really thing this needs a test. It's a pretty simple function so a unit test would be easy to write. > p += size_len; > } Regards, Simon
On Sat, Oct 25, 2014 at 1:56 AM, Simon Glass <sjg@chromium.org> wrote: > +Tom > > Hi Rob, > > On 15 October 2014 03:21, Rob Herring <robherring2@gmail.com> wrote: >> From: Rob Herring <robh@kernel.org> >> >> 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 <yamada.m@jp.panasonic.com> >> Signed-off-by: Rob Herring <robh@kernel.org> >> --- >> 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. > > Also I really thing this needs a test. It's a pretty simple function > so a unit test would be easy to write. I'll look into that. Rob > >> p += size_len; >> } > > Regards, > Simon
Hi Rob, On 24 October 2014 22:51, Rob Herring <robherring2@gmail.com> wrote: > On Sat, Oct 25, 2014 at 1:56 AM, Simon Glass <sjg@chromium.org> wrote: >> +Tom >> >> Hi Rob, >> >> On 15 October 2014 03:21, Rob Herring <robherring2@gmail.com> wrote: >>> From: Rob Herring <robh@kernel.org> >>> >>> 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 <yamada.m@jp.panasonic.com> >>> Signed-off-by: Rob Herring <robh@kernel.org> >>> --- >>> 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. > >> >> Also I really thing this needs a test. It's a pretty simple function >> so a unit test would be easy to write. > > I'll look into that. See test/command_ut.c for one way to do this with sandbox by adding a new command. Regards, Simon
Simon Glass <sjg@chromium.org> writes: > Hi Rob, > > On 24 October 2014 22:51, Rob Herring <robherring2@gmail.com> wrote: >> On Sat, Oct 25, 2014 at 1:56 AM, Simon Glass <sjg@chromium.org> wrote: >>> +Tom >>> >>> Hi Rob, >>> >>> On 15 October 2014 03:21, Rob Herring <robherring2@gmail.com> wrote: >>>> From: Rob Herring <robh@kernel.org> >>>> >>>> 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 <yamada.m@jp.panasonic.com> >>>> Signed-off-by: Rob Herring <robh@kernel.org> >>>> --- >>>> 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().
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); p += size_len; }