| Message ID | 20240307221030.3685767-3-colin.mcallister@garmin.com |
|---|---|
| State | Superseded |
| Delegated to: | Mattijs Korpershoek |
| Headers | show |
| Series | Fix Android A/B backup | expand |
On Thu, Mar 7, 2024 at 4:11 PM Colin McAllister <colin.mcallister@garmin.com> wrote: > > 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. > I think this change deserves "Fixes:" tag. Also, please keep a changelog under "---" stanza for each patch in the series starting with v2, otherwise it's hard for reviewers to track the changes between different versions. > 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. > > Signed-off-by: Colin McAllister <colin.mcallister@garmin.com> > Cc: Joshua Watt <JPEWhacker@gmail.com> > Cc: Simon Glass <sjg@chromium.org> > --- > 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) { Did you test what happens if this config option is not defined at all? Maybe it'd be better to use CONFIG_IS_ENABLED() instead, here and in other cases like this? > + 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 > + */ > + 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.43.2 > > > ________________________________ > > CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use of the intended recipient(s) and contain information that may be Garmin confidential and/or Garmin legally privileged. If you have received this email in error, please notify the sender by reply email and delete the message. Any disclosure, copying, distribution or use of this communication (including attachments) by someone other than the intended recipient is prohibited. Thank you.
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 + */ + 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);
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. Signed-off-by: Colin McAllister <colin.mcallister@garmin.com> Cc: Joshua Watt <JPEWhacker@gmail.com> Cc: Simon Glass <sjg@chromium.org> --- boot/android_ab.c | 97 ++++++++++++++++++++++------------------------- 1 file changed, 45 insertions(+), 52 deletions(-) -- 2.43.2