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 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()??
Cheers,
Biju
