[PATCH] mmc: sunxi: reset the controller after an update-clock timeout

[PATCH] mmc: sunxi: reset the controller after an update-clock timeout

[Tam Do]

If the clock-update command times out it stays pending in the
controller, and every later clock update times out as well, so the
card cannot be initialised until the next probe.

This happens on an R16 board whose SDIO Wi-Fi holds DAT0 busy while
its supply comes up during the first clock update.

Reset the controller on timeout so the next attempt can succeed.

Fixes: 3cbcb16095f9 ("mmc: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs")
Cc: stable@vger.kernel.org
Signed-off-by: Tam Do <tam.do@manyhobbies.com>
---
 drivers/mmc/host/sunxi-mmc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
index fe4c0f6..439cfa4 100644
--- a/drivers/mmc/host/sunxi-mmc.c
+++ b/drivers/mmc/host/sunxi-mmc.c
@@ -688,6 +688,7 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
 
 	if (rval & SDXC_START) {
 		dev_err(mmc_dev(host->mmc), "fatal err update clk timeout\n");
+		sunxi_mmc_init_host(host);
 		return -EIO;
 	}
 
-- 
2.39.5

Re: [PATCH] mmc: sunxi: reset the controller after an update-clock timeout

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mmc: sunxi: race on REG_IMASK and sdio_imask in sunxi_mmc_init_host()
--

commit 5bbf951431c455133c1e7c51cf18b994aea96bec
Author: Tam Do <tam.do@manyhobbies.com>

mmc: sunxi: reset the controller after an update-clock timeout

This commit resets and reinitializes the sunxi MMC controller in
sunxi_mmc_oclk_onoff() if a clock-update command times out, so that the
pending command is cleared and subsequent clock updates can succeed.

> diff --git a/drivers/mmc/host/sunxi-mmc.c b/drivers/mmc/host/sunxi-mmc.c
> index fe4c0f6d73f3c..439cfa4ac4032 100644
> --- a/drivers/mmc/host/sunxi-mmc.c
> +++ b/drivers/mmc/host/sunxi-mmc.c
> @@ -688,6 +688,7 @@ static int sunxi_mmc_oclk_onoff(struct sunxi_mmc_host *host, u32 oclk_en)
>  
>  	if (rval & SDXC_START) {
>  		dev_err(mmc_dev(host->mmc), "fatal err update clk timeout\n");
> +		sunxi_mmc_init_host(host);

[Severity: High]
Can calling sunxi_mmc_init_host() here without holding host->lock while
host->irq is enabled race with sunxi_mmc_irq() and
sunxi_mmc_enable_sdio_irq() on host->sdio_imask and REG_IMASK?

Previously, sunxi_mmc_init_host() was only called from sunxi_mmc_probe()
before mmc_add_host() and from sunxi_mmc_runtime_resume() while host->irq is
disabled. In sunxi_mmc_init_host(), the controller is reset and REG_IMASK is
written without holding host->lock:

drivers/mmc/host/sunxi-mmc.c:sunxi_mmc_init_host() {
	if (sunxi_mmc_reset_host(host))
		return -EIO;
	...
	/* Unmask SDIO interrupt if needed */
	mmc_writel(host, REG_IMASK, host->sdio_imask);
	/* Clear all pending interrupts */
	mmc_writel(host, REG_RINTR, 0xffffffff);
}

At runtime, sunxi_mmc_set_ios() -> sunxi_mmc_set_clk() ->
sunxi_mmc_clk_set_rate() -> sunxi_mmc_oclk_onoff() can be reached while an
SDIO card has claimed interrupts. Although sunxi_mmc_set_ios() runs under
mmc_claim_host(), both hardirq context (sunxi_mmc_irq() ->
mmc_signal_sdio_irq() -> sunxi_mmc_enable_sdio_irq()) and sdio_irq_thread()
(which calls host->ops->enable_sdio_irq() after mmc_release_host()) update
host->sdio_imask and REG_IMASK concurrently under host->lock:

drivers/mmc/host/sunxi-mmc.c:sunxi_mmc_enable_sdio_irq() {
	spin_lock_irqsave(&host->lock, flags);

	imask = mmc_readl(host, REG_IMASK);
	if (enable) {
		host->sdio_imask = SDXC_SDIO_INTERRUPT;
		imask |= SDXC_SDIO_INTERRUPT;
	} else {
		host->sdio_imask = 0;
		imask &= ~SDXC_SDIO_INTERRUPT;
	}
	mmc_writel(host, REG_IMASK, imask);
	spin_unlock_irqrestore(&host->lock, flags);
}

If sunxi_mmc_init_host() races with sunxi_mmc_enable_sdio_irq(), could it
either overwrite REG_IMASK with 0 after sdio_irq_thread() re-enabled
SDXC_SDIO_INTERRUPT (leaving sdio_irq_thread() sleeping with SDIO interrupts
masked in hardware), or write a stale SDXC_SDIO_INTERRUPT bit back to
REG_IMASK after hardirq context disabled it and called
pm_runtime_put_noidle() (causing repeated disable calls and underflowing the
runtime PM usage count)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179307312104.39863.12372814163472848752@manyhobbies.com?part=1