Re: [PATCH v12 2/7] PCI: Avoid saving config space state if inaccessible

[Farhan Ali <alifm@linux.ibm.com>]

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:
>>> On Mon, Mar 30, 2026 at 10:40:06AM -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). 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@linux.ibm.com>
>>>> Reviewed-by: Niklas Schnelle <schnelle@linux.ibm.com>
>>>> Reviewed-by: Bjorn Helgaas <bhelgaas@google.com>
>>>> ---
>>>>    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;
>>> I wonder if it's feasible to do the pci_save_state() into a temporary
>>> 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.
>> I just want to clarify, are you suggesting we do that in pci_save_state()
>> 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.
> This is kind of a blue-sky idea for exploration, so maybe it's not
> 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