27 Nov, 2019
1 commit
-
No changeset entries are created for #address-cells and #size-cells
properties, but the duplicated properties are never freed. This
results in a memory leak which is detected by kmemleak:unreferenced object 0x85887180 (size 64):
backtrace:
kmem_cache_alloc_trace+0x1fb/0x1fc
__of_prop_dup+0x25/0x7c
add_changeset_property+0x17f/0x370
build_changeset_next_level+0x29/0x20c
of_overlay_fdt_apply+0x32b/0x6b4
...Fixes: 6f75118800ac ("of: overlay: validate overlay properties #address-cells and #size-cells")
Reported-by: Vincent Whitchurch
Signed-off-by: Frank Rowand
Tested-by: Vincent Whitchurch
Signed-off-by: Rob Herring
11 Jan, 2019
1 commit
-
Now that all users of device_node.type pointer have been removed in
favor of accessor functions, we can remove it.Cc: Frank Rowand
Cc: devicetree@vger.kernel.org
Signed-off-by: Rob Herring
09 Nov, 2018
10 commits
-
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 , for
example, "fc4ab000.". After applying the patch the name, in
this example, for node restart@fc4ab000 is "fc4ab000.restart".Tested-by: Alan Tull
Signed-off-by: Frank Rowand -
Add test case of two fragments updating the same property. After
adding the test case, the system hangs at end of boot, after
after slub stack dumps from kfree() in crypto modprobe code.Multiple overlay fragments adding, modifying, or deleting the same
property is not supported. Add check to detect the attempt and fail
the overlay apply.Before this patch, the first fragment error would terminate
processing. Allow fragment checking to proceed and report all
of the fragment errors before terminating the overlay apply. This
is not a hot path, thus not a performance issue (the error is not
transient and requires fixing the overlay before attempting to
apply it again).After applying this patch, the devicetree unittest messages will
include:OF: overlay: ERROR: multiple fragments add, update, and/or delete property /testcase-data-2/substation@100/motor-1/rpm_avail
...
### dt-test ### end of unittest - 212 passed, 0 failed
The check to detect two fragments updating the same property is
folded into the patch that created the test case to maintain
bisectability.Tested-by: Alan Tull
Signed-off-by: Frank Rowand -
Multiple overlay fragments adding or deleting the same node is not
supported. Replace code comment of such, with check to detect the
attempt and fail the overlay apply.Devicetree unittest where multiple fragments added the same node was
added in the previous patch in the series. After applying this patch
the unittest messages will no longer include:Duplicate name in motor-1, renamed to "controller#1"
OF: overlay: of_overlay_apply() err=0
### dt-test ### of_overlay_fdt_apply() expected -22, ret=0, overlay_bad_add_dup_node
### dt-test ### FAIL of_unittest_overlay_high_level():2419 Adding overlay 'overlay_bad_add_dup_node' failed...
### dt-test ### end of unittest - 210 passed, 1 failed
but will instead include:
OF: overlay: ERROR: multiple overlay fragments add and/or delete node /testcase-data-2/substation@100/motor-1/controller
...
### dt-test ### end of unittest - 211 passed, 0 failed
Tested-by: Alan Tull
Signed-off-by: Frank Rowand -
Make overlay.c debug and error messages unique so that they can be
unambiguously found by grep.Tested-by: Alan Tull
Signed-off-by: Frank Rowand -
If overlay properties #address-cells or #size-cells are already in
the live devicetree for any given node, then the values in the
overlay must match the values in the live tree.If the properties are already in the live tree then there is no
need to create a changeset entry to add them since they must
have the same value. This reduces the memory used by the
changeset and eliminates a possible memory leak.Tested-by: Alan Tull
Signed-off-by: Frank Rowand -
Order the fields of struct fragment in the same order as
struct of_overlay_notify_data. The order in struct fragment is
not significant. If both structs are ordered the same then when
examining the data in a debugger or dump the human involved does
not have to remember which context they are examining.Tested-by: Alan Tull
Signed-off-by: Frank Rowand -
When allocating a new node, add_changeset_node() was duplicating the
properties from the respective node in the overlay instead of
allocating a node with no properties.When this patch is applied the errors reported by the devictree
unittest from patch "of: overlay: add tests to validate kfrees from
overlay removal" will no longer occur. These error messages are of
the form:"OF: ERROR: ..."
and the unittest results will change from:
### dt-test ### end of unittest - 203 passed, 7 failed
to
### dt-test ### end of unittest - 210 passed, 0 failed
Tested-by: Alan Tull
Signed-off-by: Frank Rowand -
The changeset entry 'update property' was used for new properties in
an overlay instead of 'add property'.The decision of whether to use 'update property' was based on whether
the property already exists in the subtree where the node is being
spliced into. At the top level of creating a changeset describing the
overlay, the target node is in the live devicetree, so checking whether
the property exists in the target node returns the correct result.
As soon as the changeset creation algorithm recurses into a new node,
the target is no longer in the live devicetree, but is instead in the
detached overlay tree, thus all properties are incorrectly found to
already exist in the target.This fix will expose another devicetree bug that will be fixed
in the following patch in the series.When this patch is applied the errors reported by the devictree
unittest will change, and the unittest results will change from:### dt-test ### end of unittest - 210 passed, 0 failed
to
### dt-test ### end of unittest - 203 passed, 7 failed
Tested-by: Alan Tull
Signed-off-by: Frank Rowand -
The refcount of a newly added overlay node decrements to one
(instead of zero) when the overlay changeset is destroyed. This
change will cause the final decrement be to zero.After applying this patch, new validation warnings will be
reported from the devicetree unittest during boot due to
a pre-existing devicetree bug. The warnings will be similar to:OF: ERROR: memory leak before free overlay changeset, /testcase-data/overlay-node/test-bus/test-unittest4
This pre-existing devicetree bug will also trigger a WARN_ONCE() from
refcount_sub_and_test_checked() when an overlay changeset is
destroyed without having first been applied. This scenario occurs
when an error in the overlay is detected during the overlay changeset
creation:WARNING: CPU: 0 PID: 1 at lib/refcount.c:187 refcount_sub_and_test_checked+0xa8/0xbc
refcount_t: underflow; use-after-free.(unwind_backtrace) from (show_stack+0x10/0x14)
(show_stack) from (dump_stack+0x6c/0x8c)
(dump_stack) from (__warn+0xdc/0x104)
(__warn) from (warn_slowpath_fmt+0x44/0x6c)
(warn_slowpath_fmt) from (refcount_sub_and_test_checked+0xa8/0xbc)
(refcount_sub_and_test_checked) from (kobject_put+0x24/0x208)
(kobject_put) from (of_changeset_destroy+0x2c/0xb4)
(of_changeset_destroy) from (free_overlay_changeset+0x1c/0x9c)
(free_overlay_changeset) from (of_overlay_remove+0x284/0x2cc)
(of_overlay_remove) from (of_unittest_apply_revert_overlay_check.constprop.4+0xf8/0x1e8)
(of_unittest_apply_revert_overlay_check.constprop.4) from (of_unittest_overlay+0x960/0xed8)
(of_unittest_overlay) from (of_unittest+0x1cc4/0x2138)
(of_unittest) from (do_one_initcall+0x4c/0x28c)
(do_one_initcall) from (kernel_init_freeable+0x29c/0x378)
(kernel_init_freeable) from (kernel_init+0x8/0x110)
(kernel_init) from (ret_from_fork+0x14/0x2c)Tested-by: Alan Tull
Signed-off-by: Frank Rowand -
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 changesetAfter 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-2Tested-by: Alan Tull
Signed-off-by: Frank Rowand
08 Sep, 2018
1 commit
-
In preparation to remove the node name pointer from struct device_node,
convert printf users to use the %pOFn format specifier.Reviewed-by: Frank Rowand
Cc: Andrew Lunn
Cc: Florian Fainelli
Cc: netdev@vger.kernel.org
Signed-off-by: Rob Herring
16 Jul, 2018
1 commit
-
A comment in the review of the patch adding the phandle cache said that
the cache would have to be updated when modules are applied and removed.
This patch implements the cache updates.Fixes: 0b3ce78e90fc ("of: cache phandle nodes to reduce cost of of_find_node_by_phandle()")
Reported-by: Alan Tull
Suggested-by: Alan Tull
Signed-off-by: Frank Rowand
Signed-off-by: Rob Herring
27 Apr, 2018
1 commit
-
Only the overlay notifier callbacks have a chance to potentially get
hold of references to those two resources, but they are not supposed to
store them beyond OF_OVERLAY_POST_REMOVE.Document the overlay notifier API, its constraint regarding pointer
lifetime, and then remove intentional leaks of ovcs->overlay_tree and
ovcs->fdt from free_overlay_changeset.See also https://lkml.org/lkml/2018/4/23/1063 and following.
Signed-off-by: Jan Kiszka
Reviewed-by: Frank Rowand
Signed-off-by: Rob Herring
18 Mar, 2018
1 commit
-
While technically the ovcs_id is still returned by of_overlay_apply(),
this is an internal function. All public callers of of_overlay_remove()
pass an ovcs_id returned by the public function of_overlay_fdt_apply().Fixes: 39a751a4cb7e4798 ("of: change overlay apply input data from unflattened to FDT")
Signed-off-by: Geert Uytterhoeven
Signed-off-by: Rob Herring
06 Mar, 2018
1 commit
-
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: a7e4cfb0a7ca ("of/fdt: only store the device node basename
in full_name")Signed-off-by: Frank Rowand
Signed-off-by: Rob Herring
04 Mar, 2018
2 commits
-
Errors while developing the patch to create of_overlay_fdt_apply()
exposed inadequate error messages to debug problems when overlay
devicetree fragment nodes contain an invalid target path. Improve
the messages in find_target_node() to remedy this.Signed-off-by: Frank Rowand
-
Move duplicating and unflattening of an overlay flattened devicetree
(FDT) into the overlay application code. To accomplish this,
of_overlay_apply() is replaced by of_overlay_fdt_apply().The copy of the FDT (aka "duplicate FDT") now belongs to devicetree
code, which is thus responsible for freeing the duplicate FDT. The
caller of of_overlay_fdt_apply() remains responsible for freeing the
original FDT.The unflattened devicetree now belongs to devicetree code, which is
thus responsible for freeing the unflattened devicetree.These ownership changes prevent early freeing of the duplicated FDT
or the unflattened devicetree, which could result in use after free
errors.of_overlay_fdt_apply() is a private function for the anticipated
overlay loader.Update unittest.c to use of_overlay_fdt_apply() instead of
of_overlay_apply().Move overlay fragments from artificial locations in
drivers/of/unittest-data/tests-overlay.dtsi into one devicetree
source file per overlay. This led to changes in
drivers/of/unitest-data/Makefile and drivers/of/unitest.c.- Add overlay directives to the overlay devicetree source files so
that dtc will compile them as true overlays into one FDT data
chunk per overlay.- Set CFLAGS for drivers/of/unittest-data/testcases.dts so that
symbols will be generated for overlay resolution of overlays
that are no longer artificially contained in testcases.dts- Unflatten and apply each unittest overlay FDT using
of_overlay_fdt_apply().- Enable the of_resolve_phandles() check for whether the unflattened
overlay is detached. This check was previously disabled because the
overlays from tests-overlay.dtsi were not unflattened into detached
trees.- Other changes to unittest.c infrastructure to manage multiple test
FDTs built into the kernel image (access by name instead of
arbitrary number).- of_unittest_overlay_high_level(): previously unused code to add
properties from the overlay_base devicetree to the live tree
was triggered by the restructuring of tests-overlay.dtsi and thus
testcases.dts. This exposed two bugs: (1) the need to dup a
property before adding it, and (2) property 'name' is
auto-generated in the unflatten code and thus will be a duplicate
in the __symbols__ node - do not treat this duplicate as an error.Signed-off-by: Frank Rowand
08 Jan, 2018
1 commit
-
Convert remaining DT files to use SPDX-License-Identifier tags.
Cc: Benjamin Herrenschmidt
Cc: Guennadi Liakhovetski
Cc: Paul Mackerras
Cc: Pantelis Antoniou
Reviewed-by: Frank Rowand
Reviewed-by: Philippe Ombredanne
Signed-off-by: Rob Herring
08 Dec, 2017
2 commits
-
Make it more clear that nodes without "__overlay__" subnodes are
skipped, by reverting the logic and using continue.
This also reduces indentation level.Signed-off-by: Geert Uytterhoeven
Signed-off-by: Rob Herring -
If an overlay has no "__symbols__" node, but it has nodes without
"__overlay__" subnodes at the end (e.g. a "__fixups__" node), after
filling in all fragments for nodes with "__overlay__" subnodes,
"fragment = &fragments[cnt]" will point beyond the end of the allocated
array.Hence writing to "fragment->overlay" will overwrite unallocated memory,
which may lead to a crash later.Fix this by deferring both the assignment to "fragment" and the
offending write afterwards until we know for sure the node has an
"__overlay__" subnode, and thus a valid entry in "fragments[]".Fixes: 61b4de4e0b384f4a ("of: overlay: minor restructuring")
Signed-off-by: Geert Uytterhoeven
Signed-off-by: Rob Herring
07 Dec, 2017
4 commits
-
The special overlay mutex is taken first, hence it should be released
last in the error path.of_resolve_phandles() must be called with of_mutex held. Without it, a
node and new phandle could be added via of_attach_node(), making the max
phandle wrong.free_overlay_changeset() must be called with of_mutex held, if any
non-trivial cleanup is to be done.Hence move "mutex_lock(&of_mutex)" up, as suggested by Frank, and merge
the two tail statements of the success and error paths, now they became
identical.Note that while the two mutexes are adjacent, we still need both:
__of_changeset_apply_notify(), which is called by __of_changeset_apply()
unlocks of_mutex, then does notifications then locks of_mutex. So the
mutex get released in the middle of of_overlay_apply()Fixes: f948d6d8b792bb90 ("of: overlay: avoid race condition between applying multiple overlays")
Signed-off-by: Geert Uytterhoeven
Reviewed-by: Frank Rowand
Signed-off-by: Rob Herring -
If of_resolve_phandles() fails, free_overlay_changeset() is called in
the error path. However, that function returns early if the list hasn't
been initialized yet, before freeing the object.Explicitly calling kfree() instead would solve that issue. However, that
complicates matter, by having to consider which of two different methods
to use to dispose of the same object.Hence make free_overlay_changeset() consider initialization state of the
different parts of the object, making it always safe to call (once!) to
dispose of a (partially) initialized overlay_changeset:
- Only destroy the changeset if the list was initialized,
- Make init_overlay_changeset() store the ID in ovcs->id on success,
to avoid calling idr_remove() with an error value or an already
released ID.Reported-by: Colin King
Fixes: f948d6d8b792bb90 ("of: overlay: avoid race condition between applying multiple overlays")
Signed-off-by: Geert Uytterhoeven
Reviewed-by: Frank Rowand
Signed-off-by: Rob Herring -
If an "if" branch is terminated by a "goto", there's no need to have an
"else" statement and an indented block of code.Remove the "else" statement to simplify the code flow for the casual
reviewer.Signed-off-by: Geert Uytterhoeven
Signed-off-by: Rob Herring -
Signed-off-by: Geert Uytterhoeven
Signed-off-by: Rob Herring
20 Oct, 2017
2 commits
-
The same error string occurs in drivers/of/resolver.c. Change
the error here to more precisely describe this case, and avoid
the possible confusion of looking in the wrong source location
to understand the cause of an error.Signed-off-by: Frank Rowand
Signed-off-by: Rob Herring -
A device tree overlay notifier can return NOTIFY_OK, NOTIFY_STOP,
or an embedded errno. overlay_notify() incorrectly reports an
error for NOTIFY_OK.Reported-by: atull@kernel.org
Signed-off-by: Frank Rowand
Signed-off-by: Rob Herring
18 Oct, 2017
12 commits
-
kbasename() will not return NULL if passed a valid string. If
the parameter passed to kbasename() in this case is already NULL
then the devicetree has been corrupted.Signed-off-by: Frank Rowand
Signed-off-by: Rob Herring -
The "%pOF" printf format was recently added to print the
full name of a device tree node, with the intent of changing
the node full_name field to contain only the node name instead
of the full path of the node.dup_and_fixup_symbol_prop() duplicates a property from the
"/__symbols__" node of an overlay device tree. The value
of each duplicated property must be fixed up to include
the full path of a node in the live device tree. The
current code uses the node's full_name for that purpose.
Update the code to use the "%pOF" printf format to
determine the node's full path.Signed-off-by: Frank Rowand
Signed-off-by: Rob Herring -
The code to apply symbols from an overlay to the live device tree
was implemented with the intent to be minimally intrusive on the
existing code. After recent restructuring of the overlay apply
code, it is easier to disintangle the code that applies the
symbols, and to make the overlay changeset creation code more
straight forward and understandable.Remove the extra complexity, and make the code more obvious.
Signed-off-by: Frank Rowand
Signed-off-by: Rob Herring -
The process of applying an overlay consists of:
- unflatten an overlay FDT (flattened device tree) into an
EDT (expanded device tree)
- fixup the phandle values in the overlay EDT to fit in a
range above the phandle values in the live device tree
- create the overlay changeset to reflect the contents of
the overlay EDT
- apply the overlay changeset, to modify the live device tree,
potentially changing the maximum phandle value in the live
device treeThere is currently no protection against two overlay applies
concurrently determining what range of phandle values are in use
in the live device tree, and subsequently changing that range.
Add a mutex to prevent multiple overlay applies from occurring
simultaneously.Move of_resolve_phandles() into of_overlay_apply() so that it does not
have to be duplicated by each caller of of_overlay_apply().The test in of_resolve_phandles() that the overlay tree is detached is
temporarily disabled so that old style overlay unittests do not fail.Signed-off-by: Frank Rowand
Signed-off-by: Rob Herring -
When an overlay contains a node that already exists in
the live device tree, the overlay node is not allowed
to change the phandle of the existing node.The existing check refused to allow an overlay node to
set the node phandle even when the existing node did
not have a phandle. Relax the check to allow an
overlay node to set the phandle value if the existing
node does not have a phandle.Signed-off-by: Frank Rowand
Signed-off-by: Rob Herring -
The test of whether it is safe to remove an overlay changeset
looked at whether any node in the overlay changeset was in a
subtree rooted at any more recently applied overlay changeset
node.The test failed to determine whether any node in the overlay
changeset was the root of a subtree that contained a more
recently applied overlay changeset node. Add this additional
check to the test.The test is still lacking any check for any phandle dependencies.
Signed-off-by: Frank Rowand
Signed-off-by: Rob Herring -
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
Signed-off-by: Rob Herring -
Continue improving the readability of overlay.c. The previous patches
renamed identifiers. This patch is split out from the previous patches
to make the previous patches easier to review.Changes are:
- minor code restructuring
- some initialization of an overlay changeset occurred outside of
init_overlay_changeset(), move that into init_overlay_changeset()
- consolidate freeing an overlay changeset into free_overlay_changeset()This patch is intended to not introduce any functional change.
Signed-off-by: Frank Rowand
Signed-off-by: Rob Herring -
More renaming of identifiers to better reflect what they do.
Signed-off-by: Frank Rowand
Signed-off-by: Rob Herring -
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 commentsThis patch is intended to not introduce any functional change.
Signed-off-by: Frank Rowand
Signed-off-by: Rob Herring -
Use normal shorthand for comparing a variable to zero.
For variable "XXX":
convert (XXX == 0) to (!XXX)
convert (XXX != 0) to (XXX)Signed-off-by: Frank Rowand
Signed-off-by: Rob Herring -
Follows recommendations in Documentation/process/coding-style.rst,
section 8, Commenting.Some in function comments are promoted to function header comments.
Signed-off-by: Frank Rowand
Signed-off-by: Rob Herring