On Tue, 2010-06-29 at 15:04 +0900, FUJITA Tomonori wrote: > On Mon, 28 Jun 2010 09:55:58 -0500 > James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > I think that we can safely remove the above usage: > > > > > > > > > > - such old systems haven't triger the above checking for long. > > > > > > > > > > - the above condition is important for systems that can't allocate > > > > > coherent memory if these systems do DMA. So probably it would be > > > > > better to have such checking in arch's DMA initialization code > > > > > instead of a driver. > > > > > > > > Well, we can't check in the architecture because it's a driver specific > > > > thing ... I suppose making it a rule that dma_get_cache_alignment() > > > > *must* be <= L1_CACHE_BYTES fixes it ... we seem to have no architecture > > > > violating that, so just add it to the documentation, and the check can > > > > go. > > > > > > Seems that on some architectures (arm and mips at least), > > > dma_get_cache_alignment() could greater than L1_CACHE_BYTES. But they > > > simply return the possible maximum size of cache size like: > > > > > > static inline int dma_get_cache_alignment(void) > > > { > > > /* XXX Largest on any MIPS */ > > > return 128; > > > } > > > > > > So practically, we should be safe. I guess that we can simply convert > > > them to return L1_CACHE_BYTES. > > > > As long as that's architecturally true, yes. I mean I can't imagine > > any architecture that had a dma alignment requirement that was greater > > than its L1 cache width ... but I've been surprised be for making > > "Obviously this can't happen ..." type statements where MIPS is > > concerned. > > How about using ARCH_KMALLOC_MINALIGN instead of L1_CACHE_BYTES? > > In the previous merge window, we made sure that all the architectures > defines the minimum alignment and width of DMA properly (and the fully > coherent architectures don't define ARCH_KMALLOC_MINALIGN). > > dma_get_cache_alignment should be equal to ARCH_KMALLOC_MINALIGN if an > architecture defines ARCH_KMALLOC_MINALIGN (probably, > dma_get_cache_alignment() can be implemented in the common place with > ARCH_KMALLOC_MINALIGN. It would be better to rename > ARCH_KMALLOC_MINALIGN to something like ARCH_DMA_MINALIGN). > > It might be better to place DMA_ALIGN(x) in the common place. Seems > that some drivers wrongly use L1_CACHE_ALIGN() to get the dma > alignment. Well, using cache alignment magic in drivers isn't a good > idea though... > > = > From: FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> > Subject: [PATCH] 53c700: remove dma_is_consistent usage in 53c700 > > ARCH_KMALLOC_MINALIGN returns the minimum alignment and width of DMA > on architectures that define ARCH_KMALLOC_MINALIGN (if it's not > defined, architectures are fully coherent). > > So we can use ARCH_KMALLOC_MINALIGN instead of L1_CACHE_BYTES and > safely remove the alignment checking. Actually, I'd rather not do this. The reason is that L1_CACHE_ALIGN is quite a big performance optimisation on x86 for the driver. Without it, it's functionally correct, but the DMA use of the mailboxes really thrashes the cache which damages performance (x86 has ARCH_KMALLOC_MINALIGN set to 8 ... the default) The only correctness problem, which the BUG is checking for is mismatch in dma alignment ... as I said, I'm happy just to rely on that being correct on every incoherent platform the driver operates on. James -- To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html