[PATCH] mmc: card: increase wait time

[PATCH] mmc: card: increase wait time

[Huijin Park]

Some eMMC products keep busy time a little longer intermittently.
(eg. prepare free blocks, garbage collection...)
In this case, we recommend to wait a little longer than fail.

Signed-off-by: Huijin Park <huijin.park@samsung.com>
---
 drivers/mmc/core/block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index ea80ff4..b5b0fe6 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -584,7 +584,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
 		 * Ensure RPMB command has completed by polling CMD13
 		 * "Send Status".
 		 */
-		err = ioctl_rpmb_card_status_poll(card, &status, 5);
+		err = ioctl_rpmb_card_status_poll(card, &status, 50);
 		if (err)
 			dev_err(mmc_dev(card->host),
 					"%s: Card Status=0x%08X, error %d\n",
-- 
1.9.1

RE: [PATCH] mmc: card: increase wait time

[Avri Altman]

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
> owner@vger.kernel.org] On Behalf Of Huijin Park
> Sent: Tuesday, November 14, 2017 4:48 AM
> To: ulf.hansson@linaro.org
> Cc: linux-mmc@vger.kernel.org; bbang.huijin.park@gmail.com; Huijin Park
> <huijin.park@samsung.com>
> Subject: [PATCH] mmc: card: increase wait time
> 
> Some eMMC products keep busy time a little longer intermittently.
> (eg. prepare free blocks, garbage collection...) In this case, we recommend to
> wait a little longer than fail.
> 
> Signed-off-by: Huijin Park <huijin.park@samsung.com>
> ---
>  drivers/mmc/core/block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
> ea80ff4..b5b0fe6 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -584,7 +584,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card
> *card, struct mmc_blk_data *md,
>  		 * Ensure RPMB command has completed by polling CMD13
>  		 * "Send Status".
>  		 */
> -		err = ioctl_rpmb_card_status_poll(card, &status, 5);
> +		err = ioctl_rpmb_card_status_poll(card, &status, 50);
 Is this up to 1.25 sec or am I counting wrong?
5[msec] x 5 [__mmc_send_status] x 50 

>  		if (err)
>  			dev_err(mmc_dev(card->host),
>  					"%s: Card Status=0x%08X, error
> %d\n",
> --
> 1.9.1
> 
> --
> 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

Re: [PATCH] mmc: card: increase wait time

[Park Huijin]

The" [__mmc_send_status] 5" doesn't wait 5[msec].
So it wait up to 5[msec] x 50.

Thanks.

2017-11-14 21:44 GMT+09:00 Avri Altman <Avri.Altman@wdc.com>:
>
>
>> -----Original Message-----
>> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-
>> owner@vger.kernel.org] On Behalf Of Huijin Park
>> Sent: Tuesday, November 14, 2017 4:48 AM
>> To: ulf.hansson@linaro.org
>> Cc: linux-mmc@vger.kernel.org; bbang.huijin.park@gmail.com; Huijin Park
>> <huijin.park@samsung.com>
>> Subject: [PATCH] mmc: card: increase wait time
>>
>> Some eMMC products keep busy time a little longer intermittently.
>> (eg. prepare free blocks, garbage collection...) In this case, we recommend to
>> wait a little longer than fail.
>>
>> Signed-off-by: Huijin Park <huijin.park@samsung.com>
>> ---
>>  drivers/mmc/core/block.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index
>> ea80ff4..b5b0fe6 100644
>> --- a/drivers/mmc/core/block.c
>> +++ b/drivers/mmc/core/block.c
>> @@ -584,7 +584,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card
>> *card, struct mmc_blk_data *md,
>>                * Ensure RPMB command has completed by polling CMD13
>>                * "Send Status".
>>                */
>> -             err = ioctl_rpmb_card_status_poll(card, &status, 5);
>> +             err = ioctl_rpmb_card_status_poll(card, &status, 50);
>  Is this up to 1.25 sec or am I counting wrong?
> 5[msec] x 5 [__mmc_send_status] x 50
>
>>               if (err)
>>                       dev_err(mmc_dev(card->host),
>>                                       "%s: Card Status=0x%08X, error
>> %d\n",
>> --
>> 1.9.1
>>
>> --
>> 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

Re: [PATCH] mmc: card: increase wait time

[Ulf Hansson]

On 14 November 2017 at 03:48, Huijin Park <huijin.park@samsung.com> wrote:
> Some eMMC products keep busy time a little longer intermittently.
> (eg. prepare free blocks, garbage collection...)
> In this case, we recommend to wait a little longer than fail.

Is this an rpmb specific problem, because the code seems only to change that.

Is it write and read or only write?

>
> Signed-off-by: Huijin Park <huijin.park@samsung.com>
> ---
>  drivers/mmc/core/block.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> index ea80ff4..b5b0fe6 100644
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -584,7 +584,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>                  * Ensure RPMB command has completed by polling CMD13
>                  * "Send Status".
>                  */
> -               err = ioctl_rpmb_card_status_poll(card, &status, 5);
> +               err = ioctl_rpmb_card_status_poll(card, &status, 50);

First, you are changing "re-tries" not a timeout value, which would
seem like a better approach.

Second, it seems like we should remove the
ioctl_rpmb_card_status_poll() function altogether and instead make use
of the more generic card_busy_detect() function here.

Then if card_busy_detect() needs to be adopted to suite rpmb, then we
should do that instead.

>                 if (err)
>                         dev_err(mmc_dev(card->host),
>                                         "%s: Card Status=0x%08X, error %d\n",
> --
> 1.9.1
>

Kind regards
Uffe

Re: [PATCH] mmc: card: increase wait time

[Park Huijin]

2017-11-15 18:13 GMT+09:00 Ulf Hansson <ulf.hansson@linaro.org>:
>
> On 14 November 2017 at 03:48, Huijin Park <huijin.park@samsung.com> wrote:
> > Some eMMC products keep busy time a little longer intermittently.
> > (eg. prepare free blocks, garbage collection...)
> > In this case, we recommend to wait a little longer than fail.
>
> Is this an rpmb specific problem, because the code seems only to change that.
>
> Is it write and read or only write?
Yes. It's rpmb specific problem.
RPMB R/W use same flow. but only 'write' case makes the problem(long
busy state) sometimes.
When I test it, the 5~25ms is not enough although there is not max
busy time in the spec.

>
> >
> > Signed-off-by: Huijin Park <huijin.park@samsung.com>
> > ---
> >  drivers/mmc/core/block.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> > index ea80ff4..b5b0fe6 100644
> > --- a/drivers/mmc/core/block.c
> > +++ b/drivers/mmc/core/block.c
> > @@ -584,7 +584,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
> >                  * Ensure RPMB command has completed by polling CMD13
> >                  * "Send Status".
> >                  */
> > -               err = ioctl_rpmb_card_status_poll(card, &status, 5);
> > +               err = ioctl_rpmb_card_status_poll(card, &status, 50);
>
> First, you are changing "re-tries" not a timeout value, which would
> seem like a better approach.
I will modify it. Thanks.

>
> Second, it seems like we should remove the
> ioctl_rpmb_card_status_poll() function altogether and instead make use
> of the more generic card_busy_detect() function here.
>
> Then if card_busy_detect() needs to be adopted to suite rpmb, then we
> should do that instead.
I agree that. But the two function are different a little.
'ioctl_rpmb_card_status_poll()' has interval(1~5ms) that prevent to
poll too frequently.
So I suggest to improve 'card_busy_detect()' about interval time in
another patch.
Then the 'ioctl_rpmb_card_status_poll()' can replaced by 'card_busy_detect()'

>
> >                 if (err)
> >                         dev_err(mmc_dev(card->host),
> >                                         "%s: Card Status=0x%08X, error %d\n",
> > --
> > 1.9.1
> >
>
> Kind regards
> Uffe

Re: [PATCH] mmc: card: increase wait time

[Ulf Hansson]

On 20 November 2017 at 05:50, Park Huijin <bbang.huijin.park@gmail.com> wrote:
> 2017-11-15 18:13 GMT+09:00 Ulf Hansson <ulf.hansson@linaro.org>:
>>
>> On 14 November 2017 at 03:48, Huijin Park <huijin.park@samsung.com> wrote:
>> > Some eMMC products keep busy time a little longer intermittently.
>> > (eg. prepare free blocks, garbage collection...)
>> > In this case, we recommend to wait a little longer than fail.
>>
>> Is this an rpmb specific problem, because the code seems only to change that.
>>
>> Is it write and read or only write?
> Yes. It's rpmb specific problem.
> RPMB R/W use same flow. but only 'write' case makes the problem(long
> busy state) sometimes.
> When I test it, the 5~25ms is not enough although there is not max
> busy time in the spec.

As a matter of fact this isn't specific to RPMB writes at all but to
all data writes. That's why we call card_busy_detect() after a data
write.

>
>>
>> >
>> > Signed-off-by: Huijin Park <huijin.park@samsung.com>
>> > ---
>> >  drivers/mmc/core/block.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
>> > index ea80ff4..b5b0fe6 100644
>> > --- a/drivers/mmc/core/block.c
>> > +++ b/drivers/mmc/core/block.c
>> > @@ -584,7 +584,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>> >                  * Ensure RPMB command has completed by polling CMD13
>> >                  * "Send Status".
>> >                  */
>> > -               err = ioctl_rpmb_card_status_poll(card, &status, 5);
>> > +               err = ioctl_rpmb_card_status_poll(card, &status, 50);
>>
>> First, you are changing "re-tries" not a timeout value, which would
>> seem like a better approach.
> I will modify it. Thanks.
>
>>
>> Second, it seems like we should remove the
>> ioctl_rpmb_card_status_poll() function altogether and instead make use
>> of the more generic card_busy_detect() function here.
>>
>> Then if card_busy_detect() needs to be adopted to suite rpmb, then we
>> should do that instead.
> I agree that. But the two function are different a little.
> 'ioctl_rpmb_card_status_poll()' has interval(1~5ms) that prevent to
> poll too frequently.

Well, polling too frequently is then already a problem for the general
data writes. Isn't it?

Or you think RPMB may differ a bit in this regards?

> So I suggest to improve 'card_busy_detect()' about interval time in
> another patch.

Okay.

However, I don't think it's a stopper, you should be able to convert
RPMB to use card_busy_detect() as is. Then you can fix the polling
interval time on top.

What you do need to fix is that card_busy_detect() takes an struct
request *req as in-parameter, and uses it to print the disk_name. I
suggest to change the in-parameter to be the disk_name instead, that
way it would work for RPMB as well.

> Then the 'ioctl_rpmb_card_status_poll()' can replaced by 'card_busy_detect()'

Great!

[...]

Kind regards
Uffe

Re: [PATCH] mmc: card: increase wait time

[Shawn Lin]

Hi Ulf,

On 2017/11/21 17:02, Ulf Hansson wrote:
> On 20 November 2017 at 05:50, Park Huijin <bbang.huijin.park@gmail.com> wrote:
>> 2017-11-15 18:13 GMT+09:00 Ulf Hansson <ulf.hansson@linaro.org>:
>>>
>>> On 14 November 2017 at 03:48, Huijin Park <huijin.park@samsung.com> wrote:
>>>> Some eMMC products keep busy time a little longer intermittently.
>>>> (eg. prepare free blocks, garbage collection...)
>>>> In this case, we recommend to wait a little longer than fail.
>>>
>>> Is this an rpmb specific problem, because the code seems only to change that.
>>>
>>> Is it write and read or only write?
>> Yes. It's rpmb specific problem.
>> RPMB R/W use same flow. but only 'write' case makes the problem(long
>> busy state) sometimes.
>> When I test it, the 5~25ms is not enough although there is not max
>> busy time in the spec.
> 
> As a matter of fact this isn't specific to RPMB writes at all but to
> all data writes. That's why we call card_busy_detect() after a data
> write.

Further more, we should cover all transfer pattern from userspace, not
just data writes. :)

https://www.spinics.net/lists/linux-mmc/msg46147.html


> 
>>
>>>
>>>>
>>>> Signed-off-by: Huijin Park <huijin.park@samsung.com>
>>>> ---
>>>>   drivers/mmc/core/block.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
>>>> index ea80ff4..b5b0fe6 100644
>>>> --- a/drivers/mmc/core/block.c
>>>> +++ b/drivers/mmc/core/block.c
>>>> @@ -584,7 +584,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
>>>>                   * Ensure RPMB command has completed by polling CMD13
>>>>                   * "Send Status".
>>>>                   */
>>>> -               err = ioctl_rpmb_card_status_poll(card, &status, 5);
>>>> +               err = ioctl_rpmb_card_status_poll(card, &status, 50);
>>>
>>> First, you are changing "re-tries" not a timeout value, which would
>>> seem like a better approach.
>> I will modify it. Thanks.
>>
>>>
>>> Second, it seems like we should remove the
>>> ioctl_rpmb_card_status_poll() function altogether and instead make use
>>> of the more generic card_busy_detect() function here.
>>>
>>> Then if card_busy_detect() needs to be adopted to suite rpmb, then we
>>> should do that instead.
>> I agree that. But the two function are different a little.
>> 'ioctl_rpmb_card_status_poll()' has interval(1~5ms) that prevent to
>> poll too frequently.
> 
> Well, polling too frequently is then already a problem for the general
> data writes. Isn't it?
> 
> Or you think RPMB may differ a bit in this regards?
> 
>> So I suggest to improve 'card_busy_detect()' about interval time in
>> another patch.
> 
> Okay.
> 
> However, I don't think it's a stopper, you should be able to convert
> RPMB to use card_busy_detect() as is. Then you can fix the polling
> interval time on top.
> 
> What you do need to fix is that card_busy_detect() takes an struct
> request *req as in-parameter, and uses it to print the disk_name. I
> suggest to change the in-parameter to be the disk_name instead, that
> way it would work for RPMB as well.
> 
>> Then the 'ioctl_rpmb_card_status_poll()' can replaced by 'card_busy_detect()'
> 
> Great!
> 
> [...]
> 
> Kind regards
> Uffe
> --
> 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
> 
> 
> 

Re: [PATCH] mmc: card: increase wait time

[Ulf Hansson]

On 21 November 2017 at 10:34, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> Hi Ulf,
>
> On 2017/11/21 17:02, Ulf Hansson wrote:
>>
>> On 20 November 2017 at 05:50, Park Huijin <bbang.huijin.park@gmail.com>
>> wrote:
>>>
>>> 2017-11-15 18:13 GMT+09:00 Ulf Hansson <ulf.hansson@linaro.org>:
>>>>
>>>>
>>>> On 14 November 2017 at 03:48, Huijin Park <huijin.park@samsung.com>
>>>> wrote:
>>>>>
>>>>> Some eMMC products keep busy time a little longer intermittently.
>>>>> (eg. prepare free blocks, garbage collection...)
>>>>> In this case, we recommend to wait a little longer than fail.
>>>>
>>>>
>>>> Is this an rpmb specific problem, because the code seems only to change
>>>> that.
>>>>
>>>> Is it write and read or only write?
>>>
>>> Yes. It's rpmb specific problem.
>>> RPMB R/W use same flow. but only 'write' case makes the problem(long
>>> busy state) sometimes.
>>> When I test it, the 5~25ms is not enough although there is not max
>>> busy time in the spec.
>>
>>
>> As a matter of fact this isn't specific to RPMB writes at all but to
>> all data writes. That's why we call card_busy_detect() after a data
>> write.
>
>
> Further more, we should cover all transfer pattern from userspace, not
> just data writes. :)
>
> https://www.spinics.net/lists/linux-mmc/msg46147.html

Ack!

Kind regards
Uffe