Re: [PATCH v11 3/9] PCI: Avoid saving config space state if inaccessible

Farhan Ali
2026-03-24T22:38:43Z

On 3/24/2026 2:40 PM, Bjorn Helgaas wrote:
> On Mon, Mar 16, 2026 at 12:15:38PM -0700, Farhan Ali wrote:
>> The current reset process saves the device's config space state before
>> 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) or config space may be corrupted. This results in
>> saving corrupted values that get written back to the device during state
>> restoration.
> This patch only addresses the "inaccessible" part, so I'd drop the
> "config space may be corrupted" because we aren't checking for that.

Sure, I will update the commit message.

>> 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@linux.ibm.com>
>> Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>

Thanks for reviewing!

>> ---
>> drivers/pci/pci.c | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index a93084053537..373421f4b9d8 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -5014,6 +5014,7 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
>> {
>> const struct pci_error_handlers *err_handler =
>> dev->driver ? dev->driver->err_handler : NULL;
>> + u32 val;
>>
>> /*
>> * dev->driver->err_handler->reset_prepare() is protected against
>> @@ -5033,6 +5034,19 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
>> */
>> pci_set_power_state(dev, PCI_D0);
>>
>> + /*
>> + * 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. At the very least we should
>> + * avoid restoring config space for device with error bits set in
>> + * Status register.
>> + */
>> + pci_read_config_dword(dev, PCI_COMMAND, &val);
>> + if (PCI_POSSIBLE_ERROR(val)) {
> Obviously this is still racy because the device may become
> inaccessible partway through saving the state, and it might be worth
> acknowledging that in the comment. But I think this is an improvement
> over what we do now.

Yeah, makes sense. Will update the comment. How about something like:

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. This can still be racy as a device
can become inaccessible partway through saving the state, even after this
check.

>
> Sashiko complains about this, but I think it's mainly because of that
> last sentence of the comment that says "error bits set in Status
> register". This patch has to do with *saving*, not restoring, so I'd
> just drop that last sentence. FWIW, Sashiko said:
>
> The comment indicates that we should avoid restoring config space
> for a device with error bits set in the Status register, but the
> code only uses PCI_POSSIBLE_ERROR(val).
>
> Since PCI_POSSIBLE_ERROR() only evaluates whether the entire 32-bit
> value is exactly 0xFFFFFFFF (indicating complete device
> inaccessibility), does this actually check for individual error
> flags in the Status register?
>
> If a device logs an error but is still accessible, val will reflect
> those bits but will not equal 0xFFFFFFFF, causing the check to
> evaluate to false. Should this code also check specific bits in the
> upper 16 bits of val (such as PCI_STATUS_SIG_SYSTEM_ERROR or
> PCI_STATUS_DETECTED_PARITY) to match the stated intention in the
> comment?

Yup will drop the last line.

Thanks

Farhan

>
>> + pci_warn(dev, "Device config space inaccessible\n");
>> + return;
>> + }
>> +
>> pci_save_state(dev);
>> /*
>> * Disable the device by clearing the Command register, except for
>> --
>> 2.43.0
>>