| 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
| ||
|
Message-ID: <20141205173553.GS3411@intel.com>
Date: Fri, 5 Dec 2014 23:05:53 +0530
From: Vinod Koul <vinod.koul@...el.com>
To: Andrew Bresticker <abrestic@...omium.org>
Cc: Dan Williams <dan.j.williams@...el.com>,
Rob Herring <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
Grant Likely <grant.likely@...aro.org>,
James Hartley <james.hartley@...tec.com>,
James Hogan <james.hogan@...tec.com>,
Ezequiel Garcia <ezequiel.garcia@...tec.com>,
Damien Horsley <Damien.Horsley@...tec.com>,
Arnd Bergmann <arnd@...db.de>, dmaengine@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 2/2] dmaengine: Add driver for IMG MDC
On Fri, Nov 14, 2014 at 01:59:43PM -0800, Andrew Bresticker wrote:
> +static inline unsigned int to_mdc_width(enum dma_slave_buswidth bw)
> +{
> + switch (bw) {
> + case DMA_SLAVE_BUSWIDTH_1_BYTE:
> + return 0;
> + case DMA_SLAVE_BUSWIDTH_2_BYTES:
> + return 1;
> + case DMA_SLAVE_BUSWIDTH_4_BYTES:
> + return 2;
> + case DMA_SLAVE_BUSWIDTH_8_BYTES:
> + return 3;
ffs()?
> + default:
> + return 2;
for slave cases default makes little sense, better to return error?
> + }
> +}
> +
> +static void mdc_list_desc_config(struct mdc_chan *mchan,
> + struct mdc_hw_list_desc *ldesc,
> + enum dma_transfer_direction dir,
> + dma_addr_t src, dma_addr_t dst, size_t len)
> +{
> + struct mdc_dma *mdma = mchan->mdma;
> + unsigned int max_burst, burst_size;
> +
> + ldesc->gen_conf = MDC_GENERAL_CONFIG_IEN | MDC_GENERAL_CONFIG_LIST_IEN |
> + MDC_GENERAL_CONFIG_LEVEL_INT | MDC_GENERAL_CONFIG_PHYSICAL_W |
> + MDC_GENERAL_CONFIG_PHYSICAL_R;
> + ldesc->readport_conf =
> + (mchan->thread << MDC_READ_PORT_CONFIG_STHREAD_SHIFT) |
> + (mchan->thread << MDC_READ_PORT_CONFIG_RTHREAD_SHIFT) |
> + (mchan->thread << MDC_READ_PORT_CONFIG_WTHREAD_SHIFT);
> + ldesc->read_addr = src;
> + ldesc->write_addr = dst;
> + ldesc->xfer_size = len - 1;
> + ldesc->node_addr = 0;
> + ldesc->cmds_done = 0;
> + ldesc->ctrl_status = MDC_CONTROL_AND_STATUS_LIST_EN |
> + MDC_CONTROL_AND_STATUS_EN;
> + ldesc->next_desc = NULL;
> +
> + if ((dst % mdma->bus_width == 0) && (src % mdma->bus_width == 0))
> + max_burst = mdma->bus_width * mdma->max_burst_mult;
> + else
> + max_burst = mdma->bus_width * (mdma->max_burst_mult - 1);
sounds like you should use something like ALIGN ?
> +
> + if (dir == DMA_MEM_TO_DEV) {
> + ldesc->gen_conf |= MDC_GENERAL_CONFIG_INC_R |
> + ((fls(mdma->bus_width) - 1) <<
bus_width calculation is repeated quite few times, would help readability if
we hanlde this in macro. Also this be using src/dstn_addr_widths based on
direction passed
> + MDC_GENERAL_CONFIG_WIDTH_R_SHIFT) |
> + (to_mdc_width(mchan->config.dst_addr_width) <<
> + MDC_GENERAL_CONFIG_WIDTH_W_SHIFT);
and a macro for this calculation, which take args for different cases would
help a lot!
> + ldesc->readport_conf |= MDC_READ_PORT_CONFIG_DREQ_ENABLE;
> + burst_size = min(max_burst, mchan->config.dst_maxburst *
> + mchan->config.dst_addr_width);
> + } else if (dir == DMA_DEV_TO_MEM) {
> + ldesc->gen_conf |= MDC_GENERAL_CONFIG_INC_W |
> + (to_mdc_width(mchan->config.src_addr_width) <<
> + MDC_GENERAL_CONFIG_WIDTH_R_SHIFT) |
> + ((fls(mdma->bus_width) - 1) <<
> + MDC_GENERAL_CONFIG_WIDTH_W_SHIFT);
> + ldesc->readport_conf |= MDC_READ_PORT_CONFIG_DREQ_ENABLE;
> + burst_size = min(max_burst, mchan->config.src_maxburst *
> + mchan->config.src_addr_width);
> + } else {
DEV_TO_DEV too?
> + ldesc->gen_conf |= MDC_GENERAL_CONFIG_INC_R |
> + MDC_GENERAL_CONFIG_INC_W |
> + ((fls(mdma->bus_width) - 1) <<
> + MDC_GENERAL_CONFIG_WIDTH_R_SHIFT) |
> + ((fls(mdma->bus_width) - 1) <<
> + MDC_GENERAL_CONFIG_WIDTH_W_SHIFT);
> + burst_size = max_burst;
btw this piece is very hard to read, please do improve upon
> +static struct dma_async_tx_descriptor *mdc_prep_dma_memcpy(
> + struct dma_chan *chan, dma_addr_t dest, dma_addr_t src, size_t len,
> + unsigned long flags)
> +{
> + struct mdc_chan *mchan = to_mdc_chan(chan);
> + struct mdc_dma *mdma = mchan->mdma;
> + struct mdc_tx_desc *mdesc;
> + struct mdc_hw_list_desc *curr, *prev = NULL;
> + dma_addr_t curr_phys, prev_phys;
> +
> + if (!len)
> + return NULL;
> +
> + mdesc = kzalloc(sizeof(*mdesc), GFP_NOWAIT);
> + if (!mdesc)
> + return NULL;
> + mdesc->chan = mchan;
> + mdesc->list_xfer_size = len;
> +
> + while (len > 0) {
> + size_t xfer_size;
> +
> + curr = dma_pool_alloc(mdma->desc_pool, GFP_NOWAIT, &curr_phys);
> + if (!curr)
> + goto free_desc;
> +
> + if (prev) {
> + prev->node_addr = curr_phys;
> + prev->next_desc = curr;
> + } else {
> + mdesc->list_phys = curr_phys;
> + mdesc->list = curr;
> + }
> +
> + xfer_size = min_t(size_t, mdma->max_xfer_size, len);
> +
> + mdc_list_desc_config(mchan, curr, DMA_MEM_TO_MEM, src, dest,
> + xfer_size);
and this depends on dma_slave_config being set, which shouldn't be the case
for memcpy
> +static enum dma_status mdc_tx_status(struct dma_chan *chan,
> + dma_cookie_t cookie, struct dma_tx_state *txstate)
> +{
> + struct mdc_chan *mchan = to_mdc_chan(chan);
> + struct mdc_tx_desc *mdesc;
> + struct virt_dma_desc *vd;
> + unsigned long flags;
> + size_t bytes = 0;
> + int ret;
> +
> + ret = dma_cookie_status(chan, cookie, txstate);
> + if (ret == DMA_COMPLETE)
> + return ret;
> +
you should check txstate before proceeding. It can be null too
> +
> +static int mdc_slave_config(struct dma_chan *chan,
> + struct dma_slave_config *config)
> +{
> + struct mdc_chan *mchan = to_mdc_chan(chan);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&mchan->vc.lock, flags);
> + if (config->direction == DMA_MEM_TO_DEV) {
> + config->dst_addr_width = min(config->dst_addr_width,
> + mchan->mdma->bus_width);
> + } else {
> + config->src_addr_width = min(config->src_addr_width,
> + mchan->mdma->bus_width);
direction field is deprecated pls don't use that. Use the prep_xx direction
argument instead
> +static int mdc_dma_remove(struct platform_device *pdev)
> +{
> + struct mdc_dma *mdma = platform_get_drvdata(pdev);
> +
> + of_dma_controller_free(pdev->dev.of_node);
> + dma_async_device_unregister(&mdma->dma_dev);
> + clk_disable_unprepare(mdma->clk);
you need to call devm_irq_free() explicitly and also ensure you kill your
tasklet and syncronize irq()
Plus a rebase on Maxime's series which will be merged in required
--
~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/
Powered by blists - more mailing lists