[PATCH v2 0/3] PCI: dwc: Check for device presence in suspend and resume

[PATCH v2 0/3] PCI: dwc: Check for device presence in suspend and resume

[Manivannan Sadhasivam]

Hi,

This series aims to fix the usage of dw_pcie_link_up() API to check for Link up
during system suspend. The motivation for this series comes from recent
discussions [1] [2], where developers wanted to skip PME_Turn_Off broadcast in
dw_pcie_suspend_noirq() API when devices are not attached to the bus. They ended
up using dw_pcie_link_up() to check for the device presence due to the bad
example in the pcie-qcom driver which does the same. The usage of
dw_pcie_link_up() API here would be racy as the link can go down at any time
after the check.

So to properly check for the device presence, this series introduces an API, 
pci_root_ports_have_device(), that accepts the Root bus pointer and checks for
the presence of a device under any of the Root Ports. This API is used to
replace the dw_pcie_link_up() check in suspend path of pcie-qcom driver and also
used to skip the PME_Turn_Off brodcast message in dwc_pcie_suspend_noirq() API
and to skip waiting for the link up in dwc_pcie_resume_noirq() API.

Testing
=======

This series is tested on Qualcomm Lenovo Thinkpad T14s and observed no
functional change during the system suspend path.

- Mani

[1] https://lore.kernel.org/linux-pci/CAKfTPtCtHquxtK=Zx2WSNm15MmqeUXO8XXi8FkS4EpuP80PP7g@mail.gmail.com/
[2] https://lore.kernel.org/linux-pci/27516921.17f2.1997bb2a498.Coremail.zhangsenchuan@eswincomputing.com/

Changes in v2:

* Skipped waiting for link up in dwc_pcie_resume_noirq() if there was no device
  before suspend.
* Fixed the kdoc for pci_root_ports_have_device()

Manivannan Sadhasivam (3):
  PCI: host-common: Add an API to check for any device under the Root
    Ports
  PCI: qcom: Check for the presence of a device instead of Link up
    during suspend
  PCI: dwc: Check for the device presence during suspend and resume

 .../pci/controller/dwc/pcie-designware-host.c | 13 ++++++++++++
 drivers/pci/controller/dwc/pcie-qcom.c        |  6 ++++--
 drivers/pci/controller/pci-host-common.c      | 21 +++++++++++++++++++
 drivers/pci/controller/pci-host-common.h      |  2 ++
 4 files changed, 40 insertions(+), 2 deletions(-)

-- 
2.48.1

[PATCH v2 1/3] PCI: host-common: Add an API to check for any device under the Root Ports

[Manivannan Sadhasivam]

Some controller drivers need to check if there is any device available
under the Root Ports. So add an API that returns 'true' if a device is
found under any of the Root Ports, 'false' otherwise.

Controller drivers can use this API for usecases like turning off the
controller resources only if there are no devices under the Root Ports,
skipping PME_Turn_Off broadcast etc...

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/pci-host-common.c | 21 +++++++++++++++++++++
 drivers/pci/controller/pci-host-common.h |  2 ++
 2 files changed, 23 insertions(+)

diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
index 810d1c8de24e..772bac8b3bf2 100644
--- a/drivers/pci/controller/pci-host-common.c
+++ b/drivers/pci/controller/pci-host-common.c
@@ -17,6 +17,27 @@
 
 #include "pci-host-common.h"
 
+/**
+ * pci_root_ports_have_device - Check if the Root Ports under the Root bus have
+ *				any device underneath
+ * @root_bus: Root bus to check for
+ *
+ * Return: true if a device is found, false otherwise
+ */
+bool pci_root_ports_have_device(struct pci_bus *root_bus)
+{
+	struct pci_bus *child;
+
+	/* Iterate over the Root Port busses and look for any device */
+	list_for_each_entry(child, &root_bus->children, node) {
+		if (!list_empty(&child->devices))
+			return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(pci_root_ports_have_device);
+
 static void gen_pci_unmap_cfg(void *ptr)
 {
 	pci_ecam_free((struct pci_config_window *)ptr);
diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/controller/pci-host-common.h
index 51c35ec0cf37..ff1c2ff98043 100644
--- a/drivers/pci/controller/pci-host-common.h
+++ b/drivers/pci/controller/pci-host-common.h
@@ -19,4 +19,6 @@ void pci_host_common_remove(struct platform_device *pdev);
 
 struct pci_config_window *pci_host_common_ecam_create(struct device *dev,
 	struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops);
+
+bool pci_root_ports_have_device(struct pci_bus *root_bus);
 #endif
-- 
2.48.1

[PATCH v2 2/3] PCI: qcom: Check for the presence of a device instead of Link up during suspend

[Manivannan Sadhasivam]

The suspend handler checks for the PCIe Link up to decide when to turn off
the controller resources. But this check is racy as the PCIe Link can go
down just after this check.

So use the newly introduced API, pci_root_ports_have_device() that checks
for the presence of a device under any of the Root Ports to replace the
Link up check.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 805edbbfe7eb..b2b89e2e4916 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -2018,6 +2018,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
 static int qcom_pcie_suspend_noirq(struct device *dev)
 {
 	struct qcom_pcie *pcie;
+	struct dw_pcie_rp *pp;
 	int ret = 0;
 
 	pcie = dev_get_drvdata(dev);
@@ -2053,8 +2054,9 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
 	 * 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);
+	pp = &pcie->pci->pp;
+	if (!pci_root_ports_have_device(pp->bridge->bus)) {
+		qcom_pcie_host_deinit(pp);
 		pcie->suspended = true;
 	}
 
-- 
2.48.1

[PATCH v2 3/3] PCI: dwc: Check for the device presence during suspend and resume

[Manivannan Sadhasivam]

If there is no device available under the Root Ports, there is no point in
sending PME_Turn_Off and waiting for L2/L3 transition during suspend, it
will result in a timeout. Hence, skip those steps if no device is available
during suspend.

During resume, do not wait for the link up if there was no device connected
before suspend. It is very unlikely that a device will get connected while
the host system was suspended.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 20c9333bcb1c..5a39e7139ec9 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -20,6 +20,7 @@
 #include <linux/platform_device.h>
 
 #include "../../pci.h"
+#include "../pci-host-common.h"
 #include "pcie-designware.h"
 
 static struct pci_ops dw_pcie_ops;
@@ -1129,6 +1130,9 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
 	u32 val;
 	int ret;
 
+	if (!pci_root_ports_have_device(pci->pp.bridge->bus))
+		goto stop_link;
+
 	/*
 	 * If L1SS is supported, then do not put the link into L2 as some
 	 * devices such as NVMe expect low resume latency.
@@ -1162,6 +1166,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
 	 */
 	udelay(1);
 
+stop_link:
 	dw_pcie_stop_link(pci);
 	if (pci->pp.ops->deinit)
 		pci->pp.ops->deinit(&pci->pp);
@@ -1195,6 +1200,14 @@ int dw_pcie_resume_noirq(struct dw_pcie *pci)
 	if (ret)
 		return ret;
 
+	/*
+	 * If there was no device before suspend, skip waiting for link up as
+	 * it is bound to fail. It is very unlikely that a device will get
+	 * connected *during* suspend.
+	 */
+	if (!pci_root_ports_have_device(pci->pp.bridge->bus))
+		return 0;
+
 	ret = dw_pcie_wait_for_link(pci);
 	if (ret)
 		return ret;
-- 
2.48.1

Re: [PATCH v2 0/3] PCI: dwc: Check for device presence in suspend and resume

[Manivannan Sadhasivam]

On Fri, 07 Nov 2025 10:13:16 +0530, Manivannan Sadhasivam wrote:
> This series aims to fix the usage of dw_pcie_link_up() API to check for Link up
> during system suspend. The motivation for this series comes from recent
> discussions [1] [2], where developers wanted to skip PME_Turn_Off broadcast in
> dw_pcie_suspend_noirq() API when devices are not attached to the bus. They ended
> up using dw_pcie_link_up() to check for the device presence due to the bad
> example in the pcie-qcom driver which does the same. The usage of
> dw_pcie_link_up() API here would be racy as the link can go down at any time
> after the check.
> 
> [...]

Applied, thanks!

[1/3] PCI: host-common: Add an API to check for any device under the Root Ports
      commit: 64a4b36d229120a71ede91aed16519164a181c13
[2/3] PCI: qcom: Check for the presence of a device instead of Link up during suspend
      commit: b6811cd64ac54fb2426b6ce35ba7c63588402687
[3/3] PCI: dwc: Check for the device presence during suspend and resume
      commit: e8fe6b3413a1b92b4bc0f0182ea4b49ee369541b

Best regards,
-- 
Manivannan Sadhasivam <mani@kernel.org>

Re: [PATCH v2 0/3] PCI: dwc: Check for device presence in suspend and resume

[Vincent Guittot]

On Fri, 7 Nov 2025 at 05:43, Manivannan Sadhasivam
<manivannan.sadhasivam@oss.qualcomm.com> wrote:
>
> Hi,
>
> This series aims to fix the usage of dw_pcie_link_up() API to check for Link up
> during system suspend. The motivation for this series comes from recent
> discussions [1] [2], where developers wanted to skip PME_Turn_Off broadcast in
> dw_pcie_suspend_noirq() API when devices are not attached to the bus. They ended
> up using dw_pcie_link_up() to check for the device presence due to the bad
> example in the pcie-qcom driver which does the same. The usage of
> dw_pcie_link_up() API here would be racy as the link can go down at any time
> after the check.
>
> So to properly check for the device presence, this series introduces an API,
> pci_root_ports_have_device(), that accepts the Root bus pointer and checks for
> the presence of a device under any of the Root Ports. This API is used to
> replace the dw_pcie_link_up() check in suspend path of pcie-qcom driver and also
> used to skip the PME_Turn_Off brodcast message in dwc_pcie_suspend_noirq() API
> and to skip waiting for the link up in dwc_pcie_resume_noirq() API.
>
> Testing
> =======
>
> This series is tested on Qualcomm Lenovo Thinkpad T14s and observed no
> functional change during the system suspend path.
>
> - Mani
>
> [1] https://lore.kernel.org/linux-pci/CAKfTPtCtHquxtK=Zx2WSNm15MmqeUXO8XXi8FkS4EpuP80PP7g@mail.gmail.com/
> [2] https://lore.kernel.org/linux-pci/27516921.17f2.1997bb2a498.Coremail.zhangsenchuan@eswincomputing.com/
>
> Changes in v2:
>
> * Skipped waiting for link up in dwc_pcie_resume_noirq() if there was no device
>   before suspend.
> * Fixed the kdoc for pci_root_ports_have_device()
>
> Manivannan Sadhasivam (3):
>   PCI: host-common: Add an API to check for any device under the Root
>     Ports
>   PCI: qcom: Check for the presence of a device instead of Link up
>     during suspend
>   PCI: dwc: Check for the device presence during suspend and resume

You already queued it but FWIW
Tested-by: Vincent Guittot <vincent.guittot@linaro.org>

>
>  .../pci/controller/dwc/pcie-designware-host.c | 13 ++++++++++++
>  drivers/pci/controller/dwc/pcie-qcom.c        |  6 ++++--
>  drivers/pci/controller/pci-host-common.c      | 21 +++++++++++++++++++
>  drivers/pci/controller/pci-host-common.h      |  2 ++
>  4 files changed, 40 insertions(+), 2 deletions(-)
>
> --
> 2.48.1
>

Re: [PATCH v2 3/3] PCI: dwc: Check for the device presence during suspend and resume

[Frank Li]

On Fri, Nov 07, 2025 at 10:13:19AM +0530, Manivannan Sadhasivam wrote:
> If there is no device available under the Root Ports, there is no point in
> sending PME_Turn_Off and waiting for L2/L3 transition during suspend, it
> will result in a timeout. Hence, skip those steps if no device is available
> during suspend.
>
> During resume, do not wait for the link up if there was no device connected
> before suspend. It is very unlikely that a device will get connected while
> the host system was suspended.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 20c9333bcb1c..5a39e7139ec9 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -20,6 +20,7 @@
>  #include <linux/platform_device.h>
>
>  #include "../../pci.h"
> +#include "../pci-host-common.h"
>  #include "pcie-designware.h"
>
>  static struct pci_ops dw_pcie_ops;
> @@ -1129,6 +1130,9 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
>  	u32 val;
>  	int ret;
>
> +	if (!pci_root_ports_have_device(pci->pp.bridge->bus))
> +		goto stop_link;
> +
>  	/*
>  	 * If L1SS is supported, then do not put the link into L2 as some
>  	 * devices such as NVMe expect low resume latency.
> @@ -1162,6 +1166,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
>  	 */
>  	udelay(1);

I think move pme_turn_off() to helper funciton will make code look better

	if (pci_root_ports_have_device()) {
		ret = dwc_pme_turn_off();
		if (ret)
			return ret;
	};


>
> +stop_link:
>  	dw_pcie_stop_link(pci);
>  	if (pci->pp.ops->deinit)
>  		pci->pp.ops->deinit(&pci->pp);
> @@ -1195,6 +1200,14 @@ int dw_pcie_resume_noirq(struct dw_pcie *pci)
>  	if (ret)
>  		return ret;
>
> +	/*
> +	 * If there was no device before suspend, skip waiting for link up as
> +	 * it is bound to fail. It is very unlikely that a device will get
> +	 * connected *during* suspend.

I think it should use certern term. the a device will not get linkup during
suspend, if this happen, it is a new hotjoin device after system resume.

Frank

> +	 */
> +	if (!pci_root_ports_have_device(pci->pp.bridge->bus))
> +		return 0;
> +
>  	ret = dw_pcie_wait_for_link(pci);
>  	if (ret)
>  		return ret;
> --
> 2.48.1
>

Re: [PATCH v2 1/3] PCI: host-common: Add an API to check for any device under the Root Ports

[Frank Li]

On Fri, Nov 07, 2025 at 10:13:17AM +0530, Manivannan Sadhasivam wrote:
> Some controller drivers need to check if there is any device available
> under the Root Ports. So add an API that returns 'true' if a device is
> found under any of the Root Ports, 'false' otherwise.
>
> Controller drivers can use this API for usecases like turning off the
> controller resources only if there are no devices under the Root Ports,
> skipping PME_Turn_Off broadcast etc...
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/pci/controller/pci-host-common.c | 21 +++++++++++++++++++++
>  drivers/pci/controller/pci-host-common.h |  2 ++
>  2 files changed, 23 insertions(+)
>
> diff --git a/drivers/pci/controller/pci-host-common.c b/drivers/pci/controller/pci-host-common.c
> index 810d1c8de24e..772bac8b3bf2 100644
> --- a/drivers/pci/controller/pci-host-common.c
> +++ b/drivers/pci/controller/pci-host-common.c
> @@ -17,6 +17,27 @@
>
>  #include "pci-host-common.h"
>
> +/**
> + * pci_root_ports_have_device - Check if the Root Ports under the Root bus have
> + *				any device underneath
> + * @root_bus: Root bus to check for
> + *
> + * Return: true if a device is found, false otherwise
> + */
> +bool pci_root_ports_have_device(struct pci_bus *root_bus)
> +{
> +	struct pci_bus *child;
> +
> +	/* Iterate over the Root Port busses and look for any device */
> +	list_for_each_entry(child, &root_bus->children, node) {
> +		if (!list_empty(&child->devices))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(pci_root_ports_have_device);
> +
>  static void gen_pci_unmap_cfg(void *ptr)
>  {
>  	pci_ecam_free((struct pci_config_window *)ptr);
> diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/controller/pci-host-common.h
> index 51c35ec0cf37..ff1c2ff98043 100644
> --- a/drivers/pci/controller/pci-host-common.h
> +++ b/drivers/pci/controller/pci-host-common.h
> @@ -19,4 +19,6 @@ void pci_host_common_remove(struct platform_device *pdev);
>
>  struct pci_config_window *pci_host_common_ecam_create(struct device *dev,
>  	struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops);
> +
> +bool pci_root_ports_have_device(struct pci_bus *root_bus);
>  #endif
> --
> 2.48.1
>

Re: [PATCH v2 3/3] PCI: dwc: Check for the device presence during suspend and resume

[Bjorn Helgaas]

[+cc Frank]

On Fri, Nov 07, 2025 at 10:13:19AM +0530, Manivannan Sadhasivam wrote:
> If there is no device available under the Root Ports, there is no point in
> sending PME_Turn_Off and waiting for L2/L3 transition during suspend, it
> will result in a timeout. Hence, skip those steps if no device is available
> during suspend.
> 
> During resume, do not wait for the link up if there was no device connected
> before suspend. It is very unlikely that a device will get connected while
> the host system was suspended.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 20c9333bcb1c..5a39e7139ec9 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -20,6 +20,7 @@
>  #include <linux/platform_device.h>
>  
>  #include "../../pci.h"
> +#include "../pci-host-common.h"
>  #include "pcie-designware.h"
>  
>  static struct pci_ops dw_pcie_ops;
> @@ -1129,6 +1130,9 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
>  	u32 val;
>  	int ret;
>  
> +	if (!pci_root_ports_have_device(pci->pp.bridge->bus))
> +		goto stop_link;

This looks racy.  Maybe it's still OK, but I think it would be good to
include a comment to acknowledge that and explain why either outcome
is acceptable, e.g., if a user removes a device during suspend, it
results in a timeout but nothing more terrible.

>  	/*
>  	 * If L1SS is supported, then do not put the link into L2 as some
>  	 * devices such as NVMe expect low resume latency.
> @@ -1162,6 +1166,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
>  	 */
>  	udelay(1);
>  
> +stop_link:
>  	dw_pcie_stop_link(pci);
>  	if (pci->pp.ops->deinit)
>  		pci->pp.ops->deinit(&pci->pp);
> @@ -1195,6 +1200,14 @@ int dw_pcie_resume_noirq(struct dw_pcie *pci)
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * If there was no device before suspend, skip waiting for link up as
> +	 * it is bound to fail. It is very unlikely that a device will get
> +	 * connected *during* suspend.

I'm not convinced.  Unlike the suspend side, where the race window is
tiny, here the window is the entire time the system is suspended, and
at least in laptop usage, there's no reason I would hesitate to plug
something in while suspended.

Regardless, the overall behavior needs to be acceptable whether or not
a device was connected during suspend.

This is probably the same thing you said, Frank, sorry if I'm just
repeating it.

> +	if (!pci_root_ports_have_device(pci->pp.bridge->bus))
> +		return 0;
> +
>  	ret = dw_pcie_wait_for_link(pci);
>  	if (ret)
>  		return ret;
> -- 
> 2.48.1
> 

Re: [PATCH v2 2/3] PCI: qcom: Check for the presence of a device instead of Link up during suspend

[Bjorn Helgaas]

On Fri, Nov 07, 2025 at 10:13:18AM +0530, Manivannan Sadhasivam wrote:
> The suspend handler checks for the PCIe Link up to decide when to turn off
> the controller resources. But this check is racy as the PCIe Link can go
> down just after this check.
> 
> So use the newly introduced API, pci_root_ports_have_device() that checks
> for the presence of a device under any of the Root Ports to replace the
> Link up check.

Why is pci_root_ports_have_device() itself not racy?

> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 805edbbfe7eb..b2b89e2e4916 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -2018,6 +2018,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
>  static int qcom_pcie_suspend_noirq(struct device *dev)
>  {
>  	struct qcom_pcie *pcie;
> +	struct dw_pcie_rp *pp;
>  	int ret = 0;
>  
>  	pcie = dev_get_drvdata(dev);
> @@ -2053,8 +2054,9 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
>  	 * 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);
> +	pp = &pcie->pci->pp;
> +	if (!pci_root_ports_have_device(pp->bridge->bus)) {
> +		qcom_pcie_host_deinit(pp);
>  		pcie->suspended = true;
>  	}
>  
> -- 
> 2.48.1
> 

Re: [PATCH v2 2/3] PCI: qcom: Check for the presence of a device instead of Link up during suspend

[Manivannan Sadhasivam]

On Thu, Nov 13, 2025 at 10:41:47AM -0600, Bjorn Helgaas wrote:
> On Fri, Nov 07, 2025 at 10:13:18AM +0530, Manivannan Sadhasivam wrote:
> > The suspend handler checks for the PCIe Link up to decide when to turn off
> > the controller resources. But this check is racy as the PCIe Link can go
> > down just after this check.
> > 
> > So use the newly introduced API, pci_root_ports_have_device() that checks
> > for the presence of a device under any of the Root Ports to replace the
> > Link up check.
> 
> Why is pci_root_ports_have_device() itself not racy?
> 

Because it is very uncommon for the 'pci_dev' to go away during the host
controller suspend. It might still be possible in edge cases, but very common as
the link down. I can reword it.

- Mani

> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 805edbbfe7eb..b2b89e2e4916 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -2018,6 +2018,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> >  static int qcom_pcie_suspend_noirq(struct device *dev)
> >  {
> >  	struct qcom_pcie *pcie;
> > +	struct dw_pcie_rp *pp;
> >  	int ret = 0;
> >  
> >  	pcie = dev_get_drvdata(dev);
> > @@ -2053,8 +2054,9 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> >  	 * 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);
> > +	pp = &pcie->pci->pp;
> > +	if (!pci_root_ports_have_device(pp->bridge->bus)) {
> > +		qcom_pcie_host_deinit(pp);
> >  		pcie->suspended = true;
> >  	}
> >  
> > -- 
> > 2.48.1
> > 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 3/3] PCI: dwc: Check for the device presence during suspend and resume

[Manivannan Sadhasivam]

On Mon, Nov 10, 2025 at 11:26:38AM -0500, Frank Li wrote:
> On Fri, Nov 07, 2025 at 10:13:19AM +0530, Manivannan Sadhasivam wrote:
> > If there is no device available under the Root Ports, there is no point in
> > sending PME_Turn_Off and waiting for L2/L3 transition during suspend, it
> > will result in a timeout. Hence, skip those steps if no device is available
> > during suspend.
> >
> > During resume, do not wait for the link up if there was no device connected
> > before suspend. It is very unlikely that a device will get connected while
> > the host system was suspended.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 20c9333bcb1c..5a39e7139ec9 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/platform_device.h>
> >
> >  #include "../../pci.h"
> > +#include "../pci-host-common.h"
> >  #include "pcie-designware.h"
> >
> >  static struct pci_ops dw_pcie_ops;
> > @@ -1129,6 +1130,9 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> >  	u32 val;
> >  	int ret;
> >
> > +	if (!pci_root_ports_have_device(pci->pp.bridge->bus))
> > +		goto stop_link;
> > +
> >  	/*
> >  	 * If L1SS is supported, then do not put the link into L2 as some
> >  	 * devices such as NVMe expect low resume latency.
> > @@ -1162,6 +1166,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> >  	 */
> >  	udelay(1);
> 
> I think move pme_turn_off() to helper funciton will make code look better
> 

What about the L2 entry timeout?

> 	if (pci_root_ports_have_device()) {
> 		ret = dwc_pme_turn_off();
> 		if (ret)
> 			return ret;
> 	};
> 
> 
> >
> > +stop_link:
> >  	dw_pcie_stop_link(pci);
> >  	if (pci->pp.ops->deinit)
> >  		pci->pp.ops->deinit(&pci->pp);
> > @@ -1195,6 +1200,14 @@ int dw_pcie_resume_noirq(struct dw_pcie *pci)
> >  	if (ret)
> >  		return ret;
> >
> > +	/*
> > +	 * If there was no device before suspend, skip waiting for link up as
> > +	 * it is bound to fail. It is very unlikely that a device will get
> > +	 * connected *during* suspend.
> 
> I think it should use certern term. the a device will not get linkup during
> suspend, if this happen, it is a new hotjoin device after system resume.
> 

Sure.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 3/3] PCI: dwc: Check for the device presence during suspend and resume

[Manivannan Sadhasivam]

On Thu, Nov 13, 2025 at 10:40:13AM -0600, Bjorn Helgaas wrote:
> [+cc Frank]
> 
> On Fri, Nov 07, 2025 at 10:13:19AM +0530, Manivannan Sadhasivam wrote:
> > If there is no device available under the Root Ports, there is no point in
> > sending PME_Turn_Off and waiting for L2/L3 transition during suspend, it
> > will result in a timeout. Hence, skip those steps if no device is available
> > during suspend.
> > 
> > During resume, do not wait for the link up if there was no device connected
> > before suspend. It is very unlikely that a device will get connected while
> > the host system was suspended.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 20c9333bcb1c..5a39e7139ec9 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/platform_device.h>
> >  
> >  #include "../../pci.h"
> > +#include "../pci-host-common.h"
> >  #include "pcie-designware.h"
> >  
> >  static struct pci_ops dw_pcie_ops;
> > @@ -1129,6 +1130,9 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> >  	u32 val;
> >  	int ret;
> >  
> > +	if (!pci_root_ports_have_device(pci->pp.bridge->bus))
> > +		goto stop_link;
> 
> This looks racy.  Maybe it's still OK, but I think it would be good to
> include a comment to acknowledge that and explain why either outcome
> is acceptable, e.g., if a user removes a device during suspend, it
> results in a timeout but nothing more terrible.
> 

Ok.

> >  	/*
> >  	 * If L1SS is supported, then do not put the link into L2 as some
> >  	 * devices such as NVMe expect low resume latency.
> > @@ -1162,6 +1166,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> >  	 */
> >  	udelay(1);
> >  
> > +stop_link:
> >  	dw_pcie_stop_link(pci);
> >  	if (pci->pp.ops->deinit)
> >  		pci->pp.ops->deinit(&pci->pp);
> > @@ -1195,6 +1200,14 @@ int dw_pcie_resume_noirq(struct dw_pcie *pci)
> >  	if (ret)
> >  		return ret;
> >  
> > +	/*
> > +	 * If there was no device before suspend, skip waiting for link up as
> > +	 * it is bound to fail. It is very unlikely that a device will get
> > +	 * connected *during* suspend.
> 
> I'm not convinced.  Unlike the suspend side, where the race window is
> tiny, here the window is the entire time the system is suspended, and
> at least in laptop usage, there's no reason I would hesitate to plug
> something in while suspended.
> 

In that case, we just need to do:

	/* Ignore errors as there could be no devices connected */
	dw_pcie_wait_for_link()

I wanted to avoid the timeout if we knew that there was no device connected
during suspend.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 2/3] PCI: qcom: Check for the presence of a device instead of Link up during suspend

[Bjorn Helgaas]

On Thu, Nov 13, 2025 at 10:24:17PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Nov 13, 2025 at 10:41:47AM -0600, Bjorn Helgaas wrote:
> > On Fri, Nov 07, 2025 at 10:13:18AM +0530, Manivannan Sadhasivam wrote:
> > > The suspend handler checks for the PCIe Link up to decide when to turn off
> > > the controller resources. But this check is racy as the PCIe Link can go
> > > down just after this check.
> > > 
> > > So use the newly introduced API, pci_root_ports_have_device() that checks
> > > for the presence of a device under any of the Root Ports to replace the
> > > Link up check.
> > 
> > Why is pci_root_ports_have_device() itself not racy?
> 
> Because it is very uncommon for the 'pci_dev' to go away during the
> host controller suspend. It might still be possible in edge cases,
> but very common as the link down. I can reword it.

I guess it's better to acknowledge replacing one race with another
than it would be to suggest that this *removes* a race.

But I don't understand the point of this.  Is
pci_root_ports_have_device() *less* racy than the
qcom_pcie_suspend_noirq() check?  Why would that be?

I'm kind of skeptical about adding pci_root_ports_have_device() at
all.  It seems like it just encourages racy behavior in drivers.

> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 805edbbfe7eb..b2b89e2e4916 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -2018,6 +2018,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > >  static int qcom_pcie_suspend_noirq(struct device *dev)
> > >  {
> > >  	struct qcom_pcie *pcie;
> > > +	struct dw_pcie_rp *pp;
> > >  	int ret = 0;
> > >  
> > >  	pcie = dev_get_drvdata(dev);
> > > @@ -2053,8 +2054,9 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> > >  	 * 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);
> > > +	pp = &pcie->pci->pp;
> > > +	if (!pci_root_ports_have_device(pp->bridge->bus)) {
> > > +		qcom_pcie_host_deinit(pp);
> > >  		pcie->suspended = true;
> > >  	}
> > >  
> > > -- 
> > > 2.48.1
> > > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்

Re: Re: [PATCH v2 3/3] PCI: dwc: Check for the device presence during suspend and resume

[zhangsenchuan]

> -----Original Messages-----
> From: "Manivannan Sadhasivam" <mani@kernel.org>
> Send time:Friday, 14/11/2025 01:01:27
> To: "Bjorn Helgaas" <helgaas@kernel.org>
> Cc: "Manivannan Sadhasivam" <manivannan.sadhasivam@oss.qualcomm.com>, lpieralisi@kernel.org, kwilczynski@kernel.org, bhelgaas@google.com, will@kernel.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, robh@kernel.org, linux-arm-msm@vger.kernel.org, zhangsenchuan@eswincomputing.com, vincent.guittot@linaro.org, "Frank Li" <Frank.li@nxp.com>
> Subject: Re: [PATCH v2 3/3] PCI: dwc: Check for the device presence during suspend and resume
> 
> On Thu, Nov 13, 2025 at 10:40:13AM -0600, Bjorn Helgaas wrote:
> > [+cc Frank]
> > 
> > On Fri, Nov 07, 2025 at 10:13:19AM +0530, Manivannan Sadhasivam wrote:
> > > If there is no device available under the Root Ports, there is no point in
> > > sending PME_Turn_Off and waiting for L2/L3 transition during suspend, it
> > > will result in a timeout. Hence, skip those steps if no device is available
> > > during suspend.
> > > 
> > > During resume, do not wait for the link up if there was no device connected
> > > before suspend. It is very unlikely that a device will get connected while
> > > the host system was suspended.
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > ---
> > >  drivers/pci/controller/dwc/pcie-designware-host.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > index 20c9333bcb1c..5a39e7139ec9 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -20,6 +20,7 @@
> > >  #include <linux/platform_device.h>
> > >  
> > >  #include "../../pci.h"
> > > +#include "../pci-host-common.h"
> > >  #include "pcie-designware.h"
> > >  
> > >  static struct pci_ops dw_pcie_ops;
> > > @@ -1129,6 +1130,9 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> > >  	u32 val;
> > >  	int ret;
> > >  
> > > +	if (!pci_root_ports_have_device(pci->pp.bridge->bus))
> > > +		goto stop_link;
> > 
> > This looks racy.  Maybe it's still OK, but I think it would be good to
> > include a comment to acknowledge that and explain why either outcome
> > is acceptable, e.g., if a user removes a device during suspend, it
> > results in a timeout but nothing more terrible.
> > 
> 
> Ok.
> 
> > >  	/*
> > >  	 * If L1SS is supported, then do not put the link into L2 as some
> > >  	 * devices such as NVMe expect low resume latency.
> > > @@ -1162,6 +1166,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> > >  	 */
> > >  	udelay(1);
> > >  
> > > +stop_link:
> > >  	dw_pcie_stop_link(pci);
> > >  	if (pci->pp.ops->deinit)
> > >  		pci->pp.ops->deinit(&pci->pp);
> > > @@ -1195,6 +1200,14 @@ int dw_pcie_resume_noirq(struct dw_pcie *pci)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	/*
> > > +	 * If there was no device before suspend, skip waiting for link up as
> > > +	 * it is bound to fail. It is very unlikely that a device will get
> > > +	 * connected *during* suspend.
> > 
> > I'm not convinced.  Unlike the suspend side, where the race window is
> > tiny, here the window is the entire time the system is suspended, and
> > at least in laptop usage, there's no reason I would hesitate to plug
> > something in while suspended.
> > 
> 
> In that case, we just need to do:
> 
> 	/* Ignore errors as there could be no devices connected */
> 	dw_pcie_wait_for_link()
> 
> I wanted to avoid the timeout if we knew that there was no device connected
> during suspend.
> 
Hi, Mani

Although it will ignore the judgment during normal device connection.
Fortunately, this function will print prompt information. Perhaps, this is a
good choice. There is also such a practice in the dw_pcie_host_init function.
for example in the dw_pcie_host_init:

    /* Ignore errors. the link may come up later */
     dw_pcie_wait_for_link(pci);

Perhaps there are other better methods. For now, this is what I have seen.

Kind regards,
Senchuan Zhang

Re: [PATCH v2 2/3] PCI: qcom: Check for the presence of a device instead of Link up during suspend

[Manivannan Sadhasivam]

On Thu, Nov 13, 2025 at 11:22:50AM -0600, Bjorn Helgaas wrote:
> On Thu, Nov 13, 2025 at 10:24:17PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Nov 13, 2025 at 10:41:47AM -0600, Bjorn Helgaas wrote:
> > > On Fri, Nov 07, 2025 at 10:13:18AM +0530, Manivannan Sadhasivam wrote:
> > > > The suspend handler checks for the PCIe Link up to decide when to turn off
> > > > the controller resources. But this check is racy as the PCIe Link can go
> > > > down just after this check.
> > > > 
> > > > So use the newly introduced API, pci_root_ports_have_device() that checks
> > > > for the presence of a device under any of the Root Ports to replace the
> > > > Link up check.
> > > 
> > > Why is pci_root_ports_have_device() itself not racy?
> > 
> > Because it is very uncommon for the 'pci_dev' to go away during the
> > host controller suspend. It might still be possible in edge cases,
> > but very common as the link down. I can reword it.
> 
> I guess it's better to acknowledge replacing one race with another
> than it would be to suggest that this *removes* a race.
> 

Ok.

> But I don't understand the point of this.  Is
> pci_root_ports_have_device() *less* racy than the
> qcom_pcie_suspend_noirq() check?  Why would that be?
> 

The check is supposed to perform deinit only if there are no devices connected
to the slot. And the reason to skip the deinit was mostly due to driver behavior
like NVMe driver, which expects the device to be in D0 even during system
suspend on non-x86 platforms.

Since the check is for the existence of the device nevertheless, I thought,
making use of pci_root_ports_have_device() serves the purpose instead of
checking the data link layer status.

> I'm kind of skeptical about adding pci_root_ports_have_device() at
> all.  It seems like it just encourages racy behavior in drivers.
> 

I agree that though it is not very common, but with async suspend, it is
possible that 'pci_dev' may get removed during controller suspend.

So I've dropped this series from controller/dwc until we conclude.

- Mani

> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > > ---
> > > >  drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > index 805edbbfe7eb..b2b89e2e4916 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > @@ -2018,6 +2018,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > >  static int qcom_pcie_suspend_noirq(struct device *dev)
> > > >  {
> > > >  	struct qcom_pcie *pcie;
> > > > +	struct dw_pcie_rp *pp;
> > > >  	int ret = 0;
> > > >  
> > > >  	pcie = dev_get_drvdata(dev);
> > > > @@ -2053,8 +2054,9 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> > > >  	 * 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);
> > > > +	pp = &pcie->pci->pp;
> > > > +	if (!pci_root_ports_have_device(pp->bridge->bus)) {
> > > > +		qcom_pcie_host_deinit(pp);
> > > >  		pcie->suspended = true;
> > > >  	}
> > > >  
> > > > -- 
> > > > 2.48.1
> > > > 
> > 
> > -- 
> > மணிவண்ணன் சதாசிவம்

-- 
மணிவண்ணன் சதாசிவம்

Re: Re: [PATCH v2 2/3] PCI: qcom: Check for the presence of a device instead of Link up during suspend

[zhangsenchuan]

> -----Original Messages-----
> From: "Manivannan Sadhasivam" <mani@kernel.org>
> Send time:Tuesday, 18/11/2025 01:37:01
> To: "Bjorn Helgaas" <helgaas@kernel.org>
> Cc: "Manivannan Sadhasivam" <manivannan.sadhasivam@oss.qualcomm.com>, lpieralisi@kernel.org, kwilczynski@kernel.org, bhelgaas@google.com, will@kernel.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, robh@kernel.org, linux-arm-msm@vger.kernel.org, zhangsenchuan@eswincomputing.com, vincent.guittot@linaro.org
> Subject: Re: [PATCH v2 2/3] PCI: qcom: Check for the presence of a device instead of Link up during suspend
> 
> On Thu, Nov 13, 2025 at 11:22:50AM -0600, Bjorn Helgaas wrote:
> > On Thu, Nov 13, 2025 at 10:24:17PM +0530, Manivannan Sadhasivam wrote:
> > > On Thu, Nov 13, 2025 at 10:41:47AM -0600, Bjorn Helgaas wrote:
> > > > On Fri, Nov 07, 2025 at 10:13:18AM +0530, Manivannan Sadhasivam wrote:
> > > > > The suspend handler checks for the PCIe Link up to decide when to turn off
> > > > > the controller resources. But this check is racy as the PCIe Link can go
> > > > > down just after this check.
> > > > > 
> > > > > So use the newly introduced API, pci_root_ports_have_device() that checks
> > > > > for the presence of a device under any of the Root Ports to replace the
> > > > > Link up check.
> > > > 
> > > > Why is pci_root_ports_have_device() itself not racy?
> > > 
> > > Because it is very uncommon for the 'pci_dev' to go away during the
> > > host controller suspend. It might still be possible in edge cases,
> > > but very common as the link down. I can reword it.
> > 
> > I guess it's better to acknowledge replacing one race with another
> > than it would be to suggest that this *removes* a race.
> > 
> 
> Ok.
> 
> > But I don't understand the point of this.  Is
> > pci_root_ports_have_device() *less* racy than the
> > qcom_pcie_suspend_noirq() check?  Why would that be?
> > 
> 
> The check is supposed to perform deinit only if there are no devices connected
> to the slot. And the reason to skip the deinit was mostly due to driver behavior
> like NVMe driver, which expects the device to be in D0 even during system
> suspend on non-x86 platforms.
> 
> Since the check is for the existence of the device nevertheless, I thought,
> making use of pci_root_ports_have_device() serves the purpose instead of
> checking the data link layer status.
> 
> > I'm kind of skeptical about adding pci_root_ports_have_device() at
> > all.  It seems like it just encourages racy behavior in drivers.
> > 
> 
> I agree that though it is not very common, but with async suspend, it is
> possible that 'pci_dev' may get removed during controller suspend.
> 
> So I've dropped this series from controller/dwc until we conclude.
> 

Hi, Mani

I see that this series from controller/dwc has been temporarily removed. 
Do I need to wait for a conclusion later before submitting the code, or
do I need to continue submitting the pcie v6 patch based on the latest 
6.18-rc6 branch?

Kind regards,
Senchuan Zhang

> 
> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > > > ---
> > > > >  drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++--
> > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > index 805edbbfe7eb..b2b89e2e4916 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > @@ -2018,6 +2018,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > > >  static int qcom_pcie_suspend_noirq(struct device *dev)
> > > > >  {
> > > > >  	struct qcom_pcie *pcie;
> > > > > +	struct dw_pcie_rp *pp;
> > > > >  	int ret = 0;
> > > > >  
> > > > >  	pcie = dev_get_drvdata(dev);
> > > > > @@ -2053,8 +2054,9 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> > > > >  	 * 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);
> > > > > +	pp = &pcie->pci->pp;
> > > > > +	if (!pci_root_ports_have_device(pp->bridge->bus)) {
> > > > > +		qcom_pcie_host_deinit(pp);
> > > > >  		pcie->suspended = true;
> > > > >  	}
> > > > >  
> > > > > -- 
> > > > > 2.48.1
> > > > > 
> > > 
> > > -- 

Re: [PATCH v2 2/3] PCI: qcom: Check for the presence of a device instead of Link up during suspend

[Manivannan Sadhasivam]

On Wed, Nov 19, 2025 at 04:03:22PM +0800, zhangsenchuan wrote:
> 
> 
> 
> > -----Original Messages-----
> > From: "Manivannan Sadhasivam" <mani@kernel.org>
> > Send time:Tuesday, 18/11/2025 01:37:01
> > To: "Bjorn Helgaas" <helgaas@kernel.org>
> > Cc: "Manivannan Sadhasivam" <manivannan.sadhasivam@oss.qualcomm.com>, lpieralisi@kernel.org, kwilczynski@kernel.org, bhelgaas@google.com, will@kernel.org, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, robh@kernel.org, linux-arm-msm@vger.kernel.org, zhangsenchuan@eswincomputing.com, vincent.guittot@linaro.org
> > Subject: Re: [PATCH v2 2/3] PCI: qcom: Check for the presence of a device instead of Link up during suspend
> > 
> > On Thu, Nov 13, 2025 at 11:22:50AM -0600, Bjorn Helgaas wrote:
> > > On Thu, Nov 13, 2025 at 10:24:17PM +0530, Manivannan Sadhasivam wrote:
> > > > On Thu, Nov 13, 2025 at 10:41:47AM -0600, Bjorn Helgaas wrote:
> > > > > On Fri, Nov 07, 2025 at 10:13:18AM +0530, Manivannan Sadhasivam wrote:
> > > > > > The suspend handler checks for the PCIe Link up to decide when to turn off
> > > > > > the controller resources. But this check is racy as the PCIe Link can go
> > > > > > down just after this check.
> > > > > > 
> > > > > > So use the newly introduced API, pci_root_ports_have_device() that checks
> > > > > > for the presence of a device under any of the Root Ports to replace the
> > > > > > Link up check.
> > > > > 
> > > > > Why is pci_root_ports_have_device() itself not racy?
> > > > 
> > > > Because it is very uncommon for the 'pci_dev' to go away during the
> > > > host controller suspend. It might still be possible in edge cases,
> > > > but very common as the link down. I can reword it.
> > > 
> > > I guess it's better to acknowledge replacing one race with another
> > > than it would be to suggest that this *removes* a race.
> > > 
> > 
> > Ok.
> > 
> > > But I don't understand the point of this.  Is
> > > pci_root_ports_have_device() *less* racy than the
> > > qcom_pcie_suspend_noirq() check?  Why would that be?
> > > 
> > 
> > The check is supposed to perform deinit only if there are no devices connected
> > to the slot. And the reason to skip the deinit was mostly due to driver behavior
> > like NVMe driver, which expects the device to be in D0 even during system
> > suspend on non-x86 platforms.
> > 
> > Since the check is for the existence of the device nevertheless, I thought,
> > making use of pci_root_ports_have_device() serves the purpose instead of
> > checking the data link layer status.
> > 
> > > I'm kind of skeptical about adding pci_root_ports_have_device() at
> > > all.  It seems like it just encourages racy behavior in drivers.
> > > 
> > 
> > I agree that though it is not very common, but with async suspend, it is
> > possible that 'pci_dev' may get removed during controller suspend.
> > 
> > So I've dropped this series from controller/dwc until we conclude.
> > 
> 
> Hi, Mani
> 
> I see that this series from controller/dwc has been temporarily removed. 
> Do I need to wait for a conclusion later before submitting the code, or
> do I need to continue submitting the pcie v6 patch based on the latest 
> 6.18-rc6 branch?
> 

I'm coming up with a new version of the patch, but you do not need to wait for
it.

- Mani

> Kind regards,
> Senchuan Zhang
> 
> > 
> > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> > > > > > ---
> > > > > >  drivers/pci/controller/dwc/pcie-qcom.c | 6 ++++--
> > > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > > index 805edbbfe7eb..b2b89e2e4916 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > > > > @@ -2018,6 +2018,7 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> > > > > >  static int qcom_pcie_suspend_noirq(struct device *dev)
> > > > > >  {
> > > > > >  	struct qcom_pcie *pcie;
> > > > > > +	struct dw_pcie_rp *pp;
> > > > > >  	int ret = 0;
> > > > > >  
> > > > > >  	pcie = dev_get_drvdata(dev);
> > > > > > @@ -2053,8 +2054,9 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> > > > > >  	 * 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);
> > > > > > +	pp = &pcie->pci->pp;
> > > > > > +	if (!pci_root_ports_have_device(pp->bridge->bus)) {
> > > > > > +		qcom_pcie_host_deinit(pp);
> > > > > >  		pcie->suspended = true;
> > > > > >  	}
> > > > > >  
> > > > > > -- 
> > > > > > 2.48.1
> > > > > > 
> > > > 
> > > > -- 
> 

-- 
மணிவண்ணன் சதாசிவம்