[RFT] mmc: tmio: reset device on timeout, too

When a command response times out, the TMIO driver has been resetting
the controller ever since. However, this means some initialization like
bus width or tuning settings will be forgotten. To ensure proper working
in all code paths, we will enforce a reset of the remote device, too.
Many thanks to the Renesas BSP team for the detailed description of the
problem.

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

This patch depends on the TMIO reset refactorization:

[RFT 0/6] mmc: refactor reset callbacks

Looking also for tests here. Thanks!

 drivers/mmc/host/tmio_mmc_core.c | 1 +
 1 file changed, 1 insertion(+)

> This isn't how mmc_hw_reset() is intended to be used. Instead, the
> idea is that it should be called by upper layer code, when some error
> path is triggered for an I/O request.

Hmm, there are some wireless drivers using it as well. I am confused, is
this considered "upper layer"?

drivers/net/wireless/ath/ath10k/sdio.c: ret = mmc_hw_reset(ar_sdio->func->card->host);
drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c:        mmc_hw_reset(sdiodev->func1->card->host);
drivers/net/wireless/marvell/mwifiex/sdio.c:    ret = mmc_hw_reset(func->card->host);
drivers/net/wireless/ti/wlcore/sdio.c:  mmc_hw_reset(card->host);

I'd like to understand, so I can add some docs. Because the intended use
is nowhere documented to the best of my knowledge.

> However, let me think a bit about this.

Sure, thanks for the help!

Hi Ulf,

> > Hmm, there are some wireless drivers using it as well. I am confused, is
> > this considered "upper layer"?
> >
> > drivers/net/wireless/ath/ath10k/sdio.c: ret = mmc_hw_reset(ar_sdio->func->card->host);
> > drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c:        mmc_hw_reset(sdiodev->func1->card->host);
> > drivers/net/wireless/marvell/mwifiex/sdio.c:    ret = mmc_hw_reset(func->card->host);
> > drivers/net/wireless/ti/wlcore/sdio.c:  mmc_hw_reset(card->host);
> 
> Correct, these are "upper layers". The same applies for the mmc block
> device driver.
> 
> In this way there is a guarantee that the struct mmc_card is still present.

Ah, now I get it. "upper layers" as in consumers. And because consumers
sit on a card, this guarantees that mmc_card is still there. Correct?

> That would be great. I appreciate all kinds of improvements on the doc parts.

You are welcome!

> Perhaps a better option is to return a specific error code for the
> last request, that makes the core run mmc_hw_reset(). Or potentially,
> add a host cap and let the core treat some error code, specifically
> for hosts like tmio.

A specific errno could work. I don't see the advantage of a CAP (besides
it is rather a quirk than a cap). We could also have
'mmc_controller_card_reset()' or something which ensures mmc_card is
present and let that controllers call when they see fit. Or?

Thanks for your help,

   Wolfram

On Wed, 9 Sep 2020 at 13:37, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Hi Ulf,
>
> > > Hmm, there are some wireless drivers using it as well. I am confused, is
> > > this considered "upper layer"?
> > >
> > > drivers/net/wireless/ath/ath10k/sdio.c: ret = mmc_hw_reset(ar_sdio->func->card->host);
> > > drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c:        mmc_hw_reset(sdiodev->func1->card->host);
> > > drivers/net/wireless/marvell/mwifiex/sdio.c:    ret = mmc_hw_reset(func->card->host);
> > > drivers/net/wireless/ti/wlcore/sdio.c:  mmc_hw_reset(card->host);
> >
> > Correct, these are "upper layers". The same applies for the mmc block
> > device driver.
> >
> > In this way there is a guarantee that the struct mmc_card is still present.
>
> Ah, now I get it. "upper layers" as in consumers. And because consumers
> sit on a card, this guarantees that mmc_card is still there. Correct?

Yes.

>
> > That would be great. I appreciate all kinds of improvements on the doc parts.
>
> You are welcome!
>
> > Perhaps a better option is to return a specific error code for the
> > last request, that makes the core run mmc_hw_reset(). Or potentially,
> > add a host cap and let the core treat some error code, specifically
> > for hosts like tmio.
>
> A specific errno could work. I don't see the advantage of a CAP (besides
> it is rather a quirk than a cap). We could also have
> 'mmc_controller_card_reset()' or something which ensures mmc_card is
> present and let that controllers call when they see fit. Or?

Maybe something like "mmc_controller_card_reset" could work, but it's
not going to be that straight forward. In the end, we depend on the
context for when the host driver would call such a function. In some
cases it must call mmc_claim_host() while in others it shouldn't.

BTW, I see that tmio_mmc_reset() is called at
tmio_mmc_host_runtime_resume(). This seems to work fine without having
to make a full reset of the card. Why can't you do something similar
to that instead?

Kind regards
Uffe

> > Ah, now I get it. "upper layers" as in consumers. And because consumers

> > sit on a card, this guarantees that mmc_card is still there. Correct?

> 

> Yes.


Good, I'll prepare a patch, hopefully in the next days.

> Maybe something like "mmc_controller_card_reset" could work, but it's

> not going to be that straight forward. In the end, we depend on the

> context for when the host driver would call such a function. In some

> cases it must call mmc_claim_host() while in others it shouldn't.


I see. It seems we should try to handle it locally in the driver then.

> BTW, I see that tmio_mmc_reset() is called at

> tmio_mmc_host_runtime_resume(). This seems to work fine without having

> to make a full reset of the card. Why can't you do something similar

> to that instead?


Good question. I'll investigate that. I am a bit afraid that it neither
works and only RPM never kicked in because of a workaround. But I need
to prove that, maybe it is something else...

Thanks for the help, Ulf!