RE: [PATCH 0/7] fsl-dma: fixes for Freescale DMA driver

Hi Ira,

My comments inline.

-----Original Message-----
From: Ira W. Snyder [mailto:iws@ovro.caltech.edu]
Sent: Wednesday, August 01, 2012 7:46 AM
To: linux-crypto@vger.kernel.org
Cc: linuxppc-dev@lists.ozlabs.org; Liu Qiang-B32616; Ira W. Snyder
Subject: [PATCH 0/7] fsl-dma: fixes for Freescale DMA driver
=20
From: "Ira W. Snyder" <redacted>
=20
Hello everyone,
=20
This is my alternative (simpler) attempt at solving the problems reported
by Qiang Liu with the async_tx API and MD RAID hardware offload support
when using the Freescale DMA driver.
=20
The bug is caused by this driver freeing descriptors before they have
been ACKed by software using the async_tx API.
=20
I don't like Qiang Liu's code to check where the hardware is in the
processing of the descriptor chain, and try to free a partial list of
descriptors. This was a source of bugs in this driver before I fixed them
several years ago.

It's a bug which you think the whole list is completed when an interrupt is=
 raised, there is a potential risk when an interrupt is raised by "Programm=
ed Error". The "ld_running" is a s/w concept, we should not depend on it to=
 judge the status of descriptors list.

I know you don't like this process, but it's a safe and common process. You=
 can refer to other dma drivers, like ioap-adma, mv-xor and ibm-ppc440x-adm=
a.
Said far point, usb also take this method to judge which descriptor is comp=
leted, I don't know which device can use a s/w list to free all descriptors=
, you can refer to the implement of dl_reverse_done_list().

If you find any problem in my patch, please point out, or you can give a li=
nk about the bug you mentioned many years ago.

Thanks.

=20
Instead, the DMA controller raises an interrupt every time it has
completed a descriptor chain. This means it is ready for new descriptors:
no need to try and figure out where it is in the middle of a descriptor
chain.

Attached again,
The interrupt is only report the state of hardware, we cannot assume all de=
scriptors are finished when an interrupt is raised.

=20
Qiang Liu: I do not have a hardware setup capable of using MD RAID.
Please test these patches to see if they fix the bug you reported. You
may use these patches as-is, or build upon them.

I hope we can discuss it based on my patch. If you think my patch will invo=
lve some issue, please point out. I'm willing to fix it.
Thanks.

=20
I have tested this using the drivers/dma/dmatest.c driver, as well as the
CARMA drivers. There are no regressions that I can find.
=20
[  355.069679] dma0chan3-copy0: terminating after 100000 tests, 0
failures (status 0) [  355.192278] dma0chan2-copy0: terminating after
100000 tests, 0 failures (status 0)
=20
Ira W. Snyder (5):
  fsl-dma: minimize locking overhead
  fsl-dma: add fsl_dma_free_descriptor() to reduce code duplication
  fsl-dma: move functions to avoid forward declarations
  fsl-dma: fix support for async_tx API
  carma: remove unnecessary DMA_INTERRUPT capability
=20
Qiang Liu (2):
  fsl-dma: remove attribute DMA_INTERRUPT of dmaengine
  fsl-dma: fix a warning of unitialized cookie
=20
 drivers/dma/fsldma.c                    |  318 +++++++++++++++----------
------
 drivers/dma/fsldma.h                    |    1 +
 drivers/misc/carma/carma-fpga-program.c |    1 -
 drivers/misc/carma/carma-fpga.c         |    3 +-
 4 files changed, 159 insertions(+), 164 deletions(-)
=20
--
1.7.8.6
=20