Re: [RFC][PATCH 4/6] staging: android: ion: Pull out ion ioctls to a separate file

[Liviu Dudau]

On Mon, Jun 06, 2016 at 11:23:31AM -0700, Laura Abbott wrote:
> 
> The number of Ion ioctls may continue to grow along with necessary
> validation. Pull it out into a separate file for easier management
> and review.
> 
> Signed-off-by: Laura Abbott <labbott@xxxxxxxxxx>

Reviewed-by: Liviu Dudau <Liviu.Dudau@xxxxxxx>

> ---
>  drivers/staging/android/ion/Makefile    |   3 +-
>  drivers/staging/android/ion/ion-ioctl.c | 144 ++++++++++++++++++++++
>  drivers/staging/android/ion/ion.c       | 208 +-------------------------------
>  drivers/staging/android/ion/ion_priv.h  |  92 ++++++++++++++
>  4 files changed, 244 insertions(+), 203 deletions(-)
>  create mode 100644 drivers/staging/android/ion/ion-ioctl.c
> 
> diff --git a/drivers/staging/android/ion/Makefile b/drivers/staging/android/ion/Makefile
> index 18cc2aa..376c2b2 100644
> --- a/drivers/staging/android/ion/Makefile
> +++ b/drivers/staging/android/ion/Makefile
> @@ -1,4 +1,5 @@
> -obj-$(CONFIG_ION) +=	ion.o ion_heap.o ion_page_pool.o ion_system_heap.o \
> +obj-$(CONFIG_ION) +=	ion.o ion-ioctl.o ion_heap.o \
> +			ion_page_pool.o ion_system_heap.o \
>  			ion_carveout_heap.o ion_chunk_heap.o ion_cma_heap.o
>  obj-$(CONFIG_ION_TEST) += ion_test.o
>  ifdef CONFIG_COMPAT
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> new file mode 100644
> index 0000000..341ba7d
> --- /dev/null
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -0,0 +1,144 @@
> +/*
> + *
> + * Copyright (C) 2011 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/file.h>
> +#include <linux/fs.h>
> +#include <linux/uaccess.h>
> +
> +#include "ion.h"
> +#include "ion_priv.h"
> +#include "compat_ion.h"
> +
> +/* fix up the cases where the ioctl direction bits are incorrect */
> +static unsigned int ion_ioctl_dir(unsigned int cmd)
> +{
> +	switch (cmd) {
> +	case ION_IOC_SYNC:
> +	case ION_IOC_FREE:
> +	case ION_IOC_CUSTOM:
> +		return _IOC_WRITE;
> +	default:
> +		return _IOC_DIR(cmd);
> +	}
> +}
> +
> +long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +	struct ion_client *client = filp->private_data;
> +	struct ion_device *dev = client->dev;
> +	struct ion_handle *cleanup_handle = NULL;
> +	int ret = 0;
> +	unsigned int dir;
> +
> +	union {
> +		struct ion_fd_data fd;
> +		struct ion_allocation_data allocation;
> +		struct ion_handle_data handle;
> +		struct ion_custom_data custom;
> +	} data;
> +
> +	dir = ion_ioctl_dir(cmd);
> +
> +	if (_IOC_SIZE(cmd) > sizeof(data))
> +		return -EINVAL;
> +
> +	if (dir & _IOC_WRITE)
> +		if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
> +			return -EFAULT;
> +
> +	switch (cmd) {
> +	case ION_IOC_ALLOC:
> +	{
> +		struct ion_handle *handle;
> +
> +		handle = ion_alloc(client, data.allocation.len,
> +						data.allocation.align,
> +						data.allocation.heap_id_mask,
> +						data.allocation.flags);
> +		if (IS_ERR(handle))
> +			return PTR_ERR(handle);
> +
> +		data.allocation.handle = handle->id;
> +
> +		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:
> +	case ION_IOC_MAP:
> +	{
> +		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_IMPORT:
> +	{
> +		struct ion_handle *handle;
> +
> +		handle = ion_import_dma_buf_fd(client, data.fd.fd);
> +		if (IS_ERR(handle))
> +			ret = PTR_ERR(handle);
> +		else
> +			data.handle.handle = handle->id;
> +		break;
> +	}
> +	case ION_IOC_SYNC:
> +	{
> +		ret = ion_sync_for_device(client, data.fd.fd);
> +		break;
> +	}
> +	case ION_IOC_CUSTOM:
> +	{
> +		if (!dev->custom_ioctl)
> +			return -ENOTTY;
> +		ret = dev->custom_ioctl(client, data.custom.cmd,
> +						data.custom.arg);
> +		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);
> +			return -EFAULT;
> +		}
> +	}
> +	return ret;
> +}
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 739060f..129707f 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -41,81 +41,6 @@
>  #include "ion_priv.h"
>  #include "compat_ion.h"
>  
> -/**
> - * struct ion_device - the metadata of the ion device node
> - * @dev:		the actual misc device
> - * @buffers:		an rb tree of all the existing buffers
> - * @buffer_lock:	lock protecting the tree of buffers
> - * @lock:		rwsem protecting the tree of heaps and clients
> - * @heaps:		list of all the heaps in the system
> - * @user_clients:	list of all the clients created from userspace
> - */
> -struct ion_device {
> -	struct miscdevice dev;
> -	struct rb_root buffers;
> -	struct mutex buffer_lock;
> -	struct rw_semaphore lock;
> -	long (*custom_ioctl)(struct ion_client *client, unsigned int cmd,
> -			     unsigned long arg);
> -	struct rb_root clients;
> -	struct dentry *debug_root;
> -	struct dentry *heaps_debug_root;
> -	struct dentry *clients_debug_root;
> -	struct idr idr;
> -	int heap_cnt;
> -};
> -
> -/**
> - * struct ion_client - a process/hw block local address space
> - * @node:		node in the tree of all clients
> - * @dev:		backpointer to ion device
> - * @handles:		an rb tree of all the handles in this client
> - * @idr:		an idr space for allocating handle ids
> - * @lock:		lock protecting the tree of handles
> - * @name:		used for debugging
> - * @display_name:	used for debugging (unique version of @name)
> - * @display_serial:	used for debugging (to make display_name unique)
> - * @task:		used for debugging
> - *
> - * A client represents a list of buffers this client may access.
> - * The mutex stored here is used to protect both handles tree
> - * as well as the handles themselves, and should be held while modifying either.
> - */
> -struct ion_client {
> -	struct rb_node node;
> -	struct ion_device *dev;
> -	struct rb_root handles;
> -	struct idr idr;
> -	struct mutex lock;
> -	const char *name;
> -	char *display_name;
> -	int display_serial;
> -	struct task_struct *task;
> -	pid_t pid;
> -	struct dentry *debug_root;
> -};
> -
> -/**
> - * ion_handle - a client local reference to a buffer
> - * @ref:		reference count
> - * @client:		back pointer to the client the buffer resides in
> - * @buffer:		pointer to the buffer
> - * @node:		node in the client's handle rbtree
> - * @kmap_cnt:		count of times this client has mapped to kernel
> - * @id:			client-unique id allocated by client->idr
> - *
> - * Modifications to node, map_cnt or mapping should be protected by the
> - * lock in the client.  Other fields are never changed after initialization.
> - */
> -struct ion_handle {
> -	struct kref ref;
> -	struct ion_client *client;
> -	struct ion_buffer *buffer;
> -	struct rb_node node;
> -	unsigned int kmap_cnt;
> -	int id;
> -};
> -
>  bool ion_buffer_fault_user_mappings(struct ion_buffer *buffer)
>  {
>  	return (buffer->flags & ION_FLAG_CACHED) &&
> @@ -388,7 +313,7 @@ static void ion_handle_get(struct ion_handle *handle)
>  	kref_get(&handle->ref);
>  }
>  
> -static int ion_handle_put_nolock(struct ion_handle *handle)
> +int ion_handle_put_nolock(struct ion_handle *handle)
>  {
>  	int ret;
>  
> @@ -397,7 +322,7 @@ static int ion_handle_put_nolock(struct ion_handle *handle)
>  	return ret;
>  }
>  
> -static int ion_handle_put(struct ion_handle *handle)
> +int ion_handle_put(struct ion_handle *handle)
>  {
>  	struct ion_client *client = handle->client;
>  	int ret;
> @@ -427,7 +352,7 @@ static struct ion_handle *ion_handle_lookup(struct ion_client *client,
>  	return ERR_PTR(-EINVAL);
>  }
>  
> -static struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
> +struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
>  						int id)
>  {
>  	struct ion_handle *handle;
> @@ -439,7 +364,7 @@ static struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
>  	return handle ? handle : ERR_PTR(-EINVAL);
>  }
>  
> -static struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
> +struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
>  					       int id)
>  {
>  	struct ion_handle *handle;
> @@ -570,7 +495,7 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
>  }
>  EXPORT_SYMBOL(ion_alloc);
>  
> -static void ion_free_nolock(struct ion_client *client, struct ion_handle *handle)
> +void ion_free_nolock(struct ion_client *client, struct ion_handle *handle)
>  {
>  	bool valid_handle;
>  
> @@ -1294,7 +1219,7 @@ struct ion_handle *ion_import_dma_buf_fd(struct ion_client *client, int fd)
>  }
>  EXPORT_SYMBOL(ion_import_dma_buf_fd);
>  
> -static int ion_sync_for_device(struct ion_client *client, int fd)
> +int ion_sync_for_device(struct ion_client *client, int fd)
>  {
>  	struct dma_buf *dmabuf;
>  	struct ion_buffer *buffer;
> @@ -1318,127 +1243,6 @@ static int ion_sync_for_device(struct ion_client *client, int fd)
>  	return 0;
>  }
>  
> -/* fix up the cases where the ioctl direction bits are incorrect */
> -static unsigned int ion_ioctl_dir(unsigned int cmd)
> -{
> -	switch (cmd) {
> -	case ION_IOC_SYNC:
> -	case ION_IOC_FREE:
> -	case ION_IOC_CUSTOM:
> -		return _IOC_WRITE;
> -	default:
> -		return _IOC_DIR(cmd);
> -	}
> -}
> -
> -static long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> -{
> -	struct ion_client *client = filp->private_data;
> -	struct ion_device *dev = client->dev;
> -	struct ion_handle *cleanup_handle = NULL;
> -	int ret = 0;
> -	unsigned int dir;
> -
> -	union {
> -		struct ion_fd_data fd;
> -		struct ion_allocation_data allocation;
> -		struct ion_handle_data handle;
> -		struct ion_custom_data custom;
> -	} data;
> -
> -	dir = ion_ioctl_dir(cmd);
> -
> -	if (_IOC_SIZE(cmd) > sizeof(data))
> -		return -EINVAL;
> -
> -	if (dir & _IOC_WRITE)
> -		if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
> -			return -EFAULT;
> -
> -	switch (cmd) {
> -	case ION_IOC_ALLOC:
> -	{
> -		struct ion_handle *handle;
> -
> -		handle = ion_alloc(client, data.allocation.len,
> -						data.allocation.align,
> -						data.allocation.heap_id_mask,
> -						data.allocation.flags);
> -		if (IS_ERR(handle))
> -			return PTR_ERR(handle);
> -
> -		data.allocation.handle = handle->id;
> -
> -		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:
> -	case ION_IOC_MAP:
> -	{
> -		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_IMPORT:
> -	{
> -		struct ion_handle *handle;
> -
> -		handle = ion_import_dma_buf_fd(client, data.fd.fd);
> -		if (IS_ERR(handle))
> -			ret = PTR_ERR(handle);
> -		else
> -			data.handle.handle = handle->id;
> -		break;
> -	}
> -	case ION_IOC_SYNC:
> -	{
> -		ret = ion_sync_for_device(client, data.fd.fd);
> -		break;
> -	}
> -	case ION_IOC_CUSTOM:
> -	{
> -		if (!dev->custom_ioctl)
> -			return -ENOTTY;
> -		ret = dev->custom_ioctl(client, data.custom.cmd,
> -						data.custom.arg);
> -		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);
> -			return -EFAULT;
> -		}
> -	}
> -	return ret;
> -}
> -
>  static int ion_release(struct inode *inode, struct file *file)
>  {
>  	struct ion_client *client = file->private_data;
> diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h
> index 35726ae..b09d751 100644
> --- a/drivers/staging/android/ion/ion_priv.h
> +++ b/drivers/staging/android/ion/ion_priv.h
> @@ -26,6 +26,7 @@
>  #include <linux/sched.h>
>  #include <linux/shrinker.h>
>  #include <linux/types.h>
> +#include <linux/miscdevice.h>
>  
>  #include "ion.h"
>  
> @@ -88,6 +89,81 @@ struct ion_buffer {
>  void ion_buffer_destroy(struct ion_buffer *buffer);
>  
>  /**
> + * struct ion_device - the metadata of the ion device node
> + * @dev:		the actual misc device
> + * @buffers:		an rb tree of all the existing buffers
> + * @buffer_lock:	lock protecting the tree of buffers
> + * @lock:		rwsem protecting the tree of heaps and clients
> + * @heaps:		list of all the heaps in the system
> + * @user_clients:	list of all the clients created from userspace
> + */
> +struct ion_device {
> +	struct miscdevice dev;
> +	struct rb_root buffers;
> +	struct mutex buffer_lock;
> +	struct rw_semaphore lock;
> +	long (*custom_ioctl)(struct ion_client *client, unsigned int cmd,
> +			     unsigned long arg);
> +	struct rb_root clients;
> +	struct dentry *debug_root;
> +	struct dentry *heaps_debug_root;
> +	struct dentry *clients_debug_root;
> +	struct idr idr;
> +	int heap_cnt;
> +};
> +
> +/**
> + * struct ion_client - a process/hw block local address space
> + * @node:		node in the tree of all clients
> + * @dev:		backpointer to ion device
> + * @handles:		an rb tree of all the handles in this client
> + * @idr:		an idr space for allocating handle ids
> + * @lock:		lock protecting the tree of handles
> + * @name:		used for debugging
> + * @display_name:	used for debugging (unique version of @name)
> + * @display_serial:	used for debugging (to make display_name unique)
> + * @task:		used for debugging
> + *
> + * A client represents a list of buffers this client may access.
> + * The mutex stored here is used to protect both handles tree
> + * as well as the handles themselves, and should be held while modifying either.
> + */
> +struct ion_client {
> +	struct rb_node node;
> +	struct ion_device *dev;
> +	struct rb_root handles;
> +	struct idr idr;
> +	struct mutex lock;
> +	const char *name;
> +	char *display_name;
> +	int display_serial;
> +	struct task_struct *task;
> +	pid_t pid;
> +	struct dentry *debug_root;
> +};
> +
> +/**
> + * ion_handle - a client local reference to a buffer
> + * @ref:		reference count
> + * @client:		back pointer to the client the buffer resides in
> + * @buffer:		pointer to the buffer
> + * @node:		node in the client's handle rbtree
> + * @kmap_cnt:		count of times this client has mapped to kernel
> + * @id:			client-unique id allocated by client->idr
> + *
> + * Modifications to node, map_cnt or mapping should be protected by the
> + * lock in the client.  Other fields are never changed after initialization.
> + */
> +struct ion_handle {
> +	struct kref ref;
> +	struct ion_client *client;
> +	struct ion_buffer *buffer;
> +	struct rb_node node;
> +	unsigned int kmap_cnt;
> +	int id;
> +};
> +
> +/**
>   * struct ion_heap_ops - ops to operate on a given heap
>   * @allocate:		allocate memory
>   * @free:		free memory
> @@ -403,4 +479,20 @@ int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t gfp_mask,
>  void ion_pages_sync_for_device(struct device *dev, struct page *page,
>  		size_t size, enum dma_data_direction dir);
>  
> +long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
> +
> +int ion_sync_for_device(struct ion_client *client, int fd);
> +
> +struct ion_handle *ion_handle_get_by_id_nolock(struct ion_client *client,
> +						int id);
> +
> +void ion_free_nolock(struct ion_client *client, struct ion_handle *handle);
> +
> +int ion_handle_put_nolock(struct ion_handle *handle);
> +
> +struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
> +						int id);
> +
> +int ion_handle_put(struct ion_handle *handle);
> +
>  #endif /* _ION_PRIV_H */
> -- 
> 2.5.5
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    Â\_(ã)_/Â