Re: [PATCH] PCI/sysfs: Ensure devices are powered for config reads

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Ethan, Andrey]

On Wed, Aug 20, 2025 at 10:26:08AM -0700, Brian Norris wrote:
> From: Brian Norris <briannorris@google.com>
> 
> max_link_speed, max_link_width, current_link_speed, current_link_width,
> secondary_bus_number, and subordinate_bus_number all access config
> registers, but they don't check the runtime PM state. If the device is
> in D3cold, we may see -EINVAL or even bogus values.
> 
> Wrap these access in pci_config_pm_runtime_{get,put}() like most of the
> rest of the similar sysfs attributes.

Protecting the config reads seems right to me.

If the device is in D3cold, a config read will result in a Completion
Timeout.  On most x86 platforms that's "fine" and merely results in ~0
data.  But that's merely convention, not a PCIe spec requirement.

I think it's a potential issue with PCIe controllers used on arm64 and
might result in an SError or synchronous abort from which we don't
recover well.  I'd love to hear actual experience about how reading
"current_link_speed" works on a device in D3cold in an arm64 system.

As Ethan and Andrey pointed out, we could skip max_link_speed_show()
because pcie_get_speed_cap() already uses a cached value and doesn't
do a config access.

max_link_width_show() is similar and also comes from PCI_EXP_LNKCAP
but is not currently cached, so I think we do need that one.  Worth a
comment to explain the non-obvious difference.

PCI_EXP_LNKCAP is ostensibly read-only and could conceivably be
cached, but the ASPM exit latencies can change based on the Common
Clock Configuration.

> Fixes: 56c1af4606f0 ("PCI: Add sysfs max_link_speed/width, current_link_speed/width, etc")
> Cc: stable@vger.kernel.org
> Signed-off-by: Brian Norris <briannorris@google.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> 
>  drivers/pci/pci-sysfs.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 5eea14c1f7f5..160df897dc5e 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -191,9 +191,16 @@ static ssize_t max_link_speed_show(struct device *dev,
>  				   struct device_attribute *attr, char *buf)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> +	ssize_t ret;
> +
> +	pci_config_pm_runtime_get(pdev);
>  
> -	return sysfs_emit(buf, "%s\n",
> -			  pci_speed_string(pcie_get_speed_cap(pdev)));
> +	ret = sysfs_emit(buf, "%s\n",
> +			 pci_speed_string(pcie_get_speed_cap(pdev)));
> +
> +	pci_config_pm_runtime_put(pdev);
> +
> +	return ret;
>  }
>  static DEVICE_ATTR_RO(max_link_speed);
>  
> @@ -201,8 +208,15 @@ static ssize_t max_link_width_show(struct device *dev,
>  				   struct device_attribute *attr, char *buf)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> +	ssize_t ret;
> +
> +	pci_config_pm_runtime_get(pdev);
> +
> +	ret = sysfs_emit(buf, "%u\n", pcie_get_width_cap(pdev));
>  
> -	return sysfs_emit(buf, "%u\n", pcie_get_width_cap(pdev));
> +	pci_config_pm_runtime_put(pdev);
> +
> +	return ret;
>  }
>  static DEVICE_ATTR_RO(max_link_width);
>  
> @@ -214,7 +228,10 @@ static ssize_t current_link_speed_show(struct device *dev,
>  	int err;
>  	enum pci_bus_speed speed;
>  
> +	pci_config_pm_runtime_get(pci_dev);
>  	err = pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA, &linkstat);
> +	pci_config_pm_runtime_put(pci_dev);
> +
>  	if (err)
>  		return -EINVAL;
>  
> @@ -231,7 +248,10 @@ static ssize_t current_link_width_show(struct device *dev,
>  	u16 linkstat;
>  	int err;
>  
> +	pci_config_pm_runtime_get(pci_dev);
>  	err = pcie_capability_read_word(pci_dev, PCI_EXP_LNKSTA, &linkstat);
> +	pci_config_pm_runtime_put(pci_dev);
> +
>  	if (err)
>  		return -EINVAL;
>  
> @@ -247,7 +267,10 @@ static ssize_t secondary_bus_number_show(struct device *dev,
>  	u8 sec_bus;
>  	int err;
>  
> +	pci_config_pm_runtime_get(pci_dev);
>  	err = pci_read_config_byte(pci_dev, PCI_SECONDARY_BUS, &sec_bus);
> +	pci_config_pm_runtime_put(pci_dev);
> +
>  	if (err)
>  		return -EINVAL;
>  
> @@ -263,7 +286,10 @@ static ssize_t subordinate_bus_number_show(struct device *dev,
>  	u8 sub_bus;
>  	int err;
>  
> +	pci_config_pm_runtime_get(pci_dev);
>  	err = pci_read_config_byte(pci_dev, PCI_SUBORDINATE_BUS, &sub_bus);
> +	pci_config_pm_runtime_put(pci_dev);
> +
>  	if (err)
>  		return -EINVAL;
>  
> -- 
> 2.51.0.rc1.193.gad69d77794-goog
>