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

[Adrian Hunter <adrian.hunter@intel.com>]

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
>>