Re: [PATCH v7 26/36] drm: host1x: fix common struct sg_table related issues
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
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
[PATCH v7 26/36] drm: host1x: 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.
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
---
drivers/gpu/host1x/job.c | 22 --
1 file changed, 8 insertions(+), 14 deletions(-)
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);
--
2.17.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
