Re: [PATCH dtc take 2] Fix reserve map output for asm format.

[Jerry Van Baren <gvb.linuxppc.dev@gmail.com>]

David Gibson wrote:
> On Sun, Apr 15, 2007 at 08:24:06PM -0400, Jerry Van Baren wrote:
>> Milton Miller wrote:
>>> Sometime around Sun Apr 15 12:29:14 EST 2007, Jerry Van Baren wrote:
>>>> Add extra reserve map slots output for asm format (previously done for 
>>>> dtb
>>>>   output).
>>>>
>>>> Signed-off-by: Gerald Van Baren <vanbaren at cideas.com>
>>>> ---
>>>>
>>>> Hi Jon, David,
>>>>
>>>> Here is a patch that fixes the asm output without the (unnecessary)
>>>> calloc change.
>>>>
>>>> Best regards,
>>>> gvb
>>>
>>> The previous description had
>>>> Use cmalloc to pre-zero memory (for dtb input) and handle dtb (binary)
>>>>   input being shorter than the total blob length (result of putting
>>>>   extra space in the blob).
>>>
>>> Which at least said in the description the unrelated things it was 
>>> doing.
>> That was my added comment WRT the change from malloc to cmalloc.  David 
>> wasn't wild about using cmalloc, so I removed it.  Using cmalloc is not 
>> necessary.
>>
>>>>         while (sizeleft) {
>>>> -               if (feof(f))
>>>> -                       die("EOF before reading %d bytes of DT blob\n",
>>>> -                           totalsize);
>>>> +               if (feof(f)) {
>>>> +                       WARNMSG("EOF after reading %d of %d bytes of 
>>>> DT blob, assuming there is extra space in the blob.\n",
>>>> +                           totalsize - sizeleft, totalsize);
>>>> +                       break;
>>>> +               }
>>> I thnk the above should be an ERROR and cause failure without
>>> the -f (force) option.
>>>
>>> The total_size says how much data should be copied.  Anything
>>> less and there is data missing.   Assuming zeros is wrong for
>>> most sections (the exception being the memory reserve list
>>> that had a terminating 0 entry within the read portion).
>>>
>>> milton
>> The reason total_size is bigger than the actual size is because I 
>> created the blob with extra space using the -S parameter.  It is 
>> intentionally bigger.  The extra space is ignored by dtc when creating a 
>> dts/asm format output which is why cmalloc() is unnecessary.
>>
>> I suppose we could require a -f force but I'm not wild about creating a 
>> nanny program.  There is nothing wrong with the blob - it parses just 
>> fine.  If there were problems with the blob contents, other errors would 
>> be raised.
> 
> I think the warning is fine, but not for exactly the reasons you
> state.  Several points:
> 
> - At least with v17 input, where it's possible, we probably *should*
> check that an input blob isn't truncated in the middle of the strings
> or structure sections.  That should be more than a warning.
> 
> - Milton, saying totalsize indicates the amount of data to be copied
> is misleading in this context.  That's a good philosphy for things
> that just read and/or slightly tweak the tree - data outside the known
> sections which it can't interpret should be left unaltered wherever
> possible.  dtc, however, *always* fully interprets and re-emits the
> tree.  Any data outside the known and understood sections is *always*
> discarded, so I don't think there's any problem assuming it to be
> zero.
> 
> - That said, I think when using -S, at least the default behaviour
> should emit extra zero bytes in addition to changing the totalsize
> header.  Then at least in the simplest case of feeding dtc's dtb
> output back into dtc, the warning will not occur.

Hi David,

Yes, emitting zeros (or asm equiv) for the extra space makes sense. 
That would be a better way to fix the read problem.

I'm thinking about a fill/nofill and/or specifying the value for the 
fill bytes, but I cannot think of a reason that would be useful (my 
first thought was filling with 0xFF for flash, but I think most or all 
fdt writes need to modify some existing values so being able to modify 
the extra space in-place doesn't help because the existing values cannot 
be modified in-place in flash).

Now to find some time to implement it...

Best regards,
gvb