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

[sashiko-bot@kernel.org]

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