lkml.org 
[lkml]   [2020]   [Jun]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Subject[PATCH v6 00/36] DRM: fix struct sg_table nents vs. orig_nents misuse
    Date
    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 and
    this is a result of my research. It looks that the incorrect pattern has
    been copied over the many drivers mainly in the DRM subsystem. Too bad in
    most cases it even worked correctly if the system used a simple, linear
    DMA-mapping implementation, for which swapping nents and orig_nents
    doesn't make any difference. To avoid similar issues in the future, I've
    introduced a common wrappers for DMA-mapping calls, which operate directly
    on the sg_table objects. I've also added wrappers for iterating over the
    scatterlists stored in the sg_table objects and applied them where
    possible. This, together with some common DRM prime helpers, allowed me
    to almost get rid of all nents/orig_nents usage in the drivers. I hope
    that such change makes the code robust, easier to follow and copy/paste
    safe.

    The biggest TODO is DRM/i915 driver and I don't feel brave enough to fix
    it fully. The driver creatively uses sg_table->orig_nents to store the
    size of the allocate scatterlist and ignores the number of the entries
    returned by dma_map_sg function. In this patchset I only fixed the
    sg_table objects exported by dmabuf related functions. I hope that I
    didn't break anything there.

    Patches are based on top of Linux next-20200618. The required changes to
    DMA-mapping framework has been already merged to v5.8-rc1.

    If possible I would like ask for merging most of the patches via DRM
    tree.

    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
    [4] https://lore.kernel.org/linux-iommu/20200512121931.GD20393@lst.de/T/#ma18c958a48c3b241d5409517fa7d192eef87459b

    Changelog:

    v6:
    - rebased onto Linux next-20200618, which is based on v5.8-rc1; fixed conflicts

    v5: https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprowski@samsung.com/T/
    - fixed some minor style issues and typos
    - fixed lack of the attrs argument in ion, dmabuf, rapidio, fastrpc and
    vfio patches

    v4: https://lore.kernel.org/linux-iommu/20200512121931.GD20393@lst.de/T/
    - added for_each_sgtable_* wrappers and applied where possible
    - added drm_prime_get_contiguous_size() and applied where possible
    - applied drm_prime_sg_to_page_addr_arrays() where possible to remove page
    extraction from sg_table objects
    - added documentation for the introduced wrappers
    - improved patches description a bit

    v3: https://lore.kernel.org/dri-devel/20200505083926.28503-1-m.szyprowski@samsung.com/
    - introduce dma_*_sgtable_* wrappers and use them in all patches

    v2: https://lore.kernel.org/linux-iommu/c01c9766-9778-fd1f-f36e-2dc7bd376ba4@arm.com/T/
    - dropped most of the changes to drm/i915
    - added fixes for rcar-du, xen, media and ion
    - fixed a few issues pointed by kbuild test robot
    - added wide cc: list for each patch

    v1: https://lore.kernel.org/linux-iommu/c01c9766-9778-fd1f-f36e-2dc7bd376ba4@arm.com/T/
    - initial version


    Patch summary:

    Marek Szyprowski (36):
    drm: prime: add common helper to check scatterlist contiguity
    drm: prime: use sgtable iterators in
    drm_prime_sg_to_page_addr_arrays()
    drm: core: fix common struct sg_table related issues
    drm: amdgpu: fix common struct sg_table related issues
    drm: armada: fix common struct sg_table related issues
    drm: etnaviv: fix common struct sg_table related issues
    drm: exynos: use common helper for a scatterlist contiguity check
    drm: exynos: fix common struct sg_table related issues
    drm: i915: fix common struct sg_table related issues
    drm: lima: fix common struct sg_table related issues
    drm: mediatek: use common helper for a scatterlist contiguity check
    drm: mediatek: use common helper for extracting pages array
    drm: msm: fix common struct sg_table related issues
    drm: omapdrm: use common helper for extracting pages array
    drm: omapdrm: fix common struct sg_table related issues
    drm: panfrost: fix common struct sg_table related issues
    drm: radeon: fix common struct sg_table related issues
    drm: rockchip: use common helper for a scatterlist contiguity check
    drm: rockchip: fix common struct sg_table related issues
    drm: tegra: fix common struct sg_table related issues
    drm: v3d: fix common struct sg_table related issues
    drm: virtio: fix common struct sg_table related issues
    drm: vmwgfx: fix common struct sg_table related issues
    xen: gntdev: fix common struct sg_table related issues
    drm: host1x: fix common struct sg_table related issues
    drm: rcar-du: fix common struct sg_table related issues
    dmabuf: fix common struct sg_table related issues
    staging: ion: remove dead code
    staging: ion: fix common struct sg_table related issues
    staging: tegra-vde: fix common struct sg_table related issues
    misc: fastrpc: fix common struct sg_table related issues
    rapidio: fix common struct sg_table related issues
    samples: vfio-mdev/mbochs: fix common struct sg_table related issues
    media: pci: fix common ALSA DMA-mapping related codes
    videobuf2: use sgtable-based scatterlist wrappers
    drm: xen: fix common struct sg_table related issues

    drivers/dma-buf/heaps/heap-helpers.c | 13 ++-
    drivers/dma-buf/udmabuf.c | 7 +-
    drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 6 +-
    drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 9 +-
    drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 8 +-
    drivers/gpu/drm/armada/armada_gem.c | 12 +--
    drivers/gpu/drm/drm_cache.c | 2 +-
    drivers/gpu/drm/drm_gem_cma_helper.c | 23 +----
    drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +--
    drivers/gpu/drm/drm_prime.c | 86 ++++++++++---------
    drivers/gpu/drm/etnaviv/etnaviv_gem.c | 12 ++-
    drivers/gpu/drm/etnaviv/etnaviv_mmu.c | 13 +--
    drivers/gpu/drm/exynos/exynos_drm_g2d.c | 10 +--
    drivers/gpu/drm/exynos/exynos_drm_gem.c | 23 +----
    drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 11 +--
    .../gpu/drm/i915/gem/selftests/mock_dmabuf.c | 7 +-
    drivers/gpu/drm/lima/lima_gem.c | 11 ++-
    drivers/gpu/drm/lima/lima_vm.c | 5 +-
    drivers/gpu/drm/mediatek/mtk_drm_gem.c | 37 ++------
    drivers/gpu/drm/msm/msm_gem.c | 13 ++-
    drivers/gpu/drm/msm/msm_gpummu.c | 14 ++-
    drivers/gpu/drm/msm/msm_iommu.c | 2 +-
    drivers/gpu/drm/omapdrm/omap_gem.c | 20 ++---
    drivers/gpu/drm/panfrost/panfrost_gem.c | 4 +-
    drivers/gpu/drm/panfrost/panfrost_mmu.c | 7 +-
    drivers/gpu/drm/radeon/radeon_ttm.c | 11 ++-
    drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 3 +-
    drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 42 +++------
    drivers/gpu/drm/tegra/gem.c | 27 +++---
    drivers/gpu/drm/tegra/plane.c | 15 ++--
    drivers/gpu/drm/v3d/v3d_mmu.c | 17 ++--
    drivers/gpu/drm/virtio/virtgpu_object.c | 36 ++++----
    drivers/gpu/drm/virtio/virtgpu_vq.c | 12 ++-
    drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 17 +---
    drivers/gpu/drm/xen/xen_drm_front_gem.c | 2 +-
    drivers/gpu/host1x/job.c | 22 ++---
    .../common/videobuf2/videobuf2-dma-contig.c | 41 ++++-----
    .../media/common/videobuf2/videobuf2-dma-sg.c | 32 +++----
    .../common/videobuf2/videobuf2-vmalloc.c | 12 +--
    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 +-
    drivers/media/platform/vsp1/vsp1_drm.c | 8 +-
    drivers/misc/fastrpc.c | 4 +-
    drivers/rapidio/devices/rio_mport_cdev.c | 8 +-
    drivers/staging/android/ion/ion.c | 25 +++---
    drivers/staging/android/ion/ion.h | 1 -
    drivers/staging/android/ion/ion_heap.c | 53 +++---------
    drivers/staging/android/ion/ion_system_heap.c | 2 +-
    drivers/staging/media/tegra-vde/iommu.c | 4 +-
    drivers/xen/gntdev-dmabuf.c | 13 ++-
    include/drm/drm_prime.h | 2 +
    samples/vfio-mdev/mbochs.c | 3 +-
    54 files changed, 311 insertions(+), 478 deletions(-)

    --
    2.17.1

    \
     
     \ /
      Last update: 2020-06-18 17:41    [W:3.041 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site