| 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: <Yx9kLuY2oGtlJfmq@google.com>
Date: Mon, 12 Sep 2022 16:54:06 +0000
From: Matthias Kaehlcke <mka@...omium.org>
To: Krishna Chaitanya Chundru <quic_krichai@...cinc.com>
Cc: helgaas@...nel.org, linux-pci@...r.kernel.org,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
quic_vbadigan@...cinc.com, quic_hemantk@...cinc.com,
quic_nitegupt@...cinc.com, quic_skananth@...cinc.com,
quic_ramkri@...cinc.com, manivannan.sadhasivam@...aro.org,
swboyd@...omium.org, dmitry.baryshkov@...aro.org,
Stanimir Varbanov <svarbanov@...sol.com>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...ainline.org>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>,
Bjorn Helgaas <bhelgaas@...gle.com>
Subject: Re: [PATCH v6 1/5] PCI: qcom: Add system suspend and resume support
On Mon, Sep 12, 2022 at 09:36:53PM +0530, Krishna Chaitanya Chundru wrote:
>
> On 9/9/2022 11:01 PM, Matthias Kaehlcke wrote:
> > On Fri, Sep 09, 2022 at 02:14:40PM +0530, Krishna chaitanya chundru wrote:
> > > Add suspend and resume syscore ops.
> > >
> > > When system suspends and if the link is in L1ss, disable the clocks
> > > and power down the phy so that system enters into low power state to
> > > save the maximum power. And when the system resumes, enable the clocks
> > > back and power on phy if they are disabled in the suspend path.
> > >
> > > we are doing this only when link is in l1ss but not in L2/L3 as
> > > nowhere we are forcing link to L2/L3 by sending PME turn off.
> > >
> > > is_suspended flag indicates if the clocks are disabled in the suspend
> > > path or not.
> > >
> > > There is access to Ep PCIe space to mask MSI/MSIX after pm suspend ops
> > > (getting hit by affinity changes while making CPUs offline during suspend,
> > > this will happen after devices are suspended (all phases of suspend ops)).
> > > When registered with pm ops there is a crash due to un-clocked access,
> > > as in the pm suspend op clocks are disabled. So, registering with syscore
> > > ops which will called after making CPUs offline.
> > My knowledge of PCI is limited, but given the issues you are seeing which
> > don't seem to impact other DWC drivers I wonder if keeping the link in
> > l1ss is the right thing to do. The intel, tegra and imx6 drivers all turn
> > the PME off, which IIUC results in the link to transition ot L2 or L3.
> > Shouldn't the QC driver do the same?
> The other dwc drivers are trying to turn off PME in the suspend and in
> resume path
> they are trying to do link training again, this is what we have done
> previously
> But this change is rejected by nvme developers because this will decrease
> device life cycle
> (turning off the link and bringing up is treated as a power cycle of nvme).
I see, so the other drivers are currently not really suitable for use with
NVMe :(
For reference, Christoph pointed out here [1] that switching the link off
wears out the flash.
So IIUC keeping the link in >= L1 during system suspend is a requirement
for NVMe and the question is how to achieve that in the dwc/pcie-qcom
drivers without resorting to ugly hacks.
[1] https://lore.kernel.org/all/20220518084356.GA6933@lst.de/#R
> So we are trying this approach.
> >
> > Some more comments inline, for if the current approach moves forward.
> >
> > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@...cinc.com>
> > > ---
> > > changes since v5:
> > > - Rebasing the code and replaced pm ops with syscore ops as
> > > we are getting acciess to pci region after pm ops. syscore ops
> > > will called after disabling non boot cpus and there is no pci
> > > access after that.
> > > Changes since v4:
> > > - Rebasing the code and removed the supports_system_suspend flag
> > > - in the resume path as is_suspended will serve its purpose.
> > > Changes since v3:
> > > - Powering down the phy in suspend and powering it on resume to
> > > acheive maximum power savings.
> > > Changes since v2:
> > > - Replaced the enable, disable clks ops with suspend and resume
> > > - Renamed support_pm_opsi flag with supports_system_suspend.
> > > Changes since v1:
> > > - Fixed compilation errors.
> > > ---
> > > drivers/pci/controller/dwc/pcie-qcom.c | 140 ++++++++++++++++++++++++++++++++-
> > > 1 file changed, 139 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 39ca06f..6e04d0d 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -27,6 +27,7 @@
> > > #include <linux/reset.h>
> > > #include <linux/slab.h>
> > > #include <linux/types.h>
> > > +#include <linux/syscore_ops.h>
> > > #include "../../pci.h"
> > > #include "pcie-designware.h"
> > > @@ -44,6 +45,9 @@
> > > #define PCIE20_PARF_PM_CTRL 0x20
> > > #define REQ_NOT_ENTR_L1 BIT(5)
> > > +#define PCIE20_PARF_PM_STTS 0x24
> > > +#define PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB BIT(8)
> > > +
> > > #define PCIE20_PARF_PHY_CTRL 0x40
> > > #define PHY_CTRL_PHY_TX0_TERM_OFFSET_MASK GENMASK(20, 16)
> > > #define PHY_CTRL_PHY_TX0_TERM_OFFSET(x) ((x) << 16)
> > > @@ -122,6 +126,8 @@
> > > #define QCOM_PCIE_CRC8_POLYNOMIAL (BIT(2) | BIT(1) | BIT(0))
> > > +static LIST_HEAD(qcom_pcie_list);
> > > +
> > > struct qcom_pcie_resources_2_1_0 {
> > > struct clk_bulk_data clks[QCOM_PCIE_2_1_0_MAX_CLOCKS];
> > > struct reset_control *pci_reset;
> > > @@ -211,13 +217,21 @@ struct qcom_pcie_ops {
> > > void (*post_deinit)(struct qcom_pcie *pcie);
> > > void (*ltssm_enable)(struct qcom_pcie *pcie);
> > > int (*config_sid)(struct qcom_pcie *pcie);
> > > + int (*suspend)(struct qcom_pcie *pcie);
> > > + int (*resume)(struct qcom_pcie *pcie);
> > > };
> > > struct qcom_pcie_cfg {
> > > const struct qcom_pcie_ops *ops;
> > > + /*
> > > + * Flag ensures which devices will turn off clks, phy
> > > + * in system suspend.
> > > + */
> > > + unsigned int supports_system_suspend:1;
> > > };
> > > struct qcom_pcie {
> > > + struct list_head list; /* list to probed instances */
> > > struct dw_pcie *pci;
> > > void __iomem *parf; /* DT parf */
> > > void __iomem *elbi; /* DT elbi */
> > > @@ -225,10 +239,14 @@ struct qcom_pcie {
> > > struct phy *phy;
> > > struct gpio_desc *reset;
> > > const struct qcom_pcie_cfg *cfg;
> > > + unsigned int is_suspended:1;
> > > };
> > > #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
> > > +static int __maybe_unused qcom_pcie_syscore_op_suspend(void);
> > > +static void __maybe_unused qcom_pcie_syscore_op_resume(void);
> > > +
> > > static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> > > {
> > > gpiod_set_value_cansleep(pcie->reset, 1);
> > > @@ -1301,6 +1319,28 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
> > > regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
> > > }
> > > +static int qcom_pcie_resume_2_7_0(struct qcom_pcie *pcie)
> > > +{
> > > + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > > + int ret;
> > > +
> > > + ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> > > +
> > > + phy_power_on(pcie->phy);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +static int qcom_pcie_suspend_2_7_0(struct qcom_pcie *pcie)
> > > +{
> > > + struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> > > +
> > > + phy_power_off(pcie->phy);
> > > +
> > > + clk_bulk_disable_unprepare(res->num_clks, res->clks);
> > > + return 0;
> > > +}
> > > +
> > > static int qcom_pcie_get_resources_2_9_0(struct qcom_pcie *pcie)
> > > {
> > > struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> > > @@ -1594,6 +1634,8 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
> > > .deinit = qcom_pcie_deinit_2_7_0,
> > > .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> > > .config_sid = qcom_pcie_config_sid_sm8250,
> > > + .suspend = qcom_pcie_suspend_2_7_0,
> > > + .resume = qcom_pcie_resume_2_7_0,
> > > };
> > > /* Qcom IP rev.: 2.9.0 Synopsys IP rev.: 5.00a */
> > > @@ -1613,6 +1655,11 @@ static const struct qcom_pcie_cfg cfg_1_9_0 = {
> > > .ops = &ops_1_9_0,
> > > };
> > > +static const struct qcom_pcie_cfg sc7280_cfg = {
> > > + .ops = &ops_1_9_0,
> > > + .supports_system_suspend = true,
> > > +};
> > > +
> > > static const struct qcom_pcie_cfg cfg_2_1_0 = {
> > > .ops = &ops_2_1_0,
> > > };
> > > @@ -1642,6 +1689,23 @@ static const struct dw_pcie_ops dw_pcie_ops = {
> > > .start_link = qcom_pcie_start_link,
> > > };
> > > +/*
> > > + * There is access to Ep PCIe space to mask MSI/MSIX after pm suspend
> > > + * ops.(getting hit by affinity changes while making CPUs offline during
> > > + * suspend, this will happen after devices are suspended
> > > + * (all phases of suspend ops)).
> > > + *
> > > + * When registered with pm ops there is a crash due to un-clocked access,
> > > + * as in the pm suspend op clocks are disabled.
> > > + *
> > > + * So, registering with syscore ops which will called after making
> > > + * CPU's offline.
> > > + */
> > > +static struct syscore_ops qcom_pcie_syscore_ops = {
> > > + .suspend = qcom_pcie_syscore_op_suspend,
> > > + .resume = qcom_pcie_syscore_op_resume,
> > > +};
> > > +
> > > static int qcom_pcie_probe(struct platform_device *pdev)
> > > {
> > > struct device *dev = &pdev->dev;
> > > @@ -1720,6 +1784,17 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > goto err_phy_exit;
> > > }
> > > + /* Register for syscore ops only when first instance probed */
> > > + if (list_empty(&qcom_pcie_list))
> > > + register_syscore_ops(&qcom_pcie_syscore_ops);
> > > +
> > > + /*
> > > + * Add the qcom_pcie list of each PCIe instance probed to
> > > + * the global list so that we use it iterate through each PCIe
> > > + * instance in the syscore ops.
> > > + */
> > > + list_add_tail(&pcie->list, &qcom_pcie_list);
> > > +
> > > return 0;
> > > err_phy_exit:
> > > @@ -1731,6 +1806,69 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > return ret;
> > > }
> > > +static int __maybe_unused qcom_pcie_pm_suspend(struct qcom_pcie *pcie)
> > > +{
> > > + u32 val;
> > > + struct dw_pcie *pci = pcie->pci;
> > > + struct device *dev = pci->dev;
> > > +
> > > + if (!pcie->cfg->supports_system_suspend)
> > > + return 0;
> > This check could be done in qcom_pcie_syscore_op_suspend()
> sure , we will take of it in next patch.
> >
> > > +
> > > + /* if the link is not active turn off clocks */
> > > + if (!dw_pcie_link_up(pci)) {
> > > + dev_info(dev, "Link is not active\n");
> > level info seems to verbose, it's not particularly interesting that the
> > link is not active, except for debugging.
> sure , we will take of it in next patch.
> >
> > > + goto suspend;
> > > + }
> > > +
> > > + /* if the link is not in l1ss don't turn off clocks */
> > > + val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> > > + if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> > > + dev_warn(dev, "Link is not in L1ss\n");
> > > + return 0;
> > > + }
> > I think the following would be clearer:
> >
> > if (dw_pcie_link_up(pci)) {
> > /* if the link is not in l1ss don't turn off clocks */
> > val = readl(pcie->parf + PCIE20_PARF_PM_STTS);
> > if (!(val & PCIE20_PARF_PM_STTS_LINKST_IN_L1SUB)) {
> > dev_warn(dev, "Link is not in L1ss\n");
> > return 0;
> > }
> > } else {
> > dev_dbg(dev, "Link is not active\n");
> > }
> But as we are adding retry logic in other patch with while loop this logic
> will not be applicable.
> > > +
> > > +suspend:
> > > + if (pcie->cfg->ops->suspend)
> > > + pcie->cfg->ops->suspend(pcie);
> > > +
> > > + pcie->is_suspended = true;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int __maybe_unused qcom_pcie_pm_resume(struct qcom_pcie *pcie)
> > > +{
> > > + if (!pcie->is_suspended)
> > > + return 0;
> > > +
> > > + if (pcie->cfg->ops->resume)
> > > + pcie->cfg->ops->resume(pcie);
> > > +
> > > + pcie->is_suspended = false;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int __maybe_unused qcom_pcie_syscore_op_suspend(void)
> > > +{
> > > + struct qcom_pcie *qcom_pcie;
> > > +
> > > + list_for_each_entry(qcom_pcie, &qcom_pcie_list, list) {
> > > + qcom_pcie_pm_suspend(qcom_pcie);
> > > + }
> > > + return 0;
> > > +}
> > > +
> > > +static void __maybe_unused qcom_pcie_syscore_op_resume(void)
> > > +{
> > > + struct qcom_pcie *qcom_pcie;
> > > +
> > > + list_for_each_entry(qcom_pcie, &qcom_pcie_list, list) {
> > > + qcom_pcie_pm_resume(qcom_pcie);
> > > + }
> > > +}
> > > +
> > > static const struct of_device_id qcom_pcie_match[] = {
> > > { .compatible = "qcom,pcie-apq8064", .data = &cfg_2_1_0 },
> > > { .compatible = "qcom,pcie-apq8084", .data = &cfg_1_0_0 },
> > > @@ -1742,7 +1880,7 @@ static const struct of_device_id qcom_pcie_match[] = {
> > > { .compatible = "qcom,pcie-msm8996", .data = &cfg_2_3_2 },
> > > { .compatible = "qcom,pcie-qcs404", .data = &cfg_2_4_0 },
> > > { .compatible = "qcom,pcie-sa8540p", .data = &cfg_1_9_0 },
> > > - { .compatible = "qcom,pcie-sc7280", .data = &cfg_1_9_0 },
> > > + { .compatible = "qcom,pcie-sc7280", .data = &sc7280_cfg },
> > > { .compatible = "qcom,pcie-sc8180x", .data = &cfg_1_9_0 },
> > > { .compatible = "qcom,pcie-sc8280xp", .data = &cfg_1_9_0 },
> > > { .compatible = "qcom,pcie-sdm845", .data = &cfg_2_7_0 },
> > > --
> > > 2.7.4
> > >
Powered by blists - more mailing lists