[PATCH v2 0/2] staging: ion: get one device per heap

[PATCH v2 0/2] staging: ion: get one device per heap

[Benjamin Gaignard]

version 2:
- simplify ioctl check like propose by Dan
- make sure that we don't register more than ION_DEV_MAX heaps

Instead a getting one common device "/dev/ion" for
all the heaps this patch allow to create one device
entry ("/dev/ionX") per heap.
Getting an entry per heap could allow to set security rules
per heap and global ones for all heaps.

Allocation requests will be only allowed if the mask_id
match with device minor.
Query request could be done on any of the devices.

Benjamin Gaignard (2):
  staging: ion: simplify ioctl args checking function
  staging: ion: create one device entry per heap

 drivers/staging/android/ion/ion-ioctl.c | 20 +++++++++++++-------
 drivers/staging/android/ion/ion.c       | 23 +++++++++++++++++------
 drivers/staging/android/ion/ion.h       | 10 +++++++---
 3 files changed, 37 insertions(+), 16 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] staging: ion: simplify ioctl args checking function

[Benjamin Gaignard]

Make arguments checking more easy to read.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/staging/android/ion/ion-ioctl.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index d9f8b14..e26b786 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -27,19 +27,18 @@ union ion_ioctl_arg {
 
 static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
 {
-	int ret = 0;
-
 	switch (cmd) {
 	case ION_IOC_HEAP_QUERY:
-		ret = arg->query.reserved0 != 0;
-		ret |= arg->query.reserved1 != 0;
-		ret |= arg->query.reserved2 != 0;
+		if (arg->query.reserved0 ||
+		    arg->query.reserved1 ||
+		    arg->query.reserved2 )
+			return -EINVAL;
 		break;
 	default:
 		break;
 	}
 
-	return ret ? -EINVAL : 0;
+	return 0;
 }
 
 /* fix up the cases where the ioctl direction bits are incorrect */
-- 
2.7.4

[PATCH v2 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

Instead a getting one common device "/dev/ion" for
all the heaps this patch allow to create one device
entry ("/dev/ionX") per heap.
Getting an entry per heap could allow to set security rules
per heap and global ones for all heaps.

Allocation requests will be only allowed if the mask_id
match with device minor.
Query request could be done on any of the devices.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++--
 drivers/staging/android/ion/ion.c       | 23 +++++++++++++++++------
 drivers/staging/android/ion/ion.h       | 10 +++++++---
 3 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index e26b786..c8c906c 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -25,8 +25,11 @@ union ion_ioctl_arg {
 	struct ion_heap_query query;
 };
 
-static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
+static int validate_ioctl_arg(struct file *filp,
+			      unsigned int cmd, union ion_ioctl_arg *arg)
 {
+	int mask = 1 << iminor(filp->f_inode);
+
 	switch (cmd) {
 	case ION_IOC_HEAP_QUERY:
 		if (arg->query.reserved0 ||
@@ -34,6 +37,10 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
 		    arg->query.reserved2 )
 			return -EINVAL;
 		break;
+	case ION_IOC_ALLOC:
+		if (!(arg->allocation.heap_id_mask & mask))
+			return -EINVAL;
+		break;
 	default:
 		break;
 	}
@@ -69,7 +76,7 @@ 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);
+	ret = validate_ioctl_arg(filp, cmd, &data);
 	if (WARN_ON_ONCE(ret))
 		return ret;
 
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 93e2c90..3f8b595 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -40,6 +40,8 @@
 
 #include "ion.h"
 
+#define ION_DEV_MAX 32
+
 static struct ion_device *internal_dev;
 static int heap_id;
 
@@ -541,11 +543,24 @@ void ion_device_add_heap(struct ion_heap *heap)
 {
 	struct dentry *debug_file;
 	struct ion_device *dev = internal_dev;
+	int ret;
 
 	if (!heap->ops->allocate || !heap->ops->free)
 		pr_err("%s: can not add heap with invalid ops struct.\n",
 		       __func__);
 
+	if (heap_id >= ION_DEV_MAX)
+		return -EBUSY;
+
+	heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
+	dev_set_name(&heap->ddev, "ion%d", heap_id);
+	device_initialize(&heap->ddev);
+	cdev_init(&heap->chrdev, &ion_fops);
+	heap->chrdev.owner = THIS_MODULE;
+	ret = cdev_device_add(&heap->chrdev, &heap->ddev);
+	if (ret < 0)
+		return;
+
 	spin_lock_init(&heap->free_lock);
 	heap->free_list_size = 0;
 
@@ -595,13 +610,9 @@ static int ion_device_create(void)
 	if (!idev)
 		return -ENOMEM;
 
-	idev->dev.minor = MISC_DYNAMIC_MINOR;
-	idev->dev.name = "ion";
-	idev->dev.fops = &ion_fops;
-	idev->dev.parent = NULL;
-	ret = misc_register(&idev->dev);
+	ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");
 	if (ret) {
-		pr_err("ion: failed to register misc device.\n");
+		pr_err("ion: unable to allocate major\n");
 		kfree(idev);
 		return ret;
 	}
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index 621e5f7..ef51ff5 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -17,6 +17,7 @@
 #ifndef _ION_H
 #define _ION_H
 
+#include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/dma-direction.h>
 #include <linux/kref.h>
@@ -26,7 +27,6 @@
 #include <linux/sched.h>
 #include <linux/shrinker.h>
 #include <linux/types.h>
-#include <linux/miscdevice.h>
 
 #include "../uapi/ion.h"
 
@@ -90,13 +90,13 @@ void ion_buffer_destroy(struct ion_buffer *buffer);
 
 /**
  * struct ion_device - the metadata of the ion device node
- * @dev:		the actual misc device
+ * @dev:		the actual 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
  */
 struct ion_device {
-	struct miscdevice dev;
+	dev_t devt;
 	struct rb_root buffers;
 	struct mutex buffer_lock;
 	struct rw_semaphore lock;
@@ -152,6 +152,8 @@ struct ion_heap_ops {
  * struct ion_heap - represents a heap in the system
  * @node:		rb node to put the heap on the device's tree of heaps
  * @dev:		back pointer to the ion_device
+ * @ddev:		device structure
+ * @chrdev:		associated character device
  * @type:		type of heap
  * @ops:		ops struct as above
  * @flags:		flags
@@ -176,6 +178,8 @@ struct ion_heap_ops {
 struct ion_heap {
 	struct plist_node node;
 	struct ion_device *dev;
+	struct device ddev;
+	struct cdev chrdev;
 	enum ion_heap_type type;
 	struct ion_heap_ops *ops;
 	unsigned long flags;
-- 
2.7.4

Re: [PATCH v2 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Tue, Sep 19, 2017 at 12:25:38PM +0200, Benjamin Gaignard wrote:
> Instead a getting one common device "/dev/ion" for
> all the heaps this patch allow to create one device
> entry ("/dev/ionX") per heap.
> Getting an entry per heap could allow to set security rules
> per heap and global ones for all heaps.
> 
> Allocation requests will be only allowed if the mask_id
> match with device minor.
> Query request could be done on any of the devices.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> ---
>  drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++--
>  drivers/staging/android/ion/ion.c       | 23 +++++++++++++++++------
>  drivers/staging/android/ion/ion.h       | 10 +++++++---
>  3 files changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> index e26b786..c8c906c 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -25,8 +25,11 @@ union ion_ioctl_arg {
>  	struct ion_heap_query query;
>  };
>  
> -static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> +static int validate_ioctl_arg(struct file *filp,
> +			      unsigned int cmd, union ion_ioctl_arg *arg)
>  {
> +	int mask = 1 << iminor(filp->f_inode);
> +
>  	switch (cmd) {
>  	case ION_IOC_HEAP_QUERY:
>  		if (arg->query.reserved0 ||
> @@ -34,6 +37,10 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>  		    arg->query.reserved2 )
>  			return -EINVAL;
>  		break;
> +	case ION_IOC_ALLOC:
> +		if (!(arg->allocation.heap_id_mask & mask))
> +			return -EINVAL;
> +		break;
>  	default:
>  		break;
>  	}
> @@ -69,7 +76,7 @@ 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);
> +	ret = validate_ioctl_arg(filp, cmd, &data);
>  	if (WARN_ON_ONCE(ret))
>  		return ret;
>  
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 93e2c90..3f8b595 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -40,6 +40,8 @@
>  
>  #include "ion.h"
>  
> +#define ION_DEV_MAX 32
> +
>  static struct ion_device *internal_dev;
>  static int heap_id;
>  
> @@ -541,11 +543,24 @@ void ion_device_add_heap(struct ion_heap *heap)
>  {
>  	struct dentry *debug_file;
>  	struct ion_device *dev = internal_dev;
> +	int ret;
>  
>  	if (!heap->ops->allocate || !heap->ops->free)
>  		pr_err("%s: can not add heap with invalid ops struct.\n",
>  		       __func__);
>  
> +	if (heap_id >= ION_DEV_MAX)
> +		return -EBUSY;
> +
> +	heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
> +	dev_set_name(&heap->ddev, "ion%d", heap_id);
> +	device_initialize(&heap->ddev);
> +	cdev_init(&heap->chrdev, &ion_fops);
> +	heap->chrdev.owner = THIS_MODULE;
> +	ret = cdev_device_add(&heap->chrdev, &heap->ddev);
> +	if (ret < 0)
> +		return;

No cleanup needed?  No reporting an error happened back up the chain?
Not nice :(

> +
>  	spin_lock_init(&heap->free_lock);
>  	heap->free_list_size = 0;
>  
> @@ -595,13 +610,9 @@ static int ion_device_create(void)
>  	if (!idev)
>  		return -ENOMEM;
>  
> -	idev->dev.minor = MISC_DYNAMIC_MINOR;
> -	idev->dev.name = "ion";
> -	idev->dev.fops = &ion_fops;
> -	idev->dev.parent = NULL;
> -	ret = misc_register(&idev->dev);
> +	ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");

Did you just change the major number for the device node as well?

Wow, that's a lot of userspace breakage (both major number, and name),
how did you test this?

Have you gotten "upstream" to agree to these changes?  We can't take
these until they think it's ok as well.

thanks,

greg k-h

Re: [PATCH v2 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

2017-09-19 13:02 GMT+02:00 Greg KH <gregkh@linuxfoundation.org>:
> On Tue, Sep 19, 2017 at 12:25:38PM +0200, Benjamin Gaignard wrote:
>> Instead a getting one common device "/dev/ion" for
>> all the heaps this patch allow to create one device
>> entry ("/dev/ionX") per heap.
>> Getting an entry per heap could allow to set security rules
>> per heap and global ones for all heaps.
>>
>> Allocation requests will be only allowed if the mask_id
>> match with device minor.
>> Query request could be done on any of the devices.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> ---
>>  drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++--
>>  drivers/staging/android/ion/ion.c       | 23 +++++++++++++++++------
>>  drivers/staging/android/ion/ion.h       | 10 +++++++---
>>  3 files changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
>> index e26b786..c8c906c 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -25,8 +25,11 @@ union ion_ioctl_arg {
>>       struct ion_heap_query query;
>>  };
>>
>> -static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>> +static int validate_ioctl_arg(struct file *filp,
>> +                           unsigned int cmd, union ion_ioctl_arg *arg)
>>  {
>> +     int mask = 1 << iminor(filp->f_inode);
>> +
>>       switch (cmd) {
>>       case ION_IOC_HEAP_QUERY:
>>               if (arg->query.reserved0 ||
>> @@ -34,6 +37,10 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>>                   arg->query.reserved2 )
>>                       return -EINVAL;
>>               break;
>> +     case ION_IOC_ALLOC:
>> +             if (!(arg->allocation.heap_id_mask & mask))
>> +                     return -EINVAL;
>> +             break;
>>       default:
>>               break;
>>       }
>> @@ -69,7 +76,7 @@ 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);
>> +     ret = validate_ioctl_arg(filp, cmd, &data);
>>       if (WARN_ON_ONCE(ret))
>>               return ret;
>>
>> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
>> index 93e2c90..3f8b595 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -40,6 +40,8 @@
>>
>>  #include "ion.h"
>>
>> +#define ION_DEV_MAX 32
>> +
>>  static struct ion_device *internal_dev;
>>  static int heap_id;
>>
>> @@ -541,11 +543,24 @@ void ion_device_add_heap(struct ion_heap *heap)
>>  {
>>       struct dentry *debug_file;
>>       struct ion_device *dev = internal_dev;
>> +     int ret;
>>
>>       if (!heap->ops->allocate || !heap->ops->free)
>>               pr_err("%s: can not add heap with invalid ops struct.\n",
>>                      __func__);
>>
>> +     if (heap_id >= ION_DEV_MAX)
>> +             return -EBUSY;
>> +
>> +     heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
>> +     dev_set_name(&heap->ddev, "ion%d", heap_id);
>> +     device_initialize(&heap->ddev);
>> +     cdev_init(&heap->chrdev, &ion_fops);
>> +     heap->chrdev.owner = THIS_MODULE;
>> +     ret = cdev_device_add(&heap->chrdev, &heap->ddev);
>> +     if (ret < 0)
>> +             return;
>
> No cleanup needed?  No reporting an error happened back up the chain?
> Not nice :(

I will change that

>> +
>>       spin_lock_init(&heap->free_lock);
>>       heap->free_list_size = 0;
>>
>> @@ -595,13 +610,9 @@ static int ion_device_create(void)
>>       if (!idev)
>>               return -ENOMEM;
>>
>> -     idev->dev.minor = MISC_DYNAMIC_MINOR;
>> -     idev->dev.name = "ion";
>> -     idev->dev.fops = &ion_fops;
>> -     idev->dev.parent = NULL;
>> -     ret = misc_register(&idev->dev);
>> +     ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");
>
> Did you just change the major number for the device node as well?
>
My understanding of alloc_chrdev_region() is that major number is chosen
dynamically but I don't understand the link with device node, sorry.

> Wow, that's a lot of userspace breakage (both major number, and name),
> how did you test this?

I had to write a test by myself:
https://git.linaro.org/people/benjamin.gaignard/ion_test_application.git/log/?h=one_device_per_heap

Laura have tried to push a test VGEM but I believe it hasn't be
accepted yet (I will check)

>
> Have you gotten "upstream" to agree to these changes?  We can't take
> these until they think it's ok as well.

Split /dev/ion into multiple nodes is one of the task listed in
staging/android/TODO
file before been able to de-stage ion.

Since it has been a big bang in ion ABI on 4.12 and the fact  that
ion.h is still in
staging/android/uapi/ directory I do believe that userland is still unstable.
I hope this kind of patch will help to clarify what is still need to
be done to de-stage ion
even if this patch is NACK-ed we can at least the item from the TODO list.

Benjamin

>
> thanks,
>
> greg k-h

Re: [PATCH v2 2/2] staging: ion: create one device entry per heap

[Dan Carpenter]

If you had spelled out how this patch breaks user space in the changelog
then you wouldn't be catching so much flak for it now...  You should
fix that in v3.

regards,
dan carpenter

Re: [PATCH v2 2/2] staging: ion: create one device entry per heap

[Greg KH]

On Tue, Sep 19, 2017 at 01:55:36PM +0200, Benjamin Gaignard wrote:
> >> +
> >>       spin_lock_init(&heap->free_lock);
> >>       heap->free_list_size = 0;
> >>
> >> @@ -595,13 +610,9 @@ static int ion_device_create(void)
> >>       if (!idev)
> >>               return -ENOMEM;
> >>
> >> -     idev->dev.minor = MISC_DYNAMIC_MINOR;
> >> -     idev->dev.name = "ion";
> >> -     idev->dev.fops = &ion_fops;
> >> -     idev->dev.parent = NULL;
> >> -     ret = misc_register(&idev->dev);
> >> +     ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");
> >
> > Did you just change the major number for the device node as well?
> >
> My understanding of alloc_chrdev_region() is that major number is chosen
> dynamically but I don't understand the link with device node, sorry.

Yes, the major is chosen dynamically if you ask for it (like you are
here), but previously you were using the misc major number.  That might
break userspace really badly if it had hard-coded /dev (like lots of
android devices do...)

> > Wow, that's a lot of userspace breakage (both major number, and name),
> > how did you test this?
> 
> I had to write a test by myself:
> https://git.linaro.org/people/benjamin.gaignard/ion_test_application.git/log/?h=one_device_per_heap
> 
> Laura have tried to push a test VGEM but I believe it hasn't be
> accepted yet (I will check)

A "test" program is a bit different than "boots and runs a working
system", right?  Please work with the correct graphics people to ensure
this change works with their code, before resending it.

thanks,

greg k-h

Re: [PATCH v2 2/2] staging: ion: create one device entry per heap

[Laura Abbott]

On 09/19/2017 04:55 AM, Benjamin Gaignard wrote:
> 2017-09-19 13:02 GMT+02:00 Greg KH <gregkh@linuxfoundation.org>:
>> On Tue, Sep 19, 2017 at 12:25:38PM +0200, Benjamin Gaignard wrote:
>>> Instead a getting one common device "/dev/ion" for
>>> all the heaps this patch allow to create one device
>>> entry ("/dev/ionX") per heap.
>>> Getting an entry per heap could allow to set security rules
>>> per heap and global ones for all heaps.
>>>
>>> Allocation requests will be only allowed if the mask_id
>>> match with device minor.
>>> Query request could be done on any of the devices.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>>> ---
>>>   drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++--
>>>   drivers/staging/android/ion/ion.c       | 23 +++++++++++++++++------
>>>   drivers/staging/android/ion/ion.h       | 10 +++++++---
>>>   3 files changed, 33 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
>>> index e26b786..c8c906c 100644
>>> --- a/drivers/staging/android/ion/ion-ioctl.c
>>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>>> @@ -25,8 +25,11 @@ union ion_ioctl_arg {
>>>        struct ion_heap_query query;
>>>   };
>>>
>>> -static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>>> +static int validate_ioctl_arg(struct file *filp,
>>> +                           unsigned int cmd, union ion_ioctl_arg *arg)
>>>   {
>>> +     int mask = 1 << iminor(filp->f_inode);
>>> +
>>>        switch (cmd) {
>>>        case ION_IOC_HEAP_QUERY:
>>>                if (arg->query.reserved0 ||
>>> @@ -34,6 +37,10 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>>>                    arg->query.reserved2 )
>>>                        return -EINVAL;
>>>                break;
>>> +     case ION_IOC_ALLOC:
>>> +             if (!(arg->allocation.heap_id_mask & mask))
>>> +                     return -EINVAL;
>>> +             break;
>>>        default:
>>>                break;
>>>        }
>>> @@ -69,7 +76,7 @@ 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);
>>> +     ret = validate_ioctl_arg(filp, cmd, &data);
>>>        if (WARN_ON_ONCE(ret))
>>>                return ret;
>>>
>>> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
>>> index 93e2c90..3f8b595 100644
>>> --- a/drivers/staging/android/ion/ion.c
>>> +++ b/drivers/staging/android/ion/ion.c
>>> @@ -40,6 +40,8 @@
>>>
>>>   #include "ion.h"
>>>
>>> +#define ION_DEV_MAX 32
>>> +
>>>   static struct ion_device *internal_dev;
>>>   static int heap_id;
>>>
>>> @@ -541,11 +543,24 @@ void ion_device_add_heap(struct ion_heap *heap)
>>>   {
>>>        struct dentry *debug_file;
>>>        struct ion_device *dev = internal_dev;
>>> +     int ret;
>>>
>>>        if (!heap->ops->allocate || !heap->ops->free)
>>>                pr_err("%s: can not add heap with invalid ops struct.\n",
>>>                       __func__);
>>>
>>> +     if (heap_id >= ION_DEV_MAX)
>>> +             return -EBUSY;
>>> +
>>> +     heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
>>> +     dev_set_name(&heap->ddev, "ion%d", heap_id);
>>> +     device_initialize(&heap->ddev);
>>> +     cdev_init(&heap->chrdev, &ion_fops);
>>> +     heap->chrdev.owner = THIS_MODULE;
>>> +     ret = cdev_device_add(&heap->chrdev, &heap->ddev);
>>> +     if (ret < 0)
>>> +             return;
>>
>> No cleanup needed?  No reporting an error happened back up the chain?
>> Not nice :(
> 
> I will change that
> 
>>> +
>>>        spin_lock_init(&heap->free_lock);
>>>        heap->free_list_size = 0;
>>>
>>> @@ -595,13 +610,9 @@ static int ion_device_create(void)
>>>        if (!idev)
>>>                return -ENOMEM;
>>>
>>> -     idev->dev.minor = MISC_DYNAMIC_MINOR;
>>> -     idev->dev.name = "ion";
>>> -     idev->dev.fops = &ion_fops;
>>> -     idev->dev.parent = NULL;
>>> -     ret = misc_register(&idev->dev);
>>> +     ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");
>>
>> Did you just change the major number for the device node as well?
>>
> My understanding of alloc_chrdev_region() is that major number is chosen
> dynamically but I don't understand the link with device node, sorry.
> 
>> Wow, that's a lot of userspace breakage (both major number, and name),
>> how did you test this?
> 
> I had to write a test by myself:
> https://git.linaro.org/people/benjamin.gaignard/ion_test_application.git/log/?h=one_device_per_heap
> 
> Laura have tried to push a test VGEM but I believe it hasn't be
> accepted yet (I will check)
> 
>>
>> Have you gotten "upstream" to agree to these changes?  We can't take
>> these until they think it's ok as well.
> 
> Split /dev/ion into multiple nodes is one of the task listed in
> staging/android/TODO
> file before been able to de-stage ion.
> 
> Since it has been a big bang in ion ABI on 4.12 and the fact  that
> ion.h is still in
> staging/android/uapi/ directory I do believe that userland is still unstable.
> I hope this kind of patch will help to clarify what is still need to
> be done to de-stage ion
> even if this patch is NACK-ed we can at least the item from the TODO list.
> 

Thanks for sending this Benjamin.

At plumbers, it was requested to not break the ABI too much or do it in
one last big bang before moving out of staging.  My thought was to
keep the old /dev/ion the same and allow all allocation and access via
a Kconfig. I'm also going to be at XDC this week so I was going to
float some ideas there as well. I won't have a chance to do much with
this until next week though.

Thanks,
Laura

Re: [PATCH v2 2/2] staging: ion: create one device entry per heap

[Benjamin Gaignard]

2017-09-20 3:01 GMT+02:00 Laura Abbott <labbott@redhat.com>:
> On 09/19/2017 04:55 AM, Benjamin Gaignard wrote:
>>
>> 2017-09-19 13:02 GMT+02:00 Greg KH <gregkh@linuxfoundation.org>:
>>>
>>> On Tue, Sep 19, 2017 at 12:25:38PM +0200, Benjamin Gaignard wrote:
>>>>
>>>> Instead a getting one common device "/dev/ion" for
>>>> all the heaps this patch allow to create one device
>>>> entry ("/dev/ionX") per heap.
>>>> Getting an entry per heap could allow to set security rules
>>>> per heap and global ones for all heaps.
>>>>
>>>> Allocation requests will be only allowed if the mask_id
>>>> match with device minor.
>>>> Query request could be done on any of the devices.
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>>>> ---
>>>>   drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++--
>>>>   drivers/staging/android/ion/ion.c       | 23 +++++++++++++++++------
>>>>   drivers/staging/android/ion/ion.h       | 10 +++++++---
>>>>   3 files changed, 33 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>>>> b/drivers/staging/android/ion/ion-ioctl.c
>>>> index e26b786..c8c906c 100644
>>>> --- a/drivers/staging/android/ion/ion-ioctl.c
>>>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>>>> @@ -25,8 +25,11 @@ union ion_ioctl_arg {
>>>>        struct ion_heap_query query;
>>>>   };
>>>>
>>>> -static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg
>>>> *arg)
>>>> +static int validate_ioctl_arg(struct file *filp,
>>>> +                           unsigned int cmd, union ion_ioctl_arg *arg)
>>>>   {
>>>> +     int mask = 1 << iminor(filp->f_inode);
>>>> +
>>>>        switch (cmd) {
>>>>        case ION_IOC_HEAP_QUERY:
>>>>                if (arg->query.reserved0 ||
>>>> @@ -34,6 +37,10 @@ static int validate_ioctl_arg(unsigned int cmd, union
>>>> ion_ioctl_arg *arg)
>>>>                    arg->query.reserved2 )
>>>>                        return -EINVAL;
>>>>                break;
>>>> +     case ION_IOC_ALLOC:
>>>> +             if (!(arg->allocation.heap_id_mask & mask))
>>>> +                     return -EINVAL;
>>>> +             break;
>>>>        default:
>>>>                break;
>>>>        }
>>>> @@ -69,7 +76,7 @@ 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);
>>>> +     ret = validate_ioctl_arg(filp, cmd, &data);
>>>>        if (WARN_ON_ONCE(ret))
>>>>                return ret;
>>>>
>>>> diff --git a/drivers/staging/android/ion/ion.c
>>>> b/drivers/staging/android/ion/ion.c
>>>> index 93e2c90..3f8b595 100644
>>>> --- a/drivers/staging/android/ion/ion.c
>>>> +++ b/drivers/staging/android/ion/ion.c
>>>> @@ -40,6 +40,8 @@
>>>>
>>>>   #include "ion.h"
>>>>
>>>> +#define ION_DEV_MAX 32
>>>> +
>>>>   static struct ion_device *internal_dev;
>>>>   static int heap_id;
>>>>
>>>> @@ -541,11 +543,24 @@ void ion_device_add_heap(struct ion_heap *heap)
>>>>   {
>>>>        struct dentry *debug_file;
>>>>        struct ion_device *dev = internal_dev;
>>>> +     int ret;
>>>>
>>>>        if (!heap->ops->allocate || !heap->ops->free)
>>>>                pr_err("%s: can not add heap with invalid ops struct.\n",
>>>>                       __func__);
>>>>
>>>> +     if (heap_id >= ION_DEV_MAX)
>>>> +             return -EBUSY;
>>>> +
>>>> +     heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id);
>>>> +     dev_set_name(&heap->ddev, "ion%d", heap_id);
>>>> +     device_initialize(&heap->ddev);
>>>> +     cdev_init(&heap->chrdev, &ion_fops);
>>>> +     heap->chrdev.owner = THIS_MODULE;
>>>> +     ret = cdev_device_add(&heap->chrdev, &heap->ddev);
>>>> +     if (ret < 0)
>>>> +             return;
>>>
>>>
>>> No cleanup needed?  No reporting an error happened back up the chain?
>>> Not nice :(
>>
>>
>> I will change that
>>
>>>> +
>>>>        spin_lock_init(&heap->free_lock);
>>>>        heap->free_list_size = 0;
>>>>
>>>> @@ -595,13 +610,9 @@ static int ion_device_create(void)
>>>>        if (!idev)
>>>>                return -ENOMEM;
>>>>
>>>> -     idev->dev.minor = MISC_DYNAMIC_MINOR;
>>>> -     idev->dev.name = "ion";
>>>> -     idev->dev.fops = &ion_fops;
>>>> -     idev->dev.parent = NULL;
>>>> -     ret = misc_register(&idev->dev);
>>>> +     ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion");
>>>
>>>
>>> Did you just change the major number for the device node as well?
>>>
>> My understanding of alloc_chrdev_region() is that major number is chosen
>> dynamically but I don't understand the link with device node, sorry.
>>
>>> Wow, that's a lot of userspace breakage (both major number, and name),
>>> how did you test this?
>>
>>
>> I had to write a test by myself:
>>
>> https://git.linaro.org/people/benjamin.gaignard/ion_test_application.git/log/?h=one_device_per_heap
>>
>> Laura have tried to push a test VGEM but I believe it hasn't be
>> accepted yet (I will check)
>>
>>>
>>> Have you gotten "upstream" to agree to these changes?  We can't take
>>> these until they think it's ok as well.
>>
>>
>> Split /dev/ion into multiple nodes is one of the task listed in
>> staging/android/TODO
>> file before been able to de-stage ion.
>>
>> Since it has been a big bang in ion ABI on 4.12 and the fact  that
>> ion.h is still in
>> staging/android/uapi/ directory I do believe that userland is still
>> unstable.
>> I hope this kind of patch will help to clarify what is still need to
>> be done to de-stage ion
>> even if this patch is NACK-ed we can at least the item from the TODO list.
>>
>
> Thanks for sending this Benjamin.
>
> At plumbers, it was requested to not break the ABI too much or do it in
> one last big bang before moving out of staging.  My thought was to
> keep the old /dev/ion the same and allow all allocation and access via
> a Kconfig. I'm also going to be at XDC this week so I was going to
> float some ideas there as well. I won't have a chance to do much with
> this until next week though.
>

Hi Laura,

I will send a v3 to fix the comments already done.
I hope that could help to clarify if this patch is needed or not at XDC.
If splitting /dev/ion is not needed to de-stage driver code than we will
only have to remove the item from the TODO list.

Regards,
Benjamin

> Thanks,
> Laura