Hello Igor, Alex, Kever,
Having these patches in mainline would be great, as this would reduce the delta between our own and community U-boot trees. After having a quick look at this series, I have some questions/review findings.
These patches appear to be slightly older than what is available in [1].
More precisely, ignoring: - change of license - drop of avb_crc32.c - updates in avb_sysdeps_posix.c - 's^#include "^#include "avb/^'
The version of libavb imported in this patch series seems to match commit b60834f7a4a8 ("uefi: Set both androidboot.slot and androidboot.slot_suffix.") from [1].
FYI, there are 12 more non-merge commits in the avb repository [2]. Is there a strong reason not to include them?
I am not an expert in AVB, but IMHO, as suggested by Alex, we shouldn't assume that the in-tree libavb will diverge from the vanilla one. IMHO a cleaner workflow would be to contribute any bugfixes to the original repository and update the U-boot libavb from time to time, similar to how it's done, as example, for dtc in kernel [3]. Or maybe you see some obstacles to achieve this in practice?
Assuming that: 1) libavb will undergo regular updates from its original repository. 2) most of libavb headers are not designed to be included from non-libavb code, throwing below: #error "Never include this file directly, include libavb.h instead."
I wonder if it would be possible to keep the internal libavb headers in lib/libavb (similar to how it's done by NXP in [4]), since this would allow not rewriting the original include paths for such headers in every *.c file and hence minimize the delta between in-tree vs out-of-tree code, as well as potentially speed up the update process (and/or simplify any update script which will take care of it, as also mentioned by Alex).
My final question is what's your opinion about the NXP-specific libavb wrapper implementation found at `lib/avb/fsl/` in [5]/[6] which seems to rely on libavb and libavb_ab. Do you see it as a good model to be followed by other platforms (both from point of view of contents and placement in the tree)? I am asking because we are in the middle of some decision-making for similar/alternative implementation on a different SoC, so your feedback will be extremely helpful and greatly appreciated.
Thank you!
Best regards, Eugeniu.
[1] https://android.googlesource.com/platform/external/avb/+/master
[2] Postt-b60834f7a4a8 libavb commits in [1] $> git log --oneline --no-merges b60834f7a4a8..avb_upstream/master -- libavb 30dd8e5a1757 libavb: Add new routine to calculate a digest of all vbmeta images. fd0ba0d49101 Implement support for on-device persistent digests. 97740e537ad1 Split kernel cmdline code in separate file. fcadbf1d1a71 Support (boot) partition preloading. 061e33b39e27 Add avb_div_by_10() sysdep operation. 36e5c43f58d2 Fix incorrect variable names in avb_replace fc2531374e30 Fix AvbAlgorithmType type-limits error 047ecf7d2361 libavb: Avoid conflict with system-provided crc32 symbol. 0922bf8970fd Make it possible to disable verification. 01ca9962bd0d libavb: Only load and verify hash partition if requested. 8221811c5da1 libavb: Allow specifying dm-verity error handling. 27a291fcc194 libavb: Load entire partition if |allow_verification_error| is true.
[3] The way in-tree kernel DTC is aligned to upsteam: $> git log --oneline --no-merges -i --grep "update.*upstream" -- scripts/dtc 9130ba884640 scripts/dtc: Update to upstream version v1.4.6-9-gaadd0b65c987 e45fe7f788dd scripts/dtc: Update to upstream version v1.4.5-6-gc1e55a5513e9 4201d057ea91 scripts/dtc: Update to upstream version v1.4.5-3-gb1a60033c110 89d123106a97 scripts/dtc: Update to upstream version v1.4.4-8-g756ffc4f52f6
[4] http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/commit/?h=60e14a6a07... [5] http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/tree/lib/avb/fsl?h=i... [6] http://git.freescale.com/git/cgit.cgi/imx/uboot-imx.git/tree/lib/avb/fsl?h=i...