Hi Guillaume,
Thank you for the patch.
On jeu., oct. 17, 2024 at 18:10, Guillaume La Roque glaroque@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 glaroque@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