* [PATCH v4 11/18] of: overlay: test case of two fragments adding same node
From: frowand.list @ 2018-10-16 2:37 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
In-Reply-To: <1539657458-24401-1-git-send-email-frowand.list@gmail.com>
From: Frank Rowand <frank.rowand@sony.com>
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
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
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
diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index 013d85e694c6..166dbdbfd1c5 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_OF_OVERLAY) += overlay.dtb.o \
overlay_12.dtb.o \
overlay_13.dtb.o \
overlay_15.dtb.o \
+ overlay_bad_add_dup_node.dtb.o \
overlay_bad_phandle.dtb.o \
overlay_bad_symbol.dtb.o \
overlay_base.dtb.o
diff --git a/drivers/of/unittest-data/overlay_bad_add_dup_node.dts b/drivers/of/unittest-data/overlay_bad_add_dup_node.dts
new file mode 100644
index 000000000000..145dfc3b1024
--- /dev/null
+++ b/drivers/of/unittest-data/overlay_bad_add_dup_node.dts
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+/dts-v1/;
+/plugin/;
+
+/*
+ * &electric_1/motor-1 and &spin_ctrl_1 are the same node:
+ * /testcase-data-2/substation@100/motor-1
+ *
+ * Thus the new node "controller" in each fragment will
+ * result in an attempt to add the same node twice.
+ * This will result in an error and the overlay apply
+ * will fail.
+ */
+
+&electric_1 {
+
+ motor-1 {
+ controller {
+ power_bus = < 0x1 0x2 >;
+ };
+ };
+};
+
+&spin_ctrl_1 {
+ controller {
+ power_bus_emergency = < 0x101 0x102 >;
+ };
+};
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 722537e14848..471b8eb6e842 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -2147,6 +2147,7 @@ struct overlay_info {
OVERLAY_INFO_EXTERN(overlay_12);
OVERLAY_INFO_EXTERN(overlay_13);
OVERLAY_INFO_EXTERN(overlay_15);
+OVERLAY_INFO_EXTERN(overlay_bad_add_dup_node);
OVERLAY_INFO_EXTERN(overlay_bad_phandle);
OVERLAY_INFO_EXTERN(overlay_bad_symbol);
@@ -2169,6 +2170,7 @@ struct overlay_info {
OVERLAY_INFO(overlay_12, 0),
OVERLAY_INFO(overlay_13, 0),
OVERLAY_INFO(overlay_15, 0),
+ OVERLAY_INFO(overlay_bad_add_dup_node, -EINVAL),
OVERLAY_INFO(overlay_bad_phandle, -EINVAL),
OVERLAY_INFO(overlay_bad_symbol, -EINVAL),
{}
@@ -2413,6 +2415,9 @@ static __init void of_unittest_overlay_high_level(void)
unittest(overlay_data_apply("overlay", NULL),
"Adding overlay 'overlay' failed\n");
+ unittest(overlay_data_apply("overlay_bad_add_dup_node", NULL),
+ "Adding overlay 'overlay_bad_add_dup_node' failed\n");
+
unittest(overlay_data_apply("overlay_bad_phandle", NULL),
"Adding overlay 'overlay_bad_phandle' failed\n");
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
* [PATCH v4 10/18] of: overlay: make all pr_debug() and pr_err() messages unique
From: frowand.list @ 2018-10-16 2:37 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
In-Reply-To: <1539657458-24401-1-git-send-email-frowand.list@gmail.com>
From: Frank Rowand <frank.rowand@sony.com>
Make overlay.c debug and error messages unique so that they can be
unambiguously found by grep.
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
drivers/of/overlay.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 3e1e519c12f0..34396d6db1b3 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -517,7 +517,7 @@ static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
for_each_property_of_node(overlay_symbols_node, prop) {
ret = add_changeset_property(ovcs, target, prop, 1);
if (ret) {
- pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
+ pr_debug("Failed to apply symbols prop @%pOF/%s, err=%d\n",
target->np, prop->name, ret);
return ret;
}
@@ -561,7 +561,8 @@ static int build_changeset(struct overlay_changeset *ovcs)
ret = build_changeset_next_level(ovcs, &target,
fragment->overlay);
if (ret) {
- pr_debug("apply failed '%pOF'\n", fragment->target);
+ pr_debug("fragment apply failed '%pOF'\n",
+ fragment->target);
return ret;
}
}
@@ -574,7 +575,8 @@ static int build_changeset(struct overlay_changeset *ovcs)
ret = build_changeset_symbols_node(ovcs, &target,
fragment->overlay);
if (ret) {
- pr_debug("apply failed '%pOF'\n", fragment->target);
+ pr_debug("symbols fragment apply failed '%pOF'\n",
+ fragment->target);
return ret;
}
}
@@ -883,7 +885,7 @@ static int of_overlay_apply(const void *fdt, struct device_node *tree,
ret = __of_changeset_apply_notify(&ovcs->cset);
if (ret)
- pr_err("overlay changeset entry notify error %d\n", ret);
+ pr_err("overlay apply changeset entry notify error %d\n", ret);
/* notify failure is not fatal, continue */
list_add_tail(&ovcs->ovcs_list, &ovcs_list);
@@ -1142,7 +1144,7 @@ int of_overlay_remove(int *ovcs_id)
ret = __of_changeset_revert_notify(&ovcs->cset);
if (ret)
- pr_err("overlay changeset entry notify error %d\n", ret);
+ pr_err("overlay remove changeset entry notify error %d\n", ret);
/* notify failure is not fatal, continue */
*ovcs_id = 0;
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
* [PATCH v4 09/18] of: overlay: validate overlay properties #address-cells and #size-cells
From: frowand.list @ 2018-10-16 2:37 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
In-Reply-To: <1539657458-24401-1-git-send-email-frowand.list@gmail.com>
From: Frank Rowand <frank.rowand@sony.com>
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. This is
verified by 12 fewer warnings during the devicetree unittest,
as the possible memory leak warnings about #address-cells and
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
Changes since v3:
- for errors of an overlay changing the value of #size-cells or
#address-cells, return -EINVAL so that overlay apply will fail
- 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
drivers/of/overlay.c | 42 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 272a0d1a5e18..3e1e519c12f0 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -287,7 +287,12 @@ static struct property *dup_and_fixup_symbol_prop(
* @target may be either in the live devicetree or in a new subtree that
* is contained in the changeset.
*
- * Some special properties are not updated (no error returned).
+ * Some special properties are not added or updated (no error returned):
+ * "name", "phandle", "linux,phandle".
+ *
+ * Properties "#address-cells" and "#size-cells" are not updated if they
+ * are already in the live tree, but if present in the live tree, the values
+ * in the overlay must match the values in the live tree.
*
* Update of property in symbols node is not allowed.
*
@@ -300,6 +305,7 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
{
struct property *new_prop = NULL, *prop;
int ret = 0;
+ bool check_for_non_overlay_node = false;
if (!of_prop_cmp(overlay_prop->name, "name") ||
!of_prop_cmp(overlay_prop->name, "phandle") ||
@@ -322,13 +328,43 @@ 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
+
+ } else if (!of_prop_cmp(prop->name, "#address-cells")) {
+
+ if (prop->length != 4 || new_prop->length != 4 ||
+ *(u32 *)prop->value != *(u32 *)new_prop->value) {
+ pr_err("ERROR: changing value of %pOF/#address-cells is not allowed\n",
+ target->np);
+ ret = -EINVAL;
+ }
+
+ } else if (!of_prop_cmp(prop->name, "#size-cells")) {
+
+ if (prop->length != 4 || new_prop->length != 4 ||
+ *(u32 *)prop->value != *(u32 *)new_prop->value) {
+ pr_err("ERROR: changing value of %pOF/#size-cells is not allowed\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: %s(), memory leak will occur if overlay removed. Property: %pOF/%s\n",
+ __func__, target->np, new_prop->name);
+
if (ret) {
kfree(new_prop->name);
kfree(new_prop->value);
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
* [PATCH v4 08/18] of: overlay: reorder fields in struct fragment
From: frowand.list @ 2018-10-16 2:37 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
In-Reply-To: <1539657458-24401-1-git-send-email-frowand.list@gmail.com>
From: Frank Rowand <frank.rowand@sony.com>
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.
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
drivers/of/overlay.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 7fcf4a812d06..272a0d1a5e18 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -49,8 +49,8 @@ struct target {
* @overlay: pointer to the __overlay__ node
*/
struct fragment {
- struct device_node *target;
struct device_node *overlay;
+ struct device_node *target;
};
/**
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
* [PATCH v4 07/18] of: dynamic: change type of of_{at, de}tach_node() to void
From: frowand.list @ 2018-10-16 2:37 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
In-Reply-To: <1539657458-24401-1-git-send-email-frowand.list@gmail.com>
From: Frank Rowand <frank.rowand@sony.com>
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.
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
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(-)
diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index e3010b14aea5..0027eea94a8b 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -244,15 +244,9 @@ struct device_node *dlpar_configure_connector(__be32 drc_index,
int dlpar_attach_node(struct device_node *dn, struct device_node *parent)
{
- int rc;
-
dn->parent = parent;
- rc = of_attach_node(dn);
- if (rc) {
- printk(KERN_ERR "Failed to add device node %pOF\n", dn);
- return rc;
- }
+ of_attach_node(dn);
return 0;
}
@@ -260,7 +254,6 @@ int dlpar_attach_node(struct device_node *dn, struct device_node *parent)
int dlpar_detach_node(struct device_node *dn)
{
struct device_node *child;
- int rc;
child = of_get_next_child(dn, NULL);
while (child) {
@@ -268,9 +261,7 @@ int dlpar_detach_node(struct device_node *dn)
child = of_get_next_child(dn, child);
}
- rc = of_detach_node(dn);
- if (rc)
- return rc;
+ of_detach_node(dn);
of_node_put(dn);
diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 0e0208117e77..0b72098da454 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c
@@ -47,11 +47,7 @@ static int pSeries_reconfig_add_node(const char *path, struct property *proplist
goto out_err;
}
- err = of_attach_node(np);
- if (err) {
- printk(KERN_ERR "Failed to add device node %s\n", path);
- goto out_err;
- }
+ of_attach_node(np);
of_node_put(np->parent);
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index fe8816cca99b..a94f727ec3da 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -224,7 +224,7 @@ static void __of_attach_node(struct device_node *np)
/**
* of_attach_node() - Plug a device node into the tree and global list.
*/
-int of_attach_node(struct device_node *np)
+void of_attach_node(struct device_node *np)
{
struct of_reconfig_data rd;
unsigned long flags;
@@ -241,8 +241,6 @@ int of_attach_node(struct device_node *np)
mutex_unlock(&of_mutex);
of_reconfig_notify(OF_RECONFIG_ATTACH_NODE, &rd);
-
- return 0;
}
void __of_detach_node(struct device_node *np)
@@ -273,11 +271,10 @@ void __of_detach_node(struct device_node *np)
/**
* of_detach_node() - "Unplug" a node from the device tree.
*/
-int of_detach_node(struct device_node *np)
+void of_detach_node(struct device_node *np)
{
struct of_reconfig_data rd;
unsigned long flags;
- int rc = 0;
memset(&rd, 0, sizeof(rd));
rd.dn = np;
@@ -291,8 +288,6 @@ int of_detach_node(struct device_node *np)
mutex_unlock(&of_mutex);
of_reconfig_notify(OF_RECONFIG_DETACH_NODE, &rd);
-
- return rc;
}
EXPORT_SYMBOL_GPL(of_detach_node);
diff --git a/include/linux/of.h b/include/linux/of.h
index aa1dafaec6ae..72c593455019 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -406,8 +406,8 @@ extern int of_phandle_iterator_args(struct of_phandle_iterator *it,
#define OF_RECONFIG_REMOVE_PROPERTY 0x0004
#define OF_RECONFIG_UPDATE_PROPERTY 0x0005
-extern int of_attach_node(struct device_node *);
-extern int of_detach_node(struct device_node *);
+extern void of_attach_node(struct device_node *np);
+extern void of_detach_node(struct device_node *np);
#define of_match_ptr(_ptr) (_ptr)
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
* [PATCH v4 06/18] of: overlay: do not duplicate properties from overlay for new nodes
From: frowand.list @ 2018-10-16 2:37 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
In-Reply-To: <1539657458-24401-1-git-send-email-frowand.list@gmail.com>
From: Frank Rowand <frank.rowand@sony.com>
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
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
drivers/of/overlay.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 94740f4ee34c..7fcf4a812d06 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -393,7 +393,7 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
break;
if (!tchild) {
- tchild = __of_node_dup(node, node_kbasename);
+ tchild = __of_node_dup(NULL, node_kbasename);
if (!tchild)
return -ENOMEM;
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
* [PATCH v4 05/18] of: overlay: use prop add changeset entry for property in new nodes
From: frowand.list @ 2018-10-16 2:37 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
In-Reply-To: <1539657458-24401-1-git-send-email-frowand.list@gmail.com>
From: Frank Rowand <frank.rowand@sony.com>
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
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
drivers/of/overlay.c | 112 ++++++++++++++++++++++++++++++++++-----------------
1 file changed, 74 insertions(+), 38 deletions(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 32cfee68f2e3..94740f4ee34c 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -24,6 +24,26 @@
#include "of_private.h"
/**
+ * struct target - info about current target node as recursing through overlay
+ * @np: node where current level of overlay will be applied
+ * @in_livetree: @np is a node in the live devicetree
+ *
+ * Used in the algorithm to create the portion of a changeset that describes
+ * an overlay fragment, which is a devicetree subtree. Initially @np is a node
+ * in the live devicetree where the overlay subtree is targeted to be grafted
+ * into. When recursing to the next level of the overlay subtree, the target
+ * also recurses to the next level of the live devicetree, as long as overlay
+ * subtree node also exists in the live devicetree. When a node in the overlay
+ * subtree does not exist at the same level in the live devicetree, target->np
+ * points to a newly allocated node, and all subsequent targets in the subtree
+ * will be newly allocated nodes.
+ */
+struct target {
+ struct device_node *np;
+ bool in_livetree;
+};
+
+/**
* struct fragment - info about fragment nodes in overlay expanded device tree
* @target: target of the overlay operation
* @overlay: pointer to the __overlay__ node
@@ -72,8 +92,7 @@ static int devicetree_corrupt(void)
}
static int build_changeset_next_level(struct overlay_changeset *ovcs,
- struct device_node *target_node,
- const struct device_node *overlay_node);
+ struct target *target, const struct device_node *overlay_node);
/*
* of_resolve_phandles() finds the largest phandle in the live tree.
@@ -257,14 +276,17 @@ static struct property *dup_and_fixup_symbol_prop(
/**
* add_changeset_property() - add @overlay_prop to overlay changeset
* @ovcs: overlay changeset
- * @target_node: where to place @overlay_prop in live tree
+ * @target: where @overlay_prop will be placed
* @overlay_prop: property to add or update, from overlay tree
* @is_symbols_prop: 1 if @overlay_prop is from node "/__symbols__"
*
- * If @overlay_prop does not already exist in @target_node, add changeset entry
- * to add @overlay_prop in @target_node, else add changeset entry to update
+ * If @overlay_prop does not already exist in live devicetree, add changeset
+ * entry to add @overlay_prop in @target, else add changeset entry to update
* value of @overlay_prop.
*
+ * @target may be either in the live devicetree or in a new subtree that
+ * is contained in the changeset.
+ *
* Some special properties are not updated (no error returned).
*
* Update of property in symbols node is not allowed.
@@ -273,20 +295,22 @@ static struct property *dup_and_fixup_symbol_prop(
* invalid @overlay.
*/
static int add_changeset_property(struct overlay_changeset *ovcs,
- struct device_node *target_node,
- struct property *overlay_prop,
+ struct target *target, struct property *overlay_prop,
bool is_symbols_prop)
{
struct property *new_prop = NULL, *prop;
int ret = 0;
- prop = of_find_property(target_node, overlay_prop->name, NULL);
-
if (!of_prop_cmp(overlay_prop->name, "name") ||
!of_prop_cmp(overlay_prop->name, "phandle") ||
!of_prop_cmp(overlay_prop->name, "linux,phandle"))
return 0;
+ if (target->in_livetree)
+ prop = of_find_property(target->np, overlay_prop->name, NULL);
+ else
+ prop = NULL;
+
if (is_symbols_prop) {
if (prop)
return -EINVAL;
@@ -299,10 +323,10 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
return -ENOMEM;
if (!prop)
- ret = of_changeset_add_property(&ovcs->cset, target_node,
+ ret = of_changeset_add_property(&ovcs->cset, target->np,
new_prop);
else
- ret = of_changeset_update_property(&ovcs->cset, target_node,
+ ret = of_changeset_update_property(&ovcs->cset, target->np,
new_prop);
if (ret) {
@@ -315,14 +339,14 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
/**
* add_changeset_node() - add @node (and children) to overlay changeset
- * @ovcs: overlay changeset
- * @target_node: where to place @node in live tree
- * @node: node from within overlay device tree fragment
+ * @ovcs: overlay changeset
+ * @target: where @node will be placed in live tree or changeset
+ * @node: node from within overlay device tree fragment
*
- * If @node does not already exist in @target_node, add changeset entry
- * to add @node in @target_node.
+ * If @node does not already exist in @target, add changeset entry
+ * to add @node in @target.
*
- * If @node already exists in @target_node, and the existing node has
+ * If @node already exists in @target, and the existing node has
* a phandle, the overlay node is not allowed to have a phandle.
*
* If @node has child nodes, add the children recursively via
@@ -355,15 +379,16 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
* invalid @overlay.
*/
static int add_changeset_node(struct overlay_changeset *ovcs,
- struct device_node *target_node, struct device_node *node)
+ struct target *target, struct device_node *node)
{
const char *node_kbasename;
struct device_node *tchild;
+ struct target target_child;
int ret = 0;
node_kbasename = kbasename(node->full_name);
- for_each_child_of_node(target_node, tchild)
+ for_each_child_of_node(target->np, tchild)
if (!of_node_cmp(node_kbasename, kbasename(tchild->full_name)))
break;
@@ -372,22 +397,28 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
if (!tchild)
return -ENOMEM;
- tchild->parent = target_node;
+ tchild->parent = target->np;
of_node_set_flag(tchild, OF_OVERLAY);
ret = of_changeset_attach_node(&ovcs->cset, tchild);
if (ret)
return ret;
- ret = build_changeset_next_level(ovcs, tchild, node);
+ target_child.np = tchild;
+ target_child.in_livetree = false;
+
+ ret = build_changeset_next_level(ovcs, &target_child, node);
of_node_put(tchild);
return ret;
}
- if (node->phandle && tchild->phandle)
+ if (node->phandle && tchild->phandle) {
ret = -EINVAL;
- else
- ret = build_changeset_next_level(ovcs, tchild, node);
+ } else {
+ target_child.np = tchild;
+ target_child.in_livetree = target->in_livetree;
+ ret = build_changeset_next_level(ovcs, &target_child, node);
+ }
of_node_put(tchild);
return ret;
@@ -396,7 +427,7 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
/**
* build_changeset_next_level() - add level of overlay changeset
* @ovcs: overlay changeset
- * @target_node: where to place @overlay_node in live tree
+ * @target: where to place @overlay_node in live tree
* @overlay_node: node from within an overlay device tree fragment
*
* Add the properties (if any) and nodes (if any) from @overlay_node to the
@@ -409,27 +440,26 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
* invalid @overlay_node.
*/
static int build_changeset_next_level(struct overlay_changeset *ovcs,
- struct device_node *target_node,
- const struct device_node *overlay_node)
+ struct target *target, const struct device_node *overlay_node)
{
struct device_node *child;
struct property *prop;
int ret;
for_each_property_of_node(overlay_node, prop) {
- ret = add_changeset_property(ovcs, target_node, prop, 0);
+ ret = add_changeset_property(ovcs, target, prop, 0);
if (ret) {
pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
- target_node, prop->name, ret);
+ target->np, prop->name, ret);
return ret;
}
}
for_each_child_of_node(overlay_node, child) {
- ret = add_changeset_node(ovcs, target_node, child);
+ ret = add_changeset_node(ovcs, target, child);
if (ret) {
pr_debug("Failed to apply node @%pOF/%s, err=%d\n",
- target_node, child->name, ret);
+ target->np, child->name, ret);
of_node_put(child);
return ret;
}
@@ -442,17 +472,17 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
* Add the properties from __overlay__ node to the @ovcs->cset changeset.
*/
static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
- struct device_node *target_node,
+ struct target *target,
const struct device_node *overlay_symbols_node)
{
struct property *prop;
int ret;
for_each_property_of_node(overlay_symbols_node, prop) {
- ret = add_changeset_property(ovcs, target_node, prop, 1);
+ ret = add_changeset_property(ovcs, target, prop, 1);
if (ret) {
pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
- target_node, prop->name, ret);
+ target->np, prop->name, ret);
return ret;
}
}
@@ -475,6 +505,7 @@ static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
static int build_changeset(struct overlay_changeset *ovcs)
{
struct fragment *fragment;
+ struct target target;
int fragments_count, i, ret;
/*
@@ -489,7 +520,9 @@ static int build_changeset(struct overlay_changeset *ovcs)
for (i = 0; i < fragments_count; i++) {
fragment = &ovcs->fragments[i];
- ret = build_changeset_next_level(ovcs, fragment->target,
+ target.np = fragment->target;
+ target.in_livetree = true;
+ ret = build_changeset_next_level(ovcs, &target,
fragment->overlay);
if (ret) {
pr_debug("apply failed '%pOF'\n", fragment->target);
@@ -499,7 +532,10 @@ static int build_changeset(struct overlay_changeset *ovcs)
if (ovcs->symbols_fragment) {
fragment = &ovcs->fragments[ovcs->count - 1];
- ret = build_changeset_symbols_node(ovcs, fragment->target,
+
+ target.np = fragment->target;
+ target.in_livetree = true;
+ ret = build_changeset_symbols_node(ovcs, &target,
fragment->overlay);
if (ret) {
pr_debug("apply failed '%pOF'\n", fragment->target);
@@ -517,7 +553,7 @@ static int build_changeset(struct overlay_changeset *ovcs)
* 1) "target" property containing the phandle of the target
* 2) "target-path" property containing the path of the target
*/
-static struct device_node *find_target_node(struct device_node *info_node)
+static struct device_node *find_target(struct device_node *info_node)
{
struct device_node *node;
const char *path;
@@ -623,7 +659,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
fragment = &fragments[cnt];
fragment->overlay = overlay_node;
- fragment->target = find_target_node(node);
+ fragment->target = find_target(node);
if (!fragment->target) {
of_node_put(fragment->overlay);
ret = -EINVAL;
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
* [PATCH v4 04/18] powerpc/pseries: add of_node_put() in dlpar_detach_node()
From: frowand.list @ 2018-10-16 2:37 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
In-Reply-To: <1539657458-24401-1-git-send-email-frowand.list@gmail.com>
From: Frank Rowand <frank.rowand@sony.com>
"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()
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
***** 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(+)
diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index a0b20c03f078..e3010b14aea5 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -272,6 +272,8 @@ int dlpar_detach_node(struct device_node *dn)
if (rc)
return rc;
+ of_node_put(dn);
+
return 0;
}
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
* [PATCH v4 03/18] of: overlay: add missing of_node_get() in __of_attach_node_sysfs
From: frowand.list @ 2018-10-16 2:37 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
In-Reply-To: <1539657458-24401-1-git-send-email-frowand.list@gmail.com>
From: 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.
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
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(-)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 24c97b7a050f..fe8816cca99b 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -272,9 +272,6 @@ void __of_detach_node(struct device_node *np)
/**
* of_detach_node() - "Unplug" a node from the device tree.
- *
- * The caller must hold a reference to the node. The memory associated with
- * the node is not freed until its refcount goes to zero.
*/
int of_detach_node(struct device_node *np)
{
diff --git a/drivers/of/kobj.c b/drivers/of/kobj.c
index 7a0a18980b98..c72eef988041 100644
--- a/drivers/of/kobj.c
+++ b/drivers/of/kobj.c
@@ -133,6 +133,9 @@ int __of_attach_node_sysfs(struct device_node *np)
}
if (!name)
return -ENOMEM;
+
+ of_node_get(np);
+
rc = kobject_add(&np->kobj, parent, "%s", name);
kfree(name);
if (rc)
@@ -159,6 +162,5 @@ void __of_detach_node_sysfs(struct device_node *np)
kobject_del(&np->kobj);
}
- /* finally remove the kobj_init ref */
of_node_put(np);
}
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
* [PATCH v4 02/18] of: overlay: add missing of_node_put() after add new node to changeset
From: frowand.list @ 2018-10-16 2:37 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
In-Reply-To: <1539657458-24401-1-git-send-email-frowand.list@gmail.com>
From: Frank Rowand <frank.rowand@sony.com>
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 of_node_release() overlay node /testcase-data/overlay-node/test-bus/test-unittest4 before free overlay changeset
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)
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
drivers/of/overlay.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 1176cb4b6e4e..32cfee68f2e3 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -379,7 +379,9 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
if (ret)
return ret;
- return build_changeset_next_level(ovcs, tchild, node);
+ ret = build_changeset_next_level(ovcs, tchild, node);
+ of_node_put(tchild);
+ return ret;
}
if (node->phandle && tchild->phandle)
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
* [PATCH v4 01/18] of: overlay: add tests to validate kfrees from overlay removal
From: frowand.list @ 2018-10-16 2:37 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
In-Reply-To: <1539657458-24401-1-git-send-email-frowand.list@gmail.com>
From: 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() overlay node /testcase-data/overlay-node/test-bus/test-unittest11/test-unittest111 contains unexpected properties
OF: ERROR: memory leak - destroy cset entry: attach overlay node /testcase-data-2/substation@100/hvac-medium-2 expected refcount 1 instead of 2. of_node_get() / of_node_put() are unbalanced for this node.
Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---
Changes since v3:
- Add expected value of refcount for destroy cset entry error. Also
explain the cause of the error.
drivers/of/dynamic.c | 29 +++++++++++++++++++++++++++++
drivers/of/overlay.c | 1 +
include/linux/of.h | 15 ++++++++++-----
3 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index f4f8ed9b5454..24c97b7a050f 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -330,6 +330,25 @@ void of_node_release(struct kobject *kobj)
if (!of_node_check_flag(node, OF_DYNAMIC))
return;
+ if (of_node_check_flag(node, OF_OVERLAY)) {
+
+ if (!of_node_check_flag(node, OF_OVERLAY_FREE_CSET)) {
+ /* premature refcount of zero, do not free memory */
+ pr_err("ERROR: memory leak %s() overlay node %pOF before free overlay changeset\n",
+ __func__, node);
+ return;
+ }
+
+ /*
+ * If node->properties non-empty then properties were added
+ * to this node either by different overlay that has not
+ * yet been removed, or by a non-overlay mechanism.
+ */
+ if (node->properties)
+ pr_err("ERROR: %s() overlay node %pOF contains unexpected properties\n",
+ __func__, node);
+ }
+
property_list_free(node->properties);
property_list_free(node->deadprops);
@@ -434,6 +453,16 @@ struct device_node *__of_node_dup(const struct device_node *np,
static void __of_changeset_entry_destroy(struct of_changeset_entry *ce)
{
+ if (ce->action == OF_RECONFIG_ATTACH_NODE &&
+ of_node_check_flag(ce->np, OF_OVERLAY)) {
+ if (kref_read(&ce->np->kobj.kref) > 1) {
+ pr_err("ERROR: memory leak - destroy cset entry: attach overlay node %pOF expected refcount 1 instead of %d. of_node_get() / of_node_put() are unbalanced for this node.\n",
+ ce->np, kref_read(&ce->np->kobj.kref));
+ } else {
+ of_node_set_flag(ce->np, OF_OVERLAY_FREE_CSET);
+ }
+ }
+
of_node_put(ce->np);
list_del(&ce->node);
kfree(ce);
diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index eda57ef12fd0..1176cb4b6e4e 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -373,6 +373,7 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
return -ENOMEM;
tchild->parent = target_node;
+ of_node_set_flag(tchild, OF_OVERLAY);
ret = of_changeset_attach_node(&ovcs->cset, tchild);
if (ret)
diff --git a/include/linux/of.h b/include/linux/of.h
index 4d25e4f952d9..aa1dafaec6ae 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -138,11 +138,16 @@ static inline void of_node_put(struct device_node *node) { }
extern struct device_node *of_stdout;
extern raw_spinlock_t devtree_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 */
+/*
+ * struct device_node flag descriptions
+ * (need to be visible even when !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)
--
Frank Rowand <frank.rowand@sony.com>
^ permalink raw reply related
* [PATCH v4 00/18] of: overlay: validation checks, subsequent fixes
From: frowand.list @ 2018-10-16 2:37 UTC (permalink / raw)
To: Rob Herring, Pantelis Antoniou, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Alan Tull, Moritz Fischer
Cc: devicetree, linux-fpga, linuxppc-dev, linux-kernel
From: Frank Rowand <frank.rowand@sony.com>
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 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 | 302 ++++++++++++++++-----
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 | 19 +-
11 files changed, 443 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 <frank.rowand@sony.com>
^ permalink raw reply
* [PATCH kernel] powerpc/powernv/ioda: Reduce a number of hooks in pnv_phb
From: Alexey Kardashevskiy @ 2018-10-16 2:34 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alexey Kardashevskiy
fixup_phb() is never used, this removes it.
pick_m64_pe() and reserve_m64_pe() are always defined for all powernv
PHBs: they are initialized by pnv_ioda_parse_m64_window() which is
called unconditionally from pnv_pci_init_ioda_phb() which initializes
all known PHB types on powernv so we can open code them.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
arch/powerpc/platforms/powernv/pci.h | 4 ----
arch/powerpc/platforms/powernv/pci-ioda.c | 9 +++------
2 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci.h b/arch/powerpc/platforms/powernv/pci.h
index 8b37b28..2131373 100644
--- a/arch/powerpc/platforms/powernv/pci.h
+++ b/arch/powerpc/platforms/powernv/pci.h
@@ -115,11 +115,7 @@ struct pnv_phb {
unsigned int hwirq, unsigned int virq,
unsigned int is_64, struct msi_msg *msg);
void (*dma_dev_setup)(struct pnv_phb *phb, struct pci_dev *pdev);
- void (*fixup_phb)(struct pci_controller *hose);
int (*init_m64)(struct pnv_phb *phb);
- void (*reserve_m64_pe)(struct pci_bus *bus,
- unsigned long *pe_bitmap, bool all);
- struct pnv_ioda_pe *(*pick_m64_pe)(struct pci_bus *bus, bool all);
int (*get_pe_state)(struct pnv_phb *phb, int pe_no);
void (*freeze_pe)(struct pnv_phb *phb, int pe_no);
int (*unfreeze_pe)(struct pnv_phb *phb, int pe_no, int opt);
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index 78b61f0..15a4556 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -518,8 +518,6 @@ static void __init pnv_ioda_parse_m64_window(struct pnv_phb *phb)
phb->init_m64 = pnv_ioda1_init_m64;
else
phb->init_m64 = pnv_ioda2_init_m64;
- phb->reserve_m64_pe = pnv_ioda_reserve_m64_pe;
- phb->pick_m64_pe = pnv_ioda_pick_m64_pe;
}
static void pnv_ioda_freeze_pe(struct pnv_phb *phb, int pe_no)
@@ -1161,8 +1159,8 @@ static struct pnv_ioda_pe *pnv_ioda_setup_bus_PE(struct pci_bus *bus, bool all)
pe = &phb->ioda.pe_array[phb->ioda.root_pe_idx];
/* Check if PE is determined by M64 */
- if (!pe && phb->pick_m64_pe)
- pe = phb->pick_m64_pe(bus, all);
+ if (!pe)
+ pe = pnv_ioda_pick_m64_pe(bus, all);
/* The PE number isn't pinned by M64 */
if (!pe)
@@ -3395,8 +3393,7 @@ static void pnv_pci_setup_bridge(struct pci_bus *bus, unsigned long type)
return;
/* Reserve PEs according to used M64 resources */
- if (phb->reserve_m64_pe)
- phb->reserve_m64_pe(bus, NULL, all);
+ pnv_ioda_reserve_m64_pe(bus, NULL, all);
/*
* Assign PE. We might run here because of partial hotplug.
--
2.11.0
^ permalink raw reply related
* [PATCH kernel] powerpc/powernv/ioda1: Remove dead code for a single device PE
From: Alexey Kardashevskiy @ 2018-10-16 2:30 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Alexey Kardashevskiy
At the moment PNV_IODA_PE_DEV is only used for NPU PEs which are not
present on IODA1 machines (i.e. POWER7) so let's remove a piece of
dead code.
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
We might actually want to get rid of the entire IODA1 there.
---
arch/powerpc/platforms/powernv/pci-ioda.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
index cde7102..78b61f0 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -2367,15 +2367,7 @@ static void pnv_pci_ioda1_setup_dma_pe(struct pnv_phb *phb,
pe->table_group.tce32_size = tbl->it_size << tbl->it_page_shift;
iommu_init_table(tbl, phb->hose->node);
- if (pe->flags & PNV_IODA_PE_DEV) {
- /*
- * Setting table base here only for carrying iommu_group
- * further down to let iommu_add_device() do the job.
- * pnv_pci_ioda_dma_dev_setup will override it later anyway.
- */
- set_iommu_table_base(&pe->pdev->dev, tbl);
- iommu_add_device(&pe->pdev->dev);
- } else if (pe->flags & (PNV_IODA_PE_BUS | PNV_IODA_PE_BUS_ALL))
+ if (pe->flags & (PNV_IODA_PE_BUS | PNV_IODA_PE_BUS_ALL))
pnv_ioda_setup_bus_dma(pe, pe->pbus, true);
return;
--
2.11.0
^ permalink raw reply related
* Re: [PATCH kernel v2] powerpc/ioda/npu: Call skiboot's hot reset hook when disabling NPU2
From: Alexey Kardashevskiy @ 2018-10-16 2:22 UTC (permalink / raw)
To: Alistair Popple; +Cc: Reza Arbab, linuxppc-dev, David Gibson
In-Reply-To: <2184134.UzzIe8q8zq@new-mexico>
On 16/10/2018 13:19, Alistair Popple wrote:
>> reset_ntl() does what npu2_dev_procedure_reset() does plus more stuff,
>> there nothing really in npu2_dev_procedure_reset() which reset_ntl()
>> does not do already from the hardware standpoint. And it did stop HMIs
>> for me though.
>>
>> but ok, what will be sufficient then if not reset_ntl()?
>
> Argh, yes you are correct. Specifically both npu2_dev_procedure_reset() and
> reset_ntl() contain:
>
> /* NTL Reset */
> val = npu2_read(ndev->npu, NPU2_NTL_MISC_CFG1(ndev));
> val |= PPC_BIT(8) | PPC_BIT(9);
> npu2_write(ndev->npu, NPU2_NTL_MISC_CFG1(ndev), val);
>
> Which should fence the brick. However from what I recall there was more to
> reliably preventing HMIs than merely fencing the brick. It invovled a sequence
> of fencing and flushing the cache with dcbf instructions at the right time which
> is why we also have the FLR. Unfortunately I don't know the precise details,
> perhaps if we send enough coffee Balbir's way he might be able remind us?
He suggested and ack'ed that skiboot patch, I can repeat beers^wcoffee
but it won't change much ;)
>
> - Alistair
>
>>
>>
>>>
>>> - Alistair
>>>
>>>>
>>>>
>>>>
>>>>>
>>>>> - Alistair
>>>>>
>>>>> On Monday, 15 October 2018 6:17:51 PM AEDT Alexey Kardashevskiy wrote:
>>>>>> Ping?
>>>>>>
>>>>>>
>>>>>> On 02/10/2018 13:20, Alexey Kardashevskiy wrote:
>>>>>>> The skiboot firmware has a hot reset handler which fences the NVIDIA V100
>>>>>>> GPU RAM on Witherspoons and makes accesses no-op instead of throwing HMIs:
>>>>>>> https://github.com/open-power/skiboot/commit/fca2b2b839a67
>>>>>>>
>>>>>>> Now we are going to pass V100 via VFIO which most certainly involves
>>>>>>> KVM guests which are often terminated without getting a chance to offline
>>>>>>> GPU RAM so we end up with a running machine with misconfigured memory.
>>>>>>> Accessing this memory produces hardware management interrupts (HMI)
>>>>>>> which bring the host down.
>>>>>>>
>>>>>>> To suppress HMIs, this wires up this hot reset hook to vfio_pci_disable()
>>>>>>> via pci_disable_device() which switches NPU2 to a safe mode and prevents
>>>>>>> HMIs.
>>>>>>>
>>>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>>>> ---
>>>>>>> Changes:
>>>>>>> v2:
>>>>>>> * updated the commit log
>>>>>>> ---
>>>>>>> arch/powerpc/platforms/powernv/pci-ioda.c | 10 ++++++++++
>>>>>>> 1 file changed, 10 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>>>>>>> index cde7102..e37b9cc 100644
>>>>>>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
>>>>>>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>>>>>>> @@ -3688,6 +3688,15 @@ static void pnv_pci_release_device(struct pci_dev *pdev)
>>>>>>> pnv_ioda_release_pe(pe);
>>>>>>> }
>>>>>>>
>>>>>>> +static void pnv_npu_disable_device(struct pci_dev *pdev)
>>>>>>> +{
>>>>>>> + struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
>>>>>>> + struct eeh_pe *eehpe = edev ? edev->pe : NULL;
>>>>>>> +
>>>>>>> + if (eehpe && eeh_ops && eeh_ops->reset)
>>>>>>> + eeh_ops->reset(eehpe, EEH_RESET_HOT);
>>>>>>> +}
>>>>>>> +
>>>>>>> static void pnv_pci_ioda_shutdown(struct pci_controller *hose)
>>>>>>> {
>>>>>>> struct pnv_phb *phb = hose->private_data;
>>>>>>> @@ -3732,6 +3741,7 @@ static const struct pci_controller_ops pnv_npu_ioda_controller_ops = {
>>>>>>> .reset_secondary_bus = pnv_pci_reset_secondary_bus,
>>>>>>> .dma_set_mask = pnv_npu_dma_set_mask,
>>>>>>> .shutdown = pnv_pci_ioda_shutdown,
>>>>>>> + .disable_device = pnv_npu_disable_device,
>>>>>>> };
>>>>>>>
>>>>>>> static const struct pci_controller_ops pnv_npu_ocapi_ioda_controller_ops = {
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
--
Alexey
^ permalink raw reply
* Re: linux-next: Tree for Oct 15
From: Stephen Rothwell @ 2018-10-16 2:19 UTC (permalink / raw)
To: Guenter Roeck
Cc: Linux Kernel Mailing List, Mike Rapoport, Linux-Next Mailing List,
Andrew Morton, PowerPC
In-Reply-To: <20181016130216.3a417381@canb.auug.org.au>
[-- Attachment #1: Type: text/plain, Size: 1727 bytes --]
Hi all,
On Tue, 16 Oct 2018 13:02:16 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Reverting fe3d2a45e8079fdd7d4da1ff07f4b40bc3cb499f (and the following 2
> commits) produces a kernel that boots.
Instead of that, I applied this patch on top of linux-next and it boots
and produces a stack trace ...
From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Tue, 16 Oct 2018 13:07:01 +1100
Subject: [PATCH] mm/memblock.c: use dump_stack() instead of WARN_ON_ONCE for
the alignment checks
Using WARN_ON_ONCE too early causes the PowerPC kernel to fail.
Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
mm/memblock.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/mm/memblock.c b/mm/memblock.c
index 5fefc70253ee..f2ef3915a356 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -1298,8 +1298,10 @@ static phys_addr_t __init memblock_alloc_range_nid(phys_addr_t size,
{
phys_addr_t found;
- if (WARN_ON_ONCE(!align))
+ if (!align) {
+ dump_stack();
align = SMP_CACHE_BYTES;
+ }
found = memblock_find_in_range_node(size, align, start, end, nid,
flags);
@@ -1423,8 +1425,10 @@ static void * __init memblock_alloc_internal(
if (WARN_ON_ONCE(slab_is_available()))
return kzalloc_node(size, GFP_NOWAIT, nid);
- if (WARN_ON_ONCE(!align))
+ if (!align) {
+ dump_stack();
align = SMP_CACHE_BYTES;
+ }
if (max_addr > memblock.current_limit)
max_addr = memblock.current_limit;
--
2.18.0
So, patch "memblock: stop using implicit alignment to SMP_CACHE_BYTES"
should *not* remove the 0 -> SMP_CACHE_BYTES update from mm/memblock.c
and just add the dump_stack().
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related
* Re: [PATCH kernel v2] powerpc/ioda/npu: Call skiboot's hot reset hook when disabling NPU2
From: Alistair Popple @ 2018-10-16 2:19 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: Reza Arbab, linuxppc-dev, David Gibson
In-Reply-To: <6b37ac1c-d611-49b1-4a1b-dec96a58c91b@ozlabs.ru>
> reset_ntl() does what npu2_dev_procedure_reset() does plus more stuff,
> there nothing really in npu2_dev_procedure_reset() which reset_ntl()
> does not do already from the hardware standpoint. And it did stop HMIs
> for me though.
>
> but ok, what will be sufficient then if not reset_ntl()?
Argh, yes you are correct. Specifically both npu2_dev_procedure_reset() and
reset_ntl() contain:
/* NTL Reset */
val = npu2_read(ndev->npu, NPU2_NTL_MISC_CFG1(ndev));
val |= PPC_BIT(8) | PPC_BIT(9);
npu2_write(ndev->npu, NPU2_NTL_MISC_CFG1(ndev), val);
Which should fence the brick. However from what I recall there was more to
reliably preventing HMIs than merely fencing the brick. It invovled a sequence
of fencing and flushing the cache with dcbf instructions at the right time which
is why we also have the FLR. Unfortunately I don't know the precise details,
perhaps if we send enough coffee Balbir's way he might be able remind us?
- Alistair
>
>
> >
> > - Alistair
> >
> >>
> >>
> >>
> >>>
> >>> - Alistair
> >>>
> >>> On Monday, 15 October 2018 6:17:51 PM AEDT Alexey Kardashevskiy wrote:
> >>>> Ping?
> >>>>
> >>>>
> >>>> On 02/10/2018 13:20, Alexey Kardashevskiy wrote:
> >>>>> The skiboot firmware has a hot reset handler which fences the NVIDIA V100
> >>>>> GPU RAM on Witherspoons and makes accesses no-op instead of throwing HMIs:
> >>>>> https://github.com/open-power/skiboot/commit/fca2b2b839a67
> >>>>>
> >>>>> Now we are going to pass V100 via VFIO which most certainly involves
> >>>>> KVM guests which are often terminated without getting a chance to offline
> >>>>> GPU RAM so we end up with a running machine with misconfigured memory.
> >>>>> Accessing this memory produces hardware management interrupts (HMI)
> >>>>> which bring the host down.
> >>>>>
> >>>>> To suppress HMIs, this wires up this hot reset hook to vfio_pci_disable()
> >>>>> via pci_disable_device() which switches NPU2 to a safe mode and prevents
> >>>>> HMIs.
> >>>>>
> >>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>>> ---
> >>>>> Changes:
> >>>>> v2:
> >>>>> * updated the commit log
> >>>>> ---
> >>>>> arch/powerpc/platforms/powernv/pci-ioda.c | 10 ++++++++++
> >>>>> 1 file changed, 10 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> >>>>> index cde7102..e37b9cc 100644
> >>>>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> >>>>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> >>>>> @@ -3688,6 +3688,15 @@ static void pnv_pci_release_device(struct pci_dev *pdev)
> >>>>> pnv_ioda_release_pe(pe);
> >>>>> }
> >>>>>
> >>>>> +static void pnv_npu_disable_device(struct pci_dev *pdev)
> >>>>> +{
> >>>>> + struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
> >>>>> + struct eeh_pe *eehpe = edev ? edev->pe : NULL;
> >>>>> +
> >>>>> + if (eehpe && eeh_ops && eeh_ops->reset)
> >>>>> + eeh_ops->reset(eehpe, EEH_RESET_HOT);
> >>>>> +}
> >>>>> +
> >>>>> static void pnv_pci_ioda_shutdown(struct pci_controller *hose)
> >>>>> {
> >>>>> struct pnv_phb *phb = hose->private_data;
> >>>>> @@ -3732,6 +3741,7 @@ static const struct pci_controller_ops pnv_npu_ioda_controller_ops = {
> >>>>> .reset_secondary_bus = pnv_pci_reset_secondary_bus,
> >>>>> .dma_set_mask = pnv_npu_dma_set_mask,
> >>>>> .shutdown = pnv_pci_ioda_shutdown,
> >>>>> + .disable_device = pnv_npu_disable_device,
> >>>>> };
> >>>>>
> >>>>> static const struct pci_controller_ops pnv_npu_ocapi_ioda_controller_ops = {
> >>>>>
> >>>>
> >>>>
> >>>
> >>>
> >>
> >>
> >
> >
>
>
^ permalink raw reply
* Re: [PATCH kernel v2] powerpc/ioda/npu: Call skiboot's hot reset hook when disabling NPU2
From: Alexey Kardashevskiy @ 2018-10-16 2:02 UTC (permalink / raw)
To: Alistair Popple; +Cc: Reza Arbab, linuxppc-dev, David Gibson
In-Reply-To: <1625714.Rr8qeDJm6q@new-mexico>
On 16/10/2018 12:44, Alistair Popple wrote:
> Hi Alexey,
>
> On Tuesday, 16 October 2018 12:37:49 PM AEDT Alexey Kardashevskiy wrote:
>>
>> On 16/10/2018 11:38, Alistair Popple wrote:
>>> Hi Alexey,
>>>
>>> Looking at the skiboot side I think we only fence the NVLink bricks as part of a
>>> PCIe function level reset (FLR) rather than a PCI Hot or Fundamental reset which
>>> I believe is what the code here does. So to fence the bricks you would need to
>>> do either a FLR on the given link or alter Skiboot to fence a given link as part
>>> of a hot reset.
>>
>> The code here calls OPAL to execute this code:
>>
>> https://github.com/open-power/skiboot/commit/fca2b2b839a673a1e52fc6b19ee6d33b2dfbc003
>>
>> This resets all links on an NPU which is fine for now as we pass GPUs in
>> groups only. Or I missed something?
>
> From what I can see in
> https://github.com/open-power/skiboot/commit/2947eaa14e771e572d4e84bf003318c590c1c7d4
> we only fence the bricks in npu2_dev_procedure_reset()
> (https://github.com/open-power/skiboot/blob/master/hw/npu2-hw-procedures.c#L937)
> which itself is only called from the FLR path and not the code path you point
> out above.
>
> The code path above only resets the NTL which I don't believe is sufficient to
> prevent HMIs.
reset_ntl() does what npu2_dev_procedure_reset() does plus more stuff,
there nothing really in npu2_dev_procedure_reset() which reset_ntl()
does not do already from the hardware standpoint. And it did stop HMIs
for me though.
but ok, what will be sufficient then if not reset_ntl()?
>
> - Alistair
>
>>
>>
>>
>>>
>>> - Alistair
>>>
>>> On Monday, 15 October 2018 6:17:51 PM AEDT Alexey Kardashevskiy wrote:
>>>> Ping?
>>>>
>>>>
>>>> On 02/10/2018 13:20, Alexey Kardashevskiy wrote:
>>>>> The skiboot firmware has a hot reset handler which fences the NVIDIA V100
>>>>> GPU RAM on Witherspoons and makes accesses no-op instead of throwing HMIs:
>>>>> https://github.com/open-power/skiboot/commit/fca2b2b839a67
>>>>>
>>>>> Now we are going to pass V100 via VFIO which most certainly involves
>>>>> KVM guests which are often terminated without getting a chance to offline
>>>>> GPU RAM so we end up with a running machine with misconfigured memory.
>>>>> Accessing this memory produces hardware management interrupts (HMI)
>>>>> which bring the host down.
>>>>>
>>>>> To suppress HMIs, this wires up this hot reset hook to vfio_pci_disable()
>>>>> via pci_disable_device() which switches NPU2 to a safe mode and prevents
>>>>> HMIs.
>>>>>
>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>>> ---
>>>>> Changes:
>>>>> v2:
>>>>> * updated the commit log
>>>>> ---
>>>>> arch/powerpc/platforms/powernv/pci-ioda.c | 10 ++++++++++
>>>>> 1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>>>>> index cde7102..e37b9cc 100644
>>>>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
>>>>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>>>>> @@ -3688,6 +3688,15 @@ static void pnv_pci_release_device(struct pci_dev *pdev)
>>>>> pnv_ioda_release_pe(pe);
>>>>> }
>>>>>
>>>>> +static void pnv_npu_disable_device(struct pci_dev *pdev)
>>>>> +{
>>>>> + struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
>>>>> + struct eeh_pe *eehpe = edev ? edev->pe : NULL;
>>>>> +
>>>>> + if (eehpe && eeh_ops && eeh_ops->reset)
>>>>> + eeh_ops->reset(eehpe, EEH_RESET_HOT);
>>>>> +}
>>>>> +
>>>>> static void pnv_pci_ioda_shutdown(struct pci_controller *hose)
>>>>> {
>>>>> struct pnv_phb *phb = hose->private_data;
>>>>> @@ -3732,6 +3741,7 @@ static const struct pci_controller_ops pnv_npu_ioda_controller_ops = {
>>>>> .reset_secondary_bus = pnv_pci_reset_secondary_bus,
>>>>> .dma_set_mask = pnv_npu_dma_set_mask,
>>>>> .shutdown = pnv_pci_ioda_shutdown,
>>>>> + .disable_device = pnv_npu_disable_device,
>>>>> };
>>>>>
>>>>> static const struct pci_controller_ops pnv_npu_ocapi_ioda_controller_ops = {
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
--
Alexey
^ permalink raw reply
* Re: linux-next: Tree for Oct 15
From: Stephen Rothwell @ 2018-10-16 2:02 UTC (permalink / raw)
To: Guenter Roeck
Cc: Linux Kernel Mailing List, Mike Rapoport, Linux-Next Mailing List,
Andrew Morton, PowerPC
In-Reply-To: <20181015222213.GA31415@roeck-us.net>
[-- Attachment #1: Type: text/plain, Size: 7773 bytes --]
Hi all,
On Mon, 15 Oct 2018 15:22:13 -0700 Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Oct 16, 2018 at 07:33:59AM +1100, Stephen Rothwell wrote:
> >
> > On Mon, 15 Oct 2018 12:39:14 -0700 Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On Mon, Oct 15, 2018 at 07:25:46PM +1100, Stephen Rothwell wrote:
> > > > Hi all,
> > > >
> > > > Changes since 20181012:
> > > >
> > > > My qemu boots of a powerpc pseries_le_defconfig kernel failed today.
> > > >
> > >
> > > Bisect log:
> > >
>
> Trying again. Not very useful since some of the steps fail with compile errors.
> The problem does seem to be related to the bootmem changes, though.
OK, I managed to do the bisect:
fe3d2a45e8079fdd7d4da1ff07f4b40bc3cb499f is the first bad commit
commit fe3d2a45e8079fdd7d4da1ff07f4b40bc3cb499f
Author: Mike Rapoport <rppt@linux.vnet.ibm.com>
Date: Sat Oct 13 14:05:51 2018 +1100
memblock: stop using implicit alignment to SMP_CACHE_BYTES
When a memblock allocation APIs are called with align = 0, the alignment
is implicitly set to SMP_CACHE_BYTES.
Implicit alignment is done deep in the memblock allocator and it can
come as a surprise. Not that such an alignment would be wrong even
when used incorrectly but it is better to be explicit for the sake of
clarity and the prinicple of the least surprise.
Replace all such uses of memblock APIs with the 'align' parameter
explicitly set to SMP_CACHE_BYTES and stop implicit alignment assignment
in the memblock internal allocation functions.
For the case when memblock APIs are used via helper functions, e.g. like
iommu_arena_new_node() in Alpha, the helper functions were detected with
Coccinelle's help and then manually examined and updated where
appropriate.
The direct memblock APIs users were updated using the semantic patch below:
@@
expression size, min_addr, max_addr, nid;
@@
(
|
- memblock_alloc_try_nid_raw(size, 0, min_addr, max_addr, nid)
+ memblock_alloc_try_nid_raw(size, SMP_CACHE_BYTES, min_addr, max_addr,
nid)
|
- memblock_alloc_try_nid_nopanic(size, 0, min_addr, max_addr, nid)
+ memblock_alloc_try_nid_nopanic(size, SMP_CACHE_BYTES, min_addr, max_addr,
nid)
|
- memblock_alloc_try_nid(size, 0, min_addr, max_addr, nid)
+ memblock_alloc_try_nid(size, SMP_CACHE_BYTES, min_addr, max_addr, nid)
|
- memblock_alloc(size, 0)
+ memblock_alloc(size, SMP_CACHE_BYTES)
|
- memblock_alloc_raw(size, 0)
+ memblock_alloc_raw(size, SMP_CACHE_BYTES)
|
- memblock_alloc_from(size, 0, min_addr)
+ memblock_alloc_from(size, SMP_CACHE_BYTES, min_addr)
|
- memblock_alloc_nopanic(size, 0)
+ memblock_alloc_nopanic(size, SMP_CACHE_BYTES)
|
- memblock_alloc_low(size, 0)
+ memblock_alloc_low(size, SMP_CACHE_BYTES)
|
- memblock_alloc_low_nopanic(size, 0)
+ memblock_alloc_low_nopanic(size, SMP_CACHE_BYTES)
|
- memblock_alloc_from_nopanic(size, 0, min_addr)
+ memblock_alloc_from_nopanic(size, SMP_CACHE_BYTES, min_addr)
|
- memblock_alloc_node(size, 0, nid)
+ memblock_alloc_node(size, SMP_CACHE_BYTES, nid)
)
[mhocko@suse.com: changelog update]
Link: http://lkml.kernel.org/r/1538687224-17535-1-git-send-email-rppt@linux.vnet.ibm.com
Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
Suggested-by: Michal Hocko <mhocko@suse.com>
Acked-by: Paul Burton <paul.burton@mips.com> [MIPS]
Acked-by: Michael Ellerman <mpe@ellerman.id.au> [powerpc]
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Guan Xuetao <gxt@pku.edu.cn>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Matt Turner <mattst88@gmail.com>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Richard Weinberger <richard@nod.at>
Cc: Russell King <linux@armlinux.org.uk>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
:040000 040000 9fdad987cc69ffad8de2d0d621facc66b096aac1 c9959a9199e01f282d1d291a6280b203a8561e7a M arch
:040000 040000 de303e4a0ad2b500de3fca2f65c8943c12c0b077 1581aed934cfb96b2706df9bfa7746edfadffea6 M drivers
:040000 040000 f9179d8189f08e3575031a76181a64eedd148db5 293bc953dcfc2b718f5c7cdd58722284f4393dec M include
:040000 040000 3019b5f917a20420537ac5cf4dc90b3c7f1aa56f 1b88f0791341d17abd259d7a8d2e0cc55147d8f6 M init
:040000 040000 8819d52ce4e6463fc95a2e841baeeddbc9fb8c52 dc167f38bd8c4a80e0ac84ecc981e95b98703393 M kernel
:040000 040000 30c64583e66fc20181d7ce2b6ced9d7e060e1042 4e57b31864cef4e921bbd73150f63637a819e3c4 M lib
:040000 040000 1b341ab0dd034f0fef37c234a771419924e0ecc9 9c489ba60978950733bf678191833fdb9689bdab M mm
# bad: [ca0591d03a2d373e0019ad357fbbee69c8272381] Add linux-next specific files for 20181015
# good: [3a27203102ebfa67bd0bced05b1def499bb59db2] Merge tag 'libnvdimm-fixes-4.19-rc8' of git://git.kernel.org/pub/scm/linux/kernel/git/nvdimm/nvdimm
git bisect start 'HEAD' 'remotes/origin/stable'
# good: [5a09f4fbf98e45de8341bc8176e62afaac8405f6] Merge remote-tracking branch 'crypto/master'
git bisect good 5a09f4fbf98e45de8341bc8176e62afaac8405f6
# good: [a54eefdf4d208f6904da9e836ff32d7dde0c9516] Merge remote-tracking branch 'tip/auto-latest'
git bisect good a54eefdf4d208f6904da9e836ff32d7dde0c9516
# good: [d71e0d25be750d02a3d04500aeb151bb94465811] Merge remote-tracking branch 'staging/staging-next'
git bisect good d71e0d25be750d02a3d04500aeb151bb94465811
# good: [7961c8ea9d81f927a78e30bb7c194310ed6b7c1d] Merge remote-tracking branch 'pinctrl/for-next'
git bisect good 7961c8ea9d81f927a78e30bb7c194310ed6b7c1d
# good: [c3d392e6aae57d54fdc683f7432c3e248602bebb] Merge remote-tracking branch 'xarray/xarray'
git bisect good c3d392e6aae57d54fdc683f7432c3e248602bebb
# good: [880c1034475c873963d6250eb95ebbbf5604a281] userfaultfd: selftest: cleanup help messages
git bisect good 880c1034475c873963d6250eb95ebbbf5604a281
# good: [9f1fa0ab60f7b09d335bbaf33db9116241059708] reiserfs: propagate errors from fill_with_dentries() properly
git bisect good 9f1fa0ab60f7b09d335bbaf33db9116241059708
# good: [596046ffd571f32fa3d3e7ffdf7861b71a258552] memblock: replace alloc_bootmem_low with memblock_alloc_low (2)
git bisect good 596046ffd571f32fa3d3e7ffdf7861b71a258552
# good: [979961b3058df1a6d24ab423dd6fa6f20982f591] memblock: replace BOOTMEM_ALLOC_* with MEMBLOCK variants
git bisect good 979961b3058df1a6d24ab423dd6fa6f20982f591
# bad: [8fab7180e6cd3126f7be81e8e33c8152b99f046b] mm/memory_hotplug: make add_memory() take the device_hotplug_lock
git bisect bad 8fab7180e6cd3126f7be81e8e33c8152b99f046b
# bad: [fe3d2a45e8079fdd7d4da1ff07f4b40bc3cb499f] memblock: stop using implicit alignment to SMP_CACHE_BYTES
git bisect bad fe3d2a45e8079fdd7d4da1ff07f4b40bc3cb499f
# good: [2b130e2ae5fdc482305c6d23b05a532addda7c40] powerpc: fix up for removal of linux/bootmem.h
git bisect good 2b130e2ae5fdc482305c6d23b05a532addda7c40
# good: [699f0940861c958f2397c727d80fe324b6d88b06] docs/boot-time-mm: remove bootmem documentation
git bisect good 699f0940861c958f2397c727d80fe324b6d88b06
# first bad commit: [fe3d2a45e8079fdd7d4da1ff07f4b40bc3cb499f] memblock: stop using implicit alignment to SMP_CACHE_BYTES
Reverting fe3d2a45e8079fdd7d4da1ff07f4b40bc3cb499f (and the following 2
commits) produces a kernel that boots.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH kernel v2] powerpc/ioda/npu: Call skiboot's hot reset hook when disabling NPU2
From: Alistair Popple @ 2018-10-16 1:44 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: Reza Arbab, linuxppc-dev, David Gibson
In-Reply-To: <c4954261-ed8f-06e9-b32b-7710c50ee9d5@ozlabs.ru>
Hi Alexey,
On Tuesday, 16 October 2018 12:37:49 PM AEDT Alexey Kardashevskiy wrote:
>
> On 16/10/2018 11:38, Alistair Popple wrote:
> > Hi Alexey,
> >
> > Looking at the skiboot side I think we only fence the NVLink bricks as part of a
> > PCIe function level reset (FLR) rather than a PCI Hot or Fundamental reset which
> > I believe is what the code here does. So to fence the bricks you would need to
> > do either a FLR on the given link or alter Skiboot to fence a given link as part
> > of a hot reset.
>
> The code here calls OPAL to execute this code:
>
> https://github.com/open-power/skiboot/commit/fca2b2b839a673a1e52fc6b19ee6d33b2dfbc003
>
> This resets all links on an NPU which is fine for now as we pass GPUs in
> groups only. Or I missed something?
From what I can see in
https://github.com/open-power/skiboot/commit/2947eaa14e771e572d4e84bf003318c590c1c7d4
we only fence the bricks in npu2_dev_procedure_reset()
(https://github.com/open-power/skiboot/blob/master/hw/npu2-hw-procedures.c#L937)
which itself is only called from the FLR path and not the code path you point
out above.
The code path above only resets the NTL which I don't believe is sufficient to
prevent HMIs.
- Alistair
>
>
>
> >
> > - Alistair
> >
> > On Monday, 15 October 2018 6:17:51 PM AEDT Alexey Kardashevskiy wrote:
> >> Ping?
> >>
> >>
> >> On 02/10/2018 13:20, Alexey Kardashevskiy wrote:
> >>> The skiboot firmware has a hot reset handler which fences the NVIDIA V100
> >>> GPU RAM on Witherspoons and makes accesses no-op instead of throwing HMIs:
> >>> https://github.com/open-power/skiboot/commit/fca2b2b839a67
> >>>
> >>> Now we are going to pass V100 via VFIO which most certainly involves
> >>> KVM guests which are often terminated without getting a chance to offline
> >>> GPU RAM so we end up with a running machine with misconfigured memory.
> >>> Accessing this memory produces hardware management interrupts (HMI)
> >>> which bring the host down.
> >>>
> >>> To suppress HMIs, this wires up this hot reset hook to vfio_pci_disable()
> >>> via pci_disable_device() which switches NPU2 to a safe mode and prevents
> >>> HMIs.
> >>>
> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>> ---
> >>> Changes:
> >>> v2:
> >>> * updated the commit log
> >>> ---
> >>> arch/powerpc/platforms/powernv/pci-ioda.c | 10 ++++++++++
> >>> 1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
> >>> index cde7102..e37b9cc 100644
> >>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
> >>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
> >>> @@ -3688,6 +3688,15 @@ static void pnv_pci_release_device(struct pci_dev *pdev)
> >>> pnv_ioda_release_pe(pe);
> >>> }
> >>>
> >>> +static void pnv_npu_disable_device(struct pci_dev *pdev)
> >>> +{
> >>> + struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
> >>> + struct eeh_pe *eehpe = edev ? edev->pe : NULL;
> >>> +
> >>> + if (eehpe && eeh_ops && eeh_ops->reset)
> >>> + eeh_ops->reset(eehpe, EEH_RESET_HOT);
> >>> +}
> >>> +
> >>> static void pnv_pci_ioda_shutdown(struct pci_controller *hose)
> >>> {
> >>> struct pnv_phb *phb = hose->private_data;
> >>> @@ -3732,6 +3741,7 @@ static const struct pci_controller_ops pnv_npu_ioda_controller_ops = {
> >>> .reset_secondary_bus = pnv_pci_reset_secondary_bus,
> >>> .dma_set_mask = pnv_npu_dma_set_mask,
> >>> .shutdown = pnv_pci_ioda_shutdown,
> >>> + .disable_device = pnv_npu_disable_device,
> >>> };
> >>>
> >>> static const struct pci_controller_ops pnv_npu_ocapi_ioda_controller_ops = {
> >>>
> >>
> >>
> >
> >
>
>
^ permalink raw reply
* Re: [PATCH kernel v2] powerpc/ioda/npu: Call skiboot's hot reset hook when disabling NPU2
From: Alexey Kardashevskiy @ 2018-10-16 1:37 UTC (permalink / raw)
To: Alistair Popple; +Cc: Reza Arbab, linuxppc-dev, David Gibson
In-Reply-To: <2435624.zC36bzMth8@new-mexico>
On 16/10/2018 11:38, Alistair Popple wrote:
> Hi Alexey,
>
> Looking at the skiboot side I think we only fence the NVLink bricks as part of a
> PCIe function level reset (FLR) rather than a PCI Hot or Fundamental reset which
> I believe is what the code here does. So to fence the bricks you would need to
> do either a FLR on the given link or alter Skiboot to fence a given link as part
> of a hot reset.
The code here calls OPAL to execute this code:
https://github.com/open-power/skiboot/commit/fca2b2b839a673a1e52fc6b19ee6d33b2dfbc003
This resets all links on an NPU which is fine for now as we pass GPUs in
groups only. Or I missed something?
>
> - Alistair
>
> On Monday, 15 October 2018 6:17:51 PM AEDT Alexey Kardashevskiy wrote:
>> Ping?
>>
>>
>> On 02/10/2018 13:20, Alexey Kardashevskiy wrote:
>>> The skiboot firmware has a hot reset handler which fences the NVIDIA V100
>>> GPU RAM on Witherspoons and makes accesses no-op instead of throwing HMIs:
>>> https://github.com/open-power/skiboot/commit/fca2b2b839a67
>>>
>>> Now we are going to pass V100 via VFIO which most certainly involves
>>> KVM guests which are often terminated without getting a chance to offline
>>> GPU RAM so we end up with a running machine with misconfigured memory.
>>> Accessing this memory produces hardware management interrupts (HMI)
>>> which bring the host down.
>>>
>>> To suppress HMIs, this wires up this hot reset hook to vfio_pci_disable()
>>> via pci_disable_device() which switches NPU2 to a safe mode and prevents
>>> HMIs.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> Changes:
>>> v2:
>>> * updated the commit log
>>> ---
>>> arch/powerpc/platforms/powernv/pci-ioda.c | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c b/arch/powerpc/platforms/powernv/pci-ioda.c
>>> index cde7102..e37b9cc 100644
>>> --- a/arch/powerpc/platforms/powernv/pci-ioda.c
>>> +++ b/arch/powerpc/platforms/powernv/pci-ioda.c
>>> @@ -3688,6 +3688,15 @@ static void pnv_pci_release_device(struct pci_dev *pdev)
>>> pnv_ioda_release_pe(pe);
>>> }
>>>
>>> +static void pnv_npu_disable_device(struct pci_dev *pdev)
>>> +{
>>> + struct eeh_dev *edev = pci_dev_to_eeh_dev(pdev);
>>> + struct eeh_pe *eehpe = edev ? edev->pe : NULL;
>>> +
>>> + if (eehpe && eeh_ops && eeh_ops->reset)
>>> + eeh_ops->reset(eehpe, EEH_RESET_HOT);
>>> +}
>>> +
>>> static void pnv_pci_ioda_shutdown(struct pci_controller *hose)
>>> {
>>> struct pnv_phb *phb = hose->private_data;
>>> @@ -3732,6 +3741,7 @@ static const struct pci_controller_ops pnv_npu_ioda_controller_ops = {
>>> .reset_secondary_bus = pnv_pci_reset_secondary_bus,
>>> .dma_set_mask = pnv_npu_dma_set_mask,
>>> .shutdown = pnv_pci_ioda_shutdown,
>>> + .disable_device = pnv_npu_disable_device,
>>> };
>>>
>>> static const struct pci_controller_ops pnv_npu_ocapi_ioda_controller_ops = {
>>>
>>
>>
>
>
--
Alexey
^ permalink raw reply
* Re: linux-next: Tree for Oct 15
From: David Miller @ 2018-10-16 1:34 UTC (permalink / raw)
To: robherring2; +Cc: sfr, linux-kernel, linux-next, linuxppc-dev, linux
In-Reply-To: <CAL_Jsq+t2ZXhtzN0KnnU1jn+JZE3Spfgm8jT60jx8j8gEEKjFQ@mail.gmail.com>
From: Rob Herring <robherring2@gmail.com>
Date: Mon, 15 Oct 2018 20:00:24 -0500
> David, Can you revert commit 0b9871a3a8cc. I'll have to find another approach.
Ok.
^ permalink raw reply
* Re: [PATCH] powerpc/traps: restore recoverability of machine_check interrupts
From: Nicholas Piggin @ 2018-10-16 1:10 UTC (permalink / raw)
To: Christophe LEROY
Cc: linux-kernel, stable@vger.kernel.org, Paul Mackerras,
linuxppc-dev
In-Reply-To: <ab520cc1-11b7-ddc6-2393-8f1315db4db3@c-s.fr>
On Mon, 15 Oct 2018 09:40:50 +0200
Christophe LEROY <christophe.leroy@c-s.fr> wrote:
> Cc: stable@vger.kernel.org <stable@vger.kernel.org>
>
> Le 13/10/2018 à 11:16, Christophe Leroy a écrit :
> > commit b96672dd840f ("powerpc: Machine check interrupt is a non-
> > maskable interrupt") added a call to nmi_enter() at the beginning of
> > machine check restart exception handler. Due to that, in_interrupt()
> > always returns true regardless of the state before entering the
> > exception, and die() panics even when the system was not already in
> > interrupt.
> >
> > This patch calls nmi_exit() before calling die() in order to restore
> > the interrupt state we had before calling nmi_enter()
> >
> > Fixes: b96672dd840f ("powerpc: Machine check interrupt is a non-maskable interrupt")
> > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
This looks good to me and probably the simplest fix.
powernv will need a fix on top of this, to remove the die and let it
just fall through unrecovered to the traps.c code. pseries seems to
be okay. But this patch looks good.
Thanks,
Nick
^ permalink raw reply
* Re: linux-next: Tree for Oct 15
From: Rob Herring @ 2018-10-16 1:00 UTC (permalink / raw)
To: Stephen Rothwell, David Miller
Cc: linux-kernel@vger.kernel.org, linux-next, linuxppc-dev,
Guenter Roeck
In-Reply-To: <CAL_Jsq+DAEEa6i9xb-9MMSxu-znc9tZLZwFRADqE+kcxQYXrTg@mail.gmail.com>
+davem
On Mon, Oct 15, 2018 at 3:35 PM Rob Herring <robherring2@gmail.com> wrote:
>
> On Mon, Oct 15, 2018 at 3:12 PM Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > Hi Guenter,
> >
> > [Just cc'ing the PPC and devicetree folks]
> >
> > On Mon, 15 Oct 2018 11:26:37 -0700 Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On Mon, Oct 15, 2018 at 07:25:46PM +1100, Stephen Rothwell wrote:
> > > >
> > > > My qemu boots of a powerpc pseries_le_defconfig kernel failed today.
> > >
> > > Same here. Interestingly, this only affects little endian pseries
> > > boots; big endian works fine. I'll try to bisect later.
> > >
> > > ALl ppc qemu tests (including big endian pseries) also generate a warning.
> > >
> > > WARNING: CPU: 0 PID: 0 at mm/memblock.c:1301 .memblock_alloc_range_nid+0x20/0x68
> > > Modules linked in:
> > > CPU: 0 PID: 0 Comm: swapper Not tainted 4.19.0-rc7-next-20181015 #1
> > > NIP: c000000000f99198 LR: c000000000f99490 CTR: c000000000bb8364
> > > REGS: c000000001217a78 TRAP: 0700 Not tainted (4.19.0-rc7-next-20181015)
> > > MSR: 0000000080021000 <CE,ME> CR: 24000422 XER: 20000000
> > > IRQMASK: 1
> > > GPR00: c000000000f99490 c000000001217d00 c00000000121a500 00000000000000c0
> > > GPR04: 0000000000000000 0000000000000000 0000000000000000 ffffffffffffffff
> > > GPR08: 0000000000000000 00000000000000c0 0000000000000018 00000000000000b7
> > > GPR12: 0000000000000040 c000000000fe7840 0000000000000000 0000000000000000
> > > GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > > GPR20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > > GPR24: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > > GPR28: c000000000000304 c000000001262088 00000000000000c0 c000000000fea500
> > > NIP [c000000000f99198] .memblock_alloc_range_nid+0x20/0x68
> > > LR [c000000000f99490] .memblock_alloc_base+0x18/0x48
> > > Call Trace:
> > > [c000000001217d00] [c000000002a00000] 0xc000000002a00000 (unreliable)
> > > [c000000001217d80] [c000000000f99490] .memblock_alloc_base+0x18/0x48
> > > [c000000001217df0] [c000000000f7a274] .allocate_paca_ptrs+0x3c/0x74
> > > [c000000001217e70] [c000000000f78bf0] .early_init_devtree+0x288/0x320
> > > [c000000001217f10] [c000000000f79b6c] .early_setup+0x80/0x130
> > > [c000000001217f90] [c000000000000528] start_here_multiplatform+0x68/0x80
> > >
> > >
> > > sparc images crash, starting with next-20181009. Bisect with
> > > next-201810112 points to the merge of devicetree/for-next, though
> > > devicetree/for-next itself does not have the problem (bisect log
> > > attached below). The crash is in devicetree code.
> > >
> > > Crash logs:
> > > https://kerneltests.org/builders/qemu-sparc64-next/builds/981/steps/qemubuildcommand_1/logs/stdio
> > > https://kerneltests.org/builders/qemu-sparc-next/builds/975/steps/qemubuildcommand_1/logs/stdio
>
> The sparc crash appears to be related to changes I made. Looking into it.
The problem is a combination of commit 0b9871a3a8cc ("sparc: Convert
to using %pOFn instead of device_node.name") and commit 6d0a70a284be
("vsprintf: print OF node name using full_name"). The Sparc functions
in prom_*.c can't use %pOFn to ultimately construct full_name as %pOFn
is derived from full_name. Reverting the former commit gets QEMU
booting again.
David, Can you revert commit 0b9871a3a8cc. I'll have to find another approach.
Rob
^ permalink raw reply
* Re: [PATCH v06 3/5] migration/memory: Add hotplug READD_MULTIPLE
From: Michael Ellerman @ 2018-10-16 0:39 UTC (permalink / raw)
To: Michael Bringmann, linuxppc-dev
Cc: Nathan Fontenot, Michael Bringmann, Juliet Kim, Thomas Falcon,
Tyrel Datwyler
In-Reply-To: <f38687fe-7d46-830b-e4bb-ef78a7f99a65@linux.vnet.ibm.com>
Michael Bringmann <mwb@linux.vnet.ibm.com> writes:
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index 2b796da..9c76345 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -541,6 +549,23 @@ static int dlpar_memory_readd_by_index(u32 drc_index)
> return rc;
> }
>
> +static int dlpar_memory_readd_multiple(void)
> +{
> + struct drmem_lmb *lmb;
> + int rc;
> +
> + pr_info("Attempting to update multiple LMBs\n");
> +
> + for_each_drmem_lmb(lmb) {
> + if (drmem_lmb_update(lmb)) {
> + rc = dlpar_memory_readd_helper(lmb);
> + drmem_remove_lmb_update(lmb);
> + }
> + }
> +
> + return rc;
> +}
This leaves rc potentially uninitialised.
What should the result be in that case, -EINVAL ?
cheers
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox