[PATCH] ARM: dts: imx: Pass an empty 'chosen' node

[PATCH] ARM: dts: imx: Pass an empty 'chosen' node

[Fabio Estevam]

Commit 7f107887d199 ("ARM: dts: imx: Remove skeleton.dtsi") causes boot
issues when the bootloader does not create a 'chosen' node if such node
is not present in the dtb.

The reason for the boot failure is well explained by Javier Martinez
Canillas: "the decompressor relies on a pre-existing chosen node to be
available to insert the command line and merge other ATAGS info."

, so pass an empty 'chosen' node to fix the boot problem.

This issue has been seen in the kernelci reports with Barebox as
bootloader.

Fixes: 7f107887d199 ("ARM: dts: imx: Remove skeleton.dtsi")
Reported-by: kernelci.org bot <bot@kernelci.org>
Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
---
 arch/arm/boot/dts/imx1.dtsi    | 1 +
 arch/arm/boot/dts/imx23.dtsi   | 1 +
 arch/arm/boot/dts/imx25.dtsi   | 1 +
 arch/arm/boot/dts/imx27.dtsi   | 1 +
 arch/arm/boot/dts/imx28.dtsi   | 1 +
 arch/arm/boot/dts/imx31.dtsi   | 1 +
 arch/arm/boot/dts/imx35.dtsi   | 1 +
 arch/arm/boot/dts/imx50.dtsi   | 1 +
 arch/arm/boot/dts/imx51.dtsi   | 1 +
 arch/arm/boot/dts/imx53.dtsi   | 1 +
 arch/arm/boot/dts/imx6qdl.dtsi | 1 +
 arch/arm/boot/dts/imx6sl.dtsi  | 1 +
 arch/arm/boot/dts/imx6sx.dtsi  | 1 +
 arch/arm/boot/dts/imx6ul.dtsi  | 1 +
 arch/arm/boot/dts/imx7s.dtsi   | 1 +
 15 files changed, 15 insertions(+)

diff --git a/arch/arm/boot/dts/imx1.dtsi b/arch/arm/boot/dts/imx1.dtsi
index b792eee..a9581b5 100644
--- a/arch/arm/boot/dts/imx1.dtsi
+++ b/arch/arm/boot/dts/imx1.dtsi
@@ -18,6 +18,7 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
 
 	aliases {
 		gpio0 = &gpio1;
diff --git a/arch/arm/boot/dts/imx23.dtsi b/arch/arm/boot/dts/imx23.dtsi
index ac2a9da..d00bc55 100644
--- a/arch/arm/boot/dts/imx23.dtsi
+++ b/arch/arm/boot/dts/imx23.dtsi
@@ -14,6 +14,7 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
 
 	interrupt-parent = <&icoll>;
 
diff --git a/arch/arm/boot/dts/imx25.dtsi b/arch/arm/boot/dts/imx25.dtsi
index 831d09a..a0348b0 100644
--- a/arch/arm/boot/dts/imx25.dtsi
+++ b/arch/arm/boot/dts/imx25.dtsi
@@ -14,6 +14,7 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
 
 	aliases {
 		ethernet0 = &fec;
diff --git a/arch/arm/boot/dts/imx27.dtsi b/arch/arm/boot/dts/imx27.dtsi
index 9d8b596..84652eb 100644
--- a/arch/arm/boot/dts/imx27.dtsi
+++ b/arch/arm/boot/dts/imx27.dtsi
@@ -19,6 +19,7 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
 
 	aliases {
 		ethernet0 = &fec;
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index 3aabf65..6635213 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -15,6 +15,7 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
 
 	interrupt-parent = <&icoll>;
 
diff --git a/arch/arm/boot/dts/imx31.dtsi b/arch/arm/boot/dts/imx31.dtsi
index 685916e..8a1ea22 100644
--- a/arch/arm/boot/dts/imx31.dtsi
+++ b/arch/arm/boot/dts/imx31.dtsi
@@ -12,6 +12,7 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
 
 	aliases {
 		serial0 = &uart1;
diff --git a/arch/arm/boot/dts/imx35.dtsi b/arch/arm/boot/dts/imx35.dtsi
index 9f40e62..4c67438 100644
--- a/arch/arm/boot/dts/imx35.dtsi
+++ b/arch/arm/boot/dts/imx35.dtsi
@@ -13,6 +13,7 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
 
 	aliases {
 		ethernet0 = &fec;
diff --git a/arch/arm/boot/dts/imx50.dtsi b/arch/arm/boot/dts/imx50.dtsi
index fe0221e..6e947bd 100644
--- a/arch/arm/boot/dts/imx50.dtsi
+++ b/arch/arm/boot/dts/imx50.dtsi
@@ -17,6 +17,7 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
 
 	aliases {
 		ethernet0 = &fec;
diff --git a/arch/arm/boot/dts/imx51.dtsi b/arch/arm/boot/dts/imx51.dtsi
index 33526ca..d1f8b93 100644
--- a/arch/arm/boot/dts/imx51.dtsi
+++ b/arch/arm/boot/dts/imx51.dtsi
@@ -19,6 +19,7 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
 
 	aliases {
 		ethernet0 = &fec;
diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi
index ca51dc0..ea308b0 100644
--- a/arch/arm/boot/dts/imx53.dtsi
+++ b/arch/arm/boot/dts/imx53.dtsi
@@ -19,6 +19,7 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
 
 	aliases {
 		ethernet0 = &fec;
diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index 53e6e63..c9efd45 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -16,6 +16,7 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
 
 	aliases {
 		ethernet0 = &fec;
diff --git a/arch/arm/boot/dts/imx6sl.dtsi b/arch/arm/boot/dts/imx6sl.dtsi
index 4fd6de2..4cd16f2 100644
--- a/arch/arm/boot/dts/imx6sl.dtsi
+++ b/arch/arm/boot/dts/imx6sl.dtsi
@@ -14,6 +14,7 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
 
 	aliases {
 		ethernet0 = &fec;
diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
index 076a30f..9bb1563 100644
--- a/arch/arm/boot/dts/imx6sx.dtsi
+++ b/arch/arm/boot/dts/imx6sx.dtsi
@@ -15,6 +15,7 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
 
 	aliases {
 		can0 = &flexcan1;
diff --git a/arch/arm/boot/dts/imx6ul.dtsi b/arch/arm/boot/dts/imx6ul.dtsi
index 39845a7..a910fa3 100644
--- a/arch/arm/boot/dts/imx6ul.dtsi
+++ b/arch/arm/boot/dts/imx6ul.dtsi
@@ -15,6 +15,7 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
 
 	aliases {
 		ethernet0 = &fec1;
diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index 8ff2cbdd..6408b9d 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -50,6 +50,7 @@
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
+	chosen {};
 
 	aliases {
 		gpio0 = &gpio1;
-- 
1.9.1

[PATCH] ARM: dts: imx: Pass an empty 'chosen' node

[Javier Martinez Canillas]

Hello Fabio,

On 12/19/2016 12:28 PM, Fabio Estevam wrote:
> Commit 7f107887d199 ("ARM: dts: imx: Remove skeleton.dtsi") causes boot
> issues when the bootloader does not create a 'chosen' node if such node
> is not present in the dtb.
> 
> The reason for the boot failure is well explained by Javier Martinez
> Canillas: "the decompressor relies on a pre-existing chosen node to be
> available to insert the command line and merge other ATAGS info."
> 
> , so pass an empty 'chosen' node to fix the boot problem.
> 
> This issue has been seen in the kernelci reports with Barebox as
> bootloader.
> 
> Fixes: 7f107887d199 ("ARM: dts: imx: Remove skeleton.dtsi")
> Reported-by: kernelci.org bot <bot@kernelci.org>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>
> ---

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[PATCH] ARM: dts: imx: Pass an empty 'chosen' node

[Uwe Kleine-König]

On Mon, Dec 19, 2016 at 01:28:18PM -0200, Fabio Estevam wrote:
> Commit 7f107887d199 ("ARM: dts: imx: Remove skeleton.dtsi") causes boot
> issues when the bootloader does not create a 'chosen' node if such node
> is not present in the dtb.
> 
> The reason for the boot failure is well explained by Javier Martinez
> Canillas: "the decompressor relies on a pre-existing chosen node to be
> available to insert the command line and merge other ATAGS info."
> 
> , so pass an empty 'chosen' node to fix the boot problem.
> 
> This issue has been seen in the kernelci reports with Barebox as
> bootloader.
> 
> Fixes: 7f107887d199 ("ARM: dts: imx: Remove skeleton.dtsi")
> Reported-by: kernelci.org bot <bot@kernelci.org>
> Signed-off-by: Fabio Estevam <fabio.estevam@nxp.com>

There is also a patch on the barebox list that makes barebox fix a dt
accordingly. It seems it's not in the repository though.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] ARM: dts: imx: Pass an empty 'chosen' node

[Uwe Kleine-König]

Hello,

On Mon, Dec 19, 2016 at 01:28:18PM -0200, Fabio Estevam wrote:
> Commit 7f107887d199 ("ARM: dts: imx: Remove skeleton.dtsi") causes boot
> issues when the bootloader does not create a 'chosen' node if such node
> is not present in the dtb.
> 
> The reason for the boot failure is well explained by Javier Martinez
> Canillas: "the decompressor relies on a pre-existing chosen node to be
> available to insert the command line and merge other ATAGS info."
> 
> , so pass an empty 'chosen' node to fix the boot problem.
> 
> This issue has been seen in the kernelci reports with Barebox as
> bootloader.

wouldn't it be better to fix the decompressor code to eventually create
the /chosen node when it doesn't exist?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] ARM: dts: imx: Pass an empty 'chosen' node

[Fabio Estevam]

Hi Uwe,

On Mon, Dec 19, 2016 at 4:41 PM, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:

> wouldn't it be better to fix the decompressor code to eventually create
> the /chosen node when it doesn't exist?

Thanks for the suggestion.

I don't have a Barebox setup handy, but would the following change fix
it as you propose?

--- a/arch/arm/boot/compressed/atags_to_fdt.c
+++ b/arch/arm/boot/compressed/atags_to_fdt.c
@@ -66,13 +66,21 @@ static uint32_t get_cell_size(const void *fdt)
        return cell_size;
 }

-static void merge_fdt_bootargs(void *fdt, const char *fdt_cmdline)
+static int merge_fdt_bootargs(void *fdt, const char *fdt_cmdline)
 {
        char cmdline[COMMAND_LINE_SIZE];
        const char *fdt_bootargs;
        char *ptr = cmdline;
+       int chosen_off;
        int len = 0;

+       /* find or add chosen node */
+       chosen_off = fdt_path_offset(fdt, "/chosen");
+       if (chosen_off == -FDT_ERR_NOTFOUND)
+               chosen_off = fdt_add_subnode(fdt, 0, "chosen");
+       if (chosen_off < 0)
+               return chosen_off;
+
        /* copy the fdt command line into the buffer */
        fdt_bootargs = getprop(fdt, "/chosen", "bootargs", &len);
        if (fdt_bootargs)

[PATCH] ARM: dts: imx: Pass an empty 'chosen' node

[Javier Martinez Canillas]

[adding Pali who first reported this issue]

Hello Pali,

On 12/19/2016 06:30 PM, Fabio Estevam wrote:
> Hi Uwe,
> 
> On Mon, Dec 19, 2016 at 4:41 PM, Uwe Kleine-K?nig
> <u.kleine-koenig@pengutronix.de> wrote:
> 
>> wouldn't it be better to fix the decompressor code to eventually create
>> the /chosen node when it doesn't exist?
>
> Thanks for the suggestion.
> 
> I don't have a Barebox setup handy, but would the following change fix
> it as you propose?
>

Could you test Fabio's patch with NoLo to see if it fixes your issue?

I also think it makes more sense for Linux to create the "chosen" node
if not present since this is a requirement due a Linux implementation
detail.

Forcing to have a "chosen" node in dts leaks this implementation detail
since the ePAPR document says that the "chosen" node should be optional.
 
> --- a/arch/arm/boot/compressed/atags_to_fdt.c
> +++ b/arch/arm/boot/compressed/atags_to_fdt.c
> @@ -66,13 +66,21 @@ static uint32_t get_cell_size(const void *fdt)
>         return cell_size;
>  }
> 
> -static void merge_fdt_bootargs(void *fdt, const char *fdt_cmdline)
> +static int merge_fdt_bootargs(void *fdt, const char *fdt_cmdline)
>  {
>         char cmdline[COMMAND_LINE_SIZE];
>         const char *fdt_bootargs;
>         char *ptr = cmdline;
> +       int chosen_off;
>         int len = 0;
> 
> +       /* find or add chosen node */
> +       chosen_off = fdt_path_offset(fdt, "/chosen");
> +       if (chosen_off == -FDT_ERR_NOTFOUND)
> +               chosen_off = fdt_add_subnode(fdt, 0, "chosen");
> +       if (chosen_off < 0)
> +               return chosen_off;
> +
>         /* copy the fdt command line into the buffer */
>         fdt_bootargs = getprop(fdt, "/chosen", "bootargs", &len);
>         if (fdt_bootargs)
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

[PATCH] ARM: dts: imx: Pass an empty 'chosen' node

[Uwe Kleine-König]

On Mon, Dec 19, 2016 at 07:30:25PM -0200, Fabio Estevam wrote:
> Hi Uwe,
> 
> On Mon, Dec 19, 2016 at 4:41 PM, Uwe Kleine-K?nig
> <u.kleine-koenig@pengutronix.de> wrote:
> 
> > wouldn't it be better to fix the decompressor code to eventually create
> > the /chosen node when it doesn't exist?
> 
> Thanks for the suggestion.
> 
> I don't have a Barebox setup handy, but would the following change fix
> it as you propose?
> 
> --- a/arch/arm/boot/compressed/atags_to_fdt.c
> +++ b/arch/arm/boot/compressed/atags_to_fdt.c
> @@ -66,13 +66,21 @@ static uint32_t get_cell_size(const void *fdt)
>         return cell_size;
>  }
> 
> -static void merge_fdt_bootargs(void *fdt, const char *fdt_cmdline)
> +static int merge_fdt_bootargs(void *fdt, const char *fdt_cmdline)
>  {
>         char cmdline[COMMAND_LINE_SIZE];
>         const char *fdt_bootargs;
>         char *ptr = cmdline;
> +       int chosen_off;
>         int len = 0;
> 
> +       /* find or add chosen node */
> +       chosen_off = fdt_path_offset(fdt, "/chosen");
> +       if (chosen_off == -FDT_ERR_NOTFOUND)
> +               chosen_off = fdt_add_subnode(fdt, 0, "chosen");
> +       if (chosen_off < 0)
> +               return chosen_off;

I would have done:

	ret = fdt_add_subnode(fdt, 0, "chosen")
	if (ret < 0 && ret != -FDT_ERR_EXISTS)
		return -ESOMETHING;

but I don't have the right machine@hand. Will check tomorrow.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] ARM: dts: imx: Pass an empty 'chosen' node

[Pali Rohár]

Hi!

On Monday 19 December 2016 22:44:40 Javier Martinez Canillas wrote:
> [adding Pali who first reported this issue]
> 
> Hello Pali,
> 
> On 12/19/2016 06:30 PM, Fabio Estevam wrote:
> > Hi Uwe,
> > 
> > On Mon, Dec 19, 2016 at 4:41 PM, Uwe Kleine-K?nig
> > 
> > <u.kleine-koenig@pengutronix.de> wrote:
> >> wouldn't it be better to fix the decompressor code to eventually
> >> create the /chosen node when it doesn't exist?
> > 
> > Thanks for the suggestion.
> > 
> > I don't have a Barebox setup handy, but would the following change
> > fix it as you propose?
> 
> Could you test Fabio's patch with NoLo to see if it fixes your issue?
> 
> I also think it makes more sense for Linux to create the "chosen"
> node if not present since this is a requirement due a Linux
> implementation detail.
> 
> Forcing to have a "chosen" node in dts leaks this implementation
> detail since the ePAPR document says that the "chosen" node should
> be optional.
> 
> > --- a/arch/arm/boot/compressed/atags_to_fdt.c
> > +++ b/arch/arm/boot/compressed/atags_to_fdt.c
> > @@ -66,13 +66,21 @@ static uint32_t get_cell_size(const void *fdt)
> > 
> >         return cell_size;
> >  
> >  }
> > 
> > -static void merge_fdt_bootargs(void *fdt, const char *fdt_cmdline)
> > +static int merge_fdt_bootargs(void *fdt, const char *fdt_cmdline)
> > 
> >  {
> >  
> >         char cmdline[COMMAND_LINE_SIZE];
> >         const char *fdt_bootargs;
> >         char *ptr = cmdline;
> > 
> > +       int chosen_off;
> > 
> >         int len = 0;
> > 
> > +       /* find or add chosen node */
> > +       chosen_off = fdt_path_offset(fdt, "/chosen");
> > +       if (chosen_off == -FDT_ERR_NOTFOUND)
> > +               chosen_off = fdt_add_subnode(fdt, 0, "chosen");
> > +       if (chosen_off < 0)
> > +               return chosen_off;
> > +
> > 
> >         /* copy the fdt command line into the buffer */
> >         fdt_bootargs = getprop(fdt, "/chosen", "bootargs", &len);
> >         if (fdt_bootargs)

This patch looks wrong. The next if condition is there to read 
/chosen/boootargs entry because this function merge_fdt_bootargs() is 
merging them together. And for reading bootargs you do not need to 
create /chosen node, if it does not exists above getprop() returns NULL 
and next if correctly handles it (= no bootargs for merging).

But setting /chosen/bootargs is also outside of this function in 
atags_to_fdt() where merge_fdt_bootargs() does not have to be called (in 
case do_extend_cmdline is not truth).

So this patch does not fix real problem, 'chosen' node does not have to 
exist even after applying this patch.

I'm for fixing this uncompressor code so 'chosen' stay optional (as 
written in that ePAPR document) and Linux kernel uncompressor correctly 
creates it. But this patch is not doing it (correctly).

-- 
Pali Roh?r
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161222/ebc24098/attachment-0001.sig>