[U-Boot] [PATCH v3 3/7] common: Implement A/B metadata

[Igor Opaniuk]

Hi Eugeniu,

On Fri, Mar 8, 2019 at 7:29 PM Eugeniu Rosca <roscaeugeniu at gmail.com> wrote:
>
> Hello Igor,
>
> Thanks for the series. Some questions below.
>
> First, my understanding is that the patches replace the deprecated
> libavb_ab and make it trully obsolete, i.e. there should be no need to
> import libavb_ab into U-Boot (unlike some of our suppliers still do).
> Can you please confirm?

That's correct. Currently there is no any integration with AVB yet,
but it's planned to be the next step when these patches are merged.

>
> On Mon, Feb 18, 2019 at 5:25 PM Igor Opaniuk <igor.opaniuk at linaro.org> wrote:
> >
> [..]
>
> > diff --git a/include/android_bl_msg.h b/include/android_bl_msg.h
> > new file mode 100644
> > index 0000000..f37e01a
> > --- /dev/null
> > +++ b/include/android_bl_msg.h
> > @@ -0,0 +1,169 @@
> > +/* SPDX-License-Identifier: BSD-2-Clause */
> > +/*
> > + * This file was taken from the AOSP Project.
> > + * Repository: https://android.googlesource.com/platform/bootable/recovery/
> > + * File: bootloader_message/include/bootloader_message/bootloader_message.h
>
> I won't object on it and it's not my purpose to do any
> teaching/mentoring, but I think it makes sense to decouple (i.e.
> allocate standalone commits for) these two activities:
>  - integration of external headers/libraries (e.g. libavb, dtc,
> headers imported from linux/avb/recovery/etc trees)
>  - in-tree U-Boot development around those headers/libraries
>
> I think mixing these two activities creates more overhead for the
> reviewers, so it's easy for various mistakes to slip in unnoticed. See
> next comment as example.
>
> > + * Commit: 8b309f6970ab3b7c53cc529c51a2cb44e1c7a7e1
>
> The contents of out-of-tree "bootloader_message.h" at this commit
> doesn't appear to contain any "Omaha" references:
> https://android.googlesource.com/platform/bootable/recovery.git/+/8b309f6970ab3b/bootloader_message/include/bootloader_message/bootloader_message.h
>
> The addition of "Omaha" comment is done in commit:
> $ git log --oneline -G Omaha  8b309f6970ab..recovery/master --
> bootloader_message/include/bootloader_message/bootloader_message.h
> 7191bf049216 Add update_channel field to bootloader_message_ab.
>
> So, I believe there is a mismatch between the contents of the newly
> created file and its documented version.

Totally agree. These patch-series were initially introduced (v1 and
v2) by Ruslan (who was actually the main author),
so unfortunately I'm barely aware why in these particular commit both
external headers/libraries and in-tree U-boot stuff are mixed.
Will be fixed in v4.

>
> [..]
>
> > +       u8 priority : 4;
> > +       /* Number of times left attempting to boot this slot */
> > +       u8 tries_remaining : 3;
> > +       /* 1 if this slot has booted successfully, 0 otherwise */
> > +       u8 successful_boot : 1;
>
> The s/uint8_t/u8/ and s/uint32_t/u32/ conversion creates noise
> comparing the in-tree versus out-of-tree files and will add some
> overhead during integration. I still see a lot of U-Boot code saying
> uint8_t/uint32_t, so I wonder if these can be preserved. BTW, some of
> the patches from this series add code using uint32_t.

Agree.

>
> Looking forward to pick the patches from mainline!
>
> Thanks and best regards,
> Eugeniu.
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

Thanks for the review!