[PATCH v2 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET
Sam Protsenko
semen.protsenko at linaro.org
Fri Mar 8 00:08:18 CET 2024
On Thu, Mar 7, 2024 at 4:11 PM Colin McAllister
<colin.mcallister at 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 at garmin.com>
> Cc: Joshua Watt <JPEWhacker at gmail.com>
> Cc: Simon Glass <sjg at 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.
More information about the U-Boot
mailing list