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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox