Re: [PATCH 1/3] drm/amdkfd: Fix error path at svm_migrate_copy_to_ram

[Felix Kuehling <felix.kuehling@amd.com>]

On 2026-08-17 09:59, Xiaogang.Chen wrote:
> From: Xiaogang Chen <xiaogang.chen@amd.com>
>
> If page migration from device to sys ram fails for some reasons driver needs
> release and unlock allocated system pages. To do that driver should use page
> physical address, or pfn, then get struct page*. Current driver uses dma
> address(for adev) that is not correct with IOMMU enabled, or even in general.
>
> The patch releases and unlocks allocated system pages based on where migration
> failed by struct page* of sys ram pages. Also dma_unmap correspodent system
> ram pages at error path.
>
> Signed-off-by: Xiaogang Chen <xiaogang.chen@amd.com>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 46 ++++++++++++++++--------
>   1 file changed, 31 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
> index 226e76ae0be7..aea82da5f575 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_migrate.c
> @@ -258,15 +258,6 @@ svm_migrate_get_sys_page(struct vm_area_struct *vma, unsigned long addr)
>   	return page;
>   }
>   
> -static void svm_migrate_put_sys_page(unsigned long addr)
> -{
> -	struct page *page;
> -
> -	page = pfn_to_page(addr >> PAGE_SHIFT);
> -	unlock_page(page);
> -	put_page(page);
> -}
> -
>   static unsigned long svm_migrate_successful_pages(struct migrate_vma *migrate)
>   {
>   	unsigned long mpages = 0;
> @@ -591,9 +582,10 @@ svm_migrate_copy_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
>   			dma_addr_t *scratch, u64 npages)
>   {
>   	struct device *dev = adev->dev;
> -	u64 *src;
> +	struct page *dpage = NULL;
>   	dma_addr_t *dst;
> -	struct page *dpage;
> +	u64 *src;
> +
>   	u64 i = 0, j;
>   	u64 addr;
>   	int r = 0;
> @@ -647,6 +639,7 @@ svm_migrate_copy_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
>   		r = dma_mapping_error(dev, dst[i]);
>   		if (r) {
>   			dev_err(adev->dev, "%s: fail %d dma_map_page\n", __func__, r);
> +			dst[i] = 0;
>   			goto out_oom;
>   		}
>   
> @@ -654,17 +647,40 @@ svm_migrate_copy_to_ram(struct amdgpu_device *adev, struct svm_range *prange,
>   				     dst[i] >> PAGE_SHIFT, page_to_pfn(dpage));
>   
>   		migrate->dst[i] = migrate_pfn(page_to_pfn(dpage));
> +
> +		dpage = NULL;
>   		j++;
>   	}
>   
> -	r = svm_migrate_copy_memory_gart(adev, dst + i - j, src + i - j, j,
> -					 FROM_VRAM_TO_RAM, mfence);
> -
> +	if (j > 0)
> +		r = svm_migrate_copy_memory_gart(adev, dst + i - j, src + i - j, j,
> +						 FROM_VRAM_TO_RAM, mfence);
>   out_oom:
>   	if (r) {
>   		pr_debug("failed %d copy to ram\n", r);
> +
> +		/* first release current dpage when dma_map_page fail */
> +		if (dpage) {
> +			unlock_page(dpage);
> +			put_page(dpage);
> +			dpage = NULL;

Minor nit-pick: You don't really need to set dpage to NULL here. The 
loop below doesn't either.


> +		}
> +
> +		/* release previous allocated sys pages and unmap dma address */
>   		while (i--) {
> -			svm_migrate_put_sys_page(dst[i]);
> +
> +			if (dst[i] && !dma_mapping_error(dev, dst[i])) {

Why do you need to check dma_mapping_error here again? If there was an 
error above, you already set dst[i] = 0.

With those two issues fixed, the patch is

Reviewed-by: Felix Kuehling <felix.kuehling@amd.com>


> +				dma_unmap_page(dev, dst[i], PAGE_SIZE,
> +					       DMA_BIDIRECTIONAL);
> +				dst[i] = 0;
> +			}
> +
> +			dpage = migrate_pfn_to_page(migrate->dst[i]);
> +			if (!dpage)
> +				continue;
> +
> +			unlock_page(dpage);
> +			put_page(dpage);
>   			migrate->dst[i] = 0;
>   		}
>   	}