Re: 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()
[The Cc list got pruned so I'm forwarding Stan's reply for the benefit of the list archives and any other interested parties.] On Mon, 10 Sep 2018, Stan Johnson wrote: > On 9/10/18 6:53 AM, Rob Herring wrote: > > > ... > > 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; > > > > Using the attached .config file, I first compiled the stock 4.18 > kernel from kernel.org; it hung at the Mac OS background screen > on my Beige G3 Desktop using the BootX extension and the BootX.app > from within MacOS. I then applied the above patch, and it booted > without any problems from both the BootX extension and using the > BootX.app from within MacOS. > > -Stan > >
Re: 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()
On 09/10/18 05:53, Rob Herring wrote: > 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: >>> 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; > > Thanks Rob! That fix makes sense, and the test results look promising. Reviewed-by: Frank Rowand
Re: 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()
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: > > > > > 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;
Re: 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()
On 09/09/18 10:04, Benjamin Herrenschmidt wrote: > On Fri, 2018-08-31 at 14:58 +1000, Benjamin Herrenschmidt wrote: >> >>> 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
Re: 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()
On Fri, 2018-08-31 at 14:58 +1000, Benjamin Herrenschmidt wrote: > > > 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.
Re: 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()
On Fri, 2018-08-31 at 14:35 +1000, Benjamin Herrenschmidt wrote:
>
> > 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 = >next;
}
+ /* add a phandle */
+ if (bootx_phandle > 0) {
+ bootx_dt_add_prop("phandle", _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", _end);
bootx_dt_add_string("linux,bootx-linebytes", _end);
bootx_dt_add_string("linux,bootx-addr", _end);
+ if (bootx_phandle > 0)
+ bootx_dt_add_string("phandle", _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;
Re: 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()
On Sat, 2018-09-01 at 09:36 +1000, Finn Thain wrote: > > The patched kernel (Finn's vmlinux-4.18.0-1-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.
Re: 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()
On Fri, 2018-08-31 at 06:28 -0600, Mac User wrote: > 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-1-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.
Re: 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()
On Fri, 31 Aug 2018, Mac User wrote:
> 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).
>
> The patched kernel (Finn's vmlinux-4.18.0-1-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 = >next;
}
+ /* add a phandle */
+ if (bootx_phandle > 0) {
+ bootx_dt_add_prop("phandle", _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", _end);
bootx_dt_add_string("linux,bootx-linebytes", _end);
bootx_dt_add_string("linux,bootx-addr", _end);
+ if (bootx_phandle > 0)
+ bootx_dt_add_string("phandle", _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;
--
Re: 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()
On Thu, 2018-08-30 at 21:36 -0700, Frank Rowand wrote: > > 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.
Re: 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()
On Fri, 2018-08-31 at 14:35 +1000, Benjamin Herrenschmidt wrote:
>
> > 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 = >next;
}
+ /* add a phandle */
+ if (bootx_phandle > 0) {
+ bootx_dt_add_prop("phandle", _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", _end);
bootx_dt_add_string("linux,bootx-linebytes", _end);
bootx_dt_add_string("linux,bootx-addr", _end);
+ if (bootx_phandle > 0)
+ bootx_dt_add_string("phandle", _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;
Re: 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 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
Re: 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()
On Thu, 2018-08-30 at 20:39 -0600, Mac User wrote:
> On 8/29/18 7:05 PM, Rob Herring wrote:
>
> > On Wed, Aug 29, 2018 at 7:44 PM 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.
> >
> > 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
Re: 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()
On Wed, Aug 29, 2018 at 7:44 PM 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.
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
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.l...@gmail.com wrote:
> From: Frank Rowand
>
> Create a cache of the nodes that contain a phandle property. Use this
> cache to find the node for a given phandle value instead of scanning
> the devicetree to find the node. If the phandle value is not found
> in the cache, of_find_node_by_phandle() will fall back to the tree
> scan algorithm.
>
> The cache is initialized in of_core_init().
>
> The cache is freed via a late_initcall_sync() if modules are not
> enabled.
>
> If the devicetree is created by the dtc compiler, with all phandle
> property values auto generated, then the size required by the cache
> could be 4 * (1 + number of phandles) bytes. This results in an O(1)
> node lookup cost for a given phandle value. Due to a concern that the
> phandle property values might not be consistent with what is generated
> by the dtc compiler, a mask has been added to the cache lookup algorithm.
> To maintain the O(1) node lookup cost, the size of the cache has been
> increased by rounding the number of entries up to the next power of
> two.
>
> The overhead of finding the devicetree node containing a given phandle
> value has been noted by several people in the recent past, in some cases
> with a patch to add a hashed index of devicetree nodes, based on the
> phandle value of the node. One concern with this approach is the extra
> space added to each node. This patch takes advantage of the phandle
> property values auto generated by the dtc compiler, which begin with
> one and monotonically increase by one, resulting in a range of 1..n
> for n phandle values. This implementation should also provide a good
> reduction of overhead for any range of phandle values that are mostly
> in a monotonic range.
>
> Performance measurements by Chintan Pandya
> of several implementations of patches that are similar to this one
> suggest an expected reduction of boot time by ~400ms for his test
> system. If the cache size was decreased to 64 entries, the boot
> time was reduced by ~340 ms. The measurements were on a 4.9.73 kernel
> for arch/arm64/boot/dts/qcom/sda670-mtp.dts, contains 2371 nodes and
> 814 phandle values.
>
> Reported-by: Chintan Pandya
> Signed-off-by: Frank Rowand
> ---
>
> Changes since v3:
> - of_populate_phandle_cache(): add check for failed memory allocation
>
> Changes since v2:
> - add mask to calculation of phandle cache entry
> - which results in better overhead reduction for devicetrees with
> phandle properties not allocated in the monotonically increasing
> range of 1..n
> - due to mask, number of entries in cache potentially increased to
> next power of two
> - minor fixes as suggested by reviewers
> - no longer using live_tree_max_phandle() so do not move it from
> drivers/of/resolver.c to drivers/of/base.c
>
> Changes since v1:
> - change short description from
> of: cache phandle nodes to reduce cost of of_find_node_by_phandle()
> - rebase on v4.16-rc1
> - reorder new functions in base.c to avoid forward declaration
> - add locking around kfree(phandle_cache) for memory ordering
> - add explicit check for non-null of phandle_cache in
> of_find_node_by_phandle(). There is already a check for !handle,
> which prevents accessing a null phandle_cache, but that dependency
> is not obvious, so this check makes it more apparent.
> - do not free phandle_cache if modules are enabled, so that
> cached phandles will be available when modules are loaded
>
> drivers/of/base.c | 86
> ++---
> drivers/of/of_private.h | 3 ++
> drivers/of/resolver.c | 3 --
> 3 files changed, 85 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index ad28de96e13f..e71d157d7149 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -91,10 +91,72 @@ int __weak of_node_to_nid(struct device_node *np)
> }
> #endif
>
> +static struct device_node **phandle_cache;
> +static u32 phandle_cache_mask;
>
