diff mbox series

[RFC,01/13] cmd: bootefi: unfold do_bootefi_image()

Message ID 20231026053052.622453-2-takahiro.akashi@linaro.org
State Superseded
Headers show
Series cmd: bootefi: refactor the code for bootmgr | expand

Commit Message

AKASHI Takahiro Oct. 26, 2023, 5:30 a.m. UTC
Unfold do_bootefi_image() into do_bootefi() for the sake of the succeeding
refactor work.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/bootefi.c | 101 ++++++++++++++++++--------------------------------
 1 file changed, 37 insertions(+), 64 deletions(-)

Comments

Heinrich Schuchardt Oct. 26, 2023, 11:01 a.m. UTC | #1
On 10/26/23 07:30, AKASHI Takahiro wrote:
> Unfold do_bootefi_image() into do_bootefi() for the sake of the succeeding
> refactor work.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   cmd/bootefi.c | 101 ++++++++++++++++++--------------------------------
>   1 file changed, 37 insertions(+), 64 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 20e5c94a33a4..1b28bf5a318d 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -425,58 +425,6 @@ static int do_efibootmgr(void)
>   	return CMD_RET_SUCCESS;
>   }
>
> -/**
> - * do_bootefi_image() - execute EFI binary
> - *
> - * Set up memory image for the binary to be loaded, prepare device path, and
> - * then call do_bootefi_exec() to execute it.
> - *
> - * @image_opt:	string with image start address
> - * @size_opt:	string with image size or NULL
> - * Return:	status code
> - */
> -static int do_bootefi_image(const char *image_opt, const char *size_opt)
> -{
> -	void *image_buf;
> -	unsigned long addr, size;
> -	efi_status_t ret;
> -
> -#ifdef CONFIG_CMD_BOOTEFI_HELLO
> -	if (!strcmp(image_opt, "hello")) {
> -		image_buf = __efi_helloworld_begin;
> -		size = __efi_helloworld_end - __efi_helloworld_begin;
> -		efi_clear_bootdev();
> -	} else
> -#endif
> -	{
> -		addr = strtoul(image_opt, NULL, 16);
> -		/* Check that a numeric value was passed */
> -		if (!addr)
> -			return CMD_RET_USAGE;
> -		image_buf = map_sysmem(addr, 0);
> -
> -		if (size_opt) {
> -			size = strtoul(size_opt, NULL, 16);
> -			if (!size)
> -				return CMD_RET_USAGE;
> -			efi_clear_bootdev();
> -		} else {
> -			if (image_buf != image_addr) {
> -				log_err("No UEFI binary known at %s\n",
> -					image_opt);
> -				return CMD_RET_FAILURE;
> -			}
> -			size = image_size;
> -		}
> -	}
> -	ret = efi_run_image(image_buf, size);
> -
> -	if (ret != EFI_SUCCESS)
> -		return CMD_RET_FAILURE;
> -
> -	return CMD_RET_SUCCESS;
> -}
> -
>   /**
>    * efi_run_image() - run loaded UEFI image
>    *
> @@ -648,8 +596,9 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
>   		      char *const argv[])
>   {
>   	efi_status_t ret;
> -	char *img_addr, *img_size, *str_copy, *pos;
> -	void *fdt;
> +	char *p;
> +	void *fdt, *image_buf;
> +	unsigned long addr, size;
>
>   	if (argc < 2)
>   		return CMD_RET_USAGE;
> @@ -684,18 +633,42 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
>   	if (!strcmp(argv[1], "selftest"))
>   		return do_efi_selftest();
>   #endif
> -	str_copy = strdup(argv[1]);
> -	if (!str_copy) {
> -		log_err("Out of memory\n");
> -		return CMD_RET_FAILURE;
> +
> +#ifdef CONFIG_CMD_BOOTEFI_HELLO

I would prefer a normal if and let the linker sort it out.

Otherwise looks good to me.

Best regards

Heinrich

> +	if (!strcmp(argv[1], "hello")) {
> +		image_buf = __efi_helloworld_begin;
> +		size = __efi_helloworld_end - __efi_helloworld_begin;
> +		efi_clear_bootdev();
> +	} else
> +#endif
> +	{
> +		addr = strtoul(argv[1], NULL, 16);
> +		/* Check that a numeric value was passed */
> +		if (!addr)
> +			return CMD_RET_USAGE;
> +		image_buf = map_sysmem(addr, 0);
> +
> +		p  = strchr(argv[1], ':');
> +		if (p) {
> +			size = strtoul(++p, NULL, 16);
> +			if (!size)
> +				return CMD_RET_USAGE;
> +			efi_clear_bootdev();
> +		} else {
> +			if (image_buf != image_addr) {
> +				log_err("No UEFI binary known at %s\n",
> +					argv[1]);
> +				return CMD_RET_FAILURE;
> +			}
> +			size = image_size;
> +		}
>   	}
> -	pos = str_copy;
> -	img_addr = strsep(&pos, ":");
> -	img_size = strsep(&pos, ":");
> -	ret = do_bootefi_image(img_addr, img_size);
> -	free(str_copy);
> +	ret = efi_run_image(image_buf, size);
>
> -	return ret;
> +	if (ret != EFI_SUCCESS)
> +		return CMD_RET_FAILURE;
> +
> +	return CMD_RET_SUCCESS;
>   }
>
>   U_BOOT_LONGHELP(bootefi,
AKASHI Takahiro Oct. 27, 2023, 12:25 a.m. UTC | #2
On Thu, Oct 26, 2023 at 01:01:52PM +0200, Heinrich Schuchardt wrote:
> On 10/26/23 07:30, AKASHI Takahiro wrote:
> > Unfold do_bootefi_image() into do_bootefi() for the sake of the succeeding
> > refactor work.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >   cmd/bootefi.c | 101 ++++++++++++++++++--------------------------------
> >   1 file changed, 37 insertions(+), 64 deletions(-)
> > 
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 20e5c94a33a4..1b28bf5a318d 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -425,58 +425,6 @@ static int do_efibootmgr(void)
> >   	return CMD_RET_SUCCESS;
> >   }
> > 
> > -/**
> > - * do_bootefi_image() - execute EFI binary
> > - *
> > - * Set up memory image for the binary to be loaded, prepare device path, and
> > - * then call do_bootefi_exec() to execute it.
> > - *
> > - * @image_opt:	string with image start address
> > - * @size_opt:	string with image size or NULL
> > - * Return:	status code
> > - */
> > -static int do_bootefi_image(const char *image_opt, const char *size_opt)
> > -{
> > -	void *image_buf;
> > -	unsigned long addr, size;
> > -	efi_status_t ret;
> > -
> > -#ifdef CONFIG_CMD_BOOTEFI_HELLO
> > -	if (!strcmp(image_opt, "hello")) {
> > -		image_buf = __efi_helloworld_begin;
> > -		size = __efi_helloworld_end - __efi_helloworld_begin;
> > -		efi_clear_bootdev();
> > -	} else
> > -#endif
> > -	{
> > -		addr = strtoul(image_opt, NULL, 16);
> > -		/* Check that a numeric value was passed */
> > -		if (!addr)
> > -			return CMD_RET_USAGE;
> > -		image_buf = map_sysmem(addr, 0);
> > -
> > -		if (size_opt) {
> > -			size = strtoul(size_opt, NULL, 16);
> > -			if (!size)
> > -				return CMD_RET_USAGE;
> > -			efi_clear_bootdev();
> > -		} else {
> > -			if (image_buf != image_addr) {
> > -				log_err("No UEFI binary known at %s\n",
> > -					image_opt);
> > -				return CMD_RET_FAILURE;
> > -			}
> > -			size = image_size;
> > -		}
> > -	}
> > -	ret = efi_run_image(image_buf, size);
> > -
> > -	if (ret != EFI_SUCCESS)
> > -		return CMD_RET_FAILURE;
> > -
> > -	return CMD_RET_SUCCESS;
> > -}
> > -
> >   /**
> >    * efi_run_image() - run loaded UEFI image
> >    *
> > @@ -648,8 +596,9 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> >   		      char *const argv[])
> >   {
> >   	efi_status_t ret;
> > -	char *img_addr, *img_size, *str_copy, *pos;
> > -	void *fdt;
> > +	char *p;
> > +	void *fdt, *image_buf;
> > +	unsigned long addr, size;
> > 
> >   	if (argc < 2)
> >   		return CMD_RET_USAGE;
> > @@ -684,18 +633,42 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> >   	if (!strcmp(argv[1], "selftest"))
> >   		return do_efi_selftest();
> >   #endif
> > -	str_copy = strdup(argv[1]);
> > -	if (!str_copy) {
> > -		log_err("Out of memory\n");
> > -		return CMD_RET_FAILURE;
> > +
> > +#ifdef CONFIG_CMD_BOOTEFI_HELLO
> 
> I would prefer a normal if and let the linker sort it out.

Here I focused on simply unfolding("moving") the function, keeping the code
as same as possible. Any how the code is in a tentative form 
at this point and will be reshaped in later patches, using IS_ENABLED().
Please look at the final code after applying the whole series.

-Takahiro Akashi


> Otherwise looks good to me.
> 
> Best regards
> 
> Heinrich
> 
> > +	if (!strcmp(argv[1], "hello")) {
> > +		image_buf = __efi_helloworld_begin;
> > +		size = __efi_helloworld_end - __efi_helloworld_begin;
> > +		efi_clear_bootdev();
> > +	} else
> > +#endif
> > +	{
> > +		addr = strtoul(argv[1], NULL, 16);
> > +		/* Check that a numeric value was passed */
> > +		if (!addr)
> > +			return CMD_RET_USAGE;
> > +		image_buf = map_sysmem(addr, 0);
> > +
> > +		p  = strchr(argv[1], ':');
> > +		if (p) {
> > +			size = strtoul(++p, NULL, 16);
> > +			if (!size)
> > +				return CMD_RET_USAGE;
> > +			efi_clear_bootdev();
> > +		} else {
> > +			if (image_buf != image_addr) {
> > +				log_err("No UEFI binary known at %s\n",
> > +					argv[1]);
> > +				return CMD_RET_FAILURE;
> > +			}
> > +			size = image_size;
> > +		}
> >   	}
> > -	pos = str_copy;
> > -	img_addr = strsep(&pos, ":");
> > -	img_size = strsep(&pos, ":");
> > -	ret = do_bootefi_image(img_addr, img_size);
> > -	free(str_copy);
> > +	ret = efi_run_image(image_buf, size);
> > 
> > -	return ret;
> > +	if (ret != EFI_SUCCESS)
> > +		return CMD_RET_FAILURE;
> > +
> > +	return CMD_RET_SUCCESS;
> >   }
> > 
> >   U_BOOT_LONGHELP(bootefi,
>
Tom Rini Oct. 27, 2023, 1 a.m. UTC | #3
On Fri, Oct 27, 2023 at 09:25:44AM +0900, AKASHI Takahiro wrote:
> On Thu, Oct 26, 2023 at 01:01:52PM +0200, Heinrich Schuchardt wrote:
> > On 10/26/23 07:30, AKASHI Takahiro wrote:
> > > Unfold do_bootefi_image() into do_bootefi() for the sake of the succeeding
> > > refactor work.
> > > 
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > ---
> > >   cmd/bootefi.c | 101 ++++++++++++++++++--------------------------------
> > >   1 file changed, 37 insertions(+), 64 deletions(-)
> > > 
> > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > > index 20e5c94a33a4..1b28bf5a318d 100644
> > > --- a/cmd/bootefi.c
> > > +++ b/cmd/bootefi.c
> > > @@ -425,58 +425,6 @@ static int do_efibootmgr(void)
> > >   	return CMD_RET_SUCCESS;
> > >   }
> > > 
> > > -/**
> > > - * do_bootefi_image() - execute EFI binary
> > > - *
> > > - * Set up memory image for the binary to be loaded, prepare device path, and
> > > - * then call do_bootefi_exec() to execute it.
> > > - *
> > > - * @image_opt:	string with image start address
> > > - * @size_opt:	string with image size or NULL
> > > - * Return:	status code
> > > - */
> > > -static int do_bootefi_image(const char *image_opt, const char *size_opt)
> > > -{
> > > -	void *image_buf;
> > > -	unsigned long addr, size;
> > > -	efi_status_t ret;
> > > -
> > > -#ifdef CONFIG_CMD_BOOTEFI_HELLO
> > > -	if (!strcmp(image_opt, "hello")) {
> > > -		image_buf = __efi_helloworld_begin;
> > > -		size = __efi_helloworld_end - __efi_helloworld_begin;
> > > -		efi_clear_bootdev();
> > > -	} else
> > > -#endif
> > > -	{
> > > -		addr = strtoul(image_opt, NULL, 16);
> > > -		/* Check that a numeric value was passed */
> > > -		if (!addr)
> > > -			return CMD_RET_USAGE;
> > > -		image_buf = map_sysmem(addr, 0);
> > > -
> > > -		if (size_opt) {
> > > -			size = strtoul(size_opt, NULL, 16);
> > > -			if (!size)
> > > -				return CMD_RET_USAGE;
> > > -			efi_clear_bootdev();
> > > -		} else {
> > > -			if (image_buf != image_addr) {
> > > -				log_err("No UEFI binary known at %s\n",
> > > -					image_opt);
> > > -				return CMD_RET_FAILURE;
> > > -			}
> > > -			size = image_size;
> > > -		}
> > > -	}
> > > -	ret = efi_run_image(image_buf, size);
> > > -
> > > -	if (ret != EFI_SUCCESS)
> > > -		return CMD_RET_FAILURE;
> > > -
> > > -	return CMD_RET_SUCCESS;
> > > -}
> > > -
> > >   /**
> > >    * efi_run_image() - run loaded UEFI image
> > >    *
> > > @@ -648,8 +596,9 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> > >   		      char *const argv[])
> > >   {
> > >   	efi_status_t ret;
> > > -	char *img_addr, *img_size, *str_copy, *pos;
> > > -	void *fdt;
> > > +	char *p;
> > > +	void *fdt, *image_buf;
> > > +	unsigned long addr, size;
> > > 
> > >   	if (argc < 2)
> > >   		return CMD_RET_USAGE;
> > > @@ -684,18 +633,42 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> > >   	if (!strcmp(argv[1], "selftest"))
> > >   		return do_efi_selftest();
> > >   #endif
> > > -	str_copy = strdup(argv[1]);
> > > -	if (!str_copy) {
> > > -		log_err("Out of memory\n");
> > > -		return CMD_RET_FAILURE;
> > > +
> > > +#ifdef CONFIG_CMD_BOOTEFI_HELLO
> > 
> > I would prefer a normal if and let the linker sort it out.
> 
> Here I focused on simply unfolding("moving") the function, keeping the code
> as same as possible. Any how the code is in a tentative form 
> at this point and will be reshaped in later patches, using IS_ENABLED().
> Please look at the final code after applying the whole series.

I also agree with this approach.
Ilias Apalodimas Oct. 27, 2023, 12:23 p.m. UTC | #4
Akashi-san

On Fri, 27 Oct 2023 at 04:00, Tom Rini <trini@konsulko.com> wrote:
>
> On Fri, Oct 27, 2023 at 09:25:44AM +0900, AKASHI Takahiro wrote:
> > On Thu, Oct 26, 2023 at 01:01:52PM +0200, Heinrich Schuchardt wrote:
> > > On 10/26/23 07:30, AKASHI Takahiro wrote:
> > > > Unfold do_bootefi_image() into do_bootefi() for the sake of the succeeding
> > > > refactor work.
> > > >

I don't disagree with the patchset, but what the patch does is pretty
obvious.  It would help a lot to briefly describe how the unfolding
process helps the refactoring.

Thanks
/Ilias
> > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > ---
> > > >   cmd/bootefi.c | 101 ++++++++++++++++++--------------------------------
> > > >   1 file changed, 37 insertions(+), 64 deletions(-)
> > > >
> > > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > > > index 20e5c94a33a4..1b28bf5a318d 100644
> > > > --- a/cmd/bootefi.c
> > > > +++ b/cmd/bootefi.c
> > > > @@ -425,58 +425,6 @@ static int do_efibootmgr(void)
> > > >           return CMD_RET_SUCCESS;
> > > >   }
> > > >
> > > > -/**
> > > > - * do_bootefi_image() - execute EFI binary
> > > > - *
> > > > - * Set up memory image for the binary to be loaded, prepare device path, and
> > > > - * then call do_bootefi_exec() to execute it.
> > > > - *
> > > > - * @image_opt:   string with image start address
> > > > - * @size_opt:    string with image size or NULL
> > > > - * Return:       status code
> > > > - */
> > > > -static int do_bootefi_image(const char *image_opt, const char *size_opt)
> > > > -{
> > > > - void *image_buf;
> > > > - unsigned long addr, size;
> > > > - efi_status_t ret;
> > > > -
> > > > -#ifdef CONFIG_CMD_BOOTEFI_HELLO
> > > > - if (!strcmp(image_opt, "hello")) {
> > > > -         image_buf = __efi_helloworld_begin;
> > > > -         size = __efi_helloworld_end - __efi_helloworld_begin;
> > > > -         efi_clear_bootdev();
> > > > - } else
> > > > -#endif
> > > > - {
> > > > -         addr = strtoul(image_opt, NULL, 16);
> > > > -         /* Check that a numeric value was passed */
> > > > -         if (!addr)
> > > > -                 return CMD_RET_USAGE;
> > > > -         image_buf = map_sysmem(addr, 0);
> > > > -
> > > > -         if (size_opt) {
> > > > -                 size = strtoul(size_opt, NULL, 16);
> > > > -                 if (!size)
> > > > -                         return CMD_RET_USAGE;
> > > > -                 efi_clear_bootdev();
> > > > -         } else {
> > > > -                 if (image_buf != image_addr) {
> > > > -                         log_err("No UEFI binary known at %s\n",
> > > > -                                 image_opt);
> > > > -                         return CMD_RET_FAILURE;
> > > > -                 }
> > > > -                 size = image_size;
> > > > -         }
> > > > - }
> > > > - ret = efi_run_image(image_buf, size);
> > > > -
> > > > - if (ret != EFI_SUCCESS)
> > > > -         return CMD_RET_FAILURE;
> > > > -
> > > > - return CMD_RET_SUCCESS;
> > > > -}
> > > > -
> > > >   /**
> > > >    * efi_run_image() - run loaded UEFI image
> > > >    *
> > > > @@ -648,8 +596,9 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> > > >                         char *const argv[])
> > > >   {
> > > >           efi_status_t ret;
> > > > - char *img_addr, *img_size, *str_copy, *pos;
> > > > - void *fdt;
> > > > + char *p;
> > > > + void *fdt, *image_buf;
> > > > + unsigned long addr, size;
> > > >
> > > >           if (argc < 2)
> > > >                   return CMD_RET_USAGE;
> > > > @@ -684,18 +633,42 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> > > >           if (!strcmp(argv[1], "selftest"))
> > > >                   return do_efi_selftest();
> > > >   #endif
> > > > - str_copy = strdup(argv[1]);
> > > > - if (!str_copy) {
> > > > -         log_err("Out of memory\n");
> > > > -         return CMD_RET_FAILURE;
> > > > +
> > > > +#ifdef CONFIG_CMD_BOOTEFI_HELLO
> > >
> > > I would prefer a normal if and let the linker sort it out.
> >
> > Here I focused on simply unfolding("moving") the function, keeping the code
> > as same as possible. Any how the code is in a tentative form
> > at this point and will be reshaped in later patches, using IS_ENABLED().
> > Please look at the final code after applying the whole series.
>
> I also agree with this approach.
>
> --
> Tom
AKASHI Takahiro Oct. 30, 2023, 12:34 a.m. UTC | #5
Hi Ilias,

On Fri, Oct 27, 2023 at 03:23:07PM +0300, Ilias Apalodimas wrote:
> Akashi-san
> 
> On Fri, 27 Oct 2023 at 04:00, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Fri, Oct 27, 2023 at 09:25:44AM +0900, AKASHI Takahiro wrote:
> > > On Thu, Oct 26, 2023 at 01:01:52PM +0200, Heinrich Schuchardt wrote:
> > > > On 10/26/23 07:30, AKASHI Takahiro wrote:
> > > > > Unfold do_bootefi_image() into do_bootefi() for the sake of the succeeding
> > > > > refactor work.
> > > > >
> 
> I don't disagree with the patchset, but what the patch does is pretty
> obvious.

Yeah, that is what I tried to do in this patch series, i.e. each step
be as trivial as possible to ensure that the semantics is unchanged
while the code is being morphed.

> It would help a lot to briefly describe how the unfolding
> process helps the refactoring.

Not sure, but will add a few more words.

Thanks,
-Takahiro Akashi


> Thanks
> /Ilias
> > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > > > > ---
> > > > >   cmd/bootefi.c | 101 ++++++++++++++++++--------------------------------
> > > > >   1 file changed, 37 insertions(+), 64 deletions(-)
> > > > >
> > > > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > > > > index 20e5c94a33a4..1b28bf5a318d 100644
> > > > > --- a/cmd/bootefi.c
> > > > > +++ b/cmd/bootefi.c
> > > > > @@ -425,58 +425,6 @@ static int do_efibootmgr(void)
> > > > >           return CMD_RET_SUCCESS;
> > > > >   }
> > > > >
> > > > > -/**
> > > > > - * do_bootefi_image() - execute EFI binary
> > > > > - *
> > > > > - * Set up memory image for the binary to be loaded, prepare device path, and
> > > > > - * then call do_bootefi_exec() to execute it.
> > > > > - *
> > > > > - * @image_opt:   string with image start address
> > > > > - * @size_opt:    string with image size or NULL
> > > > > - * Return:       status code
> > > > > - */
> > > > > -static int do_bootefi_image(const char *image_opt, const char *size_opt)
> > > > > -{
> > > > > - void *image_buf;
> > > > > - unsigned long addr, size;
> > > > > - efi_status_t ret;
> > > > > -
> > > > > -#ifdef CONFIG_CMD_BOOTEFI_HELLO
> > > > > - if (!strcmp(image_opt, "hello")) {
> > > > > -         image_buf = __efi_helloworld_begin;
> > > > > -         size = __efi_helloworld_end - __efi_helloworld_begin;
> > > > > -         efi_clear_bootdev();
> > > > > - } else
> > > > > -#endif
> > > > > - {
> > > > > -         addr = strtoul(image_opt, NULL, 16);
> > > > > -         /* Check that a numeric value was passed */
> > > > > -         if (!addr)
> > > > > -                 return CMD_RET_USAGE;
> > > > > -         image_buf = map_sysmem(addr, 0);
> > > > > -
> > > > > -         if (size_opt) {
> > > > > -                 size = strtoul(size_opt, NULL, 16);
> > > > > -                 if (!size)
> > > > > -                         return CMD_RET_USAGE;
> > > > > -                 efi_clear_bootdev();
> > > > > -         } else {
> > > > > -                 if (image_buf != image_addr) {
> > > > > -                         log_err("No UEFI binary known at %s\n",
> > > > > -                                 image_opt);
> > > > > -                         return CMD_RET_FAILURE;
> > > > > -                 }
> > > > > -                 size = image_size;
> > > > > -         }
> > > > > - }
> > > > > - ret = efi_run_image(image_buf, size);
> > > > > -
> > > > > - if (ret != EFI_SUCCESS)
> > > > > -         return CMD_RET_FAILURE;
> > > > > -
> > > > > - return CMD_RET_SUCCESS;
> > > > > -}
> > > > > -
> > > > >   /**
> > > > >    * efi_run_image() - run loaded UEFI image
> > > > >    *
> > > > > @@ -648,8 +596,9 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> > > > >                         char *const argv[])
> > > > >   {
> > > > >           efi_status_t ret;
> > > > > - char *img_addr, *img_size, *str_copy, *pos;
> > > > > - void *fdt;
> > > > > + char *p;
> > > > > + void *fdt, *image_buf;
> > > > > + unsigned long addr, size;
> > > > >
> > > > >           if (argc < 2)
> > > > >                   return CMD_RET_USAGE;
> > > > > @@ -684,18 +633,42 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
> > > > >           if (!strcmp(argv[1], "selftest"))
> > > > >                   return do_efi_selftest();
> > > > >   #endif
> > > > > - str_copy = strdup(argv[1]);
> > > > > - if (!str_copy) {
> > > > > -         log_err("Out of memory\n");
> > > > > -         return CMD_RET_FAILURE;
> > > > > +
> > > > > +#ifdef CONFIG_CMD_BOOTEFI_HELLO
> > > >
> > > > I would prefer a normal if and let the linker sort it out.
> > >
> > > Here I focused on simply unfolding("moving") the function, keeping the code
> > > as same as possible. Any how the code is in a tentative form
> > > at this point and will be reshaped in later patches, using IS_ENABLED().
> > > Please look at the final code after applying the whole series.
> >
> > I also agree with this approach.
> >
> > --
> > Tom
diff mbox series

Patch

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 20e5c94a33a4..1b28bf5a318d 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -425,58 +425,6 @@  static int do_efibootmgr(void)
 	return CMD_RET_SUCCESS;
 }
 
-/**
- * do_bootefi_image() - execute EFI binary
- *
- * Set up memory image for the binary to be loaded, prepare device path, and
- * then call do_bootefi_exec() to execute it.
- *
- * @image_opt:	string with image start address
- * @size_opt:	string with image size or NULL
- * Return:	status code
- */
-static int do_bootefi_image(const char *image_opt, const char *size_opt)
-{
-	void *image_buf;
-	unsigned long addr, size;
-	efi_status_t ret;
-
-#ifdef CONFIG_CMD_BOOTEFI_HELLO
-	if (!strcmp(image_opt, "hello")) {
-		image_buf = __efi_helloworld_begin;
-		size = __efi_helloworld_end - __efi_helloworld_begin;
-		efi_clear_bootdev();
-	} else
-#endif
-	{
-		addr = strtoul(image_opt, NULL, 16);
-		/* Check that a numeric value was passed */
-		if (!addr)
-			return CMD_RET_USAGE;
-		image_buf = map_sysmem(addr, 0);
-
-		if (size_opt) {
-			size = strtoul(size_opt, NULL, 16);
-			if (!size)
-				return CMD_RET_USAGE;
-			efi_clear_bootdev();
-		} else {
-			if (image_buf != image_addr) {
-				log_err("No UEFI binary known at %s\n",
-					image_opt);
-				return CMD_RET_FAILURE;
-			}
-			size = image_size;
-		}
-	}
-	ret = efi_run_image(image_buf, size);
-
-	if (ret != EFI_SUCCESS)
-		return CMD_RET_FAILURE;
-
-	return CMD_RET_SUCCESS;
-}
-
 /**
  * efi_run_image() - run loaded UEFI image
  *
@@ -648,8 +596,9 @@  static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
 		      char *const argv[])
 {
 	efi_status_t ret;
-	char *img_addr, *img_size, *str_copy, *pos;
-	void *fdt;
+	char *p;
+	void *fdt, *image_buf;
+	unsigned long addr, size;
 
 	if (argc < 2)
 		return CMD_RET_USAGE;
@@ -684,18 +633,42 @@  static int do_bootefi(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (!strcmp(argv[1], "selftest"))
 		return do_efi_selftest();
 #endif
-	str_copy = strdup(argv[1]);
-	if (!str_copy) {
-		log_err("Out of memory\n");
-		return CMD_RET_FAILURE;
+
+#ifdef CONFIG_CMD_BOOTEFI_HELLO
+	if (!strcmp(argv[1], "hello")) {
+		image_buf = __efi_helloworld_begin;
+		size = __efi_helloworld_end - __efi_helloworld_begin;
+		efi_clear_bootdev();
+	} else
+#endif
+	{
+		addr = strtoul(argv[1], NULL, 16);
+		/* Check that a numeric value was passed */
+		if (!addr)
+			return CMD_RET_USAGE;
+		image_buf = map_sysmem(addr, 0);
+
+		p  = strchr(argv[1], ':');
+		if (p) {
+			size = strtoul(++p, NULL, 16);
+			if (!size)
+				return CMD_RET_USAGE;
+			efi_clear_bootdev();
+		} else {
+			if (image_buf != image_addr) {
+				log_err("No UEFI binary known at %s\n",
+					argv[1]);
+				return CMD_RET_FAILURE;
+			}
+			size = image_size;
+		}
 	}
-	pos = str_copy;
-	img_addr = strsep(&pos, ":");
-	img_size = strsep(&pos, ":");
-	ret = do_bootefi_image(img_addr, img_size);
-	free(str_copy);
+	ret = efi_run_image(image_buf, size);
 
-	return ret;
+	if (ret != EFI_SUCCESS)
+		return CMD_RET_FAILURE;
+
+	return CMD_RET_SUCCESS;
 }
 
 U_BOOT_LONGHELP(bootefi,