[PATCH 1/3] dma: add dma_flags_set/get_*() interfaces

[PATCH 1/3] dma: add dma_flags_set/get_*() interfaces

[akepner]

Introduce the dma_flags_set/get_*() interfaces and give them 
default implementations. 

Architectures which allow DMA to be reordered between a device and 
host memory (within a NUMA interconnect) can redefine these to allow 
a driver to explicitly synchronize DMA from the device when necessary.

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

--- 

Andrew, this is the first in a series of three patches:

  [1/3] dma: add dma_flags_set/get_*() interfaces
  [2/3] dma: redefine dma_flags_set/get_*() for sn-ia64
  [3/3] dma: document dma_flags_set/get_*()

Variants of these patches have been discussed on several 
occasions, most recently in a thread beginning:

http://marc.info/?l=linux-kernel&m=119137949604365&w=2

Please consider this for 2.6.24.

Jes, Tony, please note that I added an explicit test for 
CONFIG_IA64_SGI_SN2 in asm-ia64/sn/io.h (Yuck). This is 
needed for IA64_GENERIC to build, since there's at least one 
driver (qla1280) that includes asm-ia64/sn/io.h for 
CONFIG_IA64_GENERIC. 

 dma-mapping.h |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 0ebfafb..132b559 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -106,4 +106,22 @@ static inline void dmam_release_declared_memory(struct device *dev)
 }
 #endif /* ARCH_HAS_DMA_DECLARE_COHERENT_MEMORY */
 
+#define DMA_BARRIER_ATTR	0x1
+#ifndef ARCH_USES_DMA_ATTRS
+static inline int dma_flags_set_attr(u32 attr, enum dma_data_direction dir) 
+{
+	return dir;
+}
+
+static inline int dma_flags_get_dir(int flags)
+{
+	return flags;
+}
+
+static inline int dma_flags_get_attr(int flags)
+{
+	return 0;
+}
+#endif /* ARCH_USES_DMA_ATTRS */
+
 #endif

Re: [PATCH 1/3] dma: add dma_flags_set/get_*() interfaces

[Andrew Morton]

On Tue, 16 Oct 2007 18:41:28 -0700 akepner@sgi.com wrote:

> +#define DMA_BARRIER_ATTR	0x1
> +#ifndef ARCH_USES_DMA_ATTRS
> +static inline int dma_flags_set_attr(u32 attr, enum dma_data_direction dir) 
> +{
> +	return dir;
> +}

This function takes an `enum dma_data_direction' as its second arg, but your
ia64 implementation takes an 'int'.

> +static inline int dma_flags_get_dir(int flags)
> +{
> +	return flags;
> +}
> +
> +static inline int dma_flags_get_attr(int flags)
> +{
> +	return 0;
> +}

Re: [PATCH 1/3] dma: add dma_flags_set/get_*() interfaces

[akepner]

[reply to the series of three mails below]

On Tue, Oct 16, 2007 at 08:27:28PM -0700, Andrew Morton wrote:
> On Tue, 16 Oct 2007 18:41:28 -0700 akepner@sgi.com wrote:
> 
> > +#define DMA_BARRIER_ATTR	0x1
> > +#ifndef ARCH_USES_DMA_ATTRS
> > +static inline int dma_flags_set_attr(u32 attr, enum dma_data_direction dir) 
> > +{
> > +	return dir;
> > +}
> 
> This function takes an `enum dma_data_direction' as its second arg, but your
> ia64 implementation takes an 'int'.
> 

This is because the dma_data_direction enum type isn't available 
at the point the ia64 implementation is defined. 


> > .....
> >  dma_addr_t sn_dma_map_single(struct device *dev, void *cpu_addr, size_t size,
> > -                          int direction)
> > +                          int flags)
> >  {
> >       dma_addr_t dma_addr;
> >       unsigned long phys_addr;
> >       struct pci_dev *pdev = to_pci_dev(dev);
> >       struct sn_pcibus_provider *provider = SN_PCIDEV_BUSPROVIDER(pdev);
> > +     int dmabarrier = dma_flags_get_attr(flags) & DMA_BARRIER_ATTR;
> 
> So we take an `enum data_direction' and then wedge it into a word alongside
> some extra flags?
> 
> Can we do something nicer than that?

Changing the type of the last argument to dma_map_* functions 
to be a bitmask? Or adding an additional argument? (Both of 
which you mention below.)

> > .....

> > +DMA_BARRIER_ATTR would be set when the memory region is mapped for DMA,
> > +e.g.:
> > +
> > +     int count;
> > +     int flags = dma_flags_set_attr(DMA_BARRIER_ATTR, DMA_BIDIRECTIONAL);
> > +     ....
> > +     count = dma_map_sg(dev, sglist, nents, flags);
> > +
> 
> Isn't this rather a kludge?

I prefer the term "hack".

> 
> What would be the cost of doing this cleanly and either redefining
> dma_data_direction to be a field-of-bits or just leave dma_data_direction
> alone (it is quite unrelated to this work, isn't it?) and adding new
> fields/arguments to manage this new functionality?

It'd be significantly more work to do change or add arguments 
to the dma_map_* functions. But if that's what I need to do to 
get this accepted, I'll back up and have another go at it.

-- 
Arthur

Re: [PATCH 1/3] dma: add dma_flags_set/get_*() interfaces

[Andrew Morton]

On Wed, 17 Oct 2007 09:03:27 -0700
akepner@sgi.com wrote:

> > 
> > What would be the cost of doing this cleanly and either redefining
> > dma_data_direction to be a field-of-bits or just leave dma_data_direction
> > alone (it is quite unrelated to this work, isn't it?) and adding new
> > fields/arguments to manage this new functionality?
> 
> It'd be significantly more work to do change or add arguments 
> to the dma_map_* functions. But if that's what I need to do to 
> get this accepted, I'll back up and have another go at it.

I don't have any particularly strong opinions on which would be the best
way to clean this up.  Hopefully someone who is more involved with the DMA
mapping interfaces can help out.

It wouldn't be efficient for you to implement something new, only to have
it criticized again.  I'd suggest that you come up with a concrete
design, describe to us what you propose to do and let's take it from there.