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

[zhangsenchuan <zhangsenchuan@eswincomputing.com>]

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