| Message ID | 20210618073950.46154-1-wsa+renesas@sang-engineering.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] mmc: disable retuning when tuning | expand |
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 >
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 >>
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
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);
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(+)