[PATCH v4 0/2] Fix Android A/B backup
- 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."
What's changed in V4? ---------------------
Nothing other than sending the patch in through a different email address in order to see if the Garmin SMTP server is borking the patches.
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(-)
From: Colin McAllister colin.mcallister@garmin.com
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 Signed-off-by: Colin McAllister colinmca242@gmail.com --- v2: No changes v3: Added "Fixes:" tag v4: No changes
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
Hi Colin,
Thank you for the patch.
On mar., mars 12, 2024 at 07:57, Colin McAllister colinmca242@gmail.com wrote:
From: Colin McAllister colin.mcallister@garmin.com
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 Signed-off-by: Colin McAllister colinmca242@gmail.com
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com
Since Sam gave his review in [1]: https://lore.kernel.org/all/CAPLW+4kHmPtfACyND4Vc2p0ZrsyGY=+bRU=fdub4K1uX5p3...
I will also add:
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
When applying.
v2: No changes v3: Added "Fixes:" tag v4: No changes
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.34.1
Hi Colin,
On Tue, Mar 12, 2024 at 1:57 PM Colin McAllister colinmca242@gmail.com wrote:
From: Colin McAllister colin.mcallister@garmin.com
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 Signed-off-by: Colin McAllister colinmca242@gmail.com
v2: No changes v3: Added "Fixes:" tag v4: No changes
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.34.1
Reviewed-by: Igor Opaniuk igor.opaniuk@gmail.com
From: Colin McAllister colin.mcallister@garmin.com
Currently, setting CONFIG_AB_BACKUP_OFFSET in a target's defconfig will not actually enable the #if protected code in android_ab.c. This is because "CONFIG_" should have been prepended to the config macro, or the macros defined in kconfig.h could have been used.
The code included by ANDROID_AB_BACKUP_OFFSET has been refactored to no longer be conditionally compiled by preprocessor conditionals and instead use C conditionals. This better aligns with the Linux kernel style guide.
Fixes: 3430f24bc6 ("android_ab: Try backup booloader_message") Signed-off-by: Colin McAllister colin.mcallister@garmin.com Cc: Joshua Watt JPEWhacker@gmail.com Cc: Simon Glass sjg@chromium.org Signed-off-by: Colin McAllister colinmca242@gmail.com --- v2: - Replaced #if conditionals with C if conditionals - Opted to use CONFIG_ANDROID_AB_BACKUP_OFFSET directly instead of macros in kconfig.h as CONFIG_ANDROID_AB_BACKUP_OFFSET is not a boolean or tristate value and doesn't have different values when building SPL or TPL. v3: - Added "Fixes:" tag v4: - No changes
boot/android_ab.c | 97 ++++++++++++++++++++++------------------------- 1 file changed, 45 insertions(+), 52 deletions(-)
diff --git a/boot/android_ab.c b/boot/android_ab.c index 9a3d15ec60..f547aa64e4 100644 --- a/boot/android_ab.c +++ b/boot/android_ab.c @@ -187,13 +187,12 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, bool dec_tries) { struct bootloader_control *abc = NULL; + struct bootloader_control *backup_abc = NULL; u32 crc32_le; int slot, i, ret; bool store_needed = false; + bool valid_backup = false; char slot_suffix[4]; -#if ANDROID_AB_BACKUP_OFFSET - struct bootloader_control *backup_abc = NULL; -#endif
ret = ab_control_create_from_disk(dev_desc, part_info, &abc, 0); if (ret < 0) { @@ -205,53 +204,49 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, return ret; }
-#if ANDROID_AB_BACKUP_OFFSET - ret = ab_control_create_from_disk(dev_desc, part_info, &backup_abc, - ANDROID_AB_BACKUP_OFFSET); - if (ret < 0) { - free(abc); - return ret; + if (CONFIG_ANDROID_AB_BACKUP_OFFSET) { + ret = ab_control_create_from_disk(dev_desc, part_info, &backup_abc, + CONFIG_ANDROID_AB_BACKUP_OFFSET); + if (ret < 0) { + free(abc); + return ret; + } } -#endif
crc32_le = ab_control_compute_crc(abc); if (abc->crc32_le != crc32_le) { log_err("ANDROID: Invalid CRC-32 (expected %.8x, found %.8x),", crc32_le, abc->crc32_le); -#if ANDROID_AB_BACKUP_OFFSET - crc32_le = ab_control_compute_crc(backup_abc); - if (backup_abc->crc32_le != crc32_le) { - log_err("ANDROID: Invalid backup CRC-32 "); - log_err("expected %.8x, found %.8x),", - crc32_le, backup_abc->crc32_le); -#endif - - log_err("re-initializing A/B metadata.\n"); + if (CONFIG_ANDROID_AB_BACKUP_OFFSET) { + crc32_le = ab_control_compute_crc(backup_abc); + if (backup_abc->crc32_le != crc32_le) { + log_err(" ANDROID: Invalid backup CRC-32 "); + log_err("(expected %.8x, found %.8x),", + crc32_le, backup_abc->crc32_le); + } else { + valid_backup = true; + log_info(" copying A/B metadata from backup.\n"); + memcpy(abc, backup_abc, sizeof(*abc)); + } + }
+ if (!valid_backup) { + log_err(" re-initializing A/B metadata.\n"); ret = ab_control_default(abc); if (ret < 0) { -#if ANDROID_AB_BACKUP_OFFSET - free(backup_abc); -#endif + if (CONFIG_ANDROID_AB_BACKUP_OFFSET) + free(backup_abc); free(abc); return -ENODATA; } -#if ANDROID_AB_BACKUP_OFFSET - } else { - /* - * Backup is valid. Copy it to the primary - */ - memcpy(abc, backup_abc, sizeof(*abc)); } -#endif store_needed = true; }
if (abc->magic != BOOT_CTRL_MAGIC) { log_err("ANDROID: Unknown A/B metadata: %.8x\n", abc->magic); -#if ANDROID_AB_BACKUP_OFFSET - free(backup_abc); -#endif + if (CONFIG_ANDROID_AB_BACKUP_OFFSET) + free(backup_abc); free(abc); return -ENODATA; } @@ -259,9 +254,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, if (abc->version > BOOT_CTRL_VERSION) { log_err("ANDROID: Unsupported A/B metadata version: %.8x\n", abc->version); -#if ANDROID_AB_BACKUP_OFFSET - free(backup_abc); -#endif + if (CONFIG_ANDROID_AB_BACKUP_OFFSET) + free(backup_abc); free(abc); return -ENODATA; } @@ -338,30 +332,29 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, abc->crc32_le = ab_control_compute_crc(abc); ret = ab_control_store(dev_desc, part_info, abc, 0); if (ret < 0) { -#if ANDROID_AB_BACKUP_OFFSET - free(backup_abc); -#endif + if (CONFIG_ANDROID_AB_BACKUP_OFFSET) + free(backup_abc); free(abc); return ret; } }
-#if ANDROID_AB_BACKUP_OFFSET - /* - * If the backup doesn't match the primary, write the primary - * to the backup offset - */ - if (memcmp(backup_abc, abc, sizeof(*abc)) != 0) { - ret = ab_control_store(dev_desc, part_info, abc, - ANDROID_AB_BACKUP_OFFSET); - if (ret < 0) { - free(backup_abc); - free(abc); - return ret; + if (CONFIG_ANDROID_AB_BACKUP_OFFSET) { + /* + * If the backup doesn't match the primary, write the primary + * to the backup offset + */ + 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);
Hi Colin,
Thank you for the patch.
On mar., mars 12, 2024 at 07:57, Colin McAllister colinmca242@gmail.com wrote:
Sam also gave his review here: https://lore.kernel.org/all/CAPLW+4kHmPtfACyND4Vc2p0ZrsyGY=+bRU=fdub4K1uX5p3...
Please include his review tag in the next submission.
I will add it at the appropriate place below:
From: Colin McAllister colin.mcallister@garmin.com
Currently, setting CONFIG_AB_BACKUP_OFFSET in a target's defconfig will not actually enable the #if protected code in android_ab.c. This is because "CONFIG_" should have been prepended to the config macro, or the macros defined in kconfig.h could have been used.
The code included by ANDROID_AB_BACKUP_OFFSET has been refactored to no longer be conditionally compiled by preprocessor conditionals and instead use C conditionals. This better aligns with the Linux kernel style guide.
Fixes: 3430f24bc6 ("android_ab: Try backup booloader_message") Signed-off-by: Colin McAllister colin.mcallister@garmin.com Cc: Joshua Watt JPEWhacker@gmail.com Cc: Simon Glass sjg@chromium.org Signed-off-by: Colin McAllister colinmca242@gmail.com
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
v2:
- Replaced #if conditionals with C if conditionals
- Opted to use CONFIG_ANDROID_AB_BACKUP_OFFSET directly instead of macros in kconfig.h as CONFIG_ANDROID_AB_BACKUP_OFFSET is not a
boolean or tristate value and doesn't have different values when building SPL or TPL. v3:
- Added "Fixes:" tag
v4:
- No changes
boot/android_ab.c | 97 ++++++++++++++++++++++------------------------- 1 file changed, 45 insertions(+), 52 deletions(-)
diff --git a/boot/android_ab.c b/boot/android_ab.c index 9a3d15ec60..f547aa64e4 100644 --- a/boot/android_ab.c +++ b/boot/android_ab.c @@ -187,13 +187,12 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, bool dec_tries) { struct bootloader_control *abc = NULL;
- struct bootloader_control *backup_abc = NULL; u32 crc32_le; int slot, i, ret; bool store_needed = false;
- bool valid_backup = false; char slot_suffix[4];
-#if ANDROID_AB_BACKUP_OFFSET
- struct bootloader_control *backup_abc = NULL;
-#endif
ret = ab_control_create_from_disk(dev_desc, part_info, &abc, 0); if (ret < 0) { @@ -205,53 +204,49 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, return ret; }
-#if ANDROID_AB_BACKUP_OFFSET
- ret = ab_control_create_from_disk(dev_desc, part_info, &backup_abc,
ANDROID_AB_BACKUP_OFFSET);- if (ret < 0) {
free(abc);return ret;
- if (CONFIG_ANDROID_AB_BACKUP_OFFSET) {
ret = ab_control_create_from_disk(dev_desc, part_info, &backup_abc,CONFIG_ANDROID_AB_BACKUP_OFFSET);if (ret < 0) {free(abc);return ret;}-#endif
crc32_le = ab_control_compute_crc(abc); if (abc->crc32_le != crc32_le) { log_err("ANDROID: Invalid CRC-32 (expected %.8x, found %.8x),", crc32_le, abc->crc32_le); -#if ANDROID_AB_BACKUP_OFFSET
crc32_le = ab_control_compute_crc(backup_abc);if (backup_abc->crc32_le != crc32_le) {log_err("ANDROID: Invalid backup CRC-32 ");log_err("expected %.8x, found %.8x),",crc32_le, backup_abc->crc32_le);-#endif
log_err("re-initializing A/B metadata.\n");
if (CONFIG_ANDROID_AB_BACKUP_OFFSET) {crc32_le = ab_control_compute_crc(backup_abc);if (backup_abc->crc32_le != crc32_le) {log_err(" ANDROID: Invalid backup CRC-32 ");log_err("(expected %.8x, found %.8x),",crc32_le, backup_abc->crc32_le);} else {valid_backup = true;log_info(" copying A/B metadata from backup.\n");memcpy(abc, backup_abc, sizeof(*abc));}}if (!valid_backup) {log_err(" re-initializing A/B metadata.\n"); ret = ab_control_default(abc); if (ret < 0) {-#if ANDROID_AB_BACKUP_OFFSET
free(backup_abc);-#endif
if (CONFIG_ANDROID_AB_BACKUP_OFFSET)free(backup_abc); free(abc); return -ENODATA; }-#if ANDROID_AB_BACKUP_OFFSET
} else {/** Backup is valid. Copy it to the primary*/memcpy(abc, backup_abc, sizeof(*abc));-#endif store_needed = true; }
if (abc->magic != BOOT_CTRL_MAGIC) { log_err("ANDROID: Unknown A/B metadata: %.8x\n", abc->magic); -#if ANDROID_AB_BACKUP_OFFSET
free(backup_abc);-#endif
if (CONFIG_ANDROID_AB_BACKUP_OFFSET)free(backup_abc);@@ -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);@@ -338,30 +332,29 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info, abc->crc32_le = ab_control_compute_crc(abc); ret = ab_control_store(dev_desc, part_info, abc, 0); if (ret < 0) { -#if ANDROID_AB_BACKUP_OFFSET
free(backup_abc);-#endif
if (CONFIG_ANDROID_AB_BACKUP_OFFSET)free(backup_abc); free(abc); return ret;-#if ANDROID_AB_BACKUP_OFFSET
- /*
* If the backup doesn't match the primary, write the primary* to the backup offset*/- if (memcmp(backup_abc, abc, sizeof(*abc)) != 0) {
ret = ab_control_store(dev_desc, part_info, abc,ANDROID_AB_BACKUP_OFFSET);if (ret < 0) {free(backup_abc);free(abc);return ret;
- if (CONFIG_ANDROID_AB_BACKUP_OFFSET) {
/** If the backup doesn't match the primary, write the primary* to the backup offset*/
checkpatch.pl seems to complain about this comment block:
WARNING: Block comments should align the * on each line #166: FILE: boot/android_ab.c:344: + /* + * If the backup doesn't match the primary, write the primary
To reproduce, run:
$ ./scripts/checkpatch.pl --strict --u-boot --git HEAD^..HEAD
With the above addressed, please send a v5 including:
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com
if (memcmp(backup_abc, abc, sizeof(*abc)) != 0) {ret = ab_control_store(dev_desc, part_info, abc,CONFIG_ANDROID_AB_BACKUP_OFFSET);if (ret < 0) {free(backup_abc);free(abc);return ret;}free(backup_abc);
- free(backup_abc);
-#endif
free(abc);
-- 2.34.1
Hi Colin,
On Tue, Mar 12, 2024 at 4:19 PM Mattijs Korpershoek < mkorpershoek@baylibre.com> wrote:
Hi Colin,
Thank you for the patch.
On mar., mars 12, 2024 at 07:57, Colin McAllister colinmca242@gmail.com wrote:
Sam also gave his review here:
https://lore.kernel.org/all/CAPLW+4kHmPtfACyND4Vc2p0ZrsyGY=+bRU=fdub4K1uX5p3...
Please include his review tag in the next submission.
I will add it at the appropriate place below:
From: Colin McAllister colin.mcallister@garmin.com
Currently, setting CONFIG_AB_BACKUP_OFFSET in a target's defconfig will not actually enable the #if protected code in android_ab.c. This is because "CONFIG_" should have been prepended to the config macro, or the macros defined in kconfig.h could have been used.
The code included by ANDROID_AB_BACKUP_OFFSET has been refactored to no longer be conditionally compiled by preprocessor conditionals and instead use C conditionals. This better aligns with the Linux kernel style guide.
Fixes: 3430f24bc6 ("android_ab: Try backup booloader_message") Signed-off-by: Colin McAllister colin.mcallister@garmin.com Cc: Joshua Watt JPEWhacker@gmail.com Cc: Simon Glass sjg@chromium.org Signed-off-by: Colin McAllister colinmca242@gmail.com
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
v2:
- Replaced #if conditionals with C if conditionals
- Opted to use CONFIG_ANDROID_AB_BACKUP_OFFSET directly instead of macros in kconfig.h as CONFIG_ANDROID_AB_BACKUP_OFFSET is not a boolean or tristate value and doesn't have different values when building SPL or TPL.
v3:
- Added "Fixes:" tag
v4:
- No changes
boot/android_ab.c | 97 ++++++++++++++++++++++------------------------- 1 file changed, 45 insertions(+), 52 deletions(-)
diff --git a/boot/android_ab.c b/boot/android_ab.c index 9a3d15ec60..f547aa64e4 100644 --- a/boot/android_ab.c +++ b/boot/android_ab.c @@ -187,13 +187,12 @@ int ab_select_slot(struct blk_desc *dev_desc,
struct disk_partition *part_info,
bool dec_tries){ struct bootloader_control *abc = NULL;
struct bootloader_control *backup_abc = NULL; u32 crc32_le; int slot, i, ret; bool store_needed = false;bool valid_backup = false; char slot_suffix[4];-#if ANDROID_AB_BACKUP_OFFSET
struct bootloader_control *backup_abc = NULL;-#endif
ret = ab_control_create_from_disk(dev_desc, part_info, &abc, 0); if (ret < 0) {@@ -205,53 +204,49 @@ int ab_select_slot(struct blk_desc *dev_desc,
struct disk_partition *part_info,
return ret; }-#if ANDROID_AB_BACKUP_OFFSET
ret = ab_control_create_from_disk(dev_desc, part_info, &backup_abc,ANDROID_AB_BACKUP_OFFSET);if (ret < 0) {free(abc);return ret;
if (CONFIG_ANDROID_AB_BACKUP_OFFSET) {ret = ab_control_create_from_disk(dev_desc, part_info,&backup_abc,
CONFIG_ANDROID_AB_BACKUP_OFFSET);
if (ret < 0) {free(abc);return ret;} }-#endif
crc32_le = ab_control_compute_crc(abc); if (abc->crc32_le != crc32_le) { log_err("ANDROID: Invalid CRC-32 (expected %.8x, found%.8x),",
crc32_le, abc->crc32_le);-#if ANDROID_AB_BACKUP_OFFSET
crc32_le = ab_control_compute_crc(backup_abc);if (backup_abc->crc32_le != crc32_le) {log_err("ANDROID: Invalid backup CRC-32 ");log_err("expected %.8x, found %.8x),",crc32_le, backup_abc->crc32_le);-#endif
log_err("re-initializing A/B metadata.\n");
if (CONFIG_ANDROID_AB_BACKUP_OFFSET) {crc32_le = ab_control_compute_crc(backup_abc);if (backup_abc->crc32_le != crc32_le) {log_err(" ANDROID: Invalid backup CRC-32");
log_err("(expected %.8x, found %.8x),",crc32_le, backup_abc->crc32_le);} else {valid_backup = true;log_info(" copying A/B metadata frombackup.\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 theprimary
* to the backup offset*/checkpatch.pl seems to complain about this comment block:
WARNING: Block comments should align the * on each line #166: FILE: boot/android_ab.c:344:
/** If the backup doesn't match the primary, write theprimary
To reproduce, run:
$ ./scripts/checkpatch.pl --strict --u-boot --git HEAD^..HEAD
With the above addressed, please send a v5 including:
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com
if (memcmp(backup_abc, abc, sizeof(*abc)) != 0) {ret = ab_control_store(dev_desc, part_info, abc,CONFIG_ANDROID_AB_BACKUP_OFFSET);
if (ret < 0) {free(backup_abc);free(abc);return ret;} }free(backup_abc); }
free(backup_abc);-#endif
free(abc);-- 2.34.1
With Mattijs comment addressed: Reviewed-by: Igor Opaniuk igor.opaniuk@gmail.com
Hi Colin,
Since both below remarks are minor nitpicks, I can also do them when applying (to avoid delaying this series too much).
Please le me know what you prefer: 1. You send a v5 at your convience 2. I do the minor fixups and I merge right away.
Again, thank you for doing your first U-Boot contributions!
Mattijs
On mer., mars 13, 2024 at 18:22, Igor Opaniuk igor.opaniuk@gmail.com wrote:
Hi Colin,
On Tue, Mar 12, 2024 at 4:19 PM Mattijs Korpershoek < mkorpershoek@baylibre.com> wrote:
Hi Colin,
Thank you for the patch.
On mar., mars 12, 2024 at 07:57, Colin McAllister colinmca242@gmail.com wrote:
Sam also gave his review here:
https://lore.kernel.org/all/CAPLW+4kHmPtfACyND4Vc2p0ZrsyGY=+bRU=fdub4K1uX5p3...
Please include his review tag in the next submission.
I will add it at the appropriate place below:
From: Colin McAllister colin.mcallister@garmin.com
Currently, setting CONFIG_AB_BACKUP_OFFSET in a target's defconfig will not actually enable the #if protected code in android_ab.c. This is because "CONFIG_" should have been prepended to the config macro, or the macros defined in kconfig.h could have been used.
The code included by ANDROID_AB_BACKUP_OFFSET has been refactored to no longer be conditionally compiled by preprocessor conditionals and instead use C conditionals. This better aligns with the Linux kernel style guide.
Fixes: 3430f24bc6 ("android_ab: Try backup booloader_message") Signed-off-by: Colin McAllister colin.mcallister@garmin.com Cc: Joshua Watt JPEWhacker@gmail.com Cc: Simon Glass sjg@chromium.org Signed-off-by: Colin McAllister colinmca242@gmail.com
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
v2:
- Replaced #if conditionals with C if conditionals
- Opted to use CONFIG_ANDROID_AB_BACKUP_OFFSET directly instead of macros in kconfig.h as CONFIG_ANDROID_AB_BACKUP_OFFSET is not a boolean or tristate value and doesn't have different values when building SPL or TPL.
v3:
- Added "Fixes:" tag
v4:
- No changes
boot/android_ab.c | 97 ++++++++++++++++++++++------------------------- 1 file changed, 45 insertions(+), 52 deletions(-)
diff --git a/boot/android_ab.c b/boot/android_ab.c index 9a3d15ec60..f547aa64e4 100644 --- a/boot/android_ab.c +++ b/boot/android_ab.c @@ -187,13 +187,12 @@ int ab_select_slot(struct blk_desc *dev_desc,
struct disk_partition *part_info,
bool dec_tries){ struct bootloader_control *abc = NULL;
struct bootloader_control *backup_abc = NULL; u32 crc32_le; int slot, i, ret; bool store_needed = false;bool valid_backup = false; char slot_suffix[4];-#if ANDROID_AB_BACKUP_OFFSET
struct bootloader_control *backup_abc = NULL;-#endif
ret = ab_control_create_from_disk(dev_desc, part_info, &abc, 0); if (ret < 0) {@@ -205,53 +204,49 @@ int ab_select_slot(struct blk_desc *dev_desc,
struct disk_partition *part_info,
return ret; }-#if ANDROID_AB_BACKUP_OFFSET
ret = ab_control_create_from_disk(dev_desc, part_info, &backup_abc,ANDROID_AB_BACKUP_OFFSET);if (ret < 0) {free(abc);return ret;
if (CONFIG_ANDROID_AB_BACKUP_OFFSET) {ret = ab_control_create_from_disk(dev_desc, part_info,&backup_abc,
CONFIG_ANDROID_AB_BACKUP_OFFSET);
if (ret < 0) {free(abc);return ret;} }-#endif
crc32_le = ab_control_compute_crc(abc); if (abc->crc32_le != crc32_le) { log_err("ANDROID: Invalid CRC-32 (expected %.8x, found%.8x),",
crc32_le, abc->crc32_le);-#if ANDROID_AB_BACKUP_OFFSET
crc32_le = ab_control_compute_crc(backup_abc);if (backup_abc->crc32_le != crc32_le) {log_err("ANDROID: Invalid backup CRC-32 ");log_err("expected %.8x, found %.8x),",crc32_le, backup_abc->crc32_le);-#endif
log_err("re-initializing A/B metadata.\n");
if (CONFIG_ANDROID_AB_BACKUP_OFFSET) {crc32_le = ab_control_compute_crc(backup_abc);if (backup_abc->crc32_le != crc32_le) {log_err(" ANDROID: Invalid backup CRC-32");
log_err("(expected %.8x, found %.8x),",crc32_le, backup_abc->crc32_le);} else {valid_backup = true;log_info(" copying A/B metadata frombackup.\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 theprimary
* to the backup offset*/checkpatch.pl seems to complain about this comment block:
WARNING: Block comments should align the * on each line #166: FILE: boot/android_ab.c:344:
/** If the backup doesn't match the primary, write theprimary
To reproduce, run:
$ ./scripts/checkpatch.pl --strict --u-boot --git HEAD^..HEAD
With the above addressed, please send a v5 including:
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com
if (memcmp(backup_abc, abc, sizeof(*abc)) != 0) {ret = ab_control_store(dev_desc, part_info, abc,CONFIG_ANDROID_AB_BACKUP_OFFSET);
if (ret < 0) {free(backup_abc);free(abc);return ret;} }free(backup_abc); }
free(backup_abc);-#endif
free(abc);-- 2.34.1
With Mattijs comment addressed: Reviewed-by: Igor Opaniuk igor.opaniuk@gmail.com
-- Best regards - Atentamente - Meilleures salutations
Igor Opaniuk
mailto: igor.opaniuk@gmail.com skype: igor.opanyuk https://www.linkedin.com/in/iopaniuk http://ua.linkedin.com/in/iopaniuk
Hi Mattijs,
Sorry, I did not realize there were outstanding issues for me to address. I would be happy to send a v5, but if you doing the fixups gets this merged quicker, that sounds better to me.
Happy to contribute, *_____________________* *Colin McAllister*
On Thu, Mar 21, 2024 at 4:29 AM Mattijs Korpershoek < mkorpershoek@baylibre.com> wrote:
Hi Colin,
Since both below remarks are minor nitpicks, I can also do them when applying (to avoid delaying this series too much).
Please le me know what you prefer:
- You send a v5 at your convience
- I do the minor fixups and I merge right away.
Again, thank you for doing your first U-Boot contributions!
Mattijs
On mer., mars 13, 2024 at 18:22, Igor Opaniuk igor.opaniuk@gmail.com wrote:
Hi Colin,
On Tue, Mar 12, 2024 at 4:19 PM Mattijs Korpershoek < mkorpershoek@baylibre.com> wrote:
Hi Colin,
Thank you for the patch.
On mar., mars 12, 2024 at 07:57, Colin McAllister <
colinmca242@gmail.com>
wrote:
Sam also gave his review here:
https://lore.kernel.org/all/CAPLW+4kHmPtfACyND4Vc2p0ZrsyGY=+bRU=fdub4K1uX5p3...
Please include his review tag in the next submission.
I will add it at the appropriate place below:
From: Colin McAllister colin.mcallister@garmin.com
Currently, setting CONFIG_AB_BACKUP_OFFSET in a target's defconfig
will
not actually enable the #if protected code in android_ab.c. This is because "CONFIG_" should have been prepended to the config macro, or
the
macros defined in kconfig.h could have been used.
The code included by ANDROID_AB_BACKUP_OFFSET has been refactored to
no
longer be conditionally compiled by preprocessor conditionals and instead use C conditionals. This better aligns with the Linux kernel style guide.
Fixes: 3430f24bc6 ("android_ab: Try backup booloader_message") Signed-off-by: Colin McAllister colin.mcallister@garmin.com Cc: Joshua Watt JPEWhacker@gmail.com Cc: Simon Glass sjg@chromium.org Signed-off-by: Colin McAllister colinmca242@gmail.com
Reviewed-by: Sam Protsenko semen.protsenko@linaro.org
v2:
- Replaced #if conditionals with C if conditionals
- Opted to use CONFIG_ANDROID_AB_BACKUP_OFFSET directly instead of macros in kconfig.h as CONFIG_ANDROID_AB_BACKUP_OFFSET is not a boolean or tristate value and doesn't have different values when building SPL or TPL.
v3:
- Added "Fixes:" tag
v4:
- No changes
boot/android_ab.c | 97
++++++++++++++++++++++-------------------------
1 file changed, 45 insertions(+), 52 deletions(-)
diff --git a/boot/android_ab.c b/boot/android_ab.c index 9a3d15ec60..f547aa64e4 100644 --- a/boot/android_ab.c +++ b/boot/android_ab.c @@ -187,13 +187,12 @@ int ab_select_slot(struct blk_desc *dev_desc,
struct disk_partition *part_info,
bool dec_tries){ struct bootloader_control *abc = NULL;
struct bootloader_control *backup_abc = NULL; u32 crc32_le; int slot, i, ret; bool store_needed = false;bool valid_backup = false; char slot_suffix[4];-#if ANDROID_AB_BACKUP_OFFSET
struct bootloader_control *backup_abc = NULL;-#endif
ret = ab_control_create_from_disk(dev_desc, part_info, &abc, 0); if (ret < 0) {@@ -205,53 +204,49 @@ int ab_select_slot(struct blk_desc *dev_desc,
struct disk_partition *part_info,
return ret; }-#if ANDROID_AB_BACKUP_OFFSET
ret = ab_control_create_from_disk(dev_desc, part_info,&backup_abc,
ANDROID_AB_BACKUP_OFFSET);if (ret < 0) {free(abc);return ret;
if (CONFIG_ANDROID_AB_BACKUP_OFFSET) {ret = ab_control_create_from_disk(dev_desc, part_info,&backup_abc,
CONFIG_ANDROID_AB_BACKUP_OFFSET);
if (ret < 0) {free(abc);return ret;} }-#endif
crc32_le = ab_control_compute_crc(abc); if (abc->crc32_le != crc32_le) { log_err("ANDROID: Invalid CRC-32 (expected %.8x, found%.8x),",
crc32_le, abc->crc32_le);-#if ANDROID_AB_BACKUP_OFFSET
crc32_le = ab_control_compute_crc(backup_abc);if (backup_abc->crc32_le != crc32_le) {log_err("ANDROID: Invalid backup CRC-32 ");log_err("expected %.8x, found %.8x),",crc32_le, backup_abc->crc32_le);-#endif
log_err("re-initializing A/B metadata.\n");
if (CONFIG_ANDROID_AB_BACKUP_OFFSET) {crc32_le = ab_control_compute_crc(backup_abc);if (backup_abc->crc32_le != crc32_le) {log_err(" ANDROID: Invalid backup CRC-32");
log_err("(expected %.8x, found %.8x),",crc32_le, backup_abc->crc32_le);} else {valid_backup = true;log_info(" copying A/B metadata frombackup.\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 theprimary
* to the backup offset*/checkpatch.pl seems to complain about this comment block:
WARNING: Block comments should align the * on each line #166: FILE: boot/android_ab.c:344:
/** If the backup doesn't match the primary, write theprimary
To reproduce, run:
$ ./scripts/checkpatch.pl --strict --u-boot --git HEAD^..HEAD
With the above addressed, please send a v5 including:
Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com
if (memcmp(backup_abc, abc, sizeof(*abc)) != 0) {ret = ab_control_store(dev_desc, part_info, abc,CONFIG_ANDROID_AB_BACKUP_OFFSET);
if (ret < 0) {free(backup_abc);free(abc);return ret;} }free(backup_abc); }
free(backup_abc);-#endif
free(abc);-- 2.34.1
With Mattijs comment addressed: Reviewed-by: Igor Opaniuk igor.opaniuk@gmail.com
-- Best regards - Atentamente - Meilleures salutations
Igor Opaniuk
mailto: igor.opaniuk@gmail.com skype: igor.opanyuk https://www.linkedin.com/in/iopaniuk <http://ua.linkedin.com/in/iopaniuk
Hi,
On Tue, 12 Mar 2024 07:57:27 -0500, Colin McAllister 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.
[...]
Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)
[1/2] android_ab: Add missing semicolon https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/c044f89bb0c2683... [2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/026e0cd31278bca...
-- Mattijs
participants (4)
-
Colin -
Colin McAllister
-
Igor Opaniuk -
Mattijs Korpershoek