Hi Guillaume,
Thank you for the patch.
On jeu., oct. 17, 2024 at 18:10, Guillaume La Roque <glaro...@baylibre.com>
wrote:
> Update android bootmeth to support non-A/B image.
> Enable AB support only when ANDROID_AB is enabled.
>
> Signed-off-by: Guillaume La Roque <glaro...@baylibre.com>
> ---
> boot/Kconfig | 1 -
> boot/bootmeth_android.c | 53
> ++++++++++++++++++++++++++++++++++------
> configs/am62x_a53_android.config | 1 +
> configs/sandbox_defconfig | 1 +
> 4 files changed, 47 insertions(+), 9 deletions(-)
>
> diff --git a/boot/Kconfig b/boot/Kconfig
> index 1d50a83a2d2f..fa0739ff05dd 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -500,7 +500,6 @@ config BOOTMETH_ANDROID
> bool "Bootdev support for Android"
> depends on X86 || ARM || SANDBOX
> depends on CMDLINE
> - select ANDROID_AB
> select ANDROID_BOOT_IMAGE
> select CMD_BCB
> imply CMD_FASTBOOT
> diff --git a/boot/bootmeth_android.c b/boot/bootmeth_android.c
> index 2e7f85e4a708..8fa4952df3f2 100644
> --- a/boot/bootmeth_android.c
> +++ b/boot/bootmeth_android.c
> @@ -29,6 +29,7 @@
> #define BCB_PART_NAME "misc"
> #define BOOT_PART_NAME "boot"
> #define VENDOR_BOOT_PART_NAME "vendor_boot"
> +#define SLOT_LEN 2
>
> /**
> * struct android_priv - Private data
> @@ -42,7 +43,7 @@
> */
> struct android_priv {
> enum android_boot_mode boot_mode;
> - char slot[2];
> + char *slot;
> u32 header_version;
> };
>
> @@ -71,7 +72,11 @@ static int scan_boot_part(struct udevice *blk, struct
> android_priv *priv)
> char *buf;
> int ret;
>
> - sprintf(partname, BOOT_PART_NAME "_%s", priv->slot);
> + if (priv->slot)
> + sprintf(partname, BOOT_PART_NAME "_%s", priv->slot);
> + else
> + sprintf(partname, BOOT_PART_NAME);
> +
> ret = part_get_info_by_name(desc, partname, &partition);
> if (ret < 0)
> return log_msg_ret("part info", ret);
> @@ -108,7 +113,11 @@ static int scan_vendor_boot_part(struct udevice *blk,
> struct android_priv *priv)
> char *buf;
> int ret;
>
> - sprintf(partname, VENDOR_BOOT_PART_NAME "_%s", priv->slot);
> + if (priv->slot)
> + sprintf(partname, VENDOR_BOOT_PART_NAME "_%s", priv->slot);
> + else
> + sprintf(partname, VENDOR_BOOT_PART_NAME);
> +
> ret = part_get_info_by_name(desc, partname, &partition);
> if (ret < 0)
> return log_msg_ret("part info", ret);
> @@ -142,6 +151,11 @@ static int android_read_slot_from_bcb(struct bootflow
> *bflow, bool decrement)
> char slot_suffix[3];
> int ret;
>
> + if (!CONFIG_IS_ENABLED(ANDROID_AB)) {
> + priv->slot = NULL;
> + return 0;
> + }
> +
> ret = part_get_info_by_name(desc, BCB_PART_NAME, &misc);
> if (ret < 0)
> return log_msg_ret("part", ret);
> @@ -150,6 +164,7 @@ static int android_read_slot_from_bcb(struct bootflow
> *bflow, bool decrement)
> if (ret < 0)
> return log_msg_ret("slot", ret);
>
> + priv->slot = malloc(SLOT_LEN);
> priv->slot[0] = BOOT_SLOT_NAME(ret);
> priv->slot[1] = '\0';
>
> @@ -274,7 +289,7 @@ static int android_read_bootflow(struct udevice *dev,
> struct bootflow *bflow)
> configure_serialno(bflow);
> configure_bootloader_version(bflow);
>
> - if (priv->boot_mode == ANDROID_BOOT_MODE_NORMAL) {
> + if (priv->boot_mode == ANDROID_BOOT_MODE_NORMAL && priv->slot) {
> ret = bootflow_cmdline_set_arg(bflow,
> "androidboot.force_normal_boot",
> "1", false);
> if (ret < 0) {
> @@ -323,14 +338,24 @@ static int read_slotted_partition(struct blk_desc
> *desc, const char *const name,
> {
> struct disk_partition partition;
> char partname[PART_NAME_LEN];
> + int partname_len;
Should be size_t since we compare it with the output of strlen().
> int ret;
> u32 n;
>
> /* Ensure name fits in partname it should be: <name>_<slot>\0 */
The comment is not valid for non A/B. Maybe we can update it?
/*
* Ensure name fits in partname.
* For A/B, it should be <name>_<slot>\0
* For non A/B, it should be <name>\0
*/
> - if (strlen(name) > (PART_NAME_LEN - 2 - 1))
> + if (CONFIG_IS_ENABLED(ANDROID_AB))
> + partname_len = PART_NAME_LEN - 2 - 1;
> + else
> + partname_len = PART_NAME_LEN - 1;
> +
> + if (strlen(name) > partname_len)
> return log_msg_ret("name too long", -EINVAL);
>
> - sprintf(partname, "%s_%s", name, slot);
> + if (slot)
> + sprintf(partname, "%s_%s", name, slot);
> + else
> + sprintf(partname, "%s", name);
> +
> ret = part_get_info_by_name(desc, partname, &partition);
> if (ret < 0)
> return log_msg_ret("part", ret);
> @@ -382,7 +407,7 @@ static int run_avb_verification(struct bootflow *bflow)
> AvbSlotVerifyData *out_data;
> enum avb_boot_state boot_state;
> char *extra_args;
> - char slot_suffix[3];
> + char *slot_suffix = "";
Why are we making this a char *?
Can't we keep slot_suffix[3] = ""; instead ?
It should work similarly and avoids using malloc() and free()
for a local variable.
> bool unlocked = false;
> int ret;
>
> @@ -390,7 +415,10 @@ static int run_avb_verification(struct bootflow *bflow)
> if (!avb_ops)
> return log_msg_ret("avb ops", -ENOMEM);
>
> - sprintf(slot_suffix, "_%s", priv->slot);
> + if (priv->slot) {
> + slot_suffix = malloc(3);
> + sprintf(slot_suffix, "_%s", priv->slot);
> + }
>
> ret = avb_ops->read_is_device_unlocked(avb_ops, &unlocked);
> if (ret != AVB_IO_RESULT_OK)
> @@ -427,12 +455,18 @@ static int run_avb_verification(struct bootflow *bflow)
> if (ret < 0)
> goto free_out_data;
>
> + if (priv->slot)
> + free(slot_suffix);
> +
> return 0;
>
> free_out_data:
> if (out_data)
> avb_slot_verify_data_free(out_data);
>
> + if (priv->slot)
> + free(slot_suffix);
> +
> return log_msg_ret("avb cmdline", ret);
> }
> #else
> @@ -480,6 +514,9 @@ static int boot_android_normal(struct bootflow *bflow)
> }
> set_abootimg_addr(loadaddr);
>
> + if (priv->slot)
> + free(priv->slot);
> +
> ret = bootm_boot_start(loadaddr, bflow->cmdline);
>
> return log_msg_ret("boot", ret);
> diff --git a/configs/am62x_a53_android.config
> b/configs/am62x_a53_android.config
> index adbe2b8e126f..2aca51e3a107 100644
> --- a/configs/am62x_a53_android.config
> +++ b/configs/am62x_a53_android.config
> @@ -11,6 +11,7 @@ CONFIG_RANDOM_UUID=y # Needed for FASTBOOT_CMD_OEM_FORMAT
> CONFIG_FASTBOOT_CMD_OEM_FORMAT=y
> # Enable Android boot flow
> CONFIG_BOOTMETH_ANDROID=y
> +CONFIG_ANDROID_AB=y
> CONFIG_SYS_BOOTM_LEN=0x4000000
> CONFIG_SYS_MALLOC_LEN=0x08000000
> CONFIG_AVB_VERIFY=y
> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index d111858082d5..6b8dedf20712 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -50,6 +50,7 @@ CONFIG_LOG_DEFAULT_LEVEL=6
> CONFIG_LOGF_FUNC=y
> CONFIG_DISPLAY_BOARDINFO_LATE=y
> CONFIG_STACKPROTECTOR=y
> +CONFIG_ANDROID_AB=y
> CONFIG_CMD_CPU=y
> CONFIG_CMD_LICENSE=y
> CONFIG_CMD_SMBIOS=y
>
> --
> 2.34.1
