| 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: <27516921.17f2.1997bb2a498.Coremail.zhangsenchuan@eswincomputing.com>
Date: Wed, 24 Sep 2025 20:28:49 +0800 (GMT+08:00)
From: zhangsenchuan <zhangsenchuan@...incomputing.com>
To: "Bjorn Helgaas" <helgaas@...nel.org>
Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kwilczynski@...nel.org,
mani@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, linux-pci@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
p.zabel@...gutronix.de, johan+linaro@...nel.org,
quic_schintav@...cinc.com, shradha.t@...sung.com, cassel@...nel.org,
thippeswamy.havalige@....com, mayank.rana@....qualcomm.com,
inochiama@...il.com, ningyu@...incomputing.com,
linmin@...incomputing.com, pinkesh.vaghela@...fochips.com,
"Yanghui Ou" <ouyanghui@...incomputing.com>
Subject: Re: Re: [PATCH v3 2/2] PCI: EIC7700: Add Eswin PCIe host controller
driver
> -----Original Messages-----
> From: "Bjorn Helgaas" <helgaas@...nel.org>
> Send time:Wednesday, 24/09/2025 00:32:54
> To: zhangsenchuan@...incomputing.com
> Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kwilczynski@...nel.org, mani@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, linux-pci@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, p.zabel@...gutronix.de, johan+linaro@...nel.org, quic_schintav@...cinc.com, shradha.t@...sung.com, cassel@...nel.org, thippeswamy.havalige@....com, mayank.rana@....qualcomm.com, inochiama@...il.com, ningyu@...incomputing.com, linmin@...incomputing.com, pinkesh.vaghela@...fochips.com, "Yanghui Ou" <ouyanghui@...incomputing.com>
> Subject: Re: [PATCH v3 2/2] PCI: EIC7700: Add Eswin PCIe host controller driver
>
> On Tue, Sep 23, 2025 at 08:12:27PM +0800, zhangsenchuan@...incomputing.com wrote:
> > Add driver for the Eswin EIC7700 PCIe host controller,the controller is
> > based on the DesignWare PCIe core, IP revision 6.00a The PCIe Gen.3
> > controller supports a data rate of 8 GT/s and 4 channels, support INTX
> > and MSI interrupts.
>
> s/host controller,the controller is/host controller, which is/
>
> Add period at end of first sentence.
>
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -375,6 +375,17 @@ config PCI_EXYNOS
> > hardware and therefore the driver re-uses the DesignWare core
> > functions to implement the driver.
> >
> >
> > +static int eswin_pcie_suspend(struct device *dev)
> > +{
> > + struct eswin_pcie *pcie = dev_get_drvdata(dev);
> > + struct eswin_pcie_port *port;
> > +
> > + /*
> > + * For controllers with active devices, resources are retained and
> > + * cannot be turned off.
> > + */
> > + if (!dw_pcie_link_up(&pcie->pci)) {
> > + list_for_each_entry(port, &pcie->ports, list)
> > + reset_control_assert(port->perst);
> > + eswin_pcie_assert(pcie);
> > + clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks);
> > + pcie->suspended = true;
>
> I'm a little dubious about this since none of the other drivers check
> dw_pcie_link_up().
dear Bjorn
Thank you very much for your review.
Clarification：
The previous patch, Mani gave me advice: "So you want to power off the
device even if it intends to be in D0?" Like NVMe."
I referred to the implementation of "pcie-qcom.c" and added the
judgment of the dw_pcie_link_up function.
The following are mani's comments on fixing this issue：
/*
* Turn OFF the resources only for controllers without active PCIe
* devices. For controllers with active devices, the resources are kept
* ON and the link is expected to be in L0/L1 (sub)states.
*
* Turning OFF the resources for controllers with active PCIe devices
* will trigger access violation during the end of the suspend cycle,
* as kernel tries to access the PCIe devices config space for masking
* MSIs.
*
* Also, it is not desirable to put the link into L2/L3 state as that
* implies VDD supply will be removed and the devices may go into
* powerdown state. This will affect the lifetime of the storage devices
* like NVMe.
*/
>
> It also seems a little bit racy since dw_pcie_link_up() can always
> change after it's called.
>
> And tracking pcie->suspended is also unusual if not unique.
>
> Should dw_pcie_suspend_noirq() and dw_pcie_resume_noirq() be used
> here?
Clarification：
The dw_pcie_suspend_noirq and dw_pcie_resume_noirq code is a nice wrapped
collection function, but the dw_pcie_suspend_noirq function implements
sending the PME_Turn_Off message. Notify the device to enter low power
consumption and wait for it to enter the LTSSM_L2 state. Our hardware only
supports entering the L0/L1 state and does not support entering the D3code
and L2/L3 states. It will cause mistakes and can't resume. Therefore, I
cannot directly call the dw_pcie_suspend_noirq function here.
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int eswin_pcie_resume(struct device *dev)
> > +{
> > + struct eswin_pcie *pcie = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + if (!pcie->suspended)
> > + return 0;
> > +
> > + ret = eswin_pcie_host_init(&pcie->pci.pp);
> > + if (ret) {
> > + dev_err(dev, "Failed to init host: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + dw_pcie_setup_rc(&pcie->pci.pp);
> > + eswin_pcie_start_link(&pcie->pci);
> > + dw_pcie_wait_for_link(&pcie->pci);
> > +
> > + pcie->suspended = false;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dev_pm_ops eswin_pcie_pm_ops = {
> > + NOIRQ_SYSTEM_SLEEP_PM_OPS(eswin_pcie_suspend, eswin_pcie_resume)
>
> Suggest adding "_noirq" to the end of these function names since this
> sets .suspend_noirq, .resume_noirq, etc. Also will match other drivers.
Powered by blists - more mailing lists