[U-Boot] [PATCH] image: fix bootm failure for FIT image

[Simon Glass]

Hi,

On 15 August 2014 12:25, Bryan Wu <cooloney at gmail.com> wrote:
> On Fri, Aug 15, 2014 at 11:04 AM, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> On 08/15/2014 11:49 AM, Bryan Wu wrote:
>>>
>>> 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...
>>
>>
>>> 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.
>>
>>
>>> diff --git a/common/image.c b/common/image.c
>>
>>
>>> -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.

>>> +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
>> )
>> {
>>
>
> 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
>
>
>>> diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
>>
>>
>>> +#define CONFIG_FIT
>>
>>
>> I think that crept in by mistake.
>>
>>
>>> diff --git a/include/image.h b/include/image.h
>>> 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