[U-Boot] [PATCH] common: bootm: check return value of strict_strtoul

[U-Boot] [PATCH] common: bootm: check return value of strict_strtoul

[Peng Fan]

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);
 	}
 
-- 
2.6.2

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

[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.

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;
 	}
-- 
2.6.2

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

[Simon Glass]

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

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

[Tom Rini]

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!

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

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

[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 <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;
 			}
 
-- 
2.6.2

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

[Simon Glass]

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

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

[Peng Fan]

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

-- 

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

[Simon Glass]

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

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

[Tom Rini]

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!

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

[U-Boot] [PATCH] video: ipu: simplify if else code

[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 <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;
-- 
2.6.2

[U-Boot] [PATCH] video: ipu: simplify if else code

[Stefano Babic]

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;
>  	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 <sbabic@denx.de>

Best regards,
Stefano Babic

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

[U-Boot] [PATCH] common: bootm: check return value of strict_strtoul

[Simon Glass]

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>

[U-Boot] common: bootm: check return value of strict_strtoul

[Tom Rini]

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!

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