On Tue, 27 Aug 2024 09:40:07 +0800
Jinjie Ruan <ruanjinjie@xxxxxxxxxx> wrote:
> On 2024/8/23 19:32, Jonathan Cameron wrote:
> > On Fri, 23 Aug 2024 17:20:49 +0800
> > Jinjie Ruan <ruanjinjie@xxxxxxxxxx> wrote:
> >
> >> Avoids the need for manual cleanup of_node_put() in early exits
> >> from the loop.
> >>
> >> Signed-off-by: Jinjie Ruan <ruanjinjie@xxxxxxxxxx>
> >
> > 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");
> >
