From: Frank Rowand <redacted>
Add checks to (1) overlay apply process and (2) memory freeing
triggered by overlay release. The checks are intended to detect
possible memory leaks and invalid overlays.
The checks revealed bugs in existing code. Fixed the bugs.
While fixing bugs, noted other issues, which are fixed in
separate patches.
***** Powerpc folks: I was not able to test the patches that
***** directly impact Powerpc systems that use dynamic
***** devicetree. Please review that code carefully and
***** test. The specific patches are: 03/16, 04/16, 07/16
FPGA folks:
I made the validation checks that should result in an
invalid live devicetree report "ERROR" and cause the overlay apply
to fail.
I made the memory leak validation tests report "WARNING" and allow
the overlay apply to complete successfully. Please let me know
if you encounter the warnings. There are at least two paths
forward to deal with the cases that trigger the warning: (1) change
the warning to an error and fail the overlay apply, or (2) find a
way to detect the potential memory leaks and free the memory
appropriately.
ALL people:
The validations do _not_ address another major concern I have with
releasing overlays, which is use after free errors.
Changes since v5:
- move from 4.19-rc1 to 4.20-rc1
- all patches: add tested-by Alan Tull
- 05/18: update for context change from commit a613b26a50136 ("of:
Convert to using %pOFn instead of device_node.name")
Changes since v4:
- 01/18: make error message format consistent, error first, path last
- 09/18: create of_prop_val_eq() and change open code to use it
- 09/18: remove extra blank lines
Changes since v3:
- 01/18: Add expected value of refcount for destroy cset entry error. Also
explain the cause of the error.
- 09/18: for errors of an overlay changing the value of #size-cells or
#address-cells, return -EINVAL so that overlay apply will fail
- 09/18: for errors of an overlay changing the value of #size-cells or
#address-cells, make the message more direct.
Old message:
OF: overlay: ERROR: overlay and/or live tree #size-cells invalid in node /soc/base_fpga_region
New message:
OF: overlay: ERROR: changing value of /soc/base_fpga_region/#size-cells not allowed
- 13/18: Update patch comment header to state that this patch modifies the
previous patch to not return immediately on fragment error and
explain this is not a performance issue.
- 13/18: remove redundant "overlay" from two error messages. "OF: overlay:"
is already present in pr_fmt()
Changes since v2:
- 13/18: Use continue to reduce indentation in find_dup_cset_node_entry()
and find_dup_cset_prop()
Changes since v1:
- move patch 16/16 to 17/18
- move patch 15/16 to 18/18
- new patch 15/18
- new patch 16/18
- 05/18: add_changeset_node() header comment: incorrect comment for @target
- 18/18: add same fix for of_parse_phandle_with_args()
- 18/18: add same fix for of_parse_phandle_with_args_map()
Frank Rowand (18):
of: overlay: add tests to validate kfrees from overlay removal
of: overlay: add missing of_node_put() after add new node to changeset
of: overlay: add missing of_node_get() in __of_attach_node_sysfs
powerpc/pseries: add of_node_put() in dlpar_detach_node()
of: overlay: use prop add changeset entry for property in new nodes
of: overlay: do not duplicate properties from overlay for new nodes
of: dynamic: change type of of_{at,de}tach_node() to void
of: overlay: reorder fields in struct fragment
of: overlay: validate overlay properties #address-cells and
#size-cells
of: overlay: make all pr_debug() and pr_err() messages unique
of: overlay: test case of two fragments adding same node
of: overlay: check prevents multiple fragments add or delete same node
of: overlay: check prevents multiple fragments touching same property
of: unittest: remove unused of_unittest_apply_overlay() argument
of: overlay: set node fields from properties when add new overlay node
of: unittest: allow base devicetree to have symbol metadata
of: unittest: find overlays[] entry by name instead of index
of: unittest: initialize args before calling of_*parse_*()
arch/powerpc/platforms/pseries/dlpar.c | 15 +-
arch/powerpc/platforms/pseries/reconfig.c | 6 +-
drivers/of/dynamic.c | 68 +++--
drivers/of/kobj.c | 4 +-
drivers/of/overlay.c | 292 ++++++++++++++++-----
drivers/of/unittest-data/Makefile | 2 +
.../of/unittest-data/overlay_bad_add_dup_node.dts | 28 ++
.../of/unittest-data/overlay_bad_add_dup_prop.dts | 24 ++
drivers/of/unittest-data/overlay_base.dts | 1 +
drivers/of/unittest.c | 96 +++++--
include/linux/of.h | 25 +-
11 files changed, 439 insertions(+), 122 deletions(-)
create mode 100644 drivers/of/unittest-data/overlay_bad_add_dup_node.dts
create mode 100644 drivers/of/unittest-data/overlay_bad_add_dup_prop.dts
--
Frank Rowand [off-list ref]
From: Frank Rowand <redacted>
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 <redacted>
---
drivers/of/dynamic.c | 29 +++++++++++++++++++++++++++++
drivers/of/overlay.c | 1 +
include/linux/of.h | 15 ++++++++++-----
3 files changed, 40 insertions(+), 5 deletions(-)
@@ -138,11 +138,16 @@ static inline void of_node_put(struct device_node *node) { }externstructdevice_node*of_stdout;externraw_spinlock_tdevtree_lock;-/* flag descriptions (need to be visible even when !CONFIG_OF) */-#define OF_DYNAMIC 1 /* node and properties were allocated via kmalloc */-#define OF_DETACHED 2 /* node has been detached from the device tree */-#define OF_POPULATED 3 /* device already created for the node */-#define OF_POPULATED_BUS 4 /* of_platform_populate recursed to children of this node */+/*+*structdevice_nodeflagdescriptions+*(needtobevisibleevenwhen!CONFIG_OF)+*/+#define OF_DYNAMIC 1 /* (and properties) allocated via kmalloc */+#define OF_DETACHED 2 /* detached from the device tree */+#define OF_POPULATED 3 /* device already created */+#define OF_POPULATED_BUS 4 /* platform bus created for children */+#define OF_OVERLAY 5 /* allocated for an overlay */+#define OF_OVERLAY_FREE_CSET 6 /* in overlay cset being freed */#define OF_BAD_ADDR ((u64)-1)
From: Frank Rowand <redacted>
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 <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
drivers/of/overlay.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Frank Rowand <redacted>
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.
Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
This patch 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 patch in the series will fix the leak.
drivers/of/dynamic.c | 3 ---
drivers/of/kobj.c | 4 +++-
2 files changed, 3 insertions(+), 4 deletions(-)
From: Frank Rowand <redacted>
"of: overlay: add missing of_node_get() in __of_attach_node_sysfs"
added a missing of_node_get() to __of_attach_node_sysfs(). This
results in a refcount imbalance for nodes attached with
dlpar_attach_node(). The calling sequence from dlpar_attach_node()
to __of_attach_node_sysfs() is:
dlpar_attach_node()
of_attach_node()
__of_attach_node_sysfs()
Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
***** UNTESTED. I need people with the affected PowerPC systems
***** (systems that dynamically allocate and deallocate
***** devicetree nodes) to test this patch.
arch/powerpc/platforms/pseries/dlpar.c | 2 ++
1 file changed, 2 insertions(+)
From: Frank Rowand <redacted>
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 <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
Changes since v5:
- update for context change from a613b26a50136 "of: Convert to using
%pOFn instead of device_node.name"
drivers/of/overlay.c | 112 ++++++++++++++++++++++++++++++++++-----------------
1 file changed, 74 insertions(+), 38 deletions(-)
From: Frank Rowand <redacted>
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 <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
drivers/of/overlay.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Frank Rowand <redacted>
of_attach_node() and of_detach_node() always return zero, so
their return value is meaningless. Change their type to void
and fix all callers to ignore return value.
Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
arch/powerpc/platforms/pseries/dlpar.c | 13 ++-----------
arch/powerpc/platforms/pseries/reconfig.c | 6 +-----
drivers/of/dynamic.c | 9 ++-------
include/linux/of.h | 4 ++--
4 files changed, 7 insertions(+), 25 deletions(-)
From: Frank Rowand <redacted>
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 <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
drivers/of/overlay.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Frank Rowand <redacted>
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 <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
drivers/of/overlay.c | 32 +++++++++++++++++++++++++++++---
include/linux/of.h | 6 ++++++
2 files changed, 35 insertions(+), 3 deletions(-)
@@ -300,6 +305,7 @@ static int add_changeset_property(struct overlay_changeset *ovcs,{structproperty*new_prop=NULL,*prop;intret=0;+boolcheck_for_non_overlay_node=false;if(!of_prop_cmp(overlay_prop->name,"name")||!of_prop_cmp(overlay_prop->name,"phandle")||
@@ -322,12 +328,32 @@ static int add_changeset_property(struct overlay_changeset *ovcs,if(!new_prop)return-ENOMEM;-if(!prop)+if(!prop){+check_for_non_overlay_node=true;ret=of_changeset_add_property(&ovcs->cset,target->np,new_prop);-else+}elseif(!of_prop_cmp(prop->name,"#address-cells")){+if(!of_prop_val_eq(prop,new_prop)){+pr_err("ERROR: changing value of #address-cells is not allowed in %pOF\n",+target->np);+ret=-EINVAL;+}+}elseif(!of_prop_cmp(prop->name,"#size-cells")){+if(!of_prop_val_eq(prop,new_prop)){+pr_err("ERROR: changing value of #size-cells is not allowed in %pOF\n",+target->np);+ret=-EINVAL;+}+}else{+check_for_non_overlay_node=true;ret=of_changeset_update_property(&ovcs->cset,target->np,new_prop);+}++if(check_for_non_overlay_node&&+!of_node_check_flag(target->np,OF_OVERLAY))+pr_err("WARNING: memory leak will occur if overlay removed, property: %pOF/%s\n",+target->np,new_prop->name);if(ret){kfree(new_prop->name);
From: Frank Rowand <redacted>
Make overlay.c debug and error messages unique so that they can be
unambiguously found by grep.
Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
drivers/of/overlay.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
From: Frank Rowand <redacted>
Multiple overlay fragments adding or deleting the same node is not
supported. An attempt to do so results in an incorrect devicetree.
The node name will be munged for the second add.
After adding this patch, the unittest messages will show:
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
The incorrect (munged) node name "controller#1" can be seen in the
/proc filesystem:
$ pwd
/proc/device-tree/testcase-data-2/substation@100/motor-1
$ ls
compatible controller controller#1 name phandle spin
$ ls controller
power_bus
$ ls controller#1
power_bus_emergency
Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
drivers/of/unittest-data/Makefile | 1 +
.../of/unittest-data/overlay_bad_add_dup_node.dts | 28 ++++++++++++++++++++++
drivers/of/unittest.c | 5 ++++
3 files changed, 34 insertions(+)
create mode 100644 drivers/of/unittest-data/overlay_bad_add_dup_node.dts
From: Frank Rowand <redacted>
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 <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
checkpatch errors "line over 80 characters" and "Too many leading tabs"
are ok, they will be fixed later in this series
drivers/of/overlay.c | 58 ++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 49 insertions(+), 9 deletions(-)
From: Frank Rowand <redacted>
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 <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
drivers/of/overlay.c | 118 ++++++++++++++-------
drivers/of/unittest-data/Makefile | 1 +
.../of/unittest-data/overlay_bad_add_dup_prop.dts | 24 +++++
drivers/of/unittest-data/overlay_base.dts | 1 +
drivers/of/unittest.c | 5 +
5 files changed, 112 insertions(+), 37 deletions(-)
create mode 100644 drivers/of/unittest-data/overlay_bad_add_dup_prop.dts
From: Frank Rowand <redacted>
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 <redacted>
---
drivers/of/dynamic.c | 27 ++++++++++++++++++---------
drivers/of/overlay.c | 29 ++++++++++++++++++++++++-----
2 files changed, 42 insertions(+), 14 deletions(-)
From: Frank Rowand <redacted>
The overlay metadata nodes in the FDT created from testcases.dts
are not handled properly.
The __fixups__ and __local_fixups__ node were added to the live
devicetree, but should not be.
Only the first property in the /__symbols__ node was added to the
live devicetree if the live devicetree already contained a
/__symbols node. All of the node's properties must be added.
Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
drivers/of/unittest.c | 43 +++++++++++++++++++++++++++++++++++--------
1 file changed, 35 insertions(+), 8 deletions(-)
From: Frank Rowand <redacted>
Callers of of_irq_parse_one() blindly use the pointer args.np
without checking whether of_irq_parse_one() had an error and
thus did not set the value of args.np. Initialize args to
zero so that using the format "%pOF" to show the value of
args.np will show "(null)" when of_irq_parse_one() has an
error. This prevents the dereference of a random value.
Make the same fix for callers of of_parse_phandle_with_args()
and of_parse_phandle_with_args_map().
Reported-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
drivers/of/unittest.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
@@ -448,6 +451,7 @@ static void __init of_unittest_parse_phandle_with_args(void)unittest(rc==-EINVAL,"expected:%i got:%i\n",-EINVAL,rc);/* Check for bad phandle in list */+memset(&args,0,sizeof(args));rc=of_parse_phandle_with_args(np,"phandle-list-bad-phandle","#phandle-cells",0,&args);unittest(rc==-EINVAL,"expected:%i got:%i\n",-EINVAL,rc);
@@ -456,6 +460,7 @@ static void __init of_unittest_parse_phandle_with_args(void)unittest(rc==-EINVAL,"expected:%i got:%i\n",-EINVAL,rc);/* Check for incorrectly formed argument list */+memset(&args,0,sizeof(args));rc=of_parse_phandle_with_args(np,"phandle-list-bad-args","#phandle-cells",1,&args);unittest(rc==-EINVAL,"expected:%i got:%i\n",-EINVAL,rc);
@@ -563,21 +569,25 @@ static void __init of_unittest_parse_phandle_with_args_map(void)}/* Check for missing list property */+memset(&args,0,sizeof(args));rc=of_parse_phandle_with_args_map(np,"phandle-list-missing","phandle",0,&args);unittest(rc==-ENOENT,"expected:%i got:%i\n",-ENOENT,rc);/* Check for missing cells,map,mask property */+memset(&args,0,sizeof(args));rc=of_parse_phandle_with_args_map(np,"phandle-list","phandle-missing",0,&args);unittest(rc==-EINVAL,"expected:%i got:%i\n",-EINVAL,rc);/* Check for bad phandle in list */+memset(&args,0,sizeof(args));rc=of_parse_phandle_with_args_map(np,"phandle-list-bad-phandle","phandle",0,&args);unittest(rc==-EINVAL,"expected:%i got:%i\n",-EINVAL,rc);/* Check for incorrectly formed argument list */+memset(&args,0,sizeof(args));rc=of_parse_phandle_with_args_map(np,"phandle-list-bad-args","phandle",1,&args);unittest(rc==-EINVAL,"expected:%i got:%i\n",-EINVAL,rc);
@@ -808,7 +818,7 @@ static void __init of_unittest_parse_interrupts(void)for(i=0;i<4;i++){boolpassed=true;-args.args_count=0;+memset(&args,0,sizeof(args));rc=of_irq_parse_one(np,i,&args);/* Test the values from tests-phandle.dtsi */
@@ -864,6 +874,7 @@ static void __init of_unittest_parse_interrupts_extended(void)for(i=0;i<7;i++){boolpassed=true;+memset(&args,0,sizeof(args));rc=of_irq_parse_one(np,i,&args);/* Test the values from tests-phandle.dtsi */
From: Frank Rowand <redacted>
One accessor of overlays[] was using a hard coded index value to
find the correct array entry instead of searching for the entry
containing the correct name.
Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
drivers/of/unittest.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
@@ -2192,7 +2192,7 @@ struct overlay_info {OVERLAY_INFO_EXTERN(overlay_bad_phandle);OVERLAY_INFO_EXTERN(overlay_bad_symbol);-/* order of entries is hard-coded into users of overlays[] */+/* entries found by name */staticstructoverlay_infooverlays[]={OVERLAY_INFO(overlay_base,-9999),OVERLAY_INFO(overlay,0),
@@ -2215,7 +2215,8 @@ struct overlay_info {OVERLAY_INFO(overlay_bad_add_dup_prop,-EINVAL),OVERLAY_INFO(overlay_bad_phandle,-EINVAL),OVERLAY_INFO(overlay_bad_symbol,-EINVAL),-{}+/* end marker */+{.dtb_begin=NULL,.dtb_end=NULL,.expected_result=0,.name=NULL}};staticstructdevice_node*overlay_base_root;
@@ -2245,6 +2246,19 @@ void __init unittest_unflatten_overlay_base(void)u32data_size;void*new_fdt;u32size;+intfound=0;+constchar*overlay_name="overlay_base";++for(info=overlays;info&&info->name;info++){+if(!strcmp(overlay_name,info->name)){+found=1;+break;+}+}+if(!found){+pr_err("no overlay data for %s\n",overlay_name);+return;+}info=&overlays[0];
@@ -2292,11 +2306,10 @@ static int __init overlay_data_apply(const char *overlay_name, int *overlay_id){structoverlay_info*info;intfound=0;-intk;intret;u32size;-for(k=0,info=overlays;info&&info->name;info++,k++){+for(info=overlays;info&&info->name;info++){if(!strcmp(overlay_name,info->name)){found=1;break;
From: Frank Rowand <redacted>
Argument unittest_nr is not used in of_unittest_apply_overlay(),
remove it.
Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
drivers/of/unittest.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
From: Rob Herring <robh+dt@kernel.org> Date: 2018-11-05 19:25:30
On Mon, Nov 5, 2018 at 9:26 AM [off-list ref] wrote:
From: Frank Rowand <redacted>
Add checks to (1) overlay apply process and (2) memory freeing
triggered by overlay release. The checks are intended to detect
possible memory leaks and invalid overlays.
The checks revealed bugs in existing code. Fixed the bugs.
While fixing bugs, noted other issues, which are fixed in
separate patches.
***** Powerpc folks: I was not able to test the patches that
***** directly impact Powerpc systems that use dynamic
***** devicetree. Please review that code carefully and
***** test. The specific patches are: 03/16, 04/16, 07/16
I'm waiting for this to happen. Send me a pull req when it does or
when you give up waiting for a response.
Rob
From: Frank Rowand <hidden> Date: 2018-11-05 21:55:15
Hi Michael, Ben, Paul,
Do you know if anyone has tried this series on PowerPC?
Thanks,
-Frank
On 11/5/18 11:24 AM, Rob Herring wrote:
On Mon, Nov 5, 2018 at 9:26 AM [off-list ref] wrote:
quoted
From: Frank Rowand <redacted>
Add checks to (1) overlay apply process and (2) memory freeing
triggered by overlay release. The checks are intended to detect
possible memory leaks and invalid overlays.
The checks revealed bugs in existing code. Fixed the bugs.
While fixing bugs, noted other issues, which are fixed in
separate patches.
***** Powerpc folks: I was not able to test the patches that
***** directly impact Powerpc systems that use dynamic
***** devicetree. Please review that code carefully and
***** test. The specific patches are: 03/16, 04/16, 07/16
I'm waiting for this to happen. Send me a pull req when it does or
when you give up waiting for a response.
Rob
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2018-11-07 12:08:34
frowand.list@gmail.com writes:
From: Frank Rowand <redacted>
of_attach_node() and of_detach_node() always return zero, so
their return value is meaningless.
But should they always return zero?
At least __of_attach_node_sysfs() can fail in several ways.
And there's also this in __of_detach_node() which should probably be
returning an error:
if (WARN_ON(of_node_check_flag(np, OF_DETACHED)))
return;
Seems to me we should instead be fixing these to propagate errors,
rather than hiding them?
cheers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2018-11-07 12:09:18
Frank Rowand [off-list ref] writes:
Hi Michael, Ben, Paul,
Do you know if anyone has tried this series on PowerPC?
I have. No obvious breakage.
My test does a loop of adding and removing multiple CPUs multiple times,
and in the past that has uncovered refcounting bugs. So I don't think
we're leaking any with this series applied.
I used the tracepoint patch to keep an eye on the refcounts :)
https://patchwork.ozlabs.org/patch/751602/
I'm happy for this series to go into linux-next where it should get some
more testing.
cheers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2018-11-07 12:14:28
frowand.list@gmail.com writes:
From: Frank Rowand <redacted>
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.
Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
This seems sensible to me. I guess we could argue about whether the
sysfs code needs its own reference, but it certainly doesn't hurt that
it does, as long as it's handled symmetrically - which it is now.
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
---
This patch 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 patch in the series will fix the leak.
I think this should go in the changelog, it's useful information that we
don't want to lose track of once this is applied.
Either that or we actually squash the two patches together when applying
to avoid the bisection break.
cheers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2018-11-07 12:23:33
frowand.list@gmail.com writes:
From: Frank Rowand <redacted>
"of: overlay: add missing of_node_get() in __of_attach_node_sysfs"
It would be clearer if you said 'The previous commit "of: overlay ..."
added a missing of_node_get() to __of_attach_node_sysfs(). This
results in a refcount imbalance for nodes attached with
dlpar_attach_node(). The calling sequence from dlpar_attach_node()
to __of_attach_node_sysfs() is:
dlpar_attach_node()
of_attach_node()
__of_attach_node_sysfs()
Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
***** UNTESTED. I need people with the affected PowerPC systems
***** (systems that dynamically allocate and deallocate
***** devicetree nodes) to test this patch.
This looks OK to me in light of the previous patch.
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
It also means dlpar_detach_node() is again behaving as described in the
comment to of_detach_node().
It would be good to make mention of:
Fixes: 68baf692c435 ("powerpc/pseries: Fix of_node_put() underflow during DLPAR remove")
Which removed an of_node_put() in the exact same place for different
reasons.
cheers
From: Frank Rowand <hidden> Date: 2018-11-07 14:55:19
On 11/7/18 4:08 AM, Michael Ellerman wrote:
frowand.list@gmail.com writes:
quoted
From: Frank Rowand <redacted>
of_attach_node() and of_detach_node() always return zero, so
their return value is meaningless.
But should they always return zero?
At least __of_attach_node_sysfs() can fail in several ways.
Sigh. And of_reconfig_notify() can fail. And at one point in the
history the return value of of_reconfig_notify() was returned by
of_attach_node() if of_reconfig_notify() failed.
And there's also this in __of_detach_node() which should probably be
returning an error:
if (WARN_ON(of_node_check_flag(np, OF_DETACHED)))
return;
Seems to me we should instead be fixing these to propagate errors,
rather than hiding them?
The history of how of_attach_node() stopped propagating errors is
a bit more complex than I want to dig into at the moment. So I'll
drop this patch from the series and add investigating this onto
my todo list. I suspect that the result of investigating will be
that error return values should not be ignored in of_attach_node()
and of_detach_node(), but should instead be propagated to the
callers, as you suggest.
-Frank
From: Frank Rowand <hidden> Date: 2018-11-07 14:57:48
On 11/7/18 4:14 AM, Michael Ellerman wrote:
frowand.list@gmail.com writes:
quoted
From: Frank Rowand <redacted>
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.
Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
This seems sensible to me. I guess we could argue about whether the
sysfs code needs its own reference, but it certainly doesn't hurt that
it does, as long as it's handled symmetrically - which it is now.
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
quoted
---
This patch 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 patch in the series will fix the leak.
I think this should go in the changelog, it's useful information that we
don't want to lose track of once this is applied.
Will do.
-Frank
Either that or we actually squash the two patches together when applying
to avoid the bisection break.
cheers
From: Frank Rowand <hidden> Date: 2018-11-07 14:59:00
On 11/7/18 4:23 AM, Michael Ellerman wrote:
frowand.list@gmail.com writes:
quoted
From: Frank Rowand <redacted>
"of: overlay: add missing of_node_get() in __of_attach_node_sysfs"
It would be clearer if you said 'The previous commit "of: overlay ..."
Will fix.
quoted
added a missing of_node_get() to __of_attach_node_sysfs(). This
results in a refcount imbalance for nodes attached with
dlpar_attach_node(). The calling sequence from dlpar_attach_node()
to __of_attach_node_sysfs() is:
dlpar_attach_node()
of_attach_node()
__of_attach_node_sysfs()
Tested-by: Alan Tull <atull@kernel.org>
Signed-off-by: Frank Rowand <redacted>
---
***** UNTESTED. I need people with the affected PowerPC systems
***** (systems that dynamically allocate and deallocate
***** devicetree nodes) to test this patch.
This looks OK to me in light of the previous patch.
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
It also means dlpar_detach_node() is again behaving as described in the
comment to of_detach_node().
It would be good to make mention of:
Fixes: 68baf692c435 ("powerpc/pseries: Fix of_node_put() underflow during DLPAR remove")
Which removed an of_node_put() in the exact same place for different
reasons.
From: Frank Rowand <hidden> Date: 2018-11-07 14:59:57
On 11/7/18 4:09 AM, Michael Ellerman wrote:
Frank Rowand [off-list ref] writes:
quoted
Hi Michael, Ben, Paul,
Do you know if anyone has tried this series on PowerPC?
I have. No obvious breakage.
My test does a loop of adding and removing multiple CPUs multiple times,
and in the past that has uncovered refcounting bugs. So I don't think
we're leaking any with this series applied.
I used the tracepoint patch to keep an eye on the refcounts :)
https://patchwork.ozlabs.org/patch/751602/
I'm happy for this series to go into linux-next where it should get some
more testing.
cheers
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2018-11-07 23:48:39
Frank Rowand [off-list ref] writes:
On 11/7/18 4:08 AM, Michael Ellerman wrote:
quoted
frowand.list@gmail.com writes:
quoted
From: Frank Rowand <redacted>
of_attach_node() and of_detach_node() always return zero, so
their return value is meaningless.
But should they always return zero?
At least __of_attach_node_sysfs() can fail in several ways.
Sigh. And of_reconfig_notify() can fail. And at one point in the
history the return value of of_reconfig_notify() was returned by
of_attach_node() if of_reconfig_notify() failed.
quoted
And there's also this in __of_detach_node() which should probably be
returning an error:
if (WARN_ON(of_node_check_flag(np, OF_DETACHED)))
return;
Seems to me we should instead be fixing these to propagate errors,
rather than hiding them?
The history of how of_attach_node() stopped propagating errors is
a bit more complex than I want to dig into at the moment. So I'll
drop this patch from the series and add investigating this onto
my todo list. I suspect that the result of investigating will be
that error return values should not be ignored in of_attach_node()
and of_detach_node(), but should instead be propagated to the
callers, as you suggest.