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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Returning early from `pci_dev_save_and_disable()` when the config space is inaccessible skips the crucial hardware disablement step.
- [Medium] The variable `val` in `pci_dev_config_accessible()` is uninitialized and the return value of `pci_read_config_dword()` is ignored.
- [Low] The `pci_dev_config_accessible` function accepts a `char *msg` parameter but is called with a read-only string literal (`"save state"`).
--

commit 5aedd0a5cc35c507762b05ea728c194c780f2934
Author: Farhan Ali <alifm@linux.ibm.com>

PCI: Avoid saving config space state if inaccessible

This commit avoids saving an invalid config space state when a device
becomes inaccessible, such as during a DPC event. It introduces a check
to read the PCI_COMMAND register and skips saving the state if it returns
an error response.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index d0c9f0166af5c..e71b814153921 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)

This isn't a bug, but could msg be typed as const char *?

The string literal "save state" is passed to this function, which discards
the const qualifier.

> +{
> +	u32 val;

Does this code risk leaving val uninitialized if pci_read_config_dword()
fails?

If the function fails without modifying the output pointer, val will contain
stack garbage. Since it wouldn't necessarily equal ~0 (PCI_ERROR_RESPONSE),
could this lead the kernel to incorrectly assume the config space is
accessible?

> +
> +	/*
> +	 * 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)) {
[ ... ]
> @@ -5027,6 +5048,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;

By returning early here, does this skip the subsequent
pci_write_config_word() call?

If the link recovers right before the reset takes full effect, could the
device be left with DMA and legacy interrupts enabled, potentially leading
to memory corruption or interrupt storms?

Should this bypass pci_save_state() but still attempt the disablement
write?

> +
>  	pci_save_state(dev);
>  	/*
>  	 * Disable the device by clearing the Command register, except for
>  	 * INTx-disable which is set.  This not only disables MMIO and I/O port
>  	 * BARs, but also prevents the device from being Bus Master, preventing
>  	 * DMA from the device including MSI/MSI-X interrupts.  For PCI 2.3
>  	 * compliant devices, INTx-disable prevents legacy interrupts.
>  	 */
>  	pci_write_config_word(dev, PCI_COMMAND, PCI_COMMAND_INTX_DISABLE);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514180059.1553-1-alifm@linux.ibm.com?part=2