Re: [RFC][PATCH 2/5 v2] dma-buf: heaps: Add heap helpers

From: Brian Starkey <Brian.Starkey@arm.com>
To: John Stultz <john.stultz@linaro.org>
Cc: lkml <linux-kernel@vger.kernel.org>,
	Laura Abbott <labbott@redhat.com>,
	Benjamin Gaignard <benjamin.gaignard@linaro.org>,
	Greg KH <gregkh@linuxfoundation.org>,
	Sumit Semwal <sumit.semwal@linaro.org>,
	Liam Mark <lmark@codeaurora.org>, "Andrew F . Davis" <afd@ti.com>,
	Chenbo Feng <fengc@google.com>,
	Alistair Strachan <astrachan@google.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>, nd <nd@arm.com>
Subject: Re: [RFC][PATCH 2/5 v2] dma-buf: heaps: Add heap helpers
Date: Tue, 19 Mar 2019 14:26:05 +0000	[thread overview]
Message-ID: <20190319142604.f6puq3ugxrcxsrjs@DESKTOP-E1NTVVP.localdomain> (raw)
In-Reply-To: <1551819273-640-3-git-send-email-john.stultz@linaro.org>

Hi John,

On Tue, Mar 05, 2019 at 12:54:30PM -0800, John Stultz wrote:

...

> +
> +void dma_heap_buffer_destroy(struct dma_heap_buffer *heap_buffer)
> +{
> +	struct heap_helper_buffer *buffer = to_helper_buffer(heap_buffer);
> +
> +	if (buffer->kmap_cnt > 0) {
> +		pr_warn_once("%s: buffer still mapped in the kernel\n",
> +			     __func__);

Could be worth something louder like a full WARN.

> +		vunmap(buffer->vaddr);
> +	}
> +
> +	buffer->free(buffer);
> +}
> +

...

> +
> +static void *dma_heap_dma_buf_kmap(struct dma_buf *dmabuf,
> +					unsigned long offset)
> +{
> +	struct dma_heap_buffer *heap_buffer = dmabuf->priv;
> +	struct heap_helper_buffer *buffer = to_helper_buffer(heap_buffer);
> +
> +	return buffer->vaddr + offset * PAGE_SIZE;

I think it'd be good to check for NULL vaddr and return NULL in that
case. Less chance of an invalid pointer being accidentally used then.

Thanks,
-Brian