| Message ID | 20210618082317.58408-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: disable tuning when checking card presence | expand |
> On 06/18/2021 10:23 AM Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > > When we use the alive callback, we expect a command to fail if the card > is not present. We should not trigger a retune then which will confuse > users with a failed retune on a removed card: > > mmc2: tuning execution failed: -5 > mmc2: card 0001 removed > > Disable retuning in this code path. > > Reported-by: Ulrich Hecht <uli+renesas@fpond.eu> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/mmc/core/core.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 54f0814f110c..eb792dd845a3 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -2088,6 +2088,9 @@ int _mmc_detect_card_removed(struct mmc_host *host) > if (!host->card || mmc_card_removed(host->card)) > return 1; > > + /* we expect a failure if the card is removed */ > + mmc_retune_disable(host); > + > ret = host->bus_ops->alive(host); > > /* > @@ -2107,6 +2110,8 @@ int _mmc_detect_card_removed(struct mmc_host *host) > pr_debug("%s: card remove detected\n", mmc_hostname(host)); > } > > + mmc_retune_enable(host); > + > return ret; > } > > -- > 2.30.2 Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu> CU Uli
+ Adrian On Fri, 18 Jun 2021 at 10:23, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > When we use the alive callback, we expect a command to fail if the card > is not present. We should not trigger a retune then which will confuse > users with a failed retune on a removed card: > > mmc2: tuning execution failed: -5 > mmc2: card 0001 removed > > Disable retuning in this code path. > > Reported-by: Ulrich Hecht <uli+renesas@fpond.eu> > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/mmc/core/core.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c > index 54f0814f110c..eb792dd845a3 100644 > --- a/drivers/mmc/core/core.c > +++ b/drivers/mmc/core/core.c > @@ -2088,6 +2088,9 @@ int _mmc_detect_card_removed(struct mmc_host *host) > if (!host->card || mmc_card_removed(host->card)) > return 1; > > + /* we expect a failure if the card is removed */ > + mmc_retune_disable(host); > + Some controllers require a retune after it has been runtime suspended. In the above path, when called via the bus_ops->detect() callback, it could be that the controller may have been runtime suspended and then got resumed by the call to mmc_get_card(). I think we need something more clever here, to make sure we don't end up in that situation. I have looped in Adrian, to see if has some ideas for how this can be fixed. > ret = host->bus_ops->alive(host); > > /* > @@ -2107,6 +2110,8 @@ int _mmc_detect_card_removed(struct mmc_host *host) > pr_debug("%s: card remove detected\n", mmc_hostname(host)); > } > > + mmc_retune_enable(host); > + > return ret; > } > > -- > 2.30.2 > Kind regards Uffe
On 18/06/21 1:42 pm, Ulf Hansson wrote: > + Adrian > > On Fri, 18 Jun 2021 at 10:23, Wolfram Sang > <wsa+renesas@sang-engineering.com> wrote: >> >> When we use the alive callback, we expect a command to fail if the card >> is not present. We should not trigger a retune then which will confuse >> users with a failed retune on a removed card: >> >> mmc2: tuning execution failed: -5 >> mmc2: card 0001 removed >> >> Disable retuning in this code path. >> >> Reported-by: Ulrich Hecht <uli+renesas@fpond.eu> >> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> >> --- >> drivers/mmc/core/core.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index 54f0814f110c..eb792dd845a3 100644 >> --- a/drivers/mmc/core/core.c >> +++ b/drivers/mmc/core/core.c >> @@ -2088,6 +2088,9 @@ int _mmc_detect_card_removed(struct mmc_host *host) >> if (!host->card || mmc_card_removed(host->card)) >> return 1; >> >> + /* we expect a failure if the card is removed */ >> + mmc_retune_disable(host); >> + > > Some controllers require a retune after it has been runtime suspended. > > In the above path, when called via the bus_ops->detect() callback, it > could be that the controller may have been runtime suspended and then > got resumed by the call to mmc_get_card(). > > I think we need something more clever here, to make sure we don't end > up in that situation. I have looped in Adrian, to see if has some > ideas for how this can be fixed. Can we clarify, is the only problem that the error message is confusing? > >> ret = host->bus_ops->alive(host); >> >> /* >> @@ -2107,6 +2110,8 @@ int _mmc_detect_card_removed(struct mmc_host *host) >> pr_debug("%s: card remove detected\n", mmc_hostname(host)); >> } >> >> + mmc_retune_enable(host); >> + >> return ret; >> } >> >> -- >> 2.30.2 >> > > Kind regards > Uffe >
> On 06/21/2021 9:15 AM Adrian Hunter <adrian.hunter@intel.com> wrote: > Can we clarify, is the only problem that the error message is confusing? AFAICT there are no ill effects of the retune failing apart from the error message. CU Uli
On 21/06/21 10:32 am, Ulrich Hecht wrote: > >> On 06/21/2021 9:15 AM Adrian Hunter <adrian.hunter@intel.com> wrote: >> Can we clarify, is the only problem that the error message is confusing? > > AFAICT there are no ill effects of the retune failing apart from the error message. > So maybe the simplest thing to do is just amend the message: e.g. diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 4e52eb14198a..5cbf05e331c4 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -936,13 +936,22 @@ int mmc_execute_tuning(struct mmc_card *card) opcode = MMC_SEND_TUNING_BLOCK; err = host->ops->execute_tuning(host, opcode); - if (err) - pr_err("%s: tuning execution failed: %d\n", - mmc_hostname(host), err); - else - mmc_retune_enable(host); + goto out_err; + + mmc_retune_enable(host); + return 0; + +out_err: + if (mmc_card_is_removable(host)) { + if (err != -ENOMEDIUM) + pr_err("%s: tuning execution failed: %d (this is normal if card removed)\n", + mmc_hostname(host), err); + } else { + pr_err("%s: tuning execution failed: %d\n", + mmc_hostname(host), err); + } return err; }
> + pr_err("%s: tuning execution failed: %d (this is normal if card removed)\n", > + mmc_hostname(host), err); Hmm, an error message saying "this is normal" doesn't look like a good option to me. Can't we surpress the message somehow or even avoid tuning somehow if the card is removed? Sorry, I can't look this up myself right now, working on another task today.
On 21/06/21 11:11 am, Wolfram Sang wrote: > On 21/06/21 10:54 am, Adrian Hunter wrote: >> On 21/06/21 10:32 am, Ulrich Hecht wrote: >>> >>>> On 06/21/2021 9:15 AM Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> Can we clarify, is the only problem that the error message is confusing? >>> >>> AFAICT there are no ill effects of the retune failing apart from the error message. >>> >> >> So maybe the simplest thing to do is just amend the message: >> e.g. >> >> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c >> index 4e52eb14198a..5cbf05e331c4 100644 >> --- a/drivers/mmc/core/core.c >> +++ b/drivers/mmc/core/core.c >> @@ -936,13 +936,22 @@ int mmc_execute_tuning(struct mmc_card *card) >> opcode = MMC_SEND_TUNING_BLOCK; >> >> err = host->ops->execute_tuning(host, opcode); >> - >> if (err) >> - pr_err("%s: tuning execution failed: %d\n", >> - mmc_hostname(host), err); >> - else >> - mmc_retune_enable(host); >> + goto out_err; >> + >> + mmc_retune_enable(host); >> >> + return 0; >> + >> +out_err: >> + if (mmc_card_is_removable(host)) { >> + if (err != -ENOMEDIUM) >> + pr_err("%s: tuning execution failed: %d (this is normal if card removed)\n", >> + mmc_hostname(host), err); > > Hmm, an error message saying "this is normal" doesn't look like a good > option to me. Can't we surpress the message somehow or even avoid tuning > somehow if the card is removed? Sorry, I can't look this up myself right > now, working on another task today. With the code above, if the host controller knows the card has been removed, it can return -ENOMEDIUM from ->execute_tuning() to suppress the message. Otherwise, you need to introduce a new card state or flag to indicate that the card may not be present, and use that to suppress the message. > >> + } else { >> + pr_err("%s: tuning execution failed: %d\n", >> + mmc_hostname(host), err); >> + } >> return err; >> } >> >> >> >
Hi Adrian, Ulf, everyone, > With the code above, if the host controller knows the card has been > removed, it can return -ENOMEDIUM from ->execute_tuning() to suppress > the message. On second thought, I like the idea with -ENOMEDIUM. Because tuning can still fail for reasons other than a removed card and we want to see an error message then. So, I checked when/how to return -ENOMEDIUM for the SDHI driver but this lead me to more questions. The few driver which return this error code all follow a similar pattern: xxx_request() { if (host->get_cd == 1) submit_mrq else cmd->error = -ENOMEDIUM mmc_request_done() } So, my first question would be if we can't apply this pattern in the core before calling the .request callback? A lot of drivers are not implementing this pattern although it seems useful. Is it required? Recommended? Nice to have? However, I could imagine an answer for moving it into the core is "no, that should be checked atomically"? E.g. sdhci does it, but atmel-mci and s3cmci do not. If I just look at moving the card detection call into the core, I don't really see the reason for atomic. Am I missing something? All the best, Wolfram
On Sat, 26 Jun 2021 at 20:58, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > Hi Adrian, Ulf, everyone, > > > With the code above, if the host controller knows the card has been > > removed, it can return -ENOMEDIUM from ->execute_tuning() to suppress > > the message. > > On second thought, I like the idea with -ENOMEDIUM. Because tuning can > still fail for reasons other than a removed card and we want to see an > error message then. > > So, I checked when/how to return -ENOMEDIUM for the SDHI driver but this > lead me to more questions. The few driver which return this error code > all follow a similar pattern: > > xxx_request() > { > if (host->get_cd == 1) > submit_mrq > else > cmd->error = -ENOMEDIUM > mmc_request_done() > } > > So, my first question would be if we can't apply this pattern in the > core before calling the .request callback? A lot of drivers are not > implementing this pattern although it seems useful. Is it required? It's required for some sdhci variants, because issuing a command when a card has been removed can hang (or completes after quite a long timeout, I don't recall, Adrian?). > Recommended? Nice to have? However, I could imagine an answer for moving > it into the core is "no, that should be checked atomically"? E.g. sdhci > does it, but atmel-mci and s3cmci do not. If I just look at moving the > card detection call into the core, I don't really see the reason for > atomic. Am I missing something? My main concern would be performance/latency, as we would introduce some overhead for every single request. So, no, we don't want this in the core in my opinion. Kind regards Uffe
On 29/06/21 5:16 pm, Ulf Hansson wrote: > On Sat, 26 Jun 2021 at 20:58, Wolfram Sang > <wsa+renesas@sang-engineering.com> wrote: >> >> Hi Adrian, Ulf, everyone, >> >>> With the code above, if the host controller knows the card has been >>> removed, it can return -ENOMEDIUM from ->execute_tuning() to suppress >>> the message. >> >> On second thought, I like the idea with -ENOMEDIUM. Because tuning can >> still fail for reasons other than a removed card and we want to see an >> error message then. >> >> So, I checked when/how to return -ENOMEDIUM for the SDHI driver but this >> lead me to more questions. The few driver which return this error code >> all follow a similar pattern: >> >> xxx_request() >> { >> if (host->get_cd == 1) >> submit_mrq >> else >> cmd->error = -ENOMEDIUM >> mmc_request_done() >> } >> >> So, my first question would be if we can't apply this pattern in the >> core before calling the .request callback? A lot of drivers are not >> implementing this pattern although it seems useful. Is it required? > > It's required for some sdhci variants, because issuing a command when > a card has been removed can hang (or completes after quite a long > timeout, I don't recall, Adrian?). If the host supports SDHCI's own card detect then after the card is removed requests will not start nor error, until the 10 second software timeout. The logic to check SDHCI card present predated the use of GPIO card-detect but the same approach was copied, although it should be possible to do without it. > >> Recommended? Nice to have? However, I could imagine an answer for moving >> it into the core is "no, that should be checked atomically"? E.g. sdhci >> does it, but atmel-mci and s3cmci do not. If I just look at moving the >> card detection call into the core, I don't really see the reason for >> atomic. Am I missing something? > > My main concern would be performance/latency, as we would introduce > some overhead for every single request. So, no, we don't want this in > the core in my opinion. I agree. I would get rid of it from SDHCI but it looked like we might need to rely on ->card_event() which won't work because it claims the host, which will wait for the block driver to release it, which will wait for the request anyway. That was as far as I got, thinking about it.
> Otherwise, you need to introduce a new card state or flag to indicate > that the card may not be present, and use that to suppress the message. I now went this route and, for me, 'detect_change' worked. Will send a patch in a minute.
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c index 54f0814f110c..eb792dd845a3 100644 --- a/drivers/mmc/core/core.c +++ b/drivers/mmc/core/core.c @@ -2088,6 +2088,9 @@ int _mmc_detect_card_removed(struct mmc_host *host) if (!host->card || mmc_card_removed(host->card)) return 1; + /* we expect a failure if the card is removed */ + mmc_retune_disable(host); + ret = host->bus_ops->alive(host); /* @@ -2107,6 +2110,8 @@ int _mmc_detect_card_removed(struct mmc_host *host) pr_debug("%s: card remove detected\n", mmc_hostname(host)); } + mmc_retune_enable(host); + return ret; }
When we use the alive callback, we expect a command to fail if the card is not present. We should not trigger a retune then which will confuse users with a failed retune on a removed card: mmc2: tuning execution failed: -5 mmc2: card 0001 removed Disable retuning in this code path. Reported-by: Ulrich Hecht <uli+renesas@fpond.eu> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/mmc/core/core.c | 5 +++++ 1 file changed, 5 insertions(+)