[PATCH v5 0/8] mmc: sdhci-esdhc-imx: fix SDIO suspend/resume issues

[PATCH v5 0/8] mmc: sdhci-esdhc-imx: fix SDIO suspend/resume issues

[ziniu.wang_1]

From: Luke Wang <ziniu.wang_1@nxp.com>

This series fixes several suspend/resume issues in the sdhci-esdhc-imx
driver, primarily affecting SDIO devices (e.g., WiFi).

Issues fixed:
- Tuning delay values not saved/restored for out-of-band wakeup devices
- DLL override lost after resume for DDR modes
- Pinctrl not restored before DDR_EN is set, causing CRC errors
- Unhandled interrupt during resume causing "nobody cared" warning
- Various error handling issues in suspend/resume paths

Changes since v4:
- New patch 3: Fix esdhc_change_pinstate() to move IS_ERR checks for
  pins_100mhz/pins_200mhz into their respective switch cases, so that
  legacy timing modes can still reach pinctrl_select_default_state().
  This avoids log spam on resume for devices without UHS pin states (Sashiko)

Changes since v3:
- Patch 2: Remove unused 'boarddata' variable in esdhc_set_uhs_signaling()
- Patch 7: Expand commit message to document intentional skip of enable_irq()
  and sdhci_disable_irq_wakeups() on early return, and explain why the
  PM runtime usage counter leak is acceptable (Sashiko)

Changes since v2:
- Patch 3: Updated commit message to explain the SoC IP integration
  detail about internal loopback path change when switching pinctrl
  from GPIO to USDHC function (per Bough Chen suggestion)
- Patch 5 (v2): Split into three separate patches (5/6/7) so each
  patch fixes one problem (per Frank Li suggestion)

Changes since v1:
- Added patch 5 to fix error handling issues identified during review
- Use pm_runtime_resume_and_get() instead of pm_runtime_get_sync()
- Make pinctrl and cd-wake failures non-fatal (dev_warn only)
- Use esdhc_change_pinstate() instead of pinctrl_pm_select_default_state()
  in resume to restore correct pin state based on timing mode
- Skip pinctrl restore for wakeup devices to avoid SD bus glitch
- Check pm_runtime_force_resume() return value in resume

Luke Wang (8):
  mmc: sdhci-esdhc-imx: remove unnecessary mmc_card_wake_sdio_irq check
    for tuning save/restore
  mmc: sdhci-esdhc-imx: restore DLL override for DDR modes on resume
  mmc: sdhci-esdhc-imx: fix esdhc_change_pinstate() to allow default
    state restore
  mmc: sdhci-esdhc-imx: restore pinctrl before restoring ios timing on
    resume
  mmc: sdhci-esdhc-imx: disable irq during suspend to fix unhandled
    interrupt
  mmc: sdhci-esdhc-imx: use pm_runtime_resume_and_get() in suspend
  mmc: sdhci-esdhc-imx: make non-fatal errors non-blocking in suspend
  mmc: sdhci-esdhc-imx: fix resume error handling

 drivers/mmc/host/sdhci-esdhc-imx.c | 84 ++++++++++++++++++++----------
 1 file changed, 57 insertions(+), 27 deletions(-)

-- 
2.34.1

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

[ziniu.wang_1]

From: Luke Wang <ziniu.wang_1@nxp.com>

The tuning save/restore during system PM is conditioned on
mmc_card_wake_sdio_irq(), but this check is unrelated to whether
tuning values need to be preserved. The actual requirement is that
the card keeps power during suspend and the controller is a uSDHC.

SDIO devices using out-of-band GPIO wakeup maintain power during
suspend but do not set the SDIO IRQ wake flag. In this case the
tuning delay values are not saved/restored.

Remove the unnecessary mmc_card_wake_sdio_irq() condition from both
the suspend save and resume restore paths.

Fixes: c63d25cdc59a ("mmc: sdhci-esdhc-imx: Save tuning value when card stays powered in suspend")
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 18ecddd6df6f..6526d65538de 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)) {
@@ -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);
-- 
2.34.1

[PATCH v5 2/8] mmc: sdhci-esdhc-imx: restore DLL override for DDR modes on resume

[ziniu.wang_1]

From: Luke Wang <ziniu.wang_1@nxp.com>

sdhci_esdhc_imx_hwinit() unconditionally clears ESDHC_DLL_CTRL by
writing zero. For SDIO devices that keep power during system suspend
and operate in DDR mode, the card remains in DDR timing while the host
DLL override configuration is lost.

Extract the DLL override setup from esdhc_set_uhs_signaling() into
a helper esdhc_set_dll_override(), and call it on the resume path
when the card kept power and is using a DDR timing mode.

Fixes: 676a83855614 ("mmc: host: sdhci-esdhc-imx: refactor the system PM logic")
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 39 +++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 6526d65538de..7230d70e02ae 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1349,6 +1349,23 @@ static int esdhc_change_pinstate(struct sdhci_host *host,
 	return pinctrl_select_state(imx_data->pinctrl, pinctrl);
 }
 
+static void esdhc_set_dll_override(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
+	struct esdhc_platform_data *boarddata = &imx_data->boarddata;
+	u32 v;
+
+	if (!boarddata->delay_line)
+		return;
+
+	v = boarddata->delay_line << ESDHC_DLL_OVERRIDE_VAL_SHIFT |
+	    (1 << ESDHC_DLL_OVERRIDE_EN_SHIFT);
+	if (is_imx53_esdhc(imx_data))
+		v <<= 1;
+	writel(v, host->ioaddr + ESDHC_DLL_CTRL);
+}
+
 /*
  * For HS400 eMMC, there is a data_strobe line. This signal is generated
  * by the device and used for data output and CRC status response output
@@ -1404,7 +1421,6 @@ static void esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
 	u32 m;
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
-	struct esdhc_platform_data *boarddata = &imx_data->boarddata;
 
 	/* disable ddr mode and disable HS400 mode */
 	m = readl(host->ioaddr + ESDHC_MIX_CTRL);
@@ -1425,15 +1441,7 @@ static void esdhc_set_uhs_signaling(struct sdhci_host *host, unsigned timing)
 		m |= ESDHC_MIX_CTRL_DDREN;
 		writel(m, host->ioaddr + ESDHC_MIX_CTRL);
 		imx_data->is_ddr = 1;
-		if (boarddata->delay_line) {
-			u32 v;
-			v = boarddata->delay_line <<
-				ESDHC_DLL_OVERRIDE_VAL_SHIFT |
-				(1 << ESDHC_DLL_OVERRIDE_EN_SHIFT);
-			if (is_imx53_esdhc(imx_data))
-				v <<= 1;
-			writel(v, host->ioaddr + ESDHC_DLL_CTRL);
-		}
+		esdhc_set_dll_override(host);
 		break;
 	case MMC_TIMING_MMC_HS400:
 		m |= ESDHC_MIX_CTRL_DDREN | ESDHC_MIX_CTRL_HS400_EN;
@@ -2123,9 +2131,18 @@ 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) && esdhc_is_usdhc(imx_data))
+	if (mmc_card_keep_power(host->mmc) && esdhc_is_usdhc(imx_data)) {
 		sdhc_esdhc_tuning_restore(host);
 
+		/*
+		 * Restore DLL override for DDR modes. hwinit unconditionally
+		 * clears ESDHC_DLL_CTRL, but the card is still in DDR mode.
+		 */
+		if (host->timing == MMC_TIMING_UHS_DDR50 ||
+		    host->timing == MMC_TIMING_MMC_DDR52)
+			esdhc_set_dll_override(host);
+	}
+
 	pm_runtime_put_autosuspend(dev);
 
 	return ret;
-- 
2.34.1

[PATCH v5 3/8] mmc: sdhci-esdhc-imx: fix esdhc_change_pinstate() to allow default state restore

[ziniu.wang_1]

From: Luke Wang <ziniu.wang_1@nxp.com>

esdhc_change_pinstate() checks for pins_100mhz and pins_200mhz at the
top of the function and returns -EINVAL if either is not defined. This
prevents the default case from ever being reached, which means devices
with a sleep pinctrl state but without high-speed pin states (100mhz/
200mhz) can never restore their default pin configuration.

Move the IS_ERR checks for pins_100mhz and pins_200mhz into their
respective switch cases.

Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 7230d70e02ae..ead4685d621a 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1326,19 +1326,21 @@ static int esdhc_change_pinstate(struct sdhci_host *host,
 
 	dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs);
 
-	if (IS_ERR(imx_data->pinctrl) ||
-		IS_ERR(imx_data->pins_100mhz) ||
-		IS_ERR(imx_data->pins_200mhz))
+	if (IS_ERR(imx_data->pinctrl))
 		return -EINVAL;
 
 	switch (uhs) {
 	case MMC_TIMING_UHS_SDR50:
 	case MMC_TIMING_UHS_DDR50:
+		if (IS_ERR(imx_data->pins_100mhz))
+			return -EINVAL;
 		pinctrl = imx_data->pins_100mhz;
 		break;
 	case MMC_TIMING_UHS_SDR104:
 	case MMC_TIMING_MMC_HS200:
 	case MMC_TIMING_MMC_HS400:
+		if (IS_ERR(imx_data->pins_200mhz))
+			return -EINVAL;
 		pinctrl = imx_data->pins_200mhz;
 		break;
 	default:
-- 
2.34.1

[PATCH v5 4/8] mmc: sdhci-esdhc-imx: restore pinctrl before restoring ios timing on resume

[ziniu.wang_1]

From: Luke Wang <ziniu.wang_1@nxp.com>

SDIO devices such as WiFi may keep power during suspend, so the MMC
core skips full card re-initialization on resume and directly restores
the host controller's ios timing to match the card. For DDR mode,
pm_runtime_force_resume() sets DDR_EN before the pin configuration is
restored from sleep state.

This is related to the SoC IP integration: switching pinctrl setting
(changing alt from GPIO to USDHC) impacts the internal loopback path.
If pinctrl configures the pad to GPIO function, once DDR_EN is set, the
DLL delay will be fixed based on the GPIO function loopback path. When
the pinctrl is later changed to USDHC function, the internal loopback
path changes, making the original fixed sample point no longer suitable
for the current loopback path. This causes persistent read CRC errors on
subsequent data transfers.

SD/eMMC running in DDR mode are unaffected as they are fully
re-initialized from legacy timing after resume.

Fix this by restoring the pinctrl state based on current timing mode
using esdhc_change_pinstate() before pm_runtime_force_resume(). This
ensures the correct pin configuration (e.g., 100/200MHz for UHS modes)
is applied before DDR_EN is set. Only restore for non-wakeup devices
since wakeup devices kept their active pin state during suspend.

Fixes: 676a83855614 ("mmc: host: sdhci-esdhc-imx: refactor the system PM logic")
Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index ead4685d621a..a54da5757acc 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -2115,6 +2115,12 @@ static int sdhci_esdhc_resume(struct device *dev)
 	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
 	int ret;
 
+	if (!device_may_wakeup(dev)) {
+		ret = esdhc_change_pinstate(host, host->timing);
+		if (ret)
+			dev_warn(dev, "Failed to restore pinctrl state\n");
+	}
+
 	pm_runtime_force_resume(dev);
 
 	ret = mmc_gpio_set_cd_wake(host->mmc, false);
-- 
2.34.1

[PATCH v5 5/8] mmc: sdhci-esdhc-imx: disable irq during suspend to fix unhandled interrupt

[ziniu.wang_1]

From: Luke Wang <ziniu.wang_1@nxp.com>

When using WIFI out-of-band wakeup, an "irq xxx: nobody cared" warning
occurs. This happens because the usdhc interrupt is not disabled during
system suspend when device_may_wakeup() returns false.

The sequence of events leading to this issue:
1. System enters suspend without disabling usdhc interrupt
(because device_may_wakeup() returns false for usdhc device)
2. WIFI out-of-band wakeup triggers system resume via GPIO interrupt
3. WIFI sends a Card interrupt before usdhc has fully resumed
4. usdhc is still in runtime suspend state and cannot handle the
interrupt properly
5. The unhandled interrupt triggers "nobody cared" warning

Fix this by unconditionally disabling the usdhc interrupt during suspend
and re-enabling it during resume, regardless of the wakeup capability.
This ensures no interrupts are processed during the suspend/resume
transition.

Fixes: 676a83855614 ("mmc: host: sdhci-esdhc-imx: refactor the system PM logic")
Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index a54da5757acc..d980dea9cad1 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -2077,9 +2077,10 @@ static int sdhci_esdhc_suspend(struct device *dev)
 	if (mmc_card_keep_power(host->mmc) && esdhc_is_usdhc(imx_data))
 		sdhc_esdhc_tuning_save(host);
 
+	/* The irqs of imx are not shared. It is safe to disable */
+	disable_irq(host->irq);
+
 	if (device_may_wakeup(dev)) {
-		/* The irqs of imx are not shared. It is safe to disable */
-		disable_irq(host->irq);
 		ret = sdhci_enable_irq_wakeups(host);
 		if (!ret)
 			dev_warn(dev, "Failed to enable irq wakeup\n");
@@ -2130,10 +2131,10 @@ static int sdhci_esdhc_resume(struct device *dev)
 	/* re-initialize hw state in case it's lost in low power mode */
 	sdhci_esdhc_imx_hwinit(host);
 
-	if (host->irq_wake_enabled) {
+	if (host->irq_wake_enabled)
 		sdhci_disable_irq_wakeups(host);
-		enable_irq(host->irq);
-	}
+
+	enable_irq(host->irq);
 
 	/*
 	 * restore the saved tuning delay value for the device which keep
-- 
2.34.1

[PATCH v5 6/8] mmc: sdhci-esdhc-imx: use pm_runtime_resume_and_get() in suspend

[ziniu.wang_1]

From: Luke Wang <ziniu.wang_1@nxp.com>

Replace pm_runtime_get_sync() with pm_runtime_resume_and_get() to
simplify error handling. pm_runtime_resume_and_get() automatically
drops the usage counter on failure, avoiding the need for a separate
pm_runtime_put_noidle() call. If it fails, the device is unclocked and
accessing hardware registers would cause a kernel panic, so return the
error immediately.

Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index d980dea9cad1..7818f351a58a 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -2061,7 +2061,9 @@ static int sdhci_esdhc_suspend(struct device *dev)
 	 * 2, make sure the pm_runtime_force_resume() in sdhci_esdhc_resume() really
 	 *    invoke its ->runtime_resume callback (needs_force_resume = 1).
 	 */
-	pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret)
+		return ret;
 
 	if ((imx_data->socdata->flags & ESDHC_FLAG_STATE_LOST_IN_LPMODE) &&
 		(host->tuning_mode != SDHCI_TUNING_MODE_1)) {
-- 
2.34.1

[PATCH v5 7/8] mmc: sdhci-esdhc-imx: make non-fatal errors non-blocking in suspend

[ziniu.wang_1]

From: Luke Wang <ziniu.wang_1@nxp.com>

Make pinctrl_pm_select_sleep_state() and mmc_gpio_set_cd_wake() failures
non-fatal in the suspend path. These failures only mean slightly higher
power consumption or missing CD wakeup capability, but should not block
system suspend.

Also change the function to always return 0 on the success path instead
of propagating non-fatal warning return values.

Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 7818f351a58a..6cb84690fde4 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -2097,10 +2097,12 @@ static int sdhci_esdhc_suspend(struct device *dev)
 		 */
 		ret = pinctrl_pm_select_sleep_state(dev);
 		if (ret)
-			return ret;
+			dev_warn(dev, "Failed to select sleep pinctrl state\n");
 	}
 
 	ret = mmc_gpio_set_cd_wake(host->mmc, true);
+	if (ret)
+		dev_warn(dev, "Failed to enable cd wake\n");
 
 	/*
 	 * Make sure invoke runtime_suspend to gate off clock.
@@ -2108,7 +2110,7 @@ static int sdhci_esdhc_suspend(struct device *dev)
 	 */
 	pm_runtime_force_suspend(dev);
 
-	return ret;
+	return 0;
 }
 
 static int sdhci_esdhc_resume(struct device *dev)
-- 
2.34.1

[PATCH v5 8/8] mmc: sdhci-esdhc-imx: fix resume error handling

[ziniu.wang_1]

From: Luke Wang <ziniu.wang_1@nxp.com>

Check pm_runtime_force_resume() return value in resume. If it fails
(clock enable failure), return immediately since accessing hardware
registers on an unclocked device would cause a kernel panic.

The early return intentionally skips enable_irq() and
sdhci_disable_irq_wakeups() because the IRQ handler reads
SDHCI_INT_STATUS, which would also fault without clocks. The PM runtime
usage counter leak only affects this already-broken device instance and
is an acceptable tradeoff to preserve system stability.

Remove the return value check for mmc_gpio_set_cd_wake(host->mmc, false)
since disable_irq_wake() called internally always returns 0.

Also return 0 explicitly on the success path instead of propagating
stale return values.

Signed-off-by: Luke Wang <ziniu.wang_1@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 6cb84690fde4..f4bfb14423e4 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -2126,12 +2126,12 @@ static int sdhci_esdhc_resume(struct device *dev)
 			dev_warn(dev, "Failed to restore pinctrl state\n");
 	}
 
-	pm_runtime_force_resume(dev);
-
-	ret = mmc_gpio_set_cd_wake(host->mmc, false);
+	ret = pm_runtime_force_resume(dev);
 	if (ret)
 		return ret;
 
+	mmc_gpio_set_cd_wake(host->mmc, false);
+
 	/* re-initialize hw state in case it's lost in low power mode */
 	sdhci_esdhc_imx_hwinit(host);
 
@@ -2158,7 +2158,7 @@ static int sdhci_esdhc_resume(struct device *dev)
 
 	pm_runtime_put_autosuspend(dev);
 
-	return ret;
+	return 0;
 }
 
 static int sdhci_esdhc_runtime_suspend(struct device *dev)
-- 
2.34.1

Re: [PATCH v5 6/8] mmc: sdhci-esdhc-imx: use pm_runtime_resume_and_get() in suspend

[sashiko-bot]

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

Pre-existing issues:
- [High] The exact same unchecked pm_runtime_get_sync() bug that this patch fixes in sdhci_esdhc_suspend() is present in sdhci_esdhc_imx_remove().
- [High] Ignoring pm_runtime_force_resume() failure in sdhci_esdhc_resume() can lead to unclocked register access and a kernel panic.
--

--- Patch [6]: [PATCH v5 6/8] mmc: sdhci-esdhc-imx: use pm_runtime_resume_and_get() in suspend ---
commit 96d8d6ad1f0e2f2a06d51f1a48050880d936f2ca
Author: Luke Wang <ziniu.wang_1@nxp.com>

mmc: sdhci-esdhc-imx: use pm_runtime_resume_and_get() in suspend

Replace pm_runtime_get_sync() with pm_runtime_resume_and_get() to
simplify error handling. pm_runtime_resume_and_get() automatically
drops the usage counter on failure, avoiding the need for a separate
pm_runtime_put_noidle() call. If it fails, the device is unclocked and
accessing hardware registers would cause a kernel panic, so return the
error immediately.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index d980dea9cad1f..7818f351a58ae 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -2061,7 +2061,9 @@ static int sdhci_esdhc_suspend(struct device *dev)
>  	 * 2, make sure the pm_runtime_force_resume() in sdhci_esdhc_resume() really
>  	 *    invoke its ->runtime_resume callback (needs_force_resume = 1).
>  	 */
> -	pm_runtime_get_sync(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
>  
>  	if ((imx_data->socdata->flags & ESDHC_FLAG_STATE_LOST_IN_LPMODE) &&
>  		(host->tuning_mode != SDHCI_TUNING_MODE_1)) {

[Severity: High]
This is a pre-existing issue, but I noticed similar unchecked runtime PM calls
in this driver that could lead to the same unclocked register access panics
mentioned in the commit message.

In sdhci_esdhc_imx_remove(), the return value of pm_runtime_get_sync() is
ignored immediately before performing a hardware register read:

drivers/mmc/host/sdhci-esdhc-imx.c:sdhci_esdhc_imx_remove() {
	...
	pm_runtime_get_sync(&pdev->dev);
	dead = (readl(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff);
	...
}

If pm_runtime_get_sync() fails to resume the device (for example, due to
clk_prepare_enable() failing) during driver unbind, it returns an error. Does
ignoring this error and immediately reading SDHCI_INT_STATUS cause a
synchronous external abort that crashes the system?

[Severity: High]
This isn't a bug introduced by this patch, but a similar issue exists in the
system resume path.

In sdhci_esdhc_resume(), pm_runtime_force_resume() is called and its error is
ignored immediately before hardware register initialization:

drivers/mmc/host/sdhci-esdhc-imx.c:sdhci_esdhc_resume() {
	...
	pm_runtime_force_resume(dev);

	ret = mmc_gpio_set_cd_wake(host->mmc, false);
	if (ret)
		return ret;

	/* re-initialize hw state in case it's lost in low power mode */
	sdhci_esdhc_imx_hwinit(host);
	...
}

If pm_runtime_force_resume() fails and returns an error, the driver
proceeds to call sdhci_esdhc_imx_hwinit(), which writes to hardware
registers like ESDHC_WTMK_LVL. Could accessing these registers while
clocks are gated off cause a synchronous external abort on i.MX platforms?

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

Re: [PATCH v5 7/8] mmc: sdhci-esdhc-imx: make non-fatal errors non-blocking in suspend

[sashiko-bot]

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

Pre-existing issues:
- [High] Ignoring `pm_runtime_force_suspend()` failure leads to an unbalanced PM state and potential system crash during resume.
--

--- Patch [7]: [PATCH v5 7/8] mmc: sdhci-esdhc-imx: make non-fatal errors non-blocking in suspend ---
commit 4c281f4618f6273e8644ab8fb66535c0ebe7c6e2
Author: Luke Wang <ziniu.wang_1@nxp.com>

mmc: sdhci-esdhc-imx: make non-fatal errors non-blocking in suspend

This commit modifies the suspend path of the sdhci-esdhc-imx driver to
make failures in pinctrl_pm_select_sleep_state() and mmc_gpio_set_cd_wake()
non-fatal. It logs warnings instead of returning an error for these cases
and changes sdhci_esdhc_suspend() to always return 0 on completion.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 7818f351a58ae..6cb84690fde4e 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
[ ... ]
> @@ -2108,7 +2110,7 @@ static int sdhci_esdhc_suspend(struct device *dev)
>  	 */
>  	pm_runtime_force_suspend(dev);
>  
> -	return ret;
> +	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but does ignoring the return value of
pm_runtime_force_suspend() lead to an unbalanced PM state?

If pm_runtime_force_suspend() fails (for example, if cqhci_suspend() times
out and returns -ETIMEDOUT), the device's runtime PM status will remain
RPM_ACTIVE, but the suspend process continues.

Upon system resume, sdhci_esdhc_resume() calls pm_runtime_force_resume().
Because pm_runtime_force_resume() checks pm_runtime_status_suspended(), it
will see RPM_ACTIVE and assume the device is already active, skipping the
driver's runtime_resume callback.

Since the runtime_resume callback is skipped, cqhci_resume() is never called
to reinitialize the Command Queuing Engine (CQE), and clock enablement might
also be skipped. Subsequent accesses to the hardware in
sdhci_esdhc_imx_hwinit() or later MMC operations could then trigger a
synchronous external abort (if clocks are off) or lead to I/O errors
(due to an uninitialized CQE).

If the error from pm_runtime_force_suspend() is propagated instead, shouldn't
the driver also roll back the operations performed earlier in
sdhci_esdhc_suspend(), specifically disable_irq() and
pm_runtime_resume_and_get(), since the PM core will abort the suspend and
will not call sdhci_esdhc_resume() for this device?

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

Re: [PATCH v5 2/8] mmc: sdhci-esdhc-imx: restore DLL override for DDR modes on resume

[sashiko-bot]

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

Pre-existing issues:
- [High] Unbalanced `disable_irq()` on system resume if `sdhci_enable_irq_wakeups()` fails or returns false.
--

--- Patch [2]: [PATCH v5 2/8] mmc: sdhci-esdhc-imx: restore DLL override for DDR modes on resume ---
commit 61cde7a0a044133654432ad08827554658f6a4b1
Author: Luke Wang <ziniu.wang_1@nxp.com>

mmc: sdhci-esdhc-imx: restore DLL override for DDR modes on resume

This commit extracts the DLL override setup into a helper function and
calls it during the resume path for SDIO devices operating in DDR timing
modes that keep power.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 6526d65538de5..7230d70e02ae1 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c

[ ... ]

> @@ -2123,9 +2131,18 @@ static int sdhci_esdhc_resume(struct device *dev)

[Severity: High]
This is a pre-existing issue, but could this code lead to an unbalanced
disable_irq() on system resume?

Looking at sdhci_esdhc_suspend(), the IRQ is unconditionally disabled
if device_may_wakeup(dev) is true:

    disable_irq(host->irq);
    ret = sdhci_enable_irq_wakeups(host);

However, if sdhci_enable_irq_wakeups() fails or returns false (for example,
if no wake-capable SDIO card is present and CD IRQ wakeup is disabled),
host->irq_wake_enabled remains false.

During sdhci_esdhc_resume(), enable_irq() appears to be executed conditionally
based on host->irq_wake_enabled:

    if (host->irq_wake_enabled) {
        sdhci_disable_irq_wakeups(host);
        enable_irq(host->irq);
    }

If host->irq_wake_enabled is false, would the re-enablement be skipped?
Could this permanently disable the SDHCI controller's main interrupt after a
system suspend/resume cycle, rendering the MMC controller unusable and
potentially leading to a system crash if it is the rootfs device?

>  	 * restore the saved tuning delay value for the device which keep
>  	 * power during system PM.
>  	 */
> -	if (mmc_card_keep_power(host->mmc) && esdhc_is_usdhc(imx_data))
> +	if (mmc_card_keep_power(host->mmc) && esdhc_is_usdhc(imx_data)) {
>  		sdhc_esdhc_tuning_restore(host);
>  
> +		/*
> +		 * Restore DLL override for DDR modes. hwinit unconditionally
> +		 * clears ESDHC_DLL_CTRL, but the card is still in DDR mode.
> +		 */
> +		if (host->timing == MMC_TIMING_UHS_DDR50 ||
> +		    host->timing == MMC_TIMING_MMC_DDR52)
> +			esdhc_set_dll_override(host);
> +	}
> +
>  	pm_runtime_put_autosuspend(dev);
>  
>  	return ret;

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

Re: [PATCH v5 4/8] mmc: sdhci-esdhc-imx: restore pinctrl before restoring ios timing on resume

[sashiko-bot]

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

New issues:
- [Medium] The patch unconditionally logs a warning if `esdhc_change_pinstate` returns an error, which will spam dmesg on every system resume for platforms that do not define pinctrl or specific pinctrl states.

Pre-existing issues:
- [High] Return value of `pm_runtime_force_resume()` is ignored, which can lead to an external abort (kernel panic) if it fails.
- [High] Main SDHCI interrupt remains permanently disabled after system resume if wakeup setup fails or is unconfigured.
--

--- Patch [4]: [PATCH v5 4/8] mmc: sdhci-esdhc-imx: restore pinctrl before restoring ios timing on resume ---
commit 512978c986e4d08be55744583a21c90ad70f4d54
Author: Luke Wang <ziniu.wang_1@nxp.com>

mmc: sdhci-esdhc-imx: restore pinctrl before restoring ios timing on resume

SDIO devices such as WiFi may keep power during suspend, so the MMC
core skips full card re-initialization on resume and directly restores
the host controller's ios timing to match the card. This patch restores
the pinctrl state based on current timing mode before
pm_runtime_force_resume() to prevent persistent read CRC errors.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -2115,6 +2115,12 @@ static int sdhci_esdhc_resume(struct device *dev)
>  	struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
>  	int ret;
>  
> +	if (!device_may_wakeup(dev)) {
> +		ret = esdhc_change_pinstate(host, host->timing);
> +		if (ret)
> +			dev_warn(dev, "Failed to restore pinctrl state\n");

[Severity: Medium]
Will this unconditionally log a warning and spam dmesg on every system resume
for platforms that do not define pinctrl or specific pinctrl states? If
esdhc_change_pinstate() returns an error (like -EINVAL when imx_data->pinctrl
or pins_100mhz is ERR_PTR), it appears this warning will always be printed.

> +	}
> +
>  	pm_runtime_force_resume(dev);

[Severity: High]
This is a pre-existing issue, but does ignoring the return value of
pm_runtime_force_resume() allow execution to proceed with disabled clocks?

If pm_runtime_force_resume() fails to enable the clocks (e.g., in
sdhci_esdhc_runtime_resume()), the subsequent call to sdhci_esdhc_imx_hwinit()
will access hardware registers without an active clock. On i.MX platforms,
this can cause a synchronous external abort and crash the kernel.

>  
>  	ret = mmc_gpio_set_cd_wake(host->mmc, false);

There is also another pre-existing issue in the suspend/resume path regarding
interrupt handling:

[Severity: High]
This isn't a bug introduced by this patch, but can the main SDHCI interrupt
remain permanently disabled after system resume if wakeup setup fails or is
unconfigured?

In sdhci_esdhc_suspend():

    disable_irq(host->irq);
    ret = sdhci_enable_irq_wakeups(host);

If system suspend is initiated on a device where device_may_wakeup(dev) is
true, but sdhci_cd_irq_can_wakeup(host) and mmc_card_wake_sdio_irq(host->mmc)
are both false, sdhci_enable_irq_wakeups() returns false and sets
host->irq_wake_enabled to false.

Then on resume in sdhci_esdhc_resume():

    if (host->irq_wake_enabled) {
        sdhci_disable_irq_wakeups(host);
        enable_irq(host->irq);
    }

Would this skip calling enable_irq() because host->irq_wake_enabled is false,
leaving the main MMC interrupt permanently disabled and causing all command
and data transfers to time out?

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

Re: [PATCH v5 8/8] mmc: sdhci-esdhc-imx: fix resume error handling

[sashiko-bot]

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

New issues:
- [High] The early return delays a kernel panic because the MMC core is not notified that the device is dead, leading to subsequent unclocked hardware accesses.
- [Medium] The card detect GPIO IRQ is incorrectly left configured as a system wake source on the error path.

Pre-existing issues:
- [High] The driver ignores the return value of `pm_runtime_get_sync()` in the unbind/remove path, leading to a kernel panic.
--

--- Patch [8]: [PATCH v5 8/8] mmc: sdhci-esdhc-imx: fix resume error handling ---
commit eb59f1a91caafc261d97666e5d0b0450cef19279
Author: Luke Wang <ziniu.wang_1@nxp.com>

mmc: sdhci-esdhc-imx: fix resume error handling

This commit adds a return value check for pm_runtime_force_resume() in
sdhci_esdhc_resume(). If it fails, it returns early to prevent unclocked
hardware accesses. It also simplifies the return value handling on success.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 6cb84690fde4e..f4bfb14423e4d 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -2126,12 +2126,12 @@ static int sdhci_esdhc_resume(struct device *dev)
>  			dev_warn(dev, "Failed to restore pinctrl state\n");
>  	}
>  
> -	pm_runtime_force_resume(dev);
> -
> -	ret = mmc_gpio_set_cd_wake(host->mmc, false);
> +	ret = pm_runtime_force_resume(dev);
>  	if (ret)
>  		return ret;

[Severity: High]
If we return early here without setting host->flags |= SDHCI_DEVICE_DEAD,
will the MMC core still attempt to access unclocked hardware?

The PM core will continue system resume and eventually invoke
mmc_resume_host() on the child MMC device. The MMC core would then call host
operations like sdhci_set_ios(), which attempt to read and write SDHCI
hardware registers on an unclocked controller. Could this trigger a
synchronous external abort and result in a kernel panic?

[Severity: Medium]
Does this early return skip the GPIO CD wake IRQ cleanup, leaving the
wake source configured?

During sdhci_esdhc_suspend(), the card detect IRQ is configured as a wake
source via mmc_gpio_set_cd_wake(..., true). Because the card detect
configuration interacts with the GPIO controller and has no dependency on
SDHCI clocks, should it be executed on the error path to avoid leaking the
wake source configuration?

>  
> +	mmc_gpio_set_cd_wake(host->mmc, false);
> +
>  	/* re-initialize hw state in case it's lost in low power mode */
>  	sdhci_esdhc_imx_hwinit(host);

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does ignoring the return
value of pm_runtime_get_sync() in sdhci_esdhc_imx_remove() lead to the same
kernel panic on unclocked hardware access?

In drivers/mmc/host/sdhci-esdhc-imx.c:sdhci_esdhc_imx_remove():

	pm_runtime_get_sync(&pdev->dev);
	dead = (readl(host->ioaddr + SDHCI_INT_STATUS) == 0xffffffff);

If the device is in an error state and clocks fail to enable,
pm_runtime_get_sync() returns a negative error. By immediately executing
readl(host->ioaddr + SDHCI_INT_STATUS) on an unclocked device, could this
trigger a synchronous external abort when the device is unbound?

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

Re: [PATCH v5 3/8] mmc: sdhci-esdhc-imx: fix esdhc_change_pinstate() to allow default state restore

[sashiko-bot]

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

Pre-existing issues:
- [High] Pre-existing NULL pointer dereference in `esdhc_change_pinstate()` due to insufficient pointer validation.
--

--- Patch [3]: [PATCH v5 3/8] mmc: sdhci-esdhc-imx: fix esdhc_change_pinstate() to allow default state restore ---
commit a74e8c023dfc4c92edd0793b792f39383f06ef13
Author: Luke Wang <ziniu.wang_1@nxp.com>

mmc: sdhci-esdhc-imx: fix esdhc_change_pinstate() to allow default state restore

This commit moves the IS_ERR checks for pins_100mhz and pins_200mhz into
their respective switch cases. This allows devices with a sleep pinctrl
state but without high-speed pin states to restore their default pin
configuration.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1326,19 +1326,21 @@ static int esdhc_change_pinstate(struct sdhci_host *host,
>  
>  	dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs);
>  
> -	if (IS_ERR(imx_data->pinctrl) ||
> -		IS_ERR(imx_data->pins_100mhz) ||
> -		IS_ERR(imx_data->pins_200mhz))
> +	if (IS_ERR(imx_data->pinctrl))
>  		return -EINVAL;
>  
>  	switch (uhs) {
>  	case MMC_TIMING_UHS_SDR50:
>  	case MMC_TIMING_UHS_DDR50:
> +		if (IS_ERR(imx_data->pins_100mhz))

[Severity: High]
This is a pre-existing issue, but could this lead to a NULL pointer
dereference?

On non-USDHC platforms where the device tree incorrectly advertises UHS
capabilities, pins_100mhz might be uninitialized and left as NULL. Since
IS_ERR() evaluates to false for a NULL pointer, this check will pass,
allowing the NULL pointer to be assigned to pinctrl and later passed to
pinctrl_select_state(), which will panic in pinctrl_commit_state().

Would it be safer to use IS_ERR_OR_NULL() here instead?

> +			return -EINVAL;
>  		pinctrl = imx_data->pins_100mhz;
>  		break;
>  	case MMC_TIMING_UHS_SDR104:
>  	case MMC_TIMING_MMC_HS200:
>  	case MMC_TIMING_MMC_HS400:
> +		if (IS_ERR(imx_data->pins_200mhz))

[Severity: High]
This is also a pre-existing issue, but could the same NULL pointer
dereference happen here?

If pins_200mhz is NULL due to the same misconfigured device tree scenario
mentioned above, this check will also pass, resulting in a panic when NULL
is passed to pinctrl_select_state().

Should this check also be updated to IS_ERR_OR_NULL()?

> +			return -EINVAL;
>  		pinctrl = imx_data->pins_200mhz;
>  		break;
>  	default:

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