| 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
| ||
|
Message-ID: <yfag3ox6ifg5nvi4ayfcx3mvj5zfn2d2quwiakuczp7o3lwuy6@t3n4h23fsmiw>
Date: Fri, 21 Nov 2025 21:56:05 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Hongxing Zhu <hongxing.zhu@....com>
Cc: Shawn Lin <shawn.lin@...k-chips.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-pci@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"vincent.guittot@...aro.org" <vincent.guittot@...aro.org>,
"zhangsenchuan@...incomputing.com" <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
On Fri, Nov 21, 2025 at 05:21:58AM +0000, Hongxing Zhu wrote:
> > -----Original Message-----
> > From: Manivannan Sadhasivam <mani@...nel.org>
> > Sent: 2025年11月20日 13:37
> > To: Shawn Lin <shawn.lin@...k-chips.com>
> > Cc: Hongxing 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?
> >
> Hi Mani:
> Yes, it is.
> In my initial upstreaming patches, the intention to check this state is to
> figure out that there is an endpoint device or not.
>
If so, why do we need to check for LTSSM states other than
DW_PCIE_LTSSM_DETECT_QUIET and DW_PCIE_LTSSM_DETECT_ACT?
Did spec mandate it or you did it for some specific reason?
- Mani
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists