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:
- It happened once already, last year. You approved and applied a
patch which does overwrite the variable.
- 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