[PATCH] powerpc: Fix swiotlb ops for ppc64

[PATCH] powerpc: Fix swiotlb ops for ppc64

[Kumar Gala]

We assumed before that alloc_coherent & free_coherent ops would always
be direct because of 32-bit systems and how we utilize highmem & lowmem.
However, on 64-bit systems we typically treat all memory as lowmem so
the same assumptions are not valid.  We need to utilze the swiotlb
versions of alloc_coherent & free_coherent on 64-bit systems.

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
 arch/powerpc/kernel/dma-swiotlb.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/dma-swiotlb.c b/arch/powerpc/kernel/dma-swiotlb.c
index 1ebc918..5000fd4 100644
--- a/arch/powerpc/kernel/dma-swiotlb.c
+++ b/arch/powerpc/kernel/dma-swiotlb.c
@@ -40,15 +40,20 @@ static u64 swiotlb_powerpc_get_required(struct device *dev)
 }
 
 /*
- * At the moment, all platforms that use this code only require
- * swiotlb to be used if we're operating on HIGHMEM.  Since
+ * We assume that 32-bit systems will utilize HIGHMEM and that we're
+ * able to DMA directly to anything in the LOWMEM region. Since
  * we don't ever call anything other than map_sg, unmap_sg,
  * map_page, and unmap_page on highmem, use normal dma_ops
  * for everything else.
  */
 struct dma_map_ops swiotlb_dma_ops = {
+#ifdef CONFIG_PPC64
+	.alloc_coherent = swiotlb_alloc_coherent,
+	.free_coherent = swiotlb_free_coherent,
+#else
 	.alloc_coherent = dma_direct_alloc_coherent,
 	.free_coherent = dma_direct_free_coherent,
+#endif
 	.map_sg = swiotlb_map_sg_attrs,
 	.unmap_sg = swiotlb_unmap_sg_attrs,
 	.dma_supported = swiotlb_dma_supported,
-- 
1.7.3.4

Re: [PATCH] powerpc: Fix swiotlb ops for ppc64

[Benjamin Herrenschmidt]

On Wed, 2011-12-07 at 11:19 -0600, Kumar Gala wrote:

>  struct dma_map_ops swiotlb_dma_ops = {
> +#ifdef CONFIG_PPC64
> +	.alloc_coherent = swiotlb_alloc_coherent,
> +	.free_coherent = swiotlb_free_coherent,
> +#else
>  	.alloc_coherent = dma_direct_alloc_coherent,
>  	.free_coherent = dma_direct_free_coherent,
> +#endif
>  	.map_sg = swiotlb_map_sg_attrs,
>  	.unmap_sg = swiotlb_unmap_sg_attrs,
>  	.dma_supported = swiotlb_dma_supported,

Do we really need the ifdef ? What happens if we use
swiotlb_alloc_coherent() on ppc32 ? Won't it allocate lowmem, realize
that it doesn't need bouncing and be happy ?

Cheers,
Ben.

Re: [PATCH] powerpc: Fix swiotlb ops for ppc64

[Kumar Gala]

On Dec 7, 2011, at 9:23 PM, Benjamin Herrenschmidt wrote:

> On Wed, 2011-12-07 at 11:19 -0600, Kumar Gala wrote:
>=20
>> struct dma_map_ops swiotlb_dma_ops =3D {
>> +#ifdef CONFIG_PPC64
>> +	.alloc_coherent =3D swiotlb_alloc_coherent,
>> +	.free_coherent =3D swiotlb_free_coherent,
>> +#else
>> 	.alloc_coherent =3D dma_direct_alloc_coherent,
>> 	.free_coherent =3D dma_direct_free_coherent,
>> +#endif
>> 	.map_sg =3D swiotlb_map_sg_attrs,
>> 	.unmap_sg =3D swiotlb_unmap_sg_attrs,
>> 	.dma_supported =3D swiotlb_dma_supported,
>=20
> Do we really need the ifdef ? What happens if we use
> swiotlb_alloc_coherent() on ppc32 ? Won't it allocate lowmem, realize
> that it doesn't need bouncing and be happy ?
>=20
> Cheers,
> Ben.

Becky any comment?

I know its been a while, but wondering if you had any reason to not do =
what Ben's suggesting ?

- k=

Re: [PATCH] powerpc: Fix swiotlb ops for ppc64

[Becky Bruce]

On Dec 7, 2011, at 11:46 PM, Kumar Gala wrote:

>=20
> On Dec 7, 2011, at 9:23 PM, Benjamin Herrenschmidt wrote:
>=20
>> On Wed, 2011-12-07 at 11:19 -0600, Kumar Gala wrote:
>>=20
>>> struct dma_map_ops swiotlb_dma_ops =3D {
>>> +#ifdef CONFIG_PPC64
>>> +	.alloc_coherent =3D swiotlb_alloc_coherent,
>>> +	.free_coherent =3D swiotlb_free_coherent,
>>> +#else
>>> 	.alloc_coherent =3D dma_direct_alloc_coherent,
>>> 	.free_coherent =3D dma_direct_free_coherent,
>>> +#endif
>>> 	.map_sg =3D swiotlb_map_sg_attrs,
>>> 	.unmap_sg =3D swiotlb_unmap_sg_attrs,
>>> 	.dma_supported =3D swiotlb_dma_supported,
>>=20
>> Do we really need the ifdef ? What happens if we use
>> swiotlb_alloc_coherent() on ppc32 ? Won't it allocate lowmem, realize
>> that it doesn't need bouncing and be happy ?
>>=20
>> Cheers,
>> Ben.
>=20
> Becky any comment?
>=20
> I know its been a while, but wondering if you had any reason to not do =
what Ben's suggesting ?

Well, as you say, it's been a while, and but I think:

1) dma_direct_alloc_coherent strips GFP_HIGHMEM out of the flags field =
when calling the actual allocator and the iotlb version does not.  I =
don't know how much this matters - I did a quick grep and I don't see =
any users that specify that, but somebody went through the trouble of =
putting it in there in the first place and without knowing why I wasn't =
willing to get rid of it.  Now, since my patch it looks like someone =
added a VM_BUG_ON into __get_free_pages() if GFP_HIGHMEM so this would =
get caught.  However, I don't know if we really want to throw a bug =
there.

2)  The iotlb code doesn't deal with the !coherent parts like 8xx.  We =
can work around that by setting up the dma_ops differently for that case =
instead.

-Becky

Re: [PATCH] powerpc: Fix swiotlb ops for ppc64

[Benjamin Herrenschmidt]

On Mon, 2011-12-12 at 21:55 -0600, Becky Bruce wrote:
> 1) dma_direct_alloc_coherent strips GFP_HIGHMEM out of the flags field
> when calling the actual allocator and the iotlb version does not.  I
> don't know how much this matters - I did a quick grep and I don't see
> any users that specify that, but somebody went through the trouble of
> putting it in there in the first place and without knowing why I
> wasn't willing to get rid of it.  Now, since my patch it looks like
> someone added a VM_BUG_ON into __get_free_pages() if GFP_HIGHMEM so
> this would get caught.  However, I don't know if we really want to
> throw a bug there.
> 
> 2)  The iotlb code doesn't deal with the !coherent parts like 8xx.  We
> can work around that by setting up the dma_ops differently for that
> case instead.

Does the rest of it handle them ? I mean swiotlb_map_sg_attrs etc...

If not then it's broken anyway so may as well not care for now.

Cheers,
Ben.

Re: [PATCH] powerpc: Fix swiotlb ops for ppc64

[Becky Bruce]

On Dec 12, 2011, at 10:27 PM, Benjamin Herrenschmidt wrote:

> On Mon, 2011-12-12 at 21:55 -0600, Becky Bruce wrote:
>> 1) dma_direct_alloc_coherent strips GFP_HIGHMEM out of the flags =
field
>> when calling the actual allocator and the iotlb version does not.  I
>> don't know how much this matters - I did a quick grep and I don't see
>> any users that specify that, but somebody went through the trouble of
>> putting it in there in the first place and without knowing why I
>> wasn't willing to get rid of it.  Now, since my patch it looks like
>> someone added a VM_BUG_ON into __get_free_pages() if GFP_HIGHMEM so
>> this would get caught.  However, I don't know if we really want to
>> throw a bug there.
>>=20
>> 2)  The iotlb code doesn't deal with the !coherent parts like 8xx.  =
We
>> can work around that by setting up the dma_ops differently for that
>> case instead.
>=20
> Does the rest of it handle them ? I mean swiotlb_map_sg_attrs etc...

The non-coherent "specialness" is in the dma sync stuff and no, I don't =
think the iotlb stuff deals with that properly.

Do you not have a problem with 1)?  If not then I think we can look at =
switching over; 2) was more of a secondary thing.

-B

Re: [PATCH] powerpc: Fix swiotlb ops for ppc64

[Benjamin Herrenschmidt]

On Tue, 2011-12-13 at 20:53 -0600, Becky Bruce wrote:
> The non-coherent "specialness" is in the dma sync stuff and no, I
> don't think the iotlb stuff deals with that properly.
> 
> Do you not have a problem with 1)?  If not then I think we can look at
> switching over; 2) was more of a secondary thing.

So let's switch over, avoid an #ifdef which is always a good thing...

Cheers,
Ben.