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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2017-09-20 3:01 GMT+02:00 Laura Abbott <labbott@xxxxxxxxxx>:
> 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.
>

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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux