Re: [PATCH v2 2/2] staging: ion: create one device entry per heap
From: Benjamin Gaignard
Date: Wed Sep 20 2017 - 03:47:33 EST
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