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)
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#L11...
Can a summary phrase like “Prevent device leak in of_vtg_find()” be nicer?
Regards, Markus
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#L11...
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
…> +++ 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
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
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
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
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
linux-stable-mirror@lists.linaro.org
-
Johan Hovold -
Markus Elfring -
Raphaël Gallais-Pou