[PATCH] spi: tegra: slink: do not prepare dma transfer with DMA_CTRL_ACK flag

[PATCH] spi: tegra: slink: do not prepare dma transfer with DMA_CTRL_ACK flag

[Laxman Dewangan]

Spi starts transfer using dma with DMA_CTRL_ACK which is not require
becasue spi driver does not use completed dma_desc after transfer
done and so it does not ack the dma descriptor. Removing the
DMA_CTRL_ACK flag to avoid memory leak in dma driver.

Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/spi/spi-tegra20-slink.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-tegra20-slink.c b/drivers/spi/spi-tegra20-slink.c
index 7882b50..5b4c8c8 100644
--- a/drivers/spi/spi-tegra20-slink.c
+++ b/drivers/spi/spi-tegra20-slink.c
@@ -473,7 +473,7 @@ static int tegra_slink_start_tx_dma(struct tegra_slink_data *tspi, int len)
 	INIT_COMPLETION(tspi->tx_dma_complete);
 	tspi->tx_dma_desc = dmaengine_prep_slave_single(tspi->tx_dma_chan,
 				tspi->tx_dma_phys, len, DMA_MEM_TO_DEV,
-				DMA_PREP_INTERRUPT |  DMA_CTRL_ACK);
+				DMA_PREP_INTERRUPT);
 	if (!tspi->tx_dma_desc) {
 		dev_err(tspi->dev, "Not able to get desc for Tx\n");
 		return -EIO;
@@ -492,7 +492,7 @@ static int tegra_slink_start_rx_dma(struct tegra_slink_data *tspi, int len)
 	INIT_COMPLETION(tspi->rx_dma_complete);
 	tspi->rx_dma_desc = dmaengine_prep_slave_single(tspi->rx_dma_chan,
 				tspi->rx_dma_phys, len, DMA_DEV_TO_MEM,
-				DMA_PREP_INTERRUPT |  DMA_CTRL_ACK);
+				DMA_PREP_INTERRUPT);
 	if (!tspi->rx_dma_desc) {
 		dev_err(tspi->dev, "Not able to get desc for Rx\n");
 		return -EIO;
-- 
1.7.1.1

Re: [PATCH] spi: tegra: slink: do not prepare dma transfer with DMA_CTRL_ACK flag

[Stephen Warren]

On 11/23/2012 02:22 AM, Laxman Dewangan wrote:
> Spi starts transfer using dma with DMA_CTRL_ACK which is not require
> becasue spi driver does not use completed dma_desc after transfer
> done and so it does not ack the dma descriptor. Removing the
> DMA_CTRL_ACK flag to avoid memory leak in dma driver.

I'm not quite sure, but isn't this the opposite of what's wanted. I
think that setting this flag in prep() means that the SPI driver need
not explicitly ack it later?

At least, tegra_dma_desc_get() returns an allocated descriptor if one
exists and async_tx_test_ack()==true for it, and it's true when the
DMA_CTRL_ACK flag is set, which happens either due to calling
async_tx_ack(), or because tegra_dma_prep_slave_sg() was called with
DMA_CTRL_ACK in flags.

Re: [PATCH] spi: tegra: slink: do not prepare dma transfer with DMA_CTRL_ACK flag

[Laxman Dewangan]

On Tuesday 27 November 2012 03:33 AM, Stephen Warren wrote:
> On 11/23/2012 02:22 AM, Laxman Dewangan wrote:
>> Spi starts transfer using dma with DMA_CTRL_ACK which is not require
>> becasue spi driver does not use completed dma_desc after transfer
>> done and so it does not ack the dma descriptor. Removing the
>> DMA_CTRL_ACK flag to avoid memory leak in dma driver.
> I'm not quite sure, but isn't this the opposite of what's wanted. I
> think that setting this flag in prep() means that the SPI driver need
> not explicitly ack it later?
>
> At least, tegra_dma_desc_get() returns an allocated descriptor if one
> exists and async_tx_test_ack()==true for it, and it's true when the
> DMA_CTRL_ACK flag is set, which happens either due to calling
> async_tx_ack(), or because tegra_dma_prep_slave_sg() was called with
> DMA_CTRL_ACK in flags.

I think DMA_CTRL_ACK flag is require if we want to free/reuse desc only 
when client ack it.

Although some part of implementation is wrong in the tegra20-apb-dma.c.

static struct tegra_dma_desc *tegra_dma_desc_get(
                 struct tegra_dma_channel *tdc)
{
         struct tegra_dma_desc *dma_desc;
         unsigned long flags;

         spin_lock_irqsave(&tdc->lock, flags);

         /* Do not allocate if desc are waiting for ack */
         list_for_each_entry(dma_desc, &tdc->free_dma_desc, node) {
                 if (async_tx_test_ack(&dma_desc->txd)) {
                         list_del(&dma_desc->node);
:::::::::::::::

}

In above it should be if (!async_tx_test_ack())

And when client done with the desc, then it can say as async_tx_clear_ack().


------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov

Re: [PATCH] spi: tegra: slink: do not prepare dma transfer with DMA_CTRL_ACK flag

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 341 bytes --]

On Fri, Nov 23, 2012 at 02:52:39PM +0530, Laxman Dewangan wrote:
> Spi starts transfer using dma with DMA_CTRL_ACK which is not require
> becasue spi driver does not use completed dma_desc after transfer
> done and so it does not ack the dma descriptor. Removing the
> DMA_CTRL_ACK flag to avoid memory leak in dma driver.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: tegra: slink: do not prepare dma transfer with DMA_CTRL_ACK flag

[Laxman Dewangan]

On Monday 01 April 2013 07:05 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Fri, Nov 23, 2012 at 02:52:39PM +0530, Laxman Dewangan wrote:
>> Spi starts transfer using dma with DMA_CTRL_ACK which is not require
>> becasue spi driver does not use completed dma_desc after transfer
>> done and so it does not ack the dma descriptor. Removing the
>> DMA_CTRL_ACK flag to avoid memory leak in dma driver.
> Applied, thanks.

Mark,
There was bug in dma driver about handling of DMA_CTRL_ACK flag which we 
fixed after Stephen pointed out.
This change is no more require.
Can you please drop this patch? Let me know if I need to send the revert 
patch.

Sorry for inconvenience.

Thanks,
Laxan

Re: [PATCH] spi: tegra: slink: do not prepare dma transfer with DMA_CTRL_ACK flag

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 474 bytes --]

On Wed, Apr 03, 2013 at 03:01:25PM +0530, Laxman Dewangan wrote:

> There was bug in dma driver about handling of DMA_CTRL_ACK flag
> which we fixed after Stephen pointed out.
> This change is no more require.
> Can you please drop this patch? Let me know if I need to send the
> revert patch.

Someone really should've followed up to this patch then, the last
discussion against it was you saying you thought it was OK after Stephen
queried it.  Anyway, I've reverted now.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] spi: tegra: slink: do not prepare dma transfer with DMA_CTRL_ACK flag

[Laxman Dewangan]

On Monday 01 April 2013 07:05 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Fri, Nov 23, 2012 at 02:52:39PM +0530, Laxman Dewangan wrote:
>> Spi starts transfer using dma with DMA_CTRL_ACK which is not require
>> becasue spi driver does not use completed dma_desc after transfer
>> done and so it does not ack the dma descriptor. Removing the
>> DMA_CTRL_ACK flag to avoid memory leak in dma driver.
> Applied, thanks.

Mark,
There was bug in dma driver about handling of DMA_CTRL_ACK flag which we 
fixed after Stephen pointed out.
This change is no more require.
Can you please drop this patch? Let me know if I need to send the revert 
patch.

Sorry for inconvenience.

Thanks,
Laxan