| 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: <mr5kunnsv5syckqbclcw2xi6es36fnsnfvrvughqlmv2kblxof@tltliisb5upf>
Date: Thu, 24 Apr 2025 10:57:02 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
Cc: Mahesh J Salgaonkar <mahesh@...ux.ibm.com>,
Oliver O'Halloran <oohall@...il.com>, Bjorn Helgaas <bhelgaas@...gle.com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kw@...ux.com>,
Rob Herring <robh@...nel.org>, Zhou Wang <wangzhou1@...ilicon.com>,
Will Deacon <will@...nel.org>, Robert Richter <rric@...nel.org>,
Alyssa Rosenzweig <alyssa@...enzweig.io>, Marc Zyngier <maz@...nel.org>,
Conor Dooley <conor.dooley@...rochip.com>, Daire McNamara <daire.mcnamara@...rochip.com>,
dingwei@...vell.com, cassel@...nel.org, Lukas Wunner <lukas@...ner.de>,
linuxppc-dev@...ts.ozlabs.org, linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v3 5/5] PCI: qcom: Add support for resetting the slot due
to link down event
On Thu, Apr 24, 2025 at 10:41:24AM +0530, Krishna Chaitanya Chundru wrote:
>
>
> On 4/24/2025 10:30 AM, Manivannan Sadhasivam wrote:
> > On Fri, Apr 18, 2025 at 08:11:47AM +0530, Krishna Chaitanya Chundru wrote:
> > >
> > >
> > > On 4/17/2025 10:46 PM, Manivannan Sadhasivam via B4 Relay wrote:
> > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > > >
> > > > The PCIe link can go down under circumstances such as the device firmware
> > > > crash, link instability, etc... When that happens, the PCIe slot needs to
> > > > be reset to make it operational again. Currently, the driver is not
> > > > handling the link down event, due to which the users have to restart the
> > > > machine to make PCIe link operational again. So fix it by detecting the
> > > > link down event and resetting the slot.
> > > >
> > > > Since the Qcom PCIe controllers report the link down event through the
> > > > 'global' IRQ, enable the link down event by setting PARF_INT_ALL_LINK_DOWN
> > > > bit in PARF_INT_ALL_MASK register.
> > > >
> > > > Then in the case of the event, call pci_host_handle_link_down() API
> > > > in the handler to let the PCI core handle the link down condition.
> > > >
> > > > The API will internally call, 'pci_host_bridge::reset_slot()' callback to
> > > > reset the slot in a platform specific way. So implement the callback to
> > > > reset the slot by first resetting the PCIe core, followed by reinitializing
> > > > the resources and then finally starting the link again.
> > > >
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
> > > > ---
> > > > drivers/pci/controller/dwc/Kconfig | 1 +
> > > > drivers/pci/controller/dwc/pcie-qcom.c | 90 +++++++++++++++++++++++++++++++++-
> > > > 2 files changed, 89 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > > index d9f0386396edf66ad0e514a0f545ed24d89fcb6c..ce04ee6fbd99cbcce5d2f3a75ebd72a17070b7b7 100644
> > > > --- a/drivers/pci/controller/dwc/Kconfig
> > > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > > @@ -296,6 +296,7 @@ config PCIE_QCOM
> > > > select PCIE_DW_HOST
> > > > select CRC8
> > > > select PCIE_QCOM_COMMON
> > > > + select PCI_HOST_COMMON
> > > > help
> > > > Say Y here to enable PCIe controller support on Qualcomm SoCs. The
> > > > PCIe controller uses the DesignWare core plus Qualcomm-specific
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > index dc98ae63362db0422384b1879a2b9a7dc564d091..6b18a2775e7fcde1d634b3f58327ecc7d028e4ec 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > @@ -34,6 +34,7 @@
> > > > #include <linux/units.h>
> > > > #include "../../pci.h"
> > > > +#include "../pci-host-common.h"
> > > > #include "pcie-designware.h"
> > > > #include "pcie-qcom-common.h"
> > > > @@ -55,6 +56,7 @@
> > > > #define PARF_INT_ALL_STATUS 0x224
> > > > #define PARF_INT_ALL_CLEAR 0x228
> > > > #define PARF_INT_ALL_MASK 0x22c
> > > > +#define PARF_STATUS 0x230
> > > > #define PARF_SID_OFFSET 0x234
> > > > #define PARF_BDF_TRANSLATE_CFG 0x24c
> > > > #define PARF_DBI_BASE_ADDR_V2 0x350
> > > > @@ -130,8 +132,11 @@
> > > > /* PARF_LTSSM register fields */
> > > > #define LTSSM_EN BIT(8)
> > > > +#define SW_CLEAR_FLUSH_MODE BIT(10)
> > > > +#define FLUSH_MODE BIT(11)
> > > > /* PARF_INT_ALL_{STATUS/CLEAR/MASK} register fields */
> > > > +#define PARF_INT_ALL_LINK_DOWN BIT(1)
> > > > #define PARF_INT_ALL_LINK_UP BIT(13)
> > > > #define PARF_INT_MSI_DEV_0_7 GENMASK(30, 23)
> > > > @@ -145,6 +150,9 @@
> > > > /* PARF_BDF_TO_SID_CFG fields */
> > > > #define BDF_TO_SID_BYPASS BIT(0)
> > > > +/* PARF_STATUS fields */
> > > > +#define FLUSH_COMPLETED BIT(8)
> > > > +
> > > > /* ELBI_SYS_CTRL register fields */
> > > > #define ELBI_SYS_CTRL_LT_ENABLE BIT(0)
> > > > @@ -169,6 +177,7 @@
> > > > PCIE_CAP_SLOT_POWER_LIMIT_SCALE)
> > > > #define PERST_DELAY_US 1000
> > > > +#define FLUSH_TIMEOUT_US 100
> > > > #define QCOM_PCIE_CRC8_POLYNOMIAL (BIT(2) | BIT(1) | BIT(0))
> > > > @@ -274,11 +283,14 @@ struct qcom_pcie {
> > > > struct icc_path *icc_cpu;
> > > > const struct qcom_pcie_cfg *cfg;
> > > > struct dentry *debugfs;
> > > > + int global_irq;
> > > > bool suspended;
> > > > bool use_pm_opp;
> > > > };
> > > > #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
> > > > +static int qcom_pcie_reset_slot(struct pci_host_bridge *bridge,
> > > > + struct pci_dev *pdev);
> > > > static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> > > > {
> > > > @@ -1263,6 +1275,8 @@ static int qcom_pcie_host_init(struct dw_pcie_rp *pp)
> > > > goto err_assert_reset;
> > > > }
> > > > + pp->bridge->reset_slot = qcom_pcie_reset_slot;
> > > > +
> > > > return 0;
> > > > err_assert_reset:
> > > > @@ -1300,6 +1314,73 @@ static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> > > > .post_init = qcom_pcie_host_post_init,
> > > > };
> > > > +static int qcom_pcie_reset_slot(struct pci_host_bridge *bridge,
> > > > + struct pci_dev *pdev)
> > > > +{
> > > > + struct pci_bus *bus = bridge->bus;
> > > > + struct dw_pcie_rp *pp = bus->sysdata;
> > > > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > + struct qcom_pcie *pcie = to_qcom_pcie(pci);
> > > > + struct device *dev = pcie->pci->dev;
> > > > + u32 val;
> > > > + int ret;
> > > > +
> > > > + /* Wait for the pending transactions to be completed */
> > > > + ret = readl_relaxed_poll_timeout(pcie->parf + PARF_STATUS, val,
> > > > + val & FLUSH_COMPLETED, 10,
> > > > + FLUSH_TIMEOUT_US);
> > > > + if (ret) {
> > > > + dev_err(dev, "Flush completion failed: %d\n", ret);
> > > > + goto err_host_deinit;
> > > > + }
> > > > +
> > > > + /* Clear the FLUSH_MODE to allow the core to be reset */
> > > > + val = readl(pcie->parf + PARF_LTSSM);
> > > > + val |= SW_CLEAR_FLUSH_MODE;
> > > > + writel(val, pcie->parf + PARF_LTSSM);
> > > > +
> > > > + /* Wait for the FLUSH_MODE to clear */
> > > > + ret = readl_relaxed_poll_timeout(pcie->parf + PARF_LTSSM, val,
> > > > + !(val & FLUSH_MODE), 10,
> > > > + FLUSH_TIMEOUT_US);
> > > > + if (ret) {
> > > > + dev_err(dev, "Flush mode clear failed: %d\n", ret);
> > > > + goto err_host_deinit;
> > > > + }
> > > > +
> > > > + qcom_pcie_host_deinit(pp);
> > > > +
> > > > + ret = qcom_pcie_host_init(pp);
> > > > + if (ret) {
> > > > + dev_err(dev, "Host init failed\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = dw_pcie_setup_rc(pp);
> > > > + if (ret)
> > > > + goto err_host_deinit;
> > > > +
> > > > + /*
> > > > + * Re-enable global IRQ events as the PARF_INT_ALL_MASK register is
> > > > + * non-sticky.
> > > > + */
> > > > + if (pcie->global_irq)
> > > > + writel_relaxed(PARF_INT_ALL_LINK_UP | PARF_INT_ALL_LINK_DOWN |
> > > > + PARF_INT_MSI_DEV_0_7, pcie->parf + PARF_INT_ALL_MASK);
> > > do we need to enable linkup again here, since all the devices are
> > > enumerated previously, the linkup irq will do a rescan again which is
> > > not needed.
> >
> > Right. I was trying to keep the irq enablement on par with probe(), but LINK_UP
> > is strictly not needed. I will drop it.
> >
> > > Instead of linkup we update icc & opp bandwidths after
> > > dw_pcie_wait_for_link() in the below.
> > >
> >
> > Why do we need to update ICC and OPP?
> After link retrain, if the link data rate has reduced due to some
> electrical issue or some other reason we may need to update the icc and
> opp votings here.
>
Hmm, I was not expecting it to get changed, but considering the fact that the
link down might be happening due to any link stability issues, it is plausible.
I will add a call to qcom_pcie_icc_opp_update().
- Mani
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists