v4.17 regression: PowerMac G3 won't boot, was Re: [PATCH v5 1/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle()

Hi Frank,

Linux v4.17 and later will no longer boot on a G3 PowerMac. The boot hangs 
very early, before any video driver loads.

Stan and I were able to bisect the regression between v4.16 and v4.17 and 
arrived at commit 0b3ce78e90fc ("of: cache phandle nodes to reduce cost of 
of_find_node_by_phandle()").

I don't see any obvious bug in 0b3ce78e90fc or b9952b5218ad. But if you 
revert these from v4.18 (which is also affected) that certainly resolves 
the issue.

I did see this in the kernel messages:

Duplicate name in PowerPC,750, renamed to "l2-cache#1"
Duplicate name in mac-io, renamed to "ide#1"
Duplicate name in ide#1, renamed to "atapi-disk#1"
Duplicate name in multifunc-device, renamed to "pci1799,1#1"

No idea whether that's relevant; I haven't done any further investigation. 
Complete dmesg output is attached. Please let me know if there's any more 
information you need to help find the bug.

Thanks.

-- 

On Sun, 4 Mar 2018, frowand.list@gmail.com wrote:

On Wed, Aug 29, 2018 at 7:44 PM Finn Thain [off-list ref] wrote:

Hi Frank,

Linux v4.17 and later will no longer boot on a G3 PowerMac. The boot hangs
very early, before any video driver loads.

Stan and I were able to bisect the regression between v4.16 and v4.17 and
arrived at commit 0b3ce78e90fc ("of: cache phandle nodes to reduce cost of
of_find_node_by_phandle()").

I don't see any obvious bug in 0b3ce78e90fc or b9952b5218ad. But if you
revert these from v4.18 (which is also affected) that certainly resolves
the issue.

Perhaps a bad assumption on phandle values causing a problem. Can you
provide a dump of all the phandle or linux,phandle values from
/proc/device-tree.

Rob

On 8/29/18 7:05 PM, Rob Herring wrote:

On Wed, Aug 29, 2018 at 7:44 PM Finn Thain [off-list ref] wrote:

quoted

Hi Frank,

Linux v4.17 and later will no longer boot on a G3 PowerMac. The boot hangs
very early, before any video driver loads.

Stan and I were able to bisect the regression between v4.16 and v4.17 and
arrived at commit 0b3ce78e90fc ("of: cache phandle nodes to reduce cost of
of_find_node_by_phandle()").

I don't see any obvious bug in 0b3ce78e90fc or b9952b5218ad. But if you
revert these from v4.18 (which is also affected) that certainly resolves
the issue.

Perhaps a bad assumption on phandle values causing a problem. Can you
provide a dump of all the phandle or linux,phandle values from
/proc/device-tree.

Rob

Rob,

As suggested by Finn, I installed device-tree-compiler and
powerpc-ibm-utils.
 
Running "dtc -I fs -H both /sys/firmware/devicetree/base"
resulted in the following errors:

DTC: fs->dts on file "/sys/firmware/devicetree/base"
ERROR (name_properties): "name" property in
/pci/multifunc-device/pci1799,1#1 is incorrect ("pci1799,1" instead of
base node name)
ERROR (name_properties): "name" property in /pci/mac-io/ide#1 is
incorrect ("ide" instead of base node name)
ERROR (name_properties): "name" property in
/pci/mac-io/ide#1/atapi-disk#1 is incorrect ("atapi-disk" instead of
base node name)
ERROR (name_properties): "name" property in /cpus/PowerPC,750/l2-cache#1
is incorrect ("l2-cache" instead of base node name)
ERROR: Input tree has errors, aborting (use -f to force output)

If I force output with "-f", the resulting file has no occurrences 
of "phandle".

Running "lsprop /proc/device-tree | grep -i phandle" results in no 
output.

Please let me know if there's some other way to get information that 
would be helpful.

thanks

-Stan

On Thu, 2018-08-30 at 20:39 -0600, Mac User wrote:

On 8/29/18 7:05 PM, Rob Herring wrote:

quoted

On Wed, Aug 29, 2018 at 7:44 PM Finn Thain [off-list ref] wrote:

quoted

Hi Frank,

Linux v4.17 and later will no longer boot on a G3 PowerMac. The boot hangs
very early, before any video driver loads.

Stan and I were able to bisect the regression between v4.16 and v4.17 and
arrived at commit 0b3ce78e90fc ("of: cache phandle nodes to reduce cost of
of_find_node_by_phandle()").

I don't see any obvious bug in 0b3ce78e90fc or b9952b5218ad. But if you
revert these from v4.18 (which is also affected) that certainly resolves
the issue.

Perhaps a bad assumption on phandle values causing a problem. Can you
provide a dump of all the phandle or linux,phandle values from
/proc/device-tree.

Rob

Rob,

As suggested by Finn, I installed device-tree-compiler and
powerpc-ibm-utils.
 
Running "dtc -I fs -H both /sys/firmware/devicetree/base"
resulted in the following errors:

DTC: fs->dts on file "/sys/firmware/devicetree/base"
ERROR (name_properties): "name" property in
/pci/multifunc-device/pci1799,1#1 is incorrect ("pci1799,1" instead of
base node name)
ERROR (name_properties): "name" property in /pci/mac-io/ide#1 is
incorrect ("ide" instead of base node name)
ERROR (name_properties): "name" property in
/pci/mac-io/ide#1/atapi-disk#1 is incorrect ("atapi-disk" instead of
base node name)
ERROR (name_properties): "name" property in /cpus/PowerPC,750/l2-cache#1
is incorrect ("l2-cache" instead of base node name)
ERROR: Input tree has errors, aborting (use -f to force output)

If I force output with "-f", the resulting file has no occurrences 
of "phandle".

Are you booting with BootX or Open Firmware ?

Running "lsprop /proc/device-tree | grep -i phandle" results in no 
output.

Please let me know if there's some other way to get information that 
would be helpful.

thanks

-Stan

Hi Finn,

On 08/29/18 17:44, Finn Thain wrote:

Hi Frank,

Linux v4.17 and later will no longer boot on a G3 PowerMac. The boot hangs 
very early, before any video driver loads.

Stan and I were able to bisect the regression between v4.16 and v4.17 and 
arrived at commit 0b3ce78e90fc ("of: cache phandle nodes to reduce cost of 
of_find_node_by_phandle()").

I don't see any obvious bug in 0b3ce78e90fc or b9952b5218ad. But if you 
revert these from v4.18 (which is also affected) that certainly resolves 
the issue.

I did see this in the kernel messages:

Duplicate name in PowerPC,750, renamed to "l2-cache#1"
Duplicate name in mac-io, renamed to "ide#1"
Duplicate name in ide#1, renamed to "atapi-disk#1"
Duplicate name in multifunc-device, renamed to "pci1799,1#1"

No idea whether that's relevant; I haven't done any further investigation. 
Complete dmesg output is attached. Please let me know if there's any more 
information you need to help find the bug.

Thanks.

I don't have any useful answers yet, but I am following the thread and have
also quickly scanned the two commits for any obvious cause.  I will look
into this some more, but have a few other tasks that I need to complete
first.

A long shot, but something to consider, is that I failed to cover the
cases of dynamic devicetree updates (removing nodes that contain a
phandle) in ways other than overlays.  Michael Ellerman has reported
such a problem for powerpc/mobility with of_detach_node().  A patch to
fix that is one of the tasks I need to complete.

-Frank

On Fri, 2018-08-31 at 14:35 +1000, Benjamin Herrenschmidt wrote:

quoted

If I force output with "-f", the resulting file has no occurrences 
of "phandle".

Are you booting with BootX or Open Firmware ?

Assuming you are using BootX (or miBoot), can you try this patch ?

--- a/arch/powerpc/platforms/powermac/bootx_init.c
+++ b/arch/powerpc/platforms/powermac/bootx_init.c

@@ -37,6 +37,7 @@ static unsigned long __initdata bootx_dt_strend;
 static unsigned long __initdata bootx_node_chosen;
 static boot_infos_t * __initdata bootx_info;
 static char __initdata bootx_disp_path[256];
+static int __initdata bootx_phandle;
 
 /* Is boot-info compatible ? */
 #define BOOT_INFO_IS_COMPATIBLE(bi) \

@@ -258,6 +259,8 @@ static void __init bootx_scan_dt_build_strings(unsigned long base,
                namep = pp->name ? (char *)(base + pp->name) : NULL;
                if (namep == NULL || strcmp(namep, "name") == 0)
                        goto next;
+               if (!strcmp(namep, "phandle") || !strcmp(namep, "linux,phandle"))
+                       bootx_phandle = -1;
                /* get/create string entry */
                soff = bootx_dt_find_string(namep);
                if (soff == 0)

@@ -310,6 +313,7 @@ static void __init bootx_scan_dt_build_struct(unsigned long base,
        *mem_end = _ALIGN_UP((unsigned long)lp + 1, 4);
 
        /* get and store all properties */
+       has_phandle = false;
        while (*ppp) {
                struct bootx_dt_prop *pp =
                        (struct bootx_dt_prop *)(base + *ppp);

@@ -330,6 +334,12 @@ static void __init bootx_scan_dt_build_struct(unsigned long base,
                ppp = &pp->next;
        }
 
+       /* add a phandle */
+       if (bootx_phandle > 0) {
+               bootx_dt_add_prop("phandle", &bootx_phandle, 4, mem_end);
+               bootx_phandle++;
+       }
+
        if (node == bootx_node_chosen) {
                bootx_add_chosen_props(base, mem_end);
                if (bootx_info->dispDeviceRegEntryOffset == 0)

@@ -385,6 +395,8 @@ static unsigned long __init bootx_flatten_dt(unsigned long start)
        bootx_dt_add_string("linux,bootx-height", &mem_end);
        bootx_dt_add_string("linux,bootx-linebytes", &mem_end);
        bootx_dt_add_string("linux,bootx-addr", &mem_end);
+       if (bootx_phandle > 0)
+               bootx_dt_add_string("phandle", &mem_end);
        /* Wrap up strings */
        hdr->off_dt_strings = bootx_dt_strbase - mem_start;
        hdr->dt_strings_size = bootx_dt_strend - bootx_dt_strbase;

@@ -482,6 +494,7 @@ void __init bootx_init(unsigned long r3, unsigned long r4)
        bootx_dt_strbase = bootx_dt_strend = 0;
        bootx_node_chosen = 0;
        bootx_disp_path[0] = 0;
+       bootx_phandle = 1;
 
        if (!BOOT_INFO_IS_V2_COMPATIBLE(bi))
                bi->logicalDisplayBase = bi->dispDeviceBase;

On Thu, 2018-08-30 at 21:36 -0700, Frank Rowand wrote:

quoted

No idea whether that's relevant; I haven't done any further investigation. 
Complete dmesg output is attached. Please let me know if there's any more 
information you need to help find the bug.

Thanks.

I don't have any useful answers yet, but I am following the thread and have
also quickly scanned the two commits for any obvious cause.  I will look
into this some more, but have a few other tasks that I need to complete
first.

A long shot, but something to consider, is that I failed to cover the
cases of dynamic devicetree updates (removing nodes that contain a
phandle) in ways other than overlays.  Michael Ellerman has reported
such a problem for powerpc/mobility with of_detach_node().  A patch to
fix that is one of the tasks I need to complete.

The only thing I can think of is booting via the BootX bootloader on
those ancient macs results in a DT with no phandles. I didn't see an
obvious reason why that would cause that patch to break though.

Cheers,
Ben.

On Fri, 2018-08-31 at 14:35 +1000, Benjamin Herrenschmidt wrote:

quoted

If I force output with "-f", the resulting file has no occurrences 
of "phandle".

Are you booting with BootX or Open Firmware ?

Assuming you are using BootX (or miBoot), can you try this patch ?

--- a/arch/powerpc/platforms/powermac/bootx_init.c
+++ b/arch/powerpc/platforms/powermac/bootx_init.c

@@ -37,6 +37,7 @@ static unsigned long __initdata bootx_dt_strend;
 static unsigned long __initdata bootx_node_chosen;
 static boot_infos_t * __initdata bootx_info;
 static char __initdata bootx_disp_path[256];
+static int __initdata bootx_phandle;
 
 /* Is boot-info compatible ? */
 #define BOOT_INFO_IS_COMPATIBLE(bi) \

@@ -258,6 +259,8 @@ static void __init bootx_scan_dt_build_strings(unsigned long base,
                namep = pp->name ? (char *)(base + pp->name) : NULL;
                if (namep == NULL || strcmp(namep, "name") == 0)
                        goto next;
+               if (!strcmp(namep, "phandle") || !strcmp(namep, "linux,phandle"))
+                       bootx_phandle = -1;
                /* get/create string entry */
                soff = bootx_dt_find_string(namep);
                if (soff == 0)

@@ -330,6 +333,12 @@ static void __init bootx_scan_dt_build_struct(unsigned long base,
                ppp = &pp->next;
        }
 
+       /* add a phandle */
+       if (bootx_phandle > 0) {
+               bootx_dt_add_prop("phandle", &bootx_phandle, 4, mem_end);
+               bootx_phandle++;
+       }
+
        if (node == bootx_node_chosen) {
                bootx_add_chosen_props(base, mem_end);
                if (bootx_info->dispDeviceRegEntryOffset == 0)

@@ -385,6 +394,8 @@ static unsigned long __init bootx_flatten_dt(unsigned long start)
        bootx_dt_add_string("linux,bootx-height", &mem_end);
        bootx_dt_add_string("linux,bootx-linebytes", &mem_end);
        bootx_dt_add_string("linux,bootx-addr", &mem_end);
+       if (bootx_phandle > 0)
+               bootx_dt_add_string("phandle", &mem_end);
        /* Wrap up strings */
        hdr->off_dt_strings = bootx_dt_strbase - mem_start;
        hdr->dt_strings_size = bootx_dt_strend - bootx_dt_strbase;

@@ -482,6 +493,7 @@ void __init bootx_init(unsigned long r3, unsigned long r4)
        bootx_dt_strbase = bootx_dt_strend = 0;
        bootx_node_chosen = 0;
        bootx_disp_path[0] = 0;
+       bootx_phandle = 1;
 
        if (!BOOT_INFO_IS_V2_COMPATIBLE(bi))
                bi->logicalDisplayBase = bi->dispDeviceBase;

On 8/30/18 10:49 PM, Benjamin Herrenschmidt wrote:

On Fri, 2018-08-31 at 14:35 +1000, Benjamin Herrenschmidt wrote:

...
Assuming you are using BootX (or miBoot), can you try this patch ?

Yes, I'm using BootX.

Thanks to Finn for applying the patch (I wouldn't have been sure
which source tree to apply it to).

Thepatched kernel (Finn's vmlinux-4.18.0-00001-gd44cf7e41c19)
boots normally on my Beige G3 Desktop using BootX.

Thanks for working on this!

-Stan

On Fri, 31 Aug 2018, Mac User wrote:

On 8/30/18 10:49 PM, Benjamin Herrenschmidt wrote:

quoted

On Fri, 2018-08-31 at 14:35 +1000, Benjamin Herrenschmidt wrote:

...
Assuming you are using BootX (or miBoot), can you try this patch ?

Yes, I'm using BootX.

Thanks to Finn for applying the patch (I wouldn't have been sure which 
source tree to apply it to).

The patched kernel (Finn's vmlinux-4.18.0-00001-gd44cf7e41c19) boots 
normally on my Beige G3 Desktop using BootX.

Ben sent two patches, so I picked the most recent one and applied it by 
hand due to corrupted whitespace.

The patch you tested was the one below.

Thanks for working on this!

Thanks for testing!

-Stan

diff --git a/arch/powerpc/platforms/powermac/bootx_init.c b/arch/powerpc/platforms/powermac/bootx_init.c
index 3b3b0b9b3577..7fb1c7bb5835 100644
--- a/arch/powerpc/platforms/powermac/bootx_init.c
+++ b/arch/powerpc/platforms/powermac/bootx_init.c

@@ -37,6 +37,7 @@ static unsigned long __initdata bootx_dt_strend;
 static unsigned long __initdata bootx_node_chosen;
 static boot_infos_t * __initdata bootx_info;
 static char __initdata bootx_disp_path[256];
+static int __initdata bootx_phandle;
 
 /* Is boot-info compatible ? */
 #define BOOT_INFO_IS_COMPATIBLE(bi) \

@@ -258,6 +259,8 @@ static void __init bootx_scan_dt_build_strings(unsigned long base,
 		namep = pp->name ? (char *)(base + pp->name) : NULL;
  		if (namep == NULL || strcmp(namep, "name") == 0)
  			goto next;
+		if (!strcmp(namep, "phandle") || !strcmp(namep, "linux,phandle"))
+			bootx_phandle = -1;
 		/* get/create string entry */
 		soff = bootx_dt_find_string(namep);
 		if (soff == 0)

@@ -330,6 +333,12 @@ static void __init bootx_scan_dt_build_struct(unsigned long base,
 		ppp = &pp->next;
 	}
 
+	/* add a phandle */
+	if (bootx_phandle > 0) {
+		bootx_dt_add_prop("phandle", &bootx_phandle, 4, mem_end);
+		bootx_phandle++;
+	}
+
 	if (node == bootx_node_chosen) {
 		bootx_add_chosen_props(base, mem_end);
 		if (bootx_info->dispDeviceRegEntryOffset == 0)

@@ -385,6 +394,8 @@ static unsigned long __init bootx_flatten_dt(unsigned long start)
 	bootx_dt_add_string("linux,bootx-height", &mem_end);
 	bootx_dt_add_string("linux,bootx-linebytes", &mem_end);
 	bootx_dt_add_string("linux,bootx-addr", &mem_end);
+	if (bootx_phandle > 0)
+		bootx_dt_add_string("phandle", &mem_end);
 	/* Wrap up strings */
 	hdr->off_dt_strings = bootx_dt_strbase - mem_start;
 	hdr->dt_strings_size = bootx_dt_strend - bootx_dt_strbase;

@@ -482,6 +493,7 @@ void __init bootx_init(unsigned long r3, unsigned long r4)
 	bootx_dt_strbase = bootx_dt_strend = 0;
 	bootx_node_chosen = 0;
 	bootx_disp_path[0] = 0;
+	bootx_phandle = 1;
 
 	if (!BOOT_INFO_IS_V2_COMPATIBLE(bi))
 		bi->logicalDisplayBase = bi->dispDeviceBase;

-- 

On Fri, 2018-08-31 at 06:28 -0600, Mac User wrote:

On 8/30/18 10:49 PM, Benjamin Herrenschmidt wrote:

quoted

On Fri, 2018-08-31 at 14:35 +1000, Benjamin Herrenschmidt wrote:

...
Assuming you are using BootX (or miBoot), can you try this patch ?

Yes, I'm using BootX.

Thanks to Finn for applying the patch (I wouldn't have been sure
which source tree to apply it to).

Thepatched kernel (Finn's vmlinux-4.18.0-00001-gd44cf7e41c19)
boots normally on my Beige G3 Desktop using BootX.

Thanks for working on this!

Well it's still a sign of something wrong with the new code, my patch
just band-aids by creating fake phandles. I would be interesting to
figure out what causes the new code to barf.

Cheers,
Ben.

On Sat, 2018-09-01 at 09:36 +1000, Finn Thain wrote:

quoted

The patched kernel (Finn's vmlinux-4.18.0-00001-gd44cf7e41c19) boots 
normally on my Beige G3 Desktop using BootX.

Ben sent two patches, so I picked the most recent one and applied it by 
hand due to corrupted whitespace.

Yup, evolution seems to have broken copy/pasting of patches in a
preformatted email lately ... ugh.

The patch you tested was the one below.

On Fri, 2018-08-31 at 14:58 +1000, Benjamin Herrenschmidt wrote:

quoted

A long shot, but something to consider, is that I failed to cover the
cases of dynamic devicetree updates (removing nodes that contain a
phandle) in ways other than overlays.  Michael Ellerman has reported
such a problem for powerpc/mobility with of_detach_node().  A patch to
fix that is one of the tasks I need to complete.

The only thing I can think of is booting via the BootX bootloader on
those ancient macs results in a DT with no phandles. I didn't see an
obvious reason why that would cause that patch to break though.

Guys, we still don't have a fix for this one on its way upstream...

My test patch just creates phandle properties for all nodes, that was
not intended as a fix, more a way to check if the problem was related
to the lack of phandles.

I don't actually know why the new code causes things to fail when
phandles are absent. This needs to be looked at.

I'm travelling at the moment and generally caught up with other things,
I haven't had a chance to dig, so just a heads up. I don't intend to
submit my patch since it's just a band aid. We need to figure out what
the actual problem is.

Cheers,
Ben.

On 09/09/18 10:04, Benjamin Herrenschmidt wrote:

On Fri, 2018-08-31 at 14:58 +1000, Benjamin Herrenschmidt wrote:

quoted

quoted

A long shot, but something to consider, is that I failed to cover the
cases of dynamic devicetree updates (removing nodes that contain a
phandle) in ways other than overlays.  Michael Ellerman has reported
such a problem for powerpc/mobility with of_detach_node().  A patch to
fix that is one of the tasks I need to complete.

The only thing I can think of is booting via the BootX bootloader on
those ancient macs results in a DT with no phandles. I didn't see an
obvious reason why that would cause that patch to break though.

Guys, we still don't have a fix for this one on its way upstream...

My test patch just creates phandle properties for all nodes, that was
not intended as a fix, more a way to check if the problem was related
to the lack of phandles.

I don't actually know why the new code causes things to fail when
phandles are absent. This needs to be looked at.

I'm travelling at the moment and generally caught up with other things,
I haven't had a chance to dig, so just a heads up. I don't intend to
submit my patch since it's just a band aid. We need to figure out what
the actual problem is.

Cheers,
Ben.

Thanks for chasing after this, and the current heads up.

I agree that we need to understand what is going on and do a real
fix.

-Frank

On Sun, Sep 09, 2018 at 07:04:25PM +0200, Benjamin Herrenschmidt wrote:

On Fri, 2018-08-31 at 14:58 +1000, Benjamin Herrenschmidt wrote:

quoted

quoted

A long shot, but something to consider, is that I failed to cover the
cases of dynamic devicetree updates (removing nodes that contain a
phandle) in ways other than overlays.  Michael Ellerman has reported
such a problem for powerpc/mobility with of_detach_node().  A patch to
fix that is one of the tasks I need to complete.

The only thing I can think of is booting via the BootX bootloader on
those ancient macs results in a DT with no phandles. I didn't see an
obvious reason why that would cause that patch to break though.

Guys, we still don't have a fix for this one on its way upstream...

My test patch just creates phandle properties for all nodes, that was
not intended as a fix, more a way to check if the problem was related
to the lack of phandles.

I don't actually know why the new code causes things to fail when
phandles are absent. This needs to be looked at.

I'm travelling at the moment and generally caught up with other things,
I haven't had a chance to dig, so just a heads up. I don't intend to
submit my patch since it's just a band aid. We need to figure out what
the actual problem is.

Can you try this patch (w/o Ben's patch). I think the problem is if 
there are no phandles, then roundup_pow_of_two is passed 0 which is 
documented as undefined result.

Though, if a DT has no properties with phandles, then why are we doing a 
lookup in the first place?


8<----------------------------------------------------------------------

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 9095b8290150..74eaedd5b860 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c

@@ -140,6 +140,9 @@ void of_populate_phandle_cache(void)
 		if (np->phandle && np->phandle != OF_PHANDLE_ILLEGAL)
 			phandles++;
 
+	if (!phandles)
+		goto out;
+
 	cache_entries = roundup_pow_of_two(phandles);
 	phandle_cache_mask = cache_entries - 1;
 

On 09/10/18 05:53, Rob Herring wrote:

Thanks Rob!  That fix makes sense, and the test results look
promising.

Reviewed-by: Frank Rowand <redacted>