diff mbox

[v2] tmio_mmc_dma: fix PIO fallback on SDHI

Message ID 201308030233.29446.sergei.shtylyov@cogentembedded.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sergei Shtylyov Aug. 2, 2013, 10:33 p.m. UTC 
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. The regression was introduced by commit
162f43e31c5a376ec16336e5d0ac973373d54c89 (mmc: tmio: fix a deadlock).
Moving tmio_mmc_enable_dma() calls to the top of the PIO fallback code in
tmio_mmc_start_dma_{rx|tx}() helps.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Cc: stable@vger.kernel.org # 3.1+

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

Changes in version 2:
- removed extra check from tmio_mmc_enable_dma();
- moved tmio_mmc_enable_dma() calls to the top of the PIO fallback code in
  tmio_mmc_start_dma_{rx|tx}().

 drivers/mmc/host/tmio_mmc_dma.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Guennadi Liakhovetski Aug. 3, 2013, 10:36 p.m. UTC | #1 
On Sat, 3 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. The regression was introduced by commit
> 162f43e31c5a376ec16336e5d0ac973373d54c89 (mmc: tmio: fix a deadlock).
> Moving tmio_mmc_enable_dma() calls to the top of the PIO fallback code in
> tmio_mmc_start_dma_{rx|tx}() helps.
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Cc: stable@vger.kernel.org # 3.1+

Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Not sure though how far back in stable this really has to go. We don't 
have any real-life problem reports with older kernels, do we? I think 
approach to which patches should get into stable changed recently.

Thanks
Guennadi

> 
> ---
> The patch is against Chris Ball's 'mmc.git' repo, 'master' branch.
> 
> Changes in version 2:
> - removed extra check from tmio_mmc_enable_dma();
> - moved tmio_mmc_enable_dma() calls to the top of the PIO fallback code in
>   tmio_mmc_start_dma_{rx|tx}().
> 
>  drivers/mmc/host/tmio_mmc_dma.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Index: renesas/drivers/mmc/host/tmio_mmc_dma.c
> ===================================================================
> --- renesas.orig/drivers/mmc/host/tmio_mmc_dma.c
> +++ renesas/drivers/mmc/host/tmio_mmc_dma.c
> @@ -104,6 +104,7 @@ static void tmio_mmc_start_dma_rx(struct
>  pio:
>  	if (!desc) {
>  		/* DMA failed, fall back to PIO */
> +		tmio_mmc_enable_dma(host, false);
>  		if (ret >= 0)
>  			ret = -EIO;
>  		host->chan_rx = NULL;
> @@ -116,7 +117,6 @@ pio:
>  		}
>  		dev_warn(&host->pdev->dev,
>  			 "DMA failed: %d, falling back to PIO\n", ret);
> -		tmio_mmc_enable_dma(host, false);
>  	}
>  
>  	dev_dbg(&host->pdev->dev, "%s(): desc %p, cookie %d, sg[%d]\n", __func__,
> @@ -185,6 +185,7 @@ static void tmio_mmc_start_dma_tx(struct
>  pio:
>  	if (!desc) {
>  		/* DMA failed, fall back to PIO */
> +		tmio_mmc_enable_dma(host, false);
>  		if (ret >= 0)
>  			ret = -EIO;
>  		host->chan_tx = NULL;
> @@ -197,7 +198,6 @@ pio:
>  		}
>  		dev_warn(&host->pdev->dev,
>  			 "DMA failed: %d, falling back to PIO\n", ret);
> -		tmio_mmc_enable_dma(host, false);
>  	}
>  
>  	dev_dbg(&host->pdev->dev, "%s(): desc %p, cookie %d\n", __func__,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov Aug. 4, 2013, 2:10 p.m. UTC | #2 
Hello.

On 04-08-2013 2:36, 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. The regression was introduced by commit
>> 162f43e31c5a376ec16336e5d0ac973373d54c89 (mmc: tmio: fix a deadlock).
>> Moving tmio_mmc_enable_dma() calls to the top of the PIO fallback code in
>> tmio_mmc_start_dma_{rx|tx}() helps.

>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>> Cc: stable@vger.kernel.org # 3.1+

> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

> Not sure though how far back in stable this really has to go. We don't
> have any real-life problem reports with older kernels, do we?

    If we did, that error would have been fixed earlier, wouldn't it?

> I think
> approach to which patches should get into stable changed recently.

    Haven't heard about that (or seen any change in the policy). The issue 
with this bug is its catastrophic consequencies: you can hardly ^C or ^Z out 
of 'bonnie++' when it happens (in fact, you cannot ^C at all). IIRC you can't 
even kill it from another shell.

> Thanks
> Guennadi

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov Aug. 5, 2013, 10:37 p.m. UTC | #3 
Hello.

On 08/04/2013 06:10 PM, 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. The regression was introduced by
>>> commit
>>> 162f43e31c5a376ec16336e5d0ac973373d54c89 (mmc: tmio: fix a deadlock).
>>> Moving tmio_mmc_enable_dma() calls to the top of the PIO fallback code in
>>> tmio_mmc_start_dma_{rx|tx}() helps.

>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>> Cc: stable@vger.kernel.org # 3.1+

>> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

>> Not sure though how far back in stable this really has to go. We don't
>> have any real-life problem reports with older kernels, do we?

>     If we did, that error would have been fixed earlier, wouldn't it?

>> I think
>> approach to which patches should get into stable changed recently.

>     Haven't heard about that (or seen any change in the policy). The issue
> with this bug is its catastrophic consequencies: you can hardly ^C or ^Z out
> of 'bonnie++' when it happens (in fact, you cannot ^C at all). IIRC you can't
> even kill it from another shell.

    Yeah, and the filesystem is seriously corrupt after this (I've tested only 
on VFAT so far).

>> Thanks
>> Guennadi

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov Aug. 9, 2013, 6:32 p.m. UTC | #4 
Hello.

On 08/03/2013 02:33 AM, Sergei Shtylyov wrote:

> I'm testing SH-Mobile SDHI driver in DMA mode with  a new DMA controller  using

    The driver for this controller is using drivers/dma/sh/shdma-base.c framework.

> 'bonnie++' and getting DMA error

    And I suspect some bug/race in shdma-base.c. The S/G request that fails 
seems to always consist of 32 segments (maximum for shdma-base.c) and it fails 
like this:

hpb-dma-engine hpb-dma-engine: No free link descriptor available

which is a message that shdma_add_desc() prints when shdma_get-desc() returns 
NULL. When I turn on all dev_dbg() calls (via #define DEBUG) in shdma-base.c. 
the error is *gone*...

> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> Cc: stable@vger.kernel.org # 3.1+

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov Aug. 9, 2013, 6:41 p.m. UTC | #5 
Hello.

On 08/06/2013 02:37 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. The regression was introduced by
>>>> commit
>>>> 162f43e31c5a376ec16336e5d0ac973373d54c89 (mmc: tmio: fix a deadlock).
>>>> Moving tmio_mmc_enable_dma() calls to the top of the PIO fallback code in
>>>> tmio_mmc_start_dma_{rx|tx}() helps.

>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>> Cc: stable@vger.kernel.org # 3.1+

>>> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

>>> Not sure though how far back in stable this really has to go. We don't
>>> have any real-life problem reports with older kernels, do we?

>>     If we did, that error would have been fixed earlier, wouldn't it?

>>> I think
>>> approach to which patches should get into stable changed recently.

>>     Haven't heard about that (or seen any change in the policy). The issue
>> with this bug is its catastrophic consequencies: you can hardly ^C or ^Z out
>> of 'bonnie++' when it happens (in fact, you cannot ^C at all). IIRC you can't
>> even kill it from another shell.

>     Yeah, and the filesystem is seriously corrupt after this (I've tested only
> on VFAT so far).

    Tried running 'bonnie++' on SD card with ext3 couple of times -- no 
problems there. On VFAT the DMA bug reproduced each time I tried to run the 
test (at least without the fix).

>>> Thanks
>>> Guennadi

WBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Ball Aug. 25, 2013, 3:39 a.m. UTC | #6 
Hi,

On Sat, Aug 03 2013, Guennadi Liakhovetski wrote:
> On Sat, 3 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. The regression was
>> introduced by commit
>> 162f43e31c5a376ec16336e5d0ac973373d54c89 (mmc: tmio: fix a deadlock).
>> Moving tmio_mmc_enable_dma() calls to the top of the PIO fallback code in
>> tmio_mmc_start_dma_{rx|tx}() helps.
>> 
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>> Cc: stable@vger.kernel.org # 3.1+
>
> Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Thanks, pushed to mmc-next for 3.12.

- Chris.
diff mbox

Patch

Index: renesas/drivers/mmc/host/tmio_mmc_dma.c
===================================================================
--- renesas.orig/drivers/mmc/host/tmio_mmc_dma.c
+++ renesas/drivers/mmc/host/tmio_mmc_dma.c
@@ -104,6 +104,7 @@  static void tmio_mmc_start_dma_rx(struct
 pio:
 	if (!desc) {
 		/* DMA failed, fall back to PIO */
+		tmio_mmc_enable_dma(host, false);
 		if (ret >= 0)
 			ret = -EIO;
 		host->chan_rx = NULL;
@@ -116,7 +117,6 @@  pio:
 		}
 		dev_warn(&host->pdev->dev,
 			 "DMA failed: %d, falling back to PIO\n", ret);
-		tmio_mmc_enable_dma(host, false);
 	}
 
 	dev_dbg(&host->pdev->dev, "%s(): desc %p, cookie %d, sg[%d]\n", __func__,
@@ -185,6 +185,7 @@  static void tmio_mmc_start_dma_tx(struct
 pio:
 	if (!desc) {
 		/* DMA failed, fall back to PIO */
+		tmio_mmc_enable_dma(host, false);
 		if (ret >= 0)
 			ret = -EIO;
 		host->chan_tx = NULL;
@@ -197,7 +198,6 @@  pio:
 		}
 		dev_warn(&host->pdev->dev,
 			 "DMA failed: %d, falling back to PIO\n", ret);
-		tmio_mmc_enable_dma(host, false);
 	}
 
 	dev_dbg(&host->pdev->dev, "%s(): desc %p, cookie %d\n", __func__,