| 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: <391c4270-637a-2afb-210d-6b6dfef01efa@intel.com>
Date: Thu, 24 Aug 2023 15:18:23 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: Stanisław Kardach <skardach@...gle.com>
Cc: Sven van Ashbrook <svenva@...omium.org>,
LKML <linux-kernel@...r.kernel.org>, ulf.hansson@...aro.org,
jason.lai@...esyslogic.com.tw,
Renius Chen <reniuschengl@...il.com>,
linux-mmc@...r.kernel.org, greg.tu@...esyslogic.com.tw,
jasonlai.genesyslogic@...il.com, SeanHY.chen@...esyslogic.com.tw,
ben.chuang@...esyslogic.com.tw, victor.shih@...esyslogic.com.tw,
stable@...r.kernel.org
Subject: Re: [PATCH v2] mmc: sdhci-pci-gli: fix LPM negotiation so x86/S0ix
SoCs can suspend
On 24/08/23 14:50, Stanisław Kardach wrote:
> Hi Adrian,
>
> Thanks for reviewing our patches.
>
> On Thu, Aug 24, 2023 at 1:47 PM Adrian Hunter <adrian.hunter@...el.com <mailto:adrian.hunter@...el.com>> wrote:
>
> Hi
>
> Looks OK - a few minor comments below
>
> On 23/08/23 20:41, Sven van Ashbrook wrote:
> > To improve the r/w performance of GL9763E, the current driver inhibits LPM
> > negotiation while the device is active.
> >
> > This prevents a large number of SoCs from suspending, notably x86 systems
>
> If possible, can you give example of which SoCs / products
>
> > which use S0ix as the suspend mechanism:
> > 1. Userspace initiates s2idle suspend (e.g. via writing to
> > /sys/power/state)
> > 2. This switches the runtime_pm device state to active, which disables
> > LPM negotiation, then calls the "regular" suspend callback
> > 3. With LPM negotiation disabled, the bus cannot enter low-power state
> > 4. On a large number of SoCs, if the bus not in a low-power state, S0ix
> > cannot be entered, which in turn prevents the SoC from entering
> > suspend.
> >
> > Fix by re-enabling LPM negotiation in the device's suspend callback.
> >
> > Suggested-by: Stanislaw Kardach <skardach@...gle.com <mailto:skardach@...gle.com>>
> > Fixes: f9e5b33934ce ("mmc: host: Improve I/O read/write performance for GL9763E")
> > Cc: stable@...r.kernel.org <mailto:stable@...r.kernel.org>
> > Signed-off-by: Sven van Ashbrook <svenva@...omium.org <mailto:svenva@...omium.org>>
> > # on gladios device
> > # on 15590.0.0 with v5.10 and upstream (v6.4) kernels
> >
>
> 3 extraneous lines here - please remove
>
> > ---
> >
> > Changes in v2:
> > - improved symmetry and error path in s2idle suspend callback (internal review)
> >
> > drivers/mmc/host/sdhci-pci-gli.c | 102 +++++++++++++++++++------------
> > 1 file changed, 64 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> > index 1792665c9494a..19f577cc8bceb 100644
> > --- a/drivers/mmc/host/sdhci-pci-gli.c
> > +++ b/drivers/mmc/host/sdhci-pci-gli.c
> > @@ -745,42 +745,6 @@ static u32 sdhci_gl9750_readl(struct sdhci_host *host, int reg)
> > return value;
> > }
> >
> > -#ifdef CONFIG_PM_SLEEP
> > -static int sdhci_pci_gli_resume(struct sdhci_pci_chip *chip)
> > -{
> > - struct sdhci_pci_slot *slot = chip->slots[0];
> > -
> > - pci_free_irq_vectors(slot->chip->pdev);
> > - gli_pcie_enable_msi(slot);
> > -
> > - return sdhci_pci_resume_host(chip);
> > -}
> > -
> > -static int sdhci_cqhci_gli_resume(struct sdhci_pci_chip *chip)
> > -{
> > - struct sdhci_pci_slot *slot = chip->slots[0];
> > - int ret;
> > -
> > - ret = sdhci_pci_gli_resume(chip);
> > - if (ret)
> > - return ret;
> > -
> > - return cqhci_resume(slot->host->mmc);
> > -}
> > -
> > -static int sdhci_cqhci_gli_suspend(struct sdhci_pci_chip *chip)
> > -{
> > - struct sdhci_pci_slot *slot = chip->slots[0];
> > - int ret;
> > -
> > - ret = cqhci_suspend(slot->host->mmc);
> > - if (ret)
> > - return ret;
> > -
> > - return sdhci_suspend_host(slot->host);
> > -}
> > -#endif
> > -
> > static void gl9763e_hs400_enhanced_strobe(struct mmc_host *mmc,
> > struct mmc_ios *ios)
> > {
> > @@ -1029,6 +993,68 @@ static int gl9763e_runtime_resume(struct sdhci_pci_chip *chip)
> > }
> > #endif
> >
> > +#ifdef CONFIG_PM_SLEEP
> > +static int sdhci_pci_gli_resume(struct sdhci_pci_chip *chip)
> > +{
> > + struct sdhci_pci_slot *slot = chip->slots[0];
> > +
> > + pci_free_irq_vectors(slot->chip->pdev);
> > + gli_pcie_enable_msi(slot);
> > +
> > + return sdhci_pci_resume_host(chip);
> > +}
> > +
> > +static int gl9763e_resume(struct sdhci_pci_chip *chip)
> > +{
> > + struct sdhci_pci_slot *slot = chip->slots[0];
> > + int ret;
> > +
> > + ret = sdhci_pci_gli_resume(chip);
> > + if (ret)
> > + return ret;
> > +
> > + ret = cqhci_resume(slot->host->mmc);
> > + if (ret)
> > + return ret;
> > +
> > + /* Disable LPM negotiation to bring device back in sync
> > + * with its runtime_pm state.
> > + */
>
> I would prefer the comment style:
>
> /*
> * Blah, blah ...
> * Blah, blah, blah.
> */
>
> > + gl9763e_set_low_power_negotiation(slot, false);
> > +
> > + return 0;
> > +}
> > +
> > +static int gl9763e_suspend(struct sdhci_pci_chip *chip)
> > +{
> > + struct sdhci_pci_slot *slot = chip->slots[0];
> > + int ret;
> > +
> > + /* Certain SoCs can suspend only with the bus in low-
>
> Ditto re comment style
>
> > + * power state, notably x86 SoCs when using S0ix.
> > + * Re-enable LPM negotiation to allow entering L1 state
> > + * and entering system suspend.
> > + */
> > + gl9763e_set_low_power_negotiation(slot, true);
>
> Couldn't this be at the end of the function, save
> an error path
>
> Please correct me if I'm wrong but writing to device config
> space could trigger a side effect, so it's probably better to
> do it before calling functions suspending the device?
sdhci doesn't know anything about the bus. It is independent
of PCI, so I can't see how it would make any difference.
One of the people cc'ed might know more. Jason Lai (cc'ed)
added it for runtime PM.
>
>
> > +
> > + ret = cqhci_suspend(slot->host->mmc);
> > + if (ret)
> > + goto err_suspend;
> > +
> > + ret = sdhci_suspend_host(slot->host);
> > + if (ret)
> > + goto err_suspend_host;
> > +
> > + return 0;
> > +
> > +err_suspend_host:
> > + cqhci_resume(slot->host->mmc);
> > +err_suspend:
> > + gl9763e_set_low_power_negotiation(slot, false);
> > + return ret;
> > +}
> > +#endif
> > +
> > static int gli_probe_slot_gl9763e(struct sdhci_pci_slot *slot)
> > {
> > struct pci_dev *pdev = slot->chip->pdev;
> > @@ -1113,8 +1139,8 @@ const struct sdhci_pci_fixes sdhci_gl9763e = {
> > .probe_slot = gli_probe_slot_gl9763e,
> > .ops = &sdhci_gl9763e_ops,
> > #ifdef CONFIG_PM_SLEEP
> > - .resume = sdhci_cqhci_gli_resume,
> > - .suspend = sdhci_cqhci_gli_suspend,
> > + .resume = gl9763e_resume,
> > + .suspend = gl9763e_suspend,
> > #endif
> > #ifdef CONFIG_PM
> > .runtime_suspend = gl9763e_runtime_suspend,
>
>
>
> --
> Best Regards,
> Stanisław Kardach
Powered by blists - more mailing lists