[PATCH] mmc: fix race condition between dma and omap_hsmmc callback

[PATCH] mmc: fix race condition between dma and omap_hsmmc callback

[Venkatraman S]

This patch addresses the possible race condition between the dma
callback and hsmmc callback.
All the 'post data transfer' cleanup activities are now done in the
MMC TC handler.
The schedule_timeout in omap_hsmmc_start_dma_transfer is hence not
needed (which, incidentally,
was also throwing a "schedule while atomic" warning when executed).
Tested on OMAP4430 SDP.

CC: Adrian Hunter <adrian.hunter@nokia.com>
CC: Madhusudhan C <madhu.cr@ti.com>
CC: Tony Lindgren <tony@atomide.com>
Signed-off-by: Venkatraman S <svenkatr@ti.com>
---
 drivers/mmc/host/omap_hsmmc.c |   40 ++++++++++++++++------------------------
 1 files changed, 16 insertions(+), 24 deletions(-)

The previous discussion about this patch is here
https://patchwork.kernel.org/patch/82907/
As requested, this is now posted as a separate patch instead of
clubbing with another feature.

diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 83f0aff..9a70b36 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -1046,8 +1046,18 @@ static irqreturn_t omap_hsmmc_irq(int irq, void *dev_id)

 	if (end_cmd || ((status & CC) && host->cmd))
 		omap_hsmmc_cmd_done(host, host->cmd);
-	if ((end_trans || (status & TC)) && host->mrq)
+	if ((end_trans || (status & TC)) && host->mrq) {
+		if (host->dma_ch != -1) {
+			omap_free_dma(host->dma_ch);
+			host->dma_ch = -1;
+			/*
+			 * Callback run in interrupt context.
+			 * mutex_unlock will throw a kernel warning if used.
+			 */
+			up(&host->sem);
+		}
 		omap_hsmmc_xfer_done(host, data);
+	}

 	spin_unlock(&host->irq_lock);

@@ -1267,13 +1277,6 @@ static void omap_hsmmc_dma_cb(int lch, u16
ch_status, void *data)
 		return;
 	}

-	omap_free_dma(host->dma_ch);
-	host->dma_ch = -1;
-	/*
-	 * DMA Callback: run in interrupt context.
-	 * mutex_unlock will throw a kernel warning if used.
-	 */
-	up(&host->sem);
 }

 /*
@@ -1282,7 +1285,7 @@ static void omap_hsmmc_dma_cb(int lch, u16
ch_status, void *data)
 static int omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host,
 					struct mmc_request *req)
 {
-	int dma_ch = 0, ret = 0, err = 1, i;
+	int dma_ch = 0, ret = 0, i;
 	struct mmc_data *data = req->data;

 	/* Sanity check: all the SG entries must be aligned by block size. */
@@ -1300,22 +1303,11 @@ static int
omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host,
 		return -EINVAL;

 	/*
-	 * If for some reason the DMA transfer is still active,
-	 * we wait for timeout period and free the dma
+	 * Can't process the request if the previous
+	 * request is still active
 	 */
-	if (host->dma_ch != -1) {
-		set_current_state(TASK_UNINTERRUPTIBLE);
-		schedule_timeout(100);
-		if (down_trylock(&host->sem)) {
-			omap_free_dma(host->dma_ch);
-			host->dma_ch = -1;
-			up(&host->sem);
-			return err;
-		}
-	} else {
-		if (down_trylock(&host->sem))
-			return err;
-	}
+	if (down_trylock(&host->sem))
+			return -EBUSY;

 	ret = omap_request_dma(omap_hsmmc_get_dma_sync_dev(host, data),
 			       "MMC/SD", omap_hsmmc_dma_cb, host, &dma_ch);
-- 
1.6.3.3

Re: [PATCH] mmc: fix race condition between dma and omap_hsmmc callback

[Adrian Hunter]

Venkatraman S wrote:
> This patch addresses the possible race condition between the dma
> callback and hsmmc callback.

Can you explain the problem in more detail?  If the final DMA interrupt
comes before TC then all should be well.  If it comes after, then we need
to be sure that the DMA has finished - particularly in the "read" case.
Neither the existing code nor this patch seems to address that issue.

Also, how can we be sure the final DMA interrupt doesn't race with the
start of the next request?

> All the 'post data transfer' cleanup activities are now done in the
> MMC TC handler.
> The schedule_timeout in omap_hsmmc_start_dma_transfer is hence not
> needed (which, incidentally,
> was also throwing a "schedule while atomic" warning when executed).
> Tested on OMAP4430 SDP.
> 
> CC: Adrian Hunter <adrian.hunter@nokia.com>
> CC: Madhusudhan C <madhu.cr@ti.com>
> CC: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Venkatraman S <svenkatr@ti.com>
> ---
>  drivers/mmc/host/omap_hsmmc.c |   40 ++++++++++++++++------------------------
>  1 files changed, 16 insertions(+), 24 deletions(-)
> 
> The previous discussion about this patch is here
> https://patchwork.kernel.org/patch/82907/
> As requested, this is now posted as a separate patch instead of
> clubbing with another feature.
> 
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 83f0aff..9a70b36 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -1046,8 +1046,18 @@ static irqreturn_t omap_hsmmc_irq(int irq, void *dev_id)
> 
>  	if (end_cmd || ((status & CC) && host->cmd))
>  		omap_hsmmc_cmd_done(host, host->cmd);
> -	if ((end_trans || (status & TC)) && host->mrq)
> +	if ((end_trans || (status & TC)) && host->mrq) {
> +		if (host->dma_ch != -1) {
> +			omap_free_dma(host->dma_ch);
> +			host->dma_ch = -1;
> +			/*
> +			 * Callback run in interrupt context.
> +			 * mutex_unlock will throw a kernel warning if used.
> +			 */
> +			up(&host->sem);
> +		}
>  		omap_hsmmc_xfer_done(host, data);
> +	}
> 
>  	spin_unlock(&host->irq_lock);
> 
> @@ -1267,13 +1277,6 @@ static void omap_hsmmc_dma_cb(int lch, u16
> ch_status, void *data)
>  		return;
>  	}
> 
> -	omap_free_dma(host->dma_ch);
> -	host->dma_ch = -1;
> -	/*
> -	 * DMA Callback: run in interrupt context.
> -	 * mutex_unlock will throw a kernel warning if used.
> -	 */
> -	up(&host->sem);
>  }
> 
>  /*
> @@ -1282,7 +1285,7 @@ static void omap_hsmmc_dma_cb(int lch, u16
> ch_status, void *data)
>  static int omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host,
>  					struct mmc_request *req)
>  {
> -	int dma_ch = 0, ret = 0, err = 1, i;
> +	int dma_ch = 0, ret = 0, i;
>  	struct mmc_data *data = req->data;
> 
>  	/* Sanity check: all the SG entries must be aligned by block size. */
> @@ -1300,22 +1303,11 @@ static int
> omap_hsmmc_start_dma_transfer(struct omap_hsmmc_host *host,
>  		return -EINVAL;
> 
>  	/*
> -	 * If for some reason the DMA transfer is still active,
> -	 * we wait for timeout period and free the dma
> +	 * Can't process the request if the previous
> +	 * request is still active
>  	 */
> -	if (host->dma_ch != -1) {
> -		set_current_state(TASK_UNINTERRUPTIBLE);
> -		schedule_timeout(100);
> -		if (down_trylock(&host->sem)) {
> -			omap_free_dma(host->dma_ch);
> -			host->dma_ch = -1;
> -			up(&host->sem);
> -			return err;
> -		}
> -	} else {
> -		if (down_trylock(&host->sem))
> -			return err;
> -	}
> +	if (down_trylock(&host->sem))
> +			return -EBUSY;
> 
>  	ret = omap_request_dma(omap_hsmmc_get_dma_sync_dev(host, data),
>  			       "MMC/SD", omap_hsmmc_dma_cb, host, &dma_ch);

Re: [PATCH] mmc: fix race condition between dma and omap_hsmmc callback

[Venkatraman S]

On Tue, Apr 13, 2010 at 3:17 PM, Adrian Hunter <adrian.hunter@nokia.com> wrote:
> Venkatraman S wrote:
>>
>> This patch addresses the possible race condition between the dma
>> callback and hsmmc callback.
>
> Can you explain the problem in more detail?  If the final DMA interrupt
> comes before TC then all should be well.
   Actually it isn't, with descriptor loading. If the DMA callback arrives
"too early", the MMC TC is missed sometimes. The last transfer in the log
below is actually stalled waiting for TC. This happens more often when the
transfer is large (> 300 blocks)

>If it comes after, then we need
> to be sure that the DMA has finished - particularly in the "read" case.
> Neither the existing code nor this patch seems to address that issue.
>
   With this patch the assumption is that MMC TC correctly signals the
end of read / write as requested. I don't know if there are any specific reasons
to believe otherwise.

> Also, how can we be sure the final DMA interrupt doesn't race with the
> start of the next request?
>
Till the time omap_hsmmc_xfer_done is called, the next request is not
expected to arrive.
The requests are implicitly serialised by host->sem, which is released
only on the DMA callback.
The code releases the semaphore only after freeing the DMA channel.
---------------------------------------------------------------------------------------------

mmc0: starting CMD17 arg 00000066 flags 000000b5
mmc0:     blksz 512 blocks 1 flags 00000200 tsac 100 ms nsac 0
mmci-omap-hs mmci-omap-hs.0: mmc0: CMD17, argument 0x00000066
mmci-omap-hs mmci-omap-hs.0: IRQ Status is 1
mmci-omap-hs mmci-omap-hs.0: final dma callback
mmci-omap-hs mmci-omap-hs.0: IRQ Status is 2
mmc0: req done (CMD17): 0: 00000900 00000000 00000000 00000000
mmc0:     512 bytes transferred: 0
mmc0: starting CMD18 arg 000025df flags 000000b5
mmc0:     blksz 512 blocks 32 flags 00000200 tsac 100 ms nsac 0
mmc0:     CMD12 arg 00000000 flags 0000049d
mmci-omap-hs mmci-omap-hs.0: mmc0: CMD18, argument 0x000025df
mmci-omap-hs mmci-omap-hs.0: IRQ Status is 1
mmci-omap-hs mmci-omap-hs.0: IRQ Status is 2
mmci-omap-hs mmci-omap-hs.0: mmc0: CMD12, argument 0x00000000
mmci-omap-hs mmci-omap-hs.0: final dma callback
mmci-omap-hs mmci-omap-hs.0: IRQ Status is 3
mmc0: req done (CMD18): 0: 00000900 00000000 00000000 00000000
mmc0:     16384 bytes transferred: 0
mmc0:     (CMD12): 0: 00000b00 00000000 00000000 00000000
mmc0: starting CMD18 arg 000025ff flags 000000b5
mmc0:     blksz 512 blocks 192 flags 00000200 tsac 100 ms nsac 0
mmc0:     CMD12 arg 00000000 flags 0000049d
mmci-omap-hs mmci-omap-hs.0: new sglist 9fd00000 len =23
mmci-omap-hs mmci-omap-hs.0: mmc0: CMD18, argument 0x000025ff
mmci-omap-hs mmci-omap-hs.0: IRQ Status is 1
mmci-omap-hs mmci-omap-hs.0: final dma callback
mmci-omap-hs mmci-omap-hs.0: IRQ Status is 2
mmci-omap-hs mmci-omap-hs.0: mmc0: CMD12, argument 0x00000000
mmci-omap-hs mmci-omap-hs.0: IRQ Status is 3
mmc0: req done (CMD18): 0: 00000900 00000000 00000000 00000000
mmc0:     98304 bytes transferred: 0
mmc0:     (CMD12): 0: 00000b00 00000000 00000000 00000000
mmc0: starting CMD18 arg 000026bf flags 000000b5
mmc0:     blksz 512 blocks 512 flags 00000200 tsac 100 ms nsac 0
mmc0:     CMD12 arg 00000000 flags 0000049d
mmci-omap-hs mmci-omap-hs.0: new sglist 9fd00000 len =26
mmci-omap-hs mmci-omap-hs.0: mmc0: CMD18, argument 0x000026bf
mmci-omap-hs mmci-omap-hs.0: IRQ Status is 1
mmci-omap-hs mmci-omap-hs.0: final dma callback

Re: [PATCH] mmc: fix race condition between dma and omap_hsmmc callback

[Adrian Hunter]

Venkatraman S wrote:
> On Tue, Apr 13, 2010 at 3:17 PM, Adrian Hunter <adrian.hunter@nokia.com> wrote:
>> Venkatraman S wrote:
>>> This patch addresses the possible race condition between the dma
>>> callback and hsmmc callback.
>> Can you explain the problem in more detail?  If the final DMA interrupt
>> comes before TC then all should be well.
>    Actually it isn't, with descriptor loading. If the DMA callback arrives
> "too early", the MMC TC is missed sometimes. The last transfer in the log
> below is actually stalled waiting for TC. This happens more often when the
> transfer is large (> 300 blocks)

You seem to be saying that, in the case of descriptor loading, the DMA callback
does not indicate that the DMA is complete.  Isn't that a problem with descriptor
loading?

> 
>> If it comes after, then we need
>> to be sure that the DMA has finished - particularly in the "read" case.
>> Neither the existing code nor this patch seems to address that issue.
>>
>    With this patch the assumption is that MMC TC correctly signals the
> end of read / write as requested. I don't know if there are any specific reasons
> to believe otherwise.

It would be nice if there were a way to check that the DMA is complete. i.e.
if TC is received but DMA is not complete then set an error by calling
omap_hsmmc_dma_cleanup() instead of omap_hsmmc_xfer_done()

> 
>> Also, how can we be sure the final DMA interrupt doesn't race with the
>> start of the next request?
>>
> Till the time omap_hsmmc_xfer_done is called, the next request is not
> expected to arrive.
> The requests are implicitly serialised by host->sem, which is released
> only on the DMA callback.
> The code releases the semaphore only after freeing the DMA channel.
> ---------------------------------------------------------------------------------------------
> 
> mmc0: starting CMD17 arg 00000066 flags 000000b5
> mmc0:     blksz 512 blocks 1 flags 00000200 tsac 100 ms nsac 0
> mmci-omap-hs mmci-omap-hs.0: mmc0: CMD17, argument 0x00000066
> mmci-omap-hs mmci-omap-hs.0: IRQ Status is 1
> mmci-omap-hs mmci-omap-hs.0: final dma callback
> mmci-omap-hs mmci-omap-hs.0: IRQ Status is 2
> mmc0: req done (CMD17): 0: 00000900 00000000 00000000 00000000
> mmc0:     512 bytes transferred: 0
> mmc0: starting CMD18 arg 000025df flags 000000b5
> mmc0:     blksz 512 blocks 32 flags 00000200 tsac 100 ms nsac 0
> mmc0:     CMD12 arg 00000000 flags 0000049d
> mmci-omap-hs mmci-omap-hs.0: mmc0: CMD18, argument 0x000025df
> mmci-omap-hs mmci-omap-hs.0: IRQ Status is 1
> mmci-omap-hs mmci-omap-hs.0: IRQ Status is 2
> mmci-omap-hs mmci-omap-hs.0: mmc0: CMD12, argument 0x00000000
> mmci-omap-hs mmci-omap-hs.0: final dma callback
> mmci-omap-hs mmci-omap-hs.0: IRQ Status is 3
> mmc0: req done (CMD18): 0: 00000900 00000000 00000000 00000000
> mmc0:     16384 bytes transferred: 0
> mmc0:     (CMD12): 0: 00000b00 00000000 00000000 00000000
> mmc0: starting CMD18 arg 000025ff flags 000000b5
> mmc0:     blksz 512 blocks 192 flags 00000200 tsac 100 ms nsac 0
> mmc0:     CMD12 arg 00000000 flags 0000049d
> mmci-omap-hs mmci-omap-hs.0: new sglist 9fd00000 len =23
> mmci-omap-hs mmci-omap-hs.0: mmc0: CMD18, argument 0x000025ff
> mmci-omap-hs mmci-omap-hs.0: IRQ Status is 1
> mmci-omap-hs mmci-omap-hs.0: final dma callback
> mmci-omap-hs mmci-omap-hs.0: IRQ Status is 2
> mmci-omap-hs mmci-omap-hs.0: mmc0: CMD12, argument 0x00000000
> mmci-omap-hs mmci-omap-hs.0: IRQ Status is 3
> mmc0: req done (CMD18): 0: 00000900 00000000 00000000 00000000
> mmc0:     98304 bytes transferred: 0
> mmc0:     (CMD12): 0: 00000b00 00000000 00000000 00000000
> mmc0: starting CMD18 arg 000026bf flags 000000b5
> mmc0:     blksz 512 blocks 512 flags 00000200 tsac 100 ms nsac 0
> mmc0:     CMD12 arg 00000000 flags 0000049d
> mmci-omap-hs mmci-omap-hs.0: new sglist 9fd00000 len =26
> mmci-omap-hs mmci-omap-hs.0: mmc0: CMD18, argument 0x000026bf
> mmci-omap-hs mmci-omap-hs.0: IRQ Status is 1
> mmci-omap-hs mmci-omap-hs.0: final dma callback
> 

Re: [PATCH] mmc: fix race condition between dma and omap_hsmmc callback

[Venkatraman S]

Adrian Hunter <adrian.hunter@nokia.com> wrote:
> Venkatraman S wrote:
>>
>> On Tue, Apr 13, 2010 at 3:17 PM, Adrian Hunter <adrian.hunter@nokia.com>
>> wrote:
>>>
>>> Venkatraman S wrote:
>>>>
>>>> This patch addresses the possible race condition between the dma
>>>> callback and hsmmc callback.
>>>
>>> Can you explain the problem in more detail?  If the final DMA interrupt
>>> comes before TC then all should be well.
>>
>>   Actually it isn't, with descriptor loading. If the DMA callback arrives
>> "too early", the MMC TC is missed sometimes. The last transfer in the log
>> below is actually stalled waiting for TC. This happens more often when the
>> transfer is large (> 300 blocks)
>
> You seem to be saying that, in the case of descriptor loading, the DMA
> callback
> does not indicate that the DMA is complete.  Isn't that a problem with
> descriptor
> loading?
>
  My understanding from the existing code
<snip>
        /*
         * If for some reason the DMA transfer is still active,
         * we wait for timeout period and free the dma
         */
        if (host->dma_ch != -1) {
                set_current_state(TASK_UNINTERRUPTIBLE);
</snip>
 is that it's rare, but not impossible, for the DMA to be active even
when the TC has been received.
  At the same time, I found one issue in descriptor loading
after I sent the email yesterday, which could fix the problem I am seeing.

>>
>>> If it comes after, then we need
>>> to be sure that the DMA has finished - particularly in the "read" case.
>>> Neither the existing code nor this patch seems to address that issue.
>>>
>>   With this patch the assumption is that MMC TC correctly signals the
>> end of read / write as requested. I don't know if there are any specific
>> reasons
>> to believe otherwise.
>
> It would be nice if there were a way to check that the DMA is complete. i.e.
> if TC is received but DMA is not complete then set an error by calling
> omap_hsmmc_dma_cleanup() instead of omap_hsmmc_xfer_done()
>
  Yes, I was testing such a mechanism as well.  Is it better to abort the
transfer by setting an error, or do the omap_hsmmc_xfer_done() at the
callback of whichever
interrupt comes last ?
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html