[PATCH 1/4] mm: introduce vma_set_file function v2

[PATCH 1/4] mm: introduce vma_set_file function v2

[Christian König]

Add the new vma_set_file() function to allow changing
vma->vm_file with the necessary refcount dance.

v2: add more users of this.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/dma-buf.c                  | 16 +++++-----------
 drivers/gpu/drm/etnaviv/etnaviv_gem.c      |  4 +---
 drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  3 +--
 drivers/gpu/drm/i915/gem/i915_gem_mman.c   |  4 ++--
 drivers/gpu/drm/msm/msm_gem.c              |  4 +---
 drivers/gpu/drm/omapdrm/omap_gem.c         |  3 +--
 drivers/gpu/drm/vgem/vgem_drv.c            |  3 +--
 drivers/staging/android/ashmem.c           |  5 ++---
 include/linux/mm.h                         |  2 ++
 mm/mmap.c                                  | 16 ++++++++++++++++
 10 files changed, 32 insertions(+), 28 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index a6ba4d598f0e..e4316aa7e0f4 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -1163,20 +1163,14 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
 		return -EINVAL;
 
 	/* readjust the vma */
-	get_file(dmabuf->file);
-	oldfile = vma->vm_file;
-	vma->vm_file = dmabuf->file;
+	oldfile = vma_set_file(vma, dmabuf->file);
 	vma->vm_pgoff = pgoff;
 
 	ret = dmabuf->ops->mmap(dmabuf, vma);
-	if (ret) {
-		/* restore old parameters on failure */
-		vma->vm_file = oldfile;
-		fput(dmabuf->file);
-	} else {
-		if (oldfile)
-			fput(oldfile);
-	}
+	/* restore old parameters on failure */
+	if (ret)
+		vma_set_file(vma, oldfile);
+
 	return ret;
 
 }
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
index 312e9d58d5a7..10ce267c0947 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
@@ -145,10 +145,8 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj,
 		 * address_space (so unmap_mapping_range does what we want,
 		 * in particular in the case of mmap'd dmabufs)
 		 */
-		fput(vma->vm_file);
-		get_file(etnaviv_obj->base.filp);
 		vma->vm_pgoff = 0;
-		vma->vm_file  = etnaviv_obj->base.filp;
+		vma_set_file(vma, etnaviv_obj->base.filp);
 
 		vma->vm_page_prot = vm_page_prot;
 	}
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
index fec0e1e3dc3e..8ce4c9e28b87 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
@@ -119,8 +119,7 @@ static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *
 	if (ret)
 		return ret;
 
-	fput(vma->vm_file);
-	vma->vm_file = get_file(obj->base.filp);
+	vma_set_file(vma, obj->base.filp);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 3d69e51f3e4d..c9d5f1a38af3 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 	 * requires avoiding extraneous references to their filp, hence why
 	 * we prefer to use an anonymous file for their mmaps.
 	 */
-	fput(vma->vm_file);
-	vma->vm_file = anon;
+	vma_set_file(vma, anon);
+	fput(anon);
 
 	switch (mmo->mmap_type) {
 	case I915_MMAP_TYPE_WC:
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index de915ff6f4b4..a71f42870d5e 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -223,10 +223,8 @@ int msm_gem_mmap_obj(struct drm_gem_object *obj,
 		 * address_space (so unmap_mapping_range does what we want,
 		 * in particular in the case of mmap'd dmabufs)
 		 */
-		fput(vma->vm_file);
-		get_file(obj->filp);
 		vma->vm_pgoff = 0;
-		vma->vm_file  = obj->filp;
+		vma_set_file(vma, obj->filp);
 
 		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
 	}
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
index 979d53a93c2b..0d4542ff1d7d 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -564,9 +564,8 @@ int omap_gem_mmap_obj(struct drm_gem_object *obj,
 		 * address_space (so unmap_mapping_range does what we want,
 		 * in particular in the case of mmap'd dmabufs)
 		 */
-		fput(vma->vm_file);
 		vma->vm_pgoff = 0;
-		vma->vm_file  = get_file(obj->filp);
+		vma_set_file(vma, obj->filp);
 
 		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
 	}
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index fa54a6d1403d..ea0eecae5153 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -397,8 +397,7 @@ static int vgem_prime_mmap(struct drm_gem_object *obj,
 	if (ret)
 		return ret;
 
-	fput(vma->vm_file);
-	vma->vm_file = get_file(obj->filp);
+	vma_set_file(vma, obj->filp);
 	vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
 	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
 
diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
index 10b4be1f3e78..a51dc089896e 100644
--- a/drivers/staging/android/ashmem.c
+++ b/drivers/staging/android/ashmem.c
@@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
 		vma_set_anonymous(vma);
 	}
 
-	if (vma->vm_file)
-		fput(vma->vm_file);
-	vma->vm_file = asma->file;
+	vma_set_file(vma, asma->file);
+	fput(asma->file);
 
 out:
 	mutex_unlock(&ashmem_mutex);
diff --git a/include/linux/mm.h b/include/linux/mm.h
index ca6e6a81576b..a558602afe1b 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2693,6 +2693,8 @@ static inline void vma_set_page_prot(struct vm_area_struct *vma)
 }
 #endif
 
+struct file *vma_set_file(struct vm_area_struct *vma, struct file *file);
+
 #ifdef CONFIG_NUMA_BALANCING
 unsigned long change_prot_numa(struct vm_area_struct *vma,
 			unsigned long start, unsigned long end);
diff --git a/mm/mmap.c b/mm/mmap.c
index 40248d84ad5f..d3c3c510f643 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -136,6 +136,22 @@ void vma_set_page_prot(struct vm_area_struct *vma)
 	WRITE_ONCE(vma->vm_page_prot, vm_page_prot);
 }
 
+/*
+ * Change backing file, only valid to use during initial VMA setup.
+ */
+struct file *vma_set_file(struct vm_area_struct *vma, struct file *file)
+{
+	if (file)
+	        get_file(file);
+
+	swap(vma->vm_file, file);
+
+	if (file)
+		fput(file);
+
+	return file;
+}
+
 /*
  * Requires inode->i_mapping->i_mmap_rwsem
  */
-- 
2.17.1

[PATCH 2/4] drm/prime: document that use the page array is deprecated

[Christian König]

We have reoccurring requests on this so better document that
this approach doesn't work and dma_buf_mmap() needs to be used instead.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/drm_prime.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 4910c446db83..16fa2bfc271e 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -956,7 +956,7 @@ EXPORT_SYMBOL(drm_gem_prime_import);
 /**
  * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
  * @sgt: scatter-gather table to convert
- * @pages: optional array of page pointers to store the page array in
+ * @pages: deprecated array of page pointers to store the page array in
  * @addrs: optional array to store the dma bus address of each page
  * @max_entries: size of both the passed-in arrays
  *
@@ -965,6 +965,11 @@ EXPORT_SYMBOL(drm_gem_prime_import);
  *
  * Drivers can use this in their &drm_driver.gem_prime_import_sg_table
  * implementation.
+ *
+ * Specifying the pages array is deprecated and strongly discouraged for new
+ * drivers. The pages array is only useful for page faults and those can
+ * corrupt fields in the struct page if they are not handled by the exporting
+ * driver.
  */
 int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
 				     dma_addr_t *addrs, int max_entries)
-- 
2.17.1

[PATCH 3/4] drm/radeon: stop using pages with drm_prime_sg_to_page_addr_arrays

[Christian König]

This is deprecated.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/radeon/radeon_ttm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c
index 63e38b05a5bc..4b92cdbcd29b 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -474,8 +474,8 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_bo_device *bdev, struct ttm_tt *
 	if (r)
 		goto release_sg;
 
-	drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages,
-					 gtt->ttm.dma_address, ttm->num_pages);
+	drm_prime_sg_to_page_addr_arrays(ttm->sg, NULL, gtt->ttm.dma_address,
+					 ttm->num_pages);
 
 	return 0;
 
@@ -642,8 +642,9 @@ static int radeon_ttm_tt_populate(struct ttm_bo_device *bdev,
 	}
 
 	if (slave && ttm->sg) {
-		drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages,
-						 gtt->ttm.dma_address, ttm->num_pages);
+		drm_prime_sg_to_page_addr_arrays(ttm->sg, NULL,
+						 gtt->ttm.dma_address,
+						 ttm->num_pages);
 		ttm_tt_set_populated(ttm);
 		return 0;
 	}
-- 
2.17.1

[PATCH 4/4] drm/amdgpu: stop using pages with drm_prime_sg_to_page_addr_arrays

[Christian König]

This is deprecated.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 399961035ae6..ac463e706b19 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -1011,8 +1011,8 @@ static int amdgpu_ttm_tt_pin_userptr(struct ttm_bo_device *bdev,
 		goto release_sg;
 
 	/* convert SG to linear array of pages and dma addresses */
-	drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages,
-					 gtt->ttm.dma_address, ttm->num_pages);
+	drm_prime_sg_to_page_addr_arrays(ttm->sg, NULL, gtt->ttm.dma_address,
+					 ttm->num_pages);
 
 	return 0;
 
@@ -1345,7 +1345,7 @@ static int amdgpu_ttm_tt_populate(struct ttm_bo_device *bdev,
 			ttm->sg = sgt;
 		}
 
-		drm_prime_sg_to_page_addr_arrays(ttm->sg, ttm->pages,
+		drm_prime_sg_to_page_addr_arrays(ttm->sg, NULL,
 						 gtt->ttm.dma_address,
 						 ttm->num_pages);
 		ttm_tt_set_populated(ttm);
-- 
2.17.1

Re: [PATCH 1/4] mm: introduce vma_set_file function v2

[Matthew Wilcox]

On Thu, Oct 08, 2020 at 01:23:39PM +0200, Christian König wrote:
>  drivers/dma-buf/dma-buf.c                  | 16 +++++-----------
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c      |  4 +---
>  drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  3 +--
>  drivers/gpu/drm/i915/gem/i915_gem_mman.c   |  4 ++--
>  drivers/gpu/drm/msm/msm_gem.c              |  4 +---
>  drivers/gpu/drm/omapdrm/omap_gem.c         |  3 +--
>  drivers/gpu/drm/vgem/vgem_drv.c            |  3 +--
>  drivers/staging/android/ashmem.c           |  5 ++---
...
> +++ b/mm/mmap.c
> @@ -136,6 +136,22 @@ void vma_set_page_prot(struct vm_area_struct *vma)
>  	WRITE_ONCE(vma->vm_page_prot, vm_page_prot);
>  }
>  
> +/*
> + * Change backing file, only valid to use during initial VMA setup.
> + */
> +struct file *vma_set_file(struct vm_area_struct *vma, struct file *file)
> +{
> +	if (file)
> +	        get_file(file);
> +
> +	swap(vma->vm_file, file);
> +
> +	if (file)
> +		fput(file);
> +
> +	return file;
> +}
> +

These users are all potentially modules.  You need an EXPORT_SYMBOL()?

Re: [PATCH 1/4] mm: introduce vma_set_file function v2

[Christian König]

Am 08.10.20 um 13:39 schrieb Matthew Wilcox:
> On Thu, Oct 08, 2020 at 01:23:39PM +0200, Christian König wrote:
>>   drivers/dma-buf/dma-buf.c                  | 16 +++++-----------
>>   drivers/gpu/drm/etnaviv/etnaviv_gem.c      |  4 +---
>>   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  3 +--
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c   |  4 ++--
>>   drivers/gpu/drm/msm/msm_gem.c              |  4 +---
>>   drivers/gpu/drm/omapdrm/omap_gem.c         |  3 +--
>>   drivers/gpu/drm/vgem/vgem_drv.c            |  3 +--
>>   drivers/staging/android/ashmem.c           |  5 ++---
> ...
>> +++ b/mm/mmap.c
>> @@ -136,6 +136,22 @@ void vma_set_page_prot(struct vm_area_struct *vma)
>>   	WRITE_ONCE(vma->vm_page_prot, vm_page_prot);
>>   }
>>   
>> +/*
>> + * Change backing file, only valid to use during initial VMA setup.
>> + */
>> +struct file *vma_set_file(struct vm_area_struct *vma, struct file *file)
>> +{
>> +	if (file)
>> +	        get_file(file);
>> +
>> +	swap(vma->vm_file, file);
>> +
>> +	if (file)
>> +		fput(file);
>> +
>> +	return file;
>> +}
>> +
> These users are all potentially modules.  You need an EXPORT_SYMBOL()?

Oh, good point. Yeah I totally missed that. The initial DMA-buf use case 
was not inside a module.

Thanks,
Christian.

Re: [PATCH 2/4] drm/prime: document that use the page array is deprecated

[Daniel Vetter]

On Thu, Oct 08, 2020 at 01:23:40PM +0200, Christian König wrote:
> We have reoccurring requests on this so better document that
> this approach doesn't work and dma_buf_mmap() needs to be used instead.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/drm_prime.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 4910c446db83..16fa2bfc271e 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -956,7 +956,7 @@ EXPORT_SYMBOL(drm_gem_prime_import);
>  /**
>   * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
>   * @sgt: scatter-gather table to convert
> - * @pages: optional array of page pointers to store the page array in
> + * @pages: deprecated array of page pointers to store the page array in
>   * @addrs: optional array to store the dma bus address of each page
>   * @max_entries: size of both the passed-in arrays
>   *
> @@ -965,6 +965,11 @@ EXPORT_SYMBOL(drm_gem_prime_import);
>   *
>   * Drivers can use this in their &drm_driver.gem_prime_import_sg_table
>   * implementation.
> + *
> + * Specifying the pages array is deprecated and strongly discouraged for new
> + * drivers. The pages array is only useful for page faults and those can
> + * corrupt fields in the struct page if they are not handled by the exporting
> + * driver.
>   */

I'd make this a _lot_ stronger: Aside from amdgpu and radeon all drivers
using this only need it for the pages array. Imo just open-code the sg
table walking loop in amdgpu/radeon (it's really not much code), and then
drop the dma_addr_t parameter from this function here (it's set to NULL by
everyone else).

And then deprecate this entire function here with a big warning that a)
dma_buf_map_attachment is allowed to leave the struct page pointers NULL
and b) this breaks mmap, users must call dma_buf_mmap instead.

Also maybe make it an uppercase DEPRECATED or something like that :-)
-Daniel

>  int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
>  				     dma_addr_t *addrs, int max_entries)
> -- 
> 2.17.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/4] mm: introduce vma_set_file function v2

[Daniel Vetter]

On Thu, Oct 08, 2020 at 01:23:39PM +0200, Christian König wrote:
> Add the new vma_set_file() function to allow changing
> vma->vm_file with the necessary refcount dance.
> 
> v2: add more users of this.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/dma-buf.c                  | 16 +++++-----------
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c      |  4 +---
>  drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  3 +--
>  drivers/gpu/drm/i915/gem/i915_gem_mman.c   |  4 ++--
>  drivers/gpu/drm/msm/msm_gem.c              |  4 +---
>  drivers/gpu/drm/omapdrm/omap_gem.c         |  3 +--
>  drivers/gpu/drm/vgem/vgem_drv.c            |  3 +--
>  drivers/staging/android/ashmem.c           |  5 ++---
>  include/linux/mm.h                         |  2 ++
>  mm/mmap.c                                  | 16 ++++++++++++++++
>  10 files changed, 32 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index a6ba4d598f0e..e4316aa7e0f4 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -1163,20 +1163,14 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
>  		return -EINVAL;
>  
>  	/* readjust the vma */
> -	get_file(dmabuf->file);
> -	oldfile = vma->vm_file;
> -	vma->vm_file = dmabuf->file;
> +	oldfile = vma_set_file(vma, dmabuf->file);
>  	vma->vm_pgoff = pgoff;
>  
>  	ret = dmabuf->ops->mmap(dmabuf, vma);
> -	if (ret) {
> -		/* restore old parameters on failure */
> -		vma->vm_file = oldfile;
> -		fput(dmabuf->file);
> -	} else {
> -		if (oldfile)
> -			fput(oldfile);
> -	}
> +	/* restore old parameters on failure */
> +	if (ret)
> +		vma_set_file(vma, oldfile);

I think these two lines here are cargo-cult: If this fails, the mmap fails
and therefore the vma structure is kfreed. No point at all in restoring
anything.

With that: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +
>  	return ret;
>  
>  }
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index 312e9d58d5a7..10ce267c0947 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -145,10 +145,8 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj,
>  		 * address_space (so unmap_mapping_range does what we want,
>  		 * in particular in the case of mmap'd dmabufs)
>  		 */
> -		fput(vma->vm_file);
> -		get_file(etnaviv_obj->base.filp);
>  		vma->vm_pgoff = 0;
> -		vma->vm_file  = etnaviv_obj->base.filp;
> +		vma_set_file(vma, etnaviv_obj->base.filp);
>  
>  		vma->vm_page_prot = vm_page_prot;
>  	}
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> index fec0e1e3dc3e..8ce4c9e28b87 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> @@ -119,8 +119,7 @@ static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *
>  	if (ret)
>  		return ret;
>  
> -	fput(vma->vm_file);
> -	vma->vm_file = get_file(obj->base.filp);
> +	vma_set_file(vma, obj->base.filp);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 3d69e51f3e4d..c9d5f1a38af3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>  	 * requires avoiding extraneous references to their filp, hence why
>  	 * we prefer to use an anonymous file for their mmaps.
>  	 */
> -	fput(vma->vm_file);
> -	vma->vm_file = anon;
> +	vma_set_file(vma, anon);
> +	fput(anon);
>  
>  	switch (mmo->mmap_type) {
>  	case I915_MMAP_TYPE_WC:
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index de915ff6f4b4..a71f42870d5e 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -223,10 +223,8 @@ int msm_gem_mmap_obj(struct drm_gem_object *obj,
>  		 * address_space (so unmap_mapping_range does what we want,
>  		 * in particular in the case of mmap'd dmabufs)
>  		 */
> -		fput(vma->vm_file);
> -		get_file(obj->filp);
>  		vma->vm_pgoff = 0;
> -		vma->vm_file  = obj->filp;
> +		vma_set_file(vma, obj->filp);
>  
>  		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>  	}
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
> index 979d53a93c2b..0d4542ff1d7d 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -564,9 +564,8 @@ int omap_gem_mmap_obj(struct drm_gem_object *obj,
>  		 * address_space (so unmap_mapping_range does what we want,
>  		 * in particular in the case of mmap'd dmabufs)
>  		 */
> -		fput(vma->vm_file);
>  		vma->vm_pgoff = 0;
> -		vma->vm_file  = get_file(obj->filp);
> +		vma_set_file(vma, obj->filp);
>  
>  		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>  	}
> diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
> index fa54a6d1403d..ea0eecae5153 100644
> --- a/drivers/gpu/drm/vgem/vgem_drv.c
> +++ b/drivers/gpu/drm/vgem/vgem_drv.c
> @@ -397,8 +397,7 @@ static int vgem_prime_mmap(struct drm_gem_object *obj,
>  	if (ret)
>  		return ret;
>  
> -	fput(vma->vm_file);
> -	vma->vm_file = get_file(obj->filp);
> +	vma_set_file(vma, obj->filp);
>  	vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
>  	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>  
> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> index 10b4be1f3e78..a51dc089896e 100644
> --- a/drivers/staging/android/ashmem.c
> +++ b/drivers/staging/android/ashmem.c
> @@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
>  		vma_set_anonymous(vma);
>  	}
>  
> -	if (vma->vm_file)
> -		fput(vma->vm_file);
> -	vma->vm_file = asma->file;
> +	vma_set_file(vma, asma->file);
> +	fput(asma->file);
>  
>  out:
>  	mutex_unlock(&ashmem_mutex);
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index ca6e6a81576b..a558602afe1b 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2693,6 +2693,8 @@ static inline void vma_set_page_prot(struct vm_area_struct *vma)
>  }
>  #endif
>  
> +struct file *vma_set_file(struct vm_area_struct *vma, struct file *file);
> +
>  #ifdef CONFIG_NUMA_BALANCING
>  unsigned long change_prot_numa(struct vm_area_struct *vma,
>  			unsigned long start, unsigned long end);
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 40248d84ad5f..d3c3c510f643 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -136,6 +136,22 @@ void vma_set_page_prot(struct vm_area_struct *vma)
>  	WRITE_ONCE(vma->vm_page_prot, vm_page_prot);
>  }
>  
> +/*
> + * Change backing file, only valid to use during initial VMA setup.
> + */
> +struct file *vma_set_file(struct vm_area_struct *vma, struct file *file)
> +{
> +	if (file)
> +	        get_file(file);
> +
> +	swap(vma->vm_file, file);
> +
> +	if (file)
> +		fput(file);
> +
> +	return file;
> +}
> +
>  /*
>   * Requires inode->i_mapping->i_mmap_rwsem
>   */
> -- 
> 2.17.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/4] drm/prime: document that use the page array is deprecated

[Daniel Vetter]

On Thu, Oct 08, 2020 at 04:09:14PM +0200, Daniel Vetter wrote:
> On Thu, Oct 08, 2020 at 01:23:40PM +0200, Christian König wrote:
> > We have reoccurring requests on this so better document that
> > this approach doesn't work and dma_buf_mmap() needs to be used instead.
> > 
> > Signed-off-by: Christian König <christian.koenig@amd.com>
> > ---
> >  drivers/gpu/drm/drm_prime.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > index 4910c446db83..16fa2bfc271e 100644
> > --- a/drivers/gpu/drm/drm_prime.c
> > +++ b/drivers/gpu/drm/drm_prime.c
> > @@ -956,7 +956,7 @@ EXPORT_SYMBOL(drm_gem_prime_import);
> >  /**
> >   * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
> >   * @sgt: scatter-gather table to convert
> > - * @pages: optional array of page pointers to store the page array in
> > + * @pages: deprecated array of page pointers to store the page array in
> >   * @addrs: optional array to store the dma bus address of each page
> >   * @max_entries: size of both the passed-in arrays
> >   *
> > @@ -965,6 +965,11 @@ EXPORT_SYMBOL(drm_gem_prime_import);
> >   *
> >   * Drivers can use this in their &drm_driver.gem_prime_import_sg_table
> >   * implementation.
> > + *
> > + * Specifying the pages array is deprecated and strongly discouraged for new
> > + * drivers. The pages array is only useful for page faults and those can
> > + * corrupt fields in the struct page if they are not handled by the exporting
> > + * driver.
> >   */
> 
> I'd make this a _lot_ stronger: Aside from amdgpu and radeon all drivers
> using this only need it for the pages array. Imo just open-code the sg
> table walking loop in amdgpu/radeon (it's really not much code), and then
> drop the dma_addr_t parameter from this function here (it's set to NULL by
> everyone else).
> 
> And then deprecate this entire function here with a big warning that a)
> dma_buf_map_attachment is allowed to leave the struct page pointers NULL
> and b) this breaks mmap, users must call dma_buf_mmap instead.
> 
> Also maybe make it an uppercase DEPRECATED or something like that :-)

OK I just realized I missed nouveau. That would be 3 places where we need
to stuff the dma_addr_t list into something ttm can take. Still feels
better than this half-deprecated function kludge ...
-Daniel

> -Daniel
> 
> >  int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
> >  				     dma_addr_t *addrs, int max_entries)
> > -- 
> > 2.17.1
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/4] mm: introduce vma_set_file function v2

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2952 bytes --]

Hi "Christian,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on hnaz-linux-mm/master staging/staging-testing linux/master linus/master v5.9-rc8 next-20201008]
[cannot apply to mmotm/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Christian-K-nig/mm-introduce-vma_set_file-function-v2/20201008-192433
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: h8300-randconfig-r036-20201008 (attached as .config)
compiler: h8300-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/839555b050d42ba052565bb71a11223e3d649c7a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christian-K-nig/mm-introduce-vma_set_file-function-v2/20201008-192433
        git checkout 839555b050d42ba052565bb71a11223e3d649c7a
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=h8300 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   h8300-linux-ld: section .init.text LMA [000000000048e500,00000000004cd3ef] overlaps section .text LMA [000000000000025c,0000000000b6acbf]
   h8300-linux-ld: section .data VMA [0000000000400000,000000000048e4ff] overlaps section .text VMA [000000000000025c,0000000000b6acbf]
   h8300-linux-ld: drivers/gpu/drm/vgem/vgem_drv.o: in function `vgem_prime_mmap':
>> drivers/gpu/drm/vgem/vgem_drv.c:396: undefined reference to `vma_set_file'
   h8300-linux-ld: drivers/dma-buf/dma-buf.o: in function `dma_buf_mmap':
   drivers/dma-buf/dma-buf.c:1166: undefined reference to `vma_set_file'
>> h8300-linux-ld: drivers/dma-buf/dma-buf.c:1172: undefined reference to `vma_set_file'

vim +396 drivers/gpu/drm/vgem/vgem_drv.c

   380	
   381	static int vgem_prime_mmap(struct drm_gem_object *obj,
   382				   struct vm_area_struct *vma)
   383	{
   384		int ret;
   385	
   386		if (obj->size < vma->vm_end - vma->vm_start)
   387			return -EINVAL;
   388	
   389		if (!obj->filp)
   390			return -ENODEV;
   391	
   392		ret = call_mmap(obj->filp, vma);
   393		if (ret)
   394			return ret;
   395	
 > 396		vma_set_file(vma, obj->filp);
   397		vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
   398		vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
   399	
   400		return 0;
   401	}
   402	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 19658 bytes --]

Re: [PATCH 1/4] mm: introduce vma_set_file function v2

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1872 bytes --]

Hi "Christian,

I love your patch! Yet something to improve:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on staging/staging-testing linux/master linus/master v5.9-rc8 next-20201008]
[cannot apply to mmotm/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Christian-K-nig/mm-introduce-vma_set_file-function-v2/20201008-192433
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: arm-randconfig-r021-20201008 (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/839555b050d42ba052565bb71a11223e3d649c7a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Christian-K-nig/mm-introduce-vma_set_file-function-v2/20201008-192433
        git checkout 839555b050d42ba052565bb71a11223e3d649c7a
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arm-linux-gnueabi-ld: drivers/dma-buf/dma-buf.o: in function `dma_buf_mmap':
   drivers/dma-buf/dma-buf.c:1166: undefined reference to `vma_set_file'
>> arm-linux-gnueabi-ld: drivers/dma-buf/dma-buf.c:1172: undefined reference to `vma_set_file'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29023 bytes --]

Re: [PATCH 1/4] mm: introduce vma_set_file function v2

[John Hubbard]

On 10/8/20 4:23 AM, Christian König wrote:
> Add the new vma_set_file() function to allow changing
> vma->vm_file with the necessary refcount dance.
> 
> v2: add more users of this.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/dma-buf/dma-buf.c                  | 16 +++++-----------
>   drivers/gpu/drm/etnaviv/etnaviv_gem.c      |  4 +---
>   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  3 +--
>   drivers/gpu/drm/i915/gem/i915_gem_mman.c   |  4 ++--
>   drivers/gpu/drm/msm/msm_gem.c              |  4 +---
>   drivers/gpu/drm/omapdrm/omap_gem.c         |  3 +--
>   drivers/gpu/drm/vgem/vgem_drv.c            |  3 +--
>   drivers/staging/android/ashmem.c           |  5 ++---
>   include/linux/mm.h                         |  2 ++
>   mm/mmap.c                                  | 16 ++++++++++++++++
>   10 files changed, 32 insertions(+), 28 deletions(-)

Looks like a nice cleanup. Two comments below.

...

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index 3d69e51f3e4d..c9d5f1a38af3 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>   	 * requires avoiding extraneous references to their filp, hence why
>   	 * we prefer to use an anonymous file for their mmaps.
>   	 */
> -	fput(vma->vm_file);
> -	vma->vm_file = anon;
> +	vma_set_file(vma, anon);
> +	fput(anon);

That's one fput() too many, isn't it?

...

> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> index 10b4be1f3e78..a51dc089896e 100644
> --- a/drivers/staging/android/ashmem.c
> +++ b/drivers/staging/android/ashmem.c
> @@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
>   		vma_set_anonymous(vma);
>   	}
>   
> -	if (vma->vm_file)
> -		fput(vma->vm_file);
> -	vma->vm_file = asma->file;
> +	vma_set_file(vma, asma->file);
> +	fput(asma->file);

Same here: that fput() seems wrong, as it was already done within vma_set_file().


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 1/4] mm: introduce vma_set_file function v2

[Christian König]

Am 08.10.20 um 16:12 schrieb Daniel Vetter:
> On Thu, Oct 08, 2020 at 01:23:39PM +0200, Christian König wrote:
>> Add the new vma_set_file() function to allow changing
>> vma->vm_file with the necessary refcount dance.
>>
>> v2: add more users of this.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/dma-buf/dma-buf.c                  | 16 +++++-----------
>>   drivers/gpu/drm/etnaviv/etnaviv_gem.c      |  4 +---
>>   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  3 +--
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c   |  4 ++--
>>   drivers/gpu/drm/msm/msm_gem.c              |  4 +---
>>   drivers/gpu/drm/omapdrm/omap_gem.c         |  3 +--
>>   drivers/gpu/drm/vgem/vgem_drv.c            |  3 +--
>>   drivers/staging/android/ashmem.c           |  5 ++---
>>   include/linux/mm.h                         |  2 ++
>>   mm/mmap.c                                  | 16 ++++++++++++++++
>>   10 files changed, 32 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
>> index a6ba4d598f0e..e4316aa7e0f4 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -1163,20 +1163,14 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
>>   		return -EINVAL;
>>   
>>   	/* readjust the vma */
>> -	get_file(dmabuf->file);
>> -	oldfile = vma->vm_file;
>> -	vma->vm_file = dmabuf->file;
>> +	oldfile = vma_set_file(vma, dmabuf->file);
>>   	vma->vm_pgoff = pgoff;
>>   
>>   	ret = dmabuf->ops->mmap(dmabuf, vma);
>> -	if (ret) {
>> -		/* restore old parameters on failure */
>> -		vma->vm_file = oldfile;
>> -		fput(dmabuf->file);
>> -	} else {
>> -		if (oldfile)
>> -			fput(oldfile);
>> -	}
>> +	/* restore old parameters on failure */
>> +	if (ret)
>> +		vma_set_file(vma, oldfile);
> I think these two lines here are cargo-cult: If this fails, the mmap fails
> and therefore the vma structure is kfreed. No point at all in restoring
> anything.

This was explicitly added with this patch to fix a problem:

commit 495c10cc1c0c359871d5bef32dd173252fc17995
Author: John Sheu <sheu@google.com>
Date:   Mon Feb 11 17:50:24 2013 -0800

     CHROMIUM: dma-buf: restore args on failure of dma_buf_mmap

     Callers to dma_buf_mmap expect to fput() the vma struct's vm_file
     themselves on failure.  Not restoring the struct's data on failure
     causes a double-decrement of the vm_file's refcount.

> With that: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Can I keep that even with the error handling working? :)

Christian.

>
>> +
>>   	return ret;
>>   
>>   }
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> index 312e9d58d5a7..10ce267c0947 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>> @@ -145,10 +145,8 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj,
>>   		 * address_space (so unmap_mapping_range does what we want,
>>   		 * in particular in the case of mmap'd dmabufs)
>>   		 */
>> -		fput(vma->vm_file);
>> -		get_file(etnaviv_obj->base.filp);
>>   		vma->vm_pgoff = 0;
>> -		vma->vm_file  = etnaviv_obj->base.filp;
>> +		vma_set_file(vma, etnaviv_obj->base.filp);
>>   
>>   		vma->vm_page_prot = vm_page_prot;
>>   	}
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>> index fec0e1e3dc3e..8ce4c9e28b87 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
>> @@ -119,8 +119,7 @@ static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *
>>   	if (ret)
>>   		return ret;
>>   
>> -	fput(vma->vm_file);
>> -	vma->vm_file = get_file(obj->base.filp);
>> +	vma_set_file(vma, obj->base.filp);
>>   
>>   	return 0;
>>   }
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> index 3d69e51f3e4d..c9d5f1a38af3 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> @@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>>   	 * requires avoiding extraneous references to their filp, hence why
>>   	 * we prefer to use an anonymous file for their mmaps.
>>   	 */
>> -	fput(vma->vm_file);
>> -	vma->vm_file = anon;
>> +	vma_set_file(vma, anon);
>> +	fput(anon);
>>   
>>   	switch (mmo->mmap_type) {
>>   	case I915_MMAP_TYPE_WC:
>> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
>> index de915ff6f4b4..a71f42870d5e 100644
>> --- a/drivers/gpu/drm/msm/msm_gem.c
>> +++ b/drivers/gpu/drm/msm/msm_gem.c
>> @@ -223,10 +223,8 @@ int msm_gem_mmap_obj(struct drm_gem_object *obj,
>>   		 * address_space (so unmap_mapping_range does what we want,
>>   		 * in particular in the case of mmap'd dmabufs)
>>   		 */
>> -		fput(vma->vm_file);
>> -		get_file(obj->filp);
>>   		vma->vm_pgoff = 0;
>> -		vma->vm_file  = obj->filp;
>> +		vma_set_file(vma, obj->filp);
>>   
>>   		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>>   	}
>> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
>> index 979d53a93c2b..0d4542ff1d7d 100644
>> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
>> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
>> @@ -564,9 +564,8 @@ int omap_gem_mmap_obj(struct drm_gem_object *obj,
>>   		 * address_space (so unmap_mapping_range does what we want,
>>   		 * in particular in the case of mmap'd dmabufs)
>>   		 */
>> -		fput(vma->vm_file);
>>   		vma->vm_pgoff = 0;
>> -		vma->vm_file  = get_file(obj->filp);
>> +		vma_set_file(vma, obj->filp);
>>   
>>   		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>>   	}
>> diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
>> index fa54a6d1403d..ea0eecae5153 100644
>> --- a/drivers/gpu/drm/vgem/vgem_drv.c
>> +++ b/drivers/gpu/drm/vgem/vgem_drv.c
>> @@ -397,8 +397,7 @@ static int vgem_prime_mmap(struct drm_gem_object *obj,
>>   	if (ret)
>>   		return ret;
>>   
>> -	fput(vma->vm_file);
>> -	vma->vm_file = get_file(obj->filp);
>> +	vma_set_file(vma, obj->filp);
>>   	vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
>>   	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
>>   
>> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
>> index 10b4be1f3e78..a51dc089896e 100644
>> --- a/drivers/staging/android/ashmem.c
>> +++ b/drivers/staging/android/ashmem.c
>> @@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
>>   		vma_set_anonymous(vma);
>>   	}
>>   
>> -	if (vma->vm_file)
>> -		fput(vma->vm_file);
>> -	vma->vm_file = asma->file;
>> +	vma_set_file(vma, asma->file);
>> +	fput(asma->file);
>>   
>>   out:
>>   	mutex_unlock(&ashmem_mutex);
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index ca6e6a81576b..a558602afe1b 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -2693,6 +2693,8 @@ static inline void vma_set_page_prot(struct vm_area_struct *vma)
>>   }
>>   #endif
>>   
>> +struct file *vma_set_file(struct vm_area_struct *vma, struct file *file);
>> +
>>   #ifdef CONFIG_NUMA_BALANCING
>>   unsigned long change_prot_numa(struct vm_area_struct *vma,
>>   			unsigned long start, unsigned long end);
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 40248d84ad5f..d3c3c510f643 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -136,6 +136,22 @@ void vma_set_page_prot(struct vm_area_struct *vma)
>>   	WRITE_ONCE(vma->vm_page_prot, vm_page_prot);
>>   }
>>   
>> +/*
>> + * Change backing file, only valid to use during initial VMA setup.
>> + */
>> +struct file *vma_set_file(struct vm_area_struct *vma, struct file *file)
>> +{
>> +	if (file)
>> +	        get_file(file);
>> +
>> +	swap(vma->vm_file, file);
>> +
>> +	if (file)
>> +		fput(file);
>> +
>> +	return file;
>> +}
>> +
>>   /*
>>    * Requires inode->i_mapping->i_mmap_rwsem
>>    */
>> -- 
>> 2.17.1
>>

Re: [PATCH 1/4] mm: introduce vma_set_file function v2

[Christian König]

Am 08.10.20 um 23:49 schrieb John Hubbard:
> On 10/8/20 4:23 AM, Christian König wrote:
>> Add the new vma_set_file() function to allow changing
>> vma->vm_file with the necessary refcount dance.
>>
>> v2: add more users of this.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/dma-buf/dma-buf.c                  | 16 +++++-----------
>>   drivers/gpu/drm/etnaviv/etnaviv_gem.c      |  4 +---
>>   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  3 +--
>>   drivers/gpu/drm/i915/gem/i915_gem_mman.c   |  4 ++--
>>   drivers/gpu/drm/msm/msm_gem.c              |  4 +---
>>   drivers/gpu/drm/omapdrm/omap_gem.c         |  3 +--
>>   drivers/gpu/drm/vgem/vgem_drv.c            |  3 +--
>>   drivers/staging/android/ashmem.c           |  5 ++---
>>   include/linux/mm.h                         |  2 ++
>>   mm/mmap.c                                  | 16 ++++++++++++++++
>>   10 files changed, 32 insertions(+), 28 deletions(-)
>
> Looks like a nice cleanup. Two comments below.
>
> ...
>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c 
>> b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> index 3d69e51f3e4d..c9d5f1a38af3 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>> @@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct 
>> vm_area_struct *vma)
>>        * requires avoiding extraneous references to their filp, hence 
>> why
>>        * we prefer to use an anonymous file for their mmaps.
>>        */
>> -    fput(vma->vm_file);
>> -    vma->vm_file = anon;
>> +    vma_set_file(vma, anon);
>> +    fput(anon);
>
> That's one fput() too many, isn't it?

No, the other cases were replacing the vm_file with something 
pre-allocated and also grabbed a new reference.

But this case here uses the freshly allocated anon file and so 
vma_set_file() grabs another extra reference which we need to drop.

The alternative is to just keep it as it is. Opinions?

>
>
> ...
>
>> diff --git a/drivers/staging/android/ashmem.c 
>> b/drivers/staging/android/ashmem.c
>> index 10b4be1f3e78..a51dc089896e 100644
>> --- a/drivers/staging/android/ashmem.c
>> +++ b/drivers/staging/android/ashmem.c
>> @@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct 
>> vm_area_struct *vma)
>>           vma_set_anonymous(vma);
>>       }
>>   -    if (vma->vm_file)
>> -        fput(vma->vm_file);
>> -    vma->vm_file = asma->file;
>> +    vma_set_file(vma, asma->file);
>> +    fput(asma->file);
>
> Same here: that fput() seems wrong, as it was already done within 
> vma_set_file().

No, that case is correct as well. The Android code here has the matching 
get_file() a few lines up, see the surrounding code.

I didn't wanted to replace that since it does some strange error 
handling here, so the result is that we need to drop the extra reference 
as again.

We could also keep it like it is or maybe better put a TODO comment on it.

Regards,
Christian.

>
>
>
> thanks,

Re: [PATCH 1/4] mm: introduce vma_set_file function v2

[John Hubbard]

On 10/9/20 12:33 AM, Christian König wrote:
> Am 08.10.20 um 23:49 schrieb John Hubbard:
>> On 10/8/20 4:23 AM, Christian König wrote:
>> ...
>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>> index 3d69e51f3e4d..c9d5f1a38af3 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>> @@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>>>        * requires avoiding extraneous references to their filp, hence why
>>>        * we prefer to use an anonymous file for their mmaps.
>>>        */
>>> -    fput(vma->vm_file);
>>> -    vma->vm_file = anon;
>>> +    vma_set_file(vma, anon);
>>> +    fput(anon);
>>
>> That's one fput() too many, isn't it?
> 
> No, the other cases were replacing the vm_file with something pre-allocated and also grabbed a new 
> reference.
> 
> But this case here uses the freshly allocated anon file and so vma_set_file() grabs another extra 
> reference which we need to drop.
> 
> The alternative is to just keep it as it is. Opinions?
> 

I think just a small comment for these cases, is probably about right.

>> ...
>>
>>> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
>>> index 10b4be1f3e78..a51dc089896e 100644
>>> --- a/drivers/staging/android/ashmem.c
>>> +++ b/drivers/staging/android/ashmem.c
>>> @@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
>>>           vma_set_anonymous(vma);
>>>       }
>>>   -    if (vma->vm_file)
>>> -        fput(vma->vm_file);
>>> -    vma->vm_file = asma->file;
>>> +    vma_set_file(vma, asma->file);
>>> +    fput(asma->file);
>>
>> Same here: that fput() seems wrong, as it was already done within vma_set_file().
> 
> No, that case is correct as well. The Android code here has the matching get_file() a few lines up, 
> see the surrounding code.
> 
> I didn't wanted to replace that since it does some strange error handling here, so the result is 
> that we need to drop the extra reference as again.
> 
> We could also keep it like it is or maybe better put a TODO comment on it.
> 

Yeah, I think a comment is a good way to go.


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH 2/4] drm/prime: document that use the page array is deprecated

[Christian König]

Am 08.10.20 um 16:14 schrieb Daniel Vetter:
> On Thu, Oct 08, 2020 at 04:09:14PM +0200, Daniel Vetter wrote:
>> On Thu, Oct 08, 2020 at 01:23:40PM +0200, Christian König wrote:
>>> We have reoccurring requests on this so better document that
>>> this approach doesn't work and dma_buf_mmap() needs to be used instead.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/drm_prime.c | 7 ++++++-
>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>> index 4910c446db83..16fa2bfc271e 100644
>>> --- a/drivers/gpu/drm/drm_prime.c
>>> +++ b/drivers/gpu/drm/drm_prime.c
>>> @@ -956,7 +956,7 @@ EXPORT_SYMBOL(drm_gem_prime_import);
>>>   /**
>>>    * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
>>>    * @sgt: scatter-gather table to convert
>>> - * @pages: optional array of page pointers to store the page array in
>>> + * @pages: deprecated array of page pointers to store the page array in
>>>    * @addrs: optional array to store the dma bus address of each page
>>>    * @max_entries: size of both the passed-in arrays
>>>    *
>>> @@ -965,6 +965,11 @@ EXPORT_SYMBOL(drm_gem_prime_import);
>>>    *
>>>    * Drivers can use this in their &drm_driver.gem_prime_import_sg_table
>>>    * implementation.
>>> + *
>>> + * Specifying the pages array is deprecated and strongly discouraged for new
>>> + * drivers. The pages array is only useful for page faults and those can
>>> + * corrupt fields in the struct page if they are not handled by the exporting
>>> + * driver.
>>>    */
>> I'd make this a _lot_ stronger: Aside from amdgpu and radeon all drivers
>> using this only need it for the pages array. Imo just open-code the sg
>> table walking loop in amdgpu/radeon (it's really not much code), and then
>> drop the dma_addr_t parameter from this function here (it's set to NULL by
>> everyone else).
>>
>> And then deprecate this entire function here with a big warning that a)
>> dma_buf_map_attachment is allowed to leave the struct page pointers NULL
>> and b) this breaks mmap, users must call dma_buf_mmap instead.
>>
>> Also maybe make it an uppercase DEPRECATED or something like that :-)
> OK I just realized I missed nouveau. That would be 3 places where we need
> to stuff the dma_addr_t list into something ttm can take. Still feels
> better than this half-deprecated function kludge ...

Mhm, I don't see a reason why nouveau would need the struct page either.

How about we split that up into two function?

One for converting the sg_table into a linear dma_addr array.

And one for converting the sg_table into a linear struct page array with 
a __deprecated attribute on it?

Christian.

> -Daniel
>
>> -Daniel
>>
>>>   int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
>>>   				     dma_addr_t *addrs, int max_entries)
>>> -- 
>>> 2.17.1
>>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

Re: [PATCH 1/4] mm: introduce vma_set_file function v2

[Daniel Vetter]

On Fri, Oct 09, 2020 at 09:16:49AM +0200, Christian König wrote:
> Am 08.10.20 um 16:12 schrieb Daniel Vetter:
> > On Thu, Oct 08, 2020 at 01:23:39PM +0200, Christian König wrote:
> > > Add the new vma_set_file() function to allow changing
> > > vma->vm_file with the necessary refcount dance.
> > > 
> > > v2: add more users of this.
> > > 
> > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > ---
> > >   drivers/dma-buf/dma-buf.c                  | 16 +++++-----------
> > >   drivers/gpu/drm/etnaviv/etnaviv_gem.c      |  4 +---
> > >   drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |  3 +--
> > >   drivers/gpu/drm/i915/gem/i915_gem_mman.c   |  4 ++--
> > >   drivers/gpu/drm/msm/msm_gem.c              |  4 +---
> > >   drivers/gpu/drm/omapdrm/omap_gem.c         |  3 +--
> > >   drivers/gpu/drm/vgem/vgem_drv.c            |  3 +--
> > >   drivers/staging/android/ashmem.c           |  5 ++---
> > >   include/linux/mm.h                         |  2 ++
> > >   mm/mmap.c                                  | 16 ++++++++++++++++
> > >   10 files changed, 32 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> > > index a6ba4d598f0e..e4316aa7e0f4 100644
> > > --- a/drivers/dma-buf/dma-buf.c
> > > +++ b/drivers/dma-buf/dma-buf.c
> > > @@ -1163,20 +1163,14 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
> > >   		return -EINVAL;
> > >   	/* readjust the vma */
> > > -	get_file(dmabuf->file);
> > > -	oldfile = vma->vm_file;
> > > -	vma->vm_file = dmabuf->file;
> > > +	oldfile = vma_set_file(vma, dmabuf->file);
> > >   	vma->vm_pgoff = pgoff;
> > >   	ret = dmabuf->ops->mmap(dmabuf, vma);
> > > -	if (ret) {
> > > -		/* restore old parameters on failure */
> > > -		vma->vm_file = oldfile;
> > > -		fput(dmabuf->file);
> > > -	} else {
> > > -		if (oldfile)
> > > -			fput(oldfile);
> > > -	}
> > > +	/* restore old parameters on failure */
> > > +	if (ret)
> > > +		vma_set_file(vma, oldfile);
> > I think these two lines here are cargo-cult: If this fails, the mmap fails
> > and therefore the vma structure is kfreed. No point at all in restoring
> > anything.
> 
> This was explicitly added with this patch to fix a problem:
> 
> commit 495c10cc1c0c359871d5bef32dd173252fc17995
> Author: John Sheu <sheu@google.com>
> Date:   Mon Feb 11 17:50:24 2013 -0800
> 
>     CHROMIUM: dma-buf: restore args on failure of dma_buf_mmap
> 
>     Callers to dma_buf_mmap expect to fput() the vma struct's vm_file
>     themselves on failure.  Not restoring the struct's data on failure
>     causes a double-decrement of the vm_file's refcount.
> 
> > With that: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Can I keep that even with the error handling working? :)

Hm good find, I should have looked at git history myself.

I just noticed this here in the patch because everyone else does not do
this. But looking at the mmap_region() code in mmap.c we seem to indeed
have this problem for the error path:

unmap_and_free_vma:
	vma->vm_file = NULL;
	fput(file);

Note that the success path does things correctly (a bit above):

	file = vma->vm_file;
out:

So it indeed looks like dma-buf is the only one that does this fully
correctly. So maybe we should do a follow-up patch to change the
mmap_region exit code to pick up whatever vma->vm_file was set instead,
and fput that?

Anyway I correct, r-b: as-is.

Cheers, Daniel

> 
> Christian.
> 
> > 
> > > +
> > >   	return ret;
> > >   }
> > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > index 312e9d58d5a7..10ce267c0947 100644
> > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > @@ -145,10 +145,8 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj,
> > >   		 * address_space (so unmap_mapping_range does what we want,
> > >   		 * in particular in the case of mmap'd dmabufs)
> > >   		 */
> > > -		fput(vma->vm_file);
> > > -		get_file(etnaviv_obj->base.filp);
> > >   		vma->vm_pgoff = 0;
> > > -		vma->vm_file  = etnaviv_obj->base.filp;
> > > +		vma_set_file(vma, etnaviv_obj->base.filp);
> > >   		vma->vm_page_prot = vm_page_prot;
> > >   	}
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> > > index fec0e1e3dc3e..8ce4c9e28b87 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c
> > > @@ -119,8 +119,7 @@ static int i915_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *
> > >   	if (ret)
> > >   		return ret;
> > > -	fput(vma->vm_file);
> > > -	vma->vm_file = get_file(obj->base.filp);
> > > +	vma_set_file(vma, obj->base.filp);
> > >   	return 0;
> > >   }
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > index 3d69e51f3e4d..c9d5f1a38af3 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > > @@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
> > >   	 * requires avoiding extraneous references to their filp, hence why
> > >   	 * we prefer to use an anonymous file for their mmaps.
> > >   	 */
> > > -	fput(vma->vm_file);
> > > -	vma->vm_file = anon;
> > > +	vma_set_file(vma, anon);
> > > +	fput(anon);
> > >   	switch (mmo->mmap_type) {
> > >   	case I915_MMAP_TYPE_WC:
> > > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> > > index de915ff6f4b4..a71f42870d5e 100644
> > > --- a/drivers/gpu/drm/msm/msm_gem.c
> > > +++ b/drivers/gpu/drm/msm/msm_gem.c
> > > @@ -223,10 +223,8 @@ int msm_gem_mmap_obj(struct drm_gem_object *obj,
> > >   		 * address_space (so unmap_mapping_range does what we want,
> > >   		 * in particular in the case of mmap'd dmabufs)
> > >   		 */
> > > -		fput(vma->vm_file);
> > > -		get_file(obj->filp);
> > >   		vma->vm_pgoff = 0;
> > > -		vma->vm_file  = obj->filp;
> > > +		vma_set_file(vma, obj->filp);
> > >   		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> > >   	}
> > > diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
> > > index 979d53a93c2b..0d4542ff1d7d 100644
> > > --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> > > +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> > > @@ -564,9 +564,8 @@ int omap_gem_mmap_obj(struct drm_gem_object *obj,
> > >   		 * address_space (so unmap_mapping_range does what we want,
> > >   		 * in particular in the case of mmap'd dmabufs)
> > >   		 */
> > > -		fput(vma->vm_file);
> > >   		vma->vm_pgoff = 0;
> > > -		vma->vm_file  = get_file(obj->filp);
> > > +		vma_set_file(vma, obj->filp);
> > >   		vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> > >   	}
> > > diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
> > > index fa54a6d1403d..ea0eecae5153 100644
> > > --- a/drivers/gpu/drm/vgem/vgem_drv.c
> > > +++ b/drivers/gpu/drm/vgem/vgem_drv.c
> > > @@ -397,8 +397,7 @@ static int vgem_prime_mmap(struct drm_gem_object *obj,
> > >   	if (ret)
> > >   		return ret;
> > > -	fput(vma->vm_file);
> > > -	vma->vm_file = get_file(obj->filp);
> > > +	vma_set_file(vma, obj->filp);
> > >   	vma->vm_flags |= VM_DONTEXPAND | VM_DONTDUMP;
> > >   	vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> > > diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
> > > index 10b4be1f3e78..a51dc089896e 100644
> > > --- a/drivers/staging/android/ashmem.c
> > > +++ b/drivers/staging/android/ashmem.c
> > > @@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
> > >   		vma_set_anonymous(vma);
> > >   	}
> > > -	if (vma->vm_file)
> > > -		fput(vma->vm_file);
> > > -	vma->vm_file = asma->file;
> > > +	vma_set_file(vma, asma->file);
> > > +	fput(asma->file);
> > >   out:
> > >   	mutex_unlock(&ashmem_mutex);
> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > index ca6e6a81576b..a558602afe1b 100644
> > > --- a/include/linux/mm.h
> > > +++ b/include/linux/mm.h
> > > @@ -2693,6 +2693,8 @@ static inline void vma_set_page_prot(struct vm_area_struct *vma)
> > >   }
> > >   #endif
> > > +struct file *vma_set_file(struct vm_area_struct *vma, struct file *file);
> > > +
> > >   #ifdef CONFIG_NUMA_BALANCING
> > >   unsigned long change_prot_numa(struct vm_area_struct *vma,
> > >   			unsigned long start, unsigned long end);
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index 40248d84ad5f..d3c3c510f643 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -136,6 +136,22 @@ void vma_set_page_prot(struct vm_area_struct *vma)
> > >   	WRITE_ONCE(vma->vm_page_prot, vm_page_prot);
> > >   }
> > > +/*
> > > + * Change backing file, only valid to use during initial VMA setup.
> > > + */
> > > +struct file *vma_set_file(struct vm_area_struct *vma, struct file *file)
> > > +{
> > > +	if (file)
> > > +	        get_file(file);
> > > +
> > > +	swap(vma->vm_file, file);
> > > +
> > > +	if (file)
> > > +		fput(file);
> > > +
> > > +	return file;
> > > +}
> > > +
> > >   /*
> > >    * Requires inode->i_mapping->i_mmap_rwsem
> > >    */
> > > -- 
> > > 2.17.1
> > > 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/4] drm/prime: document that use the page array is deprecated

[Daniel Vetter]

On Fri, Oct 09, 2020 at 09:36:41AM +0200, Christian König wrote:
> Am 08.10.20 um 16:14 schrieb Daniel Vetter:
> > On Thu, Oct 08, 2020 at 04:09:14PM +0200, Daniel Vetter wrote:
> > > On Thu, Oct 08, 2020 at 01:23:40PM +0200, Christian König wrote:
> > > > We have reoccurring requests on this so better document that
> > > > this approach doesn't work and dma_buf_mmap() needs to be used instead.
> > > > 
> > > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > > ---
> > > >   drivers/gpu/drm/drm_prime.c | 7 ++++++-
> > > >   1 file changed, 6 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > > > index 4910c446db83..16fa2bfc271e 100644
> > > > --- a/drivers/gpu/drm/drm_prime.c
> > > > +++ b/drivers/gpu/drm/drm_prime.c
> > > > @@ -956,7 +956,7 @@ EXPORT_SYMBOL(drm_gem_prime_import);
> > > >   /**
> > > >    * drm_prime_sg_to_page_addr_arrays - convert an sg table into a page array
> > > >    * @sgt: scatter-gather table to convert
> > > > - * @pages: optional array of page pointers to store the page array in
> > > > + * @pages: deprecated array of page pointers to store the page array in
> > > >    * @addrs: optional array to store the dma bus address of each page
> > > >    * @max_entries: size of both the passed-in arrays
> > > >    *
> > > > @@ -965,6 +965,11 @@ EXPORT_SYMBOL(drm_gem_prime_import);
> > > >    *
> > > >    * Drivers can use this in their &drm_driver.gem_prime_import_sg_table
> > > >    * implementation.
> > > > + *
> > > > + * Specifying the pages array is deprecated and strongly discouraged for new
> > > > + * drivers. The pages array is only useful for page faults and those can
> > > > + * corrupt fields in the struct page if they are not handled by the exporting
> > > > + * driver.
> > > >    */
> > > I'd make this a _lot_ stronger: Aside from amdgpu and radeon all drivers
> > > using this only need it for the pages array. Imo just open-code the sg
> > > table walking loop in amdgpu/radeon (it's really not much code), and then
> > > drop the dma_addr_t parameter from this function here (it's set to NULL by
> > > everyone else).
> > > 
> > > And then deprecate this entire function here with a big warning that a)
> > > dma_buf_map_attachment is allowed to leave the struct page pointers NULL
> > > and b) this breaks mmap, users must call dma_buf_mmap instead.
> > > 
> > > Also maybe make it an uppercase DEPRECATED or something like that :-)
> > OK I just realized I missed nouveau. That would be 3 places where we need
> > to stuff the dma_addr_t list into something ttm can take. Still feels
> > better than this half-deprecated function kludge ...
> 
> Mhm, I don't see a reason why nouveau would need the struct page either.
> 
> How about we split that up into two function?
> 
> One for converting the sg_table into a linear dma_addr array.
> 
> And one for converting the sg_table into a linear struct page array with a
> __deprecated attribute on it?

Yeah makes sense, since converting ttm to just use sgt iterations directly
everywhere is probably a bit too much. Maybe keep that converter in ttm
code, since outside of ttm the rough consensus is to converge on sgt for
handling buffers. Well, for those drivers not stuck on struct page arrays
:-)
-Daniel

> 
> Christian.
> 
> > -Daniel
> > 
> > > -Daniel
> > > 
> > > >   int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
> > > >   				     dma_addr_t *addrs, int max_entries)
> > > > -- 
> > > > 2.17.1
> > > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/4] mm: introduce vma_set_file function v2

[Jason Gunthorpe]

On Fri, Oct 09, 2020 at 09:39:00AM +0200, Daniel Vetter wrote:
> I just noticed this here in the patch because everyone else does not do
> this. But looking at the mmap_region() code in mmap.c we seem to indeed
> have this problem for the error path:
> 
> unmap_and_free_vma:
> 	vma->vm_file = NULL;
> 	fput(file);
> 
> Note that the success path does things correctly (a bit above):
> 
> 	file = vma->vm_file;
> out:
> 
> So it indeed looks like dma-buf is the only one that does this fully
> correctly. So maybe we should do a follow-up patch to change the
> mmap_region exit code to pick up whatever vma->vm_file was set instead,
> and fput that?

Given that this new vma_set_file() should be the only way to
manipulate vm_file from the mmap op, I think this reflects a bug in
mm/mmap.c.. Should be:

unmap_and_free_vma:
        fput(vma->vm_file);
        vma->vm_file = NULL;

Then everything works the way you'd expect without tricky error
handling

Jason

Re: [PATCH 1/4] mm: introduce vma_set_file function v2

[Christian König]

Am 09.10.20 um 14:12 schrieb Jason Gunthorpe:
> On Fri, Oct 09, 2020 at 09:39:00AM +0200, Daniel Vetter wrote:
>> I just noticed this here in the patch because everyone else does not do
>> this. But looking at the mmap_region() code in mmap.c we seem to indeed
>> have this problem for the error path:
>>
>> unmap_and_free_vma:
>> 	vma->vm_file = NULL;
>> 	fput(file);
>>
>> Note that the success path does things correctly (a bit above):
>>
>> 	file = vma->vm_file;
>> out:
>>
>> So it indeed looks like dma-buf is the only one that does this fully
>> correctly. So maybe we should do a follow-up patch to change the
>> mmap_region exit code to pick up whatever vma->vm_file was set instead,
>> and fput that?
> Given that this new vma_set_file() should be the only way to
> manipulate vm_file from the mmap op, I think this reflects a bug in
> mm/mmap.c.. Should be:
>
> unmap_and_free_vma:
>          fput(vma->vm_file);
>          vma->vm_file = NULL;
>
> Then everything works the way you'd expect without tricky error
> handling

That's what Daniel suggested as well, yes.

Going to craft a separate patch for this.

Thanks,
Christian.

>
> Jason