Re: Patch "PCI/sysfs: Ensure devices are powered for config reads" has been added to the 6.6-stable tree

Re: Patch "PCI/sysfs: Ensure devices are powered for config reads" has been added to the 6.6-stable tree

[Brian Norris]

Hi,

On Thu, Oct 16, 2025 at 03:09:27PM +0200, Greg Kroah-Hartman wrote:
> 
> This is a note to let you know that I've just added the patch titled
> 
>     PCI/sysfs: Ensure devices are powered for config reads
> 
> to the 6.6-stable tree which can be found at:
>     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> 
> The filename of the patch is:
>      pci-sysfs-ensure-devices-are-powered-for-config-reads.patch
> and it can be found in the queue-6.6 subdirectory.
> 
> If you, or anyone else, feels it should not be added to the stable tree,
> please let <stable@vger.kernel.org> know about it.

Adding to the stable tree is good IMO, but one note about exactly how to
do so below:

> Wrap these access in pci_config_pm_runtime_{get,put}() like most of the
> rest of the similar sysfs attributes.
> 
> Notably, "max_link_speed" does not access config registers; it returns a
> cached value since d2bd39c0456b ("PCI: Store all PCIe Supported Link
> Speeds").

^^ This note about commit d2bd39c0456b was specifically to provide hints
about backporting. Without commit d2bd39c0456b, the solution is somewhat
incomplete. We should either backport commit d2bd39c0456b as well, or we
should adapt the change to add pci_config_pm_runtime_{get,put}() in
max_link_speed_show() too.

Commit d2bd39c0456b was already ported to 6.12.y, but seemingly no
further.

If adapting this change to pre-commit-d2bd39c0456b is better, I can
submit an updated version here.

Without commit d2bd39c0456b, it just means that the 'max_link_speed'
sysfs attribute is still susceptible to accessing a powered-down
device/link. We're in no worse state than we were without this patch.
And frankly, people are not likely to notice if they haven't already,
since I'd guess most systems don't suspend devices this aggressively.

Brian

> Fixes: 56c1af4606f0 ("PCI: Add sysfs max_link_speed/width, current_link_speed/width, etc")
> Signed-off-by: Brian Norris <briannorris@google.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Cc: stable@vger.kernel.org
> Link: https://patch.msgid.link/20250924095711.v2.1.Ibb5b6ca1e2c059e04ec53140cd98a44f2684c668@changeid
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/pci/pci-sysfs.c |   20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -196,8 +196,14 @@ static ssize_t max_link_width_show(struc
>  				   struct device_attribute *attr, char *buf)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> +	ssize_t ret;
>  
> -	return sysfs_emit(buf, "%u\n", pcie_get_width_cap(pdev));
> +	/* We read PCI_EXP_LNKCAP, so we need the device to be accessible. */
> +	pci_config_pm_runtime_get(pdev);
> +	ret = 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);
>  
> @@ -209,7 +215,10 @@ static ssize_t current_link_speed_show(s
>  	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;
>  
> @@ -226,7 +235,10 @@ static ssize_t current_link_width_show(s
>  	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;
>  
> @@ -242,7 +254,10 @@ static ssize_t secondary_bus_number_show
>  	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;
>  
> @@ -258,7 +273,10 @@ static ssize_t subordinate_bus_number_sh
>  	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;
>  
> 
> 
> Patches currently in stable-queue which might be from briannorris@google.com are
> 
> queue-6.6/pci-sysfs-ensure-devices-are-powered-for-config-reads.patch

Re: Patch "PCI/sysfs: Ensure devices are powered for config reads" has been added to the 6.6-stable tree

[Greg KH]

On Thu, Oct 16, 2025 at 08:15:46AM -0700, Brian Norris wrote:
> Hi,
> 
> On Thu, Oct 16, 2025 at 03:09:27PM +0200, Greg Kroah-Hartman wrote:
> > 
> > This is a note to let you know that I've just added the patch titled
> > 
> >     PCI/sysfs: Ensure devices are powered for config reads
> > 
> > to the 6.6-stable tree which can be found at:
> >     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> > 
> > The filename of the patch is:
> >      pci-sysfs-ensure-devices-are-powered-for-config-reads.patch
> > and it can be found in the queue-6.6 subdirectory.
> > 
> > If you, or anyone else, feels it should not be added to the stable tree,
> > please let <stable@vger.kernel.org> know about it.
> 
> Adding to the stable tree is good IMO, but one note about exactly how to
> do so below:
> 
> > Wrap these access in pci_config_pm_runtime_{get,put}() like most of the
> > rest of the similar sysfs attributes.
> > 
> > Notably, "max_link_speed" does not access config registers; it returns a
> > cached value since d2bd39c0456b ("PCI: Store all PCIe Supported Link
> > Speeds").
> 
> ^^ This note about commit d2bd39c0456b was specifically to provide hints
> about backporting. Without commit d2bd39c0456b, the solution is somewhat
> incomplete. We should either backport commit d2bd39c0456b as well, or we
> should adapt the change to add pci_config_pm_runtime_{get,put}() in
> max_link_speed_show() too.

I missed that "hint", you need to make it bindingly obvious as I churn
through the giant "-rc1 merge dump" very quickly as obviously those are
changes that were not serious enough to make it into -final :)

> Commit d2bd39c0456b was already ported to 6.12.y, but seemingly no
> further.
> 
> If adapting this change to pre-commit-d2bd39c0456b is better, I can
> submit an updated version here.
> 
> Without commit d2bd39c0456b, it just means that the 'max_link_speed'
> sysfs attribute is still susceptible to accessing a powered-down
> device/link. We're in no worse state than we were without this patch.
> And frankly, people are not likely to notice if they haven't already,
> since I'd guess most systems don't suspend devices this aggressively.

I'll gladly accept a fixed up patch for this, thanks.

greg k-h

Re: Patch "PCI/sysfs: Ensure devices are powered for config reads" has been added to the 6.6-stable tree

[Brian Norris]

Hi Greg,

On Fri, Oct 17, 2025 at 08:58:20AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Oct 16, 2025 at 08:15:46AM -0700, Brian Norris wrote:
> > Hi,
> > 
> > On Thu, Oct 16, 2025 at 03:09:27PM +0200, Greg Kroah-Hartman wrote:
> > > 
> > > This is a note to let you know that I've just added the patch titled
> > > 
> > >     PCI/sysfs: Ensure devices are powered for config reads
> > > 
> > > to the 6.6-stable tree which can be found at:
> > >     http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> > > 
> > > The filename of the patch is:
> > >      pci-sysfs-ensure-devices-are-powered-for-config-reads.patch
> > > and it can be found in the queue-6.6 subdirectory.
> > > 
> > > If you, or anyone else, feels it should not be added to the stable tree,
> > > please let <stable@vger.kernel.org> know about it.
> > 
> > Adding to the stable tree is good IMO, but one note about exactly how to
> > do so below:
> > 
> > > Wrap these access in pci_config_pm_runtime_{get,put}() like most of the
> > > rest of the similar sysfs attributes.
> > > 
> > > Notably, "max_link_speed" does not access config registers; it returns a
> > > cached value since d2bd39c0456b ("PCI: Store all PCIe Supported Link
> > > Speeds").
> > 
> > ^^ This note about commit d2bd39c0456b was specifically to provide hints
> > about backporting. Without commit d2bd39c0456b, the solution is somewhat
> > incomplete. We should either backport commit d2bd39c0456b as well, or we
> > should adapt the change to add pci_config_pm_runtime_{get,put}() in
> > max_link_speed_show() too.
> 
> I missed that "hint", you need to make it bindingly obvious as I churn
> through the giant "-rc1 merge dump" very quickly as obviously those are
> changes that were not serious enough to make it into -final :)

Oh, no, I didn't mean to imply you "missed" anything. It was more of a
self-help comment, so I could refer to it when following up here.
Otherwise, I also might not trivially remember which commit was
involved. And I didn't know at the time how many branches would contain
commit d2bd39c0456b.

Sorry if my wording was a bit off.

> > Commit d2bd39c0456b was already ported to 6.12.y, but seemingly no
> > further.
> > 
> > If adapting this change to pre-commit-d2bd39c0456b is better, I can
> > submit an updated version here.
> > 
> > Without commit d2bd39c0456b, it just means that the 'max_link_speed'
> > sysfs attribute is still susceptible to accessing a powered-down
> > device/link. We're in no worse state than we were without this patch.
> > And frankly, people are not likely to notice if they haven't already,
> > since I'd guess most systems don't suspend devices this aggressively.
> 
> I'll gladly accept a fixed up patch for this, thanks.

I'll try to get that out today.

Brian

Re: Patch "PCI/sysfs: Ensure devices are powered for config reads" has been added to the 6.6-stable tree

[Brian Norris]

On Fri, Oct 17, 2025 at 10:50:43AM -0700, Brian Norris wrote:
> On Fri, Oct 17, 2025 at 08:58:20AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Oct 16, 2025 at 08:15:46AM -0700, Brian Norris wrote:
> > > If adapting this change to pre-commit-d2bd39c0456b is better, I can
> > > submit an updated version here.
> > > 
> > > Without commit d2bd39c0456b, it just means that the 'max_link_speed'
> > > sysfs attribute is still susceptible to accessing a powered-down
> > > device/link. We're in no worse state than we were without this patch.
> > > And frankly, people are not likely to notice if they haven't already,
> > > since I'd guess most systems don't suspend devices this aggressively.
> > 
> > I'll gladly accept a fixed up patch for this, thanks.
> 
> I'll try to get that out today.

I didn't make time on Friday, and this patch was committed to 6.6.y
already. So I've submitted a "part 2" here:

Subject: [PATCH 6.6] PCI/sysfs: Ensure devices are powered for config reads (part 2)
https://lore.kernel.org/all/20251020204146.3193844-1-briannorris@chromium.org/

Brian