[PATCH v4 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET
Colin
colinmca242 at gmail.com
Thu Mar 21 13:19:47 CET 2024
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 at 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 at gmail.com>
> wrote:
>
> > 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