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

[zhangsenchuan <zhangsenchuan@eswincomputing.com>]

> -----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