[PATCH v4 0/8] Add of-graph helpers to loop over endpoints and find ports by id

[PATCH v4 0/8] Add of-graph helpers to loop over endpoints and find ports by id

[Philipp Zabel]

Hi,

in theory, I'd like the first five patches to go in together through Grant's
tree, but in practice that might not be necessary; changes in of_node
reference counting are only relevant if CONFIG_OF_DYNAMIC is enabled.
Since patch 2 depends on (or would trivially conflict with) 30e94a564d07
in the staging tree, it might be better to merge this one through staging.
What do you think?

The three 'use for_each_endpoint_of_node macro in ...' patches can be resent
later, separately.

This series converts all existing users of of_graph_get_next_endpoint that pass
a non-NULL prev argument to the function and decrement its refcount themselves
to stop doing that. The of_node_put is moved into of_graph_get_next_endpoint
instead.
This allows to add a for_each_endpoint_of_node helper macro to loop over all
endpoints in a device tree node.
The third of patch adds a of_graph_get_port_by_id function to retrieve a port
by its known port id from the device tree.
Finally, the last three patches convert functions in drm_of.c and imx-drm-core.c
to use the for_each_endpoint_of_node macro instead of of_graph_get_next_endpoint.

Changes since v3:
 - Moved of_node_put for the break case after the loop in patch 1 and 8
 - Rebased patch 2 on top of 30e94a564d07 (staging: imx-drm: Lines over 80
   characters fixed.), which added a blank line in the function we remove,
   imx_drm_of_get_next_endpoint.

The previous version can be found here: https://lkml.org/lkml/2014/9/11/696

regards
Philipp

Philipp Zabel (8):
  [media] soc_camera: Do not decrement endpoint node refcount in the
    loop
  imx-drm: Do not decrement endpoint node refcount in the loop
  of: Decrement refcount of previous endpoint in
    of_graph_get_next_endpoint
  of: Add for_each_endpoint_of_node helper macro
  of: Add of_graph_get_port_by_id function
  drm: use for_each_endpoint_of_node macro in drm_of_find_possible_crtcs
  imx-drm: use for_each_endpoint_of_node macro in
    imx_drm_encoder_get_mux_id
  imx-drm: use for_each_endpoint_of_node macro in
    imx_drm_encoder_parse_of

 drivers/gpu/drm/drm_of.c                       |  8 ++----
 drivers/media/platform/soc_camera/soc_camera.c |  3 ++-
 drivers/of/base.c                              | 35 ++++++++++++++++++++------
 drivers/staging/imx-drm/imx-drm-core.c         | 29 ++++++---------------
 include/linux/of_graph.h                       | 18 +++++++++++++
 5 files changed, 56 insertions(+), 37 deletions(-)

-- 
2.1.0

[PATCH v4 1/8] [media] soc_camera: Do not decrement endpoint node refcount in the loop

[Philipp Zabel]

In preparation for a following patch, stop decrementing the endpoint node
refcount in the loop. This temporarily leaks a reference to the endpoint node,
which will be fixed by having of_graph_get_next_endpoint decrement the refcount
of its prev argument instead.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v3:
 - Moved of_node_put for the break case after the loop
---
 drivers/media/platform/soc_camera/soc_camera.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index f4308fe..619b2d4 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1696,7 +1696,6 @@ static void scan_of_host(struct soc_camera_host *ici)
 		if (!i)
 			soc_of_bind(ici, epn, ren->parent);
 
-		of_node_put(epn);
 		of_node_put(ren);
 
 		if (i) {
@@ -1704,6 +1703,8 @@ static void scan_of_host(struct soc_camera_host *ici)
 			break;
 		}
 	}
+
+	of_node_put(epn);
 }
 
 #else
-- 
2.1.0

[PATCH v4 2/8] imx-drm: Do not decrement endpoint node refcount in the loop

[Philipp Zabel]

In preparation for the following patch, stop decrementing the endpoint node
refcount in the loop. This temporarily leaks a reference to the endpoint node,
which will be fixed by having of_graph_get_next_endpoint decrement the refcount
of its prev argument instead.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v3:
 - Rebased on top of 30e94a564d079f71f53368733720caa0c7c413c8
   (staging: imx-drm: Lines over 80 characters fixed.), which
   added a blank line in imx_drm_of_get_next_endpoint.
---
 drivers/staging/imx-drm/imx-drm-core.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
index 4289cc8..fab79ad 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -435,15 +435,6 @@ static uint32_t imx_drm_find_crtc_mask(struct imx_drm_device *imxdrm,
 	return 0;
 }
 
-static struct device_node *imx_drm_of_get_next_endpoint(
-		const struct device_node *parent, struct device_node *prev)
-{
-	struct device_node *node = of_graph_get_next_endpoint(parent, prev);
-
-	of_node_put(prev);
-	return node;
-}
-
 int imx_drm_encoder_parse_of(struct drm_device *drm,
 	struct drm_encoder *encoder, struct device_node *np)
 {
@@ -455,7 +446,7 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
 	for (i = 0; ; i++) {
 		u32 mask;
 
-		ep = imx_drm_of_get_next_endpoint(np, ep);
+		ep = of_graph_get_next_endpoint(np, ep);
 		if (!ep)
 			break;
 
@@ -503,7 +494,7 @@ int imx_drm_encoder_get_mux_id(struct device_node *node,
 		return -EINVAL;
 
 	do {
-		ep = imx_drm_of_get_next_endpoint(node, ep);
+		ep = of_graph_get_next_endpoint(node, ep);
 		if (!ep)
 			break;
 
-- 
2.1.0

[PATCH v4 3/8] of: Decrement refcount of previous endpoint in of_graph_get_next_endpoint

[Philipp Zabel]

Decrementing the reference count of the previous endpoint node allows to
use the of_graph_get_next_endpoint function in a for_each_... style macro.
Prior to this patch, all current users of this function that actually pass
a non-NULL prev parameter should be changed to not decrement the passed
prev argument's refcount themselves.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/of/base.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index d8574ad..a49b5628 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2058,8 +2058,7 @@ EXPORT_SYMBOL(of_graph_parse_endpoint);
  * @prev: previous endpoint node, or NULL to get first
  *
  * Return: An 'endpoint' node pointer with refcount incremented. Refcount
- * of the passed @prev node is not decremented, the caller have to use
- * of_node_put() on it when done.
+ * of the passed @prev node is decremented.
  */
 struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 					struct device_node *prev)
@@ -2095,12 +2094,6 @@ struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 		if (WARN_ONCE(!port, "%s(): endpoint %s has no parent node\n",
 			      __func__, prev->full_name))
 			return NULL;
-
-		/*
-		 * Avoid dropping prev node refcount to 0 when getting the next
-		 * child below.
-		 */
-		of_node_get(prev);
 	}
 
 	while (1) {
-- 
2.1.0

[PATCH v4 4/8] of: Add for_each_endpoint_of_node helper macro

[Philipp Zabel]

Note that while of_graph_get_next_endpoint decrements the reference count
of the child node passed to it, of_node_put(child) still has to be called
manually when breaking out of the loop.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/linux/of_graph.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index befef42..e43442e 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -26,6 +26,17 @@ struct of_endpoint {
 	const struct device_node *local_node;
 };
 
+/**
+ * for_each_endpoint_of_node - iterate over every endpoint in a device node
+ * @parent: parent device node containing ports and endpoints
+ * @child: loop variable pointing to the current endpoint node
+ *
+ * When breaking out of the loop, of_node_put(child) has to be called manually.
+ */
+#define for_each_endpoint_of_node(parent, child) \
+	for (child = of_graph_get_next_endpoint(parent, NULL); child != NULL; \
+	     child = of_graph_get_next_endpoint(parent, child))
+
 #ifdef CONFIG_OF
 int of_graph_parse_endpoint(const struct device_node *node,
 				struct of_endpoint *endpoint);
-- 
2.1.0

[PATCH v4 5/8] of: Add of_graph_get_port_by_id function

[Philipp Zabel]

This patch adds a function to get a port device tree node by port id,
or reg property value.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/of/base.c        | 26 ++++++++++++++++++++++++++
 include/linux/of_graph.h |  7 +++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index a49b5628..76e2651 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2053,6 +2053,32 @@ int of_graph_parse_endpoint(const struct device_node *node,
 EXPORT_SYMBOL(of_graph_parse_endpoint);
 
 /**
+ * of_graph_get_port_by_id() - get the port matching a given id
+ * @parent: pointer to the parent device node
+ * @id: id of the port
+ *
+ * Return: A 'port' node pointer with refcount incremented. The caller
+ * has to use of_node_put() on it when done.
+ */
+struct device_node *of_graph_get_port_by_id(struct device_node *node, u32 id)
+{
+	struct device_node *port;
+
+	for_each_child_of_node(node, port) {
+		u32 port_id = 0;
+
+		if (of_node_cmp(port->name, "port") != 0)
+			continue;
+		of_property_read_u32(port, "reg", &port_id);
+		if (id == port_id)
+			return port;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(of_graph_get_port_by_id);
+
+/**
  * of_graph_get_next_endpoint() - get next endpoint node
  * @parent: pointer to the parent device node
  * @prev: previous endpoint node, or NULL to get first
diff --git a/include/linux/of_graph.h b/include/linux/of_graph.h
index e43442e..3c1c95a 100644
--- a/include/linux/of_graph.h
+++ b/include/linux/of_graph.h
@@ -40,6 +40,7 @@ struct of_endpoint {
 #ifdef CONFIG_OF
 int of_graph_parse_endpoint(const struct device_node *node,
 				struct of_endpoint *endpoint);
+struct device_node *of_graph_get_port_by_id(struct device_node *node, u32 id);
 struct device_node *of_graph_get_next_endpoint(const struct device_node *parent,
 					struct device_node *previous);
 struct device_node *of_graph_get_remote_port_parent(
@@ -53,6 +54,12 @@ static inline int of_graph_parse_endpoint(const struct device_node *node,
 	return -ENOSYS;
 }
 
+static inline struct device_node *of_graph_get_port_by_id(
+					struct device_node *node, u32 id)
+{
+	return NULL;
+}
+
 static inline struct device_node *of_graph_get_next_endpoint(
 					const struct device_node *parent,
 					struct device_node *previous)
-- 
2.1.0

[PATCH v4 6/8] drm: use for_each_endpoint_of_node macro in drm_of_find_possible_crtcs

[Philipp Zabel]

Using the for_each_... macro should make the code a bit shorter and
easier to read.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/drm_of.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 16150a0..024fa77 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -46,11 +46,7 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 	struct device_node *remote_port, *ep = NULL;
 	uint32_t possible_crtcs = 0;
 
-	do {
-		ep = of_graph_get_next_endpoint(port, ep);
-		if (!ep)
-			break;
-
+	for_each_endpoint_of_node(port, ep) {
 		remote_port = of_graph_get_remote_port(ep);
 		if (!remote_port) {
 			of_node_put(ep);
@@ -60,7 +56,7 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 		possible_crtcs |= drm_crtc_port_mask(dev, remote_port);
 
 		of_node_put(remote_port);
-	} while (1);
+	}
 
 	return possible_crtcs;
 }
-- 
2.1.0

[PATCH v4 7/8] imx-drm: use for_each_endpoint_of_node macro in imx_drm_encoder_get_mux_id

[Philipp Zabel]

Using the for_each_... macro should make the code bit shorter and
easier to read. This patch also properly decrements the endpoint node
reference count before returning out of the loop.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/staging/imx-drm/imx-drm-core.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
index fab79ad..69e7194 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -493,18 +493,15 @@ int imx_drm_encoder_get_mux_id(struct device_node *node,
 	if (!node || !imx_crtc)
 		return -EINVAL;
 
-	do {
-		ep = of_graph_get_next_endpoint(node, ep);
-		if (!ep)
-			break;
-
+	for_each_endpoint_of_node(node, ep) {
 		port = of_graph_get_remote_port(ep);
 		of_node_put(port);
 		if (port == imx_crtc->port) {
 			ret = of_graph_parse_endpoint(ep, &endpoint);
+			of_node_put(ep);
 			return ret ? ret : endpoint.port;
 		}
-	} while (ep);
+	}
 
 	return -EINVAL;
 }
-- 
2.1.0

[PATCH v4 8/8] imx-drm: use for_each_endpoint_of_node macro in imx_drm_encoder_parse_of

[Philipp Zabel]

Using the for_each_... macro should make the code bit shorter and
easier to read. Since we can break out of the loop, we keep the
call to of_node_put after the loop.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v3:
 - Kept of_node_put after the loop, as in the earlier soc_camera patch.
   This reduces the line count and is safe to do since of_node_put(NULL)
   is a no-op.
---
 drivers/staging/imx-drm/imx-drm-core.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
index 69e7194..43285dc 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -439,17 +439,13 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
 	struct drm_encoder *encoder, struct device_node *np)
 {
 	struct imx_drm_device *imxdrm = drm->dev_private;
-	struct device_node *ep = NULL;
+	struct device_node *ep;
 	uint32_t crtc_mask = 0;
-	int i;
+	int i = 0;
 
-	for (i = 0; ; i++) {
+	for_each_endpoint_of_node(np, ep) {
 		u32 mask;
 
-		ep = of_graph_get_next_endpoint(np, ep);
-		if (!ep)
-			break;
-
 		mask = imx_drm_find_crtc_mask(imxdrm, ep);
 
 		/*
@@ -462,6 +458,7 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
 			return -EPROBE_DEFER;
 
 		crtc_mask |= mask;
+		i++;
 	}
 
 	of_node_put(ep);
-- 
2.1.0

Re: [PATCH v4 1/8] [media] soc_camera: Do not decrement endpoint node refcount in the loop

[Dan Carpenter]

On Mon, Sep 29, 2014 at 10:15:44AM +0200, Philipp Zabel wrote:
> In preparation for a following patch, stop decrementing the endpoint node
> refcount in the loop. This temporarily leaks a reference to the endpoint node,
> which will be fixed by having of_graph_get_next_endpoint decrement the refcount
> of its prev argument instead.

Don't do this...

My understanding (and I haven't invested much time into trying to
understand this beyond glancing at the change) is that patch 1 and 2,
introduce small bugs that are fixed in patch 3?

Just fold all three patches into one patch.  We need an Ack from Mauro
and Greg and then send the patch through Grant's tree.

regards,
dan carpenter

Re: [PATCH v4 1/8] [media] soc_camera: Do not decrement endpoint node refcount in the loop

[Philipp Zabel]

Am Montag, den 29.09.2014, 12:13 +0300 schrieb Dan Carpenter:
> On Mon, Sep 29, 2014 at 10:15:44AM +0200, Philipp Zabel wrote:
> > In preparation for a following patch, stop decrementing the endpoint node
> > refcount in the loop. This temporarily leaks a reference to the endpoint node,
> > which will be fixed by having of_graph_get_next_endpoint decrement the refcount
> > of its prev argument instead.
> 
> Don't do this...
> 
> My understanding (and I haven't invested much time into trying to
> understand this beyond glancing at the change) is that patch 1 and 2,
> introduce small bugs that are fixed in patch 3?
>
> Just fold all three patches into one patch.  We need an Ack from Mauro
> and Greg and then send the patch through Grant's tree.

Yes. Patches 1 and 2 leak a reference on of_nodes touched by the loop.
As far as I am aware, all users of this code don't use the reference
counting (CONFIG_OF_DYNAMIC is disabled), so this bug should be
theoretical.

I'd be happy do as you suggest if Mauro and Greg agree.

regards
Philipp

Re: [PATCH v4 1/8] [media] soc_camera: Do not decrement endpoint node refcount in the loop

[Greg Kroah-Hartman]

On Mon, Sep 29, 2014 at 11:45:23AM +0200, Philipp Zabel wrote:
> Am Montag, den 29.09.2014, 12:13 +0300 schrieb Dan Carpenter:
> > On Mon, Sep 29, 2014 at 10:15:44AM +0200, Philipp Zabel wrote:
> > > In preparation for a following patch, stop decrementing the endpoint node
> > > refcount in the loop. This temporarily leaks a reference to the endpoint node,
> > > which will be fixed by having of_graph_get_next_endpoint decrement the refcount
> > > of its prev argument instead.
> > 
> > Don't do this...
> > 
> > My understanding (and I haven't invested much time into trying to
> > understand this beyond glancing at the change) is that patch 1 and 2,
> > introduce small bugs that are fixed in patch 3?
> >
> > Just fold all three patches into one patch.  We need an Ack from Mauro
> > and Greg and then send the patch through Grant's tree.
> 
> Yes. Patches 1 and 2 leak a reference on of_nodes touched by the loop.
> As far as I am aware, all users of this code don't use the reference
> counting (CONFIG_OF_DYNAMIC is disabled), so this bug should be
> theoretical.
> 
> I'd be happy do as you suggest if Mauro and Greg agree.

Let's see the correct patch, don't break things and then later on fix
them...

thanks,

greg k-h