[PATCH v2 resend 0/8] use for_each_endpoint_of_node()

[PATCH v2 resend 0/8] use for_each_endpoint_of_node()

[Kuninori Morimoto]

Hi Rob, Helge

This is resend of v2 patch-set

We already have for_each_endpoint_of_node(), but some drivers are
not using it. This patch-set replace it.

This patch-set is related to "OF" (= Rob), but many driveres are for
"MultiMedia" (= Helge). I'm not sure who handle these.

I will re-post this patch-set because I couldn't find these on
linus/master branch.

[o] done
[*] this patch-set

	[o] tidyup of_graph_get_endpoint_count()
	[o] replace endpoint func - use endpoint_by_regs()
	[*] replace endpoint func - use for_each()
	[ ] rename endpoint func to device_endpoint
	[ ] add new port function
	[ ] add new endpont function
	[ ] remove of_graph_get_next_device_endpoint()

v1 -> v2
	- fixup TI patch

Link: https://lore.kernel.org/r/8734sf6mgn.wl-kuninori.morimoto.gx@renesas.com

Kuninori Morimoto (8):
  gpu: drm: use for_each_endpoint_of_node()
  hwtracing: use for_each_endpoint_of_node()
  media: platform: microchip: use for_each_endpoint_of_node()
  media: platform: ti: use for_each_endpoint_of_node()
  media: platform: xilinx: use for_each_endpoint_of_node()
  staging: media: atmel: use for_each_endpoint_of_node()
  video: fbdev: use for_each_endpoint_of_node()
  fbdev: omapfb: use of_graph_get_remote_port()

 drivers/gpu/drm/omapdrm/dss/base.c            |  3 +--
 .../hwtracing/coresight/coresight-platform.c  |  4 ++--
 .../microchip/microchip-sama5d2-isc.c         | 19 +++++++------------
 .../microchip/microchip-sama7g5-isc.c         | 19 +++++++------------
 .../media/platform/ti/am437x/am437x-vpfe.c    | 12 +++++-------
 .../media/platform/ti/davinci/vpif_capture.c  | 12 ++++++------
 drivers/media/platform/xilinx/xilinx-vipp.c   |  7 +------
 .../deprecated/atmel/atmel-sama5d2-isc.c      |  6 +-----
 .../deprecated/atmel/atmel-sama7g5-isc.c      |  6 +-----
 drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 15 +--------------
 .../omap2/omapfb/dss/omapdss-boot-init.c      |  3 +--
 11 files changed, 33 insertions(+), 73 deletions(-)

-- 
2.43.0

[PATCH v2 resend 1/8] gpu: drm: use for_each_endpoint_of_node()

[Kuninori Morimoto]

We already have for_each_endpoint_of_node(), don't use
of_graph_get_next_endpoint() directly. Replace it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/gpu/drm/omapdrm/dss/base.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/base.c b/drivers/gpu/drm/omapdrm/dss/base.c
index 050ca7eafac58..5f8002f6bb7a5 100644
--- a/drivers/gpu/drm/omapdrm/dss/base.c
+++ b/drivers/gpu/drm/omapdrm/dss/base.c
@@ -242,8 +242,7 @@ static void omapdss_walk_device(struct device *dev, struct device_node *node,
 
 	of_node_put(n);
 
-	n = NULL;
-	while ((n = of_graph_get_next_endpoint(node, n)) != NULL) {
+	for_each_endpoint_of_node(node, n) {
 		struct device_node *pn = of_graph_get_remote_port_parent(n);
 
 		if (!pn)
-- 
2.43.0

[PATCH v2 resend 2/8] hwtracing: use for_each_endpoint_of_node()

[Kuninori Morimoto]

We already have for_each_endpoint_of_node(), don't use
of_graph_get_next_endpoint() directly. Replace it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
index 9d550f5697fa8..e9683e613d520 100644
--- a/drivers/hwtracing/coresight/coresight-platform.c
+++ b/drivers/hwtracing/coresight/coresight-platform.c
@@ -275,7 +275,7 @@ static int of_get_coresight_platform_data(struct device *dev,
 	 */
 	if (!parent) {
 		/*
-		 * Avoid warnings in of_graph_get_next_endpoint()
+		 * Avoid warnings in for_each_endpoint_of_node()
 		 * if the device doesn't have any graph connections
 		 */
 		if (!of_graph_is_present(node))
@@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev,
 	}
 
 	/* Iterate through each output port to discover topology */
-	while ((ep = of_graph_get_next_endpoint(parent, ep))) {
+	for_each_endpoint_of_node(parent, ep) {
 		/*
 		 * Legacy binding mixes input/output ports under the
 		 * same parent. So, skip the input ports if we are dealing
-- 
2.43.0

[PATCH v2 resend 3/8] media: platform: microchip: use for_each_endpoint_of_node()

[Kuninori Morimoto]

We already have for_each_endpoint_of_node(), don't use
of_graph_get_next_endpoint() directly. Replace it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 .../microchip/microchip-sama5d2-isc.c         | 19 +++++++------------
 .../microchip/microchip-sama7g5-isc.c         | 19 +++++++------------
 2 files changed, 14 insertions(+), 24 deletions(-)

diff --git a/drivers/media/platform/microchip/microchip-sama5d2-isc.c b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
index 5ac149cf3647f..d9298771f5097 100644
--- a/drivers/media/platform/microchip/microchip-sama5d2-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
@@ -356,30 +356,26 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
 	struct device_node *epn = NULL;
 	struct isc_subdev_entity *subdev_entity;
 	unsigned int flags;
-	int ret;
 
 	INIT_LIST_HEAD(&isc->subdev_entities);
 
-	while (1) {
+	for_each_endpoint_of_node(np, epn) {
 		struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
-
-		epn = of_graph_get_next_endpoint(np, epn);
-		if (!epn)
-			return 0;
+		int ret;
 
 		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
 						 &v4l2_epn);
 		if (ret) {
-			ret = -EINVAL;
+			of_node_put(epn);
 			dev_err(dev, "Could not parse the endpoint\n");
-			break;
+			return -EINVAL;
 		}
 
 		subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
 					     GFP_KERNEL);
 		if (!subdev_entity) {
-			ret = -ENOMEM;
-			break;
+			of_node_put(epn);
+			return -ENOMEM;
 		}
 		subdev_entity->epn = epn;
 
@@ -400,9 +396,8 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
 
 		list_add_tail(&subdev_entity->list, &isc->subdev_entities);
 	}
-	of_node_put(epn);
 
-	return ret;
+	return 0;
 }
 
 static int microchip_isc_probe(struct platform_device *pdev)
diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
index 73445f33d26ba..36204fee10aa2 100644
--- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
+++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
@@ -339,33 +339,29 @@ static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
 	struct device_node *epn = NULL;
 	struct isc_subdev_entity *subdev_entity;
 	unsigned int flags;
-	int ret;
 	bool mipi_mode;
 
 	INIT_LIST_HEAD(&isc->subdev_entities);
 
 	mipi_mode = of_property_read_bool(np, "microchip,mipi-mode");
 
-	while (1) {
+	for_each_endpoint_of_node(np, epn) {
 		struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
-
-		epn = of_graph_get_next_endpoint(np, epn);
-		if (!epn)
-			return 0;
+		int ret;
 
 		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
 						 &v4l2_epn);
 		if (ret) {
-			ret = -EINVAL;
+			of_node_put(epn);
 			dev_err(dev, "Could not parse the endpoint\n");
-			break;
+			return -EINVAL;
 		}
 
 		subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
 					     GFP_KERNEL);
 		if (!subdev_entity) {
-			ret = -ENOMEM;
-			break;
+			of_node_put(epn);
+			return -ENOMEM;
 		}
 		subdev_entity->epn = epn;
 
@@ -389,9 +385,8 @@ static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
 
 		list_add_tail(&subdev_entity->list, &isc->subdev_entities);
 	}
-	of_node_put(epn);
 
-	return ret;
+	return 0;
 }
 
 static int microchip_xisc_probe(struct platform_device *pdev)
-- 
2.43.0

[PATCH v2 resend 4/8] media: platform: ti: use for_each_endpoint_of_node()

[Kuninori Morimoto]

We already have for_each_endpoint_of_node(), don't use
of_graph_get_next_endpoint() directly. Replace it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/media/platform/ti/am437x/am437x-vpfe.c   | 12 +++++-------
 drivers/media/platform/ti/davinci/vpif_capture.c | 12 ++++++------
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/ti/am437x/am437x-vpfe.c b/drivers/media/platform/ti/am437x/am437x-vpfe.c
index 77e12457d1495..009ff68a2b43c 100644
--- a/drivers/media/platform/ti/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/ti/am437x/am437x-vpfe.c
@@ -2287,7 +2287,7 @@ static const struct v4l2_async_notifier_operations vpfe_async_ops = {
 static struct vpfe_config *
 vpfe_get_pdata(struct vpfe_device *vpfe)
 {
-	struct device_node *endpoint = NULL;
+	struct device_node *endpoint;
 	struct device *dev = vpfe->pdev;
 	struct vpfe_subdev_info *sdinfo;
 	struct vpfe_config *pdata;
@@ -2306,14 +2306,11 @@ vpfe_get_pdata(struct vpfe_device *vpfe)
 	if (!pdata)
 		return NULL;
 
-	for (i = 0; ; i++) {
+	i = 0;
+	for_each_endpoint_of_node(dev->of_node, endpoint) {
 		struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
 		struct device_node *rem;
 
-		endpoint = of_graph_get_next_endpoint(dev->of_node, endpoint);
-		if (!endpoint)
-			break;
-
 		sdinfo = &pdata->sub_devs[i];
 		sdinfo->grp_id = 0;
 
@@ -2371,9 +2368,10 @@ vpfe_get_pdata(struct vpfe_device *vpfe)
 		of_node_put(rem);
 		if (IS_ERR(pdata->asd[i]))
 			goto cleanup;
+
+		i++;
 	}
 
-	of_node_put(endpoint);
 	return pdata;
 
 cleanup:
diff --git a/drivers/media/platform/ti/davinci/vpif_capture.c b/drivers/media/platform/ti/davinci/vpif_capture.c
index c28794b6677b7..078ae11cd0787 100644
--- a/drivers/media/platform/ti/davinci/vpif_capture.c
+++ b/drivers/media/platform/ti/davinci/vpif_capture.c
@@ -1517,16 +1517,12 @@ vpif_capture_get_pdata(struct platform_device *pdev,
 	if (!pdata->subdev_info)
 		return NULL;
 
-	for (i = 0; i < VPIF_CAPTURE_NUM_CHANNELS; i++) {
+	i = 0;
+	for_each_endpoint_of_node(pdev->dev.of_node, endpoint) {
 		struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
 		unsigned int flags;
 		int err;
 
-		endpoint = of_graph_get_next_endpoint(pdev->dev.of_node,
-						      endpoint);
-		if (!endpoint)
-			break;
-
 		rem = of_graph_get_remote_port_parent(endpoint);
 		if (!rem) {
 			dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
@@ -1577,6 +1573,10 @@ vpif_capture_get_pdata(struct platform_device *pdev,
 			goto err_cleanup;
 
 		of_node_put(rem);
+
+		i++;
+		if (i >= VPIF_CAPTURE_NUM_CHANNELS)
+			break;
 	}
 
 done:
-- 
2.43.0

[PATCH v2 resend 5/8] media: platform: xilinx: use for_each_endpoint_of_node()

[Kuninori Morimoto]

We already have for_each_endpoint_of_node(), don't use
of_graph_get_next_endpoint() directly. Replace it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/media/platform/xilinx/xilinx-vipp.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
index 996684a730383..38818b82a575e 100644
--- a/drivers/media/platform/xilinx/xilinx-vipp.c
+++ b/drivers/media/platform/xilinx/xilinx-vipp.c
@@ -205,12 +205,7 @@ static int xvip_graph_build_dma(struct xvip_composite_device *xdev)
 
 	dev_dbg(xdev->dev, "creating links for DMA engines\n");
 
-	while (1) {
-		/* Get the next endpoint and parse its link. */
-		ep = of_graph_get_next_endpoint(node, ep);
-		if (ep == NULL)
-			break;
-
+	for_each_endpoint_of_node(node, ep) {
 		dev_dbg(xdev->dev, "processing endpoint %pOF\n", ep);
 
 		ret = v4l2_fwnode_parse_link(of_fwnode_handle(ep), &link);
-- 
2.43.0

[PATCH v2 resend 6/8] staging: media: atmel: use for_each_endpoint_of_node()

[Kuninori Morimoto]

We already have for_each_endpoint_of_node(), don't use
of_graph_get_next_endpoint() directly. Replace it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c | 6 +-----
 drivers/staging/media/deprecated/atmel/atmel-sama7g5-isc.c | 6 +-----
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c b/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c
index 31b2b48085c59..cbfbec0c6cb57 100644
--- a/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c
+++ b/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c
@@ -340,13 +340,9 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
 
 	INIT_LIST_HEAD(&isc->subdev_entities);
 
-	while (1) {
+	for_each_endpoint_of_node(np, epn) {
 		struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
 
-		epn = of_graph_get_next_endpoint(np, epn);
-		if (!epn)
-			return 0;
-
 		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
 						 &v4l2_epn);
 		if (ret) {
diff --git a/drivers/staging/media/deprecated/atmel/atmel-sama7g5-isc.c b/drivers/staging/media/deprecated/atmel/atmel-sama7g5-isc.c
index 020034f631f57..7c477b1d3c484 100644
--- a/drivers/staging/media/deprecated/atmel/atmel-sama7g5-isc.c
+++ b/drivers/staging/media/deprecated/atmel/atmel-sama7g5-isc.c
@@ -326,13 +326,9 @@ static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
 
 	mipi_mode = of_property_read_bool(np, "microchip,mipi-mode");
 
-	while (1) {
+	for_each_endpoint_of_node(np, epn) {
 		struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
 
-		epn = of_graph_get_next_endpoint(np, epn);
-		if (!epn)
-			return 0;
-
 		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
 						 &v4l2_epn);
 		if (ret) {
-- 
2.43.0

[PATCH v2 resend 7/8] video: fbdev: use for_each_endpoint_of_node()

[Kuninori Morimoto]

We already have for_each_endpoint_of_node(), don't use
of_graph_get_next_endpoint() directly. Replace it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/video/fbdev/omap2/omapfb/dss/omapdss-boot-init.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/omapdss-boot-init.c b/drivers/video/fbdev/omap2/omapfb/dss/omapdss-boot-init.c
index 09f719af0d0c9..d80720c843235 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/omapdss-boot-init.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/omapdss-boot-init.c
@@ -149,8 +149,7 @@ static void __init omapdss_walk_device(struct device_node *node, bool root)
 
 	of_node_put(n);
 
-	n = NULL;
-	while ((n = of_graph_get_next_endpoint(node, n)) != NULL) {
+	for_each_endpoint_of_node(node, n) {
 		struct device_node *pn;
 
 		pn = of_graph_get_remote_port_parent(n);
-- 
2.43.0

[PATCH v2 resend 8/8] fbdev: omapfb: use of_graph_get_remote_port()

[Kuninori Morimoto]

We already have of_graph_get_remote_port(), Let's use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
index 14965a3fd05b7..4040e247e026e 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
@@ -117,19 +117,6 @@ u32 dss_of_port_get_port_number(struct device_node *port)
 	return reg;
 }
 
-static struct device_node *omapdss_of_get_remote_port(const struct device_node *node)
-{
-	struct device_node *np;
-
-	np = of_graph_get_remote_endpoint(node);
-	if (!np)
-		return NULL;
-
-	np = of_get_next_parent(np);
-
-	return np;
-}
-
 struct omap_dss_device *
 omapdss_of_find_source_for_first_ep(struct device_node *node)
 {
@@ -141,7 +128,7 @@ omapdss_of_find_source_for_first_ep(struct device_node *node)
 	if (!ep)
 		return ERR_PTR(-EINVAL);
 
-	src_port = omapdss_of_get_remote_port(ep);
+	src_port = of_graph_get_remote_port(ep);
 	if (!src_port) {
 		of_node_put(ep);
 		return ERR_PTR(-EINVAL);
-- 
2.43.0

Re: [PATCH v2 resend 1/8] gpu: drm: use for_each_endpoint_of_node()

[Dmitry Baryshkov]

On Tue, May 28, 2024 at 11:55:26PM +0000, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/base.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 

Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v2 resend 1/8] gpu: drm: use for_each_endpoint_of_node()

[Laurent Pinchart]

Hello Morimoto-san,

Thank you for the patch.

On Tue, May 28, 2024 at 11:55:26PM +0000, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

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

> ---
>  drivers/gpu/drm/omapdrm/dss/base.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/base.c b/drivers/gpu/drm/omapdrm/dss/base.c
> index 050ca7eafac58..5f8002f6bb7a5 100644
> --- a/drivers/gpu/drm/omapdrm/dss/base.c
> +++ b/drivers/gpu/drm/omapdrm/dss/base.c
> @@ -242,8 +242,7 @@ static void omapdss_walk_device(struct device *dev, struct device_node *node,
>  
>  	of_node_put(n);
>  
> -	n = NULL;
> -	while ((n = of_graph_get_next_endpoint(node, n)) != NULL) {
> +	for_each_endpoint_of_node(node, n) {
>  		struct device_node *pn = of_graph_get_remote_port_parent(n);
>  
>  		if (!pn)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 resend 2/8] hwtracing: use for_each_endpoint_of_node()

[Laurent Pinchart]

Hi Morimoto-san,

Thank you for the patch.

On Tue, May 28, 2024 at 11:55:32PM +0000, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/hwtracing/coresight/coresight-platform.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> index 9d550f5697fa8..e9683e613d520 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -275,7 +275,7 @@ static int of_get_coresight_platform_data(struct device *dev,
>  	 */
>  	if (!parent) {
>  		/*
> -		 * Avoid warnings in of_graph_get_next_endpoint()
> +		 * Avoid warnings in for_each_endpoint_of_node()
>  		 * if the device doesn't have any graph connections
>  		 */
>  		if (!of_graph_is_present(node))
> @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev,
>  	}
>  
>  	/* Iterate through each output port to discover topology */
> -	while ((ep = of_graph_get_next_endpoint(parent, ep))) {
> +	for_each_endpoint_of_node(parent, ep) {
>  		/*
>  		 * Legacy binding mixes input/output ports under the
>  		 * same parent. So, skip the input ports if we are dealing

I think there's a bug below. The loop contains

		ret = of_coresight_parse_endpoint(dev, ep, pdata);
		if (ret)
			return ret;

which leaks the reference to ep. This is not introduced by this patch,
so

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 resend 3/8] media: platform: microchip: use for_each_endpoint_of_node()

[Laurent Pinchart]

Hello Morimoto-san,

Thank you for the patch.

On Tue, May 28, 2024 at 11:55:37PM +0000, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  .../microchip/microchip-sama5d2-isc.c         | 19 +++++++------------
>  .../microchip/microchip-sama7g5-isc.c         | 19 +++++++------------
>  2 files changed, 14 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/media/platform/microchip/microchip-sama5d2-isc.c b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
> index 5ac149cf3647f..d9298771f5097 100644
> --- a/drivers/media/platform/microchip/microchip-sama5d2-isc.c
> +++ b/drivers/media/platform/microchip/microchip-sama5d2-isc.c
> @@ -356,30 +356,26 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
>  	struct device_node *epn = NULL;

There's no need anymore to initialize epn to NULL. Same in
microchip-sama7g5-isc.c. With this addressed,

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

>  	struct isc_subdev_entity *subdev_entity;
>  	unsigned int flags;
> -	int ret;
>  
>  	INIT_LIST_HEAD(&isc->subdev_entities);
>  
> -	while (1) {
> +	for_each_endpoint_of_node(np, epn) {
>  		struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
> -
> -		epn = of_graph_get_next_endpoint(np, epn);
> -		if (!epn)
> -			return 0;
> +		int ret;
>  
>  		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
>  						 &v4l2_epn);
>  		if (ret) {
> -			ret = -EINVAL;
> +			of_node_put(epn);
>  			dev_err(dev, "Could not parse the endpoint\n");
> -			break;
> +			return -EINVAL;
>  		}
>  
>  		subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
>  					     GFP_KERNEL);
>  		if (!subdev_entity) {
> -			ret = -ENOMEM;
> -			break;
> +			of_node_put(epn);
> +			return -ENOMEM;
>  		}
>  		subdev_entity->epn = epn;
>  
> @@ -400,9 +396,8 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
>  
>  		list_add_tail(&subdev_entity->list, &isc->subdev_entities);
>  	}
> -	of_node_put(epn);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int microchip_isc_probe(struct platform_device *pdev)
> diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> index 73445f33d26ba..36204fee10aa2 100644
> --- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> +++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> @@ -339,33 +339,29 @@ static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
>  	struct device_node *epn = NULL;
>  	struct isc_subdev_entity *subdev_entity;
>  	unsigned int flags;
> -	int ret;
>  	bool mipi_mode;
>  
>  	INIT_LIST_HEAD(&isc->subdev_entities);
>  
>  	mipi_mode = of_property_read_bool(np, "microchip,mipi-mode");
>  
> -	while (1) {
> +	for_each_endpoint_of_node(np, epn) {
>  		struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
> -
> -		epn = of_graph_get_next_endpoint(np, epn);
> -		if (!epn)
> -			return 0;
> +		int ret;
>  
>  		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
>  						 &v4l2_epn);
>  		if (ret) {
> -			ret = -EINVAL;
> +			of_node_put(epn);
>  			dev_err(dev, "Could not parse the endpoint\n");
> -			break;
> +			return -EINVAL;
>  		}
>  
>  		subdev_entity = devm_kzalloc(dev, sizeof(*subdev_entity),
>  					     GFP_KERNEL);
>  		if (!subdev_entity) {
> -			ret = -ENOMEM;
> -			break;
> +			of_node_put(epn);
> +			return -ENOMEM;
>  		}
>  		subdev_entity->epn = epn;
>  
> @@ -389,9 +385,8 @@ static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
>  
>  		list_add_tail(&subdev_entity->list, &isc->subdev_entities);
>  	}
> -	of_node_put(epn);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int microchip_xisc_probe(struct platform_device *pdev)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 resend 4/8] media: platform: ti: use for_each_endpoint_of_node()

[Laurent Pinchart]

Hello Morimoto-san,

Thank you for the patch.

On Tue, May 28, 2024 at 11:55:42PM +0000, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

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

> ---
>  drivers/media/platform/ti/am437x/am437x-vpfe.c   | 12 +++++-------
>  drivers/media/platform/ti/davinci/vpif_capture.c | 12 ++++++------
>  2 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/ti/am437x/am437x-vpfe.c b/drivers/media/platform/ti/am437x/am437x-vpfe.c
> index 77e12457d1495..009ff68a2b43c 100644
> --- a/drivers/media/platform/ti/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/ti/am437x/am437x-vpfe.c
> @@ -2287,7 +2287,7 @@ static const struct v4l2_async_notifier_operations vpfe_async_ops = {
>  static struct vpfe_config *
>  vpfe_get_pdata(struct vpfe_device *vpfe)
>  {
> -	struct device_node *endpoint = NULL;
> +	struct device_node *endpoint;
>  	struct device *dev = vpfe->pdev;
>  	struct vpfe_subdev_info *sdinfo;
>  	struct vpfe_config *pdata;
> @@ -2306,14 +2306,11 @@ vpfe_get_pdata(struct vpfe_device *vpfe)
>  	if (!pdata)
>  		return NULL;
>  
> -	for (i = 0; ; i++) {
> +	i = 0;
> +	for_each_endpoint_of_node(dev->of_node, endpoint) {
>  		struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
>  		struct device_node *rem;
>  
> -		endpoint = of_graph_get_next_endpoint(dev->of_node, endpoint);
> -		if (!endpoint)
> -			break;
> -
>  		sdinfo = &pdata->sub_devs[i];
>  		sdinfo->grp_id = 0;
>  
> @@ -2371,9 +2368,10 @@ vpfe_get_pdata(struct vpfe_device *vpfe)
>  		of_node_put(rem);
>  		if (IS_ERR(pdata->asd[i]))
>  			goto cleanup;
> +
> +		i++;
>  	}
>  
> -	of_node_put(endpoint);
>  	return pdata;
>  
>  cleanup:
> diff --git a/drivers/media/platform/ti/davinci/vpif_capture.c b/drivers/media/platform/ti/davinci/vpif_capture.c
> index c28794b6677b7..078ae11cd0787 100644
> --- a/drivers/media/platform/ti/davinci/vpif_capture.c
> +++ b/drivers/media/platform/ti/davinci/vpif_capture.c
> @@ -1517,16 +1517,12 @@ vpif_capture_get_pdata(struct platform_device *pdev,
>  	if (!pdata->subdev_info)
>  		return NULL;
>  
> -	for (i = 0; i < VPIF_CAPTURE_NUM_CHANNELS; i++) {
> +	i = 0;
> +	for_each_endpoint_of_node(pdev->dev.of_node, endpoint) {
>  		struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
>  		unsigned int flags;
>  		int err;
>  
> -		endpoint = of_graph_get_next_endpoint(pdev->dev.of_node,
> -						      endpoint);
> -		if (!endpoint)
> -			break;
> -
>  		rem = of_graph_get_remote_port_parent(endpoint);
>  		if (!rem) {
>  			dev_dbg(&pdev->dev, "Remote device at %pOF not found\n",
> @@ -1577,6 +1573,10 @@ vpif_capture_get_pdata(struct platform_device *pdev,
>  			goto err_cleanup;
>  
>  		of_node_put(rem);
> +
> +		i++;
> +		if (i >= VPIF_CAPTURE_NUM_CHANNELS)
> +			break;
>  	}
>  
>  done:

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 resend 5/8] media: platform: xilinx: use for_each_endpoint_of_node()

[Laurent Pinchart]

Hi Morimoto-san,

Thank you for the patch.

On Tue, May 28, 2024 at 11:55:46PM +0000, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/media/platform/xilinx/xilinx-vipp.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
> index 996684a730383..38818b82a575e 100644
> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
> @@ -205,12 +205,7 @@ static int xvip_graph_build_dma(struct xvip_composite_device *xdev)

I think ep doesn't have to be initialized to NULL anymore.

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

>  
>  	dev_dbg(xdev->dev, "creating links for DMA engines\n");
>  
> -	while (1) {
> -		/* Get the next endpoint and parse its link. */
> -		ep = of_graph_get_next_endpoint(node, ep);
> -		if (ep == NULL)
> -			break;
> -
> +	for_each_endpoint_of_node(node, ep) {
>  		dev_dbg(xdev->dev, "processing endpoint %pOF\n", ep);
>  
>  		ret = v4l2_fwnode_parse_link(of_fwnode_handle(ep), &link);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 resend 6/8] staging: media: atmel: use for_each_endpoint_of_node()

[Laurent Pinchart]

Hi Morimoto-san,

Thank you for the patch.

On Tue, May 28, 2024 at 11:55:51PM +0000, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c | 6 +-----
>  drivers/staging/media/deprecated/atmel/atmel-sama7g5-isc.c | 6 +-----
>  2 files changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c b/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c
> index 31b2b48085c59..cbfbec0c6cb57 100644
> --- a/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c
> +++ b/drivers/staging/media/deprecated/atmel/atmel-sama5d2-isc.c
> @@ -340,13 +340,9 @@ static int isc_parse_dt(struct device *dev, struct isc_device *isc)
>  
>  	INIT_LIST_HEAD(&isc->subdev_entities);
>  
> -	while (1) {
> +	for_each_endpoint_of_node(np, epn) {

I think epn doesn't have to be initialized to NULL anymore. Same below.

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

>  		struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
>  
> -		epn = of_graph_get_next_endpoint(np, epn);
> -		if (!epn)
> -			return 0;
> -
>  		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
>  						 &v4l2_epn);
>  		if (ret) {
> diff --git a/drivers/staging/media/deprecated/atmel/atmel-sama7g5-isc.c b/drivers/staging/media/deprecated/atmel/atmel-sama7g5-isc.c
> index 020034f631f57..7c477b1d3c484 100644
> --- a/drivers/staging/media/deprecated/atmel/atmel-sama7g5-isc.c
> +++ b/drivers/staging/media/deprecated/atmel/atmel-sama7g5-isc.c
> @@ -326,13 +326,9 @@ static int xisc_parse_dt(struct device *dev, struct isc_device *isc)
>  
>  	mipi_mode = of_property_read_bool(np, "microchip,mipi-mode");
>  
> -	while (1) {
> +	for_each_endpoint_of_node(np, epn) {
>  		struct v4l2_fwnode_endpoint v4l2_epn = { .bus_type = 0 };
>  
> -		epn = of_graph_get_next_endpoint(np, epn);
> -		if (!epn)
> -			return 0;
> -
>  		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(epn),
>  						 &v4l2_epn);
>  		if (ret) {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 resend 7/8] video: fbdev: use for_each_endpoint_of_node()

[Laurent Pinchart]

Hi Morimoto-san,

Thank you for the patch.

On Tue, May 28, 2024 at 11:55:55PM +0000, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

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

> ---
>  drivers/video/fbdev/omap2/omapfb/dss/omapdss-boot-init.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/omapdss-boot-init.c b/drivers/video/fbdev/omap2/omapfb/dss/omapdss-boot-init.c
> index 09f719af0d0c9..d80720c843235 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/omapdss-boot-init.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/omapdss-boot-init.c
> @@ -149,8 +149,7 @@ static void __init omapdss_walk_device(struct device_node *node, bool root)
>  
>  	of_node_put(n);
>  
> -	n = NULL;
> -	while ((n = of_graph_get_next_endpoint(node, n)) != NULL) {
> +	for_each_endpoint_of_node(node, n) {
>  		struct device_node *pn;
>  
>  		pn = of_graph_get_remote_port_parent(n);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 resend 8/8] fbdev: omapfb: use of_graph_get_remote_port()

[Laurent Pinchart]

Hi Morimoto-san,

Thank you for the patch.

On Tue, May 28, 2024 at 11:55:59PM +0000, Kuninori Morimoto wrote:
> We already have of_graph_get_remote_port(), Let's use it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

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

> ---
>  drivers/video/fbdev/omap2/omapfb/dss/dss-of.c | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
> index 14965a3fd05b7..4040e247e026e 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/dss-of.c
> @@ -117,19 +117,6 @@ u32 dss_of_port_get_port_number(struct device_node *port)
>  	return reg;
>  }
>  
> -static struct device_node *omapdss_of_get_remote_port(const struct device_node *node)
> -{
> -	struct device_node *np;
> -
> -	np = of_graph_get_remote_endpoint(node);
> -	if (!np)
> -		return NULL;
> -
> -	np = of_get_next_parent(np);
> -
> -	return np;
> -}
> -
>  struct omap_dss_device *
>  omapdss_of_find_source_for_first_ep(struct device_node *node)
>  {
> @@ -141,7 +128,7 @@ omapdss_of_find_source_for_first_ep(struct device_node *node)
>  	if (!ep)
>  		return ERR_PTR(-EINVAL);
>  
> -	src_port = omapdss_of_get_remote_port(ep);
> +	src_port = of_graph_get_remote_port(ep);
>  	if (!src_port) {
>  		of_node_put(ep);
>  		return ERR_PTR(-EINVAL);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 resend 2/8] hwtracing: use for_each_endpoint_of_node()

[James Clark]

On 29/05/2024 01:40, Laurent Pinchart wrote:
> Hi Morimoto-san,
> 
> Thank you for the patch.
> 
> On Tue, May 28, 2024 at 11:55:32PM +0000, Kuninori Morimoto wrote:
>> We already have for_each_endpoint_of_node(), don't use
>> of_graph_get_next_endpoint() directly. Replace it.
>>
>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> ---
>>  drivers/hwtracing/coresight/coresight-platform.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
>> index 9d550f5697fa8..e9683e613d520 100644
>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>> @@ -275,7 +275,7 @@ static int of_get_coresight_platform_data(struct device *dev,
>>  	 */
>>  	if (!parent) {
>>  		/*
>> -		 * Avoid warnings in of_graph_get_next_endpoint()
>> +		 * Avoid warnings in for_each_endpoint_of_node()
>>  		 * if the device doesn't have any graph connections
>>  		 */
>>  		if (!of_graph_is_present(node))
>> @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev,
>>  	}
>>  
>>  	/* Iterate through each output port to discover topology */
>> -	while ((ep = of_graph_get_next_endpoint(parent, ep))) {
>> +	for_each_endpoint_of_node(parent, ep) {
>>  		/*
>>  		 * Legacy binding mixes input/output ports under the
>>  		 * same parent. So, skip the input ports if we are dealing
> 
> I think there's a bug below. The loop contains
> 
> 		ret = of_coresight_parse_endpoint(dev, ep, pdata);
> 		if (ret)
> 			return ret;
> 
> which leaks the reference to ep. This is not introduced by this patch,
> so
> 

Nice catch, I will send a patch.

Also:

Reviewed-by: James Clark <james.clark@arm.com>

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

Re: [PATCH v2 resend 2/8] hwtracing: use for_each_endpoint_of_node()

[Dan Carpenter]

On Wed, May 29, 2024 at 03:40:47AM +0300, Laurent Pinchart wrote:
> > @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev,
> >  	}
> >  
> >  	/* Iterate through each output port to discover topology */
> > -	while ((ep = of_graph_get_next_endpoint(parent, ep))) {
> > +	for_each_endpoint_of_node(parent, ep) {
> >  		/*
> >  		 * Legacy binding mixes input/output ports under the
> >  		 * same parent. So, skip the input ports if we are dealing
> 
> I think there's a bug below. The loop contains
> 
> 		ret = of_coresight_parse_endpoint(dev, ep, pdata);
> 		if (ret)
> 			return ret;
> 
> which leaks the reference to ep. This is not introduced by this patch,

Someone should create for_each_endpoint_of_node_scoped().

#define for_each_endpoint_of_node_scoped(parent, child) \
        for (struct device_node *child __free(device_node) =           \
             of_graph_get_next_endpoint(parent, NULL); child != NULL;  \
             child = of_graph_get_next_endpoint(parent, child))

regards,
dan carpenter

Re: [PATCH v2 resend 2/8] hwtracing: use for_each_endpoint_of_node()

[Laurent Pinchart]

On Wed, May 29, 2024 at 05:34:41PM +0300, Dan Carpenter wrote:
> On Wed, May 29, 2024 at 03:40:47AM +0300, Laurent Pinchart wrote:
> > > @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev,
> > >  	}
> > >  
> > >  	/* Iterate through each output port to discover topology */
> > > -	while ((ep = of_graph_get_next_endpoint(parent, ep))) {
> > > +	for_each_endpoint_of_node(parent, ep) {
> > >  		/*
> > >  		 * Legacy binding mixes input/output ports under the
> > >  		 * same parent. So, skip the input ports if we are dealing
> > 
> > I think there's a bug below. The loop contains
> > 
> > 		ret = of_coresight_parse_endpoint(dev, ep, pdata);
> > 		if (ret)
> > 			return ret;
> > 
> > which leaks the reference to ep. This is not introduced by this patch,
> 
> Someone should create for_each_endpoint_of_node_scoped().
> 
> #define for_each_endpoint_of_node_scoped(parent, child) \
>         for (struct device_node *child __free(device_node) =           \
>              of_graph_get_next_endpoint(parent, NULL); child != NULL;  \
>              child = of_graph_get_next_endpoint(parent, child))

I was thinking about that too :-) I wondered if we should then bother
taking and releasing references, given that references to the children
can't be leaked out of the loop. My reasoning was that the parent
device_node is guaranteed to be valid throughout the loop, so borrowing
references to children instead of creating new ones within the loop
should be fine. This assumes that endpoints and ports can't vanish while
the parent is there. Thinking further about it, it may not be a safe
assumption for the future. As we anyway use functions internally that
create new references, we can as well handle them correctly.

Using this new macro, the loop body would need to call of_node_get() if
it wants to get a reference out of the loop. That's the right thing to
do, and I think it would be less error-prone than having to drop
references when exiting from the loop as we do today. It would still be
nice if we could have an API that allows catching this missing
of_node_get() automatically, but I don't see a simple way to do so at
the moment.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 resend 2/8] hwtracing: use for_each_endpoint_of_node()

[Dan Carpenter]

On Wed, May 29, 2024 at 05:52:53PM +0300, Laurent Pinchart wrote:
> On Wed, May 29, 2024 at 05:34:41PM +0300, Dan Carpenter wrote:
> > On Wed, May 29, 2024 at 03:40:47AM +0300, Laurent Pinchart wrote:
> > > > @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev,
> > > >  	}
> > > >  
> > > >  	/* Iterate through each output port to discover topology */
> > > > -	while ((ep = of_graph_get_next_endpoint(parent, ep))) {
> > > > +	for_each_endpoint_of_node(parent, ep) {
> > > >  		/*
> > > >  		 * Legacy binding mixes input/output ports under the
> > > >  		 * same parent. So, skip the input ports if we are dealing
> > > 
> > > I think there's a bug below. The loop contains
> > > 
> > > 		ret = of_coresight_parse_endpoint(dev, ep, pdata);
> > > 		if (ret)
> > > 			return ret;
> > > 
> > > which leaks the reference to ep. This is not introduced by this patch,
> > 
> > Someone should create for_each_endpoint_of_node_scoped().
> > 
> > #define for_each_endpoint_of_node_scoped(parent, child) \
> >         for (struct device_node *child __free(device_node) =           \
> >              of_graph_get_next_endpoint(parent, NULL); child != NULL;  \
> >              child = of_graph_get_next_endpoint(parent, child))
> 
> I was thinking about that too :-) I wondered if we should then bother
> taking and releasing references, given that references to the children
> can't be leaked out of the loop. My reasoning was that the parent
> device_node is guaranteed to be valid throughout the loop, so borrowing
> references to children instead of creating new ones within the loop
> should be fine. This assumes that endpoints and ports can't vanish while
> the parent is there. Thinking further about it, it may not be a safe
> assumption for the future. As we anyway use functions internally that
> create new references, we can as well handle them correctly.
> 
> Using this new macro, the loop body would need to call of_node_get() if
> it wants to get a reference out of the loop.

The child pointer is declared local to just the loop so you'd need
create a different function scoped variable.  If it's not local to the
loop then we'd end up taking a reference on each iteration and never
releasing anything except on error paths.

> That's the right thing to
> do, and I think it would be less error-prone than having to drop
> references when exiting from the loop as we do today. It would still be
> nice if we could have an API that allows catching this missing
> of_node_get() automatically, but I don't see a simple way to do so at
> the moment.

That's an interesting point.

If we did "function_scope_var = ep;" here then we'd need to take a
second reference as you say.  With other cleanup stuff like kfree() it's
very hard to miss it if we forget to call "no_free_ptr(&ep)" because
it's on the success path.  It leads to an immediate crash in testing.
But here it's just ref counting so possibly we might miss that sort of
bug.

regards,
dan carpenter

Re: [PATCH v2 resend 2/8] hwtracing: use for_each_endpoint_of_node()

[Laurent Pinchart]

On Wed, May 29, 2024 at 06:19:33PM +0300, Dan Carpenter wrote:
> On Wed, May 29, 2024 at 05:52:53PM +0300, Laurent Pinchart wrote:
> > On Wed, May 29, 2024 at 05:34:41PM +0300, Dan Carpenter wrote:
> > > On Wed, May 29, 2024 at 03:40:47AM +0300, Laurent Pinchart wrote:
> > > > > @@ -286,7 +286,7 @@ static int of_get_coresight_platform_data(struct device *dev,
> > > > >  	}
> > > > >  
> > > > >  	/* Iterate through each output port to discover topology */
> > > > > -	while ((ep = of_graph_get_next_endpoint(parent, ep))) {
> > > > > +	for_each_endpoint_of_node(parent, ep) {
> > > > >  		/*
> > > > >  		 * Legacy binding mixes input/output ports under the
> > > > >  		 * same parent. So, skip the input ports if we are dealing
> > > > 
> > > > I think there's a bug below. The loop contains
> > > > 
> > > > 		ret = of_coresight_parse_endpoint(dev, ep, pdata);
> > > > 		if (ret)
> > > > 			return ret;
> > > > 
> > > > which leaks the reference to ep. This is not introduced by this patch,
> > > 
> > > Someone should create for_each_endpoint_of_node_scoped().
> > > 
> > > #define for_each_endpoint_of_node_scoped(parent, child) \
> > >         for (struct device_node *child __free(device_node) =           \
> > >              of_graph_get_next_endpoint(parent, NULL); child != NULL;  \
> > >              child = of_graph_get_next_endpoint(parent, child))
> > 
> > I was thinking about that too :-) I wondered if we should then bother
> > taking and releasing references, given that references to the children
> > can't be leaked out of the loop. My reasoning was that the parent
> > device_node is guaranteed to be valid throughout the loop, so borrowing
> > references to children instead of creating new ones within the loop
> > should be fine. This assumes that endpoints and ports can't vanish while
> > the parent is there. Thinking further about it, it may not be a safe
> > assumption for the future. As we anyway use functions internally that
> > create new references, we can as well handle them correctly.
> > 
> > Using this new macro, the loop body would need to call of_node_get() if
> > it wants to get a reference out of the loop.
> 
> The child pointer is declared local to just the loop so you'd need
> create a different function scoped variable.  If it's not local to the
> loop then we'd end up taking a reference on each iteration and never
> releasing anything except on error paths.
> 
> > That's the right thing to
> > do, and I think it would be less error-prone than having to drop
> > references when exiting from the loop as we do today. It would still be
> > nice if we could have an API that allows catching this missing
> > of_node_get() automatically, but I don't see a simple way to do so at
> > the moment.
> 
> That's an interesting point.
> 
> If we did "function_scope_var = ep;" here then we'd need to take a
> second reference as you say.

Yes, that's what I meant above, sorry if that wasn't clear.

> With other cleanup stuff like kfree() it's
> very hard to miss it if we forget to call "no_free_ptr(&ep)" because
> it's on the success path.  It leads to an immediate crash in testing.
> But here it's just ref counting so possibly we might miss that sort of
> bug.

All this calls for std::shared_ptr<struct device_node> :-D

Jokes aside, I think for_each_endpoint_of_node_scoped() would still be
safer, as the number of cases where we would have to pass a reference to
the outer scope should be quite smaller than the number of cases where
we break from for_each_endpoint_of_node() loops today.

On the other hand, the consequence of a bug with
for_each_endpoint_of_node_scoped() would be a dangling reference,
instead of a reference leak with for_each_endpoint_of_node(), so it may
be more dangerous the same way that UAF is more dangerous than memory
leaks.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 resend 2/8] hwtracing: use for_each_endpoint_of_node()

[Kuninori Morimoto]

Hi Dan

> Someone should create for_each_endpoint_of_node_scoped().
> 
> #define for_each_endpoint_of_node_scoped(parent, child) \
>         for (struct device_node *child __free(device_node) =           \
>              of_graph_get_next_endpoint(parent, NULL); child != NULL;  \
>              child = of_graph_get_next_endpoint(parent, child))

Thank you for pointing it.
I have noticed that _scoped() loop exist, but this patch-set
want to focus to use existing for_each_xxx() loop first.
Replacing to _scoped() macro is next step.

Thank you for your help !!
Best regards
---
Kuninori Morimoto