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

[Liu Qiang-B32616]

Hi Ira,

My comments inline.

> -----Original Message-----
> From: Ira W. Snyder [mailto:iws at ovro.caltech.edu]
> Sent: Wednesday, August 01, 2012 7:46 AM
> To: linux-crypto at vger.kernel.org
> Cc: linuxppc-dev at lists.ozlabs.org; Liu Qiang-B32616; Ira W. Snyder
> Subject: [PATCH 0/7] fsl-dma: fixes for Freescale DMA driver
> 
> From: "Ira W. Snyder" <iws at ovro.caltech.edu>
> 
> Hello everyone,
> 
> 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.
> 
> The bug is caused by this driver freeing descriptors before they have
> been ACKed by software using the async_tx API.
> 
> 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 "Programmed 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-adma.
Said far point, usb also take this method to judge which descriptor is completed, 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 link about the bug you mentioned many years ago.

Thanks.


> 
> 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 descriptors are finished when an interrupt is raised.

> 
> 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 involve some issue, please point out. I'm willing to fix it.
Thanks.

> 
> 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.
> 
> [  355.069679] dma0chan3-copy0: terminating after 100000 tests, 0
> failures (status 0) [  355.192278] dma0chan2-copy0: terminating after
> 100000 tests, 0 failures (status 0)
> 
> 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
> 
> Qiang Liu (2):
>   fsl-dma: remove attribute DMA_INTERRUPT of dmaengine
>   fsl-dma: fix a warning of unitialized cookie
> 
>  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(-)
> 
> --
> 1.7.8.6
>