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

[Farhan Ali]

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@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;
> 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.

Thanks

Farhan

> > +	}
> > +
> > +	return true;
> > +}
> > +
> >   /**
> >    * pci_find_parent_resource - return resource region of parent bus of given
> >    *			      region
> > @@ -5029,6 +5050,9 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
> >   	 */
> >   	pci_set_power_state(dev, PCI_D0);
> >   
> > +	if (!pci_dev_config_accessible(dev, "save state"))
> > +		return;
> > +
> >   	pci_save_state(dev);
> >   	/*
> >   	 * Disable the device by clearing the Command register, except for
> > --
> > 2.43.0