Re: [PATCH RFC v2 02/14] drm/msm: add support for lpac_domain in msm_mmu

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] drm/msm: wrong device passed to runtime PM in msm_iommu_tlb_flush_all()
- [High] drm/msm: missing TTBR0 rollback and NULL tlb deref in msm_iommu_pagetable_create()
- [High] drm/msm: missing iommu->domain unmap on error in msm_iommu_map()
- [Medium] drm/msm: race on fault_coredump_done between gpu and lpac fault handlers
--

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

drm/msm: add support for lpac_domain in msm_mmu

This commit adds support for a separate LPAC (Low Priority Async Compute)
iommu_domain in msm_mmu and keeps its mappings, TTBR0 configuration, fault
handling, and TLB invalidations synchronized with the main domain.

> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
> index 058c71c82cf54..0e770ec91a83a 100644
> --- a/drivers/gpu/drm/msm/msm_iommu.c
> +++ b/drivers/gpu/drm/msm/msm_iommu.c

[ ... ]

> @@ -450,14 +460,22 @@ static void msm_iommu_tlb_flush_all(void *cookie)
>  	struct msm_iommu_pagetable *pagetable = cookie;
>  	struct adreno_smmu_priv *adreno_smmu;
>  
> -	if (!pm_runtime_get_if_in_use(pagetable->iommu_dev))
> -		return;
> +	if (pm_runtime_get_if_in_use(pagetable->iommu_dev)) {
> +		adreno_smmu = dev_get_drvdata(pagetable->parent->dev);
> +
> +		pagetable->tlb->tlb_flush_all((void *)adreno_smmu->cookie);
> +
> +		pm_runtime_put_autosuspend(pagetable->iommu_dev);
> +	}
>  
> -	adreno_smmu = dev_get_drvdata(pagetable->parent->dev);
> +	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);
> +		pagetable->tlb->tlb_flush_all((void *)adreno_smmu->cookie);
>  
> -	pm_runtime_put_autosuspend(pagetable->iommu_dev);
> +		pm_runtime_put_autosuspend(pagetable->parent->lpac_dev);
> +	}
>  }
>  
>  static void msm_iommu_tlb_flush_walk(unsigned long iova, size_t size,
> @@ -466,14 +484,23 @@ static void msm_iommu_tlb_flush_walk(unsigned long iova, size_t size,
>  	struct msm_iommu_pagetable *pagetable = cookie;
>  	struct adreno_smmu_priv *adreno_smmu;
>  
> -	if (!pm_runtime_get_if_in_use(pagetable->iommu_dev))
> -		return;
>  
> -	adreno_smmu = dev_get_drvdata(pagetable->parent->dev);
> +	if (pm_runtime_get_if_in_use(pagetable->iommu_dev)) {
> +		adreno_smmu = dev_get_drvdata(pagetable->parent->dev);
> +
> +		pagetable->tlb->tlb_flush_walk(iova, size, granule, (void *)adreno_smmu->cookie);
>  
> -	pagetable->tlb->tlb_flush_walk(iova, size, granule, (void *)adreno_smmu->cookie);
> +		pm_runtime_put_autosuspend(pagetable->iommu_dev);
> +	}
> +
> +	if (pagetable->parent->lpac_dev &&
> +	    pm_runtime_get_if_in_use(pagetable->parent->lpac_dev)) {
> +		adreno_smmu = dev_get_drvdata(pagetable->parent->lpac_dev);
>  
> -	pm_runtime_put_autosuspend(pagetable->iommu_dev);
> +		pagetable->tlb->tlb_flush_walk(iova, size, granule, (void *)adreno_smmu->cookie);
> +
> +		pm_runtime_put_autosuspend(pagetable->parent->lpac_dev);
> +	}
>  }

[Severity: High]
Should msm_iommu_tlb_flush_all() and msm_iommu_tlb_flush_walk() keep the
runtime PM reference on pagetable->iommu_dev across both context bank flushes
instead of calling pm_runtime_get_if_in_use() and
pm_runtime_put_autosuspend() on pagetable->parent->lpac_dev?

In msm_iommu_pagetable_create(), pagetable->iommu_dev is initialized from
ttbr1_cfg->iommu_dev (the SMMU controller device), whereas
pagetable->parent->dev and pagetable->parent->lpac_dev are the GPU and LPAC
client devices. Both adreno_smmu->cookie and lpac_adreno_smmu->cookie access
context banks on that SMMU controller device.

When runtime PM is disabled on pagetable->parent->lpac_dev,
pm_runtime_get_if_in_use(pagetable->parent->lpac_dev) returns -EINVAL, which
evaluates as true in the if condition without incrementing usage_count. After
pm_runtime_put_autosuspend(pagetable->iommu_dev) drops the SMMU reference at
the end of the primary flush block, the LPAC block can access unclocked SMMU
registers while pagetable->iommu_dev is suspended and then underflow the
runtime PM usage count on pagetable->parent->lpac_dev. Conversely, if runtime
PM is enabled on lpac_dev with a zero usage count while pagetable->iommu_dev
is active, pm_runtime_get_if_in_use(pagetable->parent->lpac_dev) returns 0
and skips the LPAC TLB flush.

[ ... ]

> @@ -588,6 +621,17 @@ struct msm_mmu *msm_iommu_pagetable_create(struct msm_mmu *parent, bool kernel_m
>  			return ERR_PTR(ret);
>  		}
>  
> +		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);
> +			}
> +		}
> +
>  		BUG_ON(iommu->prr_page);
>  		if (adreno_smmu->set_prr_bit) {
>  			/*
> @@ -606,11 +650,16 @@ struct msm_mmu *msm_iommu_pagetable_create(struct msm_mmu *parent, bool kernel_m
>  						  page_to_phys(iommu->prr_page));
>  			adreno_smmu->set_prr_bit(adreno_smmu->cookie, true);
>  		}
> +
> +		if (iommu->prr_page && lpac_adreno_smmu && lpac_adreno_smmu->set_prr_bit) {
> +			lpac_adreno_smmu->set_prr_addr(lpac_adreno_smmu->cookie,
> +						       page_to_phys(iommu->prr_page));
> +			lpac_adreno_smmu->set_prr_bit(lpac_adreno_smmu->cookie, true);
> +		}
>  	}
>  	mutex_unlock(&iommu->init_lock);
>  
>  	/* Needed later for TLB flush */
> -	pagetable->parent = parent;
>  	pagetable->tlb = ttbr1_cfg->tlb;
>  	pagetable->pgsize_bitmap = ttbr0_cfg.pgsize_bitmap;
>  	pagetable->ttbr = ttbr0_cfg.arm_lpae_s1_cfg.ttbr;

[Severity: High]
What happens in msm_iommu_pagetable_create() if
lpac_adreno_smmu->set_ttbr0_cfg() fails after adreno_smmu->set_ttbr0_cfg()
has already succeeded?

Because the error path does not call
adreno_smmu->set_ttbr0_cfg(adreno_smmu->cookie, NULL) before freeing
pagetable->pgtbl_ops, the primary context bank's TTBR0 remains enabled and
pointed at the freed root page table physical address, and subsequent calls
to msm_iommu_pagetable_create() will fail in qcom_adreno_smmu_set_ttbr0_cfg()
because ARM_SMMU_TCR_EPD0 is already cleared.

In addition, while pagetable->parent = parent was moved before
alloc_io_pgtable_ops(), pagetable->tlb = ttbr1_cfg->tlb is still assigned
after the init_lock block:

drivers/gpu/drm/msm/msm_iommu.c:msm_iommu_pagetable_create() {
    ...
	if (iommu->pagetables++ == 0) {
		...
		if (lpac_adreno_smmu) {
			ret = lpac_adreno_smmu->set_ttbr0_cfg(...);
			if (ret) {
				...
				free_io_pgtable_ops(pagetable->pgtbl_ops);
				...
			}
		}
	}
	mutex_unlock(&iommu->init_lock);

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

When free_io_pgtable_ops() runs on either set_ttbr0_cfg() error path, it
calls io_pgtable_tlb_flush_all() -> msm_iommu_tlb_flush_all(), which
dereferences pagetable->tlb while it is still NULL. Should pagetable->tlb be
initialized before alloc_io_pgtable_ops(), and should the LPAC error path roll
back adreno_smmu->set_ttbr0_cfg(adreno_smmu->cookie, NULL)?

> @@ -646,6 +695,26 @@ static int msm_gpu_fault_handler(struct iommu_domain *domain, struct device *dev
>  	return 0;
>  }
>  
> +static int msm_lpac_fault_handler(struct iommu_domain *domain, struct device *dev,
> +		unsigned long iova, int flags, void *arg)
> +{
> +	struct msm_iommu *iommu = arg;
> +	struct adreno_smmu_priv *adreno_smmu = dev_get_drvdata(iommu->base.lpac_dev);
> +	struct adreno_smmu_fault_info info, *ptr = NULL;
> +
> +	if (adreno_smmu->get_fault_info) {
> +		adreno_smmu->get_fault_info(adreno_smmu->cookie, &info);
> +		ptr = &info;
> +	}
> +
> +	if (iommu->base.handler)
> +		return iommu->base.handler(iommu->base.arg, iova, flags, ptr);
> +
> +	pr_warn_ratelimited("*** lpac fault: iova=%16lx, flags=%d\n", iova, flags);
> +
> +	return 0;
> +}

[Severity: Medium]
Can msm_lpac_fault_handler() and msm_gpu_fault_handler() race on
adreno_gpu->fault_coredump_done when both context banks fault concurrently?

Because iommu->domain and iommu->lpac_domain use separate SMMU context banks
with separate threaded fault IRQs, both handlers can invoke
iommu->base.handler (a6xx_fault_handler() -> adreno_fault_handler()) at the
same time. In adreno_fault_handler(), do_devcoredump is evaluated before
gpu->lock is taken in msm_gpu_fault_crashstate_capture(), and
reinit_completion() and complete_all() on adreno_gpu->fault_coredump_done run
outside gpu->lock:

drivers/gpu/drm/msm/adreno/adreno_gpu.c:adreno_fault_handler() {
    ...
	do_devcoredump = info && (info->fsr & ARM_SMMU_FSR_SS) &&
		!gpu->crashstate && !READ_ONCE(priv->disable_devcoredump);
    ...
	if (do_devcoredump) {
		...
		reinit_completion(&adreno_gpu->fault_coredump_done);
		...
		msm_gpu_fault_crashstate_capture(gpu, &fault_info);

		complete_all(&adreno_gpu->fault_coredump_done);
	}
}

If both context banks stall on a fault while gpu->crashstate is still NULL,
can one handler call reinit_completion() or complete_all() while the other
context bank is still stalled in the SMMU, causing concurrent GMU waits on
adreno_gpu->fault_coredump_done to wake prematurely or race on the
completion?

[ ... ]

> @@ -689,6 +768,15 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova,
>  	if (ret < 0)
>  		return ret;
>  
> +	if (ret != len)
> +		return -EINVAL;
> +
> +	if (iommu->lpac_domain) {
> +		ret = iommu_map_sgtable(iommu->lpac_domain, iova, sgt, prot);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	return (ret == len) ? 0 : -EINVAL;
>  }

[Severity: High]
If mapping into iommu->lpac_domain fails or maps fewer than len bytes in
msm_iommu_map(), does this leave the [iova, iova + len) mapping leaked in
iommu->domain?

When msm_iommu_map() returns an error to msm_gem_vma_map(), msm_vma->mapped
remains false, so msm_gem_vma_unmap() will not be called to clean up the
primary domain mapping. Should msm_iommu_map() call
iommu_unmap(iommu->domain, iova, len) when the iommu->lpac_domain mapping
fails?

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