Re: [PATCH] sdhci: fix the fake timeout bug

[Adrian Hunter]

On 30/11/18 4:13 PM, Du, Alek wrote:
> On Fri, 30 Nov 2018 11:19:03 +0200
> Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
> 
>> Commands and resets are under spin lock, so no possibility for
>> preemption, and certainly a few microseconds isn't going to make any
>> difference to these timeouts, so I don't see how this patch could
>> help.
> 
> Thanks for your review.
> 
> I believe my case the scheduling out delay sometime can reach 10~20
> milliseconds... This may be due to the hypervisor...

So you are saying this only happens under virtualization?

> 
> Please look at the sdhci_enable_clk() below, there is a window in clock
> stabilization check. The first check is to check status register, the
> second check is to check if time passed. That's why I can capture a
> case that after time passed, the actually clock control register
> indicated that clock is stable. So the error handling is wrong...

Sure, but "Internal clock never stabilised." is not one of the  errors you
listed.

> 
> Also the sdhci_send_command commands is not in spin lock There is a
> windows between mod_timer and real command send...

What code path does not have a spin lock?

> 
> Thanks,
> Alek
> 
>>
>> I recently sent a patch for GLK laptops that had BIOS issues:
>>
>> 	https://marc.info/?l=linux-mmc&m=154270005901609
>>
>> And also some improvements to error handling:
>>
>> 	https://marc.info/?l=linux-mmc&m=154229013900437
>>
>> If those don't help, please share more details of the actual problem.
> 
> Sorry, I can't see how this patches could fix my problem.
> 
> 
> 
>>
>>>
>>> Signed-off-by: Alek Du <alek.du@xxxxxxxxx>
>>> ---
>>>  drivers/mmc/host/sdhci.c | 19 ++++++++++++++++---
>>>  1 file changed, 16 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> index 99bdae53fa2e..f88c49fc574e 100644
>>> --- a/drivers/mmc/host/sdhci.c
>>> +++ b/drivers/mmc/host/sdhci.c
>>> @@ -218,12 +218,17 @@ void sdhci_reset(struct sdhci_host *host, u8
>>> mask) /* hw clears the bit when it's done */
>>>  	while (sdhci_readb(host, SDHCI_SOFTWARE_RESET) & mask) {
>>>  		if (ktime_after(ktime_get(), timeout)) {
>>> +			/* check it again, since there is a window
>>> between
>>> +			   bit check and time check */
>>> +			if (!(sdhci_readb(host,
>>> SDHCI_SOFTWARE_RESET) & mask))
>>> +				break;
>>>  			pr_err("%s: Reset 0x%x never completed.\n",
>>>  				mmc_hostname(host->mmc),
>>> (int)mask); sdhci_dumpregs(host);
>>>  			return;
>>> +		} else {
>>> +			udelay(10);
>>>  		}
>>> -		udelay(10);
>>>  	}
>>>  }
>>>  EXPORT_SYMBOL_GPL(sdhci_reset);
>>> @@ -1395,9 +1400,10 @@ void sdhci_send_command(struct sdhci_host
>>> *host, struct mmc_command *cmd) timeout +=
>>> DIV_ROUND_UP(cmd->busy_timeout, 1000) * HZ + HZ; else
>>>  		timeout += 10 * HZ;
>>> -	sdhci_mod_timer(host, cmd->mrq, timeout);
>>>  
>>>  	sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags),
>>> SDHCI_COMMAND);
>>> +	/* setup timer after command to avoid fake timeout */
>>> +	sdhci_mod_timer(host, cmd->mrq, timeout);
>>>  }
>>>  EXPORT_SYMBOL_GPL(sdhci_send_command);
>>>  
>>> @@ -1611,12 +1617,19 @@ void sdhci_enable_clk(struct sdhci_host
>>> *host, u16 clk) while (!((clk = sdhci_readw(host,
>>> SDHCI_CLOCK_CONTROL)) & SDHCI_CLOCK_INT_STABLE)) {
>>>  		if (ktime_after(ktime_get(), timeout)) {
>>> +			/* check it again since there is a window
>>> between
>>> +			   status check and time check */
>>> +			if ((clk = sdhci_readw(host,
>>> SDHCI_CLOCK_CONTROL))
>>> +					& SDHCI_CLOCK_INT_STABLE)
>>> +				break;
>>>  			pr_err("%s: Internal clock never
>>> stabilised.\n", mmc_hostname(host->mmc));
>>>  			sdhci_dumpregs(host);
>>>  			return;
>>>  		}
>>> -		udelay(10);
>>> +		else {
>>> +			udelay(10);
>>> +		}
>>>  	}
>>>  
>>>  	clk |= SDHCI_CLOCK_CARD_EN;
>>>   
>>
> 
>