[PATCH 0/4] Fix potential issues with x86 PCM SG-buffer
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(-)
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;
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)
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 */
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);
participants (1)
-
Takashi Iwai