| Message ID | 20200428132005.21424-1-m.szyprowski@samsung.com |
|---|---|
| Headers | show |
| Series | DRM: fix struct sg_table nents vs. orig_nents misuse | expand |
On Tue, Apr 28, 2020 at 04:02:57PM +0200, Christoph Hellwig wrote: > On Tue, Apr 28, 2020 at 03:19:48PM +0200, Marek Szyprowski wrote: > > 1. introduce a dma_{map,sync,unmap}_sgtable() wrappers, which will use > > a proper sg_table entries and call respective DMA-mapping functions > > and adapt current code to it > > That sounds reasonable to me. Those could be pretty trivial wrappers. > > > > > > > 2. rename nents and orig_nents to nr_pages, nr_dmas to clearly state > > which one refers to which part of the scatterlist; I'm open for > > other names for those entries > > nr_cpu_ents and nr_dma_ents might be better names, but it still would be > a whole lot of churn for little gain. I think just good wrappers like > suggested above might be more helpful. I guess long-term we could aim for both? I.e. roll out better wrappers first, once that's soaked through the tree, rename the last offenders. Personally I like nr_cpu_ents and nr_dma_ents, that's about as clear as it gets. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2020-04-28 4:32 pm, Daniel Vetter wrote: > On Tue, Apr 28, 2020 at 04:02:57PM +0200, Christoph Hellwig wrote: >> On Tue, Apr 28, 2020 at 03:19:48PM +0200, Marek Szyprowski wrote: >>> 1. introduce a dma_{map,sync,unmap}_sgtable() wrappers, which will use >>> a proper sg_table entries and call respective DMA-mapping functions >>> and adapt current code to it >> >> That sounds reasonable to me. Those could be pretty trivial wrappers. >> >>> >>> >>> 2. rename nents and orig_nents to nr_pages, nr_dmas to clearly state >>> which one refers to which part of the scatterlist; I'm open for >>> other names for those entries >> >> nr_cpu_ents and nr_dma_ents might be better names, but it still would be >> a whole lot of churn for little gain. I think just good wrappers like >> suggested above might be more helpful. > > I guess long-term we could aim for both? I.e. roll out better wrappers > first, once that's soaked through the tree, rename the last offenders. Yes, that's what I was thinking too - most of these uses are just passing them in and out of the DMA APIs, and thus would be subsumed into the wrappers anyway, then in the relatively few remaining places where the table is actually iterated for one reason or the other, renaming would stand to help review and maintenance in terms of making it far more obvious when the implementation and the intent don't match. Robin. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Dear All, During the Exynos DRM GEM rework and fixing the issues in the drm_prime_sg_to_page_addr_arrays() function [1] I've noticed that most drivers in DRM framework incorrectly use nents and orig_nents entries of the struct sg_table. In case of the most DMA-mapping implementations exchanging those two entries or using nents for all loops on the scatterlist is harmless, because they both have the same value. There exists however a DMA-mapping implementations, for which such incorrect usage breaks things. The nents returned by dma_map_sg() might be lower than the nents passed as its parameter and this is perfectly fine. DMA framework or IOMMU is allowed to join consecutive chunks while mapping if such operation is supported by the underlying HW (bus, bridge, IOMMU, etc). Example of the case where dma_map_sg() might return 1 'DMA' chunk for the 4 'physical' pages is described here [2] The DMA-mapping framework documentation [3] 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 common pattern in DRM drivers were to assign the dma_map_sg() return value to sg_table->nents and use that value for the subsequent calls to dma_sync_sg_* or dma_unmap_sg functions. Also the code iterated over nents times to access the pages stored in the processed scatterlist, while it should use orig_nents as the numer of the page entries. I've tried to identify all such incorrect usage of sg_table->nents in the DRM subsystem and this is a result of my research. It looks that the incorrect pattern has been copied over the many drivers. Too bad in most cases it even worked correctly if the system used simple, linear DMA-mapping implementation, for which swapping nents and orig_nents doesn't make any difference. I wonder what to do to avoid such misuse in the future, as this case clearly shows that the current sg_table structure is a bit hard to understand. I have the following ideas and I would like to know your opinion before I prepare more patches and check sg_table usage all over the kernel: 1. introduce a dma_{map,sync,unmap}_sgtable() wrappers, which will use a proper sg_table entries and call respective DMA-mapping functions and adapt current code to it 2. rename nents and orig_nents to nr_pages, nr_dmas to clearly state which one refers to which part of the scatterlist; I'm open for other names for those entries I will appreciate your comments. Patches are based on top of Linux next-20200428. I've reduced the recipients list mainly to mailing lists, the next version I will try to send to the maintainers of the respective drivers. Best regards, Marek Szyprowski References: [1] https://lkml.org/lkml/2020/3/27/555 [2] https://lkml.org/lkml/2020/3/29/65 [3] Documentation/DMA-API-HOWTO.txt Patch summary: Marek Szyprowski (17): drm: core: fix sg_table nents vs. orig_nents usage drm: amdgpu: fix sg_table nents vs. orig_nents usage drm: armada: fix sg_table nents vs. orig_nents usage drm: etnaviv: fix sg_table nents vs. orig_nents usage drm: exynos: fix sg_table nents vs. orig_nents usage drm: i915: fix sg_table nents vs. orig_nents usage drm: lima: fix sg_table nents vs. orig_nents usage drm: msm: fix sg_table nents vs. orig_nents usage drm: panfrost: fix sg_table nents vs. orig_nents usage drm: radeon: fix sg_table nents vs. orig_nents usage drm: rockchip: fix sg_table nents vs. orig_nents usage drm: tegra: fix sg_table nents vs. orig_nents usage drm: virtio: fix sg_table nents vs. orig_nents usage drm: vmwgfx: fix sg_table nents vs. orig_nents usage drm: xen: fix sg_table nents vs. orig_nents usage drm: host1x: fix sg_table nents vs. orig_nents usage dmabuf: fix sg_table nents vs. orig_nents usage drivers/dma-buf/heaps/heap-helpers.c | 7 ++++--- drivers/dma-buf/udmabuf.c | 5 +++-- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 7 ++++--- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 8 ++++---- drivers/gpu/drm/armada/armada_gem.c | 14 ++++++++----- drivers/gpu/drm/drm_cache.c | 2 +- drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++--- drivers/gpu/drm/drm_prime.c | 9 +++++---- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 10 ++++++---- drivers/gpu/drm/exynos/exynos_drm_g2d.c | 7 ++++--- drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 13 ++++++------ drivers/gpu/drm/i915/gem/i915_gem_internal.c | 4 ++-- drivers/gpu/drm/i915/gem/i915_gem_region.c | 4 ++-- drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 5 +++-- drivers/gpu/drm/i915/gem/selftests/huge_pages.c | 10 +++++----- drivers/gpu/drm/i915/gem/selftests/mock_dmabuf.c | 5 +++-- drivers/gpu/drm/i915/gt/intel_ggtt.c | 12 ++++++------ drivers/gpu/drm/i915/i915_gem_gtt.c | 12 +++++++----- drivers/gpu/drm/i915/i915_scatterlist.c | 4 ++-- drivers/gpu/drm/i915/selftests/scatterlist.c | 8 ++++---- drivers/gpu/drm/lima/lima_gem.c | 4 ++-- drivers/gpu/drm/msm/msm_gem.c | 8 ++++---- drivers/gpu/drm/msm/msm_iommu.c | 3 ++- drivers/gpu/drm/panfrost/panfrost_gem.c | 3 ++- drivers/gpu/drm/panfrost/panfrost_mmu.c | 4 +++- drivers/gpu/drm/radeon/radeon_ttm.c | 10 +++++----- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 15 +++++++------- drivers/gpu/drm/tegra/gem.c | 25 ++++++++++++------------ drivers/gpu/drm/tegra/plane.c | 13 ++++++------ drivers/gpu/drm/virtio/virtgpu_object.c | 11 ++++++----- drivers/gpu/drm/virtio/virtgpu_vq.c | 8 ++++---- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 6 +++--- drivers/gpu/drm/xen/xen_drm_front_gem.c | 2 +- drivers/gpu/host1x/job.c | 17 ++++++++-------- 34 files changed, 154 insertions(+), 128 deletions(-) -- 1.9.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel