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

On 5/12/26 08:07, [email protected] wrote:
> 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 <[email protected]>
> 
> 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?

No, Mani suggested placing it in the pci-host-common.h file.

> 
> [ ... ]
> 
>> @@ -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
>