[RFC PATCH] mmc: disable retuning when tuning

[RFC PATCH] mmc: disable retuning when tuning

[Wolfram Sang]

It might be that something goes wrong during tuning so the MMC core will
immediately trigger a retune. In our case it was:

 - we sent a tuning block
 - there was an error so we need to send an abort cmd to the eMMC
 - the abort cmd had a CRC error
 - retune was set by the MMC core

This lead to a vicious circle causing a performance regression of 75%.
So, disable retuning while we tune. Let the tuning complete and see then
if it worked out or not.

Reported-by Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

Hi Ulf, this patch is marked as RFC because I think this is a generic
issue. Lots of things could happen in the driver callback which cause a
retune, so I'd think it makes sense to deactivate it globally here. If
you think this is a driver specific issue, just let me know. I can
provide a small patch to create the issue for SDHI hardware, created
by Shimoda-san. We couldn't think of an easy way to reproduce it with
the fault injector, sadly. Let me know if you want to see that patch.

 drivers/mmc/core/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index b039dcff17f8..54f0814f110c 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -927,6 +927,8 @@ int mmc_execute_tuning(struct mmc_card *card)
 	if (!host->ops->execute_tuning)
 		return 0;
 
+	mmc_retune_disable(host);
+
 	if (host->cqe_on)
 		host->cqe_ops->cqe_off(host);
 
-- 
2.30.2

Re: [RFC PATCH] mmc: disable retuning when tuning

[Ulf Hansson]

On Fri, 18 Jun 2021 at 09:39, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> It might be that something goes wrong during tuning so the MMC core will
> immediately trigger a retune. In our case it was:
>
>  - we sent a tuning block
>  - there was an error so we need to send an abort cmd to the eMMC
>  - the abort cmd had a CRC error
>  - retune was set by the MMC core
>
> This lead to a vicious circle causing a performance regression of 75%.
> So, disable retuning while we tune. Let the tuning complete and see then
> if it worked out or not.
>
> Reported-by Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>
> Hi Ulf, this patch is marked as RFC because I think this is a generic
> issue. Lots of things could happen in the driver callback which cause a
> retune, so I'd think it makes sense to deactivate it globally here. If
> you think this is a driver specific issue, just let me know. I can
> provide a small patch to create the issue for SDHI hardware, created
> by Shimoda-san. We couldn't think of an easy way to reproduce it with
> the fault injector, sadly. Let me know if you want to see that patch.

This certainly makes sense to me! We should probably tag this (or
something along this change) for stable.

However, I would like to get some input from Adrian about this as
well, so I have looped him in.

Kind regards
Uffe

>
>  drivers/mmc/core/core.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index b039dcff17f8..54f0814f110c 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -927,6 +927,8 @@ int mmc_execute_tuning(struct mmc_card *card)
>         if (!host->ops->execute_tuning)
>                 return 0;
>
> +       mmc_retune_disable(host);
> +
>         if (host->cqe_on)
>                 host->cqe_ops->cqe_off(host);
>
> --
> 2.30.2
>

Re: [RFC PATCH] mmc: disable retuning when tuning

[Adrian Hunter]

On 18/06/21 1:45 pm, Ulf Hansson wrote:
> On Fri, 18 Jun 2021 at 09:39, Wolfram Sang
> <wsa+renesas@sang-engineering.com> wrote:
>>
>> It might be that something goes wrong during tuning so the MMC core will
>> immediately trigger a retune. In our case it was:
>>
>>  - we sent a tuning block
>>  - there was an error so we need to send an abort cmd to the eMMC
>>  - the abort cmd had a CRC error
>>  - retune was set by the MMC core
>>
>> This lead to a vicious circle causing a performance regression of 75%.
>> So, disable retuning while we tune. Let the tuning complete and see then
>> if it worked out or not.
>>
>> Reported-by Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> ---
>>
>> Hi Ulf, this patch is marked as RFC because I think this is a generic
>> issue. Lots of things could happen in the driver callback which cause a
>> retune, so I'd think it makes sense to deactivate it globally here. If
>> you think this is a driver specific issue, just let me know. I can
>> provide a small patch to create the issue for SDHI hardware, created
>> by Shimoda-san. We couldn't think of an easy way to reproduce it with
>> the fault injector, sadly. Let me know if you want to see that patch.
> 
> This certainly makes sense to me! We should probably tag this (or
> something along this change) for stable.
> 
> However, I would like to get some input from Adrian about this as
> well, so I have looped him in.
> 
> Kind regards
> Uffe
> 
>>
>>  drivers/mmc/core/core.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> index b039dcff17f8..54f0814f110c 100644
>> --- a/drivers/mmc/core/core.c
>> +++ b/drivers/mmc/core/core.c
>> @@ -927,6 +927,8 @@ int mmc_execute_tuning(struct mmc_card *card)
>>         if (!host->ops->execute_tuning)
>>                 return 0;
>>
>> +       mmc_retune_disable(host);

mmc_retune_disable() is not meant for temporarily preventing re-tuning.
It is meant for exiting a transfer mode that requires re-tuning.

I would prefer something like below:

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index b039dcff17f8..f6d97bffc559 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -937,11 +937,14 @@ int mmc_execute_tuning(struct mmc_card *card)
 
 	err = host->ops->execute_tuning(host, opcode);
 
-	if (err)
+	if (err) {
 		pr_err("%s: tuning execution failed: %d\n",
 			mmc_hostname(host), err);
-	else
+	} else {
 		mmc_retune_enable(host);
+		host->retune_now = 0;
+		host->need_retune = 0;
+	}
 
 	return err;
 }


Would that work?

>> +
>>         if (host->cqe_on)
>>                 host->cqe_ops->cqe_off(host);
>>
>> --
>> 2.30.2
>>

Re: [RFC PATCH] mmc: disable retuning when tuning

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 934 bytes --]

Hi Adrian,

> mmc_retune_disable() is not meant for temporarily preventing re-tuning.
> It is meant for exiting a transfer mode that requires re-tuning.

Okay, I will add this as documentation.

> I would prefer something like below:
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index b039dcff17f8..f6d97bffc559 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -937,11 +937,14 @@ int mmc_execute_tuning(struct mmc_card *card)
>  
>  	err = host->ops->execute_tuning(host, opcode);
>  
> -	if (err)
> +	if (err) {
>  		pr_err("%s: tuning execution failed: %d\n",
>  			mmc_hostname(host), err);
> -	else
> +	} else {
>  		mmc_retune_enable(host);
> +		host->retune_now = 0;
> +		host->need_retune = 0;
> +	}
>  
>  	return err;
>  }
> 
> 
> Would that work?

I will check it and report back. Thanks for the input!

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]