Re: [PATCH v15 1/5] dma-buf: Add dma-buf heaps framework

["Andrew F. Davis" <afd@ti.com>]

On 11/6/19 12:18 PM, Andrew F. Davis wrote:
> On 11/6/19 12:03 PM, John Stultz wrote:
>> On Wed, Nov 6, 2019 at 5:52 AM Andrew F. Davis <afd@ti.com> wrote:
>>>
>>> On 11/5/19 11:22 PM, John Stultz wrote:
>>>> +unsigned int dma_heap_ioctl_cmds[] = {
>>>> +     DMA_HEAP_IOC_ALLOC,
>>>> +};
>>>> +
>>>> +static long dma_heap_ioctl(struct file *file, unsigned int ucmd,
>>>> +                        unsigned long arg)
>>>> +{
>>>> +     char stack_kdata[128];
>>>> +     char *kdata = stack_kdata;
>>>> +     unsigned int kcmd;
>>>> +     unsigned int in_size, out_size, drv_size, ksize;
>>>> +     int nr = _IOC_NR(ucmd);
>>>> +     int ret = 0;
>>>> +
>>>> +     if (nr >= ARRAY_SIZE(dma_heap_ioctl_cmds))
>>>> +             return -EINVAL;
>>>> +
>>>> +     /* Get the kernel ioctl cmd that matches */
>>>> +     kcmd = dma_heap_ioctl_cmds[nr];
>>>
>>>
>>> Why do we need this indirection here and all the complexity below? I
>>> know DRM ioctl does something like this but it has a massive table,
>>> legacy ioctls, driver defined ioctls, etc..
>>>
>>> I don't expect we will ever need complex handling like this, could we
>>> switch back to the more simple handler from v13?
>>
>> I agree it does add complexity, but I'm not sure I see how to avoid
>> some of this. The logic trying to handle that the user may pass a cmd
>> that has the same _IOC_NR() as DMA_HEAP_IOC_ALLOC but not the same
>> size. So the simple "switch(cmd) { case DMA_HEAP_IOC_ALLOC:" we had
>> before won't work (as the cmd will be a different value).
>>
> 
> 
> DMA_HEAP_IOC_ALLOC encodes everything we need, if the size is different
> then the switch case will not match. It handled everything we have.
> 
> 
>> Thus why I thought the cleanest approach would be to use the
>> dma_heap_ioctl_cmds array to convert from whatever the user cmd is to
>> the matching kernel cmd value.
>>
> 
> 
> There are no kernel or user commands, just commands, they will match or
> they are not valid. If someday we some need a variable sized ioctl then
> we can deal with that then.
> 


Had a little discussion about this on IRC #dri-devel (check logs for
today if you want to follow along). Conclusion being the way it is done
here should be fine to help support forward compatibility. If optional
extensions to the structure are made that grow the size of data passed
in then we can ignore that and zero out the returned data without harm.
It is up to the flags field to mark incompatible changes that should
error out from kernel.
Andrew

> Andrew
> 
> 
>> Do you have an alternative suggestion that I'm overlooking?
>>
>> thanks
>> -john
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>