[PATCH 1/3] ASoC: atmel-pdc: Use managed DMA buffer allocation

[PATCH 1/3] ASoC: atmel-pdc: Use managed DMA buffer allocation

[Lars-Peter Clausen]

Instead of manually managing its DMA buffers using
dma_{alloc,free}_coherent() lets the sound core take care of this using
managed buffers.

On one hand this reduces the amount of boiler plate code, but the main
motivation for the change is to use the shared code where possible. This
makes it easier to argue about correctness and that the code does not
contain subtle bugs like data leakage or similar.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 sound/soc/atmel/atmel-pcm-pdc.c | 78 ++-------------------------------
 1 file changed, 4 insertions(+), 74 deletions(-)

diff --git a/sound/soc/atmel/atmel-pcm-pdc.c b/sound/soc/atmel/atmel-pcm-pdc.c
index 704f700013d3..3e7ea2021b46 100644
--- a/sound/soc/atmel/atmel-pcm-pdc.c
+++ b/sound/soc/atmel/atmel-pcm-pdc.c
@@ -34,86 +34,21 @@
 #include "atmel-pcm.h"
 
 
-static int atmel_pcm_preallocate_dma_buffer(struct snd_pcm *pcm,
-	int stream)
-{
-	struct snd_pcm_substream *substream = pcm->streams[stream].substream;
-	struct snd_dma_buffer *buf = &substream->dma_buffer;
-	size_t size = ATMEL_SSC_DMABUF_SIZE;
-
-	buf->dev.type = SNDRV_DMA_TYPE_DEV;
-	buf->dev.dev = pcm->card->dev;
-	buf->private_data = NULL;
-	buf->area = dma_alloc_coherent(pcm->card->dev, size,
-			&buf->addr, GFP_KERNEL);
-	pr_debug("atmel-pcm: alloc dma buffer: area=%p, addr=%p, size=%zu\n",
-			(void *)buf->area, (void *)(long)buf->addr, size);
-
-	if (!buf->area)
-		return -ENOMEM;
-
-	buf->bytes = size;
-	return 0;
-}
-
-static int atmel_pcm_mmap(struct snd_soc_component *component,
-			  struct snd_pcm_substream *substream,
-			  struct vm_area_struct *vma)
-{
-	return remap_pfn_range(vma, vma->vm_start,
-		       substream->dma_buffer.addr >> PAGE_SHIFT,
-		       vma->vm_end - vma->vm_start, vma->vm_page_prot);
-}
-
 static int atmel_pcm_new(struct snd_soc_component *component,
 			 struct snd_soc_pcm_runtime *rtd)
 {
 	struct snd_card *card = rtd->card->snd_card;
-	struct snd_pcm *pcm = rtd->pcm;
 	int ret;
 
 	ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32));
 	if (ret)
 		return ret;
 
-	if (pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream) {
-		pr_debug("atmel-pcm: allocating PCM playback DMA buffer\n");
-		ret = atmel_pcm_preallocate_dma_buffer(pcm,
-			SNDRV_PCM_STREAM_PLAYBACK);
-		if (ret)
-			goto out;
-	}
+	snd_pcm_set_managed_buffer_all(rtd->pcm, SNDRV_DMA_TYPE_DEV,
+				       card->dev, ATMEL_SSC_DMABUF_SIZE,
+				       ATMEL_SSC_DMABUF_SIZE);
 
-	if (pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream) {
-		pr_debug("atmel-pcm: allocating PCM capture DMA buffer\n");
-		ret = atmel_pcm_preallocate_dma_buffer(pcm,
-			SNDRV_PCM_STREAM_CAPTURE);
-		if (ret)
-			goto out;
-	}
- out:
-	return ret;
-}
-
-static void atmel_pcm_free(struct snd_soc_component *component,
-			   struct snd_pcm *pcm)
-{
-	struct snd_pcm_substream *substream;
-	struct snd_dma_buffer *buf;
-	int stream;
-
-	for (stream = 0; stream < 2; stream++) {
-		substream = pcm->streams[stream].substream;
-		if (!substream)
-			continue;
-
-		buf = &substream->dma_buffer;
-		if (!buf->area)
-			continue;
-		dma_free_coherent(pcm->card->dev, buf->bytes,
-				  buf->area, buf->addr);
-		buf->area = NULL;
-	}
+	return 0;
 }
 
 /*--------------------------------------------------------------------------*\
@@ -210,9 +145,6 @@ static int atmel_pcm_hw_params(struct snd_soc_component *component,
 	/* this may get called several times by oss emulation
 	 * with different params */
 
-	snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
-	runtime->dma_bytes = params_buffer_bytes(params);
-
 	prtd->params = snd_soc_dai_get_dma_data(asoc_rtd_to_cpu(rtd, 0), substream);
 	prtd->params->dma_intr_handler = atmel_pcm_dma_irq;
 
@@ -384,9 +316,7 @@ static const struct snd_soc_component_driver atmel_soc_platform = {
 	.prepare	= atmel_pcm_prepare,
 	.trigger	= atmel_pcm_trigger,
 	.pointer	= atmel_pcm_pointer,
-	.mmap		= atmel_pcm_mmap,
 	.pcm_construct	= atmel_pcm_new,
-	.pcm_destruct	= atmel_pcm_free,
 };
 
 int atmel_pcm_pdc_platform_register(struct device *dev)
-- 
2.20.1

[PATCH 2/3] ASoC: bcm: cygnus: Use managed DMA buffer allocation

[Lars-Peter Clausen]

Instead of manually managing its DMA buffers using
dma_{alloc,free}_coherent() lets the sound core take care of this using
managed buffers.

On one hand this reduces the amount of boiler plate code, but the main
motivation for the change is to use the shared code where possible. This
makes it easier to argue about correctness and that the code does not
contain subtle bugs like data leakage or similar.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 sound/soc/bcm/cygnus-pcm.c | 107 ++-----------------------------------
 1 file changed, 3 insertions(+), 104 deletions(-)

diff --git a/sound/soc/bcm/cygnus-pcm.c b/sound/soc/bcm/cygnus-pcm.c
index 7ad07239f99c..56b71b965624 100644
--- a/sound/soc/bcm/cygnus-pcm.c
+++ b/sound/soc/bcm/cygnus-pcm.c
@@ -636,36 +636,6 @@ static int cygnus_pcm_close(struct snd_soc_component *component,
 	return 0;
 }
 
-static int cygnus_pcm_hw_params(struct snd_soc_component *component,
-				struct snd_pcm_substream *substream,
-				struct snd_pcm_hw_params *params)
-{
-	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
-	struct snd_pcm_runtime *runtime = substream->runtime;
-	struct cygnus_aio_port *aio;
-
-	aio = cygnus_dai_get_dma_data(substream);
-	dev_dbg(asoc_rtd_to_cpu(rtd, 0)->dev, "%s  port %d\n", __func__, aio->portnum);
-
-	snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
-	runtime->dma_bytes = params_buffer_bytes(params);
-
-	return 0;
-}
-
-static int cygnus_pcm_hw_free(struct snd_soc_component *component,
-			      struct snd_pcm_substream *substream)
-{
-	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
-	struct cygnus_aio_port *aio;
-
-	aio = cygnus_dai_get_dma_data(substream);
-	dev_dbg(asoc_rtd_to_cpu(rtd, 0)->dev, "%s  port %d\n", __func__, aio->portnum);
-
-	snd_pcm_set_runtime_buffer(substream, NULL);
-	return 0;
-}
-
 static int cygnus_pcm_prepare(struct snd_soc_component *component,
 			      struct snd_pcm_substream *substream)
 {
@@ -730,87 +700,19 @@ static snd_pcm_uframes_t cygnus_pcm_pointer(struct snd_soc_component *component,
 	return bytes_to_frames(substream->runtime, res);
 }
 
-static int cygnus_pcm_preallocate_dma_buffer(struct snd_pcm *pcm, int stream)
-{
-	struct snd_pcm_substream *substream = pcm->streams[stream].substream;
-	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
-	struct snd_dma_buffer *buf = &substream->dma_buffer;
-	size_t size;
-
-	size = cygnus_pcm_hw.buffer_bytes_max;
-
-	buf->dev.type = SNDRV_DMA_TYPE_DEV;
-	buf->dev.dev = pcm->card->dev;
-	buf->private_data = NULL;
-	buf->area = dma_alloc_coherent(pcm->card->dev, size,
-			&buf->addr, GFP_KERNEL);
-
-	dev_dbg(asoc_rtd_to_cpu(rtd, 0)->dev, "%s: size 0x%zx @ %pK\n",
-				__func__, size, buf->area);
-
-	if (!buf->area) {
-		dev_err(asoc_rtd_to_cpu(rtd, 0)->dev, "%s: dma_alloc failed\n", __func__);
-		return -ENOMEM;
-	}
-	buf->bytes = size;
-
-	return 0;
-}
-
-static void cygnus_dma_free_dma_buffers(struct snd_soc_component *component,
-					struct snd_pcm *pcm)
-{
-	struct snd_pcm_substream *substream;
-	struct snd_dma_buffer *buf;
-
-	substream = pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream;
-	if (substream) {
-		buf = &substream->dma_buffer;
-		if (buf->area) {
-			dma_free_coherent(pcm->card->dev, buf->bytes,
-				buf->area, buf->addr);
-			buf->area = NULL;
-		}
-	}
-
-	substream = pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream;
-	if (substream) {
-		buf = &substream->dma_buffer;
-		if (buf->area) {
-			dma_free_coherent(pcm->card->dev, buf->bytes,
-				buf->area, buf->addr);
-			buf->area = NULL;
-		}
-	}
-}
-
 static int cygnus_dma_new(struct snd_soc_component *component,
 			  struct snd_soc_pcm_runtime *rtd)
 {
+	size_t size = cygnus_pcm_hw.buffer_bytes_max;
 	struct snd_card *card = rtd->card->snd_card;
-	struct snd_pcm *pcm = rtd->pcm;
-	int ret;
 
 	if (!card->dev->dma_mask)
 		card->dev->dma_mask = &cygnus_dma_dmamask;
 	if (!card->dev->coherent_dma_mask)
 		card->dev->coherent_dma_mask = DMA_BIT_MASK(32);
 
-	if (pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream) {
-		ret = cygnus_pcm_preallocate_dma_buffer(pcm,
-				SNDRV_PCM_STREAM_PLAYBACK);
-		if (ret)
-			return ret;
-	}
-
-	if (pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream) {
-		ret = cygnus_pcm_preallocate_dma_buffer(pcm,
-				SNDRV_PCM_STREAM_CAPTURE);
-		if (ret) {
-			cygnus_dma_free_dma_buffers(component, pcm);
-			return ret;
-		}
-	}
+	snd_pcm_set_managed_buffer_all(rtd->pcm, SNDRV_DMA_TYPE_DEV,
+				       card->dev, size, size);
 
 	return 0;
 }
@@ -818,13 +720,10 @@ static int cygnus_dma_new(struct snd_soc_component *component,
 static struct snd_soc_component_driver cygnus_soc_platform = {
 	.open		= cygnus_pcm_open,
 	.close		= cygnus_pcm_close,
-	.hw_params	= cygnus_pcm_hw_params,
-	.hw_free	= cygnus_pcm_hw_free,
 	.prepare	= cygnus_pcm_prepare,
 	.trigger	= cygnus_pcm_trigger,
 	.pointer	= cygnus_pcm_pointer,
 	.pcm_construct	= cygnus_dma_new,
-	.pcm_destruct	= cygnus_dma_free_dma_buffers,
 };
 
 int cygnus_soc_platform_register(struct device *dev,
-- 
2.20.1

[PATCH 3/3] ASoC: kirkwood: Use managed DMA buffer allocation

[Lars-Peter Clausen]

Instead of manually managing its DMA buffers using
dma_{alloc,free}_coherent() lets the sound core take care of this using
managed buffers.

On one hand this reduces the amount of boiler plate code, but the main
motivation for the change is to use the shared code where possible. This
makes it easier to argue about correctness and that the code does not
contain subtle bugs like data leakage or similar.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 sound/soc/kirkwood/kirkwood-dma.c | 79 ++-----------------------------
 1 file changed, 3 insertions(+), 76 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-dma.c b/sound/soc/kirkwood/kirkwood-dma.c
index e037826b2451..c2a5933bfcfc 100644
--- a/sound/soc/kirkwood/kirkwood-dma.c
+++ b/sound/soc/kirkwood/kirkwood-dma.c
@@ -182,25 +182,6 @@ static int kirkwood_dma_close(struct snd_soc_component *component,
 	return 0;
 }
 
-static int kirkwood_dma_hw_params(struct snd_soc_component *component,
-				  struct snd_pcm_substream *substream,
-				  struct snd_pcm_hw_params *params)
-{
-	struct snd_pcm_runtime *runtime = substream->runtime;
-
-	snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
-	runtime->dma_bytes = params_buffer_bytes(params);
-
-	return 0;
-}
-
-static int kirkwood_dma_hw_free(struct snd_soc_component *component,
-				struct snd_pcm_substream *substream)
-{
-	snd_pcm_set_runtime_buffer(substream, NULL);
-	return 0;
-}
-
 static int kirkwood_dma_prepare(struct snd_soc_component *component,
 				struct snd_pcm_substream *substream)
 {
@@ -244,82 +225,28 @@ static snd_pcm_uframes_t kirkwood_dma_pointer(
 	return count;
 }
 
-static int kirkwood_dma_preallocate_dma_buffer(struct snd_pcm *pcm,
-		int stream)
-{
-	struct snd_pcm_substream *substream = pcm->streams[stream].substream;
-	struct snd_dma_buffer *buf = &substream->dma_buffer;
-	size_t size = kirkwood_dma_snd_hw.buffer_bytes_max;
-
-	buf->dev.type = SNDRV_DMA_TYPE_DEV;
-	buf->dev.dev = pcm->card->dev;
-	buf->area = dma_alloc_coherent(pcm->card->dev, size,
-			&buf->addr, GFP_KERNEL);
-	if (!buf->area)
-		return -ENOMEM;
-	buf->bytes = size;
-	buf->private_data = NULL;
-
-	return 0;
-}
-
 static int kirkwood_dma_new(struct snd_soc_component *component,
 			    struct snd_soc_pcm_runtime *rtd)
 {
+	size_t size = kirkwood_dma_snd_hw.buffer_bytes_max;
 	struct snd_card *card = rtd->card->snd_card;
-	struct snd_pcm *pcm = rtd->pcm;
 	int ret;
 
 	ret = dma_coerce_mask_and_coherent(card->dev, DMA_BIT_MASK(32));
 	if (ret)
 		return ret;
 
-	if (pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream) {
-		ret = kirkwood_dma_preallocate_dma_buffer(pcm,
-				SNDRV_PCM_STREAM_PLAYBACK);
-		if (ret)
-			return ret;
-	}
-
-	if (pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream) {
-		ret = kirkwood_dma_preallocate_dma_buffer(pcm,
-				SNDRV_PCM_STREAM_CAPTURE);
-		if (ret)
-			return ret;
-	}
+	snd_pcm_set_managed_buffer_all(rtd->pcm, SNDRV_DMA_TYPE_DEV,
+				       card->dev, size, size);
 
 	return 0;
 }
 
-static void kirkwood_dma_free_dma_buffers(struct snd_soc_component *component,
-					  struct snd_pcm *pcm)
-{
-	struct snd_pcm_substream *substream;
-	struct snd_dma_buffer *buf;
-	int stream;
-
-	for (stream = 0; stream < 2; stream++) {
-		substream = pcm->streams[stream].substream;
-		if (!substream)
-			continue;
-		buf = &substream->dma_buffer;
-		if (!buf->area)
-			continue;
-
-		dma_free_coherent(pcm->card->dev, buf->bytes,
-				buf->area, buf->addr);
-		buf->area = NULL;
-	}
-}
-
 const struct snd_soc_component_driver kirkwood_soc_component = {
 	.name		= DRV_NAME,
 	.open		= kirkwood_dma_open,
 	.close		= kirkwood_dma_close,
-	.hw_params	= kirkwood_dma_hw_params,
-	.hw_free	= kirkwood_dma_hw_free,
 	.prepare	= kirkwood_dma_prepare,
 	.pointer	= kirkwood_dma_pointer,
 	.pcm_construct	= kirkwood_dma_new,
-	.pcm_destruct	= kirkwood_dma_free_dma_buffers,
 };
-- 
2.20.1

Re: [PATCH 1/3] ASoC: atmel-pdc: Use managed DMA buffer allocation

[Codrin.Ciubotariu]

On 06.01.2021 15:36, Lars-Peter Clausen wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Instead of manually managing its DMA buffers using
> dma_{alloc,free}_coherent() lets the sound core take care of this using
> managed buffers.
> 
> On one hand this reduces the amount of boiler plate code, but the main
> motivation for the change is to use the shared code where possible. This
> makes it easier to argue about correctness and that the code does not
> contain subtle bugs like data leakage or similar.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>

Thanks!

Re: [PATCH 1/3] ASoC: atmel-pdc: Use managed DMA buffer allocation

[Mark Brown]

On Wed, 6 Jan 2021 14:36:48 +0100, Lars-Peter Clausen wrote:
> Instead of manually managing its DMA buffers using
> dma_{alloc,free}_coherent() lets the sound core take care of this using
> managed buffers.
> 
> On one hand this reduces the amount of boiler plate code, but the main
> motivation for the change is to use the shared code where possible. This
> makes it easier to argue about correctness and that the code does not
> contain subtle bugs like data leakage or similar.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/3] ASoC: atmel-pdc: Use managed DMA buffer allocation
      commit: 22eee4d3efe370fedf71ee6a9e4dead3f32ad461
[2/3] ASoC: bcm: cygnus: Use managed DMA buffer allocation
      commit: 5ac813c83483e97a13b59aab34b89cebf9d5dcb8
[3/3] ASoC: kirkwood: Use managed DMA buffer allocation
      commit: b3c0ae75f5d3efa40174230b8c9c01848e03d4d0

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark