diff mbox series

[v7,26/36] drm: host1x: fix common struct sg_table related issues

Message ID 20200619103636.11974-27-m.szyprowski@samsung.com
State New
Headers show
Series None | expand

Commit Message

Marek Szyprowski June 19, 2020, 10:36 a.m. UTC
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/host1x/job.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

Comments

kernel test robot June 21, 2020, 2:47 p.m. UTC | #1
Hi Marek,

I love your patch! Yet something to improve:

[auto build test ERROR on next-20200618]
[also build test ERROR on v5.8-rc1]
[cannot apply to linuxtv-media/master staging/staging-testing drm-exynos/exynos-drm-next drm-intel/for-linux-next linus/master v5.8-rc1 v5.7 v5.7-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Marek-Szyprowski/DRM-fix-struct-sg_table-nents-vs-orig_nents-misuse/20200619-184302
base:    ce2cc8efd7a40cbd17841add878cb691d0ce0bba
config: arm64-randconfig-r036-20200621 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project ef455a55bcf2cfea04a99c361b182ad18b7f03f1)
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
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 

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 >>):

>> drivers/gpu/host1x/job.c:230:10: error: implicit declaration of function 'iommu_map_sgtable' [-Werror,-Wimplicit-function-declaration]
                           err = iommu_map_sgtable(host->domain,
                                 ^
   drivers/gpu/host1x/job.c:230:10: note: did you mean 'dma_map_sgtable'?
   include/linux/dma-mapping.h:628:19: note: 'dma_map_sgtable' declared here
   static inline int dma_map_sgtable(struct device *dev, struct sg_table *sgt,
                     ^
   1 error generated.

vim +/iommu_map_sgtable +230 drivers/gpu/host1x/job.c

   100	
   101	static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
   102	{
   103		struct host1x_client *client = job->client;
   104		struct device *dev = client->dev;
   105		struct iommu_domain *domain;
   106		unsigned int i;
   107		int err;
   108	
   109		domain = iommu_get_domain_for_dev(dev);
   110		job->num_unpins = 0;
   111	
   112		for (i = 0; i < job->num_relocs; i++) {
   113			struct host1x_reloc *reloc = &job->relocs[i];
   114			dma_addr_t phys_addr, *phys;
   115			struct sg_table *sgt;
   116	
   117			reloc->target.bo = host1x_bo_get(reloc->target.bo);
   118			if (!reloc->target.bo) {
   119				err = -EINVAL;
   120				goto unpin;
   121			}
   122	
   123			/*
   124			 * If the client device is not attached to an IOMMU, the
   125			 * physical address of the buffer object can be used.
   126			 *
   127			 * Similarly, when an IOMMU domain is shared between all
   128			 * host1x clients, the IOVA is already available, so no
   129			 * need to map the buffer object again.
   130			 *
   131			 * XXX Note that this isn't always safe to do because it
   132			 * relies on an assumption that no cache maintenance is
   133			 * needed on the buffer objects.
   134			 */
   135			if (!domain || client->group)
   136				phys = &phys_addr;
   137			else
   138				phys = NULL;
   139	
   140			sgt = host1x_bo_pin(dev, reloc->target.bo, phys);
   141			if (IS_ERR(sgt)) {
   142				err = PTR_ERR(sgt);
   143				goto unpin;
   144			}
   145	
   146			if (sgt) {
   147				unsigned long mask = HOST1X_RELOC_READ |
   148						     HOST1X_RELOC_WRITE;
   149				enum dma_data_direction dir;
   150	
   151				switch (reloc->flags & mask) {
   152				case HOST1X_RELOC_READ:
   153					dir = DMA_TO_DEVICE;
   154					break;
   155	
   156				case HOST1X_RELOC_WRITE:
   157					dir = DMA_FROM_DEVICE;
   158					break;
   159	
   160				case HOST1X_RELOC_READ | HOST1X_RELOC_WRITE:
   161					dir = DMA_BIDIRECTIONAL;
   162					break;
   163	
   164				default:
   165					err = -EINVAL;
   166					goto unpin;
   167				}
   168	
   169				err = dma_map_sgtable(dev, sgt, dir, 0);
   170				if (err)
   171					goto unpin;
   172	
   173				job->unpins[job->num_unpins].dev = dev;
   174				job->unpins[job->num_unpins].dir = dir;
   175				phys_addr = sg_dma_address(sgt->sgl);
   176			}
   177	
   178			job->addr_phys[job->num_unpins] = phys_addr;
   179			job->unpins[job->num_unpins].bo = reloc->target.bo;
   180			job->unpins[job->num_unpins].sgt = sgt;
   181			job->num_unpins++;
   182		}
   183	
   184		for (i = 0; i < job->num_gathers; i++) {
   185			struct host1x_job_gather *g = &job->gathers[i];
   186			size_t gather_size = 0;
   187			struct scatterlist *sg;
   188			struct sg_table *sgt;
   189			dma_addr_t phys_addr;
   190			unsigned long shift;
   191			struct iova *alloc;
   192			dma_addr_t *phys;
   193			unsigned int j;
   194	
   195			g->bo = host1x_bo_get(g->bo);
   196			if (!g->bo) {
   197				err = -EINVAL;
   198				goto unpin;
   199			}
   200	
   201			/**
   202			 * If the host1x is not attached to an IOMMU, there is no need
   203			 * to map the buffer object for the host1x, since the physical
   204			 * address can simply be used.
   205			 */
   206			if (!iommu_get_domain_for_dev(host->dev))
   207				phys = &phys_addr;
   208			else
   209				phys = NULL;
   210	
   211			sgt = host1x_bo_pin(host->dev, g->bo, phys);
   212			if (IS_ERR(sgt)) {
   213				err = PTR_ERR(sgt);
   214				goto unpin;
   215			}
   216	
   217			if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && host->domain) {
   218				for_each_sgtable_sg(sgt, sg, j)
   219					gather_size += sg->length;
   220				gather_size = iova_align(&host->iova, gather_size);
   221	
   222				shift = iova_shift(&host->iova);
   223				alloc = alloc_iova(&host->iova, gather_size >> shift,
   224						   host->iova_end >> shift, true);
   225				if (!alloc) {
   226					err = -ENOMEM;
   227					goto unpin;
   228				}
   229	
 > 230				err = iommu_map_sgtable(host->domain,
   231						iova_dma_addr(&host->iova, alloc),
   232						sgt, IOMMU_READ);
   233				if (err == 0) {
   234					__free_iova(&host->iova, alloc);
   235					err = -EINVAL;
   236					goto unpin;
   237				}
   238	
   239				job->unpins[job->num_unpins].size = gather_size;
   240				phys_addr = iova_dma_addr(&host->iova, alloc);
   241			} else if (sgt) {
   242				err = dma_map_sgtable(host->dev, sgt, DMA_TO_DEVICE, 0);
   243				if (err)
   244					goto unpin;
   245	
   246				job->unpins[job->num_unpins].dir = DMA_TO_DEVICE;
   247				job->unpins[job->num_unpins].dev = host->dev;
   248				phys_addr = sg_dma_address(sgt->sgl);
   249			}
   250	
   251			job->addr_phys[job->num_unpins] = phys_addr;
   252			job->gather_addr_phys[i] = phys_addr;
   253	
   254			job->unpins[job->num_unpins].bo = g->bo;
   255			job->unpins[job->num_unpins].sgt = sgt;
   256			job->num_unpins++;
   257		}
   258	
   259		return 0;
   260	
   261	unpin:
   262		host1x_job_unpin(job);
   263		return err;
   264	}
   265	

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

Patch

diff --git a/drivers/gpu/host1x/job.c b/drivers/gpu/host1x/job.c
index a10643aa89aa..4832b57f10c4 100644
--- a/drivers/gpu/host1x/job.c
+++ b/drivers/gpu/host1x/job.c
@@ -166,11 +166,9 @@  static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 				goto unpin;
 			}
 
-			err = dma_map_sg(dev, sgt->sgl, sgt->nents, dir);
-			if (!err) {
-				err = -ENOMEM;
+			err = dma_map_sgtable(dev, sgt, dir, 0);
+			if (err)
 				goto unpin;
-			}
 
 			job->unpins[job->num_unpins].dev = dev;
 			job->unpins[job->num_unpins].dir = dir;
@@ -217,7 +215,7 @@  static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 		}
 
 		if (!IS_ENABLED(CONFIG_TEGRA_HOST1X_FIREWALL) && host->domain) {
-			for_each_sg(sgt->sgl, sg, sgt->nents, j)
+			for_each_sgtable_sg(sgt, sg, j)
 				gather_size += sg->length;
 			gather_size = iova_align(&host->iova, gather_size);
 
@@ -229,9 +227,9 @@  static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 				goto unpin;
 			}
 
-			err = iommu_map_sg(host->domain,
+			err = iommu_map_sgtable(host->domain,
 					iova_dma_addr(&host->iova, alloc),
-					sgt->sgl, sgt->nents, IOMMU_READ);
+					sgt, IOMMU_READ);
 			if (err == 0) {
 				__free_iova(&host->iova, alloc);
 				err = -EINVAL;
@@ -241,12 +239,9 @@  static unsigned int pin_job(struct host1x *host, struct host1x_job *job)
 			job->unpins[job->num_unpins].size = gather_size;
 			phys_addr = iova_dma_addr(&host->iova, alloc);
 		} else if (sgt) {
-			err = dma_map_sg(host->dev, sgt->sgl, sgt->nents,
-					 DMA_TO_DEVICE);
-			if (!err) {
-				err = -ENOMEM;
+			err = dma_map_sgtable(host->dev, sgt, DMA_TO_DEVICE, 0);
+			if (err)
 				goto unpin;
-			}
 
 			job->unpins[job->num_unpins].dir = DMA_TO_DEVICE;
 			job->unpins[job->num_unpins].dev = host->dev;
@@ -647,8 +642,7 @@  void host1x_job_unpin(struct host1x_job *job)
 		}
 
 		if (unpin->dev && sgt)
-			dma_unmap_sg(unpin->dev, sgt->sgl, sgt->nents,
-				     unpin->dir);
+			dma_unmap_sgtable(unpin->dev, sgt, unpin->dir, 0);
 
 		host1x_bo_unpin(dev, unpin->bo, sgt);
 		host1x_bo_put(unpin->bo);