* [PATCH v4 0/2] Fix Android A/B backup
@ 2024-03-12 12:57 Colin McAllister
2024-03-12 12:57 ` [PATCH v4 1/2] android_ab: Add missing semicolon Colin McAllister
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Colin McAllister @ 2024-03-12 12:57 UTC (permalink / raw)
To: u-boot
Cc: Colin McAllister, JPEWhacker, sjg, Sam Protsenko,
Mattijs Korpershoek, Igor Opaniuk
- 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(-)
--
2.34.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v4 1/2] android_ab: Add missing semicolon 2024-03-12 12:57 [PATCH v4 0/2] Fix Android A/B backup Colin McAllister @ 2024-03-12 12:57 ` Colin McAllister 2024-03-12 15:12 ` Mattijs Korpershoek 2024-03-13 17:18 ` Igor Opaniuk 2024-03-12 12:57 ` [PATCH v4 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET Colin McAllister 2024-03-22 7:41 ` [PATCH v4 0/2] Fix Android A/B backup Mattijs Korpershoek 2 siblings, 2 replies; 10+ messages in thread From: Colin McAllister @ 2024-03-12 12:57 UTC (permalink / raw) To: u-boot Cc: Colin McAllister, JPEWhacker, sjg, Sam Protsenko, Mattijs Korpershoek, Igor Opaniuk, Colin McAllister 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/2] android_ab: Add missing semicolon 2024-03-12 12:57 ` [PATCH v4 1/2] android_ab: Add missing semicolon Colin McAllister @ 2024-03-12 15:12 ` Mattijs Korpershoek 2024-03-13 17:18 ` Igor Opaniuk 1 sibling, 0 replies; 10+ messages in thread From: Mattijs Korpershoek @ 2024-03-12 15:12 UTC (permalink / raw) To: Colin McAllister, u-boot Cc: Colin McAllister, JPEWhacker, sjg, Sam Protsenko, Igor Opaniuk, Colin McAllister 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=fdub4K1uX5p33Jw@mail.gmail.com/ 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 1/2] android_ab: Add missing semicolon 2024-03-12 12:57 ` [PATCH v4 1/2] android_ab: Add missing semicolon Colin McAllister 2024-03-12 15:12 ` Mattijs Korpershoek @ 2024-03-13 17:18 ` Igor Opaniuk 1 sibling, 0 replies; 10+ messages in thread From: Igor Opaniuk @ 2024-03-13 17:18 UTC (permalink / raw) To: Colin McAllister Cc: u-boot, Colin McAllister, JPEWhacker, sjg, Sam Protsenko, Mattijs Korpershoek 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> -- 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v4 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET 2024-03-12 12:57 [PATCH v4 0/2] Fix Android A/B backup Colin McAllister 2024-03-12 12:57 ` [PATCH v4 1/2] android_ab: Add missing semicolon Colin McAllister @ 2024-03-12 12:57 ` Colin McAllister 2024-03-12 15:19 ` Mattijs Korpershoek 2024-03-22 7:41 ` [PATCH v4 0/2] Fix Android A/B backup Mattijs Korpershoek 2 siblings, 1 reply; 10+ messages in thread From: Colin McAllister @ 2024-03-12 12:57 UTC (permalink / raw) To: u-boot Cc: Colin McAllister, JPEWhacker, sjg, Sam Protsenko, Mattijs Korpershoek, Igor Opaniuk, Colin McAllister 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); -- 2.34.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET 2024-03-12 12:57 ` [PATCH v4 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET Colin McAllister @ 2024-03-12 15:19 ` Mattijs Korpershoek 2024-03-13 17:22 ` Igor Opaniuk 0 siblings, 1 reply; 10+ messages in thread From: Mattijs Korpershoek @ 2024-03-12 15:19 UTC (permalink / raw) To: Colin McAllister, u-boot Cc: Colin McAllister, JPEWhacker, sjg, Sam Protsenko, Igor Opaniuk, Colin McAllister 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=fdub4K1uX5p33Jw@mail.gmail.com/ 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); > 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 > + */ 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET 2024-03-12 15:19 ` Mattijs Korpershoek @ 2024-03-13 17:22 ` Igor Opaniuk 2024-03-21 9:29 ` Mattijs Korpershoek 0 siblings, 1 reply; 10+ messages in thread From: Igor Opaniuk @ 2024-03-13 17:22 UTC (permalink / raw) To: Colin McAllister Cc: u-boot, Colin McAllister, JPEWhacker, sjg, Sam Protsenko, Mattijs Korpershoek 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=fdub4K1uX5p33Jw@mail.gmail.com/ > > 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); > > 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 > > + */ > > 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 > 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET 2024-03-13 17:22 ` Igor Opaniuk @ 2024-03-21 9:29 ` Mattijs Korpershoek 2024-03-21 12:19 ` Colin 0 siblings, 1 reply; 10+ messages in thread From: Mattijs Korpershoek @ 2024-03-21 9:29 UTC (permalink / raw) To: Igor Opaniuk, Colin McAllister Cc: u-boot, Colin McAllister, JPEWhacker, sjg, Sam Protsenko 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=fdub4K1uX5p33Jw@mail.gmail.com/ >> >> 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); >> > 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 >> > + */ >> >> 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 >> > > 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET 2024-03-21 9:29 ` Mattijs Korpershoek @ 2024-03-21 12:19 ` Colin 0 siblings, 0 replies; 10+ messages in thread From: Colin @ 2024-03-21 12:19 UTC (permalink / raw) To: Mattijs Korpershoek Cc: Igor Opaniuk, u-boot, Colin McAllister, JPEWhacker, sjg, Sam Protsenko 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: > 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=fdub4K1uX5p33Jw@mail.gmail.com/ > >> > >> 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); > >> > 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 > >> > + */ > >> > >> 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 > >> > > > > 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 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v4 0/2] Fix Android A/B backup 2024-03-12 12:57 [PATCH v4 0/2] Fix Android A/B backup Colin McAllister 2024-03-12 12:57 ` [PATCH v4 1/2] android_ab: Add missing semicolon Colin McAllister 2024-03-12 12:57 ` [PATCH v4 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET Colin McAllister @ 2024-03-22 7:41 ` Mattijs Korpershoek 2 siblings, 0 replies; 10+ messages in thread From: Mattijs Korpershoek @ 2024-03-22 7:41 UTC (permalink / raw) To: u-boot, Colin McAllister; +Cc: JPEWhacker, sjg, Sam Protsenko, Igor Opaniuk 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/c044f89bb0c2683bcb14006b697ee6db2240f00d [2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/026e0cd31278bcafcfb35d21478a94fc5072f78c -- Mattijs ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-03-22 7:41 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-12 12:57 [PATCH v4 0/2] Fix Android A/B backup Colin McAllister 2024-03-12 12:57 ` [PATCH v4 1/2] android_ab: Add missing semicolon Colin McAllister 2024-03-12 15:12 ` Mattijs Korpershoek 2024-03-13 17:18 ` Igor Opaniuk 2024-03-12 12:57 ` [PATCH v4 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET Colin McAllister 2024-03-12 15:19 ` Mattijs Korpershoek 2024-03-13 17:22 ` Igor Opaniuk 2024-03-21 9:29 ` Mattijs Korpershoek 2024-03-21 12:19 ` Colin 2024-03-22 7:41 ` [PATCH v4 0/2] Fix Android A/B backup Mattijs Korpershoek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox