Hi Colin,
Thank you for the patch.
On mar., mars 12, 2024 at 07:57, Colin McAllister colinmca242@gmail.com wrote:
Sam also gave his review here: https://lore.kernel.org/all/CAPLW+4kHmPtfACyND4Vc2p0ZrsyGY=+bRU=fdub4K1uX5p3...
Please include his review tag in the next submission.
I will add it at the appropriate place below:
From: Colin McAllister colin.mcallister@garmin.com
Currently, setting CONFIG_AB_BACKUP_OFFSET in a target's defconfig will not actually enable the #if protected code in android_ab.c. This is because "CONFIG_" should have been prepended to the config macro, or the macros defined in kconfig.h could have been used.
The code included by ANDROID_AB_BACKUP_OFFSET has been refactored to no longer be conditionally compiled by preprocessor conditionals and instead use C conditionals. This better aligns with the Linux kernel style guide.
Fixes: 3430f24bc6 ("android_ab: Try backup booloader_message") Signed-off-by: Colin McAllister colin.mcallister@garmin.com Cc: Joshua Watt JPEWhacker@gmail.com Cc: Simon Glass sjg@chromium.org Signed-off-by: Colin McAllister colinmca242@gmail.com
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
v2:
- Replaced #if conditionals with C if conditionals
- Opted to use CONFIG_ANDROID_AB_BACKUP_OFFSET directly instead of macros in kconfig.h as CONFIG_ANDROID_AB_BACKUP_OFFSET is not a
boolean or tristate value and doesn't have different values when building SPL or TPL. v3:
- Added "Fixes:" tag
v4:
- No changes
boot/android_ab.c | 97 ++++++++++++++++++++++------------------------- 1 file changed, 45 insertions(+), 52 deletions(-)
diff --git a/boot/android_ab.c b/boot/android_ab.c index 9a3d15ec60..f547aa64e4 100644 --- a/boot/android_ab.c +++ b/boot/android_ab.c @@ -187,13 +187,12 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, bool dec_tries) { struct bootloader_control *abc = NULL;
- struct bootloader_control *backup_abc = NULL; u32 crc32_le; int slot, i, ret; bool store_needed = false;
- bool valid_backup = false; char slot_suffix[4];
-#if ANDROID_AB_BACKUP_OFFSET
- struct bootloader_control *backup_abc = NULL;
-#endif
ret = ab_control_create_from_disk(dev_desc, part_info, &abc, 0); if (ret < 0) { @@ -205,53 +204,49 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, return ret; }
-#if ANDROID_AB_BACKUP_OFFSET
- ret = ab_control_create_from_disk(dev_desc, part_info, &backup_abc,
ANDROID_AB_BACKUP_OFFSET);- if (ret < 0) {
free(abc);return ret;
- if (CONFIG_ANDROID_AB_BACKUP_OFFSET) {
ret = ab_control_create_from_disk(dev_desc, part_info, &backup_abc,CONFIG_ANDROID_AB_BACKUP_OFFSET);if (ret < 0) {free(abc);return ret;}-#endif
crc32_le = ab_control_compute_crc(abc); if (abc->crc32_le != crc32_le) { log_err("ANDROID: Invalid CRC-32 (expected %.8x, found %.8x),", crc32_le, abc->crc32_le); -#if ANDROID_AB_BACKUP_OFFSET
crc32_le = ab_control_compute_crc(backup_abc);if (backup_abc->crc32_le != crc32_le) {log_err("ANDROID: Invalid backup CRC-32 ");log_err("expected %.8x, found %.8x),",crc32_le, backup_abc->crc32_le);-#endif
log_err("re-initializing A/B metadata.\n");
if (CONFIG_ANDROID_AB_BACKUP_OFFSET) {crc32_le = ab_control_compute_crc(backup_abc);if (backup_abc->crc32_le != crc32_le) {log_err(" ANDROID: Invalid backup CRC-32 ");log_err("(expected %.8x, found %.8x),",crc32_le, backup_abc->crc32_le);} else {valid_backup = true;log_info(" copying A/B metadata from backup.\n");memcpy(abc, backup_abc, sizeof(*abc));}}if (!valid_backup) {log_err(" re-initializing A/B metadata.\n"); ret = ab_control_default(abc); if (ret < 0) {-#if ANDROID_AB_BACKUP_OFFSET
free(backup_abc);-#endif
if (CONFIG_ANDROID_AB_BACKUP_OFFSET)free(backup_abc); free(abc); return -ENODATA; }-#if ANDROID_AB_BACKUP_OFFSET
} else {/** Backup is valid. Copy it to the primary*/memcpy(abc, backup_abc, sizeof(*abc));-#endif store_needed = true; }
if (abc->magic != BOOT_CTRL_MAGIC) { log_err("ANDROID: Unknown A/B metadata: %.8x\n", abc->magic); -#if ANDROID_AB_BACKUP_OFFSET
free(backup_abc);-#endif
if (CONFIG_ANDROID_AB_BACKUP_OFFSET)free(backup_abc);@@ -259,9 +254,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, if (abc->version > BOOT_CTRL_VERSION) { log_err("ANDROID: Unsupported A/B metadata version: %.8x\n", abc->version); -#if ANDROID_AB_BACKUP_OFFSET
free(backup_abc);-#endif
if (CONFIG_ANDROID_AB_BACKUP_OFFSET)free(backup_abc);@@ -338,30 +332,29 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, abc->crc32_le = ab_control_compute_crc(abc); ret = ab_control_store(dev_desc, part_info, abc, 0); if (ret < 0) { -#if ANDROID_AB_BACKUP_OFFSET
free(backup_abc);-#endif
if (CONFIG_ANDROID_AB_BACKUP_OFFSET)free(backup_abc); free(abc); return ret;-#if ANDROID_AB_BACKUP_OFFSET
- /*
* If the backup doesn't match the primary, write the primary* to the backup offset*/- if (memcmp(backup_abc, abc, sizeof(*abc)) != 0) {
ret = ab_control_store(dev_desc, part_info, abc,ANDROID_AB_BACKUP_OFFSET);if (ret < 0) {free(backup_abc);free(abc);return ret;
- if (CONFIG_ANDROID_AB_BACKUP_OFFSET) {
/** If the backup doesn't match the primary, write the primary* to the backup offset*/
checkpatch.pl seems to complain about this comment block:
WARNING: Block comments should align the * on each line #166: FILE: boot/android_ab.c:344: + /* + * If the backup doesn't match the primary, write the primary
To reproduce, run:
$ ./scripts/checkpatch.pl --strict --u-boot --git HEAD^..HEAD
With the above addressed, please send a v5 including:
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com
if (memcmp(backup_abc, abc, sizeof(*abc)) != 0) {ret = ab_control_store(dev_desc, part_info, abc,CONFIG_ANDROID_AB_BACKUP_OFFSET);if (ret < 0) {free(backup_abc);free(abc);return ret;}free(backup_abc);
- free(backup_abc);
-#endif
free(abc);
-- 2.34.1