[U-Boot] [PATCH v4 2/2] splash: add support for loading splash from a FIT image

[Igor Grinberg]

On 12/11/16 22:27, Simon Glass wrote:
> Hi Igor,
> 
> On 11 December 2016 at 10:37, Igor Grinberg <grinberg at compulab.co.il> wrote:
>> Hi Tomas, Simon,
>>
>> Sorry, to break in that late...
>> I have a quick question below.
>>
>> On 12/05/16 09:36, Tomas Melin wrote:
>>> Enable support for loading a splash image from within a FIT image.
>>> The image is assumed to be generated with mkimage -E flag to hold
>>> the data external to the FIT.
>>>
>>> Signed-off-by: Tomas Melin <tomas.melin at vaisala.com>
>>
>> [...]
>>
>>> diff --git a/common/splash_source.c b/common/splash_source.c
>>> index 70d724f..94b46d3 100644
>>> --- a/common/splash_source.c
>>> +++ b/common/splash_source.c
>>
>> [...]
>>
>>> +#ifdef CONFIG_FIT
>>> +static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr)
>>> +{
>>> +     int res;
>>> +     int node_offset;
>>> +     int splash_offset;
>>> +     int splash_size;
>>> +     struct image_header *img_header;
>>> +     const u32 *fit_header;
>>> +     u32 fit_size;
>>> +     const size_t header_size = sizeof(struct image_header);
>>> +
>>> +     /* Read in image header */
>>> +     res = splash_storage_read_raw(location, bmp_load_addr, header_size);
>>> +     if (res < 0)
>>> +             return res;
>>> +
>>> +     img_header = (struct image_header *)bmp_load_addr;
>>> +     fit_size = fdt_totalsize(img_header);
>>> +
>>> +     /* Read in entire FIT */
>>> +     fit_header = (const u32 *)(bmp_load_addr + header_size);
>>> +     res = splash_storage_read_raw(location, (u32)fit_header, fit_size);
>>> +     if (res < 0)
>>> +             return res;
>>> +
>>> +     res = fit_check_format(fit_header);
>>> +     if (!res) {
>>> +             debug("Could not find valid FIT image\n");
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     node_offset = fit_image_get_node(fit_header, location->name);
>>> +     if (node_offset < 0) {
>>> +             debug("Could not find splash image '%s' in FIT\n",
>>> +                   location->name);
>>> +             return -ENOENT;
>>> +     }
>>> +
>>
>> I think two above debug() are very legitimate - no need to shout if no FIT image
>> or no splash in it...
>>
>>> +     res = fit_image_get_data_offset(fit_header, node_offset,
>>> +                                     &splash_offset);
>>> +     if (res < 0) {
>>> +             debug("Could not find 'data-offset' property in FIT\n");
>>> +             return res;
>>> +     }
>>> +
>>> +     res = fit_image_get_data_size(fit_header, node_offset, &splash_size);
>>> +     if (res < 0) {
>>> +             debug("Could not find 'data-size' property in FIT\n");
>>> +             return res;
>>> +     }
>>
>> Now regarding these two, I'm not sure.
>> Since we have found a valid FIT and also a node with a correct splash name,
>> probably the intent is that we show the splash, right?
>> But in the two above checks, we find inconsistencies that do not allow us to
>> show the splash - meaning the FIT is not actually good (am I right here?).
>> So may be we should report it to the 'user' and allow correcting the FIT?
>> Otherwise, it is impossible to debug the image w/o a debug version of U-Boot...
>> Do I make sense, or do I miss something?
> 
> Yes that makes some sense, but the problem is that then you are
> including error messages always which would never happen in a working
> system (i.e. it just bloats the code).

Unless, there a kind of corruption or a user mistake and then that same
user can't even understand what happened because we do not help him with
an error message.
You cannot know that these error messages will never happen...
This is a generic code which can be used by a wide variety of platforms - 
I don't think you can foresee all the possible use cases.

We are talking about several tens of bytes vs. usability.
If there is an error, it should be stated as such. It should not just
exit silently...

I think that debug() is a good thing to use, but we shouldn't be obsessed
with it.

> 
> So long as the error is reported (even if it is not a very specific
> error), people can add DEBUG and track it down.

That depends who 'people' are? Devs? Users?


-- 
Regards,
Igor.