| 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
| ||
|
Date: Tue, 31 May 2016 19:15:54 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Keguang Zhang <keguang.zhang@...il.com>
Cc: dmaengine <dmaengine@...r.kernel.org>, linux-mips@...ux-mips.org,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Vinod Koul <vinod.koul@...el.com>,
Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH V3] dmaengine: Loongson1: add Loongson1 dmaengine driver
On Sat, May 28, 2016 at 12:47 PM, Keguang Zhang <keguang.zhang@...il.com> wrote:
> From: Kelvin Cheung <keguang.zhang@...il.com>
>
> This patch adds DMA Engine driver for Loongson1B.
>
> Signed-off-by: Kelvin Cheung <keguang.zhang@...il.com>
> +++ b/drivers/dma/loongson1-dma.c
> +#include <dma.h>
> +
> +#include "dmaengine.h"
> +#include "virt-dma.h"
> +
> +/* 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)
GENMASK() ?
Btw, didn't notice
#include <linux/bitops.h>
> +
> +/* DMA H/W Descriptor Bits */
> +#define NEXT_EN BIT(0)
> +
> +/* DMA Command Register Bits */
> +#define DMA_RAM2DEV BIT(12)
> +#define DMA_TRANS_OVER BIT(3)
> +#define DMA_SINGLE_TRANS_OVER BIT(2)
> +#define DMA_INT BIT(1)
> +#define DMA_INT_MASK BIT(0)
GENMASK() ?
> +
> +struct ls1x_dma_hwdesc {
> + u32 next; /* next descriptor address */
> + u32 saddr; /* memory DMA address */
> + u32 daddr; /* device DMA address */
> + u32 length;
> + u32 stride;
> + u32 cycles;
> + u32 cmd;
> + u32 phys; /* used by driver */
> +} __aligned(64);
> +
> +struct ls1x_dma_desc {
> + struct virt_dma_desc vdesc;
> + struct ls1x_dma_chan *chan;
> +
> + enum dma_transfer_direction dir;
> + enum dma_transaction_type type;
> +
> + unsigned int nr_descs; /* number of descriptors */
> + unsigned int nr_done; /* number of completed descriptors */
> + struct ls1x_dma_hwdesc *desc[0]; /* DMA coherent descriptors */
> +};
> +/* macros for registers read/write */
> +#define chan_writel(chan, off, val) \
> + __raw_writel((val), (chan)->reg_base + (off))
> +
> +#define chan_readl(chan, off) \
> + __raw_readl((chan)->reg_base + (off))
> +
Hmm... Needs a comment why __raw_*() variants are in use.
> +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;
return chan_id == dma_chan->id;
> +}
> +static int ls1x_dma_alloc_chan_resources(struct dma_chan *chan)
> +{
> + struct ls1x_dma_chan *dma_chan = to_ls1x_dma_chan(chan);
> +
> + if (dma_chan->desc_pool)
> + return 0;
It shouldn't be called more than once. Why do you have this check?
> +
> + dma_chan->desc_pool = dma_pool_create(dma_chan_name(chan),
> + chan->device->dev,
> + sizeof(struct ls1x_dma_hwdesc),
> + __alignof__(struct
> + ls1x_dma_hwdesc), 0);
> + if (!dma_chan->desc_pool) {
> + dev_err(&chan->dev->device,
> + "failed to allocate descriptor pool\n");
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static void ls1x_dma_free_desc(struct virt_dma_desc *vdesc)
> +{
> + struct ls1x_dma_desc *dma_desc = to_ls1x_dma_desc(vdesc);
> + int i;
> +
> + for (i = 0; i < dma_desc->nr_descs; i++)
> + dma_pool_free(dma_desc->chan->desc_pool, dma_desc->desc[i],
> + dma_desc->desc[i]->phys);
> + kfree(dma_desc);
> +}
> +
> +static struct ls1x_dma_desc *ls1x_dma_alloc_desc(struct ls1x_dma_chan *dma_chan,
> + int sg_len)
> +{
> +err:
err_free:
> + dev_err(&chan->dev->device, "failed to allocate H/W DMA descriptor\n");
> +
> + while (--i >= 0)
> + dma_pool_free(dma_chan->desc_pool, dma_desc->desc[i],
> + dma_desc->desc[i]->phys);
> + kfree(dma_desc);
I'm pretty sure you might reuse ls1x_dma_free_desc() here.
> +
> + return NULL;
> +}
> +static struct dma_async_tx_descriptor *
> +ls1x_dma_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl,
> + unsigned int sg_len,
> + enum dma_transfer_direction direction,
> + unsigned long flags, void *context)
> +{
> + struct ls1x_dma_chan *dma_chan = to_ls1x_dma_chan(chan);
> + struct dma_slave_config *config = &dma_chan->config;
> + struct ls1x_dma_desc *dma_desc;
> + struct scatterlist *sg;
> + unsigned int dev_addr, bus_width, cmd, i;
> +
> + dev_dbg(&chan->dev->device, "sg_len=%d, dir=%s, flags=0x%lx\n", sg_len,
> + direction == DMA_MEM_TO_DEV ? "to device" : "from device",
> + flags);
Btw, you may use dev_vdbg() in some cases. We have two DEBUG options
for DMAengine.
I would suggest to revisit all debugging prints in your driver and
choose suitable level.
> +
> + switch (direction) {
> + case DMA_MEM_TO_DEV:
> + dev_addr = config->dst_addr;
> + bus_width = config->dst_addr_width;
> + cmd = DMA_RAM2DEV | DMA_INT;
> + break;
> + case DMA_DEV_TO_MEM:
> + dev_addr = config->src_addr;
> + bus_width = config->src_addr_width;
> + cmd = DMA_INT;
> + break;
> + default:
> + dev_err(&chan->dev->device,
> + "unsupported DMA transfer mode: %d\n", direction);
> + return NULL;
Shouldn't happen here.
> + }
> +
> + /* allocate DMA descriptors */
> + dma_desc = ls1x_dma_alloc_desc(dma_chan, sg_len);
> + if (!dma_desc)
> + return NULL;
> + dma_desc->dir = direction;
> + dma_desc->type = DMA_SLAVE;
> +
> + /* config DMA descriptors */
> + for_each_sg(sgl, sg, sg_len, i) {
> + dma_addr_t buf_addr = sg_dma_address(sg);
> + size_t buf_len = sg_dma_len(sg);
> +
> + if (!IS_ALIGNED(buf_addr, 4 * bus_width)) {
> + dev_err(&chan->dev->device,
> + "buf_addr is not aligned on %d-byte boundary\n",
> + 4 * bus_width);
Shouldn't we use
.copy_align = DMAENGINE_ALIGN_4_BYTES;
during the probe?
> + ls1x_dma_free_desc(&dma_desc->vdesc);
> + return NULL;
> + }
> +
> + if (!IS_ALIGNED(buf_len, bus_width))
> + dev_warn(&chan->dev->device,
> + "buf_len is not aligned on %d-byte boundary\n",
> + bus_width);
> +
> + dma_desc->desc[i]->saddr = buf_addr;
> + dma_desc->desc[i]->daddr = dev_addr;
> + dma_desc->desc[i]->length = buf_len / bus_width;
> + dma_desc->desc[i]->stride = 0;
> + dma_desc->desc[i]->cycles = 1;
> + dma_desc->desc[i]->cmd = cmd;
> + dma_desc->desc[i]->next =
> + sg_is_last(sg) ? 0 : dma_desc->desc[i + 1]->phys;
> +
> + dev_dbg(&chan->dev->device,
> + "desc=%p, saddr=%08x, daddr=%08x, length=%u\n",
> + &dma_desc->desc[i], buf_addr, dev_addr, buf_len);
> + }
> +
> + return vchan_tx_prep(&dma_chan->vchan, &dma_desc->vdesc, flags);
> +}
> +
> +static int ls1x_dma_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct plat_ls1x_dma *pdata = dev_get_platdata(dev);
> + struct dma_device *dma_dev;
> + struct ls1x_dma *dma;
> + struct ls1x_dma_chan *dma_chan;
> + struct resource *res;
> + int i, ret;
> +
> + /* initialize DMA device */
> + dma =
> + devm_kzalloc(dev,
> + sizeof(struct ls1x_dma) +
> + pdata->nr_channels * sizeof(struct ls1x_dma_chan),
> + GFP_KERNEL);
> + if (!dma)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(dev, "failed to get I/O memory\n");
> + return -EINVAL;
> + }
Redundant LOCs since below call does it for you.
> +
> + dma->reg_base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(dma->reg_base))
> + return PTR_ERR(dma->reg_base);
> + /* initialize DMA channels */
> + for (i = 0; i < pdata->nr_channels; i++) {
> + dma_chan = &dma->dma_chan[i];
> + dma_chan->id = i;
> + dma_chan->reg_base = dma->reg_base;
> +
> + dma_chan->irq = platform_get_irq(pdev, i);
> + if (dma_chan->irq < 0) {
> + dev_err(dev, "failed to get IRQ: %d\n", dma_chan->irq);
> + return -EINVAL;
> + }
> +
> + ret =
> + devm_request_irq(dev, dma_chan->irq, ls1x_dma_irq_handler,
> + IRQF_SHARED, dev_name(dev), dma_chan);
> + if (ret) {
> + dev_err(dev, "failed to request IRQ %u!\n",
> + dma_chan->irq);
> + return -EINVAL;
> + }
> +
> + dma_chan->vchan.desc_free = ls1x_dma_free_desc;
> + vchan_init(&dma_chan->vchan, dma_dev);
> + }
> + dma->nr_dma_chans = i;
You can't get here when i != nr_channels, so, what's the point to use
counter variable?
> +
> + dma->clk = devm_clk_get(dev, pdev->name);
> + if (IS_ERR(dma->clk)) {
> + dev_err(dev, "failed to get %s clock\n", pdev->name);
> + return PTR_ERR(dma->clk);
> + }
> + clk_prepare_enable(dma->clk);
> +
> + ret = dma_async_device_register(dma_dev);
> + if (ret) {
> + dev_err(dev, "failed to register DMA device\n");
> + clk_disable_unprepare(dma->clk);
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, dma);
> + dev_info(dev, "Loongson1 DMA driver registered\n");
> + for (i = 0; i < pdata->nr_channels; i++) {
> + dma_chan = &dma->dma_chan[i];
> + dev = &dma_chan->vchan.chan.dev->device;
I wouldn't shadow the actual dev pointer.
> + dev_info(dev, "channel %d at 0x%p (irq %d)\n", dma_chan->id,
> + dma_chan->reg_base, dma_chan->irq);
> + }
> +
> + return 0;
> +}
> +static struct platform_driver ls1x_dma_driver = {
> + .probe = ls1x_dma_probe,
> + .remove = ls1x_dma_remove,
> + .driver = {
> + .name = "ls1x-dma",
PM ?
> + },
> +};
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists