[PATCH] tmio_mmc_dma: fix PIO fallback on SDHI

[PATCH] tmio_mmc_dma: fix PIO fallback on SDHI

[Sergei Shtylyov]

I'm testing SH-Mobile SDHI driver in DMA mode with a  new DMA controller  using
'bonnie++' and getting DMA error after which the tmio_mmc_dma.c code falls back
to PIO but all commands time out after that.  It turned out that the fallback
code calls tmio_mmc_enable_dma() with RX/TX channels already freed and pointers
to them cleared, so that the function bails out early instead  of clearing the
DMA bit in the CTL_DMA_ENABLE register.  Fixing the RX/TX channel check so that
it takes place only when enabling DMA helps with the PIO fallback.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
The patch is against Chris Ball's 'mmc.git' repo, 'master' branch.

 drivers/mmc/host/tmio_mmc_dma.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: mmc/drivers/mmc/host/tmio_mmc_dma.c
===================================================================
--- mmc.orig/drivers/mmc/host/tmio_mmc_dma.c
+++ mmc/drivers/mmc/host/tmio_mmc_dma.c
@@ -25,7 +25,7 @@
 
 void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable)
 {
-	if (!host->chan_tx || !host->chan_rx)
+	if (enable && !(host->chan_tx && host->chan_rx))
 		return;
 
 #if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE)

Re: [PATCH] tmio_mmc_dma: fix PIO fallback on SDHI

[Sergei Shtylyov]

Hello.

On 08/02/2013 01:17 AM, Sergei Shtylyov wrote:

> I'm testing SH-Mobile SDHI driver in DMA mode with a  new DMA controller  using
> 'bonnie++' and getting DMA error after which the tmio_mmc_dma.c code falls back
> to PIO but all commands time out after that.  It turned out that the fallback
> code calls tmio_mmc_enable_dma() with RX/TX channels already freed and pointers
> to them cleared, so that the function bails out early instead  of clearing the
> DMA bit in the CTL_DMA_ENABLE register.  Fixing the RX/TX channel check so that
> it takes place only when enabling DMA helps with the PIO fallback.

> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

    Forgot to mark the patch for stable. The bug was introduced by commit 
162f43e31c5a376ec16336e5d0ac973373d54c89 (mmc: tmio: fix a deadlock) back in 
2011 -- should probably have mentioned that in the changelog too.

WBR, Sergei

Re: [PATCH] tmio_mmc_dma: fix PIO fallback on SDHI

[Guennadi Liakhovetski]

Hi Sergei,

Thanks for your patch.

On Fri, 2 Aug 2013, Sergei Shtylyov wrote:

> I'm testing SH-Mobile SDHI driver in DMA mode with a  new DMA controller  using
> 'bonnie++' and getting DMA error after which the tmio_mmc_dma.c code falls back
> to PIO but all commands time out after that.  It turned out that the fallback
> code calls tmio_mmc_enable_dma() with RX/TX channels already freed and pointers
> to them cleared, so that the function bails out early instead  of clearing the
> DMA bit in the CTL_DMA_ENABLE register.  Fixing the RX/TX channel check so that
> it takes place only when enabling DMA helps with the PIO fallback.
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> 
> ---
> The patch is against Chris Ball's 'mmc.git' repo, 'master' branch.
> 
>  drivers/mmc/host/tmio_mmc_dma.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: mmc/drivers/mmc/host/tmio_mmc_dma.c
> ===================================================================
> --- mmc.orig/drivers/mmc/host/tmio_mmc_dma.c
> +++ mmc/drivers/mmc/host/tmio_mmc_dma.c
> @@ -25,7 +25,7 @@
>  
>  void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable)
>  {
> -	if (!host->chan_tx || !host->chan_rx)
> +	if (enable && !(host->chan_tx && host->chan_rx))
>  		return;

Ok, I see the problem and this does fix it. But it adds complexity to the 
driver - one more condition to an if. Whereas, I think, it can be avoided 
if we just move calls to

tmio_mmc_enable_dma(host, false);

in tmio_mmc_start_dma_rx() and tmio_mmc_start_dma_tx() a couple of lines 
up - before clearing ->chan_rx and ->chan_tx pointers? That should work 
too at no cost. I think that would be a better fix, could you, please, 
try?

Thanks
Guennadi

>  #if defined(CONFIG_SUPERH) || defined(CONFIG_ARCH_SHMOBILE)

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] tmio_mmc_dma: fix PIO fallback on SDHI

[Sergei Shtylyov]

On 08/02/2013 03:35 PM, Guennadi Liakhovetski wrote:

>> I'm testing SH-Mobile SDHI driver in DMA mode with a  new DMA controller  using
>> 'bonnie++' and getting DMA error after which the tmio_mmc_dma.c code falls back
>> to PIO but all commands time out after that.  It turned out that the fallback
>> code calls tmio_mmc_enable_dma() with RX/TX channels already freed and pointers
>> to them cleared, so that the function bails out early instead  of clearing the
>> DMA bit in the CTL_DMA_ENABLE register.  Fixing the RX/TX channel check so that
>> it takes place only when enabling DMA helps with the PIO fallback.

>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

>> ---
>> The patch is against Chris Ball's 'mmc.git' repo, 'master' branch.

>>   drivers/mmc/host/tmio_mmc_dma.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)

>> Index: mmc/drivers/mmc/host/tmio_mmc_dma.c
>> ===================================================================
>> --- mmc.orig/drivers/mmc/host/tmio_mmc_dma.c
>> +++ mmc/drivers/mmc/host/tmio_mmc_dma.c
>> @@ -25,7 +25,7 @@
>>
>>   void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable)
>>   {
>> -	if (!host->chan_tx || !host->chan_rx)
>> +	if (enable && !(host->chan_tx && host->chan_rx))
>>   		return;

> Ok, I see the problem and this does fix it. But it adds complexity to the
> driver - one more condition to an if.

    So what?

> Whereas, I think, it can be avoided if we just move calls to

> tmio_mmc_enable_dma(host, false);

> in tmio_mmc_start_dma_rx() and tmio_mmc_start_dma_tx() a couple of lines

    Well, not couple. :-P

> up - before clearing ->chan_rx and ->chan_tx pointers? That should work
> too at no cost.

    Well, it was my first variant. However, not knowing the hardware well, I 
deemed it not quite safe to clear the DMA bit before shutting down the DMA 
channels, so came up with the second version (which I consider more logically 
correct).

> I think that would be a better fix, could you, please, try?

    I tried it but mystically fixing the PIO fallback makes DMA error less 
frequent. IIRC, I didn't see DMA error with the first variant of the fix, so 
I'll have to retry it...

> Thanks
> Guennadi

WBR, Sergei