image: fix bootm failure for FIT image

PATCH New Bryan Wu 4 participants 9 messages Aug 15, 2014

Share & tools

Bryan Wu

author

2014-08-15 17:49 UTC #0 lore raw

PATCH 1/1 image: fix bootm failure for FIT image

Commit b3dd64f5d537b41fc52a03e697271774891f4a70 introduced a bug for booting FIT image. It's because calling fit_parse_config() twice will give us wrong value in img_addr.

Add a new version for CONFIG_FIT of genimg_get_kernel_addr() and return fit_uname_config and fit_uname_kernel for CONFIG_FIT.

Reported-by York Sun <[email protected]>

Signed-off-by Bryan Wu <[email protected]>

---
 common/bootm.c               |  9 +++++----
 common/cmd_pxe.c             |  9 +++++++++
 common/image.c               | 19 ++++++++++---------
 include/configs/jetson-tk1.h |  2 ++
 include/image.h              |  6 ++++++
 5 files changed, 32 insertions(+), 13 deletions(-)

common/bootm.c +5 -4

index 76d811c..85b71ba 100644
@@ -731,7 +731,12 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
 	int		os_noffset;
 #endif
 
+#if defined(CONFIG_FIT)
+	img_addr = genimg_get_kernel_addr(argv[0], &fit_uname_config,
+					  &fit_uname_kernel);
+#else
 	img_addr = genimg_get_kernel_addr(argv[0]);
+#endif
 
 	bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC);

@@ -788,10 +793,6 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc,
 #endif
 #if defined(CONFIG_FIT)
 	case IMAGE_FORMAT_FIT:
-		if (!fit_parse_conf(argv[0], load_addr, &img_addr,
-					&fit_uname_config))
-			fit_parse_subimage(argv[0], load_addr, &img_addr,
-					&fit_uname_kernel);
 		os_noffset = fit_image_load(images, img_addr,
 				&fit_uname_kernel, &fit_uname_config,
 				IH_ARCH_DEFAULT, IH_TYPE_KERNEL,

common/cmd_pxe.c +9 -0

index c816339..9a2c370 100644
@@ -612,6 +612,10 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
 	int len = 0;
 	ulong kernel_addr;
 	void *buf;
+#if defined(CONFIG_FIT)
+	const char *fit_uname_config = NULL;
+	const char *fit_uname_kernel = NULL;
+#endif
 
 	label_print(label);

@@ -774,7 +778,12 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
 	if (bootm_argv[3])
 		bootm_argc = 4;
 
+#if defined(CONFIG_FIT)
+	kernel_addr = genimg_get_kernel_addr(bootm_argv[1], &fit_uname_config,
+					     &fit_uname_kernel);
+#else
 	kernel_addr = genimg_get_kernel_addr(bootm_argv[1]);
+#endif
 	buf = map_sysmem(kernel_addr, 0);
 	/* Try bootm for legacy and FIT format image */
 	if (genimg_get_format(buf) != IMAGE_FORMAT_INVALID)

common/image.c +10 -9

index a2999c0..ea980cb 100644
@@ -652,13 +652,14 @@ int genimg_get_comp_id(const char *name)
  * returns:
  *     kernel start address
  */
-ulong genimg_get_kernel_addr(char * const img_addr)
-{
 #if defined(CONFIG_FIT)
-	const char	*fit_uname_config = NULL;
-	const char	*fit_uname_kernel = NULL;
+ulong genimg_get_kernel_addr(char * const img_addr,
+			     const char **fit_uname_config,
+			     const char **fit_uname_kernel)
+#else
+ulong genimg_get_kernel_addr(char * const img_addr)
 #endif
-
+{
 	ulong kernel_addr;
 
 	/* find out kernel image address */

@@ -668,13 +669,13 @@ ulong genimg_get_kernel_addr(char * const img_addr)
 		      load_addr);
 #if defined(CONFIG_FIT)
 	} else if (fit_parse_conf(img_addr, load_addr, &kernel_addr,
-				  &fit_uname_config)) {
+				  fit_uname_config)) {
 		debug("*  kernel: config '%s' from image at 0x%08lx\n",
-		      fit_uname_config, kernel_addr);
+		      *fit_uname_config, kernel_addr);
 	} else if (fit_parse_subimage(img_addr, load_addr, &kernel_addr,
-				     &fit_uname_kernel)) {
+				     fit_uname_kernel)) {
 		debug("*  kernel: subimage '%s' from image at 0x%08lx\n",
-		      fit_uname_kernel, kernel_addr);
+		      *fit_uname_kernel, kernel_addr);
 #endif
 	} else {
 		kernel_addr = simple_strtoul(img_addr, NULL, 16);

include/configs/jetson-tk1.h +2 -0

index 68b6fb6..b68d58e 100644
@@ -27,6 +27,8 @@
 #define CONFIG_OF_LIBFDT
 #define CONFIG_OF_BOARD_SETUP
 
+#define CONFIG_FIT
+
 #define CONFIG_SERIAL_TAG
 #define CONFIG_TEGRA_SERIAL_HIGH	0x01770000
 #define CONFIG_TEGRA_SERIAL_LOW		0x034200FF

include/image.h +6 -0

index ca2fe86..a47c146 100644
@@ -424,7 +424,13 @@ enum fit_load_op {
 #define IMAGE_FORMAT_FIT	0x02	/* new, libfdt based format */
 #define IMAGE_FORMAT_ANDROID	0x03	/* Android boot image */
 
+#if defined(CONFIG_FIT)
+ulong genimg_get_kernel_addr(char * const img_addr,
+			     const char	**fit_uname_config,
+			     const char	**fit_uname_kernel);
+#else
 ulong genimg_get_kernel_addr(char * const img_addr);
+#endif
 int genimg_get_format(const void *img_addr);
 int genimg_has_config(bootm_headers_t *images);
 ulong genimg_get_image(ulong img_addr);

Signature · 1 line

1.9.1

York Sun

2014-08-15 17:53 UTC #2 lore raw

PATCH 1/1 image: fix bootm failure for FIT image

Quoted 4 lines · Bryan Wu

York,
Could you please to try this patch on your platform? I tested on mine but it's not the same as your setup.

I am doing it now.

York

York Sun

2014-08-15 18:06 UTC #3 lore raw

PATCH 1/1 image: fix bootm failure for FIT image

In reply to Bryan Wu

Quoted 4 lines · Bryan Wu

York,
Could you please to try this patch on your platform? I tested on mine but it's not the same as your setup.

Bryan,

I did a quick test and this patch fixed the problem I had. Please continue code review process.

I noticed a change in include/configs/jetson-tk1.h. Does it have anything to do with this patch?

York

Bryan Wu

author

2014-08-15 18:09 UTC #4 lore raw

PATCH 1/1 image: fix bootm failure for FIT image

Quoted 6 lines · York Sun

Quoted 4 lines · Bryan Wu
York,
Could you please to try this patch on your platform? I tested on mine but it's not the same as your setup.

Quoted 8 lines

Bryan,
I did a quick test and this patch fixed the problem I had. Please continue code review process.
I noticed a change in include/configs/jetson-tk1.h. Does it have anything to do with this patch?

Yeah, I forget to remove that change after I tested CONFIG_FIT.

I have more clean up patches after this one and looks like Tom merged my old version patch instead the one I updated according to Simon's review.

Thanks,
-Bryan

Stephen Warren

2014-08-15 18:04 UTC #5 lore raw

PATCH 1/1 image: fix bootm failure for FIT image

In reply to Bryan Wu

Quoted 1 line · Bryan Wu

Commit b3dd64f5d537b41fc52a03e697271774891f4a70 introduced a bug for

Can we spell out the commit description too, so it's easier to know what it refers too, and is useful if someone cherry-picks the commit so the commit ID changes:

Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" introduced...

Quoted 9 lines

booting FIT image. It's because calling fit_parse_config() twice will give us wrong value in img_addr.
Add a new version for CONFIG_FIT of genimg_get_kernel_addr() and return fit_uname_config and fit_uname_kernel for CONFIG_FIT.
common/image.c +0 -1
 
-ulong genimg_get_kernel_addr(char * const img_addr)

common/image.c int genimg_get_comp_id(const char *name) · line 661

Quoted 10 lines

common/image.c +5 -3

-{
  #if defined(CONFIG_FIT)
-	const char	*fit_uname_config = NULL;
-	const char	*fit_uname_kernel = NULL;
+ulong genimg_get_kernel_addr(char * const img_addr,
+			     const char **fit_uname_config,
+			     const char **fit_uname_kernel)
+#else
+ulong genimg_get_kernel_addr(char * const img_addr)
  #endif

Indentation looks wrong on that #endif.

Wouldn't it be better to avoid repeating the common parts, so this instead:

ulong genimg_get_kernel_addr(char * const img_addr, #if defined(CONFIG_FIT)
const char **fit_uname_config, const char **fit_uname_kernel)
#endif
)
{

include/configs/jetson-tk1.h line 30

Quoted 3 lines

include/configs/jetson-tk1.h +1 -0
 
+#define CONFIG_FIT

I think that crept in by mistake.

include/image.h enum fit_load_op { · line 433

Quoted 10 lines

include/image.h +6 -0

index ca2fe86..a47c146 100644
 
+#if defined(CONFIG_FIT)
+ulong genimg_get_kernel_addr(char * const img_addr,
+			     const char	**fit_uname_config,
+			     const char	**fit_uname_kernel);
+#else
  ulong genimg_get_kernel_addr(char * const img_addr);
+#endif

Same comment about #ifdef placement here.

Bryan Wu

author

2014-08-15 18:25 UTC #6 lore raw

PATCH 1/1 image: fix bootm failure for FIT image

Quoted 39 lines · Stephen Warren

Quoted 2 lines · Bryan Wu
Commit b3dd64f5d537b41fc52a03e697271774891f4a70 introduced a bug for
Can we spell out the commit description too, so it's easier to know what it refers too, and is useful if someone cherry-picks the commit so the commit ID changes:
Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" introduced...
Quoted 11 lines
booting FIT image. It's because calling fit_parse_config() twice will give us wrong value in img_addr.
Add a new version for CONFIG_FIT of genimg_get_kernel_addr() and return fit_uname_config and fit_uname_kernel for CONFIG_FIT.
common/image.c +0 -1
 
 
-ulong genimg_get_kernel_addr(char * const img_addr)
common/image.c int genimg_get_comp_id(const char *name) · line 661
Quoted 10 lines
common/image.c +5 -3
-{
  #if defined(CONFIG_FIT)
-       const char      *fit_uname_config = NULL;
-       const char      *fit_uname_kernel = NULL;
+ulong genimg_get_kernel_addr(char * const img_addr,
+                            const char **fit_uname_config,
+                            const char **fit_uname_kernel)
+#else
+ulong genimg_get_kernel_addr(char * const img_addr)
  #endif
Indentation looks wrong on that #endif.
Wouldn't it be better to avoid repeating the common parts, so this instead:

Quoted 8 lines

ulong genimg_get_kernel_addr(char * const img_addr, #if defined(CONFIG_FIT)
const char **fit_uname_config, const char **fit_uname_kernel)
#endif
)
{

I got compiling error like this when I try this method then I moved to 2 current one:

---
ulong genimg_get_kernel_addr(char * const img_addr,
#if defined(CONFIG_FIT)
const char **fit_uname_config, const char **fit_uname_kernel
#endif
);
---
include/image.h:432:1: error: expected declaration specifiers or ?...? before ?)? token
);
^

But if I use this:

----
ulong genimg_get_kernel_addr(char * const img_addr #if defined(CONFIG_FIT)
, const char **fit_uname_config, const char **fit_uname_kernel
#endif
);
----
Then compiler is happy, but the style looks weird to me.

This is for declaration but definition has the same problem.

-Bryan

Quoted 23 lines

include/configs/jetson-tk1.h line 30
Quoted 4 lines
include/configs/jetson-tk1.h +1 -0
 
 
+#define CONFIG_FIT
I think that crept in by mistake.
include/image.h enum fit_load_op { · line 433
Quoted 11 lines
include/image.h +6 -0
index ca2fe86..a47c146 100644
 
 
+#if defined(CONFIG_FIT)
+ulong genimg_get_kernel_addr(char * const img_addr,
+                            const char **fit_uname_config,
+                            const char **fit_uname_kernel);
+#else
  ulong genimg_get_kernel_addr(char * const img_addr);
+#endif
Same comment about #ifdef placement here.

Simon Glass

2014-08-15 19:33 UTC #7 lore raw

PATCH 1/1 image: fix bootm failure for FIT image

Hi,

Quoted 23 lines · Bryan Wu

Quoted 22 lines · Stephen Warren
Quoted 2 lines · Bryan Wu
Commit b3dd64f5d537b41fc52a03e697271774891f4a70 introduced a bug for
Can we spell out the commit description too, so it's easier to know what it refers too, and is useful if someone cherry-picks the commit so the commit ID changes:
Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" introduced...
Quoted 8 lines
booting FIT image. It's because calling fit_parse_config() twice will give us wrong value in img_addr.
Add a new version for CONFIG_FIT of genimg_get_kernel_addr() and return fit_uname_config and fit_uname_kernel for CONFIG_FIT.
common/image.c +0 -0

Quoted 9 lines

common/image.c int genimg_get_comp_id(const char *name) · line 659

Quoted 9 lines

common/image.c +4 -4

-ulong genimg_get_kernel_addr(char * const img_addr)
-{
  #if defined(CONFIG_FIT)
-       const char      *fit_uname_config = NULL;
-       const char      *fit_uname_kernel = NULL;
+ulong genimg_get_kernel_addr(char * const img_addr,
+                            const char **fit_uname_config,
+                            const char **fit_uname_kernel)
+#else

Let's avoid these #ifdefs inside a function signature, just too ugly. If the extra arguments are a big problem you could create a new function perhaps?

Also, how about updating test/image/fit.py to test this behaviour? Then we can catch the bug.

Quoted 45 lines

Quoted 8 lines
Quoted 2 lines
+ulong genimg_get_kernel_addr(char * const img_addr)
#endif
Indentation looks wrong on that #endif.
Wouldn't it be better to avoid repeating the common parts, so this instead:
Quoted 8 lines
ulong genimg_get_kernel_addr(char * const img_addr, #if defined(CONFIG_FIT)
const char **fit_uname_config, const char **fit_uname_kernel)
#endif
)
{
I got compiling error like this when I try this method then I moved to 2 current one:
---
ulong genimg_get_kernel_addr(char * const img_addr,
#if defined(CONFIG_FIT)
const char **fit_uname_config, const char **fit_uname_kernel
#endif
);
---
include/image.h:432:1: error: expected declaration specifiers or ?...? before ?)? token
);
^
But if I use this:
----
ulong genimg_get_kernel_addr(char * const img_addr #if defined(CONFIG_FIT)
, const char **fit_uname_config, const char **fit_uname_kernel
#endif
);
----
Then compiler is happy, but the style looks weird to me.

Quoted 28 lines

This is for declaration but definition has the same problem.
-Bryan
Quoted 23 lines
include/configs/jetson-tk1.h line 30
Quoted 4 lines
include/configs/jetson-tk1.h +1 -0
 
 
+#define CONFIG_FIT
I think that crept in by mistake.
include/image.h enum fit_load_op { · line 433
Quoted 11 lines
include/image.h +6 -0
index ca2fe86..a47c146 100644
 
 
+#if defined(CONFIG_FIT)
+ulong genimg_get_kernel_addr(char * const img_addr,
+                            const char **fit_uname_config,
+                            const char **fit_uname_kernel);
+#else
  ulong genimg_get_kernel_addr(char * const img_addr);
+#endif
Same comment about #ifdef placement here.

Regards,
Simon

Bryan Wu

author

2014-08-15 23:17 UTC #8 lore raw

PATCH 1/1 image: fix bootm failure for FIT image

Quoted 26 lines · Simon Glass

Hi,
Quoted 23 lines · Bryan Wu
Quoted 22 lines · Stephen Warren
Quoted 2 lines · Bryan Wu
Commit b3dd64f5d537b41fc52a03e697271774891f4a70 introduced a bug for
Can we spell out the commit description too, so it's easier to know what it refers too, and is useful if someone cherry-picks the commit so the commit ID changes:
Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" introduced...
Quoted 8 lines
booting FIT image. It's because calling fit_parse_config() twice will give us wrong value in img_addr.
Add a new version for CONFIG_FIT of genimg_get_kernel_addr() and return fit_uname_config and fit_uname_kernel for CONFIG_FIT.
common/image.c +0 -0

Quoted 14 lines

Quoted 9 lines
Quoted 9 lines
common/image.c int genimg_get_comp_id(const char *name) · line 659
Quoted 9 lines
common/image.c +4 -4
-ulong genimg_get_kernel_addr(char * const img_addr)
-{
  #if defined(CONFIG_FIT)
-       const char      *fit_uname_config = NULL;
-       const char      *fit_uname_kernel = NULL;
+ulong genimg_get_kernel_addr(char * const img_addr,
+                            const char **fit_uname_config,
+                            const char **fit_uname_kernel)
+#else
Let's avoid these #ifdefs inside a function signature, just too ugly. If the extra arguments are a big problem you could create a new function perhaps?

OK, I will try to define a new function.

Quoted 3 lines

Also, how about updating test/image/fit.py to test this behaviour? Then we can catch the bug.

York, I might need your information to add some test in the test/image/fit.py. So a) you download a FIT image to somewhere
b) what's kind of FIT image you're testing? how many configs?
c) and you don't have load_addr?

-Bryan

Quoted 76 lines

Quoted 45 lines
Quoted 8 lines
Quoted 2 lines
+ulong genimg_get_kernel_addr(char * const img_addr)
#endif
Indentation looks wrong on that #endif.
Wouldn't it be better to avoid repeating the common parts, so this instead:
Quoted 8 lines
ulong genimg_get_kernel_addr(char * const img_addr, #if defined(CONFIG_FIT)
const char **fit_uname_config, const char **fit_uname_kernel)
#endif
)
{
I got compiling error like this when I try this method then I moved to 2 current one:
---
ulong genimg_get_kernel_addr(char * const img_addr,
#if defined(CONFIG_FIT)
const char **fit_uname_config, const char **fit_uname_kernel
#endif
);
---
include/image.h:432:1: error: expected declaration specifiers or ?...? before ?)? token
);
^
But if I use this:
----
ulong genimg_get_kernel_addr(char * const img_addr #if defined(CONFIG_FIT)
, const char **fit_uname_config, const char **fit_uname_kernel
#endif
);
----
Then compiler is happy, but the style looks weird to me.
Quoted 28 lines
This is for declaration but definition has the same problem.
-Bryan
Quoted 23 lines
include/configs/jetson-tk1.h line 30
Quoted 4 lines
include/configs/jetson-tk1.h +1 -0
 
 
+#define CONFIG_FIT
I think that crept in by mistake.
include/image.h enum fit_load_op { · line 433
Quoted 11 lines
include/image.h +6 -0
index ca2fe86..a47c146 100644
 
 
+#if defined(CONFIG_FIT)
+ulong genimg_get_kernel_addr(char * const img_addr,
+                            const char **fit_uname_config,
+                            const char **fit_uname_kernel);
+#else
  ulong genimg_get_kernel_addr(char * const img_addr);
+#endif
Same comment about #ifdef placement here.
Regards,
Simon