| Message ID | 20231112000923.73568-28-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | bootm: Refactoring to reduce reliance on CMDLINE (part A) | expand |
On Sat, Nov 11, 2023 at 05:09:12PM -0700, Simon Glass wrote: > Rather than passing it all the command-line args, pass in the pieces > that it needs. These are the image address, the ramdisk address/name > and the FDT address/name. > > Ultimately this will allow usage of this function without being called > from the command line. OK, so this goal is good. [snip] > + return bootm_find_images(img_addr, argc > 1 ? argv[1] : NULL, > + argc > 2 ? argv[2] : NULL, 0, 0); That we repeat this much harder to read test/if/else three times now is less good. Can we find some way to hide the complexity here in the case where it's coming from a command and so we have argc/argv[] ?
Hi Tom, On Wed, 15 Nov 2023 at 15:38, Tom Rini <trini@konsulko.com> wrote: > > On Sat, Nov 11, 2023 at 05:09:12PM -0700, Simon Glass wrote: > > > Rather than passing it all the command-line args, pass in the pieces > > that it needs. These are the image address, the ramdisk address/name > > and the FDT address/name. > > > > Ultimately this will allow usage of this function without being called > > from the command line. > > OK, so this goal is good. > > [snip] > > + return bootm_find_images(img_addr, argc > 1 ? argv[1] : NULL, > > + argc > 2 ? argv[2] : NULL, 0, 0); > > That we repeat this much harder to read test/if/else three times now is > less good. Can we find some way to hide the complexity here in the case > where it's coming from a command and so we have argc/argv[] ? I can't really think of one. Ultimately this is coming from the fact that the booti and bootz commands directly call bootm_find_images(). I haven't got far enough to know whether that will still be true in the end, but I hope not. IMO the correct place for the logic above is in the command-processing code, where it decides which arg means what. I could imagine something like: static const char *cmd_get_arg(int argc, char *const argv[], int argnum) { return argc > argnum ? argv[argnum] : NULL; } but I'm not sure that is an improvement. I have got a little further (15 more patches) and have added a 'struct bootm_info' to hold the state of the boot, including the arguments for the FIT address, ramdisk and fdt. But even then, the args will need to be set up by the individual commands based on their syntax. Regards, Simon
On Wed, Nov 15, 2023 at 06:42:19PM -0700, Simon Glass wrote: > Hi Tom, > > On Wed, 15 Nov 2023 at 15:38, Tom Rini <trini@konsulko.com> wrote: > > > > On Sat, Nov 11, 2023 at 05:09:12PM -0700, Simon Glass wrote: > > > > > Rather than passing it all the command-line args, pass in the pieces > > > that it needs. These are the image address, the ramdisk address/name > > > and the FDT address/name. > > > > > > Ultimately this will allow usage of this function without being called > > > from the command line. > > > > OK, so this goal is good. > > > > [snip] > > > + return bootm_find_images(img_addr, argc > 1 ? argv[1] : NULL, > > > + argc > 2 ? argv[2] : NULL, 0, 0); > > > > That we repeat this much harder to read test/if/else three times now is > > less good. Can we find some way to hide the complexity here in the case > > where it's coming from a command and so we have argc/argv[] ? > > I can't really think of one. Ultimately this is coming from the fact > that the booti and bootz commands directly call bootm_find_images(). I > haven't got far enough to know whether that will still be true in the > end, but I hope not. > > IMO the correct place for the logic above is in the command-processing > code, where it decides which arg means what. > > I could imagine something like: > > static const char *cmd_get_arg(int argc, char *const argv[], int argnum) > { > return argc > argnum ? argv[argnum] : NULL; > } > > but I'm not sure that is an improvement. I was thinking about this more after I sent this. And I think we might indeed want an inline / macro to handle this case more generally as I suspect we'll have similar cases where we need argN-or-NULL as we refactor other areas of code to split "here is the command" from "here is the library functionality" of it. Then we'll have: ulong foo = cmd_arg_one(argc, argv); ulong bar = cmd_arg_two(argc, argv); librarycall(foo, bar, ...);
Hi Tom, On Wed, 15 Nov 2023 at 18:47, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Nov 15, 2023 at 06:42:19PM -0700, Simon Glass wrote: > > Hi Tom, > > > > On Wed, 15 Nov 2023 at 15:38, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Sat, Nov 11, 2023 at 05:09:12PM -0700, Simon Glass wrote: > > > > > > > Rather than passing it all the command-line args, pass in the pieces > > > > that it needs. These are the image address, the ramdisk address/name > > > > and the FDT address/name. > > > > > > > > Ultimately this will allow usage of this function without being called > > > > from the command line. > > > > > > OK, so this goal is good. > > > > > > [snip] > > > > + return bootm_find_images(img_addr, argc > 1 ? argv[1] : NULL, > > > > + argc > 2 ? argv[2] : NULL, 0, 0); > > > > > > That we repeat this much harder to read test/if/else three times now is > > > less good. Can we find some way to hide the complexity here in the case > > > where it's coming from a command and so we have argc/argv[] ? > > > > I can't really think of one. Ultimately this is coming from the fact > > that the booti and bootz commands directly call bootm_find_images(). I > > haven't got far enough to know whether that will still be true in the > > end, but I hope not. > > > > IMO the correct place for the logic above is in the command-processing > > code, where it decides which arg means what. > > > > I could imagine something like: > > > > static const char *cmd_get_arg(int argc, char *const argv[], int argnum) > > { > > return argc > argnum ? argv[argnum] : NULL; > > } > > > > but I'm not sure that is an improvement. > > I was thinking about this more after I sent this. And I think we might > indeed want an inline / macro to handle this case more generally as I > suspect we'll have similar cases where we need argN-or-NULL as we > refactor other areas of code to split "here is the command" from "here > is the library functionality" of it. Then we'll have: > ulong foo = cmd_arg_one(argc, argv); > ulong bar = cmd_arg_two(argc, argv); > > librarycall(foo, bar, ...); OK I can try something. But note that the one, two terminology becomes confusing. Is the first argument zero or one? Also we often drop an argument in a subcommand to allow things to start from 0 again. It might be better to use first and second, instead? Regards, Simon
On Wed, Nov 15, 2023 at 06:56:33PM -0700, Simon Glass wrote: > Hi Tom, > > On Wed, 15 Nov 2023 at 18:47, Tom Rini <trini@konsulko.com> wrote: > > > > On Wed, Nov 15, 2023 at 06:42:19PM -0700, Simon Glass wrote: > > > Hi Tom, > > > > > > On Wed, 15 Nov 2023 at 15:38, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > On Sat, Nov 11, 2023 at 05:09:12PM -0700, Simon Glass wrote: > > > > > > > > > Rather than passing it all the command-line args, pass in the pieces > > > > > that it needs. These are the image address, the ramdisk address/name > > > > > and the FDT address/name. > > > > > > > > > > Ultimately this will allow usage of this function without being called > > > > > from the command line. > > > > > > > > OK, so this goal is good. > > > > > > > > [snip] > > > > > + return bootm_find_images(img_addr, argc > 1 ? argv[1] : NULL, > > > > > + argc > 2 ? argv[2] : NULL, 0, 0); > > > > > > > > That we repeat this much harder to read test/if/else three times now is > > > > less good. Can we find some way to hide the complexity here in the case > > > > where it's coming from a command and so we have argc/argv[] ? > > > > > > I can't really think of one. Ultimately this is coming from the fact > > > that the booti and bootz commands directly call bootm_find_images(). I > > > haven't got far enough to know whether that will still be true in the > > > end, but I hope not. > > > > > > IMO the correct place for the logic above is in the command-processing > > > code, where it decides which arg means what. > > > > > > I could imagine something like: > > > > > > static const char *cmd_get_arg(int argc, char *const argv[], int argnum) > > > { > > > return argc > argnum ? argv[argnum] : NULL; > > > } > > > > > > but I'm not sure that is an improvement. > > > > I was thinking about this more after I sent this. And I think we might > > indeed want an inline / macro to handle this case more generally as I > > suspect we'll have similar cases where we need argN-or-NULL as we > > refactor other areas of code to split "here is the command" from "here > > is the library functionality" of it. Then we'll have: > > ulong foo = cmd_arg_one(argc, argv); > > ulong bar = cmd_arg_two(argc, argv); > > > > librarycall(foo, bar, ...); > > OK I can try something. But note that the one, two terminology becomes > confusing. Is the first argument zero or one? Also we often drop an > argument in a subcommand to allow things to start from 0 again. > > It might be better to use first and second, instead? Yes, please come up with a better naming scheme, I'm terrible about stuff like that.
Hi Tom, On Wed, 15 Nov 2023 at 19:07, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Nov 15, 2023 at 06:56:33PM -0700, Simon Glass wrote: > > Hi Tom, > > > > On Wed, 15 Nov 2023 at 18:47, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Wed, Nov 15, 2023 at 06:42:19PM -0700, Simon Glass wrote: > > > > Hi Tom, > > > > > > > > On Wed, 15 Nov 2023 at 15:38, Tom Rini <trini@konsulko.com> wrote: > > > > > > > > > > On Sat, Nov 11, 2023 at 05:09:12PM -0700, Simon Glass wrote: > > > > > > > > > > > Rather than passing it all the command-line args, pass in the pieces > > > > > > that it needs. These are the image address, the ramdisk address/name > > > > > > and the FDT address/name. > > > > > > > > > > > > Ultimately this will allow usage of this function without being called > > > > > > from the command line. > > > > > > > > > > OK, so this goal is good. > > > > > > > > > > [snip] > > > > > > + return bootm_find_images(img_addr, argc > 1 ? argv[1] : NULL, > > > > > > + argc > 2 ? argv[2] : NULL, 0, 0); > > > > > > > > > > That we repeat this much harder to read test/if/else three times now is > > > > > less good. Can we find some way to hide the complexity here in the case > > > > > where it's coming from a command and so we have argc/argv[] ? > > > > > > > > I can't really think of one. Ultimately this is coming from the fact > > > > that the booti and bootz commands directly call bootm_find_images(). I > > > > haven't got far enough to know whether that will still be true in the > > > > end, but I hope not. > > > > > > > > IMO the correct place for the logic above is in the command-processing > > > > code, where it decides which arg means what. > > > > > > > > I could imagine something like: > > > > > > > > static const char *cmd_get_arg(int argc, char *const argv[], int argnum) > > > > { > > > > return argc > argnum ? argv[argnum] : NULL; > > > > } > > > > > > > > but I'm not sure that is an improvement. > > > > > > I was thinking about this more after I sent this. And I think we might > > > indeed want an inline / macro to handle this case more generally as I > > > suspect we'll have similar cases where we need argN-or-NULL as we > > > refactor other areas of code to split "here is the command" from "here > > > is the library functionality" of it. Then we'll have: > > > ulong foo = cmd_arg_one(argc, argv); > > > ulong bar = cmd_arg_two(argc, argv); > > > > > > librarycall(foo, bar, ...); > > > > OK I can try something. But note that the one, two terminology becomes > > confusing. Is the first argument zero or one? Also we often drop an > > argument in a subcommand to allow things to start from 0 again. > > > > It might be better to use first and second, instead? > > Yes, please come up with a better naming scheme, I'm terrible about > stuff like that. Actually my naming has problems too...hopefully this has been solved properly elsewhere and someone will reply to my patch. Regards, Simon
diff --git a/boot/bootm.c b/boot/bootm.c index 62b6ca957f5b..54e4b48e907c 100644 --- a/boot/bootm.c +++ b/boot/bootm.c @@ -467,35 +467,14 @@ static int bootm_find_os(const char *addr_fit) return 0; } -/** - * bootm_find_images - wrapper to find and locate various images - * @flag: Ignored Argument - * @argc: command argument count - * @argv: command argument list - * @start: OS image start address - * @size: OS image size - * - * boot_find_images() will attempt to load an available ramdisk, - * flattened device tree, as well as specifically marked - * "loadable" images (loadables are FIT only) - * - * Note: bootm_find_images will skip an image if it is not found - * - * @return: - * 0, if all existing images were loaded correctly - * 1, if an image is found but corrupted, or invalid - */ -int bootm_find_images(int flag, int argc, char *const argv[], ulong start, - ulong size) +int bootm_find_images(ulong img_addr, const char *conf_ramdisk, + const char *conf_fdt, ulong start, ulong size) { - const char *select = NULL; + const char *select = conf_ramdisk; char addr_str[17]; - ulong img_addr; void *buf; int ret; - img_addr = argc ? hextoul(argv[0], NULL) : image_load_addr; - if (IS_ENABLED(CONFIG_ANDROID_BOOT_IMAGE)) { /* Look for an Android boot image */ buf = map_sysmem(images.os.start, 0); @@ -505,8 +484,8 @@ int bootm_find_images(int flag, int argc, char *const argv[], ulong start, } } - if (argc >= 2) - select = argv[1]; + if (conf_ramdisk) + select = conf_ramdisk; /* find ramdisk */ ret = boot_get_ramdisk(select, &images, IH_INITRD_ARCH, @@ -532,9 +511,8 @@ int bootm_find_images(int flag, int argc, char *const argv[], ulong start, buf = map_sysmem(img_addr, 0); /* find flattened device tree */ - ret = boot_get_fdt(buf, argc > 2 ? argv[2] : NULL, - IH_ARCH_DEFAULT, &images, &images.ft_addr, - &images.ft_len); + ret = boot_get_fdt(buf, conf_fdt, IH_ARCH_DEFAULT, &images, + &images.ft_addr, &images.ft_len); if (ret) { puts("Could not find a valid device tree\n"); return 1; @@ -583,8 +561,13 @@ static int bootm_find_other(struct cmd_tbl *cmdtp, int flag, int argc, images.os.type == IH_TYPE_KERNEL_NOLOAD || images.os.type == IH_TYPE_MULTI) && (images.os.os == IH_OS_LINUX || images.os.os == IH_OS_VXWORKS || - images.os.os == IH_OS_EFI || images.os.os == IH_OS_TEE)) - return bootm_find_images(flag, argc, argv, 0, 0); + images.os.os == IH_OS_EFI || images.os.os == IH_OS_TEE)) { + ulong img_addr; + + img_addr = argc ? hextoul(argv[0], NULL) : image_load_addr; + return bootm_find_images(img_addr, argc > 1 ? argv[1] : NULL, + argc > 2 ? argv[2] : NULL, 0, 0); + } return 0; } diff --git a/cmd/booti.c b/cmd/booti.c index a6c7db272c57..dc73c83f0db0 100644 --- a/cmd/booti.c +++ b/cmd/booti.c @@ -95,7 +95,9 @@ static int booti_start(struct cmd_tbl *cmdtp, int flag, int argc, * Handle the BOOTM_STATE_FINDOTHER state ourselves as we do not * have a header that provide this informaiton. */ - if (bootm_find_images(flag, argc, argv, relocated_addr, image_size)) + if (bootm_find_images(image_load_addr, argc > 1 ? argv[1] : NULL, + argc > 2 ? argv[2] : NULL, relocated_addr, + image_size)) return 1; return 0; diff --git a/cmd/bootz.c b/cmd/bootz.c index dd6fe4904b02..bcf232b4f305 100644 --- a/cmd/bootz.c +++ b/cmd/bootz.c @@ -54,7 +54,9 @@ static int bootz_start(struct cmd_tbl *cmdtp, int flag, int argc, * Handle the BOOTM_STATE_FINDOTHER state ourselves as we do not * have a header that provide this informaiton. */ - if (bootm_find_images(flag, argc, argv, images->ep, zi_end - zi_start)) + if (bootm_find_images(image_load_addr, argc > 1 ? argv[1] : NULL, + argc > 2 ? argv[2] : NULL, images->ep, + zi_end - zi_start)) return 1; return 0; diff --git a/include/bootm.h b/include/bootm.h index 10a1bd65a754..f5229ea90b33 100644 --- a/include/bootm.h +++ b/include/bootm.h @@ -52,9 +52,29 @@ int boot_selected_os(int argc, char *const argv[], int state, ulong bootm_disable_interrupts(void); -/* This is a special function used by booti/bootz */ -int bootm_find_images(int flag, int argc, char *const argv[], ulong start, - ulong size); +/** + * bootm_find_images() - find and locate various images + * + * @img_addr: Address of image being loaded + * @conf_ramdisk: Indicates the ramdisk to use (typically second arg of bootm) + * @conf_fdt: Indicates the FDT to use (typically third arg of bootm) + * @start: OS image start address + * @size: OS image size + * + * boot_find_images() will attempt to load an available ramdisk, + * flattened device tree, as well as specifically marked + * "loadable" images (loadables are FIT only) + * + * Note: bootm_find_images will skip an image if it is not found + * + * This is a special function used by booti/bootz + * + * Return: + * 0, if all existing images were loaded correctly + * 1, if an image is found but corrupted, or invalid + */ +int bootm_find_images(ulong img_addr, const char *conf_ramdisk, + const char *conf_fdt, ulong start, ulong size); /* * Measure the boot images. Measurement is the process of hashing some binary
Rather than passing it all the command-line args, pass in the pieces that it needs. These are the image address, the ramdisk address/name and the FDT address/name. Ultimately this will allow usage of this function without being called from the command line. Move the function comment to the header file and tidy it a little. Signed-off-by: Simon Glass <sjg@chromium.org> --- boot/bootm.c | 45 ++++++++++++++------------------------------- cmd/booti.c | 4 +++- cmd/bootz.c | 4 +++- include/bootm.h | 26 +++++++++++++++++++++++--- 4 files changed, 43 insertions(+), 36 deletions(-)