On 2020-08-26 07:32, Marek Szyprowski wrote:

It is a common operation done by DRM drivers to check the contiguity of the DMA-mapped buffer described by a scatterlist in the sg_table object. Let's add a common helper for this operation.

I still think this could be hoisted even further out to the common sgtable API level, but let's get the individual subsystems straightened out first then worry about consolidation later.

Reviewed-by: Robin Murphy robin.murphy@arm.com

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com Reviewed-by: Andrzej Hajda a.hajda@samsung.com

---

drivers/gpu/drm/drm_gem_cma_helper.c | 23 +++------------------ drivers/gpu/drm/drm_prime.c | 31 ++++++++++++++++++++++++++++ include/drm/drm_prime.h | 2 ++ 3 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 822edeadbab3..59b9ca207b42 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -471,26 +471,9 @@ drm_gem_cma_prime_import_sg_table(struct drm_device *dev, { struct drm_gem_cma_object *cma_obj;

• if (sgt->nents != 1) {

• /* check if the entries in the sg_table are contiguous */
  

• dma_addr_t next_addr = sg_dma_address(sgt->sgl);
  

• struct scatterlist *s;
  

• unsigned int i;
  

• for_each_sg(sgt->sgl, s, sgt->nents, i) {
  

• 	/*
  

• 	 * sg_dma_address(s) is only valid for entries
  

• 	 * that have sg_dma_len(s) != 0
  

• 	 */
  

• 	if (!sg_dma_len(s))
  

• 		continue;
  

• 	if (sg_dma_address(s) != next_addr)
  

• 		return ERR_PTR(-EINVAL);
  

• 	next_addr = sg_dma_address(s) + sg_dma_len(s);
  

• }
  

• }

• /* check if the entries in the sg_table are contiguous */

• if (drm_prime_get_contiguous_size(sgt) < attach->dmabuf->size)

• return ERR_PTR(-EINVAL);
  
  

  /* Create a CMA GEM buffer. */ cma_obj = __drm_gem_cma_create(dev, attach->dmabuf->size);

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 1693aa7c14b5..4ed5ed1f078c 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -825,6 +825,37 @@ struct sg_table *drm_prime_pages_to_sg(struct page **pages, unsigned int nr_page } EXPORT_SYMBOL(drm_prime_pages_to_sg);

+/**

• • drm_prime_get_contiguous_size - returns the contiguous size of the buffer
• • @sgt: sg_table describing the buffer to check
• • This helper calculates the contiguous size in the DMA address space
• • of the the buffer described by the provided sg_table.
• • This is useful for implementing
• • &drm_gem_object_funcs.gem_prime_import_sg_table.
• */

+unsigned long drm_prime_get_contiguous_size(struct sg_table *sgt) +{

• dma_addr_t expected = sg_dma_address(sgt->sgl);
• struct scatterlist *sg;
• unsigned long size = 0;
• int i;
• for_each_sgtable_dma_sg(sgt, sg, i) {

• unsigned int len = sg_dma_len(sg);
  

• if (!len)
  

• 	break;
  

• if (sg_dma_address(sg) != expected)
  

• 	break;
  

• expected += len;
  

• size += len;
  

• }
• return size;

+} +EXPORT_SYMBOL(drm_prime_get_contiguous_size);

• /**
  • drm_gem_prime_export - helper library implementation of the export callback
  • @obj: GEM object to export

diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h index 9af7422b44cf..47ef11614627 100644 --- a/include/drm/drm_prime.h +++ b/include/drm/drm_prime.h @@ -92,6 +92,8 @@ struct sg_table *drm_prime_pages_to_sg(struct page **pages, unsigned int nr_page struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, int flags);

+unsigned long drm_prime_get_contiguous_size(struct sg_table *sgt);

• /* helper functions for importing */ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, struct dma_buf *dma_buf,

On 2020-08-26 07:32, Marek Szyprowski wrote:

Replace the current hand-crafted code for extracting pages and DMA addresses from the given scatterlist by the much more robust code based on the generic scatterlist iterators and recently introduced sg_table-based wrappers. The resulting code is simple and easy to understand, so the comment describing the old code is no longer needed.

Is removing the WARN_ON()s intentional? It certainly seems like it would be a genuine driver bug if the caller asked for addresses but didn't allocate appropriately-sized arrays. Might be worth noting either way. I'm also assuming this isn't called in performance-critical paths with massive lists such that the two separate iterations might have a noticeable impact.

Nits aside,

Reviewed-by: Robin Murphy robin.murphy@arm.com

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com Reviewed-by: Andrzej Hajda a.hajda@samsung.com

---

drivers/gpu/drm/drm_prime.c | 49 ++++++++++++------------------------- 1 file changed, 15 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 4ed5ed1f078c..5d181bf60a44 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -990,45 +990,26 @@ EXPORT_SYMBOL(drm_gem_prime_import); int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages, dma_addr_t *addrs, int max_entries) {

• unsigned count;
• struct scatterlist *sg;
• struct page *page;
• u32 page_len, page_index;
• dma_addr_t addr;
• u32 dma_len, dma_index;
• /*

• * Scatterlist elements contains both pages and DMA addresses, but
  

• * one shoud not assume 1:1 relation between them. The sg->length is
  

• * the size of the physical memory chunk described by the sg->page,
  

• * while sg_dma_len(sg) is the size of the DMA (IO virtual) chunk
  

• * described by the sg_dma_address(sg).
  

• */
  

• page_index = 0;
• dma_index = 0;
• for_each_sg(sgt->sgl, sg, sgt->nents, count) {

• page_len = sg->length;
  

• page = sg_page(sg);
  

• dma_len = sg_dma_len(sg);
  

• addr = sg_dma_address(sg);
  

• while (pages && page_len > 0) {
  

• 	if (WARN_ON(page_index >= max_entries))
  

• struct sg_dma_page_iter dma_iter;
• struct sg_page_iter page_iter;
• struct page **p = pages;
• dma_addr_t *a = addrs;
• if (pages) {

• for_each_sgtable_page(sgt, &page_iter, 0) {
  

• 	if (p - pages >= max_entries)
  	return -1;
  

• 	pages[page_index] = page;
  

• 	page++;
  

• 	page_len -= PAGE_SIZE;
  

• 	page_index++;
  

• 	*p++ = sg_page_iter_page(&page_iter);
  

  }

• while (addrs && dma_len > 0) {
  

• 	if (WARN_ON(dma_index >= max_entries))
  

• }
• if (addrs) {

• for_each_sgtable_dma_page(sgt, &dma_iter, 0) {
  

• 	if (a - addrs >= max_entries)
  	return -1;
  

• 	addrs[dma_index] = addr;
  

• 	addr += PAGE_SIZE;
  

• 	dma_len -= PAGE_SIZE;
  

• 	dma_index++;
  

• 	*a++ = sg_page_iter_dma_address(&dma_iter);
  

  } }
• return 0; } EXPORT_SYMBOL(drm_prime_sg_to_page_addr_arrays);

On 2020-08-26 07:32, Marek Szyprowski wrote:

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function returns the number of the created entries in the DMA address space. However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with the original number of the entries passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous memory buffer, used commonly in the DRM and graphics subsystems. It consists of a scatterlist with memory pages and DMA addresses (sgl entry), as well as the number of scatterlist entries: CPU pages (orig_nents entry) and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents entries, calling DMA-mapping functions with a wrong number of entries or ignoring the number of mapped entries returned by the dma_map_sg() function.

To avoid such issues, lets use a common dma-mapping wrappers operating directly on the struct sg_table objects and use scatterlist page iterators where possible. This, almost always, hides references to the nents and orig_nents entries, making the code robust, easier to follow and copy/paste safe.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com Reviewed-by: Andrzej Hajda a.hajda@samsung.com

---

drivers/gpu/drm/drm_cache.c | 2 +- drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +++++++++----- drivers/gpu/drm/drm_prime.c | 11 ++++++----- 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c index 03e01b000f7a..0fe3c496002a 100644 --- a/drivers/gpu/drm/drm_cache.c +++ b/drivers/gpu/drm/drm_cache.c @@ -127,7 +127,7 @@ drm_clflush_sg(struct sg_table *st) struct sg_page_iter sg_iter;

mb(); /*CLFLUSH is ordered only by using memory barriers*/

• for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
  

• for_each_sgtable_page(st, &sg_iter, 0)
  drm_clflush_page(sg_page_iter_page(&sg_iter));
  

  mb(); /*Make sure that all cache line entry is flushed*/

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 4b7cfbac4daa..47d8211221f2 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -126,8 +126,8 @@ void drm_gem_shmem_free_object(struct drm_gem_object *obj) drm_prime_gem_destroy(obj, shmem->sgt); } else { if (shmem->sgt) {

• 	dma_unmap_sg(obj->dev->dev, shmem->sgt->sgl,
  

• 		     shmem->sgt->nents, DMA_BIDIRECTIONAL);
  

• 	dma_unmap_sgtable(obj->dev->dev, shmem->sgt,
  

• 			  DMA_BIDIRECTIONAL, 0);
  sg_free_table(shmem->sgt);
  kfree(shmem->sgt);
  

  }

@@ -424,8 +424,7 @@ void drm_gem_shmem_purge_locked(struct drm_gem_object *obj)

WARN_ON(!drm_gem_shmem_is_purgeable(shmem));

• dma_unmap_sg(obj->dev->dev, shmem->sgt->sgl,

•      shmem->sgt->nents, DMA_BIDIRECTIONAL);
  

• dma_unmap_sgtable(obj->dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0); sg_free_table(shmem->sgt); kfree(shmem->sgt); shmem->sgt = NULL;

@@ -697,12 +696,17 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_object *obj) goto err_put_pages; } /* Map the pages for use by the h/w. */

• dma_map_sg(obj->dev->dev, sgt->sgl, sgt->nents, DMA_BIDIRECTIONAL);

• ret = dma_map_sgtable(obj->dev->dev, sgt, DMA_BIDIRECTIONAL, 0);

• if (ret)

• goto err_free_sgt;
  
  

  shmem->sgt = sgt;

  return sgt;

+err_free_sgt:

• sg_free_table(sgt);
• kfree(sgt);

Should this be a separate patch to add the missing error handling to the existing code first?

Otherwise the rest of the mechanical conversion looks straightforward enough, and I'm not the separation-of-concerns police (for this subsystem, at least), so either way,

Reviewed-by: Robin Murphy robin.murphy@arm.com

err_put_pages: drm_gem_shmem_put_pages(shmem); return ERR_PTR(ret); diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 5d181bf60a44..c45b0cc6e31d 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -617,6 +617,7 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach, { struct drm_gem_object *obj = attach->dmabuf->priv; struct sg_table *sgt;

• int ret;

  if (WARN_ON(dir == DMA_NONE)) return ERR_PTR(-EINVAL);

@@ -626,11 +627,12 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach, else sgt = obj->dev->driver->gem_prime_get_sg_table(obj);

• if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,

• 	      DMA_ATTR_SKIP_CPU_SYNC)) {
  

• ret = dma_map_sgtable(attach->dev, sgt, dir,

• 	      DMA_ATTR_SKIP_CPU_SYNC);
  

• if (ret) { sg_free_table(sgt); kfree(sgt);

• sgt = ERR_PTR(-ENOMEM);
  

• sgt = ERR_PTR(ret);
  

  }

  return sgt;

@@ -652,8 +654,7 @@ void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach, if (!sgt) return;

• dma_unmap_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,

• 	   DMA_ATTR_SKIP_CPU_SYNC);
  

• dma_unmap_sgtable(attach->dev, sgt, dir, DMA_ATTR_SKIP_CPU_SYNC); sg_free_table(sgt); kfree(sgt); }

On 2020-08-26 07:32, Marek Szyprowski wrote:

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function returns the number of the created entries in the DMA address space. However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with the original number of the entries passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous memory buffer, used commonly in the DRM and graphics subsystems. It consists of a scatterlist with memory pages and DMA addresses (sgl entry), as well as the number of scatterlist entries: CPU pages (orig_nents entry) and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents entries, calling DMA-mapping functions with a wrong number of entries or ignoring the number of mapped entries returned by the dma_map_sg() function.

To avoid such issues, lets use a common dma-mapping wrappers operating directly on the struct sg_table objects and use scatterlist page iterators where possible. This, almost always, hides references to the nents and orig_nents entries, making the code robust, easier to follow and copy/paste safe.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/gpu/drm/armada/armada_gem.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c index 8005614d2e6b..bedd8937d8a1 100644 --- a/drivers/gpu/drm/armada/armada_gem.c +++ b/drivers/gpu/drm/armada/armada_gem.c @@ -395,7 +395,7 @@ armada_gem_prime_map_dma_buf(struct dma_buf_attachment *attach,

mapping = dobj->obj.filp->f_mapping;

• for_each_sg(sgt->sgl, sg, count, i) {
  

• for_each_sgtable_sg(sgt, sg, i) {
  struct page *page;
  
  page = shmem_read_mapping_page(mapping, i);
  

@@ -407,8 +407,8 @@ armada_gem_prime_map_dma_buf(struct dma_buf_attachment *attach, sg_set_page(sg, page, PAGE_SIZE, 0); }

• if (dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir) == 0) {
  

• 	num = sgt->nents;
  

• if (dma_map_sgtable(attach->dev, sgt, dir, 0)) {
  

• 	num = count;
  

I think it might be even nicer to get rid of "num" entirely and convert the cleanup path to for_each_sgtable_sg() for completeness - AFAICS it should only need an extra "if (sg_page(sg))..." check in that loop. Then "count" could possibly be squashed into its one remaining use as well, but maybe it's worth keeping for readability.

Robin.

	goto release;
}

} else if (dobj->page) { @@ -418,7 +418,7 @@ armada_gem_prime_map_dma_buf(struct dma_buf_attachment *attach,

sg_set_page(sgt->sgl, dobj->page, dobj->obj.size, 0);

• if (dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir) == 0)
  

• if (dma_map_sgtable(attach->dev, sgt, dir, 0))
  goto free_table;
  

  } else if (dobj->linear) { /* Single contiguous physical region - no struct page */

@@ -449,11 +449,11 @@ static void armada_gem_prime_unmap_dma_buf(struct dma_buf_attachment *attach, int i;

if (!dobj->linear)

• dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
  

• dma_unmap_sgtable(attach->dev, sgt, dir, 0);
  
  

  if (dobj->obj.filp) { struct scatterlist *sg;

• for_each_sg(sgt->sgl, sg, sgt->nents, i)
  

• for_each_sgtable_sg(sgt, sg, i)
  put_page(sg_page(sg));
  

  }

On 2020-08-26 07:32, Marek Szyprowski wrote:

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function returns the number of the created entries in the DMA address space. However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with the original number of the entries passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous memory buffer, used commonly in the DRM and graphics subsystems. It consists of a scatterlist with memory pages and DMA addresses (sgl entry), as well as the number of scatterlist entries: CPU pages (orig_nents entry) and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents entries, calling DMA-mapping functions with a wrong number of entries or ignoring the number of mapped entries returned by the dma_map_sg() function.

To avoid such issues, lets use a common dma-mapping wrappers operating directly on the struct sg_table objects and use scatterlist page iterators where possible. This, almost always, hides references to the nents and orig_nents entries, making the code robust, easier to follow and copy/paste safe.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/gpu/drm/etnaviv/etnaviv_gem.c | 12 +++++------- drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 13 +++---------- 2 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index f06e19e7be04..eaf1949bc2e4 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -27,7 +27,7 @@ static void etnaviv_gem_scatter_map(struct etnaviv_gem_object *etnaviv_obj) * because display controller, GPU, etc. are not coherent. */ if (etnaviv_obj->flags & ETNA_BO_CACHE_MASK)

• dma_map_sg(dev->dev, sgt->sgl, sgt->nents, DMA_BIDIRECTIONAL);
  

• dma_map_sgtable(dev->dev, sgt, DMA_BIDIRECTIONAL, 0);
  

  }

  static void etnaviv_gem_scatterlist_unmap(struct etnaviv_gem_object *etnaviv_obj)

@@ -51,7 +51,7 @@ static void etnaviv_gem_scatterlist_unmap(struct etnaviv_gem_object *etnaviv_obj * discard those writes. */ if (etnaviv_obj->flags & ETNA_BO_CACHE_MASK)

• dma_unmap_sg(dev->dev, sgt->sgl, sgt->nents, DMA_BIDIRECTIONAL);
  

• dma_unmap_sgtable(dev->dev, sgt, DMA_BIDIRECTIONAL, 0);
  

  }

  /* called with etnaviv_obj->lock held */

@@ -404,9 +404,8 @@ int etnaviv_gem_cpu_prep(struct drm_gem_object *obj, u32 op, }

if (etnaviv_obj->flags & ETNA_BO_CACHED) {

• dma_sync_sg_for_cpu(dev->dev, etnaviv_obj->sgt->sgl,
  

• 		    etnaviv_obj->sgt->nents,
  

• 		    etnaviv_op_to_dma_dir(op));
  

• dma_sync_sgtable_for_cpu(dev->dev, etnaviv_obj->sgt,
  

• 			 etnaviv_op_to_dma_dir(op));
  

  etnaviv_obj->last_cpu_prep_op = op; }

@@ -421,8 +420,7 @@ int etnaviv_gem_cpu_fini(struct drm_gem_object *obj) if (etnaviv_obj->flags & ETNA_BO_CACHED) { /* fini without a prep is almost certainly a userspace error */ WARN_ON(etnaviv_obj->last_cpu_prep_op == 0);

• dma_sync_sg_for_device(dev->dev, etnaviv_obj->sgt->sgl,
  

• 	etnaviv_obj->sgt->nents,
  

• dma_sync_sgtable_for_device(dev->dev, etnaviv_obj->sgt,
  etnaviv_op_to_dma_dir(etnaviv_obj->last_cpu_prep_op));
  

  etnaviv_obj->last_cpu_prep_op = 0; }

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c index 3607d348c298..13b100553a0b 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_mmu.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_mmu.c @@ -79,7 +79,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova, if (!context || !sgt) return -EINVAL;

• for_each_sg(sgt->sgl, sg, sgt->nents, i) {

• for_each_sgtable_dma_sg(sgt, sg, i) { u32 pa = sg_dma_address(sg) - sg->offset; size_t bytes = sg_dma_len(sg) + sg->offset;

@@ -95,14 +95,7 @@ static int etnaviv_iommu_map(struct etnaviv_iommu_context *context, u32 iova, return 0;

fail:

• da = iova;
• for_each_sg(sgt->sgl, sg, i, j) {

• size_t bytes = sg_dma_len(sg) + sg->offset;
  

• etnaviv_context_unmap(context, da, bytes);
  

• da += bytes;
  

• }

• etnaviv_context_unmap(context, iova, da - iova);

I had to take a closer look to figure this out, but AFAICS it does indeed work out as a simpler way of achieving the exact same result, and in fact neatly mirrors how etnaviv_context_map() itself cleans up.

Reviewed-by: Robin Murphy robin.murphy@arm.com

return ret; }

@@ -113,7 +106,7 @@ static void etnaviv_iommu_unmap(struct etnaviv_iommu_context *context, u32 iova, unsigned int da = iova; int i;

• for_each_sg(sgt->sgl, sg, sgt->nents, i) {

• for_each_sgtable_dma_sg(sgt, sg, i) { size_t bytes = sg_dma_len(sg) + sg->offset;

  etnaviv_context_unmap(context, da, bytes);

On 2020-08-26 07:32, Marek Szyprowski wrote:

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function returns the number of the created entries in the DMA address space. However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with the original number of the entries passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous memory buffer, used commonly in the DRM and graphics subsystems. It consists of a scatterlist with memory pages and DMA addresses (sgl entry), as well as the number of scatterlist entries: CPU pages (orig_nents entry) and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents entries, calling DMA-mapping functions with a wrong number of entries or ignoring the number of mapped entries returned by the dma_map_sg() function.

This driver creatively uses sg_table->orig_nents to store the size of the allocated scatterlist and ignores the number of the entries returned by dma_map_sg function. The sg_table->orig_nents is (mis)used to properly free the (over)allocated scatterlist.

This patch only introduces the common DMA-mapping wrappers operating directly on the struct sg_table objects to the dmabuf related functions, so the other drivers, which might share buffers with i915 could rely on the properly set nents and orig_nents values.

This one looks mechanical enough :)

Reviewed-by: Robin Murphy robin.murphy@arm.com

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 11 +++-------- drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c | 7 +++---- 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 2679380159fc..8a988592715b 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -48,12 +48,9 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme src = sg_next(src); }

• if (!dma_map_sg_attrs(attachment->dev,

• 	      st->sgl, st->nents, dir,
  

• 	      DMA_ATTR_SKIP_CPU_SYNC)) {
  

• ret = -ENOMEM;
  

• ret = dma_map_sgtable(attachment->dev, st, dir, DMA_ATTR_SKIP_CPU_SYNC);
• if (ret) goto err_free_sg;

• }

  return st;

@@ -73,9 +70,7 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, { struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf);

• dma_unmap_sg_attrs(attachment->dev,

• 	   sg->sgl, sg->nents, dir,
  

• 	   DMA_ATTR_SKIP_CPU_SYNC);
  

• dma_unmap_sgtable(attachment->dev, sg, dir, DMA_ATTR_SKIP_CPU_SYNC); sg_free_table(sg); kfree(sg);

diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c index debaf7b18ab5..be30b27e2926 100644 --- a/drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c @@ -28,10 +28,9 @@ static struct sg_table *mock_map_dma_buf(struct dma_buf_attachment *attachment, sg = sg_next(sg); }

• if (!dma_map_sg(attachment->dev, st->sgl, st->nents, dir)) {

• err = -ENOMEM;
  

• err = dma_map_sgtable(attachment->dev, st, dir, 0);
• if (err) goto err_st;

• }

  return st;

@@ -46,7 +45,7 @@ static void mock_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *st, enum dma_data_direction dir) {

• dma_unmap_sg(attachment->dev, st->sgl, st->nents, dir);

• dma_unmap_sgtable(attachment->dev, st, dir, 0); sg_free_table(st); kfree(st); }

On 2020-09-01 20:38, Ruhl, Michael J wrote:

...

-----Original Message----- From: Intel-gfx intel-gfx-bounces@lists.freedesktop.org On Behalf Of Marek Szyprowski Sent: Wednesday, August 26, 2020 2:33 AM To: dri-devel@lists.freedesktop.org; iommu@lists.linux-foundation.org; linaro-mm-sig@lists.linaro.org; linux-kernel@vger.kernel.org Cc: Bartlomiej Zolnierkiewicz b.zolnierkie@samsung.com; David Airlie airlied@linux.ie; intel-gfx@lists.freedesktop.org; Robin Murphy robin.murphy@arm.com; Christoph Hellwig hch@lst.de; linux-arm- kernel@lists.infradead.org; Marek Szyprowski m.szyprowski@samsung.com Subject: [Intel-gfx] [PATCH v9 08/32] drm: i915: fix common struct sg_table related issues

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function returns the number of the created entries in the DMA address space. However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with the original number of the entries passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous memory buffer, used commonly in the DRM and graphics subsystems. It consists of a scatterlist with memory pages and DMA addresses (sgl entry), as well as the number of scatterlist entries: CPU pages (orig_nents entry) and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents entries, calling DMA-mapping functions with a wrong number of entries or ignoring the number of mapped entries returned by the dma_map_sg() function.

This driver creatively uses sg_table->orig_nents to store the size of the allocated scatterlist and ignores the number of the entries returned by dma_map_sg function. The sg_table->orig_nents is (mis)used to properly free the (over)allocated scatterlist.

This patch only introduces the common DMA-mapping wrappers operating directly on the struct sg_table objects to the dmabuf related functions, so the other drivers, which might share buffers with i915 could rely on the properly set nents and orig_nents values.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 11 +++-------- drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c | 7 +++---- 2 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c index 2679380159fc..8a988592715b 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c @@ -48,12 +48,9 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme src = sg_next(src); }

• if (!dma_map_sg_attrs(attachment->dev,

• 	      st->sgl, st->nents, dir,
  

• 	      DMA_ATTR_SKIP_CPU_SYNC)) {
  

• ret = -ENOMEM;
  

You have dropped this error value.

Do you now if this is a benign loss?

True, dma_map_sgtable() will return -EINVAL rather than -ENOMEM for failure. A quick look through other .map_dma_buf callbacks suggests they're returning a motley mix of error values and NULL for failure cases, so I'd imagine that importers shouldn't be too sensitive to the exact value.

Robin.

M

...

• ret = dma_map_sgtable(attachment->dev, st, dir,

DMA_ATTR_SKIP_CPU_SYNC);

• if (ret) goto err_free_sg;

• }

  return st;

@@ -73,9 +70,7 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, { struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment-

...

dmabuf);

• dma_unmap_sg_attrs(attachment->dev,

• 	   sg->sgl, sg->nents, dir,
  

• 	   DMA_ATTR_SKIP_CPU_SYNC);
  

• dma_unmap_sgtable(attachment->dev, sg, dir,

DMA_ATTR_SKIP_CPU_SYNC); sg_free_table(sg); kfree(sg);

diff --git a/drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c b/drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c index debaf7b18ab5..be30b27e2926 100644 --- a/drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c +++ b/drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c @@ -28,10 +28,9 @@ static struct sg_table *mock_map_dma_buf(struct dma_buf_attachment *attachment, sg = sg_next(sg); }

• if (!dma_map_sg(attachment->dev, st->sgl, st->nents, dir)) {

• err = -ENOMEM;
  

• err = dma_map_sgtable(attachment->dev, st, dir, 0);
• if (err) goto err_st;

• }

  return st;

@@ -46,7 +45,7 @@ static void mock_unmap_dma_buf(struct dma_buf_attachment *attachment, struct sg_table *st, enum dma_data_direction dir) {

• dma_unmap_sg(attachment->dev, st->sgl, st->nents, dir);

• dma_unmap_sgtable(attachment->dev, st, dir, 0); sg_free_table(st); kfree(st);

}

2.17.1

---

Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx

On 2020-08-26 07:32, Marek Szyprowski wrote:

Use common helper for checking the contiguity of the imported dma-buf and do this check before allocating resources, so the error path is simpler.

Reviewed-by: Robin Murphy robin.murphy@arm.com

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/gpu/drm/mediatek/mtk_drm_gem.c | 28 ++++++-------------------- 1 file changed, 6 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c index 6190cc3b7b0d..3654ec732029 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c @@ -212,37 +212,21 @@ struct drm_gem_object *mtk_gem_prime_import_sg_table(struct drm_device *dev, struct dma_buf_attachment *attach, struct sg_table *sg) { struct mtk_drm_gem_obj *mtk_gem;

• int ret;

• struct scatterlist *s;

• unsigned int i;

• dma_addr_t expected;

• mtk_gem = mtk_drm_gem_init(dev, attach->dmabuf->size);

• /* check if the entries in the sg_table are contiguous */

• if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) {

• DRM_ERROR("sg_table is not contiguous");
  

• return ERR_PTR(-EINVAL);
  

• }

• mtk_gem = mtk_drm_gem_init(dev, attach->dmabuf->size); if (IS_ERR(mtk_gem)) return ERR_CAST(mtk_gem);

• expected = sg_dma_address(sg->sgl);

• for_each_sg(sg->sgl, s, sg->nents, i) {

• if (!sg_dma_len(s))
  

• 	break;
  

• if (sg_dma_address(s) != expected) {
  

• 	DRM_ERROR("sg_table is not contiguous");
  

• 	ret = -EINVAL;
  

• 	goto err_gem_free;
  

• }
  

• expected = sg_dma_address(s) + sg_dma_len(s);
  

• }

• mtk_gem->dma_addr = sg_dma_address(sg->sgl); mtk_gem->sg = sg;

  return &mtk_gem->base;

-err_gem_free:

• kfree(mtk_gem);

• return ERR_PTR(ret); }

  void *mtk_drm_gem_prime_vmap(struct drm_gem_object *obj)

Robin Murphy robin.murphy@arm.com 於 2020年9月2日 週三 上午2:55寫道：

On 2020-08-26 07:32, Marek Szyprowski wrote:

...

Use common helper for converting a sg_table object into struct page pointer array.

Reviewed-by: Robin Murphy robin.murphy@arm.com

Side note: is mtk_drm_gem_prime_vmap() missing a call to sg_free_table(sgt) before its kfree(sgt)?

Yes, we need another patch to fix that bug, But for this patch,

Acked-by: Chun-Kuang Hu chunkuang.hu@kernel.org

...

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/gpu/drm/mediatek/mtk_drm_gem.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c b/drivers/gpu/drm/mediatek/mtk_drm_gem.c index 3654ec732029..0583e557ad37 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c @@ -233,9 +233,7 @@ void *mtk_drm_gem_prime_vmap(struct drm_gem_object *obj) { struct mtk_drm_gem_obj *mtk_gem = to_mtk_gem_obj(obj); struct sg_table *sgt;

• struct sg_page_iter iter;
  unsigned int npages;
  

• unsigned int i = 0;
  
  if (mtk_gem->kvaddr)
          return mtk_gem->kvaddr;
  

@@ -249,11 +247,8 @@ void *mtk_drm_gem_prime_vmap(struct drm_gem_object *obj) if (!mtk_gem->pages) goto out;

• for_each_sg_page(sgt->sgl, &iter, sgt->orig_nents, 0) {
  

•         mtk_gem->pages[i++] = sg_page_iter_page(&iter);
  

•         if (i > npages)
  

•                 break;
  

• }
  

• drm_prime_sg_to_page_addr_arrays(sgt, mtk_gem->pages, NULL, npages);
  

• mtk_gem->kvaddr = vmap(mtk_gem->pages, npages, VM_MAP,
                         pgprot_writecombine(PAGE_KERNEL));
  
  
  

On Tue, Sep 1, 2020 at 12:14 PM Robin Murphy robin.murphy@arm.com wrote:

On 2020-08-26 07:32, Marek Szyprowski wrote:

...

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function returns the number of the created entries in the DMA address space. However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with the original number of the entries passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous memory buffer, used commonly in the DRM and graphics subsystems. It consists of a scatterlist with memory pages and DMA addresses (sgl entry), as well as the number of scatterlist entries: CPU pages (orig_nents entry) and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents entries, calling DMA-mapping functions with a wrong number of entries or ignoring the number of mapped entries returned by the dma_map_sg() function.

To avoid such issues, lets use a common dma-mapping wrappers operating directly on the struct sg_table objects and use scatterlist page iterators where possible. This, almost always, hides references to the nents and orig_nents entries, making the code robust, easier to follow and copy/paste safe.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com Acked-by: Rob Clark robdclark@gmail.com

---

drivers/gpu/drm/msm/msm_gem.c | 13 +++++-------- drivers/gpu/drm/msm/msm_gpummu.c | 14 ++++++-------- drivers/gpu/drm/msm/msm_iommu.c | 2 +- 3 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index b2f49152b4d4..8c7ae812b813 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -53,11 +53,10 @@ static void sync_for_device(struct msm_gem_object *msm_obj) struct device *dev = msm_obj->base.dev->dev;

  if (get_dma_ops(dev) && IS_ENABLED(CONFIG_ARM64)) {

•         dma_sync_sg_for_device(dev, msm_obj->sgt->sgl,
  

•                 msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
  

•         dma_sync_sgtable_for_device(dev, msm_obj->sgt,
  

•                                     DMA_BIDIRECTIONAL);
  } else {
  

•         dma_map_sg(dev, msm_obj->sgt->sgl,
  

•                 msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
  

•         dma_map_sgtable(dev, msm_obj->sgt, DMA_BIDIRECTIONAL, 0);
  }
  

  }

@@ -66,11 +65,9 @@ static void sync_for_cpu(struct msm_gem_object *msm_obj) struct device *dev = msm_obj->base.dev->dev;

  if (get_dma_ops(dev) && IS_ENABLED(CONFIG_ARM64)) {

•         dma_sync_sg_for_cpu(dev, msm_obj->sgt->sgl,
  

•                 msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
  

•         dma_sync_sgtable_for_cpu(dev, msm_obj->sgt, DMA_BIDIRECTIONAL);
  } else {
  

•         dma_unmap_sg(dev, msm_obj->sgt->sgl,
  

•                 msm_obj->sgt->nents, DMA_BIDIRECTIONAL);
  

•         dma_unmap_sgtable(dev, msm_obj->sgt, DMA_BIDIRECTIONAL, 0);
  }
  

  }

diff --git a/drivers/gpu/drm/msm/msm_gpummu.c b/drivers/gpu/drm/msm/msm_gpummu.c index 310a31b05faa..319f06c28235 100644 --- a/drivers/gpu/drm/msm/msm_gpummu.c +++ b/drivers/gpu/drm/msm/msm_gpummu.c @@ -30,21 +30,19 @@ static int msm_gpummu_map(struct msm_mmu *mmu, uint64_t iova, { struct msm_gpummu *gpummu = to_msm_gpummu(mmu); unsigned idx = (iova - GPUMMU_VA_START) / GPUMMU_PAGE_SIZE;

• struct scatterlist *sg;
  

• struct sg_dma_page_iter dma_iter;
  unsigned prot_bits = 0;
  

• unsigned i, j;
  
  if (prot & IOMMU_WRITE)
          prot_bits |= 1;
  if (prot & IOMMU_READ)
          prot_bits |= 2;
  
  

• for_each_sg(sgt->sgl, sg, sgt->nents, i) {
  

•         dma_addr_t addr = sg->dma_address;
  

•         for (j = 0; j < sg->length / GPUMMU_PAGE_SIZE; j++, idx++) {
  

•                 gpummu->table[idx] = addr | prot_bits;
  

•                 addr += GPUMMU_PAGE_SIZE;
  

•         }
  

• for_each_sgtable_dma_page(sgt, &dma_iter, 0) {
  

•         dma_addr_t addr = sg_page_iter_dma_address(&dma_iter);
  

•         BUILD_BUG_ON(GPUMMU_PAGE_SIZE != PAGE_SIZE);
  

•         gpummu->table[idx++] = addr | prot_bits;
  

Given that the BUILD_BUG_ON might prevent valid arm64 configs from building, how about a simple tweak like:

            for (i = 0; i < PAGE_SIZE; i += GPUMMU_PAGE_SIZE)
                    gpummu->table[idx++] = i + addr | prot_bits;

?

Or alternatively perhaps some more aggressive #ifdefs or makefile tweaks to prevent the GPUMMU code building for arm64 at all if it's only relevant to 32-bit platforms (which I believe might be the case).

yes, the gpummu path is only used on older armv7 snapdragon and imx5 platforms.. I suppose maybe the easy thing would be to add a stub for msm_gpummu_new() and msm_gpummu_params() and then only build it on arm64?

BR, -R

Robin.

...

  }

  /* we can improve by deferring flush for multiple map() */

diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c index 3a381a9674c9..6c31e65834c6 100644 --- a/drivers/gpu/drm/msm/msm_iommu.c +++ b/drivers/gpu/drm/msm/msm_iommu.c @@ -36,7 +36,7 @@ static int msm_iommu_map(struct msm_mmu *mmu, uint64_t iova, struct msm_iommu *iommu = to_msm_iommu(mmu); size_t ret;

• ret = iommu_map_sg(iommu->domain, iova, sgt->sgl, sgt->nents, prot);
  

• ret = iommu_map_sgtable(iommu->domain, iova, sgt, prot);
  WARN_ON(!ret);
  
  return (ret == len) ? 0 : -EINVAL;
  
  

On 2020-08-26 07:32, Marek Szyprowski wrote:

Use common helper for converting a sg_table object into struct page pointer array.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/gpu/drm/omapdrm/omap_gem.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index d0d12d5dd76c..ff0c4b0c3fd0 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -1297,10 +1297,9 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, omap_obj->dma_addr = sg_dma_address(sgt->sgl); } else { /* Create pages list from sgt */

• struct sg_page_iter iter;
  

  struct page **pages; unsigned int npages;

• unsigned int i = 0;
  

• unsigned int ret;
  
  

  npages = DIV_ROUND_UP(size, PAGE_SIZE); pages = kcalloc(npages, sizeof(*pages), GFP_KERNEL);

@@ -1311,14 +1310,9 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, }

omap_obj->pages = pages;

• for_each_sg_page(sgt->sgl, &iter, sgt->orig_nents, 0) {
  

• 	pages[i++] = sg_page_iter_page(&iter);
  

• 	if (i > npages)
  

• 		break;
  

• }
  

• if (WARN_ON(i != npages)) {
  

• ret = drm_prime_sg_to_page_addr_arrays(sgt, pages, NULL,
  

• 				       npages);
  

• if (WARN_ON(ret)) {
  

Again, I'm inclined to think the WARN_ON should remain in drm_prime_sg_to_page_addr_arrays() itself such that it could be removed here, but either way,

Reviewed-by: Robin Murphy robin.murphy@arm.com

	omap_gem_free_object(obj);
	obj = ERR_PTR(-ENOMEM);
	goto done;

On 2020-08-26 07:32, Marek Szyprowski wrote:

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function returns the number of the created entries in the DMA address space. However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with the original number of the entries passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous memory buffer, used commonly in the DRM and graphics subsystems. It consists of a scatterlist with memory pages and DMA addresses (sgl entry), as well as the number of scatterlist entries: CPU pages (orig_nents entry) and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents entries, calling DMA-mapping functions with a wrong number of entries or ignoring the number of mapped entries returned by the dma_map_sg() function.

Fix the code to refer to proper nents or orig_nents entries. This driver checks for a buffer contiguity in DMA address space, so it should test sg_table->nents entry.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/gpu/drm/omapdrm/omap_gem.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index ff0c4b0c3fd0..a7a9a0afe2b6 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -48,7 +48,7 @@ struct omap_gem_object { * OMAP_BO_MEM_DMA_API flag set) * * - buffers imported from dmabuf (with the OMAP_BO_MEM_DMABUF flag set)

• *   if they are physically contiguous (when sgt->orig_nents == 1)
  

• *   if they are physically contiguous (when sgt->nents == 1)
  

Hmm, if this really does mean *physically* contiguous - i.e. if buffers might be shared between DMA-translatable and non-DMA-translatable devices - then these changes might not be appropriate. If not and it only actually means DMA-contiguous, then it would be good to clarify the comments to that effect.

Can anyone familiar with omapdrm clarify what exactly the case is here? I know that IOMMUs might be involved to some degree, and I've skimmed the interconnect chapters of enough OMAP TRMs to be scared by the reference to the tiler aperture in the context below :)

Robin.

*
* - buffers mapped through the TILER when dma_addr_cnt is not zero, in
*   which case the DMA address points to the TILER aperture

@@ -1279,7 +1279,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size, union omap_gem_size gsize;

/* Without a DMM only physically contiguous buffers can be supported. */

• if (sgt->orig_nents != 1 && !priv->has_dmm)

• if (sgt->nents != 1 && !priv->has_dmm) return ERR_PTR(-EINVAL);

  gsize.bytes = PAGE_ALIGN(size);

@@ -1293,7 +1293,7 @@ struct drm_gem_object *omap_gem_new_dmabuf(struct drm_device *dev, size_t size,

omap_obj->sgt = sgt;

• if (sgt->orig_nents == 1) {

• if (sgt->nents == 1) { omap_obj->dma_addr = sg_dma_address(sgt->sgl); } else { /* Create pages list from sgt */

Hi Tomi,

On 02.09.2020 10:00, Tomi Valkeinen wrote:

On 01/09/2020 22:33, Robin Murphy wrote:

...

On 2020-08-26 07:32, Marek Szyprowski wrote:

...

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function returns the number of the created entries in the DMA address space. However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with the original number of the entries passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous memory buffer, used commonly in the DRM and graphics subsystems. It consists of a scatterlist with memory pages and DMA addresses (sgl entry), as well as the number of scatterlist entries: CPU pages (orig_nents entry) and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents entries, calling DMA-mapping functions with a wrong number of entries or ignoring the number of mapped entries returned by the dma_map_sg() function.

Fix the code to refer to proper nents or orig_nents entries. This driver checks for a buffer contiguity in DMA address space, so it should test sg_table->nents entry.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/gpu/drm/omapdrm/omap_gem.c | 6 +++---   1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index ff0c4b0c3fd0..a7a9a0afe2b6 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -48,7 +48,7 @@ struct omap_gem_object {        *   OMAP_BO_MEM_DMA_API flag set)        *        * - buffers imported from dmabuf (with the OMAP_BO_MEM_DMABUF flag set) -     *   if they are physically contiguous (when sgt->orig_nents == 1) +     *   if they are physically contiguous (when sgt->nents == 1)

Hmm, if this really does mean *physically* contiguous - i.e. if buffers might be shared between DMA-translatable and non-DMA-translatable devices - then these changes might not be appropriate. If not and it only actually means DMA-contiguous, then it would be good to clarify the comments to that effect.

Can anyone familiar with omapdrm clarify what exactly the case is here? I know that IOMMUs might be involved to some degree, and I've skimmed the interconnect chapters of enough OMAP TRMs to be scared by the reference to the tiler aperture in the context below :)

DSS (like many other IPs in OMAP) does not have any MMU/PAT, and can only use contiguous buffers (contiguous in the RAM).

There's a special case with TILER (which is not part of DSS but of the memory subsystem, but it's still handled internally by the omapdrm driver), which has a PAT. PAT can create a contiguous view of scattered pages, and DSS can then use this contiguous view ("tiler aperture", which to DSS looks just like normal contiguous memory).

Note that omapdrm does not use dma_map_sg() & co. mentioned in the patch description.

If there's no MMU/PAT, is orig_nents always the same as nents? Or can we have multiple physically contiguous pages listed separately in the sgt (so orig_nents > 1) but as the pages form one big contiguous area, nents == 1?

Well, when DMA-mapping API is properly used, the difference between nents and orig_nents is only when the scatterlist have been mapped for DMA.

For the mentioned case, even if the creator of the buffer used only the pages that are consecutive in the physical memory, he is free to chose either to set nents/orig_nents to 1 and length to n*PAGE_SIZE or set nents/orig_nents to n and length to PAGE_SIZE for each. Then the buffer chunks might be merged, but this is done by the DMA-mapping code. For your case, without any call to DMA-mapping, you can only assume that the buffer is contiguous in physical memory if orig_nents is 1.

I've changed the use of nents to orig_nents to make things consistent - this code operates only on the unmapped buffers. I want to ensure that anyone who will potentially copy this code, won't make the nents/orig_nents mistake in the future.

If you don't like it, we can drop this patch, because it won't change the way the driver works.

Best regards

On 2020-08-26 07:33, Marek Szyprowski wrote:

Use common helper for checking the contiguity of the imported dma-buf.

Reviewed-by: Robin Murphy robin.murphy@arm.com

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c index b9275ba7c5a5..2970e534e2bb 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c @@ -460,23 +460,6 @@ struct sg_table *rockchip_gem_prime_get_sg_table(struct drm_gem_object *obj) return sgt; }

-static unsigned long rockchip_sg_get_contiguous_size(struct sg_table *sgt,

• 				     int count)
  

-{

• struct scatterlist *s;
• dma_addr_t expected = sg_dma_address(sgt->sgl);
• unsigned int i;
• unsigned long size = 0;
• for_each_sg(sgt->sgl, s, count, i) {

• if (sg_dma_address(s) != expected)
  

• 	break;
  

• expected = sg_dma_address(s) + sg_dma_len(s);
  

• size += sg_dma_len(s);
  

• }
• return size;

-}

• static int rockchip_gem_iommu_map_sg(struct drm_device *drm, struct dma_buf_attachment *attach,

@@ -498,7 +481,7 @@ rockchip_gem_dma_map_sg(struct drm_device *drm, if (!count) return -EINVAL;

• if (rockchip_sg_get_contiguous_size(sg, count) < attach->dmabuf->size) {

• if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) { DRM_ERROR("failed to map sg_table to contiguous linear address.\n"); dma_unmap_sg(drm->dev, sg->sgl, sg->nents, DMA_BIDIRECTIONAL);

On 2020-08-26 07:33, Marek Szyprowski wrote:

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function returns the number of the created entries in the DMA address space. However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with the original number of the entries passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous memory buffer, used commonly in the DRM and graphics subsystems. It consists of a scatterlist with memory pages and DMA addresses (sgl entry), as well as the number of scatterlist entries: CPU pages (orig_nents entry) and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents entries, calling DMA-mapping functions with a wrong number of entries or ignoring the number of mapped entries returned by the dma_map_sg() function.

To avoid such issues, lets use a common dma-mapping wrappers operating directly on the struct sg_table objects and use scatterlist page iterators where possible. This, almost always, hides references to the nents and orig_nents entries, making the code robust, easier to follow and copy/paste safe.

Reviewed-by: Robin Murphy robin.murphy@arm.com

(Until now I hadn't noticed the crimes against the API that rockchip_gem_get_pages() is committing, but it's not this patch's fault... I'll have to take a closer look at that)

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 23 +++++++++------------ 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c index 2970e534e2bb..cb50f2ba2e46 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c @@ -36,8 +36,8 @@ static int rockchip_gem_iommu_map(struct rockchip_gem_object *rk_obj)

rk_obj->dma_addr = rk_obj->mm.start;

• ret = iommu_map_sg(private->domain, rk_obj->dma_addr, rk_obj->sgt->sgl,

• 	   rk_obj->sgt->nents, prot);
  

• ret = iommu_map_sgtable(private->domain, rk_obj->dma_addr, rk_obj->sgt,

• 		prot);
  

  if (ret < rk_obj->base.size) { DRM_ERROR("failed to map buffer: size=%zd request_size=%zd\n", ret, rk_obj->base.size);

@@ -98,11 +98,10 @@ static int rockchip_gem_get_pages(struct rockchip_gem_object *rk_obj) * TODO: Replace this by drm_clflush_sg() once it can be implemented * without relying on symbols that are not exported. */

• for_each_sg(rk_obj->sgt->sgl, s, rk_obj->sgt->nents, i)

• for_each_sgtable_sg(rk_obj->sgt, s, i) sg_dma_address(s) = sg_phys(s);

• dma_sync_sg_for_device(drm->dev, rk_obj->sgt->sgl, rk_obj->sgt->nents,

• 	       DMA_TO_DEVICE);
  

• dma_sync_sgtable_for_device(drm->dev, rk_obj->sgt, DMA_TO_DEVICE);

  return 0;

@@ -350,8 +349,8 @@ void rockchip_gem_free_object(struct drm_gem_object *obj) if (private->domain) { rockchip_gem_iommu_unmap(rk_obj); } else {

• 	dma_unmap_sg(drm->dev, rk_obj->sgt->sgl,
  

• 		     rk_obj->sgt->nents, DMA_BIDIRECTIONAL);
  

• 	dma_unmap_sgtable(drm->dev, rk_obj->sgt,
  

• 			  DMA_BIDIRECTIONAL, 0);
  

  } drm_prime_gem_destroy(obj, rk_obj->sgt); } else {

@@ -476,15 +475,13 @@ rockchip_gem_dma_map_sg(struct drm_device *drm, struct sg_table *sg, struct rockchip_gem_object *rk_obj) {

• int count = dma_map_sg(drm->dev, sg->sgl, sg->nents,

• 	       DMA_BIDIRECTIONAL);
  

• if (!count)

• return -EINVAL;
  

• int err = dma_map_sgtable(drm->dev, sg, DMA_BIDIRECTIONAL, 0);

• if (err)

• return err;
  
  

  if (drm_prime_get_contiguous_size(sg) < attach->dmabuf->size) { DRM_ERROR("failed to map sg_table to contiguous linear address.\n");

• dma_unmap_sg(drm->dev, sg->sgl, sg->nents,
  

• 	     DMA_BIDIRECTIONAL);
  

• dma_unmap_sgtable(drm->dev, sg, DMA_BIDIRECTIONAL, 0);
  

  return -EINVAL; }

On 2020-08-26 07:33, Marek Szyprowski wrote:

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function returns the number of the created entries in the DMA address space. However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with the original number of the entries passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous memory buffer, used commonly in the DRM and graphics subsystems. It consists of a scatterlist with memory pages and DMA addresses (sgl entry), as well as the number of scatterlist entries: CPU pages (orig_nents entry) and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents entries, calling DMA-mapping functions with a wrong number of entries or ignoring the number of mapped entries returned by the dma_map_sg() function.

To avoid such issues, lets use a common dma-mapping wrappers operating directly on the struct sg_table objects and use scatterlist page iterators where possible. This, almost always, hides references to the nents and orig_nents entries, making the code robust, easier to follow and copy/paste safe.

Reviewed-by: Robin Murphy robin.murphy@arm.com

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/gpu/drm/tegra/gem.c | 27 ++++++++++----------------- drivers/gpu/drm/tegra/plane.c | 15 +++++---------- 2 files changed, 15 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index 723df142a981..01d94befab11 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -98,8 +98,8 @@ static struct sg_table *tegra_bo_pin(struct device *dev, struct host1x_bo *bo, * the SG table needs to be copied to avoid overwriting any * other potential users of the original SG table. */

• err = sg_alloc_table_from_sg(sgt, obj->sgt->sgl, obj->sgt->nents,
  

• 			     GFP_KERNEL);
  

• err = sg_alloc_table_from_sg(sgt, obj->sgt->sgl,
  

• 			     obj->sgt->orig_nents, GFP_KERNEL);
  

  if (err < 0) goto free; } else {

@@ -196,8 +196,7 @@ static int tegra_bo_iommu_map(struct tegra_drm *tegra, struct tegra_bo *bo)

bo->iova = bo->mm->start;

• bo->size = iommu_map_sg(tegra->domain, bo->iova, bo->sgt->sgl,

• 		bo->sgt->nents, prot);
  

• bo->size = iommu_map_sgtable(tegra->domain, bo->iova, bo->sgt, prot); if (!bo->size) { dev_err(tegra->drm->dev, "failed to map buffer\n"); err = -ENOMEM;

@@ -264,8 +263,7 @@ static struct tegra_bo *tegra_bo_alloc_object(struct drm_device *drm, static void tegra_bo_free(struct drm_device *drm, struct tegra_bo *bo) { if (bo->pages) {

• dma_unmap_sg(drm->dev, bo->sgt->sgl, bo->sgt->nents,
  

• 	     DMA_FROM_DEVICE);
  

• dma_unmap_sgtable(drm->dev, bo->sgt, DMA_FROM_DEVICE, 0);
  

  drm_gem_put_pages(&bo->gem, bo->pages, true, true); sg_free_table(bo->sgt); kfree(bo->sgt);

@@ -290,12 +288,9 @@ static int tegra_bo_get_pages(struct drm_device *drm, struct tegra_bo *bo) goto put_pages; }

• err = dma_map_sg(drm->dev, bo->sgt->sgl, bo->sgt->nents,

• 	 DMA_FROM_DEVICE);
  

• if (err == 0) {

• err = -EFAULT;
  

• err = dma_map_sgtable(drm->dev, bo->sgt, DMA_FROM_DEVICE, 0);
• if (err) goto free_sgt;

• }

  return 0;

@@ -571,7 +566,7 @@ tegra_gem_prime_map_dma_buf(struct dma_buf_attachment *attach, goto free; }

• if (dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir) == 0)

• if (dma_map_sgtable(attach->dev, sgt, dir, 0)) goto free;

  return sgt;

@@ -590,7 +585,7 @@ static void tegra_gem_prime_unmap_dma_buf(struct dma_buf_attachment *attach, struct tegra_bo *bo = to_tegra_bo(gem);

if (bo->pages)

• dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
  

• dma_unmap_sgtable(attach->dev, sgt, dir, 0);
  
  

  sg_free_table(sgt); kfree(sgt);

@@ -609,8 +604,7 @@ static int tegra_gem_prime_begin_cpu_access(struct dma_buf *buf, struct drm_device *drm = gem->dev;

if (bo->pages)

• dma_sync_sg_for_cpu(drm->dev, bo->sgt->sgl, bo->sgt->nents,
  

• 		    DMA_FROM_DEVICE);
  

• dma_sync_sgtable_for_cpu(drm->dev, bo->sgt, DMA_FROM_DEVICE);
  
  

  return 0; }

@@ -623,8 +617,7 @@ static int tegra_gem_prime_end_cpu_access(struct dma_buf *buf, struct drm_device *drm = gem->dev;

if (bo->pages)

• dma_sync_sg_for_device(drm->dev, bo->sgt->sgl, bo->sgt->nents,
  

• 		       DMA_TO_DEVICE);
  

• dma_sync_sgtable_for_device(drm->dev, bo->sgt, DMA_TO_DEVICE);
  
  

  return 0; }

diff --git a/drivers/gpu/drm/tegra/plane.c b/drivers/gpu/drm/tegra/plane.c index 4cd0461cc508..539d14935728 100644 --- a/drivers/gpu/drm/tegra/plane.c +++ b/drivers/gpu/drm/tegra/plane.c @@ -131,12 +131,9 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct tegra_plane_state *state) }

if (sgt) {

• 	err = dma_map_sg(dc->dev, sgt->sgl, sgt->nents,
  

• 			 DMA_TO_DEVICE);
  

• 	if (err == 0) {
  

• 		err = -ENOMEM;
  

• 	err = dma_map_sgtable(dc->dev, sgt, DMA_TO_DEVICE, 0);
  

• 	if (err)
  	goto unpin;
  

• 	}
  
  /*
   * The display controller needs contiguous memory, so
  

@@ -144,7 +141,7 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct tegra_plane_state *state) * map its SG table to a single contiguous chunk of * I/O virtual memory. */

• 	if (err > 1) {
  

• 	if (sgt->nents > 1) {
  	err = -EINVAL;
  	goto unpin;
  }
  

@@ -166,8 +163,7 @@ static int tegra_dc_pin(struct tegra_dc *dc, struct tegra_plane_state *state) struct sg_table *sgt = state->sgt[i];

if (sgt)

• 	dma_unmap_sg(dc->dev, sgt->sgl, sgt->nents,
  

• 		     DMA_TO_DEVICE);
  

• 	dma_unmap_sgtable(dc->dev, sgt, DMA_TO_DEVICE, 0);
  
  

  host1x_bo_unpin(dc->dev, &bo->base, sgt); state->iova[i] = DMA_MAPPING_ERROR;

@@ -186,8 +182,7 @@ static void tegra_dc_unpin(struct tegra_dc *dc, struct tegra_plane_state *state) struct sg_table *sgt = state->sgt[i];

if (sgt)

• 	dma_unmap_sg(dc->dev, sgt->sgl, sgt->nents,
  

• 		     DMA_TO_DEVICE);
  

• 	dma_unmap_sgtable(dc->dev, sgt, DMA_TO_DEVICE, 0);
  
  

  host1x_bo_unpin(dc->dev, &bo->base, sgt); state->iova[i] = DMA_MAPPING_ERROR;

On 2020-08-26 07:33, Marek Szyprowski wrote:

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function returns the number of the created entries in the DMA address space. However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with the original number of the entries passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous memory buffer, used commonly in the DRM and graphics subsystems. It consists of a scatterlist with memory pages and DMA addresses (sgl entry), as well as the number of scatterlist entries: CPU pages (orig_nents entry) and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents entries, calling DMA-mapping functions with a wrong number of entries or ignoring the number of mapped entries returned by the dma_map_sg() function.

To avoid such issues, lets use a common dma-mapping wrappers operating directly on the struct sg_table objects and use scatterlist page iterators where possible. This, almost always, hides references to the nents and orig_nents entries, making the code robust, easier to follow and copy/paste safe.

Reviewed-by: Robin Murphy robin.murphy@arm.com

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/misc/fastrpc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c index 7939c55daceb..9d6867749316 100644 --- a/drivers/misc/fastrpc.c +++ b/drivers/misc/fastrpc.c @@ -518,7 +518,7 @@ fastrpc_map_dma_buf(struct dma_buf_attachment *attachment,

table = &a->sgt;

• if (!dma_map_sg(attachment->dev, table->sgl, table->nents, dir))

• if (!dma_map_sgtable(attachment->dev, table, dir, 0)) return ERR_PTR(-ENOMEM);

  return table;

@@ -528,7 +528,7 @@ static void fastrpc_unmap_dma_buf(struct dma_buf_attachment *attach, struct sg_table *table, enum dma_data_direction dir) {

• dma_unmap_sg(attach->dev, table->sgl, table->nents, dir);

• dma_unmap_sgtable(attach->dev, table, dir, 0); }

  static void fastrpc_release(struct dma_buf *dmabuf)

On 2020-08-26 07:33, Marek Szyprowski wrote:

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function returns the number of the created entries in the DMA address space. However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with the original number of the entries passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous memory buffer, used commonly in the DRM and graphics subsystems. It consists of a scatterlist with memory pages and DMA addresses (sgl entry), as well as the number of scatterlist entries: CPU pages (orig_nents entry) and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents entries, calling DMA-mapping functions with a wrong number of entries or ignoring the number of mapped entries returned by the dma_map_sg() function.

To avoid such issues, lets use a common dma-mapping wrappers operating directly on the struct sg_table objects and use scatterlist page iterators where possible. This, almost always, hides references to the nents and orig_nents entries, making the code robust, easier to follow and copy/paste safe.

Reviewed-by: Robin Murphy robin.murphy@arm.com

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/rapidio/devices/rio_mport_cdev.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/rapidio/devices/rio_mport_cdev.c b/drivers/rapidio/devices/rio_mport_cdev.c index a30342942e26..89eb3d212652 100644 --- a/drivers/rapidio/devices/rio_mport_cdev.c +++ b/drivers/rapidio/devices/rio_mport_cdev.c @@ -573,8 +573,7 @@ static void dma_req_free(struct kref *ref) refcount); struct mport_cdev_priv *priv = req->priv;

• dma_unmap_sg(req->dmach->device->dev,

•      req->sgt.sgl, req->sgt.nents, req->dir);
  

• dma_unmap_sgtable(req->dmach->device->dev, &req->sgt, req->dir, 0); sg_free_table(&req->sgt); if (req->page_list) { unpin_user_pages(req->page_list, req->nr_pages);

@@ -814,7 +813,6 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode, struct mport_dev *md = priv->md; struct dma_chan *chan; int ret;

• int nents;

  if (xfer->length == 0) return -EINVAL;

@@ -930,15 +928,14 @@ rio_dma_transfer(struct file *filp, u32 transfer_mode, xfer->offset, xfer->length); }

• nents = dma_map_sg(chan->device->dev,

• 	   req->sgt.sgl, req->sgt.nents, dir);
  

• if (nents == 0) {

• ret = dma_map_sgtable(chan->device->dev, &req->sgt, dir, 0);
• if (ret) { rmcd_error("Failed to map SG list"); ret = -EFAULT; goto err_pg; }

• ret = do_dma_request(req, xfer, sync, nents);

• ret = do_dma_request(req, xfer, sync, req->sgt.nents);

  if (ret >= 0) { if (sync == RIO_TRANSFER_ASYNC)

On 2020-08-26 07:33, Marek Szyprowski wrote:

The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function returns the number of the created entries in the DMA address space. However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and dma_unmap_sg must be called with the original number of the entries passed to the dma_map_sg().

struct sg_table is a common structure used for describing a non-contiguous memory buffer, used commonly in the DRM and graphics subsystems. It consists of a scatterlist with memory pages and DMA addresses (sgl entry), as well as the number of scatterlist entries: CPU pages (orig_nents entry) and DMA mapped pages (nents entry).

It turned out that it was a common mistake to misuse nents and orig_nents entries, calling DMA-mapping functions with a wrong number of entries or ignoring the number of mapped entries returned by the dma_map_sg() function.

To avoid such issues, lets use a common dma-mapping wrappers operating directly on the struct sg_table objects and use scatterlist page iterators where possible. This, almost always, hides references to the nents and orig_nents entries, making the code robust, easier to follow and copy/paste safe.

While touching this code, also add missing call to dma_unmap_sgtable.

Reviewed-by: Robin Murphy robin.murphy@arm.com

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

samples/vfio-mdev/mbochs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c index 3cc5e5921682..e03068917273 100644 --- a/samples/vfio-mdev/mbochs.c +++ b/samples/vfio-mdev/mbochs.c @@ -846,7 +846,7 @@ static struct sg_table *mbochs_map_dmabuf(struct dma_buf_attachment *at, if (sg_alloc_table_from_pages(sg, dmabuf->pages, dmabuf->pagecount, 0, dmabuf->mode.size, GFP_KERNEL) < 0) goto err2;

• if (!dma_map_sg(at->dev, sg->sgl, sg->nents, direction))

• if (dma_map_sgtable(at->dev, sg, direction, 0)) goto err3;

  return sg;

@@ -868,6 +868,7 @@ static void mbochs_unmap_dmabuf(struct dma_buf_attachment *at,

dev_dbg(dev, "%s: %d\n", __func__, dmabuf->id);

• dma_unmap_sgtable(at->dev, sg, direction, 0); sg_free_table(sg); kfree(sg); }

On 2020-08-26 07:33, Marek Szyprowski wrote:

The Documentation/DMA-API-HOWTO.txt states that dma_map_sg returns the numer of the created entries in the DMA address space. However the subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg must be called with the original number of entries passed to dma_map_sg. The sg_table->nents in turn holds the result of the dma_map_sg call as stated in include/linux/scatterlist.h. Adapt the code to obey those rules.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

drivers/media/pci/cx23885/cx23885-alsa.c | 2 +- drivers/media/pci/cx25821/cx25821-alsa.c | 2 +- drivers/media/pci/cx88/cx88-alsa.c | 2 +- drivers/media/pci/saa7134/saa7134-alsa.c | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/pci/cx23885/cx23885-alsa.c b/drivers/media/pci/cx23885/cx23885-alsa.c index df44ed7393a0..3f366e4e4685 100644 --- a/drivers/media/pci/cx23885/cx23885-alsa.c +++ b/drivers/media/pci/cx23885/cx23885-alsa.c @@ -129,7 +129,7 @@ static int cx23885_alsa_dma_unmap(struct cx23885_audio_dev *dev) if (!buf->sglen) return 0;

• dma_unmap_sg(&dev->pci->dev, buf->sglist, buf->sglen, PCI_DMA_FROMDEVICE);

• dma_unmap_sg(&dev->pci->dev, buf->sglist, buf->nr_pages, PCI_DMA_FROMDEVICE);

If we're touching these lines anyway, we should update them to use the modern DMA_FROM_DEVICE definitions too.

Robin.

buf->sglen = 0; return 0; } diff --git a/drivers/media/pci/cx25821/cx25821-alsa.c b/drivers/media/pci/cx25821/cx25821-alsa.c index 301616426d8a..c40304d33776 100644 --- a/drivers/media/pci/cx25821/cx25821-alsa.c +++ b/drivers/media/pci/cx25821/cx25821-alsa.c @@ -193,7 +193,7 @@ static int cx25821_alsa_dma_unmap(struct cx25821_audio_dev *dev) if (!buf->sglen) return 0;

• dma_unmap_sg(&dev->pci->dev, buf->sglist, buf->sglen, PCI_DMA_FROMDEVICE);

• dma_unmap_sg(&dev->pci->dev, buf->sglist, buf->nr_pages, PCI_DMA_FROMDEVICE); buf->sglen = 0; return 0; }

diff --git a/drivers/media/pci/cx88/cx88-alsa.c b/drivers/media/pci/cx88/cx88-alsa.c index 7d7aceecc985..3c6fe6ceb0b7 100644 --- a/drivers/media/pci/cx88/cx88-alsa.c +++ b/drivers/media/pci/cx88/cx88-alsa.c @@ -332,7 +332,7 @@ static int cx88_alsa_dma_unmap(struct cx88_audio_dev *dev) if (!buf->sglen) return 0;

• dma_unmap_sg(&dev->pci->dev, buf->sglist, buf->sglen,

• dma_unmap_sg(&dev->pci->dev, buf->sglist, buf->nr_pages, PCI_DMA_FROMDEVICE); buf->sglen = 0; return 0;

diff --git a/drivers/media/pci/saa7134/saa7134-alsa.c b/drivers/media/pci/saa7134/saa7134-alsa.c index 544ca57eee75..398c47ff473d 100644 --- a/drivers/media/pci/saa7134/saa7134-alsa.c +++ b/drivers/media/pci/saa7134/saa7134-alsa.c @@ -313,7 +313,7 @@ static int saa7134_alsa_dma_unmap(struct saa7134_dev *dev) if (!dma->sglen) return 0;

• dma_unmap_sg(&dev->pci->dev, dma->sglist, dma->sglen, PCI_DMA_FROMDEVICE);

• dma_unmap_sg(&dev->pci->dev, dma->sglist, dma->nr_pages, PCI_DMA_FROMDEVICE); dma->sglen = 0; return 0; }

On 2020-08-26 07:33, Marek Szyprowski wrote:

Use recently introduced common wrappers operating directly on the struct sg_table objects and scatterlist page iterators to make the code a bit more compact, robust, easier to follow and copy/paste safe.

No functional change, because the code already properly did all the scaterlist related calls.

^^ typo

Otherwise,

Reviewed-by: Robin Murphy robin.murphy@arm.com

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com

.../common/videobuf2/videobuf2-dma-contig.c | 34 ++++++++----------- .../media/common/videobuf2/videobuf2-dma-sg.c | 32 +++++++---------- .../common/videobuf2/videobuf2-vmalloc.c | 12 +++---- 3 files changed, 31 insertions(+), 47 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c index ec3446cc45b8..1b242d844dde 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c @@ -58,10 +58,10 @@ static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt) unsigned int i; unsigned long size = 0;

• for_each_sg(sgt->sgl, s, sgt->nents, i) {

• for_each_sgtable_dma_sg(sgt, s, i) { if (sg_dma_address(s) != expected) break;

• expected = sg_dma_address(s) + sg_dma_len(s);
  

• expected += sg_dma_len(s);
  

  size += sg_dma_len(s); } return size;

@@ -103,8 +103,7 @@ static void vb2_dc_prepare(void *buf_priv) if (!sgt) return;

• dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents,

• 	       buf->dma_dir);
  

• dma_sync_sgtable_for_device(buf->dev, sgt, buf->dma_dir); }

  static void vb2_dc_finish(void *buf_priv)

@@ -115,7 +114,7 @@ static void vb2_dc_finish(void *buf_priv) if (!sgt) return;

• dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);

• dma_sync_sgtable_for_cpu(buf->dev, sgt, buf->dma_dir); }

  /*********************************************/

@@ -275,8 +274,8 @@ static void vb2_dc_dmabuf_ops_detach(struct dma_buf *dbuf, * memory locations do not require any explicit cache * maintenance prior or after being used by the device. */

• dma_unmap_sg_attrs(db_attach->dev, sgt->sgl, sgt->orig_nents,
  

• 		   attach->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
  

• dma_unmap_sgtable(db_attach->dev, sgt, attach->dma_dir,
  

• 		  DMA_ATTR_SKIP_CPU_SYNC);
  

  sg_free_table(sgt); kfree(attach); db_attach->priv = NULL;

@@ -301,8 +300,8 @@ static struct sg_table *vb2_dc_dmabuf_ops_map(

/* release any previous cache */ if (attach->dma_dir != DMA_NONE) {

• dma_unmap_sg_attrs(db_attach->dev, sgt->sgl, sgt->orig_nents,
  

• 		   attach->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
  

• dma_unmap_sgtable(db_attach->dev, sgt, attach->dma_dir,
  

• 		  DMA_ATTR_SKIP_CPU_SYNC);
  

  attach->dma_dir = DMA_NONE; }

@@ -310,9 +309,8 @@ static struct sg_table *vb2_dc_dmabuf_ops_map( * mapping to the client with new direction, no cache sync * required see comment in vb2_dc_dmabuf_ops_detach() */

• sgt->nents = dma_map_sg_attrs(db_attach->dev, sgt->sgl, sgt->orig_nents,

• 		      dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
  

• if (!sgt->nents) {

• if (dma_map_sgtable(db_attach->dev, sgt, dma_dir,

• 	    DMA_ATTR_SKIP_CPU_SYNC)) {
  

  pr_err("failed to map scatterlist\n"); mutex_unlock(lock); return ERR_PTR(-EIO);

@@ -455,8 +453,8 @@ static void vb2_dc_put_userptr(void *buf_priv) * No need to sync to CPU, it's already synced to the CPU * since the finish() memop will have been called before this. */

• dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
  

• 		   buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
  

• dma_unmap_sgtable(buf->dev, sgt, buf->dma_dir,
  

• 		  DMA_ATTR_SKIP_CPU_SYNC);
  

  pages = frame_vector_pages(buf->vec); /* sgt should exist only if vector contains pages... */ BUG_ON(IS_ERR(pages));

@@ -553,9 +551,8 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr, * No need to sync to the device, this will happen later when the * prepare() memop is called. */

• sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,

• 		      buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
  

• if (sgt->nents <= 0) {

• if (dma_map_sgtable(buf->dev, sgt, buf->dma_dir,

• 	    DMA_ATTR_SKIP_CPU_SYNC)) {
  

  pr_err("failed to map scatterlist\n"); ret = -EIO; goto fail_sgt_init;

@@ -577,8 +574,7 @@ static void *vb2_dc_get_userptr(struct device *dev, unsigned long vaddr, return buf;

fail_map_sg:

• dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,

• 	   buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
  

• dma_unmap_sgtable(buf->dev, sgt, buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);

  fail_sgt_init: sg_free_table(sgt);

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index 0a40e00f0d7e..0dd3b19025e0 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c @@ -148,9 +148,8 @@ static void *vb2_dma_sg_alloc(struct device *dev, unsigned long dma_attrs, * No need to sync to the device, this will happen later when the * prepare() memop is called. */

• sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,

• 		      buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
  

• if (!sgt->nents)

• if (dma_map_sgtable(buf->dev, sgt, buf->dma_dir,

• 	    DMA_ATTR_SKIP_CPU_SYNC))
  

  goto fail_map;

  buf->handler.refcount = &buf->refcount;

@@ -186,8 +185,8 @@ static void vb2_dma_sg_put(void *buf_priv) if (refcount_dec_and_test(&buf->refcount)) { dprintk(1, "%s: Freeing buffer of %d pages\n", __func__, buf->num_pages);

• dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,
  

• 		   buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
  

• dma_unmap_sgtable(buf->dev, sgt, buf->dma_dir,
  

• 		  DMA_ATTR_SKIP_CPU_SYNC);
  

  if (buf->vaddr) vm_unmap_ram(buf->vaddr, buf->num_pages); sg_free_table(buf->dma_sgt);

@@ -204,8 +203,7 @@ static void vb2_dma_sg_prepare(void *buf_priv) struct vb2_dma_sg_buf *buf = buf_priv; struct sg_table *sgt = buf->dma_sgt;

• dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents,

• 	       buf->dma_dir);
  

• dma_sync_sgtable_for_device(buf->dev, sgt, buf->dma_dir); }

  static void vb2_dma_sg_finish(void *buf_priv)

@@ -213,7 +211,7 @@ static void vb2_dma_sg_finish(void *buf_priv) struct vb2_dma_sg_buf *buf = buf_priv; struct sg_table *sgt = buf->dma_sgt;

• dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);

• dma_sync_sgtable_for_cpu(buf->dev, sgt, buf->dma_dir); }

  static void *vb2_dma_sg_get_userptr(struct device *dev, unsigned long vaddr,

@@ -256,9 +254,8 @@ static void *vb2_dma_sg_get_userptr(struct device *dev, unsigned long vaddr, * No need to sync to the device, this will happen later when the * prepare() memop is called. */

• sgt->nents = dma_map_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents,

• 		      buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
  

• if (!sgt->nents)

• if (dma_map_sgtable(buf->dev, sgt, buf->dma_dir,

• 	    DMA_ATTR_SKIP_CPU_SYNC))
  

  goto userptr_fail_map;

  return buf;

@@ -284,8 +281,7 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)

dprintk(1, "%s: Releasing userspace buffer of %d pages\n", __func__, buf->num_pages);

• dma_unmap_sg_attrs(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir,

• 	   DMA_ATTR_SKIP_CPU_SYNC);
  

• dma_unmap_sgtable(buf->dev, sgt, buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC); if (buf->vaddr) vm_unmap_ram(buf->vaddr, buf->num_pages); sg_free_table(buf->dma_sgt);

@@ -408,8 +404,7 @@ static void vb2_dma_sg_dmabuf_ops_detach(struct dma_buf *dbuf,

/* release the scatterlist cache */ if (attach->dma_dir != DMA_NONE)

• dma_unmap_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
  

• 	attach->dma_dir);
  

• dma_unmap_sgtable(db_attach->dev, sgt, attach->dma_dir, 0);
  

  sg_free_table(sgt); kfree(attach); db_attach->priv = NULL;

@@ -434,15 +429,12 @@ static struct sg_table *vb2_dma_sg_dmabuf_ops_map(

/* release any previous cache */ if (attach->dma_dir != DMA_NONE) {

• dma_unmap_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
  

• 	attach->dma_dir);
  

• dma_unmap_sgtable(db_attach->dev, sgt, attach->dma_dir, 0);
  

  attach->dma_dir = DMA_NONE; }

  /* mapping to the client with new direction */

• sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,

• 		dma_dir);
  

• if (!sgt->nents) {

• if (dma_map_sgtable(db_attach->dev, sgt, dma_dir, 0)) { pr_err("failed to map scatterlist\n"); mutex_unlock(lock); return ERR_PTR(-EIO);

diff --git a/drivers/media/common/videobuf2/videobuf2-vmalloc.c b/drivers/media/common/videobuf2/videobuf2-vmalloc.c index c66fda4a65e4..bf5ac63a5742 100644 --- a/drivers/media/common/videobuf2/videobuf2-vmalloc.c +++ b/drivers/media/common/videobuf2/videobuf2-vmalloc.c @@ -229,7 +229,7 @@ static int vb2_vmalloc_dmabuf_ops_attach(struct dma_buf *dbuf, kfree(attach); return ret; }

• for_each_sg(sgt->sgl, sg, sgt->nents, i) {

• for_each_sgtable_sg(sgt, sg, i) { struct page *page = vmalloc_to_page(vaddr);

  if (!page) {

@@ -259,8 +259,7 @@ static void vb2_vmalloc_dmabuf_ops_detach(struct dma_buf *dbuf,

/* release the scatterlist cache */ if (attach->dma_dir != DMA_NONE)

• dma_unmap_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
  

• 	attach->dma_dir);
  

• dma_unmap_sgtable(db_attach->dev, sgt, attach->dma_dir, 0);
  

  sg_free_table(sgt); kfree(attach); db_attach->priv = NULL;

@@ -285,15 +284,12 @@ static struct sg_table *vb2_vmalloc_dmabuf_ops_map(

/* release any previous cache */ if (attach->dma_dir != DMA_NONE) {

• dma_unmap_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,
  

• 	attach->dma_dir);
  

• dma_unmap_sgtable(db_attach->dev, sgt, attach->dma_dir, 0);
  

  attach->dma_dir = DMA_NONE; }

  /* mapping to the client with new direction */

• sgt->nents = dma_map_sg(db_attach->dev, sgt->sgl, sgt->orig_nents,

• 		dma_dir);
  

• if (!sgt->nents) {

• if (dma_map_sgtable(db_attach->dev, sgt, dma_dir, 0)) { pr_err("failed to map scatterlist\n"); mutex_unlock(lock); return ERR_PTR(-EIO);