linux-kernel - Re: [PATCH V3] dmaengine: Loongson1: add Loongson1 dmaengine driver

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]

Date:	Mon, 30 May 2016 10:20:14 +0530
From:	Vinod Koul <vinod.koul@...el.com>
To:	Keguang Zhang <keguang.zhang@...il.com>
Cc:	dmaengine@...r.kernel.org, linux-mips@...ux-mips.org,
	linux-kernel@...r.kernel.org,
	Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH V3] dmaengine: Loongson1: add Loongson1 dmaengine driver

On Sat, May 28, 2016 at 05:47:13PM +0800, Keguang Zhang wrote:
> +/* Loongson 1 DMA Register Definitions */
> +#define DMA_CTRL		0x0
> +
> +/* DMA Control Register Bits */
> +#define DMA_STOP		BIT(4)
> +#define DMA_START		BIT(3)
> +#define ASK_VALID		BIT(2)
> +
> +#define DMA_ADDR_MASK		(0xffffffc0)

These should be namespaced properly. Having generic one like DMA_STOP
DMA_ADDR_MASK makes it prone to conflicts with common symbols!

> +bool ls1x_dma_filter(struct dma_chan *chan, void *param)
> +{
> +	struct ls1x_dma_chan *dma_chan = to_ls1x_dma_chan(chan);
> +	unsigned int chan_id = *(unsigned int *)param;
> +
> +	if (chan_id == dma_chan->id)
> +		return true;
> +	else
> +		return false;
> +}

considering this is a new driver do you need a filter? Also where are the
bindings for this driver?

> +static struct ls1x_dma_desc *ls1x_dma_alloc_desc(struct ls1x_dma_chan *dma_chan,
> +						 int sg_len)

if you splitting up please ensure preceding line is < 80 char, this looks
quite bad

> +
> +	switch (direction) {
> +	case DMA_MEM_TO_DEV:
> +		dev_addr = config->dst_addr;
> +		bus_width = config->dst_addr_width;
> +		cmd = DMA_RAM2DEV | DMA_INT;
> +		break;

empty line here

> +	case DMA_DEV_TO_MEM:
> +		dev_addr = config->src_addr;
> +		bus_width = config->src_addr_width;
> +		cmd = DMA_INT;
> +		break;

here too

> +static size_t ls1x_dma_desc_residue(struct ls1x_dma_desc *dma_desc,
> +				    unsigned int next_sg)
> +{
> +	struct ls1x_dma_chan *dma_chan = dma_desc->chan;
> +	struct dma_slave_config *config = &dma_chan->config;
> +	unsigned int i, bus_width, bytes = 0;
> +
> +	if (dma_desc->dir == DMA_MEM_TO_DEV)
> +		bus_width = config->dst_addr_width;
> +	else
> +		bus_width = config->src_addr_width;

dma slave config can be reprogrammed for subsequent transaction, you should
read this from descriptor and not channel

> +
> +	for (i = next_sg; i < dma_desc->nr_descs; i++)
> +		bytes += dma_desc->desc[i]->length * bus_width;
> +
> +	return bytes;

Okay how does this tell me "residue"

For not yet processed this tells me all descriptor size, why would I want
that?

> +}
> +
> +static enum dma_status ls1x_dma_tx_status(struct dma_chan *chan,
> +					  dma_cookie_t cookie,
> +					  struct dma_tx_state *txstate)
> +{
> +	struct ls1x_dma_chan *dma_chan = to_ls1x_dma_chan(chan);
> +	struct ls1x_dma_desc *dma_desc = dma_chan->dma_desc;
> +	struct virt_dma_desc *vdesc;
> +	enum dma_status status;
> +	unsigned int residue = 0;
> +	unsigned long flags;
> +
> +	status = dma_cookie_status(chan, cookie, txstate);
> +	if ((status == DMA_COMPLETE) || !txstate)
> +		return status;
> +
> +	spin_lock_irqsave(&dma_chan->vchan.lock, flags);
> +
> +	vdesc = vchan_find_desc(&dma_chan->vchan, cookie);
> +	if (vdesc)
> +		/* not yet processed */
> +		residue = ls1x_dma_desc_residue(to_ls1x_dma_desc(vdesc), 0);
> +	else if (cookie == dma_chan->dma_desc->vdesc.tx.cookie)
> +		/* in progress */
> +		residue = ls1x_dma_desc_residue(dma_desc, dma_desc->nr_done);
> +	else
> +		residue = 0;
> +
> +	spin_unlock_irqrestore(&dma_chan->vchan.lock, flags);
> +
> +	dma_set_residue(txstate, residue);
> +
> +	return status;
> +}

> +static int ls1x_dma_remove(struct platform_device *pdev)
> +{
> +	struct ls1x_dma *dma = platform_get_drvdata(pdev);
> +
> +	dma_async_device_unregister(&dma->dma_dev);
> +	clk_disable_unprepare(dma->clk);

You are using devm_request_irq(), now how is the irq quisced at this point
and how do you ensure all your tasklets have finished processing!

> +MODULE_AUTHOR("Kelvin Cheung <keguang.zhang@...il.com>");
> +MODULE_DESCRIPTION("Loongson1 DMA driver");
> +MODULE_LICENSE("GPL");

No ALIAS?

-- 
~Vinod