Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework
To:     Christoph Hellwig <hch@infradead.org>,
        John Stultz <john.stultz@linaro.org>
CC:     lkml <linux-kernel@vger.kernel.org>,
        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>
References: <1551819273-640-1-git-send-email-john.stultz@linaro.org>
 <1551819273-640-2-git-send-email-john.stultz@linaro.org>
 <20190315085446.GA4470@infradead.org>
From:   "Andrew F. Davis" <afd@ti.com>
Message-ID: <525c65f4-7d73-bc77-5086-4db70e6e54f2@ti.com>
Date:   Fri, 15 Mar 2019 15:24:44 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.4.0
MIME-Version: 1.0
In-Reply-To: <20190315085446.GA4470@infradead.org>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 3/15/19 3:54 AM, Christoph Hellwig wrote:
>> +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.
> 

This was leftover from when release had some memory to free, will remove.

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

Thanks for info, I saw both used and this was used where I found the
prototype so I used it too, will fix.

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

Good idea, will fix.

>> +	dev_ret = device_create(dma_heap_class,
>> +				NULL,
>> +				heap->heap_devt,
>> +				NULL,
>> +				heap->name);
> 
> No need this weird argument alignment.
> 

I kinda like it this way, if everything cant fit on one line then
everything gets its own line, seems more consistent. If there is strong
objection I can fix.