| Message ID | 1408146699-27959-1-git-send-email-pengw@nvidia.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
Hi Simon, Tom and Stephen, Could you guys review this patch which solved the issue York reported? Thanks, -Bryan On Fri, Aug 15, 2014 at 4:51 PM, Bryan Wu <cooloney@gmail.com> wrote: > Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" 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 function genimg_get_kernel_addr_fit() whichl will always > return fit_uname_config and fit_uname_kernel for CONFIG_FIT. > genimg_get_kernel_addr() will ignore those to parameters. > > Reported-by: York Sun <yorksun@freescale.com> > Signed-off-by: Bryan Wu <pengw@nvidia.com> > --- > common/bootm.c | 9 +++------ > common/image.c | 39 +++++++++++++++++++++++++++------------ > include/image.h | 3 +++ > 3 files changed, 33 insertions(+), 18 deletions(-) > > diff --git a/common/bootm.c b/common/bootm.c > index 76d811c..245c82a 100644 > --- a/common/bootm.c > +++ b/common/bootm.c > @@ -725,13 +725,14 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc, > #endif > ulong img_addr; > const void *buf; > -#if defined(CONFIG_FIT) > const char *fit_uname_config = NULL; > const char *fit_uname_kernel = NULL; > +#if defined(CONFIG_FIT) > int os_noffset; > #endif > > - img_addr = genimg_get_kernel_addr(argv[0]); > + img_addr = genimg_get_kernel_addr_fit(argv[0], &fit_uname_config, > + &fit_uname_kernel); > > bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC); > > @@ -788,10 +789,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, > diff --git a/common/image.c b/common/image.c > index a2999c0..d4ccff0 100644 > --- a/common/image.c > +++ b/common/image.c > @@ -643,22 +643,24 @@ int genimg_get_comp_id(const char *name) > > #ifndef USE_HOSTCC > /** > - * genimg_get_kernel_addr - get the real kernel address > + * genimg_get_kernel_addr_fit - get the real kernel address and return 2 > + * FIT strings > * @img_addr: a string might contain real image address > + * @fit_uname_config: double pointer to a char, will hold pointer to a > + * configuration unit name > + * @fit_uname_kernel: double pointer to a char, will hold pointer to a subimage > + * name > * > - * genimg_get_kernel_addr() get the real kernel start address from a string > + * genimg_get_kernel_addr_fit get the real kernel start address from a string > * which is normally the first argv of bootm/bootz > * > * returns: > * kernel start address > */ > -ulong genimg_get_kernel_addr(char * const img_addr) > +ulong genimg_get_kernel_addr_fit(char * const img_addr, > + const char **fit_uname_config, > + const char **fit_uname_kernel) > { > -#if defined(CONFIG_FIT) > - const char *fit_uname_config = NULL; > - const char *fit_uname_kernel = NULL; > -#endif > - > ulong kernel_addr; > > /* find out kernel image address */ > @@ -668,13 +670,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); > @@ -686,6 +688,19 @@ ulong genimg_get_kernel_addr(char * const img_addr) > } > > /** > + * genimg_get_kernel_addr() is the simple version of > + * genimg_get_kernel_addr_fit(). It ignores those return FIT strings > + */ > +ulong genimg_get_kernel_addr(char * const img_addr) > +{ > + const char *fit_uname_config = NULL; > + const char *fit_uname_kernel = NULL; > + > + return genimg_get_kernel_addr_fit(img_addr, &fit_uname_config, > + &fit_uname_kernel); > +} > + > +/** > * genimg_get_format - get image format type > * @img_addr: image start address > * > diff --git a/include/image.h b/include/image.h > index ca2fe86..69f86ad 100644 > --- a/include/image.h > +++ b/include/image.h > @@ -424,6 +424,9 @@ enum fit_load_op { > #define IMAGE_FORMAT_FIT 0x02 /* new, libfdt based format */ > #define IMAGE_FORMAT_ANDROID 0x03 /* Android boot image */ > > +ulong genimg_get_kernel_addr_fit(char * const img_addr, > + const char **fit_uname_config, > + const char **fit_uname_kernel); > ulong genimg_get_kernel_addr(char * const img_addr); > int genimg_get_format(const void *img_addr); > int genimg_has_config(bootm_headers_t *images); > -- > 1.9.1 >
Hi Bryan, On 20 August 2014 11:24, Bryan Wu <cooloney@gmail.com> wrote: > Hi Simon, Tom and Stephen, > > Could you guys review this patch which solved the issue York reported? > Sorry, busy week, will get to it soon though. Regards, Simon
Hi Bryan, On 15 August 2014 17:51, Bryan Wu <cooloney@gmail.com> wrote: > > Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" 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 function genimg_get_kernel_addr_fit() whichl will always > return fit_uname_config and fit_uname_kernel for CONFIG_FIT. > genimg_get_kernel_addr() will ignore those to parameters. > > Reported-by: York Sun <yorksun@freescale.com> > Signed-off-by: Bryan Wu <pengw@nvidia.com> I think the function comment should be in the header file, but it's not important for now. The image.h file has a whitespace problem - tabs instead of spaces. If you run patman you will see it. Unfortunately the tests still fail due to two other issues. I'll send a few patches. Regards, Simon
On Fri, Aug 22, 2014 at 02:17:53PM -0600, Simon Glass wrote: > Hi Bryan, > > On 15 August 2014 17:51, Bryan Wu <cooloney@gmail.com> wrote: > > > > Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" 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 function genimg_get_kernel_addr_fit() whichl will always > > return fit_uname_config and fit_uname_kernel for CONFIG_FIT. > > genimg_get_kernel_addr() will ignore those to parameters. > > > > Reported-by: York Sun <yorksun@freescale.com> > > Signed-off-by: Bryan Wu <pengw@nvidia.com> > > I think the function comment should be in the header file, but it's > not important for now. > > The image.h file has a whitespace problem - tabs instead of spaces. If > you run patman you will see it. > > Unfortunately the tests still fail due to two other issues. I'll send > a few patches. ... but we're believing things are all fixed now, right?
On Fri, Aug 22, 2014 at 05:38:59PM -0400, Tom Rini wrote: > On Fri, Aug 22, 2014 at 02:17:53PM -0600, Simon Glass wrote: > > Hi Bryan, > > > > On 15 August 2014 17:51, Bryan Wu <cooloney@gmail.com> wrote: > > > > > > Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" 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 function genimg_get_kernel_addr_fit() whichl will always > > > return fit_uname_config and fit_uname_kernel for CONFIG_FIT. > > > genimg_get_kernel_addr() will ignore those to parameters. > > > > > > Reported-by: York Sun <yorksun@freescale.com> > > > Signed-off-by: Bryan Wu <pengw@nvidia.com> > > > > I think the function comment should be in the header file, but it's > > not important for now. > > > > The image.h file has a whitespace problem - tabs instead of spaces. If > > you run patman you will see it. > > > > Unfortunately the tests still fail due to two other issues. I'll send > > a few patches. > > ... but we're believing things are all fixed now, right? ... I assume so since I can just 'bootm' a FIT image now and before things hang, so pushing shortly.
Hi Tom, On Aug 22, 2014 3:54 PM, "Tom Rini" <trini@ti.com> wrote: > > On Fri, Aug 22, 2014 at 05:38:59PM -0400, Tom Rini wrote: > > On Fri, Aug 22, 2014 at 02:17:53PM -0600, Simon Glass wrote: > > > Hi Bryan, > > > > > > On 15 August 2014 17:51, Bryan Wu <cooloney@gmail.com> wrote: > > > > > > > > Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" 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 function genimg_get_kernel_addr_fit() whichl will always > > > > return fit_uname_config and fit_uname_kernel for CONFIG_FIT. > > > > genimg_get_kernel_addr() will ignore those to parameters. > > > > > > > > Reported-by: York Sun <yorksun@freescale.com> > > > > Signed-off-by: Bryan Wu <pengw@nvidia.com> > > > > > > I think the function comment should be in the header file, but it's > > > not important for now. > > > > > > The image.h file has a whitespace problem - tabs instead of spaces. If > > > you run patman you will see it. > > > > > > Unfortunately the tests still fail due to two other issues. I'll send > > > a few patches. > > > > ... but we're believing things are all fixed now, right? > > ... I assume so since I can just 'bootm' a FIT image now and before > things hang, so pushing shortly. Yes, also see my two patches. Would like to leave those for discussion for a bit though due to the ramdisk change. Regards, Simon > > -- > Tom
On Fri, Aug 15, 2014 at 04:51:38PM -0700, Bryan Wu wrote: > Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" 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 function genimg_get_kernel_addr_fit() whichl will always > return fit_uname_config and fit_uname_kernel for CONFIG_FIT. > genimg_get_kernel_addr() will ignore those to parameters. > > Reported-by: York Sun <yorksun@freescale.com> > Signed-off-by: Bryan Wu <pengw@nvidia.com> Applied to u-boot/master, thanks!
diff --git a/common/bootm.c b/common/bootm.c index 76d811c..245c82a 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -725,13 +725,14 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int flag, int argc, #endif ulong img_addr; const void *buf; -#if defined(CONFIG_FIT) const char *fit_uname_config = NULL; const char *fit_uname_kernel = NULL; +#if defined(CONFIG_FIT) int os_noffset; #endif - img_addr = genimg_get_kernel_addr(argv[0]); + img_addr = genimg_get_kernel_addr_fit(argv[0], &fit_uname_config, + &fit_uname_kernel); bootstage_mark(BOOTSTAGE_ID_CHECK_MAGIC); @@ -788,10 +789,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, diff --git a/common/image.c b/common/image.c index a2999c0..d4ccff0 100644 --- a/common/image.c +++ b/common/image.c @@ -643,22 +643,24 @@ int genimg_get_comp_id(const char *name) #ifndef USE_HOSTCC /** - * genimg_get_kernel_addr - get the real kernel address + * genimg_get_kernel_addr_fit - get the real kernel address and return 2 + * FIT strings * @img_addr: a string might contain real image address + * @fit_uname_config: double pointer to a char, will hold pointer to a + * configuration unit name + * @fit_uname_kernel: double pointer to a char, will hold pointer to a subimage + * name * - * genimg_get_kernel_addr() get the real kernel start address from a string + * genimg_get_kernel_addr_fit get the real kernel start address from a string * which is normally the first argv of bootm/bootz * * returns: * kernel start address */ -ulong genimg_get_kernel_addr(char * const img_addr) +ulong genimg_get_kernel_addr_fit(char * const img_addr, + const char **fit_uname_config, + const char **fit_uname_kernel) { -#if defined(CONFIG_FIT) - const char *fit_uname_config = NULL; - const char *fit_uname_kernel = NULL; -#endif - ulong kernel_addr; /* find out kernel image address */ @@ -668,13 +670,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); @@ -686,6 +688,19 @@ ulong genimg_get_kernel_addr(char * const img_addr) } /** + * genimg_get_kernel_addr() is the simple version of + * genimg_get_kernel_addr_fit(). It ignores those return FIT strings + */ +ulong genimg_get_kernel_addr(char * const img_addr) +{ + const char *fit_uname_config = NULL; + const char *fit_uname_kernel = NULL; + + return genimg_get_kernel_addr_fit(img_addr, &fit_uname_config, + &fit_uname_kernel); +} + +/** * genimg_get_format - get image format type * @img_addr: image start address * diff --git a/include/image.h b/include/image.h index ca2fe86..69f86ad 100644 --- a/include/image.h +++ b/include/image.h @@ -424,6 +424,9 @@ enum fit_load_op { #define IMAGE_FORMAT_FIT 0x02 /* new, libfdt based format */ #define IMAGE_FORMAT_ANDROID 0x03 /* Android boot image */ +ulong genimg_get_kernel_addr_fit(char * const img_addr, + const char **fit_uname_config, + const char **fit_uname_kernel); ulong genimg_get_kernel_addr(char * const img_addr); int genimg_get_format(const void *img_addr); int genimg_has_config(bootm_headers_t *images);
Commit b3dd64f5d537 "bootm: use genimg_get_kernel_addr()" 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 function genimg_get_kernel_addr_fit() whichl will always return fit_uname_config and fit_uname_kernel for CONFIG_FIT. genimg_get_kernel_addr() will ignore those to parameters. Reported-by: York Sun <yorksun@freescale.com> Signed-off-by: Bryan Wu <pengw@nvidia.com> --- common/bootm.c | 9 +++------ common/image.c | 39 +++++++++++++++++++++++++++------------ include/image.h | 3 +++ 3 files changed, 33 insertions(+), 18 deletions(-)