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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190315085446.GA4470@infradead.org>
Date:   Fri, 15 Mar 2019 01:54:46 -0700
From:   Christoph Hellwig <hch@...radead.org>
To:     John Stultz <john.stultz@...aro.org>
Cc:     lkml <linux-kernel@...r.kernel.org>,
        "Andrew F. Davis" <afd@...com>, Laura Abbott <labbott@...hat.com>,
        Benjamin Gaignard <benjamin.gaignard@...aro.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Liam Mark <lmark@...eaurora.org>,
        Brian Starkey <Brian.Starkey@....com>,
        Chenbo Feng <fengc@...gle.com>,
        Alistair Strachan <astrachan@...gle.com>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [RFC][PATCH 1/5 v2] dma-buf: Add dma-buf heaps framework

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