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

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

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.

Andrew


> Do you have an alternative suggestion that I'm overlooking?
> 
> thanks
> -john
>