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

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

[akepner]

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.

Signed-off-by: Arthur Kepner <akepner@sgi.com>
Acked-by:  Jesse Barnes <jesse.barnes@intel.com>

---

 dma-attrs.h   |   55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dma-mapping.h |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+)

diff --git a/include/linux/dma-attrs.h b/include/linux/dma-attrs.h
index e69de29..05b6e91 100644
--- a/include/linux/dma-attrs.h
+++ b/include/linux/dma-attrs.h
@@ -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 */
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 3320307..5a0e924 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -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

Re: [PATCH 0/4 v4] dma: dma_{un}map_{single|sg}_attrs() interface

[Andrew Morton]

On Wed, 12 Mar 2008 21:00:22 -0700 akepner@sgi.com 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_BARRIER in anticipation of its use by ia64/sn.
> 
> ...
>
> diff --git a/include/linux/dma-attrs.h b/include/linux/dma-attrs.h
> index e69de29..05b6e91 100644
> --- a/include/linux/dma-attrs.h
> +++ b/include/linux/dma-attrs.h
> @@ -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;
> +};

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?

> +#define __DMA_ATTRS_INIT() { .flags = 0, }

Does this need to exist?

> +#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().

> +#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?

> +#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.

> +/*
> + * 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;
> +}

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.

> +/*
> + * 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;
> +}

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.

> +#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?

> +#endif /* ARCH_USES_DMA_ATTRS */
> +#endif /* _DMA_ATTR_H */

Re: [PATCH 0/4 v4] dma: dma_{un}map_{single|sg}_attrs() interface

[Randy Dunlap]

On Thu, 13 Mar 2008 01:41:06 -0700 Andrew Morton wrote:

> > +#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.

Sam has been pushing HAVE_* and even added that to
Documentation/kbuild/kconfig-language.txt.

> > +/*
> > + * dma_set_attr - set a specific attribute
> > + * may be called with a null attrs
> > + */

Use kernel-doc notation?

> > +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.
> 
> > +/*
> > + * dma_get_attr - check for a specific attribute
> > + * may be called with a null attrs
> > + */

Ditto.

> > +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

Re: [PATCH 0/4 v4] dma: dma_{un}map_{single|sg}_attrs() interface

[akepner]

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.

> On Thu, 13 Mar 2008 01:41:06 -0700 Andrew Morton wrote:
> 
> > > +#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.
> 
> Sam has been pushing HAVE_* and even added that to
> Documentation/kbuild/kconfig-language.txt.

OK, changed it to CONFIG_HAVE_DMA_ATTRS.

> 
> > > +/*
> > > + * dma_set_attr - set a specific attribute
> > > + * may be called with a null attrs
> > > + */
> 
> Use kernel-doc notation?

Done.

-- 
Arthur

(503) 234 4671

Re: [PATCH 0/4 v4] dma: dma_{un}map_{single|sg}_attrs() interface

[akepner]

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. 

> On Wed, 12 Mar 2008 21:00:22 -0700 akepner@sgi.com wrote:
> 
> > ...
> >
> > diff --git a/include/linux/dma-attrs.h b/include/linux/dma-attrs.h
> > index e69de29..05b6e91 100644
> > --- a/include/linux/dma-attrs.h
> > +++ b/include/linux/dma-attrs.h
> > @@ -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;
> > +};
> 
> 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.

> 
> 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.

> 
> > +#define __DMA_ATTRS_INIT() { .flags = 0, }
> 
> Does this need to exist?

No, it's gone from the next patchset.

> 
> > +#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.

 
> > +#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?
> 

Yes ;-)

> > +#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.
> 

OK, I've changed this to CONFIG_HAVE_DMA_ATTRS, selected in Kconfig.

> > +/*
> > + * 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;
> > +}
> 
> 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().

> 
> ....
> We have a dma_set_attr() stub but no dma_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.

-- 
Arthur