[PATCH v2 1/4] boot: fdt: Change type of env_get_bootm_low() to phys_addr_t

Change type of ulong env_get_bootm_low() to phys_addr_t env_get_bootm_low(). The PPC/LS systems already treat env_get_bootm_low() result as phys_addr_t, while the function itself still returns ulong. This is potentially dangerous on 64bit systems, where ulong might not be large enough to hold the content of "bootm_low" environment variable. Fix it by using phys_addr_t, similar to what env_get_bootm_size() does, which returns phys_size_t .
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Reported-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org --- Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Simon Glass sjg@chromium.org Cc: Tom Rini trini@konsulko.com --- V2: - Drop now unnecessary conversion in boot_start_lmb() lmb_init_and_reserve_range() - Drop tmp variable in env_get_bootm_low() - Print initrd_high as u64/%llx - Add RB from Laurent --- boot/bootm.c | 4 ++-- boot/image-board.c | 14 ++++++-------- boot/image-fdt.c | 9 +++++---- include/image.h | 2 +- 4 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/boot/bootm.c b/boot/bootm.c index d071537d692..032f5a4a160 100644 --- a/boot/bootm.c +++ b/boot/bootm.c @@ -242,13 +242,13 @@ static int boot_get_kernel(const char *addr_fit, struct bootm_headers *images, #ifdef CONFIG_LMB static void boot_start_lmb(struct bootm_headers *images) { - ulong mem_start; + phys_addr_t mem_start; phys_size_t mem_size;
mem_start = env_get_bootm_low(); mem_size = env_get_bootm_size();
- lmb_init_and_reserve_range(&images->lmb, (phys_addr_t)mem_start, + lmb_init_and_reserve_range(&images->lmb, mem_start, mem_size, NULL); } #else diff --git a/boot/image-board.c b/boot/image-board.c index 75f6906cd56..3263497a1d5 100644 --- a/boot/image-board.c +++ b/boot/image-board.c @@ -107,14 +107,12 @@ static int on_loadaddr(const char *name, const char *value, enum env_op op, } U_BOOT_ENV_CALLBACK(loadaddr, on_loadaddr);
-ulong env_get_bootm_low(void) +phys_addr_t env_get_bootm_low(void) { char *s = env_get("bootm_low");
- if (s) { - ulong tmp = hextoul(s, NULL); - return tmp; - } + if (s) + return simple_strtoull(s, NULL, 16);
#if defined(CFG_SYS_SDRAM_BASE) return CFG_SYS_SDRAM_BASE; @@ -538,7 +536,7 @@ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len, ulong *initrd_start, ulong *initrd_end) { char *s; - ulong initrd_high; + phys_addr_t initrd_high; int initrd_copy_to_ram = 1;
s = env_get("initrd_high"); @@ -553,8 +551,8 @@ int boot_ramdisk_high(struct lmb *lmb, ulong rd_data, ulong rd_len, initrd_high = env_get_bootm_mapsize() + env_get_bootm_low(); }
- debug("## initrd_high = 0x%08lx, copy_to_ram = %d\n", - initrd_high, initrd_copy_to_ram); + debug("## initrd_high = 0x%llx, copy_to_ram = %d\n", + (u64)initrd_high, initrd_copy_to_ram);
if (rd_data) { if (!initrd_copy_to_ram) { /* zero-copy ramdisk support */ diff --git a/boot/image-fdt.c b/boot/image-fdt.c index 5e4aa9de0d2..c2571b22244 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -160,9 +160,10 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) { void *fdt_blob = *of_flat_tree; void *of_start = NULL; - u64 start, size, usable; + phys_addr_t start, size, usable; char *fdt_high; - ulong mapsize, low; + phys_addr_t low; + phys_size_t mapsize; ulong of_len = 0; int bank; int err; @@ -217,7 +218,7 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) if (start + size < low) continue;
- usable = min(start + size, (u64)(low + mapsize)); + usable = min(start + size, low + mapsize);
/* * At least part of this DRAM bank is usable, try @@ -233,7 +234,7 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) * Reduce the mapping size in the next bank * by the size of attempt in current bank. */ - mapsize -= usable - max(start, (u64)low); + mapsize -= usable - max(start, low); if (!mapsize) break; } diff --git a/include/image.h b/include/image.h index 21de70f0c9e..acffd17e0df 100644 --- a/include/image.h +++ b/include/image.h @@ -946,7 +946,7 @@ static inline void image_set_name(struct legacy_img_hdr *hdr, const char *name) int image_check_hcrc(const struct legacy_img_hdr *hdr); int image_check_dcrc(const struct legacy_img_hdr *hdr); #ifndef USE_HOSTCC -ulong env_get_bootm_low(void); +phys_addr_t env_get_bootm_low(void); phys_size_t env_get_bootm_size(void); phys_size_t env_get_bootm_mapsize(void); #endif

Clean up tmp variable use and type cast in env_get_bootm_size(). No functional change.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org --- Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Simon Glass sjg@chromium.org Cc: Tom Rini trini@konsulko.com --- V2: - New patch --- boot/image-board.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/boot/image-board.c b/boot/image-board.c index 3263497a1d5..e3d63745299 100644 --- a/boot/image-board.c +++ b/boot/image-board.c @@ -129,10 +129,8 @@ phys_size_t env_get_bootm_size(void) phys_addr_t start; char *s = env_get("bootm_size");
- if (s) { - tmp = (phys_size_t)simple_strtoull(s, NULL, 16); - return tmp; - } + if (s) + return simple_strtoull(s, NULL, 16);
start = gd->ram_base; size = gd->ram_size; @@ -142,7 +140,7 @@ phys_size_t env_get_bootm_size(void)
s = env_get("bootm_low"); if (s) - tmp = (phys_size_t)simple_strtoull(s, NULL, 16); + tmp = simple_strtoull(s, NULL, 16); else tmp = start;

Hi Marek,
Thank you for the patch.
On Mon, Mar 18, 2024 at 04:00:45PM +0100, Marek Vasut wrote:
Clean up tmp variable use and type cast in env_get_bootm_size(). No functional change.
The code change looks fine, but you may want to expand the commit message to explain why this patch improves the code.
Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org
Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Simon Glass sjg@chromium.org Cc: Tom Rini trini@konsulko.com
V2: - New patch
boot/image-board.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/boot/image-board.c b/boot/image-board.c index 3263497a1d5..e3d63745299 100644 --- a/boot/image-board.c +++ b/boot/image-board.c @@ -129,10 +129,8 @@ phys_size_t env_get_bootm_size(void) phys_addr_t start; char *s = env_get("bootm_size");
- if (s) {
tmp = (phys_size_t)simple_strtoull(s, NULL, 16);
return tmp;
- }
if (s)
return simple_strtoull(s, NULL, 16);
start = gd->ram_base; size = gd->ram_size;
@@ -142,7 +140,7 @@ phys_size_t env_get_bootm_size(void)
s = env_get("bootm_low"); if (s)
tmp = (phys_size_t)simple_strtoull(s, NULL, 16);
else tmp = start;tmp = simple_strtoull(s, NULL, 16);
Maybe you could even drop the tmp variable completely by writing this
if (s) size -= simple_strtoull(s, NULL, 16) - start;
return size;
I've never liked variables named tmp :-)

On 3/18/24 5:18 PM, Laurent Pinchart wrote:
@@ -142,7 +140,7 @@ phys_size_t env_get_bootm_size(void)
s = env_get("bootm_low"); if (s)
tmp = (phys_size_t)simple_strtoull(s, NULL, 16);
else tmp = start;tmp = simple_strtoull(s, NULL, 16);
Maybe you could even drop the tmp variable completely by writing this
if (s) size -= simple_strtoull(s, NULL, 16) - start;
return size;
I've never liked variables named tmp :-)
No, let's not do this. With this FDT part, the code should be verbose and as easy to understand at first glance as possible, no subtraction assignments and other shenanigans please.

On Wed, Mar 20, 2024 at 09:52:34PM +0100, Marek Vasut wrote:
On 3/18/24 5:18 PM, Laurent Pinchart wrote:
@@ -142,7 +140,7 @@ phys_size_t env_get_bootm_size(void)
s = env_get("bootm_low"); if (s)
tmp = (phys_size_t)simple_strtoull(s, NULL, 16);
else tmp = start;tmp = simple_strtoull(s, NULL, 16);
Maybe you could even drop the tmp variable completely by writing this
if (s) size -= simple_strtoull(s, NULL, 16) - start;
return size;
I've never liked variables named tmp :-)
No, let's not do this. With this FDT part, the code should be verbose and as easy to understand at first glance as possible, no subtraction assignments and other shenanigans please.
How about this ?
s = env_get("bootm_low"); if (s) { phys_addr_t low_addr = simple_strtoull(s, NULL, 16); size -= low_addr - start; }
return size;
If you're going for readability, that's clearer than
return size - (tmp - start);
and it also interestingly points out that tmp/low_addr should be a phys_addr_t, not a phys_size_t.

On 3/20/24 10:00 PM, Laurent Pinchart wrote:
On Wed, Mar 20, 2024 at 09:52:34PM +0100, Marek Vasut wrote:
On 3/18/24 5:18 PM, Laurent Pinchart wrote:
@@ -142,7 +140,7 @@ phys_size_t env_get_bootm_size(void)
s = env_get("bootm_low"); if (s)
tmp = (phys_size_t)simple_strtoull(s, NULL, 16);
else tmp = start;tmp = simple_strtoull(s, NULL, 16);
Maybe you could even drop the tmp variable completely by writing this
if (s) size -= simple_strtoull(s, NULL, 16) - start;
return size;
I've never liked variables named tmp :-)
No, let's not do this. With this FDT part, the code should be verbose and as easy to understand at first glance as possible, no subtraction assignments and other shenanigans please.
How about this ?
s = env_get("bootm_low"); if (s) { phys_addr_t low_addr = simple_strtoull(s, NULL, 16); size -= low_addr - start; }
return size;
If you're going for readability, that's clearer than
return size - (tmp - start);
I do not share this opinion, the subtraction assignment makes this harder to read. If that makes for a more verbose code, so be it.
and it also interestingly points out that tmp/low_addr should be a phys_addr_t, not a phys_size_t.
The original code already contains trivial 'tmp = start' assignment, so these two types should match, and they don't. Fixed in V4, thanks.

The lmb_alloc_base() returns phys_addr_t , map_sysmem() accepts phys_addr_t as first parameter. Declare 'addr' as phys_addr_t and get rid of the casts.
Reported-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org --- Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Simon Glass sjg@chromium.org Cc: Tom Rini trini@konsulko.com --- V2: - Replace $addr with 'addr' to somehow delimit the variable name --- boot/image-fdt.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index c2571b22244..c37442c9130 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -162,6 +162,7 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) void *of_start = NULL; phys_addr_t start, size, usable; char *fdt_high; + phys_addr_t addr; phys_addr_t low; phys_size_t mapsize; ulong of_len = 0; @@ -186,7 +187,6 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) fdt_high = env_get("fdt_high"); if (fdt_high) { ulong desired_addr = hextoul(fdt_high, NULL); - ulong addr;
if (desired_addr == ~0UL) { /* All ones means use fdt in place */ @@ -224,8 +224,8 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) * At least part of this DRAM bank is usable, try * using it for LMB allocation. */ - of_start = map_sysmem((ulong)lmb_alloc_base(lmb, - of_len, 0x1000, usable), of_len); + addr = lmb_alloc_base(lmb, of_len, 0x1000, usable); + of_start = map_sysmem(addr, of_len); /* Allocation succeeded, use this block. */ if (of_start != NULL) break;

Hi Marek,
Thank you for the patch.
On Mon, Mar 18, 2024 at 04:00:46PM +0100, Marek Vasut wrote:
The lmb_alloc_base() returns phys_addr_t , map_sysmem() accepts phys_addr_t as first parameter. Declare 'addr' as phys_addr_t and get rid of the casts.
Reported-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Simon Glass sjg@chromium.org Cc: Tom Rini trini@konsulko.com
V2: - Replace $addr with 'addr' to somehow delimit the variable name
boot/image-fdt.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index c2571b22244..c37442c9130 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -162,6 +162,7 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) void *of_start = NULL; phys_addr_t start, size, usable; char *fdt_high;
- phys_addr_t addr; phys_addr_t low; phys_size_t mapsize; ulong of_len = 0;
@@ -186,7 +187,6 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) fdt_high = env_get("fdt_high"); if (fdt_high) { ulong desired_addr = hextoul(fdt_high, NULL);
ulong addr;
if (desired_addr == ~0UL) { /* All ones means use fdt in place */
@@ -224,8 +224,8 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) * At least part of this DRAM bank is usable, try * using it for LMB allocation. */
of_start = map_sysmem((ulong)lmb_alloc_base(lmb,
of_len, 0x1000, usable), of_len);
addr = lmb_alloc_base(lmb, of_len, 0x1000, usable);
of_start = map_sysmem(addr, of_len); /* Allocation succeeded, use this block. */ if (of_start != NULL) break;

Move the variable below comment which explains what the variable means. Update the comment. No functional change.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Suggested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Marek Vasut marek.vasut+renesas@mailbox.org --- Cc: Heinrich Schuchardt xypron.glpk@gmx.de Cc: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Simon Glass sjg@chromium.org Cc: Tom Rini trini@konsulko.com --- V2: - Update the comment, s@the the@the@ and quote 'usable' - Add RB from Laurent --- boot/image-fdt.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/boot/image-fdt.c b/boot/image-fdt.c index c37442c9130..2b92bdaff16 100644 --- a/boot/image-fdt.c +++ b/boot/image-fdt.c @@ -218,12 +218,12 @@ int boot_relocate_fdt(struct lmb *lmb, char **of_flat_tree, ulong *of_size) if (start + size < low) continue;
- usable = min(start + size, low + mapsize); - /* * At least part of this DRAM bank is usable, try - * using it for LMB allocation. + * using the DRAM bank up to 'usable' address limit + * for LMB allocation. */ + usable = min(start + size, low + mapsize); addr = lmb_alloc_base(lmb, of_len, 0x1000, usable); of_start = map_sysmem(addr, of_len); /* Allocation succeeded, use this block. */
participants (3)
-
Laurent Pinchart
-
Marek Vasut
-
Marek Vasut