linux-kernel - Re: [PATCH 2/2] PCI: dwc: Do not return failure from dw_pcie_wait_for

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <jmysdqydimjl7min6dw34bdcf6hiyk3pqb4plzvzl6folgat5n@v55h5i7pufg3>
Date: Thu, 20 Nov 2025 11:07:25 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Shawn Lin <shawn.lin@...k-chips.com>
Cc: Richard Zhu <hongxing.zhu@....com>, Frank Li <Frank.li@....com>, 
	Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>, Jingoo Han <jingoohan1@...il.com>, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org, 
	linux-kernel@...r.kernel.org, vincent.guittot@...aro.org, zhangsenchuan@...incomputing.com
Subject: Re: [PATCH 2/2] PCI: dwc: Do not return failure from
 dw_pcie_wait_for_link() if link is in Detect/Poll state

+ Richard, Frank

On Thu, Nov 20, 2025 at 09:13:24AM +0800, Shawn Lin wrote:
> 在 2025/11/20 星期四 2:10, Manivannan Sadhasivam 写道:
> > dw_pcie_wait_for_link() API waits for the link to be up and returns failure
> > if the link is not up within the 1 second interval. But if there was no
> > device connected to the bus, then the link up failure would be expected.
> > In that case, the callers might want to skip the failure in a hope that the
> > link will be up later when a device gets connected.
> > 
> > One of the callers, dw_pcie_host_init() is currently skipping the failure
> > irrespective of the link state, in an assumption that the link may come up
> > later. But this assumption is wrong, since LTSSM states other than Detect
> > and Poll during link training phase are considered to be fatal and the link
> > needs to be retrained.
> > 
> > So to avoid callers making wrong assumptions, skip returning failure from
> > dw_pcie_wait_for_link() if the link is in Detect or Poll state after
> > timeout and also check the return value of the API in dw_pcie_host_init().
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> > ---
> >   drivers/pci/controller/dwc/pcie-designware-host.c | 8 +++++---
> >   drivers/pci/controller/dwc/pcie-designware.c      | 8 ++++++++
> >   2 files changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 8fe3454f3b13..8c4845fd24aa 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -671,9 +671,11 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> >   	 * If there is no Link Up IRQ, we should not bypass the delay
> >   	 * because that would require users to manually rescan for devices.
> >   	 */
> > -	if (!pp->use_linkup_irq)
> > -		/* Ignore errors, the link may come up later */
> > -		dw_pcie_wait_for_link(pci);
> > +	if (!pp->use_linkup_irq) {
> > +		ret = dw_pcie_wait_for_link(pci);
> > +		if (ret)
> > +			goto err_stop_link;
> > +	}
> >   	ret = pci_host_probe(bridge);
> >   	if (ret)
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index c644216995f6..fe13c6b10ccb 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -651,6 +651,14 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci)
> >   	}
> >   	if (retries >= PCIE_LINK_WAIT_MAX_RETRIES) {
> > +		/*
> > +		 * If the link is in Detect or Poll state, it indicates that no
> > +		 * device is connected. So return success to allow the device to
> > +		 * show up later.
> > +		 */
> > +		if (dw_pcie_get_ltssm(pci) <= DW_PCIE_LTSSM_DETECT_WAIT)
> > +			return 0;
> 
> I'm afraid this might not be true. If there is no devices connected or
> the device connected without power supplied, it means there is no
> far-end pull-up termination resistor from TX view of RC. TX pulse detection
> signal from the RC side will not undergo voltage division, and
> its LTSSM state machine will only toggle between
> DW_PCIE_LTSSM_DETECT_QUIET and DW_PCIE_LTSSM_DETECT_ACT.
> 

I must admit that I just inherited this check from dw_pcie_suspend_noirq(). But
I cross checked the PCIe base spec and it mentions clearly that the LTSSM will
be in Detect.Quiet/Active states if no endpoint is detected i.e., within the 1s
timeout, the LTSSM should've transitioned back to these Detect states.

I'm wondering why we are checking for Poll and other states in
dw_pcie_suspend_noirq(). I believe the intention was to check for the presence
of an endpoint or not.

Richard, Frank, thoughts?

- Mani

-- 
மணிவண்ணன் சதாசிவம்