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 <[email protected]> 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-a21a5c850...@gmail.com?part=3
