| Message ID | 1323278391-21849-1-git-send-email-galak@kernel.crashing.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Benjamin Herrenschmidt |
| Headers | show |
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.
On Dec 7, 2011, at 9:23 PM, Benjamin Herrenschmidt wrote: > 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. Becky any comment? I know its been a while, but wondering if you had any reason to not do what Ben's suggesting ? - k
On Dec 7, 2011, at 11:46 PM, Kumar Gala wrote: > > On Dec 7, 2011, at 9:23 PM, Benjamin Herrenschmidt wrote: > >> 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. > > Becky any comment? > > 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
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.
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. >> >> 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... 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
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.
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,
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(-)