* [PATCH v5 1/6] include/android_ab: move ab_select_slot() documentation to @ notation
2024-10-17 14:12 [PATCH v5 0/6] android_ab: introduce bcb ab_dump command and provide several bcb fixes Dmitry Rokosov
@ 2024-10-17 14:12 ` Dmitry Rokosov
2024-10-17 14:12 ` [PATCH v5 2/6] cmd: bcb: rework the command to U_BOOT_LONGHELP approach Dmitry Rokosov
` (7 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Dmitry Rokosov @ 2024-10-17 14:12 UTC (permalink / raw)
To: Igor Opaniuk, Mattijs Korpershoek, Sam Protsenko, Tom Rini,
Andrew F. Davis, Neil Armstrong, Simon Glass, Mario Six
Cc: u-boot, u-boot-amlogic, rockosov, kernel, Dmitry Rokosov,
Guillaume La Roque
There are new function documentation requirements in U-Boot, so apply
these changes for android_ab.
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Tested-by: Guillaume La Roque <glaroque@baylibre.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
boot/android_ab.c | 43 ++++++++++++++++++++++++-------------------
include/android_ab.h | 7 ++++---
2 files changed, 28 insertions(+), 22 deletions(-)
diff --git a/boot/android_ab.c b/boot/android_ab.c
index 1196a189ed5349ca5c088e3b6ef1796b11c69a84..0045c8133a8e164f1fdd4c0f9b683de0f13f26e0 100644
--- a/boot/android_ab.c
+++ b/boot/android_ab.c
@@ -13,13 +13,13 @@
#include <u-boot/crc.h>
/**
- * Compute the CRC-32 of the bootloader control struct.
+ * ab_control_compute_crc() - Compute the CRC32 of the bootloader control.
+ *
+ * @abc: Bootloader control block
*
* Only the bytes up to the crc32_le field are considered for the CRC-32
* calculation.
*
- * @param[in] abc bootloader control block
- *
* Return: crc32 sum
*/
static uint32_t ab_control_compute_crc(struct bootloader_control *abc)
@@ -28,14 +28,14 @@ static uint32_t ab_control_compute_crc(struct bootloader_control *abc)
}
/**
- * Initialize bootloader_control to the default value.
+ * ab_control_default() - Initialize bootloader_control to the default value.
+ *
+ * @abc: Bootloader control block
*
* 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.
*
- * @param[in] abc bootloader control block
- *
* Return: 0 on success and a negative on error
*/
static int ab_control_default(struct bootloader_control *abc)
@@ -67,7 +67,13 @@ static int ab_control_default(struct bootloader_control *abc)
}
/**
- * Load the boot_control struct from disk into newly allocated memory.
+ * ab_control_create_from_disk() - Load the boot_control from disk into memory.
+ *
+ * @dev_desc: Device where to read the boot_control struct from
+ * @part_info: Partition in 'dev_desc' where to read from, normally
+ * the "misc" partition should be used
+ * @abc: pointer to pointer to bootloader_control data
+ * @offset: boot_control struct offset
*
* This function allocates and returns an integer number of disk blocks,
* based on the block size of the passed device to help performing a
@@ -75,10 +81,6 @@ static int ab_control_default(struct bootloader_control *abc)
* 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
- * @param[out] pointer to pointer to bootloader_control data
* Return: 0 on success and a negative on error
*/
static int ab_control_create_from_disk(struct blk_desc *dev_desc,
@@ -122,15 +124,17 @@ static int ab_control_create_from_disk(struct blk_desc *dev_desc,
}
/**
- * Store the loaded boot_control block.
+ * ab_control_store() - Store the loaded boot_control block.
+ *
+ * @dev_desc: Device where we should write the boot_control struct
+ * @part_info: Partition on the 'dev_desc' where to write
+ * @abc Pointer to the boot control struct and the extra bytes after
+ * it up to the nearest block boundary
+ * @offset: boot_control struct offset
*
* Store back to the same location it was read from with
* ab_control_create_from_misc().
*
- * @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
- * @param[in] abc Pointer to the boot control struct and the extra bytes after
- * it up to the nearest block boundary
* Return: 0 on success and a negative on error
*/
static int ab_control_store(struct blk_desc *dev_desc,
@@ -160,12 +164,13 @@ static int ab_control_store(struct blk_desc *dev_desc,
}
/**
- * Compare two slots.
+ * ab_compare_slots() - Compare two slots.
+ *
+ * @a: The first bootable slot metadata
+ * @b: The second bootable slot metadata
*
* 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.
*/
diff --git a/include/android_ab.h b/include/android_ab.h
index dbf20343da62447a237ec845e216517d34455ad3..1e53879a25f145a9d18ac0a6553d8c217123aa6f 100644
--- a/include/android_ab.h
+++ b/include/android_ab.h
@@ -18,7 +18,10 @@ struct disk_partition;
#define NUM_SLOTS 2
/**
- * Select the slot where to boot from.
+ * ab_select_slot() - Select the slot where to boot from.
+ *
+ * @dev_desc: Place to store the device description pointer
+ * @part_info: Place to store the partition information
*
* 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
@@ -28,8 +31,6 @@ struct disk_partition;
* 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 a negative on error
*/
int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 2/6] cmd: bcb: rework the command to U_BOOT_LONGHELP approach
2024-10-17 14:12 [PATCH v5 0/6] android_ab: introduce bcb ab_dump command and provide several bcb fixes Dmitry Rokosov
2024-10-17 14:12 ` [PATCH v5 1/6] include/android_ab: move ab_select_slot() documentation to @ notation Dmitry Rokosov
@ 2024-10-17 14:12 ` Dmitry Rokosov
2024-10-22 8:44 ` Mattijs Korpershoek
2024-10-17 14:12 ` [PATCH v5 3/6] treewide: bcb: move ab_select command to bcb subcommands Dmitry Rokosov
` (6 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Dmitry Rokosov @ 2024-10-17 14:12 UTC (permalink / raw)
To: Igor Opaniuk, Mattijs Korpershoek, Sam Protsenko, Tom Rini,
Andrew F. Davis, Neil Armstrong, Simon Glass, Mario Six
Cc: u-boot, u-boot-amlogic, rockosov, kernel, Dmitry Rokosov
U_BOOT_LONGHELP and U_BOOT_CMD_WITH_SUBCMDS offer numerous advantages,
including:
- common argument restrictions checking
- automatic subcommand matching
- improved usage and help handling
By utilizing the U_BOOT_LONGHELP approach, we can reduce the amount of
command management code and describe commands more succinctly.
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
cmd/bcb.c | 151 ++++++++++++++++++--------------------------------------------
1 file changed, 43 insertions(+), 108 deletions(-)
diff --git a/cmd/bcb.c b/cmd/bcb.c
index 97a96c009641cc094645607ef833575f3c03fe4b..98b2a71087a27b1721f4bed4160095d65ec75402 100644
--- a/cmd/bcb.c
+++ b/cmd/bcb.c
@@ -16,15 +16,6 @@
#include <vsprintf.h>
#include <linux/err.h>
-enum bcb_cmd {
- BCB_CMD_LOAD,
- BCB_CMD_FIELD_SET,
- BCB_CMD_FIELD_CLEAR,
- BCB_CMD_FIELD_TEST,
- BCB_CMD_FIELD_DUMP,
- BCB_CMD_STORE,
-};
-
static const char * const fields[] = {
"command",
"status",
@@ -38,67 +29,9 @@ static struct disk_partition partition_data;
static struct blk_desc *block;
static struct disk_partition *partition = &partition_data;
-static int bcb_cmd_get(char *cmd)
-{
- if (!strcmp(cmd, "load"))
- return BCB_CMD_LOAD;
- if (!strcmp(cmd, "set"))
- return BCB_CMD_FIELD_SET;
- if (!strcmp(cmd, "clear"))
- return BCB_CMD_FIELD_CLEAR;
- if (!strcmp(cmd, "test"))
- return BCB_CMD_FIELD_TEST;
- if (!strcmp(cmd, "store"))
- return BCB_CMD_STORE;
- if (!strcmp(cmd, "dump"))
- return BCB_CMD_FIELD_DUMP;
- else
- return -1;
-}
-
-static int bcb_is_misused(int argc, char *const argv[])
+static int bcb_not_loaded(void)
{
- int cmd = bcb_cmd_get(argv[0]);
-
- switch (cmd) {
- case BCB_CMD_LOAD:
- if (argc != 3 && argc != 4)
- goto err;
- break;
- case BCB_CMD_FIELD_SET:
- if (argc != 3)
- goto err;
- break;
- case BCB_CMD_FIELD_TEST:
- if (argc != 4)
- goto err;
- break;
- case BCB_CMD_FIELD_CLEAR:
- if (argc != 1 && argc != 2)
- goto err;
- break;
- case BCB_CMD_STORE:
- if (argc != 1)
- goto err;
- break;
- case BCB_CMD_FIELD_DUMP:
- if (argc != 2)
- goto err;
- break;
- default:
- printf("Error: 'bcb %s' not supported\n", argv[0]);
- return -1;
- }
-
- if (cmd != BCB_CMD_LOAD && !block) {
- printf("Error: Please, load BCB first!\n");
- return -1;
- }
-
- return 0;
-err:
- printf("Error: Bad usage of 'bcb %s'\n", argv[0]);
-
+ printf("Error: Please, load BCB first!\n");
return -1;
}
@@ -216,6 +149,9 @@ static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc,
char *endp;
char *iface = "mmc";
+ if (argc < 3)
+ return CMD_RET_USAGE;
+
if (argc == 4) {
iface = argv[1];
argc--;
@@ -270,6 +206,12 @@ static int __bcb_set(const char *fieldp, const char *valp)
static int do_bcb_set(struct cmd_tbl *cmdtp, int flag, int argc,
char * const argv[])
{
+ if (argc < 3)
+ return CMD_RET_USAGE;
+
+ if (!block)
+ return bcb_not_loaded();
+
return __bcb_set(argv[1], argv[2]);
}
@@ -279,6 +221,9 @@ static int do_bcb_clear(struct cmd_tbl *cmdtp, int flag, int argc,
int size;
char *field;
+ if (!block)
+ return bcb_not_loaded();
+
if (argc == 1) {
memset(&bcb, 0, sizeof(bcb));
return CMD_RET_SUCCESS;
@@ -297,7 +242,15 @@ static int do_bcb_test(struct cmd_tbl *cmdtp, int flag, int argc,
{
int size;
char *field;
- char *op = argv[2];
+ char *op;
+
+ if (argc < 4)
+ return CMD_RET_USAGE;
+
+ if (!block)
+ return bcb_not_loaded();
+
+ op = argv[2];
if (bcb_field_get(argv[1], &field, &size))
return CMD_RET_FAILURE;
@@ -325,6 +278,12 @@ static int do_bcb_dump(struct cmd_tbl *cmdtp, int flag, int argc,
int size;
char *field;
+ if (argc < 2)
+ return CMD_RET_USAGE;
+
+ if (!block)
+ return bcb_not_loaded();
+
if (bcb_field_get(argv[1], &field, &size))
return CMD_RET_FAILURE;
@@ -356,6 +315,9 @@ err:
static int do_bcb_store(struct cmd_tbl *cmdtp, int flag, int argc,
char * const argv[])
{
+ if (!block)
+ return bcb_not_loaded();
+
return __bcb_store();
}
@@ -414,44 +376,7 @@ void bcb_reset(void)
__bcb_reset();
}
-static struct cmd_tbl cmd_bcb_sub[] = {
- U_BOOT_CMD_MKENT(load, CONFIG_SYS_MAXARGS, 1, do_bcb_load, "", ""),
- U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 1, do_bcb_set, "", ""),
- U_BOOT_CMD_MKENT(clear, CONFIG_SYS_MAXARGS, 1, do_bcb_clear, "", ""),
- U_BOOT_CMD_MKENT(test, CONFIG_SYS_MAXARGS, 1, do_bcb_test, "", ""),
- U_BOOT_CMD_MKENT(dump, CONFIG_SYS_MAXARGS, 1, do_bcb_dump, "", ""),
- U_BOOT_CMD_MKENT(store, CONFIG_SYS_MAXARGS, 1, do_bcb_store, "", ""),
-};
-
-static int do_bcb(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
-{
- struct cmd_tbl *c;
-
- if (argc < 2)
- return CMD_RET_USAGE;
-
- argc--;
- argv++;
-
- c = find_cmd_tbl(argv[0], cmd_bcb_sub, ARRAY_SIZE(cmd_bcb_sub));
- if (!c)
- return CMD_RET_USAGE;
-
- if (bcb_is_misused(argc, argv)) {
- /*
- * We try to improve the user experience by reporting the
- * root-cause of misusage, so don't return CMD_RET_USAGE,
- * since the latter prints out the full-blown help text
- */
- return CMD_RET_FAILURE;
- }
-
- return c->cmd(cmdtp, flag, argc, argv);
-}
-
-U_BOOT_CMD(
- bcb, CONFIG_SYS_MAXARGS, 1, do_bcb,
- "Load/set/clear/test/dump/store Android BCB fields",
+U_BOOT_LONGHELP(bcb,
"load <interface> <dev> <part> - load BCB from <interface> <dev>:<part>\n"
"load <dev> <part> - load BCB from mmc <dev>:<part>\n"
"bcb set <field> <val> - set BCB <field> to <val>\n"
@@ -472,3 +397,13 @@ U_BOOT_CMD(
" NOTE: any ':' character in <val> will be replaced by line feed\n"
" during 'bcb set' and used as separator by upper layers\n"
);
+
+U_BOOT_CMD_WITH_SUBCMDS(bcb,
+ "Load/set/clear/test/dump/store Android BCB fields", bcb_help_text,
+ U_BOOT_SUBCMD_MKENT(load, 4, 1, do_bcb_load),
+ U_BOOT_SUBCMD_MKENT(set, 3, 1, do_bcb_set),
+ U_BOOT_SUBCMD_MKENT(clear, 2, 1, do_bcb_clear),
+ U_BOOT_SUBCMD_MKENT(test, 4, 1, do_bcb_test),
+ U_BOOT_SUBCMD_MKENT(dump, 2, 1, do_bcb_dump),
+ U_BOOT_SUBCMD_MKENT(store, 1, 1, do_bcb_store),
+);
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 2/6] cmd: bcb: rework the command to U_BOOT_LONGHELP approach
2024-10-17 14:12 ` [PATCH v5 2/6] cmd: bcb: rework the command to U_BOOT_LONGHELP approach Dmitry Rokosov
@ 2024-10-22 8:44 ` Mattijs Korpershoek
0 siblings, 0 replies; 21+ messages in thread
From: Mattijs Korpershoek @ 2024-10-22 8:44 UTC (permalink / raw)
To: Dmitry Rokosov, Igor Opaniuk, Sam Protsenko, Tom Rini,
Andrew F. Davis, Neil Armstrong, Simon Glass, Mario Six
Cc: u-boot, u-boot-amlogic, rockosov, kernel, Dmitry Rokosov
Hi Dmitry,
Thank you for the patch.
On jeu., oct. 17, 2024 at 17:12, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> U_BOOT_LONGHELP and U_BOOT_CMD_WITH_SUBCMDS offer numerous advantages,
> including:
> - common argument restrictions checking
> - automatic subcommand matching
> - improved usage and help handling
>
> By utilizing the U_BOOT_LONGHELP approach, we can reduce the amount of
> command management code and describe commands more succinctly.
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
> cmd/bcb.c | 151 ++++++++++++++++++--------------------------------------------
> 1 file changed, 43 insertions(+), 108 deletions(-)
Nice diffstat, always great to see code removed!
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>
> diff --git a/cmd/bcb.c b/cmd/bcb.c
> index 97a96c009641cc094645607ef833575f3c03fe4b..98b2a71087a27b1721f4bed4160095d65ec75402 100644
> --- a/cmd/bcb.c
> +++ b/cmd/bcb.c
> @@ -16,15 +16,6 @@
> #include <vsprintf.h>
> #include <linux/err.h>
>
> -enum bcb_cmd {
> - BCB_CMD_LOAD,
> - BCB_CMD_FIELD_SET,
> - BCB_CMD_FIELD_CLEAR,
> - BCB_CMD_FIELD_TEST,
> - BCB_CMD_FIELD_DUMP,
> - BCB_CMD_STORE,
> -};
> -
> static const char * const fields[] = {
> "command",
> "status",
> @@ -38,67 +29,9 @@ static struct disk_partition partition_data;
> static struct blk_desc *block;
> static struct disk_partition *partition = &partition_data;
>
> -static int bcb_cmd_get(char *cmd)
> -{
> - if (!strcmp(cmd, "load"))
> - return BCB_CMD_LOAD;
> - if (!strcmp(cmd, "set"))
> - return BCB_CMD_FIELD_SET;
> - if (!strcmp(cmd, "clear"))
> - return BCB_CMD_FIELD_CLEAR;
> - if (!strcmp(cmd, "test"))
> - return BCB_CMD_FIELD_TEST;
> - if (!strcmp(cmd, "store"))
> - return BCB_CMD_STORE;
> - if (!strcmp(cmd, "dump"))
> - return BCB_CMD_FIELD_DUMP;
> - else
> - return -1;
> -}
> -
> -static int bcb_is_misused(int argc, char *const argv[])
> +static int bcb_not_loaded(void)
> {
> - int cmd = bcb_cmd_get(argv[0]);
> -
> - switch (cmd) {
> - case BCB_CMD_LOAD:
> - if (argc != 3 && argc != 4)
> - goto err;
> - break;
> - case BCB_CMD_FIELD_SET:
> - if (argc != 3)
> - goto err;
> - break;
> - case BCB_CMD_FIELD_TEST:
> - if (argc != 4)
> - goto err;
> - break;
> - case BCB_CMD_FIELD_CLEAR:
> - if (argc != 1 && argc != 2)
> - goto err;
> - break;
> - case BCB_CMD_STORE:
> - if (argc != 1)
> - goto err;
> - break;
> - case BCB_CMD_FIELD_DUMP:
> - if (argc != 2)
> - goto err;
> - break;
> - default:
> - printf("Error: 'bcb %s' not supported\n", argv[0]);
> - return -1;
> - }
> -
> - if (cmd != BCB_CMD_LOAD && !block) {
> - printf("Error: Please, load BCB first!\n");
> - return -1;
> - }
> -
> - return 0;
> -err:
> - printf("Error: Bad usage of 'bcb %s'\n", argv[0]);
> -
> + printf("Error: Please, load BCB first!\n");
> return -1;
> }
>
> @@ -216,6 +149,9 @@ static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc,
> char *endp;
> char *iface = "mmc";
>
> + if (argc < 3)
> + return CMD_RET_USAGE;
> +
> if (argc == 4) {
> iface = argv[1];
> argc--;
> @@ -270,6 +206,12 @@ static int __bcb_set(const char *fieldp, const char *valp)
> static int do_bcb_set(struct cmd_tbl *cmdtp, int flag, int argc,
> char * const argv[])
> {
> + if (argc < 3)
> + return CMD_RET_USAGE;
> +
> + if (!block)
> + return bcb_not_loaded();
> +
> return __bcb_set(argv[1], argv[2]);
> }
>
> @@ -279,6 +221,9 @@ static int do_bcb_clear(struct cmd_tbl *cmdtp, int flag, int argc,
> int size;
> char *field;
>
> + if (!block)
> + return bcb_not_loaded();
> +
> if (argc == 1) {
> memset(&bcb, 0, sizeof(bcb));
> return CMD_RET_SUCCESS;
> @@ -297,7 +242,15 @@ static int do_bcb_test(struct cmd_tbl *cmdtp, int flag, int argc,
> {
> int size;
> char *field;
> - char *op = argv[2];
> + char *op;
> +
> + if (argc < 4)
> + return CMD_RET_USAGE;
> +
> + if (!block)
> + return bcb_not_loaded();
> +
> + op = argv[2];
>
> if (bcb_field_get(argv[1], &field, &size))
> return CMD_RET_FAILURE;
> @@ -325,6 +278,12 @@ static int do_bcb_dump(struct cmd_tbl *cmdtp, int flag, int argc,
> int size;
> char *field;
>
> + if (argc < 2)
> + return CMD_RET_USAGE;
> +
> + if (!block)
> + return bcb_not_loaded();
> +
> if (bcb_field_get(argv[1], &field, &size))
> return CMD_RET_FAILURE;
>
> @@ -356,6 +315,9 @@ err:
> static int do_bcb_store(struct cmd_tbl *cmdtp, int flag, int argc,
> char * const argv[])
> {
> + if (!block)
> + return bcb_not_loaded();
> +
> return __bcb_store();
> }
>
> @@ -414,44 +376,7 @@ void bcb_reset(void)
> __bcb_reset();
> }
>
> -static struct cmd_tbl cmd_bcb_sub[] = {
> - U_BOOT_CMD_MKENT(load, CONFIG_SYS_MAXARGS, 1, do_bcb_load, "", ""),
> - U_BOOT_CMD_MKENT(set, CONFIG_SYS_MAXARGS, 1, do_bcb_set, "", ""),
> - U_BOOT_CMD_MKENT(clear, CONFIG_SYS_MAXARGS, 1, do_bcb_clear, "", ""),
> - U_BOOT_CMD_MKENT(test, CONFIG_SYS_MAXARGS, 1, do_bcb_test, "", ""),
> - U_BOOT_CMD_MKENT(dump, CONFIG_SYS_MAXARGS, 1, do_bcb_dump, "", ""),
> - U_BOOT_CMD_MKENT(store, CONFIG_SYS_MAXARGS, 1, do_bcb_store, "", ""),
> -};
> -
> -static int do_bcb(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> -{
> - struct cmd_tbl *c;
> -
> - if (argc < 2)
> - return CMD_RET_USAGE;
> -
> - argc--;
> - argv++;
> -
> - c = find_cmd_tbl(argv[0], cmd_bcb_sub, ARRAY_SIZE(cmd_bcb_sub));
> - if (!c)
> - return CMD_RET_USAGE;
> -
> - if (bcb_is_misused(argc, argv)) {
> - /*
> - * We try to improve the user experience by reporting the
> - * root-cause of misusage, so don't return CMD_RET_USAGE,
> - * since the latter prints out the full-blown help text
> - */
> - return CMD_RET_FAILURE;
> - }
> -
> - return c->cmd(cmdtp, flag, argc, argv);
> -}
> -
> -U_BOOT_CMD(
> - bcb, CONFIG_SYS_MAXARGS, 1, do_bcb,
> - "Load/set/clear/test/dump/store Android BCB fields",
> +U_BOOT_LONGHELP(bcb,
> "load <interface> <dev> <part> - load BCB from <interface> <dev>:<part>\n"
> "load <dev> <part> - load BCB from mmc <dev>:<part>\n"
> "bcb set <field> <val> - set BCB <field> to <val>\n"
> @@ -472,3 +397,13 @@ U_BOOT_CMD(
> " NOTE: any ':' character in <val> will be replaced by line feed\n"
> " during 'bcb set' and used as separator by upper layers\n"
> );
> +
> +U_BOOT_CMD_WITH_SUBCMDS(bcb,
> + "Load/set/clear/test/dump/store Android BCB fields", bcb_help_text,
> + U_BOOT_SUBCMD_MKENT(load, 4, 1, do_bcb_load),
> + U_BOOT_SUBCMD_MKENT(set, 3, 1, do_bcb_set),
> + U_BOOT_SUBCMD_MKENT(clear, 2, 1, do_bcb_clear),
> + U_BOOT_SUBCMD_MKENT(test, 4, 1, do_bcb_test),
> + U_BOOT_SUBCMD_MKENT(dump, 2, 1, do_bcb_dump),
> + U_BOOT_SUBCMD_MKENT(store, 1, 1, do_bcb_store),
> +);
>
> --
> 2.43.0
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 3/6] treewide: bcb: move ab_select command to bcb subcommands
2024-10-17 14:12 [PATCH v5 0/6] android_ab: introduce bcb ab_dump command and provide several bcb fixes Dmitry Rokosov
2024-10-17 14:12 ` [PATCH v5 1/6] include/android_ab: move ab_select_slot() documentation to @ notation Dmitry Rokosov
2024-10-17 14:12 ` [PATCH v5 2/6] cmd: bcb: rework the command to U_BOOT_LONGHELP approach Dmitry Rokosov
@ 2024-10-17 14:12 ` Dmitry Rokosov
2024-10-22 9:01 ` Mattijs Korpershoek
2024-10-17 14:12 ` [PATCH v5 4/6] cmd: bcb: change strcmp() usage style in the do_bcb_ab_select() Dmitry Rokosov
` (5 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Dmitry Rokosov @ 2024-10-17 14:12 UTC (permalink / raw)
To: Igor Opaniuk, Mattijs Korpershoek, Sam Protsenko, Tom Rini,
Andrew F. Davis, Neil Armstrong, Simon Glass, Mario Six
Cc: u-boot, u-boot-amlogic, rockosov, kernel, Dmitry Rokosov
To enhance code organization, it is beneficial to consolidate all A/B
BCB management routines into a single super-command.
The 'bcb' command is an excellent candidate for this purpose.
This patch integrates the separate 'ab_select' command into the 'bcb'
group as the 'ab_select' subcommand, maintaining the same parameter list
for consistency.
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
MAINTAINERS | 1 -
cmd/Kconfig | 14 -------
cmd/Makefile | 1 -
cmd/ab_select.c | 66 -------------------------------
cmd/bcb.c | 63 +++++++++++++++++++++++++++++
configs/am57xx_evm_defconfig | 1 -
configs/am57xx_hs_evm_defconfig | 1 -
configs/am57xx_hs_evm_usb_defconfig | 1 -
configs/khadas-vim3_android_ab_defconfig | 1 -
configs/khadas-vim3l_android_ab_defconfig | 1 -
configs/sandbox64_defconfig | 2 +
configs/sandbox_defconfig | 1 -
doc/android/ab.rst | 12 +++---
include/configs/khadas-vim3_android.h | 2 +-
include/configs/khadas-vim3l_android.h | 2 +-
include/configs/meson64_android.h | 4 +-
include/configs/ti_omap5_common.h | 4 +-
test/py/tests/test_android/test_ab.py | 8 ++--
18 files changed, 81 insertions(+), 104 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 7c6c368285e729dee11bb116cd7dd30d369b1a87..e0c6a6b4b43774258ce1e20100e4a2f01cbeaab0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -65,7 +65,6 @@ R: Sam Protsenko <semen.protsenko@linaro.org>
S: Maintained
T: git https://source.denx.de/u-boot/custodians/u-boot-dfu.git
F: boot/android_ab.c
-F: cmd/ab_select.c
F: doc/android/ab.rst
F: include/android_ab.h
F: test/py/tests/test_android/test_ab.py
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 8c677b1e4864164d985914ea6ac4a65c6caee099..82b5852d47c1057ca9d5f13d078e428bb4a3b539 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1775,20 +1775,6 @@ config CMD_XXD
endmenu
-menu "Android support commands"
-
-config CMD_AB_SELECT
- bool "ab_select"
- depends on ANDROID_AB
- help
- On Android devices with more than one boot slot (multiple copies of
- the kernel and system images) this provides a command to select which
- slot should be used to boot from and register the boot attempt. This
- is used by the new A/B update model where one slot is updated in the
- background while running from the other slot.
-
-endmenu
-
if NET || NET_LWIP
menuconfig CMD_NET
diff --git a/cmd/Makefile b/cmd/Makefile
index f30914053721d7e8ac17c828aef4d379cf3ecdb5..d1bb9d0e57d80855dd2da8af0cb510dbad65933e 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -17,7 +17,6 @@ obj-$(CONFIG_CMD_2048) += 2048.o
obj-$(CONFIG_CMD_ACPI) += acpi.o
obj-$(CONFIG_CMD_ADDRMAP) += addrmap.o
obj-$(CONFIG_CMD_AES) += aes.o
-obj-$(CONFIG_CMD_AB_SELECT) += ab_select.o
obj-$(CONFIG_CMD_ADC) += adc.o
obj-$(CONFIG_CMD_ARMFLASH) += armflash.o
obj-$(CONFIG_BLK) += blk_common.o
diff --git a/cmd/ab_select.c b/cmd/ab_select.c
deleted file mode 100644
index 7c178c728ca4c8b5bcba02a04eef2d6a7c86afb6..0000000000000000000000000000000000000000
--- a/cmd/ab_select.c
+++ /dev/null
@@ -1,66 +0,0 @@
-// SPDX-License-Identifier: BSD-2-Clause
-/*
- * Copyright (C) 2017 The Android Open Source Project
- */
-
-#include <android_ab.h>
-#include <command.h>
-#include <env.h>
-#include <part.h>
-
-static int do_ab_select(struct cmd_tbl *cmdtp, int flag, int argc,
- char *const argv[])
-{
- int ret;
- struct blk_desc *dev_desc;
- struct disk_partition part_info;
- char slot[2];
- bool dec_tries = true;
-
- if (argc < 4)
- return CMD_RET_USAGE;
-
- for (int i = 4; i < argc; i++) {
- if (strcmp(argv[i], "--no-dec") == 0) {
- dec_tries = false;
- } else {
- return CMD_RET_USAGE;
- }
- }
-
- /* Lookup the "misc" partition from argv[2] and argv[3] */
- if (part_get_info_by_dev_and_name_or_num(argv[2], argv[3],
- &dev_desc, &part_info,
- false) < 0) {
- return CMD_RET_FAILURE;
- }
-
- ret = ab_select_slot(dev_desc, &part_info, dec_tries);
- if (ret < 0) {
- printf("Android boot failed, error %d.\n", ret);
- return CMD_RET_FAILURE;
- }
-
- /* Android standard slot names are 'a', 'b', ... */
- slot[0] = BOOT_SLOT_NAME(ret);
- slot[1] = '\0';
- env_set(argv[1], slot);
- printf("ANDROID: Booting slot: %s\n", slot);
- return CMD_RET_SUCCESS;
-}
-
-U_BOOT_CMD(ab_select, 5, 0, do_ab_select,
- "Select the slot used to boot from and register the boot attempt.",
- "<slot_var_name> <interface> <dev[:part|#part_name]> [--no-dec]\n"
- " - Load the slot metadata from the partition 'part' on\n"
- " device type 'interface' instance 'dev' and store the active\n"
- " slot in the 'slot_var_name' variable. This also updates the\n"
- " Android slot metadata with a boot attempt, which can cause\n"
- " successive calls to this function to return a different result\n"
- " if the returned slot runs out of boot attempts.\n"
- " - If 'part_name' is passed, preceded with a # instead of :, the\n"
- " partition name whose label is 'part_name' will be looked up in\n"
- " the partition table. This is commonly the \"misc\" partition.\n"
- " - If '--no-dec' is set, the number of tries remaining will not\n"
- " decremented for the selected boot slot\n"
-);
diff --git a/cmd/bcb.c b/cmd/bcb.c
index 98b2a71087a27b1721f4bed4160095d65ec75402..2854408e5669b0d7d2a06073fa81158b21834b99 100644
--- a/cmd/bcb.c
+++ b/cmd/bcb.c
@@ -8,6 +8,7 @@
#include <android_bootloader_message.h>
#include <bcb.h>
#include <command.h>
+#include <android_ab.h>
#include <display_options.h>
#include <log.h>
#include <part.h>
@@ -376,6 +377,48 @@ void bcb_reset(void)
__bcb_reset();
}
+__maybe_unused static int do_bcb_ab_select(struct cmd_tbl *cmdtp,
+ int flag, int argc,
+ char * const argv[])
+{
+ int ret;
+ struct blk_desc *dev_desc;
+ struct disk_partition part_info;
+ char slot[2];
+ bool dec_tries = true;
+
+ if (argc < 4)
+ return CMD_RET_USAGE;
+
+ for (int i = 4; i < argc; i++) {
+ if (strcmp(argv[i], "--no-dec") == 0)
+ dec_tries = false;
+ else
+ return CMD_RET_USAGE;
+ }
+
+ /* Lookup the "misc" partition from argv[2] and argv[3] */
+ if (part_get_info_by_dev_and_name_or_num(argv[2], argv[3],
+ &dev_desc, &part_info,
+ false) < 0) {
+ return CMD_RET_FAILURE;
+ }
+
+ ret = ab_select_slot(dev_desc, &part_info, dec_tries);
+ if (ret < 0) {
+ printf("Android boot failed, error %d.\n", ret);
+ return CMD_RET_FAILURE;
+ }
+
+ /* Android standard slot names are 'a', 'b', ... */
+ slot[0] = BOOT_SLOT_NAME(ret);
+ slot[1] = '\0';
+ env_set(argv[1], slot);
+ printf("ANDROID: Booting slot: %s\n", slot);
+
+ return CMD_RET_SUCCESS;
+}
+
U_BOOT_LONGHELP(bcb,
"load <interface> <dev> <part> - load BCB from <interface> <dev>:<part>\n"
"load <dev> <part> - load BCB from mmc <dev>:<part>\n"
@@ -385,6 +428,23 @@ U_BOOT_LONGHELP(bcb,
"bcb dump <field> - dump BCB <field>\n"
"bcb store - store BCB back to <interface>\n"
"\n"
+#if IS_ENABLED(CONFIG_ANDROID_AB)
+ "bcb ab_select -\n"
+ " Select the slot used to boot from and register the boot attempt.\n"
+ " <slot_var_name> <interface> <dev[:part|#part_name]> [--no-dec]\n"
+ " - Load the slot metadata from the partition 'part' on\n"
+ " device type 'interface' instance 'dev' and store the active\n"
+ " slot in the 'slot_var_name' variable. This also updates the\n"
+ " Android slot metadata with a boot attempt, which can cause\n"
+ " successive calls to this function to return a different result\n"
+ " if the returned slot runs out of boot attempts.\n"
+ " - If 'part_name' is passed, preceded with a # instead of :, the\n"
+ " partition name whose label is 'part_name' will be looked up in\n"
+ " the partition table. This is commonly the \"misc\" partition.\n"
+ " - If '--no-dec' is set, the number of tries remaining will not\n"
+ " decremented for the selected boot slot\n"
+ "\n"
+#endif
"Legend:\n"
"<interface> - storage device interface (virtio, mmc, etc)\n"
"<dev> - storage device index containing the BCB partition\n"
@@ -406,4 +466,7 @@ U_BOOT_CMD_WITH_SUBCMDS(bcb,
U_BOOT_SUBCMD_MKENT(test, 4, 1, do_bcb_test),
U_BOOT_SUBCMD_MKENT(dump, 2, 1, do_bcb_dump),
U_BOOT_SUBCMD_MKENT(store, 1, 1, do_bcb_store),
+#if IS_ENABLED(CONFIG_ANDROID_AB)
+ U_BOOT_SUBCMD_MKENT(ab_select, 5, 1, do_bcb_ab_select),
+#endif
);
diff --git a/configs/am57xx_evm_defconfig b/configs/am57xx_evm_defconfig
index efc154eda043ff3ea08194288e33792ce48282f9..b793f00babe474ea3f15292fb4015a7120401238 100644
--- a/configs/am57xx_evm_defconfig
+++ b/configs/am57xx_evm_defconfig
@@ -48,7 +48,6 @@ CONFIG_CMD_SPL=y
CONFIG_SYS_I2C_EEPROM_ADDR_LEN=2
CONFIG_CMD_BCB=y
# CONFIG_CMD_SETEXPR is not set
-CONFIG_CMD_AB_SELECT=y
CONFIG_BOOTP_DNS2=y
# CONFIG_CMD_PMIC is not set
CONFIG_CMD_AVB=y
diff --git a/configs/am57xx_hs_evm_defconfig b/configs/am57xx_hs_evm_defconfig
index 0f8533e15dbd7c0186a513b27b46a0407b6f79f1..5cacd7f9cc53d338d52120186b16684add93fd21 100644
--- a/configs/am57xx_hs_evm_defconfig
+++ b/configs/am57xx_hs_evm_defconfig
@@ -44,7 +44,6 @@ CONFIG_CMD_ADTIMG=y
CONFIG_CMD_ABOOTIMG=y
CONFIG_SYS_I2C_EEPROM_ADDR_LEN=2
CONFIG_CMD_BCB=y
-CONFIG_CMD_AB_SELECT=y
CONFIG_BOOTP_DNS2=y
# CONFIG_CMD_PMIC is not set
CONFIG_CMD_AVB=y
diff --git a/configs/am57xx_hs_evm_usb_defconfig b/configs/am57xx_hs_evm_usb_defconfig
index 81a938339d5934605cb7defa04ea92f76468b21a..2d8068ecdc79c01c1281ab3873fc892aa4c96be7 100644
--- a/configs/am57xx_hs_evm_usb_defconfig
+++ b/configs/am57xx_hs_evm_usb_defconfig
@@ -46,7 +46,6 @@ CONFIG_CMD_ADTIMG=y
CONFIG_CMD_ABOOTIMG=y
CONFIG_SYS_I2C_EEPROM_ADDR_LEN=2
CONFIG_CMD_BCB=y
-CONFIG_CMD_AB_SELECT=y
CONFIG_BOOTP_DNS2=y
# CONFIG_CMD_PMIC is not set
CONFIG_CMD_AVB=y
diff --git a/configs/khadas-vim3_android_ab_defconfig b/configs/khadas-vim3_android_ab_defconfig
index 510fe4f8928fe39a040a615636fa550b3e0dc5db..de5357c45cbfe4742d9491a29386850570acc235 100644
--- a/configs/khadas-vim3_android_ab_defconfig
+++ b/configs/khadas-vim3_android_ab_defconfig
@@ -47,7 +47,6 @@ CONFIG_CMD_SPI=y
CONFIG_CMD_USB=y
CONFIG_CMD_USB_MASS_STORAGE=y
# CONFIG_CMD_SETEXPR is not set
-CONFIG_CMD_AB_SELECT=y
CONFIG_CMD_REGULATOR=y
CONFIG_CMD_AVB=y
CONFIG_OF_CONTROL=y
diff --git a/configs/khadas-vim3l_android_ab_defconfig b/configs/khadas-vim3l_android_ab_defconfig
index d2da8ff2a69b209b8fb22a48be537bd4dd17a3bb..4d7b90f23002e464d7dc40516bcd3161b0f59439 100644
--- a/configs/khadas-vim3l_android_ab_defconfig
+++ b/configs/khadas-vim3l_android_ab_defconfig
@@ -47,7 +47,6 @@ CONFIG_CMD_SPI=y
CONFIG_CMD_USB=y
CONFIG_CMD_USB_MASS_STORAGE=y
# CONFIG_CMD_SETEXPR is not set
-CONFIG_CMD_AB_SELECT=y
CONFIG_CMD_REGULATOR=y
CONFIG_CMD_AVB=y
CONFIG_OF_CONTROL=y
diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index 1b3b8c6e788cd6845b61e62a06b730da28831edc..b5f80b8572ad32b2a92d4fe82c9068c453c11dfd 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -27,6 +27,7 @@ CONFIG_CONSOLE_RECORD=y
CONFIG_CONSOLE_RECORD_OUT_SIZE=0x6000
CONFIG_PRE_CONSOLE_BUFFER=y
CONFIG_DISPLAY_BOARDINFO_LATE=y
+CONFIG_ANDROID_AB=y
CONFIG_CMD_CPU=y
CONFIG_CMD_LICENSE=y
CONFIG_CMD_BOOTZ=y
@@ -46,6 +47,7 @@ CONFIG_CMD_MD5SUM=y
CONFIG_CMD_MEMINFO=y
CONFIG_CMD_MX_CYCLIC=y
CONFIG_CMD_MEMTEST=y
+CONFIG_CMD_BCB=y
CONFIG_CMD_CLK=y
CONFIG_CMD_DEMO=y
CONFIG_CMD_GPIO=y
diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index f596f1cc55a45ade4862122a6944d109c6c29238..d111858082d5a8ed3725c95462b2c81a406b57d2 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -103,7 +103,6 @@ CONFIG_CMD_AXI=y
CONFIG_CMD_CAT=y
CONFIG_CMD_SETEXPR_FMT=y
CONFIG_CMD_XXD=y
-CONFIG_CMD_AB_SELECT=y
CONFIG_CMD_DHCP6=y
CONFIG_BOOTP_DNS2=y
CONFIG_CMD_PCAP=y
diff --git a/doc/android/ab.rst b/doc/android/ab.rst
index 2adf88781d60b61d1b3c74efe976a684b590c813..7fd4aeb6a724b839de9be5e9a8843ade2ad3667e 100644
--- a/doc/android/ab.rst
+++ b/doc/android/ab.rst
@@ -18,7 +18,7 @@ The A/B updates support can be activated by specifying next options in
your board configuration file::
CONFIG_ANDROID_AB=y
- CONFIG_CMD_AB_SELECT=y
+ CONFIG_CMD_BCB=y
The disk space on target device must be partitioned in a way so that each
partition which needs to be updated has two or more instances. The name of
@@ -26,8 +26,8 @@ each instance must be formed by adding suffixes: ``_a``, ``_b``, ``_c``, etc.
For example: ``boot_a``, ``boot_b``, ``system_a``, ``system_b``, ``vendor_a``,
``vendor_b``.
-As a result you can use ``ab_select`` command to ensure A/B boot process in your
-boot script. This command analyzes and processes A/B metadata stored on a
+As a result you can use ``bcb ab_select`` command to ensure A/B boot process in
+your boot script. This command analyzes and processes A/B metadata stored on a
special partition (e.g. ``misc``) and determines which slot should be used for
booting up.
@@ -42,15 +42,15 @@ Command usage
.. code-block:: none
- ab_select <slot_var_name> <interface> <dev[:part_number|#part_name]>
+ bcb ab_select <slot_var_name> <interface> <dev[:part_number|#part_name]>
for example::
- => ab_select slot_name mmc 1:4
+ => bcb ab_select slot_name mmc 1:4
or::
- => ab_select slot_name mmc 1#misc
+ => bcb ab_select slot_name mmc 1#misc
Result::
diff --git a/include/configs/khadas-vim3_android.h b/include/configs/khadas-vim3_android.h
index b76e049f09cc014837981e914a3792c829f42cdc..fc89efb4c36e5216a1344bb2d758300eec3c8f44 100644
--- a/include/configs/khadas-vim3_android.h
+++ b/include/configs/khadas-vim3_android.h
@@ -12,7 +12,7 @@
#define LOGO_UUID "43a3305d-150f-4cc9-bd3b-38fca8693846;"
#define ROOT_UUID "ddb8c3f6-d94d-4394-b633-3134139cc2e0;"
-#if defined(CONFIG_CMD_AB_SELECT)
+#if defined(CONFIG_CMD_BCB) && defined(CONFIG_ANDROID_AB)
#define PARTS_DEFAULT \
"uuid_disk=${uuid_gpt_disk};" \
"name=logo,start=512K,size=2M,uuid=" LOGO_UUID \
diff --git a/include/configs/khadas-vim3l_android.h b/include/configs/khadas-vim3l_android.h
index 0ab8ffd372a4ae44804fca41c11ee930aaa72460..5b2aed1cf62af213f0ea6688fde6d07267aa4f55 100644
--- a/include/configs/khadas-vim3l_android.h
+++ b/include/configs/khadas-vim3l_android.h
@@ -12,7 +12,7 @@
#define LOGO_UUID "43a3305d-150f-4cc9-bd3b-38fca8693846;"
#define ROOT_UUID "ddb8c3f6-d94d-4394-b633-3134139cc2e0;"
-#if defined(CONFIG_CMD_AB_SELECT)
+#if defined(CONFIG_CMD_BCB) && defined(CONFIG_ANDROID_AB)
#define PARTS_DEFAULT \
"uuid_disk=${uuid_gpt_disk};" \
"name=logo,start=512K,size=2M,uuid=" LOGO_UUID \
diff --git a/include/configs/meson64_android.h b/include/configs/meson64_android.h
index fa520265800ccf081862bc97643b6a8e28b13327..77364bbf9cf0d1b68d413773902ea9d18b720928 100644
--- a/include/configs/meson64_android.h
+++ b/include/configs/meson64_android.h
@@ -47,13 +47,13 @@
#define AVB_VERIFY_CMD ""
#endif
-#if defined(CONFIG_CMD_AB_SELECT)
+#if defined(CONFIG_CMD_BCB) && defined(CONFIG_ANDROID_AB)
#define ANDROIDBOOT_GET_CURRENT_SLOT_CMD "get_current_slot=" \
"if part number mmc ${mmcdev} " CONTROL_PARTITION " control_part_number; " \
"then " \
"echo " CONTROL_PARTITION \
" partition number:${control_part_number};" \
- "ab_select current_slot mmc ${mmcdev}:${control_part_number};" \
+ "bcb ab_select current_slot mmc ${mmcdev}:${control_part_number};" \
"else " \
"echo " CONTROL_PARTITION " partition not found;" \
"fi;\0"
diff --git a/include/configs/ti_omap5_common.h b/include/configs/ti_omap5_common.h
index 26494ae980108ad84eb173a0deaa7b701a5309d4..26b6c1cd188c05371c6455cd6247f07a1773d885 100644
--- a/include/configs/ti_omap5_common.h
+++ b/include/configs/ti_omap5_common.h
@@ -93,13 +93,13 @@
#define CONTROL_PARTITION "misc"
-#if defined(CONFIG_CMD_AB_SELECT)
+#if defined(CONFIG_CMD_BCB) && defined(CONFIG_ANDROID_AB)
#define AB_SELECT_SLOT \
"if part number mmc 1 " CONTROL_PARTITION " control_part_number; " \
"then " \
"echo " CONTROL_PARTITION \
" partition number:${control_part_number};" \
- "ab_select slot_name mmc ${mmcdev}:${control_part_number};" \
+ "bcb ab_select slot_name mmc ${mmcdev}:${control_part_number};" \
"else " \
"echo " CONTROL_PARTITION " partition not found;" \
"exit;" \
diff --git a/test/py/tests/test_android/test_ab.py b/test/py/tests/test_android/test_ab.py
index c79cb07fda35dfeb608ac7345cd3f22744e2e491..0d7b7995a9fab6e3daad748721818b9e4cfac452 100644
--- a/test/py/tests/test_android/test_ab.py
+++ b/test/py/tests/test_android/test_ab.py
@@ -56,20 +56,20 @@ def ab_disk_image(u_boot_console):
@pytest.mark.boardspec('sandbox')
@pytest.mark.buildconfigspec('android_ab')
-@pytest.mark.buildconfigspec('cmd_ab_select')
+@pytest.mark.buildconfigspec('cmd_bcb')
@pytest.mark.requiredtool('sgdisk')
def test_ab(ab_disk_image, u_boot_console):
- """Test the 'ab_select' command."""
+ """Test the 'bcb ab_select' command."""
u_boot_console.run_command('host bind 0 ' + ab_disk_image.path)
- output = u_boot_console.run_command('ab_select slot_name host 0#misc')
+ output = u_boot_console.run_command('bcb ab_select slot_name host 0#misc')
assert 're-initializing A/B metadata' in output
assert 'Attempting slot a, tries remaining 7' in output
output = u_boot_console.run_command('printenv slot_name')
assert 'slot_name=a' in output
- output = u_boot_console.run_command('ab_select slot_name host 0:1')
+ output = u_boot_console.run_command('bcb ab_select slot_name host 0:1')
assert 'Attempting slot b, tries remaining 7' in output
output = u_boot_console.run_command('printenv slot_name')
assert 'slot_name=b' in output
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 3/6] treewide: bcb: move ab_select command to bcb subcommands
2024-10-17 14:12 ` [PATCH v5 3/6] treewide: bcb: move ab_select command to bcb subcommands Dmitry Rokosov
@ 2024-10-22 9:01 ` Mattijs Korpershoek
0 siblings, 0 replies; 21+ messages in thread
From: Mattijs Korpershoek @ 2024-10-22 9:01 UTC (permalink / raw)
To: Dmitry Rokosov, Igor Opaniuk, Sam Protsenko, Tom Rini,
Andrew F. Davis, Neil Armstrong, Simon Glass, Mario Six
Cc: u-boot, u-boot-amlogic, rockosov, kernel, Dmitry Rokosov
Hi Dmitry,
Thank you for the patch.
On jeu., oct. 17, 2024 at 17:12, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> To enhance code organization, it is beneficial to consolidate all A/B
> BCB management routines into a single super-command.
> The 'bcb' command is an excellent candidate for this purpose.
>
> This patch integrates the separate 'ab_select' command into the 'bcb'
> group as the 'ab_select' subcommand, maintaining the same parameter list
> for consistency.
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
> MAINTAINERS | 1 -
> cmd/Kconfig | 14 -------
> cmd/Makefile | 1 -
> cmd/ab_select.c | 66 -------------------------------
> cmd/bcb.c | 63 +++++++++++++++++++++++++++++
> configs/am57xx_evm_defconfig | 1 -
> configs/am57xx_hs_evm_defconfig | 1 -
> configs/am57xx_hs_evm_usb_defconfig | 1 -
> configs/khadas-vim3_android_ab_defconfig | 1 -
> configs/khadas-vim3l_android_ab_defconfig | 1 -
> configs/sandbox64_defconfig | 2 +
> configs/sandbox_defconfig | 1 -
> doc/android/ab.rst | 12 +++---
> include/configs/khadas-vim3_android.h | 2 +-
> include/configs/khadas-vim3l_android.h | 2 +-
> include/configs/meson64_android.h | 4 +-
> include/configs/ti_omap5_common.h | 4 +-
> test/py/tests/test_android/test_ab.py | 8 ++--
> 18 files changed, 81 insertions(+), 104 deletions(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7c6c368285e729dee11bb116cd7dd30d369b1a87..e0c6a6b4b43774258ce1e20100e4a2f01cbeaab0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -65,7 +65,6 @@ R: Sam Protsenko <semen.protsenko@linaro.org>
> S: Maintained
> T: git https://source.denx.de/u-boot/custodians/u-boot-dfu.git
> F: boot/android_ab.c
> -F: cmd/ab_select.c
> F: doc/android/ab.rst
> F: include/android_ab.h
> F: test/py/tests/test_android/test_ab.py
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 8c677b1e4864164d985914ea6ac4a65c6caee099..82b5852d47c1057ca9d5f13d078e428bb4a3b539 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1775,20 +1775,6 @@ config CMD_XXD
>
> endmenu
>
> -menu "Android support commands"
> -
> -config CMD_AB_SELECT
> - bool "ab_select"
> - depends on ANDROID_AB
> - help
> - On Android devices with more than one boot slot (multiple copies of
> - the kernel and system images) this provides a command to select which
> - slot should be used to boot from and register the boot attempt. This
> - is used by the new A/B update model where one slot is updated in the
> - background while running from the other slot.
> -
> -endmenu
> -
> if NET || NET_LWIP
>
> menuconfig CMD_NET
> diff --git a/cmd/Makefile b/cmd/Makefile
> index f30914053721d7e8ac17c828aef4d379cf3ecdb5..d1bb9d0e57d80855dd2da8af0cb510dbad65933e 100644
> --- a/cmd/Makefile
> +++ b/cmd/Makefile
> @@ -17,7 +17,6 @@ obj-$(CONFIG_CMD_2048) += 2048.o
> obj-$(CONFIG_CMD_ACPI) += acpi.o
> obj-$(CONFIG_CMD_ADDRMAP) += addrmap.o
> obj-$(CONFIG_CMD_AES) += aes.o
> -obj-$(CONFIG_CMD_AB_SELECT) += ab_select.o
> obj-$(CONFIG_CMD_ADC) += adc.o
> obj-$(CONFIG_CMD_ARMFLASH) += armflash.o
> obj-$(CONFIG_BLK) += blk_common.o
> diff --git a/cmd/ab_select.c b/cmd/ab_select.c
> deleted file mode 100644
> index 7c178c728ca4c8b5bcba02a04eef2d6a7c86afb6..0000000000000000000000000000000000000000
> --- a/cmd/ab_select.c
> +++ /dev/null
> @@ -1,66 +0,0 @@
> -// SPDX-License-Identifier: BSD-2-Clause
> -/*
> - * Copyright (C) 2017 The Android Open Source Project
> - */
> -
> -#include <android_ab.h>
> -#include <command.h>
> -#include <env.h>
> -#include <part.h>
> -
> -static int do_ab_select(struct cmd_tbl *cmdtp, int flag, int argc,
> - char *const argv[])
> -{
> - int ret;
> - struct blk_desc *dev_desc;
> - struct disk_partition part_info;
> - char slot[2];
> - bool dec_tries = true;
> -
> - if (argc < 4)
> - return CMD_RET_USAGE;
> -
> - for (int i = 4; i < argc; i++) {
> - if (strcmp(argv[i], "--no-dec") == 0) {
> - dec_tries = false;
> - } else {
> - return CMD_RET_USAGE;
> - }
> - }
> -
> - /* Lookup the "misc" partition from argv[2] and argv[3] */
> - if (part_get_info_by_dev_and_name_or_num(argv[2], argv[3],
> - &dev_desc, &part_info,
> - false) < 0) {
> - return CMD_RET_FAILURE;
> - }
> -
> - ret = ab_select_slot(dev_desc, &part_info, dec_tries);
> - if (ret < 0) {
> - printf("Android boot failed, error %d.\n", ret);
> - return CMD_RET_FAILURE;
> - }
> -
> - /* Android standard slot names are 'a', 'b', ... */
> - slot[0] = BOOT_SLOT_NAME(ret);
> - slot[1] = '\0';
> - env_set(argv[1], slot);
> - printf("ANDROID: Booting slot: %s\n", slot);
> - return CMD_RET_SUCCESS;
> -}
> -
> -U_BOOT_CMD(ab_select, 5, 0, do_ab_select,
> - "Select the slot used to boot from and register the boot attempt.",
> - "<slot_var_name> <interface> <dev[:part|#part_name]> [--no-dec]\n"
> - " - Load the slot metadata from the partition 'part' on\n"
> - " device type 'interface' instance 'dev' and store the active\n"
> - " slot in the 'slot_var_name' variable. This also updates the\n"
> - " Android slot metadata with a boot attempt, which can cause\n"
> - " successive calls to this function to return a different result\n"
> - " if the returned slot runs out of boot attempts.\n"
> - " - If 'part_name' is passed, preceded with a # instead of :, the\n"
> - " partition name whose label is 'part_name' will be looked up in\n"
> - " the partition table. This is commonly the \"misc\" partition.\n"
> - " - If '--no-dec' is set, the number of tries remaining will not\n"
> - " decremented for the selected boot slot\n"
> -);
> diff --git a/cmd/bcb.c b/cmd/bcb.c
> index 98b2a71087a27b1721f4bed4160095d65ec75402..2854408e5669b0d7d2a06073fa81158b21834b99 100644
> --- a/cmd/bcb.c
> +++ b/cmd/bcb.c
> @@ -8,6 +8,7 @@
> #include <android_bootloader_message.h>
> #include <bcb.h>
> #include <command.h>
> +#include <android_ab.h>
> #include <display_options.h>
> #include <log.h>
> #include <part.h>
> @@ -376,6 +377,48 @@ void bcb_reset(void)
> __bcb_reset();
> }
>
> +__maybe_unused static int do_bcb_ab_select(struct cmd_tbl *cmdtp,
> + int flag, int argc,
> + char * const argv[])
> +{
> + int ret;
> + struct blk_desc *dev_desc;
> + struct disk_partition part_info;
> + char slot[2];
> + bool dec_tries = true;
> +
> + if (argc < 4)
> + return CMD_RET_USAGE;
> +
> + for (int i = 4; i < argc; i++) {
> + if (strcmp(argv[i], "--no-dec") == 0)
> + dec_tries = false;
> + else
> + return CMD_RET_USAGE;
> + }
> +
> + /* Lookup the "misc" partition from argv[2] and argv[3] */
> + if (part_get_info_by_dev_and_name_or_num(argv[2], argv[3],
> + &dev_desc, &part_info,
> + false) < 0) {
> + return CMD_RET_FAILURE;
> + }
> +
> + ret = ab_select_slot(dev_desc, &part_info, dec_tries);
> + if (ret < 0) {
> + printf("Android boot failed, error %d.\n", ret);
> + return CMD_RET_FAILURE;
> + }
> +
> + /* Android standard slot names are 'a', 'b', ... */
> + slot[0] = BOOT_SLOT_NAME(ret);
> + slot[1] = '\0';
> + env_set(argv[1], slot);
> + printf("ANDROID: Booting slot: %s\n", slot);
> +
> + return CMD_RET_SUCCESS;
> +}
> +
> U_BOOT_LONGHELP(bcb,
> "load <interface> <dev> <part> - load BCB from <interface> <dev>:<part>\n"
> "load <dev> <part> - load BCB from mmc <dev>:<part>\n"
> @@ -385,6 +428,23 @@ U_BOOT_LONGHELP(bcb,
> "bcb dump <field> - dump BCB <field>\n"
> "bcb store - store BCB back to <interface>\n"
> "\n"
> +#if IS_ENABLED(CONFIG_ANDROID_AB)
> + "bcb ab_select -\n"
> + " Select the slot used to boot from and register the boot attempt.\n"
> + " <slot_var_name> <interface> <dev[:part|#part_name]> [--no-dec]\n"
> + " - Load the slot metadata from the partition 'part' on\n"
> + " device type 'interface' instance 'dev' and store the active\n"
> + " slot in the 'slot_var_name' variable. This also updates the\n"
> + " Android slot metadata with a boot attempt, which can cause\n"
> + " successive calls to this function to return a different result\n"
> + " if the returned slot runs out of boot attempts.\n"
> + " - If 'part_name' is passed, preceded with a # instead of :, the\n"
> + " partition name whose label is 'part_name' will be looked up in\n"
> + " the partition table. This is commonly the \"misc\" partition.\n"
> + " - If '--no-dec' is set, the number of tries remaining will not\n"
> + " decremented for the selected boot slot\n"
> + "\n"
> +#endif
> "Legend:\n"
> "<interface> - storage device interface (virtio, mmc, etc)\n"
> "<dev> - storage device index containing the BCB partition\n"
> @@ -406,4 +466,7 @@ U_BOOT_CMD_WITH_SUBCMDS(bcb,
> U_BOOT_SUBCMD_MKENT(test, 4, 1, do_bcb_test),
> U_BOOT_SUBCMD_MKENT(dump, 2, 1, do_bcb_dump),
> U_BOOT_SUBCMD_MKENT(store, 1, 1, do_bcb_store),
> +#if IS_ENABLED(CONFIG_ANDROID_AB)
> + U_BOOT_SUBCMD_MKENT(ab_select, 5, 1, do_bcb_ab_select),
> +#endif
> );
> diff --git a/configs/am57xx_evm_defconfig b/configs/am57xx_evm_defconfig
> index efc154eda043ff3ea08194288e33792ce48282f9..b793f00babe474ea3f15292fb4015a7120401238 100644
> --- a/configs/am57xx_evm_defconfig
> +++ b/configs/am57xx_evm_defconfig
> @@ -48,7 +48,6 @@ CONFIG_CMD_SPL=y
> CONFIG_SYS_I2C_EEPROM_ADDR_LEN=2
> CONFIG_CMD_BCB=y
> # CONFIG_CMD_SETEXPR is not set
> -CONFIG_CMD_AB_SELECT=y
> CONFIG_BOOTP_DNS2=y
> # CONFIG_CMD_PMIC is not set
> CONFIG_CMD_AVB=y
> diff --git a/configs/am57xx_hs_evm_defconfig b/configs/am57xx_hs_evm_defconfig
> index 0f8533e15dbd7c0186a513b27b46a0407b6f79f1..5cacd7f9cc53d338d52120186b16684add93fd21 100644
> --- a/configs/am57xx_hs_evm_defconfig
> +++ b/configs/am57xx_hs_evm_defconfig
> @@ -44,7 +44,6 @@ CONFIG_CMD_ADTIMG=y
> CONFIG_CMD_ABOOTIMG=y
> CONFIG_SYS_I2C_EEPROM_ADDR_LEN=2
> CONFIG_CMD_BCB=y
> -CONFIG_CMD_AB_SELECT=y
> CONFIG_BOOTP_DNS2=y
> # CONFIG_CMD_PMIC is not set
> CONFIG_CMD_AVB=y
> diff --git a/configs/am57xx_hs_evm_usb_defconfig b/configs/am57xx_hs_evm_usb_defconfig
> index 81a938339d5934605cb7defa04ea92f76468b21a..2d8068ecdc79c01c1281ab3873fc892aa4c96be7 100644
> --- a/configs/am57xx_hs_evm_usb_defconfig
> +++ b/configs/am57xx_hs_evm_usb_defconfig
> @@ -46,7 +46,6 @@ CONFIG_CMD_ADTIMG=y
> CONFIG_CMD_ABOOTIMG=y
> CONFIG_SYS_I2C_EEPROM_ADDR_LEN=2
> CONFIG_CMD_BCB=y
> -CONFIG_CMD_AB_SELECT=y
> CONFIG_BOOTP_DNS2=y
> # CONFIG_CMD_PMIC is not set
> CONFIG_CMD_AVB=y
> diff --git a/configs/khadas-vim3_android_ab_defconfig b/configs/khadas-vim3_android_ab_defconfig
> index 510fe4f8928fe39a040a615636fa550b3e0dc5db..de5357c45cbfe4742d9491a29386850570acc235 100644
> --- a/configs/khadas-vim3_android_ab_defconfig
> +++ b/configs/khadas-vim3_android_ab_defconfig
> @@ -47,7 +47,6 @@ CONFIG_CMD_SPI=y
> CONFIG_CMD_USB=y
> CONFIG_CMD_USB_MASS_STORAGE=y
> # CONFIG_CMD_SETEXPR is not set
> -CONFIG_CMD_AB_SELECT=y
> CONFIG_CMD_REGULATOR=y
> CONFIG_CMD_AVB=y
> CONFIG_OF_CONTROL=y
> diff --git a/configs/khadas-vim3l_android_ab_defconfig b/configs/khadas-vim3l_android_ab_defconfig
> index d2da8ff2a69b209b8fb22a48be537bd4dd17a3bb..4d7b90f23002e464d7dc40516bcd3161b0f59439 100644
> --- a/configs/khadas-vim3l_android_ab_defconfig
> +++ b/configs/khadas-vim3l_android_ab_defconfig
> @@ -47,7 +47,6 @@ CONFIG_CMD_SPI=y
> CONFIG_CMD_USB=y
> CONFIG_CMD_USB_MASS_STORAGE=y
> # CONFIG_CMD_SETEXPR is not set
> -CONFIG_CMD_AB_SELECT=y
> CONFIG_CMD_REGULATOR=y
> CONFIG_CMD_AVB=y
> CONFIG_OF_CONTROL=y
> diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
> index 1b3b8c6e788cd6845b61e62a06b730da28831edc..b5f80b8572ad32b2a92d4fe82c9068c453c11dfd 100644
> --- a/configs/sandbox64_defconfig
> +++ b/configs/sandbox64_defconfig
> @@ -27,6 +27,7 @@ CONFIG_CONSOLE_RECORD=y
> CONFIG_CONSOLE_RECORD_OUT_SIZE=0x6000
> CONFIG_PRE_CONSOLE_BUFFER=y
> CONFIG_DISPLAY_BOARDINFO_LATE=y
> +CONFIG_ANDROID_AB=y
> CONFIG_CMD_CPU=y
> CONFIG_CMD_LICENSE=y
> CONFIG_CMD_BOOTZ=y
> @@ -46,6 +47,7 @@ CONFIG_CMD_MD5SUM=y
> CONFIG_CMD_MEMINFO=y
> CONFIG_CMD_MX_CYCLIC=y
> CONFIG_CMD_MEMTEST=y
> +CONFIG_CMD_BCB=y
> CONFIG_CMD_CLK=y
> CONFIG_CMD_DEMO=y
> CONFIG_CMD_GPIO=y
> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index f596f1cc55a45ade4862122a6944d109c6c29238..d111858082d5a8ed3725c95462b2c81a406b57d2 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -103,7 +103,6 @@ CONFIG_CMD_AXI=y
> CONFIG_CMD_CAT=y
> CONFIG_CMD_SETEXPR_FMT=y
> CONFIG_CMD_XXD=y
> -CONFIG_CMD_AB_SELECT=y
> CONFIG_CMD_DHCP6=y
> CONFIG_BOOTP_DNS2=y
> CONFIG_CMD_PCAP=y
> diff --git a/doc/android/ab.rst b/doc/android/ab.rst
> index 2adf88781d60b61d1b3c74efe976a684b590c813..7fd4aeb6a724b839de9be5e9a8843ade2ad3667e 100644
> --- a/doc/android/ab.rst
> +++ b/doc/android/ab.rst
> @@ -18,7 +18,7 @@ The A/B updates support can be activated by specifying next options in
> your board configuration file::
>
> CONFIG_ANDROID_AB=y
> - CONFIG_CMD_AB_SELECT=y
> + CONFIG_CMD_BCB=y
>
> The disk space on target device must be partitioned in a way so that each
> partition which needs to be updated has two or more instances. The name of
> @@ -26,8 +26,8 @@ each instance must be formed by adding suffixes: ``_a``, ``_b``, ``_c``, etc.
> For example: ``boot_a``, ``boot_b``, ``system_a``, ``system_b``, ``vendor_a``,
> ``vendor_b``.
>
> -As a result you can use ``ab_select`` command to ensure A/B boot process in your
> -boot script. This command analyzes and processes A/B metadata stored on a
> +As a result you can use ``bcb ab_select`` command to ensure A/B boot process in
> +your boot script. This command analyzes and processes A/B metadata stored on a
> special partition (e.g. ``misc``) and determines which slot should be used for
> booting up.
>
> @@ -42,15 +42,15 @@ Command usage
>
> .. code-block:: none
>
> - ab_select <slot_var_name> <interface> <dev[:part_number|#part_name]>
> + bcb ab_select <slot_var_name> <interface> <dev[:part_number|#part_name]>
>
> for example::
>
> - => ab_select slot_name mmc 1:4
> + => bcb ab_select slot_name mmc 1:4
>
> or::
>
> - => ab_select slot_name mmc 1#misc
> + => bcb ab_select slot_name mmc 1#misc
>
> Result::
>
> diff --git a/include/configs/khadas-vim3_android.h b/include/configs/khadas-vim3_android.h
> index b76e049f09cc014837981e914a3792c829f42cdc..fc89efb4c36e5216a1344bb2d758300eec3c8f44 100644
> --- a/include/configs/khadas-vim3_android.h
> +++ b/include/configs/khadas-vim3_android.h
> @@ -12,7 +12,7 @@
> #define LOGO_UUID "43a3305d-150f-4cc9-bd3b-38fca8693846;"
> #define ROOT_UUID "ddb8c3f6-d94d-4394-b633-3134139cc2e0;"
>
> -#if defined(CONFIG_CMD_AB_SELECT)
> +#if defined(CONFIG_CMD_BCB) && defined(CONFIG_ANDROID_AB)
> #define PARTS_DEFAULT \
> "uuid_disk=${uuid_gpt_disk};" \
> "name=logo,start=512K,size=2M,uuid=" LOGO_UUID \
> diff --git a/include/configs/khadas-vim3l_android.h b/include/configs/khadas-vim3l_android.h
> index 0ab8ffd372a4ae44804fca41c11ee930aaa72460..5b2aed1cf62af213f0ea6688fde6d07267aa4f55 100644
> --- a/include/configs/khadas-vim3l_android.h
> +++ b/include/configs/khadas-vim3l_android.h
> @@ -12,7 +12,7 @@
> #define LOGO_UUID "43a3305d-150f-4cc9-bd3b-38fca8693846;"
> #define ROOT_UUID "ddb8c3f6-d94d-4394-b633-3134139cc2e0;"
>
> -#if defined(CONFIG_CMD_AB_SELECT)
> +#if defined(CONFIG_CMD_BCB) && defined(CONFIG_ANDROID_AB)
> #define PARTS_DEFAULT \
> "uuid_disk=${uuid_gpt_disk};" \
> "name=logo,start=512K,size=2M,uuid=" LOGO_UUID \
> diff --git a/include/configs/meson64_android.h b/include/configs/meson64_android.h
> index fa520265800ccf081862bc97643b6a8e28b13327..77364bbf9cf0d1b68d413773902ea9d18b720928 100644
> --- a/include/configs/meson64_android.h
> +++ b/include/configs/meson64_android.h
> @@ -47,13 +47,13 @@
> #define AVB_VERIFY_CMD ""
> #endif
>
> -#if defined(CONFIG_CMD_AB_SELECT)
> +#if defined(CONFIG_CMD_BCB) && defined(CONFIG_ANDROID_AB)
> #define ANDROIDBOOT_GET_CURRENT_SLOT_CMD "get_current_slot=" \
> "if part number mmc ${mmcdev} " CONTROL_PARTITION " control_part_number; " \
> "then " \
> "echo " CONTROL_PARTITION \
> " partition number:${control_part_number};" \
> - "ab_select current_slot mmc ${mmcdev}:${control_part_number};" \
> + "bcb ab_select current_slot mmc ${mmcdev}:${control_part_number};" \
> "else " \
> "echo " CONTROL_PARTITION " partition not found;" \
> "fi;\0"
> diff --git a/include/configs/ti_omap5_common.h b/include/configs/ti_omap5_common.h
> index 26494ae980108ad84eb173a0deaa7b701a5309d4..26b6c1cd188c05371c6455cd6247f07a1773d885 100644
> --- a/include/configs/ti_omap5_common.h
> +++ b/include/configs/ti_omap5_common.h
> @@ -93,13 +93,13 @@
>
> #define CONTROL_PARTITION "misc"
>
> -#if defined(CONFIG_CMD_AB_SELECT)
> +#if defined(CONFIG_CMD_BCB) && defined(CONFIG_ANDROID_AB)
> #define AB_SELECT_SLOT \
> "if part number mmc 1 " CONTROL_PARTITION " control_part_number; " \
> "then " \
> "echo " CONTROL_PARTITION \
> " partition number:${control_part_number};" \
> - "ab_select slot_name mmc ${mmcdev}:${control_part_number};" \
> + "bcb ab_select slot_name mmc ${mmcdev}:${control_part_number};" \
> "else " \
> "echo " CONTROL_PARTITION " partition not found;" \
> "exit;" \
> diff --git a/test/py/tests/test_android/test_ab.py b/test/py/tests/test_android/test_ab.py
> index c79cb07fda35dfeb608ac7345cd3f22744e2e491..0d7b7995a9fab6e3daad748721818b9e4cfac452 100644
> --- a/test/py/tests/test_android/test_ab.py
> +++ b/test/py/tests/test_android/test_ab.py
> @@ -56,20 +56,20 @@ def ab_disk_image(u_boot_console):
>
> @pytest.mark.boardspec('sandbox')
> @pytest.mark.buildconfigspec('android_ab')
> -@pytest.mark.buildconfigspec('cmd_ab_select')
> +@pytest.mark.buildconfigspec('cmd_bcb')
> @pytest.mark.requiredtool('sgdisk')
> def test_ab(ab_disk_image, u_boot_console):
> - """Test the 'ab_select' command."""
> + """Test the 'bcb ab_select' command."""
>
> u_boot_console.run_command('host bind 0 ' + ab_disk_image.path)
>
> - output = u_boot_console.run_command('ab_select slot_name host 0#misc')
> + output = u_boot_console.run_command('bcb ab_select slot_name host 0#misc')
> assert 're-initializing A/B metadata' in output
> assert 'Attempting slot a, tries remaining 7' in output
> output = u_boot_console.run_command('printenv slot_name')
> assert 'slot_name=a' in output
>
> - output = u_boot_console.run_command('ab_select slot_name host 0:1')
> + output = u_boot_console.run_command('bcb ab_select slot_name host 0:1')
> assert 'Attempting slot b, tries remaining 7' in output
> output = u_boot_console.run_command('printenv slot_name')
> assert 'slot_name=b' in output
>
> --
> 2.43.0
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 4/6] cmd: bcb: change strcmp() usage style in the do_bcb_ab_select()
2024-10-17 14:12 [PATCH v5 0/6] android_ab: introduce bcb ab_dump command and provide several bcb fixes Dmitry Rokosov
` (2 preceding siblings ...)
2024-10-17 14:12 ` [PATCH v5 3/6] treewide: bcb: move ab_select command to bcb subcommands Dmitry Rokosov
@ 2024-10-17 14:12 ` Dmitry Rokosov
2024-10-17 14:12 ` [PATCH v5 5/6] cmd: bcb: introduce 'ab_dump' command to print BCB block content Dmitry Rokosov
` (4 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: Dmitry Rokosov @ 2024-10-17 14:12 UTC (permalink / raw)
To: Igor Opaniuk, Mattijs Korpershoek, Sam Protsenko, Tom Rini,
Andrew F. Davis, Neil Armstrong, Simon Glass, Mario Six
Cc: u-boot, u-boot-amlogic, rockosov, kernel, Dmitry Rokosov,
Guillaume La Roque
In the entire cmd/bcb.c file, the return value of strcmp() is not
directly compared to 0. Therefore, it would be better to maintain this
style in the new do_bcb_ab_select() function as well.
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Tested-by: Guillaume La Roque <glaroque@baylibre.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
cmd/bcb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cmd/bcb.c b/cmd/bcb.c
index 2854408e5669b0d7d2a06073fa81158b21834b99..b33c046af0385a112fd40634ab7f48e05542ca48 100644
--- a/cmd/bcb.c
+++ b/cmd/bcb.c
@@ -391,7 +391,7 @@ __maybe_unused static int do_bcb_ab_select(struct cmd_tbl *cmdtp,
return CMD_RET_USAGE;
for (int i = 4; i < argc; i++) {
- if (strcmp(argv[i], "--no-dec") == 0)
+ if (!strcmp(argv[i], "--no-dec"))
dec_tries = false;
else
return CMD_RET_USAGE;
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v5 5/6] cmd: bcb: introduce 'ab_dump' command to print BCB block content
2024-10-17 14:12 [PATCH v5 0/6] android_ab: introduce bcb ab_dump command and provide several bcb fixes Dmitry Rokosov
` (3 preceding siblings ...)
2024-10-17 14:12 ` [PATCH v5 4/6] cmd: bcb: change strcmp() usage style in the do_bcb_ab_select() Dmitry Rokosov
@ 2024-10-17 14:12 ` Dmitry Rokosov
2024-10-22 9:03 ` Mattijs Korpershoek
2024-10-17 14:12 ` [PATCH v5 6/6] common: android_ab: fix slot suffix for abc block Dmitry Rokosov
` (3 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: Dmitry Rokosov @ 2024-10-17 14:12 UTC (permalink / raw)
To: Igor Opaniuk, Mattijs Korpershoek, Sam Protsenko, Tom Rini,
Andrew F. Davis, Neil Armstrong, Simon Glass, Mario Six
Cc: u-boot, u-boot-amlogic, rockosov, kernel, Dmitry Rokosov
It's really helpful to have the ability to dump BCB block for debugging
A/B logic on the board supported this partition schema.
Command 'bcb ab_dump' prints all fields of bootloader_control struct
including slot_metadata for all presented slots.
Output example:
=====
> board# bcb ab_dump ubi 0#misc
> Read 512 bytes from volume misc to 000000000bf07580
> Read 512 bytes from volume misc to 000000000bf42f40
> Bootloader Control: [misc]
> Active Slot: _a
> Magic Number: 0x42414342
> Version: 1
> Number of Slots: 2
> Recovery Tries Remaining: 0
> CRC: 0x2c8b50bc (Valid)
>
> Slot[0] Metadata:
> - Priority: 15
> - Tries Remaining: 0
> - Successful Boot: 1
> - Verity Corrupted: 0
>
> Slot[1] Metadata:
> - Priority: 14
> - Tries Remaining: 7
> - Successful Boot: 0
> - Verity Corrupted: 0
====
The ab_dump command allows you to display ABC data directly on the
U-Boot console. During an A/B test execution, this test verifies the
accuracy of each field within the ABC data.
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
boot/android_ab.c | 68 +++++++++++++++++++++++++++++++++++
cmd/bcb.c | 31 ++++++++++++++++
include/android_ab.h | 10 ++++++
test/py/tests/test_android/test_ab.py | 23 ++++++++++++
4 files changed, 132 insertions(+)
diff --git a/boot/android_ab.c b/boot/android_ab.c
index 0045c8133a8e164f1fdd4c0f9b683de0f13f26e0..c93e51541019d0fe793303c4b3d5286df061906f 100644
--- a/boot/android_ab.c
+++ b/boot/android_ab.c
@@ -372,3 +372,71 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
return slot;
}
+
+int ab_dump_abc(struct blk_desc *dev_desc, struct disk_partition *part_info)
+{
+ struct bootloader_control *abc;
+ u32 crc32_le;
+ int i, ret;
+ struct slot_metadata *slot;
+
+ if (!dev_desc || !part_info) {
+ log_err("ANDROID: Empty device descriptor or partition info\n");
+ return -EINVAL;
+ }
+
+ ret = ab_control_create_from_disk(dev_desc, part_info, &abc, 0);
+ if (ret < 0) {
+ log_err("ANDROID: Cannot create bcb from disk %d\n", ret);
+ return ret;
+ }
+
+ if (abc->magic != BOOT_CTRL_MAGIC) {
+ log_err("ANDROID: Unknown A/B metadata: %.8x\n", abc->magic);
+ ret = -ENODATA;
+ goto error;
+ }
+
+ if (abc->version > BOOT_CTRL_VERSION) {
+ log_err("ANDROID: Unsupported A/B metadata version: %.8x\n",
+ abc->version);
+ ret = -ENODATA;
+ goto error;
+ }
+
+ if (abc->nb_slot > ARRAY_SIZE(abc->slot_info)) {
+ log_err("ANDROID: Wrong number of slots %u, expected %zu\n",
+ abc->nb_slot, ARRAY_SIZE(abc->slot_info));
+ ret = -ENODATA;
+ goto error;
+ }
+
+ printf("Bootloader Control: [%s]\n", part_info->name);
+ printf("Active Slot: %s\n", abc->slot_suffix);
+ printf("Magic Number: 0x%x\n", abc->magic);
+ printf("Version: %u\n", abc->version);
+ printf("Number of Slots: %u\n", abc->nb_slot);
+ printf("Recovery Tries Remaining: %u\n", abc->recovery_tries_remaining);
+
+ printf("CRC: 0x%.8x", abc->crc32_le);
+
+ crc32_le = ab_control_compute_crc(abc);
+ if (abc->crc32_le != crc32_le)
+ printf(" (Invalid, Expected: 0x%.8x)\n", crc32_le);
+ else
+ printf(" (Valid)\n");
+
+ for (i = 0; i < abc->nb_slot; ++i) {
+ slot = &abc->slot_info[i];
+ printf("\nSlot[%d] Metadata:\n", i);
+ printf("\t- Priority: %u\n", slot->priority);
+ printf("\t- Tries Remaining: %u\n", slot->tries_remaining);
+ printf("\t- Successful Boot: %u\n", slot->successful_boot);
+ printf("\t- Verity Corrupted: %u\n", slot->verity_corrupted);
+ }
+
+error:
+ free(abc);
+
+ return ret;
+}
diff --git a/cmd/bcb.c b/cmd/bcb.c
index b33c046af0385a112fd40634ab7f48e05542ca48..16eabfe00f5a443f710e5084d187e374f89f6d3a 100644
--- a/cmd/bcb.c
+++ b/cmd/bcb.c
@@ -419,6 +419,32 @@ __maybe_unused static int do_bcb_ab_select(struct cmd_tbl *cmdtp,
return CMD_RET_SUCCESS;
}
+__maybe_unused static int do_bcb_ab_dump(struct cmd_tbl *cmdtp,
+ int flag, int argc,
+ char *const argv[])
+{
+ int ret;
+ struct blk_desc *dev_desc;
+ struct disk_partition part_info;
+
+ if (argc < 3)
+ return CMD_RET_USAGE;
+
+ if (part_get_info_by_dev_and_name_or_num(argv[1], argv[2],
+ &dev_desc, &part_info,
+ false) < 0) {
+ return CMD_RET_FAILURE;
+ }
+
+ ret = ab_dump_abc(dev_desc, &part_info);
+ if (ret < 0) {
+ printf("Cannot dump ABC data, error %d.\n", ret);
+ return CMD_RET_FAILURE;
+ }
+
+ return CMD_RET_SUCCESS;
+}
+
U_BOOT_LONGHELP(bcb,
"load <interface> <dev> <part> - load BCB from <interface> <dev>:<part>\n"
"load <dev> <part> - load BCB from mmc <dev>:<part>\n"
@@ -444,6 +470,10 @@ U_BOOT_LONGHELP(bcb,
" - If '--no-dec' is set, the number of tries remaining will not\n"
" decremented for the selected boot slot\n"
"\n"
+ "bcb ab_dump -\n"
+ " Dump boot_control information from specific partition.\n"
+ " <interface> <dev[:part|#part_name]>\n"
+ "\n"
#endif
"Legend:\n"
"<interface> - storage device interface (virtio, mmc, etc)\n"
@@ -468,5 +498,6 @@ U_BOOT_CMD_WITH_SUBCMDS(bcb,
U_BOOT_SUBCMD_MKENT(store, 1, 1, do_bcb_store),
#if IS_ENABLED(CONFIG_ANDROID_AB)
U_BOOT_SUBCMD_MKENT(ab_select, 5, 1, do_bcb_ab_select),
+ U_BOOT_SUBCMD_MKENT(ab_dump, 3, 1, do_bcb_ab_dump),
#endif
);
diff --git a/include/android_ab.h b/include/android_ab.h
index 1e53879a25f145a9d18ac0a6553d8c217123aa6f..838230e06f8cbf7a5d79d9d84d9ebe9f96aca10d 100644
--- a/include/android_ab.h
+++ b/include/android_ab.h
@@ -36,4 +36,14 @@ struct disk_partition;
int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
bool dec_tries);
+/**
+ * ab_dump_abc() - Dump ABC information for specific partition.
+ *
+ * @dev_desc: Device description pointer
+ * @part_info: Partition information
+ *
+ * Return: 0 on success, or a negative on error
+ */
+int ab_dump_abc(struct blk_desc *dev_desc, struct disk_partition *part_info);
+
#endif /* __ANDROID_AB_H */
diff --git a/test/py/tests/test_android/test_ab.py b/test/py/tests/test_android/test_ab.py
index 0d7b7995a9fab6e3daad748721818b9e4cfac452..9bf1a0eb00a6ae1edebf62f07fd162b9c8c02e49 100644
--- a/test/py/tests/test_android/test_ab.py
+++ b/test/py/tests/test_android/test_ab.py
@@ -54,6 +54,27 @@ def ab_disk_image(u_boot_console):
di = ABTestDiskImage(u_boot_console)
return di
+def ab_dump(u_boot_console, slot_num, crc):
+ output = u_boot_console.run_command('bcb ab_dump host 0#misc')
+ header, slot0, slot1 = output.split('\r\r\n\r\r\n')
+ slots = [slot0, slot1]
+ slot_suffixes = ['_a', '_b']
+
+ header = dict(map(lambda x: map(str.strip, x.split(':')), header.split('\r\r\n')))
+ assert header['Bootloader Control'] == '[misc]'
+ assert header['Active Slot'] == slot_suffixes[slot_num]
+ assert header['Magic Number'] == '0x42414342'
+ assert header['Version'] == '1'
+ assert header['Number of Slots'] == '2'
+ assert header['Recovery Tries Remaining'] == '0'
+ assert header['CRC'] == '{} (Valid)'.format(crc)
+
+ slot = dict(map(lambda x: map(str.strip, x.split(':')), slots[slot_num].split('\r\r\n\t- ')[1:]))
+ assert slot['Priority'] == '15'
+ assert slot['Tries Remaining'] == '6'
+ assert slot['Successful Boot'] == '0'
+ assert slot['Verity Corrupted'] == '0'
+
@pytest.mark.boardspec('sandbox')
@pytest.mark.buildconfigspec('android_ab')
@pytest.mark.buildconfigspec('cmd_bcb')
@@ -68,8 +89,10 @@ def test_ab(ab_disk_image, u_boot_console):
assert 'Attempting slot a, tries remaining 7' in output
output = u_boot_console.run_command('printenv slot_name')
assert 'slot_name=a' in output
+ ab_dump(u_boot_console, 0, '0xd438d1b9')
output = u_boot_console.run_command('bcb ab_select slot_name host 0:1')
assert 'Attempting slot b, tries remaining 7' in output
output = u_boot_console.run_command('printenv slot_name')
assert 'slot_name=b' in output
+ ab_dump(u_boot_console, 1, '0x011ec016')
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 5/6] cmd: bcb: introduce 'ab_dump' command to print BCB block content
2024-10-17 14:12 ` [PATCH v5 5/6] cmd: bcb: introduce 'ab_dump' command to print BCB block content Dmitry Rokosov
@ 2024-10-22 9:03 ` Mattijs Korpershoek
0 siblings, 0 replies; 21+ messages in thread
From: Mattijs Korpershoek @ 2024-10-22 9:03 UTC (permalink / raw)
To: Dmitry Rokosov, Igor Opaniuk, Sam Protsenko, Tom Rini,
Andrew F. Davis, Neil Armstrong, Simon Glass, Mario Six
Cc: u-boot, u-boot-amlogic, rockosov, kernel, Dmitry Rokosov
Hi Dmitry,
Thank you for the patch.
On jeu., oct. 17, 2024 at 17:12, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> It's really helpful to have the ability to dump BCB block for debugging
> A/B logic on the board supported this partition schema.
>
> Command 'bcb ab_dump' prints all fields of bootloader_control struct
> including slot_metadata for all presented slots.
>
> Output example:
> =====
>> board# bcb ab_dump ubi 0#misc
>> Read 512 bytes from volume misc to 000000000bf07580
>> Read 512 bytes from volume misc to 000000000bf42f40
>> Bootloader Control: [misc]
>> Active Slot: _a
>> Magic Number: 0x42414342
>> Version: 1
>> Number of Slots: 2
>> Recovery Tries Remaining: 0
>> CRC: 0x2c8b50bc (Valid)
>>
>> Slot[0] Metadata:
>> - Priority: 15
>> - Tries Remaining: 0
>> - Successful Boot: 1
>> - Verity Corrupted: 0
>>
>> Slot[1] Metadata:
>> - Priority: 14
>> - Tries Remaining: 7
>> - Successful Boot: 0
>> - Verity Corrupted: 0
> ====
>
> The ab_dump command allows you to display ABC data directly on the
> U-Boot console. During an A/B test execution, this test verifies the
> accuracy of each field within the ABC data.
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> ---
> boot/android_ab.c | 68 +++++++++++++++++++++++++++++++++++
> cmd/bcb.c | 31 ++++++++++++++++
> include/android_ab.h | 10 ++++++
> test/py/tests/test_android/test_ab.py | 23 ++++++++++++
> 4 files changed, 132 insertions(+)
>
> diff --git a/boot/android_ab.c b/boot/android_ab.c
> index 0045c8133a8e164f1fdd4c0f9b683de0f13f26e0..c93e51541019d0fe793303c4b3d5286df061906f 100644
> --- a/boot/android_ab.c
> +++ b/boot/android_ab.c
> @@ -372,3 +372,71 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>
> return slot;
> }
> +
> +int ab_dump_abc(struct blk_desc *dev_desc, struct disk_partition *part_info)
> +{
> + struct bootloader_control *abc;
> + u32 crc32_le;
> + int i, ret;
> + struct slot_metadata *slot;
> +
> + if (!dev_desc || !part_info) {
> + log_err("ANDROID: Empty device descriptor or partition info\n");
> + return -EINVAL;
> + }
> +
> + ret = ab_control_create_from_disk(dev_desc, part_info, &abc, 0);
> + if (ret < 0) {
> + log_err("ANDROID: Cannot create bcb from disk %d\n", ret);
> + return ret;
> + }
> +
> + if (abc->magic != BOOT_CTRL_MAGIC) {
> + log_err("ANDROID: Unknown A/B metadata: %.8x\n", abc->magic);
> + ret = -ENODATA;
> + goto error;
> + }
> +
> + if (abc->version > BOOT_CTRL_VERSION) {
> + log_err("ANDROID: Unsupported A/B metadata version: %.8x\n",
> + abc->version);
> + ret = -ENODATA;
> + goto error;
> + }
> +
> + if (abc->nb_slot > ARRAY_SIZE(abc->slot_info)) {
> + log_err("ANDROID: Wrong number of slots %u, expected %zu\n",
> + abc->nb_slot, ARRAY_SIZE(abc->slot_info));
> + ret = -ENODATA;
> + goto error;
> + }
> +
> + printf("Bootloader Control: [%s]\n", part_info->name);
> + printf("Active Slot: %s\n", abc->slot_suffix);
> + printf("Magic Number: 0x%x\n", abc->magic);
> + printf("Version: %u\n", abc->version);
> + printf("Number of Slots: %u\n", abc->nb_slot);
> + printf("Recovery Tries Remaining: %u\n", abc->recovery_tries_remaining);
> +
> + printf("CRC: 0x%.8x", abc->crc32_le);
> +
> + crc32_le = ab_control_compute_crc(abc);
> + if (abc->crc32_le != crc32_le)
> + printf(" (Invalid, Expected: 0x%.8x)\n", crc32_le);
> + else
> + printf(" (Valid)\n");
> +
> + for (i = 0; i < abc->nb_slot; ++i) {
> + slot = &abc->slot_info[i];
> + printf("\nSlot[%d] Metadata:\n", i);
> + printf("\t- Priority: %u\n", slot->priority);
> + printf("\t- Tries Remaining: %u\n", slot->tries_remaining);
> + printf("\t- Successful Boot: %u\n", slot->successful_boot);
> + printf("\t- Verity Corrupted: %u\n", slot->verity_corrupted);
> + }
> +
> +error:
> + free(abc);
> +
> + return ret;
> +}
> diff --git a/cmd/bcb.c b/cmd/bcb.c
> index b33c046af0385a112fd40634ab7f48e05542ca48..16eabfe00f5a443f710e5084d187e374f89f6d3a 100644
> --- a/cmd/bcb.c
> +++ b/cmd/bcb.c
> @@ -419,6 +419,32 @@ __maybe_unused static int do_bcb_ab_select(struct cmd_tbl *cmdtp,
> return CMD_RET_SUCCESS;
> }
>
> +__maybe_unused static int do_bcb_ab_dump(struct cmd_tbl *cmdtp,
> + int flag, int argc,
> + char *const argv[])
> +{
> + int ret;
> + struct blk_desc *dev_desc;
> + struct disk_partition part_info;
> +
> + if (argc < 3)
> + return CMD_RET_USAGE;
> +
> + if (part_get_info_by_dev_and_name_or_num(argv[1], argv[2],
> + &dev_desc, &part_info,
> + false) < 0) {
> + return CMD_RET_FAILURE;
> + }
> +
> + ret = ab_dump_abc(dev_desc, &part_info);
> + if (ret < 0) {
> + printf("Cannot dump ABC data, error %d.\n", ret);
> + return CMD_RET_FAILURE;
> + }
> +
> + return CMD_RET_SUCCESS;
> +}
> +
> U_BOOT_LONGHELP(bcb,
> "load <interface> <dev> <part> - load BCB from <interface> <dev>:<part>\n"
> "load <dev> <part> - load BCB from mmc <dev>:<part>\n"
> @@ -444,6 +470,10 @@ U_BOOT_LONGHELP(bcb,
> " - If '--no-dec' is set, the number of tries remaining will not\n"
> " decremented for the selected boot slot\n"
> "\n"
> + "bcb ab_dump -\n"
> + " Dump boot_control information from specific partition.\n"
> + " <interface> <dev[:part|#part_name]>\n"
> + "\n"
> #endif
> "Legend:\n"
> "<interface> - storage device interface (virtio, mmc, etc)\n"
> @@ -468,5 +498,6 @@ U_BOOT_CMD_WITH_SUBCMDS(bcb,
> U_BOOT_SUBCMD_MKENT(store, 1, 1, do_bcb_store),
> #if IS_ENABLED(CONFIG_ANDROID_AB)
> U_BOOT_SUBCMD_MKENT(ab_select, 5, 1, do_bcb_ab_select),
> + U_BOOT_SUBCMD_MKENT(ab_dump, 3, 1, do_bcb_ab_dump),
> #endif
> );
> diff --git a/include/android_ab.h b/include/android_ab.h
> index 1e53879a25f145a9d18ac0a6553d8c217123aa6f..838230e06f8cbf7a5d79d9d84d9ebe9f96aca10d 100644
> --- a/include/android_ab.h
> +++ b/include/android_ab.h
> @@ -36,4 +36,14 @@ struct disk_partition;
> int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
> bool dec_tries);
>
> +/**
> + * ab_dump_abc() - Dump ABC information for specific partition.
> + *
> + * @dev_desc: Device description pointer
> + * @part_info: Partition information
> + *
> + * Return: 0 on success, or a negative on error
> + */
> +int ab_dump_abc(struct blk_desc *dev_desc, struct disk_partition *part_info);
> +
> #endif /* __ANDROID_AB_H */
> diff --git a/test/py/tests/test_android/test_ab.py b/test/py/tests/test_android/test_ab.py
> index 0d7b7995a9fab6e3daad748721818b9e4cfac452..9bf1a0eb00a6ae1edebf62f07fd162b9c8c02e49 100644
> --- a/test/py/tests/test_android/test_ab.py
> +++ b/test/py/tests/test_android/test_ab.py
> @@ -54,6 +54,27 @@ def ab_disk_image(u_boot_console):
> di = ABTestDiskImage(u_boot_console)
> return di
>
> +def ab_dump(u_boot_console, slot_num, crc):
> + output = u_boot_console.run_command('bcb ab_dump host 0#misc')
> + header, slot0, slot1 = output.split('\r\r\n\r\r\n')
> + slots = [slot0, slot1]
> + slot_suffixes = ['_a', '_b']
> +
> + header = dict(map(lambda x: map(str.strip, x.split(':')), header.split('\r\r\n')))
> + assert header['Bootloader Control'] == '[misc]'
> + assert header['Active Slot'] == slot_suffixes[slot_num]
> + assert header['Magic Number'] == '0x42414342'
> + assert header['Version'] == '1'
> + assert header['Number of Slots'] == '2'
> + assert header['Recovery Tries Remaining'] == '0'
> + assert header['CRC'] == '{} (Valid)'.format(crc)
> +
> + slot = dict(map(lambda x: map(str.strip, x.split(':')), slots[slot_num].split('\r\r\n\t- ')[1:]))
> + assert slot['Priority'] == '15'
> + assert slot['Tries Remaining'] == '6'
> + assert slot['Successful Boot'] == '0'
> + assert slot['Verity Corrupted'] == '0'
> +
> @pytest.mark.boardspec('sandbox')
> @pytest.mark.buildconfigspec('android_ab')
> @pytest.mark.buildconfigspec('cmd_bcb')
> @@ -68,8 +89,10 @@ def test_ab(ab_disk_image, u_boot_console):
> assert 'Attempting slot a, tries remaining 7' in output
> output = u_boot_console.run_command('printenv slot_name')
> assert 'slot_name=a' in output
> + ab_dump(u_boot_console, 0, '0xd438d1b9')
>
> output = u_boot_console.run_command('bcb ab_select slot_name host 0:1')
> assert 'Attempting slot b, tries remaining 7' in output
> output = u_boot_console.run_command('printenv slot_name')
> assert 'slot_name=b' in output
> + ab_dump(u_boot_console, 1, '0x011ec016')
>
> --
> 2.43.0
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v5 6/6] common: android_ab: fix slot suffix for abc block
2024-10-17 14:12 [PATCH v5 0/6] android_ab: introduce bcb ab_dump command and provide several bcb fixes Dmitry Rokosov
` (4 preceding siblings ...)
2024-10-17 14:12 ` [PATCH v5 5/6] cmd: bcb: introduce 'ab_dump' command to print BCB block content Dmitry Rokosov
@ 2024-10-17 14:12 ` Dmitry Rokosov
2024-11-03 9:43 ` Igor Opaniuk
2024-11-04 23:06 ` Sam Protsenko
2024-10-18 12:41 ` [PATCH v5 0/6] android_ab: introduce bcb ab_dump command and provide several bcb fixes Dmitry Rokosov
` (2 subsequent siblings)
8 siblings, 2 replies; 21+ messages in thread
From: Dmitry Rokosov @ 2024-10-17 14:12 UTC (permalink / raw)
To: Igor Opaniuk, Mattijs Korpershoek, Sam Protsenko, Tom Rini,
Andrew F. Davis, Neil Armstrong, Simon Glass, Mario Six
Cc: u-boot, u-boot-amlogic, rockosov, kernel, Dmitry Rokosov,
Guillaume La Roque
To align with the official Android BCB (Bootloader Control Block)
specifications, it's important to note that the slot_suffix should start
with an underscore symbol.
For a comprehensive understanding of the expected slot_suffix format in
userspace, please refer to the provided reference [1].
Links:
[1] - https://source.android.com/docs/core/architecture/bootloader/updating#slots
Based-on: https://android-review.googlesource.com/c/platform/external/u-boot/+/1446439
Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
Tested-by: Guillaume La Roque <glaroque@baylibre.com>
Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
boot/android_ab.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/boot/android_ab.c b/boot/android_ab.c
index c93e51541019d0fe793303c4b3d5286df061906f..a287eac04fe88ad08bdcf1b1b1d6e564d503d800 100644
--- a/boot/android_ab.c
+++ b/boot/android_ab.c
@@ -52,7 +52,7 @@ static int ab_control_default(struct bootloader_control *abc)
if (!abc)
return -EFAULT;
- memcpy(abc->slot_suffix, "a\0\0\0", 4);
+ memcpy(abc->slot_suffix, "_a\0\0", 4);
abc->magic = BOOT_CTRL_MAGIC;
abc->version = BOOT_CTRL_VERSION;
abc->nb_slot = NUM_SLOTS;
@@ -328,7 +328,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
* or the device tree.
*/
memset(slot_suffix, 0, sizeof(slot_suffix));
- slot_suffix[0] = BOOT_SLOT_NAME(slot);
+ slot_suffix[0] = '_';
+ slot_suffix[1] = BOOT_SLOT_NAME(slot);
if (memcmp(abc->slot_suffix, slot_suffix,
sizeof(slot_suffix))) {
memcpy(abc->slot_suffix, slot_suffix,
--
2.43.0
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v5 6/6] common: android_ab: fix slot suffix for abc block
2024-10-17 14:12 ` [PATCH v5 6/6] common: android_ab: fix slot suffix for abc block Dmitry Rokosov
@ 2024-11-03 9:43 ` Igor Opaniuk
2024-11-05 14:54 ` Mattijs Korpershoek
2024-11-04 23:06 ` Sam Protsenko
1 sibling, 1 reply; 21+ messages in thread
From: Igor Opaniuk @ 2024-11-03 9:43 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: Mattijs Korpershoek, Sam Protsenko, Tom Rini, Andrew F. Davis,
Neil Armstrong, Simon Glass, Mario Six, u-boot, u-boot-amlogic,
rockosov, kernel, Guillaume La Roque
Hi Dmitry,
On Thu, Oct 17, 2024 at 4:12 PM Dmitry Rokosov
<ddrokosov@salutedevices.com> wrote:
>
> To align with the official Android BCB (Bootloader Control Block)
> specifications, it's important to note that the slot_suffix should start
> with an underscore symbol.
>
> For a comprehensive understanding of the expected slot_suffix format in
> userspace, please refer to the provided reference [1].
>
> Links:
> [1] - https://source.android.com/docs/core/architecture/bootloader/updating#slots
>
> Based-on: https://android-review.googlesource.com/c/platform/external/u-boot/+/1446439
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Tested-by: Guillaume La Roque <glaroque@baylibre.com>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
> boot/android_ab.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/boot/android_ab.c b/boot/android_ab.c
> index c93e51541019d0fe793303c4b3d5286df061906f..a287eac04fe88ad08bdcf1b1b1d6e564d503d800 100644
> --- a/boot/android_ab.c
> +++ b/boot/android_ab.c
> @@ -52,7 +52,7 @@ static int ab_control_default(struct bootloader_control *abc)
> if (!abc)
> return -EFAULT;
>
> - memcpy(abc->slot_suffix, "a\0\0\0", 4);
> + memcpy(abc->slot_suffix, "_a\0\0", 4);
> abc->magic = BOOT_CTRL_MAGIC;
> abc->version = BOOT_CTRL_VERSION;
> abc->nb_slot = NUM_SLOTS;
> @@ -328,7 +328,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
> * or the device tree.
> */
> memset(slot_suffix, 0, sizeof(slot_suffix));
> - slot_suffix[0] = BOOT_SLOT_NAME(slot);
> + slot_suffix[0] = '_';
> + slot_suffix[1] = BOOT_SLOT_NAME(slot);
> if (memcmp(abc->slot_suffix, slot_suffix,
> sizeof(slot_suffix))) {
> memcpy(abc->slot_suffix, slot_suffix,
>
> --
> 2.43.0
>
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
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 6/6] common: android_ab: fix slot suffix for abc block
2024-11-03 9:43 ` Igor Opaniuk
@ 2024-11-05 14:54 ` Mattijs Korpershoek
0 siblings, 0 replies; 21+ messages in thread
From: Mattijs Korpershoek @ 2024-11-05 14:54 UTC (permalink / raw)
To: Igor Opaniuk, Dmitry Rokosov
Cc: Sam Protsenko, Tom Rini, Andrew F. Davis, Neil Armstrong,
Simon Glass, Mario Six, u-boot, u-boot-amlogic, rockosov, kernel,
Guillaume La Roque
Hi Igor,
Thank you for the review.
On dim., nov. 03, 2024 at 10:43, Igor Opaniuk <igor.opaniuk@gmail.com> wrote:
> Hi Dmitry,
>
> On Thu, Oct 17, 2024 at 4:12 PM Dmitry Rokosov
> <ddrokosov@salutedevices.com> wrote:
>>
>> To align with the official Android BCB (Bootloader Control Block)
>> specifications, it's important to note that the slot_suffix should start
>> with an underscore symbol.
>>
>> For a comprehensive understanding of the expected slot_suffix format in
>> userspace, please refer to the provided reference [1].
>>
>> Links:
>> [1] - https://source.android.com/docs/core/architecture/bootloader/updating#slots
>>
>> Based-on: https://android-review.googlesource.com/c/platform/external/u-boot/+/1446439
>> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> Tested-by: Guillaume La Roque <glaroque@baylibre.com>
>> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>> ---
>> boot/android_ab.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/boot/android_ab.c b/boot/android_ab.c
>> index c93e51541019d0fe793303c4b3d5286df061906f..a287eac04fe88ad08bdcf1b1b1d6e564d503d800 100644
>> --- a/boot/android_ab.c
>> +++ b/boot/android_ab.c
>> @@ -52,7 +52,7 @@ static int ab_control_default(struct bootloader_control *abc)
>> if (!abc)
>> return -EFAULT;
>>
>> - memcpy(abc->slot_suffix, "a\0\0\0", 4);
>> + memcpy(abc->slot_suffix, "_a\0\0", 4);
>> abc->magic = BOOT_CTRL_MAGIC;
>> abc->version = BOOT_CTRL_VERSION;
>> abc->nb_slot = NUM_SLOTS;
>> @@ -328,7 +328,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>> * or the device tree.
>> */
>> memset(slot_suffix, 0, sizeof(slot_suffix));
>> - slot_suffix[0] = BOOT_SLOT_NAME(slot);
>> + slot_suffix[0] = '_';
>> + slot_suffix[1] = BOOT_SLOT_NAME(slot);
>> if (memcmp(abc->slot_suffix, slot_suffix,
>> sizeof(slot_suffix))) {
>> memcpy(abc->slot_suffix, slot_suffix,
>>
>> --
>> 2.43.0
>>
>
> Reviewed-by: Igor Opaniuk <igor.opaniuk@gmail.com>
Sorry, I can't apply that R-b tag because this series has been merged
into master already:
https://lore.kernel.org/r/all/20241025175409.GB4959@bill-the-cat/
> --
> Best regards - Atentamente - Meilleures salutations
>
> Igor Opaniuk
>
> mailto: igor.opaniuk@gmail.com
> skype: igor.opanyuk
> https://www.linkedin.com/in/iopaniuk
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 6/6] common: android_ab: fix slot suffix for abc block
2024-10-17 14:12 ` [PATCH v5 6/6] common: android_ab: fix slot suffix for abc block Dmitry Rokosov
2024-11-03 9:43 ` Igor Opaniuk
@ 2024-11-04 23:06 ` Sam Protsenko
2024-11-05 15:05 ` Mattijs Korpershoek
1 sibling, 1 reply; 21+ messages in thread
From: Sam Protsenko @ 2024-11-04 23:06 UTC (permalink / raw)
To: Dmitry Rokosov
Cc: Igor Opaniuk, Mattijs Korpershoek, Tom Rini, Andrew F. Davis,
Neil Armstrong, Simon Glass, Mario Six, u-boot, u-boot-amlogic,
rockosov, kernel, Guillaume La Roque
On Thu, Oct 17, 2024 at 9:12 AM Dmitry Rokosov
<ddrokosov@salutedevices.com> wrote:
>
> To align with the official Android BCB (Bootloader Control Block)
> specifications, it's important to note that the slot_suffix should start
> with an underscore symbol.
>
> For a comprehensive understanding of the expected slot_suffix format in
> userspace, please refer to the provided reference [1].
>
> Links:
> [1] - https://source.android.com/docs/core/architecture/bootloader/updating#slots
>
> Based-on: https://android-review.googlesource.com/c/platform/external/u-boot/+/1446439
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Tested-by: Guillaume La Roque <glaroque@baylibre.com>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
Would be nice to add "Fixes:" tag here, pointing to the corresponding
commit where the issue was introduced (see kernel docs for details).
It could be quite useful for possible stable branches and other
purposes, I'd recommend to add that tag for all fixes if you have more
in this series.
> ---
> boot/android_ab.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/boot/android_ab.c b/boot/android_ab.c
> index c93e51541019d0fe793303c4b3d5286df061906f..a287eac04fe88ad08bdcf1b1b1d6e564d503d800 100644
> --- a/boot/android_ab.c
> +++ b/boot/android_ab.c
> @@ -52,7 +52,7 @@ static int ab_control_default(struct bootloader_control *abc)
> if (!abc)
> return -EFAULT;
>
> - memcpy(abc->slot_suffix, "a\0\0\0", 4);
> + memcpy(abc->slot_suffix, "_a\0\0", 4);
> abc->magic = BOOT_CTRL_MAGIC;
> abc->version = BOOT_CTRL_VERSION;
> abc->nb_slot = NUM_SLOTS;
> @@ -328,7 +328,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
> * or the device tree.
> */
> memset(slot_suffix, 0, sizeof(slot_suffix));
> - slot_suffix[0] = BOOT_SLOT_NAME(slot);
> + slot_suffix[0] = '_';
> + slot_suffix[1] = BOOT_SLOT_NAME(slot);
AFAIU, this changes the behavior of two above functions, and
consequently of "bcb ab_select" command? If so, just to double check:
were all users of those reworked correspondingly? I can see next
occurrences (there may be more):
$ grep -sIrHn '"_' boot/bootmeth_android.c
boot/bootmeth_android.c:74: sprintf(partname, BOOT_PART_NAME "_%s",
priv->slot);
boot/bootmeth_android.c:111: sprintf(partname,
VENDOR_BOOT_PART_NAME "_%s", priv->slot);
boot/bootmeth_android.c:156: sprintf(slot_suffix, "_%s", priv->slot);
boot/bootmeth_android.c:397: sprintf(slot_suffix, "_%s", priv->slot);
$ grep -sIrHn 'slot_suffix _' include/configs/
include/configs/ti_omap5_common.h:107: "setenv slot_suffix _${slot_name};"
include/configs/meson64_android.h:65: "setenv slot_suffix
_${current_slot}; " \
> if (memcmp(abc->slot_suffix, slot_suffix,
> sizeof(slot_suffix))) {
> memcpy(abc->slot_suffix, slot_suffix,
>
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 6/6] common: android_ab: fix slot suffix for abc block
2024-11-04 23:06 ` Sam Protsenko
@ 2024-11-05 15:05 ` Mattijs Korpershoek
2024-11-06 0:58 ` Sam Protsenko
0 siblings, 1 reply; 21+ messages in thread
From: Mattijs Korpershoek @ 2024-11-05 15:05 UTC (permalink / raw)
To: Sam Protsenko, Dmitry Rokosov
Cc: Igor Opaniuk, Tom Rini, Andrew F. Davis, Neil Armstrong,
Simon Glass, Mario Six, u-boot, u-boot-amlogic, rockosov, kernel,
Guillaume La Roque
Hi Sam,
Thank you for the review.
On lun., nov. 04, 2024 at 17:06, Sam Protsenko <semen.protsenko@linaro.org> wrote:
> On Thu, Oct 17, 2024 at 9:12 AM Dmitry Rokosov
> <ddrokosov@salutedevices.com> wrote:
>>
>> To align with the official Android BCB (Bootloader Control Block)
>> specifications, it's important to note that the slot_suffix should start
>> with an underscore symbol.
>>
>> For a comprehensive understanding of the expected slot_suffix format in
>> userspace, please refer to the provided reference [1].
>>
>> Links:
>> [1] - https://source.android.com/docs/core/architecture/bootloader/updating#slots
>>
>> Based-on: https://android-review.googlesource.com/c/platform/external/u-boot/+/1446439
>> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> Tested-by: Guillaume La Roque <glaroque@baylibre.com>
>> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>
> Would be nice to add "Fixes:" tag here, pointing to the corresponding
> commit where the issue was introduced (see kernel docs for details).
> It could be quite useful for possible stable branches and other
> purposes, I'd recommend to add that tag for all fixes if you have more
> in this series.
I agree. Unfortunately, this series has already been merged here:
https://lore.kernel.org/r/all/20241025175409.GB4959@bill-the-cat/
>
>> ---
>> boot/android_ab.c | 5 +++--
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/boot/android_ab.c b/boot/android_ab.c
>> index c93e51541019d0fe793303c4b3d5286df061906f..a287eac04fe88ad08bdcf1b1b1d6e564d503d800 100644
>> --- a/boot/android_ab.c
>> +++ b/boot/android_ab.c
>> @@ -52,7 +52,7 @@ static int ab_control_default(struct bootloader_control *abc)
>> if (!abc)
>> return -EFAULT;
>>
>> - memcpy(abc->slot_suffix, "a\0\0\0", 4);
>> + memcpy(abc->slot_suffix, "_a\0\0", 4);
>> abc->magic = BOOT_CTRL_MAGIC;
>> abc->version = BOOT_CTRL_VERSION;
>> abc->nb_slot = NUM_SLOTS;
>> @@ -328,7 +328,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>> * or the device tree.
>> */
>> memset(slot_suffix, 0, sizeof(slot_suffix));
>> - slot_suffix[0] = BOOT_SLOT_NAME(slot);
>> + slot_suffix[0] = '_';
>> + slot_suffix[1] = BOOT_SLOT_NAME(slot);
>
> AFAIU, this changes the behavior of two above functions, and
> consequently of "bcb ab_select" command? If so, just to double check:
> were all users of those reworked correspondingly? I can see next
> occurrences (there may be more):
>
> $ grep -sIrHn '"_' boot/bootmeth_android.c
I thought the same when first reviewing the patch.
Looking a bit closer...
>
> boot/bootmeth_android.c:74: sprintf(partname, BOOT_PART_NAME "_%s",
> priv->slot);
> boot/bootmeth_android.c:111: sprintf(partname,
> VENDOR_BOOT_PART_NAME "_%s", priv->slot);
> boot/bootmeth_android.c:156: sprintf(slot_suffix, "_%s", priv->slot);
> boot/bootmeth_android.c:397: sprintf(slot_suffix, "_%s", priv->slot);
... We can see that ab_select_slot() returns an integer
That integer is used later on to initialize priv->slot:
"""
priv->slot[0] = BOOT_SLOT_NAME(ret);
priv->slot[1] = '\0';
"""
The change from Dmitry only changes what we **write** to the BCB (into
the misc partition), not what is returned by ab_select_slot().
ab_select_slot() still returns an integer which needs to be converted
via the BOOT_SLOT_NAME() macro.
>
> $ grep -sIrHn 'slot_suffix _' include/configs/
> include/configs/ti_omap5_common.h:107: "setenv slot_suffix _${slot_name};"
> include/configs/meson64_android.h:65: "setenv slot_suffix
> _${current_slot}; " \
Same goes for these 2 examples, we use:
The "bcb ab_select current_slot" command to store the slot into the
"current_slot" environment variable.
Looking at cmd/bcb.c we can see:
"""
ret = ab_select_slot(dev_desc, &part_info, dec_tries);
if (ret < 0) {
printf("Android boot failed, error %d.\n", ret);
return CMD_RET_FAILURE;
}
/* Android standard slot names are 'a', 'b', ... */
slot[0] = BOOT_SLOT_NAME(ret);
slot[1] = '\0';
env_set(argv[1], slot);
printf("ANDROID: Booting slot: %s\n", slot);
"""
So I think this is fine.
>
>> if (memcmp(abc->slot_suffix, slot_suffix,
>> sizeof(slot_suffix))) {
>> memcpy(abc->slot_suffix, slot_suffix,
>>
>> --
>> 2.43.0
>>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 6/6] common: android_ab: fix slot suffix for abc block
2024-11-05 15:05 ` Mattijs Korpershoek
@ 2024-11-06 0:58 ` Sam Protsenko
2024-11-06 10:02 ` Mattijs Korpershoek
0 siblings, 1 reply; 21+ messages in thread
From: Sam Protsenko @ 2024-11-06 0:58 UTC (permalink / raw)
To: Mattijs Korpershoek
Cc: Dmitry Rokosov, Igor Opaniuk, Tom Rini, Andrew F. Davis,
Neil Armstrong, Simon Glass, Mario Six, u-boot, u-boot-amlogic,
rockosov, kernel, Guillaume La Roque
On Tue, Nov 5, 2024 at 9:06 AM Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
>
> Hi Sam,
>
Hey Mattijs,
[snip]
> >> @@ -328,7 +328,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
> >> * or the device tree.
> >> */
> >> memset(slot_suffix, 0, sizeof(slot_suffix));
> >> - slot_suffix[0] = BOOT_SLOT_NAME(slot);
> >> + slot_suffix[0] = '_';
> >> + slot_suffix[1] = BOOT_SLOT_NAME(slot);
> >
> > AFAIU, this changes the behavior of two above functions, and
> > consequently of "bcb ab_select" command? If so, just to double check:
> > were all users of those reworked correspondingly? I can see next
> > occurrences (there may be more):
> >
> > $ grep -sIrHn '"_' boot/bootmeth_android.c
>
> I thought the same when first reviewing the patch.
> Looking a bit closer...
>
> >
> > boot/bootmeth_android.c:74: sprintf(partname, BOOT_PART_NAME "_%s",
> > priv->slot);
> > boot/bootmeth_android.c:111: sprintf(partname,
> > VENDOR_BOOT_PART_NAME "_%s", priv->slot);
> > boot/bootmeth_android.c:156: sprintf(slot_suffix, "_%s", priv->slot);
> > boot/bootmeth_android.c:397: sprintf(slot_suffix, "_%s", priv->slot);
>
> ... We can see that ab_select_slot() returns an integer
> That integer is used later on to initialize priv->slot:
>
> """
> priv->slot[0] = BOOT_SLOT_NAME(ret);
> priv->slot[1] = '\0';
> """
>
> The change from Dmitry only changes what we **write** to the BCB (into
> the misc partition), not what is returned by ab_select_slot().
>
Sure. Just wanted to double check that the behavior is not changed in
any related parts, as the commit message doesn't mention that. Btw,
BCB is an interface between the bootloader and AOSP, so if this patch
changes what's being written into BCB, does it affect AOSP part of it
somehow? Especially for already existing devices with particular BCB
data, in case U-Boot gets updated there.
> ab_select_slot() still returns an integer which needs to be converted
> via the BOOT_SLOT_NAME() macro.
>
> >
> > $ grep -sIrHn 'slot_suffix _' include/configs/
> > include/configs/ti_omap5_common.h:107: "setenv slot_suffix _${slot_name};"
> > include/configs/meson64_android.h:65: "setenv slot_suffix
> > _${current_slot}; " \
>
> Same goes for these 2 examples, we use:
> The "bcb ab_select current_slot" command to store the slot into the
> "current_slot" environment variable.
> Looking at cmd/bcb.c we can see:
>
> """
> ret = ab_select_slot(dev_desc, &part_info, dec_tries);
> if (ret < 0) {
> printf("Android boot failed, error %d.\n", ret);
> return CMD_RET_FAILURE;
> }
>
> /* Android standard slot names are 'a', 'b', ... */
> slot[0] = BOOT_SLOT_NAME(ret);
> slot[1] = '\0';
> env_set(argv[1], slot);
> printf("ANDROID: Booting slot: %s\n", slot);
> """
>
> So I think this is fine.
>
[snip]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 6/6] common: android_ab: fix slot suffix for abc block
2024-11-06 0:58 ` Sam Protsenko
@ 2024-11-06 10:02 ` Mattijs Korpershoek
2024-11-07 3:17 ` Sam Protsenko
0 siblings, 1 reply; 21+ messages in thread
From: Mattijs Korpershoek @ 2024-11-06 10:02 UTC (permalink / raw)
To: Sam Protsenko
Cc: Dmitry Rokosov, Igor Opaniuk, Tom Rini, Andrew F. Davis,
Neil Armstrong, Simon Glass, Mario Six, u-boot, u-boot-amlogic,
rockosov, kernel, Guillaume La Roque
Hi Sam,
On mar., nov. 05, 2024 at 18:58, Sam Protsenko <semen.protsenko@linaro.org> wrote:
> On Tue, Nov 5, 2024 at 9:06 AM Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
>>
>> Hi Sam,
>>
>
> Hey Mattijs,
>
> [snip]
>
>> >> @@ -328,7 +328,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>> >> * or the device tree.
>> >> */
>> >> memset(slot_suffix, 0, sizeof(slot_suffix));
>> >> - slot_suffix[0] = BOOT_SLOT_NAME(slot);
>> >> + slot_suffix[0] = '_';
>> >> + slot_suffix[1] = BOOT_SLOT_NAME(slot);
>> >
>> > AFAIU, this changes the behavior of two above functions, and
>> > consequently of "bcb ab_select" command? If so, just to double check:
>> > were all users of those reworked correspondingly? I can see next
>> > occurrences (there may be more):
>> >
>> > $ grep -sIrHn '"_' boot/bootmeth_android.c
>>
>> I thought the same when first reviewing the patch.
>> Looking a bit closer...
>>
>> >
>> > boot/bootmeth_android.c:74: sprintf(partname, BOOT_PART_NAME "_%s",
>> > priv->slot);
>> > boot/bootmeth_android.c:111: sprintf(partname,
>> > VENDOR_BOOT_PART_NAME "_%s", priv->slot);
>> > boot/bootmeth_android.c:156: sprintf(slot_suffix, "_%s", priv->slot);
>> > boot/bootmeth_android.c:397: sprintf(slot_suffix, "_%s", priv->slot);
>>
>> ... We can see that ab_select_slot() returns an integer
>> That integer is used later on to initialize priv->slot:
>>
>> """
>> priv->slot[0] = BOOT_SLOT_NAME(ret);
>> priv->slot[1] = '\0';
>> """
>>
>> The change from Dmitry only changes what we **write** to the BCB (into
>> the misc partition), not what is returned by ab_select_slot().
>>
>
> Sure. Just wanted to double check that the behavior is not changed in
> any related parts, as the commit message doesn't mention that. Btw,
> BCB is an interface between the bootloader and AOSP, so if this patch
> changes what's being written into BCB, does it affect AOSP part of it
> somehow? Especially for already existing devices with particular BCB
> data, in case U-Boot gets updated there.
Those are valid concerns.
Per my understanding, on recent Android versions the slot suffix is not
read from BCB, but from the ro.boot.slot_suffix property:
"""
// Initialize the current_slot from the read-only property. If the property
// was not set (from either the command line or the device tree), we can later
// initialize it from the bootloader_control struct.
std::string suffix_prop = android::base::GetProperty("ro.boot.slot_suffix", "");
if (suffix_prop.empty()) {
LOG(ERROR) << "Slot suffix property is not set";
return false;
}
current_slot_ = SlotSuffixToIndex(suffix_prop.c_str());
"""
See:
https://cs.android.com/android/platform/superproject/main/+/main:hardware/interfaces/boot/1.1/default/boot_control/libboot_control.cpp;l=185;drc=86b8f575059a1799c760ca7012f540a528d68a9d;bpv=1;bpt=1
This has been the case since 2019.
If we look at earlier implementations of libboot_control (which was in
bootable/recovery)
https://android-review.googlesource.com/c/platform/bootable/recovery/+/1191517
So implementations before 2019 that do not have this patch:
https://android-review.googlesource.com/c/platform/bootable/recovery/+/1111899
Will get the slot suffix from the BCB (not from the commandline)
For these older implementations, we will go through the following:
BootControl::Init()
LoadBootloaderControl(device.c_str(), &boot_ctrl)
android::base::ReadFully(fd.get(), buffer, sizeof(bootloader_control)
And struct bootloader_control has:
struct bootloader_control {
// NUL terminated active slot suffix.
char slot_suffix[4];
And if we look at how the BCB is initialized from userspace in:
https://cs.android.com/android/platform/superproject/main/+/main:hardware/interfaces/boot/1.1/default/boot_control/libboot_control.cpp;l=120;drc=86b8f575059a1799c760ca7012f540a528d68a9d
We can see that we copy _a, not a (for example, if slot == 0).
So I think this is fine.
If needed, I can dig more for behaviour on older devices (<2019), let me know!
>
>> ab_select_slot() still returns an integer which needs to be converted
>> via the BOOT_SLOT_NAME() macro.
>>
>> >
>> > $ grep -sIrHn 'slot_suffix _' include/configs/
>> > include/configs/ti_omap5_common.h:107: "setenv slot_suffix _${slot_name};"
>> > include/configs/meson64_android.h:65: "setenv slot_suffix
>> > _${current_slot}; " \
>>
>> Same goes for these 2 examples, we use:
>> The "bcb ab_select current_slot" command to store the slot into the
>> "current_slot" environment variable.
>> Looking at cmd/bcb.c we can see:
>>
>> """
>> ret = ab_select_slot(dev_desc, &part_info, dec_tries);
>> if (ret < 0) {
>> printf("Android boot failed, error %d.\n", ret);
>> return CMD_RET_FAILURE;
>> }
>>
>> /* Android standard slot names are 'a', 'b', ... */
>> slot[0] = BOOT_SLOT_NAME(ret);
>> slot[1] = '\0';
>> env_set(argv[1], slot);
>> printf("ANDROID: Booting slot: %s\n", slot);
>> """
>>
>> So I think this is fine.
>>
>
> [snip]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 6/6] common: android_ab: fix slot suffix for abc block
2024-11-06 10:02 ` Mattijs Korpershoek
@ 2024-11-07 3:17 ` Sam Protsenko
2024-11-07 8:53 ` Mattijs Korpershoek
0 siblings, 1 reply; 21+ messages in thread
From: Sam Protsenko @ 2024-11-07 3:17 UTC (permalink / raw)
To: Mattijs Korpershoek
Cc: Dmitry Rokosov, Igor Opaniuk, Tom Rini, Andrew F. Davis,
Neil Armstrong, Simon Glass, Mario Six, u-boot, u-boot-amlogic,
rockosov, kernel, Guillaume La Roque
On Wed, Nov 6, 2024 at 4:02 AM Mattijs Korpershoek
<mkorpershoek@baylibre.com> wrote:
>
> Hi Sam,
>
> On mar., nov. 05, 2024 at 18:58, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> > On Tue, Nov 5, 2024 at 9:06 AM Mattijs Korpershoek
> > <mkorpershoek@baylibre.com> wrote:
> >>
> >> Hi Sam,
> >>
> >
> > Hey Mattijs,
> >
> > [snip]
> >
> >> >> @@ -328,7 +328,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
> >> >> * or the device tree.
> >> >> */
> >> >> memset(slot_suffix, 0, sizeof(slot_suffix));
> >> >> - slot_suffix[0] = BOOT_SLOT_NAME(slot);
> >> >> + slot_suffix[0] = '_';
> >> >> + slot_suffix[1] = BOOT_SLOT_NAME(slot);
> >> >
> >> > AFAIU, this changes the behavior of two above functions, and
> >> > consequently of "bcb ab_select" command? If so, just to double check:
> >> > were all users of those reworked correspondingly? I can see next
> >> > occurrences (there may be more):
> >> >
> >> > $ grep -sIrHn '"_' boot/bootmeth_android.c
> >>
> >> I thought the same when first reviewing the patch.
> >> Looking a bit closer...
> >>
> >> >
> >> > boot/bootmeth_android.c:74: sprintf(partname, BOOT_PART_NAME "_%s",
> >> > priv->slot);
> >> > boot/bootmeth_android.c:111: sprintf(partname,
> >> > VENDOR_BOOT_PART_NAME "_%s", priv->slot);
> >> > boot/bootmeth_android.c:156: sprintf(slot_suffix, "_%s", priv->slot);
> >> > boot/bootmeth_android.c:397: sprintf(slot_suffix, "_%s", priv->slot);
> >>
> >> ... We can see that ab_select_slot() returns an integer
> >> That integer is used later on to initialize priv->slot:
> >>
> >> """
> >> priv->slot[0] = BOOT_SLOT_NAME(ret);
> >> priv->slot[1] = '\0';
> >> """
> >>
> >> The change from Dmitry only changes what we **write** to the BCB (into
> >> the misc partition), not what is returned by ab_select_slot().
> >>
> >
> > Sure. Just wanted to double check that the behavior is not changed in
> > any related parts, as the commit message doesn't mention that. Btw,
> > BCB is an interface between the bootloader and AOSP, so if this patch
> > changes what's being written into BCB, does it affect AOSP part of it
> > somehow? Especially for already existing devices with particular BCB
> > data, in case U-Boot gets updated there.
>
> Those are valid concerns.
>
> Per my understanding, on recent Android versions the slot suffix is not
> read from BCB, but from the ro.boot.slot_suffix property:
>
That probably still leaves the possible combination of some devices
running new U-Boot versions (with this patch applied) together with
older Android versions. E.g. in case when U-Boot is updated but
Android isn't, may be especially relevant for some dev boards out
there.
> """
> // Initialize the current_slot from the read-only property. If the property
> // was not set (from either the command line or the device tree), we can later
> // initialize it from the bootloader_control struct.
So even in recent Android versions it's being initialized from BCB in
case the property is not set.
> std::string suffix_prop = android::base::GetProperty("ro.boot.slot_suffix", "");
> if (suffix_prop.empty()) {
> LOG(ERROR) << "Slot suffix property is not set";
> return false;
> }
> current_slot_ = SlotSuffixToIndex(suffix_prop.c_str());
> """
>
> See:
> https://cs.android.com/android/platform/superproject/main/+/main:hardware/interfaces/boot/1.1/default/boot_control/libboot_control.cpp;l=185;drc=86b8f575059a1799c760ca7012f540a528d68a9d;bpv=1;bpt=1
>
> This has been the case since 2019.
>
> If we look at earlier implementations of libboot_control (which was in
> bootable/recovery)
> https://android-review.googlesource.com/c/platform/bootable/recovery/+/1191517
>
> So implementations before 2019 that do not have this patch:
> https://android-review.googlesource.com/c/platform/bootable/recovery/+/1111899
>
> Will get the slot suffix from the BCB (not from the commandline)
>
> For these older implementations, we will go through the following:
> BootControl::Init()
> LoadBootloaderControl(device.c_str(), &boot_ctrl)
> android::base::ReadFully(fd.get(), buffer, sizeof(bootloader_control)
>
> And struct bootloader_control has:
>
> struct bootloader_control {
> // NUL terminated active slot suffix.
> char slot_suffix[4];
>
> And if we look at how the BCB is initialized from userspace in:
> https://cs.android.com/android/platform/superproject/main/+/main:hardware/interfaces/boot/1.1/default/boot_control/libboot_control.cpp;l=120;drc=86b8f575059a1799c760ca7012f540a528d68a9d
>
> We can see that we copy _a, not a (for example, if slot == 0).
>
> So I think this is fine.
>
> If needed, I can dig more for behaviour on older devices (<2019), let me know!
>
My point is, if it's possible to introduce this change but keep the
same old behavior (across both U-Boot and AOSP), it's usually better
to do that that way. If I'm not missing anything and that's a valid
concern, maybe a separate patch can be developed on top of the merged
patch series handling that. Anyways, I don't have enough time to work
on this right now, so just pointing out what I noticed, if it's useful
for anybody. I'll let the maintainers decide :)
Thanks!
> >
> >> ab_select_slot() still returns an integer which needs to be converted
> >> via the BOOT_SLOT_NAME() macro.
> >>
> >> >
> >> > $ grep -sIrHn 'slot_suffix _' include/configs/
> >> > include/configs/ti_omap5_common.h:107: "setenv slot_suffix _${slot_name};"
> >> > include/configs/meson64_android.h:65: "setenv slot_suffix
> >> > _${current_slot}; " \
> >>
> >> Same goes for these 2 examples, we use:
> >> The "bcb ab_select current_slot" command to store the slot into the
> >> "current_slot" environment variable.
> >> Looking at cmd/bcb.c we can see:
> >>
> >> """
> >> ret = ab_select_slot(dev_desc, &part_info, dec_tries);
> >> if (ret < 0) {
> >> printf("Android boot failed, error %d.\n", ret);
> >> return CMD_RET_FAILURE;
> >> }
> >>
> >> /* Android standard slot names are 'a', 'b', ... */
> >> slot[0] = BOOT_SLOT_NAME(ret);
> >> slot[1] = '\0';
> >> env_set(argv[1], slot);
> >> printf("ANDROID: Booting slot: %s\n", slot);
> >> """
> >>
> >> So I think this is fine.
> >>
> >
> > [snip]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 6/6] common: android_ab: fix slot suffix for abc block
2024-11-07 3:17 ` Sam Protsenko
@ 2024-11-07 8:53 ` Mattijs Korpershoek
0 siblings, 0 replies; 21+ messages in thread
From: Mattijs Korpershoek @ 2024-11-07 8:53 UTC (permalink / raw)
To: Sam Protsenko
Cc: Dmitry Rokosov, Igor Opaniuk, Tom Rini, Andrew F. Davis,
Neil Armstrong, Simon Glass, Mario Six, u-boot, u-boot-amlogic,
rockosov, kernel, Guillaume La Roque
Hi Sam,
On mer., nov. 06, 2024 at 21:17, Sam Protsenko <semen.protsenko@linaro.org> wrote:
> On Wed, Nov 6, 2024 at 4:02 AM Mattijs Korpershoek
> <mkorpershoek@baylibre.com> wrote:
>>
>> Hi Sam,
>>
>> On mar., nov. 05, 2024 at 18:58, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>>
>> > On Tue, Nov 5, 2024 at 9:06 AM Mattijs Korpershoek
>> > <mkorpershoek@baylibre.com> wrote:
>> >>
>> >> Hi Sam,
>> >>
>> >
>> > Hey Mattijs,
>> >
>> > [snip]
>> >
>> >> >> @@ -328,7 +328,8 @@ int ab_select_slot(struct blk_desc *dev_desc, struct disk_partition *part_info,
>> >> >> * or the device tree.
>> >> >> */
>> >> >> memset(slot_suffix, 0, sizeof(slot_suffix));
>> >> >> - slot_suffix[0] = BOOT_SLOT_NAME(slot);
>> >> >> + slot_suffix[0] = '_';
>> >> >> + slot_suffix[1] = BOOT_SLOT_NAME(slot);
>> >> >
>> >> > AFAIU, this changes the behavior of two above functions, and
>> >> > consequently of "bcb ab_select" command? If so, just to double check:
>> >> > were all users of those reworked correspondingly? I can see next
>> >> > occurrences (there may be more):
>> >> >
>> >> > $ grep -sIrHn '"_' boot/bootmeth_android.c
>> >>
>> >> I thought the same when first reviewing the patch.
>> >> Looking a bit closer...
>> >>
>> >> >
>> >> > boot/bootmeth_android.c:74: sprintf(partname, BOOT_PART_NAME "_%s",
>> >> > priv->slot);
>> >> > boot/bootmeth_android.c:111: sprintf(partname,
>> >> > VENDOR_BOOT_PART_NAME "_%s", priv->slot);
>> >> > boot/bootmeth_android.c:156: sprintf(slot_suffix, "_%s", priv->slot);
>> >> > boot/bootmeth_android.c:397: sprintf(slot_suffix, "_%s", priv->slot);
>> >>
>> >> ... We can see that ab_select_slot() returns an integer
>> >> That integer is used later on to initialize priv->slot:
>> >>
>> >> """
>> >> priv->slot[0] = BOOT_SLOT_NAME(ret);
>> >> priv->slot[1] = '\0';
>> >> """
>> >>
>> >> The change from Dmitry only changes what we **write** to the BCB (into
>> >> the misc partition), not what is returned by ab_select_slot().
>> >>
>> >
>> > Sure. Just wanted to double check that the behavior is not changed in
>> > any related parts, as the commit message doesn't mention that. Btw,
>> > BCB is an interface between the bootloader and AOSP, so if this patch
>> > changes what's being written into BCB, does it affect AOSP part of it
>> > somehow? Especially for already existing devices with particular BCB
>> > data, in case U-Boot gets updated there.
>>
>> Those are valid concerns.
>>
>> Per my understanding, on recent Android versions the slot suffix is not
>> read from BCB, but from the ro.boot.slot_suffix property:
>>
>
> That probably still leaves the possible combination of some devices
> running new U-Boot versions (with this patch applied) together with
> older Android versions. E.g. in case when U-Boot is updated but
> Android isn't, may be especially relevant for some dev boards out
> there.
Yes, you are right. Boards with Android older than 2019 and U-Boot v2025.01+
My experience is that most of the time, it's the bootloader that stays
out of date and the Android that gets updated. But that might not be the
general case.
>
>> """
>> // Initialize the current_slot from the read-only property. If the property
>> // was not set (from either the command line or the device tree), we can later
>> // initialize it from the bootloader_control struct.
>
> So even in recent Android versions it's being initialized from BCB in
> case the property is not set.
Nope, the comment is wrong. If look at the lines below you can see that
the function exits early when the property is not set.
So in recent Android versions if the propery is not set, we just fail to initialize.
>
>> std::string suffix_prop = android::base::GetProperty("ro.boot.slot_suffix", "");
>> if (suffix_prop.empty()) {
>> LOG(ERROR) << "Slot suffix property is not set";
>> return false;
>> }
>> current_slot_ = SlotSuffixToIndex(suffix_prop.c_str());
>> """
>>
>> See:
>> https://cs.android.com/android/platform/superproject/main/+/main:hardware/interfaces/boot/1.1/default/boot_control/libboot_control.cpp;l=185;drc=86b8f575059a1799c760ca7012f540a528d68a9d;bpv=1;bpt=1
>>
>> This has been the case since 2019.
>>
>> If we look at earlier implementations of libboot_control (which was in
>> bootable/recovery)
>> https://android-review.googlesource.com/c/platform/bootable/recovery/+/1191517
>>
>> So implementations before 2019 that do not have this patch:
>> https://android-review.googlesource.com/c/platform/bootable/recovery/+/1111899
>>
>> Will get the slot suffix from the BCB (not from the commandline)
>>
>> For these older implementations, we will go through the following:
>> BootControl::Init()
>> LoadBootloaderControl(device.c_str(), &boot_ctrl)
>> android::base::ReadFully(fd.get(), buffer, sizeof(bootloader_control)
>>
>> And struct bootloader_control has:
>>
>> struct bootloader_control {
>> // NUL terminated active slot suffix.
>> char slot_suffix[4];
>>
>> And if we look at how the BCB is initialized from userspace in:
>> https://cs.android.com/android/platform/superproject/main/+/main:hardware/interfaces/boot/1.1/default/boot_control/libboot_control.cpp;l=120;drc=86b8f575059a1799c760ca7012f540a528d68a9d
>>
>> We can see that we copy _a, not a (for example, if slot == 0).
>>
>> So I think this is fine.
>>
>> If needed, I can dig more for behaviour on older devices (<2019), let me know!
>>
>
> My point is, if it's possible to introduce this change but keep the
> same old behavior (across both U-Boot and AOSP), it's usually better
> to do that that way. If I'm not missing anything and that's a valid
> concern, maybe a separate patch can be developed on top of the merged
> patch series handling that. Anyways, I don't have enough time to work
> on this right now, so just pointing out what I noticed, if it's useful
> for anybody. I'll let the maintainers decide :)
Yes, thanks a lot for noticing and taking the time to report your valid
concerns :)
>
> Thanks!
>
>> >
>> >> ab_select_slot() still returns an integer which needs to be converted
>> >> via the BOOT_SLOT_NAME() macro.
>> >>
>> >> >
>> >> > $ grep -sIrHn 'slot_suffix _' include/configs/
>> >> > include/configs/ti_omap5_common.h:107: "setenv slot_suffix _${slot_name};"
>> >> > include/configs/meson64_android.h:65: "setenv slot_suffix
>> >> > _${current_slot}; " \
>> >>
>> >> Same goes for these 2 examples, we use:
>> >> The "bcb ab_select current_slot" command to store the slot into the
>> >> "current_slot" environment variable.
>> >> Looking at cmd/bcb.c we can see:
>> >>
>> >> """
>> >> ret = ab_select_slot(dev_desc, &part_info, dec_tries);
>> >> if (ret < 0) {
>> >> printf("Android boot failed, error %d.\n", ret);
>> >> return CMD_RET_FAILURE;
>> >> }
>> >>
>> >> /* Android standard slot names are 'a', 'b', ... */
>> >> slot[0] = BOOT_SLOT_NAME(ret);
>> >> slot[1] = '\0';
>> >> env_set(argv[1], slot);
>> >> printf("ANDROID: Booting slot: %s\n", slot);
>> >> """
>> >>
>> >> So I think this is fine.
>> >>
>> >
>> > [snip]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 0/6] android_ab: introduce bcb ab_dump command and provide several bcb fixes
2024-10-17 14:12 [PATCH v5 0/6] android_ab: introduce bcb ab_dump command and provide several bcb fixes Dmitry Rokosov
` (5 preceding siblings ...)
2024-10-17 14:12 ` [PATCH v5 6/6] common: android_ab: fix slot suffix for abc block Dmitry Rokosov
@ 2024-10-18 12:41 ` Dmitry Rokosov
2024-10-22 8:32 ` Mattijs Korpershoek
2024-10-24 7:47 ` Mattijs Korpershoek
8 siblings, 0 replies; 21+ messages in thread
From: Dmitry Rokosov @ 2024-10-18 12:41 UTC (permalink / raw)
To: Igor Opaniuk, Mattijs Korpershoek, Sam Protsenko, Tom Rini,
Andrew F. Davis, Neil Armstrong, Simon Glass, Mario Six
Cc: u-boot, u-boot-amlogic, rockosov, kernel, Guillaume La Roque
On Thu, Oct 17, 2024 at 05:12:05PM +0300, Dmitry Rokosov wrote:
> The patch series include changes:
> - move ab_select_slot() documentation to @ notation
> - redesign 'bcb' command to U_BOOT_LONGHELP approach
> - move ab_select command to bcb subcommands
> - introduce the ab_dump command to print the content of the BCB
> block; it's useful for debugging A/B logic on supported boards
> - fix the slot suffix format in the ABC block to align with official
> Android BCB specifications
> - add a test for the ab_dump command to verify the accuracy of each
> field within the ABC data displayed, it's also useful for testing
> slot_suffix problem code paths
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
> Changes in v5:
> - rework direct #ifdefs to IS_ENABLED() macro
> - redesign 'bcb' command to U_BOOT_LONGHELP approach
> - check argc directly in the ab_select and ab_dump subcommands handlers
> - Link to v4: https://lore.kernel.org/r/20241015-android_ab_master-v4-0-a91cca9513c4@salutedevices.com
>
> Changes in v4:
> - add #ifdefs for CONFIG_ANDROID_AB in cmd/bcb.c to allow the usage of
> the bcb command without A/B enabled
> - run the savedefconfig command for all defconfigs that include the
> CMD_BCB configuration
> - resolve merge conflicts with latest master
> - provide additional trailers from the previous version (excluding
> changed patches)
> - Link to v3: https://lore.kernel.org/r/20241008-android_ab_master-v3-0-f292c45a33e4@salutedevices.com
>
> Changes in v3:
> - return "Legend" block for bcb command
> - additionally, verify the CONFIG_ANDROID_AB configuration alongside
> CONFIG_CMD_BCB to ensure that the A/B scheme is used for the
> designated board.
> - Link to v2: https://lore.kernel.org/all/20240911214945.15873-1-ddrokosov@salutedevices.com/
>
> Changes in v2:
> - move ab_select_slot() documentation to @ notation
> - move ab_select command to bcb subcommands per Simon and Mattijs
> suggestions
> - redesign ab_dump as bcb subcommand
> - use spaces instead of tabs in the ab_dump command output
> - print hex values in the lowercase
> - add RvB tags
> - Link to v1: https://lore.kernel.org/all/20240725194716.32232-1-ddrokosov@salutedevices.com/
>
> ---
> Dmitry Rokosov (6):
> include/android_ab: move ab_select_slot() documentation to @ notation
> cmd: bcb: rework the command to U_BOOT_LONGHELP approach
> treewide: bcb: move ab_select command to bcb subcommands
> cmd: bcb: change strcmp() usage style in the do_bcb_ab_select()
> cmd: bcb: introduce 'ab_dump' command to print BCB block content
> common: android_ab: fix slot suffix for abc block
>
> MAINTAINERS | 1 -
> boot/android_ab.c | 116 +++++++++++++---
> cmd/Kconfig | 14 --
> cmd/Makefile | 1 -
> cmd/ab_select.c | 66 ---------
> cmd/bcb.c | 221 +++++++++++++++++-------------
> configs/am57xx_evm_defconfig | 1 -
> configs/am57xx_hs_evm_defconfig | 1 -
> configs/am57xx_hs_evm_usb_defconfig | 1 -
> configs/khadas-vim3_android_ab_defconfig | 1 -
> configs/khadas-vim3l_android_ab_defconfig | 1 -
> configs/sandbox64_defconfig | 2 +
> configs/sandbox_defconfig | 1 -
> doc/android/ab.rst | 12 +-
> include/android_ab.h | 17 ++-
> include/configs/khadas-vim3_android.h | 2 +-
> include/configs/khadas-vim3l_android.h | 2 +-
> include/configs/meson64_android.h | 4 +-
> include/configs/ti_omap5_common.h | 4 +-
> test/py/tests/test_android/test_ab.py | 31 ++++-
> 20 files changed, 275 insertions(+), 224 deletions(-)
> ---
> base-commit: 98a36deb9ab7aaea70b0b0db47718100e08cf3e8
> change-id: 20241008-android_ab_master-d86d71c839ae
>
> Best regards,
> --
> Dmitry Rokosov <ddrokosov@salutedevices.com>
>
CI/CD tests have been successfully completed.
Please see more details:
https://github.com/u-boot/u-boot/pull/625/checks
--
Thank you,
Dmitry
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 0/6] android_ab: introduce bcb ab_dump command and provide several bcb fixes
2024-10-17 14:12 [PATCH v5 0/6] android_ab: introduce bcb ab_dump command and provide several bcb fixes Dmitry Rokosov
` (6 preceding siblings ...)
2024-10-18 12:41 ` [PATCH v5 0/6] android_ab: introduce bcb ab_dump command and provide several bcb fixes Dmitry Rokosov
@ 2024-10-22 8:32 ` Mattijs Korpershoek
2024-10-24 7:47 ` Mattijs Korpershoek
8 siblings, 0 replies; 21+ messages in thread
From: Mattijs Korpershoek @ 2024-10-22 8:32 UTC (permalink / raw)
To: Dmitry Rokosov, Igor Opaniuk, Sam Protsenko, Tom Rini,
Andrew F. Davis, Neil Armstrong, Simon Glass, Mario Six
Cc: u-boot, u-boot-amlogic, rockosov, kernel, Dmitry Rokosov,
Guillaume La Roque
Hi Dmitry,
Thank you for this series.
On jeu., oct. 17, 2024 at 17:12, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> The patch series include changes:
> - move ab_select_slot() documentation to @ notation
> - redesign 'bcb' command to U_BOOT_LONGHELP approach
> - move ab_select command to bcb subcommands
> - introduce the ab_dump command to print the content of the BCB
> block; it's useful for debugging A/B logic on supported boards
> - fix the slot suffix format in the ABC block to align with official
> Android BCB specifications
> - add a test for the ab_dump command to verify the accuracy of each
> field within the ABC data displayed, it's also useful for testing
> slot_suffix problem code paths
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
Boot tested AOSP using on Khadas VIM3 using khadas_vim3_android_defconfig
Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> # vim3_android
Boot tested Android 14 on Beagle Play using:
am62x_beagleplay_a53_defconfig and am62x_a53_android.config
Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> # beagleplay
> ---
> Changes in v5:
> - rework direct #ifdefs to IS_ENABLED() macro
> - redesign 'bcb' command to U_BOOT_LONGHELP approach
> - check argc directly in the ab_select and ab_dump subcommands handlers
> - Link to v4: https://lore.kernel.org/r/20241015-android_ab_master-v4-0-a91cca9513c4@salutedevices.com
>
> Changes in v4:
> - add #ifdefs for CONFIG_ANDROID_AB in cmd/bcb.c to allow the usage of
> the bcb command without A/B enabled
> - run the savedefconfig command for all defconfigs that include the
> CMD_BCB configuration
> - resolve merge conflicts with latest master
> - provide additional trailers from the previous version (excluding
> changed patches)
> - Link to v3: https://lore.kernel.org/r/20241008-android_ab_master-v3-0-f292c45a33e4@salutedevices.com
>
> Changes in v3:
> - return "Legend" block for bcb command
> - additionally, verify the CONFIG_ANDROID_AB configuration alongside
> CONFIG_CMD_BCB to ensure that the A/B scheme is used for the
> designated board.
> - Link to v2: https://lore.kernel.org/all/20240911214945.15873-1-ddrokosov@salutedevices.com/
>
> Changes in v2:
> - move ab_select_slot() documentation to @ notation
> - move ab_select command to bcb subcommands per Simon and Mattijs
> suggestions
> - redesign ab_dump as bcb subcommand
> - use spaces instead of tabs in the ab_dump command output
> - print hex values in the lowercase
> - add RvB tags
> - Link to v1: https://lore.kernel.org/all/20240725194716.32232-1-ddrokosov@salutedevices.com/
>
> ---
> Dmitry Rokosov (6):
> include/android_ab: move ab_select_slot() documentation to @ notation
> cmd: bcb: rework the command to U_BOOT_LONGHELP approach
> treewide: bcb: move ab_select command to bcb subcommands
> cmd: bcb: change strcmp() usage style in the do_bcb_ab_select()
> cmd: bcb: introduce 'ab_dump' command to print BCB block content
> common: android_ab: fix slot suffix for abc block
>
> MAINTAINERS | 1 -
> boot/android_ab.c | 116 +++++++++++++---
> cmd/Kconfig | 14 --
> cmd/Makefile | 1 -
> cmd/ab_select.c | 66 ---------
> cmd/bcb.c | 221 +++++++++++++++++-------------
> configs/am57xx_evm_defconfig | 1 -
> configs/am57xx_hs_evm_defconfig | 1 -
> configs/am57xx_hs_evm_usb_defconfig | 1 -
> configs/khadas-vim3_android_ab_defconfig | 1 -
> configs/khadas-vim3l_android_ab_defconfig | 1 -
> configs/sandbox64_defconfig | 2 +
> configs/sandbox_defconfig | 1 -
> doc/android/ab.rst | 12 +-
> include/android_ab.h | 17 ++-
> include/configs/khadas-vim3_android.h | 2 +-
> include/configs/khadas-vim3l_android.h | 2 +-
> include/configs/meson64_android.h | 4 +-
> include/configs/ti_omap5_common.h | 4 +-
> test/py/tests/test_android/test_ab.py | 31 ++++-
> 20 files changed, 275 insertions(+), 224 deletions(-)
> ---
> base-commit: 98a36deb9ab7aaea70b0b0db47718100e08cf3e8
> change-id: 20241008-android_ab_master-d86d71c839ae
>
> Best regards,
> --
> Dmitry Rokosov <ddrokosov@salutedevices.com>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v5 0/6] android_ab: introduce bcb ab_dump command and provide several bcb fixes
2024-10-17 14:12 [PATCH v5 0/6] android_ab: introduce bcb ab_dump command and provide several bcb fixes Dmitry Rokosov
` (7 preceding siblings ...)
2024-10-22 8:32 ` Mattijs Korpershoek
@ 2024-10-24 7:47 ` Mattijs Korpershoek
8 siblings, 0 replies; 21+ messages in thread
From: Mattijs Korpershoek @ 2024-10-24 7:47 UTC (permalink / raw)
To: Igor Opaniuk, Sam Protsenko, Tom Rini, Andrew F. Davis,
Neil Armstrong, Simon Glass, Mario Six, Dmitry Rokosov
Cc: u-boot, u-boot-amlogic, rockosov, kernel, Guillaume La Roque
Hi,
On Thu, 17 Oct 2024 17:12:05 +0300, Dmitry Rokosov wrote:
> The patch series include changes:
> - move ab_select_slot() documentation to @ notation
> - redesign 'bcb' command to U_BOOT_LONGHELP approach
> - move ab_select command to bcb subcommands
> - introduce the ab_dump command to print the content of the BCB
> block; it's useful for debugging A/B logic on supported boards
> - fix the slot suffix format in the ABC block to align with official
> Android BCB specifications
> - add a test for the ab_dump command to verify the accuracy of each
> field within the ABC data displayed, it's also useful for testing
> slot_suffix problem code paths
>
> [...]
Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)
[1/6] include/android_ab: move ab_select_slot() documentation to @ notation
https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/01874ac7c06b8c657e7c3fda5ac555f821ef5b36
[2/6] cmd: bcb: rework the command to U_BOOT_LONGHELP approach
https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/a8ca7d46ea744381185c0bf70cbdf4db7dc734b7
[3/6] treewide: bcb: move ab_select command to bcb subcommands
https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/b523b4d2c32f07ca0793bb0f926b02ecb0556cc6
[4/6] cmd: bcb: change strcmp() usage style in the do_bcb_ab_select()
https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/b1bc9a2fc93da8ed25dfc4078e76220ac26eb6b0
[5/6] cmd: bcb: introduce 'ab_dump' command to print BCB block content
https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/a995084beb6682748002878ccb4b8c4de9fc0136
[6/6] common: android_ab: fix slot suffix for abc block
https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/55c876c6f9b787488c7a9f9c678398c48d13db6f
--
Mattijs
^ permalink raw reply [flat|nested] 21+ messages in thread