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

[Niklas Schnelle <schnelle@linux.ibm.com>]

On Mon, 2025-12-01 at 14:08 -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 the device may become inaccessible or the config space itself 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/corrupted.
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>

I think the commit message needs more focus. Specifically I think the
main point is the case that Lukas mentioned in the following quote from
the cover letter of his "PCI: Universal error recoverability of
devices" series:

"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. So it must be saved ahead of time."

That case will inevitably happen when state save / reset happens while
a PCI device is in the error state on a platform like s390, POWER, or
with DPC where Config Space will be inaccessible.

Moreover, I'd like to stress that this is an issue independent from the
rest of your series. As we've seen in your experiments this can be
triggered today when a vfio-pci user process blocks recovery, e.g. by
not handling the eventfd, and then the user tries to mitigate the
situation by performing a reset through sysfs, which then saves the
0xff bytes from inaccessible config space which may subsequently kill
the device on restore.

> ---
>  drivers/pci/pci.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 608d64900fee..28c6b9e7f526 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -5105,6 +5105,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;
>  
> @@ -5125,6 +5126,12 @@ static void pci_dev_save_and_disable(struct pci_dev *dev)
>  	 */
>  	pci_set_power_state(dev, PCI_D0);
>  
> +	pci_read_config_dword(dev, PCI_COMMAND, &val);
> +	if (PCI_POSSIBLE_ERROR(val)) {
> +		pci_warn(dev, "Device config space inaccessible\n");
> +		return;
> +	}
> +

Can you explain your reasoning for not using pci_channel_offline()
here? This was suggested by Lukas in a previous iteration (link below)
and I would tend to prefer that as well.

https://lore.kernel.org/all/aOZoWDQV0TNh-NiM@wunner.de/

>  	pci_save_state(dev);
>  	/*
>  	 * Disable the device by clearing the Command register, except for