[PATCH 0/4] Fix potential issues with x86 PCM SG-buffer

[PATCH 0/4] Fix potential issues with x86 PCM SG-buffer

[Takashi Iwai]

Hi,

it turned out that x86 SG-pages that are used for PCM buffers aren't
safe on x86 in all cases, and we need some limitation and workarounds
for non-standard configurations.  This patch set is an attempt to
paper over it in a minimalistic way.


Takashi

===

Takashi Iwai (4):
  ALSA: pcm: Use dma_mmap_coherent() on x86, too
  ALSA: memalloc: Initialize all fields of snd_dma_buffer properly
  ALSA: memalloc: Make SG-buffer helper usable for continuous buffer,
    too
  ALSA: pcm: Use SG-buffer only when direct DMA is available

 include/sound/memalloc.h |  9 ++++++++-
 sound/core/memalloc.c    |  7 +++----
 sound/core/pcm_memory.c  | 13 +++++++++++++
 sound/core/pcm_native.c  |  2 --
 sound/core/sgbuf.c       |  3 +++
 5 files changed, 27 insertions(+), 7 deletions(-)

-- 
2.16.4

[PATCH 1/4] ALSA: pcm: Use dma_mmap_coherent() on x86, too

[Takashi Iwai]

We avoided the explicit use of dma_mmap_coherent() on x86 because of a
spurious warning in x86 APT code in the past.  However, this blindly
assumes that the pages allocated via dma_alloc_coherent() on x86 are
the ones convertible via virt_to_page() (that is used in the default
mmap handler), and it's no longer true; with the indirect DMA ops,
this can be handled differently.  The only certain way for doing mmap
such pages is the dma_mmap_coherent(), and the warning seems already
gone in the recent code, so let's use it consistently.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm_native.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index 9630d2523948..1e51c849d3d4 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -3713,7 +3713,6 @@ int snd_pcm_lib_default_mmap(struct snd_pcm_substream *substream,
 				area->vm_end - area->vm_start, area->vm_page_prot);
 	}
 #endif /* CONFIG_GENERIC_ALLOCATOR */
-#ifndef CONFIG_X86 /* for avoiding warnings arch/x86/mm/pat.c */
 	if (IS_ENABLED(CONFIG_HAS_DMA) && !substream->ops->page &&
 	    (substream->dma_buffer.dev.type == SNDRV_DMA_TYPE_DEV ||
 	     substream->dma_buffer.dev.type == SNDRV_DMA_TYPE_DEV_UC))
@@ -3722,7 +3721,6 @@ int snd_pcm_lib_default_mmap(struct snd_pcm_substream *substream,
 					 substream->runtime->dma_area,
 					 substream->runtime->dma_addr,
 					 substream->runtime->dma_bytes);
-#endif /* CONFIG_X86 */
 	/* mmap with fault handler */
 	area->vm_ops = &snd_pcm_vm_ops_data_fault;
 	return 0;
-- 
2.16.4

[PATCH 2/4] ALSA: memalloc: Initialize all fields of snd_dma_buffer properly

[Takashi Iwai]

Some fields in snd_dma_buffer aren't touched in snd_dma_alloc_pages()
and might be left uninitialized.  Let's clear all fields properly, so
that we can use a NULL check (e.g. dmab->private_data) as conditional
in a later patch.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/memalloc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c
index bea46ed157a6..d8f28d030985 100644
--- a/sound/core/memalloc.c
+++ b/sound/core/memalloc.c
@@ -135,16 +135,17 @@ int snd_dma_alloc_pages(int type, struct device *device, size_t size,
 	dmab->dev.type = type;
 	dmab->dev.dev = device;
 	dmab->bytes = 0;
+	dmab->area = NULL;
+	dmab->addr = 0;
+	dmab->private_data = NULL;
 	switch (type) {
 	case SNDRV_DMA_TYPE_CONTINUOUS:
 		gfp = snd_mem_get_gfp_flags(device, GFP_KERNEL);
 		dmab->area = alloc_pages_exact(size, gfp);
-		dmab->addr = 0;
 		break;
 	case SNDRV_DMA_TYPE_VMALLOC:
 		gfp = snd_mem_get_gfp_flags(device, GFP_KERNEL | __GFP_HIGHMEM);
 		dmab->area = __vmalloc(size, gfp);
-		dmab->addr = 0;
 		break;
 #ifdef CONFIG_HAS_DMA
 #ifdef CONFIG_GENERIC_ALLOCATOR
@@ -171,8 +172,6 @@ int snd_dma_alloc_pages(int type, struct device *device, size_t size,
 #endif
 	default:
 		pr_err("snd-malloc: invalid device type %d\n", type);
-		dmab->area = NULL;
-		dmab->addr = 0;
 		return -ENXIO;
 	}
 	if (! dmab->area)
-- 
2.16.4

[PATCH 3/4] ALSA: memalloc: Make SG-buffer helper usable for continuous buffer, too

[Takashi Iwai]

We have a few helper functions for making the access to the buffer
address easier on SG-buffer.  Those are specific to the buffer that is
allocated with SG-buffer type, and it makes hard to use both SG and
non-SG buffers in the same code.

This patch adds a few simple checks and lets the helpers to deal with
both SG- and continuous buffers gracefully.  It's a preliminary step
for the upcoming patch that mimics the buffer type on the fly.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/memalloc.h | 9 ++++++++-
 sound/core/sgbuf.c       | 3 +++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/sound/memalloc.h b/include/sound/memalloc.h
index 3b47832b1c1f..5daa937684a4 100644
--- a/include/sound/memalloc.h
+++ b/include/sound/memalloc.h
@@ -94,7 +94,11 @@ static inline dma_addr_t snd_sgbuf_get_addr(struct snd_dma_buffer *dmab,
 					   size_t offset)
 {
 	struct snd_sg_buf *sgbuf = dmab->private_data;
-	dma_addr_t addr = sgbuf->table[offset >> PAGE_SHIFT].addr;
+	dma_addr_t addr;
+
+	if (!sgbuf)
+		return dmab->addr + offset;
+	addr = sgbuf->table[offset >> PAGE_SHIFT].addr;
 	addr &= ~((dma_addr_t)PAGE_SIZE - 1);
 	return addr + offset % PAGE_SIZE;
 }
@@ -106,6 +110,9 @@ static inline void *snd_sgbuf_get_ptr(struct snd_dma_buffer *dmab,
 				     size_t offset)
 {
 	struct snd_sg_buf *sgbuf = dmab->private_data;
+
+	if (!sgbuf)
+		return dmab->area + offset;
 	return sgbuf->table[offset >> PAGE_SHIFT].buf + offset % PAGE_SIZE;
 }
 
diff --git a/sound/core/sgbuf.c b/sound/core/sgbuf.c
index c42217e2dd19..29ddb76187e5 100644
--- a/sound/core/sgbuf.c
+++ b/sound/core/sgbuf.c
@@ -142,6 +142,9 @@ unsigned int snd_sgbuf_get_chunk_size(struct snd_dma_buffer *dmab,
 	struct snd_sg_buf *sg = dmab->private_data;
 	unsigned int start, end, pg;
 
+	if (!sg)
+		return size;
+
 	start = ofs >> PAGE_SHIFT;
 	end = (ofs + size - 1) >> PAGE_SHIFT;
 	/* check page continuity */
-- 
2.16.4

[PATCH 4/4] ALSA: pcm: Use SG-buffer only when direct DMA is available

[Takashi Iwai]

The DMA-coherent SG-buffer is tricky to use, as it does need the
mapping.  It used to work stably on x86 over years (and that's why we
had enabled SG-buffer on solely x86) with the default mmap handler and
vmap(), but our luck seems no forever success.  The chance of breakage
is high when the special DMA handling is introduced in the arch side.

In this patch, we change the buffer allocation to use the SG-buffer
only when the device in question is with the direct DMA.  It's a bit
hackish, but it's currently the only condition that may work (more or
less) reliably with the default mmap and vmap() for mapping the pages
that are deduced via virt_to_page().

In theory, we can apply the similar hack in the sound/core memory
allocation helper, too; but it's used by SOF for allocating SG pages
without re-mapping via vmap() or mmap, and it's fine to use it in that
way, so let's keep it and adds the workaround in PCM side.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/pcm_memory.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/sound/core/pcm_memory.c b/sound/core/pcm_memory.c
index 860935e3aea4..8326d16d3596 100644
--- a/sound/core/pcm_memory.c
+++ b/sound/core/pcm_memory.c
@@ -11,6 +11,7 @@
 #include <linux/moduleparam.h>
 #include <linux/vmalloc.h>
 #include <linux/export.h>
+#include <linux/dma-mapping.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/info.h>
@@ -39,6 +40,18 @@ static int do_alloc_pages(struct snd_card *card, int type, struct device *dev,
 	if (max_alloc_per_card &&
 	    card->total_pcm_alloc_bytes + size > max_alloc_per_card)
 		return -ENOMEM;
+
+	if (IS_ENABLED(CONFIG_SND_DMA_SGBUF) &&
+	    (type == SNDRV_DMA_TYPE_DEV_SG || type == SNDRV_DMA_TYPE_DEV_UC_SG) &&
+	    !dma_is_direct(get_dma_ops(dev))) {
+		/* mutate to continuous page allocation */
+		dev_dbg(dev, "Use continuous page allocator\n");
+		if (type == SNDRV_DMA_TYPE_DEV_SG)
+			type = SNDRV_DMA_TYPE_DEV;
+		else
+			type = SNDRV_DMA_TYPE_DEV_UC;
+	}
+
 	err = snd_dma_alloc_pages(type, dev, size, dmab);
 	if (!err) {
 		mutex_lock(&card->memory_mutex);
-- 
2.16.4