[PATCH v9 00/12] Device property improvements, add %pfw format specifier

[PATCH v9 00/12] Device property improvements, add %pfw format specifier

[Sakari Ailus]

Hi all,

This set adds functionality into the device property API (counting a
node's parents as well as obtaining its name) in order to support printing
fwnode names using a new conversion specifier "%pfw". The names that are
produced are equivalent to its OF counterpart "%pOF" on OF systems for the
two supported modifiers ("f" and "P").

Printing a node's name is something that's been available on OF for a long
time and if something is converted to device property API (such as the
V4L2 fwnode framework) it always got removed of a nice feature that was
sometimes essential in debugging. With this set, that no longer is the
case.

Note: the set now depends on commit b295c3e39c13 ("tools lib traceevent:
Convert remaining %p[fF] users to %p[sS]") and commit 2d44d165e939 ("scsi:
lpfc: Convert existing %pf users to %ps") which both are in v5.4-rc1.

Note 2: There's no full v8 set as I sent a patch fixing a single issue but
there turned out to be a little more to fix, making this one v9.

since v7:

- Reword warning on re-use of %pf and %pF.

- Remove %pf and %pF support from vbin_printf() as well.

- Removed the first patch (now commit b295c3e39c13 ("tools lib traceevent:
  Convert remaining %p[fF] users to %p[sS]")) from the set as it was
  already merged through Arnoldo Carvalho de Melo's tree. Added
  Depends-on: tag to the same patch.

since v6:

- Added a patch for a warning note on re-using obsolete %pf oand %pF
  extensions.

- Rework %pfw validatition in checkpatch.pl according to Joe's comments.

- Unwrap a line wrapped by a previous versions in get_bprint_format().

- Handle %pf and %pF modifiers again in make_bprint_args() in case they're
  no followed by 'w'.

since v5:

- Added a patch to convert %pf to %ps in tools/lib/traceevent.c (first in
  the set).

- Fix ReST syntax in Documentation/core-api/printk-formats.rst.

- Fix returning root swnode name in patch "device property: Add
  fwnode_get_name for returning the name of a node". Use to_swnode()
  directly as well in the same patch.

- Tests: take root node name into account, use direct indices and remove
  the comma from the guardian entry.

- Add a comment on how fwnode_full_name_string() enumerates the nodes.

- Fix error string in fwnode_string().

- Move 'f' + default case as last in the switch in fwnode_string().

- Fix %pfw validation in checkpatch.pl.

since v4:

- Improved documentation for fwnode_get_nth_parent().

- Removed comma from the guardian entry in fwnode_pointer() testcase.

since v3:

- Remove underscores in argument name of fwnode_count_parents().

- Re-introduce "%pO?" error string.

- Unwrap a call to string() in fwnode_string().

- Removed a useless Depends-on: on a patch that was merged long ago.

- Unwrap a Fixes: line.

- Added a patch to move fwnode_get_parent() up to make the review of the
  following patch easier.

since v2:

- Better comments in acpi_fwnode_get_name_prefix().

- Added swnode implementation.

- Fixed swnode refcounting in get_parent() ("swnode: Get reference to
  parent swnode in get_parent op")

- Make argument to to_software_node() const (a new patch)

- Factored out confusingly named kobject_string() that had a single
  caller.

- Cleaner fwnode_count_parents() implementation (as discussed in review).

- Made fwnode_count_parents() argument const.

- Added tests (last patch in the set).

since v1:

- Add patch to remove support for %pf and %pF (depends on another patch
  removing all use of %pf and %pF) (now 4th patch)

- Fix kerneldoc argument documentation for fwnode_get_name (2nd patch)

- Align kerneldoc style with the rest of drivers/base/property.c (no extra
  newline after function name)

- Make checkpatch.pl complain about "%pf" not followed by "w" (6th patch)

- WARN_ONCE() on use of invalid conversion specifiers ("%pf" not followed
  by "w")

Sakari Ailus (12):
  software node: Get reference to parent swnode in get_parent op
  software node: Make argument to to_software_node const
  device property: Move fwnode_get_parent() up
  device property: Add functions for accessing node's parents
  device property: Add fwnode_get_name for returning the name of a node
  device property: Add a function to obtain a node's prefix
  lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps
  lib/vsprintf: Add a note on re-using %pf or %pF
  lib/vsprintf: Make use of fwnode API to obtain node names and
    separators
  lib/vsprintf: OF nodes are first and foremost, struct device_nodes
  lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
  lib/test_printf: Add tests for %pfw printk modifier

 Documentation/core-api/printk-formats.rst | 34 ++++++---
 drivers/acpi/property.c                   | 48 ++++++++++++
 drivers/base/property.c                   | 83 ++++++++++++++++++--
 drivers/base/swnode.c                     | 43 ++++++++++-
 drivers/of/property.c                     | 16 ++++
 include/linux/fwnode.h                    |  4 +
 include/linux/property.h                  |  8 +-
 lib/test_printf.c                         | 32 ++++++++
 lib/vsprintf.c                            | 93 ++++++++++++++---------
 scripts/checkpatch.pl                     |  9 ++-
 10 files changed, 309 insertions(+), 61 deletions(-)

-- 
2.20.1

[PATCH v9 01/12] software node: Get reference to parent swnode in get_parent op

[Sakari Ailus]

The software_node_get_parent() returned a pointer to the parent swnode,
but did not take a reference to it, leading the caller to put a reference
that was not taken. Take that reference now.

Fixes: 59abd83672f7 ("drivers: base: Introducing software nodes to the firmware node framework")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/swnode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index e7b3aa3bd55ad..a7cb41812cfda 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -520,7 +520,10 @@ software_node_get_parent(const struct fwnode_handle *fwnode)
 {
 	struct swnode *swnode = to_swnode(fwnode);
 
-	return swnode ? (swnode->parent ? &swnode->parent->fwnode : NULL) : NULL;
+	if (!swnode || !swnode->parent)
+		return NULL;
+
+	return fwnode_handle_get(&swnode->parent->fwnode);
 }
 
 static struct fwnode_handle *
-- 
2.20.1

[PATCH v9 02/12] software node: Make argument to to_software_node const

[Sakari Ailus]

to_software_node() does not need to modify the fwnode_handle it operates
on; therefore make it const. This allows passing a const fwnode_handle to
to_software_node().

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/swnode.c    | 4 ++--
 include/linux/property.h | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index a7cb41812cfda..951e7efd47c23 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -71,9 +71,9 @@ software_node_to_swnode(const struct software_node *node)
 	return swnode;
 }
 
-const struct software_node *to_software_node(struct fwnode_handle *fwnode)
+const struct software_node *to_software_node(const struct fwnode_handle *fwnode)
 {
-	struct swnode *swnode = to_swnode(fwnode);
+	const struct swnode *swnode = to_swnode(fwnode);
 
 	return swnode ? swnode->node : NULL;
 }
diff --git a/include/linux/property.h b/include/linux/property.h
index 5a910ad795910..421c76e53708d 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -418,7 +418,8 @@ struct software_node {
 };
 
 bool is_software_node(const struct fwnode_handle *fwnode);
-const struct software_node *to_software_node(struct fwnode_handle *fwnode);
+const struct software_node *
+to_software_node(const struct fwnode_handle *fwnode);
 struct fwnode_handle *software_node_fwnode(const struct software_node *node);
 
 int software_node_register_nodes(const struct software_node *nodes);
-- 
2.20.1

[PATCH v9 03/12] device property: Move fwnode_get_parent() up

[Sakari Ailus]

Move fwnode_get_parent() above fwnode_get_next_parent(), making the order
the same as in the header file.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/property.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 81bd01ed40427..3d9dffbe96378 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -556,6 +556,19 @@ int device_add_properties(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(device_add_properties);
 
+/**
+ * fwnode_get_parent - Return parent firwmare node
+ * @fwnode: Firmware whose parent is retrieved
+ *
+ * Return parent firmware node of the given node if possible or %NULL if no
+ * parent was available.
+ */
+struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode)
+{
+	return fwnode_call_ptr_op(fwnode, get_parent);
+}
+EXPORT_SYMBOL_GPL(fwnode_get_parent);
+
 /**
  * fwnode_get_next_parent - Iterate to the node's parent
  * @fwnode: Firmware whose parent is retrieved
@@ -577,19 +590,6 @@ struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL_GPL(fwnode_get_next_parent);
 
-/**
- * fwnode_get_parent - Return parent firwmare node
- * @fwnode: Firmware whose parent is retrieved
- *
- * Return parent firmware node of the given node if possible or %NULL if no
- * parent was available.
- */
-struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode)
-{
-	return fwnode_call_ptr_op(fwnode, get_parent);
-}
-EXPORT_SYMBOL_GPL(fwnode_get_parent);
-
 /**
  * fwnode_get_next_child_node - Return the next child node handle for a node
  * @fwnode: Firmware node to find the next child node for.
-- 
2.20.1

[PATCH v9 04/12] device property: Add functions for accessing node's parents

[Sakari Ailus]

Add two convenience functions for accessing node's parents:

fwnode_count_parents() returns the number of parent nodes a given node
has. fwnode_get_nth_parent() returns node's parent at a given distance
from the node itself.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/property.c  | 46 ++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  3 +++
 2 files changed, 49 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 3d9dffbe96378..f2e555e68b56a 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -590,6 +590,52 @@ struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL_GPL(fwnode_get_next_parent);
 
+/**
+ * fwnode_count_parents - Return the number of parents a node has
+ * @fwnode: The node the parents of which are to be counted
+ *
+ * Returns the number of parents a node has.
+ */
+unsigned int fwnode_count_parents(const struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *__fwnode;
+	unsigned int count;
+
+	__fwnode = fwnode_get_parent(fwnode);
+
+	for (count = 0; __fwnode; count++)
+		__fwnode = fwnode_get_next_parent(__fwnode);
+
+	return count;
+}
+EXPORT_SYMBOL_GPL(fwnode_count_parents);
+
+/**
+ * fwnode_get_nth_parent - Return an nth parent of a node
+ * @fwnode: The node the parent of which is requested
+ * @depth: Distance of the parent from the node
+ *
+ * Returns the nth parent of a node. If there is no parent at the requested
+ * @depth, %NULL is returned. If @depth is 0, the functionality is equivalent to
+ * fwnode_handle_get(). For @depth == 1, it is fwnode_get_parent() and so on.
+ *
+ * The caller is responsible for calling fwnode_handle_put() for the returned
+ * node.
+ */
+struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwnode,
+					    unsigned int depth)
+{
+	unsigned int i;
+
+	fwnode_handle_get(fwnode);
+
+	for (i = 0; i < depth && fwnode; i++)
+		fwnode = fwnode_get_next_parent(fwnode);
+
+	return fwnode;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_nth_parent);
+
 /**
  * fwnode_get_next_child_node - Return the next child node handle for a node
  * @fwnode: Firmware node to find the next child node for.
diff --git a/include/linux/property.h b/include/linux/property.h
index 421c76e53708d..5450e7ec219ac 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -83,6 +83,9 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode,
 struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_next_parent(
 	struct fwnode_handle *fwnode);
+unsigned int fwnode_count_parents(const struct fwnode_handle *fwn);
+struct fwnode_handle *fwnode_get_nth_parent(struct fwnode_handle *fwn,
+					    unsigned int depth);
 struct fwnode_handle *fwnode_get_next_child_node(
 	const struct fwnode_handle *fwnode, struct fwnode_handle *child);
 struct fwnode_handle *fwnode_get_next_available_child_node(
-- 
2.20.1

[PATCH v9 05/12] device property: Add fwnode_get_name for returning the name of a node

[Sakari Ailus]

The fwnode framework did not have means to obtain the name of a node. Add
that now, in form of the fwnode_get_name() function and a corresponding
get_name fwnode op. OF and ACPI support is included.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Rob Herring <robh@kernel.org> (for OF)
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/property.c  | 26 ++++++++++++++++++++++++++
 drivers/base/property.c  | 11 +++++++++++
 drivers/base/swnode.c    | 12 ++++++++++++
 drivers/of/property.c    |  6 ++++++
 include/linux/fwnode.h   |  2 ++
 include/linux/property.h |  1 +
 6 files changed, 58 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index ea3d700da3ca6..5a9397a390f41 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1311,6 +1311,31 @@ acpi_fwnode_get_reference_args(const struct fwnode_handle *fwnode,
 						  args_count, args);
 }
 
+static const char *acpi_fwnode_get_name(const struct fwnode_handle *fwnode)
+{
+	const struct acpi_device *adev;
+	struct fwnode_handle *parent;
+
+	/* Is this the root node? */
+	parent = fwnode_get_parent(fwnode);
+	if (!parent)
+		return "\\";
+
+	fwnode_handle_put(parent);
+
+	if (is_acpi_data_node(fwnode)) {
+		const struct acpi_data_node *dn = to_acpi_data_node(fwnode);
+
+		return dn->name;
+	}
+
+	adev = to_acpi_device_node(fwnode);
+	if (WARN_ON(!adev))
+		return NULL;
+
+	return acpi_device_bid(adev);
+}
+
 static struct fwnode_handle *
 acpi_fwnode_get_parent(struct fwnode_handle *fwnode)
 {
@@ -1351,6 +1376,7 @@ acpi_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
 		.get_parent = acpi_node_get_parent,			\
 		.get_next_child_node = acpi_get_next_subnode,		\
 		.get_named_child_node = acpi_fwnode_get_named_child_node, \
+		.get_name = acpi_fwnode_get_name,			\
 		.get_reference_args = acpi_fwnode_get_reference_args,	\
 		.graph_get_next_endpoint =				\
 			acpi_graph_get_next_endpoint,			\
diff --git a/drivers/base/property.c b/drivers/base/property.c
index f2e555e68b56a..9b5ec88e72d8b 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -556,6 +556,17 @@ int device_add_properties(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(device_add_properties);
 
+/**
+ * fwnode_get_name - Return the name of a node
+ * @fwnode: The firmware node
+ *
+ * Returns a pointer to the node name.
+ */
+const char *fwnode_get_name(const struct fwnode_handle *fwnode)
+{
+	return fwnode_call_ptr_op(fwnode, get_name);
+}
+
 /**
  * fwnode_get_parent - Return parent firwmare node
  * @fwnode: Firmware whose parent is retrieved
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 951e7efd47c23..ccbc3d9d6c5f5 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -515,6 +515,17 @@ static int software_node_read_string_array(const struct fwnode_handle *fwnode,
 						propname, val, nval);
 }
 
+static const char *
+software_node_get_name(const struct fwnode_handle *fwnode)
+{
+	const struct swnode *swnode = to_swnode(fwnode);
+
+	if (!swnode)
+		return "(null)";
+
+	return kobject_name(&swnode->kobj);
+}
+
 static struct fwnode_handle *
 software_node_get_parent(const struct fwnode_handle *fwnode)
 {
@@ -615,6 +626,7 @@ static const struct fwnode_operations software_node_ops = {
 	.property_present = software_node_property_present,
 	.property_read_int_array = software_node_read_int_array,
 	.property_read_string_array = software_node_read_string_array,
+	.get_name = software_node_get_name,
 	.get_parent = software_node_get_parent,
 	.get_next_child_node = software_node_get_next_child,
 	.get_named_child_node = software_node_get_named_child_node,
diff --git a/drivers/of/property.c b/drivers/of/property.c
index d7fa75e31f224..5bed634551ea6 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -872,6 +872,11 @@ of_fwnode_property_read_string_array(const struct fwnode_handle *fwnode,
 		of_property_count_strings(node, propname);
 }
 
+static const char *of_fwnode_get_name(const struct fwnode_handle *fwnode)
+{
+	return kbasename(to_of_node(fwnode)->full_name);
+}
+
 static struct fwnode_handle *
 of_fwnode_get_parent(const struct fwnode_handle *fwnode)
 {
@@ -993,6 +998,7 @@ const struct fwnode_operations of_fwnode_ops = {
 	.property_present = of_fwnode_property_present,
 	.property_read_int_array = of_fwnode_property_read_int_array,
 	.property_read_string_array = of_fwnode_property_read_string_array,
+	.get_name = of_fwnode_get_name,
 	.get_parent = of_fwnode_get_parent,
 	.get_next_child_node = of_fwnode_get_next_child_node,
 	.get_named_child_node = of_fwnode_get_named_child_node,
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index a11c8c56c78b4..c331e0ef31e80 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -56,6 +56,7 @@ struct fwnode_reference_args {
  *				 otherwise.
  * @property_read_string_array: Read an array of string properties. Return zero
  *				on success, a negative error code otherwise.
+ * @get_name: Return the name of an fwnode.
  * @get_parent: Return the parent of an fwnode.
  * @get_next_child_node: Return the next child node in an iteration.
  * @get_named_child_node: Return a child node with a given name.
@@ -82,6 +83,7 @@ struct fwnode_operations {
 	(*property_read_string_array)(const struct fwnode_handle *fwnode_handle,
 				      const char *propname, const char **val,
 				      size_t nval);
+	const char *(*get_name)(const struct fwnode_handle *fwnode);
 	struct fwnode_handle *(*get_parent)(const struct fwnode_handle *fwnode);
 	struct fwnode_handle *
 	(*get_next_child_node)(const struct fwnode_handle *fwnode,
diff --git a/include/linux/property.h b/include/linux/property.h
index 5450e7ec219ac..ebc5e2016bb66 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -80,6 +80,7 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode,
 					    const char *name,
 					    unsigned int index);
 
+const char *fwnode_get_name(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_next_parent(
 	struct fwnode_handle *fwnode);
-- 
2.20.1

[PATCH v9 06/12] device property: Add a function to obtain a node's prefix

[Sakari Ailus]

The prefix is used for printing purpose before a node, and it also works
as a separator between two nodes.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Rob Herring <robh@kernel.org> (for OF)
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/property.c  | 22 ++++++++++++++++++++++
 drivers/base/property.c  | 12 ++++++++++++
 drivers/base/swnode.c    | 22 ++++++++++++++++++++++
 drivers/of/property.c    | 10 ++++++++++
 include/linux/fwnode.h   |  2 ++
 include/linux/property.h |  1 +
 6 files changed, 69 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 5a9397a390f41..466239d3bb345 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1336,6 +1336,27 @@ static const char *acpi_fwnode_get_name(const struct fwnode_handle *fwnode)
 	return acpi_device_bid(adev);
 }
 
+static const char *
+acpi_fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *parent;
+
+	/* Is this the root node? */
+	parent = fwnode_get_parent(fwnode);
+	if (!parent)
+		return "";
+
+	/* Is this 2nd node from the root? */
+	parent = fwnode_get_next_parent(parent);
+	if (!parent)
+		return "";
+
+	fwnode_handle_put(parent);
+
+	/* ACPI device or data node. */
+	return ".";
+}
+
 static struct fwnode_handle *
 acpi_fwnode_get_parent(struct fwnode_handle *fwnode)
 {
@@ -1377,6 +1398,7 @@ acpi_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
 		.get_next_child_node = acpi_get_next_subnode,		\
 		.get_named_child_node = acpi_fwnode_get_named_child_node, \
 		.get_name = acpi_fwnode_get_name,			\
+		.get_name_prefix = acpi_fwnode_get_name_prefix,		\
 		.get_reference_args = acpi_fwnode_get_reference_args,	\
 		.graph_get_next_endpoint =				\
 			acpi_graph_get_next_endpoint,			\
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 9b5ec88e72d8b..511f6d7acdfe4 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -567,6 +567,18 @@ const char *fwnode_get_name(const struct fwnode_handle *fwnode)
 	return fwnode_call_ptr_op(fwnode, get_name);
 }
 
+/**
+ * fwnode_get_name_prefix - Return the prefix of node for printing purposes
+ * @fwnode: The firmware node
+ *
+ * Returns the prefix of a node, intended to be printed right before the node.
+ * The prefix works also as a separator between the nodes.
+ */
+const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
+{
+	return fwnode_call_ptr_op(fwnode, get_name_prefix);
+}
+
 /**
  * fwnode_get_parent - Return parent firwmare node
  * @fwnode: Firmware whose parent is retrieved
diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index ccbc3d9d6c5f5..6a8c427355f8d 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -526,6 +526,27 @@ software_node_get_name(const struct fwnode_handle *fwnode)
 	return kobject_name(&swnode->kobj);
 }
 
+static const char *
+software_node_get_name_prefix(const struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *parent;
+	const char *prefix;
+
+	parent = fwnode_get_parent(fwnode);
+	if (!parent)
+		return "";
+
+	/* Figure out the prefix from the parents. */
+	while (is_software_node(parent))
+		parent = fwnode_get_next_parent(parent);
+
+	prefix = fwnode_get_name_prefix(parent);
+	fwnode_handle_put(parent);
+
+	/* Guess something if prefix was NULL. */
+	return prefix ?: "/";
+}
+
 static struct fwnode_handle *
 software_node_get_parent(const struct fwnode_handle *fwnode)
 {
@@ -627,6 +648,7 @@ static const struct fwnode_operations software_node_ops = {
 	.property_read_int_array = software_node_read_int_array,
 	.property_read_string_array = software_node_read_string_array,
 	.get_name = software_node_get_name,
+	.get_name_prefix = software_node_get_name_prefix,
 	.get_parent = software_node_get_parent,
 	.get_next_child_node = software_node_get_next_child,
 	.get_named_child_node = software_node_get_named_child_node,
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 5bed634551ea6..e8202f61a5d93 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -877,6 +877,15 @@ static const char *of_fwnode_get_name(const struct fwnode_handle *fwnode)
 	return kbasename(to_of_node(fwnode)->full_name);
 }
 
+static const char *of_fwnode_get_name_prefix(const struct fwnode_handle *fwnode)
+{
+	/* Root needs no prefix here (its name is "/"). */
+	if (!to_of_node(fwnode)->parent)
+		return "";
+
+	return "/";
+}
+
 static struct fwnode_handle *
 of_fwnode_get_parent(const struct fwnode_handle *fwnode)
 {
@@ -999,6 +1008,7 @@ const struct fwnode_operations of_fwnode_ops = {
 	.property_read_int_array = of_fwnode_property_read_int_array,
 	.property_read_string_array = of_fwnode_property_read_string_array,
 	.get_name = of_fwnode_get_name,
+	.get_name_prefix = of_fwnode_get_name_prefix,
 	.get_parent = of_fwnode_get_parent,
 	.get_next_child_node = of_fwnode_get_next_child_node,
 	.get_named_child_node = of_fwnode_get_named_child_node,
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index c331e0ef31e80..755709703fe6f 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -57,6 +57,7 @@ struct fwnode_reference_args {
  * @property_read_string_array: Read an array of string properties. Return zero
  *				on success, a negative error code otherwise.
  * @get_name: Return the name of an fwnode.
+ * @get_name_prefix: Get a prefix for a node (for printing purposes).
  * @get_parent: Return the parent of an fwnode.
  * @get_next_child_node: Return the next child node in an iteration.
  * @get_named_child_node: Return a child node with a given name.
@@ -84,6 +85,7 @@ struct fwnode_operations {
 				      const char *propname, const char **val,
 				      size_t nval);
 	const char *(*get_name)(const struct fwnode_handle *fwnode);
+	const char *(*get_name_prefix)(const struct fwnode_handle *fwnode);
 	struct fwnode_handle *(*get_parent)(const struct fwnode_handle *fwnode);
 	struct fwnode_handle *
 	(*get_next_child_node)(const struct fwnode_handle *fwnode,
diff --git a/include/linux/property.h b/include/linux/property.h
index ebc5e2016bb66..c40fcfc5774a7 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -81,6 +81,7 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode,
 					    unsigned int index);
 
 const char *fwnode_get_name(const struct fwnode_handle *fwnode);
+const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode);
 struct fwnode_handle *fwnode_get_next_parent(
 	struct fwnode_handle *fwnode);
-- 
2.20.1

[PATCH v9 07/12] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps

[Sakari Ailus]

%pS and %ps are now the preferred conversion specifiers to print function
names. The functionality is equivalent; remove the old, deprecated %pF
and %pf support.

Depends-on: commit 2d44d165e939 ("scsi: lpfc: Convert existing %pf users to %ps")
Depends-on: commit b295c3e39c13 ("tools lib traceevent: Convert remaining %p[fF] users to %p[sS]")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/core-api/printk-formats.rst | 10 ----------
 lib/vsprintf.c                            | 10 ++--------
 scripts/checkpatch.pl                     |  1 -
 3 files changed, 2 insertions(+), 19 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index c6224d039bcbe..922a29eb70e6c 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -86,8 +86,6 @@ Symbols/Function Pointers
 
 	%pS	versatile_init+0x0/0x110
 	%ps	versatile_init
-	%pF	versatile_init+0x0/0x110
-	%pf	versatile_init
 	%pSR	versatile_init+0x9/0x110
 		(with __builtin_extract_return_addr() translation)
 	%pB	prev_fn_of_versatile_init+0x88/0x88
@@ -97,14 +95,6 @@ The ``S`` and ``s`` specifiers are used for printing a pointer in symbolic
 format. They result in the symbol name with (S) or without (s)
 offsets. If KALLSYMS are disabled then the symbol address is printed instead.
 
-Note, that the ``F`` and ``f`` specifiers are identical to ``S`` (``s``)
-and thus deprecated. We have ``F`` and ``f`` because on ia64, ppc64 and
-parisc64 function pointers are indirect and, in fact, are function
-descriptors, which require additional dereferencing before we can lookup
-the symbol. As of now, ``S`` and ``s`` perform dereferencing on those
-platforms (when needed), so ``F`` and ``f`` exist for compatibility
-reasons only.
-
 The ``B`` specifier results in the symbol name with offsets and should be
 used when printing stack backtraces. The specifier takes into
 consideration the effect of compiler optimisations which may occur
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b0967cf17137d..a58329c8fa750 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -909,7 +909,7 @@ char *symbol_string(char *buf, char *end, void *ptr,
 #ifdef CONFIG_KALLSYMS
 	if (*fmt == 'B')
 		sprint_backtrace(sym, value);
-	else if (*fmt != 'f' && *fmt != 's')
+	else if (*fmt != 's')
 		sprint_symbol(sym, value);
 	else
 		sprint_symbol_no_offset(sym, value);
@@ -2007,9 +2007,7 @@ static char *kobject_string(char *buf, char *end, void *ptr,
  *
  * - 'S' For symbolic direct pointers (or function descriptors) with offset
  * - 's' For symbolic direct pointers (or function descriptors) without offset
- * - 'F' Same as 'S'
- * - 'f' Same as 's'
- * - '[FfSs]R' as above with __builtin_extract_return_addr() translation
+ * - '[Ss]R' as above with __builtin_extract_return_addr() translation
  * - 'B' For backtraced symbolic direct pointers with offset
  * - 'R' For decoded struct resource, e.g., [mem 0x0-0x1f 64bit pref]
  * - 'r' For raw struct resource, e.g., [mem 0x0-0x1f flags 0x201]
@@ -2112,8 +2110,6 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	      struct printf_spec spec)
 {
 	switch (*fmt) {
-	case 'F':
-	case 'f':
 	case 'S':
 	case 's':
 		ptr = dereference_symbol_descriptor(ptr);
@@ -2812,8 +2808,6 @@ int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args)
 			/* Dereference of functions is still OK */
 			case 'S':
 			case 's':
-			case 'F':
-			case 'f':
 			case 'x':
 			case 'K':
 				save_arg(void *);
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 93a7edfe0f059..a60c241112cd4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6012,7 +6012,6 @@ sub process {
 					my $ext_type = "Invalid";
 					my $use = "";
 					if ($bad_specifier =~ /p[Ff]/) {
-						$ext_type = "Deprecated";
 						$use = " - use %pS instead";
 						$use =~ s/pS/ps/ if ($bad_specifier =~ /pf/);
 					}
-- 
2.20.1

[PATCH v9 08/12] lib/vsprintf: Add a note on re-using %pf or %pF

[Sakari Ailus]

Add a note warning of re-use of obsolete %pf or %pF extensions.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 lib/vsprintf.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a58329c8fa750..cf48769fe330c 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2008,6 +2008,8 @@ static char *kobject_string(char *buf, char *end, void *ptr,
  * - 'S' For symbolic direct pointers (or function descriptors) with offset
  * - 's' For symbolic direct pointers (or function descriptors) without offset
  * - '[Ss]R' as above with __builtin_extract_return_addr() translation
+ * - '[Ff]' %pf and %pF were obsoleted and later removed in favor of
+ *	    %ps and %pS. Be careful when re-using these specifiers.
  * - 'B' For backtraced symbolic direct pointers with offset
  * - 'R' For decoded struct resource, e.g., [mem 0x0-0x1f 64bit pref]
  * - 'r' For raw struct resource, e.g., [mem 0x0-0x1f flags 0x201]
-- 
2.20.1

[PATCH v9 09/12] lib/vsprintf: Make use of fwnode API to obtain node names and separators

[Sakari Ailus]

Instead of implementing our own means of discovering parent nodes, node
names or counting how many parents a node has, use the newly added
functions in the fwnode API to obtain that information.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 lib/vsprintf.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index cf48769fe330c..c2be0e5bc087d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -38,6 +38,7 @@
 #include <net/addrconf.h>
 #include <linux/siphash.h>
 #include <linux/compiler.h>
+#include <linux/property.h>
 #ifdef CONFIG_BLOCK
 #include <linux/blkdev.h>
 #endif
@@ -1863,32 +1864,25 @@ char *flags_string(char *buf, char *end, void *flags_ptr,
 	return format_flags(buf, end, flags, names);
 }
 
-static const char *device_node_name_for_depth(const struct device_node *np, int depth)
-{
-	for ( ; np && depth; depth--)
-		np = np->parent;
-
-	return kbasename(np->full_name);
-}
-
 static noinline_for_stack
-char *device_node_gen_full_name(const struct device_node *np, char *buf, char *end)
+char *fwnode_full_name_string(struct fwnode_handle *fwnode, char *buf,
+			      char *end)
 {
 	int depth;
-	const struct device_node *parent = np->parent;
 
-	/* special case for root node */
-	if (!parent)
-		return string_nocheck(buf, end, "/", default_str_spec);
+	/* Loop starting from the root node to the current node. */
+	for (depth = fwnode_count_parents(fwnode); depth >= 0; depth--) {
+		struct fwnode_handle *__fwnode =
+			fwnode_get_nth_parent(fwnode, depth);
 
-	for (depth = 0; parent->parent; depth++)
-		parent = parent->parent;
-
-	for ( ; depth >= 0; depth--) {
-		buf = string_nocheck(buf, end, "/", default_str_spec);
-		buf = string(buf, end, device_node_name_for_depth(np, depth),
+		buf = string(buf, end, fwnode_get_name_prefix(__fwnode),
+			     default_str_spec);
+		buf = string(buf, end, fwnode_get_name(__fwnode),
 			     default_str_spec);
+
+		fwnode_handle_put(__fwnode);
 	}
+
 	return buf;
 }
 
@@ -1933,10 +1927,11 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 
 		switch (*fmt) {
 		case 'f':	/* full_name */
-			buf = device_node_gen_full_name(dn, buf, end);
+			buf = fwnode_full_name_string(of_fwnode_handle(dn), buf,
+						      end);
 			break;
 		case 'n':	/* name */
-			p = kbasename(of_node_full_name(dn));
+			p = fwnode_get_name(of_fwnode_handle(dn));
 			precision = str_spec.precision;
 			str_spec.precision = strchrnul(p, '@') - p;
 			buf = string(buf, end, p, str_spec);
@@ -1946,7 +1941,7 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 			buf = number(buf, end, (unsigned int)dn->phandle, num_spec);
 			break;
 		case 'P':	/* path-spec */
-			p = kbasename(of_node_full_name(dn));
+			p = fwnode_get_name(of_fwnode_handle(dn));
 			if (!p[1])
 				p = "/";
 			buf = string(buf, end, p, str_spec);
-- 
2.20.1

[PATCH v9 10/12] lib/vsprintf: OF nodes are first and foremost, struct device_nodes

[Sakari Ailus]

Factor out static kobject_string() function that simply calls
device_node_string(), and thus remove references to kobjects (as these are
struct device_node).

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 lib/vsprintf.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index c2be0e5bc087d..3c56528dcc3d9 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1906,6 +1906,9 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	struct printf_spec str_spec = spec;
 	str_spec.field_width = -1;
 
+	if (fmt[0] != 'F')
+		return error_string(buf, end, "(%pO?)", spec);
+
 	if (!IS_ENABLED(CONFIG_OF))
 		return error_string(buf, end, "(%pOF?)", spec);
 
@@ -1979,17 +1982,6 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
-static char *kobject_string(char *buf, char *end, void *ptr,
-			    struct printf_spec spec, const char *fmt)
-{
-	switch (fmt[1]) {
-	case 'F':
-		return device_node_string(buf, end, ptr, spec, fmt + 1);
-	}
-
-	return error_string(buf, end, "(%pO?)", spec);
-}
-
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -2170,7 +2162,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 	case 'G':
 		return flags_string(buf, end, ptr, spec, fmt);
 	case 'O':
-		return kobject_string(buf, end, ptr, spec, fmt);
+		return device_node_string(buf, end, ptr, spec, fmt + 1);
 	case 'x':
 		return pointer_string(buf, end, ptr, spec);
 	}
-- 
2.20.1

[PATCH v9 11/12] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names

[Sakari Ailus]

Add support for %pfw conversion specifier (with "f" and "P" modifiers) to
support printing full path of the node, including its name ("f") and only
the node's name ("P") in the printk family of functions. The two flags
have equivalent functionality to existing %pOF with the same two modifiers
("f" and "P") on OF based systems. The ability to do the same on ACPI
based systems is added by this patch.

On ACPI based systems the resulting strings look like

	\_SB.PCI0.CIO2.port@1.endpoint@0

where the nodes are separated by a dot (".") and the first three are
ACPI device nodes and the latter two ACPI data nodes.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 Documentation/core-api/printk-formats.rst | 24 +++++++++++++++
 lib/vsprintf.c                            | 36 +++++++++++++++++++++++
 scripts/checkpatch.pl                     |  8 +++--
 3 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 922a29eb70e6c..1ab3841686dc5 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -418,6 +418,30 @@ Examples::
 
 Passed by reference.
 
+Fwnode handles
+--------------
+
+::
+
+	%pfw[fP]
+
+For printing information on fwnode handles. The default is to print the full
+node name, including the path. The modifiers are functionally equivalent to
+%pOF above.
+
+	- f - full name of the node, including the path
+	- P - the name of the node including an address (if there is one)
+
+Examples (ACPI)::
+
+	%pfwf	\_SB.PCI0.CIO2.port@1.endpoint@0	- Full node name
+	%pfwP	endpoint@0				- Node name
+
+Examples (OF)::
+
+	%pfwf	/ocp@68000000/i2c@48072000/camera@10/port/endpoint - Full name
+	%pfwP	endpoint				- Node name
+
 Time and date (struct rtc_time)
 -------------------------------
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3c56528dcc3d9..3b668ea2d1aca 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1982,6 +1982,36 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
 	return widen_string(buf, buf - buf_start, end, spec);
 }
 
+static noinline_for_stack
+char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
+		    struct printf_spec spec, const char *fmt)
+{
+	struct printf_spec str_spec = spec;
+	char *buf_start = buf;
+
+	str_spec.field_width = -1;
+
+	if (*fmt != 'w')
+		return error_string(buf, end, "(%pf?)", spec);
+
+	if (check_pointer(&buf, end, fwnode, spec))
+		return buf;
+
+	fmt++;
+
+	switch (*fmt) {
+	case 'P':	/* name */
+		buf = string(buf, end, fwnode_get_name(fwnode), str_spec);
+		break;
+	case 'f':	/* full_name */
+	default:
+		buf = fwnode_full_name_string(fwnode, buf, end);
+		break;
+	}
+
+	return widen_string(buf, buf - buf_start, end, spec);
+}
+
 /*
  * Show a '%p' thing.  A kernel extension is that the '%p' is followed
  * by an extra set of alphanumeric characters that are extended format
@@ -2086,6 +2116,10 @@ char *device_node_string(char *buf, char *end, struct device_node *dn,
  *                  F device node flags
  *                  c major compatible string
  *                  C full compatible string
+ * - 'fw[fP]'	For a firmware node (struct fwnode_handle) pointer
+ *		Without an option prints the full name of the node
+ *		f full name
+ *		P node name, including a possible unit address
  * - 'x' For printing the address. Equivalent to "%lx".
  *
  * ** When making changes please also update:
@@ -2163,6 +2197,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return flags_string(buf, end, ptr, spec, fmt);
 	case 'O':
 		return device_node_string(buf, end, ptr, spec, fmt + 1);
+	case 'f':
+		return fwnode_string(buf, end, ptr, spec, fmt + 1);
 	case 'x':
 		return pointer_string(buf, end, ptr, spec);
 	}
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a60c241112cd4..726b6951bce78 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5988,14 +5988,18 @@ sub process {
 		        for (my $count = $linenr; $count <= $lc; $count++) {
 				my $specifier;
 				my $extension;
+				my $qualifier;
 				my $bad_specifier = "";
 				my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0));
 				$fmt =~ s/%%//g;
 
-				while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) {
+				while ($fmt =~ /(\%[\*\d\.]*p(\w)(\w*))/g) {
 					$specifier = $1;
 					$extension = $2;
-					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxt]/) {
+					$qualifier = $3;
+					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxtf]/ ||
+					    ($extension eq "f" &&
+					     defined $qualifier && $qualifier !~ /^w/)) {
 						$bad_specifier = $specifier;
 						last;
 					}
-- 
2.20.1

[PATCH v9 12/12] lib/test_printf: Add tests for %pfw printk modifier

[Sakari Ailus]

Add a test for the %pfw printk modifier using software nodes.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 lib/test_printf.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 944eb50f38625..bb6a7d334084b 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -22,6 +22,8 @@
 #include <linux/gfp.h>
 #include <linux/mm.h>
 
+#include <linux/property.h>
+
 #include "../tools/testing/selftests/kselftest_module.h"
 
 #define BUF_SIZE 256
@@ -588,6 +590,35 @@ flags(void)
 	kfree(cmp_buffer);
 }
 
+static void __init fwnode_pointer(void)
+{
+	const struct software_node softnodes[] = {
+		{ .name = "first", },
+		{ .name = "second", .parent = &softnodes[0], },
+		{ .name = "third", .parent = &softnodes[1], },
+		{ NULL /* Guardian */ }
+	};
+	const char * const full_name = "first/second/third";
+	const char * const full_name_second = "first/second";
+	const char * const second_name = "second";
+	const char * const third_name = "third";
+	int rval;
+
+	rval = software_node_register_nodes(softnodes);
+	if (rval) {
+		pr_warn("cannot register softnodes; rval %d\n", rval);
+		return;
+	}
+
+	test(full_name_second, "%pfw", software_node_fwnode(&softnodes[1]));
+	test(full_name, "%pfw", software_node_fwnode(&softnodes[2]));
+	test(full_name, "%pfwf", software_node_fwnode(&softnodes[2]));
+	test(second_name, "%pfwP", software_node_fwnode(&softnodes[1]));
+	test(third_name, "%pfwP", software_node_fwnode(&softnodes[2]));
+
+	software_node_unregister_nodes(softnodes);
+}
+
 static void __init
 test_pointer(void)
 {
@@ -610,6 +641,7 @@ test_pointer(void)
 	bitmap();
 	netdev_features();
 	flags();
+	fwnode_pointer();
 }
 
 static void __init selftest(void)
-- 
2.20.1

Re: [PATCH v9 07/12] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps

[Petr Mladek]

On Thu 2019-10-03 15:32:14, Sakari Ailus wrote:
> %pS and %ps are now the preferred conversion specifiers to print function
> names. The functionality is equivalent; remove the old, deprecated %pF
> and %pf support.
> 
> Depends-on: commit 2d44d165e939 ("scsi: lpfc: Convert existing %pf users to %ps")
> Depends-on: commit b295c3e39c13 ("tools lib traceevent: Convert remaining %p[fF] users to %p[sS]")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v9 08/12] lib/vsprintf: Add a note on re-using %pf or %pF

[Petr Mladek]

On Thu 2019-10-03 15:32:15, Sakari Ailus wrote:
> Add a note warning of re-use of obsolete %pf or %pF extensions.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v9 00/12] Device property improvements, add %pfw format specifier

[Petr Mladek]

On Thu 2019-10-03 15:32:07, Sakari Ailus wrote:
> Hi all,
> 
> This set adds functionality into the device property API (counting a
> node's parents as well as obtaining its name) in order to support printing
> fwnode names using a new conversion specifier "%pfw". The names that are
> produced are equivalent to its OF counterpart "%pOF" on OF systems for the
> two supported modifiers ("f" and "P").
> 
> Printing a node's name is something that's been available on OF for a long
> time and if something is converted to device property API (such as the
> V4L2 fwnode framework) it always got removed of a nice feature that was
> sometimes essential in debugging. With this set, that no longer is the
> case.

The patchset is ready to go upstream from my POV.

Should it go via a tree that maintains the device property API
or via printk.git? Both possibilities work for me.

Best Regards,
Petr

Re: [PATCH v9 00/12] Device property improvements, add %pfw format specifier

[Rafael J. Wysocki]

On Thursday, October 3, 2019 2:32:07 PM CEST Sakari Ailus wrote:
> Hi all,
> 
> This set adds functionality into the device property API (counting a
> node's parents as well as obtaining its name) in order to support printing
> fwnode names using a new conversion specifier "%pfw". The names that are
> produced are equivalent to its OF counterpart "%pOF" on OF systems for the
> two supported modifiers ("f" and "P").
> 
> Printing a node's name is something that's been available on OF for a long
> time and if something is converted to device property API (such as the
> V4L2 fwnode framework) it always got removed of a nice feature that was
> sometimes essential in debugging. With this set, that no longer is the
> case.
> 
> Note: the set now depends on commit b295c3e39c13 ("tools lib traceevent:
> Convert remaining %p[fF] users to %p[sS]") and commit 2d44d165e939 ("scsi:
> lpfc: Convert existing %pf users to %ps") which both are in v5.4-rc1.
> 
> Note 2: There's no full v8 set as I sent a patch fixing a single issue but
> there turned out to be a little more to fix, making this one v9.
> 
> since v7:
> 
> - Reword warning on re-use of %pf and %pF.
> 
> - Remove %pf and %pF support from vbin_printf() as well.
> 
> - Removed the first patch (now commit b295c3e39c13 ("tools lib traceevent:
>   Convert remaining %p[fF] users to %p[sS]")) from the set as it was
>   already merged through Arnoldo Carvalho de Melo's tree. Added
>   Depends-on: tag to the same patch.
> 
> since v6:
> 
> - Added a patch for a warning note on re-using obsolete %pf oand %pF
>   extensions.
> 
> - Rework %pfw validatition in checkpatch.pl according to Joe's comments.
> 
> - Unwrap a line wrapped by a previous versions in get_bprint_format().
> 
> - Handle %pf and %pF modifiers again in make_bprint_args() in case they're
>   no followed by 'w'.
> 
> since v5:
> 
> - Added a patch to convert %pf to %ps in tools/lib/traceevent.c (first in
>   the set).
> 
> - Fix ReST syntax in Documentation/core-api/printk-formats.rst.
> 
> - Fix returning root swnode name in patch "device property: Add
>   fwnode_get_name for returning the name of a node". Use to_swnode()
>   directly as well in the same patch.
> 
> - Tests: take root node name into account, use direct indices and remove
>   the comma from the guardian entry.
> 
> - Add a comment on how fwnode_full_name_string() enumerates the nodes.
> 
> - Fix error string in fwnode_string().
> 
> - Move 'f' + default case as last in the switch in fwnode_string().
> 
> - Fix %pfw validation in checkpatch.pl.
> 
> since v4:
> 
> - Improved documentation for fwnode_get_nth_parent().
> 
> - Removed comma from the guardian entry in fwnode_pointer() testcase.
> 
> since v3:
> 
> - Remove underscores in argument name of fwnode_count_parents().
> 
> - Re-introduce "%pO?" error string.
> 
> - Unwrap a call to string() in fwnode_string().
> 
> - Removed a useless Depends-on: on a patch that was merged long ago.
> 
> - Unwrap a Fixes: line.
> 
> - Added a patch to move fwnode_get_parent() up to make the review of the
>   following patch easier.
> 
> since v2:
> 
> - Better comments in acpi_fwnode_get_name_prefix().
> 
> - Added swnode implementation.
> 
> - Fixed swnode refcounting in get_parent() ("swnode: Get reference to
>   parent swnode in get_parent op")
> 
> - Make argument to to_software_node() const (a new patch)
> 
> - Factored out confusingly named kobject_string() that had a single
>   caller.
> 
> - Cleaner fwnode_count_parents() implementation (as discussed in review).
> 
> - Made fwnode_count_parents() argument const.
> 
> - Added tests (last patch in the set).
> 
> since v1:
> 
> - Add patch to remove support for %pf and %pF (depends on another patch
>   removing all use of %pf and %pF) (now 4th patch)
> 
> - Fix kerneldoc argument documentation for fwnode_get_name (2nd patch)
> 
> - Align kerneldoc style with the rest of drivers/base/property.c (no extra
>   newline after function name)
> 
> - Make checkpatch.pl complain about "%pf" not followed by "w" (6th patch)
> 
> - WARN_ONCE() on use of invalid conversion specifiers ("%pf" not followed
>   by "w")
> 
> Sakari Ailus (12):
>   software node: Get reference to parent swnode in get_parent op
>   software node: Make argument to to_software_node const
>   device property: Move fwnode_get_parent() up
>   device property: Add functions for accessing node's parents
>   device property: Add fwnode_get_name for returning the name of a node
>   device property: Add a function to obtain a node's prefix
>   lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps
>   lib/vsprintf: Add a note on re-using %pf or %pF
>   lib/vsprintf: Make use of fwnode API to obtain node names and
>     separators
>   lib/vsprintf: OF nodes are first and foremost, struct device_nodes
>   lib/vsprintf: Add %pfw conversion specifier for printing fwnode names
>   lib/test_printf: Add tests for %pfw printk modifier
> 
>  Documentation/core-api/printk-formats.rst | 34 ++++++---
>  drivers/acpi/property.c                   | 48 ++++++++++++
>  drivers/base/property.c                   | 83 ++++++++++++++++++--
>  drivers/base/swnode.c                     | 43 ++++++++++-
>  drivers/of/property.c                     | 16 ++++
>  include/linux/fwnode.h                    |  4 +
>  include/linux/property.h                  |  8 +-
>  lib/test_printf.c                         | 32 ++++++++
>  lib/vsprintf.c                            | 93 ++++++++++++++---------
>  scripts/checkpatch.pl                     |  9 ++-
>  10 files changed, 309 insertions(+), 61 deletions(-)
> 
> 

Applying the series (with the tags collected so far) as 5.5 material, thanks!

Re: [PATCH v9 09/12] lib/vsprintf: Make use of fwnode API to obtain node names and separators

[Guenter Roeck]

Hi,

On Thu, Oct 03, 2019 at 03:32:16PM +0300, Sakari Ailus wrote:
> Instead of implementing our own means of discovering parent nodes, node
> names or counting how many parents a node has, use the newly added
> functions in the fwnode API to obtain that information.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> ---

This patch results in a lockdep splat when running one of my qemu
emulations. See below for log and bisect results. A complete log
is available at
https://kerneltests.org/builders/qemu-arm-master/builds/1408/steps/qemubuildcommand/logs/stdio

Guenter

---
======================================================
WARNING: possible circular locking dependency detected
5.5.0-rc4-00066-g738d2902773e #1 Not tainted
------------------------------------------------------
swapper/0/1 is trying to acquire lock:
c1313b00 (logbuf_lock){-.-.}, at: vprintk_emit+0x68/0x2d4

but task is already holding lock:
ef030b90 (&(&n->list_lock)->rlock){..-.}, at: free_debug_processing+0x38/0x418

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #2 (&(&n->list_lock)->rlock){..-.}:
       ___slab_alloc.constprop.23+0x12c/0x798
       __slab_alloc.constprop.22+0x44/0x70
       __kmalloc+0x384/0x41c
       of_populate_phandle_cache+0xcc/0x148
       of_core_init+0x8/0xbc
       driver_init+0x1c/0x2c
       kernel_init_freeable+0xac/0x1b4
       kernel_init+0x8/0x118
       ret_from_fork+0x14/0x20
       0x0

-> #1 (devtree_lock){....}:
       of_get_parent+0x18/0x34
       of_fwnode_get_parent+0x34/0x40
       fwnode_count_parents+0x28/0x58
       fwnode_full_name_string+0x18/0xa0
       device_node_string+0x490/0x4f0
       pointer+0x440/0x4d8
       vsnprintf+0x1bc/0x3d8
       vscnprintf+0xc/0x24
       vprintk_store+0x34/0x204
       vprintk_emit+0x94/0x2d4
       vprintk_default+0x20/0x28
       printk+0x30/0x54
       exynos4_pm_init_power_domain+0x220/0x258
       do_one_initcall+0x8c/0x440
       kernel_init_freeable+0x150/0x1b4
       kernel_init+0x8/0x118
       ret_from_fork+0x14/0x20
       0x0

-> #0 (logbuf_lock){-.-.}:
       lock_acquire+0xec/0x290
       _raw_spin_lock+0x38/0x48
       vprintk_emit+0x68/0x2d4
       vprintk_default+0x20/0x28
       printk+0x30/0x54
       unwind_frame+0x6a8/0x6fc
       walk_stackframe+0x2c/0x38
       __save_stack_trace+0x84/0x8c
       stack_trace_save+0x3c/0x5c
       set_track+0x40/0x9c
       free_debug_processing+0x1a4/0x418
       __slab_free+0x2d4/0x510
       kmem_cache_free+0x44c/0x49c
       rcu_core+0x348/0x994
       __do_softirq+0x164/0x668
       irq_exit+0x16c/0x170
       __handle_domain_irq+0x80/0xec
       gic_handle_irq+0x58/0x9c
       __irq_svc+0x70/0xb0
       raid6_neon8_gen_syndrome_real+0x264/0x39c
       raid6_neon8_gen_syndrome_real+0x264/0x39c

other info that might help us debug this:

Chain exists of:
  logbuf_lock --> devtree_lock --> &(&n->list_lock)->rlock

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&(&n->list_lock)->rlock);
                               lock(devtree_lock);
                               lock(&(&n->list_lock)->rlock);
  lock(logbuf_lock);

 *** DEADLOCK ***

2 locks held by swapper/0/1:
 #0: c131466c (rcu_callback){....}, at: rcu_core+0x304/0x994
 #1: ef030b90 (&(&n->list_lock)->rlock){..-.}, at: free_debug_processing+0x38/0x418

stack backtrace:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.5.0-rc4-00066-g738d2902773e #1
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[<c0113264>] (unwind_backtrace) from [<c010e448>] (show_stack+0x10/0x14)
[<c010e448>] (show_stack) from [<c0c9b754>] (dump_stack+0xa4/0xd0)
[<c0c9b754>] (dump_stack) from [<c018cbd8>] (check_noncircular+0x258/0x274)
[<c018cbd8>] (check_noncircular) from [<c019043c>] (__lock_acquire+0x1870/0x2860)
[<c019043c>] (__lock_acquire) from [<c018e088>] (lock_acquire+0xec/0x290)
[<c018e088>] (lock_acquire) from [<c0cbf278>] (_raw_spin_lock+0x38/0x48)
[<c0cbf278>] (_raw_spin_lock) from [<c01a1f84>] (vprintk_emit+0x68/0x2d4)
[<c01a1f84>] (vprintk_emit) from [<c01a2210>] (vprintk_default+0x20/0x28)
[<c01a2210>] (vprintk_default) from [<c01a2844>] (printk+0x30/0x54)
[<c01a2844>] (printk) from [<c0113210>] (unwind_frame+0x6a8/0x6fc)
[<c0113210>] (unwind_frame) from [<c010ddf0>] (walk_stackframe+0x2c/0x38)
[<c010ddf0>] (walk_stackframe) from [<c010df54>] (__save_stack_trace+0x84/0x8c)
[<c010df54>] (__save_stack_trace) from [<c01c2d40>] (stack_trace_save+0x3c/0x5c)
[<c01c2d40>] (stack_trace_save) from [<c02ae258>] (set_track+0x40/0x9c)
[<c02ae258>] (set_track) from [<c02b06a8>] (free_debug_processing+0x1a4/0x418)
[<c02b06a8>] (free_debug_processing) from [<c02b0bf0>] (__slab_free+0x2d4/0x510)
[<c02b0bf0>] (__slab_free) from [<c02b17ac>] (kmem_cache_free+0x44c/0x49c)
[<c02b17ac>] (kmem_cache_free) from [<c01bd608>] (rcu_core+0x348/0x994)
[<c01bd608>] (rcu_core) from [<c010230c>] (__do_softirq+0x164/0x668)
[<c010230c>] (__do_softirq) from [<c0131310>] (irq_exit+0x16c/0x170)
[<c0131310>] (irq_exit) from [<c01a3740>] (__handle_domain_irq+0x80/0xec)
[<c01a3740>] (__handle_domain_irq) from [<c0630828>] (gic_handle_irq+0x58/0x9c)
[<c0630828>] (gic_handle_irq) from [<c0101a70>] (__irq_svc+0x70/0xb0)
Exception stack(0xef19bd30 to 0xef19bd78)
bd20:                                     c0d43e50 c0d43e60 ef19bebc c0d43e70
bd40: c0d43e20 c0d43e10 00000400 00000430 00000440 00000450 00000460 00000470
bd60: c0d43e40 ef19bd80 c0d43e30 c0625c00 20000013 ffffffff
[<c0101a70>] (__irq_svc) from [<c0625c00>] (raid6_neon8_gen_syndrome_real+0x264/0x39c)

---
Bisect:

# bad: [738d2902773e30939a982c8df7a7f94293659810] Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
# good: [219d54332a09e8d8741c1e1982f5eae56099de85] Linux 5.4
git bisect start 'HEAD' 'v5.4'
# bad: [8c39f71ee2019e77ee14f88b1321b2348db51820] Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
git bisect bad 8c39f71ee2019e77ee14f88b1321b2348db51820
# good: [3b397c7ccafe0624018cb09fc96729f8f6165573] Merge tag 'regmap-v5.5' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap
git bisect good 3b397c7ccafe0624018cb09fc96729f8f6165573
# bad: [89d57dddd7d319ded00415790a0bb3c954b7e386] Merge tag 'media/v5.5-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
git bisect bad 89d57dddd7d319ded00415790a0bb3c954b7e386
# good: [9e7a03233e02afd3ee061e373355f34d7254f1e6] Merge tag 'pm-5.5-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
git bisect good 9e7a03233e02afd3ee061e373355f34d7254f1e6
# good: [09578eacaaa44149738267083ccc050990409f86] Merge tag 'asoc-v5.5-2' of https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound into for-linus
git bisect good 09578eacaaa44149738267083ccc050990409f86
# good: [1c7ae4a51298d52a21f63b2214657982036c7498] media: ad5820: Add support for of-autoload
git bisect good 1c7ae4a51298d52a21f63b2214657982036c7498
# good: [99cf8a7074c4ce3ff3685cd389f54e7bd4bbf510] media: dt-bindings: Fix building error for dt_binding_check
git bisect good 99cf8a7074c4ce3ff3685cd389f54e7bd4bbf510
# good: [782b59711e1561ee0da06bc478ca5e8249aa8d09] Merge branch 'acpi-mm'
git bisect good 782b59711e1561ee0da06bc478ca5e8249aa8d09
# good: [0ca40f41d795fd91811e44506bb73d0b9ca33bdd] Merge branch 'patchwork' into v4l_for_linus
git bisect good 0ca40f41d795fd91811e44506bb73d0b9ca33bdd
# bad: [a00351687f8a05773c1c57be80a5bbca68fa9ae8] software node: remove DEV_PROP_MAX
git bisect bad a00351687f8a05773c1c57be80a5bbca68fa9ae8
# good: [9af7706492f985867d070861fe39fee0fe41326f] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps
git bisect good 9af7706492f985867d070861fe39fee0fe41326f
# bad: [83abc5a77f3b028b8c845c39ce4053119e1de35b] lib/vsprintf: OF nodes are first and foremost, struct device_nodes
git bisect bad 83abc5a77f3b028b8c845c39ce4053119e1de35b
# bad: [a92eb7621b9fb2c28a588ce333d226f56fab6a85] lib/vsprintf: Make use of fwnode API to obtain node names and separators
git bisect bad a92eb7621b9fb2c28a588ce333d226f56fab6a85
# good: [1586c5ae2f9310235b5e70abe712c73fc32eb98f] lib/vsprintf: Add a note on re-using %pf or %pF
git bisect good 1586c5ae2f9310235b5e70abe712c73fc32eb98f
# first bad commit: [a92eb7621b9fb2c28a588ce333d226f56fab6a85] lib/vsprintf: Make use of fwnode API to obtain node names and separators

Re: [PATCH v9 09/12] lib/vsprintf: Make use of fwnode API to obtain node names and separators

[Sakari Ailus]

Hi Guenter,

On Thu, Jan 02, 2020 at 02:20:41PM -0800, Guenter Roeck wrote:
> Hi,
> 
> On Thu, Oct 03, 2019 at 03:32:16PM +0300, Sakari Ailus wrote:
> > Instead of implementing our own means of discovering parent nodes, node
> > names or counting how many parents a node has, use the newly added
> > functions in the fwnode API to obtain that information.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > ---
> 
> This patch results in a lockdep splat when running one of my qemu
> emulations. See below for log and bisect results. A complete log
> is available at
> https://kerneltests.org/builders/qemu-arm-master/builds/1408/steps/qemubuildcommand/logs/stdio
> 
> Guenter

Thank you for reporting this.

I looked into the issue, and indeed I can conform the patch introduces this
as it takes the devtree_lock for printing the name of the fwnode. There is
however chance of a deadlock in practice as the code in mm/slub.c does not
deal with fwnodes (in which case acquiring devtree_lock could be possible),
maybe for other reasons as well. The patch however introduces an unpleasant
source of such warnings.

One approach to address this could be not allocating memory while holding
devtree_lock spinlock. That seems entirely feasible. But could also
releasing memory cause something to be printed, effectively causing the
same problem? The code in mm/slub.c is non-trivial, and I haven't checked
other allocators.

Perhaps a safest way to fix this could be returning to use dn->full_name
for printing node names, in which case the devtree_lock would no longer be
taken for printing names. The effect would be though that there would be
again one more user for the full_name field, information that can be
reconstructed from the node's parents.

> 
> ---
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.5.0-rc4-00066-g738d2902773e #1 Not tainted
> ------------------------------------------------------
> swapper/0/1 is trying to acquire lock:
> c1313b00 (logbuf_lock){-.-.}, at: vprintk_emit+0x68/0x2d4
> 
> but task is already holding lock:
> ef030b90 (&(&n->list_lock)->rlock){..-.}, at: free_debug_processing+0x38/0x418
> 
> which lock already depends on the new lock.
> 
> 
> the existing dependency chain (in reverse order) is:
> 
> -> #2 (&(&n->list_lock)->rlock){..-.}:
>        ___slab_alloc.constprop.23+0x12c/0x798
>        __slab_alloc.constprop.22+0x44/0x70
>        __kmalloc+0x384/0x41c
>        of_populate_phandle_cache+0xcc/0x148
>        of_core_init+0x8/0xbc
>        driver_init+0x1c/0x2c
>        kernel_init_freeable+0xac/0x1b4
>        kernel_init+0x8/0x118
>        ret_from_fork+0x14/0x20
>        0x0
> 
> -> #1 (devtree_lock){....}:
>        of_get_parent+0x18/0x34
>        of_fwnode_get_parent+0x34/0x40
>        fwnode_count_parents+0x28/0x58
>        fwnode_full_name_string+0x18/0xa0
>        device_node_string+0x490/0x4f0
>        pointer+0x440/0x4d8
>        vsnprintf+0x1bc/0x3d8
>        vscnprintf+0xc/0x24
>        vprintk_store+0x34/0x204
>        vprintk_emit+0x94/0x2d4
>        vprintk_default+0x20/0x28
>        printk+0x30/0x54
>        exynos4_pm_init_power_domain+0x220/0x258
>        do_one_initcall+0x8c/0x440
>        kernel_init_freeable+0x150/0x1b4
>        kernel_init+0x8/0x118
>        ret_from_fork+0x14/0x20
>        0x0
> 
> -> #0 (logbuf_lock){-.-.}:
>        lock_acquire+0xec/0x290
>        _raw_spin_lock+0x38/0x48
>        vprintk_emit+0x68/0x2d4
>        vprintk_default+0x20/0x28
>        printk+0x30/0x54
>        unwind_frame+0x6a8/0x6fc
>        walk_stackframe+0x2c/0x38
>        __save_stack_trace+0x84/0x8c
>        stack_trace_save+0x3c/0x5c
>        set_track+0x40/0x9c
>        free_debug_processing+0x1a4/0x418
>        __slab_free+0x2d4/0x510
>        kmem_cache_free+0x44c/0x49c
>        rcu_core+0x348/0x994
>        __do_softirq+0x164/0x668
>        irq_exit+0x16c/0x170
>        __handle_domain_irq+0x80/0xec
>        gic_handle_irq+0x58/0x9c
>        __irq_svc+0x70/0xb0
>        raid6_neon8_gen_syndrome_real+0x264/0x39c
>        raid6_neon8_gen_syndrome_real+0x264/0x39c
> 
> other info that might help us debug this:
> 
> Chain exists of:
>   logbuf_lock --> devtree_lock --> &(&n->list_lock)->rlock
> 
>  Possible unsafe locking scenario:
> 
>        CPU0                    CPU1
>        ----                    ----
>   lock(&(&n->list_lock)->rlock);
>                                lock(devtree_lock);
>                                lock(&(&n->list_lock)->rlock);
>   lock(logbuf_lock);
> 
>  *** DEADLOCK ***
> 
> 2 locks held by swapper/0/1:
>  #0: c131466c (rcu_callback){....}, at: rcu_core+0x304/0x994
>  #1: ef030b90 (&(&n->list_lock)->rlock){..-.}, at: free_debug_processing+0x38/0x418
> 
> stack backtrace:
> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.5.0-rc4-00066-g738d2902773e #1
> Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> [<c0113264>] (unwind_backtrace) from [<c010e448>] (show_stack+0x10/0x14)
> [<c010e448>] (show_stack) from [<c0c9b754>] (dump_stack+0xa4/0xd0)
> [<c0c9b754>] (dump_stack) from [<c018cbd8>] (check_noncircular+0x258/0x274)
> [<c018cbd8>] (check_noncircular) from [<c019043c>] (__lock_acquire+0x1870/0x2860)
> [<c019043c>] (__lock_acquire) from [<c018e088>] (lock_acquire+0xec/0x290)
> [<c018e088>] (lock_acquire) from [<c0cbf278>] (_raw_spin_lock+0x38/0x48)
> [<c0cbf278>] (_raw_spin_lock) from [<c01a1f84>] (vprintk_emit+0x68/0x2d4)
> [<c01a1f84>] (vprintk_emit) from [<c01a2210>] (vprintk_default+0x20/0x28)
> [<c01a2210>] (vprintk_default) from [<c01a2844>] (printk+0x30/0x54)
> [<c01a2844>] (printk) from [<c0113210>] (unwind_frame+0x6a8/0x6fc)
> [<c0113210>] (unwind_frame) from [<c010ddf0>] (walk_stackframe+0x2c/0x38)
> [<c010ddf0>] (walk_stackframe) from [<c010df54>] (__save_stack_trace+0x84/0x8c)
> [<c010df54>] (__save_stack_trace) from [<c01c2d40>] (stack_trace_save+0x3c/0x5c)
> [<c01c2d40>] (stack_trace_save) from [<c02ae258>] (set_track+0x40/0x9c)
> [<c02ae258>] (set_track) from [<c02b06a8>] (free_debug_processing+0x1a4/0x418)
> [<c02b06a8>] (free_debug_processing) from [<c02b0bf0>] (__slab_free+0x2d4/0x510)
> [<c02b0bf0>] (__slab_free) from [<c02b17ac>] (kmem_cache_free+0x44c/0x49c)
> [<c02b17ac>] (kmem_cache_free) from [<c01bd608>] (rcu_core+0x348/0x994)
> [<c01bd608>] (rcu_core) from [<c010230c>] (__do_softirq+0x164/0x668)
> [<c010230c>] (__do_softirq) from [<c0131310>] (irq_exit+0x16c/0x170)
> [<c0131310>] (irq_exit) from [<c01a3740>] (__handle_domain_irq+0x80/0xec)
> [<c01a3740>] (__handle_domain_irq) from [<c0630828>] (gic_handle_irq+0x58/0x9c)
> [<c0630828>] (gic_handle_irq) from [<c0101a70>] (__irq_svc+0x70/0xb0)
> Exception stack(0xef19bd30 to 0xef19bd78)
> bd20:                                     c0d43e50 c0d43e60 ef19bebc c0d43e70
> bd40: c0d43e20 c0d43e10 00000400 00000430 00000440 00000450 00000460 00000470
> bd60: c0d43e40 ef19bd80 c0d43e30 c0625c00 20000013 ffffffff
> [<c0101a70>] (__irq_svc) from [<c0625c00>] (raid6_neon8_gen_syndrome_real+0x264/0x39c)
> 
> ---
> Bisect:
> 
> # bad: [738d2902773e30939a982c8df7a7f94293659810] Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> # good: [219d54332a09e8d8741c1e1982f5eae56099de85] Linux 5.4
> git bisect start 'HEAD' 'v5.4'
> # bad: [8c39f71ee2019e77ee14f88b1321b2348db51820] Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> git bisect bad 8c39f71ee2019e77ee14f88b1321b2348db51820
> # good: [3b397c7ccafe0624018cb09fc96729f8f6165573] Merge tag 'regmap-v5.5' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap
> git bisect good 3b397c7ccafe0624018cb09fc96729f8f6165573
> # bad: [89d57dddd7d319ded00415790a0bb3c954b7e386] Merge tag 'media/v5.5-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
> git bisect bad 89d57dddd7d319ded00415790a0bb3c954b7e386
> # good: [9e7a03233e02afd3ee061e373355f34d7254f1e6] Merge tag 'pm-5.5-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
> git bisect good 9e7a03233e02afd3ee061e373355f34d7254f1e6
> # good: [09578eacaaa44149738267083ccc050990409f86] Merge tag 'asoc-v5.5-2' of https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound into for-linus
> git bisect good 09578eacaaa44149738267083ccc050990409f86
> # good: [1c7ae4a51298d52a21f63b2214657982036c7498] media: ad5820: Add support for of-autoload
> git bisect good 1c7ae4a51298d52a21f63b2214657982036c7498
> # good: [99cf8a7074c4ce3ff3685cd389f54e7bd4bbf510] media: dt-bindings: Fix building error for dt_binding_check
> git bisect good 99cf8a7074c4ce3ff3685cd389f54e7bd4bbf510
> # good: [782b59711e1561ee0da06bc478ca5e8249aa8d09] Merge branch 'acpi-mm'
> git bisect good 782b59711e1561ee0da06bc478ca5e8249aa8d09
> # good: [0ca40f41d795fd91811e44506bb73d0b9ca33bdd] Merge branch 'patchwork' into v4l_for_linus
> git bisect good 0ca40f41d795fd91811e44506bb73d0b9ca33bdd
> # bad: [a00351687f8a05773c1c57be80a5bbca68fa9ae8] software node: remove DEV_PROP_MAX
> git bisect bad a00351687f8a05773c1c57be80a5bbca68fa9ae8
> # good: [9af7706492f985867d070861fe39fee0fe41326f] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps
> git bisect good 9af7706492f985867d070861fe39fee0fe41326f
> # bad: [83abc5a77f3b028b8c845c39ce4053119e1de35b] lib/vsprintf: OF nodes are first and foremost, struct device_nodes
> git bisect bad 83abc5a77f3b028b8c845c39ce4053119e1de35b
> # bad: [a92eb7621b9fb2c28a588ce333d226f56fab6a85] lib/vsprintf: Make use of fwnode API to obtain node names and separators
> git bisect bad a92eb7621b9fb2c28a588ce333d226f56fab6a85
> # good: [1586c5ae2f9310235b5e70abe712c73fc32eb98f] lib/vsprintf: Add a note on re-using %pf or %pF
> git bisect good 1586c5ae2f9310235b5e70abe712c73fc32eb98f
> # first bad commit: [a92eb7621b9fb2c28a588ce333d226f56fab6a85] lib/vsprintf: Make use of fwnode API to obtain node names and separators

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v9 09/12] lib/vsprintf: Make use of fwnode API to obtain node names and separators

[Petr Mladek]

On Fri 2020-01-03 13:21:45, Sakari Ailus wrote:
> Hi Guenter,
> 
> On Thu, Jan 02, 2020 at 02:20:41PM -0800, Guenter Roeck wrote:
> > Hi,
> > 
> > On Thu, Oct 03, 2019 at 03:32:16PM +0300, Sakari Ailus wrote:
> > > Instead of implementing our own means of discovering parent nodes, node
> > > names or counting how many parents a node has, use the newly added
> > > functions in the fwnode API to obtain that information.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > > ---
> > 
> > This patch results in a lockdep splat when running one of my qemu
> > emulations. See below for log and bisect results. A complete log
> > is available at
> > https://kerneltests.org/builders/qemu-arm-master/builds/1408/steps/qemubuildcommand/logs/stdio
> > 
> > Guenter
> 
> Thank you for reporting this.
> 
> I looked into the issue, and indeed I can conform the patch introduces this
> as it takes the devtree_lock for printing the name of the fwnode. There is

I guess that you meant "is not".


> however chance of a deadlock in practice as the code in mm/slub.c does not
> deal with fwnodes (in which case acquiring devtree_lock could be possible),
> maybe for other reasons as well. The patch however introduces an unpleasant
> source of such warnings.

I agree that it is a false positive. alloc/free is called in OF code
under devtree_lock. But OF code is not called from alloc/free (slub.c)
and it should not happen.

lockdep sees the cycle only because the chains are connected via
printk() and logbuf_lock.


> One approach to address this could be not allocating memory while holding
> devtree_lock spinlock. That seems entirely feasible. But could also
> releasing memory cause something to be printed, effectively causing the
> same problem?

I expect that &n->list_lock)->rlock will be needed in kfree() as well.

Anyway, IMHO, allocation outside devtree_lock spinlock would create hairy
and tricky code.

The number of needed "cache_entries" need to be counted under
devtree_lock before the allocation. It means that we would need to
take and release the lock twice. It might create a bunch of possible
races. For example, when new entries are added in the mean time. Or
when this function is called twice in parallel.


> Perhaps a safest way to fix this could be returning to use dn->full_name
> for printing node names, in which case the devtree_lock would no longer be
> taken for printing names. The effect would be though that there would be
> again one more user for the full_name field, information that can be
> reconstructed from the node's parents.

Would this avoid using devtree_lock in all %pO? modifiers?

Removing the lock usage in vsprintf() would make sense
from two reasons:

  + It would allow to use %pOF from inside drivers/of/
    code called under devtree_lock. The current implementation
    would cause a deadlock because of the recursion.

  + There is a huge effort to make printk() lockless. Any lock
    in vsprintf() is just a call for troubles. We probably should
    not have allowed this in the first place.


Finally, this problem will be gone when printk uses a lockless
ringbuffer. I hope that it will happen either for-5.6 or 5.7.
The most tricky part, the ringbuffer itself is in good shape now, see
https://lkml.kernel.org/r/20191128015235.12940-1-john.ogness@linutronix.de

Temporary solution would be to disable lockdep in vsprintf() code.
But I would really prefer to avoid the lock in vsprintf() at all.

Best Regards,
Petr

> > ---
> > ======================================================
> > WARNING: possible circular locking dependency detected
> > 5.5.0-rc4-00066-g738d2902773e #1 Not tainted
> > ------------------------------------------------------
> > swapper/0/1 is trying to acquire lock:
> > c1313b00 (logbuf_lock){-.-.}, at: vprintk_emit+0x68/0x2d4
> > 
> > but task is already holding lock:
> > ef030b90 (&(&n->list_lock)->rlock){..-.}, at: free_debug_processing+0x38/0x418
> > 
> > which lock already depends on the new lock.
> > 
> > 
> > the existing dependency chain (in reverse order) is:
> > 
> > -> #2 (&(&n->list_lock)->rlock){..-.}:
> >        ___slab_alloc.constprop.23+0x12c/0x798
> >        __slab_alloc.constprop.22+0x44/0x70
> >        __kmalloc+0x384/0x41c
> >        of_populate_phandle_cache+0xcc/0x148
> >        of_core_init+0x8/0xbc
> >        driver_init+0x1c/0x2c
> >        kernel_init_freeable+0xac/0x1b4
> >        kernel_init+0x8/0x118
> >        ret_from_fork+0x14/0x20
> >        0x0
> > 
> > -> #1 (devtree_lock){....}:
> >        of_get_parent+0x18/0x34
> >        of_fwnode_get_parent+0x34/0x40
> >        fwnode_count_parents+0x28/0x58
> >        fwnode_full_name_string+0x18/0xa0
> >        device_node_string+0x490/0x4f0
> >        pointer+0x440/0x4d8
> >        vsnprintf+0x1bc/0x3d8
> >        vscnprintf+0xc/0x24
> >        vprintk_store+0x34/0x204
> >        vprintk_emit+0x94/0x2d4
> >        vprintk_default+0x20/0x28
> >        printk+0x30/0x54
> >        exynos4_pm_init_power_domain+0x220/0x258
> >        do_one_initcall+0x8c/0x440
> >        kernel_init_freeable+0x150/0x1b4
> >        kernel_init+0x8/0x118
> >        ret_from_fork+0x14/0x20
> >        0x0
> > 
> > -> #0 (logbuf_lock){-.-.}:
> >        lock_acquire+0xec/0x290
> >        _raw_spin_lock+0x38/0x48
> >        vprintk_emit+0x68/0x2d4
> >        vprintk_default+0x20/0x28
> >        printk+0x30/0x54
> >        unwind_frame+0x6a8/0x6fc
> >        walk_stackframe+0x2c/0x38
> >        __save_stack_trace+0x84/0x8c
> >        stack_trace_save+0x3c/0x5c
> >        set_track+0x40/0x9c
> >        free_debug_processing+0x1a4/0x418
> >        __slab_free+0x2d4/0x510
> >        kmem_cache_free+0x44c/0x49c
> >        rcu_core+0x348/0x994
> >        __do_softirq+0x164/0x668
> >        irq_exit+0x16c/0x170
> >        __handle_domain_irq+0x80/0xec
> >        gic_handle_irq+0x58/0x9c
> >        __irq_svc+0x70/0xb0
> >        raid6_neon8_gen_syndrome_real+0x264/0x39c
> >        raid6_neon8_gen_syndrome_real+0x264/0x39c
> > 
> > other info that might help us debug this:
> > 
> > Chain exists of:
> >   logbuf_lock --> devtree_lock --> &(&n->list_lock)->rlock
> > 
> >  Possible unsafe locking scenario:
> > 
> >        CPU0                    CPU1
> >        ----                    ----
> >   lock(&(&n->list_lock)->rlock);
> >                                lock(devtree_lock);
> >                                lock(&(&n->list_lock)->rlock);
> >   lock(logbuf_lock);
> > 
> >  *** DEADLOCK ***
> > 
> > 2 locks held by swapper/0/1:
> >  #0: c131466c (rcu_callback){....}, at: rcu_core+0x304/0x994
> >  #1: ef030b90 (&(&n->list_lock)->rlock){..-.}, at: free_debug_processing+0x38/0x418
> > 
> > stack backtrace:
> > CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.5.0-rc4-00066-g738d2902773e #1
> > Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> > [<c0113264>] (unwind_backtrace) from [<c010e448>] (show_stack+0x10/0x14)
> > [<c010e448>] (show_stack) from [<c0c9b754>] (dump_stack+0xa4/0xd0)
> > [<c0c9b754>] (dump_stack) from [<c018cbd8>] (check_noncircular+0x258/0x274)
> > [<c018cbd8>] (check_noncircular) from [<c019043c>] (__lock_acquire+0x1870/0x2860)
> > [<c019043c>] (__lock_acquire) from [<c018e088>] (lock_acquire+0xec/0x290)
> > [<c018e088>] (lock_acquire) from [<c0cbf278>] (_raw_spin_lock+0x38/0x48)
> > [<c0cbf278>] (_raw_spin_lock) from [<c01a1f84>] (vprintk_emit+0x68/0x2d4)
> > [<c01a1f84>] (vprintk_emit) from [<c01a2210>] (vprintk_default+0x20/0x28)
> > [<c01a2210>] (vprintk_default) from [<c01a2844>] (printk+0x30/0x54)
> > [<c01a2844>] (printk) from [<c0113210>] (unwind_frame+0x6a8/0x6fc)
> > [<c0113210>] (unwind_frame) from [<c010ddf0>] (walk_stackframe+0x2c/0x38)
> > [<c010ddf0>] (walk_stackframe) from [<c010df54>] (__save_stack_trace+0x84/0x8c)
> > [<c010df54>] (__save_stack_trace) from [<c01c2d40>] (stack_trace_save+0x3c/0x5c)
> > [<c01c2d40>] (stack_trace_save) from [<c02ae258>] (set_track+0x40/0x9c)
> > [<c02ae258>] (set_track) from [<c02b06a8>] (free_debug_processing+0x1a4/0x418)
> > [<c02b06a8>] (free_debug_processing) from [<c02b0bf0>] (__slab_free+0x2d4/0x510)
> > [<c02b0bf0>] (__slab_free) from [<c02b17ac>] (kmem_cache_free+0x44c/0x49c)
> > [<c02b17ac>] (kmem_cache_free) from [<c01bd608>] (rcu_core+0x348/0x994)
> > [<c01bd608>] (rcu_core) from [<c010230c>] (__do_softirq+0x164/0x668)
> > [<c010230c>] (__do_softirq) from [<c0131310>] (irq_exit+0x16c/0x170)
> > [<c0131310>] (irq_exit) from [<c01a3740>] (__handle_domain_irq+0x80/0xec)
> > [<c01a3740>] (__handle_domain_irq) from [<c0630828>] (gic_handle_irq+0x58/0x9c)
> > [<c0630828>] (gic_handle_irq) from [<c0101a70>] (__irq_svc+0x70/0xb0)
> > Exception stack(0xef19bd30 to 0xef19bd78)
> > bd20:                                     c0d43e50 c0d43e60 ef19bebc c0d43e70
> > bd40: c0d43e20 c0d43e10 00000400 00000430 00000440 00000450 00000460 00000470
> > bd60: c0d43e40 ef19bd80 c0d43e30 c0625c00 20000013 ffffffff
> > [<c0101a70>] (__irq_svc) from [<c0625c00>] (raid6_neon8_gen_syndrome_real+0x264/0x39c)
> > 
> > ---
> > Bisect:
> > 
> > # bad: [738d2902773e30939a982c8df7a7f94293659810] Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> > # good: [219d54332a09e8d8741c1e1982f5eae56099de85] Linux 5.4
> > git bisect start 'HEAD' 'v5.4'
> > # bad: [8c39f71ee2019e77ee14f88b1321b2348db51820] Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> > git bisect bad 8c39f71ee2019e77ee14f88b1321b2348db51820
> > # good: [3b397c7ccafe0624018cb09fc96729f8f6165573] Merge tag 'regmap-v5.5' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap
> > git bisect good 3b397c7ccafe0624018cb09fc96729f8f6165573
> > # bad: [89d57dddd7d319ded00415790a0bb3c954b7e386] Merge tag 'media/v5.5-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
> > git bisect bad 89d57dddd7d319ded00415790a0bb3c954b7e386
> > # good: [9e7a03233e02afd3ee061e373355f34d7254f1e6] Merge tag 'pm-5.5-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
> > git bisect good 9e7a03233e02afd3ee061e373355f34d7254f1e6
> > # good: [09578eacaaa44149738267083ccc050990409f86] Merge tag 'asoc-v5.5-2' of https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound into for-linus
> > git bisect good 09578eacaaa44149738267083ccc050990409f86
> > # good: [1c7ae4a51298d52a21f63b2214657982036c7498] media: ad5820: Add support for of-autoload
> > git bisect good 1c7ae4a51298d52a21f63b2214657982036c7498
> > # good: [99cf8a7074c4ce3ff3685cd389f54e7bd4bbf510] media: dt-bindings: Fix building error for dt_binding_check
> > git bisect good 99cf8a7074c4ce3ff3685cd389f54e7bd4bbf510
> > # good: [782b59711e1561ee0da06bc478ca5e8249aa8d09] Merge branch 'acpi-mm'
> > git bisect good 782b59711e1561ee0da06bc478ca5e8249aa8d09
> > # good: [0ca40f41d795fd91811e44506bb73d0b9ca33bdd] Merge branch 'patchwork' into v4l_for_linus
> > git bisect good 0ca40f41d795fd91811e44506bb73d0b9ca33bdd
> > # bad: [a00351687f8a05773c1c57be80a5bbca68fa9ae8] software node: remove DEV_PROP_MAX
> > git bisect bad a00351687f8a05773c1c57be80a5bbca68fa9ae8
> > # good: [9af7706492f985867d070861fe39fee0fe41326f] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps
> > git bisect good 9af7706492f985867d070861fe39fee0fe41326f
> > # bad: [83abc5a77f3b028b8c845c39ce4053119e1de35b] lib/vsprintf: OF nodes are first and foremost, struct device_nodes
> > git bisect bad 83abc5a77f3b028b8c845c39ce4053119e1de35b
> > # bad: [a92eb7621b9fb2c28a588ce333d226f56fab6a85] lib/vsprintf: Make use of fwnode API to obtain node names and separators
> > git bisect bad a92eb7621b9fb2c28a588ce333d226f56fab6a85
> > # good: [1586c5ae2f9310235b5e70abe712c73fc32eb98f] lib/vsprintf: Add a note on re-using %pf or %pF
> > git bisect good 1586c5ae2f9310235b5e70abe712c73fc32eb98f
> > # first bad commit: [a92eb7621b9fb2c28a588ce333d226f56fab6a85] lib/vsprintf: Make use of fwnode API to obtain node names and separators
> 
> -- 
> Kind regards,
> 
> Sakari Ailus

Re: [PATCH v9 09/12] lib/vsprintf: Make use of fwnode API to obtain node names and separators

[Guenter Roeck]

On Fri, Jan 03, 2020 at 03:42:53PM +0100, Petr Mladek wrote:
> On Fri 2020-01-03 13:21:45, Sakari Ailus wrote:
> > Hi Guenter,
> > 
> > On Thu, Jan 02, 2020 at 02:20:41PM -0800, Guenter Roeck wrote:
> > > Hi,
> > > 
> > > On Thu, Oct 03, 2019 at 03:32:16PM +0300, Sakari Ailus wrote:
> > > > Instead of implementing our own means of discovering parent nodes, node
> > > > names or counting how many parents a node has, use the newly added
> > > > functions in the fwnode API to obtain that information.
> > > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > > > ---
> > > 
> > > This patch results in a lockdep splat when running one of my qemu
> > > emulations. See below for log and bisect results. A complete log
> > > is available at
> > > https://kerneltests.org/builders/qemu-arm-master/builds/1408/steps/qemubuildcommand/logs/stdio
> > > 
> > > Guenter
> > 
> > Thank you for reporting this.
> > 
> > I looked into the issue, and indeed I can conform the patch introduces this
> > as it takes the devtree_lock for printing the name of the fwnode. There is
> 
> I guess that you meant "is not".
> 
> 
> > however chance of a deadlock in practice as the code in mm/slub.c does not
> > deal with fwnodes (in which case acquiring devtree_lock could be possible),
> > maybe for other reasons as well. The patch however introduces an unpleasant
> > source of such warnings.
> 
> I agree that it is a false positive. alloc/free is called in OF code
> under devtree_lock. But OF code is not called from alloc/free (slub.c)
> and it should not happen.
> 

Assuming that memory allocation is indeed called from code holding
devtree_lock: The problem, as I see it, is that the order of acquiring
locks is different. In OF code, the order is
	devtree_lock
	(&n->list_lock)->rlock

Elsewhere, in %pOF print sequences, it is
	(&n->list_lock)->rlock
	devtree_lock

The OF code, while holding devtree_lock, may try to allocate or release
memory and is waiting for (&n->list_lock)->rlock. At the same time, some
other thread may try to print %pOF, has acquired (&n->list_lock)->rlock,
and is waiting for devtree_lock.

Are you sure that this can not happen ?

Thanks,
Guenter

> lockdep sees the cycle only because the chains are connected via
> printk() and logbuf_lock.
> 
> 
> > One approach to address this could be not allocating memory while holding
> > devtree_lock spinlock. That seems entirely feasible. But could also
> > releasing memory cause something to be printed, effectively causing the
> > same problem?
> 
> I expect that &n->list_lock)->rlock will be needed in kfree() as well.
> 
> Anyway, IMHO, allocation outside devtree_lock spinlock would create hairy
> and tricky code.
> 
> The number of needed "cache_entries" need to be counted under
> devtree_lock before the allocation. It means that we would need to
> take and release the lock twice. It might create a bunch of possible
> races. For example, when new entries are added in the mean time. Or
> when this function is called twice in parallel.
> 
> 
> > Perhaps a safest way to fix this could be returning to use dn->full_name
> > for printing node names, in which case the devtree_lock would no longer be
> > taken for printing names. The effect would be though that there would be
> > again one more user for the full_name field, information that can be
> > reconstructed from the node's parents.
> 
> Would this avoid using devtree_lock in all %pO? modifiers?
> 
> Removing the lock usage in vsprintf() would make sense
> from two reasons:
> 
>   + It would allow to use %pOF from inside drivers/of/
>     code called under devtree_lock. The current implementation
>     would cause a deadlock because of the recursion.
> 
>   + There is a huge effort to make printk() lockless. Any lock
>     in vsprintf() is just a call for troubles. We probably should
>     not have allowed this in the first place.
> 
> 
> Finally, this problem will be gone when printk uses a lockless
> ringbuffer. I hope that it will happen either for-5.6 or 5.7.
> The most tricky part, the ringbuffer itself is in good shape now, see
> https://lkml.kernel.org/r/20191128015235.12940-1-john.ogness@linutronix.de
> 
> Temporary solution would be to disable lockdep in vsprintf() code.
> But I would really prefer to avoid the lock in vsprintf() at all.
> 
> Best Regards,
> Petr
> 
> > > ---
> > > ======================================================
> > > WARNING: possible circular locking dependency detected
> > > 5.5.0-rc4-00066-g738d2902773e #1 Not tainted
> > > ------------------------------------------------------
> > > swapper/0/1 is trying to acquire lock:
> > > c1313b00 (logbuf_lock){-.-.}, at: vprintk_emit+0x68/0x2d4
> > > 
> > > but task is already holding lock:
> > > ef030b90 (&(&n->list_lock)->rlock){..-.}, at: free_debug_processing+0x38/0x418
> > > 
> > > which lock already depends on the new lock.
> > > 
> > > 
> > > the existing dependency chain (in reverse order) is:
> > > 
> > > -> #2 (&(&n->list_lock)->rlock){..-.}:
> > >        ___slab_alloc.constprop.23+0x12c/0x798
> > >        __slab_alloc.constprop.22+0x44/0x70
> > >        __kmalloc+0x384/0x41c
> > >        of_populate_phandle_cache+0xcc/0x148
> > >        of_core_init+0x8/0xbc
> > >        driver_init+0x1c/0x2c
> > >        kernel_init_freeable+0xac/0x1b4
> > >        kernel_init+0x8/0x118
> > >        ret_from_fork+0x14/0x20
> > >        0x0
> > > 
> > > -> #1 (devtree_lock){....}:
> > >        of_get_parent+0x18/0x34
> > >        of_fwnode_get_parent+0x34/0x40
> > >        fwnode_count_parents+0x28/0x58
> > >        fwnode_full_name_string+0x18/0xa0
> > >        device_node_string+0x490/0x4f0
> > >        pointer+0x440/0x4d8
> > >        vsnprintf+0x1bc/0x3d8
> > >        vscnprintf+0xc/0x24
> > >        vprintk_store+0x34/0x204
> > >        vprintk_emit+0x94/0x2d4
> > >        vprintk_default+0x20/0x28
> > >        printk+0x30/0x54
> > >        exynos4_pm_init_power_domain+0x220/0x258
> > >        do_one_initcall+0x8c/0x440
> > >        kernel_init_freeable+0x150/0x1b4
> > >        kernel_init+0x8/0x118
> > >        ret_from_fork+0x14/0x20
> > >        0x0
> > > 
> > > -> #0 (logbuf_lock){-.-.}:
> > >        lock_acquire+0xec/0x290
> > >        _raw_spin_lock+0x38/0x48
> > >        vprintk_emit+0x68/0x2d4
> > >        vprintk_default+0x20/0x28
> > >        printk+0x30/0x54
> > >        unwind_frame+0x6a8/0x6fc
> > >        walk_stackframe+0x2c/0x38
> > >        __save_stack_trace+0x84/0x8c
> > >        stack_trace_save+0x3c/0x5c
> > >        set_track+0x40/0x9c
> > >        free_debug_processing+0x1a4/0x418
> > >        __slab_free+0x2d4/0x510
> > >        kmem_cache_free+0x44c/0x49c
> > >        rcu_core+0x348/0x994
> > >        __do_softirq+0x164/0x668
> > >        irq_exit+0x16c/0x170
> > >        __handle_domain_irq+0x80/0xec
> > >        gic_handle_irq+0x58/0x9c
> > >        __irq_svc+0x70/0xb0
> > >        raid6_neon8_gen_syndrome_real+0x264/0x39c
> > >        raid6_neon8_gen_syndrome_real+0x264/0x39c
> > > 
> > > other info that might help us debug this:
> > > 
> > > Chain exists of:
> > >   logbuf_lock --> devtree_lock --> &(&n->list_lock)->rlock
> > > 
> > >  Possible unsafe locking scenario:
> > > 
> > >        CPU0                    CPU1
> > >        ----                    ----
> > >   lock(&(&n->list_lock)->rlock);
> > >                                lock(devtree_lock);
> > >                                lock(&(&n->list_lock)->rlock);
> > >   lock(logbuf_lock);
> > > 
> > >  *** DEADLOCK ***
> > > 
> > > 2 locks held by swapper/0/1:
> > >  #0: c131466c (rcu_callback){....}, at: rcu_core+0x304/0x994
> > >  #1: ef030b90 (&(&n->list_lock)->rlock){..-.}, at: free_debug_processing+0x38/0x418
> > > 
> > > stack backtrace:
> > > CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.5.0-rc4-00066-g738d2902773e #1
> > > Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
> > > [<c0113264>] (unwind_backtrace) from [<c010e448>] (show_stack+0x10/0x14)
> > > [<c010e448>] (show_stack) from [<c0c9b754>] (dump_stack+0xa4/0xd0)
> > > [<c0c9b754>] (dump_stack) from [<c018cbd8>] (check_noncircular+0x258/0x274)
> > > [<c018cbd8>] (check_noncircular) from [<c019043c>] (__lock_acquire+0x1870/0x2860)
> > > [<c019043c>] (__lock_acquire) from [<c018e088>] (lock_acquire+0xec/0x290)
> > > [<c018e088>] (lock_acquire) from [<c0cbf278>] (_raw_spin_lock+0x38/0x48)
> > > [<c0cbf278>] (_raw_spin_lock) from [<c01a1f84>] (vprintk_emit+0x68/0x2d4)
> > > [<c01a1f84>] (vprintk_emit) from [<c01a2210>] (vprintk_default+0x20/0x28)
> > > [<c01a2210>] (vprintk_default) from [<c01a2844>] (printk+0x30/0x54)
> > > [<c01a2844>] (printk) from [<c0113210>] (unwind_frame+0x6a8/0x6fc)
> > > [<c0113210>] (unwind_frame) from [<c010ddf0>] (walk_stackframe+0x2c/0x38)
> > > [<c010ddf0>] (walk_stackframe) from [<c010df54>] (__save_stack_trace+0x84/0x8c)
> > > [<c010df54>] (__save_stack_trace) from [<c01c2d40>] (stack_trace_save+0x3c/0x5c)
> > > [<c01c2d40>] (stack_trace_save) from [<c02ae258>] (set_track+0x40/0x9c)
> > > [<c02ae258>] (set_track) from [<c02b06a8>] (free_debug_processing+0x1a4/0x418)
> > > [<c02b06a8>] (free_debug_processing) from [<c02b0bf0>] (__slab_free+0x2d4/0x510)
> > > [<c02b0bf0>] (__slab_free) from [<c02b17ac>] (kmem_cache_free+0x44c/0x49c)
> > > [<c02b17ac>] (kmem_cache_free) from [<c01bd608>] (rcu_core+0x348/0x994)
> > > [<c01bd608>] (rcu_core) from [<c010230c>] (__do_softirq+0x164/0x668)
> > > [<c010230c>] (__do_softirq) from [<c0131310>] (irq_exit+0x16c/0x170)
> > > [<c0131310>] (irq_exit) from [<c01a3740>] (__handle_domain_irq+0x80/0xec)
> > > [<c01a3740>] (__handle_domain_irq) from [<c0630828>] (gic_handle_irq+0x58/0x9c)
> > > [<c0630828>] (gic_handle_irq) from [<c0101a70>] (__irq_svc+0x70/0xb0)
> > > Exception stack(0xef19bd30 to 0xef19bd78)
> > > bd20:                                     c0d43e50 c0d43e60 ef19bebc c0d43e70
> > > bd40: c0d43e20 c0d43e10 00000400 00000430 00000440 00000450 00000460 00000470
> > > bd60: c0d43e40 ef19bd80 c0d43e30 c0625c00 20000013 ffffffff
> > > [<c0101a70>] (__irq_svc) from [<c0625c00>] (raid6_neon8_gen_syndrome_real+0x264/0x39c)
> > > 
> > > ---
> > > Bisect:
> > > 
> > > # bad: [738d2902773e30939a982c8df7a7f94293659810] Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> > > # good: [219d54332a09e8d8741c1e1982f5eae56099de85] Linux 5.4
> > > git bisect start 'HEAD' 'v5.4'
> > > # bad: [8c39f71ee2019e77ee14f88b1321b2348db51820] Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
> > > git bisect bad 8c39f71ee2019e77ee14f88b1321b2348db51820
> > > # good: [3b397c7ccafe0624018cb09fc96729f8f6165573] Merge tag 'regmap-v5.5' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap
> > > git bisect good 3b397c7ccafe0624018cb09fc96729f8f6165573
> > > # bad: [89d57dddd7d319ded00415790a0bb3c954b7e386] Merge tag 'media/v5.5-1' of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
> > > git bisect bad 89d57dddd7d319ded00415790a0bb3c954b7e386
> > > # good: [9e7a03233e02afd3ee061e373355f34d7254f1e6] Merge tag 'pm-5.5-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm
> > > git bisect good 9e7a03233e02afd3ee061e373355f34d7254f1e6
> > > # good: [09578eacaaa44149738267083ccc050990409f86] Merge tag 'asoc-v5.5-2' of https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound into for-linus
> > > git bisect good 09578eacaaa44149738267083ccc050990409f86
> > > # good: [1c7ae4a51298d52a21f63b2214657982036c7498] media: ad5820: Add support for of-autoload
> > > git bisect good 1c7ae4a51298d52a21f63b2214657982036c7498
> > > # good: [99cf8a7074c4ce3ff3685cd389f54e7bd4bbf510] media: dt-bindings: Fix building error for dt_binding_check
> > > git bisect good 99cf8a7074c4ce3ff3685cd389f54e7bd4bbf510
> > > # good: [782b59711e1561ee0da06bc478ca5e8249aa8d09] Merge branch 'acpi-mm'
> > > git bisect good 782b59711e1561ee0da06bc478ca5e8249aa8d09
> > > # good: [0ca40f41d795fd91811e44506bb73d0b9ca33bdd] Merge branch 'patchwork' into v4l_for_linus
> > > git bisect good 0ca40f41d795fd91811e44506bb73d0b9ca33bdd
> > > # bad: [a00351687f8a05773c1c57be80a5bbca68fa9ae8] software node: remove DEV_PROP_MAX
> > > git bisect bad a00351687f8a05773c1c57be80a5bbca68fa9ae8
> > > # good: [9af7706492f985867d070861fe39fee0fe41326f] lib/vsprintf: Remove support for %pF and %pf in favour of %pS and %ps
> > > git bisect good 9af7706492f985867d070861fe39fee0fe41326f
> > > # bad: [83abc5a77f3b028b8c845c39ce4053119e1de35b] lib/vsprintf: OF nodes are first and foremost, struct device_nodes
> > > git bisect bad 83abc5a77f3b028b8c845c39ce4053119e1de35b
> > > # bad: [a92eb7621b9fb2c28a588ce333d226f56fab6a85] lib/vsprintf: Make use of fwnode API to obtain node names and separators
> > > git bisect bad a92eb7621b9fb2c28a588ce333d226f56fab6a85
> > > # good: [1586c5ae2f9310235b5e70abe712c73fc32eb98f] lib/vsprintf: Add a note on re-using %pf or %pF
> > > git bisect good 1586c5ae2f9310235b5e70abe712c73fc32eb98f
> > > # first bad commit: [a92eb7621b9fb2c28a588ce333d226f56fab6a85] lib/vsprintf: Make use of fwnode API to obtain node names and separators
> > 
> > -- 
> > Kind regards,
> > 
> > Sakari Ailus

Re: [PATCH v9 09/12] lib/vsprintf: Make use of fwnode API to obtain node names and separators

[Petr Mladek]

On Fri 2020-01-03 10:35:55, Guenter Roeck wrote:
> On Fri, Jan 03, 2020 at 03:42:53PM +0100, Petr Mladek wrote:
> > On Fri 2020-01-03 13:21:45, Sakari Ailus wrote:
> > > Hi Guenter,
> > > 
> > > On Thu, Jan 02, 2020 at 02:20:41PM -0800, Guenter Roeck wrote:
> > > > Hi,
> > > > 
> > > > On Thu, Oct 03, 2019 at 03:32:16PM +0300, Sakari Ailus wrote:
> > > > > Instead of implementing our own means of discovering parent nodes, node
> > > > > names or counting how many parents a node has, use the newly added
> > > > > functions in the fwnode API to obtain that information.
> > > > > 
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > > > > ---
> > > > 
> > > > This patch results in a lockdep splat when running one of my qemu
> > > > emulations. See below for log and bisect results. A complete log
> > > > is available at
> > > > https://kerneltests.org/builders/qemu-arm-master/builds/1408/steps/qemubuildcommand/logs/stdio
> > > > 
> > > > Guenter
> > > 
> > > Thank you for reporting this.
> > > 
> > > I looked into the issue, and indeed I can conform the patch introduces this
> > > as it takes the devtree_lock for printing the name of the fwnode. There is
> > 
> > I guess that you meant "is not".
> > 
> > 
> > > however chance of a deadlock in practice as the code in mm/slub.c does not
> > > deal with fwnodes (in which case acquiring devtree_lock could be possible),
> > > maybe for other reasons as well. The patch however introduces an unpleasant
> > > source of such warnings.
> > 
> > I agree that it is a false positive. alloc/free is called in OF code
> > under devtree_lock. But OF code is not called from alloc/free (slub.c)
> > and it should not happen.
> > 
> 
> Assuming that memory allocation is indeed called from code holding
> devtree_lock: The problem, as I see it, is that the order of acquiring
> locks is different. In OF code, the order is
> 	devtree_lock
> 	(&n->list_lock)->rlock

Yes, this happens when alloc is called in OF code under devtree_lock.

> Elsewhere, in %pOF print sequences, it is
> 	(&n->list_lock)->rlock
> 	devtree_lock

I believe that this order does not exist in reality. lockep "just"
connected this the two locks via logbuf_lock. When printk() is
called in the allocator:

	(&n->list_lock)->rlock
	logbuf_lock

and when %pOF is used in printk():

	logbuf_lock
	devtree_lock

From this two lockdep assumes that it might be possible to
use %pOF in printk() from allocator code:

	(&n->list_lock)->rlock
	logbuf_lock
	devtree_lock

But I believe that this does not make sense and never happen reality.

That said, I would still prefer when %pOF could be implemented
without the lock. It would make it usable anywhere without any
risk of deadlock.

Sakari, what is your opinion, please?

Best Regards,
Petr

Re: [PATCH v9 09/12] lib/vsprintf: Make use of fwnode API to obtain node names and separators

[Sakari Ailus]

Hi Petr,

On Mon, Jan 13, 2020 at 10:17:51AM +0100, Petr Mladek wrote:
> On Fri 2020-01-03 10:35:55, Guenter Roeck wrote:
> > On Fri, Jan 03, 2020 at 03:42:53PM +0100, Petr Mladek wrote:
> > > On Fri 2020-01-03 13:21:45, Sakari Ailus wrote:
> > > > Hi Guenter,
> > > > 
> > > > On Thu, Jan 02, 2020 at 02:20:41PM -0800, Guenter Roeck wrote:
> > > > > Hi,
> > > > > 
> > > > > On Thu, Oct 03, 2019 at 03:32:16PM +0300, Sakari Ailus wrote:
> > > > > > Instead of implementing our own means of discovering parent nodes, node
> > > > > > names or counting how many parents a node has, use the newly added
> > > > > > functions in the fwnode API to obtain that information.
> > > > > > 
> > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > > > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > > > > > ---
> > > > > 
> > > > > This patch results in a lockdep splat when running one of my qemu
> > > > > emulations. See below for log and bisect results. A complete log
> > > > > is available at
> > > > > https://kerneltests.org/builders/qemu-arm-master/builds/1408/steps/qemubuildcommand/logs/stdio
> > > > > 
> > > > > Guenter
> > > > 
> > > > Thank you for reporting this.
> > > > 
> > > > I looked into the issue, and indeed I can conform the patch introduces this
> > > > as it takes the devtree_lock for printing the name of the fwnode. There is
> > > 
> > > I guess that you meant "is not".

Right, that's what I meant. Sometimes small words can make a big
difference. :-)

> > > 
> > > 
> > > > however chance of a deadlock in practice as the code in mm/slub.c does not
> > > > deal with fwnodes (in which case acquiring devtree_lock could be possible),
> > > > maybe for other reasons as well. The patch however introduces an unpleasant
> > > > source of such warnings.
> > > 
> > > I agree that it is a false positive. alloc/free is called in OF code
> > > under devtree_lock. But OF code is not called from alloc/free (slub.c)
> > > and it should not happen.
> > > 
> > 
> > Assuming that memory allocation is indeed called from code holding
> > devtree_lock: The problem, as I see it, is that the order of acquiring
> > locks is different. In OF code, the order is
> > 	devtree_lock
> > 	(&n->list_lock)->rlock
> 
> Yes, this happens when alloc is called in OF code under devtree_lock.
> 
> > Elsewhere, in %pOF print sequences, it is
> > 	(&n->list_lock)->rlock
> > 	devtree_lock
> 
> I believe that this order does not exist in reality. lockep "just"
> connected this the two locks via logbuf_lock. When printk() is
> called in the allocator:
> 
> 	(&n->list_lock)->rlock
> 	logbuf_lock
> 
> and when %pOF is used in printk():
> 
> 	logbuf_lock
> 	devtree_lock
> 
> From this two lockdep assumes that it might be possible to
> use %pOF in printk() from allocator code:
> 
> 	(&n->list_lock)->rlock
> 	logbuf_lock
> 	devtree_lock
> 
> But I believe that this does not make sense and never happen reality.
> 
> That said, I would still prefer when %pOF could be implemented
> without the lock. It would make it usable anywhere without any
> risk of deadlock.
> 
> Sakari, what is your opinion, please?

The reason taking that lock is to be able to traverse the device tree data
structure to find the names of the nodes upto the root node. This happens
only when the full node name is printed.

Traversing the tree takes place through the fwnode framework, and currently
the framework uses only the name of the fwnode itself to print the full
path. It *could* use the full name directly, but that way removing the
full_name field (taking up some memory right now) would not be possible.
That said, I don't know if there are plans to do so. A quick look at one
system tells the size of this memory is around 20 kB.

ACPI does not use such locks in traversing to the tree, but it might do
that in the future, and avoiding the lock there would also require copying
the full node name to each node in the system.

If there are plans to avoid having logbuf_lock, then the problem disappears
with that.

How about disabling the lockdep warning now, and if it seems we're not
getting rid of the logbuf_lock in a reasonable timeframe, then look at the
alternatives, such as using the full_name in printing node names instead?

-- 
Kind regards,

Sakari Ailus