On 05/07/2018 07:51 AM, Nathan Chancellor wrote:
On Mon, May 07, 2018 at 06:46:23AM -0700, Laura Abbott wrote:
On 05/06/2018 06:43 PM, Nathan Chancellor wrote:
Hi everyone,
I compiled the ion driver with W=1 where I encountered the two following
warnings and I had a couple of questions about solving them.
1.
drivers/staging/android/ion/ion.c: In function ‘ion_dma_buf_begin_cpu_access’:
drivers/staging/android/ion/ion.c:316:8: warning: variable ‘vaddr’ set but not used [-Wunused-but-set-variable]
which concerns the following function:
static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
enum dma_data_direction direction)
{
struct ion_buffer *buffer = dmabuf->priv;
void *vaddr;
struct ion_dma_buf_attachment *a;
/*
* TODO: Move this elsewhere because we don't always need a vaddr
*/
if (buffer->heap->ops->map_kernel) {
mutex_lock(&buffer->lock);
vaddr = ion_buffer_kmap_get(buffer);
mutex_unlock(&buffer->lock);
}
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);
}
mutex_unlock(&buffer->lock);
return 0;
}
Can vaddr be removed and just have ion_buffer_kmap_get remain, since it is never used again in the function?
I think a better solution is to check the return value of vaddr
and error out if it fails.
Something like this? I was unsure if -ENOMEM or -EINVAL was a better
error return code in this context.
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index d10b60fe4a29..d1c149bb15c3 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -315,6 +315,7 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
struct ion_buffer *buffer = dmabuf->priv;
void *vaddr;
struct ion_dma_buf_attachment *a;
+ int ret = 0;
/*
* TODO: Move this elsewhere because we don't always need a vaddr
@@ -322,6 +323,10 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
if (buffer->heap->ops->map_kernel) {
mutex_lock(&buffer->lock);
vaddr = ion_buffer_kmap_get(buffer);
+ if (IS_ERR(vaddr)) {
+ ret = -ENOMEM;
A better practice is to just return the error that's returned
ret = PTR_ERR(vaddr);
Other than that looks okay for submission as a patch.
Thanks,
Laura
+ goto unlock;
+ }
mutex_unlock(&buffer->lock);
}
@@ -330,9 +335,10 @@ static int ion_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
dma_sync_sg_for_cpu(a->dev, a->table->sgl, a->table->nents,
direction);
}
- mutex_unlock(&buffer->lock);
- return 0;
+unlock:
+ mutex_unlock(&buffer->lock);
+ return ret;
}
static int ion_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
2.
drivers/staging/android/ion/ion_carveout_heap.c:106:18: warning: no previous prototype for ‘ion_carveout_heap_create’ [-Wmissing-prototypes]
drivers/staging/android/ion/ion_chunk_heap.c:111:18: warning: no previous prototype for ‘ion_chunk_heap_create’ [-Wmissing-prototypes]
It appears neither of these functions are used since commit 2f87f50b2340
("staging: android: ion: Rework heap registration/enumeration"). Ultimately,
removing the whole file fixes this warning; is there still a need to rework
them or can they be removed?
I'd still like to delete it. I haven't seen anyone come out to re-work it.
That said, I think my preference is still to keep it for now until
we do another round of updates.
Understood, I figured it probably isn't wise to remove stuff in the
middle of a release cycle but hey, never know. I will leave it alone
for now.
Thanks!
Nathan
Thanks for looking at these.
Laura
If any part of this email was formatted incorrectly or could be done better,
please let me know!
Thank you,
Nathan Chancellor
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
