[U-Boot-Users] [PATCH] of dump command

[Jerry Van Baren <gerald.vanbaren@smiths-aerospace.com>]

Wow, you guys are _tough_!  I'm glad you didn't grade me for CompSci 101 
  :-)

Three patches will follow
* vsprintf "ll" extension (pretty trivial)
* oftdump command (da meat)
* PQ2FADS changes to use the flattened OF tree (pretty trivial)

More comments embedded...

Wolfgang Denk wrote:
> Dear Grant,
> 
> thanks a lot for your thorough comments.
> 
> In message <528646bc0610300943j5752c2c7i9b66864ec678a86e@mail.gmail.com> you wrote:
>> These should probably be split up into separate patches.  One for the
>> vsprintf 'll' support; one for the new command and one for the
>> MPC8260ADS config changes.
> 
> Agreed. Eventually these are separate commits in Jerry's repo anyway.
> Jerry?

Done, patch will be submitted.

>> This is not a lot of code; and it's pretty useful stuff; maybe just
>> make it automatically built when CONFIG_OF_FLAT_TREE is defined?
>> (Rather than adding a new CONFIG_COMMANDS macro at the moment)
> 
> Agreed.

Done, patch will be submitted.

>> If a new CFG_CMD_ macro set is needed; I wouldn't do it now.  There is
>> still 1 bit left.  :)  If it needs to be changed, it might make sense
>> to audit the full list and see if there is anything that should be
>> removed.
>>
>> Wolfgang needs to weigh in on this one.
> 
> I think we need a major cleanup of the config system, so I would like
> to touch as few things in this area as possible. Here, I  agree  with
> Grant  that  this  should  be  just  compiled  in when OFT support is
> enabled. Probably everybody will want this.

CONFIG1_* and CFG1_CMD_* have been removed.

>>> +U_BOOT_CMD(
>>> +       of,     3,      0,      do_of,
>>> +       "of      - Open Firmware utility commands\n",
>>> +       "of <addr>        - Dump the address as an OF tree\n"
>>> +       "of <addr> <prop> - Dump the given property\n"
>>> +);
>> I think 'of' is a bit terse.  Maybe "ofinfo", "ofdump", or "ftinfo"?
> 
> Additionally, I think that "of" [= Open Firmware] is wrong. It should
> be "oft" [Open Firmware Tree] at least. The command  should  IMHO  be
> named "oftdump", but "ftdump" [FT = Flat Tree] would be ok, too.

OK, I renamed it "oftdump" (but, thanks to the beauty of Wolfgang's 
minimum unique characters parsing, "of" still works ;-).

>>> + * Used by ft_dump_blob() and ft_get_prop() to build up property names
>>> + */
>>> +static char path[256], prop[256];
>> Is moving these out of ft_get_prop truly necessary?  (I haven't dug
>> into the full context, but seeing this makes me scared)
> 
> Me too :-)

Yes, this is ugly.  The existing ft_get_prop() had the two declarations 
as statics within the function declaration and I simply moved them out 
so they could be re-used by ft_dump_blob().  Note that they remain 
static, but they are available to ft_dump_blob() as well as 
ft_get_prop() now.  One alternative is to duplicate the static 
declarations in both routines, but that seems extremely silly.

(I'm also not wild about the fact that the buffer lengths are fixed and 
unchecked, so it is a buffer overflow vulnerability - I figure (a) I 
just used existing code and (b) simply having access to u-boot is more 
of a security issue than buffer overflows in u-boot.)

The problem is that the flattened OF tree only has the incremental 
path/property names rather than the full name on each property (I 
believe this was a version 0x10 optimization).  The easiest way to match 
the path/property is to build up (as you nest in) and back up (as you 
unnest) the path and then do a string compare of the path/property 
built-up string against the desired string.  This is what the existing 
ft_get_prop() did and what I extended ft_dump_blob() to do too.

I looked at least twice at doing a more clever matching and threw my 
changes away because I ended up going down a blind alley.  Keeping track 
of the nesting and unnesting while following the tree takes some 
non-trivial logic.  I'm not saying that there _isn't_ a better way, but 
building up the string and doing a string compare is _much_ easier and 
quite likely takes less code than being clever.

>> There's still one more bit left in CFG_CMD_ALL; just use it!  :)
>>
>> Or eliminate CFG_CMD_OF entirely.
> 
> That's what I'd suggest.

Done.

> Thanks again, to both of you.
> 
> Best regards,
> 
> Wolfgang Denk

Patches to follow...

Best regards,
gvb