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

[Benjamin Gaignard]

2017-09-19 12:15 GMT+02:00 Tomas Winkler <tomasw@xxxxxxxxx>:
> On Tue, Sep 19, 2017 at 1:07 PM, Benjamin Gaignard
> <benjamin.gaignard@xxxxxxxxxx> wrote:
>> 2017-09-19 11:40 GMT+02:00 Dan Carpenter <dan.carpenter@xxxxxxxxxx>:
>>> On Mon, Sep 18, 2017 at 04:58:46PM +0200, Benjamin Gaignard wrote:
>>>> -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 ret = 0;
>>>> +     int mask = 1 << iminor(filp->f_inode);
>>>>
>>>>       switch (cmd) {
>>>>       case ION_IOC_HEAP_QUERY:
>>>> @@ -35,6 +37,9 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>>>>               ret |= arg->query.reserved1 != 0;
>>>>               ret |= arg->query.reserved2 != 0;
>>>>               break;
>>>> +     case ION_IOC_ALLOC:
>>>> +             ret = !(arg->allocation.heap_id_mask & mask);
>>>
>>>
>>> validate_ioctl_arg() is really convoluted.  From reading just the patch
>>> I at first thought we were returning 1 on failure.  Just say:
>>>
>>>         if (!(arg->allocation.heap_id_mask & mask))
>>>                 return -EINVAL;
>>>         return 0;
>>>
>>> If you want to fix the surrounding code in a separate patch that would
>>> be good.  It would be more clear to say:
>>>
>>>                 if (arg->query.reserved0 != 0 ||
>>>                     arg->query.reserved1 != 0 ||
>>>                     arg->query.reserved2 != 0)
>>>                         return -EINVAL;
>>
>> I agree I will add a fix for that in next version
>>
>>>
>>>> +             break;
>>>>       default:
>>>>               break;
>>>>       }
>>>> @@ -70,7 +75,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..5144f1a 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,21 @@ 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__);
>>>>
>>>
>>> I don't think it can happen in current code but we should proabably have
>>> a check here for:
>>>
>>>         if (heap_id >= ION_DEV_MAX)
>>>                 return -EBUSY;
>>>
>>> (It's possible I have missed something).
>>>
>>
>> You are right I will add that
>>
>> Thanks
>>>
>>>> +     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;
>
> What will happen to an application which looks for /dev/ion?

/dev/ion will no more exist with this patch.
Since ion ABI have already change a lot I don't think that could
be a problem to change also ion device.

>
> Thanks
> Tomas