dmapool (was: Re: Linux 2.6.3-rc2)

dmapool (was: Re: Linux 2.6.3-rc2)

[Geert Uytterhoeven]

The dmapool code makes dma_{alloc,free}_coherent() a requirement for all
platforms, breaking platforms that don't have it (e.g. m68k, and from a quick
browse sparc and sparc64 probably, too).

May not be that nice for a release candidate in a stable series...

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: dmapool (was: Re: Linux 2.6.3-rc2)

[Geert Uytterhoeven]

On Tue, 10 Feb 2004, Geert Uytterhoeven wrote:
> The dmapool code makes dma_{alloc,free}_coherent() a requirement for all
> platforms, breaking platforms that don't have it (e.g. m68k, and from a quick
> browse sparc and sparc64 probably, too).
>
> May not be that nice for a release candidate in a stable series...

This patch seems to fix the problem (all offending platforms include
<asm/generic.h> if CONFIG_PCI only):

--- linux-2.6.3-rc2/drivers/base/Makefile	2004-02-10 11:14:46.000000000 +0100
+++ linux-m68k-2.6.3-rc2/drivers/base/Makefile	2004-02-10 15:23:13.000000000 +0100
@@ -2,7 +2,8 @@

 obj-y			:= core.o sys.o interface.o bus.o \
 			   driver.o class.o class_simple.o platform.o \
-			   cpu.o firmware.o init.o map.o dmapool.o
+			   cpu.o firmware.o init.o map.o
 obj-y			+= power/
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
 obj-$(CONFIG_NUMA)	+= node.o
+obj-$(CONFIG_PCI)	+= dmapool.o

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: dmapool (was: Re: Linux 2.6.3-rc2)

[Russell King]

On Tue, Feb 10, 2004 at 03:32:47PM +0100, Geert Uytterhoeven wrote:
> On Tue, 10 Feb 2004, Geert Uytterhoeven wrote:
> > The dmapool code makes dma_{alloc,free}_coherent() a requirement for all
> > platforms, breaking platforms that don't have it (e.g. m68k, and from a quick
> > browse sparc and sparc64 probably, too).
> >
> > May not be that nice for a release candidate in a stable series...
> 
> This patch seems to fix the problem (all offending platforms include
> <asm/generic.h> if CONFIG_PCI only):

Please don't - that breaks ARM.  Part of the whole point of dmapool is
that it provides a generic DMA pool implementation, especially for
non-PCI USB devices.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: dmapool (was: Re: Linux 2.6.3-rc2)

[Christoph Hellwig]

On Tue, Feb 10, 2004 at 03:32:47PM +0100, Geert Uytterhoeven wrote:
> This patch seems to fix the problem (all offending platforms include
> <asm/generic.h> if CONFIG_PCI only):

Umm, no the whole point of the dmapool is that it's not pci-dependent.
Just fix your arch to have proper stub dma_ routines.  There were at
least two headsups during 2.5 and 2.6-test that this will be required.

Re: dmapool (was: Re: Linux 2.6.3-rc2)

[Geert Uytterhoeven]

On Tue, 10 Feb 2004, Russell King wrote:
> On Tue, Feb 10, 2004 at 03:32:47PM +0100, Geert Uytterhoeven wrote:
> > On Tue, 10 Feb 2004, Geert Uytterhoeven wrote:
> > > The dmapool code makes dma_{alloc,free}_coherent() a requirement for all
> > > platforms, breaking platforms that don't have it (e.g. m68k, and from a quick
> > > browse sparc and sparc64 probably, too).
> > >
> > > May not be that nice for a release candidate in a stable series...
> >
> > This patch seems to fix the problem (all offending platforms include
> > <asm/generic.h> if CONFIG_PCI only):
>
> Please don't - that breaks ARM.  Part of the whole point of dmapool is
> that it provides a generic DMA pool implementation, especially for
> non-PCI USB devices.

Fine, so what about this (add your favorite subsystem that _does use_ dmapool)?

--- linux-2.6.3-rc2/drivers/base/Makefile	2004-02-10 11:14:46.000000000 +0100
+++ linux-m68k-2.6.3-rc2/drivers/base/Makefile	2004-02-10 15:23:13.000000000 +0100
@@ -2,7 +2,9 @@

 obj-y			:= core.o sys.o interface.o bus.o \
 			   driver.o class.o class_simple.o platform.o \
-			   cpu.o firmware.o init.o map.o dmapool.o
+			   cpu.o firmware.o init.o map.o
 obj-y			+= power/
 obj-$(CONFIG_FW_LOADER)	+= firmware_class.o
 obj-$(CONFIG_NUMA)	+= node.o
+obj-$(CONFIG_PCI)	+= dmapool.o
+obj-$(CONFIG_USB)	+= dmapool.o

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: dmapool (was: Re: Linux 2.6.3-rc2)

[Greg KH]

On Tue, Feb 10, 2004 at 02:55:58PM +0000, Christoph Hellwig wrote:
> On Tue, Feb 10, 2004 at 03:32:47PM +0100, Geert Uytterhoeven wrote:
> > This patch seems to fix the problem (all offending platforms include
> > <asm/generic.h> if CONFIG_PCI only):
> 
> Umm, no the whole point of the dmapool is that it's not pci-dependent.
> Just fix your arch to have proper stub dma_ routines.  There were at
> least two headsups during 2.5 and 2.6-test that this will be required.

Exactly.  Why is your arch including a header file that you can't build?

How about dropping this into your arch if you can't use the
include/asm-generic/dma-mapping.h file.  Or I can add it as
include/asm-generic/dma-mapping-broken.h and you can repoint your arch
to use it.  Which would be easier for you?

thanks,

greg k-h


-----


/* This is used for archs that do not support dma */

#ifndef _ASM_DMA_MAPPING_H
#define _ASM_DMA_MAPPING_H

static inline int
dma_supported(struct device *dev, u64 mask)
{
	BUG();
}

static inline int
dma_set_mask(struct device *dev, u64 dma_mask)
{
	BUG();
}

static inline void *
dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
		   int flag)
{
	BUG();
}

static inline void
dma_free_coherent(struct device *dev, size_t size, void *cpu_addr,
		    dma_addr_t dma_handle)
{
	BUG();
}

static inline dma_addr_t
dma_map_single(struct device *dev, void *cpu_addr, size_t size,
	       enum dma_data_direction direction)
{
	BUG();
}

static inline void
dma_unmap_single(struct device *dev, dma_addr_t dma_addr, size_t size,
		 enum dma_data_direction direction)
{
	BUG();
}

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)
{
	BUG();
}

static inline void
dma_unmap_page(struct device *dev, dma_addr_t dma_address, size_t size,
	       enum dma_data_direction direction)
{
	BUG();
}

static inline int
dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
	   enum dma_data_direction direction)
{
	BUG();
}

static inline void
dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nhwentries,
	     enum dma_data_direction direction)
{
	BUG();
}

static inline void
dma_sync_single(struct device *dev, dma_addr_t dma_handle, size_t size,
		enum dma_data_direction direction)
{
	BUG();
}

static inline void
dma_sync_sg(struct device *dev, struct scatterlist *sg, int nelems,
	    enum dma_data_direction direction)
{
	BUG();
}

#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)
#define dma_is_consistent(d)	(1)

static inline int
dma_get_cache_alignment(void)
{
	BUG();
}

static inline void
dma_sync_single_range(struct device *dev, dma_addr_t dma_handle,
		      unsigned long offset, size_t size,
		      enum dma_data_direction direction)
{
	BUG();
}

static inline void
dma_cache_sync(void *vaddr, size_t size,
	       enum dma_data_direction direction)
{
	BUG();
}

#endif

Re: dmapool (was: Re: Linux 2.6.3-rc2)

[Geert Uytterhoeven]

On Tue, 10 Feb 2004, Greg KH wrote:
> On Tue, Feb 10, 2004 at 02:55:58PM +0000, Christoph Hellwig wrote:
> > On Tue, Feb 10, 2004 at 03:32:47PM +0100, Geert Uytterhoeven wrote:
> > > This patch seems to fix the problem (all offending platforms include
> > > <asm/generic.h> if CONFIG_PCI only):
> >
> > Umm, no the whole point of the dmapool is that it's not pci-dependent.
> > Just fix your arch to have proper stub dma_ routines.  There were at
> > least two headsups during 2.5 and 2.6-test that this will be required.
>
> Exactly.  Why is your arch including a header file that you can't build?

It's included if CONFIG_PCI is enabled (for the few m68k platforms that do have
PCI). In that case everything should work fine (fingers crossed).

> How about dropping this into your arch if you can't use the
> include/asm-generic/dma-mapping.h file.  Or I can add it as
> include/asm-generic/dma-mapping-broken.h and you can repoint your arch
> to use it.  Which would be easier for you?

Since others may need it as well, include/asm-generic/dma-mapping-broken.h
should be fine.

Let's see what the sparc guys have to comment...

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: dmapool (was: Re: Linux 2.6.3-rc2)

[David S. Miller]

On Tue, 10 Feb 2004 17:29:15 +0100 (MET)
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Let's see what the sparc guys have to comment...

I think we'll just add the necessary stubs, it's not unreasonable.

Re: dmapool (was: Re: Linux 2.6.3-rc2)

[Geert Uytterhoeven]

On Tue, 10 Feb 2004, David S. Miller wrote:
> On Tue, 10 Feb 2004 17:29:15 +0100 (MET)
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> > Let's see what the sparc guys have to comment...
>
> I think we'll just add the necessary stubs, it's not unreasonable.

So here's a patch for m68k, based on Greg's stubs. I only added a few
`return 0;' statements to kill compiler warnings. It increased kernel size by a
bit less than 0.5 KiB.

Feel free to move the stubs to asm-generic/no-dma-mapping.h, if there are
enough users to warrant that.

--- linux-2.6.3-rc2/include/asm-m68k/dma-mapping.h	2003-07-29 18:19:20.000000000 +0200
+++ linux-m68k-2.6.3-rc2/include/asm-m68k/dma-mapping.h	2004-02-12 10:21:29.000000000 +0100
@@ -5,6 +5,123 @@

 #ifdef CONFIG_PCI
 #include <asm-generic/dma-mapping.h>
+#else
+
+static inline int
+dma_supported(struct device *dev, u64 mask)
+{
+	BUG();
+	return 0;
+}
+
+static inline int
+dma_set_mask(struct device *dev, u64 dma_mask)
+{
+	BUG();
+	return 0;
+}
+
+static inline void *
+dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
+		   int flag)
+{
+	BUG();
+	return 0;
+}
+
+static inline void
+dma_free_coherent(struct device *dev, size_t size, void *cpu_addr,
+		    dma_addr_t dma_handle)
+{
+	BUG();
+}
+
+static inline dma_addr_t
+dma_map_single(struct device *dev, void *cpu_addr, size_t size,
+	       enum dma_data_direction direction)
+{
+	BUG();
+	return 0;
+}
+
+static inline void
+dma_unmap_single(struct device *dev, dma_addr_t dma_addr, size_t size,
+		 enum dma_data_direction direction)
+{
+	BUG();
+}
+
+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)
+{
+	BUG();
+	return 0;
+}
+
+static inline void
+dma_unmap_page(struct device *dev, dma_addr_t dma_address, size_t size,
+	       enum dma_data_direction direction)
+{
+	BUG();
+}
+
+static inline int
+dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
+	   enum dma_data_direction direction)
+{
+	BUG();
+	return 0;
+}
+
+static inline void
+dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nhwentries,
+	     enum dma_data_direction direction)
+{
+	BUG();
+}
+
+static inline void
+dma_sync_single(struct device *dev, dma_addr_t dma_handle, size_t size,
+		enum dma_data_direction direction)
+{
+	BUG();
+}
+
+static inline void
+dma_sync_sg(struct device *dev, struct scatterlist *sg, int nelems,
+	    enum dma_data_direction direction)
+{
+	BUG();
+}
+
+#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)
+#define dma_is_consistent(d)	(1)
+
+static inline int
+dma_get_cache_alignment(void)
+{
+	BUG();
+	return 0;
+}
+
+static inline void
+dma_sync_single_range(struct device *dev, dma_addr_t dma_handle,
+		      unsigned long offset, size_t size,
+		      enum dma_data_direction direction)
+{
+	BUG();
+}
+
+static inline void
+dma_cache_sync(void *vaddr, size_t size,
+	       enum dma_data_direction direction)
+{
+	BUG();
+}
+
 #endif

 #endif  /* _M68K_DMA_MAPPING_H */

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: dmapool (was: Re: Linux 2.6.3-rc2)

[Gerd Knorr]

Geert Uytterhoeven <geert@linux-m68k.org> writes:

> Feel free to move the stubs to asm-generic/no-dma-mapping.h, if there are
> enough users to warrant that.

Yes, please.  user-mode-linux needs this too.

  Gerd

-- 
"... und auch das ganze Wochenende oll" -- Wetterbericht auf RadioEins

Re: dmapool (was: Re: Linux 2.6.3-rc2)

[Geert Uytterhoeven]

On Thu, 12 Feb 2004, Gerd Knorr wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> > Feel free to move the stubs to asm-generic/no-dma-mapping.h, if there are
> > enough users to warrant that.
>
> Yes, please.  user-mode-linux needs this too.

So here's a new patch:

--- /dev/null	2004-02-08 10:50:36.000000000 +0100
+++ linux-m68k-2.6.3-rc2/include/asm-generic/dma-mapping-broken.h	2004-02-13 16:17:05.000000000 +0100
@@ -0,0 +1,121 @@
+#ifndef _ASM_GENERIC_DMA_MAPPING_H
+#define _ASM_GENERIC_DMA_MAPPING_H
+
+/* This is used for archs that do not support DMA */
+
+
+static inline int
+dma_supported(struct device *dev, u64 mask)
+{
+	BUG();
+	return 0;
+}
+
+static inline int
+dma_set_mask(struct device *dev, u64 dma_mask)
+{
+	BUG();
+	return 0;
+}
+
+static inline void *
+dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
+		   int flag)
+{
+	BUG();
+	return 0;
+}
+
+static inline void
+dma_free_coherent(struct device *dev, size_t size, void *cpu_addr,
+		    dma_addr_t dma_handle)
+{
+	BUG();
+}
+
+static inline dma_addr_t
+dma_map_single(struct device *dev, void *cpu_addr, size_t size,
+	       enum dma_data_direction direction)
+{
+	BUG();
+	return 0;
+}
+
+static inline void
+dma_unmap_single(struct device *dev, dma_addr_t dma_addr, size_t size,
+		 enum dma_data_direction direction)
+{
+	BUG();
+}
+
+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)
+{
+	BUG();
+	return 0;
+}
+
+static inline void
+dma_unmap_page(struct device *dev, dma_addr_t dma_address, size_t size,
+	       enum dma_data_direction direction)
+{
+	BUG();
+}
+
+static inline int
+dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
+	   enum dma_data_direction direction)
+{
+	BUG();
+	return 0;
+}
+
+static inline void
+dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nhwentries,
+	     enum dma_data_direction direction)
+{
+	BUG();
+}
+
+static inline void
+dma_sync_single(struct device *dev, dma_addr_t dma_handle, size_t size,
+		enum dma_data_direction direction)
+{
+	BUG();
+}
+
+static inline void
+dma_sync_sg(struct device *dev, struct scatterlist *sg, int nelems,
+	    enum dma_data_direction direction)
+{
+	BUG();
+}
+
+#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)
+#define dma_is_consistent(d)			(1)
+
+static inline int
+dma_get_cache_alignment(void)
+{
+	BUG();
+	return 0;
+}
+
+static inline void
+dma_sync_single_range(struct device *dev, dma_addr_t dma_handle,
+		      unsigned long offset, size_t size,
+		      enum dma_data_direction direction)
+{
+	BUG();
+}
+
+static inline void
+dma_cache_sync(void *vaddr, size_t size, enum dma_data_direction direction)
+{
+	BUG();
+}
+
+#endif /* _ASM_GENERIC_DMA_MAPPING_H */
--- linux-2.6.3-rc2/include/asm-m68k/dma-mapping.h	2003-07-29 18:19:20.000000000 +0200
+++ linux-m68k-2.6.3-rc2/include/asm-m68k/dma-mapping.h	2004-02-13 16:14:30.000000000 +0100
@@ -5,6 +5,8 @@

 #ifdef CONFIG_PCI
 #include <asm-generic/dma-mapping.h>
+#else
+#include <asm-generic/dma-mapping-broken.h>
 #endif

 #endif  /* _M68K_DMA_MAPPING_H */

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: dmapool (was: Re: Linux 2.6.3-rc2)

[David Brownell]

> +static inline int
> +dma_supported(struct device *dev, u64 mask)
> +{
> +	BUG();
> +	return 0;
> +}

dma_supported() in particular shouldn't BUG(); this should work:

    if (use_dma && dma_supported(dev, ~(u32)0)) {
        ... set up for DMA ...
    } else {
        ... just use PIO instead ...
    }

Likewise the other routines that have clearly defined fault paths
should probably just use those instead of BUG().  Examples of this
type would be dma_alloc_coherent(), dma_map_sg() and maybe also
dma_get_cache_alignment().


Of course the API still lacks clean fault reporting for the single
shot mapping calls -- like maybe an arch-settable

   #define DMA_ADDR_INVALID (~(dma_addr_t)0)

return value -- but that'd be a different patch.

- Dave

Re: dmapool (was: Re: Linux 2.6.3-rc2)

[Roman Zippel]

Hi,

On Tue, 10 Feb 2004, Greg KH wrote:

> How about dropping this into your arch if you can't use the
> include/asm-generic/dma-mapping.h file.  Or I can add it as
> include/asm-generic/dma-mapping-broken.h and you can repoint your arch
> to use it.  Which would be easier for you?

I don't think adding a dma-mapping-broken.h is correct either.
Some m68k scsi drivers are still badly broken, because they use some
archaic API and need an update to something sane.
I think the problem is that DMA-API.txt suggest there is separate API for
coherent/noncoherent (especially dma_alloc_coherent()/
dma_alloc_noncoherent()). DMA-mapping.txt is better here as it
distinguishes between consistent and streaming mappings.
m68k needs something similiar as arm, consistent allocations are mapped
noncachable into the kernel and dma_map* does the necessary cache flushes.
With that you would already get very far and would do the right thing for
most noncoherent archs. This would only would only leave us with some
special cases which had to be fixed to make a driver coherency aware.
So I would suggest to drop e.g. dma_alloc_noncoherent() (parisc seems to
be the only user and it's simply a kmalloc) and merge the rest of
DMA-API.txt with DMA-mapping.txt or make at least consistent with it and
maybe add a bit more information what the functions do in the coherent/
noncoherent case.

bye, Roman