diff mbox series

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

Message ID 20240823092053.3170445-2-ruanjinjie@huawei.com (mailing list archive)
State New
Headers show
Series drm: Use for_each_child_of_node_scoped() | expand

Commit Message

Jinjie Ruan Aug. 23, 2024, 9:20 a.m. UTC
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(-)

Comments

Heiko Stuebner Aug. 23, 2024, 9:45 a.m. UTC | #1
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");
>
Jonathan Cameron Aug. 23, 2024, 11:32 a.m. UTC | #2
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");
Jinjie Ruan Aug. 26, 2024, 9:04 a.m. UTC | #3
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");
>
Jinjie Ruan Aug. 27, 2024, 1:40 a.m. UTC | #4
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");
>
Jonathan Cameron Aug. 27, 2024, 9:49 a.m. UTC | #5
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");  
> >
diff mbox series

Patch

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");