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

[Eugeniu Rosca]

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?

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.

[..]

> +       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.

Looking forward to pick the patches from mainline!

Thanks and best regards,
Eugeniu.