Hi,
This is a follow up to my previous series[1] for Ion ioctls. I've changed the focus slightly based on the feedback. The ID remapping was less useful than I originally thought and without that addition there isn't much benefit to have a new alloc ioctl. The ABI check and query interface still seem beneficial. There was some discussion on where exactly these types of ioctls would be called. I expect the answer will depend on exactly how it's integrated.
Long term, I'd still like to fix the ABI to not be a checklist of botching up ioctls but that focus will come later.
Changes from v1: - Rebased - Dropped RFC - Dropped ID remapping and dependent logic - Changed query logic to only need one ioctl - Fixed alignment of query ioctl structure
[1] http://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg48036....
Laura Abbott (4): staging: android: ion: Drop heap type masks staging: android: ion: Pull out ion ioctls to a separate file staging: android: ion: Add an ioctl for ABI checking staging: android: ion: Add ioctl to query available heaps
drivers/staging/android/ion/Makefile | 3 +- drivers/staging/android/ion/ion-ioctl.c | 188 ++++++++++++++++++++++++++ drivers/staging/android/ion/ion.c | 226 ++++++-------------------------- drivers/staging/android/ion/ion_priv.h | 94 +++++++++++++ drivers/staging/android/uapi/ion.h | 67 +++++++++- 5 files changed, 382 insertions(+), 196 deletions(-) create mode 100644 drivers/staging/android/ion/ion-ioctl.c
There is no advantage to having heap types be a mask. The ion client has long since dropped the mask. Drop the notion of heap type masks as well.
Signed-off-by: Laura Abbott labbott@redhat.com --- drivers/staging/android/uapi/ion.h | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h index 0a8e40f..a9c4e8b 100644 --- a/drivers/staging/android/uapi/ion.h +++ b/drivers/staging/android/uapi/ion.h @@ -44,14 +44,8 @@ enum ion_heap_type { * must be last so device specific heaps always * are at the end of this enum */ - ION_NUM_HEAPS = 16, };
-#define ION_HEAP_SYSTEM_MASK (1 << ION_HEAP_TYPE_SYSTEM) -#define ION_HEAP_SYSTEM_CONTIG_MASK (1 << ION_HEAP_TYPE_SYSTEM_CONTIG) -#define ION_HEAP_CARVEOUT_MASK (1 << ION_HEAP_TYPE_CARVEOUT) -#define ION_HEAP_TYPE_DMA_MASK (1 << ION_HEAP_TYPE_DMA) - #define ION_NUM_HEAP_IDS (sizeof(unsigned int) * 8)
/**
Hi Laura,
On Thu, Sep 01, 2016 at 03:40:41PM -0700, Laura Abbott wrote:
There is no advantage to having heap types be a mask. The ion client has long since dropped the mask. Drop the notion of heap type masks as well.
I know this is the same patch you sent last time, so sorry for not picking this up then - but I'm curious what "The" ion client is here?
Our ion client(s) certainly still use these masks, and it's still used as a mask within ion itself - even if the relationship between a mask and a heap type has been somewhat lost.
Thanks, Brian
Signed-off-by: Laura Abbott labbott@redhat.com
drivers/staging/android/uapi/ion.h | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h index 0a8e40f..a9c4e8b 100644 --- a/drivers/staging/android/uapi/ion.h +++ b/drivers/staging/android/uapi/ion.h @@ -44,14 +44,8 @@ enum ion_heap_type { * must be last so device specific heaps always * are at the end of this enum */
- ION_NUM_HEAPS = 16,
};
-#define ION_HEAP_SYSTEM_MASK (1 << ION_HEAP_TYPE_SYSTEM) -#define ION_HEAP_SYSTEM_CONTIG_MASK (1 << ION_HEAP_TYPE_SYSTEM_CONTIG) -#define ION_HEAP_CARVEOUT_MASK (1 << ION_HEAP_TYPE_CARVEOUT) -#define ION_HEAP_TYPE_DMA_MASK (1 << ION_HEAP_TYPE_DMA)
#define ION_NUM_HEAP_IDS (sizeof(unsigned int) * 8)
/**
2.7.4
On 09/02/2016 06:41 AM, Brian Starkey wrote:
Hi Laura,
On Thu, Sep 01, 2016 at 03:40:41PM -0700, Laura Abbott wrote:
There is no advantage to having heap types be a mask. The ion client has long since dropped the mask. Drop the notion of heap type masks as well.
I know this is the same patch you sent last time, so sorry for not picking this up then - but I'm curious what "The" ion client is here?
ion_client_create used to take a mask to indicate what heap types it could allocate from. This hasn't been the case since 2bb9f5034ec7 ("gpu: ion: Remove heapmask from client"). "The ion client" probably should have been "struct ion_client"
Our ion client(s) certainly still use these masks, and it's still used as a mask within ion itself - even if the relationship between a mask and a heap type has been somewhat lost.
Where is it used in Ion? I don't see it in tree unless I missed something and I'm not eager to keep this around for out of tree code. What's the actual use for this?
Thanks, Brian
Signed-off-by: Laura Abbott labbott@redhat.com
drivers/staging/android/uapi/ion.h | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h index 0a8e40f..a9c4e8b 100644 --- a/drivers/staging/android/uapi/ion.h +++ b/drivers/staging/android/uapi/ion.h @@ -44,14 +44,8 @@ enum ion_heap_type { * must be last so device specific heaps always * are at the end of this enum */
- ION_NUM_HEAPS = 16,
};
-#define ION_HEAP_SYSTEM_MASK (1 << ION_HEAP_TYPE_SYSTEM) -#define ION_HEAP_SYSTEM_CONTIG_MASK (1 << ION_HEAP_TYPE_SYSTEM_CONTIG) -#define ION_HEAP_CARVEOUT_MASK (1 << ION_HEAP_TYPE_CARVEOUT) -#define ION_HEAP_TYPE_DMA_MASK (1 << ION_HEAP_TYPE_DMA)
#define ION_NUM_HEAP_IDS (sizeof(unsigned int) * 8)
/**
2.7.4
Hi,
On Fri, Sep 02, 2016 at 12:36:25PM -0700, Laura Abbott wrote:
On 09/02/2016 06:41 AM, Brian Starkey wrote:
Hi Laura,
On Thu, Sep 01, 2016 at 03:40:41PM -0700, Laura Abbott wrote:
There is no advantage to having heap types be a mask. The ion client has long since dropped the mask. Drop the notion of heap type masks as well.
I know this is the same patch you sent last time, so sorry for not picking this up then - but I'm curious what "The" ion client is here?
ion_client_create used to take a mask to indicate what heap types it could allocate from. This hasn't been the case since 2bb9f5034ec7 ("gpu: ion: Remove heapmask from client"). "The ion client" probably should have been "struct ion_client"
Ah I see, the in-kernel ion_client. Sorry, I completely forgot that even existed (because it's totally useless - how is a driver meant to find the global ion_device?)
Our ion client(s) certainly still use these masks, and it's still used as a mask within ion itself - even if the relationship between a mask and a heap type has been somewhat lost.
Where is it used in Ion? I don't see it in tree unless I missed something and I'm not eager to keep this around for out of tree code. What's the actual use for this?
You're certainly right that these heap-ID-to-allocation-mask macros are unused in the kernel, but I don't really see the reason for removing them - they are convenient (for now).
Example: I'm using the dummy ion driver, and I want to allocate from the SYSTEM_CONTIG heap - the ION_HEAP_SYSTEM_CONTIG_MASK gives me the exact mask I need for that.
It seems your opinion is that heap-IDs are already, and should be, completely decoupled from their type. That sounds like a good idea to me, but it's not true (yet) - again check out the dummy driver.
At the moment, heap-IDs are assigned by ion drivers in any way they see fit. For as long as that stays the case there's always going to be heap-masks hard-coded in UAPI kernel headers (in-tree or not), so removing these particular masks seems a bit fruitless.
I'd rather see driver-assigned heap-IDs disappear completely, and have them assigned by ion core from an idr or something. At that point these macros really *are* meaningless, and I'd be totally fine with removing them (and userspace won't be able to depend on hard-coded allocation masks any more - it will have to use the query ioctl, which I assume is the whole point?).
IMO it's not the right time to remove these macros, because they still have meaning and usefulness.
Cheers, Brian
Thanks, Brian
Signed-off-by: Laura Abbott labbott@redhat.com
drivers/staging/android/uapi/ion.h | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h index 0a8e40f..a9c4e8b 100644 --- a/drivers/staging/android/uapi/ion.h +++ b/drivers/staging/android/uapi/ion.h @@ -44,14 +44,8 @@ enum ion_heap_type { * must be last so device specific heaps always * are at the end of this enum */
- ION_NUM_HEAPS = 16,
};
-#define ION_HEAP_SYSTEM_MASK (1 << ION_HEAP_TYPE_SYSTEM) -#define ION_HEAP_SYSTEM_CONTIG_MASK (1 << ION_HEAP_TYPE_SYSTEM_CONTIG) -#define ION_HEAP_CARVEOUT_MASK (1 << ION_HEAP_TYPE_CARVEOUT) -#define ION_HEAP_TYPE_DMA_MASK (1 << ION_HEAP_TYPE_DMA)
#define ION_NUM_HEAP_IDS (sizeof(unsigned int) * 8)
/**
2.7.4
On 09/05/2016 04:20 AM, Brian Starkey wrote:
Hi,
On Fri, Sep 02, 2016 at 12:36:25PM -0700, Laura Abbott wrote:
On 09/02/2016 06:41 AM, Brian Starkey wrote:
Hi Laura,
On Thu, Sep 01, 2016 at 03:40:41PM -0700, Laura Abbott wrote:
There is no advantage to having heap types be a mask. The ion client has long since dropped the mask. Drop the notion of heap type masks as well.
I know this is the same patch you sent last time, so sorry for not picking this up then - but I'm curious what "The" ion client is here?
ion_client_create used to take a mask to indicate what heap types it could allocate from. This hasn't been the case since 2bb9f5034ec7 ("gpu: ion: Remove heapmask from client"). "The ion client" probably should have been "struct ion_client"
Ah I see, the in-kernel ion_client. Sorry, I completely forgot that even existed (because it's totally useless - how is a driver meant to find the global ion_device?)
Our ion client(s) certainly still use these masks, and it's still used as a mask within ion itself - even if the relationship between a mask and a heap type has been somewhat lost.
Where is it used in Ion? I don't see it in tree unless I missed something and I'm not eager to keep this around for out of tree code. What's the actual use for this?
You're certainly right that these heap-ID-to-allocation-mask macros are unused in the kernel, but I don't really see the reason for removing them - they are convenient (for now).
Example: I'm using the dummy ion driver, and I want to allocate from the SYSTEM_CONTIG heap - the ION_HEAP_SYSTEM_CONTIG_MASK gives me the exact mask I need for that.
It seems your opinion is that heap-IDs are already, and should be, completely decoupled from their type. That sounds like a good idea to me, but it's not true (yet) - again check out the dummy driver.
Good point, I need to clean up the dummy driver to stop using heap types as the id ;)
I get that it's convenient but it's a bad practice to conflate the namespaces.
At the moment, heap-IDs are assigned by ion drivers in any way they see fit. For as long as that stays the case there's always going to be heap-masks hard-coded in UAPI kernel headers (in-tree or not), so removing these particular masks seems a bit fruitless.
It's not fruitless, the concept of type as mask makes no sense. They are two different name spaces and I've found Ion users have a hard time keeping them separate and pass in the heap type mask when using non dummy
I'd rather see driver-assigned heap-IDs disappear completely, and have them assigned by ion core from an idr or something. At that point these macros really *are* meaningless, and I'd be totally fine with removing them (and userspace won't be able to depend on hard-coded allocation masks any more - it will have to use the query ioctl, which I assume is the whole point?).
Ideally yes we'd be able to get rid of the hard coded device IDs. I consider the query ioctl a stepping stone to that, depending on how enthusiastic people are about Ion.
IMO it's not the right time to remove these macros, because they still have meaning and usefulness.
I still think they should be deleted to avoid namespace polution.
Cheers, Brian
Thanks, Brian
Signed-off-by: Laura Abbott labbott@redhat.com
drivers/staging/android/uapi/ion.h | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h index 0a8e40f..a9c4e8b 100644 --- a/drivers/staging/android/uapi/ion.h +++ b/drivers/staging/android/uapi/ion.h @@ -44,14 +44,8 @@ enum ion_heap_type { * must be last so device specific heaps always * are at the end of this enum */
- ION_NUM_HEAPS = 16,
};
-#define ION_HEAP_SYSTEM_MASK (1 << ION_HEAP_TYPE_SYSTEM) -#define ION_HEAP_SYSTEM_CONTIG_MASK (1 << ION_HEAP_TYPE_SYSTEM_CONTIG) -#define ION_HEAP_CARVEOUT_MASK (1 << ION_HEAP_TYPE_CARVEOUT) -#define ION_HEAP_TYPE_DMA_MASK (1 << ION_HEAP_TYPE_DMA)
#define ION_NUM_HEAP_IDS (sizeof(unsigned int) * 8)
/**
2.7.4
On Tue, Sep 06, 2016 at 03:16:52PM -0700, Laura Abbott wrote:
On 09/05/2016 04:20 AM, Brian Starkey wrote:
Hi,
On Fri, Sep 02, 2016 at 12:36:25PM -0700, Laura Abbott wrote:
On 09/02/2016 06:41 AM, Brian Starkey wrote:
Hi Laura,
On Thu, Sep 01, 2016 at 03:40:41PM -0700, Laura Abbott wrote:
There is no advantage to having heap types be a mask. The ion client has long since dropped the mask. Drop the notion of heap type masks as well.
I know this is the same patch you sent last time, so sorry for not picking this up then - but I'm curious what "The" ion client is here?
ion_client_create used to take a mask to indicate what heap types it could allocate from. This hasn't been the case since 2bb9f5034ec7 ("gpu: ion: Remove heapmask from client"). "The ion client" probably should have been "struct ion_client"
Ah I see, the in-kernel ion_client. Sorry, I completely forgot that even existed (because it's totally useless - how is a driver meant to find the global ion_device?)
Our ion client(s) certainly still use these masks, and it's still used as a mask within ion itself - even if the relationship between a mask and a heap type has been somewhat lost.
Where is it used in Ion? I don't see it in tree unless I missed something and I'm not eager to keep this around for out of tree code. What's the actual use for this?
You're certainly right that these heap-ID-to-allocation-mask macros are unused in the kernel, but I don't really see the reason for removing them - they are convenient (for now).
Example: I'm using the dummy ion driver, and I want to allocate from the SYSTEM_CONTIG heap - the ION_HEAP_SYSTEM_CONTIG_MASK gives me the exact mask I need for that.
It seems your opinion is that heap-IDs are already, and should be, completely decoupled from their type. That sounds like a good idea to me, but it's not true (yet) - again check out the dummy driver.
Good point, I need to clean up the dummy driver to stop using heap types as the id ;)
I get that it's convenient but it's a bad practice to conflate the namespaces.
At the moment, heap-IDs are assigned by ion drivers in any way they see fit. For as long as that stays the case there's always going to be heap-masks hard-coded in UAPI kernel headers (in-tree or not), so removing these particular masks seems a bit fruitless.
It's not fruitless, the concept of type as mask makes no sense. They are two different name spaces and I've found Ion users have a hard time keeping them separate and pass in the heap type mask when using non dummy
You can add me to that group :-)
I'd rather see driver-assigned heap-IDs disappear completely, and have them assigned by ion core from an idr or something. At that point these macros really *are* meaningless, and I'd be totally fine with removing them (and userspace won't be able to depend on hard-coded allocation masks any more - it will have to use the query ioctl, which I assume is the whole point?).
Ideally yes we'd be able to get rid of the hard coded device IDs. I consider the query ioctl a stepping stone to that, depending on how enthusiastic people are about Ion.
IMO it's not the right time to remove these macros, because they still have meaning and usefulness.
I still think they should be deleted to avoid namespace polution.
If you nuke type-as-ID in dummy, and put a clear comment on the ion_heap_type enum stating that heap-type and heap-ID are strictly different, then I'm happy. I think without that there'll still be confusion.
Thanks! Brian
Cheers, Brian
Thanks, Brian
Signed-off-by: Laura Abbott labbott@redhat.com
drivers/staging/android/uapi/ion.h | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h index 0a8e40f..a9c4e8b 100644 --- a/drivers/staging/android/uapi/ion.h +++ b/drivers/staging/android/uapi/ion.h @@ -44,14 +44,8 @@ enum ion_heap_type { * must be last so device specific heaps always * are at the end of this enum */
- ION_NUM_HEAPS = 16,
};
-#define ION_HEAP_SYSTEM_MASK (1 << ION_HEAP_TYPE_SYSTEM) -#define ION_HEAP_SYSTEM_CONTIG_MASK (1 << ION_HEAP_TYPE_SYSTEM_CONTIG) -#define ION_HEAP_CARVEOUT_MASK (1 << ION_HEAP_TYPE_CARVEOUT) -#define ION_HEAP_TYPE_DMA_MASK (1 << ION_HEAP_TYPE_DMA)
#define ION_NUM_HEAP_IDS (sizeof(unsigned int) * 8)
/**
2.7.4
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 --- drivers/staging/android/ion/Makefile | 3 +- drivers/staging/android/ion/ion-ioctl.c | 144 ++++++++++++++++++++++ drivers/staging/android/ion/ion.c | 210 ++------------------------------ drivers/staging/android/ion/ion_priv.h | 91 ++++++++++++++ 4 files changed, 244 insertions(+), 204 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 88dd17e..975b48f 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -41,80 +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; - struct plist_head heaps; - 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 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) && @@ -381,7 +307,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;
@@ -390,7 +316,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; @@ -420,7 +346,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; @@ -432,7 +358,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; @@ -545,8 +471,8 @@ 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;
@@ -1224,7 +1150,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; @@ -1248,128 +1174,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 5eed5e2..95df6a9 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"
@@ -83,6 +84,80 @@ 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; + struct plist_head heaps; + 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 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 @@ -378,4 +453,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 */
On Thu, Sep 01, 2016 at 03:40:42PM -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
drivers/staging/android/ion/Makefile | 3 +- drivers/staging/android/ion/ion-ioctl.c | 144 ++++++++++++++++++++++ drivers/staging/android/ion/ion.c | 210 ++------------------------------ drivers/staging/android/ion/ion_priv.h | 91 ++++++++++++++ 4 files changed, 244 insertions(+), 204 deletions(-) create mode 100644 drivers/staging/android/ion/ion-ioctl.c
This patch doesn't apply cleanly to my tree, are we out of sync somehow?
Can you rebase your outstanding patches against my staging-testing branch and resend?
thanks,
greg k-h
On 09/02/2016 05:44 AM, Greg Kroah-Hartman wrote:
On Thu, Sep 01, 2016 at 03:40:42PM -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
drivers/staging/android/ion/Makefile | 3 +- drivers/staging/android/ion/ion-ioctl.c | 144 ++++++++++++++++++++++ drivers/staging/android/ion/ion.c | 210 ++------------------------------ drivers/staging/android/ion/ion_priv.h | 91 ++++++++++++++ 4 files changed, 244 insertions(+), 204 deletions(-) create mode 100644 drivers/staging/android/ion/ion-ioctl.c
This patch doesn't apply cleanly to my tree, are we out of sync somehow?
Can you rebase your outstanding patches against my staging-testing branch and resend?
thanks,
greg k-h
Looks like I missed a rebase with one of the cleanup patches. I'll resend.
Thanks, Laura
The current Ion ioctls lack a good way to tell what ioctls are available. Introduce an ioctl to give an ABI version. This way when the ABI inevitably gets screwed up userspace will have a way to tell what version of the screw up is available.
Signed-off-by: Laura Abbott labbott@redhat.com --- drivers/staging/android/ion/ion-ioctl.c | 53 ++++++++++++++++++++++++++------- drivers/staging/android/uapi/ion.h | 22 ++++++++++++++ 2 files changed, 65 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c index 341ba7d..53b9520 100644 --- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -22,6 +22,29 @@ #include "ion_priv.h" #include "compat_ion.h"
+union ion_ioctl_arg { + struct ion_fd_data fd; + struct ion_allocation_data allocation; + struct ion_handle_data handle; + struct ion_custom_data custom; + struct ion_abi_version abi_version; +}; + +static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) +{ + int ret = 0; + + switch (cmd) { + case ION_IOC_ABI_VERSION: + ret = arg->abi_version.reserved != 0; + break; + default: + break; + } + + return ret ? -EINVAL : 0; +} + /* fix up the cases where the ioctl direction bits are incorrect */ static unsigned int ion_ioctl_dir(unsigned int cmd) { @@ -42,22 +65,27 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) 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; + union ion_ioctl_arg 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; + /* + * The copy_from_user is unconditional here for both read and write + * to do the validate. If there is no write for the ioctl, the + * buffer is cleared + */ + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) + return -EFAULT; + + ret = validate_ioctl_arg(cmd, &data); + if (WARN_ON_ONCE(ret)) + return ret; + + if (!(dir & _IOC_WRITE)) + memset(&data, 0, sizeof(data));
switch (cmd) { case ION_IOC_ALLOC: @@ -129,6 +157,11 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) data.custom.arg); break; } + case ION_IOC_ABI_VERSION: + { + data.abi_version.abi_version = ION_ABI_VERSION; + break; + } default: return -ENOTTY; } diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h index a9c4e8b..7ca4e8b 100644 --- a/drivers/staging/android/uapi/ion.h +++ b/drivers/staging/android/uapi/ion.h @@ -19,6 +19,7 @@
#include <linux/ioctl.h> #include <linux/types.h> +#include <linux/version.h>
typedef int ion_user_handle_t;
@@ -128,6 +129,19 @@ struct ion_custom_data { unsigned long arg; };
+/** + * struct ion_abi_version + * + * @version - current ABI version + */ + +#define ION_ABI_VERSION KERNEL_VERSION(0, 1, 0) + +struct ion_abi_version { + __u32 abi_version; + __u32 reserved; +}; + #define ION_IOC_MAGIC 'I'
/** @@ -194,4 +208,12 @@ struct ion_custom_data { */ #define ION_IOC_CUSTOM _IOWR(ION_IOC_MAGIC, 6, struct ion_custom_data)
+/** + * DOC: ION_IOC_ABI_VERSION - return ABI version + * + * Returns the ABI version of this driver. + */ +#define ION_IOC_ABI_VERSION _IOR(ION_IOC_MAGIC, 8, \ + struct ion_abi_version) + #endif /* _UAPI_LINUX_ION_H */
On Thu, Sep 01, 2016 at 03:40:43PM -0700, Laura Abbott wrote:
The current Ion ioctls lack a good way to tell what ioctls are available. Introduce an ioctl to give an ABI version. This way when the ABI inevitably gets screwed up userspace will have a way to tell what version of the screw up is available.
This worries me. Why do we need this? Shouldn't any "new" abi changes just add on, and not change existing ioctl structure calls? Or worst case, you remove an ioctl and then userspace "knows" that when the call fails.
And who is the major userspace user of this interface? Who controls it? How are we keeping things in sync here?
thanks,
greg k-h
On 09/01/2016 11:10 PM, Greg Kroah-Hartman wrote:
On Thu, Sep 01, 2016 at 03:40:43PM -0700, Laura Abbott wrote:
The current Ion ioctls lack a good way to tell what ioctls are available. Introduce an ioctl to give an ABI version. This way when the ABI inevitably gets screwed up userspace will have a way to tell what version of the screw up is available.
This worries me. Why do we need this? Shouldn't any "new" abi changes just add on, and not change existing ioctl structure calls? Or worst case, you remove an ioctl and then userspace "knows" that when the call fails.
This may be more of an "I wish we had this when some poor decisions were made in the past". There were a couple of instances when the Ion ABI was broken (adding new fields, new ioctl numbers) that were a nightmare to deal with and a similar ioctl would have helped a lot. The botching-ioctls document also made reference to something simliar.
And who is the major userspace user of this interface? Who controls it? How are we keeping things in sync here?
I would expect this to not actually be used until we have breakage. The broken ioctl would then be checked as needed.
Reading all this and thinking some, it sounds like this shouldn't actually be needed so long as we continue to not break the ioctls. I had a thought of this possibly making life easier for out of tree users to eventually convert over but I haven't heard much from actual out of tree users.
I'd like to keep it just to hedge my bets but I also haven't had as much experience maintaining stable ioctls for the long term. If, from others experience, this type of ioctl is actually just more prone to breakage and doesn't help then I don't want to push something that will eventually break.
thanks,
greg k-h
Thanks, Laura
On Thursday, September 1, 2016 3:40:43 PM CEST Laura Abbott wrote:
--- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -22,6 +22,29 @@ #include "ion_priv.h" #include "compat_ion.h" +union ion_ioctl_arg {
- struct ion_fd_data fd;
- struct ion_allocation_data allocation;
- struct ion_handle_data handle;
- struct ion_custom_data custom;
- struct ion_abi_version abi_version;
+};
Are you introducing this, or just clarifying the defintion of the existing interface. For new interfaces, we should not have a union as an ioctl argument. Instead each ioctl command should have one specific structure (or better a scalar argument).
+static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) +{
- int ret = 0;
- switch (cmd) {
- case ION_IOC_ABI_VERSION:
ret = arg->abi_version.reserved != 0;break;- default:
break;- }
- return ret ? -EINVAL : 0;
+}
I agree with Greg, ioctl interfaces should normally not be versioned, the usual way is to try a command and see if it fails or not.
+/**
- struct ion_abi_version
- @version - current ABI version
- */
+#define ION_ABI_VERSION KERNEL_VERSION(0, 1, 0)
+struct ion_abi_version {
- __u32 abi_version;
- __u32 reserved;
+};
This interface doesn't really need a "reserved" field, you could as well use a __u32 by itself. If you ever need a second field, just add a new command number.
Arnd
On 09/02/2016 02:02 AM, Arnd Bergmann wrote:
On Thursday, September 1, 2016 3:40:43 PM CEST Laura Abbott wrote:
--- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -22,6 +22,29 @@ #include "ion_priv.h" #include "compat_ion.h"
+union ion_ioctl_arg {
- struct ion_fd_data fd;
- struct ion_allocation_data allocation;
- struct ion_handle_data handle;
- struct ion_custom_data custom;
- struct ion_abi_version abi_version;
+};
Are you introducing this, or just clarifying the defintion of the existing interface. For new interfaces, we should not have a union as an ioctl argument. Instead each ioctl command should have one specific structure (or better a scalar argument).
This was just a structure inside ion_ioctl. I pulled it out for the validate function. It's not an actual argument to any ioctl from userspace. ion_ioctl copies using _IOC_SIZE.
+static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) +{
- int ret = 0;
- switch (cmd) {
- case ION_IOC_ABI_VERSION:
ret = arg->abi_version.reserved != 0;break;- default:
break;- }
- return ret ? -EINVAL : 0;
+}
I agree with Greg, ioctl interfaces should normally not be versioned, the usual way is to try a command and see if it fails or not.
The concern was trying ioctls that wouldn't actually fail or would have some other unexpected side effect.
My conclusion from the other thread was that assuming we don't botch up adding new ioctls in the future or make incompatible changes to these in the future we shouldn't technically need it. I was still trying to hedge my bets against the future but that might just be making the problem worse?
+/**
- struct ion_abi_version
- @version - current ABI version
- */
+#define ION_ABI_VERSION KERNEL_VERSION(0, 1, 0)
+struct ion_abi_version {
- __u32 abi_version;
- __u32 reserved;
+};
This interface doesn't really need a "reserved" field, you could as well use a __u32 by itself. If you ever need a second field, just add a new command number.
The botching-ioctls.txt document suggested everything should be aligned to 64-bits. Was I interpreting that too literally?
Arnd
Thanks, Laura
On Friday, September 2, 2016 1:33:44 PM CEST Laura Abbott wrote:
On 09/02/2016 02:02 AM, Arnd Bergmann wrote:
On Thursday, September 1, 2016 3:40:43 PM CEST Laura Abbott wrote:
--- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -22,6 +22,29 @@ #include "ion_priv.h" #include "compat_ion.h"
+union ion_ioctl_arg {
- struct ion_fd_data fd;
- struct ion_allocation_data allocation;
- struct ion_handle_data handle;
- struct ion_custom_data custom;
- struct ion_abi_version abi_version;
+};
Are you introducing this, or just clarifying the defintion of the existing interface. For new interfaces, we should not have a union as an ioctl argument. Instead each ioctl command should have one specific structure (or better a scalar argument).
This was just a structure inside ion_ioctl. I pulled it out for the validate function. It's not an actual argument to any ioctl from userspace. ion_ioctl copies using _IOC_SIZE.
Ok, got it. This is fine from an interface point of view, just a bit unusual in the way it's written.
+static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) +{
- int ret = 0;
- switch (cmd) {
- case ION_IOC_ABI_VERSION:
ret = arg->abi_version.reserved != 0;break;- default:
break;- }
- return ret ? -EINVAL : 0;
+}
I agree with Greg, ioctl interfaces should normally not be versioned, the usual way is to try a command and see if it fails or not.
The concern was trying ioctls that wouldn't actually fail or would have some other unexpected side effect.
My conclusion from the other thread was that assuming we don't botch up adding new ioctls in the future or make incompatible changes to these in the future we shouldn't technically need it. I was still trying to hedge my bets against the future but that might just be making the problem worse?
We've had a number of cases where versioned ABIs just didn't work out.
The versions are either used to distinguish incompatible APIs, which we should avoid to start with, or they are used for backwards-compatible extensions that you should detect by checking whether an ioctl succeeds. Relying on the API version number breaks if you get a partial backport of features from a later version, and it's unclear what a user space tool should expect when the kernel reports a newer ABI than it knows.
I think the wireless extensions and KVM are examples of versioned APIs that turned out to make things more complicated than they would have been otherwise.
+/**
- struct ion_abi_version
- @version - current ABI version
- */
+#define ION_ABI_VERSION KERNEL_VERSION(0, 1, 0)
+struct ion_abi_version {
- __u32 abi_version;
- __u32 reserved;
+};
This interface doesn't really need a "reserved" field, you could as well use a __u32 by itself. If you ever need a second field, just add a new command number.
The botching-ioctls.txt document suggested everything should be aligned to 64-bits. Was I interpreting that too literally?
I didn't even know that file existed ;-)
I'm pretty sure the paragraph refers to the problem of x86 of having a structure like
struct ioctl_arg { __u64 first; __u32 second; };
which is 12 bytes long on x86, but 16 bytes long including implied padding on all 64-bit architectures and most (maybe all) 32-bit ones other than x86.
If there is no 64-bit member in the struct, there is no need for padding at the end.
Arnd
On 09/02/2016 02:33 PM, Arnd Bergmann wrote:
On Friday, September 2, 2016 1:33:44 PM CEST Laura Abbott wrote:
On 09/02/2016 02:02 AM, Arnd Bergmann wrote:
On Thursday, September 1, 2016 3:40:43 PM CEST Laura Abbott wrote:
--- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -22,6 +22,29 @@ #include "ion_priv.h" #include "compat_ion.h"
+union ion_ioctl_arg {
- struct ion_fd_data fd;
- struct ion_allocation_data allocation;
- struct ion_handle_data handle;
- struct ion_custom_data custom;
- struct ion_abi_version abi_version;
+};
Are you introducing this, or just clarifying the defintion of the existing interface. For new interfaces, we should not have a union as an ioctl argument. Instead each ioctl command should have one specific structure (or better a scalar argument).
This was just a structure inside ion_ioctl. I pulled it out for the validate function. It's not an actual argument to any ioctl from userspace. ion_ioctl copies using _IOC_SIZE.
Ok, got it. This is fine from an interface point of view, just a bit unusual in the way it's written.
+static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) +{
- int ret = 0;
- switch (cmd) {
- case ION_IOC_ABI_VERSION:
ret = arg->abi_version.reserved != 0;break;- default:
break;- }
- return ret ? -EINVAL : 0;
+}
I agree with Greg, ioctl interfaces should normally not be versioned, the usual way is to try a command and see if it fails or not.
The concern was trying ioctls that wouldn't actually fail or would have some other unexpected side effect.
My conclusion from the other thread was that assuming we don't botch up adding new ioctls in the future or make incompatible changes to these in the future we shouldn't technically need it. I was still trying to hedge my bets against the future but that might just be making the problem worse?
We've had a number of cases where versioned ABIs just didn't work out.
The versions are either used to distinguish incompatible APIs, which we should avoid to start with, or they are used for backwards-compatible extensions that you should detect by checking whether an ioctl succeeds. Relying on the API version number breaks if you get a partial backport of features from a later version, and it's unclear what a user space tool should expect when the kernel reports a newer ABI than it knows.
I think the wireless extensions and KVM are examples of versioned APIs that turned out to make things more complicated than they would have been otherwise.
Okay it sounds like the answer is to strive to never run into a case where versioned ioctls are necessary. Shouldn't be too hard, right? ;)
+/**
- struct ion_abi_version
- @version - current ABI version
- */
+#define ION_ABI_VERSION KERNEL_VERSION(0, 1, 0)
+struct ion_abi_version {
- __u32 abi_version;
- __u32 reserved;
+};
This interface doesn't really need a "reserved" field, you could as well use a __u32 by itself. If you ever need a second field, just add a new command number.
The botching-ioctls.txt document suggested everything should be aligned to 64-bits. Was I interpreting that too literally?
I didn't even know that file existed ;-)
I'm pretty sure the paragraph refers to the problem of x86 of having a structure like
struct ioctl_arg { __u64 first; __u32 second; };
which is 12 bytes long on x86, but 16 bytes long including implied padding on all 64-bit architectures and most (maybe all) 32-bit ones other than x86.
Right, that's the problem it's trying to avoid.
If there is no 64-bit member in the struct, there is no need for padding at the end.
That's what I thought as well. I think I'll submit a patch to the docs clarifying a few things.
Arnd
Thanks, Laura
Ion clients currently lack a good method to determine what heaps are available and what ids they map to. This leads to tight coupling between user and kernel space and headaches. Add a query ioctl to let userspace know the availability of heaps.
Signed-off-by: Laura Abbott labbott@redhat.com --- drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++ drivers/staging/android/ion/ion.c | 44 +++++++++++++++++++++++++++++++++ drivers/staging/android/ion/ion_priv.h | 3 +++ drivers/staging/android/uapi/ion.h | 39 +++++++++++++++++++++++++++++ 4 files changed, 97 insertions(+)
diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c index 53b9520..e76d517 100644 --- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -28,6 +28,7 @@ union ion_ioctl_arg { struct ion_handle_data handle; struct ion_custom_data custom; struct ion_abi_version abi_version; + struct ion_heap_query query; };
static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) @@ -38,6 +39,11 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) case ION_IOC_ABI_VERSION: ret = arg->abi_version.reserved != 0; break; + case ION_IOC_HEAP_QUERY: + ret = arg->query.reserved0 != 0; + ret |= arg->query.reserved1 != 0; + ret |= arg->query.reserved2 != 0; + break; default: break; } @@ -162,6 +168,11 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) data.abi_version.abi_version = ION_ABI_VERSION; break; } + case ION_IOC_HEAP_QUERY: + { + ret = ion_query_heaps(client, &data.query); + break; + } default: return -ENOTTY; } diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index 975b48f..91b765c 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -1174,6 +1174,49 @@ int ion_sync_for_device(struct ion_client *client, int fd) return 0; }
+int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query) +{ + struct ion_device *dev = client->dev; + struct ion_heap_data __user *buffer = + (struct ion_heap_data __user *)query->heaps; + int ret = -EINVAL, cnt = 0, max_cnt; + struct ion_heap *heap; + struct ion_heap_data hdata; + + memset(&hdata, 0, sizeof(hdata)); + + down_read(&dev->lock); + if (!buffer) { + query->cnt = dev->heap_cnt; + ret = 0; + goto out; + } + + if (query->cnt <= 0) + goto out; + + max_cnt = query->cnt; + + plist_for_each_entry(heap, &dev->heaps, node) { + strncpy(hdata.name, heap->name, MAX_HEAP_NAME); + hdata.name[sizeof(hdata.name) - 1] = '\0'; + hdata.type = heap->type; + hdata.heap_id = heap->id; + + ret = copy_to_user(&buffer[cnt], + &hdata, sizeof(hdata)); + + cnt++; + if (cnt >= max_cnt) + break; + } + + query->cnt = cnt; +out: + up_read(&dev->lock); + return ret; +} + static int ion_release(struct inode *inode, struct file *file) { struct ion_client *client = file->private_data; @@ -1391,6 +1434,7 @@ void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap) } }
+ dev->heap_cnt++; up_write(&dev->lock); } EXPORT_SYMBOL(ion_device_add_heap); diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h index 95df6a9..0d0c0aa 100644 --- a/drivers/staging/android/ion/ion_priv.h +++ b/drivers/staging/android/ion/ion_priv.h @@ -104,6 +104,7 @@ struct ion_device { struct dentry *debug_root; struct dentry *heaps_debug_root; struct dentry *clients_debug_root; + int heap_cnt; };
/** @@ -469,4 +470,6 @@ struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
int ion_handle_put(struct ion_handle *handle);
+int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query); + #endif /* _ION_PRIV_H */ diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h index 7ca4e8b..112f257 100644 --- a/drivers/staging/android/uapi/ion.h +++ b/drivers/staging/android/uapi/ion.h @@ -137,11 +137,41 @@ struct ion_custom_data {
#define ION_ABI_VERSION KERNEL_VERSION(0, 1, 0)
+#define MAX_HEAP_NAME 32 + struct ion_abi_version { __u32 abi_version; __u32 reserved; };
+/** + * struct ion_heap_data - data about a heap + * @name - first 32 characters of the heap name + * @type - heap type + * @heap_id - heap id for the heap + */ +struct ion_heap_data { + char name[MAX_HEAP_NAME]; + __u32 type; + __u32 heap_id; + __u32 reserved0; + __u32 reserved1; + __u32 reserved2; +}; + +/** + * struct ion_heap_query - collection of data about all heaps + * @cnt - total number of heaps to be copied + * @heaps - buffer to copy heap data + */ +struct ion_heap_query { + __u32 cnt; /* Total number of heaps to be copied */ + __u32 reserved0; /* align to 64bits */ + __u64 heaps; /* buffer to be populated */ + __u32 reserved1; + __u32 reserved2; +}; + #define ION_IOC_MAGIC 'I'
/** @@ -216,4 +246,13 @@ struct ion_abi_version { #define ION_IOC_ABI_VERSION _IOR(ION_IOC_MAGIC, 8, \ struct ion_abi_version)
+/** + * DOC: ION_IOC_HEAP_QUERY - information about available heaps + * + * Takes an ion_heap_query structure and populates information about + * available Ion heaps. + */ +#define ION_IOC_HEAP_QUERY _IOWR(ION_IOC_MAGIC, 12, \ + struct ion_heap_query) + #endif /* _UAPI_LINUX_ION_H */
Hi Laura,
[auto build test WARNING on next-20160825] [cannot apply to staging/staging-testing v4.8-rc4 v4.8-rc3 v4.8-rc2 v4.8-rc4] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] [Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on] [Check https://git-scm.com/docs/git-format-patch for more information]
url: https://github.com/0day-ci/linux/commits/Laura-Abbott/staging-android-ion-Dr... config: mips-allyesconfig (attached as .config) compiler: mips-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/ma... -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=mips
All warnings (new ones prefixed by >>):
drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':
drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
(struct ion_heap_data __user *)query->heaps; ^
vim +1181 drivers/staging/android/ion/ion.c
1165 __func__); 1166 dma_buf_put(dmabuf); 1167 return -EINVAL; 1168 } 1169 buffer = dmabuf->priv; 1170 1171 dma_sync_sg_for_device(NULL, buffer->sg_table->sgl, 1172 buffer->sg_table->nents, DMA_BIDIRECTIONAL); 1173 dma_buf_put(dmabuf); 1174 return 0; 1175 } 1176 1177 int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query) 1178 { 1179 struct ion_device *dev = client->dev; 1180 struct ion_heap_data __user *buffer =
1181 (struct ion_heap_data __user *)query->heaps;
1182 int ret = -EINVAL, cnt = 0, max_cnt; 1183 struct ion_heap *heap; 1184 struct ion_heap_data hdata; 1185 1186 memset(&hdata, 0, sizeof(hdata)); 1187 1188 down_read(&dev->lock); 1189 if (!buffer) {
--- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 09/01/2016 04:44 PM, kbuild test robot wrote:
Hi Laura,
[auto build test WARNING on next-20160825] [cannot apply to staging/staging-testing v4.8-rc4 v4.8-rc3 v4.8-rc2 v4.8-rc4] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] [Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on] [Check https://git-scm.com/docs/git-format-patch for more information]
url: https://github.com/0day-ci/linux/commits/Laura-Abbott/staging-android-ion-Dr... config: mips-allyesconfig (attached as .config) compiler: mips-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/ma... -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=mips
All warnings (new ones prefixed by >>):
drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':
drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
(struct ion_heap_data __user *)query->heaps; ^
botching-up-ioctls.txt says:
* Pointers are __u64, cast from/to a uintprt_t on the userspace side and from/to a void __user * in the kernel. Try really hard not to delay this conversion or worse, fiddle the raw __u64 through your code since that diminishes the checking tools like sparse can provide.
This doesn't seem like it will work on 32-bit systems since you'll end up with the warning above. Is there a better option or did I misunderstand how that is supposed to work?
Thanks, Laura
On Friday, September 2, 2016 2:27:21 PM CEST Laura Abbott wrote:
All warnings (new ones prefixed by >>):
drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':
drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
(struct ion_heap_data __user *)query->heaps; ^botching-up-ioctls.txt says:
- Pointers are __u64, cast from/to a uintprt_t on the userspace side and from/to a void __user * in the kernel. Try really hard not to delay this conversion or worse, fiddle the raw __u64 through your code since that diminishes the checking tools like sparse can provide.
This doesn't seem like it will work on 32-bit systems since you'll end up with the warning above. Is there a better option or did I misunderstand how that is supposed to work?
I don't know a better way that two cast, doing (struct ion_heap_data __user *)(uintptr_t)query->heaps;
It may help to put this into an inline function though.
Arnd
On 09/02/2016 02:37 PM, Arnd Bergmann wrote:
On Friday, September 2, 2016 2:27:21 PM CEST Laura Abbott wrote:
All warnings (new ones prefixed by >>):
drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':
drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
(struct ion_heap_data __user *)query->heaps; ^botching-up-ioctls.txt says:
- Pointers are __u64, cast from/to a uintprt_t on the userspace side and from/to a void __user * in the kernel. Try really hard not to delay this conversion or worse, fiddle the raw __u64 through your code since that diminishes the checking tools like sparse can provide.
This doesn't seem like it will work on 32-bit systems since you'll end up with the warning above. Is there a better option or did I misunderstand how that is supposed to work?
I don't know a better way that two cast, doing (struct ion_heap_data __user *)(uintptr_t)query->heaps;
It may help to put this into an inline function though.
Arnd
There already is one it turns out in kernel.h:
#define u64_to_user_ptr(x) ( \ { \ typecheck(u64, x); \ (void __user *)(uintptr_t)x; \ } \ )
At least this way I don't have to look at the double casts.
Thanks, Laura
On Thu, Sep 01, 2016 at 03:40:44PM -0700, Laura Abbott wrote:
Ion clients currently lack a good method to determine what heaps are available and what ids they map to. This leads to tight coupling between user and kernel space and headaches. Add a query ioctl to let userspace know the availability of heaps.
Signed-off-by: Laura Abbott labbott@redhat.com
drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++ drivers/staging/android/ion/ion.c | 44 +++++++++++++++++++++++++++++++++ drivers/staging/android/ion/ion_priv.h | 3 +++ drivers/staging/android/uapi/ion.h | 39 +++++++++++++++++++++++++++++ 4 files changed, 97 insertions(+)
diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c index 53b9520..e76d517 100644 --- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -28,6 +28,7 @@ union ion_ioctl_arg { struct ion_handle_data handle; struct ion_custom_data custom; struct ion_abi_version abi_version;
- struct ion_heap_query query;
}; static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) @@ -38,6 +39,11 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) case ION_IOC_ABI_VERSION: ret = arg->abi_version.reserved != 0; break;
- case ION_IOC_HEAP_QUERY:
ret = arg->query.reserved0 != 0;ret |= arg->query.reserved1 != 0;ret |= arg->query.reserved2 != 0;break;@@ -162,6 +168,11 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) data.abi_version.abi_version = ION_ABI_VERSION; break; }
- case ION_IOC_HEAP_QUERY:
- {
ret = ion_query_heaps(client, &data.query);break;- }
Minor nit, the { } aren't needed here. Yeah, I know the other cases have them, but they aren't all needed there either, no need to keep copying bad code style :)
default: return -ENOTTY; } diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index 975b48f..91b765c 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -1174,6 +1174,49 @@ int ion_sync_for_device(struct ion_client *client, int fd) return 0; } +int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query) +{
- struct ion_device *dev = client->dev;
- struct ion_heap_data __user *buffer =
(struct ion_heap_data __user *)query->heaps;
Shouldn't query be marked as __user instead of having this cast?
- int ret = -EINVAL, cnt = 0, max_cnt;
- struct ion_heap *heap;
- struct ion_heap_data hdata;
- memset(&hdata, 0, sizeof(hdata));
- down_read(&dev->lock);
- if (!buffer) {
query->cnt = dev->heap_cnt;
Wait, query is __user?
I think the mixing of user/kernel pointers here isn't quite right, or I just really can't figure it out...
And kbuild didn't seem to like this patch either :(
But your first 2 patches are great, I'll queue them up later today.
thanks,
greg k-h
On 09/01/2016 11:14 PM, Greg Kroah-Hartman wrote:
On Thu, Sep 01, 2016 at 03:40:44PM -0700, Laura Abbott wrote:
Ion clients currently lack a good method to determine what heaps are available and what ids they map to. This leads to tight coupling between user and kernel space and headaches. Add a query ioctl to let userspace know the availability of heaps.
Signed-off-by: Laura Abbott labbott@redhat.com
drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++ drivers/staging/android/ion/ion.c | 44 +++++++++++++++++++++++++++++++++ drivers/staging/android/ion/ion_priv.h | 3 +++ drivers/staging/android/uapi/ion.h | 39 +++++++++++++++++++++++++++++ 4 files changed, 97 insertions(+)
diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c index 53b9520..e76d517 100644 --- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -28,6 +28,7 @@ union ion_ioctl_arg { struct ion_handle_data handle; struct ion_custom_data custom; struct ion_abi_version abi_version;
- struct ion_heap_query query;
};
static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) @@ -38,6 +39,11 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) case ION_IOC_ABI_VERSION: ret = arg->abi_version.reserved != 0; break;
- case ION_IOC_HEAP_QUERY:
ret = arg->query.reserved0 != 0;ret |= arg->query.reserved1 != 0;ret |= arg->query.reserved2 != 0;break;@@ -162,6 +168,11 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) data.abi_version.abi_version = ION_ABI_VERSION; break; }
- case ION_IOC_HEAP_QUERY:
- {
ret = ion_query_heaps(client, &data.query);break;- }
Minor nit, the { } aren't needed here. Yeah, I know the other cases have them, but they aren't all needed there either, no need to keep copying bad code style :)
Huh, might deserve a checkpatch addition then. Never heard that one before.
default: return -ENOTTY; } diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index 975b48f..91b765c 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -1174,6 +1174,49 @@ int ion_sync_for_device(struct ion_client *client, int fd) return 0; }
+int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query) +{
- struct ion_device *dev = client->dev;
- struct ion_heap_data __user *buffer =
(struct ion_heap_data __user *)query->heaps;Shouldn't query be marked as __user instead of having this cast?
No, the query structure itself is copied into the kernel in ion_ioctl. The sub field query->heaps is a user pointer which is marked as _u64 for compatability ala botching-ioctls.txt hence the cast.
- int ret = -EINVAL, cnt = 0, max_cnt;
- struct ion_heap *heap;
- struct ion_heap_data hdata;
- memset(&hdata, 0, sizeof(hdata));
- down_read(&dev->lock);
- if (!buffer) {
query->cnt = dev->heap_cnt;Wait, query is __user?
I think the mixing of user/kernel pointers here isn't quite right, or I just really can't figure it out...
And kbuild didn't seem to like this patch either :(
But your first 2 patches are great, I'll queue them up later today.
thanks,
greg k-h
On Fri, Sep 02, 2016 at 01:41:48PM -0700, Laura Abbott wrote:
On 09/01/2016 11:14 PM, Greg Kroah-Hartman wrote:
On Thu, Sep 01, 2016 at 03:40:44PM -0700, Laura Abbott wrote:
Ion clients currently lack a good method to determine what heaps are available and what ids they map to. This leads to tight coupling between user and kernel space and headaches. Add a query ioctl to let userspace know the availability of heaps.
Signed-off-by: Laura Abbott labbott@redhat.com
drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++ drivers/staging/android/ion/ion.c | 44 +++++++++++++++++++++++++++++++++ drivers/staging/android/ion/ion_priv.h | 3 +++ drivers/staging/android/uapi/ion.h | 39 +++++++++++++++++++++++++++++ 4 files changed, 97 insertions(+)
diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c index 53b9520..e76d517 100644 --- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -28,6 +28,7 @@ union ion_ioctl_arg { struct ion_handle_data handle; struct ion_custom_data custom; struct ion_abi_version abi_version;
- struct ion_heap_query query;
};
static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) @@ -38,6 +39,11 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) case ION_IOC_ABI_VERSION: ret = arg->abi_version.reserved != 0; break;
- case ION_IOC_HEAP_QUERY:
ret = arg->query.reserved0 != 0;ret |= arg->query.reserved1 != 0;ret |= arg->query.reserved2 != 0;break;@@ -162,6 +168,11 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) data.abi_version.abi_version = ION_ABI_VERSION; break; }
- case ION_IOC_HEAP_QUERY:
- {
ret = ion_query_heaps(client, &data.query);break;- }
Minor nit, the { } aren't needed here. Yeah, I know the other cases have them, but they aren't all needed there either, no need to keep copying bad code style :)
Huh, might deserve a checkpatch addition then. Never heard that one before.
It's not a checkpatch issue, just a "is this { } even needed" type of an issue :)
default: return -ENOTTY; } diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index 975b48f..91b765c 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -1174,6 +1174,49 @@ int ion_sync_for_device(struct ion_client *client, int fd) return 0; }
+int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query) +{
- struct ion_device *dev = client->dev;
- struct ion_heap_data __user *buffer =
(struct ion_heap_data __user *)query->heaps;Shouldn't query be marked as __user instead of having this cast?
No, the query structure itself is copied into the kernel in ion_ioctl. The sub field query->heaps is a user pointer which is marked as _u64 for compatability ala botching-ioctls.txt hence the cast.
Ah, ok. Messy :)
thanks,
greg k-h
linaro-mm-sig@lists.linaro.org
-
Arnd Bergmann -
Brian Starkey -
Greg Kroah-Hartman -
kbuild test robot -
Laura Abbott