All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RFC] swiotlb: Remove SWIOTLB overflow buffer support
@ 2012-07-06 23:06 Shuah Khan
  2012-07-09 20:25 ` Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 14+ messages in thread
From: Shuah Khan @ 2012-07-06 23:06 UTC (permalink / raw)
  To: LKML; +Cc: shuahkhan, akpm, paul.gortmaker, konrad.wilk, bhelgaas, amwang

Remove SWIOTLB overflow buffer support and return DMA_ERROR_CODE
(a value of zero) to make it consistent with iommu implementation
on Intel, AMD, and swiotlb-xen.

Tested only on x86.

Signed-off-by: Shuah Khan <shuah.khan@hp.com>
---
 lib/swiotlb.c |   44 ++++++++------------------------------------
 1 file changed, 8 insertions(+), 36 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 45bc1f8..7f0a5d1 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -15,6 +15,7 @@
  * 05/09/10 linville	Add support for syncing ranges, support syncing for
  *			DMA_BIDIRECTIONAL mappings, miscellaneous cleanup.
  * 08/12/11 beckyb	Add highmem support
+ * 06/12    shuahkhan	Remove io tlb overflow support
  */
 
 #include <linux/cache.h>
@@ -66,13 +67,6 @@ static char *io_tlb_start, *io_tlb_end;
 static unsigned long io_tlb_nslabs;
 
 /*
- * When the IOMMU overflows we return a fallback buffer. This sets the size.
- */
-static unsigned long io_tlb_overflow = 32*1024;
-
-static void *io_tlb_overflow_buffer;
-
-/*
  * This is a free list describing the number of free entries available from
  * each index
  */
@@ -108,7 +102,6 @@ setup_io_tlb_npages(char *str)
 	return 1;
 }
 __setup("swiotlb=", setup_io_tlb_npages);
-/* make io_tlb_overflow tunable too? */
 
 unsigned long swiotlb_nr_tbl(void)
 {
@@ -156,12 +149,6 @@ void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 	io_tlb_index = 0;
 	io_tlb_orig_addr = alloc_bootmem_pages(PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
 
-	/*
-	 * Get the overflow emergency buffer
-	 */
-	io_tlb_overflow_buffer = alloc_bootmem_low_pages(PAGE_ALIGN(io_tlb_overflow));
-	if (!io_tlb_overflow_buffer)
-		panic("Cannot allocate SWIOTLB overflow buffer!\n");
 	if (verbose)
 		swiotlb_print_info();
 }
@@ -195,7 +182,8 @@ swiotlb_init_with_default_size(size_t default_size, int verbose)
 void __init
 swiotlb_init(int verbose)
 {
-	swiotlb_init_with_default_size(64 * (1<<20), verbose);	/* default to 64MB */
+	/* default to 64MB */
+	swiotlb_init_with_default_size(64 * (1<<20), verbose);
 }
 
 /*
@@ -264,24 +252,12 @@ swiotlb_late_init_with_default_size(size_t default_size)
 
 	memset(io_tlb_orig_addr, 0, io_tlb_nslabs * sizeof(phys_addr_t));
 
-	/*
-	 * Get the overflow emergency buffer
-	 */
-	io_tlb_overflow_buffer = (void *)__get_free_pages(GFP_DMA,
-	                                          get_order(io_tlb_overflow));
-	if (!io_tlb_overflow_buffer)
-		goto cleanup4;
-
 	swiotlb_print_info();
 
 	late_alloc = 1;
 
 	return 0;
 
-cleanup4:
-	free_pages((unsigned long)io_tlb_orig_addr,
-		   get_order(io_tlb_nslabs * sizeof(phys_addr_t)));
-	io_tlb_orig_addr = NULL;
 cleanup3:
 	free_pages((unsigned long)io_tlb_list, get_order(io_tlb_nslabs *
 	                                                 sizeof(int)));
@@ -297,12 +273,10 @@ cleanup1:
 
 void __init swiotlb_free(void)
 {
-	if (!io_tlb_overflow_buffer)
+	if (!io_tlb_orig_addr)
 		return;
 
 	if (late_alloc) {
-		free_pages((unsigned long)io_tlb_overflow_buffer,
-			   get_order(io_tlb_overflow));
 		free_pages((unsigned long)io_tlb_orig_addr,
 			   get_order(io_tlb_nslabs * sizeof(phys_addr_t)));
 		free_pages((unsigned long)io_tlb_list, get_order(io_tlb_nslabs *
@@ -310,8 +284,6 @@ void __init swiotlb_free(void)
 		free_pages((unsigned long)io_tlb_start,
 			   get_order(io_tlb_nslabs << IO_TLB_SHIFT));
 	} else {
-		free_bootmem_late(__pa(io_tlb_overflow_buffer),
-				  PAGE_ALIGN(io_tlb_overflow));
 		free_bootmem_late(__pa(io_tlb_orig_addr),
 				  PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
 		free_bootmem_late(__pa(io_tlb_list),
@@ -639,7 +611,7 @@ swiotlb_full(struct device *dev, size_t size, enum dma_data_direction dir,
 	printk(KERN_ERR "DMA: Out of SW-IOMMU space for %zu bytes at "
 	       "device %s\n", size, dev ? dev_name(dev) : "?");
 
-	if (size <= io_tlb_overflow || !do_panic)
+	if (!do_panic)
 		return;
 
 	if (dir == DMA_BIDIRECTIONAL)
@@ -681,7 +653,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 	map = map_single(dev, phys, size, dir);
 	if (!map) {
 		swiotlb_full(dev, size, dir, 1);
-		map = io_tlb_overflow_buffer;
+		return DMA_ERROR_CODE;
 	}
 
 	dev_addr = swiotlb_virt_to_bus(dev, map);
@@ -691,7 +663,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 	 */
 	if (!dma_capable(dev, dev_addr, size)) {
 		swiotlb_tbl_unmap_single(dev, map, size, dir);
-		dev_addr = swiotlb_virt_to_bus(dev, io_tlb_overflow_buffer);
+		dev_addr = 0;
 	}
 
 	return dev_addr;
@@ -910,7 +882,7 @@ EXPORT_SYMBOL(swiotlb_sync_sg_for_device);
 int
 swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t dma_addr)
 {
-	return (dma_addr == swiotlb_virt_to_bus(hwdev, io_tlb_overflow_buffer));
+	return !dma_addr;
 }
 EXPORT_SYMBOL(swiotlb_dma_mapping_error);
 
-- 
1.7.9.5




^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH RFC] swiotlb: Remove SWIOTLB overflow buffer support
  2012-07-06 23:06 [PATCH RFC] swiotlb: Remove SWIOTLB overflow buffer support Shuah Khan
@ 2012-07-09 20:25 ` Konrad Rzeszutek Wilk
  2012-07-10 16:33   ` FUJITA Tomonori
  2012-07-10 16:55   ` Shuah Khan
  0 siblings, 2 replies; 14+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-07-09 20:25 UTC (permalink / raw)
  To: Shuah Khan, fujita.tomonori
  Cc: LKML, shuahkhan, akpm, paul.gortmaker, bhelgaas, amwang

On Fri, Jul 06, 2012 at 05:06:12PM -0600, Shuah Khan wrote:
> Remove SWIOTLB overflow buffer support and return DMA_ERROR_CODE
> (a value of zero) to make it consistent with iommu implementation
> on Intel, AMD, and swiotlb-xen.

While this is a good forward step and this needs to be done eventually,
you should first send out patches for the drivers that don't check
for the DMA_ERROR_CODE when doing mapping. In other words for the
drivers that map but don't call dma_mapping_error to check.

When that is fixed and *all the drivers that don't call dma_mapping_error
are fixed, then this patch makes sense. 

So for right now, NACK.

Also you missed CC-ing fujita.tomonori@lab.ntt.co.jp, so let me add
that to this email.

*: for those that it makes sense. I am not sure if folks are still using
ancient drivers..

> 
> Tested only on x86.
> 
> Signed-off-by: Shuah Khan <shuah.khan@hp.com>
> ---
>  lib/swiotlb.c |   44 ++++++++------------------------------------
>  1 file changed, 8 insertions(+), 36 deletions(-)
> 
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index 45bc1f8..7f0a5d1 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -15,6 +15,7 @@
>   * 05/09/10 linville	Add support for syncing ranges, support syncing for
>   *			DMA_BIDIRECTIONAL mappings, miscellaneous cleanup.
>   * 08/12/11 beckyb	Add highmem support
> + * 06/12    shuahkhan	Remove io tlb overflow support
>   */
>  
>  #include <linux/cache.h>
> @@ -66,13 +67,6 @@ static char *io_tlb_start, *io_tlb_end;
>  static unsigned long io_tlb_nslabs;
>  
>  /*
> - * When the IOMMU overflows we return a fallback buffer. This sets the size.
> - */
> -static unsigned long io_tlb_overflow = 32*1024;
> -
> -static void *io_tlb_overflow_buffer;
> -
> -/*
>   * This is a free list describing the number of free entries available from
>   * each index
>   */
> @@ -108,7 +102,6 @@ setup_io_tlb_npages(char *str)
>  	return 1;
>  }
>  __setup("swiotlb=", setup_io_tlb_npages);
> -/* make io_tlb_overflow tunable too? */
>  
>  unsigned long swiotlb_nr_tbl(void)
>  {
> @@ -156,12 +149,6 @@ void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
>  	io_tlb_index = 0;
>  	io_tlb_orig_addr = alloc_bootmem_pages(PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
>  
> -	/*
> -	 * Get the overflow emergency buffer
> -	 */
> -	io_tlb_overflow_buffer = alloc_bootmem_low_pages(PAGE_ALIGN(io_tlb_overflow));
> -	if (!io_tlb_overflow_buffer)
> -		panic("Cannot allocate SWIOTLB overflow buffer!\n");
>  	if (verbose)
>  		swiotlb_print_info();
>  }
> @@ -195,7 +182,8 @@ swiotlb_init_with_default_size(size_t default_size, int verbose)
>  void __init
>  swiotlb_init(int verbose)
>  {
> -	swiotlb_init_with_default_size(64 * (1<<20), verbose);	/* default to 64MB */
> +	/* default to 64MB */
> +	swiotlb_init_with_default_size(64 * (1<<20), verbose);
>  }
>  
>  /*
> @@ -264,24 +252,12 @@ swiotlb_late_init_with_default_size(size_t default_size)
>  
>  	memset(io_tlb_orig_addr, 0, io_tlb_nslabs * sizeof(phys_addr_t));
>  
> -	/*
> -	 * Get the overflow emergency buffer
> -	 */
> -	io_tlb_overflow_buffer = (void *)__get_free_pages(GFP_DMA,
> -	                                          get_order(io_tlb_overflow));
> -	if (!io_tlb_overflow_buffer)
> -		goto cleanup4;
> -
>  	swiotlb_print_info();
>  
>  	late_alloc = 1;
>  
>  	return 0;
>  
> -cleanup4:
> -	free_pages((unsigned long)io_tlb_orig_addr,
> -		   get_order(io_tlb_nslabs * sizeof(phys_addr_t)));
> -	io_tlb_orig_addr = NULL;
>  cleanup3:
>  	free_pages((unsigned long)io_tlb_list, get_order(io_tlb_nslabs *
>  	                                                 sizeof(int)));
> @@ -297,12 +273,10 @@ cleanup1:
>  
>  void __init swiotlb_free(void)
>  {
> -	if (!io_tlb_overflow_buffer)
> +	if (!io_tlb_orig_addr)
>  		return;
>  
>  	if (late_alloc) {
> -		free_pages((unsigned long)io_tlb_overflow_buffer,
> -			   get_order(io_tlb_overflow));
>  		free_pages((unsigned long)io_tlb_orig_addr,
>  			   get_order(io_tlb_nslabs * sizeof(phys_addr_t)));
>  		free_pages((unsigned long)io_tlb_list, get_order(io_tlb_nslabs *
> @@ -310,8 +284,6 @@ void __init swiotlb_free(void)
>  		free_pages((unsigned long)io_tlb_start,
>  			   get_order(io_tlb_nslabs << IO_TLB_SHIFT));
>  	} else {
> -		free_bootmem_late(__pa(io_tlb_overflow_buffer),
> -				  PAGE_ALIGN(io_tlb_overflow));
>  		free_bootmem_late(__pa(io_tlb_orig_addr),
>  				  PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
>  		free_bootmem_late(__pa(io_tlb_list),
> @@ -639,7 +611,7 @@ swiotlb_full(struct device *dev, size_t size, enum dma_data_direction dir,
>  	printk(KERN_ERR "DMA: Out of SW-IOMMU space for %zu bytes at "
>  	       "device %s\n", size, dev ? dev_name(dev) : "?");
>  
> -	if (size <= io_tlb_overflow || !do_panic)
> +	if (!do_panic)
>  		return;
>  
>  	if (dir == DMA_BIDIRECTIONAL)
> @@ -681,7 +653,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
>  	map = map_single(dev, phys, size, dir);
>  	if (!map) {
>  		swiotlb_full(dev, size, dir, 1);
> -		map = io_tlb_overflow_buffer;
> +		return DMA_ERROR_CODE;
>  	}
>  
>  	dev_addr = swiotlb_virt_to_bus(dev, map);
> @@ -691,7 +663,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
>  	 */
>  	if (!dma_capable(dev, dev_addr, size)) {
>  		swiotlb_tbl_unmap_single(dev, map, size, dir);
> -		dev_addr = swiotlb_virt_to_bus(dev, io_tlb_overflow_buffer);
> +		dev_addr = 0;
>  	}
>  
>  	return dev_addr;
> @@ -910,7 +882,7 @@ EXPORT_SYMBOL(swiotlb_sync_sg_for_device);
>  int
>  swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t dma_addr)
>  {
> -	return (dma_addr == swiotlb_virt_to_bus(hwdev, io_tlb_overflow_buffer));
> +	return !dma_addr;
>  }
>  EXPORT_SYMBOL(swiotlb_dma_mapping_error);
>  
> -- 
> 1.7.9.5
> 
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH RFC] swiotlb: Remove SWIOTLB overflow buffer support
  2012-07-09 20:25 ` Konrad Rzeszutek Wilk
@ 2012-07-10 16:33   ` FUJITA Tomonori
  2012-07-10 16:55   ` Shuah Khan
  1 sibling, 0 replies; 14+ messages in thread
From: FUJITA Tomonori @ 2012-07-10 16:33 UTC (permalink / raw)
  To: konrad.wilk
  Cc: shuah.khan, linux-kernel, shuahkhan, akpm, paul.gortmaker,
	bhelgaas, amwang

On Mon, 9 Jul 2012 16:25:05 -0400
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> On Fri, Jul 06, 2012 at 05:06:12PM -0600, Shuah Khan wrote:
>> Remove SWIOTLB overflow buffer support and return DMA_ERROR_CODE
>> (a value of zero) to make it consistent with iommu implementation
>> on Intel, AMD, and swiotlb-xen.
> 
> While this is a good forward step and this needs to be done eventually,
> you should first send out patches for the drivers that don't check
> for the DMA_ERROR_CODE when doing mapping. In other words for the
> drivers that map but don't call dma_mapping_error to check.
> 
> When that is fixed and *all the drivers that don't call dma_mapping_error
> are fixed, then this patch makes sense. 
> 
> So for right now, NACK.

Yeah, I'm not sure we could fix (or remove) *all the drivers though.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH RFC] swiotlb: Remove SWIOTLB overflow buffer support
  2012-07-09 20:25 ` Konrad Rzeszutek Wilk
  2012-07-10 16:33   ` FUJITA Tomonori
@ 2012-07-10 16:55   ` Shuah Khan
  2012-07-10 17:32     ` Konrad Rzeszutek Wilk
  1 sibling, 1 reply; 14+ messages in thread
From: Shuah Khan @ 2012-07-10 16:55 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: fujita.tomonori, LKML, akpm, paul.gortmaker, bhelgaas, amwang, shuahkhan

On Mon, 2012-07-09 at 16:25 -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, Jul 06, 2012 at 05:06:12PM -0600, Shuah Khan wrote:
> > Remove SWIOTLB overflow buffer support and return DMA_ERROR_CODE
> > (a value of zero) to make it consistent with iommu implementation
> > on Intel, AMD, and swiotlb-xen.
> 
> While this is a good forward step and this needs to be done eventually,
> you should first send out patches for the drivers that don't check
> for the DMA_ERROR_CODE when doing mapping. In other words for the
> drivers that map but don't call dma_mapping_error to check.
> 
> When that is fixed and *all the drivers that don't call dma_mapping_error
> are fixed, then this patch makes sense.

The challenge will be catching all the drivers and have confidence that
all of them are covered. I will start looking into this to get a feel
for how many drivers needs fixing.

Also, isn't this problem today with other iommu implementations? All of
the other iommu implementation don't have support for overflow buffers?

-- Shuah




^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH RFC] swiotlb: Remove SWIOTLB overflow buffer support
  2012-07-10 16:55   ` Shuah Khan
@ 2012-07-10 17:32     ` Konrad Rzeszutek Wilk
  2012-07-10 23:06       ` Shuah Khan
  2012-07-12 16:17       ` [PATCH RFC] swiotlb: Disable swiotlb overflow support when CONFIG_ISA is enabled Shuah Khan
  0 siblings, 2 replies; 14+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-07-10 17:32 UTC (permalink / raw)
  To: Shuah Khan
  Cc: fujita.tomonori, LKML, akpm, paul.gortmaker, bhelgaas, amwang, shuahkhan

On Tue, Jul 10, 2012 at 10:55:07AM -0600, Shuah Khan wrote:
> On Mon, 2012-07-09 at 16:25 -0400, Konrad Rzeszutek Wilk wrote:
> > On Fri, Jul 06, 2012 at 05:06:12PM -0600, Shuah Khan wrote:
> > > Remove SWIOTLB overflow buffer support and return DMA_ERROR_CODE
> > > (a value of zero) to make it consistent with iommu implementation
> > > on Intel, AMD, and swiotlb-xen.
> > 
> > While this is a good forward step and this needs to be done eventually,
> > you should first send out patches for the drivers that don't check
> > for the DMA_ERROR_CODE when doing mapping. In other words for the
> > drivers that map but don't call dma_mapping_error to check.
> > 
> > When that is fixed and *all the drivers that don't call dma_mapping_error
> > are fixed, then this patch makes sense.
> 
> The challenge will be catching all the drivers and have confidence that
> all of them are covered. I will start looking into this to get a feel
> for how many drivers needs fixing.

I don't know if all is needed. Some of them might be dead or not used
at all anymore - who knows? This treasure hunt would give a good idea
of which ones are not using the PCI/DMA API right at least.

If it is say, CONFIG_ISA enabled, well, we could #ifdef the overflow
buffer in the swiotlb with that option. And then work through fixing
up those drivers - except that finding folks with that driver to
see if it works .. yuck.. I do have some few ISA cards and some boxes
with ISA slots :-)

> 
> Also, isn't this problem today with other iommu implementations? All of
> the other iommu implementation don't have support for overflow buffers?

Right, but you are in high likehood not going to run the drivers that
don't check this with an IOMMU. Because the drivers are mostly the PCI
variants .. or PCMCIA. And the hardware that would use this did not have
IOMMU.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH RFC] swiotlb: Remove SWIOTLB overflow buffer support
  2012-07-10 17:32     ` Konrad Rzeszutek Wilk
@ 2012-07-10 23:06       ` Shuah Khan
  2012-07-10 23:13         ` Shuah Khan
  2012-07-12 16:17       ` [PATCH RFC] swiotlb: Disable swiotlb overflow support when CONFIG_ISA is enabled Shuah Khan
  1 sibling, 1 reply; 14+ messages in thread
From: Shuah Khan @ 2012-07-10 23:06 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: fujita.tomonori, LKML, akpm, paul.gortmaker, bhelgaas, amwang

On Tue, 2012-07-10 at 13:32 -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Jul 10, 2012 at 10:55:07AM -0600, Shuah Khan wrote:
> > On Mon, 2012-07-09 at 16:25 -0400, Konrad Rzeszutek Wilk wrote:
> > > On Fri, Jul 06, 2012 at 05:06:12PM -0600, Shuah Khan wrote:
> > > > Remove SWIOTLB overflow buffer support and return DMA_ERROR_CODE
> > > > (a value of zero) to make it consistent with iommu implementation
> > > > on Intel, AMD, and swiotlb-xen.
> > > 
> > > While this is a good forward step and this needs to be done eventually,
> > > you should first send out patches for the drivers that don't check
> > > for the DMA_ERROR_CODE when doing mapping. In other words for the
> > > drivers that map but don't call dma_mapping_error to check.
> > > 
> > > When that is fixed and *all the drivers that don't call dma_mapping_error
> > > are fixed, then this patch makes sense.
> > 
> > The challenge will be catching all the drivers and have confidence that
> > all of them are covered. I will start looking into this to get a feel
> > for how many drivers needs fixing.
> 
> I don't know if all is needed. Some of them might be dead or not used
> at all anymore - who knows? This treasure hunt would give a good idea
> of which ones are not using the PCI/DMA API right at least.
> 
> If it is say, CONFIG_ISA enabled, well, we could #ifdef the overflow
> buffer in the swiotlb with that option. And then work through fixing
> up those drivers - except that finding folks with that driver to
> see if it works .. yuck.. I do have some few ISA cards and some boxes
> with ISA slots :-)

Looking at the history CONFIG_ISA was disabled back in 2002 and
CONFIG_ISA_DMA_API was disabled in 2004. Sounds like it will be fun
finding drivers that could fail. 

Would it make sense to make io_tlb_overflow a tunable and disable by
default instead of ifdef? Currently it is always enabled, disabling it
by default is another way to find problems?

Good to know you have a few ISA cards. :)

-- Shuah



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH RFC] swiotlb: Remove SWIOTLB overflow buffer support
  2012-07-10 23:06       ` Shuah Khan
@ 2012-07-10 23:13         ` Shuah Khan
  0 siblings, 0 replies; 14+ messages in thread
From: Shuah Khan @ 2012-07-10 23:13 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: fujita.tomonori, LKML, akpm, paul.gortmaker, bhelgaas, amwang, shuahkhan

On Tue, 2012-07-10 at 17:06 -0600, Shuah Khan wrote:
> On Tue, 2012-07-10 at 13:32 -0400, Konrad Rzeszutek Wilk wrote:
> > On Tue, Jul 10, 2012 at 10:55:07AM -0600, Shuah Khan wrote:
> > > On Mon, 2012-07-09 at 16:25 -0400, Konrad Rzeszutek Wilk wrote:
> > > > On Fri, Jul 06, 2012 at 05:06:12PM -0600, Shuah Khan wrote:
> > > > > Remove SWIOTLB overflow buffer support and return DMA_ERROR_CODE
> > > > > (a value of zero) to make it consistent with iommu implementation
> > > > > on Intel, AMD, and swiotlb-xen.
> > > > 
> > > > While this is a good forward step and this needs to be done eventually,
> > > > you should first send out patches for the drivers that don't check
> > > > for the DMA_ERROR_CODE when doing mapping. In other words for the
> > > > drivers that map but don't call dma_mapping_error to check.
> > > > 
> > > > When that is fixed and *all the drivers that don't call dma_mapping_error
> > > > are fixed, then this patch makes sense.
> > > 
> > > The challenge will be catching all the drivers and have confidence that
> > > all of them are covered. I will start looking into this to get a feel
> > > for how many drivers needs fixing.
> > 
> > I don't know if all is needed. Some of them might be dead or not used
> > at all anymore - who knows? This treasure hunt would give a good idea
> > of which ones are not using the PCI/DMA API right at least.
> > 
> > If it is say, CONFIG_ISA enabled, well, we could #ifdef the overflow
> > buffer in the swiotlb with that option. And then work through fixing
> > up those drivers - except that finding folks with that driver to
> > see if it works .. yuck.. I do have some few ISA cards and some boxes
> > with ISA slots :-)
> 
> Looking at the history CONFIG_ISA was disabled back in 2002 and
> CONFIG_ISA_DMA_API was disabled in 2004. Sounds like it will be fun
> finding drivers that could fail.

Correction. CONFIG_ISA_DMA_API is made configurable on x86_64 it appears
back in 2011.

-- Shuah



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH RFC] swiotlb: Disable swiotlb overflow support when CONFIG_ISA is enabled
  2012-07-10 17:32     ` Konrad Rzeszutek Wilk
  2012-07-10 23:06       ` Shuah Khan
@ 2012-07-12 16:17       ` Shuah Khan
  2012-07-16 14:45         ` Konrad Rzeszutek Wilk
  1 sibling, 1 reply; 14+ messages in thread
From: Shuah Khan @ 2012-07-12 16:17 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: fujita.tomonori, LKML, akpm, paul.gortmaker, bhelgaas, amwang, shuahkhan

Disable iotlb overflow support when CONFIG_ISA is enabled. This is the
first step towards removing overflow support, to be consistent with other
iommu implementations and return DMA_ERROR_CODE. This disabling step is
for finding drivers that don't call dma_mapping_error to check for errors
returned by the mapping interface. Once drivers are fixed overflow support
can be removed.

Tested on x86 with io_tlb_overflow = 32*1024 and io_tlb_overflow = 0.

Signed-off-by: Shuah Khan <shuah.khan@hp.com>
---
 lib/swiotlb.c |   65 +++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 18 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 45bc1f8..f7d285c 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -15,6 +15,9 @@
  * 05/09/10 linville	Add support for syncing ranges, support syncing for
  *			DMA_BIDIRECTIONAL mappings, miscellaneous cleanup.
  * 08/12/11 beckyb	Add highmem support
+ * 07/2012  shuahkhan	Disable iotlb overflow support when CONFIG_ISA
+ *			is enabled. Remove it for all configs when drivers
+ *			that don't check for mapping errors are fixed.
  */
 
 #include <linux/cache.h>
@@ -68,7 +71,19 @@ static unsigned long io_tlb_nslabs;
 /*
  * When the IOMMU overflows we return a fallback buffer. This sets the size.
  */
+#if defined(CONFIG_ISA)
+/*
+ * Disable iotlb overflow support when CONFIG_ISA is enabled. This is the
+ * first step towards removing overflow support, to be consistent with other
+ * iommu implementations and return DMA_ERROR_CODE. This disabling step is
+ * for finding drivers that don't call dma_mapping_error to check for errors
+ * returned by the mapping interface. Once drivers are fixed overflow support
+ * can be removed.
+*/
+static unsigned long io_tlb_overflow;
+#else
 static unsigned long io_tlb_overflow = 32*1024;
+#endif
 
 static void *io_tlb_overflow_buffer;
 
@@ -156,12 +171,16 @@ void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 	io_tlb_index = 0;
 	io_tlb_orig_addr = alloc_bootmem_pages(PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
 
-	/*
-	 * Get the overflow emergency buffer
-	 */
-	io_tlb_overflow_buffer = alloc_bootmem_low_pages(PAGE_ALIGN(io_tlb_overflow));
-	if (!io_tlb_overflow_buffer)
-		panic("Cannot allocate SWIOTLB overflow buffer!\n");
+	if (io_tlb_overflow) {
+		/*
+		 * Get the overflow emergency buffer
+		 */
+		io_tlb_overflow_buffer = alloc_bootmem_low_pages(
+						PAGE_ALIGN(io_tlb_overflow));
+		if (!io_tlb_overflow_buffer)
+			panic("Cannot allocate SWIOTLB overflow buffer!\n");
+	}
+
 	if (verbose)
 		swiotlb_print_info();
 }
@@ -264,13 +283,15 @@ swiotlb_late_init_with_default_size(size_t default_size)
 
 	memset(io_tlb_orig_addr, 0, io_tlb_nslabs * sizeof(phys_addr_t));
 
-	/*
-	 * Get the overflow emergency buffer
-	 */
-	io_tlb_overflow_buffer = (void *)__get_free_pages(GFP_DMA,
-	                                          get_order(io_tlb_overflow));
-	if (!io_tlb_overflow_buffer)
-		goto cleanup4;
+	if (io_tlb_overflow) {
+		/*
+		 * Get the overflow emergency buffer
+		 */
+		io_tlb_overflow_buffer = (void *)
+			__get_free_pages(GFP_DMA, get_order(io_tlb_overflow));
+		if (!io_tlb_overflow_buffer)
+			goto cleanup4;
+	}
 
 	swiotlb_print_info();
 
@@ -297,12 +318,13 @@ cleanup1:
 
 void __init swiotlb_free(void)
 {
-	if (!io_tlb_overflow_buffer)
+	if (!io_tlb_orig_addr)
 		return;
 
 	if (late_alloc) {
-		free_pages((unsigned long)io_tlb_overflow_buffer,
-			   get_order(io_tlb_overflow));
+		if (io_tlb_overflow_buffer)
+			free_pages((unsigned long)io_tlb_overflow_buffer,
+				   get_order(io_tlb_overflow));
 		free_pages((unsigned long)io_tlb_orig_addr,
 			   get_order(io_tlb_nslabs * sizeof(phys_addr_t)));
 		free_pages((unsigned long)io_tlb_list, get_order(io_tlb_nslabs *
@@ -310,8 +332,9 @@ void __init swiotlb_free(void)
 		free_pages((unsigned long)io_tlb_start,
 			   get_order(io_tlb_nslabs << IO_TLB_SHIFT));
 	} else {
-		free_bootmem_late(__pa(io_tlb_overflow_buffer),
-				  PAGE_ALIGN(io_tlb_overflow));
+		if (io_tlb_overflow_buffer)
+			free_bootmem_late(__pa(io_tlb_overflow_buffer),
+					  PAGE_ALIGN(io_tlb_overflow));
 		free_bootmem_late(__pa(io_tlb_orig_addr),
 				  PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
 		free_bootmem_late(__pa(io_tlb_list),
@@ -681,6 +704,8 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 	map = map_single(dev, phys, size, dir);
 	if (!map) {
 		swiotlb_full(dev, size, dir, 1);
+		if (!io_tlb_overflow)
+			return DMA_ERROR_CODE;
 		map = io_tlb_overflow_buffer;
 	}
 
@@ -691,6 +716,8 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 	 */
 	if (!dma_capable(dev, dev_addr, size)) {
 		swiotlb_tbl_unmap_single(dev, map, size, dir);
+		if (!io_tlb_overflow)
+			return DMA_ERROR_CODE;
 		dev_addr = swiotlb_virt_to_bus(dev, io_tlb_overflow_buffer);
 	}
 
@@ -910,6 +937,8 @@ EXPORT_SYMBOL(swiotlb_sync_sg_for_device);
 int
 swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t dma_addr)
 {
+	if (!io_tlb_overflow)
+		return DMA_ERROR_CODE;
 	return (dma_addr == swiotlb_virt_to_bus(hwdev, io_tlb_overflow_buffer));
 }
 EXPORT_SYMBOL(swiotlb_dma_mapping_error);
-- 
1.7.9.5




^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH RFC] swiotlb: Disable swiotlb overflow support when CONFIG_ISA is enabled
  2012-07-12 16:17       ` [PATCH RFC] swiotlb: Disable swiotlb overflow support when CONFIG_ISA is enabled Shuah Khan
@ 2012-07-16 14:45         ` Konrad Rzeszutek Wilk
  2012-07-16 15:48           ` Shuah Khan
  0 siblings, 1 reply; 14+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-07-16 14:45 UTC (permalink / raw)
  To: Shuah Khan
  Cc: fujita.tomonori, LKML, akpm, paul.gortmaker, bhelgaas, amwang, shuahkhan

On Thu, Jul 12, 2012 at 10:17:50AM -0600, Shuah Khan wrote:
> Disable iotlb overflow support when CONFIG_ISA is enabled. This is the
> first step towards removing overflow support, to be consistent with other
> iommu implementations and return DMA_ERROR_CODE. This disabling step is
> for finding drivers that don't call dma_mapping_error to check for errors
> returned by the mapping interface. Once drivers are fixed overflow support
> can be removed.

I think I explained myself poorly. We don't want to break old drivers.

If the old drivers expect this behavior (while they should be updated
to check the DMA), we need to retain this behavior.

So I think the patch should be done the other way:

Disable IOTLB overflow when CONFIG_ISA is disabled.

But we also need to check whether there are other drivers
that don't properly check the DMA address. And if so, add them
to this list of must have enabled b/c you might be using this
driver.

The first goal is to figure out which of the drivers aren't doing this
properly. This should be possible by just grepping for 'dma_map' and 
seeing which ones don't do the 'dma_check' right after.

> 
> Tested on x86 with io_tlb_overflow = 32*1024 and io_tlb_overflow = 0.
> 
> Signed-off-by: Shuah Khan <shuah.khan@hp.com>
> ---
>  lib/swiotlb.c |   65 +++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 47 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index 45bc1f8..f7d285c 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -15,6 +15,9 @@
>   * 05/09/10 linville	Add support for syncing ranges, support syncing for
>   *			DMA_BIDIRECTIONAL mappings, miscellaneous cleanup.
>   * 08/12/11 beckyb	Add highmem support
> + * 07/2012  shuahkhan	Disable iotlb overflow support when CONFIG_ISA
> + *			is enabled. Remove it for all configs when drivers
> + *			that don't check for mapping errors are fixed.
>   */
>  
>  #include <linux/cache.h>
> @@ -68,7 +71,19 @@ static unsigned long io_tlb_nslabs;
>  /*
>   * When the IOMMU overflows we return a fallback buffer. This sets the size.
>   */
> +#if defined(CONFIG_ISA)
> +/*
> + * Disable iotlb overflow support when CONFIG_ISA is enabled. This is the
> + * first step towards removing overflow support, to be consistent with other
> + * iommu implementations and return DMA_ERROR_CODE. This disabling step is
> + * for finding drivers that don't call dma_mapping_error to check for errors
> + * returned by the mapping interface. Once drivers are fixed overflow support
> + * can be removed.
> +*/
> +static unsigned long io_tlb_overflow;
> +#else
>  static unsigned long io_tlb_overflow = 32*1024;
> +#endif
>  
>  static void *io_tlb_overflow_buffer;
>  
> @@ -156,12 +171,16 @@ void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
>  	io_tlb_index = 0;
>  	io_tlb_orig_addr = alloc_bootmem_pages(PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
>  
> -	/*
> -	 * Get the overflow emergency buffer
> -	 */
> -	io_tlb_overflow_buffer = alloc_bootmem_low_pages(PAGE_ALIGN(io_tlb_overflow));
> -	if (!io_tlb_overflow_buffer)
> -		panic("Cannot allocate SWIOTLB overflow buffer!\n");
> +	if (io_tlb_overflow) {
> +		/*
> +		 * Get the overflow emergency buffer
> +		 */
> +		io_tlb_overflow_buffer = alloc_bootmem_low_pages(
> +						PAGE_ALIGN(io_tlb_overflow));
> +		if (!io_tlb_overflow_buffer)
> +			panic("Cannot allocate SWIOTLB overflow buffer!\n");
> +	}
> +
>  	if (verbose)
>  		swiotlb_print_info();
>  }
> @@ -264,13 +283,15 @@ swiotlb_late_init_with_default_size(size_t default_size)
>  
>  	memset(io_tlb_orig_addr, 0, io_tlb_nslabs * sizeof(phys_addr_t));
>  
> -	/*
> -	 * Get the overflow emergency buffer
> -	 */
> -	io_tlb_overflow_buffer = (void *)__get_free_pages(GFP_DMA,
> -	                                          get_order(io_tlb_overflow));
> -	if (!io_tlb_overflow_buffer)
> -		goto cleanup4;
> +	if (io_tlb_overflow) {
> +		/*
> +		 * Get the overflow emergency buffer
> +		 */
> +		io_tlb_overflow_buffer = (void *)
> +			__get_free_pages(GFP_DMA, get_order(io_tlb_overflow));
> +		if (!io_tlb_overflow_buffer)
> +			goto cleanup4;
> +	}
>  
>  	swiotlb_print_info();
>  
> @@ -297,12 +318,13 @@ cleanup1:
>  
>  void __init swiotlb_free(void)
>  {
> -	if (!io_tlb_overflow_buffer)
> +	if (!io_tlb_orig_addr)
>  		return;
>  
>  	if (late_alloc) {
> -		free_pages((unsigned long)io_tlb_overflow_buffer,
> -			   get_order(io_tlb_overflow));
> +		if (io_tlb_overflow_buffer)
> +			free_pages((unsigned long)io_tlb_overflow_buffer,
> +				   get_order(io_tlb_overflow));
>  		free_pages((unsigned long)io_tlb_orig_addr,
>  			   get_order(io_tlb_nslabs * sizeof(phys_addr_t)));
>  		free_pages((unsigned long)io_tlb_list, get_order(io_tlb_nslabs *
> @@ -310,8 +332,9 @@ void __init swiotlb_free(void)
>  		free_pages((unsigned long)io_tlb_start,
>  			   get_order(io_tlb_nslabs << IO_TLB_SHIFT));
>  	} else {
> -		free_bootmem_late(__pa(io_tlb_overflow_buffer),
> -				  PAGE_ALIGN(io_tlb_overflow));
> +		if (io_tlb_overflow_buffer)
> +			free_bootmem_late(__pa(io_tlb_overflow_buffer),
> +					  PAGE_ALIGN(io_tlb_overflow));
>  		free_bootmem_late(__pa(io_tlb_orig_addr),
>  				  PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
>  		free_bootmem_late(__pa(io_tlb_list),
> @@ -681,6 +704,8 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
>  	map = map_single(dev, phys, size, dir);
>  	if (!map) {
>  		swiotlb_full(dev, size, dir, 1);
> +		if (!io_tlb_overflow)
> +			return DMA_ERROR_CODE;
>  		map = io_tlb_overflow_buffer;
>  	}
>  
> @@ -691,6 +716,8 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
>  	 */
>  	if (!dma_capable(dev, dev_addr, size)) {
>  		swiotlb_tbl_unmap_single(dev, map, size, dir);
> +		if (!io_tlb_overflow)
> +			return DMA_ERROR_CODE;
>  		dev_addr = swiotlb_virt_to_bus(dev, io_tlb_overflow_buffer);
>  	}
>  
> @@ -910,6 +937,8 @@ EXPORT_SYMBOL(swiotlb_sync_sg_for_device);
>  int
>  swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t dma_addr)
>  {
> +	if (!io_tlb_overflow)
> +		return DMA_ERROR_CODE;
>  	return (dma_addr == swiotlb_virt_to_bus(hwdev, io_tlb_overflow_buffer));
>  }
>  EXPORT_SYMBOL(swiotlb_dma_mapping_error);
> -- 
> 1.7.9.5
> 
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH RFC] swiotlb: Disable swiotlb overflow support when CONFIG_ISA is enabled
  2012-07-16 14:45         ` Konrad Rzeszutek Wilk
@ 2012-07-16 15:48           ` Shuah Khan
  2012-07-16 16:01             ` Konrad Rzeszutek Wilk
  0 siblings, 1 reply; 14+ messages in thread
From: Shuah Khan @ 2012-07-16 15:48 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: fujita.tomonori, LKML, akpm, paul.gortmaker, bhelgaas, amwang, shuahkhan

On Mon, 2012-07-16 at 10:45 -0400, Konrad Rzeszutek Wilk wrote:
> On Thu, Jul 12, 2012 at 10:17:50AM -0600, Shuah Khan wrote:
> > Disable iotlb overflow support when CONFIG_ISA is enabled. This is the
> > first step towards removing overflow support, to be consistent with other
> > iommu implementations and return DMA_ERROR_CODE. This disabling step is
> > for finding drivers that don't call dma_mapping_error to check for errors
> > returned by the mapping interface. Once drivers are fixed overflow support
> > can be removed.
> 
> I think I explained myself poorly. We don't want to break old drivers.
> 
> If the old drivers expect this behavior (while they should be updated
> to check the DMA), we need to retain this behavior.
> 
> So I think the patch should be done the other way:
> 
> Disable IOTLB overflow when CONFIG_ISA is disabled.

That makes lot of sense :) I will redo the patch. It might be good to a
tunable to control enable/disable behavior. Even though support for this
new tunable "swiotlb_overflow" projected to be short lived, it will be
lot less painful to disable/enable as needed. Thought and comments?

> 
> But we also need to check whether there are other drivers
> that don't properly check the DMA address. And if so, add them
> to this list of must have enabled b/c you might be using this
> driver.

Sound good. Again having a tunable would help in this case as well.
> 
> The first goal is to figure out which of the drivers aren't doing this
> properly. This should be possible by just grepping for 'dma_map' and 
> seeing which ones don't do the 'dma_check' right after.

I started some research into this and will continue and provide an
update.

-- Shuah


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH RFC] swiotlb: Disable swiotlb overflow support when CONFIG_ISA is enabled
  2012-07-16 15:48           ` Shuah Khan
@ 2012-07-16 16:01             ` Konrad Rzeszutek Wilk
  2012-07-16 16:47               ` Shuah Khan
  2012-07-17 18:27               ` Shuah Khan
  0 siblings, 2 replies; 14+ messages in thread
From: Konrad Rzeszutek Wilk @ 2012-07-16 16:01 UTC (permalink / raw)
  To: Shuah Khan
  Cc: fujita.tomonori, LKML, akpm, paul.gortmaker, bhelgaas, amwang, shuahkhan

On Mon, Jul 16, 2012 at 09:48:20AM -0600, Shuah Khan wrote:
> On Mon, 2012-07-16 at 10:45 -0400, Konrad Rzeszutek Wilk wrote:
> > On Thu, Jul 12, 2012 at 10:17:50AM -0600, Shuah Khan wrote:
> > > Disable iotlb overflow support when CONFIG_ISA is enabled. This is the
> > > first step towards removing overflow support, to be consistent with other
> > > iommu implementations and return DMA_ERROR_CODE. This disabling step is
> > > for finding drivers that don't call dma_mapping_error to check for errors
> > > returned by the mapping interface. Once drivers are fixed overflow support
> > > can be removed.
> > 
> > I think I explained myself poorly. We don't want to break old drivers.
> > 
> > If the old drivers expect this behavior (while they should be updated
> > to check the DMA), we need to retain this behavior.
> > 
> > So I think the patch should be done the other way:
> > 
> > Disable IOTLB overflow when CONFIG_ISA is disabled.
> 
> That makes lot of sense :) I will redo the patch. It might be good to a
> tunable to control enable/disable behavior. Even though support for this
> new tunable "swiotlb_overflow" projected to be short lived, it will be
> lot less painful to disable/enable as needed. Thought and comments?

Tunnable have a habit of becoming permanant and we really don't want that.
The right solution is to drop the overflow - as you have rightly
pointed out in the first patch. But the road is blocked by these old
drivers that are making it hard.

I would say if you really really want a patch in for 3.6 before the
final mega-patch of drivers-that-suck-and-need-to-be-updated-here-is
-the-big-patch-that-does-it, then just add a WARN mentioning your
name and mine (and LKML) saying to report what you see. That way
we can at least get feedback from the field.

And for good measure include in the Documentation/*deprecate-schedule
that the overflow functionality is going away when all the drivers that
depend on it have been fixed up.

> 
> > 
> > But we also need to check whether there are other drivers
> > that don't properly check the DMA address. And if so, add them
> > to this list of must have enabled b/c you might be using this
> > driver.
> 
> Sound good. Again having a tunable would help in this case as well.
> > 
> > The first goal is to figure out which of the drivers aren't doing this
> > properly. This should be possible by just grepping for 'dma_map' and 
> > seeing which ones don't do the 'dma_check' right after.
> 
> I started some research into this and will continue and provide an
> update.

Thank you! It is pretty griddy work so I really appreciate you taking
the time to do this. If are at the LinuxCon this year the beers (or coffee
if that is your preference) are on me for tackling on this cleanup.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH RFC] swiotlb: Disable swiotlb overflow support when CONFIG_ISA is enabled
  2012-07-16 16:01             ` Konrad Rzeszutek Wilk
@ 2012-07-16 16:47               ` Shuah Khan
  2012-07-17 18:27               ` Shuah Khan
  1 sibling, 0 replies; 14+ messages in thread
From: Shuah Khan @ 2012-07-16 16:47 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: fujita.tomonori, LKML, akpm, paul.gortmaker, bhelgaas, amwang, shuahkhan

On Mon, 2012-07-16 at 12:01 -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Jul 16, 2012 at 09:48:20AM -0600, Shuah Khan wrote:
> > On Mon, 2012-07-16 at 10:45 -0400, Konrad Rzeszutek Wilk wrote:
> > > On Thu, Jul 12, 2012 at 10:17:50AM -0600, Shuah Khan wrote:
> > > > Disable iotlb overflow support when CONFIG_ISA is enabled. This is the
> > > > first step towards removing overflow support, to be consistent with other
> > > > iommu implementations and return DMA_ERROR_CODE. This disabling step is
> > > > for finding drivers that don't call dma_mapping_error to check for errors
> > > > returned by the mapping interface. Once drivers are fixed overflow support
> > > > can be removed.
> > > 
> > > I think I explained myself poorly. We don't want to break old drivers.
> > > 
> > > If the old drivers expect this behavior (while they should be updated
> > > to check the DMA), we need to retain this behavior.
> > > 
> > > So I think the patch should be done the other way:
> > > 
> > > Disable IOTLB overflow when CONFIG_ISA is disabled.
> > 
> > That makes lot of sense :) I will redo the patch. It might be good to a
> > tunable to control enable/disable behavior. Even though support for this
> > new tunable "swiotlb_overflow" projected to be short lived, it will be
> > lot less painful to disable/enable as needed. Thought and comments?
> 
> Tunnable have a habit of becoming permanant and we really don't want that.
> The right solution is to drop the overflow - as you have rightly
> pointed out in the first patch. But the road is blocked by these old
> drivers that are making it hard.
> 
> I would say if you really really want a patch in for 3.6 before the
> final mega-patch of drivers-that-suck-and-need-to-be-updated-here-is
> -the-big-patch-that-does-it, then just add a WARN mentioning your
> name and mine (and LKML) saying to report what you see. That way
> we can at least get feedback from the field.
> 
> And for good measure include in the Documentation/*deprecate-schedule
> that the overflow functionality is going away when all the drivers that
> depend on it have been fixed up.

Will work on it and get the patch with deprecation schedule document out
soon.

> 
> > 
> > > 
> > > But we also need to check whether there are other drivers
> > > that don't properly check the DMA address. And if so, add them
> > > to this list of must have enabled b/c you might be using this
> > > driver.
> > 
> > Sound good. Again having a tunable would help in this case as well.
> > > 
> > > The first goal is to figure out which of the drivers aren't doing this
> > > properly. This should be possible by just grepping for 'dma_map' and 
> > > seeing which ones don't do the 'dma_check' right after.
> > 
> > I started some research into this and will continue and provide an
> > update.
> 
> Thank you! It is pretty griddy work so I really appreciate you taking
> the time to do this. If are at the LinuxCon this year the beers (or coffee
> if that is your preference) are on me for tackling on this cleanup.

Thanks. Unfortunately I won't be at this year's LinuxCon, maybe I get to
take you up on your offer of coffee at one of the future ones.

-- Shuah


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH RFC] swiotlb: Disable swiotlb overflow support when CONFIG_ISA is enabled
  2012-07-16 16:01             ` Konrad Rzeszutek Wilk
  2012-07-16 16:47               ` Shuah Khan
@ 2012-07-17 18:27               ` Shuah Khan
  2012-07-17 20:13                 ` [PATCH] swiotlb: Disable swiotlb overflow support when CONFIG_ISA is disabled Shuah Khan
  1 sibling, 1 reply; 14+ messages in thread
From: Shuah Khan @ 2012-07-17 18:27 UTC (permalink / raw)
  To: Konrad Rzeszutek Wilk
  Cc: fujita.tomonori, LKML, akpm, paul.gortmaker, bhelgaas, amwang, shuahkhan

On Mon, 2012-07-16 at 12:01 -0400, Konrad Rzeszutek Wilk wrote:

> > > The first goal is to figure out which of the drivers aren't doing this
> > > properly. This should be possible by just grepping for 'dma_map' and 
> > > seeing which ones don't do the 'dma_check' right after.
> > 

I did cscope searches two different ways on linux-next July 12th git

map_page - in this case I looked at all of the dma_ops.map_page() calls
to see if they are followed by dma_mapping_error() calls. Found the
following cases that don't check map_page() return value.

arch/powerpc/platforms/cell/iommu.c - dma_fixed_map_page()
drivers/net/ethernet/sun/niu.c - niu_rbr_add_page()
net/sunrpc/xprtrdma/svc_rdma_sendto.c - dma_map_xdr()

dma_map - in this case again I looked for calls to dma_map(). Found the
following cases where return isn't checked:

drivers/atm/fore200e.c
drivers/isdn/hardware/eicon/di.c

I am somewhat surprised to find only a few cases. Hoping I covered all
the bases, can you think of anything else I should look for before
concluding these are the cases that need fixing?

-- Shuah


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH] swiotlb: Disable swiotlb overflow support when CONFIG_ISA is disabled
  2012-07-17 18:27               ` Shuah Khan
@ 2012-07-17 20:13                 ` Shuah Khan
  0 siblings, 0 replies; 14+ messages in thread
From: Shuah Khan @ 2012-07-17 20:13 UTC (permalink / raw)
  To: fujita.tomonori, akpm, paul.gortmaker, bhelgaas, amwang,
	Konrad Rzeszutek Wilk, rob
  Cc: LKML, linux-doc, shuahkhan

Disable iotlb overflow support when CONFIG_ISA is disabled. Add deprecation
notice warning message and deprecation schedule documentation. This is the
first step towards removing overflow support, to be consistent with other
iommu implementations and return DMA_ERROR_CODE. This disabling step is for
finding drivers that don't call dma_mapping_error to check for errors returned
by the mapping interface. Once drivers are fixed overflow support can be
removed.

Signed-off-by: Shuah Khan <shuah.khan@hp.com>
---
 Documentation/feature-removal-schedule.txt |   22 ++++++--
 lib/swiotlb.c                              |   79 +++++++++++++++++++++-------
 2 files changed, 79 insertions(+), 22 deletions(-)

diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
index 5979c3e..ce6c0ae 100644
--- a/Documentation/feature-removal-schedule.txt
+++ b/Documentation/feature-removal-schedule.txt
@@ -35,7 +35,7 @@ What:	x86_32 "no-hlt" cmdline param
 When:	2012
 Why:	remove a branch from idle path, simplify code used by everybody.
 	This option disabled the use of HLT in idle and machine_halt()
-	for hardware that was flakey 15-years ago.  Today we have
+	for hardware that was flaky 15-years ago.  Today we have
 	"idle=poll" that removed HLT from idle, and so if such a machine
 	is still running the upstream kernel, "idle=poll" is likely sufficient.
 Who:	Len Brown <len.brown@intel.com>
@@ -160,7 +160,7 @@ Files:	arch/*/kernel/*_ksyms.c
 Check:	kernel_thread
 Why:	kernel_thread is a low-level implementation detail.  Drivers should
         use the <linux/kthread.h> API instead which shields them from
-	implementation details and provides a higherlevel interface that
+	implementation details and provides a higher level interface that
 	prevents bugs and code duplication
 Who:	Christoph Hellwig <hch@lst.de>
 
@@ -236,7 +236,7 @@ Who:	David Brownell <dbrownell@users.sourceforge.net>
 
 What:	b43 support for firmware revision < 410
 When:	The schedule was July 2008, but it was decided that we are going to keep the
-        code as long as there are no major maintanance headaches.
+        code as long as there are no major maintenance headaches.
 	So it _could_ be removed _any_ time now, if it conflicts with something new.
 Why:	The support code for the old firmware hurts code readability/maintainability
 	and slightly hurts runtime performance. Bugfixes for the old firmware
@@ -608,3 +608,19 @@ When:	June 2013
 Why:	Unsupported/unmaintained/unused since 2.6
 
 ----------------------------
+
+What:	SWIOTLB overflow buffer support.
+When:	3.8
+Why:	Remove SWIOTLB overflow buffer support and return DMA_ERROR_CODE
+	(a value of zero) to make it consistent with iommu implementation
+	on Intel, AMD, and swiotlb-xen. In 3.6, Disable iotlb overflow
+	support when CONFIG_ISA is disabled with the intent to find drivers
+	that don't call dma_mapping_error to check for errors returned by the
+	mapping interface. Once drivers are fixed overflow support can be
+	removed.
+	If you see any problems related to disabling SWIOTLB overflow buffer,
+	please report to us!
+	E-mail us at: linux-kernel@vger.kernel.org
+Who:	Shuah Khan <shuah.khan@hp.com> <shuahkhan@gmail.com>
+
+----------------------------
diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 45bc1f8..0123bb8 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -15,6 +15,9 @@
  * 05/09/10 linville	Add support for syncing ranges, support syncing for
  *			DMA_BIDIRECTIONAL mappings, miscellaneous cleanup.
  * 08/12/11 beckyb	Add highmem support
+ * 07/2012  shuahkhan	Disable iotlb overflow support when CONFIG_ISA
+ *			is enabled. Remove it for all configs when drivers
+ *			that don't check for mapping errors are fixed.
  */
 
 #include <linux/cache.h>
@@ -68,7 +71,11 @@ static unsigned long io_tlb_nslabs;
 /*
  * When the IOMMU overflows we return a fallback buffer. This sets the size.
  */
+#if defined(CONFIG_ISA)
 static unsigned long io_tlb_overflow = 32*1024;
+#else
+static unsigned long io_tlb_overflow;
+#endif
 
 static void *io_tlb_overflow_buffer;
 
@@ -92,6 +99,24 @@ static DEFINE_SPINLOCK(io_tlb_lock);
 
 static int late_alloc;
 
+static void swiotlb_print_overflow_deprecation_notice(void)
+{
+	if (io_tlb_overflow) {
+		pr_warn("SWIOTLB overflow buffer will be deprecated.\n"
+			"  If you have a driver that depends on this feature\n"
+			"  please Email us at: linux-kernel@vger.kernel.org,\n"
+			"  Shuah Khan (shuahkhan@gmail.com), and\n"
+			"  Konrad Wilk (konrad.wilk@oracle.com)\n");
+	} else {
+		pr_warn("SWIOTLB overflow buffer is disabled and will be\n"
+			"  deprecated. Please report problems related to\n"
+			"  disabling overflow buffer to\n"
+			"  linux-kernel@vger.kernel.org,\n"
+			"  Shuah Khan (shuahkhan@gmail.com), and\n"
+			"  Konrad Wilk (konrad.wilk@oracle.com)\n");
+	}
+}
+
 static int __init
 setup_io_tlb_npages(char *str)
 {
@@ -108,7 +133,6 @@ setup_io_tlb_npages(char *str)
 	return 1;
 }
 __setup("swiotlb=", setup_io_tlb_npages);
-/* make io_tlb_overflow tunable too? */
 
 unsigned long swiotlb_nr_tbl(void)
 {
@@ -156,12 +180,18 @@ void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 	io_tlb_index = 0;
 	io_tlb_orig_addr = alloc_bootmem_pages(PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
 
-	/*
-	 * Get the overflow emergency buffer
-	 */
-	io_tlb_overflow_buffer = alloc_bootmem_low_pages(PAGE_ALIGN(io_tlb_overflow));
-	if (!io_tlb_overflow_buffer)
-		panic("Cannot allocate SWIOTLB overflow buffer!\n");
+	if (io_tlb_overflow) {
+		/*
+		 * Get the overflow emergency buffer
+		 */
+		io_tlb_overflow_buffer = alloc_bootmem_low_pages(
+						PAGE_ALIGN(io_tlb_overflow));
+		if (!io_tlb_overflow_buffer)
+			panic("Cannot allocate SWIOTLB overflow buffer!\n");
+	}
+
+	swiotlb_print_overflow_deprecation_notice();
+
 	if (verbose)
 		swiotlb_print_info();
 }
@@ -264,14 +294,17 @@ swiotlb_late_init_with_default_size(size_t default_size)
 
 	memset(io_tlb_orig_addr, 0, io_tlb_nslabs * sizeof(phys_addr_t));
 
-	/*
-	 * Get the overflow emergency buffer
-	 */
-	io_tlb_overflow_buffer = (void *)__get_free_pages(GFP_DMA,
-	                                          get_order(io_tlb_overflow));
-	if (!io_tlb_overflow_buffer)
-		goto cleanup4;
+	if (io_tlb_overflow) {
+		/*
+		 * Get the overflow emergency buffer
+		 */
+		io_tlb_overflow_buffer = (void *)
+			__get_free_pages(GFP_DMA, get_order(io_tlb_overflow));
+		if (!io_tlb_overflow_buffer)
+			goto cleanup4;
+	}
 
+	swiotlb_print_overflow_deprecation_notice();
 	swiotlb_print_info();
 
 	late_alloc = 1;
@@ -297,12 +330,13 @@ cleanup1:
 
 void __init swiotlb_free(void)
 {
-	if (!io_tlb_overflow_buffer)
+	if (!io_tlb_orig_addr)
 		return;
 
 	if (late_alloc) {
-		free_pages((unsigned long)io_tlb_overflow_buffer,
-			   get_order(io_tlb_overflow));
+		if (io_tlb_overflow_buffer)
+			free_pages((unsigned long)io_tlb_overflow_buffer,
+				   get_order(io_tlb_overflow));
 		free_pages((unsigned long)io_tlb_orig_addr,
 			   get_order(io_tlb_nslabs * sizeof(phys_addr_t)));
 		free_pages((unsigned long)io_tlb_list, get_order(io_tlb_nslabs *
@@ -310,8 +344,9 @@ void __init swiotlb_free(void)
 		free_pages((unsigned long)io_tlb_start,
 			   get_order(io_tlb_nslabs << IO_TLB_SHIFT));
 	} else {
-		free_bootmem_late(__pa(io_tlb_overflow_buffer),
-				  PAGE_ALIGN(io_tlb_overflow));
+		if (io_tlb_overflow_buffer)
+			free_bootmem_late(__pa(io_tlb_overflow_buffer),
+					  PAGE_ALIGN(io_tlb_overflow));
 		free_bootmem_late(__pa(io_tlb_orig_addr),
 				  PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
 		free_bootmem_late(__pa(io_tlb_list),
@@ -681,6 +716,8 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 	map = map_single(dev, phys, size, dir);
 	if (!map) {
 		swiotlb_full(dev, size, dir, 1);
+		if (!io_tlb_overflow)
+			return DMA_ERROR_CODE;
 		map = io_tlb_overflow_buffer;
 	}
 
@@ -691,6 +728,8 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 	 */
 	if (!dma_capable(dev, dev_addr, size)) {
 		swiotlb_tbl_unmap_single(dev, map, size, dir);
+		if (!io_tlb_overflow)
+			return DMA_ERROR_CODE;
 		dev_addr = swiotlb_virt_to_bus(dev, io_tlb_overflow_buffer);
 	}
 
@@ -910,6 +949,8 @@ EXPORT_SYMBOL(swiotlb_sync_sg_for_device);
 int
 swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t dma_addr)
 {
+	if (!io_tlb_overflow)
+		return DMA_ERROR_CODE;
 	return (dma_addr == swiotlb_virt_to_bus(hwdev, io_tlb_overflow_buffer));
 }
 EXPORT_SYMBOL(swiotlb_dma_mapping_error);
-- 
1.7.9.5




^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2012-07-17 20:14 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-06 23:06 [PATCH RFC] swiotlb: Remove SWIOTLB overflow buffer support Shuah Khan
2012-07-09 20:25 ` Konrad Rzeszutek Wilk
2012-07-10 16:33   ` FUJITA Tomonori
2012-07-10 16:55   ` Shuah Khan
2012-07-10 17:32     ` Konrad Rzeszutek Wilk
2012-07-10 23:06       ` Shuah Khan
2012-07-10 23:13         ` Shuah Khan
2012-07-12 16:17       ` [PATCH RFC] swiotlb: Disable swiotlb overflow support when CONFIG_ISA is enabled Shuah Khan
2012-07-16 14:45         ` Konrad Rzeszutek Wilk
2012-07-16 15:48           ` Shuah Khan
2012-07-16 16:01             ` Konrad Rzeszutek Wilk
2012-07-16 16:47               ` Shuah Khan
2012-07-17 18:27               ` Shuah Khan
2012-07-17 20:13                 ` [PATCH] swiotlb: Disable swiotlb overflow support when CONFIG_ISA is disabled Shuah Khan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.