Re: [PATCH V1] accel/amdxdna: wait uninterruptibly for jobs in the HMM invalidate callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: circular locking deadlock between MMU notifier and IOMMU page fault
--

commit b2f7ee3cb752f6d69d4b4ef2181868d9befe8e3c
Author: Matthew Brost <matthew.brost@intel.com>

accel/amdxdna: wait uninterruptibly for jobs in the HMM invalidate callback

This commit changes the BO wait in aie2_hmm_invalidate() to be
uninterruptible and not time-limited, since MMU notifiers cannot fail a
blockable invalidation.

> diff --git a/drivers/accel/amdxdna/aie2_ctx.c b/drivers/accel/amdxdna/aie2_ctx.c
> index d927c8c9d557a..9d38703501963 100644
> --- a/drivers/accel/amdxdna/aie2_ctx.c
> +++ b/drivers/accel/amdxdna/aie2_ctx.c
> @@ -1280,16 +1280,12 @@ int aie2_cmd_submit(struct amdxdna_hwctx *hwctx, struct amdxdna_sched_job *job,
>  void aie2_hmm_invalidate(struct amdxdna_gem_obj *abo,
>  			 unsigned long cur_seq)
>  {
> -	struct amdxdna_dev *xdna = to_xdna_dev(to_gobj(abo)->dev);
> -	struct drm_gem_object *gobj = to_gobj(abo);
> -	long ret;
> -
> -	ret = dma_resv_wait_timeout(gobj->resv, DMA_RESV_USAGE_BOOKKEEP,
> -				    true, MAX_SCHEDULE_TIMEOUT);
> -	if (!ret)
> -		XDNA_ERR(xdna, "Failed to wait for bo, ret %ld", ret);
> -	else if (ret == -ERESTARTSYS)
> -		XDNA_DBG(xdna, "Wait for bo interrupted by signal");
> +	/*
> +	 * The pages must not be released while the device may still access
> +	 * them, so this wait must be neither interruptible nor time limited.
> +	 */
> +	dma_resv_wait_timeout(to_gobj(abo)->resv, DMA_RESV_USAGE_BOOKKEEP,
> +			      false, MAX_SCHEDULE_TIMEOUT);

[Severity: High]
Does this introduce a circular locking deadlock with the IOMMU page fault
handler?

Since aie2_hmm_invalidate() is called from MMU interval notifiers (for
example, during munmap), it executes with the mmap_lock held for writing. By
performing an uninterruptible wait here for the BO's fences to signal, we
block waiting on the GPU job.

If that GPU job triggers an IOMMU page fault (IOPF), iommu_sva_handle_iopf()
will attempt to acquire mmap_read_lock() to handle the fault:

drivers/iommu/iommu-sva.c:iommu_sva_handle_iopf() {
    ...
    mmap_read_lock(mm);
    ...
    ret = handle_mm_fault(vma, prm->addr, fault_flags, NULL);
    ...
}

This creates a circular dependency:
1. The MMU notifier holds mmap_lock and waits for the GPU job.
2. The GPU job stalls waiting for the IOPF handler.
3. The IOPF handler waits for the mmap_lock.

Additionally, if the Timeout Detection and Recovery (TDR) is disabled
(which is supported via the tdr_timeout_ms module parameter), this deadlock
is permanent and freezes the process. Even with TDR enabled, resolving a
page fault by timing out and resetting the hardware is a functional regression
and a severe performance penalty.

>  }
>  
>  int aie2_hwctx_heap_expand(struct amdxdna_hwctx *hwctx,

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015183248.3414119-1-lizhi.hou@amd.com?part=1