Re: [PATCH v2 4/6] media: arm: mali-c55: Use devm_of_reserved_mem_device_init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Race condition between runtime PM autosuspend and manual power-off during device removal and probe error paths.
- [High] The IRQ handler is never freed if the device is removed while in the active runtime PM state.
- [High] Media nodes are registered and exposed to userspace before the device's IRQ is requested, breaking initial video capture.
--

commit c75be1887c52469e855778c08a0a6679f1af97b0
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

media: arm: mali-c55: Use devm_of_reserved_mem_device_init()

This commit refactors the driver to use the devres-managed
devm_of_reserved_mem_device_init() instead of manual initialization and
cleanup.

> 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 f28e9f4354aca..3d37cd4465838 100644
> --- a/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> +++ b/drivers/media/platform/arm/mali-c55/mali-c55-core.c
> @@ -803,14 +803,12 @@ static int mali_c55_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to acquire resets\n");
>  
> -	of_reserved_mem_device_init(dev);
> +	devm_of_reserved_mem_device_init(dev);
>  	vb2_dma_contig_set_max_seg_size(dev, UINT_MAX);
>  
>  	ret = __mali_c55_power_on(mali_c55);

[Severity: High]
This is a pre-existing issue, but are the media nodes registered and exposed
to userspace before the device's IRQ is requested?

Later in mali_c55_probe():

	ret = mali_c55_media_frameworks_init(mali_c55);
	if (ret)
		goto err_pm_runtime_disable;

	pm_runtime_idle(&pdev->dev);

	mali_c55->irqnum = platform_get_irq(pdev, 0);

The device is explicitly set to RPM_ACTIVE during probe, but the IRQ handler
is only requested inside mali_c55_runtime_resume().

If userspace opens the device and begins streaming before the autosuspend
delay forces a suspend cycle, the device will process frames without an IRQ
handler. Will this cause video streaming to hang indefinitely because the
capture buffers are never completed?

> -	if (ret) {
> -		dev_err_probe(dev, ret, "failed to power on\n");
> -		goto err_release_mem;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to power on\n");
>  
>  	ret = mali_c55_check_hwcfg(mali_c55);
>  	if (ret)

[ ... ]

> @@ -849,9 +847,6 @@ static int mali_c55_probe(struct platform_device *pdev)
>  	kfree(mali_c55->context.registers);
>  err_power_off:
>  	__mali_c55_power_off(mali_c55);

[Severity: High]
This is a pre-existing issue, but is there a risk of a double power-off
here if the device successfully auto-suspended before pm_runtime_disable()
is called?

If the autosuspend timer triggered, calling __mali_c55_power_off() again
could lead to imbalanced clock disables (calling clk_bulk_disable_unprepare()
without a matching enable) and reset assertions, potentially causing kernel
warnings in the clock and reset cores.

> -err_release_mem:
> -	of_reserved_mem_device_release(dev);
> -
>  	return ret;
>  }
>  
> @@ -866,7 +861,6 @@ static void mali_c55_remove(struct platform_device *pdev)
>  	}

[Severity: High]
This is a pre-existing issue, but does this manual power-off in
mali_c55_remove() create a race window with concurrent autosuspend?

In mali_c55_remove(), if the device is not suspended:

	if (!pm_runtime_suspended(&pdev->dev)) {
		__mali_c55_power_off(mali_c55);
		pm_runtime_set_suspended(&pdev->dev);
	}

Because pm_runtime_disable() is called after this check, an asynchronous
autosuspend could execute concurrently. Could this result in both threads
calling __mali_c55_power_off() at the same time?

[Severity: High]
This is a pre-existing issue, but is it possible that bypassing the runtime
suspend callback here causes an IRQ resource leak?

The driver dynamically registers its IRQ handler in mali_c55_runtime_resume()
and frees it in mali_c55_runtime_suspend(). If the device is active during
removal, open-coding the power-off with __mali_c55_power_off() bypasses the
free_irq() call.

Could a subsequent interrupt on this line (or shared IRQ) trigger the handler
and cause a use-after-free when accessing the freed device context?

>  	pm_runtime_disable(&pdev->dev);
>  	kfree(mali_c55->context.registers);
> -	of_reserved_mem_device_release(&pdev->dev);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902200703.2016410-1-mukesh.ojha@oss.qualcomm.com?part=4