The phandle cache was added to speed up of_find_node_by_phandle() by
avoiding walking the whole DT to find a matching phandle. The
implementation has several shortcomings:
- The cache is designed to work on a linear set of phandle values.
This is true for dtc generated DTs, but not for other cases such as
Power.
- The cache isn't enabled until of_core_init() and a typical system
may see hundreds of calls to of_find_node_by_phandle() before that
point.
- The cache is freed and re-allocated when the number of phandles
changes.
- It takes a raw spinlock around a memory allocation which breaks on
RT.
Change the implementation to a fixed size and use hash_32() as the
cache index. This greatly simplifies the implementation. It avoids
the need for any re-alloc of the cache and taking a reference on nodes
in the cache. We only have a single source of removing cache entries
which is of_detach_node().
Using hash_32() removes any assumption on phandle values improving
the hit rate for non-linear phandle values. The effect on linear values
using hash_32() is about a 10% collision. The chances of thrashing on
colliding values seems to be low.
To compare performance, I used a RK3399 board which is a pretty typical
system. I found that just measuring boot time as done previously is
noisy and may be impacted by other things. Also bringing up secondary
cores causes some issues with measuring, so I booted with 'nr_cpus=1'.
With no caching, calls to of_find_node_by_phandle() take about 20124 us
for 1248 calls. There's an additional 288 calls before time keeping is
up. Using the average time per hit/miss with the cache, we can calculate
these calls to take 690 us (277 hit / 11 miss) with a 128 entry cache
and 13319 us with no cache or an uninitialized cache.
Comparing the 3 implementations the time spent in
of_find_node_by_phandle() is:
no cache: 20124 us (+ 13319 us)
128 entry cache: 5134 us (+ 690 us)
current cache: 819 us (+ 13319 us)
We could move the allocation of the cache earlier to improve the
current cache, but that just further complicates the situation as it
needs to be after slab is up, so we can't do it when unflattening (which
uses memblock).
Reported-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Segher Boessenkool <segher@kernel.crashing.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Reviewed-by: Frank Rowand <frowand.list@gmail.com>
Tested-by: Frank Rowand <frowand.list@gmail.com>
Signed-off-by: Rob Herring <robh@kernel.org>
Non-overlay dynamic devicetree node removal may leave the node in
the phandle cache. Subsequent calls to of_find_node_by_phandle()
will incorrectly find the stale entry. Remove the node from the
cache.
Add paranoia checks in of_find_node_by_phandle() as a second level
of defense (do not return cached node if detached, do not add node
to cache if detached).
Fixes: 0b3ce78e90 ("of: cache phandle nodes to reduce cost of of_find_node_by_phandle()")
Reported-by: Michael Bringmann <mwb@linux.vnet.ibm.com>
Cc: stable@vger.kernel.org # v4.17+
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
Signed-off-by: Rob Herring <robh@kernel.org>
Overlay nodes added by add_changeset_node() do not have the node
fields name, phandle, and type set.
The node passed to __of_attach_node() when the add node changeset
entry is processed does not contain any properties. The node's
properties are located in add property changeset entries that will
be processed after the add node changeset is applied.
Set the node's fields in the node contained in the add node
changeset entry and do not set them to incorrect values in
add_changeset_node().
A visible symptom that is fixed by this patch is the names of nodes
added by overlays that have an entry in /sys/bus/platform/drivers/*/
will contain the unit-address but the node-name will be <NULL>, for
example, "fc4ab000.<NULL>". After applying the patch the name, in
this example, for node restart@fc4ab000 is "fc4ab000.restart".
Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
There is a matching of_node_put() in __of_detach_node_sysfs()
Remove misleading comment from function header comment for
of_detach_node().
This patch may result in memory leaks from code that directly calls
the dynamic node add and delete functions directly instead of
using changesets.
This commit should result in powerpc systems that dynamically
allocate a node, then later deallocate the node to have a
memory leak when the node is deallocated.
The next commit will fix the leak.
Tested-by: Alan Tull <atull@kernel.org>
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
Add checks:
- attempted kfree due to refcount reaching zero before overlay
is removed
- properties linked to an overlay node when the node is removed
- node refcount > one during node removal in a changeset destroy,
if the node was created by the changeset
After applying this patch, several validation warnings will be
reported from the devicetree unittest during boot due to
pre-existing devicetree bugs. The warnings will be similar to:
OF: ERROR: of_node_release(), unexpected properties in /testcase-data/overlay-node/test-bus/test-unittest11
OF: ERROR: memory leak, expected refcount 1 instead of 2, of_node_get()/of_node_put() unbalanced - destroy cset entry: attach overlay node /testcase-data-2/substation@100/
hvac-medium-2
Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
Struct device_node full_name no longer includes the full path name
when the devicetree is created from a flattened device tree (FDT).
The overlay node creation code was not modified to reflect this
change. Fix the node full_name generated by overlay code to contain
only the basename.
Unittests call an overlay internal function to create new nodes.
Fix up these calls to provide basename only instead of the full
path.
Fixes: a7e4cfb0a7 ("of/fdt: only store the device node basename
in full_name")
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
Signed-off-by: Rob Herring <robh@kernel.org>
If a node with no properties is dynamically added, then a property is
dynamically added to the node, then the property is dynamically removed,
the result will be node->properties == NULL and node->deadprops != NULL.
Add a separate function to release the properties in both lists.
Signed-off-by: Lixin Wang <alan.1.wang@nokia-sbell.com>
Reviewed-by: Frank Rowand <frank.rowand@sony.com>
Signed-off-by: Rob Herring <robh@kernel.org>
When an attempt to apply an overlay changeset fails, an effort
is made to revert any partial application of the changeset.
When an attempt to remove an overlay changeset fails, an effort
is made to re-apply any partial reversion of the changeset.
The existing code does not check for failure to recover a failed
overlay changeset application or overlay changeset revert.
Add the missing checks and flag the devicetree as corrupt if the
state of the devicetree can not be determined.
Improve and expand the returned errors to more fully reflect the
result of the effort to undo the partial effects of a failed attempt
to apply or remove an overlay changeset.
If the device tree might be corrupt, do not allow further attempts
to apply or remove an overlay changeset.
When creating an overlay changeset from an overlay device tree,
add some additional warnings if the state of the overlay device
tree is not as expected.
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
Signed-off-by: Rob Herring <robh@kernel.org>
This patch is aimed primarily at drivers/of/overlay.c, but those
changes also have a small impact in a few other files.
overlay.c is difficult to read and maintain. Improve readability:
- Rename functions, types and variables to better reflect what
they do and to be consistent with names in other places,
such as the device tree overlay FDT (flattened device tree),
and make the algorithms more clear
- Use the same names consistently throughout the file
- Update comments for name changes
- Fix incorrect comments
This patch is intended to not introduce any functional change.
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
Signed-off-by: Rob Herring <robh@kernel.org>
Having device_nodes be kobjects is only needed if sysfs or OF_DYNAMIC is
enabled. Otherwise, having a kobject in struct device_node is
unnecessary bloat in minimal kernel configurations.
Likewise, bin_attribute is only needed in struct property when sysfs is
enabled, so we can make it configurable too.
Tested-by: Nicolas Pitre <nico@linaro.org>
Reviewed-by: Frank Rowand <frowand.list@gmail.com>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Rob Herring <robh@kernel.org>
The only user of kobj_to_device_node() is in dynamic.c, so move it
there. This avoids having to make it conditional once kobject is
configurable.
Tested-by: Nicolas Pitre <nico@linaro.org>
Reviewed-by: Frank Rowand <frowand.list@gmail.com>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
Signed-off-by: Rob Herring <robh@kernel.org>
Now that we have a custom printf format specifier, convert users of
full_name to use %pOF instead. This is preparation to remove storing
of the full path string for each node.
Signed-off-by: Rob Herring <robh@kernel.org>
__of_attach_node() is not used outside of drivers/of/dynamic.c. Make
it static and remove it from drivers/of/of_private.h.
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
Signed-off-by: Rob Herring <robh@kernel.org>
Fix a memory leak resulting from memory allocation in safe_name().
This patch fixes all call sites of safe_name().
Mathieu Malaterre reported the memory leak on boot:
On my PowerMac device-tree would generate a duplicate name:
[ 0.023043] device-tree: Duplicate name in PowerPC,G4@0, renamed to "l2-cache#1"
in this case a newly allocated name is generated by `safe_name`. However
in this case it is never deallocated.
The bug was found using kmemleak reported as:
unreferenced object 0xdf532e60 (size 32):
comm "swapper", pid 1, jiffies 4294892300 (age 1993.532s)
hex dump (first 32 bytes):
6c 32 2d 63 61 63 68 65 23 31 00 dd e4 dd 1e c2 l2-cache#1......
ec d4 ba ce 04 ec cc de 8e 85 e9 ca c4 ec cc 9e ................
backtrace:
[<c02d3350>] kvasprintf+0x64/0xc8
[<c02d3400>] kasprintf+0x4c/0x5c
[<c0453814>] safe_name.isra.1+0x80/0xc4
[<c04545d8>] __of_attach_node_sysfs+0x6c/0x11c
[<c075f21c>] of_core_init+0x8c/0xf8
[<c0729594>] kernel_init_freeable+0xd4/0x208
[<c00047e8>] kernel_init+0x24/0x11c
[<c00158ec>] ret_from_kernel_thread+0x5c/0x64
Link: https://bugzilla.kernel.org/show_bug.cgi?id=120331
Signed-off-by: Frank Rowand <frank.rowand@am.sony.com>
Reported-by: mathieu.malaterre@gmail.com
Tested-by: Mathieu Malaterre <mathieu.malaterre@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Rob Herring <robh@kernel.org>
When reverting an update property changeset entry that created a
property the reverse operation is a remove property and not an update.
Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
Signed-off-by: Rob Herring <robh@kernel.org>
"IS_ENABLED(PPC_PSERIES)" always evaluates to false, as IS_ENABLED() is
supposed to be used with the full Kconfig symbol name, including the
"CONFIG_" prefix.
Add the missing "CONFIG_" prefix to fix this.
Fixes: a25095d451 ("of: Move dynamic node fixups out of powerpc and into common code")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: stable@vger.kernel.org #+3.17
Signed-off-by: Grant Likely <grant.likely@linaro.org>
Add some additional debug output to cover OF_RECONFIG notifier activity.
At the same time, refactor the changeset debug output to use the same
strings as the notifier debug output.
Signed-off-by: Grant Likely <grant.likely@linaro.org>
