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: <7kmf767g4jelftwbbvikk6h3tclkih3t5zrkffeyfjuy2qe3uw@tok2rpim6f7d>
Date: Sat, 6 Sep 2025 16:13:08 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: manivannan.sadhasivam@....qualcomm.com, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Bartosz Golaszewski <brgl@...ev.pl>, Saravana Kannan <saravanak@...gle.com>, 
	linux-pci@...r.kernel.org, linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>, 
	Brian Norris <briannorris@...omium.org>, stable+noautosel@...nel.org
Subject: Re: [PATCH v2 1/5] PCI: qcom: Wait for PCIE_RESET_CONFIG_WAIT_MS
 after PERST# deassert

On Fri, Sep 05, 2025 at 05:44:30PM GMT, Bjorn Helgaas wrote:
> On Wed, Sep 03, 2025 at 12:43:23PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > PCIe spec r6.0, sec 6.6.1 mandates waiting for 100ms before deasserting
> > PERST# if the downstream port does not support Link speeds greater than
> > 5.0 GT/s.
> 
> I guess you mean we need to wait 100ms *after* deasserting PERST#?
> 

Right, that's a typo.

> I.e., this wait before sending config requests to a downstream device:
> 
>   ◦ With a Downstream Port that does not support Link speeds greater
>     than 5.0 GT/s, software must wait a minimum of 100 ms following
>     exit from a Conventional Reset before sending a Configuration
>     Request to the device immediately below that Port.
> 
> > But in practice, this delay seem to be required irrespective of
> > the supported link speed as it gives the endpoints enough time to
> > initialize.
> 
> Saying "but in practice ... seems to be required" suggests that the
> spec requirement isn't actually enough.  But the spec does say the
> 100ms delay before config requests is required for all link speeds.
> The difference is when we start that timer: for 5 GT/s or slower it
> starts at exit from Conventional Reset; for faster than 5 GT/s it
> starts when link training completes.
> 
> > Hence, add the delay by reusing the PCIE_RESET_CONFIG_WAIT_MS definition if
> > the linkup_irq is not supported. If the linkup_irq is supported, the driver
> > already waits for 100ms in the IRQ handler post link up.
> 
> I didn't look into this, but I wondered whether it's possible to miss
> the interrupt, especially the first time during probe.
> 

No, it is not. Since, the controller reinitializes itself during probe() and it
starts LTSSM. So once link up happens, this IRQ will be triggered.

> > Also, remove the redundant comment for PCIE_T_PVPERL_MS. Finally, the
> > PERST_DELAY_US sleep can be moved to PERST# assert where it should be.
> 
> Unless this PERST_DELAY_US move is logically part of the
> PCIE_RESET_CONFIG_WAIT_MS change, putting it in a separate patch would
> make *this* patch easier to read.
> 
> > Cc: stable+noautosel@...nel.org # non-trivial dependency
> > Fixes: 82a823833f4e ("PCI: qcom: Add Qualcomm PCIe controller driver")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-qcom.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 294babe1816e4d0c2b2343fe22d89af72afcd6cd..bcd080315d70e64eafdefd852740fe07df3dbe75 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -302,20 +302,22 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> >  	else
> >  		list_for_each_entry(port, &pcie->ports, list)
> >  			gpiod_set_value_cansleep(port->reset, val);
> > -
> > -	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> >  }
> >  
> >  static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> >  {
> >  	qcom_perst_assert(pcie, true);
> > +	usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> >  }
> >  
> >  static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> >  {
> > -	/* Ensure that PERST has been asserted for at least 100 ms */
> > +	struct dw_pcie_rp *pp = &pcie->pci->pp;
> > +
> >  	msleep(PCIE_T_PVPERL_MS);
> >  	qcom_perst_assert(pcie, false);
> > +	if (!pp->use_linkup_irq)
> > +		msleep(PCIE_RESET_CONFIG_WAIT_MS);
> 
> I'm a little confused about why you test pp->use_linkup_irq here
> instead of testing the max supported link speed.  And I'm not positive
> that this is the right place, at least at this point in the series.
> 

Because, pci->max_link_speed used to only cache the value of the
'max-link-speed' DT property. But I totally forgot that I changed that behavior
to cache the max supported link speed of the Root Port with commit:
19a69cbd9d43 ("PCI: dwc: Always cache the maximum link speed value in
dw_pcie::max_link_speed")

So yes, I should've been using 'pci->max_link_speed' here.

> At this point, qcom_ep_reset_deassert() is only used by
> qcom_pcie_host_init(), so the flow is like this:
> 
>   qcom_pcie_probe
>     irq = platform_get_irq_byname_optional(pdev, "global")
>     if (irq > 0)
>       pp->use_linkup_irq = true
>     dw_pcie_host_init
>       pp->ops->init
>         qcom_pcie_host_init                         # .init
>           qcom_ep_reset_deassert                    # <--
>  +          if (!pp->use_linkup_irq)
>  +            msleep(PCIE_RESET_CONFIG_WAIT_MS)     # 100ms
>       if (!dw_pcie_link_up(pci))
>         dw_pcie_start_link
>       if (!pp->use_linkup_irq)
>         dw_pcie_wait_for_link
>           for (retries = 0; retries < PCIE_LINK_WAIT_MAX_RETRIES; retries++)
>             if (dw_pcie_link_up(pci))
>               break
>             msleep(PCIE_LINK_WAIT_SLEEP_MS)         # 90ms
>           if (pci->max_link_speed > 2)              # > 5.0 GT/s
>             msleep(PCIE_RESET_CONFIG_WAIT_MS)       # 100ms
> 
> For slow links (<= 5 GT/s), it's possible that the link comes up
> before we even call dw_pcie_link_up() the first time, which would mean
> we really don't wait at all.
> 
> My guess is that most links wouldn't come up that fast but *would*
> come up within 90ms.  Even in that case, we wouldn't quite meet the
> spec 100ms requirement.
> 
> I wonder if dw_pcie_wait_for_link() should look more like this:
> 
>   dw_pcie_wait_for_link
>     if (pci->max_link_speed <= 2)                   # <= 5.0 GT/s
>       msleep(PCIE_RESET_CONFIG_WAIT_MS)         dw_pcie_wait_for_link    # 100ms
> 
>     for (retries = 0; retries < PCIE_LINK_WAIT_MAX_RETRIES; retries++)
>       if (dw_pcie_link_up(pci))
>         break;
>       msleep(...)
> 
>     if (pci->max_link_speed > 2)                    # > 5.0 GT/s
>       msleep(PCIE_RESET_CONFIG_WAIT_MS)             # 100ms
> 
> Then we'd definitely wait the required 100ms even for the slow links.
> The retry loop could start with a much shorter interval and back off.
> 

Your concerns are valid. I'll submit a separate series along with *this* patch
to fix these. I don't think clubbing these with this series is a good idea.

> I wish the max_link_speed checks used some kind of #define to make
> them readable.
> 

It is mostly because it used to hold the DT property value which starts from 1.
We could still convert it to 'enum pci_bus_speed', but that is for a separate
cleanup.

- Mani

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ