| Message ID | 20240823092053.3170445-2-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Use for_each_child_of_node_scoped() | expand |
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"); >
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");
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"); >
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"); >
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 --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");
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(-)