Re: [PATCH v8 3/6] PCI: Use standard wait times for PCIe link monitoring

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] readl_relaxed_poll_timeout() has an extremely short 100ms timeout due to incorrectly mapped arguments, breaking standard PCIe link up expectations.
- [High] PCIe Configuration Request 100ms timing constraint is violated if the link trains quickly after a fundamental reset.
--

--- Patch [3]: [PATCH v8 3/6] PCI: Use standard wait times for PCIe link monitoring ---
commit dd424bc144818153a04d4fe26f753cd9432c9695
Author: Thierry Reding <treding@nvidia.com>

PCI: Use standard wait times for PCIe link monitoring

Instead of defining the wait values for each driver, use common values
defined in the core pci.h header file. Note that while most drivers use
the usleep_range(), it looks like these were mostly cargo culted and
msleep() is a better choice given the fixed delay that the specification
calls for. Convert all drivers to msleep() and use the existing
definition.

> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index fd9c7d53e8a78..272c5c8fc1e5c 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
[ ... ]
> @@ -350,13 +347,13 @@ static int advk_pcie_wait_for_link(struct advk_pcie *pcie)
>  	int retries;
>  
>  	/* check if the link is up or not */
> -	for (retries = 0; retries < LINK_WAIT_MAX_RETRIES; retries++) {
> +	for (retries = 0; retries < PCIE_LINK_WAIT_MAX_RETRIES; retries++) {
>  		if (advk_pcie_link_up(pcie)) {
>  			pci_host_common_link_train_delay(pcie->link_gen);
>  			return 0;
>  		}
>  
> -		usleep_range(LINK_WAIT_USLEEP_MIN, LINK_WAIT_USLEEP_MAX);
> +		msleep(PCIE_LINK_WAIT_SLEEP_MS);
>  	}
>  
>  	return -ETIMEDOUT;

[Severity: High]
This is a pre-existing issue, but does this violate the 100ms PCIe
configuration request delay specification?

In advk_pcie_setup_hw(), advk_pcie_issue_perst() is called to deassert
PERST#, followed immediately by advk_pcie_wait_for_link():

advk_pcie_setup_hw() {
    advk_pcie_issue_perst(pcie);
    ...
    ret = advk_pcie_wait_for_link(pcie);
}

If the link trains instantly, advk_pcie_link_up() will return true before
any sleep occurs.

For PCIe Gen 1 or 2, pci_host_common_link_train_delay() adds no delay,
causing the function to return immediately. Doesn't this cause Configuration
Requests to be sent before the mandatory 100ms window has elapsed after
a fundamental reset?

[Severity: High]
This isn't a bug introduced by this patch, but since this series cleans up
cargo-culted delay values across drivers, should visconti_pcie_start_link()
in drivers/pci/controller/dwc/pcie-visconti.c also be addressed?

It currently uses readl_relaxed_poll_timeout() with the same 90000 and 100000
values:

visconti_pcie_start_link() {
    ...
    ret = readl_relaxed_poll_timeout(addr + PCIE_UL_REG_V_PHY_ST_02,
                                     val, (val & PCIE_UL_S_L0),
                                     90000, 100000);
}

Because the delay interval is 90ms and the total timeout is 100ms, doesn't
this allow at most two polling attempts before timing out? Could this break
enumeration for devices that take longer than 100ms to train their link?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-tegra264-pcie-v8-0-23e51589229b@nvidia.com?part=3