Re: [PATCH v3 1/7] PCI: Add pci_host_common_link_train_delay() helper

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Negative error codes passed as `max_link_speed` silently bypass the mandatory PCIe delay.
- [Medium] Architectural modularity violation by placing a generic PCIe helper in an ECAM framework header.
- [Low] The helper API relies on raw integer values for link speed instead of the typed `enum pci_bus_speed`, creating a brittle interface.
--

commit 47720e87ef05a9f0115d64b2e2d6eaa190242b88
Author: Hans Zhang <18255117159@163.com>

PCI: Add pci_host_common_link_train_delay() helper

This commit introduces a static inline helper to wait 100 ms after link
training completes if the maximum link speed is greater than 5.0 GT/s,
satisfying the PCIe r6.0 specification.

> diff --git a/drivers/pci/controller/pci-host-common.h b/drivers/pci/controller/pci-host-common.h
> index b5075d4bd7eb3..d709f7e3e11a3 100644
> --- a/drivers/pci/controller/pci-host-common.h
> +++ b/drivers/pci/controller/pci-host-common.h

This isn't a bug, but this places a generic PCIe helper inside an ECAM
framework header. Unrelated host controller drivers will have to include
an ECAM-specific header just to use this delay function.

Should this generic helper be placed in a more widely accessible internal
header like drivers/pci/pci.h?

[ ... ]

> @@ -20,4 +23,18 @@ void pci_host_common_remove(struct platform_device *pdev);
>  
>  struct pci_config_window *pci_host_common_ecam_create(struct device *dev,
>  	struct pci_host_bridge *bridge, const struct pci_ecam_ops *ops);
> +
> +/**
> + * pci_host_common_link_train_delay - Wait 100 ms if link speed > 5 GT/s
> + * @max_link_speed: the maximum link speed (2 = 5.0 GT/s, 3 = 8.0 GT/s, ...)
> + *
> + * Must be called after Link training completes and before the first
> + * Configuration Request is sent.
> + */
> +static inline void pci_host_common_link_train_delay(int max_link_speed)

This isn't a bug, but relying on raw integer values for link speed instead
of enum pci_bus_speed creates a brittle interface.

The PCI subsystem uses enum pci_bus_speed for link speeds, where
PCIE_SPEED_2_5GT is 20 and PCIE_SPEED_5_0GT is 21. If a driver mistakenly
passes an enum pci_bus_speed, the condition greater than 2 will evaluate
to true for all speeds, applying the delay unconditionally.

> +{
> +	if (max_link_speed > 2)

Will negative error codes passed as max_link_speed silently bypass the
mandatory PCIe delay?

Drivers retrieve the maximum link speed using of_pci_get_max_link_speed(),
which returns -EINVAL if the max-link-speed property is missing from the
device tree.

If -EINVAL is passed directly to this helper, the condition evaluates to
false and the 100 ms delay is skipped. If the hardware supports greater
than 5.0 GT/s, skipping this delay violates the PCIe r6.0 specification.

> +		msleep(PCIE_RESET_CONFIG_WAIT_MS);
> +}
> +
>  #endif

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