From: Bjorn Helgaas <bhelgaas@google.com>
Many of the save/restore functions in the pci_save_state() and
pci_restore_state() paths depend on both a PCI capability of the device and
a pci_cap_saved_state structure to hold the configuration data, and they
skip the operation if either is missing.
Look for the pci_cap_saved_state first so if we don't have one, we can skip
searching for the device capability, which requires several slow config
space accesses.
Remove some error messages if the pci_cap_saved_state is not found so we
don't complain about having no saved state for a capability the device
doesn't have. We have already warned in pci_allocate_cap_save_buffers() if
the capability is present but we were unable to allocate a buffer.
Other than the message change, no functional change intended.
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
drivers/pci/pci.c | 27 ++++++++++++++-------------
drivers/pci/pcie/aspm.c | 15 ++++++++-------
2 files changed, 22 insertions(+), 20 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 869d204a70a3..503376bf7e75 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1686,10 +1686,8 @@ static int pci_save_pcie_state(struct pci_dev *dev)
return 0;
save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
- if (!save_state) {
- pci_err(dev, "buffer not found in %s\n", __func__);
+ if (!save_state)
return -ENOMEM;
- }
cap = (u16 *)&save_state->cap.data[0];
pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &cap[i++]);
@@ -1742,19 +1740,17 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
static int pci_save_pcix_state(struct pci_dev *dev)
{
- int pos;
struct pci_cap_saved_state *save_state;
+ u8 pos;
+
+ save_state = pci_find_saved_cap(dev, PCI_CAP_ID_PCIX);
+ if (!save_state)
+ return -ENOMEM;
pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
if (!pos)
return 0;
- save_state = pci_find_saved_cap(dev, PCI_CAP_ID_PCIX);
- if (!save_state) {
- pci_err(dev, "buffer not found in %s\n", __func__);
- return -ENOMEM;
- }
-
pci_read_config_word(dev, pos + PCI_X_CMD,
(u16 *)save_state->cap.data);
@@ -1763,14 +1759,19 @@ static int pci_save_pcix_state(struct pci_dev *dev)
static void pci_restore_pcix_state(struct pci_dev *dev)
{
- int i = 0, pos;
struct pci_cap_saved_state *save_state;
+ u8 pos;
+ int i = 0;
u16 *cap;
save_state = pci_find_saved_cap(dev, PCI_CAP_ID_PCIX);
- pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
- if (!save_state || !pos)
+ if (!save_state)
return;
+
+ pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
+ if (!pos)
+ return;
+
cap = (u16 *)&save_state->cap.data[0];
pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index e0bc90597dca..007e4a082e6f 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -35,16 +35,14 @@ void pci_save_ltr_state(struct pci_dev *dev)
if (!pci_is_pcie(dev))
return;
+ save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
+ if (!save_state)
+ return;
+
ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
if (!ltr)
return;
- save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
- if (!save_state) {
- pci_err(dev, "no suspend buffer for LTR; ASPM issues possible after resume\n");
- return;
- }
-
/* Some broken devices only support dword access to LTR */
cap = &save_state->cap.data[0];
pci_read_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap);
@@ -57,8 +55,11 @@ void pci_restore_ltr_state(struct pci_dev *dev)
u32 *cap;
save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
+ if (!save_state)
+ return;
+
ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
- if (!save_state || !ltr)
+ if (!ltr)
return;
/* Some broken devices only support dword access to LTR */
--
2.34.1
Hello,
kernel test robot noticed "last_state.booting" on:
commit: c8a9382e172ac80bc96820b3ec758e35cdc05c06 ("[PATCH v2 1/2] PCI: Avoid pointless capability searches")
url: https://github.com/intel-lab-lkp/linux/commits/Bjorn-Helgaas/PCI-Avoid-pointless-capability-searches/20250215-080525
base: https://git.kernel.org/cgit/linux/kernel/git/pci/pci.git next
patch link: https://lore.kernel.org/all/20250215000301.175097-2-helgaas@kernel.org/
patch subject: [PATCH v2 1/2] PCI: Avoid pointless capability searches
in testcase: xfstests
version: xfstests-x86_64-8467552f-1_20241215
with following parameters:
disk: 4HDD
fs: ext2
test: generic-holetest
config: x86_64-rhel-9.4-func
compiler: gcc-12
test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz (Skylake) with 32G memory
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202503042124.7627f722-lkp@intel.com
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20250304/202503042124.7627f722-lkp@intel.com
[ 62.784123][ T284] LKP: waiting for network...
[ 62.784128][ T284]
[ 63.218408][ T284] ls /sys/class/net
[ 63.218417][ T284]
[ 63.224484][ T284] lo
[ 63.224490][ T284]
[ 64.076175][ T1] watchdog: watchdog0: watchdog did not stop!
[ 64.157917][ T1] watchdog: watchdog0: watchdog did not stop!
[ 64.196710][ T1] sd 1:0:0:0: [sdb] Synchronizing SCSI cache
[ 64.202659][ T1] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[ 64.969888][ T1] pcieport 0000:00:01.0: VC0 negotiation stuck pending
[ 64.977342][ T1] ACPI: PM: Preparing to enter system sleep state S5
[ 64.986446][ T1] kvm: exiting hardware virtualization
reboot: Restarting system
there is no more useful information, seems our test machine just reboot here.
this is not observed on the parent commit, c71f7bbc5d794, which chosen as the
base by bot.
* a234e07a63859 (linux-review/Bjorn-Helgaas/PCI-Avoid-pointless-capability-searches/20250215-080525) PCI: Cache offset of Resizable BAR capability
* c8a9382e172ac PCI: Avoid pointless capability searches
* c71f7bbc5d794 Merge branch 'pci/endpoint'
c71f7bbc5d794984 c8a9382e172ac80bc96820b3ec7
---------------- ---------------------------
fail:runs %reproduction fail:runs
| | |
:6 100% 6:6 last_state.booting
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Tue, Mar 04, 2025 at 10:12:48PM +0800, kernel test robot wrote:
> kernel test robot noticed "last_state.booting" on:
>
> commit: c8a9382e172ac80bc96820b3ec758e35cdc05c06 ("[PATCH v2 1/2] PCI: Avoid pointless capability searches")
> url: https://github.com/intel-lab-lkp/linux/commits/Bjorn-Helgaas/PCI-Avoid-pointless-capability-searches/20250215-080525
> base: https://git.kernel.org/cgit/linux/kernel/git/pci/pci.git next
> patch link: https://lore.kernel.org/all/20250215000301.175097-2-helgaas@kernel.org/
> patch subject: [PATCH v2 1/2] PCI: Avoid pointless capability searches
This patch was dropped in next-20250224.
> in testcase: xfstests
> version: xfstests-x86_64-8467552f-1_20241215
> with following parameters:
>
> disk: 4HDD
> fs: ext2
> test: generic-holetest
>
>
>
> config: x86_64-rhel-9.4-func
> compiler: gcc-12
> test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz (Skylake) with 32G memory
>
> (please refer to attached dmesg/kmsg for entire log/backtrace)
>
>
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <oliver.sang@intel.com>
> | Closes: https://lore.kernel.org/oe-lkp/202503042124.7627f722-lkp@intel.com
>
>
> The kernel config and materials to reproduce are available at:
> https://download.01.org/0day-ci/archive/20250304/202503042124.7627f722-lkp@intel.com
>
>
>
> [ 62.784123][ T284] LKP: waiting for network...
> [ 62.784128][ T284]
> [ 63.218408][ T284] ls /sys/class/net
> [ 63.218417][ T284]
> [ 63.224484][ T284] lo
> [ 63.224490][ T284]
> [ 64.076175][ T1] watchdog: watchdog0: watchdog did not stop!
> [ 64.157917][ T1] watchdog: watchdog0: watchdog did not stop!
> [ 64.196710][ T1] sd 1:0:0:0: [sdb] Synchronizing SCSI cache
> [ 64.202659][ T1] sd 0:0:0:0: [sda] Synchronizing SCSI cache
> [ 64.969888][ T1] pcieport 0000:00:01.0: VC0 negotiation stuck pending
> [ 64.977342][ T1] ACPI: PM: Preparing to enter system sleep state S5
> [ 64.986446][ T1] kvm: exiting hardware virtualization
> reboot: Restarting system
>
>
> there is no more useful information, seems our test machine just reboot here.
> this is not observed on the parent commit, c71f7bbc5d794, which chosen as the
> base by bot.
>
> * a234e07a63859 (linux-review/Bjorn-Helgaas/PCI-Avoid-pointless-capability-searches/20250215-080525) PCI: Cache offset of Resizable BAR capability
> * c8a9382e172ac PCI: Avoid pointless capability searches
> * c71f7bbc5d794 Merge branch 'pci/endpoint'
>
> c71f7bbc5d794984 c8a9382e172ac80bc96820b3ec7
> ---------------- ---------------------------
> fail:runs %reproduction fail:runs
> | | |
> :6 100% 6:6 last_state.booting
>
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Many of the save/restore functions in the pci_save_state() and
> pci_restore_state() paths depend on both a PCI capability of the device and
> a pci_cap_saved_state structure to hold the configuration data, and they
> skip the operation if either is missing.
>
> Look for the pci_cap_saved_state first so if we don't have one, we can skip
> searching for the device capability, which requires several slow config
> space accesses.
>
> Remove some error messages if the pci_cap_saved_state is not found so we
> don't complain about having no saved state for a capability the device
> doesn't have. We have already warned in pci_allocate_cap_save_buffers() if
> the capability is present but we were unable to allocate a buffer.
>
> Other than the message change, no functional change intended.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> drivers/pci/pci.c | 27 ++++++++++++++-------------
> drivers/pci/pcie/aspm.c | 15 ++++++++-------
> 2 files changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a70a3..503376bf7e75 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1686,10 +1686,8 @@ static int pci_save_pcie_state(struct pci_dev *dev)
> return 0;
>
> save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
> - if (!save_state) {
> - pci_err(dev, "buffer not found in %s\n", __func__);
> + if (!save_state)
> return -ENOMEM;
> - }
>
> cap = (u16 *)&save_state->cap.data[0];
> pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &cap[i++]);
> @@ -1742,19 +1740,17 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
>
> static int pci_save_pcix_state(struct pci_dev *dev)
> {
> - int pos;
> struct pci_cap_saved_state *save_state;
> + u8 pos;
> +
> + save_state = pci_find_saved_cap(dev, PCI_CAP_ID_PCIX);
> + if (!save_state)
> + return -ENOMEM;
>
> pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> if (!pos)
> return 0;
>
> - save_state = pci_find_saved_cap(dev, PCI_CAP_ID_PCIX);
> - if (!save_state) {
> - pci_err(dev, "buffer not found in %s\n", __func__);
> - return -ENOMEM;
> - }
When devices don't have PCI_CAP_ID_PCIX, this change in order appears to
cause a functional change.
Since probe functions of some drivers (e.g. Intel e1000) rely on the return
value, I think they fail after that change in this situation.
Actually in my QEMU VM, e1000 driver failed to probe the device due to
-ENOMEM from pci_save_pcix_state().
```
[root@localhost ~]# dmesg | grep e1000
[ 0.400303] [ T1] e1000: Intel(R) PRO/1000 Network Driver
[ 0.400805] [ T1] e1000: Copyright (c) 1999-2006 Intel Corporation.
[ 0.710970] [ T1] e1000 0000:00:03.0: probe with driver e1000 failed with error -12
[root@localhost ~]# lspci -nnvs 00:03.0
00:03.0 Ethernet controller [0200]: Intel Corporation 82540EM Gigabit Ethernet Controller [8086:100e] (rev 03)
Subsystem: Red Hat, Inc. QEMU Virtual Machine [1af4:1100]
Flags: fast devsel, IRQ 11
Memory at febc0000 (32-bit, non-prefetchable) [size=128K]
I/O ports at c000 [size=64]
Expansion ROM at feb80000 [disabled] [size=256K]
lspci: Unable to load libkmod resources: error -2
```
Regarding pci_save_vc_state(), I found that similar comments were provided in
this context:
https://lore.kernel.org/all/7dbb0d8b-3708-60ba-ee9e-78aa48bee160@linux.intel.com/
However, the same type of order change is still left in
pci_save_pcix_state().
> -
> pci_read_config_word(dev, pos + PCI_X_CMD,
> (u16 *)save_state->cap.data);
>
> @@ -1763,14 +1759,19 @@ static int pci_save_pcix_state(struct pci_dev *dev)
>
> static void pci_restore_pcix_state(struct pci_dev *dev)
> {
> - int i = 0, pos;
> struct pci_cap_saved_state *save_state;
> + u8 pos;
> + int i = 0;
> u16 *cap;
>
> save_state = pci_find_saved_cap(dev, PCI_CAP_ID_PCIX);
> - pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> - if (!save_state || !pos)
> + if (!save_state)
> return;
> +
> + pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> + if (!pos)
> + return;
> +
> cap = (u16 *)&save_state->cap.data[0];
>
> pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index e0bc90597dca..007e4a082e6f 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -35,16 +35,14 @@ void pci_save_ltr_state(struct pci_dev *dev)
> if (!pci_is_pcie(dev))
> return;
>
> + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
> + if (!save_state)
> + return;
> +
> ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
> if (!ltr)
> return;
>
> - save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
> - if (!save_state) {
> - pci_err(dev, "no suspend buffer for LTR; ASPM issues possible after resume\n");
> - return;
> - }
> -
> /* Some broken devices only support dword access to LTR */
> cap = &save_state->cap.data[0];
> pci_read_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap);
> @@ -57,8 +55,11 @@ void pci_restore_ltr_state(struct pci_dev *dev)
> u32 *cap;
>
> save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
> + if (!save_state)
> + return;
> +
> ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
> - if (!save_state || !ltr)
> + if (!ltr)
> return;
>
> /* Some broken devices only support dword access to LTR */
> --
> 2.34.1
Regards,
Kohei
On Fri, 14 Feb 2025, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Many of the save/restore functions in the pci_save_state() and
> pci_restore_state() paths depend on both a PCI capability of the device and
> a pci_cap_saved_state structure to hold the configuration data, and they
> skip the operation if either is missing.
>
> Look for the pci_cap_saved_state first so if we don't have one, we can skip
> searching for the device capability, which requires several slow config
> space accesses.
>
> Remove some error messages if the pci_cap_saved_state is not found so we
> don't complain about having no saved state for a capability the device
> doesn't have. We have already warned in pci_allocate_cap_save_buffers() if
> the capability is present but we were unable to allocate a buffer.
>
> Other than the message change, no functional change intended.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
> drivers/pci/pci.c | 27 ++++++++++++++-------------
> drivers/pci/pcie/aspm.c | 15 ++++++++-------
> 2 files changed, 22 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 869d204a70a3..503376bf7e75 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1686,10 +1686,8 @@ static int pci_save_pcie_state(struct pci_dev *dev)
> return 0;
>
> save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
> - if (!save_state) {
> - pci_err(dev, "buffer not found in %s\n", __func__);
> + if (!save_state)
> return -ENOMEM;
> - }
>
> cap = (u16 *)&save_state->cap.data[0];
> pcie_capability_read_word(dev, PCI_EXP_DEVCTL, &cap[i++]);
> @@ -1742,19 +1740,17 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
>
> static int pci_save_pcix_state(struct pci_dev *dev)
> {
> - int pos;
> struct pci_cap_saved_state *save_state;
> + u8 pos;
> +
> + save_state = pci_find_saved_cap(dev, PCI_CAP_ID_PCIX);
> + if (!save_state)
> + return -ENOMEM;
>
> pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> if (!pos)
> return 0;
>
> - save_state = pci_find_saved_cap(dev, PCI_CAP_ID_PCIX);
> - if (!save_state) {
> - pci_err(dev, "buffer not found in %s\n", __func__);
> - return -ENOMEM;
> - }
> -
> pci_read_config_word(dev, pos + PCI_X_CMD,
> (u16 *)save_state->cap.data);
>
> @@ -1763,14 +1759,19 @@ static int pci_save_pcix_state(struct pci_dev *dev)
>
> static void pci_restore_pcix_state(struct pci_dev *dev)
> {
> - int i = 0, pos;
> struct pci_cap_saved_state *save_state;
> + u8 pos;
> + int i = 0;
> u16 *cap;
>
> save_state = pci_find_saved_cap(dev, PCI_CAP_ID_PCIX);
> - pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> - if (!save_state || !pos)
> + if (!save_state)
> return;
> +
> + pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
> + if (!pos)
> + return;
> +
> cap = (u16 *)&save_state->cap.data[0];
>
> pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index e0bc90597dca..007e4a082e6f 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -35,16 +35,14 @@ void pci_save_ltr_state(struct pci_dev *dev)
> if (!pci_is_pcie(dev))
> return;
>
> + save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
> + if (!save_state)
> + return;
> +
> ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
> if (!ltr)
> return;
>
> - save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
> - if (!save_state) {
> - pci_err(dev, "no suspend buffer for LTR; ASPM issues possible after resume\n");
> - return;
> - }
> -
> /* Some broken devices only support dword access to LTR */
> cap = &save_state->cap.data[0];
> pci_read_config_dword(dev, ltr + PCI_LTR_MAX_SNOOP_LAT, cap);
> @@ -57,8 +55,11 @@ void pci_restore_ltr_state(struct pci_dev *dev)
> u32 *cap;
>
> save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_LTR);
> + if (!save_state)
> + return;
> +
> ltr = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_LTR);
> - if (!save_state || !ltr)
> + if (!ltr)
> return;
>
> /* Some broken devices only support dword access to LTR */
>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
--
i.
© 2016 - 2025 Red Hat, Inc.