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

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@redhat.com

Reviewed-by: Liviu Dudau Liviu.Dudau@arm.com

---

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