[U-Boot,v2] cmd: pxe: Use internal FDT if retrieving from FDTDIR fails
diff mbox series

Message ID 20190903075224.9404-1-scileont@gmail.com
State New
Delegated to: Tom Rini
Headers show
Series
  • [U-Boot,v2] cmd: pxe: Use internal FDT if retrieving from FDTDIR fails
Related show

Commit Message

Anton Leontiev Sept. 3, 2019, 7:52 a.m. UTC
From: Anton Leontiev <aleontiev@elvees.com>

As FDTDIR label doesn't specify exact file to be loaded, it should
not fail if no file exists in the directory. In this case try to boot
with internal FDT if it exists.

Signed-off-by: Anton Leontiev <aleontiev@elvees.com>
---
 cmd/pxe.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Comments

Stephen Warren Sept. 3, 2019, 4:18 p.m. UTC | #1
On 9/3/19 1:52 AM, Anton Leontiev wrote:
> From: Anton Leontiev <aleontiev@elvees.com>
> 
> As FDTDIR label doesn't specify exact file to be loaded, it should
> not fail if no file exists in the directory. In this case try to boot
> with internal FDT if it exists.
> 
> Signed-off-by: Anton Leontiev <aleontiev@elvees.com>
> ---
>   cmd/pxe.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/cmd/pxe.c b/cmd/pxe.c
> index 2059975446..1eec6d29bf 100644
> --- a/cmd/pxe.c
> +++ b/cmd/pxe.c
> @@ -727,11 +727,14 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
>   
>   	/*
>   	 * fdt usage is optional:
> -	 * It handles the following scenarios. All scenarios are exclusive
> +	 * It handles the following scenarios.
>   	 *
> -	 * Scenario 1: If fdt_addr_r specified and "fdt" label is defined in
> -	 * pxe file, retrieve fdt blob from server. Pass fdt_addr_r to bootm,
> -	 * and adjust argc appropriately.
> +	 * Scenario 1: If fdt_addr_r specified and "fdt" or "fdtdir" label is
> +	 * defined in pxe file, retrieve fdt blob from server. Pass fdt_addr_r to
> +	 * bootm, and adjust argc appropriately.
> +	 *
> +	 * If retrieve fails and no exact fdt blob is specified in pxe file with
> +	 * "fdt" label, try Scenario 2.

Is it possible/sensible to distinguish between "file not found" and 
"error during retrieval"? "File not found" indicates the case you care 
about, and continuing to use a built-in DT makes sense here. "Error 
during retrieval" indicates that the file was found, and hence really 
should be use, yet couldn't be due to download failure. In this case, I 
wonder if we shouldn't outright fail this boot option, just like the 
existing code does?

But either way, I suppose this patch is reasonable.

>   	 * Scenario 2: If there is an fdt_addr specified, pass it along to
>   	 * bootm, and adjust argc appropriately.
> @@ -795,9 +798,13 @@ static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
>   			int err = get_relfile_envaddr(cmdtp, fdtfile, "fdt_addr_r");
>   			free(fdtfilefree);
>   			if (err < 0) {
> -				printf("Skipping %s for failure retrieving fdt\n",
> -						label->name);
> -				goto cleanup;
> +				bootm_argv[3] = NULL;
> +
> +				if (label->fdt) {
> +					printf("Skipping %s for failure retrieving FDT\n",
> +					       label->name);
> +					goto cleanup;
> +				}
>   			}
>   		} else {
>   			bootm_argv[3] = NULL;
>
Anton Leontiev Sept. 17, 2019, 6:07 a.m. UTC | #2
2019-09-03 19:18, Stephen Warren <swarren@wwwdotorg.org>:
> Is it possible/sensible to distinguish between "file not found" and
> "error during retrieval"? "File not found" indicates the case you care
> about, and continuing to use a built-in DT makes sense here. "Error
> during retrieval" indicates that the file was found, and hence really
> should be use, yet couldn't be due to download failure. In this case, I
> wonder if we shouldn't outright fail this boot option, just like the
> existing code does?
>
> But either way, I suppose this patch is reasonable.

As I see, for example from do_get_ext2(), currently it is not possible
to distinguish between "file not found" and "error during retrieval".
It seems possible to update all do_get_*() functions to check file
existence before retrieving, but it will require extensive testing and
I prefer this to be changed independently to current patch.

Best regards,
Anton Leontiev Oct. 15, 2019, 5:59 a.m. UTC | #3
2019-09-03 10:52, Anton Leontiev <scileont@gmail.com>:
> From: Anton Leontiev <aleontiev@elvees.com>
>
> As FDTDIR label doesn't specify exact file to be loaded, it should
> not fail if no file exists in the directory. In this case try to boot
> with internal FDT if it exists.
>
> Signed-off-by: Anton Leontiev <aleontiev@elvees.com>

Tom, could you take a look at my patch during the merge window? Or
should I pass it through some custodian?

Best regards,
Anton Leontiev

Patch
diff mbox series

diff --git a/cmd/pxe.c b/cmd/pxe.c
index 2059975446..1eec6d29bf 100644
--- a/cmd/pxe.c
+++ b/cmd/pxe.c
@@ -727,11 +727,14 @@  static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
 
 	/*
 	 * fdt usage is optional:
-	 * It handles the following scenarios. All scenarios are exclusive
+	 * It handles the following scenarios.
 	 *
-	 * Scenario 1: If fdt_addr_r specified and "fdt" label is defined in
-	 * pxe file, retrieve fdt blob from server. Pass fdt_addr_r to bootm,
-	 * and adjust argc appropriately.
+	 * Scenario 1: If fdt_addr_r specified and "fdt" or "fdtdir" label is
+	 * defined in pxe file, retrieve fdt blob from server. Pass fdt_addr_r to
+	 * bootm, and adjust argc appropriately.
+	 *
+	 * If retrieve fails and no exact fdt blob is specified in pxe file with
+	 * "fdt" label, try Scenario 2.
 	 *
 	 * Scenario 2: If there is an fdt_addr specified, pass it along to
 	 * bootm, and adjust argc appropriately.
@@ -795,9 +798,13 @@  static int label_boot(cmd_tbl_t *cmdtp, struct pxe_label *label)
 			int err = get_relfile_envaddr(cmdtp, fdtfile, "fdt_addr_r");
 			free(fdtfilefree);
 			if (err < 0) {
-				printf("Skipping %s for failure retrieving fdt\n",
-						label->name);
-				goto cleanup;
+				bootm_argv[3] = NULL;
+
+				if (label->fdt) {
+					printf("Skipping %s for failure retrieving FDT\n",
+					       label->name);
+					goto cleanup;
+				}
 			}
 		} else {
 			bootm_argv[3] = NULL;