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