From: Mattijs Korpershoek <mkorpershoek@baylibre.com>
To: Igor Opaniuk <igor.opaniuk@gmail.com>,
Colin McAllister <colinmca242@gmail.com>
Cc: u-boot@lists.denx.de,
Colin McAllister <colin.mcallister@garmin.com>,
JPEWhacker@gmail.com, sjg@chromium.org,
Sam Protsenko <semen.protsenko@linaro.org>
Subject: Re: [PATCH v4 2/2] android_ab: Fix ANDROID_AB_BACKUP_OFFSET
Date: Thu, 21 Mar 2024 10:29:34 +0100 [thread overview]
Message-ID: <87cyronda9.fsf@baylibre.com> (raw)
In-Reply-To: <CAByghJZjX863HRWbK7U5PPmaQ_vcuuBN9r+tqEqV7h3sL4HuFg@mail.gmail.com>
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>
next prev parent reply other threads:[~2024-03-21 9:29 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2024-03-21 12:19 ` Colin
2024-03-22 7:41 ` [PATCH v4 0/2] Fix Android A/B backup Mattijs Korpershoek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87cyronda9.fsf@baylibre.com \
--to=mkorpershoek@baylibre.com \
--cc=JPEWhacker@gmail.com \
--cc=colin.mcallister@garmin.com \
--cc=colinmca242@gmail.com \
--cc=igor.opaniuk@gmail.com \
--cc=semen.protsenko@linaro.org \
--cc=sjg@chromium.org \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.