[PATCH] Move the judgement of timeout into mmc_switch_status_error()

[PATCH] Move the judgement of timeout into mmc_switch_status_error()

[장영진]

>From bc1e491b29d9199f16c6e002a69f9377677b93af Mon Sep 17 00:00:00 2001

From: "yj84.jang" <yj84.jang@samsung.com>

Date: Wed, 16 Dec 2015 08:52:47 +0900

Subject: [PATCH] Move the judgement of timeout into
mmc_switch_status_error()

 

__mmc_switch() is possible to misjudge error.

 

Although jiffies was overed timeout, 

the chance about "R1_CURRENT_STATE(status) != R1_STATE_PRG" is exists. 

In other words, Successful completion of CMD6 with timeout error.

 

Kernel expects failure of __mmc_switch,

but card is completed CMD6 in this case.

It makes mismatch status between mmc subsystem and mmc card.

 

Especially, in case of partition switch,

If timeout occurs with normal status of CMD13 response unfortunatley, 

"main_md->curr" will not be set, even card switched successfully.

 

Checking state of mmc card is needed when judging an timeout error, 

if it is just timeout or timeout with error.

 

Signed-off-by: yj84.jang <yj84.jang@samsung.com>

---

drivers/mmc/core/mmc_ops.c |    5 +++--

1 file changed, 3 insertions(+), 2 deletions(-)

 

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c

index 1f44426..033f475 100644

--- a/drivers/mmc/core/mmc_ops.c

+++ b/drivers/mmc/core/mmc_ops.c

@@ -456,6 +456,8 @@ int mmc_switch_status_error(struct mmc_host *host, u32
status)

                    if (status & R1_SPI_ILLEGAL_COMMAND)

                               return -EBADMSG;

         } else {

+                   if (R1_CURRENT_STATE(status) == R1_STATE_PRG)

+                              return -ETIMEDOUT;

                    if (status & 0xFDFFA000)

                               pr_warn("%s: unexpected status %#x after
switch\n",

                                         mmc_hostname(host), status);

@@ -568,8 +570,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8
index, u8 value,

                    if (time_after(jiffies, timeout)) {

                               pr_err("%s: Card stuck in programming state!
%s\n",

                                         mmc_hostname(host), __func__);

-                               err = -ETIMEDOUT;

-                               goto out;

+                              break;

                    }

         } while (R1_CURRENT_STATE(status) == R1_STATE_PRG);

 

-- 

1.7.9.5

 

Re: [PATCH] Move the judgement of timeout into mmc_switch_status_error()

[Jaehoon Chung]

Hi,

Before send the patch at mailing list, you need to make the patch as mainline style.
Maybe..maintainer should not review this patch.

Refer to "Linux kernel coding style" and "submitting Patches".
Also set  your email environment. (Documentation/email-clients.txt)

Best Regards,
Jaehoon Chung

On 12/24/2015 06:13 PM, 장영진 wrote:
>>From bc1e491b29d9199f16c6e002a69f9377677b93af Mon Sep 17 00:00:00 2001
> 
> From: "yj84.jang" <yj84.jang@samsung.com>
> 
> Date: Wed, 16 Dec 2015 08:52:47 +0900
> 
> Subject: [PATCH] Move the judgement of timeout into
> mmc_switch_status_error()
> 
>  
> 
> __mmc_switch() is possible to misjudge error.
> 
>  
> 
> Although jiffies was overed timeout, 
> 
> the chance about "R1_CURRENT_STATE(status) != R1_STATE_PRG" is exists. 
> 
> In other words, Successful completion of CMD6 with timeout error.
> 
>  
> 
> Kernel expects failure of __mmc_switch,
> 
> but card is completed CMD6 in this case.
> 
> It makes mismatch status between mmc subsystem and mmc card.
> 
>  
> 
> Especially, in case of partition switch,
> 
> If timeout occurs with normal status of CMD13 response unfortunatley, 
> 
> "main_md->curr" will not be set, even card switched successfully.
> 
>  
> 
> Checking state of mmc card is needed when judging an timeout error, 
> 
> if it is just timeout or timeout with error.
> 
>  
> 
> Signed-off-by: yj84.jang <yj84.jang@samsung.com>
> 
> ---
> 
> drivers/mmc/core/mmc_ops.c |    5 +++--
> 
> 1 file changed, 3 insertions(+), 2 deletions(-)
> 
>  
> 
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> 
> index 1f44426..033f475 100644
> 
> --- a/drivers/mmc/core/mmc_ops.c
> 
> +++ b/drivers/mmc/core/mmc_ops.c
> 
> @@ -456,6 +456,8 @@ int mmc_switch_status_error(struct mmc_host *host, u32
> status)
> 
>                     if (status & R1_SPI_ILLEGAL_COMMAND)
> 
>                                return -EBADMSG;
> 
>          } else {
> 
> +                   if (R1_CURRENT_STATE(status) == R1_STATE_PRG)
> 
> +                              return -ETIMEDOUT;
> 
>                     if (status & 0xFDFFA000)
> 
>                                pr_warn("%s: unexpected status %#x after
> switch\n",
> 
>                                          mmc_hostname(host), status);
> 
> @@ -568,8 +570,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8
> index, u8 value,
> 
>                     if (time_after(jiffies, timeout)) {
> 
>                                pr_err("%s: Card stuck in programming state!
> %s\n",
> 
>                                          mmc_hostname(host), __func__);
> 
> -                               err = -ETIMEDOUT;
> 
> -                               goto out;
> 
> +                              break;
> 
>                     }
> 
>          } while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
> 
>  
> 

Re: [PATCH] Move the judgement of timeout into mmc_switch_status_error()

[Jaehoon Chung]

Hi,

NAK.

Maybe..It seems that this is not the patch based on latest kernel..
Which kernel version did you use? Plz, check the latest kernel.

Best Regards,
Jaehoon Chung

On 12/24/2015 06:24 PM, Jaehoon Chung wrote:
> Hi,
> 
> Before send the patch at mailing list, you need to make the patch as mainline style.
> Maybe..maintainer should not review this patch.
> 
> Refer to "Linux kernel coding style" and "submitting Patches".
> Also set  your email environment. (Documentation/email-clients.txt)
> 
> Best Regards,
> Jaehoon Chung
> 
> On 12/24/2015 06:13 PM, 장영진 wrote:
>> >From bc1e491b29d9199f16c6e002a69f9377677b93af Mon Sep 17 00:00:00 2001
>>
>> From: "yj84.jang" <yj84.jang@samsung.com>
>>
>> Date: Wed, 16 Dec 2015 08:52:47 +0900
>>
>> Subject: [PATCH] Move the judgement of timeout into
>> mmc_switch_status_error()
>>
>>  
>>
>> __mmc_switch() is possible to misjudge error.
>>
>>  
>>
>> Although jiffies was overed timeout, 
>>
>> the chance about "R1_CURRENT_STATE(status) != R1_STATE_PRG" is exists. 
>>
>> In other words, Successful completion of CMD6 with timeout error.
>>
>>  
>>
>> Kernel expects failure of __mmc_switch,
>>
>> but card is completed CMD6 in this case.
>>
>> It makes mismatch status between mmc subsystem and mmc card.
>>
>>  
>>
>> Especially, in case of partition switch,
>>
>> If timeout occurs with normal status of CMD13 response unfortunatley, 
>>
>> "main_md->curr" will not be set, even card switched successfully.
>>
>>  
>>
>> Checking state of mmc card is needed when judging an timeout error, 
>>
>> if it is just timeout or timeout with error.
>>
>>  
>>
>> Signed-off-by: yj84.jang <yj84.jang@samsung.com>
>>
>> ---
>>
>> drivers/mmc/core/mmc_ops.c |    5 +++--
>>
>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>
>>  
>>
>> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
>>
>> index 1f44426..033f475 100644
>>
>> --- a/drivers/mmc/core/mmc_ops.c
>>
>> +++ b/drivers/mmc/core/mmc_ops.c
>>
>> @@ -456,6 +456,8 @@ int mmc_switch_status_error(struct mmc_host *host, u32
>> status)
>>
>>                     if (status & R1_SPI_ILLEGAL_COMMAND)
>>
>>                                return -EBADMSG;
>>
>>          } else {
>>
>> +                   if (R1_CURRENT_STATE(status) == R1_STATE_PRG)
>>
>> +                              return -ETIMEDOUT;
>>
>>                     if (status & 0xFDFFA000)
>>
>>                                pr_warn("%s: unexpected status %#x after
>> switch\n",
>>
>>                                          mmc_hostname(host), status);
>>
>> @@ -568,8 +570,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8
>> index, u8 value,
>>
>>                     if (time_after(jiffies, timeout)) {
>>
>>                                pr_err("%s: Card stuck in programming state!
>> %s\n",
>>
>>                                          mmc_hostname(host), __func__);
>>
>> -                               err = -ETIMEDOUT;
>>
>> -                               goto out;
>>
>> +                              break;
>>
>>                     }
>>
>>          } while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
>>
>>  
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

[PATCH] Move the judgement of timeout into mmc_switch_status_error()

[Youngjin Jang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 1865 bytes --]

__mmc_switch() is possible to misjudge error.

Although jiffies was overed timeout,
the chance about "R1_CURRENT_STATE(status) != R1_STATE_PRG" is exists.
In other words, Successful completion of CMD6 with timeout error.

Kernel expects failure of __mmc_switch,
but card is completed CMD6 in this case.
It makes mismatch status between mmc subsystem and mmc card.

Especially, in case of partition switch,
If timeout occurs with normal status of CMD13 response unfortunatley,
"main_md->curr" will not be set, even card switched successfully.

Checking state of mmc card is needed when judging an timeout error,
if it is just timeout or timeout with error.

Reviewed-by: Jungseung Lee <js07.lee@samsung.com>
Signed-off-by: Youngjin Jang <yj84.jang@samsung.com>
---
 drivers/mmc/core/mmc_ops.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
index 1f44426..033f475 100644
--- a/drivers/mmc/core/mmc_ops.c
+++ b/drivers/mmc/core/mmc_ops.c
@@ -456,6 +456,8 @@ int mmc_switch_status_error(struct mmc_host *host, u32 status)
 		if (status & R1_SPI_ILLEGAL_COMMAND)
 			return -EBADMSG;
 	} else {
+		if (R1_CURRENT_STATE(status) == R1_STATE_PRG)
+			return -ETIMEDOUT;
 		if (status & 0xFDFFA000)
 			pr_warn("%s: unexpected status %#x after switch\n",
 				mmc_hostname(host), status);
@@ -568,8 +570,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
 		if (time_after(jiffies, timeout)) {
 			pr_err("%s: Card stuck in programming state! %s\n",
 				mmc_hostname(host), __func__);
-			err = -ETIMEDOUT;
-			goto out;
+			break;
 		}
 	} while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
 
-- 
1.7.9.5

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] Move the judgement of timeout into mmc_switch_status_error()

[Jaehoon Chung]

Hi,

Did you read my comment? Which kernel version did you use?

I think your patch doesn't need on mainline kernel.
If you want to apply the patch, work on the latest kernel.

https://patchwork.kernel.org/patch/7719811/

Author:     Chaotian Jing <chaotian.jing@mediatek.com>
AuthorDate: Mon Nov 30 09:27:30 2015 +0800
Commit:     Ulf Hansson <ulf.hansson@linaro.org>
CommitDate: Tue Dec 22 11:32:18 2015 +0100

    mmc: core: fix __mmc_switch timeout caused by preempt

    there is a time window between __mmc_send_status() and time_afer(),
    on some eMMC chip, the timeout_ms is only 10ms, if this thread was
    scheduled out during this period, then, even card has already changes
    to transfer state by the result of CMD13, this part of code also treat
    it to timeout error.
    So, need calculate timeout first, then call __mmc_send_status(), if
    already timeout and card still in programing state, then treat it to
    the real timeout error.

    Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
    Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>


Best Regards,
Jaehoon Chung

On 12/28/2015 11:07 AM, Youngjin Jang wrote:
> __mmc_switch() is possible to misjudge error.
> 
> Although jiffies was overed timeout,
> the chance about "R1_CURRENT_STATE(status) != R1_STATE_PRG" is exists.
> In other words, Successful completion of CMD6 with timeout error.
> 
> Kernel expects failure of __mmc_switch,
> but card is completed CMD6 in this case.
> It makes mismatch status between mmc subsystem and mmc card.
> 
> Especially, in case of partition switch,
> If timeout occurs with normal status of CMD13 response unfortunatley,
> "main_md->curr" will not be set, even card switched successfully.

main_md->curr is right? main_md->part_curr?

> 
> Checking state of mmc card is needed when judging an timeout error,
> if it is just timeout or timeout with error.
> 
> Reviewed-by: Jungseung Lee <js07.lee@samsung.com>
> Signed-off-by: Youngjin Jang <yj84.jang@samsung.com>
> ---
>  drivers/mmc/core/mmc_ops.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc_ops.c b/drivers/mmc/core/mmc_ops.c
> index 1f44426..033f475 100644
> --- a/drivers/mmc/core/mmc_ops.c
> +++ b/drivers/mmc/core/mmc_ops.c
> @@ -456,6 +456,8 @@ int mmc_switch_status_error(struct mmc_host *host, u32 status)
>  		if (status & R1_SPI_ILLEGAL_COMMAND)
>  			return -EBADMSG;
>  	} else {
> +		if (R1_CURRENT_STATE(status) == R1_STATE_PRG)
> +			return -ETIMEDOUT;
>  		if (status & 0xFDFFA000)
>  			pr_warn("%s: unexpected status %#x after switch\n",
>  				mmc_hostname(host), status);
> @@ -568,8 +570,7 @@ int __mmc_switch(struct mmc_card *card, u8 set, u8 index, u8 value,
>  		if (time_after(jiffies, timeout)) {
>  			pr_err("%s: Card stuck in programming state! %s\n",
>  				mmc_hostname(host), __func__);
> -			err = -ETIMEDOUT;
> -			goto out;
> +			break;
>  		}
>  	} while (R1_CURRENT_STATE(status) == R1_STATE_PRG);
>  
>