[U-Boot] [PATCH] cmd: pxe: Use internal FDT if external one cannot be retrieved

[U-Boot] [PATCH] cmd: pxe: Use internal FDT if external one cannot be retrieved

[Anton Leontiev]

From: Anton Leontiev <aleontiev@elvees.com>

Original commit c61d94d86035 ("pxe: implement fdtdir extlinux.conf tag")
states, that if FDT file cannot be retrieved then FDT packaged in
firmware should be used.

If FDT file cannot be retrieved and it is specified explicitly using
FDT keyword then the label is skipped. If it cannot be found in
FDTDIR then internal FDT is tried first.

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

diff --git a/cmd/pxe.c b/cmd/pxe.c
index 2059975446..28390c114c 100644
--- a/cmd/pxe.c
+++ b/cmd/pxe.c
@@ -795,9 +795,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;
-- 
2.22.1

[U-Boot] [PATCH] cmd: pxe: Use internal FDT if external one cannot be retrieved

[Stephen Warren]

On 8/23/19 8:40 AM, Anton Leontiev wrote:
> From: Anton Leontiev <aleontiev@elvees.com>
> 
> Original commit c61d94d86035 ("pxe: implement fdtdir extlinux.conf tag")
> states, that if FDT file cannot be retrieved then FDT packaged in
> firmware should be used.

It's not meant to say that. I believe the part of the description you're 
referring to is:

     if no FDT file was loaded, and $fdtaddr is set:
       # This indicates an FDT packaged with firmware
       use the FDT at $fdtaddr

That wasn't meant to say anything about "if there was an error loading 
the FDT file", but rather is meant to mean "if no FDT file was loaded 
because extlinux.conf contained no fdt or fdtdir statement". Nothing 
there is intended to refer to errors loading a specified FDT file.

> If FDT file cannot be retrieved and it is specified explicitly using
> FDT keyword then the label is skipped. If it cannot be found in
> FDTDIR then internal FDT is tried first.

This makes the fdt/fdtdir keywords work differently. I don't think we 
want that.

What specific problem are you trying to solve?

And note that if we do change anything here, we should update the 
comment at around line 730, which describes the algorithm that's 
implemented.

[U-Boot] [PATCH] cmd: pxe: Use internal FDT if external one cannot be retrieved

[Anton Leontiev]

2019-08-26 at 18:59, Stephen Warren <swarren@wwwdotorg.org>:
> On 8/23/19 8:40 AM, Anton Leontiev wrote:
> > From: Anton Leontiev <aleontiev@elvees.com>
> >
> > Original commit c61d94d86035 ("pxe: implement fdtdir extlinux.conf tag")
> > states, that if FDT file cannot be retrieved then FDT packaged in
> > firmware should be used.
>
> It's not meant to say that. I believe the part of the description you're
> referring to is:
>
>      if no FDT file was loaded, and $fdtaddr is set:
>        # This indicates an FDT packaged with firmware
>        use the FDT at $fdtaddr
>
> That wasn't meant to say anything about "if there was an error loading
> the FDT file", but rather is meant to mean "if no FDT file was loaded
> because extlinux.conf contained no fdt or fdtdir statement". Nothing
> there is intended to refer to errors loading a specified FDT file.

Indeed, I read it strictly as "if no FDT file was loaded" regardless the reason.
Thank you for clarification.

> > If FDT file cannot be retrieved and it is specified explicitly using
> > FDT keyword then the label is skipped. If it cannot be found in
> > FDTDIR then internal FDT is tried first.
>
> This makes the fdt/fdtdir keywords work differently. I don't think we
> want that.

FDT will work as always. FDTDIR will be less strict. It doesn't
specify exact file to be loaded, that's why it should not fail if
there is no such file.

> What specific problem are you trying to solve?

We have a GNU/Linux distribution that use FDTDIR in its extlinux.conf
to support several boards. But some boards have FDT embedded in U-Boot
and it is't present in FDTDIR. In such configuration U-Boot fails to
boot an entry, despite no exact FDT is specified in it. Distribution
itself is designed to work on any board.

> And note that if we do change anything here, we should update the
> comment at around line 730, which describes the algorithm that's
> implemented.

Thank you, I'll update the comment.

[U-Boot] [PATCH] cmd: pxe: Use internal FDT if external one cannot be retrieved

[Stephen Warren]

On 8/29/19 5:20 AM, Anton Leontiev wrote:
> 2019-08-26 at 18:59, Stephen Warren <swarren@wwwdotorg.org>:
>> On 8/23/19 8:40 AM, Anton Leontiev wrote:
>>> From: Anton Leontiev <aleontiev@elvees.com>
>>>
>>> Original commit c61d94d86035 ("pxe: implement fdtdir extlinux.conf tag")
>>> states, that if FDT file cannot be retrieved then FDT packaged in
>>> firmware should be used.
>>
>> It's not meant to say that. I believe the part of the description you're
>> referring to is:
>>
>>       if no FDT file was loaded, and $fdtaddr is set:
>>         # This indicates an FDT packaged with firmware
>>         use the FDT at $fdtaddr
>>
>> That wasn't meant to say anything about "if there was an error loading
>> the FDT file", but rather is meant to mean "if no FDT file was loaded
>> because extlinux.conf contained no fdt or fdtdir statement". Nothing
>> there is intended to refer to errors loading a specified FDT file.
...
>> What specific problem are you trying to solve?
> 
> We have a GNU/Linux distribution that use FDTDIR in its extlinux.conf
> to support several boards. But some boards have FDT embedded in U-Boot
> and it is't present in FDTDIR. In such configuration U-Boot fails to
> boot an entry, despite no exact FDT is specified in it. Distribution
> itself is designed to work on any board.

I lookead at that referenced commit description in full and the code, 
and I believe what you want is for U-Boot to set fdt_addr to the 
location of the in-RAM DT, and leave fdt_addr_r unset. This will 
indicate to the pxe code that no FDT should be loaded when parsing 
extlinux.conf, but instead to use fdt_addr directly.

Does that work for you, or does it break some other script/use-case on 
this board?

[U-Boot] [PATCH] cmd: pxe: Use internal FDT if external one cannot be retrieved

[Anton Leontiev]

чт, 29 авг. 2019 г. в 23:35, Stephen Warren <swarren@wwwdotorg.org>:
> On 8/29/19 5:20 AM, Anton Leontiev wrote:
> > 2019-08-26 at 18:59, Stephen Warren <swarren@wwwdotorg.org>:
> > We have a GNU/Linux distribution that use FDTDIR in its extlinux.conf
> > to support several boards. But some boards have FDT embedded in U-Boot
> > and it is't present in FDTDIR. In such configuration U-Boot fails to
> > boot an entry, despite no exact FDT is specified in it. Distribution
> > itself is designed to work on any board.
>
> I lookead at that referenced commit description in full and the code,
> and I believe what you want is for U-Boot to set fdt_addr to the
> location of the in-RAM DT, and leave fdt_addr_r unset. This will
> indicate to the pxe code that no FDT should be loaded when parsing
> extlinux.conf, but instead to use fdt_addr directly.
>
> Does that work for you, or does it break some other script/use-case on
> this board?

Indeed, it's a possible option. However, if fdt_addr_r is not set,
user can't override embedded FDT using extlinux.conf. README.distro
says about fdt_addr_r: "This is mandatory even when fdt_addr is
provided, since extlinux.conf must always be able to provide a DTB
which overrides any copy provided by the HW."

So it should be considered as a workaround rather than a solution.

Best regards,
Anton Leontiev

[U-Boot] [PATCH] cmd: pxe: Use internal FDT if external one cannot be retrieved

[Stephen Warren]

On 8/31/19 1:52 PM, Anton Leontiev wrote:
> чт, 29 авг. 2019 г. в 23:35, Stephen Warren <swarren@wwwdotorg.org>:
>> On 8/29/19 5:20 AM, Anton Leontiev wrote:
>>> 2019-08-26 at 18:59, Stephen Warren <swarren@wwwdotorg.org>:
>>> We have a GNU/Linux distribution that use FDTDIR in its extlinux.conf
>>> to support several boards. But some boards have FDT embedded in U-Boot
>>> and it is't present in FDTDIR. In such configuration U-Boot fails to
>>> boot an entry, despite no exact FDT is specified in it. Distribution
>>> itself is designed to work on any board.
>>
>> I lookead at that referenced commit description in full and the code,
>> and I believe what you want is for U-Boot to set fdt_addr to the
>> location of the in-RAM DT, and leave fdt_addr_r unset. This will
>> indicate to the pxe code that no FDT should be loaded when parsing
>> extlinux.conf, but instead to use fdt_addr directly.
>>
>> Does that work for you, or does it break some other script/use-case on
>> this board?
> 
> Indeed, it's a possible option. However, if fdt_addr_r is not set,
> user can't override embedded FDT using extlinux.conf. README.distro
> says about fdt_addr_r: "This is mandatory even when fdt_addr is
> provided, since extlinux.conf must always be able to provide a DTB
> which overrides any copy provided by the HW."
> 
> So it should be considered as a workaround rather than a solution.

OK, I guess that makes sense.

I suppose it's not reasonable to ask that extlinux.conf doesn't contain 
an FDTDIR statement in the case where the specific board has a built-in 
DT, since the extlinux.conf content is supposed to be generic, and not 
tailored to the current HW.

[U-Boot] [PATCH v2] cmd: pxe: Use internal FDT if retrieving from FDTDIR fails

[Anton Leontiev]

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.
 	 *
 	 * 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;
-- 
2.23.0

[U-Boot] [PATCH v2] cmd: pxe: Use internal FDT if retrieving from FDTDIR fails

[Stephen Warren]

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;
> 

[U-Boot] [PATCH v2] cmd: pxe: Use internal FDT if retrieving from FDTDIR fails

[Anton Leontiev]

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

[U-Boot] [PATCH v2] cmd: pxe: Use internal FDT if retrieving from FDTDIR fails

[Anton Leontiev]

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

[U-Boot] [PATCH v2] cmd: pxe: Use internal FDT if retrieving from FDTDIR fails

[Tom Rini]

On Tue, Sep 03, 2019 at 10:52:24AM +0300, 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>

Sorry for the delay, applied to u-boot/next, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201202/dee40257/attachment.sig>