[PATCH V2 0/7] swiotlb: changes for powerpc/highmem

[PATCH V2 0/7] swiotlb: changes for powerpc/highmem

[Becky Bruce]

This is v2 of 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.

In response to commentary on the previous series, I've also
refactored the code a bit, altough I did this slightly
differently than was suggested because I noticed we could use the
new helper function in 2 places instead of one. I've reformatted
a bit of code based on commentary as well.

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

I'm going to be offline for the next week, but will respond to
commentary as soon as I return.

Cheers,
Becky

diffstat:
arch/x86/kernel/pci-swiotlb.c |    2 +-
include/linux/swiotlb.h       |    3 +-
lib/swiotlb.c                 |  115 ++++++++++++++++++++++-------------------
3 files changed, 64 insertions(+), 56 deletions(-)

[PATCH 1/7] 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/7] swiotlb: fix compile warning

[Becky Bruce]

Squash a build warning seen on 32-bit powerpc caused by calling min()
with 2 different types. Use min_t() instead

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..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),
-- 
1.5.6.6

[PATCH 3/7] 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 fa62498..eb6dbbd 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 4/7] 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 eb6dbbd..af2ec25 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -145,6 +145,12 @@ static void *swiotlb_bus_to_virt(dma_addr_t address)
 	return phys_to_virt(swiotlb_bus_to_phys(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

[PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Becky Bruce]

This mirrors the current swiotlb_sync_single() setup
where the swiotlb_unmap_single() function is static to this
file and contains the logic required to determine if we need
to call actual sync_single.  Previously, swiotlb_unmap_page
and swiotlb_unmap_sg were duplicating very similar code.
The duplicated code has also been reformatted for
readability.

Note that the swiotlb_unmap_sg code was previously doing
a complicated comparison to determine if an addresses needed
to be unmapped where a simple is_swiotlb_buffer() call
would have sufficed.

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

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index af2ec25..602315b 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -688,17 +688,30 @@ EXPORT_SYMBOL_GPL(swiotlb_map_page);
  * After this call, reads by the cpu to the buffer are guaranteed to see
  * whatever the device wrote there.
  */
-void swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
-			size_t size, enum dma_data_direction dir,
-			struct dma_attrs *attrs)
+static void
+swiotlb_unmap_single(struct device *hwdev, dma_addr_t dev_addr,
+		     size_t size, int dir)
 {
 	char *dma_addr = swiotlb_bus_to_virt(dev_addr);
 
 	BUG_ON(dir == DMA_NONE);
-	if (is_swiotlb_buffer(dma_addr))
+
+	if (is_swiotlb_buffer(dma_addr)) {
 		unmap_single(hwdev, dma_addr, size, dir);
-	else if (dir == DMA_FROM_DEVICE)
-		dma_mark_clean(dma_addr, size);
+		return;
+	}
+
+	if (dir != DMA_FROM_DEVICE)
+		return;
+
+	dma_mark_clean(dma_addr, size);
+}
+
+void swiotlb_unmap_page(struct device *hwdev, dma_addr_t dev_addr,
+			size_t size, enum dma_data_direction dir,
+			struct dma_attrs *attrs)
+{
+	swiotlb_unmap_single(hwdev, dev_addr, size, dir);
 }
 EXPORT_SYMBOL_GPL(swiotlb_unmap_page);
 
@@ -850,13 +863,10 @@ swiotlb_unmap_sg_attrs(struct device *hwdev, struct scatterlist *sgl,
 
 	BUG_ON(dir == DMA_NONE);
 
-	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);
-		else if (dir == DMA_FROM_DEVICE)
-			dma_mark_clean(swiotlb_bus_to_virt(sg->dma_address), sg->dma_length);
-	}
+	for_each_sg(sgl, sg, nelems, i)
+		swiotlb_unmap_single(hwdev, sg->dma_address, sg->dma_length,
+				     dir);
+
 }
 EXPORT_SYMBOL(swiotlb_unmap_sg_attrs);
 
-- 
1.5.6.6

[PATCH 6/7] swiotlb: Use swiotlb_sync_single instead of duplicating code

[Becky Bruce]

Right now both swiotlb_sync_single_range and swiotlb_sync_sg
were duplicating the code in swiotlb_sync_single.  Just call it
instead.  Also rearrange the sync_single code for readability.

Note that the swiotlb_sync_sg code was previously doing
a complicated comparison to determine if an addresses needed
to be unmapped where a simple is_swiotlb_buffer() call
would have sufficed.

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

diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 602315b..c0f185a 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -732,10 +732,16 @@ swiotlb_sync_single(struct device *hwdev, dma_addr_t dev_addr,
 	char *dma_addr = swiotlb_bus_to_virt(dev_addr);
 
 	BUG_ON(dir == DMA_NONE);
-	if (is_swiotlb_buffer(dma_addr))
+
+	if (is_swiotlb_buffer(dma_addr)) {
 		sync_single(hwdev, dma_addr, size, dir, target);
-	else if (dir == DMA_FROM_DEVICE)
-		dma_mark_clean(dma_addr, size);
+		return;
+	}
+
+	if (dir != DMA_FROM_DEVICE)
+		return;
+
+	dma_mark_clean(dma_addr, size);
 }
 
 void
@@ -762,13 +768,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;
-
-	BUG_ON(dir == DMA_NONE);
-	if (is_swiotlb_buffer(dma_addr))
-		sync_single(hwdev, dma_addr, size, dir, target);
-	else if (dir == DMA_FROM_DEVICE)
-		dma_mark_clean(dma_addr, size);
+	swiotlb_sync_single(hwdev, dev_addr + offset, size, dir, target);
 }
 
 void
@@ -892,15 +892,9 @@ swiotlb_sync_sg(struct device *hwdev, struct scatterlist *sgl,
 	struct scatterlist *sg;
 	int i;
 
-	BUG_ON(dir == DMA_NONE);
-
-	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),
+	for_each_sg(sgl, sg, nelems, i)
+		swiotlb_sync_single(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);
-	}
 }
 
 void
-- 
1.5.6.6

[PATCH 7/7] swiotlb: Change swiotlb_bus_to[phys,virt] prototypes

[Becky Bruce]

Add a hwdev argument that is needed on some architectures
in order to access a per-device offset that is taken into
account when producing a physical address (also needed to
get from bus address to virtual address because the physical
address is an intermediate step).

Also make swiotlb_bus_to_virt weak so architectures can
override it.

Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
---
 arch/x86/kernel/pci-swiotlb.c |    2 +-
 include/linux/swiotlb.h       |    3 ++-
 lib/swiotlb.c                 |   10 +++++-----
 3 files changed, 8 insertions(+), 7 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..cb1a663 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -29,7 +29,8 @@ 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 int swiotlb_arch_range_needs_mapping(phys_addr_t paddr, size_t size);
 
diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index c0f185a..d006f6d 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -129,7 +129,7 @@ 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;
 }
@@ -140,9 +140,9 @@ static dma_addr_t swiotlb_virt_to_bus(struct device *hwdev,
 	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_address_needs_mapping(struct device *hwdev,
@@ -692,7 +692,7 @@ static void
 swiotlb_unmap_single(struct device *hwdev, dma_addr_t dev_addr,
 		     size_t size, int dir)
 {
-	char *dma_addr = swiotlb_bus_to_virt(dev_addr);
+	char *dma_addr = swiotlb_bus_to_virt(hwdev, dev_addr);
 
 	BUG_ON(dir == DMA_NONE);
 
@@ -729,7 +729,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);
 
-- 
1.5.6.6

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[FUJITA Tomonori]

On Fri,  3 Apr 2009 20:56:47 -0500
Becky Bruce <beckyb@kernel.crashing.org> wrote:

> This mirrors the current swiotlb_sync_single() setup
> where the swiotlb_unmap_single() function is static to this
> file and contains the logic required to determine if we need
> to call actual sync_single.  Previously, swiotlb_unmap_page
> and swiotlb_unmap_sg were duplicating very similar code.
> The duplicated code has also been reformatted for
> readability.
> 
> Note that the swiotlb_unmap_sg code was previously doing
> a complicated comparison to determine if an addresses needed
> to be unmapped where a simple is_swiotlb_buffer() call
> would have sufficed.
> 
> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> ---
>  lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
>  1 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index af2ec25..602315b 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c

I don't think 'swiotlb_unmap_single' name is appropriate.

swiotlb_unmap_single sounds like an exported function that IOMMUs can
use (and it was) however it should not be.

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Kumar Gala]

On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:

> On Fri,  3 Apr 2009 20:56:47 -0500
> Becky Bruce <beckyb@kernel.crashing.org> wrote:
>
>> This mirrors the current swiotlb_sync_single() setup
>> where the swiotlb_unmap_single() function is static to this
>> file and contains the logic required to determine if we need
>> to call actual sync_single.  Previously, swiotlb_unmap_page
>> and swiotlb_unmap_sg were duplicating very similar code.
>> The duplicated code has also been reformatted for
>> readability.
>>
>> Note that the swiotlb_unmap_sg code was previously doing
>> a complicated comparison to determine if an addresses needed
>> to be unmapped where a simple is_swiotlb_buffer() call
>> would have sufficed.
>>
>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
>> ---
>> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
>> 1 files changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
>> index af2ec25..602315b 100644
>> --- a/lib/swiotlb.c
>> +++ b/lib/swiotlb.c
>
> I don't think 'swiotlb_unmap_single' name is appropriate.
>
> swiotlb_unmap_single sounds like an exported function that IOMMUs can
> use (and it was) however it should not be.

What do you suggest we call it?  __swiotlb_unmap_single.

- k

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[FUJITA Tomonori]

On Tue, 7 Apr 2009 01:34:44 -0500
Kumar Gala <galak@kernel.crashing.org> wrote:

> 
> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:
> 
> > On Fri,  3 Apr 2009 20:56:47 -0500
> > Becky Bruce <beckyb@kernel.crashing.org> wrote:
> >
> >> This mirrors the current swiotlb_sync_single() setup
> >> where the swiotlb_unmap_single() function is static to this
> >> file and contains the logic required to determine if we need
> >> to call actual sync_single.  Previously, swiotlb_unmap_page
> >> and swiotlb_unmap_sg were duplicating very similar code.
> >> The duplicated code has also been reformatted for
> >> readability.
> >>
> >> Note that the swiotlb_unmap_sg code was previously doing
> >> a complicated comparison to determine if an addresses needed
> >> to be unmapped where a simple is_swiotlb_buffer() call
> >> would have sufficed.
> >>
> >> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> >> ---
> >> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
> >> 1 files changed, 23 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> >> index af2ec25..602315b 100644
> >> --- a/lib/swiotlb.c
> >> +++ b/lib/swiotlb.c
> >
> > I don't think 'swiotlb_unmap_single' name is appropriate.
> >
> > swiotlb_unmap_single sounds like an exported function that IOMMUs can
> > use (and it was) however it should not be.
> 
> What do you suggest we call it?  __swiotlb_unmap_single.

I think that __swiotlb_unmap_single is better because the name implies
that it's an internal function. It's fine by me.

If it is odd that __swiotlb_unmap_single() is just a wrapper function
of unmap_single(), which does the real job to unmap a dma mapping, it
might be another possible option to rename unmap_single to
do_unamp_single and use unmap_single.

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Kumar Gala]

On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote:

> On Tue, 7 Apr 2009 01:34:44 -0500
> Kumar Gala <galak@kernel.crashing.org> wrote:
>
>>
>> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:
>>
>>> On Fri,  3 Apr 2009 20:56:47 -0500
>>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
>>>
>>>> This mirrors the current swiotlb_sync_single() setup
>>>> where the swiotlb_unmap_single() function is static to this
>>>> file and contains the logic required to determine if we need
>>>> to call actual sync_single.  Previously, swiotlb_unmap_page
>>>> and swiotlb_unmap_sg were duplicating very similar code.
>>>> The duplicated code has also been reformatted for
>>>> readability.
>>>>
>>>> Note that the swiotlb_unmap_sg code was previously doing
>>>> a complicated comparison to determine if an addresses needed
>>>> to be unmapped where a simple is_swiotlb_buffer() call
>>>> would have sufficed.
>>>>
>>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
>>>> ---
>>>> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
>>>> 1 files changed, 23 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
>>>> index af2ec25..602315b 100644
>>>> --- a/lib/swiotlb.c
>>>> +++ b/lib/swiotlb.c
>>>
>>> I don't think 'swiotlb_unmap_single' name is appropriate.
>>>
>>> swiotlb_unmap_single sounds like an exported function that IOMMUs  
>>> can
>>> use (and it was) however it should not be.
>>
>> What do you suggest we call it?  __swiotlb_unmap_single.
>
> I think that __swiotlb_unmap_single is better because the name implies
> that it's an internal function. It's fine by me.
>
> If it is odd that __swiotlb_unmap_single() is just a wrapper function
> of unmap_single(), which does the real job to unmap a dma mapping, it
> might be another possible option to rename unmap_single to
> do_unamp_single and use unmap_single.

I think you lost me here.  I'd prefer to just use  
__swiotlb_unmap_single at this point and get this code into the tree  
and work on such renaming after the fact (if that's ok).

- k

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[FUJITA Tomonori]

On Tue, 7 Apr 2009 10:32:20 -0500
Kumar Gala <galak@kernel.crashing.org> wrote:

> 
> On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote:
> 
> > On Tue, 7 Apr 2009 01:34:44 -0500
> > Kumar Gala <galak@kernel.crashing.org> wrote:
> >
> >>
> >> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:
> >>
> >>> On Fri,  3 Apr 2009 20:56:47 -0500
> >>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
> >>>
> >>>> This mirrors the current swiotlb_sync_single() setup
> >>>> where the swiotlb_unmap_single() function is static to this
> >>>> file and contains the logic required to determine if we need
> >>>> to call actual sync_single.  Previously, swiotlb_unmap_page
> >>>> and swiotlb_unmap_sg were duplicating very similar code.
> >>>> The duplicated code has also been reformatted for
> >>>> readability.
> >>>>
> >>>> Note that the swiotlb_unmap_sg code was previously doing
> >>>> a complicated comparison to determine if an addresses needed
> >>>> to be unmapped where a simple is_swiotlb_buffer() call
> >>>> would have sufficed.
> >>>>
> >>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> >>>> ---
> >>>> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
> >>>> 1 files changed, 23 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> >>>> index af2ec25..602315b 100644
> >>>> --- a/lib/swiotlb.c
> >>>> +++ b/lib/swiotlb.c
> >>>
> >>> I don't think 'swiotlb_unmap_single' name is appropriate.
> >>>
> >>> swiotlb_unmap_single sounds like an exported function that IOMMUs  
> >>> can
> >>> use (and it was) however it should not be.
> >>
> >> What do you suggest we call it?  __swiotlb_unmap_single.
> >
> > I think that __swiotlb_unmap_single is better because the name implies
> > that it's an internal function. It's fine by me.
> >
> > If it is odd that __swiotlb_unmap_single() is just a wrapper function
> > of unmap_single(), which does the real job to unmap a dma mapping, it
> > might be another possible option to rename unmap_single to
> > do_unamp_single and use unmap_single.
> 
> I think you lost me here.  I'd prefer to just use  
> __swiotlb_unmap_single at this point and get this code into the tree  
> and work on such renaming after the fact (if that's ok).

If you are rushing to merge this right now, the original patchset is
fine by me (I thought that you missed this merge window). I'll rename
it later.

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Kumar Gala]

On Apr 7, 2009, at 11:37 AM, FUJITA Tomonori wrote:

> On Tue, 7 Apr 2009 10:32:20 -0500
> Kumar Gala <galak@kernel.crashing.org> wrote:
>
>>
>> On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote:
>>
>>> On Tue, 7 Apr 2009 01:34:44 -0500
>>> Kumar Gala <galak@kernel.crashing.org> wrote:
>>>
>>>>
>>>> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:
>>>>
>>>>> On Fri,  3 Apr 2009 20:56:47 -0500
>>>>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
>>>>>
>>>>>> This mirrors the current swiotlb_sync_single() setup
>>>>>> where the swiotlb_unmap_single() function is static to this
>>>>>> file and contains the logic required to determine if we need
>>>>>> to call actual sync_single.  Previously, swiotlb_unmap_page
>>>>>> and swiotlb_unmap_sg were duplicating very similar code.
>>>>>> The duplicated code has also been reformatted for
>>>>>> readability.
>>>>>>
>>>>>> Note that the swiotlb_unmap_sg code was previously doing
>>>>>> a complicated comparison to determine if an addresses needed
>>>>>> to be unmapped where a simple is_swiotlb_buffer() call
>>>>>> would have sufficed.
>>>>>>
>>>>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
>>>>>> ---
>>>>>> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
>>>>>> 1 files changed, 23 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
>>>>>> index af2ec25..602315b 100644
>>>>>> --- a/lib/swiotlb.c
>>>>>> +++ b/lib/swiotlb.c
>>>>>
>>>>> I don't think 'swiotlb_unmap_single' name is appropriate.
>>>>>
>>>>> swiotlb_unmap_single sounds like an exported function that IOMMUs
>>>>> can
>>>>> use (and it was) however it should not be.
>>>>
>>>> What do you suggest we call it?  __swiotlb_unmap_single.
>>>
>>> I think that __swiotlb_unmap_single is better because the name  
>>> implies
>>> that it's an internal function. It's fine by me.
>>>
>>> If it is odd that __swiotlb_unmap_single() is just a wrapper  
>>> function
>>> of unmap_single(), which does the real job to unmap a dma mapping,  
>>> it
>>> might be another possible option to rename unmap_single to
>>> do_unamp_single and use unmap_single.
>>
>> I think you lost me here.  I'd prefer to just use
>> __swiotlb_unmap_single at this point and get this code into the tree
>> and work on such renaming after the fact (if that's ok).
>
> If you are rushing to merge this right now, the original patchset is
> fine by me (I thought that you missed this merge window). I'll rename
> it later.

We probably did, but one can never tell with these things.  It seemed  
like Ingo merged and pushed some swiotlb changes late in the game for . 
29

I'm still not clear on what you are suggesting... "rename unmap_single  
to do_unamp_single and use unmap_single".

- k

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[FUJITA Tomonori]

On Tue, 7 Apr 2009 11:50:56 -0500
Kumar Gala <galak@kernel.crashing.org> wrote:

> 
> On Apr 7, 2009, at 11:37 AM, FUJITA Tomonori wrote:
> 
> > On Tue, 7 Apr 2009 10:32:20 -0500
> > Kumar Gala <galak@kernel.crashing.org> wrote:
> >
> >>
> >> On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote:
> >>
> >>> On Tue, 7 Apr 2009 01:34:44 -0500
> >>> Kumar Gala <galak@kernel.crashing.org> wrote:
> >>>
> >>>>
> >>>> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:
> >>>>
> >>>>> On Fri,  3 Apr 2009 20:56:47 -0500
> >>>>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
> >>>>>
> >>>>>> This mirrors the current swiotlb_sync_single() setup
> >>>>>> where the swiotlb_unmap_single() function is static to this
> >>>>>> file and contains the logic required to determine if we need
> >>>>>> to call actual sync_single.  Previously, swiotlb_unmap_page
> >>>>>> and swiotlb_unmap_sg were duplicating very similar code.
> >>>>>> The duplicated code has also been reformatted for
> >>>>>> readability.
> >>>>>>
> >>>>>> Note that the swiotlb_unmap_sg code was previously doing
> >>>>>> a complicated comparison to determine if an addresses needed
> >>>>>> to be unmapped where a simple is_swiotlb_buffer() call
> >>>>>> would have sufficed.
> >>>>>>
> >>>>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> >>>>>> ---
> >>>>>> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
> >>>>>> 1 files changed, 23 insertions(+), 13 deletions(-)
> >>>>>>
> >>>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> >>>>>> index af2ec25..602315b 100644
> >>>>>> --- a/lib/swiotlb.c
> >>>>>> +++ b/lib/swiotlb.c
> >>>>>
> >>>>> I don't think 'swiotlb_unmap_single' name is appropriate.
> >>>>>
> >>>>> swiotlb_unmap_single sounds like an exported function that IOMMUs
> >>>>> can
> >>>>> use (and it was) however it should not be.
> >>>>
> >>>> What do you suggest we call it?  __swiotlb_unmap_single.
> >>>
> >>> I think that __swiotlb_unmap_single is better because the name  
> >>> implies
> >>> that it's an internal function. It's fine by me.
> >>>
> >>> If it is odd that __swiotlb_unmap_single() is just a wrapper  
> >>> function
> >>> of unmap_single(), which does the real job to unmap a dma mapping,  
> >>> it
> >>> might be another possible option to rename unmap_single to
> >>> do_unamp_single and use unmap_single.
> >>
> >> I think you lost me here.  I'd prefer to just use
> >> __swiotlb_unmap_single at this point and get this code into the tree
> >> and work on such renaming after the fact (if that's ok).
> >
> > If you are rushing to merge this right now, the original patchset is
> > fine by me (I thought that you missed this merge window). I'll rename
> > it later.
> 
> We probably did, but one can never tell with these things.  It seemed  
> like Ingo merged and pushed some swiotlb changes late in the game for . 
> 29

Well, merging patches that have not been tested linux-next late is
what we should not do, I guess. I like to see Becky's patch in 2.6.30
because I have some swiotlb changes for 2.6.31 though.


> I'm still not clear on what you are suggesting... "rename unmap_single  
> to do_unamp_single and use unmap_single".

This can be applied after Becky's patchset.


diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 1c86553..bffe6d7 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -482,7 +482,7 @@ found:
  * dma_addr is the kernel virtual address of the bounce buffer to unmap.
  */
 static void
-unmap_single(struct device *hwdev, char *dma_addr, size_t size, int dir)
+do_unmap_single(struct device *hwdev, char *dma_addr, size_t size, int dir)
 {
 	unsigned long flags;
 	int i, count, nslots = ALIGN(size, 1 << IO_TLB_SHIFT) >> IO_TLB_SHIFT;
@@ -591,7 +591,7 @@ swiotlb_alloc_coherent(struct device *hwdev, size_t size,
 		       (unsigned long long)dev_addr);
 
 		/* DMA_TO_DEVICE to avoid memcpy in unmap_single */
-		unmap_single(hwdev, ret, size, DMA_TO_DEVICE);
+		do_unmap_single(hwdev, ret, size, DMA_TO_DEVICE);
 		return NULL;
 	}
 	*dma_handle = dev_addr;
@@ -608,7 +608,7 @@ swiotlb_free_coherent(struct device *hwdev, size_t size, void *vaddr,
 		free_pages((unsigned long) vaddr, get_order(size));
 	else
 		/* DMA_TO_DEVICE to avoid memcpy in unmap_single */
-		unmap_single(hwdev, vaddr, size, DMA_TO_DEVICE);
+		do_unmap_single(hwdev, vaddr, size, DMA_TO_DEVICE);
 }
 EXPORT_SYMBOL(swiotlb_free_coherent);
 
@@ -688,16 +688,15 @@ EXPORT_SYMBOL_GPL(swiotlb_map_page);
  * After this call, reads by the cpu to the buffer are guaranteed to see
  * whatever the device wrote there.
  */
-static void
-swiotlb_unmap_single(struct device *hwdev, dma_addr_t dev_addr,
-		     size_t size, int dir)
+static void unmap_single(struct device *hwdev, dma_addr_t dev_addr,
+			 size_t size, int dir)
 {
 	char *dma_addr = swiotlb_bus_to_virt(hwdev, dev_addr);
 
 	BUG_ON(dir == DMA_NONE);
 
 	if (is_swiotlb_buffer(dma_addr)) {
-		unmap_single(hwdev, dma_addr, size, dir);
+		do_unmap_single(hwdev, dma_addr, size, dir);
 		return;
 	}
 
@@ -711,7 +710,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)
 {
-	swiotlb_unmap_single(hwdev, dev_addr, size, dir);
+	unmap_single(hwdev, dev_addr, size, dir);
 }
 EXPORT_SYMBOL_GPL(swiotlb_unmap_page);
 
@@ -864,8 +863,7 @@ swiotlb_unmap_sg_attrs(struct device *hwdev, struct scatterlist *sgl,
 	BUG_ON(dir == DMA_NONE);
 
 	for_each_sg(sgl, sg, nelems, i)
-		swiotlb_unmap_single(hwdev, sg->dma_address, sg->dma_length,
-				     dir);
+		unmap_single(hwdev, sg->dma_address, sg->dma_length, dir);
 
 }
 EXPORT_SYMBOL(swiotlb_unmap_sg_attrs);

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Kumar Gala]

On Apr 7, 2009, at 12:22 PM, FUJITA Tomonori wrote:

> On Tue, 7 Apr 2009 11:50:56 -0500
> Kumar Gala <galak@kernel.crashing.org> wrote:
>
>>
>> On Apr 7, 2009, at 11:37 AM, FUJITA Tomonori wrote:
>>
>>> On Tue, 7 Apr 2009 10:32:20 -0500
>>> Kumar Gala <galak@kernel.crashing.org> wrote:
>>>
>>>>
>>>> On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote:
>>>>
>>>>> On Tue, 7 Apr 2009 01:34:44 -0500
>>>>> Kumar Gala <galak@kernel.crashing.org> wrote:
>>>>>
>>>>>>
>>>>>> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:
>>>>>>
>>>>>>> On Fri,  3 Apr 2009 20:56:47 -0500
>>>>>>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
>>>>>>>
>>>>>>>> This mirrors the current swiotlb_sync_single() setup
>>>>>>>> where the swiotlb_unmap_single() function is static to this
>>>>>>>> file and contains the logic required to determine if we need
>>>>>>>> to call actual sync_single.  Previously, swiotlb_unmap_page
>>>>>>>> and swiotlb_unmap_sg were duplicating very similar code.
>>>>>>>> The duplicated code has also been reformatted for
>>>>>>>> readability.
>>>>>>>>
>>>>>>>> Note that the swiotlb_unmap_sg code was previously doing
>>>>>>>> a complicated comparison to determine if an addresses needed
>>>>>>>> to be unmapped where a simple is_swiotlb_buffer() call
>>>>>>>> would have sufficed.
>>>>>>>>
>>>>>>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
>>>>>>>> ---
>>>>>>>> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
>>>>>>>> 1 files changed, 23 insertions(+), 13 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
>>>>>>>> index af2ec25..602315b 100644
>>>>>>>> --- a/lib/swiotlb.c
>>>>>>>> +++ b/lib/swiotlb.c
>>>>>>>
>>>>>>> I don't think 'swiotlb_unmap_single' name is appropriate.
>>>>>>>
>>>>>>> swiotlb_unmap_single sounds like an exported function that  
>>>>>>> IOMMUs
>>>>>>> can
>>>>>>> use (and it was) however it should not be.
>>>>>>
>>>>>> What do you suggest we call it?  __swiotlb_unmap_single.
>>>>>
>>>>> I think that __swiotlb_unmap_single is better because the name
>>>>> implies
>>>>> that it's an internal function. It's fine by me.
>>>>>
>>>>> If it is odd that __swiotlb_unmap_single() is just a wrapper
>>>>> function
>>>>> of unmap_single(), which does the real job to unmap a dma mapping,
>>>>> it
>>>>> might be another possible option to rename unmap_single to
>>>>> do_unamp_single and use unmap_single.
>>>>
>>>> I think you lost me here.  I'd prefer to just use
>>>> __swiotlb_unmap_single at this point and get this code into the  
>>>> tree
>>>> and work on such renaming after the fact (if that's ok).
>>>
>>> If you are rushing to merge this right now, the original patchset is
>>> fine by me (I thought that you missed this merge window). I'll  
>>> rename
>>> it later.
>>
>> We probably did, but one can never tell with these things.  It seemed
>> like Ingo merged and pushed some swiotlb changes late in the game  
>> for .
>> 29
>
> Well, merging patches that have not been tested linux-next late is
> what we should not do, I guess. I like to see Becky's patch in 2.6.30
> because I have some swiotlb changes for 2.6.31 though.

Same here.  It makes it easier for us to work on the powerpc arch  
specific changes for .31 if we can get these into .30.  What are you  
looking at for .31?

Ingo, any comments on that?

>> I'm still not clear on what you are suggesting... "rename  
>> unmap_single
>> to do_unamp_single and use unmap_single".
>
> This can be applied after Becky's patchset.

thanks. I'll merge this into her patch set and repost it.

- k

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[FUJITA Tomonori]

On Tue, 7 Apr 2009 12:32:12 -0500
Kumar Gala <galak@kernel.crashing.org> wrote:

> 
> On Apr 7, 2009, at 12:22 PM, FUJITA Tomonori wrote:
> 
> > On Tue, 7 Apr 2009 11:50:56 -0500
> > Kumar Gala <galak@kernel.crashing.org> wrote:
> >
> >>
> >> On Apr 7, 2009, at 11:37 AM, FUJITA Tomonori wrote:
> >>
> >>> On Tue, 7 Apr 2009 10:32:20 -0500
> >>> Kumar Gala <galak@kernel.crashing.org> wrote:
> >>>
> >>>>
> >>>> On Apr 7, 2009, at 4:09 AM, FUJITA Tomonori wrote:
> >>>>
> >>>>> On Tue, 7 Apr 2009 01:34:44 -0500
> >>>>> Kumar Gala <galak@kernel.crashing.org> wrote:
> >>>>>
> >>>>>>
> >>>>>> On Apr 6, 2009, at 9:24 PM, FUJITA Tomonori wrote:
> >>>>>>
> >>>>>>> On Fri,  3 Apr 2009 20:56:47 -0500
> >>>>>>> Becky Bruce <beckyb@kernel.crashing.org> wrote:
> >>>>>>>
> >>>>>>>> This mirrors the current swiotlb_sync_single() setup
> >>>>>>>> where the swiotlb_unmap_single() function is static to this
> >>>>>>>> file and contains the logic required to determine if we need
> >>>>>>>> to call actual sync_single.  Previously, swiotlb_unmap_page
> >>>>>>>> and swiotlb_unmap_sg were duplicating very similar code.
> >>>>>>>> The duplicated code has also been reformatted for
> >>>>>>>> readability.
> >>>>>>>>
> >>>>>>>> Note that the swiotlb_unmap_sg code was previously doing
> >>>>>>>> a complicated comparison to determine if an addresses needed
> >>>>>>>> to be unmapped where a simple is_swiotlb_buffer() call
> >>>>>>>> would have sufficed.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Becky Bruce <beckyb@kernel.crashing.org>
> >>>>>>>> ---
> >>>>>>>> lib/swiotlb.c |   36 +++++++++++++++++++++++-------------
> >>>>>>>> 1 files changed, 23 insertions(+), 13 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> >>>>>>>> index af2ec25..602315b 100644
> >>>>>>>> --- a/lib/swiotlb.c
> >>>>>>>> +++ b/lib/swiotlb.c
> >>>>>>>
> >>>>>>> I don't think 'swiotlb_unmap_single' name is appropriate.
> >>>>>>>
> >>>>>>> swiotlb_unmap_single sounds like an exported function that  
> >>>>>>> IOMMUs
> >>>>>>> can
> >>>>>>> use (and it was) however it should not be.
> >>>>>>
> >>>>>> What do you suggest we call it?  __swiotlb_unmap_single.
> >>>>>
> >>>>> I think that __swiotlb_unmap_single is better because the name
> >>>>> implies
> >>>>> that it's an internal function. It's fine by me.
> >>>>>
> >>>>> If it is odd that __swiotlb_unmap_single() is just a wrapper
> >>>>> function
> >>>>> of unmap_single(), which does the real job to unmap a dma mapping,
> >>>>> it
> >>>>> might be another possible option to rename unmap_single to
> >>>>> do_unamp_single and use unmap_single.
> >>>>
> >>>> I think you lost me here.  I'd prefer to just use
> >>>> __swiotlb_unmap_single at this point and get this code into the  
> >>>> tree
> >>>> and work on such renaming after the fact (if that's ok).
> >>>
> >>> If you are rushing to merge this right now, the original patchset is
> >>> fine by me (I thought that you missed this merge window). I'll  
> >>> rename
> >>> it later.
> >>
> >> We probably did, but one can never tell with these things.  It seemed
> >> like Ingo merged and pushed some swiotlb changes late in the game  
> >> for .
> >> 29
> >
> > Well, merging patches that have not been tested linux-next late is
> > what we should not do, I guess. I like to see Becky's patch in 2.6.30
> > because I have some swiotlb changes for 2.6.31 though.
> 
> Same here.  It makes it easier for us to work on the powerpc arch  
> specific changes for .31 if we can get these into .30.  What are you  
> looking at for .31?

I need to finish the dma_mapping_ops cleanups and cross-arch
unification stuff:

http://marc.info/?l=linux-kernel&m=123827903216314&w=2

But the changes to swiotlb is minor. BTW, I have the patches for
powerpc too.


> Ingo, any comments on that?

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Ingo Molnar]

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

> > Same here.  It makes it easier for us to work on the powerpc arch  
> > specific changes for .31 if we can get these into .30.  What are you  
> > looking at for .31?
> 
> I need to finish the dma_mapping_ops cleanups and cross-arch 
> unification stuff:
> 
> http://marc.info/?l=linux-kernel&m=123827903216314&w=2
> 
> But the changes to swiotlb is minor. BTW, I have the patches for 
> powerpc too.
> 
> > Ingo, any comments on that?

if it's genuine fixes we can do it post-rc1, but it looks a tad wide 
for that:

 arch/x86/kernel/pci-swiotlb.c |    2 +-
 include/linux/swiotlb.h       |    3 +-
 lib/swiotlb.c                 |  115 ++++++++++++++++++++++-------------------

the patches came right in the merge window - that's too late for 
IOMMU bits, the patch cutoff is generally the beginning of the merge 
window. (Which was on March 23 in this window)

But i can queue them up in the .31 queue if it has all the Acks from 
you folks. Becky, mind resending the latest version, with all the 
acks in place?

Thanks,

	Ingo

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Kumar Gala]

On Apr 8, 2009, at 7:43 AM, Ingo Molnar wrote:

>
> * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>
>>> Same here.  It makes it easier for us to work on the powerpc arch
>>> specific changes for .31 if we can get these into .30.  What are you
>>> looking at for .31?
>>
>> I need to finish the dma_mapping_ops cleanups and cross-arch
>> unification stuff:
>>
>> http://marc.info/?l=linux-kernel&m=123827903216314&w=2
>>
>> But the changes to swiotlb is minor. BTW, I have the patches for
>> powerpc too.
>>
>>> Ingo, any comments on that?
>
> if it's genuine fixes we can do it post-rc1, but it looks a tad wide
> for that:
>
> arch/x86/kernel/pci-swiotlb.c |    2 +-
> include/linux/swiotlb.h       |    3 +-
> lib/swiotlb.c                 |  115 +++++++++++++++++++++ 
> +-------------------
>
> the patches came right in the merge window - that's too late for
> IOMMU bits, the patch cutoff is generally the beginning of the merge
> window. (Which was on March 23 in this window)

Fair.

> But i can queue them up in the .31 queue if it has all the Acks from
> you folks. Becky, mind resending the latest version, with all the
> acks in place?

I can do this in Becky's abscense this week.  The only "acks" I've  
seen are from FUJITA Tomonori.

Can we ask there be a unique swiotlb branch in your tree to make it  
easier for us to continue additional swiotlb work as well as arch  
specific work.

- k

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Ingo Molnar]

* Kumar Gala <galak@kernel.crashing.org> wrote:

>
> On Apr 8, 2009, at 7:43 AM, Ingo Molnar wrote:
>
>>
>> * FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>>
>>>> Same here.  It makes it easier for us to work on the powerpc arch
>>>> specific changes for .31 if we can get these into .30.  What are you
>>>> looking at for .31?
>>>
>>> I need to finish the dma_mapping_ops cleanups and cross-arch
>>> unification stuff:
>>>
>>> http://marc.info/?l=linux-kernel&m=123827903216314&w=2
>>>
>>> But the changes to swiotlb is minor. BTW, I have the patches for
>>> powerpc too.
>>>
>>>> Ingo, any comments on that?
>>
>> if it's genuine fixes we can do it post-rc1, but it looks a tad wide
>> for that:
>>
>> arch/x86/kernel/pci-swiotlb.c |    2 +-
>> include/linux/swiotlb.h       |    3 +-
>> lib/swiotlb.c                 |  115 +++++++++++++++++++++ 
>> +-------------------
>>
>> the patches came right in the merge window - that's too late for
>> IOMMU bits, the patch cutoff is generally the beginning of the merge
>> window. (Which was on March 23 in this window)
>
> Fair.
>
>> But i can queue them up in the .31 queue if it has all the Acks from
>> you folks. Becky, mind resending the latest version, with all the
>> acks in place?
>
> I can do this in Becky's abscense this week.  The only "acks" I've 
> seen are from FUJITA Tomonori.

_your_ ack (or signoff) counts too - so to me it's plural :-)

> Can we ask there be a unique swiotlb branch in your tree to make 
> it easier for us to continue additional swiotlb work as well as 
> arch specific work.

yes - tip:core/iommu is already distinct from tip:x86/iommu and is 
intended for such more generic patches. It has no patches pending 
right now (it just got all merged upstream) - so please use an 
upstream base - 2.6.30-rc1 would be a good starting point.

Thanks,

	Ingo

Re: [PATCH 5/7] swiotlb: (re)Create swiotlb_unmap_single

[Kumar Gala]

>>>
>>> the patches came right in the merge window - that's too late for
>>> IOMMU bits, the patch cutoff is generally the beginning of the merge
>>> window. (Which was on March 23 in this window)
>>
>> Fair.
>>
>>> But i can queue them up in the .31 queue if it has all the Acks from
>>> you folks. Becky, mind resending the latest version, with all the
>>> acks in place?
>>
>> I can do this in Becky's abscense this week.  The only "acks" I've
>> seen are from FUJITA Tomonori.
>
> _your_ ack (or signoff) counts too - so to me it's plural :-)

I've added my signoff :)

>> Can we ask there be a unique swiotlb branch in your tree to make
>> it easier for us to continue additional swiotlb work as well as
>> arch specific work.
>
> yes - tip:core/iommu is already distinct from tip:x86/iommu and is
> intended for such more generic patches. It has no patches pending
> right now (it just got all merged upstream) - so please use an
> upstream base - 2.6.30-rc1 would be a good starting point.

Just reposted v3 based on 2.6.30-rc1

- k

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

[FUJITA Tomonori]

On Fri,  3 Apr 2009 20:56:42 -0500
Becky Bruce <beckyb@kernel.crashing.org> wrote:

> This is v2 of 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.
> 
> In response to commentary on the previous series, I've also
> refactored the code a bit, altough I did this slightly
> differently than was suggested because I noticed we could use the
> new helper function in 2 places instead of one. I've reformatted
> a bit of code based on commentary as well.
> 
> I have not tested this in any way on any non-ppc platforms,
> so commentary/testing from x86/ia64 folks is, once again,
> greatly appreciated.
> 
> I'm going to be offline for the next week, but will respond to
> commentary as soon as I return.
> 
> Cheers,
> Becky
> 
> diffstat:
> arch/x86/kernel/pci-swiotlb.c |    2 +-
> include/linux/swiotlb.h       |    3 +-
> lib/swiotlb.c                 |  115 ++++++++++++++++++++++-------------------
> 3 files changed, 64 insertions(+), 56 deletions(-)

I have a minor comment on 5/7 but the rest looks fine to me.