[PATCH v4 00/11] Device property improvements, add %pfw format specifier

[PATCH v4 00/11] 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.

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 (11):
  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: 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                     | 55 +++++++++++++-
 drivers/of/property.c                     | 16 +++++
 include/linux/fwnode.h                    |  4 ++
 include/linux/property.h                  |  8 ++-
 lib/test_printf.c                         | 37 ++++++++++
 lib/vsprintf.c                            | 88 ++++++++++++++---------
 scripts/checkpatch.pl                     |  4 +-
 10 files changed, 319 insertions(+), 58 deletions(-)

-- 
2.20.1

[PATCH v4 01/11] 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>
---
 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 v4 02/11] 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 v4 03/11] 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>
---
 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 v4 04/11] 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>
---
 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..d2461d79139f3 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 @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 v4 05/11] 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    | 20 ++++++++++++++++++++
 drivers/of/property.c    |  6 ++++++
 include/linux/fwnode.h   |  2 ++
 include/linux/property.h |  1 +
 6 files changed, 66 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 d2461d79139f3..0ce5052a7f9b7 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..a4a0f5b80bad3 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -515,6 +515,25 @@ 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 software_node *softnode = to_software_node(fwnode);
+	const struct swnode *swnode = software_node_to_swnode(softnode);
+	struct fwnode_handle *parent;
+
+	if (!swnode)
+		return "(null)";
+
+	parent = fwnode_get_parent(&swnode->fwnode);
+	if (!parent)
+		return "";
+
+	fwnode_handle_put(parent);
+
+	return kobject_name(&swnode->kobj);
+}
+
 static struct fwnode_handle *
 software_node_get_parent(const struct fwnode_handle *fwnode)
 {
@@ -615,6 +634,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 v4 06/11] 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)
---
 drivers/acpi/property.c  | 22 ++++++++++++++++++++++
 drivers/base/property.c  | 12 ++++++++++++
 drivers/base/swnode.c    | 26 ++++++++++++++++++++++++++
 drivers/of/property.c    | 10 ++++++++++
 include/linux/fwnode.h   |  2 ++
 include/linux/property.h |  1 +
 6 files changed, 73 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 0ce5052a7f9b7..9563db780e039 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 a4a0f5b80bad3..9cc17baa193fc 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -534,6 +534,31 @@ 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 "";
+
+	parent = fwnode_get_next_parent(parent);
+	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)
 {
@@ -635,6 +660,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 v4 07/11] 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.

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                            |  8 ++------
 scripts/checkpatch.pl                     |  1 -
 3 files changed, 2 insertions(+), 17 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..b00b57f9f911f 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);
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 v4 08/11] 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>
---
 lib/vsprintf.c | 38 ++++++++++++++++----------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index b00b57f9f911f..a04a2167101ef 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,24 @@ 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);
+	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 +1926,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 +1940,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 v4 09/11] 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>
---
 lib/vsprintf.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index a04a2167101ef..4ad9332d54ba6 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1905,6 +1905,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);
 
@@ -1978,17 +1981,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
@@ -2167,7 +2159,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 v4 10/11] 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>
---
 Documentation/core-api/printk-formats.rst | 24 +++++++++++++++
 lib/vsprintf.c                            | 36 +++++++++++++++++++++++
 scripts/checkpatch.pl                     |  3 +-
 3 files changed, 62 insertions(+), 1 deletion(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 922a29eb70e6c..abba210f67567 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 4ad9332d54ba6..b9b4c835db063 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1981,6 +1981,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, "(%pfw?)", spec);
+
+	if (check_pointer(&buf, end, fwnode, spec))
+		return buf;
+
+	fmt++;
+
+	switch (*fmt) {
+	case 'f':	/* full_name */
+	default:
+		buf = fwnode_full_name_string(fwnode, buf, end);
+		break;
+	case 'P':	/* name */
+		buf = string(buf, end, fwnode_get_name(fwnode), str_spec);
+		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
@@ -2083,6 +2113,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:
@@ -2160,6 +2194,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..8df50911ff4e9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5995,7 +5995,8 @@ sub process {
 				while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) {
 					$specifier = $1;
 					$extension = $2;
-					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxt]/) {
+					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxtf]/ ||
+					    $extension =~ /^f[^w]/) {
 						$bad_specifier = $specifier;
 						last;
 					}
-- 
2.20.1

[PATCH v4 11/11] 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>
---
 lib/test_printf.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/lib/test_printf.c b/lib/test_printf.c
index 944eb50f38625..9c6d716979fb1 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,40 @@ 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 = "/second/third";
+	const char * const full_name_second = "/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[ARRAY_SIZE(softnodes) - 3]));
+	test(full_name, "%pfw",
+	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2]));
+	test(full_name, "%pfwf",
+	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2]));
+	test(second_name, "%pfwP",
+	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 3]));
+	test(third_name, "%pfwP",
+	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2]));
+
+	software_node_unregister_nodes(softnodes);
+}
+
 static void __init
 test_pointer(void)
 {
@@ -610,6 +646,7 @@ test_pointer(void)
 	bitmap();
 	netdev_features();
 	flags();
+	fwnode_pointer();
 }
 
 static void __init selftest(void)
-- 
2.20.1

Re: [PATCH v4 01/11] software node: Get reference to parent swnode in get_parent op

[Andy Shevchenko]

On Mon, Sep 02, 2019 at 11:32:30AM +0300, Sakari Ailus wrote:
> 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.
> 

Missed already given

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 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>
> ---
>  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
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 03/11] device property: Move fwnode_get_parent() up

[Andy Shevchenko]

On Mon, Sep 02, 2019 at 11:32:32AM +0300, Sakari Ailus wrote:
> Move fwnode_get_parent() above fwnode_get_next_parent(), making the order
> the same as in the header file.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Sakari Ailus <sakari.ailus@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
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 04/11] device property: Add functions for accessing node's parents

[Andy Shevchenko]

On Mon, Sep 02, 2019 at 11:32:33AM +0300, Sakari Ailus wrote:
> 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.
> 

Much better now, thanks!

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

though one question below.

> Signed-off-by: Sakari Ailus <sakari.ailus@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..d2461d79139f3 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 @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);
> +

If 'fnode == NULL' and 'i < depth', shan't we return some kind of error?

> +	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
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 06/11] device property: Add a function to obtain a node's prefix

[Andy Shevchenko]

On Mon, Sep 02, 2019 at 11:32:35AM +0300, Sakari Ailus wrote:
> The prefix is used for printing purpose before a node, and it also works
> as a separator between two nodes.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Rob Herring <robh@kernel.org> (for OF)
> ---
>  drivers/acpi/property.c  | 22 ++++++++++++++++++++++
>  drivers/base/property.c  | 12 ++++++++++++
>  drivers/base/swnode.c    | 26 ++++++++++++++++++++++++++
>  drivers/of/property.c    | 10 ++++++++++
>  include/linux/fwnode.h   |  2 ++
>  include/linux/property.h |  1 +
>  6 files changed, 73 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 0ce5052a7f9b7..9563db780e039 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 a4a0f5b80bad3..9cc17baa193fc 100644
> --- a/drivers/base/swnode.c
> +++ b/drivers/base/swnode.c
> @@ -534,6 +534,31 @@ 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 "";
> +
> +	parent = fwnode_get_next_parent(parent);
> +	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)
>  {
> @@ -635,6 +660,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
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 09/11] lib/vsprintf: OF nodes are first and foremost, struct device_nodes

[Andy Shevchenko]

On Mon, Sep 02, 2019 at 11:32:38AM +0300, Sakari Ailus wrote:
> Factor out static kobject_string() function that simply calls
> device_node_string(), and thus remove references to kobjects (as these are
> struct device_node).
> 

FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  lib/vsprintf.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index a04a2167101ef..4ad9332d54ba6 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1905,6 +1905,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);
>  
> @@ -1978,17 +1981,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
> @@ -2167,7 +2159,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
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 11/11] lib/test_printf: Add tests for %pfw printk modifier

[Andy Shevchenko]

On Mon, Sep 02, 2019 at 11:32:40AM +0300, Sakari Ailus wrote:
> Add a test for the %pfw printk modifier using software nodes.

> +	const struct software_node softnodes[] = {
> +		{ .name = "first", },
> +		{ .name = "second", .parent = &softnodes[0], },
> +		{ .name = "third", .parent = &softnodes[1], },
> +		{ NULL /* Guardian */ },
> +	};

> +	test(full_name_second, "%pfw",
> +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 3]));
> +	test(full_name, "%pfw",
> +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2]));
> +	test(full_name, "%pfwf",
> +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2]));
> +	test(second_name, "%pfwP",
> +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 3]));
> +	test(third_name, "%pfwP",
> +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2]));


These can be shorted and easier to parse if you use absolute indexes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 04/11] device property: Add functions for accessing node's parents

[Sakari Ailus]

Hi Andy,

On Mon, Sep 02, 2019 at 01:14:26PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 02, 2019 at 11:32:33AM +0300, Sakari Ailus wrote:
> > 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.
> > 
> 
> Much better now, thanks!
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> though one question below.

Thanks!

...

> > +/**
> > + * 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 @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);
> > +
> 
> If 'fnode == NULL' and 'i < depth', shan't we return some kind of error?

How about adding to the comment, after the first sentence of the
description:

	If there is no parent at the requested depth, NULL is returned.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v4 04/11] device property: Add functions for accessing node's parents

[Andy Shevchenko]

On Mon, Sep 02, 2019 at 03:34:31PM +0300, Sakari Ailus wrote:
> Hi Andy,
> 
> On Mon, Sep 02, 2019 at 01:14:26PM +0300, Andy Shevchenko wrote:
> > On Mon, Sep 02, 2019 at 11:32:33AM +0300, Sakari Ailus wrote:
> > > 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.
> > > 
> > 
> > Much better now, thanks!
> > 
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > though one question below.

> > > +/**
> > > + * 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 @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);
> > > +
> > 
> > If 'fnode == NULL' and 'i < depth', shan't we return some kind of error?
> 
> How about adding to the comment, after the first sentence of the
> description:
> 
> 	If there is no parent at the requested depth, NULL is returned.

Works for me!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 11/11] lib/test_printf: Add tests for %pfw printk modifier

[Sakari Ailus]

On Mon, Sep 02, 2019 at 03:26:27PM +0300, Andy Shevchenko wrote:
> On Mon, Sep 02, 2019 at 11:32:40AM +0300, Sakari Ailus wrote:
> > Add a test for the %pfw printk modifier using software nodes.
> 
> > +	const struct software_node softnodes[] = {
> > +		{ .name = "first", },
> > +		{ .name = "second", .parent = &softnodes[0], },
> > +		{ .name = "third", .parent = &softnodes[1], },
> > +		{ NULL /* Guardian */ },
> > +	};
> 
> > +	test(full_name_second, "%pfw",
> > +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 3]));
> > +	test(full_name, "%pfw",
> > +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2]));
> > +	test(full_name, "%pfwf",
> > +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2]));
> > +	test(second_name, "%pfwP",
> > +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 3]));
> > +	test(third_name, "%pfwP",
> > +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2]));
> 
> 
> These can be shorted and easier to parse if you use absolute indexes.

The above doesn't end up accessing out-of-bounds memory without compiler
errors or warnings if the array is changed, therefore I'd prefer to keep it
as-is.

But I'll remove the comma from the guardian entry for v5. :-)

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

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

[Petr Mladek]

On Mon 2019-09-02 11:32:36, 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.

Hmm, I see the following in master:

$> git grep %pF
tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt:or events have "%pF" or "%pS" parameter in its format string. It is common to

$> git grep %pf
tools/lib/traceevent/event-parse.c:             if (asprintf(&format, "%%pf: (NO FORMAT FOUND at %llx)\n", addr) < 0)
tools/lib/traceevent/event-parse.c:     if (asprintf(&format, "%s: %s", "%pf", printk->printk) < 0)

I wonder how this is related to printk(). In each case, it seems
that libtraceevent somehow implements the non-standard kernel
%p mofifiers. It looks error-prone to keep another %pf user
with the old semantic around.

I am adding some tracing people into CC.

Best Regards,
Petr

> 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                            |  8 ++------
>  scripts/checkpatch.pl                     |  1 -
>  3 files changed, 2 insertions(+), 17 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..b00b57f9f911f 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);
> 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
> 

Re: [PATCH v4 08/11] lib/vsprintf: Make use of fwnode API to obtain node names and separators

[Petr Mladek]

On Mon 2019-09-02 11:32:37, 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>
> ---
>  lib/vsprintf.c | 38 ++++++++++++++++----------------------
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index b00b57f9f911f..a04a2167101ef 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1863,32 +1864,24 @@ 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);
> +	for (depth = fwnode_count_parents(fwnode); depth >= 0; depth--) {

It looked suspicious that it iterated "depth + 1" times. It might be
obvious for people traversing paths every day but not for me ;-)
Please, add a comment, for example:

	/* Iterate over parents and current node. */

With the above comment:

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

Best Regards,
Petr

Re: [PATCH v4 08/11] lib/vsprintf: Make use of fwnode API to obtain node names and separators

[Sakari Ailus]

Hi Petr,

Thanks for the review.

On Mon, Sep 02, 2019 at 05:18:03PM +0200, Petr Mladek wrote:
> On Mon 2019-09-02 11:32:37, 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>
> > ---
> >  lib/vsprintf.c | 38 ++++++++++++++++----------------------
> >  1 file changed, 16 insertions(+), 22 deletions(-)
> > 
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index b00b57f9f911f..a04a2167101ef 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1863,32 +1864,24 @@ 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);
> > +	for (depth = fwnode_count_parents(fwnode); depth >= 0; depth--) {
> 
> It looked suspicious that it iterated "depth + 1" times. It might be
> obvious for people traversing paths every day but not for me ;-)
> Please, add a comment, for example:
> 
> 	/* Iterate over parents and current node. */
> 
> With the above comment:
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>

Thanks.

How about:

	/* Loop starting from the root node to the current node. */

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

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

[Andy Shevchenko]

On Mon, Sep 02, 2019 at 04:39:35PM +0200, Petr Mladek wrote:
> On Mon 2019-09-02 11:32:36, 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.
> 
> Hmm, I see the following in master:
> 
> $> git grep %pF
> tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt:or events have "%pF" or "%pS" parameter in its format string. It is common to
> 
> $> git grep %pf
> tools/lib/traceevent/event-parse.c:             if (asprintf(&format, "%%pf: (NO FORMAT FOUND at %llx)\n", addr) < 0)
> tools/lib/traceevent/event-parse.c:     if (asprintf(&format, "%s: %s", "%pf", printk->printk) < 0)
> 
> I wonder how this is related to printk(). In each case, it seems

It's going thru binary printf() I suppose. The fist stage just saves the format
string and argument addresses or so and prints in later on when user is looking
for human-readable output.

> that libtraceevent somehow implements the non-standard kernel
> %p mofifiers. It looks error-prone to keep another %pf user
> with the old semantic around.
> 
> I am adding some tracing people into CC.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 10/11] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names

[Petr Mladek]

On Mon 2019-09-02 11:32:39, Sakari Ailus wrote:
> 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.
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index 922a29eb70e6c..abba210f67567 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):

s/:/::/ for the .rst formar.

> +
> +	%pfwf	\_SB.PCI0.CIO2.port@1.endpoint@0	- Full node name
> +	%pfwP	endpoint@0				- Node name
> +
> +Examples (OF):

Same here.

> +
> +	%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 4ad9332d54ba6..b9b4c835db063 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1981,6 +1981,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, "(%pfw?)", spec);

This means that only "%pfw" will dereference the pointer by
fwnode_full_name_string() or fwnode_get_name(). All the other
eventual misuses of the obsolete %pf format will result in this
error message.

OK, it is hard to imagine using "%pf" to get symbol name and always add
'w' suffix. Therefore it looks that reusing the obsolete %pf format
modifier is pretty safe after all.


> +	if (check_pointer(&buf, end, fwnode, spec))
> +		return buf;
> +
> +	fmt++;
> +
> +	switch (*fmt) {
> +	case 'f':	/* full_name */
> +	default:

Using default: in the middle of switch might cause a lot of confusion.
Please, make it the last label.


> +		buf = fwnode_full_name_string(fwnode, buf, end);
> +		break;
> +	case 'P':	/* name */
> +		buf = string(buf, end, fwnode_get_name(fwnode), str_spec);
> +		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
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index a60c241112cd4..8df50911ff4e9 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5995,7 +5995,8 @@ sub process {
>  				while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) {
>  					$specifier = $1;
>  					$extension = $2;
> -					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxt]/) {
> +					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxtf]/ ||
> +					    $extension =~ /^f[^w]/) {

This does not work. $extension seems to have only one character.

Best Regards,
Petr

Re: [PATCH v4 11/11] lib/test_printf: Add tests for %pfw printk modifier

[Petr Mladek]

On Mon 2019-09-02 11:32:40, Sakari Ailus wrote:
> 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>
> ---
>  lib/test_printf.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/lib/test_printf.c b/lib/test_printf.c
> index 944eb50f38625..9c6d716979fb1 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,40 @@ 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 = "/second/third";
> +	const char * const full_name_second = "/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[ARRAY_SIZE(softnodes) - 3]));

"ARRAY_SIZE(softnodes) - 3" is quite cryptic.
Is there any particular reason to use it instead of &softnodes[1] ?

And is it expected that it does not print the "/first" parent?

> +	test(full_name, "%pfw",
> +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2]));
> +	test(full_name, "%pfwf",
> +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2]));
> +	test(second_name, "%pfwP",
> +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 3]));
> +	test(third_name, "%pfwP",
> +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2]));
> +
> +	software_node_unregister_nodes(softnodes);
> +}

Anyway, thanks for the tests.

Best Regards,
Petr

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

[Petr Mladek]

On Mon 2019-09-02 19:01:39, Andy Shevchenko wrote:
> On Mon, Sep 02, 2019 at 04:39:35PM +0200, Petr Mladek wrote:
> > On Mon 2019-09-02 11:32:36, 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.
> > 
> > Hmm, I see the following in master:
> > 
> > $> git grep %pF
> > tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt:or events have "%pF" or "%pS" parameter in its format string. It is common to
> > 
> > $> git grep %pf
> > tools/lib/traceevent/event-parse.c:             if (asprintf(&format, "%%pf: (NO FORMAT FOUND at %llx)\n", addr) < 0)
> > tools/lib/traceevent/event-parse.c:     if (asprintf(&format, "%s: %s", "%pf", printk->printk) < 0)
> > 
> > I wonder how this is related to printk(). In each case, it seems
> 
> It's going thru binary printf() I suppose. The fist stage just saves the format
> string and argument addresses or so and prints in later on when user is looking
> for human-readable output.

It seems that vbin_printf() still thinks that %pf and %pF
handle function pointers. If I get it correctly, it just
stores the binary data and the formating is done when
tracing log is read. The idea is the function pointers
will stay the same.

We need to fix/obsolete this path as well.

Best Regards,
Petr

Re: [PATCH v4 11/11] lib/test_printf: Add tests for %pfw printk modifier

[Sakari Ailus]

Hi Petr,

Thanks for the comments.

On Tue, Sep 03, 2019 at 03:38:41PM +0200, Petr Mladek wrote:
> On Mon 2019-09-02 11:32:40, Sakari Ailus wrote:
> > 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>
> > ---
> >  lib/test_printf.c | 37 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> > 
> > diff --git a/lib/test_printf.c b/lib/test_printf.c
> > index 944eb50f38625..9c6d716979fb1 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,40 @@ 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 = "/second/third";
> > +	const char * const full_name_second = "/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[ARRAY_SIZE(softnodes) - 3]));
> 
> "ARRAY_SIZE(softnodes) - 3" is quite cryptic.
> Is there any particular reason to use it instead of &softnodes[1] ?

I'm fine using a direct index, rather than refer to entries from the top
downwards.

> 
> And is it expected that it does not print the "/first" parent?

Heikki actually commented on an issue related to the "root" nodes. I'll
reply to his comment, on the 5th patch of the set.

> 
> > +	test(full_name, "%pfw",
> > +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2]));
> > +	test(full_name, "%pfwf",
> > +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2]));
> > +	test(second_name, "%pfwP",
> > +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 3]));
> > +	test(third_name, "%pfwP",
> > +	     software_node_fwnode(&softnodes[ARRAY_SIZE(softnodes) - 2]));
> > +
> > +	software_node_unregister_nodes(softnodes);
> > +}
> 
> Anyway, thanks for the tests.

You're welcome!

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v4 10/11] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names

[Sakari Ailus]

Hi Petr,

On Tue, Sep 03, 2019 at 03:06:07PM +0200, Petr Mladek wrote:
> On Mon 2019-09-02 11:32:39, Sakari Ailus wrote:
> > 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.
> > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> > index 922a29eb70e6c..abba210f67567 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):
> 
> s/:/::/ for the .rst formar.

Fixed both.

> 
> > +
> > +	%pfwf	\_SB.PCI0.CIO2.port@1.endpoint@0	- Full node name
> > +	%pfwP	endpoint@0				- Node name
> > +
> > +Examples (OF):
> 
> Same here.
> 
> > +
> > +	%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 4ad9332d54ba6..b9b4c835db063 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1981,6 +1981,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, "(%pfw?)", spec);

This should have been:

		return error_string(buf, end, "(%pf?)", spec);

I'll address that for v6.

> 
> This means that only "%pfw" will dereference the pointer by
> fwnode_full_name_string() or fwnode_get_name(). All the other
> eventual misuses of the obsolete %pf format will result in this
> error message.
> 
> OK, it is hard to imagine using "%pf" to get symbol name and always add
> 'w' suffix. Therefore it looks that reusing the obsolete %pf format
> modifier is pretty safe after all.
> 
> 
> > +	if (check_pointer(&buf, end, fwnode, spec))
> > +		return buf;
> > +
> > +	fmt++;
> > +
> > +	switch (*fmt) {
> > +	case 'f':	/* full_name */
> > +	default:
> 
> Using default: in the middle of switch might cause a lot of confusion.
> Please, make it the last label.

Fixed.

> 
> 
> > +		buf = fwnode_full_name_string(fwnode, buf, end);
> > +		break;
> > +	case 'P':	/* name */
> > +		buf = string(buf, end, fwnode_get_name(fwnode), str_spec);
> > +		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
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index a60c241112cd4..8df50911ff4e9 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -5995,7 +5995,8 @@ sub process {
> >  				while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) {
> >  					$specifier = $1;
> >  					$extension = $2;
> > -					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxt]/) {
> > +					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxtf]/ ||
> > +					    $extension =~ /^f[^w]/) {
> 
> This does not work. $extension seems to have only one character.

Good catch. \w indeed matches a single letter; I'll change that to \w+ and
change the other uses accordingly.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v4 10/11] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names

[Andy Shevchenko]

On Wed, Sep 04, 2019 at 06:04:13PM +0300, Sakari Ailus wrote:
> On Tue, Sep 03, 2019 at 03:06:07PM +0200, Petr Mladek wrote:
> > On Mon 2019-09-02 11:32:39, Sakari Ailus wrote:
> > > 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.

> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -5995,7 +5995,8 @@ sub process {
> > >  				while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) {
> > >  					$specifier = $1;
> > >  					$extension = $2;
> > > -					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxt]/) {
> > > +					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxtf]/ ||
> > > +					    $extension =~ /^f[^w]/) {
> > 
> > This does not work. $extension seems to have only one character.
> 
> Good catch. \w indeed matches a single letter; I'll change that to \w+ and
> change the other uses accordingly.

It's weird. \w stands for word matching. How can it match one letter only?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 10/11] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names

[Joe Perches]

On Wed, 2019-09-04 at 18:04 +0300, Sakari Ailus wrote:
> On Tue, Sep 03, 2019 at 03:06:07PM +0200, Petr Mladek wrote:
> > On Mon 2019-09-02 11:32:39, Sakari Ailus wrote:
[]
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> > > @@ -5995,7 +5995,8 @@ sub process {
> > >  				while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) {
> > >  					$specifier = $1;
> > >  					$extension = $2;
> > > -					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxt]/) {
> > > +					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxtf]/ ||
> > > +					    $extension =~ /^f[^w]/) {
> > 
> > This does not work. $extension seems to have only one character.
> 
> Good catch. \w indeed matches a single letter; I'll change that to \w+ and
> change the other uses accordingly.

If you want to make changes to checkpatch, please
send patches to the checkpatch maintainers.

Don't break other parsing of $2/#extension.

If you really need to know whatever follows the specific
extension letter use another capture group.

			while ($fmt =~  /(\%[\*\d\.]*p(\w)(\w*))/g) {
				$specifier = $1;
				$extension = $2;
				$qualifier = $3;

etc...

Then verify $qualifier or $3 is not undef if necessary

Re: [PATCH v4 10/11] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names

[Joe Perches]

On Wed, 2019-09-04 at 18:17 +0300, Andy Shevchenko wrote:
> On Wed, Sep 04, 2019 at 06:04:13PM +0300, Sakari Ailus wrote:
> > On Tue, Sep 03, 2019 at 03:06:07PM +0200, Petr Mladek wrote:
> > > On Mon 2019-09-02 11:32:39, Sakari Ailus wrote:
> > > > 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.
> > > > --- a/scripts/checkpatch.pl
> > > > +++ b/scripts/checkpatch.pl
> > > > @@ -5995,7 +5995,8 @@ sub process {
> > > >  				while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) {
> > > >  					$specifier = $1;
> > > >  					$extension = $2;
> > > > -					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxt]/) {
> > > > +					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxtf]/ ||
> > > > +					    $extension =~ /^f[^w]/) {
> > > 
> > > This does not work. $extension seems to have only one character.
> > 
> > Good catch. \w indeed matches a single letter; I'll change that to \w+ and
> > change the other uses accordingly.
> 
> It's weird. \w stands for word matching. How can it match one letter only?

\w 	matches any single character classified as a “word” character (alphanumeric or “_”)

Re: [PATCH v4 10/11] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names

[Andy Shevchenko]

On Wed, Sep 04, 2019 at 08:39:42AM -0700, Joe Perches wrote:
> On Wed, 2019-09-04 at 18:17 +0300, Andy Shevchenko wrote:
> > On Wed, Sep 04, 2019 at 06:04:13PM +0300, Sakari Ailus wrote:
> > > On Tue, Sep 03, 2019 at 03:06:07PM +0200, Petr Mladek wrote:

> > > Good catch. \w indeed matches a single letter; I'll change that to \w+ and
> > > change the other uses accordingly.
> > 
> > It's weird. \w stands for word matching. How can it match one letter only?
> 
> \w 	matches any single character classified as a “word” character (alphanumeric or “_”)

Thank you for explanation!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 10/11] lib/vsprintf: Add %pfw conversion specifier for printing fwnode names

[Sakari Ailus]

Hi Joe,

On Wed, Sep 04, 2019 at 08:37:08AM -0700, Joe Perches wrote:
> On Wed, 2019-09-04 at 18:04 +0300, Sakari Ailus wrote:
> > On Tue, Sep 03, 2019 at 03:06:07PM +0200, Petr Mladek wrote:
> > > On Mon 2019-09-02 11:32:39, Sakari Ailus wrote:
> []
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > > > @@ -5995,7 +5995,8 @@ sub process {
> > > >  				while ($fmt =~ /(\%[\*\d\.]*p(\w))/g) {
> > > >  					$specifier = $1;
> > > >  					$extension = $2;
> > > > -					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxt]/) {
> > > > +					if ($extension !~ /[SsBKRraEhMmIiUDdgVCbGNOxtf]/ ||
> > > > +					    $extension =~ /^f[^w]/) {
> > > 
> > > This does not work. $extension seems to have only one character.
> > 
> > Good catch. \w indeed matches a single letter; I'll change that to \w+ and
> > change the other uses accordingly.
> 
> If you want to make changes to checkpatch, please
> send patches to the checkpatch maintainers.
> 
> Don't break other parsing of $2/#extension.
> 
> If you really need to know whatever follows the specific
> extension letter use another capture group.
> 
> 			while ($fmt =~  /(\%[\*\d\.]*p(\w)(\w*))/g) {
> 				$specifier = $1;
> 				$extension = $2;
> 				$qualifier = $3;
> 
> etc...
> 
> Then verify $qualifier or $3 is not undef if necessary

There are just a couple of users, but indeed, the extension handlers work
based on a single letter as well so this way it's better aligned with the
kernel implementation. Forks for me.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

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

[Sakari Ailus]

On Tue, Sep 03, 2019 at 04:04:20PM +0200, Petr Mladek wrote:
> On Mon 2019-09-02 19:01:39, Andy Shevchenko wrote:
> > On Mon, Sep 02, 2019 at 04:39:35PM +0200, Petr Mladek wrote:
> > > On Mon 2019-09-02 11:32:36, 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.
> > > 
> > > Hmm, I see the following in master:
> > > 
> > > $> git grep %pF
> > > tools/lib/traceevent/Documentation/libtraceevent-func_apis.txt:or events have "%pF" or "%pS" parameter in its format string. It is common to
> > > 
> > > $> git grep %pf
> > > tools/lib/traceevent/event-parse.c:             if (asprintf(&format, "%%pf: (NO FORMAT FOUND at %llx)\n", addr) < 0)
> > > tools/lib/traceevent/event-parse.c:     if (asprintf(&format, "%s: %s", "%pf", printk->printk) < 0)
> > > 
> > > I wonder how this is related to printk(). In each case, it seems
> > 
> > It's going thru binary printf() I suppose. The fist stage just saves the format
> > string and argument addresses or so and prints in later on when user is looking
> > for human-readable output.
> 
> It seems that vbin_printf() still thinks that %pf and %pF
> handle function pointers. If I get it correctly, it just
> stores the binary data and the formating is done when
> tracing log is read. The idea is the function pointers
> will stay the same.
> 
> We need to fix/obsolete this path as well.

Agreed. I'll include a patch to do that in v6.

-- 
Sakari Ailus
sakari.ailus@linux.intel.com