Re: [Linaro-mm-sig] [PATCHv4 10/12] staging: android: ion: Remove ion_handle and ion_client
On Tue, Apr 18, 2017 at 11:27:12AM -0700, Laura Abbott wrote:
> ion_handle was introduced as an abstraction to represent a reference to
> a buffer via an ion_client. As frameworks outside of Ion evolved, the dmabuf
> emerged as the preferred standard for use in the kernel. This has made
> the ion_handle an unnecessary abstraction and prone to race
> conditions. ion_client is also now only used internally. We have enough
> mechanisms for race conditions and leaks already so just drop ion_handle
> and ion_client. This also includes ripping out most of the debugfs
> infrastructure since much of that was tied to clients and handles.
> The debugfs infrastructure was prone to give confusing data (orphaned
> allocations) so it can be replaced with something better if people
> actually want it.
>
> Signed-off-by: Laura Abbott
Yeah I think improving the dma-buf debugfs stuff (maybe with an allocator
callback to dump additional data) is the better option.
Acked-by: Daniel Vetter
> ---
> drivers/staging/android/ion/ion-ioctl.c | 53 +--
> drivers/staging/android/ion/ion.c | 701
> ++--
> drivers/staging/android/ion/ion.h | 77 +---
> drivers/staging/android/uapi/ion.h | 25 +-
> 4 files changed, 51 insertions(+), 805 deletions(-)
>
> diff --git a/drivers/staging/android/ion/ion-ioctl.c
> b/drivers/staging/android/ion/ion-ioctl.c
> index 4e7bf16..76427e4 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -21,9 +21,7 @@
> #include "ion.h"
>
> union ion_ioctl_arg {
> - struct ion_fd_data fd;
> struct ion_allocation_data allocation;
> - struct ion_handle_data handle;
> struct ion_heap_query query;
> };
>
> @@ -48,8 +46,6 @@ static int validate_ioctl_arg(unsigned int cmd, union
> ion_ioctl_arg *arg)
> static unsigned int ion_ioctl_dir(unsigned int cmd)
> {
> switch (cmd) {
> - case ION_IOC_FREE:
> - return _IOC_WRITE;
> default:
> return _IOC_DIR(cmd);
> }
> @@ -57,8 +53,6 @@ static unsigned int ion_ioctl_dir(unsigned int cmd)
>
> long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> {
> - struct ion_client *client = filp->private_data;
> - struct ion_handle *cleanup_handle = NULL;
> int ret = 0;
> unsigned int dir;
> union ion_ioctl_arg data;
> @@ -86,61 +80,28 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
> unsigned long arg)
> switch (cmd) {
> case ION_IOC_ALLOC:
> {
> - struct ion_handle *handle;
> + int fd;
>
> - handle = ion_alloc(client, data.allocation.len,
> + fd = ion_alloc(data.allocation.len,
> data.allocation.heap_id_mask,
> data.allocation.flags);
> - if (IS_ERR(handle))
> - return PTR_ERR(handle);
> + if (fd < 0)
> + return fd;
>
> - data.allocation.handle = handle->id;
> + data.allocation.fd = fd;
>
> - cleanup_handle = handle;
> - break;
> - }
> - case ION_IOC_FREE:
> - {
> - struct ion_handle *handle;
> -
> - mutex_lock(&client->lock);
> - handle = ion_handle_get_by_id_nolock(client,
> - data.handle.handle);
> - if (IS_ERR(handle)) {
> - mutex_unlock(&client->lock);
> - return PTR_ERR(handle);
> - }
> - ion_free_nolock(client, handle);
> - ion_handle_put_nolock(handle);
> - mutex_unlock(&client->lock);
> - break;
> - }
> - case ION_IOC_SHARE:
> - {
> - struct ion_handle *handle;
> -
> - handle = ion_handle_get_by_id(client, data.handle.handle);
> - if (IS_ERR(handle))
> - return PTR_ERR(handle);
> - data.fd.fd = ion_share_dma_buf_fd(client, handle);
> - ion_handle_put(handle);
> - if (data.fd.fd < 0)
> - ret = data.fd.fd;
> break;
> }
> case ION_IOC_HEAP_QUERY:
> - ret = ion_query_heaps(client, &data.query);
> + ret = ion_query_heaps(&data.query);
> break;
> default:
> return -ENOTTY;
> }
>
> if (dir & _IOC_READ) {
> - if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) {
> - if (cleanup_handle)
> - ion_free(client, cleanup_handle);
> + if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd)))
> return -EFAULT;
> - }
> }
> return ret;
> }
> diff --git a/drivers/staging/android/ion/ion.c
> b/drivers/staging/android/ion/ion.c
> index 5a8
[PATCHv4 10/12] staging: android: ion: Remove ion_handle and ion_client
ion_handle was introduced as an abstraction to represent a reference to
a buffer via an ion_client. As frameworks outside of Ion evolved, the dmabuf
emerged as the preferred standard for use in the kernel. This has made
the ion_handle an unnecessary abstraction and prone to race
conditions. ion_client is also now only used internally. We have enough
mechanisms for race conditions and leaks already so just drop ion_handle
and ion_client. This also includes ripping out most of the debugfs
infrastructure since much of that was tied to clients and handles.
The debugfs infrastructure was prone to give confusing data (orphaned
allocations) so it can be replaced with something better if people
actually want it.
Signed-off-by: Laura Abbott
---
drivers/staging/android/ion/ion-ioctl.c | 53 +--
drivers/staging/android/ion/ion.c | 701 ++--
drivers/staging/android/ion/ion.h | 77 +---
drivers/staging/android/uapi/ion.h | 25 +-
4 files changed, 51 insertions(+), 805 deletions(-)
diff --git a/drivers/staging/android/ion/ion-ioctl.c
b/drivers/staging/android/ion/ion-ioctl.c
index 4e7bf16..76427e4 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -21,9 +21,7 @@
#include "ion.h"
union ion_ioctl_arg {
- struct ion_fd_data fd;
struct ion_allocation_data allocation;
- struct ion_handle_data handle;
struct ion_heap_query query;
};
@@ -48,8 +46,6 @@ static int validate_ioctl_arg(unsigned int cmd, union
ion_ioctl_arg *arg)
static unsigned int ion_ioctl_dir(unsigned int cmd)
{
switch (cmd) {
- case ION_IOC_FREE:
- return _IOC_WRITE;
default:
return _IOC_DIR(cmd);
}
@@ -57,8 +53,6 @@ static unsigned int ion_ioctl_dir(unsigned int cmd)
long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
{
- struct ion_client *client = filp->private_data;
- struct ion_handle *cleanup_handle = NULL;
int ret = 0;
unsigned int dir;
union ion_ioctl_arg data;
@@ -86,61 +80,28 @@ long ion_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg)
switch (cmd) {
case ION_IOC_ALLOC:
{
- struct ion_handle *handle;
+ int fd;
- handle = ion_alloc(client, data.allocation.len,
+ fd = ion_alloc(data.allocation.len,
data.allocation.heap_id_mask,
data.allocation.flags);
- if (IS_ERR(handle))
- return PTR_ERR(handle);
+ if (fd < 0)
+ return fd;
- data.allocation.handle = handle->id;
+ data.allocation.fd = fd;
- cleanup_handle = handle;
- break;
- }
- case ION_IOC_FREE:
- {
- struct ion_handle *handle;
-
- mutex_lock(&client->lock);
- handle = ion_handle_get_by_id_nolock(client,
-data.handle.handle);
- if (IS_ERR(handle)) {
- mutex_unlock(&client->lock);
- return PTR_ERR(handle);
- }
- ion_free_nolock(client, handle);
- ion_handle_put_nolock(handle);
- mutex_unlock(&client->lock);
- break;
- }
- case ION_IOC_SHARE:
- {
- struct ion_handle *handle;
-
- handle = ion_handle_get_by_id(client, data.handle.handle);
- if (IS_ERR(handle))
- return PTR_ERR(handle);
- data.fd.fd = ion_share_dma_buf_fd(client, handle);
- ion_handle_put(handle);
- if (data.fd.fd < 0)
- ret = data.fd.fd;
break;
}
case ION_IOC_HEAP_QUERY:
- ret = ion_query_heaps(client, &data.query);
+ ret = ion_query_heaps(&data.query);
break;
default:
return -ENOTTY;
}
if (dir & _IOC_READ) {
- if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) {
- if (cleanup_handle)
- ion_free(client, cleanup_handle);
+ if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd)))
return -EFAULT;
- }
}
return ret;
}
diff --git a/drivers/staging/android/ion/ion.c
b/drivers/staging/android/ion/ion.c
index 5a82bea..9eeb06f 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -90,7 +90,6 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap
*heap,
buffer->heap = heap;
buffer->flags = flags;
- kref_init(&buffer->ref);
ret = heap->ops->allocate(heap,
