[PATCH -next 0/5] drm: Use for_each_child_of_node_scoped()

[PATCH -next 0/5] drm: Use for_each_child_of_node_scoped()

[Jinjie Ruan]

Use for_each_child_of_node_scoped() to simplfy code and fix a bug
by the way.

Jinjie Ruan (5):
  drm/rockchip: Use for_each_child_of_node_scoped()
  drm/mediatek: Fix missing of_node_put() for mtk_drm_get_all_drm_priv()
  drm: of: Use for_each_child_of_node_scoped()
  drm/nouveau: Use for_each_child_of_node_scoped()
  gpu: host1x: Use for_each_available_child_of_node_scoped()

 drivers/gpu/drm/drm_of.c                    | 11 +++--------
 drivers/gpu/drm/mediatek/mtk_drm_drv.c      | 10 +++-------
 drivers/gpu/drm/nouveau/nouveau_connector.c |  5 ++---
 drivers/gpu/drm/rockchip/rockchip_lvds.c    |  8 +++-----
 drivers/gpu/host1x/bus.c                    | 11 +++--------
 5 files changed, 14 insertions(+), 31 deletions(-)

-- 
2.34.1

[PATCH -next 1/5] drm/rockchip: Use for_each_child_of_node_scoped()

[Jinjie Ruan]

Avoids the need for manual cleanup of_node_put() in early exits
from the loop.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 drivers/gpu/drm/rockchip/rockchip_lvds.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
index 9a01aa450741..f5b3f18794dd 100644
--- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
+++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
@@ -548,7 +548,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
 	struct device_node *remote = NULL;
-	struct device_node  *port, *endpoint;
+	struct device_node  *port;
 	int ret = 0, child_count = 0;
 	const char *name;
 	u32 endpoint_id = 0;
@@ -560,15 +560,13 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
 			      "can't found port point, please init lvds panel port!\n");
 		return -EINVAL;
 	}
-	for_each_child_of_node(port, endpoint) {
+	for_each_child_of_node_scoped(port, endpoint) {
 		child_count++;
 		of_property_read_u32(endpoint, "reg", &endpoint_id);
 		ret = drm_of_find_panel_or_bridge(dev->of_node, 1, endpoint_id,
 						  &lvds->panel, &lvds->bridge);
-		if (!ret) {
-			of_node_put(endpoint);
+		if (!ret)
 			break;
-		}
 	}
 	if (!child_count) {
 		DRM_DEV_ERROR(dev, "lvds port does not have any children\n");
-- 
2.34.1

[PATCH -next 2/5] drm/mediatek: Fix missing of_node_put() for mtk_drm_get_all_drm_priv()

[Jinjie Ruan]

In mtk_drm_get_all_drm_priv(), break in for_each_child_of_node() should
call of_node_put() to avoid child node resource leak, use
for_each_child_of_node_scoped() to fix it.

And avoid the need for manual cleanup of_node_put() in early exits
from the loop for another one.

Fixes: d761b9450e31 ("drm/mediatek: Add cnt checking for coverity issue")
Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_drv.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 77b50c56c124..41aff0183cbd 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -371,12 +371,11 @@ static bool mtk_drm_get_all_drm_priv(struct device *dev)
 	struct mtk_drm_private *temp_drm_priv;
 	struct device_node *phandle = dev->parent->of_node;
 	const struct of_device_id *of_id;
-	struct device_node *node;
 	struct device *drm_dev;
 	unsigned int cnt = 0;
 	int i, j;
 
-	for_each_child_of_node(phandle->parent, node) {
+	for_each_child_of_node_scoped(phandle->parent, node) {
 		struct platform_device *pdev;
 
 		of_id = of_match_node(mtk_drm_of_ids, node);
@@ -828,7 +827,6 @@ static int mtk_drm_probe(struct platform_device *pdev)
 	struct device_node *phandle = dev->parent->of_node;
 	const struct of_device_id *of_id;
 	struct mtk_drm_private *private;
-	struct device_node *node;
 	struct component_match *match = NULL;
 	struct platform_device *ovl_adaptor;
 	int ret;
@@ -869,7 +867,7 @@ static int mtk_drm_probe(struct platform_device *pdev)
 	}
 
 	/* Iterate over sibling DISP function blocks */
-	for_each_child_of_node(phandle->parent, node) {
+	for_each_child_of_node_scoped(phandle->parent, node) {
 		const struct of_device_id *of_id;
 		enum mtk_ddp_comp_type comp_type;
 		int comp_id;
@@ -933,10 +931,8 @@ static int mtk_drm_probe(struct platform_device *pdev)
 		}
 
 		ret = mtk_ddp_comp_init(node, &private->ddp_comp[comp_id], comp_id);
-		if (ret) {
-			of_node_put(node);
+		if (ret)
 			goto err_node;
-		}
 	}
 
 	if (!private->mutex_node) {
-- 
2.34.1

[PATCH -next 3/5] drm: of: Use for_each_child_of_node_scoped()

[Jinjie Ruan]

Avoids the need for manual cleanup of_node_put() in early exits
from the loop.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 drivers/gpu/drm/drm_of.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 177b600895d3..41d9288c97a7 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -304,10 +304,9 @@ static int drm_of_lvds_get_port_pixels_type(struct device_node *port_node)
 static int drm_of_lvds_get_remote_pixels_type(
 			const struct device_node *port_node)
 {
-	struct device_node *endpoint = NULL;
 	int pixels_type = -EPIPE;
 
-	for_each_child_of_node(port_node, endpoint) {
+	for_each_child_of_node_scoped(port_node, endpoint) {
 		struct device_node *remote_port;
 		int current_pt;
 
@@ -315,10 +314,8 @@ static int drm_of_lvds_get_remote_pixels_type(
 			continue;
 
 		remote_port = of_graph_get_remote_port(endpoint);
-		if (!remote_port) {
-			of_node_put(endpoint);
+		if (!remote_port)
 			return -EPIPE;
-		}
 
 		current_pt = drm_of_lvds_get_port_pixels_type(remote_port);
 		of_node_put(remote_port);
@@ -332,10 +329,8 @@ static int drm_of_lvds_get_remote_pixels_type(
 		 * configurations by passing the endpoints explicitly to
 		 * drm_of_lvds_get_dual_link_pixel_order().
 		 */
-		if (!current_pt || pixels_type != current_pt) {
-			of_node_put(endpoint);
+		if (!current_pt || pixels_type != current_pt)
 			return -EINVAL;
-		}
 	}
 
 	return pixels_type;
-- 
2.34.1

[PATCH -next 4/5] drm/nouveau: Use for_each_child_of_node_scoped()

[Jinjie Ruan]

Avoids the need for manual cleanup of_node_put() in early exits
from the loop.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 drivers/gpu/drm/nouveau/nouveau_connector.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index b06aa473102b..8d5c9c74cbb9 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -477,14 +477,14 @@ nouveau_connector_of_detect(struct drm_connector *connector)
 	struct nouveau_connector *nv_connector = nouveau_connector(connector);
 	struct nouveau_encoder *nv_encoder;
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
-	struct device_node *cn, *dn = pci_device_to_OF_node(pdev);
+	struct device_node *dn = pci_device_to_OF_node(pdev);
 
 	if (!dn ||
 	    !((nv_encoder = find_encoder(connector, DCB_OUTPUT_TMDS)) ||
 	      (nv_encoder = find_encoder(connector, DCB_OUTPUT_ANALOG))))
 		return NULL;
 
-	for_each_child_of_node(dn, cn) {
+	for_each_child_of_node_scoped(dn, cn) {
 		const char *name = of_get_property(cn, "name", NULL);
 		const void *edid = of_get_property(cn, "EDID", NULL);
 		int idx = name ? name[strlen(name) - 1] - 'A' : 0;
@@ -492,7 +492,6 @@ nouveau_connector_of_detect(struct drm_connector *connector)
 		if (nv_encoder->dcb->i2c_index == idx && edid) {
 			nv_connector->edid =
 				kmemdup(edid, EDID_LENGTH, GFP_KERNEL);
-			of_node_put(cn);
 			return nv_encoder;
 		}
 	}
-- 
2.34.1

[PATCH -next 5/5] gpu: host1x: Use for_each_available_child_of_node_scoped()

[Jinjie Ruan]

Avoids the need for manual cleanup of_node_put() in early exits
from the loop.

Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
---
 drivers/gpu/host1x/bus.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index 8e09d6d328d2..344cc9e741c1 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -41,7 +41,6 @@ static int host1x_subdev_add(struct host1x_device *device,
 			     struct device_node *np)
 {
 	struct host1x_subdev *subdev;
-	struct device_node *child;
 	int err;
 
 	subdev = kzalloc(sizeof(*subdev), GFP_KERNEL);
@@ -56,13 +55,12 @@ static int host1x_subdev_add(struct host1x_device *device,
 	mutex_unlock(&device->subdevs_lock);
 
 	/* recursively add children */
-	for_each_child_of_node(np, child) {
+	for_each_child_of_node_scoped(np, child) {
 		if (of_match_node(driver->subdevs, child) &&
 		    of_device_is_available(child)) {
 			err = host1x_subdev_add(device, driver, child);
 			if (err < 0) {
 				/* XXX cleanup? */
-				of_node_put(child);
 				return err;
 			}
 		}
@@ -90,17 +88,14 @@ static void host1x_subdev_del(struct host1x_subdev *subdev)
 static int host1x_device_parse_dt(struct host1x_device *device,
 				  struct host1x_driver *driver)
 {
-	struct device_node *np;
 	int err;
 
-	for_each_child_of_node(device->dev.parent->of_node, np) {
+	for_each_child_of_node_scoped(device->dev.parent->of_node, np) {
 		if (of_match_node(driver->subdevs, np) &&
 		    of_device_is_available(np)) {
 			err = host1x_subdev_add(device, driver, np);
-			if (err < 0) {
-				of_node_put(np);
+			if (err < 0)
 				return err;
-			}
 		}
 	}
 
-- 
2.34.1

Re: [PATCH -next 1/5] drm/rockchip: Use for_each_child_of_node_scoped()

[Heiko Stübner]

Am Freitag, 23. August 2024, 11:20:49 CEST schrieb Jinjie Ruan:
> Avoids the need for manual cleanup of_node_put() in early exits
> from the loop.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>

Not sure if this should go in in one part or individually, but anyway
Reviewed-by: Heiko Stuebner <heiko@sntech.de>


>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 9a01aa450741..f5b3f18794dd 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -548,7 +548,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  	struct drm_encoder *encoder;
>  	struct drm_connector *connector;
>  	struct device_node *remote = NULL;
> -	struct device_node  *port, *endpoint;
> +	struct device_node  *port;
>  	int ret = 0, child_count = 0;
>  	const char *name;
>  	u32 endpoint_id = 0;
> @@ -560,15 +560,13 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  			      "can't found port point, please init lvds panel port!\n");
>  		return -EINVAL;
>  	}
> -	for_each_child_of_node(port, endpoint) {
> +	for_each_child_of_node_scoped(port, endpoint) {
>  		child_count++;
>  		of_property_read_u32(endpoint, "reg", &endpoint_id);
>  		ret = drm_of_find_panel_or_bridge(dev->of_node, 1, endpoint_id,
>  						  &lvds->panel, &lvds->bridge);
> -		if (!ret) {
> -			of_node_put(endpoint);
> +		if (!ret)
>  			break;
> -		}
>  	}
>  	if (!child_count) {
>  		DRM_DEV_ERROR(dev, "lvds port does not have any children\n");
> 

Re: [PATCH -next 2/5] drm/mediatek: Fix missing of_node_put() for mtk_drm_get_all_drm_priv()

[Christophe JAILLET]

Le 23/08/2024 à 11:20, Jinjie Ruan a écrit :
> In mtk_drm_get_all_drm_priv(), break in for_each_child_of_node() should
> call of_node_put() to avoid child node resource leak, use
> for_each_child_of_node_scoped() to fix it.
> 
> And avoid the need for manual cleanup of_node_put() in early exits
> from the loop for another one.
> 
> Fixes: d761b9450e31 ("drm/mediatek: Add cnt checking for coverity issue")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---
>   drivers/gpu/drm/mediatek/mtk_drm_drv.c | 10 +++-------
>   1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 77b50c56c124..41aff0183cbd 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -371,12 +371,11 @@ static bool mtk_drm_get_all_drm_priv(struct device *dev)
>   	struct mtk_drm_private *temp_drm_priv;
>   	struct device_node *phandle = dev->parent->of_node;
>   	const struct of_device_id *of_id;
> -	struct device_node *node;
>   	struct device *drm_dev;
>   	unsigned int cnt = 0;
>   	int i, j;
>   
> -	for_each_child_of_node(phandle->parent, node) {
> +	for_each_child_of_node_scoped(phandle->parent, node) {
>   		struct platform_device *pdev;
>   
>   		of_id = of_match_node(mtk_drm_of_ids, node);
> @@ -828,7 +827,6 @@ static int mtk_drm_probe(struct platform_device *pdev)
>   	struct device_node *phandle = dev->parent->of_node;
>   	const struct of_device_id *of_id;
>   	struct mtk_drm_private *private;
> -	struct device_node *node;
>   	struct component_match *match = NULL;
>   	struct platform_device *ovl_adaptor;
>   	int ret;
> @@ -869,7 +867,7 @@ static int mtk_drm_probe(struct platform_device *pdev)
>   	}
>   
>   	/* Iterate over sibling DISP function blocks */
> -	for_each_child_of_node(phandle->parent, node) {
> +	for_each_child_of_node_scoped(phandle->parent, node) {
>   		const struct of_device_id *of_id;
>   		enum mtk_ddp_comp_type comp_type;
>   		int comp_id;
> @@ -933,10 +931,8 @@ static int mtk_drm_probe(struct platform_device *pdev)
>   		}
>   
>   		ret = mtk_ddp_comp_init(node, &private->ddp_comp[comp_id], comp_id);
> -		if (ret) {
> -			of_node_put(node);
> +		if (ret)
>   			goto err_node;

Hi,

I've seen on another thread that is was not sure that scoped versions 
and gotos played well together.

It was asked to check more in details and confirm that it was safe 
before applying the patch.

I've not followed the discussion, so I just point it out, in case it helps.

I'll try to give it a look in the coming days.


CJ

> -		}
>   	}
>   
>   	if (!private->mutex_node) {

Re: [PATCH -next 1/5] drm/rockchip: Use for_each_child_of_node_scoped()

[Jonathan Cameron]

On Fri, 23 Aug 2024 17:20:49 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> Avoids the need for manual cleanup of_node_put() in early exits
> from the loop.
> 
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>

There is more to do here, and looking at the code, I'm far from
sure it isn't releasing references it never had.

> ---
>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 9a01aa450741..f5b3f18794dd 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -548,7 +548,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  	struct drm_encoder *encoder;
>  	struct drm_connector *connector;
>  	struct device_node *remote = NULL;
> -	struct device_node  *port, *endpoint;

Odd extra space before *port in original. Clean that up whilst here.


> +	struct device_node  *port;

Use __free(device_node) for *port as well.

So where the current asignment is.
	struct device_node *port = of_graph_get_port_by_id(dev->of_node, 1);

>  	int ret = 0, child_count = 0;
>  	const char *name;
>  	u32 endpoint_id = 0;
> @@ -560,15 +560,13 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  			      "can't found port point, please init lvds panel port!\n");
>  		return -EINVAL;
>  	}
> -	for_each_child_of_node(port, endpoint) {
> +	for_each_child_of_node_scoped(port, endpoint) {
>  		child_count++;
>  		of_property_read_u32(endpoint, "reg", &endpoint_id);
>  		ret = drm_of_find_panel_or_bridge(dev->of_node, 1, endpoint_id,
>  						  &lvds->panel, &lvds->bridge);
> -		if (!ret) {
> -			of_node_put(endpoint);
> +		if (!ret)
>  			break;

This then can simply be
			return dev_err_probe(dev, ret,
					     "failed to find pannel and bridge node\n");
> -		}

Various other paths become direct returns as well.

>  	}

The later code with remote looks suspect as not obvious who got the reference that
is being put but assuming that is correct, it's another possible place for __free based
cleanup.


>  	if (!child_count) {
>  		DRM_DEV_ERROR(dev, "lvds port does not have any children\n");

Re: [PATCH -next 2/5] drm/mediatek: Fix missing of_node_put() for mtk_drm_get_all_drm_priv()

[Marion & Christophe JAILLET]

Le 23/08/2024 à 12:46, Christophe JAILLET a écrit :
>> @@ -933,10 +931,8 @@ static int mtk_drm_probe(struct platform_device 
>> *pdev)
>>           }
>>           ret = mtk_ddp_comp_init(node, &private->ddp_comp[comp_id], 
>> comp_id);
>> -        if (ret) {
>> -            of_node_put(node);
>> +        if (ret)
>>               goto err_node;
> 
> Hi,
> 
> I've seen on another thread that is was not sure that scoped versions 
> and gotos played well together.
> 
> It was asked to check more in details and confirm that it was safe 
> before applying the patch.
> 
> I've not followed the discussion, so I just point it out, in case it helps.
> 
> I'll try to give it a look in the coming days.
> 
> 
> CJ
> 

Hi,
looking at the generated asm file (gcc 14.2.1), everything looks fine.

# drivers/gpu/drm/mediatek/mtk_drm_drv.c:933: 		ret = 
mtk_ddp_comp_init(node, &private->ddp_comp[comp_id], comp_id);
	salq	$5, %rax	#, _36
	movl	%r14d, %edx	# comp_id,
	movq	%rbx, %rdi	# node,
	leaq	552(%rbp,%rax), %rsi	#, _28
	call	mtk_ddp_comp_init	#
	movl	%eax, %r12d	# tmp205, <retval>
# drivers/gpu/drm/mediatek/mtk_drm_drv.c:934: 		if (ret)
	testl	%eax, %eax	# <retval>
	jne	.L212	#,

...

.L212:
# ./include/linux/of.h:138: DEFINE_FREE(device_node, struct device_node 
*, if (_T) of_node_put(_T))
	movq	%rbx, %rdi	# node,
	call	of_node_put	#
	jmp	.L171	#

CJ

Re: [PATCH -next 1/5] drm/rockchip: Use for_each_child_of_node_scoped()

[Jinjie Ruan]

On 2024/8/23 19:32, Jonathan Cameron wrote:
> On Fri, 23 Aug 2024 17:20:49 +0800
> Jinjie Ruan <ruanjinjie@huawei.com> wrote:
> 
>> Avoids the need for manual cleanup of_node_put() in early exits
>> from the loop.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> 
> There is more to do here, and looking at the code, I'm far from
> sure it isn't releasing references it never had.
> 
>> ---
>>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> index 9a01aa450741..f5b3f18794dd 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> @@ -548,7 +548,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>>  	struct drm_encoder *encoder;
>>  	struct drm_connector *connector;
>>  	struct device_node *remote = NULL;
>> -	struct device_node  *port, *endpoint;
> 
> Odd extra space before *port in original. Clean that up whilst here.
> 
> 
>> +	struct device_node  *port;
> 
> Use __free(device_node) for *port as well.
> 
> So where the current asignment is.
> 	struct device_node *port = of_graph_get_port_by_id(dev->of_node, 1);
> 
>>  	int ret = 0, child_count = 0;
>>  	const char *name;
>>  	u32 endpoint_id = 0;
>> @@ -560,15 +560,13 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>>  			      "can't found port point, please init lvds panel port!\n");
>>  		return -EINVAL;
>>  	}
>> -	for_each_child_of_node(port, endpoint) {
>> +	for_each_child_of_node_scoped(port, endpoint) {
>>  		child_count++;
>>  		of_property_read_u32(endpoint, "reg", &endpoint_id);
>>  		ret = drm_of_find_panel_or_bridge(dev->of_node, 1, endpoint_id,
>>  						  &lvds->panel, &lvds->bridge);
>> -		if (!ret) {
>> -			of_node_put(endpoint);
>> +		if (!ret)
>>  			break;
> 
> This then can simply be
> 			return dev_err_probe(dev, ret,
> 					     "failed to find pannel and bridge node\n");
>> -		}

Thank you! I'll resend and cleanup them.

> 
> Various other paths become direct returns as well.
> 
>>  	}
> 
> The later code with remote looks suspect as not obvious who got the reference that
> is being put but assuming that is correct, it's another possible place for __free based
> cleanup.
> 
> 
>>  	if (!child_count) {
>>  		DRM_DEV_ERROR(dev, "lvds port does not have any children\n");
> 

Re: [PATCH -next 1/5] drm/rockchip: Use for_each_child_of_node_scoped()

[Jinjie Ruan]

On 2024/8/23 19:32, Jonathan Cameron wrote:
> On Fri, 23 Aug 2024 17:20:49 +0800
> Jinjie Ruan <ruanjinjie@huawei.com> wrote:
> 
>> Avoids the need for manual cleanup of_node_put() in early exits
>> from the loop.
>>
>> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> 
> There is more to do here, and looking at the code, I'm far from
> sure it isn't releasing references it never had.
> 
>> ---
>>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> index 9a01aa450741..f5b3f18794dd 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
>> @@ -548,7 +548,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>>  	struct drm_encoder *encoder;
>>  	struct drm_connector *connector;
>>  	struct device_node *remote = NULL;
>> -	struct device_node  *port, *endpoint;
> 
> Odd extra space before *port in original. Clean that up whilst here.
> 
> 
>> +	struct device_node  *port;
> 
> Use __free(device_node) for *port as well.

Yes，that is right.

> 
> So where the current asignment is.
> 	struct device_node *port = of_graph_get_port_by_id(dev->of_node, 1);
> 
>>  	int ret = 0, child_count = 0;
>>  	const char *name;
>>  	u32 endpoint_id = 0;
>> @@ -560,15 +560,13 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>>  			      "can't found port point, please init lvds panel port!\n");
>>  		return -EINVAL;
>>  	}
>> -	for_each_child_of_node(port, endpoint) {
>> +	for_each_child_of_node_scoped(port, endpoint) {
>>  		child_count++;
>>  		of_property_read_u32(endpoint, "reg", &endpoint_id);
>>  		ret = drm_of_find_panel_or_bridge(dev->of_node, 1, endpoint_id,
>>  						  &lvds->panel, &lvds->bridge);
>> -		if (!ret) {
>> -			of_node_put(endpoint);
>> +		if (!ret)
>>  			break;
> 
> This then can simply be
> 			return dev_err_probe(dev, ret,
> 					     "failed to find pannel and bridge node\n");
>> -		}

It seems to me there's no easy way return here, as it will try
drm_of_find_panel_or_bridge() for each child node, only "child_count =
0" or all child node  drm_of_find_panel_or_bridge() fails it will error
and return.

> 
> Various other paths become direct returns as well.
> 
>>  	}
> 
> The later code with remote looks suspect as not obvious who got the reference that
> is being put but assuming that is correct, it's another possible place for __free based
> cleanup.

Yes, the remote looks suspect.

> 
> 
>>  	if (!child_count) {
>>  		DRM_DEV_ERROR(dev, "lvds port does not have any children\n");
> 

Re: [PATCH -next 2/5] drm/mediatek: Fix missing of_node_put() for mtk_drm_get_all_drm_priv()

[Jinjie Ruan]

On 2024/8/25 13:16, Marion & Christophe JAILLET wrote:
> 
> 
> Le 23/08/2024 à 12:46, Christophe JAILLET a écrit :
>>> @@ -933,10 +931,8 @@ static int mtk_drm_probe(struct platform_device
>>> *pdev)
>>>           }
>>>           ret = mtk_ddp_comp_init(node, &private->ddp_comp[comp_id],
>>> comp_id);
>>> -        if (ret) {
>>> -            of_node_put(node);
>>> +        if (ret)
>>>               goto err_node;
>>
>> Hi,
>>
>> I've seen on another thread that is was not sure that scoped versions
>> and gotos played well together.
>>
>> It was asked to check more in details and confirm that it was safe
>> before applying the patch.
>>
>> I've not followed the discussion, so I just point it out, in case it
>> helps.
>>
>> I'll try to give it a look in the coming days.
>>
>>
>> CJ
>>
> 
> Hi,
> looking at the generated asm file (gcc 14.2.1), everything looks fine.

Yes, as I pointed out in another thread, the test show that goto with
this scoped function is good.

> 
> # drivers/gpu/drm/mediatek/mtk_drm_drv.c:933:         ret =
> mtk_ddp_comp_init(node, &private->ddp_comp[comp_id], comp_id);
>     salq    $5, %rax    #, _36
>     movl    %r14d, %edx    # comp_id,
>     movq    %rbx, %rdi    # node,
>     leaq    552(%rbp,%rax), %rsi    #, _28
>     call    mtk_ddp_comp_init    #
>     movl    %eax, %r12d    # tmp205, <retval>
> # drivers/gpu/drm/mediatek/mtk_drm_drv.c:934:         if (ret)
>     testl    %eax, %eax    # <retval>
>     jne    .L212    #,
> 
> ...
> 
> .L212:
> # ./include/linux/of.h:138: DEFINE_FREE(device_node, struct device_node
> *, if (_T) of_node_put(_T))
>     movq    %rbx, %rdi    # node,
>     call    of_node_put    #
>     jmp    .L171    #
> 
> CJ
> 

Re: [PATCH -next 1/5] drm/rockchip: Use for_each_child_of_node_scoped()

[Jonathan Cameron]

On Tue, 27 Aug 2024 09:40:07 +0800
Jinjie Ruan <ruanjinjie@huawei.com> wrote:

> On 2024/8/23 19:32, Jonathan Cameron wrote:
> > On Fri, 23 Aug 2024 17:20:49 +0800
> > Jinjie Ruan <ruanjinjie@huawei.com> wrote:
> >   
> >> Avoids the need for manual cleanup of_node_put() in early exits
> >> from the loop.
> >>
> >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>  
> > 
> > There is more to do here, and looking at the code, I'm far from
> > sure it isn't releasing references it never had.
> >   
> >> ---
> >>  drivers/gpu/drm/rockchip/rockchip_lvds.c | 8 +++-----
> >>  1 file changed, 3 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> >> index 9a01aa450741..f5b3f18794dd 100644
> >> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> >> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> >> @@ -548,7 +548,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
> >>  	struct drm_encoder *encoder;
> >>  	struct drm_connector *connector;
> >>  	struct device_node *remote = NULL;
> >> -	struct device_node  *port, *endpoint;  
> > 
> > Odd extra space before *port in original. Clean that up whilst here.
> > 
> >   
> >> +	struct device_node  *port;  
> > 
> > Use __free(device_node) for *port as well.  
> 
> Yes，that is right.
> 
> > 
> > So where the current asignment is.
> > 	struct device_node *port = of_graph_get_port_by_id(dev->of_node, 1);
> >   
> >>  	int ret = 0, child_count = 0;
> >>  	const char *name;
> >>  	u32 endpoint_id = 0;
> >> @@ -560,15 +560,13 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
> >>  			      "can't found port point, please init lvds panel port!\n");
> >>  		return -EINVAL;
> >>  	}
> >> -	for_each_child_of_node(port, endpoint) {
> >> +	for_each_child_of_node_scoped(port, endpoint) {
> >>  		child_count++;
> >>  		of_property_read_u32(endpoint, "reg", &endpoint_id);
> >>  		ret = drm_of_find_panel_or_bridge(dev->of_node, 1, endpoint_id,
> >>  						  &lvds->panel, &lvds->bridge);
> >> -		if (!ret) {
> >> -			of_node_put(endpoint);
> >> +		if (!ret)
> >>  			break;  
> > 
> > This then can simply be
> > 			return dev_err_probe(dev, ret,
> > 					     "failed to find pannel and bridge node\n");  
> >> -		}  
> 
> It seems to me there's no easy way return here, as it will try
> drm_of_find_panel_or_bridge() for each child node, only "child_count =
> 0" or all child node  drm_of_find_panel_or_bridge() fails it will error
> and return.
Ah. Good point. That is an odd code structure that I read wrong but it indeed
carries on and ignores the error if for an earlier loop
the drm_of_find_pannel_or_bridge() failed and a later one succeeds.

If you want to make it more 'standard I'd do
if (ret)
	continue;

and have the code code path of the early break 'inline'

e.g.
	for_each_child_of_node(port, endpoint) {
		child_count++;
		of_property_read_u32(endpoint, "reg", &endpoint_id);
		ret = drm_of_find_panel_or_bridge(dev->of_node, 1, endpoint_id,
						  &lvds->panel, &lvds->bridge);
		if (ret)
			continue;

		of_node_put(endpoint);
		break;
	}

I'd also be tempted to pull the child_count before this with

if (of_get_child_count() == 0) {
	DRM_DEV_ERROR(dev, "...");
	return -EINVAL;

Then can simply check ret at the end of the loop rather than needing
the else if as we can't get there with child_count non zero.
Can also drop the increment of child_count in the loop. So overall that
becomes something like

	if (of_get_child_count(endpoint) == 0) {
		DRM_DEV_ERROR(dev, "...");
		return -EINVAL;
	}

	for_each_child_of_node_scoped(port, endpoint) {
		of_property_read_u32(endpoint, "reg", &endpoint_id);
		ret = drm_of_find_panel_or_bridge(dev->of_node, 1, endpoint_id,
						  &lvds->panel, &lvds->bridge);
		/* A later child node may succeed */
		if (ret)
			continue;
		break;
	}
	if (ret)
		return dev_err_probe();


> 
> > 
> > Various other paths become direct returns as well.
> >   
> >>  	}  
> > 
> > The later code with remote looks suspect as not obvious who got the reference that
> > is being put but assuming that is correct, it's another possible place for __free based
> > cleanup.  
> 
> Yes, the remote looks suspect.
> 
> > 
> >   
> >>  	if (!child_count) {
> >>  		DRM_DEV_ERROR(dev, "lvds port does not have any children\n");  
> >