On 18/06/21 1:45 pm, Ulf Hansson wrote:
> On Fri, 18 Jun 2021 at 09:39, Wolfram Sang
> <wsa+renesas@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxx>
>> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
>> ---
>>
>> 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
>>
