| Message ID | 20250201105658.37043-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/tegra: rgb: Simplify tegra_dc_rgb_probe() | expand |
Hi Biju, Thanks for your patch! On Sat, 1 Feb 2025 at 11:57, Biju Das <biju.das.jz@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@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()... > + 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? Gr{oetje,eeting}s, Geert
Hi Geert, Thanks for the feedback. > -----Original Message----- > From: dri-devel <dri-devel-bounces@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@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@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?? Cheers, Biju
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@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@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@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. 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. Thierry
Hi Thierry Reding, > -----Original Message----- > From: Thierry Reding <thierry.reding@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@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@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@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()?? Cheers, Biju
On Tue, Feb 04, 2025 at 03:33:53PM +0000, Biju Das wrote: > Hi Thierry Reding, > > > -----Original Message----- > > From: Thierry Reding <thierry.reding@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@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@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@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
diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c index 1e8ec50b759e..6e540291a960 100644 --- 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) = + 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; rgb = devm_kzalloc(dc->dev, sizeof(*rgb), GFP_KERNEL);
Simplify tegra_dc_rgb_probe() by using of_get_available_child_by_name(). Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- This patch is only compile tested and depend upon[1] [1] https://lore.kernel.org/all/20250201093126.7322-1-biju.das.jz@bp.renesas.com/ --- drivers/gpu/drm/tegra/rgb.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)