common: bootm: check return value of strict_strtoul

PATCH In review Peng Fan 5 participants 13 messages Nov 24, 2015 – Dec 6, 2015

Subsystems (2)

Arm (32-bit) Framebuffer layer

lore

Share & tools

lore

Peng Fan

author

2015-11-24 08:54 UTC #0 lore raw

PATCH 1/1 common: bootm: check return value of strict_strtoul

Before continue, check return value of strict_strtoul.

Signed-off-by Peng Fan <[email protected]>

Cc Albert Aribaud <[email protected]>

Cc Simon Glass <[email protected]>

Cc Jan Kiszka <[email protected]>

Cc Joe Hershberger <[email protected]>

Cc Hans de Goede <[email protected]>

Cc York Sun <[email protected]>

Cc Tom Rini <[email protected]>

---
 arch/arm/lib/bootm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

arch/arm/lib/bootm.c +4 -1

index ee56d74..a477cae 100644
@@ -290,7 +290,10 @@ static void boot_jump_linux(bootm_headers_t *images, int flag)
 
 	s = getenv("machid");
 	if (s) {
-		strict_strtoul(s, 16, &machid);
+		if (strict_strtoul(s, 16, &machid) < 0) {
+			debug("strict_strtoul failed!\n");
+			return;
+		}
 		printf("Using machid 0x%lx from environment\n", machid);
 	}

Signature · 1 line

2.6.2

Peng Fan

author

2015-11-24 08:54 UTC #1 lore raw

RFC 1/1 common: cli_hush: avoid dead code

Condition "(value == NULL && ++value == NULL)" actully will always return false.

Instead, use condition "(value == NULL || *(value + 1) == 0)" to detect such expression "c=". To "c=", *(value + 1) is 0, so directly return -1, but not continue.

Signed-off-by Peng Fan <[email protected]>

Cc Rabin Vincent <[email protected]>

Cc Simon Glass <[email protected]>

Cc Tom Rini <[email protected]>

---
 common/cli_hush.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

common/cli_hush.c +1 -1

index a7cac4f..f075459 100644
@@ -2162,7 +2162,7 @@ int set_local_var(const char *s, int flg_export)
 	 * NAME=VALUE format.  So the first order of business is to
 	 * split 's' on the '=' into 'name' and 'value' */
 	value = strchr(name, '=');
-	if (value == NULL && ++value == NULL) {
+	if (value == NULL || *(value + 1) == 0) {
 		free(name);
 		return -1;
 	}

Signature · 1 line

2.6.2

Tom Rini

2015-12-06 22:06 UTC #3 lore raw

[U-Boot] [U-Boot,RFC] common: cli_hush: avoid dead code

In reply to Peng Fan

Quoted 7 lines · Peng Fan

Condition "(value == NULL && ++value == NULL)" actully will always return false.
Instead, use condition "(value == NULL || *(value + 1) == 0)" to detect such expression "c=". To "c=", *(value + 1) is 0, so directly return -1, but not continue.

Quoted 5 lines

Signed-off-by Peng Fan <[email protected]>
Cc Rabin Vincent <[email protected]>
Cc Simon Glass <[email protected]>
Cc Tom Rini <[email protected]>
Reviewed-by Simon Glass <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151206/b1d1b087/attachment.sig>

Peng Fan

author

2015-11-24 08:54 UTC #4 lore raw

PATCH 1/1 common: image-fdt: correct fdt_blob for IMAGE_FORMAT_LEGACY

In reply to Peng Fan

If condition of "(load == image_start || load == image_data)" is true, should use "fdt_addr = load;", but not "fdt_blob = (char *)image_data;", or fdt_blob will be overridden by "fdt_blob = map_sysmem(fdt_addr, 0);" at the end of the switch case.

Signed-off-by Peng Fan <[email protected]>

Cc Simon Glass <[email protected]>

Cc Joe Hershberger <[email protected]>

Cc Max Krummenacher <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Suriyan Ramasami <[email protected]>

Cc Paul Kocialkowski <[email protected]>

Cc Tom Rini <[email protected]>

---
 common/image-fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

common/image-fdt.c +1 -1

index 5180a03..5e4e5bd 100644
@@ -326,7 +326,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
 
 			if (load == image_start ||
 			    load == image_data) {
-				fdt_blob = (char *)image_data;
+				fdt_addr = load;
 				break;
 			}

Signature · 1 line

2.6.2

Simon Glass

2015-11-24 19:04 UTC #5 lore raw

PATCH 1/1 common: image-fdt: correct fdt_blob for IMAGE_FORMAT_LEGACY

Hi Peng,

Quoted 22 lines · Peng Fan

If condition of "(load == image_start || load == image_data)" is true, should use "fdt_addr = load;", but not "fdt_blob = (char *)image_data;", or fdt_blob will be overridden by "fdt_blob = map_sysmem(fdt_addr, 0);" at the end of the switch case.
Signed-off-by Peng Fan <[email protected]>
Cc Simon Glass <[email protected]>
Cc Joe Hershberger <[email protected]>
Cc Max Krummenacher <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Suriyan Ramasami <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Tom Rini <[email protected]>
---
 common/image-fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/image-fdt.c +0 -0
index 5180a03..5e4e5bd 100644

common/image-fdt.c int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, · line 331

Quoted 7 lines

common/image-fdt.c +1 -1

@@ -326,7 +326,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
 
                        if (load == image_start ||
                            load == image_data) {
-                               fdt_blob = (char *)image_data;
+                               fdt_addr = load;
                                break;
                        }

Are you sure that should not be:

fdt_addr = image_data

The idea is to pick up the FDT from inside the image, since the load address indicates that it should not be relocated.

BTW one more thing I noticed:

image_data = (ulong)image_get_data(fdt_hdr);

The cast is confusing, and can be removed.

Regards,
Simon

Peng Fan

2015-11-25 01:12 UTC #6 lore raw

PATCH 1/1 common: image-fdt: correct fdt_blob for IMAGE_FORMAT_LEGACY

Hi Simon,

Quoted 35 lines · Simon Glass

Hi Peng,
Quoted 22 lines · Peng Fan
If condition of "(load == image_start || load == image_data)" is true, should use "fdt_addr = load;", but not "fdt_blob = (char *)image_data;", or fdt_blob will be overridden by "fdt_blob = map_sysmem(fdt_addr, 0);" at the end of the switch case.
Signed-off-by Peng Fan <[email protected]>
Cc Simon Glass <[email protected]>
Cc Joe Hershberger <[email protected]>
Cc Max Krummenacher <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Suriyan Ramasami <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Tom Rini <[email protected]>
---
 common/image-fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/image-fdt.c +0 -0
index 5180a03..5e4e5bd 100644
common/image-fdt.c int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, · line 331
Quoted 7 lines
common/image-fdt.c +1 -1
@@ -326,7 +326,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
 
                        if (load == image_start ||
                            load == image_data) {
-                               fdt_blob = (char *)image_data;
+                               fdt_addr = load;
                                break;
                        }
Are you sure that should not be:

Quoted 3 lines

fdt_addr = image_data
?

Just code inspection.

See the following code piece:

319                         image_start = (ulong)fdt_hdr;
320                         image_data = (ulong)image_get_data(fdt_hdr);
321                         image_end = image_get_image_end(fdt_hdr);

322
323 load = image_get_load(fdt_hdr);
324 load_end = load + image_get_data_size(fdt_hdr);
325
326 if (load == image_start ||
327 load == image_data) {
328 fdt_blob = (char *)image_data;
329 break;
330 }
331
332 if ((load < image_end) && (load_end > image_start)) {
333 fdt_error("fdt overwritten");
334 goto error;
335 }
336
337 debug(" Loading FDT from 0x%08lx to 0x%08lx\n",
338 image_data, load);
339
340 memmove((void *)load,
341 (void *)image_data,
342 image_get_data_size(fdt_hdr));
343
344 fdt_addr = load;
345 break;

					.........[snip code].........

386 printf(" Booting using the fdt blob at %#08lx\n", fdt_addr);
387 fdt_blob = map_sysmem(fdt_addr, 0);

Line 387 will override the settings of line 328.
To line 328, means we do not need to relocate image_data to load, since they are same. So according to line 344, I use same way "fdt_addr = load".

Quoted 9 lines

The idea is to pick up the FDT from inside the image, since the load address indicates that it should not be relocated.
BTW one more thing I noticed:
image_data = (ulong)image_get_data(fdt_hdr);
The cast is confusing, and can be removed.

Yeah. But this patch is to avoid override settings of fdt_blob, line 328 and line 387. This cast can be discarded using another patch.

Thanks,
Peng.

Quoted 3 lines

Regards,
Simon

Simon Glass

2015-12-01 20:01 UTC #7 lore raw

PATCH 1/1 common: image-fdt: correct fdt_blob for IMAGE_FORMAT_LEGACY

Quoted 76 lines · Peng Fan

Hi Simon,
Quoted 35 lines · Simon Glass
Hi Peng,
Quoted 22 lines · Peng Fan
If condition of "(load == image_start || load == image_data)" is true, should use "fdt_addr = load;", but not "fdt_blob = (char *)image_data;", or fdt_blob will be overridden by "fdt_blob = map_sysmem(fdt_addr, 0);" at the end of the switch case.
Signed-off-by Peng Fan <[email protected]>
Cc Simon Glass <[email protected]>
Cc Joe Hershberger <[email protected]>
Cc Max Krummenacher <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Suriyan Ramasami <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Tom Rini <[email protected]>
---
 common/image-fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
common/image-fdt.c +0 -0
index 5180a03..5e4e5bd 100644
common/image-fdt.c int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch, · line 331
Quoted 7 lines
common/image-fdt.c +1 -1
@@ -326,7 +326,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
 
                        if (load == image_start ||
                            load == image_data) {
-                               fdt_blob = (char *)image_data;
+                               fdt_addr = load;
                                break;
                        }
Are you sure that should not be:
Quoted 3 lines
fdt_addr = image_data
?
Just code inspection.
See the following code piece:
319                         image_start = (ulong)fdt_hdr;
320                         image_data = (ulong)image_get_data(fdt_hdr);
321                         image_end = image_get_image_end(fdt_hdr);
322
323 load = image_get_load(fdt_hdr);
324 load_end = load + image_get_data_size(fdt_hdr);
325
326 if (load == image_start ||
327 load == image_data) {
328 fdt_blob = (char *)image_data;
329 break;
330 }
331
332 if ((load < image_end) && (load_end > image_start)) {
333 fdt_error("fdt overwritten");
334 goto error;
335 }
336
337 debug(" Loading FDT from 0x%08lx to 0x%08lx\n",
338 image_data, load);
339
340 memmove((void *)load,
341 (void *)image_data,
342 image_get_data_size(fdt_hdr));
343
344 fdt_addr = load;
345 break;
                                        .........[snip code].........

Quoted 7 lines

386 printf(" Booting using the fdt blob at %#08lx\n", fdt_addr);
387 fdt_blob = map_sysmem(fdt_addr, 0);
Line 387 will override the settings of line 328.
To line 328, means we do not need to relocate image_data to load, since they are same. So according to line 344, I use same way "fdt_addr = load".

OK I see.

Reviewed-by Simon Glass <[email protected]>

Quoted 5 lines

Quoted 5 lines
The idea is to pick up the FDT from inside the image, since the load address indicates that it should not be relocated.

Quoted 9 lines

Quoted 5 lines
BTW one more thing I noticed:
image_data = (ulong)image_get_data(fdt_hdr);
The cast is confusing, and can be removed.
Yeah. But this patch is to avoid override settings of fdt_blob, line 328 and line 387. This cast can be discarded using another patch.

Yes it should be a separate patch. But since you are in there...

Regards,
Simon

Tom Rini

2015-12-06 22:06 UTC #8 lore raw

[U-Boot] common: image-fdt: correct fdt_blob for IMAGE_FORMAT_LEGACY

In reply to Peng Fan

Quoted 5 lines · Peng Fan

If condition of "(load == image_start || load == image_data)" is true, should use "fdt_addr = load;", but not "fdt_blob = (char *)image_data;", or fdt_blob will be overridden by "fdt_blob = map_sysmem(fdt_addr, 0);" at the end of the switch case.

Quoted 9 lines

Signed-off-by Peng Fan <[email protected]>
Cc Simon Glass <[email protected]>
Cc Joe Hershberger <[email protected]>
Cc Max Krummenacher <[email protected]>
Cc Marek Vasut <[email protected]>
Cc Suriyan Ramasami <[email protected]>
Cc Paul Kocialkowski <[email protected]>
Cc Tom Rini <[email protected]>
Reviewed-by Simon Glass <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151206/4fc71fe3/attachment.sig>

Peng Fan

author

2015-11-24 08:54 UTC #9 lore raw

PATCH 1/1 video: ipu: simplify if else code

In reply to Peng Fan

Simplify if/else code, since if channel equals to MEM_BG_SYNC or MEM_FG_SYNC, we have value 5 for 'dc_chan'.

Signed-off-by Peng Fan <[email protected]>

Cc Anatolij Gustschin <[email protected]>

Cc Stefano Babic <[email protected]>

---
 drivers/video/ipu_disp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

drivers/video/ipu_disp.c +1 -3

index 4faeafb..7a87478 100644
@@ -611,11 +611,9 @@ void ipu_dp_dc_enable(ipu_channel_t channel)
 	uint32_t reg;
 	uint32_t dc_chan;
 
-	if (channel == MEM_FG_SYNC)
-		dc_chan = 5;
 	if (channel == MEM_DC_SYNC)
 		dc_chan = 1;
-	else if (channel == MEM_BG_SYNC)
+	else if ((channel == MEM_BG_SYNC) || (channel == MEM_FG_SYNC))
 		dc_chan = 5;
 	else
 		return;

Signature · 1 line

2.6.2

Stefano Babic

2015-11-24 09:55 UTC #10 lore raw

PATCH 1/1 video: ipu: simplify if else code

Quoted 18 lines · Peng Fan

Simplify if/else code, since if channel equals to MEM_BG_SYNC or MEM_FG_SYNC, we have value 5 for 'dc_chan'.
Signed-off-by Peng Fan <[email protected]>
Cc Anatolij Gustschin <[email protected]>
Cc Stefano Babic <[email protected]>
---
 drivers/video/ipu_disp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/video/ipu_disp.c +0 -0
index 4faeafb..7a87478 100644
@@ -611,11 +611,9 @@ void ipu_dp_dc_enable(ipu_channel_t channel)
 	uint32_t reg;
 	uint32_t dc_chan;

drivers/video/ipu_disp.c void ipu_dp_dc_enable(ipu_channel_t channel) · line 619

Quoted 10 lines

drivers/video/ipu_disp.c +1 -3

 
-	if (channel == MEM_FG_SYNC)
-		dc_chan = 5;
 	if (channel == MEM_DC_SYNC)
 		dc_chan = 1;
-	else if (channel == MEM_BG_SYNC)
+	else if ((channel == MEM_BG_SYNC) || (channel == MEM_FG_SYNC))
 		dc_chan = 5;
 	else
 		return;

Reviewed-by Stefano Babic <[email protected]>

Best regards,
Stefano Babic

Signature · 5 lines

=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

Simon Glass

2015-11-24 19:04 UTC #11 lore raw

PATCH 1/1 common: bootm: check return value of strict_strtoul

In reply to Peng Fan

Quoted 10 lines · Peng Fan

Before continue, check return value of strict_strtoul.
Signed-off-by Peng Fan <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jan Kiszka <[email protected]>
Cc Joe Hershberger <[email protected]>
Cc Hans de Goede <[email protected]>
Cc York Sun <[email protected]>
Cc Tom Rini <[email protected]>

Quoted 3 lines

---
 arch/arm/lib/bootm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by Simon Glass <[email protected]>

Tom Rini

2015-12-06 22:05 UTC #12 lore raw

In reply to Peng Fan

Quoted 11 lines · Peng Fan

Before continue, check return value of strict_strtoul.
Signed-off-by Peng Fan <[email protected]>
Cc Albert Aribaud <[email protected]>
Cc Simon Glass <[email protected]>
Cc Jan Kiszka <[email protected]>
Cc Joe Hershberger <[email protected]>
Cc Hans de Goede <[email protected]>
Cc York Sun <[email protected]>
Cc Tom Rini <[email protected]>
Reviewed-by Simon Glass <[email protected]>

Applied to u-boot/master, thanks!

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151206/196ef3c5/attachment.sig>