linux-kernel - Re: [PATCH] MTD: LPC32xx MLC NAND 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]

Message-ID: <20120515143428.GC13860@n2100.arm.linux.org.uk>
Date:	Tue, 15 May 2012 15:34:28 +0100
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Roland Stigge <stigge@...com.de>
Cc:	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org, devicetree-discuss@...ts.ozlabs.org,
	dwmw2@...radead.org, kevin.wells@....com, srinivas.bakki@....com,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] MTD: LPC32xx MLC NAND driver

On Tue, May 15, 2012 at 04:23:52PM +0200, Roland Stigge wrote:
> +static int lpc32xx_xmit_dma(struct mtd_info *mtd, dma_addr_t dma,
> +			    void *mem, int len, enum dma_transfer_direction dir)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	struct lpc32xx_nand_host *host = chip->priv;
> +	struct dma_async_tx_descriptor *desc;
> +	int flags = DMA_CTRL_ACK | DMA_PREP_INTERRUPT;
> +	int res;
> +	dma_cookie_t cookie;
> +
> +	host->dma_slave_config.direction = dir;
> +	host->dma_slave_config.src_addr = dma;
> +	host->dma_slave_config.dst_addr = dma;
> +	if (dmaengine_slave_config(host->dma_chan, &host->dma_slave_config)) {
> +		dev_err(mtd->dev.parent, "Failed to setup DMA slave\n");
> +		return -ENXIO;
> +	}

Can you explain why you need to issue this call for every transfer?

The 'direction' argument should be ignored by all modern DMA engine
drivers (we've decided previously to get rid of it).  As far as I can
see, the device address nor any of the other parameters ever change
between transfers.

> +
> +	sg_init_one(&host->sgl, mem, len);
> +
> +	res = dma_map_sg(host->dma_chan->device->dev, &host->sgl, 1, dir);

Also note that dma transfer directions and dma data directions are
different things.  You shouldn't mix the two.

> +	if (res != 1) {
> +		dev_err(mtd->dev.parent, "Failed to map sg list\n");
> +		return -ENXIO;
> +	}
> +	desc = dmaengine_prep_slave_sg(host->dma_chan, &host->sgl, 1, dir,
> +				       flags);
> +	if (!desc) {
> +		dev_err(mtd->dev.parent, "Failed to prepare slave sg\n");
> +		goto out1;
> +	}
> +
> +	init_completion(&host->comp_dma);
> +	desc->callback = lpc32xx_dma_complete_func;
> +	desc->callback_param = &host->comp_dma;
> +
> +	cookie = dmaengine_submit(desc);
> +	if (dma_submit_error(cookie)) {
> +		dev_err(mtd->dev.parent, "Failed to dmaengine_submit()\n");
> +		goto out1;
> +	}
> +	dma_async_issue_pending(host->dma_chan);
> +
> +	wait_for_completion_timeout(&host->comp_dma, msecs_to_jiffies(1000));
> +
> +	dma_sync_sg_for_cpu(host->dma_chan->device->dev, &host->sgl, 1, dir);

Why sync?  What's wrong with dma_unmap()?  Have you tested this with
DMA API debugging enabled?

> +	if (dma_set_mask(host->dma_chan->device->dev, 0xFFFFFFFF)) {
> +		dev_err(mtd->dev.parent, "Failed to set dma mask\n");
> +		goto out1;
> +	}

No.  You shouldn't do this to a device that you're not in charge of.  If
that's important for the DMA engine device, then the DMA engine device
driver should have already done that.

Otherwise, you'll be potentially modifying the DMA mask for a device
_you_ don't own, and which is potentially already in use.
--
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/