Hi Eugeniu,
On Fri, Mar 8, 2019 at 7:29 PM Eugeniu Rosca roscaeugeniu@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@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.
- 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/+/8b309f6970...
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@lists.denx.de https://lists.denx.de/listinfo/u-boot
Thanks for the review!