* [PATCH v5 0/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle() @ 2018-03-05 0:14 frowand.list 2018-03-05 0:14 ` [PATCH v5 1/3] " frowand.list ` (2 more replies) 0 siblings, 3 replies; 25+ messages in thread From: frowand.list @ 2018-03-05 0:14 UTC (permalink / raw) To: Rob Herring, cpandya; +Cc: devicetree, linux-kernel 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v5 1/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle() 2018-03-05 0:14 [PATCH v5 0/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle() frowand.list @ 2018-03-05 0:14 ` frowand.list 2018-03-09 23:03 ` Rob Herring ` (2 more replies) 2018-03-05 0:14 ` [PATCH v5 2/3] memblock: add memblock_free() alloc when CONFIG_HAVE_MEMBLOCK is not set frowand.list 2018-03-05 0:14 ` [PATCH v5 3/3] of: add early boot allocation of of_find_node_by_phandle() cache frowand.list 2 siblings, 3 replies; 25+ messages in thread From: frowand.list @ 2018-03-05 0:14 UTC (permalink / raw) To: Rob Herring, cpandya; +Cc: devicetree, linux-kernel 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> ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle() 2018-03-05 0:14 ` [PATCH v5 1/3] " frowand.list @ 2018-03-09 23:03 ` Rob Herring 2018-03-10 1:20 ` Frank Rowand 2018-06-12 18:16 ` Alan Tull 2018-08-30 0:44 ` v4.17 regression: PowerMac G3 won't boot, was " Finn Thain 2 siblings, 1 reply; 25+ messages in thread From: Rob Herring @ 2018-03-09 23:03 UTC (permalink / raw) To: frowand.list; +Cc: cpandya, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle() 2018-03-09 23:03 ` Rob Herring @ 2018-03-10 1:20 ` Frank Rowand 0 siblings, 0 replies; 25+ messages in thread From: Frank Rowand @ 2018-03-10 1:20 UTC (permalink / raw) To: Rob Herring; +Cc: cpandya, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle() 2018-03-05 0:14 ` [PATCH v5 1/3] " frowand.list 2018-03-09 23:03 ` Rob Herring @ 2018-06-12 18:16 ` Alan Tull 2018-06-13 14:42 ` Alan Tull 2018-08-30 0:44 ` v4.17 regression: PowerMac G3 won't boot, was " Finn Thain 2 siblings, 1 reply; 25+ messages in thread From: Alan Tull @ 2018-06-12 18:16 UTC (permalink / raw) To: Frank Rowand Cc: Rob Herring, cpandya, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, linux-fpga, Moritz Fischer 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> > ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle() 2018-06-12 18:16 ` Alan Tull @ 2018-06-13 14:42 ` Alan Tull 2018-06-13 21:47 ` Frank Rowand 0 siblings, 1 reply; 25+ messages in thread From: Alan Tull @ 2018-06-13 14:42 UTC (permalink / raw) To: Frank Rowand Cc: Rob Herring, cpandya, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, linux-fpga, Moritz Fischer 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle() 2018-06-13 14:42 ` Alan Tull @ 2018-06-13 21:47 ` Frank Rowand 2018-06-14 20:59 ` Alan Tull 0 siblings, 1 reply; 25+ messages in thread From: Frank Rowand @ 2018-06-13 21:47 UTC (permalink / raw) To: Alan Tull Cc: Rob Herring, cpandya, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, linux-fpga, Moritz Fischer 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle() 2018-06-13 21:47 ` Frank Rowand @ 2018-06-14 20:59 ` Alan Tull 0 siblings, 0 replies; 25+ messages in thread From: Alan Tull @ 2018-06-14 20:59 UTC (permalink / raw) To: Frank Rowand Cc: Rob Herring, cpandya, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-kernel, linux-fpga, Moritz Fischer 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* 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() 2018-03-05 0:14 ` [PATCH v5 1/3] " frowand.list 2018-03-09 23:03 ` Rob Herring 2018-06-12 18:16 ` Alan Tull @ 2018-08-30 0:44 ` Finn Thain 2018-08-30 1:05 ` Rob Herring 2018-08-31 4:36 ` Frank Rowand 2 siblings, 2 replies; 25+ messages in thread From: Finn Thain @ 2018-08-30 0:44 UTC (permalink / raw) To: Frank Rowand Cc: Stan Johnson, Rob Herring, Benjamin Herrenschmidt, Chintan Pandya, devicetree, linux-kernel, linuxppc-dev [-- 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). ^ permalink raw reply [flat|nested] 25+ messages in thread
* 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() 2018-08-30 0:44 ` v4.17 regression: PowerMac G3 won't boot, was " Finn Thain @ 2018-08-30 1:05 ` Rob Herring [not found] ` <569e4bc3-2149-4b2d-562f-e400dd05a8a8@yahoo.com> 2018-08-31 4:36 ` Frank Rowand 1 sibling, 1 reply; 25+ messages in thread From: Rob Herring @ 2018-08-30 1:05 UTC (permalink / raw) To: fthain Cc: Frank Rowand, userm57, Benjamin Herrenschmidt, Chintan Pandya, devicetree, linux-kernel, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <569e4bc3-2149-4b2d-562f-e400dd05a8a8@yahoo.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() [not found] ` <569e4bc3-2149-4b2d-562f-e400dd05a8a8@yahoo.com> @ 2018-08-31 4:35 ` Benjamin Herrenschmidt 2018-08-31 4:49 ` Benjamin Herrenschmidt 2018-08-31 4:49 ` Benjamin Herrenschmidt 0 siblings, 2 replies; 25+ messages in thread From: Benjamin Herrenschmidt @ 2018-08-31 4:35 UTC (permalink / raw) To: Mac User, Rob Herring Cc: fthain, Frank Rowand, Chintan Pandya, devicetree, linux-kernel, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* 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() 2018-08-31 4:35 ` Benjamin Herrenschmidt @ 2018-08-31 4:49 ` Benjamin Herrenschmidt 2018-08-31 4:49 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 25+ messages in thread From: Benjamin Herrenschmidt @ 2018-08-31 4:49 UTC (permalink / raw) To: Mac User, Rob Herring Cc: fthain, Frank Rowand, Chintan Pandya, devicetree, linux-kernel, linuxppc-dev 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; ^ permalink raw reply [flat|nested] 25+ messages in thread
* 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() 2018-08-31 4:35 ` Benjamin Herrenschmidt 2018-08-31 4:49 ` Benjamin Herrenschmidt @ 2018-08-31 4:49 ` Benjamin Herrenschmidt 1 sibling, 0 replies; 25+ messages in thread From: Benjamin Herrenschmidt @ 2018-08-31 4:49 UTC (permalink / raw) To: Mac User, Rob Herring Cc: fthain, Frank Rowand, Chintan Pandya, devicetree, linux-kernel, linuxppc-dev 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; ^ permalink raw reply [flat|nested] 25+ messages in thread
* 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() 2018-08-30 0:44 ` v4.17 regression: PowerMac G3 won't boot, was " Finn Thain 2018-08-30 1:05 ` Rob Herring @ 2018-08-31 4:36 ` Frank Rowand 2018-08-31 4:58 ` Benjamin Herrenschmidt 1 sibling, 1 reply; 25+ messages in thread From: Frank Rowand @ 2018-08-31 4:36 UTC (permalink / raw) To: Finn Thain Cc: Stan Johnson, Rob Herring, Benjamin Herrenschmidt, Chintan Pandya, devicetree, linux-kernel, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* 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() 2018-08-31 4:36 ` Frank Rowand @ 2018-08-31 4:58 ` Benjamin Herrenschmidt 2018-09-09 17:04 ` Benjamin Herrenschmidt 0 siblings, 1 reply; 25+ messages in thread From: Benjamin Herrenschmidt @ 2018-08-31 4:58 UTC (permalink / raw) To: Frank Rowand, Finn Thain Cc: Stan Johnson, Rob Herring, Chintan Pandya, devicetree, linux-kernel, linuxppc-dev 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* 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() 2018-08-31 4:58 ` Benjamin Herrenschmidt @ 2018-09-09 17:04 ` Benjamin Herrenschmidt 2018-09-09 23:52 ` Frank Rowand 2018-09-10 12:53 ` Rob Herring 0 siblings, 2 replies; 25+ messages in thread From: Benjamin Herrenschmidt @ 2018-09-09 17:04 UTC (permalink / raw) To: Frank Rowand, Finn Thain Cc: Stan Johnson, Rob Herring, Chintan Pandya, devicetree, linux-kernel, linuxppc-dev 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. ^ permalink raw reply [flat|nested] 25+ messages in thread
* 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() 2018-09-09 17:04 ` Benjamin Herrenschmidt @ 2018-09-09 23:52 ` Frank Rowand 2018-09-10 12:53 ` Rob Herring 1 sibling, 0 replies; 25+ messages in thread From: Frank Rowand @ 2018-09-09 23:52 UTC (permalink / raw) To: Benjamin Herrenschmidt, Finn Thain Cc: Stan Johnson, Rob Herring, Chintan Pandya, devicetree, linux-kernel, linuxppc-dev, 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* 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() 2018-09-09 17:04 ` Benjamin Herrenschmidt 2018-09-09 23:52 ` Frank Rowand @ 2018-09-10 12:53 ` Rob Herring 2018-09-11 15:53 ` Frank Rowand [not found] ` <abb0dec2-da3a-2c04-0e9f-28851b22cf75@yahoo.com> 1 sibling, 2 replies; 25+ messages in thread From: Rob Herring @ 2018-09-10 12:53 UTC (permalink / raw) To: Benjamin Herrenschmidt, Finn Thain, Stan Johnson Cc: Frank Rowand, Chintan Pandya, devicetree, linux-kernel, linuxppc-dev 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; ^ permalink raw reply related [flat|nested] 25+ messages in thread
* 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() 2018-09-10 12:53 ` Rob Herring @ 2018-09-11 15:53 ` Frank Rowand [not found] ` <abb0dec2-da3a-2c04-0e9f-28851b22cf75@yahoo.com> 1 sibling, 0 replies; 25+ messages in thread From: Frank Rowand @ 2018-09-11 15:53 UTC (permalink / raw) To: Rob Herring, Benjamin Herrenschmidt, Finn Thain, Stan Johnson Cc: Chintan Pandya, devicetree, linux-kernel, linuxppc-dev 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
[parent not found: <abb0dec2-da3a-2c04-0e9f-28851b22cf75@yahoo.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() [not found] ` <abb0dec2-da3a-2c04-0e9f-28851b22cf75@yahoo.com> @ 2018-09-12 0:15 ` Finn Thain 0 siblings, 0 replies; 25+ messages in thread From: Finn Thain @ 2018-09-12 0:15 UTC (permalink / raw) Cc: Frank Rowand, Stan Johnson, Rob Herring, Benjamin Herrenschmidt, Chintan Pandya, devicetree, linux-kernel, linuxppc-dev [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 > > ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v5 2/3] memblock: add memblock_free() alloc when CONFIG_HAVE_MEMBLOCK is not set 2018-03-05 0:14 [PATCH v5 0/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle() frowand.list 2018-03-05 0:14 ` [PATCH v5 1/3] " frowand.list @ 2018-03-05 0:14 ` frowand.list 2018-03-06 0:00 ` Andrew Morton 2018-03-05 0:14 ` [PATCH v5 3/3] of: add early boot allocation of of_find_node_by_phandle() cache frowand.list 2 siblings, 1 reply; 25+ messages in thread From: frowand.list @ 2018-03-05 0:14 UTC (permalink / raw) To: Rob Herring, Andrew Morton, Michal Hocko, Catalin Marinas, Pavel Tatashin, Vlastimil Babka, cpandya Cc: devicetree, linux-kernel 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> ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/3] memblock: add memblock_free() alloc when CONFIG_HAVE_MEMBLOCK is not set 2018-03-05 0:14 ` [PATCH v5 2/3] memblock: add memblock_free() alloc when CONFIG_HAVE_MEMBLOCK is not set frowand.list @ 2018-03-06 0:00 ` Andrew Morton 0 siblings, 0 replies; 25+ messages in thread From: Andrew Morton @ 2018-03-06 0:00 UTC (permalink / raw) To: frowand.list Cc: Rob Herring, Michal Hocko, Catalin Marinas, Pavel Tatashin, Vlastimil Babka, cpandya, devicetree, linux-kernel 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> ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v5 3/3] of: add early boot allocation of of_find_node_by_phandle() cache 2018-03-05 0:14 [PATCH v5 0/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle() frowand.list 2018-03-05 0:14 ` [PATCH v5 1/3] " frowand.list 2018-03-05 0:14 ` [PATCH v5 2/3] memblock: add memblock_free() alloc when CONFIG_HAVE_MEMBLOCK is not set frowand.list @ 2018-03-05 0:14 ` frowand.list 2018-03-05 20:26 ` Rob Herring 2 siblings, 1 reply; 25+ messages in thread From: frowand.list @ 2018-03-05 0:14 UTC (permalink / raw) To: Rob Herring, cpandya; +Cc: devicetree, linux-kernel 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> ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v5 3/3] of: add early boot allocation of of_find_node_by_phandle() cache 2018-03-05 0:14 ` [PATCH v5 3/3] of: add early boot allocation of of_find_node_by_phandle() cache frowand.list @ 2018-03-05 20:26 ` Rob Herring 2018-03-06 3:12 ` Frank Rowand 0 siblings, 1 reply; 25+ messages in thread From: Rob Herring @ 2018-03-05 20:26 UTC (permalink / raw) To: Frank Rowand; +Cc: Chintan Pandya, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 3/3] of: add early boot allocation of of_find_node_by_phandle() cache 2018-03-05 20:26 ` Rob Herring @ 2018-03-06 3:12 ` Frank Rowand 0 siblings, 0 replies; 25+ messages in thread From: Frank Rowand @ 2018-03-06 3:12 UTC (permalink / raw) To: Rob Herring; +Cc: Chintan Pandya, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2018-09-12 0:15 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-05 0:14 [PATCH v5 0/3] of: cache phandle nodes to reduce cost of of_find_node_by_phandle() frowand.list 2018-03-05 0:14 ` [PATCH v5 1/3] " frowand.list 2018-03-09 23:03 ` Rob Herring 2018-03-10 1:20 ` Frank Rowand 2018-06-12 18:16 ` Alan Tull 2018-06-13 14:42 ` Alan Tull 2018-06-13 21:47 ` Frank Rowand 2018-06-14 20:59 ` Alan Tull 2018-08-30 0:44 ` v4.17 regression: PowerMac G3 won't boot, was " Finn Thain 2018-08-30 1:05 ` Rob Herring [not found] ` <569e4bc3-2149-4b2d-562f-e400dd05a8a8@yahoo.com> 2018-08-31 4:35 ` Benjamin Herrenschmidt 2018-08-31 4:49 ` Benjamin Herrenschmidt 2018-08-31 4:49 ` Benjamin Herrenschmidt 2018-08-31 4:36 ` Frank Rowand 2018-08-31 4:58 ` Benjamin Herrenschmidt 2018-09-09 17:04 ` Benjamin Herrenschmidt 2018-09-09 23:52 ` Frank Rowand 2018-09-10 12:53 ` Rob Herring 2018-09-11 15:53 ` Frank Rowand [not found] ` <abb0dec2-da3a-2c04-0e9f-28851b22cf75@yahoo.com> 2018-09-12 0:15 ` Finn Thain 2018-03-05 0:14 ` [PATCH v5 2/3] memblock: add memblock_free() alloc when CONFIG_HAVE_MEMBLOCK is not set frowand.list 2018-03-06 0:00 ` Andrew Morton 2018-03-05 0:14 ` [PATCH v5 3/3] of: add early boot allocation of of_find_node_by_phandle() cache frowand.list 2018-03-05 20:26 ` Rob Herring 2018-03-06 3:12 ` Frank Rowand
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).