| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAPDyKFp+ATxghU2g1XxVLB1Oy4TkFvUqj6SpyKCV_ro9bayXtQ@mail.gmail.com>
Date: Mon, 19 May 2025 14:31:34 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: ziniu.wang_1@....com, Adrian Hunter <adrian.hunter@...el.com>
Cc: linux-mmc@...r.kernel.org, haibo.chen@....com, shawnguo@...nel.org,
s.hauer@...gutronix.de, kernel@...gutronix.de, festevam@...il.com,
imx@...ts.linux.dev, s32@....com, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] mmc: host: sdhci-esdhc-imx: refactor the system PM logic
On Thu, 15 May 2025 at 14:53, Adrian Hunter <adrian.hunter@...el.com> wrote:
>
> On 14/05/2025 12:49, ziniu.wang_1@....com wrote:
> > From: Luke Wang <ziniu.wang_1@....com>
> >
> > Current suspend/resume logic has one issue. In suspend, will config
> > register when call sdhci_suspend_host(), but at this time, can't
> > guarantee host in runtime resume state. If not, the per clock is gate
> > off, access register will hang.
> >
> > In sdhci_esdhc_suspend/sdhci_esdhc_resume, remove sdhci_suspend_host()
> > and sdhci_resume_host(), all are handled in runtime PM callbacks except
> > the wakeup irq setting. For wakeup irq setting, use pm_runtime_get_sync()
> > in sdhci_esdhc_suspend() to make sure clock gate on.
> >
> > Remove pinctrl_pm_select_default_state() in sdhci_esdhc_resume, because
> > pm_runtime_force_resume() already config the pinctrl state according to
> > ios timing, and here config the default pinctrl state again is wrong for
> > SDIO3.0 device if it keep power in suspend.
> >
> > Signed-off-by: Haibo Chen <haibo.chen@....com>
> > Signed-off-by: Luke Wang <ziniu.wang_1@....com>
>
> Looks OK to me, at least, although Ulf may still have comments.
> Otherwise, for both patches:
Looks good to me too!
>
> Acked-by: Adrian Hunter <adrian.hunter@...el.com>
Both patches applied for next, thanks!
Kind regards
Uffe
>
> > ---
> > drivers/mmc/host/sdhci-esdhc-imx.c | 50 ++++++++++++++++++------------
> > 1 file changed, 30 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> > index c0160c69a027..7611682f10c3 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -2009,11 +2009,14 @@ static int sdhci_esdhc_suspend(struct device *dev)
> > struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
> > int ret;
> >
> > - if (host->mmc->caps2 & MMC_CAP2_CQE) {
> > - ret = cqhci_suspend(host->mmc);
> > - if (ret)
> > - return ret;
> > - }
> > + /*
> > + * Switch to runtime resume for two reasons:
> > + * 1, there is register access (e.g., wakeup control register), so
> > + * need to make sure gate on ipg clock.
> > + * 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);
> >
> > if ((imx_data->socdata->flags & ESDHC_FLAG_STATE_LOST_IN_LPMODE) &&
> > (host->tuning_mode != SDHCI_TUNING_MODE_1)) {
> > @@ -2021,9 +2024,6 @@ static int sdhci_esdhc_suspend(struct device *dev)
> > mmc_retune_needed(host->mmc);
> > }
> >
> > - if (host->tuning_mode != SDHCI_TUNING_MODE_3)
> > - mmc_retune_needed(host->mmc);
> > -
> > /*
> > * For the device need to keep power during system PM, need
> > * to save the tuning delay value just in case the usdhc
> > @@ -2033,9 +2033,13 @@ static int sdhci_esdhc_suspend(struct device *dev)
> > esdhc_is_usdhc(imx_data))
> > sdhc_esdhc_tuning_save(host);
> >
> > - ret = sdhci_suspend_host(host);
> > - if (ret)
> > - return ret;
> > + 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");
> > + }
> >
> > ret = pinctrl_pm_select_sleep_state(dev);
> > if (ret)
> > @@ -2043,6 +2047,12 @@ static int sdhci_esdhc_suspend(struct device *dev)
> >
> > ret = mmc_gpio_set_cd_wake(host->mmc, true);
> >
> > + /*
> > + * Make sure invoke runtime_suspend to gate off clock.
> > + * uSDHC IP supports in-band SDIO wakeup even without clock.
> > + */
> > + pm_runtime_force_suspend(dev);
> > +
> > return ret;
> > }
> >
> > @@ -2053,16 +2063,19 @@ static int sdhci_esdhc_resume(struct device *dev)
> > struct pltfm_imx_data *imx_data = sdhci_pltfm_priv(pltfm_host);
> > int ret;
> >
> > - ret = pinctrl_pm_select_default_state(dev);
> > + 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);
> >
> > - ret = sdhci_resume_host(host);
> > - if (ret)
> > - return ret;
> > + if (host->irq_wake_enabled) {
> > + sdhci_disable_irq_wakeups(host);
> > + enable_irq(host->irq);
> > + }
> >
> > /*
> > * restore the saved tuning delay value for the device which keep
> > @@ -2072,11 +2085,8 @@ static int sdhci_esdhc_resume(struct device *dev)
> > esdhc_is_usdhc(imx_data))
> > sdhc_esdhc_tuning_restore(host);
> >
> > - if (host->mmc->caps2 & MMC_CAP2_CQE)
> > - ret = cqhci_resume(host->mmc);
> > -
> > - if (!ret)
> > - ret = mmc_gpio_set_cd_wake(host->mmc, false);
> > + pm_runtime_mark_last_busy(dev);
> > + pm_runtime_put_autosuspend(dev);
> >
> > return ret;
> > }
>
Powered by blists - more mailing lists