Thread (5 messages) 5 messages, 2 authors, 2007-02-23

Re: libfdt: a fix but still broken

From: Jerry Van Baren <hidden>
Date: 2007-02-23 14:33:13

Jerry Van Baren wrote:
David Gibson wrote:
quoted
On Thu, Feb 22, 2007 at 08:27:29AM -0500, Jerry Van Baren wrote:
quoted
Hi Dave,

I have good news and bad news. ;-)  The good news is that the 
incompatibility between libfdt and jdl's dtc is that libfdt has the 
name offset and the length of the name switched.  
booting_without_of.txt says the length comes first, so libfdt is in 
the wrong.
Ouch.  That's.. a very embarrassing bug.  Actually, I know where it
came from: I was looking at flat_dt.h from dtc, which also gets this
wrong (but the declaration in question is unused).  Of course, I also
wrote flat_dt.h ...
quoted
The bad news is that, when I fix this, nearly all of the tests fail 
(but they fail the same way for both tree.S and jdl's dtc).  I have 
not started on that layer of the onion yet.
Found it, there was a direct use of the position of the length in
_fdt_next_tag().  Just pushed out a fix for this in the libfdt tree,
along with some other small fixes which I found while tracking this
one down.

Oh, incidentally, I applied your patch by eye rather than with
patch(1), which was handy, because it appears to have been whitespace
damaged.
Hi David,

Sorry about the whitespace, I actually generated it at home, emailed it 
to my day job, and edited and forwarded it from there (I'm not 
subscribed at home - probably should be).

It sounds like we found and fixed the same bug last night.

I went quite a bit further (before finding and fixing the real bug).  I 
was looking at the *_type "function like" #defines that generate a 
temporary variable of indeterminate (until compile time) type.  An example:

#define fdt_setprop_inplace_typed(fdt, nodeoffset, name, val) \
    ({ \
        typeof(val) x = val; \
        fdt_setprop_inplace(fdt, nodeoffset, name, &x, sizeof(x)); \
    })

IMHO this is borderline evil and unnecessary, and I removed all of them 
by replacing the call of the *_type macros with the equivalent actual 
function.  What I was suspicious of is when I changed all the calls to 
use proper endian by calling cpu_to_fdt32(), the parameter "val" is no 
longer a simple constant or variable but is, instead, a function call or 
macro or nothing at all, depending on the host endian and compiler 
implementation.  It turns out that this apparently wasn't a problem, but 
I'm still of the opinion that those #defines have minimal value and 
makes the code baroque. ;-)

I've attached a patch file that shouldn't be whitespace damaged this 
time.  :-/  I can also make my git repository accessible if that is 
helpful.

Best regards,
gvb
Hi David,

I exposed my git repository for my hacks on libfdt:
<http://www.cideas.us/cgi-bin/gitweb.cgi?p=linux/libfdt.git;a=summary>

BTW, a patch I didn't publish (yet)added some comments:
<http://www.cideas.us/cgi-bin/gitweb.cgi?p=linux/libfdt.git;a=commitdiff;h=775fee94444b4cda010e3d55f8c2aa1a0efe8856>

I also neglected to mention that, in the patch that was attached to the 
parent email, I added a configuration option to tests/Makefile to more 
easily select trees.S vs. dtc (now it is just ugly instead of fugly).

Best regards,
gvb
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help