[PATCH v5 0/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle()

[PATCH v5 0/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle()

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

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.

Size and performance data is in patch 1/3 comments

Changes since v4:
  - Add patch 2/3 "memblock: add memblock_free() alloc when
    CONFIG_HAVE_MEMBLOCK is not set"
  - patch 3/3 (previously 2/2): add checks for zero returned by
    memblock_virt_alloc()

Changes since v3:
  - of_populate_phandle_cache(): add check for failed memory allocation
  - add patch 2/2 into series instead of as a standalone patch that was
    dependent on patch 1/2 of this series

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



Frank Rowand (3):
  of: cache phandle nodes to reduce cost of of_find_node_by_phandle()
  memblock: add memblock_free() alloc when CONFIG_HAVE_MEMBLOCK is not
    set
  of: add early boot allocation of of_find_node_by_phandle() cache

 drivers/of/base.c        | 124 +++++++++++++++++++++++++++++++++++++++++++++--
 drivers/of/fdt.c         |   2 +
 drivers/of/of_private.h  |   5 ++
 drivers/of/resolver.c    |   3 --
 include/linux/memblock.h |   4 ++
 5 files changed, 131 insertions(+), 7 deletions(-)

-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH v5 1/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle()

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

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 <cpandya@codeaurora.org>
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 <cpandya@codeaurora.org>
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---

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;
+
+/*
+ * Assumptions behind phandle_cache implementation:
+ *   - phandle property values are in a contiguous range of 1..n
+ *
+ * If the assumptions do not hold, then
+ *   - the phandle lookup overhead reduction provided by the cache
+ *     will likely be less
+ */
+static void of_populate_phandle_cache(void)
+{
+	unsigned long flags;
+	u32 cache_entries;
+	struct device_node *np;
+	u32 phandles = 0;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+
+	kfree(phandle_cache);
+	phandle_cache = NULL;
+
+	for_each_of_allnodes(np)
+		if (np->phandle && np->phandle != OF_PHANDLE_ILLEGAL)
+			phandles++;
+
+	cache_entries = roundup_pow_of_two(phandles);
+	phandle_cache_mask = cache_entries - 1;
+
+	phandle_cache = kcalloc(cache_entries, sizeof(*phandle_cache),
+				GFP_ATOMIC);
+	if (!phandle_cache)
+		goto out;
+
+	for_each_of_allnodes(np)
+		if (np->phandle && np->phandle != OF_PHANDLE_ILLEGAL)
+			phandle_cache[np->phandle & phandle_cache_mask] = np;
+
+out:
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+}
+
+#ifndef CONFIG_MODULES
+static int __init of_free_phandle_cache(void)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&devtree_lock, flags);
+
+	kfree(phandle_cache);
+	phandle_cache = NULL;
+
+	raw_spin_unlock_irqrestore(&devtree_lock, flags);
+
+	return 0;
+}
+late_initcall_sync(of_free_phandle_cache);
+#endif
+
 void __init of_core_init(void)
 {
 	struct device_node *np;
 
+	of_populate_phandle_cache();
+
 	/* Create the kset, and register existing nodes */
 	mutex_lock(&of_mutex);
 	of_kset = kset_create_and_add("devicetree", NULL, firmware_kobj);
@@ -1021,16 +1083,32 @@ int of_modalias_node(struct device_node *node, char *modalias, int len)
  */
 struct device_node *of_find_node_by_phandle(phandle handle)
 {
-	struct device_node *np;
+	struct device_node *np = NULL;
 	unsigned long flags;
+	phandle masked_handle;
 
 	if (!handle)
 		return NULL;
 
 	raw_spin_lock_irqsave(&devtree_lock, flags);
-	for_each_of_allnodes(np)
-		if (np->phandle == handle)
-			break;
+
+	masked_handle = handle & phandle_cache_mask;
+
+	if (phandle_cache) {
+		if (phandle_cache[masked_handle] &&
+		    handle == phandle_cache[masked_handle]->phandle)
+			np = phandle_cache[masked_handle];
+	}
+
+	if (!np) {
+		for_each_of_allnodes(np)
+			if (np->phandle == handle) {
+				if (phandle_cache)
+					phandle_cache[masked_handle] = np;
+				break;
+			}
+	}
+
 	of_node_get(np);
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 	return np;
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index 0c609e7d0334..fa70650136b4 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -131,6 +131,9 @@ extern void __of_update_property_sysfs(struct device_node *np,
 extern void __of_sysfs_remove_bin_file(struct device_node *np,
 				       struct property *prop);
 
+/* illegal phandle value (set when unresolved) */
+#define OF_PHANDLE_ILLEGAL	0xdeadbeef
+
 /* iterators for transactions, used for overlays */
 /* forward iterator */
 #define for_each_transaction_entry(_oft, _te) \
diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
index 740d19bde601..b2ca8185c8c6 100644
--- a/drivers/of/resolver.c
+++ b/drivers/of/resolver.c
@@ -19,9 +19,6 @@
 
 #include "of_private.h"
 
-/* illegal phandle value (set when unresolved) */
-#define OF_PHANDLE_ILLEGAL	0xdeadbeef
-
 static phandle live_tree_max_phandle(void)
 {
 	struct device_node *node;
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH v5 2/3] memblock: add memblock_free() alloc when CONFIG_HAVE_MEMBLOCK is not set

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

When CONFIG_HAVE_MEMBLOCK is not set, an error version of
memblock_alloc() exists.  Add the matching memblock_free().

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---

Andrew or Michal, can you please ack this patch to be accepted
by Rob?


With "of: add early boot allocation of
of_find_node_by_phandle() cache" applied, kbuild test robot reports
tilex architecture build error due to no prototype for memblock_free().

The version of the patch that kbuild test robot reported was
"[PATCH v4 2/2] of: add early boot allocation of
of_find_node_by_phandle() cache".  An updated version of that
patch is now patch 3/3 of this series.

 include/linux/memblock.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 8be5077efb5f..d6973b1d92bc 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -432,6 +432,10 @@ static inline unsigned long memblock_reserved_memory_within(phys_addr_t start_ad
 	return 0;
 }
 
+static inline int memblock_free(phys_addr_t base, phys_addr_t size)
+{
+	return 0;
+}
 #endif /* CONFIG_HAVE_MEMBLOCK */
 
 #endif /* __KERNEL__ */
-- 
Frank Rowand <frank.rowand@sony.com>

[PATCH v5 3/3] of: add early boot allocation of of_find_node_by_phandle() cache

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

The initial implementation of the of_find_node_by_phandle() cache
allocates the cache using kcalloc().  Add an early boot allocation
of the cache so it will be usable during early boot.  Switch over
to the kcalloc() based cache once normal memory allocation
becomes available.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---

Changes from v4:
  - Add checks for memblock_virt_alloc() returning zero.

Changes from previous versions:
  - no changes

Version 5 adds checks for memblock_virt_alloc() returning zero.

kernel test robot reports 10 out of 10 boot failures on
qemu-system-x86_64 with "[PATCH v4 2/2] of: add early boot
allocation of of_find_node_by_phandle() cache" applied.

The failure appears to me to be due to memblock_virt_alloc() returning
zero.

kernel BUG at arch/x86/mm/physaddr.c:27
invalid opcode: 0000 [#1] PREEMPT PTI
CPU: 0 PID: 1 Comm: swapper Not tainted 4.16.0-rc1-00008-gb013aa4 #1
RIP: 0010:__phys_addr+0x39/0x50
RSP: 0000:ffff880018de3ee0 EFLAGS: 00010087
RAX: 0000780000000000 RBX: 0000000000000001 RCX: 0000000000000002
RDX: 0000000080000000 RSI: ffffffff82e77239 RDI: 0000000000000000
RBP: ffff880018de3ee0 R08: 0000000000000000 R09: 0000000000000001
R10: 00000000000029cb R11: 63561fc2891644ad R12: 0000000000000286
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffffffff8309b000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00000000ffffffff CR3: 0000000003074000 CR4: 00000000000006f0
Call Trace:
of_core_init+0x30/0x21f
driver_init+0x36/0x38
kernel_init_freeable+0x82/0x19f
? rest_init+0x220/0x220
kernel_init+0xe/0x100
ret_from_fork+0x24/0x30

I'm not conversent in the x86 dialect of assembly, but it looks
like phandle_cache has the value zero, when calling __phys_addr():

==== the call from of_core_init() to __pa(phandle_cache) aka
__physaddr():

186		size = (phandle_cache_mask + 1) * sizeof(*phandle_cache);
   0xffffffff83334d35 <+27>:	mov    0x1771e1d(%rip),%eax        # 0xffffffff84aa6b58 <phandle_cache_mask>
   0xffffffff83334d42 <+40>:	lea    0x1(%rax),%r12d
   0xffffffff83334d4b <+49>:	shl    $0x3,%r12

187		memblock_free(__pa(phandle_cache), size);
   0xffffffff83334d3b <+33>:	mov    0x1771e1e(%rip),%rdi        # 0xffffffff84aa6b60 <phandle_cache>
   0xffffffff83334d46 <+44>:	callq  0xffffffff81049ad0 <__phys_addr>
   0xffffffff83334d4f <+53>:	mov    %rax,%rdi
   0xffffffff83334d52 <+56>:	mov    %r12,%rsi
   0xffffffff83334d55 <+59>:	callq  0xffffffff8334e45b <memblock_free>

188		phandle_cache = NULL;
   0xffffffff83334d64 <+74>:	movq   $0x0,0x1771df1(%rip)        # 0xffffffff84aa6b60 <phandle_cache>

==== __pa():

(gdb) disass /m __phys_addr
Dump of assembler code for function __phys_addr:
15	{
   0xffffffff81049ad0 <+0>:	callq  0xffffffff822018e0 <__fentry__>
   0xffffffff81049ad5 <+5>:	push   %rbp
   0xffffffff81049ade <+14>:	mov    %rsp,%rbp

16		unsigned long y = x - __START_KERNEL_map;
   0xffffffff81049ad6 <+6>:	mov    $0x80000000,%eax

17
18		/* use the carry flag to determine if x was < __START_KERNEL_map */
19		if (unlikely(x > y)) {
   0xffffffff81049adb <+11>:	add    %rdi,%rax
   0xffffffff81049ae1 <+17>:	mov    %rax,%rdx
   0xffffffff81049ae4 <+20>:	jae    0xffffffff81049af8 <__phys_addr+40>

20			x = y + phys_base;
   0xffffffff81049ae6 <+22>:	add    0x1e30523(%rip),%rax        # 0xffffffff82e7a010

21
22			VIRTUAL_BUG_ON(y >= KERNEL_IMAGE_SIZE);
   0xffffffff81049aed <+29>:	cmp    $0x1fffffff,%rdx
   0xffffffff81049af4 <+36>:	jbe    0xffffffff81049b1e <__phys_addr+78>
   0xffffffff81049af6 <+38>:	ud2

23		} else {
24			x = y + (__START_KERNEL_map - PAGE_OFFSET);
   0xffffffff81049af8 <+40>:	movabs $0x780000000000,%rax
   0xffffffff81049b02 <+50>:	add    %rdi,%rax

25
26			/* carry flag will be set if starting x was >= PAGE_OFFSET */
27			VIRTUAL_BUG_ON((x > y) || !phys_addr_valid(x));
   0xffffffff81049b05 <+53>:	cmp    %rax,%rdx
   0xffffffff81049b08 <+56>:	jb     0xffffffff81049b1c <__phys_addr+76>
   0xffffffff81049b17 <+71>:	test   %rdx,%rdx
   0xffffffff81049b1a <+74>:	je     0xffffffff81049b1e <__phys_addr+78>
   0xffffffff81049b1c <+76>:	ud2

28		}
29
30		return x;
31	}
   0xffffffff81049b1e <+78>:	pop    %rbp
   0xffffffff81049b1f <+79>:	retq


 drivers/of/base.c       | 38 ++++++++++++++++++++++++++++++++++++++
 drivers/of/fdt.c        |  2 ++
 drivers/of/of_private.h |  2 ++
 3 files changed, 42 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index e71d157d7149..48714d8e0bc9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -16,9 +16,11 @@
 
 #define pr_fmt(fmt)	"OF: " fmt
 
+#include <linux/bootmem.h>
 #include <linux/console.h>
 #include <linux/ctype.h>
 #include <linux/cpu.h>
+#include <linux/memblock.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -134,6 +136,32 @@ static void of_populate_phandle_cache(void)
 	raw_spin_unlock_irqrestore(&devtree_lock, flags);
 }
 
+void __init of_populate_phandle_cache_early(void)
+{
+	u32 cache_entries;
+	struct device_node *np;
+	u32 phandles = 0;
+	size_t size;
+
+	for_each_of_allnodes(np)
+		if (np->phandle && np->phandle != OF_PHANDLE_ILLEGAL)
+			phandles++;
+
+	cache_entries = roundup_pow_of_two(phandles);
+	phandle_cache_mask = cache_entries - 1;
+
+	size = cache_entries * sizeof(*phandle_cache);
+	phandle_cache = memblock_virt_alloc(size, 4);
+	if (!phandle_cache)
+		return;
+
+	memset(phandle_cache, 0, size);
+
+	for_each_of_allnodes(np)
+		if (np->phandle && np->phandle != OF_PHANDLE_ILLEGAL)
+			phandle_cache[np->phandle & phandle_cache_mask] = np;
+}
+
 #ifndef CONFIG_MODULES
 static int __init of_free_phandle_cache(void)
 {
@@ -153,7 +181,17 @@ static int __init of_free_phandle_cache(void)
 
 void __init of_core_init(void)
 {
+	unsigned long flags;
 	struct device_node *np;
+	phys_addr_t size;
+
+	if (phandle_cache) {
+		raw_spin_lock_irqsave(&devtree_lock, flags);
+		size = (phandle_cache_mask + 1) * sizeof(*phandle_cache);
+		memblock_free(__pa(phandle_cache), size);
+		phandle_cache = NULL;
+		raw_spin_unlock_irqrestore(&devtree_lock, flags);
+	}
 
 	of_populate_phandle_cache();
 
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 84aa9d676375..cb320df23f26 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -1264,6 +1264,8 @@ void __init unflatten_device_tree(void)
 	of_alias_scan(early_init_dt_alloc_memory_arch);
 
 	unittest_unflatten_overlay_base();
+
+	of_populate_phandle_cache_early();
 }
 
 /**
diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
index fa70650136b4..6720448c84cc 100644
--- a/drivers/of/of_private.h
+++ b/drivers/of/of_private.h
@@ -134,6 +134,8 @@ extern void __of_sysfs_remove_bin_file(struct device_node *np,
 /* illegal phandle value (set when unresolved) */
 #define OF_PHANDLE_ILLEGAL	0xdeadbeef
 
+extern void __init of_populate_phandle_cache_early(void);
+
 /* iterators for transactions, used for overlays */
 /* forward iterator */
 #define for_each_transaction_entry(_oft, _te) \
-- 
Frank Rowand <frank.rowand@sony.com>

Re: [PATCH v5 3/3] of: add early boot allocation of of_find_node_by_phandle() cache

[Rob Herring]

On Sun, Mar 4, 2018 at 6:14 PM,  <frowand.list@gmail.com> wrote:
> From: Frank Rowand <frank.rowand@sony.com>
>
> The initial implementation of the of_find_node_by_phandle() cache
> allocates the cache using kcalloc().  Add an early boot allocation
> of the cache so it will be usable during early boot.  Switch over
> to the kcalloc() based cache once normal memory allocation
> becomes available.

Do we get a lot of lookups early? It makes sense to me to do early,
but freeing and repopulating seems to add needless complexity.

Rob

Re: [PATCH v5 2/3] memblock: add memblock_free() alloc when CONFIG_HAVE_MEMBLOCK is not set

[Andrew Morton]

On Sun,  4 Mar 2018 16:14:48 -0800 frowand.list@gmail.com wrote:

> From: Frank Rowand <frank.rowand@sony.com>
> 
> When CONFIG_HAVE_MEMBLOCK is not set, an error version of
> memblock_alloc() exists.  Add the matching memblock_free().
> 
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
> 
> Andrew or Michal, can you please ack this patch to be accepted
> by Rob?
> 
> 
> With "of: add early boot allocation of
> of_find_node_by_phandle() cache" applied, kbuild test robot reports
> tilex architecture build error due to no prototype for memblock_free().
> 
> The version of the patch that kbuild test robot reported was
> "[PATCH v4 2/2] of: add early boot allocation of
> of_find_node_by_phandle() cache".  An updated version of that
> patch is now patch 3/3 of this series.
> 
> ...
>
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -432,6 +432,10 @@ static inline unsigned long memblock_reserved_memory_within(phys_addr_t start_ad
>  	return 0;
>  }
>  
> +static inline int memblock_free(phys_addr_t base, phys_addr_t size)
> +{
> +	return 0;
> +}
>  #endif /* CONFIG_HAVE_MEMBLOCK */
>  
>  #endif /* __KERNEL__ */

Acked-by: Andrew Morton <akpm@linux-foundation.org>

Re: [PATCH v5 3/3] of: add early boot allocation of of_find_node_by_phandle() cache

[Frank Rowand]

On 03/05/18 12:26, Rob Herring wrote:
> On Sun, Mar 4, 2018 at 6:14 PM,  <frowand.list@gmail.com> wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> The initial implementation of the of_find_node_by_phandle() cache
>> allocates the cache using kcalloc().  Add an early boot allocation
>> of the cache so it will be usable during early boot.  Switch over
>> to the kcalloc() based cache once normal memory allocation
>> becomes available.
> 
> Do we get a lot of lookups early? It makes sense to me to do early,
> but freeing and repopulating seems to add needless complexity.

Hey, that was my argument.  :-)

I would be quite happy leaving off patch 3/3.  And without 3/3,
2/3 becomes not necessary here, but still could go in separately
for completeness.  Though it could go in via Andrew.

-Frank

Re: [PATCH v5 1/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle()

[Rob Herring]

On Sun, Mar 04, 2018 at 04:14:47PM -0800, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> 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 <cpandya@codeaurora.org>
> 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 <cpandya@codeaurora.org>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---

I've applied this one, but not the others.

Rob

Re: [PATCH v5 1/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle()

[Frank Rowand]

On 03/09/18 15:03, Rob Herring wrote:
> On Sun, Mar 04, 2018 at 04:14:47PM -0800, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> 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 <cpandya@codeaurora.org>
>> 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 <cpandya@codeaurora.org>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
> 
> I've applied this one, but not the others.
> 
> Rob
> 

Thank you.  I'll let the other two disappear into a distant memory.

-Frank

Re: [PATCH v5 1/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle()

[Alan Tull]

On Sun, Mar 4, 2018 at 6:14 PM,  <frowand.list@gmail.com> wrote:

Hi Frank,

I'm investigating a refcount use-after-free warning that happens after
overlays are applied, removed, reapplied a few (typically three) times
(see below).  This is new in v4.17, didn't happen in v4.16.  As I was
investigating I found that rebuilding the phandle_cache after overlays
are applied or removed seems to help.  I exported
of_populate_phandle_cache() and called it after overlays were applied
or removed such as the snippet below, it seemed to fix the problem.
I'll keep digging to understand the problem better.

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 848f549..4184273 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -102,7 +102,7 @@ static u32 phandle_cache_mask;
  *   - the phandle lookup overhead reduction provided by the cache
  *     will likely be less
  */
-static void of_populate_phandle_cache(void)
+void of_populate_phandle_cache(void)
 {
        unsigned long flags;
        u32 cache_entries;
@@ -133,6 +133,7 @@ static void of_populate_phandle_cache(void)
 out:
        raw_spin_unlock_irqrestore(&devtree_lock, flags);
 }
+EXPORT_SYMBOL_GPL(of_populate_phandle_cache);

 #ifndef CONFIG_MODULES
 static int __init of_free_phandle_cache(void)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 7baa53e..55caf42 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -885,6 +885,8 @@ int of_overlay_fdt_apply(const void *overlay_fdt,
u32 overlay_fdt_size,
                goto out;
        }

+       of_populate_phandle_cache();
+
        return 0;


@@ -1070,6 +1072,7 @@ int of_overlay_remove(int *ovcs_id)
        }

        free_overlay_changeset(ovcs);
+       of_populate_phandle_cache();

 out_unlock:
        mutex_unlock(&of_mutex);
diff --git a/include/linux/of.h b/include/linux/of.h
index 4d25e4f..a662d4e 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1342,6 +1342,9 @@ static inline int
of_reconfig_get_state_change(unsigned long action,
 }
 #endif /* CONFIG_OF_DYNAMIC */

+//todo locate this more correctly, just testing for now
+void of_populate_phandle_cache(void);
+
 /**
  * of_device_is_system_power_controller - Tells if
system-power-controller is found for device_node
  * @np: Pointer to the given device_node


Dump (with my added pr_err's in of_node_get) without the above snippet
to help out.

[  226.115974] OF: Got from phandle_cache np=/soc/base_fpga_region/
[  226.121956] OF: about to get np=/soc/base_fpga_region/
[  226.127073] ------------[ cut here ]------------
[  226.131682] WARNING: CPU: 1 PID: 1529 at
/home/atull/repos/linux-socfpga/lib/refcount.c:153
refcount_inc+0x4c/0x50
[  226.141988] refcount_t: increment on 0; use-after-free.
[  226.147191] Modules linked in:
[  226.150241] CPU: 1 PID: 1529 Comm: python Not tainted
4.17.0-00134-gb6fd158 #10
[  226.157521] Hardware name: Altera SOCFPGA Arria10
[  226.162223] [<c01132d8>] (unwind_backtrace) from [<c010defc>]
(show_stack+0x20/0x24)
[  226.169943] [<c010defc>] (show_stack) from [<c07be4e0>]
(dump_stack+0x8c/0xa0)
[  226.177146] [<c07be4e0>] (dump_stack) from [<c0123620>] (__warn+0x104/0x11c)
[  226.184170] [<c0123620>] (__warn) from [<c012368c>]
(warn_slowpath_fmt+0x54/0x70)
[  226.191631] [<c012368c>] (warn_slowpath_fmt) from [<c0481ac8>]
(refcount_inc+0x4c/0x50)
[  226.199613] [<c0481ac8>] (refcount_inc) from [<c07c2ff4>]
(kobject_get+0x2c/0x5c)
[  226.207073] [<c07c2ff4>] (kobject_get) from [<c06485bc>]
(of_node_get.part.0+0x30/0x44)
[  226.215050] [<c06485bc>] (of_node_get.part.0) from [<c06485f8>]
(of_node_get+0x28/0x2c)
[  226.223028] [<c06485f8>] (of_node_get) from [<c06434c8>]
(of_find_node_by_phandle+0xa0/0xec)
[  226.231438] [<c06434c8>] (of_find_node_by_phandle) from
[<c06435c4>] (of_phandle_iterator_next+0xb0/0x178)
[  226.241057] [<c06435c4>] (of_phandle_iterator_next) from
[<c0643f48>] (__of_parse_phandle_with_args+0x50/0xf8)
[  226.251022] [<c0643f48>] (__of_parse_phandle_with_args) from
[<c0644038>] (of_parse_phandle+0x48/0x78)
[  226.260298] [<c0644038>] (of_parse_phandle) from [<c0657424>]
(of_fpga_region_get_bridges+0x140/0x1d0)
[  226.269572] [<c0657424>] (of_fpga_region_get_bridges) from
[<c0657070>] (fpga_region_program_fpga+0x98/0x184)
[  226.279450] [<c0657070>] (fpga_region_program_fpga) from
[<c06578b4>] (of_fpga_region_notify+0x2c4/0x340)
[  226.288984] [<c06578b4>] (of_fpga_region_notify) from [<c01472f4>]
(notifier_call_chain+0x54/0x94)
[  226.297913] [<c01472f4>] (notifier_call_chain) from [<c01476d8>]
(__blocking_notifier_call_chain+0x58/0x70)
[  226.307619] [<c01476d8>] (__blocking_notifier_call_chain) from
[<c0147718>] (blocking_notifier_call_chain+0x28/0x30)
[  226.318105] [<c0147718>] (blocking_notifier_call_chain) from
[<c064e930>] (overlay_notify+0x8c/0xec)
[  226.327206] [<c064e930>] (overlay_notify) from [<c064f2f4>]
(of_overlay_fdt_apply+0x41c/0x714)
[  226.335791] [<c064f2f4>] (of_overlay_fdt_apply) from [<c06481dc>]
(cfs_overlay_item_dtbo_write+0x68/0xbc)
[  226.345327] [<c06481dc>] (cfs_overlay_item_dtbo_write) from
[<c02e3d38>] (configfs_release_bin_file+0x6c/0xa0)
[  226.355294] [<c02e3d38>] (configfs_release_bin_file) from
[<c026b920>] (__fput+0x94/0x1e4)
[  226.363530] [<c026b920>] (__fput) from [<c026bae0>] (____fput+0x18/0x1c)
[  226.370207] [<c026bae0>] (____fput) from [<c0143aa8>]
(task_work_run+0xb4/0xd8)
[  226.377492] [<c0143aa8>] (task_work_run) from [<c010d438>]
(do_work_pending+0xac/0xcc)
[  226.385382] [<c010d438>] (do_work_pending) from [<c010106c>]
(slow_work_pending+0xc/0x20)

Alan

> From: Frank Rowand <frank.rowand@sony.com>
>
> 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 <cpandya@codeaurora.org>
> 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 <cpandya@codeaurora.org>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>
> 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;
> +
> +/*
> + * Assumptions behind phandle_cache implementation:
> + *   - phandle property values are in a contiguous range of 1..n
> + *
> + * If the assumptions do not hold, then
> + *   - the phandle lookup overhead reduction provided by the cache
> + *     will likely be less
> + */
> +static void of_populate_phandle_cache(void)
> +{
> +       unsigned long flags;
> +       u32 cache_entries;
> +       struct device_node *np;
> +       u32 phandles = 0;
> +
> +       raw_spin_lock_irqsave(&devtree_lock, flags);
> +
> +       kfree(phandle_cache);
> +       phandle_cache = NULL;
> +
> +       for_each_of_allnodes(np)
> +               if (np->phandle && np->phandle != OF_PHANDLE_ILLEGAL)
> +                       phandles++;
> +
> +       cache_entries = roundup_pow_of_two(phandles);
> +       phandle_cache_mask = cache_entries - 1;
> +
> +       phandle_cache = kcalloc(cache_entries, sizeof(*phandle_cache),
> +                               GFP_ATOMIC);
> +       if (!phandle_cache)
> +               goto out;
> +
> +       for_each_of_allnodes(np)
> +               if (np->phandle && np->phandle != OF_PHANDLE_ILLEGAL)
> +                       phandle_cache[np->phandle & phandle_cache_mask] = np;
> +
> +out:
> +       raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +}
> +
> +#ifndef CONFIG_MODULES
> +static int __init of_free_phandle_cache(void)
> +{
> +       unsigned long flags;
> +
> +       raw_spin_lock_irqsave(&devtree_lock, flags);
> +
> +       kfree(phandle_cache);
> +       phandle_cache = NULL;
> +
> +       raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +
> +       return 0;
> +}
> +late_initcall_sync(of_free_phandle_cache);
> +#endif
> +
>  void __init of_core_init(void)
>  {
>         struct device_node *np;
>
> +       of_populate_phandle_cache();
> +
>         /* Create the kset, and register existing nodes */
>         mutex_lock(&of_mutex);
>         of_kset = kset_create_and_add("devicetree", NULL, firmware_kobj);
> @@ -1021,16 +1083,32 @@ int of_modalias_node(struct device_node *node, char *modalias, int len)
>   */
>  struct device_node *of_find_node_by_phandle(phandle handle)
>  {
> -       struct device_node *np;
> +       struct device_node *np = NULL;
>         unsigned long flags;
> +       phandle masked_handle;
>
>         if (!handle)
>                 return NULL;
>
>         raw_spin_lock_irqsave(&devtree_lock, flags);
> -       for_each_of_allnodes(np)
> -               if (np->phandle == handle)
> -                       break;
> +
> +       masked_handle = handle & phandle_cache_mask;
> +
> +       if (phandle_cache) {
> +               if (phandle_cache[masked_handle] &&
> +                   handle == phandle_cache[masked_handle]->phandle)
> +                       np = phandle_cache[masked_handle];
> +       }
> +
> +       if (!np) {
> +               for_each_of_allnodes(np)
> +                       if (np->phandle == handle) {
> +                               if (phandle_cache)
> +                                       phandle_cache[masked_handle] = np;
> +                               break;
> +                       }
> +       }
> +
>         of_node_get(np);
>         raw_spin_unlock_irqrestore(&devtree_lock, flags);
>         return np;
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 0c609e7d0334..fa70650136b4 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -131,6 +131,9 @@ extern void __of_update_property_sysfs(struct device_node *np,
>  extern void __of_sysfs_remove_bin_file(struct device_node *np,
>                                        struct property *prop);
>
> +/* illegal phandle value (set when unresolved) */
> +#define OF_PHANDLE_ILLEGAL     0xdeadbeef
> +
>  /* iterators for transactions, used for overlays */
>  /* forward iterator */
>  #define for_each_transaction_entry(_oft, _te) \
> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> index 740d19bde601..b2ca8185c8c6 100644
> --- a/drivers/of/resolver.c
> +++ b/drivers/of/resolver.c
> @@ -19,9 +19,6 @@
>
>  #include "of_private.h"
>
> -/* illegal phandle value (set when unresolved) */
> -#define OF_PHANDLE_ILLEGAL     0xdeadbeef
> -
>  static phandle live_tree_max_phandle(void)
>  {
>         struct device_node *node;
> --
> Frank Rowand <frank.rowand@sony.com>
>

Re: [PATCH v5 1/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle()

[Alan Tull]

On Tue, Jun 12, 2018 at 1:16 PM, Alan Tull <atull@kernel.org> wrote:
> On Sun, Mar 4, 2018 at 6:14 PM,  <frowand.list@gmail.com> wrote:
>
> Hi Frank,
>
> I'm investigating a refcount use-after-free warning that happens after
> overlays are applied, removed, reapplied a few (typically three) times
> (see below).  This is new in v4.17, didn't happen in v4.16.  As I was
> investigating I found that rebuilding the phandle_cache after overlays
> are applied or removed seems to help.

I was probably wrong about this.  The more I look at the phandle_cache code,
the more it looks looks good and straightforward.  Probably disabling
phandle_cache is 'fixing' things through some weird side effect.  I'll
keep investigating.  Sorry for the noise.

Alan

Re: [PATCH v5 1/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle()

[Frank Rowand]

On 06/13/18 07:42, Alan Tull wrote:
> On Tue, Jun 12, 2018 at 1:16 PM, Alan Tull <atull@kernel.org> wrote:
>> On Sun, Mar 4, 2018 at 6:14 PM,  <frowand.list@gmail.com> wrote:
>>
>> Hi Frank,
>>
>> I'm investigating a refcount use-after-free warning that happens after
>> overlays are applied, removed, reapplied a few (typically three) times
>> (see below).  This is new in v4.17, didn't happen in v4.16.  As I was
>> investigating I found that rebuilding the phandle_cache after overlays
>> are applied or removed seems to help.
> 
> I was probably wrong about this.  The more I look at the phandle_cache code,
> the more it looks looks good and straightforward.  Probably disabling
> phandle_cache is 'fixing' things through some weird side effect.  I'll
> keep investigating.  Sorry for the noise.

I suspect that you have found an issue, even if it is not the cause of
the refcount issue.  I noted in a reply to v4 of the patch:

  >> +static void of_populate_phandle_cache(void)
  >> +{
  >> +Â Â Â  unsigned long flags;
  >> +Â Â Â  u32 cache_entries;
  >> +Â Â Â  struct device_node *np;
  >> +Â Â Â  u32 phandles = 0;
  >> +
  >> +Â Â Â  raw_spin_lock_irqsave(&devtree_lock, flags);
  >> +
  >> +Â Â Â  kfree(phandle_cache);
  > 
  > I couldn't understood this. Everything else looks good to me.

  I will be adding a call to of_populate_phandle_cache() from the
  devicetree overlay code.  I put the kfree here so that the previous
  cache memory is freed when a new cache is created.
  
  Adding the call from the overlay code is not done in this
  series because I have a patch series modifying overlays and
  I do not want to create a conflict or ordering between that
  series and that patch.  The lack of the call from overlay
  code means that overlay code will gain some of the overhead
  reduction from this patch, but possibly not the entire reduction.

Sorry I'm not giving a link to the archive of this message - I have
a class I have to go to so I don't have enough time to find it.  The
email was

  Subject: Re: [PATCH v3] of: cache phandle nodes to reduce cost of
  of_find_node_by_phandle()
  Date: Fri, 16 Feb 2018 14:20:22 -0800
  Message-ID: <46d5fc76-33e3-d54a-26b8-e9bb8332924d@gmail.com>

Quickly looking at the current code, I don't see the overlay patch
that I mentioned.  I have to dig into what happened to that.

Leaving a phandle from an overlay in the phandle cache after the
overlay is removed would clearly be a bug.

-Frank

Re: [PATCH v5 1/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle()

[Alan Tull]

On Wed, Jun 13, 2018 at 4:47 PM, Frank Rowand <frowand.list@gmail.com> wrote:
> On 06/13/18 07:42, Alan Tull wrote:
>> On Tue, Jun 12, 2018 at 1:16 PM, Alan Tull <atull@kernel.org> wrote:
>>> On Sun, Mar 4, 2018 at 6:14 PM,  <frowand.list@gmail.com> wrote:
>>>
>>> Hi Frank,
>>>
>>> I'm investigating a refcount use-after-free warning that happens after
>>> overlays are applied, removed, reapplied a few (typically three) times
>>> (see below).  This is new in v4.17, didn't happen in v4.16.  As I was
>>> investigating I found that rebuilding the phandle_cache after overlays
>>> are applied or removed seems to help.
>>
>> I was probably wrong about this.  The more I look at the phandle_cache code,
>> the more it looks looks good and straightforward.  Probably disabling
>> phandle_cache is 'fixing' things through some weird side effect.  I'll
>> keep investigating.  Sorry for the noise.
>
> I suspect that you have found an issue, even if it is not the cause of
> the refcount issue.  I noted in a reply to v4 of the patch:
>
>   >> +static void of_populate_phandle_cache(void)
>   >> +{
>   >> +Â Â Â  unsigned long flags;
>   >> +Â Â Â  u32 cache_entries;
>   >> +Â Â Â  struct device_node *np;
>   >> +Â Â Â  u32 phandles = 0;
>   >> +
>   >> +Â Â Â  raw_spin_lock_irqsave(&devtree_lock, flags);
>   >> +
>   >> +Â Â Â  kfree(phandle_cache);
>   >
>   > I couldn't understood this. Everything else looks good to me.
>
>   I will be adding a call to of_populate_phandle_cache() from the
>   devicetree overlay code.  I put the kfree here so that the previous
>   cache memory is freed when a new cache is created.
>
>   Adding the call from the overlay code is not done in this
>   series because I have a patch series modifying overlays and
>   I do not want to create a conflict or ordering between that
>   series and that patch.  The lack of the call from overlay
>   code means that overlay code will gain some of the overhead
>   reduction from this patch, but possibly not the entire reduction.
>
> Sorry I'm not giving a link to the archive of this message - I have
> a class I have to go to so I don't have enough time to find it.

I understand, that's cool.  I've found the email chain, thanks for
pointing me to it!

>  The
> email was
>
>   Subject: Re: [PATCH v3] of: cache phandle nodes to reduce cost of
>   of_find_node_by_phandle()
>   Date: Fri, 16 Feb 2018 14:20:22 -0800
>   Message-ID: <46d5fc76-33e3-d54a-26b8-e9bb8332924d@gmail.com>
>
> Quickly looking at the current code, I don't see the overlay patch
> that I mentioned.  I have to dig into what happened to that.
>
> Leaving a phandle from an overlay in the phandle cache after the
> overlay is removed would clearly be a bug.

Yes that's totally it, when I get a match of a phandle and the np
address is stale, then comes the kernel warning.

I added more debug code to print out the results of looking up dynamic
nodes only (nodes > the highest node added when the cache was
originally built).  Also I added debug code to also force the lookup
code to go back and look up dynamic nodes the slow, uncached way so I
could see if it got the correct np address.  I'm unloading and
reloading an overlay, so sometimes the new np (after the overlay was
removed and reapplied) is at the same address, but sometimes it isn't
and then I get the kernel warning.  Just for entertainment value, here
is my prints of the np from the cache and the correct np that' at a
different address.  The '88' is the phandle, after that is the np
address, then the np=<full_name>.  Note also the corruption in the
cached node's full_name

OF: of_find_node_by_phandle line 1108 found dynamic node in cache 88
ee518680 np=/soc/base_fpga_region/\300\252~\356ze_controller@0x100000450

OF: of_find_node_by_phandle line 1119 found the slow way: 88 eeaf2d00
np=/soc/base_fpga_region/freeze_controller@0x100000450

I'm just scratching my head and wondering: should we be zeroing out
some things in memory after an overlay is removed?  Then this bug
would led to a pointer that would have thrown an oops.

If it's useful for you, next week I can send a cleaned up version of
my patch that rebuilds the cache after overlays are applied or
removed.

Alan

>
> -Frank

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()

[Finn Thain]

[-- Attachment #1: Type: text/plain, Size: 8766 bytes --]

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:

> From: Frank Rowand <frank.rowand@sony.com>
> 
> 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 <cpandya@codeaurora.org>
> 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 <cpandya@codeaurora.org>
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
> 
> 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;
> +
> +/*
> + * Assumptions behind phandle_cache implementation:
> + *   - phandle property values are in a contiguous range of 1..n
> + *
> + * If the assumptions do not hold, then
> + *   - the phandle lookup overhead reduction provided by the cache
> + *     will likely be less
> + */
> +static void of_populate_phandle_cache(void)
> +{
> +	unsigned long flags;
> +	u32 cache_entries;
> +	struct device_node *np;
> +	u32 phandles = 0;
> +
> +	raw_spin_lock_irqsave(&devtree_lock, flags);
> +
> +	kfree(phandle_cache);
> +	phandle_cache = NULL;
> +
> +	for_each_of_allnodes(np)
> +		if (np->phandle && np->phandle != OF_PHANDLE_ILLEGAL)
> +			phandles++;
> +
> +	cache_entries = roundup_pow_of_two(phandles);
> +	phandle_cache_mask = cache_entries - 1;
> +
> +	phandle_cache = kcalloc(cache_entries, sizeof(*phandle_cache),
> +				GFP_ATOMIC);
> +	if (!phandle_cache)
> +		goto out;
> +
> +	for_each_of_allnodes(np)
> +		if (np->phandle && np->phandle != OF_PHANDLE_ILLEGAL)
> +			phandle_cache[np->phandle & phandle_cache_mask] = np;
> +
> +out:
> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +}
> +
> +#ifndef CONFIG_MODULES
> +static int __init of_free_phandle_cache(void)
> +{
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&devtree_lock, flags);
> +
> +	kfree(phandle_cache);
> +	phandle_cache = NULL;
> +
> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
> +
> +	return 0;
> +}
> +late_initcall_sync(of_free_phandle_cache);
> +#endif
> +
>  void __init of_core_init(void)
>  {
>  	struct device_node *np;
>  
> +	of_populate_phandle_cache();
> +
>  	/* Create the kset, and register existing nodes */
>  	mutex_lock(&of_mutex);
>  	of_kset = kset_create_and_add("devicetree", NULL, firmware_kobj);
> @@ -1021,16 +1083,32 @@ int of_modalias_node(struct device_node *node, char *modalias, int len)
>   */
>  struct device_node *of_find_node_by_phandle(phandle handle)
>  {
> -	struct device_node *np;
> +	struct device_node *np = NULL;
>  	unsigned long flags;
> +	phandle masked_handle;
>  
>  	if (!handle)
>  		return NULL;
>  
>  	raw_spin_lock_irqsave(&devtree_lock, flags);
> -	for_each_of_allnodes(np)
> -		if (np->phandle == handle)
> -			break;
> +
> +	masked_handle = handle & phandle_cache_mask;
> +
> +	if (phandle_cache) {
> +		if (phandle_cache[masked_handle] &&
> +		    handle == phandle_cache[masked_handle]->phandle)
> +			np = phandle_cache[masked_handle];
> +	}
> +
> +	if (!np) {
> +		for_each_of_allnodes(np)
> +			if (np->phandle == handle) {
> +				if (phandle_cache)
> +					phandle_cache[masked_handle] = np;
> +				break;
> +			}
> +	}
> +
>  	of_node_get(np);
>  	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>  	return np;
> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> index 0c609e7d0334..fa70650136b4 100644
> --- a/drivers/of/of_private.h
> +++ b/drivers/of/of_private.h
> @@ -131,6 +131,9 @@ extern void __of_update_property_sysfs(struct device_node *np,
>  extern void __of_sysfs_remove_bin_file(struct device_node *np,
>  				       struct property *prop);
>  
> +/* illegal phandle value (set when unresolved) */
> +#define OF_PHANDLE_ILLEGAL	0xdeadbeef
> +
>  /* iterators for transactions, used for overlays */
>  /* forward iterator */
>  #define for_each_transaction_entry(_oft, _te) \
> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
> index 740d19bde601..b2ca8185c8c6 100644
> --- a/drivers/of/resolver.c
> +++ b/drivers/of/resolver.c
> @@ -19,9 +19,6 @@
>  
>  #include "of_private.h"
>  
> -/* illegal phandle value (set when unresolved) */
> -#define OF_PHANDLE_ILLEGAL	0xdeadbeef
> -
>  static phandle live_tree_max_phandle(void)
>  {
>  	struct device_node *node;
> 

[-- Attachment #2: Type: text/plain, Size: 14087 bytes --]

Total memory = 768MB; using 2048kB for hash table (at (ptrval))
Linux version 4.18.0-g5d852e5b3424 (root@nippy) (gcc version 8.1.0 (Debian 8.1.0-12)) #4 SMP Wed Aug 29 14:54:01 AEST 2018
Found a Heathrow mac-io controller, rev: 1, mapped at 0x(ptrval)
PowerMac motherboard: PowerMac G3 (Silk)
Using PowerMac machine description
bootconsole [udbg0] enabled
CPU maps initialized for 1 thread per core
 (thread shift is 0)
-----------------------------------------------------
Hash_size         = 0x200000
phys_mem_size     = 0x30000000
dcache_bsize      = 0x20
icache_bsize      = 0x20
cpu_features      = 0x000000000501a008
  possible        = 0x000000002f7ff04b
  always          = 0x0000000001000000
cpu_user_features = 0x8c000001 0x00000000
mmu_features      = 0x00000001
Hash              = 0x(ptrval)
Hash_mask         = 0x7fff
-----------------------------------------------------
Found Grackle (MPC106) PCI host bridge at 0x0000000080000000. Firmware bus number: 0->0
PCI host bridge /pci (primary) ranges:
  IO 0x00000000fe000000..0x00000000fe7fffff -> 0x0000000000000000
 MEM 0x00000000fd000000..0x00000000fdffffff -> 0x0000000000000000 
 MEM 0x0000000080000000..0x00000000fcffffff -> 0x0000000080000000 
nvram: OF partition at 0x1800
nvram: XP partition at 0x1300
nvram: NR partition at 0x1400
Top of RAM: 0x30000000, Total RAM: 0x30000000
Memory hole size: 0MB
Zone ranges:
  DMA      [mem 0x0000000000000000-0x000000002fffffff]
  Normal   empty
  HighMem  empty
Movable zone start for each node
Early memory node ranges
  node   0: [mem 0x0000000000000000-0x000000002fffffff]
Initmem setup node 0 [mem 0x0000000000000000-0x000000002fffffff]
On node 0 totalpages: 196608
  DMA zone: 1536 pages used for memmap
  DMA zone: 0 pages reserved
  DMA zone: 196608 pages, LIFO batch:31
percpu: Embedded 14 pages/cpu @(ptrval) s24684 r8192 d24468 u57344
pcpu-alloc: s24684 r8192 d24468 u57344 alloc=14*4096
pcpu-alloc: [0] 0 [0] 1 
Built 1 zonelists, mobility grouping on.  Total pages: 195072
Kernel command line: root=/dev/sda12 video=atyfb:vmode:14 log_buf_len=64k earlyprintk console=tty0
log_buf_len: 65536 bytes
early log buf free: 29884(91%)
Dentry cache hash table entries: 131072 (order: 7, 524288 bytes)
Inode-cache hash table entries: 65536 (order: 6, 262144 bytes)
Memory: 760236K/786432K available (5756K kernel code, 244K rwdata, 1332K rodata, 268K init, 175K bss, 26196K reserved, 0K cma-reserved, 0K highmem)
Kernel virtual memory layout:
  * 0xfffbf000..0xfffff000  : fixmap
  * 0xff800000..0xffc00000  : highmem PTEs
  * 0xfef5b000..0xff800000  : early ioremap
  * 0xf1000000..0xfef5b000  : vmalloc & ioremap
Hierarchical RCU implementation.
NR_IRQS: 512, nr_irqs: 512, preallocated irqs: 16
irq: Found primary Apple PIC /pci/mac-io for 64 irqs
irq: System has 64 possible interrupts
GMT Delta read from XPRAM: -360 minutes, DST: on
time_init: decrementer frequency = 16.708416 MHz
time_init: processor frequency   = 501.150000 MHz
clocksource: timebase: mask: 0xffffffffffffffff max_cycles: 0x3da7d2cd7, max_idle_ns: 440795202411 ns
clocksource: timebase mult[3bd99eb5] shift[24] registered
clockevent: decrementer mult[44700b4] shift[32] cpu[0]
Console: colour dummy device 80x25
console [tty0] enabled
bootconsole [udbg0] disabled
pid_max: default: 32768 minimum: 301
Mount-cache hash table entries: 2048 (order: 1, 8192 bytes)
Mountpoint-cache hash table entries: 2048 (order: 1, 8192 bytes)
Hierarchical SRCU implementation.
smp: Bringing up secondary CPUs ...
smp: Brought up 1 node, 1 CPU
Using standard scheduler topology
devtmpfs: initialized
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"
random: get_random_u32 called from bucket_table_alloc+0x90/0x214 with crng_init=0
clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns
futex hash table entries: 512 (order: 2, 16384 bytes)
NET: Registered protocol family 16
PCI: Probing PCI hardware
PCI host bridge to bus 0000:00
pci_bus 0000:00: root bus resource [io  0x0000-0x7fffff]
pci_bus 0000:00: root bus resource [mem 0xfd000000-0xfdffffff] (bus address [0x00000000-0x00ffffff])
pci_bus 0000:00: root bus resource [mem 0x80000000-0xfcffffff]
pci_bus 0000:00: root bus resource [bus 00-ff]
pci_bus 0000:00: busn_res: [bus 00-ff] end is updated to ff
pci 0000:00:00.0: [1057:0002] type 00 class 0x060000
pci 0000:00:0e.0: [1033:00cd] type 00 class 0x0c0010
pci 0000:00:0e.0: reg 0x10: [mem 0x80803000-0x80803fff]
pci 0000:00:0e.0: supports D2
pci 0000:00:0e.0: PME# supported from D2 D3hot
pci 0000:00:0f.0: [1033:0035] type 00 class 0x0c0310
pci 0000:00:0f.0: reg 0x10: [mem 0x80802000-0x80802fff]
pci 0000:00:0f.0: supports D1 D2
pci 0000:00:0f.0: PME# supported from D0 D1 D2 D3hot
pci 0000:00:0f.1: [1033:0035] type 00 class 0x0c0310
pci 0000:00:0f.1: reg 0x10: [mem 0x80801000-0x80801fff]
pci 0000:00:0f.1: supports D1 D2
pci 0000:00:0f.1: PME# supported from D0 D1 D2 D3hot
pci 0000:00:0f.2: [1033:00e0] type 00 class 0x0c0320
pci 0000:00:0f.2: reg 0x10: [mem 0x80800000-0x808000ff]
pci 0000:00:0f.2: supports D1 D2
pci 0000:00:0f.2: PME# supported from D0 D1 D2 D3hot
pci 0000:00:10.0: [106b:0010] type 00 class 0xff0000
pci 0000:00:10.0: reg 0x10: [mem 0xf3000000-0xf307ffff]
pci 0000:00:12.0: [1002:4750] type 00 class 0x030000
pci 0000:00:12.0: reg 0x10: [mem 0x81000000-0x81ffffff]
pci 0000:00:12.0: reg 0x14: [io  0x0c00-0x0cff]
pci 0000:00:12.0: reg 0x18: [mem 0x80804000-0x80804fff]
pci 0000:00:12.0: reg 0x30: [mem 0xfd000000-0xfd01ffff pref]
pci_bus 0000:00: busn_res: [bus 00-ff] end is updated to 00
PCI 0000:00 Cannot reserve Legacy IO [io  0x0000-0x0fff]
pci 0000:00:12.0: BAR 6: assigned [mem 0xfd000000-0xfd01ffff pref]
pci_bus 0000:00: resource 4 [io  0x0000-0x7fffff]
pci_bus 0000:00: resource 5 [mem 0xfd000000-0xfdffffff]
pci_bus 0000:00: resource 6 [mem 0x80000000-0xfcffffff]
pci 0000:00:12.0: vgaarb: VGA device added: decodes=io+mem,owns=mem,locks=none
pci 0000:00:12.0: vgaarb: bridge control possible
pci 0000:00:12.0: vgaarb: setting as boot device (VGA legacy resources not available)
vgaarb: loaded
SCSI subsystem initialized
libata version 3.00 loaded.
usbcore: registered new interface driver usbfs
usbcore: registered new interface driver hub
usbcore: registered new device driver usb
clocksource: Switched to clocksource timebase
NET: Registered protocol family 2
tcp_listen_portaddr_hash hash table entries: 512 (order: 0, 6144 bytes)
TCP established hash table entries: 8192 (order: 3, 32768 bytes)
TCP bind hash table entries: 8192 (order: 4, 65536 bytes)
TCP: Hash tables configured (established 8192 bind 8192)
UDP hash table entries: 512 (order: 2, 16384 bytes)
UDP-Lite hash table entries: 512 (order: 2, 16384 bytes)
NET: Registered protocol family 1
RPC: Registered named UNIX socket transport module.
RPC: Registered udp transport module.
RPC: Registered tcp transport module.
RPC: Registered tcp NFSv4.1 backchannel transport module.
pci 0000:00:0f.0: enabling device (0014 -> 0016)
pci 0000:00:0f.1: enabling device (0014 -> 0016)
pci 0000:00:0f.2: enabling device (0014 -> 0016)
PCI: CLS 32 bytes, default 32
Thermal assist unit 
using timers, 
shrink_timer: 200 jiffies
Initialise system trusted keyrings
workingset: timestamp_bits=30 max_order=18 bucket_order=0
squashfs: version 4.0 (2009/01/31) Phillip Lougher
Key type asymmetric registered
Asymmetric key parser 'x509' registered
Block layer SCSI generic (bsg) driver version 0.4 loaded (major 253)
io scheduler noop registered
io scheduler deadline registered (default)
io scheduler mq-deadline registered (default)
io scheduler kyber registered
atyfb 0000:00:12.0: enabling device (0086 -> 0087)
atyfb: using auxiliary register aperture
atyfb: 3D RAGE PRO (Mach64 GP, PQFP, PCI) [0x4750 rev 0x7c]
atyfb: 6M SGRAM (1:1), 14.31818 MHz XTAL, 230 MHz PLL, 100 Mhz MCLK, 100 MHz XCLK
Console: switching to colour frame buffer device 128x48
atyfb: fb0: ATY Mach64 frame buffer device on PCI
pmac_zilog: 0.6 (Benjamin Herrenschmidt <benh@kernel.crashing.org>)
Generic non-volatile memory driver v1.1
brd: module loaded
loop: module loaded
MacIO PCI driver attached to Heathrow chipset
swim3 0.00015000:swim3: [fd0] SWIM3 floppy controller 
0.00013020:ch-a: ttyS0 at MMIO 0xf3013020 (irq = 16, base_baud = 230400) is a Z85c30 ESCC - Serial port
0.00013000:ch-b: ttyS1 at MMIO 0xf3013000 (irq = 17, base_baud = 230400) is a Z85c30 ESCC - Serial port
Macintosh Cuda and Egret driver.
mesh: configured for synchronous 5 MB/s
random: fast init done
ADB keyboard at 2, handler 1
Detected ADB keyboard, type ANSI.
input: ADB keyboard as /devices/virtual/input/input0
mesh: performing initial bus reset...
ADB mouse at 3, handler set to 2
input: ADB mouse as /devices/virtual/input/input1
scsi host0: MESH
pata-macio 0.00020000:ide: Activating pata-macio chipset Heathrow ATA, Apple bus ID 0
scsi host1: pata_macio
ata1: PATA max MWDMA2 irq 28
ata1.00: ATA-7: Maxtor 6Y120P0, YAR41BW0, max UDMA/133
ata1.00: 240121728 sectors, multi 0: LBA 
scsi 1:0:0:0: Direct-Access     ATA      Maxtor 6Y120P0   1BW0 PQ: 0 ANSI: 5
sd 1:0:0:0: [sda] 240121728 512-byte logical blocks: (123 GB/114 GiB)
sd 1:0:0:0: Attached scsi generic sg0 type 0
sd 1:0:0:0: [sda] Write Protect is off
sd 1:0:0:0: [sda] Mode Sense: 00 3a 00 00
sd 1:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
pata-macio 0.00021000:ide: Activating pata-macio chipset Heathrow ATA, Apple bus ID 1
scsi host2: pata_macio
ata2: PATA max MWDMA2 irq 30
eth0: BMAC at 00:05:02:fd:98:9c

firewire_ohci 0000:00:0e.0: enabling device (0014 -> 0016)
ata2.00: ATAPI: IOMEGA  ZIP 100       ATAPI, 25.D, max PIO2, CDB intr
ata2.01: ATAPI: MATSHITADVD-ROM SR-8582, 0B8b, max MWDMA2
scsi 2:0:0:0: Direct-Access     IOMEGA   ZIP 100          25.D PQ: 0 ANSI: 5
firewire_ohci 0000:00:0e.0: added OHCI v1.0 device as card 0, 4 IR + 4 IT contexts, quirks 0x1
aoe: cannot create debugfs directory
aoe: AoE v85 initialised.
ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver
ehci-pci: EHCI PCI platform driver
sd 2:0:0:0: Attached scsi generic sg1 type 0
ehci-pci 0000:00:0f.2: EHCI Host Controller
ehci-pci 0000:00:0f.2: new USB bus registered, assigned bus number 1
ehci-pci 0000:00:0f.2: irq 25, io mem 0x80800000
ehci-pci 0000:00:0f.2: USB 2.0 started, EHCI 1.00
usb usb1: New USB device found, idVendor=1d6b, idProduct=0002, bcdDevice= 4.18
usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1
usb usb1: Product: EHCI Host Controller
usb usb1: Manufacturer: Linux 4.18.0-g5d852e5b3424 ehci_hcd
usb usb1: SerialNumber: 0000:00:0f.2
hub 1-0:1.0: USB hub found
hub 1-0:1.0: 5 ports detected
ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver
ohci-pci: OHCI PCI platform driver
ohci-pci 0000:00:0f.0: OHCI PCI host controller
ohci-pci 0000:00:0f.0: new USB bus registered, assigned bus number 2
sd 2:0:0:0: [sdb] Attached SCSI removable disk
ohci-pci 0000:00:0f.0: irq 25, io mem 0x80802000
scsi 2:0:1:0: CD-ROM            MATSHITA DVD-ROM SR-8582  0B8b PQ: 0 ANSI: 5
sr 2:0:1:0: [sr0] scsi3-mmc drive: 24x/24x cd/rw xa/form2 cdda tray
cdrom: Uniform CD-ROM driver Revision: 3.20
sr 2:0:1:0: Attached scsi CD-ROM sr0
sr 2:0:1:0: Attached scsi generic sg2 type 5
usb usb2: New USB device found, idVendor=1d6b, idProduct=0001, bcdDevice= 4.18
usb usb2: New USB device strings: Mfr=3, Product=2, SerialNumber=1
usb usb2: Product: OHCI PCI host controller
usb usb2: Manufacturer: Linux 4.18.0-g5d852e5b3424 ohci_hcd
usb usb2: SerialNumber: 0000:00:0f.0
hub 2-0:1.0: USB hub found
hub 2-0:1.0: 3 ports detected
ohci-pci 0000:00:0f.1: OHCI PCI host controller
ohci-pci 0000:00:0f.1: new USB bus registered, assigned bus number 3
ohci-pci 0000:00:0f.1: irq 25, io mem 0x80801000
usb usb3: New USB device found, idVendor=1d6b, idProduct=0001, bcdDevice= 4.18
usb usb3: New USB device strings: Mfr=3, Product=2, SerialNumber=1
usb usb3: Product: OHCI PCI host controller
usb usb3: Manufacturer: Linux 4.18.0-g5d852e5b3424 ohci_hcd
usb usb3: SerialNumber: 0000:00:0f.1
firewire_core 0000:00:0e.0: created device fw0: GUID 00d0f52008006aff, S400
hub 3-0:1.0: USB hub found
hub 3-0:1.0: 2 ports detected
usbcore: registered new interface driver uas
usbcore: registered new interface driver usb-storage
mousedev: PS/2 mouse device common for all mice
rtc-generic rtc-generic: rtc core: registered rtc-generic as rtc0
i2c /dev entries driver
usbcore: registered new interface driver usbhid
usbhid: USB HID core driver
NET: Registered protocol family 17
drmem: No dynamic reconfiguration memory found
Loading compiled-in X.509 certificates
net firewire0: IP over IEEE 1394 on card 0000:00:0e.0
firewire_core 0000:00:0e.0: refreshed device fw0
 sda: [mac] sda1 sda2 sda3 sda4 sda5 sda6 sda7 sda8 sda9 sda10 sda11 sda12 sda13 sda14 sda15
sd 1:0:0:0: [sda] Attached SCSI disk
EXT4-fs (sda12): mounting ext3 file system using the ext4 subsystem
EXT4-fs (sda12): mounted filesystem with ordered data mode. Opts: (null)
VFS: Mounted root (ext3 filesystem) readonly on device 8:12.
Freeing unused kernel memory: 268K
This architecture does not have kernel memory protection.
Adding 3145724k swap on /dev/sda13.  Priority:-2 extents:1 across:3145724k 
EXT4-fs (sda12): re-mounted. Opts: (null)
random: crng init done
EXT4-fs (sda12): re-mounted. Opts: errors=remount-ro
EXT4-fs (sda14): mounting ext3 file system using the ext4 subsystem
EXT4-fs (sda14): mounted filesystem with ordered data mode. Opts: (null)
phy registers:
 0000 0000 0000 0000 0000 0000 0000 0000
 0000 0000 0000 0000 0000 0000 0000 0000
 0000 0000 0000 0000 0000 0000 0000 0000
 0000 0000 0000 0000 0000 0000 0000 0000
Installing knfsd (copyright (C) 1996 okir@monad.swb.de).

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()

[Rob Herring]

On Wed, Aug 29, 2018 at 7:44 PM Finn Thain <fthain@telegraphics.com.au> 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

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()

[Benjamin Herrenschmidt]

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 <fthain@telegraphics.com.au> 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()

[Frank Rowand]

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()

[Benjamin Herrenschmidt]

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 = &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;

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()

[Benjamin Herrenschmidt]

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 = &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;

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()

[Benjamin Herrenschmidt]

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()

[Benjamin Herrenschmidt]

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()

[Frank Rowand]

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()

[Rob Herring]

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()

[Frank Rowand]

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 <frank.rowand@sony.com>

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()

[Finn Thain]

[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
> 
>