[RFC][PATCH 0/6] ion: improved ABI

[RFC][PATCH 0/6] ion: improved ABI

[Laura Abbott]

The ABI for Ion's ioctl interface are a pain to work with. The heap IDs
are a 32-bit non-discoverable namespace that form part of the ABI. There's
no way to determine what ABI version is in use which leads to problems
if the ABI changes or needs to be updated.

This series is a first approach to give a better ABI for Ion. This includes:

- Following the advice in botching-up-ioctls.txt
- Ioctl for ABI version
- Dynamic assignment of heap ids
- queryable heap ids
- Runtime mapping of heap ids, including fallbacks. This avoids the need to
  encode the fallbacks as an ABI.

I'm most interested in feedback if this ABI is actually an improvement and
usable. The heap id map/query interface seems error prone but I didn't have
a cleaner solution. There aren't any kernel APIs for the new features as the
focus was on a userspace API but I anticipate that following easily once
the userspace API is established.


Thanks,
Laura

P.S. Not to turn this into a bike shedding session but if you have suggestions
for a name for this framework other than Ion I would be interested to hear
them. Too many other things are already named Ion.

Laura Abbott (6):
  staging: android: ion: return error value for ion_device_add_heap
  staging: android: ion: Switch to using an idr to manage heaps
  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: Introduce new ioctls for dynamic heaps

 drivers/staging/android/ion/Makefile    |   3 +-
 drivers/staging/android/ion/ion-ioctl.c | 243 ++++++++++++++++++
 drivers/staging/android/ion/ion.c       | 438 ++++++++++++++++----------------
 drivers/staging/android/ion/ion_priv.h  | 109 +++++++-
 drivers/staging/android/uapi/ion.h      | 164 +++++++++++-
 5 files changed, 728 insertions(+), 229 deletions(-)
 create mode 100644 drivers/staging/android/ion/ion-ioctl.c

-- 
2.5.5

[RFC][PATCH 1/6] staging: android: ion: return error value for ion_device_add_heap

[Laura Abbott]

From: Laura Abbott <labbott@fedoraproject.org>


ion_device_add_heap doesn't return an error value. Change it to return
information to callers.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
 drivers/staging/android/ion/ion.c      | 7 +++++--
 drivers/staging/android/ion/ion_priv.h | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index a2cf93b..306340a 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -1584,14 +1584,16 @@ static int debug_shrink_get(void *data, u64 *val)
 DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
 			debug_shrink_set, "%llu\n");
 
-void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
+int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
 {
 	struct dentry *debug_file;
 
 	if (!heap->ops->allocate || !heap->ops->free || !heap->ops->map_dma ||
-	    !heap->ops->unmap_dma)
+	    !heap->ops->unmap_dma) {
 		pr_err("%s: can not add heap with invalid ops struct.\n",
 		       __func__);
+		return -EINVAL;
+	}
 
 	spin_lock_init(&heap->free_lock);
 	heap->free_list_size = 0;
@@ -1639,6 +1641,7 @@ void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
 	}
 
 	up_write(&dev->lock);
+	return 0;
 }
 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 0239883..35726ae 100644
--- a/drivers/staging/android/ion/ion_priv.h
+++ b/drivers/staging/android/ion/ion_priv.h
@@ -221,7 +221,7 @@ void ion_device_destroy(struct ion_device *dev);
  * @dev:		the device
  * @heap:		the heap to add
  */
-void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap);
+int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap);
 
 /**
  * some helpers for common operations on buffers using the sg_table
-- 
2.5.5

Re: [RFC][PATCH 1/6] staging: android: ion: return error value for ion_device_add_heap

[Liviu Dudau]

On Mon, Jun 06, 2016 at 11:23:28AM -0700, Laura Abbott wrote:
> From: Laura Abbott <labbott@fedoraproject.org>
> 
> 
> ion_device_add_heap doesn't return an error value. Change it to return
> information to callers.
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>

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


> ---
>  drivers/staging/android/ion/ion.c      | 7 +++++--
>  drivers/staging/android/ion/ion_priv.h | 2 +-
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index a2cf93b..306340a 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -1584,14 +1584,16 @@ static int debug_shrink_get(void *data, u64 *val)
>  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>  			debug_shrink_set, "%llu\n");
>  
> -void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
> +int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
>  {
>  	struct dentry *debug_file;
>  
>  	if (!heap->ops->allocate || !heap->ops->free || !heap->ops->map_dma ||
> -	    !heap->ops->unmap_dma)
> +	    !heap->ops->unmap_dma) {
>  		pr_err("%s: can not add heap with invalid ops struct.\n",
>  		       __func__);
> +		return -EINVAL;
> +	}
>  
>  	spin_lock_init(&heap->free_lock);
>  	heap->free_list_size = 0;
> @@ -1639,6 +1641,7 @@ void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
>  	}
>  
>  	up_write(&dev->lock);
> +	return 0;
>  }
>  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 0239883..35726ae 100644
> --- a/drivers/staging/android/ion/ion_priv.h
> +++ b/drivers/staging/android/ion/ion_priv.h
> @@ -221,7 +221,7 @@ void ion_device_destroy(struct ion_device *dev);
>   * @dev:		the device
>   * @heap:		the heap to add
>   */
> -void ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap);
> +int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap);
>  
>  /**
>   * some helpers for common operations on buffers using the sg_table
> -- 
> 2.5.5
> 

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

[RFC][PATCH 2/6] staging: android: ion: Switch to using an idr to manage heaps

[Laura Abbott]

From: Laura Abbott <labbott@fedoraproject.org>


In anticipation of dynamic registration of heaps, switch to using
an idr for heaps. The idr makes it easier to control the assignment
and management + lookup of heap numbers.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
 drivers/staging/android/ion/ion.c | 83 +++++++++++++++++++++++++--------------
 1 file changed, 53 insertions(+), 30 deletions(-)

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 306340a..739060f 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -55,13 +55,14 @@ struct ion_device {
 	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 idr idr;
+	int heap_cnt;
 };
 
 /**
@@ -488,39 +489,22 @@ static int ion_handle_add(struct ion_client *client, struct ion_handle *handle)
 	return 0;
 }
 
-struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
-			     size_t align, unsigned int heap_id_mask,
+static struct ion_handle *__ion_alloc(struct ion_client *client, size_t len,
+			     size_t align, struct ion_heap *heap,
 			     unsigned int flags)
 {
 	struct ion_handle *handle;
 	struct ion_device *dev = client->dev;
 	struct ion_buffer *buffer = NULL;
-	struct ion_heap *heap;
 	int ret;
 
-	pr_debug("%s: len %zu align %zu heap_id_mask %u flags %x\n", __func__,
-		 len, align, heap_id_mask, flags);
-	/*
-	 * traverse the list of heaps available in this system in priority
-	 * order.  If the heap type is supported by the client, and matches the
-	 * request of the caller allocate from it.  Repeat until allocate has
-	 * succeeded or all heaps have been tried
-	 */
+
 	len = PAGE_ALIGN(len);
 
 	if (!len)
 		return ERR_PTR(-EINVAL);
 
-	down_read(&dev->lock);
-	plist_for_each_entry(heap, &dev->heaps, node) {
-		/* if the caller didn't specify this heap id */
-		if (!((1 << heap->id) & heap_id_mask))
-			continue;
-		buffer = ion_buffer_create(heap, dev, len, align, flags);
-		if (!IS_ERR(buffer))
-			break;
-	}
-	up_read(&dev->lock);
+	buffer = ion_buffer_create(heap, dev, len, align, flags);
 
 	if (buffer == NULL)
 		return ERR_PTR(-ENODEV);
@@ -549,6 +533,41 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
 
 	return handle;
 }
+
+struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
+				size_t align, unsigned int heap_mask,
+				unsigned int flags)
+{
+	int bit;
+	struct ion_handle *handle = ERR_PTR(-ENODEV);
+
+	pr_debug("%s: len %zu align %zu heap_id_mask %u flags %x\n", __func__,
+		 len, align, heap_mask, flags);
+
+	down_read(&client->dev->lock);
+	/*
+	 * traverse the list of heaps available in this system in priority
+	 * order.  If the heap type is supported by the client, and matches the
+	 * request of the caller allocate from it.  Repeat until allocate has
+	 * succeeded or all heaps have been tried
+	 */
+	for_each_set_bit(bit, (unsigned long *)&heap_mask, 32) {
+		struct ion_heap *heap;
+
+		heap = idr_find(&client->dev->idr, bit);
+		if (!heap)
+			continue;
+
+		handle = __ion_alloc(client, len, align, heap, flags);
+		if (IS_ERR(handle))
+			continue;
+		else
+			break;
+	}
+
+	up_read(&client->dev->lock);
+	return handle;
+}
 EXPORT_SYMBOL(ion_alloc);
 
 static void ion_free_nolock(struct ion_client *client, struct ion_handle *handle)
@@ -1587,6 +1606,7 @@ DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
 int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
 {
 	struct dentry *debug_file;
+	int ret;
 
 	if (!heap->ops->allocate || !heap->ops->free || !heap->ops->map_dma ||
 	    !heap->ops->unmap_dma) {
@@ -1606,12 +1626,15 @@ int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
 
 	heap->dev = dev;
 	down_write(&dev->lock);
-	/*
-	 * use negative heap->id to reverse the priority -- when traversing
-	 * the list later attempt higher id numbers first
-	 */
-	plist_node_init(&heap->node, -heap->id);
-	plist_add(&heap->node, &dev->heaps);
+
+	ret = idr_alloc(&dev->idr, heap, heap->id, heap->id + 1, GFP_KERNEL);
+	if (ret < 0 || ret != heap->id) {
+		pr_info("%s: Failed to add heap id, expected %d got %d\n",
+			__func__, heap->id, ret);
+		up_write(&dev->lock);
+		return ret < 0 ? ret : -EINVAL;
+	}
+
 	debug_file = debugfs_create_file(heap->name, 0664,
 					dev->heaps_debug_root, heap,
 					&debug_heap_fops);
@@ -1639,7 +1662,7 @@ int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
 				path, debug_name);
 		}
 	}
-
+	dev->heap_cnt++;
 	up_write(&dev->lock);
 	return 0;
 }
@@ -1689,7 +1712,7 @@ debugfs_done:
 	idev->buffers = RB_ROOT;
 	mutex_init(&idev->buffer_lock);
 	init_rwsem(&idev->lock);
-	plist_head_init(&idev->heaps);
+	idr_init(&idev->idr);
 	idev->clients = RB_ROOT;
 	ion_root_client = &idev->clients;
 	mutex_init(&debugfs_mutex);
-- 
2.5.5

Re: [RFC][PATCH 2/6] staging: android: ion: Switch to using an idr to manage heaps

[Liviu Dudau]

On Mon, Jun 06, 2016 at 11:23:29AM -0700, Laura Abbott wrote:
> From: Laura Abbott <labbott@fedoraproject.org>
> 
> 
> In anticipation of dynamic registration of heaps, switch to using
> an idr for heaps. The idr makes it easier to control the assignment
> and management + lookup of heap numbers.
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
>  drivers/staging/android/ion/ion.c | 83 +++++++++++++++++++++++++--------------
>  1 file changed, 53 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 306340a..739060f 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -55,13 +55,14 @@ struct ion_device {
>  	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 idr idr;
> +	int heap_cnt;
>  };
>  
>  /**
> @@ -488,39 +489,22 @@ static int ion_handle_add(struct ion_client *client, struct ion_handle *handle)
>  	return 0;
>  }
>  
> -struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
> -			     size_t align, unsigned int heap_id_mask,
> +static struct ion_handle *__ion_alloc(struct ion_client *client, size_t len,
> +			     size_t align, struct ion_heap *heap,
>  			     unsigned int flags)
>  {
>  	struct ion_handle *handle;
>  	struct ion_device *dev = client->dev;
>  	struct ion_buffer *buffer = NULL;
> -	struct ion_heap *heap;
>  	int ret;
>  
> -	pr_debug("%s: len %zu align %zu heap_id_mask %u flags %x\n", __func__,
> -		 len, align, heap_id_mask, flags);
> -	/*
> -	 * traverse the list of heaps available in this system in priority
> -	 * order.  If the heap type is supported by the client, and matches the
> -	 * request of the caller allocate from it.  Repeat until allocate has
> -	 * succeeded or all heaps have been tried
> -	 */
> +

Drop the (second) empty line here.

>  	len = PAGE_ALIGN(len);
>  
>  	if (!len)
>  		return ERR_PTR(-EINVAL);
>  
> -	down_read(&dev->lock);
> -	plist_for_each_entry(heap, &dev->heaps, node) {
> -		/* if the caller didn't specify this heap id */
> -		if (!((1 << heap->id) & heap_id_mask))
> -			continue;
> -		buffer = ion_buffer_create(heap, dev, len, align, flags);
> -		if (!IS_ERR(buffer))
> -			break;
> -	}
> -	up_read(&dev->lock);
> +	buffer = ion_buffer_create(heap, dev, len, align, flags);
>  
>  	if (buffer == NULL)
>  		return ERR_PTR(-ENODEV);
> @@ -549,6 +533,41 @@ struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
>  
>  	return handle;
>  }
> +
> +struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
> +				size_t align, unsigned int heap_mask,
> +				unsigned int flags)
> +{
> +	int bit;
> +	struct ion_handle *handle = ERR_PTR(-ENODEV);
> +
> +	pr_debug("%s: len %zu align %zu heap_id_mask %u flags %x\n", __func__,
> +		 len, align, heap_mask, flags);
> +
> +	down_read(&client->dev->lock);
> +	/*
> +	 * traverse the list of heaps available in this system in priority
> +	 * order.  If the heap type is supported by the client, and matches the
> +	 * request of the caller allocate from it.  Repeat until allocate has
> +	 * succeeded or all heaps have been tried
> +	 */
> +	for_each_set_bit(bit, (unsigned long *)&heap_mask, 32) {
> +		struct ion_heap *heap;
> +
> +		heap = idr_find(&client->dev->idr, bit);
> +		if (!heap)
> +			continue;
> +
> +		handle = __ion_alloc(client, len, align, heap, flags);
> +		if (IS_ERR(handle))
> +			continue;
> +		else
> +			break;
> +	}
> +
> +	up_read(&client->dev->lock);
> +	return handle;
> +}
>  EXPORT_SYMBOL(ion_alloc);
>  
>  static void ion_free_nolock(struct ion_client *client, struct ion_handle *handle)
> @@ -1587,6 +1606,7 @@ DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>  int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
>  {
>  	struct dentry *debug_file;
> +	int ret;
>  
>  	if (!heap->ops->allocate || !heap->ops->free || !heap->ops->map_dma ||
>  	    !heap->ops->unmap_dma) {
> @@ -1606,12 +1626,15 @@ int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
>  
>  	heap->dev = dev;
>  	down_write(&dev->lock);
> -	/*
> -	 * use negative heap->id to reverse the priority -- when traversing
> -	 * the list later attempt higher id numbers first
> -	 */
> -	plist_node_init(&heap->node, -heap->id);
> -	plist_add(&heap->node, &dev->heaps);
> +
> +	ret = idr_alloc(&dev->idr, heap, heap->id, heap->id + 1, GFP_KERNEL);
> +	if (ret < 0 || ret != heap->id) {
> +		pr_info("%s: Failed to add heap id, expected %d got %d\n",
> +			__func__, heap->id, ret);
> +		up_write(&dev->lock);
> +		return ret < 0 ? ret : -EINVAL;
> +	}
> +
>  	debug_file = debugfs_create_file(heap->name, 0664,
>  					dev->heaps_debug_root, heap,
>  					&debug_heap_fops);
> @@ -1639,7 +1662,7 @@ int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
>  				path, debug_name);
>  		}
>  	}
> -
> +	dev->heap_cnt++;
>  	up_write(&dev->lock);
>  	return 0;
>  }
> @@ -1689,7 +1712,7 @@ debugfs_done:
>  	idev->buffers = RB_ROOT;
>  	mutex_init(&idev->buffer_lock);
>  	init_rwsem(&idev->lock);
> -	plist_head_init(&idev->heaps);
> +	idr_init(&idev->idr);
>  	idev->clients = RB_ROOT;
>  	ion_root_client = &idev->clients;
>  	mutex_init(&debugfs_mutex);
> -- 
> 2.5.5
> 

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

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

[RFC][PATCH 3/6] staging: android: ion: Drop heap type masks

[Laura Abbott]

From: Laura Abbott <labbott@fedoraproject.org>


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)
 
 /**
-- 
2.5.5

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

[Laura Abbott]

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

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

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

[RFC][PATCH 5/6] staging: android: ion: Add an ioctl for ABI checking

[Laura Abbott]

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 |  6 ++++++
 drivers/staging/android/uapi/ion.h      | 23 +++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index 341ba7d..45b89e8 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -48,6 +48,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		struct ion_allocation_data allocation;
 		struct ion_handle_data handle;
 		struct ion_custom_data custom;
+		struct ion_abi_version abi_version;
 	} data;
 
 	dir = ion_ioctl_dir(cmd);
@@ -129,6 +130,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..145005f 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,13 @@ 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 */
-- 
2.5.5

[RFC][PATCH 6/6] staging: android: ion: Introduce new ioctls for dynamic heaps

[Laura Abbott]

From: Laura Abbott <labbott@fedoraproject.org>


The Ion ABI for heaps is limiting to work with for more complex systems.
Heaps have to be registered at boot time with known ids available to
userspace. This becomes a tight ABI which is prone to breakage.

Introduce a new mechanism for registering heap ids dynamically. A base
set of heap ids are registered at boot time but there is no knowledge
of fallbacks. Fallback information can be remapped and changed
dynamically. Information about available heaps can also be queried with
an ioctl to avoid the need to have heap ids be an ABI with userspace.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
 drivers/staging/android/ion/ion-ioctl.c | 109 +++++++++++++++++--
 drivers/staging/android/ion/ion.c       | 184 ++++++++++++++++++++++++++++++--
 drivers/staging/android/ion/ion_priv.h  |  15 +++
 drivers/staging/android/uapi/ion.h      | 135 +++++++++++++++++++++++
 4 files changed, 426 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index 45b89e8..169cad8 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -22,6 +22,49 @@
 #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;
+	struct ion_new_alloc_data allocation2;
+	struct ion_usage_id_map id_map;
+	struct ion_usage_cnt usage_cnt;
+	struct ion_heap_query query;
+};
+
+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;
+	case ION_IOC_ALLOC2:
+		ret = arg->allocation2.reserved0 != 0;
+		ret |= arg->allocation2.reserved1 != 0;
+		ret |= arg->allocation2.reserved2 != 0;
+		break;
+	case ION_IOC_ID_MAP:
+		ret = arg->id_map.reserved0 != 0;
+		ret |= arg->id_map.reserved1 != 0;
+		break;
+	case ION_IOC_USAGE_CNT:
+		ret = arg->usage_cnt.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;
+	}
+	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,14 +85,7 @@ 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;
-		struct ion_abi_version abi_version;
-	} data;
+	union ion_ioctl_arg data;
 
 	dir = ion_ioctl_dir(cmd);
 
@@ -60,7 +96,12 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
 			return -EFAULT;
 
+	ret = validate_ioctl_arg(cmd, &data);
+	if (ret)
+		return ret;
+
 	switch (cmd) {
+	/* Old ioctl */
 	case ION_IOC_ALLOC:
 	{
 		struct ion_handle *handle;
@@ -77,6 +118,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		cleanup_handle = handle;
 		break;
 	}
+	/* Old ioctl */
 	case ION_IOC_FREE:
 	{
 		struct ion_handle *handle;
@@ -92,6 +134,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		mutex_unlock(&client->lock);
 		break;
 	}
+	/* Old ioctl */
 	case ION_IOC_SHARE:
 	case ION_IOC_MAP:
 	{
@@ -106,6 +149,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			ret = data.fd.fd;
 		break;
 	}
+	/* Old ioctl */
 	case ION_IOC_IMPORT:
 	{
 		struct ion_handle *handle;
@@ -117,11 +161,13 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			data.handle.handle = handle->id;
 		break;
 	}
+	/* Old ioctl */
 	case ION_IOC_SYNC:
 	{
 		ret = ion_sync_for_device(client, data.fd.fd);
 		break;
 	}
+	/* Old ioctl */
 	case ION_IOC_CUSTOM:
 	{
 		if (!dev->custom_ioctl)
@@ -135,6 +181,53 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		data.abi_version.abi_version = ION_ABI_VERSION;
 		break;
 	}
+	case ION_IOC_ALLOC2:
+	{
+		struct ion_handle *handle;
+
+		handle = ion_alloc2(client, data.allocation2.len,
+					data.allocation2.align,
+					data.allocation2.usage_id,
+					data.allocation2.flags);
+		if (IS_ERR(handle))
+			return PTR_ERR(handle);
+
+		if (data.allocation2.flags & ION_FLAG_NO_HANDLE) {
+			data.allocation2.fd = ion_share_dma_buf_fd(client,
+								handle);
+			ion_handle_put(handle);
+			if (data.allocation2.fd < 0)
+				ret = data.allocation2.fd;
+		} else {
+			data.allocation2.handle = handle->id;
+
+			cleanup_handle = handle;
+		}
+		break;
+	}
+	case ION_IOC_ID_MAP:
+	{
+		ret = ion_map_usage_ids(client,
+				(unsigned int __user *)data.id_map.usage_ids,
+				data.id_map.cnt);
+		if (ret > 0)
+			data.id_map.new_id = ret;
+		break;
+	}
+	case ION_IOC_USAGE_CNT:
+	{
+		down_read(&client->dev->lock);
+		data.usage_cnt.cnt = client->dev->heap_cnt;
+		up_read(&client->dev->lock);
+		break;
+	}
+	case ION_IOC_HEAP_QUERY:
+	{
+		ret = ion_query_heaps(client,
+				(struct ion_heap_data __user *)data.query.heaps,
+				data.query.cnt);
+		break;
+	}
 	default:
 		return -ENOTTY;
 	}
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 129707f..c096cb9 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -459,6 +459,48 @@ static struct ion_handle *__ion_alloc(struct ion_client *client, size_t len,
 	return handle;
 }
 
+struct ion_handle *ion_alloc2(struct ion_client *client, size_t len,
+				size_t align, unsigned int usage_id,
+				unsigned int flags)
+{
+	struct ion_device *dev = client->dev;
+	struct ion_heap *heap;
+	struct ion_handle *handle = ERR_PTR(-ENODEV);
+
+	down_read(&dev->lock);
+	heap = idr_find(&dev->idr, usage_id);
+	if (!heap) {
+		handle = ERR_PTR(-EINVAL);
+		goto out;
+	}
+
+	if (heap->type == ION_USAGE_ID_MAP) {
+		int i;
+
+		for (i = 0; i < heap->fallback_cnt; i++){
+			heap = idr_find(&dev->idr, heap->fallbacks[i]);
+			if (!heap)
+				continue;
+
+			/* Don't recurse for now? */
+			if (heap->type == ION_USAGE_ID_MAP)
+				continue;
+
+			handle =  __ion_alloc(client, len, align, heap, flags);
+			if (IS_ERR(handle))
+				continue;
+			else
+				break;
+		}
+	} else {
+		handle = __ion_alloc(client, len, align, heap, flags);
+	}
+out:
+	up_read(&dev->lock);
+	return handle;
+}
+EXPORT_SYMBOL(ion_alloc2);
+
 struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
 				size_t align, unsigned int heap_mask,
 				unsigned int flags)
@@ -1243,6 +1285,55 @@ int ion_sync_for_device(struct ion_client *client, int fd)
 	return 0;
 }
 
+struct ion_query_data {
+	struct ion_heap_data __user *buffer;
+	int cnt;
+	int max_cnt;
+};
+
+int __ion_query(int id, void *p, void *data)
+{
+	struct ion_heap *heap = p;
+	struct ion_query_data *query = data;
+	struct ion_heap_data hdata = {0};
+
+	if (query->cnt >= query->max_cnt)
+		return -ENOSPC;
+
+	strncpy(hdata.name, heap->name, 20);
+	hdata.name[sizeof(hdata.name) - 1] = '\0';
+	hdata.type = heap->type;
+	hdata.usage_id = heap->id;
+
+	return copy_to_user(&query->buffer[query->cnt++], &hdata, sizeof(hdata));
+}
+
+int ion_query_heaps(struct ion_client *client,
+		struct ion_heap_data __user *buffer,
+		int cnt)
+{
+	struct ion_device *dev = client->dev;
+	struct ion_query_data data;
+	int ret;
+
+	data.buffer = buffer;
+	data.cnt = 0;
+	data.max_cnt = cnt;
+
+	down_read(&dev->lock);
+	if (data.max_cnt < 0 || data.max_cnt > dev->heap_cnt) {
+		ret = -EINVAL;
+		goto out;
+	}
+	ret = idr_for_each(&dev->idr, __ion_query, &data);
+out:
+	up_read(&dev->lock);
+
+	return ret;
+}
+
+
+
 static int ion_release(struct inode *inode, struct file *file)
 {
 	struct ion_client *client = file->private_data;
@@ -1407,13 +1498,69 @@ static int debug_shrink_get(void *data, u64 *val)
 DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
 			debug_shrink_set, "%llu\n");
 
+int ion_map_usage_ids(struct ion_client *client,
+			unsigned int __user *usage_ids,
+			int cnt)
+{
+	struct ion_heap *heap = kzalloc(sizeof(*heap), GFP_KERNEL);
+	unsigned int *fallbacks;
+	int i;
+	int ret;
+
+	if (!heap)
+		return -ENOMEM;
+
+	fallbacks = kzalloc(sizeof(unsigned int)*cnt, GFP_KERNEL);
+	if (!fallbacks) {
+		ret = -ENOMEM;
+		goto out1;
+	}
+
+	ret = copy_from_user(fallbacks, usage_ids, sizeof(unsigned int)*cnt);
+	if (ret)
+		goto out2;
+
+	down_read(&client->dev->lock);
+	for (i = 0; i < cnt; i++) {
+		if (idr_find(&client->dev->idr, fallbacks[i]) == NULL) {
+			ret = -EINVAL;
+			goto out3;
+		}
+	}
+	up_read(&client->dev->lock);
+
+	/*
+	 * This is a racy check since the lock is dropped before the heap
+	 * is actually added. It's okay though because ids are never actually
+	 * deleted. Worst case some user gets an error back and an indication
+	 * to fix races in their code.
+	 */
+
+	heap->fallbacks = fallbacks;
+	heap->fallback_cnt = cnt;
+	heap->type = ION_USAGE_ID_MAP;
+	heap->id = ION_DYNAMIC_HEAP_ASSIGN;
+	ion_device_add_heap(client->dev, heap);
+	return heap->id;
+out3:
+	up_read(&client->dev->lock);
+out2:
+	kfree(fallbacks);
+out1:
+	kfree(heap);
+	return ret;
+}
+
 int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
 {
 	struct dentry *debug_file;
 	int ret;
 
-	if (!heap->ops->allocate || !heap->ops->free || !heap->ops->map_dma ||
-	    !heap->ops->unmap_dma) {
+	if (heap->type != ION_USAGE_ID_MAP &&
+	   (!heap->ops->allocate ||
+	    !heap->ops->free ||
+	    !heap->ops->map_dma ||
+	    !heap->ops->unmap_dma)) {
 		pr_err("%s: can not add heap with invalid ops struct.\n",
 		       __func__);
 		return -EINVAL;
@@ -1425,18 +1572,36 @@ int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
 	if (heap->flags & ION_HEAP_FLAG_DEFER_FREE)
 		ion_heap_init_deferred_free(heap);
 
-	if ((heap->flags & ION_HEAP_FLAG_DEFER_FREE) || heap->ops->shrink)
+	if ((heap->flags & ION_HEAP_FLAG_DEFER_FREE) || (heap->ops && heap->ops->shrink))
 		ion_heap_init_shrinker(heap);
 
 	heap->dev = dev;
 	down_write(&dev->lock);
 
-	ret = idr_alloc(&dev->idr, heap, heap->id, heap->id + 1, GFP_KERNEL);
-	if (ret < 0 || ret != heap->id) {
-		pr_info("%s: Failed to add heap id, expected %d got %d\n",
-			__func__, heap->id, ret);
-		up_write(&dev->lock);
-		return ret < 0 ? ret : -EINVAL;
+	if (heap->id == ION_DYNAMIC_HEAP_ASSIGN) {
+		ret = idr_alloc(&dev->idr, heap,
+				ION_DYNAMIC_HEAP_ASSIGN + 1, 0, GFP_KERNEL);
+		if (ret < 0)
+			goto out_unlock;
+
+		heap->id = ret;
+	} else {
+		ret = idr_alloc(&dev->idr, heap, heap->id, heap->id + 1,
+				GFP_KERNEL);
+		if (ret < 0 || ret != heap->id) {
+			pr_info("%s: Failed to add heap id, expected %d got %d\n",
+				__func__, heap->id, ret);
+			ret = ret < 0 ? ret : -EINVAL;
+			goto out_unlock;
+		}
+	}
+
+	if (!heap->name) {
+		heap->name = kasprintf(GFP_KERNEL, "heap%d", heap->id);
+		if (!heap->name) {
+			ret = -ENOMEM;
+			goto out_unlock;
+		}
 	}
 
 	debug_file = debugfs_create_file(heap->name, 0664,
@@ -1467,6 +1632,7 @@ int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
 		}
 	}
 	dev->heap_cnt++;
+out_unlock:
 	up_write(&dev->lock);
 	return 0;
 }
diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h
index b09d751..e03e940 100644
--- a/drivers/staging/android/ion/ion_priv.h
+++ b/drivers/staging/android/ion/ion_priv.h
@@ -255,6 +255,9 @@ struct ion_heap {
 	wait_queue_head_t waitqueue;
 	struct task_struct *task;
 
+	unsigned int *fallbacks;
+	int fallback_cnt;
+
 	int (*debug_show)(struct ion_heap *heap, struct seq_file *, void *);
 };
 
@@ -381,6 +384,10 @@ size_t ion_heap_freelist_shrink(struct ion_heap *heap,
 size_t ion_heap_freelist_size(struct ion_heap *heap);
 
 
+int ion_map_usage_ids(struct ion_client *client,
+			unsigned int __user *usage_ids,
+			int cnt);
+
 /**
  * functions for creating and destroying the built in ion heaps.
  * architectures can add their own custom architecture specific
@@ -495,4 +502,12 @@ struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
 
 int ion_handle_put(struct ion_handle *handle);
 
+struct ion_handle *ion_alloc2(struct ion_client *client, size_t len,
+				size_t align, unsigned int usage_id,
+				unsigned int flags);
+
+int ion_query_heaps(struct ion_client *client,
+		struct ion_heap_data __user *buffer,
+		int cnt);
+
 #endif /* _ION_PRIV_H */
diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
index 145005f..d36b4e4 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -45,10 +45,17 @@ enum ion_heap_type {
 			       * must be last so device specific heaps always
 			       * are at the end of this enum
 			       */
+	ION_USAGE_ID_MAP,
 };
 
 #define ION_NUM_HEAP_IDS		(sizeof(unsigned int) * 8)
 
+/*
+ * This isn't completely random. The old Ion heap ID namespace goes up to
+ * 32 bits so take the first one after that to use as a dynamic setting
+ */
+#define ION_DYNAMIC_HEAP_ASSIGN		33
+
 /**
  * allocation flags - the lower 16 bits are used by core ion, the upper 16
  * bits are reserved for use by the heaps themselves.
@@ -65,6 +72,11 @@ enum ion_heap_type {
 					 * caches must be managed
 					 * manually
 					 */
+#define ION_FLAG_NO_HANDLE 3		/*
+					 * Return only an fd associated with
+					 * this buffer and not a handle
+					 */
+
 
 /**
  * DOC: Ion Userspace API
@@ -142,6 +154,96 @@ struct ion_abi_version {
 	__u32 reserved;
 };
 
+/**
+ * struct ion_new_alloc_data - metadata to/from usersapce allocation v2
+ * @len:		size of the allocation
+ * @align:		required alignment of the allocation
+ * @usage_id:		mapping to heap id to allocate. Will try fallbacks
+ *			if specified in the heap mapping
+ * @flags:		flags passed to heap
+ * @handle:		pointer that will be populated with a cookie to use to
+ *			refer to this allocation
+ * @fd:			optionally, may return just an fd with no handle
+ *			reference
+ */
+struct ion_new_alloc_data {
+	__u64 len;
+	__u64 align;
+	__u32 usage_id;
+	__u32 flags;
+	/*
+	 * I'd like to add a flag to just return the fd instead of just
+	 * a handle for those who want to skip the next step.
+	 */
+	union {
+		__u32 fd;
+		__u32 handle;
+	};
+	/*
+	 * Allocation has a definite problem of 'creep' so give more than
+	 * enough extra bits for expansion
+	 */
+	__u32 reserved0;
+	__u32 reserved1;
+	__u32 reserved2;
+};
+
+/**
+ * struct ion_usage_id_map - metadata to create a new usage map
+ * @usage_id - userspace allocated array of existing usage ids
+ * @cnt - number of ids to be mapped
+ * @new_id - will be populated with the new usage id
+ */
+struct ion_usage_id_map {
+	/* Array of usage ids provided by user */
+	__u64 usage_ids;
+	__u32 cnt;
+
+	/* Returned on success */
+	__u32 new_id;
+	/* Fields for future growth */
+	__u32 reserved0;
+	__u32 reserved1;
+};
+
+/**
+ * struct ion_usage_cnt - meta data for the count of heaps
+ * @cnt - returned count of number of heaps present
+ */
+struct ion_usage_cnt {
+	__u32 cnt; /* cnt returned */
+	__u32 reserved;
+};
+
+/**
+ * struct ion_heap_data - data about a heap
+ * @name - first 32 characters of the heap name
+ * @type - heap type
+ * @usage_id - usage id for the heap
+ */
+struct ion_heap_data {
+	char name[32];
+	__u32 type;
+	__u32 usage_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 */
+	__u64 heaps; /* buffer to be populated */
+	__u32 reserved0;
+	__u32 reserved1;
+	__u32 reserved2;
+};
+
+
 #define ION_IOC_MAGIC		'I'
 
 /**
@@ -216,5 +318,38 @@ struct ion_abi_version {
 #define ION_IOC_ABI_VERSION    _IOR(ION_IOC_MAGIC, 8, \
 					struct ion_abi_version)
 
+/**
+ * DOC: ION_IOC_ALLOC2 - allocate memory via new API
+ *
+ * Same concept as ION_IOC_ALLOC except with updated ABI. Can return an fd
+ * directly in addition to a handle
+ */
+#define ION_IOC_ALLOC2           _IOWR(ION_IOC_MAGIC, 9, \
+					struct ion_new_alloc_data)
+
+/**
+ * DOC: ION_IOC_ID_MAP - remap an array of heap IDs
+ *
+ * Takes an ion_usage_id_map structure populated with information about
+ * fallback information. Returns a new usage id for use in allocation.
+ */
+#define ION_IOC_ID_MAP         _IOWR(ION_IOC_MAGIC, 10, \
+					struct ion_usage_id_map)
+/**
+ * DOC: ION_IOC_USAGE_CNT - returns a count of the number of usage ids
+ *
+ * Takes an ion_usage_cnt structure and returns the total number of usage
+ * ids available.
+ */
+#define ION_IOC_USAGE_CNT      _IOR(ION_IOC_MAGIC, 11, \
+					struct ion_usage_cnt)
+/**
+ * DOC: ION_IOC_HEAP_QUERY - information about available heaps
+ *
+ * Takes an ion_heap_query structure and populates information about
+ * availble Ion heaps.
+ */
+#define ION_IOC_HEAP_QUERY     _IOWR(ION_IOC_MAGIC, 12, \
+					struct ion_heap_query)
 
 #endif /* _UAPI_LINUX_ION_H */
-- 
2.5.5

Re: [RFC][PATCH 6/6] staging: android: ion: Introduce new ioctls for dynamic heaps

[Liviu Dudau]

On Mon, Jun 06, 2016 at 11:23:33AM -0700, Laura Abbott wrote:
> From: Laura Abbott <labbott@fedoraproject.org>
> 
> 
> The Ion ABI for heaps is limiting to work with for more complex systems.
> Heaps have to be registered at boot time with known ids available to
> userspace. This becomes a tight ABI which is prone to breakage.
> 
> Introduce a new mechanism for registering heap ids dynamically. A base
> set of heap ids are registered at boot time but there is no knowledge
> of fallbacks. Fallback information can be remapped and changed
> dynamically. Information about available heaps can also be queried with
> an ioctl to avoid the need to have heap ids be an ABI with userspace.
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
>  drivers/staging/android/ion/ion-ioctl.c | 109 +++++++++++++++++--
>  drivers/staging/android/ion/ion.c       | 184 ++++++++++++++++++++++++++++++--
>  drivers/staging/android/ion/ion_priv.h  |  15 +++
>  drivers/staging/android/uapi/ion.h      | 135 +++++++++++++++++++++++
>  4 files changed, 426 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> index 45b89e8..169cad8 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -22,6 +22,49 @@
>  #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;
> +	struct ion_new_alloc_data allocation2;
> +	struct ion_usage_id_map id_map;
> +	struct ion_usage_cnt usage_cnt;
> +	struct ion_heap_query query;
> +};
> +
> +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;
> +	case ION_IOC_ALLOC2:
> +		ret = arg->allocation2.reserved0 != 0;
> +		ret |= arg->allocation2.reserved1 != 0;
> +		ret |= arg->allocation2.reserved2 != 0;
> +		break;
> +	case ION_IOC_ID_MAP:
> +		ret = arg->id_map.reserved0 != 0;
> +		ret |= arg->id_map.reserved1 != 0;
> +		break;
> +	case ION_IOC_USAGE_CNT:
> +		ret = arg->usage_cnt.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;
> +	}
> +	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,14 +85,7 @@ 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;
> -		struct ion_abi_version abi_version;
> -	} data;
> +	union ion_ioctl_arg data;
>  
>  	dir = ion_ioctl_dir(cmd);
>  
> @@ -60,7 +96,12 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
>  			return -EFAULT;
>  
> +	ret = validate_ioctl_arg(cmd, &data);
> +	if (ret)
> +		return ret;
> +
>  	switch (cmd) {
> +	/* Old ioctl */

I can see the value of this comment, given ION_IOC_ALLOC2, but not for the other cases.

>  	case ION_IOC_ALLOC:
>  	{
>  		struct ion_handle *handle;
> @@ -77,6 +118,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		cleanup_handle = handle;
>  		break;
>  	}
> +	/* Old ioctl */
>  	case ION_IOC_FREE:
>  	{
>  		struct ion_handle *handle;
> @@ -92,6 +134,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		mutex_unlock(&client->lock);
>  		break;
>  	}
> +	/* Old ioctl */
>  	case ION_IOC_SHARE:
>  	case ION_IOC_MAP:
>  	{
> @@ -106,6 +149,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  			ret = data.fd.fd;
>  		break;
>  	}
> +	/* Old ioctl */
>  	case ION_IOC_IMPORT:
>  	{
>  		struct ion_handle *handle;
> @@ -117,11 +161,13 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  			data.handle.handle = handle->id;
>  		break;
>  	}
> +	/* Old ioctl */
>  	case ION_IOC_SYNC:
>  	{
>  		ret = ion_sync_for_device(client, data.fd.fd);
>  		break;
>  	}
> +	/* Old ioctl */
>  	case ION_IOC_CUSTOM:
>  	{
>  		if (!dev->custom_ioctl)
> @@ -135,6 +181,53 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		data.abi_version.abi_version = ION_ABI_VERSION;
>  		break;
>  	}
> +	case ION_IOC_ALLOC2:
> +	{
> +		struct ion_handle *handle;
> +
> +		handle = ion_alloc2(client, data.allocation2.len,
> +					data.allocation2.align,
> +					data.allocation2.usage_id,
> +					data.allocation2.flags);
> +		if (IS_ERR(handle))
> +			return PTR_ERR(handle);
> +
> +		if (data.allocation2.flags & ION_FLAG_NO_HANDLE) {
> +			data.allocation2.fd = ion_share_dma_buf_fd(client,
> +								handle);
> +			ion_handle_put(handle);
> +			if (data.allocation2.fd < 0)
> +				ret = data.allocation2.fd;
> +		} else {
> +			data.allocation2.handle = handle->id;
> +
> +			cleanup_handle = handle;
> +		}
> +		break;
> +	}
> +	case ION_IOC_ID_MAP:
> +	{
> +		ret = ion_map_usage_ids(client,
> +				(unsigned int __user *)data.id_map.usage_ids,
> +				data.id_map.cnt);
> +		if (ret > 0)
> +			data.id_map.new_id = ret;
> +		break;
> +	}
> +	case ION_IOC_USAGE_CNT:
> +	{
> +		down_read(&client->dev->lock);
> +		data.usage_cnt.cnt = client->dev->heap_cnt;
> +		up_read(&client->dev->lock);
> +		break;
> +	}
> +	case ION_IOC_HEAP_QUERY:
> +	{
> +		ret = ion_query_heaps(client,
> +				(struct ion_heap_data __user *)data.query.heaps,
> +				data.query.cnt);
> +		break;
> +	}
>  	default:
>  		return -ENOTTY;
>  	}
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 129707f..c096cb9 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -459,6 +459,48 @@ static struct ion_handle *__ion_alloc(struct ion_client *client, size_t len,
>  	return handle;
>  }
>  
> +struct ion_handle *ion_alloc2(struct ion_client *client, size_t len,
> +				size_t align, unsigned int usage_id,
> +				unsigned int flags)
> +{
> +	struct ion_device *dev = client->dev;
> +	struct ion_heap *heap;
> +	struct ion_handle *handle = ERR_PTR(-ENODEV);
> +
> +	down_read(&dev->lock);
> +	heap = idr_find(&dev->idr, usage_id);
> +	if (!heap) {
> +		handle = ERR_PTR(-EINVAL);
> +		goto out;
> +	}
> +
> +	if (heap->type == ION_USAGE_ID_MAP) {
> +		int i;
> +
> +		for (i = 0; i < heap->fallback_cnt; i++){
> +			heap = idr_find(&dev->idr, heap->fallbacks[i]);
> +			if (!heap)
> +				continue;
> +
> +			/* Don't recurse for now? */
> +			if (heap->type == ION_USAGE_ID_MAP)
> +				continue;
> +
> +			handle =  __ion_alloc(client, len, align, heap, flags);
> +			if (IS_ERR(handle))
> +				continue;
> +			else
> +				break;
> +		}
> +	} else {
> +		handle = __ion_alloc(client, len, align, heap, flags);
> +	}
> +out:
> +	up_read(&dev->lock);
> +	return handle;
> +}
> +EXPORT_SYMBOL(ion_alloc2);
> +
>  struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
>  				size_t align, unsigned int heap_mask,
>  				unsigned int flags)
> @@ -1243,6 +1285,55 @@ int ion_sync_for_device(struct ion_client *client, int fd)
>  	return 0;
>  }
>  
> +struct ion_query_data {
> +	struct ion_heap_data __user *buffer;
> +	int cnt;
> +	int max_cnt;
> +};
> +
> +int __ion_query(int id, void *p, void *data)
> +{
> +	struct ion_heap *heap = p;
> +	struct ion_query_data *query = data;
> +	struct ion_heap_data hdata = {0};
> +
> +	if (query->cnt >= query->max_cnt)
> +		return -ENOSPC;
> +
> +	strncpy(hdata.name, heap->name, 20);
> +	hdata.name[sizeof(hdata.name) - 1] = '\0';
> +	hdata.type = heap->type;
> +	hdata.usage_id = heap->id;
> +
> +	return copy_to_user(&query->buffer[query->cnt++], &hdata, sizeof(hdata));
> +}
> +
> +int ion_query_heaps(struct ion_client *client,
> +		struct ion_heap_data __user *buffer,
> +		int cnt)
> +{
> +	struct ion_device *dev = client->dev;
> +	struct ion_query_data data;
> +	int ret;
> +
> +	data.buffer = buffer;
> +	data.cnt = 0;
> +	data.max_cnt = cnt;
> +
> +	down_read(&dev->lock);
> +	if (data.max_cnt < 0 || data.max_cnt > dev->heap_cnt) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	ret = idr_for_each(&dev->idr, __ion_query, &data);
> +out:
> +	up_read(&dev->lock);
> +
> +	return ret;
> +}
> +
> +
> +
>  static int ion_release(struct inode *inode, struct file *file)
>  {
>  	struct ion_client *client = file->private_data;
> @@ -1407,13 +1498,69 @@ static int debug_shrink_get(void *data, u64 *val)
>  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>  			debug_shrink_set, "%llu\n");
>  
> +int ion_map_usage_ids(struct ion_client *client,
> +			unsigned int __user *usage_ids,
> +			int cnt)
> +{
> +	struct ion_heap *heap = kzalloc(sizeof(*heap), GFP_KERNEL);
> +	unsigned int *fallbacks;
> +	int i;
> +	int ret;
> +
> +	if (!heap)
> +		return -ENOMEM;
> +
> +	fallbacks = kzalloc(sizeof(unsigned int)*cnt, GFP_KERNEL);
> +	if (!fallbacks) {
> +		ret = -ENOMEM;
> +		goto out1;
> +	}
> +
> +	ret = copy_from_user(fallbacks, usage_ids, sizeof(unsigned int)*cnt);
> +	if (ret)
> +		goto out2;
> +
> +	down_read(&client->dev->lock);
> +	for (i = 0; i < cnt; i++) {
> +		if (idr_find(&client->dev->idr, fallbacks[i]) == NULL) {
> +			ret = -EINVAL;
> +			goto out3;
> +		}
> +	}
> +	up_read(&client->dev->lock);
> +
> +	/*
> +	 * This is a racy check since the lock is dropped before the heap
> +	 * is actually added. It's okay though because ids are never actually
> +	 * deleted. Worst case some user gets an error back and an indication
> +	 * to fix races in their code.
> +	 */
> +
> +	heap->fallbacks = fallbacks;
> +	heap->fallback_cnt = cnt;
> +	heap->type = ION_USAGE_ID_MAP;
> +	heap->id = ION_DYNAMIC_HEAP_ASSIGN;
> +	ion_device_add_heap(client->dev, heap);
> +	return heap->id;
> +out3:
> +	up_read(&client->dev->lock);
> +out2:
> +	kfree(fallbacks);
> +out1:
> +	kfree(heap);
> +	return ret;
> +}
> +
>  int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
>  {
>  	struct dentry *debug_file;
>  	int ret;
>  
> -	if (!heap->ops->allocate || !heap->ops->free || !heap->ops->map_dma ||
> -	    !heap->ops->unmap_dma) {
> +	if (heap->type != ION_USAGE_ID_MAP &&
> +	   (!heap->ops->allocate ||
> +	    !heap->ops->free ||
> +	    !heap->ops->map_dma ||
> +	    !heap->ops->unmap_dma)) {
>  		pr_err("%s: can not add heap with invalid ops struct.\n",
>  		       __func__);
>  		return -EINVAL;
> @@ -1425,18 +1572,36 @@ int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
>  	if (heap->flags & ION_HEAP_FLAG_DEFER_FREE)
>  		ion_heap_init_deferred_free(heap);
>  
> -	if ((heap->flags & ION_HEAP_FLAG_DEFER_FREE) || heap->ops->shrink)
> +	if ((heap->flags & ION_HEAP_FLAG_DEFER_FREE) || (heap->ops && heap->ops->shrink))
>  		ion_heap_init_shrinker(heap);
>  
>  	heap->dev = dev;
>  	down_write(&dev->lock);
>  
> -	ret = idr_alloc(&dev->idr, heap, heap->id, heap->id + 1, GFP_KERNEL);
> -	if (ret < 0 || ret != heap->id) {
> -		pr_info("%s: Failed to add heap id, expected %d got %d\n",
> -			__func__, heap->id, ret);
> -		up_write(&dev->lock);
> -		return ret < 0 ? ret : -EINVAL;
> +	if (heap->id == ION_DYNAMIC_HEAP_ASSIGN) {
> +		ret = idr_alloc(&dev->idr, heap,
> +				ION_DYNAMIC_HEAP_ASSIGN + 1, 0, GFP_KERNEL);

start = ION_DYNAMIC_HEAP_ASSIGN + 1, end = 0 (aka max) ? Why not end = ION_DYNAMIC_HEAP_ASSIGN + head->fallback_cnt + 1?

At least some comment is warranted explaining the choices here.

> +		if (ret < 0)
> +			goto out_unlock;
> +
> +		heap->id = ret;
> +	} else {
> +		ret = idr_alloc(&dev->idr, heap, heap->id, heap->id + 1,
> +				GFP_KERNEL);
> +		if (ret < 0 || ret != heap->id) {
> +			pr_info("%s: Failed to add heap id, expected %d got %d\n",
> +				__func__, heap->id, ret);
> +			ret = ret < 0 ? ret : -EINVAL;
> +			goto out_unlock;
> +		}
> +	}
> +
> +	if (!heap->name) {
> +		heap->name = kasprintf(GFP_KERNEL, "heap%d", heap->id);
> +		if (!heap->name) {
> +			ret = -ENOMEM;
> +			goto out_unlock;
> +		}
>  	}
>  
>  	debug_file = debugfs_create_file(heap->name, 0664,
> @@ -1467,6 +1632,7 @@ int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
>  		}
>  	}
>  	dev->heap_cnt++;
> +out_unlock:
>  	up_write(&dev->lock);
>  	return 0;
>  }
> diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h
> index b09d751..e03e940 100644
> --- a/drivers/staging/android/ion/ion_priv.h
> +++ b/drivers/staging/android/ion/ion_priv.h
> @@ -255,6 +255,9 @@ struct ion_heap {
>  	wait_queue_head_t waitqueue;
>  	struct task_struct *task;
>  
> +	unsigned int *fallbacks;
> +	int fallback_cnt;
> +
>  	int (*debug_show)(struct ion_heap *heap, struct seq_file *, void *);
>  };
>  
> @@ -381,6 +384,10 @@ size_t ion_heap_freelist_shrink(struct ion_heap *heap,
>  size_t ion_heap_freelist_size(struct ion_heap *heap);
>  
>  
> +int ion_map_usage_ids(struct ion_client *client,
> +			unsigned int __user *usage_ids,
> +			int cnt);
> +
>  /**
>   * functions for creating and destroying the built in ion heaps.
>   * architectures can add their own custom architecture specific
> @@ -495,4 +502,12 @@ struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
>  
>  int ion_handle_put(struct ion_handle *handle);
>  
> +struct ion_handle *ion_alloc2(struct ion_client *client, size_t len,
> +				size_t align, unsigned int usage_id,
> +				unsigned int flags);
> +
> +int ion_query_heaps(struct ion_client *client,
> +		struct ion_heap_data __user *buffer,
> +		int cnt);
> +
>  #endif /* _ION_PRIV_H */
> diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
> index 145005f..d36b4e4 100644
> --- a/drivers/staging/android/uapi/ion.h
> +++ b/drivers/staging/android/uapi/ion.h
> @@ -45,10 +45,17 @@ enum ion_heap_type {
>  			       * must be last so device specific heaps always
>  			       * are at the end of this enum
>  			       */
> +	ION_USAGE_ID_MAP,
>  };
>  
>  #define ION_NUM_HEAP_IDS		(sizeof(unsigned int) * 8)
>  
> +/*
> + * This isn't completely random. The old Ion heap ID namespace goes up to
> + * 32 bits so take the first one after that to use as a dynamic setting
> + */
> +#define ION_DYNAMIC_HEAP_ASSIGN		33

Also worth adding that no higher IDs should be defined after this because it's going
to break the code.

> +
>  /**
>   * allocation flags - the lower 16 bits are used by core ion, the upper 16
>   * bits are reserved for use by the heaps themselves.
> @@ -65,6 +72,11 @@ enum ion_heap_type {
>  					 * caches must be managed
>  					 * manually
>  					 */
> +#define ION_FLAG_NO_HANDLE 3		/*
> +					 * Return only an fd associated with
> +					 * this buffer and not a handle
> +					 */
> +
>  
>  /**
>   * DOC: Ion Userspace API
> @@ -142,6 +154,96 @@ struct ion_abi_version {
>  	__u32 reserved;
>  };
>  
> +/**
> + * struct ion_new_alloc_data - metadata to/from usersapce allocation v2
> + * @len:		size of the allocation
> + * @align:		required alignment of the allocation
> + * @usage_id:		mapping to heap id to allocate. Will try fallbacks
> + *			if specified in the heap mapping
> + * @flags:		flags passed to heap
> + * @handle:		pointer that will be populated with a cookie to use to
> + *			refer to this allocation
> + * @fd:			optionally, may return just an fd with no handle
> + *			reference
> + */
> +struct ion_new_alloc_data {
> +	__u64 len;
> +	__u64 align;
> +	__u32 usage_id;
> +	__u32 flags;
> +	/*
> +	 * I'd like to add a flag to just return the fd instead of just
> +	 * a handle for those who want to skip the next step.
> +	 */
> +	union {
> +		__u32 fd;
> +		__u32 handle;
> +	};
> +	/*
> +	 * Allocation has a definite problem of 'creep' so give more than
> +	 * enough extra bits for expansion
> +	 */
> +	__u32 reserved0;
> +	__u32 reserved1;
> +	__u32 reserved2;
> +};
> +
> +/**
> + * struct ion_usage_id_map - metadata to create a new usage map
> + * @usage_id - userspace allocated array of existing usage ids
> + * @cnt - number of ids to be mapped
> + * @new_id - will be populated with the new usage id
> + */
> +struct ion_usage_id_map {
> +	/* Array of usage ids provided by user */
> +	__u64 usage_ids;
> +	__u32 cnt;
> +
> +	/* Returned on success */
> +	__u32 new_id;
> +	/* Fields for future growth */
> +	__u32 reserved0;
> +	__u32 reserved1;
> +};
> +
> +/**
> + * struct ion_usage_cnt - meta data for the count of heaps
> + * @cnt - returned count of number of heaps present
> + */
> +struct ion_usage_cnt {
> +	__u32 cnt; /* cnt returned */
> +	__u32 reserved;
> +};
> +
> +/**
> + * struct ion_heap_data - data about a heap
> + * @name - first 32 characters of the heap name
> + * @type - heap type
> + * @usage_id - usage id for the heap
> + */
> +struct ion_heap_data {
> +	char name[32];
> +	__u32 type;
> +	__u32 usage_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 */
> +	__u64 heaps; /* buffer to be populated */
> +	__u32 reserved0;
> +	__u32 reserved1;
> +	__u32 reserved2;
> +};
> +
> +
>  #define ION_IOC_MAGIC		'I'
>  
>  /**
> @@ -216,5 +318,38 @@ struct ion_abi_version {
>  #define ION_IOC_ABI_VERSION    _IOR(ION_IOC_MAGIC, 8, \
>  					struct ion_abi_version)
>  
> +/**
> + * DOC: ION_IOC_ALLOC2 - allocate memory via new API
> + *
> + * Same concept as ION_IOC_ALLOC except with updated ABI. Can return an fd
> + * directly in addition to a handle
> + */
> +#define ION_IOC_ALLOC2           _IOWR(ION_IOC_MAGIC, 9, \
> +					struct ion_new_alloc_data)
> +
> +/**
> + * DOC: ION_IOC_ID_MAP - remap an array of heap IDs
> + *
> + * Takes an ion_usage_id_map structure populated with information about
> + * fallback information. Returns a new usage id for use in allocation.
> + */
> +#define ION_IOC_ID_MAP         _IOWR(ION_IOC_MAGIC, 10, \
> +					struct ion_usage_id_map)
> +/**
> + * DOC: ION_IOC_USAGE_CNT - returns a count of the number of usage ids
> + *
> + * Takes an ion_usage_cnt structure and returns the total number of usage
> + * ids available.
> + */
> +#define ION_IOC_USAGE_CNT      _IOR(ION_IOC_MAGIC, 11, \
> +					struct ion_usage_cnt)
> +/**
> + * DOC: ION_IOC_HEAP_QUERY - information about available heaps
> + *
> + * Takes an ion_heap_query structure and populates information about
> + * availble Ion heaps.
> + */
> +#define ION_IOC_HEAP_QUERY     _IOWR(ION_IOC_MAGIC, 12, \
> +					struct ion_heap_query)
>  
>  #endif /* _UAPI_LINUX_ION_H */
> -- 
> 2.5.5
> 

I know that ION doesn't have any kernel documentation or examples in terms of what userspace
should do, but after this patchset I feel like adding some text describing how the dynamic
heap mapping might work on an ideal device that you target is useful.

Best regards,
Liviu

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

Re: [RFC][PATCH 6/6] staging: android: ion: Introduce new ioctls for dynamic heaps

[Laura Abbott]

On 06/08/2016 06:50 AM, Liviu Dudau wrote:
> On Mon, Jun 06, 2016 at 11:23:33AM -0700, Laura Abbott wrote:
>> From: Laura Abbott <labbott@fedoraproject.org>
>>
>>
>> The Ion ABI for heaps is limiting to work with for more complex systems.
>> Heaps have to be registered at boot time with known ids available to
>> userspace. This becomes a tight ABI which is prone to breakage.
>>
>> Introduce a new mechanism for registering heap ids dynamically. A base
>> set of heap ids are registered at boot time but there is no knowledge
>> of fallbacks. Fallback information can be remapped and changed
>> dynamically. Information about available heaps can also be queried with
>> an ioctl to avoid the need to have heap ids be an ABI with userspace.
>>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>>  drivers/staging/android/ion/ion-ioctl.c | 109 +++++++++++++++++--
>>  drivers/staging/android/ion/ion.c       | 184 ++++++++++++++++++++++++++++++--
>>  drivers/staging/android/ion/ion_priv.h  |  15 +++
>>  drivers/staging/android/uapi/ion.h      | 135 +++++++++++++++++++++++
>>  4 files changed, 426 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
>> index 45b89e8..169cad8 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -22,6 +22,49 @@
>>  #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;
>> +	struct ion_new_alloc_data allocation2;
>> +	struct ion_usage_id_map id_map;
>> +	struct ion_usage_cnt usage_cnt;
>> +	struct ion_heap_query query;
>> +};
>> +
>> +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;
>> +	case ION_IOC_ALLOC2:
>> +		ret = arg->allocation2.reserved0 != 0;
>> +		ret |= arg->allocation2.reserved1 != 0;
>> +		ret |= arg->allocation2.reserved2 != 0;
>> +		break;
>> +	case ION_IOC_ID_MAP:
>> +		ret = arg->id_map.reserved0 != 0;
>> +		ret |= arg->id_map.reserved1 != 0;
>> +		break;
>> +	case ION_IOC_USAGE_CNT:
>> +		ret = arg->usage_cnt.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;
>> +	}
>> +	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,14 +85,7 @@ 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;
>> -		struct ion_abi_version abi_version;
>> -	} data;
>> +	union ion_ioctl_arg data;
>>
>>  	dir = ion_ioctl_dir(cmd);
>>
>> @@ -60,7 +96,12 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>  		if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
>>  			return -EFAULT;
>>
>> +	ret = validate_ioctl_arg(cmd, &data);
>> +	if (ret)
>> +		return ret;
>> +
>>  	switch (cmd) {
>> +	/* Old ioctl */
>
> I can see the value of this comment, given ION_IOC_ALLOC2, but not for the other cases.
>
>>  	case ION_IOC_ALLOC:
>>  	{
>>  		struct ion_handle *handle;
>> @@ -77,6 +118,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>  		cleanup_handle = handle;
>>  		break;
>>  	}
>> +	/* Old ioctl */
>>  	case ION_IOC_FREE:
>>  	{
>>  		struct ion_handle *handle;
>> @@ -92,6 +134,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>  		mutex_unlock(&client->lock);
>>  		break;
>>  	}
>> +	/* Old ioctl */
>>  	case ION_IOC_SHARE:
>>  	case ION_IOC_MAP:
>>  	{
>> @@ -106,6 +149,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>  			ret = data.fd.fd;
>>  		break;
>>  	}
>> +	/* Old ioctl */
>>  	case ION_IOC_IMPORT:
>>  	{
>>  		struct ion_handle *handle;
>> @@ -117,11 +161,13 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>  			data.handle.handle = handle->id;
>>  		break;
>>  	}
>> +	/* Old ioctl */
>>  	case ION_IOC_SYNC:
>>  	{
>>  		ret = ion_sync_for_device(client, data.fd.fd);
>>  		break;
>>  	}
>> +	/* Old ioctl */
>>  	case ION_IOC_CUSTOM:
>>  	{
>>  		if (!dev->custom_ioctl)
>> @@ -135,6 +181,53 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>  		data.abi_version.abi_version = ION_ABI_VERSION;
>>  		break;
>>  	}
>> +	case ION_IOC_ALLOC2:
>> +	{
>> +		struct ion_handle *handle;
>> +
>> +		handle = ion_alloc2(client, data.allocation2.len,
>> +					data.allocation2.align,
>> +					data.allocation2.usage_id,
>> +					data.allocation2.flags);
>> +		if (IS_ERR(handle))
>> +			return PTR_ERR(handle);
>> +
>> +		if (data.allocation2.flags & ION_FLAG_NO_HANDLE) {
>> +			data.allocation2.fd = ion_share_dma_buf_fd(client,
>> +								handle);
>> +			ion_handle_put(handle);
>> +			if (data.allocation2.fd < 0)
>> +				ret = data.allocation2.fd;
>> +		} else {
>> +			data.allocation2.handle = handle->id;
>> +
>> +			cleanup_handle = handle;
>> +		}
>> +		break;
>> +	}
>> +	case ION_IOC_ID_MAP:
>> +	{
>> +		ret = ion_map_usage_ids(client,
>> +				(unsigned int __user *)data.id_map.usage_ids,
>> +				data.id_map.cnt);
>> +		if (ret > 0)
>> +			data.id_map.new_id = ret;
>> +		break;
>> +	}
>> +	case ION_IOC_USAGE_CNT:
>> +	{
>> +		down_read(&client->dev->lock);
>> +		data.usage_cnt.cnt = client->dev->heap_cnt;
>> +		up_read(&client->dev->lock);
>> +		break;
>> +	}
>> +	case ION_IOC_HEAP_QUERY:
>> +	{
>> +		ret = ion_query_heaps(client,
>> +				(struct ion_heap_data __user *)data.query.heaps,
>> +				data.query.cnt);
>> +		break;
>> +	}
>>  	default:
>>  		return -ENOTTY;
>>  	}
>> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
>> index 129707f..c096cb9 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -459,6 +459,48 @@ static struct ion_handle *__ion_alloc(struct ion_client *client, size_t len,
>>  	return handle;
>>  }
>>
>> +struct ion_handle *ion_alloc2(struct ion_client *client, size_t len,
>> +				size_t align, unsigned int usage_id,
>> +				unsigned int flags)
>> +{
>> +	struct ion_device *dev = client->dev;
>> +	struct ion_heap *heap;
>> +	struct ion_handle *handle = ERR_PTR(-ENODEV);
>> +
>> +	down_read(&dev->lock);
>> +	heap = idr_find(&dev->idr, usage_id);
>> +	if (!heap) {
>> +		handle = ERR_PTR(-EINVAL);
>> +		goto out;
>> +	}
>> +
>> +	if (heap->type == ION_USAGE_ID_MAP) {
>> +		int i;
>> +
>> +		for (i = 0; i < heap->fallback_cnt; i++){
>> +			heap = idr_find(&dev->idr, heap->fallbacks[i]);
>> +			if (!heap)
>> +				continue;
>> +
>> +			/* Don't recurse for now? */
>> +			if (heap->type == ION_USAGE_ID_MAP)
>> +				continue;
>> +
>> +			handle =  __ion_alloc(client, len, align, heap, flags);
>> +			if (IS_ERR(handle))
>> +				continue;
>> +			else
>> +				break;
>> +		}
>> +	} else {
>> +		handle = __ion_alloc(client, len, align, heap, flags);
>> +	}
>> +out:
>> +	up_read(&dev->lock);
>> +	return handle;
>> +}
>> +EXPORT_SYMBOL(ion_alloc2);
>> +
>>  struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
>>  				size_t align, unsigned int heap_mask,
>>  				unsigned int flags)
>> @@ -1243,6 +1285,55 @@ int ion_sync_for_device(struct ion_client *client, int fd)
>>  	return 0;
>>  }
>>
>> +struct ion_query_data {
>> +	struct ion_heap_data __user *buffer;
>> +	int cnt;
>> +	int max_cnt;
>> +};
>> +
>> +int __ion_query(int id, void *p, void *data)
>> +{
>> +	struct ion_heap *heap = p;
>> +	struct ion_query_data *query = data;
>> +	struct ion_heap_data hdata = {0};
>> +
>> +	if (query->cnt >= query->max_cnt)
>> +		return -ENOSPC;
>> +
>> +	strncpy(hdata.name, heap->name, 20);
>> +	hdata.name[sizeof(hdata.name) - 1] = '\0';
>> +	hdata.type = heap->type;
>> +	hdata.usage_id = heap->id;
>> +
>> +	return copy_to_user(&query->buffer[query->cnt++], &hdata, sizeof(hdata));
>> +}
>> +
>> +int ion_query_heaps(struct ion_client *client,
>> +		struct ion_heap_data __user *buffer,
>> +		int cnt)
>> +{
>> +	struct ion_device *dev = client->dev;
>> +	struct ion_query_data data;
>> +	int ret;
>> +
>> +	data.buffer = buffer;
>> +	data.cnt = 0;
>> +	data.max_cnt = cnt;
>> +
>> +	down_read(&dev->lock);
>> +	if (data.max_cnt < 0 || data.max_cnt > dev->heap_cnt) {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +	ret = idr_for_each(&dev->idr, __ion_query, &data);
>> +out:
>> +	up_read(&dev->lock);
>> +
>> +	return ret;
>> +}
>> +
>> +
>> +
>>  static int ion_release(struct inode *inode, struct file *file)
>>  {
>>  	struct ion_client *client = file->private_data;
>> @@ -1407,13 +1498,69 @@ static int debug_shrink_get(void *data, u64 *val)
>>  DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>>  			debug_shrink_set, "%llu\n");
>>
>> +int ion_map_usage_ids(struct ion_client *client,
>> +			unsigned int __user *usage_ids,
>> +			int cnt)
>> +{
>> +	struct ion_heap *heap = kzalloc(sizeof(*heap), GFP_KERNEL);
>> +	unsigned int *fallbacks;
>> +	int i;
>> +	int ret;
>> +
>> +	if (!heap)
>> +		return -ENOMEM;
>> +
>> +	fallbacks = kzalloc(sizeof(unsigned int)*cnt, GFP_KERNEL);
>> +	if (!fallbacks) {
>> +		ret = -ENOMEM;
>> +		goto out1;
>> +	}
>> +
>> +	ret = copy_from_user(fallbacks, usage_ids, sizeof(unsigned int)*cnt);
>> +	if (ret)
>> +		goto out2;
>> +
>> +	down_read(&client->dev->lock);
>> +	for (i = 0; i < cnt; i++) {
>> +		if (idr_find(&client->dev->idr, fallbacks[i]) == NULL) {
>> +			ret = -EINVAL;
>> +			goto out3;
>> +		}
>> +	}
>> +	up_read(&client->dev->lock);
>> +
>> +	/*
>> +	 * This is a racy check since the lock is dropped before the heap
>> +	 * is actually added. It's okay though because ids are never actually
>> +	 * deleted. Worst case some user gets an error back and an indication
>> +	 * to fix races in their code.
>> +	 */
>> +
>> +	heap->fallbacks = fallbacks;
>> +	heap->fallback_cnt = cnt;
>> +	heap->type = ION_USAGE_ID_MAP;
>> +	heap->id = ION_DYNAMIC_HEAP_ASSIGN;
>> +	ion_device_add_heap(client->dev, heap);
>> +	return heap->id;
>> +out3:
>> +	up_read(&client->dev->lock);
>> +out2:
>> +	kfree(fallbacks);
>> +out1:
>> +	kfree(heap);
>> +	return ret;
>> +}
>> +
>>  int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
>>  {
>>  	struct dentry *debug_file;
>>  	int ret;
>>
>> -	if (!heap->ops->allocate || !heap->ops->free || !heap->ops->map_dma ||
>> -	    !heap->ops->unmap_dma) {
>> +	if (heap->type != ION_USAGE_ID_MAP &&
>> +	   (!heap->ops->allocate ||
>> +	    !heap->ops->free ||
>> +	    !heap->ops->map_dma ||
>> +	    !heap->ops->unmap_dma)) {
>>  		pr_err("%s: can not add heap with invalid ops struct.\n",
>>  		       __func__);
>>  		return -EINVAL;
>> @@ -1425,18 +1572,36 @@ int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
>>  	if (heap->flags & ION_HEAP_FLAG_DEFER_FREE)
>>  		ion_heap_init_deferred_free(heap);
>>
>> -	if ((heap->flags & ION_HEAP_FLAG_DEFER_FREE) || heap->ops->shrink)
>> +	if ((heap->flags & ION_HEAP_FLAG_DEFER_FREE) || (heap->ops && heap->ops->shrink))
>>  		ion_heap_init_shrinker(heap);
>>
>>  	heap->dev = dev;
>>  	down_write(&dev->lock);
>>
>> -	ret = idr_alloc(&dev->idr, heap, heap->id, heap->id + 1, GFP_KERNEL);
>> -	if (ret < 0 || ret != heap->id) {
>> -		pr_info("%s: Failed to add heap id, expected %d got %d\n",
>> -			__func__, heap->id, ret);
>> -		up_write(&dev->lock);
>> -		return ret < 0 ? ret : -EINVAL;
>> +	if (heap->id == ION_DYNAMIC_HEAP_ASSIGN) {
>> +		ret = idr_alloc(&dev->idr, heap,
>> +				ION_DYNAMIC_HEAP_ASSIGN + 1, 0, GFP_KERNEL);
>
> start = ION_DYNAMIC_HEAP_ASSIGN + 1, end = 0 (aka max) ? Why not end = ION_DYNAMIC_HEAP_ASSIGN + head->fallback_cnt + 1?
>
> At least some comment is warranted explaining the choices here.
>
>> +		if (ret < 0)
>> +			goto out_unlock;
>> +
>> +		heap->id = ret;
>> +	} else {
>> +		ret = idr_alloc(&dev->idr, heap, heap->id, heap->id + 1,
>> +				GFP_KERNEL);
>> +		if (ret < 0 || ret != heap->id) {
>> +			pr_info("%s: Failed to add heap id, expected %d got %d\n",
>> +				__func__, heap->id, ret);
>> +			ret = ret < 0 ? ret : -EINVAL;
>> +			goto out_unlock;
>> +		}
>> +	}
>> +
>> +	if (!heap->name) {
>> +		heap->name = kasprintf(GFP_KERNEL, "heap%d", heap->id);
>> +		if (!heap->name) {
>> +			ret = -ENOMEM;
>> +			goto out_unlock;
>> +		}
>>  	}
>>
>>  	debug_file = debugfs_create_file(heap->name, 0664,
>> @@ -1467,6 +1632,7 @@ int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
>>  		}
>>  	}
>>  	dev->heap_cnt++;
>> +out_unlock:
>>  	up_write(&dev->lock);
>>  	return 0;
>>  }
>> diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h
>> index b09d751..e03e940 100644
>> --- a/drivers/staging/android/ion/ion_priv.h
>> +++ b/drivers/staging/android/ion/ion_priv.h
>> @@ -255,6 +255,9 @@ struct ion_heap {
>>  	wait_queue_head_t waitqueue;
>>  	struct task_struct *task;
>>
>> +	unsigned int *fallbacks;
>> +	int fallback_cnt;
>> +
>>  	int (*debug_show)(struct ion_heap *heap, struct seq_file *, void *);
>>  };
>>
>> @@ -381,6 +384,10 @@ size_t ion_heap_freelist_shrink(struct ion_heap *heap,
>>  size_t ion_heap_freelist_size(struct ion_heap *heap);
>>
>>
>> +int ion_map_usage_ids(struct ion_client *client,
>> +			unsigned int __user *usage_ids,
>> +			int cnt);
>> +
>>  /**
>>   * functions for creating and destroying the built in ion heaps.
>>   * architectures can add their own custom architecture specific
>> @@ -495,4 +502,12 @@ struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
>>
>>  int ion_handle_put(struct ion_handle *handle);
>>
>> +struct ion_handle *ion_alloc2(struct ion_client *client, size_t len,
>> +				size_t align, unsigned int usage_id,
>> +				unsigned int flags);
>> +
>> +int ion_query_heaps(struct ion_client *client,
>> +		struct ion_heap_data __user *buffer,
>> +		int cnt);
>> +
>>  #endif /* _ION_PRIV_H */
>> diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
>> index 145005f..d36b4e4 100644
>> --- a/drivers/staging/android/uapi/ion.h
>> +++ b/drivers/staging/android/uapi/ion.h
>> @@ -45,10 +45,17 @@ enum ion_heap_type {
>>  			       * must be last so device specific heaps always
>>  			       * are at the end of this enum
>>  			       */
>> +	ION_USAGE_ID_MAP,
>>  };
>>
>>  #define ION_NUM_HEAP_IDS		(sizeof(unsigned int) * 8)
>>
>> +/*
>> + * This isn't completely random. The old Ion heap ID namespace goes up to
>> + * 32 bits so take the first one after that to use as a dynamic setting
>> + */
>> +#define ION_DYNAMIC_HEAP_ASSIGN		33
>
> Also worth adding that no higher IDs should be defined after this because it's going
> to break the code.
>
>> +
>>  /**
>>   * allocation flags - the lower 16 bits are used by core ion, the upper 16
>>   * bits are reserved for use by the heaps themselves.
>> @@ -65,6 +72,11 @@ enum ion_heap_type {
>>  					 * caches must be managed
>>  					 * manually
>>  					 */
>> +#define ION_FLAG_NO_HANDLE 3		/*
>> +					 * Return only an fd associated with
>> +					 * this buffer and not a handle
>> +					 */
>> +
>>
>>  /**
>>   * DOC: Ion Userspace API
>> @@ -142,6 +154,96 @@ struct ion_abi_version {
>>  	__u32 reserved;
>>  };
>>
>> +/**
>> + * struct ion_new_alloc_data - metadata to/from usersapce allocation v2
>> + * @len:		size of the allocation
>> + * @align:		required alignment of the allocation
>> + * @usage_id:		mapping to heap id to allocate. Will try fallbacks
>> + *			if specified in the heap mapping
>> + * @flags:		flags passed to heap
>> + * @handle:		pointer that will be populated with a cookie to use to
>> + *			refer to this allocation
>> + * @fd:			optionally, may return just an fd with no handle
>> + *			reference
>> + */
>> +struct ion_new_alloc_data {
>> +	__u64 len;
>> +	__u64 align;
>> +	__u32 usage_id;
>> +	__u32 flags;
>> +	/*
>> +	 * I'd like to add a flag to just return the fd instead of just
>> +	 * a handle for those who want to skip the next step.
>> +	 */
>> +	union {
>> +		__u32 fd;
>> +		__u32 handle;
>> +	};
>> +	/*
>> +	 * Allocation has a definite problem of 'creep' so give more than
>> +	 * enough extra bits for expansion
>> +	 */
>> +	__u32 reserved0;
>> +	__u32 reserved1;
>> +	__u32 reserved2;
>> +};
>> +
>> +/**
>> + * struct ion_usage_id_map - metadata to create a new usage map
>> + * @usage_id - userspace allocated array of existing usage ids
>> + * @cnt - number of ids to be mapped
>> + * @new_id - will be populated with the new usage id
>> + */
>> +struct ion_usage_id_map {
>> +	/* Array of usage ids provided by user */
>> +	__u64 usage_ids;
>> +	__u32 cnt;
>> +
>> +	/* Returned on success */
>> +	__u32 new_id;
>> +	/* Fields for future growth */
>> +	__u32 reserved0;
>> +	__u32 reserved1;
>> +};
>> +
>> +/**
>> + * struct ion_usage_cnt - meta data for the count of heaps
>> + * @cnt - returned count of number of heaps present
>> + */
>> +struct ion_usage_cnt {
>> +	__u32 cnt; /* cnt returned */
>> +	__u32 reserved;
>> +};
>> +
>> +/**
>> + * struct ion_heap_data - data about a heap
>> + * @name - first 32 characters of the heap name
>> + * @type - heap type
>> + * @usage_id - usage id for the heap
>> + */
>> +struct ion_heap_data {
>> +	char name[32];
>> +	__u32 type;
>> +	__u32 usage_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 */
>> +	__u64 heaps; /* buffer to be populated */
>> +	__u32 reserved0;
>> +	__u32 reserved1;
>> +	__u32 reserved2;
>> +};
>> +
>> +
>>  #define ION_IOC_MAGIC		'I'
>>
>>  /**
>> @@ -216,5 +318,38 @@ struct ion_abi_version {
>>  #define ION_IOC_ABI_VERSION    _IOR(ION_IOC_MAGIC, 8, \
>>  					struct ion_abi_version)
>>
>> +/**
>> + * DOC: ION_IOC_ALLOC2 - allocate memory via new API
>> + *
>> + * Same concept as ION_IOC_ALLOC except with updated ABI. Can return an fd
>> + * directly in addition to a handle
>> + */
>> +#define ION_IOC_ALLOC2           _IOWR(ION_IOC_MAGIC, 9, \
>> +					struct ion_new_alloc_data)
>> +
>> +/**
>> + * DOC: ION_IOC_ID_MAP - remap an array of heap IDs
>> + *
>> + * Takes an ion_usage_id_map structure populated with information about
>> + * fallback information. Returns a new usage id for use in allocation.
>> + */
>> +#define ION_IOC_ID_MAP         _IOWR(ION_IOC_MAGIC, 10, \
>> +					struct ion_usage_id_map)
>> +/**
>> + * DOC: ION_IOC_USAGE_CNT - returns a count of the number of usage ids
>> + *
>> + * Takes an ion_usage_cnt structure and returns the total number of usage
>> + * ids available.
>> + */
>> +#define ION_IOC_USAGE_CNT      _IOR(ION_IOC_MAGIC, 11, \
>> +					struct ion_usage_cnt)
>> +/**
>> + * DOC: ION_IOC_HEAP_QUERY - information about available heaps
>> + *
>> + * Takes an ion_heap_query structure and populates information about
>> + * availble Ion heaps.
>> + */
>> +#define ION_IOC_HEAP_QUERY     _IOWR(ION_IOC_MAGIC, 12, \
>> +					struct ion_heap_query)
>>
>>  #endif /* _UAPI_LINUX_ION_H */
>> --
>> 2.5.5
>>
>

Thanks for the comments above.

> I know that ION doesn't have any kernel documentation or examples in terms of what userspace
> should do, but after this patchset I feel like adding some text describing how the dynamic
> heap mapping might work on an ideal device that you target is useful.
>

Yes, sounds good. I'll send out some documentation on the next iteration.

> Best regards,
> Liviu
>

Thanks,
Laura

Re: [RFC][PATCH 6/6] staging: android: ion: Introduce new ioctls for dynamic heaps

[Brian Starkey]

Hi,

I'm finding "usage_id" a bit confusing - there's not a very clear
distinction between usage_id and heap ID.

For instance, ION_IOC_USAGE_CNT claims to return the number of usage
IDs, but seems to return the number of heaps (i.e. number heap IDs, some
of which might be usage_ids).

Similarly, alloc2 claims to allocate by usage_id, but will just as
happily allocate by heap ID.

Maybe this should just be described as a single heap ID namespace, where
some IDs represent groups of heap IDs.

I've a few inline comments below.

-Brian

On Mon, Jun 06, 2016 at 11:23:33AM -0700, Laura Abbott wrote:
>From: Laura Abbott <labbott@fedoraproject.org>
>
>
>The Ion ABI for heaps is limiting to work with for more complex systems.
>Heaps have to be registered at boot time with known ids available to
>userspace. This becomes a tight ABI which is prone to breakage.
>
>Introduce a new mechanism for registering heap ids dynamically. A base
>set of heap ids are registered at boot time but there is no knowledge
>of fallbacks. Fallback information can be remapped and changed
>dynamically. Information about available heaps can also be queried with
>an ioctl to avoid the need to have heap ids be an ABI with userspace.
>
>Signed-off-by: Laura Abbott <labbott@redhat.com>
>---
> drivers/staging/android/ion/ion-ioctl.c | 109 +++++++++++++++++--
> drivers/staging/android/ion/ion.c       | 184 ++++++++++++++++++++++++++++++--
> drivers/staging/android/ion/ion_priv.h  |  15 +++
> drivers/staging/android/uapi/ion.h      | 135 +++++++++++++++++++++++
> 4 files changed, 426 insertions(+), 17 deletions(-)
>
>diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
>index 45b89e8..169cad8 100644
>--- a/drivers/staging/android/ion/ion-ioctl.c
>+++ b/drivers/staging/android/ion/ion-ioctl.c
>@@ -22,6 +22,49 @@
> #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;
>+	struct ion_new_alloc_data allocation2;
>+	struct ion_usage_id_map id_map;
>+	struct ion_usage_cnt usage_cnt;
>+	struct ion_heap_query query;
>+};
>+
>+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;
>+	case ION_IOC_ALLOC2:
>+		ret = arg->allocation2.reserved0 != 0;
>+		ret |= arg->allocation2.reserved1 != 0;
>+		ret |= arg->allocation2.reserved2 != 0;
>+		break;
>+	case ION_IOC_ID_MAP:
>+		ret = arg->id_map.reserved0 != 0;
>+		ret |= arg->id_map.reserved1 != 0;
>+		break;
>+	case ION_IOC_USAGE_CNT:
>+		ret = arg->usage_cnt.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;
>+	}
>+	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,14 +85,7 @@ 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;
>-		struct ion_abi_version abi_version;
>-	} data;
>+	union ion_ioctl_arg data;
>
> 	dir = ion_ioctl_dir(cmd);
>
>@@ -60,7 +96,12 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> 		if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
> 			return -EFAULT;
>
>+	ret = validate_ioctl_arg(cmd, &data);
>+	if (ret)
>+		return ret;
>+
> 	switch (cmd) {
>+	/* Old ioctl */
> 	case ION_IOC_ALLOC:
> 	{
> 		struct ion_handle *handle;
>@@ -77,6 +118,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> 		cleanup_handle = handle;
> 		break;
> 	}
>+	/* Old ioctl */
> 	case ION_IOC_FREE:
> 	{
> 		struct ion_handle *handle;
>@@ -92,6 +134,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> 		mutex_unlock(&client->lock);
> 		break;
> 	}
>+	/* Old ioctl */
> 	case ION_IOC_SHARE:
> 	case ION_IOC_MAP:
> 	{
>@@ -106,6 +149,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> 			ret = data.fd.fd;
> 		break;
> 	}
>+	/* Old ioctl */
> 	case ION_IOC_IMPORT:
> 	{
> 		struct ion_handle *handle;
>@@ -117,11 +161,13 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> 			data.handle.handle = handle->id;
> 		break;
> 	}
>+	/* Old ioctl */
> 	case ION_IOC_SYNC:
> 	{
> 		ret = ion_sync_for_device(client, data.fd.fd);
> 		break;
> 	}
>+	/* Old ioctl */
> 	case ION_IOC_CUSTOM:
> 	{
> 		if (!dev->custom_ioctl)
>@@ -135,6 +181,53 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> 		data.abi_version.abi_version = ION_ABI_VERSION;
> 		break;
> 	}
>+	case ION_IOC_ALLOC2:
>+	{
>+		struct ion_handle *handle;
>+
>+		handle = ion_alloc2(client, data.allocation2.len,
>+					data.allocation2.align,
>+					data.allocation2.usage_id,
>+					data.allocation2.flags);
>+		if (IS_ERR(handle))
>+			return PTR_ERR(handle);
>+
>+		if (data.allocation2.flags & ION_FLAG_NO_HANDLE) {
>+			data.allocation2.fd = ion_share_dma_buf_fd(client,
>+								handle);
>+			ion_handle_put(handle);
>+			if (data.allocation2.fd < 0)
>+				ret = data.allocation2.fd;
>+		} else {
>+			data.allocation2.handle = handle->id;
>+
>+			cleanup_handle = handle;
>+		}
>+		break;
>+	}
>+	case ION_IOC_ID_MAP:
>+	{
>+		ret = ion_map_usage_ids(client,
>+				(unsigned int __user *)data.id_map.usage_ids,
>+				data.id_map.cnt);
>+		if (ret > 0)
>+			data.id_map.new_id = ret;
>+		break;
>+	}
>+	case ION_IOC_USAGE_CNT:
>+	{
>+		down_read(&client->dev->lock);
>+		data.usage_cnt.cnt = client->dev->heap_cnt;
>+		up_read(&client->dev->lock);
>+		break;
>+	}
>+	case ION_IOC_HEAP_QUERY:
>+	{
>+		ret = ion_query_heaps(client,
>+				(struct ion_heap_data __user *)data.query.heaps,
>+				data.query.cnt);
>+		break;
>+	}
> 	default:
> 		return -ENOTTY;
> 	}
>diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
>index 129707f..c096cb9 100644
>--- a/drivers/staging/android/ion/ion.c
>+++ b/drivers/staging/android/ion/ion.c
>@@ -459,6 +459,48 @@ static struct ion_handle *__ion_alloc(struct ion_client *client, size_t len,
> 	return handle;
> }
>
>+struct ion_handle *ion_alloc2(struct ion_client *client, size_t len,
>+				size_t align, unsigned int usage_id,
>+				unsigned int flags)
>+{
>+	struct ion_device *dev = client->dev;
>+	struct ion_heap *heap;
>+	struct ion_handle *handle = ERR_PTR(-ENODEV);
>+
>+	down_read(&dev->lock);
>+	heap = idr_find(&dev->idr, usage_id);
>+	if (!heap) {
>+		handle = ERR_PTR(-EINVAL);
>+		goto out;
>+	}
>+
>+	if (heap->type == ION_USAGE_ID_MAP) {
>+		int i;
>+
>+		for (i = 0; i < heap->fallback_cnt; i++){
>+			heap = idr_find(&dev->idr, heap->fallbacks[i]);
>+			if (!heap)
>+				continue;
>+
>+			/* Don't recurse for now? */
>+			if (heap->type == ION_USAGE_ID_MAP)
>+				continue;
>+
>+			handle =  __ion_alloc(client, len, align, heap, flags);
>+			if (IS_ERR(handle))
>+				continue;
>+			else
>+				break;
>+		}
>+	} else {
>+		handle = __ion_alloc(client, len, align, heap, flags);
>+	}
>+out:
>+	up_read(&dev->lock);
>+	return handle;
>+}
>+EXPORT_SYMBOL(ion_alloc2);
>+
> struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
> 				size_t align, unsigned int heap_mask,
> 				unsigned int flags)
>@@ -1243,6 +1285,55 @@ int ion_sync_for_device(struct ion_client *client, int fd)
> 	return 0;
> }
>
>+struct ion_query_data {
>+	struct ion_heap_data __user *buffer;
>+	int cnt;
>+	int max_cnt;
>+};
>+
>+int __ion_query(int id, void *p, void *data)
>+{
>+	struct ion_heap *heap = p;
>+	struct ion_query_data *query = data;
>+	struct ion_heap_data hdata = {0};
>+
>+	if (query->cnt >= query->max_cnt)
>+		return -ENOSPC;
>+
>+	strncpy(hdata.name, heap->name, 20);

Why 20?

>+	hdata.name[sizeof(hdata.name) - 1] = '\0';
>+	hdata.type = heap->type;
>+	hdata.usage_id = heap->id;
>+
>+	return copy_to_user(&query->buffer[query->cnt++], &hdata, sizeof(hdata));
>+}
>+
>+int ion_query_heaps(struct ion_client *client,
>+		struct ion_heap_data __user *buffer,
>+		int cnt)
>+{
>+	struct ion_device *dev = client->dev;
>+	struct ion_query_data data;
>+	int ret;
>+
>+	data.buffer = buffer;
>+	data.cnt = 0;
>+	data.max_cnt = cnt;
>+
>+	down_read(&dev->lock);
>+	if (data.max_cnt < 0 || data.max_cnt > dev->heap_cnt) {

Is the check against heap_cnt necessary? You could update the cnt
field in ion_heap_query to say how many were actually copied.

I think you could probably drop ION_IOC_USAGE_COUNT by giving
ION_IOC_HEAP_QUERY similar semantics to the DRM ioctls:
   - First call with buffer pointer = NULL, kernel sets cnt to space
     required
   - Userspace allocates buffer and calls the ioctl again, kernel fills
     the buffer

>+		ret = -EINVAL;
>+		goto out;
>+	}
>+	ret = idr_for_each(&dev->idr, __ion_query, &data);
>+out:
>+	up_read(&dev->lock);
>+
>+	return ret;
>+}
>+
>+
>+
> static int ion_release(struct inode *inode, struct file *file)
> {
> 	struct ion_client *client = file->private_data;
>@@ -1407,13 +1498,69 @@ static int debug_shrink_get(void *data, u64 *val)
> DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
> 			debug_shrink_set, "%llu\n");
>
>+int ion_map_usage_ids(struct ion_client *client,
>+			unsigned int __user *usage_ids,
>+			int cnt)
>+{
>+	struct ion_heap *heap = kzalloc(sizeof(*heap), GFP_KERNEL);
>+	unsigned int *fallbacks;
>+	int i;
>+	int ret;
>+
>+	if (!heap)
>+		return -ENOMEM;
>+
>+	fallbacks = kzalloc(sizeof(unsigned int)*cnt, GFP_KERNEL);
>+	if (!fallbacks) {
>+		ret = -ENOMEM;
>+		goto out1;
>+	}
>+
>+	ret = copy_from_user(fallbacks, usage_ids, sizeof(unsigned int)*cnt);
>+	if (ret)
>+		goto out2;
>+
>+	down_read(&client->dev->lock);
>+	for (i = 0; i < cnt; i++) {
>+		if (idr_find(&client->dev->idr, fallbacks[i]) == NULL) {
>+			ret = -EINVAL;
>+			goto out3;
>+		}
>+	}
>+	up_read(&client->dev->lock);
>+
>+	/*
>+	 * This is a racy check since the lock is dropped before the heap
>+	 * is actually added. It's okay though because ids are never actually
>+	 * deleted. Worst case some user gets an error back and an indication
>+	 * to fix races in their code.
>+	 */
>+
>+	heap->fallbacks = fallbacks;
>+	heap->fallback_cnt = cnt;
>+	heap->type = ION_USAGE_ID_MAP;
>+	heap->id = ION_DYNAMIC_HEAP_ASSIGN;

It would be nice for userspace to be able to give these usage heaps
meaningful names - That could be their intended usage ("camera",
"display") or their properties ("contiguous", "sram").

>+	ion_device_add_heap(client->dev, heap);
>+	return heap->id;
>+out3:
>+	up_read(&client->dev->lock);
>+out2:
>+	kfree(fallbacks);
>+out1:
>+	kfree(heap);
>+	return ret;
>+}
>+
> int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
> {
> 	struct dentry *debug_file;
> 	int ret;
>
>-	if (!heap->ops->allocate || !heap->ops->free || !heap->ops->map_dma ||
>-	    !heap->ops->unmap_dma) {
>+	if (heap->type != ION_USAGE_ID_MAP &&
>+	   (!heap->ops->allocate ||
>+	    !heap->ops->free ||
>+	    !heap->ops->map_dma ||
>+	    !heap->ops->unmap_dma)) {
> 		pr_err("%s: can not add heap with invalid ops struct.\n",
> 		       __func__);
> 		return -EINVAL;
>@@ -1425,18 +1572,36 @@ int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
> 	if (heap->flags & ION_HEAP_FLAG_DEFER_FREE)
> 		ion_heap_init_deferred_free(heap);
>
>-	if ((heap->flags & ION_HEAP_FLAG_DEFER_FREE) || heap->ops->shrink)
>+	if ((heap->flags & ION_HEAP_FLAG_DEFER_FREE) || (heap->ops && heap->ops->shrink))
> 		ion_heap_init_shrinker(heap);
>
> 	heap->dev = dev;
> 	down_write(&dev->lock);
>
>-	ret = idr_alloc(&dev->idr, heap, heap->id, heap->id + 1, GFP_KERNEL);
>-	if (ret < 0 || ret != heap->id) {
>-		pr_info("%s: Failed to add heap id, expected %d got %d\n",
>-			__func__, heap->id, ret);
>-		up_write(&dev->lock);
>-		return ret < 0 ? ret : -EINVAL;
>+	if (heap->id == ION_DYNAMIC_HEAP_ASSIGN) {
>+		ret = idr_alloc(&dev->idr, heap,
>+				ION_DYNAMIC_HEAP_ASSIGN + 1, 0, GFP_KERNEL);
>+		if (ret < 0)
>+			goto out_unlock;
>+
>+		heap->id = ret;
>+	} else {
>+		ret = idr_alloc(&dev->idr, heap, heap->id, heap->id + 1,
>+				GFP_KERNEL);
>+		if (ret < 0 || ret != heap->id) {
>+			pr_info("%s: Failed to add heap id, expected %d got %d\n",
>+				__func__, heap->id, ret);
>+			ret = ret < 0 ? ret : -EINVAL;
>+			goto out_unlock;
>+		}
>+	}
>+
>+	if (!heap->name) {
>+		heap->name = kasprintf(GFP_KERNEL, "heap%d", heap->id);
>+		if (!heap->name) {
>+			ret = -ENOMEM;
>+			goto out_unlock;
>+		}
> 	}
>
> 	debug_file = debugfs_create_file(heap->name, 0664,
>@@ -1467,6 +1632,7 @@ int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
> 		}
> 	}
> 	dev->heap_cnt++;
>+out_unlock:
> 	up_write(&dev->lock);
> 	return 0;
> }
>diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h
>index b09d751..e03e940 100644
>--- a/drivers/staging/android/ion/ion_priv.h
>+++ b/drivers/staging/android/ion/ion_priv.h
>@@ -255,6 +255,9 @@ struct ion_heap {
> 	wait_queue_head_t waitqueue;
> 	struct task_struct *task;
>
>+	unsigned int *fallbacks;
>+	int fallback_cnt;
>+
> 	int (*debug_show)(struct ion_heap *heap, struct seq_file *, void *);
> };
>
>@@ -381,6 +384,10 @@ size_t ion_heap_freelist_shrink(struct ion_heap *heap,
> size_t ion_heap_freelist_size(struct ion_heap *heap);
>
>
>+int ion_map_usage_ids(struct ion_client *client,
>+			unsigned int __user *usage_ids,
>+			int cnt);
>+
> /**
>  * functions for creating and destroying the built in ion heaps.
>  * architectures can add their own custom architecture specific
>@@ -495,4 +502,12 @@ struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
>
> int ion_handle_put(struct ion_handle *handle);
>
>+struct ion_handle *ion_alloc2(struct ion_client *client, size_t len,
>+				size_t align, unsigned int usage_id,
>+				unsigned int flags);
>+
>+int ion_query_heaps(struct ion_client *client,
>+		struct ion_heap_data __user *buffer,
>+		int cnt);
>+
> #endif /* _ION_PRIV_H */
>diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
>index 145005f..d36b4e4 100644
>--- a/drivers/staging/android/uapi/ion.h
>+++ b/drivers/staging/android/uapi/ion.h
>@@ -45,10 +45,17 @@ enum ion_heap_type {
> 			       * must be last so device specific heaps always
> 			       * are at the end of this enum
> 			       */
>+	ION_USAGE_ID_MAP,

A short description for this would be nice

> };
>
> #define ION_NUM_HEAP_IDS		(sizeof(unsigned int) * 8)
>
>+/*
>+ * This isn't completely random. The old Ion heap ID namespace goes up to
>+ * 32 bits so take the first one after that to use as a dynamic setting
>+ */
>+#define ION_DYNAMIC_HEAP_ASSIGN		33
>+

The old comment says 32, but the implementation looks like 16.

Either way, shouldn't that be 32? If the previous max was 31,
32 is next available - but in ion_device_add_heap() you use
ION_DYNAMIC_HEAP_ASSIGN + 1, which makes the first dynamically-assigned
ID 34.

> /**
>  * allocation flags - the lower 16 bits are used by core ion, the upper 16
>  * bits are reserved for use by the heaps themselves.
>@@ -65,6 +72,11 @@ enum ion_heap_type {
> 					 * caches must be managed
> 					 * manually
> 					 */
>+#define ION_FLAG_NO_HANDLE 3		/*
>+					 * Return only an fd associated with
>+					 * this buffer and not a handle
>+					 */
>+
>
> /**
>  * DOC: Ion Userspace API
>@@ -142,6 +154,96 @@ struct ion_abi_version {
> 	__u32 reserved;
> };
>
>+/**
>+ * struct ion_new_alloc_data - metadata to/from usersapce allocation v2
>+ * @len:		size of the allocation
>+ * @align:		required alignment of the allocation
>+ * @usage_id:		mapping to heap id to allocate. Will try fallbacks
>+ *			if specified in the heap mapping
>+ * @flags:		flags passed to heap
>+ * @handle:		pointer that will be populated with a cookie to use to
>+ *			refer to this allocation
>+ * @fd:			optionally, may return just an fd with no handle
>+ *			reference
>+ */
>+struct ion_new_alloc_data {
>+	__u64 len;
>+	__u64 align;
>+	__u32 usage_id;
>+	__u32 flags;
>+	/*
>+	 * I'd like to add a flag to just return the fd instead of just
>+	 * a handle for those who want to skip the next step.
>+	 */
>+	union {
>+		__u32 fd;
>+		__u32 handle;
>+	};
>+	/*
>+	 * Allocation has a definite problem of 'creep' so give more than
>+	 * enough extra bits for expansion
>+	 */
>+	__u32 reserved0;
>+	__u32 reserved1;
>+	__u32 reserved2;
>+};
>+
>+/**
>+ * struct ion_usage_id_map - metadata to create a new usage map
>+ * @usage_id - userspace allocated array of existing usage ids
>+ * @cnt - number of ids to be mapped
>+ * @new_id - will be populated with the new usage id
>+ */
>+struct ion_usage_id_map {
>+	/* Array of usage ids provided by user */
>+	__u64 usage_ids;
>+	__u32 cnt;
>+
>+	/* Returned on success */
>+	__u32 new_id;
>+	/* Fields for future growth */
>+	__u32 reserved0;
>+	__u32 reserved1;
>+};
>+
>+/**
>+ * struct ion_usage_cnt - meta data for the count of heaps
>+ * @cnt - returned count of number of heaps present
>+ */
>+struct ion_usage_cnt {
>+	__u32 cnt; /* cnt returned */
>+	__u32 reserved;
>+};
>+
>+/**
>+ * struct ion_heap_data - data about a heap
>+ * @name - first 32 characters of the heap name
>+ * @type - heap type
>+ * @usage_id - usage id for the heap
>+ */
>+struct ion_heap_data {
>+	char name[32];
>+	__u32 type;
>+	__u32 usage_id;

For heaps with type ION_USAGE_ID_MAP I think userspace would want a way
to turn a usage_id back into a list of fallbacks to figure out that
usage_id == 34 means "only contiguous buffers" for instance.
Otherwise only the process that made the mapping can ever know what its
for.

>+	__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 */
>+	__u64 heaps; /* buffer to be populated */

I guess this field needs explicit alignment to 64 bits

>+	__u32 reserved0;
>+	__u32 reserved1;
>+	__u32 reserved2;
>+};
>+
>+
> #define ION_IOC_MAGIC		'I'
>
> /**
>@@ -216,5 +318,38 @@ struct ion_abi_version {
> #define ION_IOC_ABI_VERSION    _IOR(ION_IOC_MAGIC, 8, \
> 					struct ion_abi_version)
>
>+/**
>+ * DOC: ION_IOC_ALLOC2 - allocate memory via new API
>+ *
>+ * Same concept as ION_IOC_ALLOC except with updated ABI. Can return an fd
>+ * directly in addition to a handle
>+ */
>+#define ION_IOC_ALLOC2           _IOWR(ION_IOC_MAGIC, 9, \
>+					struct ion_new_alloc_data)
>+
>+/**
>+ * DOC: ION_IOC_ID_MAP - remap an array of heap IDs
>+ *
>+ * Takes an ion_usage_id_map structure populated with information about
>+ * fallback information. Returns a new usage id for use in allocation.

Can you mention the implied priority of the fallbacks here?

>+ */
>+#define ION_IOC_ID_MAP         _IOWR(ION_IOC_MAGIC, 10, \
>+					struct ion_usage_id_map)
>+/**
>+ * DOC: ION_IOC_USAGE_CNT - returns a count of the number of usage ids
>+ *
>+ * Takes an ion_usage_cnt structure and returns the total number of usage
>+ * ids available.

Number of usage IDs or number of heaps?

>+ */
>+#define ION_IOC_USAGE_CNT      _IOR(ION_IOC_MAGIC, 11, \
>+					struct ion_usage_cnt)
>+/**
>+ * DOC: ION_IOC_HEAP_QUERY - information about available heaps
>+ *
>+ * Takes an ion_heap_query structure and populates information about
>+ * availble Ion heaps.
>+ */
>+#define ION_IOC_HEAP_QUERY     _IOWR(ION_IOC_MAGIC, 12, \
>+					struct ion_heap_query)
>
> #endif /* _UAPI_LINUX_ION_H */
>-- 
>2.5.5
>

Re: [RFC][PATCH 6/6] staging: android: ion: Introduce new ioctls for dynamic heaps

[Laura Abbott]

On 06/08/2016 08:34 AM, Brian Starkey wrote:
> Hi,
>
> I'm finding "usage_id" a bit confusing - there's not a very clear
> distinction between usage_id and heap ID.
>
> For instance, ION_IOC_USAGE_CNT claims to return the number of usage
> IDs, but seems to return the number of heaps (i.e. number heap IDs, some
> of which might be usage_ids).
>
> Similarly, alloc2 claims to allocate by usage_id, but will just as
> happily allocate by heap ID.
>
> Maybe this should just be described as a single heap ID namespace, where
> some IDs represent groups of heap IDs.
>

For now I'm just going to focus on comments not about the heap ID mapping
because I'm leaning towards dropping the heap ID mapping.

> I've a few inline comments below.
>
> -Brian
>
> On Mon, Jun 06, 2016 at 11:23:33AM -0700, Laura Abbott wrote:
>> From: Laura Abbott <labbott@fedoraproject.org>
>>
>>
>> The Ion ABI for heaps is limiting to work with for more complex systems.
>> Heaps have to be registered at boot time with known ids available to
>> userspace. This becomes a tight ABI which is prone to breakage.
>>
>> Introduce a new mechanism for registering heap ids dynamically. A base
>> set of heap ids are registered at boot time but there is no knowledge
>> of fallbacks. Fallback information can be remapped and changed
>> dynamically. Information about available heaps can also be queried with
>> an ioctl to avoid the need to have heap ids be an ABI with userspace.
>>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>> drivers/staging/android/ion/ion-ioctl.c | 109 +++++++++++++++++--
>> drivers/staging/android/ion/ion.c       | 184 ++++++++++++++++++++++++++++++--
>> drivers/staging/android/ion/ion_priv.h  |  15 +++
>> drivers/staging/android/uapi/ion.h      | 135 +++++++++++++++++++++++
>> 4 files changed, 426 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
>> index 45b89e8..169cad8 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -22,6 +22,49 @@
>> #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;
>> +    struct ion_new_alloc_data allocation2;
>> +    struct ion_usage_id_map id_map;
>> +    struct ion_usage_cnt usage_cnt;
>> +    struct ion_heap_query query;
>> +};
>> +
>> +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;
>> +    case ION_IOC_ALLOC2:
>> +        ret = arg->allocation2.reserved0 != 0;
>> +        ret |= arg->allocation2.reserved1 != 0;
>> +        ret |= arg->allocation2.reserved2 != 0;
>> +        break;
>> +    case ION_IOC_ID_MAP:
>> +        ret = arg->id_map.reserved0 != 0;
>> +        ret |= arg->id_map.reserved1 != 0;
>> +        break;
>> +    case ION_IOC_USAGE_CNT:
>> +        ret = arg->usage_cnt.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;
>> +    }
>> +    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,14 +85,7 @@ 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;
>> -        struct ion_abi_version abi_version;
>> -    } data;
>> +    union ion_ioctl_arg data;
>>
>>     dir = ion_ioctl_dir(cmd);
>>
>> @@ -60,7 +96,12 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>         if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
>>             return -EFAULT;
>>
>> +    ret = validate_ioctl_arg(cmd, &data);
>> +    if (ret)
>> +        return ret;
>> +
>>     switch (cmd) {
>> +    /* Old ioctl */
>>     case ION_IOC_ALLOC:
>>     {
>>         struct ion_handle *handle;
>> @@ -77,6 +118,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>         cleanup_handle = handle;
>>         break;
>>     }
>> +    /* Old ioctl */
>>     case ION_IOC_FREE:
>>     {
>>         struct ion_handle *handle;
>> @@ -92,6 +134,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>         mutex_unlock(&client->lock);
>>         break;
>>     }
>> +    /* Old ioctl */
>>     case ION_IOC_SHARE:
>>     case ION_IOC_MAP:
>>     {
>> @@ -106,6 +149,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>             ret = data.fd.fd;
>>         break;
>>     }
>> +    /* Old ioctl */
>>     case ION_IOC_IMPORT:
>>     {
>>         struct ion_handle *handle;
>> @@ -117,11 +161,13 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>             data.handle.handle = handle->id;
>>         break;
>>     }
>> +    /* Old ioctl */
>>     case ION_IOC_SYNC:
>>     {
>>         ret = ion_sync_for_device(client, data.fd.fd);
>>         break;
>>     }
>> +    /* Old ioctl */
>>     case ION_IOC_CUSTOM:
>>     {
>>         if (!dev->custom_ioctl)
>> @@ -135,6 +181,53 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>>         data.abi_version.abi_version = ION_ABI_VERSION;
>>         break;
>>     }
>> +    case ION_IOC_ALLOC2:
>> +    {
>> +        struct ion_handle *handle;
>> +
>> +        handle = ion_alloc2(client, data.allocation2.len,
>> +                    data.allocation2.align,
>> +                    data.allocation2.usage_id,
>> +                    data.allocation2.flags);
>> +        if (IS_ERR(handle))
>> +            return PTR_ERR(handle);
>> +
>> +        if (data.allocation2.flags & ION_FLAG_NO_HANDLE) {
>> +            data.allocation2.fd = ion_share_dma_buf_fd(client,
>> +                                handle);
>> +            ion_handle_put(handle);
>> +            if (data.allocation2.fd < 0)
>> +                ret = data.allocation2.fd;
>> +        } else {
>> +            data.allocation2.handle = handle->id;
>> +
>> +            cleanup_handle = handle;
>> +        }
>> +        break;
>> +    }
>> +    case ION_IOC_ID_MAP:
>> +    {
>> +        ret = ion_map_usage_ids(client,
>> +                (unsigned int __user *)data.id_map.usage_ids,
>> +                data.id_map.cnt);
>> +        if (ret > 0)
>> +            data.id_map.new_id = ret;
>> +        break;
>> +    }
>> +    case ION_IOC_USAGE_CNT:
>> +    {
>> +        down_read(&client->dev->lock);
>> +        data.usage_cnt.cnt = client->dev->heap_cnt;
>> +        up_read(&client->dev->lock);
>> +        break;
>> +    }
>> +    case ION_IOC_HEAP_QUERY:
>> +    {
>> +        ret = ion_query_heaps(client,
>> +                (struct ion_heap_data __user *)data.query.heaps,
>> +                data.query.cnt);
>> +        break;
>> +    }
>>     default:
>>         return -ENOTTY;
>>     }
>> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
>> index 129707f..c096cb9 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -459,6 +459,48 @@ static struct ion_handle *__ion_alloc(struct ion_client *client, size_t len,
>>     return handle;
>> }
>>
>> +struct ion_handle *ion_alloc2(struct ion_client *client, size_t len,
>> +                size_t align, unsigned int usage_id,
>> +                unsigned int flags)
>> +{
>> +    struct ion_device *dev = client->dev;
>> +    struct ion_heap *heap;
>> +    struct ion_handle *handle = ERR_PTR(-ENODEV);
>> +
>> +    down_read(&dev->lock);
>> +    heap = idr_find(&dev->idr, usage_id);
>> +    if (!heap) {
>> +        handle = ERR_PTR(-EINVAL);
>> +        goto out;
>> +    }
>> +
>> +    if (heap->type == ION_USAGE_ID_MAP) {
>> +        int i;
>> +
>> +        for (i = 0; i < heap->fallback_cnt; i++){
>> +            heap = idr_find(&dev->idr, heap->fallbacks[i]);
>> +            if (!heap)
>> +                continue;
>> +
>> +            /* Don't recurse for now? */
>> +            if (heap->type == ION_USAGE_ID_MAP)
>> +                continue;
>> +
>> +            handle =  __ion_alloc(client, len, align, heap, flags);
>> +            if (IS_ERR(handle))
>> +                continue;
>> +            else
>> +                break;
>> +        }
>> +    } else {
>> +        handle = __ion_alloc(client, len, align, heap, flags);
>> +    }
>> +out:
>> +    up_read(&dev->lock);
>> +    return handle;
>> +}
>> +EXPORT_SYMBOL(ion_alloc2);
>> +
>> struct ion_handle *ion_alloc(struct ion_client *client, size_t len,
>>                 size_t align, unsigned int heap_mask,
>>                 unsigned int flags)
>> @@ -1243,6 +1285,55 @@ int ion_sync_for_device(struct ion_client *client, int fd)
>>     return 0;
>> }
>>
>> +struct ion_query_data {
>> +    struct ion_heap_data __user *buffer;
>> +    int cnt;
>> +    int max_cnt;
>> +};
>> +
>> +int __ion_query(int id, void *p, void *data)
>> +{
>> +    struct ion_heap *heap = p;
>> +    struct ion_query_data *query = data;
>> +    struct ion_heap_data hdata = {0};
>> +
>> +    if (query->cnt >= query->max_cnt)
>> +        return -ENOSPC;
>> +
>> +    strncpy(hdata.name, heap->name, 20);
>
> Why 20?
>

Arbitrary limit for a fixed size buffer. As noted below I need
to make this a consistent #define.

>> +    hdata.name[sizeof(hdata.name) - 1] = '\0';
>> +    hdata.type = heap->type;
>> +    hdata.usage_id = heap->id;
>> +
>> +    return copy_to_user(&query->buffer[query->cnt++], &hdata, sizeof(hdata));
>> +}
>> +
>> +int ion_query_heaps(struct ion_client *client,
>> +        struct ion_heap_data __user *buffer,
>> +        int cnt)
>> +{
>> +    struct ion_device *dev = client->dev;
>> +    struct ion_query_data data;
>> +    int ret;
>> +
>> +    data.buffer = buffer;
>> +    data.cnt = 0;
>> +    data.max_cnt = cnt;
>> +
>> +    down_read(&dev->lock);
>> +    if (data.max_cnt < 0 || data.max_cnt > dev->heap_cnt) {
>
> Is the check against heap_cnt necessary? You could update the cnt
> field in ion_heap_query to say how many were actually copied.
>

This was mostly a paranoid check to reduce risk of accidentally
giving userspace access to something it shouldn't. Updating the
count is still a good idea in case userspace wants fewer than
are available.

> I think you could probably drop ION_IOC_USAGE_COUNT by giving
> ION_IOC_HEAP_QUERY similar semantics to the DRM ioctls:
>   - First call with buffer pointer = NULL, kernel sets cnt to space
>     required
>   - Userspace allocates buffer and calls the ioctl again, kernel fills
>     the buffer
>

I'm not that familiar with the DRM ioctls but this sounds like a
good suggestion. One less ioctl to worry about.

>> +        ret = -EINVAL;
>> +        goto out;
>> +    }
>> +    ret = idr_for_each(&dev->idr, __ion_query, &data);
>> +out:
>> +    up_read(&dev->lock);
>> +
>> +    return ret;
>> +}
>> +
>> +
>> +
>> static int ion_release(struct inode *inode, struct file *file)
>> {
>>     struct ion_client *client = file->private_data;
>> @@ -1407,13 +1498,69 @@ static int debug_shrink_get(void *data, u64 *val)
>> DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get,
>>             debug_shrink_set, "%llu\n");
>>
>> +int ion_map_usage_ids(struct ion_client *client,
>> +            unsigned int __user *usage_ids,
>> +            int cnt)
>> +{
>> +    struct ion_heap *heap = kzalloc(sizeof(*heap), GFP_KERNEL);
>> +    unsigned int *fallbacks;
>> +    int i;
>> +    int ret;
>> +
>> +    if (!heap)
>> +        return -ENOMEM;
>> +
>> +    fallbacks = kzalloc(sizeof(unsigned int)*cnt, GFP_KERNEL);
>> +    if (!fallbacks) {
>> +        ret = -ENOMEM;
>> +        goto out1;
>> +    }
>> +
>> +    ret = copy_from_user(fallbacks, usage_ids, sizeof(unsigned int)*cnt);
>> +    if (ret)
>> +        goto out2;
>> +
>> +    down_read(&client->dev->lock);
>> +    for (i = 0; i < cnt; i++) {
>> +        if (idr_find(&client->dev->idr, fallbacks[i]) == NULL) {
>> +            ret = -EINVAL;
>> +            goto out3;
>> +        }
>> +    }
>> +    up_read(&client->dev->lock);
>> +
>> +    /*
>> +     * This is a racy check since the lock is dropped before the heap
>> +     * is actually added. It's okay though because ids are never actually
>> +     * deleted. Worst case some user gets an error back and an indication
>> +     * to fix races in their code.
>> +     */
>> +
>> +    heap->fallbacks = fallbacks;
>> +    heap->fallback_cnt = cnt;
>> +    heap->type = ION_USAGE_ID_MAP;
>> +    heap->id = ION_DYNAMIC_HEAP_ASSIGN;
>
> It would be nice for userspace to be able to give these usage heaps
> meaningful names - That could be their intended usage ("camera",
> "display") or their properties ("contiguous", "sram").
>
>> +    ion_device_add_heap(client->dev, heap);
>> +    return heap->id;
>> +out3:
>> +    up_read(&client->dev->lock);
>> +out2:
>> +    kfree(fallbacks);
>> +out1:
>> +    kfree(heap);
>> +    return ret;
>> +}
>> +
>> int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
>> {
>>     struct dentry *debug_file;
>>     int ret;
>>
>> -    if (!heap->ops->allocate || !heap->ops->free || !heap->ops->map_dma ||
>> -        !heap->ops->unmap_dma) {
>> +    if (heap->type != ION_USAGE_ID_MAP &&
>> +       (!heap->ops->allocate ||
>> +        !heap->ops->free ||
>> +        !heap->ops->map_dma ||
>> +        !heap->ops->unmap_dma)) {
>>         pr_err("%s: can not add heap with invalid ops struct.\n",
>>                __func__);
>>         return -EINVAL;
>> @@ -1425,18 +1572,36 @@ int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
>>     if (heap->flags & ION_HEAP_FLAG_DEFER_FREE)
>>         ion_heap_init_deferred_free(heap);
>>
>> -    if ((heap->flags & ION_HEAP_FLAG_DEFER_FREE) || heap->ops->shrink)
>> +    if ((heap->flags & ION_HEAP_FLAG_DEFER_FREE) || (heap->ops && heap->ops->shrink))
>>         ion_heap_init_shrinker(heap);
>>
>>     heap->dev = dev;
>>     down_write(&dev->lock);
>>
>> -    ret = idr_alloc(&dev->idr, heap, heap->id, heap->id + 1, GFP_KERNEL);
>> -    if (ret < 0 || ret != heap->id) {
>> -        pr_info("%s: Failed to add heap id, expected %d got %d\n",
>> -            __func__, heap->id, ret);
>> -        up_write(&dev->lock);
>> -        return ret < 0 ? ret : -EINVAL;
>> +    if (heap->id == ION_DYNAMIC_HEAP_ASSIGN) {
>> +        ret = idr_alloc(&dev->idr, heap,
>> +                ION_DYNAMIC_HEAP_ASSIGN + 1, 0, GFP_KERNEL);
>> +        if (ret < 0)
>> +            goto out_unlock;
>> +
>> +        heap->id = ret;
>> +    } else {
>> +        ret = idr_alloc(&dev->idr, heap, heap->id, heap->id + 1,
>> +                GFP_KERNEL);
>> +        if (ret < 0 || ret != heap->id) {
>> +            pr_info("%s: Failed to add heap id, expected %d got %d\n",
>> +                __func__, heap->id, ret);
>> +            ret = ret < 0 ? ret : -EINVAL;
>> +            goto out_unlock;
>> +        }
>> +    }
>> +
>> +    if (!heap->name) {
>> +        heap->name = kasprintf(GFP_KERNEL, "heap%d", heap->id);
>> +        if (!heap->name) {
>> +            ret = -ENOMEM;
>> +            goto out_unlock;
>> +        }
>>     }
>>
>>     debug_file = debugfs_create_file(heap->name, 0664,
>> @@ -1467,6 +1632,7 @@ int ion_device_add_heap(struct ion_device *dev, struct ion_heap *heap)
>>         }
>>     }
>>     dev->heap_cnt++;
>> +out_unlock:
>>     up_write(&dev->lock);
>>     return 0;
>> }
>> diff --git a/drivers/staging/android/ion/ion_priv.h b/drivers/staging/android/ion/ion_priv.h
>> index b09d751..e03e940 100644
>> --- a/drivers/staging/android/ion/ion_priv.h
>> +++ b/drivers/staging/android/ion/ion_priv.h
>> @@ -255,6 +255,9 @@ struct ion_heap {
>>     wait_queue_head_t waitqueue;
>>     struct task_struct *task;
>>
>> +    unsigned int *fallbacks;
>> +    int fallback_cnt;
>> +
>>     int (*debug_show)(struct ion_heap *heap, struct seq_file *, void *);
>> };
>>
>> @@ -381,6 +384,10 @@ size_t ion_heap_freelist_shrink(struct ion_heap *heap,
>> size_t ion_heap_freelist_size(struct ion_heap *heap);
>>
>>
>> +int ion_map_usage_ids(struct ion_client *client,
>> +            unsigned int __user *usage_ids,
>> +            int cnt);
>> +
>> /**
>>  * functions for creating and destroying the built in ion heaps.
>>  * architectures can add their own custom architecture specific
>> @@ -495,4 +502,12 @@ struct ion_handle *ion_handle_get_by_id(struct ion_client *client,
>>
>> int ion_handle_put(struct ion_handle *handle);
>>
>> +struct ion_handle *ion_alloc2(struct ion_client *client, size_t len,
>> +                size_t align, unsigned int usage_id,
>> +                unsigned int flags);
>> +
>> +int ion_query_heaps(struct ion_client *client,
>> +        struct ion_heap_data __user *buffer,
>> +        int cnt);
>> +
>> #endif /* _ION_PRIV_H */
>> diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
>> index 145005f..d36b4e4 100644
>> --- a/drivers/staging/android/uapi/ion.h
>> +++ b/drivers/staging/android/uapi/ion.h
>> @@ -45,10 +45,17 @@ enum ion_heap_type {
>>                    * must be last so device specific heaps always
>>                    * are at the end of this enum
>>                    */
>> +    ION_USAGE_ID_MAP,
>
> A short description for this would be nice
>
>> };
>>
>> #define ION_NUM_HEAP_IDS        (sizeof(unsigned int) * 8)
>>
>> +/*
>> + * This isn't completely random. The old Ion heap ID namespace goes up to
>> + * 32 bits so take the first one after that to use as a dynamic setting
>> + */
>> +#define ION_DYNAMIC_HEAP_ASSIGN        33
>> +
>
> The old comment says 32, but the implementation looks like 16.
>
> Either way, shouldn't that be 32? If the previous max was 31,
> 32 is next available - but in ion_device_add_heap() you use
> ION_DYNAMIC_HEAP_ASSIGN + 1, which makes the first dynamically-assigned
> ID 34.
>
>> /**
>>  * allocation flags - the lower 16 bits are used by core ion, the upper 16
>>  * bits are reserved for use by the heaps themselves.
>> @@ -65,6 +72,11 @@ enum ion_heap_type {
>>                      * caches must be managed
>>                      * manually
>>                      */
>> +#define ION_FLAG_NO_HANDLE 3        /*
>> +                     * Return only an fd associated with
>> +                     * this buffer and not a handle
>> +                     */
>> +
>>
>> /**
>>  * DOC: Ion Userspace API
>> @@ -142,6 +154,96 @@ struct ion_abi_version {
>>     __u32 reserved;
>> };
>>
>> +/**
>> + * struct ion_new_alloc_data - metadata to/from usersapce allocation v2
>> + * @len:        size of the allocation
>> + * @align:        required alignment of the allocation
>> + * @usage_id:        mapping to heap id to allocate. Will try fallbacks
>> + *            if specified in the heap mapping
>> + * @flags:        flags passed to heap
>> + * @handle:        pointer that will be populated with a cookie to use to
>> + *            refer to this allocation
>> + * @fd:            optionally, may return just an fd with no handle
>> + *            reference
>> + */
>> +struct ion_new_alloc_data {
>> +    __u64 len;
>> +    __u64 align;
>> +    __u32 usage_id;
>> +    __u32 flags;
>> +    /*
>> +     * I'd like to add a flag to just return the fd instead of just
>> +     * a handle for those who want to skip the next step.
>> +     */
>> +    union {
>> +        __u32 fd;
>> +        __u32 handle;
>> +    };
>> +    /*
>> +     * Allocation has a definite problem of 'creep' so give more than
>> +     * enough extra bits for expansion
>> +     */
>> +    __u32 reserved0;
>> +    __u32 reserved1;
>> +    __u32 reserved2;
>> +};
>> +
>> +/**
>> + * struct ion_usage_id_map - metadata to create a new usage map
>> + * @usage_id - userspace allocated array of existing usage ids
>> + * @cnt - number of ids to be mapped
>> + * @new_id - will be populated with the new usage id
>> + */
>> +struct ion_usage_id_map {
>> +    /* Array of usage ids provided by user */
>> +    __u64 usage_ids;
>> +    __u32 cnt;
>> +
>> +    /* Returned on success */
>> +    __u32 new_id;
>> +    /* Fields for future growth */
>> +    __u32 reserved0;
>> +    __u32 reserved1;
>> +};
>> +
>> +/**
>> + * struct ion_usage_cnt - meta data for the count of heaps
>> + * @cnt - returned count of number of heaps present
>> + */
>> +struct ion_usage_cnt {
>> +    __u32 cnt; /* cnt returned */
>> +    __u32 reserved;
>> +};
>> +
>> +/**
>> + * struct ion_heap_data - data about a heap
>> + * @name - first 32 characters of the heap name
>> + * @type - heap type
>> + * @usage_id - usage id for the heap
>> + */
>> +struct ion_heap_data {
>> +    char name[32];

I realized I upped this to 32 and didn't update the code. I'm going to
make this consistent.

>> +    __u32 type;
>> +    __u32 usage_id;
>
> For heaps with type ION_USAGE_ID_MAP I think userspace would want a way
> to turn a usage_id back into a list of fallbacks to figure out that
> usage_id == 34 means "only contiguous buffers" for instance.
> Otherwise only the process that made the mapping can ever know what its
> for.
>
>> +    __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 */
>> +    __u64 heaps; /* buffer to be populated */
>
> I guess this field needs explicit alignment to 64 bits
>

Yes, I was going by the suggestion in botching-up-ioctls.txt

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt#n42

>> +    __u32 reserved0;
>> +    __u32 reserved1;
>> +    __u32 reserved2;
>> +};
>> +
>> +
>> #define ION_IOC_MAGIC        'I'
>>
>> /**
>> @@ -216,5 +318,38 @@ struct ion_abi_version {
>> #define ION_IOC_ABI_VERSION    _IOR(ION_IOC_MAGIC, 8, \
>>                     struct ion_abi_version)
>>
>> +/**
>> + * DOC: ION_IOC_ALLOC2 - allocate memory via new API
>> + *
>> + * Same concept as ION_IOC_ALLOC except with updated ABI. Can return an fd
>> + * directly in addition to a handle
>> + */
>> +#define ION_IOC_ALLOC2           _IOWR(ION_IOC_MAGIC, 9, \
>> +                    struct ion_new_alloc_data)
>> +
>> +/**
>> + * DOC: ION_IOC_ID_MAP - remap an array of heap IDs
>> + *
>> + * Takes an ion_usage_id_map structure populated with information about
>> + * fallback information. Returns a new usage id for use in allocation.
>
> Can you mention the implied priority of the fallbacks here?
>
>> + */
>> +#define ION_IOC_ID_MAP         _IOWR(ION_IOC_MAGIC, 10, \
>> +                    struct ion_usage_id_map)
>> +/**
>> + * DOC: ION_IOC_USAGE_CNT - returns a count of the number of usage ids
>> + *
>> + * Takes an ion_usage_cnt structure and returns the total number of usage
>> + * ids available.
>
> Number of usage IDs or number of heaps?
>
>> + */
>> +#define ION_IOC_USAGE_CNT      _IOR(ION_IOC_MAGIC, 11, \
>> +                    struct ion_usage_cnt)
>> +/**
>> + * DOC: ION_IOC_HEAP_QUERY - information about available heaps
>> + *
>> + * Takes an ion_heap_query structure and populates information about
>> + * availble Ion heaps.
>> + */
>> +#define ION_IOC_HEAP_QUERY     _IOWR(ION_IOC_MAGIC, 12, \
>> +                    struct ion_heap_query)
>>
>> #endif /* _UAPI_LINUX_ION_H */
>> --
>> 2.5.5
>>

Re: [RFC][PATCH 6/6] staging: android: ion: Introduce new ioctls for dynamic heaps

[Brian Starkey]

Hi Laura,

On Wed, Jun 08, 2016 at 12:14:12PM -0700, Laura Abbott wrote:
>
>For now I'm just going to focus on comments not about the heap ID mapping
>because I'm leaning towards dropping the heap ID mapping.
>

Fair enough. Like you said, giving userspace enough information to just
figure out the right heap IDs is already a big improvement.

>
>I'm not that familiar with the DRM ioctls but this sounds like a
>good suggestion. One less ioctl to worry about.
>

Sorry, I probably should have pointed to an example:
http://lxr.free-electrons.com/source/drivers/gpu/drm/drm_crtc.c#L2325

>>>+ */
>>>+struct ion_heap_query {
>>>+    __u32 cnt; /* Total number of heaps to be copied */
>>>+    __u64 heaps; /* buffer to be populated */
>>
>>I guess this field needs explicit alignment to 64 bits
>>
>
>Yes, I was going by the suggestion in botching-up-ioctls.txt
>
>https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt#n42
>

The type is fine, but you should add a 32-bit padding field before
heaps:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ioctl/botching-up-ioctls.txt#n32

Cheers,

-Brian

Re: [Linaro-mm-sig] [RFC][PATCH 0/6] ion: improved ABI

[Chen Feng]

The idea is good, define the heap ids in header file is inconvenient.

But if we query the heaps information from user-space.

It need to maintain this ids and name userspace one by one. The code may
be complicated in different module user-space.

In android, the gralloc and other lib will all use ion to alloc memory.

This will make it more difficult to maintain user-space code.


But beyond this, The new alloc2 with not-handle flag is good.
And the pull out of ioctl interface is also a good cleanup.

On 2016/6/7 2:23, Laura Abbott wrote:
> 
> The ABI for Ion's ioctl interface are a pain to work with. The heap IDs
> are a 32-bit non-discoverable namespace that form part of the ABI. There's
> no way to determine what ABI version is in use which leads to problems
> if the ABI changes or needs to be updated.
> 
> This series is a first approach to give a better ABI for Ion. This includes:
> 
> - Following the advice in botching-up-ioctls.txt
> - Ioctl for ABI version
> - Dynamic assignment of heap ids
> - queryable heap ids
> - Runtime mapping of heap ids, including fallbacks. This avoids the need to
>   encode the fallbacks as an ABI.
> 
> I'm most interested in feedback if this ABI is actually an improvement and
> usable. The heap id map/query interface seems error prone but I didn't have
> a cleaner solution. There aren't any kernel APIs for the new features as the
> focus was on a userspace API but I anticipate that following easily once
> the userspace API is established.
> 
> 
> Thanks,
> Laura
> 
> P.S. Not to turn this into a bike shedding session but if you have suggestions
> for a name for this framework other than Ion I would be interested to hear
> them. Too many other things are already named Ion.
> 
> Laura Abbott (6):
>   staging: android: ion: return error value for ion_device_add_heap
>   staging: android: ion: Switch to using an idr to manage heaps
>   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: Introduce new ioctls for dynamic heaps
> 
>  drivers/staging/android/ion/Makefile    |   3 +-
>  drivers/staging/android/ion/ion-ioctl.c | 243 ++++++++++++++++++
>  drivers/staging/android/ion/ion.c       | 438 ++++++++++++++++----------------
>  drivers/staging/android/ion/ion_priv.h  | 109 +++++++-
>  drivers/staging/android/uapi/ion.h      | 164 +++++++++++-
>  5 files changed, 728 insertions(+), 229 deletions(-)
>  create mode 100644 drivers/staging/android/ion/ion-ioctl.c
> 

Re: [Linaro-mm-sig] [RFC][PATCH 0/6] ion: improved ABI

[Laura Abbott]

On 06/06/2016 11:59 PM, Chen Feng wrote:
> The idea is good, define the heap ids in header file is inconvenient.
>
> But if we query the heaps information from user-space.
>
> It need to maintain this ids and name userspace one by one. The code may
> be complicated in different module user-space.
>
> In android, the gralloc and other lib will all use ion to alloc memory.
>
> This will make it more difficult to maintain user-space code.
>

This was a concern I had had as well. Anything that involves dynamic
probing is going to involve more tracking. Do you think some library
code in libion would help?

Thanks,
Laura

>
> But beyond this, The new alloc2 with not-handle flag is good.
> And the pull out of ioctl interface is also a good cleanup.
>
> On 2016/6/7 2:23, Laura Abbott wrote:
>>
>> The ABI for Ion's ioctl interface are a pain to work with. The heap IDs
>> are a 32-bit non-discoverable namespace that form part of the ABI. There's
>> no way to determine what ABI version is in use which leads to problems
>> if the ABI changes or needs to be updated.
>>
>> This series is a first approach to give a better ABI for Ion. This includes:
>>
>> - Following the advice in botching-up-ioctls.txt
>> - Ioctl for ABI version
>> - Dynamic assignment of heap ids
>> - queryable heap ids
>> - Runtime mapping of heap ids, including fallbacks. This avoids the need to
>>   encode the fallbacks as an ABI.
>>
>> I'm most interested in feedback if this ABI is actually an improvement and
>> usable. The heap id map/query interface seems error prone but I didn't have
>> a cleaner solution. There aren't any kernel APIs for the new features as the
>> focus was on a userspace API but I anticipate that following easily once
>> the userspace API is established.
>>
>>
>> Thanks,
>> Laura
>>
>> P.S. Not to turn this into a bike shedding session but if you have suggestions
>> for a name for this framework other than Ion I would be interested to hear
>> them. Too many other things are already named Ion.
>>
>> Laura Abbott (6):
>>   staging: android: ion: return error value for ion_device_add_heap
>>   staging: android: ion: Switch to using an idr to manage heaps
>>   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: Introduce new ioctls for dynamic heaps
>>
>>  drivers/staging/android/ion/Makefile    |   3 +-
>>  drivers/staging/android/ion/ion-ioctl.c | 243 ++++++++++++++++++
>>  drivers/staging/android/ion/ion.c       | 438 ++++++++++++++++----------------
>>  drivers/staging/android/ion/ion_priv.h  | 109 +++++++-
>>  drivers/staging/android/uapi/ion.h      | 164 +++++++++++-
>>  5 files changed, 728 insertions(+), 229 deletions(-)
>>  create mode 100644 drivers/staging/android/ion/ion-ioctl.c
>>
>

Re: [RFC][PATCH 0/6] ion: improved ABI

[Brian Starkey]

Hi Laura,

On Mon, Jun 06, 2016 at 11:23:27AM -0700, Laura Abbott wrote:
>The ABI for Ion's ioctl interface are a pain to work with. The heap IDs
>are a 32-bit non-discoverable namespace that form part of the ABI. There's
>no way to determine what ABI version is in use which leads to problems
>if the ABI changes or needs to be updated.
>
>This series is a first approach to give a better ABI for Ion. This includes:
>
>- Following the advice in botching-up-ioctls.txt
>- Ioctl for ABI version
>- Dynamic assignment of heap ids
>- queryable heap ids
>- Runtime mapping of heap ids, including fallbacks. This avoids the need to
>  encode the fallbacks as an ABI.
>
>I'm most interested in feedback if this ABI is actually an improvement and
>usable. The heap id map/query interface seems error prone but I didn't have
>a cleaner solution. There aren't any kernel APIs for the new features as the
>focus was on a userspace API but I anticipate that following easily once
>the userspace API is established.
>

If I understand it right, the big improvement here is that userspace can
find out what heap IDs are available, and what type of heap they
correspond to? This seems good.

I'm not sure about how userspace is meant to use the usage mappings
though. Who calls ION_IOC_ID_MAP?

(also, map/mapping is pretty overloaded. How about groups/groupings?)

If I assume that the thing calling ION_IOC_ID_MAP is the same thing
doing the allocating, then there doesn't seem to be much need for
creating mappings. The combined mapper/allocator would necessarily need
some knowledge about which types can satisfy which usage, and so could
follow something like this:
  1. The heaps can be queried, finding their IDs and types
  2. A mask of heap IDs can be created which satisfies a "usage", based
     on the queried types
  3. Allocation operations can then simply use this constructed mask

On the other hand, if I assume that the thing doing the allocating is
different to the thing doing the usage mapping (i.e. the allocator
doesn't need to know about heap _types_, only usages); then I can't see
a way for the allocator to figure out which usage_id it's meant to
allocate with - which puts it right back to the old problem of opaque
heap IDs (-> opaque usage_ids), except it's worse because they can
change dynamically.

Thanks,
Brian

>
>Thanks,
>Laura
>
>P.S. Not to turn this into a bike shedding session but if you have suggestions
>for a name for this framework other than Ion I would be interested to hear
>them. Too many other things are already named Ion.
>
>Laura Abbott (6):
>  staging: android: ion: return error value for ion_device_add_heap
>  staging: android: ion: Switch to using an idr to manage heaps
>  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: Introduce new ioctls for dynamic heaps
>
> drivers/staging/android/ion/Makefile    |   3 +-
> drivers/staging/android/ion/ion-ioctl.c | 243 ++++++++++++++++++
> drivers/staging/android/ion/ion.c       | 438 ++++++++++++++++----------------
> drivers/staging/android/ion/ion_priv.h  | 109 +++++++-
> drivers/staging/android/uapi/ion.h      | 164 +++++++++++-
> 5 files changed, 728 insertions(+), 229 deletions(-)
> create mode 100644 drivers/staging/android/ion/ion-ioctl.c
>
>-- 2.5.5

Re: [RFC][PATCH 0/6] ion: improved ABI

[Laura Abbott]

On 06/08/2016 08:15 AM, Brian Starkey wrote:
> Hi Laura,
>
> On Mon, Jun 06, 2016 at 11:23:27AM -0700, Laura Abbott wrote:
>> The ABI for Ion's ioctl interface are a pain to work with. The heap IDs
>> are a 32-bit non-discoverable namespace that form part of the ABI. There's
>> no way to determine what ABI version is in use which leads to problems
>> if the ABI changes or needs to be updated.
>>
>> This series is a first approach to give a better ABI for Ion. This includes:
>>
>> - Following the advice in botching-up-ioctls.txt
>> - Ioctl for ABI version
>> - Dynamic assignment of heap ids
>> - queryable heap ids
>> - Runtime mapping of heap ids, including fallbacks. This avoids the need to
>>  encode the fallbacks as an ABI.
>>
>> I'm most interested in feedback if this ABI is actually an improvement and
>> usable. The heap id map/query interface seems error prone but I didn't have
>> a cleaner solution. There aren't any kernel APIs for the new features as the
>> focus was on a userspace API but I anticipate that following easily once
>> the userspace API is established.
>>
>
> If I understand it right, the big improvement here is that userspace can
> find out what heap IDs are available, and what type of heap they
> correspond to? This seems good.
>
> I'm not sure about how userspace is meant to use the usage mappings
> though. Who calls ION_IOC_ID_MAP?
>

The thought was daemons (surfaceflinger, mediaserver etc.) would set this
up.

> (also, map/mapping is pretty overloaded. How about groups/groupings?)
>

That's a good suggestion.

> If I assume that the thing calling ION_IOC_ID_MAP is the same thing
> doing the allocating, then there doesn't seem to be much need for
> creating mappings. The combined mapper/allocator would necessarily need
> some knowledge about which types can satisfy which usage, and so could
> follow something like this:
>  1. The heaps can be queried, finding their IDs and types
>  2. A mask of heap IDs can be created which satisfies a "usage", based
>     on the queried types
>  3. Allocation operations can then simply use this constructed mask
>
> On the other hand, if I assume that the thing doing the allocating is
> different to the thing doing the usage mapping (i.e. the allocator
> doesn't need to know about heap _types_, only usages); then I can't see
> a way for the allocator to figure out which usage_id it's meant to
> allocate with - which puts it right back to the old problem of opaque
> heap IDs (-> opaque usage_ids), except it's worse because they can
> change dynamically.
>

I see your point about the mapping. My thought was that whoever was doing
the mapping would have a way to pass information to the allocator or the
allocator could do a query. Relying on the passing of information may
not be plausible and I realize there isn't enough information from a query
to determine what usage id to use.

I like the suggestion of just using the query. One of
the goals I initially had with this series was to get rid of the heap mask.
The 32-bit heap mask became a name space that needed to be controlled
across all targets and the fallbacks were difficult to change. The problem
that actually wants to be solved is giving userspace enough information
to determine what heap masks to allocate from.

Just a query ioctl should be able to give that information as long as the
requirement is that userspace clients match on the heap name + type only
and not rely on the heap ids being constant (I don't think I made that
clear with this version of the query ioctl so I'll make sure to clarify).

The platform registration can adjust the priority order of the heap ids
as necessary. Different names should be able to take care of any changes
to the platform configuration.

I'll think about this more when I work on v2.

Thanks,
Laura


> Thanks,
> Brian
>
>>
>> Thanks,
>> Laura
>>
>> P.S. Not to turn this into a bike shedding session but if you have suggestions
>> for a name for this framework other than Ion I would be interested to hear
>> them. Too many other things are already named Ion.
>>
>> Laura Abbott (6):
>>  staging: android: ion: return error value for ion_device_add_heap
>>  staging: android: ion: Switch to using an idr to manage heaps
>>  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: Introduce new ioctls for dynamic heaps
>>
>> drivers/staging/android/ion/Makefile    |   3 +-
>> drivers/staging/android/ion/ion-ioctl.c | 243 ++++++++++++++++++
>> drivers/staging/android/ion/ion.c       | 438 ++++++++++++++++----------------
>> drivers/staging/android/ion/ion_priv.h  | 109 +++++++-
>> drivers/staging/android/uapi/ion.h      | 164 +++++++++++-
>> 5 files changed, 728 insertions(+), 229 deletions(-)
>> create mode 100644 drivers/staging/android/ion/ion-ioctl.c
>>
>> -- 2.5.5