Re: [PATCH v6 23/36] drm: vmwgfx: 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-000417 base:ce2cc8efd7a40cbd17841add878cb691d0ce0bba config: x86_64-rhel-7.6 (attached as .config) compiler: gcc-9 (Debian 9.3.0-13) 9.3.0 reproduce (this is a W=1 build): # save the attached .config to linux build tree make W=1 ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All error/warnings (new ones prefixed by >>): drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c: In function 'vmw_ttm_unmap_from_dma': >> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c:365:31: error: incompatible type >> for argument 2 of 'dma_unmap_sgtable' 365 | dma_unmap_sgtable(dev, vmw_tt->sgt, DMA_BIDIRECTIONAL, 0); | ~~^ | | | struct sg_table In file included from include/linux/dma-buf.h:20, from drivers/gpu/drm/vmwgfx/ttm_object.h:40, from drivers/gpu/drm/vmwgfx/ttm_lock.h:55, from drivers/gpu/drm/vmwgfx/vmwgfx_drv.h:44, from drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c:28: include/linux/dma-mapping.h:651:75: note: expected 'struct sg_table *' but argument is of type 'struct sg_table' 651 | static inline void dma_unmap_sgtable(struct device *dev, struct sg_table *sgt, | ~^~~ drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c: In function 'vmw_ttm_map_for_dma': >> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c:386:36: error: incompatible type >> for argument 2 of 'dma_map_sgtable' 386 | return dma_map_sgtable(dev, vmw_tt->sgt, DMA_BIDIRECTIONAL, 0); | ~~^ || |struct sg_table In file included from include/linux/dma-buf.h:20, from drivers/gpu/drm/vmwgfx/ttm_object.h:40, from drivers/gpu/drm/vmwgfx/ttm_lock.h:55, from drivers/gpu/drm/vmwgfx/vmwgfx_drv.h:44, from drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c:28: include/linux/dma-mapping.h:628:72: note: expected 'struct sg_table *' but argument is of type 'struct sg_table' 628 | static inline int dma_map_sgtable(struct device *dev, struct sg_table *sgt, | ~^~~ >> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c:387:1: warning: control reaches >> end of non-void function [-Wreturn-type] 387 | } | ^ vim +/dma_unmap_sgtable +365 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c 352 353 /** 354 * vmw_ttm_unmap_from_dma - unmap device addresses previsouly mapped for 355 * TTM pages 356 * 357 * @vmw_tt: Pointer to a struct vmw_ttm_backend 358 * 359 * Used to free dma mappings previously mapped by vmw_ttm_map_for_dma. 360 */ 361 static void vmw_ttm_unmap_from_dma(struct vmw_ttm_tt *vmw_tt) 362 { 363 struct device *dev = vmw_tt->dev_priv->dev->dev; 364 > 365 dma_unmap_sgtable(dev, vmw_tt->sgt, DMA_BIDIRECTIONAL, 0); 366 vmw_tt->sgt.nents = vmw_tt->sgt.orig_nents; 367 } 368 369 /** 370 * vmw_ttm_map_for_dma - map TTM pages to get device addresses 371 * 372 * @vmw_tt: Pointer to a struct vmw_ttm_backend 373 * 374 * This function is used to get device addresses from the kernel DMA layer. 375 * However, it's violating the DMA API in that when this operation has been 376 * performed, it's illegal for the CPU to write to the pages without first 377 * unmapping the DMA mappings, or calling dma_sync_sg_for_cpu(). It is 378 * therefore only legal to call this function if we know that the function 379 * dma_sync_sg_for_cpu() is a NOP, and dma_sync_sg_for_device() is at most 380 * a CPU write buffer flush. 381 */ 382 static int vmw_ttm_map_for_dma(struct vmw_ttm_tt *vmw_tt) 383 { 384 struct device *dev = vmw_tt->dev_priv->dev->dev; 385 > 386 return dma_map_sgtable(dev, vmw_tt->sgt, DMA_BIDIRECTIONAL, 0); > 387 } 388 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.g
Re: [PATCH v6 23/36] drm: vmwgfx: fix common struct sg_table related issues
Hi Marek, I love your patch! Perhaps something to improve: [auto build test WARNING on next-20200618] [also build test WARNING 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-000417 base:ce2cc8efd7a40cbd17841add878cb691d0ce0bba config: x86_64-randconfig-s021-20200618 (attached as .config) compiler: gcc-9 (Debian 9.3.0-13) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.2-rc1-10-gc17b1b06-dirty # save the attached .config to linux build tree make W=1 C=1 ARCH=x86_64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot sparse warnings: (new ones prefixed by >>) >> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c:365:38: sparse: sparse: incorrect >> type in argument 2 (different base types) @@ expected struct sg_table >> *sgt @@ got struct sg_table sgt @@ >> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c:365:38: sparse: expected >> struct sg_table *sgt >> drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c:365:38: sparse: got struct >> sg_table sgt drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c:386:43: sparse: sparse: incorrect type in argument 2 (different base types) @@ expected struct sg_table *sgt @@ got struct sg_table sgt @@ drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c:386:43: sparse: expected struct sg_table *sgt drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c:386:43: sparse: got struct sg_table sgt vim +365 drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c 352 353 /** 354 * vmw_ttm_unmap_from_dma - unmap device addresses previsouly mapped for 355 * TTM pages 356 * 357 * @vmw_tt: Pointer to a struct vmw_ttm_backend 358 * 359 * Used to free dma mappings previously mapped by vmw_ttm_map_for_dma. 360 */ 361 static void vmw_ttm_unmap_from_dma(struct vmw_ttm_tt *vmw_tt) 362 { 363 struct device *dev = vmw_tt->dev_priv->dev->dev; 364 > 365 dma_unmap_sgtable(dev, vmw_tt->sgt, DMA_BIDIRECTIONAL, 0); 366 vmw_tt->sgt.nents = vmw_tt->sgt.orig_nents; 367 } 368 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v6 23/36] drm: vmwgfx: 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 Acked-by: Roland Scheidegger --- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 17 - 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c index bf0bc4697959..49ed6add6969 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c @@ -362,8 +362,7 @@ static void vmw_ttm_unmap_from_dma(struct vmw_ttm_tt *vmw_tt) { struct device *dev = vmw_tt->dev_priv->dev->dev; - dma_unmap_sg(dev, vmw_tt->sgt.sgl, vmw_tt->sgt.nents, - DMA_BIDIRECTIONAL); + dma_unmap_sgtable(dev, vmw_tt->sgt, DMA_BIDIRECTIONAL, 0); vmw_tt->sgt.nents = vmw_tt->sgt.orig_nents; } @@ -383,16 +382,8 @@ static void vmw_ttm_unmap_from_dma(struct vmw_ttm_tt *vmw_tt) static int vmw_ttm_map_for_dma(struct vmw_ttm_tt *vmw_tt) { struct device *dev = vmw_tt->dev_priv->dev->dev; - int ret; - - ret = dma_map_sg(dev, vmw_tt->sgt.sgl, vmw_tt->sgt.orig_nents, -DMA_BIDIRECTIONAL); - if (unlikely(ret == 0)) - return -ENOMEM; - vmw_tt->sgt.nents = ret; - - return 0; + return dma_map_sgtable(dev, vmw_tt->sgt, DMA_BIDIRECTIONAL, 0); } /** @@ -449,10 +440,10 @@ static int vmw_ttm_map_dma(struct vmw_ttm_tt *vmw_tt) if (unlikely(ret != 0)) goto out_sg_alloc_fail; - if (vsgt->num_pages > vmw_tt->sgt.nents) { + if (vsgt->num_pages > vmw_tt->sgt.orig_nents) { uint64_t over_alloc = sgl_size * (vsgt->num_pages - - vmw_tt->sgt.nents); + vmw_tt->sgt.orig_nents); ttm_mem_global_free(glob, over_alloc); vmw_tt->sg_alloc_size -= over_alloc; -- 2.17.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu