* [U-Boot] [PATCH] fdt_support: fix unaligned u64 accesses in fdt_fixup_memory_banks
@ 2014-10-15 9:21 Rob Herring
2014-10-24 17:56 ` Simon Glass
0 siblings, 1 reply; 5+ messages in thread
From: Rob Herring @ 2014-10-15 9:21 UTC (permalink / raw)
To: u-boot
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.
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);
p += size_len;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] fdt_support: fix unaligned u64 accesses in fdt_fixup_memory_banks
2014-10-15 9:21 [U-Boot] [PATCH] fdt_support: fix unaligned u64 accesses in fdt_fixup_memory_banks Rob Herring
@ 2014-10-24 17:56 ` Simon Glass
2014-10-25 4:51 ` Rob Herring
0 siblings, 1 reply; 5+ messages in thread
From: Simon Glass @ 2014-10-24 17:56 UTC (permalink / raw)
To: u-boot
+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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] fdt_support: fix unaligned u64 accesses in fdt_fixup_memory_banks
2014-10-24 17:56 ` Simon Glass
@ 2014-10-25 4:51 ` Rob Herring
2014-10-28 0:11 ` Simon Glass
0 siblings, 1 reply; 5+ messages in thread
From: Rob Herring @ 2014-10-25 4:51 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] fdt_support: fix unaligned u64 accesses in fdt_fixup_memory_banks
2014-10-25 4:51 ` Rob Herring
@ 2014-10-28 0:11 ` Simon Glass
2014-10-28 10:58 ` Måns Rullgård
0 siblings, 1 reply; 5+ messages in thread
From: Simon Glass @ 2014-10-28 0:11 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH] fdt_support: fix unaligned u64 accesses in fdt_fixup_memory_banks
2014-10-28 0:11 ` Simon Glass
@ 2014-10-28 10:58 ` Måns Rullgård
0 siblings, 0 replies; 5+ messages in thread
From: Måns Rullgård @ 2014-10-28 10:58 UTC (permalink / raw)
To: u-boot
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().
--
M?ns Rullg?rd
mans at mansr.com
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-10-28 10:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-15 9:21 [U-Boot] [PATCH] fdt_support: fix unaligned u64 accesses in fdt_fixup_memory_banks Rob Herring
2014-10-24 17:56 ` Simon Glass
2014-10-25 4:51 ` Rob Herring
2014-10-28 0:11 ` Simon Glass
2014-10-28 10:58 ` Måns Rullgård
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox