Re: [PATCH v12 2/7] PCI: Avoid saving config space state if inaccessible
From: Farhan Ali
Date: Tue Apr 07 2026 - 18:02:58 EST
On 4/7/2026 2:32 PM, Bjorn Helgaas wrote:
On Tue, Apr 07, 2026 at 02:17:07PM -0700, Farhan Ali wrote:
On 4/7/2026 12:56 PM, Bjorn Helgaas wrote:This is kind of a blue-sky idea for exploration, so maybe it's not
On Mon, Mar 30, 2026 at 10:40:06AM -0700, Farhan Ali wrote:I just want to clarify, are you suggesting we do that in pci_save_state()
The current reset process saves the device's config space state beforeI wonder if it's feasible to do the pci_save_state() into a temporary
reset and restores it afterward. However errors may occur unexpectedly and
it may then be impossible to save config space because the device may be
inaccessible (e.g. DPC). This results in saving invalid values that get
written back to the device during state restoration.
With a reset we want to recover/restore the device into a functional state.
So avoid saving the state of the config space when the device config space
is inaccessible.
Signed-off-by: Farhan Ali <alifm@xxxxxxxxxxxxx>
Reviewed-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
Reviewed-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
---
drivers/pci/pci.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 70162f15a72c..b36263834289 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -722,6 +722,27 @@ u16 pci_find_dvsec_capability(struct pci_dev *dev, u16 vendor, u16 dvsec)
}
EXPORT_SYMBOL_GPL(pci_find_dvsec_capability);
+static bool pci_dev_config_accessible(struct pci_dev *dev, char *msg)
+{
+ u32 val;
+
+ /*
+ * If device's config space is inaccessible it can return ~0 for
+ * any reads. Since VFs can also return ~0 for Device and Vendor ID
+ * check Command and Status registers. Note that this is racy
+ * because the device may become inaccessible partway through
+ * next access.
+ */
+ pci_read_config_dword(dev, PCI_COMMAND, &val);
+ if (PCI_POSSIBLE_ERROR(val)) {
+ pci_warn(dev, "Device config space inaccessible; unable to %s\n",
+ msg);
+ return false;
buffer, check the buffer for PCI_POSSIBLE_ERROR(), and copy the temp
buffer into the real buffer if we think the save was successful. I
know this is a lot more work, but I would like to avoid the raciness
if possible.
function? If not then we need to do something similar to pci_save_state()
and then check for errors. At that point wouldn't it just make sense to add
the check in places where we save the bits the kernel wants? Please correct
me if I misunderstood you.
practical. I think it have to be done inside pci_save_state(). It
would be a little messy to implement since the pci_cap_saved_state
structures are in the save_cap_space list, not all in one place. So I
think we'd have to allocate a duplicate list for this purpose.
Yeah this would require quite a bit of work and would need some refactoring on how we save state today. I would prefer to avoid doing that as part of this series. I also think doing this for checking FLR (the following patch in this series) would be an overkill?
Just trying to think out loud (and exploring the idea further), I wonder how expensive it would be then to save the state as we would need to check all the bits the kernel cares about?
Thanks
Farhan