From mboxrd@z Thu Jan  1 00:00:00 1970
From: Christoph Hellwig <hch@infradead.org>
Subject: Re: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework
Date: Fri, 15 Mar 2019 01:54:46 -0700
Message-ID: <20190315085446.GA4470@infradead.org>
References: <1551819273-640-1-git-send-email-john.stultz@linaro.org>
 <1551819273-640-2-git-send-email-john.stultz@linaro.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Return-path: <linux-kernel-owner@vger.kernel.org>
Content-Disposition: inline
In-Reply-To: <1551819273-640-2-git-send-email-john.stultz@linaro.org>
Sender: linux-kernel-owner@vger.kernel.org
To: John Stultz <john.stultz@linaro.org>
Cc: lkml <linux-kernel@vger.kernel.org>, "Andrew F. Davis" <afd@ti.com>, Laura Abbott <labbott@redhat.com>, Benjamin Gaignard <benjamin.gaignard@linaro.org>, Greg KH <gregkh@linuxfoundation.org>, Sumit Semwal <sumit.semwal@linaro.org>, Liam Mark <lmark@codeaurora.org>, Brian Starkey <Brian.Starkey@arm.com>, Chenbo Feng <fengc@google.com>, Alistair Strachan <astrachan@google.com>, dri-devel@lists.freedesktop.org
List-Id: dri-devel@lists.freedesktop.org

> +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.