Hi Ben,

thanks for signaling this long-standing issue again.

At Tue, 06 May 2008 10:08:28 +1000,
Benjamin Herrenschmidt wrote:
quoted text
> 
> Hi Takashi !
> 
> I'm bringing up an old thread as I'm just discovering that the problem
> still hasn't been fixed.
> 
> There seem to be a few issues with ALSA current usage of mmap vs. non
> cache coherent architecture, such as embedded PowerPC's.

Yep.  And on MIPS, obviously.

quoted text
> I can see at least two with a quick look to pcm-native.c, one I don't
> understand and one I think I do:
> 
>  - The control/status mapping. Can you elaborate a bit on what this is
> actually doing and why it shouldn't be done on "non coherent"
> architectures ?

This is a mmap of the data record to be shared in realtime with apps.
The app updates its data pointer (appl_ptr) on the mmapped buffer
while the driver updates the data (e.g. DMA position, called hwptr) on
the fly on the mmapped record.  Due to its real-time nature, it has to
be coherent -- at least, it was a problem on ARM.

quoted text
> Currently this -is- done on all powerpc's, whether they
> are coherent or not and I want to understand what the underlying issue
> is.

It's actually buggy.  Should check more precisely.

quoted text
>  - The mmap of DMA pages. Here, the problem appears two fold:
> 
> 	* Use of virt_to_page() on virtual addresses returned by
> dma_alloc_coherent().
> 
> 	* No using the appropriate page protection for a DMA coherent mapping
> to userspace.
> 
> It seems like you have solved that in part with implementing a generic
> dma_mmap_coherent() in the past that for some reason you never merged
> upstream (I can track that to about 2 years ago). Is there a reason ?

IIRC, dma_mmap_coherent() cannot be implemented properly on some
architectures.  This is no big problem for ALSA as long as it returns
an error or make it out via ifdef.  But, the fact that this API cannot
be done for all archs discourage arch maintainers, and the idea faded
out again.

quoted text
> I think we need to at least apply a band-aid today as it's becoming a
> nasty issue for several non-coherent powerpc platforms. It could be in
> the form of implementing dma_mmap_coherent() and changing Alsa to use it
> with the appropriate ifdef, or just adding an ifdef CONFIG_PPC with the
> right code in there for now until a better solution is found.

Agreed.

quoted text
> It should be trivial though. Getting the PFN from the DMA address is
> easy if we have the dma handle and the virtual address, though that -is-
> definitely platform specific. I can implement a function for that if you
> need.

That'll be great.  dma_mmap_coherent() and friends would be then
really helpful to solve this issue.

quoted text
> As for the pgprot, we can come up with something like
> pgprot_mmap_dma(). Either that or I can fold it all in a powerpc wide
> implementation of a dma_mmap_coherent() like we envisioned initially.

In principle, pgprot_*() isn't actually needed in the driver side at
all.  We use pgprot_noncached() in one part, and it's for hacky way to
mmap the ioremapped pages.  It's not available on all architectures,
and I'm not sure whether it works on all PPC models although it's
enabled right now: in include/sound/pcm.h,

/* mmap for io-memory area */
#if defined(CONFIG_X86) || defined(CONFIG_PPC) || defined(CONFIG_ALPHA)
#define SNDRV_PCM_INFO_MMAP_IOMEM	SNDRV_PCM_INFO_MMAP
int snd_pcm_lib_mmap_iomem(struct snd_pcm_substream *substream, struct vm_area_struct *area);
#else
#define SNDRV_PCM_INFO_MMAP_IOMEM	0
#define snd_pcm_lib_mmap_iomem	NULL
#endif

Highly likely we need to fix this, too.  In the easiest way, disable
this except for X86...

quoted text
> Let me know what approach is preferred here and I'll come up with
> patches ASAP. As far as I'm concerned, this is a bug and thus must be
> fixed now for .26 and possibly backported to stable even if we can come
> up with a non invasive solution). I'm annoyed because it represents a
> trivial amount of code, this problem should have been fixed a long time
> ago.

As a pragmatic solution, as you mentioned in the above, we can disable
or change the problematic code with ifdefs.  At best, use
dma_mmap_coherent() if it's available.  If not, and if the arch is
known to have not-simply-mappable DMA pages (like MIPS), we can simply
disable the mmap feature.

Once after we have dma_mmap_*() generally, we can clean up codes.


thanks,

Takashi
--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/