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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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