lkml.org 
[lkml]   [2020]   [May]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH v4 32/38] staging: ion: fix common struct sg_table related issues
    Date
    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 <m.szyprowski@samsung.com>
    ---
    For more information, see '[PATCH v4 00/38] DRM: fix struct sg_table nents
    vs. orig_nents misuse' thread:
    https://lore.kernel.org/dri-devel/20200512085710.14688-1-m.szyprowski@samsung.com/T/
    ---
    drivers/staging/android/ion/ion.c | 25 +++++++--------
    drivers/staging/android/ion/ion_heap.c | 44 ++++++++-------------------
    drivers/staging/android/ion/ion_system_heap.c | 2 +-
    3 files changed, 25 insertions(+), 46 deletions(-)

    diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
    index 38b51ea..2db041d 100644
    --- a/drivers/staging/android/ion/ion.c
    +++ b/drivers/staging/android/ion/ion.c
    @@ -147,14 +147,14 @@ static struct sg_table *dup_sg_table(struct sg_table *table)
    if (!new_table)
    return ERR_PTR(-ENOMEM);

    - ret = sg_alloc_table(new_table, table->nents, GFP_KERNEL);
    + ret = sg_alloc_table(new_table, table->orig_nents, GFP_KERNEL);
    if (ret) {
    kfree(new_table);
    return ERR_PTR(-ENOMEM);
    }

    new_sg = new_table->sgl;
    - for_each_sg(table->sgl, sg, table->nents, i) {
    + for_each_sgtable_sg(table, sg, i) {
    memcpy(new_sg, sg, sizeof(*sg));
    new_sg->dma_address = 0;
    new_sg = sg_next(new_sg);
    @@ -224,12 +224,13 @@ static struct sg_table *ion_map_dma_buf(struct dma_buf_attachment *attachment,
    {
    struct ion_dma_buf_attachment *a = attachment->priv;
    struct sg_table *table;
    + int ret;

    table = a->table;

    - if (!dma_map_sg(attachment->dev, table->sgl, table->nents,
    - direction))
    - return ERR_PTR(-ENOMEM);
    + ret = dma_map_sgtable(attachment->dev, table, direction);
    + if (ret)
    + return ERR_PTR(ret);

    return table;
    }
    @@ -238,7 +239,7 @@ static void ion_unmap_dma_buf(struct dma_buf_attachment *attachment,
    struct sg_table *table,
    enum dma_data_direction direction)
    {
    - dma_unmap_sg(attachment->dev, table->sgl, table->nents, direction);
    + dma_unmap_sgtable(attachment->dev, table, direction);
    }

    static int ion_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
    @@ -296,10 +297,8 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
    }

    mutex_lock(&buffer->lock);
    - list_for_each_entry(a, &buffer->attachments, list) {
    - dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents,
    - direction);
    - }
    + list_for_each_entry(a, &buffer->attachments, list)
    + dma_sync_sgtable_for_cpu(a->dev, a->table, direction);

    unlock:
    mutex_unlock(&buffer->lock);
    @@ -319,10 +318,8 @@ static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
    }

    mutex_lock(&buffer->lock);
    - list_for_each_entry(a, &buffer->attachments, list) {
    - dma_sync_sg_for_device(a->dev, a->table->sgl, a->table->nents,
    - direction);
    - }
    + list_for_each_entry(a, &buffer->attachments, list)
    + dma_sync_sgtable_for_device(a->dev, a->table, direction);
    mutex_unlock(&buffer->lock);

    return 0;
    diff --git a/drivers/staging/android/ion/ion_heap.c b/drivers/staging/android/ion/ion_heap.c
    index 9c23b23..79f2794 100644
    --- a/drivers/staging/android/ion/ion_heap.c
    +++ b/drivers/staging/android/ion/ion_heap.c
    @@ -20,8 +20,7 @@
    void *ion_heap_map_kernel(struct ion_heap *heap,
    struct ion_buffer *buffer)
    {
    - struct scatterlist *sg;
    - int i, j;
    + struct sg_page_iter piter;
    void *vaddr;
    pgprot_t pgprot;
    struct sg_table *table = buffer->sg_table;
    @@ -38,14 +37,11 @@ void *ion_heap_map_kernel(struct ion_heap *heap,
    else
    pgprot = pgprot_writecombine(PAGE_KERNEL);

    - for_each_sg(table->sgl, sg, table->nents, i) {
    - int npages_this_entry = PAGE_ALIGN(sg->length) / PAGE_SIZE;
    - struct page *page = sg_page(sg);
    -
    - BUG_ON(i >= npages);
    - for (j = 0; j < npages_this_entry; j++)
    - *(tmp++) = page++;
    + for_each_sgtable_page(table, &piter, 0) {
    + BUG_ON(tmp - pages >= npages);
    + *tmp++ = sg_page_iter_page(&piter);
    }
    +
    vaddr = vmap(pages, npages, VM_MAP, pgprot);
    vfree(pages);

    @@ -64,32 +60,19 @@ void ion_heap_unmap_kernel(struct ion_heap *heap,
    int ion_heap_map_user(struct ion_heap *heap, struct ion_buffer *buffer,
    struct vm_area_struct *vma)
    {
    + struct sg_page_iter piter;
    struct sg_table *table = buffer->sg_table;
    unsigned long addr = vma->vm_start;
    - unsigned long offset = vma->vm_pgoff * PAGE_SIZE;
    - struct scatterlist *sg;
    - int i;
    int ret;

    - for_each_sg(table->sgl, sg, table->nents, i) {
    - struct page *page = sg_page(sg);
    - unsigned long remainder = vma->vm_end - addr;
    - unsigned long len = sg->length;
    + for_each_sgtable_page(table, &piter, vma->vm_pgoff) {
    + struct page *page = sg_page_iter_page(&piter);

    - if (offset >= sg->length) {
    - offset -= sg->length;
    - continue;
    - } else if (offset) {
    - page += offset / PAGE_SIZE;
    - len = sg->length - offset;
    - offset = 0;
    - }
    - len = min(len, remainder);
    - ret = remap_pfn_range(vma, addr, page_to_pfn(page), len,
    + ret = remap_pfn_range(vma, addr, page_to_pfn(page), PAGE_SIZE,
    vma->vm_page_prot);
    if (ret)
    return ret;
    - addr += len;
    + addr += PAGE_SIZE;
    if (addr >= vma->vm_end)
    return 0;
    }
    @@ -109,15 +92,14 @@ static int ion_heap_clear_pages(struct page **pages, int num, pgprot_t pgprot)
    return 0;
    }

    -static int ion_heap_sglist_zero(struct scatterlist *sgl, unsigned int nents,
    - pgprot_t pgprot)
    +static int ion_heap_sglist_zero(struct sg_table *sgt, pgprot_t pgprot)
    {
    int p = 0;
    int ret = 0;
    struct sg_page_iter piter;
    struct page *pages[32];

    - for_each_sg_page(sgl, &piter, nents, 0) {
    + for_each_sgtable_page(sgt, &piter, 0) {
    pages[p++] = sg_page_iter_page(&piter);
    if (p == ARRAY_SIZE(pages)) {
    ret = ion_heap_clear_pages(pages, p, pgprot);
    @@ -142,7 +124,7 @@ int ion_heap_buffer_zero(struct ion_buffer *buffer)
    else
    pgprot = pgprot_writecombine(PAGE_KERNEL);

    - return ion_heap_sglist_zero(table->sgl, table->nents, pgprot);
    + return ion_heap_sglist_zero(table, pgprot);
    }

    void ion_heap_freelist_add(struct ion_heap *heap, struct ion_buffer *buffer)
    diff --git a/drivers/staging/android/ion/ion_system_heap.c b/drivers/staging/android/ion/ion_system_heap.c
    index b83a1d1..eac0632 100644
    --- a/drivers/staging/android/ion/ion_system_heap.c
    +++ b/drivers/staging/android/ion/ion_system_heap.c
    @@ -162,7 +162,7 @@ static void ion_system_heap_free(struct ion_buffer *buffer)
    if (!(buffer->private_flags & ION_PRIV_FLAG_SHRINKER_FREE))
    ion_heap_buffer_zero(buffer);

    - for_each_sg(table->sgl, sg, table->nents, i)
    + for_each_sgtable_sg(table, sg, i)
    free_buffer_page(sys_heap, buffer, sg_page(sg));
    sg_free_table(table);
    kfree(table);
    --
    1.9.1
    \
     
     \ /
      Last update: 2020-05-12 11:02    [W:4.563 / U:0.116 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site