On 09/19/2017 04:55 AM, Benjamin Gaignard wrote:
2017-09-19 13:02 GMT+02:00 Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>:
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@xxxxxxxxxx>
---
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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
