[PATCH v2] drm/vc4: release firmware on error paths in vc4_hvs_bind()

[PATCH v2] drm/vc4: release firmware on error paths in vc4_hvs_bind()

[Dan Carpenter]

There was a bug where we should have called rpi_firmware_put(firmware)
if devm_clk_get() failed.  Really, it's better and more readable to
move all the firmware code together so that we can release it one
time.

Fixes: 2fa4ef5fb943 ("drm/vc4: hvs: Create hw_init function")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
v2: Use a cleaner solution

 drivers/gpu/drm/vc4/vc4_hvs.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
index 4811d794001f..06aedd906d74 100644
--- a/drivers/gpu/drm/vc4/vc4_hvs.c
+++ b/drivers/gpu/drm/vc4/vc4_hvs.c
@@ -1675,6 +1675,17 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
 		if (!firmware)
 			return -EPROBE_DEFER;
 
+		max_rate = rpi_firmware_clk_get_max_rate(firmware,
+							 RPI_FIRMWARE_CORE_CLK_ID);
+		rpi_firmware_put(firmware);
+		if (max_rate >= 550000000)
+			hvs->vc5_hdmi_enable_hdmi_20 = true;
+
+		if (max_rate >= 600000000)
+			hvs->vc5_hdmi_enable_4096by2160 = true;
+
+		hvs->max_core_rate = max_rate;
+
 		hvs->core_clk = devm_clk_get(&pdev->dev,
 					     (vc4->gen >= VC4_GEN_6_C) ? "core" : NULL);
 		if (IS_ERR(hvs->core_clk)) {
@@ -1689,17 +1700,6 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
 			return PTR_ERR(hvs->disp_clk);
 		}
 
-		max_rate = rpi_firmware_clk_get_max_rate(firmware,
-							 RPI_FIRMWARE_CORE_CLK_ID);
-		rpi_firmware_put(firmware);
-		if (max_rate >= 550000000)
-			hvs->vc5_hdmi_enable_hdmi_20 = true;
-
-		if (max_rate >= 600000000)
-			hvs->vc5_hdmi_enable_4096by2160 = true;
-
-		hvs->max_core_rate = max_rate;
-
 		ret = clk_prepare_enable(hvs->core_clk);
 		if (ret) {
 			dev_err(&pdev->dev, "Couldn't enable the core clock\n");
-- 
2.47.2

Re: [PATCH v2] drm/vc4: release firmware on error paths in vc4_hvs_bind()

[Dave Stevenson]

On Thu, 3 Apr 2025 at 13:14, Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> There was a bug where we should have called rpi_firmware_put(firmware)
> if devm_clk_get() failed.  Really, it's better and more readable to
> move all the firmware code together so that we can release it one
> time.
>
> Fixes: 2fa4ef5fb943 ("drm/vc4: hvs: Create hw_init function")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Thanks for the respin

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> ---
> v2: Use a cleaner solution
>
>  drivers/gpu/drm/vc4/vc4_hvs.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
> index 4811d794001f..06aedd906d74 100644
> --- a/drivers/gpu/drm/vc4/vc4_hvs.c
> +++ b/drivers/gpu/drm/vc4/vc4_hvs.c
> @@ -1675,6 +1675,17 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
>                 if (!firmware)
>                         return -EPROBE_DEFER;
>
> +               max_rate = rpi_firmware_clk_get_max_rate(firmware,
> +                                                        RPI_FIRMWARE_CORE_CLK_ID);
> +               rpi_firmware_put(firmware);
> +               if (max_rate >= 550000000)
> +                       hvs->vc5_hdmi_enable_hdmi_20 = true;
> +
> +               if (max_rate >= 600000000)
> +                       hvs->vc5_hdmi_enable_4096by2160 = true;
> +
> +               hvs->max_core_rate = max_rate;
> +
>                 hvs->core_clk = devm_clk_get(&pdev->dev,
>                                              (vc4->gen >= VC4_GEN_6_C) ? "core" : NULL);
>                 if (IS_ERR(hvs->core_clk)) {
> @@ -1689,17 +1700,6 @@ static int vc4_hvs_bind(struct device *dev, struct device *master, void *data)
>                         return PTR_ERR(hvs->disp_clk);
>                 }
>
> -               max_rate = rpi_firmware_clk_get_max_rate(firmware,
> -                                                        RPI_FIRMWARE_CORE_CLK_ID);
> -               rpi_firmware_put(firmware);
> -               if (max_rate >= 550000000)
> -                       hvs->vc5_hdmi_enable_hdmi_20 = true;
> -
> -               if (max_rate >= 600000000)
> -                       hvs->vc5_hdmi_enable_4096by2160 = true;
> -
> -               hvs->max_core_rate = max_rate;
> -
>                 ret = clk_prepare_enable(hvs->core_clk);
>                 if (ret) {
>                         dev_err(&pdev->dev, "Couldn't enable the core clock\n");
> --
> 2.47.2
>

Re: [PATCH v2] drm/vc4: release firmware on error paths in vc4_hvs_bind()

[Tomasz Unger]

On Thu, Apr 03, 2025 at 03:14:33PM +0300, Dan Carpenter wrote:
> There was a bug where we should have called rpi_firmware_put(firmware)
> if devm_clk_get() failed.  Really, it's better and more readable to
> move all the firmware code together so that we can release it one
> time.
>
> Fixes: 2fa4ef5fb943 ("drm/vc4: hvs: Create hw_init function")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Hi Dan,

It looks like this patch was never applied: the leak is still there in
v7.4-rc1. The patch still applies on top of v7.4-rc1 (with an offset), so
I gave it a test on a Raspberry Pi 4 Model B Rev 1.5 (arm64 defconfig).

Normal path: with and without the patch vc4 binds the HVS and the other
components in the same way, the core clock is acquired by fe400000.hvs
and dmesg shows the same messages in both cases.

Error path: to reach it I forced the core clock lookup to fail with a
test-only change (hvs->core_clk = ERR_PTR(-ENODEV) right after the
devm_clk_get() call) and traced rpi_firmware_get()/rpi_firmware_put()
with the function tracer while re-binding the vc4-drm driver.

Without the patch:

  rpi_firmware_get <-vc4_drm_bind
  rpi_firmware_put <-vc4_drm_bind
  rpi_firmware_get <-vc4_hvs_bind

With the patch:

  rpi_firmware_get <-vc4_drm_bind
  rpi_firmware_put <-vc4_drm_bind
  rpi_firmware_get <-vc4_hvs_bind
  rpi_firmware_put <-vc4_hvs_bind

So without the patch every failed bind leaks one reference, and with the
patch the reference is dropped.

Tested-by: Tomasz Unger <tomasz.unger@yahoo.pl>

One note about the Fixes tag: 2fa4ef5fb943 only moved this code around.
As far as I can tell, the rpi_firmware_get() before devm_clk_get()
without a put on the error path was introduced by commit 2a001ca00ad5
("drm/vc4: hdmi: Rework hdmi_enable_4kp60 detection code"), which went
into v6.2.

Thanks,
Tomasz

Re: [PATCH v2] drm/vc4: release firmware on error paths in vc4_hvs_bind()

[Dan Carpenter]

On Wed, Nov 11, 2026 at 02:15:28PM +0100, Tomasz Unger wrote:
> So without the patch every failed bind leaks one reference, and with the
> patch the reference is dropped.
> 
> Tested-by: Tomasz Unger <tomasz.unger@yahoo.pl>
> 
> One note about the Fixes tag: 2fa4ef5fb943 only moved this code around.
> As far as I can tell, the rpi_firmware_get() before devm_clk_get()
> without a put on the error path was introduced by commit 2a001ca00ad5
> ("drm/vc4: hdmi: Rework hdmi_enable_4kp60 detection code"), which went
> into v6.2.

Huh.  Wow.  Thanks.  I'll resend the patch with the correct Fixes tag.

regards,
dan carpenter

Re: [PATCH v2] drm/vc4: release firmware on error paths in vc4_hvs_bind()

[Maíra Canal]

Hi Dan,

On 11/11/26 10:45, Dan Carpenter wrote:
> On Wed, Nov 11, 2026 at 02:15:28PM +0100, Tomasz Unger wrote:
>> So without the patch every failed bind leaks one reference, and with the
>> patch the reference is dropped.
>>
>> Tested-by: Tomasz Unger <tomasz.unger@yahoo.pl>
>>
>> One note about the Fixes tag: 2fa4ef5fb943 only moved this code around.
>> As far as I can tell, the rpi_firmware_get() before devm_clk_get()
>> without a put on the error path was introduced by commit 2a001ca00ad5
>> ("drm/vc4: hdmi: Rework hdmi_enable_4kp60 detection code"), which went
>> into v6.2.
> 
> Huh.  Wow.  Thanks.  I'll resend the patch with the correct Fixes tag.

A patch addressing this issue is already queued for this week's drm PR
[1]. Sorry for letting your patch slip through the cracks.

[1] 
https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/af200e91c7b996177aa90981b2f0e794b085d43a

Best regards,
- Maíra

> 
> regards,
> dan carpenter
>