[1/3] mmc: tmio: core: Add end operation into tmio_mmc_dma_ops
diff mbox series

Message ID 1590044466-28372-2-git-send-email-yoshihiro.shimoda.uh@renesas.com
State New
Headers show
Series
  • mmc: tmio and renesas_sdhi_internal_dmac: fix dma unmapping
Related show

Commit Message

Yoshihiro Shimoda May 21, 2020, 7:01 a.m. UTC
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(+)

Comments

Wolfram Sang June 11, 2020, 8:32 p.m. UTC | #1
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
Yoshihiro Shimoda June 12, 2020, 4:18 a.m. UTC | #2
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);

Patch
diff mbox series

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);