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