| Message ID | 201308020117.17897.sergei.shtylyov@cogentembedded.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)
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(-) -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html