Hi John,
I spotted one thing below which might be harmless, but best to check.
On Fri, Sep 06, 2019 at 06:47:11PM +0000, John Stultz wrote:
> This adds a CMA heap, which allows userspace to allocate
> a dma-buf of contiguous memory out of a CMA region.
>
> This code is an evolution of the Android ION implementation, so
> thanks to its original author and maintainters:
> Benjamin Gaignard, Laura Abbott, and others!
>
> Cc: Laura Abbott <labb...@redhat.com>
> Cc: Benjamin Gaignard <benjamin.gaign...@linaro.org>
> Cc: Sumit Semwal <sumit.sem...@linaro.org>
> Cc: Liam Mark <lm...@codeaurora.org>
> Cc: Pratik Patel <prat...@codeaurora.org>
> Cc: Brian Starkey <brian.star...@arm.com>
> Cc: Vincent Donnefort <vincent.donnef...@arm.com>
> Cc: Sudipto Paul <sudipto.p...@arm.com>
> Cc: Andrew F. Davis <a...@ti.com>
> Cc: Christoph Hellwig <h...@infradead.org>
> Cc: Chenbo Feng <fe...@google.com>
> Cc: Alistair Strachan <astrac...@google.com>
> Cc: Hridya Valsaraju <hri...@google.com>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Benjamin Gaignard <benjamin.gaign...@linaro.org>
> Signed-off-by: John Stultz <john.stu...@linaro.org>
> ---
> v2:
> * Switch allocate to return dmabuf fd
> * Simplify init code
> * Checkpatch fixups
> v3:
> * Switch to inline function for to_cma_heap()
> * Minor cleanups suggested by Brian
> * Fold in new registration style from Andrew
> * Folded in changes from Andrew to use simplified page list
> from the heap helpers
> v4:
> * Use the fd_flags when creating dmabuf fd (Suggested by
> Benjamin)
> * Use precalculated pagecount (Suggested by Andrew)
> v6:
> * Changed variable names to improve clarity, as suggested
> by Brian
> v7:
> * Use newly lower-cased init_heap_helper_buffer helper
> * Use new dmabuf export helper
> v8:
> * Make struct dma_heap_ops const (Suggested by Christoph)
> * Condense dma_heap_buffer and heap_helper_buffer (suggested by
> Christoph)
> * Checkpatch whitespace fixups
> ---
...
> +
> +/* dmabuf heap CMA operations functions */
> +static int cma_heap_allocate(struct dma_heap *heap,
> + unsigned long len,
> + unsigned long fd_flags,
> + unsigned long heap_flags)
> +{
> + struct cma_heap *cma_heap = dma_heap_get_data(heap);
> + struct heap_helper_buffer *helper_buffer;
> + struct page *cma_pages;
> + size_t size = PAGE_ALIGN(len);
> + unsigned long nr_pages = size >> PAGE_SHIFT;
> + unsigned long align = get_order(size);
> + struct dma_buf *dmabuf;
> + int ret = -ENOMEM;
> + pgoff_t pg;
> +
> + if (align > CONFIG_CMA_ALIGNMENT)
> + align = CONFIG_CMA_ALIGNMENT;
> +
> + helper_buffer = kzalloc(sizeof(*helper_buffer), GFP_KERNEL);
> + if (!helper_buffer)
> + return -ENOMEM;
> +
> + init_heap_helper_buffer(helper_buffer, cma_heap_free);
> + helper_buffer->flags = heap_flags;
> + helper_buffer->heap = heap;
> + helper_buffer->size = len;
> +
> + cma_pages = cma_alloc(cma_heap->cma, nr_pages, align, false);
> + if (!cma_pages)
> + goto free_buf;
> +
> + if (PageHighMem(cma_pages)) {
> + unsigned long nr_clear_pages = nr_pages;
> + struct page *page = cma_pages;
> +
> + while (nr_clear_pages > 0) {
> + void *vaddr = kmap_atomic(page);
> +
> + memset(vaddr, 0, PAGE_SIZE);
> + kunmap_atomic(vaddr);
> + page++;
> + nr_clear_pages--;
> + }
> + } else {
> + memset(page_address(cma_pages), 0, size);
> + }
> +
> + helper_buffer->pagecount = nr_pages;
> + helper_buffer->pages = kmalloc_array(helper_buffer->pagecount,
> + sizeof(*helper_buffer->pages),
> + GFP_KERNEL);
> + if (!helper_buffer->pages) {
> + ret = -ENOMEM;
> + goto free_cma;
> + }
> +
> + for (pg = 0; pg < helper_buffer->pagecount; pg++) {
> + helper_buffer->pages[pg] = &cma_pages[pg];
> + if (!helper_buffer->pages[pg])
Is this ever really possible? If cma_pages is non-NULL (which you
check earlier), then only if the pointer arithmetic overflows right?
If it's just redundant, then you could remove it (and in that case add
my r-b). But maybe you meant to check something else?
Cheers,
-Brian
