Re: [RESEND v3 1/5] PCI: dwc: Don't poll L2 if QUIRK_NOL2POLL_IN_PM is existing in suspend

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Aug 19, 2025 at 05:51:41AM +0000, Hongxing Zhu wrote:
> > From: Bjorn Helgaas <helgaas@kernel.org>
> > On Mon, Aug 18, 2025 at 03:32:01PM +0800, Richard Zhu wrote:
> > > Refer to PCIe r6.0, sec 5.2, fig 5-1 Link Power Management State Flow
> > > Diagram. Both L0 and L2/L3 Ready can be transferred to LDn directly.
> ...

> > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > @@ -1007,7 +1007,7 @@ int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> > > {
> > >  	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > >  	u32 val;
> > > -	int ret;
> > > +	int ret = 0;
> > >
> > >       /*
> > >        * If L1SS is supported, then do not put the link into L2 as
> > > some
> >          * devices such as NVMe expect low resume latency.
> >          */
> >          if (dw_pcie_readw_dbi(pci, offset + PCI_EXP_LNKCTL) &
> > PCI_EXP_LNKCTL_ASPM_L1)
> >                 return 0;
> > 
> > You didn't change it in this patch (the L1SS test was added by
> > 4774faf854f5 ("PCI: dwc: Implement generic suspend/resume
> > functionality")), but this L1SS check is an encapsulation problem.
> > The ASPM configuration shouldn't leak out here in such an ad hoc
> > way.

> Should I created another commit to get ride of the L1SS check codes?

If we remove that check, I guess we would put the link into L2 when
ASPM L1 is enabled (not when "L1SS is supported" as the comment
claims).

Obviously this check was added for a reason, so I assume something bad
would happen if we removed it.  But at the same time, AFAICS this
check only applies to imx6 and layerscape because none of the other
drivers use dw_pcie_suspend_noirq().

So yes, I do think it should be removed because it's only a partial
band-aid for whatever the problem is.  It would probably break
something, but it looks to me like it's already broken on most
platforms, and we need to figure out a real solution that fixes
everybody.

> > *All* drivers, not just NVMe, would prefer low resume latency.
> > 
> > How do we deal with this in other host controller drivers?  If any
> > other driver puts links in L2, I suppose they would have the same
> > issue?  Maybe DWC is the only one that puts the link in L2?
> > 
> > What happens when we add a new driver that puts links in L2?  I
> > guess we'll be debugging some NVMe issue again?
>
> Up to now, this is the first one routine to do the L1SS check in L2
> entry.