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

From: Christoph Hellwig
Date: Fri Mar 15 2019 - 04:54:57 EST


> +static int dma_heap_release(struct inode *inode, struct file *filp)
> +{
> + filp->private_data = NULL;
> +
> + return 0;
> +}

No point in clearing ->private_data, the file is about to be freed.

> +
> +static long dma_heap_ioctl(struct file *filp, unsigned int cmd,
> + unsigned long arg)

Pleae don't use the weird legacy filp naming, file is a perfectly
valid and readable default name for struct file pointers.

> +{
> + switch (cmd) {
> + case DMA_HEAP_IOC_ALLOC:
> + {
> + struct dma_heap_allocation_data heap_allocation;
> + struct dma_heap *heap = filp->private_data;
> + int fd;

Please split each ioctl into a separate function from the very start,
otherwise this will grow into a spaghetty mess sooner than you can
see cheese.

> + dev_ret = device_create(dma_heap_class,
> + NULL,
> + heap->heap_devt,
> + NULL,
> + heap->name);

No need this weird argument alignment.