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 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
Attachment:
signature.asc
Description: PGP signature
