[PATCH] DMA: Correct invalid assumptions in the Kconfig text

[PATCH] DMA: Correct invalid assumptions in the Kconfig text

[Haavard Skinnemoen]

This patch corrects what I hope are invalid assumptions about the DMA
engine layer: Not only Intel(R) hardware can do DMA, and DMA can be
used for other things than memcpy and RAID offloading.

At the same time, make the DMA Engine menu visible again on AVR32. I'm
currently working on a driver for a DMA controller that can do
mem-to-mem transfers (which is supported by the framework) as well as
device-to-mem and mem-to-device transfers (not currently supported.)

Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
---
Don't get me wrong; I think Intel deserves lots of respect for
creating this framework. But this is also why I got a bit disappointed
when I discovered that it seems to be less generic than I initially
hoped.

DMA controllers, which may support plain memcpy acceleration in
addition to more traditional "slave DMA", are very common in SoC
devices, and I think Linux needs a common framework for it. The
existing DMA Engine framework seems to come pretty close already, but
I think it needs more input from the embedded crowd before it can be
completely usable on a large number of embedded systems.

I'm not going to suggest any changes to the actual framework for
2.6.24, but I think the _intention_ of the framework needs to be
clarified.

Haavard

 drivers/dma/Kconfig |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 9c91b0f..62a9fe5 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -3,11 +3,13 @@
 #
 
 menuconfig DMADEVICES
-	bool "DMA Offload Engine support"
-	depends on (PCI && X86) || ARCH_IOP32X || ARCH_IOP33X || ARCH_IOP13XX
+	bool "DMA Engine support"
+	depends on (PCI && X86) || ARCH_IOP32X || ARCH_IOP33X || ARCH_IOP13XX || AVR32
 	help
-	  Intel(R) offload engines enable offloading memory copies in the
-	  network stack and RAID operations in the MD driver.
+	  DMA engines can do asynchronous data transfers without
+	  involving the host CPU. This can be used to offload memory
+	  copies in the network stack and RAID operations in the MD
+	  driver.
 
 if DMADEVICES
 
-- 
1.5.3.4

RE: [PATCH] DMA: Correct invalid assumptions in the Kconfig text

[Nelson, Shannon]

>From: Haavard Skinnemoen [mailto:hskinnemoen@atmel.com] 
>Subject: [PATCH] DMA: Correct invalid assumptions in the Kconfig text
>
>This patch corrects what I hope are invalid assumptions about the DMA
>engine layer: Not only Intel(R) hardware can do DMA, and DMA can be
>used for other things than memcpy and RAID offloading.
>
>At the same time, make the DMA Engine menu visible again on AVR32. I'm
>currently working on a driver for a DMA controller that can do
>mem-to-mem transfers (which is supported by the framework) as well as
>device-to-mem and mem-to-device transfers (not currently supported.)
>
>Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
>---
>Don't get me wrong; I think Intel deserves lots of respect for
>creating this framework. But this is also why I got a bit disappointed
>when I discovered that it seems to be less generic than I initially
>hoped.
>
>DMA controllers, which may support plain memcpy acceleration in
>addition to more traditional "slave DMA", are very common in SoC
>devices, and I think Linux needs a common framework for it. The
>existing DMA Engine framework seems to come pretty close already, but
>I think it needs more input from the embedded crowd before it can be
>completely usable on a large number of embedded systems.
>
>I'm not going to suggest any changes to the actual framework for
>2.6.24, but I think the _intention_ of the framework needs to be
>clarified.
>
>Haavard
>
> drivers/dma/Kconfig |   10 ++++++----
> 1 files changed, 6 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
>index 9c91b0f..62a9fe5 100644
>--- a/drivers/dma/Kconfig
>+++ b/drivers/dma/Kconfig
>@@ -3,11 +3,13 @@
> #
> 
> menuconfig DMADEVICES
>-	bool "DMA Offload Engine support"
>-	depends on (PCI && X86) || ARCH_IOP32X || ARCH_IOP33X 
>|| ARCH_IOP13XX
>+	bool "DMA Engine support"
>+	depends on (PCI && X86) || ARCH_IOP32X || ARCH_IOP33X 
>|| ARCH_IOP13XX || AVR32
> 	help
>-	  Intel(R) offload engines enable offloading memory 
>copies in the
>-	  network stack and RAID operations in the MD driver.
>+	  DMA engines can do asynchronous data transfers without
>+	  involving the host CPU. This can be used to offload memory
>+	  copies in the network stack and RAID operations in the MD
>+	  driver.
> 
> if DMADEVICES
> 
>-- 
>1.5.3.4
>

Acked-by: Shannon Nelson <shannon.nelson@intel.com>

Re: [PATCH] DMA: Correct invalid assumptions in the Kconfig text

[Dan Williams]

Hi Haavard,

On 10/24/07, Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
> This patch corrects what I hope are invalid assumptions about the DMA
> engine layer: Not only Intel(R) hardware can do DMA, and DMA can be
> used for other things than memcpy and RAID offloading.
>
> At the same time, make the DMA Engine menu visible again on AVR32. I'm
> currently working on a driver for a DMA controller that can do
> mem-to-mem transfers (which is supported by the framework) as well as
> device-to-mem and mem-to-device transfers (not currently supported.)
>
> Signed-off-by: Haavard Skinnemoen <hskinnemoen@atmel.com>
> ---
> Don't get me wrong; I think Intel deserves lots of respect for
> creating this framework. But this is also why I got a bit disappointed
> when I discovered that it seems to be less generic than I initially
> hoped.
>

Patches welcome :-)

> DMA controllers, which may support plain memcpy acceleration in
> addition to more traditional "slave DMA", are very common in SoC
> devices, and I think Linux needs a common framework for it. The
> existing DMA Engine framework seems to come pretty close already, but
> I think it needs more input from the embedded crowd before it can be
> completely usable on a large number of embedded systems.
>

Part of the problem of supporting slave/device DMA along side generic
memcpy/xor/memset acceleration is that it adds a number of caveats and
restrictions to the interface.  One idea is to create another client
layer, similar to async_tx, that can handle the architecture specific
address, bus, and device pairing restrictions.  In other words make
device-dma a superset of the generic offload capabilities and move it
to its own channel management layer.

> I'm not going to suggest any changes to the actual framework for
> 2.6.24, but I think the _intention_ of the framework needs to be
> clarified.
>

Should this patch wait until the framework has been extended?

Otherwise, Acked-by: Dan Williams <dan.j.williams@intel.com>

> Haavard
>

Regards,
Dan

Re: [PATCH] DMA: Correct invalid assumptions in the Kconfig text

[Haavard Skinnemoen]

On Wed, 24 Oct 2007 08:55:58 -0700
"Dan Williams" <dan.j.williams@intel.com> wrote:
> > Don't get me wrong; I think Intel deserves lots of respect for
> > creating this framework. But this is also why I got a bit
> > disappointed when I discovered that it seems to be less generic
> > than I initially hoped.
> >
> 
> Patches welcome :-)

Sure, that's the plan, and this patch is a start, isn't it? I just
wanted to make sure that the generic-sounding DMA Engine API would
allow more "traditional" DMA controller functionality in addition to
all the high-end RAID and networking stuff.

> Part of the problem of supporting slave/device DMA along side generic
> memcpy/xor/memset acceleration is that it adds a number of caveats and
> restrictions to the interface.  One idea is to create another client
> layer, similar to async_tx, that can handle the architecture specific
> address, bus, and device pairing restrictions.  In other words make
> device-dma a superset of the generic offload capabilities and move it
> to its own channel management layer.

Yes, I've been thinking along those lines, and
Documentation/crypto/async-tx-api.txt seems to suggest something like
that for platform-specific extensions. More specifically, I'm thinking
about adding a few new structs which wrap around some of the existing
ones, like struct dma_async_tx_descriptor, adding new ops and data
members. So it will be sort of a generic, optional extension of the API.

If that works out well, we may also consider moving some of the
async_tx-specific fields into an extended struct of its own. But that
will be more of an optimization or cleanup. I don't want to hurt any
existing users.

Then we're going to need some new hooks for creating those extended
descriptors. This can be done either by adding more hooks in struct
dma_device or by creating another "subclass".

Yes, I'm mostly handwaving at this point, but I intend to follow up
with actual code soon. Unless someone else beats me to it, of course.

> > I'm not going to suggest any changes to the actual framework for
> > 2.6.24, but I think the _intention_ of the framework needs to be
> > clarified.
> >
> 
> Should this patch wait until the framework has been extended?

IMO, no. The first step I'm planning to do is to get the driver working
within the existing framework, i.e. as a pure memcpy offload
engine, and verify that it can indeed improve networking performance.
This would imply that the framework itself isn't Intel-specific even
though all the existing drivers are for Intel hardware. And I don't
think the proposed text makes any new promises that weren't there
before.

And I think it's important to clarify that what is meant to live under
drivers/dma isn't just RAID and networking acceleration engines -- it
is realy meant to be a generic DMA engine framework. If this isn't
true, I might as well stop writing the driver based on this framework
and instead focus on creating a new one.

But I'm pretty optimistic about being able to extend the API in a way
that will not hurt existing functionality and still provide what we
need to support embedded DMA controllers. In fact, it looks a lot
easier to do with the current incarnation of the API than it did
earlier.

> Otherwise, Acked-by: Dan Williams <dan.j.williams@intel.com>

Thanks. Are one of you going to pick it up as well?

Håvard

Re: [PATCH] DMA: Correct invalid assumptions in the Kconfig text

[Haavard Skinnemoen]

On Wed, 24 Oct 2007 20:16:16 +0200
Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:

> [handwaving about API extensions]

Oh, and we definitely need a way to report errors. Looks like the
existing drivers want this as well -- I couldn't help but notice this
in the iop-adma driver:

static irqreturn_t iop_adma_err_handler(int irq, void *data)
{
	(...)

	BUG();
}

That's a panic waiting to happen, isn't it?

Håvard

Re: [PATCH] DMA: Correct invalid assumptions in the Kconfig text

[Dan Williams]

On 10/25/07, Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
> On Wed, 24 Oct 2007 20:16:16 +0200
> Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
>
> > [handwaving about API extensions]
>
> Oh, and we definitely need a way to report errors. Looks like the
> existing drivers want this as well -- I couldn't help but notice this
> in the iop-adma driver:
>
> static irqreturn_t iop_adma_err_handler(int irq, void *data)
> {
>         (...)
>
>         BUG();
> }
>
> That's a panic waiting to happen, isn't it?

Yes, and it should have a comment, because for now this is deliberate.
 This was primarily driven by the fact that MD has no way of
recovering from hardware errors during software-memcpy or
software-xor_blocks so there was no where to plug-in
accelerated-memcpy/xor error recovery.  I can foresee other clients
wanting to have this information reported but async_tx based clients
are supposed to be blissfully unaware of under the covers hardware
acceleration.

One idea is to pass an error-pointer as the parameter to callback
routines, but that would hamper the client's ability to recover the
context of the failure...

>
> Håvard

--
Dan

Re: [PATCH] DMA: Correct invalid assumptions in the Kconfig text

[Haavard Skinnemoen]

On Fri, 26 Oct 2007 10:02:24 -0700
"Dan Williams" <dan.j.williams@intel.com> wrote:
> On 10/25/07, Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
> > static irqreturn_t iop_adma_err_handler(int irq, void *data)
> > {
> >         (...)
> >
> >         BUG();
> > }
> >
> > That's a panic waiting to happen, isn't it?
> 
> Yes, and it should have a comment, because for now this is deliberate.
>  This was primarily driven by the fact that MD has no way of
> recovering from hardware errors during software-memcpy or
> software-xor_blocks so there was no where to plug-in
> accelerated-memcpy/xor error recovery.  I can foresee other clients
> wanting to have this information reported but async_tx based clients
> are supposed to be blissfully unaware of under the covers hardware
> acceleration.

Yeah, it's a pretty serious bug if the DMA engine flags an error. But
wouldn't it be better to BUG() in the context of the caller? That way,
you won't necessarily bring down the whole system.

> One idea is to pass an error-pointer as the parameter to callback
> routines, but that would hamper the client's ability to recover the
> context of the failure...

It's probably a good idea to dump the descriptors at some point. I
don't think the client has many options to deal with such failures
other than reset something or kill something, but if the client gets
notified, it at least has some chance of recovering.

Håvard

Re: [PATCH] DMA: Correct invalid assumptions in the Kconfig text

[Dan Williams]

On 10/27/07, Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
>
> Yeah, it's a pretty serious bug if the DMA engine flags an error. But
> wouldn't it be better to BUG() in the context of the caller? That way,
> you won't necessarily bring down the whole system.
>

I see your point...  We could track the caller's task_struct in
dma_async_tx_descriptor, and deliver a SIGBUS in the case of an error.
 It limits the client's recovery options, but at least the damaged is
localized to the correct process.  I need to go read up on what this
would imply for kernel threads like raid5d...

Re: [PATCH] DMA: Correct invalid assumptions in the Kconfig text

[Dan Williams]

On 10/24/07, Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
> > Otherwise, Acked-by: Dan Williams <dan.j.williams@intel.com>
>
> Thanks. Are one of you going to pick it up as well?
>

Yeah, I'll pick it up.  I'll leave off the AVR addition to DMADEVICES
because I assume it will come with the future patch implementing the
driver, no?

> Håvard

Thanks,
Dan

Re: [PATCH] DMA: Correct invalid assumptions in the Kconfig text

[Haavard Skinnemoen]

On Fri, 26 Oct 2007 09:44:44 -0700
"Dan Williams" <dan.j.williams@gmail.com> wrote:

> On 10/24/07, Haavard Skinnemoen <hskinnemoen@atmel.com> wrote:
> > > Otherwise, Acked-by: Dan Williams <dan.j.williams@intel.com>  
> >
> > Thanks. Are one of you going to pick it up as well?
> >  
> 
> Yeah, I'll pick it up.  I'll leave off the AVR addition to DMADEVICES
> because I assume it will come with the future patch implementing the
> driver, no?

Sure, either way is fine with me.

Thanks,
Håvard