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

[Liviu Dudau]

>  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(>lock);
> - /*
> -  * use negative heap->id to reverse the priority -- when traversing
> -  * the list later attempt higher id numbers first
> -  */
> - plist_node_init(>node, -heap->id);
> - plist_add(>node, >heaps);
> +
> + ret = idr_alloc(>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(>lock);
> + return ret < 0 ? ret : -EINVAL;
> + }
> +
>   debug_file = debugfs_create_file(heap->name, 0664,
>   dev->heaps_debug_root, heap,
>       _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(>lock);
>   return 0;
>  }
> @@ -1689,7 +1712,7 @@ debugfs_done:
>   idev->buffers = RB_ROOT;
>   mutex_init(>buffer_lock);
>   init_rwsem(>lock);
> - plist_head_init(>heaps);
> + idr_init(>idr);
>   idev->clients = RB_ROOT;
>   ion_root_client = >clients;
>   mutex_init(_mutex);
> -- 
> 2.5.5
> 

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

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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 <labb...@fedoraproject.org>
> 
> 
> ion_device_add_heap doesn't return an error value. Change it to return
> information to callers.
> 
> Signed-off-by: Laura Abbott <labb...@redhat.com>

Reviewed-by: Liviu Dudau <liviu.du...@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(>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(>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!  /
  ---
¯\_(ツ)_/¯
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

Reviewed-by: Liviu Dudau <liviu.du...@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 000..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 
> +#include 
> +#include 
> +#include 
> +
> +#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(, (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(>lock);
> + handle = ion_handle_get_by_id_nolock(client, 
> data.handle.handle);
> + if (IS_ERR(handle)) {
> + mutex_unlock(>lock);
> + return PTR_ERR(handle);
> + }
> + ion_free_nolock(client, handle);
> + ion_handle_put_nolock(handle);
> + mutex_unlock(>lock);
> + break;
> + }
> + case ION_IOC_SHARE:
> + case ION_IOC_MAP:
> + {
> + struct

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 
> 
> 
> 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 
> ---
>  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(, (void __user *)arg, _IOC_SIZE(cmd)))
>   return -EFAULT;
>  
> + ret = validate_ioctl_arg(cmd, );
> + 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(>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;
>   }
> 

Re: [RFC][PATCH 1/3] staging: ion: Move away from the DMA APIs for cache flushing

[Liviu Dudau]

On Wed, May 25, 2016 at 12:48:02PM -0700, Laura Abbott wrote:
> 
> Ion is currently using the DMA APIs in non-compliant ways for cache
> maintaince. The issue is Ion needs to do cache operations outside of
> the regular DMA model. The Ion model matches more closely with the
> DRM model which calls cache APIs directly. Add an appropriate
> abstraction layer for Ion to call cache operations outside of the
> DMA API.
> 
> Signed-off-by: Laura Abbott 

Hi Laura,

By no means a full review, just some comments:

> ---
>  drivers/staging/android/ion/Kconfig | 14 -
>  drivers/staging/android/ion/Makefile|  3 +
>  drivers/staging/android/ion/ion-arm.c   | 83 
> +
>  drivers/staging/android/ion/ion-arm64.c | 46 ++
>  drivers/staging/android/ion/ion-x86.c   | 34 ++
>  drivers/staging/android/ion/ion.c   | 59 ++
>  drivers/staging/android/ion/ion_carveout_heap.c |  5 +-
>  drivers/staging/android/ion/ion_chunk_heap.c|  7 +--
>  drivers/staging/android/ion/ion_page_pool.c |  3 +-
>  drivers/staging/android/ion/ion_priv.h  | 14 ++---
>  drivers/staging/android/ion/ion_system_heap.c   |  5 +-
>  11 files changed, 210 insertions(+), 63 deletions(-)
>  create mode 100644 drivers/staging/android/ion/ion-arm.c
>  create mode 100644 drivers/staging/android/ion/ion-arm64.c
>  create mode 100644 drivers/staging/android/ion/ion-x86.c
> 
> diff --git a/drivers/staging/android/ion/Kconfig 
> b/drivers/staging/android/ion/Kconfig
> index 19c1572..c1b1813 100644
> --- a/drivers/staging/android/ion/Kconfig
> +++ b/drivers/staging/android/ion/Kconfig
> @@ -1,6 +1,6 @@
>  menuconfig ION
>   bool "Ion Memory Manager"
> - depends on HAVE_MEMBLOCK && HAS_DMA && MMU
> + depends on HAVE_MEMBLOCK && HAS_DMA && MMU && (X86 || ARM || ARM64)
>   select GENERIC_ALLOCATOR
>   select DMA_SHARED_BUFFER
>   ---help---
> @@ -10,6 +10,18 @@ menuconfig ION
> If you're not using Android its probably safe to
> say N here.
>  
> +config ION_ARM
> + depends on ION && ARM
> + def_bool y
> +
> +config ION_ARM64
> + depends on ION && ARM64
> + def_bool y
> +
> +config ION_X86
> + depends on ION && X86
> + def_bool y

If you are going to make this arch specific, can I suggest that you make 
CONFIG_ION
depend on CONFIG_ARCH_ION (or any name you like) and then in each arch select 
it?
I don't particularly like the enumeration of all ION enabled arches above 
(is the list exhaustive?)

> +
>  config ION_TEST
>   tristate "Ion Test Device"
>   depends on ION
> diff --git a/drivers/staging/android/ion/Makefile 
> b/drivers/staging/android/ion/Makefile
> index 18cc2aa..1b82bb5 100644
> --- a/drivers/staging/android/ion/Makefile
> +++ b/drivers/staging/android/ion/Makefile
> @@ -1,5 +1,8 @@
>  obj-$(CONFIG_ION) += ion.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_X86) += ion-x86.o
> +obj-$(CONFIG_ION_ARM) += ion-arm.o
> +obj-$(CONFIG_ION_ARM64) += ion-arm64.o
>  obj-$(CONFIG_ION_TEST) += ion_test.o
>  ifdef CONFIG_COMPAT
>  obj-$(CONFIG_ION) += compat_ion.o
> diff --git a/drivers/staging/android/ion/ion-arm.c 
> b/drivers/staging/android/ion/ion-arm.c
> new file mode 100644
> index 000..b91287f
> --- /dev/null
> +++ b/drivers/staging/android/ion/ion-arm.c
> @@ -0,0 +1,83 @@
> +/*
> + * Copyright (C) 2016 Laura Abbott 
> + *
> + * 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 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include "ion_priv.h"
> +
> +
> +void ion_clean_page(struct page *page, size_t size)
> +{
> + unsigned long pfn;
> + size_t left = size;
> +
> + pfn = page_to_pfn(page);
> +
> + /*
> +  * A single sg entry may refer to multiple physically contiguous
> +  * pages.  But we still need to process highmem pages individually.
> +  * If highmem is not configured then the bulk of this loop gets
> +  * optimized out.
> +  */
> + do {
> + size_t len = left;
> + void *vaddr;
> +
> + page = pfn_to_page(pfn);
> +
> + if (PageHighMem(page)) {
> + if (len > PAGE_SIZE)
> + len = PAGE_SIZE;
> +
> + if (cache_is_vipt_nonaliasing()) {
> + vaddr = kmap_atomic(page);
> +