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

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

[Philipp Zabel]

Hi,

now the first three patches of the previous series are combined into one. This
patch touches of, media, and staging. Since it is rebased onto Grant's tree, it
now trivially conflicts with 30e94a564d07 (staging: imx-drm: Lines over 80
characters fixed.)

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 v4:
 - Combined patches 1-3 into one

The previous version can be found here: https://lkml.org/lkml/2014/9/29/78

regards
Philipp

Philipp Zabel (6):
  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         | 28 ++++++---------------
 include/linux/of_graph.h                       | 18 +++++++++++++
 5 files changed, 56 insertions(+), 36 deletions(-)

-- 
2.1.0

[PATCH v5 1/6] 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.
All current users of this function that pass a non-NULL prev parameter
(that is, soc_camera and imx-drm) are changed to not decrement the passed
prev argument's refcount themselves.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v4:
 - Folded patches 1-3 into this one
---
 drivers/media/platform/soc_camera/soc_camera.c |  3 ++-
 drivers/of/base.c                              |  9 +--------
 drivers/staging/imx-drm/imx-drm-core.c         | 12 ++----------
 3 files changed, 5 insertions(+), 19 deletions(-)

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
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 293ed4b..f7a9aa8 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2070,8 +2070,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)
@@ -2107,12 +2106,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) {
diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
index 6b22106..12303b3 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -434,14 +434,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)
 {
@@ -453,7 +445,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;
 
@@ -502,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 v5 2/6] 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 v5 3/6] 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 f7a9aa8..e17e534 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2065,6 +2065,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 v5 4/6] 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 v5 5/6] 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 12303b3..9b5222c 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 v5 6/6] 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>
---
 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 9b5222c..8f2a802 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -438,17 +438,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);
 
 		/*
@@ -461,6 +457,7 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
 			return -EPROBE_DEFER;
 
 		crtc_mask |= mask;
+		i++;
 	}
 
 	if (ep)
-- 
2.1.0

Re: [PATCH v5 1/6] of: Decrement refcount of previous endpoint in of_graph_get_next_endpoint

[Greg Kroah-Hartman]

On Mon, Sep 29, 2014 at 08:03:34PM +0200, Philipp Zabel wrote:
> 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.
> All current users of this function that pass a non-NULL prev parameter
> (that is, soc_camera and imx-drm) are changed to not decrement the passed
> prev argument's refcount themselves.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v4:
>  - Folded patches 1-3 into this one
> ---
>  drivers/media/platform/soc_camera/soc_camera.c |  3 ++-
>  drivers/of/base.c                              |  9 +--------
>  drivers/staging/imx-drm/imx-drm-core.c         | 12 ++----------
>  3 files changed, 5 insertions(+), 19 deletions(-)

No objection from me for this, but Grant is in "charge" of
drivers/of/base.c, so I'll leave it for him to apply.

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

thanks,

greg k-h

Re: [PATCH v5 1/6] of: Decrement refcount of previous endpoint in of_graph_get_next_endpoint

[Philipp Zabel]

Am Montag, den 29.09.2014, 18:10 -0400 schrieb Greg Kroah-Hartman:
> On Mon, Sep 29, 2014 at 08:03:34PM +0200, Philipp Zabel wrote:
> > 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.
> > All current users of this function that pass a non-NULL prev parameter
> > (that is, soc_camera and imx-drm) are changed to not decrement the passed
> > prev argument's refcount themselves.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > Changes since v4:
> >  - Folded patches 1-3 into this one
> > ---
> >  drivers/media/platform/soc_camera/soc_camera.c |  3 ++-
> >  drivers/of/base.c                              |  9 +--------
> >  drivers/staging/imx-drm/imx-drm-core.c         | 12 ++----------
> >  3 files changed, 5 insertions(+), 19 deletions(-)
> 
> No objection from me for this, but Grant is in "charge" of
> drivers/of/base.c, so I'll leave it for him to apply.
> 
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thank you. Guennadi, Mauro, could I get your ack on the soc_camera part?

regards
Philipp

Re: [PATCH v5 1/6] of: Decrement refcount of previous endpoint in of_graph_get_next_endpoint

[Philipp Zabel]

Hi Mauro, Guennadi,

Am Dienstag, den 30.09.2014, 10:06 +0200 schrieb Philipp Zabel:
> Am Montag, den 29.09.2014, 18:10 -0400 schrieb Greg Kroah-Hartman:
> > On Mon, Sep 29, 2014 at 08:03:34PM +0200, Philipp Zabel wrote:
> > > 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.
> > > All current users of this function that pass a non-NULL prev parameter
> > > (that is, soc_camera and imx-drm) are changed to not decrement the passed
> > > prev argument's refcount themselves.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > ---
> > > Changes since v4:
> > >  - Folded patches 1-3 into this one
> > > ---
> > >  drivers/media/platform/soc_camera/soc_camera.c |  3 ++-
> > >  drivers/of/base.c                              |  9 +--------
> > >  drivers/staging/imx-drm/imx-drm-core.c         | 12 ++----------
> > >  3 files changed, 5 insertions(+), 19 deletions(-)
> > 
> > No objection from me for this, but Grant is in "charge" of
> > drivers/of/base.c, so I'll leave it for him to apply.
> > 
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Thank you. Guennadi, Mauro, could I get your ack on the soc_camera part?

I'd really like to get this series merged through Grant's tree, but
since it touches the soc_camera core, could you please give me an ack
for this?

regards
Philipp

Re: [PATCH v5 1/6] of: Decrement refcount of previous endpoint in of_graph_get_next_endpoint

[Mauro Carvalho Chehab]

Em Mon, 29 Sep 2014 20:03:34 +0200
Philipp Zabel <p.zabel@pengutronix.de> escreveu:

> 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.
> All current users of this function that pass a non-NULL prev parameter
> (that is, soc_camera and imx-drm) are changed to not decrement the passed
> prev argument's refcount themselves.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Feel free to merge it via Grant's tree.

Acked-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

> ---
> Changes since v4:
>  - Folded patches 1-3 into this one
> ---
>  drivers/media/platform/soc_camera/soc_camera.c |  3 ++-
>  drivers/of/base.c                              |  9 +--------
>  drivers/staging/imx-drm/imx-drm-core.c         | 12 ++----------
>  3 files changed, 5 insertions(+), 19 deletions(-)
> 
> 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
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 293ed4b..f7a9aa8 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2070,8 +2070,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)
> @@ -2107,12 +2106,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) {
> diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
> index 6b22106..12303b3 100644
> --- a/drivers/staging/imx-drm/imx-drm-core.c
> +++ b/drivers/staging/imx-drm/imx-drm-core.c
> @@ -434,14 +434,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)
>  {
> @@ -453,7 +445,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;
>  
> @@ -502,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;
>  

Re: [PATCH v5 1/6] of: Decrement refcount of previous endpoint in of_graph_get_next_endpoint

[Guennadi Liakhovetski]

Hi Philipp,

Thanks for the patch and sorry for a late reply. I did look at your 
patches earlier too, but maybe not attentively enough, or maybe I'm 
misunderstanding something now. In the scan_of_host() function in 
soc_camera.c as of current -next I see:

		epn = of_graph_get_next_endpoint(np, epn);

which already looks like a refcount leak to me. If epn != NULL, its 
refcount is incremented, but then immediately the variable gets 
overwritten, and there's no extra copy of that variable to fix this. If 
I'm right, then that bug in itself should be fixed, ideally before your 
patch is applied. But in fact, your patch fixes this, since it modifies 
of_graph_get_next_endpoint() to return with prev's refcount not 
incremented, right? Whereas the of_node_put(epn) later down in 
scan_of_host() decrements refcount of the _next_ endpoint, not the 
previous one, so, it should be left alone? I.e. AFAICT your modification 
to of_graph_get_next_endpoint() fixes soc_camera.c with no further 
modifications to it required?

Thanks
Guennadi

On Mon, 29 Sep 2014, Philipp Zabel wrote:

> 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.
> All current users of this function that pass a non-NULL prev parameter
> (that is, soc_camera and imx-drm) are changed to not decrement the passed
> prev argument's refcount themselves.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v4:
>  - Folded patches 1-3 into this one
> ---
>  drivers/media/platform/soc_camera/soc_camera.c |  3 ++-
>  drivers/of/base.c                              |  9 +--------
>  drivers/staging/imx-drm/imx-drm-core.c         | 12 ++----------
>  3 files changed, 5 insertions(+), 19 deletions(-)
> 
> 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
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 293ed4b..f7a9aa8 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2070,8 +2070,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)
> @@ -2107,12 +2106,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) {
> diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
> index 6b22106..12303b3 100644
> --- a/drivers/staging/imx-drm/imx-drm-core.c
> +++ b/drivers/staging/imx-drm/imx-drm-core.c
> @@ -434,14 +434,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)
>  {
> @@ -453,7 +445,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;
>  
> @@ -502,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
> 

Re: [PATCH v5 1/6] of: Decrement refcount of previous endpoint in of_graph_get_next_endpoint

[Philipp Zabel]

Hi Guennadi,

On Fri, Nov 07, 2014 at 11:06:21PM +0100, Guennadi Liakhovetski wrote:
> Hi Philipp,
> 
> Thanks for the patch and sorry for a late reply. I did look at your 
> patches earlier too, but maybe not attentively enough, or maybe I'm 
> misunderstanding something now. In the scan_of_host() function in 
> soc_camera.c as of current -next I see:
> 
> 		epn = of_graph_get_next_endpoint(np, epn);
> 
> which already looks like a refcount leak to me. If epn != NULL, its 
> refcount is incremented, but then immediately the variable gets 
> overwritten, and there's no extra copy of that variable to fix this. If 
> I'm right, then that bug in itself should be fixed, ideally before your 
> patch is applied. But in fact, your patch fixes this, since it modifies 
> of_graph_get_next_endpoint() to return with prev's refcount not 
> incremented, right? Whereas the of_node_put(epn) later down in 
> scan_of_host() decrements refcount of the _next_ endpoint, not the 
> previous one, so, it should be left alone? I.e. AFAICT your modification 
> to of_graph_get_next_endpoint() fixes soc_camera.c with no further 
> modifications to it required?

You are right. With the old implementation, you'd have to do the
epn = of_graph_get_next_endpoint(np, prev); of_node_put(prev); prev = epn;
dance to avoid leaking a reference to the first endpoint. This series
accidentally fixes soc_camera by changing of_graph_get_next_endpoint
to decrement the reference count itself.

regards
Philipp

Re: [PATCH v5 1/6] of: Decrement refcount of previous endpoint in of_graph_get_next_endpoint

[Guennadi Liakhovetski]

On Sun, 9 Nov 2014, Philipp Zabel wrote:

> Hi Guennadi,
> 
> On Fri, Nov 07, 2014 at 11:06:21PM +0100, Guennadi Liakhovetski wrote:
> > Hi Philipp,
> > 
> > Thanks for the patch and sorry for a late reply. I did look at your 
> > patches earlier too, but maybe not attentively enough, or maybe I'm 
> > misunderstanding something now. In the scan_of_host() function in 
> > soc_camera.c as of current -next I see:
> > 
> > 		epn = of_graph_get_next_endpoint(np, epn);
> > 
> > which already looks like a refcount leak to me. If epn != NULL, its 
> > refcount is incremented, but then immediately the variable gets 
> > overwritten, and there's no extra copy of that variable to fix this. If 
> > I'm right, then that bug in itself should be fixed, ideally before your 
> > patch is applied. But in fact, your patch fixes this, since it modifies 
> > of_graph_get_next_endpoint() to return with prev's refcount not 
> > incremented, right? Whereas the of_node_put(epn) later down in 
> > scan_of_host() decrements refcount of the _next_ endpoint, not the 
> > previous one, so, it should be left alone? I.e. AFAICT your modification 
> > to of_graph_get_next_endpoint() fixes soc_camera.c with no further 
> > modifications to it required?
> 
> You are right. With the old implementation, you'd have to do the
> epn = of_graph_get_next_endpoint(np, prev); of_node_put(prev); prev = epn;
> dance to avoid leaking a reference to the first endpoint. This series
> accidentally fixes soc_camera by changing of_graph_get_next_endpoint
> to decrement the reference count itself.

Right, so, the patch has to be adjusted not to touch soc_camera.c at all.

Thanks
Guennadi

> 
> regards
> Philipp
> 

Re: [PATCH v5 1/6] of: Decrement refcount of previous endpoint in of_graph_get_next_endpoint

[Philipp Zabel]

Am Sonntag, den 09.11.2014, 16:51 +0100 schrieb Guennadi Liakhovetski:
> On Sun, 9 Nov 2014, Philipp Zabel wrote:
> 
> > Hi Guennadi,
> > 
> > On Fri, Nov 07, 2014 at 11:06:21PM +0100, Guennadi Liakhovetski wrote:
> > > Hi Philipp,
> > > 
> > > Thanks for the patch and sorry for a late reply. I did look at your 
> > > patches earlier too, but maybe not attentively enough, or maybe I'm 
> > > misunderstanding something now. In the scan_of_host() function in 
> > > soc_camera.c as of current -next I see:
> > > 
> > > 		epn = of_graph_get_next_endpoint(np, epn);
> > > 
> > > which already looks like a refcount leak to me. If epn != NULL, its 
> > > refcount is incremented, but then immediately the variable gets 
> > > overwritten, and there's no extra copy of that variable to fix this. If 
> > > I'm right, then that bug in itself should be fixed, ideally before your 
> > > patch is applied. But in fact, your patch fixes this, since it modifies 
> > > of_graph_get_next_endpoint() to return with prev's refcount not 
> > > incremented, right? Whereas the of_node_put(epn) later down in 
> > > scan_of_host() decrements refcount of the _next_ endpoint, not the 
> > > previous one, so, it should be left alone? I.e. AFAICT your modification 
> > > to of_graph_get_next_endpoint() fixes soc_camera.c with no further 
> > > modifications to it required?
> > 
> > You are right. With the old implementation, you'd have to do the
> > epn = of_graph_get_next_endpoint(np, prev); of_node_put(prev); prev = epn;
> > dance to avoid leaking a reference to the first endpoint. This series
> > accidentally fixes soc_camera by changing of_graph_get_next_endpoint
> > to decrement the reference count itself.
> 
> Right, so, the patch has to be adjusted not to touch soc_camera.c at all.

No. As of the current media-tree we still need move the of_node_put(epn)
out of the loop:

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

We can do this in two steps (step 1 fixing the current status quo, step
2 as part of this series). Step 1:

--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1691,11 +1691,13 @@ static void scan_of_host(struct soc_camera_host *ici)
 {
 	struct device *dev = ici->v4l2_dev.dev;
 	struct device_node *np = dev->of_node;
-	struct device_node *epn = NULL, *ren;
+	struct device_node *epn, *prev = NULL, *ren;
 	unsigned int i;
 
 	for (i = 0; ; i++) {
-		epn = of_graph_get_next_endpoint(np, epn);
+		epn = of_graph_get_next_endpoint(np, prev);
+		of_node_put(prev);
+		prev = epn;
 		if (!epn)
 			break;
 
@@ -1710,7 +1712,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) {

And step 2:

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 3d44773..1de62bf 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1691,13 +1691,11 @@ static void scan_of_host(struct soc_camera_host *ici)
 {
 	struct device *dev = ici->v4l2_dev.dev;
 	struct device_node *np = dev->of_node;
-	struct device_node *epn, *prev = NULL, *ren;
+	struct device_node *epn = NULL, *ren;
 	unsigned int i;
 
 	for (i = 0; ; i++) {
-		epn = of_graph_get_next_endpoint(np, prev);
-		of_node_put(prev);
-		prev = epn;
+		epn = of_graph_get_next_endpoint(np, epn);
 		if (!epn)
 			break;
 
@@ -1719,6 +1717,8 @@ static void scan_of_host(struct soc_camera_host *ici)
 			break;
 		}
 	}
+
+	of_node_put(epn);
 }
 
 #else

Would you prefer that option?

regards
Philipp