> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: 2024年11月6日 7:27
> To: Hongxing Zhu <hongxing.zhu@xxxxxxx>
> Cc: kwilczynski@xxxxxxxxxx; bhelgaas@xxxxxxxxxx;
> lorenzo.pieralisi@xxxxxxx; Frank Li <frank.li@xxxxxxx>; mani@xxxxxxxxxx;
> linux-pci@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx
> 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@xxxxxxx>
> > Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> > .../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
> >
