[PATCH v2 0/7] Refactor generic fastboot_set_reboot_flag implementation
Current generic implementation of fastboot_set_reboot_flag is somewhat messy and requires some additional configuration option to be enabled besides CMD_BCB, so it reverts that implementtion in order to bring a new cleaner one.
New function called bcb_set_reboot_reason should be exposed by BCB commands, so that all of the details could be put there instead of dealing with it in fastboot implementation directly. After that, fastboot_set_reboot_flag could simply pass correct reboot reason string to the BCB implementation. If CMD_BCB is disabled then the whole operation would return error code, which is no different behaviour than the current implementation.
Changes since v1[1] * Fixed compilation issue for sandbox platform caused by undefined FASTBOOT_FLASH_MMC_DEV Kconfig option. * Fixed compilation issue in bcb.h caused by missing common.h header for sandbox platform
[1]: https://patchwork.ozlabs.org/project/uboot/cover/cover.1603442753.git.roman....
Eugeniu Rosca (5): cmd: bcb: Extract '__bcb_load' from 'do_bcb_load' for internal needs cmd: bcb: Extract '__bcb_set' from 'do_bcb_set' for internal needs cmd: bcb: Extract '__bcb_store' from 'do_bcb_store' for internal needs cmd: bcb: Expose 'bcb_write_reboot_reason' to external callers cmd: bcb: Add support for processing const string literals in bcb_set()
Roman Kovalivskyi (2): Revert "fastboot: Add default fastboot_set_reboot_flag implementation" fastboot: Implement generic fastboot_set_reboot_flag
cmd/bcb.c | 88 +++++++++++++++++++++++++++------- drivers/fastboot/Kconfig | 12 ----- drivers/fastboot/Makefile | 1 - drivers/fastboot/fb_bcb_impl.c | 43 ----------------- drivers/fastboot/fb_common.c | 16 ++++++- include/bcb.h | 22 +++++++++ include/fastboot.h | 9 ---- 7 files changed, 107 insertions(+), 84 deletions(-) delete mode 100644 drivers/fastboot/fb_bcb_impl.c create mode 100644 include/bcb.h
From: Eugeniu Rosca erosca@de.adit-jv.com
Enriching the functionality of U-Boot 'bcb' may assume using the existing sub-commands as building blocks for the next ones.
A clean way to achive the above is to expose a number of static routines, each mapped to an existing user command (e.g. load/set/store), with a user/caller-friendly prototype (i.e. do not force the caller to wrap an integer into a string).
This first patch makes '__bcb_load' available for internal needs.
No functional change, except for a tiny update in error handling.
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com --- cmd/bcb.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/cmd/bcb.c b/cmd/bcb.c index e03218066bf2..2ed8b801a3e2 100644 --- a/cmd/bcb.c +++ b/cmd/bcb.c @@ -110,8 +110,7 @@ static int bcb_field_get(char *name, char **fieldp, int *sizep) return 0; }
-static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc, - char *const argv[]) +static int __bcb_load(int devnum, const char *partp) { struct blk_desc *desc; struct disk_partition info; @@ -119,17 +118,19 @@ static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc, char *endp; int part, ret;
- ret = blk_get_device_by_str("mmc", argv[1], &desc); - if (ret < 0) + desc = blk_get_devnum_by_type(IF_TYPE_MMC, devnum); + if (!desc) { + ret = -ENODEV; goto err_read_fail; + }
- part = simple_strtoul(argv[2], &endp, 0); + part = simple_strtoul(partp, &endp, 0); if (*endp == '\0') { ret = part_get_info(desc, part, &info); if (ret) goto err_read_fail; } else { - part = part_get_info_by_name(desc, argv[2], &info); + part = part_get_info_by_name(desc, partp, &info); if (part < 0) { ret = part; goto err_read_fail; @@ -151,10 +152,10 @@ static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc,
return CMD_RET_SUCCESS; err_read_fail: - printf("Error: mmc %s:%s read failed (%d)\n", argv[1], argv[2], ret); + printf("Error: mmc %d:%s read failed (%d)\n", devnum, partp, ret); goto err; err_too_small: - printf("Error: mmc %s:%s too small!", argv[1], argv[2]); + printf("Error: mmc %d:%s too small!", devnum, partp); goto err; err: bcb_dev = -1; @@ -163,6 +164,20 @@ err: return CMD_RET_FAILURE; }
+static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc, + char * const argv[]) +{ + char *endp; + int devnum = simple_strtoul(argv[1], &endp, 0); + + if (*endp != '\0') { + printf("Error: Device id '%s' not a number\n", argv[1]); + return CMD_RET_FAILURE; + } + + return __bcb_load(devnum, argv[2]); +} + static int do_bcb_set(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) {
From: Eugeniu Rosca erosca@de.adit-jv.com
Enriching the functionality of U-Boot 'bcb' may assume using the existing sub-commands as building blocks for the next ones.
A clean way to achive the above is to expose a number of static routines, each mapped to an existing user command (e.g. load/set/store), with a user/caller-friendly prototype (i.e. do not force the caller to wrap an integer into a string).
This second patch makes '__bcb_set' available for internal needs.
No functional change intended.
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com --- cmd/bcb.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/cmd/bcb.c b/cmd/bcb.c index 2ed8b801a3e2..113f04ffe6b2 100644 --- a/cmd/bcb.c +++ b/cmd/bcb.c @@ -178,22 +178,21 @@ static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc, return __bcb_load(devnum, argv[2]); }
-static int do_bcb_set(struct cmd_tbl *cmdtp, int flag, int argc, - char *const argv[]) +static int __bcb_set(char *fieldp, char *valp) { int size, len; char *field, *str, *found;
- if (bcb_field_get(argv[1], &field, &size)) + if (bcb_field_get(fieldp, &field, &size)) return CMD_RET_FAILURE;
- len = strlen(argv[2]); + len = strlen(valp); if (len >= size) { printf("Error: sizeof('%s') = %d >= %d = sizeof(bcb.%s)\n", - argv[2], len, size, argv[1]); + valp, len, size, fieldp); return CMD_RET_FAILURE; } - str = argv[2]; + str = valp;
field[0] = '\0'; while ((found = strsep(&str, ":"))) { @@ -205,6 +204,12 @@ static int do_bcb_set(struct cmd_tbl *cmdtp, int flag, int argc, return CMD_RET_SUCCESS; }
+static int do_bcb_set(struct cmd_tbl *cmdtp, int flag, int argc, + char * const argv[]) +{ + return __bcb_set(argv[1], argv[2]); +} + static int do_bcb_clear(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) {
From: Eugeniu Rosca erosca@de.adit-jv.com
Enriching the functionality of U-Boot 'bcb' may assume using the existing sub-commands as building blocks for the next ones.
A clean way to achive the above is to expose a number of static routines, each mapped to an existing user command (e.g. load/set/store), with a user/caller-friendly prototype (i.e. do not force the caller to wrap an integer into a string).
This third patch makes '__bcb_store' available for internal needs.
No functional change intended.
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com --- cmd/bcb.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/cmd/bcb.c b/cmd/bcb.c index 113f04ffe6b2..b9cd20ea3d56 100644 --- a/cmd/bcb.c +++ b/cmd/bcb.c @@ -270,8 +270,7 @@ static int do_bcb_dump(struct cmd_tbl *cmdtp, int flag, int argc, return CMD_RET_SUCCESS; }
-static int do_bcb_store(struct cmd_tbl *cmdtp, int flag, int argc, - char *const argv[]) +static int __bcb_store(void) { struct blk_desc *desc; struct disk_partition info; @@ -302,6 +301,12 @@ err: return CMD_RET_FAILURE; }
+static int do_bcb_store(struct cmd_tbl *cmdtp, int flag, int argc, + char * const argv[]) +{ + return __bcb_store(); +} + 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, "", ""),
From: Eugeniu Rosca erosca@de.adit-jv.com
Fastboot is evolving and beginning with commit [1], the upstream implementation expects bootloaders to offer support for: - reboot-recovery - reboot-fastboot
The most natural way to achieve the above is through a set of pre-defined "reboot reason" strings, written into / read from the BCB "command" field, e.g.: - bootonce-bootloader [2] - boot-fastboot [3] - boot-recovery [4]
Expose the first 'bcb' API meant to be called by e.g. fastboot stack, to allow updating the BCB reboot reason via the BCB 'command' field.
[1] https://android.googlesource.com/platform/system/core/+/dea91b4b5354af2 ("Add fastbootd.") [2] https://android.googlesource.com/platform/bootable/recovery/+/cba7fa88d8b9 ("Add 'reboot bootloader' to bootloader_message.") [3] https://android.googlesource.com/platform/bootable/recovery/+/eee4e260f9f6 ("recovery: Add "boot-fastboot" command to BCB.") [4] https://android.googlesource.com/platform/system/core/+/5e98b633a748695f ("init: Write the reason in BCB on "reboot recovery"")
Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com --- cmd/bcb.c | 20 ++++++++++++++++++++ include/bcb.h | 22 ++++++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 include/bcb.h
diff --git a/cmd/bcb.c b/cmd/bcb.c index b9cd20ea3d56..5da3526142ad 100644 --- a/cmd/bcb.c +++ b/cmd/bcb.c @@ -6,6 +6,7 @@ */
#include <android_bootloader_message.h> +#include <bcb.h> #include <command.h> #include <common.h> #include <log.h> @@ -307,6 +308,25 @@ static int do_bcb_store(struct cmd_tbl *cmdtp, int flag, int argc, return __bcb_store(); }
+int bcb_write_reboot_reason(int devnum, char *partp, char *reasonp) +{ + int ret; + + ret = __bcb_load(devnum, partp); + if (ret != CMD_RET_SUCCESS) + return ret; + + ret = __bcb_set("command", reasonp); + if (ret != CMD_RET_SUCCESS) + return ret; + + ret = __bcb_store(); + if (ret != CMD_RET_SUCCESS) + return ret; + + return 0; +} + 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, "", ""), diff --git a/include/bcb.h b/include/bcb.h new file mode 100644 index 000000000000..05db5935e0e7 --- /dev/null +++ b/include/bcb.h @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2020 Eugeniu Rosca rosca.eugeniu@gmail.com + * + * Android Bootloader Control Block Header + */ + +#ifndef __BCB_H__ +#define __BCB_H__ + +#include <common.h> + +#if CONFIG_IS_ENABLED(CMD_BCB) +int bcb_write_reboot_reason(int devnum, char *partp, char *reasonp); +#else +static inline int bcb_write_reboot_reason(int devnum, char *partp, char *reasonp) +{ + return -EOPNOTSUPP; +} +#endif + +#endif /* __BCB_H__ */
From: Eugeniu Rosca erosca@de.adit-jv.com
On request/suggestion from Simon Glass back in May 22 2019 [1], the 'strsep' mechanism implemented in bcb_set() was set to work directly with user-provided argv strings, to avoid duplicating memory and for the sake of simpler implementation.
However, since we recently exposed bcb_write_reboot_reason() API to be called by U-Boot fastboot, the idea is to be able to pass const string literals to this new BCB API, carrying the reboot reason.
Since 'strsep' (just like its older/superseded sibling 'strtok') modifies the input string passed as parameter, BCB command in its current state would attempt to perform in-place modifications in a readonly string, which might lead to unexpected results.
Fix the above with the cost of one dynamic memory allocation ('strdup'). This will also ensure no compiler warnings when passing string literals to bcb_write_reboot_reason().
[1] http://u-boot.10912.n7.nabble.com/PATCH-v2-0-2-Add-bcb-command-to-read-modif...
Cc: Simon Glass sjg@chromium.org Signed-off-by: Eugeniu Rosca erosca@de.adit-jv.com Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com --- cmd/bcb.c | 17 ++++++++++++----- include/bcb.h | 4 ++-- 2 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/cmd/bcb.c b/cmd/bcb.c index 5da3526142ad..6b6f1e9a2f10 100644 --- a/cmd/bcb.c +++ b/cmd/bcb.c @@ -11,6 +11,7 @@ #include <common.h> #include <log.h> #include <part.h> +#include <malloc.h>
enum bcb_cmd { BCB_CMD_LOAD, @@ -179,10 +180,10 @@ static int do_bcb_load(struct cmd_tbl *cmdtp, int flag, int argc, return __bcb_load(devnum, argv[2]); }
-static int __bcb_set(char *fieldp, char *valp) +static int __bcb_set(char *fieldp, const char *valp) { int size, len; - char *field, *str, *found; + char *field, *str, *found, *tmp;
if (bcb_field_get(fieldp, &field, &size)) return CMD_RET_FAILURE; @@ -193,14 +194,20 @@ static int __bcb_set(char *fieldp, char *valp) valp, len, size, fieldp); return CMD_RET_FAILURE; } - str = valp; + str = strdup(valp); + if (!str) { + printf("Error: Out of memory while strdup\n"); + return CMD_RET_FAILURE; + }
+ tmp = str; field[0] = '\0'; - while ((found = strsep(&str, ":"))) { + while ((found = strsep(&tmp, ":"))) { if (field[0] != '\0') strcat(field, "\n"); strcat(field, found); } + free(str);
return CMD_RET_SUCCESS; } @@ -308,7 +315,7 @@ static int do_bcb_store(struct cmd_tbl *cmdtp, int flag, int argc, return __bcb_store(); }
-int bcb_write_reboot_reason(int devnum, char *partp, char *reasonp) +int bcb_write_reboot_reason(int devnum, char *partp, const char *reasonp) { int ret;
diff --git a/include/bcb.h b/include/bcb.h index 05db5935e0e7..c91edb91265a 100644 --- a/include/bcb.h +++ b/include/bcb.h @@ -11,9 +11,9 @@ #include <common.h>
#if CONFIG_IS_ENABLED(CMD_BCB) -int bcb_write_reboot_reason(int devnum, char *partp, char *reasonp); +int bcb_write_reboot_reason(int devnum, char *partp, const char *reasonp); #else -static inline int bcb_write_reboot_reason(int devnum, char *partp, char *reasonp) +static inline int bcb_write_reboot_reason(int devnum, char *partp, const char *reasonp) { return -EOPNOTSUPP; }
This reverts commit 0ebf9842e56c5b8cb7cb1f990bb452cc14af6225.
Current generic implementation of fastboot_set_reboot_flag is somewhat messy and requires some additional configuration option to be enabled besides CMD_BCB, so it reverts that implementtion in order to bring a new cleaner one.
Next commit introduces new generic implementation of fastboot_set_reboot_flag.
Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com --- drivers/fastboot/Kconfig | 12 ---------- drivers/fastboot/Makefile | 1 - drivers/fastboot/fb_bcb_impl.c | 43 ---------------------------------- include/fastboot.h | 9 ------- 4 files changed, 65 deletions(-) delete mode 100644 drivers/fastboot/fb_bcb_impl.c
diff --git a/drivers/fastboot/Kconfig b/drivers/fastboot/Kconfig index 4352ba67a713..d4436dfc9173 100644 --- a/drivers/fastboot/Kconfig +++ b/drivers/fastboot/Kconfig @@ -165,18 +165,6 @@ config FASTBOOT_CMD_OEM_FORMAT relies on the env variable partitions to contain the list of partitions as required by the gpt command.
-config FASTBOOT_USE_BCB_SET_REBOOT_FLAG - bool "Use BCB by fastboot to set boot reason" - depends on CMD_BCB && !ARCH_MESON && !ARCH_ROCKCHIP && !TARGET_KC1 && \ - !TARGET_SNIPER && !TARGET_AM57XX_EVM && !TARGET_DRA7XX_EVM - default y - help - Fastboot could implement setting of reboot reason in a generic fashion - via BCB commands. BCB commands are able to write reboot reason into - command field of boot control block. In general case it is sufficient - implementation if your platform supports BCB commands and doesn't - require any specific reboot reason handling. - endif # FASTBOOT
endmenu diff --git a/drivers/fastboot/Makefile b/drivers/fastboot/Makefile index 2b2c390fe4de..048af5aa8234 100644 --- a/drivers/fastboot/Makefile +++ b/drivers/fastboot/Makefile @@ -5,4 +5,3 @@ obj-y += fb_getvar.o obj-y += fb_command.o obj-$(CONFIG_FASTBOOT_FLASH_MMC) += fb_mmc.o obj-$(CONFIG_FASTBOOT_FLASH_NAND) += fb_nand.o -obj-$(CONFIG_FASTBOOT_USE_BCB_SET_REBOOT_FLAG) += fb_bcb_impl.o diff --git a/drivers/fastboot/fb_bcb_impl.c b/drivers/fastboot/fb_bcb_impl.c deleted file mode 100644 index 89ec3601b6f6..000000000000 --- a/drivers/fastboot/fb_bcb_impl.c +++ /dev/null @@ -1,43 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0+ -/* - * Copyright 2020 GlobalLogic. - * Roman Kovalivskyi roman.kovalivskyi@globallogic.com - */ - -#include <common.h> -#include <fastboot.h> - -/** - * fastboot_set_reboot_flag() - Set flag to indicate reboot-bootloader - * - * Set flag which indicates that we should reboot into the bootloader - * following the reboot that fastboot executes after this function. - * - * This function should be overridden in your board file with one - * which sets whatever flag your board specific Android bootloader flow - * requires in order to re-enter the bootloader. - */ -int fastboot_set_reboot_flag(enum fastboot_reboot_reason reason) -{ - char cmd[64]; - - if (reason >= FASTBOOT_REBOOT_REASONS_COUNT) - return -EINVAL; - - snprintf(cmd, sizeof(cmd), "bcb load %d misc", - CONFIG_FASTBOOT_FLASH_MMC_DEV); - - if (run_command(cmd, 0)) - return -ENODEV; - - snprintf(cmd, sizeof(cmd), "bcb set command %s", - fastboot_boot_cmds[reason]); - - if (run_command(cmd, 0)) - return -ENOEXEC; - - if (run_command("bcb store", 0)) - return -EIO; - - return 0; -} diff --git a/include/fastboot.h b/include/fastboot.h index 8e9ee80907df..b86b508e69fd 100644 --- a/include/fastboot.h +++ b/include/fastboot.h @@ -52,15 +52,6 @@ enum fastboot_reboot_reason { FASTBOOT_REBOOT_REASONS_COUNT };
-/** - * BCB boot commands - */ -static const char * const fastboot_boot_cmds[] = { - [FASTBOOT_REBOOT_REASON_BOOTLOADER] = "bootonce-bootloader", - [FASTBOOT_REBOOT_REASON_FASTBOOTD] = "boot-fastboot", - [FASTBOOT_REBOOT_REASON_RECOVERY] = "boot-recovery" -}; - /** * fastboot_response() - Writes a response of the form "$tag$reason". *
It is possible to implement fastboot_set_reboot_flag in a generic way if BCB commands are turned on for a target. Using bcb_set_reboot_reason allows to do this by simply passing string with correct reboot reason that should be handled during next boot process.
If BCB are turned off, then bcb_set_reboot_reason would simply return error, so it won't introduce any new behaviour for such targets.
Signed-off-by: Roman Kovalivskyi roman.kovalivskyi@globallogic.com --- drivers/fastboot/fb_common.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/fastboot/fb_common.c b/drivers/fastboot/fb_common.c index 736ce1cd024f..cbcc3683c471 100644 --- a/drivers/fastboot/fb_common.c +++ b/drivers/fastboot/fb_common.c @@ -10,6 +10,7 @@ * Rob Herring robh@kernel.org */
+#include <bcb.h> #include <common.h> #include <command.h> #include <env.h> @@ -90,7 +91,20 @@ void fastboot_okay(const char *reason, char *response) */ int __weak fastboot_set_reboot_flag(enum fastboot_reboot_reason reason) { - return -ENOSYS; +#if CONFIG_IS_ENABLED(FASTBOOT_FLASH_MMC_DEV) + static const char * const boot_cmds[] = { + [FASTBOOT_REBOOT_REASON_BOOTLOADER] = "bootonce-bootloader", + [FASTBOOT_REBOOT_REASON_FASTBOOTD] = "boot-fastboot", + [FASTBOOT_REBOOT_REASON_RECOVERY] = "boot-recovery" + }; + + if (reason >= FASTBOOT_REBOOT_REASONS_COUNT) + return -EINVAL; + + return bcb_write_reboot_reason(CONFIG_FASTBOOT_FLASH_MMC_DEV, "misc", boot_cmds[reason]); +#else + return -EINVAL; +#endif }
/**
participants (1)
-
Roman Kovalivskyi