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 <[email protected]> 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-01d50c3e0...@gmail.com?part=4
