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;
+ 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
