[PATCH 0/5] swiotlb: changes for powerpc/highmem

[PATCH 0/5] swiotlb: changes for powerpc/highmem

[Becky Bruce]

This is a series of fairly minor patches that get swiotlb working
on 32-bit powerpc systems with HIGHMEM, plus some cleanup of the
outdated comments in the code.  I've made a couple of things
weak that ppc needs to override, and have changed the prototypes
for a couple of functions to include the hwdev pointer, which
we need to ppc to convert bus addresses to and from phys/virt
addresses.  I've also fixed a build warning I've been seeing on
ppc.

I have not tested this in any way on any non-ppc platforms,
so commentary/testing from x86/ia64 folks is greatly 
appreciated.

diffstat:
 arch/x86/kernel/pci-swiotlb.c |    2 +-
 include/linux/swiotlb.h       |    4 ++-
 lib/swiotlb.c                 |   70 +++++++++++++++++++++++-----------------
 3 files changed, 44 insertions(+), 32 deletions(-)

Cheers,
Becky

[PATCH 1/5] swiotlb: comment corrections (no code changes)

[Becky Bruce]

swiotlb_map/unmap_single are now swiotlb_map/unmap_page;
trivially change all the comments to reference new names.
Also, there were some comments that should have been
referring to just plain old map_single, not swiotlb_map_single;
fix those as well. Also change a use of the word "pointer", when
what is referred to is actually a dma/physical address.

Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
---
 lib/swiotlb.c |   24 +++++++++++-------------
 1 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 32e2bd3..f59cf30 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -60,8 +60,8 @@ enum dma_sync_target {
 int swiotlb_force;
 
 /*
- * Used to do a quick range check in swiotlb_unmap_single and
- * swiotlb_sync_single_*, to see if the memory was in fact allocated by this
+ * Used to do a quick range check in unmap_single and
+ * sync_single_*, to see if the memory was in fact allocated by this
  * API.
  */
 static char *io_tlb_start, *io_tlb_end;
@@ -560,7 +560,6 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 				   size)) {
 		/*
 		 * The allocated memory isn't reachable by the device.
-		 * Fall back on swiotlb_map_single().
 		 */
 		free_pages((unsigned long) ret, order);
 		ret = NULL;
@@ -568,9 +567,8 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 	if (!ret) {
 		/*
 		 * We are either out of memory or the device can't DMA
-		 * to GFP_DMA memory; fall back on
-		 * swiotlb_map_single(), which will grab memory from
-		 * the lowest available address range.
+		 * to GFP_DMA memory; fall back on map_single(), which
+		 * will grab memory from the lowest available address range.
 		 */
 		ret = map_single(hwdev, 0, size, DMA_FROM_DEVICE);
 		if (!ret)
@@ -634,7 +632,7 @@ swiotlb_full(struct device *dev, size_t size, int dir, int do_panic)
  * physical address to use is returned.
  *
  * Once the device is given the dma address, the device owns this memory until
- * either swiotlb_unmap_single or swiotlb_dma_sync_single is performed.
+ * either swiotlb_unmap_page or swiotlb_dma_sync_single is performed.
  */
 dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 			    unsigned long offset, size_t size,
@@ -648,7 +646,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 
 	BUG_ON(dir == DMA_NONE);
 	/*
-	 * If the pointer passed in happens to be in the device's DMA window,
+	 * If the address happens to be in the device's DMA window,
 	 * we can safely return the device addr and not worry about bounce
 	 * buffering it.
 	 */
@@ -679,7 +677,7 @@ EXPORT_SYMBOL_GPL(swiotlb_map_page);
 
 /*
  * Unmap a single streaming mode DMA translation.  The dma_addr and size must
- * match what was provided for in a previous swiotlb_map_single call.  All
+ * match what was provided for in a previous swiotlb_map_page call.  All
  * other usages are undefined.
  *
  * After this call, reads by the cpu to the buffer are guaranteed to see
@@ -703,7 +701,7 @@ EXPORT_SYMBOL_GPL(swiotlb_unmap_page);
  * Make physical memory consistent for a single streaming mode DMA translation
  * after a transfer.
  *
- * If you perform a swiotlb_map_single() but wish to interrogate the buffer
+ * If you perform a swiotlb_map_page() but wish to interrogate the buffer
  * using the cpu, yet do not wish to teardown the dma mapping, you must
  * call this function before doing so.  At the next point you give the dma
  * address back to the card, you must first perform a
@@ -777,7 +775,7 @@ EXPORT_SYMBOL_GPL(swiotlb_sync_single_range_for_device);
 
 /*
  * Map a set of buffers described by scatterlist in streaming mode for DMA.
- * This is the scatter-gather version of the above swiotlb_map_single
+ * This is the scatter-gather version of the above swiotlb_map_page
  * interface.  Here the scatter gather list elements are each tagged with the
  * appropriate dma address and length.  They are obtained via
  * sg_dma_{address,length}(SG).
@@ -788,7 +786,7 @@ EXPORT_SYMBOL_GPL(swiotlb_sync_single_range_for_device);
  *       The routine returns the number of addr/length pairs actually
  *       used, at most nents.
  *
- * Device ownership issues as mentioned above for swiotlb_map_single are the
+ * Device ownership issues as mentioned above for swiotlb_map_page are the
  * same here.
  */
 int
@@ -836,7 +834,7 @@ EXPORT_SYMBOL(swiotlb_map_sg);
 
 /*
  * Unmap a set of streaming mode DMA translations.  Again, cpu read rules
- * concerning calls here are the same as for swiotlb_unmap_single() above.
+ * concerning calls here are the same as for swiotlb_unmap_page() above.
  */
 void
 swiotlb_unmap_sg_attrs(struct device *hwdev, struct scatterlist *sgl,
-- 
1.5.6.6

[PATCH 2/5] swiotlb: fix compile warning

[Becky Bruce]

Squash a build warning seen on 32-bit powerpc caused by calling min()
with 2 different types.  Cast the first arg to size_t, which is the
type of the second, and should be portable across architectures.

Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
---
 lib/swiotlb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index f59cf30..62f5f75 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -341,7 +341,7 @@ static void swiotlb_bounce(phys_addr_t phys, char *dma_addr, size_t size,
 		unsigned long flags;
 
 		while (size) {
-			sz = min(PAGE_SIZE - offset, size);
+			sz = min((size_t)(PAGE_SIZE - offset), size);
 
 			local_irq_save(flags);
 			buffer = kmap_atomic(pfn_to_page(pfn),
-- 
1.5.6.6

[PATCH 3/5] swiotlb: swiotlb_bus_to_virt/phys prototype changes

[Becky Bruce]

Make these functions take the hwdev as an argument because on some
platforms it contains a per-device offset that is used to convert
from bus addresses to/from other types of addresses.

Also, make these weak so architectures can override the default
behavior (for example, by adding an offset in the hwdev).

Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
---
 arch/x86/kernel/pci-swiotlb.c |    2 +-
 include/linux/swiotlb.h       |    4 +++-
 lib/swiotlb.c                 |   31 +++++++++++++++++++------------
 3 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/pci-swiotlb.c b/arch/x86/kernel/pci-swiotlb.c
index 34f12e9..887388a 100644
--- a/arch/x86/kernel/pci-swiotlb.c
+++ b/arch/x86/kernel/pci-swiotlb.c
@@ -28,7 +28,7 @@ dma_addr_t swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr)
 	return paddr;
 }
 
-phys_addr_t swiotlb_bus_to_phys(dma_addr_t baddr)
+phys_addr_t swiotlb_bus_to_phys(struct device *hwdev, dma_addr_t baddr)
 {
 	return baddr;
 }
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index ac9ff54..a27bca3 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -29,7 +29,9 @@ extern void *swiotlb_alloc(unsigned order, unsigned long nslabs);
 
 extern dma_addr_t swiotlb_phys_to_bus(struct device *hwdev,
 				      phys_addr_t address);
-extern phys_addr_t swiotlb_bus_to_phys(dma_addr_t address);
+extern phys_addr_t swiotlb_bus_to_phys(struct device *hwdev,
+				       dma_addr_t address);
+extern void *swiotlb_bus_to_virt(struct device *hwdev, dma_addr_t address);
 
 extern int swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size);
 
diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 62f5f75..c152f17 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -129,20 +129,20 @@ dma_addr_t __weak swiotlb_phys_to_bus(struct device *hwdev, phys_addr_t paddr)
 	return paddr;
 }
 
-phys_addr_t __weak swiotlb_bus_to_phys(dma_addr_t baddr)
+phys_addr_t __weak swiotlb_bus_to_phys(struct device *hwdev, dma_addr_t baddr)
 {
 	return baddr;
 }
 
-static dma_addr_t swiotlb_virt_to_bus(struct device *hwdev,
+dma_addr_t __weak swiotlb_virt_to_bus(struct device *hwdev,
 				      volatile void *address)
 {
 	return swiotlb_phys_to_bus(hwdev, virt_to_phys(address));
 }
 
-static void *swiotlb_bus_to_virt(dma_addr_t address)
+void __weak *swiotlb_bus_to_virt(struct device *hwdev, dma_addr_t address)
 {
-	return phys_to_virt(swiotlb_bus_to_phys(address));
+	return phys_to_virt(swiotlb_bus_to_phys(hwdev, address));
 }
 
 int __weak swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size)
@@ -687,7 +687,7 @@ void swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
 			size_t size, enum dma_data_direction dir,
 			struct dma_attrs *attrs)
 {
-	char *dma_addr = swiotlb_bus_to_virt(dev_addr);
+	char *dma_addr = swiotlb_bus_to_virt(hwdev, dev_addr);
 
 	BUG_ON(dir == DMA_NONE);
 	if (is_swiotlb_buffer(dma_addr))
@@ -711,7 +711,7 @@ static void
 swiotlb_sync_single(struct device *hwdev, dma_addr_t dev_addr,
 		    size_t size, int dir, int target)
 {
-	char *dma_addr = swiotlb_bus_to_virt(dev_addr);
+	char *dma_addr = swiotlb_bus_to_virt(hwdev, dev_addr);
 
 	BUG_ON(dir == DMA_NONE);
 	if (is_swiotlb_buffer(dma_addr))
@@ -744,7 +744,7 @@ swiotlb_sync_single_range(struct device *hwdev, dma_addr_t dev_addr,
 			  unsigned long offset, size_t size,
 			  int dir, int target)
 {
-	char *dma_addr = swiotlb_bus_to_virt(dev_addr) + offset;
+	char *dma_addr = swiotlb_bus_to_virt(hwdev, dev_addr) + offset;
 
 	BUG_ON(dir == DMA_NONE);
 	if (is_swiotlb_buffer(dma_addr))
@@ -847,10 +847,14 @@ swiotlb_unmap_sg_attrs(struct device *hwdev, struct scatterlist *sgl,
 
 	for_each_sg(sgl, sg, nelems, i) {
 		if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
-			unmap_single(hwdev, swiotlb_bus_to_virt(sg->dma_address),
-				     sg->dma_length, dir);
+			unmap_single(
+				hwdev,
+				swiotlb_bus_to_virt(hwdev, sg->dma_address),
+				sg->dma_length, dir);
 		else if (dir == DMA_FROM_DEVICE)
-			dma_mark_clean(swiotlb_bus_to_virt(sg->dma_address), sg->dma_length);
+			dma_mark_clean(
+				swiotlb_bus_to_virt(hwdev, sg->dma_address),
+				sg->dma_length);
 	}
 }
 EXPORT_SYMBOL(swiotlb_unmap_sg_attrs);
@@ -881,10 +885,13 @@ swiotlb_sync_sg(struct device *hwdev, struct scatterlist *sgl,
 
 	for_each_sg(sgl, sg, nelems, i) {
 		if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
-			sync_single(hwdev, swiotlb_bus_to_virt(sg->dma_address),
+			sync_single(hwdev,
+				    swiotlb_bus_to_virt(hwdev, sg->dma_address),
 				    sg->dma_length, dir, target);
 		else if (dir == DMA_FROM_DEVICE)
-			dma_mark_clean(swiotlb_bus_to_virt(sg->dma_address), sg->dma_length);
+			dma_mark_clean(
+				swiotlb_bus_to_virt(hwdev, sg->dma_address),
+				sg->dma_length);
 	}
 }
 
-- 
1.5.6.6

[PATCH 4/5] swiotlb: map_page fix for highmem systems

[Becky Bruce]

The current code calls virt_to_phys() on address that might
be in highmem, which is bad.  This wasn't needed, anyway, because
we already have the physical address we need. Get rid of the
now-unused virtual address as well.

Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
---
 lib/swiotlb.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index c152f17..b33180e 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -640,7 +640,6 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 			    struct dma_attrs *attrs)
 {
 	phys_addr_t phys = page_to_phys(page) + offset;
-	void *ptr = page_address(page) + offset;
 	dma_addr_t dev_addr = swiotlb_phys_to_bus(dev, phys);
 	void *map;
 
@@ -651,7 +650,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 	 * buffering it.
 	 */
 	if (!address_needs_mapping(dev, dev_addr, size) &&
-	    !range_needs_mapping(virt_to_phys(ptr), size))
+	    !range_needs_mapping(phys, size))
 		return dev_addr;
 
 	/*
-- 
1.5.6.6

[PATCH 5/5] swiotlb: Allow arch override of address_needs_mapping

[Becky Bruce]

Some architectures require additional checking to determine
if a device can dma to an address and need to provide their
own address_needs_mapping..

Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
---
 lib/swiotlb.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index b33180e..f365735 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -145,6 +145,12 @@ void __weak *swiotlb_bus_to_virt(struct device *hwdev, dma_addr_t address)
 	return phys_to_virt(swiotlb_bus_to_phys(hwdev, address));
 }
 
+int __weak swiotlb_arch_address_needs_mapping(struct device *hwdev,
+					       dma_addr_t addr, size_t size)
+{
+	return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size);
+}
+
 int __weak swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size)
 {
 	return 0;
@@ -309,10 +315,10 @@ cleanup1:
 	return -ENOMEM;
 }
 
-static int
+static inline int
 address_needs_mapping(struct device *hwdev, dma_addr_t addr, size_t size)
 {
-	return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size);
+	return swiotlb_arch_address_needs_mapping(hwdev, addr, size);
 }
 
 static inline int range_needs_mapping(phys_addr_t paddr, size_t size)
-- 
1.5.6.6

Re: [PATCH 5/5] swiotlb: Allow arch override of address_needs_mapping

[FUJITA Tomonori]

On Tue, 24 Mar 2009 16:28:46 -0500
Becky Bruce <beckyb@kernel.crashing.org> wrote:

> Some architectures require additional checking to determine
> if a device can dma to an address and need to provide their
> own address_needs_mapping..
> 
> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> ---
>  lib/swiotlb.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index b33180e..f365735 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -145,6 +145,12 @@ void __weak *swiotlb_bus_to_virt(struct device *hwdev, dma_addr_t address)
>  	return phys_to_virt(swiotlb_bus_to_phys(hwdev, address));
>  }
>  
> +int __weak swiotlb_arch_address_needs_mapping(struct device *hwdev,
> +					       dma_addr_t addr, size_t size)
> +{
> +	return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size);
> +}
> +
>  int __weak swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size)
>  {
>  	return 0;
> @@ -309,10 +315,10 @@ cleanup1:
>  	return -ENOMEM;
>  }
>  
> -static int
> +static inline int
>  address_needs_mapping(struct device *hwdev, dma_addr_t addr, size_t size)
>  {
> -	return !is_buffer_dma_capable(dma_get_mask(hwdev), addr, size);
> +	return swiotlb_arch_address_needs_mapping(hwdev, addr, size);
>  }
>  
>  static inline int range_needs_mapping(phys_addr_t paddr, size_t size)

Looks fine to me. Though I guess that we could clean up
swiotlb_arch_range_needs_mapping and
swiotlb_arch_address_needs_mapping a bit (at least, the names are
confusing).

Re: [PATCH 4/5] swiotlb: map_page fix for highmem systems

[FUJITA Tomonori]

On Tue, 24 Mar 2009 16:28:45 -0500
Becky Bruce <beckyb@kernel.crashing.org> wrote:

> The current code calls virt_to_phys() on address that might
> be in highmem, which is bad.  This wasn't needed, anyway, because
> we already have the physical address we need. Get rid of the
> now-unused virtual address as well.
> 
> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> ---
>  lib/swiotlb.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index c152f17..b33180e 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -640,7 +640,6 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
>  			    struct dma_attrs *attrs)
>  {
>  	phys_addr_t phys = page_to_phys(page) + offset;
> -	void *ptr = page_address(page) + offset;
>  	dma_addr_t dev_addr = swiotlb_phys_to_bus(dev, phys);
>  	void *map;
>  
> @@ -651,7 +650,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
>  	 * buffering it.
>  	 */
>  	if (!address_needs_mapping(dev, dev_addr, size) &&
> -	    !range_needs_mapping(virt_to_phys(ptr), size))
> +	    !range_needs_mapping(phys, size))
>  		return dev_addr;

Acked-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

Re: [PATCH 3/5] swiotlb: swiotlb_bus_to_virt/phys prototype changes

[FUJITA Tomonori]

On Tue, 24 Mar 2009 16:28:44 -0500
Becky Bruce <beckyb@kernel.crashing.org> wrote:

> Make these functions take the hwdev as an argument because on some
> platforms it contains a per-device offset that is used to convert
> from bus addresses to/from other types of addresses.
> 
> Also, make these weak so architectures can override the default
> behavior (for example, by adding an offset in the hwdev).
> 
> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> ---
>  arch/x86/kernel/pci-swiotlb.c |    2 +-
>  include/linux/swiotlb.h       |    4 +++-
>  lib/swiotlb.c                 |   31 +++++++++++++++++++------------
>  3 files changed, 23 insertions(+), 14 deletions(-)

> @@ -847,10 +847,14 @@ swiotlb_unmap_sg_attrs(struct device *hwdev, struct scatterlist *sgl,
>  
>  	for_each_sg(sgl, sg, nelems, i) {
>  		if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
> -			unmap_single(hwdev, swiotlb_bus_to_virt(sg->dma_address),
> -				     sg->dma_length, dir);
> +			unmap_single(
> +				hwdev,
> +				swiotlb_bus_to_virt(hwdev, sg->dma_address),
> +				sg->dma_length, dir);
>  		else if (dir == DMA_FROM_DEVICE)
> -			dma_mark_clean(swiotlb_bus_to_virt(sg->dma_address), sg->dma_length);
> +			dma_mark_clean(
> +				swiotlb_bus_to_virt(hwdev, sg->dma_address),
> +				sg->dma_length);
>  	}

The coding style looks a bit odd to me. How about something like this?

for_each_sg(sgl, sg, nelems, i) {
	void *virt = swiotlb_bus_to_virt(hwdev, sg->dma_address),
	if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
		unmap_single(hwdev, virt, sg->dma_length, dir);
	else if (dir == DMA_FROM_DEVICE)
		dma_mark_clean(virt, sg->dma_length);


>  }
>  EXPORT_SYMBOL(swiotlb_unmap_sg_attrs);
> @@ -881,10 +885,13 @@ swiotlb_sync_sg(struct device *hwdev, struct scatterlist *sgl,
>  
>  	for_each_sg(sgl, sg, nelems, i) {
>  		if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
> -			sync_single(hwdev, swiotlb_bus_to_virt(sg->dma_address),
> +			sync_single(hwdev,
> +				    swiotlb_bus_to_virt(hwdev, sg->dma_address),
>  				    sg->dma_length, dir, target);
>  		else if (dir == DMA_FROM_DEVICE)
> -			dma_mark_clean(swiotlb_bus_to_virt(sg->dma_address), sg->dma_length);
> +			dma_mark_clean(
> +				swiotlb_bus_to_virt(hwdev, sg->dma_address),
> +				sg->dma_length);
>  	}

Ditto.


The rest looks fine to me.

Re: [PATCH 3/5] swiotlb: swiotlb_bus_to_virt/phys prototype changes

[Becky Bruce]

On Mar 24, 2009, at 9:58 PM, FUJITA Tomonori wrote:

> On Tue, 24 Mar 2009 16:28:44 -0500
> Becky Bruce <beckyb@kernel.crashing.org> wrote:
>
>> Make these functions take the hwdev as an argument because on some
>> platforms it contains a per-device offset that is used to convert
>> from bus addresses to/from other types of addresses.
>>
>> Also, make these weak so architectures can override the default
>> behavior (for example, by adding an offset in the hwdev).
>>
>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
>> ---
>> arch/x86/kernel/pci-swiotlb.c |    2 +-
>> include/linux/swiotlb.h       |    4 +++-
>> lib/swiotlb.c                 |   31 +++++++++++++++++++------------
>> 3 files changed, 23 insertions(+), 14 deletions(-)
>
>> @@ -847,10 +847,14 @@ swiotlb_unmap_sg_attrs(struct device *hwdev,  
>> struct scatterlist *sgl,
>>
>> 	for_each_sg(sgl, sg, nelems, i) {
>> 		if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
>> -			unmap_single(hwdev, swiotlb_bus_to_virt(sg->dma_address),
>> -				     sg->dma_length, dir);
>> +			unmap_single(
>> +				hwdev,
>> +				swiotlb_bus_to_virt(hwdev, sg->dma_address),
>> +				sg->dma_length, dir);
>> 		else if (dir == DMA_FROM_DEVICE)
>> -			dma_mark_clean(swiotlb_bus_to_virt(sg->dma_address), sg- 
>> >dma_length);
>> +			dma_mark_clean(
>> +				swiotlb_bus_to_virt(hwdev, sg->dma_address),
>> +				sg->dma_length);
>> 	}
>
> The coding style looks a bit odd to me. How about something like this?

>
>
> for_each_sg(sgl, sg, nelems, i) {
> 	void *virt = swiotlb_bus_to_virt(hwdev, sg->dma_address),
> 	if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
> 		unmap_single(hwdev, virt, sg->dma_length, dir);
> 	else if (dir == DMA_FROM_DEVICE)
> 		dma_mark_clean(virt, sg->dma_length);

Heh, I was trying to avoid > 80 character lines there, and it ended up  
looking a bit gross.  I originally had *exactly* what you suggest in  
my tree, but changed it, so I'm more than happy to change this back.

>
>
>
>> }
>> EXPORT_SYMBOL(swiotlb_unmap_sg_attrs);
>> @@ -881,10 +885,13 @@ swiotlb_sync_sg(struct device *hwdev, struct  
>> scatterlist *sgl,
>>
>> 	for_each_sg(sgl, sg, nelems, i) {
>> 		if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
>> -			sync_single(hwdev, swiotlb_bus_to_virt(sg->dma_address),
>> +			sync_single(hwdev,
>> +				    swiotlb_bus_to_virt(hwdev, sg->dma_address),
>> 				    sg->dma_length, dir, target);
>> 		else if (dir == DMA_FROM_DEVICE)
>> -			dma_mark_clean(swiotlb_bus_to_virt(sg->dma_address), sg- 
>> >dma_length);
>> +			dma_mark_clean(
>> +				swiotlb_bus_to_virt(hwdev, sg->dma_address),
>> +				sg->dma_length);
>> 	}
>
> Ditto.
>
>
> The rest looks fine to me.

Thanks for the review!

Cheers,
-Becky

Re: [PATCH 3/5] swiotlb: swiotlb_bus_to_virt/phys prototype changes

[Ingo Molnar]

* Becky Bruce <beckyb@kernel.crashing.org> wrote:

>
> On Mar 24, 2009, at 9:58 PM, FUJITA Tomonori wrote:
>
>> On Tue, 24 Mar 2009 16:28:44 -0500
>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
>>
>>> Make these functions take the hwdev as an argument because on some
>>> platforms it contains a per-device offset that is used to convert
>>> from bus addresses to/from other types of addresses.
>>>
>>> Also, make these weak so architectures can override the default
>>> behavior (for example, by adding an offset in the hwdev).
>>>
>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
>>> ---
>>> arch/x86/kernel/pci-swiotlb.c |    2 +-
>>> include/linux/swiotlb.h       |    4 +++-
>>> lib/swiotlb.c                 |   31 +++++++++++++++++++------------
>>> 3 files changed, 23 insertions(+), 14 deletions(-)
>>
>>> @@ -847,10 +847,14 @@ swiotlb_unmap_sg_attrs(struct device *hwdev,  
>>> struct scatterlist *sgl,
>>>
>>> 	for_each_sg(sgl, sg, nelems, i) {
>>> 		if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
>>> -			unmap_single(hwdev, swiotlb_bus_to_virt(sg->dma_address),
>>> -				     sg->dma_length, dir);
>>> +			unmap_single(
>>> +				hwdev,
>>> +				swiotlb_bus_to_virt(hwdev, sg->dma_address),
>>> +				sg->dma_length, dir);
>>> 		else if (dir == DMA_FROM_DEVICE)
>>> -			dma_mark_clean(swiotlb_bus_to_virt(sg->dma_address), sg- 
>>> >dma_length);
>>> +			dma_mark_clean(
>>> +				swiotlb_bus_to_virt(hwdev, sg->dma_address),
>>> +				sg->dma_length);
>>> 	}
>>
>> The coding style looks a bit odd to me. How about something like this?
>
>>
>>
>> for_each_sg(sgl, sg, nelems, i) {
>> 	void *virt = swiotlb_bus_to_virt(hwdev, sg->dma_address),
>> 	if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
>> 		unmap_single(hwdev, virt, sg->dma_length, dir);
>> 	else if (dir == DMA_FROM_DEVICE)
>> 		dma_mark_clean(virt, sg->dma_length);

that looks odd too.

> Heh, I was trying to avoid > 80 character lines there, and it 
> ended up looking a bit gross.  I originally had *exactly* what you 
> suggest in my tree, but changed it, so I'm more than happy to 
> change this back.

The proper solution is to split out the loop body into a helper 
function, __swiotlb_unmap_sg_attrs() or so. Something like:

__swiotlb_unmap_sg_attrs()
{
	if (sg->dma_address != swiotlb_phys_to_bus(hwdev, sg_phys(sg)))
		unmap_single(hwdev,
			swiotlb_bus_to_virt(hwdev, sg->dma_address),
			sg->dma_length, dir);
		return;
	}

	if (dir != DMA_FROM_DEVICE)
		return;

	dma_mark_clean(swiotlb_bus_to_virt(hwdev, sg->dma_address),
		       sg->dma_length);
}

	Ingo

Re: [PATCH 2/5] swiotlb: fix compile warning

[FUJITA Tomonori]

On Tue, 24 Mar 2009 16:28:43 -0500
Becky Bruce <beckyb@kernel.crashing.org> wrote:

> Squash a build warning seen on 32-bit powerpc caused by calling min()
> with 2 different types.  Cast the first arg to size_t, which is the
> type of the second, and should be portable across architectures.
> 
> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> ---
>  lib/swiotlb.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index f59cf30..62f5f75 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -341,7 +341,7 @@ static void swiotlb_bounce(phys_addr_t phys, char *dma_addr, size_t size,
>  		unsigned long flags;
>  
>  		while (size) {
> -			sz = min(PAGE_SIZE - offset, size);
> +			sz = min((size_t)(PAGE_SIZE - offset), size);
>  
>  			local_irq_save(flags);
>  			buffer = kmap_atomic(pfn_to_page(pfn),

?

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index f59cf30..fa62498 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -341,7 +341,7 @@ static void swiotlb_bounce(phys_addr_t phys, char *dma_addr, size_t size,
 		unsigned long flags;
 
 		while (size) {
-			sz = min(PAGE_SIZE - offset, size);
+			sz = min_t(size_t, PAGE_SIZE - offset, size);
 
 			local_irq_save(flags);
 			buffer = kmap_atomic(pfn_to_page(pfn),

Re: [PATCH 2/5] swiotlb: fix compile warning

[Becky Bruce]

On Mar 24, 2009, at 9:58 PM, FUJITA Tomonori wrote:

> On Tue, 24 Mar 2009 16:28:43 -0500
> Becky Bruce <beckyb@kernel.crashing.org> wrote:
>
>> Squash a build warning seen on 32-bit powerpc caused by calling min()
>> with 2 different types.  Cast the first arg to size_t, which is the
>> type of the second, and should be portable across architectures.
>>
>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
>> ---
>> lib/swiotlb.c |    2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
>> index f59cf30..62f5f75 100644
>> --- a/lib/swiotlb.c
>> +++ b/lib/swiotlb.c
>> @@ -341,7 +341,7 @@ static void swiotlb_bounce(phys_addr_t phys,  
>> char *dma_addr, size_t size,
>> 		unsigned long flags;
>>
>> 		while (size) {
>> -			sz = min(PAGE_SIZE - offset, size);
>> +			sz = min((size_t)(PAGE_SIZE - offset), size);
>>
>> 			local_irq_save(flags);
>> 			buffer = kmap_atomic(pfn_to_page(pfn),
>
> ?
>
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index f59cf30..fa62498 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -341,7 +341,7 @@ static void swiotlb_bounce(phys_addr_t phys,  
> char *dma_addr, size_t size,
> 		unsigned long flags;
>
> 		while (size) {
> -			sz = min(PAGE_SIZE - offset, size);
> +			sz = min_t(size_t, PAGE_SIZE - offset, size);
>
> 			local_irq_save(flags);
> 			buffer = kmap_atomic(pfn_to_page(pfn),

OK, we're clearly pointed at different trees here, and it looks like  
I'm behind - this patch is based on Ingo's master.  Which tree has  
this change?

Thanks,
Becky

Re: [PATCH 2/5] swiotlb: fix compile warning

[FUJITA Tomonori]

On Tue, 24 Mar 2009 22:42:33 -0500
Becky Bruce <beckyb@kernel.crashing.org> wrote:

> 
> On Mar 24, 2009, at 9:58 PM, FUJITA Tomonori wrote:
> 
> > On Tue, 24 Mar 2009 16:28:43 -0500
> > Becky Bruce <beckyb@kernel.crashing.org> wrote:
> >
> >> Squash a build warning seen on 32-bit powerpc caused by calling min()
> >> with 2 different types.  Cast the first arg to size_t, which is the
> >> type of the second, and should be portable across architectures.
> >>
> >> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> >> ---
> >> lib/swiotlb.c |    2 +-
> >> 1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> >> index f59cf30..62f5f75 100644
> >> --- a/lib/swiotlb.c
> >> +++ b/lib/swiotlb.c
> >> @@ -341,7 +341,7 @@ static void swiotlb_bounce(phys_addr_t phys,  
> >> char *dma_addr, size_t size,
> >> 		unsigned long flags;
> >>
> >> 		while (size) {
> >> -			sz = min(PAGE_SIZE - offset, size);
> >> +			sz = min((size_t)(PAGE_SIZE - offset), size);
> >>
> >> 			local_irq_save(flags);
> >> 			buffer = kmap_atomic(pfn_to_page(pfn),
> >
> > ?
> >
> > diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> > index f59cf30..fa62498 100644
> > --- a/lib/swiotlb.c
> > +++ b/lib/swiotlb.c
> > @@ -341,7 +341,7 @@ static void swiotlb_bounce(phys_addr_t phys,  
> > char *dma_addr, size_t size,
> > 		unsigned long flags;
> >
> > 		while (size) {
> > -			sz = min(PAGE_SIZE - offset, size);
> > +			sz = min_t(size_t, PAGE_SIZE - offset, size);
> >
> > 			local_irq_save(flags);
> > 			buffer = kmap_atomic(pfn_to_page(pfn),
> 
> OK, we're clearly pointed at different trees here, and it looks like  
> I'm behind - this patch is based on Ingo's master.  Which tree has  
> this change?

I also use tip/master. I think that we are on the same page. No tree
has this change. I just send it because I'm not sure why you don't use
min_t().

Re: [PATCH 2/5] swiotlb: fix compile warning

[Ingo Molnar]

* FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> On Tue, 24 Mar 2009 22:42:33 -0500
> Becky Bruce <beckyb@kernel.crashing.org> wrote:
> 
> > 
> > On Mar 24, 2009, at 9:58 PM, FUJITA Tomonori wrote:
> > 
> > > On Tue, 24 Mar 2009 16:28:43 -0500
> > > Becky Bruce <beckyb@kernel.crashing.org> wrote:
> > >
> > >> Squash a build warning seen on 32-bit powerpc caused by calling min()
> > >> with 2 different types.  Cast the first arg to size_t, which is the
> > >> type of the second, and should be portable across architectures.
> > >>
> > >> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> > >> ---
> > >> lib/swiotlb.c |    2 +-
> > >> 1 files changed, 1 insertions(+), 1 deletions(-)
> > >>
> > >> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> > >> index f59cf30..62f5f75 100644
> > >> --- a/lib/swiotlb.c
> > >> +++ b/lib/swiotlb.c
> > >> @@ -341,7 +341,7 @@ static void swiotlb_bounce(phys_addr_t phys,  
> > >> char *dma_addr, size_t size,
> > >> 		unsigned long flags;
> > >>
> > >> 		while (size) {
> > >> -			sz = min(PAGE_SIZE - offset, size);
> > >> +			sz = min((size_t)(PAGE_SIZE - offset), size);
> > >>
> > >> 			local_irq_save(flags);
> > >> 			buffer = kmap_atomic(pfn_to_page(pfn),
> > >
> > > ?
> > >
> > > diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> > > index f59cf30..fa62498 100644
> > > --- a/lib/swiotlb.c
> > > +++ b/lib/swiotlb.c
> > > @@ -341,7 +341,7 @@ static void swiotlb_bounce(phys_addr_t phys,  
> > > char *dma_addr, size_t size,
> > > 		unsigned long flags;
> > >
> > > 		while (size) {
> > > -			sz = min(PAGE_SIZE - offset, size);
> > > +			sz = min_t(size_t, PAGE_SIZE - offset, size);
> > >
> > > 			local_irq_save(flags);
> > > 			buffer = kmap_atomic(pfn_to_page(pfn),
> > 
> > OK, we're clearly pointed at different trees here, and it looks like  
> > I'm behind - this patch is based on Ingo's master.  Which tree has  
> > this change?
> 
> I also use tip/master. I think that we are on the same page. No 
> tree has this change. I just send it because I'm not sure why you 
> don't use min_t().

yes, min_t() is safer and cleaner than min()+cast.

	Ingo

Re: [PATCH 2/5] swiotlb: fix compile warning

[Becky Bruce]

On Mar 25, 2009, at 7:45 AM, Ingo Molnar wrote:

>
> * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>
>> On Tue, 24 Mar 2009 22:42:33 -0500
>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
>>
>>>
>>> On Mar 24, 2009, at 9:58 PM, FUJITA Tomonori wrote:
>>>
>>>> On Tue, 24 Mar 2009 16:28:43 -0500
>>>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
>>>>
>>>>> Squash a build warning seen on 32-bit powerpc caused by calling  
>>>>> min()
>>>>> with 2 different types.  Cast the first arg to size_t, which is  
>>>>> the
>>>>> type of the second, and should be portable across architectures.
>>>>>
>>>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
>>>>> ---
>>>>> lib/swiotlb.c |    2 +-
>>>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
>>>>> index f59cf30..62f5f75 100644
>>>>> --- a/lib/swiotlb.c
>>>>> +++ b/lib/swiotlb.c
>>>>> @@ -341,7 +341,7 @@ static void swiotlb_bounce(phys_addr_t phys,
>>>>> char *dma_addr, size_t size,
>>>>> 		unsigned long flags;
>>>>>
>>>>> 		while (size) {
>>>>> -			sz = min(PAGE_SIZE - offset, size);
>>>>> +			sz = min((size_t)(PAGE_SIZE - offset), size);
>>>>>
>>>>> 			local_irq_save(flags);
>>>>> 			buffer = kmap_atomic(pfn_to_page(pfn),
>>>>
>>>> ?
>>>>
>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
>>>> index f59cf30..fa62498 100644
>>>> --- a/lib/swiotlb.c
>>>> +++ b/lib/swiotlb.c
>>>> @@ -341,7 +341,7 @@ static void swiotlb_bounce(phys_addr_t phys,
>>>> char *dma_addr, size_t size,
>>>> 		unsigned long flags;
>>>>
>>>> 		while (size) {
>>>> -			sz = min(PAGE_SIZE - offset, size);
>>>> +			sz = min_t(size_t, PAGE_SIZE - offset, size);
>>>>
>>>> 			local_irq_save(flags);
>>>> 			buffer = kmap_atomic(pfn_to_page(pfn),
>>>
>>> OK, we're clearly pointed at different trees here, and it looks like
>>> I'm behind - this patch is based on Ingo's master.  Which tree has
>>> this change?
>>
>> I also use tip/master. I think that we are on the same page. No
>> tree has this change. I just send it because I'm not sure why you
>> don't use min_t().
>
> yes, min_t() is safer and cleaner than min()+cast.

Ahh, then, that's just an oversight on my part, and I'll fix it.  I'll  
send out a respun set of patches as soon as I'm out of today's endless  
set of meetings.

Thanks,
Becky

Re: [PATCH 0/5] swiotlb: changes for powerpc/highmem

[Jeremy Fitzhardinge]

Becky Bruce wrote:
> This is a series of fairly minor patches that get swiotlb working
> on 32-bit powerpc systems with HIGHMEM, plus some cleanup of the
> outdated comments in the code.  I've made a couple of things
> weak that ppc needs to override, and have changed the prototypes
> for a couple of functions to include the hwdev pointer, which
> we need to ppc to convert bus addresses to and from phys/virt
> addresses.  I've also fixed a build warning I've been seeing on
> ppc.
>
> I have not tested this in any way on any non-ppc platforms,
> so commentary/testing from x86/ia64 folks is greatly 
> appreciated.
>   

This works fine for me under Xen.

Acked-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

    J