Re: [PATCH v7 00/36] DRM: fix struct sg_table nents vs. orig_nents misuse

[Marek Szyprowski]

Hi All,

On 19.06.2020 12:36, Marek Szyprowski wrote:
> 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.

David & Daniel: how would you like to merge those patches? They got 
quite a lot acks and some of them have dependencies on the DRM core. I 
would really like to get patches 1-28 merged via DRM (misc?) tree. Do 
you want me to prepare a branch and send a pull request?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland