[U-Boot,3/7] common: Implement A/B metadata

This patch determines the A/B-specific bootloader message structure
that is the basis for implementation of recovery and A/B update
functions. A/B metadata is stored in this structure and used to decide
which slot should we use to boot the device. Also some basic functions
for A/B metadata manipulation are implemented (like slot selection).

The patch was extracted from commits [1], [2] with some coding style
fixes.

[1] https://android-review.googlesource.com/c/platform/external/u-boot/+/729878/2
[2] https://android-review.googlesource.com/c/platform/external/u-boot/+/729880/2

Signed-off-by: Ruslan Trofymenko <ruslan.trofymenko@linaro.org>
---
 common/Kconfig                       |  10 ++
 common/Makefile                      |   1 +
 common/android_ab.c                  | 278 +++++++++++++++++++++++++++++++++++
 include/android_ab.h                 |  34 +++++
 include/android_bootloader_message.h | 164 +++++++++++++++++++++
 5 files changed, 487 insertions(+)
 create mode 100644 common/android_ab.c
 create mode 100644 include/android_ab.h
 create mode 100644 include/android_bootloader_message.h

Hi Ruslan,

On Tue, 27 Nov 2018 at 12:57, Ruslan Trofymenko
<ruslan.trofymenko@linaro.org> wrote:
>
> This patch determines the A/B-specific bootloader message structure
> that is the basis for implementation of recovery and A/B update
> functions. A/B metadata is stored in this structure and used to decide
> which slot should we use to boot the device. Also some basic functions
> for A/B metadata manipulation are implemented (like slot selection).
>
> The patch was extracted from commits [1], [2] with some coding style
> fixes.
>
> [1] https://android-review.googlesource.com/c/platform/external/u-boot/+/729878/2
> [2] https://android-review.googlesource.com/c/platform/external/u-boot/+/729880/2
>
> Signed-off-by: Ruslan Trofymenko <ruslan.trofymenko@linaro.org>
> ---
>  common/Kconfig                       |  10 ++
>  common/Makefile                      |   1 +
>  common/android_ab.c                  | 278 +++++++++++++++++++++++++++++++++++
>  include/android_ab.h                 |  34 +++++
>  include/android_bootloader_message.h | 164 +++++++++++++++++++++
>  5 files changed, 487 insertions(+)
>  create mode 100644 common/android_ab.c
>  create mode 100644 include/android_ab.h
>  create mode 100644 include/android_bootloader_message.h
>
> diff --git a/common/Kconfig b/common/Kconfig
> index 57bd16d..0ff4679 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -748,6 +748,16 @@ config UPDATE_TFTP_MSEC_MAX
>         default 100
>         depends on UPDATE_TFTP
>
> +config ANDROID_AB
> +       bool "Android A/B updates"
> +       default n
> +       help
> +         If enabled, adds support for the new Android A/B update model. This
> +         allows the bootloader to select which slot to boot from based on the
> +         information provided by userspace via the Android boot_ctrl HAL. This
> +         allows a bootloader to try a new version of the system but roll back
> +         to previous version if the new one didn't boot all the way.
> +
>  endmenu
>
>  menu "Blob list"
> diff --git a/common/Makefile b/common/Makefile
> index 88079d1..a1a252c 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -104,6 +104,7 @@ endif
>  endif
>
>  obj-y += image.o
> +obj-$(CONFIG_ANDROID_AB) += android_ab.o
>  obj-$(CONFIG_ANDROID_BOOT_IMAGE) += image-android.o
>  obj-$(CONFIG_$(SPL_TPL_)OF_LIBFDT) += image-fdt.o
>  obj-$(CONFIG_$(SPL_TPL_)FIT) += image-fit.o
> diff --git a/common/android_ab.c b/common/android_ab.c
> new file mode 100644
> index 0000000..25daba7
> --- /dev/null
> +++ b/common/android_ab.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: BSD-2-Clause
> +/*
> + * Copyright (C) 2017 The Android Open Source Project
> + */
> +
> +#include <android_ab.h>

Please put common.h first

> +
> +#include <android_bootloader_message.h>
> +#include <common.h>
> +#include <memalign.h>
> +#include <u-boot/crc.h>
> +
> +/**
> + * Compute the CRC-32 of the bootloader control struct.
> + *
> + * Only the bytes up to the crc32_le field are considered for the CRC-32
> + * calculation.
> + */
> +static uint32_t ab_control_compute_crc(struct android_bootloader_control *abc)
> +{
> +       return crc32(0, (void *)abc, offsetof(typeof(*abc), crc32_le));
> +}
> +
> +/**
> + * Initialize android_bootloader_control to the default value.
> + *
> + * It allows us to boot all slots in order from the first one. This value
> + * should be used when the bootloader message is corrupted, but not when
> + * a valid message indicates that all slots are unbootable.
> + */
> +static void ab_control_default(struct android_bootloader_control *abc)
> +{
> +       int i;
> +       const struct android_slot_metadata metadata = {
> +               .priority = 15,
> +               .tries_remaining = 7,
> +               .successful_boot = 0,
> +               .verity_corrupted = 0,
> +               .reserved = 0
> +       };
> +
> +       memcpy(abc->slot_suffix, "a\0\0\0", 4);
> +       abc->magic = ANDROID_BOOT_CTRL_MAGIC;
> +       abc->version = ANDROID_BOOT_CTRL_VERSION;
> +       abc->nb_slot = ANDROID_NUM_SLOTS;
> +       memset(abc->reserved0, 0, sizeof(abc->reserved0));
> +       for (i = 0; i < abc->nb_slot; ++i)
> +               abc->slot_info[i] = metadata;
> +
> +       memset(abc->reserved1, 0, sizeof(abc->reserved1));
> +       abc->crc32_le = ab_control_compute_crc(abc);
> +}
> +
> +/**
> + * Load the boot_control struct from disk into newly allocated memory.
> + *
> + * This function allocates and returns an integer number of disk blocks,
> + * based on the block size of the passed device to help performing a
> + * read-modify-write operation on the boot_control struct.
> + * The boot_control struct offset (2 KiB) must be a multiple of the device
> + * block size, for simplicity.
> + *
> + * @param[in] dev_desc Device where to read the boot_control struct from
> + * @param[in] part_info Partition in 'dev_desc' where to read from, normally
> + *                     the "misc" partition should be used
> + * @return boot_control loaded from disk or NULL on error

You have different error types in this function. Can you change it to
return an int?

> + */
> +static struct android_bootloader_control *ab_control_create_from_disk(
> +               struct blk_desc *dev_desc,  const disk_partition_t *part_info)
> +{
> +       ulong abc_offset, abc_blocks;
> +       struct android_bootloader_control *abc;
> +
> +       abc_offset = offsetof(struct android_bootloader_message_ab,
> +                             slot_suffix);
> +       if (abc_offset % part_info->blksz) {
> +               printf("ANDROID: Boot control block not block aligned.\n");
> +               return NULL;
> +       }
> +       abc_offset /= part_info->blksz;
> +
> +       abc_blocks = DIV_ROUND_UP(sizeof(struct android_bootloader_control),
> +                                 part_info->blksz);
> +       if (abc_offset + abc_blocks > part_info->size) {
> +               printf("ANDROID: boot control partition too small. Need at");
> +               printf(" least %lu blocks but have %lu blocks.\n",
> +                      abc_offset + abc_blocks, part_info->size);
> +               return NULL;
> +       }
> +       abc = malloc_cache_aligned(abc_blocks * part_info->blksz);
> +       if (!abc)
> +               return NULL;
> +
> +       if (blk_dread(dev_desc, part_info->start + abc_offset, abc_blocks,
> +                     abc) != abc_blocks) {
> +               printf("ANDROID: Could not read from boot control partition\n");
> +               free(abc);
> +               return NULL;
> +       }
> +       debug("ANDROID: Loaded ABC, %lu blocks.\n", abc_blocks);
> +       return abc;
> +}
> +
> +/**
> + * Store the loaded boot_control block.
> + *
> + * Store back to the same location it was read from with
> + * ab_control_create_from_misc().
> + *
> + * @param[in] abc Pointer to the boot_control struct and the extra bytes after
> + *                it up to the nearest block boundary
> + * @param[in] dev_desc Device where we should write the boot_control struct
> + * @param[in] part_info Partition on the 'dev_desc' where to write
> + * @return 0 on success and -1 on error
> + */
> +static int ab_control_store(struct android_bootloader_control *abc,
> +                           struct blk_desc *dev_desc,
> +                           const disk_partition_t *part_info)
> +{
> +       ulong abc_offset, abc_blocks;
> +
> +       abc_offset = offsetof(struct android_bootloader_message_ab,
> +                             slot_suffix) / part_info->blksz;
> +       abc_blocks = DIV_ROUND_UP(sizeof(struct android_bootloader_control),
> +                                 part_info->blksz);
> +       if (blk_dwrite(dev_desc, part_info->start + abc_offset, abc_blocks,
> +                      abc) != abc_blocks) {
> +               printf("ANDROID: Could not write back the misc partition\n");
> +               return -1;

Please use an errno error number

> +       }
> +       return 0;
> +}
> +
> +/**
> + * Compare two slots.
> + *
> + * The function determines slot which is should we boot from among the two.
> + *
> + * @param[in] a The first bootable slot metadata
> + * @param[in] b The second bootable slot metadata
> + * @return Negative if the slot "a" is better, positive of the slot "b" is
> + *         better or 0 if they are equally good.
> + */
> +static int ab_compare_slots(const struct android_slot_metadata *a,
> +                           const struct android_slot_metadata *b)
> +{
> +       /* Higher priority is better */
> +       if (a->priority != b->priority)
> +               return b->priority - a->priority;
> +
> +       /* Higher successful_boot value is better, in case of same priority */
> +       if (a->successful_boot != b->successful_boot)
> +               return b->successful_boot - a->successful_boot;
> +
> +       /* Higher tries_remaining is better to ensure round-robin */
> +       if (a->tries_remaining != b->tries_remaining)
> +               return b->tries_remaining - a->tries_remaining;
> +
> +       return 0;
> +}
> +
> +int ab_select_slot(struct blk_desc *dev_desc, disk_partition_t *part_info)
> +{
> +       struct android_bootloader_control *abc;
> +       u32 crc32_le;
> +       int slot, i;
> +       bool store_needed = false;
> +       char slot_suffix[4];
> +
> +       abc = ab_control_create_from_disk(dev_desc, part_info);
> +       if (!abc) {
> +               /*
> +                * This condition represents an actual problem with the code or
> +                * the board setup, like an invalid partition information.
> +                * Signal a repair mode and do not try to boot from either slot.
> +                */
> +               return -1;
> +       }
> +
> +       crc32_le = ab_control_compute_crc(abc);
> +       if (abc->crc32_le != crc32_le) {
> +               printf("ANDROID: Invalid CRC-32 (expected %.8x, found %.8x), ",
> +                      crc32_le, abc->crc32_le);
> +               printf("re-initializing A/B metadata.\n");
> +               ab_control_default(abc);
> +               store_needed = true;
> +       }
> +
> +       if (abc->magic != ANDROID_BOOT_CTRL_MAGIC) {
> +               printf("ANDROID: Unknown A/B metadata: %.8x\n", abc->magic);
> +               free(abc);
> +               return -1;
> +       }
> +
> +       if (abc->version > ANDROID_BOOT_CTRL_VERSION) {
> +               printf("ANDROID: Unsupported A/B metadata version: %.8x\n",
> +                      abc->version);
> +               free(abc);
> +               return -1;
> +       }
> +
> +       /*
> +        * At this point a valid boot control metadata is stored in abc,
> +        * followed by other reserved data in the same block. We select a with
> +        * the higher priority slot that
> +        *  - is not marked as corrupted and
> +        *  - either has tries_remaining > 0 or successful_boot is true.
> +        * If the selected slot has a false successful_boot, we also decrement
> +        * the tries_remaining until it eventually becomes unbootable because
> +        * tries_remaining reaches 0. This mechanism produces a bootloader
> +        * induced rollback, typically right after a failed update.
> +        */
> +
> +       /* Safety check: limit the number of slots. */
> +       if (abc->nb_slot > ARRAY_SIZE(abc->slot_info)) {
> +               abc->nb_slot = ARRAY_SIZE(abc->slot_info);
> +               store_needed = true;
> +       }
> +
> +       slot = -1;
> +       for (i = 0; i < abc->nb_slot; ++i) {
> +               if (abc->slot_info[i].verity_corrupted ||
> +                   !abc->slot_info[i].tries_remaining) {
> +                       debug("ANDROID: unbootable slot %d tries: %d, ",
> +                             i, abc->slot_info[i].tries_remaining);
> +                       debug("corrupt: %d\n",
> +                             abc->slot_info[i].verity_corrupted);
> +                       continue;
> +               }
> +               debug("ANDROID: bootable slot %d pri: %d, tries: %d, ",
> +                     i, abc->slot_info[i].priority,
> +                     abc->slot_info[i].tries_remaining);

Consider using log_debug() with a suitable LOGC_... or UCLASS.

> +               debug("corrupt: %d, successful: %d\n",
> +                     abc->slot_info[i].verity_corrupted,
> +                     abc->slot_info[i].successful_boot);
> +
> +               if (slot < 0 ||
> +                   ab_compare_slots(&abc->slot_info[i],
> +                                    &abc->slot_info[slot]) < 0) {
> +                       slot = i;
> +               }
> +       }
> +
> +       if (slot >= 0 && !abc->slot_info[slot].successful_boot) {
> +               printf("ANDROID: Attempting slot %c, tries remaining %d\n",
> +                      ANDROID_BOOT_SLOT_NAME(slot),
> +                      abc->slot_info[slot].tries_remaining);
> +               abc->slot_info[slot].tries_remaining--;
> +               store_needed = true;
> +       }
> +
> +       if (slot >= 0) {
> +               /*
> +                * Legacy user-space requires this field to be set in the BCB.
> +                * Newer releases load this slot suffix from the command line
> +                * or the device tree.
> +                */
> +               memset(slot_suffix, 0, sizeof(slot_suffix));
> +               slot_suffix[0] = ANDROID_BOOT_SLOT_NAME(slot);
> +               if (memcmp(abc->slot_suffix, slot_suffix,
> +                          sizeof(slot_suffix))) {
> +                       memcpy(abc->slot_suffix, slot_suffix,
> +                              sizeof(slot_suffix));
> +                       store_needed = true;
> +               }
> +       }
> +
> +       if (store_needed) {
> +               abc->crc32_le = ab_control_compute_crc(abc);
> +               ab_control_store(abc, dev_desc, part_info);
> +       }
> +       free(abc);
> +
> +       if (slot < 0)
> +               return -1;
> +
> +       return slot;
> +}
> diff --git a/include/android_ab.h b/include/android_ab.h
> new file mode 100644
> index 0000000..53f6dc6
> --- /dev/null
> +++ b/include/android_ab.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Copyright (C) 2017 The Android Open Source Project
> + */
> +
> +#ifndef __ANDROID_AB_H
> +#define __ANDROID_AB_H
> +
> +#include <common.h>
> +
> +/* Android standard boot slot names are 'a', 'b', 'c', ... */
> +#define ANDROID_BOOT_SLOT_NAME(slot_num) ('a' + (slot_num))
> +
> +/* Number of slots */
> +#define ANDROID_NUM_SLOTS 2
> +
> +/**
> + * Select the slot where to boot from.
> + *
> + * On Android devices with more than one boot slot (multiple copies of the
> + * kernel and system images) selects which slot should be used to boot from and
> + * registers the boot attempt. This is used in by the new A/B update model where
> + * one slot is updated in the background while running from the other slot. If
> + * the selected slot did not successfully boot in the past, a boot attempt is
> + * registered before returning from this function so it isn't selected
> + * indefinitely.
> + *
> + * @param[in] dev_desc Place to store the device description pointer
> + * @param[in] part_info Place to store the partition information
> + * @return The slot number (>= 0) on success, or -1 on error
> + */
> +int ab_select_slot(struct blk_desc *dev_desc, disk_partition_t *part_info);
> +
> +#endif /* __ANDROID_AB_H */
> diff --git a/include/android_bootloader_message.h b/include/android_bootloader_message.h
> new file mode 100644
> index 0000000..1f19273
> --- /dev/null
> +++ b/include/android_bootloader_message.h

Too long. How about android_bl_msg.h ?

> @@ -0,0 +1,164 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * This file was taken from the AOSP Project.
> + * Repository: https://android.googlesource.com/platform/bootable/recovery/
> + * File: bootloader_message/include/bootloader_message/bootloader_message.h
> + * Commit: 8b309f6970ab3b7c53cc529c51a2cb44e1c7a7e1
> + *
> + * Copyright (C) 2008 The Android Open Source Project
> + */
> +
> +#ifndef __ANDROID_BOOTLOADER_MESSAGE_H
> +#define __ANDROID_BOOTLOADER_MESSAGE_H
> +
> +/*
> + * compiler.h defines the types that otherwise are included from stdint.h and
> + * stddef.h
> + */
> +#include <compiler.h>
> +
> +/*
> + * Spaces used by misc partition are as below:
> + * 0   - 2K     Bootloader Message
> + * 2K  - 16K    Used by Vendor's bootloader (the 2K - 4K range may be optionally
> + *              used as bootloader_message_ab struct)
> + * 16K - 64K    Used by uncrypt and recovery to store wipe_package for A/B
> + *              devices
> + * Note that these offsets are admitted by bootloader,recovery and uncrypt, so
> + * they are not configurable without changing all of them.
> + */
> +static const size_t ANDROID_BOOTLOADER_MESSAGE_OFFSET_IN_MISC;
> +static const size_t ANDROID_WIPE_PACKAGE_OFFSET_IN_MISC = 16 * 1024;

OMH way too long :-)

> +
> +/*
> + * Bootloader Message (2-KiB)
> + *
> + * This structure describes the content of a block in flash
> + * that is used for recovery and the bootloader to talk to
> + * each other.
> + *
> + * The command field is updated by linux when it wants to
> + * reboot into recovery or to update radio or bootloader firmware.
> + * It is also updated by the bootloader when firmware update
> + * is complete (to boot into recovery for any final cleanup)
> + *
> + * The status field is written by the bootloader after the
> + * completion of an "update-radio" or "update-hboot" command.
> + *
> + * The recovery field is only written by linux and used
> + * for the system to send a message to recovery or the
> + * other way around.
> + *
> + * The stage field is written by packages which restart themselves
> + * multiple times, so that the UI can reflect which invocation of the
> + * package it is.  If the value is of the format "#/#" (eg, "1/3"),
> + * the UI will add a simple indicator of that status.
> + *
> + * We used to have slot_suffix field for A/B boot control metadata in
> + * this struct, which gets unintentionally cleared by recovery or
> + * uncrypt. Move it into struct bootloader_message_ab to avoid the
> + * issue.
> + */
> +struct android_bootloader_message {

How about andr_bl_msg ? Similarly below


> +       char command[32];
> +       char status[32];
> +       char recovery[768];
> +
> +       /*
> +        * The 'recovery' field used to be 1024 bytes.  It has only ever
> +        * been used to store the recovery command line, so 768 bytes
> +        * should be plenty.  We carve off the last 256 bytes to store the
> +        * stage string (for multistage packages) and possible future
> +        * expansion.
> +        */
> +       char stage[32];
> +
> +       /*
> +        * The 'reserved' field used to be 224 bytes when it was initially
> +        * carved off from the 1024-byte recovery field. Bump it up to
> +        * 1184-byte so that the entire bootloader_message struct rounds up
> +        * to 2048-byte.
> +        */
> +       char reserved[1184];
> +};
> +
> +/*
> + * The A/B-specific bootloader message structure (4-KiB).
> + *
> + * We separate A/B boot control metadata from the regular bootloader
> + * message struct and keep it here. Everything that's A/B-specific
> + * stays after struct bootloader_message, which should be managed by
> + * the A/B-bootloader or boot control HAL.
> + *
> + * The slot_suffix field is used for A/B implementations where the
> + * bootloader does not set the androidboot.ro.boot.slot_suffix kernel
> + * commandline parameter. This is used by fs_mgr to mount /system and
> + * other partitions with the slotselect flag set in fstab. A/B
> + * implementations are free to use all 32 bytes and may store private
> + * data past the first NUL-byte in this field. It is encouraged, but
> + * not mandatory, to use 'struct bootloader_control' described below.
> + */
> +struct android_bootloader_message_ab {
> +       struct android_bootloader_message message;
> +       char slot_suffix[32];
> +
> +       /* Round up the entire struct to 4096-byte */
> +       char reserved[2016];
> +};
> +
> +#define ANDROID_BOOT_CTRL_MAGIC   0x42414342 /* Bootloader Control AB */
> +#define ANDROID_BOOT_CTRL_VERSION 1
> +
> +struct android_slot_metadata {
> +       /*
> +        * Slot priority with 15 meaning highest priority, 1 lowest
> +        * priority and 0 the slot is unbootable
> +        */
> +       u8 priority : 4;
> +       /* Number of times left attempting to boot this slot */
> +       u8 tries_remaining : 3;
> +       /* 1 if this slot has booted successfully, 0 otherwise */
> +       u8 successful_boot : 1;
> +       /*
> +        * 1 if this slot is corrupted from a dm-verity corruption,
> +        * 0 otherwise
> +        */
> +       u8 verity_corrupted : 1;
> +       /* Reserved for further use */
> +       u8 reserved : 7;
> +} __packed;
> +
> +/*
> + * Bootloader Control AB.
> + *
> + * This struct can be used to manage A/B metadata. It is designed to
> + * be put in the 'slot_suffix' field of the 'bootloader_message'
> + * structure described above. It is encouraged to use the
> + * 'bootloader_control' structure to store the A/B metadata, but not
> + * mandatory.
> + */
> +struct android_bootloader_control {
> +       /* NULL terminated active slot suffix */
> +       char slot_suffix[4];
> +       /* Bootloader Control AB magic number (see BOOT_CTRL_MAGIC) */
> +       u32 magic;
> +       /* Version of struct being used (see BOOT_CTRL_VERSION) */
> +       u8 version;
> +       /* Number of slots being managed */
> +       u8 nb_slot : 3;
> +       /* Number of times left attempting to boot recovery */
> +       u8 recovery_tries_remaining : 3;
> +       /* Ensure 4-bytes alignment for slot_info field */
> +       u8 reserved0[2];
> +       /* Per-slot information. Up to 4 slots */
> +       struct android_slot_metadata slot_info[4];
> +       /* Reserved for further use */
> +       u8 reserved1[8];
> +       /*
> +        * CRC32 of all 28 bytes preceding this field (little endian
> +        * format)
> +        */
> +       u32 crc32_le;
> +} __packed;
> +
> +#endif  /* __ANDROID_BOOTLOADER_MESSAGE_H */
> --
> 2.7.4
>

Regards,
Simon

Hi Simon, Igor, Ruslan,

On Thu, Dec 6, 2018 at 4:05 AM Simon Glass <sjg@chromium.org> wrote:
> Hi Ruslan,
>
> On Tue, 27 Nov 2018 at 12:57, Ruslan Trofymenko
> <ruslan.trofymenko@linaro.org> wrote:
> >
[..]
> > +struct android_bootloader_message {
>
> How about andr_bl_msg ? Similarly below

Simon, I am currently working on a new U-Boot command which requires
the same AOSP header in-tree. Since the v4 of the whole "A/B support"
series is still WIP by Igor (Ruslan?), may I kindly ask you whether
you feel strong about these specific header and struct renames? We've
recently got some feedback from Tom [1] that it should be OK to keep
the out-of-tree headers untouched. My main motivation is 1) minimizing
the effort of updating this specific header from upstream and 2) using
the U-Boot-compliant header/struct names in my WIP changes. I am open
minded if the original filename is not preserved, but the struct
renames imply some amount of changes in the comments (see [2]). Also,
renaming the structs will imply parsing and verifying the comments
each time the header is updated. It's this kind of tiny bit of
integration effort which you always want to avoid, since it doesn't
require any creativity and can't be automated easily. I am looking
forward for your feedback.

Dear Igor, Ruslan,

How should we handle the import of
bootloader_message/include/bootloader_message/bootloader_message.h ?
If it takes more time for you to submit the next version of A/B
support, would it be fine for you if I do the importing of the header
myself along with my other patches which depend on it?

[1] https://patchwork.ozlabs.org/patch/1044158/#2129429
[2] https://patchwork.ozlabs.org/patch/1044158/#2109299

Many thanks,
Eugeniu.

Hi Simon, Igor, All,

On Wed, Apr 03, 2019 at 07:55:30PM +0200, Eugeniu Rosca wrote:
> Hi Simon, Igor, Ruslan,
> 
> On Thu, Dec 6, 2018 at 4:05 AM Simon Glass <sjg@chromium.org> wrote:
> > Hi Ruslan,
> >
> > On Tue, 27 Nov 2018 at 12:57, Ruslan Trofymenko
> > <ruslan.trofymenko@linaro.org> wrote:
> > >
> [..]
> > > +struct android_bootloader_message {
> >
> > How about andr_bl_msg ? Similarly below
> 
> Simon, I am currently working on a new U-Boot command which requires
> the same AOSP header in-tree. Since the v4 of the whole "A/B support"
> series is still WIP by Igor (Ruslan?), may I kindly ask you whether
> you feel strong about these specific header and struct renames? We've
> recently got some feedback from Tom [1] that it should be OK to keep
> the out-of-tree headers untouched. My main motivation is 1) minimizing
> the effort of updating this specific header from upstream and 2) using
> the U-Boot-compliant header/struct names in my WIP changes. I am open
> minded if the original filename is not preserved, but the struct
> renames imply some amount of changes in the comments (see [2]). Also,
> renaming the structs will imply parsing and verifying the comments
> each time the header is updated. It's this kind of tiny bit of
> integration effort which you always want to avoid, since it doesn't
> require any creativity and can't be automated easily. I am looking
> forward for your feedback.
> 
> Dear Igor, Ruslan,
> 
> How should we handle the import of
> bootloader_message/include/bootloader_message/bootloader_message.h ?
> If it takes more time for you to submit the next version of A/B
> support, would it be fine for you if I do the importing of the header
> myself along with my other patches which depend on it?
> 
> [1] https://patchwork.ozlabs.org/patch/1044158/#2129429
> [2] https://patchwork.ozlabs.org/patch/1044158/#2109299

Apologize for making another iteration on this, but what about below
solution WRT struct renames upon importing bootloader_message.h in-tree.
The following typedef statements (placed in the imported header) would
allow keeping the original structures and the associated comments
untouched (hence speeding up the updates from the source), while U-Boot
would use use the new/rightmost types mirroring the upstream ones.

typedef struct bootloader_message       andr_bl_msg;
typedef struct bootloader_message_ab    andr_bl_msg_ab;
typedef struct bootloader_control       andr_bl_control;

> 
> Many thanks,
> Eugeniu.

Hi Eugeniu,

Sorry for the late reply.

Please go ahead,
I'm currently on a business trip, so I won't have access to my setup
with A/B till April 19 (so I definetely won't send any updates till
that time).

I'll sync with the patches you're going to send.

Thanks!

On Wed, Apr 3, 2019 at 7:55 PM Eugeniu Rosca <roscaeugeniu@gmail.com> wrote:
>
> Hi Simon, Igor, Ruslan,
>
> On Thu, Dec 6, 2018 at 4:05 AM Simon Glass <sjg@chromium.org> wrote:
> > Hi Ruslan,
> >
> > On Tue, 27 Nov 2018 at 12:57, Ruslan Trofymenko
> > <ruslan.trofymenko@linaro.org> wrote:
> > >
> [..]
> > > +struct android_bootloader_message {
> >
> > How about andr_bl_msg ? Similarly below
>
> Simon, I am currently working on a new U-Boot command which requires
> the same AOSP header in-tree. Since the v4 of the whole "A/B support"
> series is still WIP by Igor (Ruslan?), may I kindly ask you whether
> you feel strong about these specific header and struct renames? We've
> recently got some feedback from Tom [1] that it should be OK to keep
> the out-of-tree headers untouched. My main motivation is 1) minimizing
> the effort of updating this specific header from upstream and 2) using
> the U-Boot-compliant header/struct names in my WIP changes. I am open
> minded if the original filename is not preserved, but the struct
> renames imply some amount of changes in the comments (see [2]). Also,
> renaming the structs will imply parsing and verifying the comments
> each time the header is updated. It's this kind of tiny bit of
> integration effort which you always want to avoid, since it doesn't
> require any creativity and can't be automated easily. I am looking
> forward for your feedback.
>
> Dear Igor, Ruslan,
>
> How should we handle the import of
> bootloader_message/include/bootloader_message/bootloader_message.h ?
> If it takes more time for you to submit the next version of A/B
> support, would it be fine for you if I do the importing of the header
> myself along with my other patches which depend on it?
>
> [1] https://patchwork.ozlabs.org/patch/1044158/#2129429
> [2] https://patchwork.ozlabs.org/patch/1044158/#2109299
>
> Many thanks,
> Eugeniu.

Hi Igor,

On Sun, Apr 07, 2019 at 09:11:39PM +0200, Igor Opaniuk wrote:
> Hi Eugeniu,
> 
> Sorry for the late reply.
> 
> Please go ahead,
> I'm currently on a business trip, so I won't have access to my setup
> with A/B till April 19 (so I definetely won't send any updates till
> that time).
> 
> I'll sync with the patches you're going to send.

Thanks for feedback!

jFTR, here is the link to the newly submitted patches making use of
the same AOSP header: https://patchwork.ozlabs.org/cover/1080393/ .

Best regards,
Eugeniu.