[PATCH 0/2] Fix Android A/B backup

[PATCH 0/2] Fix Android A/B backup

[Colin McAllister]

- Addresses compiler error due to missing semicolon
- Fixes use of ANDROID_AB_BACKUP_OFFSET in preprocessor macros

Bug was found by noticing a semicolon was missing and not causing a
compiler error when CONFIG_ANDROID_AB_BACKUP_OFFSET was set. I submitted
a patch to fix the semicolon before fixing the #if's. Testing the latter
patch without the former with ANDORID_AB_BACKUP_OFFSET set will cause a
compiler error.

Colin McAllister (2):
  android_ab: Add missing semicolon
  android_ab: Fix ANDROID_AB_BACKUP_OFFSET

 boot/android_ab.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

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

[PATCH 1/2] android_ab: Add missing semicolon

[Colin McAllister]

Found a missing semicolon in code protected by a #if that will never
evaluate to true due to a separate issue. Fixing this issue before
addressing the #if.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/boot/android_ab.c b/boot/android_ab.c
index c9df6d2b4b..9a3d15ec60 100644
--- a/boot/android_ab.c
+++ b/boot/android_ab.c
@@ -221,7 +221,7 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
 #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("ANDROID: Invalid backup CRC-32 ");
                        log_err("expected %.8x, found %.8x),",
                                crc32_le, backup_abc->crc32_le);
 #endif
--
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.

[PATCH 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET

[Colin McAllister]

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@garmin.com>
Cc: Joshua Watt <JPEWhacker@gmail.com>
Cc: Simon Glass <sjg@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);
--
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.

Re: [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@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@garmin.com>
> Cc: Joshua Watt <JPEWhacker@gmail.com>
> Cc: Simon Glass <sjg@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@gmail.com
skype: igor.opanyuk
http://ua.linkedin.com/in/iopaniuk

[PATCH v2 0/2] Fix Android A/B backup

[Colin McAllister]

- Addresses compiler error due to missing semicolon
- Removes use of preprocessor macros with ANDROID_AB_BACKUP_OFFSET

Bug was found by noticing a semicolon was missing and not causing a
compiler error when CONFIG_ANDROID_AB_BACKUP_OFFSET was set. I submitted
a patch to fix the semicolon before fixing the #if's. Testing the latter
patch without the former with ANDORID_AB_BACKUP_OFFSET set will cause a
compiler error.

What's changed in V2?
---------------------

The second verison of changes removes the #if preprocessor macros to use
C conditionals instead. There was some minor refactoring required to get
this to work, so I did more thourough testing to ensure the backup data
works as expected.

I also realized that CONFIG_VAL is not needed because there's no reason
for the SPL or TPL to have different values for the backup address. I
opted to just use CONFIG_ANDROID_AB_BACKUP_OFFSET directly.

How was this patch series tested?
---------------------------------

I built for my target with CONFIG_ANDROID_AB_BACKUP_OFFSET set to
0x1000. I first verified that the device can normally boot. I then ran
dd before rebooting to corrupt the primary data. I confirmed that the
backup was used to properly restore the primary. I then corrupted both
the primary and backup data and confirmed that the primary and backup
were both reinitialized to default values. Lastly, I corrupted the
backup data and not the primary data and confirmed that the backup was
restored the primary data.

Colin McAllister (2):
  android_ab: Add missing semicolon
  android_ab: Fix ANDROID_AB_BACKUP_OFFSET

 boot/android_ab.c | 97 ++++++++++++++++++++++-------------------------
 1 file changed, 45 insertions(+), 52 deletions(-)

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

[PATCH v2 1/2] android_ab: Add missing semicolon

[Colin McAllister]

Found a missing semicolon in code protected by a #if that will never
evaluate to true due to a separate issue. Fixing this issue before
addressing the #if.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/boot/android_ab.c b/boot/android_ab.c
index c9df6d2b4b..9a3d15ec60 100644
--- a/boot/android_ab.c
+++ b/boot/android_ab.c
@@ -221,7 +221,7 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
 #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("ANDROID: Invalid backup CRC-32 ");
                        log_err("expected %.8x, found %.8x),",
                                crc32_le, backup_abc->crc32_le);
 #endif
--
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.

[PATCH v2 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET

[Colin McAllister]

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(-)

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);

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

Re: [PATCH v2 1/2] android_ab: Add missing semicolon

[Sam Protsenko]

On Thu, Mar 7, 2024 at 4:11 PM Colin McAllister
<colin.mcallister@garmin.com> wrote:
>
> Found a missing semicolon in code protected by a #if that will never
> evaluate to true due to a separate issue. Fixing this issue before
> addressing the #if.
>
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/boot/android_ab.c b/boot/android_ab.c
> index c9df6d2b4b..9a3d15ec60 100644
> --- a/boot/android_ab.c
> +++ b/boot/android_ab.c
> @@ -221,7 +221,7 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>  #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("ANDROID: Invalid backup CRC-32 ");

Good catch! Wonder why there is also a trailing space in the end of
the string. Anyways, I think this patch deserves "Fixes:" tag, would
you mind adding it? Other than that:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

>                         log_err("expected %.8x, found %.8x),",
>                                 crc32_le, backup_abc->crc32_le);
>  #endif
> --
> 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.

Re: [PATCH v2 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET

[Sam Protsenko]

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.

[PATCH v3 0/2] Fix Android A/B backup

[Colin McAllister]

- Addresses compiler error due to missing semicolon
- Removes use of preprocessor macros with ANDROID_AB_BACKUP_OFFSET

Bug was found by noticing a semicolon was missing and not causing a
compiler error when CONFIG_ANDROID_AB_BACKUP_OFFSET was set. I submitted
a patch to fix the semicolon before fixing the #if's. Testing the latter
patch without the former with ANDORID_AB_BACKUP_OFFSET set will cause a
compiler error.

What's changed in V2?
---------------------

The second verison of changes removes the #if preprocessor macros to use
C conditionals instead. There was some minor refactoring required to get
this to work, so I did more thourough testing to ensure the backup data
works as expected.

I also realized that CONFIG_VAL is not needed because there's no reason
for the SPL or TPL to have different values for the backup address. I
opted to just use CONFIG_ANDROID_AB_BACKUP_OFFSET directly.

What's changed in V3?
---------------------

Added "Fixes:" tag to patches. Performed additonal testing as described
in the second paragraph in the testing notes below.

I opted to not use CONFIG_IS_ENABLED because that macro appears to only
be intended for y/n configurations. See note at top of linux/kconfig.h:

"Note that these only work with boolean and tristate options."

How was this patch series tested?
---------------------------------

I built for my target with CONFIG_ANDROID_AB_BACKUP_OFFSET set to
0x1000. I first verified that the device can normally boot. I then ran
dd before rebooting to corrupt the primary data. I confirmed that the
backup was used to properly restore the primary. I then corrupted both
the primary and backup data and confirmed that the primary and backup
were both reinitialized to default values. Lastly, I corrupted the
backup data and not the primary data and confirmed that the backup was
restored the primary data.

Addtionally, I disabled CONFIG_ANDROID_AB_BACKUP_OFFSET for my device
and confirmed that after I corrupt the primary data, no backup is used.
When the primary data is not corrupt, the device boots normally. This is
what I would expect, because CONFIG_ANDROID_AB_BACKUP_OFFSET's default
value is 0x0, which would evaluate to false for all C if conditions.

Colin McAllister (2):
  android_ab: Add missing semicolon
  android_ab: Fix ANDROID_AB_BACKUP_OFFSET

 boot/android_ab.c | 97 ++++++++++++++++++++++-------------------------
 1 file changed, 45 insertions(+), 52 deletions(-)

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

[PATCH v3 1/2] android_ab: Add missing semicolon

[Colin McAllister]

Found a missing semicolon in code protected by a #if that will never
evaluate to true due to a separate issue. Fixing this issue before
addressing the #if.

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>
---
v2: No changes
v3: Added "Fixes:" tag

 boot/android_ab.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/boot/android_ab.c b/boot/android_ab.c
index c9df6d2b4b..9a3d15ec60 100644
--- a/boot/android_ab.c
+++ b/boot/android_ab.c
@@ -221,7 +221,7 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
 #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("ANDROID: Invalid backup CRC-32 ");
                        log_err("expected %.8x, found %.8x),",
                                crc32_le, backup_abc->crc32_le);
 #endif
--
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.

[PATCH v3 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET

[Colin McAllister]

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

 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
+               */
+               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.

Re: [PATCH v3 1/2] android_ab: Add missing semicolon

[Sam Protsenko]

On Fri, Mar 8, 2024 at 11:00 AM Colin McAllister
<colin.mcallister@garmin.com> wrote:
>
> Found a missing semicolon in code protected by a #if that will never
> evaluate to true due to a separate issue. Fixing this issue before
> addressing the #if.
>
> 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>
> ---

My R-b tag wasn't added in v3, adding it here so that whoever applies
this patch can add it as well:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

> v2: No changes
> v3: Added "Fixes:" tag
>
>  boot/android_ab.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/boot/android_ab.c b/boot/android_ab.c
> index c9df6d2b4b..9a3d15ec60 100644
> --- a/boot/android_ab.c
> +++ b/boot/android_ab.c
> @@ -221,7 +221,7 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>  #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("ANDROID: Invalid backup CRC-32 ");
>                         log_err("expected %.8x, found %.8x),",
>                                 crc32_le, backup_abc->crc32_le);
>  #endif
> --
> 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.

Re: [PATCH v3 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET

[Sam Protsenko]

On Fri, Mar 8, 2024 at 11:00 AM 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.
>
> 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>
> ---
> 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

Can you please also address my comment about CONFIG_IS_ENABLED() in
the previous mail? It might be just an answer if you think there are
no issues with that, not necessarily v4.

[snip]

Re: [PATCH v3 0/2] Fix Android A/B backup

[Sam Protsenko]

On Fri, Mar 8, 2024 at 11:00 AM Colin McAllister
<colin.mcallister@garmin.com> wrote:
>
> - Addresses compiler error due to missing semicolon
> - Removes use of preprocessor macros with ANDROID_AB_BACKUP_OFFSET
>
> Bug was found by noticing a semicolon was missing and not causing a
> compiler error when CONFIG_ANDROID_AB_BACKUP_OFFSET was set. I submitted
> a patch to fix the semicolon before fixing the #if's. Testing the latter
> patch without the former with ANDORID_AB_BACKUP_OFFSET set will cause a
> compiler error.
>
> What's changed in V2?
> ---------------------
>
> The second verison of changes removes the #if preprocessor macros to use
> C conditionals instead. There was some minor refactoring required to get
> this to work, so I did more thourough testing to ensure the backup data
> works as expected.
>
> I also realized that CONFIG_VAL is not needed because there's no reason
> for the SPL or TPL to have different values for the backup address. I
> opted to just use CONFIG_ANDROID_AB_BACKUP_OFFSET directly.
>
> What's changed in V3?
> ---------------------
>
> Added "Fixes:" tag to patches. Performed additonal testing as described
> in the second paragraph in the testing notes below.
>
> I opted to not use CONFIG_IS_ENABLED because that macro appears to only
> be intended for y/n configurations. See note at top of linux/kconfig.h:
>
> "Note that these only work with boolean and tristate options."

Ah, ok, I see you replied to my comment here. So when that config
option is not defined at all, the build still works, right? One way to
test that would be to use buildman tool for all boards. Although I
have to say it might be very time consuming (I usually run it
overnight), so I'm not asking you to do it in this case, it's just
FYI.

>
> How was this patch series tested?
> ---------------------------------
>
> I built for my target with CONFIG_ANDROID_AB_BACKUP_OFFSET set to
> 0x1000. I first verified that the device can normally boot. I then ran
> dd before rebooting to corrupt the primary data. I confirmed that the
> backup was used to properly restore the primary. I then corrupted both
> the primary and backup data and confirmed that the primary and backup
> were both reinitialized to default values. Lastly, I corrupted the
> backup data and not the primary data and confirmed that the backup was
> restored the primary data.
>
> Addtionally, I disabled CONFIG_ANDROID_AB_BACKUP_OFFSET for my device
> and confirmed that after I corrupt the primary data, no backup is used.
> When the primary data is not corrupt, the device boots normally. This is
> what I would expect, because CONFIG_ANDROID_AB_BACKUP_OFFSET's default
> value is 0x0, which would evaluate to false for all C if conditions.
>
> Colin McAllister (2):
>   android_ab: Add missing semicolon
>   android_ab: Fix ANDROID_AB_BACKUP_OFFSET
>
>  boot/android_ab.c | 97 ++++++++++++++++++++++-------------------------
>  1 file changed, 45 insertions(+), 52 deletions(-)
>
> --
> 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.

Re: [PATCH v3 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET

[Sam Protsenko]

On Fri, Mar 8, 2024 at 11:50 AM Sam Protsenko
<semen.protsenko@linaro.org> wrote:
>
> On Fri, Mar 8, 2024 at 11:00 AM 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.
> >
> > 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>
> > ---
> > 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
>
> Can you please also address my comment about CONFIG_IS_ENABLED() in
> the previous mail? It might be just an answer if you think there are
> no issues with that, not necessarily v4.
>

Ok, I just saw your reply in patch #0. Given that you tested that this
change doesn't break any other boards/configuration (e.g. when the
mentioned config is not defined), feel free to add:

Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>

> [snip]

Re: [PATCH v3 0/2] Fix Android A/B backup

[McAllister, Colin]

> Ah, ok, I see you replied to my comment here.

Yes, sorry. Outlook is terrible to send inline responses too. I figured
just adding responses in the patch contents would be better. Next time I'll submit
my patch with a different email :)

> So when that config option is not defined at all, the build still
> works, right?

Yes, the default value for CONFIG_ANDROID_AB_BACKUP_OFFSET is 0x0, which
would evaluate to a false bool value in the if conditions. I did do some
testing with the config value not defined for my board and confirmed
back-up data is not used.

In your other emails you include your reviewed-by tag. For clarity, Am I
supposed to append my patches and upload a new version? This is my
first time contributing to u-boot, so I'm still learning the workflow. I
didn't see anything glancing through the "Sending patches" page in the
U-Boot documentation.

Best,
Colin

________________________________________
From: Sam Protsenko <semen.protsenko@linaro.org>
Sent: Friday, March 8, 2024 11:54
To: McAllister, Colin
Cc: u-boot@lists.denx.de; JPEWhacker@gmail.com; sjg@chromium.org; Mattijs Korpershoek; Igor Opaniuk
Subject: Re: [PATCH v3 0/2] Fix Android A/B backup

On Fri, Mar 8, 2024 at 11: 00 AM Colin McAllister <colin. mcallister@ garmin. com> wrote: > > - Addresses compiler error due to missing semicolon > - Removes use of preprocessor macros with ANDROID_AB_BACKUP_OFFSET > > Bug


On Fri, Mar 8, 2024 at 11:00 AM Colin McAllister
<colin.mcallister@garmin.com> wrote:
>
> - Addresses compiler error due to missing semicolon
> - Removes use of preprocessor macros with ANDROID_AB_BACKUP_OFFSET
>
> Bug was found by noticing a semicolon was missing and not causing a
> compiler error when CONFIG_ANDROID_AB_BACKUP_OFFSET was set. I submitted
> a patch to fix the semicolon before fixing the #if's. Testing the latter
> patch without the former with ANDORID_AB_BACKUP_OFFSET set will cause a
> compiler error.
>
> What's changed in V2?
> ---------------------
>
> The second verison of changes removes the #if preprocessor macros to use
> C conditionals instead. There was some minor refactoring required to get
> this to work, so I did more thourough testing to ensure the backup data
> works as expected.
>
> I also realized that CONFIG_VAL is not needed because there's no reason
> for the SPL or TPL to have different values for the backup address. I
> opted to just use CONFIG_ANDROID_AB_BACKUP_OFFSET directly.
>
> What's changed in V3?
> ---------------------
>
> Added "Fixes:" tag to patches. Performed additonal testing as described
> in the second paragraph in the testing notes below.
>
> I opted to not use CONFIG_IS_ENABLED because that macro appears to only
> be intended for y/n configurations. See note at top of linux/kconfig.h:
>
> "Note that these only work with boolean and tristate options."

Ah, ok, I see you replied to my comment here. So when that config
option is not defined at all, the build still works, right? One way to
test that would be to use buildman tool for all boards. Although I
have to say it might be very time consuming (I usually run it
overnight), so I'm not asking you to do it in this case, it's just
FYI.

>
> How was this patch series tested?
> ---------------------------------
>
> I built for my target with CONFIG_ANDROID_AB_BACKUP_OFFSET set to
> 0x1000. I first verified that the device can normally boot. I then ran
> dd before rebooting to corrupt the primary data. I confirmed that the
> backup was used to properly restore the primary. I then corrupted both
> the primary and backup data and confirmed that the primary and backup
> were both reinitialized to default values. Lastly, I corrupted the
> backup data and not the primary data and confirmed that the backup was
> restored the primary data.
>
> Addtionally, I disabled CONFIG_ANDROID_AB_BACKUP_OFFSET for my device
> and confirmed that after I corrupt the primary data, no backup is used.
> When the primary data is not corrupt, the device boots normally. This is
> what I would expect, because CONFIG_ANDROID_AB_BACKUP_OFFSET's default
> value is 0x0, which would evaluate to false for all C if conditions.
>
> Colin McAllister (2):
>   android_ab: Add missing semicolon
>   android_ab: Fix ANDROID_AB_BACKUP_OFFSET
>
>  boot/android_ab.c | 97 ++++++++++++++++++++++-------------------------
>  1 file changed, 45 insertions(+), 52 deletions(-)
>
> --
> 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.

Re: [PATCH v3 0/2] Fix Android A/B backup

[Sam Protsenko]

On Fri, Mar 8, 2024 at 1:24 PM McAllister, Colin
<Colin.McAllister@garmin.com> wrote:
>
> > Ah, ok, I see you replied to my comment here.
>
> Yes, sorry. Outlook is terrible to send inline responses too. I figured
> just adding responses in the patch contents would be better. Next time I'll submit
> my patch with a different email :)
>
> > So when that config option is not defined at all, the build still
> > works, right?
>
> Yes, the default value for CONFIG_ANDROID_AB_BACKUP_OFFSET is 0x0, which
> would evaluate to a false bool value in the if conditions. I did do some
> testing with the config value not defined for my board and confirmed
> back-up data is not used.
>

Looks good to me, thanks.

> In your other emails you include your reviewed-by tag. For clarity, Am I
> supposed to append my patches and upload a new version? This is my
> first time contributing to u-boot, so I'm still learning the workflow. I
> didn't see anything glancing through the "Sending patches" page in the
> U-Boot documentation.
>

Welcome to the community! And thanks for your patches :) U-Boot
workflow is quite similar to Linux kernel one. It's useful to collect
all tags when sending out your next version. When the maintainer takes
your patch, they usually also apply all R-b tags for the final patch
version, so you only have to worry about that when sending out a new
version. I know that U-Boot contributors are often using `patman' tool
[1] for submitting patches (and corresponding updated versions), and
I'm pretty sure it collects all pending tags automatically for you.
FWIW, I'm not experienced with `patman', as I'm trying to use somehow
unified submitting process for both U-Boot and Linux kernel, and I
know that using `patman' is sometimes discouraged in Linux kernel
community.

[1] https://docs.u-boot.org/en/latest/develop/patman.html

[snip]

Re: [PATCH v3 0/2] Fix Android A/B backup

[Mattijs Korpershoek]

Hi Colin,

On ven., mars 08, 2024 at 15:59, Sam Protsenko <semen.protsenko@linaro.org> wrote:

> On Fri, Mar 8, 2024 at 1:24 PM McAllister, Colin
> <Colin.McAllister@garmin.com> wrote:
>>
>> > Ah, ok, I see you replied to my comment here.
>>
>> Yes, sorry. Outlook is terrible to send inline responses too. I figured
>> just adding responses in the patch contents would be better. Next time I'll submit
>> my patch with a different email :)
>>
>> > So when that config option is not defined at all, the build still
>> > works, right?
>>
>> Yes, the default value for CONFIG_ANDROID_AB_BACKUP_OFFSET is 0x0, which
>> would evaluate to a false bool value in the if conditions. I did do some
>> testing with the config value not defined for my board and confirmed
>> back-up data is not used.
>>
>
> Looks good to me, thanks.
>
>> In your other emails you include your reviewed-by tag. For clarity, Am I
>> supposed to append my patches and upload a new version? This is my
>> first time contributing to u-boot, so I'm still learning the workflow. I
>> didn't see anything glancing through the "Sending patches" page in the
>> U-Boot documentation.
>>
>
> Welcome to the community! And thanks for your patches :) U-Boot
> workflow is quite similar to Linux kernel one. It's useful to collect
> all tags when sending out your next version. When the maintainer takes
> your patch, they usually also apply all R-b tags for the final patch
> version, so you only have to worry about that when sending out a new
> version. I know that U-Boot contributors are often using `patman' tool
> [1] for submitting patches (and corresponding updated versions), and
> I'm pretty sure it collects all pending tags automatically for you.
> FWIW, I'm not experienced with `patman', as I'm trying to use somehow
> unified submitting process for both U-Boot and Linux kernel, and I
> know that using `patman' is sometimes discouraged in Linux kernel
> community.

Welcome to the U-Boot community! It takes quite some time to start
contributing so thank you for the patches.

The changes look fine and the detailed approach on how you tested is
very much appreciated.

I have a couple of suggestions on the whole series.

1. The patches don't apply:

$ b4 shazam -s -l 20240308165937.270499-1-colin.mcallister@garmin.com

error: patch failed: boot/android_ab.c:187
error: boot/android_ab.c: patch does not apply
error: Did you hand edit your patch?
It does not apply to blobs recorded in its index.
Patch failed at 0002 android_ab: Fix ANDROID_AB_BACKUP_OFFSET
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

I suspect your email provider swapped tabs to spaces. It's also possible
that the Garmin confidentiality footer caused this.

2. Using the khadas-vim3_android_ab_defconfig, this does not build:

boot/android_ab.c: In function 'ab_select_slot':
boot/android_ab.c:350:48: error: 'ANDROID_AB_BACKUP_OFFSET' undeclared (first use in this function); did you mean 'CONFIG_ANDROID_AB_BACKUP_OFFSET'?
  350 |                                                ANDROID_AB_BACKUP_OFFSET);
      |                                                ^~~~~~~~~~~~~~~~~~~~~~~~
      |                                                CONFIG_ANDROID_AB_BACKUP_OFFSET

Both are minor problems.
I re-applied the diffs manually to be able to build/boot test this.

Since this is your first contribution, I can either:
- fix both myself and merge this.
- let you spin a v4 to fix the above.

Please let me know what you prefer.

If you do intend to send a v4, please:
- Do it in a new email thread
- Make sure you cc me, Igor and Sam
- Make sure the patches you send can be applied.
  http://git-send-email.io/ is a friendly service you can use to test
  your workflow.

On workflow, tooling, I usually suggest the b4 tool:
https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1

Regards,
Mattijs

>
> [1] https://docs.u-boot.org/en/latest/develop/patman.html
>
> [snip]

Re: [PATCH v2 1/2] android_ab: Add missing semicolon

[Mattijs Korpershoek]

On jeu., mars 07, 2024 at 16:54, Sam Protsenko <semen.protsenko@linaro.org> wrote:

> On Thu, Mar 7, 2024 at 4:11 PM Colin McAllister
> <colin.mcallister@garmin.com> wrote:
>>
>> Found a missing semicolon in code protected by a #if that will never
>> evaluate to true due to a separate issue. Fixing this issue before
>> addressing the #if.
>>
>> 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 | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/boot/android_ab.c b/boot/android_ab.c
>> index c9df6d2b4b..9a3d15ec60 100644
>> --- a/boot/android_ab.c
>> +++ b/boot/android_ab.c
>> @@ -221,7 +221,7 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>>  #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("ANDROID: Invalid backup CRC-32 ");
>
> Good catch! Wonder why there is also a trailing space in the end of
> the string. Anyways, I think this patch deserves "Fixes:" tag, would
> you mind adding it? Other than that:

The file seems to have weird trailing/leading whitespaces in some of the
error messages.

I think it's fine to keep this as is for now.

We can make another series to fix this after Colin's work gets merged.

>
> Reviewed-by: Sam Protsenko <semen.protsenko@linaro.org>
>
>>                         log_err("expected %.8x, found %.8x),",
>>                                 crc32_le, backup_abc->crc32_le);
>>  #endif
>> --
>> 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.

RE: [PATCH v3 0/2] Fix Android A/B backup

[McAllister, Colin]

Hi Mattijs,

I’ve been using git send-email, but there might be issues with what the Garmin smtp server is doing to the email, like adding the footer. I sent a v4 PS in a new thread using my personal email, but that email isn’t subscribed to this ML so I think the patches are pending approval to be added to the ML.

Best,
Colin

From: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Sent: Tuesday, March 12, 2024 4:47 AM
To: Sam Protsenko <semen.protsenko@linaro.org>; McAllister, Colin <Colin.McAllister@garmin.com>
Cc: u-boot@lists.denx.de; JPEWhacker@gmail.com; sjg@chromium.org; Igor Opaniuk <igor.opaniuk@gmail.com>
Subject: Re: [PATCH v3 0/2] Fix Android A/B backup

Hi Colin, On ven. , mars 08, 2024 at 15: 59, Sam Protsenko <semen. protsenko@ linaro. org> wrote: > On Fri, Mar 8, 2024 at 1: 24 PM McAllister, Colin > <Colin. McAllister@ garmin. com> wrote: >> >> > Ah, ok, I see you


Hi Colin,



On ven., mars 08, 2024 at 15:59, Sam Protsenko <semen.protsenko@linaro.org<mailto:semen.protsenko@linaro.org>> wrote:



> On Fri, Mar 8, 2024 at 1:24 PM McAllister, Colin

> <Colin.McAllister@garmin.com<mailto:Colin.McAllister@garmin.com>> wrote:

>>

>> > Ah, ok, I see you replied to my comment here.

>>

>> Yes, sorry. Outlook is terrible to send inline responses too. I figured

>> just adding responses in the patch contents would be better. Next time I'll submit

>> my patch with a different email :)

>>

>> > So when that config option is not defined at all, the build still

>> > works, right?

>>

>> Yes, the default value for CONFIG_ANDROID_AB_BACKUP_OFFSET is 0x0, which

>> would evaluate to a false bool value in the if conditions. I did do some

>> testing with the config value not defined for my board and confirmed

>> back-up data is not used.

>>

>

> Looks good to me, thanks.

>

>> In your other emails you include your reviewed-by tag. For clarity, Am I

>> supposed to append my patches and upload a new version? This is my

>> first time contributing to u-boot, so I'm still learning the workflow. I

>> didn't see anything glancing through the "Sending patches" page in the

>> U-Boot documentation.

>>

>

> Welcome to the community! And thanks for your patches :) U-Boot

> workflow is quite similar to Linux kernel one. It's useful to collect

> all tags when sending out your next version. When the maintainer takes

> your patch, they usually also apply all R-b tags for the final patch

> version, so you only have to worry about that when sending out a new

> version. I know that U-Boot contributors are often using `patman' tool

> [1] for submitting patches (and corresponding updated versions), and

> I'm pretty sure it collects all pending tags automatically for you.

> FWIW, I'm not experienced with `patman', as I'm trying to use somehow

> unified submitting process for both U-Boot and Linux kernel, and I

> know that using `patman' is sometimes discouraged in Linux kernel

> community.



Welcome to the U-Boot community! It takes quite some time to start

contributing so thank you for the patches.



The changes look fine and the detailed approach on how you tested is

very much appreciated.



I have a couple of suggestions on the whole series.



1. The patches don't apply:



$ b4 shazam -s -l 20240308165937.270499-1-colin.mcallister@garmin.com<mailto:20240308165937.270499-1-colin.mcallister@garmin.com>



error: patch failed: boot/android_ab.c:187

error: boot/android_ab.c: patch does not apply

error: Did you hand edit your patch?

It does not apply to blobs recorded in its index.

Patch failed at 0002 android_ab: Fix ANDROID_AB_BACKUP_OFFSET

hint: Use 'git am --show-current-patch=diff' to see the failed patch

When you have resolved this problem, run "git am --continue".

If you prefer to skip this patch, run "git am --skip" instead.

To restore the original branch and stop patching, run "git am --abort".



I suspect your email provider swapped tabs to spaces. It's also possible

that the Garmin confidentiality footer caused this.



2. Using the khadas-vim3_android_ab_defconfig, this does not build:



boot/android_ab.c: In function 'ab_select_slot':

boot/android_ab.c:350:48: error: 'ANDROID_AB_BACKUP_OFFSET' undeclared (first use in this function); did you mean 'CONFIG_ANDROID_AB_BACKUP_OFFSET'?

  350 |                                                ANDROID_AB_BACKUP_OFFSET);

      |                                                ^~~~~~~~~~~~~~~~~~~~~~~~

      |                                                CONFIG_ANDROID_AB_BACKUP_OFFSET



Both are minor problems.

I re-applied the diffs manually to be able to build/boot test this.



Since this is your first contribution, I can either:

- fix both myself and merge this.

- let you spin a v4 to fix the above.



Please let me know what you prefer.



If you do intend to send a v4, please:

- Do it in a new email thread

- Make sure you cc me, Igor and Sam

- Make sure the patches you send can be applied.

  https://urldefense.com/v3/__http://git-send-email.io/__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldw1zsFDgg$<https://urldefense.com/v3/__http:/git-send-email.io/__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldw1zsFDgg$> is a friendly service you can use to test

  your workflow.



On workflow, tooling, I usually suggest the b4 tool:

https://urldefense.com/v3/__https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8LdwuYhSjtw$<https://urldefense.com/v3/__https:/people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8LdwuYhSjtw$>



Regards,

Mattijs



>

> [1] https://urldefense.com/v3/__https://docs.u-boot.org/en/latest/develop/patman.html__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldyum8H6Ow$<https://urldefense.com/v3/__https:/docs.u-boot.org/en/latest/develop/patman.html__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldyum8H6Ow$>

>

> [snip]

RE: [PATCH v3 0/2] Fix Android A/B backup

[Mattijs Korpershoek]

Hi Colin,

On mar., mars 12, 2024 at 14:04, "McAllister, Colin" <Colin.McAllister@garmin.com> wrote:

> Hi Mattijs,
>
> I’ve been using git send-email, but there might be issues with what the Garmin smtp server is doing to the email, like adding the footer. I sent a v4 PS in a new thread using my personal email, but that email isn’t subscribed to this ML so I think the patches are pending approval to be added to the ML.

Yep, seems they got approved. I will follow-up on the v4 series.

>
> Best,
> Colin
>
> From: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Sent: Tuesday, March 12, 2024 4:47 AM
> To: Sam Protsenko <semen.protsenko@linaro.org>; McAllister, Colin <Colin.McAllister@garmin.com>
> Cc: u-boot@lists.denx.de; JPEWhacker@gmail.com; sjg@chromium.org; Igor Opaniuk <igor.opaniuk@gmail.com>
> Subject: Re: [PATCH v3 0/2] Fix Android A/B backup
>
> Hi Colin, On ven. , mars 08, 2024 at 15: 59, Sam Protsenko <semen. protsenko@ linaro. org> wrote: > On Fri, Mar 8, 2024 at 1: 24 PM McAllister, Colin > <Colin. McAllister@ garmin. com> wrote: >> >> > Ah, ok, I see you
>
>
> Hi Colin,
>
>
>
> On ven., mars 08, 2024 at 15:59, Sam Protsenko <semen.protsenko@linaro.org<mailto:semen.protsenko@linaro.org>> wrote:
>
>
>
>> On Fri, Mar 8, 2024 at 1:24 PM McAllister, Colin
>
>> <Colin.McAllister@garmin.com<mailto:Colin.McAllister@garmin.com>> wrote:
>
>>>
>
>>> > Ah, ok, I see you replied to my comment here.
>
>>>
>
>>> Yes, sorry. Outlook is terrible to send inline responses too. I figured
>
>>> just adding responses in the patch contents would be better. Next time I'll submit
>
>>> my patch with a different email :)
>
>>>
>
>>> > So when that config option is not defined at all, the build still
>
>>> > works, right?
>
>>>
>
>>> Yes, the default value for CONFIG_ANDROID_AB_BACKUP_OFFSET is 0x0, which
>
>>> would evaluate to a false bool value in the if conditions. I did do some
>
>>> testing with the config value not defined for my board and confirmed
>
>>> back-up data is not used.
>
>>>
>
>>
>
>> Looks good to me, thanks.
>
>>
>
>>> In your other emails you include your reviewed-by tag. For clarity, Am I
>
>>> supposed to append my patches and upload a new version? This is my
>
>>> first time contributing to u-boot, so I'm still learning the workflow. I
>
>>> didn't see anything glancing through the "Sending patches" page in the
>
>>> U-Boot documentation.
>
>>>
>
>>
>
>> Welcome to the community! And thanks for your patches :) U-Boot
>
>> workflow is quite similar to Linux kernel one. It's useful to collect
>
>> all tags when sending out your next version. When the maintainer takes
>
>> your patch, they usually also apply all R-b tags for the final patch
>
>> version, so you only have to worry about that when sending out a new
>
>> version. I know that U-Boot contributors are often using `patman' tool
>
>> [1] for submitting patches (and corresponding updated versions), and
>
>> I'm pretty sure it collects all pending tags automatically for you.
>
>> FWIW, I'm not experienced with `patman', as I'm trying to use somehow
>
>> unified submitting process for both U-Boot and Linux kernel, and I
>
>> know that using `patman' is sometimes discouraged in Linux kernel
>
>> community.
>
>
>
> Welcome to the U-Boot community! It takes quite some time to start
>
> contributing so thank you for the patches.
>
>
>
> The changes look fine and the detailed approach on how you tested is
>
> very much appreciated.
>
>
>
> I have a couple of suggestions on the whole series.
>
>
>
> 1. The patches don't apply:
>
>
>
> $ b4 shazam -s -l 20240308165937.270499-1-colin.mcallister@garmin.com<mailto:20240308165937.270499-1-colin.mcallister@garmin.com>
>
>
>
> error: patch failed: boot/android_ab.c:187
>
> error: boot/android_ab.c: patch does not apply
>
> error: Did you hand edit your patch?
>
> It does not apply to blobs recorded in its index.
>
> Patch failed at 0002 android_ab: Fix ANDROID_AB_BACKUP_OFFSET
>
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
>
> When you have resolved this problem, run "git am --continue".
>
> If you prefer to skip this patch, run "git am --skip" instead.
>
> To restore the original branch and stop patching, run "git am --abort".
>
>
>
> I suspect your email provider swapped tabs to spaces. It's also possible
>
> that the Garmin confidentiality footer caused this.
>
>
>
> 2. Using the khadas-vim3_android_ab_defconfig, this does not build:
>
>
>
> boot/android_ab.c: In function 'ab_select_slot':
>
> boot/android_ab.c:350:48: error: 'ANDROID_AB_BACKUP_OFFSET' undeclared (first use in this function); did you mean 'CONFIG_ANDROID_AB_BACKUP_OFFSET'?
>
>   350 |                                                ANDROID_AB_BACKUP_OFFSET);
>
>       |                                                ^~~~~~~~~~~~~~~~~~~~~~~~
>
>       |                                                CONFIG_ANDROID_AB_BACKUP_OFFSET
>
>
>
> Both are minor problems.
>
> I re-applied the diffs manually to be able to build/boot test this.
>
>
>
> Since this is your first contribution, I can either:
>
> - fix both myself and merge this.
>
> - let you spin a v4 to fix the above.
>
>
>
> Please let me know what you prefer.
>
>
>
> If you do intend to send a v4, please:
>
> - Do it in a new email thread
>
> - Make sure you cc me, Igor and Sam
>
> - Make sure the patches you send can be applied.
>
>   https://urldefense.com/v3/__http://git-send-email.io/__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldw1zsFDgg$<https://urldefense.com/v3/__http:/git-send-email.io/__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldw1zsFDgg$> is a friendly service you can use to test
>
>   your workflow.
>
>
>
> On workflow, tooling, I usually suggest the b4 tool:
>
> https://urldefense.com/v3/__https://people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8LdwuYhSjtw$<https://urldefense.com/v3/__https:/people.kernel.org/monsieuricon/sending-a-kernel-patch-with-b4-part-1__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8LdwuYhSjtw$>
>
>
>
> Regards,
>
> Mattijs
>
>
>
>>
>
>> [1] https://urldefense.com/v3/__https://docs.u-boot.org/en/latest/develop/patman.html__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldyum8H6Ow$<https://urldefense.com/v3/__https:/docs.u-boot.org/en/latest/develop/patman.html__;!!EJc4YC3iFmQ!SPI0-0rQ9vj-ywEEcv6DQJbMQAyFwaCwC-RJNoupu3oyPLgiELEAXx-cPq4j8G_SXOw5oXUX621ACOzFcWz8Ldyum8H6Ow$>
>
>>
>
>> [snip]