Re: [PATCH RFC v2 04/14] 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 8 potential issue(s) to consider:
- [Critical] drm/msm: parent-child link failure in adreno_gpu_init() skips LPAC TLB flush
- [High] arm64: dts: qcom: sm8650: moving SID 1 to lpac breaks older kernels
- [High] drm/msm: double put_device() on lpac_pdev in adreno_gpu_init() error path
- [High] drm/msm: premature of_platform_depopulate() in adreno_remove()
- [High] drm/msm: missing TTBR0 rollback and NULL tlb in msm_iommu_pagetable_create()
- [High] drm/msm: leaked primary IOMMU mapping on lpac failure in msm_iommu_map()
- [Medium] arm64: dts: qcom: sm8650: undocumented lpac child node breaks dtbs_check
- [Medium] drm/msm: child platform device leak in adreno_probe() error path
--

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

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

This commit moves stream id 1 out of the main gpu node on sm8650 and places
it into a new lpac child node so that lpac can be attached to a separate
iommu domain.

> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index 1604bc8cff373..f47d1dbd06da4 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> @@ -4144,8 +4144,7 @@ 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>;

[Severity: High]
Will removing <&adreno_smmu 1 0x0> from gpu@3d00000 break device tree
backwards compatibility with existing kernels?

As noted in the commit description, gpu firmware accesses memory through
stream id 1 during initialization using the graphics page table even when
lpac is not used. On any kernel without support for the new qcom,lpac child
node, removing <&adreno_smmu 1 0x0> from gpu@3d00000 leaves stream id 1
unattached to the gpu iommu domain, causing translation faults during
firmware initialization.

Also, since lpac is an internal execution pipe of the gpu rather than a
separate hardware block with its own register space, should the second domain
attachment be handled in the driver via iommu apis rather than splitting
stream id 1 into a virtual child node in the device tree?

>  
>  			operating-points-v2 = <&gpu_opp_table>;
>  
> @@ -4158,6 +4157,12 @@ gpu: gpu@3d00000 {
>  
>  			status = "disabled";
>  
> +			lpac: lpac {
> +				compatible = "qcom,lpac";
> +
> +				iommus = <&adreno_smmu 1 0x0>;
> +			};

[Severity: Medium]
Does adding the lpac child node here cause dtbs_check schema validation to
fail on sm8650 device trees?

In Documentation/devicetree/bindings/display/msm/gpu.yaml, the binding for
qcom,adreno specifies:

required:
  - compatible
  - reg
  - interrupts

additionalProperties: false

Neither the lpac child node nor the qcom,lpac compatible string is documented
in Documentation/devicetree/bindings/display/msm/gpu.yaml, so schema
validation rejects the new node.

[Severity: Critical]
When this lpac child node is populated on sm8650, will device_link_add() in
adreno_gpu_init() always fail and cause msm_iommu_tlb_flush_all() and
msm_iommu_tlb_flush_walk() to skip flushing the lpac context bank?

In drivers/gpu/drm/msm/adreno/adreno_device.c, adreno_probe() populates child
nodes of gpu@3d00000 with &pdev->dev as their parent:

	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);

Then in drivers/gpu/drm/msm/adreno/adreno_gpu.c, adreno_gpu_init() attempts
to create a runtime pm link with the parent &pdev->dev as consumer and its
child &gpu->lpac_pdev->dev as supplier:

	if (gpu->lpac_pdev) {
		pm_runtime_enable(&gpu->lpac_pdev->dev);
		if (!device_link_add(&pdev->dev, &gpu->lpac_pdev->dev,
				     DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_CONSUMER))
			dev_err(&gpu->lpac_pdev->dev, "failed to link to lpac device\n");
	}

Inside drivers/base/core.c, device_link_add() -> device_is_dependent() checks
whether the consumer is an ancestor of the supplier:

	if (dev == target || device_is_ancestor(dev, target))
		return 1;

Because &pdev->dev is the parent of &gpu->lpac_pdev->dev, device_link_add()
rejects the link as a dependency cycle and returns NULL. In addition,
gpu->lpac_pdev has no driver bound to it, so a managed link would remain in
DL_STATE_DORMANT.

As a result, gpu->lpac_pdev->dev is never runtime-resumed. When per-context
page tables are flushed in drivers/gpu/drm/msm/msm_iommu.c via
msm_iommu_tlb_flush_all() and msm_iommu_tlb_flush_walk():

	if (pagetable->parent->lpac_dev &&
	    pm_runtime_get_if_in_use(pagetable->parent->lpac_dev)) {
		adreno_smmu = dev_get_drvdata(pagetable->parent->lpac_dev);

		pagetable->tlb->tlb_flush_walk(iova, size, granule, (void *)adreno_smmu->cookie);

		pm_runtime_put_autosuspend(pagetable->parent->lpac_dev);
	}

pm_runtime_get_if_in_use(pagetable->parent->lpac_dev) always returns 0, so
tlb invalidations on the lpac context bank are skipped when buffers are
unmapped, leaving stale tlb entries pointing to freed pages.

[Severity: High]
If adreno_gpu_init() fails after acquiring gpu->lpac_pdev on sm8650, can
gpu->lpac_pdev->dev undergo a double put_device()?

In drivers/gpu/drm/msm/adreno/adreno_gpu.c, if devm_kasprintf() or
adreno_get_pwrlevels() fails in adreno_gpu_init(), the error path drops the
reference on gpu->lpac_pdev->dev without disabling runtime pm or clearing
gpu->lpac_pdev:

err_put_lpac_pdev:
	if (gpu->lpac_pdev)
		put_device(&gpu->lpac_pdev->dev);
	return ret;

The caller in drivers/gpu/drm/msm/adreno/a6xx_gpu.c, a6xx_gpu_init(), then
invokes a6xx_destroy() -> adreno_gpu_cleanup():

	ret = adreno_gpu_init(dev, pdev, adreno_gpu, info->funcs, nr_rings);
	if (ret) {
		a6xx_destroy(&(a6xx_gpu->base.base));
		return ERR_PTR(ret);
	}

In adreno_gpu_cleanup(), gpu->lpac_pdev is still non-NULL, so it calls
pm_runtime_disable() and put_device() a second time on &gpu->lpac_pdev->dev:

	if (gpu->lpac_pdev) {
		pm_runtime_disable(&gpu->lpac_pdev->dev);
		put_device(&gpu->lpac_pdev->dev);
	}

[Severity: High]
In drivers/gpu/drm/msm/adreno/adreno_device.c, does calling
of_platform_depopulate() at the start of adreno_remove() unregister
lpac_pdev while iommu->lpac_domain is still attached to it?

In adreno_remove():

static void adreno_remove(struct platform_device *pdev)
{
	struct msm_drm_private *priv = platform_get_drvdata(pdev);

	of_platform_depopulate(&pdev->dev);

	if (priv->kms_init)
		component_del(&pdev->dev, &a3xx_ops);
	else
		msm_gpu_remove(pdev, &a3xx_ops);
}

of_platform_depopulate(&pdev->dev) unregisters the child lpac platform device
before component_del() or msm_gpu_remove() runs adreno_unbind() ->
a6xx_destroy() -> adreno_gpu_cleanup() -> msm_gpu_cleanup() ->
msm_iommu_detach().

Because iommu->lpac_domain is still attached when lpac_pdev is unregistered,
__iommu_group_free_device() in drivers/iommu/iommu.c hits:

	if (list_empty(&group->devices))
		WARN_ON(group->owner_cnt ||
			group->domain != group->default_domain);

and msm_iommu_detach() and adreno_gpu_cleanup() subsequently operate on an
already-unregistered device.

[Severity: Medium]
What happens to the populated lpac child platform device if msm_gpu_probe()
or component_add() fails in adreno_probe()?

In drivers/gpu/drm/msm/adreno/adreno_device.c:

	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
	if (ret)
		return ret;

	if (of_device_is_compatible(pdev->dev.of_node, "amd,imageon") ||
	    msm_gpu_no_components())
		return msm_gpu_probe(pdev, &a3xx_ops);

	return component_add(&pdev->dev, &a3xx_ops);

If msm_gpu_probe() or component_add() returns an error,
of_platform_depopulate(&pdev->dev) is not called, leaking the child platform
device created for lpac.

[Severity: High]
In drivers/gpu/drm/msm/msm_iommu.c, if lpac_adreno_smmu->set_ttbr0_cfg()
fails in msm_iommu_pagetable_create(), does the error path dereference a NULL
pagetable->tlb and leave ttbr0 enabled on the primary context bank with freed
page tables?

When creating the first pagetable in msm_iommu_pagetable_create(),
adreno_smmu->set_ttbr0_cfg(adreno_smmu->cookie, &ttbr0_cfg) enables ttbr0 on
 the primary context bank before lpac_adreno_smmu->set_ttbr0_cfg() is called:

		if (lpac_adreno_smmu) {
			ret = lpac_adreno_smmu->set_ttbr0_cfg(lpac_adreno_smmu->cookie, &ttbr0_cfg);
			if (ret) {
				iommu->pagetables--;
				mutex_unlock(&iommu->init_lock);
				free_io_pgtable_ops(pagetable->pgtbl_ops);
				kfree(pagetable);
				return ERR_PTR(ret);
			}
		}

At this point pagetable->tlb has not been assigned yet, as that happens later
in msm_iommu_pagetable_create():

	/* Needed later for TLB flush */
	pagetable->tlb = ttbr1_cfg->tlb;

Calling free_io_pgtable_ops(pagetable->pgtbl_ops) invokes
msm_iommu_tlb_flush_all(), which dereferences pagetable->tlb if
pagetable->iommu_dev is runtime-active. In addition, because
adreno_smmu->set_ttbr0_cfg(adreno_smmu->cookie, NULL) is not called to roll
back the primary context bank configuration, context bank 0 retains a ttbr0
register pointing to the freed page table and qcom_adreno_smmu_set_ttbr0_cfg()
in drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c will reject subsequent calls
with -EINVAL because ARM_SMMU_TCR_EPD0 is already cleared:

		/* Don't call this again if TTBR0 is already enabled */
		if (!(cb->tcr[0] & ARM_SMMU_TCR_EPD0))
			return -EINVAL;

[Severity: High]
Can msm_iommu_map() in drivers/gpu/drm/msm/msm_iommu.c leak the mapping in
iommu->domain if mapping into iommu->lpac_domain fails?

In msm_iommu_map():

	ret = iommu_map_sgtable(iommu->domain, iova, sgt, prot);
	if (ret < 0)
		return ret;

	if (iommu->lpac_domain) {
		ret = iommu_map_sgtable(iommu->lpac_domain, iova, sgt, prot);
		if (ret < 0)
			return ret;
	}

	return (ret == len) ? 0 : -EINVAL;

If the second iommu_map_sgtable() call on iommu->lpac_domain returns an error
or maps fewer than len bytes after the first iommu_map_sgtable() on
iommu->domain succeeded, msm_iommu_map() returns an error without calling
iommu_unmap(iommu->domain, iova, len). In drivers/gpu/drm/msm/msm_gem_vma.c,
msm_gem_vma_map() then leaves msm_vma->mapped set to false:

	if (ret)
		msm_vma->mapped = false;

	return ret;

so msm_gem_vma_unmap() will never clean up the mapping left behind in
iommu->domain.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-descriptive-name-lpac-upstream-v2-0-a21a5c850961@gmail.com?part=4