[PATCH 1/3] mmc: dw_mmc: Increase cmd11 timeout to 500ms

[PATCH 1/3] mmc: dw_mmc: Increase cmd11 timeout to 500ms

[Doug Anderson]

The Designware databook claims that cmd11 should be finished in 2ms,
but my testing showed that not to be the case in some situations.
I've seen cmd11 timeouts of up to 130ms (!) during reboot tests.
Let's bump the timeout way up so that we're absolutely sure.  CMD11 is
only sent during card insertion, so this extra timeout shouldn't be
terrible.

Fixes: 5c935165da79 ("mmc: dw_mmc: Add a timeout for sending CMD11")
Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/mmc/host/dw_mmc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 3883fe6..339a929 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1023,11 +1023,13 @@ static void __dw_mci_start_request(struct dw_mci *host,
 
 	if (cmd->opcode == SD_SWITCH_VOLTAGE) {
 		/*
-		 * Databook says to fail after 2ms w/ no response; give an
-		 * extra jiffy just in case we're about to roll over.
+		 * Databook says to fail after 2ms w/ no response, but evidence
+		 * shows that sometimes the cmd11 interrupt takes over 130ms.
+		 * We'll set to 500ms, plus an extra jiffy just in case jiffies
+		 * is just about to roll over.
 		 */
 		mod_timer(&host->cmd11_timer,
-			  jiffies + msecs_to_jiffies(2) + 1);
+			  jiffies + msecs_to_jiffies(500) + 1);
 	}
 
 	if (mrq->stop)
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 2/3] mmc: dw_mmc: Add a return in an unexpected cmd11 timeout

[Doug Anderson]

If we get an unexpected cmd11 timeout we shouldn't actually treat it
as a timeout (not that we really expect to get an unexpected cmd11
timeout, but still).

Fixes: 5c935165da79 ("mmc: dw_mmc: Add a timeout for sending CMD11")
Reported-by: Jaehoon Chung <jh80.chung@samsung.com>
Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/mmc/host/dw_mmc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 339a929..357ef04 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -2589,8 +2589,10 @@ static void dw_mci_cmd11_timer(unsigned long arg)
 {
 	struct dw_mci *host = (struct dw_mci *)arg;
 
-	if (host->state != STATE_SENDING_CMD11)
-		dev_info(host->dev, "Unexpected CMD11 timeout\n");
+	if (host->state != STATE_SENDING_CMD11) {
+		dev_warn(host->dev, "Unexpected CMD11 timeout\n");
+		return;
+	}
 
 	host->cmd_status = SDMMC_INT_RTO;
 	set_bit(EVENT_CMD_COMPLETE, &host->pending_events);
-- 
2.2.0.rc0.207.ga3a616c

[PATCH 3/3] mmc: dw_mmc: Add locking around cmd11 timer

[Doug Anderson]

It is possible for the cmd11 interrupt to fire and delete the
cmd11_timer before the cmd11_timer was actually setup.  Let's fix this
race by adding a few spinlocks.  Note that the race wasn't seen in
practice without adding some printk statements, but it still seems
wise to fix.

Fixes: 5c935165da79 ("mmc: dw_mmc: Add a timeout for sending CMD11")
Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/mmc/host/dw_mmc.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 357ef04..105a602 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1022,14 +1022,23 @@ static void __dw_mci_start_request(struct dw_mci *host,
 	dw_mci_start_command(host, cmd, cmdflags);
 
 	if (cmd->opcode == SD_SWITCH_VOLTAGE) {
+		unsigned long irqflags;
+
 		/*
 		 * Databook says to fail after 2ms w/ no response, but evidence
 		 * shows that sometimes the cmd11 interrupt takes over 130ms.
 		 * We'll set to 500ms, plus an extra jiffy just in case jiffies
 		 * is just about to roll over.
+		 *
+		 * We do this whole thing under spinlock and only if the
+		 * command hasn't already completed (indicating the the irq
+		 * already ran so we don't want the timeout).
 		 */
-		mod_timer(&host->cmd11_timer,
-			  jiffies + msecs_to_jiffies(500) + 1);
+		spin_lock_irqsave(&host->irq_lock, irqflags);
+		if (!test_bit(EVENT_CMD_COMPLETE, &host->pending_events))
+			mod_timer(&host->cmd11_timer,
+				jiffies + msecs_to_jiffies(500) + 1);
+		spin_unlock_irqrestore(&host->irq_lock, irqflags);
 	}
 
 	if (mrq->stop)
@@ -2170,11 +2179,20 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
 		/* Check volt switch first, since it can look like an error */
 		if ((host->state == STATE_SENDING_CMD11) &&
 		    (pending & SDMMC_INT_VOLT_SWITCH)) {
-			del_timer(&host->cmd11_timer);
+			unsigned long irqflags;
 
 			mci_writel(host, RINTSTS, SDMMC_INT_VOLT_SWITCH);
 			pending &= ~SDMMC_INT_VOLT_SWITCH;
+
+			/*
+			 * Hold the lock; we know cmd11_timer can't be kicked
+			 * off after the lock is released, so safe to delete.
+			 */
+			spin_lock_irqsave(&host->irq_lock, irqflags);
 			dw_mci_cmd_interrupt(host, pending);
+			spin_unlock_irqrestore(&host->irq_lock, irqflags);
+
+			del_timer(&host->cmd11_timer);
 		}
 
 		if (pending & DW_MCI_CMD_ERROR_FLAGS) {
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH 1/3] mmc: dw_mmc: Increase cmd11 timeout to 500ms

[Jaehoon Chung]

Hi, Doug.

On 04/04/2015 03:13 AM, Doug Anderson wrote:
> The Designware databook claims that cmd11 should be finished in 2ms,
> but my testing showed that not to be the case in some situations.
> I've seen cmd11 timeouts of up to 130ms (!) during reboot tests.
> Let's bump the timeout way up so that we're absolutely sure.  CMD11 is
> only sent during card insertion, so this extra timeout shouldn't be
> terrible.

Is it h/w problem? Could you explain to me about "some situations"?
As you said, this timeout only used during card inserting. So, it's not critical..
But there is much different between 2ms and 500ms(or 130ms).

Best Regards,
Jaehoon Chung

> 
> Fixes: 5c935165da79 ("mmc: dw_mmc: Add a timeout for sending CMD11")
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/mmc/host/dw_mmc.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 3883fe6..339a929 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1023,11 +1023,13 @@ static void __dw_mci_start_request(struct dw_mci *host,
>  
>  	if (cmd->opcode == SD_SWITCH_VOLTAGE) {
>  		/*
> -		 * Databook says to fail after 2ms w/ no response; give an
> -		 * extra jiffy just in case we're about to roll over.
> +		 * Databook says to fail after 2ms w/ no response, but evidence
> +		 * shows that sometimes the cmd11 interrupt takes over 130ms.
> +		 * We'll set to 500ms, plus an extra jiffy just in case jiffies
> +		 * is just about to roll over.
>  		 */
>  		mod_timer(&host->cmd11_timer,
> -			  jiffies + msecs_to_jiffies(2) + 1);
> +			  jiffies + msecs_to_jiffies(500) + 1);
>  	}
>  
>  	if (mrq->stop)
> 

Re: [PATCH 1/3] mmc: dw_mmc: Increase cmd11 timeout to 500ms

[Doug Anderson]

Jaehoon,

On Mon, Apr 6, 2015 at 3:46 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> Hi, Doug.
>
> On 04/04/2015 03:13 AM, Doug Anderson wrote:
>> The Designware databook claims that cmd11 should be finished in 2ms,
>> but my testing showed that not to be the case in some situations.
>> I've seen cmd11 timeouts of up to 130ms (!) during reboot tests.
>> Let's bump the timeout way up so that we're absolutely sure.  CMD11 is
>> only sent during card insertion, so this extra timeout shouldn't be
>> terrible.
>
> Is it h/w problem? Could you explain to me about "some situations"?
> As you said, this timeout only used during card inserting. So, it's not critical..
> But there is much different between 2ms and 500ms(or 130ms).

Very good question, and it makes sense to dig into this...

OK, I think I've got it.  Dang printk bites me again.  I have serial
console enabled and my printouts were actually causing these delays.
With serial console turned off I reliably get ~280us for the interrupt
to fire (tested across SD and WiFi across 137 + 128 + 111 + 127 = 503
reboots)

I think it makes sense to land this patch anyway, but with an updated
description.  I'm happy to repost this or happy if you just want to
update the description when applying.

---

Although the cmd11 interrupt should come within 2ms, that's a very
short time.  Let's increase the timeout to be really sure that we
don't get an accidnetal timeout.  One case in particular this is
useful is if you've got a serial console and printk in just the right
places.  Under that scenario I've seen delays of up to 130ms before
the interrupt fired.

CMD11 is only sent during card insertion, so this extra timeout
shouldn't be terrible.

---

-Doug

Re: [PATCH 1/3] mmc: dw_mmc: Increase cmd11 timeout to 500ms

[Jaehoon Chung]

Hi, Doug.

On 04/07/2015 04:32 AM, Doug Anderson wrote:
> Jaehoon,
> 
> On Mon, Apr 6, 2015 at 3:46 AM, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> Hi, Doug.
>>
>> On 04/04/2015 03:13 AM, Doug Anderson wrote:
>>> The Designware databook claims that cmd11 should be finished in 2ms,
>>> but my testing showed that not to be the case in some situations.
>>> I've seen cmd11 timeouts of up to 130ms (!) during reboot tests.
>>> Let's bump the timeout way up so that we're absolutely sure.  CMD11 is
>>> only sent during card insertion, so this extra timeout shouldn't be
>>> terrible.
>>
>> Is it h/w problem? Could you explain to me about "some situations"?
>> As you said, this timeout only used during card inserting. So, it's not critical..
>> But there is much different between 2ms and 500ms(or 130ms).
> 
> Very good question, and it makes sense to dig into this...
> 
> OK, I think I've got it.  Dang printk bites me again.  I have serial
> console enabled and my printouts were actually causing these delays.
> With serial console turned off I reliably get ~280us for the interrupt
> to fire (tested across SD and WiFi across 137 + 128 + 111 + 127 = 503
> reboots)

Oh..agreed. I also think printouts can be caused the delay.
Thanks for your explanation.

> 
> I think it makes sense to land this patch anyway, but with an updated
> description.  I'm happy to repost this or happy if you just want to
> update the description when applying.

To save your time, when applying, i will do the updating description.

Best Regards,
Jaehoon Chung

> 
> ---
> 
> Although the cmd11 interrupt should come within 2ms, that's a very
> short time.  Let's increase the timeout to be really sure that we
> don't get an accidnetal timeout.  One case in particular this is
> useful is if you've got a serial console and printk in just the right
> places.  Under that scenario I've seen delays of up to 130ms before
> the interrupt fired.
> 
> CMD11 is only sent during card insertion, so this extra timeout
> shouldn't be terrible.
> 
> ---
> 
> -Doug
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>