[PATCH] media: xilinx-video: fix bad of_node_put() on endpoint error

[PATCH] media: xilinx-video: fix bad of_node_put() on endpoint error

[Akinobu Mita]

When iterating through all endpoints using of_graph_get_next_endpoint(),
the refcount of the returned endpoint node is incremented and the refcount
of the node which is passed as previous endpoint is decremented.

So the caller doesn't need to call of_node_put() for each iterated node
except for error exit paths.  Otherwise we get "OF: ERROR: Bad
of_node_put() on ..." messages.

Cc: Hyun Kwon <hyun.kwon@xilinx.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/media/platform/xilinx/xilinx-vipp.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
index ebfdf33..e5c80c9 100644
--- a/drivers/media/platform/xilinx/xilinx-vipp.c
+++ b/drivers/media/platform/xilinx/xilinx-vipp.c
@@ -76,20 +76,16 @@ static int xvip_graph_build_one(struct xvip_composite_device *xdev,
 	struct xvip_graph_entity *ent;
 	struct v4l2_fwnode_link link;
 	struct device_node *ep = NULL;
-	struct device_node *next;
 	int ret = 0;
 
 	dev_dbg(xdev->dev, "creating links for entity %s\n", local->name);
 
 	while (1) {
 		/* Get the next endpoint and parse its link. */
-		next = of_graph_get_next_endpoint(entity->node, ep);
-		if (next == NULL)
+		ep = of_graph_get_next_endpoint(entity->node, ep);
+		if (ep == NULL)
 			break;
 
-		of_node_put(ep);
-		ep = next;
-
 		dev_dbg(xdev->dev, "processing endpoint %pOF\n", ep);
 
 		ret = v4l2_fwnode_parse_link(of_fwnode_handle(ep), &link);
@@ -200,7 +196,6 @@ static int xvip_graph_build_dma(struct xvip_composite_device *xdev)
 	struct xvip_graph_entity *ent;
 	struct v4l2_fwnode_link link;
 	struct device_node *ep = NULL;
-	struct device_node *next;
 	struct xvip_dma *dma;
 	int ret = 0;
 
@@ -208,13 +203,10 @@ static int xvip_graph_build_dma(struct xvip_composite_device *xdev)
 
 	while (1) {
 		/* Get the next endpoint and parse its link. */
-		next = of_graph_get_next_endpoint(node, ep);
-		if (next == NULL)
+		ep = of_graph_get_next_endpoint(node, ep);
+		if (ep == NULL)
 			break;
 
-		of_node_put(ep);
-		ep = next;
-
 		dev_dbg(xdev->dev, "processing endpoint %pOF\n", ep);
 
 		ret = v4l2_fwnode_parse_link(of_fwnode_handle(ep), &link);
-- 
2.7.4

Re: [PATCH] media: xilinx-video: fix bad of_node_put() on endpoint error

[Laurent Pinchart]

Hi Akinobu,

Thank you for the patch.

On Thursday, 12 October 2017 19:04:44 EET Akinobu Mita wrote:
> When iterating through all endpoints using of_graph_get_next_endpoint(),
> the refcount of the returned endpoint node is incremented and the refcount
> of the node which is passed as previous endpoint is decremented.
> 
> So the caller doesn't need to call of_node_put() for each iterated node
> except for error exit paths.  Otherwise we get "OF: ERROR: Bad
> of_node_put() on ..." messages.
> 
> Cc: Hyun Kwon <hyun.kwon@xilinx.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and applied to my tree.

> ---
>  drivers/media/platform/xilinx/xilinx-vipp.c | 16 ++++------------
>  1 file changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c
> b/drivers/media/platform/xilinx/xilinx-vipp.c index ebfdf33..e5c80c9 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -76,20 +76,16 @@ static int xvip_graph_build_one(struct
> xvip_composite_device *xdev, struct xvip_graph_entity *ent;
>  	struct v4l2_fwnode_link link;
>  	struct device_node *ep = NULL;
> -	struct device_node *next;
>  	int ret = 0;
> 
>  	dev_dbg(xdev->dev, "creating links for entity %s\n", local->name);
> 
>  	while (1) {
>  		/* Get the next endpoint and parse its link. */
> -		next = of_graph_get_next_endpoint(entity->node, ep);
> -		if (next == NULL)
> +		ep = of_graph_get_next_endpoint(entity->node, ep);
> +		if (ep == NULL)
>  			break;
> 
> -		of_node_put(ep);
> -		ep = next;
> -
>  		dev_dbg(xdev->dev, "processing endpoint %pOF\n", ep);
> 
>  		ret = v4l2_fwnode_parse_link(of_fwnode_handle(ep), &link);
> @@ -200,7 +196,6 @@ static int xvip_graph_build_dma(struct
> xvip_composite_device *xdev) struct xvip_graph_entity *ent;
>  	struct v4l2_fwnode_link link;
>  	struct device_node *ep = NULL;
> -	struct device_node *next;
>  	struct xvip_dma *dma;
>  	int ret = 0;
> 
> @@ -208,13 +203,10 @@ static int xvip_graph_build_dma(struct
> xvip_composite_device *xdev)
> 
>  	while (1) {
>  		/* Get the next endpoint and parse its link. */
> -		next = of_graph_get_next_endpoint(node, ep);
> -		if (next == NULL)
> +		ep = of_graph_get_next_endpoint(node, ep);
> +		if (ep == NULL)
>  			break;
> 
> -		of_node_put(ep);
> -		ep = next;
> -
>  		dev_dbg(xdev->dev, "processing endpoint %pOF\n", ep);
> 
>  		ret = v4l2_fwnode_parse_link(of_fwnode_handle(ep), &link);

-- 
Regards,

Laurent Pinchart

[PATCH] media: xilinx-video: fix bad of_node_put() on endpoint error

[Akinobu Mita]

The fwnode_graph_get_next_endpoint() returns an 'endpoint' node pointer
with refcount incremented, and refcount of the passed as a previous
'endpoint' node is decremented.

So when iterating over all nodes using fwnode_graph_get_next_endpoint(),
we don't need to call fwnode_handle_put() for each node except for error
exit paths.  Otherwise we get "OF: ERROR: Bad of_node_put() on ..."
messages.

Fixes: d079f94c9046 ("media: platform: Switch to v4l2_async_notifier_add_subdev")
Cc: Steve Longerbeam <slongerbeam@gmail.com>
Cc: Hyun Kwon <hyun.kwon@xilinx.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
 drivers/media/platform/xilinx/xilinx-vipp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
index 574614d..26b13fd 100644
--- a/drivers/media/platform/xilinx/xilinx-vipp.c
+++ b/drivers/media/platform/xilinx/xilinx-vipp.c
@@ -377,8 +377,6 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
 			goto err_notifier_cleanup;
 		}
 
-		fwnode_handle_put(ep);
-
 		/* Skip entities that we have already processed. */
 		if (remote == of_fwnode_handle(xdev->dev->of_node) ||
 		    xvip_graph_find_entity(xdev, remote)) {
-- 
2.7.4

Re: [PATCH] media: xilinx-video: fix bad of_node_put() on endpoint error

[Hyun Kwon]

Hi Akinobu,

Thanks for the patch.

On Sun, 2018-11-04 at 05:11:10 -0800, Akinobu Mita wrote:
> The fwnode_graph_get_next_endpoint() returns an 'endpoint' node pointer
> with refcount incremented, and refcount of the passed as a previous
> 'endpoint' node is decremented.
> 
> So when iterating over all nodes using fwnode_graph_get_next_endpoint(),
> we don't need to call fwnode_handle_put() for each node except for error
> exit paths.  Otherwise we get "OF: ERROR: Bad of_node_put() on ..."
> messages.
> 
> Fixes: d079f94c9046 ("media: platform: Switch to v4l2_async_notifier_add_subdev")
> Cc: Steve Longerbeam <slongerbeam@gmail.com>
> Cc: Hyun Kwon <hyun.kwon@xilinx.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>

This looks good to me,

     Reviewed-by: Hyun Kwon <hyun.kwon@xilinx.com>

Thanks,
-hyun

> ---
>  drivers/media/platform/xilinx/xilinx-vipp.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
> index 574614d..26b13fd 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -377,8 +377,6 @@ static int xvip_graph_parse_one(struct xvip_composite_device *xdev,
>  			goto err_notifier_cleanup;
>  		}
>  
> -		fwnode_handle_put(ep);
> -
>  		/* Skip entities that we have already processed. */
>  		if (remote == of_fwnode_handle(xdev->dev->of_node) ||
>  		    xvip_graph_find_entity(xdev, remote)) {
> -- 
> 2.7.4
>