Re: [PATCH v3 3/7] PCI: cadence: HPA: Add post-link delay

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing device tree property silently disables the mandatory 100 ms PCIe link training delay.
- [High] The function `cdns_pcie_hpa_host_link_setup()` aborts the entire PCIe root complex probe if no downstream endpoint is connected.
--

commit 53fefd8ead01e523abe2d99bb9e1db2677c7f7ec
Author: Hans Zhang <18255117159@163.com>

PCI: cadence: HPA: Add post-link delay

This commit updates the Cadence HPA link setup function to include a 
100 ms delay after link training completes for speeds greater than 
5.0 GT/s. It also reads the max-link-speed device tree property to 
initialize the max link speed if it hasn't been set by a glue driver.

> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
> index 0f540bed58e88..8ef58ed01daaa 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host-hpa.c
[ ... ]
> @@ -304,6 +306,8 @@ int cdns_pcie_hpa_host_link_setup(struct cdns_pcie_rc *rc)
>  	ret = cdns_pcie_host_wait_for_link(pcie, cdns_pcie_hpa_link_up);
>  	if (ret)
>  		dev_dbg(dev, "PCIe link never came up\n");
> +	else
> +		pci_host_common_link_train_delay(pcie->max_link_speed);
>  
>  	return ret;

This isn't a regression introduced by this patch, but since we are modifying 
this function, does returning ret here abort the entire PCIe root complex 
probe if no downstream endpoint is connected?

When no endpoint is connected, cdns_pcie_host_wait_for_link() returns 
-ETIMEDOUT. Returning that error back to the caller causes 
cdns_pcie_hpa_host_setup() to fail initialization. 

Could this break hotplug functionality or standard empty-slot handling 
when the slot is unoccupied at boot?

>  }
> @@ -313,6 +317,7 @@ int cdns_pcie_hpa_host_setup(struct cdns_pcie_rc *rc)
[ ... ]
> @@ -343,6 +348,9 @@ int cdns_pcie_hpa_host_setup(struct cdns_pcie_rc *rc)
>  		rc->cfg_res = res;
>  	}
>  
> +	if (pcie->max_link_speed < 1)
> +		pcie->max_link_speed = of_pci_get_max_link_speed(np);

If the max-link-speed property is missing from the device tree, 
of_pci_get_max_link_speed() will return -EINVAL.

Will assigning this negative error code directly to pcie->max_link_speed 
cause the post-link delay check to fail?

When pci_host_common_link_train_delay() is called:

static inline void pci_host_common_link_train_delay(int max_link_speed)
{
        if (max_link_speed > 2)
                msleep(PCIE_RESET_CONFIG_WAIT_MS);
}

Since -EINVAL is a negative value, the condition max_link_speed > 2 
evaluates to false. This appears to silently bypass the required 100 ms 
delay for Gen3+ speeds entirely.

Should the return value of of_pci_get_max_link_speed() be validated 
before assignment?

> +
>  	/* Put EROM Bar aperture to 0 */
>  	cdns_pcie_hpa_writel(pcie, REG_BANK_IP_CFG_CTRL_REG, CDNS_PCIE_EROM, 0x0);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511055923.37117-1-18255117159@163.com?part=3