[PATCH 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET

[Igor Opaniuk]

Hello Colin,

+CC Mattijs, Sam

On Thu, Mar 7, 2024 at 5:19 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.
>
> Each use of AB_BACKUP_OFFSET is now wrapped within CONFIG_VAL().
>
> 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 | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/boot/android_ab.c b/boot/android_ab.c
> index 9a3d15ec60..a80040749d 100644
> --- a/boot/android_ab.c
> +++ b/boot/android_ab.c
> @@ -191,7 +191,7 @@ int ab_select_slot(struct blk_desc *dev_desc, struct
> disk_partition *part_info,
>         int slot, i, ret;
>         bool store_needed = false;
>         char slot_suffix[4];
> -#if ANDROID_AB_BACKUP_OFFSET
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>         struct bootloader_control *backup_abc = NULL;
>  #endif
>
> @@ -205,9 +205,9 @@ int ab_select_slot(struct blk_desc *dev_desc, struct
> disk_partition *part_info,
>                 return ret;
>         }
>
> -#if ANDROID_AB_BACKUP_OFFSET
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>         ret = ab_control_create_from_disk(dev_desc, part_info, &backup_abc,
> -                                         ANDROID_AB_BACKUP_OFFSET);
> +
>  CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET));
>         if (ret < 0) {
>                 free(abc);
>                 return ret;
> @@ -218,7 +218,7 @@ int ab_select_slot(struct blk_desc *dev_desc, struct
> disk_partition *part_info,
>         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
> +#if CONFIG_VAL(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 ");
> @@ -230,13 +230,13 @@ int ab_select_slot(struct blk_desc *dev_desc, struct
> disk_partition *part_info,
>
>                         ret = ab_control_default(abc);
>                         if (ret < 0) {
> -#if ANDROID_AB_BACKUP_OFFSET
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>                                 free(backup_abc);
>  #endif
>                                 free(abc);
>                                 return -ENODATA;
>                         }
> -#if ANDROID_AB_BACKUP_OFFSET
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>                 } else {
>                         /*
>                          * Backup is valid. Copy it to the primary
> @@ -249,7 +249,7 @@ int ab_select_slot(struct blk_desc *dev_desc, struct
> disk_partition *part_info,
>
>         if (abc->magic != BOOT_CTRL_MAGIC) {
>                 log_err("ANDROID: Unknown A/B metadata: %.8x\n",
> abc->magic);
> -#if ANDROID_AB_BACKUP_OFFSET
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>                 free(backup_abc);
>  #endif
>                 free(abc);
> @@ -259,7 +259,7 @@ 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
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>                 free(backup_abc);
>  #endif
>                 free(abc);
> @@ -338,7 +338,7 @@ 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
> +#if CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET)
>                         free(backup_abc);
>  #endif
>                         free(abc);
> @@ -346,14 +346,14 @@ int ab_select_slot(struct blk_desc *dev_desc, struct
> disk_partition *part_info,
>                 }
>         }
>
> -#if ANDROID_AB_BACKUP_OFFSET
> +#if CONFIG_VAL(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);
> +
> CONFIG_VAL(ANDROID_AB_BACKUP_OFFSET));
>                 if (ret < 0) {
>                         free(backup_abc);
>                         free(abc);
>

Thanks for the patch. The only suggestion: I see no reason in wrapping it in
preprocessor conditionals (in this particular case), better to use C
conditionals instead (with CONFIG_VAL macro) as explained in [1].

-
> 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.
>

[1]
https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation
-- 
Best regards - Atentamente - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk at gmail.com
skype: igor.opanyuk
http://ua.linkedin.com/in/iopaniuk