[PATCH] [POWERPC] Add struct iommu_table argument to iommu_map_sg()

[PATCH] [POWERPC] Add struct iommu_table argument to iommu_map_sg()

[Mark Nelson]

Make iommu_map_sg take a struct iommu_table. It did so before commit
740c3ce66700640a6e6136ff679b067e92125794 (iommu sg merging: ppc: make
iommu respect the segment size limits).

This stops the function looking in the archdata.dma_data for the iommu
table because in the future it will be called with a device that has
no table there.

This also has the nice side effect of making iommu_map_sg() match the
other map functions.

Signed-off-by: Mark Nelson <markn@au1.ibm.com>
---
 arch/powerpc/kernel/dma_64.c |    2 +-
 arch/powerpc/kernel/iommu.c  |    7 +++----
 include/asm-powerpc/iommu.h  |    6 +++---
 3 files changed, 7 insertions(+), 8 deletions(-)

Index: upstream/arch/powerpc/kernel/dma_64.c
===================================================================
--- upstream.orig/arch/powerpc/kernel/dma_64.c
+++ upstream/arch/powerpc/kernel/dma_64.c
@@ -68,7 +68,7 @@ static void dma_iommu_unmap_single(struc
 static int dma_iommu_map_sg(struct device *dev, struct scatterlist *sglist,
 			    int nelems, enum dma_data_direction direction)
 {
-	return iommu_map_sg(dev, sglist, nelems,
+	return iommu_map_sg(dev, dev->archdata.dma_data, sglist, nelems,
 			    device_to_mask(dev), direction);
 }
 
Index: upstream/arch/powerpc/kernel/iommu.c
===================================================================
--- upstream.orig/arch/powerpc/kernel/iommu.c
+++ upstream/arch/powerpc/kernel/iommu.c
@@ -267,11 +267,10 @@ static void iommu_free(struct iommu_tabl
 	spin_unlock_irqrestore(&(tbl->it_lock), flags);
 }
 
-int iommu_map_sg(struct device *dev, struct scatterlist *sglist,
-		 int nelems, unsigned long mask,
-		 enum dma_data_direction direction)
+int iommu_map_sg(struct device *dev, struct iommu_table *tbl,
+		 struct scatterlist *sglist, int nelems,
+		 unsigned long mask, enum dma_data_direction direction)
 {
-	struct iommu_table *tbl = dev->archdata.dma_data;
 	dma_addr_t dma_next = 0, dma_addr;
 	unsigned long flags;
 	struct scatterlist *s, *outs, *segstart;
Index: upstream/include/asm-powerpc/iommu.h
===================================================================
--- upstream.orig/include/asm-powerpc/iommu.h
+++ upstream/include/asm-powerpc/iommu.h
@@ -79,9 +79,9 @@ extern void iommu_free_table(struct iomm
 extern struct iommu_table *iommu_init_table(struct iommu_table * tbl,
 					    int nid);
 
-extern int iommu_map_sg(struct device *dev, struct scatterlist *sglist,
-			int nelems, unsigned long mask,
-			enum dma_data_direction direction);
+extern int iommu_map_sg(struct device *dev, struct iommu_table *tbl,
+			struct scatterlist *sglist, int nelems,
+			unsigned long mask, enum dma_data_direction direction);
 extern void iommu_unmap_sg(struct iommu_table *tbl, struct scatterlist *sglist,
 			   int nelems, enum dma_data_direction direction);
 

Re: [PATCH] [POWERPC] Add struct iommu_table argument to iommu_map_sg()

[Olof Johansson]

On Tue, Apr 29, 2008 at 03:17:45PM +1000, Mark Nelson wrote:
> Make iommu_map_sg take a struct iommu_table. It did so before commit
> 740c3ce66700640a6e6136ff679b067e92125794 (iommu sg merging: ppc: make
> iommu respect the segment size limits).
>
> This stops the function looking in the archdata.dma_data for the iommu
> table because in the future it will be called with a device that has
> no table there.

The logical thing would be to add the archdata.dma_data to said device
instead, no? Without seeing the rest of the code that makes use of it
it's hard to tell anyway, so please post that.

> This also has the nice side effect of making iommu_map_sg() match the
> other map functions.

Consistency is good, but I wonder if the opposite wouldn't be the better
way to go here: always just pass down just the dev pointer instead. The
table can be reached from it.


-Olof

Re: [PATCH] [POWERPC] Add struct iommu_table argument to iommu_map_sg()

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 583 bytes --]

Hi Olof,

On Tue, 29 Apr 2008 12:54:17 -0500 Olof Johansson <olof@lixom.net> wrote:
>
> Consistency is good, but I wonder if the opposite wouldn't be the better
> way to go here: always just pass down just the dev pointer instead. The
> table can be reached from it.

I made the changes that passed the iommu_table so that e.g. iSeries could
call iommu_alloc_coherent without a dev pointer at all so that the
generic powerpc code could be cleaned up quite a bit.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] [POWERPC] Add struct iommu_table argument to iommu_map_sg()

[Mark Nelson]

Olof Johansson wrote:
> On Tue, Apr 29, 2008 at 03:17:45PM +1000, Mark Nelson wrote:
>> Make iommu_map_sg take a struct iommu_table. It did so before commit
>> 740c3ce66700640a6e6136ff679b067e92125794 (iommu sg merging: ppc: make
>> iommu respect the segment size limits).
>>
>> This stops the function looking in the archdata.dma_data for the iommu
>> table because in the future it will be called with a device that has
>> no table there.
> 
> The logical thing would be to add the archdata.dma_data to said device
> instead, no? Without seeing the rest of the code that makes use of it
> it's hard to tell anyway, so please post that.

I'll post the follow on series of patches as an RFC to give you a better
understanding of what I'm trying to do; but basically I'm building on
the new dma_*map*_attrs() interfaces that Andrew Morton sent to Linus
last night and implementing them for powerpc (in particular the Cell
platform). To cut a longish story short, the fixed linear mapping for
the Cell's IOMMU can be set to either be weakly or strongly ordered,
and so a device that has been identified as being able to use the
fixed mapping can later request a mapping that cannot be satisfied using
the fixed mapping (eg: it may request a weakly ordered mapping when
the fixed mapping is strongly ordered, or vice versa). Devices that
use the fixed mapping use the dma_direct_ops with their dma_data being
the necessary offset, but for the case above they have to use the
iommu_map*() functions that the dma_iommu_ops use. So that's how we end
up calling iommu_map_sg() with a device that doesn't have the table
in the dma_data (and already has something useful in dma_data).

I'll try to get the follow on series of patches cleaned up today and
send them out asap, so if my ramblings above turned out to be too
obscure the code will hopefully clear things up.

Thanks!

Mark.

> 
>> This also has the nice side effect of making iommu_map_sg() match the
>> other map functions.
> 
> Consistency is good, but I wonder if the opposite wouldn't be the better
> way to go here: always just pass down just the dev pointer instead. The
> table can be reached from it.
> 
> 
> -Olof
>