Re: [PATCH] media: mali-c55: fix resource leaks in probe and remove

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hello David

   first of all, there are two version of this patch, one sent in
response the the other. Which one should I look at ?

If you intended to send a v2, mark it as v2 (depending on the tooling
you use either let b4 handle it or use -vX switch to
git-format-patches) and send it out -not- in reply to the first
version.

On Thu, Mar 26, 2026 at 08:33:39PM +0000, David Carlier wrote:
> mali_c55_probe() calls of_reserved_mem_device_init() to associate
> reserved memory regions with the device. This function allocates a
> struct rmem_assigned_device and adds it to a global linked list, which
> must be explicitly released via of_reserved_mem_device_release() — there
> is no devm variant of this API.
>
> However, neither the probe error paths nor mali_c55_remove() called
> of_reserved_mem_device_release(). Any probe failure after the
> of_reserved_mem_device_init() call, as well as every normal device
> removal, leaked the reserved memory association on the global list.

Thanks! The of_reserved_mem_device_ APIs documentation is -horrible-.

It tells you what the functions do but not how the API should be
used. I can only resort of the fact that a _release() should
logically follow an _init()...

>
> Additionally, pm_runtime_enable() called during probe was never undone
> in mali_c55_remove(), leaving the device's runtime PM state enabled

Ok, but this is a separate patch

> after the driver is unbound. The probe error path had a related issue:
> when mali_c55_media_frameworks_init() failed, the goto target jumped
> directly to err_free_context_registers, skipping pm_runtime_disable()
> despite pm_runtime having already been enabled earlier in the function.

Ups, yes.

A separate patch as well please.

>
> Fix these issues by:
>  - Adding an err_release_mem label at the end of the error chain so all
>    post-init failure paths release the reserved memory association.
>  - Splitting pm_runtime_disable() into its own err_runtime_disable label
>    so the media frameworks init failure correctly unwinds it.
>  - Adding of_reserved_mem_device_release() and pm_runtime_disable() to
>    mali_c55_remove(), with the teardown order mirroring probe in
>    reverse.
>
> Fixes: d5f281f3dd29 ("media: mali-c55: Add Mali-C55 ISP driver")
> Signed-off-by: David Carlier <devnexen@gmail.com>
> ---
>  .../media/platform/arm/mali-c55/mali-c55-core.c  | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/platform/arm/mali-c55/mali-c55-core.c b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> index c1a562cd214e..de895b69d786 100644
> --- a/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> @@ -806,8 +806,10 @@ static int mali_c55_probe(struct platform_device *pdev)
>  	vb2_dma_contig_set_max_seg_size(dev, UINT_MAX);
>
>  	ret = __mali_c55_power_on(mali_c55);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "failed to power on\n");
> +	if (ret) {
> +		dev_err_probe(dev, ret, "failed to power on\n");
> +		goto err_release_mem;
> +	}
>
>  	ret = mali_c55_check_hwcfg(mali_c55);
>  	if (ret)
> @@ -826,7 +828,7 @@ static int mali_c55_probe(struct platform_device *pdev)
>
>  	ret = mali_c55_media_frameworks_init(mali_c55);
>  	if (ret)
> -		goto err_free_context_registers;
> +		goto err_runtime_disable;
>
>  	pm_runtime_idle(&pdev->dev);
>
> @@ -841,11 +843,13 @@ static int mali_c55_probe(struct platform_device *pdev)
>
>  err_deinit_media_frameworks:
>  	mali_c55_media_frameworks_deinit(mali_c55);
> +err_runtime_disable:

maybe err_pm_runtime_disable

>  	pm_runtime_disable(&pdev->dev);
> -err_free_context_registers:
>  	kfree(mali_c55->context.registers);
>  err_power_off:
>  	__mali_c55_power_off(mali_c55);
> +err_release_mem:
> +	of_reserved_mem_device_release(dev);
>
>  	return ret;
>  }
> @@ -854,8 +858,10 @@ static void mali_c55_remove(struct platform_device *pdev)
>  {
>  	struct mali_c55 *mali_c55 = platform_get_drvdata(pdev);
>
> -	kfree(mali_c55->context.registers);
>  	mali_c55_media_frameworks_deinit(mali_c55);
> +	pm_runtime_disable(&pdev->dev);
> +	kfree(mali_c55->context.registers);
> +	of_reserved_mem_device_release(&pdev->dev);

This patched prompted me to looking into module load/unload more
closely and indeed there are more things to fix on top.

If you split this patch in three I'll take them in and work on top of
them. Would this work ?

Thanks
  j

>  }
>
>  static const struct of_device_id mali_c55_of_match[] = {
> --
> 2.53.0
>
>