| 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: <f9c95baf-f2a9-4057-aeca-2c35dcd943fb@oss.qualcomm.com>
Date: Thu, 29 Jan 2026 10:59:36 +0530
From: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
To: Bjorn Andersson <andersson@...nel.org>
Cc: Jingoo Han <jingoohan1@...il.com>,
Manivannan Sadhasivam
<mani@...nel.org>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kwilczynski@...nel.org>,
Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
Will Deacon <will@...nel.org>, linux-pci@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, jonathanh@...dia.com
Subject: Re: [PATCH 3/3] PCI: qcom: Add D3cold support
On 1/28/2026 8:17 PM, Bjorn Andersson wrote:
> On Wed, Jan 28, 2026 at 05:10:43PM +0530, Krishna Chaitanya Chundru wrote:
>> Add pme_turn_off() support and use DWC common suspend resume methods
>> for device D3cold entry & exit. If the device is not kept in D3cold
>> use existing methods like keeping icc votes, opp votes etc.. intact.
>>
>> In qcom_pcie_deinit_2_7_0(), explicitly disable PCIe clocks and resets
>> in the controller.
>>
>> Remove suspended flag from qcom_pcie structure as it is no longer needed.
>>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
>> ---
>> drivers/pci/controller/dwc/pcie-qcom.c | 114 ++++++++++++++++++++-------------
>> 1 file changed, 68 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> [..]
>> @@ -2016,53 +2030,51 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>> if (!pcie)
>> return 0;
>>
>> - /*
>> - * Set minimum bandwidth required to keep data path functional during
>> - * suspend.
>> - */
>> - if (pcie->icc_mem) {
>> - ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
>> - if (ret) {
>> - dev_err(dev,
>> - "Failed to set bandwidth for PCIe-MEM interconnect path: %d\n",
>> - ret);
>> - return ret;
>> - }
>> - }
>> + ret = dw_pcie_suspend_noirq(pcie->pci);
>> + if (ret)
>> + return ret;
>>
>> - /*
>> - * 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.
>> - */
>> - if (!dw_pcie_link_up(pcie->pci)) {
>> - qcom_pcie_host_deinit(&pcie->pci->pp);
>> - pcie->suspended = true;
>> - }
>> + if (pcie->pci->suspended) {
> I think this is okay for now, but I'd prefer changing the return value
> of dw_pcie_suspend_noirq() to indicate if it did stop the link or not
> (two different success values) - rather than deriving that information
> by peeking into the dw_pcie struct and conclude that
> dw_pcie_suspend_noirq() did reach the end.
>
>> + ret = icc_disable(pcie->icc_mem);
>> + if (ret)
>> + dev_err(dev, "Failed to disable PCIe-MEM interconnect path: %d\n", ret);
>>
>> - /*
>> - * Only disable CPU-PCIe interconnect path if the suspend is non-S2RAM.
>> - * Because on some platforms, DBI access can happen very late during the
>> - * S2RAM and a non-active CPU-PCIe interconnect path may lead to NoC
>> - * error.
>> - */
>> - if (pm_suspend_target_state != PM_SUSPEND_MEM) {
>> ret = icc_disable(pcie->icc_cpu);
>> if (ret)
>> dev_err(dev, "Failed to disable CPU-PCIe interconnect path: %d\n", ret);
>>
>> if (pcie->use_pm_opp)
>> dev_pm_opp_set_opp(pcie->pci->dev, NULL);
>> + } else {
>> + /*
>> + * Set minimum bandwidth required to keep data path functional during
>> + * suspend.
>> + */
>> + if (pcie->icc_mem) {
>> + ret = icc_set_bw(pcie->icc_mem, 0, kBps_to_icc(1));
>> + if (ret) {
>> + dev_err(dev,
>> + "Failed to set bandwidth for PCIe-MEM interconnect path: %d\n",
>> + ret);
>> + return ret;
>> + }
>> + }
>> +
>> + /*
>> + * Only disable CPU-PCIe interconnect path if the suspend is non-S2RAM.
>> + * Because on some platforms, DBI access can happen very late during the
>> + * S2RAM and a non-active CPU-PCIe interconnect path may lead to NoC
>> + * error.
>> + */
>> + if (pm_suspend_target_state != PM_SUSPEND_MEM) {
>> + ret = icc_disable(pcie->icc_cpu);
>> + if (ret)
>> + dev_err(dev, "Failed to disable CPU-PCIe interconnect path: %d\n",
>> + ret);
>> +
>> + if (pcie->use_pm_opp)
>> + dev_pm_opp_set_opp(pcie->pci->dev, NULL);
>> + }
>> }
>> return ret;
>> }
>> @@ -2076,20 +2088,30 @@ static int qcom_pcie_resume_noirq(struct device *dev)
>> if (!pcie)
>> return 0;
>>
>> - if (pm_suspend_target_state != PM_SUSPEND_MEM) {
>> + if (pcie->pci->suspended) {
>> ret = icc_enable(pcie->icc_cpu);
>> if (ret) {
>> dev_err(dev, "Failed to enable CPU-PCIe interconnect path: %d\n", ret);
>> return ret;
>> }
>> - }
>>
>> - if (pcie->suspended) {
>> - ret = qcom_pcie_host_init(&pcie->pci->pp);
>> + ret = icc_enable(pcie->icc_mem);
>> + if (ret) {
>> + dev_err(dev, "Failed to enable PCIe-MEM interconnect path: %d\n", ret);
> I think you should revert icc_enable(pcie->icc_cpu) here, to avoid
> leaving the bus voted for with the PCIe controller resume aborted.
>
>> + return ret;
>> + }
>> + ret = dw_pcie_resume_noirq(pcie->pci);
>> if (ret)
> And Both icc_cpu and icc_mem here.
Ack, I will do this in V2.
- Krishna Chaitanya.
> Regards,
> Bjorn
>
>> return ret;
>> -
>> - pcie->suspended = false;
>> + } else {
>> + if (pm_suspend_target_state != PM_SUSPEND_MEM) {
>> + ret = icc_enable(pcie->icc_cpu);
>> + if (ret) {
>> + dev_err(dev, "Failed to enable CPU-PCIe interconnect path: %d\n",
>> + ret);
>> + return ret;
>> + }
>> + }
>> }
>>
>> qcom_pcie_icc_opp_update(pcie);
>>
>> --
>> 2.34.1
>>
>>
Powered by blists - more mailing lists