diff mbox

[v2,4/5] ALSA: emu10k1: make sure synth DMA pages are allocated with DMA functions

Message ID 619e9569-717a-1b4b-6470-4fa6b186bf41@maciej.szmigiero.name (mailing list archive)
State New, archived
Headers show

Commit Message

Maciej S. Szmigiero Jan. 27, 2018, 2:42 p.m. UTC
Commit a5003fc04113 ("[ALSA] emu10k1 - simplify page allocation for synth")
switched from using the DMA allocator for synth DMA pages to manually
calling alloc_page().
However, this usage has an implicit assumption that the DMA address space
for the emu10k1-family chip is the same as the CPU physical address space
which is not true for a system with a IOMMU.

Since this made the synth part of the driver non-functional on such systems
let's effectively revert that commit.

Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
---
Changes from v1: None in this patch.

 sound/pci/emu10k1/memory.c | 70 ++++++++++++++++++++++++++--------------------
 1 file changed, 39 insertions(+), 31 deletions(-)

Comments

Takashi Iwai Feb. 12, 2018, 12:44 p.m. UTC | #1
On Sat, 27 Jan 2018 15:42:47 +0100,
 Maciej S. Szmigiero  wrote:
> 
> Commit a5003fc04113 ("[ALSA] emu10k1 - simplify page allocation for synth")
> switched from using the DMA allocator for synth DMA pages to manually
> calling alloc_page().
> However, this usage has an implicit assumption that the DMA address space
> for the emu10k1-family chip is the same as the CPU physical address space
> which is not true for a system with a IOMMU.
> 
> Since this made the synth part of the driver non-functional on such systems
> let's effectively revert that commit.

Let's keep (or re-add after this revert) the simplification with
__synth_free_pages().  Other than that I have no objection to this
change.


thanks,

Takashi


> 
> Signed-off-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> ---
> Changes from v1: None in this patch.
> 
>  sound/pci/emu10k1/memory.c | 70 ++++++++++++++++++++++++++--------------------
>  1 file changed, 39 insertions(+), 31 deletions(-)
> 
> diff --git a/sound/pci/emu10k1/memory.c b/sound/pci/emu10k1/memory.c
> index fcb04cbbc9ab..5cdffe2d31e1 100644
> --- a/sound/pci/emu10k1/memory.c
> +++ b/sound/pci/emu10k1/memory.c
> @@ -457,49 +457,44 @@ static void get_single_page_range(struct snd_util_memhdr *hdr,
>  	*last_page_ret = last_page;
>  }
>  
> -/* release allocated pages */
> -static void __synth_free_pages(struct snd_emu10k1 *emu, int first_page,
> -			       int last_page)
> -{
> -	int page;
> -
> -	for (page = first_page; page <= last_page; page++) {
> -		free_page((unsigned long)emu->page_ptr_table[page]);
> -		emu->page_addr_table[page] = 0;
> -		emu->page_ptr_table[page] = NULL;
> -	}
> -}
> -
>  /*
>   * allocate kernel pages
>   */
>  static int synth_alloc_pages(struct snd_emu10k1 *emu, struct snd_emu10k1_memblk *blk)
>  {
>  	int page, first_page, last_page;
> +	struct snd_dma_buffer dmab;
>  
>  	emu10k1_memblk_init(blk);
>  	get_single_page_range(emu->memhdr, blk, &first_page, &last_page);
>  	/* allocate kernel pages */
>  	for (page = first_page; page <= last_page; page++) {
> -		/* first try to allocate from <4GB zone */
> -		struct page *p = alloc_page(GFP_KERNEL | GFP_DMA32 |
> -					    __GFP_NOWARN);
> -		if (!p || (page_to_pfn(p) & ~(emu->dma_mask >> PAGE_SHIFT))) {
> -			if (p)
> -				__free_page(p);
> -			/* try to allocate from <16MB zone */
> -			p = alloc_page(GFP_ATOMIC | GFP_DMA |
> -				       __GFP_NORETRY | /* no OOM-killer */
> -				       __GFP_NOWARN);
> -		}
> -		if (!p) {
> -			__synth_free_pages(emu, first_page, page - 1);
> -			return -ENOMEM;
> +		if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV,
> +					snd_dma_pci_data(emu->pci),
> +					PAGE_SIZE, &dmab) < 0)
> +			goto __fail;
> +		if (!is_valid_page(emu, dmab.addr)) {
> +			snd_dma_free_pages(&dmab);
> +			goto __fail;
>  		}
> -		emu->page_addr_table[page] = page_to_phys(p);
> -		emu->page_ptr_table[page] = page_address(p);
> +		emu->page_addr_table[page] = dmab.addr;
> +		emu->page_ptr_table[page] = dmab.area;
>  	}
>  	return 0;
> +
> +__fail:
> +	/* release allocated pages */
> +	last_page = page - 1;
> +	for (page = first_page; page <= last_page; page++) {
> +		dmab.area = emu->page_ptr_table[page];
> +		dmab.addr = emu->page_addr_table[page];
> +		dmab.bytes = PAGE_SIZE;
> +		snd_dma_free_pages(&dmab);
> +		emu->page_addr_table[page] = 0;
> +		emu->page_ptr_table[page] = NULL;
> +	}
> +
> +	return -ENOMEM;
>  }
>  
>  /*
> @@ -507,10 +502,23 @@ static int synth_alloc_pages(struct snd_emu10k1 *emu, struct snd_emu10k1_memblk
>   */
>  static int synth_free_pages(struct snd_emu10k1 *emu, struct snd_emu10k1_memblk *blk)
>  {
> -	int first_page, last_page;
> +	int page, first_page, last_page;
> +	struct snd_dma_buffer dmab;
>  
>  	get_single_page_range(emu->memhdr, blk, &first_page, &last_page);
> -	__synth_free_pages(emu, first_page, last_page);
> +	dmab.dev.type = SNDRV_DMA_TYPE_DEV;
> +	dmab.dev.dev = snd_dma_pci_data(emu->pci);
> +	for (page = first_page; page <= last_page; page++) {
> +		if (emu->page_ptr_table[page] == NULL)
> +			continue;
> +		dmab.area = emu->page_ptr_table[page];
> +		dmab.addr = emu->page_addr_table[page];
> +		dmab.bytes = PAGE_SIZE;
> +		snd_dma_free_pages(&dmab);
> +		emu->page_addr_table[page] = 0;
> +		emu->page_ptr_table[page] = NULL;
> +	}
> +
>  	return 0;
>  }
>  
>
Maciej S. Szmigiero Feb. 12, 2018, 10:14 p.m. UTC | #2
On 12.02.2018 13:44, Takashi Iwai wrote:
> On Sat, 27 Jan 2018 15:42:47 +0100,
>  Maciej S. Szmigiero  wrote:
>>
>> Commit a5003fc04113 ("[ALSA] emu10k1 - simplify page allocation for synth")
>> switched from using the DMA allocator for synth DMA pages to manually
>> calling alloc_page().
>> However, this usage has an implicit assumption that the DMA address space
>> for the emu10k1-family chip is the same as the CPU physical address space
>> which is not true for a system with a IOMMU.
>>
>> Since this made the synth part of the driver non-functional on such systems
>> let's effectively revert that commit.
> 
> Let's keep (or re-add after this revert) the simplification with
> __synth_free_pages().  Other than that I have no objection to this
> change.

Will keep the simplification via __synth_free_pages() function in a respin.

> thanks,
> 
> Takashi

Thanks,
Maciej
diff mbox

Patch

diff --git a/sound/pci/emu10k1/memory.c b/sound/pci/emu10k1/memory.c
index fcb04cbbc9ab..5cdffe2d31e1 100644
--- a/sound/pci/emu10k1/memory.c
+++ b/sound/pci/emu10k1/memory.c
@@ -457,49 +457,44 @@  static void get_single_page_range(struct snd_util_memhdr *hdr,
 	*last_page_ret = last_page;
 }
 
-/* release allocated pages */
-static void __synth_free_pages(struct snd_emu10k1 *emu, int first_page,
-			       int last_page)
-{
-	int page;
-
-	for (page = first_page; page <= last_page; page++) {
-		free_page((unsigned long)emu->page_ptr_table[page]);
-		emu->page_addr_table[page] = 0;
-		emu->page_ptr_table[page] = NULL;
-	}
-}
-
 /*
  * allocate kernel pages
  */
 static int synth_alloc_pages(struct snd_emu10k1 *emu, struct snd_emu10k1_memblk *blk)
 {
 	int page, first_page, last_page;
+	struct snd_dma_buffer dmab;
 
 	emu10k1_memblk_init(blk);
 	get_single_page_range(emu->memhdr, blk, &first_page, &last_page);
 	/* allocate kernel pages */
 	for (page = first_page; page <= last_page; page++) {
-		/* first try to allocate from <4GB zone */
-		struct page *p = alloc_page(GFP_KERNEL | GFP_DMA32 |
-					    __GFP_NOWARN);
-		if (!p || (page_to_pfn(p) & ~(emu->dma_mask >> PAGE_SHIFT))) {
-			if (p)
-				__free_page(p);
-			/* try to allocate from <16MB zone */
-			p = alloc_page(GFP_ATOMIC | GFP_DMA |
-				       __GFP_NORETRY | /* no OOM-killer */
-				       __GFP_NOWARN);
-		}
-		if (!p) {
-			__synth_free_pages(emu, first_page, page - 1);
-			return -ENOMEM;
+		if (snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV,
+					snd_dma_pci_data(emu->pci),
+					PAGE_SIZE, &dmab) < 0)
+			goto __fail;
+		if (!is_valid_page(emu, dmab.addr)) {
+			snd_dma_free_pages(&dmab);
+			goto __fail;
 		}
-		emu->page_addr_table[page] = page_to_phys(p);
-		emu->page_ptr_table[page] = page_address(p);
+		emu->page_addr_table[page] = dmab.addr;
+		emu->page_ptr_table[page] = dmab.area;
 	}
 	return 0;
+
+__fail:
+	/* release allocated pages */
+	last_page = page - 1;
+	for (page = first_page; page <= last_page; page++) {
+		dmab.area = emu->page_ptr_table[page];
+		dmab.addr = emu->page_addr_table[page];
+		dmab.bytes = PAGE_SIZE;
+		snd_dma_free_pages(&dmab);
+		emu->page_addr_table[page] = 0;
+		emu->page_ptr_table[page] = NULL;
+	}
+
+	return -ENOMEM;
 }
 
 /*
@@ -507,10 +502,23 @@  static int synth_alloc_pages(struct snd_emu10k1 *emu, struct snd_emu10k1_memblk
  */
 static int synth_free_pages(struct snd_emu10k1 *emu, struct snd_emu10k1_memblk *blk)
 {
-	int first_page, last_page;
+	int page, first_page, last_page;
+	struct snd_dma_buffer dmab;
 
 	get_single_page_range(emu->memhdr, blk, &first_page, &last_page);
-	__synth_free_pages(emu, first_page, last_page);
+	dmab.dev.type = SNDRV_DMA_TYPE_DEV;
+	dmab.dev.dev = snd_dma_pci_data(emu->pci);
+	for (page = first_page; page <= last_page; page++) {
+		if (emu->page_ptr_table[page] == NULL)
+			continue;
+		dmab.area = emu->page_ptr_table[page];
+		dmab.addr = emu->page_addr_table[page];
+		dmab.bytes = PAGE_SIZE;
+		snd_dma_free_pages(&dmab);
+		emu->page_addr_table[page] = 0;
+		emu->page_ptr_table[page] = NULL;
+	}
+
 	return 0;
 }