Re: [PATCH RFC v2 03/14] drm/msm: look for lpac from dts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/msm: adreno_gpu_init() links parent as consumer of child lpac_pdev
- [High] drm/msm: double put_device() on lpac_pdev when adreno_gpu_init() fails
- [High] drm/msm: adreno_remove() depopulates lpac_pdev before unbinding GPU
- [Medium] drm/msm: adreno_probe() leaks populated child devices on probe failure
--

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

drm/msm: look for lpac from dts

This commit populates child platform devices from the Adreno GPU device
tree node and looks up the "qcom,lpac" child device during GPU
initialization so its SMMU context bank and IOMMU domain can be wired up.

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index 7f20320ef66af..4d22223c3323c 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
[ ... ]
> @@ -271,6 +273,12 @@ static const struct component_ops a3xx_ops = {
>  
>  static int adreno_probe(struct platform_device *pdev)
>  {
> +	int ret = 0;
> +
> +	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);

[Severity: Medium]
If msm_gpu_probe() or component_add() fails in adreno_probe() after
of_platform_populate() succeeds, does this leak the populated child
platform_device structures (such as gpu->lpac_pdev) because
of_platform_depopulate(&pdev->dev) is not called on the error path (or via
devm_of_platform_populate())?

> @@ -282,6 +290,8 @@ 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

[Severity: High]
In adreno_remove(), shouldn't of_platform_depopulate(&pdev->dev) happen
after component_del() / msm_gpu_remove() rather than before?

Calling of_platform_depopulate(&pdev->dev) first unregisters
gpu->lpac_pdev via platform_device_unregister() -> device_del() while the
GPU is still bound and iommu->lpac_domain is still attached to
&gpu->lpac_pdev->dev.

During device_del(&gpu->lpac_pdev->dev), the bus notifier runs:

  iommu_release_device()
    -> __iommu_group_remove_device()
      -> __iommu_group_free_device()

which triggers WARN_ON(group->owner_cnt || group->domain !=
group->default_domain) and tears down the LPAC SMMU master state before
component_del() or msm_gpu_remove() runs adreno_unbind() ->
adreno_gpu_cleanup() -> msm_gpu_cleanup() to detach the IOMMU domain and
disable runtime PM on &gpu->lpac_pdev->dev.

> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index f3fc7032fadcd..1dedeb19946ec 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
[ ... ]
> @@ -1200,6 +1205,18 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  	gpu->allow_relocs = config->info->family < ADRENO_6XX_GEN1;
>  	gpu->pdev = pdev;
>  
> +	lpac_node = of_get_compatible_child(pdev->dev.of_node, "qcom,lpac");
> +	if (lpac_node)
> +		gpu->lpac_pdev = of_find_device_by_node(lpac_node);
> +	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");
> +	}
> +	if (lpac_node)
> +		of_node_put(lpac_node);

[Severity: High]
Will device_link_add(&pdev->dev, &gpu->lpac_pdev->dev, ...) always fail and
return NULL here?

In adreno_probe(), of_platform_populate() creates gpu->lpac_pdev with
&pdev->dev as its parent. In adreno_gpu_init(), device_link_add() is called
with &pdev->dev (the parent) as the consumer and &gpu->lpac_pdev->dev (its
child) as the supplier.

Inside drivers/base/core.c:device_link_add():

	if (!device_pm_initialized(supplier)
	    || (!(flags & DL_FLAG_SYNC_STATE_ONLY) &&
		  device_is_dependent(consumer, supplier))) {
		link = NULL;
		goto out;
	}

Because consumer (&pdev->dev) is the parent of supplier
(&gpu->lpac_pdev->dev), device_is_dependent(consumer, supplier) returns 1
via device_is_ancestor(consumer, supplier), rejecting the link as a
circular dependency. In addition, gpu->lpac_pdev has no driver bound to it,
so passing DL_FLAG_AUTOREMOVE_CONSUMER without DL_FLAG_STATELESS is not
valid for a driverless device.

As a result, the device link is never created and &gpu->lpac_pdev->dev
remains in RPM_SUSPENDED with usage_count == 0, causing
msm_iommu_tlb_flush_all() and msm_iommu_tlb_flush_walk() in
drivers/gpu/drm/msm/msm_iommu.c:

	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_all((void *)adreno_smmu->cookie);

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

to skip all IOTLB invalidations on the LPAC SMMU context bank when buffers
are unmapped.

[ ... ]
> @@ -1241,6 +1260,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  
>  	return msm_gpu_init(drm, pdev, &adreno_gpu->base, &funcs->base,
>  			gpu_name, &adreno_gpu_config);
> +err_put_lpac_pdev:
> +	if (gpu->lpac_pdev)
> +		put_device(&gpu->lpac_pdev->dev);
> +	return ret;
>  }
>  
>  void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu)
> @@ -1256,4 +1279,9 @@ void adreno_gpu_cleanup(struct adreno_gpu *adreno_gpu)
>  		pm_runtime_disable(&priv->gpu_pdev->dev);
>  
>  	msm_gpu_cleanup(&adreno_gpu->base);
> +
> +	if (gpu->lpac_pdev) {
> +		pm_runtime_disable(&gpu->lpac_pdev->dev);
> +		put_device(&gpu->lpac_pdev->dev);
> +	}
>  }

[Severity: High]
When adreno_gpu_init() fails via err_put_lpac_pdev (when devm_kasprintf()
or adreno_get_pwrlevels() fails), doesn't this cause a double
put_device(&gpu->lpac_pdev->dev)?

The err_put_lpac_pdev path drops the reference on &gpu->lpac_pdev->dev
without clearing gpu->lpac_pdev (or disabling runtime PM), and the callers
of adreno_gpu_init() (such as a6xx_gpu_init()) then call their destroy
function on failure:

drivers/gpu/drm/msm/adreno/a6xx_gpu.c:a6xx_gpu_init() {
	...
	ret = adreno_gpu_init(dev, pdev, adreno_gpu, info->funcs, nr_rings);
	if (ret) {
		a6xx_destroy(&(a6xx_gpu->base.base));
		return ERR_PTR(ret);
	}
	...
}

which calls a6xx_destroy() -> adreno_gpu_cleanup(), seeing gpu->lpac_pdev
still non-NULL and calling put_device(&gpu->lpac_pdev->dev) a second time.

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