Re: [PATCH RFC 04/13] arm64: dts: qcom: sm8650: move smmu sid 1 to new lpac device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Device Tree Forward-Compatibility Breakage (GPU Firmware Crash on Older Kernels)
- [High] Resource Leak and Unbalanced PM Runtime in GPU Driver LPAC Initialization
- [Medium] Device Tree Anti-Pattern and Syntax Violation (Overlapping Reg / Mismatched Unit Address)
--

commit e35e1c1ccf71441e2995511a6a0a9c69917e0605
Author: Anna Maniscalco <anna.maniscalco2000@gmail.com>

arm64: dts: qcom: sm8650: move smmu sid 1 to new lpac device

Moves SID 1 from the GPU node to a new LPAC node to allow SID 1 to use a
different domain when LPAC is used.

> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index 1604bc8cff373..44e5f9d4b335f 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> @@ -4133,6 +4133,13 @@ tcsr: clock-controller@1fc0000 {
>  			#reset-cells = <1>;
>  		};
>  
> +		lpac: lpac@3d00000 {
> +			compatible = "qcom,lpac";
> +			reg = <0x0 0x03d00000 0x0 0x61000>;
> +
> +			iommus = <&adreno_smmu 1 0x0>;
> +		};
> +

[Severity: Medium]
Does this node violate the Devicetree specification by overlapping with its
sibling node?

The memory region (0x3d00000) for the lpac@3d00000 node completely overlaps
with the gpu@3d00000 node below.

Also, creating a top-level dummy platform device solely to bind a second
IOMMU SID to a separate struct device (to work around driver domain
isolation limitations) appears to be an architectural anti-pattern.

>  		gpu: gpu@3d00000 {
>  			compatible = "qcom,adreno-43051401", "qcom,adreno";
>  			reg = <0x0 0x03d00000 0x0 0x40000>,
[ ... ]
> @@ -4144,8 +4151,9 @@ gpu: gpu@3d00000 {
>  
>  			interrupts = <GIC_SPI 300 IRQ_TYPE_LEVEL_HIGH 0>;
>  
> -			iommus = <&adreno_smmu 0 0x0>,
> -				 <&adreno_smmu 1 0x0>;
> +			iommus = <&adreno_smmu 0 0x0>;
> +
> +			qcom,lpac= <&lpac>;

[Severity: High]
Does removing SID 1 from the gpu@3d00000 node break forward-compatibility
for older kernels?

If an older kernel (which lacks the LPAC DRM driver modifications to parse
qcom,lpac) boots with this updated DT, it will only attach SID 0 to the
GPU's SMMU domain, leaving SID 1 unmapped.

Since the commit message notes that firmware requires both SID 0 and 1
mapped to the same page table during initialization, attempting to boot an
older kernel with this DT could trigger an SMMU context fault and completely
break GPU initialization.


Regarding the DRM driver code that handles the new lpac node:

[Severity: High]
Does the driver initialization leak the LPAC device reference and leave PM
runtime unbalanced?

Looking at adreno_gpu_init() in drivers/gpu/drm/msm/adreno/adreno_gpu.c:

drivers/gpu/drm/msm/adreno/adreno_gpu.c:adreno_gpu_init() {
    ...
    gpu->lpac_pdev = of_find_device_by_node(lpac_node);
    if (gpu->lpac_pdev) {
        ...
        pm_runtime_enable(&gpu->lpac_pdev->dev);
    }
    ...
}

The driver acquires a reference using of_find_device_by_node() (which
increments the reference count) and calls pm_runtime_enable().

However, adreno_gpu_cleanup() handles the main gpu_pdev but does not seem
to call put_device() or pm_runtime_disable() to clean up gpu->lpac_pdev:

drivers/gpu/drm/msm/adreno/adreno_gpu.c:adreno_gpu_cleanup() {
    ...
    if (priv && pm_runtime_enabled(&priv->gpu_pdev->dev))
        pm_runtime_disable(&priv->gpu_pdev->dev);
    
    msm_gpu_cleanup(&adreno_gpu->base);
    ...
}

Could this result in a memory leak of the device structure and trigger an
"Unbalanced pm_runtime_enable!" warning when the GPU driver is unbound
or fails to probe?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-descriptive-name-lpac-upstream-v1-0-01d50c3e0c99@gmail.com?part=4