| Message ID | 1505816738-30017-3-git-send-email-benjamin.gaignard@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | staging: ion: get one device per heap | expand |
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
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
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
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
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
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
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;
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(-) -- 2.7.4