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

[Bjorn Helgaas <helgaas@kernel.org>]

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.