From mboxrd@z Thu Jan 1 00:00:00 1970 From: Igor Opaniuk Date: Thu, 21 Mar 2019 17:39:36 +0200 Subject: [U-Boot] [PATCH v3 3/7] common: Implement A/B metadata In-Reply-To: References: <1550506917-25547-1-git-send-email-igor.opaniuk@linaro.org> <1550506917-25547-4-git-send-email-igor.opaniuk@linaro.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Eugeniu, On Fri, Mar 8, 2019 at 7:29 PM Eugeniu Rosca 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 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!