| Message ID | 1590044466-28372-2-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: tmio and renesas_sdhi_internal_dmac: fix dma unmapping | expand |
Hi, On Thu, May 21, 2020 at 04:01:04PM +0900, Yoshihiro Shimoda wrote: > Related drivers like renesas_sdhi_internal_dmac are possible > to lack dma unmaping in error cases (for example response timeout). > > Since tmio_mmc_finish_request() will be always called in any case, > to fix the issue, add end operation into struct tmio_mmc_dma_ops and > call the operation in tmio_mmc_finish_request() to call dma_ummap API > by the related drivers correctly. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > --- > drivers/mmc/host/tmio_mmc.h | 3 +++ > drivers/mmc/host/tmio_mmc_core.c | 8 ++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > index b4cf101..0a4f365 100644 > --- a/drivers/mmc/host/tmio_mmc.h > +++ b/drivers/mmc/host/tmio_mmc.h > @@ -118,6 +118,9 @@ struct tmio_mmc_dma_ops { > void (*release)(struct tmio_mmc_host *host); > void (*abort)(struct tmio_mmc_host *host); > void (*dataend)(struct tmio_mmc_host *host); > + > + /* optional */ > + void (*end)(struct tmio_mmc_host *host); /* held host->lock */ Okay, the good news is that I can reproduce the error case. I also get a growing list in /sys/kernel/debug/dma-api/dump. However, here, the list does not grow at the same rate as the fake timeouts are injected. So, it doesn't look like the unmapping is missed every time but only occasionally, so this seems like a race somewhere? And if that is true, I wonder if we couldn't fix the current error paths instead of adding another callback? Or do you get a missed unmap for every timeout, Shimoda-san? All the best, Wolfram
Hi Wolfram-san, > From: Wolfram Sang, Sent: Friday, June 12, 2020 5:33 AM > > Hi, > > On Thu, May 21, 2020 at 04:01:04PM +0900, Yoshihiro Shimoda wrote: <snip> > > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > > index b4cf101..0a4f365 100644 > > --- a/drivers/mmc/host/tmio_mmc.h > > +++ b/drivers/mmc/host/tmio_mmc.h > > @@ -118,6 +118,9 @@ struct tmio_mmc_dma_ops { > > void (*release)(struct tmio_mmc_host *host); > > void (*abort)(struct tmio_mmc_host *host); > > void (*dataend)(struct tmio_mmc_host *host); > > + > > + /* optional */ > > + void (*end)(struct tmio_mmc_host *host); /* held host->lock */ > > Okay, the good news is that I can reproduce the error case. I also get a > growing list in /sys/kernel/debug/dma-api/dump. Good news! > However, here, the list does not grow at the same rate as the fake > timeouts are injected. So, it doesn't look like the unmapping is missed > every time but only occasionally, so this seems like a race somewhere? > > And if that is true, I wonder if we couldn't fix the current error paths > instead of adding another callback? > > Or do you get a missed unmap for every timeout, Shimoda-san? No, I got. So, I investigate why, and then it found the injected timeout happened on CMD13 didn't cause a missed unmap because the command didn't map any buffer (host->dma_on == false). JFYI, my debug patch and log messages are pasted below: --- diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c index 15e21894bd44..8c306597a105 100644 --- a/drivers/mmc/host/renesas_sdhi_core.c +++ b/drivers/mmc/host/renesas_sdhi_core.c @@ -664,6 +664,12 @@ static int renesas_sdhi_wait_idle(struct tmio_mmc_host *host, u32 bit) if (!timeout) { dev_warn(&host->pdev->dev, "timeout waiting for SD bus idle\n"); + dev_warn(&host->pdev->dev, + "debug=%d timeout=%d no_dma=%d real=%d both=%d map=%d unmap=%d\n", + host->debug, host->debug_timeout, + host->debug_timeout_no_dma, + host->debug_real_timeout, host->debug_both_timeout, + host->debug_map, host->debug_unmap); return -EBUSY; } diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c index 47ac53e91241..af16ff3b0868 100644 --- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c +++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c @@ -208,6 +208,7 @@ renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host, sg_dma_address(sg)); host->dma_on = true; + host->debug_map++; return; @@ -246,6 +247,7 @@ static void renesas_sdhi_internal_dmac_complete_tasklet_fn(unsigned long arg) renesas_sdhi_internal_dmac_enable_dma(host, false); dma_unmap_sg(&host->pdev->dev, host->sg_ptr, host->sg_len, dir); + host->debug_unmap++; if (dir == DMA_FROM_DEVICE) clear_bit(SDHI_INTERNAL_DMAC_RX_IN_USE, &global_flags); diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index b4cf10109162..ee950e453a26 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -189,6 +189,13 @@ struct tmio_mmc_host { void (*hs400_complete)(struct tmio_mmc_host *host); const struct tmio_mmc_dma_ops *dma_ops; + int debug; + int debug_timeout; + int debug_real_timeout; + int debug_both_timeout; + int debug_timeout_no_dma; + int debug_map; + int debug_unmap; }; struct tmio_mmc_host *tmio_mmc_host_alloc(struct platform_device *pdev, diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index d7fde57c78c1..e5aebd44b0c7 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -546,8 +546,19 @@ static void tmio_mmc_cmd_irq(struct tmio_mmc_host *host, unsigned int stat) cmd->resp[0] = cmd->resp[3]; } - if (stat & TMIO_STAT_CMDTIMEOUT) + host->debug++; + if (stat & TMIO_STAT_CMDTIMEOUT || !(host->debug & 0xff)) { cmd->error = -ETIMEDOUT; + if (stat & TMIO_STAT_CMDTIMEOUT) + host->debug_real_timeout++; + if (!(host->debug & 0xff)) { + host->debug_timeout++; + if (!host->dma_on) + host->debug_timeout_no_dma++; + } + if (stat & TMIO_STAT_CMDTIMEOUT && !(host->debug & 0xff)) + host->debug_both_timeout++; + } else if ((stat & TMIO_STAT_CRCFAIL && cmd->flags & MMC_RSP_CRC) || stat & TMIO_STAT_STOPBIT_ERR || stat & TMIO_STAT_CMD_IDX_ERR) @@ -797,6 +808,8 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host) spin_lock_irqsave(&host->lock, flags); + host->dma_on = false; + mrq = host->mrq; if (IS_ERR_OR_NULL(mrq)) { spin_unlock_irqrestore(&host->lock, flags);
diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index b4cf101..0a4f365 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -118,6 +118,9 @@ struct tmio_mmc_dma_ops { void (*release)(struct tmio_mmc_host *host); void (*abort)(struct tmio_mmc_host *host); void (*dataend)(struct tmio_mmc_host *host); + + /* optional */ + void (*end)(struct tmio_mmc_host *host); /* held host->lock */ }; struct tmio_mmc_host { diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c index d7fde57..946fb01 100644 --- a/drivers/mmc/host/tmio_mmc_core.c +++ b/drivers/mmc/host/tmio_mmc_core.c @@ -57,6 +57,12 @@ static inline void tmio_mmc_start_dma(struct tmio_mmc_host *host, host->dma_ops->start(host, data); } +static inline void tmio_mmc_end_dma(struct tmio_mmc_host *host) +{ + if (host->dma_ops && host->dma_ops->end) + host->dma_ops->end(host); +} + static inline void tmio_mmc_enable_dma(struct tmio_mmc_host *host, bool enable) { if (host->dma_ops) @@ -797,6 +803,8 @@ static void tmio_mmc_finish_request(struct tmio_mmc_host *host) spin_lock_irqsave(&host->lock, flags); + tmio_mmc_end_dma(host); + mrq = host->mrq; if (IS_ERR_OR_NULL(mrq)) { spin_unlock_irqrestore(&host->lock, flags);
Related drivers like renesas_sdhi_internal_dmac are possible to lack dma unmaping in error cases (for example response timeout). Since tmio_mmc_finish_request() will be always called in any case, to fix the issue, add end operation into struct tmio_mmc_dma_ops and call the operation in tmio_mmc_finish_request() to call dma_ummap API by the related drivers correctly. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/mmc/host/tmio_mmc.h | 3 +++ drivers/mmc/host/tmio_mmc_core.c | 8 ++++++++ 2 files changed, 11 insertions(+)