[PATCH] mmc_core: fix data timeout for SEND_EXT_CSD

[PATCH] mmc_core: fix data timeout for SEND_EXT_CSD

[Adrian Hunter]

Commit 0d3e0460f307e84904968aad6cff97bd688583d8
"MMC: CSD and CID timeout values" inadvertently broke
the timeout for the MMC command SEND_EXT_CSD.

This patch puts it back again.

Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
---
 drivers/mmc/core/mmc_ops.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 9c50e6f..418a270 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -248,12 +248,16 @@ mmc_send_cxd_data(struct mmc_card *card, struct mmc_host *host,
 
 	sg_init_one(&sg, data_buf, len);
 
-	/*
-	 * The spec states that CSR and CID accesses have a timeout
-	 * of 64 clock cycles.
-	 */
-	data.timeout_ns = 0;
-	data.timeout_clks = 64;
+	if (!mmc_host_is_spi(card->host) && opcode == MMC_SEND_EXT_CSD)
+		mmc_set_data_timeout(&data, card);
+	else {
+		/*
+		 * The spec states that CSR and CID accesses have a timeout
+		 * of 64 clock cycles (8 for SPI).
+		 */
+		data.timeout_ns = 0;
+		data.timeout_clks = 64;
+	}
 
 	mmc_wait_for_req(host, &mrq);
 
-- 
1.5.6.3

Re: [PATCH] mmc_core: fix data timeout for SEND_EXT_CSD

[Matt Fleming]

On Tue, Feb 10, 2009 at 05:40:59PM +0200, Adrian Hunter wrote:
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 9c50e6f..418a270 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -248,12 +248,16 @@ mmc_send_cxd_data(struct mmc_card *card, struct mmc_host *host,
>
> 	sg_init_one(&sg, data_buf, len);
>
> -	/*
> -	 * The spec states that CSR and CID accesses have a timeout
> -	 * of 64 clock cycles.
> -	 */
> -	data.timeout_ns = 0;
> -	data.timeout_clks = 64;
> +	if (!mmc_host_is_spi(card->host) && opcode == MMC_SEND_EXT_CSD)
> +		mmc_set_data_timeout(&data, card);
> +	else {
> +		/*
> +		 * The spec states that CSR and CID accesses have a timeout
> +		 * of 64 clock cycles (8 for SPI).
> +		 */
> +		data.timeout_ns = 0;
> +		data.timeout_clks = 64;
> +	}
>
> 	mmc_wait_for_req(host, &mrq);
>

Doh! You're right, I did inadvertently break this. Your fix looks good.

Re: [PATCH] mmc_core: fix data timeout for SEND_EXT_CSD

[Adrian Hunter]

Matt Fleming wrote:
> On Tue, Feb 10, 2009 at 05:40:59PM +0200, Adrian Hunter wrote:
>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>> index 9c50e6f..418a270 100644
>> --- a/drivers/mmc/core/mmc_ops.c
>> +++ b/drivers/mmc/core/mmc_ops.c
>> @@ -248,12 +248,16 @@ mmc_send_cxd_data(struct mmc_card *card, struct mmc_host *host,
>>
>>       sg_init_one(&sg, data_buf, len);
>>
>> -     /*
>> -      * The spec states that CSR and CID accesses have a timeout
>> -      * of 64 clock cycles.
>> -      */
>> -     data.timeout_ns = 0;
>> -     data.timeout_clks = 64;
>> +     if (!mmc_host_is_spi(card->host) && opcode == MMC_SEND_EXT_CSD)
>> +             mmc_set_data_timeout(&data, card);
>> +     else {
>> +             /*
>> +              * The spec states that CSR and CID accesses have a timeout
>> +              * of 64 clock cycles (8 for SPI).
>> +              */
>> +             data.timeout_ns = 0;
>> +             data.timeout_clks = 64;
>> +     }
>>
>>       mmc_wait_for_req(host, &mrq);
>>
> 
> Doh! You're right, I did inadvertently break this. Your fix looks good.
> 

Actually looking at it now, it seems to have an error, because card may be
NULL in the MMC_SEND_CID case.

Should be 

	if (!mmc_host_is_spi(host) && opcode == MMC_SEND_EXT_CSD)

I will resend.

[PATCH] mmc_core: fix data timeout for SEND_EXT_CSD

[Adrian Hunter]

Commit 0d3e0460f307e84904968aad6cff97bd688583d8
"MMC: CSD and CID timeout values" inadvertently broke
the timeout for the MMC command SEND_EXT_CSD.

This patch puts it back again.

Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
---
 drivers/mmc/core/mmc_ops.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 9c50e6f..418a270 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -248,12 +248,16 @@ mmc_send_cxd_data(struct mmc_card *card, struct mmc_host *host,
 
 	sg_init_one(&sg, data_buf, len);
 
-	/*
-	 * The spec states that CSR and CID accesses have a timeout
-	 * of 64 clock cycles.
-	 */
-	data.timeout_ns = 0;
-	data.timeout_clks = 64;
+	if (!mmc_host_is_spi(host) && opcode == MMC_SEND_EXT_CSD)
+		mmc_set_data_timeout(&data, card);
+	else {
+		/*
+		 * The spec states that CSR and CID accesses have a timeout
+		 * of 64 clock cycles (8 for SPI).
+		 */
+		data.timeout_ns = 0;
+		data.timeout_clks = 64;
+	}
 
 	mmc_wait_for_req(host, &mrq);
 
-- 
1.5.6.3

Re: [PATCH] mmc_core: fix data timeout for SEND_EXT_CSD

[Andrew Morton]

On Wed, 11 Feb 2009 16:49:59 +0200 Adrian Hunter <ext-adrian.hunter@nokia.com> wrote:

> Commit 0d3e0460f307e84904968aad6cff97bd688583d8
> "MMC: CSD and CID timeout values" inadvertently broke
> the timeout for the MMC command SEND_EXT_CSD.
> 
> This patch puts it back again.
> 

People will be interested in knowing whether this regression fix should
be backported into 2.6.28.x.  But apart from the word "broke" we have
no hint as to what the impact of the bug is.  Hence we are unable to
make that decision.

Please be more careful about changelogging.  It helps avoid mistakes.


> ---
>  drivers/mmc/core/mmc_ops.c |   16 ++++++++++------
>  1 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 9c50e6f..418a270 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -248,12 +248,16 @@ mmc_send_cxd_data(struct mmc_card *card, struct mmc_host *host,
>  
>  	sg_init_one(&sg, data_buf, len);
>  
> -	/*
> -	 * The spec states that CSR and CID accesses have a timeout
> -	 * of 64 clock cycles.
> -	 */
> -	data.timeout_ns = 0;
> -	data.timeout_clks = 64;
> +	if (!mmc_host_is_spi(host) && opcode == MMC_SEND_EXT_CSD)
> +		mmc_set_data_timeout(&data, card);
> +	else {
> +		/*
> +		 * The spec states that CSR and CID accesses have a timeout
> +		 * of 64 clock cycles (8 for SPI).
> +		 */
> +		data.timeout_ns = 0;
> +		data.timeout_clks = 64;
> +	}
>  
>  	mmc_wait_for_req(host, &mrq);
>  

Re: [PATCH] mmc_core: fix data timeout for SEND_EXT_CSD

[Pierre Ossman]

On Wed, 11 Feb 2009 16:49:59 +0200
Adrian Hunter <ext-adrian.hunter@nokia.com> wrote:

> Commit 0d3e0460f307e84904968aad6cff97bd688583d8
> "MMC: CSD and CID timeout values" inadvertently broke
> the timeout for the MMC command SEND_EXT_CSD.
> 
> This patch puts it back again.
> 
> Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
> ---
>  drivers/mmc/core/mmc_ops.c |   16 ++++++++++------
>  1 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 9c50e6f..418a270 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -248,12 +248,16 @@ mmc_send_cxd_data(struct mmc_card *card, struct mmc_host *host,
>  
>  	sg_init_one(&sg, data_buf, len);
>  
> -	/*
> -	 * The spec states that CSR and CID accesses have a timeout
> -	 * of 64 clock cycles.
> -	 */
> -	data.timeout_ns = 0;
> -	data.timeout_clks = 64;
> +	if (!mmc_host_is_spi(host) && opcode == MMC_SEND_EXT_CSD)
> +		mmc_set_data_timeout(&data, card);
> +	else {
> +		/*
> +		 * The spec states that CSR and CID accesses have a timeout
> +		 * of 64 clock cycles (8 for SPI).
> +		 */
> +		data.timeout_ns = 0;
> +		data.timeout_clks = 64;
> +	}
>  
>  	mmc_wait_for_req(host, &mrq);
>  

I'm confused. Where did the 64 come from in the first place? That
function will not be called for CID/CSD when !SPI. So the way I see it
the code should be:

if ((opcode == MMC_SEND_CSD) || (opcode == (MMC_SEND_CID)) {
	data.timeout_ns = 0;
	data.timeout_clks = 8;
} else {
	mmc_set_data_timeout(&data, card);
}


-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [PATCH] mmc_core: fix data timeout for SEND_EXT_CSD

[Adrian Hunter]

ext Pierre Ossman wrote:
> On Wed, 11 Feb 2009 16:49:59 +0200
> Adrian Hunter <ext-adrian.hunter@nokia.com> wrote:
> 
>> Commit 0d3e0460f307e84904968aad6cff97bd688583d8
>> "MMC: CSD and CID timeout values" inadvertently broke
>> the timeout for the MMC command SEND_EXT_CSD.
>>
>> This patch puts it back again.
>>
>> Signed-off-by: Adrian Hunter <ext-adrian.hunter@nokia.com>
>> ---
>>  drivers/mmc/core/mmc_ops.c |   16 ++++++++++------
>>  1 files changed, 10 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>> index 9c50e6f..418a270 100644
>> --- a/drivers/mmc/core/mmc_ops.c
>> +++ b/drivers/mmc/core/mmc_ops.c
>> @@ -248,12 +248,16 @@ mmc_send_cxd_data(struct mmc_card *card, struct mmc_host *host,
>>
>>       sg_init_one(&sg, data_buf, len);
>>
>> -     /*
>> -      * The spec states that CSR and CID accesses have a timeout
>> -      * of 64 clock cycles.
>> -      */
>> -     data.timeout_ns = 0;
>> -     data.timeout_clks = 64;
>> +     if (!mmc_host_is_spi(host) && opcode == MMC_SEND_EXT_CSD)
>> +             mmc_set_data_timeout(&data, card);
>> +     else {
>> +             /*
>> +              * The spec states that CSR and CID accesses have a timeout
>> +              * of 64 clock cycles (8 for SPI).
>> +              */
>> +             data.timeout_ns = 0;
>> +             data.timeout_clks = 64;
>> +     }
>>
>>       mmc_wait_for_req(host, &mrq);
>>
> 
> I'm confused. Where did the 64 come from in the first place? That
> function will not be called for CID/CSD when !SPI. So the way I see it
> the code should be:
> 
> if ((opcode == MMC_SEND_CSD) || (opcode == (MMC_SEND_CID)) {
>         data.timeout_ns = 0;
>         data.timeout_clks = 8;
> } else {
>         mmc_set_data_timeout(&data, card);
> }

Theoretically yes, it should be 8 not 64 - if all the SPI devices obey
the standard.  As I do not have an SPI device I did not feel comfortable
changing it.  Also 64 clocks is not a long time anyway, so it did not
seem to do any harm.

Re: [PATCH] mmc_core: fix data timeout for SEND_EXT_CSD

[Matt Fleming]

On Thu, Feb 19, 2009 at 09:33:48AM +0200, Adrian Hunter wrote:
> ext Pierre Ossman wrote:
>>
>> I'm confused. Where did the 64 come from in the first place? That
>> function will not be called for CID/CSD when !SPI. So the way I see it
>> the code should be:
>>
>> if ((opcode == MMC_SEND_CSD) || (opcode == (MMC_SEND_CID)) {
>>         data.timeout_ns = 0;
>>         data.timeout_clks = 8;
>> } else {
>>         mmc_set_data_timeout(&data, card);
>> }
>
> Theoretically yes, it should be 8 not 64 - if all the SPI devices obey
> the standard.  As I do not have an SPI device I did not feel comfortable
> changing it.  Also 64 clocks is not a long time anyway, so it did not
> seem to do any harm.

When I wrote the code, I got the 64 clock cycle timeout from the MMC
spec that I was looking at. Unfortunately, I don't have the spec in
front of me at the moment. It is possible that I read the timeout for
the !SPI case, though.

Re: [PATCH] mmc_core: fix data timeout for SEND_EXT_CSD

[Adrian Hunter]

Matt Fleming wrote:
> On Thu, Feb 19, 2009 at 09:33:48AM +0200, Adrian Hunter wrote:
>> ext Pierre Ossman wrote:
>>> I'm confused. Where did the 64 come from in the first place? That
>>> function will not be called for CID/CSD when !SPI. So the way I see it
>>> the code should be:
>>>
>>> if ((opcode == MMC_SEND_CSD) || (opcode == (MMC_SEND_CID)) {
>>>         data.timeout_ns = 0;
>>>         data.timeout_clks = 8;
>>> } else {
>>>         mmc_set_data_timeout(&data, card);
>>> }
>> Theoretically yes, it should be 8 not 64 - if all the SPI devices obey
>> the standard.  As I do not have an SPI device I did not feel comfortable
>> changing it.  Also 64 clocks is not a long time anyway, so it did not
>> seem to do any harm.
> 
> When I wrote the code, I got the 64 clock cycle timeout from the MMC
> spec that I was looking at. Unfortunately, I don't have the spec in
> front of me at the moment. It is possible that I read the timeout for
> the !SPI case, though.

No it is my mistake.  The value is 8 but the unit is 8 clock cycles
i.e. 64 clock cycles.  So my comment was wrong.

Re: [PATCH] mmc_core: fix data timeout for SEND_EXT_CSD

[Pierre Ossman]

On Thu, 19 Feb 2009 14:12:38 +0200
Adrian Hunter <ext-adrian.hunter@nokia.com> wrote:

> 
> No it is my mistake.  The value is 8 but the unit is 8 clock cycles
> i.e. 64 clock cycles.  So my comment was wrong.
> 

That was an annoyingly confusing way of writing a spec.

Could you fix up the comment and the if-clause and we'll have this in
for .29.

Thanks
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.

Re: [PATCH] mmc_core: fix data timeout for SEND_EXT_CSD

[Adrian Hunter]

>From 58e858f20149d279368441055d8ba3ae43a1fc6d Mon Sep 17 00:00:00 2001
From: Adrian Hunter <adrian.hunter@nokia.com>
Date: Tue, 10 Feb 2009 16:32:33 +0200
Subject: [PATCH] mmc_core: fix data timeout for SEND_EXT_CSD

Commit 0d3e0460f307e84904968aad6cff97bd688583d8
"MMC: CSD and CID timeout values" inadvertently broke
the timeout for the MMC command SEND_EXT_CSD.

This patch puts it back again.

Depending on the characteristics of the controller,
this bug may prevent the use of MMC cards.

Signed-off-by: Adrian Hunter <adrian.hunter@nokia.com>
---
 drivers/mmc/core/mmc_ops.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 9c50e6f..34ce270 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -248,12 +248,15 @@ mmc_send_cxd_data(struct mmc_card *card, struct mmc_host *host,
 
 	sg_init_one(&sg, data_buf, len);
 
-	/*
-	 * The spec states that CSR and CID accesses have a timeout
-	 * of 64 clock cycles.
-	 */
-	data.timeout_ns = 0;
-	data.timeout_clks = 64;
+	if (opcode == MMC_SEND_CSD || opcode == MMC_SEND_CID) {
+		/*
+		 * The spec states that CSR and CID accesses have a timeout
+		 * of 64 clock cycles.
+		 */
+		data.timeout_ns = 0;
+		data.timeout_clks = 64;
+	} else
+		mmc_set_data_timeout(&data, card);
 
 	mmc_wait_for_req(host, &mrq);
 
-- 
1.5.6.3

Re: [PATCH] mmc_core: fix data timeout for SEND_EXT_CSD

[Pierre Ossman]

On Tue, 03 Mar 2009 13:48:36 +0200
Adrian Hunter <adrian.hunter@nokia.com> wrote:

> From 58e858f20149d279368441055d8ba3ae43a1fc6d Mon Sep 17 00:00:00 2001
> From: Adrian Hunter <adrian.hunter@nokia.com>
> Date: Tue, 10 Feb 2009 16:32:33 +0200
> Subject: [PATCH] mmc_core: fix data timeout for SEND_EXT_CSD
> 
> Commit 0d3e0460f307e84904968aad6cff97bd688583d8
> "MMC: CSD and CID timeout values" inadvertently broke
> the timeout for the MMC command SEND_EXT_CSD.
> 
> This patch puts it back again.
> 
> Depending on the characteristics of the controller,
> this bug may prevent the use of MMC cards.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@nokia.com>
> ---

Queued, thanks.

Rgds
-- 
     -- Pierre Ossman

  Linux kernel, MMC maintainer        http://www.kernel.org
  rdesktop, core developer          http://www.rdesktop.org

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.