Re: [PATCH] PCI/sysfs: document the link speed and width attributes

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The added documentation incorrectly describes `max_link_speed` and `max_link_width` as the negotiated link ceiling (the minimum of what both ends support), which contradicts the underlying implementation. The `max_link_speed_show()` and `max_link_width_show()` sysfs callbacks return the device's own capabilities, not the negotiated link ceiling. The documentation for `max_link_speed` contradicts its own first sentence by subsequently acknowledging the endpoint will report its own higher speed. Furthermore, the `current_link_speed` documentation incorrectly advises users to "use max_link_speed if what they want is the ceiling" of what the link will actually deliver, which can lead user-space tools to overestimate the link capacity if the upstream port is slower. Lastly, the claim that the speed is "Read from the Max Link Speed field of the device's Link Capabilities register" is outdated; since PCIe r3.0, the kernel determines this primarily via the Supported Link Speeds Vector in the Link Capabilities 2 Register (PCI_EXP_LNKCAP2).
--

--- Patch [1]: [PATCH] PCI/sysfs: document the link speed and width attributes ---
commit 24d4206c0d1e19c29d8e3f471fd2f8be01ffaa11
Author: Ferran Duarri <[email protected]>

PCI/sysfs: document the link speed and width attributes

This commit documents the max_link_speed, max_link_width, current_link_speed,
and current_link_width sysfs attributes. It clarifies that current link
attributes represent instantaneous values and should be sampled under load.

> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index b767db2c52cb7..ad18bd634ee6a 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -174,6 +174,71 @@ Description:
>  		similiar to writing 1 to their individual "reset" file, so use
>  		with caution.
>  
> +What:		/sys/bus/pci/devices/.../max_link_speed
> +Date:		September 2018
> +Contact:	[email protected]
> +Description:
> +		The maximum link speed this device's link can operate at, as a
> +		human-readable string such as "16.0 GT/s PCIe". Read from the
> +		Max Link Speed field of the device's Link Capabilities register.

[Severity: Medium]
Is the claim that this is read from the Max Link Speed field of the Link
Capabilities register fully accurate?

Since PCIe r3.0, the kernel primarily determines this capability via the
Supported Link Speeds Vector in the Link Capabilities 2 Register
(PCI_EXP_LNKCAP2).

> +
> +		This is the ceiling the link may negotiate, which is the lower
> +		of what the two ends of the link support. An endpoint capable of
> +		a higher speed than the port above it will report that higher
> +		speed here while the port above it reports the lower one, and
> +		the link will train at the lower of the two. To reason about a
> +		link, read this attribute on both ends.

[Severity: Medium]
Does this description contradict itself and the sysfs implementation?

The documentation states this is the "ceiling the link may negotiate, which is
the lower of what the two ends of the link support", but then acknowledges the
endpoint will report its own higher capability. 

Because the max_link_speed_show() and max_link_width_show() sysfs callbacks
return the device's own capabilities rather than the negotiated link ceiling,
calling it the "ceiling the link may negotiate" could cause user-space tools
to overestimate link capacities.

[ ... ]

> +What:		/sys/bus/pci/devices/.../current_link_speed
> +Date:		September 2018
> +Contact:	[email protected]
> +Description:
> +		The speed the link is operating at right now, as a
> +		human-readable string such as "16.0 GT/s PCIe". Read fresh from
> +		the device's Link Status register on every read of this file;
> +		nothing is cached.
> +
> +		This value is instantaneous and may change at any time. A link
> +		is permitted to retrain to a lower speed and back, and devices
> +		with aggressive link power management (GPUs in particular) do so
> +		routinely while idle. Two reads seconds apart, with no
> +		configuration change in between, can legitimately differ by
> +		several generations.
> +
> +		Consequently, comparing this attribute against max_link_speed is
> +		not by itself a test for a degraded link: an idle device will
> +		frequently report a lower speed and is working correctly.
> +		Callers that need a figure representing what the link will
> +		actually deliver should sample while the device is under load,
> +		or use max_link_speed if what they want is the ceiling.

[Severity: Medium]
Is it correct to advise users to use max_link_speed if they want the link's
functional ceiling?

If the upstream port is slower than the device, relying on the device's
max_link_speed will overestimate what the link will actually deliver. This
could lead user-space tools to incorrectly evaluate the available capacity.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1