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 < mkorpersh...@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.opan...@gmail.com> > wrote: > > > Hi Colin, > > > > On Tue, Mar 12, 2024 at 4:19 PM Mattijs Korpershoek < > > mkorpersh...@baylibre.com> wrote: > > > >> Hi Colin, > >> > >> Thank you for the patch. > >> > >> On mar., mars 12, 2024 at 07:57, Colin McAllister < > colinmca...@gmail.com> > >> wrote: > >> > >> Sam also gave his review here: > >> > >> > https://lore.kernel.org/all/CAPLW+4kHmPtfACyND4Vc2p0ZrsyGY=+bRU=fdub4k1ux5p3...@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.mcallis...@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.mcallis...@garmin.com> > >> > Cc: Joshua Watt <jpewhac...@gmail.com> > >> > Cc: Simon Glass <s...@chromium.org> > >> > Signed-off-by: Colin McAllister <colinmca...@gmail.com> > >> Reviewed-by: Sam Protsenko <semen.protse...@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 <mkorpersh...@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.opan...@gmail.com> > > > > -- > > Best regards - Atentamente - Meilleures salutations > > > > Igor Opaniuk > > > > mailto: igor.opan...@gmail.com > > skype: igor.opanyuk > > https://www.linkedin.com/in/iopaniuk <http://ua.linkedin.com/in/iopaniuk > > >