On Thu, 28 May 2009 21:04:55 +0100
Arnd Bergmann <[email protected]> wrote:
> This adds a version of the dma-mapping API to asm-generic that can be
> used by most architectures that only need a linear mapping.
>
> An architecture using this still needs to provide definitions for
> dma_get_cache_alignment, dma_cache_sync and a new dma_coherent_dev
> function as well as out-of-line versions of dma_alloc_coherent and
> dma_free_coherent.
>
> Thanks to Fujita-san for an endless supply of feedback.
>
> Cc: FUJITA Tomonori <[email protected]>
> Signed-off-by: Arnd Bergmann <[email protected]>
>
> ---
> include/asm-generic/dma-mapping-linear.h | 336 ++++++++++++++++++++++++++++++
> 1 files changed, 336 insertions(+), 0 deletions(-)
> create mode 100644 include/asm-generic/dma-mapping-linear.h
>
> ---
>
> I've added this version to
> git://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic#next
>
> I also have patches to convert the existing architectures to use
> it, but I plan to submit those to the arch maintainers once asm-generic
> series has been merged.
IMO, this need to be merged with some users. We don't want to merge
something that nobody wants to use.
> Please ack if you are happy with this version.
>
> diff --git a/include/asm-generic/dma-mapping-linear.h b/include/asm-generic/dma-mapping-linear.h
> new file mode 100644
> index 0000000..c3b987d
> --- /dev/null
> +++ b/include/asm-generic/dma-mapping-linear.h
> @@ -0,0 +1,336 @@
> +#ifndef __ASM_GENERIC_DMA_MAPPING_H
> +#define __ASM_GENERIC_DMA_MAPPING_H
> +
> +#include <linux/mm.h>
> +#include <linux/device.h>
> +#include <linux/dma-debug.h>
> +#include <linux/scatterlist.h>
> +#include <asm/cacheflush.h>
> +#include <asm/io.h>
> +
> +/**
> + * dma_alloc_coherent - allocate consistent memory for DMA
> + * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices
> + * @size: required memory size
> + * @handle: bus-specific DMA address
> + *
> + * Allocate some uncached, unbuffered memory for a device for
> + * performing DMA. This function allocates pages, and will
> + * return the CPU-viewed address, and sets @handle to be the
> + * device-viewed address.
> + */
> +extern void *
> +dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
> + gfp_t flag);
> +
> +/**
> + * dma_free_coherent - free memory allocated by dma_alloc_coherent
> + * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices
> + * @size: size of memory originally requested in dma_alloc_coherent
> + * @cpu_addr: CPU-view address returned from dma_alloc_coherent
> + * @handle: device-view address returned from dma_alloc_coherent
> + *
> + * Free (and unmap) a DMA buffer previously allocated by
> + * dma_alloc_coherent().
> + *
> + * References to memory and mappings associated with cpu_addr/handle
> + * during and after this call executing are illegal.
> + */
> +extern void
> +dma_free_coherent(struct device *dev, size_t size, void *cpu_addr,
> + dma_addr_t dma_handle);
> +
> +#define dma_alloc_noncoherent(d, s, h, f) dma_alloc_coherent(d, s, h, f)
> +#define dma_free_noncoherent(d, s, v, h) dma_free_coherent(d, s, v, h)
> +
> +/**
> + * dma_map_single - map a single buffer for streaming DMA
> + * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices
> + * @cpu_addr: CPU direct mapped address of buffer
> + * @size: size of buffer to map
> + * @dir: DMA transfer direction
> + *
> + * Ensure that any data held in the cache is appropriately discarded
> + * or written back.
> + *
> + * The device owns this memory once this call has completed. The CPU
> + * can regain ownership by calling dma_unmap_single() or dma_sync_single().
> + */
> +static inline dma_addr_t
> +dma_map_single(struct device *dev, void *ptr, size_t size,
> + enum dma_data_direction direction)
> +{
> + dma_addr_t dma_addr = virt_to_bus(ptr);
> + BUG_ON(!valid_dma_direction(direction));
> +
> + if (!dma_coherent_dev(dev))
> + dma_cache_sync(dev, ptr, size, direction);
Where can I find dma_coherent_dev?
I don't fancy this since this is architecture-specific stuff (not
generic things).
> + debug_dma_map_page(dev, virt_to_page(ptr),
> + (unsigned long)ptr & ~PAGE_MASK, size,
> + direction, dma_addr, true);
> +
> + return dma_addr;
> +}
> +
> +/**
> + * dma_unmap_single - unmap a single buffer previously mapped
> + * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices
> + * @handle: DMA address of buffer
> + * @size: size of buffer to map
> + * @dir: DMA transfer direction
> + *
> + * Unmap a single streaming mode DMA translation. The handle and size
> + * must match what was provided in the previous dma_map_single() call.
> + * All other usages are undefined.
> + *
> + * After this call, reads by the CPU to the buffer are guaranteed to see
> + * whatever the device wrote there.
> + */
> +static inline void
> +dma_unmap_single(struct device *dev, dma_addr_t dma_addr, size_t size,
> + enum dma_data_direction direction)
> +{
> + debug_dma_unmap_page(dev, dma_addr, size, direction, true);
> +}
> +
> +/**
> + * dma_map_sg - map a set of SG buffers for streaming mode DMA
> + * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices
> + * @sg: list of buffers
> + * @nents: number of buffers to map
> + * @dir: DMA transfer direction
> + *
> + * Map a set of buffers described by scatterlist in streaming
> + * mode for DMA. This is the scatter-gather version of the
> + * above pci_map_single interface. Here the scatter gather list
> + * elements are each tagged with the appropriate dma address
> + * and length. They are obtained via sg_dma_{address,length}(SG).
> + *
> + * NOTE: An implementation may be able to use a smaller number of
> + * DMA address/length pairs than there are SG table elements.
> + * (for example via virtual mapping capabilities)
> + * The routine returns the number of addr/length pairs actually
> + * used, at most nents.
> + *
> + * Device ownership issues as mentioned above for pci_map_single are
> + * the same here.
> + */
> +static inline int
> +dma_map_sg(struct device *dev, struct scatterlist *sglist, int nents,
> + enum dma_data_direction direction)
> +{
> + struct scatterlist *sg;
> + int i, sync;
> +
> + BUG_ON(!valid_dma_direction(direction));
> + WARN_ON(nents == 0 || sglist[0].length == 0);
> +
> + sync = !dma_coherent_dev(dev);
> +
> + for_each_sg(sglist, sg, nents, i) {
> + BUG_ON(!sg_page(sg));
> +
> + sg->dma_address = page_to_bus(sg_page(sg)) + sg->offset;
> + sg_dma_len(sg) = sg->length;
> + if (sync)
> + dma_cache_sync(dev, sg_virt(sg), sg->length, direction);
> + }
> +
> + debug_dma_map_sg(dev, sg, nents, i, direction);
> +
> + return nents;
> +}
> +
> +/**
> + * dma_unmap_sg - unmap a set of SG buffers mapped by dma_map_sg
> + * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices
> + * @sg: list of buffers
> + * @nents: number of buffers to map
> + * @dir: DMA transfer direction
> + *
> + * Unmap a set of streaming mode DMA translations.
> + * Again, CPU read rules concerning calls here are the same as for
> + * pci_unmap_single() above.
> + */
> +static inline void
> +dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nhwentries,
> + enum dma_data_direction direction)
> +{
> + debug_dma_unmap_sg(dev, sg, nhwentries, direction);
> +}
> +
> +/**
> + * dma_map_page - map a portion of a page for streaming DMA
> + * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices
> + * @page: page that buffer resides in
> + * @offset: offset into page for start of buffer
> + * @size: size of buffer to map
> + * @dir: DMA transfer direction
> + *
> + * Ensure that any data held in the cache is appropriately discarded
> + * or written back.
> + *
> + * The device owns this memory once this call has completed. The CPU
> + * can regain ownership by calling dma_unmap_page() or dma_sync_single().
> + */
> +static inline dma_addr_t
> +dma_map_page(struct device *dev, struct page *page, unsigned long offset,
> + size_t size, enum dma_data_direction direction)
> +{
> + return dma_map_single(dev, page_address(page) + offset,
> + size, direction);
> +}
> +
> +/**
> + * dma_unmap_page - unmap a buffer previously mapped through dma_map_page()
> + * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices
> + * @handle: DMA address of buffer
> + * @size: size of buffer to map
> + * @dir: DMA transfer direction
> + *
> + * Unmap a single streaming mode DMA translation. The handle and size
> + * must match what was provided in the previous dma_map_single() call.
> + * All other usages are undefined.
> + *
> + * After this call, reads by the CPU to the buffer are guaranteed to see
> + * whatever the device wrote there.
> + */
> +static inline void
> +dma_unmap_page(struct device *dev, dma_addr_t dma_address, size_t size,
> + enum dma_data_direction direction)
> +{
> + dma_unmap_single(dev, dma_address, size, direction);
> +}
> +
> +/**
> + * dma_sync_single_for_cpu
> + * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices
> + * @handle: DMA address of buffer
> + * @size: size of buffer to map
> + * @dir: DMA transfer direction
> + *
> + * Make physical memory consistent for a single streaming mode DMA
> + * translation after a transfer.
> + *
> + * If you perform a dma_map_single() but wish to interrogate the
> + * buffer using the cpu, yet do not wish to teardown the DMA mapping,
> + * you must call this function before doing so. At the next point you
> + * give the DMA address back to the card, you must first perform a
> + * dma_sync_single_for_device, and then the device again owns the
> + * buffer.
> + */
> +static inline void
> +dma_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle, size_t size,
> + enum dma_data_direction direction)
> +{
> + debug_dma_sync_single_for_cpu(dev, dma_handle, size, direction);
> +}
> +
> +static inline void
> +dma_sync_single_range_for_cpu(struct device *dev, dma_addr_t dma_handle,
> + unsigned long offset, size_t size,
> + enum dma_data_direction direction)
> +{
> + debug_dma_sync_single_range_for_cpu(dev, dma_handle,
> + offset, size, direction);
> +}
This looks wrong. You put dma_coherent_dev hook in sync_*_for_device
but why you don't need it sync_*_for_cpu. It's architecture
specific. Some need both, some need either, and some need nothing.
> +/**
> + * dma_sync_sg_for_cpu
> + * @dev: valid struct device pointer, or NULL for ISA and EISA-like devices
> + * @sg: list of buffers
> + * @nents: number of buffers to map
> + * @dir: DMA transfer direction
> + *
> + * Make physical memory consistent for a set of streaming
> + * mode DMA translations after a transfer.
> + *
> + * The same as dma_sync_single_for_* but for a scatter-gather list,
> + * same rules and usage.
> + */
> +static inline void
> +dma_sync_sg_for_cpu(struct device *dev, struct scatterlist *sg, int nents,
> + enum dma_data_direction direction)
> +{
> + debug_dma_sync_sg_for_cpu(dev, sg, nents, direction);
> +}
> +
> +static inline void
> +dma_sync_single_for_device(struct device *dev, dma_addr_t dma_handle,
> + size_t size, enum dma_data_direction direction)
> +{
> + if (!dma_coherent_dev(dev))
> + dma_cache_sync(dev, bus_to_virt(dma_handle), size, direction);
> + debug_dma_sync_single_for_device(dev, dma_handle, size, direction);
> +}
> +
> +static inline void
> +dma_sync_single_range_for_device(struct device *dev, dma_addr_t dma_handle,
> + unsigned long offset, size_t size,
> + enum dma_data_direction direction)
> +{
> + if (!dma_coherent_dev(dev))
> + dma_cache_sync(dev, bus_to_virt(dma_handle), size, direction);
> + debug_dma_sync_single_range_for_device(dev, dma_handle,
> + offset, size, direction);
> +}
> +
> +static inline void
> +dma_sync_sg_for_device(struct device *dev, struct scatterlist *sglist,
> + int nents, enum dma_data_direction direction)
> +{
> + struct scatterlist *sg;
> + int i;
> +
> + if (!dma_coherent_dev(dev))
> + for_each_sg(sglist, sg, nents, i)
> + dma_cache_sync(dev, sg_virt(sg), sg->length, direction);
> +
> + debug_dma_sync_sg_for_device(dev, sg, nents, direction);
> +}
> +
> +static inline int
> +dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
> +{
> + return 0;
> +}
> +
> +/*
> + * Return whether the given device DMA address mask can be supported
> + * properly. For example, if your device can only drive the low 24-bits
> + * during bus mastering, then you would pass 0x00ffffff as the mask
> + * to this function.
> + */
> +static inline int
> +dma_supported(struct device *dev, u64 mask)
> +{
> + /*
> + * we fall back to GFP_DMA when the mask isn't all 1s,
> + * so we can't guarantee allocations that must be
> + * within a tighter range than GFP_DMA.
> + */
> + if (mask < 0x00ffffff)
> + return 0;
I think that this is pretty architecture specific.
> +
> + return 1;
> +}
> +
> +static inline int
> +dma_set_mask(struct device *dev, u64 dma_mask)
> +{
> + if (!dev->dma_mask || !dma_supported(dev, dma_mask))
> + return -EIO;
> +
> + *dev->dma_mask = dma_mask;
> +
> + return 0;
> +}
> +
> +static inline int
> +dma_is_consistent(struct device *dev, dma_addr_t dma_addr)
> +{
> + return dma_coherent_dev(dev);
> +}
> +
> +#endif /* __ASM_GENERIC_DMA_MAPPING_H */
> --
> 1.6.3.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Mon, Jun 01, 2009 at 01:02:42PM +0900, FUJITA Tomonori wrote:
> Where can I find dma_coherent_dev?
>
> I don't fancy this since this is architecture-specific stuff (not
> generic things).
It _is_ very architecture specific. The coherent-ness of devices hardly
depends on the device itself. Eg, PCI devices on x86 are coherent, but
on ARM they aren't.
> > +static inline void
> > +dma_unmap_single(struct device *dev, dma_addr_t dma_addr, size_t size,
> > + enum dma_data_direction direction)
> > +{
> > + debug_dma_unmap_page(dev, dma_addr, size, direction, true);
Future ARMs will need to do something on unmaps.
> > +static inline void
> > +dma_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle, size_t size,
> > + enum dma_data_direction direction)
> > +{
> > + debug_dma_sync_single_for_cpu(dev, dma_handle, size, direction);
> > +}
> > +
> > +static inline void
> > +dma_sync_single_range_for_cpu(struct device *dev, dma_addr_t dma_handle,
> > + unsigned long offset, size_t size,
> > + enum dma_data_direction direction)
> > +{
> > + debug_dma_sync_single_range_for_cpu(dev, dma_handle,
> > + offset, size, direction);
> > +}
>
> This looks wrong. You put dma_coherent_dev hook in sync_*_for_device
> but why you don't need it sync_*_for_cpu. It's architecture
> specific. Some need both, some need either, and some need nothing.
If you're non-coherent, you need to implement the sync APIs.
> > +static inline int
> > +dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
> > +{
> > + return 0;
So mappings never fail?
> > +}
> > +
> > +/*
> > + * Return whether the given device DMA address mask can be supported
> > + * properly. For example, if your device can only drive the low 24-bits
> > + * during bus mastering, then you would pass 0x00ffffff as the mask
> > + * to this function.
> > + */
> > +static inline int
> > +dma_supported(struct device *dev, u64 mask)
> > +{
> > + /*
> > + * we fall back to GFP_DMA when the mask isn't all 1s,
> > + * so we can't guarantee allocations that must be
> > + * within a tighter range than GFP_DMA.
> > + */
> > + if (mask < 0x00ffffff)
> > + return 0;
>
> I think that this is pretty architecture specific.
It is - it depends exactly on how you setup the DMA zone and whether
all your RAM is DMA-able.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Mon, 1 Jun 2009 08:51:14 +0100
Russell King <[email protected]> wrote:
> On Mon, Jun 01, 2009 at 01:02:42PM +0900, FUJITA Tomonori wrote:
> > Where can I find dma_coherent_dev?
> >
> > I don't fancy this since this is architecture-specific stuff (not
> > generic things).
>
> It _is_ very architecture specific. The coherent-ness of devices hardly
> depends on the device itself. Eg, PCI devices on x86 are coherent, but
> on ARM they aren't.
>
> > > +static inline void
> > > +dma_unmap_single(struct device *dev, dma_addr_t dma_addr, size_t size,
> > > + enum dma_data_direction direction)
> > > +{
> > > + debug_dma_unmap_page(dev, dma_addr, size, direction, true);
>
> Future ARMs will need to do something on unmaps.
>
> > > +static inline void
> > > +dma_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle, size_t size,
> > > + enum dma_data_direction direction)
> > > +{
> > > + debug_dma_sync_single_for_cpu(dev, dma_handle, size, direction);
> > > +}
> > > +
> > > +static inline void
> > > +dma_sync_single_range_for_cpu(struct device *dev, dma_addr_t dma_handle,
> > > + unsigned long offset, size_t size,
> > > + enum dma_data_direction direction)
> > > +{
> > > + debug_dma_sync_single_range_for_cpu(dev, dma_handle,
> > > + offset, size, direction);
> > > +}
> >
> > This looks wrong. You put dma_coherent_dev hook in sync_*_for_device
> > but why you don't need it sync_*_for_cpu. It's architecture
> > specific. Some need both, some need either, and some need nothing.
>
> If you're non-coherent, you need to implement the sync APIs.
>
> > > +static inline int
> > > +dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
> > > +{
> > > + return 0;
>
> So mappings never fail?
Hmm, possibly, you might misunderstand what he is trying to do.
http://lkml.org/lkml/2009/5/28/393:
This adds a version of the dma-mapping API to asm-generic that can be
used by most architectures that only need a linear mapping.
=
The original thread is here (I'm not sure if we can have something
clean and useful like this header file):
http://lkml.org/lkml/2009/5/19/262
I think that ARM can't be an user of this (ARM needs more complicated
dma mapping stuff).
On Mon, Jun 01, 2009 at 05:08:09PM +0900, FUJITA Tomonori wrote:
> This adds a version of the dma-mapping API to asm-generic that can be
> used by most architectures that only need a linear mapping.
It depends what is meant by "linear mapping".
If that's just a way of saying "all that needs to be done for the
DMA streaming APIs is to flush the cache" then the vast majority of
ARMs fall into that category.
The DMA bounce code is a broken design concept that really needs to
be put to death.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Mon, 1 Jun 2009 09:29:43 +0100
Russell King <[email protected]> wrote:
> On Mon, Jun 01, 2009 at 05:08:09PM +0900, FUJITA Tomonori wrote:
> > This adds a version of the dma-mapping API to asm-generic that can be
> > used by most architectures that only need a linear mapping.
>
> It depends what is meant by "linear mapping".
>
> If that's just a way of saying "all that needs to be done for the
> DMA streaming APIs is to flush the cache" then the vast majority of
> ARMs fall into that category.
I guess that his definition is 'no dynamic remapping'.
> The DMA bounce code is a broken design concept that really needs to
> be put to death.
You are talking about arch/arm/common/dmabounce.c? If so, it sounds
more interesting (to me at least). It's kinda swiotlb per device,
right? What you want to do for arch/arm/common/dmabounce.c?
On Mon, Jun 01, 2009 at 06:16:56PM +0900, FUJITA Tomonori wrote:
> On Mon, 1 Jun 2009 09:29:43 +0100
> Russell King <[email protected]> wrote:
>
> > On Mon, Jun 01, 2009 at 05:08:09PM +0900, FUJITA Tomonori wrote:
> > > This adds a version of the dma-mapping API to asm-generic that can be
> > > used by most architectures that only need a linear mapping.
> >
> > It depends what is meant by "linear mapping".
> >
> > If that's just a way of saying "all that needs to be done for the
> > DMA streaming APIs is to flush the cache" then the vast majority of
> > ARMs fall into that category.
>
> I guess that his definition is 'no dynamic remapping'.
... which as I say is what ARM does for the streaming mappings.
> > The DMA bounce code is a broken design concept that really needs to
> > be put to death.
>
> You are talking about arch/arm/common/dmabounce.c? If so, it sounds
> more interesting (to me at least). It's kinda swiotlb per device,
> right? What you want to do for arch/arm/common/dmabounce.c?
Yes.
It's a nasty hack which leads to OOMs on various platforms since it
causes additional memory pressure from parts of the kernel which we
don't expect, and also causes additional difficulty with allocating
and freeing DMA memory from IRQ context. There's an unsolved bug in
the kernel bugzilla for this which I see no hope of ever being resolved.
I _really_ wish that dmabounce never existed. It's a right royal
pain in the preverbial.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Mon, 1 Jun 2009 10:22:59 +0100
Russell King <[email protected]> wrote:
> On Mon, Jun 01, 2009 at 06:16:56PM +0900, FUJITA Tomonori wrote:
> > On Mon, 1 Jun 2009 09:29:43 +0100
> > Russell King <[email protected]> wrote:
> >
> > > On Mon, Jun 01, 2009 at 05:08:09PM +0900, FUJITA Tomonori wrote:
> > > > This adds a version of the dma-mapping API to asm-generic that can be
> > > > used by most architectures that only need a linear mapping.
> > >
> > > It depends what is meant by "linear mapping".
> > >
> > > If that's just a way of saying "all that needs to be done for the
> > > DMA streaming APIs is to flush the cache" then the vast majority of
> > > ARMs fall into that category.
> >
> > I guess that his definition is 'no dynamic remapping'.
>
> ... which as I say is what ARM does for the streaming mappings.
Ok, then we agree.
> > > The DMA bounce code is a broken design concept that really needs to
> > > be put to death.
> >
> > You are talking about arch/arm/common/dmabounce.c? If so, it sounds
> > more interesting (to me at least). It's kinda swiotlb per device,
> > right? What you want to do for arch/arm/common/dmabounce.c?
>
> Yes.
>
> It's a nasty hack which leads to OOMs on various platforms since it
> causes additional memory pressure from parts of the kernel which we
> don't expect, and also causes additional difficulty with allocating
> and freeing DMA memory from IRQ context. There's an unsolved bug in
> the kernel bugzilla for this which I see no hope of ever being resolved.
>
> I _really_ wish that dmabounce never existed. It's a right royal
> pain in the preverbial.
Can ARM replace the dmabounce with swiotlb?
On Monday 01 June 2009, FUJITA Tomonori wrote:
> On Thu, 28 May 2009 21:04:55 +0100
> > I've added this version to
> > git://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic#next
> >
> > I also have patches to convert the existing architectures to use
> > it, but I plan to submit those to the arch maintainers once asm-generic
> > series has been merged.
>
> IMO, this need to be merged with some users. We don't want to merge
> something that nobody wants to use.
I've deliberately kept my asm-generic tree free from architecture specific
code for now, in order to make the merging process much easier.
I do have branches in there that convert x86 and microblaze to use
all the files that they can share with the generic implementation
and the plan is to do more of those over time, but to avoid dealing
with all arch maintainers at the same time.
Would it be ok for you if I only do this on microblaze for now (which
does not have a dma-mapping.h yet) and leave the conversion of
more architectures for later?
Otherwise, I'd probably just skip dma-mapping.h in its entirety
because there is a different is a simpler alternative for new architectures
(setting CONFIG_NO_DMA).
> > +static inline dma_addr_t
> > +dma_map_single(struct device *dev, void *ptr, size_t size,
> > + enum dma_data_direction direction)
> > +{
> > + dma_addr_t dma_addr = virt_to_bus(ptr);
> > + BUG_ON(!valid_dma_direction(direction));
> > +
> > + if (!dma_coherent_dev(dev))
> > + dma_cache_sync(dev, ptr, size, direction);
>
> Where can I find dma_coherent_dev?
>
> I don't fancy this since this is architecture-specific stuff (not
> generic things).
I made this up in order to deal with the different requirements of
the architectures I converted. In SH, it's truly device specific
(pci may be coherent, others are never), on cris it returns false
and on most others always true.
Architectures like x86 and frv could use this hook to do th
global flush_write_buffers() but still return zero so that
dma_sync_sg_for_device does not have to iterate all SG
elements doing nothing for them.
Maybe it should be named __dma_coherent_dev() to make clear
that it is a helper internal to dma_mapping.h and not supposed to
be used by device drivers.
> > +static inline void
> > +dma_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle, size_t size,
> > + enum dma_data_direction direction)
> > +{
> > + debug_dma_sync_single_for_cpu(dev, dma_handle, size, direction);
> > +}
> > +
> > +static inline void
> > +dma_sync_single_range_for_cpu(struct device *dev, dma_addr_t dma_handle,
> > + unsigned long offset, size_t size,
> > + enum dma_data_direction direction)
> > +{
> > + debug_dma_sync_single_range_for_cpu(dev, dma_handle,
> > + offset, size, direction);
> > +}
>
> This looks wrong. You put dma_coherent_dev hook in sync_*_for_device
> but why you don't need it sync_*_for_cpu. It's architecture
> specific. Some need both, some need either, and some need nothing.
It took me a while to find out what the architectures do in case of
dma_sync_single_for_*. The noncoherent implementations all flush
and invalidate the dcache in _for_device, which seems reasonable.
In arch/sh and arch/xtensa, we also flush and invalidate the
dcache in *_for_cpu.
This does not seem to make sense to me, because there should
not be any valid cache lines for that region at that time. My plan
was to submit patches to sh and xtensa to no longer flush the
dcache in _for_cpu with that argument before submitting the patch
to use the common code. Maybe Paul or Chris can comment on
whether there is any reason for the dma flush here, if there is
one, we probably should also flush in dma_unmap_*.
AFAICT, the _for_cpu hooks are only required for swiotlb
or for an obscure HW implementation of an IOMMU.
> > +static inline int
> > +dma_supported(struct device *dev, u64 mask)
> > +{
> > + /*
> > + * we fall back to GFP_DMA when the mask isn't all 1s,
> > + * so we can't guarantee allocations that must be
> > + * within a tighter range than GFP_DMA.
> > + */
> > + if (mask < 0x00ffffff)
> > + return 0;
>
> I think that this is pretty architecture specific.
Yes, I wondered about this one. I suspect that the same code got
copied blindly from x86 into all the other architectures. The
same code exists in arm, cris, frv and mn10300, while avr32, sh
and xtensa seem to assume that nobody passes a smaller than
24-bit mask in there and always return 1. They clearly could not
handle that situation either, so the version I chose is the
conservative approach.
Do you have a better suggestion?
Arnd <><
On Mon, Jun 01, 2009 at 06:32:44PM +0900, FUJITA Tomonori wrote:
> Can ARM replace the dmabounce with swiotlb?
We could, if there was some effort with swiotlb to make it safe on
non-cache coherent architectures. As it stands, it sources its
memory from __get_free_pages() which is non-coherent on ARM.
There's nothing wrong with that per-se, provided we use the streaming
DMA mapping API on those pages and obey the buffer ownership rules.
Also, it needs cache flushing - one thing davem said to me a while
back is that the effects of DMA should be the same as far as the
cache is concerned no matter how it is performed.
In other words, from the external perspective:
dma_map_single()
hardware DMA happens
dma_unmap_single()
should result in exactly the same situation on an architecture as
far as the buffer is concerned no matter what the underlying
implementation is.
So, on a non-DMA coherent cache architecture, when DMA is normally
performed the data ends up in RAM with the cache flushed for that
region. If, instead dma_map_single uses a bounce buffer to do that
DMA, then the same needs to be true of the original buffer - the
data needs to be in RAM with the cache flushed.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Monday 01 June 2009, Russell King wrote:
> On Mon, Jun 01, 2009 at 05:08:09PM +0900, FUJITA Tomonori wrote:
> > This adds a version of the dma-mapping API to asm-generic that can be
> > used by most architectures that only need a linear mapping.
>
> It depends what is meant by "linear mapping".
>
> If that's just a way of saying "all that needs to be done for the
> DMA streaming APIs is to flush the cache" then the vast majority of
> ARMs fall into that category.
Right. You can probably split out the arm dma-mapping.h implementation
into the dmabounce version and a version that falls back to my
asm-generic code.
One feature that the arm code currently has that I'm still missing is
highmem support, which seems to be relatively complex in arm
(three different implementations). Not sure how to best fit that in there.
Arnd <><
On Monday 01 June 2009, Russell King wrote:
> So, on a non-DMA coherent cache architecture, when DMA is normally
> performed the data ends up in RAM with the cache flushed for that
> region. If, instead dma_map_single uses a bounce buffer to do that
> DMA, then the same needs to be true of the original buffer - the
> data needs to be in RAM with the cache flushed.
While this seems logical from a correctness perspective, I would
like to understand why it actually matters. Flushing the cache on
the original buffer will impact performance but doesn't generally
make a difference to users. In cases where you need the cache
to be flushed for aliasing reasons (VIPT caches...), the architecture
specific code should flush that buffer somewhere, but do we really
need to flush it for all architectures?
Arnd <><
On Mon, Jun 01, 2009 at 11:28:09AM +0100, Arnd Bergmann wrote:
> On Monday 01 June 2009, Russell King wrote:
> > On Mon, Jun 01, 2009 at 05:08:09PM +0900, FUJITA Tomonori wrote:
> > > This adds a version of the dma-mapping API to asm-generic that can be
> > > used by most architectures that only need a linear mapping.
> >
> > It depends what is meant by "linear mapping".
> >
> > If that's just a way of saying "all that needs to be done for the
> > DMA streaming APIs is to flush the cache" then the vast majority of
> > ARMs fall into that category.
>
> Right. You can probably split out the arm dma-mapping.h implementation
> into the dmabounce version and a version that falls back to my
> asm-generic code.
>
> One feature that the arm code currently has that I'm still missing is
> highmem support, which seems to be relatively complex in arm
> (three different implementations). Not sure how to best fit that in there.
Err - there's only one highmem implementation on ARM.
Anyway, as I hinted, things in this area will most likely become more
complex in the near future, so any unification of the ARM dma-mapping
will have to be undone.
Let's wait for the ARMv7 support to mature before trying to unify too
much.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Monday 01 June 2009, Russell King wrote:
>
> Err - there's only one highmem implementation on ARM.
Sorry for not being clear enough, I meant three implementations of
page_to_dma(), depending on highmem and platform:
return (dma_addr_t)__virt_to_bus((unsigned long)page_address(page));
return (dma_addr_t)__pfn_to_bus(page_to_pfn(page));
return __arch_page_to_dma(dev, page);
The generic code right now does the first, which won't work with highmem
and also won't handle platform specific requirements that are not present
on the other architectures.
Arnd <><
On Mon, Jun 01, 2009 at 11:41:32AM +0100, Arnd Bergmann wrote:
> On Monday 01 June 2009, Russell King wrote:
>
> > So, on a non-DMA coherent cache architecture, when DMA is normally
> > performed the data ends up in RAM with the cache flushed for that
> > region. If, instead dma_map_single uses a bounce buffer to do that
> > DMA, then the same needs to be true of the original buffer - the
> > data needs to be in RAM with the cache flushed.
>
> While this seems logical from a correctness perspective, I would
> like to understand why it actually matters. Flushing the cache on
> the original buffer will impact performance but doesn't generally
> make a difference to users. In cases where you need the cache
> to be flushed for aliasing reasons (VIPT caches...), the architecture
> specific code should flush that buffer somewhere, but do we really
> need to flush it for all architectures?
I didn't say "for all architectures". I said that the end conditions
need to be the same no matter how DMA is done.
And yes, it does matter with some cache types. VIPT aliasing caches
and VIVT caches both need to ensure that condition is met, otherwise
userspace doesn't see the data.
While we can hand-wave and say "some other part of the code should
handle this" I've had that disucssion several times, and that's where
this requirement eventually was stated. And, really, I'm not going
to re-discuss it yet again - I really don't have time or motivation
at present to be involved in yet another hand-waving egotistical
debate over it.
Last time I got accused of not being helpful because I wouldn't test
a patch - and the reason I couldn't test the patch was because I don't
have the hardware which exhibited the problem. Duh.
So, let's leave sleeping dogs to continue their deep sleep.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Monday 01 June 2009, Russell King wrote:
> I didn't say "for all architectures". I said that the end conditions
> need to be the same no matter how DMA is done.
>
> And yes, it does matter with some cache types. VIPT aliasing caches
> and VIVT caches both need to ensure that condition is met, otherwise
> userspace doesn't see the data.
Ok, thanks for the explanation.
> While we can hand-wave and say "some other part of the code should
> handle this" I've had that disucssion several times, and that's where
> this requirement eventually was stated. And, really, I'm not going
> to re-discuss it yet again - I really don't have time or motivation
> at present to be involved in yet another hand-waving egotistical
> debate over it.
I was not trying to start a debate over this, just being curious.
Arnd <><
Arnd Bergmann wrote:
> On Monday 01 June 2009, FUJITA Tomonori wrote:
>
>> On Thu, 28 May 2009 21:04:55 +0100
>>
>>> I've added this version to
>>> git://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic#next
>>>
>>> I also have patches to convert the existing architectures to use
>>> it, but I plan to submit those to the arch maintainers once asm-generic
>>> series has been merged.
>>>
>> IMO, this need to be merged with some users. We don't want to merge
>> something that nobody wants to use.
>>
>
> I've deliberately kept my asm-generic tree free from architecture specific
> code for now, in order to make the merging process much easier.
>
> I do have branches in there that convert x86 and microblaze to use
> all the files that they can share with the generic implementation
> and the plan is to do more of those over time, but to avoid dealing
> with all arch maintainers at the same time.
>
> Would it be ok for you if I only do this on microblaze for now (which
> does not have a dma-mapping.h yet) and leave the conversion of
> more architectures for later?
>
Microblaze have it but it is not cleared(checked) and not in mainline -
I want to look at it when mmu is in mainline.
As I wrote before you can use Microblaze as tested arch.
Michal
> Otherwise, I'd probably just skip dma-mapping.h in its entirety
> because there is a different is a simpler alternative for new architectures
> (setting CONFIG_NO_DMA).
>
>
>>> +static inline dma_addr_t
>>> +dma_map_single(struct device *dev, void *ptr, size_t size,
>>> + enum dma_data_direction direction)
>>> +{
>>> + dma_addr_t dma_addr = virt_to_bus(ptr);
>>> + BUG_ON(!valid_dma_direction(direction));
>>> +
>>> + if (!dma_coherent_dev(dev))
>>> + dma_cache_sync(dev, ptr, size, direction);
>>>
>> Where can I find dma_coherent_dev?
>>
>> I don't fancy this since this is architecture-specific stuff (not
>> generic things).
>>
>
> I made this up in order to deal with the different requirements of
> the architectures I converted. In SH, it's truly device specific
> (pci may be coherent, others are never), on cris it returns false
> and on most others always true.
>
> Architectures like x86 and frv could use this hook to do th
> global flush_write_buffers() but still return zero so that
> dma_sync_sg_for_device does not have to iterate all SG
> elements doing nothing for them.
>
> Maybe it should be named __dma_coherent_dev() to make clear
> that it is a helper internal to dma_mapping.h and not supposed to
> be used by device drivers.
>
>
>>> +static inline void
>>> +dma_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle, size_t size,
>>> + enum dma_data_direction direction)
>>> +{
>>> + debug_dma_sync_single_for_cpu(dev, dma_handle, size, direction);
>>> +}
>>> +
>>> +static inline void
>>> +dma_sync_single_range_for_cpu(struct device *dev, dma_addr_t dma_handle,
>>> + unsigned long offset, size_t size,
>>> + enum dma_data_direction direction)
>>> +{
>>> + debug_dma_sync_single_range_for_cpu(dev, dma_handle,
>>> + offset, size, direction);
>>> +}
>>>
>> This looks wrong. You put dma_coherent_dev hook in sync_*_for_device
>> but why you don't need it sync_*_for_cpu. It's architecture
>> specific. Some need both, some need either, and some need nothing.
>>
>
> It took me a while to find out what the architectures do in case of
> dma_sync_single_for_*. The noncoherent implementations all flush
> and invalidate the dcache in _for_device, which seems reasonable.
>
> In arch/sh and arch/xtensa, we also flush and invalidate the
> dcache in *_for_cpu.
> This does not seem to make sense to me, because there should
> not be any valid cache lines for that region at that time. My plan
> was to submit patches to sh and xtensa to no longer flush the
> dcache in _for_cpu with that argument before submitting the patch
> to use the common code. Maybe Paul or Chris can comment on
> whether there is any reason for the dma flush here, if there is
> one, we probably should also flush in dma_unmap_*.
>
> AFAICT, the _for_cpu hooks are only required for swiotlb
> or for an obscure HW implementation of an IOMMU.
>
>
>>> +static inline int
>>> +dma_supported(struct device *dev, u64 mask)
>>> +{
>>> + /*
>>> + * we fall back to GFP_DMA when the mask isn't all 1s,
>>> + * so we can't guarantee allocations that must be
>>> + * within a tighter range than GFP_DMA.
>>> + */
>>> + if (mask < 0x00ffffff)
>>> + return 0;
>>>
>> I think that this is pretty architecture specific.
>>
>
> Yes, I wondered about this one. I suspect that the same code got
> copied blindly from x86 into all the other architectures. The
> same code exists in arm, cris, frv and mn10300, while avr32, sh
> and xtensa seem to assume that nobody passes a smaller than
> 24-bit mask in there and always return 1. They clearly could not
> handle that situation either, so the version I chose is the
> conservative approach.
>
> Do you have a better suggestion?
>
> Arnd <><
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arch" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Michal Simek, Ing. (M.Eng)
PetaLogix - Linux Solutions for a Reconfigurable World
w: http://www.petalogix.com p: +61-7-30090663,+42-0-721842854 f: +61-7-30090663
On Monday 01 June 2009, Michal Simek wrote:
> Microblaze have it but it is not cleared(checked) and not in mainline -
> I want to look at it when mmu is in mainline.
> As I wrote before you can use Microblaze as tested arch.
Ok. It should become really easy once the asm-generic version is there.
Do you know if all DMA capable devices on microblaze are coherent
(or if all of them are noncoherent)?
If it is indeed coherent, the below code should be enough, otherwise
you need to add some cache flushes in the functions below.
Arnd <><
---
#ifndef _ASM_MICROBLAZE_DMA_MAPPING_H
#define _ASM_MICROBLAZE_DMA_MAPPING_H
static inline int
__dma_coherent_dev(struct device *dev)
{
return 1;
}
static inline void
dma_cache_sync(struct device *dev, void *cpu_addr, size_t size,
enum dma_data_direction direction)
{
}
static inline int dma_get_cache_alignment(void)
{
return 1 << L1_CACHE_SHIFT;
}
#include <asm-generic/dma-mapping-linear.h>
#endif _ASM_MICROBLAZE_DMA_MAPPING_H
Arnd Bergmann wrote:
> On Monday 01 June 2009, Michal Simek wrote:
>
>
>> Microblaze have it but it is not cleared(checked) and not in mainline -
>> I want to look at it when mmu is in mainline.
>> As I wrote before you can use Microblaze as tested arch.
>>
>
> Ok. It should become really easy once the asm-generic version is there.
> Do you know if all DMA capable devices on microblaze are coherent
> (or if all of them are noncoherent)?
>
We don't have cache coherency modul.
> If it is indeed coherent, the below code should be enough, otherwise
> you need to add some cache flushes in the functions below.
>
I'll send you special email about to find out proper solution for
Microblaze.
Michal
> Arnd <><
>
> ---
> #ifndef _ASM_MICROBLAZE_DMA_MAPPING_H
> #define _ASM_MICROBLAZE_DMA_MAPPING_H
>
> static inline int
> __dma_coherent_dev(struct device *dev)
> {
> return 1;
> }
>
> static inline void
> dma_cache_sync(struct device *dev, void *cpu_addr, size_t size,
> enum dma_data_direction direction)
> {
> }
>
> static inline int dma_get_cache_alignment(void)
> {
> return 1 << L1_CACHE_SHIFT;
> }
>
> #include <asm-generic/dma-mapping-linear.h>
>
> #endif _ASM_MICROBLAZE_DMA_MAPPING_H
>
--
Michal Simek, Ing. (M.Eng)
PetaLogix - Linux Solutions for a Reconfigurable World
w: http://www.petalogix.com p: +61-7-30090663,+42-0-721842854 f: +61-7-30090663
On Mon, 1 Jun 2009 11:11:27 +0100
Arnd Bergmann <[email protected]> wrote:
> On Monday 01 June 2009, FUJITA Tomonori wrote:
> > On Thu, 28 May 2009 21:04:55 +0100
> > > I've added this version to
> > > git://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic#next
> > >
> > > I also have patches to convert the existing architectures to use
> > > it, but I plan to submit those to the arch maintainers once asm-generic
> > > series has been merged.
> >
> > IMO, this need to be merged with some users. We don't want to merge
> > something that nobody wants to use.
>
> I've deliberately kept my asm-generic tree free from architecture specific
> code for now, in order to make the merging process much easier.
I understand what you mean however we need to merge something
useful. I don't say that you need to merge everything together however
we can't know if this is useful or not unless you show how some
architectures use it.
> I do have branches in there that convert x86 and microblaze to use
x86? How can it use asm-generic-linear.h?
> all the files that they can share with the generic implementation
> and the plan is to do more of those over time, but to avoid dealing
> with all arch maintainers at the same time.
>
> Would it be ok for you if I only do this on microblaze for now (which
> does not have a dma-mapping.h yet) and leave the conversion of
> more architectures for later?
> Otherwise, I'd probably just skip dma-mapping.h in its entirety
> because there is a different is a simpler alternative for new architectures
> (setting CONFIG_NO_DMA).
At least one architecture sounds fine to me.
> > > +static inline dma_addr_t
> > > +dma_map_single(struct device *dev, void *ptr, size_t size,
> > > + enum dma_data_direction direction)
> > > +{
> > > + dma_addr_t dma_addr = virt_to_bus(ptr);
> > > + BUG_ON(!valid_dma_direction(direction));
> > > +
> > > + if (!dma_coherent_dev(dev))
> > > + dma_cache_sync(dev, ptr, size, direction);
> >
> > Where can I find dma_coherent_dev?
> >
> > I don't fancy this since this is architecture-specific stuff (not
> > generic things).
>
> I made this up in order to deal with the different requirements of
> the architectures I converted. In SH, it's truly device specific
> (pci may be coherent, others are never), on cris it returns false
> and on most others always true.
I know what you are trying here.
However, dma_cache_sync() is an exported function to drivers; which
should be used only with a buffer returned by dma_alloc_noncoherent()
(see DMA-API.txt). So using dma_cache_sync() in this way is confusing
to me.
I might misunderstand dma_cache_sync (recently I learned what it
should do).
> Architectures like x86 and frv could use this hook to do th
> global flush_write_buffers() but still return zero so that
> dma_sync_sg_for_device does not have to iterate all SG
> elements doing nothing for them.
>
> Maybe it should be named __dma_coherent_dev() to make clear
> that it is a helper internal to dma_mapping.h and not supposed to
> be used by device drivers.
Might be. However, we have dma_is_consistent() for a different purpose
so it might be confusing anyway.
> > > +static inline void
> > > +dma_sync_single_for_cpu(struct device *dev, dma_addr_t dma_handle, size_t size,
> > > + enum dma_data_direction direction)
> > > +{
> > > + debug_dma_sync_single_for_cpu(dev, dma_handle, size, direction);
> > > +}
> > > +
> > > +static inline void
> > > +dma_sync_single_range_for_cpu(struct device *dev, dma_addr_t dma_handle,
> > > + unsigned long offset, size_t size,
> > > + enum dma_data_direction direction)
> > > +{
> > > + debug_dma_sync_single_range_for_cpu(dev, dma_handle,
> > > + offset, size, direction);
> > > +}
> >
> > This looks wrong. You put dma_coherent_dev hook in sync_*_for_device
> > but why you don't need it sync_*_for_cpu. It's architecture
> > specific. Some need both, some need either, and some need nothing.
>
> It took me a while to find out what the architectures do in case of
> dma_sync_single_for_*. The noncoherent implementations all flush
> and invalidate the dcache in _for_device, which seems reasonable.
>
> In arch/sh and arch/xtensa, we also flush and invalidate the
> dcache in *_for_cpu.
> This does not seem to make sense to me, because there should
> not be any valid cache lines for that region at that time. My plan
> was to submit patches to sh and xtensa to no longer flush the
> dcache in _for_cpu with that argument before submitting the patch
> to use the common code. Maybe Paul or Chris can comment on
> whether there is any reason for the dma flush here, if there is
> one, we probably should also flush in dma_unmap_*.
>
> AFAICT, the _for_cpu hooks are only required for swiotlb
> or for an obscure HW implementation of an IOMMU.
? Why we don't need to remove stale cache after DMA_FROM_DEVICE
transfer?
> > > +static inline int
> > > +dma_supported(struct device *dev, u64 mask)
> > > +{
> > > + /*
> > > + * we fall back to GFP_DMA when the mask isn't all 1s,
> > > + * so we can't guarantee allocations that must be
> > > + * within a tighter range than GFP_DMA.
> > > + */
> > > + if (mask < 0x00ffffff)
> > > + return 0;
> >
> > I think that this is pretty architecture specific.
>
> Yes, I wondered about this one. I suspect that the same code got
> copied blindly from x86 into all the other architectures. The
> same code exists in arm, cris, frv and mn10300, while avr32, sh
> and xtensa seem to assume that nobody passes a smaller than
> 24-bit mask in there and always return 1. They clearly could not
> handle that situation either, so the version I chose is the
> conservative approach.
I think that it's better not have this in
dma-mapping-linear.h. Everyone blindly copies it from x86 but they are
not correct.
On Thursday 04 June 2009 07:57:12 FUJITA Tomonori wrote:
> On Mon, 1 Jun 2009 11:11:27 +0100 Arnd Bergmann <[email protected]> wrote:
> > I've deliberately kept my asm-generic tree free from architecture specific
> > code for now, in order to make the merging process much easier.
>
> I understand what you mean however we need to merge something
> useful. I don't say that you need to merge everything together however
> we can't know if this is useful or not unless you show how some
> architectures use it.
ok.
>
> > I do have branches in there that convert x86 and microblaze to use
>
> x86? How can it use asm-generic-linear.h?
Not dma-mapping-linear.h, but a lot of other files in asm-generic. I have
a set of all the common header files in my tree, and x86 can e.g. use
ioctls.h, ipcbuf.h, mman.h, or types.h. For x86, it amounts to 15 files,
microblaze can use almost 50 of the generic files.
> > I made this up in order to deal with the different requirements of
> > the architectures I converted. In SH, it's truly device specific
> > (pci may be coherent, others are never), on cris it returns false
> > and on most others always true.
>
> I know what you are trying here.
>
> However, dma_cache_sync() is an exported function to drivers; which
> should be used only with a buffer returned by dma_alloc_noncoherent()
> (see DMA-API.txt). So using dma_cache_sync() in this way is confusing
> to me.
I think it is technically correct, but there are two plausible ways of
doing it, I chose the one that requires slightly less code.
I call dma_cache_sync() for streaming mappings on dma_map_* and
dma_sync_*_for_device iff the mapping is noncoherent. AFAICT, this
is the same case as dma_alloc_noncoherent, which is expected to give
out a noncoherent mapping.
It is done the same way in the existing code for avr32 and sh. The
alternative (implemented in mn10300, and xtensa) is to define a
new function for flushing a data range before a DMA and call that
unconditionally from dma_cache_sync and for noncoherent mappings
in dma_map* and dma_sync_*_for_device.
> > Architectures like x86 and frv could use this hook to do th
> > global flush_write_buffers() but still return zero so that
> > dma_sync_sg_for_device does not have to iterate all SG
> > elements doing nothing for them.
> >
> > Maybe it should be named __dma_coherent_dev() to make clear
> > that it is a helper internal to dma_mapping.h and not supposed to
> > be used by device drivers.
>
> Might be. However, we have dma_is_consistent() for a different purpose
I tried (ab)using that, but since dma_is_consistent takes a memory range,
dma_sync_sg_for_device would have to use less efficient code:
(1)
static inline void
dma_sync_sg_for_device(struct device *dev, struct scatterlist *sglist,
int nents, enum dma_data_direction direction)
{
struct scatterlist *sg;
int i;
if (!dma_coherent_dev(dev))
for_each_sg(sglist, sg, nents, i)
dma_cache_sync(dev, sg_virt(sg), sg->length, direction);
debug_dma_sync_sg_for_device(dev, sg, nents, direction);
}
(2)
static inline void
dma_sync_sg_for_device(struct device *dev, struct scatterlist *sglist,
int nents, enum dma_data_direction direction)
{
struct scatterlist *sg;
int i;
for_each_sg(sglist, sg, nents, i)
if (!dma_is_consistent(dev, sg_virt(sg)))
dma_cache_sync(dev, sg_virt(sg), sg->length, direction);
debug_dma_sync_sg_for_device(dev, sg, nents, direction);
}
In (1), gcc can completely eliminate the object code if dma_coherent_dev()
returns 1, or do a single branch on a nonconstant return value. In (2), we will
walk the full sg list. In some cases, gcc may be able to optimize this as well,
but not in general.
> > It took me a while to find out what the architectures do in case of
> > dma_sync_single_for_*. The noncoherent implementations all flush
> > and invalidate the dcache in _for_device, which seems reasonable.
> >
> > In arch/sh and arch/xtensa, we also flush and invalidate the
> > dcache in *_for_cpu.
> > This does not seem to make sense to me, because there should
> > not be any valid cache lines for that region at that time. My plan
> > was to submit patches to sh and xtensa to no longer flush the
> > dcache in _for_cpu with that argument before submitting the patch
> > to use the common code. Maybe Paul or Chris can comment on
> > whether there is any reason for the dma flush here, if there is
> > one, we probably should also flush in dma_unmap_*.
> >
> > AFAICT, the _for_cpu hooks are only required for swiotlb
> > or for an obscure HW implementation of an IOMMU.
>
> ? Why we don't need to remove stale cache after DMA_FROM_DEVICE
> transfer?
The way that the existing architectures handle this case is to
invalidate the cache before DMA_FROM_DEVICE transfers.
This only works if the CPU is forbidden from touching the buffer
between dma_map_*/dma_sync_*_for_device and
dma_unmap_*/dma_sync_*_for_cpu, while the device is forbidden
to touch it at any other time. I believe this is what we are expecting
the driver to enforce.
In fact, the documentation of dma_sync_single_for_cpu tells exactly that:
/*
* Make physical memory consistent for a single streaming mode DMA
* translation after a transfer.
*
* If you perform a dma_map_single() but wish to interrogate the
* buffer using the cpu, yet do not wish to teardown the DMA mapping,
* you must call this function before doing so. At the next point you
* give the DMA address back to the card, you must first perform a
* dma_sync_single_for_device, and then the device again owns the
* buffer.
*/
> > > > +static inline int
> > > > +dma_supported(struct device *dev, u64 mask)
> > > > +{
> > > > + /*
> > > > + * we fall back to GFP_DMA when the mask isn't all 1s,
> > > > + * so we can't guarantee allocations that must be
> > > > + * within a tighter range than GFP_DMA.
> > > > + */
> > > > + if (mask < 0x00ffffff)
> > > > + return 0;
> > >
> > > I think that this is pretty architecture specific.
> >
> > Yes, I wondered about this one. I suspect that the same code got
> > copied blindly from x86 into all the other architectures. The
> > same code exists in arm, cris, frv and mn10300, while avr32, sh
> > and xtensa seem to assume that nobody passes a smaller than
> > 24-bit mask in there and always return 1. They clearly could not
> > handle that situation either, so the version I chose is the
> > conservative approach.
>
> I think that it's better not have this in
> dma-mapping-linear.h. Everyone blindly copies it from x86 but they are
> not correct.
If all the architectures are wrong, then I see this as even more reason
to put a correct version into dma-mapping-linear.h.
The existing implementations that check for 0x00ffffff all seem to
assume that dma_supported() returns whether or not
dma_alloc_consistent() will work, falling back to GFP_DMA if necessary.
In this case, we would need to check the maximum address of ZONE_DMA,
ZONE_DMA32 or ZONE_NORMAL (whichever comes first) for correctness:
static inline int
dma_supported(struct device *dev, u64 mask)
{
/*
* dma_supported means that dma_alloc_{non,}coherent
* will succeed in finding an addressable page.
*/
u64 zone_dma_max_pfn;
struct pgdat *pgdat;
struct zone *zone;
for_each_online_pgdat(pgdat) {
/* the first zone on each node is most likely to fit in the mask */
zone = pgdat->node_zones[0];
if (populated_zone(zone) {
max_dma_pfn = zone->zone_start_pfn + zone->spanned_pages);
/* max_dma_pfn is actually constant, we could store it
somewhere instead of looking it up every time. */
if (mask < (max_dma_pfn << PAGE_SHIFT))
return 0;
}
}
return 1;
}
The other interpretation is to tread dma_supported as meaning that dma_map_*
can succeed. Right now, we assume that it always will on the present
architectures I copied from, but that is not necessarily true if mask is smaller
than the highest physical page that can be passed into dma_map_*.
static inline int
dma_supported(struct device *dev, u64 mask)
{
/*
* dma_supported means that dma_map_page and others
* can make any RAM address in the system visible to the device.
*/
if (mask < (max_pfn << PAGE_SHIFT))
return 0;
return 1;
}
Arnd <><
On Thu, Jun 04, 2009 at 04:57:12PM +0900, FUJITA Tomonori wrote:
> ? Why we don't need to remove stale cache after DMA_FROM_DEVICE
> transfer?
Think about a CPU which does speculative prefetches into the cache (which
later ARMs do).
The result is that, for a DMA_FROM_DEVICE transfer, you need to:
1. ensure that no cache writebacks occur to the region while DMA is
being performed
2. ensure that any data which is present in the cache for the region
is invalidated once DMA has completed
If you don't have speculative prefetches, then (1) and (2) can be (and
are at present) combined into the initial mapping setup or whenever the
buffer is handed from the CPUs ownership to the DMA device's ownership.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Thu, Jun 04, 2009 at 12:35:34PM +0000, Arnd Bergmann wrote:
> static inline int
> dma_supported(struct device *dev, u64 mask)
> {
> /*
> * dma_supported means that dma_alloc_{non,}coherent
> * will succeed in finding an addressable page.
> */
> u64 zone_dma_max_pfn;
> struct pgdat *pgdat;
> struct zone *zone;
>
> for_each_online_pgdat(pgdat) {
> /* the first zone on each node is most likely to fit in the mask */
> zone = pgdat->node_zones[0];
> if (populated_zone(zone) {
> max_dma_pfn = zone->zone_start_pfn + zone->spanned_pages);
> /* max_dma_pfn is actually constant, we could store it
> somewhere instead of looking it up every time. */
> if (mask < (max_dma_pfn << PAGE_SHIFT))
And here we go promoting dma-mask-is-not-a-mask-but-a-limit (which I
brought up in the KS thread on linux-arch.)
It's fine if your DMA-able memory starts at physical address 0 and ends
somewhere else, but this is no longer the case with embedded platforms.
As pointed out in the other thread, there are platforms which have two
separate banks of memory, the one at the lower physical address is not
DMA capable.
This means that treating the DMA mask as a limit does not work for these
platforms (and, sometimes, treating it as a real mask also doesn't work.)
A good step forward would be to get a concensus on this stupid DMA mask
thing. Is it a _mask_, or is it a _limit_? If it's a mask then let's
start treating it as such throughout the kernel code. If it's a limit,
let's rename the damned thing so everyone knows that.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Thursday 04 June 2009, Russell King wrote:
> And here we go promoting dma-mask-is-not-a-mask-but-a-limit (which I
> brought up in the KS thread on linux-arch.)
>
> It's fine if your DMA-able memory starts at physical address 0 and ends
> somewhere else, but this is no longer the case with embedded platforms.
> As pointed out in the other thread, there are platforms which have two
> separate banks of memory, the one at the lower physical address is not
> DMA capable.
The dma-mapping-linear.h only cares about the platforms that have a
linear contigous mapping between DMA and phys addresses. Other
platforms will have to do something more fancy in their dma mapping
implementation.
The device sets a mask (really a limit) of the address ranges it can
handle. Basically every user in the kernel currently passes DMA_BIT_MASK()
limit into {dma,pci}_set_mask, and I am not aware of any driver
that needs something more fancy. If you know one, please tell us.
The DMA-capable addresses of the platform are a completely different
issue and these should not be confused. Some architecture solve this
by bus-specific mapping operations, which sounds like what you would
need here as well. The bus is the only instance in the system that knows
how the device-visible DMA space maps into the physical memory space,
and it needs to be the instance checking the mask.
The only place where the mask is really used later is dma_alloc_coherent,
which can also be bus specific, and should know how to interpret the
mask.
Arnd <><
On Thu, Jun 04, 2009 at 02:42:52PM +0100, Arnd Bergmann wrote:
> On Thursday 04 June 2009, Russell King wrote:
> > And here we go promoting dma-mask-is-not-a-mask-but-a-limit (which I
> > brought up in the KS thread on linux-arch.)
> >
> > It's fine if your DMA-able memory starts at physical address 0 and ends
> > somewhere else, but this is no longer the case with embedded platforms.
> > As pointed out in the other thread, there are platforms which have two
> > separate banks of memory, the one at the lower physical address is not
> > DMA capable.
>
> The dma-mapping-linear.h only cares about the platforms that have a
> linear contigous mapping between DMA and phys addresses. Other
> platforms will have to do something more fancy in their dma mapping
> implementation.
I'm not talking about anything fancy here. There's still a 1:1
relationship between DMA and phys addresses.
> The device sets a mask (really a limit) of the address ranges it can
> handle. Basically every user in the kernel currently passes DMA_BIT_MASK()
> limit into {dma,pci}_set_mask, and I am not aware of any driver
> that needs something more fancy. If you know one, please tell us.
Hmm, this opens yet another one of my bugbears. The range of addressible
memory has doesn't really have much to do with drivers, yet in Linux we
_do_ tie it closely to the driver.
The range of addressible memory does have something to do with the device,
and we do carry that information in the driver. However, there's another
element to it which is the hardware path from the device through to the
system memory.
There are cases on ARM (eg) where there's a restriction of 64MB.
There's also the case which I mentioned where we have two banks of memory,
and only the _second_ bank is DMA-capable. Currently, in Linux, the only
way to use such a setup is basically to say "sorry, Linux can't use the
first bank, sorry vendor, Linux sucks."
> The DMA-capable addresses of the platform are a completely different
> issue and these should not be confused. Some architecture solve this
> by bus-specific mapping operations, which sounds like what you would
> need here as well. The bus is the only instance in the system that knows
> how the device-visible DMA space maps into the physical memory space,
> and it needs to be the instance checking the mask.
Yes, and then you end up triggering the OOM killer (see open unresolved
bug in the kernel bugzilla for IXP4xx)...
I hardly call that a solution.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Thu, Jun 04, 2009 at 03:38:03PM +0100, Russell King wrote:
> On Thu, Jun 04, 2009 at 02:42:52PM +0100, Arnd Bergmann wrote:
> > The device sets a mask (really a limit) of the address ranges it can
> > handle. Basically every user in the kernel currently passes DMA_BIT_MASK()
> > limit into {dma,pci}_set_mask, and I am not aware of any driver
> > that needs something more fancy. If you know one, please tell us.
BTW, I don't think you really got my point about DMA mask being a mask
or being a limit.
The following assumption has been made by the kernel:
maximum_physical_address = dma_mask
Yes, that's _physical_ address, not bus specific DMA address:
void blk_queue_bounce_limit(struct request_queue *q, u64 dma_mask)
{
unsigned long b_pfn = dma_mask >> PAGE_SHIFT;
...
q->bounce_pfn = b_pfn;
}
static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
struct bio *bio)
{
...
high = page_to_pfn(bv->bv_page) > q->bounce_pfn;
}
It's not "is this page DMA-able according to the DMA mask" it's effectively
"is this page's physical address greater than the maximum physical address
that can be DMA'd from".
As I've already pointed out, there are ARM platforms where this is just
a total nonsense.
As I say, what is the DMA mask? Is it really a limit? Is it really
supposed to be a mask?
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Thu, 4 Jun 2009 12:35:34 +0000
Arnd Bergmann <[email protected]> wrote:
> > > I do have branches in there that convert x86 and microblaze to use
> >
> > x86? How can it use asm-generic-linear.h?
>
> Not dma-mapping-linear.h, but a lot of other files in asm-generic. I have
> a set of all the common header files in my tree, and x86 can e.g. use
> ioctls.h, ipcbuf.h, mman.h, or types.h. For x86, it amounts to 15 files,
> microblaze can use almost 50 of the generic files.
I see, but I'm not sure why dma-mapping-linear needs to be merged with
them together.
> > > I made this up in order to deal with the different requirements of
> > > the architectures I converted. In SH, it's truly device specific
> > > (pci may be coherent, others are never), on cris it returns false
> > > and on most others always true.
> >
> > I know what you are trying here.
> >
> > However, dma_cache_sync() is an exported function to drivers; which
> > should be used only with a buffer returned by dma_alloc_noncoherent()
> > (see DMA-API.txt). So using dma_cache_sync() in this way is confusing
> > to me.
>
> I think it is technically correct, but there are two plausible ways of
> doing it, I chose the one that requires slightly less code.
>
> I call dma_cache_sync() for streaming mappings on dma_map_* and
> dma_sync_*_for_device iff the mapping is noncoherent. AFAICT, this
> is the same case as dma_alloc_noncoherent, which is expected to give
> out a noncoherent mapping.
If I correctly understand DMA-API.txt, dma_alloc_noncoherent can
return either consistent or non-consistent memory. On architectures
that return consistent memory via dma_alloc_noncoherent,
dma_cache_sync should be null. dma_cache_sync() is supposed to be used
only with the returned buffers of dma_alloc_noncoherent().
> It is done the same way in the existing code for avr32 and sh. The
> alternative (implemented in mn10300, and xtensa) is to define a
> new function for flushing a data range before a DMA and call that
> unconditionally from dma_cache_sync and for noncoherent mappings
> in dma_map* and dma_sync_*_for_device.
>
> > > Architectures like x86 and frv could use this hook to do th
> > > global flush_write_buffers() but still return zero so that
> > > dma_sync_sg_for_device does not have to iterate all SG
> > > elements doing nothing for them.
> > >
> > > Maybe it should be named __dma_coherent_dev() to make clear
> > > that it is a helper internal to dma_mapping.h and not supposed to
> > > be used by device drivers.
> >
> > Might be. However, we have dma_is_consistent() for a different purpose
>
> I tried (ab)using that, but since dma_is_consistent takes a memory range,
> dma_sync_sg_for_device would have to use less efficient code:
>
> (1)
> static inline void
> dma_sync_sg_for_device(struct device *dev, struct scatterlist *sglist,
> int nents, enum dma_data_direction direction)
> {
> struct scatterlist *sg;
> int i;
>
> if (!dma_coherent_dev(dev))
> for_each_sg(sglist, sg, nents, i)
> dma_cache_sync(dev, sg_virt(sg), sg->length, direction);
>
> debug_dma_sync_sg_for_device(dev, sg, nents, direction);
> }
>
> (2)
> static inline void
> dma_sync_sg_for_device(struct device *dev, struct scatterlist *sglist,
> int nents, enum dma_data_direction direction)
> {
> struct scatterlist *sg;
> int i;
>
> for_each_sg(sglist, sg, nents, i)
> if (!dma_is_consistent(dev, sg_virt(sg)))
> dma_cache_sync(dev, sg_virt(sg), sg->length, direction);
I don't think you can do such; dma_is_consistent can be used only with
dma_alloc_noncoherent.
>
> debug_dma_sync_sg_for_device(dev, sg, nents, direction);
> }
>
> In (1), gcc can completely eliminate the object code if dma_coherent_dev()
> returns 1, or do a single branch on a nonconstant return value. In (2), we will
> walk the full sg list. In some cases, gcc may be able to optimize this as well,
> but not in general.
On Thursday 04 June 2009, Russell King wrote:
> The following assumption has been made by the kernel:
>
> maximum_physical_address = dma_mask
>
> Yes, that's _physical_ address, not bus specific DMA address:
Right, this is an oversimplification that I did not expect the kernel
to make, and it seems to stem from the days before we had the dma
mapping API.
> void blk_queue_bounce_limit(struct request_queue *q, u64 dma_mask)
> {
> unsigned long b_pfn = dma_mask >> PAGE_SHIFT;
> ...
> q->bounce_pfn = b_pfn;
> }
>
> static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
> struct bio *bio)
> {
> ...
> high = page_to_pfn(bv->bv_page) > q->bounce_pfn;
> }
>
> It's not "is this page DMA-able according to the DMA mask" it's effectively
> "is this page's physical address greater than the maximum physical address
> that can be DMA'd from".
>
> As I've already pointed out, there are ARM platforms where this is just
> a total nonsense.
Agreed. I still think that a per-device limit combined with the set of
per-bus remapping rules should handle all cases right, but the block
bounce buffer handling seems to prevent that. Looking at
scsi_cacluclate_bounce_limit:
u64 scsi_calculate_bounce_limit(struct Scsi_Host *shost)
{
struct device *host_dev;
u64 bounce_limit = 0xffffffff;
if (shost->unchecked_isa_dma)
return BLK_BOUNCE_ISA;
/*
* Platforms with virtual-DMA translation
* hardware have no practical limit.
*/
if (!PCI_DMA_BUS_IS_PHYS)
return BLK_BOUNCE_ANY;
host_dev = scsi_get_device(shost);
if (host_dev && host_dev->dma_mask)
bounce_limit = *host_dev->dma_mask;
return bounce_limit;
}
This is more or less hardcoding all the common cases
(ISA, PCI with IOMMU, and phys address limit per device),
but not the one you are interested in.
The bounce buffer handling is more or less an older and simpler
way of doing the swiotlb, maybe you can simply unset the
(horribly misnamed) PCI_DMA_BUS_IS_PHYS in order to effectively
disable bounce buffers at the block layer and instead do them
in the dma mapping layer in the form of swiotlb.
Are there similar problems outside of the block layer?
It seems that in networking, device drivers themselves are
always in charge of the allocation, so unlike block drivers,
they can do the right thing on all platforms that have this
problem.
> As I say, what is the DMA mask? Is it really a limit?
I still don't think this is the right question to ask. The way that
the code is structured, it's very clear that it treats a mask and
a limit the same way. Simply assuming that it's a mask that can contain
zeroes gets you into trouble.
> Is it really supposed to be a mask?
Would it solve any problem if it were a mask? All uses essentially
come down to the point where you allocate memory and decide between
GFP_DMA, GFP_DMA32, GFP_NORMAL or GFP_HIGHMEM. If none of them
fit into the mask, you still lose. Also, a bitmask is not enough
to encode a range, as I mentioned before.
Arnd <><
On Thursday 04 June 2009, FUJITA Tomonori wrote:
> On Thu, 4 Jun 2009 12:35:34 +0000
> Arnd Bergmann <[email protected]> wrote:
>
> > > > I do have branches in there that convert x86 and microblaze to use
> > >
> > > x86? How can it use asm-generic-linear.h?
> >
> > Not dma-mapping-linear.h, but a lot of other files in asm-generic. I have
> > a set of all the common header files in my tree, and x86 can e.g. use
> > ioctls.h, ipcbuf.h, mman.h, or types.h. For x86, it amounts to 15 files,
> > microblaze can use almost 50 of the generic files.
>
> I see, but I'm not sure why dma-mapping-linear needs to be merged with
> them together.
It doesn't need to, but it would be much more convenient for me having
to go through the architectures only once. Some of the arch maintainers
are harder to get hold of than others.
> >
> > I think it is technically correct, but there are two plausible ways of
> > doing it, I chose the one that requires slightly less code.
> >
> > I call dma_cache_sync() for streaming mappings on dma_map_* and
> > dma_sync_*_for_device iff the mapping is noncoherent. AFAICT, this
> > is the same case as dma_alloc_noncoherent, which is expected to give
> > out a noncoherent mapping.
>
> If I correctly understand DMA-API.txt, dma_alloc_noncoherent can
> return either consistent or non-consistent memory. On architectures
> that return consistent memory via dma_alloc_noncoherent,
> dma_cache_sync should be null. dma_cache_sync() is supposed to be used
> only with the returned buffers of dma_alloc_noncoherent().
Good point. This is unfortunately not what is implemented on many
architectures, which #define dma_alloc_noncoherent dma_alloc_coherent
but still provide a synchronizing operation in dma_cache_sync().
dma_alloc_noncoherent is actually only implemented on parisc, mips
and m68k.
However, I don't think I have the energy to fix this problem, but
I agree that it should be fixed eventually. I can leave out
the declarations of dma_{free,alloc}_coherent from dma-mapping-linear.h
so that the broken code remains in the architecture specific
files, and change all references to dma_cache_sync to something
else. The best I can think of is __dma_cache_sync() with the same
calling conventions as dma_cache_sync(). Does that make sense?
Arnd <><
On Thu, Jun 4, 2009 at 18:47, Arnd Bergmann <[email protected]> wrote:
> On Thursday 04 June 2009, FUJITA Tomonori wrote:
>> On Thu, 4 Jun 2009 12:35:34 +0000
>> Arnd Bergmann <[email protected]> wrote:
>> If I correctly understand DMA-API.txt, dma_alloc_noncoherent can
>> return either consistent or non-consistent memory. On architectures
>> that return consistent memory via dma_alloc_noncoherent,
>> dma_cache_sync should be null. dma_cache_sync() is supposed to be used
>> only with the returned buffers of dma_alloc_noncoherent().
>
> Good point. This is unfortunately not what is implemented on many
> architectures, which #define dma_alloc_noncoherent dma_alloc_coherent
> but still provide a synchronizing operation in dma_cache_sync().
>
> dma_alloc_noncoherent is actually only implemented on parisc, mips
> and m68k.
And m68k is a false positive, as it just uses a static inline wrapper
instead of a
define to map it to dma_alloc_coherent().
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, 4 Jun 2009 17:47:03 +0100
Arnd Bergmann <[email protected]> wrote:
> On Thursday 04 June 2009, FUJITA Tomonori wrote:
> > On Thu, 4 Jun 2009 12:35:34 +0000
> > Arnd Bergmann <[email protected]> wrote:
> >
> > > > > I do have branches in there that convert x86 and microblaze to use
> > > >
> > > > x86? How can it use asm-generic-linear.h?
> > >
> > > Not dma-mapping-linear.h, but a lot of other files in asm-generic. I have
> > > a set of all the common header files in my tree, and x86 can e.g. use
> > > ioctls.h, ipcbuf.h, mman.h, or types.h. For x86, it amounts to 15 files,
> > > microblaze can use almost 50 of the generic files.
> >
> > I see, but I'm not sure why dma-mapping-linear needs to be merged with
> > them together.
>
> It doesn't need to, but it would be much more convenient for me having
> to go through the architectures only once. Some of the arch maintainers
> are harder to get hold of than others.
>
> > >
> > > I think it is technically correct, but there are two plausible ways of
> > > doing it, I chose the one that requires slightly less code.
> > >
> > > I call dma_cache_sync() for streaming mappings on dma_map_* and
> > > dma_sync_*_for_device iff the mapping is noncoherent. AFAICT, this
> > > is the same case as dma_alloc_noncoherent, which is expected to give
> > > out a noncoherent mapping.
> >
> > If I correctly understand DMA-API.txt, dma_alloc_noncoherent can
> > return either consistent or non-consistent memory. On architectures
> > that return consistent memory via dma_alloc_noncoherent,
> > dma_cache_sync should be null. dma_cache_sync() is supposed to be used
> > only with the returned buffers of dma_alloc_noncoherent().
>
> Good point. This is unfortunately not what is implemented on many
> architectures, which #define dma_alloc_noncoherent dma_alloc_coherent
> but still provide a synchronizing operation in dma_cache_sync().
>
> dma_alloc_noncoherent is actually only implemented on parisc, mips
> and m68k.
>
> However, I don't think I have the energy to fix this problem, but
> I agree that it should be fixed eventually. I can leave out
> the declarations of dma_{free,alloc}_coherent from dma-mapping-linear.h
> so that the broken code remains in the architecture specific
> files, and change all references to dma_cache_sync to something
> else. The best I can think of is __dma_cache_sync() with the same
> calling conventions as dma_cache_sync(). Does that make sense?
Sorry, but it doesn't make sense to me because __dma_cache_sync() hack
is against the goal of dma-mapping-linear.h, having a clean, ideal,
unified header file.
On Monday 08 June 2009, FUJITA Tomonori wrote:
>
> > However, I don't think I have the energy to fix this problem, but
> > I agree that it should be fixed eventually. I can leave out
> > the declarations of dma_{free,alloc}_coherent from dma-mapping-linear.h
> > so that the broken code remains in the architecture specific
> > files, and change all references to dma_cache_sync to something
> > else. The best I can think of is __dma_cache_sync() with the same
> > calling conventions as dma_cache_sync(). Does that make sense?
>
> Sorry, but it doesn't make sense to me because __dma_cache_sync() hack
> is against the goal of dma-mapping-linear.h, having a clean, ideal,
> unified header file.
Do you have any other suggestion? The operation that an architecture
performs to synchronize the DMA buffer after a DMA is just not generic
and needs to have some name that we can call from the generic file.
Right now we use one of dma_cache_sync, frv_cache_wback_inv,
mn10300_dcache_flush_inv or consistent_sync for this and I was just
looking for a new internal name for this operation.
Arnd <><
On Mon, 8 Jun 2009 10:03:58 +0200
Arnd Bergmann <[email protected]> wrote:
> On Monday 08 June 2009, FUJITA Tomonori wrote:
> >
> > > However, I don't think I have the energy to fix this problem, but
> > > I agree that it should be fixed eventually. I can leave out
> > > the declarations of dma_{free,alloc}_coherent from dma-mapping-linear.h
> > > so that the broken code remains in the architecture specific
> > > files, and change all references to dma_cache_sync to something
> > > else. The best I can think of is __dma_cache_sync() with the same
> > > calling conventions as dma_cache_sync(). Does that make sense?
> >
> > Sorry, but it doesn't make sense to me because __dma_cache_sync() hack
> > is against the goal of dma-mapping-linear.h, having a clean, ideal,
> > unified header file.
>
> Do you have any other suggestion? The operation that an architecture
> performs to synchronize the DMA buffer after a DMA is just not generic
> and needs to have some name that we can call from the generic file.
> Right now we use one of dma_cache_sync, frv_cache_wback_inv,
> mn10300_dcache_flush_inv or consistent_sync for this and I was just
> looking for a new internal name for this operation.
If you don't clean up them, I think that it's better to leave out
asm-generic/dma-generic-linear.h for
now. asm-generic/dma-generic-linear.h is supposed to clean up the
mess; unify (generalize, or fix) some architecture. It's not supported
to add another temporary hack that should be removed.
On Monday 08 June 2009, FUJITA Tomonori wrote:
> If you don't clean up them, I think that it's better to leave out
> asm-generic/dma-generic-linear.h for
> now. asm-generic/dma-generic-linear.h is supposed to clean up the
> mess; unify (generalize, or fix) some architecture. It's not supported
> to add another temporary hack that should be removed.
Ok. I have no more ideas on how to do that in a cleaner way
than what I suggested, so I'll stop bothering you with it.
Arnd <><