Re: [PATCH v7 1/8] mmc: sdhci-esdhc-imx: remove unnecessary mmc_card_wake_sdio_irq check for tuning save/restore

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Unbalanced `disable_irq()` and `enable_irq()` during suspend/resume for devices with out-of-band wakeup, causing the SDHCI IRQ to remain permanently disabled after system resume.
- [High] The system suspend callback `sdhci_esdhc_suspend()` ignores the return value of `pm_runtime_force_suspend()`, leading to an unbalanced `pm_runtime_enable()` and PM state machine corruption on failure.
- [High] The runtime resume callback `sdhci_esdhc_runtime_resume()` leaks hardware clocks and PM QoS requests if `cqhci_resume()` fails.
--

--- Patch [1]: [PATCH v7 1/8] mmc: sdhci-esdhc-imx: remove unnecessary mmc_card_wake_sdio_irq check for tuning save/restore ---
commit e08eb8101cb72edc4b3a76b418077811fe74beba
Author: Luke Wang <ziniu.wang_1@nxp.com>

mmc: sdhci-esdhc-imx: remove unnecessary mmc_card_wake_sdio_irq check for tuning save/restore

This commit removes the mmc_card_wake_sdio_irq() check from the tuning
delay save and restore paths during system PM. The check is removed
because tuning values need to be preserved based on whether the card keeps
power, regardless of if it sets the SDIO IRQ wake flag.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index d0fa83f67a806..6e06b3eeacce9 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -2064,8 +2064,7 @@ static int sdhci_esdhc_suspend(struct device *dev)
>  	 * to save the tuning delay value just in case the usdhc
>  	 * lost power during system PM.
>  	 */
> -	if (mmc_card_keep_power(host->mmc) && mmc_card_wake_sdio_irq(host->mmc) &&
> -	    esdhc_is_usdhc(imx_data))
> +	if (mmc_card_keep_power(host->mmc) && esdhc_is_usdhc(imx_data))
>  		sdhc_esdhc_tuning_save(host);
>  
>  	if (device_may_wakeup(dev)) {

[Severity: High]
This isn't a bug introduced by this patch, but can the host IRQ remain
permanently disabled after a system resume for devices with out-of-band wakeup?

In sdhci_esdhc_suspend(), the irq is unconditionally disabled and wakeups are
enabled if the device may wake up:

    disable_irq(host->irq);
    if (device_may_wakeup(dev)) {
        if (!sdhci_enable_irq_wakeups(host))
            ...

For SDIO devices using out-of-band GPIO wakeup, mmc_card_wake_sdio_irq()
evaluates to false. This causes sdhci_enable_irq_wakeups() to return false
immediately without setting host->irq_wake_enabled = true.

If enable_irq() in sdhci_esdhc_resume() is explicitly gated behind 
host->irq_wake_enabled, does this leave the IRQ permanently disabled after
the first suspend/resume cycle, requiring a system reboot or unbinding to
recover?


[Severity: High]
This is a pre-existing issue, but does sdhci_esdhc_suspend() ignore the return
value of pm_runtime_force_suspend()?

If pm_runtime_force_suspend() fails, it handles cleanup by calling
pm_runtime_enable(dev) and returns an error code. Since sdhci_esdhc_suspend()
ignores this return value, the PM core considers the device successfully
suspended.

Upon system resume, sdhci_esdhc_resume() calls pm_runtime_force_resume(dev).
Since the device failed to suspend and never set needs_force_resume, this skips
the active resume step and executes its out path, which unconditionally calls
pm_runtime_enable(dev). 

Does this second pm_runtime_enable() drive dev->power.disable_depth below 0,
triggering an unbalanced pm_runtime_enable kernel warning and corrupting the
runtime PM state machine?

> @@ -2124,8 +2123,7 @@ static int sdhci_esdhc_resume(struct device *dev)
>  	 * restore the saved tuning delay value for the device which keep
>  	 * power during system PM.
>  	 */
> -	if (mmc_card_keep_power(host->mmc) && mmc_card_wake_sdio_irq(host->mmc) &&
> -	    esdhc_is_usdhc(imx_data))
> +	if (mmc_card_keep_power(host->mmc) && esdhc_is_usdhc(imx_data))
>  		sdhc_esdhc_tuning_restore(host);
>  
>  	pm_runtime_put_autosuspend(dev);

[Severity: High]
This isn't a bug introduced by this patch, but does sdhci_esdhc_runtime_resume()
leak hardware clocks and PM QoS requests if cqhci_resume() fails?

If the device supports Command Queuing Engine (CQE), the function calls
cqhci_resume():

    if (host->mmc->caps2 & MMC_CAP2_CQE)
        err = cqhci_resume(host->mmc);

    return err;

If cqhci_resume() returns an error, the function returns directly instead of
jumping to the disable_per_clk labels to call clk_disable_unprepare() for the
three clocks and cpu_latency_qos_remove_request(). 

Does this leave the hardware resources permanently active while the PM core
treats the device as runtime suspended or in an error state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715071818.272318-1-ziniu.wang_1@oss.nxp.com?part=1