At Wed, 20 Aug 2008 11:27:12 -0500, James Bottomley wrote: > > On Mon, 2008-08-18 at 15:21 +0200, Takashi Iwai wrote: > > I've been trying to fix a long-standing bug in ALSA, the mmap of > > pages via dma_mmap_coherent(). Since the sound drivers need to expose > > the whole buffer via mmap and the buffers are allocated via > > dma_alloc_coherent(), it causes Oops on some architectures like MIPS. > > One of the fix patches is this one, the addition of > > dma_mmap_coherent() to MIPS architecture. > > > > This implementation is pretty lazy (and untested) as you see below. > > > > The whole patches are found on topic/dma-fix branch on sound-2.6 git > > tree > > git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound-2.6.git > > > > The gitweb URL of the branch is: > > http://git.kernel.org/?p=linux/kernel/git/tiwai/sound-2.6.git;a=shortlog;h=topic/dma-fix > > > > Any review and comments would be appreciated. > > I've inlined the parisc piece ... although it would be helpful if you > had posted it to the parisc list rather than letting the mips people > forward it. Oh sorry, I just wanted to discuss things first about MIPS implementation to sort things out (I did with Ben about PPC patch). But, appears better to discuss together, and I'm happy that you join. > I'm afraid there are several problems. The first is that it doesn't do > what you want. You can't map a coherent page to userspace (which is at > a non congruent address on parisc) and still expect it to be > coherent ... there's going to have to be fiddling with the page table > caches to make sure coherency isn't destroyed by aliasing effects Hmm... how bad would be the coherency with such a simple mmap method? In most cases, we don't need the "perfect" coherency. Usually one process mmaps the whole buffer and keep reading/writing. There is another use case (sharing the mmapped buffer by multiple processes), but this can be disabled if we know it's not feasible beforehand. > Secondly, it's incomplete ... there are two other instances of > hppa_dma_ops in drivers/parisc ccio-dma.c and sba_iommu.c that would > also need to have this added OK, that must be fixed. > Finally, there's the meta observation that this is exactly the type of > thing that the framebuffer code already does, so why reinvent a new way > of doing it rather than coming up with the correct infrastructure and > making them both use it? I don't think the method framebuffer using is so portable as reusable. And, when the buffer is allocated via dma_mmap_coherent(), the fb method can't be used anyway. On x86 and others, dma_alloc_coherent() is the right method to allocate pages, I suppose. Thanks! Takashi > > James > > --- > > From: Takashi Iwai <tiwai@xxxxxxx> > Date: Tue, 17 Jun 2008 14:39:05 +0000 (+0200) > Subject: parisc: implement dma_mmap_coherent() > X-Git-Url: http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Ftiwai%2Fsound-2.6.git;a=commitdiff_plain;h=7a9dbc6e9d4798fd00005faebeff720c87f098df;hp=fb959151b618360fc74b8978d918182c82f67a4a > > parisc: implement dma_mmap_coherent() > > A lazy version of dma_mmap_coherent() implementation for PA-RISC. > > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> > --- > > diff --git a/arch/parisc/kernel/pci-dma.c b/arch/parisc/kernel/pci-dma.c > index ccd61b9..ddeecc2 100644 > --- a/arch/parisc/kernel/pci-dma.c > +++ b/arch/parisc/kernel/pci-dma.c > @@ -545,6 +545,19 @@ static void pa11_dma_sync_sg_for_device(struct device *dev, struct scatterlist * > flush_kernel_dcache_range(sg_virt_addr(sglist), sglist->length); > } > > +static int pa11_dma_mmap_coherent(struct device *dev, > + struct vm_area_struct *vma, > + void *cpu_addr, dma_addr_t handle, > + size_t size) > +{ > + struct page *pg; > + cpu_addr = __va(handle); > + pg = virt_to_page(cpu_addr); > + return remap_pfn_range(vma, vma->vm_start, > + page_to_pfn(pg) + vma->vm_pgoff, > + size, vma->vm_page_prot); > +} > + > struct hppa_dma_ops pcxl_dma_ops = { > .dma_supported = pa11_dma_supported, > .alloc_consistent = pa11_dma_alloc_consistent, > @@ -558,6 +571,7 @@ struct hppa_dma_ops pcxl_dma_ops = { > .dma_sync_single_for_device = pa11_dma_sync_single_for_device, > .dma_sync_sg_for_cpu = pa11_dma_sync_sg_for_cpu, > .dma_sync_sg_for_device = pa11_dma_sync_sg_for_device, > + .mmap_coherent = pa11_dma_mmap_coherent, > }; > > static void *fail_alloc_consistent(struct device *dev, size_t size, > @@ -598,4 +612,5 @@ struct hppa_dma_ops pcx_dma_ops = { > .dma_sync_single_for_device = pa11_dma_sync_single_for_device, > .dma_sync_sg_for_cpu = pa11_dma_sync_sg_for_cpu, > .dma_sync_sg_for_device = pa11_dma_sync_sg_for_device, > + .mmap_coherent = pa11_dma_mmap_coherent, > }; > diff --git a/include/asm-parisc/dma-mapping.h b/include/asm-parisc/dma-mapping.h > index 53af696..a66235b 100644 > --- a/include/asm-parisc/dma-mapping.h > +++ b/include/asm-parisc/dma-mapping.h > @@ -19,6 +19,9 @@ struct hppa_dma_ops { > void (*dma_sync_single_for_device)(struct device *dev, dma_addr_t iova, unsigned long offset, size_t size, enum dma_data_direction direction); > void (*dma_sync_sg_for_cpu)(struct device *dev, struct scatterlist *sg, int nelems, enum dma_data_direction direction); > void (*dma_sync_sg_for_device)(struct device *dev, struct scatterlist *sg, int nelems, enum dma_data_direction direction); > + int (*mmap_coherent)(struct device *dev, struct vm_area_struct *vma, > + void *cpu_addr, dma_addr_t handle, size_t size); > + > }; > > /* > @@ -204,6 +207,13 @@ dma_cache_sync(struct device *dev, void *vaddr, size_t size, > flush_kernel_dcache_range((unsigned long)vaddr, size); > } > > +static inline int > +dma_mmap_coherent(struct device *dev, struct vm_area_struct *vma, > + void *cpu_addr, dma_addr_t handle, size_t size) > +{ > + return hppa_dma_ops->mmap_coherent(dev, vma, cpu_addr, handle, size); > +} > + > static inline void * > parisc_walk_tree(struct device *dev) > { >
