[PATCH v2] PCI: dwc: Fix resume failure if no EP is connected at some platforms

[Hongxing Zhu]

> -----Original Message-----
> From: Bjorn Helgaas <helgaas at kernel.org>
> Sent: 2024年11月6日 7:27
> To: Hongxing Zhu <hongxing.zhu at nxp.com>
> Cc: kwilczynski at kernel.org; bhelgaas at google.com;
> lorenzo.pieralisi at arm.com; Frank Li <frank.li at nxp.com>; mani at kernel.org;
> linux-pci at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> linux-kernel at vger.kernel.org; kernel at pengutronix.de; imx at lists.linux.dev
> Subject: Re: [PATCH v2] PCI: dwc: Fix resume failure if no EP is connected at
> some platforms
> 
> On Mon, Jul 22, 2024 at 02:15:13PM +0800, Richard Zhu wrote:
> > The dw_pcie_suspend_noirq() function currently returns success
> > directly if no endpoint (EP) device is connected. However, on some
> > platforms, power loss occurs during suspend, causing dw_resume() to do
> nothing in this case.
> > This results in a system halt because the DWC controller is not
> > initialized after power-on during resume.
> 
> dw_resume() doesn't exist.  What function did you mean?
Actually, it is dw_pcie_resume_noirq()
> 
> System halt?  In dw_pcie_resume_noirq()?  What causes the halt?  A
> NULL pointer dereference?  A CPU hang because a read of some controller
> register never completes?  Feels a little hand-wavy.
When no endpoint(EP) device is connected. Power loss occurs during suspend,
 then the controllers isn't a ready stat anymore. Since dw_pcie_suspend_noirq()
 return directly with success, dw_pcie_resume_noirq() would assume that the
 controller is still ready, and wouldn't re-initialized the controller.
At end, there would be a halt when driver accesses controller's registers.

> 
> Another comment below.
> 
> > Change call to deinit() in suspend and init() at resume regardless of
> > whether there are EP device connections or not. It is not harmful to
> > perform deinit() and init() again for the no power-off case, and it
> > keeps the code simple and consistent in logic.
> >
> > Fixes: 4774faf854f5 ("PCI: dwc: Implement generic suspend/resume
> > functionality")
> > Signed-off-by: Richard Zhu <hongxing.zhu at nxp.com>
> > Reviewed-by: Frank Li <Frank.Li at nxp.com>
> > ---
> >  .../pci/controller/dwc/pcie-designware-host.c | 30
> > +++++++++----------
> >  1 file changed, 15 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c
> > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index a0822d5371bc5..cb8c3c2bcc790 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -933,23 +933,23 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> >  	if (dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKCTL) &
> PCI_EXP_LNKCTL_ASPM_L1)
> >  		return 0;
> >
> > -	if (dw_pcie_get_ltssm(pci) <= DW_PCIE_LTSSM_DETECT_ACT)
> > -		return 0;
> > -
> > -	if (pci->pp.ops->pme_turn_off)
> > -		pci->pp.ops->pme_turn_off(&pci->pp);
> > -	else
> > -		ret = dw_pcie_pme_turn_off(pci);
> > +	if (dw_pcie_get_ltssm(pci) > DW_PCIE_LTSSM_DETECT_ACT) {
> > +		/* Only send out PME_TURN_OFF when PCIE link is up */
> > +		if (pci->pp.ops->pme_turn_off)
> > +			pci->pp.ops->pme_turn_off(&pci->pp);
> > +		else
> > +			ret = dw_pcie_pme_turn_off(pci);
> 
> This looks possibly racy since the link can go down at any point.
> 
When link is down and without this commit changes, dw_pcie_suspend_noirq()
 return directly, and the PME_TURN_OFF wouldn't be kicked off.

I change the behavior to issue the PME_TURN_OFF when link is up here.

Best Regards
Richard Zhu
> > -	if (ret)
> > -		return ret;
> > +		if (ret)
> > +			return ret;
> >
> > -	ret = read_poll_timeout(dw_pcie_get_ltssm, val, val ==
> DW_PCIE_LTSSM_L2_IDLE,
> > -				PCIE_PME_TO_L2_TIMEOUT_US/10,
> > -				PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> > -	if (ret) {
> > -		dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM: 0x%x\n",
> val);
> > -		return ret;
> > +		ret = read_poll_timeout(dw_pcie_get_ltssm, val, val ==
> DW_PCIE_LTSSM_L2_IDLE,
> > +					PCIE_PME_TO_L2_TIMEOUT_US/10,
> > +					PCIE_PME_TO_L2_TIMEOUT_US, false, pci);
> > +		if (ret) {
> > +			dev_err(pci->dev, "Timeout waiting for L2 entry! LTSSM:
> 0x%x\n", val);
> > +			return ret;
> > +		}
> >  	}
> >
> >  	if (pci->pp.ops->deinit)
> > --
> > 2.37.1
> >