Re: [PATCH v6 1/4] PCI: dwc: Remove the L1SS check before putting the link into L2

[Frank Li <Frank.li@nxp.com>]

On Wed, Sep 24, 2025 at 05:06:05PM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 24, 2025 at 04:59:11PM -0400, Frank Li wrote:
> > On Wed, Sep 24, 2025 at 02:44:57PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Sep 24, 2025 at 03:23:21PM +0800, Richard Zhu wrote:
> > > > The ASPM configuration shouldn't leak out here. Remove the L1SS check
> > > > during L2 entry.
> > >
> > > I'm all in favor of removing this code if possible, but we need to
> > > explain why this is safe.  The L1SS check was added for some reason,
> > > and we need to explain why that reason doesn't apply.
> >
> > That's original discussion
> > https://lore.kernel.org/linux-pci/20230720160738.GC48270@thinkpad/
> >
> > "To be precise, NVMe driver will shutdown the device if there is no
> > ASPM support and keep it in low power mode otherwise (there are
> > other cases as well but we do not need to worry).
> >
> > But here you are not checking for ASPM state in the suspend path,
> > and just forcing the link to be in L2/L3 (thereby D3Cold) even
> > though NVMe driver may expect it to be in low power state like
> > ASPM/APST.
> >
> > So you should only put the link to L2/L3 if there is no ASPM
> > support. Otherwise, you'll ending up with bug reports when users
> > connect NVMe to it.
> >
> > - Mani"
>
> Whatever the reasoning is, it needs to be in the commit log.  The
> above might be leading to the reasoning, but it would need a lot more
> dots to be connected to be persuasive.
>
> If NVMe is making assumptions about the ASPM configuration, there
> needs to be some generic way to keep track of that.  E.g., if NVMe
> doesn't work correctly with some ASPM states, maybe it shouldn't
> advertise support for those states.  Hacking up every host controller
> driver doesn't seem like a viable approach.

Manivannan Sadhasivam:

	Does above situation still exist at current kernel?

Frank

>
> > > > Cc: stable@vger.kernel.org
> > > > Fixes: 4774faf854f5 ("PCI: dwc: Implement generic suspend/resume functionality")
> > > > Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> > > > Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
> > > > ---
> > > >  drivers/pci/controller/dwc/pcie-designware-host.c | 8 --------
> > > >  1 file changed, 8 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > index 952f8594b501..9d46d1f0334b 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > @@ -1005,17 +1005,9 @@ static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
> > > >
> > > >  int dw_pcie_suspend_noirq(struct dw_pcie *pci)
> > > >  {
> > > > -	u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > > >  	u32 val;
> > > >  	int ret;
> > > >
> > > > -	/*
> > > > -	 * 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;
> > > > -
> > > >  	if (pci->pp.ops->pme_turn_off) {
> > > >  		pci->pp.ops->pme_turn_off(&pci->pp);
> > > >  	} else {
> > > > --
> > > > 2.37.1
> > > >