dmaengine: bcm2835-dma: Convert to use DMA pool

f93178291712 dmaengine: bcm2835-dma: Fix memory leak when stopping a
running transfer

Fixed the memleak, but introduced another issue: the terminate_all callback might be called with interrupts disabled and the dma_free_coherent() is not allowed to be called when IRQs are disabled.
Convert the driver to use dma_pool_* for managing the list of control blocks for the transfer.

Fixes f93178291712 ("dmaengine: bcm2835-dma: Fix memory leak when stopping a running transfer")

Signed-off-by Peter Ujfalusi <[email protected]>

---
Hi,

It was brought to my attention that the memleak fix broke the bcm2835 DMA. I did not noticed the use of dma_free_coherent() in the driver when I did the memleak fix.
Since the driver does leaking memory every time the audio is stopped, the other option is to convert it to use DMA pool.
I do not have access the Raspberry Pi, so I can not test this patch but it compiles ;)
Can someone test this one out if it is working?

Regards,
Peter

 drivers/dma/bcm2835-dma.c | 78 ++++++++++++++++++++++++++++++++---------------
 1 file changed, 54 insertions(+), 24 deletions(-)

drivers/dma/bcm2835-dma.c +54 -24

index c92d6a70ccf3..996c4b00d323 100644
@@ -31,6 +31,7 @@
  */
 #include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
+#include <linux/dmapool.h>
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>

@@ -62,6 +63,11 @@ struct bcm2835_dma_cb {
 	uint32_t pad[2];
 };
 
+struct bcm2835_cb_entry {
+	struct bcm2835_dma_cb *cb;
+	dma_addr_t paddr;
+};
+
 struct bcm2835_chan {
 	struct virt_dma_chan vc;
 	struct list_head node;

@@ -72,18 +78,18 @@ struct bcm2835_chan {
 
 	int ch;
 	struct bcm2835_desc *desc;
+	struct dma_pool *cb_pool;
 
 	void __iomem *chan_base;
 	int irq_number;
 };
 
 struct bcm2835_desc {
+	struct bcm2835_chan *c;
 	struct virt_dma_desc vd;
 	enum dma_transfer_direction dir;
 
-	unsigned int control_block_size;
-	struct bcm2835_dma_cb *control_block_base;
-	dma_addr_t control_block_base_phys;
+	struct bcm2835_cb_entry *cb_list;
 
 	unsigned int frames;
 	size_t size;

@@ -143,10 +149,13 @@ static inline struct bcm2835_desc *to_bcm2835_dma_desc(
 static void bcm2835_dma_desc_free(struct virt_dma_desc *vd)
 {
 	struct bcm2835_desc *desc = container_of(vd, struct bcm2835_desc, vd);
-	dma_free_coherent(desc->vd.tx.chan->device->dev,
-			desc->control_block_size,
-			desc->control_block_base,
-			desc->control_block_base_phys);
+	int i;
+
+	for (i = 0; i < desc->frames; i++)
+		dma_pool_free(desc->c->cb_pool, desc->cb_list[i].cb,
+			      desc->cb_list[i].paddr);
+
+	kfree(desc->cb_list);
 	kfree(desc);
 }
 

@@ -199,7 +208,7 @@ static void bcm2835_dma_start_desc(struct bcm2835_chan *c)
 
 	c->desc = d = to_bcm2835_dma_desc(&vd->tx);
 
-	writel(d->control_block_base_phys, c->chan_base + BCM2835_DMA_ADDR);
+	writel(d->cb_list[0].paddr, c->chan_base + BCM2835_DMA_ADDR);
 	writel(BCM2835_DMA_ACTIVE, c->chan_base + BCM2835_DMA_CS);
 }
 

@@ -232,9 +241,16 @@ static irqreturn_t bcm2835_dma_callback(int irq, void *data)
 static int bcm2835_dma_alloc_chan_resources(struct dma_chan *chan)
 {
 	struct bcm2835_chan *c = to_bcm2835_dma_chan(chan);
+	struct device *dev = c->vc.chan.device->dev;
+
+	dev_dbg(dev, "Allocating DMA channel %d\n", c->ch);
 
-	dev_dbg(c->vc.chan.device->dev,
-			"Allocating DMA channel %d\n", c->ch);
+	c->cb_pool = dma_pool_create(dev_name(dev), dev,
+				     sizeof(struct bcm2835_dma_cb), 0, 0);
+	if (!c->cb_pool) {
+		dev_err(dev, "unable to allocate descriptor pool\n");
+		return -ENOMEM;
+	}
 
 	return request_irq(c->irq_number,
 			bcm2835_dma_callback, 0, "DMA IRQ", c);

@@ -246,6 +262,7 @@ static void bcm2835_dma_free_chan_resources(struct dma_chan *chan)
 
 	vchan_free_chan_resources(&c->vc);
 	free_irq(c->irq_number, c);
+	dma_pool_destroy(c->cb_pool);
 
 	dev_dbg(c->vc.chan.device->dev, "Freeing DMA channel %u\n", c->ch);
 }

@@ -261,8 +278,7 @@ static size_t bcm2835_dma_desc_size_pos(struct bcm2835_desc *d, dma_addr_t addr)
 	size_t size;
 
 	for (size = i = 0; i < d->frames; i++) {
-		struct bcm2835_dma_cb *control_block =
-			&d->control_block_base[i];
+		struct bcm2835_dma_cb *control_block = d->cb_list[i].cb;
 		size_t this_size = control_block->length;
 		dma_addr_t dma;
 

@@ -343,6 +359,7 @@ static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic(
 	dma_addr_t dev_addr;
 	unsigned int es, sync_type;
 	unsigned int frame;
+	int i;
 
 	/* Grab configuration */
 	if (!is_slave_direction(direction)) {

@@ -374,27 +391,31 @@ static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic(
 	if (!d)
 		return NULL;
 
+	d->c = c;
 	d->dir = direction;
 	d->frames = buf_len / period_len;
 
-	/* Allocate memory for control blocks */
-	d->control_block_size = d->frames * sizeof(struct bcm2835_dma_cb);
-	d->control_block_base = dma_zalloc_coherent(chan->device->dev,
-			d->control_block_size, &d->control_block_base_phys,
-			GFP_NOWAIT);
-
-	if (!d->control_block_base) {
+	d->cb_list = kcalloc(d->frames, sizeof(*d->cb_list), GFP_KERNEL);
+	if (!d->cb_list) {
 		kfree(d);
 		return NULL;
 	}
+	/* Allocate memory for control blocks */
+	for (i = 0; i < d->frames; i++) {
+		struct bcm2835_cb_entry *cb_entry = &d->cb_list[i];
+
+		cb_entry->cb = dma_pool_zalloc(c->cb_pool, GFP_ATOMIC,
+					       &cb_entry->paddr);
+		if (!cb_entry->cb)
+			goto error_cb;
+	}
 
 	/*
 	 * Iterate over all frames, create a control block
 	 * for each frame and link them together.
 	 */
 	for (frame = 0; frame < d->frames; frame++) {
-		struct bcm2835_dma_cb *control_block =
-			&d->control_block_base[frame];
+		struct bcm2835_dma_cb *control_block = d->cb_list[frame].cb;
 
 		/* Setup adresses */
 		if (d->dir == DMA_DEV_TO_MEM) {

@@ -428,12 +449,21 @@ static struct dma_async_tx_descriptor *bcm2835_dma_prep_dma_cyclic(
 		 * This DMA engine driver currently only supports cyclic DMA.
 		 * Therefore, wrap around at number of frames.
 		 */
-		control_block->next = d->control_block_base_phys +
-			sizeof(struct bcm2835_dma_cb)
-			* ((frame + 1) % d->frames);
+		control_block->next = d->cb_list[((frame + 1) % d->frames)].paddr;
 	}
 
 	return vchan_tx_prep(&c->vc, &d->vd, flags);
+error_cb:
+	i--;
+	for (; i >= 0; i--) {
+		struct bcm2835_cb_entry *cb_entry = &d->cb_list[i];
+
+		dma_pool_free(c->cb_pool, cb_entry->cb, cb_entry->paddr);
+	}
+
+	kfree(d->cb_list);
+	kfree(d);
+	return NULL;
 }
 
 static int bcm2835_dma_slave_config(struct dma_chan *chan,

Hi,

Quoted 8 lines · Peter Ujfalusi

f93178291712 dmaengine: bcm2835-dma: Fix memory leak when stopping a
running transfer

Fixed the memleak, but introduced another issue: the terminate_all callback might be called with interrupts disabled and the dma_free_coherent() is not allowed to be called when IRQs are disabled.
Convert the driver to use dma_pool_* for managing the list of control blocks for the transfer.

FWIW: the patch has been tested and verified on Raspbery Pi:
https://github.com/raspberrypi/linux/pull/1178#issuecomment-157026794
https://github.com/raspberrypi/linux/pull/1178#issuecomment-157030190

It needed some modification since the Raspberry Pi kernel have non upstreamed changes in bcm2835-dma driver (slave_sg support for example).

It would be great if this patch can make it to 4.4 as a fix.

Thanks,
P?ter

Quoted 14 lines

Fixes f93178291712 ("dmaengine: bcm2835-dma: Fix memory leak when stopping a running transfer")

Signed-off-by Peter Ujfalusi <[email protected]>

---
Hi,

It was brought to my attention that the memleak fix broke the bcm2835 DMA. I did not noticed the use of dma_free_coherent() in the driver when I did the memleak fix.
Since the driver does leaking memory every time the audio is stopped, the other option is to convert it to use DMA pool.
I do not have access the Raspberry Pi, so I can not test this patch but it compiles ;)
Can someone test this one out if it is working?

Quoted 4 lines

Regards,
Peter

Hi Peter,

Am 17.11.2015 um 08:46 schrieb Peter Ujfalusi:

Quoted 16 lines

Hi,

Quoted 8 lines · Peter Ujfalusi

f93178291712 dmaengine: bcm2835-dma: Fix memory leak when stopping a
running transfer

Fixed the memleak, but introduced another issue: the terminate_all callback might be called with interrupts disabled and the dma_free_coherent() is not allowed to be called when IRQs are disabled.
Convert the driver to use dma_pool_* for managing the list of control blocks for the transfer.

FWIW: the patch has been tested and verified on Raspbery Pi:
https://github.com/raspberrypi/linux/pull/1178#issuecomment-157026794
https://github.com/raspberrypi/linux/pull/1178#issuecomment-157030190

Quoted 26 lines

It needed some modification since the Raspberry Pi kernel have non upstreamed changes in bcm2835-dma driver (slave_sg support for example).

It would be great if this patch can make it to 4.4 as a fix.

Thanks,
P?ter

Quoted 18 lines

Fixes f93178291712 ("dmaengine: bcm2835-dma: Fix memory leak when stopping

a running transfer")

Signed-off-by Peter Ujfalusi <[email protected]>

---
Hi,

It was brought to my attention that the memleak fix broke the bcm2835 DMA. I did
not noticed the use of dma_free_coherent() in the driver when I did the memleak
fix.
Since the driver does leaking memory every time the audio is stopped, the other
option is to convert it to use DMA pool.
I do not have access the Raspberry Pi, so I can not test this patch but it compiles ;)
Can someone test this one out if it is working?

i tried to test it yesterday with dmatest until i noticed that the bcm2835 dmaengine didn't provide the necessary capabilities (DMA_MEMCPY).

Sorry, i'm not an expert here. Could you please provide at least one test scenario which should work with Linux 4.4-rc1 without any special hardware?

Do you know which bcm2835 drivers make use of the relevant code?

Btw if you want that somebody test the patch for you, then it's possible to mark it as request for testing "[PATCH RFT]".

f93178291712 dmaengine: bcm2835-dma: Fix memory leak when stopping a
running transfer

Fixed the memleak, but introduced another issue: the terminate_all callback might be called with interrupts disabled and the dma_free_coherent() is not allowed to be called when IRQs are disabled.
Convert the driver to use dma_pool_* for managing the list of control blocks for the transfer.

Fixes f93178291712 ("dmaengine: bcm2835-dma: Fix memory leak when stopping a running transfer")

Signed-off-by Peter Ujfalusi <[email protected]>

Quoted 9 lines

f93178291712 dmaengine: bcm2835-dma: Fix memory leak when stopping a
running transfer

Fixed the memleak, but introduced another issue: the terminate_all callback might be called with interrupts disabled and the dma_free_coherent() is not allowed to be called when IRQs are disabled.
Convert the driver to use dma_pool_* for managing the list of control blocks for the transfer.

Quoted 2 lines

Fixes f93178291712 ("dmaengine: bcm2835-dma: Fix memory leak when stopping a running transfer")

Signed-off-by Peter Ujfalusi <[email protected]>

Tested-by Matthias Reichl <[email protected]>

Thanks a lot, the fix seems to be working fine with mainline 4.4-rc1, 4.4-rc2 and the 4.3 Raspberry Pi tree.

On the RPi tree you can simply enable the rpi-dac overlay for testing, "dtoverlay=rpi-dac" in config.txt, it'll work without any additional hardware.

As Peter mentioned I extended his patch to the non-mainlined slave_sg code in the RPi tree. I've been using this during the last week and haven't noticed any issues.

For testing with 4.4-rc mainline I used the (quick-and-dirty) code below. It adds the missing PCM configuration to bcm2835-i2s, adds some DT fixes (so DT and bcm2835-i2s are similar to the RPi tree) and installs a card with a dummy-codec. You can then use aplay to test the DMA code path and don't need any additional hardware.

I've checked the I2S GPIOs on a RPi B+ with my scope, PCM-clock and frame-clock are fine and pcm-data-out also looks OK.

There's a small gotcha though: bcm2835-i2s hasn't been converted to the new clock driver yet and also can't coexist with it (overlapping iomem regions...). Fortunately, this can be solved by disabling the new clock driver in the devicetree.

Here are the steps I used for testing:

- revert commits 121432c7a02f and 94cb7f76caa0 to disable the new clock driver in DT
- apply the test-code patch below
- make bcm2835_defconfig
- enable bcm2835-dma and bcm2835-i2s in kernel config:

   CONFIG_SOUND=y
   CONFIG_SND=y
   CONFIG_SND_SOC=y
   CONFIG_SND_BCM2835_SOC_I2S=y
   CONFIG_DMADEVICES=y
   CONFIG_DMA_BCM2835=y

- (optionally) apply Peter's DMA patch
- on the RPi use aplay to play a 44.1kHz 16-bit stereo WAV (32bit and other sample rates will work, too)

so long,

Hias

---

arch/arm/boot/dts/bcm2835.dtsi +8 -3

index 2029394..c3a1cfc 100644
@@ -102,13 +102,13 @@
 
  		i2s: i2s at 7e203000 {
  			compatible = "brcm,bcm2835-i2s";
-			reg = <0x7e203000 0x20>,
-			      <0x7e101098 0x02>;
+			reg = <0x7e203000 0x24>,
+			      <0x7e101098 0x08>;
 
  			dmas = <&dma 2>,
  			       <&dma 3>;
  			dma-names = "tx", "rx";
-			status = "disabled";
+			status = "okay";
  		};
 
  		spi: spi at 7e204000 {

@@ -189,4 +189,9 @@
  			clock-frequency = <250000000>;
  		};
  	};
+
+	sound {
+		compatible = "brcm,i2s-dummy-card";
+		i2s-controller = <&i2s>;
+	};
  };

sound/soc/bcm/Makefile +2 -0

index bc816b7..d599b62 100644
@@ -1,5 +1,7 @@
  # BCM2835 Platform Support
  snd-soc-bcm2835-i2s-objs := bcm2835-i2s.o
+snd-soc-i2s-dummy-card-objs := i2s-dummy-card.o
 
  obj-$(CONFIG_SND_BCM2835_SOC_I2S) += snd-soc-bcm2835-i2s.o
+obj-$(CONFIG_SND_BCM2835_SOC_I2S) += snd-soc-i2s-dummy-card.o
 

sound/soc/bcm/bcm2835-i2s.c +21 -1

index 8c435be..b1ff172 100644
@@ -784,6 +784,24 @@ static const struct snd_soc_component_driver bcm2835_i2s_component = {
  	.name		= "bcm2835-i2s-comp",
  };
 
+static const struct snd_pcm_hardware bcm2835_pcm_hardware = {
+	.info			= SNDRV_PCM_INFO_INTERLEAVED |
+				  SNDRV_PCM_INFO_JOINT_DUPLEX,
+	.formats		= SNDRV_PCM_FMTBIT_S16_LE |
+				  SNDRV_PCM_FMTBIT_S32_LE,
+	.period_bytes_min	= 32,
+	.period_bytes_max	= SZ_64K - 4,
+	.periods_min		= 2,
+	.periods_max		= 255,
+	.buffer_bytes_max	= SZ_512K
+};
+
+static const struct snd_dmaengine_pcm_config bcm2835_dmaengine_pcm_config = {
+	.pcm_hardware = &bcm2835_pcm_hardware,
+	.prepare_slave_config = snd_dmaengine_pcm_prepare_slave_config,
+	.prealloc_buffer_size = SZ_1M,
+};
+
  static int bcm2835_i2s_probe(struct platform_device *pdev)
  {
  	struct bcm2835_i2s_dev *dev;

@@ -848,7 +866,9 @@ static int bcm2835_i2s_probe(struct platform_device *pdev)
  		return ret;
  	}
 
-	ret = devm_snd_dmaengine_pcm_register(&pdev->dev, NULL, 0);
+	ret = devm_snd_dmaengine_pcm_register(&pdev->dev,
+			&bcm2835_dmaengine_pcm_config,
+			SND_DMAENGINE_PCM_FLAG_COMPAT);
  	if (ret) {
  		dev_err(&pdev->dev, "Could not register PCM: %d\n", ret);
  		return ret;

sound/soc/bcm/i2s-dummy-card.c new file +87 -0

new file mode 100644
index 0000000..b3ad842
@@ -0,0 +1,87 @@
+/*
+ * Dummy card driver for testing bcm2835-i2s
+ *
+ * Author:	Matthias Reichl <[email protected]>
+ *		Copyright 2015
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include <sound/core.h>
+#include <sound/soc.h>
+
+static struct snd_soc_dai_link snd_i2s_dummy_card_dai[] = {
+{
+	.name		= "i2s-dummy",
+	.stream_name	= "i2s-dummy HiFi",
+	.codec_name	= "snd-soc-dummy",
+	.codec_dai_name	= "snd-soc-dummy-dai",
+	.dai_fmt	= SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
+				SND_SOC_DAIFMT_CBS_CFS,
+},
+};
+
+static struct snd_soc_card snd_i2s_dummy_card = {
+	.name		= "i2s_dummy_card",
+	.dai_link	= snd_i2s_dummy_card_dai,
+	.num_links	= ARRAY_SIZE(snd_i2s_dummy_card_dai),
+};
+
+static int snd_i2s_dummy_card_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+	struct device_node *i2s_node;
+
+	if (!pdev->dev.of_node) {
+		dev_err(&pdev->dev, "no of_node, exiting\n");
+		return -ENODEV;
+	}
+
+	i2s_node = of_parse_phandle(pdev->dev.of_node, "i2s-controller", 0);
+
+	if (!i2s_node) {
+		dev_err(&pdev->dev, "error getting i2s-controller, exiting\n");
+		return -ENODEV;
+	}
+
+	snd_i2s_dummy_card.dev = &pdev->dev;
+	snd_i2s_dummy_card_dai[0].cpu_of_node = i2s_node;
+	snd_i2s_dummy_card_dai[0].platform_of_node = i2s_node;
+
+	ret = devm_snd_soc_register_card(&pdev->dev, &snd_i2s_dummy_card);
+	if (ret)
+		dev_err(&pdev->dev, "snd_soc_register_card() failed: %d\n", ret);
+
+	return ret;
+}
+
+static const struct of_device_id snd_i2s_dummy_card_of_match[] = {
+	{ .compatible = "brcm,i2s-dummy-card", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, snd_i2s_dummy_card_of_match);
+
+static struct platform_driver snd_i2s_dummy_card_driver = {
+	.driver	= {
+		.name = "snd-i2s-dummy-card",
+		.of_match_table = snd_i2s_dummy_card_of_match,
+	},
+	.probe	= snd_i2s_dummy_card_probe,
+};
+
+module_platform_driver(snd_i2s_dummy_card_driver);
+
+MODULE_AUTHOR("Matthias Reichl <[email protected]>");
+MODULE_DESCRIPTION("ASoC dummy card driver for testing bcm2835-i2s");
+MODULE_LICENSE("GPL v2");

f93178291712 dmaengine: bcm2835-dma: Fix memory leak when stopping a
running transfer

Fixed the memleak, but introduced another issue: the terminate_all callback might be called with interrupts disabled and the dma_free_coherent() is not allowed to be called when IRQs are disabled.
Convert the driver to use dma_pool_* for managing the list of control blocks for the transfer.

Fixes f93178291712 ("dmaengine: bcm2835-dma: Fix memory leak when stopping a running transfer")

Signed-off-by Peter Ujfalusi <[email protected]>

f93178291712 dmaengine: bcm2835-dma: Fix memory leak when stopping a
running transfer

Fixed the memleak, but introduced another issue: the terminate_all callback might be called with interrupts disabled and the dma_free_coherent() is not allowed to be called when IRQs are disabled.
Convert the driver to use dma_pool_* for managing the list of control blocks for the transfer.