[PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only

[PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only

[Leif Lindholm]

drivers/of/fdt.c contains a workaround for a missing memory type
entry on longtrail firmware. Make that quirk PPC32 only, and while
at it - fix up the .dts files in the tree currently working only
because of that quirk.

Cc: devicetree@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
Cc: Mark Rutland <mark.rutland@arm.com>

Leif Lindholm (3):
  arm: dts: add device_type="memory" for ste-ccu8540
  mips: dts: add device_type="memory" where missing
  of: Handle memory@0 node on PPC32 only

 arch/arm/boot/dts/ste-ccu8540.dts     |    1 +
 arch/mips/lantiq/dts/easy50712.dts    |    1 +
 arch/mips/ralink/dts/mt7620a_eval.dts |    1 +
 arch/mips/ralink/dts/rt2880_eval.dts  |    1 +
 arch/mips/ralink/dts/rt3052_eval.dts  |    1 +
 arch/mips/ralink/dts/rt3883_eval.dts  |    1 +
 drivers/of/fdt.c                      |    7 ++++++-
 7 files changed, 12 insertions(+), 1 deletion(-)

-- 
1.7.10.4

[PATCH 3/3] of: Handle memory@0 node on PPC32 only

[Leif Lindholm]

In order to deal with an firmware bug on a specific ppc32 platform
(longtrail), early_init_dt_scan_memory() looks for a node called
memory@0 on all platforms. Restrict this quirk to ppc32 kernels only.

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/of/fdt.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index fa16a91..7368472 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -887,14 +887,19 @@ int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
 
 	/* We are scanning "memory" nodes only */
 	if (type == NULL) {
+#ifdef CONFIG_PPC32
 		/*
 		 * The longtrail doesn't have a device_type on the
 		 * /memory node, so look for the node called /memory@0.
 		 */
 		if (depth != 1 || strcmp(uname, "memory@0") != 0)
 			return 0;
-	} else if (strcmp(type, "memory") != 0)
+#else
+		return 0;
+#endif
+	} else if (strcmp(type, "memory") != 0) {
 		return 0;
+	}
 
 	reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
 	if (reg == NULL)
-- 
1.7.10.4

Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only

[Rob Herring]

On Thu, Apr 17, 2014 at 12:41 PM, Leif Lindholm
<leif.lindholm@linaro.org> wrote:
> drivers/of/fdt.c contains a workaround for a missing memory type
> entry on longtrail firmware. Make that quirk PPC32 only, and while
> at it - fix up the .dts files in the tree currently working only
> because of that quirk.

But why do you need this?

>
> Cc: devicetree@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-kernel@vger.kernel.org
> Cc: Mark Rutland <mark.rutland@arm.com>
>
> Leif Lindholm (3):
>   arm: dts: add device_type="memory" for ste-ccu8540
>   mips: dts: add device_type="memory" where missing
>   of: Handle memory@0 node on PPC32 only
>
>  arch/arm/boot/dts/ste-ccu8540.dts     |    1 +
>  arch/mips/lantiq/dts/easy50712.dts    |    1 +
>  arch/mips/ralink/dts/mt7620a_eval.dts |    1 +
>  arch/mips/ralink/dts/rt2880_eval.dts  |    1 +
>  arch/mips/ralink/dts/rt3052_eval.dts  |    1 +
>  arch/mips/ralink/dts/rt3883_eval.dts  |    1 +

I'm not worried about these MIPS dts files because they are all
built-in, but you are breaking compatibility with old dtbs for this
ARM board.

Rob

>  drivers/of/fdt.c                      |    7 ++++++-
>  7 files changed, 12 insertions(+), 1 deletion(-)
>
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] of: Handle memory@0 node on PPC32 only

[Geert Uytterhoeven]

Hi Leif,

On Thu, Apr 17, 2014 at 7:42 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> In order to deal with an firmware bug on a specific ppc32 platform
> (longtrail), early_init_dt_scan_memory() looks for a node called
> memory@0 on all platforms. Restrict this quirk to ppc32 kernels only.

This breaks backwards compatibilty with old DTSes (at least on ARM/MIPS,
where you added the missing property in patches 1 and 2 of the series)?

For the Longtrail, I don't care much anymore, as mine died in 2004.
AFAIK, there have never been many users anyway.

> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Grant Likely <grant.likely@linaro.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/of/fdt.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index fa16a91..7368472 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -887,14 +887,19 @@ int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
>
>         /* We are scanning "memory" nodes only */
>         if (type == NULL) {
> +#ifdef CONFIG_PPC32
>                 /*
>                  * The longtrail doesn't have a device_type on the
>                  * /memory node, so look for the node called /memory@0.
>                  */
>                 if (depth != 1 || strcmp(uname, "memory@0") != 0)
>                         return 0;
> -       } else if (strcmp(type, "memory") != 0)
> +#else
> +               return 0;
> +#endif
> +       } else if (strcmp(type, "memory") != 0) {
>                 return 0;
> +       }
>
>         reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
>         if (reg == NULL)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only

[Leif Lindholm]

On Thu, Apr 17, 2014 at 07:43:13PM -0500, Rob Herring wrote:
> On Thu, Apr 17, 2014 at 12:41 PM, Leif Lindholm
> <leif.lindholm@linaro.org> wrote:
> > drivers/of/fdt.c contains a workaround for a missing memory type
> > entry on longtrail firmware. Make that quirk PPC32 only, and while
> > at it - fix up the .dts files in the tree currently working only
> > because of that quirk.
> 
> But why do you need this?

Apart from the current code permitting recreating a 15+ year old
firmware bug into completely new platform ports?

Because the UEFI stub for arm/arm64 needs to delete all of the "memory"
nodes from the DT. And it would be nice to at least not have to compile
the "and also delete anything called memory@0" into the arm64 image. Or
any image not including support for affected platforms.

> >  arch/arm/boot/dts/ste-ccu8540.dts     |    1 +
> >  arch/mips/lantiq/dts/easy50712.dts    |    1 +
> >  arch/mips/ralink/dts/mt7620a_eval.dts |    1 +
> >  arch/mips/ralink/dts/rt2880_eval.dts  |    1 +
> >  arch/mips/ralink/dts/rt3052_eval.dts  |    1 +
> >  arch/mips/ralink/dts/rt3883_eval.dts  |    1 +
> 
> I'm not worried about these MIPS dts files because they are all
> built-in, but you are breaking compatibility with old dtbs for this
> ARM board.

Yeah, sorry. Sending out a v2 of part 3 shortly.

/
    Leif

Re: [PATCH 3/3] of: Handle memory@0 node on PPC32 only

[Leif Lindholm]

Hi Geert,

On Fri, Apr 18, 2014 at 10:04:15AM +0200, Geert Uytterhoeven wrote:
> On Thu, Apr 17, 2014 at 7:42 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > In order to deal with an firmware bug on a specific ppc32 platform
> > (longtrail), early_init_dt_scan_memory() looks for a node called
> > memory@0 on all platforms. Restrict this quirk to ppc32 kernels only.
> 
> This breaks backwards compatibilty with old DTSes (at least on ARM/MIPS,
> where you added the missing property in patches 1 and 2 of the series)?

As Rob said in response to 0/3, the MIPSs would likely not be affected,
since they embed the DT.

> For the Longtrail, I don't care much anymore, as mine died in 2004.
> AFAIK, there have never been many users anyway.

There are still a few mentions of it under arch/powerpc/, so I wouldn't
want to be the one to kill it off...

How about the below v2 3/3 to address the ARM platform?

Regards,

Leif

>From 6fa0b837ad71780334eb97d63c507165b6c57add Mon Sep 17 00:00:00 2001
From: Leif Lindholm <leif.lindholm@linaro.org>
Date: Thu, 17 Apr 2014 14:24:47 +0100
Subject: [PATCH] of: arm: powerpc: Restrict memory@0 node handling to
 affected platforms

In order to deal with a firmware bug on a specific ppc32 platform
(longtrail), early_init_dt_scan_memory() looks for a node called
memory@0 on all platforms, for all nodes lacking a device_type.
Restrict this quirk to ppc32 and the arm mach-ux500 platforms (one of
which has depended on this special handling).

Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
---
 arch/arm/mach-ux500/Kconfig |    1 +
 arch/powerpc/Kconfig        |    1 +
 drivers/of/Kconfig          |    3 +++
 drivers/of/fdt.c            |   10 +++++++++-
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-ux500/Kconfig b/arch/arm/mach-ux500/Kconfig
index b41a42d..e6b0c3b 100644
--- a/arch/arm/mach-ux500/Kconfig
+++ b/arch/arm/mach-ux500/Kconfig
@@ -13,6 +13,7 @@ config ARCH_U8500
 	select CLKSRC_NOMADIK_MTU
 	select HAVE_ARM_SCU if SMP
 	select HAVE_ARM_TWD if SMP
+	select OF_MEMORY_AT_0_QUIRK
 	select PINCTRL
 	select PINCTRL_ABX500
 	select PINCTRL_NOMADIK
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index e099899..d78452d 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -3,6 +3,7 @@ source "arch/powerpc/platforms/Kconfig.cputype"
 config PPC32
 	bool
 	default y if !PPC64
+	select OF_MEMORY_AT_0_QUIRK
 
 config 32BIT
 	bool
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 889005f..230c747 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -77,4 +77,7 @@ config OF_RESERVED_MEM
 	help
 	  Helpers to allow for reservation of memory regions
 
+config OF_MEMORY_AT_0_QUIRK
+	def_bool n
+
 endmenu # OF
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index fa16a91..1b80b94 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -887,14 +887,22 @@ int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
 
 	/* We are scanning "memory" nodes only */
 	if (type == NULL) {
+#ifdef CONFIG_OF_MEMORY_AT_0_QUIRK
 		/*
 		 * The longtrail doesn't have a device_type on the
 		 * /memory node, so look for the node called /memory@0.
+		 * Converted to generic quirk to handle later platforms
+		 * with inforrect DTs that work only because of this
+		 * special handling.
 		 */
 		if (depth != 1 || strcmp(uname, "memory@0") != 0)
 			return 0;
-	} else if (strcmp(type, "memory") != 0)
+#else
+		return 0;
+#endif
+	} else if (strcmp(type, "memory") != 0) {
 		return 0;
+	}
 
 	reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
 	if (reg == NULL)
-- 
1.7.10.4

Re: [PATCH 3/3] of: Handle memory@0 node on PPC32 only

[Geert Uytterhoeven]

Hei Leif,

On Fri, Apr 18, 2014 at 2:59 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Apr 18, 2014 at 10:04:15AM +0200, Geert Uytterhoeven wrote:
>> On Thu, Apr 17, 2014 at 7:42 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > In order to deal with an firmware bug on a specific ppc32 platform
>> > (longtrail), early_init_dt_scan_memory() looks for a node called
>> > memory@0 on all platforms. Restrict this quirk to ppc32 kernels only.
>>
>> This breaks backwards compatibilty with old DTSes (at least on ARM/MIPS,
>> where you added the missing property in patches 1 and 2 of the series)?
>
> As Rob said in response to 0/3, the MIPSs would likely not be affected,
> since they embed the DT.
>
> How about the below v2 3/3 to address the ARM platform?

Looks fine to me, thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only

[Rob Herring]

On Fri, Apr 18, 2014 at 7:48 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Apr 17, 2014 at 07:43:13PM -0500, Rob Herring wrote:
>> On Thu, Apr 17, 2014 at 12:41 PM, Leif Lindholm
>> <leif.lindholm@linaro.org> wrote:
>> > drivers/of/fdt.c contains a workaround for a missing memory type
>> > entry on longtrail firmware. Make that quirk PPC32 only, and while
>> > at it - fix up the .dts files in the tree currently working only
>> > because of that quirk.
>>
>> But why do you need this?
>
> Apart from the current code permitting recreating a 15+ year old
> firmware bug into completely new platform ports?

I would prefer to see a "WARN_ON(!IS_ENABLED(CONFIG_PPC32));" added here.

Really, I would like to see quirks like this fixed up out of line from
the normal flow somewhat like how PCI quirks are handled. So in this
example, we would just add the missing property to the dtb for the
broken platform before doing the memory scan. That does then require
libfdt which is something I'm working on.

> Because the UEFI stub for arm/arm64 needs to delete all of the "memory"
> nodes from the DT. And it would be nice to at least not have to compile
> the "and also delete anything called memory@0" into the arm64 image. Or
> any image not including support for affected platforms.

I don't see why you would handle that in the EFI stub. Given our lack
of validation, I can see there is a chance this happens but I think it
is pretty small. Given we only have a ARM board, I'd say we are doing
surprisingly well.

Rob

>> >  arch/arm/boot/dts/ste-ccu8540.dts     |    1 +
>> >  arch/mips/lantiq/dts/easy50712.dts    |    1 +
>> >  arch/mips/ralink/dts/mt7620a_eval.dts |    1 +
>> >  arch/mips/ralink/dts/rt2880_eval.dts  |    1 +
>> >  arch/mips/ralink/dts/rt3052_eval.dts  |    1 +
>> >  arch/mips/ralink/dts/rt3883_eval.dts  |    1 +
>>
>> I'm not worried about these MIPS dts files because they are all
>> built-in, but you are breaking compatibility with old dtbs for this
>> ARM board.
>
> Yeah, sorry. Sending out a v2 of part 3 shortly.
>
> /
>     Leif

Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only

[Leif Lindholm]

On Fri, Apr 18, 2014 at 10:37:58AM -0500, Rob Herring wrote:
> >> But why do you need this?
> >
> > Apart from the current code permitting recreating a 15+ year old
> > firmware bug into completely new platform ports?
> 
> I would prefer to see a "WARN_ON(!IS_ENABLED(CONFIG_PPC32));" added here.

In addition to, or instead of, the QUIRK ifdef?

> Really, I would like to see quirks like this fixed up out of line from
> the normal flow somewhat like how PCI quirks are handled. So in this
> example, we would just add the missing property to the dtb for the
> broken platform before doing the memory scan. That does then require
> libfdt which is something I'm working on.

Getting rid of all this handling from generic code would clearly be
preferable. Is that code going in in the near future, or could we add
the quirk as a stopgap?

> > Because the UEFI stub for arm/arm64 needs to delete all of the "memory"
> > nodes from the DT. And it would be nice to at least not have to compile
> > the "and also delete anything called memory@0" into the arm64 image. Or
> > any image not including support for affected platforms.
> 
> I don't see why you would handle that in the EFI stub. Given our lack
> of validation, I can see there is a chance this happens but I think it
> is pretty small. Given we only have a ARM board, I'd say we are doing
> surprisingly well.

I'm not too bothered personally, but Mark Rutland handed me a patch to
improve the memory node handling in the stub, and he seemed to really
want this there. You guys can fight it out :)

What would be the effect of the UEFI code adding all its memblocks,
minus the reserved areas, and then the DT code doing a memblock_add
of all RAM (including reserved areas)? Would memblock_reserve()s on
the protected regions suffice to prevent crazy stuff from happening?

/
    Leif

Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only

[Rob Herring]

On Fri, Apr 18, 2014 at 3:13 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Apr 18, 2014 at 10:37:58AM -0500, Rob Herring wrote:
>> >> But why do you need this?
>> >
>> > Apart from the current code permitting recreating a 15+ year old
>> > firmware bug into completely new platform ports?
>>
>> I would prefer to see a "WARN_ON(!IS_ENABLED(CONFIG_PPC32));" added here.
>
> In addition to, or instead of, the QUIRK ifdef?

Instead of because I don't see how you handle the ARM board
compatibility with the ifdef. (And please, no ifdef for that board).

>> Really, I would like to see quirks like this fixed up out of line from
>> the normal flow somewhat like how PCI quirks are handled. So in this
>> example, we would just add the missing property to the dtb for the
>> broken platform before doing the memory scan. That does then require
>> libfdt which is something I'm working on.
>
> Getting rid of all this handling from generic code would clearly be
> preferable. Is that code going in in the near future, or could we add
> the quirk as a stopgap?

Some sort of quirk infrastructure is not going to happen soon. It's
just an idea bouncing in my head ATM.

>> > Because the UEFI stub for arm/arm64 needs to delete all of the "memory"
>> > nodes from the DT. And it would be nice to at least not have to compile
>> > the "and also delete anything called memory@0" into the arm64 image. Or
>> > any image not including support for affected platforms.
>>
>> I don't see why you would handle that in the EFI stub. Given our lack
>> of validation, I can see there is a chance this happens but I think it
>> is pretty small. Given we only have a ARM board, I'd say we are doing
>> surprisingly well.
>
> I'm not too bothered personally, but Mark Rutland handed me a patch to
> improve the memory node handling in the stub, and he seemed to really
> want this there. You guys can fight it out :)

Simply put, we shouldn't put work-arounds in new code for new platforms.

> What would be the effect of the UEFI code adding all its memblocks,
> minus the reserved areas, and then the DT code doing a memblock_add
> of all RAM (including reserved areas)? Would memblock_reserve()s on
> the protected regions suffice to prevent crazy stuff from happening?

So use UEFI to add the memory, but then add reserved areas with DT?
I'm not sure I follow, but even if I did I don't know memblock code
well enough to say what it would do.

Rob

Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only

[Leif Lindholm]

On Fri, Apr 18, 2014 at 04:28:17PM -0500, Rob Herring wrote:
> >> > Apart from the current code permitting recreating a 15+ year old
> >> > firmware bug into completely new platform ports?
> >>
> >> I would prefer to see a "WARN_ON(!IS_ENABLED(CONFIG_PPC32));" added here.
> >
> > In addition to, or instead of, the QUIRK ifdef?
> 
> Instead of because I don't see how you handle the ARM board
> compatibility with the ifdef. (And please, no ifdef for that board).

Umm, according to my memory as well as my sent mail folder, I cc:d you
on v2 of part 3. Could you have a look at that, please?

A WARN_ON would still mean this ancient workaround for a specific ppc32
platform remains enabled on ~10 architectures that don't use it.

> >> Really, I would like to see quirks like this fixed up out of line from
> >> the normal flow somewhat like how PCI quirks are handled. So in this
> >> example, we would just add the missing property to the dtb for the
> >> broken platform before doing the memory scan. That does then require
> >> libfdt which is something I'm working on.
> >
> > Getting rid of all this handling from generic code would clearly be
> > preferable. Is that code going in in the near future, or could we add
> > the quirk as a stopgap?
> 
> Some sort of quirk infrastructure is not going to happen soon. It's
> just an idea bouncing in my head ATM.

Mmm...

> > What would be the effect of the UEFI code adding all its memblocks,
> > minus the reserved areas, and then the DT code doing a memblock_add
> > of all RAM (including reserved areas)? Would memblock_reserve()s on
> > the protected regions suffice to prevent crazy stuff from happening?
> 
> So use UEFI to add the memory, but then add reserved areas with DT?

No, to add memory and reserved areas based on UEFI memory map.
And then add any memory@0/!type nodes as well, if they're left around.

> I'm not sure I follow, but even if I did I don't know memblock code
> well enough to say what it would do.

If we did end up with stray memory@0/!type nodes, we could initialise
memblock multiple times with overlapping but incompatible areas.
And I don't know if that would be a problem. Which makes me a little
bit nervous.

/
    Leif

Re: [PATCH 3/3] of: Handle memory@0 node on PPC32 only

[Rob Herring]

On Fri, Apr 18, 2014 at 7:59 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> Hi Geert,
>
> On Fri, Apr 18, 2014 at 10:04:15AM +0200, Geert Uytterhoeven wrote:
>> On Thu, Apr 17, 2014 at 7:42 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> > In order to deal with an firmware bug on a specific ppc32 platform
>> > (longtrail), early_init_dt_scan_memory() looks for a node called
>> > memory@0 on all platforms. Restrict this quirk to ppc32 kernels only.
>>
>> This breaks backwards compatibilty with old DTSes (at least on ARM/MIPS,
>> where you added the missing property in patches 1 and 2 of the series)?
>
> As Rob said in response to 0/3, the MIPSs would likely not be affected,
> since they embed the DT.
>
>> For the Longtrail, I don't care much anymore, as mine died in 2004.
>> AFAIK, there have never been many users anyway.
>
> There are still a few mentions of it under arch/powerpc/, so I wouldn't
> want to be the one to kill it off...
>
> How about the below v2 3/3 to address the ARM platform?
>
> Regards,
>
> Leif
>
> From 6fa0b837ad71780334eb97d63c507165b6c57add Mon Sep 17 00:00:00 2001
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Date: Thu, 17 Apr 2014 14:24:47 +0100
> Subject: [PATCH] of: arm: powerpc: Restrict memory@0 node handling to
>  affected platforms
>
> In order to deal with a firmware bug on a specific ppc32 platform
> (longtrail), early_init_dt_scan_memory() looks for a node called
> memory@0 on all platforms, for all nodes lacking a device_type.
> Restrict this quirk to ppc32 and the arm mach-ux500 platforms (one of
> which has depended on this special handling).


> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 889005f..230c747 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -77,4 +77,7 @@ config OF_RESERVED_MEM
>         help
>           Helpers to allow for reservation of memory regions
>
> +config OF_MEMORY_AT_0_QUIRK
> +       def_bool n

I do not like this because it would not scale to many quirks. As I
said,, my preference here would be to just add a WARN.

The other option is get approval to break compatibility on the ST
platform. It may not be a concern on certain platforms.

Rob

Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only

[Grant Likely]

On Fri, 18 Apr 2014 10:37:58 -0500, Rob Herring <robherring2@gmail.com> wrote:
> On Fri, Apr 18, 2014 at 7:48 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Apr 17, 2014 at 07:43:13PM -0500, Rob Herring wrote:
> >> On Thu, Apr 17, 2014 at 12:41 PM, Leif Lindholm
> >> <leif.lindholm@linaro.org> wrote:
> >> > drivers/of/fdt.c contains a workaround for a missing memory type
> >> > entry on longtrail firmware. Make that quirk PPC32 only, and while
> >> > at it - fix up the .dts files in the tree currently working only
> >> > because of that quirk.
> >>
> >> But why do you need this?
> >
> > Apart from the current code permitting recreating a 15+ year old
> > firmware bug into completely new platform ports?
> 
> I would prefer to see a "WARN_ON(!IS_ENABLED(CONFIG_PPC32));" added here.

I completely agree.

> Really, I would like to see quirks like this fixed up out of line from
> the normal flow somewhat like how PCI quirks are handled. So in this
> example, we would just add the missing property to the dtb for the
> broken platform before doing the memory scan. That does then require
> libfdt which is something I'm working on.

In fact, for Leif's purposes, I would rather have a flag when booting via
UEFI, and make the kernel skip the memory node parsing if the UEFI
memory map is available. Then the stub doesn't need to do any munging of
the DTB at all.

g.

Re: [PATCH 3/3] of: Handle memory@0 node on PPC32 only

[Grant Likely]

On Fri, 18 Apr 2014 13:59:24 +0100, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> Hi Geert,
> 
> On Fri, Apr 18, 2014 at 10:04:15AM +0200, Geert Uytterhoeven wrote:
> > On Thu, Apr 17, 2014 at 7:42 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > > In order to deal with an firmware bug on a specific ppc32 platform
> > > (longtrail), early_init_dt_scan_memory() looks for a node called
> > > memory@0 on all platforms. Restrict this quirk to ppc32 kernels only.
> > 
> > This breaks backwards compatibilty with old DTSes (at least on ARM/MIPS,
> > where you added the missing property in patches 1 and 2 of the series)?
> 
> As Rob said in response to 0/3, the MIPSs would likely not be affected,
> since they embed the DT.
> 
> > For the Longtrail, I don't care much anymore, as mine died in 2004.
> > AFAIK, there have never been many users anyway.
> 
> There are still a few mentions of it under arch/powerpc/, so I wouldn't
> want to be the one to kill it off...
> 
> How about the below v2 3/3 to address the ARM platform?

The problem with this approach is that selecting one board that needs it
automatically makes it active for all boards. It would need to be
something more like the following:

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 399e242e1a42..55d65b2b4c74 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -887,12 +887,10 @@ int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
 
 	/* We are scanning "memory" nodes only */
 	if (type == NULL) {
-		/*
-		 * The longtrail doesn't have a device_type on the
-		 * /memory node, so look for the node called /memory@0.
-		 */
 		if (depth != 1 || strcmp(uname, "memory@0") != 0)
 			return 0;
+		if (!of_flat_dt_match(dt_root, memory_quirk_list))
+			return 0;
 	} else if (strcmp(type, "memory") != 0)
 		return 0;
 
With a list of compatible properties for affected boards.

g.

Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only

[Leif Lindholm]

On Tue, Apr 22, 2014 at 02:08:29PM +0100, Grant Likely wrote:
> > I would prefer to see a "WARN_ON(!IS_ENABLED(CONFIG_PPC32));" added here.
> 
> I completely agree.

OK. So do we keep this around on unaffected architectures in perpetuity?

Or can there be some cut-off date when the majority of DT-enabled
platforms can stop including this workaround which permits new incorrect
DTs to be implemented so long as they are incorrect in this particular
way?

> > Really, I would like to see quirks like this fixed up out of line from
> > the normal flow somewhat like how PCI quirks are handled. So in this
> > example, we would just add the missing property to the dtb for the
> > broken platform before doing the memory scan. That does then require
> > libfdt which is something I'm working on.
> 
> In fact, for Leif's purposes, I would rather have a flag when booting via
> UEFI, and make the kernel skip the memory node parsing if the UEFI
> memory map is available. Then the stub doesn't need to do any munging of
> the DTB at all.

The reason for me doing that is because we (including you) agreed at
the discussion held during LCU13 that this was the safest way of
preventing "mischief" like userland trying to read information from
/proc/device-tree...

/
    Leif

Re: [PATCH 3/3] of: Handle memory@0 node on PPC32 only

[Mark Rutland]

Hi,

> > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> > index 889005f..230c747 100644
> > --- a/drivers/of/Kconfig
> > +++ b/drivers/of/Kconfig
> > @@ -77,4 +77,7 @@ config OF_RESERVED_MEM
> >         help
> >           Helpers to allow for reservation of memory regions
> >
> > +config OF_MEMORY_AT_0_QUIRK
> > +       def_bool n
> 
> I do not like this because it would not scale to many quirks. As I
> said,, my preference here would be to just add a WARN.

I would very much like to be able to remove the quirk entirely for arm64
if possible, which would require more than the addition of a WARN.

> The other option is get approval to break compatibility on the ST
> platform. It may not be a concern on certain platforms.

Lee, would you be able to provide an answer either way so we can get
the discussion moving?

Cheers,
Mark.

Re: [PATCH 3/3] of: Handle memory@0 node on PPC32 only

[Mark Rutland]

On Tue, Apr 22, 2014 at 02:35:15PM +0100, Grant Likely wrote:
> On Fri, 18 Apr 2014 13:59:24 +0100, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > Hi Geert,
> > 
> > On Fri, Apr 18, 2014 at 10:04:15AM +0200, Geert Uytterhoeven wrote:
> > > On Thu, Apr 17, 2014 at 7:42 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > > > In order to deal with an firmware bug on a specific ppc32 platform
> > > > (longtrail), early_init_dt_scan_memory() looks for a node called
> > > > memory@0 on all platforms. Restrict this quirk to ppc32 kernels only.
> > > 
> > > This breaks backwards compatibilty with old DTSes (at least on ARM/MIPS,
> > > where you added the missing property in patches 1 and 2 of the series)?
> > 
> > As Rob said in response to 0/3, the MIPSs would likely not be affected,
> > since they embed the DT.
> > 
> > > For the Longtrail, I don't care much anymore, as mine died in 2004.
> > > AFAIK, there have never been many users anyway.
> > 
> > There are still a few mentions of it under arch/powerpc/, so I wouldn't
> > want to be the one to kill it off...
> > 
> > How about the below v2 3/3 to address the ARM platform?
> 
> The problem with this approach is that selecting one board that needs it
> automatically makes it active for all boards. It would need to be
> something more like the following:
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 399e242e1a42..55d65b2b4c74 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -887,12 +887,10 @@ int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
>  
>  	/* We are scanning "memory" nodes only */
>  	if (type == NULL) {
> -		/*
> -		 * The longtrail doesn't have a device_type on the
> -		 * /memory node, so look for the node called /memory@0.
> -		 */
>  		if (depth != 1 || strcmp(uname, "memory@0") != 0)
>  			return 0;
> +		if (!of_flat_dt_match(dt_root, memory_quirk_list))
> +			return 0;
>  	} else if (strcmp(type, "memory") != 0)
>  		return 0;
>  
> With a list of compatible properties for affected boards.

That looks sane to me.

Does anyone have a LongTrail DT to hand, and if so does the root have a
compatible string? From grepping through the kernel I could only find a
model string ("IBM,LongTrail").

Is anyone aware of strings other than that and "st-ericsson,ccu8540" to
look out for?

Cheers,
Mark.

Re: [PATCH 3/3] of: Handle memory@0 node on PPC32 only

[Geert Uytterhoeven]

Hi Mark,

On Wed, Apr 23, 2014 at 12:45 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> Does anyone have a LongTrail DT to hand, and if so does the root have a
> compatible string? From grepping through the kernel I could only find a
> model string ("IBM,LongTrail").

It's compatible with "prep":

http://users.telenet.be/geertu/Linux/PPC/root.html

Full tree at:

http://users.telenet.be/geertu/Linux/PPC/DeviceTree.html

Sorry, the HTML version is all I still have.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 3/3] of: Handle memory@0 node on PPC32 only

[Grant Likely]

On Wed, 23 Apr 2014 11:45:28 +0100, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Apr 22, 2014 at 02:35:15PM +0100, Grant Likely wrote:
> > On Fri, 18 Apr 2014 13:59:24 +0100, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > > Hi Geert,
> > > 
> > > On Fri, Apr 18, 2014 at 10:04:15AM +0200, Geert Uytterhoeven wrote:
> > > > On Thu, Apr 17, 2014 at 7:42 PM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > > > > In order to deal with an firmware bug on a specific ppc32 platform
> > > > > (longtrail), early_init_dt_scan_memory() looks for a node called
> > > > > memory@0 on all platforms. Restrict this quirk to ppc32 kernels only.
> > > > 
> > > > This breaks backwards compatibilty with old DTSes (at least on ARM/MIPS,
> > > > where you added the missing property in patches 1 and 2 of the series)?
> > > 
> > > As Rob said in response to 0/3, the MIPSs would likely not be affected,
> > > since they embed the DT.
> > > 
> > > > For the Longtrail, I don't care much anymore, as mine died in 2004.
> > > > AFAIK, there have never been many users anyway.
> > > 
> > > There are still a few mentions of it under arch/powerpc/, so I wouldn't
> > > want to be the one to kill it off...
> > > 
> > > How about the below v2 3/3 to address the ARM platform?
> > 
> > The problem with this approach is that selecting one board that needs it
> > automatically makes it active for all boards. It would need to be
> > something more like the following:
> > 
> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > index 399e242e1a42..55d65b2b4c74 100644
> > --- a/drivers/of/fdt.c
> > +++ b/drivers/of/fdt.c
> > @@ -887,12 +887,10 @@ int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
> >  
> >  	/* We are scanning "memory" nodes only */
> >  	if (type == NULL) {
> > -		/*
> > -		 * The longtrail doesn't have a device_type on the
> > -		 * /memory node, so look for the node called /memory@0.
> > -		 */
> >  		if (depth != 1 || strcmp(uname, "memory@0") != 0)
> >  			return 0;
> > +		if (!of_flat_dt_match(dt_root, memory_quirk_list))
> > +			return 0;
> >  	} else if (strcmp(type, "memory") != 0)
> >  		return 0;
> >  
> > With a list of compatible properties for affected boards.
> 
> That looks sane to me.
> 
> Does anyone have a LongTrail DT to hand, and if so does the root have a
> compatible string? From grepping through the kernel I could only find a
> model string ("IBM,LongTrail").

Actually, on LongTrail this can be removed from the common code
entirely. It has real open firmware and PowerPC already has the
infrastructure for fixing up the device tree.

Here's a draft patch that I've compile tested, but nothing else.

g.

---

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 078145acf7fb..18b2c3fee98f 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -2587,9 +2587,18 @@ static void __init fixup_device_tree_chrp(void)
 	phandle ph;
 	u32 prop[6];
 	u32 rloc = 0x01006000; /* IO space; PCI device = 12 */
-	char *name;
+	char *name, strprop[16];
 	int rc;
 
+	/* Deal with missing device_type in LongTrail memory node */
+	name = "/memory@0";
+	ph = call_prom("finddevice", 1, 1, ADDR(name));
+	rc = prom_getprop(ph, "device_type", strprop, sizeof(strprop));
+	if (rc == PROM_ERROR || strcmp(strprop, "memory") != 0) {
+		prom_printf("Fixing up missing device_type on /memory@0 node...\n");
+		prom_setprop(ph, name, "device_type", "memory", sizeof("memory"));
+	}
+
 	name = "/pci@80000000/isa@c";
 	ph = call_prom("finddevice", 1, 1, ADDR(name));
 	if (!PHANDLE_VALID(ph)) {
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 7a2ef7bb8022..7cda0d279cbe 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -886,14 +886,7 @@ int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
 	unsigned long l;
 
 	/* We are scanning "memory" nodes only */
-	if (type == NULL) {
-		/*
-		 * The longtrail doesn't have a device_type on the
-		 * /memory node, so look for the node called /memory@0.
-		 */
-		if (depth != 1 || strcmp(uname, "memory@0") != 0)
-			return 0;
-	} else if (strcmp(type, "memory") != 0)
+	if (!type || strcmp(type, "memory") != 0)
 		return 0;
 
 	reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);

Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only

[Grant Likely]

On Tue, 22 Apr 2014 15:05:26 +0100, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Apr 22, 2014 at 02:08:29PM +0100, Grant Likely wrote:
> > > I would prefer to see a "WARN_ON(!IS_ENABLED(CONFIG_PPC32));" added here.
> > 
> > I completely agree.
> 
> OK. So do we keep this around on unaffected architectures in perpetuity?
> 
> Or can there be some cut-off date when the majority of DT-enabled
> platforms can stop including this workaround which permits new incorrect
> DTs to be implemented so long as they are incorrect in this particular
> way?
> 
> > > Really, I would like to see quirks like this fixed up out of line from
> > > the normal flow somewhat like how PCI quirks are handled. So in this
> > > example, we would just add the missing property to the dtb for the
> > > broken platform before doing the memory scan. That does then require
> > > libfdt which is something I'm working on.
> > 
> > In fact, for Leif's purposes, I would rather have a flag when booting via
> > UEFI, and make the kernel skip the memory node parsing if the UEFI
> > memory map is available. Then the stub doesn't need to do any munging of
> > the DTB at all.
> 
> The reason for me doing that is because we (including you) agreed at
> the discussion held during LCU13 that this was the safest way of
> preventing "mischief" like userland trying to read information from
> /proc/device-tree...

I'm not the most consistent of people. I often change my mind and then
have no recollection of ever thinking differently.

Userland reading from /proc/device-tree isn't so much a problem, but
kernel drivers doing it might be.

But, regardless of whether or not the stub clears out the memory
nodes, it is still I think good practice for the kernel to make the
decision to ignore memory nodes, and not rely on them being cleared
correctly.

g.

Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only

[Grant Likely]

On Tue, 22 Apr 2014 15:05:26 +0100, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Tue, Apr 22, 2014 at 02:08:29PM +0100, Grant Likely wrote:
> > > I would prefer to see a "WARN_ON(!IS_ENABLED(CONFIG_PPC32));" added here.
> > 
> > I completely agree.
> 
> OK. So do we keep this around on unaffected architectures in perpetuity?
> 
> Or can there be some cut-off date when the majority of DT-enabled
> platforms can stop including this workaround which permits new incorrect
> DTs to be implemented so long as they are incorrect in this particular
> way?

I'd be fine with a patch that can enable the workaround selectively for
specific machines. Some people will still hit breakage, but we can fix
that by adding their machine to the quirk list.

g.

Re: [PATCH 0/3] of: dts: enable memory@0 quirk for PPC32 only

[Leif Lindholm]

On Wed, Apr 23, 2014 at 02:15:08PM +0100, Grant Likely wrote:
> > The reason for me doing that is because we (including you) agreed at
> > the discussion held during LCU13 that this was the safest way of
> > preventing "mischief" like userland trying to read information from
> > /proc/device-tree...
> 
> I'm not the most consistent of people. I often change my mind and then
> have no recollection of ever thinking differently.

And that is fine, but you were not the only person agreeing.

> Userland reading from /proc/device-tree isn't so much a problem, but
> kernel drivers doing it might be.
> 
> But, regardless of whether or not the stub clears out the memory
> nodes, it is still I think good practice for the kernel to make the
> decision to ignore memory nodes, and not rely on them being cleared
> correctly.

I also remember you saying that relaxing restrictions later on is a lot
easier than tightening them. On that basis, can we please get the UEFI
set merged before we start redefining the stub/kernel protocol which was
agreed at LCU (November last year) after spending a month or two trying
to get sufficient number of interested parties in the same room?

/
	Leif

Re: [PATCH 3/3] of: Handle memory@0 node on PPC32 only

[Leif Lindholm]

On Wed, Apr 23, 2014 at 02:10:58PM +0100, Grant Likely wrote:
> > Does anyone have a LongTrail DT to hand, and if so does the root have a
> > compatible string? From grepping through the kernel I could only find a
> > model string ("IBM,LongTrail").
> 
> Actually, on LongTrail this can be removed from the common code
> entirely. It has real open firmware and PowerPC already has the
> infrastructure for fixing up the device tree.
> 
> Here's a draft patch that I've compile tested, but nothing else.

I would certainly be happy with that.

Consider my 3/3 withdrawn.

And if the kernel proper will stop honoring nodes with no type,
there is no need for the stub to treat those specially either.

/
    Leif

> g.
> 
> ---
> 
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index 078145acf7fb..18b2c3fee98f 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -2587,9 +2587,18 @@ static void __init fixup_device_tree_chrp(void)
>  	phandle ph;
>  	u32 prop[6];
>  	u32 rloc = 0x01006000; /* IO space; PCI device = 12 */
> -	char *name;
> +	char *name, strprop[16];
>  	int rc;
>  
> +	/* Deal with missing device_type in LongTrail memory node */
> +	name = "/memory@0";
> +	ph = call_prom("finddevice", 1, 1, ADDR(name));
> +	rc = prom_getprop(ph, "device_type", strprop, sizeof(strprop));
> +	if (rc == PROM_ERROR || strcmp(strprop, "memory") != 0) {
> +		prom_printf("Fixing up missing device_type on /memory@0 node...\n");
> +		prom_setprop(ph, name, "device_type", "memory", sizeof("memory"));
> +	}
> +
>  	name = "/pci@80000000/isa@c";
>  	ph = call_prom("finddevice", 1, 1, ADDR(name));
>  	if (!PHANDLE_VALID(ph)) {
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 7a2ef7bb8022..7cda0d279cbe 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -886,14 +886,7 @@ int __init early_init_dt_scan_memory(unsigned long node, const char *uname,
>  	unsigned long l;
>  
>  	/* We are scanning "memory" nodes only */
> -	if (type == NULL) {
> -		/*
> -		 * The longtrail doesn't have a device_type on the
> -		 * /memory node, so look for the node called /memory@0.
> -		 */
> -		if (depth != 1 || strcmp(uname, "memory@0") != 0)
> -			return 0;
> -	} else if (strcmp(type, "memory") != 0)
> +	if (!type || strcmp(type, "memory") != 0)
>  		return 0;
>  
>  	reg = of_get_flat_dt_prop(node, "linux,usable-memory", &l);
> 

Re: [PATCH 3/3] of: Handle memory@0 node on PPC32 only

[Grant Likely]

On Thu, 24 Apr 2014 10:26:42 +0100, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Apr 23, 2014 at 02:10:58PM +0100, Grant Likely wrote:
> > > Does anyone have a LongTrail DT to hand, and if so does the root have a
> > > compatible string? From grepping through the kernel I could only find a
> > > model string ("IBM,LongTrail").
> > 
> > Actually, on LongTrail this can be removed from the common code
> > entirely. It has real open firmware and PowerPC already has the
> > infrastructure for fixing up the device tree.
> > 
> > Here's a draft patch that I've compile tested, but nothing else.
> 
> I would certainly be happy with that.
> 
> Consider my 3/3 withdrawn.
> 
> And if the kernel proper will stop honoring nodes with no type,
> there is no need for the stub to treat those specially either.

So, after thinking about it some more, I've changed my minde about the
whole thing again. The impact is quite contained because there weren't a
lot of systems that had ram based at 0. I'm going to commit this patch,
but I'll include a note in the commit text that if it causes trouble for
anyone that they should yell and I'll revert it. I don't think it will
though.

g.