From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Andrew F. Davis" <afd@ti.com>
Subject: Re: [RFC][PATCH 2/5 v2] dma-buf: heaps: Add heap helpers
Date: Thu, 21 Mar 2019 15:35:10 -0500
Message-ID: <9be37cc0-6eae-9bae-d8ee-89a91d7a4d89@ti.com>
References: <1551819273-640-1-git-send-email-john.stultz@linaro.org>
 <1551819273-640-3-git-send-email-john.stultz@linaro.org>
 <20190319142604.f6puq3ugxrcxsrjs@DESKTOP-E1NTVVP.localdomain>
Mime-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Return-path: <linux-kernel-owner@vger.kernel.org>
In-Reply-To: <20190319142604.f6puq3ugxrcxsrjs@DESKTOP-E1NTVVP.localdomain>
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
To: Brian Starkey <Brian.Starkey@arm.com>, 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>, Chenbo Feng <fengc@google.com>, Alistair Strachan <astrachan@google.com>, "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>, nd <nd@arm.com>
List-Id: dri-devel@lists.freedesktop.org

On 3/19/19 9:26 AM, Brian Starkey wrote:
> 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.
> 

Why do we assume vaddr is set at all here? I'm guessing we expected
dma_heap_map_kernel to have been called, that is not going to always be
the case. kmap should perform it's own single page kmap here and not
rely on the clunky full buffer vmap (which is probably broken on 32bit
systems here when the buffers are large).

Andrew

> Thanks,
> -Brian
>