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

[Thierry Reding]

On Tue, Feb 04, 2025 at 03:33:53PM +0000, Biju Das wrote:
> Hi Thierry Reding,
> 
> > -----Original Message-----
> > From: Thierry Reding <thierry.reding at gmail.com>
> > 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 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()
> > > >
> > > > Hi Biju,
> > > >
> > > > Thanks for your patch!
> > > >
> > > > 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 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.

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.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20250204/8dbda5b6/attachment.sig>