linux-kernel - Re: [PATCH v3 2/2] dma: Add Xilinx AXI Direct Memory Access Engine driver support

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: <20140819170346.GS13288@intel.com>
Date:	Tue, 19 Aug 2014 22:33:46 +0530
From:	Vinod Koul <vinod.koul@...el.com>
To:	Srikanth Thokala <sthokal@...inx.com>
Cc:	dan.j.williams@...el.com, michal.simek@...inx.com,
	grant.likely@...aro.org, robh+dt@...nel.org, levex@...ux.com,
	jassisinghbrar@...il.com, linux-kernel@...r.kernel.org,
	dmaengine@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	devicetree@...r.kernel.org, anirudh@...inx.com, svemula@...inx.com
Subject: Re: [PATCH v3 2/2] dma: Add Xilinx AXI Direct Memory Access Engine
 driver support

On Mon, Jul 28, 2014 at 05:47:49PM +0530, Srikanth Thokala wrote:
> +struct xilinx_dma_chan {
> +	struct xilinx_dma_device *xdev;
> +	u32 ctrl_offset;
> +	spinlock_t lock;
> +	struct list_head pending_list;
> +	struct xilinx_dma_tx_descriptor *active_desc;
> +	struct xilinx_dma_tx_descriptor *allocated_desc;
> +	struct list_head done_list;
> +	struct list_head free_seg_list;
> +	struct dma_chan common;
> +	struct xilinx_dma_tx_segment *seg_v;
> +	dma_addr_t seg_p;
> +	struct device *dev;
> +	int irq;
> +	int id;
> +	enum dma_transfer_direction direction;
This looks suspect. Why should channel have direction, for a descriptor it
makes sense though.

> +static enum dma_status xilinx_dma_tx_status(struct dma_chan *dchan,
> +					    dma_cookie_t cookie,
> +					    struct dma_tx_state *txstate)
> +{
> +	struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
> +	enum dma_status ret;
> +	unsigned long flags;
> +
> +	ret = dma_cookie_status(dchan, cookie, txstate);
> +	if (ret != DMA_COMPLETE) {
> +		spin_lock_irqsave(&chan->lock, flags);
> +		dma_set_residue(txstate, chan->residue);
> +		spin_unlock_irqrestore(&chan->lock, flags);
> +	}
No residue reporting?

> +static int xilinx_dma_device_control(struct dma_chan *dchan,
> +				     enum dma_ctrl_cmd cmd, unsigned long arg)
> +{
> +	struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
> +	unsigned long flags;
> +
> +	if (cmd != DMA_TERMINATE_ALL)
> +		return -ENXIO;
> +
> +	/* Halt the DMA engine */
> +	xilinx_dma_halt(chan);
> +
> +	spin_lock_irqsave(&chan->lock, flags);
> +
> +	/* Remove and free all of the descriptors in the lists */
> +	xilinx_dma_free_desc_list(chan, &chan->pending_list);
> +	xilinx_dma_free_desc_list(chan, &chan->done_list);

> +
> +	spin_unlock_irqrestore(&chan->lock, flags);
> +
> +	return 0;
> +}
> +
> +/**
> + * xilinx_dma_channel_set_config - Configure DMA channel
> + * @dchan: DMA channel
> + * @cfg: DMA device configuration pointer
> + *
> + * Return: '0' on success and failure value on error
> + */
> +int xilinx_dma_channel_set_config(struct dma_chan *dchan,
> +				  struct xilinx_dma_config *cfg)
> +{
> +	struct xilinx_dma_chan *chan = to_xilinx_chan(dchan);
> +	u32 reg = dma_ctrl_read(chan, XILINX_DMA_REG_CONTROL);
> +
> +	if (cfg->reset)
> +		return xilinx_dma_reset(chan);
> +
> +	if (cfg->coalesc <= XILINX_DMA_CR_COALESCE_MAX)
> +		reg |= cfg->coalesc << XILINX_DMA_CR_COALESCE_SHIFT;
> +
> +	if (cfg->delay <= XILINX_DMA_CR_DELAY_MAX)
> +		reg |= cfg->delay << XILINX_DMA_CR_DELAY_SHIFT;
> +
> +	dma_ctrl_write(chan, XILINX_DMA_REG_CONTROL, reg);
You aren't checking if a transaction is already running on this channel.
Also don't you need other slave parameters, I see you have removed the
dma_slave_config entirely

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/