Re: [RESEND][PATCH v8 4/5] dma-buf: heaps: Add CMA heap to dmabuf heaps

[Brian Starkey]

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 <labbott@xxxxxxxxxx>
> Cc: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxx>
> Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
> Cc: Liam Mark <lmark@xxxxxxxxxxxxxx>
> Cc: Pratik Patel <pratikp@xxxxxxxxxxxxxx>
> Cc: Brian Starkey <Brian.Starkey@xxxxxxx>
> Cc: Vincent Donnefort <Vincent.Donnefort@xxxxxxx>
> Cc: Sudipto Paul <Sudipto.Paul@xxxxxxx>
> Cc: Andrew F. Davis <afd@xxxxxx>
> Cc: Christoph Hellwig <hch@xxxxxxxxxxxxx>
> Cc: Chenbo Feng <fengc@xxxxxxxxxx>
> Cc: Alistair Strachan <astrachan@xxxxxxxxxx>
> Cc: Hridya Valsaraju <hridya@xxxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Reviewed-by: Benjamin Gaignard <benjamin.gaignard@xxxxxxxxxx>
> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
> ---
> 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