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.
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;
On 8/21/2025 1:26 AM, 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.
My understanding, if your device is in D3cold, returning of -EINVAL is the right behavior. >
Wrap these access in pci_config_pm_runtime_{get,put}() like most of the rest of the similar sysfs attributes.
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);
This function would potentially change the power state of device, that would be a complex process, beyond the meaning of max_link_speed_show(), given the semantics of these functions ( max_link_speed_show()/max_link_width_show()/current_link_speed_show()/ ....), this cannot be done !
Thanks, Ethan>
- 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;
On Thu, Aug 21, 2025 at 08:54:52AM +0800, Ethan Zhao wrote:
On 8/21/2025 1:26 AM, 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.
My understanding, if your device is in D3cold, returning of -EINVAL is the right behavior.
That's not the guaranteed result though. Some hosts don't properly return PCIBIOS_DEVICE_NOT_FOUND, for one. But also, it's racy -- because we don't even try to hold a pm_runtime reference, the device could possibly enter D3cold while we're in the middle of reading from it. If you're lucky, that'll get you a completion timeout and an all-1's result, and we'll return a garbage result.
So if we want to purposely not resume the device and retain "I can't give you what you asked for" behavior, we'd at least need a pm_runtime_get_noresume() or similar.
Wrap these access in pci_config_pm_runtime_{get,put}() like most of the rest of the similar sysfs attributes.
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);
This function would potentially change the power state of device, that would be a complex process, beyond the meaning of max_link_speed_show(), given the semantics of these functions ( max_link_speed_show()/max_link_width_show()/current_link_speed_show()/ ....), this cannot be done !
What makes this different than the 'config' attribute (i.e., "read config register")? Why shouldn't that just return -EINVAL? I don't really buy your reasoning -- "it's a complex process" is not a reason not to do something. The user asked for the link speed; why not give it? If the user wanted to know if the device was powered, they could check the 'power_state' attribute instead.
(Side note: these attributes don't show up anywhere in Documentation/, so it's also a bit hard to declare "best" semantics for them.)
To flip this question around a bit: if I have a system that aggressively suspends devices when there's no recent activity, how am I supposed to check what the link speed is? Probabilistically hammer the file while hoping some other activity wakes the device, so I can find the small windows of time where it's RPM_ACTIVE? Disable runtime_pm for the device while I check?
Brian
On 8/21/2025 10:56 AM, Brian Norris wrote:
On Thu, Aug 21, 2025 at 08:54:52AM +0800, Ethan Zhao wrote:
On 8/21/2025 1:26 AM, 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.
My understanding, if your device is in D3cold, returning of -EINVAL is the right behavior.
That's not the guaranteed result though. Some hosts don't properly return PCIBIOS_DEVICE_NOT_FOUND, for one. But also, it's racy -- because we don't even try to hold a pm_runtime reference, the device could possibly enter D3cold while we're in the middle of reading from it. If you're lucky, that'll get you a completion timeout and an all-1's result, and we'll return a garbage result.
So if we want to purposely not resume the device and retain "I can't give you what you asked for" behavior, we'd at least need a pm_runtime_get_noresume() or similar.
I understand you just want the stable result of these caps, meanwhile you don't want the side effect either.>
Wrap these access in pci_config_pm_runtime_{get,put}() like most of the rest of the similar sysfs attributes.
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);
This function would potentially change the power state of device, that would be a complex process, beyond the meaning of max_link_speed_show(), given the semantics of these functions ( max_link_speed_show()/max_link_width_show()/current_link_speed_show()/ ....), this cannot be done !
What makes this different than the 'config' attribute (i.e., "read config register")? Why shouldn't that just return -EINVAL? I don't really buy your reasoning -- "it's a complex process" is not a reason
It is a reason to know there is side effect to be taken into account.> not to do something. The user asked for the link speed; why not give it?
If the user wanted to know if the device was powered, they could check the 'power_state' attribute instead.
(Side note: these attributes don't show up anywhere in Documentation/, so it's also a bit hard to declare "best" semantics for them.)
To flip this question around a bit: if I have a system that aggressively suspends devices when there's no recent activity, how am I supposed to check what the link speed is? Probabilistically hammer the file while hoping some other activity wakes the device, so I can find the small windows of time where it's RPM_ACTIVE? Disable runtime_pm for the device while I check?
Hold a PM reference by pci_config_pm_runtime_get() and then write some data to the PCIe config space, no objection.
To know about the linkspeed etc capabilities/not status, how about creating a cached version of these caps, no need to change their power state.
If there is aggressive power saving requirement, and the polling of these caps will make up wakeup/poweron bugs.
Thanks, Ethan
Brian
Hi Ethan,
Note: I'm having a hard time reading your emails sometimes, because you aren't really adding in appropriate newlines that separate your reply from quoted text. So your own sentences just run together with parts of my sentences at times. I've tried to resolve this as best I can.
On Thu, Aug 21, 2025 at 08:41:28PM +0800, Ethan Zhao wrote:
On 8/21/2025 10:56 AM, Brian Norris wrote:
On Thu, Aug 21, 2025 at 08:54:52AM +0800, Ethan Zhao wrote:
On 8/21/2025 1:26 AM, 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.
My understanding, if your device is in D3cold, returning of -EINVAL is the right behavior.
That's not the guaranteed result though. Some hosts don't properly return PCIBIOS_DEVICE_NOT_FOUND, for one. But also, it's racy -- because we don't even try to hold a pm_runtime reference, the device could possibly enter D3cold while we're in the middle of reading from it. If you're lucky, that'll get you a completion timeout and an all-1's result, and we'll return a garbage result.
So if we want to purposely not resume the device and retain "I can't give you what you asked for" behavior, we'd at least need a pm_runtime_get_noresume() or similar.
I understand you just want the stable result of these caps,
Yes, I'd like a valid result, not EINVAL. Why would I check this file if I didn't want the result?
meanwhile you don't want the side effect either.
Personally, I think side effect is completely fine. Or, it's just as fine as it is for the 'config' attribute or for 'resource_N_size' attributes that already do the same.
Wrap these access in pci_config_pm_runtime_{get,put}() like most of the rest of the similar sysfs attributes.
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);
This function would potentially change the power state of device, that would be a complex process, beyond the meaning of max_link_speed_show(), given the semantics of these functions ( max_link_speed_show()/max_link_width_show()/current_link_speed_show()/ ....), this cannot be done !
What makes this different than the 'config' attribute (i.e., "read config register")? Why shouldn't that just return -EINVAL? I don't really buy your reasoning -- "it's a complex process" is not a reason
It is a reason to know there is side effect to be taken into account.
OK, agreed, there's a side effect. I don't think you've convinced me the side effect is bad though.
not to do something. The user asked for the link speed; why not give it? If the user wanted to know if the device was powered, they could check the 'power_state' attribute instead.
(Side note: these attributes don't show up anywhere in Documentation/, so it's also a bit hard to declare "best" semantics for them.)
To flip this question around a bit: if I have a system that aggressively suspends devices when there's no recent activity, how am I supposed to check what the link speed is? Probabilistically hammer the file while hoping some other activity wakes the device, so I can find the small windows of time where it's RPM_ACTIVE? Disable runtime_pm for the device while I check?
Hold a PM reference by pci_config_pm_runtime_get() and then write some data to the PCIe config space, no objection.
To know about the linkspeed etc capabilities/not status, how about creating a cached version of these caps, no need to change their power state.
For static values like the "max" attributes, maybe that's fine.
But Linux is not always the one changing the link speed. I've seen PCI devices that autonomously request link-speed changes, and AFAICT, the only way we'd know in host software is to go reread the config registers. So caching just produces cache invalidation problems.
If there is aggressive power saving requirement, and the polling of these caps will make up wakeup/poweron bugs.
If you're worried about wakeup frequency, I think that's a matter of user space / system administraction to decide -- if it doesn't want to potentially wake up the link, it shouldn't be poking at config-based sysfs attributes.
Brian
On 8/21/2025 11:28 PM, Brian Norris wrote:
Hi Ethan,
Note: I'm having a hard time reading your emails sometimes, because you aren't really adding in appropriate newlines that separate your reply from quoted text. So your own sentences just run together with parts of my sentences at times. I've tried to resolve this as best I can.
On Thu, Aug 21, 2025 at 08:41:28PM +0800, Ethan Zhao wrote:
On 8/21/2025 10:56 AM, Brian Norris wrote:
On Thu, Aug 21, 2025 at 08:54:52AM +0800, Ethan Zhao wrote:
On 8/21/2025 1:26 AM, 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.
My understanding, if your device is in D3cold, returning of -EINVAL is the right behavior.
That's not the guaranteed result though. Some hosts don't properly return PCIBIOS_DEVICE_NOT_FOUND, for one. But also, it's racy -- because we don't even try to hold a pm_runtime reference, the device could possibly enter D3cold while we're in the middle of reading from it. If you're lucky, that'll get you a completion timeout and an all-1's result, and we'll return a garbage result.
So if we want to purposely not resume the device and retain "I can't give you what you asked for" behavior, we'd at least need a pm_runtime_get_noresume() or similar.
I understand you just want the stable result of these caps,
Yes, I'd like a valid result, not EINVAL. Why would I check this file if I didn't want the result?
meanwhile you don't want the side effect either.
Personally, I think side effect is completely fine. Or, it's just as fine as it is for the 'config' attribute or for 'resource_N_size' attributes that already do the same.
Wrap these access in pci_config_pm_runtime_{get,put}() like most of the rest of the similar sysfs attributes.
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);
This function would potentially change the power state of device, that would be a complex process, beyond the meaning of max_link_speed_show(), given the semantics of these functions ( max_link_speed_show()/max_link_width_show()/current_link_speed_show()/ ....), this cannot be done !
What makes this different than the 'config' attribute (i.e., "read config register")? Why shouldn't that just return -EINVAL? I don't really buy your reasoning -- "it's a complex process" is not a reason
It is a reason to know there is side effect to be taken into account.
OK, agreed, there's a side effect. I don't think you've convinced me the side effect is bad though.
not to do something. The user asked for the link speed; why not give it? If the user wanted to know if the device was powered, they could check the 'power_state' attribute instead.
(Side note: these attributes don't show up anywhere in Documentation/, so it's also a bit hard to declare "best" semantics for them.)
To flip this question around a bit: if I have a system that aggressively suspends devices when there's no recent activity, how am I supposed to check what the link speed is? Probabilistically hammer the file while hoping some other activity wakes the device, so I can find the small windows of time where it's RPM_ACTIVE? Disable runtime_pm for the device while I check?
Hold a PM reference by pci_config_pm_runtime_get() and then write some data to the PCIe config space, no objection.
To know about the linkspeed etc capabilities/not status, how about creating a cached version of these caps, no need to change their power state.
For static values like the "max" attributes, maybe that's fine.
But Linux is not always the one changing the link speed. I've seen PCI devices that autonomously request link-speed changes, and AFAICT, the only way we'd know in host software is to go reread the config registers. So caching just produces cache invalidation problems.
Maybe you meant the link-speed status, that would be volatile based on link retraining. Here we are talking about some non-volatile capabilities value no invalidation needed to their cached variables.>
If there is aggressive power saving requirement, and the polling of these caps will make up wakeup/poweron bugs.
If you're worried about wakeup frequency, I think that's a matter of user space / system administraction to decide -- if it doesn't want to potentially wake up the link, it shouldn't be poking at config-based
IMHO, sysfs interface is part of KABI, you change its behavior , you definitely would break some running binaries. there is alternative way to avoid re-cooking binaries or waking up administrator to modify their configuration/script in the deep night. you already got it.
Thanks, Ethan > sysfs attributes.
Brian
linux-stable-mirror@lists.linaro.org
-
Brian Norris -
Ethan Zhao