[PATCH] drm: sti: fix device leaks at component probe

[PATCH] drm: sti: fix device leaks at component probe

[Johan Hovold]

Make sure to drop the references taken to the vtg devices by
of_find_device_by_node() when looking up their driver data during
component probe.

Note that holding a reference to a platform device does not prevent its
driver data from going away so there is no point in keeping the
reference after the lookup helper returns.

Fixes: cc6b741c6f63 ("drm: sti: remove useless fields from vtg structure")
Cc: stable@vger.kernel.org	# 4.16
Cc: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/gpu/drm/sti/sti_vtg.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sti/sti_vtg.c b/drivers/gpu/drm/sti/sti_vtg.c
index ee81691b3203..ce6bc7e7b135 100644
--- a/drivers/gpu/drm/sti/sti_vtg.c
+++ b/drivers/gpu/drm/sti/sti_vtg.c
@@ -143,12 +143,17 @@ struct sti_vtg {
 struct sti_vtg *of_vtg_find(struct device_node *np)
 {
 	struct platform_device *pdev;
+	struct sti_vtg *vtg;
 
 	pdev = of_find_device_by_node(np);
 	if (!pdev)
 		return NULL;
 
-	return (struct sti_vtg *)platform_get_drvdata(pdev);
+	vtg = platform_get_drvdata(pdev);
+
+	put_device(&pdev->dev);
+
+	return vtg;
 }
 
 static void vtg_reset(struct sti_vtg *vtg)
-- 
2.49.1

Re: [PATCH] drm: sti: fix device leaks at component probe

[Markus Elfring]

> Make sure to drop the references taken to the vtg devices by

                                                VTG device?


> of_find_device_by_node() when looking up their driver data during
> component probe.
…

How do you think about to increase the application of scope-based resource management?
https://elixir.bootlin.com/linux/v6.17-rc7/source/include/linux/device.h#L1180

Can a summary phrase like “Prevent device leak in of_vtg_find()” be nicer?

Regards,
Markus

Re: [PATCH] drm: sti: fix device leaks at component probe

[Raphaël Gallais-Pou]

Le Mon, Sep 22, 2025 at 06:16:47PM +0200, Markus Elfring a écrit :
> > Make sure to drop the references taken to the vtg devices by
> 
>                                                 VTG device?

Video Timing Generator.  This IP creates a vsync pulse and synchonize
the components together.

> 
> 
> > of_find_device_by_node() when looking up their driver data during
> > component probe.
> …
> 
> How do you think about to increase the application of scope-based resource management?
> https://elixir.bootlin.com/linux/v6.17-rc7/source/include/linux/device.h#L1180

Oh... I wasn't aware of this.  FWIU it is a way to directly free an
allocated memory whenever a variable goes out of scope using the cleanup
attribute.

IMO this is also a clever solution to prevent the memory leak, and it
would be a shorter patch.  So basically, instead of calling put_device()
as Johan did, you would suggest something like this ?

diff --git i/drivers/gpu/drm/sti/sti_vtg.c w/drivers/gpu/drm/sti/sti_vtg.c
index ee81691b3203..5193196d9291 100644
--- i/drivers/gpu/drm/sti/sti_vtg.c
+++ w/drivers/gpu/drm/sti/sti_vtg.c
@@ -142,7 +142,7 @@ struct sti_vtg {

 struct sti_vtg *of_vtg_find(struct device_node *np)
 {
-       struct platform_device *pdev;
+       struct platform_device *pdev __free(put_device) = NULL;

Best regards,
Raphaël


> 
> Can a summary phrase like “Prevent device leak in of_vtg_find()” be nicer?
> 
> Regards,
> Markus

Re: [PATCH] drm: sti: fix device leaks at component probe

[Markus Elfring]

…> +++ w/drivers/gpu/drm/sti/sti_vtg.c
> @@ -142,7 +142,7 @@ struct sti_vtg {
> 
>  struct sti_vtg *of_vtg_find(struct device_node *np)
>  {
> -       struct platform_device *pdev;
> +       struct platform_device *pdev __free(put_device) = NULL;
…

The scope may be reduced for such a variable.

Regards,
Markus

Re: [PATCH] drm: sti: fix device leaks at component probe

[Johan Hovold]

On Mon, Nov 03, 2025 at 07:56:55PM +0100, Raphaël Gallais-Pou wrote:

> diff --git i/drivers/gpu/drm/sti/sti_vtg.c w/drivers/gpu/drm/sti/sti_vtg.c
> index ee81691b3203..5193196d9291 100644
> --- i/drivers/gpu/drm/sti/sti_vtg.c
> +++ w/drivers/gpu/drm/sti/sti_vtg.c
> @@ -142,7 +142,7 @@ struct sti_vtg {
> 
>  struct sti_vtg *of_vtg_find(struct device_node *np)
>  {
> -       struct platform_device *pdev;
> +       struct platform_device *pdev __free(put_device) = NULL;

You'd need to declare the variable when looking up pdev, which is one of
the reasons I don't like the cleanup helpers. It also often makes the
code harder to reason about for no good reason (especially with some of
the more esoteric cleanup helpers).

Keep it simple.

Johan

Re: [PATCH] drm: sti: fix device leaks at component probe

[Johan Hovold]

On Mon, Sep 22, 2025 at 02:20:12PM +0200, Johan Hovold wrote:
> Make sure to drop the references taken to the vtg devices by
> of_find_device_by_node() when looking up their driver data during
> component probe.
> 
> Note that holding a reference to a platform device does not prevent its
> driver data from going away so there is no point in keeping the
> reference after the lookup helper returns.
> 
> Fixes: cc6b741c6f63 ("drm: sti: remove useless fields from vtg structure")
> Cc: stable@vger.kernel.org	# 4.16
> Cc: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

Can this one be picked up for 6.19?

Johan

Re: [PATCH] drm: sti: fix device leaks at component probe

[Raphaël Gallais-Pou]

Hi Johan, 

For some reason this thread went through my filters, sorry.

Le Mon, Sep 22, 2025 at 02:20:12PM +0200, Johan Hovold a écrit :
> Make sure to drop the references taken to the vtg devices by
> of_find_device_by_node() when looking up their driver data during
> component probe.

Markus suggested “Prevent device leak in of_vtg_find()” as commit
summary.

> 
> Note that holding a reference to a platform device does not prevent its
> driver data from going away so there is no point in keeping the
> reference after the lookup helper returns.
> 
> Fixes: cc6b741c6f63 ("drm: sti: remove useless fields from vtg structure")
> Cc: stable@vger.kernel.org	# 4.16
> Cc: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  drivers/gpu/drm/sti/sti_vtg.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/sti/sti_vtg.c b/drivers/gpu/drm/sti/sti_vtg.c
> index ee81691b3203..ce6bc7e7b135 100644
> --- a/drivers/gpu/drm/sti/sti_vtg.c
> +++ b/drivers/gpu/drm/sti/sti_vtg.c
> @@ -143,12 +143,17 @@ struct sti_vtg {
>  struct sti_vtg *of_vtg_find(struct device_node *np)
>  {
>  	struct platform_device *pdev;
> +	struct sti_vtg *vtg;
>  
>  	pdev = of_find_device_by_node(np);
>  	if (!pdev)
>  		return NULL;
>  
> -	return (struct sti_vtg *)platform_get_drvdata(pdev);
> +	vtg = platform_get_drvdata(pdev);
> +
> +	put_device(&pdev->dev);

I would prefer of_node_put() instead, which does the same basically, but
at least it is more obviously linked to of_find_device_by_node().

Best regards,
Raphaël
> +
> +	return vtg;
>  }
>  
>  static void vtg_reset(struct sti_vtg *vtg)
> -- 
> 2.49.1
> 

Re: [PATCH] drm: sti: fix device leaks at component probe

[Johan Hovold]

On Fri, Oct 31, 2025 at 06:10:46PM +0100, Raphaël Gallais-Pou wrote:

> Le Mon, Sep 22, 2025 at 02:20:12PM +0200, Johan Hovold a écrit :
> > Make sure to drop the references taken to the vtg devices by
> > of_find_device_by_node() when looking up their driver data during
> > component probe.
> 
> Markus suggested “Prevent device leak in of_vtg_find()” as commit
> summary.

Markus has gotten himself banned from the mailing lists some years ago
and even if he is now back with a new mail address most of us still
ignore him.

I prefer the Subject as it stands since it captures when the leaks
happens, but I don't mind mentioning of_vtg_find() instead if you
insist.

> > Note that holding a reference to a platform device does not prevent its
> > driver data from going away so there is no point in keeping the
> > reference after the lookup helper returns.
> > 
> > Fixes: cc6b741c6f63 ("drm: sti: remove useless fields from vtg structure")
> > Cc: stable@vger.kernel.org	# 4.16
> > Cc: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> > ---
> >  drivers/gpu/drm/sti/sti_vtg.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/sti/sti_vtg.c b/drivers/gpu/drm/sti/sti_vtg.c
> > index ee81691b3203..ce6bc7e7b135 100644
> > --- a/drivers/gpu/drm/sti/sti_vtg.c
> > +++ b/drivers/gpu/drm/sti/sti_vtg.c
> > @@ -143,12 +143,17 @@ struct sti_vtg {
> >  struct sti_vtg *of_vtg_find(struct device_node *np)
> >  {
> >  	struct platform_device *pdev;
> > +	struct sti_vtg *vtg;
> >  
> >  	pdev = of_find_device_by_node(np);
> >  	if (!pdev)
> >  		return NULL;
> >  
> > -	return (struct sti_vtg *)platform_get_drvdata(pdev);
> > +	vtg = platform_get_drvdata(pdev);
> > +
> > +	put_device(&pdev->dev);
> 
> I would prefer of_node_put() instead, which does the same basically, but
> at least it is more obviously linked to of_find_device_by_node().

of_node_put() operates on OF nodes, but here it is the platform device
that is leaking.

> > +
> > +	return vtg;
> >  }

Johan