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

[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.

Thanks,
Laura