[PATCH] drm/tegra: rgb: Simplify tegra_dc_rgb_probe()

[Biju Das]

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert at linux-m68k.org>
> Sent: 05 February 2025 10:25
> Subject: Re: [PATCH] drm/tegra: rgb: Simplify tegra_dc_rgb_probe()
> 
> Hi Biju,
> 
> On Wed, 5 Feb 2025 at 11:20, Biju Das <biju.das.jz at bp.renesas.com> wrote:
> > > -----Original Message-----
> > > From: Thierry Reding <thierry.reding at gmail.com> On Tue, Feb 04, 2025
> > > at 03:33:53PM +0000, Biju Das wrote:
> > > > > -----Original Message-----
> > > > > From: Thierry Reding <thierry.reding at gmail.com> On Tue, Feb 04,
> > > > > 2025 at 09:07:05AM +0000, Biju Das wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: dri-devel <dri-devel-bounces at lists.freedesktop.org> On
> > > > > > > Behalf Of Geert Uytterhoeven
> > > > > > > Sent: 03 February 2025 11:06
> > > > > > > Subject: Re: [PATCH] drm/tegra: rgb: Simplify
> > > > > > > tegra_dc_rgb_probe()
> > > > > > >
> > > > > > > On Sat, 1 Feb 2025 at 11:57, Biju Das <biju.das.jz at bp.renesas.com> 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 at bp.renesas.com>
> > > > > > >
> > > > > > > > --- 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
> > > > > output->dev->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()??
> > >
> > > 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.
> 
> Can't you already fix the unbound reference count now, as it is orthogonal to the conversion to
> of_get_available_child_by_name()?

Sure, Will send now.

Cheers,
Biju