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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Generic PCIe link training logic is incorrectly scoped to a framework-specific header, causing unnecessary coupling.
- [Medium] Including `../pci.h` inside a shared header file breaks subsystem encapsulation.
--

commit e409ebd1e8d25418f5cc0346099d7e22ffac79dd
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 if the link speed is greater than 5.0 GT/s, in accordance with
PCIe r6.0 sec 6.6.1. The helper is placed in pci-host-common.h to allow
multiple host controller drivers to share the logic.

> 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
> @@ -10,6 +10,9 @@
>  #ifndef _PCI_HOST_COMMON_H
>  #define _PCI_HOST_COMMON_H
>  
> +#include <linux/delay.h>
> +#include "../pci.h"

Does including ../pci.h inside a shared header file create a regression
in subsystem encapsulation? 

drivers/pci/pci.h is a private header meant exclusively for internal PCI
core subsystem definitions. Exposing it in this shared header could pollute
the namespace of any driver that includes it. Moving the helper directly
into drivers/pci/pci.h might resolve this regression without requiring
additional includes.

[ ... ]

> @@ -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)

Since the 100 ms delay requirement for downstream ports supporting > 5.0 GT/s
is a universal PCIe specification rule (PCIe r6.0 sec 6.6.1), does placing
this helper in the ECAM-specific pci-host-common.h introduce an
architectural regression?

Placing it here forces non-ECAM host controllers to include an unrelated
ECAM header just to use it. Could this be named generically and placed in
a central location like drivers/pci/pci.h instead?

> +{
> +	if (max_link_speed > 2)
> +		msleep(PCIE_RESET_CONFIG_WAIT_MS);
> +}
> +
>  #endif

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