linux-kernel - Re: [PATCH] sdhci: always use max timeout for xfers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 25 Feb 2011 10:12:35 -0800
From:	Philip Rakity <prakity@...vell.com>
To:	Wolfram Sang <w.sang@...gutronix.de>
CC:	"linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Jae hoon Chung <jh80.chung@...il.com>,
	Chuanxiao Dong <chuanxiao.dong@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] sdhci: always use max timeout for xfers


On Feb 25, 2011, at 10:02 AM, Wolfram Sang wrote:

> Hi Philip,
> 
> On Fri, Feb 25, 2011 at 09:54:35AM -0800, Philip Rakity wrote:
>> Rather then special case busy etc .. lets just use the max value.  
> 
> OK.
> 
>> 
>> Did not remove BROKEN_TIMEOUT QUIRK so existing code will compile
>> we can remove this once existing platform drivers delete usage and get
>> quirk back.
> 
> If we wait for that, we'll probably wait till eternity ;) I'd vote that
> removing the quirk should be part of the patch.

I concur (see below)

> 
>> 
>> Patch starts after ====
>> =====
> 
> The usual nomenclature is that such comments simply go between '---' and the
> diffstat. Most tools are prepared for this...
> 
>> The card/host controller may sometimes return a value that is
>> too low and cause the h/w to timeout a transfer that would have
>> worked.  Using the maximum value avoids this.
>> 
>> Signed-off-by: Philip Rakity <prakity@...vell.com>
>> ---
> 
> ... to handle them here.
> 
>> drivers/mmc/host/sdhci.c |   48 ++++-----------------------------------------
>> 1 files changed, 5 insertions(+), 43 deletions(-)
>> 
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 655617c..dd99da8 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -592,53 +592,15 @@ static void sdhci_adma_table_post(struct sdhci_host *host,
>> 		data->sg_len, direction);
>> }
>> 
>> -static u8 sdhci_calc_timeout(struct sdhci_host *host, struct mmc_data *data)
>> +static inline u8 sdhci_calc_timeout(void)
>> {
>> -	u8 count;
>> -	unsigned target_timeout, current_timeout;
>> -
>> 	/*
>> -	 * If the host controller provides us with an incorrect timeout
>> -	 * value, just skip the check and use 0xE.  The hardware may take
>> +	 * The host controller/card can provide us with an incorrect timeout
>> +	 * value, just use the maximum value 0xE.  The hardware may take
>> 	 * longer to time out, but that's much better than having a too-short
>> 	 * timeout value.
>> 	 */
>> -	if (host->quirks & SDHCI_QUIRK_BROKEN_TIMEOUT_VAL)
>> -		return 0xE;
>> -
>> -	/* timeout in us */
>> -	target_timeout = data->timeout_ns / 1000 +
>> -		data->timeout_clks / host->clock;
>> -
>> -	if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
>> -		host->timeout_clk = host->clock / 1000;
> 
> This quirk could go then as well?

I am all for that -- did not want to touch other drivers but will remove for sdhci-pxa if
patch is okay.  

> 
>> -
>> -	/*
>> -	 * Figure out needed cycles.
>> -	 * We do this in steps in order to fit inside a 32 bit int.
>> -	 * The first step is the minimum timeout, which will have a
>> -	 * minimum resolution of 6 bits:
>> -	 * (1) 2^13*1000 > 2^22,
>> -	 * (2) host->timeout_clk < 2^16
>> -	 *     =>
>> -	 *     (1) / (2) > 2^6
>> -	 */
>> -	count = 0;
>> -	current_timeout = (1 << 13) * 1000 / host->timeout_clk;
>> -	while (current_timeout < target_timeout) {
>> -		count++;
>> -		current_timeout <<= 1;
>> -		if (count >= 0xF)
>> -			break;
>> -	}
>> -
>> -	if (count >= 0xF) {
>> -		printk(KERN_WARNING "%s: Too large timeout requested!\n",
>> -			mmc_hostname(host->mmc));
>> -		count = 0xE;
>> -	}
>> -
>> -	return count;
>> +	return 0xE;
> 
> Why don't you remove the function entirely?

better to rename it --- to set_maximum_timeout  since a little clearer.
left the old name for historical reasons -- if no need I will change it

> 
>> }
>> 
>> static void sdhci_set_transfer_irqs(struct sdhci_host *host)
>> @@ -671,7 +633,7 @@ static void sdhci_prepare_data(struct sdhci_host *host, struct mmc_data *data)
>> 	host->data = data;
>> 	host->data_early = 0;
>> 
>> -	count = sdhci_calc_timeout(host, data);
>> +	count = sdhci_calc_timeout();
>> 	sdhci_writeb(host, count, SDHCI_TIMEOUT_CONTROL);
>> 
>> 	if (host->flags & (SDHCI_USE_SDMA | SDHCI_USE_ADMA))
> 
> Thanks,
> 
>   Wolfram
> 
> -- 
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/