dma: dma_{un}map_{single|sg}_attrs() interface
v4 cover dma: dma_{un}map_{single|sg}_attrs() interface
v3-v4 changes:
slight reorganization: split off one arch-independent patch, and moved both new include/linux/*.h files to the first patch in the series.
changed the name DMA_ATTR_SYNC_ON_WRITE to DMA_ATTR_BARRIER.
describe the semantics of DMA_ATTR_BARRIER in a very general, architecture-neutral way.
---
Introduce a new interface for passing architecture-specific attributes when memory is mapped and unmapped for DMA. Give the interface a default implementation which ignores attributes. Also introduce the dma_{set|get}_attr() interface for setting and retrieving individual attributes. Define one attribute DMA_ATTR_BARRIER in anticipation of its use by ia64/sn.
---
dma-attrs.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ dma-mapping.h | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+)
include/linux/dma-attrs.h +55 -0
@@ -0,0 +1,55 @@ +#ifndef _DMA_ATTR_H +#define _DMA_ATTR_H + +enum dma_attr { +DMA_ATTR_BARRIER , +DMA_ATTR_MAX , +}; + +struct dma_attrs { +unsigned flags; +}; + +#define __DMA_ATTRS_INIT () { .flags = 0, } + +#define DECLARE_DMA_ATTRS (x) struct dma_attrs x = __DMA_ATTRS_INIT() + +#define INIT_DMA_ATTRS (x) (x)->flags = 0; + +#ifdef ARCH_USES_DMA_ATTRS +/* +* dma_set_attr - set a specific attribute +* may be called with a null attrs +*/ +static inline int dma_set_attr (struct dma_attrs *attrs,enum dma_attr attr) +{ +if (!attrs) +return 0 ; +if (attr <DMA_ATTR_MAX ) { + attrs->flags |= (1 << attr); +return 0 ; + } +return -EINVAL ; +} + +/* +* dma_get_attr - check for a specific attribute +* may be called with a null attrs +*/ +static inline int dma_get_attr (struct dma_attrs *attrs,enum dma_attr attr) +{ +if (!attrs) +return 0 ; +if (attr <DMA_ATTR_MAX ) { +int ret = attrs->flags & (1 << attr); +return !!ret; + } +return -EINVAL ; +} +#else /* !ARCH_USES_DMA_ATTRS */ +static inline int dma_set_attr (struct dma_attrs *attrs,enum dma_attr attr) +{ +return 0 ; +} +#endif /* ARCH_USES_DMA_ATTRS */ +#endif /* _DMA_ATTR_H */
include/linux/dma-mapping.h +35 -0
@@ -146,4 +146,39 @@ static inline void dmam_release_declared_memory(struct device *dev) }#endif /* ARCH_HAS_DMA_DECLARE_COHERENT_MEMORY */ +#ifndef ARCH_USES_DMA_ATTRS +struct dma_attrs ; + +static inline dma_addr_t dma_map_single_attrs (struct device *dev, +void *cpu_addr,size_t size, +enum dma_data_direction dir, +struct dma_attrs *attrs) +{ +return dma_map_single (dev, cpu_addr, size, dir); +} + +static inline void dma_unmap_single_attrs (struct device *dev, +dma_addr_t dma_addr,size_t size, +enum dma_data_direction dir, +struct dma_attrs *attrs) +{ +return dma_unmap_single (dev, dma_addr, size, dir); +} + +static inline int dma_map_sg_attrs (struct device *dev,struct scatterlist *sgl, +int nents,enum dma_data_direction dir, +struct dma_attrs *attrs) +{ +return dma_map_sg (dev, sgl, nents, dir); +} + +static inline void dma_unmap_sg_attrs (struct device *dev, +struct scatterlist *sgl,int nents, +enum dma_data_direction dir, +struct dma_attrs *attrs) +{ +return dma_unmap_sg (dev, sgl, nents, dir); +} +#endif /* ARCH_USES_DMA_ATTRS */ +#endif
Quoted 16 lines · On Wed, 12 Mar 2008 21:00:22 -0700 [email protected] wrote:
Introduce a new interface for passing architecture-specific attributes when memory is mapped and unmapped for DMA. Give the interface a default implementation which ignores attributes. Also introduce the dma_{set|get}_attr() interface for setting and retrieving individual attributes. Define one attribute
DMA_ATTR_BARRIERin anticipation of its use by ia64/sn....
include/linux/dma-attrs.h +1
-0@@ -0,0 +1,55 @@ +#ifndef _DMA_ATTR_H
include/linux/dma-attrs.h line 11
Quoted 10 lines
include/linux/dma-attrs.h +10
-0+#define _DMA_ATTR_H + +enum dma_attr { +DMA_ATTR_BARRIER , +DMA_ATTR_MAX , +}; + +struct dma_attrs { +unsigned flags; +};
Please comment the above. Describe the semantic meaning of the enums and how they are mapped into `struct dma_attrs' (which is what I assume they do). I see there's a documentation update here so if you think it's best to direct the code reader to that file then fine, add a suitable reference.
Why did we need to implement a struct for a plain old flags word?
include/linux/dma-attrs.h line 13
Quoted 1 line
include/linux/dma-attrs.h +1
-0+#define __DMA_ATTRS_INIT () { .flags = 0, }
Does this need to exist?
include/linux/dma-attrs.h line 15
Quoted 1 line
include/linux/dma-attrs.h +1
-0+#define DECLARE_DMA_ATTRS (x) struct dma_attrs x = __DMA_ATTRS_INIT()
This is a definition, not a declaration. Please rename it to DEFINE_DMA_ATTRS().
include/linux/dma-attrs.h line 17
Quoted 1 line
include/linux/dma-attrs.h +1
-0+#define INIT_DMA_ATTRS (x) (x)->flags = 0;
So if I do
if (expr) INIT_DMA_ATTRS(bar); else something_else();
it won't compile.
Golden rule: implement not in cpp that which can be implemented in C.
static inline void init_dma_attrs(struct dma_attrs *attrs)
{
attrs->flags = 0;
}is much nicer, no?
include/linux/dma-attrs.h line 19
Quoted 1 line
include/linux/dma-attrs.h +1
-0+#ifdef ARCH_USES_DMA_ATTRS
There is no precedent for ARCH_USES_*.
There is a little bit of precedent for ARCH_HAVE_*
There is lots of precendence for ARCH_HAS_*.
We don't like ARCH_HAS_* anyway ;) What can we do to get rid of this? Ideally, make it available on all architectures at zero cost to those which don't need it. If that is impractical (why?) then it is preferable to do this in Kconfig.
Quoted 4 lines
include/linux/dma-attrs.h +4
-0+/* +* dma_set_attr - set a specific attribute +* may be called with a null attrs +*/
include/linux/dma-attrs.h line 33
Quoted 10 lines
include/linux/dma-attrs.h +10
-0+static inline int dma_set_attr (struct dma_attrs *attrs,enum dma_attr attr) +{ +if (!attrs) +return 0 ; +if (attr <DMA_ATTR_MAX ) { + attrs->flags |= (1 << attr); +return 0 ; + } +return -EINVAL ; +}
Is there any non-buggy reason why code would pass an out-of-range attribute into this function? If not, BUG_ON() would be appropriate treatment.
This function might already be too large to inline, and a BUG_ON() might make it larger.
Quoted 4 lines
include/linux/dma-attrs.h +4
-0+/* +* dma_get_attr - check for a specific attribute +* may be called with a null attrs +*/
include/linux/dma-attrs.h line 48
Quoted 10 lines
include/linux/dma-attrs.h +10
-0+static inline int dma_get_attr (struct dma_attrs *attrs,enum dma_attr attr) +{ +if (!attrs) +return 0 ; +if (attr <DMA_ATTR_MAX ) { +int ret = attrs->flags & (1 << attr); +return !!ret; + } +return -EINVAL ; +}
The two callers to this function just do a WARN_ON_ONCE() if it failed. It looks like adding a BUG_ON() and removing those WARN_ONs is the way to go.
include/linux/dma-attrs.h line 53
Quoted 5 lines
include/linux/dma-attrs.h +5
-0+#else /* !ARCH_USES_DMA_ATTRS */ +static inline int dma_set_attr (struct dma_attrs *attrs,enum dma_attr attr) +{ +return 0 ; +}
We have a dma_set_attr() stub but no dma_get_attr() stub?
Quoted 2 lines
include/linux/dma-attrs.h +2
-0+#endif /* ARCH_USES_DMA_ATTRS */ +#endif /* _DMA_ATTR_H */
Quoted 4 lines · On Thu, 13 Mar 2008 01:41:06 -0700 Andrew Morton wrote:
Quoted 1 line
+#ifdef
ARCH_USES_DMA_ATTRSThere is no precedent for ARCH_USES_*.
Quoted 8 lines
There is a little bit of precedent for ARCH_HAVE_*
There is lots of precendence for ARCH_HAS_*.
We don't like ARCH_HAS_* anyway ;) What can we do to get rid of this? Ideally, make it available on all architectures at zero cost to those which don't need it. If that is impractical (why?) then it is preferable to do this in
Kconfig.
Sam has been pushing HAVE_* and even added that to Documentation/kbuild/kconfig-language.txt.
Quoted 4 lines
Quoted 4 lines
+/*
+ *dma_set_attr- set a specific attribute
+ * may be called with a null attrs
+ */
Use kernel-doc notation?
Quoted 14 lines
include/linux/dma-attrs.h line 33
Quoted 10 lines
include/linux/dma-attrs.h +10
-0+static inline int dma_set_attr (struct dma_attrs *attrs,enum dma_attr attr) +{ +if (!attrs) +return 0 ; +if (attr <DMA_ATTR_MAX ) { + attrs->flags |= (1 << attr); +return 0 ; + } +return -EINVAL ; +}Is there any non-buggy reason why code would pass an out-of-range attribute into this function? If not,
BUG_ON()would be appropriate treatment.
Quoted 7 lines
This function might already be too large to inline, and a
BUG_ON()might make it larger.Quoted 4 lines
+/*
+ *dma_get_attr- check for a specific attribute
+ * may be called with a null attrs
+ */
Ditto.
Quoted 10 lines
include/linux/dma-attrs.h line 48
Quoted 10 lines
include/linux/dma-attrs.h +10
-0+static inline int dma_get_attr (struct dma_attrs *attrs,enum dma_attr attr) +{ +if (!attrs) +return 0 ; +if (attr <DMA_ATTR_MAX ) { +int ret = attrs->flags & (1 << attr); +return !!ret; + } +return -EINVAL ; +}
---
~Randy
On Thu, Mar 13, 2008 at 08:27:54AM -0700, Randy Dunlap wrote:
Thanks for looking at this. A new patchset is on the way.
Quoted 10 lines · On Thu, 13 Mar 2008 01:41:06 -0700 Andrew Morton wrote:
Quoted 9 lines
Quoted 2 lines
+#ifdef
ARCH_USES_DMA_ATTRSThere is no precedent for ARCH_USES_*.
There is a little bit of precedent for ARCH_HAVE_*
There is lots of precendence for ARCH_HAS_*.
Quoted 7 lines
Quoted 4 lines
We don't like ARCH_HAS_* anyway ;) What can we do to get rid of this? Ideally, make it available on all architectures at zero cost to those which don't need it. If that is impractical (why?) then it is preferable to do this in
Kconfig.Sam has been pushing HAVE_* and even added that to
Documentation/kbuild/kconfig-language.txt.
OK, changed it to CONFIG_HAVE_DMA_ATTRS.
Quoted 7 lines
Quoted 5 lines
Quoted 5 lines
+/*
+ *dma_set_attr- set a specific attribute
+ * may be called with a null attrs
+ */Use kernel-doc notation?
Done.
Signature · 3 lines
Arthur (503) 234 4671
On Thu, Mar 13, 2008 at 01:41:06AM -0700, Andrew Morton wrote:
Thanks for the review.
I have a new patchset which will address most of your comments.
Quoted 17 lines · On Wed, 12 Mar 2008 21:00:22 -0700 [email protected] wrote:
Quoted 16 lines
...
include/linux/dma-attrs.h +8
-0@@ -0,0 +1,55 @@ +#ifndef _DMA_ATTR_H +#define _DMA_ATTR_H + +enum dma_attr { +DMA_ATTR_BARRIER , +DMA_ATTR_MAX , +}; +
Quoted 8 lines
include/linux/dma-attrs.h line 11
Quoted 3 lines
include/linux/dma-attrs.h +3
-0+struct dma_attrs { +unsigned flags; +};Please comment the above. Describe the semantic meaning of the enums and how they are mapped into `struct
dma_attrs' (which is what I assume they do). I see there's a documentation update here so if you think it's best to direct the code reader to that file then fine, add a suitable reference.
OK, I've added a reference to the Documentation/DMA-attributes.txt file.
Quoted 2 lines
Why did we need to implement a struct for a plain old flags word?
To make the type that carries the attributes opaque, so that it could be expanded. It's a bitmap in the latest version.
Quoted 4 lines
include/linux/dma-attrs.h line 13
Quoted 2 lines
include/linux/dma-attrs.h +1
-0+#define __DMA_ATTRS_INIT () { .flags = 0, }Does this need to exist?
No, it's gone from the next patchset.
Quoted 6 lines
include/linux/dma-attrs.h line 15
Quoted 2 lines
include/linux/dma-attrs.h +1
-0+#define DECLARE_DMA_ATTRS (x) struct dma_attrs x = __DMA_ATTRS_INIT()This is a definition, not a declaration. Please rename it to
DEFINE_DMA_ATTRS().
You're right. Done.
Quoted 11 lines
include/linux/dma-attrs.h line 17
Quoted 1 line
include/linux/dma-attrs.h +1
-0+#define INIT_DMA_ATTRS (x) (x)->flags = 0;So if I do
if (expr) INIT_DMA_ATTRS(bar); else something_else();it won't compile.
Quoted 9 lines
Golden rule: implement not in cpp that which can be implemented in C.
static inline void
init_dma_attrs(struct dma_attrs *attrs){ attrs->flags = 0; }is much nicer, no?
Yes ;-)
Quoted 4 lines
include/linux/dma-attrs.h line 19
Quoted 1 line
include/linux/dma-attrs.h +1
-0+#ifdef ARCH_USES_DMA_ATTRS There is no precedent for ARCH_USES_*.
Quoted 9 lines
There is a little bit of precedent for ARCH_HAVE_*
There is lots of precendence for ARCH_HAS_*.
We don't like ARCH_HAS_* anyway ;) What can we do to get rid of this? Ideally, make it available on all architectures at zero cost to those which don't need it. If that is impractical (why?) then it is preferable to do this in
Kconfig.
OK, I've changed this to CONFIG_HAVE_DMA_ATTRS, selected in Kconfig.
Quoted 9 lines
Quoted 9 lines
include/linux/dma-attrs.h +9
-0+/* +* dma_set_attr - set a specific attribute +* may be called with a null attrs +*/ +static inline int dma_set_attr (struct dma_attrs *attrs,enum dma_attr attr) +{ +if (!attrs) +return 0 ; +if (attr <DMA_ATTR_MAX ) {
Quoted 8 lines
include/linux/dma-attrs.h line 33
Quoted 5 lines
include/linux/dma-attrs.h +5
-0+ attrs->flags |= (1 << attr); +return 0 ; + } +return -EINVAL ; +}Is there any non-buggy reason why code would pass an out-of-range attribute into this function? If not,
BUG_ON()would be appropriate treatment.
Latest version uses BUG_ON().
Quoted 4 lines
....
We have adma_set_attr()stub but nodma_get_attr()stub?
I put a stub in, but dma_get_attr() is only used by arch-specific code, and only code that has CONFIG_HAVE_DMA_ATTRS set. dma_set_attr() is used by arch-independent code, so it has to be there whether CONFIG_HAVE_DMA_ATTRS is set or no.
Signature · 1 line
Arthur