[U-Boot] [PATCH 1/2] libfdt: overlay: Fix missing symbols condition

[U-Boot] [PATCH 1/2] libfdt: overlay: Fix missing symbols condition

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

When there is no symbols section in the device tree,
overlay_fixup_phandles should return FDT_ERR_NOTFOUND instead of
FDT_ERR_BADOFFSET.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---

 lib/libfdt/fdt_overlay.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/libfdt/fdt_overlay.c b/lib/libfdt/fdt_overlay.c
index bb41404..4a9ba40 100644
--- a/lib/libfdt/fdt_overlay.c
+++ b/lib/libfdt/fdt_overlay.c
@@ -495,9 +495,9 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
 	if ((fixups_off < 0 && (fixups_off != -FDT_ERR_NOTFOUND)))
 		return fixups_off;
 
-	/* And base DTs without symbols */
+	/* But if we need to fixup phandles, symbols are required */
 	symbols_off = fdt_path_offset(fdt, "/__symbols__");
-	if ((symbols_off < 0 && (symbols_off != -FDT_ERR_NOTFOUND)))
+	if (symbols_off < 0)
 		return symbols_off;
 
 	fdt_for_each_property_offset(property, fdto, fixups_off) {
-- 
2.10.2

[U-Boot] [PATCH 2/2] cmd: fdt: Print error message when fdt application fails

[Stefan Agner]

From: Stefan Agner <stefan.agner@toradex.com>

There are lots of reason why a FDT application might fail, the
error code might give an indication. Let the error code translate
in a error string so users can try to understand what went wrong.

Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
---
Currently I get this when I apply a device tree overlay to a device
tree without symbols:

  Colibri iMX7 # fdt apply ${loadaddr}
  Colibri iMX7 # fdt print
  libfdt fdt_path_offset() returned FDT_ERR_BADMAGIC
  Colibri iMX7 #

With this change, the situation is a bit more indicative:

  Colibri iMX7 # fdt apply ${loadaddr}
  fdt_overlay_apply(): FDT_ERR_NOTFOUND
  Colibri iMX7 # fdt print
  libfdt fdt_path_offset() returned FDT_ERR_BADMAGIC
  Colibri iMX7 #

 cmd/fdt.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/cmd/fdt.c b/cmd/fdt.c
index 8bd345a..6883e75 100644
--- a/cmd/fdt.c
+++ b/cmd/fdt.c
@@ -642,6 +642,7 @@ static int do_fdt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	else if (strncmp(argv[1], "ap", 2) == 0) {
 		unsigned long addr;
 		struct fdt_header *blob;
+		int ret;
 
 		if (argc != 3)
 			return CMD_RET_USAGE;
@@ -654,8 +655,11 @@ static int do_fdt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (!fdt_valid(&blob))
 			return CMD_RET_FAILURE;
 
-		if (fdt_overlay_apply(working_fdt, blob))
+		ret = fdt_overlay_apply(working_fdt, blob);
+		if (ret) {
+			printf("fdt_overlay_apply(): %s\n", fdt_strerror(ret));
 			return CMD_RET_FAILURE;
+		}
 	}
 #endif
 	/* resize the fdt */
-- 
2.10.2

[U-Boot] [PATCH 2/2] cmd: fdt: Print error message when fdt application fails

[Maxime Ripard]

On Thu, Dec 15, 2016 at 03:03:27PM -0800, Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> There are lots of reason why a FDT application might fail, the
> error code might give an indication. Let the error code translate
> in a error string so users can try to understand what went wrong.
> 
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>

Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161216/2a109358/attachment.sig>

[U-Boot] [PATCH 2/2] cmd: fdt: Print error message when fdt application fails

[Simon Glass]

On 16 December 2016 at 02:33, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Thu, Dec 15, 2016 at 03:03:27PM -0800, Stefan Agner wrote:
>> From: Stefan Agner <stefan.agner@toradex.com>
>>
>> There are lots of reason why a FDT application might fail, the
>> error code might give an indication. Let the error code translate
>> in a error string so users can try to understand what went wrong.
>>
>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>
> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>

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

[U-Boot] [PATCH 2/2] cmd: fdt: Print error message when fdt application fails

[Simon Glass]

On 17 December 2016 at 15:48, Simon Glass <sjg@chromium.org> wrote:
> On 16 December 2016 at 02:33, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
>> On Thu, Dec 15, 2016 at 03:03:27PM -0800, Stefan Agner wrote:
>>> From: Stefan Agner <stefan.agner@toradex.com>
>>>
>>> There are lots of reason why a FDT application might fail, the
>>> error code might give an indication. Let the error code translate
>>> in a error string so users can try to understand what went wrong.
>>>
>>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>>
>> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>
> Acked-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-fdt, thanks!

[U-Boot] [PATCH 1/2] libfdt: overlay: Fix missing symbols condition

[Maxime Ripard]

On Thu, Dec 15, 2016 at 03:03:26PM -0800, Stefan Agner wrote:
> From: Stefan Agner <stefan.agner@toradex.com>
> 
> When there is no symbols section in the device tree,
> overlay_fixup_phandles should return FDT_ERR_NOTFOUND instead of
> FDT_ERR_BADOFFSET.
> 
> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
> ---
> 
>  lib/libfdt/fdt_overlay.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/libfdt/fdt_overlay.c b/lib/libfdt/fdt_overlay.c
> index bb41404..4a9ba40 100644
> --- a/lib/libfdt/fdt_overlay.c
> +++ b/lib/libfdt/fdt_overlay.c
> @@ -495,9 +495,9 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
>  	if ((fixups_off < 0 && (fixups_off != -FDT_ERR_NOTFOUND)))
>  		return fixups_off;
>  
> -	/* And base DTs without symbols */
> +	/* But if we need to fixup phandles, symbols are required */
>  	symbols_off = fdt_path_offset(fdt, "/__symbols__");
> -	if ((symbols_off < 0 && (symbols_off != -FDT_ERR_NOTFOUND)))
> +	if (symbols_off < 0)
>  		return symbols_off;

A base device tree doesn't need to have a symbols node. In the
(uncommon, true, but still real) case where you wouldn't have any
label in the base DT, __symbols__ wouldn't be there.

Now, that code would return an error only if there's such a reference
expressed in __fixups__, but no __symbols__ node that also has that
reference. This is definitely an error but not really a NOTFOUND. You
had an offset already, but this offset was bad. Hence the error.

Do you have a base DT and an overlay showing up the error you're
trying to fix? Either way, that should be discussed with upstream's
libfdt ml and maintainer, not (only) on U-Boot.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161216/57b1a34a/attachment.sig>

[U-Boot] [PATCH 1/2] libfdt: overlay: Fix missing symbols condition

[Stefan Agner]

On 2016-12-16 01:32, Maxime Ripard wrote:
> On Thu, Dec 15, 2016 at 03:03:26PM -0800, Stefan Agner wrote:
>> From: Stefan Agner <stefan.agner@toradex.com>
>>
>> When there is no symbols section in the device tree,
>> overlay_fixup_phandles should return FDT_ERR_NOTFOUND instead of
>> FDT_ERR_BADOFFSET.
>>
>> Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
>> ---
>>
>>  lib/libfdt/fdt_overlay.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/libfdt/fdt_overlay.c b/lib/libfdt/fdt_overlay.c
>> index bb41404..4a9ba40 100644
>> --- a/lib/libfdt/fdt_overlay.c
>> +++ b/lib/libfdt/fdt_overlay.c
>> @@ -495,9 +495,9 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
>>  	if ((fixups_off < 0 && (fixups_off != -FDT_ERR_NOTFOUND)))
>>  		return fixups_off;
>>
>> -	/* And base DTs without symbols */
>> +	/* But if we need to fixup phandles, symbols are required */
>>  	symbols_off = fdt_path_offset(fdt, "/__symbols__");
>> -	if ((symbols_off < 0 && (symbols_off != -FDT_ERR_NOTFOUND)))
>> +	if (symbols_off < 0)
>>  		return symbols_off;
> 
> A base device tree doesn't need to have a symbols node. In the
> (uncommon, true, but still real) case where you wouldn't have any
> label in the base DT, __symbols__ wouldn't be there.

Yeah I know, but in case there are fixup offsets found (just above),
symbols get essentially mandatory.

> 
> Now, that code would return an error only if there's such a reference
> expressed in __fixups__, but no __symbols__ node that also has that
> reference. This is definitely an error but not really a NOTFOUND. You
> had an offset already, but this offset was bad. Hence the error.

It probably would a new error code?

> 
> Do you have a base DT and an overlay showing up the error you're
> trying to fix? Either way, that should be discussed with upstream's
> libfdt ml and maintainer, not (only) on U-Boot.

Yeah my problem was that my base DT had no symbols and used labels.
Wasn't aware of the whole symbols problematic...

I actually just realized that this got fixed upstream:
https://git.kernel.org/cgit/utils/dtc/dtc.git/commit/libfdt/fdt_overlay.c?id=7d8ef6e1db9794f72805a0855f4f7f12fadd03d3

I guess we could backport that fix?

--
Stefan

[U-Boot] [PATCH 1/2] libfdt: overlay: Fix missing symbols condition

[Maxime Ripard]

On Sat, Dec 17, 2016 at 04:56:49PM -0800, Stefan Agner wrote:
> > Do you have a base DT and an overlay showing up the error you're
> > trying to fix? Either way, that should be discussed with upstream's
> > libfdt ml and maintainer, not (only) on U-Boot.
> 
> Yeah my problem was that my base DT had no symbols and used labels.
> Wasn't aware of the whole symbols problematic...
> 
> I actually just realized that this got fixed upstream:
> https://git.kernel.org/cgit/utils/dtc/dtc.git/commit/libfdt/fdt_overlay.c?id=7d8ef6e1db9794f72805a0855f4f7f12fadd03d3
> 
> I guess we could backport that fix?

Yep.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20161219/5f904f3a/attachment.sig>