[PATCH v4 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET
Igor Opaniuk
igor.opaniuk at gmail.com
Wed Mar 13 18:22:06 CET 2024
Hi Colin,
On Tue, Mar 12, 2024 at 4:19 PM Mattijs Korpershoek <
mkorpershoek at baylibre.com> wrote:
> Hi Colin,
>
> Thank you for the patch.
>
> On mar., mars 12, 2024 at 07:57, Colin McAllister <colinmca242 at 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 at 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 at garmin.com>
> > Cc: Joshua Watt <JPEWhacker at gmail.com>
> > Cc: Simon Glass <sjg at chromium.org>
> > Signed-off-by: Colin McAllister <colinmca242 at gmail.com>
> Reviewed-by: Sam Protsenko <semen.protsenko at 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 at 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 at gmail.com>
--
Best regards - Atentamente - Meilleures salutations
Igor Opaniuk
mailto: igor.opaniuk at gmail.com
skype: igor.opanyuk
https://www.linkedin.com/in/iopaniuk <http://ua.linkedin.com/in/iopaniuk>
More information about the U-Boot
mailing list