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

Farhan Ali
2026-02-09T18:20:38Z

Hi Bjorn,

Thanks for reviewing!

On 2/6/2026 4:39 PM, Bjorn Helgaas wrote:
> On Thu, Jan 22, 2026 at 11:44:31AM -0800, 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.
>>
>> 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>
>> ---
>> drivers/pci/pci.c | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index c105e285cff8..e7beaf1f65a7 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -4960,6 +4960,7 @@ EXPORT_SYMBOL_GPL(pci_dev_unlock);
>>
>> static void pci_dev_save_and_disable(struct pci_dev *dev)
>> {
>> + u32 val;
>> const struct pci_error_handlers *err_handler =
>> dev->driver ? dev->driver->err_handler : NULL;
>>
>> @@ -4980,6 +4981,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)) {
>> + pci_warn(dev, "Device config space inaccessible\n");
>> + return;
>> + }
> This seems related to Lukas' recent patches:
>
> a2f1e22390ac ("PCI/ERR: Ensure error recoverability at all times")
> 5e09895b4063 ("Documentation: PCI: Amend error recovery doc with pci_save_state() rules")
>
> My poor understanding is that the PCI core now saves config space for
> every device at enumeration time, and if a driver wants to capture an
> updated config space after it has changed things, it is responsible
> for doing that explicitly.
>
> a2f1e22390ac gives us a baseline saved state that will be restored in
> some cases. This pci_dev_save_and_disable() is part of the reset path
> and saves a potentially different state. I'm a little uncomfortable
> about the fact that we save the state at different times, including
> unpredictable times after an error, and I'm not sure the driver can
> always know what gets restored.
>
> Maybe the reset path shouldn't even try to save config space again,
> since we're now guaranteed to have at least the state from
> enumeration? I suppose skipping the save here would expose cases
> where the driver changed config space without doing a pci_save_state()
> itself, and a driver- or sysfs-initiated reset would lose that change,
> whereas today we preserve it because we save/restore as part of that
> reset.
>
> That change would also be lost if the reset was unintentional, e.g.,
> during error recovery, but I guess in that case the driver does know
> that an error occurred, so it could redo the change.
>
> It seems like the ideal thing would be if every restore used the
> enumeration saved-state or the driver's intentional saved-state, never
> a state saved at the unpredictable time of an error recovery reset.

Maybe it does makes sense to not save the config space again here. Since
we have a baseline saved state with commit a2f1e22390ac, removing the
saving of config space here would restore the baseline state unless a
driver saved its most recent state. So now we would have a working saved
state when we restore.

However my fear is drivers may have come to rely on this to save/restore
its latest updated state and we could break some drivers?

Thanks

Farhan

>
> I hope Lukas and Alex can chime in here.
>
>> pci_save_state(dev);
>> /*
>> * Disable the device by clearing the Command register, except for
>> --
>> 2.43.0
>>