A few questions about warnings in the ion driver

[Laura Abbott]

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