PCI: dwc: Check for device presence in suspend and resume

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

Posted by Manivannan Sadhasivam 1 month, 1 week ago

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 3/3] PCI: dwc: Check for the device presence during suspend and resume

Posted by Bjorn Helgaas 1 month ago

[+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 3/3] PCI: dwc: Check for the device presence during suspend and resume

Posted by Manivannan Sadhasivam 1 month ago

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: Re: [PATCH v2 3/3] PCI: dwc: Check for the device presence during suspend and resume

Posted by zhangsenchuan 1 month ago



> -----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 3/3] PCI: dwc: Check for the device presence during suspend and resume

Posted by Frank Li 1 month, 1 week ago

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 3/3] PCI: dwc: Check for the device presence during suspend and resume

Posted by Manivannan Sadhasivam 1 month ago

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

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