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

[Bryan Wu]

On Fri, Aug 15, 2014 at 12:33 PM, Simon Glass <sjg at chromium.org> wrote:
> 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?
>

OK, I will try to define a new function.

> 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

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