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

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

2016-06-08 Thread 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 


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

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

2016-06-08 Thread 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 


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;
}
+   /* Old ioctl */
case ION_IOC_SYNC:
{

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

2016-06-08 Thread 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 


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 */
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;

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

2016-06-08 Thread 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 6/6] staging: android: ion: Introduce new ioctls for dynamic heaps

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information