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> Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot