[U-Boot] [PATCH v2 0/45] Verified boot implementation based on FIT

[Simon Glass]

Hi Kim,

On Mon, Apr 1, 2013 at 5:13 PM, Kim Phillips <kim.phillips at freescale.com> wrote:
> On Mon, 18 Mar 2013 16:51:20 -0700
> Simon Glass <sjg at chromium.org> wrote:
>
>> I have received a number of off-list comments - please do copy the list when
>> replying so that everyone can see your comments.
>
> I don't have time to fully review 45 patches, let alone the subject
> matter (e.g., no support for RSA in h/w, eh? ;), but I did notice
> some things that bugged me:
>
> Re: "[PATCH v2 04/45] libfdt: Add fdt_next_subnode() to permit easy subnode iteration":
>
> - Where's our libfdt maintainer?  libfdt patches should be submitted
> to the dtc project first.  It appears "[PATCH v2 40/45] libfdt: Add
> fdt_find_regions()" also suffers from this problem.

The fdt_next_subnode() is a pretty trivial change. I will submit it to
the dtc project.

The fdt_find_regions() was submitted to dtc as part of the dtc fdtgrep
series. I modified the form of it in response to review feedback and
that is ongoing.

>
> - can we improve on the readability of that for loop - the improved
> one the patch gives us is this:
>
>> for (ndepth = 0,
>>               noffset = fdt_next_subnode(fit, image_noffset, &ndepth);
>>               noffset >= 0;
>>               noffset = fdt_next_subnode(fit, noffset, &ndepth)) {
>
> whereas perhaps something like this:
>
> ndepth = 0;
> noffset = fdt_next_subnode(fit, image_noffset, &ndepth);
> while (noffset >= 0) {
>         // body
>         noffset = fdt_next_subnode(fit, noffset, &ndepth);
> }
>
> would be much easier to quickly visually parse?

This suffers from the problem that a 'continue' will not work as
expected. I think the existing for() loop is a little better overall -
everything is in one place.

>
> this would also affect:
>
> Re: "[PATCH v2 12/45] image: Move hash checking into its own function":
> - which also adds lines like these:
>
>> +             *err_msgp = " error!\nCan't get hash algo "
>> +                             "property";
>
> upon which a subsequent patch ("[PATCH v2 13/45] image: Move error!
> string to common place", if I'm not mistaken) corrects.  Are you
> intentionally trying to make people review the same code twice??
>

No, what is happening here is that the first patch moves the code into
a function, and the second patch removes the 'error' part of the
strings, not just from that moved code but from everywhere. I find
that doing two things at once is harder to review.

I am certainly not added 'error' to the strings in one patch and
taking it away in the next :-)

> Re: "[PATCH v2 07/45] image: Split FIT code into new image-fit.c"
> - after the code is split, it appears the rest of the patchseries
> works on improving image-fit.c instead of updating equivalent code
> still existing in image.c, IIRC, "[PATCH v2 13/45] image: Move error!
> string to common place"
> - I also found it odd that git format-patch -M on this doesn't
> produce a shorter patch with "% equivalent" statistics: what else
> changed?

Are you suggesting that more patches should affect image.c? If so,
please let me know what should specifically you want done and I will
take a look. Or perhaps we can improve it once the dust clears.

>
> Re: "[PATCH v2 22/45] env: Fix minor comment typos in cmd_nvedit":
> - I personally appreciate it when authors of such large patchseries
> put the trivial stuff first, as if to follow the logical progression
> of the complexity (and therefore acceptability) of the patches in
> the series.  Make things like this the first patch in the series -
> that way maintainers can be tempted to start applying obvious
> patches from early patchseries versions.  This also applies to
> patches like [PATCH v2 24/45] Revert "fdt- Tell the FDT library
> where the device tree is", and:
>
> Re: "[PATCH v2 25/45] Add stdarg to vsprintf.h":
> - which contains the text "TODO: Probably should drop this patch?".
> Well?  Did you mean this series to be an RFC?

Yes I am going to split this up into 3-4 series. The first one
(sandbox changes) was sent earlier today. The next one will focus on
the initial improvements to the image code, and will include these
trivial patches - I will ensure that they are at the start - I agree
it is better that way.

The series was originally RFC, but in version 2 I removed that tag.
Part 25 was added to deal with HOSTCC building vsprintf.h, but I
subsequently found that we could avoid that. I will drop it from v3.

>
> I'm stopping my already brief review here - please clean this up.

Thanks for looking at this.

>
> Kim
>

Regards,
Simon