[PATCH 0/5] ALSA: Drop hackish GFP giveaway for CONTINUOUS pages

[PATCH 0/5] ALSA: Drop hackish GFP giveaway for CONTINUOUS pages

[Takashi Iwai]

Hi,

this is a series of cleanup patches for dropping the current hackish
way of passing the GFP_* flags for CONTINOUS and VMALLOC memory
allocations.  There are only three users for this legacy feature, and
all of them seem superfluous.  And, if any driver requires the memory
restriction in future, it can now pass the proper device pointer for
specifying the DMA mask.


Takashi

===

Takashi Iwai (5):
  ALSA: vx: Drop superfluous GFP setup
  ALSA: pdaudiocf: Drop superfluous GFP setup
  ASoC: Intel: sst: Switch to standard device pages
  ALSA: memalloc: Drop special handling of GFP for CONTINUOUS allocation
  ALSA: doc: Drop snd_dma_continuous_data() usages

 .../kernel-api/writing-an-alsa-driver.rst     |  21 ++--
 include/sound/memalloc.h                      |   3 -
 sound/core/memalloc.c                         | 113 ++++++++----------
 sound/drivers/vx/vx_pcm.c                     |   3 +-
 sound/pcmcia/pdaudiocf/pdaudiocf_pcm.c        |   3 +-
 sound/soc/intel/atom/sst-mfld-platform-pcm.c  |   7 +-
 6 files changed, 61 insertions(+), 89 deletions(-)

-- 
2.35.3

[PATCH 1/5] ALSA: vx: Drop superfluous GFP setup

[Takashi Iwai]

The extra setup with GFP_DMA32 is superfluous for this driver.  The
whole operation is a simple copy loop, and there is no memory address
restriction at all.  Drop the useless GFP setup.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/drivers/vx/vx_pcm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/drivers/vx/vx_pcm.c b/sound/drivers/vx/vx_pcm.c
index 3924f5283745..ceaeb257003b 100644
--- a/sound/drivers/vx/vx_pcm.c
+++ b/sound/drivers/vx/vx_pcm.c
@@ -1215,8 +1215,7 @@ int snd_vx_pcm_new(struct vx_core *chip)
 		if (ins)
 			snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &vx_pcm_capture_ops);
 		snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_VMALLOC,
-					       snd_dma_continuous_data(GFP_KERNEL | GFP_DMA32),
-					       0, 0);
+					       NULL, 0, 0);
 
 		pcm->private_data = chip;
 		pcm->private_free = snd_vx_pcm_free;
-- 
2.35.3

[PATCH 2/5] ALSA: pdaudiocf: Drop superfluous GFP setup

[Takashi Iwai]

The extra setup with GFP_DMA32 is superfluous for this driver.  The
whole operation is a simple copy loop, and there is no memory address
restriction at all.  Drop the useless GFP setup.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/pcmcia/pdaudiocf/pdaudiocf_pcm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/pcmcia/pdaudiocf/pdaudiocf_pcm.c b/sound/pcmcia/pdaudiocf/pdaudiocf_pcm.c
index dfc4295b69c4..aaa82ec36540 100644
--- a/sound/pcmcia/pdaudiocf/pdaudiocf_pcm.c
+++ b/sound/pcmcia/pdaudiocf/pdaudiocf_pcm.c
@@ -257,8 +257,7 @@ int snd_pdacf_pcm_new(struct snd_pdacf *chip)
 		return err;
 		
 	snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &pdacf_pcm_capture_ops);
-	snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_VMALLOC,
-				       snd_dma_continuous_data(GFP_KERNEL | GFP_DMA32),
+	snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_VMALLOC, NULL,
 				       0, 0);
 
 	pcm->private_data = chip;
-- 
2.35.3

[PATCH 3/5] ASoC: Intel: sst: Switch to standard device pages

[Takashi Iwai]

ASoC Atom SST driver is using the continuous RAM pages with GFP_DMA
flag for its PCM buffer, but this should work fine with the standard
DMA pages.  As a part of cleanup work, this patch replaces the buffer
allocation to the standard device pages with SNDRV_DMA_TYPE_DEV.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/intel/atom/sst-mfld-platform-pcm.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
index a56dd48c045f..c75616a5fd0a 100644
--- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
+++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
@@ -676,10 +676,9 @@ static int sst_soc_pcm_new(struct snd_soc_component *component,
 
 	if (dai->driver->playback.channels_min ||
 			dai->driver->capture.channels_min) {
-		snd_pcm_set_managed_buffer_all(pcm,
-			SNDRV_DMA_TYPE_CONTINUOUS,
-			snd_dma_continuous_data(GFP_DMA),
-			SST_MIN_BUFFER, SST_MAX_BUFFER);
+		snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_DEV,
+					       pcm->card->dev,
+					       SST_MIN_BUFFER, SST_MAX_BUFFER);
 	}
 	return 0;
 }
-- 
2.35.3

[PATCH 4/5] ALSA: memalloc: Drop special handling of GFP for CONTINUOUS allocation

[Takashi Iwai]

Now that all users of snd_dma_continuous_data() is gone, let's drop
this ugly (and dangerous) way.

After this commit, SNDRV_DMA_TYPE_CONTINUOUS may take the standard
device pointer instead of the hacked pointer by the macro above, and
the memalloc core refers to the coherent_dma_mask of the given
device like other SNDRV_DMA_TYPE.  It's still allowed to pass NULL
there, and in that case, the allocation is performed always in the
normal zone.

For SNDRV_DMA_TYPE_VMALLOC, the device pointer is simply ignored.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/memalloc.h |   3 --
 sound/core/memalloc.c    | 113 +++++++++++++++++----------------------
 2 files changed, 48 insertions(+), 68 deletions(-)

diff --git a/include/sound/memalloc.h b/include/sound/memalloc.h
index 8d79cebf95f3..43d524580bd2 100644
--- a/include/sound/memalloc.h
+++ b/include/sound/memalloc.h
@@ -26,9 +26,6 @@ struct snd_dma_device {
 	struct device *dev;		/* generic device */
 };
 
-#define snd_dma_continuous_data(x)	((struct device *)(__force unsigned long)(x))
-
-
 /*
  * buffer types
  */
diff --git a/sound/core/memalloc.c b/sound/core/memalloc.c
index b665ac66ccbe..39561faef6e9 100644
--- a/sound/core/memalloc.c
+++ b/sound/core/memalloc.c
@@ -18,25 +18,18 @@
 #include <sound/memalloc.h>
 #include "memalloc_local.h"
 
+#define DEFAULT_GFP \
+	(GFP_KERNEL | \
+	 __GFP_COMP |    /* compound page lets parts be mapped */ \
+	 __GFP_NORETRY | /* don't trigger OOM-killer */ \
+	 __GFP_NOWARN)   /* no stack trace print - this call is non-critical */
+
 static const struct snd_malloc_ops *snd_dma_get_ops(struct snd_dma_buffer *dmab);
 
 #ifdef CONFIG_SND_DMA_SGBUF
-static void *do_alloc_fallback_pages(struct device *dev, size_t size,
-				     dma_addr_t *addr, bool wc);
-static void do_free_fallback_pages(void *p, size_t size, bool wc);
 static void *snd_dma_sg_fallback_alloc(struct snd_dma_buffer *dmab, size_t size);
 #endif
 
-/* a cast to gfp flag from the dev pointer; for CONTINUOUS and VMALLOC types */
-static inline gfp_t snd_mem_get_gfp_flags(const struct snd_dma_buffer *dmab,
-					  gfp_t default_gfp)
-{
-	if (!dmab->dev.dev)
-		return default_gfp;
-	else
-		return (__force gfp_t)(unsigned long)dmab->dev.dev;
-}
-
 static void *__snd_dma_alloc_pages(struct snd_dma_buffer *dmab, size_t size)
 {
 	const struct snd_malloc_ops *ops = snd_dma_get_ops(dmab);
@@ -284,24 +277,54 @@ EXPORT_SYMBOL(snd_sgbuf_get_chunk_size);
 /*
  * Continuous pages allocator
  */
-static void *do_alloc_pages(size_t size, dma_addr_t *addr, gfp_t gfp)
+static void *do_alloc_pages(struct device *dev, size_t size, dma_addr_t *addr,
+			    bool wc)
 {
-	void *p = alloc_pages_exact(size, gfp);
+	void *p;
+	gfp_t gfp = GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN;
 
-	if (p)
-		*addr = page_to_phys(virt_to_page(p));
+ again:
+	p = alloc_pages_exact(size, gfp);
+	if (!p)
+		return NULL;
+	*addr = page_to_phys(virt_to_page(p));
+	if (!dev)
+		return p;
+	if ((*addr + size - 1) & ~dev->coherent_dma_mask) {
+		if (IS_ENABLED(CONFIG_ZONE_DMA32) && !(gfp & GFP_DMA32)) {
+			gfp |= GFP_DMA32;
+			goto again;
+		}
+		if (IS_ENABLED(CONFIG_ZONE_DMA) && !(gfp & GFP_DMA)) {
+			gfp = (gfp & ~GFP_DMA32) | GFP_DMA;
+			goto again;
+		}
+	}
+#ifdef CONFIG_X86
+	if (wc)
+		set_memory_wc((unsigned long)(p), size >> PAGE_SHIFT);
+#endif
 	return p;
 }
 
+static void do_free_pages(void *p, size_t size, bool wc)
+{
+#ifdef CONFIG_X86
+	if (wc)
+		set_memory_wb((unsigned long)(p), size >> PAGE_SHIFT);
+#endif
+	free_pages_exact(p, size);
+}
+
+
 static void *snd_dma_continuous_alloc(struct snd_dma_buffer *dmab, size_t size)
 {
-	return do_alloc_pages(size, &dmab->addr,
-			      snd_mem_get_gfp_flags(dmab, GFP_KERNEL));
+	return do_alloc_pages(dmab->dev.dev, size, &dmab->addr, false);
 }
 
 static void snd_dma_continuous_free(struct snd_dma_buffer *dmab)
 {
-	free_pages_exact(dmab->area, dmab->bytes);
+	do_free_pages(dmab->area, dmab->bytes, false);
 }
 
 static int snd_dma_continuous_mmap(struct snd_dma_buffer *dmab,
@@ -324,9 +347,7 @@ static const struct snd_malloc_ops snd_dma_continuous_ops = {
  */
 static void *snd_dma_vmalloc_alloc(struct snd_dma_buffer *dmab, size_t size)
 {
-	gfp_t gfp = snd_mem_get_gfp_flags(dmab, GFP_KERNEL | __GFP_HIGHMEM);
-
-	return __vmalloc(size, gfp);
+	return vmalloc(size);
 }
 
 static void snd_dma_vmalloc_free(struct snd_dma_buffer *dmab)
@@ -440,12 +461,6 @@ static const struct snd_malloc_ops snd_dma_iram_ops = {
 };
 #endif /* CONFIG_GENERIC_ALLOCATOR */
 
-#define DEFAULT_GFP \
-	(GFP_KERNEL | \
-	 __GFP_COMP |    /* compound page lets parts be mapped */ \
-	 __GFP_NORETRY | /* don't trigger OOM-killer */ \
-	 __GFP_NOWARN)   /* no stack trace print - this call is non-critical */
-
 /*
  * Coherent device pages allocator
  */
@@ -479,12 +494,12 @@ static const struct snd_malloc_ops snd_dma_dev_ops = {
 #ifdef CONFIG_SND_DMA_SGBUF
 static void *snd_dma_wc_alloc(struct snd_dma_buffer *dmab, size_t size)
 {
-	return do_alloc_fallback_pages(dmab->dev.dev, size, &dmab->addr, true);
+	return do_alloc_pages(dmab->dev.dev, size, &dmab->addr, true);
 }
 
 static void snd_dma_wc_free(struct snd_dma_buffer *dmab)
 {
-	do_free_fallback_pages(dmab->area, dmab->bytes, true);
+	do_free_pages(dmab->area, dmab->bytes, true);
 }
 
 static int snd_dma_wc_mmap(struct snd_dma_buffer *dmab,
@@ -697,37 +712,6 @@ static const struct snd_malloc_ops snd_dma_sg_wc_ops = {
 	.get_chunk_size = snd_dma_noncontig_get_chunk_size,
 };
 
-/* manual page allocations with wc setup */
-static void *do_alloc_fallback_pages(struct device *dev, size_t size,
-				     dma_addr_t *addr, bool wc)
-{
-	gfp_t gfp = GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN;
-	void *p;
-
- again:
-	p = do_alloc_pages(size, addr, gfp);
-	if (!p || (*addr + size - 1) & ~dev->coherent_dma_mask) {
-		if (IS_ENABLED(CONFIG_ZONE_DMA32) && !(gfp & GFP_DMA32)) {
-			gfp |= GFP_DMA32;
-			goto again;
-		}
-		if (IS_ENABLED(CONFIG_ZONE_DMA) && !(gfp & GFP_DMA)) {
-			gfp = (gfp & ~GFP_DMA32) | GFP_DMA;
-			goto again;
-		}
-	}
-	if (p && wc)
-		set_memory_wc((unsigned long)(p), size >> PAGE_SHIFT);
-	return p;
-}
-
-static void do_free_fallback_pages(void *p, size_t size, bool wc)
-{
-	if (wc)
-		set_memory_wb((unsigned long)(p), size >> PAGE_SHIFT);
-	free_pages_exact(p, size);
-}
-
 /* Fallback SG-buffer allocations for x86 */
 struct snd_dma_sg_fallback {
 	size_t count;
@@ -742,7 +726,7 @@ static void __snd_dma_sg_fallback_free(struct snd_dma_buffer *dmab,
 	size_t i;
 
 	for (i = 0; i < sgbuf->count && sgbuf->pages[i]; i++)
-		do_free_fallback_pages(page_address(sgbuf->pages[i]), PAGE_SIZE, wc);
+		do_free_pages(page_address(sgbuf->pages[i]), PAGE_SIZE, wc);
 	kvfree(sgbuf->pages);
 	kvfree(sgbuf->addrs);
 	kfree(sgbuf);
@@ -769,8 +753,7 @@ static void *snd_dma_sg_fallback_alloc(struct snd_dma_buffer *dmab, size_t size)
 		goto error;
 
 	for (i = 0; i < count; sgbuf->count++, i++) {
-		p = do_alloc_fallback_pages(dmab->dev.dev, PAGE_SIZE,
-					    &sgbuf->addrs[i], wc);
+		p = do_alloc_pages(dmab->dev.dev, PAGE_SIZE, &sgbuf->addrs[i], wc);
 		if (!p)
 			goto error;
 		sgbuf->pages[i] = virt_to_page(p);
-- 
2.35.3

[PATCH 5/5] ALSA: doc: Drop snd_dma_continuous_data() usages

[Takashi Iwai]

Update the documentation to follow the recent change of the memory
allocation helpers.  The macro snd_dma_continuous_data() is gone, and
the driver needs to set up the coherent dma mask for allocating in the
lower memory addresses, instead.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 .../kernel-api/writing-an-alsa-driver.rst     | 21 +++++++------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
index 176b73583b7a..07a620c5ca74 100644
--- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
+++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst
@@ -3565,13 +3565,17 @@ given size.
 The second argument (type) and the third argument (device pointer) are
 dependent on the bus. For normal devices, pass the device pointer
 (typically identical as ``card->dev``) to the third argument with
-``SNDRV_DMA_TYPE_DEV`` type. For the continuous buffer unrelated to the
+``SNDRV_DMA_TYPE_DEV`` type.
+
+For the continuous buffer unrelated to the
 bus can be pre-allocated with ``SNDRV_DMA_TYPE_CONTINUOUS`` type.
 You can pass NULL to the device pointer in that case, which is the
 default mode implying to allocate with ``GFP_KERNEL`` flag.
-If you need a different GFP flag, you can pass it by encoding the flag
-into the device pointer via a special macro
-:c:func:`snd_dma_continuous_data()`.
+If you need a restricted (lower) address, set up the coherent DMA mask
+bits for the device, and pass the device pointer, like the normal
+device memory allocations.  For this type, it's still allowed to pass
+NULL to the device pointer, too, if no address restriction is needed.
+
 For the scatter-gather buffers, use ``SNDRV_DMA_TYPE_DEV_SG`` with the
 device pointer (see the `Non-Contiguous Buffers`_ section).
 
@@ -3811,15 +3815,6 @@ arguments here.  Since each vmalloc call should succeed at any time,
 we don't need to pre-allocate the buffers like other continuous
 pages.
 
-If you need the 32bit DMA allocation, pass the device pointer encoded
-by :c:func:`snd_dma_continuous_data()` with ``GFP_KERNEL|__GFP_DMA32``
-argument.
-
-::
-
-  snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_VMALLOC,
-          snd_dma_continuous_data(GFP_KERNEL | __GFP_DMA32), 0, 0);
-
 Proc Interface
 ==============
 
-- 
2.35.3

Re: [PATCH 3/5] ASoC: Intel: sst: Switch to standard device pages

[Cezary Rojewski]

On 2022-08-23 1:57 PM, Takashi Iwai wrote:
> ASoC Atom SST driver is using the continuous RAM pages with GFP_DMA
> flag for its PCM buffer, but this should work fine with the standard
> DMA pages.  As a part of cleanup work, this patch replaces the buffer
> allocation to the standard device pages with SNDRV_DMA_TYPE_DEV.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>   sound/soc/intel/atom/sst-mfld-platform-pcm.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/intel/atom/sst-mfld-platform-pcm.c b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> index a56dd48c045f..c75616a5fd0a 100644
> --- a/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> +++ b/sound/soc/intel/atom/sst-mfld-platform-pcm.c
> @@ -676,10 +676,9 @@ static int sst_soc_pcm_new(struct snd_soc_component *component,
>   
>   	if (dai->driver->playback.channels_min ||
>   			dai->driver->capture.channels_min) {
> -		snd_pcm_set_managed_buffer_all(pcm,
> -			SNDRV_DMA_TYPE_CONTINUOUS,
> -			snd_dma_continuous_data(GFP_DMA),
> -			SST_MIN_BUFFER, SST_MAX_BUFFER);
> +		snd_pcm_set_managed_buffer_all(pcm, SNDRV_DMA_TYPE_DEV,
> +					       pcm->card->dev,
> +					       SST_MIN_BUFFER, SST_MAX_BUFFER);
>   	}
>   	return 0;
>   }


Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>