Hi Thierry Reding,
> -----Original Message-----
> From: Thierry Reding <thierry.reding@xxxxxxxxx>
> Sent: 04 February 2025 18:02
> Subject: Re: [PATCH] drm/tegra: rgb: Simplify tegra_dc_rgb_probe()
>
> On Tue, Feb 04, 2025 at 03:33:53PM +0000, Biju Das wrote:
> > Hi Thierry Reding,
> >
> > > -----Original Message-----
> > > From: Thierry Reding <thierry.reding@xxxxxxxxx>
> > > Sent: 04 February 2025 15:25
> > > Subject: Re: [PATCH] drm/tegra: rgb: Simplify tegra_dc_rgb_probe()
> > >
> > > On Tue, Feb 04, 2025 at 09:07:05AM +0000, Biju Das wrote:
> > > > Hi Geert,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > -----Original Message-----
> > > > > From: dri-devel <dri-devel-bounces@xxxxxxxxxxxxxxxxxxxxx> On
> > > > > Behalf Of Geert Uytterhoeven
> > > > > Sent: 03 February 2025 11:06
> > > > > Subject: Re: [PATCH] drm/tegra: rgb: Simplify
> > > > > tegra_dc_rgb_probe()
> > > > >
> > > > > Hi Biju,
> > > > >
> > > > > Thanks for your patch!
> > > > >
> > > > > On Sat, 1 Feb 2025 at 11:57, Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
> > > > > > Simplify tegra_dc_rgb_probe() by using of_get_available_child_by_name().
> > > > >
> > > > > That's not the only thing this patch does...
> > > > >
> > > > > >
> > > > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > > >
> > > > > > --- a/drivers/gpu/drm/tegra/rgb.c
> > > > > > +++ b/drivers/gpu/drm/tegra/rgb.c
> > > > > > @@ -202,12 +202,12 @@ static const struct
> > > > > > drm_encoder_helper_funcs tegra_rgb_encoder_helper_funcs = {
> > > > > >
> > > > > > int tegra_dc_rgb_probe(struct tegra_dc *dc) {
> > > > > > - struct device_node *np;
> > > > > > + struct device_node *np _free(device_node) =
> > > > >
> > > > > Adding the _free()...
> > > >
> > > > Yes it fixes a memory leak aswell.
> > > >
> > > > >
> > > > > > +
> > > > > > + of_get_available_child_by_name(dc->dev->of_node,
> > > > > > + "rgb");
> > > > > > struct tegra_rgb *rgb;
> > > > > > int err;
> > > > > >
> > > > > > - np = of_get_child_by_name(dc->dev->of_node, "rgb");
> > > > > > - if (!np || !of_device_is_available(np))
> > > > > > + if (!np)
> > > > > > return -ENODEV;
> > > > >
> > > > > ... fixes the reference count in case of an unavailable node...
> > > > >
> > > > > >
> > > > > > rgb = devm_kzalloc(dc->dev, sizeof(*rgb), GFP_KERNEL);
> > > > >
> > > > > ... but as np is stored below, it must not be freed when it goes out of context?
> > > >
> > > > OK, But it is used in tegra_output_probe() and never freed.
> > > > Maybe remove should free it??
> > >
> > > It's not quite as simple as that. tegra_output_probe() can also
> > > store
> > > output->dev->of_node in output->of_node, so we'd also need to track
> > > output->dev->a
> > > flag of some sort to denote that this needs to be freed.
> >
> > OK.
> >
> > >
> > > Ultimately I'm not sure if it's really worth it. Do we really expect
> > > these nodes to ever be freed (in which case this might leak memory)?
> > > These are built-in devices and there's no code anywhere to remove any such nodes.
> >
> > If there is no use case for bind/rebind for the built-in device then no issue.
> > Or in .remove() free the node or use dev_action_reset()??
>
> Bind/rebind is possible, but that's not even a problem. Worst case the reference count on the device
> node will keep increasing, so we'll just keep leaking the same node over and over again. I guess
> potentially there's a problem when we rebind for the 2^32-th time, but I'm not sure that's something
> we need to consider.
Agreed.
>
> That said, devm_add_action_or_reset() sounds like a good solution if we really want to make sure that
> this doesn't leak, so yeah, I'm in favour of that.
OK, Will send a patch after of_get_available_child_by_name() [1] hit on mainline.
https://lore.kernel.org/all/TY3PR01MB1134656CBDAF5FFCEB6C8D20F86F42@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
Cheers,
Biju
