diff mbox series

[U-Boot,v2,2/5] fdt: boot_get_fdt: really boot w/o FDT when "goto no_fdt"

Message ID 20190401104537.29801-3-erosca@de.adit-jv.com
State Accepted
Delegated to: Simon Glass
Headers show
Series boot_get_fdt: clean up and use 'fdtaddr' as fallback for Android | expand

Commit Message

Eugeniu Rosca April 1, 2019, 10:45 a.m. UTC 
The 'no_fdt' goto label was introduced by v2015.01 commit [0] and it
had two review stages [1-2]. The *documented* purpose behind commit [0]
is (excerpt from commit description):

> allows both FDT and non-FDT kernels to boot by making the
> third parameter to the bootm/bootz optional

While [1] and [2] share the same goal, they have very different
implementations:
 - [1] was based on a very simple 'argc' check at function error out
   with returning success to the caller if the third parameter was NOT
   passed to bootm/bootz command. This approach had the downside of
   returning success to the caller even in case of legitimate internal
   errors, which should halt booting.
 - [2] added the "no_fdt" label and several "goto no_fdt" statements.
   This allowed to report the legitimate internal errors to the caller.

IOW the major difference between [1] and [2] is:
 - [1] boot w/o FDT if FDT address is not passed to boot{m,z,*}
 - [2] give *freedom* to the developer to boot w/o FDT from any
   (more or less) arbitrary point in the function flow (and here
   comes the peculiar aspect, which looks to be a leftover from [1])
   with the precondition that the 3rd argument (FDT address) is NOT
   provided to boot{m,z,*}. In practice, this means that only a subset
   of "goto no_fdt" end up booting w/o FDT while the other subset is
   returning an error to the caller.

This patch removes the peculiar behavior described above, such that
"goto no_fdt" performs really what it tells to the developer.

The motivation of this patch is to decrease the unneeded complexity
and increase the readability of boot_get_fdt().

[0] 48aead71c1ad ("fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined")
[1] https://patchwork.ozlabs.org/patch/412923/
    ("[U-Boot,v1] fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined")
[2] https://patchwork.ozlabs.org/patch/415635/
    ("[U-Boot,v2] fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined")

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
Changes in v2:
 - NA
 - Link v1: https://patchwork.ozlabs.org/patch/1071587/
---
 common/image-fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Simon Glass April 21, 2019, 7:26 p.m. UTC | #1 
On Mon, 1 Apr 2019 at 03:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> The 'no_fdt' goto label was introduced by v2015.01 commit [0] and it
> had two review stages [1-2]. The *documented* purpose behind commit [0]
> is (excerpt from commit description):
>
> > allows both FDT and non-FDT kernels to boot by making the
> > third parameter to the bootm/bootz optional
>
> While [1] and [2] share the same goal, they have very different
> implementations:
>  - [1] was based on a very simple 'argc' check at function error out
>    with returning success to the caller if the third parameter was NOT
>    passed to bootm/bootz command. This approach had the downside of
>    returning success to the caller even in case of legitimate internal
>    errors, which should halt booting.
>  - [2] added the "no_fdt" label and several "goto no_fdt" statements.
>    This allowed to report the legitimate internal errors to the caller.
>
> IOW the major difference between [1] and [2] is:
>  - [1] boot w/o FDT if FDT address is not passed to boot{m,z,*}
>  - [2] give *freedom* to the developer to boot w/o FDT from any
>    (more or less) arbitrary point in the function flow (and here
>    comes the peculiar aspect, which looks to be a leftover from [1])
>    with the precondition that the 3rd argument (FDT address) is NOT
>    provided to boot{m,z,*}. In practice, this means that only a subset
>    of "goto no_fdt" end up booting w/o FDT while the other subset is
>    returning an error to the caller.
>
> This patch removes the peculiar behavior described above, such that
> "goto no_fdt" performs really what it tells to the developer.
>
> The motivation of this patch is to decrease the unneeded complexity
> and increase the readability of boot_get_fdt().
>
> [0] 48aead71c1ad ("fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined")
> [1] https://patchwork.ozlabs.org/patch/412923/
>     ("[U-Boot,v1] fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined")
> [2] https://patchwork.ozlabs.org/patch/415635/
>     ("[U-Boot,v2] fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined")
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
> Changes in v2:
>  - NA
>  - Link v1: https://patchwork.ozlabs.org/patch/1071587/
> ---
>  common/image-fdt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Can we create a test of the boot_get_fdt() behaviour?

We have test_fit and test_vboot as possible examples, although perhaps
a test that directly calls this function would be better?

Regards,
Simon



> diff --git a/common/image-fdt.c b/common/image-fdt.c
> index 1817ce6bce30..c335e7e2f220 100644
> --- a/common/image-fdt.c
> +++ b/common/image-fdt.c
> @@ -489,7 +489,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
>  no_fdt:
>         ok_no_fdt = 1;
>  error:
> -       if (!select && ok_no_fdt) {
> +       if (ok_no_fdt) {
>                 debug("Continuing to boot without FDT\n");
>                 return 0;
>         }
> --
> 2.21.0
>
Simon Glass April 22, 2019, 2:38 a.m. UTC | #2 
On Mon, 1 Apr 2019 at 03:46, Eugeniu Rosca <erosca@de.adit-jv.com> wrote:
>
> The 'no_fdt' goto label was introduced by v2015.01 commit [0] and it
> had two review stages [1-2]. The *documented* purpose behind commit [0]
> is (excerpt from commit description):
>
> > allows both FDT and non-FDT kernels to boot by making the
> > third parameter to the bootm/bootz optional
>
> While [1] and [2] share the same goal, they have very different
> implementations:
>  - [1] was based on a very simple 'argc' check at function error out
>    with returning success to the caller if the third parameter was NOT
>    passed to bootm/bootz command. This approach had the downside of
>    returning success to the caller even in case of legitimate internal
>    errors, which should halt booting.
>  - [2] added the "no_fdt" label and several "goto no_fdt" statements.
>    This allowed to report the legitimate internal errors to the caller.
>
> IOW the major difference between [1] and [2] is:
>  - [1] boot w/o FDT if FDT address is not passed to boot{m,z,*}
>  - [2] give *freedom* to the developer to boot w/o FDT from any
>    (more or less) arbitrary point in the function flow (and here
>    comes the peculiar aspect, which looks to be a leftover from [1])
>    with the precondition that the 3rd argument (FDT address) is NOT
>    provided to boot{m,z,*}. In practice, this means that only a subset
>    of "goto no_fdt" end up booting w/o FDT while the other subset is
>    returning an error to the caller.
>
> This patch removes the peculiar behavior described above, such that
> "goto no_fdt" performs really what it tells to the developer.
>
> The motivation of this patch is to decrease the unneeded complexity
> and increase the readability of boot_get_fdt().
>
> [0] 48aead71c1ad ("fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined")
> [1] https://patchwork.ozlabs.org/patch/412923/
>     ("[U-Boot,v1] fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined")
> [2] https://patchwork.ozlabs.org/patch/415635/
>     ("[U-Boot,v2] fdt: Allow non-FDT kernels to boot when CONFIG_OF_LIBFDT is defined")
>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
> Changes in v2:
>  - NA
>  - Link v1: https://patchwork.ozlabs.org/patch/1071587/
> ---
>  common/image-fdt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Can we create a test of the boot_get_fdt() behaviour?

We have test_fit and test_vboot as possible examples, although perhaps
a test that directly calls this function would be better?

Regards,
Simon



Applied to u-boot-dm, thanks!
Eugeniu Rosca April 25, 2019, 5:04 p.m. UTC | #3 
Hi Simon,

On Sun, Apr 21, 2019 at 07:38:52PM -0700, sjg@google.com wrote:
[..]
> Can we create a test of the boot_get_fdt() behaviour?
> 
> We have test_fit and test_vboot as possible examples, although perhaps
> a test that directly calls this function would be better?

On my list. Thanks.

> 
> Regards,
> Simon
> 
> 
> 
> Applied to u-boot-dm, thanks!
diff mbox series

Patch

diff --git a/common/image-fdt.c b/common/image-fdt.c
index 1817ce6bce30..c335e7e2f220 100644
--- a/common/image-fdt.c
+++ b/common/image-fdt.c
@@ -489,7 +489,7 @@  int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
 no_fdt:
 	ok_no_fdt = 1;
 error:
-	if (!select && ok_no_fdt) {
+	if (ok_no_fdt) {
 		debug("Continuing to boot without FDT\n");
 		return 0;
 	}