[U-Boot] [PATCH] common: bootm: check return value of strict_strtoul
Before continue, check return value of strict_strtoul.
Signed-off-by: Peng Fan Peng.Fan@freescale.com Cc: Albert Aribaud albert.u.boot@aribaud.net Cc: Simon Glass sjg@chromium.org Cc: Jan Kiszka jan.kiszka@siemens.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Hans de Goede hdegoede@redhat.com Cc: York Sun yorksun@freescale.com Cc: Tom Rini trini@konsulko.com --- arch/arm/lib/bootm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c index ee56d74..a477cae 100644 --- a/arch/arm/lib/bootm.c +++ b/arch/arm/lib/bootm.c @@ -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); }
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 Peng.Fan@freescale.com Cc: Rabin Vincent rabin@rab.in Cc: Simon Glass sjg@chromium.org Cc: Tom Rini trini@konsulko.com --- common/cli_hush.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/cli_hush.c b/common/cli_hush.c index a7cac4f..f075459 100644 --- a/common/cli_hush.c +++ b/common/cli_hush.c @@ -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; }
On 24 November 2015 at 01:54, Peng Fan Peng.Fan@freescale.com wrote:
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 Peng.Fan@freescale.com Cc: Rabin Vincent rabin@rab.in Cc: Simon Glass sjg@chromium.org Cc: Tom Rini trini@konsulko.com
common/cli_hush.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
diff --git a/common/cli_hush.c b/common/cli_hush.c index a7cac4f..f075459 100644 --- a/common/cli_hush.c +++ b/common/cli_hush.c @@ -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) {
I suggest:
if (!value || !value[1])
but please feel free to ignore this. What you have will work.
free(name); return -1; }-- 2.6.2
On Tue, Nov 24, 2015 at 04:54:21PM +0800, Peng Fan wrote:
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 Peng.Fan@freescale.com Cc: Rabin Vincent rabin@rab.in Cc: Simon Glass sjg@chromium.org Cc: Tom Rini trini@konsulko.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
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 Peng.Fan@freescale.com Cc: Simon Glass sjg@chromium.org Cc: Joe Hershberger joe.hershberger@ni.com Cc: Max Krummenacher max.krummenacher@toradex.com Cc: Marek Vasut marex@denx.de Cc: Suriyan Ramasami suriyan.r@gmail.com Cc: Paul Kocialkowski contact@paulk.fr Cc: Tom Rini trini@konsulko.com --- common/image-fdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/image-fdt.c b/common/image-fdt.c index 5180a03..5e4e5bd 100644 --- a/common/image-fdt.c +++ b/common/image-fdt.c @@ -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; }
Hi Peng,
On 24 November 2015 at 01:54, Peng Fan Peng.Fan@freescale.com wrote:
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 Peng.Fan@freescale.com Cc: Simon Glass sjg@chromium.org Cc: Joe Hershberger joe.hershberger@ni.com Cc: Max Krummenacher max.krummenacher@toradex.com Cc: Marek Vasut marex@denx.de Cc: Suriyan Ramasami suriyan.r@gmail.com Cc: Paul Kocialkowski contact@paulk.fr Cc: Tom Rini trini@konsulko.com
common/image-fdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/image-fdt.c b/common/image-fdt.c index 5180a03..5e4e5bd 100644 --- a/common/image-fdt.c +++ b/common/image-fdt.c @@ -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
Hi Simon, On Tue, Nov 24, 2015 at 12:04:56PM -0700, Simon Glass wrote:
Hi Peng,
On 24 November 2015 at 01:54, Peng Fan Peng.Fan@freescale.com wrote:
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 Peng.Fan@freescale.com Cc: Simon Glass sjg@chromium.org Cc: Joe Hershberger joe.hershberger@ni.com Cc: Max Krummenacher max.krummenacher@toradex.com Cc: Marek Vasut marex@denx.de Cc: Suriyan Ramasami suriyan.r@gmail.com Cc: Paul Kocialkowski contact@paulk.fr Cc: Tom Rini trini@konsulko.com
common/image-fdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/image-fdt.c b/common/image-fdt.c index 5180a03..5e4e5bd 100644 --- a/common/image-fdt.c +++ b/common/image-fdt.c @@ -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
?
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".
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.
Regards, Simon
--
On 24 November 2015 at 18:12, Peng Fan b51431@freescale.com wrote:
Hi Simon, On Tue, Nov 24, 2015 at 12:04:56PM -0700, Simon Glass wrote:
Hi Peng,
On 24 November 2015 at 01:54, Peng Fan Peng.Fan@freescale.com wrote:
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 Peng.Fan@freescale.com Cc: Simon Glass sjg@chromium.org Cc: Joe Hershberger joe.hershberger@ni.com Cc: Max Krummenacher max.krummenacher@toradex.com Cc: Marek Vasut marex@denx.de Cc: Suriyan Ramasami suriyan.r@gmail.com Cc: Paul Kocialkowski contact@paulk.fr Cc: Tom Rini trini@konsulko.com
common/image-fdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/image-fdt.c b/common/image-fdt.c index 5180a03..5e4e5bd 100644 --- a/common/image-fdt.c +++ b/common/image-fdt.c @@ -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
?
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".
OK I see.
Reviewed-by: Simon Glass sjg@chromium.org
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.
Yes it should be a separate patch. But since you are in there...
Regards, Simon
On Tue, Nov 24, 2015 at 04:54:22PM +0800, Peng Fan wrote:
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 Peng.Fan@freescale.com Cc: Simon Glass sjg@chromium.org Cc: Joe Hershberger joe.hershberger@ni.com Cc: Max Krummenacher max.krummenacher@toradex.com Cc: Marek Vasut marex@denx.de Cc: Suriyan Ramasami suriyan.r@gmail.com Cc: Paul Kocialkowski contact@paulk.fr Cc: Tom Rini trini@konsulko.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
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 Peng.Fan@freescale.com Cc: Anatolij Gustschin agust@denx.de Cc: Stefano Babic sbabic@denx.de --- drivers/video/ipu_disp.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/video/ipu_disp.c b/drivers/video/ipu_disp.c index 4faeafb..7a87478 100644 --- a/drivers/video/ipu_disp.c +++ b/drivers/video/ipu_disp.c @@ -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;
On 24/11/2015 09:54, Peng Fan wrote:
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 Peng.Fan@freescale.com Cc: Anatolij Gustschin agust@denx.de Cc: Stefano Babic sbabic@denx.de
drivers/video/ipu_disp.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/video/ipu_disp.c b/drivers/video/ipu_disp.c index 4faeafb..7a87478 100644 --- a/drivers/video/ipu_disp.c +++ b/drivers/video/ipu_disp.c @@ -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;- 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 sbabic@denx.de
Best regards, Stefano Babic
On 24 November 2015 at 01:54, Peng Fan Peng.Fan@freescale.com wrote:
Before continue, check return value of strict_strtoul.
Signed-off-by: Peng Fan Peng.Fan@freescale.com Cc: Albert Aribaud albert.u.boot@aribaud.net Cc: Simon Glass sjg@chromium.org Cc: Jan Kiszka jan.kiszka@siemens.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Hans de Goede hdegoede@redhat.com Cc: York Sun yorksun@freescale.com Cc: Tom Rini trini@konsulko.com
arch/arm/lib/bootm.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Tue, Nov 24, 2015 at 04:54:20PM +0800, Peng Fan wrote:
Before continue, check return value of strict_strtoul.
Signed-off-by: Peng Fan Peng.Fan@freescale.com Cc: Albert Aribaud albert.u.boot@aribaud.net Cc: Simon Glass sjg@chromium.org Cc: Jan Kiszka jan.kiszka@siemens.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Hans de Goede hdegoede@redhat.com Cc: York Sun yorksun@freescale.com Cc: Tom Rini trini@konsulko.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
participants (5)
-
Peng Fan -
Peng Fan -
Simon Glass -
Stefano Babic -
Tom Rini