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

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

[Marek Vasut]

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
-- 
2.43.0

[PATCH v2 2/4] boot: fdt: Clean up env_get_bootm_size()

[Marek Vasut]

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;
 
-- 
2.43.0

[PATCH v2 3/4] boot: fdt: Drop lmb_alloc*() typecasts

[Marek Vasut]

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;
-- 
2.43.0

[PATCH v2 4/4] boot: fdt: Move usable variable below updated comment

[Marek Vasut]

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. */
-- 
2.43.0

Re: [PATCH v2 2/4] boot: fdt: Clean up env_get_bootm_size()

[Laurent Pinchart]

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);
> +		tmp = simple_strtoull(s, NULL, 16);
>  	else
>  		tmp = start;
>  

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 :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/4] boot: fdt: Drop lmb_alloc*() typecasts

[Laurent Pinchart]

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;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/4] boot: fdt: Clean up env_get_bootm_size()

[Marek Vasut]

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);
>> +		tmp = simple_strtoull(s, NULL, 16);
>>   	else
>>   		tmp = start;
>>   
> 
> 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.

Re: [PATCH v2 2/4] boot: fdt: Clean up env_get_bootm_size()

[Laurent Pinchart]

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);
> >> +		tmp = simple_strtoull(s, NULL, 16);
> >>   	else
> >>   		tmp = start;
> >>   
> > 
> > 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.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/4] boot: fdt: Clean up env_get_bootm_size()

[Marek Vasut]

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);
>>>> +		tmp = simple_strtoull(s, NULL, 16);
>>>>    	else
>>>>    		tmp = start;
>>>>    
>>>
>>> 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.