| 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: <20250905224430.GA1325412@bhelgaas>
Date: Fri, 5 Sep 2025 17:44:30 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: manivannan.sadhasivam@....qualcomm.com
Cc: Manivannan Sadhasivam <mani@...nel.org>,
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 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#?
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.
> 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.
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) # 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.
I wish the max_link_speed checks used some kind of #define to make
them readable.
Bjorn
Powered by blists - more mailing lists