On Tue, Aug 19, 2025 at 05:51:41AM +0000, Hongxing Zhu wrote:

quoted

From: Bjorn Helgaas <helgaas@kernel.org>
On Mon, Aug 18, 2025 at 03:32:01PM +0800, Richard Zhu wrote:

quoted

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.

...

quoted

quoted

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

quoted

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