Hi Colin, Thank you for the patch.
On mar., mars 12, 2024 at 07:57, Colin McAllister <colinmca...@gmail.com> wrote: Sam also gave his review here: https://lore.kernel.org/all/CAPLW+4kHmPtfACyND4Vc2p0ZrsyGY=+bRU=fdub4k1ux5p3...@mail.gmail.com/ Please include his review tag in the next submission. I will add it at the appropriate place below: > From: Colin McAllister <colin.mcallis...@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.mcallis...@garmin.com> > Cc: Joshua Watt <jpewhac...@gmail.com> > Cc: Simon Glass <s...@chromium.org> > Signed-off-by: Colin McAllister <colinmca...@gmail.com> Reviewed-by: Sam Protsenko <semen.protse...@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); > free(abc); > return -ENODATA; > } > @@ -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); > free(abc); > return -ENODATA; > } > @@ -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 <mkorpersh...@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
