[U-Boot-Users] [PATCH v3] Fix initrd length miscalculation in bootm command when using a dtu

Jerry Van Baren
2007-05-04T14:22:09Z

Stefan Roese wrote:
> Hi Timur,
>
> On Friday 04 May 2007 15:25, Timur Tabi wrote:
>> I'm glad you fixed the bug, so I'll just add a few comments:
>>> Your comment is actually wrong. The use of "len" is not limited to
>>> that purpose.
>> If you apply my patch, then the comment becomes correct. My goal was to
>> lock the variables 'len' and 'data' into one purpose. The reason the
>> bug existed is because the other developer didn't realize this. He used
>> 'len' thinking it was available. In a sense, I was trying to implement
>> some "defensive programming", so that the next time someone hacks up
>> do_bootm_linux(), he won't re-introduce the bug.
>>
>> Now, you might say that that won't happen again, but I disagree. I
>> think it can, for two reasons:
>>
>> 1) It happened once already, last year. You approved and applied a
>> patch which does overwrite the variable.
>>
>> 2) The libfdt code introduced a number of other bugs relating to dtu
>> usage, which have not yet been fixed.
>>
>> So I believe there is a real possibility that another patch could
>> re-introduce this bug. If you had applied my patch as-is, that
>> possibility would have been eliminated. This is why I think my patch is
>> better than yours.
>>
>> But I guess only time will tell who's right. :-)
>
> You have a point with your variable usage "restriction", and Wolfgang has a
> point with the "readability" of your patch as it doesn't really tell what is
> fixed without read the current source. Could be that your implementation is
> the "better" one, but the patch Wolfgang applied was just less intrusive.
>
>>> And please accept my apologies thatt his was so complicated and took
>>> so long. [Nevertheless you still might want to try to find a way to
>>> access the repository I created for you in case you have more
>>> patches.]
>> Stefan said he had a testing repo of some kind. How about we just use
>> that? If Stefan is willing, he can apply my emailed patches to his repo.
>
> No, we shouldn't "fork" the code here. Let's focus on the other open issues.
> You mention other bugs introduced by the libfdt code above. ;-)
>
> Best regards,
> Stefan

Timur has sent me two libfdt/dtu-related fixes
* fdt_copy_into() had the source and destination addresses reversed
* fdt_check_header() had the wrong sense on ==/!= 0
which I applied to my local repo last night and will push back to
u-boot-fdt soon (I still have not been successful in figuring out how to
make a multi-image to test the changes grrrr). The two bugs I
introduced in the conversion to libfdt were unrelated to the "len"
variable, but I probably replicated the "len" bug when I duplicated and
modified the original code. I will pull Wolfgang's fix tonight and see
what needs to be done to make all three bugs go away.

WRT to building a multi-image, I'm looking to combine linux, a dtb, and
an initrd into a single image to test that path of bootm (the path I had
the above screwups in).

Timur's hint for me was:
> mkimage -A ppc -T flat_dt -C none -d mpc836x_mds.dtb mpc836x_mds.dtu
>
> This, of course, creates a dtu with a value of 0 for ih_load. You
> probably need to specify -a or -e to set that field.
but I don't understand how to build an image with all three pieces in it
(I tried to use the ":" in the -d source parameter and mkimage just got
confused, couldn't find the files). Am I expecting too much??? Should
I just be wrapping the three pieces individually and loading them
separately? What exactly are you doing to test this, Timur?

Best regards,
gvb