On 1 April 2018 at 22:19, Simon Glass <s...@chromium.org> wrote:
> Hi,
>
> On 22 February 2018 at 08:08, Alexander Graf <ag...@suse.de> wrote:
>>
>>
>> On 15.02.18 07:40, Andre Heider wrote:
>>> The value at the end of the rom is not a pointer, it is an offset
>>> relative to the end of rom.
>>
>> Do you have any documentation pointers to this? Even just pointing to a
>> specific line of code in coreboot would be helpful in case anyone wants
>> to read it up.
>>
>>>
>>> Signed-off-by: Andre Heider <a.hei...@gmail.com>
>>> ---
>>> fs/cbfs/cbfs.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/cbfs/cbfs.c b/fs/cbfs/cbfs.c
>>> index 6e1107d751..46da8f134f 100644
>>> --- a/fs/cbfs/cbfs.c
>>> +++ b/fs/cbfs/cbfs.c
>>> @@ -168,9 +168,9 @@ static int file_cbfs_load_header(uintptr_t end_of_rom,
>>> struct cbfs_header *header)
>>> {
>>> struct cbfs_header *header_in_rom;
>>> + int32_t offset = *(u32 *)(end_of_rom - 3);
>>
>> Accessing a pointer that gets subtracted by 3 looks terribly wrong.
>> Unfortunately it's correct, because the pointer itself is unaligned.
>>
>> How about we change the logic so that we calculate an aligned pointer
>> (after_rom?) which we sanity check for alignment and base all
>> calculations on that instead?
>>
>> That way the next time someone comes around he's not getting puzzled why
>> we're subtracting 3 and adding 1 to the pointer ;).
>
> Either that or a comment would be nice. But since this has been
> sitting around for a while and fixes a bug I think it is best to take
> it. Please feel free to send a follow-up patch..
>
> We also have no tests for this code, so I'd really like to get some
> tests in there!
>
> Reviewed-by: Simon Glass <s...@chromium.org>
Applied to u-boot-dm, thanks!
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
