| 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: <561D1689.90703@linaro.org>
Date: Tue, 13 Oct 2015 15:34:49 +0100
From: Daniel Thompson <daniel.thompson@...aro.org>
To: M'boumba Cedric Madianga <cedric.madianga@...il.com>,
mcoquelin.stm32@...il.com, robh+dt@...nel.org, pawel.moll@....com,
mark.rutland@....com, ijc+devicetree@...lion.org.uk,
galak@...eaurora.org, linux@....linux.org.uk, vinod.koul@...el.com,
linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, dmaengine@...r.kernel.org
Subject: Re: [PATCH v2 2/4] dmaengine: Add STM32 DMA driver
On 13/10/15 15:05, M'boumba Cedric Madianga wrote:
> This patch adds support for the STM32 DMA controller.
>
> Signed-off-by: M'boumba Cedric Madianga <cedric.madianga@...il.com>
> ---
> drivers/dma/Kconfig | 12 +
> drivers/dma/Makefile | 1 +
> drivers/dma/stm32-dma.c | 1175 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 1188 insertions(+)
> create mode 100644 drivers/dma/stm32-dma.c
> diff --git a/drivers/dma/stm32-dma.c b/drivers/dma/stm32-dma.c
> new file mode 100644
> index 0000000..031bab7
> --- /dev/null
> +++ b/drivers/dma/stm32-dma.c
> +enum stm32_dma_width {
> + STM32_DMA_BYTE,
> + STM32_DMA_HALF_WORD,
> + STM32_DMA_WORD,
> +};
> +
> +enum stm32_dma_burst_size {
> + STM32_DMA_BURST_SINGLE,
> + STM32_DMA_BURST_INCR4,
> + STM32_DMA_BURST_INCR8,
> + STM32_DMA_BURST_INCR16,
> +};
> +
> +enum stm32_dma_channel_id {
> + STM32_DMA_CHANNEL0,
> + STM32_DMA_CHANNEL1,
> + STM32_DMA_CHANNEL2,
> + STM32_DMA_CHANNEL3,
> + STM32_DMA_CHANNEL4,
> + STM32_DMA_CHANNEL5,
> + STM32_DMA_CHANNEL6,
> + STM32_DMA_CHANNEL7,
> +};
Why have (unused) enumerations to map numeric symbols to their own
natural values? Using normal integers would be better.
> +enum stm32_dma_request_id {
> + STM32_DMA_REQUEST0,
> + STM32_DMA_REQUEST1,
> + STM32_DMA_REQUEST2,
> + STM32_DMA_REQUEST3,
> + STM32_DMA_REQUEST4,
> + STM32_DMA_REQUEST5,
> + STM32_DMA_REQUEST6,
> + STM32_DMA_REQUEST7,
> +};
Ditto.
> +static void stm32_dma_dump_reg(struct stm32_dma_chan *chan)
> +{
> + struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
> +
> + dev_dbg(chan2dev(chan), "SCR: 0x%08x\n",
> + stm32_dma_read(dmadev, STM32_DMA_SCR(chan->id)));
> + dev_dbg(chan2dev(chan), "NDTR: 0x%08x\n",
> + stm32_dma_read(dmadev, STM32_DMA_SNDTR(chan->id)));
> + dev_dbg(chan2dev(chan), "SPAR: 0x%08x\n",
> + stm32_dma_read(dmadev, STM32_DMA_SPAR(chan->id)));
> + dev_dbg(chan2dev(chan), "SM0AR: 0x%08x\n",
> + stm32_dma_read(dmadev, STM32_DMA_SM0AR(chan->id)));
> + dev_dbg(chan2dev(chan), "SM1AR: 0x%08x\n",
> + stm32_dma_read(dmadev, STM32_DMA_SM1AR(chan->id)));
> + dev_dbg(chan2dev(chan), "SFCR: 0x%08x\n",
> + stm32_dma_read(dmadev, STM32_DMA_SFCR(chan->id)));
> +}
Even at dev_dbg() this looks like log spam. This debug info gets issued
every time we set up a transfer!
> +static int stm32_dma_alloc_chan_resources(struct dma_chan *c)
> +{
> + struct stm32_dma_chan *chan = to_stm32_dma_chan(c);
> + struct stm32_dma_device *dmadev = stm32_dma_chan_get_dev(chan);
> + int ret;
> +
> + chan->config_init = false;
> + ret = clk_prepare_enable(dmadev->clk);
> + if (ret < 0) {
> + dev_err(chan2dev(chan), "clk_prepare_enable failed: %d\n", ret);
> + return ret;
> + }
> +
> + ret = stm32_dma_disable_chan(chan);
> +
> + return ret;
> +}
The error path here looks like it will leak clock references.
> +static int stm32_dma_probe(struct platform_device *pdev)
> +{
> + struct stm32_dma_chan *chan;
> + struct stm32_dma_device *dmadev;
> + struct dma_device *dd;
> + const struct of_device_id *match;
> + unsigned int i;
> + struct resource *res;
> + int ret;
> +
> + match = of_match_device(stm32_dma_of_match, &pdev->dev);
> + if (!match) {
> + dev_err(&pdev->dev, "Error: No device match found\n");
> + return -ENODEV;
> + }
> +
> + dmadev = devm_kzalloc(&pdev->dev, sizeof(*dmadev), GFP_KERNEL);
> + if (!dmadev)
> + return -ENOMEM;
> +
> + dd = &dmadev->ddev;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + dmadev->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(dmadev->base))
> + return PTR_ERR(dmadev->base);
> +
> + dmadev->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(dmadev->clk)) {
> + dev_err(&pdev->dev, "Error: Missing controller clock\n");
> + return PTR_ERR(dmadev->clk);
> + }
> +
> + dmadev->mem2mem = of_property_read_bool(pdev->dev.of_node,
> + "st,mem2mem");
> +
> + dmadev->rst = devm_reset_control_get(&pdev->dev, NULL);
> + if (!IS_ERR(dmadev->rst)) {
> + reset_control_assert(dmadev->rst);
> + udelay(2);
> + reset_control_deassert(dmadev->rst);
> + }
> +
> + dma_cap_set(DMA_SLAVE, dd->cap_mask);
> + dma_cap_set(DMA_PRIVATE, dd->cap_mask);
> + dma_cap_set(DMA_CYCLIC, dd->cap_mask);
> + dd->device_alloc_chan_resources = stm32_dma_alloc_chan_resources;
> + dd->device_free_chan_resources = stm32_dma_free_chan_resources;
> + dd->device_tx_status = stm32_dma_tx_status;
> + dd->device_issue_pending = stm32_dma_issue_pending;
> + dd->device_prep_slave_sg = stm32_dma_prep_slave_sg;
> + dd->device_prep_dma_cyclic = stm32_dma_prep_dma_cyclic;
> + dd->device_config = stm32_dma_slave_config;
> + dd->device_terminate_all = stm32_dma_terminate_all;
> + dd->src_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> + BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> + BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> + dd->dst_addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
> + BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
> + BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
> + dd->directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
> + dd->residue_granularity = DMA_RESIDUE_GRANULARITY_BURST;
> + dd->dev = &pdev->dev;
> + INIT_LIST_HEAD(&dd->channels);
> +
> + if (dmadev->mem2mem) {
> + dma_cap_set(DMA_MEMCPY, dd->cap_mask);
> + dd->device_prep_dma_memcpy = stm32_dma_prep_dma_memcpy;
> + dd->directions |= BIT(DMA_MEM_TO_MEM);
> + }
> +
> + for (i = 0; i < STM32_DMA_MAX_CHANNELS; i++) {
> + chan = &dmadev->chan[i];
> + chan->id = i;
> + chan->vchan.desc_free = stm32_dma_desc_free;
> + vchan_init(&chan->vchan, dd);
> + }
> +
> + ret = dma_async_device_register(dd);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < STM32_DMA_MAX_CHANNELS; i++) {
> + chan = &dmadev->chan[i];
> + res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
> + if (!res) {
> + ret = -EINVAL;
> + dev_err(&pdev->dev, "No irq resource for chan %d\n", i);
> + goto err_unregister;
> + }
> + chan->irq = res->start;
> + ret = devm_request_irq(&pdev->dev, chan->irq,
> + stm32_dma_chan_irq, 0,
> + dev_name(chan2dev(chan)), chan);
> + if (ret) {
> + dev_err(&pdev->dev,
> + "request_irq failed with err %d channel %d\n",
> + ret, i);
> + goto err_unregister;
> + }
> + }
> +
> + ret = of_dma_controller_register(pdev->dev.of_node,
> + stm32_dma_of_xlate, dmadev);
> + if (ret < 0) {
> + dev_err(&pdev->dev,
> + "STM32 DMA DMA OF registration failed %d\n", ret);
> + goto err_unregister;
> + }
> +
> + platform_set_drvdata(pdev, dmadev);
> +
> + dev_info(&pdev->dev, "STM32 DMA driver registered\n");
> +
> + return 0;
> +
> +err_unregister:
> + dma_async_device_unregister(dd);
> +
> + return ret;
> +}
> +
> +static int stm32_dma_remove(struct platform_device *pdev)
> +{
> + struct stm32_dma_device *dmadev = platform_get_drvdata(pdev);
> +
> + of_dma_controller_free(pdev->dev.of_node);
> +
> + dma_async_device_unregister(&dmadev->ddev);
> +
> + clk_disable_unprepare(dmadev->clk);
What is the purpose of disabling/unpreparing the clock here?
stm32_dma_alloc_chan_resources() and stm32_dma_free_chan_resources()
should pair up and the clock should already be stopped.
Daniel.
--
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