[RFC/PATCH] dma: dma_{un}map_{single|sg}_attrs() interface

Previous thread: WARNING: at kernel/mutex.c:134 by Dave Young on Monday, January 21, 2008 - 10:36 pm. (6 messages)

Next thread: ext4 merge plans for 2.6.25 by Theodore Ts'o on Monday, January 21, 2008 - 11:01 pm. (8 messages)
To: Tony Luck <tony.luck@...>, Grant Grundler <grundler@...>, Jesse Barnes <jbarnes@...>, Jes Sorensen <jes@...>, Randy Dunlap <randy.dunlap@...>, Roland Dreier <rdreier@...>, James Bottomley <James.Bottomley@...>, David Miller <davem@...>, Muli Ben-Yehuda <muli@...>
Cc: <linux-kernel@...>
Date: Monday, January 21, 2008 - 10:42 pm

Here's a new interface for passing attributes to the dma mapping
and unmapping routines. (I have patches that make use of the
interface as well, but let's discuss this piece first.)

For ia64, new machvec entries replace the dma map/unmap interface,
and the old interface is implemented in terms of the new. (All
implementations other than ia64/sn2 ignore the new attributes.)

For architectures other than ia64, the new interface is implemented
in terms of the old (attributes are always ignored).

Tested on hpzx1 and ia64/sn2 (IA64_GENERIC kernels) and on x86_64.

Signed-off-by: Arthur Kepner <akepner@sgi.com>

--

arch/ia64/hp/common/hwsw_iommu.c | 60 +++++++++++++-------------
arch/ia64/hp/common/sba_iommu.c | 62 +++++++++++++++------------
arch/ia64/sn/pci/pci_dma.c | 71 +++++++++++++++++++------------
include/asm-ia64/dma-mapping.h | 28 ++++++++++--
include/asm-ia64/machvec.h | 52 +++++++++++++---------
include/asm-ia64/machvec_hpzx1.h | 16 +++---
include/asm-ia64/machvec_hpzx1_swiotlb.h | 16 +++---
include/asm-ia64/machvec_sn2.h | 16 +++---
include/linux/dma-attrs.h | 33 ++++++++++++++
include/linux/dma-mapping.h | 33 ++++++++++++++
lib/swiotlb.c | 50 ++++++++++++++++++---
11 files changed, 301 insertions(+), 136 deletions(-)

diff --git a/arch/ia64/hp/common/hwsw_iommu.c b/arch/ia64/hp/common/hwsw_iommu.c
index 94e5710..8cedd6c 100644
--- a/arch/ia64/hp/common/hwsw_iommu.c
+++ b/arch/ia64/hp/common/hwsw_iommu.c
@@ -20,10 +20,10 @@
extern int swiotlb_late_init_with_default_size (size_t size);
extern ia64_mv_dma_alloc_coherent swiotlb_alloc_coherent;
extern ia64_mv_dma_free_coherent swiotlb_free_coherent;
-extern ia64_mv_dma_map_single swiotlb_map_single;
-extern ia64_mv_dma_unmap_single swiotlb_unmap_single;
-extern ia64_mv_dma_map_sg swiotlb_map_sg;
-extern ia64_mv_dma_unmap_sg swiotlb_un...

To: <akepner@...>
Cc: Tony Luck <tony.luck@...>, Grant Grundler <grundler@...>, Jesse Barnes <jbarnes@...>, Jes Sorensen <jes@...>, Randy Dunlap <randy.dunlap@...>, James Bottomley <James.Bottomley@...>, David Miller <davem@...>, Muli Ben-Yehuda <muli@...>, <linux-kernel@...>
Date: Wednesday, January 23, 2008 - 12:59 am

> --- a/include/linux/dma-attrs.h
> +++ b/include/linux/dma-attrs.h
> @@ -0,0 +1,33 @@
> +#ifndef _DMA_ATTR_H
> +#define _DMA_ATTR_H
> +
> +#include <linux/bug.h>
> +
> +enum {
> + DMA_ATTR_INVALID,
> + DMA_ATTR_BARRIER,
> + DMA_ATTR_FOO,
> + DMA_ATTR_GOO,
> + DMA_ATTR_MAX,
> +};
> +
> +struct dma_attrs {
> + unsigned flags;
> +};
> +
> +static inline int dma_set_attr(struct dma_attrs *attrs, unsigned attr) {

maybe this would be cleaner if you named the DMA_ATTR enum and used
that instead of unsigned here (and below)?

> + BUG_ON(attrs == NULL);

does this BUG_ON() buy us much? It seems the only thing we would fail
to oops on is if someone did dma_set_attr(NULL, INVALID) and I'm not
sure it's worth it to BUG here.

> + if (attr > DMA_ATTR_INVALID && attr < DMA_ATTR_MAX) {
> + attrs->flags = (1 << attr);
> + return 0;
> + }
> + return 1;

returning -EINVAL here instead of 1 would probably be more "kernelish".

> +}
> +
> +static inline int dma_get_attr(struct dma_attrs *attrs, unsigned attr) {
> + if (attrs)
> + return attrs->flags & (1 << attr);

so it's OK to pass attrs == NULL into dma_get_attr() but not into
dma_set_attr()? seems kind of odd.

> + return 0;
> +}

It seems you're missing a way to initialize a struct dma_attrs. How
do I clear the flags field to start with?

A macro like DEFINE_DMA_ATTRS() that initializes things for you (like
LIST_HEAD or DEFINE_SPIN_LOCK) would probably be a good thing to have
as well.

Also I guess you could test ARCH_USES_DMA_ATTRS in this file and stub
everything out and define an empty structure if it's not defined.
save a few bytes of stack etc.

> +
> +#endif /* _DMA_ATTR_H */
--

To: Roland Dreier <rdreier@...>
Cc: Tony Luck <tony.luck@...>, Grant Grundler <grundler@...>, Jesse Barnes <jbarnes@...>, Jes Sorensen <jes@...>, Randy Dunlap <randy.dunlap@...>, James Bottomley <James.Bottomley@...>, David Miller <davem@...>, Muli Ben-Yehuda <muli@...>, <linux-kernel@...>
Date: Thursday, January 24, 2008 - 7:36 pm

Thanks for the review.

An updated version of the patch follows.

Yeah, I fixed that. Passing a NULL struct dma_attrs * is now

Yep. (Mainly I was concerned with getting the "plumbing" right for
introducing dma attributes, so I implemented as little as possible
of the dma attributes interface itself. It's a bit more fleshed out

Good idea. In fact, the forward declaration of struct dma_attrs
is sufficient when ARCH_USES_DMA_ATTRS isn't defined, since the
structure isn't used in that case anyway.

Updated patch below.

Signed-off-by: Arthur Kepner <akepner@sgi.com>

--

arch/ia64/hp/common/hwsw_iommu.c | 60 ++++++++++++------------
arch/ia64/hp/common/sba_iommu.c | 62 ++++++++++++++----------
arch/ia64/sn/pci/pci_dma.c | 77 ++++++++++++++++++++-----------
include/asm-ia64/dma-mapping.h | 28 +++++++++--
include/asm-ia64/machvec.h | 52 ++++++++++++--------
include/asm-ia64/machvec_hpzx1.h | 16 +++---
include/asm-ia64/machvec_hpzx1_swiotlb.h | 16 +++---
include/asm-ia64/machvec_sn2.h | 16 +++---
include/linux/dma-attrs.h | 48 +++++++++++++++++++
include/linux/dma-mapping.h | 33 +++++++++++++
lib/swiotlb.c | 50 ++++++++++++++++----
11 files changed, 322 insertions(+), 136 deletions(-)

diff --git a/arch/ia64/hp/common/hwsw_iommu.c b/arch/ia64/hp/common/hwsw_iommu.c
index 94e5710..8cedd6c 100644
--- a/arch/ia64/hp/common/hwsw_iommu.c
+++ b/arch/ia64/hp/common/hwsw_iommu.c
@@ -20,10 +20,10 @@
extern int swiotlb_late_init_with_default_size (size_t size);
extern ia64_mv_dma_alloc_coherent swiotlb_alloc_coherent;
extern ia64_mv_dma_free_coherent swiotlb_free_coherent;
-extern ia64_mv_dma_map_single swiotlb_map_single;
-extern ia64_mv_dma_unmap_single swiotlb_unmap_single;
-extern ia64_mv_dma_map_sg swiotlb_map_sg;
-extern ia64_mv_dma_unmap_sg swiotlb_unmap_sg;
+extern ia64_mv_dma_map_sing...

Previous thread: WARNING: at kernel/mutex.c:134 by Dave Young on Monday, January 21, 2008 - 10:36 pm. (6 messages)

Next thread: ext4 merge plans for 2.6.25 by Theodore Ts'o on Monday, January 21, 2008 - 11:01 pm. (8 messages)