[PATCH v2 0/4] device property: Add fwnode_get_name() helper

[PATCH v2 0/4] device property: Add fwnode_get_name() helper

[Heikki Krogerus]

Hi,

This is the second version of my proposal for this helper. The
first version can be checked here:
https://lkml.org/lkml/2018/11/5/326

In order to support also ACPI properly, I decided to change the API.
The function fwnode_name() is now fwnode_get_name(), and instead of
returning pointer to the name, the function copies it to a buffer. I
did that because acpica does not offer a way to get a pointer to the
node name, and the name is clearly expected to be accessed only with
the namespace lock held.

I think this is better approach in any case. It will also solve the
problem of getting rid of the unit-address part from DT node names.

Let me know what you guys think.

--
heikki


Heikki Krogerus (4):
  device property: Introduce fwnode_get_name()
  ACPI: property: Add acpi_fwnode_name()
  of/property: Add of_fwnode_name()
  device property: Drop get_named_child_node callback

 drivers/acpi/property.c  | 43 ++++++++++++++++++++++------------------
 drivers/base/property.c  | 34 ++++++++++++++++++++++++++++++-
 drivers/of/property.c    | 26 ++++++++++--------------
 include/linux/fwnode.h   |  6 +++---
 include/linux/property.h |  2 ++
 5 files changed, 73 insertions(+), 38 deletions(-)

-- 
2.19.1

[PATCH v2 1/4] device property: Introduce fwnode_get_name()

[Heikki Krogerus]

This helper returns the name of the node. The name is
primarily expected to be returned from a new fwnode
operation meant for this purpose, but when no name is
returned, the helper will also attempt to read a device
property "name".

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/base/property.c  | 25 +++++++++++++++++++++++++
 include/linux/fwnode.h   |  3 +++
 include/linux/property.h |  2 ++
 3 files changed, 30 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 240ab5230ff6..4db710e2ce34 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1156,6 +1156,31 @@ void fwnode_handle_put(struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL_GPL(fwnode_handle_put);
 
+/**
+ * fwnode_get_name - Read the name of a node
+ * @fwnode: Pointer to the node.
+ * @buf: Buffer where the name is copied to
+ *
+ * Reads the node name of @fwnode with get_name fwnode op. If get_name op fails,
+ * the routine attempts to also read a property "name".
+ */
+int fwnode_get_name(const struct fwnode_handle *fwnode, char *buf)
+{
+	const char *name;
+	int ret;
+
+	if (!fwnode_call_int_op(fwnode, get_name, buf))
+		return 0;
+
+	ret = fwnode_call_int_op(fwnode, property_read_string_array,
+				 "name", &name, 1);
+	if (!ret)
+		sprintf(buf, "%s", name);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_name);
+
 /**
  * fwnode_device_is_available - check if a device is available for use
  * @fwnode: Pointer to the fwnode of the device.
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index faebf0ca0686..cc234f5d2b40 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -48,6 +48,8 @@ struct fwnode_reference_args {
 	u64 args[NR_FWNODE_REFERENCE_ARGS];
 };
 
+#define FWNODE_NAME_MAX_SIZE		32
+
 /**
  * struct fwnode_operations - Operations for fwnode interface
  * @get: Get a reference to an fwnode.
@@ -72,6 +74,7 @@ struct fwnode_reference_args {
 struct fwnode_operations {
 	struct fwnode_handle *(*get)(struct fwnode_handle *fwnode);
 	void (*put)(struct fwnode_handle *fwnode);
+	int (*get_name)(const struct fwnode_handle *fwnode, char *buf);
 	bool (*device_is_available)(const struct fwnode_handle *fwnode);
 	const void *(*device_get_match_data)(const struct fwnode_handle *fwnode,
 					     const struct device *dev);
diff --git a/include/linux/property.h b/include/linux/property.h
index ac8a1ebc4c1b..da37bdebf7ab 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -109,6 +109,8 @@ struct fwnode_handle *device_get_named_child_node(struct device *dev,
 struct fwnode_handle *fwnode_handle_get(struct fwnode_handle *fwnode);
 void fwnode_handle_put(struct fwnode_handle *fwnode);
 
+int fwnode_get_name(const struct fwnode_handle *fwnode, char *name);
+
 int fwnode_irq_get(struct fwnode_handle *fwnode, unsigned int index);
 
 unsigned int device_get_child_node_count(struct device *dev);
-- 
2.19.1

[PATCH v2 2/4] ACPI: property: Add acpi_fwnode_name()

[Heikki Krogerus]

This implements the get_name fwnode op for ACPI.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/acpi/property.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 8c7c4583b52d..5b4c659c95d4 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1219,6 +1219,28 @@ acpi_graph_get_remote_endpoint(const struct fwnode_handle *__fwnode)
 	return acpi_graph_get_child_prop_value(fwnode, "endpoint", endpoint_nr);
 }
 
+static int acpi_fwnode_get_name(const struct fwnode_handle *fwnode, char *buf)
+{
+	struct acpi_buffer buffer;
+	acpi_handle handle;
+	acpi_status status;
+
+	if (is_acpi_data_node(fwnode)) {
+		sprintf(buf, "%s", to_acpi_data_node(fwnode)->name);
+		return 0;
+	}
+
+	handle = to_acpi_device_node(fwnode)->handle;
+
+	buffer.length = ACPI_NAME_SIZE + 1;
+	buffer.pointer = buf;
+
+	status = acpi_get_name(handle, ACPI_SINGLE_NAME, &buffer);
+	if (ACPI_FAILURE(status))
+		return -ENXIO;
+	return 0;
+}
+
 static bool acpi_fwnode_device_is_available(const struct fwnode_handle *fwnode)
 {
 	if (!is_acpi_device_node(fwnode))
@@ -1310,6 +1332,7 @@ acpi_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
 
 #define DECLARE_ACPI_FWNODE_OPS(ops) \
 	const struct fwnode_operations ops = {				\
+		.get_name = acpi_fwnode_get_name,			\
 		.device_is_available = acpi_fwnode_device_is_available, \
 		.device_get_match_data = acpi_fwnode_device_get_match_data, \
 		.property_present = acpi_fwnode_property_present,	\
-- 
2.19.1

[PATCH v2 3/4] of/property: Add of_fwnode_name()

[Heikki Krogerus]

This implements get_name fwnode op for DT.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/of/property.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index f46828e3b082..9bc8fe136fa3 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -823,6 +823,16 @@ static void of_fwnode_put(struct fwnode_handle *fwnode)
 	of_node_put(to_of_node(fwnode));
 }
 
+static int of_fwnode_get_name(const struct fwnode_handle *fwnode, char *buf)
+{
+	const char *name = kbasename(to_of_node(fwnode)->full_name);
+	size_t len = strchrnul(name, '@') - name;
+
+	snprintf(buf, len + 1, "%s", name);
+
+	return 0;
+}
+
 static bool of_fwnode_device_is_available(const struct fwnode_handle *fwnode)
 {
 	return of_device_is_available(to_of_node(fwnode));
@@ -987,6 +997,7 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
 const struct fwnode_operations of_fwnode_ops = {
 	.get = of_fwnode_get,
 	.put = of_fwnode_put,
+	.get_name = of_fwnode_get_name,
 	.device_is_available = of_fwnode_device_is_available,
 	.device_get_match_data = of_fwnode_device_get_match_data,
 	.property_present = of_fwnode_property_present,
-- 
2.19.1

[PATCH v2 4/4] device property: Drop get_named_child_node callback

[Heikki Krogerus]

By using fwnode_get_name() in fwnode_get_named_child_node(),
get_named_child_node implementations become boilerplate.
Removing all of them.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/acpi/property.c | 20 +-------------------
 drivers/base/property.c |  9 ++++++++-
 drivers/of/property.c   | 15 ---------------
 include/linux/fwnode.h  |  3 ---
 4 files changed, 9 insertions(+), 38 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 5b4c659c95d4..03cb46018920 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -587,23 +587,6 @@ static int acpi_data_get_property_array(const struct acpi_device_data *data,
 	return 0;
 }
 
-static struct fwnode_handle *
-acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
-				 const char *childname)
-{
-	struct fwnode_handle *child;
-
-	/*
-	 * Find first matching named child node of this fwnode.
-	 * For ACPI this will be a data only sub-node.
-	 */
-	fwnode_for_each_child_node(fwnode, child)
-		if (acpi_data_node_match(child, childname))
-			return child;
-
-	return NULL;
-}
-
 /**
  * __acpi_node_get_property_reference - returns handle to the referenced object
  * @fwnode: Firmware node to get the property from
@@ -712,7 +695,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
 			for (ref_fwnode = acpi_fwnode_handle(device);
 			     element < end && element->type == ACPI_TYPE_STRING;
 			     element++) {
-				ref_fwnode = acpi_fwnode_get_named_child_node(
+				ref_fwnode = fwnode_get_named_child_node(
 					ref_fwnode, element->string.pointer);
 				if (!ref_fwnode)
 					return -EINVAL;
@@ -1342,7 +1325,6 @@ acpi_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
 			acpi_fwnode_property_read_string_array,		\
 		.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_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 4db710e2ce34..802f55f9fab9 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1111,7 +1111,14 @@ struct fwnode_handle *
 fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
 			    const char *childname)
 {
-	return fwnode_call_ptr_op(fwnode, get_named_child_node, childname);
+	char name[FWNODE_NAME_MAX_SIZE];
+	struct fwnode_handle *child;
+
+	fwnode_for_each_child_node(fwnode, child)
+		if (!fwnode_get_name(child, name) &&
+		    !strcmp(name, childname))
+			return child;
+	return NULL;
 }
 EXPORT_SYMBOL_GPL(fwnode_get_named_child_node);
 
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 9bc8fe136fa3..34b157921568 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -895,20 +895,6 @@ of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode,
 							    to_of_node(child)));
 }
 
-static struct fwnode_handle *
-of_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
-			       const char *childname)
-{
-	const struct device_node *node = to_of_node(fwnode);
-	struct device_node *child;
-
-	for_each_available_child_of_node(node, child)
-		if (!of_node_cmp(child->name, childname))
-			return of_fwnode_handle(child);
-
-	return NULL;
-}
-
 static int
 of_fwnode_get_reference_args(const struct fwnode_handle *fwnode,
 			     const char *prop, const char *nargs_prop,
@@ -1005,7 +991,6 @@ const struct fwnode_operations of_fwnode_ops = {
 	.property_read_string_array = of_fwnode_property_read_string_array,
 	.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,
 	.get_reference_args = of_fwnode_get_reference_args,
 	.graph_get_next_endpoint = of_fwnode_graph_get_next_endpoint,
 	.graph_get_remote_endpoint = of_fwnode_graph_get_remote_endpoint,
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index cc234f5d2b40..45df74f91603 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -92,9 +92,6 @@ struct fwnode_operations {
 	struct fwnode_handle *
 	(*get_next_child_node)(const struct fwnode_handle *fwnode,
 			       struct fwnode_handle *child);
-	struct fwnode_handle *
-	(*get_named_child_node)(const struct fwnode_handle *fwnode,
-				const char *name);
 	int (*get_reference_args)(const struct fwnode_handle *fwnode,
 				  const char *prop, const char *nargs_prop,
 				  unsigned int nargs, unsigned int index,
-- 
2.19.1

Re: [PATCH v2 3/4] of/property: Add of_fwnode_name()

[Rob Herring]

On Thu, Nov 8, 2018 at 10:52 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> This implements get_name fwnode op for DT.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/of/property.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index f46828e3b082..9bc8fe136fa3 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -823,6 +823,16 @@ static void of_fwnode_put(struct fwnode_handle *fwnode)
>         of_node_put(to_of_node(fwnode));
>  }
>
> +static int of_fwnode_get_name(const struct fwnode_handle *fwnode, char *buf)
> +{
> +       const char *name = kbasename(to_of_node(fwnode)->full_name);
> +       size_t len = strchrnul(name, '@') - name;
> +
> +       snprintf(buf, len + 1, "%s", name);

This can be simplified to:

snprintf(..., "%pOFn", to_of_node(fwnode))

But that presents a problem with knowing the length. Not passing in
the buf length is not good design because you can't tell if you
overflow the buffer. Either you can pass in the length of buf or do
the allocation here. In the latter case, then you can use kasnprintf.
The downside to doing the allocation here is then get_name() has side
effect of allocating memory that the caller needs to be aware of.

However, I think the current API is better. It leaves low-level
details up to the firmware implementation. But as long as .get_name()
is not exposed to drivers I don't really care that much.

Rob

Re: [PATCH v2 3/4] of/property: Add of_fwnode_name()

[Andy Shevchenko]

On Thu, Nov 08, 2018 at 12:23:51PM -0600, Rob Herring wrote:
> On Thu, Nov 8, 2018 at 10:52 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > This implements get_name fwnode op for DT.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  drivers/of/property.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index f46828e3b082..9bc8fe136fa3 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -823,6 +823,16 @@ static void of_fwnode_put(struct fwnode_handle *fwnode)
> >         of_node_put(to_of_node(fwnode));
> >  }
> >
> > +static int of_fwnode_get_name(const struct fwnode_handle *fwnode, char *buf)
> > +{
> > +       const char *name = kbasename(to_of_node(fwnode)->full_name);
> > +       size_t len = strchrnul(name, '@') - name;
> > +
> > +       snprintf(buf, len + 1, "%s", name);
> 
> This can be simplified to:
> 
> snprintf(..., "%pOFn", to_of_node(fwnode))
> 
> But that presents a problem with knowing the length. Not passing in
> the buf length is not good design because you can't tell if you
> overflow the buffer. Either you can pass in the length of buf or do
> the allocation here. In the latter case, then you can use kasnprintf.
> The downside to doing the allocation here is then get_name() has side
> effect of allocating memory that the caller needs to be aware of.

Agree on matter of potential overflow.

I wouldn't limit users with 32 characters for node name if it's not by both
ACPI and DT specifications. OTOH allocating and freeing memory in a loop each
time when we would like to go through the child nodes sounds much worse
scenario to me. Thus, giving a length of the buffer is a good enough compromise.

> However, I think the current API is better. It leaves low-level
> details up to the firmware implementation. But as long as .get_name()
> is not exposed to drivers I don't really care that much.

I don't think this concept is changed by Heikki's patch series. It provides a
common abstract function on top of more low-level firmware implementation which
I consider a good approach.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] of/property: Add of_fwnode_name()

[Rob Herring]

On Thu, Nov 8, 2018 at 1:25 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Nov 08, 2018 at 12:23:51PM -0600, Rob Herring wrote:
> > On Thu, Nov 8, 2018 at 10:52 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > This implements get_name fwnode op for DT.
> > >
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > ---
> > >  drivers/of/property.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > index f46828e3b082..9bc8fe136fa3 100644
> > > --- a/drivers/of/property.c
> > > +++ b/drivers/of/property.c
> > > @@ -823,6 +823,16 @@ static void of_fwnode_put(struct fwnode_handle *fwnode)
> > >         of_node_put(to_of_node(fwnode));
> > >  }
> > >
> > > +static int of_fwnode_get_name(const struct fwnode_handle *fwnode, char *buf)
> > > +{
> > > +       const char *name = kbasename(to_of_node(fwnode)->full_name);
> > > +       size_t len = strchrnul(name, '@') - name;
> > > +
> > > +       snprintf(buf, len + 1, "%s", name);
> >
> > This can be simplified to:
> >
> > snprintf(..., "%pOFn", to_of_node(fwnode))
> >
> > But that presents a problem with knowing the length. Not passing in
> > the buf length is not good design because you can't tell if you
> > overflow the buffer. Either you can pass in the length of buf or do
> > the allocation here. In the latter case, then you can use kasnprintf.
> > The downside to doing the allocation here is then get_name() has side
> > effect of allocating memory that the caller needs to be aware of.
>
> Agree on matter of potential overflow.
>
> I wouldn't limit users with 32 characters for node name if it's not by both
> ACPI and DT specifications.

While the DT spec says 31 characters, this has never been enforced. As
you might guess, we have node names longer than 31 characters. There's
been some discussion about what to do and the consensus seems to be
change the spec.

> OTOH allocating and freeing memory in a loop each
> time when we would like to go through the child nodes sounds much worse
> scenario to me.

Yes, I wrote that before looking at how you were using it... Of
course, if you want efficient, then you shouldn't use sprintf either
and use of_node_name_eq() as I've suggested.

> Thus, giving a length of the buffer is a good enough compromise.
>
> > However, I think the current API is better. It leaves low-level
> > details up to the firmware implementation. But as long as .get_name()
> > is not exposed to drivers I don't really care that much.
>
> I don't think this concept is changed by Heikki's patch series. It provides a
> common abstract function on top of more low-level firmware implementation which
> I consider a good approach.

Generally, I would agree that's a worthwhile goal. However, in this
case you aren't saving anything. We still have at least a DT version
of the same thing (of_get_child_by_name). Maybe there's some dream
that the fwnode API will become the only one for both drivers and
non-drivers, but I really don't see that happening. As long as both DT
and fwnode APIs are in use, it is best to keep the APIs aligned.

There's another aspect that the node name comparison is case
insensitive on powerpc (and until 4.20, was for everything but Sparc).
I changed it in 4.20 and hopefully don't have to revert. Patch 4 does
a case sensitive compare. That probably is fine (as lots of powerpc
code already does case sensitive name compares), but no one really
knows until we break users.

Another issue is how are disabled nodes dealt with by different
firmwares? It's a frequent bug that we don't honor the 'status'
property (such as in the very code we're discussing). But then there
are some cases were want to ignore it so we can't just go add that
check in and we end up needing 2 flavors of everything. You're
probably okay though. Most devices with child nodes are
enabled/disabled only in the parent device node.

Rob
Rob

Re: [PATCH v2 3/4] of/property: Add of_fwnode_name()

[Heikki Krogerus]

On Thu, Nov 08, 2018 at 03:18:21PM -0600, Rob Herring wrote:
> On Thu, Nov 8, 2018 at 1:25 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Thu, Nov 08, 2018 at 12:23:51PM -0600, Rob Herring wrote:
> > > On Thu, Nov 8, 2018 at 10:52 AM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > This implements get_name fwnode op for DT.
> > > >
> > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > ---
> > > >  drivers/of/property.c | 11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > > index f46828e3b082..9bc8fe136fa3 100644
> > > > --- a/drivers/of/property.c
> > > > +++ b/drivers/of/property.c
> > > > @@ -823,6 +823,16 @@ static void of_fwnode_put(struct fwnode_handle *fwnode)
> > > >         of_node_put(to_of_node(fwnode));
> > > >  }
> > > >
> > > > +static int of_fwnode_get_name(const struct fwnode_handle *fwnode, char *buf)
> > > > +{
> > > > +       const char *name = kbasename(to_of_node(fwnode)->full_name);
> > > > +       size_t len = strchrnul(name, '@') - name;
> > > > +
> > > > +       snprintf(buf, len + 1, "%s", name);
> > >
> > > This can be simplified to:
> > >
> > > snprintf(..., "%pOFn", to_of_node(fwnode))
> > >
> > > But that presents a problem with knowing the length. Not passing in
> > > the buf length is not good design because you can't tell if you
> > > overflow the buffer. Either you can pass in the length of buf or do
> > > the allocation here. In the latter case, then you can use kasnprintf.
> > > The downside to doing the allocation here is then get_name() has side
> > > effect of allocating memory that the caller needs to be aware of.
> >
> > Agree on matter of potential overflow.
> >
> > I wouldn't limit users with 32 characters for node name if it's not by both
> > ACPI and DT specifications.
> 
> While the DT spec says 31 characters, this has never been enforced. As
> you might guess, we have node names longer than 31 characters. There's
> been some discussion about what to do and the consensus seems to be
> change the spec.
> 
> > OTOH allocating and freeing memory in a loop each
> > time when we would like to go through the child nodes sounds much worse
> > scenario to me.
> 
> Yes, I wrote that before looking at how you were using it... Of
> course, if you want efficient, then you shouldn't use sprintf either
> and use of_node_name_eq() as I've suggested.

Since the fwnode API is just a wrapper layer (at least IMO), I don't
think there should be any assumptions that it provides the optimal
solution for anything. The low-level APIs should be the ones providing
the optimal solutions.

> > Thus, giving a length of the buffer is a good enough compromise.

OK. That's what we'll do then.

> > > However, I think the current API is better. It leaves low-level
> > > details up to the firmware implementation. But as long as .get_name()
> > > is not exposed to drivers I don't really care that much.

Side note:

I would prefer that we had something like of_node_name_get() function
that of_fwnode_get_name() could simply call. I don't know how you want
that implemented, but I'm expecting you will implement something like
that in any case once you start removing that ->name member. I figured
that at that point we can update of_fwnode_get_name() as well.

> > I don't think this concept is changed by Heikki's patch series. It provides a
> > common abstract function on top of more low-level firmware implementation which
> > I consider a good approach.
> 
> Generally, I would agree that's a worthwhile goal. However, in this
> case you aren't saving anything. We still have at least a DT version
> of the same thing (of_get_child_by_name). Maybe there's some dream
> that the fwnode API will become the only one for both drivers and
> non-drivers, but I really don't see that happening. As long as both DT
> and fwnode APIs are in use, it is best to keep the APIs aligned.

I don't think that anybody was planning to have the fwnode API as
the only available API for the drivers. It's just a helper for the
drivers on top of the low-level APIs, so the low-level APIs really
need to always stay available for the drivers. There will always be
corner cases.

> There's another aspect that the node name comparison is case
> insensitive on powerpc (and until 4.20, was for everything but Sparc).
> I changed it in 4.20 and hopefully don't have to revert. Patch 4 does
> a case sensitive compare. That probably is fine (as lots of powerpc
> code already does case sensitive name compares), but no one really
> knows until we break users.

I actually used stccasecmp() in the first version. I don't know how,
or why, I've changed it to strcmp(). I'll fix that.

> Another issue is how are disabled nodes dealt with by different
> firmwares? It's a frequent bug that we don't honor the 'status'
> property (such as in the very code we're discussing). But then there
> are some cases were want to ignore it so we can't just go add that
> check in and we end up needing 2 flavors of everything. You're
> probably okay though. Most devices with child nodes are
> enabled/disabled only in the parent device node.


thanks,

-- 
heikki

Re: [PATCH v2 3/4] of/property: Add of_fwnode_name()

[Rob Herring]

On Fri, Nov 9, 2018 at 7:34 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Thu, Nov 08, 2018 at 03:18:21PM -0600, Rob Herring wrote:
> > On Thu, Nov 8, 2018 at 1:25 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Thu, Nov 08, 2018 at 12:23:51PM -0600, Rob Herring wrote:
> > > > On Thu, Nov 8, 2018 at 10:52 AM Heikki Krogerus
> > > > <heikki.krogerus@linux.intel.com> wrote:
> > > > >
> > > > > This implements get_name fwnode op for DT.
> > > > >
> > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > ---
> > > > >  drivers/of/property.c | 11 +++++++++++
> > > > >  1 file changed, 11 insertions(+)
> > > > >
> > > > > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > > > > index f46828e3b082..9bc8fe136fa3 100644
> > > > > --- a/drivers/of/property.c
> > > > > +++ b/drivers/of/property.c
> > > > > @@ -823,6 +823,16 @@ static void of_fwnode_put(struct fwnode_handle *fwnode)
> > > > >         of_node_put(to_of_node(fwnode));
> > > > >  }
> > > > >
> > > > > +static int of_fwnode_get_name(const struct fwnode_handle *fwnode, char *buf)
> > > > > +{
> > > > > +       const char *name = kbasename(to_of_node(fwnode)->full_name);
> > > > > +       size_t len = strchrnul(name, '@') - name;
> > > > > +
> > > > > +       snprintf(buf, len + 1, "%s", name);
> > > >
> > > > This can be simplified to:
> > > >
> > > > snprintf(..., "%pOFn", to_of_node(fwnode))
> > > >
> > > > But that presents a problem with knowing the length. Not passing in
> > > > the buf length is not good design because you can't tell if you
> > > > overflow the buffer. Either you can pass in the length of buf or do
> > > > the allocation here. In the latter case, then you can use kasnprintf.
> > > > The downside to doing the allocation here is then get_name() has side
> > > > effect of allocating memory that the caller needs to be aware of.
> > >
> > > Agree on matter of potential overflow.
> > >
> > > I wouldn't limit users with 32 characters for node name if it's not by both
> > > ACPI and DT specifications.
> >
> > While the DT spec says 31 characters, this has never been enforced. As
> > you might guess, we have node names longer than 31 characters. There's
> > been some discussion about what to do and the consensus seems to be
> > change the spec.
> >
> > > OTOH allocating and freeing memory in a loop each
> > > time when we would like to go through the child nodes sounds much worse
> > > scenario to me.
> >
> > Yes, I wrote that before looking at how you were using it... Of
> > course, if you want efficient, then you shouldn't use sprintf either
> > and use of_node_name_eq() as I've suggested.
>
> Since the fwnode API is just a wrapper layer (at least IMO), I don't
> think there should be any assumptions that it provides the optimal
> solution for anything. The low-level APIs should be the ones providing
> the optimal solutions.
>
> > > Thus, giving a length of the buffer is a good enough compromise.
>
> OK. That's what we'll do then.
>
> > > > However, I think the current API is better. It leaves low-level
> > > > details up to the firmware implementation. But as long as .get_name()
> > > > is not exposed to drivers I don't really care that much.
>
> Side note:
>
> I would prefer that we had something like of_node_name_get() function
> that of_fwnode_get_name() could simply call. I don't know how you want
> that implemented, but I'm expecting you will implement something like
> that in any case once you start removing that ->name member. I figured
> that at that point we can update of_fwnode_get_name() as well.

I don't plan to implement anything like that. Here's what I'm planning:
https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/log/?h=dt/testing

Which is eliminating the need to access .name by: using %pOFn (mostly
in 4.20), using of_node_name_eq() wrapper, or using full_name instead.
The last case generally is cases where the name is not important such
as request_irq() or irq_chip.name.

> > > I don't think this concept is changed by Heikki's patch series. It provides a
> > > common abstract function on top of more low-level firmware implementation which
> > > I consider a good approach.
> >
> > Generally, I would agree that's a worthwhile goal. However, in this
> > case you aren't saving anything. We still have at least a DT version
> > of the same thing (of_get_child_by_name). Maybe there's some dream
> > that the fwnode API will become the only one for both drivers and
> > non-drivers, but I really don't see that happening. As long as both DT
> > and fwnode APIs are in use, it is best to keep the APIs aligned.
>
> I don't think that anybody was planning to have the fwnode API as
> the only available API for the drivers. It's just a helper for the
> drivers on top of the low-level APIs, so the low-level APIs really
> need to always stay available for the drivers. There will always be
> corner cases.

So either it needs to be the only interface to drivers or the fwnode
and DT APIs need to have the same calls. We don't want drivers to use
different style/design of API based on picking fwnode or DT API. If
you have the same calls, there's no point to trying to copy the common
parts into fwnode code. Then you have 3 implementations of the same
thing instead of 2.

> > There's another aspect that the node name comparison is case
> > insensitive on powerpc (and until 4.20, was for everything but Sparc).
> > I changed it in 4.20 and hopefully don't have to revert. Patch 4 does
> > a case sensitive compare. That probably is fine (as lots of powerpc
> > code already does case sensitive name compares), but no one really
> > knows until we break users.
>
> I actually used stccasecmp() in the first version. I don't know how,
> or why, I've changed it to strcmp(). I'll fix that.

That's not what I'm suggesting. I'm saying that is a firmware level
detail that should remain in firmware code.

I'm actually hoping to move things over to be case sensitive in most
cases. That may mean having some work-around like:

if (IS_ENABLED(CONFIG_PPC_PMAC))
  ... strcasecmp()
else
  ... strcmp()

Or whatever system needs this. I think it is old PowerMacs, but am not
really sure. And yeah, that would be ugly, but at least we'd know what
exactly needs it. Do you want this crap is fwnode code?

Rob

Re: [PATCH v2 3/4] of/property: Add of_fwnode_name()

[Heikki Krogerus]

Hi,

On Fri, Nov 09, 2018 at 10:14:49AM -0600, Rob Herring wrote:
> > I would prefer that we had something like of_node_name_get() function
> > that of_fwnode_get_name() could simply call. I don't know how you want
> > that implemented, but I'm expecting you will implement something like
> > that in any case once you start removing that ->name member. I figured
> > that at that point we can update of_fwnode_get_name() as well.
> 
> I don't plan to implement anything like that. Here's what I'm planning:
> https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git/log/?h=dt/testing
> 
> Which is eliminating the need to access .name by: using %pOFn (mostly
> in 4.20), using of_node_name_eq() wrapper, or using full_name instead.
> The last case generally is cases where the name is not important such
> as request_irq() or irq_chip.name.

OK. Thanks for the explanation.


thanks,

-- 
heikki

Re: [PATCH v2 0/4] device property: Add fwnode_get_name() helper

[Rafael J. Wysocki]

On Thursday, November 8, 2018 5:51:52 PM CET Heikki Krogerus wrote:
> Hi,
> 
> This is the second version of my proposal for this helper. The
> first version can be checked here:
> https://lkml.org/lkml/2018/11/5/326
> 
> In order to support also ACPI properly, I decided to change the API.
> The function fwnode_name() is now fwnode_get_name(), and instead of
> returning pointer to the name, the function copies it to a buffer. I
> did that because acpica does not offer a way to get a pointer to the
> node name, and the name is clearly expected to be accessed only with
> the namespace lock held.
> 
> I think this is better approach in any case. It will also solve the
> problem of getting rid of the unit-address part from DT node names.
> 
> Let me know what you guys think.
> 
> --
> heikki
> 
> 
> Heikki Krogerus (4):
>   device property: Introduce fwnode_get_name()
>   ACPI: property: Add acpi_fwnode_name()
>   of/property: Add of_fwnode_name()
>   device property: Drop get_named_child_node callback
> 
>  drivers/acpi/property.c  | 43 ++++++++++++++++++++++------------------
>  drivers/base/property.c  | 34 ++++++++++++++++++++++++++++++-
>  drivers/of/property.c    | 26 ++++++++++--------------
>  include/linux/fwnode.h   |  6 +++---
>  include/linux/property.h |  2 ++
>  5 files changed, 73 insertions(+), 38 deletions(-)
> 
> 

I will be expecting at least one more iteration of this series to address the
Rob's comments.

Thanks,
Rafael

Re: [PATCH v2 0/4] device property: Add fwnode_get_name() helper

[Heikki Krogerus]

On Tue, Nov 20, 2018 at 11:03:45PM +0100, Rafael J. Wysocki wrote:
> On Thursday, November 8, 2018 5:51:52 PM CET Heikki Krogerus wrote:
> > Hi,
> > 
> > This is the second version of my proposal for this helper. The
> > first version can be checked here:
> > https://lkml.org/lkml/2018/11/5/326
> > 
> > In order to support also ACPI properly, I decided to change the API.
> > The function fwnode_name() is now fwnode_get_name(), and instead of
> > returning pointer to the name, the function copies it to a buffer. I
> > did that because acpica does not offer a way to get a pointer to the
> > node name, and the name is clearly expected to be accessed only with
> > the namespace lock held.
> > 
> > I think this is better approach in any case. It will also solve the
> > problem of getting rid of the unit-address part from DT node names.
> > 
> > Let me know what you guys think.
> > 
> > --
> > heikki
> > 
> > 
> > Heikki Krogerus (4):
> >   device property: Introduce fwnode_get_name()
> >   ACPI: property: Add acpi_fwnode_name()
> >   of/property: Add of_fwnode_name()
> >   device property: Drop get_named_child_node callback
> > 
> >  drivers/acpi/property.c  | 43 ++++++++++++++++++++++------------------
> >  drivers/base/property.c  | 34 ++++++++++++++++++++++++++++++-
> >  drivers/of/property.c    | 26 ++++++++++--------------
> >  include/linux/fwnode.h   |  6 +++---
> >  include/linux/property.h |  2 ++
> >  5 files changed, 73 insertions(+), 38 deletions(-)
> > 
> > 
> 
> I will be expecting at least one more iteration of this series to address the
> Rob's comments.

Yes, I'll send a new version soon.

thanks,

-- 
heikki